Unexpected output: Affected paths: relative - resubmit patch.
⚙ D222 INP-1327 Simplify custom permission checking in Event Handler
Page MenuHomeIn-Portal Phabricator
Differential D222

INP-1327 Simplify custom permission checking in Event Handler
Needs ReviewPublic
Actions

Authored by erik on May 3 2016, 4:17 AM.

Details

Reviewers
alex
Summary

limited usage of finalizePermissionCheck method

Test Plan

Part 1 - test that no permission redirect happens when user has no permissions, and not happens when user has permissions for action

  1. change code of the core/admin_templates/languages/email_template_list.tpl - remove <inp2:m_if check="m_IsDebugMode"> check around Add` button
  2. go to User Management → Administrators
  3. add standard administrator user by entering only required fields - Password and E-mail
  4. log into adm.console with non-root administrative user
  5. switch off debug mode
  6. go to Website & Content → E-mail Templates
  7. press Add button
  8. confirm that happened redirect to the No Permission page
  9. go to Website & Content → E-mail Templates
  10. choose some template in the E-mail Templates grid and press Edit button
  11. confirm that opened Editing E-mail Template page without any permission errors

Part 2 - tests that no permissions redirect happens correctly after changes in kCatDBEventHandler::CheckPermission

  1. login as non-root administrator user
  2. go to Website & Content → Structure & Data → Products
  3. go to Products tab
  4. press Tools -> Export button
  5. confirm that opened "Products Export" window without any permissions errors
  6. go to Tools → Query Database section
  7. run SQL like DELETE FROM inp_Permissions WHERE Permission = 'in-portal:main_import.view'
  8. go to Website & Content → Structure & Data → Products
  9. go to Products tab
  10. press Tools -> Export button
  11. confirm that No Permission page is shown

Part 3 - tests that no permissions redirect happens correctly after changes in CategoriesEventHandler::CheckPermission

  1. login as non-root administrator user
  2. go to Tools → System Tools
  3. press Reset button in the section Front-end, in subsection Reset SMS Menu Cache
  4. confirm, that operation is succesful with message "Last operation has been successfully completed!" and without any permissions errors
  5. go to Tools → Query Database section
  6. execute SQL like DELETE FROM inp_Permissions WHERE Permission = 'in-portal:service.edit'
  7. go to Tools → System Tools
  8. press Reset button in the section Front-end, in subsection Reset SMS Menu Cache
  9. confirm that No Permission page is shown

Part 4 - tests that no permissions redirect happens correctly after changes in AdminEventsHandler::CheckPermission

  1. login as non-root administrator user
  2. go to Tools → Query Database
  3. press Run SQL button in the toolbar
  4. confirm that page refreshes without any permission errors
  5. go to Tools → Query Database section
  6. execute SQL like DELETE FROM inp_Permissions WHERE Permission = 'in-portal:sql_query.view'
  7. press Run SQL button in the toolbar
  8. confirm that No Permission page is shown

Part 5 - tests that no permissions redirect happens correctly after changes in ContentEventHandler::CheckPermission

  1. login as non-root administrator user
  2. go to Browse Website
  3. click on Content Mode tab
  4. confirm, that page with Edit Content button is shown
  5. click on Edit Content button
  6. confirm that opened "Content Editor" window without any permission errors
  7. go to Tools → Query Database
  8. execute SQL like UPDATE inp_Permissions SET PermissionValue = 0 WHERE Permission = 'CATEGORY.MODIFY';
  9. go to Browse Website
  10. click on Content Mode tab
  11. confirm, that page with Edit Content button is shown
  12. click on Edit Content button
  13. confirm that No Permission page is shown

Part 6 - tests that no permissions redirect happens when somebody trying to edit link owned by different user

  1. log in to adm.console
  2. go to User Management → Users
  3. add standard member user
  4. go to Website & Content → Structure & Data → Directory
  5. create some link in the adm.console, under Directory category, with recent front-end user as owner
  6. login with created front-end user to front-end
  7. go to link like http://localhost/5.2.x/index.php?events[l]=OnMassMoveUp&l[1][LinkId]=on where 1 is id of created link
  8. confirm that page refreshes without any permission errors
  9. login with another front-end user to front-end
  10. go to link like http://localhost/5.2.x/index.php?events[l]=OnMassMoveUp&l[1][LinkId]=on where 1 is id of created link
  11. confirm that No Permission page is shown

Part 7 - tests that no permissions redirect happens when somebody trying to edit category item (link) under category, where links creation is disabled

  1. log in as non root administrator user
  2. create some link, confirm, that link creation is successful, without "no permission" redirect
  3. open to edit category, in which link was created
  4. on permissions tab of category edit form disable "LINK.ADD" and "LINK.ADD.PENDING" permissions (uncheck "Inherited" and "Access" checkboxes for "Add Link" and "Pending Link" permissions), press Save button
  5. try create some link under same category as previous link was created
  6. confirm, that after pressing Save while adding new link happens "no permission" redirect

Part 8 - tests that no permissions redirect happens when somebody trying to edit category owned by different user

  1. log in as non root administrator user
  2. create some category in the administrative console, with current logged in user as owner
  3. confirm that "Up" / "Down" buttons works properly in the administrative console categories grid with just created category, checked for these actions
  4. ensure log-out status on front-end
  5. go to link like http://localhost/5.2.x/index.php?events[c]=OnMassMoveUp&c[139][CategoryId]=on where 139 is id of created category
  6. confirm that No Permission page is shown

Part 9 - tests that no permissions redirect happens when somebody trying to edit category item (link) under category, where links creation is disabled

  1. log in as non root administrator user
  2. create some category under "Directory" category, confirm, that category creation is successful, without "no permission" redirect
  3. open to edit "Directory" category (in which link was created)
  4. on permissions tab of category edit form disable "CATEGORY.ADD" and "CATEGORY.ADD.PENDING" permissions (uncheck "Inherited" and "Access" checkboxes for "Add Category" and "Add Pending Category" permissions), press Save button
  5. try create some category under "Directory" category
  6. confirm, that after pressing Save while adding new category happens "no permission" redirect

Part 10 - tests that no permissions redirect happens when somebody is is trying to edit a category item and .OWNER. permission is turned off for primary category of that category item

  1. log in to adm.console
  2. go to User Management → Users
  3. add standard member user
  4. go to Website & Content → Structure & Data → Directory
  5. create some link in the adm.console, under Directory category, with recent front-end user as owner
  6. go to User Management → Users
  7. press Login As button to open front-end with new user logged in
  8. go to My Links page (url like http://localhost/5.2.x/in-link/my_account/my_links.html)
  9. confirm that recently created link is visible on page with Details, Modify, Delete links under it
  10. press modify link
  11. confirm that link edit form is opened
  12. press Update button in the link edit form
  13. confirm, that link is successfully updated
  14. go to My Links page (url like http://localhost/5.2.x/in-link/my_account/my_links.html)
  15. confirm that recently created link is visible on page with Details, Modify, Delete links under it
  16. press modify link
  17. confirm that link edit form is opened
  18. in the another tab/window open adm.console
  19. open parent category of tested link (Directory) for editing
  20. go to tab Permissions, disable for group member LINK.OWNER.MODIFY and LINK.OWNER.MODIFY.PENDING permissions, save
  21. return to front-end tab/window
  22. press Update button in the link edit form
  23. confirm that No Permission page is shown

Part 11 - tests that no permissions redirect happens when somebody is is trying delete swf-uploaded file while editing category item and .DELETE permission is turned off for primary category of that category item

  1. change themes/advanced/in-link/links/modify_link.tpl - use inp_edit_swf_upload block in place of inp_edit_file_upload to upload files
  2. change modules/in-link/units/links/links_event_handler.php - add 'OnDeleteFile' => array('self' => 'delete'), line to the array in the mapPermissions method
  3. log in to adm.console
  4. go to User Management → Users
  5. add standard member user
  6. go to Website & Content → Structure & Data → Directory
  7. create some link in the adm.console, under Directory category, with recent front-end user as owner
  8. go to User Management → Users
  9. press Login As button to open front-end with new user logged in
  10. go to My Links page (url like http://localhost/5.2.x/in-link/my_account/my_links.html)
  11. confirm that recently created link is visible on page with Details, Modify, Delete links under it
  12. press modify link
  13. confirm that link edit form is opened
  14. in the another tab/window open adm.console
  15. open parent category of tested link (Directory) for editing
  16. go to tab Permissions, disable for group member LINK.DELETE permission, save
  17. return to front-end tab/window
  18. upload some file by using swf-upload control
  19. make visible ajax requests in the browser window
  20. uncheck checkbox besides uploaded file name
  21. confirm file deletion by pressing OK in the javascript confirmation window
  22. confirm, that ajax request happened
  23. confirm, that response of ajax request contain "no permission" redirect code like /no_permission.html?next_template=external

Part 12 - test that no permissions redirect happens when somebody is trying to save changes to a category item, that is being edited in live table

  1. log into adm.console with non-root administrative user
  2. go to Website & Content → Links
  3. add two links with same "Link URL" value
  4. go to Directory → Duplicate Checker
  5. confirm that duplicate link is shown in the grid
  6. double click on the link in the grid
  7. confirm, that is opened window with grid of two links with equal "Link URL" value
  8. double click on one of the two links in the grid, shown in the previous step
  9. confirm, that "Editing Link ..." window is opened
  10. change value in link's "Name" field
  11. press "Save" button
  12. confirm, that link is successfully updated
  13. modify modules/in-link/admin_templates/duplicate_checker/duplicate_link_view.tpl file - replace "std_edit_item" with "std_edit_temp_item"
  14. go to Directory → Duplicate Checker
  15. confirm that duplicate link is shown in the grid
  16. double click on the link in the grid
  17. confirm, that is opened window with grid of two links with equal "Link URL" value
  18. double click on one of the two links in the grid, shown in the previous step
  19. confirm, that "Editing Link ..." window is opened
  20. change value in link's "Name" field
  21. press "Save" button
  22. confirm, that link is successfully updated
  23. confirm that No Permission page is shown

Part 13 - test that no permissions redirect happens when somebody is trying to add draft revision with lack of necessary permission

  1. log into adm.console with non-root administrative user
  2. go to Configuration → Website → Advanced
  3. make "Enable Revision Control for Section Content" checkbox checked, press "Save" button
  4. go to Website & Content → Browse Website
  5. press "Content Mode" tab
  6. edit content block on "Home" page, save
  7. confirm, that become enabled "Save" button in the revision management toolbar
  8. press that "Save" button in the revision management toolbar
  9. confirm that green title "Viewing Revision ... (published)" appeared in the revision management toolbar
  10. press "Section Properties" button to edit "Home" category
  11. on permissions tab of category edit form disable "CATEGORY.REVISION.ADD" and "CATEGORY.REVISION.ADD.PENDING" permissions (uncheck "Inherited" and "Access" checkboxes for "Allow Adding Content Revisions" and "Allow Adding Pending Content Revisions" permissions), press Save button
  12. edit content block on "Home" page, save
  13. confirm, that become enabled "Save" button in the revision management toolbar
  14. press that "Save" button in the revision management toolbar
  15. confirm that No Permission page is shown

Part 14 - test that no permissions redirect happens when somebody is trying to approve draft revision with lack of necessary permission

  1. go to Tools → Query Database section
  2. run SQL like: "REPLACE INTO inp_Permissions (Permission, GroupId, PermissionValue, Type, CatId) VALUES ('CATEGORY.REVISION.MODERATE', 11, 1, 0, 0);" to add revision moderation permission for admins
  3. log into adm.console with non-root administrative user
  4. go to Configuration → Website → Advanced
  5. make "Enable Revision Control for Section Content" checkbox checked, press "Save" button
  6. go to Website & Content → Browse Website
  7. press "Content Mode" tab
  8. press "Section Properties" button to edit "Home" category
  9. on permissions tab of category edit form disable "CATEGORY.REVISION.ADD" and enable "CATEGORY.REVISION.ADD.PENDING" permissions (uncheck "Inherited" and "Access" checkboxes for "Allow Adding Content Revisions" and check "Access" checkbox for "Allow Adding Pending Content Revisions" permissions), press Save button
  10. edit content block on "Home" page, save
  11. confirm, that become enabled "Save" button in the revision management toolbar for new revision "Editing Draft (pending)"
  12. press that "Save" button in the revision management toolbar
  13. press "History" button in the revision management toolbar
  14. confirm that appeared revisions list, where on the top is new created pending revision
  15. choose that revision from list by mouse click
  16. confirm, that in the revision management toolbar become enabled "Publish" and "Decline" buttons
  17. press "Publish" button in the revision management toolbar
  18. confirm that pending revision is successfully published
  19. edit content block on "Home" page, save
  20. confirm, that become enabled "Save" button in the revision management toolbar for new revision "Editing Draft (pending)"
  21. press that "Save" button in the revision management toolbar
  22. press "History" button in the revision management toolbar
  23. confirm that appeared revisions list, where on the top is new created pending revision
  24. choose that revision from list by mouse click
  25. confirm, that in the revision management toolbar become enabled "Publish" and "Decline" buttons
  26. open same in-portal system in the another browser window
  27. go to Tools → Query Database section
  28. run SQL like: "REPLACE INTO inp_Permissions (Permission, GroupId, PermissionValue, Type, CatId) VALUES ('CATEGORY.REVISION.MODERATE', 11, 0, 0, 0);" to remove revision moderation permission for admins
  29. return to the browser window with revision management toolbar
  30. press "Publish" button in the revision management toolbar
  31. confirm that No Permission page is shown

Diff Detail

Repository
rINP In-Portal
Branch
/in-portal/branches/5.3.x
Lint
Lint ErrorsExcuse: Full file re-formatting is not part of this task.
SeverityLocationCodeMessage
Errorcore/units/helpers/permissions_helper.php:847PHPCS.E.CodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBraceCodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBrace
Unit
No Unit Test Coverage
Build Status
Buildable 382
Build 382: arc lint + arc unit

Event Timeline

erik updated this revision to Diff 522.May 3 2016, 4:17 AM
erik retitled this revision from to INP-1327 Simplified custom permission checking in Event Handler.
erik updated this object.
erik edited the test plan for this revision. (Show Details)
erik added a reviewer: alex.
erik added 1 JIRA issue(s): INP-1327.
alex requested changes to this revision.May 3 2016, 4:38 AM
alex edited edge metadata.

Test plan should also include scenarios for testing each changed code fragment (e.g. where finalizePermissionCheck was called, but is no more) to demonstrate that in case of permission violation proper No Permission page is displayed.

This is called regression testing.

core/kernel/managers/request_manager.php
149–157
  1. Why this change was made?
  2. Does this give any benefits compared to original code?
This revision now requires changes to proceed.May 3 2016, 4:38 AM
erik added inline comments.May 5 2016, 4:08 AM
core/kernel/managers/request_manager.php
149–157

Yes - benefit is that in new version event handler object is not recalled for root user.

alex added inline comments.May 5 2016, 4:15 AM
core/kernel/managers/request_manager.php
149–157

But what I think you're not aware of however is fact, that later on, when kApplication::HandleEvent is executed (after successful permission check, always for root) the EventHandler object is created anyway to run that event.

Knowing that I ask you to revert this change.

alex edited edge metadata.May 5 2016, 5:55 AM

Actually please create this patch against 5.2.x branch. The changes doesn't look as breaking ones and 5.2.x developers would be happy.

alex added a comment.May 5 2016, 6:22 AM

Adding comment to trigger automatic link creation between Phabricator and JIRA (was broken after last upgrade of Phabricator).

erik updated this revision to Diff 523.May 11 2016, 4:47 AM
erik edited edge metadata.

Patch transformed to 5.2.x version.

erik edited the test plan for this revision. (Show Details)May 11 2016, 4:49 AM
erik edited edge metadata.
alex requested changes to this revision.May 11 2016, 5:19 AM
alex edited edge metadata.
alex added inline comments.
core/kernel/db/cat_event_handler.php
120–122

Please improve test plan or specify plan part, that lead to this code execution.

Seems not to be covered in test plan.

128

Please improve test plan or specify plan part, that lead to this code execution.

Seems not to be covered in test plan.

core/units/categories/categories_event_handler.php
110–112

Please improve test plan or specify plan part, that lead to this code execution.

Seems not to be covered in test plan.

118

Please improve test plan or specify plan part, that lead to this code execution.

Seems not to be covered in test plan.

core/units/helpers/permissions_helper.php
137

Please improve test plan or specify plan part, that lead to this code execution.

Seems not to be covered in test plan.

241

Please improve test plan or specify plan part, that lead to this code execution.

Seems not to be covered in test plan.

279

Please improve test plan or specify plan part, that lead to this code execution.

Seems not to be covered in test plan.

398

Please improve test plan or specify plan part, that lead to this code execution.

Seems not to be covered in test plan.

core/units/page_revisions/page_revision_eh.php
36–38

Please improve test plan or specify plan part, that lead to this code execution.

Seems not to be covered in test plan.

42

Please improve test plan or specify plan part, that lead to this code execution.

Seems not to be covered in test plan.

This revision now requires changes to proceed.May 11 2016, 5:19 AM
alex added a comment.May 11 2016, 5:49 AM

To help you out I've pointed out places that should trigger all code paths not covered by test plan. You still need to very that it all works like this and write step-by-step test plan to execute changed code of course.

core/kernel/db/cat_event_handler.php
120–122

It happens when somebody trying to edit/delete category item (e.g. link) owned by different user.

128

It happens when somebody trying to edit/delete category item (e.g. link) owned by current user.

core/units/categories/categories_event_handler.php
110–112

It happens when somebody trying to edit/delete category owned by different user.

118

It happens when somebody trying to edit/delete category owned by current user.

core/units/helpers/permissions_helper.php
137

It happens when somebody is pressing add button in the grid and OnPreCreate/OnNew event is mapped to add permission.

241

It happens on Front-End only, when somebody is is trying to edit a category item and .OWNER. permission is defined (turned off or on) for primary category of that category item.

279

It happens on Front-End only, when somebody is is trying to edit a category item and .PENDING permission is defined (turned off or on) for primary category of that category item.

398

It happens in Admin Console only, when somebody is trying to save changes to a category item, that is being edited in temp table.

core/units/page_revisions/page_revision_eh.php
36–38

It happens in Admin Console only with Enable Revision Control for Section Content system setting turned on, when creating new draft revision as result of changing content block translation.

42

It happens in Admin Console only with Enable Revision Control for Section Content system setting turned on, when creating publishing/discarding a page revision from toolbar (left top side on Front-End).

erik added inline comments.May 16 2016, 5:11 AM
core/kernel/db/cat_event_handler.php
128

We must have permission error there, to test proper redirect, but this $perm_value is always true in my tests.

erik edited the test plan for this revision. (Show Details)May 16 2016, 5:12 AM
erik edited edge metadata.
alex added a comment.May 17 2016, 2:41 AM

Since permission system is affected please also include tests that would demonstrate, that if user has needed permissions no redirect happens and whatever should happen (upon user taking action) will happen.

erik edited the test plan for this revision. (Show Details)May 17 2016, 4:56 AM
erik added inline comments.May 19 2016, 4:54 AM
core/units/helpers/permissions_helper.php
137

This code is executed in the part 1 of the plan (1 .. 8 subparts of the part 1). As I understand, there $perm_status may have only false value (true case is returned from cycle, before this line executes).

erik edited the test plan for this revision. (Show Details)May 19 2016, 4:55 AM
erik edited the test plan for this revision. (Show Details)May 20 2016, 4:57 AM
erik edited the test plan for this revision. (Show Details)May 26 2016, 4:30 AM
erik edited the test plan for this revision. (Show Details)Jun 13 2016, 4:29 AM
erik edited the test plan for this revision. (Show Details)Jun 14 2016, 4:18 AM
erik edited the test plan for this revision. (Show Details)Jun 15 2016, 4:53 AM
erik edited the test plan for this revision. (Show Details)Jun 16 2016, 4:54 AM
erik edited the test plan for this revision. (Show Details)Jun 20 2016, 4:44 AM
erik added a comment.Jun 20 2016, 4:48 AM

Done - test plan improved.

erik requested a review of this revision.Jun 20 2016, 5:01 AM
erik edited edge metadata.
alex retitled this revision from INP-1327 Simplified custom permission checking in Event Handler to INP-1327 Simplify custom permission checking in Event Handler.Jul 16 2016, 7:30 AM
alex edited edge metadata.

Revision Contents
Changeset List

DiffIDBaseDescriptionCreatedLintUnit
BaseBase
Diff 152216347May 3 2016, 4:17 AM
★
★
Diff 252316347Patch transformed to 5.2.x version.May 11 2016, 4:47 AM
★
★

Diff 522

View Options

core/kernel/db/cat_event_handler.php

Loading...
View Options

core/kernel/managers/request_manager.php

Loading...
View Options

core/units/categories/categories_event_handler.php

Loading...
View Options

core/units/content/content_eh.php

Loading...
View Options

core/units/helpers/permissions_helper.php

Loading...
View Options

core/units/page_revisions/page_revision_eh.php

Loading...
In-Portal CMS · In-Portal Community Pages · In-Portal Issue Tracker · In-Portal Repository Browser · Quality Assurance · In-Portal CI Server