-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Service Accounts - Get service account API #71315
Conversation
Pinging @elastic/es-security (Team:Security) |
|
||
public class GetServiceAccountResponse extends ActionResponse implements ToXContentObject { | ||
|
||
private final Map<String, RoleDescriptor> serviceAccounts; |
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.
I think this will be easier to expand in the future if we introduce a ServiceAccountInfo
object here.
If we do, then the serialization code here stays mostly the same, even as the response evolves.
However, with the code as it is, we would have to introduce some ugly version compat code.
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.
Added ServiceAccountInfo
as suggested.
...n/java/org/elasticsearch/xpack/security/action/service/TransportGetServiceAccountAction.java
Outdated
Show resolved
Hide resolved
@Override | ||
protected RestChannelConsumer innerPrepareRequest(RestRequest request, NodeClient client) throws IOException { | ||
final String namespace = request.param("namespace"); | ||
final String serviceName = request.param("service"); |
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.
Should we support *
here?
I don't think we need to, but I wonder if we should for consistency with other APIs.
Although, that then opens the question of comma separated lists, etc.
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.
I thought about it and decided to keep it simple for now. Also wildcard (*
) is not supported for APIs like GetRole or GetUser. The main reason is likely because their names allow the literal *
character. Neither service account namespace nor service-name allow it, so technically it can support *
. But I decided not to for somewhat consistency and simplicity.
The comma separated list is more widely supported. It can also be supported here as well. But again I decided for simplicity (both namespace
and service
can support comma separated list, the natural relation is likely combination of the two sets, but we haven't really talked about it).
Given we only have a single service account, I think it is viable to have this simplest form first. It is easy to expand once the need arises.
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.
PS: GetUser and GetRole has a minor bug when it comes to comma separated lists. Because they both allow comma to be used in the name, this means if we created an user a,b
, we cannot get it back by itself anymore. The API will interpret GET _security/user/a,b
as retrieving user a
and b
and return an empty result.
…ecurity/action/service/TransportGetServiceAccountAction.java Co-authored-by: Tim Vernum <tim@adjective.org>
This PR adds a new API endpoint to retrieve service accounts. Depends on the request parameters, it returns either all accounts, accounts belong to a namespace, a specific account, or an empty map if nothing is found.
This PR adds a new API endpoint to retrieve service accounts. Depends on the request parameter, it returns either all accounts, accounts belong to a namespace, a specific account, or an empty map if nothing is found.