From 36a9bca99e1407b8ea23a2dcce48f44b09af5294 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Mon, 29 Jul 2019 14:36:05 +1000 Subject: [PATCH 1/5] Refactor API key service This commit merely refactors API key service interface for retrieving and invalidating API keys. This avoids the use of `*Response` classes to service layer. The service layer need not do any authorization so we do not need multiple ways to retrieve or invalidate API keys but one interface to do each operation. --- .../action/TransportGetApiKeyAction.java | 13 +- .../TransportInvalidateApiKeyAction.java | 12 +- .../xpack/security/authc/ApiKeyService.java | 265 +++++++----------- 3 files changed, 110 insertions(+), 180 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportGetApiKeyAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportGetApiKeyAction.java index 403ce482805a2..63918d7d28795 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportGetApiKeyAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportGetApiKeyAction.java @@ -32,15 +32,10 @@ public TransportGetApiKeyAction(TransportService transportService, ActionFilters @Override protected void doExecute(Task task, GetApiKeyRequest request, ActionListener listener) { - if (Strings.hasText(request.getRealmName()) || Strings.hasText(request.getUserName())) { - apiKeyService.getApiKeysForRealmAndUser(request.getRealmName(), request.getUserName(), listener); - } else if (Strings.hasText(request.getApiKeyId())) { - apiKeyService.getApiKeyForApiKeyId(request.getApiKeyId(), listener); - } else if (Strings.hasText(request.getApiKeyName())) { - apiKeyService.getApiKeyForApiKeyName(request.getApiKeyName(), listener); - } else { - listener.onFailure(new IllegalArgumentException("One of [api key id, api key name, username, realm name] must be specified")); - } + apiKeyService.getApiKeys(request.getRealmName(), request.getUserName(), request.getApiKeyName(), request.getApiKeyId(), + ActionListener.wrap(getApiKeysResult -> { + listener.onResponse(new GetApiKeyResponse(getApiKeysResult.getApiKeyInfos())); + }, listener::onFailure)); } } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportInvalidateApiKeyAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportInvalidateApiKeyAction.java index 886d15b1f257d..198ac0427f41b 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportInvalidateApiKeyAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportInvalidateApiKeyAction.java @@ -32,13 +32,11 @@ public TransportInvalidateApiKeyAction(TransportService transportService, Action @Override protected void doExecute(Task task, InvalidateApiKeyRequest request, ActionListener listener) { - if (Strings.hasText(request.getRealmName()) || Strings.hasText(request.getUserName())) { - apiKeyService.invalidateApiKeysForRealmAndUser(request.getRealmName(), request.getUserName(), listener); - } else if (Strings.hasText(request.getId())) { - apiKeyService.invalidateApiKeyForApiKeyId(request.getId(), listener); - } else { - apiKeyService.invalidateApiKeyForApiKeyName(request.getName(), listener); - } + apiKeyService.invalidateApiKeys(request.getRealmName(), request.getUserName(), request.getName(), request.getId(), + ActionListener.wrap(invalidateApiKeysResult -> { + listener.onResponse(new InvalidateApiKeyResponse(invalidateApiKeysResult.getInvalidatedApiKeys(), + invalidateApiKeysResult.getPreviouslyInvalidatedApiKeys(), invalidateApiKeysResult.getErrors())); + }, listener::onFailure)); } } 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 index ee1a15bb8535e..d2856f82836a1 100644 --- 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 @@ -62,7 +62,6 @@ import org.elasticsearch.xpack.core.security.action.ApiKey; import org.elasticsearch.xpack.core.security.action.CreateApiKeyRequest; import org.elasticsearch.xpack.core.security.action.CreateApiKeyResponse; -import org.elasticsearch.xpack.core.security.action.GetApiKeyResponse; import org.elasticsearch.xpack.core.security.action.InvalidateApiKeyResponse; import org.elasticsearch.xpack.core.security.authc.Authentication; import org.elasticsearch.xpack.core.security.authc.AuthenticationResult; @@ -71,7 +70,6 @@ import org.elasticsearch.xpack.core.security.user.User; import org.elasticsearch.xpack.security.support.SecurityIndexManager; -import javax.crypto.SecretKeyFactory; import java.io.Closeable; import java.io.IOException; import java.io.UncheckedIOException; @@ -95,6 +93,8 @@ import java.util.function.Function; import java.util.stream.Collectors; +import javax.crypto.SecretKeyFactory; + import static org.elasticsearch.index.mapper.MapperService.SINGLE_MAPPING_NAME; import static org.elasticsearch.search.SearchService.DEFAULT_KEEPALIVE_SETTING; import static org.elasticsearch.xpack.core.ClientHelper.SECURITY_ORIGIN; @@ -105,7 +105,7 @@ public class ApiKeyService { private static final Logger logger = LogManager.getLogger(ApiKeyService.class); private static final DeprecationLogger deprecationLogger = new DeprecationLogger(logger); - static final String API_KEY_ID_KEY = "_security_api_key_id"; + public static final String API_KEY_ID_KEY = "_security_api_key_id"; static final String API_KEY_ROLE_DESCRIPTORS_KEY = "_security_api_key_role_descriptors"; static final String API_KEY_LIMITED_ROLE_DESCRIPTORS_KEY = "_security_api_key_limited_by_role_descriptors"; @@ -640,96 +640,38 @@ public void usedDeprecatedField(String usedName, String replacedWith) { } /** - * Invalidate API keys for given realm and user name. + * Invalidate API keys for given realm, user name, API key name and id. * @param realmName realm name - * @param userName user name - * @param invalidateListener listener for {@link InvalidateApiKeyResponse} - */ - public void invalidateApiKeysForRealmAndUser(String realmName, String userName, - ActionListener invalidateListener) { - ensureEnabled(); - if (Strings.hasText(realmName) == false && Strings.hasText(userName) == false) { - logger.trace("No realm name or username provided"); - invalidateListener.onFailure(new IllegalArgumentException("realm name or username must be provided")); - } else { - findApiKeysForUserAndRealm(userName, realmName, true, false, ActionListener.wrap(apiKeyIds -> { - if (apiKeyIds.isEmpty()) { - logger.warn("No active api keys to invalidate for realm [{}] and username [{}]", realmName, userName); - invalidateListener.onResponse(InvalidateApiKeyResponse.emptyResponse()); - } else { - invalidateAllApiKeys(apiKeyIds.stream().map(apiKey -> apiKey.getId()).collect(Collectors.toSet()), invalidateListener); - } - }, invalidateListener::onFailure)); - } - } - - private void invalidateAllApiKeys(Collection apiKeyIds, ActionListener invalidateListener) { - indexInvalidation(apiKeyIds, invalidateListener, null); - } - - /** - * Invalidate API key for given API key id - * @param apiKeyId API key id - * @param invalidateListener listener for {@link InvalidateApiKeyResponse} - */ - public void invalidateApiKeyForApiKeyId(String apiKeyId, ActionListener invalidateListener) { - ensureEnabled(); - if (Strings.hasText(apiKeyId) == false) { - logger.trace("No api key id provided"); - invalidateListener.onFailure(new IllegalArgumentException("api key id must be provided")); - } else { - findApiKeysForApiKeyId(apiKeyId, true, false, ActionListener.wrap(apiKeyIds -> { - if (apiKeyIds.isEmpty()) { - logger.warn("No api key to invalidate for api key id [{}]", apiKeyId); - invalidateListener.onResponse(InvalidateApiKeyResponse.emptyResponse()); - } else { - invalidateAllApiKeys(apiKeyIds.stream().map(apiKey -> apiKey.getId()).collect(Collectors.toSet()), invalidateListener); - } - }, invalidateListener::onFailure)); - } - } - - /** - * Invalidate API key for given API key name + * @param username user name * @param apiKeyName API key name - * @param invalidateListener listener for {@link InvalidateApiKeyResponse} + * @param apiKeyId API key id + * @param invalidateListener listener for {@link InvalidateApiKeysResult} */ - public void invalidateApiKeyForApiKeyName(String apiKeyName, ActionListener invalidateListener) { + public void invalidateApiKeys(String realmName, String username, String apiKeyName, String apiKeyId, + ActionListener invalidateListener) { ensureEnabled(); - if (Strings.hasText(apiKeyName) == false) { - logger.trace("No api key name provided"); - invalidateListener.onFailure(new IllegalArgumentException("api key name must be provided")); + if (Strings.hasText(realmName) == false && Strings.hasText(username) == false && Strings.hasText(apiKeyName) == false + && Strings.hasText(apiKeyId) == false) { + invalidateListener + .onFailure(new IllegalArgumentException("One of [api key id, api key name, username, realm name] must be specified")); } else { - findApiKeyForApiKeyName(apiKeyName, true, false, ActionListener.wrap(apiKeyIds -> { - if (apiKeyIds.isEmpty()) { - logger.warn("No api key to invalidate for api key name [{}]", apiKeyName); - invalidateListener.onResponse(InvalidateApiKeyResponse.emptyResponse()); - } else { - invalidateAllApiKeys(apiKeyIds.stream().map(apiKey -> apiKey.getId()).collect(Collectors.toSet()), invalidateListener); - } - }, invalidateListener::onFailure)); + findApiKeysForUserRealmApiKeyIdAndNameCombination(realmName, username, apiKeyName, apiKeyId, true, false, + ActionListener.wrap(apiKeyIds -> { + if (apiKeyIds.isEmpty()) { + logger.warn( + "No active api keys to invalidate for realm [{}], username [{}], api key name [{}] and api key id [{}]", + realmName, username, apiKeyName, apiKeyId); + invalidateListener.onResponse(InvalidateApiKeysResult.EMPTY_RESULT); + } else { + invalidateAllApiKeys(apiKeyIds.stream().map(apiKey -> apiKey.getId()).collect(Collectors.toSet()), + invalidateListener); + } + }, invalidateListener::onFailure)); } } - private void findApiKeysForUserAndRealm(String userName, String realmName, boolean filterOutInvalidatedKeys, - boolean filterOutExpiredKeys, ActionListener> listener) { - final SecurityIndexManager frozenSecurityIndex = securityIndex.freeze(); - if (frozenSecurityIndex.indexExists() == false) { - listener.onResponse(Collections.emptyList()); - } else if (frozenSecurityIndex.isAvailable() == false) { - listener.onFailure(frozenSecurityIndex.getUnavailableReason()); - } else { - final BoolQueryBuilder boolQuery = QueryBuilders.boolQuery() - .filter(QueryBuilders.termQuery("doc_type", "api_key")); - if (Strings.hasText(userName)) { - boolQuery.filter(QueryBuilders.termQuery("creator.principal", userName)); - } - if (Strings.hasText(realmName)) { - boolQuery.filter(QueryBuilders.termQuery("creator.realm", realmName)); - } - - findApiKeys(boolQuery, filterOutInvalidatedKeys, filterOutExpiredKeys, listener); - } + private void invalidateAllApiKeys(Collection apiKeyIds, ActionListener invalidateListener) { + indexInvalidation(apiKeyIds, invalidateListener, null); } private void findApiKeys(final BoolQueryBuilder boolQuery, boolean filterOutInvalidatedKeys, boolean filterOutExpiredKeys, @@ -768,35 +710,28 @@ private void findApiKeys(final BoolQueryBuilder boolQuery, boolean filterOutInva } } - private void findApiKeyForApiKeyName(String apiKeyName, boolean filterOutInvalidatedKeys, boolean filterOutExpiredKeys, - ActionListener> listener) { + private void findApiKeysForUserRealmApiKeyIdAndNameCombination(String realmName, String userName, String apiKeyName, String apiKeyId, + boolean filterOutInvalidatedKeys, boolean filterOutExpiredKeys, + ActionListener> listener) { final SecurityIndexManager frozenSecurityIndex = securityIndex.freeze(); if (frozenSecurityIndex.indexExists() == false) { listener.onResponse(Collections.emptyList()); } else if (frozenSecurityIndex.isAvailable() == false) { listener.onFailure(frozenSecurityIndex.getUnavailableReason()); } else { - final BoolQueryBuilder boolQuery = QueryBuilders.boolQuery() - .filter(QueryBuilders.termQuery("doc_type", "api_key")); + final BoolQueryBuilder boolQuery = QueryBuilders.boolQuery().filter(QueryBuilders.termQuery("doc_type", "api_key")); + if (Strings.hasText(realmName)) { + boolQuery.filter(QueryBuilders.termQuery("creator.realm", realmName)); + } + if (Strings.hasText(userName)) { + boolQuery.filter(QueryBuilders.termQuery("creator.principal", userName)); + } if (Strings.hasText(apiKeyName)) { boolQuery.filter(QueryBuilders.termQuery("name", apiKeyName)); } - - findApiKeys(boolQuery, filterOutInvalidatedKeys, filterOutExpiredKeys, listener); - } - } - - private void findApiKeysForApiKeyId(String apiKeyId, boolean filterOutInvalidatedKeys, boolean filterOutExpiredKeys, - ActionListener> listener) { - final SecurityIndexManager frozenSecurityIndex = securityIndex.freeze(); - if (frozenSecurityIndex.indexExists() == false) { - listener.onResponse(Collections.emptyList()); - } else if (frozenSecurityIndex.isAvailable() == false) { - listener.onFailure(frozenSecurityIndex.getUnavailableReason()); - } else { - final BoolQueryBuilder boolQuery = QueryBuilders.boolQuery() - .filter(QueryBuilders.termQuery("doc_type", "api_key")) - .filter(QueryBuilders.termQuery("_id", apiKeyId)); + if (Strings.hasText(apiKeyId)) { + boolQuery.filter(QueryBuilders.termQuery("_id", apiKeyId)); + } findApiKeys(boolQuery, filterOutInvalidatedKeys, filterOutExpiredKeys, listener); } @@ -810,7 +745,7 @@ private void findApiKeysForApiKeyId(String apiKeyId, boolean filterOutInvalidate * @param previousResult if this not the initial attempt for invalidation, it contains the result of invalidating * api keys up to the point of the retry. This result is added to the result of the current attempt */ - private void indexInvalidation(Collection apiKeyIds, ActionListener listener, + private void indexInvalidation(Collection apiKeyIds, ActionListener listener, @Nullable InvalidateApiKeyResponse previousResult) { maybeStartApiKeyRemover(); if (apiKeyIds.isEmpty()) { @@ -819,9 +754,9 @@ private void indexInvalidation(Collection apiKeyIds, ActionListener apiKeyIds, ActionListener { @@ -925,67 +860,28 @@ private void maybeStartApiKeyRemover() { } /** - * Get API keys for given realm and user name. + * Get API key information for given realm, user, API key name and id combination * @param realmName realm name - * @param userName user name - * @param listener listener for {@link GetApiKeyResponse} - */ - public void getApiKeysForRealmAndUser(String realmName, String userName, ActionListener listener) { - ensureEnabled(); - if (Strings.hasText(realmName) == false && Strings.hasText(userName) == false) { - logger.trace("No realm name or username provided"); - listener.onFailure(new IllegalArgumentException("realm name or username must be provided")); - } else { - findApiKeysForUserAndRealm(userName, realmName, false, false, ActionListener.wrap(apiKeyInfos -> { - if (apiKeyInfos.isEmpty()) { - logger.warn("No active api keys found for realm [{}] and username [{}]", realmName, userName); - listener.onResponse(GetApiKeyResponse.emptyResponse()); - } else { - listener.onResponse(new GetApiKeyResponse(apiKeyInfos)); - } - }, listener::onFailure)); - } - } - - /** - * Get API key for given API key id - * @param apiKeyId API key id - * @param listener listener for {@link GetApiKeyResponse} - */ - public void getApiKeyForApiKeyId(String apiKeyId, ActionListener listener) { - ensureEnabled(); - if (Strings.hasText(apiKeyId) == false) { - logger.trace("No api key id provided"); - listener.onFailure(new IllegalArgumentException("api key id must be provided")); - } else { - findApiKeysForApiKeyId(apiKeyId, false, false, ActionListener.wrap(apiKeyInfos -> { - if (apiKeyInfos.isEmpty()) { - logger.warn("No api key found for api key id [{}]", apiKeyId); - listener.onResponse(GetApiKeyResponse.emptyResponse()); - } else { - listener.onResponse(new GetApiKeyResponse(apiKeyInfos)); - } - }, listener::onFailure)); - } - } - - /** - * Get API key for given API key name + * @param username user name * @param apiKeyName API key name - * @param listener listener for {@link GetApiKeyResponse} + * @param apiKeyId API key id + * @param listener listener for {@link GetApiKeysResult} */ - public void getApiKeyForApiKeyName(String apiKeyName, ActionListener listener) { + public void getApiKeys(String realmName, String username, String apiKeyName, String apiKeyId, + ActionListener listener) { ensureEnabled(); - if (Strings.hasText(apiKeyName) == false) { - logger.trace("No api key name provided"); - listener.onFailure(new IllegalArgumentException("api key name must be provided")); + if (Strings.hasText(realmName) == false && Strings.hasText(username) == false && Strings.hasText(apiKeyName) == false + && Strings.hasText(apiKeyId) == false) { + listener.onFailure(new IllegalArgumentException("One of [api key id, api key name, username, realm name] must be specified")); } else { - findApiKeyForApiKeyName(apiKeyName, false, false, ActionListener.wrap(apiKeyInfos -> { + findApiKeysForUserRealmApiKeyIdAndNameCombination(realmName, username, apiKeyName, apiKeyId, false, false, + ActionListener.wrap(apiKeyInfos -> { if (apiKeyInfos.isEmpty()) { - logger.warn("No api key found for api key name [{}]", apiKeyName); - listener.onResponse(GetApiKeyResponse.emptyResponse()); + logger.warn("No active api keys found for realm [{}], user [{}], api key name [{}] and api key id [{}]", + realmName, username, apiKeyName, apiKeyId); + listener.onResponse(GetApiKeysResult.EMPTY_RESULT); } else { - listener.onResponse(new GetApiKeyResponse(apiKeyInfos)); + listener.onResponse(new GetApiKeysResult(apiKeyInfos)); } }, listener::onFailure)); } @@ -1004,4 +900,45 @@ private boolean verify(SecureString password) { return hash != null && cacheHasher.verify(password, hash); } } + + public static final class GetApiKeysResult { + static final GetApiKeysResult EMPTY_RESULT = new GetApiKeysResult(Collections.emptyList()); + private final Collection foundApiKeysInfo; + + public GetApiKeysResult(Collection foundApiKeysInfo) { + this.foundApiKeysInfo = foundApiKeysInfo; + } + + public Collection getApiKeyInfos() { + return foundApiKeysInfo; + } + } + + public static final class InvalidateApiKeysResult { + static final InvalidateApiKeysResult EMPTY_RESULT = new InvalidateApiKeysResult(Collections.emptyList(), Collections.emptyList(), + null); + private final List invalidatedApiKeys; + private final List previouslyInvalidatedApiKeys; + private final List errors; + + public InvalidateApiKeysResult(List invalidatedApiKeys, List previouslyInvalidatedApiKeys, + List errors) { + this.invalidatedApiKeys = (invalidatedApiKeys == null) ? List.of() : List.copyOf(invalidatedApiKeys); + this.previouslyInvalidatedApiKeys = (previouslyInvalidatedApiKeys == null) ? List.of() + : List.copyOf(previouslyInvalidatedApiKeys); + this.errors = (errors == null) ? List.of() : List.copyOf(errors); + } + + public List getInvalidatedApiKeys() { + return invalidatedApiKeys; + } + + public List getPreviouslyInvalidatedApiKeys() { + return previouslyInvalidatedApiKeys; + } + + public List getErrors() { + return errors; + } + } } From 39f0a041c31a0c415af997ceb8c4b266c2b57110 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Mon, 29 Jul 2019 15:32:00 +1000 Subject: [PATCH 2/5] fix precommit errors --- .../xpack/security/action/TransportGetApiKeyAction.java | 1 - .../xpack/security/action/TransportInvalidateApiKeyAction.java | 1 - 2 files changed, 2 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportGetApiKeyAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportGetApiKeyAction.java index 63918d7d28795..869c088897c2b 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportGetApiKeyAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportGetApiKeyAction.java @@ -9,7 +9,6 @@ import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.action.support.HandledTransportAction; -import org.elasticsearch.common.Strings; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.tasks.Task; diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportInvalidateApiKeyAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportInvalidateApiKeyAction.java index 198ac0427f41b..37b794c45f54d 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportInvalidateApiKeyAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportInvalidateApiKeyAction.java @@ -9,7 +9,6 @@ import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.action.support.HandledTransportAction; -import org.elasticsearch.common.Strings; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.tasks.Task; From 298b139e221a0db000e9c72ac343bd7b5b4ff3b8 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Tue, 30 Jul 2019 23:46:49 +1000 Subject: [PATCH 3/5] address review comments --- .../xpack/security/authc/ApiKeyService.java | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) 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 index d2856f82836a1..0dd6838e7879f 100644 --- 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 @@ -658,7 +658,7 @@ public void invalidateApiKeys(String realmName, String username, String apiKeyNa findApiKeysForUserRealmApiKeyIdAndNameCombination(realmName, username, apiKeyName, apiKeyId, true, false, ActionListener.wrap(apiKeyIds -> { if (apiKeyIds.isEmpty()) { - logger.warn( + logger.debug( "No active api keys to invalidate for realm [{}], username [{}], api key name [{}] and api key id [{}]", realmName, username, apiKeyName, apiKeyId); invalidateListener.onResponse(InvalidateApiKeysResult.EMPTY_RESULT); @@ -877,7 +877,7 @@ public void getApiKeys(String realmName, String username, String apiKeyName, Str findApiKeysForUserRealmApiKeyIdAndNameCombination(realmName, username, apiKeyName, apiKeyId, false, false, ActionListener.wrap(apiKeyInfos -> { if (apiKeyInfos.isEmpty()) { - logger.warn("No active api keys found for realm [{}], user [{}], api key name [{}] and api key id [{}]", + logger.debug("No active api keys found for realm [{}], user [{}], api key name [{}] and api key id [{}]", realmName, username, apiKeyName, apiKeyId); listener.onResponse(GetApiKeysResult.EMPTY_RESULT); } else { @@ -902,11 +902,11 @@ private boolean verify(SecureString password) { } public static final class GetApiKeysResult { - static final GetApiKeysResult EMPTY_RESULT = new GetApiKeysResult(Collections.emptyList()); + static final GetApiKeysResult EMPTY_RESULT = new GetApiKeysResult(null); private final Collection foundApiKeysInfo; public GetApiKeysResult(Collection foundApiKeysInfo) { - this.foundApiKeysInfo = foundApiKeysInfo; + this.foundApiKeysInfo = (foundApiKeysInfo == null) ? List.of() : foundApiKeysInfo; } public Collection getApiKeyInfos() { @@ -915,8 +915,7 @@ public Collection getApiKeyInfos() { } public static final class InvalidateApiKeysResult { - static final InvalidateApiKeysResult EMPTY_RESULT = new InvalidateApiKeysResult(Collections.emptyList(), Collections.emptyList(), - null); + static final InvalidateApiKeysResult EMPTY_RESULT = new InvalidateApiKeysResult(null, null, null); private final List invalidatedApiKeys; private final List previouslyInvalidatedApiKeys; private final List errors; From fd3f7f0bc57eec422df305535dd7638604d4a403 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Thu, 1 Aug 2019 15:20:12 +1000 Subject: [PATCH 4/5] remove intermediate classes since its okay to use *Response at service layer --- .../action/TransportGetApiKeyAction.java | 5 +- .../TransportInvalidateApiKeyAction.java | 6 +- .../xpack/security/authc/ApiKeyService.java | 61 ++++--------------- 3 files changed, 13 insertions(+), 59 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportGetApiKeyAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportGetApiKeyAction.java index 869c088897c2b..606f23b0fd190 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportGetApiKeyAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportGetApiKeyAction.java @@ -31,10 +31,7 @@ public TransportGetApiKeyAction(TransportService transportService, ActionFilters @Override protected void doExecute(Task task, GetApiKeyRequest request, ActionListener listener) { - apiKeyService.getApiKeys(request.getRealmName(), request.getUserName(), request.getApiKeyName(), request.getApiKeyId(), - ActionListener.wrap(getApiKeysResult -> { - listener.onResponse(new GetApiKeyResponse(getApiKeysResult.getApiKeyInfos())); - }, listener::onFailure)); + apiKeyService.getApiKeys(request.getRealmName(), request.getUserName(), request.getApiKeyName(), request.getApiKeyId(), listener); } } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportInvalidateApiKeyAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportInvalidateApiKeyAction.java index 37b794c45f54d..9b552982fca9e 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportInvalidateApiKeyAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportInvalidateApiKeyAction.java @@ -31,11 +31,7 @@ public TransportInvalidateApiKeyAction(TransportService transportService, Action @Override protected void doExecute(Task task, InvalidateApiKeyRequest request, ActionListener listener) { - apiKeyService.invalidateApiKeys(request.getRealmName(), request.getUserName(), request.getName(), request.getId(), - ActionListener.wrap(invalidateApiKeysResult -> { - listener.onResponse(new InvalidateApiKeyResponse(invalidateApiKeysResult.getInvalidatedApiKeys(), - invalidateApiKeysResult.getPreviouslyInvalidatedApiKeys(), invalidateApiKeysResult.getErrors())); - }, listener::onFailure)); + apiKeyService.invalidateApiKeys(request.getRealmName(), request.getUserName(), request.getName(), request.getId(), listener); } } 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 index 0dd6838e7879f..7570a726f0b18 100644 --- 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 @@ -62,6 +62,7 @@ import org.elasticsearch.xpack.core.security.action.ApiKey; import org.elasticsearch.xpack.core.security.action.CreateApiKeyRequest; import org.elasticsearch.xpack.core.security.action.CreateApiKeyResponse; +import org.elasticsearch.xpack.core.security.action.GetApiKeyResponse; import org.elasticsearch.xpack.core.security.action.InvalidateApiKeyResponse; import org.elasticsearch.xpack.core.security.authc.Authentication; import org.elasticsearch.xpack.core.security.authc.AuthenticationResult; @@ -645,10 +646,10 @@ public void usedDeprecatedField(String usedName, String replacedWith) { * @param username user name * @param apiKeyName API key name * @param apiKeyId API key id - * @param invalidateListener listener for {@link InvalidateApiKeysResult} + * @param invalidateListener listener for {@link InvalidateApiKeyResponse} */ public void invalidateApiKeys(String realmName, String username, String apiKeyName, String apiKeyId, - ActionListener invalidateListener) { + ActionListener invalidateListener) { ensureEnabled(); if (Strings.hasText(realmName) == false && Strings.hasText(username) == false && Strings.hasText(apiKeyName) == false && Strings.hasText(apiKeyId) == false) { @@ -661,7 +662,7 @@ public void invalidateApiKeys(String realmName, String username, String apiKeyNa logger.debug( "No active api keys to invalidate for realm [{}], username [{}], api key name [{}] and api key id [{}]", realmName, username, apiKeyName, apiKeyId); - invalidateListener.onResponse(InvalidateApiKeysResult.EMPTY_RESULT); + invalidateListener.onResponse(InvalidateApiKeyResponse.emptyResponse()); } else { invalidateAllApiKeys(apiKeyIds.stream().map(apiKey -> apiKey.getId()).collect(Collectors.toSet()), invalidateListener); @@ -670,7 +671,7 @@ public void invalidateApiKeys(String realmName, String username, String apiKeyNa } } - private void invalidateAllApiKeys(Collection apiKeyIds, ActionListener invalidateListener) { + private void invalidateAllApiKeys(Collection apiKeyIds, ActionListener invalidateListener) { indexInvalidation(apiKeyIds, invalidateListener, null); } @@ -745,7 +746,7 @@ private void findApiKeysForUserRealmApiKeyIdAndNameCombination(String realmName, * @param previousResult if this not the initial attempt for invalidation, it contains the result of invalidating * api keys up to the point of the retry. This result is added to the result of the current attempt */ - private void indexInvalidation(Collection apiKeyIds, ActionListener listener, + private void indexInvalidation(Collection apiKeyIds, ActionListener listener, @Nullable InvalidateApiKeyResponse previousResult) { maybeStartApiKeyRemover(); if (apiKeyIds.isEmpty()) { @@ -787,7 +788,7 @@ private void indexInvalidation(Collection apiKeyIds, ActionListener { @@ -865,10 +866,10 @@ private void maybeStartApiKeyRemover() { * @param username user name * @param apiKeyName API key name * @param apiKeyId API key id - * @param listener listener for {@link GetApiKeysResult} + * @param listener listener for {@link GetApiKeyResponse} */ public void getApiKeys(String realmName, String username, String apiKeyName, String apiKeyId, - ActionListener listener) { + ActionListener listener) { ensureEnabled(); if (Strings.hasText(realmName) == false && Strings.hasText(username) == false && Strings.hasText(apiKeyName) == false && Strings.hasText(apiKeyId) == false) { @@ -879,9 +880,9 @@ public void getApiKeys(String realmName, String username, String apiKeyName, Str if (apiKeyInfos.isEmpty()) { logger.debug("No active api keys found for realm [{}], user [{}], api key name [{}] and api key id [{}]", realmName, username, apiKeyName, apiKeyId); - listener.onResponse(GetApiKeysResult.EMPTY_RESULT); + listener.onResponse(GetApiKeyResponse.emptyResponse()); } else { - listener.onResponse(new GetApiKeysResult(apiKeyInfos)); + listener.onResponse(new GetApiKeyResponse(apiKeyInfos)); } }, listener::onFailure)); } @@ -900,44 +901,4 @@ private boolean verify(SecureString password) { return hash != null && cacheHasher.verify(password, hash); } } - - public static final class GetApiKeysResult { - static final GetApiKeysResult EMPTY_RESULT = new GetApiKeysResult(null); - private final Collection foundApiKeysInfo; - - public GetApiKeysResult(Collection foundApiKeysInfo) { - this.foundApiKeysInfo = (foundApiKeysInfo == null) ? List.of() : foundApiKeysInfo; - } - - public Collection getApiKeyInfos() { - return foundApiKeysInfo; - } - } - - public static final class InvalidateApiKeysResult { - static final InvalidateApiKeysResult EMPTY_RESULT = new InvalidateApiKeysResult(null, null, null); - private final List invalidatedApiKeys; - private final List previouslyInvalidatedApiKeys; - private final List errors; - - public InvalidateApiKeysResult(List invalidatedApiKeys, List previouslyInvalidatedApiKeys, - List errors) { - this.invalidatedApiKeys = (invalidatedApiKeys == null) ? List.of() : List.copyOf(invalidatedApiKeys); - this.previouslyInvalidatedApiKeys = (previouslyInvalidatedApiKeys == null) ? List.of() - : List.copyOf(previouslyInvalidatedApiKeys); - this.errors = (errors == null) ? List.of() : List.copyOf(errors); - } - - public List getInvalidatedApiKeys() { - return invalidatedApiKeys; - } - - public List getPreviouslyInvalidatedApiKeys() { - return previouslyInvalidatedApiKeys; - } - - public List getErrors() { - return errors; - } - } } From 70240b65e3707ac67a099548757d6707121e0cba Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Tue, 6 Aug 2019 19:25:28 +1000 Subject: [PATCH 5/5] address review comments --- .../elasticsearch/xpack/security/authc/ApiKeyService.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) 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 index 7570a726f0b18..b3859ad27a1fd 100644 --- 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 @@ -653,18 +653,19 @@ public void invalidateApiKeys(String realmName, String username, String apiKeyNa ensureEnabled(); if (Strings.hasText(realmName) == false && Strings.hasText(username) == false && Strings.hasText(apiKeyName) == false && Strings.hasText(apiKeyId) == false) { + logger.trace("none of the parameters [api key id, api key name, username, realm name] were specified for invalidation"); invalidateListener .onFailure(new IllegalArgumentException("One of [api key id, api key name, username, realm name] must be specified")); } else { findApiKeysForUserRealmApiKeyIdAndNameCombination(realmName, username, apiKeyName, apiKeyId, true, false, - ActionListener.wrap(apiKeyIds -> { - if (apiKeyIds.isEmpty()) { + ActionListener.wrap(apiKeys -> { + if (apiKeys.isEmpty()) { logger.debug( "No active api keys to invalidate for realm [{}], username [{}], api key name [{}] and api key id [{}]", realmName, username, apiKeyName, apiKeyId); invalidateListener.onResponse(InvalidateApiKeyResponse.emptyResponse()); } else { - invalidateAllApiKeys(apiKeyIds.stream().map(apiKey -> apiKey.getId()).collect(Collectors.toSet()), + invalidateAllApiKeys(apiKeys.stream().map(apiKey -> apiKey.getId()).collect(Collectors.toSet()), invalidateListener); } }, invalidateListener::onFailure)); @@ -873,6 +874,7 @@ public void getApiKeys(String realmName, String username, String apiKeyName, Str ensureEnabled(); if (Strings.hasText(realmName) == false && Strings.hasText(username) == false && Strings.hasText(apiKeyName) == false && Strings.hasText(apiKeyId) == false) { + logger.trace("none of the parameters [api key id, api key name, username, realm name] were specified for retrieval"); listener.onFailure(new IllegalArgumentException("One of [api key id, api key name, username, realm name] must be specified")); } else { findApiKeysForUserRealmApiKeyIdAndNameCombination(realmName, username, apiKeyName, apiKeyId, false, false,