michaeljmarshall commented on code in PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#discussion_r1142515206
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java:
##########
@@ -20,39 +20,60 @@
import static java.nio.charset.StandardCharsets.UTF_8;
import java.net.SocketAddress;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
import javax.naming.AuthenticationException;
import javax.net.ssl.SSLSession;
import javax.servlet.http.HttpServletRequest;
import org.apache.pulsar.common.api.AuthData;
/**
- * Interface for authentication state.
- *
- * It tell broker whether the authentication is completed or not,
- * if completed, what is the AuthRole is.
+ * A class to track single stage authentication. This class assumes that:
+ * 1. {@link #authenticateAsync(AuthData)} is called once and when the {@link
CompletableFuture} completes,
+ * authentication is complete.
+ * 2. Authentication does not expire, so {@link #isExpired()} always returns
false.
+ * <p>
+ * See {@link AuthenticationState} for Pulsar's contract on how this interface
is used by Pulsar.
*/
public class OneStageAuthenticationState implements AuthenticationState {
- private final AuthenticationDataSource authenticationDataSource;
- private final String authRole;
+ private AuthenticationDataSource authenticationDataSource;
+ private final SocketAddress remoteAddress;
+ private final SSLSession sslSession;
+ private final AuthenticationProvider provider;
+ private volatile String authRole;
+
+ /**
+ * Constructor for a {@link OneStageAuthenticationState} where there is no
authentication performed during
+ * initialization.
+ * @param remoteAddress - remoteAddress associated with the {@link
AuthenticationState}
+ * @param sslSession - sslSession associated with the {@link
AuthenticationState}
+ * @param provider - {@link AuthenticationProvider} to use to verify
{@link AuthData}
+ */
public OneStageAuthenticationState(AuthData authData,
SocketAddress remoteAddress,
SSLSession sslSession,
- AuthenticationProvider provider) throws
AuthenticationException {
- this.authenticationDataSource = new AuthenticationDataCommand(
- new String(authData.getBytes(), UTF_8), remoteAddress, sslSession);
- this.authRole = provider.authenticate(authenticationDataSource);
+ AuthenticationProvider provider) {
+ this.provider = provider;
+ this.remoteAddress = remoteAddress;
+ this.sslSession = sslSession;
}
- public OneStageAuthenticationState(HttpServletRequest request,
AuthenticationProvider provider)
- throws AuthenticationException {
+ public OneStageAuthenticationState(HttpServletRequest request,
AuthenticationProvider provider) {
+ // Must initialize this here for backwards compatibility with http
authentication
this.authenticationDataSource = new AuthenticationDataHttps(request);
- this.authRole = provider.authenticate(authenticationDataSource);
+ this.provider = provider;
+ // These are not used when invoking this constructor.
+ this.remoteAddress = null;
+ this.sslSession = null;
}
@Override
- public String getAuthRole() {
+ public String getAuthRole() throws AuthenticationException {
+ if (authRole == null) {
+ throw new AuthenticationException("Must authenticate before
calling getAuthRole");
+ }
Review Comment:
> Could you show an example of a possible unexpected behavior? IMO, the
existing authentication provider only overrides the previous `newAuthState`
method still has the previous behavior and is not affected by the new
`newAuthState` method.
The challenge comes from the fact that Apache Pulsar will transition to
using the new method to build the state object and if that method is not
overridden by the custom `AuthenticationProvider` implementation, the
`OneStageAuthenticationState` will be used. That will lead to unexpected
behavior.
A good example is the `AuthenticationProviderToken` class. That one would
"work" with the `OneStageAuthenticationState` with the key difference being the
`isExpired` value from `OneStageAuthenticationState` always returns `false`
while the `TokenAuthenticationState` returns `true` when the token's `exp`
claim indicates the token has expired.
--
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]
