feat(consent): record consent in the same transaction as the user - #1914
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage Report for CI Build 33670678027Coverage increased (+0.3%) to 50.021%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
481c8eb to
c396eaa
Compare
c396eaa to
6989f83
Compare
The invariant this establishes: a user row without a consent record is impossible. ResolveAll runs before the transaction opens, so an incomplete payload never starts one; inside it, the user insert and the consent insert both land or neither does. user.Repository and the new user_consents repository each gain a Create that takes a *sqlx.Tx. pkg/db has WithTxn but carries no transaction on the context, so the transaction is threaded through explicitly rather than found on one. Both are additive, and the user repository change is the one place this feature reaches outside its own domain. The consent repository has Create and nothing else, because the table is immutable. consent.Grant writes one record for the documents it is given and has no completeness rule of its own. ResolveAll is what decides a signup covers every configured document, and keeping that out of Grant leaves room for a later re-consent covering a subset without a second write path. getOrCreateUser now has three outcomes for a new user. A complete payload writes both rows in one transaction. An incomplete one returns ErrConsentRequired and writes nothing. An existing user gets no record at all, which is absolute: a record written outside a user creation would carry that moment's timestamp and IP for an agreement made elsewhere, which is worse than no record because it reads like evidence. A nil flow means one of the paths that create a user without one, and those stay exempt because no account holder is present to consent. The completeness check runs at user creation under every intent, not for the error but as the invariant guarding the write. An unset intent is permissive for the login gate but never for consent. With app.consent disabled ResolveAll resolves nothing, and an empty document set means write no record, so nothing changes for a deployment that does not ask for consent. Each signup also writes one audit record, with UserConsentGrantedEvent and ConsentType added to pkg/auditrecord following the entity.verb naming already there. It goes through the repository with the actor filled in, as userpat does for its PAT events: the repository enriches an empty actor from the context, and these endpoints are on the authentication skip list with no actor in it, so the record would otherwise land as the system actor for an act a person performed. It is written after the commit, since the audit repository has no transactional create, so it cannot be atomic with the record it describes. That is why the consent record is the source of truth and this one is a breadcrumb: a failure is logged and the signup stands. Its target metadata carries the whole document snapshot rather than the id and the version alone, so a reader working from the audit trail can say what was accepted without reading back a record they may not have access to. The rollback is tested against a real Postgres rather than a mocked transaction, since a mock can only pretend to roll back. See docs/rfcs/0002-explicit-consent-at-signup.md, Enforcement and Storage.
6989f83 to
fa677e9
Compare
| // written once, so there is nothing to deduplicate. | ||
| }); err != nil && s.logger != nil { | ||
| s.logger.ErrorContext(ctx, "failed to write the audit record for a granted consent", | ||
| "consent_id", granted.ID, "user_id", granted.UserID, "error", err) |
There was a problem hiding this comment.
In case of error we can manually create the record for book keeping. In that case we will need all the fields and info. Check if we need to print more info here or the rest is already there in consents table?
There was a problem hiding this comment.
All the info needed is present in user_consents table. Only org_id is missing but that will always be PlatformOrgID
| type Repository interface { | ||
| Create(ctx context.Context, tx *sqlx.Tx, cnst Consent) (Consent, error) | ||
| } |
There was a problem hiding this comment.
We will need to establish a pattern across the codebase on how to move transactions across services — this doesn't exist as of now, and will be done later. The approach may vary as well. So instead of doing it as a one-off case and making db trnx as dependency in service, can we instead look at alternatives, e.g. a postgres-layer method (say CreateWithConsent with the interface declared on authenticate's side since it already imports both domains) which opens WithTxn internally and inserts all the rows required?
3 participants
Summary
ResolveAllruns before the transaction opens, so an incomplete payload never starts one; inside it, the user insert and the consent insert both land or neither does. Per RFC 0002, Enforcement and Storage.user.RepositorygainsCreateWithTx, which is the one place this feature reaches outside its own domain.pkg/dbhasWithTxnbut carries no transaction on the context, so it is threaded through explicitly. Both create paths share one query builder, so they cannot drift.user.consent_grantedaudit record is written after the commit, through the repository rather than the service, withActorfilled in explicitly — these endpoints are skip-listed, so an empty actor would be enriched to the nil UUID and thesystemactor for an act a person performed. It cannot be atomic with the consent row, which is why that row is the source of truth and a failed audit write is logged and carried past.