search

omniauth / omniauth_openid_connect

MIT License
171 stars 187 forks source link

Feature/pkce #80

Closed danjay closed 2 years ago

danjay commented 3 years ago

Add support for PKCE flow (see https://oauth.net/2/pkce/).

Largely based on the implementation in the omniauth-oauth2 gem.

danjay commented 3 years ago

Hi @m0n9oose are you likely to have any time to look at this in the near future? Thanks

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

davidwessman commented 2 years ago

@Eric-Guo Can you open this one again?

Eric-Guo commented 2 years ago

@davidwessman I found gitlab-omniauth-openid-connect maintain a copy, not sure if it can resolve your problem.

davidwessman commented 2 years ago

@davidwessman I found gitlab-omniauth-openid-connect maintain a copy, not sure if it can resolve your problem.

Thank you, I will investigate it! We have been using the above commits in production for a while now, @danjay do you have time to rebase and fix this or should we create a separate PR?

danjay commented 2 years ago

@davidwessman sure, I will take a look later. The gitlab one didn't support PKCE last time I checked.

@Eric-Guo are you looking for additional collaborators? Like others I have been using a fork for a while but would be good to get back to using a shared project.

danjay commented 2 years ago

@Eric-Guo should be good to go now.

davidwessman commented 2 years ago

@danjay Can you rebase so we can see if the tests run? I just added Github Actions

davidwessman commented 2 years ago

@stanhu can you allow the tests to run?

davidwessman commented 2 years ago

I have been using this in production for almost a year and it works for our pkce-integration. But I do not know how to review this.

stanhu commented 2 years ago

Could you resolve conflicts here?

danjay commented 2 years ago

@stanhu done. Also added a Rubocop exception for class length so the checks should pass this time.

lukejpreston commented 2 years ago

Hello, I noticed this has been approved but hasn't moved. I have tested it against our codebase and it resolves the PKCE issue we were having. Would it be possible to get this merged and deployed soon, please?

stanhu commented 2 years ago

This pull request has conflicts and still needs to address some review comments. If anyone wants to address these, feel free.

skycocker commented 2 years ago

I addressed them in https://github.com/omniauth/omniauth_openid_connect/pull/128.

danjay commented 2 years ago

Closing this now improvements have been made in another PR.