Authorization: enforced some assumptions through asserts

- made sure that clear_scroll all gets converted to the correspoinding shield cluster action in both action filter and transport filter (used to happen only on the action filter before): introduced the context of ShieldActionMapper that allows to convert action names based on an incoming request and its action name (will be useful for analyze api too)
- made sure that potential clear_scroll all errors contain the shield action name rather than the es core original one
- made it clearer that the only indices actions known not to be indices requests are scroll related ones, which we assert on and grant. Everything else gets denied.
- made it clearer that the only indices request whose indices might end up being resolved to an empty set is analyze request, as its index is optional
- simplified permissions check in Permission.Group by asserting on index argument not null

Original commit: elastic/x-pack-elasticsearch@7c01159b03

src/main/java/org/elasticsearch/shield/action/ShieldActionFilter.java

@@ -46,16 +46,19 @@ public class ShieldActionFilter extends AbstractComponent implements ActionFilte
     private final AuthorizationService authzService;
     private final SignatureService signatureService;
     private final AuditTrail auditTrail;
+    private final ShieldActionMapper actionMapper;
 
     private volatile boolean licenseEnabled;
 
     @Inject
-    public ShieldActionFilter(Settings settings, AuthenticationService authcService, AuthorizationService authzService, SignatureService signatureService, AuditTrail auditTrail, LicenseEventsNotifier licenseEventsNotifier) {
+    public ShieldActionFilter(Settings settings, AuthenticationService authcService, AuthorizationService authzService, SignatureService signatureService,
+                              AuditTrail auditTrail, LicenseEventsNotifier licenseEventsNotifier, ShieldActionMapper actionMapper) {
         super(settings);
         this.authcService = authcService;
         this.authzService = authzService;
         this.signatureService = signatureService;
         this.auditTrail = auditTrail;
+        this.actionMapper = actionMapper;
         licenseEventsNotifier.register(new LicensesClientService.Listener() {
             @Override
             public void onEnabled() {
@@ -92,9 +95,11 @@ public class ShieldActionFilter extends AbstractComponent implements ActionFilte
              the {@link Rest} filter and the {@link ServerTransport} filter respectively), it's safe to assume a system user
              here if a request is not associated with any other user.
              */
-            User user = authcService.authenticate(action, request, User.SYSTEM);
+
-            authzService.authorize(user, action, request);
+            String shieldAction = actionMapper.action(action, request);
-            request = unsign(user, action, request);
+            User user = authcService.authenticate(shieldAction, request, User.SYSTEM);
+            authzService.authorize(user, shieldAction, request);
+            request = unsign(user, shieldAction, request);
             chain.proceed(action, request, new SigningListener(this, listener));
         } catch (Throwable t) {
             listener.onFailure(t);
@@ -125,9 +130,7 @@ public class ShieldActionFilter extends AbstractComponent implements ActionFilte
             if (request instanceof ClearScrollRequest) {
                 ClearScrollRequest clearScrollRequest = (ClearScrollRequest) request;
                 boolean isClearAllScrollRequest = clearScrollRequest.scrollIds().contains("_all");
-                if (isClearAllScrollRequest) {
+                if (!isClearAllScrollRequest) {
-                    authzService.authorize(user, CLUSTER_PERMISSION_SCROLL_CLEAR_ALL_NAME, request);
-                } else {
                     List<String> signedIds = clearScrollRequest.scrollIds();
                     List<String> unsignedIds = new ArrayList<>(signedIds.size());
                     for (String signedId : signedIds) {

src/main/java/org/elasticsearch/shield/action/ShieldActionMapper.java

@@ -0,0 +1,33 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+package org.elasticsearch.shield.action;
+
+import org.elasticsearch.action.search.ClearScrollAction;
+import org.elasticsearch.action.search.ClearScrollRequest;
+import org.elasticsearch.transport.TransportRequest;
+
+/**
+ * This class analyzes an incoming request and its action name, and returns the shield action name for it.
+ * In many cases the action name is the same as the original one used in es core, but in some exceptional cases it might need
+ * to be converted. For instance a clear_scroll that targets all opened scrolls gets converted to a different action that requires
+ * cluster privileges instead of the default indices privileges, still valid for clear scrolls that target specific scroll ids.
+ */
+public class ShieldActionMapper {
+
+    /**
+     * Returns the shield specific action name given the incoming action name and request
+     */
+    public String action(String action, TransportRequest request) {
+        if (action.equals(ClearScrollAction.NAME)) {
+            assert request instanceof ClearScrollRequest;
+            boolean isClearAllScrollRequest =  ((ClearScrollRequest) request).scrollIds().contains("_all");
+            if (isClearAllScrollRequest) {
+                return ShieldActionFilter.CLUSTER_PERMISSION_SCROLL_CLEAR_ALL_NAME;
+            }
+        }
+        return action;
+    }
+}

src/main/java/org/elasticsearch/shield/action/ShieldActionModule.java

@@ -39,6 +39,7 @@ public class ShieldActionModule extends AbstractShieldModule implements PreProce
     @Override
     protected void configure(boolean clientMode) {
         if (!clientMode) {
+            bind(ShieldActionMapper.class).asEagerSingleton();
             // we need to ensure that there's only a single instance of this filter.
             bind(ShieldActionFilter.class).asEagerSingleton();
         }

src/main/java/org/elasticsearch/shield/authz/InternalAuthorizationService.java

@@ -7,6 +7,9 @@ package org.elasticsearch.shield.authz;
 
 import org.elasticsearch.action.CompositeIndicesRequest;
 import org.elasticsearch.action.IndicesRequest;
+import org.elasticsearch.action.admin.indices.analyze.AnalyzeRequest;
+import org.elasticsearch.action.search.ClearScrollAction;
+import org.elasticsearch.action.search.SearchScrollAction;
 import org.elasticsearch.cluster.ClusterService;
 import org.elasticsearch.cluster.metadata.MetaData;
 import org.elasticsearch.common.base.Predicate;
@@ -15,6 +18,7 @@ import org.elasticsearch.common.collect.ImmutableList;
 import org.elasticsearch.common.component.AbstractComponent;
 import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.search.action.SearchServiceTransportAction;
 import org.elasticsearch.shield.User;
 import org.elasticsearch.shield.audit.AuditTrail;
 import org.elasticsearch.shield.authz.indicesresolver.DefaultIndicesResolver;
@@ -22,7 +26,6 @@ import org.elasticsearch.shield.authz.indicesresolver.IndicesResolver;
 import org.elasticsearch.shield.authz.store.RolesStore;
 import org.elasticsearch.transport.TransportRequest;
 
-import java.util.Collections;
 import java.util.Iterator;
 import java.util.Set;
 
@@ -99,7 +102,7 @@ public class InternalAuthorizationService extends AbstractComponent implements A
         }
 
         // first, we'll check if the action is a cluster action. If it is, we'll only check it
-        // agaist the cluster permissions
+        // against the cluster permissions
         if (Privilege.Cluster.ACTION_MATCHER.apply(action)) {
             Permission.Cluster cluster = permission.cluster();
             if (cluster != null && cluster.check(action)) {
@@ -114,25 +117,39 @@ public class InternalAuthorizationService extends AbstractComponent implements A
             throw denial(user, action, request);
         }
 
-        Set<String> indexNames = resolveIndices(user, action, request);
+        // some APIs are indices requests that are not actually associated with indices. For example,
-        if (indexNames == null) {
+        // search scroll request, is categorized under the indices context, but doesn't hold indices names
-            // the only time this will be null, is for those requests that are
+        // (in this case, the security check on the indices was done on the search request that initialized
-            // categorized as indices request but they're actully not (for example, scroll)
+        // the scroll... and we rely on the signed scroll id to provide security over this request).
-            // in these cases, we only grant/deny based on the action name (performed above)
+        // so we only check indices if indeed the request is an actual IndicesRequest, if it's not,
-            grant(user, action, request);
+        // we just grant it if it's a scroll, deny otherwise
-            return;
+        if (!(request instanceof IndicesRequest) && !(request instanceof CompositeIndicesRequest)) {
-        }
+            if (isScrollRelatedAction(action)) {
-
+                //note that clear scroll shard level actions can originate from a clear scroll all, which doesn't require any
-        Permission.Indices indices = permission.indices();
+                //indices permission as it's categorized under cluster. This is why the scroll check is performed
-        if (indices == null || indices.isEmpty()) {
+                //even before checking if the user has any indices permission.
+                grant(user, action, request);
+                return;
+            }
+            assert false : "only scroll related requests are known indices api that don't support retrieving the indices they relate to";
             throw denial(user, action, request);
         }
 
+        if (permission.indices() == null || permission.indices().isEmpty()) {
+            throw denial(user, action, request);
+        }
+
+        Set<String> indexNames = resolveIndices(user, action, request);
+        //Note: some APIs (e.g. analyze) are categorized under indices, but their indices are optional.
+        //In that case the resolved indices set is empty (for now). See https://github.com/elasticsearch/elasticsearch-shield/issues/566
+        assert !indexNames.isEmpty() || request instanceof AnalyzeRequest
+                : "no indices request other than the analyze api has optional indices thus the resolved indices must not be empty";
+
         // now... every index that is associated with the request, must be granted
         // by at least one indices permission group
         for (String index : indexNames) {
             boolean granted = false;
-            for (Permission.Indices.Group group : indices) {
+            for (Permission.Indices.Group group : permission.indices()) {
                 if (group.check(action, index)) {
                     granted = true;
                     break;
@@ -180,25 +197,23 @@ public class InternalAuthorizationService extends AbstractComponent implements A
 
     private Set<String> resolveIndices(User user, String action, TransportRequest request) {
         MetaData metaData = clusterService.state().metaData();
-
+        for (IndicesResolver resolver : indicesResolvers) {
-        // some APIs are indices requests that are not actually associated with indices. For example,
+            if (resolver.requestType().isInstance(request)) {
-        // search scroll request, is categorized under the indices context, but doesn't hold indices names
+                return resolver.resolve(user, action, request, metaData);
-        // (in this case, the security check on the indices was done on the search request that initialized
-        // the scroll... and we rely on the signed scroll id to provide security over this request).
-
-        // so we only check indices if indeed the request is an actual IndicesRequest, if it's not, we only
-        // perform the check on the action name.
-        Set<String> indices = null;
-
-        if (request instanceof IndicesRequest || request instanceof CompositeIndicesRequest) {
-            indices = Collections.emptySet();
-            for (IndicesResolver resolver : indicesResolvers) {
-                if (resolver.requestType().isInstance(request)) {
-                    indices = resolver.resolve(user, action, request, metaData);
-                    break;
-                }
             }
         }
-        return indices;
+        assert false : "we should be able to resolve indices for any known request that requires indices privileges";
+        throw denial(user, action, request);
+    }
+
+    private static boolean isScrollRelatedAction(String action) {
+        return action.equals(SearchScrollAction.NAME) ||
+                action.equals(SearchServiceTransportAction.SCAN_SCROLL_ACTION_NAME) ||
+                action.equals(SearchServiceTransportAction.FETCH_ID_SCROLL_ACTION_NAME) ||
+                action.equals(SearchServiceTransportAction.QUERY_FETCH_SCROLL_ACTION_NAME) ||
+                action.equals(SearchServiceTransportAction.QUERY_SCROLL_ACTION_NAME) ||
+                action.equals(SearchServiceTransportAction.FREE_CONTEXT_SCROLL_ACTION_NAME) ||
+                action.equals(ClearScrollAction.NAME) ||
+                action.equals(SearchServiceTransportAction.CLEAR_SCROLL_CONTEXTS_ACTION_NAME);
     }
 }

src/main/java/org/elasticsearch/shield/authz/Permission.java

@@ -348,7 +348,6 @@ public interface Permission {
         }
 
         public static class Group {
-
             private final Privilege.Index privilege;
             private final Predicate<String> actionMatcher;
             private final String[] indices;
@@ -371,10 +370,9 @@ public interface Permission {
             }
 
             public boolean check(String action, String index) {
-                return actionMatcher.apply(action) && !(index != null && !indexNameMatcher.apply(index));
+                assert index != null;
+                return actionMatcher.apply(action) && indexNameMatcher.apply(index);
             }
-
-
         }
     }
 

src/main/java/org/elasticsearch/shield/transport/ServerTransportFilter.java

@@ -7,6 +7,7 @@ package org.elasticsearch.shield.transport;
 
 import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.shield.User;
+import org.elasticsearch.shield.action.ShieldActionMapper;
 import org.elasticsearch.shield.authc.AuthenticationException;
 import org.elasticsearch.shield.authc.AuthenticationService;
 import org.elasticsearch.shield.authz.AuthorizationService;
@@ -35,25 +36,28 @@ public interface ServerTransportFilter {
 
         private final AuthenticationService authcService;
         private final AuthorizationService authzService;
+        private final ShieldActionMapper actionMapper;
 
         @Inject
-        public NodeProfile(AuthenticationService authcService, AuthorizationService authzService) {
+        public NodeProfile(AuthenticationService authcService, AuthorizationService authzService, ShieldActionMapper actionMapper) {
             this.authcService = authcService;
             this.authzService = authzService;
+            this.actionMapper = actionMapper;
         }
 
         @Override
         public void inbound(String action, TransportRequest request) {
-            /**
+            /*
-                here we don't have a fallback user, as all incoming request are
+             here we don't have a fallback user, as all incoming request are
-                expected to have a user attached (either in headers or in context)
+             expected to have a user attached (either in headers or in context)
-                We can make this assumption because in nodes we also have the
+             We can make this assumption because in nodes we also have the
-                {@link ClientTransportFilter.Node} that makes sure all outgoing requsts
+             {@link ClientTransportFilter.Node} that makes sure all outgoing requsts
-                from all the nodes are attached with a user (either a serialize user
+             from all the nodes are attached with a user (either a serialize user
-                an authentication token
+             an authentication token
              */
-            User user = authcService.authenticate(action, request, null);
+            String shieldAction = actionMapper.action(action, request);
-            authzService.authorize(user, action, request);
+            User user = authcService.authenticate(shieldAction, request, null);
+            authzService.authorize(user, shieldAction, request);
         }
     }
 
@@ -66,8 +70,8 @@ public interface ServerTransportFilter {
     public static class ClientProfile extends NodeProfile {
 
         @Inject
-        public ClientProfile(AuthenticationService authcService, AuthorizationService authzService) {
+        public ClientProfile(AuthenticationService authcService, AuthorizationService authzService, ShieldActionMapper actionMapper) {
-            super(authcService, authzService);
+            super(authcService, authzService, actionMapper);
         }
 
         @Override

src/test/java/org/elasticsearch/shield/action/ShieldActionFilterTests.java

@@ -47,7 +47,7 @@ public class ShieldActionFilterTests extends ElasticsearchTestCase {
         signatureService = mock(SignatureService.class);
         auditTrail = mock(AuditTrail.class);
         licenseEventsNotifier = new MockLicenseEventsNotifier();
-        filter = new ShieldActionFilter(ImmutableSettings.EMPTY, authcService, authzService, signatureService, auditTrail, licenseEventsNotifier);
+        filter = new ShieldActionFilter(ImmutableSettings.EMPTY, authcService, authzService, signatureService, auditTrail, licenseEventsNotifier, new ShieldActionMapper());
     }
 
     @Test

src/test/java/org/elasticsearch/shield/transport/ServerTransportFilterTests.java

@@ -6,6 +6,7 @@
 package org.elasticsearch.shield.transport;
 
 import org.elasticsearch.shield.User;
+import org.elasticsearch.shield.action.ShieldActionMapper;
 import org.elasticsearch.shield.authc.AuthenticationException;
 import org.elasticsearch.shield.authc.AuthenticationService;
 import org.elasticsearch.shield.authz.AuthorizationException;
@@ -31,7 +32,7 @@ public class ServerTransportFilterTests extends ElasticsearchTestCase {
     public void init() throws Exception {
         authcService = mock(AuthenticationService.class);
         authzService = mock(AuthorizationService.class);
-        filter = new ServerTransportFilter.NodeProfile(authcService, authzService);
+        filter = new ServerTransportFilter.NodeProfile(authcService, authzService, new ShieldActionMapper());
     }
 
     @Test

src/test/java/org/elasticsearch/test/ShieldClearScrollTests.java

@@ -14,7 +14,7 @@ import org.elasticsearch.action.search.MultiSearchResponse;
 import org.elasticsearch.action.search.SearchPhaseExecutionException;
 import org.elasticsearch.shield.authc.support.SecuredString;
 import org.elasticsearch.shield.authz.AuthorizationException;
-import org.elasticsearch.test.junit.annotations.TestLogging;
+import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -24,6 +24,7 @@ import java.util.List;
 import static org.elasticsearch.shield.authc.support.UsernamePasswordToken.basicAuthHeaderValue;
 import static org.elasticsearch.test.ElasticsearchIntegrationTest.ClusterScope;
 import static org.elasticsearch.test.ElasticsearchIntegrationTest.Scope;
+import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertThrows;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.is;
 
@@ -77,8 +78,13 @@ public class ShieldClearScrollTests extends ShieldIntegrationTest {
         scrollIds = getScrollIds(multiSearchResponse);
     }
 
+    @After
+    public void clearScrolls() {
+        //clear all scroll ids from the default admin user, just in case any of test fails
+        client().prepareClearScroll().addScrollId("_all").get();
+    }
+
     @Test
-    @TestLogging("shield:TRACE")
     public void testThatClearingAllScrollIdsWorks() throws Exception {
         String shieldUser = "allowed_user:change_me";
         String basicAuth = basicAuthHeaderValue("allowed_user", new SecuredString("change_me".toCharArray()));
@@ -95,15 +101,11 @@ public class ShieldClearScrollTests extends ShieldIntegrationTest {
     public void testThatClearingAllScrollIdsRequirePermissions() throws Exception {
         String shieldUser = "denied_user:change_me";
         String basicAuth = basicAuthHeaderValue("denied_user", new SecuredString("change_me".toCharArray()));
-        try {
+
-            internalCluster().transportClient().prepareClearScroll()
+        assertThrows(internalCluster().transportClient().prepareClearScroll()
                 .putHeader("shield.user", shieldUser)
                 .putHeader("Authorization", basicAuth)
-                .addScrollId("_all").get();
+                .addScrollId("_all"), AuthorizationException.class, "action [cluster:admin/indices/scroll/clear_all] is unauthorized for user [denied_user]");
-            fail("Expected AuthorizationException but did not happen");
-        } catch (AuthorizationException exc) {
-            // yay, we weren't allowed!
-        }
 
         // deletion of scroll ids should work
         ClearScrollResponse clearByIdScrollResponse = client().prepareClearScroll().setScrollIds(scrollIds).get();