Use encoding/json for SessionState serialization #63
Conversation
There was a problem hiding this comment.
Looks pretty good in general, one minor thing I picked up on about the non-ciphered case maybe not preserving existing behaviour?
It might be nice to have a legacyDecode that would decode older sessions if they don't unmarshal into JSON (basically the original DecodeSessionState), this would mean we don't force people to re-authenticate if they already have an existing session. If we do this I would add a TODO to then remove it in a couple of releases time, WDYT @yaegashi @syscll? I'm not 100% certain this is worth doing but might be a slightly smoother upgrade for people
|
@yaegashi great job on this 👍 @JoelSpeed I agree with what you're saying, but I don't think re-authenticating is the worst thing? |
yaegashi
force-pushed
the
session-state-json
branch
2 times, most recently
from
7d6fa21
to
4101e11
Compare
|
@JoelSpeed @syscll Thanks for the comments. Actually I did't think it's so important to keep cookie compatibility across versions either, but I added support for older session cookies and a bit more strict tests anyway. |
There was a problem hiding this comment.
Thanks for adding the legacy decode, I think that's a great way to keep faith in the project!
Couple of minor things picked up, one ignore error by the looks of it which we should fix and then the annoying encoding of the zero value of time.
Looks pretty good but I'd like to have a few days to think and see if I can come up with a workaround for this zero time problem that would mean it doesn't zero to a "wrong" value and get encoded. Let me know if you have any ideas!
yaegashi
force-pushed
the
session-state-json
branch
from
ef8054f
to
9941039
Compare
|
@costelmoraru Your changes will break the cookie compatibility across versions this PR tries to honor. Please make it stay as a separate PR for now. You might need to implement the transition from unencrypted cookies. @JoelSpeed What do you think about it? And I think the current code of this PR is ready for your review regarding time.Time zero value workaround. Please take a look in order to make this PR done! |
There was a problem hiding this comment.
I think for now it is best to leave the changes from #67 out of this and follow up with a separate PR, this is primarily because this PR is large enough already and is ready at this point.
@yaegashi The only thing I need now is an update to the Changelog detailing the changes you have made in this PR and I'm happy to get this merged
yaegashi
force-pushed
the
session-state-json
branch
from
9941039
to
4cb4a14
Compare
|
@JoelSpeed Updated CHANGELOG.md and rebased to master. You could amend my log description if needed. CI failure has persisted since yesterday? It seems something is wrong with google-api-go-client upstream. |
yaegashi
force-pushed
the
session-state-json
branch
from
4cb4a14
to
abc3a79
Compare
|
@JoelSpeed can you please have this one merged into master? |
|
Looks like there are some conflicts in the CHANGELOG that need resolving first. |
In order to make it easier to extend in future.
This improves safety and robustness, and also preserves the existing behaviour.
Use the test vectors with JSON encoding just introduced.
yaegashi
force-pushed
the
session-state-json
branch
from
abc3a79
to
7a73a1c
Compare
|
@JoelSpeed Please review and merge this. |
Description
This PR implements more generic way to serialize session state to be stored in browser cookie.
json:",omitempty"tag and SessionStateJSON struct to encode SessionState without exposing unused members.Motivation and Context
Using JSON greatly simplifies implementation of SessionState serialization and makes it much easier to extend in future, like adding IDToken which recently happened.
For more specific example, I have a plan to enable azure provider to pass session user's group information retrieved from Azure AD to the upstream using X-Forwarded-Groups header.
How Has This Been Tested?
Checklist: