diff --git a/.pr-drafts/cld-3570.md b/.pr-drafts/cld-3570.md new file mode 100644 index 0000000000..e6a5ea5dcd --- /dev/null +++ b/.pr-drafts/cld-3570.md @@ -0,0 +1,59 @@ +# feat(consent): add the user_consents table with immutability triggers + + + +Part of [RFC 0002: Explicit consent at signup](https://github.com/raystack/frontier/blob/main/docs/rfcs/0002-explicit-consent-at-signup.md). Based on #1908. + +## Summary + +Adds the `user_consents` table from the RFC's Storage section. Schema only — there is no repository and no write path here. The repository lands with its caller in the transactional-write PR, so it arrives with something that exercises it rather than as dead code. `audit_records` shipped the same way: the table migration in #1118, the repository in #1124. + +A consent record says which documents a user accepted, in what version, when, and from where. It has to be readable years later, it has to survive the user row, and nothing may edit it after the fact — those three requirements are what the rest of this description is about. + +## Changes + +- `20260830100000_create_user_consents.up.sql` — the table, the `documents_not_empty` CHECK, the partial unique index `uq_user_consents_signup`, and `BEFORE UPDATE` / `BEFORE DELETE` triggers raising `45000`. +- `20260830100000_create_user_consents.down.sql` — drops the triggers, the functions, the index and the table. + +`documents` is a JSONB array holding one object per accepted document, copied from config at write time with the same four fields config holds (`id`, `title`, `version`, `url`). The CHECK enforces that it is an array and non-empty, so a consent that covers nothing cannot be stored. + +`consented_at` is when the user accepted, taken from the flow, not when the row was written. `created_at` is the write time. They differ by an OIDC round trip, and conflating them would put the post-redirect moment on the record. + +## Technical Details + +**Four choices that read as mistakes without the RFC.** Each is also commented in the migration, since the schema outlives this PR description. + +*No foreign key to `users`.* `UserRepository.Delete` does a hard `DELETE`, so `ON DELETE CASCADE` would drop consent records along with the account and `ON DELETE RESTRICT` would block account deletion outright. Neither is acceptable: the records have to outlive the user. That is also why `user_email` is denormalized onto the row — after the user is gone, the email is the only thing left identifying whose consent it was. + +*`ip_address` is `TEXT` and nullable, not `INET`.* The value comes from a request header. A proxy that sends a malformed value, or a deployment that sends none, must not fail a signup over it — `INET` would reject the row and take the account creation down with it. The RFC's Limitations section is explicit that the IP is only as good as the header it comes from. + +*Versions and URLs are copies, not references.* A record stays readable after the document leaves config, and stays correct after the version bumps. It also means a `consent_documents` table can be added later with no backfill, since every record already carries its own snapshot. The tradeoff is that a record ties to a version string rather than to the document text — a per-document hash would close that and can be added later. + +*The partial unique index* gives a user at most one signup consent. Nothing in the design repairs or rewrites a record, so a second signup write is a bug rather than a legitimate update. The index makes that bug fail loudly instead of leaving two rows that disagree about what the user accepted. It is partial on `source = 'signup'` so a later re-consent, written with a different `source`, is not blocked by it. + +**Also deliberate:** there is no `metadata` column. Nothing would write it today, and a re-consent that needs one can add it in its own migration — an unused JSONB column invites unstructured writes that no reader expects. + +**Immutability, and why `DELETE` is guarded too.** The triggers follow `20250904105226_add_audit_records_immutability.up.sql`: a `plpgsql` function per operation that does nothing but `RAISE EXCEPTION` with `ERRCODE = '45000'`, a `BEFORE ... FOR EACH ROW` trigger, and a `COMMENT ON TRIGGER` recording intent. That precedent guards `UPDATE` only. `DELETE` is guarded here as well, because the failure modes are not symmetric: a modified record is visibly wrong, whereas a deleted one leaves a user who simply looks like they never consented, which is indistinguishable from a user who never did. The triggers fire per row and so do not block `DROP TABLE`, which is what keeps the down migration working. + +**One migration pair, not two.** The `audit_records` precedent is two pairs only because the immutability trigger was an afterthought that arrived days later with the repository. Here the table and its triggers are one design landing at one time, and splitting them would create an intermediate version in which `user_consents` is mutable — a state no deployment should ever be in. `20260218100000_create_user_pats.up.sql` is the closer precedent: table, indexes, function and trigger in a single migration. + +`uuid_generate_v7()` is the existing function from `20250901054744_create_audits_table.up.sql`, which runs first. The down migration deliberately leaves it in place rather than dropping something it does not own. + +## Test Plan + +Verified against Postgres 16 with the repo's own migrate path (`migrations.MigrationFs` through `golang-migrate`), the same code `frontier server migrate` runs. + +- [x] `migrate up` from an empty database — clean, lands on `20260830100000`, not dirty. +- [x] `UPDATE` on a stored row → `ERROR: 45000: user_consents cannot be updated to maintain consent integrity`. +- [x] `DELETE` on a stored row → `ERROR: 45000: user_consents cannot be deleted to maintain consent integrity`. +- [x] Second `source = 'signup'` row for the same `user_id` → `23505` on `uq_user_consents_signup`. A row for the same user with a different `source` inserts, confirming the index is partial. +- [x] `documents` as `'[]'` and as a JSON object → both rejected by `documents_not_empty` (`23514`). +- [x] Null `ip_address` and null `auth_strategy` insert fine; the RFC's `documents @> ...` containment query returns the expected row. +- [x] `migrate down` one step with rows present — table, triggers and functions gone, `uuid_generate_v7()` untouched. Confirms the `BEFORE DELETE` trigger does not block `DROP TABLE`. +- [x] Full up / down / up cycle, and a full `Down()` to version 0 followed by a fresh `up`. +- [x] `make lint` — 0 issues. +- [x] `make test` — passes, including `internal/store/postgres`, which boots a Dockerized Postgres and applies every migration. + +## SQL Safety + +Not applicable — this PR adds two `.sql` migration files and touches no `*_repository.go` and no `goqu.*`. There is no query construction here at all; the repository that will query this table lands in a later PR, where the checklist applies. diff --git a/.pr-drafts/cld-3571.md b/.pr-drafts/cld-3571.md new file mode 100644 index 0000000000..e1a6448885 --- /dev/null +++ b/.pr-drafts/cld-3571.md @@ -0,0 +1,75 @@ +# feat(authenticate): separate login from signup with an explicit intent + + + +Part of [RFC 0002: Explicit consent at signup](https://github.com/raystack/frontier/blob/main/docs/rfcs/0002-explicit-consent-at-signup.md). Based on the user_consents migration. + +## Summary + +Frontier cannot currently tell a signup from a login. `SignInView` and `SignUpView` are the same view with different strings, `AuthenticateRequest` and `Flow` carry no intent, and every strategy ends at `getOrCreateUser`, which returns the existing user or creates one. So a login with an unknown address creates the account, and there is no point in the code where frontier knows that is what just happened. + +This adds a flow intent and both gates from the RFC's intent-by-strategy table. It ships value on its own, with no consent involved: **a login never creates an account, and a signup never logs an existing user in.** It is also the prerequisite for consent, because a consent check that cannot identify a signup has nothing to attach itself to. + +No migration. No proto or handler changes — the RPC fields exist already and the wiring from the handler is a separate PR. + +## Changes + +- `FlowIntent` (`""` / `"login"` / `"signup"`) with `Intent`, `AcceptedDocumentIDs` and `IPAddress` on `RegistrationStartRequest`. +- `StartFlow` writes the intent and the consent payload (accepted ids, IP, acceptance time) into `flow.Metadata`. +- `Flow.Intent()` and `Flow.Consent()` — nil-receiver-safe typed accessors, plus a `FlowConsent` struct. +- `StartFlow` gates both mail strategies and passkey per the table, and picks the passkey ceremony by intent instead of guessing. +- `getOrCreateUser` takes the flow and applies the same gate at user creation. +- `ErrLoginUserNotFound` and `ErrSignupUserExists`. +- Unit coverage: three intents against an address that does and does not have an account, at both enforcement points, for mail OTP, mail link and passkey. + +## Technical Details + +**The gate table, and why there are two gates.** + +| Intent | Strategy | At `StartFlow` | At user creation | +|---|---|---|---| +| login | mailotp, maillink, passkey | reject if no user exists | reject if no user exists | +| login | oidc | email unknown, no check | reject if no user exists | +| signup | mailotp, maillink, passkey | reject if a user exists | reject if a user exists | +| signup | oidc | email unknown, no check | reject if a user exists | +| unspecified | all | no check | create or get, as today | + +`StartFlow` is the fast path and exists for the error message. It fails before an OTP is sent, so a login with no account gets "no account for this email" on the form the user is looking at, rather than waiting for a code that will never be accepted. RFC alternative 9 weighed keeping `Authenticate` quiet about whether an address has an account; the clearer error won, and Limitations records the cost. + +User creation is the gate that matters. Every strategy ends at `getOrCreateUser`, and it is the last point before an account would exist. OIDC has no email until the provider returns, so the flow-start check cannot cover it and the creation check is the only one it gets. That asymmetry is the whole reason there are two. + +**The intent lives on the flow, not on the request that finishes it.** Only `state` survives a redirect to an identity provider, and it already carries the flow id, so anything that has to outlive the redirect goes where the flow id points. `Flow.Metadata` is an existing JSONB column that already carries `callback_url`, so this needs no migration — RFC alternative 10 considered a first-class `Intent` column on `flows` and rejected it, since it costs a migration for one string while the consent payload has to go in metadata regardless. + +The consent payload (`accepted_document_ids`, `ip_address`, `at`) is written here but read by nobody yet. It is in this PR because it is the same write, on the same line of the same function, and splitting it would mean touching `StartFlow` twice. The IP and the timestamp are from when the user accepted, not from the callback — the flow row is written before the browser leaves and read after it returns, so neither value passes through the browser. + +**The accessors parse rather than assert.** `flow.Metadata` is stored as JSONB, which does not return the types it was given: the ids come back as `[]any` and the timestamp as an RFC 3339 string, not `[]string` and `time.Time`. The existing `flow.Metadata["callback_url"].(string)` is an unchecked assertion that only survives because nothing ever writes a non-string there; `otpAttempts` is the better precedent, handling both `int` and `float64` for exactly this reason. `Intent()` and `Consent()` follow it, and treat anything unparseable as absent — a missing or malformed consent key means no consent, never an empty one. + +Both are methods on `*Flow` and both handle a nil receiver. `authenticateWithPassthroughHeader` provisions users from `app.identity_proxy_header` with no flow at all, so it passes `nil` and gets an unspecified intent, with no branch at the call site and no second code path to keep in step. + +**Unspecified stays permissive, deliberately.** An intent-less request behaves exactly as it does today: no lookup at flow start, create-or-get at user creation, and the same passkey guess. That is what lets this land without a client release and without breaking any deployment. It also means the login gate is a UX boundary rather than a security one — any client can opt out by omitting the field. Turning it into a boundary needs a server switch that rejects an unset intent, and a deprecation window; the RFC lists it under future work. + +The same reasoning applies to what gets written: the `intent` and `consent` keys are only written when the caller actually sends them, so a client that sends neither produces a byte-identical flow row to today's. The alternative — always stamping `"intent": ""` — would put a meaningless key on every flow row ever created for no reader's benefit. + +**The passkey guess.** `StartFlow` currently infers signup from login by looking the address up and checking for a stored credential, and `finishPassKeyLoginMethod` calls `getOrCreateUser`, which is how a passkey *login* can create an account. With an intent, signup picks `startPassKeyRegisterMethod` and login picks `startPassKeyLoginMethod`; without one, the guess is untouched. + +That routing change makes `startPassKeyLoginMethod` reachable for a user who exists but has no stored passkey — previously impossible, since the guess only sent users there after confirming the credential. Its first line was `loggedInUser.Metadata["passkey_credentials"].(string)`, an unchecked assertion that would now panic on that input. It is a comma-ok read returning an error instead. Small, but worth flagging: it is the one change here that is not in the ticket scope, and leaving it out would have introduced a panic. + +**Not in scope.** Error mapping at the handlers is a separate PR — until it lands, `ErrLoginUserNotFound` and `ErrSignupUserExists` surface with the current default mapping rather than as `NotFound` and `AlreadyExists`. So is passing the intent, the ids and the IP in from `Authenticate`, which is what makes the fields on `RegistrationStartRequest` reachable from outside Go. All consent work — the config, the service, the `ResolveAll` check and the transactional write — follows separately. + +The RFC's `applyOIDC` / `consumeFlow` fix (OIDC flow rows surviving until the expiry cron, while mail OTP rows are deleted on use) is **deliberately not implemented**, here or elsewhere in this stack. The RFC still describes it; it has been descoped. Worth knowing when reading the RFC against this code, and worth revisiting once those rows hold consent. + +**Mail link is gated too, which the RFC's table does not say.** The table names mailotp and passkey. But `MailLinkAuthMethod` knows the address exactly as early as mail OTP — the handler validates it on the way in — and the two share `applyMailOTP` at the other end, so gating one and not the other was an omission rather than a decision. Ungated, a mail link login for an unknown address sent a link that would only be rejected once the user clicked it. So `gateFlowStart` covers `MailOTP || MailLink` here, hoisted out of the mail OTP branch to the top of `StartFlow` so a rejected request allocates no flow. Passkey keeps its own inline gate, because it already looks the user up to choose a ceremony and a second lookup would be waste. The RFC is left as it is; the divergence is deliberate and recorded. + +## Test Plan + +- [x] `TestService_StartFlow_Intent` — 20 cases: three intents against an address that does and does not have an account, for mail OTP, mail link and passkey, asserting the error for the rejections and the recorded `passkey_type` for the passes. The four intent-carrying mail link cases fail without the gate. Covers all three unspecified-intent passkey guesses so the old behaviour is pinned, and the login-with-no-registered-passkey case that the comma-ok read now handles. +- [x] `TestService_StartFlow_WritesIntentAndConsent` — the metadata write, including that the acceptance timestamp is `s.Now()` at flow start, and that a request with neither field produces the same `{"callback_url": ""}` metadata as before. +- [x] `TestFlow_IntentAndConsent` — the accessors: a nil receiver, a full round trip through the JSON marshalling the Postgres repository does (which is what turns `[]string` into `[]any` and `time.Time` into a string), and the unparseable cases — missing key, wrong type, no documents, non-string ids — all reading as no consent. +- [x] `TestService_FinishFlow_Intent` — the user-creation gate: login logs an existing user in and never creates, signup creates and never logs an existing user in, unspecified does both as before. `userService.Create` is asserted against so a rejection provably creates nothing. +- [x] `make lint` — 0 issues. +- [x] `make test` — passes, 46 packages, including `internal/store/postgres` against a Dockerized Postgres. +- [ ] `make generate` — **fails on this machine and on a clean checkout alike**: `.mockery.yaml` uses mockery v2 keys (`with-expecter`, `mockname`, `outpkg`) and the installed binary is v3.7.3, which rejects them. Pre-existing and unrelated. No mocks needed regenerating regardless: `getOrCreateUser` is unexported, the changed types are struct fields, and no interface in `core/authenticate` or `internal/api/v1beta1connect/interfaces.go` changed signature. + +## SQL Safety + +Not applicable — no `*_repository.go` and no `goqu.*` changes. The flow metadata written here goes through the existing `FlowRepository.Set`, whose serialisation is untouched. diff --git a/.pr-drafts/cld-3572.md b/.pr-drafts/cld-3572.md new file mode 100644 index 0000000000..b8f54665a4 --- /dev/null +++ b/.pr-drafts/cld-3572.md @@ -0,0 +1,82 @@ +# feat(consent): add app.consent config, the consent service, and ListConsentDocuments + + + +Part of [RFC 0002: Explicit consent at signup](https://github.com/raystack/frontier/blob/main/docs/rfcs/0002-explicit-consent-at-signup.md). Based on the flow-intent change. + +## Summary + +A deployment has to be able to say what it asks people to accept, and a client has to be able to find that out before it renders a sign-up form. This adds both halves: `app.consent` on `server.Config`, a `core/consent` service that owns the config and the checks that read it, and `ListConsentDocuments`, an unauthenticated read-only endpoint that serves the resolved set. + +No database, no writes, no migration. Frontier never reads a document and never parses a version string — it copies the four fields config holds, compares versions for equality, and nothing else. The transactional write that turns an accepted set into a consent record is the next PR; this one is what that PR, and the SDK, are built on. + +With `app.consent` absent or disabled, nothing changes: the endpoint returns an empty list and no signup is gated. + +## Changes + +- `consent.Config` (`enabled` plus a `documents` map keyed by id) and `consent.DocumentConfig`, mounted at `app.consent` beside `app.authentication` and `app.pat`. +- `Config.Validate()`, called from `buildAPIDependencies`, so bad config stops the server. +- `logConsentDocuments` in `cmd/serve.go`, writing the resolved set to the startup log. +- `consent.Service` with `Documents()`, `Resolve(ids)` and `ResolveAll(ids)`. +- `ListConsentDocuments` on `ConnectHandler`, plus the `ConsentService` interface and its mock. +- Entries in both `authenticationSkipList` and `authorizationSkipEndpoints`. +- `config/sample.config.yaml` and the configuration reference. + +## Technical Details + +**A map keyed by document id, not a list.** It matches `authenticate.Config` keying `oidc_config` by strategy name, so it is the shape this config file already uses for "several of a thing, each named". The key does real work: it enforces unique ids without a validation pass, and it keeps a single document field env-overridable, which a list index does not. The id is also the value the client sends back, so making it the key means the thing a deployment names is the thing the protocol carries. + +**No per-document `required` flag.** Every document in the map is required at signup. An optional document is only meaningful if it can later be declined or withdrawn, and withdrawal is out of scope — it needs a decision on what happens to the account. A flag that is always `true` is a flag that will be read as a promise the code does not keep. + +Adding a document id breaks signup for a client that sends a hardcoded list, which is exactly why the list is served over an endpoint rather than published as a constant. Bumping a version is safe either way, since the client only ever sends ids. + +**Boot validation, and what it does not check.** Ids, versions and URLs must be non-empty, URLs must parse, and an enabled block needs at least one document. `enabled: true` with no documents is the interesting case: silently disabling itself would leave a deployment that looks identical to a working one while asking nobody to accept anything, and the only way to find out would be to read a signup that succeeded when it should not have. So it fails at boot. + +Two judgement calls worth a reviewer's eye. First, a URL has to be absolute and have a host, not merely parse: `url.Parse` accepts `example.org/legal/terms` as a relative path, and a document the client cannot link to is as useless as one that does not parse at all. Second, `title` is **not** required — the RFC lists ids, versions and URLs and stops there, so an untitled document validates. It renders badly and serves correctly. Say so if the intent was to require all four. + +A disabled block is not validated at all, so a half-written `documents` map on a deployment that has not turned consent on yet is not an error. Turning it on is what makes it one. + +The validator walks the ids in sorted order. Map iteration is randomised, so an unsorted validator names a different document on every boot for the same broken config, which is a miserable thing to debug. + +**Why the resolved set is logged.** Every field here is env-overridable, which is the point of the map, and an override cannot alter a consent record that already exists — but it can produce wrong new ones, silently, for as long as it is set. The config repository will not show it. So the startup log carries the id, title, version and URL of every document the deployment resolved, and that log is what answers "what was this deployment serving on the day this record was written". It is one line per document rather than one line for the set, so a log search for a version string finds the boot that served it. + +**The three service functions, and why they are three.** `Documents()` serves the endpoint. `Resolve(ids)` maps ids to their config snapshots and rejects ids config does not know, and says nothing about whether the set is complete — the flow-start check needs exactly that, since a request without an intent cannot yet know whether it is a signup. `ResolveAll(ids)` adds the completeness rule: the ids must cover every configured document, no more and no less. + +`ResolveAll` compares the two sets in both directions, so the error names what is actually wrong — `ErrUnknownDocuments` with the ids config does not know, or `ErrMissingDocuments` with the required ids the caller left out. One error saying "consent invalid" would make a client-side drift and a config-side drift indistinguishable, and those have opposite fixes. When a set is both incomplete and unknown, the unknown ids are reported, because `ResolveAll` is literally `Resolve` plus one more check and because an unknown id is the more diagnostic of the two. + +Duplicates are removed before either check, and both functions return documents ordered by id. That ordering is not cosmetic: the same accepted set has to produce the same document list whatever order the client sent it in, because that list is what the next PR writes into the record. + +**What the service does when consent is disabled.** `Documents()` is empty, and `Resolve` and `ResolveAll` accept anything and resolve nothing — ids are ignored rather than rejected. This is the RFC's rule ("`accepted_document_ids` is ignored rather than rejected, so one client build works against both kinds of deployment") implemented in the service rather than left to each caller. The alternative was to make the disabled case an unknown-id error and expose an `Enabled()` accessor for callers to branch on, which puts the same `if` at every call site and gets one of them wrong eventually. **Flagging it as a decision, not a detail:** it means a caller cannot distinguish "disabled" from "nothing configured" through these three functions, and the next PR treats an empty resolved set as "write no consent record", which is the behaviour we want in both cases. + +**Why the endpoint is unauthenticated.** The documents are already public — the URLs are meant to be read by anyone considering an account — and the ids are an input to an unauthenticated `Authenticate`. Requiring a session to learn what to accept before the account exists is a cycle. It leaks nothing about who has an account and nothing that is not on the marketing site already. + +**Why it is not folded into `ListAuthStrategies`.** Consent is not a strategy. `AuthStrategy` carries `name` and `params` and nothing else, so the documents would land in a `params` map that every client has to parse, keyed by convention rather than by schema. Two thin endpoints that each mean one thing beat one endpoint that means two. + +**Disabled returns an empty list, not an error.** A client build works against a deployment that asks for consent and one that does not, and gets the same shaped response from both: no documents means no checkbox. An error would force every client to special-case a deployment configuration it has no business knowing about. + +**Two skip lists, not one.** The ticket names `authenticationSkipList`, and that alone is not enough. `AuthorizationInterceptor` denies by default: an endpoint that is in neither `authorizationSkipEndpoints` nor `authorizationValidationMap` returns `PermissionDenied`, so skipping authentication and stopping there would have produced a public endpoint that always 403s. `ListConsentDocuments` joins both lists, beside `ListAuthStrategies` in each. + +**Config is read at boot, so a version change needs a restart.** That is a real limitation and the RFC records it — a `consent_documents` table with a reconcile kind is the future-work escape hatch. It is the right trade for now: config sits in git, which is a better change log than rows an admin can edit, and every consent record already copies its own versions and URLs, so the records are the version history. + +## Test Plan + +- [x] `TestService_Documents` — every document with all four fields, ordered by id; the ordering asserted over 20 runs so a map-order implementation cannot pass by luck; empty when disabled with documents still configured; empty for the zero config. +- [x] `TestService_Resolve` — known ids map to their snapshots; a subset resolves cleanly, since `Resolve` says nothing about completeness; the result is ordered by id and deduplicated; an unknown id is rejected with `ErrUnknownDocuments` and the error names it and not the valid ids; every unknown id is named; no ids resolves to nothing; disabled ignores ids rather than rejecting them. +- [x] `TestService_ResolveAll` — a complete set passes; duplicates are removed before the check; an incomplete set fails with `ErrMissingDocuments` naming what is missing and not what was sent; an empty set fails; an unknown id fails with `ErrUnknownDocuments`; a set that is both incomplete and unknown reports the unknown one; disabled accepts both an empty and an unknown set. +- [x] `TestConfig_Validate` — a full block passes; a disabled block is not checked, and the same block fails once enabled; enabled with no documents fails; empty id, version and URL each fail and name the document; a URL that does not parse and one that parses but is relative both fail; a missing title passes; a config with two broken documents names the same one over 20 runs. +- [x] `TestConnectHandler_ListConsentDocuments` — all four fields per document with consent enabled, and an empty list with no error when disabled; plus a case pinning that the handler passes the service's ordering through untouched. +- [x] `make lint` — 0 issues. +- [x] `make test` — passes, including `internal/store/postgres` against a Dockerized Postgres. `core/consent` is at 100% statement coverage. +- [ ] `make generate` — **fails on this machine and on a clean checkout alike**: `.mockery.yaml` uses mockery v2 keys (`with-expecter`, `mockname`, `outpkg`) and the installed binary is v3.7.3, which rejects them. Pre-existing and unrelated to this change. `internal/api/v1beta1connect/mocks/consent_service.go` is therefore **hand-written** to the exact shape mockery v2.53.5 produces for a no-argument single-return method (compare `AuthnService.SupportedStrategies`), including the expecter type. It must be regenerated once the mockery config is fixed, and should come back byte-identical or close to it. + +## SQL Safety + +Not applicable. No `*_repository.go` and no `goqu.*` changes, no migration, and no query of any kind: the consent service reads server config held in memory and the handler reads the service. The table this feature eventually writes to landed in an earlier PR and is not touched here. + +## Notes for review + +At 18 files this is above the repo's usual `feat` size, though ~8 of those are tests and docs and the Go surface is smaller than the count suggests. It splits cleanly in two if that is preferred: **config + service** (`core/consent/*`, `pkg/server/config.go`, `cmd/serve.go`, the sample config and the docs) and **handler + skip lists** (`internal/api/v1beta1connect/*`, both interceptor lists). The second half is meaningless without the first, so the order is fixed. + +Three things a reviewer should confirm rather than assume: the absolute-URL requirement, `title` not being required, and the disabled-means-ignore behaviour of `Resolve` / `ResolveAll` described above. + +The configured set here is three documents — Terms & Conditions, privacy policy and the EULA. The RFC's example shows two; it is illustrative, and the sample config and docs carry all three. diff --git a/.pr-drafts/cld-3573.md b/.pr-drafts/cld-3573.md new file mode 100644 index 0000000000..2756e95114 --- /dev/null +++ b/.pr-drafts/cld-3573.md @@ -0,0 +1,84 @@ +# feat(consent): record consent in the same transaction as the user + + + +Part of [RFC 0002: Explicit consent at signup](https://github.com/raystack/frontier/blob/main/docs/rfcs/0002-explicit-consent-at-signup.md). Based on the consent config and service change. + +## Summary + +The previous PR gave a deployment a way to say what it asks people to accept, and a client a way to find that out. This one writes the record. The invariant it establishes is a single sentence: **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. There is no window in which a user exists and their consent does not, and no repair path that could invent one later. + +With `app.consent` disabled nothing changes. `ResolveAll` resolves nothing, an empty document set means write no record, and the create is the plain one it has always been — no transaction, no second insert, no behaviour difference at all. + +The error mapping on both auth RPCs, and the SDK that sends the ids, follow separately. Until the first of those lands, `ErrConsentRequired` surfaces as a 500 from `AuthCallback`, exactly as `ErrLoginUserNotFound` and `ErrSignupUserExists` do today. + +## Changes + +- `user.Repository` and `user.Service` gain `CreateWithTx`, and `UserRepository` gains the postgres implementation. Both create paths now share one insert builder. +- `postgres.UserConsentRepository` with `Create(ctx, tx, consent)` and nothing else, plus the `UserConsent` row model and the JSONB document shape. +- `consent.Consent`, `consent.GrantRequest`, `consent.SourceSignup`, `consent.ErrInvalidGrant` and `consent.ErrConsentExists`. +- `consent.Service` gains a repository and an audit record repository, `Grant(ctx, tx, req)` and `RecordGranted(ctx, granted)`. +- `authenticate.ErrConsentRequired`, the `ConsentService` and `Transactor` interfaces, `UserService.CreateWithTx`, and the transactional write in `getOrCreateUser`. +- `UserConsentGrantedEvent` and `ConsentType` in `pkg/auditrecord`. +- Wiring in `cmd/serve.go`; the consent service is now built after the audit record repository it needs. + +## Technical Details + +**Why a transaction, and why the repositories had to change for it.** The alternative was to write the user, write the consent, and delete the user if the second write failed — which is what the RFC names as the fallback if threading a transaction through the user repository is rejected. It is worse in a specific way: the compensating delete can itself fail, and then the row that the whole feature exists to prevent is sitting in `users` with nothing to say what its owner agreed to. A transaction has no such branch. `pkg/db` has `WithTxn` but carries no transaction on a context, so there is no way to make an existing `Create` transactional invisibly; both repositories need a create that takes a `*sqlx.Tx`, and the caller opens it. + +That is additive — `Create` is untouched in behaviour, and both paths now render the same insert through one builder so they cannot drift. It is also, as the RFC's Limitations says, **the one place this feature reaches outside its own domain**: `core/user` now imports `sqlx`. Say so if that is the wrong trade and the delete-and-log fallback is preferred; it is a contained change either way. + +**Why an existing user gets no record, ever.** This is the part that looks like a missing feature and is not. A consent record carries the timestamp and the IP of the act it describes. If frontier wrote one when an existing user logged in, that record would carry *that* moment's timestamp and IP for an agreement made somewhere else, possibly years earlier, possibly never. That is worse than having no record, because it reads like evidence. So: no record on login, no record for a user who already exists under any intent, and no repair path. Users who predate this feature have no consent record and nothing will ever give them one — the RFC declares that a non-goal and Limitations records the gap. + +The same reasoning exempts the three paths that create a user without a flow. `authenticateWithPassthroughHeader` passes a nil flow, and `organization.Service.AdminCreate` and the `CreateUser` RPC do not go through `getOrCreateUser` at all. No account holder is present on any of them to consent, so a record written there would attest to nothing. There is a test pinning the passthrough case specifically, because it is the one of the three that runs through the changed code and so is the one that could have been gated by accident. + +**Where the completeness check runs, and why it runs here at all.** `ResolveAll` runs at user creation under **every** intent — not for the error, but as the invariant guarding the write. It is the last point before the insert. An unset intent is permissive for the login gate, deliberately, but it is never permissive for consent: a client cannot opt out of a consent record by omitting the intent field the way it can opt out of the login gate. + +Under a signup intent the same check will also run at `Authenticate`, before the redirect and before mail OTP sends anything, so the user retries and loses nothing. That is the handler half and lands with the error mapping PR. This PR is the half that cannot be bypassed. + +**`Grant` has no completeness rule of its own.** It writes a record for the documents it is given, and that is all. `ResolveAll` is what decides a signup payload covers every configured document. Keeping the two apart is what leaves room for a later re-consent covering a single document without a second write path — `source` already separates one occasion from another, and `Grant` already takes the document list as an argument. Nothing here anticipates re-consent beyond not foreclosing it. + +**Why the audit write goes through the repository and not the service.** `auditrecord.Service.Create` calls `enrichUserActor`, which reads `Actor.ID` as a session id and looks the session up. At a signup there is no session yet — the user was created seconds ago — so it returns `ErrActorNotFound` and the record is never written. `AuditRecordRepository.Create` is usable, but it calls `enrichActorFromContext` when the actor is empty, and `Authenticate` and `AuthCallback` are both on `authenticationSkipList`, so there is no actor in the context to enrich from: the record would land with `uuid.Nil` and the `system` actor for an act a person performed. + +So the write goes through the repository with **every field set explicitly**, as `userpat` does for its PAT events. `Actor` is the new user — its id, `app/user`, email as name. `Resource` is the same user. `Target` is the consent record, with the document ids and versions in `Metadata`. `OccurredAt` is `consented_at` from the flow, not the write time. `OrgID` is `schema.PlatformOrgID` rather than blank, matching what `user.Service` and `userpat.Service` already stamp on platform-level events, so no reader has to special-case an event that belongs to no organization. `IdempotencyKey` is empty: it is nullable, and a consent record is written once, so there is nothing to deduplicate. + +**Why it is after the commit, and why a failure is not fatal.** `AuditRecordRepository.Create` has no `*sqlx.Tx` variant, so the audit record cannot be atomic with the consent record. Rather than pretend otherwise, the write happens after the transaction commits and its failure is logged and swallowed. That is the right way round: `user_consents` is the source of truth, and the audit record is a breadcrumb. Failing a signup that has already succeeded, in order to record that it succeeded, would be a strictly worse outcome. `RecordGranted` lives on `consent.Service` rather than in `authenticate`, so the audit record is derivable entirely from the consent record and the whole concern sits in one package. + +Worth naming: `postgres.InsertAuditRecordInTx` does exist, and a future change could make the audit record atomic with the consent by writing it from inside the repository. The RFC chose the after-commit write, and this follows it. Flagging it as a decision rather than an oversight. + +**The disabled case, and why it is an empty set rather than a flag.** `Resolve` and `ResolveAll` return `(nil, nil)` for any input when `app.consent` is off — that was settled in the previous PR, and there is no `Enabled()` accessor to branch on. This PR consumes that directly: an empty resolved document set means write no consent record, and no transaction is opened. A deployment with consent off gets today's code path exactly, and a deployment that configures no documents gets the same, which is what we want from both. + +**Sizing.** 20 files, of which 4 are tests and 4 are mocks; the Go surface is ~7 files. It carries two concerns, and if that is one too many the audit record splits off cleanly as `feat(consent): write an audit record for each consent granted` — `RecordGranted`, the two `pkg/auditrecord` constants, one call site and one test file, which is roughly what #1839 did for the deleter at 4 files. The transactional write is the half that has to land first either way. + +## Test Plan + +- [x] `TestUserConsentRepository/TestCreateIsAtomicWithTheUserRow` — **the "done when" of this PR**, against a Dockerized Postgres rather than a mocked transaction, because a mock can only pretend to roll back. Both inserts succeeding leaves both rows; a consent insert that fails on the `documents_not_empty` CHECK leaves *no* user row, verified by id and by email. +- [x] `TestUserConsentRepository/TestCreate` — a record round-trips all four fields of every document; a missing strategy and IP store as NULL rather than failing the signup; a second signup consent for the same user is rejected by the partial unique index as `ErrConsentExists`; a record naming no document is rejected; a nil transaction is refused rather than dereferenced. +- [x] `TestService_FinishFlow_Consent` — the three outcomes at user creation. A complete payload opens a transaction and writes both, with the grant carrying the flow's strategy and the accepted-at IP and time rather than the callback's. An incomplete payload returns `ErrConsentRequired` with the underlying `ErrMissingDocuments` still wrapped, and neither create is called — the service is built with a nil transactor in that case, so opening one would panic. A flow carrying no consent at all is rejected the same way. An existing user reaches the consent service not at all, asserted by giving it a mock with no expectations. A disabled deployment takes the plain create. A failed grant fails the signup and writes no audit breadcrumb. +- [x] `TestService_PassthroughHeader_Consent` — the exempt path stays exempt with the consent service wired and enabled. +- [x] `TestService_Grant` / `TestService_RecordGranted` — what `Grant` hands the repository, that it has no completeness rule (one document out of three configured writes fine), the source default, the four rejections that never reach the repository, and every audit field including the explicitly-set actor, `occurred_at` being `consented_at`, and the platform org id. Plus: an audit failure does not panic and does not propagate. +- [x] `TestService_CreateWithTx` (core/user) — the transactional create normalises a user exactly as `Create` does. +- [x] `make lint` — 0 issues. +- [x] `make test` — passes, including `internal/store/postgres` against a Dockerized Postgres. `core/consent` is at 100% statement coverage. +- [ ] `make generate` — **fails on this machine and on a clean checkout alike**, pre-existing and unrelated: `.mockery.yaml` uses mockery v2 keys (`with-expecter`, `mockname`, `outpkg`) and the installed binary is v3.7.3, which rejects them. This PR changes `user.Repository` and `authenticate.UserService`, both of which have generated mocks, and adds two interfaces to a package configured with `all: true`. Four mock files are therefore **hand-written or hand-edited** to the exact shape mockery v2.53.5 produces, and must be regenerated once the mockery config is fixed: `core/user/mocks/repository.go`, `core/authenticate/mocks/user_service.go`, `core/authenticate/mocks/consent_service.go` and `core/authenticate/mocks/transactor.go`. They should come back byte-identical or close to it. `core/consent` is not in `.mockery.yaml`, so its two new interfaces are faked in the test file rather than mocked. + +## SQL Safety + +- [x] Values flow through `?` placeholders, `goqu.Ex{}`, or `goqu.Record{}` — never `fmt.Sprintf` or `+` building a query that gets executed. Both new inserts are `dialect.Insert(...).Rows(model)` with a struct carrying `db` tags; every value including the JSONB document list is a bound parameter. No string concatenation anywhere in either repository. The only `fmt.Sprintf` on a query in this PR is in the *test* file, interpolating a `postgres.TABLE_*` constant into a count query with the user id bound as `$1`, which matches how the surrounding repository tests are written. +- [x] `ToSQL()` callers capture and forward params. `UserConsentRepository.Create` and `UserRepository.CreateWithTx` both do `query, params, err := ...ToSQL()` and pass `params...` to `QueryRowxContext`. `buildUserInsertQuery` returns `(string, []any, error)` so the extracted builder cannot drop the params on either path — a `query, _, err :=` there would have been the easy mistake and the signature forbids it. +- [x] No `?` placeholders inside single-quoted SQL literals in `goqu.L`. The one `goqu.L` in the touched code is the pre-existing `goqu.L("now()")` in the user insert, which takes no arguments. +- [x] No `//nolint` or `#nosec` annotations added. + +Two further notes for a SQL reviewer. The consent insert relies on three things the migration already enforces rather than re-checking them in Go: the `documents_not_empty` CHECK, the partial unique index on `(user_id) WHERE source = 'signup'`, and the immutability triggers. `Create` maps the unique violation to `ErrConsentExists` and lets the CHECK violation surface, and the rollback test uses that CHECK deliberately as a real database failure inside a real transaction. And `Create` is the only method on the repository: there is no update, no delete and no soft delete to review, because the triggers would raise `45000` on any of them. + +## Notes for review + +Three things to confirm rather than assume. + +**`auth_strategy` stores `flow.Method`.** The RFC calls it "the flow's own word for it" and gives `oidc`, `mailotp` or `passkey` as examples. `Flow.Method` is the `strategy_name` the client sent, which for OIDC is the *provider key* from `oidc_config` — so a Google signup records `google`, not `oidc`. That is the flow's own word, and it is strictly more informative, but it is not literally what the RFC's example line shows. + +**`Grant` returns the record, not just an error.** The RFC's sketch is `func (s Service) Grant(...) error`. The audit record's `Target` is the consent record id, so the id has to come back; returning the whole record is the smaller change and gives `RecordGranted` everything it needs from one argument. + +**`Target.Name` is the source.** The RFC's field table specifies `Target` as "the consent record id, `consent` type, document ids and versions in `Metadata`" and says nothing about the name. `signup` is what is there, so a reader scanning `target_name` sees the occasion; the metadata carries the ids and versions as specified. diff --git a/.pr-drafts/cld-3575.md b/.pr-drafts/cld-3575.md new file mode 100644 index 0000000000..0ea26eed81 --- /dev/null +++ b/.pr-drafts/cld-3575.md @@ -0,0 +1,82 @@ +# feat(authenticate): map the gate and consent errors on both auth RPCs + + + +Part of [RFC 0002: Explicit consent at signup](https://github.com/raystack/frontier/blob/main/docs/rfcs/0002-explicit-consent-at-signup.md). Based on the transactional consent write. + +## Summary + +The previous three PRs built the machinery: an intent that separates a login from a signup, a consent service that knows what a deployment asks for, and a transaction that makes a user row without a consent record impossible. All three can refuse a request, and until this PR every one of those refusals arrived at the client as a 500. + +This is the PR that makes them legible. The two new request fields reach `StartFlow`, and the three errors get codes a client can act on: `ErrLoginUserNotFound` → `NotFound`, `ErrSignupUserExists` → `AlreadyExists`, `ErrConsentRequired` → `FailedPrecondition`, from **both** `Authenticate` and `AuthCallback`. + +It also settles the question the RFC left open, and settles it as "a code, on both RPCs". The RFC weighed a redirect back to the originating view against rendering in place. Neither applies: frontier serves no route for the callback URL, so `AuthCallback` is called by a page the application hosts, over connect, like every other RPC. It already holds the rejection and decides where the user goes. + +With `app.consent` disabled nothing changes, and a client that sends no intent behaves exactly as it does today. + +## Changes + +- `Authenticate` passes the intent, the accepted document ids and the client IP into `StartFlow`, and maps the three errors to their codes. +- `AuthCallback` maps the same three, joining the fixed list it handles explicitly so they do not fall through to `Internal`. +- `lookupAuthFlowRejection` and `toFlowIntent` in `internal/api/v1beta1connect/authenticate.go`. +- `ErrConsentOnLoginIntent` in `internal/api/v1beta1connect/errors.go`, for ids sent with a login intent. +- `StartFlow` gains `gateFlowConsent`, the consent half of the flow start gate; `authenticate.ConsentService` gains `Resolve` for it. +- No change to `FinishFlow`'s contract: it still returns no response with an error. + +## Technical Details + +**Why `FailedPrecondition` specifically.** The other five errors `AuthCallback` handles explicitly are all `InvalidArgument`: a bad OTP, a missing OIDC code, an invalid state, an expired flow, a failed token exchange, an unknown method. They share a shape — the client sent something wrong, and sending it again will not help. A consent rejection is a different thing. The request was well formed; the *deployment's* precondition was not met, and the client can meet it by asking the user to accept what is missing and retrying. `FailedPrecondition` is the code that says that, and it is what lets a client separate "you did not accept the documents" from "your code was wrong" without parsing a message string. The other two follow the same reasoning at less depth: a login for an address with no account is a `NotFound`, and a signup for an address that has one is an `AlreadyExists`. Both are already the code a REST reader would guess. + +**Why all three had to be handled where they are, or become 500s.** `AuthCallback` has one error block: a fixed list of errors that map to a 4xx, and `default` → `Internal`. Anything not named in that block is a 500 by construction. So the three had to join it — that is not a stylistic preference, it is the difference between a consent rejection reaching the client and a consent rejection reading as a server fault in the logs. They join it with their **own** codes rather than the `InvalidArgument` the rest of the list gets, which is the whole point; a shared 4xx would be better than a 500 but would still leave a client unable to tell the three cases apart. + +**A code and not a redirect, and why that supersedes the RFC.** The RFC's *Errors* section describes returning codes from both RPCs and leaves the OIDC surface undecided, weighing a redirect back to the originating view against rendering in place. This PR answers it: the code is the whole answer, on both RPCs, for every strategy. Read this section as replacing the open question. + +The reason is that `AuthCallback` is never a browser navigation. Frontier's mux serves the connect paths and nothing else — there is no route for `/v1beta1/auth/callback`. The callback URL points at a page the *application* hosts (`http://localhost:3000/callback` by default), the identity provider or the mail link sends the browser to that page, and that page calls `AuthCallback` over connect. So the rejection arrives in JavaScript that is already deciding where the user goes next, and a code is exactly what it needs. + +A redirect was implemented first and then withdrawn, for three reasons worth recording: + +- **Nothing follows it.** `consts.LocationGatewayKey` becomes a `location` response *header* via the session interceptor. No status is rewritten, and the UI proxy is a plain `httputil.NewSingleHostReverseProxy`. A client has to be written to read that header — and none is. The demo callback page ignores it on the success path too. +- **A 2xx carrying it reads as success.** `Callback.tsx` in the demo app sets `isAuthorized(true)` and navigates to `/` on any non-error response, so a rejected signup would mark the user authorized with no session. Worse than the 500 it was replacing. +- **The callback page is consumer code.** `web/sdk/client/views/auth/` ships `magic-link`, `magic-link-verify`, `sign-in` and `sign-up` — no callback view. A mechanism obliging every consumer to implement header-following is a poor way to deliver an error a code already carries. + +**The message is the bare sentinel.** The response carries `ErrConsentRequired.Error()` and never the wrapped error naming which documents were missing or unknown — that stays in the log, where it was already going. The three codes are distinct, so a client separates the cases from the code alone and needs no second vocabulary beside it. + +**The flow start consent gate.** `Authenticate` could not return `ErrConsentRequired` before this PR, because nothing checked consent at flow start — the only check was at user creation, in the previous PR. So `StartFlow` gains `gateFlowConsent`, per the RFC's *Enforcement → Consent*: a signup intent runs `ResolveAll` there, before an OTP is sent and before the browser leaves for the identity provider, so a rejection costs the user a retry and nothing else. An unspecified intent runs `Resolve` instead, which still catches an unknown id before the browser leaves for the provider while completeness waits for user creation, the first point where frontier knows the request will create a user. A login intent checks nothing. This adds `Resolve` to `authenticate.ConsentService`; the method already existed on `consent.Service`. + +The user-creation check from the previous PR is untouched and still runs under every intent. It is the invariant guarding the write; this one is for the error message. + +**Ids with a login intent are rejected, and that rejection is unconditional.** A login writes no consent record, so accepting the ids would leave a client believing it recorded a consent that does not exist. The handler turns it down with `InvalidArgument` before the flow starts. This is deliberately *not* conditional on `app.consent` being enabled: it is a request-shape error, not a consent-content one, and no correct client sends ids on a login against either kind of deployment. The "ignored rather than rejected" rule is about content, and it is already implemented one layer down — `Resolve` and `ResolveAll` return `(nil, nil)` for any input when consent is off, so a signup carrying ids against a deployment that asks for none goes straight through. That is what lets one client build work against both. + +**The IP.** `Authenticate` and `AuthCallback` are both on `authenticationSkipList`, so the interceptor never puts session metadata on the context and the handler has to extract it itself. It calls the same `sessionutils.ExtractSessionMetadata` that `AuthCallback` already calls for the session, and passes only `IpAddress` into `StartFlow`. That helper parses the user agent into an OS and a browser family and drops the raw string, and neither is passed on — the consent record keeps the IP and nothing else, from when the user accepted rather than from the callback. This is the RFC's choice and it disagrees with the Linear project description, which asks for browser details. + +**Nothing is appended to `Flow.FinishURL`, so no redirect target is widened.** Worth stating because the withdrawn design would have. For the record: `FinishURL` is client-supplied via `RegistrationStartRequest.ReturnToURL`, and `StartFlow` does not validate it, but its only caller passes `return_to` through `Service.SanitizeReturnToURL` first — an **exact** match against `config.AuthorizedRedirectURLs`, empty string if the list is unconfigured. It was constrained; it is now also untouched on the rejection path. + +**Rendering any of this is a separate change.** Frontend and backend do not share a PR here. Three surfaces need the codes: the sign-in and sign-up views for what `Authenticate` returns, the application's callback page for what `AuthCallback` returns after OIDC or a mail link, and `OTPVerifyHandler` in `magic-link-verify-view.tsx` for what it returns after a mail OTP — that last one currently shows "Please enter a valid OTP" for every failure, which is the wrong copy for all three. Note the RFC points the SDK ticket at `magicLinkHandler`; that handler only ever sees `Authenticate` rejections. + +## Test Plan + +- [x] `TestConnectHandler_Authenticate_Rejections` — each of the three errors from `StartFlow` reaching the client with its own code, and the message being the bare sentinel: the wrapped error naming the missing document ids does not appear in the response. +- [x] `TestConnectHandler_AuthCallback_Rejections` — all three codes arriving from the callback, no response alongside them, and the message being the bare sentinel with no document id in it. +- [x] `TestConnectHandler_AuthCallback_UnmappedErrorStaysInternal` — the boundary: an error with no code of its own stays a 500 rather than picking up a 4xx it has no claim to. +- [x] `TestConnectHandler_Authenticate_PassesIntentConsentAndIP` — the intent, the ids and the first hop of `x-forwarded-for` reaching `StartFlow`, with nothing from the user agent. +- [x] `TestConnectHandler_Authenticate_RejectsIdsWithALoginIntent` — `InvalidArgument`, and the flow is never started. +- [x] `TestToFlowIntent` — an unknown enum value reading as unspecified, which is the create-or-get behaviour every client had before intents existed. +- [x] `TestService_StartFlow_Consent` — the flow start gate: a complete signup starts the flow; an incomplete one is rejected with the underlying `ErrMissingDocuments` still wrapped and never reaches `SendMail` (the dialer is a bare mock, so a call would fail the test); an unspecified intent checks only that the ids are known; a login reaches the consent service not at all, asserted with a mock that has no expectations; and a **real** `consent.Service` with `Enabled: false` ignores a full id list rather than rejecting it. +- [x] `TestService_FinishFlow_Intent` / `TestService_FinishFlow_Consent` — a rejection returns the error and no response, and provably creates nothing. +- [x] `make lint` — 0 issues. +- [x] `make test` — passes, including `internal/store/postgres` against a Dockerized Postgres. +- [ ] `make generate` — **fails on this branch and on a clean checkout alike**, pre-existing and unrelated: `.mockery.yaml` uses mockery v2 keys (`with-expecter`, `mockname`, `outpkg`) and the installed binary is v3.7.3, which rejects them. Verified against a stashed tree. This PR adds one method to `authenticate.ConsentService`, so **`core/authenticate/mocks/consent_service.go` is hand-edited** — the `Resolve` block is written to the exact shape mockery v2.53.5 produces for `ResolveAll` beside it, and should come back byte-identical once the mockery config is fixed. No other mock changed. + +## SQL Safety + +Not applicable. This PR touches no `*_repository.go` and contains no `goqu` call, no `ToSQL`, and no query of any kind — it is the connect handler, the flow start gate and one mock. The database work for this feature landed in the previous two PRs and their checklists. + +## Notes for review + +Three things to confirm rather than assume. + +**The flow start consent gate is here rather than in the previous PR.** The RFC puts it at `Authenticate` and the previous PR's comments already refer to it as having run, but nothing implemented it. It has to be in this PR: `Done when` requires `ErrConsentRequired` to reach a client from `Authenticate`, and it cannot do that if nothing at flow start ever returns it. It also brings `Resolve` onto `authenticate.ConsentService`. + +**The unconditional rejection of ids on a login intent.** Argued above. If the preference is to make it conditional on `app.consent` being enabled, it is a two-line change — but it would mean a client bug goes unreported against exactly the deployments where it does the least harm, and stays unreported until someone points that build at a deployment that does ask for consent. + +**That a code is enough, with no redirect anywhere.** Argued above from what the server actually routes and what the demo callback page actually does. If a consumer really does want a redirect rather than a code, that is a callback-page concern — it has `FinishURL`'s value in its own configuration and can navigate there itself. Say so if that reasoning has a hole, because reversing it later means reintroducing a parallel error vocabulary. diff --git a/.pr-drafts/cld-3576.md b/.pr-drafts/cld-3576.md new file mode 100644 index 0000000000..f65ea44020 --- /dev/null +++ b/.pr-drafts/cld-3576.md @@ -0,0 +1,92 @@ +# test(e2e): cover intent and consent across every auth strategy + + + +Part of [RFC 0002: Explicit consent at signup](https://github.com/raystack/frontier/blob/main/docs/rfcs/0002-explicit-consent-at-signup.md). Based on the error-mapping change; completes the backend stack. + +## Summary + +Five PRs built this feature and each carried its own unit tests. This one carries the tests none of them could: the matrix that only exists when the intent, the consent rules and all four strategies are running together in one server. + +That distinction is the whole justification for the PR. `core/authenticate/service_test.go` proves the gates fire with a mocked user service. `core/consent/service_test.go` proves `ResolveAll` compares both sets in both directions. `internal/api/v1beta1connect/authenticate_test.go` proves the three errors get their codes from both RPCs. `internal/store/postgres/user_consent_repository_test.go` proves the insert lands and the transaction rolls back. Every one of those holds with the rest of the stack absent — mocked out, or simply not involved. None of them answers the question this PR asks: does a signup that arrives at a running frontier over ConnectRPC, goes out to an identity provider and comes back, end with a user row and a consent record that say the right things — and does a rejected one leave the database exactly as it found it. + +One new file, `test/e2e/regression/consent_test.go`, with two suites, 7 test methods, 26 cases and 1 documented skip. `make e2e-test` runs in 2m39s, of which the two new suites are about 20s. + +Standalone `test(e2e)` PRs are conventional here (#1861, #1883). + +## Changes + +- `test/e2e/regression/consent_test.go`. Nothing else — no production code is touched, and nothing outside `test/e2e/` is. +- `ConsentRegressionTestSuite`, with `app.consent` enabled and two documents configured: `ListConsentDocuments`, the full intent matrix through mail OTP and through OIDC, the passkey gates, and the transactional rollback. +- `ConsentDisabledRegressionTestSuite`, with the block absent: the empty document list, ids ignored rather than rejected, and the unspecified intent behaving exactly as it did before this feature existed. +- `consentHarness`, the machinery both share — a server with every flow-based strategy wired up, mockoidc, a mock callback target, and a database handle. + +## Technical Details + +**Why two servers rather than one.** `app.consent` is read at boot and there is no way to change it on a running server, so "enabled" and "disabled" are two `testbench.Init` calls and two sets of containers. The second suite is not a formality. "With `app.consent` disabled, nothing changes" is a promise the RFC makes to every deployment that has not turned the feature on, and the only thing that can keep it is a server that has not turned the feature on. It costs about 8s. + +**Why the suites read the database directly.** Two of the things this matrix has to assert are, by design, not on the API surface. A rejected signup is asserted by the *absence* of a user row, and nothing reports the absence of a user. And `user_consents` has a `Create` and nothing else — the RFC's *Storage* section is explicit that reporting reads the table directly — so the record a signup wrote is only visible this way. `testbench.Init` fills the config it is handed with the database it started, so the suite opens its own `db.New` handle from it; `billing_test.go` already reaches for a `*db.Client` for the same class of reason. This is not a shortcut around the API: every write under test is performed through the real RPCs, and the handle is only ever read from, except for the one case below. + +**How the rollback case injects a failure, and why it has to.** "A consent insert failure rolls back the user row" is a checklist item with a problem: nothing reachable from the API can produce that failure. `ResolveAll` runs *before* the transaction opens, so a bad payload never starts one; a well-formed write satisfies `documents_not_empty`, satisfies the partial unique index (the user id is brand new), and violates nothing else. There is no request that gets a user insert to succeed and the consent insert beside it to fail. + +So the case installs a `BEFORE INSERT` trigger on `user_consents` that raises for one address, runs a real mail OTP signup, and asserts the outcome: an error, no user row, no consent row. Then it drops the trigger and runs the *same signup again*, which now succeeds — which is what turns "nothing is there" into "the rollback left nothing behind for a retry to collide with". Without that second half the first half is also consistent with the user insert never having run. + +This does not duplicate the rollback case already in `internal/store/postgres/user_consent_repository_test.go`, and it is worth being precise about the difference. That one opens a transaction by hand, calls `CreateWithTx` and `Create` in it, and uses an empty document list to violate `documents_not_empty` — a real Postgres failure in a real transaction, proving the storage layer discards the user row. What it does not touch is `createUser`, `getOrCreateUser`, `applyMailOTP` or the handler. This case proves the wiring above it: that a rollback is what a real signup request reaches when the insert under it fails, and that the client is told so rather than being handed a half-created account it could then log in to. + +**What the passkey column of the matrix actually contains, and what it cannot.** The RFC lists four flow paths to `getOrCreateUser` and asks for one case each. Two of them — `finishPassKeyRegisterMethod` and `finishPassKeyLoginMethod` — are not covered by this PR, and I want to be exact about that rather than round it off, because the two are unreachable for different reasons. + +`finishPassKeyRegisterMethod` validates the attestation *before* it reaches the gate: `webAuth.CreateCredential` at `service.go:695`, `getOrCreateUser` at `:699`. So the gate sits behind a ceremony this suite cannot perform — `AuthCallback` expects a WebAuthn response signed by an authenticator over the challenge the server just issued, there is no authenticator wired up, and mocking the signature would test the mock. + +`finishPassKeyLoginMethod` is the other way round: `getOrCreateUser` at `:760`, `ValidateLogin` at `:780`. The gate is not behind the signature. What blocks it is upstream — reaching that method at all requires a flow of type `login`, which requires an account carrying a `passkey_credentials` blob, and the only thing that writes one is the register ceremony above. Forging one through `UpdateUser` would mean fabricating the state under test. + +So `TestPassKeyFinishIsNotReachableEndToEnd` is a `t.Skip` carrying that explanation, and the gap shows up in a test run rather than only here. **These two cells are not covered at unit level either** — `TestService_FinishFlow_Intent` and `TestService_FinishFlow_Consent` both drive `MailOTPAuthMethod`. What can be said is that both methods call the same `getOrCreateUser` through the same `flowRejection` wrapper as the two strategies this file does cover end to end, which is an argument that the risk is low, not a claim that the cells are tested. Closing them properly means a virtual authenticator — `github.com/descope/virtualwebauthn` is the usual one for go-webauthn — which is a new dependency in a tests-only PR and a decision worth making deliberately rather than inside this one. + +What *is* covered end to end is the passkey half that changed most in this stack: the flow start gates. `StartFlow` used to guess registration from login by looking the user up, and `finishPassKeyLoginMethod` then called `getOrCreateUser`, so a passkey "login" for an unknown address created the account. Four cases pin the replacement — the consent gate firing before the ceremony starts, the signup gate on an address that has an account, the login gate on one that does not, and a login against an account with no stored credential now failing instead of silently registering one, which is a behaviour the previous PR introduced when it replaced a bare type assertion. + +**The OIDC cases are where the second gate is the only gate.** OIDC is the reason the gates exist in two places at all: the address is unknown until the provider asserts it, so nothing can be checked against it at flow start. The suite runs the full round trip — `Authenticate`, queue a profile and a code on mockoidc, follow the authorization endpoint the way a browser would, `AuthCallback` — and asserts all three rejections arrive as their own connect code with no session started, and that the wrapped error naming the missing documents does not reach the response. The unit test asserts the same against a mocked service; this one asserts it survives a real provider and a real flow row. A case with no `return_to` is kept to pin that the finish URL has no bearing on a rejection. + +**The IP assertion is not incidental.** `Authenticate` is on the authentication skip list, so the handler extracts session metadata itself and passes only the IP into `StartFlow`, where it goes onto the flow and survives to the consent record. The suite names the header in config, sends it on every call, and asserts the value comes back out of `user_consents.ip_address`. That is a three-hop path — handler, flow metadata JSONB, insert — and no unit test crosses all three. + +**What each accepted signup is checked for.** One user row, exactly one consent record, `source = signup`, `auth_strategy` matching the flow's own word for itself (`mailotp` for mail OTP, the configured provider name for OIDC), the IP the client sent, and `consented_at` sitting between the start of the case and `created_at` — because it is meant to be when the user accepted, at flow start, not when the row was written. And the document list compared field by field against config: the client sent ids and nothing else, so every title, version and URL in the record came from the server, which is the property the RFC's *The request fields* section is built around. + +## Test Plan + +`make e2e-test` is the gate for this PR, and it is the only thing that runs the new file. + +- [x] `make e2e-test` — 2m39s, all 9 regression suites plus smoke pass. The two new suites run in 11.9s and 8.5s. 26 cases pass, 1 skips with a reason. +- [x] `make lint` — 0 issues. +- [x] `make test` — passes, unchanged by this PR, which adds no non-e2e file. +- [ ] `make generate` — not run: it fails on this branch and on a clean checkout alike, pre-existing and unrelated (`.mockery.yaml` uses mockery v2 keys against an installed v3.7.3). This PR adds no interface and no mock. + +The matrix, stated plainly. + +**Covered end to end, in this PR:** + +| | mail OTP | OIDC | passkey start | passkey finish | +|---|---|---|---|---| +| login, no account | ✅ flow start | ✅ callback | ✅ flow start | ❌ | +| login, account exists | ✅ signs in, no record | — | ✅ fails, no silent register | ❌ | +| signup, no account | ✅ user + record | ✅ user + record | ✅ ceremony starts, nothing written | ❌ | +| signup, account exists | ✅ flow start | ✅ callback | ✅ flow start | ❌ | +| unspecified, no account | ✅ both consent outcomes | ✅ callback | — | ❌ | +| unspecified, account exists | ✅ signs in, no record | — | — | ❌ | + +Consent: the complete set writing both rows ✅; the incomplete set rejected with nothing written ✅; an unknown id rejected ✅; ids sent with a login intent rejected as a client bug ✅; a consent insert failure rolling back the user row ✅; `ListConsentDocuments` enabled ✅ and disabled ✅. Disabled behaviour: ids ignored rather than rejected ✅, unspecified intent create-or-get unchanged ✅, gates still applying ✅. + +**Covered only at unit level, in earlier PRs:** the resolve rules against a disabled service, the boot-time config validation, the URL-building helper's unparseable and replace-existing branches, and the repository's duplicate-signup and immutability behaviour. All of those are properties of one component and are better tested where they live than through a server. + +**Not covered at all, and this PR does not change that:** `finishPassKeyRegisterMethod` and `finishPassKeyLoginMethod`, for the two different reasons above — neither here nor at unit level, since the finish-flow unit tests all drive mail OTP. "A first-time passkey login is a signup" is therefore an untested cell, and the one thing in this ticket's checklist that this PR does not deliver. **Out of scope by design:** the three non-flow user creation paths — `authenticateWithPassthroughHeader`, `organization.Service.AdminCreate` and the `CreateUser` RPC. The RFC exempts them because no account holder is present to consent; `authenticateWithPassthroughHeader` has its own unit case in `TestService_PassthroughHeader_Consent`, and all three are already exercised throughout the existing e2e suites, every one of which runs with consent disabled. Boot-time config validation is out of scope for the same class of reason: it lives in `cmd/serve.go`, has unit coverage in `core/consent/config_test.go`, and an e2e case for it would have to assert that a server fails to start, which `testbench.Init` has no shape for. + +The two blank cells in the table are combinations with nothing of their own to say — an OIDC login for an address that exists is the plain success path, already covered by `authentication_test.go`, and the passkey unspecified rows are the pre-existing guess, unchanged by this stack and covered where the guess lives. + +## SQL Safety + +Not applicable. No `*_repository.go` and no `goqu` call anywhere in this PR — the only SQL is in the test file, and all of it is either a constant `SELECT` with `$1` parameters or the fault-injection trigger, which is a constant string with no interpolation. Deliberately so: a formatted query would trip the linters for no benefit in a fixture, which is why the address the trigger matches is spelled out as a literal in the SQL rather than built with `fmt.Sprintf`, with a comment saying why. + +## Notes for review + +**The trigger.** It is the one place a test writes to the database rather than reading, and it is worth a look. It is created and dropped inside the one test method, matches a single hardcoded address so it cannot affect any other case, and the drop is idempotent and deferred. If injecting a fault this way is not wanted, the honest alternative is to drop the cell from the e2e matrix and say in the RFC that it is unit-only — not to write a case that asserts less than it appears to. + +**The skip, and the decision behind it.** This is the one thing in the ticket's checklist that this PR does not deliver, so it should be an explicit call rather than something noticed later. The two passkey finish methods are untested at every level, and closing that needs a virtual authenticator — a new dependency, and a fair amount of ceremony plumbing, in a PR that otherwise touches nothing but one test file. My read is that it belongs in its own change, and that a `t.Skip` carrying the reason is the honest placeholder in the meantime. If the view is that the matrix should not merge with a hole in it, say so and it becomes a follow-up ticket rather than a skip. What I would not do is delete the skip and leave the gap invisible. + +**Whether the disabled suite earns its containers.** It is about 8s and one more Postgres and SpiceDB pair. The alternative is to fold its four cases into the existing `authentication_test.go` suite, which already runs with consent disabled, and save the boot. That mixes the feature's coverage into a file that predates it, which is why it is separate here — but it is a reasonable thing to disagree with. diff --git a/internal/store/postgres/migrations/20260830100000_create_user_consents.down.sql b/internal/store/postgres/migrations/20260830100000_create_user_consents.down.sql new file mode 100644 index 0000000000..1e3adbd649 --- /dev/null +++ b/internal/store/postgres/migrations/20260830100000_create_user_consents.down.sql @@ -0,0 +1,10 @@ +DROP TRIGGER IF EXISTS trg_user_consents_prevent_delete ON user_consents; +DROP TRIGGER IF EXISTS trg_user_consents_prevent_update ON user_consents; + +DROP FUNCTION IF EXISTS prevent_user_consent_deletes(); +DROP FUNCTION IF EXISTS prevent_user_consent_updates(); + +DROP INDEX IF EXISTS uq_user_consents_signup; + +-- a BEFORE DELETE trigger fires per row and does not block DROP TABLE. +DROP TABLE IF EXISTS user_consents; diff --git a/internal/store/postgres/migrations/20260830100000_create_user_consents.up.sql b/internal/store/postgres/migrations/20260830100000_create_user_consents.up.sql new file mode 100644 index 0000000000..e55a35a1fd --- /dev/null +++ b/internal/store/postgres/migrations/20260830100000_create_user_consents.up.sql @@ -0,0 +1,59 @@ +-- One consent record per consent act, listing the documents it covers. +-- See docs/rfcs/0002-explicit-consent-at-signup.md, "Storage". +CREATE TABLE user_consents ( + id UUID PRIMARY KEY DEFAULT uuid_generate_v7(), + -- no FK to users(id): Delete is a hard DELETE, so CASCADE would drop these + -- records with the account and RESTRICT would block deleting it at all. + user_id UUID NOT NULL, + user_email TEXT NOT NULL, -- denormalized so the record outlives the user row. + documents JSONB NOT NULL, -- [{id, title, version, url}, ...], copied from config at write time. + source TEXT NOT NULL DEFAULT 'signup', + auth_strategy TEXT, + ip_address TEXT, -- TEXT and nullable, not INET: it comes from a request header. + consented_at TIMESTAMPTZ NOT NULL, -- when the user accepted, not when the row was written. + created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + + CONSTRAINT documents_not_empty CHECK ( + jsonb_typeof(documents) = 'array' AND jsonb_array_length(documents) > 0 + ) +); + +-- At most one signup consent per user: nothing repairs a record, so a second +-- write is a bug and should fail rather than leave two rows disagreeing. +CREATE UNIQUE INDEX uq_user_consents_signup + ON user_consents(user_id) WHERE source = 'signup'; + +-- Following 20250904105226_add_audit_records_immutability.up.sql, which guards +-- UPDATE. DELETE is guarded too: a deleted record leaves a user who looks like +-- they never consented. +CREATE OR REPLACE FUNCTION prevent_user_consent_updates() + RETURNS TRIGGER AS $$ +BEGIN + RAISE EXCEPTION 'user_consents cannot be updated to maintain consent integrity' + USING ERRCODE = '45000', -- User-defined error (Postgres convention: user-defined error codes are in the 45000-45999 range) + DETAIL = 'Consent records are immutable once created'; +END; + $$ LANGUAGE plpgsql; + +CREATE TRIGGER trg_user_consents_prevent_update + BEFORE UPDATE ON user_consents + FOR EACH ROW EXECUTE FUNCTION prevent_user_consent_updates(); + +COMMENT ON TRIGGER trg_user_consents_prevent_update ON user_consents IS + 'Enforces immutability of consent records by preventing any UPDATE operation.'; + +CREATE OR REPLACE FUNCTION prevent_user_consent_deletes() + RETURNS TRIGGER AS $$ +BEGIN + RAISE EXCEPTION 'user_consents cannot be deleted to maintain consent integrity' + USING ERRCODE = '45000', -- User-defined error (Postgres convention: user-defined error codes are in the 45000-45999 range) + DETAIL = 'Consent records must outlive the user they describe'; +END; + $$ LANGUAGE plpgsql; + +CREATE TRIGGER trg_user_consents_prevent_delete + BEFORE DELETE ON user_consents + FOR EACH ROW EXECUTE FUNCTION prevent_user_consent_deletes(); + +COMMENT ON TRIGGER trg_user_consents_prevent_delete ON user_consents IS + 'Enforces immutability of consent records by preventing any DELETE operation.';