From 712d114dc53bd93b18d251fe2d86d9508b0ce889 Mon Sep 17 00:00:00 2001 From: Paul Trowbridge Date: Fri, 18 Sep 2026 11:14:03 -0400 Subject: [PATCH 1/8] Scope what an account can see and change to its territory Territory filtering was deferred from v1, so every account saw and could write every row. With the sales team about to adjust their own territories that is the thing standing in the way, and it is also why a rep would wait fifteen seconds to load 2.7M rows to work on a few thousand. The list lives on pf.app_user.territory with is_admin beside it, and col_meta.is_territory marks which column of a source the values belong to -- flagged rather than named in code, so a second source can be divided by something other than a sales rep. Fail closed: buildTerritoryClause returns FALSE for an empty list or an unflagged source. An account nobody configured sees nothing, rather than everything because a column was left null. Built from the session, never the request. That is what separates it from `scope`, which the browser sends and should: a filter the user chose belongs in the payload, a permission cannot come from the thing it restrains. It is ANDed on last, where nothing in the request can undo it. Enforced on /data (the cursor and the count behind X-Row-Count), on /agg before the GROUP BY since the territory column need not be in the grain, on every operation through sliceUnits, and on the value completion endpoint -- which reads the source table, so without it a dropdown enumerates every customer and rep in the business to someone shown none of their rows. Undo is gated by owner rather than territory: it removes an entry's rows wholesale, so half-undoing one would leave a state nothing describes. Recode refuses to set the territory column unless you are an admin, since moving a row between territories is reassignment, not forecasting. ./pf.sh gains set-territory, set-admin and orphan-territory. The last lists territory values no account owns -- work under one is invisible to everybody but an admin, which a typo causes easily and nothing in the app reveals. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 44 +++++++++++++++++ lib/auth.js | 24 ++++++++- lib/sql_generator.js | 24 ++++++++- pf.sh | 105 +++++++++++++++++++++++++++++++++++++++- routes/auth.js | 2 + routes/log.js | 12 +++++ routes/operations.js | 78 +++++++++++++++++++++++++---- routes/sources.js | 15 +++++- setup_sql/01_schema.sql | 12 +++++ setup_sql/02_auth.sql | 12 +++++ 10 files changed, 313 insertions(+), 15 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 5dd76a6..c77a920 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -353,6 +353,50 @@ effects call the API immediately. `pf_user: 'admin'`, which any client could have set to anything. The audit log now names the account that made the change. +## Territory scoping + +An account sees and changes only its own territory. The list lives on +`pf.app_user.territory` (jsonb array) with `is_admin` for the accounts that see +everything, and `col_meta.is_territory` marks which column of a given source +the values belong to — one per source, flagged rather than named in code so a +second source can be divided by something other than a sales rep. + +**Fail closed.** No territory and not an admin means no rows. +`buildTerritoryClause()` returns `FALSE`, not `TRUE`, for an empty list or an +unflagged source: an account somebody forgot to configure sees nothing instead +of the whole book. + +**Built from the session, never the request.** This is the difference between +it and `scope`, which the browser sends and which is right to send, being a +filter the user chose. A permission cannot come from the thing it restrains, so +the territory predicate is ANDed on last, in `sliceUnits()` for writes and per +route for reads, where nothing in the payload can remove it. + +Enforced at: + +- `/data` — clause on the cursor *and* on the count behind `X-Row-Count` +- `/agg` — a `{{territory_clause}}` token applied **before** the GROUP BY, since + the territory column need not be part of the grain and may not survive it +- every operation, through `sliceUnits()` +- `/sources/:id/values/:col` — completion reads the *source* table, which no + scope has touched, so without it a dropdown enumerates the whole business +- `DELETE /log/:logid` — by owner, not territory: undo removes an entry's rows + wholesale, and half-undoing one would leave a state nothing describes. Your + own entries, or an admin's override. +- recode's `set` — a scoped account cannot set the territory column at all. + Moving a row between territories is reassignment, not forecasting, and it + would vanish from the view that would have shown what happened. + +**The change log shows an entry's full impact**, not the reader's share. The +totals stamped on `pf.log` are company-wide, so an admin's version-wide scale +reads the same in every account — deliberate, and labelled, rather than +re-aggregating per territory. + +Managed with `./pf.sh set-territory | set-admin | orphan-territory`. +`orphan-territory` lists values present in the data that no account owns; work +under one is invisible to everyone but an admin, which a typo causes easily and +nothing inside the app reveals. + ## Light / dark mode Theme state lives in `ui/src/theme.jsx` — a React context (`ThemeContext`) with a `ThemeProvider` that wraps the app in `main.jsx`. diff --git a/lib/auth.js b/lib/auth.js index 1279f2e..1d0bf93 100644 --- a/lib/auth.js +++ b/lib/auth.js @@ -50,7 +50,29 @@ function sessionUser(req) { return req.session?.user?.username || null; } -module.exports = { hashPassword, verifyPassword, requireAuth, sessionUser, SCRYPT }; +// What this account may see and change, read from the session for the same +// reason the username is: the browser must not be able to widen it. +// +// Returns { admin: true } for an account that sees everything, or +// { admin: false, values: [...] } for a scoped one. An empty list is a real +// answer meaning "nothing", not a missing one meaning "everything" -- an +// account created without a territory sees no rows until it is granted some. +function sessionTerritory(req) { + const u = req.session?.user; + if (!u) return { admin: false, values: [] }; + if (u.is_admin) return { admin: true, values: null }; + return { admin: false, values: Array.isArray(u.territory) ? u.territory : [] }; +} + +function requireAdmin(req, res, next) { + if (req.session?.user?.is_admin) return next(); + res.status(403).json({ error: 'Administrator access required' }); +} + +module.exports = { + hashPassword, verifyPassword, requireAuth, requireAdmin, + sessionUser, sessionTerritory, SCRYPT, +}; // CLI: `node lib/auth.js hash` reads a password on stdin and prints its hash, // so ./pf.sh can create users without the plaintext touching argv or psql. diff --git a/lib/sql_generator.js b/lib/sql_generator.js index 0fcc7ed..901fd0f 100644 --- a/lib/sql_generator.js +++ b/lib/sql_generator.js @@ -8,7 +8,7 @@ // Tokens baked in at generation time: column names, source schema.table // Tokens substituted at request time: {{fc_table}}, {{where_clause}}, {{exclude_clause}}, // {{version_id}}, {{logid}}, {{pf_user}}, {{note}}, -// {{label}}, {{bucket}}, {{tag}}, +// {{label}}, {{bucket}}, {{tag}}, {{territory_clause}}, // {{params}}, {{slice}}, {{date_from}}, {{date_to}}, // {{value_incr}}, {{units_incr}}, {{set_clause}}, {{scale_factor}} @@ -308,6 +308,7 @@ SELECT FROM {{fc_table}} t LEFT JOIN pf.log l ON l.id = t.pf_logid${VERSION_JOIN} +WHERE {{territory_clause}} GROUP BY ${grain.groupCols('t.').join('\n ,')} ,${LABEL_GROUP_COLS.join('\n ,')}`.trim(); @@ -658,6 +659,25 @@ function buildScopeClause(scope, dimCols, versionId) { return parts.join('\nAND '); } +// The territory predicate: what this account may see and change. +// +// Server-side by construction. Today's `scope` is sent by the browser, which is +// right for a filter the user chose and would be fatal for a permission -- so +// this one is built from the session and ANDed on last, where nothing in the +// request can remove it. +// +// FALSE, not TRUE, for an account with no territory. The whole point is that a +// missing grant means no rows: an empty list that fell through to TRUE would +// hand the entire book to the first account somebody forgot to configure. +function buildTerritoryClause(territory, territoryCol, alias = '') { + if (!territory || territory.admin) return ''; + if (!territoryCol) return 'FALSE'; + const vals = (territory.values || []).filter(v => v != null && v !== ''); + if (vals.length === 0) return 'FALSE'; + const pfx = alias ? `${alias}.` : ''; + return `${pfx}"${territoryCol}" IN (${vals.map(v => `'${esc(String(v))}'`).join(', ')})`; +} + // build a WHERE clause spanning several slices — an OR of AND-groups. // A union of slices cannot be flattened into one IN list per column: slices // {Region:East, State:NY} and {Region:West, State:CA} would become @@ -754,6 +774,6 @@ function esc(val) { return String(val).replace(/'/g, "''"); } -module.exports = { generateSQL, grainOf, COMPUTED_SLICE_COLS, buildScopeClause, +module.exports = { generateSQL, grainOf, COMPUTED_SLICE_COLS, buildScopeClause, buildTerritoryClause, SEGMENT_EXPR, BUCKET_EXPR, NOTE_EXPR, LABEL_GROUP_COLS, VERSION_JOIN, ADJUSTMENT_SEGMENT, ADJUSTMENT_BUCKET, UNLABELED_LOAD, dateGroupsOf, dimPeriodMapOf, dimPeriodJoins, applyTokens, buildWhere, buildWhereAny, buildExcludeClause, buildExcludePredicate, buildSetClause, buildFilterClause, esc }; diff --git a/pf.sh b/pf.sh index 0b68683..59fdf3b 100755 --- a/pf.sh +++ b/pf.sh @@ -5,6 +5,7 @@ set -euo pipefail # pf.sh — Pivot Forecast management script # Usage: ./pf.sh [deploy|start|stop|restart|status|logs|db-setup|config] # ./pf.sh [add-user|passwd|list-users|disable-user|enable-user] +# ./pf.sh [set-territory|set-admin|orphan-territory] # ./pf.sh (interactive menu) # --------------------------------------------------------------------------- @@ -360,11 +361,102 @@ cmd_list_users() { require_env; load_env echo; bold "Accounts" run_psql -c " - SELECT username, display_name, is_active, + SELECT username, display_name, is_active, is_admin, + coalesce(jsonb_array_length(territory), 0) AS territory_values, to_char(last_login_at, 'YYYY-MM-DD HH24:MI') AS last_login FROM pf.app_user ORDER BY username" } +# Territory is what an account may see and change, as a list of values in the +# source's is_territory column. No territory and not an admin means no rows -- +# so a new account is blind until this is run, which is the intended direction +# to fail in. +# +# Values are given comma-separated and have to match the column exactly, since +# that is what the SQL compares. set-territory with no values clears it. +cmd_set_territory() { + require_env; load_env + local username="${1:-}"; shift || true + [[ -z "$username" ]] && { read -rp " Username: " username; } + [[ -z "$username" ]] && die "Username is required." + local values="${*:-}" + [[ -z "$values" ]] && { read -rp " Territory values (comma separated, blank to clear): " values; } + + local json="null" + if [[ -n "$values" ]]; then + json=$(python3 - "$values" <<'PYEOF' +import json, sys +vals = [v.strip() for v in sys.argv[1].split(',') if v.strip()] +print(json.dumps(vals)) +PYEOF +) + fi + + run_psql -v ON_ERROR_STOP=1 -tAc " + WITH upd AS ( + UPDATE pf.app_user SET territory = $(if [[ "$json" == "null" ]]; then echo NULL; else echo "'$(sql_lit "$json")'::jsonb"; fi) + WHERE lower(username) = lower('$(sql_lit "$username")') + RETURNING username + ) + SELECT count(*) FROM upd" | grep -q '^1$' \ + || die "No such account: $username" + ok "Territory updated for $username" + run_psql -c "SELECT username, is_admin, territory FROM pf.app_user WHERE lower(username) = lower('$(sql_lit "$username")')" +} + +# An admin sees and changes everything, and is the only account that can recode +# the territory column or undo someone else's entry. +cmd_set_admin() { + require_env; load_env + local username="${1:-}" flag="${2:-true}" + [[ -z "$username" ]] && { read -rp " Username: " username; } + [[ -z "$username" ]] && die "Username is required." + [[ "$flag" != "true" && "$flag" != "false" ]] && die "Second argument must be true or false." + + run_psql -v ON_ERROR_STOP=1 -tAc " + WITH upd AS ( + UPDATE pf.app_user SET is_admin = $flag + WHERE lower(username) = lower('$(sql_lit "$username")') + RETURNING username + ) + SELECT count(*) FROM upd" | grep -q '^1$' \ + || die "No such account: $username" + ok "$username is_admin = $flag" +} + +# Territory values present in the data that belong to no account. Work under one +# is invisible to everybody but an admin, which is easy to cause by a typo and +# impossible to notice from inside the app. +cmd_orphan_territory() { + require_env; load_env + local source_id="${1:-}" + [[ -z "$source_id" ]] && { read -rp " Source id: " source_id; } + [[ -z "$source_id" ]] && die "Source id is required." + + # The column and table are data, so the query is built in two steps rather + # than one clever one: read the names, then run the listing. + local meta col schema tname + meta=$(run_psql -tAF'|' -c " + SELECT m.cname, x.schema, x.tname + FROM pf.col_meta m JOIN pf.source x ON x.id = m.source_id + WHERE m.source_id = $source_id AND m.is_territory") + [[ -z "$meta" ]] && die "Source $source_id has no column marked is_territory." + IFS='|' read -r col schema tname <<< "$meta" + + echo; bold "Territory values in $schema.$tname with no account" + run_psql -c " + SELECT DISTINCT s.\"$col\" AS unassigned + FROM \"$schema\".\"$tname\" s + WHERE TRUE + AND s.\"$col\" IS NOT NULL + AND s.\"$col\"::text NOT IN ( + SELECT jsonb_array_elements_text(territory) + FROM pf.app_user + WHERE territory IS NOT NULL + ) + ORDER BY 1" +} + # Deactivating leaves the row (and its history) in place, and drops any live # session so the account loses access immediately rather than at cookie expiry. cmd_disable_user() { @@ -489,6 +581,9 @@ interactive_menu() { echo " 11) add-user create a login account" echo " 12) passwd change an account password" echo " 13) list-users show accounts" + echo " 16) set-territory grant an account its territory values" + echo " 17) set-admin make an account an administrator" + echo " 18) orphan-territory territory values no account owns" echo " 14) disable-user deactivate an account and sign it out" echo " 15) enable-user reactivate an account" echo " q) quit" @@ -508,6 +603,9 @@ interactive_menu() { 11|add-user) cmd_add_user ;; 12|passwd) cmd_passwd ;; 13|list-users) cmd_list_users ;; + 16|set-territory) cmd_set_territory ;; + 17|set-admin) cmd_set_admin ;; + 18|orphan-territory) cmd_orphan_territory ;; 14|disable-user) cmd_disable_user ;; 15|enable-user) cmd_enable_user ;; q|Q|quit|exit) echo "Bye."; exit 0 ;; @@ -532,8 +630,11 @@ case "${1:-}" in add-user) cmd_add_user ;; passwd) cmd_passwd ;; list-users) cmd_list_users ;; + set-territory) shift; cmd_set_territory "$@" ;; + set-admin) shift; cmd_set_admin "$@" ;; + orphan-territory) shift; cmd_orphan_territory "$@" ;; disable-user) cmd_disable_user "${2:-}" ;; enable-user) cmd_enable_user "${2:-}" ;; "") interactive_menu ;; - *) die "Unknown command: $1. Valid: deploy start stop restart status logs db-setup config install-service uninstall-service add-user passwd list-users disable-user enable-user" ;; + *) die "Unknown command: $1. Valid: deploy start stop restart status logs db-setup config install-service uninstall-service add-user passwd list-users disable-user enable-user set-territory set-admin orphan-territory" ;; esac diff --git a/routes/auth.js b/routes/auth.js index b286bca..1c64b51 100644 --- a/routes/auth.js +++ b/routes/auth.js @@ -70,6 +70,8 @@ module.exports = function(pool) { id: user.id, username: user.username, display_name: user.display_name, + is_admin: !!user.is_admin, + territory: Array.isArray(user.territory) ? user.territory : [], }; pool.query(`UPDATE pf.app_user SET last_login_at = now() WHERE id = $1`, [user.id]) .catch(e => console.error('last_login_at update failed', e)); diff --git a/routes/log.js b/routes/log.js index 4400a9e..f021d5c 100644 --- a/routes/log.js +++ b/routes/log.js @@ -1,5 +1,6 @@ const express = require('express'); const { grainOf } = require('../lib/sql_generator'); +const { sessionUser } = require('../lib/auth'); const { fcTable } = require('../lib/utils'); module.exports = function(pool) { @@ -112,6 +113,17 @@ module.exports = function(pool) { if (!logResult.rows.length) return res.status(404).json({ error: 'Log entry not found' }); const log = logResult.rows[0]; if (log.status === 'closed') return res.status(403).json({ error: 'Version is closed' }); + + // Undo deletes rows wholesale by logid, so it cannot be territory + // filtered the way a read or a write can -- half-undoing an entry + // would leave the version in a state nothing describes. Ownership + // instead: your own entries, or an admin's override. Territory alone + // would not do it anyway, since two accounts can share one. + if (!req.session?.user?.is_admin && log.pf_user !== sessionUser(req)) { + return res.status(403).json({ + error: `That entry was made by ${log.pf_user || 'someone else'} — only they or an administrator can undo it` + }); + } const table = fcTable(log.tname, log.version_id); // In grain mode the client's table is indexed on pf_gkey, so undo has to diff --git a/routes/operations.js b/routes/operations.js index 603866c..bbea6c2 100644 --- a/routes/operations.js +++ b/routes/operations.js @@ -1,9 +1,9 @@ const express = require('express'); const { tableFromArrays, tableToIPC } = require('apache-arrow'); -const { applyTokens, buildWhere, buildWhereAny, COMPUTED_SLICE_COLS, buildScopeClause, buildExcludeClause, buildExcludePredicate, buildSetClause, dateGroupsOf, dimPeriodMapOf, esc, +const { applyTokens, buildWhere, buildWhereAny, COMPUTED_SLICE_COLS, buildScopeClause, buildTerritoryClause, buildExcludeClause, buildExcludePredicate, buildSetClause, dateGroupsOf, dimPeriodMapOf, esc, SEGMENT_EXPR, BUCKET_EXPR, NOTE_EXPR, VERSION_JOIN, ADJUSTMENT_SEGMENT, ADJUSTMENT_BUCKET } = require('../lib/sql_generator'); -const { sessionUser } = require('../lib/auth'); +const { sessionUser, sessionTerritory } = require('../lib/auth'); const { fcTable } = require('../lib/utils'); module.exports = function(pool) { @@ -47,10 +47,18 @@ module.exports = function(pool) { // The scope is ANDed onto every unit rather than folded into the slices: it // applies to all of them equally, and under apply_mode 'each' folding it in // would repeat the same predicate in every statement for no gain. - function sliceUnits(slices, ctx, applyMode, scope) { + function sliceUnits(slices, ctx, applyMode, scope, req) { const vid = ctx.version.id; const scl = buildScopeClause(scope, ctx.filterCols, vid); - const and = (w) => (scl ? (w === 'TRUE' ? scl : `${w}\nAND ${scl}`) : w); + const terr = req ? territoryOf(req, ctx) : ''; + const and = (w) => { + let out = w; + for (const extra of [scl, terr]) { + if (!extra) continue; + out = (out === 'TRUE' || !out) ? extra : `${out}\nAND ${extra}`; + } + return out; + }; return applyMode === 'each' ? slices.map(sl => ({ slices: [sl], where: and(buildWhere(sl, ctx.filterCols, vid)) })) : [{ slices, where: and(buildWhereAny(slices, ctx.filterCols, vid)) }]; @@ -127,6 +135,23 @@ module.exports = function(pool) { } } + // Moving a row between territories is reassignment, not forecasting, so a + // scoped account cannot set the territory column -- otherwise a rep could + // recode work into their own book, or quietly out of it, and the row would + // be gone from the view that would have shown what happened. + // + // Reads and writes are already scoped, so the *source* rows are safely the + // account's own; this is only about the destination. + function assertMayRecodeTerritory(req, ctx, set, res) { + if (!ctx.territoryCol) return true; + if (req.session?.user?.is_admin) return true; + if (!set || set[ctx.territoryCol] === undefined) return true; + res.status(403).json({ + error: `Only an administrator can recode ${ctx.territoryCol} — that moves rows between territories` + }); + return false; + } + // echo back what the caller asked for, for the audit log function pickIntent(body) { const keys = ['mode', 'target_basis', 'value_incr', 'units_incr', 'value_pct', 'units_pct', 'pct', @@ -337,10 +362,25 @@ module.exports = function(pool) { filterCols: [...dimCols, ...dateCols], valueCol, unitsCol, + territoryCol: colMeta.find(c => c.is_territory)?.cname || null, sql: sqlResult.rows[0].sql }; } + // Every read and every write goes through this, so a scoped account cannot + // reach a row outside its territory by any route. ANDed on last, after the + // slice and the client's own scope, where nothing in the request can undo + // it. + function territoryOf(req, ctx) { + return buildTerritoryClause(sessionTerritory(req), ctx.territoryCol); + } + + function andTerritory(where, req, ctx) { + const t = territoryOf(req, ctx); + if (!t) return where; + return (!where || where === 'TRUE') ? t : `${where}\nAND ${t}`; + } + function guardOpen(version, res) { if (version.status === 'closed') { res.status(403).json({ error: 'Version is closed' }); @@ -363,7 +403,19 @@ module.exports = function(pool) { } const tbl = fcTable(verResult.rows[0].tname, versionId); - const { rows: [{ count }] } = await pool.query(`SELECT COUNT(*) FROM ${tbl}`); + // /data does not go through getContext, so it resolves the territory + // column itself. The count is scoped too, or the progress bar + // promises rows this account will never be sent. + const terrCol = (await pool.query( + `SELECT cname FROM pf.col_meta WHERE source_id = $1 AND is_territory LIMIT 1`, + [verResult.rows[0].source_id] + )).rows[0]?.cname || null; + const territory = sessionTerritory(req); + const terrBare = buildTerritoryClause(territory, terrCol); + const terrAlias = buildTerritoryClause(territory, terrCol, 't'); + const terrWhere = terrBare ? `WHERE ${terrBare}` : ''; + + const { rows: [{ count }] } = await pool.query(`SELECT COUNT(*) FROM ${tbl} ${terrWhere}`); const rowCount = parseInt(count); res.setHeader('Content-Type', 'application/vnd.apache.arrow.stream'); @@ -382,6 +434,7 @@ module.exports = function(pool) { FROM ${tbl} t LEFT JOIN pf.log l ON l.id = t.pf_logid${VERSION_JOIN} + ${terrAlias ? `WHERE ${terrAlias}` : ''} `); // Accumulate into column arrays (not row objects) to avoid allocating one JS @@ -425,7 +478,13 @@ module.exports = function(pool) { router.get('/versions/:id/agg', async (req, res) => { try { const ctx = await getContext(parseInt(req.params.id), 'get_agg'); - const sql = applyTokens(ctx.sql, { fc_table: ctx.table }); + // Before the GROUP BY, not after: the territory column need not be + // part of the grain, so an aggregated row may not carry it at all. + const sql = applyTokens(ctx.sql, { + fc_table: ctx.table, + territory_clause: + buildTerritoryClause(sessionTerritory(req), ctx.territoryCol, 't') || 'TRUE', + }); const { rows } = await runSQL(sql); res.setHeader('Content-Type', 'application/vnd.apache.arrow.stream'); @@ -666,7 +725,7 @@ module.exports = function(pool) { // sum() OVER () distribute the increment across the whole pool. // 'each' runs the same statement once per slice, so every slice // reaches the target on its own and gets its own log entry. - const units = sliceUnits(slices, ctx, applyMode, req.body.scope); + const units = sliceUnits(slices, ctx, applyMode, req.body.scope, req); const client = await pool.connect(); let committed = false; @@ -749,10 +808,11 @@ module.exports = function(pool) { const ctx = await getContext(parseInt(req.params.id), 'recode'); if (!guardOpen(ctx.version, res)) return; assertSelective(slices, ctx); + if (!assertMayRecodeTerritory(req, ctx, set, res)) return; const excludeClause = buildExcludeClause(ctx.version.exclude_iters); const setClause = buildSetClause(ctx.dimCols, set); - const units = sliceUnits(slices, ctx, apply_mode === 'each' ? 'each' : 'prorate', req.body.scope); + const units = sliceUnits(slices, ctx, apply_mode === 'each' ? 'each' : 'prorate', req.body.scope, req); const client = await pool.connect(); let committed = false; @@ -845,7 +905,7 @@ module.exports = function(pool) { [cname, `${alias}."${periodCol}"`]) ); const setClause = buildSetClause(ctx.dimCols, set, { derivedExprs, alias: 's' }); - const units = sliceUnits(slices, ctx, apply_mode === 'each' ? 'each' : 'prorate', req.body.scope); + const units = sliceUnits(slices, ctx, apply_mode === 'each' ? 'each' : 'prorate', req.body.scope, req); const client = await pool.connect(); let committed = false; diff --git a/routes/sources.js b/routes/sources.js index 6073756..9500104 100644 --- a/routes/sources.js +++ b/routes/sources.js @@ -1,5 +1,6 @@ const express = require('express'); -const { generateSQL } = require('../lib/sql_generator'); +const { generateSQL, buildTerritoryClause } = require('../lib/sql_generator'); +const { sessionTerritory } = require('../lib/auth'); const { sessionUser } = require('../lib/auth'); module.exports = function(pool) { @@ -235,6 +236,18 @@ module.exports = function(pool) { const params = []; let filter = `WHERE "${col}" IS NOT NULL`; + + // Completion reads the *source* table, which no territory scope has + // touched -- so without this a scoped account could enumerate every + // customer, part and rep in the business from a dropdown, having + // been shown none of their rows. + const terrRow = (await pool.query( + `SELECT cname FROM pf.col_meta WHERE source_id = $1 AND is_territory LIMIT 1`, + [req.params.id] + )).rows[0]; + const terrClause = buildTerritoryClause(sessionTerritory(req), terrRow?.cname || null); + if (terrClause) filter += ` AND ${terrClause}`; + if (q) { params.push(`%${q}%`); filter += ` AND "${col}"::text ILIKE $${params.length}`; diff --git a/setup_sql/01_schema.sql b/setup_sql/01_schema.sql index 071d0d7..3cb6f48 100644 --- a/setup_sql/01_schema.sql +++ b/setup_sql/01_schema.sql @@ -139,6 +139,18 @@ ALTER TABLE pf.log ADD COLUMN IF NOT EXISTS value_total double precision; ALTER TABLE pf.log ADD COLUMN IF NOT EXISTS units_total double precision; ALTER TABLE pf.log ADD COLUMN IF NOT EXISTS measure_cols jsonb; +-- The column an account's territory is expressed in -- the rep, the region, +-- whatever this source divides ownership by. Exactly one per source. +-- +-- Flagged here rather than named in code because it is source-specific, the +-- same way the grain is: a second source should not need a code change to be +-- scoped by something other than a sales rep. +-- +-- It does two jobs. The server scopes every read and write to the values on the +-- account, and recode refuses to *set* this column unless the account is an +-- admin -- moving a row between territories is reassignment, not forecasting. +ALTER TABLE pf.col_meta ADD COLUMN IF NOT EXISTS is_territory boolean NOT NULL DEFAULT false; + -- Master data for a dim_group: one row per key value, with its sibling columns. -- -- The source is transactional and often a view over all history, so deriving a diff --git a/setup_sql/02_auth.sql b/setup_sql/02_auth.sql index fb74460..f8041a0 100644 --- a/setup_sql/02_auth.sql +++ b/setup_sql/02_auth.sql @@ -24,3 +24,15 @@ CREATE TABLE IF NOT EXISTS pf.session ( ); CREATE INDEX IF NOT EXISTS session_expire_idx ON pf.session (expire); + +-- What an account may see and change. +-- +-- territory is the list of values allowed in the source's territory column +-- (col_meta.is_territory). It is a filter the server applies to every read and +-- every write; it is never sent by the client, which could remove it. +-- +-- Fail closed: no territory and not an admin means no rows. An account created +-- without one sees nothing until someone grants it, rather than seeing the whole +-- book because a column was left null. +ALTER TABLE pf.app_user ADD COLUMN IF NOT EXISTS territory jsonb; +ALTER TABLE pf.app_user ADD COLUMN IF NOT EXISTS is_admin boolean NOT NULL DEFAULT false; From e60dd3ad961401a751ea4af5c6bbfa254096a125 Mon Sep 17 00:00:00 2001 From: Paul Trowbridge Date: Fri, 18 Sep 2026 11:38:55 -0400 Subject: [PATCH 2/8] Put the territory column in the Setup editor The flag was enforced everywhere and settable nowhere but psql, so the one piece of configuration a second account depends on was invisible. A radio rather than a checkbox, because exactly one column per source can be the territory -- the control should say so rather than leaving it to an error on save. Clicking the chosen one again clears it, which a radio has no other way to express. The save still refuses two, since the UI is not the only caller, and two would mean whichever a .find() reached first -- the trap is_key already fell into. Restricted to dimension columns: a territory is something rows are divided by, and scoping on a date or a measure is not a thing to offer. Co-Authored-By: Claude Opus 5 (1M context) --- routes/sources.js | 16 ++++++++++++++-- ui/src/views/Setup.jsx | 20 ++++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/routes/sources.js b/routes/sources.js index 9500104..67eefcd 100644 --- a/routes/sources.js +++ b/routes/sources.js @@ -87,13 +87,23 @@ module.exports = function(pool) { if (!Array.isArray(cols)) { return res.status(400).json({ error: 'body must be an array' }); } + // Exactly one per source: the scope is a single IN list against a single + // column, and two flagged would silently mean whichever one a .find() + // reached first -- the trap is_key already fell into (see CLAUDE.md). + const territoryCols = cols.filter(c => c.is_territory).map(c => c.cname); + if (territoryCols.length > 1) { + return res.status(400).json({ + error: `Only one column can be the territory. Flagged: ${territoryCols.join(', ')}` + }); + } + const client = await pool.connect(); try { await client.query('BEGIN'); for (const col of cols) { await client.query(` - INSERT INTO pf.col_meta (source_id, cname, label, role, is_key, dim_group, dim_period_col, in_grain, opos) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9) + INSERT INTO pf.col_meta (source_id, cname, label, role, is_key, dim_group, dim_period_col, in_grain, is_territory, opos) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) ON CONFLICT (source_id, cname) DO UPDATE SET label = EXCLUDED.label, role = EXCLUDED.role, @@ -101,6 +111,7 @@ module.exports = function(pool) { dim_group = EXCLUDED.dim_group, dim_period_col = EXCLUDED.dim_period_col, in_grain = EXCLUDED.in_grain, + is_territory = EXCLUDED.is_territory, opos = EXCLUDED.opos `, [ sourceId, @@ -111,6 +122,7 @@ module.exports = function(pool) { col.dim_group || null, col.dim_period_col || null, col.in_grain || false, + col.is_territory || false, col.opos || null ]); } diff --git a/ui/src/views/Setup.jsx b/ui/src/views/Setup.jsx index 09f1ed1..8b137ac 100644 --- a/ui/src/views/Setup.jsx +++ b/ui/src/views/Setup.jsx @@ -354,6 +354,7 @@ export default function Setup({ refreshSources }) { role key grain + territory group period col label @@ -390,6 +391,25 @@ export default function Setup({ refreshSources }) { className="cursor-pointer disabled:opacity-20" /> + {/* Radio, not a checkbox: exactly one column per source, + and the shape of the control should say so rather than + leaving it to a save-time error. */} + + {}} + onClick={() => setEditedCols(prev => { + // clicking the chosen one again clears it, since a + // radio otherwise has no way back to "no territory" + const already = !!prev[i].is_territory + return prev.map((c, x) => ({ ...c, is_territory: !already && x === i })) + })} + disabled={col.role !== 'dimension'} + className="cursor-pointer disabled:opacity-20" + /> + Date: Fri, 18 Sep 2026 11:47:02 -0400 Subject: [PATCH 3/8] Fix the territory commands' success message and menu order They called ok(), which this script does not have -- the helper is success() -- so set-territory ended on "ok: command not found" after having worked. The three new entries also sat between 13 and 14 in the menu, having been appended where the list-users case was rather than at the end. Co-Authored-By: Claude Opus 5 (1M context) --- pf.sh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/pf.sh b/pf.sh index 59fdf3b..6983403 100755 --- a/pf.sh +++ b/pf.sh @@ -400,7 +400,7 @@ PYEOF ) SELECT count(*) FROM upd" | grep -q '^1$' \ || die "No such account: $username" - ok "Territory updated for $username" + success "Territory updated for $username" run_psql -c "SELECT username, is_admin, territory FROM pf.app_user WHERE lower(username) = lower('$(sql_lit "$username")')" } @@ -421,7 +421,7 @@ cmd_set_admin() { ) SELECT count(*) FROM upd" | grep -q '^1$' \ || die "No such account: $username" - ok "$username is_admin = $flag" + success "$username is_admin = $flag" } # Territory values present in the data that belong to no account. Work under one @@ -581,11 +581,11 @@ interactive_menu() { echo " 11) add-user create a login account" echo " 12) passwd change an account password" echo " 13) list-users show accounts" - echo " 16) set-territory grant an account its territory values" - echo " 17) set-admin make an account an administrator" - echo " 18) orphan-territory territory values no account owns" echo " 14) disable-user deactivate an account and sign it out" echo " 15) enable-user reactivate an account" + echo " 16) set-territory grant an account the territory values it may see" + echo " 17) set-admin make an account an administrator" + echo " 18) orphan-territory territory values no account owns" echo " q) quit" echo read -rp " Choice: " choice @@ -603,11 +603,11 @@ interactive_menu() { 11|add-user) cmd_add_user ;; 12|passwd) cmd_passwd ;; 13|list-users) cmd_list_users ;; + 14|disable-user) cmd_disable_user ;; + 15|enable-user) cmd_enable_user ;; 16|set-territory) cmd_set_territory ;; 17|set-admin) cmd_set_admin ;; 18|orphan-territory) cmd_orphan_territory ;; - 14|disable-user) cmd_disable_user ;; - 15|enable-user) cmd_enable_user ;; q|Q|quit|exit) echo "Bye."; exit 0 ;; *) warn "Unknown option: $choice" ;; esac From 68e2d81c8c05328c6e34b949239c58c429d3c1e3 Mon Sep 17 00:00:00 2001 From: Paul Trowbridge Date: Fri, 18 Sep 2026 11:48:57 -0400 Subject: [PATCH 4/8] Let an empty result be an answer, not an abort worker.table([], { index: 'pf_gkey' }) aborts: an empty array carries no columns, so the index names one that does not exist and the page dies with "Specified index `pf_gkey` does not exist in dataset" instead of saying it found nothing. Nothing is a legitimate answer -- an empty version, and now a territory with no rows in it, which is what surfaced this. The empty table is built without an index and the page says which of the two it is. Co-Authored-By: Claude Opus 5 (1M context) --- ui/src/views/Forecast.jsx | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/ui/src/views/Forecast.jsx b/ui/src/views/Forecast.jsx index 3618c8f..0510d42 100644 --- a/ui/src/views/Forecast.jsx +++ b/ui/src/views/Forecast.jsx @@ -777,8 +777,18 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio if (stale) await stale.delete() } catch {} - const opts = { name: tableName, index: indexCol } + // An empty result gets no index. `[]` carries no columns, so naming one + // aborts the worker outright -- "Specified index `pf_gkey` does not exist + // in dataset" -- and the page dies rather than saying it found nothing. + // Nothing is a legitimate answer: an empty version, or a territory with no + // rows in it. + const opts = rowCount > 0 + ? { name: tableName, index: indexCol } + : { name: tableName } tableRef.current = await (rowCount > 0 ? worker.table(buffer, opts) : worker.table([], opts)) + if (rowCount === 0) { + flash('No rows to show — the version is empty, or none of it is in your territory', 'error') + } if (myId !== initIdRef.current) { try { await tableRef.current.delete() } catch {} From 9d08638a9224b84704d62309b0605a98c0b1e69a Mon Sep 17 00:00:00 2001 From: Paul Trowbridge Date: Fri, 18 Sep 2026 11:52:00 -0400 Subject: [PATCH 5/8] Read territory at login, and survive having none The login query names its columns, and territory and is_admin were not among them -- so every session carried an empty list, every account scoped to FALSE, and the forecast page came back with nothing however the grant was set. The CLI had written it correctly; nothing read it. The empty case then aborted twice over. First on the index, fixed already. Then on the layout: an empty table has no schema, so restoring a saved config asks for the dtype of a column that is not there and the worker dies -- "Could not get dtype for column `sseas_e`". With no rows there is nothing to lay out, so nothing is restored, and the saved layout waits in localStorage for rows to come back. Co-Authored-By: Claude Opus 5 (1M context) --- routes/auth.js | 3 ++- ui/src/views/Forecast.jsx | 11 +++++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/routes/auth.js b/routes/auth.js index 1c64b51..34ac0cf 100644 --- a/routes/auth.js +++ b/routes/auth.js @@ -46,7 +46,8 @@ module.exports = function(pool) { try { const result = await pool.query( - `SELECT id, username, display_name, pass_hash, is_active + `SELECT id, username, display_name, pass_hash, is_active, + is_admin, territory FROM pf.app_user WHERE lower(username) = lower($1)`, [username] ); diff --git a/ui/src/views/Forecast.jsx b/ui/src/views/Forecast.jsx index 0510d42..affd063 100644 --- a/ui/src/views/Forecast.jsx +++ b/ui/src/views/Forecast.jsx @@ -806,8 +806,15 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio // restore last-used layout or build default // Strip cfg.table — table is already loaded by reference above; a stale name // in a saved config would cause Perspective to fail the name lookup. - const saved = localStorage.getItem(LAYOUT_KEY(vid)) - if (saved) { + // + // An empty table has no schema at all, so any config naming any column + // aborts the worker -- "Could not get dtype for column `sseas_e`". There + // is nothing to lay out, so nothing is restored; the saved layout stays in + // localStorage and comes back when there are rows again. + const saved = rowCount > 0 ? localStorage.getItem(LAYOUT_KEY(vid)) : null + if (rowCount === 0) { + await viewer.restore({ settings: false, plugin_config: { edit_mode: 'SELECT_REGION' } }) + } else if (saved) { const { table: _t, ...rest } = cleanLayout(JSON.parse(saved), validCols) const cfg = { ...rest, plugin_config: { ...(rest.plugin_config || {}), edit_mode: 'SELECT_REGION' } } await viewer.restore(cfg) From 61268f2a7a5d4b1942a1c683d053dcac40974b42 Mon Sep 17 00:00:00 2001 From: Paul Trowbridge Date: Fri, 18 Sep 2026 12:03:06 -0400 Subject: [PATCH 6/8] Say who made each change, and which ones you can undo The change log showed what happened and never who did it, which stops being a detail the moment more than one person is in the version. The Undo button greys out on entries belonging to someone else, with the reason on hover. The server already refused them; the button offered the click anyway and answered with a 403, which reads as a fault rather than a rule. Co-Authored-By: Claude Opus 5 (1M context) --- ui/src/views/Forecast.jsx | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/ui/src/views/Forecast.jsx b/ui/src/views/Forecast.jsx index affd063..fe05b10 100644 --- a/ui/src/views/Forecast.jsx +++ b/ui/src/views/Forecast.jsx @@ -1,5 +1,6 @@ import { useState, useEffect, useRef } from 'react' import useTheme from '../theme.jsx' +import useAuth from '../auth.jsx' import OperationPanel from '../components/OperationPanel.jsx' import BridgeView from '../components/BridgeView.jsx' @@ -54,6 +55,11 @@ const DEAD_ORDER_EXPRS = ['pf_bucket_ord', 'pf_segment_ord', 'Bucket', 'Segment' export default function Forecast({ sources = [], sourceId, versions = [], versionId, refreshSources }) { const { dark } = useTheme() + const { user } = useAuth() + // Undo removes an entry's rows wholesale, so the server allows it only to the + // account that made it, or an admin. Mirrored here to say so before the click + // rather than after the 403. + const canUndo = (entry) => !!user && (user.is_admin || entry.pf_user === user.username) const [loading, setLoading] = useState(false) const [largeDataset, setLargeDataset] = useState(false) // The pivot's own filter, refreshed whenever the ledger recomputes. A ref @@ -1902,6 +1908,7 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio Slice Tag Note + By Value Rows @@ -1935,6 +1942,9 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio )} /> + + {entry.pf_user || '—'} + 0 ? 'text-green-700' : entry.value_total < 0 ? 'text-red-600' : 'text-gray-400'}`}> {entry.value_total == null ? '—' @@ -1944,8 +1954,15 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio @@ -1955,7 +1972,7 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio if (expandedLog !== entry.id) return [row] return [row, ( - +
From 39b2a7e4a236eefc5f8213b4c165d2f194bc1397 Mon Sep 17 00:00:00 2001 From: Paul Trowbridge Date: Fri, 18 Sep 2026 12:06:13 -0400 Subject: [PATCH 7/8] Only the author or an admin can edit a log entry PATCH /log/:logid had no check, so any account could edit the tag, note, label and bucket of any entry -- including loads whose rows it cannot see. That reads as harmless annotation and is not: label and bucket name the pivot's columns for everyone in the version, so a rep could rename the company's segments. Same rule as undo now, author or admin, with the fields shown read-only rather than editable-then-403 -- in the change log's tag and note cells and on the Baseline page's label and bucket. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 10 +++++++--- routes/log.js | 15 +++++++++++++++ ui/src/views/Baseline.jsx | 11 +++++++++++ ui/src/views/Forecast.jsx | 22 ++++++++++++++++++---- 4 files changed, 51 insertions(+), 7 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index c77a920..e5faf78 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -380,9 +380,13 @@ Enforced at: - every operation, through `sliceUnits()` - `/sources/:id/values/:col` — completion reads the *source* table, which no scope has touched, so without it a dropdown enumerates the whole business -- `DELETE /log/:logid` — by owner, not territory: undo removes an entry's rows - wholesale, and half-undoing one would leave a state nothing describes. Your - own entries, or an admin's override. +- `DELETE /log/:logid` and `PATCH /log/:logid` — by owner, not territory. Undo + removes an entry's rows wholesale, and half-undoing one would leave a state + nothing describes. The PATCH looks like a private annotation and is not: + `label` and `bucket` name the pivot's columns for everyone in the version, so + unguarded it let any account rename the company's segments. Your own entries, + or an admin's override, and the UI greys out the rest rather than offering a + click that answers 403. - recode's `set` — a scoped account cannot set the territory column at all. Moving a row between territories is reassignment, not forecasting, and it would vanish from the view that would have shown what happened. diff --git a/routes/log.js b/routes/log.js index f021d5c..94e5e10 100644 --- a/routes/log.js +++ b/routes/log.js @@ -190,6 +190,21 @@ module.exports = function(pool) { }); } try { + // Same rule as undo: your own entries, or an admin's. These are + // annotations, but label and bucket name the pivot's columns for + // everyone who opens the version, so an unguarded PATCH let any + // account rename the company's segments -- including on loads whose + // rows it cannot see. + const owner = await pool.query( + `SELECT pf_user FROM pf.log WHERE id = $1`, [logId] + ); + if (!owner.rows.length) return res.status(404).json({ error: 'Log entry not found' }); + if (!req.session?.user?.is_admin && owner.rows[0].pf_user !== sessionUser(req)) { + return res.status(403).json({ + error: `That entry was made by ${owner.rows[0].pf_user || 'someone else'} — only they or an administrator can change it` + }); + } + // COALESCE on the flag, not the value: an explicit null or '' must be // able to clear a field, which COALESCE on the value alone would ignore const result = await pool.query( diff --git a/ui/src/views/Baseline.jsx b/ui/src/views/Baseline.jsx index 5a862f8..6e92d01 100644 --- a/ui/src/views/Baseline.jsx +++ b/ui/src/views/Baseline.jsx @@ -1,5 +1,6 @@ import { useState, useEffect } from 'react' import Timeline from '../components/Timeline.jsx' +import useAuth from '../auth.jsx' const OPERATORS = ['BETWEEN', '=', '!=', 'IN', 'NOT IN', 'IS NULL', 'IS NOT NULL'] @@ -86,6 +87,7 @@ function normalizeFilters(stored) { } export default function Baseline({ sources = [], sourceId, versions = [], versionId, setVersionId, refreshVersions }) { + const { user: me } = useAuth() const [filterCols, setFilterCols] = useState([]) const [log, setLog] = useState([]) @@ -149,6 +151,11 @@ export default function Baseline({ sources = [], sourceId, versions = [], versio // Both are only read at load time, hence the reload in the confirmation: the // label is part of the aggregated row the pivot holds, not something it can // re-derive in place. + // Same rule the server enforces: your own entries, or an admin's. A segment's + // label and bucket name the pivot's columns for everyone in the version, so + // they are not the private annotation they look like. + const canEdit = (entry) => !!me && (me.is_admin || entry.pf_user === me.username) + async function saveLogField(entry, field, value) { const next = value.trim() if (next === (entry[field] || '')) return @@ -516,6 +523,8 @@ export default function Baseline({ sources = [], sourceId, versions = [], versio defaultValue={entry.label || ''} key={`label-${entry.id}-${entry.label || ''}`} onBlur={e => saveLogField(entry, 'label', e.target.value)} + readOnly={!canEdit(entry)} + title={canEdit(entry) ? '' : `${entry.pf_user || 'Another account'} made this segment`} placeholder={entry.tag || entry.note || '—'} className="w-full border border-transparent hover:border-gray-200 focus:border-blue-400 rounded px-1 py-0.5 text-xs @@ -539,6 +548,8 @@ export default function Baseline({ sources = [], sourceId, versions = [], versio list="pf-bucket-options" onChange={e => setBuckets(b => ({ ...b, [entry.id]: e.target.value }))} onBlur={e => saveLogField(entry, 'bucket', e.target.value)} + readOnly={!canEdit(entry)} + title={canEdit(entry) ? '' : `${entry.pf_user || 'Another account'} made this segment`} placeholder="—" className="w-full border border-transparent hover:border-gray-200 focus:border-blue-400 rounded px-1 py-0.5 text-xs focus:outline-none bg-transparent" /> diff --git a/ui/src/views/Forecast.jsx b/ui/src/views/Forecast.jsx index fe05b10..c588132 100644 --- a/ui/src/views/Forecast.jsx +++ b/ui/src/views/Forecast.jsx @@ -1932,7 +1932,7 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio {fmtSliceSummary(entry.slice)} - ( @@ -1940,7 +1940,7 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio {v} )} /> - {entry.pf_user || '—'} @@ -2117,10 +2117,24 @@ function PanelChrome({ dock, setDock, floating, onMouseDown, onClose }) { // One inline-editable annotation cell in the change log. Click to edit, Enter to // save, Escape to cancel — the same gesture for note and tag. -function LogCell({ entry, field, placeholder, editing, setEditing, onSave, listId, render }) { - const active = editing?.id === entry.id && editing?.field === field +function LogCell({ entry, field, placeholder, editing, setEditing, onSave, listId, render, canEdit = true }) { + const active = canEdit && editing?.id === entry.id && editing?.field === field const value = entry[field] || '' + // Someone else's entry: shown, not editable. label and bucket name the + // pivot's columns for everyone, so these are not the private annotations + // they look like. + if (!canEdit) { + return ( + + + {value ? (render ? render(value) : value) : —} + + + ) + } + if (active) { return ( From c6005bd17c9cc4b924915bc4ed7bcf39fa9fd38a Mon Sep 17 00:00:00 2001 From: Paul Trowbridge Date: Fri, 18 Sep 2026 12:19:40 -0400 Subject: [PATCH 8/8] Record what a write ran against, and the statement it ran params says what was asked for. That is not enough to explain a surprising result, because the same intent produces different rows depending on state the entry does not carry, and because the translation from intent to SQL is itself a place bugs live. So both, not one. env records the state that cannot be reconstructed later: the territory in force, the version's exclude_iters, and when the template was generated -- all mutable rows elsewhere with nothing remembering what they were. sql_text records the statement as executed, territory and scope already resolved into it. The template generation is a fingerprint rather than a version. It cannot bring the old template back; it can tell you the entry did not run under the current one, which is what would otherwise make a comparison quietly wrong. Generate SQL has overwritten those templates four times today. The statement is fetched on demand through GET /log/:logid/debug and left out of the list, which is opened to scan rather than to read SQL. Under an entry's payload in the change log there is now an "executed SQL" toggle. Co-Authored-By: Claude Opus 5 (1M context) --- routes/log.js | 30 ++++++++++++++++++- routes/operations.js | 62 ++++++++++++++++++++++++++++++++------- setup_sql/01_schema.sql | 19 ++++++++++++ ui/src/views/Forecast.jsx | 41 ++++++++++++++++++++++++++ 4 files changed, 140 insertions(+), 12 deletions(-) diff --git a/routes/log.js b/routes/log.js index 94e5e10..bd657c5 100644 --- a/routes/log.js +++ b/routes/log.js @@ -92,7 +92,35 @@ module.exports = function(pool) { ); } } - res.json(result.rows); + // The statement is kilobytes per entry and the list is opened to scan, + // not to read SQL. It stays in the row for the debug endpoint below. + res.json(result.rows.map(({ sql_text, ...r }) => ({ + ...r, has_sql: !!sql_text, + }))); + } catch (err) { + console.error(err); + res.status(err.status || 500).json({ error: err.message }); + } + }); + + // Everything about one entry, for when the rows look wrong: what was asked + // for (params), what it ran against (env), and the statement that actually + // executed with territory and scope resolved into it (sql_text). + // + // Both the intent and the SQL, because the translation between them is + // exactly what is in doubt when a result is surprising. + router.get('/log/:logid/debug', async (req, res) => { + const logId = parseInt(req.params.logid); + try { + const { rows } = await pool.query( + `SELECT l.*, v.name AS version_name, s.schema, s.tname + FROM pf.log l + JOIN pf.version v ON v.id = l.version_id + JOIN pf.source s ON s.id = v.source_id + WHERE l.id = $1`, [logId] + ); + if (!rows.length) return res.status(404).json({ error: 'Log entry not found' }); + res.json(rows[0]); } catch (err) { console.error(err); res.status(err.status || 500).json({ error: err.message }); diff --git a/routes/operations.js b/routes/operations.js index bbea6c2..04d22b7 100644 --- a/routes/operations.js +++ b/routes/operations.js @@ -111,16 +111,30 @@ module.exports = function(pool) { // // Best-effort by design. A failure here must not roll back a write that // succeeded -- the totals can always be recomputed, the adjustment cannot. - async function stampLogTotals(client, ctx, logId) { + async function stampLogTotals(client, ctx, logId, extra = {}) { if (!logId) return; const v = ctx.valueCol, u = ctx.unitsCol; + // The state the write ran against, none of which can be reconstructed + // later: territory and exclude_iters are mutable rows elsewhere, and the + // template is overwritten in place every time Generate SQL runs. The + // generation timestamp is a fingerprint, not a version -- it cannot bring + // the old template back, only tell you the entry did not run under this + // one. + const env = { + territory: extra.territory ?? null, + territory_col: ctx.territoryCol || null, + exclude_iters: ctx.version.exclude_iters ?? null, + sql_generated_at: ctx.sqlGeneratedAt || null, + }; try { await client.query(` UPDATE pf.log SET row_count = t.n, value_total = t.v, units_total = t.u, - measure_cols = $2::jsonb + measure_cols = $2::jsonb, + env = $3::jsonb, + sql_text = $4::text FROM ( SELECT count(*)::int AS n ,${v ? `sum(f."${v}")::float8` : 'NULL::float8'} AS v @@ -129,7 +143,8 @@ module.exports = function(pool) { WHERE f.pf_logid = $1 ) t WHERE pf.log.id = $1 - `, [logId, JSON.stringify({ value: v || null, units: u || null })]); + `, [logId, JSON.stringify({ value: v || null, units: u || null }), + JSON.stringify(env), extra.sql || null]); } catch (err) { console.error('[stampLogTotals]', err); } @@ -345,7 +360,7 @@ module.exports = function(pool) { const unitsCol = colMeta.find(c => c.role === 'units')?.cname; const sqlResult = await pool.query( - `SELECT sql FROM pf.sql WHERE source_id = $1 AND operation = $2`, + `SELECT sql, generated_at FROM pf.sql WHERE source_id = $1 AND operation = $2`, [version.source_id, operation] ); if (sqlResult.rows.length === 0) { @@ -363,7 +378,8 @@ module.exports = function(pool) { valueCol, unitsCol, territoryCol: colMeta.find(c => c.is_territory)?.cname || null, - sql: sqlResult.rows[0].sql + sql: sqlResult.rows[0].sql, + sqlGeneratedAt: sqlResult.rows[0].generated_at }; } @@ -536,7 +552,7 @@ module.exports = function(pool) { }); const result = await runSQL(sql); - await stampLogTotals(pool, ctx, result.rows[0]?.log_id); + await stampLogTotals(pool, ctx, result.rows[0]?.log_id, { sql, territory: sessionTerritory(req) }); res.json(result.rows[0]); } catch (err) { console.error(err); @@ -617,7 +633,7 @@ module.exports = function(pool) { await client.query(`DELETE FROM pf.log WHERE id = $1`, [logid]); const insResult = await client.query(sql); await client.query('COMMIT'); - await stampLogTotals(pool, ctx, insResult.rows[0]?.log_id); + await stampLogTotals(pool, ctx, insResult.rows[0]?.log_id, { sql, territory: sessionTerritory(req) }); res.json({ rows_deleted: delRows.rowCount, @@ -695,7 +711,7 @@ module.exports = function(pool) { }); const result = await runSQL(sql); - await stampLogTotals(pool, ctx, result.rows[0]?.log_id); + await stampLogTotals(pool, ctx, result.rows[0]?.log_id, { sql, territory: sessionTerritory(req) }); res.json(result.rows[0]); } catch (err) { console.error(err); @@ -732,6 +748,8 @@ module.exports = function(pool) { try { await client.query('BEGIN'); const allRows = []; + // the statement that produced each entry, for pf.log.sql_text + const sqlByLogId = new Map(); let applied = 0; const skipped = []; @@ -761,6 +779,9 @@ module.exports = function(pool) { }); const result = await runSQL(sql, client); await tagLog(client, result.rows, req.body.tag); + for (const r of result.rows) { + if (r.pf_logid != null) sqlByLogId.set(r.pf_logid, sql); + } allRows.push(...result.rows); } @@ -775,7 +796,10 @@ module.exports = function(pool) { committed = true; // one log id per unit: apply_mode 'each' writes an entry per slice for (const id of new Set(allRows.map(r => r.pf_logid).filter(Boolean))) { - await stampLogTotals(pool, ctx, id); + await stampLogTotals(pool, ctx, id, { + sql: sqlByLogId.get(id) || null, + territory: sessionTerritory(req), + }); } const opLabel = (req.body.tag || '').trim() || note || null; const rows = allRows.map(r => ({ ...r, pf_segment: ADJUSTMENT_SEGMENT, pf_bucket: ADJUSTMENT_BUCKET, pf_note: opLabel, pf_op: 'scale' })); @@ -819,6 +843,8 @@ module.exports = function(pool) { try { await client.query('BEGIN'); const allRows = []; + // the statement that produced each entry, for pf.log.sql_text + const sqlByLogId = new Map(); for (const unit of units) { const loggedSlice = unit.slices.length === 1 ? unit.slices[0] : unit.slices; const sql = applyTokens(ctx.sql, { @@ -834,13 +860,19 @@ module.exports = function(pool) { }); const result = await runSQL(sql, client); await tagLog(client, result.rows, req.body.tag); + for (const r of result.rows) { + if (r.pf_logid != null) sqlByLogId.set(r.pf_logid, sql); + } allRows.push(...result.rows); } await client.query('COMMIT'); committed = true; // one log id per unit: apply_mode 'each' writes an entry per slice for (const id of new Set(allRows.map(r => r.pf_logid).filter(Boolean))) { - await stampLogTotals(pool, ctx, id); + await stampLogTotals(pool, ctx, id, { + sql: sqlByLogId.get(id) || null, + territory: sessionTerritory(req), + }); } const opLabel = (req.body.tag || '').trim() || note || null; const rows = allRows.map(r => ({ ...r, pf_segment: ADJUSTMENT_SEGMENT, pf_bucket: ADJUSTMENT_BUCKET, pf_note: opLabel, pf_op: 'recode' })); @@ -912,6 +944,8 @@ module.exports = function(pool) { try { await client.query('BEGIN'); const allRows = []; + // the statement that produced each entry, for pf.log.sql_text + const sqlByLogId = new Map(); for (const unit of units) { const loggedSlice = unit.slices.length === 1 ? unit.slices[0] : unit.slices; const sql = applyTokens(ctx.sql, { @@ -933,13 +967,19 @@ module.exports = function(pool) { }); const result = await runSQL(sql, client); await tagLog(client, result.rows, req.body.tag); + for (const r of result.rows) { + if (r.pf_logid != null) sqlByLogId.set(r.pf_logid, sql); + } allRows.push(...result.rows); } await client.query('COMMIT'); committed = true; // one log id per unit: apply_mode 'each' writes an entry per slice for (const id of new Set(allRows.map(r => r.pf_logid).filter(Boolean))) { - await stampLogTotals(pool, ctx, id); + await stampLogTotals(pool, ctx, id, { + sql: sqlByLogId.get(id) || null, + territory: sessionTerritory(req), + }); } const opLabel = (req.body.tag || '').trim() || note || null; const rows = allRows.map(r => ({ ...r, pf_segment: ADJUSTMENT_SEGMENT, pf_bucket: ADJUSTMENT_BUCKET, pf_note: opLabel, pf_op: 'clone' })); diff --git a/setup_sql/01_schema.sql b/setup_sql/01_schema.sql index 3cb6f48..c0a8169 100644 --- a/setup_sql/01_schema.sql +++ b/setup_sql/01_schema.sql @@ -151,6 +151,25 @@ ALTER TABLE pf.log ADD COLUMN IF NOT EXISTS measure_cols jsonb; -- admin -- moving a row between territories is reassignment, not forecasting. ALTER TABLE pf.col_meta ADD COLUMN IF NOT EXISTS is_territory boolean NOT NULL DEFAULT false; +-- The debug path: what the entry was trying to do, and what that became. +-- +-- Both, deliberately. params records the intent -- the slice, the scope, the +-- resolved increments -- and sql_text records the statement as executed, with +-- territory and scope already resolved into it. Keeping only one of them +-- assumes the translation between them is correct, which is exactly the +-- assumption in doubt when the rows look wrong. +-- +-- env captures the state the intent was executed against that cannot be +-- reconstructed afterwards: the territory in force, the version's +-- exclude_iters, and when the template was generated. All three are mutable +-- rows elsewhere, and nothing remembers what they were. +-- +-- The template generation is a fingerprint, not a version: it cannot reproduce +-- the old template, but it can tell you the entry ran under a different one, +-- which is what would otherwise make a replay quietly wrong. +ALTER TABLE pf.log ADD COLUMN IF NOT EXISTS env jsonb; +ALTER TABLE pf.log ADD COLUMN IF NOT EXISTS sql_text text; + -- Master data for a dim_group: one row per key value, with its sibling columns. -- -- The source is transactional and often a view over all history, so deriving a diff --git a/ui/src/views/Forecast.jsx b/ui/src/views/Forecast.jsx index c588132..4de19c6 100644 --- a/ui/src/views/Forecast.jsx +++ b/ui/src/views/Forecast.jsx @@ -1976,7 +1976,12 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio
+ {entry.env && }
+ {/* The statement is fetched on demand: it is + kilobytes, and the list is opened to scan rather + than to read SQL. */} + {entry.has_sql && } )] @@ -2168,6 +2173,42 @@ function LogCell({ entry, field, placeholder, editing, setEditing, onSave, listI ) } +// The statement as executed, with territory and scope already resolved into it. +// Read this when the rows look wrong: params says what was asked for, and this +// says what actually ran -- the gap between them being where the bug lives. +function LogSql({ logId }) { + const [sql, setSql] = useState(null) + const [open, setOpen] = useState(false) + const [err, setErr] = useState(null) + + async function toggle() { + const next = !open + setOpen(next) + if (next && sql == null && !err) { + try { + const r = await fetch(`/api/log/${logId}/debug`) + const d = await r.json() + if (!r.ok) throw new Error(d.error || 'Could not load the statement') + setSql(d.sql_text || '(not recorded)') + } catch (e) { setErr(e.message) } + } + } + + return ( +
+ + {open && ( + 
+          {err || sql || 'Loading…'}
+
+ )} +
+ ) +} + // Perspective's internal columns, which are never dimensions. Mirrors isMetaColumn() // in @perspective-dev/viewer-datagrid — the DuckDB backend emits per-level // __ROW_PATH___ columns alongside the __ROW_PATH__ sidecar.