From 5cad241b0ead8c12065384e380f9ea543cae8fdc Mon Sep 17 00:00:00 2001 From: Albert Louis Rossi Date: Fri, 11 Aug 2023 14:00:33 -0500 Subject: [PATCH] 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 --- .../attributes/DenyActivityRestriction.java | 2 +- .../attributes/MultiTargetedRestriction.java | 39 ++++++++++++------- .../auth/attributes/PrefixRestriction.java | 32 ++++++++------- .../dcache/auth/attributes/Restrictions.java | 6 +-- 4 files changed, 46 insertions(+), 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 index 7c8910c50e2..4ce0136d1fc 100644 --- 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 index fcf5aa1d253..6671e03d800 100644 --- 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 resolver) { + private boolean isRestricted(Activity activity, FsPath path, boolean skipPrefixCheck) { for (Authorisation authorisation : authorisations) { - FsPath allowedPath = resolver.apply(authorisation.getPath()); EnumSet 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 index 023c32a8a89..1a6800e3a63 100644 --- 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 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 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 index 2731e047c44..c4a321ec13c 100644 --- 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; } }