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

Rules on save #105

Open
wants to merge 13 commits into
base: staging
Choose a base branch
from
Open

Rules on save #105

wants to merge 13 commits into from

Conversation

andrepereiradasilva
Copy link
Owner

@andrepereiradasilva andrepereiradasilva commented Nov 13, 2016

Pull Request for Issue # and improvements.

Summary of Changes

This PR makes several changes to the ACL system on save.

  • Adds new methods to JAccess API (JAccessRules::setAction/JAccessRule::setIdentities/JAccessRule::setIdentity) to set the rules (instead of merging), this is needed because a merge of the inhrited rule (fallback to parent rule) is different than a adding/replacing an inhrited rule (empty).
  • Reviews ACL ajax save to use the JAccess API
  • Stop saving parent assets rules when creating a new article. Each article should be created with a empty asset rule.
  • Don't save empty action (ex: core.edit) rules, because of the inhrited acl system, there no need to store those in the database and we also don't need to process them in JAccessRules/JAccessRule
  • Don't allow to save the permissions if there is no asset in the database for that item, ask the user to save first to generate the new asset. This allows to calculated the correct parent id.

Note: There is a very slight performance/memory improvement on acl checks, this only when there are lots of similiar ACL checks. Since no action with empty rule are saved now, there will be more identical rules and so more use the JAccess rules and identities static cache. Also less action to process.

Testing Instructions

Please test with caution. This is not a fast test.

Pre-requisite: Code review and confirm no B/C issues, then:

  • First apply patch [ACL] JAccess bug fixes and optimizations [B/C alternative proposal] joomla/joomla-cms#12850 (if not merged yet)
  • Apply this patch
  • Now, for faster testing, you need two browser windows, one with the db #__assets table opened (order by latest id) and other with joomla backend.
  • Do the new/edit test for each item type with ACL (article, category, menu, module, component, global config):
    • Create a new item, edit the acl (without saving), you shouldn't be allowed to.
    • Save the item, confirm a new ACL rule is created in the assets table.
    • Now edit the item, edit the acl (without saving), confirm the ACL rule is edited in the assets table and it's corrected - change several rules to be sure.
    • Save the item, confirm the ACL staus the same.
    • Now delete the ACL rule row from the database table.
    • Edit the item again, try to edit the acl (without saving), you shouldn't be allowed to.
    • Save the item, confirm a new ACL rule is creted.
  • Do the edit test for component options ACL:
    • Open any component options, edit the acl (without saving) and confirm it's working
    • Delete component acl record and then edit the ACL and confirm a new component ACL is created with the values you edit.
    • Save the component options and confirm ACL rules are ok.
  • Do the global config ACL test:
    • Edit the acl and confirm it's working
    • Save the global config options and confirm ACL rules are ok.

The ACL system needs to be tested as a whole.
So after those tests do a general review of ACL to confirm all works fine.

Documentation Changes Required

None.

andrepereiradasilva pushed a commit that referenced this pull request Dec 24, 2018
Now that I haave seen this in action I realise that we have to add the word "urgent" otherwise it will confuse people that the count doesnt match the total number of requests
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.

1 participant