Repository navigation
[sql-18] sessions: tightly couple sessions & accounts - #989
Merged
Merged
Conversation
ellemouton
added
the
no-changelog
ellemouton
force-pushed
the
sql18Sessions10
branch
2 times, most recently
from
February 28, 2025 07:55
d1b0cf8 to
66b8330
Compare
ViktorT-11
reviewed
Feb 28, 2025
ViktorT-11
left a comment
ViktorT-11
left a comment
Contributor
There was a problem hiding this comment.
Generally looks great 🚀 !
I'd like to get some clarifications on the first commit thought, before ACKing :).
The last to comments are optional and non-blocking.
ellemouton
force-pushed
the
sql18Sessions10
branch
2 times, most recently
from
February 28, 2025 12:18
638c03c to
021daae
Compare
bitromortac
approved these changes
Mar 3, 2025
bitromortac
left a comment
bitromortac
left a comment
Contributor
There was a problem hiding this comment.
LGTM 🎉, have only a few nits
| string(caveatPrefix), | ||
| 2, | ||
| ) | ||
| if len(caveatSplit) == 2 { |
Contributor
There was a problem hiding this comment.
strings.Cut could maybe be a nicer alternative here. Is it problematic if the macaroon has two account caveats?
Contributor
Author
There was a problem hiding this comment.
strings.Cut could maybe be a nicer alternative here.
nice!
Contributor
Author
There was a problem hiding this comment.
Is it problematic if the macaroon has two account caveats?
Yeah - but that would already be a problem today.
This commit is just a refactor of how the extraction logic already works.
Today, the interceptor would just match on the first account.
ellemouton
force-pushed
the
sql18Sessions10
branch
from
March 4, 2025 11:42
021daae to
d7f3859
Compare
When we link a session to an account, we want to be able to validate that the account actually does exist. So in preparation for that, we first give the session store access to the accounts store.
And use that from the existing accountFromMacaroon helper (which will then test the new helper by proxy). We add this helper so that we can use it later on from the sessions package where we want to extract an account ID from a caveat (we wont have a full macaroon available).
In this commit, we more tightly & explicitly link a session to an account. At a persitance layer, we have always only linked a session to an account by encoding the AccountID within the macaroon caveat that we store with the session. We still keep this persistence the same but now we first ensure that the account exists and we also add an AccountID field to the Session struct.
ellemouton
force-pushed
the
sql18Sessions10
branch
from
March 4, 2025 11:43
d7f3859 to
bfb3c7b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In this commit, we more tightly & explicitly link a session to an
account. At a persitance layer, we have always only linked a session to
an account by encoding the AccountID within the macaroon caveat that we
store with the session. We still keep this persistence the same but now
we first ensure that the account exists and we also add an AccountID
field to the Session struct.
This is in preparation for adding a SQL implementation of the session Store
where this link will be done at a relational DB level.
part of #966