search

omniauth / omniauth-okta

OAuth2 strategy for Okta
MIT License
41 stars 37 forks source link

Store ID token not access token for ID token in extra info #24

Closed ryanswood closed 2 years ago

ryanswood commented 2 years ago

Followup to https://github.com/omniauth/omniauth-okta/pull/17 which replaces the access token with the ID token in the extra info.

This PR slightly changes the solution and includes a spec.

rockorequin commented 2 years ago

+1 for this.

Btw, should it check oauth2_access_token['id_token'] is not nil as well as oauth2_access_token?

ryanswood commented 2 years ago

@rockorequin

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 extra info hash contain the key id_token with a nil value as a way to explain the ID's absence rather than not having the key. Plus, this code has worked this way for ~5 years so it would be problematic if the key were not to exist on the next release.

@rockorequin Thoughts?

@BobbyMcWho What are the steps to get this merged? Let me know if there is anything else needed.

ryanswood commented 2 years ago

@rockorequin

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 extra info hash contain the key id_token with a nil value as a way to explain the ID's absence rather than not having the key. Plus, this code has worked this way for ~5 years so it would be problematic if the key were not to exist on the next release.

@rockorequin Thoughts?

@BobbyMcWho What are the steps to get this merged? Let me know if there is anything else needed.

rockorequin commented 2 years ago

@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?

BobbyMcWho commented 2 years ago

Yeah, when this gets merged it'll be a major version bump most likely since it breaks the existing expected data

ryanswood commented 2 years ago

@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 commented 2 years ago

@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?

BobbyMcWho commented 2 years ago

@ryanswood @rockorequin please check out #25

BobbyMcWho commented 2 years ago

I appreciate your patience on this folks, this has been released in v2.0.0. Released on rubygems.