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] Return if namespace policies are read only #12514

Conversation

michaeljmarshall
Copy link
Member

@michaeljmarshall michaeljmarshall commented Oct 27, 2021

Motivation

This is a minor fix to a flow control in the default PulsarAuthorizationProvider class. Currently, when the policies are "read only", the grantPermissionAsync method will complete the future with a failure but then proceed to attempt to update the policy anyway.

Modifications

  • Return from the method early when policies are marked as read only.

Verifying this change

This section of the code does not have good test coverage, so it wouldn't be very easy to add a test for this small case. Since this fix is small and easy to understand, I think it is safe to merge it without adding a test. Ideally, we'll come back and add tests for this part of the code base.

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?

  • no-need-doc

It is a small change to internal behavior.

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 27, 2021
@michaeljmarshall
Copy link
Member Author

@rdhabalia, @lhotari, @merlimat - PTAL

@michaeljmarshall
Copy link
Member Author

/pulsarbot run-failure-checks

@github-actions
Copy link

@michaeljmarshall:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@michaeljmarshall
Copy link
Member Author

@merlimat - given our discussion on #12515, I am going to close this PR and follow up with a new one that is asynchronous. It will solve the underlying issue for this PR and make the grantPermissionAsync method truly asynchronous.

@michaeljmarshall
Copy link
Member Author

In order to cherry-pick this commit to older branches without also affecting the asynchronous code, I am going to re-open this PR. I'll submit a follow up that will make this method actually async.

@michaeljmarshall michaeljmarshall added area/security type/bug The PR fixed a bug or issue reported a bug type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use labels Jan 21, 2022
@michaeljmarshall michaeljmarshall added this to the 2.10.0 milestone Jan 21, 2022
@michaeljmarshall michaeljmarshall 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 cherry-picked/branch-2.9 Archived: 2.9 is end of life labels Jan 21, 2022
@michaeljmarshall michaeljmarshall changed the title [Authorization Provider] Return early when namespace policies are rea… [Authorization] Return if namespace policies are read only Jan 21, 2022
@eolivelli
Copy link
Contributor

Why dis you add the 'cherry picked' labels?
Did you cherry pick this change or are you planning to do so?

@michaeljmarshall michaeljmarshall removed cherry-picked/branch-2.7 Archived: 2.7 is end of life cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life labels Jan 21, 2022
@michaeljmarshall
Copy link
Member Author

@eolivelli - thanks for your question. I meant to put release labels, but instead used the cherry-picked label. I fixed it now. I'll merge this when tests pass and then I'll cherry pick to the old branches.

@michaeljmarshall michaeljmarshall merged commit f1e72d6 into apache:master Jan 21, 2022
@michaeljmarshall michaeljmarshall deleted the fix-authz-provider-behavior-for-read-only-case branch January 21, 2022 23:08
michaeljmarshall added a commit that referenced this pull request Jan 21, 2022
* [Authorization Provider] Return early when namespace policies are read only

* Remove typo fix to simplify cherry-picking

(cherry picked from commit f1e72d6)
@michaeljmarshall michaeljmarshall added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Jan 21, 2022
michaeljmarshall added a commit that referenced this pull request Jan 21, 2022
* [Authorization Provider] Return early when namespace policies are read only

* Remove typo fix to simplify cherry-picking

(cherry picked from commit f1e72d6)
@michaeljmarshall michaeljmarshall added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Jan 21, 2022
michaeljmarshall added a commit that referenced this pull request Jan 24, 2022
* [Authorization Provider] Return early when namespace policies are read only

* Remove typo fix to simplify cherry-picking

(cherry picked from commit f1e72d6)
@michaeljmarshall michaeljmarshall added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security cherry-picked/branch-2.7 Archived: 2.7 is end of life cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life doc-not-needed Your PR changes do not impact docs release/2.7.5 release/2.8.3 release/2.9.2 type/bug The PR fixed a bug or issue reported a bug type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants