Bug 5540 - Implement new authorization approach that works for AJAX components and page handlers.
Implement new authorization approach that works for AJAX components and page ...
Status: RESOLVED FIXED
Product: OMP
Classification: Unclassified
Component: Framework
1.1
PC Linux
: P3 normal
Assigned To: jerico
: 5237 (view as bug list)
Depends on: 5557
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-01 11:06 PDT by jerico
Modified: 2010-09-02 14:43 PDT (History)
3 users (show)

See Also:
Version Reported In:
Also Affects: OCS 2.3.3, OHS 2.3.1, OJS 2.3.2


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description jerico 2010-07-01 11:06:26 PDT
Collect all information necessary to implement authorization, then implement it. This includes a review of the current authorization implementation.
Comment 1 jerico 2010-07-10 00:52:29 PDT
Committed a large part of this change, see:
http://github.com/pkp/omp/commit/364a5a95cdfc24b682b57910080cd5dd33972e25
http://github.com/pkp/omp/commit/053b2ad9fc0d9fdeeb83889a8ce660305f7e6b98
http://github.com/pkp/omp/commit/279ca6ec98da483c1f45a7c35b131f6228d97c87
http://github.com/pkp/omp/commit/73eaeb5393f468a7f87fc39edb12056ef6323fe6
http://github.com/pkp/omp/commit/1191e5627f5dc79b84b32c49a2521a0b66dd0887
http://github.com/pkp/pkp-lib/commit/3e55fdc7ff620cb1ea560b073bc6a55c852f86cd
http://github.com/pkp/pkp-lib/commit/dba617bf045dd5712d77cce6e0c4f8a4e80725dc
http://github.com/pkp/pkp-lib/commit/d2232a4613abdf8c29ec263eaa0e4bcf477a7e1f
http://github.com/pkp/pkp-lib/commit/d3850e517b457f29f1650d3c78a7bb3f7ae696e7
http://github.com/pkp/pkp-lib/commit/3eb1b879c07e7c225552be44b0493787e441aec2
http://github.com/pkp/pkp-lib/commit/2af1e51a6747b7585a67c7738791f37f5a6e9f3f
http://github.com/pkp/pkp-lib/commit/47a7bfe6305cbfbbda0cef7ac9dce0f27b848c1a

What is still missing...

Remaining refactoring:
* add authorization policies to author/AuthorHandler|SubmitHandler, reviewer/ReviewerHandler (+add role assignments, + remove validate() calls)
* refactor all HandlerValidator* classes in all apps to the new policy classes
* create a deprecation message whenever a HandlerValidator is instantiated or whenever a check is being found in PKPHandler::validate()/addCheck(), etc.
* remove PKPHandler::getLoginExemptions() and make HandlerOperationRestrictSiteAccessPolicy::getLoginExemptions() private
* remove inclusion of HandlerValidators in PKPHandler class once all handler validators have been removed.
* refactor "Validation" class into "Authentication" class

Improve and consolidate authorization framework...
* Retrieve authorization messages (and other advice) from the decision manager and display them in the router.
* make sure policy sets can have advice (introduce a common base class with authorization policy) and that this advice is being read in the decision manager.
* push $request into AuthorizationRequest and introduce a getRequest() method that all sub-classes can use to avoid the current request-related code duplication

Testing:
* check authorization in PHP4 to see whether we don't have the instantiation state problem (e.g. authorization decision manager or policies added to it could be lost on instantiation)
* write lots of tests for the new code (also look at PKPComponentRouterTest, that one will now fail due to the removed getRemoteOperations() method)

Documentation:
* Add sample patches to this bug report.
** a main policy (workflow stage policy)
** a handler (setup handler + manager handler)
** a component (e.g. sponsors grid)
* Update authorization framework wiki page.
* Update migration wiki page.
** Add links to patches to the migration wiki
* Insert BugZilla entries for remaining migration work.

When you refactor code yourself you can use the following refactoring checklist which contains several checkpoints that avoid introduction of difficult-to-identify vulnerabilities:
- Don't do ad-hoc policy design. Make sure you first have a full permission specification for the app you're refactoring (see the OMP permission spec as an example).
- Make sure you transform validate() method calls into authorization policies (unless you really want to check non-fatal data integrity issues in the validate() method).
- Remove all validate() calls in the class once you've moved the validate() code to policies.
- Make sure that you have a single policy in the authorize() method that contains all other policies without duplicating code from already existing policies.
- Make sure getRemoteOperations() has been removed everywhere and that a corresponding addRoleAssignment() has been added to the constructor
- Make sure that you also include operations from inherited classes (e.g. "fetch", "fetchGrid", etc.)
- Make sure that the $roleAssignment parameter is passed into all authorize() calls and from there into all policies. Follow the inheritance hierarchy of the policy to see whether it's correctly passing the role assignment on to the role policies.
- Make sure that all policy constructor args are passed in in the right order.
- Make sure that the authorize() method has the right signature.
- Make sure that the parent::authorize() method is being called (go up along the inheritance hierarchy until reaching PKPHandler)
- Make sure that the parent::authorize() value is actually being returned along the chain (go up the hierarchy again) and back into the router.
- Make sure that you have all necessary authorization context objects made available to the component in the initialize() method.
- Remove import statements to HandlerValidator.
Comment 2 jerico 2010-07-10 01:07:14 PDT
And one more commit: http://github.com/pkp/ojs/commit/3ebcaf0b5af15c5bcaf03cd50fc43baaeed0cef9
Comment 4 jerico 2010-07-13 13:46:25 PDT
Now passing $args to validate() and initialize() as well, see: http://github.com/pkp/pkp-lib/commit/ebf023a4cefe1c3237b287bf24d5524794f2f0d9
Comment 6 jerico 2010-07-13 18:54:49 PDT
Fixed a few authorization bugs in FilterGridHandler and CitationGridHandler:
- see http://github.com/pkp/ojs/commit/53eab3d5b57a03a6fe0e676d6e30c658c3f4373c and
pkp-lib 6777d74cd03f2ac55aad1699c92ec8f06c7909d9 (github temporarily offline, no link)
Comment 7 jerico 2010-07-27 16:59:03 PDT
We also have to refactor the folder hierarchy for validation:
- Policy sets that are meant to be used in components or pages should be put into folders that are named after the main page, e.g. submission, workflow, etc. 
- Policy sets or policies that are meant to be used as sub-policies only should remain in the authorization folder.
Comment 8 Alec Smecher 2010-07-27 17:07:59 PDT
Florian, I think OJS 2.3.3 will be affected by this, not OJS 2.3.2 -- OJS 2.3.2 is the forthcoming incremental release and won't include the authorization changes.
Comment 9 jerico 2010-07-27 18:39:08 PDT
Sorry, I didn't see the version in the end. In Eclipse I cannot see the whole line. It's cut off. Can you maybe change the long names to abbreviations? That would make the "also affected" list usable in Eclipse.
Comment 10 Alec Smecher 2010-07-27 18:46:58 PDT
Florian, I've changed the "Also Affected" values to use abbreviated names -- but I'm not sure what the other field you're seeing is. Does Eclipse appear to prepend the product name to the version number?
Comment 11 jerico 2010-07-27 19:02:29 PDT
No, no other field involved. It was only about the "also affected" field. I thought that was the one I got wrong?
Comment 12 jerico 2010-08-24 15:27:36 PDT
*** Bug 5237 has been marked as a duplicate of this bug. ***
Comment 13 jerico 2010-08-24 15:30:05 PDT
Adding a mail exchange with Alec for documentation:

Florian Grandel wrote:
> Hi Alec and Juan,
>
> I'd like to rename the "Validation" class into "Authentication" and move
> it to .../security/authentication.
>
> 1) Do you agree this makes sense?
> 2) Is it probable that anyone outside PKP uses that class? Is it
> necessary that I write a compatibility class for that one as well?
>
> I'll move everything that is authorization related to
> .../security/authorization btw.

Hi Florian,

Just to be on the safe side, I'd prefer to include a deprecated Validation class. FYI, it's probably a good idea to start filing bugs for the removal of some of these deprecated pieces of code. Just file them against 2.x for now, and we can figure out a plan when we set the release schedule post-2.3.2.

Thanks!
Alec
Comment 14 jerico 2010-08-30 18:36:28 PDT
Cleaned up all policies in OMP (reduced number of policies, improved spec), see http://github.com/pkp/omp/commit/9de8944a009a226400edff0dc6c752b7d7e2390e
Comment 15 jerico 2010-09-01 23:13:24 PDT
Comments re remaining todos:
* "Remaining refactoring" has been posted as migration task #5868
* "Improve and consolidate authorization framework" has been implemented or is irrelevant by now, except for the authorization message stuff which is now in #5869
* "Testing" has been moved to #5870 and #5871
* "Documentation"  has moved to #5868 (for sample migration patches), the authorization framework and migration wiki pages have been updated and bugzilla entried have been created for all remaining work.

I'm closing this entry then as the core framework is now in place and fully functional/documented.
Comment 16 jerico 2010-09-02 14:43:06 PDT
Renamed to better reflect the cross app impact.

Fixed a few inconsistencies in pkp-lib and omp:
http://github.com/pkp/pkp-lib/commit/a38b689c9b7444f53a0db8c335cc18390376a80d
http://github.com/pkp/omp/commit/48fd042006e1ee6f47285ed946149420c11279be

Introduced the correct nomenclature and folder structure in ojs:
http://github.com/pkp/ojs/commit/f37e53c6ac8123a8b621ad651d16e2ac2dd48611