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:
[email protected]
