[fix][broker] Fix MultiRoles token provider NPE when using anonymous clients

[Technoboy-]

Motivation

2023-10-24T01:39:33,477+0000 [pulsar-io-4-2] WARN  org.apache.pulsar.broker.service.ServerCnx - [/10.90.12.95:39414] Got exception java.lang.NullPointerException: Cannot invoke "org.apache.pulsar.broker.authentication.AuthenticationDataSource.hasDataFromCommand()" because "this.authData" is null
        at org.apache.pulsar.broker.authentication.AuthenticationDataSubscription.hasDataFromCommand(AuthenticationDataSubscription.java:37)
        at org.apache.pulsar.broker.authorization.MultiRolesTokenAuthorizationProvider.getRoles(MultiRolesTokenAuthorizationProvider.java:153)
        at org.apache.pulsar.broker.authorization.MultiRolesTokenAuthorizationProvider.authorize(MultiRolesTokenAuthorizationProvider.java:201)

Modification

• Fix the NPE when consume.
• Check if the role is super-user when using MultiRolesTokenAuthorizationProvider

Documentation

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

[codecov-commenter]

Codecov Report

Merging #21429 (a00ed88) into master (618aede) will increase coverage by 43.10%.
Report is 9 commits behind head on master.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##             master   #21429       +/-   ##
=============================================
+ Coverage     30.15%   73.26%   +43.10%     
- Complexity      324    32575    +32251     
=============================================
  Files          1709     1888      +179     
  Lines        130685   140292     +9607     
  Branches      14245    15417     +1172     
=============================================
+ Hits          39414   102781    +63367     
+ Misses        85327    29424    -55903     
- Partials       5944     8087     +2143     

Flag	Coverage Δ
inttests	24.22% <0.00%> (+0.07%)	⬆️
systests	24.79% <0.00%> (+0.06%)	⬆️
unittests	72.54% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...authentication/AuthenticationDataSubscription.java	64.28% <100.00%> (+18.13%)	⬆️
...rization/MultiRolesTokenAuthorizationProvider.java	60.19% <100.00%> (+60.19%)	⬆️

... and 1508 files with indirect coverage changes

[lhotari]

relates to #21338

[nodece]

Why not you check the authData in the constructor or override methods?

    @Override
    public boolean hasDataFromTls() {
        return authData != null && authData.hasDataFromTls();
    }

We should avoid type checking.

(authData instanceof AuthenticationDataSubscription && ((AuthenticationDataSubscription) authData).getAuthData() == null)

[Technoboy-]

But the getRoles method will return empty

[nodece]

I have communicated with @Technoboy- T offline. I don't suggest that add getAuthData() method, and makes the code difficult to maintain.

The following are my ideas:

• idea 1
  Add a new AuthenticationDataSource for the anonymous role, and then check the authData type.

• idea 2
  Change the org.apache.pulsar.broker.authorization.MultiRolesTokenAuthorizationProvider#getRoles logic to quickly return role when role equals anonymous role:

    private Set<String> getRoles(String role, AuthenticationDataSource authData) {
        if (authData == null || role.equals(conf.getAnonymousUserRole())) {
            return Collections.singleton(role);
        }

There will be a pitfall here, if the client role is equal to the anonymous role, the real roles cannot be obtained from authData.

[Technoboy-]

idea 1 is more pretty, I will create a issue to track this.