PKP Bugzilla – Full Text Bug Listing
|Summary:||Refactor all HandlerValidators into authorization policies (across all apps)|
|Component:||General||Assignee:||Alec Smecher <alec>|
|Version Reported In:||Also Affects:||OCS 2.3.4, OHS 2.3.2, OJS 2.3.4, OMP 1.0|
Description jerico 2010-09-01 22:57:02 PDT
Todos moved over from #5540 which will have to wait for OJS 2.3.3 to be released. * Specify permissions for all apps similar to the spreadsheet that defines permissions for OMP, see https://spreadsheets.google.com/ccc?key=tw_MobQ-ZwFI1deqxdnxiIA * 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 but maintain a deprecated Validation class for backwards compatibility This is a migration task which is also listed here: http://pkp.sfu.ca/wiki/index.php/Migration_issues#Replacing_HandlerValidators_with_AuthorizationPolicies_.28Florian.29
Comment 1 jerico 2010-09-01 23:09:18 PDT
• 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)
Comment 2 jerico 2010-09-01 23:12:44 PDT
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 3 jerico 2010-09-02 08:04:07 PDT
Execute the following commands within the OMP repository to retrieve sample diffs/see examples for this migration: 1) Sample policy (this shows how complex policies will look like): http://github.com/pkp/omp/blob/master/classes/security/authorization/OmpWorkflowStageAccessPolicy.inc.php 2) Sample component: http://github.com/pkp/ojs/blob/master/controllers/grid/citation/CitationGridHandler.inc.php - see constructor for role assignment - see authorize() method for policy assignment - see missing validate() method 3) Page handlers are secured in the exact same way as the sample component.
Comment 4 Alec Smecher 2013-05-29 16:05:14 PDT
Probably baked into 3.0 but worth reviewing before release.
Comment 5 Jason Nugent 2013-06-20 10:45:02 PDT
Remove reliance on HandlerValiatorJournal and use ContextRequired policy instead https://github.com/pkp/ojs/commit/ce5da64c0ad672768585f9ccd342297759970e4f
Comment 6 Jason Nugent 2013-06-21 06:46:02 PDT
remove unused SubmissionComment validator https://github.com/pkp/ojs/commit/0de0f9586a54cf55685a3df978fa3dad88cb4188
Comment 7 Jason Nugent 2013-06-21 07:34:02 PDT
remove instances of HandlerValidatorCustom from OJS, introduce OjsJournalMustPublish policy https://github.com/pkp/ojs/commit/fdfafdee80f35e669705614b5c5f3fa895b8b29f
Comment 8 Jason Nugent 2013-06-26 12:13:02 PDT
remove un-necessary variable https://github.com/pkp/ojs/commit/2b927037ce8387192758c2dfb781cb2570f91b0a
Comment 9 Jason Nugent 2013-06-27 09:59:02 PDT
Comment 10 Jason Nugent 2013-06-27 10:10:02 PDT
remove context requirement from policy https://github.com/pkp/ojs/commit/7b86481270ae85d21d28568495217164a8656b36
Comment 11 Alec Smecher 2015-03-31 10:19:28 PDT
Pulls: https://github.com/pkp/pkp-lib/pull/445 https://github.com/pkp/ojs/pull/462 https://github.com/pkp/omp/pull/112 Jason, could you review these?
Comment 12 Jason Nugent 2015-03-31 11:38:29 PDT
Reviewed, merge away, Alec.
Comment 13 Alec Smecher 2015-03-31 11:43:24 PDT
Thanks, Jason! Also removed PKPHandler::getLoginExemptions. Closing.