limited usage of finalizePermissionCheck method
Details
- Reviewers
alex
Part 1 - test that no permission redirect happens when user has no permissions, and not happens when user has permissions for action
- change code of the core/admin_templates/languages/email_template_list.tpl - remove <inp2:m_if check="m_IsDebugMode"> check around Add` button
- go to User Management → Administrators
- add standard administrator user by entering only required fields - Password and E-mail
- log into adm.console with non-root administrative user
- switch off debug mode
- go to Website & Content → E-mail Templates
- press Add button
- confirm that happened redirect to the No Permission page
- go to Website & Content → E-mail Templates
- choose some template in the E-mail Templates grid and press Edit button
- 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
- login as non-root administrator user
- go to Website & Content → Structure & Data → Products
- go to Products tab
- press Tools -> Export button
- confirm that opened "Products Export" window without any permissions errors
- go to Tools → Query Database section
- run SQL like DELETE FROM inp_Permissions WHERE Permission = 'in-portal:main_import.view'
- go to Website & Content → Structure & Data → Products
- go to Products tab
- press Tools -> Export button
- confirm that No Permission page is shown
Part 3 - tests that no permissions redirect happens correctly after changes in CategoriesEventHandler::CheckPermission
- login as non-root administrator user
- go to Tools → System Tools
- press Reset button in the section Front-end, in subsection Reset SMS Menu Cache
- confirm, that operation is succesful with message "Last operation has been successfully completed!" and without any permissions errors
- go to Tools → Query Database section
- execute SQL like DELETE FROM inp_Permissions WHERE Permission = 'in-portal:service.edit'
- go to Tools → System Tools
- press Reset button in the section Front-end, in subsection Reset SMS Menu Cache
- confirm that No Permission page is shown
Part 4 - tests that no permissions redirect happens correctly after changes in AdminEventsHandler::CheckPermission
- login as non-root administrator user
- go to Tools → Query Database
- press Run SQL button in the toolbar
- confirm that page refreshes without any permission errors
- go to Tools → Query Database section
- execute SQL like DELETE FROM inp_Permissions WHERE Permission = 'in-portal:sql_query.view'
- press Run SQL button in the toolbar
- confirm that No Permission page is shown
Part 5 - tests that no permissions redirect happens correctly after changes in ContentEventHandler::CheckPermission
- login as non-root administrator user
- go to Browse Website
- click on Content Mode tab
- confirm, that page with Edit Content button is shown
- click on Edit Content button
- confirm that opened "Content Editor" window without any permission errors
- go to Tools → Query Database
- execute SQL like UPDATE inp_Permissions SET PermissionValue = 0 WHERE Permission = 'CATEGORY.MODIFY';
- go to Browse Website
- click on Content Mode tab
- confirm, that page with Edit Content button is shown
- click on Edit Content button
- 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
- log in to adm.console
- go to User Management → Users
- add standard member user
- go to Website & Content → Structure & Data → Directory
- create some link in the adm.console, under Directory category, with recent front-end user as owner
- login with created front-end user to front-end
- 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
- confirm that page refreshes without any permission errors
- login with another front-end user to front-end
- 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
- 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
- log in as non root administrator user
- create some link, confirm, that link creation is successful, without "no permission" redirect
- open to edit category, in which link was created
- 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
- try create some link under same category as previous link was created
- 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
- log in as non root administrator user
- create some category in the administrative console, with current logged in user as owner
- confirm that "Up" / "Down" buttons works properly in the administrative console categories grid with just created category, checked for these actions
- ensure log-out status on front-end
- 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
- 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
- log in as non root administrator user
- create some category under "Directory" category, confirm, that category creation is successful, without "no permission" redirect
- open to edit "Directory" category (in which link was created)
- 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
- try create some category under "Directory" category
- 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
- log in to adm.console
- go to User Management → Users
- add standard member user
- go to Website & Content → Structure & Data → Directory
- create some link in the adm.console, under Directory category, with recent front-end user as owner
- go to User Management → Users
- press Login As button to open front-end with new user logged in
- go to My Links page (url like http://localhost/5.2.x/in-link/my_account/my_links.html)
- confirm that recently created link is visible on page with Details, Modify, Delete links under it
- press modify link
- confirm that link edit form is opened
- press Update button in the link edit form
- confirm, that link is successfully updated
- go to My Links page (url like http://localhost/5.2.x/in-link/my_account/my_links.html)
- confirm that recently created link is visible on page with Details, Modify, Delete links under it
- press modify link
- confirm that link edit form is opened
- in the another tab/window open adm.console
- open parent category of tested link (Directory) for editing
- go to tab Permissions, disable for group member LINK.OWNER.MODIFY and LINK.OWNER.MODIFY.PENDING permissions, save
- return to front-end tab/window
- press Update button in the link edit form
- 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
- 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
- change modules/in-link/units/links/links_event_handler.php - add 'OnDeleteFile' => array('self' => 'delete'), line to the array in the mapPermissions method
- log in to adm.console
- go to User Management → Users
- add standard member user
- go to Website & Content → Structure & Data → Directory
- create some link in the adm.console, under Directory category, with recent front-end user as owner
- go to User Management → Users
- press Login As button to open front-end with new user logged in
- go to My Links page (url like http://localhost/5.2.x/in-link/my_account/my_links.html)
- confirm that recently created link is visible on page with Details, Modify, Delete links under it
- press modify link
- confirm that link edit form is opened
- in the another tab/window open adm.console
- open parent category of tested link (Directory) for editing
- go to tab Permissions, disable for group member LINK.DELETE permission, save
- return to front-end tab/window
- upload some file by using swf-upload control
- make visible ajax requests in the browser window
- uncheck checkbox besides uploaded file name
- confirm file deletion by pressing OK in the javascript confirmation window
- confirm, that ajax request happened
- 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
- log into adm.console with non-root administrative user
- go to Website & Content → Links
- add two links with same "Link URL" value
- go to Directory → Duplicate Checker
- confirm that duplicate link is shown in the grid
- double click on the link in the grid
- confirm, that is opened window with grid of two links with equal "Link URL" value
- double click on one of the two links in the grid, shown in the previous step
- confirm, that "Editing Link ..." window is opened
- change value in link's "Name" field
- press "Save" button
- confirm, that link is successfully updated
- modify modules/in-link/admin_templates/duplicate_checker/duplicate_link_view.tpl file - replace "std_edit_item" with "std_edit_temp_item"
- go to Directory → Duplicate Checker
- confirm that duplicate link is shown in the grid
- double click on the link in the grid
- confirm, that is opened window with grid of two links with equal "Link URL" value
- double click on one of the two links in the grid, shown in the previous step
- confirm, that "Editing Link ..." window is opened
- change value in link's "Name" field
- press "Save" button
- confirm, that link is successfully updated
- 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
- log into adm.console with non-root administrative user
- go to Configuration → Website → Advanced
- make "Enable Revision Control for Section Content" checkbox checked, press "Save" button
- go to Website & Content → Browse Website
- press "Content Mode" tab
- edit content block on "Home" page, save
- confirm, that become enabled "Save" button in the revision management toolbar
- press that "Save" button in the revision management toolbar
- confirm that green title "Viewing Revision ... (published)" appeared in the revision management toolbar
- press "Section Properties" button to edit "Home" category
- 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
- edit content block on "Home" page, save
- confirm, that become enabled "Save" button in the revision management toolbar
- press that "Save" button in the revision management toolbar
- 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
- go to Tools → Query Database section
- 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
- log into adm.console with non-root administrative user
- go to Configuration → Website → Advanced
- make "Enable Revision Control for Section Content" checkbox checked, press "Save" button
- go to Website & Content → Browse Website
- press "Content Mode" tab
- press "Section Properties" button to edit "Home" category
- 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
- edit content block on "Home" page, save
- confirm, that become enabled "Save" button in the revision management toolbar for new revision "Editing Draft (pending)"
- press that "Save" button in the revision management toolbar
- press "History" button in the revision management toolbar
- confirm that appeared revisions list, where on the top is new created pending revision
- choose that revision from list by mouse click
- confirm, that in the revision management toolbar become enabled "Publish" and "Decline" buttons
- press "Publish" button in the revision management toolbar
- confirm that pending revision is successfully published
- edit content block on "Home" page, save
- confirm, that become enabled "Save" button in the revision management toolbar for new revision "Editing Draft (pending)"
- press that "Save" button in the revision management toolbar
- press "History" button in the revision management toolbar
- confirm that appeared revisions list, where on the top is new created pending revision
- choose that revision from list by mouse click
- confirm, that in the revision management toolbar become enabled "Publish" and "Decline" buttons
- open same in-portal system in the another browser window
- go to Tools → Query Database section
- 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
- return to the browser window with revision management toolbar
- press "Publish" button in the revision management toolbar
- confirm that No Permission page is shown
Diff Detail
- Repository
- rINP In-Portal
- Branch
- /in-portal/branches/5.2.x
- Lint
Lint Errors Excuse: Full file re-formatting is not part of this task. Severity Location Code Message Error core/units/helpers/permissions_helper.php:839 PHPCS.E.CodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBrace CodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBrace - Unit
No Unit Test Coverage - Build Status
Buildable 383 Build 383: arc lint + arc unit
Event Timeline
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 | ||
---|---|---|
155–163 |
|
core/kernel/managers/request_manager.php | ||
---|---|---|
155–163 | Yes - benefit is that in new version event handler object is not recalled for root user. |
core/kernel/managers/request_manager.php | ||
---|---|---|
155–163 | 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. |
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.
Adding comment to trigger automatic link creation between Phabricator and JIRA (was broken after last upgrade of Phabricator).
core/kernel/db/cat_event_handler.php | ||
---|---|---|
120–121 | 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–111 | 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 | ||
133 | Please improve test plan or specify plan part, that lead to this code execution. Seems not to be covered in test plan. | |
238 | Please improve test plan or specify plan part, that lead to this code execution. Seems not to be covered in test plan. | |
271 | Please improve test plan or specify plan part, that lead to this code execution. Seems not to be covered in test plan. | |
390 | 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 | ||
37–40 | Please improve test plan or specify plan part, that lead to this code execution. Seems not to be covered in test plan. | |
46 | Please improve test plan or specify plan part, that lead to this code execution. Seems not to be covered in test plan. |
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–121 | 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–111 | 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 | ||
133 | It happens when somebody is pressing add button in the grid and OnPreCreate/OnNew event is mapped to add permission. | |
238 | 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. | |
271 | 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. | |
390 | 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 | ||
37–40 | 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. | |
46 | 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). |
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. |
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.
core/units/helpers/permissions_helper.php | ||
---|---|---|
133 | 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). |