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] Support UNSUBSCRIBE namespace op after enable auth #12742

Merged
merged 1 commit into from
Nov 12, 2021
Merged

[Authorization] Support UNSUBSCRIBE namespace op after enable auth #12742

merged 1 commit into from
Nov 12, 2021

Conversation

yuruguo
Copy link
Contributor

@yuruguo yuruguo commented Nov 11, 2021

Motivation

Currently, we can unsubscribe the given subscription on all topics on a namespace through bin/pulsar-admin namespaces unsubscribe -s sub tn1/ns1. However, role(not super-user or administrator) with consume auth action for namespace cannot perform unsubscribe operation when enable auth.

The root of the problem is that PulsarAuthorizationProvider lacks support for namespace operation UNSUBSCRIBE when verifying the role's authorization, code as below:

protected void internalUnsubscribeNamespace(AsyncResponse asyncResponse, String subscription,
boolean authoritative) {
validateNamespaceOperation(namespaceName, NamespaceOperation.UNSUBSCRIBE);

public CompletableFuture<Boolean> allowNamespaceOperationAsync(NamespaceName namespaceName,
String role,
NamespaceOperation operation,
AuthenticationDataSource authData) {
CompletableFuture<Boolean> isAuthorizedFuture;
switch (operation) {
case PACKAGES:
isAuthorizedFuture = allowTheSpecifiedActionOpsAsync(namespaceName, role, authData, AuthAction.packages);
break;
case GET_TOPICS:
isAuthorizedFuture = allowConsumeOpsAsync(namespaceName, role, authData);
break;
default:
isAuthorizedFuture = CompletableFuture.completedFuture(false);
}

The purpose of this PR is to support that role with consume namespace authorization could unsubscribe subscriptions on a namespace.

Documentation

  • no-need-doc

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 11, 2021
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.

The change looks good

I believe that an integration test is too much heavyweight for this code.

Can we add a test somewhere in a simple unit test please ?

@yuruguo
Copy link
Contributor Author

yuruguo commented Nov 11, 2021

The change looks good

I believe that an integration test is too much heavyweight for this code.

Can we add a test somewhere in a simple unit test please ?

Okay :)
I have move the test of this PR and previous PR to pulsar-broker module, it wii be lighter.

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

@codelipenghui codelipenghui added this to the 2.10.0 milestone Nov 12, 2021
@codelipenghui codelipenghui merged commit 8926631 into apache:master Nov 12, 2021
codelipenghui pushed a commit that referenced this pull request Nov 18, 2021
…12742)

### Motivation
Currently, we can `unsubscribe` the given subscription on all topics on a namespace through `bin/pulsar-admin namespaces unsubscribe -s sub tn1/ns1`. However, role(not super-user or administrator) with `consume` auth action for namespace cannot perform `unsubscribe` operation when enable auth.

The root of the problem is that `PulsarAuthorizationProvider` lacks support for namespace operation `UNSUBSCRIBE` when verifying the role's authorization, code as below:
https://github.com/apache/pulsar/blob/8cae63557a318240e95697f382b4f61c22b70d64/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java#L1667-L1669
https://github.com/apache/pulsar/blob/8cae63557a318240e95697f382b4f61c22b70d64/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java#L522-L536

The purpose of this PR is to support that role with `consume` namespace authorization could `unsubscribe` subscriptions on a namespace.

(cherry picked from commit 8926631)
@codelipenghui codelipenghui added cherry-picked/branch-2.8 Archived: 2.8 is end of life release/2.8.2 and removed release/2.8.3 labels Nov 18, 2021
eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request Nov 29, 2021
…pache#12742)

### Motivation
Currently, we can `unsubscribe` the given subscription on all topics on a namespace through `bin/pulsar-admin namespaces unsubscribe -s sub tn1/ns1`. However, role(not super-user or administrator) with `consume` auth action for namespace cannot perform `unsubscribe` operation when enable auth.

The root of the problem is that `PulsarAuthorizationProvider` lacks support for namespace operation `UNSUBSCRIBE` when verifying the role's authorization, code as below: 
https://github.com/apache/pulsar/blob/8cae63557a318240e95697f382b4f61c22b70d64/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java#L1667-L1669
https://github.com/apache/pulsar/blob/8cae63557a318240e95697f382b4f61c22b70d64/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java#L522-L536

The purpose of this PR is to support that role with `consume` namespace authorization could `unsubscribe` subscriptions on a namespace.
michaeljmarshall pushed a commit that referenced this pull request Dec 6, 2021
…12742)

### Motivation
Currently, we can `unsubscribe` the given subscription on all topics on a namespace through `bin/pulsar-admin namespaces unsubscribe -s sub tn1/ns1`. However, role(not super-user or administrator) with `consume` auth action for namespace cannot perform `unsubscribe` operation when enable auth.

The root of the problem is that `PulsarAuthorizationProvider` lacks support for namespace operation `UNSUBSCRIBE` when verifying the role's authorization, code as below:
https://github.com/apache/pulsar/blob/8cae63557a318240e95697f382b4f61c22b70d64/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java#L1667-L1669
https://github.com/apache/pulsar/blob/8cae63557a318240e95697f382b4f61c22b70d64/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java#L522-L536

The purpose of this PR is to support that role with `consume` namespace authorization could `unsubscribe` subscriptions on a namespace.

(cherry picked from commit 8926631)
@michaeljmarshall michaeljmarshall added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Dec 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker 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.2 release/2.9.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants