[GitHub] [jackrabbit-filevault] adamcin commented on a diff in pull request #294: Bugfix/jcrvlt 683

["adamcin (via GitHub)" <gi...@apache.org>]

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: dev-unsubscribe@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org