change manage own api cluster privielge

Yogesh Gaikwad · Yogesh Gaikwad · commit adc6d69898e1 · 2019-08-20T18:04:10.000+10:00
diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/ClusterPermission.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/ClusterPermission.java
@@ -152,11 +152,11 @@ public interface PermissionCheck {
     }
 
     // Automaton based permission check
-    private static class AutomatonPermissionCheck implements PermissionCheck {
+    public static class AutomatonPermissionCheck implements PermissionCheck {
         private final Automaton automaton;
         private final Predicate<String> actionPredicate;
 
-        AutomatonPermissionCheck(final Automaton automaton) {
+        public AutomatonPermissionCheck(final Automaton automaton) {
             this.automaton = automaton;
             this.actionPredicate = Automatons.predicate(automaton);
         }
diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/ManageOwnApiKeyClusterPrivilege.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/ManageOwnApiKeyClusterPrivilege.java
@@ -17,61 +17,14 @@
 import org.elasticsearch.xpack.core.security.authz.permission.ClusterPermission;
 import org.elasticsearch.xpack.core.security.support.Automatons;
 
-import java.util.function.BiPredicate;
-import java.util.function.Predicate;
-
 /**
  * Named cluster privilege for managing API keys owned by the current authenticated user.
  */
 public class ManageOwnApiKeyClusterPrivilege implements NamedClusterPrivilege {
     public static final ManageOwnApiKeyClusterPrivilege INSTANCE = new ManageOwnApiKeyClusterPrivilege();
-    private static final Predicate<String> ACTION_PREDICATE = Automatons.predicate("cluster:admin/xpack/security/api_key/*");
     private static final String PRIVILEGE_NAME = "manage_own_api_key";
-    private final BiPredicate<TransportRequest, Authentication> requestAuthnPredicate;
 
     private ManageOwnApiKeyClusterPrivilege() {
-        this.requestAuthnPredicate = (request, authentication) -> {
-            if (request instanceof CreateApiKeyRequest) {
-                return true;
-            } else if (request instanceof GetApiKeyRequest) {
-                final GetApiKeyRequest getApiKeyRequest = (GetApiKeyRequest) request;
-                return checkIfUserIsOwnerOfApiKeys(authentication, getApiKeyRequest.getApiKeyId(), getApiKeyRequest.getUserName(),
-                    getApiKeyRequest.getRealmName());
-            } else if (request instanceof InvalidateApiKeyRequest) {
-                final InvalidateApiKeyRequest invalidateApiKeyRequest = (InvalidateApiKeyRequest) request;
-                return checkIfUserIsOwnerOfApiKeys(authentication, invalidateApiKeyRequest.getId(), invalidateApiKeyRequest.getUserName(),
-                    invalidateApiKeyRequest.getRealmName());
-            }
-            return false;
-        };
-    }
-
-    private boolean checkIfUserIsOwnerOfApiKeys(Authentication authentication, String apiKeyId, String username, String realmName) {
-        if (isCurrentAuthenticationUsingSameApiKeyIdFromRequest(authentication, apiKeyId)) {
-            return true;
-        } else {
-            /*
-             * TODO bizybot we need to think on how we can propagate appropriate error message to the end user when username, realm name
-             *   is missing. This is similar to the problem of propagating right error messages in case of access denied.
-             */
-            String authenticatedUserPrincipal = authentication.getUser().principal();
-            String authenticatedUserRealm = authentication.getAuthenticatedBy().getName();
-            if (Strings.hasText(username) && Strings.hasText(realmName)) {
-                return username.equals(authenticatedUserPrincipal) && realmName.equals(authenticatedUserRealm);
-            }
-        }
-        return false;
-    }
-
-    private boolean isCurrentAuthenticationUsingSameApiKeyIdFromRequest(Authentication authentication, String apiKeyId) {
-        if (authentication.getAuthenticatedBy().getType().equals("_es_api_key")) {
-            // API key id from authentication must match the id from request
-            String authenticatedApiKeyId = (String) authentication.getMetadata().get("_security_api_key_id");
-            if (Strings.hasText(apiKeyId)) {
-                return apiKeyId.equals(authenticatedApiKeyId);
-            }
-        }
-        return false;
     }
 
     @Override
@@ -81,6 +34,64 @@ public String name() {
 
     @Override
     public ClusterPermission.Builder buildPermission(ClusterPermission.Builder builder) {
-        return builder.add(this, ACTION_PREDICATE, requestAuthnPredicate);
+        return builder.add(this, ManageOwnClusterPermissionCheck.INSTANCE);
+    }
+
+    private static final class ManageOwnClusterPermissionCheck extends ClusterPermission.AutomatonPermissionCheck {
+        public static final ManageOwnClusterPermissionCheck INSTANCE = new ManageOwnClusterPermissionCheck();
+        private ManageOwnClusterPermissionCheck() {
+            super(Automatons.patterns("cluster:admin/xpack/security/api_key/*"));
+        }
+
+        @Override
+        public boolean check(final String action, final TransportRequest request, final Authentication authentication) {
+            if (super.check(action, request, authentication)) {
+                if (request instanceof CreateApiKeyRequest) {
+                    return true;
+                } else if (request instanceof GetApiKeyRequest) {
+                    final GetApiKeyRequest getApiKeyRequest = (GetApiKeyRequest) request;
+                    return checkIfUserIsOwnerOfApiKeys(authentication, getApiKeyRequest.getApiKeyId(), getApiKeyRequest.getUserName(),
+                        getApiKeyRequest.getRealmName());
+                } else if (request instanceof InvalidateApiKeyRequest) {
+                    final InvalidateApiKeyRequest invalidateApiKeyRequest = (InvalidateApiKeyRequest) request;
+                    return checkIfUserIsOwnerOfApiKeys(authentication, invalidateApiKeyRequest.getId(), invalidateApiKeyRequest.getUserName(),
+                        invalidateApiKeyRequest.getRealmName());
+                }
+            }
+            return false;
+        }
+
+        @Override
+        public boolean implies(final ClusterPermission.PermissionCheck permissionCheck) {
+            return super.implies(permissionCheck);
+        }
+
+        private boolean checkIfUserIsOwnerOfApiKeys(Authentication authentication, String apiKeyId, String username, String realmName) {
+            if (isCurrentAuthenticationUsingSameApiKeyIdFromRequest(authentication, apiKeyId)) {
+                return true;
+            } else {
+                /*
+                 * TODO bizybot we need to think on how we can propagate appropriate error message to the end user when username, realm name
+                 *   is missing. This is similar to the problem of propagating right error messages in case of access denied.
+                 */
+                String authenticatedUserPrincipal = authentication.getUser().principal();
+                String authenticatedUserRealm = authentication.getAuthenticatedBy().getName();
+                if (Strings.hasText(username) && Strings.hasText(realmName)) {
+                    return username.equals(authenticatedUserPrincipal) && realmName.equals(authenticatedUserRealm);
+                }
+            }
+            return false;
+        }
+
+        private boolean isCurrentAuthenticationUsingSameApiKeyIdFromRequest(Authentication authentication, String apiKeyId) {
+            if (authentication.getAuthenticatedBy().getType().equals("_es_api_key")) {
+                // API key id from authentication must match the id from request
+                String authenticatedApiKeyId = (String) authentication.getMetadata().get("_security_api_key_id");
+                if (Strings.hasText(apiKeyId)) {
+                    return apiKeyId.equals(authenticatedApiKeyId);
+                }
+            }
+            return false;
+        }
     }
-}
+ }
diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/ManageOwnApiKeyClusterPrivilegeTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/ManageOwnApiKeyClusterPrivilegeTests.java
@@ -23,55 +23,61 @@
 
 public class ManageOwnApiKeyClusterPrivilegeTests extends ESTestCase {
 
-    public void testActionRequestAuthenticationBasedPredicateWhenAuthenticatingWithApiKey() {
+    public void testAuthenticationWithApiKeyAllowsAccessToApiKeyActionsWhenItIsOwner() {
         final ClusterPermission clusterPermission =
             ManageOwnApiKeyClusterPrivilege.INSTANCE.buildPermission(ClusterPermission.builder()).build();
-        {
-            final String apiKeyId = randomAlphaOfLengthBetween(4, 7);
-            final Authentication authentication = createMockAuthentication("_es_api_key", "_es_api_key",
-                Map.of("_security_api_key_id", apiKeyId));
-            final TransportRequest request = randomFrom(GetApiKeyRequest.usingApiKeyId(apiKeyId, randomBoolean()),
-                InvalidateApiKeyRequest.usingApiKeyId(apiKeyId, randomBoolean()));
-
-            assertTrue(clusterPermission.check("cluster:admin/xpack/security/api_key/get", request, authentication));
-            assertTrue(clusterPermission.check("cluster:admin/xpack/security/api_key/invalidate", request, authentication));
-            assertFalse(clusterPermission.check("cluster:admin/something", request, authentication));
-        }
-        {
-            final String apiKeyId = randomAlphaOfLengthBetween(4, 7);
-            final Authentication authentication = createMockAuthentication("_es_api_key", "_es_api_key",
-                Map.of("_security_api_key_id", randomAlphaOfLength(7)));
-            final TransportRequest request = randomFrom(GetApiKeyRequest.usingApiKeyId(apiKeyId, randomBoolean()),
-                InvalidateApiKeyRequest.usingApiKeyId(apiKeyId, randomBoolean()));
-
-            assertFalse(clusterPermission.check("cluster:admin/xpack/security/api_key/get", request, authentication));
-            assertFalse(clusterPermission.check("cluster:admin/xpack/security/api_key/invalidate", request, authentication));
-        }
+
+        final String apiKeyId = randomAlphaOfLengthBetween(4, 7);
+        final Authentication authentication = createMockAuthentication("_es_api_key", "_es_api_key",
+            Map.of("_security_api_key_id", apiKeyId));
+        final TransportRequest request = randomFrom(GetApiKeyRequest.usingApiKeyId(apiKeyId, randomBoolean()),
+            InvalidateApiKeyRequest.usingApiKeyId(apiKeyId, randomBoolean()));
+
+        assertTrue(clusterPermission.check("cluster:admin/xpack/security/api_key/get", request, authentication));
+        assertTrue(clusterPermission.check("cluster:admin/xpack/security/api_key/invalidate", request, authentication));
+        assertFalse(clusterPermission.check("cluster:admin/something", request, authentication));
+    }
+
+    public void testAuthenticationWithApiKeyDeniesAccessToApiKeyActionsWhenItIsNotOwner() {
+        final ClusterPermission clusterPermission =
+            ManageOwnApiKeyClusterPrivilege.INSTANCE.buildPermission(ClusterPermission.builder()).build();
+
+        final String apiKeyId = randomAlphaOfLengthBetween(4, 7);
+        final Authentication authentication = createMockAuthentication("_es_api_key", "_es_api_key",
+            Map.of("_security_api_key_id", randomAlphaOfLength(7)));
+        final TransportRequest request = randomFrom(GetApiKeyRequest.usingApiKeyId(apiKeyId, randomBoolean()),
+            InvalidateApiKeyRequest.usingApiKeyId(apiKeyId, randomBoolean()));
+
+        assertFalse(clusterPermission.check("cluster:admin/xpack/security/api_key/get", request, authentication));
+        assertFalse(clusterPermission.check("cluster:admin/xpack/security/api_key/invalidate", request, authentication));
     }
 
-    public void testActionRequestAuthenticationBasedPredicateWhenRequestContainsUsernameAndRealmName() {
+    public void testAuthenticationWithUserAllowsAccessToApiKeyActionsWhenItIsOwner() {
         final ClusterPermission clusterPermission =
             ManageOwnApiKeyClusterPrivilege.INSTANCE.buildPermission(ClusterPermission.builder()).build();
-        {
-            final Authentication authentication = createMockAuthentication("realm1", "native", Map.of());
-            final TransportRequest request = randomFrom(GetApiKeyRequest.usingRealmAndUserName("realm1", "joe"),
-                InvalidateApiKeyRequest.usingRealmAndUserName("realm1", "joe"));
-
-            assertTrue(clusterPermission.check("cluster:admin/xpack/security/api_key/get", request, authentication));
-            assertTrue(clusterPermission.check("cluster:admin/xpack/security/api_key/invalidate", request, authentication));
-            assertFalse(clusterPermission.check("cluster:admin/something", request, authentication));
-        }
-        {
-            final Authentication authentication = createMockAuthentication("realm1", "native", Map.of());
-            final TransportRequest request = randomFrom(
-                GetApiKeyRequest.usingRealmAndUserName("realm1", randomAlphaOfLength(7)),
-                GetApiKeyRequest.usingRealmAndUserName(randomAlphaOfLength(5), "joe"),
-                InvalidateApiKeyRequest.usingRealmAndUserName("realm1", randomAlphaOfLength(7)),
-                InvalidateApiKeyRequest.usingRealmAndUserName(randomAlphaOfLength(5), "joe"));
-
-            assertFalse(clusterPermission.check("cluster:admin/xpack/security/api_key/get", request, authentication));
-            assertFalse(clusterPermission.check("cluster:admin/xpack/security/api_key/invalidate", request, authentication));
-        }
+
+        final Authentication authentication = createMockAuthentication("realm1", "native", Map.of());
+        final TransportRequest request = randomFrom(GetApiKeyRequest.usingRealmAndUserName("realm1", "joe"),
+            InvalidateApiKeyRequest.usingRealmAndUserName("realm1", "joe"));
+
+        assertTrue(clusterPermission.check("cluster:admin/xpack/security/api_key/get", request, authentication));
+        assertTrue(clusterPermission.check("cluster:admin/xpack/security/api_key/invalidate", request, authentication));
+        assertFalse(clusterPermission.check("cluster:admin/something", request, authentication));
+    }
+
+    public void testAuthenticationWithUserAllowsAccessToApiKeyActionsWhenItIsNotOwner() {
+        final ClusterPermission clusterPermission =
+            ManageOwnApiKeyClusterPrivilege.INSTANCE.buildPermission(ClusterPermission.builder()).build();
+
+        final Authentication authentication = createMockAuthentication("realm1", "native", Map.of());
+        final TransportRequest request = randomFrom(
+            GetApiKeyRequest.usingRealmAndUserName("realm1", randomAlphaOfLength(7)),
+            GetApiKeyRequest.usingRealmAndUserName(randomAlphaOfLength(5), "joe"),
+            InvalidateApiKeyRequest.usingRealmAndUserName("realm1", randomAlphaOfLength(7)),
+            InvalidateApiKeyRequest.usingRealmAndUserName(randomAlphaOfLength(5), "joe"));
+
+        assertFalse(clusterPermission.check("cluster:admin/xpack/security/api_key/get", request, authentication));
+        assertFalse(clusterPermission.check("cluster:admin/xpack/security/api_key/invalidate", request, authentication));
     }
 
     private Authentication createMockAuthentication(String realmName, String realmType, Map<String, Object> metadata) {

Original file line number	Diff line number	Diff line change
`@@ -152,11 +152,11 @@ public interface PermissionCheck {`
`152`	`152`	`}`
`153`	`153`
`154`	`154`	`// Automaton based permission check`
`155`		`- private static class AutomatonPermissionCheck implements PermissionCheck {`
	`155`	`+ public static class AutomatonPermissionCheck implements PermissionCheck {`
`156`	`156`	`private final Automaton automaton;`
`157`	`157`	`private final Predicate<String> actionPredicate;`
`158`	`158`
`159`		`- AutomatonPermissionCheck(final Automaton automaton) {`
	`159`	`+ public AutomatonPermissionCheck(final Automaton automaton) {`
`160`	`160`	`this.automaton = automaton;`
`161`	`161`	`this.actionPredicate = Automatons.predicate(automaton);`
`162`	`162`	`}`