galaxy icon indicating copy to clipboard operation
galaxy copied to clipboard

Split Login and Register, enable OIDC Registration.

Open uwwint opened this issue 7 months ago • 4 comments

This change introduces the ability to configure OIDC registration endpoints. In order to achieve this functionality Login and Register need to be split. The new functionality will do the following:

  1. Add enduser registration endpoint to oidc_backends.xml

  2. Expose them in /api/configuration

  3. Add a galaxy configuration option to disable local accounts

  4. Change the frontend to: a. Split Login / Registration into two buttons Screenshot 2025-05-20 at 01 27 30

    b. In case only one OIDC provider is configured and local accounts are disabled, Login will redirect the user to the OIDC endpoint. In case local accounts are enabled the behavior of previous galaxy is retained. Screenshot 2025-05-20 at 01 27 39 c) In case local accounts are disabled and one OIDC provider with end user registration endpoint exists register will redirect the user straight to the registration of the OIDC provider. In case no OIDC provider with end user registration is configured and local accounts are disabled, or local registration is disabled in galaxy.yml, registration is not shown on the UI (as per current behavior) If more than one OIDC provider with an endpoint are defined, or local accounts are enabled the Registration page will show all options. Screenshot 2025-05-20 at 01 27 44

How to test the changes?

(Select all options that apply)

  • [x] I've included appropriate automated tests.
  • [x] This is a refactoring of components with existing test coverage.
  • [x] Instructions for manual testing are as follows: In order to test oidc registration, or disabling local accounts manual testing is still required. The best way is to test the following:
    1. disable local accounts, and configure one OIDC provider -> Only Login is visible on the UI, clicking login will directly point the user to the OIDC login.
    2. Enable local accounts and configure one OIDC provider -> Behavior as today.
    3. Enable local accounts and configure one OIDC provider with end user registration endpoint -> Registration page shows button <Register with ...> Screenshot 2025-05-20 at 01 27 44
    4. Disable local accounts and configure one OIDC provider with end user registration endpoint -> Clicking registration will redirect the user to the end user registration point.

License

  • [x] I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

uwwint avatar May 19 '25 15:05 uwwint

@dannon

uwwint avatar May 19 '25 15:05 uwwint

Thanks for these changes @uwwint !!! I've made some minor styling/refactoring/linting/import ordering etc fixes, used Galaxy base components ...

Will try out all the use cases you've added next!

ahmedhamidawan avatar May 23 '25 00:05 ahmedhamidawan

Hi @ahmedhamidawan do you need any help on setting up the Tests?

uwwint avatar May 29 '25 05:05 uwwint

Hi @ahmedhamidawan do you need any help on setting up the Tests?

Hey @uwwint 👋 !

Feel free to add/adjust tests, and make changes on top of mine if needed!

Just make sure to lint the changes before you commit:

  • cd client && yarn run prettier --check
  • cd client && yarn run eslint
  • make format (for backend)
  • tox -e mypy (for backend)

ahmedhamidawan avatar May 29 '25 13:05 ahmedhamidawan

I think these are fine. Do you think we can merge?

uwwint avatar Sep 23 '25 01:09 uwwint

I think these are fine. Do you think we can merge?

Hey @uwwint ! I'll get back to this now and resolve the conflicts, then this should be good to go

Update: I've rebased and cleaned up the commits. @uwwint it would be ideal if you can test this out locally to see if it looks good.

ahmedhamidawan avatar Sep 23 '25 13:09 ahmedhamidawan

@marius-mather do you have capacity to give this a test - ? I can only get to it next week

uwwint avatar Sep 24 '25 05:09 uwwint

Hi @ahmedhamidawan , I've checked this PR out locally and I've confirmed that I'm able to disable local accounts, add a single OIDC provider, and have the separate Login and Register buttons work as expected (going to the OIDC login, or the configured registration page). Everything's looking good to me

marius-mather avatar Sep 25 '25 04:09 marius-mather

Several seleniums are failing, maybe at the login/registration step? I reran them but will investigate this further as well

Update: Yes, they were failing because we changed the singular Login or Register button to two Login and Register buttons. I've fixed the selectors for those.

ahmedhamidawan avatar Sep 25 '25 05:09 ahmedhamidawan

Thanks @ahmedhamidawan ! the remaining test failures look unrelated now

marius-mather avatar Sep 26 '25 01:09 marius-mather

Can this be merged now @nuwang ?

marius-mather avatar Sep 29 '25 01:09 marius-mather

lgtm @ahmedhamidawan. You ok with merging this?

nuwang avatar Sep 29 '25 20:09 nuwang

lgtm @ahmedhamidawan. You ok with merging this?

Hey @nuwang , @dannon wanted to take a look at this and provide some feedback

ahmedhamidawan avatar Sep 29 '25 20:09 ahmedhamidawan

Thanks @uwwint and @ahmedhamidawan for the work on this!

I'm a bit concerned with the configuration changes here though. We're spreading auth config even more in galaxy.yml, when we've been talking about converging everything auth-related into a single place.

A few thoughts:

  1. The allow_local_account_creation rename - This looks like it'll break existing configs. We should probably keep backward compatibility here.

  2. The auto-redirect behavior - Having it magically redirect when there's only one OIDC provider feels a bit too implicit. Admins should be able to control this explicitly.

  3. Split login/register buttons - This goes against how most sites handle auth these days. Usually there's just one "Sign in" button that figures out if you need to login or register.

I'm not against merging this since it solves a real problem, but what if we:

  • Make sure old configs work identically (and then I think the changes to the 1607 config sample should not be required)
  • Add an explicit auto_redirect option (maybe roll this into below)
  • Open a follow-up to consolidate everything into a single auth block in galaxy.yml

I'd love to see us move toward something like this eventually (rough, but kind of what I'm thinking)

auth:
  require_login: false
  allow_registration: true
  registration_warning: "Please use institutional account if available"
  providers:
    - name: google
      client_id: ${GOOGLE_CLIENT_ID}
      client_secret: ${GOOGLE_CLIENT_SECRET}
    - name: keycloak
      url: https://auth.example.org/realms/galaxy
      client_id: galaxy
      client_secret: ${KEYCLOAK_SECRET}
      auto_redirect: true
    - name: galaxy
      type: local
      allow_creation: false

This would give us one clean place to understand all auth behavior, and we could revisit the unified login/register flow as part of that work. We could also then use this same config block/handling in parent config management, I guess? (auth block if provided, otherwise auth.yml or whatever)

What do you think? (again, not -1, but I think we need to make sure we're on track to clean the config matrix back up)

dannon avatar Sep 29 '25 20:09 dannon