cckellogg commented on a change in pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#discussion_r639707307
##########
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 =
Lists.newArrayList(provider.authenticate(authenticationDataSource));
Review comment:
Collections.singletonList(provider.authenticate(authenticationDataSource)));
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java
##########
@@ -19,12 +19,15 @@
package org.apache.pulsar.broker.authentication;
+import com.google.common.collect.Lists;
Review comment:
This is not needed
##########
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
##########
@@ -778,9 +794,12 @@ protected void handleConnect(CommandConnect connect) {
// Not find provider named authMethod. Most used for tests.
// In AuthenticationDisabled, it will set authMethod "none".
if (authenticationProvider == null) {
- authRole =
getBrokerService().getAuthenticationService().getAnonymousUserRole()
Review comment:
What is the value of this authRole if the `AnonymousUserRole` is not
set? is it null. We should avoid any lists where null is an element.
```
final String anonymousUserRole =
getBrokerService().getAuthenticationService().getAnonymousUserRole().orElseThrow(()
-> new AuthenticationException("No anonymous role, and no authentication
provider " + "configured")));
authRoles = StringUtils.isNotEmpty(anonymousUserRole) ?
Collections.singletonList(anonymousUserRole) : Collections.emptyList()
```
##########
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:
This is confusing to me `roles` are a set of permissions assigned to a
principal. The job of authentication is to identify the user/app and get the
principal/app id. For any given authData there should only be one identifier
for it.
Roles should only be applicable with performing an authorization check.
These roles can be accessed in the authorization provider by reading them from
the authdata with will have the token.
I see other changes are references from the
(https://github.com/apache/pulsar/pull/10375)
https://docs.microsoft.com/en-us/azure/active-directory/develop/id-tokens.
I think the `AuthenticationProviderToken.java` probably needs to be adjusted
first.
The `sub` claim should be used to identify the principal or app id
https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.2
##########
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
##########
@@ -346,8 +348,19 @@ 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) {
+ e.printStackTrace();
Review comment:
remove `e.printStackTrace()` and add log.warn or log.info
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -144,14 +145,29 @@ public String getAuthMethodName() {
@Override
public String authenticate(AuthenticationDataSource authData) throws
AuthenticationException {
+ List<String> principals = authenticate(authData, false);
+ if (principals == null) {
+ return null;
+ }
+ return principals.get(0);
+ }
+
+ @Override
+ public List<String> authenticate(AuthenticationDataSource authData,
boolean multiRoles) throws AuthenticationException {
try {
// Get Token
String token;
token = getToken(authData);
// Parse Token by validating
- String role = getPrincipal(authenticateToken(token));
+ List<String> principals = getPrincipals(authenticateToken(token));
AuthenticationMetrics.authenticateSuccess(getClass().getSimpleName(),
getAuthMethodName());
- return role;
+ if(multiRoles){
Review comment:
spacing
--
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]
