fix authorization not working when subscription permission is empty

[wuzhanpeng]

Motivation

This PR fixed the problem that org.apache.pulsar.broker.authorization.AuthorizationProvider#canConsumeAsync returned true when org.apache.pulsar.common.policies.data.AuthPolicies#getSubscriptionAuthentication was empty (i.e. the set of subscription_auth_roles in admin policies is empty). This is equivalent to allowing subscription permissions not to be checked during authorization.

Modifications

• modifies canConsumeAsync of PulsarAuthorizationProvider and its relative test case.

[wuzhanpeng]

@rdhabalia @hangc0276
Could you help review these changes ?

[eolivelli]

This looks like a behaviour change.
You we advertise this in the release notes ?
Will existing users be affected by this change ?

[wuzhanpeng]

This looks like a behaviour change.
You we advertise this in the release notes ?
Will existing users be affected by this change ?

1. for regular users who enable authorization, they will grant topic and subscription permissions before actually consuming messages, thus these users would not be affected.
2. for those noncompliant users who never grant subscription permissions but still want authorization service, the subscriber would not be able to establish after this behavior takes effect.

[wuzhanpeng]

@hangc0276 @eolivelli @codelipenghui Could you help review these changes?

[hangc0276]

@hangc0276 @eolivelli @codelipenghui Could you help review these changes?

@wuzhanpeng This PR will break the existing subscription behavior although the existing behavior not expected. It will cause the existing applications which doesn't grant role and subscription permission, subscribe failed after upgrading Pulsar to the version including this PR. We'd better start a discussion in dev@pulsar.apache.org first.

[hangc0276]

move to 2.8.2

[michaeljmarshall]

+1 for moving to the dev mailing list.

[michaeljmarshall]

It appears that the original intent of this code was to consider an empty list as a special case that had special logic. I don't have enough context to know why, but it might be worth reviewing #2981, where this code was first introduced. If I had to guess, it's because it doesn't make much sense to have a subscription that cannot be consumed.

If this change is accepted, the comment on this line and the one above need to be updated:

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

Lines 119 to 120 in 879ab3a

	// validate if role is authorize to access subscription. (skip validatation if authorization
	// list is empty)

[315157973]

@wuzhanpeng Hello, is there an email address for discussion?

[github-actions]

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

[Anonymitaet]

@wuzhanpeng is this a bug fix (no need to update docs?)

[michaeljmarshall]

I am going to remove the 2.8 label for now. @wuzhanpeng do you plan to continue with this work, or should we close this PR?

[michaeljmarshall]

@wuzhanpeng - if you're still interested in making this change, I think this feature might be viable if we put it behind a feature flag. The underlying question for this PR is how to interpret a lack of configuration for a subscription. Given that null has already been chosen to implicitly mean that the subscription permission feature is ignored, I think we need to maintain that functionality. However, a cluster focused on strict security where no permission is granted implicitly will interpret a lack of configuration for a subscription to mean the role doesn't have permission.

Additionally, I notice that admin roles not included in the subscription's allow list will get rejected from consuming a subscription. It's worth discussing if admins get implicit permission to consume from a subscription. Note that admins will have permission to update the subscription at a namespace level with clearBacklog, but might not be able to clear an individual subscription backlog (this conclusion is based on my memory, so we'd need to verify it).

[michaeljmarshall]

I've created #15597 to make this behavior configurable. Please feel free to add your feedback on the mailing list thread: https://lists.apache.org/thread/x6zg2l7hrtopd0yty93fhctsnm9n0wbt.

[tisonkun]

Closed as stale and conflict. Please rebase and resubmit the patch if it's still relevant.