cckellogg commented on a change in pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#discussion_r639732409
##########
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:
Why do the roles need to be returned here?
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. Im my opinion this in the the right way to go about this.
Roles should only be applicable when performing an authorization check.
These roles can be accessed in the authorization provider by reading them from
the authdata (this contains the token or cert). The default authorization
provider could be updated to have some type of adapter to return the `roles`
based on the authdata and principal that gets passed in. Then the default
authorization provider can make checks against those roles.
I see this is related to (https://github.com/apache/pulsar/pull/10375)
According the the microsoft docs
https://docs.microsoft.com/en-us/azure/active-directory/develop/id-tokens.
I think the `AuthenticationProviderToken.java` probably needs to be adjusted
first.
Something other than roles like `sub` should be used to identify the
principal or app id
https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.2
--
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]
