-
Notifications
You must be signed in to change notification settings - Fork 24.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simplify API key service API #44935
Simplify API key service API #44935
Conversation
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.
Pinging @elastic/es-security |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor nits, otherwise LGTM.
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java
Outdated
Show resolved
Hide resolved
static final GetApiKeysResult EMPTY_RESULT = new GetApiKeysResult(Collections.emptyList()); | ||
private final Collection<ApiKey> foundApiKeysInfo; | ||
|
||
public GetApiKeysResult(Collection<ApiKey> foundApiKeysInfo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe make this constructor look more like InvalidateApiKeysResult
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ScrollHelper.fetchAllByEntity
returns a Collection<T>
and so this is a Collection than a List, avoiding the conversion here.
I don't think there's any need to create |
*Response
in service layerThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with a couple of nits.
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java
Show resolved
Hide resolved
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java
Outdated
Show resolved
Hide resolved
@elasticmachine test this please |
This commit merely refactors API key service interface
for retrieving and invalidating API keys.
The service layer need not do any authorization so we do not need
multiple interfaces to retrieve or invalidate API keys but one interface
to do each operation.
Relates #40031