User Profile: Activate user profile API #82400
Conversation
ywangd
added
>enhancement
WIP
:Security/Security
|
Pinging @elastic/es-security (Team:Security) |
|
|
||
| @Override | ||
| protected void doExecute(Task task, ActivateProfileRequest request, ActionListener<ActivateProfileResponse> listener) { | ||
| try (ThreadContext.StoredContext ignore = threadContext.stashContext()) { |
There was a problem hiding this comment.
I realise this is taken from TransportGrantApiKeyAction, but I think you could make use of SecurityContext#executeWithAuthentication to simplify it all.
There was a problem hiding this comment.
I am not entirely sure about using SecurityContext#executeWithAuthentication. It is currently only used for secondary authentication where I think it is a better fit because the it means to pretend the secondary authentication is the only authentication object and forget about the initial authentication entirely.
Grant is very similar. Indeed, it currently has the same practical behaviour once the authentication from the request body is resolved. But I think the semantics is slight different. There were discussions about how the grant type requests could work in the background and one proposal is to serialised both authentication objects. This may not end up to be the solution. But I think it does highlight the difference between grant and secondary authentication in that Grant needs 2 authentiation objects to fully work. So I'd refrain from using SecurityContext#executeWithAuthentication right now.
Also, in the latest commit I extracted a superclass for grant type action to have common executeWithGrantAuthentication method. So hopefully this refactoring can still achieve better simplicity without resorting to SecurityContext#executeWithAuthentication.
| request.getGrant(), | ||
| request, | ||
| ActionListener.wrap( | ||
| authentication -> profileService.activateProfile(authentication, listener.map(ActivateProfileResponse::new)), |
There was a problem hiding this comment.
I would avoid passing in the authentication as a parameter, instead you could make the service only activate the profile of the currently authenticated user. Otherwise there is the potential for ambiguity, when the two authentications diverge.
There was a problem hiding this comment.
This is only possible if we replace authentication with SecurityContext#executeWithAuthentication. As commented above, I am not entirely convinced about this approach. Replacing authentication object also incur additional serialisation cost which I am not fond of either. We have many methods that accept Authentication as a parameter, e.g. methods of TokenService, ApiKeyService, CompositeRolesStore etc. Given callers of these methods are well controlled, I am not too worried about the confusion. I can also add some comment to help clarify the usage.
| TransportRequest transportRequest, | ||
| ActionListener<Authentication> listener | ||
| ) { | ||
| switch (grant.getType()) { |
There was a problem hiding this comment.
The switch basically chooses between new UsernamePasswordToken(grant.getUsername(), grant.getPassword()) and new BearerToken(grant.getAccessToken()), plus it validates the grant.Type() request parameter. I think this method can be improved for readability.
There was a problem hiding this comment.
This method is also copied from TransportGrantApiKeyAction. I had a TODO in the beginning of this method and was meant to refactor it in a separate PR. But we may as well handle it now. I extracted Grant into a separate class and extract base classes for both GrantApiKey and ActivateProfile related classes. Hopefully the new change makes it read better.
There was a problem hiding this comment.
LGTM Yang!
I only had a couple of small comments, but I admit I wasn't very diligent on this one. This being a new API (rather than changing an existing one), I'll go over it again when all the pieces are in place, notably the core change #82639 .
This PR adds a REST endpoint to activate user profile.
Notes:
displayName, can be specified by the calling application.