Backend logic has leaked into the TPAC (and friends)

Bug #1347774 reported by Mike Rylander
12
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Evergreen
Confirmed
Wishlist
Unassigned

Bug Description

Over the years, a good bit of logic that is arguably the concern of middle layer APIs has leaked into the mod_perl code supporting the TPAC and others. This mod_perl code is really a client of the backend, and not an appropriate place to implement business logic that requires access to backend services, so let's push that code out into backend services where we benefit from the ability to scale, handle failures more transparently, and decrease security risks of client-level access to unauthenticated services.

Part of this will be the creation of an "anonymous" mode for pcrud, which will be able to replace cstore calls that would not, in pcrud, require permissions of any kind. These are a fairly common form of logic leakage.

Additionally, uses of json_query, and authentication-wrapped cstore calls, will be pushed into appropriate backend services such as cat, circ, search, etc.

Tags: opac
Revision history for this message
Mike Rylander (mrylander) wrote :

Here's a branch containing an initial anonymous mode for pcrud.

If the auth token passed is exactly "ANONYMOUS" and there are no permission restrictions attached to the retrieve action for the object, and on of the 'retrieve', 'search', or 'id_list' methods was called, allow the data to flow.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/miker/lp-1347774-anon_pcrud_mode

Revision history for this message
Mike Rylander (mrylander) wrote :

I have pushed some more commits to this branch that:

1) attempt to create a pcrud personality for CStoreEditor
2) enable that personality when loaded by EGWeb and EGCatLoader

Note: this will break existing code! Disable the changes in EGWeb and EGCatLoader to unbreak it. I added those to expose "bad" uses of CStoreEditor.

Revision history for this message
Bill Erickson (berick) wrote :

I've pushed a few small fixes and an anon-pcrud live test to the collab branch. At this point, I believe the next steps are to start working through the tpac explosions and porting the json_query's (etc.) to API calls.

Revision history for this message
Bill Erickson (berick) wrote :

I pushed a few more fixes and 2 json_query call migrations. An unauthenticated home page of the catalog now appears w/o explosion. So, there's that. Will continue through other pages as I can. If anyone else wants to pitch in, just comment here.

Revision history for this message
Bill Erickson (berick) wrote :

Search results page mostly working now. more todo.

Revision history for this message
Bill Erickson (berick) wrote :

I take my previous comments back about some pages working. In my initial tests, I was still giving Apache access to the private domain. When I plopped it onto the public domain, a lot of new issues crop up. Here's a general recap of outstanding TODO's:

* opensrf.settings is accessed directly in a number of cases. XSL paths, added content configuration, xml/rpc configuration, vandalay configuration. These will all have to be moved to public API's or some other internal, Apache-accessible configuration mechanism, like eg_vhost.conf.
  ** Note, I've already pushed a branch to avoid direct opensrf.settings calls to lookup the IDL path with an API-driven version.

* There are numerous instances of direct use of open-ils.cstore (not via CStoreEditor) throughout the WWW code. Those will have to be ported to CStoreEditor where applicable or otherwise replaced with public APIs.

* TPAC manages the main authenticated user via some cstore-only code within CStoreEditor, e.g. $e->allowed. The PCRUD personality will need a few additions to continue in that manner. $e->checkauth is fine (uses open-ils.auth), so we might only need to modify $e->allowed to fall back to using a public perm check API when needed.

* Porting all json_query's to similar pcrud-supported searches or public APIs.

* Misc. Other stuff will pop up that we haven't thought of.

Revision history for this message
Bill Erickson (berick) wrote :

One thing to add to the misc. pile:

OpenSRF::Utils::Cache is used throughout WWW/*. Cache gets its settings (server, default timeout) from opensrf.settings. Two possible solutions:

1. copy the cache settings into eg_vhost.conf (some precedent w/ OSRFTranslatorCacheServer) and modify OpenSRF::Utils::Cache to allow the cache settings to be passed into the constructor.

2. move the cache settings from opensf.xml into opensrf_core.xml, so they're accessible to all clients.

Revision history for this message
Mike Rylander (mrylander) wrote :

Another option, which pushes this well outside the 2.7 realm (as if it weren't there already) is to secure opensrf.settings such that the clients can't ask for any old chunk of the file. One way this might be done is to inspect the domain portion of the jid of the client, and if it's domain does not match that of the settings server itself then only deliver data from the <default> section, excising <apps> entirely for this purpose. Then the settings server could potentially register in the public domain.

Of course, there's nothing saying we can't do this manually today, by running a separate settings service whose native domain is public, and whose opensrf.xml is trimmed to what in-apache clients need. That wouldn't require anything new, but would increase the initial setup burden.

More thinking required for either of those options...

Revision history for this message
Bill Erickson (berick) wrote :

Removing milestone, since there is a significant amount of work yet to do on this one.

Changed in evergreen:
milestone: 2.7.0-beta1 → none
Ben Shum (bshum)
Changed in evergreen:
milestone: none → 2.next
status: New → Confirmed
Revision history for this message
Bill Erickson (berick) wrote :

Pushed a re-tested and squashed version of anon-pcrud mode *only* code:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1347774-anon-pcrud

My commit still needs a sign-off.

Adding pullrequest for this ^-- branch only for now.

tags: added: pullrequest
Revision history for this message
Mike Rylander (mrylander) wrote :

Signed off and picked into master. 2nd off the hack-a-way, right behind csharp's first Perl addition.

tags: removed: pullrequest
Revision history for this message
Bill Erickson (berick) wrote :

As a reminder for future civilizations reviewing this bug, some features were added and released, but the key issues remain. I'm removing the milestone since there is no active development on this at the moment.

Changed in evergreen:
milestone: 2.next → none
tags: added: opac
removed: tpac
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.