michaeljmarshall commented on code in PR #19519:
URL: https://github.com/apache/pulsar/pull/19519#discussion_r1106643857
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockAuthenticationProvider.java:
##########
@@ -65,4 +71,41 @@ public String authenticate(AuthenticationDataSource
authData) throws Authenticat
"Not a valid principle. Should be
[pass|fail|error].[pass|fail|error], found " + principal);
}
+ @Override
+ public AuthenticationState newAuthState(AuthData authData, SocketAddress
remoteAddress, SSLSession sslSession)
+ throws AuthenticationException {
+ return new MockAuthState(this);
+ }
+
+ private static class MockAuthState implements AuthenticationState {
Review Comment:
I think it might make sense to extend this `MockAuthenticationProvider` to
get the semantics for the `newAuthState`. I did that with
`MockAlwaysExpiredAuthenticationProvider` and with
`MockMultiStageAuthenticationProvider`.
My primary concern is that existing tests are likely verifying the behavior
of the `OneStageAuthenticationState` class, and changing this method might
affect those tests.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -737,15 +737,15 @@ public void authChallengeSuccessCallback(AuthData
authChallenge,
// 2. an authentication refresh, in which case we need to
refresh authenticationData
AuthenticationState authState = useOriginalAuthState ?
originalAuthState : this.authState;
String newAuthRole = authState.getAuthRole();
+ AuthenticationDataSource newAuthDataSource =
authState.getAuthDataSource();
- // Refresh the auth data.
- this.authenticationData = authState.getAuthDataSource();
- if (log.isDebugEnabled()) {
- log.debug("[{}] Auth data refreshed for role={}",
remoteAddress, this.authRole);
- }
-
+ // Refresh the auth data and role.
if (!useOriginalAuthState) {
this.authRole = newAuthRole;
+ this.authenticationData = newAuthDataSource;
+ } else {
+ this.originalAuthData = newAuthDataSource;
+ this.originalPrincipal = newAuthRole;
Review Comment:
My primary concern about this approach is that it doesn't address the fact
that the `authenticationData` will never be updated after the initial handshake
when a proxy is involved.
I documented some of the challenges here
https://github.com/apache/pulsar/issues/19332.
I have been thinking it might make sense to get rid of the
`originalAuthData` field entirely because when it is set, we won't ever get the
proxy's auth data. The drawback of my approach is that it also means we would
skip authorization of the proxy's role. I personally think this is reasonable
because we can verify that the proxy is in the `proxyRoles` initially and then
not worry about the actual permission later.
However, it could make sense to merge this PR and then implement my proposed
change later. I will spend some time thinking about this and provide more
feedback tomorrow.
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java:
##########
@@ -1030,6 +1030,55 @@ public void
testVerifyAuthRoleAndAuthDataFromDirectConnectionBroker() throws Exc
}));
}
+ @Test
+ public void testRefreshOriginalPrincipalWithAuthDataForwardedFromProxy()
throws Exception {
Review Comment:
Note that the tests that comment about
`https://github.com/apache/pulsar/issues/19332` should fail because they make
assertions on the current behavior.
--
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]
