[GitHub] [pulsar] cckellogg commented on a change in pull request #10685: [Broker] Add multi roles support for authentication and authorization

[GitBox <gi...@apache.org>]

cckellogg commented on a change in pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#discussion_r643214504



##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java
##########
@@ -35,20 +38,36 @@
 public class OneStageAuthenticationState implements AuthenticationState {
 
     private final AuthenticationDataSource authenticationDataSource;
-    private final String authRole;
+    private List<String> authRoles;
 
     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);
+        try {
+            this.authRoles = provider.authenticate(authenticationDataSource, true);
+        } catch (AuthenticationException e) {
+            if (e.getMessage().equals(MULTI_ROLE_NOT_SUPPORTED)) {
+                this.authRoles = Collections.singletonList(provider.authenticate(authenticationDataSource));
+            } else {
+                throw e;
+            }
+        }
     }
 
     @Override
     public String getAuthRole() {
-        return authRole;
+        if (authRoles == null || authRoles.isEmpty()) {
+            return null;
+        }
+        return authRoles.get(0);
+    }
+
+    @Override
+    public List<String> getAuthRoles() {
+        return authRoles;

Review comment:
       Yes, that was a mistake. It's better to return an empty collection instead of null.

##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java
##########
@@ -66,6 +69,10 @@ default String authenticate(AuthenticationDataSource authData) throws Authentica
         throw new AuthenticationException("Not supported");
     }
 
+    default List<String> authenticate(AuthenticationDataSource authData, boolean multiRoles) throws AuthenticationException {

Review comment:
       I think this should be done right and we should not push through something without thinking about the design and what role the authorization should play.  What you are trying to do can already be accomplished with the current framework (no code changes to any core pieces) by implementing a custom authorization provider. If we are going to make interfaces changes a design should be proposed to the community. I think it's better to think about this and come up with a design to support other potential use cases. 




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org