Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions .pr-drafts/cld-3570.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
# feat(consent): add the user_consents table with immutability triggers

<!-- Draft PR description. Not committed. Stacks on PR #1908. -->

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.
75 changes: 75 additions & 0 deletions .pr-drafts/cld-3571.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
# feat(authenticate): separate login from signup with an explicit intent

<!-- Draft PR description. Not committed. Stacks on the user_consents migration. -->

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.
Loading
Loading