Feat/tier2 config and oauth health - #4
Merged
Merged
Conversation
Tier 2's config-only half: the engine features that need no new routes and no new stores, exposed through the same env-var surface every other optional setting already uses. Anomaly detection and credential-stuffing detection get one switch (ANOMALY_DETECTION) rather than two, because they share one store as their on/off switch in cryden — the same login_attempts history read two ways. Both stay report-only: a flagged attempt records an audit event, no login is ever blocked or delayed by them. The threshold structs copy the engine's own defaults across first and only then apply the knobs an env var actually names. That ordering is load-bearing rather than stylistic: cryden reads every field of a non-zero thresholds value, so assembling the struct from just the env vars that happened to be set would silently switch off every check left out instead of defaulting it. Restating the two rate-limit bounds is the same kind of decision — cryden fills those in only for the in-process limiter it builds itself, and with REDIS_URL set it is this repo that calls security.NewRedisRateLimiter, whose constructor rejects a zero bound. Leaving them at zero would have made REDIS_URL on its own a startup failure. REDIS_URL swaps cryden's in-process limiter for the shared one so several replicas count against a single window; it deliberately does not touch the coarse per-IP edge limiter in httpapi/ratelimit.go, which stays in-process either way. Nothing dials Redis at startup — the client is injected already constructed like every store, so an unreachable Redis surfaces as failing-closed rate-limit checks on the three entry points that use it, which is cryden's own documented trade-off for the shared limiter. config_test.go pins all of it: the defaults really are the engine's and not a re-typed copy, overrides leave untouched knobs alone, an explicit 0 survives as a real setting, and a malformed value is a startup error rather than one that was quietly ignored. README and .env.example carry the new vars, in .env.example's case with the reason the numeric ones default to the engine's numbers.
cryden.ListNamedSessions attaches a human-readable label to each session
plus the parsed device and location it was built from, so a "your
devices" screen has something to print instead of a raw session ID.
This is a deliberate change to an endpoint's response shape, not a
field quietly added: id, ip, user_agent and created_at keep their names
and types, and label, device and location are new. openapi/spec.yaml
carries it (version bumped to 1.1, with the session schema and the path
description saying so) and the README says the same thing in prose.
Labels are computed on read from the IP and User-Agent the session
already holds, so there is no new table, no migration and no backfill —
every session ever recorded gets a label the moment this is called.
No geolocator is wired, deliberately. Config.Geolocator is where the
location half of a label comes from, and cryden ships no implementation
of it because every implementation calls somebody else's internet
service; that is a deployment's decision to make, not this repo's to
make on its behalf. The consequence is documented rather than hidden:
labels are device-only ("Chrome on macOS") and the location object is
present-but-empty until someone wires one. `label` itself is never
empty either way — the engine falls back to "Unknown device".
The new test is the first in this repo that runs an endpoint end to
end: it builds a real engine on cryden's own in-memory stores, signs up
and logs in through the engine, then calls the handler behind
RequireAuth with the real access token. No Postgres, no network, but
every line below the HTTP layer is the production one. It pins the
label, the parsed device, the empty-without-geolocator location, and —
in a second test that reads the raw JSON — that no token-shaped field
(token_hash, family_id, refresh_token) came along with the richer
types. The DB-backed smoketest also stops accepting "200 with anything
in it": it now requires the session's id and a non-empty label.
Tier 1's tests could only reach error mapping because there was no
store to build on. There is now, which is squarely why this tier's
verification goes further than the last one's.
The first admin endpoint in this repo, and therefore the first real use of Tier 0.5's RequireAdmin. Per provider: whether this deployment is configured for it at all, and whether its authorize endpoint answers. Four states rather than a bool. "not_configured" and "configured but unreachable" are the same nothing to a login but very different things to an operator, and a 5xx from an endpoint that is otherwise up is worth telling apart from a connect timeout. An unconfigured provider is reported without any request being made: it cannot log anyone in, and not probing it keeps this endpoint from reaching out to providers the deployment never opted into. Probes are deliberately not OAuth flows — no client ID, no state, no redirect, nothing that could mint a session. A bare GET to an authorize endpoint gets a 4xx (the provider's "missing client_id" complaint), which still proves it is up and serving, so 4xx is "ok" and only 5xx is "degraded". They run concurrently with a 5s timeout each, so six providers cost one timeout rather than six, and the provider list lives next to the switch in oauth_handlers.go so the two cannot drift apart without a reviewer noticing. The tests are the strongest part of this commit and they run with no database at all. The verdicts are covered against real httptest servers (400 -> ok, 503 -> degraded, a closed port -> unreachable, an unparseable URL -> unreachable before any dial, an absent provider -> not_configured), including that the probe carries no query string. Then the route itself is exercised through the real router: no token -> 401, an ordinary user's token -> 403 not_operator, an operator's token -> 200. That last test builds its operator with the same ClaimsProvider mechanism main.go uses, on the in-memory engine, and constructs the router with a nil *sql.DB on purpose — nothing on that path touches the database, so needing one would have been a lie about what is tested. README gains the operator section it never had (Tier 0.5 shipped cmd/grant-operator without documenting it here): what an operator token is, how the first one is granted, and why the grant takes effect at the next login rather than immediately. openapi/spec.yaml gains the path and its row schema.
A tier is not finished until the docs say so, and this is that step for Tier 2. NEXT.md annotates each sub-item with what was actually built and the decision behind it, CURRENT-STATE.md gains the tier's section, and PROGRESS.md gains this session's entry: the three commits, what the verification actually covered and what it did not, the assumptions made (none blocking), and three things noticed but left alone — the spec still predating Tier 1, the edge limiter still being in-process with REDIS_URL set, and the smoketest having no way to cover the admin surface. PROGRESS.md also records the environment problem this session ran under, since it is exactly the kind of thing CODEX.md's verification rule exists for: both sandboxes in this container are broken — command execution fails with "bwrap: setting up uid map: Permission denied" and apply_patch cannot touch workspace paths at all — so every command ran escalated and every edit was applied through apply_patch against a /tmp hard link to the same inode. The files are the same as they would have been; the next session should know the workaround if it hits the same wall.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.