search

SohoHouse / nuxt-oauth

Simple OAuth2 integration for your Nuxt app
MIT License
122 stars 27 forks source link

refactor: change client-sessions for cookie-session #56

Closed Venefilyn closed 1 year ago

Venefilyn commented 4 years ago

Fixes #55

This could be considered a breaking change since session is now accessed through req.session rather than req[cookieName]. This can probably be fixed with a Proxy but not sure if that's ideal

This dependency change also goes away from encryption and uses verification instead

Venefilyn commented 4 years ago

@Rhysjc could we get this reviewed?

hamishhossack commented 4 years ago

This could be considered a breaking change since session is now accessed through req.session rather than req[cookieName]. This can probably be fixed with a Proxy but not sure if that's ideal

I think that the req[opts.sessionName] -> req.session is a valid change - this would rarely be used (if at all) externally since the session would be accessed via the cookie not request.

LGTM, but agree it's breaking the api so a major version would be required. @samtgarson thoughts?

samtgarson commented 4 years ago

Agreed, this looks great. Thanks!

Quick question though @SpyTec, I'm wondering why you chose lax for the SameSite policy, and if this should be strict given this is an authentication session, or at least user configurable (with a sensible default).

Venefilyn commented 4 years ago

strict would definitely be more secure, but that would definitely invoke a breaking change. lax is the default in browsers and has always been. Making it user configurable makes sense to me, I can get on that if you want

samtgarson commented 4 years ago

@SpyTec makes sense.

~For now we'd like to try and keep the session encrypted, @hamishhossack has an alternative fix for #55 that will hopefully allow us to achieve a win win—we'll get back to you next week.~ Ignore this, he's already pushed #63 and replied on #55.