michaeljmarshall commented on a change in pull request #14044:
URL: https://github.com/apache/pulsar/pull/14044#discussion_r805504203
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationService.java
##########
@@ -96,6 +97,14 @@ public String authenticateHttpRequest(HttpServletRequest
request) throws Authent
String.format("Unsupported authentication method:
[%s].", authMethodName));
}
try {
+ if (authData == null) {
+ // In the default implementation clas
OneStageAuthenticationStates, the authentication has been
+ // done, in order to avoid secondary authentication here
directly return the role, if the user
+ // custom implementation, should consider adding
authentication in their own implementation class.
+ AuthenticationState authenticationState =
providerToUse.newHttpAuthState(request);
Review comment:
I just realized that my suggestion to remove the
`providerToUse.authenticate(authData)` might have been misguided. The role of
the `authenticate` method is unclear to me in this new design. We know that the
default implementations call authenticate internally in the constructor.
However, I don't think the spec explicitly declares this requirement. I think
we might need the above or `authenticationState.authenticat(authData)` here.
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationService.java
##########
@@ -137,6 +143,10 @@ public String authenticateHttpRequest(HttpServletRequest
request) throws Authent
}
}
+ public String authenticateHttpRequest(HttpServletRequest request) throws
AuthenticationException {
Review comment:
Do you plan to modify the `AbstractWebSocketHandler`?
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -330,12 +356,20 @@ public String getAuthRole() throws
AuthenticationException {
return provider.getPrincipal(jwt);
}
+ /**
+ * Here is an explanation of why the null value is returned.
+ *
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationState.java#L49
+ */
@Override
public AuthData authenticate(AuthData authData) throws
AuthenticationException {
Review comment:
I think we should be able to remove the logic from this `authenticate`
method. That would also be an optimization for the `AuthenticationService` and
the `ServerCnx` so that it doesn't validate the token twice.
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -330,12 +356,20 @@ public String getAuthRole() throws
AuthenticationException {
return provider.getPrincipal(jwt);
}
+ /**
+ * Here is an explanation of why the null value is returned.
+ *
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationState.java#L49
+ */
@Override
public AuthData authenticate(AuthData authData) throws
AuthenticationException {
String token = new String(authData.getBytes(), UTF_8);
this.jwt = provider.authenticateToken(token);
- this.authenticationDataSource = new
AuthenticationDataCommand(token, remoteAddress, sslSession);
+ if (authHttpState) {
+ this.authenticationDataSource = new
AuthenticationDataHttps(this.request);
+ } else {
+ this.authenticationDataSource = new
AuthenticationDataCommand(token, remoteAddress, sslSession);
+ }
Review comment:
Nit: it seems like we should just create the `authenticationDataSource`
in the constructor. Then, we can get rid of the `authHttpState` as well as the
`remoteAddress` and `remoteAddress` fields.
--
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]
