michaeljmarshall commented on code in PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#discussion_r1141038277
##########
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:
@BewareMyPower - I finally got a chance to come back to this, and now I
remember that I did try to use a similar solution that you proposed while
writing this PR, but I found it didn't actually work as you propose. There are
a few details to show the issue:
First, the `OneStageAuthenticationState` is not used directly by the broker
or by plugins. Instead, the broker and plugins use
`AuthenticationProvider#newAuthState` to create the `AuthenticationState`
object. Here is the code:
https://github.com/apache/pulsar/blob/fb7f14ceb04d612e456b2e5a834385ae3a97f68f/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java#L96-L104
As such, changing the constructor in the way you propose is unlikely to help
users that built their own extensions. For example, KoP does the following:
https://github.com/streamnative/kop/blob/c9f66e4d24608dea6c7f4badde1d288b7dc1f74d/kafka-impl/src/main/java/io/streamnative/pulsar/handlers/kop/SchemaRegistryManager.java#L105-L106
Users must rely on the `AuthenticationProvider#newAuthState` in order to
make the `AuthenticationProvider` pluggable.
When I noticed this detail while writing this PR, I thought about adding a
new method to the `AuthenticationProvider` that would not take the `AuthData`
object, in the same way that you proposed for the new
`OneStageAuthenticationState` constructor. However, the main issue is how to
default that method in the interface. We could use the following definition:
```java
default AuthenticationState newAuthState(SocketAddress remoteAddress,
SSLSession sslSession)
throws AuthenticationException {
return new OneStageAuthenticationState(remoteAddress, sslSession,
this);
}
```
This would be analogous to the current `newAuthState` method, but the issue
is that custom `AuthenticationProvider` implementations that override the
current `newAuthState` method likely don't want to use the
`OneStageAuthenticationState`, and that would lead to unexpected behavior.
Another option could be:
```java
default AuthenticationState newAuthState(SocketAddress remoteAddress,
SSLSession sslSession)
throws AuthenticationException {
return newAuthState(null, remoteAddress, sslSession);
}
```
However, that would require breaking the current implementation in the way
you're concerned about. (For what it's worth, this option might be the best
long term solution for cleaning up the interface if we want to remove the
`authData` that is passed. The main point is that it doesn't take care of your
concern of not breaking user code.)
Another option:
```java
AuthenticationState newAuthState(SocketAddress remoteAddress, SSLSession
sslSession);
```
This unimplemented option has two problems. First, it will break user code
if they do not implement the method. Since this whole exercise is an effort to
try to prevent users from needing to change any code, that isn't a good option.
Second, it removes the `OneStageAuthenticationState` default already present in
the interface, which seems problematic.
In my view, the primary challenge is that the `AuthenticationProvider`
interface has a brittle default behavior.
Given the above, do you have any ideas on how to make this better for users
building their own broker extensions?
--
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]
