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

Add PKCE verification support #128

Merged
merged 3 commits into from
Nov 8, 2022
Merged

Conversation

skycocker
Copy link
Contributor

@skycocker skycocker commented Nov 2, 2022

This is pretty much a 1:1 copy of https://github.com/omniauth/omniauth_openid_connect/pull/80/commits with addressed PR suggestions and removed unused code.

You may want to review it with ?w=1 appended to the URL - it'll make reading much easier.

Also, there's one small improvement to the original PR: https://github.com/omniauth/omniauth_openid_connect/pull/128/files#diff-ee829bbbbd9d35ee93126d96347fffbfa8756868001c1d4ad6aff9c4650e8151R251

This allows providing a parameter in case one does not use sessions.

Copy link
Contributor

@stanhu stanhu left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

@stanhu
Copy link
Contributor

stanhu commented Nov 2, 2022

@omniauth/omniauth-oidc Can anyone else review these changes?

@danjay
Copy link

danjay commented Nov 3, 2022

lgtm too. I'll close my PR now the improvements have been implemented in this one.

@skycocker
Copy link
Contributor Author

skycocker commented Nov 8, 2022

@BobbyMcWho @pboling ping ping

this should introduce no changes to people that already have been using #80, except they would now be able to use a version from the master upstream

btw - what do you think about reducing the number of required reviews before workflows are enabled?

@BobbyMcWho
Copy link
Member

I unfortunately don't know much about this strategy, I'll have to find some time to get up to speed on what this proposes

@skycocker
Copy link
Contributor Author

I unfortunately don't know much about this strategy, I'll have to find some time to get up to speed on what this proposes

here's a good, simple explanation: https://doorkeeper.gitbook.io/guides/ruby-on-rails/pkce-flow

here's less simplified, but still a good one: https://developer.okta.com/docs/guides/implement-grant-type/authcodepkce/main/#about-the-authorization-code-grant-with-pkce

Not supporting PKCE these days excludes you from a lot of OIDC providers.

Copy link
Member

@pboling pboling left a comment

Choose a reason for hiding this comment

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

LGTM

@BobbyMcWho
Copy link
Member

I'm going to merge this, but I don't manage releases for this gem, @stanhu looks like the last one who pushed this to rubygems, so I'll defer to them to release

@BobbyMcWho BobbyMcWho merged commit f9b06d0 into omniauth:master Nov 8, 2022
@skycocker skycocker deleted the ms-pkce branch November 10, 2022 01:36
@skycocker
Copy link
Contributor Author

Thank youuu! Before the release, we may want to merge #130 and #127, so the complete support of the PKCE flow with optional checks is added as a feature.

@stanhu
Copy link
Contributor

stanhu commented Dec 26, 2022

v0.5.0 has been released.

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.

5 participants