Skip to content

[improve][test] Add topic operation checker for topic API #22468

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
Merged
Technoboy- merged 1 commit into from
Apr 15, 2024
Merged

[improve][test] Add topic operation checker for topic API #22468

Technoboy- merged 1 commit into from
Apr 15, 2024

Conversation

@coderzc
Copy link
Member

@coderzc coderzc commented Apr 10, 2024 •
edited
Loading

Motivation

Currently, the permission test of topic API only verifies the AuthAction but does not verify whether the topic operation meets expectations. This is a guarantee for other authorizationProvider implementations.

Modifications

Add checker topic operation for topic API

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

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

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 10, 2024
@coderzc coderzc force-pushed the admin_auth_test_topic_operation branch from 043ba77 to 2f437e0 Compare April 10, 2024 02:11
@coderzc coderzc requested review from Technoboy- and liangyepianzhou and removed request for Technoboy- April 10, 2024 02:13
@lhotari
Copy link
Member

lhotari commented Apr 10, 2024

Add checker topic operation for topic API

Please provide more context. What is "checker topic operation" ?

@coderzc coderzc changed the title [improve][test] Add checker topic operation for topic API [improve][test] Add topic operation checker for topic API Apr 10, 2024
@coderzc
Copy link
Member Author

coderzc commented Apr 10, 2024

Add checker topic operation for topic API

Please provide more context. What is "checker topic operation" ?

@lhotari I added some context in the PR description

@coderzc coderzc requested a review from Technoboy- April 10, 2024 10:08
@coderzc coderzc self-assigned this Apr 10, 2024
@lhotari
Copy link
Member

lhotari commented Apr 10, 2024

Currently, the permission test of topic API only verifies the AuthAction but does not verify whether the topic operation meets expectations. This is a guarantee for other authorizationProvider implementations.

@coderzc Thanks. Unfortunately, this leaves many open questions.

  • About "whether the topic operation meets expectations".
    • Q: What "expectations"? Please define.
  • "This is a guarantee for other authorizationProvider implementations."
    • What is "this is a guarantee" ? What "guarantee". What "other authorization provider implementations"?

A representative example could clarify what this all means.

@coderzc
Copy link
Member Author

coderzc commented Apr 10, 2024 •
edited
Loading

@lhotari Let me use an example to explain why this PR is needed.

Such as for SkipMessage API we should check whether the user has the permission for the SKIP operation, but in the default implementation of PulsarAuthorizationProvider we convert it into the AuthAction.consume permission, see:

pulsar/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java

Lines 560 to 595 in 6de711d

public CompletableFuture<Boolean> allowTopicOperationAsync(TopicName topicName,
String role,
TopicOperation operation,
AuthenticationDataSource authData) {
if (log.isDebugEnabled()) {
log.debug("Check allowTopicOperationAsync [{}] on [{}].", operation.name(), topicName);
}
return validateTenantAdminAccess(topicName.getTenant(), role, authData)
.thenCompose(isSuperUserOrAdmin -> {
if (log.isDebugEnabled()) {
log.debug("Verify if role {} is allowed to {} to topic {}: isSuperUserOrAdmin={}",
role, operation, topicName, isSuperUserOrAdmin);
}
if (isSuperUserOrAdmin) {
return CompletableFuture.completedFuture(true);
} else {
switch (operation) {
case LOOKUP:
case GET_STATS:
case GET_METADATA:
return canLookupAsync(topicName, role, authData);
case PRODUCE:
return canProduceAsync(topicName, role, authData);
case GET_SUBSCRIPTIONS:
case CONSUME:
case SUBSCRIBE:
case UNSUBSCRIBE:
case SKIP:
case EXPIRE_MESSAGES:
case PEEK_MESSAGES:
case RESET_CURSOR:
case GET_BACKLOG_SIZE:
case SET_REPLICATED_SUBSCRIPTION_STATUS:
case GET_REPLICATED_SUBSCRIPTION_STATUS:
return canConsumeAsync(topicName, role, authData, authData.getSubscription());
However, in the customized AuthorizationProvider implementation, the user may It has its own authentication logic, the customized AuthorizationProvider wants operation parameters passed to the allowTopicOperationAsync method is SKIP instead of other operation (such as CONSUME). In order to avoid code regression, I added an operation check to ensure API verified topic operation is expected.

@Technoboy- Technoboy- added this to the 3.3.0 milestone Apr 15, 2024
@Technoboy- Technoboy- closed this Apr 15, 2024
@Technoboy- Technoboy- reopened this Apr 15, 2024
@Technoboy- Technoboy- merged commit 9d72e6b into apache:master Apr 15, 2024
hanmz pushed a commit to hanmz/pulsar that referenced this pull request Feb 12, 2025
@coderzc @hanmz
[improve][test] Add topic operation checker for topic API (apache#22468)
cdbe75f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Technoboy- Technoboy- Technoboy- approved these changes

@liangyepianzhou liangyepianzhou Awaiting requested review from liangyepianzhou

Assignees

@coderzc coderzc

Labels

doc-not-needed Your PR changes do not impact docs ready-to-test

Projects

None yet

Milestone

3.3.0

Development

Successfully merging this pull request may close these issues.

3 participants

@coderzc @lhotari @Technoboy-