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)
`roles` are the set of roles that were assigned to the user who is logging in.
The `sub` or `oid` can be used to get the identifier of the token
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]
