Check authentication type using enum instead of string (#54145)

ywangd · web-flow · commit e244a3bc7a07 · 2020-03-26T14:29:31.000+11:00
Avoid string comparison when we can use safter enums. This refactor is a follow up for #52178. Resolves: #52511
diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Authentication.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Authentication.java
@@ -77,6 +77,10 @@ public RealmRef getLookedUpBy() {
         return lookedUpBy;
     }
 
+    /**
+     * Get the realm where the effective user comes from.
+     * The effective user is the es-security-runas-user if present or the authenticated user.
+     */
     public RealmRef getSourceRealm() {
         return lookedUpBy == null ? authenticatedBy : lookedUpBy;
     }
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
@@ -14,6 +14,7 @@
 import org.elasticsearch.xpack.core.security.action.GetApiKeyRequest;
 import org.elasticsearch.xpack.core.security.action.InvalidateApiKeyRequest;
 import org.elasticsearch.xpack.core.security.authc.Authentication;
+import org.elasticsearch.xpack.core.security.authc.Authentication.AuthenticationType;
 import org.elasticsearch.xpack.core.security.authz.permission.ClusterPermission;
 import org.elasticsearch.xpack.core.security.support.Automatons;
 
@@ -23,7 +24,6 @@
 public class ManageOwnApiKeyClusterPrivilege implements NamedClusterPrivilege {
     public static final ManageOwnApiKeyClusterPrivilege INSTANCE = new ManageOwnApiKeyClusterPrivilege();
     private static final String PRIVILEGE_NAME = "manage_own_api_key";
-    private static final String API_KEY_REALM_TYPE = "_es_api_key";
     private static final String API_KEY_ID_KEY = "_security_api_key_id";
 
     private ManageOwnApiKeyClusterPrivilege() {
@@ -78,7 +78,7 @@ private boolean checkIfUserIsOwnerOfApiKeys(Authentication authentication, Strin
                  * 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.
                  */
-                if (authentication.getSourceRealm().getType().equals(API_KEY_REALM_TYPE)) {
+                if (AuthenticationType.API_KEY == authentication.getAuthenticationType()) {
                     // API key cannot own any other API key so deny access
                     return false;
                 } else if (ownedByAuthenticatedUser) {
@@ -93,7 +93,7 @@ private boolean checkIfUserIsOwnerOfApiKeys(Authentication authentication, Strin
         }
 
         private boolean isCurrentAuthenticationUsingSameApiKeyIdFromRequest(Authentication authentication, String apiKeyId) {
-            if (authentication.getSourceRealm().getType().equals(API_KEY_REALM_TYPE)) {
+            if (AuthenticationType.API_KEY == authentication.getAuthenticationType()) {
                 // API key id from authentication must match the id from request
                 final String authenticatedApiKeyId = (String) authentication.getMetadata().get(API_KEY_ID_KEY);
                 if (Strings.hasText(apiKeyId)) {
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
@@ -13,6 +13,7 @@
 import org.elasticsearch.xpack.core.security.action.GetApiKeyRequest;
 import org.elasticsearch.xpack.core.security.action.InvalidateApiKeyRequest;
 import org.elasticsearch.xpack.core.security.authc.Authentication;
+import org.elasticsearch.xpack.core.security.authc.Authentication.AuthenticationType;
 import org.elasticsearch.xpack.core.security.authz.permission.ClusterPermission;
 import org.elasticsearch.xpack.core.security.user.User;
 
@@ -28,8 +29,8 @@ public void testAuthenticationWithApiKeyAllowsAccessToApiKeyActionsWhenItIsOwner
             ManageOwnApiKeyClusterPrivilege.INSTANCE.buildPermission(ClusterPermission.builder()).build();
 
         final String apiKeyId = randomAlphaOfLengthBetween(4, 7);
-        final Authentication authentication = createMockAuthentication("joe","_es_api_key", "_es_api_key",
-            Map.of("_security_api_key_id", apiKeyId));
+        final Authentication authentication = createMockAuthentication("joe","_es_api_key",
+            AuthenticationType.API_KEY, Map.of("_security_api_key_id", apiKeyId));
         final TransportRequest getApiKeyRequest = GetApiKeyRequest.usingApiKeyId(apiKeyId, randomBoolean());
         final TransportRequest invalidateApiKeyRequest = InvalidateApiKeyRequest.usingApiKeyId(apiKeyId, randomBoolean());
 
@@ -43,8 +44,8 @@ public void testAuthenticationWithApiKeyDeniesAccessToApiKeyActionsWhenItIsNotOw
             ManageOwnApiKeyClusterPrivilege.INSTANCE.buildPermission(ClusterPermission.builder()).build();
 
         final String apiKeyId = randomAlphaOfLengthBetween(4, 7);
-        final Authentication authentication = createMockAuthentication("joe","_es_api_key", "_es_api_key",
-            Map.of("_security_api_key_id", randomAlphaOfLength(7)));
+        final Authentication authentication = createMockAuthentication("joe","_es_api_key",
+            AuthenticationType.API_KEY, Map.of("_security_api_key_id", randomAlphaOfLength(7)));
         final TransportRequest getApiKeyRequest = GetApiKeyRequest.usingApiKeyId(apiKeyId, randomBoolean());
         final TransportRequest invalidateApiKeyRequest = InvalidateApiKeyRequest.usingApiKeyId(apiKeyId, randomBoolean());
 
@@ -56,7 +57,8 @@ public void testAuthenticationWithUserAllowsAccessToApiKeyActionsWhenItIsOwner()
         final ClusterPermission clusterPermission =
             ManageOwnApiKeyClusterPrivilege.INSTANCE.buildPermission(ClusterPermission.builder()).build();
 
-        final Authentication authentication = createMockAuthentication("joe","realm1", "native", Map.of());
+        final Authentication authentication = createMockAuthentication("joe","realm1",
+            AuthenticationType.REALM, Map.of());
         final TransportRequest getApiKeyRequest = GetApiKeyRequest.usingRealmAndUserName("realm1", "joe");
         final TransportRequest invalidateApiKeyRequest = InvalidateApiKeyRequest.usingRealmAndUserName("realm1", "joe");
 
@@ -69,7 +71,8 @@ public void testAuthenticationWithUserAllowsAccessToApiKeyActionsWhenItIsOwner_W
         final ClusterPermission clusterPermission =
             ManageOwnApiKeyClusterPrivilege.INSTANCE.buildPermission(ClusterPermission.builder()).build();
 
-        final Authentication authentication = createMockAuthentication("joe","realm1", "native", Map.of());
+        final Authentication authentication = createMockAuthentication("joe","realm1",
+            AuthenticationType.REALM, Map.of());
         final TransportRequest getApiKeyRequest = GetApiKeyRequest.forOwnedApiKeys();
         final TransportRequest invalidateApiKeyRequest = InvalidateApiKeyRequest.forOwnedApiKeys();
 
@@ -82,7 +85,8 @@ public void testAuthenticationWithUserDeniesAccessToApiKeyActionsWhenItIsNotOwne
         final ClusterPermission clusterPermission =
             ManageOwnApiKeyClusterPrivilege.INSTANCE.buildPermission(ClusterPermission.builder()).build();
 
-        final Authentication authentication = createMockAuthentication("joe", "realm1", "native", Map.of());
+        final Authentication authentication = createMockAuthentication("joe", "realm1",
+            AuthenticationType.REALM, Map.of());
         final TransportRequest getApiKeyRequest = randomFrom(
             GetApiKeyRequest.usingRealmAndUserName("realm1", randomAlphaOfLength(7)),
             GetApiKeyRequest.usingRealmAndUserName(randomAlphaOfLength(5), "joe"),
@@ -110,14 +114,15 @@ public void testGetAndInvalidateApiKeyWillRespectRunAsUser() {
             InvalidateApiKeyRequest.usingRealmAndUserName("realm_b", "user_b"), authentication));
     }
 
-    private Authentication createMockAuthentication(String username, String realmName, String realmType, Map<String, Object> metadata) {
+    private Authentication createMockAuthentication(String username, String realmName,
+                                                    AuthenticationType authenticationType, Map<String, Object> metadata) {
         final User user = new User(username);
         final Authentication authentication = mock(Authentication.class);
         final Authentication.RealmRef authenticatedBy = mock(Authentication.RealmRef.class);
         when(authentication.getUser()).thenReturn(user);
         when(authentication.getSourceRealm()).thenReturn(authenticatedBy);
+        when(authentication.getAuthenticationType()).thenReturn(authenticationType);
         when(authenticatedBy.getName()).thenReturn(realmName);
-        when(authenticatedBy.getType()).thenReturn(realmType);
         when(authentication.getMetadata()).thenReturn(metadata);
         return authentication;
     }
@@ -136,6 +141,7 @@ private Authentication createMockRunAsAuthentication(String username, String rea
         when(authentication.getAuthenticatedBy()).thenReturn(authenticatedBy);
         when(authentication.getSourceRealm()).thenReturn(lookedUpBy);
         when(authentication.getMetadata()).thenReturn(Map.of());
+        when(authentication.getAuthenticationType()).thenReturn(AuthenticationType.REALM);
         return authentication;
     }
 }
diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java
@@ -102,6 +102,7 @@
 import static org.elasticsearch.search.SearchService.DEFAULT_KEEPALIVE_SETTING;
 import static org.elasticsearch.xpack.core.ClientHelper.SECURITY_ORIGIN;
 import static org.elasticsearch.xpack.core.ClientHelper.executeAsyncWithOrigin;
+import static org.elasticsearch.xpack.core.security.authc.Authentication.AuthenticationType;
 import static org.elasticsearch.xpack.core.security.index.RestrictedIndicesNames.SECURITY_MAIN_ALIAS;
 
 public class ApiKeyService {
@@ -343,7 +344,7 @@ private void loadApiKeyAndValidateCredentials(ThreadContext ctx, ApiKeyCredentia
      * retrieval of role descriptors that are associated with the api key
      */
     public void getRoleForApiKey(Authentication authentication, ActionListener<ApiKeyRoleDescriptors> listener) {
-        if (authentication.getAuthenticationType() != Authentication.AuthenticationType.API_KEY) {
+        if (authentication.getAuthenticationType() != AuthenticationType.API_KEY) {
             throw new IllegalStateException("authentication type must be api key but is " + authentication.getAuthenticationType());
         }
 
@@ -894,7 +895,7 @@ public void getApiKeys(String realmName, String username, String apiKeyName, Str
      * @return realm name
      */
     public static String getCreatorRealmName(final Authentication authentication) {
-        if (authentication.getAuthenticatedBy().getType().equals(API_KEY_REALM_TYPE)) {
+        if (AuthenticationType.API_KEY == authentication.getAuthenticationType()) {
             return (String) authentication.getMetadata().get(API_KEY_CREATOR_REALM_NAME);
         } else {
             return authentication.getSourceRealm().getName();
@@ -909,7 +910,7 @@ public static String getCreatorRealmName(final Authentication authentication) {
      * @return realm type
      */
     public static String getCreatorRealmType(final Authentication authentication) {
-        if (authentication.getAuthenticatedBy().getType().equals(API_KEY_REALM_TYPE)) {
+        if (AuthenticationType.API_KEY == authentication.getAuthenticationType()) {
             return (String) authentication.getMetadata().get(API_KEY_CREATOR_REALM_TYPE);
         } else {
             return authentication.getSourceRealm().getType();
diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java
@@ -42,6 +42,7 @@
 import org.elasticsearch.xpack.core.security.action.user.HasPrivilegesRequest;
 import org.elasticsearch.xpack.core.security.action.user.HasPrivilegesResponse;
 import org.elasticsearch.xpack.core.security.authc.Authentication;
+import org.elasticsearch.xpack.core.security.authc.Authentication.AuthenticationType;
 import org.elasticsearch.xpack.core.security.authc.AuthenticationFailureHandler;
 import org.elasticsearch.xpack.core.security.authc.esnative.ClientReservedRealm;
 import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine;
@@ -585,7 +586,7 @@ private ElasticsearchSecurityException denialException(Authentication authentica
                     authentication.getUser().principal());
         }
         // check for authentication by API key
-        if (authentication.getAuthenticatedBy().getType().equals(ApiKeyService.API_KEY_REALM_TYPE)) {
+        if (AuthenticationType.API_KEY == authentication.getAuthenticationType()) {
             final String apiKeyId = (String) authentication.getMetadata().get(ApiKeyService.API_KEY_ID_KEY);
             assert apiKeyId != null : "api key id must be present in the metadata";
             logger.debug("action [{}] is unauthorized for API key id [{}] of user [{}]", action, apiKeyId, authUser.principal());
diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java
@@ -47,6 +47,7 @@
 import org.elasticsearch.xpack.core.security.action.user.HasPrivilegesResponse;
 import org.elasticsearch.xpack.core.security.action.user.UserRequest;
 import org.elasticsearch.xpack.core.security.authc.Authentication;
+import org.elasticsearch.xpack.core.security.authc.Authentication.AuthenticationType;
 import org.elasticsearch.xpack.core.security.authc.esnative.NativeRealmSettings;
 import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine;
 import org.elasticsearch.xpack.core.security.authz.ResolvedIndices;
@@ -179,7 +180,7 @@ boolean checkSameUserPermissions(String action, TransportRequest request, Authen
                 return sameUsername;
             } else if (request instanceof GetApiKeyRequest) {
                 GetApiKeyRequest getApiKeyRequest = (GetApiKeyRequest) request;
-                if (authentication.getAuthenticatedBy().getType().equals(ApiKeyService.API_KEY_REALM_TYPE)) {
+                if (AuthenticationType.API_KEY == authentication.getAuthenticationType()) {
                     assert authentication.getLookedUpBy() == null : "runAs not supported for api key authentication";
                     // if authenticated by API key then the request must also contain same API key id
                     String authenticatedApiKeyId = (String) authentication.getMetadata().get(ApiKeyService.API_KEY_ID_KEY);
@@ -542,8 +543,8 @@ private static boolean checkChangePasswordAction(Authentication authentication)
         // Ensure that the user is not authenticated with an access token or an API key.
         // Also ensure that the user was authenticated by a realm that we can change a password for. The native realm is an internal realm
         // and right now only one can exist in the realm configuration - if this changes we should update this check
-        final Authentication.AuthenticationType authType = authentication.getAuthenticationType();
-        return (authType.equals(Authentication.AuthenticationType.REALM)
+        final AuthenticationType authType = authentication.getAuthenticationType();
+        return (authType.equals(AuthenticationType.REALM)
             && (ReservedRealm.TYPE.equals(realmType) || NativeRealmSettings.TYPE.equals(realmType)));
     }
 
diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java
@@ -36,6 +36,7 @@
 import org.elasticsearch.xpack.core.security.action.user.PutUserAction;
 import org.elasticsearch.xpack.core.security.action.user.UserRequest;
 import org.elasticsearch.xpack.core.security.authc.Authentication;
+import org.elasticsearch.xpack.core.security.authc.Authentication.AuthenticationType;
 import org.elasticsearch.xpack.core.security.authc.esnative.NativeRealmSettings;
 import org.elasticsearch.xpack.core.security.authc.file.FileRealmSettings;
 import org.elasticsearch.xpack.core.security.authc.ldap.LdapRealmSettings;
@@ -291,7 +292,7 @@ public void testSameUserPermissionAllowsSelfApiKeyInfoRetrievalWhenAuthenticated
         final Authentication.RealmRef authenticatedBy = mock(Authentication.RealmRef.class);
         when(authentication.getUser()).thenReturn(user);
         when(authentication.getAuthenticatedBy()).thenReturn(authenticatedBy);
-        when(authenticatedBy.getType()).thenReturn(ApiKeyService.API_KEY_REALM_TYPE);
+        when(authentication.getAuthenticationType()).thenReturn(AuthenticationType.API_KEY);
         when(authentication.getMetadata()).thenReturn(Map.of(ApiKeyService.API_KEY_ID_KEY, apiKeyId));
 
         assertTrue(engine.checkSameUserPermissions(GetApiKeyAction.NAME, request, authentication));
@@ -321,7 +322,7 @@ public void testSameUserPermissionDeniesApiKeyInfoRetrievalWhenLookedupByIsPrese
         when(authentication.getUser()).thenReturn(user);
         when(authentication.getAuthenticatedBy()).thenReturn(authenticatedBy);
         when(authentication.getLookedUpBy()).thenReturn(lookedupBy);
-        when(authenticatedBy.getType()).thenReturn(ApiKeyService.API_KEY_REALM_TYPE);
+        when(authentication.getAuthenticationType()).thenReturn(AuthenticationType.API_KEY);
         when(authentication.getMetadata()).thenReturn(Map.of(ApiKeyService.API_KEY_ID_KEY, randomAlphaOfLengthBetween(4, 7)));
 
         final AssertionError assertionError = expectThrows(AssertionError.class, () -> engine.checkSameUserPermissions(GetApiKeyAction.NAME,

Original file line number	Diff line number	Diff line change
`@@ -77,6 +77,10 @@ public RealmRef getLookedUpBy() {`
`77`	`77`	`return lookedUpBy;`
`78`	`78`	`}`
`79`	`79`
	`80`	`+ /**`
	`81`	`+ * Get the realm where the effective user comes from.`
	`82`	`+ * The effective user is the es-security-runas-user if present or the authenticated user.`
	`83`	`+ */`
`80`	`84`	`public RealmRef getSourceRealm() {`
`81`	`85`	`return lookedUpBy == null ? authenticatedBy : lookedUpBy;`
`82`	`86`	`}`