Skip to content

Conversation

@nodece
Copy link
Member

@nodece nodece commented Jul 18, 2022

Fixes #16574

Motivation

The MultiRolesTokenAuthorizationProvider cannot handle the superuser and tenant admin correctly. It only checks if the role passed in is a superuser, which is incorrect. The correct way is to get a role list from authentication data and then check if the role list contains a superuser.

Modifications

  • Override the isSuperAdmin
  • Override the validateTenantAdminAccess
  • Add test for the MultiRolesTokenAuthorizationProvider

Documentation

  • doc-not-needed

@nodece nodece requested a review from RobertIndie July 18, 2022 07:32
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jul 18, 2022
@nodece nodece self-assigned this Jul 18, 2022
@nodece nodece added cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 component/authorization type/bug The PR fixed a bug or issue reported a bug labels Jul 18, 2022
@nodece nodece removed cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 labels Jul 18, 2022
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@nodece nodece force-pushed the fix-multiple-roles-authz branch from 0d67b5d to 224ca1b Compare July 18, 2022 10:35
@nodece nodece requested a review from Technoboy- July 18, 2022 10:35
throw new RestException(Response.Status.NOT_FOUND, "Tenant does not exist");
}
log.error("Failed to get tenant {}", tenantName, cause);
throw new RestException(cause);
Copy link
Contributor

Choose a reason for hiding this comment

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

If line 129 throws RestException, will here wrap to new RestException(RestException ...) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

RestException can get the real exception.

public RestException(Throwable t) {
        super(getResponse(t));
    }

    private static Response getResponse(Throwable t) {
        if (t instanceof WebApplicationException) {
            WebApplicationException e = (WebApplicationException) t;
            return e.getResponse();
        } else {
            return Response
                .status(Status.INTERNAL_SERVER_ERROR)
                .entity(getExceptionData(t))
                .type(MediaType.TEXT_PLAIN)
                .build();
        }
    }

@nodece nodece requested a review from Technoboy- July 25, 2022 09:49
@Technoboy- Technoboy- merged commit d8483d4 into apache:master Jul 25, 2022
@BewareMyPower
Copy link
Contributor

This PR relies on the TenantResources#getTenantAsync method, which relies on the BaseResources#joinPath method that I tried to migrate in #16867. Could you help review that PR so that I can continue cherry-picking and resolving the conflicts?

@nodece
Copy link
Member Author

nodece commented Aug 1, 2022

@BewareMyPower Done.

@nodece nodece deleted the fix-multiple-roles-authz branch August 1, 2022 10:01
BewareMyPower pushed a commit that referenced this pull request Aug 5, 2022
@BewareMyPower BewareMyPower added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Aug 5, 2022
codelipenghui pushed a commit that referenced this pull request Aug 8, 2022
@mattisonchao mattisonchao added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Aug 10, 2022
mattisonchao pushed a commit that referenced this pull request Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/authn cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 doc-not-needed Your PR changes do not impact docs release/2.8.4 release/2.9.4 release/2.10.2 type/bug The PR fixed a bug or issue reported a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MultiRolesTokenAuthorizationProvider only authorizes first role (at least for superuser)

8 participants