nodece commented on code in PR #19197:
URL: https://github.com/apache/pulsar/pull/19197#discussion_r1073220853
##########
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:
Your PR is awesome! but I have second thoughts about whether to deprecate
the `newHttpAuthState`, I don't suggest you deprecate this method,
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.
2. Keep the same logic with the `newAuthState`, it looks cleaner.
> This PR deprecates `newHttpAuthState` because that method creates an
object that is unnecessary and that triggers an authentication check for no
reason in the `OneStageAuthenticationState` class.
I can accept authentication checks in the constructor of
`OneStageAuthenticationState`, it is a quick check.
Maybe we can improve here, but the Pulsar must explicitly call the
`authenticate` of `AuthenticationState`.
--
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]
