[improve][broker] Support get/remove permissions for AuthorizationProvider #20496

Technoboy- · 2023-06-05T15:08:19Z

Motivation

This is a follow-up improvement for AuthorizationProvider. As discussed in the #20478-comment, we need more methods to avoid admin handling permissions directly.

Modification

Add removePermissionsAsync: when deleting partitioned topics, we need to delete the authentication data.
Add getPermissionsAsync: we have topic level API to get all the permissions. so we need this.
Add getSubscriptionPermissionsAsync: we have topic level API to get all subscription permissions.
Add getPermissionsAsync: we have namespace level API to get all the permissions.

Documentation

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

michaeljmarshall

Nice direction @Technoboy-. I'll review again when it is ready for review.

Technoboy- · 2023-06-06T07:03:47Z

https://github.com/apache/pulsar/actions/runs/5184201141/jobs/9344064663

Error:  Medium: org.apache.pulsar.websocket.WebSocketService.getAuthorizationService() may expose internal representation by returning WebSocketService.authorizationService [org.apache.pulsar.websocket.WebSocketService] At WebSocketService.java:[line 177] EI_EXPOSE_REP
[INFO] 


To see bug detail using the Spotbugs GUI, use the following command "mvn spotbugs:gui"

For the above spotbugs issue, I have added this fix
023781c

Technoboy- · 2023-06-06T07:04:19Z

Nice direction @Technoboy-. I'll review again when it is ready for review.

Thanks

…nDisableAsync`

codecov-commenter · 2023-06-07T04:57:23Z

Codecov Report

Merging #20496 (0b283dc) into master (3b862ae) will increase coverage by 36.12%.
The diff coverage is 59.53%.

@@              Coverage Diff              @@
##             master   #20496       +/-   ##
=============================================
+ Coverage     36.78%   72.90%   +36.12%     
- Complexity    12059    31942    +19883     
=============================================
  Files          1690     1867      +177     
  Lines        129001   138614     +9613     
  Branches      14041    15222     +1181     
=============================================
+ Hits          47453   101061    +53608     
+ Misses        75294    29535    -45759     
- Partials       6254     8018     +1764

Flag	Coverage Δ
inttests	`24.21% <10.98%> (+0.07%)`	⬆️
systests	`24.97% <4.62%> (+0.01%)`	⬆️
unittests	`72.19% <59.53%> (+40.25%)`	⬆️

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

Impacted Files	Coverage Δ
...ar/broker/authorization/AuthorizationProvider.java	`7.27% <0.00%> (+5.14%)`	⬆️
.../apache/pulsar/broker/admin/impl/PackagesBase.java	`76.63% <ø> (+24.34%)`	⬆️
.../apache/pulsar/functions/instance/ContextImpl.java	`61.00% <0.00%> (+18.41%)`	⬆️
.../org/apache/pulsar/broker/admin/v1/Namespaces.java	`52.28% <50.00%> (+43.97%)`	⬆️
...s/runtime/kubernetes/KubernetesRuntimeFactory.java	`80.21% <50.00%> (+80.21%)`	⬆️
...pache/pulsar/broker/admin/impl/NamespacesBase.java	`72.05% <56.86%> (+41.36%)`	⬆️
...ker/authorization/PulsarAuthorizationProvider.java	`68.11% <60.24%> (+36.42%)`	⬆️
...sar/broker/authorization/AuthorizationService.java	`58.20% <100.00%> (+45.90%)`	⬆️
.../org/apache/pulsar/broker/admin/AdminResource.java	`78.85% <100.00%> (+35.05%)`	⬆️
...pulsar/broker/admin/impl/PersistentTopicsBase.java	`63.73% <100.00%> (+53.55%)`	⬆️
... and 2 more

... and 1423 files with indirect coverage changes

michaeljmarshall

The migration code looks great. I left several comments.

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

michaeljmarshall · 2023-06-09T04:47:54Z

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

+     * @param topicName
+     * @return CompletableFuture<Map<String, Set<AuthAction>>>
+     */
+    default CompletableFuture<Map<String, Set<AuthAction>>> getPermissionsAsync(TopicName topicName) {


One interesting way we could consider expanding this method is to add the role of the user requesting this operation. As it is currently implemented, the broker will first verify that the role is a tenant admin or a superuser and then will call this method. It seems simpler to defer that kind of checking to the authentication provider. However, we don't do that kind of thing for any of the other methods, so that might not be consistent with the current design. What do you think @Technoboy-?

Currently, my design and implementation are simple. It's only for Admin API usage.
For this getPermissionsAsync, it's only used for getPermissionsOnTopic in Admin
If we change getPermissionsAsync(TopicName topicName, String role), I think it's really match other methods in the AuthorizationProvider, but we don't need it right now.

~~Your design and implementation are correct for the built-in policy, but we also need to consider the external policy.~~

These methods are used to get all permissions on a topic or namespace, so don't need to pass the role and auth data.

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

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

...er/src/test/java/org/apache/pulsar/broker/transaction/buffer/TransactionBufferCloseTest.java

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

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

…rovider ### Motivation apache#20496 introduced a breaking change that if the custom authz provider does not implement the `removePermissionsAsync` method, the built-in admin will fail to delete a topic. It's because `BrokerService#deleteTopicAuthenticationWithRetry` calls `authorizationService.removePermissionsAsync`. ### Modifications To make it backward compatible, return a normal completed future in `removePermissionsAsync`. Verify the change by deleting a topic in `AuthorizationWithAuthDataTest#testAdmin`.

Technoboy- self-assigned this Jun 5, 2023

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

Technoboy- added area/broker ready-to-test labels Jun 5, 2023

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

lhotari requested review from michaeljmarshall and nodece June 5, 2023 17:33

michaeljmarshall reviewed Jun 5, 2023

View reviewed changes

Technoboy- added 3 commits June 6, 2023 10:31

add

41b4f8c

add

1979d4b

add

6fd6843

Technoboy- force-pushed the support-remove-permissions branch from bc599cc to 6fd6843 Compare June 6, 2023 02:35

fix

126aee8

Technoboy- marked this pull request as ready for review June 6, 2023 02:45

fix checkstyle issue

48088d7

Technoboy- added the area/authn label Jun 6, 2023

Technoboy- added 3 commits June 6, 2023 11:17

fix checkstyle issue

94c1296

remove

ee0dcb4

fix spotbugs issue

023781c

Technoboy- added 4 commits June 6, 2023 22:32

fix test

a0e2034

fix checkstyle issue

fba07e2

fix test

acf83a9

rollback modification for `internalRemovePartitionsTopicNoAutoCreatio…

5535cb7

…nDisableAsync`

michaeljmarshall reviewed Jun 9, 2023

View reviewed changes

address comment

0b283dc

Technoboy- requested a review from michaeljmarshall June 12, 2023 10:10

mattisonchao reviewed Jul 7, 2023

View reviewed changes

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

mattisonchao reviewed Jul 7, 2023

View reviewed changes

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

mattisonchao reviewed Jul 7, 2023

View reviewed changes

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

mattisonchao reviewed Jul 7, 2023

View reviewed changes

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

mattisonchao approved these changes Jul 7, 2023

View reviewed changes

Technoboy- merged commit e96b339 into apache:master Jul 10, 2023

BewareMyPower mentioned this pull request Jul 21, 2023

[fix][broker] Fix the built-in admin failed to delete a topic with a custom authz provider #20848

Merged

4 tasks

nodece mentioned this pull request Jul 24, 2023

[improve][broker] Pass role and auth data parameters to the get permission method #20861

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improve][broker] Support get/remove permissions for AuthorizationProvider #20496

[improve][broker] Support get/remove permissions for AuthorizationProvider #20496

Technoboy- commented Jun 5, 2023 •

edited

Loading

michaeljmarshall left a comment

Technoboy- commented Jun 6, 2023 •

edited

Loading

Technoboy- commented Jun 6, 2023

codecov-commenter commented Jun 7, 2023 •

edited

Loading

michaeljmarshall left a comment

michaeljmarshall Jun 9, 2023

Technoboy- Jun 10, 2023

nodece Jul 21, 2023 •

edited

Loading

[improve][broker] Support get/remove permissions for AuthorizationProvider #20496

[improve][broker] Support get/remove permissions for AuthorizationProvider #20496

Conversation

Technoboy- commented Jun 5, 2023 • edited Loading

Motivation

Modification

Documentation

michaeljmarshall left a comment

Choose a reason for hiding this comment

Technoboy- commented Jun 6, 2023 • edited Loading

Technoboy- commented Jun 6, 2023

codecov-commenter commented Jun 7, 2023 • edited Loading

Codecov Report

michaeljmarshall left a comment

Choose a reason for hiding this comment

michaeljmarshall Jun 9, 2023

Choose a reason for hiding this comment

Technoboy- Jun 10, 2023

Choose a reason for hiding this comment

nodece Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

Technoboy- commented Jun 5, 2023 •

edited

Loading

Technoboy- commented Jun 6, 2023 •

edited

Loading

codecov-commenter commented Jun 7, 2023 •

edited

Loading

nodece Jul 21, 2023 •

edited

Loading