[Authorization] Use AuthorizationService#grantPermissionAsync to gran…

[michaeljmarshall]

…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 ofPulsarAuthorizationProvider#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)

• 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)

[merlimat]

the setPolicies() would be a blocking call here. Instead we should use setPoliciesAsync() and use that future.

[michaeljmarshall]

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.

[michaeljmarshall]

I've updated it to use the same logic found in the updateSubscriptionPermissionAsync method.

[michaeljmarshall]

@merlimat - it looks like the AuthorizationProvider and the AuthorizationService have some out of date Javadocs. For example, the AuthroizationProvider has this one:

    /**
     * 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.

[michaeljmarshall]

@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!

[michaeljmarshall]

@merlimat, @eolivelli, @codelipenghui, @freeznet, @lhotari - PTAL