-
Notifications
You must be signed in to change notification settings - Fork 37
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
Store ID token not access token for ID token in extra info #24
Conversation
+1 for this. Btw, should it check oauth2_access_token['id_token'] is not nil as well as oauth2_access_token? |
I am not sure though I do prefer to leave as is. It is much more clear to have the @rockorequin Thoughts? @BobbyMcWho What are the steps to get this merged? Let me know if there is anything else needed. |
1 similar comment
I am not sure though I do prefer to leave as is. It is much more clear to have the @rockorequin Thoughts? @BobbyMcWho What are the steps to get this merged? Let me know if there is anything else needed. |
@ryanswood Good points, I agree. Also, fwiw I tested the patch and it works fine with my logout code. Is it possible that someone might be using the access token (that is currently mislabeled as id_token)? In which case, would it be a good idea to at least add a note to the readme file saying that the id_token key now actually refers to the id_token instead of the access_token? |
Yeah, when this gets merged it'll be a major version bump most likely since it breaks the existing expected data |
@BobbyMcWho PR look good? Anything else needed? I am assuming the PR would be merged and you would bump the version and do a release? |
1 similar comment
@BobbyMcWho PR look good? Anything else needed? I am assuming the PR would be merged and you would bump the version and do a release? |
@ryanswood @rockorequin please check out #25 |
Followup to #17 which replaces the access token with the ID token in the extra info.
This PR slightly changes the solution and includes a spec.