From 1788af5c4c4e16a08593d7c8005ef0ac1cf77b22 Mon Sep 17 00:00:00 2001 From: angela Date: Fri, 4 Nov 2022 13:14:12 +0100 Subject: [PATCH 1/2] SLING-11663 : Create all service users with forced path --- .../accesscontrol/DefaultAclManager.java | 22 +++++--- .../accesscontrol/EnforceInfo.java | 3 +- ...Package2FeatureModelConverterLauncher.java | 5 +- .../repoinit/SystemUserVisitor.java | 4 ++ .../shared/ConverterConstants.java | 7 +++ .../ConverterUserAndPermissionTest.java | 53 +++++++++++-------- .../sling/feature/cpconverter/Util.java | 9 ++++ .../accesscontrol/AclManagerTest.java | 41 ++++++++++---- .../accesscontrol/EnforceInfoTest.java | 53 ++++++++++++++----- .../EnforcePrincipalBasedTest.java | 8 +-- .../cpconverter/handlers/RepoInitTest.java | 16 +++--- 11 files changed, 160 insertions(+), 61 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 9a878492..862e638a 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 @@ -75,6 +75,7 @@ public class DefaultAclManager implements AclManager, EnforceInfo { private final RepoPath enforcePrincipalBasedSupportedPath; private final String systemRelPath; + private final boolean alwaysForceSystemUserPath; private final OperationProcessor processor = new OperationProcessor(); @@ -96,15 +97,16 @@ public class DefaultAclManager implements AclManager, EnforceInfo { private RepoPath userRootPath; public DefaultAclManager() { - this(null, ConverterConstants.SYSTEM_USER_REL_PATH_DEFAULT); + this(null, ConverterConstants.SYSTEM_USER_REL_PATH_DEFAULT, false); } - - public DefaultAclManager(@Nullable String enforcePrincipalBasedSupportedPath, @NotNull String systemRelPath) { + + public DefaultAclManager(@Nullable String enforcePrincipalBasedSupportedPath, @NotNull String systemRelPath, boolean alwaysForceSystemUserPath) { if (enforcePrincipalBasedSupportedPath != null && !enforcePrincipalBasedSupportedPath.contains(systemRelPath)) { throw new RuntimeException("Relative path for system users "+ systemRelPath + " not included in " + enforcePrincipalBasedSupportedPath); } this.enforcePrincipalBasedSupportedPath = (enforcePrincipalBasedSupportedPath == null) ? null : new RepoPath(enforcePrincipalBasedSupportedPath); this.systemRelPath = systemRelPath; + this.alwaysForceSystemUserPath = alwaysForceSystemUserPath; } @Override @@ -195,7 +197,8 @@ public void addRepoinitExtention(@NotNull String source, @Nullable String repoIn @Override public void visitCreateServiceUser(CreateServiceUser createServiceUser) { recordSystemUserIds(createServiceUser.getUsername()); - }}); + } + }); } } catch (RepoInitParsingException e) { throw new ConverterException(e.getMessage(), e); @@ -203,7 +206,7 @@ public void visitCreateServiceUser(CreateServiceUser createServiceUser) { return; } try (Formatter formatter = new Formatter()) { - if (enforcePrincipalBased()) { + if (enforcePrincipalBased() || alwaysForceSystemUserPath) { List ops = new RepoInitParserService().parse(new StringReader(repoInitText)); processor.apply(ops, formatter,this); } else { @@ -222,7 +225,8 @@ public void visitCreateServiceUser(CreateServiceUser createServiceUser) { private void addUsersAndGroups(@NotNull Formatter formatter) throws ConverterException { for (SystemUser systemUser : systemUsers) { // make sure all system users are created first - CreateServiceUser operation = new CreateServiceUser(systemUser.getId(), new WithPathOptions(calculateIntermediatePath(systemUser), enforcePrincipalBased(systemUser))); + boolean withForcedPath = (alwaysForceSystemUserPath || enforcePrincipalBased(systemUser)); + CreateServiceUser operation = new CreateServiceUser(systemUser.getId(), new WithPathOptions(calculateIntermediatePath(systemUser), withForcedPath)); formatter.format("%s", operation.asRepoInitString()); if (systemUser.getDisabledReason() != null) { DisableServiceUser disable = new DisableServiceUser(systemUser.getId(), systemUser.getDisabledReason()); @@ -376,6 +380,12 @@ public boolean enforcePrincipalBased(@NotNull String systemUserId) { return false; } + @Override + public boolean enforcePath(@NotNull String systemUserId) { + // NOTE: defined by a global configuration option and thus the 'systemUserId' is ignored + return alwaysForceSystemUserPath; + } + @Override @NotNull public String calculateEnforcedIntermediatePath(@Nullable String intermediatePath) throws ConverterException { diff --git a/src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/EnforceInfo.java b/src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/EnforceInfo.java index de9c0eff..178dc452 100644 --- a/src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/EnforceInfo.java +++ b/src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/EnforceInfo.java @@ -25,8 +25,9 @@ public interface EnforceInfo { void recordSystemUserIds(@NotNull String... systemUserIds); boolean enforcePrincipalBased(@NotNull String systemUserId); + + boolean enforcePath(@NotNull String systemUserId); @NotNull String calculateEnforcedIntermediatePath(@Nullable String intermediatePath) throws ConverterException; - } \ No newline at end of file diff --git a/src/main/java/org/apache/sling/feature/cpconverter/cli/ContentPackage2FeatureModelConverterLauncher.java b/src/main/java/org/apache/sling/feature/cpconverter/cli/ContentPackage2FeatureModelConverterLauncher.java index 14948ff2..64304fb3 100644 --- a/src/main/java/org/apache/sling/feature/cpconverter/cli/ContentPackage2FeatureModelConverterLauncher.java +++ b/src/main/java/org/apache/sling/feature/cpconverter/cli/ContentPackage2FeatureModelConverterLauncher.java @@ -116,6 +116,9 @@ public final class ContentPackage2FeatureModelConverterLauncher implements Runna @Option(names= {"--system-user-rel-path"}, description = "Relative path for system user as configured with Apache Jackrabbit Oak", required = false) private String systemUserRelPath = ConverterConstants.SYSTEM_USER_REL_PATH_DEFAULT; + @Option(names = { "--enforce-system-user-path" }, description = "If this option is enabled all generated 'create service user' repoinit statements with come with 'forced path' set to make sure the intermediate path matches.", required = false) + private boolean alwaysForceSystemUserPath = false; + @Option(names = { "--enforce-servicemapping-by-principal" }, description = "Converts service user mappings with the form 'service:sub=userID' to 'service:sub=[principalname]'. Note, this may result in group membership no longer being resolved upon service login.", required = false) private boolean enforceServiceMappingByPrincipal = false; @@ -175,7 +178,7 @@ public void run() { try { try { - AclManager aclManager = new DefaultAclManager(enforcePrincipalBasedSupportedPath, systemUserRelPath); + AclManager aclManager = new DefaultAclManager(enforcePrincipalBasedSupportedPath, systemUserRelPath, alwaysForceSystemUserPath); DefaultFeaturesManager featuresManager = new DefaultFeaturesManager(mergeConfigurations, bundlesStartOrder, diff --git a/src/main/java/org/apache/sling/feature/cpconverter/repoinit/SystemUserVisitor.java b/src/main/java/org/apache/sling/feature/cpconverter/repoinit/SystemUserVisitor.java index bc599058..ea1e5ddd 100644 --- a/src/main/java/org/apache/sling/feature/cpconverter/repoinit/SystemUserVisitor.java +++ b/src/main/java/org/apache/sling/feature/cpconverter/repoinit/SystemUserVisitor.java @@ -18,6 +18,7 @@ import org.apache.sling.feature.cpconverter.ConverterException; import org.apache.sling.feature.cpconverter.accesscontrol.EnforceInfo; +import org.apache.sling.feature.cpconverter.shared.ConverterConstants; import org.apache.sling.repoinit.parser.operations.CreateServiceUser; import org.apache.sling.repoinit.parser.operations.WithPathOptions; import org.jetbrains.annotations.NotNull; @@ -43,6 +44,9 @@ public void visitCreateServiceUser(CreateServiceUser createServiceUser) { if (enforceInfo.enforcePrincipalBased(id)) { CreateServiceUser operation = new CreateServiceUser(id, new WithPathOptions(enforceInfo.calculateEnforcedIntermediatePath(path), true)); formatter.format("%s", operation.asRepoInitString()); + } else if (enforceInfo.enforcePath(id)) { + CreateServiceUser operation = new CreateServiceUser(id, new WithPathOptions(ConverterConstants.getValidSystemUserPath(path), true)); + formatter.format("%s", operation.asRepoInitString()); } else { formatter.format("%s", createServiceUser.asRepoInitString()); } diff --git a/src/main/java/org/apache/sling/feature/cpconverter/shared/ConverterConstants.java b/src/main/java/org/apache/sling/feature/cpconverter/shared/ConverterConstants.java index 94507fc3..3d909e4b 100644 --- a/src/main/java/org/apache/sling/feature/cpconverter/shared/ConverterConstants.java +++ b/src/main/java/org/apache/sling/feature/cpconverter/shared/ConverterConstants.java @@ -16,6 +16,9 @@ */ package org.apache.sling.feature.cpconverter.shared; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + public final class ConverterConstants { private ConverterConstants() {} @@ -23,4 +26,8 @@ private ConverterConstants() {} public static final String SYSTEM_USER_REL_PATH_DEFAULT = "system"; public static final String SLASH = "/"; + + public static @NotNull String getValidSystemUserPath(@Nullable String intermediatePath) { + return (intermediatePath == null || intermediatePath.isEmpty()) ? ConverterConstants.SYSTEM_USER_REL_PATH_DEFAULT : intermediatePath; + } } \ No newline at end of file diff --git a/src/test/java/org/apache/sling/feature/cpconverter/ConverterUserAndPermissionTest.java b/src/test/java/org/apache/sling/feature/cpconverter/ConverterUserAndPermissionTest.java index 6ff8aea2..c9ce8214 100644 --- a/src/test/java/org/apache/sling/feature/cpconverter/ConverterUserAndPermissionTest.java +++ b/src/test/java/org/apache/sling/feature/cpconverter/ConverterUserAndPermissionTest.java @@ -72,6 +72,7 @@ import java.util.zip.ZipEntry; import java.util.zip.ZipFile; +import static org.apache.sling.feature.cpconverter.Util.createServiceUserStatement; import static org.apache.sling.feature.cpconverter.Util.normalize; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -115,6 +116,8 @@ public class ConverterUserAndPermissionTest extends AbstractConverterTest { COMMON_NOT_EXPECTED_PATHS.add("jcr_root/home/users/system/_cq_services/demo-cp/qStDu7IQBLa95gURmer1/.content.xml"); COMMON_NOT_EXPECTED_PATHS.add("jcr_root/home/users/system/_cq_services/demo-cp/qStDu7IQBLa95gURmer1/_rep_principalPolicy.xml"); } + + private static final String SUPPORTED_PATH = "/home/users/system/cq:services"; private ContentPackage2FeatureModelConverter converter; @@ -122,23 +125,28 @@ public class ConverterUserAndPermissionTest extends AbstractConverterTest { private final AclManager aclManager; private final boolean enforcePrincipalBased; private final String withRelPath; + private final boolean alwaysEnforcePath; private File outputDirectory; private FeaturesManager featuresManager; - @Parameterized.Parameters(name = "name={2}") + @Parameterized.Parameters(name = "name={3}") public static Collection parameters() { return Arrays.asList( - new Object[] {ConverterConstants.SYSTEM_USER_REL_PATH_DEFAULT, null, "Default system user rel-path"}, - new Object[] {ConverterConstants.SYSTEM_USER_REL_PATH_DEFAULT, "/home/users/system/cq:services", "Default system user rel-path with enforce-principal-based ac-setup"}); + new Object[] {ConverterConstants.SYSTEM_USER_REL_PATH_DEFAULT, null, false, "Default system user rel-path, enforcePath=false"}, + new Object[] {ConverterConstants.SYSTEM_USER_REL_PATH_DEFAULT, null, true, "Default system user rel-path, enforcePath=true"}, + new Object[] {ConverterConstants.SYSTEM_USER_REL_PATH_DEFAULT, SUPPORTED_PATH, false, "Default system user rel-path with enforce-principal-based ac-setup, enforcePath=false"}, + new Object[] {ConverterConstants.SYSTEM_USER_REL_PATH_DEFAULT, SUPPORTED_PATH, true, "Default system user rel-path with enforce-principal-based ac-setup, enforcePath=true"}); } - public ConverterUserAndPermissionTest(@NotNull String systemUserRelPath, @Nullable String enforcePrincipalBasedSupportedPath, @NotNull String name) throws Exception { - this.aclManager = new DefaultAclManager(enforcePrincipalBasedSupportedPath, systemUserRelPath); + public ConverterUserAndPermissionTest(@NotNull String systemUserRelPath, @Nullable String enforcePrincipalBasedSupportedPath, + boolean alwaysEnforcePath, @NotNull String name) { + this.aclManager = new DefaultAclManager(enforcePrincipalBasedSupportedPath, systemUserRelPath, alwaysEnforcePath); this.handlersManager = new DefaultEntryHandlersManager(Collections.emptyMap(), false, ContentPackage2FeatureModelConverter.SlingInitialContentPolicy.KEEP, new BundleSlingInitialContentExtractor(), systemUserRelPath); this.enforcePrincipalBased = (enforcePrincipalBasedSupportedPath != null); this.withRelPath = (enforcePrincipalBased) ? enforcePrincipalBasedSupportedPath.substring(enforcePrincipalBasedSupportedPath.indexOf(systemUserRelPath)) : systemUserRelPath; + this.alwaysEnforcePath = alwaysEnforcePath; } @Before @@ -213,19 +221,19 @@ public void testMutlipePackagesWithServiceUsersAndAcls() throws Exception { Set notExpected2 = new HashSet<>(); notExpected1.add("jcr_root/content/_rep_policy.xml"); - notExpected1.add("jcr_root/var/eventproxy/_rep_policy.xml"); - notExpected1.add("jcr_root/content/"); - notExpected1.add("jcr_root/var/"); - notExpected1.add("jcr_root/var/eventproxy/"); - notExpected1.add("jcr_root/var/eventproxy/.content.xml"); - notExpected1.add("jcr_root/home/"); - notExpected1.add("jcr_root/home/users/"); - notExpected1.add("jcr_root/home/users/system/"); - notExpected1.add("jcr_root/home/users/system/eventproxy/"); - notExpected1.add("jcr_root/home/users/system/eventproxy/eventproxy-service/"); - notExpected1.add("jcr_root/home/users/system/eventproxy/eventproxy-service/.content.xml"); - notExpected1.add("jcr_root/home/users/system/eventproxy/eventproxy-service/_rep_policy.xml"); - notExpected1.add("jcr_root/home/users/system/eventproxy/.content.xml"); + notExpected1.add("jcr_root/var/eventproxy/_rep_policy.xml"); + notExpected1.add("jcr_root/content/"); + notExpected1.add("jcr_root/var/"); + notExpected1.add("jcr_root/var/eventproxy/"); + notExpected1.add("jcr_root/var/eventproxy/.content.xml"); + notExpected1.add("jcr_root/home/"); + notExpected1.add("jcr_root/home/users/"); + notExpected1.add("jcr_root/home/users/system/"); + notExpected1.add("jcr_root/home/users/system/eventproxy/"); + notExpected1.add("jcr_root/home/users/system/eventproxy/eventproxy-service/"); + notExpected1.add("jcr_root/home/users/system/eventproxy/eventproxy-service/.content.xml"); + notExpected1.add("jcr_root/home/users/system/eventproxy/eventproxy-service/_rep_policy.xml"); + notExpected1.add("jcr_root/home/users/system/eventproxy/.content.xml"); Set expected2 = new HashSet<>(); expected1.add("META-INF/MANIFEST.MF"); @@ -249,7 +257,7 @@ public void testMutlipePackagesWithServiceUsersAndAcls() throws Exception { " end\n"), normalize(repoinit.getText())); } else { assertEquals(normalize( - "create service user eventproxy-service with path system/eventproxy\n" + + createServiceUserStatement(alwaysEnforcePath, "eventproxy-service", "system/eventproxy") + " create path /content\n" + " create path /var/eventproxy(nt:unstructured mixin rep:AccessControllable)\n" + " set ACL for eventproxy-service\n" + @@ -271,7 +279,7 @@ public void testMutlipePackagesWithServiceUsersAndAcls() throws Exception { " end\n"), normalize(repoinit.getText())); } else { assertEquals(normalize( - " create service user eventproxy-service2 with path system/eventproxy\n" + + createServiceUserStatement(alwaysEnforcePath, "eventproxy-service2", "system/eventproxy") + " set ACL for eventproxy-service2\n" + " allow jcr:read,rep:write on home(eventproxy-service2)\n" + " end\n"), normalize(repoinit.getText())); @@ -483,7 +491,10 @@ private void verifyRepoInit() throws RepoInitParsingException { return csu.isForcedPath() && csu.getPath().startsWith(withRelPath); })); } else { - assertFalse(createServiceUsers.stream().anyMatch(operation -> ((CreateServiceUser) operation).isForcedPath())); + boolean matchingForcedPath = (alwaysEnforcePath) ? + createServiceUsers.stream().allMatch(operation -> ((CreateServiceUser) operation).isForcedPath()) : + createServiceUsers.stream().noneMatch(operation -> ((CreateServiceUser) operation).isForcedPath()); + assertTrue(matchingForcedPath); assertEquals(2, createServiceUsers.stream().filter(operation -> ((CreateServiceUser) operation).getPath().startsWith(withRelPath+"/demo-cp")).count()); } diff --git a/src/test/java/org/apache/sling/feature/cpconverter/Util.java b/src/test/java/org/apache/sling/feature/cpconverter/Util.java index f1e4c3c6..3e9a33bc 100644 --- a/src/test/java/org/apache/sling/feature/cpconverter/Util.java +++ b/src/test/java/org/apache/sling/feature/cpconverter/Util.java @@ -20,6 +20,7 @@ import org.apache.sling.repoinit.parser.RepoInitParsingException; import org.apache.sling.repoinit.parser.impl.RepoInitParserService; import org.apache.sling.repoinit.parser.operations.Operation; +import org.jetbrains.annotations.NotNull; import java.io.StringReader; import java.util.Formatter; @@ -44,4 +45,12 @@ public static String normalizeUnchecked(String repoinit) { throw new RuntimeException(e); } } + + public static @NotNull String createServiceUserStatement(boolean enforcePath, @NotNull String id, @NotNull String intermediatePath) { + if (enforcePath) { + return "create service user "+id+" with forced path "+intermediatePath+"\n"; + } else { + return "create service user "+id+" with path "+intermediatePath+"\n"; + } + } } 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 6730c885..1f6f8d25 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 @@ -16,6 +16,7 @@ */ package org.apache.sling.feature.cpconverter.accesscontrol; +import static org.apache.sling.feature.cpconverter.Util.createServiceUserStatement; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; @@ -31,6 +32,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.Comparator; import java.util.List; @@ -43,24 +45,43 @@ import org.apache.sling.feature.cpconverter.Util; import org.apache.sling.feature.cpconverter.features.DefaultFeaturesManager; import org.apache.sling.feature.cpconverter.features.FeaturesManager; +import org.apache.sling.feature.cpconverter.shared.ConverterConstants; import org.apache.sling.feature.cpconverter.shared.RepoPath; import org.apache.sling.feature.cpconverter.vltpkg.VaultPackageAssembler; import org.apache.sling.repoinit.parser.RepoInitParser; import org.apache.sling.repoinit.parser.impl.RepoInitParserService; import org.apache.sling.repoinit.parser.operations.CreatePath; import org.apache.sling.repoinit.parser.operations.Operation; +import org.jetbrains.annotations.NotNull; import org.junit.After; import org.junit.Before; import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; import org.mockito.Mockito; +@RunWith(Parameterized.class) public class AclManagerTest { + + private final boolean enforcePath; + private AclManager aclManager; private Path tempDir; + + @Parameterized.Parameters(name = "name={1}") + public static Collection parameters() { + return Arrays.asList( + new Object[] {true, "Always enforce system user path = true"}, + new Object[] {false, "Always enforce system user path = false"}); + } + public AclManagerTest(boolean enforcePath, @NotNull String name) { + this.enforcePath = enforcePath; + } + @Before public void setUp() throws Exception { - aclManager = new DefaultAclManager(); + aclManager = new DefaultAclManager(null, ConverterConstants.SYSTEM_USER_REL_PATH_DEFAULT, enforcePath); tempDir = Files.createTempDirectory(getClass().getSimpleName()); } @@ -100,7 +121,7 @@ public void makeSureAclsAreCreatedOnlyoutsideSytemUsersPaths() throws Exception // acs-commons-on-deploy-scripts-service will be missed String expected = "# origin= source=content-package" + System.lineSeparator() + Util.normalize( - "create service user acs-commons-package-replication-status-event-service with path system\n" + + createServiceUserStatement(enforcePath, "acs-commons-package-replication-status-event-service", "system") + "create path /sling:tests/not(nt:unstructured mixin rep:AccessControllable,mix:created)/system/user/path\n" + "set ACL for acs-commons-package-replication-status-event-service\n" + " allow jcr:read,rep:write,rep:indexDefinitionManagement on /sling:tests/not/system/user/path\n" + @@ -142,7 +163,7 @@ public void testReset() throws Exception { // aacs-commons-ensure-oak-index-service will be missed String expected = "# origin= source=content-package" + System.lineSeparator() + Util.normalize( - "create service user acs-commons-package-replication-status-event-service with path system\n" + + createServiceUserStatement(enforcePath, "acs-commons-package-replication-status-event-service","system") + "create path /sling:tests/not(nt:unstructured mixin rep:AccessControllable,mix:created)/system/user/path\n" + "set ACL for acs-commons-package-replication-status-event-service\n" + " allow jcr:read,rep:write,rep:indexDefinitionManagement on /sling:tests/not/system/user/path\n" + @@ -192,7 +213,7 @@ public void pathWithSpecialCharactersTest() throws Exception { assertNotNull(repoinitExtension); String expected = "# origin= source=content-package" + System.lineSeparator() + Util.normalize( - "create service user sys-usr with path system\n" + + createServiceUserStatement(enforcePath, "sys-usr", "system") + "create path /content/cq:tags\n"+ "set ACL for sys-usr\n" + " allow jcr:read on /content/cq:tags\n" + @@ -244,7 +265,7 @@ public void testGroupHandlingWithGroupNotUsed() throws Exception { assertNotNull(repoinitExtension); String expected = "# origin= source=content-package" + System.lineSeparator() + Util.normalize( - "create service user sys-usr with path system\n" + + createServiceUserStatement(enforcePath, "sys-usr", "system") + "create path /content/test\n" + "set ACL for sys-usr\n" + " allow jcr:read on /content/test\n" + @@ -304,7 +325,7 @@ public void testUserHandlingWithNonMatchingUser() throws Exception { assertNotNull(repoinitExtension); String expected = "# origin= source=content-package" + System.lineSeparator() + Util.normalize( - "create service user sys-usr with path system\n" + + createServiceUserStatement(enforcePath, "sys-usr", "system") + "create path /content/test\n" + "set ACL for sys-usr\n" + " allow jcr:read on /content/test\n" + @@ -336,7 +357,7 @@ public void testPathHandlingWithUser() throws Exception { // as user-home-path and thus is processed like a regular path (no 'home(uid)' repo-init statement and no exception).\ // however, no attempt is made to create the path without any available node type information. String expected = "# origin= source=content-package" + System.lineSeparator() + Util.normalize( - "create service user sys-usr with path system\n" + + createServiceUserStatement(enforcePath, "sys-usr", "system") + "set ACL for sys-usr\n" + " allow jcr:read on /home/users/notMatching\n" + "end\n"); @@ -347,14 +368,14 @@ public void testPathHandlingWithUser() throws Exception { @Test(expected = ConverterException.class) public void testAddRepoinitExtentionInvalidTxt() throws Exception { - DefaultAclManager aclManager = new DefaultAclManager("/home/users/system/cq:services", "system"); + DefaultAclManager aclManager = new DefaultAclManager("/home/users/system/cq:services", "system", false); aclManager.addRepoinitExtention("test", "some invalid txt", null, mock(FeaturesManager.class)); } @Test public void testAddRepoinitExtentionEmptyTxt()throws Exception { FeaturesManager fm = mock(FeaturesManager.class); - DefaultAclManager aclManager = new DefaultAclManager("/home/users/system/cq:services", "system"); + DefaultAclManager aclManager = new DefaultAclManager("/home/users/system/cq:services", "system", false); aclManager.addRepoinitExtention("test", "", null, fm); verifyNoInteractions(fm); @@ -363,7 +384,7 @@ public void testAddRepoinitExtentionEmptyTxt()throws Exception { @Test public void testAddRepoinitExtentionNullTxt() throws Exception { FeaturesManager fm = mock(FeaturesManager.class); - DefaultAclManager aclManager = new DefaultAclManager("/home/users/system/cq:services", "system"); + DefaultAclManager aclManager = new DefaultAclManager("/home/users/system/cq:services", "system", false); aclManager.addRepoinitExtention("test", null, null, fm); verifyNoInteractions(fm); diff --git a/src/test/java/org/apache/sling/feature/cpconverter/accesscontrol/EnforceInfoTest.java b/src/test/java/org/apache/sling/feature/cpconverter/accesscontrol/EnforceInfoTest.java index 953587fb..b2bd79de 100644 --- a/src/test/java/org/apache/sling/feature/cpconverter/accesscontrol/EnforceInfoTest.java +++ b/src/test/java/org/apache/sling/feature/cpconverter/accesscontrol/EnforceInfoTest.java @@ -17,19 +17,30 @@ package org.apache.sling.feature.cpconverter.accesscontrol; import org.apache.sling.feature.cpconverter.ConverterException; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; import org.junit.Test; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +import static org.apache.sling.feature.cpconverter.shared.ConverterConstants.SYSTEM_USER_REL_PATH_DEFAULT; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; public class EnforceInfoTest { + private static final String SUPPORTED_PATH = "/home/users/system/cq:services"; + + private static DefaultAclManager createEnforceInfo(@Nullable String enforcePrincipalBasedSupportedPath, @NotNull String systemRelPath) { + return createEnforceInfo(enforcePrincipalBasedSupportedPath, systemRelPath, false); + } + + private static DefaultAclManager createEnforceInfo(@Nullable String enforcePrincipalBasedSupportedPath, @NotNull String systemRelPath, boolean alwaysForceSystemUserPath) { + return new DefaultAclManager(enforcePrincipalBasedSupportedPath, systemRelPath, alwaysForceSystemUserPath); + } + @Test public void testEnforcePrincipalBasedMissingPath() { - DefaultAclManager aclManager = new DefaultAclManager(null, "system"); + DefaultAclManager aclManager = createEnforceInfo(null, "system"); assertFalse(aclManager.enforcePrincipalBased("systemuser")); aclManager.recordSystemUserIds("systemuser"); @@ -38,7 +49,7 @@ public void testEnforcePrincipalBasedMissingPath() { @Test public void testEnforcePrincipalBased() { - DefaultAclManager aclManager = new DefaultAclManager("/home/users/system/cq:services", "system"); + DefaultAclManager aclManager = createEnforceInfo(SUPPORTED_PATH, "system"); assertFalse(aclManager.enforcePrincipalBased("systemuser")); aclManager.recordSystemUserIds("systemuser"); @@ -47,7 +58,7 @@ public void testEnforcePrincipalBased() { @Test public void testEnforcePrincipalBasedMappedById() { - DefaultAclManager aclManager = new DefaultAclManager("/home/users/system/cq:services", "system"); + DefaultAclManager aclManager = createEnforceInfo(SUPPORTED_PATH, "system"); aclManager.addMapping(new Mapping("service:subservice=systemuser", false)); aclManager.recordSystemUserIds("systemuser"); assertFalse(aclManager.enforcePrincipalBased("systemuser")); @@ -55,7 +66,7 @@ public void testEnforcePrincipalBasedMappedById() { @Test public void testEnforcePrincipalBasedMappedByIdEnforceConversion() { - DefaultAclManager aclManager = new DefaultAclManager("/home/users/system/cq:services", "system"); + DefaultAclManager aclManager = createEnforceInfo(SUPPORTED_PATH, "system"); aclManager.addMapping(new Mapping("service:subservice=systemuser", true)); aclManager.recordSystemUserIds("systemuser"); assertTrue(aclManager.enforcePrincipalBased("systemuser")); @@ -63,7 +74,7 @@ public void testEnforcePrincipalBasedMappedByIdEnforceConversion() { @Test public void testCalculateEnforcedIntermediateMissingPath() throws Exception { - DefaultAclManager aclManager = new DefaultAclManager("/home/users/system/cq:services", "system"); + DefaultAclManager aclManager = createEnforceInfo(SUPPORTED_PATH, "system"); assertEquals("system/cq:services", aclManager.calculateEnforcedIntermediatePath(null)); assertEquals("system/cq:services", aclManager.calculateEnforcedIntermediatePath("")); @@ -71,7 +82,7 @@ public void testCalculateEnforcedIntermediateMissingPath() throws Exception { @Test public void testCalculateEnforcedIntermediatePathSubTree() throws Exception { - DefaultAclManager aclManager = new DefaultAclManager("/home/users/system/cq:services", "system"); + DefaultAclManager aclManager = createEnforceInfo(SUPPORTED_PATH, SYSTEM_USER_REL_PATH_DEFAULT); assertEquals("system/cq:services/some/path", aclManager.calculateEnforcedIntermediatePath("/home/users/system/cq:services/some/path")); assertEquals("system/cq:services/some/path", aclManager.calculateEnforcedIntermediatePath("/home/users/system/some/path")); @@ -79,21 +90,39 @@ public void testCalculateEnforcedIntermediatePathSubTree() throws Exception { @Test public void testCalculateEnforcedIntermediatePath() throws Exception { - DefaultAclManager aclManager = new DefaultAclManager("/home/users/system/cq:services", "system"); + DefaultAclManager aclManager = createEnforceInfo(SUPPORTED_PATH, SYSTEM_USER_REL_PATH_DEFAULT); - assertEquals("system/cq:services", aclManager.calculateEnforcedIntermediatePath("/home/users/system/cq:services")); + assertEquals("system/cq:services", aclManager.calculateEnforcedIntermediatePath(SUPPORTED_PATH)); assertEquals("system/cq:services", aclManager.calculateEnforcedIntermediatePath("/home/users/system")); } @Test(expected = IllegalStateException.class) public void testCalculateEnforcedIntermediatePathMissingPath() throws Exception { - DefaultAclManager aclManager = new DefaultAclManager(null, "system"); + DefaultAclManager aclManager = createEnforceInfo(null, SYSTEM_USER_REL_PATH_DEFAULT); aclManager.calculateEnforcedIntermediatePath("/home/users/system/some/path"); } @Test(expected = ConverterException.class) public void testCalculateEnforcedIntermediatePathOutsideSupportedScope() throws Exception { - DefaultAclManager aclManager = new DefaultAclManager("/home/users/system/cq:services", "system"); + DefaultAclManager aclManager = createEnforceInfo(SUPPORTED_PATH, SYSTEM_USER_REL_PATH_DEFAULT); aclManager.calculateEnforcedIntermediatePath("/home/users"); } + + @Test + public void testEnforcePath() { + DefaultAclManager aclManager = new DefaultAclManager(); + assertFalse(aclManager.enforcePath("id")); + + aclManager = createEnforceInfo(SUPPORTED_PATH, SYSTEM_USER_REL_PATH_DEFAULT, false); + assertFalse(aclManager.enforcePath("id")); + + aclManager = createEnforceInfo(SUPPORTED_PATH, SYSTEM_USER_REL_PATH_DEFAULT, true); + assertTrue(aclManager.enforcePath("id")); + + aclManager = createEnforceInfo(null, SYSTEM_USER_REL_PATH_DEFAULT, false); + assertFalse(aclManager.enforcePath("id")); + + aclManager = createEnforceInfo(null, SYSTEM_USER_REL_PATH_DEFAULT, true); + assertTrue(aclManager.enforcePath("id")); + } } \ No newline at end of file diff --git a/src/test/java/org/apache/sling/feature/cpconverter/accesscontrol/EnforcePrincipalBasedTest.java b/src/test/java/org/apache/sling/feature/cpconverter/accesscontrol/EnforcePrincipalBasedTest.java index 3e621372..a10076cc 100644 --- a/src/test/java/org/apache/sling/feature/cpconverter/accesscontrol/EnforcePrincipalBasedTest.java +++ b/src/test/java/org/apache/sling/feature/cpconverter/accesscontrol/EnforcePrincipalBasedTest.java @@ -67,7 +67,7 @@ public class EnforcePrincipalBasedTest { @Before public void setUp() throws Exception { - aclManager = new DefaultAclManager("/home/users/system/some/subtree", "system"); + aclManager = new DefaultAclManager("/home/users/system/some/subtree", "system", false); tempDir = Files.createTempDirectory(getClass().getSimpleName()); assembler = mock(VaultPackageAssembler.class); @@ -91,19 +91,19 @@ public void tearDown() throws Exception { @Test(expected = ConverterException.class) public void testInvalidSupportedPath() throws Exception { - AclManager acMgr = new DefaultAclManager("/an/invalid/supported/path", "invalid"); + AclManager acMgr = new DefaultAclManager("/an/invalid/supported/path", "invalid", false); RepoPath accessControlledPath = new RepoPath("/content/feature"); getRepoInitExtension(acMgr, accessControlledPath, systemUser, false); } @Test(expected = RuntimeException.class) public void testPathMismatch() throws RuntimeException { - new DefaultAclManager("/an/invalid/supported/path", "system"); + new DefaultAclManager("/an/invalid/supported/path", "system", false); } @Test public void testResourceBasedConversionWithoutForce() throws Exception { - AclManager acMgr = new DefaultAclManager(null, "system") { + AclManager acMgr = new DefaultAclManager(null, "system", false) { @Override protected @Nullable CreatePath getCreatePath(@NotNull RepoPath path, @NotNull List packageAssemblers) { CreatePath cp = new CreatePath(null); diff --git a/src/test/java/org/apache/sling/feature/cpconverter/handlers/RepoInitTest.java b/src/test/java/org/apache/sling/feature/cpconverter/handlers/RepoInitTest.java index 67dbd831..eb685990 100644 --- a/src/test/java/org/apache/sling/feature/cpconverter/handlers/RepoInitTest.java +++ b/src/test/java/org/apache/sling/feature/cpconverter/handlers/RepoInitTest.java @@ -56,20 +56,24 @@ public class RepoInitTest { private final AbstractConfigurationEntryHandler configurationEntryHandler; private final boolean enforcePrincipalBasedAcSetup; private final String enforcedPath; + private final boolean alwaysForceSystemUserPath; private final String name; - @Parameterized.Parameters(name = "name={1}") + @Parameterized.Parameters(name = "name={2}") public static Collection parameters() { return Arrays.asList( - new Object[] { true, "Enforce principal-based ac setup" }, - new Object[] { false, "Don't enforce principal-based ac setup" }); - }; + new Object[] { true, false, "Enforce principal-based ac setup, Force system user path = false" }, + new Object[] { true, true, "Enforce principal-based ac setup, Force system user path = true" }, + new Object[] { false, false, "Don't enforce principal-based ac setup, Force system user path = false" }, + new Object[] { false, true, "Don't enforce principal-based ac setup, Force system user path = true" }); + } - public RepoInitTest(boolean enforcePrincipalBasedAcSetup, String name) { + public RepoInitTest(boolean enforcePrincipalBasedAcSetup, boolean alwaysForceSystemUserPath, String name) { this.configurationEntryHandler = new ConfigurationEntryHandler(); this.enforcePrincipalBasedAcSetup = enforcePrincipalBasedAcSetup; this.enforcedPath = (enforcePrincipalBasedAcSetup) ? "/home/users/system/cq:services" : null; + this.alwaysForceSystemUserPath = alwaysForceSystemUserPath; this.name = name; } @@ -165,7 +169,7 @@ private Extension extractExtensions(@NotNull String path, boolean enforcePrincip doCallRealMethod().when(featuresManager).addConfiguration(anyString(), any(), anyString(), any()); when(featuresManager.getRunMode(anyString())).thenReturn(feature); - AclManager aclManager = spy(new DefaultAclManager((enforcePrincipalBasedAcSetup) ? enforcedPath : null, "system")); + AclManager aclManager = spy(new DefaultAclManager((enforcePrincipalBasedAcSetup) ? enforcedPath : null, "system", alwaysForceSystemUserPath)); if (addMappingById) { aclManager.addMapping(new Mapping("org.apache.sling.testbundle:sub1=su1")); aclManager.addMapping(new Mapping("org.apache.sling.testbundle:sub2=su2")); From 6ec10c3047fdc7ced26195840a451dd40cdbde65 Mon Sep 17 00:00:00 2001 From: angela Date: Tue, 8 Nov 2022 13:24:16 +0100 Subject: [PATCH 2/2] SLING-11663 : avoid removing existing constructor but instead deprecate it --- .../accesscontrol/DefaultAclManager.java | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) 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 862e638a..b6650859 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 @@ -96,10 +96,32 @@ public class DefaultAclManager implements AclManager, EnforceInfo { private RepoPath userRootPath; + /** + * Same as {@code DefaultAclManager(null, "system", false)} + * @see ConverterConstants#SYSTEM_USER_REL_PATH_DEFAULT + */ public DefaultAclManager() { this(null, ConverterConstants.SYSTEM_USER_REL_PATH_DEFAULT, false); } - + + /** + * @param enforcePrincipalBasedSupportedPath The supported path if principal-based access control setup for service users should be enforced; {@code null} otherwise. + * @param systemRelPath The relative intermediate path used for all system users. + * @deprecated Use DefaultAclManager(String,String,boolean) instead + */ + @Deprecated + public DefaultAclManager(@Nullable String enforcePrincipalBasedSupportedPath, @NotNull String systemRelPath) { + this(enforcePrincipalBasedSupportedPath, systemRelPath, false); + log.warn("Deprecated. Please refactor to use DefaultAclManager(String,String,boolean) instead"); + } + + /** + * Creates a new instance of {@code DefaultAclManager}. + * + * @param enforcePrincipalBasedSupportedPath The supported path if principal-based access control setup for service users should be enforced; {@code null} otherwise. + * @param systemRelPath The relative intermediate path used for all system users. + * @param alwaysForceSystemUserPath Option to make sure all system users are being created with the specified intermediate path (i.e. translating to 'with forced path' statements in repoinit). + */ public DefaultAclManager(@Nullable String enforcePrincipalBasedSupportedPath, @NotNull String systemRelPath, boolean alwaysForceSystemUserPath) { if (enforcePrincipalBasedSupportedPath != null && !enforcePrincipalBasedSupportedPath.contains(systemRelPath)) { throw new RuntimeException("Relative path for system users "+ systemRelPath + " not included in " + enforcePrincipalBasedSupportedPath);