Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed frontend module editing permissions. #30778

Merged
merged 1 commit into from Oct 30, 2020

Conversation

Harmageddon
Copy link
Contributor

@Harmageddon Harmageddon commented Sep 26, 2020

Version: Joomla! 3.x

In #6113, the permission check for editing a module in frontend was checked from "return error if user is not allowed to edit this module OR if user is not allowed to edit any module" to "return error if user is not allowed to edit this module AND if user is not allowed to edit any module". The intention was to allow users to edit a single module even if they are lacking the general permission to edit modules in frontend.
However, this introduces a problem for the inverse case: A user that generally may edit frontend modules, but should not be allowed to edit one particular module. For this case, the "OR" construction worked and the "AND" doesn't.

Summary of Changes

I suggest to get rid of the check of the general permission. If there are no permission rules for the particular module, Joomla's ACL has an automatic fallback to the general permissions for frontend module editing. So I don't see any need to check both rules. Please correct me if I'm mistaken!

Testing Instructions

For the frontend steps, you need a user who is no "Super Administrator", but in another user group, for example "Administrator". For the backend steps, use your "Super Administrator" account or at least an account who has the permissions to edit permissions.

  1. Backend: Navigate to "Extensions - Modules - Options". Under "Permissions", set the "Frontend Editing" permission for the "Administrator" user group to "inherited". This should result to a calculated permission of "Not Allowed (Inherited)".
  2. Backend: Edit a module. In the "Permissions" tab, set the "Frontend Editing" permission for "Administrator" to "Allowed". Hit "Save and Close".
  3. Frontend: Make sure you can edit this particular module and no other module. While editing the module, save the URL of the edit form for later usage. Change something and hit "Save and Close".
  4. Backend: Navigate to "Extensions - Modules - Options". Under "Permissions", set the "Frontend Editing" permission for the "Administrator" user group to "Allowed".
  5. Frontend: Make sure you can edit and save all modules on the site.
  6. Backend: Edit a module. In the "Permissions" tab, set the "Frontend Editing" permission for "Administrator" to "Denied". Hit "Save and Close".
  7. Frontend: Make sure that there is no button to edit this particular module. For all others however, the button is there. Make sure you still can edit all other modules by changing values and saving them.
  8. Frontend: Nevertheless, try to access the module edit form using the URL you saved in step 3. Change some values and hit "Save and Close".

Actual result BEFORE applying this Pull Request

Although you shouldn't be allowed to do this, you can edit the module in step 8.

Expected result AFTER applying this Pull Request

Step 8 should result in a "You are not allowed to view this resource" error. All other steps should still work like before (e.g. follow the ACL permissions).

Documentation Changes Required

None

@richard67
Copy link
Member

@Harmageddon Could you merge latest staging of the cms repo into your branch for this PR to get the latest updates from the CMS? There was an error in the staging branch for a while which is fixed now, and this error made all system and unit tests fail, so we can't really see if they would be successful for your PR or not. Thanks in advance.

@Harmageddon Harmageddon force-pushed the fix_frontend_module_permissions branch from c5554d5 to fd18571 Compare October 1, 2020 09:13
@Harmageddon
Copy link
Contributor Author

I rebased it and the tests passed. Thank you!

@ghost
Copy link

ghost commented Oct 10, 2020

I have tested this item ✅ successfully on fd18571

Perfect test instructions!


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30778.

@ChristineWk
Copy link

I have tested this item ✅ successfully on fd18571

Phew :-) Thanks for your instructions!


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30778.

@richard67
Copy link
Member

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30778.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 24, 2020
@rdeutz rdeutz merged commit a907b4c into joomla:staging Oct 30, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 30, 2020
@rdeutz rdeutz added this to the Joomla! 3.9.23 milestone Oct 30, 2020
zero-24 pushed a commit that referenced this pull request Nov 1, 2020
* syncing

* revert incorrectly changed files

* adding incorrectly ignored file.
@N6REJ N6REJ mentioned this pull request Nov 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants