cckellogg commented on a change in pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#discussion_r641165597
##########
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 the confusing part is we are using too many terms (role,
principal id, app id, etc..) that are being overloaded. In my opinion the job
of the authentication provider is to identity the client and this should be a
single distinct value (principal id). So I guess the question is what should
the function be for the authorization provider? Should it identity the client
or provide the client roles? I think it should identity the client because
multiple clients could have the same roles and then it can become tricky from
the logs and and auditing perspective what client is doing what specific action.
This change forces the broker server context and other classes to iterate
over a list in multiple places and compare/sort lists. I think that complicates
the code, makes it harder to maintain and reason about. The auth checks in the
broker are already complicated and we should try to simply that. We should be
trying to abstract and delegate these type of things to the auth providers,
especially since this feature can already be done without any changes to the
broker code or auth interfaces. In my opinion there are better ways to go about
this.
Here are a few options:
1 - have the authorization provider discover the roles it should check for
permissions. This requires none or minimal changes to the broker code. The
authdata passed in to the authorization provider contains everything that is
needed.
2 - Create an AuthInfo class or something similar that is returned by the
authentication provider and that will contain a principal id and possible list
of roles. However, this will involve changing the authorization interface to
accept this new object and will be much larger change. The positive is this
makes things much more flexible and extensible in the future.
On a side note there probably needs to be a cleanup of these terms
(principal, role, app-id, client-id, etc...) within the code base so were
consistent on what they mean.
--
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]
