[ACL] Don't explicit add the parent asset permissions on creating a new item + other issues

[andrepereiradasilva]

Pull Request for ACL Issues (part 2).

Summary of Changes

What this PR does:

• Stop explicit writing the parent asset permissions on creating a new item (module, article, etc). Now all are created with Inhrited in all groups/actions so they Inhrited them.
• Displays the correct calculated permissions in item <-> section(s) <-> component <-> global config scenario.
• On create a new item (module, article, etc) and not saving the Permmission tab calculated are now the component ones.
• Further simplifies the ACL rules code to make it more undertstandable.
• Solves some other minor bugs.

To get the explicit item ACL permission in a item <-> section(s) <-> component <-> global config scenario i had to change JAccess::getAssetRules() behaviour.

Mantainers before merging please confirm this is correct and will not have side effects.

Information: ACL Inheritance Scenarios

There are four ACL Asset Inheritance scenarios:

• item <-> section(s) <-> component <-> global config
  (ex: article <-> category <-> com_content <-> global config)
• item <-> component <-> global config
  (ex: category <-> com_content <-> global config)
• component <-> global config
  (ex: com_content <-> global config)
• global config (no asset inheritance)

Besides inheriting from the asset(s), ACL also inherits from parent User Group(s).

Mantainers please confirm this are the scenarios at play.

Testing Instructions

Pre-requisites

Use latest staging.
Clean install.

Test 1: Testing default ACL Inheritance on creating a new iitem

This tests creating a new item without explicit inhreting permissions

• Create a new category "Test ACL" category, don't change anything and save
• Edit the category
• Check category permissions are inhreited form com_config + com_content
• Check there is no explict "Allowed" or "Denied" permission.
• Change one permission to "Denied" and exit
• Now create a new article assign to "Test ACL" category, don't change anything and save
• Now edit the article
• Check the com_config + com_content + category permissions are all inhrited as they should.
• Check there is no explict "Allowed" or "Denied" permission.

Note: You can also check that before the patch the item(s) have explicity "Allowed" or "Denied" permission on create.

Test 2: Testing ACL asset/groups Inheritance

This test should be made in sequence.

1: This tests Global config child groups Inherit

• Go to Global config
• Change Public "Delete" to Denied
• Change Public "Edit" to Allowed
• Refresh the page.
• Change Registered "Edit state" to Denied
• Change Registered "Edit Own" to Allowed
• Refresh the page
• Check all Global config Permissions are all inhrited as they should.

2: This tests Global config -> Component Inherit

• Go to Articles (com_content) Config
• Check the com_config permission are all inhrited as they should.
• Change Author "Edit Own" to Denied
• Refresh the page
• Check the child groups permissions are all inhrited as they should.

3: This tests Global config -> Component -> Item Inherit

• Create new category, name it "Test ACL" don't change anything and save
• Now edit the category
• Check the com_config + com_content permissions are all inhrited as they should.
• Change Editor "Edit" to Denied
• Refresh the page
• Check the child groups permissions are all inhrited as they should.

4: This tests Global config -> Component -> Section -> Item Inherit

• Create a new article assign to "Test ACL" category, don't change anything and save
• Now edit the article
• Check the com_config + com_content + category permissions are all inhrited as they should.
• Change Manager "Edit" to Denied
• Refresh the page
• Check the child groups permissions are all inhrited as they should.

You can and should make other tests to confirm all is ok.

Test 3: code review

Do a code review.

Still known issues (that will not be solved by this PR)

1: When creating a new item (not saving) it uses the calculated permissions from the component (item <-> component <-> global config).
But if we have a section too (item <-> section(s) <-> component <-> global config) this is not correct.
This is a incorrect info bug.

2: In "1:", it uses the component permission, but should use the calculated permissions for achild of the component/section.
This is a incorrect info bug.

3: If a component as a permission that doesn't exists in global config (ex: frontend editing in com_modules) by default we get "Not Allowed (Inherited)" when we should get "Not Allowed (Default)".
In resume, if doesn't exist in the parent asset it can't Inherit from it.
This is a incorrect info bug.

4: When changing a permission of an item that doesn't have a row in the asset table the row a new row is created.
This works fine for item <-> component <-> global config scenario and component <-> global config scenario.
But doesn't work properly for item <-> section(s) <-> component <-> global config scenario because a wrong parent asset id (the component) is stored.
This is a incorrect ACL bug, happens when there is no row in the asset table (ex: deleted or not created on update).

Note: This known issues are marked in comments as todo in the code.

@infograf768 @roland-d thanks for all your help on checking this.
And please check.

[roland-d]

I have tested this item ✅ successfully on 58adad0

All tests are successful. Inheritance across children works as described. Inheritance across item -> section -> component -> global level works as expected. Super User status is shown correct as well.

The 4 described scenarios are the ones we have in play as far as I can see. Although a 5th scenario could be added that is for Super Users as they don't play by any rule but their own ;)

Job well done.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10894.}

[infograf768]

We could set it RTC but better, as for the other one, get more testers on this.

[ggppdk]

@infograf768

If you would wait a day and i would like to test this too

[pritalpatel]

I have tested Test 1 and Test 2 -> 1 without applying patch and found it is already working. using the staging branch.

[Bodge-IT]

Also tested but didn't verify as wasn't clear on difference before or after patch, seemed to be same. Now @pritalpatel has confirmed my suspicion and quelled my confusion.

Think test script:
4: This tests Global config -> Component -> Extension -> Item Inherit

Should read:
4: This tests Global config -> Component -> Section -> Item Inherit

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10894.}

[andrepereiradasilva]

@pritalpatel @Bodge-IT yes Test 2, part 1, 2 and 3 shoudl already work fine in current staging.

The reason i ask to test all the ACL inheritance system is, is because this PR rewrotes some part of the code that affect those parts, and those tests are to make sure there are no regressions on this.

If you already made all tests with success please mark them as tested successfully.

Think test script:
4: This tests Global config -> Component -> Extension -> Item Inherit
Should read:
4: This tests Global config -> Component -> Section -> Item Inherit

updated

[alikon]

I have tested this item ✅ successfully on 58adad0

works as described

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10894.}

[Bodge-IT]

I have tested this item ✅ successfully on 58adad0

Tested 1 and 2
All inheritances applied correctly.

Did not review code as per test 3. I'm not a coder.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10894.}

[infograf768]

@ggppdk
Just waiting for you now. :)

[ggppdk]

I have tested this item ✅ successfully on 58adad0

Tested custom forms, with custom rules, it works !! ))))

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10894.}

[pritalpatel]

I have tested this item ✅ successfully on 58adad0

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10894.}

[infograf768]

RTC. Thanks to all.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10894.}

[RonakParmar]

While testing first Step after applying patch found JLIB_RULES_NOT_ALLOWED_INHERITED language variable missing. Without patch language variable working fine.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10894.}

[RonakParmar]

I have tested this item ✅ successfully on 58adad0

After applied patch found : In Test 1: Testing default ACL Inheritance on creating a new item JLIB_RULES_NOT_ALLOWED_INHERITED missing language variable.

In Test 2: Testing ACL asset/groups Inheritance found JLIB_RULES_NOT_ALLOWED_DEFAULT, JLIB_RULES_NOT_ALLOWED_INHERITED, JLIB_RULES_ALLOWED_INHERITED missing language variables.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10894.}

[RonakParmar]

Missing language variables.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10894.}

[brianteeman]

It is not a successful test then

On 24 June 2016 at 13:54, RonakParmar notifications@github.com wrote:

Missing language variables.[image: screen shot 2016-06-24 at 07 54 03]

https://camo.githubusercontent.com/9d49e4d867fc727eb69f52720114b69273ffddf8/68747470733a2f2f6973737565732e6a6f6f6d6c612e6f72672f75706c6f6164732f312f66623938666162343764366333646338343038643662373133356132393066642e706e67

This comment was created with the J!Tracker Application
https://github.com/joomla/jissues at issues.joomla.org/joomla-cms/10894
https://issues.joomla.org/tracker/joomla-cms/10894.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#10894 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABPH8Q3UKtZVLEkJYLqjBcXjvMzkfUGiks5qO9N5gaJpZM4I63bF
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

[RonakParmar]

Given steps are working fine as described. So, I have made it successful test.

[andrepereiradasilva]

@RonakParmar are you using the latest staging?

Those language vars were added in a recent PR. See
b982eec#diff-fa3a45e6746f58d4a54e42207ee8f4adR669

[brianteeman]

If you are seeing untranslated strings then it is not a successful test

[RonakParmar]

Here is my System Information:

Joomla! Version : Joomla! 3.6.0-beta2-dev Development [ Noether ] 9-June-2016 12:33 GMT
Joomla! Platform Version: Joomla Platform 13.1.0 Stable [ Curiosity ] 24-Apr-2013 00:00 GMT
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10894.}

[RonakParmar]

@andrepereiradasilva Thank you to update me, I have downloaded current staging branch zip and installed in my system, Language variable are working fine with latest zip.

[andrepereiradasilva]

ok thanks. so all ok.

[andrepereiradasilva]

As an additional info, please notice this PR, as it is, also solves a regression from latest ACL PR (already merged).

To check it do the following.

Use a clean 3.5.1 install

• New category "Test ACL Category", don't change anything and save
• Edit category "Test ACL Category" and change "Editor" "Edit" action from "Allowed" to "Denied" and save
• Create a new article "Test ACL Article", don't change anything and save
• Edit article "Test ACL Article" and check "Editor" calculated permission for "Edit" action check is "Not Allowed (Locked)".

Do the same steps on Joomla staging (check the calculated permission is "Allowed").
This is the regression. The section -> item ACL calculated permissions inheritance.

Apply patch, check the calculated permission is "Not Allowed (Locked)".

[wilsonge]

Thanks guys