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 GET_METADATA topic op after enable auth #12656

Merged
merged 2 commits into from
Nov 8, 2021
Merged

[Authorization] Support GET_METADATA topic op after enable auth #12656

merged 2 commits into from
Nov 8, 2021

Conversation

yuruguo
Copy link
Contributor

@yuruguo yuruguo commented Nov 7, 2021

Motivation

Currently, we can get the internal stats of a topic through bin/pulsar-admin topics stats-internal tn1/ns1/tp1 and also get ledger metadata by specifying flag --metadata.

However I found that PulsarAuthorizationProvider lacks support for topic operation GET_METADATA when verifying the role's authorization, code as below:

if (metadata) {
validateTopicOperation(topicName, TopicOperation.GET_METADATA);
}

switch (operation) {
case LOOKUP:
case GET_STATS:
isAuthorizedFuture = canLookupAsync(topicName, role, authData);
break;
case PRODUCE:
isAuthorizedFuture = canProduceAsync(topicName, role, authData);
break;
case GET_SUBSCRIPTIONS:
case CONSUME:
case SUBSCRIBE:
case UNSUBSCRIBE:
case SKIP:
case EXPIRE_MESSAGES:
case PEEK_MESSAGES:
case RESET_CURSOR:
case SET_REPLICATED_SUBSCRIPTION_STATUS:
isAuthorizedFuture = canConsumeAsync(topicName, role, authData, authData.getSubscription());
break;
case TERMINATE:
case COMPACT:
case OFFLOAD:
case UNLOAD:
case ADD_BUNDLE_RANGE:
case GET_BUNDLE_RANGE:
case DELETE_BUNDLE_RANGE:
return validateTenantAdminAccess(topicName.getTenant(), role, authData);
default:
return FutureUtil.failedFuture(
new IllegalStateException("TopicOperation [" + operation.name() + "] is not supported."));

The purpose of this PR is to support role with lookup topic authorization to GET_METADATA of ledger.

Documentation

  • no-need-doc

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

@yuruguo Could you please help add a test? The change looks good to me.

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 please take a look as you are working on this topic during these days

@yuruguo
Copy link
Contributor Author

yuruguo commented Nov 7, 2021

@yuruguo Could you please help add a test? The change looks good to me.

Okay :)
Done, PTAL

Copy link
Contributor

@freeznet freeznet left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM. My one additional thought is that the added test is more complex than necessary, and that complexity will increase the time required to run this pretty simple test. I think there is a test Authentication Provider that would make it easier to test authorization without needing to explicitly test authentication, but I could be wrong.

@yuruguo yuruguo changed the title [Authorization] Fix not support GET_METADATA topic operation [Authorization] Support GET_METADATA topic operation after enable auth Nov 8, 2021
@yuruguo yuruguo changed the title [Authorization] Support GET_METADATA topic operation after enable auth [Authorization] Support GET_METADATA topic op after enable auth Nov 8, 2021
@codelipenghui codelipenghui merged commit e476735 into apache:master Nov 8, 2021
zeo1995 pushed a commit to zeo1995/pulsar that referenced this pull request Nov 9, 2021
* up/master:
  [Doc] Add explanations for setting geo-replication at topic level (apache#12633)
  commit chapter Tiered Storage (apache#12592)
  [pulsar-admin] Add remove-subscription-types-enabled command for namespace (apache#12392)
  Enable CLI to publish non-batched messages (apache#12641)
  [Doc] Add doc for tokenSettingPrefix (apache#12662)
  [pulsar-admin] Add corresponding get command for namespace (apache#12322)
  [pulsar-admin] Perfect judgment conditions of pulsar-admin (apache#12315)
  [broker] Avoid unnecessary recalculation of maxSubscriptionsPerTopic in AbstractTopic (apache#12658)
  [Transaction]Stop TB recovering with exception (apache#12636)
  [website][upgrade]feat: docs migration - 2.7.1 / client (apache#12612)
  [website][upgrade]feat: docs migration - 2.7.1 / performance (apache#12611)
  [website][upgrade]feat: docs migration - 2.7.1 / security (apache#12610)
  [Modernizer] Apply Modernizer plugin for pulsar broker common module and fix violation. (apache#12657)
  [Authorization] Support GET_METADATA topic op after enable auth (apache#12656)
  Fix StringIndexOutOfBoundsException in org.apache.pulsar.broker.resources.NamespaceResources#pathIsFromNamespace (apache#12659)
zeo1995 pushed a commit to zeo1995/pulsar that referenced this pull request Nov 9, 2021
* up/master:
  [Doc] Add explanations for setting geo-replication at topic level (apache#12633)
  commit chapter Tiered Storage (apache#12592)
  [pulsar-admin] Add remove-subscription-types-enabled command for namespace (apache#12392)
  Enable CLI to publish non-batched messages (apache#12641)
  [Doc] Add doc for tokenSettingPrefix (apache#12662)
  [pulsar-admin] Add corresponding get command for namespace (apache#12322)
  [pulsar-admin] Perfect judgment conditions of pulsar-admin (apache#12315)
  [broker] Avoid unnecessary recalculation of maxSubscriptionsPerTopic in AbstractTopic (apache#12658)
  [Transaction]Stop TB recovering with exception (apache#12636)
  [website][upgrade]feat: docs migration - 2.7.1 / client (apache#12612)
  [website][upgrade]feat: docs migration - 2.7.1 / performance (apache#12611)
  [website][upgrade]feat: docs migration - 2.7.1 / security (apache#12610)
  [Modernizer] Apply Modernizer plugin for pulsar broker common module and fix violation. (apache#12657)
  [Authorization] Support GET_METADATA topic op after enable auth (apache#12656)
  Fix StringIndexOutOfBoundsException in org.apache.pulsar.broker.resources.NamespaceResources#pathIsFromNamespace (apache#12659)

# Conflicts:
#	site2/website-next/versioned_sidebars/version-2.7.2-sidebars.json
eolivelli pushed a commit that referenced this pull request Nov 9, 2021
### Motivation
Currently, we can get the internal stats of a topic through `bin/pulsar-admin topics stats-internal tn1/ns1/tp1` and also get ledger metadata by specifying flag `--metadata`.

However I found that `PulsarAuthorizationProvider` lacks support for topic operation `GET_METADATA` when verifying the role's authorization, code as below:
https://github.com/apache/pulsar/blob/08a49c06bff4a52d26319a114961aed6cb6c4791/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java#L1162-L1164
https://github.com/apache/pulsar/blob/08a49c06bff4a52d26319a114961aed6cb6c4791/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java#L567-L596

The purpose of this PR is to support role with `lookup` topic authorization to `GET_METADATA` of ledger.

(cherry picked from commit e476735)
@eolivelli eolivelli modified the milestones: 2.10.0, 2.9.0 Nov 9, 2021
codelipenghui pushed a commit that referenced this pull request Nov 18, 2021
### Motivation
Currently, we can get the internal stats of a topic through `bin/pulsar-admin topics stats-internal tn1/ns1/tp1` and also get ledger metadata by specifying flag `--metadata`.

However I found that `PulsarAuthorizationProvider` lacks support for topic operation `GET_METADATA` when verifying the role's authorization, code as below:
https://github.com/apache/pulsar/blob/08a49c06bff4a52d26319a114961aed6cb6c4791/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java#L1162-L1164
https://github.com/apache/pulsar/blob/08a49c06bff4a52d26319a114961aed6cb6c4791/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java#L567-L596

The purpose of this PR is to support role with `lookup` topic authorization to `GET_METADATA` of ledger.

(cherry picked from commit e476735)
@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
…he#12656)

### Motivation
Currently, we can get the internal stats of a topic through `bin/pulsar-admin topics stats-internal tn1/ns1/tp1` and also get ledger metadata by specifying flag `--metadata`.

However I found that `PulsarAuthorizationProvider` lacks support for topic operation `GET_METADATA` when verifying the role's authorization, code as below: 
https://github.com/apache/pulsar/blob/08a49c06bff4a52d26319a114961aed6cb6c4791/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java#L1162-L1164
https://github.com/apache/pulsar/blob/08a49c06bff4a52d26319a114961aed6cb6c4791/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java#L567-L596

The purpose of this PR is to support role with `lookup` topic authorization to `GET_METADATA` of ledger.
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 doc-not-needed Your PR changes do not impact docs release/2.8.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants