cckellogg commented on a change in pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#discussion_r639000237
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java
##########
@@ -35,20 +38,33 @@
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);
Review comment:
We should check multi is implemented and then call the appropriated
method. This change will cause users who have custom implementations to always
throw an exception
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java
##########
@@ -63,7 +66,11 @@
* if the credentials are not valid
*/
default String authenticate(AuthenticationDataSource authData) throws
AuthenticationException {
- throw new AuthenticationException("Not supported");
+ return authenticate(authData, false).get(0);
Review comment:
Keep the AuthenticationException("Not supported").
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java
##########
@@ -35,20 +38,33 @@
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 =
Lists.newArrayList(provider.authenticate(authenticationDataSource));
+ } else {
+ throw e;
+ }
+ }
}
@Override
public String getAuthRole() {
- return authRole;
+ return authRoles.get(0);
Review comment:
What is the list is empty or null?
##########
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
##########
@@ -346,8 +348,18 @@ private boolean invalidOriginalPrincipal(String
originalPrincipal) {
} else {
isProxyAuthorizedFuture =
CompletableFuture.completedFuture(true);
}
- isAuthorizedFuture =
service.getAuthorizationService().allowTopicOperationAsync(
- topicName, operation, authRole, authenticationData);
+ isAuthorizedFuture = CompletableFuture.supplyAsync(() -> {
+ for (String authRole : authRoles) {
+ try {
+ if
(service.getAuthorizationService().allowTopicOperationAsync(
+ topicName, operation, authRole,
authenticationData).get()) {
+ return true;
+ }
+ } catch (InterruptedException | ExecutionException e) {
Review comment:
Will this hide any errors we could be interested in?
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationState.java
##########
@@ -37,6 +39,10 @@
*/
String getAuthRole() throws AuthenticationException;
+ default List<String> getAuthRoles() throws AuthenticationException {
+ return Collections.singletonList(getAuthRole());
Review comment:
What if the getAuthRole returns null or empty string I think this should
return an empty list
--
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]
