-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Allow OIDC to update EMAIL and username (if mapped) #21386
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
base: dev
Are you sure you want to change the base?
Conversation
01edf2c to
0455da9
Compare
|
@nuwang Can you help with this one? |
nuwang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can see, this is always syncing when logging in through an OIDC client? However, since Galaxy allows multiple accounts to be connected, I'm not sure that behaviour is desirable, since users may have one primary email they prefer but have secondary accounts connected.
IMO, I think we should simplify this to use the same conditions as here: #21356
That is, update account only if enable_account_interface: false and only one login provider is configured. That way, we also don't need toast messages and can simplify that bit too.
|
The enable_account_interface flag is alrrady presenr so i dont think we need #21356 for this. |
0455da9 to
6ec4ea2
Compare
|
@nuwang Done |
nuwang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments/suggestions:
- We can't auto-update the username/email since Galaxy could have multiple account associations, and that raises a question of, which the authoritative ones should be. Instead, I think we could just piggy back on the
fixed_delegated_authconfig setting - and auto-update the username/email iffixed_delegated_authis on. (we already use fixed_delegated_auth to auto-associate emails with no confirmation - and it essentially implies a trust relationship with the OIDC provider) - That will also allow us to get rid of the toast message - which adds complexity
- We should also have an integration test - modelled after the tests here: https://github.com/nuwang/galaxy/blob/26f86934cb0ae0e003aaf871c2ae0ad4e266b0e4/test/integration/oidc/test_auth_oidc.py#L516
(Please replace this header with a description of your pull request. Please include BOTH what you did and why you made the changes. The "why" may simply be citing a relevant Galaxy issue.)
(If fixing a bug, please add any relevant error or traceback)
(For UI components, it is recommended to include screenshots or screencasts)
How to test the changes?
(Select all options that apply)
The username and email are updated.
License