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

Refactor access token rewrite and id_token #25

Merged
merged 1 commit into from
Apr 18, 2022

Conversation

BobbyMcWho
Copy link
Member

@BobbyMcWho BobbyMcWho commented Apr 14, 2022

While the approach in #24 and #17, worked I was a bit perplexed on why in the first place we were creating a new token from the old one, overwriting Strategies::OAuth2's @access_token attr_reader, and creating a new token that is essentially identical to the old one.

I've removed that method alias/overwrite, rely on the access_token that the base OAuth2 strategy gives us, and properly access the id_token on the access_token.

I've also fleshed out the specs a bit more, to validate that things are working as expected w/ the deleted access_token method.

I've released 2f11b42 on RubyGems as v2.0.0.rc1

@BobbyMcWho
Copy link
Member Author

Closing to run CI

@BobbyMcWho BobbyMcWho closed this Apr 14, 2022
@BobbyMcWho BobbyMcWho reopened this Apr 14, 2022
@BobbyMcWho BobbyMcWho changed the base branch from 2_0 to master April 14, 2022 02:48
@ryanswood
Copy link

@BobbyMcWho Glad you went that extra step to simplify the handling of the access token. The change here looks good. Thank you for working with us to make this happen.

@BobbyMcWho
Copy link
Member Author

@ryanswood if you wouldn't mind trying out the release candidate, I can merge and release an official sometime next week. I don't have access to a production app with Okta oauth these days, else I'd validate

@BobbyMcWho BobbyMcWho merged commit b7d530a into master Apr 18, 2022
@ryanswood
Copy link

@BobbyMcWho I was out Friday and see this has now been merged with an official 2.0 release. Thank you for working with us to make this change. I was able to test it and all is well.

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 this pull request may close these issues.

2 participants