Add initial session-store interface and implementation #147
Conversation
|
This is really nice, idiomatic code 👍 Couple of questions:
|
It definitely can be called concurrently, but at the moment we don't protect against that at all (in master that is). I'm not sure how you could lock across multiple instances to prevent it being called concurrently for a particular user so I was planning to just leave this pretty much as is. This PR plus #148 should be functionally equivalent to the existing behaviour. I would like to add a lockable session store implementation at some point but that will likely have to be a server side session store implementation. The only reason this would cause a problem being called concurrently is if you are using session refreshing with the upstream provider who rotates keys (looking at OIDC here) on every refresh, then you can end up with users being logged out periodically unless you do some caching fun in front of the proxy. This has been on my mind for a while... 😅
If you check #148, the actual implementation should just be all the code (pretty much) deleted from
Im planning to build an image from #148 and will deploy it somewhere for testing, maybe internally at Pusher 🤔 I'll make sure we test it for a few days and see that everything is working as expected before we merge any of this code. I would like to (long term) set up so proper acceptance/black box tests for this project though I'm not entirely sure how to test projects with UIs, will need some research doing. |
| if value != "" { | ||
| value = cookie.SignedValue(s.CookieSecret, s.CookieName, value, now) | ||
| } | ||
| c := s.makeCookie(req, s.CookieName, value, expiration) |
There was a problem hiding this comment.
Should the signature of makeCookie include a now time.Time?
There was a problem hiding this comment.
I actually made the same change in #148, do you think I should move the change across to this PR?
https://github.com/pusher/oauth2_proxy/pull/148/files#diff-6795d5c635b0b599acbb55d7272d11f8R114
| // interface that stores sessions in client side cookies | ||
| type SessionStore struct { | ||
| CookieCipher *cookie.Cipher | ||
| CookieDomain string |
There was a problem hiding this comment.
Should we be sending along the options.CookieOptions struct? This might make makeCookie a bit more usable across different store implementations
There was a problem hiding this comment.
That's a great idea! If we keep the CookieOptions struct we can move makeCookie to a shared package and make it reusable, I'll do that and join it to #148
|
I've rebased, upped the linter timeout to stop the tests timing out, added a changelog entry and started a documentation page for the session storage stuff so will need another quick review on that last two commits before merge |
Signed-off-by: bitliu <bitliu@tencent.com>
Description
This adds a SessionStore interface that can/will be used by the proxy to provide both client-side and server-side session storage implementations.
This PR is mainly copying and pasting code without much tidy up (especially in the cookie store implementation) but does add some tests to the SessionStore using ginkgo and gomega.
I haven't implemented using the
SessionStorein the proxy itself yet so this PR should make no functional changes (bar the config changes that have been made).I will follow with a separate PR to implement the
SessionStorein the main proxy.CC @brianv0
Motivation and Context
We want to add server side sessions, see issue #138
How Has This Been Tested?
Test suite added for
SessionStoreto ensure cookies have correct values set and Save/Load cycle produces the same session.I manually tested that the configuration loading changes still work, have performed full authentication flow using image built from this branch
Checklist: