[fix][broker] Move superuser/tenantAdmin checks to AuthorizationService from PulsarAuthorizationProvider #20145
Conversation
michaeljmarshall
left a comment
michaeljmarshall
left a comment
There was a problem hiding this comment.
Thanks for putting together this PR, @nodece . I think we are coming at this from two different angles. I do not think we should remove logic from the AuthorizationProvider in this way. Let me know your thoughts on the comments. Thanks!
| } | ||
| return provider.allowTopicOperationAsync(topicName, role, TopicOperation.PRODUCE, authenticationData); | ||
|
|
||
| return isSuperUserOrAdmin(topicName.getTenant(), role, authenticationData) |
There was a problem hiding this comment.
I do not think we should move in this direction. I think our target design should be to let the AuthorizationProvider implementation decide what level of permission is required. This line here removes that abstraction from the AuthorizationProvider and puts it in the AuthorizationService. I concede that there are some places where we use these checks outside of the AuthorizationProvider, however, those are legacy details that we have to support until we find a way to move them into the AuthorizationProvider.
There was a problem hiding this comment.
I think our target design should be to let the
AuthorizationProviderimplementation decide what level of permission is required
We have isTenantAdmin and isSuperUser methods in the AuthorizationProvider. The user just overrides that to decide what level of permission is required. When isTenantAdmin and isSuperUser return false, the AuthorizationService call to the AuthorizationProvider.
There is another reason that I think plugin authors should not pay attention to isTenantAdmin and isSuperUser details.
There was a problem hiding this comment.
We have
isTenantAdminandisSuperUsermethods in theAuthorizationProvider.
That is a fair point. I think I've been operating under the assumption that these are a bit of an anti-pattern with the idea that it is better to give users the ability to configure granular permissions.
I think plugin authors should not pay attention to
isTenantAdminandisSuperUserdetails.
I agree with this, and this is part of why I do not think we should create new dependencies on these methods. When I added isSuperUserOrAdmin recently, I did so because it was necessary to consolidate existing logic. I made it private in an attempt to prevent new usage, but of course that wouldn't stop the AuthorizationService from using it.
There was a problem hiding this comment.
When
isTenantAdminandisSuperUserreturnfalse, theAuthorizationServicecall to theAuthorizationProvider.
My primary objection to moving this logic out of the AuthorizationProvider is that implementations have almost certainly already accounted for the fact that this method (and all of the similar ones modified in this PR) do not themselves check if the client is a super user or a tenant admin. As such, we could be creating unnecessary overhead by moving this logic out of the AuthorizationProvider and into the AuthorizationService.
These are my initial thoughts. I'll continue to think about this more. Thanks for the discussion!
There was a problem hiding this comment.
@michaeljmarshall Do you have a new idea? IMO, I think this change makes sense.
| log.debug("Check allowNamespaceOperationAsync [{}] on [{}].", operation.name(), namespaceName); | ||
| } | ||
|
|
||
| return validateTenantAdminAccess(namespaceName.getTenant(), role, authData) |
There was a problem hiding this comment.
I think this call rightly belongs in the AuthorizationProvider impelmentation.
|
/pulsarbot rerun-failure-checks |
…ce from PulsarAuthorizationProvider Signed-off-by: nodece <nodeces@gmail.com>
|
The pr had no activity for 30 days, mark with Stale label. |
5 participants
Fixes #20066
Motivation
When a role wants to use the resource, the role needs to have resource permissions. If a role is a super user or tenant admin we should allow this request immediately, otherwise go to call the
AuthorizationProviderto check the permission.We currently have superuser/tenant administrator checks in
PulsarAuthorizationProviderandAuthorizationService, there is confusion so the #20066 issue is introduced. So I suggest unifying superuser/tenantAdmin checks in theAuthorizationService, and then theAuthorizationProvideronly needs to consider their business permissions.Modifications
AuthorizationServicefromPulsarAuthorizationProviderVerifying this change
Documentation
docdoc-requireddoc-not-neededdoc-complete