This is an automated email from the ASF dual-hosted git repository. pauls pushed a commit to branch issues/SLING-9953 in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-feature-cpconverter.git
commit 1c575f830b0ecb66d55ff949f610a5c5ef641514 Author: Karl Pauls <[email protected]> AuthorDate: Thu Jan 14 01:05:39 2021 +0100 SLING-9953: ACEs on/below user nodes are ignored upon conversion --- .../accesscontrol/DefaultAclManager.java | 109 ++++++++++++--------- .../features/DefaultFeaturesManager.java | 2 +- .../cpconverter/accesscontrol/AclManagerTest.java | 42 -------- .../handlers/RepPolicyEntryHandlerTest.java | 73 +++++++------- 4 files changed, 97 insertions(+), 129 deletions(-) diff --git a/src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/DefaultAclManager.java b/src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/DefaultAclManager.java index 900923b..580473d 100644 --- a/src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/DefaultAclManager.java +++ b/src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/DefaultAclManager.java @@ -35,15 +35,15 @@ import java.util.Optional; import java.util.Formatter; import java.util.HashMap; import java.util.HashSet; -import java.util.Iterator; import java.util.LinkedHashSet; import java.util.LinkedList; import java.util.List; +import java.util.LinkedHashMap; import java.util.Map; import java.util.Set; import java.util.Collection; -import java.util.Map.Entry; import java.util.Objects; +import java.util.Map.Entry; import java.util.function.Predicate; import java.util.stream.Collectors; @@ -53,6 +53,9 @@ public final class DefaultAclManager implements AclManager { private static final String DEFAULT_TYPE = "sling:Folder"; + // FIXME: SLING-9969. avoid hardcoding + private static final RepoPath USER_GROUP_ROOT = new RepoPath("/home"); + private final Set<RepoPath> preProvidedSystemPaths = new HashSet<>(); private final Set<RepoPath> preProvidedPaths = new HashSet<>(); @@ -77,17 +80,6 @@ public final class DefaultAclManager implements AclManager { return false; } - private void addPath(@NotNull RepoPath path, @NotNull Set<RepoPath> paths) { - if (preProvidedPaths.add(path)) { - paths.add(path); - } - - RepoPath parent = path.getParent(); - if (parent != null && parent.getSegmentCount() > 0) { - addPath(parent, paths); - } - } - public void addRepoinitExtension(@NotNull List<VaultPackageAssembler> packageAssemblers, @NotNull FeaturesManager featureManager) { try (Formatter formatter = new Formatter()) { @@ -139,18 +131,34 @@ public final class DefaultAclManager implements AclManager { @NotNull List<AccessControlEntry> authorizations, @NotNull List<VaultPackageAssembler> packageAssemblers, @NotNull Formatter formatter) { - // clean the unneeded ACLs, see SLING-8561 - Iterator<AccessControlEntry> authorizationsIterator = authorizations.iterator(); - while (authorizationsIterator.hasNext()) { - AccessControlEntry acl = authorizationsIterator.next(); + if (authorizations.isEmpty()) { + return; + } - if (acl.getRepositoryPath().startsWith(systemUser.getIntermediatePath())) { - authorizationsIterator.remove(); + Map<AccessControlEntry, String> entries = new LinkedHashMap<>(); + authorizations.forEach(entry -> { + String path = getRepoInitPath(entry.getRepositoryPath(), systemUser); + if (path != null) { + entries.put(entry, path); } - } - // finally add ACLs + }); + if (!entries.isEmpty()) { + formatter.format("set ACL for %s%n", systemUser.getId()); + entries.forEach((entry, path) -> { + formatter.format("%s %s on %s", + entry.getOperation(), + entry.getPrivileges(), + path); + + if (!entry.getRestrictions().isEmpty()) { + formatter.format(" restriction(%s)", + String.join(",", entry.getRestrictions())); + } - addAclStatement(formatter, systemUser, authorizations); + formatter.format("%n"); + }); + formatter.format("end%n"); + } } private @NotNull Optional<SystemUser> getSystemUser(@NotNull String id) { @@ -215,44 +223,47 @@ public final class DefaultAclManager implements AclManager { return type ? new RepoPath(current).toString() : null; } - private static void addAclStatement(@NotNull Formatter formatter, @NotNull SystemUser systemUser, @NotNull List<AccessControlEntry> authorizations) { - if (authorizations.isEmpty()) { - return; + @Nullable + private String getRepoInitPath(@NotNull RepoPath path, @NotNull SystemUser systemUser) { + if (path.isRepositoryPath()) { + return ":repository"; + } else if (isHomePath(path, systemUser.getPath())) { + return getHomePath(path, systemUser); + } else if (isUserGroupPath(path)) { + SystemUser otherSystemUser = getOtherSystemUser(path); + if (otherSystemUser != null) { + return getHomePath(path, otherSystemUser); + } else { + return path.toString(); + } + } else { + return path.toString(); } - - writeAccessControl(authorizations, "set ACL for %s%n", systemUser, formatter); } - private static void writeAccessControl(@NotNull List<AccessControlEntry> accessControlEntries, @NotNull String statement, @NotNull SystemUser systemUser, @NotNull Formatter formatter) { - formatter.format(statement, systemUser.getId()); - writeEntries(accessControlEntries, systemUser, formatter); - formatter.format("end%n"); + private static boolean isHomePath(@NotNull RepoPath path, @NotNull RepoPath systemUserPath) { + return path.startsWith(systemUserPath); } - private static void writeEntries(@NotNull List<AccessControlEntry> accessControlEntries, @NotNull SystemUser systemUser, @NotNull Formatter formatter) { - for (AccessControlEntry entry : accessControlEntries) { - formatter.format("%s %s on %s", - entry.getOperation(), - entry.getPrivileges(), - getRepoInitPath(entry.getRepositoryPath(), systemUser)); + private static boolean isUserGroupPath(@NotNull RepoPath path) { + return path.startsWith(USER_GROUP_ROOT); + } - if (!entry.getRestrictions().isEmpty()) { - formatter.format(" restriction(%s)", - String.join(",", entry.getRestrictions())); + @Nullable + private SystemUser getOtherSystemUser(@NotNull RepoPath path) { + for (SystemUser su : systemUsers) { + if (path.startsWith(su.getPath())) { + return su; } - - formatter.format("%n"); } + return null; } @NotNull - private static String getRepoInitPath(@NotNull RepoPath path, @NotNull SystemUser systemUser) { - // FIXME SLING-9953 : add special handing for path pointing to the system-user home or some other user/group home - if (path.isRepositoryPath()) { - return ":repository"; - } else { - return path.toString(); - } + private static String getHomePath(@NotNull RepoPath path, @NotNull SystemUser systemUser) { + RepoPath systemUserPath = systemUser.getPath(); + String subpath = (path.equals(systemUserPath) ? "" : path.toString().substring(systemUserPath.toString().length())); + return "home("+systemUser.getId()+")"+subpath; } private static void registerPrivileges(@NotNull PrivilegeDefinitions definitions, @NotNull Formatter formatter) { diff --git a/src/main/java/org/apache/sling/feature/cpconverter/features/DefaultFeaturesManager.java b/src/main/java/org/apache/sling/feature/cpconverter/features/DefaultFeaturesManager.java index 86fa637..ab4ea25 100644 --- a/src/main/java/org/apache/sling/feature/cpconverter/features/DefaultFeaturesManager.java +++ b/src/main/java/org/apache/sling/feature/cpconverter/features/DefaultFeaturesManager.java @@ -88,7 +88,7 @@ public class DefaultFeaturesManager implements FeaturesManager { private Feature targetFeature = null; DefaultFeaturesManager() { - this(null); + this(new File("")); } public DefaultFeaturesManager(@NotNull File tempDir) { diff --git a/src/test/java/org/apache/sling/feature/cpconverter/accesscontrol/AclManagerTest.java b/src/test/java/org/apache/sling/feature/cpconverter/accesscontrol/AclManagerTest.java index 98c1863..f5a510d 100644 --- a/src/test/java/org/apache/sling/feature/cpconverter/accesscontrol/AclManagerTest.java +++ b/src/test/java/org/apache/sling/feature/cpconverter/accesscontrol/AclManagerTest.java @@ -68,48 +68,6 @@ public class AclManagerTest { } @Test - public void makeSureAclsAreCreatedOnlyoutsideSytemUsersPaths() throws Exception { - aclManager.addSystemUser(new SystemUser("acs-commons-package-replication-status-event-service", new RepoPath("/home/users/system/foo"), new RepoPath("/home/users/system"))); - - aclManager.addAcl("acs-commons-package-replication-status-event-service", newAcl(true, "jcr:read,rep:write,rep:indexDefinitionManagement", "/asd/not/system/user/path")); - aclManager.addAcl("acs-commons-package-replication-status-event-service", newAcl(true, "jcr:read,crx:replicate,jcr:removeNode", "/home/users/system")); - - VaultPackageAssembler assembler = mock(VaultPackageAssembler.class); - when(assembler.getEntry(anyString())).thenReturn(tempDir.toFile()); - when(assembler.getEntry("asd/not/.content.xml")).thenReturn(new File(getClass().getResource("asd/not/.content.xml").getFile())); - - - Feature feature = new Feature(new ArtifactId("org.apache.sling", "org.apache.sling.cp2fm", "0.0.1", null, null)); - - FeaturesManager fm = Mockito.spy(new DefaultFeaturesManager(tempDir.toFile())); - when(fm.getTargetFeature()).thenReturn(feature); - - aclManager.addRepoinitExtension(Arrays.asList(assembler), fm); - - - Extension repoinitExtension = feature.getExtensions().getByName(Extension.EXTENSION_NAME_REPOINIT); - assertNotNull(repoinitExtension); - - // acs-commons-on-deploy-scripts-service will be missed - String expected = - "create service user acs-commons-package-replication-status-event-service with path /home/users/system" + System.lineSeparator() + - "create path /asd/not(nt:unstructured mixin rep:AccessControllable,mix:created)/system/user/path" + System.lineSeparator() + - // see SLING-8561 - // "set ACL for acs-commons-package-replication-status-event-service\n" + - // "allow jcr:read,crx:replicate,jcr:removeNode on /asd/public\n" + - // "end\n" + - "set ACL for acs-commons-package-replication-status-event-service" + System.lineSeparator() + - "allow jcr:read,rep:write,rep:indexDefinitionManagement on /asd/not/system/user/path" + System.lineSeparator() + - "end" + System.lineSeparator(); - String actual = repoinitExtension.getText(); - assertEquals(expected, actual); - - RepoInitParser repoInitParser = new RepoInitParserService(); - List<Operation> operations = repoInitParser.parse(new StringReader(actual)); - assertFalse(operations.isEmpty()); - } - - @Test public void testReset() throws RepoInitParsingException { // We assume this user will not be in the result because of the reset in the next line aclManager.addSystemUser(new SystemUser("acs-commons-ensure-oak-index-service", new RepoPath("/home/users/system/foo"), new RepoPath("/home/users/system"))); diff --git a/src/test/java/org/apache/sling/feature/cpconverter/handlers/RepPolicyEntryHandlerTest.java b/src/test/java/org/apache/sling/feature/cpconverter/handlers/RepPolicyEntryHandlerTest.java index a9bf8eb..267a6b0 100644 --- a/src/test/java/org/apache/sling/feature/cpconverter/handlers/RepPolicyEntryHandlerTest.java +++ b/src/test/java/org/apache/sling/feature/cpconverter/handlers/RepPolicyEntryHandlerTest.java @@ -96,31 +96,32 @@ public final class RepPolicyEntryHandlerTest { // commented ACLs are due SLING-8561 String expected = "create service user acs-commons-ensure-oak-index-service with path /home/users/system" + System.lineSeparator() + - // "create path (sling:Folder) /asd\n" + - // "create path (sling:Folder) /asd/public\n" + - // "set ACL for acs-commons-ensure-oak-index-service\n" + - // "allow jcr:read,rep:write,rep:indexDefinitionManagement on /asd/public restriction(rep:glob,*/oak:index/*)\n" + - // "end\n" + "create service user acs-commons-dispatcher-flush-service with path /home/users/system" + System.lineSeparator() + - // "set ACL for acs-commons-dispatcher-flush-service\n" + - // "allow jcr:read,crx:replicate,jcr:removeNode on /asd/public\n" + - // "end\n" + "create service user acs-commons-package-replication-status-event-service with path /home/users/system" + System.lineSeparator() + - // "set ACL for acs-commons-package-replication-status-event-service\n" + - // "allow jcr:read,rep:write,jcr:readAccessControl,jcr:modifyAccessControl on /asd/public\n" + - // "end\n" + "create service user acs-commons-ensure-service-user-service with path /home/users/system" + System.lineSeparator() + - // "set ACL for acs-commons-ensure-service-user-service\n" + - // "allow jcr:read,rep:write,jcr:readAccessControl,jcr:modifyAccessControl on /asd/public\n" + - // "end\n" + "create service user acs-commons-automatic-package-replicator-service with path /home/users/system" + System.lineSeparator() + - // "set ACL for acs-commons-automatic-package-replicator-service\n" + - // "allow jcr:read on /asd/public\n" + - // "end\n" + - "create service user acs-commons-on-deploy-scripts-service with path /home/users/system" + System.lineSeparator(); - // "set ACL for acs-commons-on-deploy-scripts-service\n" + - // "allow jcr:read on /asd/public\n" + - // "end\n"; + "create service user acs-commons-on-deploy-scripts-service with path /home/users/system" + System.lineSeparator() + + "set ACL for acs-commons-automatic-package-replicator-service" + System.lineSeparator() + + "allow jcr:read on home(acs-commons-automatic-package-replicator-service)" + System.lineSeparator() + + "end" + System.lineSeparator() + + "set ACL for acs-commons-package-replication-status-event-service" + System.lineSeparator() + + "allow jcr:read,rep:write,jcr:readAccessControl,jcr:modifyAccessControl on home(acs-commons-package-replication-status-event-service)" + System.lineSeparator() + + "deny jcr:write on home(acs-commons-package-replication-status-event-service)" + System.lineSeparator() + + "end" + System.lineSeparator() + + "set ACL for acs-commons-dispatcher-flush-service" + System.lineSeparator() + + "allow jcr:read,crx:replicate,jcr:removeNode on home(acs-commons-dispatcher-flush-service)" + System.lineSeparator() + + "deny jcr:write on home(acs-commons-dispatcher-flush-service)" + System.lineSeparator() + + "end" + System.lineSeparator() + + "set ACL for acs-commons-ensure-oak-index-service" + System.lineSeparator() + + "allow jcr:read,rep:write,rep:indexDefinitionManagement on home(acs-commons-ensure-oak-index-service) restriction(rep:glob,*/oak:index/*)" + System.lineSeparator() + + "end" + System.lineSeparator() + + "set ACL for acs-commons-on-deploy-scripts-service" + System.lineSeparator() + + "allow jcr:read on home(acs-commons-on-deploy-scripts-service)" + System.lineSeparator() + + "end" + System.lineSeparator() + + "set ACL for acs-commons-ensure-service-user-service" + System.lineSeparator() + + "allow jcr:read,rep:write,jcr:readAccessControl,jcr:modifyAccessControl on home(acs-commons-ensure-service-user-service)" + System.lineSeparator() + + "end" + System.lineSeparator(); + String actual = repoinitExtension.getText(); assertEquals(expected, actual); @@ -140,26 +141,24 @@ public final class RepPolicyEntryHandlerTest { assertNotNull(repoinitExtension); assertEquals(ExtensionType.TEXT, repoinitExtension.getType()); - // commented ACLs are due SLING-8561 String expected = "create service user acs-commons-package-replication-status-event-service with path /home/users/system" + System.lineSeparator() + - // "create path (sling:Folder) /asd\n" + - // "create path (sling:Folder) /asd/public\n" + - // "set ACL for acs-commons-package-replication-status-event-service\n" + - // "allow jcr:read,rep:write,jcr:readAccessControl,jcr:modifyAccessControl on /asd/public\n" + - // "end\n" + "create service user acs-commons-ensure-service-user-service with path /home/users/system" + System.lineSeparator() + - // "set ACL for acs-commons-ensure-service-user-service\n" + - // "allow jcr:read,rep:write,jcr:readAccessControl,jcr:modifyAccessControl on /asd/public\n" + - // "end\n" + "create service user acs-commons-automatic-package-replicator-service with path /home/users/system" + System.lineSeparator() + - // "set ACL for acs-commons-automatic-package-replicator-service\n" + - // "allow jcr:read on /asd/public\n" + - // "end\n" + - "create service user acs-commons-on-deploy-scripts-service with path /home/users/system" + System.lineSeparator(); - //"set ACL for acs-commons-on-deploy-scripts-service\n" + - //"allow jcr:read on /asd/public\n" + - //"end\n"; + "create service user acs-commons-on-deploy-scripts-service with path /home/users/system" + System.lineSeparator() + + "set ACL for acs-commons-automatic-package-replicator-service" + System.lineSeparator() + + "allow jcr:read on home(acs-commons-automatic-package-replicator-service)" + System.lineSeparator() + + "end" + System.lineSeparator() + + "set ACL for acs-commons-package-replication-status-event-service" + System.lineSeparator() + + "allow jcr:read,rep:write,jcr:readAccessControl,jcr:modifyAccessControl on home(acs-commons-package-replication-status-event-service)" + System.lineSeparator() + + "deny jcr:write on home(acs-commons-package-replication-status-event-service)" + System.lineSeparator() + + "end" + System.lineSeparator() + + "set ACL for acs-commons-on-deploy-scripts-service" + System.lineSeparator() + + "allow jcr:read on home(acs-commons-on-deploy-scripts-service)" + System.lineSeparator() + + "end" + System.lineSeparator() + + "set ACL for acs-commons-ensure-service-user-service" + System.lineSeparator() + + "allow jcr:read,rep:write,jcr:readAccessControl,jcr:modifyAccessControl on home(acs-commons-ensure-service-user-service)" + System.lineSeparator() + + "end" + System.lineSeparator(); String actual = repoinitExtension.getText(); assertEquals(expected, actual);
