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

use cookies instead of JWT #3420

Merged
merged 5 commits into from
Aug 19, 2019
Merged

use cookies instead of JWT #3420

merged 5 commits into from
Aug 19, 2019

Conversation

Rich-Harris
Copy link
Member

Related to #3415. We want to be able to server-render a list of the user's saved REPLs (and eventually, expose published ones publicly). JWT seems to make this harder — it seems like the logical thing to do would be to use cookies instead. (@lukeed, I am happy to be corrected on this point, if I've misunderstood everything)

This isn't finished yet, but I'm leaving for the day so thought I'd leave it here. Changes still to make:

  • code organisation (/auth/* should just be regular server routes)
  • cache user objects in memory
  • purge expired sessions

This is the first time I've had a crack at this sort of thing, so if I'm making any wince-inducing errors (especially regarding security) please yell at me.

@lukeed
Copy link
Member

lukeed commented Aug 18, 2019

Hey hey, if you just wanna use cookies instead that's a totally fine and valid reason. I see nothing here though that can't be easily done with JWT. If you aren't rushed I can open a PR for a second option

@Rich-Harris
Copy link
Member Author

Would definitely be curious to see how it would work with JWT. When I try and educate myself on the topic I come up against phrases like

JWT is not a good alternative to real sessions for 90% of applications

and

If you are doing SSR, you are going to need to use a Cookie no matter what

I can't see how we could (for example) expose a user-accessible JSON endpoint for the user's saved gists without using a cookie (as in this amendment to #3415), but if we need to use cookies anyway, is there much of a benefit to using JWT as well?

There's also the question of the degree to which this app should be an example of how to authenticate a user and pass data from server to client, and I think I'd argue that the cookie version is more immediately grokkable — certainly compared to cookies plus JWT.

So if you think that JWT is a better approach then I'm definitely interested to see what it looks like, but I do think we need to meet those criteria.

@Rich-Harris Rich-Harris marked this pull request as ready for review August 18, 2019 17:21
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@120ee28). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3420   +/-   ##
=========================================
  Coverage          ?   50.25%           
=========================================
  Files             ?        1           
  Lines             ?      197           
  Branches          ?        0           
=========================================
  Hits              ?       99           
  Misses            ?       98           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 120ee28...eb9e405. Read the comment docs.

@Rich-Harris Rich-Harris merged commit c8a44ec into master Aug 19, 2019
@Rich-Harris Rich-Harris deleted the cookies branch August 19, 2019 12:08
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.

3 participants