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

feat: add PKCE (OAuth) #891

Merged
merged 98 commits into from
Mar 24, 2023
Merged

feat: add PKCE (OAuth) #891

merged 98 commits into from
Mar 24, 2023

Conversation

J0
Copy link
Contributor

@J0 J0 commented Jan 16, 2023

What kind of change does this PR introduce?

Adds POC for PKCE on Server Side.
Key points

  • We make use of the FlowStateID, which represents an entry in the FlowState table to track state
  • We add the provider access token and refresh token to the token response instead of in the cookies
  • Given the multitude of providers it might not be feasible to test the flow against all of them - we test against Github as it is the main provider we use at Supabase.
  • userID on oauthState is not set as a foreign key as it is not set until the identity is created in the callback. Therefore, it could be null and is not set as a foreign key.

What is the current behavior?

We use the implicit flow.

What is the new behavior?

We will use the PKCE flow.

Additional context

You may wish to view the end to end test on external_github_test.go before looking into the details

Short demo: https://share.cleanshot.com/Rf3bPR6t

@J0 J0 requested a review from a team as a code owner January 16, 2023 10:54
@J0 J0 mentioned this pull request Jan 25, 2023
1 task
api/external.go Outdated Show resolved Hide resolved
api/api.go Outdated Show resolved Hide resolved
api/external.go Outdated Show resolved Hide resolved
internal/api/token.go Outdated Show resolved Hide resolved
internal/api/token.go Outdated Show resolved Hide resolved
internal/api/token.go Outdated Show resolved Hide resolved
internal/models/flow_state.go Outdated Show resolved Hide resolved
@hf
Copy link
Contributor

hf commented Mar 20, 2023

@J0 Can you please change the timestamp of the migration to be closer to today and also work with @kangmingtay to get this merged this week?

@J0
Copy link
Contributor Author

J0 commented Mar 21, 2023

@hf sure, let's change the migration timestamp at the end, once we are sure there are no further changes. I'll self review today and we can each do a final pass. Once we're all happy with where we're at with the PR we can merge.

Upd: updated the timestamp

Copy link
Member

@kangmingtay kangmingtay left a comment

Choose a reason for hiding this comment

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

thanks for putting this together!

@J0
Copy link
Contributor Author

J0 commented Mar 24, 2023

Thanks for the refactors! The code is cleaner to read now. Going to merge

@J0 J0 merged commit cf47ec2 into master Mar 24, 2023
@J0 J0 deleted the j0/pkce branch March 24, 2023 10:15
@github-actions
Copy link
Contributor

🎉 This PR is included in version 2.55.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

J0 added a commit to supabase/auth-js that referenced this pull request Mar 29, 2023
The corresponding client side implementation for:
supabase/auth#891

Points of note:

- We don't support `plain` as a method on the client side for now. Users
can make use of `plain` on the server side endpoints if they wish.

TODO:
- [x] Add support for devices without a `window`

After PR:
- Update reference spec with example on how to use PKCE

---------

Co-authored-by: joel@joellee.org <joel@joellee.org>
Co-authored-by: Stojan Dimitrovski <sdimitrovski@gmail.com>
Co-authored-by: Kang Ming <kang.ming1996@gmail.com>
@J0 J0 mentioned this pull request Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants