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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should OIDC fetch_user, fetch_userinfo, and validate_id_token allow for dynamic OpenID configuration? #72

Closed
oshbec opened this issue Dec 9, 2020 · 3 comments 路 Fixed by #73

Comments

@oshbec
Copy link

oshbec commented Dec 9, 2020

Hi, and thanks for building and maintaining this 馃憢.

In the configuration documentation for OIDC, :openid_configuration isn't strictly required since it can be fetched from :openid_configuration_uri if it isn't defined. Similarly, :openid_configuration_uri is also optional, since it defaults to /.well-known/openid-configuration based on :site.

Both authorize_url/1 and callback/3 work this way by calling openid_configuration/1. However, fetch_user/2, fetch_userinfo/2, and validate_id_token/2 are using Config.fetch/2 to resolve configuration, so they aren't getting it dynamically.

Should these work consistently? I'm a bit new to both Elixir and OIDC, so it's quite possible my understanding is off here.

If the intent is to have them all get :openid_configuration dynamically, I'm happy to try submitting a PR.

Thanks for your time!

@danschultzer
Copy link
Collaborator

danschultzer commented Dec 10, 2020

Good call!

Yeah, I didn't think of fetch_user/2, fetch_userinfo/2 or validate_id_token/2 functions being called outside the callback/3 flow even though they are public functions. You are right that we should use the same logic always to fetch :openid_configuration in case you are calling these functions separately.

Feel free to open PR calling openid_configuration(config) instead of Config.fetch(config, :openid_configuration) in fetch_userinfo/2 and validate_id_token/2 (fetch_user/2 calls validate_id_token/2 so only those two a necessary to update) 馃槃

@danschultzer
Copy link
Collaborator

Going to publish new release soon so went ahead and update the functions in #73

@oshbec
Copy link
Author

oshbec commented Jan 5, 2021

Thanks for your work on this!

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 a pull request may close this issue.

2 participants