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.

Sign up for GitHub

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

Jump to bottom

[Broker] Remove tenant permission verification when list partitioned-topic #13138

Merged
merged 3 commits into from Dec 14, 2021
Merged

[Broker] Remove tenant permission verification when list partitioned-topic #13138

merged 3 commits into from Dec 14, 2021

Conversation

yuruguo
Copy link
Contributor

@yuruguo yuruguo commented Dec 5, 2021 •
edited

Motivation

We should not verify tenant permission when get partitioned-topic list, in fact, role with consumption permissions of namespace can get it.

Modifications

  • delete validateAdminAccessForTenant(namespaceName.getTenant()); in PersistentTopicsBase#internalGetPartitionedTopicList

Documentation

  • no-need-doc

@github-actions
Copy link

github-actions bot commented Dec 5, 2021

@yuruguo: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)

@github-actions
Copy link

github-actions bot commented Dec 5, 2021

@yuruguo:Thanks for providing doc info!

@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Dec 5, 2021
codelipenghui
codelipenghui reviewed Dec 6, 2021
View reviewed changes
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

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

The change LGTM, just left a comment about the test.

pulsar-broker/src/test/java/org/apache/pulsar/client/api/AuthorizationProducerConsumerTest.java Show resolved Hide resolved
@codelipenghui codelipenghui added this to the 2.10.0 milestone Dec 6, 2021
michaeljmarshall
michaeljmarshall requested changes Dec 7, 2021
View reviewed changes
Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

After second thought, I think this one might need a little more attention. I think I agree that you shouldn't need tenant admin permissions here, but the annotation on the endpoints that call this method indicate that the role should be an admin. It'd be worth looking at the history for these methods to see why that level of permission is required. It might also be worth sending a note to the dev@ mailing list since this is a change the affects security and authorization.

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
@@ -172,7 +172,6 @@
}

protected List<String> internalGetPartitionedTopicList() {
validateAdminAccessForTenant(namespaceName.getTenant());
Copy link
Member

Choose a reason for hiding this comment

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

If we are going to remove this line, we should update the annotations on the two methods that call internalGetPartitionedTopicList().

Copy link
Contributor Author

@yuruguo yuruguo Dec 8, 2021 •
edited

Choose a reason for hiding this comment

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

Nice!
In addition, we should also update the annotations on the method that call internalGetList().

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

Lines 149 to 150 in 01d73aa

protected List<String> internalGetList() {
validateNamespaceOperation(namespaceName, NamespaceOperation.GET_TOPICS);

Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

eolivelli
eolivelli approved these changes Dec 9, 2021
View reviewed changes
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
this is very good.

Probably we should do this also for "produce" permissions

@codelipenghui
Copy link
Contributor

@michaeljmarshall Could you please help take a look at this PR?

@michaeljmarshall michaeljmarshall merged commit 4988108 into apache:master Dec 14, 2021
michaeljmarshall pushed a commit that referenced this pull request Dec 14, 2021
@yuruguo @michaeljmarshall
[Broker] Remove tenant admin verification when listing partitioned to…
547526a 
…pics (#13138)

* [Broker] Remove tenant permission verification when list partitioned-topic

* Improve test

* Update annotation

(cherry picked from commit 4988108)
@michaeljmarshall michaeljmarshall added cherry-picked/branch-2.9 Archived: 2.9 is end of life and removed release/2.9.2 labels Dec 14, 2021
michaeljmarshall pushed a commit that referenced this pull request Dec 14, 2021
@yuruguo @michaeljmarshall
[Broker] Remove tenant admin verification when listing partitioned to…
755d193 
…pics (#13138)

* [Broker] Remove tenant permission verification when list partitioned-topic

* Improve test

* Update annotation

(cherry picked from commit 4988108)
@michaeljmarshall michaeljmarshall added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Dec 14, 2021
fxbing pushed a commit to fxbing/pulsar that referenced this pull request Dec 19, 2021
@yuruguo
[Broker] Remove tenant admin verification when listing partitioned to…
ddf6312 
…pics (apache#13138)

* [Broker] Remove tenant permission verification when list partitioned-topic

* Improve test

* Update annotation
@yuruguo yuruguo mentioned this pull request Jan 16, 2022
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eolivelli eolivelli eolivelli approved these changes

@315157973 315157973 315157973 approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@shoothzj shoothzj shoothzj approved these changes

@michaeljmarshall michaeljmarshall michaeljmarshall approved these changes

@gaoran10 gaoran10 Awaiting requested review from gaoran10

@hangc0276 hangc0276 Awaiting requested review from hangc0276

@merlimat merlimat Awaiting requested review from merlimat

Assignees

@yuruguo yuruguo

Labels
area/broker area/security 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.8.3 release/2.9.1
Projects
None yet
Milestone
2.10.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@yuruguo @codelipenghui @eolivelli @315157973 @shoothzj @michaeljmarshall @gaoran10