common: modify the way the flag on isRestricted works

Motivation: see https://rb.dcache.org/r/14006/ master@9eae61bbac6749664467e92971354b9c7490965f There was a slight misconstrual of the real issue. The patch stopped calling symlink resolution but still did the prefix comparison. This means that, for instance, paths with a symlink in the prefix would not be visible (in some cases effectively returning 0 results). Modification: Change to boolean parameter's meaning to `skipPrefixCheck`, and do not do prefix comparisons in that case. This means that children are in fact only checked for permissions on READ_METADATA. Result: Identical list results returned, e.g., for directories specified with or without a symlink in the prefix. Target: master Request: 9.1 Request: 9.0 Request: 8.2 Patch: https://rb.dcache.org/r/14054/ Requires-notes: yes (alas) Depends-on: #14006 Acked-by: Dmitry
dCache · Aug 16, 2023 · b17e6b1 · b17e6b1
1 parent ff1889f
commit b17e6b1
Show file tree

Hide file tree

Showing 4 changed files with 46 additions and 33 deletions.
diff --git a/modules/common/src/main/java/org/dcache/auth/attributes/DenyActivityRestriction.java b/modules/common/src/main/java/org/dcache/auth/attributes/DenyActivityRestriction.java
@@ -60,7 +60,7 @@ public boolean isRestricted(Activity activity, FsPath path) {
     }
 
     @Override
-    public boolean isRestricted(Activity activity, FsPath directory, String name, boolean skipSymlinkResolution) {
+    public boolean isRestricted(Activity activity, FsPath directory, String name, boolean skipPrefixCheck) {
         return denied.contains(activity);
     }
 

diff --git a/modules/common/src/main/java/org/dcache/auth/attributes/MultiTargetedRestriction.java b/modules/common/src/main/java/org/dcache/auth/attributes/MultiTargetedRestriction.java
@@ -140,14 +140,13 @@ public boolean hasUnrestrictedChild(Activity activity, FsPath parent) {
 
     @Override
     public boolean isRestricted(Activity activity, FsPath path) {
-        return isRestricted(activity, path, getPathResolver());
+        return isRestricted(activity, path, false);
     }
 
     @Override
     public boolean isRestricted(Activity activity, FsPath directory, String child,
-          boolean skipSymlinkResolution) {
-        return isRestricted(activity, directory.child(child),
-              skipSymlinkResolution ? getIdentityResolver() : getPathResolver());
+          boolean skipPrefixCheck) {
+        return isRestricted(activity, directory.child(child), skipPrefixCheck);
     }
 
     @Override
@@ -216,19 +215,29 @@ public String toString() {
               .collect(Collectors.joining(", ", "MultiTargetedRestriction[", "]"));
     }
 
-    private boolean isRestricted(Activity activity, FsPath path, Function<FsPath, FsPath> resolver) {
+    private boolean isRestricted(Activity activity, FsPath path, boolean skipPrefixCheck) {
         for (Authorisation authorisation : authorisations) {
-            FsPath allowedPath = resolver.apply(authorisation.getPath());
             EnumSet<Activity> allowedActivity = authorisation.getActivity();
-            path = resolver.apply(path);
-            if (allowedActivity.contains(activity) && path.hasPrefix(allowedPath)) {
-                return false;
-            }
-
-            // As a special case, certain activities are always allowed for
-            // parents of an AllowedPath.
-            if (ALLOWED_PARENT_ACTIVITIES.contains(activity) && allowedPath.hasPrefix(path)) {
-                return false;
+            if (skipPrefixCheck) {
+                if (allowedActivity.contains(activity)) {
+                    return false;
+                }
+
+                if (ALLOWED_PARENT_ACTIVITIES.contains(activity)) {
+                    return false;
+                }
+            } else {
+                FsPath allowedPath = getPathResolver().apply(authorisation.getPath());
+                path = getPathResolver().apply(path);
+                if (allowedActivity.contains(activity) && path.hasPrefix(allowedPath)) {
+                    return false;
+                }
+
+                // As a special case, certain activities are always allowed for
+                // parents of an AllowedPath.
+                if (ALLOWED_PARENT_ACTIVITIES.contains(activity) && allowedPath.hasPrefix(path)) {
+                    return false;
+                }
             }
         }
 

diff --git a/modules/common/src/main/java/org/dcache/auth/attributes/PrefixRestriction.java b/modules/common/src/main/java/org/dcache/auth/attributes/PrefixRestriction.java
@@ -46,14 +46,13 @@ public ImmutableSet<FsPath> getPrefixes() {
 
     @Override
     public boolean isRestricted(Activity activity, FsPath path) {
-        return isRestricted(activity, path, getPathResolver());
+        return isRestricted(activity, path, false);
     }
 
     @Override
     public boolean isRestricted(Activity activity, FsPath directory, String child,
-          boolean skipSymlinkResolution) {
-        return isRestricted(activity, directory.child(child),
-              skipSymlinkResolution ? getIdentityResolver() : getPathResolver());
+          boolean skipPrefixCheck) {
+        return isRestricted(activity, directory.child(child), skipPrefixCheck);
     }
 
     @Override
@@ -145,18 +144,23 @@ public String toString() {
         return sb.append(']').toString();
     }
 
-    private boolean isRestricted(Activity activity, FsPath path, Function<FsPath, FsPath> resolver) {
-        for (FsPath prefix : prefixes) {
-            prefix = resolver.apply(prefix);
-            path = resolver.apply(path);
-            if (path.hasPrefix(prefix)) {
-                return false;
-            }
-            if (prefix.hasPrefix(path) && (activity == Activity.READ_METADATA
-                  || activity == Activity.LIST)) {
-                return false;
+    private boolean isRestricted(Activity activity, FsPath path, boolean skipPrefixCheck) {
+        if (skipPrefixCheck) {
+            return !(activity == Activity.READ_METADATA || activity == Activity.LIST);
+        } else {
+            for (FsPath prefix : prefixes) {
+                prefix = getPathResolver().apply(prefix);
+                path = getPathResolver().apply(path);
+                if (path.hasPrefix(prefix)) {
+                    return false;
+                }
+                if (prefix.hasPrefix(path) && (activity == Activity.READ_METADATA
+                      || activity == Activity.LIST)) {
+                    return false;
+                }
             }
         }
+
         return true;
     }
 }
diff --git a/modules/common/src/main/java/org/dcache/auth/attributes/Restrictions.java b/modules/common/src/main/java/org/dcache/auth/attributes/Restrictions.java
@@ -165,10 +165,10 @@ public boolean isRestricted(Activity activity, FsPath path) {
         }
 
         @Override
-        public boolean isRestricted(Activity activity, FsPath directory, String name, boolean skipSymlink) {
+        public boolean isRestricted(Activity activity, FsPath directory, String name, boolean skipPrefixCheck) {
             for (Restriction r : restrictions) {
-                r.setPathResolver(skipSymlink? getIdentityResolver() : getPathResolver());
-                if (r.isRestricted(activity, directory, name)) {
+                r.setPathResolver(getPathResolver());
+                if (r.isRestricted(activity, directory, name, skipPrefixCheck)) {
                     return true;
                 }
             }