Make Authn and Audit take a HTTP interface when dealing with Rest requests#94990
Conversation
| .with(PRINCIPAL_FIELD_NAME, token.principal()) | ||
| .withRestUriAndMethod(request) | ||
| .withRestOrigin(threadContext) | ||
| .withRequestBody(request) |
There was a problem hiding this comment.
Authn failed & similar events are going to not display the request body anymore.
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| public interface HttpRequestLineAndHeaders { |
There was a problem hiding this comment.
New slim interface for HTTP requests which, importantly, doesn't expose access to the contents fo the request body.
There was a problem hiding this comment.
nit: can we call this HttpRequestLite or maybe or HttpNoBodyRequest or maybe change the existing one to HttpRequestWithBody and just call this HttpRequest ? The existing name IMO is hard to mentally parse since request line isn't something we tend to deal with much (and this class doesn't actually expose the raw line). Also, the name limits future uses of this interface to just a line and headers. (just a nitpick, feel free to ignore)
There was a problem hiding this comment.
I don't like HttpRequestLite because it's too ambiguous.
HttpNoBodyRequest makes it odd to serve as the base class of a request which does have a body (HttpRequest extends HttpNoBodyRequest). In addition, there are requests with no body (GETs), but this is not that. It's just a part of a full request that might or might not have a body, just that the body is not available yet, or in this particular abstraction.
I also don't like HttpRequestWithBody as it's very specific, as usually it's not important that it has a body; it's assumed to have one in all the many contexts where HTTP requests exist.
I went with HttpPreRequest it's like HttpRequestLite I think but conveys that it's actually part of a full request, more specifically the initial part of such.
| final RestRequest wrappedRequest = maybeWrapRestRequest(request); | ||
| RemoteHostHeader.process(request, threadContext); | ||
| authenticationService.authenticate(wrappedRequest, ActionListener.wrap(authentication -> { | ||
| authenticationService.authenticate(wrappedRequest.getHttpRequest(), ActionListener.wrap(authentication -> { |
There was a problem hiding this comment.
This is just temporary: the authenticationService.authenticate method is going to be invoked before a RestRequest is built. This is to be handled in the follow-up PR. For now, just call the authenticate method with the HTTP request that's used to construct the RestRequest.
| * @return the {@link RestRequest.Method} used in the REST request | ||
| * @throws IllegalArgumentException if the HTTP method is invalid | ||
| */ | ||
| RestRequest.Method method(); |
There was a problem hiding this comment.
I would like to refactor RestRequest.Method and bring the Method inner class to this interface, but the changeset is going to be huge. Should I go ahead?
There was a problem hiding this comment.
If it is massive, I would prefer a standalone PR. If it is just moving stuff around, that is pretty quick/easy to review.
|
Pinging @elastic/es-security (Team:Security) |
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| public interface HttpRequestLineAndHeaders { |
There was a problem hiding this comment.
nit: can we call this HttpRequestLite or maybe or HttpNoBodyRequest or maybe change the existing one to HttpRequestWithBody and just call this HttpRequest ? The existing name IMO is hard to mentally parse since request line isn't something we tend to deal with much (and this class doesn't actually expose the raw line). Also, the name limits future uses of this interface to just a line and headers. (just a nitpick, feel free to ignore)
| * @return the {@link RestRequest.Method} used in the REST request | ||
| * @throws IllegalArgumentException if the HTTP method is invalid | ||
| */ | ||
| RestRequest.Method method(); |
There was a problem hiding this comment.
If it is massive, I would prefer a standalone PR. If it is just moving stuff around, that is pretty quick/easy to review.
|
@elasticsearchmachine run elasticsearch-ci/part-1-fips |
|
@elasticsearchmachine run elasticsearch-ci/bwc |
|
@elasticsearchmachine update branch |
|
@elasticmachine run elasticsearch-ci/part-1-fips |
…uests (elastic#94990) AuthN and Audit are NOT going to have access to the Rest interface anymore, because the AuthenticationService is going to be invoked before the RestRequest is built. This refactoring makes the AuthenticationService and the AuditTrail take as parameters HTTP-related request interfaces.
AuthN and Audit are NOT going to have access to the
Restinterface anymore, because the
AuthenticationServiceisgoing to be invoked before the
RestRequestis built.This refactoring makes the
AuthenticationServiceandthe
AuditTrailtake as parameters HTTP-related requestinterfaces.