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

SSO Support #1492

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

SSO Support #1492

wants to merge 27 commits into from

Conversation

fservida
Copy link
Contributor

@fservida fservida commented Jan 26, 2024

This PR implements support for SSO (#1490)

Currently very much a WIP, following points need investigation/development:

General

  • Dynamically hide SSO button in frontend if SSO not configured
  • Assign users default role of "Crawler"
    • OR implement setting to define default role (currently crawler)
    • OR pull role from a group (probably more complicated and could be done at later stage)
  • Assign users to organizations depending on users groups (currently defaults to default org).
  • Disable user ability to edit username, email and password (might require a database flag to indicate the user is from SSO)
  • Update documentation
  • Update values.yml with example values
  • Critical - Understand how to change the logic to allow for login from interface. Currently only Header based is working IF the login at the proxy is initiated beforehand and the Headers are already being sent. This is because the login is an ajax process and not a frontend process.

Header Based

  • Verify request IP matches trusted proxy (to be set in config.yml) - Might be hard given there is always an additional proxy in front (K8s NGINX ingress). Better to specify in doc to restrict direct access to K8s ingress only to the authenticating proxy.
  • Allow custom values for "X-REMOTE-USER", "X-REMOTE-EMAIL" and "X-REMOTE-GROUPS" to accomodate different proxy setups (to be set in config.yml)

OIDC Based

  • Allow custom values for all necessary fields (to be set in config.yml)
image

(Easiest way to test locally: https://microsoftedge.microsoft.com/addons/detail/modheader-modify-http-h/opgbiafapkbbnbnjcdomjaghbckfkglc)

@fservida
Copy link
Contributor Author

Lost in how to dynamically show/hide elements on login page, I've implemented one endpoint returning the available login methods, but not familiar enough with the frontend framework to know how to use that to hide/show the elements.
image

Was thinking a request should fire on page load, and depending on the result do the necessary edits to the interface. Inputs are welcome

@fservida fservida changed the title Header Based SSO Support SSO Support Jan 27, 2024
@fservida
Copy link
Contributor Author

SSO login is working fully, both based on header, but most importantly OIDC is now supported, opening a very wide range of options.

Missing for now:

  • ability to hide the buttons if SSO is not enabled
  • disabling user ability to edit personal info/password
  • potentially finding better labels for the buttons, or user configurable labels.

Grateful if you have inputs on those as well as general feedback :) @Shrinks99 @ikreymer

@Shrinks99
Copy link
Member

Shrinks99 commented Jan 31, 2024

Hey! Will discuss this with the team today, thank you for contributing!

If the user fields could be set to disabled for all SSO users and text added to the page "Some fields are managed by your organization." that would be great. I don't know much about SSO, would it be possible to provide users with a link to edit them in their institution's user management portal? The password section should probably be completely hidden for all SSO users as well.

Would we be able to configure more than one SSO provider? I generally like how Jstor handles this with a "find your institution" system. I assume this PR only supports adding one?


If SSO is configured, the SSO Log In button should be the primary button type. The default Log In button does need to stick around but it should be given the default button type.

@fservida
Copy link
Contributor Author

fservida commented Jan 31, 2024

Hey! Will discuss this with the team today, thank you for contributing!

Always welcome, thanks for the great software

If the user fields could be set to disabled for all SSO users and text added to the page "Some fields are managed by your organization." that would be great. I don't know much about SSO, would it be possible to provide users with a link to edit them in their institution's user management portal? The password section should probably be completely hidden for all SSO users as well.

Agree, just need to understand how to set a flag indicating the user is from SSO and propagate that to the frontend. Link to edit should also be doable.

Would we be able to configure more than one SSO provider? I generally like how Jstor handles this with a "find your institution" system. I assume this PR only supports adding one?

This I think might be much more complicated, the best way to handle it, for any organization hosting browsertrix-cloud and wanting to implement SSO, would be to do federated IDP with selection of the SSO provider on the IDP instead of the SP (browsertrix). This is actually very similar to how SSO is handled by Swiss universities, with a central IDP being the single point of contact, which will redirect as needed to the various Universities IDPs depending on affiliation.

If SSO is configured, the SSO Log In button should be the primary button type. The default Log In button does need to stick around but it should be given the default button type.

Agree, will do at the same time as dynamically showing the SSO buttons.

My main issue is now understanding the best way to pull variables to easily use across the interface from the backend.

@fservida fservida marked this pull request as ready for review February 2, 2024 16:59
@fservida
Copy link
Contributor Author

fservida commented Feb 2, 2024

I've implemented last changes, everything is working now, only missing documentation.

I've created the option to disable user invites (both frontend and backend); this is because if SSO is enabled and used to manage users, inviting users can create problems with users' org membership (they now depend on users groups), as well as having new users signing up with username/password instead of SSO.

Users can be assigned superuser role based on group membership, and can manage orgs as needed.

When SSO is enabled, it is potentially suggested to therefore activate the option to disable invites, and possibly also disabling password login altogether. All of this is optional though and can be tailored by the instance admin.

Maybe a better way to handle invitations/org memberships that does not create conflicts is possible, but I couldn't figure it out, I think current solution is still very good. Possibly to improve in future.

Please review and let me know what you think should be changed, especially on the frontend side, I feel like it was a lot of monkey patching to get what I needed, there is surely a cleaner way to implement all the checks I've done.

@Shrinks99 Shrinks99 requested a review from tw4l February 2, 2024 17:24
Copy link
Member

@Shrinks99 Shrinks99 left a comment

Choose a reason for hiding this comment

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

Reviewed docs and frontend code for lit localization. Have not tested locally.

Thanks for writing up this guide! Haven't tested it but seems pretty thourough. Have made a bunch of docs suggestions. Found two instances of un-localization-wrapped frontend text (happens to the best of us haha).

docs/deploy/sso.md Outdated Show resolved Hide resolved
docs/deploy/sso.md Outdated Show resolved Hide resolved
docs/deploy/sso.md Outdated Show resolved Hide resolved
docs/deploy/sso.md Outdated Show resolved Hide resolved
docs/deploy/sso.md Outdated Show resolved Hide resolved
docs/deploy/sso.md Outdated Show resolved Hide resolved
docs/deploy/sso.md Outdated Show resolved Hide resolved
docs/deploy/sso.md Outdated Show resolved Hide resolved
frontend/src/features/accounts/account-settings.ts Outdated Show resolved Hide resolved
frontend/src/features/accounts/account-settings.ts Outdated Show resolved Hide resolved
Copy link
Member

@Shrinks99 Shrinks99 left a comment

Choose a reason for hiding this comment

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

Hyphenation and casing check (I missed some before!)

frontend/src/pages/log-in-header.ts Outdated Show resolved Hide resolved
frontend/src/pages/log-in-oidc.ts Outdated Show resolved Hide resolved
frontend/src/pages/log-in-oidc.ts Outdated Show resolved Hide resolved
frontend/src/pages/log-in.ts Outdated Show resolved Hide resolved
frontend/src/pages/log-in.ts Outdated Show resolved Hide resolved
fservida and others added 2 commits February 27, 2024 10:48
Batch accepting review suggestions, thanks @Shrinks99

Co-authored-by: Henry Wilkinson <[email protected]>
Appliying one missed one suggestion

Co-authored-by: Henry Wilkinson <[email protected]>
@fservida
Copy link
Contributor Author

fservida commented Feb 27, 2024

@Shrinks99 committed your suggestions, thanks.
Unsure why the K3D run is failing, I'm not getting that Helm template error when running on my laptop or even test infrastructure, might want to check if there are no compatibility issues in the template changes I had made before merging (not a Kubernetes/Helm expert personally).

@Shrinks99 Shrinks99 self-requested a review March 3, 2024 05:18
@holnburger
Copy link

Is this still being considered for merging? Would be interested in OIDC implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants