[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: |
|
||||||||||||||||||||||||||||
| Issue Links: |
|
||||||||||||||||||||||||||||
| 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 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;
}
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 "/" |