[fix][broker] Fix auth test cases.

[dragonls]

Master Issue: #20068

Motivation

Fix auth test cases. The tenant admin should have all permissions of the topics.

In current master branch, role1 is the tenant admin in org.apache.pulsar.broker.auth.AuthorizationTest#simple, and should have all permissions of the topics.

pulsar/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/AuthorizationTest.java

Lines 220 to 223 in 575cf23

	try {
	assertFalse(auth.canConsume(TopicName.get("persistent://p1/c1/ns1/ds1"), "role1", null, "sub1"));
	fail();
	} catch (Exception ignored) {}

auth.canConsume(TopicName.get("persistent://p1/c1/ns1/ds1"), "role1", null, "sub1") should be true.

Modifications

Add and fix the test in org.apache.pulsar.broker.auth.AuthorizationTest#simple.
Add and fix the test in org.apache.pulsar.websocket.proxy.ProxyAuthorizationTest.

Verifying this change

• Make sure that the change passes the CI checks.

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: dragonls#8

[github-actions]

@dragonls Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

[dragonls]

Hi, @michaeljmarshall
After revert #20068, do you have any suggestion to fix the test cases? Maybe be fixed by #20145.

[michaeljmarshall]

Hi, @michaeljmarshall After revert #20068, do you have any suggestion to fix the test cases? Maybe be fixed by #20145.

Are the modified tests failing in master?

[dragonls]

Hi, @michaeljmarshall After revert #20068, do you have any suggestion to fix the test cases? Maybe be fixed by #20145.

Are the modified tests failing in master?

Yes. Because this part of codes is wrong in master:

https://github.com/apache/pulsar/blob/master/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/AuthorizationTest.java#L220-L223

role1 is the tenant admin, and it should have the consume permission.

[michaeljmarshall]

That behavior only affects the WebSocket Proxy when subscription auth mode by prefix is in use. The test was added a while ago #899. I think we should migrate away from using those methods and then we can deprecate them. I don't see a need to change the behavior. Here is a PR to remove some of the final usages of the canConsume and canProduce methods #20299.

[dragonls]

Yes, the test was added long time ago. But it's still inconsistent with our conventional understanding, which means the behavior is different between WebSocket Proxy and others.

I once think about changing the code in PulsarAuthorizationProvider from
canProduceAsync -> checkAuthorization
to
canProduceAsync -> allowTopicOperationAsync -> innerCanProduceAsync -> checkAuthorization

But I am not sure whether this is the right way. My main purpose is to hope that all code logic meets expectations, including WebSocket Proxy as well as PulsarAuthorizationProvider itself.

[github-actions]

The pr had no activity for 30 days, mark with Stale label.