michaeljmarshall commented on code in PR #19197:
URL: https://github.com/apache/pulsar/pull/19197#discussion_r1073797306
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java:
##########
@@ -160,6 +163,20 @@ public String authenticate(AuthenticationDataSource
authData) throws Authenticat
}
}
+ @Override
+ public boolean authenticateHttpRequest(HttpServletRequest request,
HttpServletResponse response) throws Exception {
Review Comment:
My primary motivation for this PR is to implement PIP 97. If we want
authentication to be asynchronous, we cannot assume that when the
`AuthenticationState` object is initialized, the `authenticationDataSource` and
the `authRole` are present because the authentication might not yet be
completed. My goal with this PR is to break an unnecessary relationship between
`AuthenticationState` and http request authentication that was created by
https://github.com/apache/pulsar/pull/14044.
> 1. Using `newHttpAuthState` returns `AuthenticationState`, which includes
role and authentication data, we can simply get these from
`AuthenticationState`, and also quickly check the user authentication data.
This is not the intention of the `newHttpAuthState` method. The method was
added by https://github.com/apache/pulsar/pull/14044 so that the
`AuthenticationDataSource` for an `AuthenticationProvider` can be added on the
http request as the `AuthenticatedDataAttributeName` attribute on the
`request`. This PR proposes an alternate way to meet those criteria so that we
can implement PIP 97.
> 2. Keep the same logic with the `newAuthState`, it looks cleaner.
I disagree that it looks cleaner. The primary reason that we need a state
object in the `pulsar` protocol handlers is that we track `isExpired` and
whether or not the authentication is complete (first with `isComplete` and in
2.12 with the result of the `authenticationAsync`). We do not track state for
individual HTTP request connections, so we shouldn't have a method to create a
state object for HTTP requests.
Further, we're only building the `AuthenticationState` object to get the
`AuthenticationDataSource` and we're not calling the
`AuthenticationState#authenticate` method. There is no need for this
indirection. We can give `AuthenticationProvider` implementations freedom to
add the right `request` attributes by using the `authenticateHttpRequest` (and
eventually `authenticateHttpRequestAsync`).
Also, another problem is with the `AuthenticationDataSource` in the
`AuthenticationState`. I think we should recreate the
`AuthenticationDataSource` each time we call `authenticate`. However, if we
have `newHttpAuthState`, the `AuthenticationState` object is not able to create
a new `AuthenticationDataSource`. I think the `newHttpAuthState` creates an
confusion about how to use the `AuthenticationState`. This is a separate issue
that I will follow up on in another PR.
> I can accept authentication checks in the constructor of
`OneStageAuthenticationState`, it is a quick check.
It does not matter that it is "quick". It is a waste of resources to verify
authentication information twice.
> Maybe we can improve here, but the Pulsar must explicitly call the
`authenticate` of `AuthenticationState`.
This is already the way that Pulsar operates, but it has not been a stated
requirement. I think this is essential for making authentication asynchronous.
I will follow up with a separate PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
