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

[All] add OAuthenticator.modify_auth_state_hook, allow get_user_groups / auth_state_groups_key to be async #751

Merged
merged 4 commits into from
Aug 29, 2024

Conversation

minrk
Copy link
Member

@minrk minrk commented Aug 23, 2024

closes #750

it feels a little weird to have auth_state_groups_key be a callable that actually fetches external info purely based on the config name, but it's what we have. Is there a better arrangement for adding more fields to the user model? e.g. something after token_to_user? I noted in #750 that extending token_to_user would work. Should we have a hook after token_to_user to allow additional modifications to the user model before things like groups get added, or is making this method/hook async enough?

@minrk minrk requested a review from yuvipanda August 23, 2024 07:45
@yuvipanda
Copy link
Collaborator

ya, how about an augment_auth_state_hook that gets called after authenticate, and can put random stuff in auth_state? Then we can leave the existing group management stuff as is, and replace all the 'override authenticator, override authenticate just to put stuff in auth_state' with that one hook.

@yuvipanda
Copy link
Collaborator

It can take the authenticator instance and existing auth_state as params, and return the augmented auth_state

for populating additional fields in auth_state
@minrk minrk changed the title allow get_user_groups / auth_state_groups_key to be async add OAuthenticator.modify_auth_state_hook, allow get_user_groups / auth_state_groups_key to be async Aug 29, 2024
add versionchanged notes
@minrk
Copy link
Member Author

minrk commented Aug 29, 2024

Added modify_auth_state_hook config, run after build_auth_state but before update_auth_model, so it can modify the inputs to get_user_groups.

I left the async allowance in get_user_groups, since I think that makes sense generally, but now 'the way to do this' would be to use the hook (user) / build_auth_state (subclass).

Copy link
Collaborator

@yuvipanda yuvipanda left a comment

Choose a reason for hiding this comment

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

Generally looks good, one specific question about exception handling mechanism.

@minrk
Copy link
Member Author

minrk commented Aug 29, 2024

I don't see your question, did I miss something?

self.log.error(f"Error in modify_auth_state_hook: {e}")
raise
if isawaitable(auth_state):
auth_state = await auth_state
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't the exception be thrown here on the await than earlier? Wouldn't that require us to be inside the try? I could be wrong...

Copy link
Member Author

Choose a reason for hiding this comment

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

You're 100% right, yes! Fixed.

@yuvipanda
Copy link
Collaborator

oops, something ate it! I've asked again.

@yuvipanda yuvipanda merged commit 022b989 into jupyterhub:main Aug 29, 2024
11 checks passed
@yuvipanda
Copy link
Collaborator

Thanks, @minrk

@consideRatio consideRatio changed the title add OAuthenticator.modify_auth_state_hook, allow get_user_groups / auth_state_groups_key to be async [All] add OAuthenticator.modify_auth_state_hook, allow get_user_groups / auth_state_groups_key to be async Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Managing group information properly when tokens don't include groups claim key?
3 participants