Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Authorization] AuthorizationService should use provider's canLookupAsync method #11777

Merged
merged 1 commit into from
Sep 2, 2021

Conversation

michaeljmarshall
Copy link
Member

@michaeljmarshall michaeljmarshall commented Aug 25, 2021

Motivation

The AuthenticationService essentially implements the canLookupAsync method instead of relying on the provider's canLookupAsync method.

Before this change, the AuthenticationService was essentially calling provider.canConsume and provider.canProduce in order to determine if the role had sufficient permission to lookup. While the logic should be sufficient, it wasn't the correct implementation because the provider has a canLookup method that ought to be used.

Modifications

  • Modified AuthenticationService method canLookupAsync to first check if the role is a super user and then call the configured provider's canLookupAsync method. Note that this implementation follows the same paradigm as canProduceAsync and canConsumeAsync in the AuthenticationService.
  • Clean up the PulsarAuthorizationProvider implementation of canLookupAsync to improve readability.

Verifying this change

This change is already covered by existing tests, such as the AuthorizationTest.

Does this pull request potentially affect one of the following parts:

This test could result in a change of behavior for users that have implemented their own AuthorizationProvider. The PulsarAuthorizationProvider is not affected because its logic was equivalent before and after this change.

Documentation

For contributor

For this PR, do we need to update docs?

It's possible we'll want to update docs or to include a note about this fix for users who are upgrading. The main change in behavior is for users with a custom AuthorizationProvider.

For committer

For this PR, do we need to update docs?

  • If yes,

    • if you update docs in this PR, label this PR with the doc label.

    • if you plan to update docs later, label this PR with the doc-required label.

    • if you need help on updating docs, create a follow-up issue with the doc-required label.

  • If no, label this PR with the no-need-doc label and explain why.

@michaeljmarshall
Copy link
Member Author

/pulsarbot run-failure-checks

@Anonymitaet Anonymitaet added the doc-required Your PR changes impact docs and you will update later. label Aug 26, 2021
@Anonymitaet
Copy link
Member

@michaeljmarshall Thanks for your contribution. Please do not forget to update docs later. And you can ping me to review the docs, thanks.

@michaeljmarshall
Copy link
Member Author

/pulsarbot run-failure-checks

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@michaeljmarshall
Copy link
Member Author

@Anonymitaet, @eolivelli, @sijie - how do we document changes that correct existing behavior? This change will only affect users with custom AuthorizationProvider plugins, and even then, it might not change the behavior.

@Anonymitaet
Copy link
Member

@michaeljmarshall if it is a major change or affects how to use this feature, maybe you add a note about the previous usage and current changes here?

@michaeljmarshall
Copy link
Member Author

@michaeljmarshall if it is a major change or affects how to use this feature, maybe you add a note about the previous usage and current changes here?

@Anonymitaet - I don't classify this as a major change. It is a correction in behavior. I think it only needs to be documented in the release notes. @eolivelli - do you have any insight here?

@rdhabalia
Copy link
Contributor

it's not a major change or api change so, it won't require documentation.

@eolivelli eolivelli merged commit 1d5403e into apache:master Sep 2, 2021
@michaeljmarshall michaeljmarshall deleted the use-provider-canlookup branch September 2, 2021 05:54
@Anonymitaet Anonymitaet removed the doc-required Your PR changes impact docs and you will update later. label Oct 14, 2021
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
@github-actions
Copy link

@michaeljmarshall Please provide a correct documentation label for your PR.
Instructions see Pulsar Documentation Label Guide.

@merlimat merlimat added cherry-picked/branch-2.7 Archived: 2.7 is end of life cherry-picked/branch-2.8 Archived: 2.8 is end of life labels Jun 22, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants