You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2022/02/25 09:42:43 UTC
[GitHub] [sling-org-apache-sling-jcr-repoinit] anchela opened a new pull request #22: SLING-11160 : Repoinit does not allow to remove individual ACEs (jcr impl)
anchela opened a new pull request #22:
URL: https://github.com/apache/sling-org-apache-sling-jcr-repoinit/pull/22
@bdelacretaz , @cziegeler , @karlpauls , implementation of the REMOVE of a dedicated ACE which was not supported up to now. i would appreciate if you had time to review it.
NOTE: build will fail unless PR to repoinit.parser is merged. so for the time being this is for illustration of the problem with the existing repo-init language and why i suggest to change it).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-repoinit] anchela merged pull request #22: SLING-11160 : Repoinit does not allow to remove individual ACEs (jcr impl)
Posted by GitBox <gi...@apache.org>.
anchela merged pull request #22:
URL: https://github.com/apache/sling-org-apache-sling-jcr-repoinit/pull/22
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-repoinit] bdelacretaz commented on a change in pull request #22: SLING-11160 : Repoinit does not allow to remove individual ACEs (jcr impl)
Posted by GitBox <gi...@apache.org>.
bdelacretaz commented on a change in pull request #22:
URL: https://github.com/apache/sling-org-apache-sling-jcr-repoinit/pull/22#discussion_r816846413
##########
File path: src/test/java/org/apache/sling/jcr/repoinit/PrincipalBasedAclTest.java
##########
@@ -692,28 +693,64 @@ public void testRemoveMatchingEntry() throws Exception {
}
}
+ @Test
+ public void testRemoveNoExistingPolicy() throws Exception {
+ String setup = "remove principal ACL for " + U.username + "\n"
+ + "allow jcr:read on " + path + "\n"
+ + "end";
+ U.parseAndExecute(setup);
+ }
+
+ @Test
+ public void testRemoveMatchingEntry() throws Exception {
+ Principal principal = getPrincipal(U.username);
+ String setup = "set principal ACL for " + U.username + "\n"
+ + "allow jcr:write on "+path+"\n"
+ + "end";
+ U.parseAndExecute(setup);
+ assertPolicy(principal, U.adminSession, 1);
+
+ // privilege mismatch
+ setup = "remove principal ACL for " + U.username + "\n"
+ + "allow jcr:read,jcr:write on " + path + "\n"
+ + "end";
+ U.parseAndExecute(setup);
+ assertPolicy(principal, U.adminSession, 1);
+
+ // path mismatch
+ setup = "remove principal ACL for " + U.username + "\n"
+ + "allow jcr:write on " + path + "/mismatch\n"
+ + "end";
+ U.parseAndExecute(setup);
+ assertPolicy(principal, U.adminSession, 1);
+
+ // restriction mismatch
+ setup = "remove principal ACL for " + U.username + "\n"
+ + "allow jcr:write on " + path + " restriction(rep:glob, /*/jcr:content/*)\n"
+ + "end";
+ U.parseAndExecute(setup);
+ assertPolicy(principal, U.adminSession, 1);
+ }
+
@Test
public void testRemoveNoMatchingEntry() throws Exception {
+ Principal principal = getPrincipal(U.username);
String setup = "set principal ACL for " + U.username + "\n"
+ "allow jcr:write on "+path+"\n"
+ "end";
U.parseAndExecute(setup);
+ assertPolicy(principal, U.adminSession, 1);
- setup = "set principal ACL for " + U.username + "\n"
Review comment:
IIUC this test goes away with your changes, shouldn't it be kept? To test backwards compatibility - even if the corresponding operation is deprecated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-repoinit] anchela commented on a change in pull request #22: SLING-11160 : Repoinit does not allow to remove individual ACEs (jcr impl)
Posted by GitBox <gi...@apache.org>.
anchela commented on a change in pull request #22:
URL: https://github.com/apache/sling-org-apache-sling-jcr-repoinit/pull/22#discussion_r817643481
##########
File path: src/main/java/org/apache/sling/jcr/repoinit/impl/AclUtil.java
##########
@@ -291,6 +328,42 @@ public static void setPrincipalAcl(Session session, String principalName, Collec
}
}
+ public static void removePrincipalEntries(Session session, String principalName, Collection<AclLine> lines) throws RepositoryException {
+ final JackrabbitAccessControlManager acMgr = getJACM(session);
+ Principal principal = AccessControlUtils.getPrincipal(session, principalName);
+ if (principal == null) {
+ // due to transient nature of the repo-init the principal lookup may not succeed if completed through query
+ // -> save transient changes and retry principal lookup
+ session.save();
+ principal = AccessControlUtils.getPrincipal(session, principalName);
+ checkState(principal != null, "Principal not found: " + principalName);
+ }
+
+ final PrincipalAccessControlList acl = getPrincipalAccessControlList(acMgr, principal, true);
+ boolean modified = false;
+ for (AclLine line : lines) {
+ List<String> jcrPaths = getJcrPaths(session, line.getProperty(PROP_PATHS));
+ LocalRestrictions restr = createLocalRestrictions(line.getRestrictions(), acl, session);
+ List<String> privNames = line.getProperty(PROP_PRIVILEGES);
+ Privilege[] privs = AccessControlUtils.privilegesFromNames(session, privNames.toArray(new String[0]));
+ Predicate<PrincipalAccessControlList.Entry> predicate = entry -> {
+ if (!jcrPaths.contains(entry.getEffectivePath())) {
+ return false;
+ }
+ LocalAccessControlEntry lace = new LocalAccessControlEntry(entry.getPrincipal(), privs, line.getAction()== AclLine.Action.ALLOW, restr);
+ return lace.isEqual(entry);
+ };
+ if (removePrincipalEntries(acl, principalName, predicate)) {
+ modified = true;
Review comment:
@bdelacretaz , in fact there was a test-method for that but i managed to garble the test during the many refactorings. should be fixed now. can you please confirm?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-repoinit] bdelacretaz commented on a change in pull request #22: SLING-11160 : Repoinit does not allow to remove individual ACEs (jcr impl)
Posted by GitBox <gi...@apache.org>.
bdelacretaz commented on a change in pull request #22:
URL: https://github.com/apache/sling-org-apache-sling-jcr-repoinit/pull/22#discussion_r817567427
##########
File path: src/main/java/org/apache/sling/jcr/repoinit/impl/AclUtil.java
##########
@@ -291,6 +328,42 @@ public static void setPrincipalAcl(Session session, String principalName, Collec
}
}
+ public static void removePrincipalEntries(Session session, String principalName, Collection<AclLine> lines) throws RepositoryException {
+ final JackrabbitAccessControlManager acMgr = getJACM(session);
+ Principal principal = AccessControlUtils.getPrincipal(session, principalName);
+ if (principal == null) {
+ // due to transient nature of the repo-init the principal lookup may not succeed if completed through query
+ // -> save transient changes and retry principal lookup
+ session.save();
+ principal = AccessControlUtils.getPrincipal(session, principalName);
+ checkState(principal != null, "Principal not found: " + principalName);
+ }
+
+ final PrincipalAccessControlList acl = getPrincipalAccessControlList(acMgr, principal, true);
+ boolean modified = false;
+ for (AclLine line : lines) {
+ List<String> jcrPaths = getJcrPaths(session, line.getProperty(PROP_PATHS));
+ LocalRestrictions restr = createLocalRestrictions(line.getRestrictions(), acl, session);
+ List<String> privNames = line.getProperty(PROP_PRIVILEGES);
+ Privilege[] privs = AccessControlUtils.privilegesFromNames(session, privNames.toArray(new String[0]));
+ Predicate<PrincipalAccessControlList.Entry> predicate = entry -> {
+ if (!jcrPaths.contains(entry.getEffectivePath())) {
+ return false;
+ }
+ LocalAccessControlEntry lace = new LocalAccessControlEntry(entry.getPrincipal(), privs, line.getAction()== AclLine.Action.ALLOW, restr);
+ return lace.isEqual(entry);
+ };
+ if (removePrincipalEntries(acl, principalName, predicate)) {
+ modified = true;
Review comment:
Looking at the test coverage with a `-P jacoco-report` build reports that this line is not covered by tests, and that's also the case for the `if (modified)` statement below. Those lines look important, I suppose they should be tested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-repoinit] anchela commented on a change in pull request #22: SLING-11160 : Repoinit does not allow to remove individual ACEs (jcr impl)
Posted by GitBox <gi...@apache.org>.
anchela commented on a change in pull request #22:
URL: https://github.com/apache/sling-org-apache-sling-jcr-repoinit/pull/22#discussion_r816926678
##########
File path: src/test/java/org/apache/sling/jcr/repoinit/PrincipalBasedAclTest.java
##########
@@ -692,28 +693,64 @@ public void testRemoveMatchingEntry() throws Exception {
}
}
+ @Test
+ public void testRemoveNoExistingPolicy() throws Exception {
+ String setup = "remove principal ACL for " + U.username + "\n"
+ + "allow jcr:read on " + path + "\n"
+ + "end";
+ U.parseAndExecute(setup);
+ }
+
+ @Test
+ public void testRemoveMatchingEntry() throws Exception {
+ Principal principal = getPrincipal(U.username);
+ String setup = "set principal ACL for " + U.username + "\n"
+ + "allow jcr:write on "+path+"\n"
+ + "end";
+ U.parseAndExecute(setup);
+ assertPolicy(principal, U.adminSession, 1);
+
+ // privilege mismatch
+ setup = "remove principal ACL for " + U.username + "\n"
+ + "allow jcr:read,jcr:write on " + path + "\n"
+ + "end";
+ U.parseAndExecute(setup);
+ assertPolicy(principal, U.adminSession, 1);
+
+ // path mismatch
+ setup = "remove principal ACL for " + U.username + "\n"
+ + "allow jcr:write on " + path + "/mismatch\n"
+ + "end";
+ U.parseAndExecute(setup);
+ assertPolicy(principal, U.adminSession, 1);
+
+ // restriction mismatch
+ setup = "remove principal ACL for " + U.username + "\n"
+ + "allow jcr:write on " + path + " restriction(rep:glob, /*/jcr:content/*)\n"
+ + "end";
+ U.parseAndExecute(setup);
+ assertPolicy(principal, U.adminSession, 1);
+ }
+
@Test
public void testRemoveNoMatchingEntry() throws Exception {
+ Principal principal = getPrincipal(U.username);
String setup = "set principal ACL for " + U.username + "\n"
+ "allow jcr:write on "+path+"\n"
+ "end";
U.parseAndExecute(setup);
+ assertPolicy(principal, U.adminSession, 1);
- setup = "set principal ACL for " + U.username + "\n"
- + "remove jcr:read on " + path + "\n"
+ setup = "remove principal ACL for " + U.username + "\n"
+ + "allow jcr:read on " + path + "\n"
+ "end";
- try {
- U.parseAndExecute(setup);
- fail("Expecting REMOVE to fail");
- } catch(RuntimeException rex) {
- assertRegex(REMOVE_NOT_SUPPORTED_REGEX, rex.getMessage());
- }
+ assertPolicy(principal, U.adminSession, 1);
}
- @Test(expected = RuntimeException.class)
+ @Test(expected = RepoInitException.class)
public void testRemoveNonExistingPrincipal() throws Exception {
- String setup = "set principal ACL for nonExistingPrincipal\n"
Review comment:
this one didn't go away.... instead i refactored the tests. i think 1 test to make sure the unsupported remove-action really throws an exception is sufficient.... instead wanted to have extra coverage for the new removal.
##########
File path: src/test/java/org/apache/sling/jcr/repoinit/PrincipalBasedAclTest.java
##########
@@ -725,16 +762,16 @@ public void testRemovePrincipalMismatch() throws Exception {
+ "end";
U.parseAndExecute(setup);
U.parseAndExecute("create service user otherSystemPrincipal");
+ assertPolicy(getPrincipal(U.username), U.adminSession, 1);
- try {
- setup = "set principal ACL for otherSystemPrincipal\n"
- + "remove jcr:write on " + path + "\n"
- + "end";
- U.parseAndExecute(setup);
- fail("Expecting REMOVE to fail");
- } catch(RuntimeException rex) {
- assertRegex(REMOVE_NOT_SUPPORTED_REGEX, rex.getMessage());
Review comment:
see above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-repoinit] anchela commented on pull request #22: SLING-11160 : Repoinit does not allow to remove individual ACEs (jcr impl)
Posted by GitBox <gi...@apache.org>.
anchela commented on pull request #22:
URL: https://github.com/apache/sling-org-apache-sling-jcr-repoinit/pull/22#issuecomment-1055786285
@bdelacretaz , i renamed the methods/classes as discussed and would appreciate if you could take another look
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-repoinit] anchela commented on a change in pull request #22: SLING-11160 : Repoinit does not allow to remove individual ACEs (jcr impl)
Posted by GitBox <gi...@apache.org>.
anchela commented on a change in pull request #22:
URL: https://github.com/apache/sling-org-apache-sling-jcr-repoinit/pull/22#discussion_r817578444
##########
File path: src/main/java/org/apache/sling/jcr/repoinit/impl/AclUtil.java
##########
@@ -291,6 +328,42 @@ public static void setPrincipalAcl(Session session, String principalName, Collec
}
}
+ public static void removePrincipalEntries(Session session, String principalName, Collection<AclLine> lines) throws RepositoryException {
+ final JackrabbitAccessControlManager acMgr = getJACM(session);
+ Principal principal = AccessControlUtils.getPrincipal(session, principalName);
+ if (principal == null) {
+ // due to transient nature of the repo-init the principal lookup may not succeed if completed through query
+ // -> save transient changes and retry principal lookup
+ session.save();
+ principal = AccessControlUtils.getPrincipal(session, principalName);
+ checkState(principal != null, "Principal not found: " + principalName);
+ }
+
+ final PrincipalAccessControlList acl = getPrincipalAccessControlList(acMgr, principal, true);
+ boolean modified = false;
+ for (AclLine line : lines) {
+ List<String> jcrPaths = getJcrPaths(session, line.getProperty(PROP_PATHS));
+ LocalRestrictions restr = createLocalRestrictions(line.getRestrictions(), acl, session);
+ List<String> privNames = line.getProperty(PROP_PRIVILEGES);
+ Privilege[] privs = AccessControlUtils.privilegesFromNames(session, privNames.toArray(new String[0]));
+ Predicate<PrincipalAccessControlList.Entry> predicate = entry -> {
+ if (!jcrPaths.contains(entry.getEffectivePath())) {
+ return false;
+ }
+ LocalAccessControlEntry lace = new LocalAccessControlEntry(entry.getPrincipal(), privs, line.getAction()== AclLine.Action.ALLOW, restr);
+ return lace.isEqual(entry);
+ };
+ if (removePrincipalEntries(acl, principalName, predicate)) {
+ modified = true;
Review comment:
yes, i agree
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-repoinit] bdelacretaz commented on a change in pull request #22: SLING-11160 : Repoinit does not allow to remove individual ACEs (jcr impl)
Posted by GitBox <gi...@apache.org>.
bdelacretaz commented on a change in pull request #22:
URL: https://github.com/apache/sling-org-apache-sling-jcr-repoinit/pull/22#discussion_r816846895
##########
File path: src/test/java/org/apache/sling/jcr/repoinit/PrincipalBasedAclTest.java
##########
@@ -692,28 +693,64 @@ public void testRemoveMatchingEntry() throws Exception {
}
}
+ @Test
+ public void testRemoveNoExistingPolicy() throws Exception {
+ String setup = "remove principal ACL for " + U.username + "\n"
+ + "allow jcr:read on " + path + "\n"
+ + "end";
+ U.parseAndExecute(setup);
+ }
+
+ @Test
+ public void testRemoveMatchingEntry() throws Exception {
+ Principal principal = getPrincipal(U.username);
+ String setup = "set principal ACL for " + U.username + "\n"
+ + "allow jcr:write on "+path+"\n"
+ + "end";
+ U.parseAndExecute(setup);
+ assertPolicy(principal, U.adminSession, 1);
+
+ // privilege mismatch
+ setup = "remove principal ACL for " + U.username + "\n"
+ + "allow jcr:read,jcr:write on " + path + "\n"
+ + "end";
+ U.parseAndExecute(setup);
+ assertPolicy(principal, U.adminSession, 1);
+
+ // path mismatch
+ setup = "remove principal ACL for " + U.username + "\n"
+ + "allow jcr:write on " + path + "/mismatch\n"
+ + "end";
+ U.parseAndExecute(setup);
+ assertPolicy(principal, U.adminSession, 1);
+
+ // restriction mismatch
+ setup = "remove principal ACL for " + U.username + "\n"
+ + "allow jcr:write on " + path + " restriction(rep:glob, /*/jcr:content/*)\n"
+ + "end";
+ U.parseAndExecute(setup);
+ assertPolicy(principal, U.adminSession, 1);
+ }
+
@Test
public void testRemoveNoMatchingEntry() throws Exception {
+ Principal principal = getPrincipal(U.username);
String setup = "set principal ACL for " + U.username + "\n"
+ "allow jcr:write on "+path+"\n"
+ "end";
U.parseAndExecute(setup);
+ assertPolicy(principal, U.adminSession, 1);
- setup = "set principal ACL for " + U.username + "\n"
- + "remove jcr:read on " + path + "\n"
+ setup = "remove principal ACL for " + U.username + "\n"
+ + "allow jcr:read on " + path + "\n"
+ "end";
- try {
- U.parseAndExecute(setup);
- fail("Expecting REMOVE to fail");
- } catch(RuntimeException rex) {
- assertRegex(REMOVE_NOT_SUPPORTED_REGEX, rex.getMessage());
- }
+ assertPolicy(principal, U.adminSession, 1);
}
- @Test(expected = RuntimeException.class)
+ @Test(expected = RepoInitException.class)
public void testRemoveNonExistingPrincipal() throws Exception {
- String setup = "set principal ACL for nonExistingPrincipal\n"
Review comment:
Same comment as above for a test that goes away
##########
File path: src/test/java/org/apache/sling/jcr/repoinit/PrincipalBasedAclTest.java
##########
@@ -725,16 +762,16 @@ public void testRemovePrincipalMismatch() throws Exception {
+ "end";
U.parseAndExecute(setup);
U.parseAndExecute("create service user otherSystemPrincipal");
+ assertPolicy(getPrincipal(U.username), U.adminSession, 1);
- try {
- setup = "set principal ACL for otherSystemPrincipal\n"
- + "remove jcr:write on " + path + "\n"
- + "end";
- U.parseAndExecute(setup);
- fail("Expecting REMOVE to fail");
- } catch(RuntimeException rex) {
- assertRegex(REMOVE_NOT_SUPPORTED_REGEX, rex.getMessage());
Review comment:
Same comment as above for a test that goes away
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org