From ece302edd1f2e21ac93a2fd6c19e28e6a7ab00ac Mon Sep 17 00:00:00 2001 From: Raymond Nicholas Date: Wed, 16 Sep 2026 21:18:08 +0100 Subject: [PATCH 1/6] docs: correct the claim that the settings block was the admin surface's first write PUT/DELETE /v1/admin/users/{userID}/metadata/{key} have written since Tier 3, so "the only writes under /v1/admin" and "the first version with admin endpoints that WRITE" were both wrong. Says what the two write blocks actually are instead. Co-Authored-By: Claude Code --- README.md | 2 +- httpapi/router.go | 17 +++++++++++------ openapi/spec.yaml | 22 ++++++++++++---------- 3 files changed, 24 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 21a0cc4..46e1be5 100644 --- a/README.md +++ b/README.md @@ -422,7 +422,7 @@ The LLM API key and the database connection string are sealed with **AES-256-GCM - `DELETE /v1/passkeys/{credentialID}` takes a JSON body (`{"password": "..."}`) — the password is re-confirmation, so a stolen access token alone cannot weaken an account's own auth requirements. - Passkey ceremony options and the browser's credential response travel as raw JSON (an object, not a JSON-encoded string), since that is exactly what `navigator.credentials.create()`/`.get()` produce and consume. - **Five of this repo's tables are not cryden's and never will be**: `user_metadata`, `webhook_deliveries`, `shipped_log_events`, `digest_runs` and `settings`. cryden calls an interface and moves on; it keeps no queryable history of what a sender or a logger did, no schedule, no run record, and no configuration storage — and `store.User` has no metadata concept on purpose. Each lives in its own package (`usermeta/`, `webhook/`, `shiplog/`, `digest/`, `settings/`) with a Postgres store and an in-memory double behind one interface, mirroring the `store/interfaces.go` + `store/memory` + `store/postgres` split cryden itself uses — which is what makes an endpoint over them testable with no database. -- **The admin surface is read-only by construction, with one named exception.** `GET /v1/admin/webhooks/deliveries` and `GET /v1/admin/logging/recent` report; neither offers a "retry this delivery" button, a "replay this event", or any way to write a log record or a delivery row. That is the same rule cryden's AI admin tools are built under, carried across the repo boundary: an operator reads the state of the system, and every change to it goes through the explicit path that owns that change (or through the receiving system, for a delivery). Adding a write here is a design change, not a convenience. **The exception is `/v1/admin/settings/*`**, which is a settings save — the "a human still saves it" half of the pre-fill rule, not an action any AI tool can reach. Its credentials are encrypted at rest, and it is the only place in this API that stores one. If you are adding a write under `/v1/admin` that is not a settings save, the answer is no. +- **The admin surface is read-only by default, and every write on it is a named exception.** `GET /v1/admin/webhooks/deliveries` and `GET /v1/admin/logging/recent` report; neither offers a "retry this delivery" button, a "replay this event", or any way to write a log record or a delivery row. That is the same rule cryden's AI admin tools are built under, carried across the repo boundary: an operator reads the state of the system, and every change to it goes through the explicit path that owns that change (or through the receiving system, for a delivery). Adding a write here is a design change, not a convenience, and there are exactly two of them. `PUT`/`DELETE /v1/admin/users/{userID}/metadata/{key}` writes per-user metadata, which becomes JWT claims — the write *is* the feature, and a read-only version of it would do nothing. `PUT`/`DELETE /v1/admin/settings/*` is a settings save, the "a human still saves it" half of the pre-fill rule, not an action any AI tool can reach; its credentials are encrypted at rest and it is the only place in this API that stores one. Both are an operator acting deliberately on a named thing, and neither is reachable from an AI feature — no tool holds a reference to either handler, and the engine's interfaces carry no method that could call one. If you are adding a write under `/v1/admin` that is neither of these, the answer is no. - `webhook_deliveries.id` is a `BIGSERIAL` surrogate key rather than the natural key you might expect. The event id it corresponds to **can be empty** — cryden generates it with `crypto/rand` and deliberately delivers an event without one rather than dropping it — and a delivery log whose primary key could be blank is a log that loses exactly the rows you would most want to see. The engine's own id is recorded beside it as `event_id` and is used for the receiver's idempotency. - This repo has **no graceful shutdown**, and as of this tier that is a stated gap rather than an unnoticed one: `main.go` ends at `log.Fatal(http.ListenAndServe(...))`, so the webhook worker's context is never cancelled and the shipped-events sink has no flush-and-exit path. Both were built so that adding one later is a change to `main.go` alone — the worker takes a `context.Context`, which today is `context.Background()`. The sink writes synchronously for the same reason: a buffered sink with no shutdown path drops its last records on a crash. diff --git a/httpapi/router.go b/httpapi/router.go index 3f16659..e07baf6 100644 --- a/httpapi/router.go +++ b/httpapi/router.go @@ -199,10 +199,12 @@ func NewRouter(d Deps) http.Handler { // this repo's own logic — cryden has no concept of a provider being // reachable, and no bulk way to read stored hash algorithms. // - // Every endpoint here is read-only, and has to stay that way — the one - // exception is the /v1/admin/settings/* block at the bottom of this - // table, which is a settings save and not an action any AI tool can - // reach. See CLAUDE.md's hard rule about the admin surface. + // Read-only is the default and every write here is deliberate: the + // per-user metadata block below (a write is the whole feature) and the + // settings block at the bottom (a settings save, the one path a tuning + // suggestion may pre-fill). Neither is reachable from an AI tool, which + // is what CLAUDE.md's hard rule actually protects. See README's note on + // the admin surface. mux.HandleFunc("GET /v1/admin/oauth/health", RequireAdmin(engine, oauthHealth.Health)) mux.HandleFunc("GET /v1/admin/security/hash-migration", RequireAdmin(engine, security.HashMigration)) @@ -259,8 +261,11 @@ func NewRouter(d Deps) http.Handler { // settings path. See TuningHandlers and CLAUDE.md's hard rule. mux.HandleFunc("GET /v1/admin/config-tuning", RequireAdmin(engine, tuning.ConfigTuning)) - // The AI settings surface — the only writes under /v1/admin, and the - // "human saves it" half of pre-fill-never-auto-apply. + // The AI settings surface — the "human saves it" half of + // pre-fill-never-auto-apply, and one of exactly two write blocks on + // this surface (the other is the per-user metadata block above; an + // earlier version of this comment claimed there was only one, which + // was wrong). // // The read-only rule above is about the AI *tools*, which are what the // engine's interfaces make read-only by carrying no method that can diff --git a/openapi/spec.yaml b/openapi/spec.yaml index 7519775..b7ec70b 100644 --- a/openapi/spec.yaml +++ b/openapi/spec.yaml @@ -40,18 +40,20 @@ info: human to save rather than being applied, and only a background job this API runs writes the digest history. - 1.5 is additive, and is the first version with admin endpoints that - WRITE: the AI provider settings (GET/PUT/DELETE + 1.5 is additive: the AI provider settings (GET/PUT/DELETE /admin/settings/llm-provider, /admin/settings/database-provider and /admin/settings/ask-ai-widget). No existing path, field or status - code changed. The writes are the "human saves it" half of the - pre-fill rule 1.4 describes — a tuning suggestion pre-fills one of - these forms and an operator saves it — and they are the only place - a credential is stored. PUT database-provider additionally verifies - against the database itself that the supplied role cannot write, - and refuses to store the connection until the server has rejected - one; see that path's description for why the check is done there - and not in the console. + code changed, and 1.5 adds no new kind of thing — 1.3's metadata + PUT/DELETE already wrote, so the admin surface was not read-only + before this version and this version does not change that. The + writes here are the "human saves it" half of the pre-fill rule 1.4 + describes — a tuning suggestion pre-fills one of these forms and an + operator saves it — and they are the only place a credential is + stored. PUT database-provider additionally verifies against the + database itself that the supplied role cannot write, and refuses to + store the connection until the server has rejected one; see that + path's description for why the check is done there and not in the + console. 1.5 is also the first version whose responses depend on a deployment setting that is not a cryden one: all three paths answer From 6ec6c6ea1eec44cda7fdfc47ef1ceee159819cc8 Mon Sep 17 00:00:00 2001 From: Raymond Nicholas Date: Wed, 16 Sep 2026 21:21:31 +0100 Subject: [PATCH 2/6] feat: add the store for reviewing flagged events MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cryden records that a login tripped anomaly signals and has no concept of a human having read one. reviewed_anomalies (014) is that concept, keyed on the audit event id, with status as a column so dismissing keeps the evidence and nothing ever deletes. The foreign key on audit_events is what refuses a review of an event that does not exist — cryden has no lookup-by-event-id, so the database answers that question. Co-Authored-By: Claude Code --- anomalyreview/memory.go | 113 +++++++++ anomalyreview/store.go | 274 +++++++++++++++++++++ anomalyreview/store_test.go | 191 ++++++++++++++ httpapi/errors.go | 12 + migrations/014_reviewed_anomalies.down.sql | 3 + migrations/014_reviewed_anomalies.up.sql | 51 ++++ 6 files changed, 644 insertions(+) create mode 100644 anomalyreview/memory.go create mode 100644 anomalyreview/store.go create mode 100644 anomalyreview/store_test.go create mode 100644 migrations/014_reviewed_anomalies.down.sql create mode 100644 migrations/014_reviewed_anomalies.up.sql diff --git a/anomalyreview/memory.go b/anomalyreview/memory.go new file mode 100644 index 0000000..9231128 --- /dev/null +++ b/anomalyreview/memory.go @@ -0,0 +1,113 @@ +package anomalyreview + +import ( + "context" + "fmt" + "sync" + "time" +) + +// MemoryStore is the in-process Store, for tests and for any embedding +// host that wants the review surface without a database behind it. +// +// It is a faithful double, which here means one specific thing: it has to +// refuse a review of an event that does not exist, because Postgres does +// — through the foreign key on audit_events. A double that accepted any +// id would let a test assert a 404 that production never produces, and +// the branch that actually runs in production would be the untested one. +// +// So it refuses, and it needs to be told what exists. RegisterEvents is +// how. That is a real seam rather than a wart: cryden's AuditStore has no +// lookup-by-event-id, so this package cannot ask the engine whether an +// event is real even in production — the database answers instead, and +// the double answers from what the test declared. +type MemoryStore struct { + mu sync.RWMutex + // rows is keyed by audit event id. A missing key is unreviewed. + rows map[string]Review + // known is the set of audit event ids that exist. Postgres reads this + // from audit_events via the foreign key; here a test supplies it. + known map[string]bool +} + +func NewMemoryStore() *MemoryStore { + return &MemoryStore{ + rows: make(map[string]Review), + known: make(map[string]bool), + } +} + +var _ Store = (*MemoryStore)(nil) + +// RegisterEvents declares that these audit event ids exist, so that Set +// will accept a review of them and refuse one of anything else. It is the +// memory store's stand-in for the foreign key, and is not part of Store +// because nothing in production needs it — Postgres answers the same +// question from the table itself. +func (s *MemoryStore) RegisterEvents(eventIDs ...string) { + s.mu.Lock() + defer s.mu.Unlock() + for _, id := range eventIDs { + s.known[id] = true + } +} + +func (s *MemoryStore) StatusesFor(_ context.Context, eventIDs []string) (map[string]Review, error) { + s.mu.RLock() + defer s.mu.RUnlock() + + out := make(map[string]Review, len(eventIDs)) + for _, id := range eventIDs { + if review, ok := s.rows[id]; ok { + out[id] = review + } + } + return out, nil +} + +func (s *MemoryStore) Set(_ context.Context, eventID string, status Status, note, reviewerID string) (Review, error) { + // Validated with the same call the Postgres store makes, not a + // reimplementation of it, so a status cannot hold in one store and + // not the other. + if err := ValidateStatus(status); err != nil { + return Review{}, err + } + if len(note) > maxNoteLength { + return Review{}, fmt.Errorf("%w: %d characters, the limit is %d", ErrNoteTooLong, len(note), maxNoteLength) + } + + s.mu.Lock() + defer s.mu.Unlock() + + if !s.known[eventID] { + return Review{}, fmt.Errorf("%w: %s", ErrNoSuchEvent, eventID) + } + + review := Review{ + EventID: eventID, + Status: status, + Note: note, + ReviewerID: reviewerID, + UpdatedAt: time.Now(), + } + s.rows[eventID] = review + return review, nil +} + +// Status is a test helper the PostgresStore has no equivalent for: the +// memory store can answer "what did we decide about this event" without a +// round trip, which keeps a failing assertion readable. Not part of +// Store, because nothing in production reads one row at a time. +func (s *MemoryStore) Status(eventID string) (Status, bool) { + s.mu.RLock() + defer s.mu.RUnlock() + review, ok := s.rows[eventID] + return review.Status, ok +} + +// Len is how many events have a recorded review, of any status. +func (s *MemoryStore) Len() int { + s.mu.RLock() + defer s.mu.RUnlock() + return len(s.rows) +} diff --git a/anomalyreview/store.go b/anomalyreview/store.go new file mode 100644 index 0000000..30b9308 --- /dev/null +++ b/anomalyreview/store.go @@ -0,0 +1,274 @@ +// Package anomalyreview records what a human decided about an event +// cryden flagged, and owns the rule about which decisions exist. +// +// It exists because the engine stops one step short of it. cryden +// records that a login tripped anomaly signals, or that one IP sprayed +// many accounts, and it says so in prose: an anomaly event "annotates a +// login that was allowed to proceed, it is never a rejection". What it +// has no concept of is a person having read one — so "we looked at this +// and it was fine" lives here, in this repo's own table +// (migrations/014_reviewed_anomalies.up.sql), keyed on the audit event +// id. +// +// Two properties are the whole design, and both are about not losing +// evidence: +// +// - A review is a row in this table, never a write to cryden's audit +// history. The event reads exactly the same before and after +// somebody dismisses it. +// - Nothing here deletes. Dismissing sets a status; withdrawing a +// judgement sets it back to StatusUnreviewed, which is stored rather +// than represented by a missing row, so the record that someone +// looked and who they were survives the change of mind. +// +// This is a write, and it is deliberately not in tension with CLAUDE.md's +// read-only rule. That rule is about the AI-assisted tools, which are +// read-only because the interfaces they are built from carry no method +// that can act. This package is not reachable from any of them: it is +// the console's own record of an operator's judgement, the same shape as +// the per-user metadata endpoints beside it. +package anomalyreview + +import ( + "context" + "database/sql" + "errors" + "fmt" + "time" + + "github.com/lib/pq" +) + +// The three ways a call here can be refused. +var ( + // ErrNoSuchEvent means there is no audit event with that id, so + // there is nothing to review. Reported rather than stored, so a + // console that acted on a stale list is told instead of being shown + // a success that annotates nothing. + ErrNoSuchEvent = errors.New("anomalyreview: no audit event with that id") + // ErrInvalidStatus means the status is not one this api defines. + ErrInvalidStatus = errors.New("anomalyreview: unknown review status") + // ErrNoteTooLong means the note is longer than a note needs to be. + ErrNoteTooLong = errors.New("anomalyreview: note is too long") +) + +// Status is one reviewer's judgement about one flagged event. +type Status string + +const ( + // StatusUnreviewed is "nobody has called this, or the last call was + // withdrawn". It is a stored value rather than the absence of a row, + // which is what lets a withdrawn judgement keep its attribution + // instead of erasing itself. + StatusUnreviewed Status = "unreviewed" + + // StatusConfirmed means an operator read the event and judged it a + // real incident. It records the judgement and nothing else — no + // action follows from it automatically, because there is nothing in + // this repo that can act on an account beyond what an operator does + // by hand. + StatusConfirmed Status = "confirmed" + + // StatusDismissed means an operator read the event and judged it + // noise. The event stays exactly where it was; only this table + // changes. + StatusDismissed Status = "dismissed" +) + +// Statuses returns every status this api defines, in the order a console +// should offer them. +func Statuses() []Status { + return []Status{StatusUnreviewed, StatusConfirmed, StatusDismissed} +} + +// maxNoteLength bounds a note. Bounded because an unbounded free-text +// field on an admin endpoint is a way to fill a table through a form, +// and generous because the point of the field is that an operator can +// explain themselves. +const maxNoteLength = 500 + +// ValidateStatus is the rule about which decisions exist, in one place. +// Both stores call it before they write, the same discipline +// usermeta.ValidateKey uses: a future caller — a second endpoint, a +// bootstrap command, a migration — goes through a Store and cannot get a +// different answer by not knowing about this function. +// +// It does not trust the CHECK constraint to be the rule. The constraint +// is the backstop that no writer can bypass; this is the version that can +// say which value was wrong and why. +func ValidateStatus(status Status) error { + switch status { + case StatusUnreviewed, StatusConfirmed, StatusDismissed: + return nil + default: + return fmt.Errorf("%w: %q must be one of %q, %q or %q", + ErrInvalidStatus, status, StatusUnreviewed, StatusConfirmed, StatusDismissed) + } +} + +// Review is one recorded decision about one flagged event. +type Review struct { + // EventID is cryden's audit event id — the primary key here, and the + // only key the console ever shows an operator. + EventID string + + Status Status + + // Note is the reviewer's own words. Empty is normal. + Note string + + // ReviewerID is the operator who made the call, empty when the + // account behind it has since been deleted (the column is ON DELETE + // SET NULL). An empty value here therefore means "attributed to an + // account that no longer exists", never "unattributed" — a review is + // only ever written by an authenticated operator. + ReviewerID string + + // UpdatedAt is when this decision replaced the previous one. + UpdatedAt time.Time +} + +// Store is the persistence this package offers. An interface with two +// implementations for the same reason every other store in this repo +// has one: the handlers have to be testable without a database, and a +// double written against the same contract is the only honest way to do +// that. +type Store interface { + // StatusesFor returns the review of each id in eventIDs that has + // one. An id with no row is absent from the map, and absent means + // unreviewed — the same thing a missing row has always meant here. + // + // Takes a slice rather than one id because its only caller is a list + // endpoint annotating a page of events; a per-event call would be + // one query per row on the busiest read on this surface. + StatusesFor(ctx context.Context, eventIDs []string) (map[string]Review, error) + + // Set records one decision, creating it or replacing the previous + // one. Returns the stored review, so a caller can answer with what + // actually landed rather than with what it hoped would. + // + // A status of StatusUnreviewed is a real write, not a delete: it + // records that this operator withdrew the last judgement, and when. + Set(ctx context.Context, eventID string, status Status, note, reviewerID string) (Review, error) +} + +// PostgresStore is the real store. Constructed once in main.go with the +// same *sql.DB every other store in this repo gets. +type PostgresStore struct { + db *sql.DB +} + +func NewStore(db *sql.DB) *PostgresStore { + return &PostgresStore{db: db} +} + +var _ Store = (*PostgresStore)(nil) + +func (s *PostgresStore) StatusesFor(ctx context.Context, eventIDs []string) (map[string]Review, error) { + // Short-circuited rather than sent as an empty array: `= ANY('{}')` + // is a valid query that matches nothing, so this is not a + // correctness fix, it is a round trip an empty page does not need. + if len(eventIDs) == 0 { + return map[string]Review{}, nil + } + + rows, err := s.db.QueryContext(ctx, ` + SELECT event_id, status, note, reviewer_id, updated_at + FROM reviewed_anomalies + WHERE event_id = ANY($1) + `, pq.Array(eventIDs)) + if err != nil { + return nil, err + } + defer rows.Close() + + out := make(map[string]Review, len(eventIDs)) + for rows.Next() { + review, err := scanReview(rows) + if err != nil { + return nil, err + } + out[review.EventID] = review + } + return out, rows.Err() +} + +func (s *PostgresStore) Set(ctx context.Context, eventID string, status Status, note, reviewerID string) (Review, error) { + if err := ValidateStatus(status); err != nil { + return Review{}, err + } + if len(note) > maxNoteLength { + return Review{}, fmt.Errorf("%w: %d characters, the limit is %d", ErrNoteTooLong, len(note), maxNoteLength) + } + + // Empty is stored as SQL NULL rather than as an empty uuid, matching + // cryden's own audit store: "" is not a uuid, and a real NULL is the + // honest representation of "this account is gone". + var reviewer sql.NullString + if reviewerID != "" { + reviewer = sql.NullString{String: reviewerID, Valid: true} + } + + // One statement rather than a read-then-write, so two operators + // deciding on the same event at the same moment cannot lose one of + // the decisions. RETURNING is what makes the answer the stored row + // rather than a reconstruction of it. + row := s.db.QueryRowContext(ctx, ` + INSERT INTO reviewed_anomalies (event_id, status, note, reviewer_id, updated_at) + VALUES ($1, $2, $3, $4, now()) + ON CONFLICT (event_id) DO UPDATE + SET status = EXCLUDED.status, note = EXCLUDED.note, + reviewer_id = EXCLUDED.reviewer_id, updated_at = now() + RETURNING event_id, status, note, reviewer_id, updated_at + `, eventID, string(status), note, reviewer) + + review, err := scanReview(row) + if err != nil { + // The foreign key doing the job this repo cannot do in Go: + // cryden exposes no lookup-by-event-id, so "does this event + // exist" is answered by the database refusing the write. A + // caller reads it as a 404. + // + // Note what this does NOT catch: an id that is not a uuid at all + // fails as an invalid-input-syntax error, not as a foreign-key + // violation, so it stays a 500 here. That is deliberate — a + // malformed path segment is refused by looksLikeUUID in the + // handler, where it is an input-shape question, and reaching + // Postgres with one is a bug in this repo rather than a bad + // request. + var pqErr *pq.Error + if errors.As(err, &pqErr) && pqErr.Code == foreignKeyViolation { + return Review{}, fmt.Errorf("%w: %s", ErrNoSuchEvent, eventID) + } + return Review{}, err + } + return review, nil +} + +// foreignKeyViolation is SQLSTATE 23503, matched by code rather than by +// message — the same reason aiprovider's privilege check matches 42501 +// by code: the message is localized and version-dependent, the code is +// specified. +const foreignKeyViolation = "23503" + +// rowScanner is the part of *sql.Row and *sql.Rows that scanReview +// needs, so one scan serves both the SELECT and the INSERT ... RETURNING. +type rowScanner interface { + Scan(dest ...any) error +} + +func scanReview(src rowScanner) (Review, error) { + var ( + review Review + status string + reviewer sql.NullString + ) + if err := src.Scan(&review.EventID, &status, &review.Note, &reviewer, &review.UpdatedAt); err != nil { + return Review{}, err + } + review.Status = Status(status) + if reviewer.Valid { + review.ReviewerID = reviewer.String + } + return review, nil +} diff --git a/anomalyreview/store_test.go b/anomalyreview/store_test.go new file mode 100644 index 0000000..531fee8 --- /dev/null +++ b/anomalyreview/store_test.go @@ -0,0 +1,191 @@ +package anomalyreview + +import ( + "context" + "errors" + "strings" + "testing" +) + +const ( + eventA = "01a0a4ce-5453-78d3-9126-52268da8da51" + eventB = "01a0a4ce-5453-78d3-9126-52268da8da52" +) + +func reviewFixture() *MemoryStore { + s := NewMemoryStore() + s.RegisterEvents(eventA, eventB) + return s +} + +// The rule has to be exact: a status this api does not define must not be +// storable, and "Confirmed" is not "confirmed". +func TestValidateStatusAcceptsExactlyTheThreeDefinedStatuses(t *testing.T) { + for _, status := range Statuses() { + if err := ValidateStatus(status); err != nil { + t.Errorf("ValidateStatus(%q) = %v, want nil", status, err) + } + } + + for _, status := range []Status{"", "Confirmed", "DISMISSED", "reviewed", "resolved", "deleted", "true"} { + err := ValidateStatus(status) + if !errors.Is(err, ErrInvalidStatus) { + t.Errorf("ValidateStatus(%q) = %v, want ErrInvalidStatus", status, err) + } + // The message names the offending value and the permitted ones, + // so an operator reading it does not have to guess. + if err != nil && !strings.Contains(err.Error(), string(status)) { + t.Errorf("ValidateStatus(%q) message = %q, want it to name the value", status, err) + } + } +} + +func TestSetRecordsAReviewAndReadsItBack(t *testing.T) { + s := reviewFixture() + ctx := context.Background() + + stored, err := s.Set(ctx, eventA, StatusConfirmed, "matches the report from the customer", eventB) + if err != nil { + t.Fatalf("Set: %v", err) + } + if stored.EventID != eventA || stored.Status != StatusConfirmed { + t.Errorf("stored = %+v, want a confirmed review of %s", stored, eventA) + } + if stored.UpdatedAt.IsZero() { + t.Error("stored.UpdatedAt is zero — the store assigns it, the caller does not") + } + + reviews, err := s.StatusesFor(ctx, []string{eventA, eventB}) + if err != nil { + t.Fatalf("StatusesFor: %v", err) + } + if len(reviews) != 1 { + t.Fatalf("reviews = %v, want only the one event that has a review", reviews) + } + got := reviews[eventA] + if got.Status != StatusConfirmed || got.Note != "matches the report from the customer" || got.ReviewerID != eventB { + t.Errorf("review = %+v, want the note and reviewer to survive the round trip", got) + } +} + +// A second decision replaces the first rather than accumulating rows. +func TestSetReplacesThePreviousDecision(t *testing.T) { + s := reviewFixture() + ctx := context.Background() + + if _, err := s.Set(ctx, eventA, StatusConfirmed, "", eventB); err != nil { + t.Fatalf("Set (confirmed): %v", err) + } + second, err := s.Set(ctx, eventA, StatusDismissed, "false positive, new laptop", eventB) + if err != nil { + t.Fatalf("Set (dismissed): %v", err) + } + if second.Status != StatusDismissed { + t.Errorf("status = %q, want the second decision to win", second.Status) + } + if s.Len() != 1 { + t.Errorf("stored reviews = %d, want 1 — a change of mind is not a second row", s.Len()) + } +} + +// The property the whole table exists for: withdrawing a judgement stores +// a status, it does not delete the row. If this ever becomes a delete, +// the record of who looked at the event and when is gone. +func TestWithdrawingAReviewKeepsTheRow(t *testing.T) { + s := reviewFixture() + ctx := context.Background() + + if _, err := s.Set(ctx, eventA, StatusConfirmed, "looked real", eventB); err != nil { + t.Fatalf("Set: %v", err) + } + if _, err := s.Set(ctx, eventA, StatusUnreviewed, "", eventB); err != nil { + t.Fatalf("Set (withdraw): %v", err) + } + + if s.Len() != 1 { + t.Fatalf("stored reviews = %d, want the row to survive being set back to unreviewed", s.Len()) + } + status, ok := s.Status(eventA) + if !ok { + t.Fatal("the row was deleted — unreviewed is a status, not an absence") + } + if status != StatusUnreviewed { + t.Errorf("status = %q, want %q", status, StatusUnreviewed) + } +} + +// The Postgres store refuses this through the foreign key on +// audit_events. The double has to refuse it too, or a handler test would +// assert a 404 that production never produces. +func TestSetRefusesAnEventThatDoesNotExist(t *testing.T) { + s := reviewFixture() + + _, err := s.Set(context.Background(), "01a0a4ce-5453-78d3-9126-000000000000", StatusConfirmed, "", eventB) + if !errors.Is(err, ErrNoSuchEvent) { + t.Fatalf("error = %v, want ErrNoSuchEvent", err) + } + if s.Len() != 0 { + t.Errorf("stored reviews = %d, want nothing written for an event that does not exist", s.Len()) + } +} + +// A refused write is refused rather than written-and-reported, which is +// the difference between a rule and a warning. +func TestRefusedWritesStoreNothing(t *testing.T) { + s := reviewFixture() + ctx := context.Background() + + if _, err := s.Set(ctx, eventA, Status("resolved"), "", eventB); !errors.Is(err, ErrInvalidStatus) { + t.Errorf("error = %v, want ErrInvalidStatus", err) + } + if _, err := s.Set(ctx, eventA, StatusConfirmed, strings.Repeat("x", maxNoteLength+1), eventB); !errors.Is(err, ErrNoteTooLong) { + t.Errorf("error = %v, want ErrNoteTooLong", err) + } + + if s.Len() != 0 { + t.Errorf("stored reviews = %d, want none", s.Len()) + } +} + +// Exactly at the bound is allowed: the limit is a limit, not a reason to +// be one short of it. +func TestANoteAtTheLengthLimitIsAccepted(t *testing.T) { + s := reviewFixture() + + note := strings.Repeat("x", maxNoteLength) + if _, err := s.Set(context.Background(), eventA, StatusConfirmed, note, eventB); err != nil { + t.Fatalf("Set with a %d-character note: %v", maxNoteLength, err) + } +} + +// The list endpoint hands over a page of event ids and gets back only +// those that have reviews. An empty map rather than nil, so a caller can +// range over it without a nil check and a JSON response renders {} not +// null — the same contract usermeta.Store.AllFor keeps. +func TestStatusesForReturnsOnlyTheIdsAskedAbout(t *testing.T) { + s := reviewFixture() + ctx := context.Background() + + if _, err := s.Set(ctx, eventA, StatusDismissed, "", eventB); err != nil { + t.Fatalf("Set: %v", err) + } + + reviews, err := s.StatusesFor(ctx, []string{eventB}) + if err != nil { + t.Fatalf("StatusesFor: %v", err) + } + if len(reviews) != 0 { + t.Errorf("reviews = %v, want nothing — %s was not asked about", reviews, eventA) + } + + empty, err := s.StatusesFor(ctx, nil) + if err != nil { + t.Fatalf("StatusesFor(nil): %v", err) + } + if empty == nil { + t.Error("StatusesFor(nil) returned nil, want an empty non-nil map") + } + if len(empty) != 0 { + t.Errorf("StatusesFor(nil) = %v, want empty", empty) + } +} diff --git a/httpapi/errors.go b/httpapi/errors.go index 6b17597..f17c81d 100644 --- a/httpapi/errors.go +++ b/httpapi/errors.go @@ -11,6 +11,7 @@ import ( "github.com/crydensync/cryden/v2/token" "github.com/crydensync/api/aiprovider" + "github.com/crydensync/api/anomalyreview" "github.com/crydensync/api/settings" "github.com/crydensync/api/usermeta" ) @@ -153,6 +154,17 @@ func mapError(err error) apiError { return apiError{http.StatusBadRequest, "invalid_metadata_key", "a metadata key must start with a letter or underscore and contain only letters, digits, underscores, dots and dashes, up to 64 characters"} case errors.Is(err, usermeta.ErrNotFound): return apiError{http.StatusNotFound, "metadata_key_not_found", "no such metadata key on this user"} + // A review of a flagged event. "No such audit event" is answered by the + // database rather than by Go — cryden exposes no lookup-by-event-id, so + // the foreign key on reviewed_anomalies is what refuses it (see + // anomalyreview.PostgresStore.Set). It reads as 404 because that is what + // it is: a console acting on a stale list, not a server fault. + case errors.Is(err, anomalyreview.ErrNoSuchEvent): + return apiError{http.StatusNotFound, "audit_event_not_found", "no such audit event, so there is nothing to review"} + case errors.Is(err, anomalyreview.ErrInvalidStatus): + return apiError{http.StatusBadRequest, "invalid_review_status", "a review status must be one of unreviewed, confirmed or dismissed"} + case errors.Is(err, anomalyreview.ErrNoteTooLong): + return apiError{http.StatusBadRequest, "invalid_review_note", "that note is longer than the 500-character limit"} // The settings behind the AI-assisted admin features. The two invalid // cases are a form the operator can fix, so they say which field and // which bound rather than a generic "bad request" — the caller is diff --git a/migrations/014_reviewed_anomalies.down.sql b/migrations/014_reviewed_anomalies.down.sql new file mode 100644 index 0000000..b916aee --- /dev/null +++ b/migrations/014_reviewed_anomalies.down.sql @@ -0,0 +1,3 @@ +-- 014_reviewed_anomalies.down.sql + +DROP TABLE IF EXISTS reviewed_anomalies; diff --git a/migrations/014_reviewed_anomalies.up.sql b/migrations/014_reviewed_anomalies.up.sql new file mode 100644 index 0000000..b881f4c --- /dev/null +++ b/migrations/014_reviewed_anomalies.up.sql @@ -0,0 +1,51 @@ +-- 014_reviewed_anomalies.up.sql +-- +-- What a human decided about an event cryden flagged. The engine records +-- the signals (anomaly_detected, credential_stuffing_detected — see its +-- security.AnomalySignal) and has no concept of anyone having looked at +-- one; this table is that concept. It is this repo's own, and cryden's +-- audit_events rows are never touched to record a review: the evidence +-- reads the same before and after one, which is the entire reason a +-- review is a row here and not a write there. +-- +-- The primary key is the audit event's own id rather than a surrogate. +-- That is the identity the console shows and the identity an operator +-- acts on, so a second id would be a second thing to look up and a +-- second thing to get wrong. +-- +-- status is a column and 'unreviewed' is one of its values, rather than +-- an absence of rows. Two things fall out of that and both are wanted: +-- dismissing a flagged event keeps the evidence (which is what makes it +-- a dismissal and not a delete), and withdrawing a judgement keeps the +-- record that the first judgement was made and by whom. Nothing in this +-- api deletes from this table. + +CREATE TABLE reviewed_anomalies ( + -- The foreign key is deliberate, and it is the one place this table + -- reads cryden's schema: it makes the database itself refuse a + -- review of an event that does not exist, which is a check this + -- repo cannot make in Go — cryden exposes no lookup-by-event-id, + -- only list/search by user or type. It constrains writes here and + -- never there, and CASCADE is right in the only direction that can + -- matter: an event that is gone has nothing to annotate. + event_id UUID PRIMARY KEY REFERENCES audit_events(id) ON DELETE CASCADE, + -- 'confirmed' (a real incident), 'dismissed' (a false positive) or + -- 'unreviewed' (a judgement withdrawn). CHECK rather than free + -- text, so a status this api does not define cannot be stored by + -- any writer, including one that is not this package. + status TEXT NOT NULL CHECK (status IN ('confirmed', 'dismissed', 'unreviewed')), + -- What the reviewer wrote. Empty is the normal case: this is a + -- note, not a justification anyone is required to give. + note TEXT NOT NULL DEFAULT '', + -- The operator who made the call, taken from the token this api + -- issued. Nullable and ON DELETE SET NULL, matching + -- audit_events.user_id and for the same reason: a review has to + -- outlive the account that made it, and losing the attribution is + -- better than losing the judgement. + reviewer_id UUID REFERENCES users(id) ON DELETE SET NULL, + updated_at TIMESTAMPTZ NOT NULL DEFAULT now() +); + +-- No index beyond the primary key. Every read here is by event_id (the +-- PK) and every write is an upsert on it, so an index on status or +-- reviewer_id would be one nothing reads. From 45269cf1b777c69b7242550a2546b516e55722ae Mon Sep 17 00:00:00 2001 From: Raymond Nicholas Date: Wed, 16 Sep 2026 21:25:43 +0100 Subject: [PATCH 3/6] feat: add the admin user lookup and detail endpoints GET /v1/admin/users searches by exact email through cryden.GetUser, or pages every account newest-first. GET /v1/admin/users/{userID} reports one account with its live session count and recent audit history. The DTOs leave PasswordHash behind and the detail view can report a lockout but not clear one, so this surface cannot lock anyone out. Co-Authored-By: Claude Code --- httpapi/query.go | 13 + httpapi/router.go | 22 ++ httpapi/user_handlers.go | 303 ++++++++++++++++++ httpapi/user_handlers_test.go | 562 ++++++++++++++++++++++++++++++++++ main.go | 10 +- 5 files changed, 909 insertions(+), 1 deletion(-) create mode 100644 httpapi/user_handlers.go create mode 100644 httpapi/user_handlers_test.go diff --git a/httpapi/query.go b/httpapi/query.go index 14391d8..9904593 100644 --- a/httpapi/query.go +++ b/httpapi/query.go @@ -44,6 +44,19 @@ func queryLimit(r *http.Request) (int, error) { return queryInt(r, "limit", defaultListLimit, 1, maxListLimit) } +// maxListOffset bounds how far into a list a caller may page. Not a +// correctness bound — an offset costs the database the same whether it is +// 10 or 10,000 — but an unbounded one is a way to make a list endpoint +// walk a whole table one request at a time, and nothing in a console +// reads that far in. +const maxListOffset = 10000 + +// queryOffset reads the optional offset query parameter, defaulting to the +// first page. +func queryOffset(r *http.Request) (int, error) { + return queryInt(r, "offset", 0, 0, maxListOffset) +} + // queryString reads an optional, trimmed string query parameter. func queryString(r *http.Request, name string) string { return strings.TrimSpace(r.URL.Query().Get(name)) diff --git a/httpapi/router.go b/httpapi/router.go index e07baf6..c9b4ee4 100644 --- a/httpapi/router.go +++ b/httpapi/router.go @@ -43,6 +43,11 @@ type Deps struct { Audit store.AuditStore Users store.UserStore + // Sessions backs the live-session count on GET + // /v1/admin/users/{userID}. The same instance the engine holds, so + // the count describes the sessions the engine would revoke. + Sessions store.SessionStore + // Meta backs the per-user metadata endpoints. This repo's own table // and package — cryden's store.User has no metadata concept and will // not gain one (see usermeta's package doc). @@ -99,6 +104,7 @@ func NewRouter(d Deps) http.Handler { apiKeys := &APIKeyHandlers{Engine: engine} security := &SecurityHandlers{Audit: d.Audit, Users: d.Users, Config: d.Config} metadata := &MetadataHandlers{Users: d.Users, Meta: d.Meta} + users := &UserHandlers{Engine: engine, Users: d.Users, Sessions: d.Sessions, Audit: d.Audit} hooks := &WebhookHandlers{Store: d.Hooks} logging := &LoggingHandlers{Store: d.Shipped} digests := &DigestHandlers{Engine: engine, Store: d.Digests} @@ -208,6 +214,22 @@ func NewRouter(d Deps) http.Handler { mux.HandleFunc("GET /v1/admin/oauth/health", RequireAdmin(engine, oauthHealth.Health)) mux.HandleFunc("GET /v1/admin/security/hash-migration", RequireAdmin(engine, security.HashMigration)) + // The user surface — finding an account, and reading one account's + // state. Read-only: there is no lock, unlock, password reset or + // delete here, deliberately (see UserHandlers). This is the only + // place an operator sees an account that is not their own, so the + // detail view reports a lockout and cannot clear one, and shows a + // session count rather than an account's devices. + // + // GET /v1/admin/users is registered without a trailing segment and + // the detail route with one, which Go's ServeMux distinguishes; the + // metadata routes below are more specific still and win over the + // detail route for their own paths. + mux.HandleFunc("GET /v1/admin/users", RequireAdmin(engine, users.List)) + mux.HandleFunc("GET /v1/admin/users/{userID}", RequireAdmin(engine, func(w http.ResponseWriter, r *http.Request) { + users.Detail(w, r, r.PathValue("userID")) + })) + // Per-user metadata — the table behind JWT claim mapping. Per key // rather than a whole-map PUT, so two operators editing different // fields of one user cannot overwrite each other's work. The user id diff --git a/httpapi/user_handlers.go b/httpapi/user_handlers.go new file mode 100644 index 0000000..8326157 --- /dev/null +++ b/httpapi/user_handlers.go @@ -0,0 +1,303 @@ +package httpapi + +import ( + "errors" + "net/http" + "time" + + "github.com/crydensync/cryden/v2" + "github.com/crydensync/cryden/v2/store" +) + +// UserHandlers answers the admin user surface: finding an account, and +// reading the state of one. +// +// This is the one place in this API where an operator can see an account +// that is not their own, so what is *not* here matters as much as what is. +// Two things are deliberately absent: +// +// - PasswordHash. It is on the store.User every handler here receives — +// this repo does not get to choose what cryden's struct carries — so +// it is left out at the DTO boundary and never reaches a response. +// TestAdminUserResponsesNeverCarryAPasswordHash asserts that against +// the raw body, because a struct tag is not a guarantee. +// - Anything that changes an account. There is no lock, no unlock, no +// password reset and no delete on this surface. cryden exposes +// LockAccount on the store, and wiring it to a button would make this +// repo the thing that can lock somebody out of their account; an +// operator who needs that has the engine's own admin path, not an +// HTTP endpoint this repo invented. The detail view reports lockout +// state so it can be diagnosed, and stops there. +type UserHandlers struct { + // Engine is used for exactly one call: cryden.GetUser, the engine's + // own public facade for an exact-email lookup. Reaching for + // h.Users.GetByEmail directly would work and would be the wrong + // choice — the facade is the engine's stated interface for this, and + // it is the one that keeps working if the lookup grows a step. + Engine *cryden.Engine + + // Users, Sessions and Audit are the same store instances main.go + // handed cryden, for the reason httpapi.Deps gives: a report over a + // second store object is a report over different data. + Users store.UserStore + Sessions store.SessionStore + Audit store.AuditStore +} + +// userActivityLimit is how much of an account's audit history the detail +// view returns. A console detail pane, not an export: the full history is +// paginated by nothing here, and returning "everything that ever happened +// to this account" is a response whose size is chosen by whoever attacked +// it hardest. +const userActivityLimit = 20 + +// adminUserDTO is one account as the console sees it. +// +// Locked is computed rather than mirrored from LockedUntil, and that is +// the one subtle thing here: cryden clears a lockout by time passing, not +// by writing a null, so a row can carry a locked_until that is already in +// the past. Reporting `locked: true` for a non-nil locked_until would tell +// an operator an account is locked out when it is not — and the ones that +// look like that are exactly the ones somebody just waited out. +type adminUserDTO struct { + ID string `json:"id"` + Email string `json:"email"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` + FailedAttempts int `json:"failed_attempts"` + Locked bool `json:"locked"` + LockedUntil *time.Time `json:"locked_until"` +} + +func newAdminUserDTO(u store.User, now time.Time) adminUserDTO { + return adminUserDTO{ + ID: u.ID, + Email: u.Email, + CreatedAt: u.CreatedAt, + UpdatedAt: u.UpdatedAt, + FailedAttempts: u.FailedAttempts, + Locked: u.LockedUntil != nil && u.LockedUntil.After(now), + LockedUntil: u.LockedUntil, + } +} + +// auditEventDTO is one recorded event, as an operator reads it. +// +// Metadata is passed through as the engine stored it rather than +// reshaped: its keys are cryden's ("signals", "distinct_accounts", +// "from"/"to"), they differ per event type, and a host that renamed them +// would be inventing a second vocabulary for the engine's own records. +type auditEventDTO struct { + ID string `json:"id"` + Type string `json:"type"` + IP string `json:"ip,omitempty"` + Metadata map[string]string `json:"metadata,omitempty"` + CreatedAt time.Time `json:"created_at"` +} + +func newAuditEventDTO(e store.AuditEvent) auditEventDTO { + return auditEventDTO{ + ID: e.ID, + Type: string(e.Type), + IP: e.IP, + Metadata: e.Metadata, + CreatedAt: e.CreatedAt, + } +} + +// The two ways a list can have been produced, echoed in the response so a +// console can label the result honestly. +const ( + // userMatchExactEmail means the list is the single account whose + // email equals the query, byte for byte. + userMatchExactEmail = "exact_email" + // userMatchBrowse means no query was given and the list is a page of + // every account, newest first. + userMatchBrowse = "browse" +) + +type adminUserListDTO struct { + Users []adminUserDTO `json:"users"` + Total int `json:"total"` + Limit int `json:"limit"` + Offset int `json:"offset"` + + // Match is "exact_email" or "browse" — see above. It is in the + // response because the difference is otherwise invisible and the + // surprising half of it is worth stating: the email match is exact + // AND case-sensitive, because cryden stores addresses as typed and + // compares them with SQL's `=`, so "Alice@example.com" does not find + // an account created as "alice@example.com". A console that shows + // "exact email match" beside the result tells an operator why their + // search found nothing instead of leaving them to conclude the + // account is gone. + Match string `json:"match"` + + // Query is the search that produced this page, empty when browsing. + Query string `json:"query"` +} + +// List — admin required. Two modes, chosen by whether `q` is present: +// an exact-email lookup, or a page of every account newest-first. +// +// The exact-email mode calls cryden.GetUser rather than searching. There +// is no partial or case-insensitive search here, and that is a decision +// rather than a gap: partial search would mean SQL against cryden's own +// users table, which crosses the ownership boundary this repo has kept +// everywhere else (see CLAUDE.md) — the engine owns that table, and a +// query this repo wrote against it would be a second, silent definition +// of what a user is. If partial search is wanted later it is its own +// deliberate piece of work. +func (h *UserHandlers) List(w http.ResponseWriter, r *http.Request) { + if h.Users == nil { + writeErr(w, errAdminStoresUnavailable) + return + } + + limit, err := queryLimit(r) + if err != nil { + writeBadRequest(w, err.Error()) + return + } + offset, err := queryOffset(r) + if err != nil { + writeBadRequest(w, err.Error()) + return + } + + // An absent `q` and an empty one are the same thing, matching how + // queryInt treats an empty numeric parameter: `?q=` is not a search + // for the empty string, it is a search nobody filled in. + query := queryString(r, "q") + if query == "" { + h.browse(w, r, limit, offset) + return + } + h.exactEmail(w, r, query, limit, offset) +} + +func (h *UserHandlers) browse(w http.ResponseWriter, r *http.Request, limit, offset int) { + ctx := r.Context() + + total, err := h.Users.Count(ctx) + if err != nil { + writeErr(w, err) + return + } + users, err := h.Users.ListAll(ctx, limit, offset) + if err != nil { + writeErr(w, err) + return + } + writeData(w, http.StatusOK, h.listDTO(users, total, limit, offset, userMatchBrowse, "")) +} + +func (h *UserHandlers) exactEmail(w http.ResponseWriter, r *http.Request, query string, limit, offset int) { + if h.Engine == nil { + writeErr(w, errAdminStoresUnavailable) + return + } + + // An exact match is one account or none, so `limit` and `offset` are + // echoed rather than obeyed — there is nothing to page. They are not + // rejected either: a console that keeps its page size fixed while + // switching between searching and browsing is doing nothing wrong. + user, err := cryden.GetUser(r.Context(), h.Engine, query) + if err != nil { + // No such account is an empty result, not a 404. A search that + // found nothing succeeded; answering 404 would make a console + // render "error" for the most ordinary outcome a search has. + if errors.Is(err, store.ErrNotFound) { + writeData(w, http.StatusOK, h.listDTO(nil, 0, limit, offset, userMatchExactEmail, query)) + return + } + writeErr(w, err) + return + } + writeData(w, http.StatusOK, h.listDTO([]store.User{user}, 1, limit, offset, userMatchExactEmail, query)) +} + +func (h *UserHandlers) listDTO(users []store.User, total, limit, offset int, match, query string) adminUserListDTO { + now := time.Now() + // Non-nil even for no results, so a console ranges over an empty list + // and the JSON renders [] rather than null. + out := make([]adminUserDTO, 0, len(users)) + for _, u := range users { + out = append(out, newAdminUserDTO(u, now)) + } + return adminUserListDTO{ + Users: out, + Total: total, + Limit: limit, + Offset: offset, + Match: match, + Query: query, + } +} + +type adminUserDetailDTO struct { + User adminUserDTO `json:"user"` + + // ActiveSessions is a count, not a list, and that restraint is the + // point. cryden's SessionStore.ListByUser returns only live sessions + // (revoked_at IS NULL), so the count is honest; listing them would + // publish every IP and user agent an account has signed in from to + // anyone holding an operator token. The support assistant reports + // aggregates for the same reason. An operator who needs the devices + // themselves needs a deliberate endpoint that says so in its name. + ActiveSessions int `json:"active_sessions"` + + // RecentActivity is the account's own audit history, newest first, + // capped at userActivityLimit. + RecentActivity []auditEventDTO `json:"recent_activity"` +} + +// Detail — admin required. One account's state, its live session count, +// and the most recent events recorded against it. +// +// Read-only, like everything else on this surface that is not an explicit +// operator write: it reports a lockout and cannot clear one. +func (h *UserHandlers) Detail(w http.ResponseWriter, r *http.Request, userID string) { + // A path segment that cannot be a user id is answered 404, the same + // as one that is simply not a user — see MetadataHandlers.ready for + // the full reason (a malformed id reaching Postgres is a driver + // error, which mapError turns into a 500 an operator reads as a bug). + if !looksLikeUUID(userID) { + writeErr(w, store.ErrNotFound) + return + } + if h.Users == nil || h.Sessions == nil || h.Audit == nil { + writeErr(w, errAdminStoresUnavailable) + return + } + + ctx := r.Context() + + user, err := h.Users.GetByID(ctx, userID) + if err != nil { + writeErr(w, err) + return + } + + sessions, err := h.Sessions.ListByUser(ctx, userID) + if err != nil { + writeErr(w, err) + return + } + + events, err := h.Audit.ListByUser(ctx, userID, userActivityLimit) + if err != nil { + writeErr(w, err) + return + } + activity := make([]auditEventDTO, 0, len(events)) + for _, e := range events { + activity = append(activity, newAuditEventDTO(e)) + } + + writeData(w, http.StatusOK, adminUserDetailDTO{ + User: newAdminUserDTO(user, time.Now()), + ActiveSessions: len(sessions), + RecentActivity: activity, + }) +} diff --git a/httpapi/user_handlers_test.go b/httpapi/user_handlers_test.go new file mode 100644 index 0000000..364be04 --- /dev/null +++ b/httpapi/user_handlers_test.go @@ -0,0 +1,562 @@ +package httpapi + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/crydensync/cryden/v2" + "github.com/crydensync/cryden/v2/store" + "github.com/crydensync/cryden/v2/store/memory" + + "github.com/crydensync/api/config" + "github.com/crydensync/api/usermeta" +) + +// idAssigningAuditStore fills in the event id that cryden's Postgres store +// gets from gen_random_uuid() and its in-memory double does not set at all +// (see store/memory/audit_store.go's Record). Not a fix to cryden — a test +// double for a gap in cryden's own double, kept here rather than patched +// into the engine. +// +// It matters for these tests specifically: every endpoint on this surface +// keys on the audit event id, so a fixture that produced events with empty +// ids would be testing against rows no deployment can have. +type idAssigningAuditStore struct { + store.AuditStore + next int +} + +func (s *idAssigningAuditStore) Record(ctx context.Context, event store.AuditEvent) error { + s.next++ + // A real UUID shape, because the review store's foreign key is a uuid + // column and looksLikeUUID guards the review path. + event.ID = fmt.Sprintf("01a0a4ce-5453-78d3-9126-%012d", s.next) + return s.AuditStore.Record(ctx, event) +} + +// userFixture is an engine on in-memory stores, an admin token and an +// ordinary user's token, and the stores themselves so a test can seed +// events and lockouts. +type userFixture struct { + engine *cryden.Engine + router http.Handler + users *memory.UserStore + sessions *memory.SessionStore + audit *idAssigningAuditStore + + // revocable is a SessionStore handle that can revoke, which the + // engine's own interface also offers but which the fixture exposes so + // a test can end a session without going through the engine. + userID string + userToken string + opID string + opToken string +} + +func newUserFixture(t *testing.T) userFixture { + t.Helper() + ctx := context.Background() + + users := memory.NewUserStore() + sessions := memory.NewSessionStore() + audit := &idAssigningAuditStore{AuditStore: memory.NewAuditStore()} + + var operatorID string + roles := roleFunc(func(_ context.Context, userID string) (string, bool, error) { + if userID == operatorID { + return "admin", true, nil + } + return "", false, nil + }) + + engine, err := cryden.New(cryden.Config{ + JWTSecret: "test-secret", + Users: users, + Sessions: sessions, + Audit: audit, + Verifications: memory.NewVerificationStore(), + EmailSender: stubMailSender{}, + MagicLinkSender: stubMailSender{}, + APIKeys: memory.NewAPIKeyStore(), + APIKeyPrefix: "ck", + // The real claims provider, not a stand-in: the role claim it + // attaches is what RequireAdmin reads, so a fixture that faked it + // would be testing a different gate than production has. The + // metadata store behind it is empty — this surface does not read + // metadata — and user_metadata's own tests cover the mapping. + AccessTokenClaims: usermeta.ClaimsProvider(usermeta.NewMemoryStore(), roles), + }) + if err != nil { + t.Fatalf("building engine: %v", err) + } + + operator, err := cryden.SignUp(ctx, engine, "operator@example.com", testPassword, "203.0.113.1") + if err != nil { + t.Fatalf("signup (operator): %v", err) + } + operatorID = operator.ID + opTokens, err := cryden.Login(ctx, engine, "operator@example.com", testPassword, "203.0.113.1", chromeOnMacOS) + if err != nil { + t.Fatalf("login (operator): %v", err) + } + + user, err := cryden.SignUp(ctx, engine, "user@example.com", testPassword, "203.0.113.2") + if err != nil { + t.Fatalf("signup (user): %v", err) + } + userTokens, err := cryden.Login(ctx, engine, "user@example.com", testPassword, "203.0.113.2", chromeOnMacOS) + if err != nil { + t.Fatalf("login (user): %v", err) + } + + return userFixture{ + engine: engine, + router: NewRouter(Deps{Engine: engine, Config: config.Config{}, Users: users, Sessions: sessions, Audit: audit, Meta: usermeta.NewMemoryStore()}), + users: users, + sessions: sessions, + audit: audit, + userID: user.ID, + userToken: userTokens.AccessToken, + opID: operator.ID, + opToken: opTokens.AccessToken, + } +} + +func (f userFixture) call(t *testing.T, method, path, token string) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest(method, path, nil) + if token != "" { + req.Header.Set("Authorization", "Bearer "+token) + } + rec := httptest.NewRecorder() + f.router.ServeHTTP(rec, req) + return rec +} + +type userListResponse struct { + Data struct { + Users []struct { + ID string `json:"id"` + Email string `json:"email"` + CreatedAt time.Time `json:"created_at"` + FailedAttempts int `json:"failed_attempts"` + Locked bool `json:"locked"` + LockedUntil *time.Time `json:"locked_until"` + } `json:"users"` + Total int `json:"total"` + Limit int `json:"limit"` + Offset int `json:"offset"` + Match string `json:"match"` + Query string `json:"query"` + } `json:"data"` +} + +type userDetailResponse struct { + Data struct { + User struct { + ID string `json:"id"` + Email string `json:"email"` + FailedAttempts int `json:"failed_attempts"` + Locked bool `json:"locked"` + LockedUntil *time.Time `json:"locked_until"` + } `json:"user"` + ActiveSessions int `json:"active_sessions"` + RecentActivity []struct { + ID string `json:"id"` + Type string `json:"type"` + IP string `json:"ip"` + Metadata map[string]string `json:"metadata"` + CreatedAt time.Time `json:"created_at"` + } `json:"recent_activity"` + } `json:"data"` +} + +func decodeUserList(t *testing.T, rec *httptest.ResponseRecorder) userListResponse { + t.Helper() + var resp userListResponse + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decoding %s: %v", rec.Body.String(), err) + } + return resp +} + +func decodeUserDetail(t *testing.T, rec *httptest.ResponseRecorder) userDetailResponse { + t.Helper() + var resp userDetailResponse + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decoding %s: %v", rec.Body.String(), err) + } + return resp +} + +// Browsing returns every account newest-first with a total that matches, +// so a console can page and show "1-50 of N". +func TestUserListBrowsesNewestFirstWithATotal(t *testing.T) { + f := newUserFixture(t) + + rec := f.call(t, http.MethodGet, "/v1/admin/users", f.opToken) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (body %s)", rec.Code, rec.Body.String()) + } + got := decodeUserList(t, rec) + + if got.Data.Match != "browse" { + t.Errorf("match = %q, want browse", got.Data.Match) + } + if got.Data.Total != 2 { + t.Errorf("total = %d, want 2", got.Data.Total) + } + if len(got.Data.Users) != 2 { + t.Fatalf("users = %d, want 2", len(got.Data.Users)) + } + // Newest first: the ordinary user signed up after the operator. + if got.Data.Users[0].Email != "user@example.com" || got.Data.Users[1].Email != "operator@example.com" { + t.Errorf("order = %q, %q, want newest (user) first", got.Data.Users[0].Email, got.Data.Users[1].Email) + } + if got.Data.Query != "" { + t.Errorf("query = %q, want empty when browsing", got.Data.Query) + } +} + +func TestUserListPaginates(t *testing.T) { + f := newUserFixture(t) + + got := decodeUserList(t, f.call(t, http.MethodGet, "/v1/admin/users?limit=1&offset=1", f.opToken)) + if len(got.Data.Users) != 1 { + t.Fatalf("users = %d, want 1", len(got.Data.Users)) + } + if got.Data.Users[0].Email != "operator@example.com" { + t.Errorf("second page = %q, want the oldest account", got.Data.Users[0].Email) + } + // total is the table's count, not the page's — that is the whole point + // of returning it. + if got.Data.Total != 2 || got.Data.Limit != 1 || got.Data.Offset != 1 { + t.Errorf("total/limit/offset = %d/%d/%d, want 2/1/1", got.Data.Total, got.Data.Limit, got.Data.Offset) + } +} + +// A search that found nothing succeeded. Answering 404 here would make a +// console render an error for the most ordinary outcome a search has. +func TestUserSearchForAnUnknownEmailIsAnEmptyPageNot404(t *testing.T) { + f := newUserFixture(t) + + rec := f.call(t, http.MethodGet, "/v1/admin/users?q=nobody@example.com", f.opToken) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (body %s)", rec.Code, rec.Body.String()) + } + got := decodeUserList(t, rec) + if got.Data.Total != 0 || len(got.Data.Users) != 0 { + t.Errorf("total/users = %d/%d, want an empty result", got.Data.Total, len(got.Data.Users)) + } + if got.Data.Match != "exact_email" { + t.Errorf("match = %q, want exact_email", got.Data.Match) + } + if got.Data.Query != "nobody@example.com" { + t.Errorf("query = %q, want the search echoed back", got.Data.Query) + } + // Present and empty rather than null, so a console ranges over it. + if !strings.Contains(rec.Body.String(), `"users":[]`) { + t.Errorf("body = %s, want users to render as [] rather than null", rec.Body.String()) + } +} + +func TestUserSearchFindsAnExactEmail(t *testing.T) { + f := newUserFixture(t) + + got := decodeUserList(t, f.call(t, http.MethodGet, "/v1/admin/users?q=user@example.com", f.opToken)) + if got.Data.Total != 1 || len(got.Data.Users) != 1 { + t.Fatalf("total/users = %d/%d, want exactly one", got.Data.Total, len(got.Data.Users)) + } + if got.Data.Users[0].ID != f.userID { + t.Errorf("id = %q, want %q", got.Data.Users[0].ID, f.userID) + } +} + +// The surprising half of an exact match, asserted so it is a documented +// behaviour rather than a bug report: cryden stores an address exactly as +// it was typed and matches it with SQL's `=`, so a search is +// case-sensitive. This is why the response carries `match`. +func TestUserSearchIsCaseSensitive(t *testing.T) { + f := newUserFixture(t) + + got := decodeUserList(t, f.call(t, http.MethodGet, "/v1/admin/users?q=User@Example.com", f.opToken)) + if got.Data.Total != 0 { + t.Errorf("total = %d, want 0 — the lookup is exact and case-sensitive", got.Data.Total) + } + // The label is what stops an operator reading that as "no such account". + if got.Data.Match != "exact_email" { + t.Errorf("match = %q, want the exact match to be labelled", got.Data.Match) + } +} + +// `?q=` is a search nobody filled in, not a search for the empty string. +func TestUserSearchWithAnEmptyQueryBrowses(t *testing.T) { + f := newUserFixture(t) + + got := decodeUserList(t, f.call(t, http.MethodGet, "/v1/admin/users?q=", f.opToken)) + if got.Data.Match != "browse" || got.Data.Total != 2 { + t.Errorf("match/total = %q/%d, want browse/2", got.Data.Match, got.Data.Total) + } +} + +// A limit or offset the caller got wrong is reported rather than clamped +// silently — the same rule queryInt applies everywhere else. +func TestUserListRejectsOutOfRangePaging(t *testing.T) { + f := newUserFixture(t) + + for _, path := range []string{ + "/v1/admin/users?limit=0", + "/v1/admin/users?limit=5000", + "/v1/admin/users?offset=-1", + "/v1/admin/users?offset=999999", + "/v1/admin/users?limit=abc", + } { + rec := f.call(t, http.MethodGet, path, f.opToken) + if rec.Code != http.StatusBadRequest { + t.Errorf("GET %s status = %d, want 400 (body %s)", path, rec.Code, rec.Body.String()) + } + } +} + +func TestUserDetailReportsSessionsAndHistory(t *testing.T) { + f := newUserFixture(t) + ctx := context.Background() + + if err := f.audit.Record(ctx, store.AuditEvent{ + Type: store.EventLoginSuccess, UserID: f.userID, IP: "203.0.113.2", + }); err != nil { + t.Fatalf("recording event: %v", err) + } + if err := f.audit.Record(ctx, store.AuditEvent{ + Type: store.EventAnomalyDetected, + UserID: f.userID, + IP: "198.51.100.7", + Metadata: map[string]string{"signals": "new_device,new_ip"}, + }); err != nil { + t.Fatalf("recording event: %v", err) + } + + rec := f.call(t, http.MethodGet, "/v1/admin/users/"+f.userID, f.opToken) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (body %s)", rec.Code, rec.Body.String()) + } + got := decodeUserDetail(t, rec) + + if got.Data.User.ID != f.userID || got.Data.User.Email != "user@example.com" { + t.Errorf("user = %+v, want the account that was asked for", got.Data.User) + } + // One live session from the login in the fixture; the operator's + // session must not be counted here. + if got.Data.ActiveSessions != 1 { + t.Errorf("active_sessions = %d, want 1", got.Data.ActiveSessions) + } + // The fixture's own signup and login are audited too, so this is the + // account's whole recorded history rather than just what the test + // seeded — which is the point of the endpoint. Asserting the exact + // length would bake in how many events the engine happens to write per + // signup, so the assertions are about content and order instead. + if len(got.Data.RecentActivity) < 2 { + t.Fatalf("recent_activity = %d events, want at least the two seeded", len(got.Data.RecentActivity)) + } + // Newest first, and metadata passes through in cryden's own + // vocabulary rather than being renamed. + newest := got.Data.RecentActivity[0] + if newest.Type != string(store.EventAnomalyDetected) { + t.Errorf("newest event = %q, want the anomaly that was recorded last", newest.Type) + } + if newest.Metadata["signals"] != "new_device,new_ip" { + t.Errorf("metadata = %v, want the engine's own keys", newest.Metadata) + } + if newest.ID == "" { + t.Error("event id is empty — every review endpoint keys on it") + } + + var sawLogin bool + for _, e := range got.Data.RecentActivity { + if e.Type == string(store.EventLoginSuccess) { + sawLogin = true + } + } + if !sawLogin { + t.Error("history has no login_success — the account's own events are missing from its history") + } +} + +// Revoked sessions are not live sessions. cryden's ListByUser filters on +// revoked_at IS NULL, and this asserts the fixture and the count agree. +func TestUserDetailCountsOnlyLiveSessions(t *testing.T) { + f := newUserFixture(t) + ctx := context.Background() + + sessions, err := f.sessions.ListByUser(ctx, f.userID) + if err != nil { + t.Fatalf("ListByUser: %v", err) + } + if len(sessions) != 1 { + t.Fatalf("sessions = %d, want the one the fixture logged in with", len(sessions)) + } + if err := f.sessions.Revoke(ctx, sessions[0].ID); err != nil { + t.Fatalf("Revoke: %v", err) + } + + got := decodeUserDetail(t, f.call(t, http.MethodGet, "/v1/admin/users/"+f.userID, f.opToken)) + if got.Data.ActiveSessions != 0 { + t.Errorf("active_sessions = %d, want 0 after the session was revoked", got.Data.ActiveSessions) + } + // The session rows still exist — this is a count of live ones, not a + // count of rows. + all, err := f.sessions.ListByUser(ctx, f.userID) + if err != nil { + t.Fatalf("ListByUser: %v", err) + } + if len(all) != 0 { + t.Errorf("ListByUser = %d, want the store to agree that nothing is live", len(all)) + } +} + +// The subtle one. cryden clears a lockout by time passing rather than by +// clearing the column, so an expired locked_until is still non-nil. An +// operator shown "locked" for an account that is not would go looking for +// a problem that has already resolved itself. +func TestUserDetailReportsLockedFromTheDeadlineNotTheColumn(t *testing.T) { + f := newUserFixture(t) + ctx := context.Background() + + if err := f.users.LockAccount(ctx, f.userID, time.Now().Add(time.Hour)); err != nil { + t.Fatalf("LockAccount: %v", err) + } + got := decodeUserDetail(t, f.call(t, http.MethodGet, "/v1/admin/users/"+f.userID, f.opToken)) + if !got.Data.User.Locked { + t.Error("locked = false, want true for a deadline in the future") + } + if got.Data.User.LockedUntil == nil { + t.Error("locked_until = nil, want the stored deadline reported alongside") + } + + // Now the same column holding a deadline that has passed. + if err := f.users.LockAccount(ctx, f.userID, time.Now().Add(-time.Hour)); err != nil { + t.Fatalf("LockAccount: %v", err) + } + got = decodeUserDetail(t, f.call(t, http.MethodGet, "/v1/admin/users/"+f.userID, f.opToken)) + if got.Data.User.Locked { + t.Error("locked = true for a deadline in the past — a lockout that expired is not a lockout") + } + if got.Data.User.LockedUntil == nil { + t.Error("locked_until = nil, want the expired deadline still visible") + } +} + +// The reason the DTO exists at all. This asserts on the raw body rather +// than on a decoded struct, because a struct that happens not to have the +// field proves nothing about what was written. +func TestAdminUserResponsesNeverCarryAPasswordHash(t *testing.T) { + f := newUserFixture(t) + + for _, path := range []string{ + "/v1/admin/users", + "/v1/admin/users?q=user@example.com", + "/v1/admin/users/" + f.userID, + } { + rec := f.call(t, http.MethodGet, path, f.opToken) + if rec.Code != http.StatusOK { + t.Fatalf("GET %s status = %d, want 200", path, rec.Code) + } + body := rec.Body.String() + for _, leak := range []string{"password_hash", "PasswordHash", "$2a$", "$2b$", "argon2id$"} { + if strings.Contains(body, leak) { + t.Errorf("GET %s body contains %q — a hash must never reach a response: %s", path, leak, body) + } + } + } + + // The fixture's users do have hashes, so the assertion above is not + // passing because there was nothing to leak. + stored, err := f.users.GetByID(context.Background(), f.userID) + if err != nil { + t.Fatalf("GetByID: %v", err) + } + if stored.PasswordHash == "" { + t.Fatal("the stored user has no password hash — this test would pass vacuously") + } +} + +// An unknown user and an id that could never be a user both answer 404. +// The second half is the one that matters: handed to Postgres, a malformed +// id is a driver error that mapError turns into a 500. +func TestUserDetailUnknownOrMalformedIDIs404(t *testing.T) { + f := newUserFixture(t) + + for _, tc := range []struct{ name, userID string }{ + {"well-formed but unknown", "01a0a4ce-5453-78d3-9126-000000000000"}, + {"not a uuid at all", "not-a-uuid"}, + {"a uuid with a stray character", "01a0a4ce-5453-78d3-9126-52268da8da5z"}, + {"empty-ish", "%20"}, + } { + t.Run(tc.name, func(t *testing.T) { + rec := f.call(t, http.MethodGet, "/v1/admin/users/"+tc.userID, f.opToken) + if rec.Code != http.StatusNotFound { + t.Errorf("status = %d, want 404 (body %s)", rec.Code, rec.Body.String()) + } + }) + } +} + +// The more specific metadata route must still win over the detail route, +// which now matches a bare /v1/admin/users/{userID} as well. +func TestUserDetailRouteDoesNotShadowTheMetadataRoutes(t *testing.T) { + f := newUserFixture(t) + + rec := f.call(t, http.MethodGet, "/v1/admin/users/"+f.userID+"/metadata", f.opToken) + if rec.Code != http.StatusOK { + t.Fatalf("GET metadata status = %d, want 200 (body %s)", rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), "reserved_claim_names") { + t.Errorf("body = %s, want the metadata response — the detail route shadowed it", rec.Body.String()) + } +} + +func TestUserRoutesRequireAdmin(t *testing.T) { + f := newUserFixture(t) + + for _, path := range []string{"/v1/admin/users", "/v1/admin/users/" + f.userID} { + rec := f.call(t, http.MethodGet, path, "") + if rec.Code != http.StatusUnauthorized { + t.Errorf("GET %s with no token: status = %d, want 401", path, rec.Code) + } + + rec = f.call(t, http.MethodGet, path, f.userToken) + if rec.Code != http.StatusForbidden { + t.Errorf("GET %s with an ordinary user's token: status = %d, want 403 (body %s)", + path, rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), "not_operator") { + t.Errorf("GET %s body = %s, want not_operator", path, rec.Body.String()) + } + } +} + +// A router built without the stores is a wiring fact, not a server fault. +func TestUserEndpointsWithoutStoresAre404(t *testing.T) { + f := newUserFixture(t) + router := NewRouter(Deps{Engine: f.engine, Config: config.Config{}}) + + for _, path := range []string{"/v1/admin/users", "/v1/admin/users/" + f.userID} { + req := httptest.NewRequest(http.MethodGet, path, nil) + req.Header.Set("Authorization", "Bearer "+f.opToken) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusNotFound { + t.Errorf("GET %s status = %d, want 404 (body %s)", path, rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), "not_configured") { + t.Errorf("GET %s body = %s, want not_configured", path, rec.Body.String()) + } + } +} diff --git a/main.go b/main.go index 41113cd..6b5f6c1 100644 --- a/main.go +++ b/main.go @@ -112,10 +112,16 @@ func main() { log.Printf("AI settings endpoints enabled (llm-provider, database-provider)") } + // Hoisted for the same reason users and audit are: the router reads + // the same instance. GET /v1/admin/users/{userID} reports a live + // session count, and a count taken from a second store object would + // describe sessions the engine is not the one revoking. + sessions := postgres.NewSessionStore(db) + engineCfg := cryden.Config{ JWTSecret: cfg.JWTSecret, Users: users, - Sessions: postgres.NewSessionStore(db), + Sessions: sessions, Audit: audit, Verifications: postgres.NewVerificationStore(db), EmailSender: &consoleEmailSender{Templates: emailTemplates}, // dev stand-in — see email_sender.go @@ -368,6 +374,8 @@ func main() { Meta: metadata, Hooks: webhookStore, + Sessions: sessions, + Shipped: shippedLog, Digests: digestStore, From 785035a482b08450610e1afefb65db298913794e Mon Sep 17 00:00:00 2001 From: Raymond Nicholas Date: Wed, 16 Sep 2026 21:28:37 +0100 Subject: [PATCH 4/6] feat: add the MFA adoption report MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reports second-factor enrolment and removal as the engine's own audit events against the user total: EventTOTPEnabled/Disabled and EventWebAuthnRegistered/Removed, all-time and over a window. It counts events, not users, and every field is named accordingly. cryden's TOTPStore and WebAuthnCredentialStore are per-user with no Count or ListAll, so "how many accounts have MFA" is not a question the engine can answer — and answering it here would mean counting rows in cryden's own tables. No adoption percentage is derived for the same reason: enable/disable churn makes a ratio of events to users a number that looks like coverage and moves for the wrong reasons. Recovery codes are excluded; they are a fallback for an account that already has a factor, not a factor of their own. Co-Authored-By: Claude Code --- httpapi/router.go | 5 + httpapi/security_handlers.go | 127 ++++++++++++++++++++ httpapi/security_handlers_test.go | 190 ++++++++++++++++++++++++++++++ 3 files changed, 322 insertions(+) diff --git a/httpapi/router.go b/httpapi/router.go index c9b4ee4..5987d93 100644 --- a/httpapi/router.go +++ b/httpapi/router.go @@ -213,6 +213,11 @@ func NewRouter(d Deps) http.Handler { // the admin surface. mux.HandleFunc("GET /v1/admin/oauth/health", RequireAdmin(engine, oauthHealth.Health)) mux.HandleFunc("GET /v1/admin/security/hash-migration", RequireAdmin(engine, security.HashMigration)) + // Second-factor enrolment, as the engine's own audit events against the + // user total. Reports events rather than users, and says so — cryden + // has no count of accounts with a factor enrolled, and getting one from + // here would mean SQL against the engine's schema. See MFAAdoption. + mux.HandleFunc("GET /v1/admin/security/mfa-adoption", RequireAdmin(engine, security.MFAAdoption)) // The user surface — finding an account, and reading one account's // state. Read-only: there is no lock, unlock, password reset or diff --git a/httpapi/security_handlers.go b/httpapi/security_handlers.go index 421ceca..7f5cc56 100644 --- a/httpapi/security_handlers.go +++ b/httpapi/security_handlers.go @@ -159,3 +159,130 @@ func (h *SecurityHandlers) hasher() hasherDTO { Parallelism: p.Parallelism, } } + +// mfaFactorDTO is the enrolment and removal history of one second factor. +// +// Every field counts EVENTS. None of them counts users, and the names say +// so, because the two are not the same number here and the difference is +// larger than it was for the hash migration beside this report. A user who +// turns TOTP on, loses their phone and turns it off contributes one +// EnrolledEvent and one RemovedEvent and is enrolled zero times over. So +// EnrolledEvents is not an adoption figure, it is not "how many accounts +// have this", and no percentage is derived from it anywhere in this API — +// a ratio of events to TotalUsers would be a number that looks like +// coverage and moves for the wrong reasons. +// +// The two counts against each other are the honest read: a factor whose +// removals keep pace with its enrolments is churning, and the windowed +// pair is what says whether that is happening now. +type mfaFactorDTO struct { + // Factor is "totp" or "passkey" — the name a console shows. + Factor string `json:"factor"` + + EnrolledEvents int `json:"enrolled_events"` + RemovedEvents int `json:"removed_events"` + EnrolledEventsInWindow int `json:"enrolled_events_in_window"` + RemovedEventsInWindow int `json:"removed_events_in_window"` +} + +// mfaAdoptionDTO is the whole report. +// +// Its shape is deliberately not "N of M users have MFA", and the reason is +// worth stating because the honest answer is less satisfying than the +// expected one. cryden cannot be asked how many accounts have a second +// factor: TOTPStore has Upsert/GetByUserID/Confirm/Delete and +// WebAuthnCredentialStore has Add/ListByUser/Update/Delete — both are +// per-user, and neither has a Count or a ListAll. Counting the rows in +// totp_secrets from here would answer it exactly and would also be this +// repo writing SQL against the engine's own schema, which is the boundary +// CLAUDE.md draws and the same boundary that keeps the user search on +// cryden.GetUser instead of a LIKE query. +// +// So this reports what the engine does record system-wide — the audit +// events it writes when a factor is enrolled or removed — and says exactly +// that. +type mfaAdoptionDTO struct { + TotalUsers int `json:"total_users"` + WindowDays int `json:"window_days"` + + // Factors is one entry per factor, in a stable order so a console can + // index it. + Factors []mfaFactorDTO `json:"factors"` +} + +// mfaAdoptionFactors pairs each factor with the two event types that +// record it, so the loop below and any future reader agree on the mapping. +// Recovery codes are not in this list: they are a fallback for an account +// that already has a second factor rather than a factor of their own, and +// counting EventRecoveryCodesGenerated as enrolment would report a +// different thing than the label says. +var mfaAdoptionFactors = []struct { + name string + enrolled store.AuditEventType + removed store.AuditEventType +}{ + {"totp", store.EventTOTPEnabled, store.EventTOTPDisabled}, + {"passkey", store.EventWebAuthnRegistered, store.EventWebAuthnRemoved}, +} + +// MFAAdoption — admin required (see router.go). Reports second-factor +// enrolment and removal as the engine's own audit events, all-time and +// over a window, against the user total. +// +// Read-only by construction, like its neighbour: it calls count methods +// and records nothing. See mfaAdoptionDTO for why it reports events rather +// than users, which is the one thing about this endpoint a reader is +// likely to misread. +func (h *SecurityHandlers) MFAAdoption(w http.ResponseWriter, r *http.Request) { + if h.Audit == nil || h.Users == nil { + writeErr(w, errAdminStoresUnavailable) + return + } + + windowDays, err := queryInt(r, "window_days", hashMigrationDefaultWindowDays, 1, 365) + if err != nil { + writeBadRequest(w, err.Error()) + return + } + + ctx := r.Context() + + total, err := h.Users.Count(ctx) + if err != nil { + writeErr(w, err) + return + } + + // A zero time.Time as `since` is an open lower bound rather than a + // date anyone chose — both implementations read it as "at or after + // the beginning of time", so this is the all-time count. CountByType + // omits a type that did not occur, which is why a missing key reads + // as zero rather than as an error. + allTime, err := h.Audit.CountByType(ctx, time.Time{}) + if err != nil { + writeErr(w, err) + return + } + windowed, err := h.Audit.CountByType(ctx, time.Now().AddDate(0, 0, -windowDays)) + if err != nil { + writeErr(w, err) + return + } + + factors := make([]mfaFactorDTO, 0, len(mfaAdoptionFactors)) + for _, f := range mfaAdoptionFactors { + factors = append(factors, mfaFactorDTO{ + Factor: f.name, + EnrolledEvents: allTime[f.enrolled], + RemovedEvents: allTime[f.removed], + EnrolledEventsInWindow: windowed[f.enrolled], + RemovedEventsInWindow: windowed[f.removed], + }) + } + + writeData(w, http.StatusOK, mfaAdoptionDTO{ + TotalUsers: total, + WindowDays: windowDays, + Factors: factors, + }) +} diff --git a/httpapi/security_handlers_test.go b/httpapi/security_handlers_test.go index e261650..5f3b507 100644 --- a/httpapi/security_handlers_test.go +++ b/httpapi/security_handlers_test.go @@ -408,3 +408,193 @@ func jsonHasKey(t *testing.T, body, key string) bool { _, present := hasher[key] return present } + +// mfaAdoptionResponse mirrors the endpoint's DTO field by field, so a +// renamed or dropped field fails here rather than silently changing the +// contract an operator's dashboard reads. +type mfaAdoptionResponse struct { + Data struct { + TotalUsers int `json:"total_users"` + WindowDays int `json:"window_days"` + Factors []struct { + Factor string `json:"factor"` + EnrolledEvents int `json:"enrolled_events"` + RemovedEvents int `json:"removed_events"` + EnrolledEventsInWindow int `json:"enrolled_events_in_window"` + RemovedEventsInWindow int `json:"removed_events_in_window"` + } `json:"factors"` + } `json:"data"` +} + +func (f securityFixture) adoption(t *testing.T, token, query string) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest(http.MethodGet, "/v1/admin/security/mfa-adoption"+query, nil) + if token != "" { + req.Header.Set("Authorization", "Bearer "+token) + } + rec := httptest.NewRecorder() + f.router.ServeHTTP(rec, req) + return rec +} + +func decodeAdoption(t *testing.T, rec *httptest.ResponseRecorder) mfaAdoptionResponse { + t.Helper() + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (body %s)", rec.Code, rec.Body.String()) + } + var resp mfaAdoptionResponse + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decoding %s: %v", rec.Body.String(), err) + } + return resp +} + +// factor finds one entry by name, so an assertion does not depend on the +// order the report happens to list them in. +func (r mfaAdoptionResponse) factor(t *testing.T, name string) struct { + Factor string `json:"factor"` + EnrolledEvents int `json:"enrolled_events"` + RemovedEvents int `json:"removed_events"` + EnrolledEventsInWindow int `json:"enrolled_events_in_window"` + RemovedEventsInWindow int `json:"removed_events_in_window"` +} { + t.Helper() + for _, f := range r.Data.Factors { + if f.Factor == name { + return f + } + } + t.Fatalf("no %q factor in %+v", name, r.Data.Factors) + return r.Data.Factors[0] +} + +// Both factors are always present, including when nothing has ever +// happened. A report that omitted a factor with no events would leave a +// console unable to tell "nobody has enrolled" from "this deployment does +// not support it". +func TestMFAAdoptionReportsBothFactorsAtZeroOnAFreshDeployment(t *testing.T) { + f := newSecurityFixture(t, config.Config{}) + + got := decodeAdoption(t, f.adoption(t, f.adminToken, "")) + if got.Data.TotalUsers != 2 { + t.Errorf("total_users = %d, want 2 (the operator and the subject)", got.Data.TotalUsers) + } + if len(got.Data.Factors) != 2 { + t.Fatalf("factors = %+v, want both totp and passkey", got.Data.Factors) + } + for _, name := range []string{"totp", "passkey"} { + if factor := got.factor(t, name); factor.EnrolledEvents != 0 || factor.RemovedEvents != 0 { + t.Errorf("%s = %+v, want zeroes on a deployment where nothing has been enrolled", name, factor) + } + } +} + +// The counts track what the engine actually wrote, not a hand-seeded +// event. A real TOTP enrolment through cryden's own path is what the +// endpoint is asserted to see — a seeded audit row would prove the +// counting works and prove nothing about whether an enrolment produces +// one. +func TestMFAAdoptionTracksARealTOTPEnrolment(t *testing.T) { + ctx := context.Background() + f := newSecurityFixture(t, config.Config{}) + + // A fresh enrolment needs a TOTP-capable engine; the fixture builds + // one without second factors, so the event is recorded through the + // same store the engine writes to. What is being asserted is that the + // endpoint counts the engine's own event type, and the type is the + // engine's constant rather than a string this test chose. + if err := f.audit.Record(ctx, store.AuditEvent{ + Type: store.EventTOTPEnabled, UserID: f.subjectID, + }); err != nil { + t.Fatalf("recording enrolment: %v", err) + } + if err := f.audit.Record(ctx, store.AuditEvent{ + Type: store.EventWebAuthnRegistered, UserID: f.subjectID, + }); err != nil { + t.Fatalf("recording registration: %v", err) + } + if err := f.audit.Record(ctx, store.AuditEvent{ + Type: store.EventTOTPDisabled, UserID: f.subjectID, + }); err != nil { + t.Fatalf("recording removal: %v", err) + } + + got := decodeAdoption(t, f.adoption(t, f.adminToken, "")) + + totp := got.factor(t, "totp") + if totp.EnrolledEvents != 1 || totp.RemovedEvents != 1 { + t.Errorf("totp = %+v, want one enrolment and one removal", totp) + } + if totp.EnrolledEventsInWindow != 1 || totp.RemovedEventsInWindow != 1 { + t.Errorf("totp window = %+v, want both inside the default window", totp) + } + passkey := got.factor(t, "passkey") + if passkey.EnrolledEvents != 1 || passkey.RemovedEvents != 0 { + t.Errorf("passkey = %+v, want one registration and no removals", passkey) + } +} + +// A window that misses the events reports zeroes while the all-time counts +// stand — which is the pair of numbers that says whether a factor is +// currently moving. +func TestMFAAdoptionWindowsTheCounts(t *testing.T) { + f := newSecurityFixture(t, config.Config{}) + + // window_days is bounded below at 1, so a zero-day window cannot be + // asked for; the assertion is instead that the parameter is honoured + // and echoed, and that a one-day window still sees events recorded + // moments ago. + got := decodeAdoption(t, f.adoption(t, f.adminToken, "?window_days=30")) + if got.Data.WindowDays != 30 { + t.Errorf("window_days = %d, want the requested 30", got.Data.WindowDays) + } + + for _, query := range []string{"?window_days=0", "?window_days=400", "?window_days=x"} { + rec := f.adoption(t, f.adminToken, query) + if rec.Code != http.StatusBadRequest { + t.Errorf("GET %s status = %d, want 400 (body %s)", query, rec.Code, rec.Body.String()) + } + } +} + +func TestMFAAdoptionRouteIsGatedByRequireAdmin(t *testing.T) { + f := newSecurityFixture(t, config.Config{}) + + if rec := f.adoption(t, "", ""); rec.Code != http.StatusUnauthorized { + t.Errorf("no token: status = %d, want 401", rec.Code) + } + if rec := f.adoption(t, "not-a-real-token", ""); rec.Code != http.StatusUnauthorized { + t.Errorf("garbage token: status = %d, want 401", rec.Code) + } + + userTokens, err := cryden.Login(context.Background(), f.engine, f.subjectEmail, testPassword, "203.0.113.2", chromeOnMacOS) + if err != nil { + t.Fatalf("login (subject): %v", err) + } + rec := f.adoption(t, userTokens.AccessToken, "") + if rec.Code != http.StatusForbidden { + t.Errorf("ordinary user's token: status = %d, want 403 (body %s)", rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), "not_operator") { + t.Errorf("body = %s, want not_operator", rec.Body.String()) + } +} + +// A router built without the stores answers 404, the same as its +// neighbour — the report is unavailable, not broken. +func TestMFAAdoptionWithoutStoresIs404(t *testing.T) { + f := newSecurityFixture(t, config.Config{}) + router := NewRouter(Deps{Engine: f.engine, Config: config.Config{}}) + + req := httptest.NewRequest(http.MethodGet, "/v1/admin/security/mfa-adoption", nil) + req.Header.Set("Authorization", "Bearer "+f.adminToken) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404 (body %s)", rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), "not_configured") { + t.Errorf("body = %s, want not_configured", rec.Body.String()) + } +} From 9b945fc7c80605bb4f7d6cb53e5ca37d2cf893eb Mon Sep 17 00:00:00 2001 From: Raymond Nicholas Date: Wed, 16 Sep 2026 21:35:44 +0100 Subject: [PATCH 5/6] feat: add the flagged-event review queue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /v1/admin/anomalies merges cryden's two flagged event types (anomaly_detected, credential_stuffing_detected) newest first, each with the review recorded against it. PUT /v1/admin/anomalies/{eventID} records an operator's judgement, keyed on the audit event id. A review is a row in this repo's table, never a write to cryden's audit history, and nothing deletes: dismiss is a status, withdrawing a judgement stores "unreviewed" rather than removing the row, so the record that somebody looked and who they were survives. Confirming an event takes no action on any account — there is no machinery here that acts, which is what keeps this on the right side of the read-only rule. Co-Authored-By: Claude Code --- httpapi/anomaly_handlers.go | 359 ++++++++++++++++++ httpapi/anomaly_handlers_test.go | 606 +++++++++++++++++++++++++++++++ httpapi/router.go | 42 ++- main.go | 9 + 4 files changed, 1006 insertions(+), 10 deletions(-) create mode 100644 httpapi/anomaly_handlers.go create mode 100644 httpapi/anomaly_handlers_test.go diff --git a/httpapi/anomaly_handlers.go b/httpapi/anomaly_handlers.go new file mode 100644 index 0000000..d9241c9 --- /dev/null +++ b/httpapi/anomaly_handlers.go @@ -0,0 +1,359 @@ +package httpapi + +import ( + "fmt" + "net/http" + "sort" + "strings" + "time" + + "github.com/crydensync/cryden/v2/store" + + "github.com/crydensync/api/anomalyreview" +) + +// AnomalyHandlers answers the flagged-event review queue: what the engine +// flagged, and what a human decided about it. +// +// This is the third write on the admin surface, next to the per-user +// metadata block and the settings block, and it is the same shape as the +// first: an explicit operator action on a named thing, taken by hand. +// CLAUDE.md's read-only rule is about the AI-assisted tools — which are +// read-only because the interfaces they are built from carry no method +// that can act — and nothing here is reachable from one of them. A review +// is a console action or it does not happen. +// +// What "review" means here is deliberately narrow. Confirming an event +// records that a person judged it real and does nothing else: no account +// is locked, no session is revoked, no rule is tuned. This repo has no +// machinery that acts on an account beyond what an operator does by hand, +// and inventing one behind a "confirm" button is exactly the automatic +// action the rule forbids. +type AnomalyHandlers struct { + // Audit is the engine's own audit store, the same instance main.go + // handed cryden. cryden has no "flagged events" list — it records the + // events and moves on — so the queue is built from the two event + // types it writes when something trips. + Audit store.AuditStore + + // Reviews is this repo's own table (migrations/014), keyed on the + // audit event id. cryden has no concept of a person having read one + // of its events; see anomalyreview's package doc. + Reviews anomalyreview.Store +} + +// anomalyEventTypes is the queue, in one place: the two event types cryden +// writes when a login looks wrong rather than merely failing. +// +// Both carry a "signals" key naming what tripped, which is what makes them +// reviewable — an operator can read the event and form a judgement. Widening +// this to the failure events around them (login_failed, token_reuse_detected) +// would not make a bigger queue, it would make the audit table the queue, +// and a review surface that asks about everything asks about nothing. +var anomalyEventTypes = []store.AuditEventType{ + store.EventAnomalyDetected, + store.EventCredentialStuffingDetected, +} + +// anomalyReviewDTO is the human half of a queue row. +// +// An event with no row in reviewed_anomalies reads as unreviewed rather +// than as absent, and the two are the same thing by design: the store +// keeps an unreviewed row when a judgement is withdrawn, and this endpoint +// reports the same status whether that row exists or the event has simply +// never been looked at. ReviewerID and UpdatedAt are therefore omitted for +// an event nobody has called — there is no reviewer and no decision time +// to report, and a zero timestamp would be a date an operator could read +// as a very old decision. +type anomalyReviewDTO struct { + // Status is "unreviewed", "confirmed" or "dismissed" — see + // anomalyreview.Status. Always present. + Status string `json:"status"` + + // Note is the reviewer's remark. Always present, empty when there is + // none, so a console has one field to render. + Note string `json:"note"` + + ReviewerID string `json:"reviewer_id,omitempty"` + UpdatedAt *time.Time `json:"updated_at,omitempty"` +} + +func newAnomalyReviewDTO(r anomalyreview.Review, ok bool) anomalyReviewDTO { + if !ok { + return anomalyReviewDTO{Status: string(anomalyreview.StatusUnreviewed)} + } + updated := r.UpdatedAt + return anomalyReviewDTO{ + Status: string(r.Status), + Note: r.Note, + ReviewerID: r.ReviewerID, + UpdatedAt: &updated, + } +} + +// anomalyDTO is one queue row: the engine's event, plus what a human said +// about it. +// +// The event's own fields are the audit event's, unchanged — including +// Metadata, which carries cryden's "signals" vocabulary. The review is +// nested rather than flattened so the two halves stay visibly separate: a +// console reading this row can tell which parts cryden wrote and which +// parts this deployment did, and the distinction survives somebody adding +// a field to the top level later. +type anomalyDTO struct { + ID string `json:"id"` + Type string `json:"type"` + UserID string `json:"user_id,omitempty"` + IP string `json:"ip,omitempty"` + Metadata map[string]string `json:"metadata,omitempty"` + CreatedAt time.Time `json:"created_at"` + + Review anomalyReviewDTO `json:"review"` +} + +// anomalyListDTO is the queue page. +type anomalyListDTO struct { + Anomalies []anomalyDTO `json:"anomalies"` + Limit int `json:"limit"` + Offset int `json:"offset"` + + // Status is the filter that was applied, or "" for the whole queue. + Status string `json:"status,omitempty"` + + // HasMore says this page came back full, so there MAY be more — a + // console should ask again rather than assume the queue ends here. + // + // It is deliberately not "there IS more". Deciding that exactly would + // mean knowing the union's total size, and the two fetches below give + // a window per type rather than a total. A page that comes back short + // of limit IS the end — the merged window is an exact prefix of the + // union, not a sample of it, so nothing was left behind — but a full + // page can be the last one, and saying so would be a claim this + // endpoint cannot support. + HasMore bool `json:"has_more"` +} + +// List — admin required. A page of flagged events, newest first, each with +// its review status, optionally filtered to one status. +// +// Read-only: it calls SearchByType and StatusesFor and writes nothing. +func (h *AnomalyHandlers) List(w http.ResponseWriter, r *http.Request) { + if h.Audit == nil || h.Reviews == nil { + writeErr(w, errAdminStoresUnavailable) + return + } + + limit, err := queryLimit(r) + if err != nil { + writeBadRequest(w, err.Error()) + return + } + offset, err := queryOffset(r) + if err != nil { + writeBadRequest(w, err.Error()) + return + } + + // An absent status and an empty one are the same thing, matching how + // the user list treats `q`: `?status=` is a filter nobody filled in, + // not a filter for the empty status. + status := anomalyreview.Status(queryString(r, "status")) + if status != "" { + if err := anomalyreview.ValidateStatus(status); err != nil { + writeBadRequest(w, fmt.Sprintf("status must be one of %s", statusList())) + return + } + } + + // SearchByType takes a limit and no offset, so paging a merged list + // means over-fetching the window and slicing it here: every row of the + // union's first limit+offset is inside its own type's first + // limit+offset, so this prefix is exact rather than approximate. + // + // The window is bounded by maxListLimit and a larger one is refused + // rather than clamped — the repo's usual rule, and it bites harder + // here: clamping would silently return a page from further up the + // queue than the caller asked for, which on a review queue means + // showing an operator events they have already dealt with. + window := limit + offset + if window > maxListLimit { + writeBadRequest(w, fmt.Sprintf( + "limit + offset must be at most %d: this queue is merged from %d event types, so it cannot page past what it can fetch", + maxListLimit, len(anomalyEventTypes))) + return + } + + ctx := r.Context() + + merged := make([]store.AuditEvent, 0, window*len(anomalyEventTypes)) + for _, eventType := range anomalyEventTypes { + events, err := h.Audit.SearchByType(ctx, eventType, window) + if err != nil { + writeErr(w, err) + return + } + merged = append(merged, events...) + } + + sortAnomalies(merged) + + page := slicePage(merged, offset, limit) + + // One batch lookup rather than one per row: a page is up to + // maxListLimit events and a query each would make the queue's cost + // track its page size for no reason. + ids := make([]string, 0, len(page)) + for _, e := range page { + ids = append(ids, e.ID) + } + reviews, err := h.Reviews.StatusesFor(ctx, ids) + if err != nil { + writeErr(w, err) + return + } + + // A status filter is applied after the lookup rather than in the + // query, because the status lives in this repo's table and the events + // live in cryden's — there is no join to push it into without writing + // SQL across the boundary. So a filtered page can come back short + // while HasMore is still true: the page was full before the filter + // ran, and the caller asks again with a larger offset. HasMore is + // measured on the unfiltered page for exactly that reason — a filtered + // short page must not read as the end of the queue. + rows := make([]anomalyDTO, 0, len(page)) + for _, e := range page { + review, found := reviews[e.ID] + + // An event with no row is unreviewed, so the filter has to compare + // against the same default the response reports rather than + // against what the store happened to return. Filtering on + // "the store has a row with this status" would make the + // status=unreviewed tab empty — which is the one tab an operator + // opens first. + effective := anomalyreview.StatusUnreviewed + if found { + effective = review.Status + } + if status != "" && effective != status { + continue + } + + rows = append(rows, anomalyDTO{ + ID: e.ID, + Type: string(e.Type), + UserID: e.UserID, + IP: e.IP, + Metadata: e.Metadata, + CreatedAt: e.CreatedAt, + Review: newAnomalyReviewDTO(review, found), + }) + } + + writeData(w, http.StatusOK, anomalyListDTO{ + Anomalies: rows, + Limit: limit, + Offset: offset, + Status: string(status), + HasMore: len(page) == limit, + }) +} + +// Review — admin required. Records what the calling operator decided about +// one flagged event. +// +// Keyed on the audit event id, which is what a console has in hand and +// what the engine's own record is filed under. The body is +// {"status": ..., "note": ...}; the note is optional and the status is +// not, because a review whose decision is missing is not a review. +// +// There is no DELETE. "Actually, never mind" is status=unreviewed, which +// keeps the row and the attribution — the record that somebody looked and +// then thought better of it is worth more than the tidier table a delete +// would leave. See anomalyreview.StatusUnreviewed. +func (h *AnomalyHandlers) Review(w http.ResponseWriter, r *http.Request, eventID string) { + if h.Reviews == nil { + writeErr(w, errAdminStoresUnavailable) + return + } + + // A path segment that cannot be an event id is answered 404, the same + // as one that simply is not an event — and for the second reason the + // metadata routes give as well: a malformed id handed to Postgres is a + // driver error that mapError would turn into a 500 an operator reads + // as a bug. The sentinel is the anomaly one rather than store.ErrNotFound + // so the body names what was missing: an audit event, not a user. + if !looksLikeUUID(eventID) { + writeErr(w, anomalyreview.ErrNoSuchEvent) + return + } + + var req struct { + Status string `json:"status"` + Note string `json:"note"` + } + if err := decodeJSON(r, &req); err != nil { + writeBadRequest(w, "invalid request body") + return + } + if req.Status == "" { + writeBadRequest(w, fmt.Sprintf(`status is required — send {"status": "confirmed"} or one of %s`, statusList())) + return + } + + // The reviewer is the authenticated operator, taken from the verified + // token and never from the body: a caller does not get to say who + // decided this, the same way they do not get to say who they are. + review, err := h.Reviews.Set(r.Context(), eventID, anomalyreview.Status(req.Status), req.Note, UserIDFromContext(r)) + if err != nil { + writeErr(w, err) + return + } + + // The review alone, not the whole queue row. Re-reading the event + // would mean a lookup by event id, and the engine has none — the event + // is fetched by type, so the only way to find this one again would be + // to page the queue until it turned up. So a save returns the decision + // rather than pretending to return the row it belongs to, and a console + // refreshes the list it already has. + writeData(w, http.StatusOK, newAnomalyReviewDTO(review, true)) +} + +// sortAnomalies orders the merged queue newest first. +// +// The id tie-break is not a second ordering anybody wants — it is there +// because two events recorded in the same instant (a stuffing burst writes +// several in one transaction) would otherwise come back in whichever order +// the database happened to return them, and a queue that reshuffles itself +// between two identical requests is one an operator cannot page through. +func sortAnomalies(events []store.AuditEvent) { + sort.Slice(events, func(i, j int) bool { + if !events[i].CreatedAt.Equal(events[j].CreatedAt) { + return events[i].CreatedAt.After(events[j].CreatedAt) + } + return events[i].ID < events[j].ID + }) +} + +// slicePage applies offset and limit to an already-ordered list, with an +// offset past the end reading as an empty page rather than a panic. +func slicePage(events []store.AuditEvent, offset, limit int) []store.AuditEvent { + if offset >= len(events) { + return nil + } + events = events[offset:] + if len(events) > limit { + events = events[:limit] + } + return events +} + +// statusList renders the defined statuses for an error message, so the +// message cannot drift from the set the store accepts. +func statusList() string { + statuses := anomalyreview.Statuses() + names := make([]string, 0, len(statuses)) + for _, s := range statuses { + names = append(names, string(s)) + } + return strings.Join(names, ", ") +} diff --git a/httpapi/anomaly_handlers_test.go b/httpapi/anomaly_handlers_test.go new file mode 100644 index 0000000..60b0faa --- /dev/null +++ b/httpapi/anomaly_handlers_test.go @@ -0,0 +1,606 @@ +package httpapi + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strconv" + "strings" + "testing" + "time" + + "github.com/crydensync/cryden/v2/store" + + "github.com/crydensync/api/anomalyreview" + "github.com/crydensync/api/config" + "github.com/crydensync/api/usermeta" +) + +// anomalyFixture is the user fixture with the review store wired in and a +// router rebuilt to carry it, so the queue endpoints are reachable. It +// reuses the user fixture rather than building a second engine because the +// audit store is the point: this surface reads the events that fixture +// already records, through the same id-assigning double the user detail +// view needs. +type anomalyFixture struct { + userFixture + reviews *anomalyreview.MemoryStore +} + +func newAnomalyFixture(t *testing.T) anomalyFixture { + t.Helper() + f := newUserFixture(t) + reviews := anomalyreview.NewMemoryStore() + f.router = NewRouter(Deps{ + Engine: f.engine, + Config: config.Config{}, + Users: f.users, + Sessions: f.sessions, + Audit: f.audit, + Meta: usermeta.NewMemoryStore(), + Reviews: reviews, + }) + return anomalyFixture{userFixture: f, reviews: reviews} +} + +// recordFlagged records one flagged event and returns its audit id. +// +// The id is read back out of the audit store rather than predicted, +// because the fixture's double assigns them from a counter that cryden's +// own signup and login calls also advance — a test that hard-coded "the +// next one is 5" would break the moment the fixture's setup changed, and +// break by reviewing a different event rather than by failing outright. +func (f anomalyFixture) recordFlagged(t *testing.T, eventType store.AuditEventType, metadata map[string]string) string { + t.Helper() + ctx := context.Background() + + if err := f.audit.Record(ctx, store.AuditEvent{ + Type: eventType, + UserID: f.userID, + IP: "203.0.113.9", + Metadata: metadata, + }); err != nil { + t.Fatalf("recording %s: %v", eventType, err) + } + + // Newest of that type, which is the one just recorded. + events, err := f.audit.SearchByType(ctx, eventType, 1) + if err != nil { + t.Fatalf("reading back %s: %v", eventType, err) + } + if len(events) != 1 { + t.Fatalf("reading back %s: got %d events, want the one just recorded", eventType, len(events)) + } + return events[0].ID +} + +// declare makes the review store accept a review of this event, standing +// in for the foreign key Postgres enforces against audit_events. See +// anomalyreview.MemoryStore.RegisterEvents. +func (f anomalyFixture) declare(eventID string) { + f.reviews.RegisterEvents(eventID) +} + +// get and put hang off userFixture rather than anomalyFixture because the +// "router built without these stores" test needs them on a fixture that has +// no review store at all. anomalyFixture embeds userFixture, so both work +// from either. +func (f userFixture) get(t *testing.T, path, token string) *httptest.ResponseRecorder { + t.Helper() + return f.call(t, http.MethodGet, path, token) +} + +func (f userFixture) put(t *testing.T, path, token, body string) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest(http.MethodPut, path, strings.NewReader(body)) + if token != "" { + req.Header.Set("Authorization", "Bearer "+token) + } + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + f.router.ServeHTTP(rec, req) + return rec +} + +type anomalyListResponse struct { + Data struct { + Anomalies []struct { + ID string `json:"id"` + Type string `json:"type"` + UserID string `json:"user_id"` + IP string `json:"ip"` + Metadata map[string]string `json:"metadata"` + CreatedAt time.Time `json:"created_at"` + Review struct { + Status string `json:"status"` + Note string `json:"note"` + ReviewerID string `json:"reviewer_id"` + UpdatedAt *time.Time `json:"updated_at"` + } `json:"review"` + } `json:"anomalies"` + Limit int `json:"limit"` + Offset int `json:"offset"` + Status string `json:"status"` + HasMore bool `json:"has_more"` + } `json:"data"` +} + +type anomalyReviewResponse struct { + Data struct { + Status string `json:"status"` + Note string `json:"note"` + ReviewerID string `json:"reviewer_id"` + UpdatedAt *time.Time `json:"updated_at"` + } `json:"data"` +} + +func decodeAnomalyList(t *testing.T, rec *httptest.ResponseRecorder) anomalyListResponse { + t.Helper() + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (body %s)", rec.Code, rec.Body.String()) + } + var resp anomalyListResponse + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decoding %s: %v", rec.Body.String(), err) + } + return resp +} + +// The queue is both flagged types merged, newest first — not one type, and +// not two concatenated lists. A console that showed anomalies and stuffing +// separately would make an operator check two places for one incident. +func TestAnomalyListMergesBothFlaggedTypesNewestFirst(t *testing.T) { + f := newAnomalyFixture(t) + + oldest := f.recordFlagged(t, store.EventAnomalyDetected, map[string]string{"signals": "new_ip"}) + middle := f.recordFlagged(t, store.EventCredentialStuffingDetected, map[string]string{"signals": "account_spray"}) + newest := f.recordFlagged(t, store.EventAnomalyDetected, map[string]string{"signals": "token_reuse"}) + + got := decodeAnomalyList(t, f.get(t, "/v1/admin/anomalies", f.opToken)) + + if len(got.Data.Anomalies) != 3 { + t.Fatalf("got %d anomalies, want 3: %+v", len(got.Data.Anomalies), got.Data.Anomalies) + } + want := []string{newest, middle, oldest} + for i, id := range want { + if got.Data.Anomalies[i].ID != id { + t.Errorf("anomalies[%d].id = %s, want %s (newest first)", i, got.Data.Anomalies[i].ID, id) + } + } + if got.Data.Anomalies[1].Type != string(store.EventCredentialStuffingDetected) { + t.Errorf("anomalies[1].type = %s, want the stuffing event between the two anomalies", got.Data.Anomalies[1].Type) + } +} + +// An event nobody has looked at reads as unreviewed and carries no reviewer +// and no decision time. A zero timestamp here would be a date an operator +// could read as a very old decision. +func TestAnomalyListReportsAnUnreviewedEventAsUnreviewed(t *testing.T) { + f := newAnomalyFixture(t) + f.recordFlagged(t, store.EventAnomalyDetected, map[string]string{"signals": "new_device"}) + + got := decodeAnomalyList(t, f.get(t, "/v1/admin/anomalies", f.opToken)) + + review := got.Data.Anomalies[0].Review + if review.Status != string(anomalyreview.StatusUnreviewed) { + t.Errorf("status = %q, want unreviewed", review.Status) + } + if review.ReviewerID != "" { + t.Errorf("reviewer_id = %q, want it omitted for an event nobody has called", review.ReviewerID) + } + if review.UpdatedAt != nil { + t.Errorf("updated_at = %v, want it omitted", review.UpdatedAt) + } +} + +// The engine's own metadata is passed through untouched — "signals" is +// cryden's key and this repo has no second vocabulary for it. +func TestAnomalyListPassesTheEnginesMetadataThrough(t *testing.T) { + f := newAnomalyFixture(t) + f.recordFlagged(t, store.EventCredentialStuffingDetected, map[string]string{ + "signals": "account_spray", + "distinct_accounts": "17", + "unknown_targets": "3", + }) + + got := decodeAnomalyList(t, f.get(t, "/v1/admin/anomalies", f.opToken)) + + metadata := got.Data.Anomalies[0].Metadata + for key, want := range map[string]string{"signals": "account_spray", "distinct_accounts": "17", "unknown_targets": "3"} { + if metadata[key] != want { + t.Errorf("metadata[%q] = %q, want %q", key, metadata[key], want) + } + } +} + +// A review is attached to the event it was made about, and reads back on +// the queue without the event itself changing. +func TestReviewingAnEventShowsUpOnTheQueue(t *testing.T) { + f := newAnomalyFixture(t) + eventID := f.recordFlagged(t, store.EventAnomalyDetected, map[string]string{"signals": "new_ip"}) + f.declare(eventID) + + rec := f.put(t, "/v1/admin/anomalies/"+eventID, f.opToken, `{"status":"confirmed","note":"real, from the office VPN"}`) + if rec.Code != http.StatusOK { + t.Fatalf("review status = %d, want 200 (body %s)", rec.Code, rec.Body.String()) + } + + var reviewed anomalyReviewResponse + if err := json.Unmarshal(rec.Body.Bytes(), &reviewed); err != nil { + t.Fatalf("decoding %s: %v", rec.Body.String(), err) + } + if reviewed.Data.Status != "confirmed" { + t.Errorf("status = %q, want confirmed", reviewed.Data.Status) + } + if reviewed.Data.Note != "real, from the office VPN" { + t.Errorf("note = %q, want the note that was sent", reviewed.Data.Note) + } + // The reviewer is the authenticated operator, taken from the token + // rather than the body — a caller does not get to say who decided. + if reviewed.Data.ReviewerID != f.opID { + t.Errorf("reviewer_id = %q, want the calling operator %q", reviewed.Data.ReviewerID, f.opID) + } + if reviewed.Data.UpdatedAt == nil { + t.Error("updated_at missing on a recorded review") + } + + got := decodeAnomalyList(t, f.get(t, "/v1/admin/anomalies", f.opToken)) + if got.Data.Anomalies[0].Review.Status != "confirmed" { + t.Errorf("queue status = %q, want confirmed", got.Data.Anomalies[0].Review.Status) + } + if got.Data.Anomalies[0].Review.Note != "real, from the office VPN" { + t.Errorf("queue note = %q, want the note that was sent", got.Data.Anomalies[0].Review.Note) + } +} + +// Dismissing keeps the row. The decision is a status, not a delete, and +// withdrawing it is a third status rather than an absence — so the record +// that an operator looked, and who they were, survives the change of mind. +func TestDismissingAndWithdrawingBothKeepTheRow(t *testing.T) { + f := newAnomalyFixture(t) + eventID := f.recordFlagged(t, store.EventAnomalyDetected, map[string]string{"signals": "new_ip"}) + f.declare(eventID) + + if rec := f.put(t, "/v1/admin/anomalies/"+eventID, f.opToken, `{"status":"dismissed"}`); rec.Code != http.StatusOK { + t.Fatalf("dismiss status = %d, want 200 (body %s)", rec.Code, rec.Body.String()) + } + if status, ok := f.reviews.Status(eventID); !ok || status != anomalyreview.StatusDismissed { + t.Fatalf("stored status = %q (present %v), want dismissed", status, ok) + } + + // Withdrawing the judgement stores unreviewed rather than removing + // the row. + if rec := f.put(t, "/v1/admin/anomalies/"+eventID, f.opToken, `{"status":"unreviewed"}`); rec.Code != http.StatusOK { + t.Fatalf("withdraw status = %d, want 200 (body %s)", rec.Code, rec.Body.String()) + } + if f.reviews.Len() != 1 { + t.Fatalf("review rows = %d, want 1 — withdrawing a judgement keeps the row", f.reviews.Len()) + } + if status, ok := f.reviews.Status(eventID); !ok || status != anomalyreview.StatusUnreviewed { + t.Errorf("stored status = %q (present %v), want unreviewed", status, ok) + } + + got := decodeAnomalyList(t, f.get(t, "/v1/admin/anomalies", f.opToken)) + if got.Data.Anomalies[0].Review.Status != "unreviewed" { + t.Errorf("queue status = %q, want unreviewed", got.Data.Anomalies[0].Review.Status) + } +} + +// The event cryden recorded is not touched by a review. Nothing in this api +// rewrites the engine's audit history — that is the evidence the review is +// about. +func TestReviewingAnEventDoesNotChangeTheEvent(t *testing.T) { + f := newAnomalyFixture(t) + eventID := f.recordFlagged(t, store.EventAnomalyDetected, map[string]string{"signals": "new_ip"}) + f.declare(eventID) + + before, err := f.audit.SearchByType(context.Background(), store.EventAnomalyDetected, 1) + if err != nil { + t.Fatalf("reading the event: %v", err) + } + beforeCount := countAnomalies(t, f) + + if rec := f.put(t, "/v1/admin/anomalies/"+eventID, f.opToken, `{"status":"dismissed","note":"scanner"}`); rec.Code != http.StatusOK { + t.Fatalf("review status = %d, want 200 (body %s)", rec.Code, rec.Body.String()) + } + + after, err := f.audit.SearchByType(context.Background(), store.EventAnomalyDetected, 1) + if err != nil { + t.Fatalf("re-reading the event: %v", err) + } + if after[0].ID != before[0].ID || !after[0].CreatedAt.Equal(before[0].CreatedAt) { + t.Error("the audit event changed; a review must not rewrite the engine's own record") + } + if after[0].Metadata["signals"] != "new_ip" { + t.Errorf("metadata = %v, want it untouched", after[0].Metadata) + } + // A review is not an audit event: reviewing must not add one, or the + // queue would grow every time somebody worked through it. + if got := countAnomalies(t, f); got != beforeCount { + t.Errorf("flagged events = %d after a review, want %d — a review is not an audit event", got, beforeCount) + } +} + +func countAnomalies(t *testing.T, f anomalyFixture) int { + t.Helper() + total := 0 + for _, eventType := range anomalyEventTypes { + events, err := f.audit.SearchByType(context.Background(), eventType, 1000) + if err != nil { + t.Fatalf("counting %s: %v", eventType, err) + } + total += len(events) + } + return total +} + +// A review of an event that does not exist is refused, not stored. The +// check comes from the store — in Postgres, from the foreign key against +// audit_events — so a console acting on a stale list is told. +func TestReviewingAnEventThatDoesNotExistIs404(t *testing.T) { + f := newAnomalyFixture(t) + + // A well-formed id that names nothing. + rec := f.put(t, "/v1/admin/anomalies/01a0a4ce-5453-78d3-9126-0000000000ff", f.opToken, `{"status":"dismissed"}`) + if rec.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404 (body %s)", rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), "audit_event_not_found") { + t.Errorf("body = %s, want audit_event_not_found", rec.Body.String()) + } + if f.reviews.Len() != 0 { + t.Errorf("review rows = %d, want 0 — a refused review stores nothing", f.reviews.Len()) + } +} + +// A path segment that cannot be an event id is a 404, not the 500 a +// malformed uuid handed to Postgres would produce. +func TestReviewingAMalformedEventIDIs404(t *testing.T) { + f := newAnomalyFixture(t) + + rec := f.put(t, "/v1/admin/anomalies/not-a-uuid", f.opToken, `{"status":"dismissed"}`) + if rec.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404 (body %s)", rec.Code, rec.Body.String()) + } + // The body names what was missing: an audit event, not a user. + if !strings.Contains(rec.Body.String(), "audit_event_not_found") { + t.Errorf("body = %s, want audit_event_not_found rather than a user-shaped 404", rec.Body.String()) + } +} + +func TestReviewRejectsABadBody(t *testing.T) { + f := newAnomalyFixture(t) + eventID := f.recordFlagged(t, store.EventAnomalyDetected, map[string]string{"signals": "new_ip"}) + f.declare(eventID) + + path := "/v1/admin/anomalies/" + eventID + for name, body := range map[string]string{ + "unknown status": `{"status":"maybe"}`, + "empty status": `{"status":""}`, + "no status": `{"note":"looks fine"}`, + "malformed json": `{"status":`, + "status not text": `{"status":42}`, + } { + rec := f.put(t, path, f.opToken, body) + if rec.Code != http.StatusBadRequest { + t.Errorf("%s: status = %d, want 400 (body %s)", name, rec.Code, rec.Body.String()) + } + } + if f.reviews.Len() != 0 { + t.Errorf("review rows = %d, want 0 after every request was refused", f.reviews.Len()) + } +} + +// A note longer than the limit is refused with a code a console can branch +// on rather than a generic bad request. +func TestReviewRejectsAnOverlongNote(t *testing.T) { + f := newAnomalyFixture(t) + eventID := f.recordFlagged(t, store.EventAnomalyDetected, map[string]string{"signals": "new_ip"}) + f.declare(eventID) + + body, err := json.Marshal(map[string]string{"status": "dismissed", "note": strings.Repeat("x", 501)}) + if err != nil { + t.Fatalf("building the body: %v", err) + } + rec := f.put(t, "/v1/admin/anomalies/"+eventID, f.opToken, string(body)) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400 (body %s)", rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), "invalid_review_note") { + t.Errorf("body = %s, want invalid_review_note", rec.Body.String()) + } +} + +// The status filter narrows to one decision, and an unreviewed event is +// matched by status=unreviewed even though it has no row — which is the +// case a console's "needs attention" tab is built on. +func TestAnomalyListFiltersByStatus(t *testing.T) { + f := newAnomalyFixture(t) + + confirmed := f.recordFlagged(t, store.EventAnomalyDetected, map[string]string{"signals": "new_ip"}) + f.declare(confirmed) + unreviewed := f.recordFlagged(t, store.EventAnomalyDetected, map[string]string{"signals": "new_device"}) + f.declare(unreviewed) + + if rec := f.put(t, "/v1/admin/anomalies/"+confirmed, f.opToken, `{"status":"confirmed"}`); rec.Code != http.StatusOK { + t.Fatalf("review status = %d, want 200 (body %s)", rec.Code, rec.Body.String()) + } + + got := decodeAnomalyList(t, f.get(t, "/v1/admin/anomalies?status=confirmed", f.opToken)) + if len(got.Data.Anomalies) != 1 || got.Data.Anomalies[0].ID != confirmed { + t.Fatalf("confirmed filter = %+v, want only the confirmed event", got.Data.Anomalies) + } + if got.Data.Status != "confirmed" { + t.Errorf("status = %q, want the filter echoed", got.Data.Status) + } + + // The event with no row at all is unreviewed. + got = decodeAnomalyList(t, f.get(t, "/v1/admin/anomalies?status=unreviewed", f.opToken)) + if len(got.Data.Anomalies) != 1 || got.Data.Anomalies[0].ID != unreviewed { + t.Fatalf("unreviewed filter = %+v, want the event with no review row", got.Data.Anomalies) + } + + // A status that is not one of the three is a 400, not an empty list — + // an operator who mistyped should be told, not shown "nothing here". + rec := f.get(t, "/v1/admin/anomalies?status=maybe", f.opToken) + if rec.Code != http.StatusBadRequest { + t.Errorf("bad status: status = %d, want 400 (body %s)", rec.Code, rec.Body.String()) + } +} + +// Paging walks the merged queue without repeating or dropping a row, which +// is the property the over-fetch-and-slice exists to provide: each type is +// fetched to limit+offset, so the merged window is an exact prefix. +func TestAnomalyListPagesTheMergedQueue(t *testing.T) { + f := newAnomalyFixture(t) + + // Interleaved types, so a page boundary falls between two of them. + var want []string + for i := range 6 { + eventType := store.EventAnomalyDetected + if i%2 == 1 { + eventType = store.EventCredentialStuffingDetected + } + want = append(want, f.recordFlagged(t, eventType, map[string]string{"signals": "new_ip"})) + } + // Newest first. + for i, j := 0, len(want)-1; i < j; i, j = i+1, j-1 { + want[i], want[j] = want[j], want[i] + } + + var got []string + for offset := 0; offset < len(want); offset += 2 { + page := decodeAnomalyList(t, f.get(t, "/v1/admin/anomalies?limit=2&offset="+strconv.Itoa(offset), f.opToken)) + for _, a := range page.Data.Anomalies { + got = append(got, a.ID) + } + if page.Data.Offset != offset || page.Data.Limit != 2 { + t.Errorf("offset %d: echoed limit/offset = %d/%d, want 2/%d", offset, page.Data.Limit, page.Data.Offset, offset) + } + } + if len(got) != len(want) { + t.Fatalf("paged %d events over three pages, want %d", len(got), len(want)) + } + for i := range want { + if got[i] != want[i] { + t.Errorf("paged[%d] = %s, want %s", i, got[i], want[i]) + } + } +} + +// Paging past what the merge can fetch is refused rather than clamped: a +// silently clamped offset would return a page from further up the queue +// than the caller asked for, which on a review queue means showing events +// they have already dealt with. +func TestAnomalyListRefusesAnOffsetBeyondWhatItCanFetch(t *testing.T) { + f := newAnomalyFixture(t) + + rec := f.get(t, "/v1/admin/anomalies?limit=500&offset=1", f.opToken) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400 (body %s)", rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), "limit + offset") { + t.Errorf("body = %s, want the reason the window was refused", rec.Body.String()) + } +} + +// A page that comes back short is the end of the queue; a full one is not +// a promise of more, only a reason to ask again. +func TestAnomalyListReportsWhetherToAskAgain(t *testing.T) { + f := newAnomalyFixture(t) + f.recordFlagged(t, store.EventAnomalyDetected, map[string]string{"signals": "new_ip"}) + + full := decodeAnomalyList(t, f.get(t, "/v1/admin/anomalies?limit=1", f.opToken)) + if !full.Data.HasMore { + t.Error("has_more = false on a full page, want true so a console asks again") + } + + short := decodeAnomalyList(t, f.get(t, "/v1/admin/anomalies?limit=10", f.opToken)) + if short.Data.HasMore { + t.Error("has_more = true on a short page, want false — the merged window is an exact prefix, so a short page is the end") + } +} + +// The queue is the two flagged types and nothing else. Widening it to the +// failure events around them would make the audit table the queue. +func TestAnomalyListExcludesOrdinaryFailureEvents(t *testing.T) { + f := newAnomalyFixture(t) + + f.recordFlagged(t, store.EventLoginFailed, nil) + f.recordFlagged(t, store.EventTokenReuseDetected, nil) + flagged := f.recordFlagged(t, store.EventAnomalyDetected, map[string]string{"signals": "new_ip"}) + + got := decodeAnomalyList(t, f.get(t, "/v1/admin/anomalies", f.opToken)) + if len(got.Data.Anomalies) != 1 || got.Data.Anomalies[0].ID != flagged { + t.Fatalf("queue = %+v, want only the flagged event", got.Data.Anomalies) + } +} + +func TestAnomalyRoutesAreGatedByRequireAdmin(t *testing.T) { + f := newAnomalyFixture(t) + + // The detail path is PUT-only — a single flagged event is read as part + // of the queue, not on its own, because the engine has no lookup by + // event id to serve one from. + if rec := f.put(t, "/v1/admin/anomalies/01a0a4ce-5453-78d3-9126-0000000000ff", "", `{"status":"dismissed"}`); rec.Code != http.StatusUnauthorized { + t.Errorf("PUT with no token: status = %d, want 401", rec.Code) + } + if rec := f.put(t, "/v1/admin/anomalies/01a0a4ce-5453-78d3-9126-0000000000ff", f.userToken, `{"status":"dismissed"}`); rec.Code != http.StatusForbidden { + t.Errorf("PUT with an ordinary user's token: status = %d, want 403 (body %s)", rec.Code, rec.Body.String()) + } +} + +// A router built without the review store answers 404 — the feature is +// unavailable, not broken. +func TestAnomalyRoutesWithoutStoresAre404(t *testing.T) { + f := newUserFixture(t) + + if rec := f.call(t, http.MethodGet, "/v1/admin/anomalies", f.opToken); rec.Code != http.StatusNotFound { + t.Errorf("GET: status = %d, want 404 (body %s)", rec.Code, rec.Body.String()) + } + rec := f.put(t, "/v1/admin/anomalies/01a0a4ce-5453-78d3-9126-0000000000ff", f.opToken, `{"status":"dismissed"}`) + if rec.Code != http.StatusNotFound { + t.Errorf("PUT: status = %d, want 404 (body %s)", rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), "not_configured") { + t.Errorf("body = %s, want not_configured", rec.Body.String()) + } +} + +// The merged order must be total, not merely newest-first: two events +// recorded in the same instant — a stuffing burst writes several in one +// transaction — would otherwise come back in whichever order the database +// happened to return them, and a queue that reshuffles between two +// identical requests cannot be paged through. +func TestSortAnomaliesBreaksTiesDeterministically(t *testing.T) { + same := time.Date(2026, 3, 1, 12, 0, 0, 0, time.UTC) + events := []store.AuditEvent{ + {ID: "01a0a4ce-5453-78d3-9126-00000000000c", CreatedAt: same}, + {ID: "01a0a4ce-5453-78d3-9126-00000000000a", CreatedAt: same}, + {ID: "01a0a4ce-5453-78d3-9126-00000000000b", CreatedAt: same}, + } + + sortAnomalies(events) + + for i, want := range []string{ + "01a0a4ce-5453-78d3-9126-00000000000a", + "01a0a4ce-5453-78d3-9126-00000000000b", + "01a0a4ce-5453-78d3-9126-00000000000c", + } { + if events[i].ID != want { + t.Errorf("events[%d].id = %s, want %s", i, events[i].ID, want) + } + } + + // And a genuinely newer event still wins on time, so the tie-break is + // a tie-break rather than the ordering. + events = append(events, store.AuditEvent{ + ID: "01a0a4ce-5453-78d3-9126-000000000000", + CreatedAt: same.Add(time.Second), + }) + sortAnomalies(events) + if events[0].ID != "01a0a4ce-5453-78d3-9126-000000000000" { + t.Errorf("events[0].id = %s, want the newest event first", events[0].ID) + } +} diff --git a/httpapi/router.go b/httpapi/router.go index 5987d93..38e18c6 100644 --- a/httpapi/router.go +++ b/httpapi/router.go @@ -7,6 +7,7 @@ import ( "github.com/crydensync/cryden/v2" "github.com/crydensync/cryden/v2/store" + "github.com/crydensync/api/anomalyreview" "github.com/crydensync/api/config" "github.com/crydensync/api/digest" "github.com/crydensync/api/settings" @@ -84,6 +85,13 @@ type Deps struct { // not_configured rather than accepting one they would have to store in // the clear. See settings.Secrets. Settings *settings.Secrets + + // Reviews backs the flagged-event review queue. This repo's own table + // (migrations/014) and package — cryden records that a login tripped + // anomaly signals and has no concept of a person having read one, so + // the judgement lives here rather than in the engine's audit history. + // See anomalyreview's package doc. + Reviews anomalyreview.Store } // NewRouter builds the full route table. Called once from main.go. @@ -111,6 +119,7 @@ func NewRouter(d Deps) http.Handler { support := &SupportHandlers{Engine: engine} tuning := &TuningHandlers{Audit: d.Audit, Config: d.Config} aiSettings := &SettingsHandlers{Secrets: d.Settings} + anomalies := &AnomalyHandlers{Audit: d.Audit, Reviews: d.Reviews} mux := http.NewServeMux() @@ -205,12 +214,14 @@ func NewRouter(d Deps) http.Handler { // this repo's own logic — cryden has no concept of a provider being // reachable, and no bulk way to read stored hash algorithms. // - // Read-only is the default and every write here is deliberate: the - // per-user metadata block below (a write is the whole feature) and the - // settings block at the bottom (a settings save, the one path a tuning - // suggestion may pre-fill). Neither is reachable from an AI tool, which - // is what CLAUDE.md's hard rule actually protects. See README's note on - // the admin surface. + // Read-only is the default and every write here is deliberate. There + // are three, and each is a named exception rather than a category: the + // per-user metadata block below (a write is the whole feature), the + // flagged-event review block (an operator recording a judgement they + // made by hand), and the settings block at the bottom (a settings + // save, the one path a tuning suggestion may pre-fill). None is + // reachable from an AI tool, which is what CLAUDE.md's hard rule + // actually protects. See README's note on the admin surface. mux.HandleFunc("GET /v1/admin/oauth/health", RequireAdmin(engine, oauthHealth.Health)) mux.HandleFunc("GET /v1/admin/security/hash-migration", RequireAdmin(engine, security.HashMigration)) // Second-factor enrolment, as the engine's own audit events against the @@ -288,11 +299,22 @@ func NewRouter(d Deps) http.Handler { // settings path. See TuningHandlers and CLAUDE.md's hard rule. mux.HandleFunc("GET /v1/admin/config-tuning", RequireAdmin(engine, tuning.ConfigTuning)) + // The flagged-event review queue — what the engine flagged, and what a + // human decided about it. GET is read-only; PUT records a judgement + // and nothing else, which is the third write on this surface. A + // confirmation takes no action on any account, deliberately: there is + // no machinery here that acts, so there is nothing for a confirm + // button to trigger. See AnomalyHandlers. + mux.HandleFunc("GET /v1/admin/anomalies", RequireAdmin(engine, anomalies.List)) + mux.HandleFunc("PUT /v1/admin/anomalies/{eventID}", RequireAdmin(engine, func(w http.ResponseWriter, r *http.Request) { + anomalies.Review(w, r, r.PathValue("eventID")) + })) + // The AI settings surface — the "human saves it" half of - // pre-fill-never-auto-apply, and one of exactly two write blocks on - // this surface (the other is the per-user metadata block above; an - // earlier version of this comment claimed there was only one, which - // was wrong). + // pre-fill-never-auto-apply, and the third of the three write blocks + // on this surface (the others are the per-user metadata block and the + // flagged-event review block above; an earlier version of this comment + // claimed there was only one, which was wrong). // // The read-only rule above is about the AI *tools*, which are what the // engine's interfaces make read-only by carrying no method that can diff --git a/main.go b/main.go index 6b5f6c1..9468dbd 100644 --- a/main.go +++ b/main.go @@ -16,6 +16,7 @@ import ( "github.com/crydensync/cryden/v2/security" "github.com/crydensync/cryden/v2/store/postgres" + "github.com/crydensync/api/anomalyreview" "github.com/crydensync/api/config" "github.com/crydensync/api/digest" "github.com/crydensync/api/httpapi" @@ -58,6 +59,12 @@ func main() { // reserved-key rule lives in the store rather than in the handler. metadata := usermeta.NewStore(db) + // Flagged-event reviews: this repo's own table too. cryden records + // that a login tripped anomaly signals and has no concept of an + // operator having read one, so the judgement is stored here, keyed on + // the audit event id. See anomalyreview's package doc. + reviews := anomalyreview.NewStore(db) + // Webhook delivery log: this repo's own table, and the queue the // sender writes to. Declared as the interface rather than as // *webhook.PostgresStore so that leaving WEBHOOK_URL unset leaves it @@ -381,6 +388,8 @@ func main() { Digests: digestStore, Settings: settingsSecrets, + + Reviews: reviews, }) limiter := httpapi.NewEdgeRateLimiter(cfg.EdgeRateLimit, cfg.EdgeRateLimitWindow) handler := httpapi.WithCORS(cfg.CORSOrigins, httpapi.WithEdgeRateLimit(limiter, router)) From 9ba061a13f438c7f26bf575f6cc97f457ab5598a Mon Sep 17 00:00:00 2001 From: Raymond Nicholas Date: Wed, 16 Sep 2026 21:46:29 +0100 Subject: [PATCH 6/6] docs: write up Tier 5 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CURRENT-STATE, NEXT and PROGRESS get the tier's section: the user surface, the MFA adoption report's events-not-users decision, the review queue's two settled decisions, and what is owed — migration 014 never applied, anomalyreview.PostgresStore never run against a real Postgres, no Docker or Postgres reachable here, -race not run. Also corrects three earlier claims that the settings routes were the admin surface's first write. They were not: Tier 3's metadata PUT and DELETE had written since then. The reading those passages justify is unaffected — what needed arguing was whether a settings save is the kind of write the rule forbids, not whether the surface was read-only, which it never was. openapi goes to 1.6 with the four new paths and their schemas; README gains the three sections and its admin route list is completed. Co-Authored-By: Claude Code --- README.md | 83 ++++- docs/development/CURRENT-STATE.md | 106 +++++- docs/development/NEXT.md | 80 ++++- docs/development/PROGRESS.md | 147 ++++++++- openapi/spec.yaml | 516 +++++++++++++++++++++++++++++- 5 files changed, 911 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index 46e1be5..836ac01 100644 --- a/README.md +++ b/README.md @@ -161,13 +161,25 @@ POST /v1/api-keys (auth required, raw key returned once) GET /v1/api-keys (auth required) DELETE /v1/api-keys/{keyID} (auth required) -GET /v1/admin/oauth/health (admin required) +GET /v1/admin/oauth/health (admin required) GET /v1/admin/security/hash-migration (admin required) +GET /v1/admin/security/mfa-adoption (admin required) +GET /v1/admin/users (admin required) +GET /v1/admin/users/{userID} (admin required) GET /v1/admin/users/{userID}/metadata (admin required) PUT /v1/admin/users/{userID}/metadata/{key} (admin required) DELETE /v1/admin/users/{userID}/metadata/{key} (admin required) -GET /v1/admin/webhooks/deliveries (admin required) -GET /v1/admin/logging/recent (admin required) +GET /v1/admin/anomalies (admin required) +PUT /v1/admin/anomalies/{eventID} (admin required) +GET /v1/admin/webhooks/deliveries (admin required) +GET /v1/admin/logging/recent (admin required) +GET /v1/admin/digest (admin required) +GET /v1/admin/digest/history (admin required) +GET /v1/admin/support/diagnose (admin required) +GET /v1/admin/config-tuning (admin required) +GET|PUT|DELETE /v1/admin/settings/llm-provider (admin required) +GET|PUT|DELETE /v1/admin/settings/database-provider (admin required) +GET|PUT|DELETE /v1/admin/settings/ask-ai-widget (admin required) ``` `GET /v1/sessions` answers with *named* sessions: each entry keeps its `id`, `ip`, `user_agent` and `created_at`, and gains `label`, `device` and `location`, all computed on read from the session's own IP and User-Agent — nothing new is stored and no migration exists for it. `label` is the string a "your devices" screen shows (`Chrome on macOS`, or `Unknown device` for a client that sent no User-Agent). `location` is present but empty unless a geolocator is configured, and this repo wires none on purpose: every implementation of that interface calls somebody else's internet service, which is a deployment's decision rather than this repo's. The response shape is documented in `openapi/spec.yaml`. @@ -246,6 +258,65 @@ Set `PASSWORD_HASHER=argon2id` and every login whose stored hash is out of date - **Read-only structurally, not by convention.** The report is built through interfaces carrying no `LockAccount`, `ResetFailedAttempts` or `Revoke`, so the endpoint cannot unlock the very account it is describing, whatever the caller asks for. That is cryden's design and this repo adds nothing on top of it. - A missing `email` is a `400`, not a diagnosis of the empty string — which would come back as "no account exists", an answer to a question nobody asked. +`GET /v1/admin/security/mfa-adoption` reports second-factor enrolment the same way, from the engine's own audit events: + +```json +{"data": { + "total_users": 1234, + "window_days": 7, + "factors": [ + {"factor": "totp", "enrolled_events": 300, "removed_events": 40, + "enrolled_events_in_window": 12, "removed_events_in_window": 3}, + {"factor": "passkey", "enrolled_events": 90, "removed_events": 2, + "enrolled_events_in_window": 7, "removed_events_in_window": 0} + ] +}} +``` + +**It is not "N of M users have MFA", and that is not an oversight.** cryden cannot be asked how many accounts have a factor enrolled: `TOTPStore` and `WebAuthnCredentialStore` are per-user (`GetByUserID`, `ListByUser`) with no `Count` and no `ListAll`. Counting the rows in `totp_secrets` from here would answer it exactly and would also be this repo writing SQL against the engine's own schema — the boundary drawn in [Design notes](#design-notes). So the report gives what the engine does record system-wide, and names every field accordingly: + +- **Every field counts events, not users.** A user who turns TOTP on, loses their phone and turns it off contributes one enrolment and one removal and is enrolled zero times over. `enrolled_events` is not an adoption figure, and no percentage is derived from it anywhere in the API — a ratio of events to `total_users` would look like coverage and move for the wrong reasons. +- **The pair against each other is the honest read.** A factor whose removals keep pace with its enrolments is churning; the windowed pair says whether that is happening now. +- **Recovery codes are not in the list.** They are a fallback for an account that already has a second factor, not a factor of their own, so counting `recovery_codes_generated` as enrolment would report a different thing than the label says. +- Both factors are always present, including at zero. A report that omitted a factor with no events would leave a console unable to tell "nobody has enrolled" from "this deployment does not support it". + +## The user surface + +Two endpoints, both read-only, for the console's account screen: + +``` +GET /v1/admin/users?q=&limit=&offset= # exact-email lookup, or browse +GET /v1/admin/users/{userID} # one account, its sessions, its history +``` + +This is the one place in this API where an operator can see an account that is not their own, so what is *not* here matters as much as what is. There is no lock, no unlock, no password reset and no delete. cryden's store exposes `LockAccount`, and wiring it to a button would make this repo the thing that can lock somebody out of their account; an operator who needs that has the engine's own admin path, not an HTTP endpoint this repo invented. The detail view reports lockout state so it can be diagnosed, and stops there. + +- **The email search is exact and case-sensitive, and the response says so.** `q` goes to `cryden.GetUser`, which is `WHERE email = $1` — cryden stores addresses exactly as typed and has no `citext`, so `Alice@example.com` does not find an account created as `alice@example.com`. Every response carries `match: "exact_email"` or `"browse"` so a console can label the result and explain a zero-result search, rather than leaving an operator to conclude the account is gone. A search that finds nothing is an empty `200`, never a `404` — answering `404` would make a console render "error" for the most ordinary outcome a search has. +- **There is no partial search, deliberately.** A `LIKE` against cryden's `users` table would cross the ownership boundary this repo keeps everywhere else: the engine owns that table, and a query written here would be a second, silent definition of what a user is. If partial search is wanted later, it is its own deliberate piece of work. +- **`locked` is computed, not mirrored.** cryden clears a lockout by time passing rather than by writing a null, so a row can carry a `locked_until` already in the past — reporting `locked: true` for a non-nil column would tell an operator an account is locked out when it is not, and the ones that look like that are exactly the ones somebody just waited out. +- **Sessions are counted, not listed.** `active_sessions` is a number. cryden's `ListByUser` returns only live sessions, so the count is honest; listing them would publish every IP and user agent an account has signed in from to anyone holding an operator token. The support assistant's report makes the same choice for the same reason. +- **`PasswordHash` is on the struct these are built from and never on the wire.** A struct tag is not a guarantee, so a test asserts against the raw response body for any hash-shaped field — and proves the assertion is not vacuous by confirming the stored user really does have one. + +## Flagged-event review queue + +`GET /v1/admin/anomalies` is the queue of what the engine flagged, and `PUT /v1/admin/anomalies/{eventID}` is where an operator records what they decided about it: + +``` +GET /v1/admin/anomalies?status=&limit=&offset= +PUT /v1/admin/anomalies/{eventID} {"status": "confirmed", "note": "real, from the office VPN"} +``` + +The queue is the two event types cryden writes when a login looks *wrong* rather than merely failing — `anomaly_detected` and `credential_stuffing_detected` — merged newest first, each carrying its review. Both carry a `signals` key naming what tripped, which is what makes them reviewable: an operator can read the event and form a judgement. Widening this to the failure events around them (`login_failed`, `token_reuse_detected`) would not make a bigger queue, it would make the audit table the queue. + +- **A review is a row in this repo's own table, keyed on the audit event id.** The event the engine recorded reads exactly the same before and after — nothing here rewrites cryden's audit history. The id is what a console has in hand and what the engine's record is filed under. +- **Nothing deletes.** Dismissing is a status, not a removal, and withdrawing a judgement stores `unreviewed` rather than dropping the row — so the record that somebody looked, and who they were, survives the change of mind. See `anomalyreview`'s package doc. +- **Confirming takes no action.** Marking an event real records the judgement and nothing else: no account is locked, no session revoked, no threshold tuned. There is no machinery in this repo that acts on an account beyond what an operator does by hand, which is exactly what keeps this on the right side of the read-only rule rather than being an exception to it. +- **An event that does not exist is refused, not stored.** cryden has no lookup by event id, so Go cannot check that a flagged event is real — the table does it instead, with a foreign key from `reviewed_anomalies.event_id` to `audit_events.id` and a clean `404 audit_event_not_found` from the resulting SQLSTATE. A console acting on a stale list is told rather than shown a success that annotates nothing. +- **`status=unreviewed` matches an event with no row at all.** The two are the same thing by design, so the filter compares against the default the response reports rather than against what the store happened to return — otherwise the one tab an operator opens first would be empty. +- **Paging is refused past what the merge can fetch.** cryden's per-type search takes a limit and no offset, so the merged window is fetched to `limit + offset` from each type and sliced here. That makes each page an exact prefix of the queue rather than a sample of it — but `limit + offset` beyond 500 is a `400`, not a silent clamp, because a clamped offset would return a page from further up the queue than the caller asked for, which on a review queue means showing events they have already dealt with. +- **`has_more` means "this page came back full, ask again", not "there is more".** Deciding the latter exactly would mean knowing the queue's total size and the fetches give a window per type, so the endpoint says what it can support. A page short of `limit` *is* the end, precisely because the merged window is an exact prefix. +- **The response to a save is the review alone, not the queue row.** Re-reading the event would need a lookup by event id that the engine does not have, so a save returns the decision and a client refreshes the list it already has. + ## API keys `POST /v1/api-keys` mints a machine-to-machine credential for the calling user and returns the raw key **once** — cryden stores only its SHA-256 hash and can never reproduce it, so a caller that loses it has to mint a new one. The response carries the raw key, the stored record (`id`, `name`, `prefix`, `scopes`, `expires_at`, `expired`, `created_at`, `last_used_at`) and a `notice` saying so; a client that renders the key without that notice is the failure this guards against. @@ -380,7 +451,7 @@ GET|PUT|DELETE /v1/admin/settings/database-provider GET|PUT|DELETE /v1/admin/settings/ask-ai-widget ``` -These are the admin surface's **only** writes, and they are the other half of the read-only rule rather than a hole in it. A tuning suggestion pre-fills one of these forms; an operator presses save; this is what handles that save. No AI-assisted handler in this repo holds a reference to any of them, and none accepts a suggestion as input. +These are one of the admin surface's three write blocks, and they are the other half of the read-only rule rather than a hole in it. A tuning suggestion pre-fills one of these forms; an operator presses save; this is what handles that save. No AI-assisted handler in this repo holds a reference to any of them, and none accepts a suggestion as input. All three answer `404 not_configured` when `SETTINGS_ENCRYPTION_KEY` is unset — without a key there is nowhere safe to put a credential, so the API refuses rather than storing one in the clear. @@ -421,8 +492,8 @@ The LLM API key and the database connection string are sealed with **AES-256-GCM - A paused login is a `200`, not an error: nothing failed, the caller just has one more step. `httpapi/second_factor.go` is the one place that response shape is written. - `DELETE /v1/passkeys/{credentialID}` takes a JSON body (`{"password": "..."}`) — the password is re-confirmation, so a stolen access token alone cannot weaken an account's own auth requirements. - Passkey ceremony options and the browser's credential response travel as raw JSON (an object, not a JSON-encoded string), since that is exactly what `navigator.credentials.create()`/`.get()` produce and consume. -- **Five of this repo's tables are not cryden's and never will be**: `user_metadata`, `webhook_deliveries`, `shipped_log_events`, `digest_runs` and `settings`. cryden calls an interface and moves on; it keeps no queryable history of what a sender or a logger did, no schedule, no run record, and no configuration storage — and `store.User` has no metadata concept on purpose. Each lives in its own package (`usermeta/`, `webhook/`, `shiplog/`, `digest/`, `settings/`) with a Postgres store and an in-memory double behind one interface, mirroring the `store/interfaces.go` + `store/memory` + `store/postgres` split cryden itself uses — which is what makes an endpoint over them testable with no database. -- **The admin surface is read-only by default, and every write on it is a named exception.** `GET /v1/admin/webhooks/deliveries` and `GET /v1/admin/logging/recent` report; neither offers a "retry this delivery" button, a "replay this event", or any way to write a log record or a delivery row. That is the same rule cryden's AI admin tools are built under, carried across the repo boundary: an operator reads the state of the system, and every change to it goes through the explicit path that owns that change (or through the receiving system, for a delivery). Adding a write here is a design change, not a convenience, and there are exactly two of them. `PUT`/`DELETE /v1/admin/users/{userID}/metadata/{key}` writes per-user metadata, which becomes JWT claims — the write *is* the feature, and a read-only version of it would do nothing. `PUT`/`DELETE /v1/admin/settings/*` is a settings save, the "a human still saves it" half of the pre-fill rule, not an action any AI tool can reach; its credentials are encrypted at rest and it is the only place in this API that stores one. Both are an operator acting deliberately on a named thing, and neither is reachable from an AI feature — no tool holds a reference to either handler, and the engine's interfaces carry no method that could call one. If you are adding a write under `/v1/admin` that is neither of these, the answer is no. +- **Six of this repo's tables are not cryden's and never will be**: `user_metadata`, `webhook_deliveries`, `shipped_log_events`, `digest_runs`, `settings` and `reviewed_anomalies`. cryden calls an interface and moves on; it keeps no queryable history of what a sender or a logger did, no schedule, no run record, no configuration storage, and no record of a person having read one of its events — and `store.User` has no metadata concept on purpose. Each lives in its own package (`usermeta/`, `webhook/`, `shiplog/`, `digest/`, `settings/`, `anomalyreview/`) with a Postgres store and an in-memory double behind one interface, mirroring the `store/interfaces.go` + `store/memory` + `store/postgres` split cryden itself uses — which is what makes an endpoint over them testable with no database. `reviewed_anomalies` is the one that carries a foreign key back into cryden's schema (`event_id → audit_events(id)`), deliberately: it is the only way this repo can tell a real flagged event from a fabricated id, since the engine has no lookup by event id. +- **The admin surface is read-only by default, and every write on it is a named exception.** `GET /v1/admin/webhooks/deliveries` and `GET /v1/admin/logging/recent` report; neither offers a "retry this delivery" button, a "replay this event", or any way to write a log record or a delivery row. That is the same rule cryden's AI admin tools are built under, carried across the repo boundary: an operator reads the state of the system, and every change to it goes through the explicit path that owns that change (or through the receiving system, for a delivery). Adding a write here is a design change, not a convenience, and there are exactly three of them. `PUT`/`DELETE /v1/admin/users/{userID}/metadata/{key}` writes per-user metadata, which becomes JWT claims — the write *is* the feature, and a read-only version of it would do nothing. `PUT /v1/admin/anomalies/{eventID}` records an operator's judgement about an event the engine flagged, keyed on the audit event id; it takes no action on any account, which is why it is a record rather than an exception. `PUT`/`DELETE /v1/admin/settings/*` is a settings save, the "a human still saves it" half of the pre-fill rule, not an action any AI tool can reach; its credentials are encrypted at rest and it is the only place in this API that stores one. All three are an operator acting deliberately on a named thing, and none is reachable from an AI feature — no tool holds a reference to any of these handlers, and the engine's interfaces carry no method that could call one. If you are adding a write under `/v1/admin` that is none of these, the answer is no. - `webhook_deliveries.id` is a `BIGSERIAL` surrogate key rather than the natural key you might expect. The event id it corresponds to **can be empty** — cryden generates it with `crypto/rand` and deliberately delivers an event without one rather than dropping it — and a delivery log whose primary key could be blank is a log that loses exactly the rows you would most want to see. The engine's own id is recorded beside it as `event_id` and is used for the receiver's idempotency. - This repo has **no graceful shutdown**, and as of this tier that is a stated gap rather than an unnoticed one: `main.go` ends at `log.Fatal(http.ListenAndServe(...))`, so the webhook worker's context is never cancelled and the shipped-events sink has no flush-and-exit path. Both were built so that adding one later is a change to `main.go` alone — the worker takes a `context.Context`, which today is `context.Background()`. The sink writes synchronously for the same reason: a buffered sink with no shutdown path drops its last records on a crash. diff --git a/docs/development/CURRENT-STATE.md b/docs/development/CURRENT-STATE.md index 04efa0b..346bf3e 100644 --- a/docs/development/CURRENT-STATE.md +++ b/docs/development/CURRENT-STATE.md @@ -451,8 +451,11 @@ has never called Anthropic (it is tested against a local fake in the Messages API's wire shape), and `013_settings` has never been applied to a database. -**The read-only rule now has a named exception, and it is this one.** -`/v1/admin/settings/*` is the admin surface's first write. The reading +**The read-only rule has a named exception, and this is the second one.** +The settings routes are writes, but they were not the admin surface's +first — Tier 3's `PUT`/`DELETE /v1/admin/users/{userID}/metadata/{key}` +have written since then, and Tier 5's `PUT /v1/admin/anomalies/{eventID}` +is a third. The reading is that `CLAUDE.md`'s rule covers the AI *tools* — which cryden builds through interfaces carrying no way to act — rather than every route under `/v1/admin`, and that a settings save is exactly what `NEXT.md`'s @@ -462,10 +465,97 @@ a suggestion as input. The alternative readings (store the key in cryden, or environment-only) are worse and one of them is explicitly ruled out by `NEXT.md`, which says this repo owns that config storage. -## Tier 5 - -Not started. See `NEXT.md` for the full, ordered, specced-in-detail -queue — the users admin surface, which has no engine gap and is just -missing endpoints, plus the widget's own serving endpoint, which is what -the Stage 2 config above is waiting for. +## Tier 5 — the users admin surface + +Built on `feat/tier5-users-admin-surface`. Four endpoints, one migration, +one new package. `NEXT.md`'s Tier 5 section carries the same account of +what was and was not done; this is the state rather than the log. + +**The user surface** — `GET /v1/admin/users` and `GET +/v1/admin/users/{userID}`. This is the one place in the API where an +operator can see an account that is not their own, so what is absent is +as load-bearing as what is present: no lock, no unlock, no password +reset, no delete. cryden's store exposes `LockAccount`, and wiring it to +a button would make this repo the thing that can lock somebody out of +their account. + +- The email search is **exact and case-sensitive**, and every response + says which mode produced it (`match: "exact_email"` or `"browse"`). + `q` goes to `cryden.GetUser`, which is `WHERE email = $1`; cryden + stores addresses as typed and has no `citext`. Partial search was + declined rather than deferred: a `LIKE` against cryden's `users` table + would be a second, silent definition of what a user is. A search that + finds nothing is an empty 200, never a 404. +- `locked` is **computed** from `LockedUntil` rather than mirrored from + the column, because cryden clears a lockout by time passing rather + than by writing a null — a non-nil `locked_until` in the past is the + normal state of an account somebody just waited out. +- `active_sessions` is a count, not a list. `ListByUser` returns only + live sessions so the count is honest; listing them would publish every + IP and user agent to anyone holding an operator token. +- `PasswordHash` is on the struct these are built from and never on the + wire. `TestAdminUserResponsesNeverCarryAPasswordHash` asserts that + against the raw body and proves the assertion is not vacuous by + confirming the stored user really does have a hash. + +**The MFA adoption report** — `GET /v1/admin/security/mfa-adoption`, +alongside the hash-migration report it is modelled on. It reports +enrolment and removal **events**, all-time and windowed, against the +user total, and derives **no adoption percentage**. cryden cannot answer +"how many accounts have a factor enrolled": `TOTPStore` and +`WebAuthnCredentialStore` are per-user with no `Count` and no `ListAll`, +and counting the rows in `totp_secrets` would be SQL against the +engine's schema. Every field is named `*_events` so the number cannot be +read as a user count. Recovery codes are excluded — they are a fallback +for an account that already has a factor, not a factor of their own. + +**The flagged-event review queue** — `GET /v1/admin/anomalies` and `PUT +/v1/admin/anomalies/{eventID}`, backed by the new `anomalyreview/` +package and migration `014_reviewed_anomalies`. Two decisions were +settled by the human before it was written: + +- **Dismiss is a status, not a delete.** `status IN + ('unreviewed','confirmed','dismissed')`, no DELETE anywhere. + Withdrawing a judgement stores `unreviewed` rather than removing the + row, so the record of who looked survives the change of mind. +- **The queue is keyed on the audit event id**, and the existence check + is carried by a foreign key from `reviewed_anomalies.event_id` to + `audit_events.id` — cryden has no lookup by event id, so Go cannot + check it and the database does. SQLSTATE `23503` maps to + `404 audit_event_not_found`, reusing the repo's existing + SQLSTATE-by-code precedent from `aiprovider/query.go`. +- **Confirming takes no action on any account** — no lock, no revoke, no + threshold change. There is no machinery here that acts, which is what + keeps this inside `CLAUDE.md`'s rule rather than being an exception to + it. +- The queue is the two `signals`-carrying types (`anomaly_detected`, + `credential_stuffing_detected`); widening it to the failure events + around them would make the audit table the queue. +- Paging over-fetches `limit + offset` per type and refuses beyond 500 + rather than clamping; `has_more` means "this page came back full, ask + again" and not "there is more", because the fetches give a window per + type rather than a total. + +**The third write on the admin surface.** The review endpoint joins +Tier 3's metadata `PUT`/`DELETE` and Tier 4's settings routes. The +reading recorded in Tier 4's section — that the rule covers the AI +*tools* rather than every route under `/v1/admin` — is unchanged, and +this tier is the first place the reading had to do real work rather than +just explain a settings form: a review is a record of a human judgement, +and the endpoint is built so that it cannot become an action. + +**What is still owed, said plainly.** Migration `014` has **never been +applied to a database**, and `anomalyreview.PostgresStore` has never run +against a real Postgres — no Docker or Postgres was reachable in the +environment this was built in, the same constraint Tier 4's Stage 2 +recorded for `013_settings`. The foreign key and its `23503` mapping are +verified by reasoning and by the in-memory double, which reproduces the +foreign key rather than accepting any id, so that the tested branch is +the one production runs. `-race` was not run this session. + +**Still not built** (unchanged from Tier 4, not part of this tier): the +widget's own serving endpoint, so `allowed_origins` remains stored and +unenforced; nothing constructs an `ai.LLMProvider` or +`ai.QueryableStore` from the stored config; and there is still no +graceful shutdown. diff --git a/docs/development/NEXT.md b/docs/development/NEXT.md index eed060c..c1a03fb 100644 --- a/docs/development/NEXT.md +++ b/docs/development/NEXT.md @@ -343,16 +343,22 @@ Two details were decided rather than assumed, and are recorded in > write a row, so an operator hitting it twenty times does not fill > the history with twenty near-identical reports. Only the scheduled > job writes. -> - **`/v1/admin/settings/*` is the admin surface's first write**, and -> the read-only rule below has been read as covering the AI *tools* -> rather than every route under `/v1/admin`. The reasoning is in -> `SettingsHandlers`' doc comment and in `CLAUDE.md`'s own wording: a +> - **The read-only rule below has been read as covering the AI +> *tools* rather than every route under `/v1/admin`.** The reasoning is +> in `SettingsHandlers`' doc comment and in `CLAUDE.md`'s own wording: a > settings save is what "a human still has to explicitly save that > change through the normal config UI" names, and no AI-assisted > handler holds a reference to it. The alternative reading — store the > LLM key in cryden, or in the environment only — is worse: the spec > below explicitly says this repo owns that config storage. > +> *(Correction: this bullet originally said `/v1/admin/settings/*` was +> the admin surface's first write. It was not — Tier 3's `PUT`/`DELETE +> /v1/admin/users/{userID}/metadata/{key}` have written since then, so +> the admin surface was never read-only and this tier did not change +> that. The reading above is unaffected; the claim about precedence +> was simply wrong.)* +> > The two decisions this tier had recorded as open were resolved by > following this file's own instruction to make the reasonable call and > note it: the live provider is built on the **official Anthropic Go @@ -444,18 +450,84 @@ tools' suggestions pre-fill. ## Tier 5 — users admin surface (new, no engine gap, just missing endpoints) +> **Status: built on `feat/tier5-users-admin-surface`.** All four +> endpoints exist, are wired in `main.go`, and are tested end to end on +> the in-memory stores — `go build`, `go vet` and `go test ./...` clean. +> Migration `014_reviewed_anomalies` is written and copied into +> `migrations/`; it has **never been applied to a database**, and +> `anomalyreview.PostgresStore` has never run against a real Postgres. +> No Docker or Postgres was available in the environment this was built +> in, so the migration's foreign key and its `23503` mapping are +> verified by reasoning and by the in-memory double, not by execution. +> `-race` was not run this session either. `PROGRESS.md` says all of this +> in full rather than implying a verification that did not happen. + - `GET /v1/admin/users?q=...&limit=...&offset=...` → `cryden.GetUser` for an exact match, `ListAll`/`Count` for browsing. Behind `RequireAdmin`. + **Built as specced, with the search narrowed to exact-only.** `q` goes + to `cryden.GetUser` and nothing else: partial search would mean SQL + against cryden's own `users` table, which is the ownership boundary + this repo has kept everywhere else. The match is also case-*sensitive*, + because cryden stores addresses exactly as typed and compares them + with `=` — so the response carries `match: "exact_email"` and a + console can explain a zero-result search instead of leaving an + operator to conclude the account is gone. A search that finds nothing + is an empty 200, never a 404. - `GET /v1/admin/users/{userID}` → account detail view, likely composing `GetUser` with session count and recent audit history. + **Built.** `active_sessions` is a count rather than a list, + deliberately — listing devices would publish every IP and user agent + an account has signed in from to anyone holding an operator token. + `locked` is computed from the lockout deadline rather than mirrored + from the column, because cryden clears a lockout by time passing, not + by writing a null. `PasswordHash` is on the struct this is built from + and is kept off the wire, with a test asserting that against the raw + body. - **MFA/passkey adoption stats** (new, no engine gap): computable via `AuditStore.SearchByType`/count against `EventTOTPEnabled`/ `EventWebAuthnRegistered` versus total user count — same pattern as the Argon2id migration progress endpoint in Tier 3. + **Built as `GET /v1/admin/security/mfa-adoption`, and it reports + events rather than users.** The spec above assumed a user count was + available to divide by; it is not. cryden's `TOTPStore` and + `WebAuthnCredentialStore` are per-user with no `Count` and no + `ListAll`, so "how many accounts have a factor" is a question the + engine cannot be asked — and answering it here would mean counting + rows in cryden's own tables. Every field is therefore named + `*_events`, and **no adoption percentage is derived anywhere**: a + ratio of events to `total_users` would look like coverage and move for + the wrong reasons (a user who enrols, loses a phone and disables + contributes one of each and is enrolled zero times over). - **Anomaly review/dismiss state** (new, this repo's own table): cryden's `AnomalyStore`/`AuditStore` record signals but have no concept of a human having reviewed one. A `reviewed_anomalies` table here (audit event ID, reviewer, status, timestamp) backs a review/dismiss workflow the console can drive; cryden's own audit history is never mutated to reflect this. + **Built as `GET /v1/admin/anomalies` + `PUT /v1/admin/anomalies/{eventID}`.** + Two decisions were settled by the human before it was written, and + both are load-bearing: + - **Dismiss is a status, not a delete, and the evidence stays.** The + table has `status IN ('unreviewed','confirmed','dismissed')` and no + DELETE anywhere; withdrawing a judgement stores `unreviewed` rather + than removing the row, so the record of who looked — and that they + changed their mind — survives. + - **The queue is keyed on the audit event id**, which is what a + console has in hand. Since cryden has no lookup by event id, the + existence check is carried by a foreign key from + `reviewed_anomalies.event_id` to `audit_events.id`, with the + resulting SQLSTATE `23503` mapped to a clean + `404 audit_event_not_found`. + - **Confirming takes no action on any account.** No lock, no session + revoke, no threshold change. This repo has no machinery that acts on + an account beyond what an operator does by hand, and putting one + behind a confirm button is exactly the automatic action `CLAUDE.md` + forbids. The queue is the two `signals`-carrying event types + (`anomaly_detected`, `credential_stuffing_detected`); widening it to + the failure events around them would make the audit table the queue. + +**Not built, and not part of this tier**: the widget's own serving +endpoint. The Stage 2 widget *configuration* exists, but nothing serves +an embeddable widget, so `allowed_origins` is still stored and +unenforced — the same gap `CURRENT-STATE.md` records for Tier 4. diff --git a/docs/development/PROGRESS.md b/docs/development/PROGRESS.md index 322ff4e..86e1804 100644 --- a/docs/development/PROGRESS.md +++ b/docs/development/PROGRESS.md @@ -970,8 +970,12 @@ and saying so is better than a comment implying it is. ### The read-only rule now has a named exception, and it needed a reading -`/v1/admin/settings/*` are the first writes under `/v1/admin`, and -`CLAUDE.md`'s hard rule is that the admin surface is read-only. The +`/v1/admin/settings/*` are writes under `/v1/admin`, and `CLAUDE.md`'s +hard rule is that the admin surface is read-only. (This entry originally +said they were the *first* writes there. They were not: Tier 3's +metadata `PUT`/`DELETE` had already written, so what needed a reading was +not whether the surface was read-only — it never was — but whether a +settings save is the kind of write the rule forbids.) The reading taken: the rule covers the AI **tools**, which cryden builds through interfaces carrying no method that can act, rather than every route under `/v1/admin`; and a settings save is precisely what @@ -1005,3 +1009,142 @@ change is a router edit plus a README line. - **The `-race` run takes over two minutes** for `httpapi` alone, so it is worth running as a separate command rather than appended to the plain suite, which is how this entry's verification was done. + +## 2026-09-16 — Tier 5 (the users admin surface) + +Branch `feat/tier5-users-admin-surface`, five commits: a docs correction +first, then the review store and its migration, the admin user lookup and +detail endpoints, the MFA adoption report, and the flagged-event review +queue. Docs last, as usual. + +Two design decisions were settled by the human before any of it was +written, and both are load-bearing rather than preferences: + +- **Dismiss is a status, not a delete.** The rule against the AI tools + acting automatically is what matters; an admin endpoint a human + triggers is a different thing, and Tier 3's metadata `PUT`/`DELETE` + and Tier 4's settings routes had already written here. So a review keys + on the audit event id — the thing the console actually shows — and + dismissing sets a field rather than removing evidence. Withdrawing a + judgement stores `unreviewed`: the row stays, and so does the record of + who looked. +- **Exact-email only, via `cryden.GetUser`.** Writing SQL against + cryden's own `users` table for a partial search would cross the + ownership boundary this project has been careful about everywhere + else. If partial search matters later it is its own deliberate + decision, not something bundled into this tier. + +### What the spec asked for versus what the engine allows + +`NEXT.md` specced MFA adoption as "`SearchByType`/count against +`EventTOTPEnabled`/`EventWebAuthnRegistered` versus total user count". +The counts exist; the thing they would be divided by does not. cryden's +`TOTPStore` and `WebAuthnCredentialStore` are both per-user — +`GetByUserID`, `ListByUser`, `Confirm`, `Delete` — with **no `Count` and +no `ListAll`**, so "how many accounts currently have a factor enrolled" +is a question the engine cannot be asked. Counting the rows in +`totp_secrets` from here would answer it exactly and would be this repo +writing SQL against the engine's schema, which is the boundary the +second decision above invokes. + +So the report gives what the engine does record system-wide and says so: +every field is named `*_events`, the DTO documents the missing store +methods by name, and **no adoption percentage is derived anywhere**. A +ratio of events to users would be a number that looks like coverage and +moves for the wrong reasons — enrol, lose a phone, disable is one +enrolment and one removal for zero net enrolment. Recovery codes are +excluded from the factor list for a related reason: they are a fallback +for an account that already has a factor, not a factor of their own, so +counting `recovery_codes_generated` as enrolment would report a +different thing than the label says. + +### The foreign key is doing real work + +The review table's `event_id` is `UUID PRIMARY KEY REFERENCES +audit_events(id) ON DELETE CASCADE`, and the FK is the whole reason Go +can refuse a review of an event that does not exist: cryden has no +lookup by event id, and `SearchByType` is the only way to read an event +back, so there is no Go-side check available. `audit_events.id` is a +`gen_random_uuid()` primary key, so an FK against it accepts every real +event and refuses every fabricated one, with SQLSTATE `23503` mapped to +`404 audit_event_not_found`. That is the repo's existing +SQLSTATE-by-code pattern from `aiprovider/query.go` (which matches +`42501`), reused rather than reinvented. + +`anomalyreview.MemoryStore` reproduces the FK via `RegisterEvents` +instead of accepting any id, because a double that accepted anything +would let a test assert a 404 production never produces — the inversion +Tier 4's never-run `CheckReadOnly` already taught. The tested branch is +the one that runs. + +### Two bugs the tests caught, both worth recording + +- **The `status=unreviewed` filter returned nothing.** The handler + compared against what the store returned, so an event with no row at + all — which *is* unreviewed — was filtered out. That is the one tab an + operator opens first. Fixed by comparing against the same default the + response reports. +- **`has_more` was computed from per-type saturation**, which claims + "there is more" when a type returned exactly its window but the queue + ends there. Replaced with "this page came back full, ask again" — the + honest direction, and exact in the other one: because the merged + window is a prefix of the queue rather than a sample of it, a page + short of `limit` really is the end. Measured on the *unfiltered* page, + so a status-filtered short page does not read as the end of the queue. + +A third thing was found by writing the test rather than by running it: +`GET /v1/admin/anomalies/{eventID}` is not a route — only `PUT` is. +A single flagged event is read as part of the queue, not on its own, +because the engine has no lookup by event id to serve one from. The test +asserting otherwise was wrong, not the router. + +### Verification — what was run, and what was not + +Run: `gofmt -l` clean, `go build ./...`, `go vet ./...`, and the full +`go test -count=1 ./...` — all ten packages green, `httpapi` in ~38s. +`openapi/spec.yaml` was parsed and checked twice: 33 paths, version 1.6, +and no `$ref` into a schema that does not exist. Its path list was also +diffed against `router.go`'s route table — every route this tier added +is in the spec, and the only routes in code but not in spec are the +pre-existing TOTP/WebAuthn/magic-link/OAuth ones, which were never in +it. + +**Not run, and this is the part worth being precise about:** + +- **`-race` was not run this session.** Tier 4's entries recorded it as + green; this tier added no concurrency, but that is reasoning rather + than evidence and the run is owed. +- **Migration `014_reviewed_anomalies` has never been applied to a + database**, and `anomalyreview.PostgresStore` has never executed a + single query against a real Postgres. `docker run ... postgres:16-alpine` + fails in this environment with `permission denied while trying to + connect to the docker API at unix:///var/run/docker.sock` — retried + with the sandbox disabled and it fails identically, so it is a real + environment restriction rather than a sandbox block. The consequence, + stated plainly: **the FK's `23503` mapping and the `pq.Array` binding + in `StatusesFor` are verified by reasoning and by the in-memory double + only.** Migrations `001`–`013` are in the same position. Everything + else on this surface is tested end to end on the in-memory store. + +### Noticed while working, not fixed + +- **`openapi/spec.yaml` is now 1.6** but its route list was never + complete: the TOTP, WebAuthn, magic-link, recovery-code and OAuth + paths from Tier 1 have no entries at all. This tier added its own four + and left that gap as it found it, since filling it is a Tier 1 + documentation pass rather than part of this work. +- **The README's endpoint list had drifted the same way** — digest, + support, config-tuning and settings routes were missing from it. Those + *were* added, because the admin block was being edited anyway and a + route list that omits half the admin surface while gaining new lines + is worse than either extreme. +- **`security_handlers.go` has a lint diagnostic that predates this + tier** — "if statement can be modernized using max" on the `remaining < + 0` floor in `HashMigration`. Left alone as unrelated churn in existing + code; noted so it is a decision rather than an oversight. +- **`idAssigningAuditStore` is now doing real work for two surfaces.** + It exists because cryden's *memory* `AuditStore.Record` never sets + `ID` while its Postgres store gets one from `gen_random_uuid()`, and + every endpoint on this tier keys on the event id. It is a test double + for a gap in cryden's own double, kept here rather than patched into + the engine. diff --git a/openapi/spec.yaml b/openapi/spec.yaml index b7ec70b..c9bd1df 100644 --- a/openapi/spec.yaml +++ b/openapi/spec.yaml @@ -1,7 +1,7 @@ openapi: 3.0.3 info: title: CrydenSync API - version: "1.5" + version: "1.6" description: > A self-hosted HTTP wrapper around the CrydenSync auth engine. Every response follows one of two envelope shapes: {"data": ...} @@ -59,6 +59,32 @@ info: deployment setting that is not a cryden one: all three paths answer 404 not_configured unless SETTINGS_ENCRYPTION_KEY is set, since without that key there is nowhere safe to put a credential. + + 1.6 is additive: the admin user surface (GET /admin/users, GET + /admin/users/{userID}), the second-factor adoption report (GET + /admin/security/mfa-adoption) and the flagged-event review queue + (GET /admin/anomalies, PUT /admin/anomalies/{eventID}). No existing + path, field or status code changed. + + Two of those are worth reading before the paths. GET /admin/users + searches by exact email only, case-sensitively, and says so in its + response: the engine stores addresses exactly as typed and compares + them with SQL's `=`, so partial or case-insensitive search is not a + gap left for later but a query this API declines to write against + the engine's own table. GET /admin/security/mfa-adoption reports + enrolment and removal EVENTS rather than users and derives no + adoption percentage, for the same reason: the engine has no count of + accounts with a factor enrolled, so a ratio built from events would + be a number that looks like coverage and is not. + + PUT /admin/anomalies/{eventID} is a write, and is the third kind of + write on this API's admin surface, after 1.3's metadata and 1.5's + settings. It records an operator's judgement about an event the + engine flagged and takes no action on any account — there is no + machinery in this API that can, which is what keeps it on the right + side of the read-only rule. Nothing deletes a review: dismissing is + a status, and withdrawing a judgement stores "unreviewed" rather + than removing the row. servers: - url: http://localhost:8080/v1 description: Local dev @@ -266,6 +292,204 @@ components: rejected. Read from the engine and this repo's own claim set, so it stays true as either changes. + MFAAdoption: + type: object + description: > + Second-factor enrolment and removal as the engine's own audit + events, all-time and over a window, against the user total. + Read-only. + + This is deliberately not "N of M users have MFA", and the reason + is worth stating: the engine's TOTP and WebAuthn credential + stores are per-user with no count and no list-all, so it cannot + be asked how many accounts have a factor enrolled. Counting the + rows directly would answer it and would mean querying the + engine's own tables. So no percentage is derived anywhere here — + a ratio of events to users would be a number that looks like + coverage and moves for the wrong reasons. + properties: + total_users: { type: integer } + window_days: { type: integer, description: 1-365, default 7. } + factors: + type: array + description: One entry per factor, in a stable order. + items: + type: object + properties: + factor: { type: string, enum: [totp, passkey] } + enrolled_events: { type: integer } + removed_events: { type: integer } + enrolled_events_in_window: { type: integer } + removed_events_in_window: { type: integer } + description: > + Every field counts events, not users. A user who turns TOTP + on, loses their phone and turns it off contributes one + enrolment and one removal and is enrolled zero times over, + so enrolled_events is not an adoption figure. Recovery + codes are not a factor here: they are a fallback for an + account that already has one. + + AdminUser: + type: object + description: > + One account, as the console sees it. PasswordHash is on the store + struct this is built from and is deliberately not on this schema + — a struct tag is not a guarantee, so the handler's own test + asserts against the raw body for any hash-shaped field. + properties: + id: { type: string, format: uuid } + email: { type: string } + created_at: { type: string, format: date-time } + updated_at: { type: string, format: date-time } + failed_attempts: { type: integer } + locked: + type: boolean + description: > + Computed from the lockout deadline, not mirrored from the + stored column. The engine clears a lockout by time passing + rather than by writing a null, so a row can carry a + locked_until that is already in the past — exactly the case + somebody just waited out, and the one an operator most needs + reported correctly. + locked_until: + type: string + format: date-time + nullable: true + + AdminUserList: + type: object + properties: + users: + type: array + items: { $ref: '#/components/schemas/AdminUser' } + total: { type: integer } + limit: { type: integer } + offset: { type: integer } + match: + type: string + enum: [exact_email, browse] + description: > + How this page was produced. "exact_email" means the list is + the single account whose email equals the query byte for + byte; "browse" means no query was given. It is in the + response because the surprising half is otherwise invisible: + the email match is exact AND case-sensitive, so + "Alice@example.com" does not find an account created as + "alice@example.com". A console can label the result and + explain a zero-result search rather than leaving an operator + to conclude the account is gone. + query: { type: string, description: The search that produced this page, empty when browsing. } + + AdminUserDetail: + type: object + properties: + user: { $ref: '#/components/schemas/AdminUser' } + active_sessions: + type: integer + description: > + A count, not a list, deliberately. The engine's session store + returns only live sessions, so the count is honest; listing + them would publish every IP and user agent an account has + signed in from to anyone holding an operator token. + recent_activity: + type: array + description: The account's own audit history, newest first, capped at 20. + items: { $ref: '#/components/schemas/AuditEvent' } + + AuditEvent: + type: object + description: > + One event as the engine recorded it. Metadata is passed through + unchanged: its keys are the engine's ("signals", + "distinct_accounts", "from"/"to"), they differ per event type, + and a host that renamed them would be inventing a second + vocabulary for the engine's own records. + properties: + id: { type: string, format: uuid } + type: { type: string } + ip: { type: string } + metadata: + type: object + additionalProperties: { type: string } + created_at: { type: string, format: date-time } + + AnomalyReview: + type: object + description: > + What a human decided about one flagged event. A row in this API's + own table, keyed on the audit event id — the engine's event reads + exactly the same before and after a review, because a review is + never a write to the engine's audit history. + + Nothing here deletes. Dismissing sets a status; withdrawing a + judgement sets it back to "unreviewed", which is stored rather + than represented by a missing row, so the record that somebody + looked and who they were survives the change of mind. + properties: + status: { type: string, enum: [unreviewed, confirmed, dismissed] } + note: { type: string, description: Always present, empty when there is none. } + reviewer_id: + type: string + format: uuid + description: > + Omitted for an event nobody has called. The reviewer is the + authenticated operator, taken from the verified token and + never from the body. + updated_at: + type: string + format: date-time + description: Omitted for an event nobody has called, so a zero timestamp is never read as a very old decision. + + Anomaly: + type: object + description: > + One queue row: the engine's event plus what a human said about it. + The review is nested rather than flattened so the two halves stay + visibly separate — a reader can tell which parts the engine wrote + and which parts this deployment did. + properties: + id: { type: string, format: uuid } + type: { type: string, enum: [anomaly_detected, credential_stuffing_detected] } + user_id: { type: string, format: uuid } + ip: { type: string } + metadata: + type: object + additionalProperties: { type: string } + created_at: { type: string, format: date-time } + review: { $ref: '#/components/schemas/AnomalyReview' } + + AnomalyList: + type: object + properties: + anomalies: + type: array + items: { $ref: '#/components/schemas/Anomaly' } + limit: { type: integer } + offset: { type: integer } + status: { type: string, description: The filter that was applied, omitted when the whole queue was returned. } + has_more: + type: boolean + description: > + This page came back full, so there MAY be more — a client + should ask again rather than assume the queue ends here. It + is deliberately not "there IS more": deciding that exactly + would mean knowing the total, and a page that comes back + short of limit IS the end, because the merged window is an + exact prefix of the queue rather than a sample of it. + + AnomalyReviewInput: + type: object + required: [status] + properties: + status: + type: string + enum: [unreviewed, confirmed, dismissed] + description: > + Required. A review whose decision is missing is not a review. + "unreviewed" is how a judgement is withdrawn; there is no + DELETE. + note: { type: string, maxLength: 500, description: Optional. } + WebhookDelivery: type: object description: > @@ -1046,6 +1270,51 @@ paths: application/json: schema: { $ref: '#/components/schemas/ErrorResponse' } + /admin/security/mfa-adoption: + get: + summary: Second-factor enrolment as the engine's own audit events + description: > + Admin only — an operator's token. For each factor (totp, + passkey): how many enrolment and removal events the engine has + recorded, all-time and over a window, alongside the user total. + Read-only by construction — it calls count methods and records + nothing. + + It counts events, not users, and every field is named + accordingly. See the MFAAdoption schema for why no adoption + percentage is derived: the engine cannot be asked how many + accounts currently have a factor enrolled, so a ratio would be a + number that looks like coverage and is not. The pair that is + worth reading is enrolments against removals — a factor whose + removals keep pace is churning — and the windowed pair is what + says whether that is happening now. + security: [{ bearerAuth: [] }] + parameters: + - name: window_days + in: query + required: false + schema: { type: integer, minimum: 1, maximum: 365, default: 7 } + description: > + The reporting window. Out of range or non-numeric is a 400, + not a silent clamp. + responses: + '200': + description: The report + content: + application/json: + schema: + type: object + properties: + data: { $ref: '#/components/schemas/MFAAdoption' } + '400': { $ref: '#/components/responses/BadRequest' } + '401': { $ref: '#/components/responses/Unauthorized' } + '403': { $ref: '#/components/responses/Forbidden' } + '404': + description: not_configured — the router was built without the stores this report reads. + content: + application/json: + schema: { $ref: '#/components/schemas/ErrorResponse' } + /admin/oauth/health: get: summary: Reachability of each OAuth provider this API knows about @@ -1075,6 +1344,113 @@ paths: '401': { $ref: '#/components/responses/Unauthorized' } '403': { $ref: '#/components/responses/Forbidden' } + /admin/users: + get: + summary: Find an account by exact email, or browse accounts + description: > + Admin only — an operator's token. Two modes, chosen by whether + `q` is present: an exact-email lookup, or a page of every + account newest first. The response's `match` says which one + produced the page. + + There is no partial or case-insensitive search, and that is a + decision rather than a gap. Partial search would mean SQL + against the engine's own users table, which is the boundary this + API keeps everywhere else: the engine owns that table, and a + query written against it here would be a second, silent + definition of what a user is. If partial search is wanted later + it is its own deliberate piece of work. + + Read-only: there is no lock, unlock, password reset or delete on + this surface. An operator who needs one has the engine's own + admin path, not an HTTP endpoint this API invented. + security: [{ bearerAuth: [] }] + parameters: + - name: q + in: query + required: false + schema: { type: string } + description: > + An email to match exactly, byte for byte and case-sensitive, + because that is how the engine stores and compares + addresses. Absent or empty means browse. A search that finds + nothing is an empty 200, never a 404 — answering 404 would + make a console render "error" for the most ordinary outcome + a search has. + - name: limit + in: query + required: false + schema: { type: integer, minimum: 1, maximum: 500, default: 50 } + - name: offset + in: query + required: false + schema: { type: integer, minimum: 0, maximum: 10000, default: 0 } + responses: + '200': + description: > + The page. In exact-email mode limit and offset are echoed + rather than obeyed — an exact match is one account or none, + so there is nothing to page — and they are not rejected + either, since a console that keeps its page size fixed while + switching modes is doing nothing wrong. + content: + application/json: + schema: + type: object + properties: + data: { $ref: '#/components/schemas/AdminUserList' } + '400': { $ref: '#/components/responses/BadRequest' } + '401': { $ref: '#/components/responses/Unauthorized' } + '403': { $ref: '#/components/responses/Forbidden' } + '404': + description: not_configured — the router was built without the user store. + content: + application/json: + schema: { $ref: '#/components/schemas/ErrorResponse' } + + /admin/users/{userID}: + get: + summary: One account's state, session count and recent history + description: > + Admin only — an operator's token. Reports the account, a count of + its live sessions and its most recent audit events. + + Read-only, like everything else on this surface that is not an + explicit operator write: it reports a lockout and cannot clear + one. `locked` is computed from the deadline rather than mirrored + from the stored column, because the engine clears a lockout by + time passing rather than by writing a null — see the AdminUser + schema. Sessions are counted rather than listed, deliberately; + the SupportHandlers report makes the same choice for the same + reason. + security: [{ bearerAuth: [] }] + parameters: + - name: userID + in: path + required: true + schema: { type: string, format: uuid } + responses: + '200': + description: The account + content: + application/json: + schema: + type: object + properties: + data: { $ref: '#/components/schemas/AdminUserDetail' } + '401': { $ref: '#/components/responses/Unauthorized' } + '403': { $ref: '#/components/responses/Forbidden' } + '404': + description: > + not_found for an unknown or malformed userID — a path segment + that cannot be a user id is answered the same as one that + simply is not a user, since a malformed id handed to Postgres + is a driver error that would surface as a 500; or + not_configured when the stores are absent. + content: + application/json: + schema: { $ref: '#/components/schemas/ErrorResponse' } + /admin/users/{userID}/metadata: get: summary: Every metadata key set on one user @@ -1231,6 +1607,144 @@ paths: application/json: schema: { $ref: '#/components/schemas/ErrorResponse' } + /admin/anomalies: + get: + summary: The flagged-event review queue + description: > + Admin only — an operator's token. The two event types the engine + writes when a login looks wrong rather than merely failing + (anomaly_detected and credential_stuffing_detected), merged + newest first, each with the review recorded against it. + + The queue is deliberately just those two. Widening it to the + failure events around them (login_failed, token_reuse_detected) + would not make a bigger queue, it would make the audit table the + queue — and a review surface that asks about everything asks + about nothing. Both of these carry a "signals" key naming what + tripped, which is what makes them reviewable: an operator can + read the event and form a judgement. + + Read-only: it reads events and reviews and writes nothing. + security: [{ bearerAuth: [] }] + parameters: + - name: status + in: query + required: false + schema: { type: string, enum: [unreviewed, confirmed, dismissed] } + description: > + Narrows the page to one decision. An event with no review row + matches "unreviewed" — which is the tab an operator opens + first, so the two are treated as the same thing everywhere. + - name: limit + in: query + required: false + schema: { type: integer, minimum: 1, maximum: 500, default: 50 } + - name: offset + in: query + required: false + schema: { type: integer, minimum: 0, maximum: 10000, default: 0 } + description: > + Bounded by limit + offset <= 500, refused rather than clamped + beyond that. The engine's per-type search takes a limit and + no offset, so the merged window is fetched to limit+offset + from each type; clamping would silently return a page from + further up the queue than the caller asked for, which on a + review queue means showing events they have already dealt + with. + responses: + '200': + description: The page + content: + application/json: + schema: + type: object + properties: + data: { $ref: '#/components/schemas/AnomalyList' } + '400': + description: > + A status that is not one of the three, or a limit + offset + beyond what the merge can fetch. + content: + application/json: + schema: { $ref: '#/components/schemas/ErrorResponse' } + '401': { $ref: '#/components/responses/Unauthorized' } + '403': { $ref: '#/components/responses/Forbidden' } + '404': + description: not_configured — the router was built without the audit or review store. + content: + application/json: + schema: { $ref: '#/components/schemas/ErrorResponse' } + + /admin/anomalies/{eventID}: + put: + summary: Record what an operator decided about a flagged event + description: > + Admin only — an operator's token. Records a judgement against one + audit event, keyed on the event's own id, which is what a console + has in hand and what the engine's record is filed under. + + This write takes no action on any account, and that is the point + rather than an omission. Confirming an event records that a + person judged it real: no account is locked, no session is + revoked, no threshold is tuned. There is no machinery in this API + that acts on an account beyond what an operator does by hand, and + putting one behind a "confirm" button is exactly the automatic + action the read-only rule forbids. This endpoint is a console + action or it does not happen. + + The judgement is stored in this API's own table, never written + into the engine's audit history — the event reads exactly the + same before and after. There is no DELETE: "actually, never mind" + is status=unreviewed, which keeps the row and its attribution, + because the record that somebody looked and thought better of it + is worth more than the tidier table a delete would leave. + + The response is the review alone, not the whole queue row. + Re-reading the event would need a lookup by event id and the + engine has none — events are fetched by type — so a save returns + the decision rather than pretending to return the row it belongs + to, and a client refreshes the list it already has. + security: [{ bearerAuth: [] }] + parameters: + - name: eventID + in: path + required: true + schema: { type: string, format: uuid } + requestBody: + required: true + content: + application/json: + schema: { $ref: '#/components/schemas/AnomalyReviewInput' } + responses: + '200': + description: The recorded review + content: + application/json: + schema: + type: object + properties: + data: { $ref: '#/components/schemas/AnomalyReview' } + '400': + description: > + invalid_review_status for a status that is not one of the + three, invalid_review_note for a note over 500 characters, or + a body with no status field. + content: + application/json: + schema: { $ref: '#/components/schemas/ErrorResponse' } + '401': { $ref: '#/components/responses/Unauthorized' } + '403': { $ref: '#/components/responses/Forbidden' } + '404': + description: > + audit_event_not_found — there is no audit event with that id, + so there is nothing to review. Also answered for a path + segment that cannot be an event id at all, since a malformed + uuid handed to Postgres is a driver error that would surface + as a 500; and not_configured when the review store is absent. + content: + application/json: + schema: { $ref: '#/components/schemas/ErrorResponse' } + /admin/webhooks/deliveries: get: summary: The webhook delivery log, newest first