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:
[email protected]
