Fix intersection logic

Author: n1v0lg

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

@@ -108,19 +108,12 @@ private IndicesPermission(RestrictedIndices restrictedIndices, Group[] groups) {
      * @return A matcher that will match all non-restricted index names in the ordinaryIndices
      * collection and all index names in the restrictedIndices collection.
      */
-    private StringMatcher indexMatcher(
-        Collection<String> ordinaryIndices,
-        Collection<String> restrictedIndices,
-        boolean allowFailureStoreAccess
-    ) {
+    private StringMatcher indexMatcher(Collection<String> ordinaryIndices, Collection<String> restrictedIndices) {
         StringMatcher matcher;
         if (ordinaryIndices.isEmpty()) {
             matcher = StringMatcher.of(restrictedIndices);
         } else {
             matcher = StringMatcher.of(ordinaryIndices);
-            if (false == allowFailureStoreAccess) {
-                matcher = matcher.and("<not-failure-store>", name -> name.endsWith("::failure") == false);
-            }
             if (this.restrictedIndices != null) {
                 matcher = matcher.and("<not-restricted>", name -> this.restrictedIndices.isRestricted(name) == false);
             }
@@ -151,13 +144,19 @@ private IsResourceAuthorizedPredicate buildIndexMatcherPredicateForAction(String
         final Set<String> ordinaryIndices = new HashSet<>();
         final Set<String> ordinaryIndicesWithFailureStoreAccess = new HashSet<>();
         final Set<String> restrictedIndices = new HashSet<>();
+        final Set<String> restrictedIndicesWithFailureStoreAccess = new HashSet<>();
+
         final Set<String> grantMappingUpdatesOnIndices = new HashSet<>();
         final Set<String> grantMappingUpdatesOnRestrictedIndices = new HashSet<>();
         final boolean isMappingUpdateAction = isMappingUpdateAction(action);
         for (final Group group : groups) {
             if (group.actionMatcher.test(action)) {
                 if (group.allowRestrictedIndices) {
-                    restrictedIndices.addAll(Arrays.asList(group.indices()));
+                    if (group.allowFailureStoreAccess) {
+                        restrictedIndicesWithFailureStoreAccess.addAll(Arrays.asList(group.indices()));
+                    } else {
+                        restrictedIndices.addAll(Arrays.asList(group.indices()));
+                    }
                 } else {
                     if (group.allowFailureStoreAccess) {
                         ordinaryIndicesWithFailureStoreAccess.addAll(Arrays.asList(group.indices()));
@@ -175,15 +174,12 @@ private IsResourceAuthorizedPredicate buildIndexMatcherPredicateForAction(String
                 }
             }
         }
-        final StringMatcher nameMatcher = indexMatcher(ordinaryIndices, restrictedIndices, false);
-        // TODO restricted indices
-        final StringMatcher nameMatcherWithFailureAccess = indexMatcher(ordinaryIndicesWithFailureStoreAccess, restrictedIndices, true);
-        final StringMatcher bwcSpecialCaseMatcher = indexMatcher(
-            grantMappingUpdatesOnIndices,
-            grantMappingUpdatesOnRestrictedIndices,
-            false
-        );
-        return new IsResourceAuthorizedPredicate(nameMatcher, nameMatcherWithFailureAccess, bwcSpecialCaseMatcher);
+        final StringMatcher nameMatcher = indexMatcher(ordinaryIndices, restrictedIndices).and(
+            "<not-failure-store>",
+            name -> name.endsWith("::failures") == false
+        ).or(indexMatcher(ordinaryIndicesWithFailureStoreAccess, restrictedIndicesWithFailureStoreAccess));
+        final StringMatcher bwcSpecialCaseMatcher = indexMatcher(grantMappingUpdatesOnIndices, grantMappingUpdatesOnRestrictedIndices);
+        return new IsResourceAuthorizedPredicate(nameMatcher, bwcSpecialCaseMatcher);
     }
 
     /**
@@ -193,30 +189,18 @@ private IsResourceAuthorizedPredicate buildIndexMatcherPredicateForAction(String
     public static class IsResourceAuthorizedPredicate {
 
         private final BiPredicate<String, IndexAbstraction> biPredicate;
-        private final BiPredicate<String, IndexAbstraction> failureAccessBiPredicate;
 
         // public for tests
-        public IsResourceAuthorizedPredicate(
-            StringMatcher resourceNameMatcher,
-            StringMatcher resourceNameWithFailureAccessMatcher,
-            StringMatcher additionalNonDatastreamNameMatcher
-        ) {
+        public IsResourceAuthorizedPredicate(StringMatcher resourceNameMatcher, StringMatcher additionalNonDatastreamNameMatcher) {
             this((String name, @Nullable IndexAbstraction indexAbstraction) -> {
                 assert indexAbstraction == null || name.equals(indexAbstraction.getName());
                 return resourceNameMatcher.test(name)
                     || (isPartOfDatastream(indexAbstraction) == false && additionalNonDatastreamNameMatcher.test(name));
-            }, (String name, @Nullable IndexAbstraction indexAbstraction) -> {
-                assert indexAbstraction == null || name.equals(indexAbstraction.getName());
-                return resourceNameWithFailureAccessMatcher.test(name);
             });
         }
 
-        private IsResourceAuthorizedPredicate(
-            BiPredicate<String, IndexAbstraction> biPredicate,
-            BiPredicate<String, IndexAbstraction> failureAccessBiPredicate
-        ) {
+        private IsResourceAuthorizedPredicate(BiPredicate<String, IndexAbstraction> biPredicate) {
             this.biPredicate = biPredicate;
-            this.failureAccessBiPredicate = failureAccessBiPredicate;
         }
 
         /**
@@ -225,10 +209,7 @@ private IsResourceAuthorizedPredicate(
          * authorization tests of that other instance and this one.
          */
         public final IsResourceAuthorizedPredicate and(IsResourceAuthorizedPredicate other) {
-            return new IsResourceAuthorizedPredicate(
-                this.biPredicate.and(other.biPredicate),
-                this.failureAccessBiPredicate.and(other.failureAccessBiPredicate)
-            );
+            return new IsResourceAuthorizedPredicate(this.biPredicate.and(other.biPredicate));
         }
 
         /**
@@ -247,8 +228,7 @@ public final boolean test(IndexAbstraction indexAbstraction) {
          * Returns {@code true} if access to the given resource is authorized or {@code false} otherwise.
          */
         public boolean test(String name, @Nullable IndexAbstraction indexAbstraction) {
-            // TODO clean up
-            return biPredicate.test(name, indexAbstraction) || failureAccessBiPredicate.test(name, indexAbstraction);
+            return biPredicate.test(name, indexAbstraction);
         }
 
         /**
@@ -258,7 +238,7 @@ public boolean test(String name, @Nullable IndexAbstraction indexAbstraction) {
         public final boolean testDataStreamForFailureAccess(IndexAbstraction indexAbstraction) {
             // TODO clean up
             assert indexAbstraction != null && indexAbstraction.getType() == IndexAbstraction.Type.DATA_STREAM;
-            return failureAccessBiPredicate.test(
+            return biPredicate.test(
                 IndexNameExpressionResolver.combineSelector(indexAbstraction.getName(), IndexComponentSelector.FAILURES),
                 null
             );
@@ -414,7 +394,7 @@ private static class IndexResource {
         private final IndexAbstraction indexAbstraction;
 
         private IndexResource(String name, @Nullable IndexAbstraction abstraction, @Nullable IndexComponentSelector selector) {
-            // assert name != null : "Resource name cannot be null";
+            assert name != null : "Resource name cannot be null";
             // assert abstraction == null || abstraction.getName().equals(name)
             // : "Index abstraction has unexpected name [" + abstraction.getName() + "] vs [" + name + "]";
             // assert abstraction == null
@@ -426,6 +406,9 @@ private IndexResource(String name, @Nullable IndexAbstraction abstraction, @Null
             // + "] applied to abstraction of type ["
             // + abstraction.getType()
             // + "]";
+            var tuple = IndexNameExpressionResolver.splitSelectorExpression(name);
+            assert tuple.v2() == null || tuple.v2().equals(IndexComponentSelector.FAILURES.getKey())
+                : "Unexpected selector [" + tuple.v2() + "] in index name [" + name + "]";
             this.name = name;
             this.indexAbstraction = abstraction;
             this.selector = selector;

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolverTests.java

@@ -2683,6 +2683,6 @@ private void assertSameValues(List<String> indices, String[] expectedIndices) {
     }
 
     private boolean runFailureStore() {
-        return DataStream.isFailureStoreFeatureFlagEnabled() && Boolean.TRUE;// && randomBoolean();
+        return DataStream.isFailureStoreFeatureFlagEnabled() && randomBoolean();
     }
 }

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/IndicesPermissionTests.java

@@ -698,7 +698,6 @@ public void testResourceAuthorizedPredicateForDatastreams() {
         );
         IndicesPermission.IsResourceAuthorizedPredicate predicate = new IndicesPermission.IsResourceAuthorizedPredicate(
             StringMatcher.of("other"),
-            StringMatcher.of(),
             StringMatcher.of(dataStreamName, backingIndex.getName(), concreteIndex.getName(), alias.getName())
         );
         assertThat(predicate.test(dataStream), is(false));
@@ -714,12 +713,10 @@ public void testResourceAuthorizedPredicateForDatastreams() {
     public void testResourceAuthorizedPredicateAnd() {
         IndicesPermission.IsResourceAuthorizedPredicate predicate1 = new IndicesPermission.IsResourceAuthorizedPredicate(
             StringMatcher.of("c", "a"),
-            StringMatcher.of(),
             StringMatcher.of("b", "d")
         );
         IndicesPermission.IsResourceAuthorizedPredicate predicate2 = new IndicesPermission.IsResourceAuthorizedPredicate(
             StringMatcher.of("c", "b"),
-            StringMatcher.of(),
             StringMatcher.of("a", "d")
         );
         Metadata.Builder mb = Metadata.builder(