diff --git a/CLAUDE.md b/CLAUDE.md index 5dd76a6..e5faf78 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -353,6 +353,54 @@ 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` 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. + +**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..6983403 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" + success "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" + success "$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() { @@ -491,6 +583,9 @@ interactive_menu() { echo " 13) list-users show accounts" 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 @@ -510,6 +605,9 @@ interactive_menu() { 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 ;; q|Q|quit|exit) echo "Bye."; exit 0 ;; *) warn "Unknown option: $choice" ;; esac @@ -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..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] ); @@ -70,6 +71,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..bd657c5 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) { @@ -91,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 }); @@ -112,6 +141,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 @@ -178,6 +218,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/routes/operations.js b/routes/operations.js index 603866c..04d22b7 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)) }]; @@ -103,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 @@ -121,12 +143,30 @@ 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); } } + // 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', @@ -320,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) { @@ -337,10 +377,26 @@ module.exports = function(pool) { filterCols: [...dimCols, ...dateCols], valueCol, unitsCol, - sql: sqlResult.rows[0].sql + territoryCol: colMeta.find(c => c.is_territory)?.cname || null, + sql: sqlResult.rows[0].sql, + sqlGeneratedAt: sqlResult.rows[0].generated_at }; } + // 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 +419,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 +450,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 +494,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'); @@ -477,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); @@ -558,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, @@ -636,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); @@ -666,13 +741,15 @@ 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; 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 = []; @@ -702,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); } @@ -716,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' })); @@ -749,16 +832,19 @@ 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; 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, { @@ -774,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' })); @@ -845,13 +937,15 @@ 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; 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, { @@ -873,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/routes/sources.js b/routes/sources.js index 6073756..67eefcd 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) { @@ -86,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, @@ -100,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, @@ -110,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 ]); } @@ -235,6 +248,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..c0a8169 100644 --- a/setup_sql/01_schema.sql +++ b/setup_sql/01_schema.sql @@ -139,6 +139,37 @@ 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; + +-- 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/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; 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 3618c8f..4de19c6 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 @@ -777,8 +783,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 {} @@ -796,8 +812,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) @@ -1885,6 +1908,7 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio Slice Tag Note + By Value Rows @@ -1908,7 +1932,7 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio {fmtSliceSummary(entry.slice)} - ( @@ -1916,8 +1940,11 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio {v} )} /> - + + {entry.pf_user || '—'} + 0 ? 'text-green-700' : entry.value_total < 0 ? 'text-red-600' : 'text-gray-400'}`}> {entry.value_total == null ? '—' @@ -1927,8 +1954,15 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio @@ -1938,11 +1972,16 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio if (expandedLog !== entry.id) return [row] return [row, ( - +
+ {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 && } )] @@ -2083,10 +2122,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 ( @@ -2120,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. 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" + /> +