Skip to content

Commit 1afd510

Browse files
ywangdywangd
authored
Check authentication type using enum instead of string (#54145) (#54246)
Avoid string comparison when we can use safer enums. This refactor is a follow up for #52178. Resolves: #52511
1 parent 6af89e6 commit 1afd510

File tree

7 files changed

+35
-21
lines changed

7 files changed

+35
-21
lines changed

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Authentication.java

+4
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,10 @@ public RealmRef getLookedUpBy() {
8282
return lookedUpBy;
8383
}
8484

85+
/**
86+
* Get the realm where the effective user comes from.
87+
* The effective user is the es-security-runas-user if present or the authenticated user.
88+
*/
8589
public RealmRef getSourceRealm() {
8690
return lookedUpBy == null ? authenticatedBy : lookedUpBy;
8791
}

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/ManageOwnApiKeyClusterPrivilege.java

+3-3
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import org.elasticsearch.xpack.core.security.action.GetApiKeyRequest;
1515
import org.elasticsearch.xpack.core.security.action.InvalidateApiKeyRequest;
1616
import org.elasticsearch.xpack.core.security.authc.Authentication;
17+
import org.elasticsearch.xpack.core.security.authc.Authentication.AuthenticationType;
1718
import org.elasticsearch.xpack.core.security.authz.permission.ClusterPermission;
1819
import org.elasticsearch.xpack.core.security.support.Automatons;
1920

@@ -23,7 +24,6 @@
2324
public class ManageOwnApiKeyClusterPrivilege implements NamedClusterPrivilege {
2425
public static final ManageOwnApiKeyClusterPrivilege INSTANCE = new ManageOwnApiKeyClusterPrivilege();
2526
private static final String PRIVILEGE_NAME = "manage_own_api_key";
26-
private static final String API_KEY_REALM_TYPE = "_es_api_key";
2727
private static final String API_KEY_ID_KEY = "_security_api_key_id";
2828

2929
private ManageOwnApiKeyClusterPrivilege() {
@@ -78,7 +78,7 @@ private boolean checkIfUserIsOwnerOfApiKeys(Authentication authentication, Strin
7878
* TODO bizybot we need to think on how we can propagate appropriate error message to the end user when username, realm name
7979
* is missing. This is similar to the problem of propagating right error messages in case of access denied.
8080
*/
81-
if (authentication.getSourceRealm().getType().equals(API_KEY_REALM_TYPE)) {
81+
if (AuthenticationType.API_KEY == authentication.getAuthenticationType()) {
8282
// API key cannot own any other API key so deny access
8383
return false;
8484
} else if (ownedByAuthenticatedUser) {
@@ -93,7 +93,7 @@ private boolean checkIfUserIsOwnerOfApiKeys(Authentication authentication, Strin
9393
}
9494

9595
private boolean isCurrentAuthenticationUsingSameApiKeyIdFromRequest(Authentication authentication, String apiKeyId) {
96-
if (authentication.getSourceRealm().getType().equals(API_KEY_REALM_TYPE)) {
96+
if (AuthenticationType.API_KEY == authentication.getAuthenticationType()) {
9797
// API key id from authentication must match the id from request
9898
final String authenticatedApiKeyId = (String) authentication.getMetadata().get(API_KEY_ID_KEY);
9999
if (Strings.hasText(apiKeyId)) {

‎x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/ManageOwnApiKeyClusterPrivilegeTests.java

+15-9
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import org.elasticsearch.xpack.core.security.action.GetApiKeyRequest;
1414
import org.elasticsearch.xpack.core.security.action.InvalidateApiKeyRequest;
1515
import org.elasticsearch.xpack.core.security.authc.Authentication;
16+
import org.elasticsearch.xpack.core.security.authc.Authentication.AuthenticationType;
1617
import org.elasticsearch.xpack.core.security.authz.permission.ClusterPermission;
1718
import org.elasticsearch.xpack.core.security.user.User;
1819

@@ -29,8 +30,8 @@ public void testAuthenticationWithApiKeyAllowsAccessToApiKeyActionsWhenItIsOwner
2930
ManageOwnApiKeyClusterPrivilege.INSTANCE.buildPermission(ClusterPermission.builder()).build();
3031

3132
final String apiKeyId = randomAlphaOfLengthBetween(4, 7);
32-
final Authentication authentication = createMockAuthentication("joe","_es_api_key", "_es_api_key",
33-
Collections.singletonMap("_security_api_key_id", apiKeyId));
33+
final Authentication authentication = createMockAuthentication("joe","_es_api_key",
34+
AuthenticationType.API_KEY, Collections.singletonMap("_security_api_key_id", apiKeyId));
3435
final TransportRequest getApiKeyRequest = GetApiKeyRequest.usingApiKeyId(apiKeyId, randomBoolean());
3536
final TransportRequest invalidateApiKeyRequest = InvalidateApiKeyRequest.usingApiKeyId(apiKeyId, randomBoolean());
3637

@@ -44,8 +45,8 @@ public void testAuthenticationWithApiKeyDeniesAccessToApiKeyActionsWhenItIsNotOw
4445
ManageOwnApiKeyClusterPrivilege.INSTANCE.buildPermission(ClusterPermission.builder()).build();
4546

4647
final String apiKeyId = randomAlphaOfLengthBetween(4, 7);
47-
final Authentication authentication = createMockAuthentication("joe","_es_api_key", "_es_api_key",
48-
Collections.singletonMap("_security_api_key_id", randomAlphaOfLength(7)));
48+
final Authentication authentication = createMockAuthentication("joe","_es_api_key",
49+
AuthenticationType.API_KEY, Collections.singletonMap("_security_api_key_id", randomAlphaOfLength(7)));
4950
final TransportRequest getApiKeyRequest = GetApiKeyRequest.usingApiKeyId(apiKeyId, randomBoolean());
5051
final TransportRequest invalidateApiKeyRequest = InvalidateApiKeyRequest.usingApiKeyId(apiKeyId, randomBoolean());
5152

@@ -57,7 +58,8 @@ public void testAuthenticationWithUserAllowsAccessToApiKeyActionsWhenItIsOwner()
5758
final ClusterPermission clusterPermission =
5859
ManageOwnApiKeyClusterPrivilege.INSTANCE.buildPermission(ClusterPermission.builder()).build();
5960

60-
final Authentication authentication = createMockAuthentication("joe","realm1", "native", Collections.emptyMap());
61+
final Authentication authentication = createMockAuthentication("joe","realm1",
62+
AuthenticationType.REALM, Collections.emptyMap());
6163
final TransportRequest getApiKeyRequest = GetApiKeyRequest.usingRealmAndUserName("realm1", "joe");
6264
final TransportRequest invalidateApiKeyRequest = InvalidateApiKeyRequest.usingRealmAndUserName("realm1", "joe");
6365

@@ -70,7 +72,8 @@ public void testAuthenticationWithUserAllowsAccessToApiKeyActionsWhenItIsOwner_W
7072
final ClusterPermission clusterPermission =
7173
ManageOwnApiKeyClusterPrivilege.INSTANCE.buildPermission(ClusterPermission.builder()).build();
7274

73-
final Authentication authentication = createMockAuthentication("joe","realm1", "native", Collections.emptyMap());
75+
final Authentication authentication = createMockAuthentication("joe","realm1",
76+
AuthenticationType.REALM, Collections.emptyMap());
7477
final TransportRequest getApiKeyRequest = GetApiKeyRequest.forOwnedApiKeys();
7578
final TransportRequest invalidateApiKeyRequest = InvalidateApiKeyRequest.forOwnedApiKeys();
7679

@@ -83,7 +86,8 @@ public void testAuthenticationWithUserDeniesAccessToApiKeyActionsWhenItIsNotOwne
8386
final ClusterPermission clusterPermission =
8487
ManageOwnApiKeyClusterPrivilege.INSTANCE.buildPermission(ClusterPermission.builder()).build();
8588

86-
final Authentication authentication = createMockAuthentication("joe", "realm1", "native", Collections.emptyMap());
89+
final Authentication authentication = createMockAuthentication("joe", "realm1",
90+
AuthenticationType.REALM, Collections.emptyMap());
8791
final TransportRequest getApiKeyRequest = randomFrom(
8892
GetApiKeyRequest.usingRealmAndUserName("realm1", randomAlphaOfLength(7)),
8993
GetApiKeyRequest.usingRealmAndUserName(randomAlphaOfLength(5), "joe"),
@@ -111,14 +115,15 @@ public void testGetAndInvalidateApiKeyWillRespectRunAsUser() {
111115
InvalidateApiKeyRequest.usingRealmAndUserName("realm_b", "user_b"), authentication));
112116
}
113117

114-
private Authentication createMockAuthentication(String username, String realmName, String realmType, Map<String, Object> metadata) {
118+
private Authentication createMockAuthentication(String username, String realmName,
119+
AuthenticationType authenticationType, Map<String, Object> metadata) {
115120
final User user = new User(username);
116121
final Authentication authentication = mock(Authentication.class);
117122
final Authentication.RealmRef authenticatedBy = mock(Authentication.RealmRef.class);
118123
when(authentication.getUser()).thenReturn(user);
119124
when(authentication.getSourceRealm()).thenReturn(authenticatedBy);
125+
when(authentication.getAuthenticationType()).thenReturn(authenticationType);
120126
when(authenticatedBy.getName()).thenReturn(realmName);
121-
when(authenticatedBy.getType()).thenReturn(realmType);
122127
when(authentication.getMetadata()).thenReturn(metadata);
123128
return authentication;
124129
}
@@ -137,6 +142,7 @@ private Authentication createMockRunAsAuthentication(String username, String rea
137142
when(authentication.getAuthenticatedBy()).thenReturn(authenticatedBy);
138143
when(authentication.getSourceRealm()).thenReturn(lookedUpBy);
139144
when(authentication.getMetadata()).thenReturn(Collections.emptyMap());
145+
when(authentication.getAuthenticationType()).thenReturn(AuthenticationType.REALM);
140146
return authentication;
141147
}
142148
}

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java

+4-3
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@
104104
import static org.elasticsearch.search.SearchService.DEFAULT_KEEPALIVE_SETTING;
105105
import static org.elasticsearch.xpack.core.ClientHelper.SECURITY_ORIGIN;
106106
import static org.elasticsearch.xpack.core.ClientHelper.executeAsyncWithOrigin;
107+
import static org.elasticsearch.xpack.core.security.authc.Authentication.AuthenticationType;
107108
import static org.elasticsearch.xpack.core.security.index.RestrictedIndicesNames.SECURITY_MAIN_ALIAS;
108109

109110
public class ApiKeyService {
@@ -351,7 +352,7 @@ private void loadApiKeyAndValidateCredentials(ThreadContext ctx, ApiKeyCredentia
351352
* retrieval of role descriptors that are associated with the api key
352353
*/
353354
public void getRoleForApiKey(Authentication authentication, ActionListener<ApiKeyRoleDescriptors> listener) {
354-
if (authentication.getAuthenticationType() != Authentication.AuthenticationType.API_KEY) {
355+
if (authentication.getAuthenticationType() != AuthenticationType.API_KEY) {
355356
throw new IllegalStateException("authentication type must be api key but is " + authentication.getAuthenticationType());
356357
}
357358

@@ -902,7 +903,7 @@ public void getApiKeys(String realmName, String username, String apiKeyName, Str
902903
* @return realm name
903904
*/
904905
public static String getCreatorRealmName(final Authentication authentication) {
905-
if (authentication.getAuthenticatedBy().getType().equals(API_KEY_REALM_TYPE)) {
906+
if (AuthenticationType.API_KEY == authentication.getAuthenticationType()) {
906907
return (String) authentication.getMetadata().get(API_KEY_CREATOR_REALM_NAME);
907908
} else {
908909
return authentication.getSourceRealm().getName();
@@ -917,7 +918,7 @@ public static String getCreatorRealmName(final Authentication authentication) {
917918
* @return realm type
918919
*/
919920
public static String getCreatorRealmType(final Authentication authentication) {
920-
if (authentication.getAuthenticatedBy().getType().equals(API_KEY_REALM_TYPE)) {
921+
if (AuthenticationType.API_KEY == authentication.getAuthenticationType()) {
921922
return (String) authentication.getMetadata().get(API_KEY_CREATOR_REALM_TYPE);
922923
} else {
923924
return authentication.getSourceRealm().getType();

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

+2-1
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
import org.elasticsearch.xpack.core.security.action.user.HasPrivilegesRequest;
4343
import org.elasticsearch.xpack.core.security.action.user.HasPrivilegesResponse;
4444
import org.elasticsearch.xpack.core.security.authc.Authentication;
45+
import org.elasticsearch.xpack.core.security.authc.Authentication.AuthenticationType;
4546
import org.elasticsearch.xpack.core.security.authc.AuthenticationFailureHandler;
4647
import org.elasticsearch.xpack.core.security.authc.esnative.ClientReservedRealm;
4748
import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine;
@@ -585,7 +586,7 @@ private ElasticsearchSecurityException denialException(Authentication authentica
585586
authentication.getUser().principal());
586587
}
587588
// check for authentication by API key
588-
if (authentication.getAuthenticatedBy().getType().equals(ApiKeyService.API_KEY_REALM_TYPE)) {
589+
if (AuthenticationType.API_KEY == authentication.getAuthenticationType()) {
589590
final String apiKeyId = (String) authentication.getMetadata().get(ApiKeyService.API_KEY_ID_KEY);
590591
assert apiKeyId != null : "api key id must be present in the metadata";
591592
logger.debug("action [{}] is unauthorized for API key id [{}] of user [{}]", action, apiKeyId, authUser.principal());

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

+4-3
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
import org.elasticsearch.xpack.core.security.action.user.HasPrivilegesResponse;
4848
import org.elasticsearch.xpack.core.security.action.user.UserRequest;
4949
import org.elasticsearch.xpack.core.security.authc.Authentication;
50+
import org.elasticsearch.xpack.core.security.authc.Authentication.AuthenticationType;
5051
import org.elasticsearch.xpack.core.security.authc.esnative.NativeRealmSettings;
5152
import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine;
5253
import org.elasticsearch.xpack.core.security.authz.ResolvedIndices;
@@ -179,7 +180,7 @@ boolean checkSameUserPermissions(String action, TransportRequest request, Authen
179180
return sameUsername;
180181
} else if (request instanceof GetApiKeyRequest) {
181182
GetApiKeyRequest getApiKeyRequest = (GetApiKeyRequest) request;
182-
if (authentication.getAuthenticatedBy().getType().equals(ApiKeyService.API_KEY_REALM_TYPE)) {
183+
if (AuthenticationType.API_KEY == authentication.getAuthenticationType()) {
183184
assert authentication.getLookedUpBy() == null : "runAs not supported for api key authentication";
184185
// if authenticated by API key then the request must also contain same API key id
185186
String authenticatedApiKeyId = (String) authentication.getMetadata().get(ApiKeyService.API_KEY_ID_KEY);
@@ -542,8 +543,8 @@ private static boolean checkChangePasswordAction(Authentication authentication)
542543
// Ensure that the user is not authenticated with an access token or an API key.
543544
// Also ensure that the user was authenticated by a realm that we can change a password for. The native realm is an internal realm
544545
// and right now only one can exist in the realm configuration - if this changes we should update this check
545-
final Authentication.AuthenticationType authType = authentication.getAuthenticationType();
546-
return (authType.equals(Authentication.AuthenticationType.REALM)
546+
final AuthenticationType authType = authentication.getAuthenticationType();
547+
return (authType.equals(AuthenticationType.REALM)
547548
&& (ReservedRealm.TYPE.equals(realmType) || NativeRealmSettings.TYPE.equals(realmType)));
548549
}
549550

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

+3-2
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import org.elasticsearch.xpack.core.security.action.user.PutUserAction;
3737
import org.elasticsearch.xpack.core.security.action.user.UserRequest;
3838
import org.elasticsearch.xpack.core.security.authc.Authentication;
39+
import org.elasticsearch.xpack.core.security.authc.Authentication.AuthenticationType;
3940
import org.elasticsearch.xpack.core.security.authc.esnative.NativeRealmSettings;
4041
import org.elasticsearch.xpack.core.security.authc.file.FileRealmSettings;
4142
import org.elasticsearch.xpack.core.security.authc.ldap.LdapRealmSettings;
@@ -290,7 +291,7 @@ public void testSameUserPermissionAllowsSelfApiKeyInfoRetrievalWhenAuthenticated
290291
final Authentication.RealmRef authenticatedBy = mock(Authentication.RealmRef.class);
291292
when(authentication.getUser()).thenReturn(user);
292293
when(authentication.getAuthenticatedBy()).thenReturn(authenticatedBy);
293-
when(authenticatedBy.getType()).thenReturn(ApiKeyService.API_KEY_REALM_TYPE);
294+
when(authentication.getAuthenticationType()).thenReturn(AuthenticationType.API_KEY);
294295
when(authentication.getMetadata()).thenReturn(Collections.singletonMap(ApiKeyService.API_KEY_ID_KEY, apiKeyId));
295296

296297
assertTrue(engine.checkSameUserPermissions(GetApiKeyAction.NAME, request, authentication));
@@ -321,7 +322,7 @@ public void testSameUserPermissionDeniesApiKeyInfoRetrievalWhenLookedupByIsPrese
321322
when(authentication.getUser()).thenReturn(user);
322323
when(authentication.getAuthenticatedBy()).thenReturn(authenticatedBy);
323324
when(authentication.getLookedUpBy()).thenReturn(lookedupBy);
324-
when(authenticatedBy.getType()).thenReturn(ApiKeyService.API_KEY_REALM_TYPE);
325+
when(authentication.getAuthenticationType()).thenReturn(AuthenticationType.API_KEY);
325326
when(authentication.getMetadata()).thenReturn(
326327
Collections.singletonMap(ApiKeyService.API_KEY_ID_KEY, randomAlphaOfLengthBetween(4, 7)));
327328

0 commit comments

Comments
 (0)