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 2021/01/15 12:24:29 UTC
[GitHub] [sling-org-apache-sling-feature-cpconverter] anchela commented on a change in pull request #52: SLING-9692: Add support for principal-based access control entries
anchela commented on a change in pull request #52:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/52#discussion_r558266803
##########
File path: src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/DefaultAclManager.java
##########
@@ -166,32 +194,46 @@ private void addStatements(@NotNull SystemUser systemUser,
return;
}
- Map<AccessControlEntry, String> entries = new LinkedHashMap<>();
+ Map<AccessControlEntry, String> resourceEntries = new LinkedHashMap<>();
+ Map<AccessControlEntry, String> principalEntries = new LinkedHashMap<>();
+
authorizations.forEach(entry -> {
String path = getRepoInitPath(entry.getRepositoryPath(), systemUser);
if (path != null) {
- entries.put(entry, path);
+ if (enforcePrincipalBased || entry.isPrincipalBased()) {
+ principalEntries.put(entry, path);
+ } else {
+ resourceEntries.put(entry, path);
+ }
}
});
- 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()));
- }
- formatter.format("%n");
- });
+ if (!principalEntries.isEmpty()) {
+ formatter.format("set principal ACL for %s%n", systemUser.getId());
+ principalEntries.forEach((entry, path) -> writeEntry(entry, path, formatter));
+ formatter.format("end%n");
+ }
+ if (!enforcePrincipalBased && !resourceEntries.isEmpty()) {
Review comment:
isn't` !enforcePrincipalBased` redundant here?
##########
File path: src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/DefaultAclManager.java
##########
@@ -122,6 +134,8 @@ public void addRepoinitExtension(@NotNull List<VaultPackageAssembler> packageAss
.filter(entry -> getSystemUser(entry.getKey()).isPresent())
.map(Entry::getValue)
.flatMap(Collection::stream)
+ // paths only should/need to be create with resource-based access control
+ .filter(((Predicate<AccessControlEntry>) AccessControlEntry::isPrincipalBased).negate())
Review comment:
if principal-based ac-setup is enforced for all service user permissions (and thus resource-based entries get converted to principal-based), no paths should be created either.... so in addition to filtering out principal-based entries, the whole block might be excluded if the enforce-prinicipal-based-ac-setup is turned on.
and we should have a test verifying this :)
##########
File path: src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/DefaultAclManager.java
##########
@@ -51,6 +51,9 @@
private static final String CONTENT_XML_FILE_NAME = ".content.xml";
+ private final boolean enforcePrincipalBased;
+ private RepoPath supportedPrincipalBasedPath;
Review comment:
may be final
##########
File path: src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/DefaultAclManager.java
##########
@@ -166,32 +194,46 @@ private void addStatements(@NotNull SystemUser systemUser,
return;
}
- Map<AccessControlEntry, String> entries = new LinkedHashMap<>();
+ Map<AccessControlEntry, String> resourceEntries = new LinkedHashMap<>();
+ Map<AccessControlEntry, String> principalEntries = new LinkedHashMap<>();
+
authorizations.forEach(entry -> {
String path = getRepoInitPath(entry.getRepositoryPath(), systemUser);
if (path != null) {
- entries.put(entry, path);
+ if (enforcePrincipalBased || entry.isPrincipalBased()) {
+ principalEntries.put(entry, path);
+ } else {
+ resourceEntries.put(entry, path);
+ }
}
});
- 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()));
- }
- formatter.format("%n");
- });
+ if (!principalEntries.isEmpty()) {
+ formatter.format("set principal ACL for %s%n", systemUser.getId());
+ principalEntries.forEach((entry, path) -> writeEntry(entry, path, formatter));
+ formatter.format("end%n");
+ }
+ if (!enforcePrincipalBased && !resourceEntries.isEmpty()) {
+ formatter.format("set ACL for %s%n", systemUser.getId());
+ resourceEntries.forEach((entry, path) -> writeEntry(entry, path, formatter));
formatter.format("end%n");
}
}
+ private void writeEntry(AccessControlEntry entry, String path, Formatter formatter) {
Review comment:
missing notnull annotations
##########
File path: src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/DefaultAclManager.java
##########
@@ -166,32 +194,46 @@ private void addStatements(@NotNull SystemUser systemUser,
return;
}
- Map<AccessControlEntry, String> entries = new LinkedHashMap<>();
+ Map<AccessControlEntry, String> resourceEntries = new LinkedHashMap<>();
+ Map<AccessControlEntry, String> principalEntries = new LinkedHashMap<>();
+
authorizations.forEach(entry -> {
String path = getRepoInitPath(entry.getRepositoryPath(), systemUser);
if (path != null) {
Review comment:
can/will `getRepoInitPath` ever return null? maybe it has the wrong nullability annotation?
##########
File path: src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/DefaultAclManager.java
##########
@@ -150,6 +164,20 @@ public void addRepoinitExtension(@NotNull List<VaultPackageAssembler> packageAss
}
}
+ private String calculateIntermediatePath(RepoPath intermediatePath) {
+ if (enforcePrincipalBased && supportedPrincipalBasedPath!= null && !intermediatePath.startsWith(supportedPrincipalBasedPath)) {
+ RepoPath parent = supportedPrincipalBasedPath.getParent();
+ if (parent.equals(intermediatePath)) {
+ return supportedPrincipalBasedPath.toString();
+ } else {
+ String relpath = intermediatePath.toString().substring(parent.toString().length());
+ return supportedPrincipalBasedPath.toString() + relpath;
+ }
+ } else {
+ return intermediatePath.toString();
+ }
+ }
+
private boolean aclStartsWith(RepoPath path) {
Review comment:
this method is also called for groups, where we only wanted to create groups if a policy is located at the group-node (but not below).... the subsequent test for 'isBelow' will the cause the failure.... but IMO the correct statement here would to check if there is a acl whose access controlled path equals to the group path.
for users the starts-with is correct...
##########
File path: src/test/java/org/apache/sling/feature/cpconverter/accesscontrol/EnforcePrincipalBasedTest.java
##########
@@ -0,0 +1,163 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.sling.feature.cpconverter.accesscontrol;
+
+import org.apache.sling.feature.ArtifactId;
+import org.apache.sling.feature.Extension;
+import org.apache.sling.feature.Feature;
+import org.apache.sling.feature.cpconverter.features.DefaultFeaturesManager;
+import org.apache.sling.feature.cpconverter.features.FeaturesManager;
+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.RepoInitParsingException;
+import org.apache.sling.repoinit.parser.impl.RepoInitParserService;
+import org.apache.sling.repoinit.parser.operations.Operation;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.io.File;
+import java.io.StringReader;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.Comparator;
+import java.util.List;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class EnforcePrincipalBasedTest {
+
+ private final SystemUser systemUser = new SystemUser("user1", new RepoPath("/home/users/system/intermediate/usernode"), new RepoPath("/home/users/system/intermediate"));
+
+ private AclManager aclManager;
+ private Path tempDir;
+
+ private VaultPackageAssembler assembler;
+ private FeaturesManager fm;
+ private Feature feature;
+
+ @Before
+ public void setUp() throws Exception {
+ aclManager = new DefaultAclManager(true, "/home/users/system/cq:services");
+ tempDir = Files.createTempDirectory(getClass().getSimpleName());
+
+ assembler = mock(VaultPackageAssembler.class);
+ when(assembler.getEntry(anyString())).thenReturn(new File(System.getProperty("java.io.tmpdir")));
+ feature = new Feature(new ArtifactId("org.apache.sling", "org.apache.sling.cp2fm", "0.0.1", null, null));
+
+ fm = Mockito.spy(new DefaultFeaturesManager(tempDir.toFile()));
+ when(fm.getTargetFeature()).thenReturn(feature);
+ }
+
+ @After
+ public void tearDown() throws Exception {
+ aclManager = null;
+
+ // Delete the temp dir again
+ Files.walk(tempDir)
+ .sorted(Comparator.reverseOrder())
+ .map(Path::toFile)
+ .forEach(File::delete);
+ }
+
+ @Test
+ public void testResourceBasedConversion() throws RepoInitParsingException {
+ aclManager.addSystemUser(systemUser);
+
+ RepoPath accessControlledPath = new RepoPath("/content/feature");
+ aclManager.addAcl("user1", new AccessControlEntry(true, "jcr:read", accessControlledPath , false));
+
+ aclManager.addRepoinitExtension(Arrays.asList(assembler), fm);
+
+ Extension repoinitExtension = feature.getExtensions().getByName(Extension.EXTENSION_NAME_REPOINIT);
+ assertNotNull(repoinitExtension);
+
+ String expected =
+ "create service user user1 with path /home/users/system/cq:services/intermediate" + System.lineSeparator() +
+ "set principal ACL for user1" + System.lineSeparator() +
+ "allow jcr:read on /content/feature" + 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 testPrincipalBased() throws RepoInitParsingException {
+ aclManager.addSystemUser(systemUser);
+
+ RepoPath accessControlledPath = new RepoPath("/content/feature");
+ aclManager.addAcl("user1", new AccessControlEntry(true, "jcr:read", accessControlledPath, true));
+
+ aclManager.addRepoinitExtension(Arrays.asList(assembler), fm);
+
+ Extension repoinitExtension = feature.getExtensions().getByName(Extension.EXTENSION_NAME_REPOINIT);
+ assertNotNull(repoinitExtension);
+
+ String expected =
+ "create service user user1 with path /home/users/system/cq:services/intermediate" + System.lineSeparator() +
+ "set principal ACL for user1" + System.lineSeparator() +
+ "allow jcr:read on /content/feature" + 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 testPrincipalBasedForUserHome() throws RepoInitParsingException {
+ aclManager.addSystemUser(systemUser);
+
+ RepoPath accessControlledPath = new RepoPath("/home/users/system/cq:services/intermediate/usernode");
+ AccessControlEntry acl = new AccessControlEntry(true, "jcr:read", accessControlledPath, true);
+ aclManager.addAcl("user1", acl);
+
+ aclManager.addRepoinitExtension(Arrays.asList(assembler), fm);
+
+ Extension repoinitExtension = feature.getExtensions().getByName(Extension.EXTENSION_NAME_REPOINIT);
+ assertNotNull(repoinitExtension);
+
+ String expected =
+ "create service user user1 with path /home/users/system/cq:services/intermediate" + System.lineSeparator() +
+ "set principal ACL for user1" + System.lineSeparator() +
+ "allow jcr:read on home(user1)" + 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());
+ }
+}
Review comment:
oh... seems to lack a new line
##########
File path: src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/DefaultAclManager.java
##########
@@ -150,6 +164,20 @@ public void addRepoinitExtension(@NotNull List<VaultPackageAssembler> packageAss
}
}
+ private String calculateIntermediatePath(RepoPath intermediatePath) {
Review comment:
missing notnull
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org