anchela commented on a change in pull request #51:
URL:
https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/51#discussion_r557103532
##########
File path:
src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/DefaultAclManager.java
##########
@@ -139,18 +131,34 @@ private void addStatements(@NotNull SystemUser systemUser,
@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);
Review comment:
i would suggest to leave the `addAclStatement` call (and the addition
methods called from there).... the reason for this: once we add support for
principal-based access control setup (SLING-9692) there will be a bit more
complexity to sort out which parts are principal-based and which are
resource-based, while the repo-init statements for the entries remain
identical.... see patch attached to SLING-9692 for illustration
##########
File path:
src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/DefaultAclManager.java
##########
@@ -53,6 +53,9 @@
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<>();
Review comment:
this field is no longer used (only spotted it in the checkout of the
branch)
##########
File path:
src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/DefaultAclManager.java
##########
@@ -215,44 +223,47 @@ public void reset() {
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);
Review comment:
see above: while all user/group nodes are located below the configured
user/group homes there are also other nodes there that don't need the special
home(id) repo-init statement.....usually those are the authorizable folders.
but by means of mixins also other types of nodes might find their way to the
user/group tree structure.
##########
File path:
src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/DefaultAclManager.java
##########
@@ -215,44 +223,47 @@ public void reset() {
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();
Review comment:
access control entries defined for user/group homes must never reference
a path but should always be in the format `home(id)`. sling repo-init has no
ability to control where a given user/group is being created/located as this is
an implementation detail. if you look at jackrabbit/fvault code you will notice
that it contains a fair amount of extra logic to keep track of user/group nodes
and ultimately uses JCR xml-import that delegates the creation to the
repository. since Sling repo-init relying on the user/group path to be stable
when converting from content packages to repo-init won't work.
without having carefully thought it through i see the following options to
do this:
1. adjust SystemUserHandler to deal with all kind of users/groups not just
system users (or create seperate handleres for regular users and groups) and
push all types of users/groups to the aclmanager (i.e. addSystemUser, addUser,
addGroup)
2. retrieve the information about regular users and groups from the content
package on demand as it is done for the primary type and mixin types.
if the content package contains an policy located on a user/group node but
doesn't reveal the id of the user/group (i.e. missing .content.xml), the
converter should abort as there is pretty much nothing it could do to properly
setup the access control entries. silently ignoring it will lead to an
incomplete ac setup which might leave the application disfunctional or in the
worst case introduce vulnerabilities.
there is one notable exception (and here the method isUserGroupPath isn't
really accurate: it might be that a given ac policy is actually located at an
authorizable folder.... in this case `path.toString` would be correct.
##########
File path:
src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/DefaultAclManager.java
##########
@@ -53,6 +53,9 @@
private static final String DEFAULT_TYPE = "sling:Folder";
+ // FIXME: SLING-9969. avoid hardcoding
+ private static final RepoPath USER_GROUP_ROOT = new RepoPath("/home");
Review comment:
if there were handlers in place the listen to all kind of user/group
nodes, this could be avoided as the aclmanager could keep a map of
user-group-paths to user-group-objects.
##########
File path:
src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/DefaultAclManager.java
##########
@@ -215,44 +223,47 @@ public void reset() {
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) {
Review comment:
see above
##########
File path:
src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/DefaultAclManager.java
##########
@@ -215,44 +223,47 @@ public void reset() {
return type ? new RepoPath(current).toString() : null;
}
- private static void addAclStatement(@NotNull Formatter formatter, @NotNull
SystemUser systemUser, @NotNull List<AccessControlEntry> authorizations) {
Review comment:
see above
##########
File path:
src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/DefaultAclManager.java
##########
@@ -53,6 +53,9 @@
private static final String DEFAULT_TYPE = "sling:Folder";
Review comment:
this constant is never used (spotted in the checkout of the branch)
##########
File path:
src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/DefaultAclManager.java
##########
@@ -53,6 +53,9 @@
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<>();
Review comment:
this field is no longer used (spotted in the checkout of the branch)
----------------------------------------------------------------
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]
