diff --git a/API.md b/API.md index 22a3c9d..005b44d 100644 --- a/API.md +++ b/API.md @@ -43,6 +43,15 @@ An unknown reference in a **path** is `404`; an unknown one in a **query filter* is `400`, because the collection itself was fine and it was the filter that was wrong. +**A reference never changes.** A shop's slug, a camera's id and a customer's +number are immutable in the database, so it is safe to store one — in a saved +URL, a config file or a scheduled report. The **display name** beside it +(`"TeNext Chennai"`, `"Front door"`) is free to change and should be; do not key +on it. + +The uuid is still returned everywhere and still works. Use it if you want a key +you never have to think about; use the reference when a person will read it. + --- ## 1. Signing in diff --git a/CLAUDE.md b/CLAUDE.md index 6fd3b4e..29cd8fa 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -2047,6 +2047,53 @@ the engine's own diagnostic dashboard. human name. The prop carrying it is `customerRef`, not `ref` — React reserves that name, so it would never have reached the component. +### Why the uuid stays, when the slug would do + +Asked directly: `site_id` is 36 characters, why not a small number? + +The honest answer is that **the length was never the problem — needing it was**, +and that is already fixed: `?site=chennai` and `/api/sites/chennai/check` work, +and the shop PC has always identified itself by slug (`agent.json` holds +`"site_id": "chennai"`, never the uuid). The uuid in a *response* is the stable +key for a client that wants to store one. + +Two reasons not to replace it, and one reason that is NOT among them: + +- **Enumeration.** `/api/sites/3/check` makes any future tenancy hole walkable + by counting; a uuid makes it require a leak first. Every handler scopes by the + session's client today, so this is defence in depth rather than the control — + but this database holds biometric templates, and defence in depth is the point + of a second layer. +- **The payoff is now zero.** Eight tables carry a foreign key to `sites(id)`, + against a live database, to make a field shorter that a client is already told + not to use. +- **NOT because ids must be minted offline.** Sites, visitors and visits are all + created server-side with a database in hand. That argument holds for the + agent's `event_id` — which is derived precisely so it needs no coordination — + and it does not hold here; claiming it would be a defence of the status quo + rather than a reason for it. + +What DID need fixing is that the references were only stable by accident. +Migration 013 makes `clients.slug`, `sites.slug`, `site_cameras.camera_id` and +`visitors.number` immutable in the database, because 012 turned them from +descriptive columns into identifiers other systems store: + +- `clients.slug` is an MQTT topic segment the broker ACL is written against. + Rename one and that tenant's whole estate is silently refused by the broker, + with no way to tell the agents. +- `sites.slug` is what a shop PC calls itself. A rename orphans the PC from the + shop it is standing in. +- `site_cameras.camera_id` lands in `visits.camera_id`, which is text and not a + foreign key. A rename orphans every visit already attributed to the old name: + the footfall is still there and no longer joins to a camera. This was + half-enforced in `handleUpdateCamera` and nowhere else — the shape of a rule + that holds until somebody adds a second write path. + +A trigger rather than a CHECK, because a CHECK cannot see the old row and the +rule is about the transition. **The display name is deliberately NOT frozen** — +"TeNext Chennai", "Front door" — it is what a person reads, nothing keys on it, +and a system that cannot fix a typo in a shop's name has confused the two. + ### Three uuids on one arrival, three different answers Asked of the row the feed actually returns, and they do not get the same reply: diff --git a/server/internal/store/api_immutable_live_test.go b/server/internal/store/api_immutable_live_test.go new file mode 100644 index 0000000..86ae74f --- /dev/null +++ b/server/internal/store/api_immutable_live_test.go @@ -0,0 +1,57 @@ +package store + +import ( + "context" + "strings" + "testing" +) + +// A reference clients are told to use must not be able to change underneath +// them. +// +// 012 turned three descriptive columns into IDENTIFIERS other people store: in +// agent.json on a shop counter, in a saved URL, in a scheduled report. All +// three were already treated as stable and none of it was enforced - the +// camera case was half-enforced in one handler and nowhere else, which is the +// shape of a rule that holds until somebody adds a second write path. +// +// Only a real database can test this: the rule is a trigger, and an in-memory +// fake would happily agree with any implementation. +func TestLiveAReferenceCannotBeRenamed(t *testing.T) { + st := liveStore(t) + ctx := context.Background() + site := seedAgentSite(t, st, "frozen-"+stamp()) + + if _, err := st.pool.Exec(ctx, ` + INSERT INTO site_cameras (client_id, site_id, camera_id, label, host) + VALUES ($1::uuid, $2::uuid, 'Office1', 'Front door', '10.0.0.5')`, + site.ClientID, site.SiteID); err != nil { + t.Fatal(err) + } + + for _, c := range []struct{ what, sql string }{ + {"a shop's slug", `UPDATE sites SET slug = 'moved' WHERE id = $1::uuid`}, + {"a camera's id", `UPDATE site_cameras SET camera_id = 'Office2' WHERE site_id = $1::uuid`}, + } { + _, err := st.pool.Exec(ctx, c.sql, site.SiteID) + if err == nil { + t.Fatalf("%s was renamed - it is a reference other systems store", c.what) + } + if !strings.Contains(err.Error(), "cannot be changed") { + t.Fatalf("%s: unexpected error %v", c.what, err) + } + } + + // The DISPLAY name is not frozen and must not be. It is what a person + // reads, nothing keys on it, and a system that cannot fix a typo in a + // shop's name has confused the two. + if _, err := st.pool.Exec(ctx, + `UPDATE sites SET name = 'Renamed Shop' WHERE id = $1::uuid`, site.SiteID); err != nil { + t.Fatalf("a shop's display name must stay editable: %v", err) + } + if _, err := st.pool.Exec(ctx, + `UPDATE site_cameras SET label = 'Back door' WHERE site_id = $1::uuid`, + site.SiteID); err != nil { + t.Fatalf("a camera's label must stay editable: %v", err) + } +} diff --git a/server/migrations/013_references_are_immutable.sql b/server/migrations/013_references_are_immutable.sql new file mode 100644 index 0000000..b1ef802 --- /dev/null +++ b/server/migrations/013_references_are_immutable.sql @@ -0,0 +1,97 @@ +-- The references clients are now told to use must not be able to change. +-- +-- 012 made `chennai`, `Office1` and `V-42` first-class: every route that takes +-- an id takes one of these instead, and the documentation tells a client to +-- prefer them. That turns three columns which were merely descriptive into +-- IDENTIFIERS other people store - in an agent's config file on a shop counter, +-- in a saved URL, in a report somebody scheduled. +-- +-- All three were already treated as stable, and none of it was enforced: +-- +-- * `clients.slug` appears in MQTT topics as bv/./... and the +-- broker ACL is written against it. Renaming one silently stops that +-- tenant's estate from being able to publish, and the agents cannot be told +-- - they would simply be refused by the broker. +-- * `sites.slug` is what a shop PC calls itself: agent.json holds +-- "site_id": "chennai". A rename orphans the PC from the shop it is +-- standing in. +-- * `site_cameras.camera_id` is what lands in `visits.camera_id`, which is a +-- text column and not a foreign key. Renaming it orphans every visit +-- already attributed to the old name - the footfall is still there and no +-- longer joins to a camera. +-- +-- The camera case was half-enforced in one handler (`handleUpdateCamera` nils +-- CameraID before saving) and nowhere else, which is the shape of a rule that +-- holds until somebody adds a second write path. This is the backstop, in the +-- one place every write has to go through. +-- +-- Deliberately a trigger and not a CHECK: a CHECK cannot see the old row, and +-- the rule is about the transition, not the value. +-- +-- Note what this does NOT freeze. `name` - "TeNext Chennai", "Front door" - is +-- free to change and always should be: it is what a person reads, it is not what +-- anything keys on, and conflating the two is how a system ends up unable to fix +-- a typo in a shop's name. + +BEGIN; + +CREATE OR REPLACE FUNCTION reference_is_immutable() RETURNS trigger +LANGUAGE plpgsql AS $$ +BEGIN + RAISE EXCEPTION + '% is a public reference and cannot be changed (% -> %); ' + 'create a new row instead, or change the display name', + TG_ARGV[0], OLD.slug, NEW.slug + USING ERRCODE = 'check_violation'; +END; +$$; + +-- camera_id lives in its own function only because the column is named +-- differently; splitting it keeps the message honest about which value moved. +CREATE OR REPLACE FUNCTION camera_reference_is_immutable() RETURNS trigger +LANGUAGE plpgsql AS $$ +BEGIN + RAISE EXCEPTION + 'camera_id is a public reference and cannot be changed (% -> %); ' + 'every visit already recorded names the old one. Change the label instead', + OLD.camera_id, NEW.camera_id + USING ERRCODE = 'check_violation'; +END; +$$; + +DROP TRIGGER IF EXISTS clients_slug_immutable ON clients; +CREATE TRIGGER clients_slug_immutable + BEFORE UPDATE OF slug ON clients + FOR EACH ROW WHEN (OLD.slug IS DISTINCT FROM NEW.slug) + EXECUTE FUNCTION reference_is_immutable('clients.slug'); + +DROP TRIGGER IF EXISTS sites_slug_immutable ON sites; +CREATE TRIGGER sites_slug_immutable + BEFORE UPDATE OF slug ON sites + FOR EACH ROW WHEN (OLD.slug IS DISTINCT FROM NEW.slug) + EXECUTE FUNCTION reference_is_immutable('sites.slug'); + +DROP TRIGGER IF EXISTS site_cameras_id_immutable ON site_cameras; +CREATE TRIGGER site_cameras_id_immutable + BEFORE UPDATE OF camera_id ON site_cameras + FOR EACH ROW WHEN (OLD.camera_id IS DISTINCT FROM NEW.camera_id) + EXECUTE FUNCTION camera_reference_is_immutable(); + +-- A visitor's number is assigned once from the tenant's counter and read back +-- as V-42. Nothing writes it after the insert; this says so. +DROP TRIGGER IF EXISTS visitors_number_immutable ON visitors; +CREATE OR REPLACE FUNCTION visitor_number_is_immutable() RETURNS trigger +LANGUAGE plpgsql AS $$ +BEGIN + RAISE EXCEPTION + 'visitors.number is a public reference and cannot be changed (% -> %)', + OLD.number, NEW.number + USING ERRCODE = 'check_violation'; +END; +$$; +CREATE TRIGGER visitors_number_immutable + BEFORE UPDATE OF number ON visitors + FOR EACH ROW WHEN (OLD.number IS DISTINCT FROM NEW.number) + EXECUTE FUNCTION visitor_number_is_immutable(); + +COMMIT;