[MGNLUI-3838] Wrong ACL-validation results in AccessViolation Created: 04/Apr/16  Updated: 11/Aug/16  Resolved: 29/Jul/16

Status: Closed
Project: Magnolia UI
Component/s: None
Affects Version/s: None
Fix Version/s: 5.4.8

Type: Bug Priority: Critical
Reporter: Sigurd Rolfes Assignee: Oanh Thai Hoang
Resolution: Fixed Votes: 0
Labels: security, security-app, support
Remaining Estimate: 0d
Time Spent: 7d
Original Estimate: 4d

Attachments: Text File acls-anonymous.txt     Text File acls-custromAdmin.txt     XML File userroles.anonymous.xml     XML File userroles.zeg-admin.xml    
Issue Links:
Relates
dependency
depends upon MGNLUI-3920 Remove changedProperties from adapter... Closed
depends upon MGNLUI-3919 Refactor inline field in WorkspaceAcc... Closed
supersession
is superseded by MGNLUI-3979 Cannot grant URI permissions to root Closed
is superseded by MGNLUI-3980 Consider loosening validation of recu... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:
Sprint: Saigon 54
Story Points: 5

 Description   

We can't modify the anonymous role anymore.

We created a separate account customAdmin that has not superuser role.

To shorten the problem and for reproducing the error here an example for workspace category:

anonymous has ACL read-only on "/" selected and subnodes
customAdmin has ACL read/write on "/" selected and subnodes

In SaveRoleDialogAction line 262 ff. (validateAccessControlLists()) the ACLs of the current user are checked against the ACLs of the role to be saved. The current user needs at least write permissions to the workspace and node.

The Exception ist fired in line 295. Reason:

In methoid isCurrentUserEntitledToGrantRights(workspaceName, path, accessType, permissions) the boolean recursive is true (line 349), wildcard is stripped off of the original path and ownPermission will always be "/" in findBestMatchingPermissions(acl.getList(), stripWildcardsFromPath(path)) (line 344).

But if recursive is true the permission check wants the path to macth "/*" (line 352):

if (recursive && !ownPermissions.getPattern().getPatternString().endsWith("/*"))

If I havn't overseen anything the implementation of findBestMatchingPermissions() returns the wrong value for ownPermission ("/" instead of "/*").

Find attached the XML export snippets for the roles and workspace category.

BTW: the validation method validates the ACL list one after the other. It does not matter how many entries there are. category is first and when category fails the exception is thrown.



 Comments   
Comment by Oanh Thai Hoang [ 12/Apr/16 ]

Here is a unit test that reproduce the issue, put it to info.magnolia.security.app.dialog.action.SaveRoleDialogActionTest

    @Test
    public void givingAllPermissionToRootWhenUserHasRecursiveWritePermission() throws Exception {

        // GIVEN
        grant(RepositoryConstants.WEBSITE, "/", Permission.ALL);
        grant(RepositoryConstants.WEBSITE, "/*", Permission.ALL);

        // WHEN
        JcrNewNodeAdapter roleItem = new JcrNewNodeAdapter(session.getRootNode(), NodeTypes.Role.NAME);
        roleItem.addItemProperty(ModelConstants.JCR_NAME, new DefaultProperty<String>("testRole"));

        addAclEntry(addAclItem(roleItem, "acl_website"), "0", "/", Permission.ALL, AccessControlList.ACCESS_TYPE_NODE_AND_CHILDREN);

        createAction(roleItem).execute();

        // THEN
        assertRoleHasReadAccessToItself("testRole");

        assertTrue(session.nodeExists("/testRole/acl_website/0"));
        assertEquals("/", session.getProperty("/testRole/acl_website/0/path").getString());
        assertEquals(Permission.ALL, session.getProperty("/testRole/acl_website/0/permissions").getLong());
    }
Comment by Oanh Thai Hoang [ 12/Apr/16 ]

Root cause: Issue happens from below code while examining whether the user have required rights to do with current path

        Permission ownPermissions = findBestMatchingPermissions(acl.getList(), stripWildcardsFromPath(path));
        if (ownPermissions == null) {
            return false;
        }

        boolean recursive = (accessType & AccessControlList.ACCESS_TYPE_CHILDREN) != 0;

        if (recursive && !ownPermissions.getPattern().getPatternString().endsWith("/*")) {
            return false;
        }
  • The #findBestMatchingPermissions works in a correct way. "/" path should match with permission has "/" pattern rather than permission has "/* pattern. So the function will return false because of condition if (recursive && !ownPermissions.getPattern().getPatternString().endsWith("/*"))
  • A unit test for above condition can be found at SaveRoleDialogActionTest#deniesGivingRecursiveReadPermissionWhenUserDoesNotHaveRecursiveReadPermission

Solution: should change if condition or #findBestMatchingPermissions

1. Should ignore case of "/"

        if (recursive && !ownPermissions.getPattern().getPatternString().endsWith("/*") && !ownPermissions.getPattern().getPatternString().equals("/")) {
            return false;
        }

2. Modify #findBestMatchingPermissions to return permission has "/*" instead of permission has "/" incase find path "/"
There has the similar logic can be found at AccessManagerImpl#getPermissions. If we choose solution, #getPermissions may be changed also

Generated at Mon Feb 12 09:10:35 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.