adamcin commented on code in PR #294:
URL:
https://github.com/apache/jackrabbit-filevault/pull/294#discussion_r1180888788
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/fs/io/Importer.java:
##########
@@ -565,6 +574,52 @@ public void run(Archive archive, Session session, String
parentPath)
}
}
+ private void restorePrincipalAcls(Session session) throws
RepositoryException {
+ for (String authorizableId : createdAuthorizableIds) {
+ String principalName = userManagement.getPrincipalName(session,
authorizableId);
+ if (deletedPrincipalAcls.containsKey(principalName)) {
+ if (opts.isDryRun()) {
+ track("Dry run: Would potentially restore principal ACLs
of " + principalName + " ...", "");
+ } else {
+ for (AccessControlPolicy policy :
deletedPrincipalAcls.get(principalName)) {
+ // CUG or ACL handling relevant?
+ AccessControlHandling aclHandling;
+ if (policy instanceof PrincipalSetPolicy) {
+ aclHandling = opts.getCugHandling();
+ } else {
+ aclHandling = opts.getAccessControlHandling();
+ }
+ // convert aclHandling (as this was set for the
imported ACLs, not the existing ones)
+ final AccessControlHandling
aclHandlingForRestoredPolicy;
+ switch (aclHandling) {
+ case OVERWRITE:
+ aclHandlingForRestoredPolicy =
AccessControlHandling.IGNORE;
+ break;
+ case IGNORE:
+ aclHandlingForRestoredPolicy =
AccessControlHandling.OVERWRITE;
+ break;
+ case CLEAR:
+ aclHandlingForRestoredPolicy =
AccessControlHandling.IGNORE;
+ break;
+ case MERGE:
+ aclHandlingForRestoredPolicy =
AccessControlHandling.MERGE;
Review Comment:
@kwin I read this switch case and my immediate reaction to it is that it
should be:
```java
case MERGE:
aclHandlingForRestoredPolicy = AccessControlHandling.MERGE_PRESERVE;
```
However, after changing this locally and running PrincipalBasedIT, the
`#testNoPolicyHandlingMergeModeReplace` test failed, presumably because
`PrincipalBasedAccessControlList.apply()` method treats MERGE_PRESERVE the same
as IGNORE.
```java
if (aclHandling == AccessControlHandling.MERGE_PRESERVE) {
log.debug("MERGE_PRESERVE for principal-based access control list is
equivalent to IGNORE.");
return Collections.emptyList();
}
```
So, after rethinking it, I have these reactions:
1. As a filevault user, ***I'm happy with this PR as-is,*** because it
certainly resolves the issue for the most important ACHandling use-cases for
packaged principal policies, which are OVERWRITE and IGNORE.
2. It is also implemented in a way that makes MERGE_PRESERVE behave at least
as safely for existing ACLs as one would expect, and any surprise for the user
would be for situations where packaged principal ACLs were expected to be
installed but weren't. ***I think this is acceptable for all known practical
purposes***.
3. I think there is an opportunity to discuss a clearer set of expectations
for MERGE and MERGE_PRESERVE behavior for Principal ACLs, which I elaborate on
below, but such further discussion and any related refactoring may not be worth
holding up this PR.
#### Bikeshedding on MERGE and MERGE_PRESERVE for Principal ACLs
Now that I've had a chance to review my original IT PR for this issue, I'm
not perfectly satisfied with the behavior of
`#testNoPolicyHandlingMergeModeReplace` with the switch case change I described
above, nor with the behavior of `#testHandlingMergeModeReplace` after thinking
through the implications of that effect. I think my assumption of how `MERGE`
should work, as implemented in `#testHandlingMergeModeReplace`, which is to
simply append the package entries to the existing entries, fails to align with
a strict interpretation of the javadoc for `AccessControlHandling#MERGE` with
respect to Principal ACLs.
> Merge access control provided with the package with the one in the content
by replacing the access control entries of corresponding principals (i.e.
package first). It never alters access control entries of principals not
present in the package.
There are probably two strict approaches that would leave no room for
surprises.
One way, based on a literal reading of the javadocs, would be to treat
`MERGE` as identical to `OVERWRITE` and `MERGE_PRESERVE` as identical to
`IGNORE`, because a `rep:PrincipalACL` node only contains entries for the
containing principal. I think this approach would be more correct for MERGE
than is implemented in this PR, but it would come with additional risk that a
mistake in package creation like choosing MERGE instead of MERGE_PRESERVE would
be more destructive for system functionality, whe
The other way to read the javadoc with respect to principal policies would
be clearer if the javadoc was rewritten to include a content path constraint
that was simply assumed when the `AccessControlHandling` type was originally
introduced.
> Merge access control provided with the package, ***effective over some
content path***, with the one in the content, ***effective over the same
content path***, by replacing the access control entries of corresponding
principals (i.e. package first). It never alters access control entries of
principals not present in the package.
With that clarification, under `MERGE` and `MERGE_PRESERVE` modes, it would
make sense to apply principal policy entries in groups, based on their
`rep:path` restriction, just as if the package contained traditional ACLs at
each effective path instead of the single principal ACL under the principal's
path.
`MERGE` Example:
```
Content Principal ACL (/home/users/bob/rep:PrincipalACL):
bob, allow, jcr:all on /home/users/bob
bob, allow, jcr:read on /content
bob, allow, rep:write on /content/usergenerated
Package Principal ACL:
bob, deny, jcr:all on /conf/private
bob, allow, jcr:read on /content/usergenerated
Result Principal ACL:
bob, allow, jcr:all on /home/users/bob
bob, allow, jcr:read on /content
bob, deny, jcr:all on /conf/private
bob, allow, jcr:read on /content/usergenerated // rep:write was
overwritten with jcr:read
```
`MERGE_PRESERVE` Example:
```
Content Principal ACL (/home/users/bob/rep:PrincipalACL):
bob, allow, jcr:all on /home/users/bob
bob, allow, jcr:read on /content
bob, allow, rep:write on /content/usergenerated
Package Principal ACL:
bob, deny, jcr:all on /conf/private
bob, allow, jcr:read on /content/usergenerated
Result Principal ACL:
bob, allow, jcr:all on /home/users/bob
bob, allow, jcr:read on /content
bob, allow, rep:write on /content/usergenerated // jcr:read was
preserved without adding rep:write
bob, deny, jcr:all on /conf/private
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
