You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/10/27 22:00:54 UTC
[GitHub] [pulsar] michaeljmarshall opened a new pull request #12515: [Authorization] Use AuthorizationService#grantPermissionAsync to gran…
michaeljmarshall opened a new pull request #12515:
URL: https://github.com/apache/pulsar/pull/12515
…t topic permission
### Motivation
There are several motivating factors here.
1. The `AuthorizationProvider` interface has a method to grant permission for a role to a topic. However, that method is not currently used. The other methods in the interface for granting permission on namespaces and on subscriptions are used. This PR seeks to bring the implementation into alignment. Without this change, a custom authorization provider would not be able to create custom logic for topic level permissions.
2. The current implementation of`PulsarAuthorizationProvider#grantPermissionAsync(TopicName topicName, Set<AuthAction> actions, String role, String authDataJson)` is surprising. It currently sets permissions using the namespace method instead of setting the permissions at the topic level. This could result in granting more permission than intended. However, the method is not actually called right now, so this unexpected behavior is irrelevant.
### Modifications
* Move the logic for granting topic permissions to the `PulsarAuthorizationProvider`. This change closely resembles the existing code in the `NamespacesBase` class.
### Verifying this change
There are already tests that cover the granting of permission at a topic level. For example, `PersistentTopicsTest` tests this. Existing test coverage should be sufficient for validating this change.
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): no
- The public API: no
- The schema: no
- The default values of configurations: no
- The wire protocol: no
- The rest endpoints: no
- The admin cli options: no
- Anything that affects deployment: no
### Documentation
Check the box below and label this PR (if you have committer privilege).
Need to update docs?
- [ ] `doc-required`
(If you need help on updating docs, create a doc issue)
- [x] `no-need-doc`
No behavior is changed here. The fundamental change is to rely on the `AuthorizationProvider` interface when granting topic level permissions.
- [ ] `doc`
(If this PR contains doc changes)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #12515: [Authorization] Use AuthorizationService#grantPermissionAsync to gran…
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #12515:
URL: https://github.com/apache/pulsar/pull/12515#discussion_r738015302
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
##########
@@ -274,7 +275,39 @@ public void initialize(ServiceConfiguration conf, PulsarResources pulsarResource
@Override
public CompletableFuture<Void> grantPermissionAsync(TopicName topicName, Set<AuthAction> actions,
String role, String authDataJson) {
- return grantPermissionAsync(topicName.getNamespaceObject(), actions, role, authDataJson);
+ CompletableFuture<Void> result = new CompletableFuture<>();
+ try {
+ validatePoliciesReadOnlyAccess();
+ } catch (Exception e) {
+ result.completeExceptionally(e);
+ return result;
+ }
+
+ String topicUri = topicName.toString();
+ try {
+ pulsarResources.getNamespaceResources().setPolicies(topicName.getNamespaceObject(), policies -> {
Review comment:
Great catch. It looks like `grantPermissionAsync(NamespaceName namespaceName, Set<AuthAction> actions, String role, String authDataJson)` has the same issue. I'll follow up with an update to that method separately. Further, I'd like to update the calling methods to be asynchronous instead of calling `.get` on the future.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #12515: [Authorization] Use AuthorizationService#grantPermissionAsync to gran…
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #12515:
URL: https://github.com/apache/pulsar/pull/12515
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #12515: [Authorization] Use AuthorizationService#grantPermissionAsync to gran…
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #12515:
URL: https://github.com/apache/pulsar/pull/12515#discussion_r738021805
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
##########
@@ -274,7 +275,39 @@ public void initialize(ServiceConfiguration conf, PulsarResources pulsarResource
@Override
public CompletableFuture<Void> grantPermissionAsync(TopicName topicName, Set<AuthAction> actions,
String role, String authDataJson) {
- return grantPermissionAsync(topicName.getNamespaceObject(), actions, role, authDataJson);
+ CompletableFuture<Void> result = new CompletableFuture<>();
+ try {
+ validatePoliciesReadOnlyAccess();
+ } catch (Exception e) {
+ result.completeExceptionally(e);
+ return result;
+ }
+
+ String topicUri = topicName.toString();
+ try {
+ pulsarResources.getNamespaceResources().setPolicies(topicName.getNamespaceObject(), policies -> {
Review comment:
I've updated it to use the same logic found in the `updateSubscriptionPermissionAsync` method.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #12515: [Authorization] Use AuthorizationService#grantPermissionAsync to gran…
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #12515:
URL: https://github.com/apache/pulsar/pull/12515#issuecomment-977202526
@merlimat - PTAL. Sorry, I pushed with force, so the history is gone. It's mostly a small change though, so hopefully it isn't too hard to review. Thanks!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #12515: [Authorization] Use AuthorizationService#grantPermissionAsync to gran…
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #12515:
URL: https://github.com/apache/pulsar/pull/12515#issuecomment-988453226
@merlimat, @eolivelli, @codelipenghui, @freeznet, @lhotari - PTAL
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #12515: [Authorization] Use AuthorizationService#grantPermissionAsync to gran…
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #12515:
URL: https://github.com/apache/pulsar/pull/12515#issuecomment-953520355
@merlimat - it looks like the `AuthorizationProvider` and the `AuthorizationService` have some out of date Javadocs. For example, the `AuthroizationProvider` has this one:
```java
/**
* Grant authorization-action permission on a topic to the given client
*
* @param topicName
* @param role
* @param authDataJson
* additional authdata in json format
* @return CompletableFuture
* @completesWith <br/>
* IllegalArgumentException when namespace not found<br/>
* IllegalStateException when failed to grant permission
*/
CompletableFuture<Void> grantPermissionAsync(TopicName topicName, Set<AuthAction> actions, String role,
String authDataJson);
```
Based on inspecting the `setPoliciesAsync` method, it looks like the expected exceptions are more specific. I see that exceptions are expected to be of type `MetadataStoreException` when there are issues like a namespace not being found or a concurrent modification exception. Of course other errors can happen too. I am assuming this all stems back to the work for PIP-45. What is your recommendation for the right way to reconcile the behavior described above and the new behaviors introduced in PIP-45? My main thought is that this could introduce a breaking change in behavior for any users that have implemented their own `AuthorizationProvider`. (Technically, there is already a change here because this `grantPermissionAsync` method wasn't called by the `AuthorizationService`.) The problem is the same for the variant of this method that updates the permissions on the whole namespace, so we'll need to solve for that as well.
Since we're already fully in on PIP-45, I think we should just update these Javadocs and make sure that the changes are mentioned in the release notes for 2.10.
Let me know how you think we should proceed. I'm happy to open up a PR to update the Javadocs.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #12515: [Authorization] Use AuthorizationService#grantPermissionAsync to gran…
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #12515:
URL: https://github.com/apache/pulsar/pull/12515#discussion_r737880636
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
##########
@@ -274,7 +275,39 @@ public void initialize(ServiceConfiguration conf, PulsarResources pulsarResource
@Override
public CompletableFuture<Void> grantPermissionAsync(TopicName topicName, Set<AuthAction> actions,
String role, String authDataJson) {
- return grantPermissionAsync(topicName.getNamespaceObject(), actions, role, authDataJson);
+ CompletableFuture<Void> result = new CompletableFuture<>();
+ try {
+ validatePoliciesReadOnlyAccess();
+ } catch (Exception e) {
+ result.completeExceptionally(e);
+ return result;
+ }
+
+ String topicUri = topicName.toString();
+ try {
+ pulsarResources.getNamespaceResources().setPolicies(topicName.getNamespaceObject(), policies -> {
Review comment:
the `setPolicies()` would be a blocking call here. Instead we should use `setPoliciesAsync()` and use that future.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org