[improve][broker] Support revoking permission for AuthorizationProvider #20456

Technoboy- · 2023-06-01T08:46:00Z

Motivation

Current AuthorizationProvider does not actually fully support the revocation of ACLs :

Revocation is only supported for subscription-specific ACLs on a namespace
When we revoke the permission on Pulsar admin REST API, the handler will always try to remove it from the namespace policies (https://github.com/apache/pulsar/blob/242758d5770de46e506855ff881472cbc274cedb/pul[…]in/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java), instead of asking the AuthorizationProvider to do so.

Documentation

doc
doc-required
doc-not-needed
doc-complete

jiazhai

Thanks for the help. overall lgtm. Added some minor comments.

jiazhai · 2023-06-01T09:27:16Z

...roker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java

+     * @param role
+     * @return
+     */
+    default CompletableFuture<Void> revokePermissionAsync(NamespaceName namespace, String role) {


Why do we need a default implementation here? How about removing it so that we can keep aligned with other APIs in this file?

It’s better to have default implementations here, otherwise it will cause compile issue when upgrade.

Agree, also the "revoke" is already broken today when using a custom authz plugin. Even if today it succeeds, the revoke does nothing because it would just try to remove the ACL from zookeeper, where there are not any.

...roker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java

...common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java

lifepuzzlefun · 2023-06-01T14:05:13Z

...roker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java

+     * @return CompletableFuture<Void>
+     */
+    default CompletableFuture<Void> revokePermissionAsync(NamespaceName namespace, String role) {
+        return FutureUtil.failedFuture(new IllegalStateException(


maybe java.lang.UnsupportedOperationException is more suitable.

We'd better keep the same exception as with other default implementation

lifepuzzlefun · 2023-06-01T14:15:41Z

I have the same question as jiazhai if the default method is return CompletableFuture with Exception.
maybe we need cover the exception in the outer admin api handler to avoid earlier version of user-define AuthorizationProvider fail.

Technoboy- · 2023-06-01T15:05:34Z

I have the same question as jizhai if the default method is return CompletableFuture with Exception. maybe we need cover the exception in the outer admin api handler to avoid earlier version of user-define AuthorizationProvider fail.

I may not get what you mean. Earlier version won't fail

merlimat · 2023-06-01T15:32:50Z

...roker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java

+     * @param role
+     * @return
+     */
+    default CompletableFuture<Void> revokePermissionAsync(NamespaceName namespace, String role) {


Agree, also the "revoke" is already broken today when using a custom authz plugin. Even if today it succeeds, the revoke does nothing because it would just try to remove the ACL from zookeeper, where there are not any.

lifepuzzlefun · 2023-06-01T15:48:29Z

I have the same question as jizhai if the default method is return CompletableFuture with Exception. maybe we need cover the exception in the outer admin api handler to avoid earlier version of user-define AuthorizationProvider fail.

I may not get what you mean. Earlier version won't fail

Sorry for my misunderstanding comments. I mean that if previous version user-define AuthorizationProvider didn't override the default method. when user call the admin api and execute the default method. will it be failed ? like org.apache.pulsar.broker.authorization.MockAuthorizationProvider.

Technoboy- · 2023-06-01T16:17:01Z

I have the same question as jizhai if the default method is return CompletableFuture with Exception. maybe we need cover the exception in the outer admin api handler to avoid earlier version of user-define AuthorizationProvider fail.

I may not get what you mean. Earlier version won't fail

Sorry for my misunderstanding comments. I mean that if previous version user-define AuthorizationProvider didn't override the default method. when user call the admin api and execute the default method. will it be failed ? like org.apache.pulsar.broker.authorization.MockAuthorizationProvider.

Ah, yes, it will fail in this case

codecov-commenter · 2023-06-02T03:31:53Z

Codecov Report

Attention: Patch coverage is 57.89474% with 16 lines in your changes missing coverage. Please review.

Project coverage is 72.91%. Comparing base (5e6e6ce) to head (4bf5972).
Report is 1288 commits behind head on master.

Files with missing lines	Patch %	Lines
...ker/authorization/PulsarAuthorizationProvider.java	61.29%	8 Missing and 4 partials ⚠️
...ar/broker/authorization/AuthorizationProvider.java	0.00%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #20456      +/-   ##
============================================
- Coverage     72.95%   72.91%   -0.04%     
- Complexity    31914    31916       +2     
============================================
  Files          1867     1867              
  Lines        138485   138522      +37     
  Branches      15202    15204       +2     
============================================
- Hits         101037   101009      -28     
- Misses        29438    29503      +65     
  Partials       8010     8010

Flag	Coverage Δ
inttests	`24.16% <0.00%> (+0.04%)`	⬆️
systests	`24.96% <2.63%> (-0.08%)`	⬇️
unittests	`72.20% <57.89%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...sar/broker/authorization/AuthorizationService.java	`57.53% <100.00%> (+0.33%)`	⬆️
...ar/broker/authorization/AuthorizationProvider.java	`8.51% <0.00%> (-0.80%)`	⬇️
...ker/authorization/PulsarAuthorizationProvider.java	`71.19% <61.29%> (-1.45%)`	⬇️

... and 62 files with indirect coverage changes

codelipenghui

LGTM

@Technoboy- It looks like you will create another PR to make sure all the revoking calls point to the newly added method, right?

Technoboy- · 2023-06-02T06:16:39Z

LGTM

@Technoboy- It looks like you will create another PR to make sure all the revoking calls point to the newly added method, right?

yes, right.

michaeljmarshall

I support this API expansion. It would have been nice to send a notice to the private or dev list, though. Thanks.

michaeljmarshall · 2023-06-02T20:39:54Z

@Technoboy- - one important detail that we should also cover is the fact that all of the "get permission" endpoints make the assumption that the permissions are stored in zk. We should probably expand the AuthorizationProvider to include get methods.

merlimat · 2023-06-03T17:18:55Z

@Technoboy- - one important detail that we should also cover is the fact that all of the "get permission" endpoints make the assumption that the permissions are stored in zk. We should probably expand the AuthorizationProvider to include get methods.

I think these might be trickier to support. Many backends would not be able to answer that question directly (or at least the broker might not have access to it).

michaeljmarshall · 2023-06-03T17:21:56Z

That's fair, but I think it might make sense to let the interface's implementation decide how to handle that instead of reading potentially irrelevant information from the metadata store. I don't think it has to be fixed along with this PR, but in my opinion, that is the right direction.

merlimat · 2023-06-03T17:27:53Z

Sure, it makes sense. Right now it's anyway broken when it returns an empty list

Technoboy- added 2 commits June 1, 2023 16:35

Support revoking permission for AuthorizationProvider

515297c

remove blank

93de33d

Technoboy- self-assigned this Jun 1, 2023

Technoboy- added the ready-to-test label Jun 1, 2023

Technoboy- added this to the 3.1.0 milestone Jun 1, 2023

Technoboy- added type/feature The PR added a new feature or issue requested a new feature area/broker labels Jun 1, 2023

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 1, 2023

Technoboy- changed the title ~~[feat][broker] Support revoking permission for AuthorizationProvider.java~~ [feat][broker] Support revoking permission for AuthorizationProvider Jun 1, 2023

Technoboy- changed the title ~~[feat][broker] Support revoking permission for AuthorizationProvider~~ [improve][broker] Support revoking permission for AuthorizationProvider Jun 1, 2023

jiazhai reviewed Jun 1, 2023

View reviewed changes

address comment

1171eb6

lifepuzzlefun reviewed Jun 1, 2023

View reviewed changes

address comment

4bf5972

merlimat approved these changes Jun 1, 2023

View reviewed changes

jiazhai approved these changes Jun 2, 2023

View reviewed changes

codelipenghui approved these changes Jun 2, 2023

View reviewed changes

Technoboy- merged commit e220a5d into apache:master Jun 2, 2023

Technoboy- mentioned this pull request Jun 2, 2023

[improve][admin] Using AuthorizationService to revoke the permissions. #20478

Merged

4 tasks

michaeljmarshall reviewed Jun 2, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improve][broker] Support revoking permission for AuthorizationProvider #20456

[improve][broker] Support revoking permission for AuthorizationProvider #20456

Technoboy- commented Jun 1, 2023

jiazhai left a comment

jiazhai Jun 1, 2023

Technoboy- Jun 1, 2023

merlimat Jun 1, 2023

lifepuzzlefun Jun 1, 2023

Technoboy- Jun 1, 2023

lifepuzzlefun Jun 1, 2023

lifepuzzlefun commented Jun 1, 2023 •

edited

Loading

Technoboy- commented Jun 1, 2023

merlimat Jun 1, 2023

lifepuzzlefun commented Jun 1, 2023

Technoboy- commented Jun 1, 2023

codecov-commenter commented Jun 2, 2023 •

edited

Loading

codelipenghui left a comment

Technoboy- commented Jun 2, 2023

michaeljmarshall left a comment

michaeljmarshall commented Jun 2, 2023

merlimat commented Jun 3, 2023

michaeljmarshall commented Jun 3, 2023 •

edited

Loading

merlimat commented Jun 3, 2023

[improve][broker] Support revoking permission for AuthorizationProvider #20456

[improve][broker] Support revoking permission for AuthorizationProvider #20456

Conversation

Technoboy- commented Jun 1, 2023

Motivation

Documentation

jiazhai left a comment

Choose a reason for hiding this comment

jiazhai Jun 1, 2023

Choose a reason for hiding this comment

Technoboy- Jun 1, 2023

Choose a reason for hiding this comment

merlimat Jun 1, 2023

Choose a reason for hiding this comment

lifepuzzlefun Jun 1, 2023

Choose a reason for hiding this comment

Technoboy- Jun 1, 2023

Choose a reason for hiding this comment

lifepuzzlefun Jun 1, 2023

Choose a reason for hiding this comment

lifepuzzlefun commented Jun 1, 2023 • edited Loading

Technoboy- commented Jun 1, 2023

merlimat Jun 1, 2023

Choose a reason for hiding this comment

lifepuzzlefun commented Jun 1, 2023

Technoboy- commented Jun 1, 2023

codecov-commenter commented Jun 2, 2023 • edited Loading

Codecov Report

codelipenghui left a comment

Choose a reason for hiding this comment

Technoboy- commented Jun 2, 2023

michaeljmarshall left a comment

Choose a reason for hiding this comment

michaeljmarshall commented Jun 2, 2023

merlimat commented Jun 3, 2023

michaeljmarshall commented Jun 3, 2023 • edited Loading

merlimat commented Jun 3, 2023

lifepuzzlefun commented Jun 1, 2023 •

edited

Loading

codecov-commenter commented Jun 2, 2023 •

edited

Loading

michaeljmarshall commented Jun 3, 2023 •

edited

Loading