[sql-38] multi: couple sessions and accounts to actions #1071
Conversation
ellemouton
added
the
no-changelog
ellemouton
force-pushed
the
sql38
branch
3 times, most recently
from
be966f6 to
f93f70e
Compare
In this commit, we do our best to ensure that at least at the time of action creation, if the session ID is set, then our bbolt actions store impl will at least first check that the session really does exist. This also forces us to update our tests in preparation for the SQL store which will tightly couple the actions and sessions.
We only use it in one place and we can just handle the unwrap of the option there.
In this commit we add an optional AccountID to the RequestInfo type. Then, we populate it if the caveat of the macaroon being used contains an accounts caveat. We also add an unused AccountID type to the AddActionReq and pass in the value from the RequestLogger.
In this commit, we do our best to ensure that at least at the time of action creation, if the account ID is set, then our bbolt actions store impl will at least first check that the account really does exist. This also forces us to update our tests in preparation for the SQL store which will tightly couple the actions and accounts.
There was a problem hiding this comment.
Nice, LGTM 🚀! Only leaving a few nits which are optional.
Also: Could potentially add test coverage under "kvdb" builds that show that the sessions + accounts aren't tightly coupled to actions. I think that'll be a quite nice coverage to have once the tightly coupling under "sql" builds exists, so that you can show that it's coupled there. Just to prove that this is expected behaviour. Potentially though, that's something that would make more sense to add coverage of in that PR.
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
nit: could potentially wrap the error on session.ErrSessionNotFound errors to make it clear that was a linked session for an action that wasn't found.
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
nit:
Same here but for accounts.ErrAccNotFound.
| // Accounts are not explicitly linked in our bbolt DB implementation. | ||
| got.AccountID = expected.AccountID |
There was a problem hiding this comment.
nit: Should we remove the overwritten value from got at the end of this function as this is a pointer we're passing?
In this PR, we add best effort logic to ensure tight coupling between actions and sessions&accounts at the
time of action creation. This forces us to update our tests to expect certain errors if linked sessions/accounts
do not yet exist at the time of action creation. This sets us up nicely for the SQL impl of the actions DB which will
enforce strict coupling.
Part of #917