Tests and comments

Author: n1v0lg

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java

@@ -143,9 +143,9 @@ public boolean hasFieldOrDocumentLevelSecurity() {
 
     private IsResourceAuthorizedPredicate buildIndexMatcherPredicateForAction(String action) {
         final Set<String> dataAccessOrdinaryIndices = new HashSet<>();
-        final Set<String> failureAccessOrdinaryIndices = new HashSet<>();
+        final Set<String> failuresAccessOrdinaryIndices = new HashSet<>();
         final Set<String> dataAccessRestrictedIndices = new HashSet<>();
-        final Set<String> failureAccessRestrictedIndices = new HashSet<>();
+        final Set<String> failuresAccessRestrictedIndices = new HashSet<>();
         final Set<String> grantMappingUpdatesOnIndices = new HashSet<>();
         final Set<String> grantMappingUpdatesOnRestrictedIndices = new HashSet<>();
         final boolean isMappingUpdateAction = isMappingUpdateAction(action);
@@ -157,15 +157,15 @@ private IsResourceAuthorizedPredicate buildIndexMatcherPredicateForAction(String
                         dataAccessRestrictedIndices.addAll(indexList);
                     }
                     if (group.checkSelector(IndexComponentSelector.FAILURES)) {
-                        failureAccessRestrictedIndices.addAll(indexList);
+                        failuresAccessRestrictedIndices.addAll(indexList);
                     }
                 } else {
                     List<String> indexList = Arrays.asList(group.indices());
                     if (group.checkSelector(IndexComponentSelector.DATA)) {
                         dataAccessOrdinaryIndices.addAll(indexList);
                     }
                     if (group.checkSelector(IndexComponentSelector.FAILURES)) {
-                        failureAccessOrdinaryIndices.addAll(indexList);
+                        failuresAccessOrdinaryIndices.addAll(indexList);
                     }
                 }
             } else if (isMappingUpdateAction && containsPrivilegeThatGrantsMappingUpdatesForBwc(group)) {
@@ -179,9 +179,9 @@ private IsResourceAuthorizedPredicate buildIndexMatcherPredicateForAction(String
             }
         }
         final StringMatcher dataAccessNameMatcher = indexMatcher(dataAccessOrdinaryIndices, dataAccessRestrictedIndices);
-        final StringMatcher failureAccessNameMatcher = indexMatcher(failureAccessOrdinaryIndices, failureAccessRestrictedIndices);
+        final StringMatcher failuresAccessNameMatcher = indexMatcher(failuresAccessOrdinaryIndices, failuresAccessRestrictedIndices);
         final StringMatcher bwcSpecialCaseMatcher = indexMatcher(grantMappingUpdatesOnIndices, grantMappingUpdatesOnRestrictedIndices);
-        return new IsResourceAuthorizedPredicate(dataAccessNameMatcher, failureAccessNameMatcher, bwcSpecialCaseMatcher);
+        return new IsResourceAuthorizedPredicate(dataAccessNameMatcher, failuresAccessNameMatcher, bwcSpecialCaseMatcher);
     }
 
     /**
@@ -190,32 +190,32 @@ private IsResourceAuthorizedPredicate buildIndexMatcherPredicateForAction(String
      */
     public static class IsResourceAuthorizedPredicate {
 
-        private final BiPredicate<String, IndexAbstraction> isAuthorizedForData;
-        private final BiPredicate<String, IndexAbstraction> isAuthorizedForFailureStore;
+        private final BiPredicate<String, IndexAbstraction> isAuthorizedForDataAccess;
+        private final BiPredicate<String, IndexAbstraction> isAuthorizedForFailuresAccess;
 
         // public for tests
         public IsResourceAuthorizedPredicate(
-            StringMatcher resourceNameMatcher,
-            StringMatcher failureStoreNameMatcher,
+            StringMatcher dataResourceNameMatcher,
+            StringMatcher failuresResourceNameMatcher,
             StringMatcher additionalNonDatastreamNameMatcher
         ) {
             this((String name, @Nullable IndexAbstraction indexAbstraction) -> {
                 assert indexAbstraction == null || name.equals(indexAbstraction.getName());
-                return resourceNameMatcher.test(name)
+                return dataResourceNameMatcher.test(name)
                     || (isPartOfDatastream(indexAbstraction) == false && additionalNonDatastreamNameMatcher.test(name));
             }, (String name, @Nullable IndexAbstraction indexAbstraction) -> {
                 assert indexAbstraction == null || name.equals(indexAbstraction.getName());
                 // we can't enforce that the abstraction is part of a data stream since we need to account for non-existent resources
-                return failureStoreNameMatcher.test(name);
+                return failuresResourceNameMatcher.test(name);
             });
         }
 
         private IsResourceAuthorizedPredicate(
-            BiPredicate<String, IndexAbstraction> isAuthorizedForData,
-            BiPredicate<String, IndexAbstraction> isAuthorizedForFailureStore
+            BiPredicate<String, IndexAbstraction> isAuthorizedForDataAccess,
+            BiPredicate<String, IndexAbstraction> isAuthorizedForFailuresAccess
         ) {
-            this.isAuthorizedForData = isAuthorizedForData;
-            this.isAuthorizedForFailureStore = isAuthorizedForFailureStore;
+            this.isAuthorizedForDataAccess = isAuthorizedForDataAccess;
+            this.isAuthorizedForFailuresAccess = isAuthorizedForFailuresAccess;
         }
 
         /**
@@ -225,14 +225,14 @@ private IsResourceAuthorizedPredicate(
         */
         public final IsResourceAuthorizedPredicate and(IsResourceAuthorizedPredicate other) {
             return new IsResourceAuthorizedPredicate(
-                this.isAuthorizedForData.and(other.isAuthorizedForData),
-                this.isAuthorizedForFailureStore.and(other.isAuthorizedForFailureStore)
+                this.isAuthorizedForDataAccess.and(other.isAuthorizedForDataAccess),
+                this.isAuthorizedForFailuresAccess.and(other.isAuthorizedForFailuresAccess)
             );
         }
 
         // TODO remove me (this has >700 usages in tests which would make for a horrible diff; will remove this once the main PR is merged)
         public boolean test(IndexAbstraction indexAbstraction) {
-            return test(indexAbstraction.getName(), indexAbstraction, null);
+            return test(indexAbstraction.getName(), indexAbstraction, IndexComponentSelector.DATA);
         }
 
         /**
@@ -252,8 +252,8 @@ public boolean test(IndexAbstraction indexAbstraction, @Nullable IndexComponentS
          */
         public boolean test(String name, @Nullable IndexAbstraction indexAbstraction, @Nullable IndexComponentSelector selector) {
             return IndexComponentSelector.FAILURES.equals(selector)
-                ? isAuthorizedForFailureStore.test(name, indexAbstraction)
-                : isAuthorizedForData.test(name, indexAbstraction);
+                ? isAuthorizedForFailuresAccess.test(name, indexAbstraction)
+                : isAuthorizedForDataAccess.test(name, indexAbstraction);
         }
 
         private static boolean isPartOfDatastream(IndexAbstraction indexAbstraction) {

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java

@@ -905,6 +905,8 @@ static AuthorizedIndices resolveAuthorizedIndicesFromRole(
             } else {
                 for (IndexAbstraction indexAbstraction : lookup.values()) {
                     if (indexAbstraction.getType() != IndexAbstraction.Type.DATA_STREAM
+                        // data check is correct, even for failure indices -- in this context, we treat them as regular indices, and
+                        // only include them if direct data access to them is granted (e.g., if a role has `read` over "*")
                         && predicate.test(indexAbstraction, IndexComponentSelector.DATA)) {
                         indicesAndAliases.add(indexAbstraction.getName());
                     }
@@ -1072,31 +1074,31 @@ private static boolean isAsyncRelatedAction(String action) {
     }
 
     static final class AuthorizedIndices implements AuthorizationEngine.AuthorizedIndices {
-        private final CachedSupplier<Set<String>> authorizedAndAvailableSupplier;
-        private final CachedSupplier<Set<String>> failureStoreAuthorizedAndAvailableSupplier;
+        private final CachedSupplier<Set<String>> authorizedAndAvailableDataResources;
+        private final CachedSupplier<Set<String>> authorizedAndAvailableFailuresResources;
         private final BiPredicate<String, IndexComponentSelector> isAuthorizedPredicate;
 
         AuthorizedIndices(
-            Supplier<Set<String>> authorizedAndAvailableSupplier,
-            Supplier<Set<String>> failureStoreAuthorizedAndAvailableSupplier,
+            Supplier<Set<String>> authorizedAndAvailableDataResources,
+            Supplier<Set<String>> authorizedAndAvailableFailuresResources,
             BiPredicate<String, IndexComponentSelector> isAuthorizedPredicate
         ) {
-            this.authorizedAndAvailableSupplier = CachedSupplier.wrap(authorizedAndAvailableSupplier);
-            this.failureStoreAuthorizedAndAvailableSupplier = CachedSupplier.wrap(failureStoreAuthorizedAndAvailableSupplier);
+            this.authorizedAndAvailableDataResources = CachedSupplier.wrap(authorizedAndAvailableDataResources);
+            this.authorizedAndAvailableFailuresResources = CachedSupplier.wrap(authorizedAndAvailableFailuresResources);
             this.isAuthorizedPredicate = Objects.requireNonNull(isAuthorizedPredicate);
         }
 
         @Override
         public Set<String> all(IndexComponentSelector selector) {
-            Objects.requireNonNull(selector);
+            Objects.requireNonNull(selector, "must specify a selector to get authorized indices");
             return IndexComponentSelector.FAILURES.equals(selector)
-                ? failureStoreAuthorizedAndAvailableSupplier.get()
-                : authorizedAndAvailableSupplier.get();
+                ? authorizedAndAvailableFailuresResources.get()
+                : authorizedAndAvailableDataResources.get();
         }
 
         @Override
         public boolean check(String name, IndexComponentSelector selector) {
-            Objects.requireNonNull(selector);
+            Objects.requireNonNull(selector, "must specify a selector for authorization check");
             return isAuthorizedPredicate.test(name, selector);
         }
     }