anchela commented on a change in pull request #40:
URL:
https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/40#discussion_r539376904
##########
File path:
src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/DefaultAclManager.java
##########
@@ -149,7 +149,7 @@ private void addStatements(SystemUser systemUser,
while (authorizationsIterator.hasNext()) {
AccessControlEntry acl = authorizationsIterator.next();
- if (acl.getRepositoryPath().startsWith(systemUser.getPath())) {
+ if
(acl.getRepositoryPath().startsWith(systemUser.getIntermediatePath())) {
Review comment:
what exactly does `RepoPath.startsWith` do? if it evaluates that the
entry-path is a descendant of the system-user intermediate path it might be
actually filter out the entries located at or below the user home.... if
however it would return true if the the ACL was actually pointing _at_ the node
at the intermediate path, it's likely wrong.
maybe not that critical since that block is afaik subject of SLING-9953....
##########
File path:
src/main/java/org/apache/sling/feature/cpconverter/handlers/SystemUsersEntryHandler.java
##########
@@ -43,9 +43,10 @@ public void handle(String path, Archive archive, Entry
entry, ContentPackage2Fea
path = matcher.group(1);
}
- RepoPath currentPath = new
RepoPath(PlatformNameFormat.getRepositoryPath(path)).getParent();
+ RepoPath originalPath = new
RepoPath(PlatformNameFormat.getRepositoryPath(path));
+ RepoPath intermediatePath = originalPath.getParent();
Review comment:
see above. that could also be move into the 'SystemUser' constructor....
but it depends a bit on the design and which class should be in charge of
knowing this kind of details.
##########
File path:
src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/SystemUser.java
##########
@@ -25,10 +25,12 @@
private final String id;
private final RepoPath path;
+ private final RepoPath intermediatePath;
- public SystemUser(String id, RepoPath path) {
+ public SystemUser(String id, RepoPath path, RepoPath intermediatePath) {
Review comment:
alternatively, use the old constructor and call `path.getParent()` in
the constructor.... i don't recall if RepoPath.getParent can return null but
that might be worth verify if one wants to be on the safe side.
----------------------------------------------------------------
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]
