Skip to content
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.

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

Fix rest user info to match API auth with Webapp Auth #310

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

cghyzel
Copy link
Contributor

@cghyzel cghyzel commented Apr 9, 2024

Fixes the restUserInfo function to return Trino Gateway Roles (ADMIN, USER, API) to the Webapp, which is what the webapp expects.

Additional context and related issues

I opened this PR to map LDAP groups to Trino gateway roles and then came up with this simpler solution.

The following configs will break in the current (prior to this commit) version of Trino gateway because the admin user will not have admin privileges within the webapp like it should. This is because the webapp checks role membership by comparing the roles passed to those in an enum.

breaking auth example:

authorization:
    admin: .*FOO.*
    user: .*BAR.*
    api: .*BAZ.*

presetUsers:
    admin1:
        password: password
        privileges: FOO_BAR
    user1:
        password: password
        privileges: BAR_BAZ

This broken example works as expected after this fix.

This is how the webapp checks role membership:
https://github.com/trinodb/trino-gateway/blob/739ec5e67799dd5df0ab8aaec7934e21a63507fa/webapp/src/store/access.ts#L73C9-L76C9

And where those role membership checks are made:

{access.hasRole(Role.ADMIN) && (

{access.hasRole(Role.ADMIN) && (

<Form.Input field='user' label='User' initValue={form.user} disabled={!access.hasRole(Role.ADMIN)} style={{ width: 150 }} showClear />

{access.hasRole(Role.ADMIN) && (

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(X) Release notes are required, with the following suggested text:

* fixed userInfo resource to pass role information used by the api, so that webapp auth matches api auth

@cla-bot cla-bot bot added the cla-signed label Apr 9, 2024
@oneonestar
Copy link
Member

securityContext.isUserInRole() is actually calling this:

public boolean authorize(LbPrincipal principal,

authorization:
    admin: .*FOO.*
    user: .*BAR.*
    api: .*BAZ.*

presetUsers:
    admin1:
        password: password
        privileges: FOO_BAR
    user1:
        password: password
        privileges: BAR_BAZ

authentication:
    defaultType: "form"
    form:
        selfSignKeyPair:
            privateKeyRsa: docs/private.pem
            publicKeyRsa: docs/public.pem

Before:
image
image

After:
image
image

Copy link
Member

@oneonestar oneonestar left a comment

Choose a reason for hiding this comment

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

LGTM.

nit: We should have a test for this.

Copy link
Member

@Chaho12 Chaho12 left a comment

Choose a reason for hiding this comment

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

@andythsu
Copy link
Member

andythsu commented Apr 9, 2024

securityContext.isUserInRole() is actually calling this:

public boolean authorize(LbPrincipal principal,

authorization:
    admin: .*FOO.*
    user: .*BAR.*
    api: .*BAZ.*

presetUsers:
    admin1:
        password: password
        privileges: FOO_BAR
    user1:
        password: password
        privileges: BAR_BAZ

authentication:
    defaultType: "form"
    form:
        selfSignKeyPair:
            privateKeyRsa: docs/private.pem
            publicKeyRsa: docs/public.pem

Before: image image

After: image image

good call!

Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

Please squash commits into one and follow the commit message guideline: https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#format-git-commit-messages

The restUserInfo method in the LoginResource class and
the findQueryHistory method in the GatewayWapResource
were using string manipulation of the getMemberOf method
from the LbPrincipal to do role authorization. This change
fixes the role authorization to be consistent throughout
the app by utilizing the isUserInRole method that keeps
the context of the LbAuthorization when checking user
access to the ADMIN, USER, and API roles.
@ebyhr ebyhr merged commit d4992c8 into trinodb:main Apr 10, 2024
2 checks passed
@github-actions github-actions bot added this to the 8 milestone Apr 10, 2024
@cghyzel cghyzel deleted the ldap_refactor branch April 24, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants