From 3fe766b2694123986bca32b41a0d0884f191d0a2 Mon Sep 17 00:00:00 2001 From: Ty Tremblay Date: Thu, 20 Aug 2026 22:35:38 -0400 Subject: [PATCH 1/3] Let a group edit be the first thing hawk-mod learns about someone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The roster-row guard on `/hawkmod group` was a deadlock. Rows are created by the user group sync, which only creates them for people already in a role group — so the command whose job is putting somebody in their first group refused everybody who needed it. The only ways onto the roster were editing the group by hand in Slack, or the CSV import, which needs a shell on the host. An app whose first-run instruction is "SSH into the server" is broken, and this had quietly reintroduced that. The comment justifying the guard — that an edit should never be the first thing hawk-mod learns about someone — had it backwards. Joining a role group is *how* someone becomes known; that is what the sync's `create` decision is for. So the subject of an edit is now a Slack account, not a roster row. `GroupEditRequest.subject` already allowed this; only the command insisted otherwise. The write fires `subteam_members_changed`, the sync creates the row moments later, and the reply says so rather than leaving a caller to wonder why `whois` finds nothing yet. Unchanged: an unrostered person added to the adults group needs no written reason, because they have no role to lose. That is what the sync has always done with someone added to that group by hand. Co-Authored-By: Claude Opus 5 --- src/slack/commands.ts | 73 +++++++++++++++++++++++++++++++++++++------ 1 file changed, 63 insertions(+), 10 deletions(-) diff --git a/src/slack/commands.ts b/src/slack/commands.ts index d301a21..baa68f4 100644 --- a/src/slack/commands.ts +++ b/src/slack/commands.ts @@ -294,6 +294,43 @@ async function resolvePerson( return undefined; } +/** + * Resolves a Slack account, roster row or not. + * + * `resolvePerson` answers "who is this on the roster", which is the right + * question almost everywhere and the wrong one for group edits: joining a role + * user group is *how* somebody gets a roster row, so demanding one first is a + * deadlock. This answers the smaller question — which Slack account did they + * mean — so the edit can proceed and the sync can create the row from it. + */ +async function resolveSlackId( + client: WebClient, + mention: string +): Promise { + const raw = mention.trim(); + if (!raw) return undefined; + + const escaped = raw.match(/^<@([A-Z0-9]+)/i)?.[1]; + if (escaped) return escaped.toUpperCase(); + if (/^U[A-Z0-9]{4,}$/i.test(raw)) return raw.toUpperCase(); + + const wanted = raw.replace(/^@/, "").toLowerCase(); + try { + const list = await client.users.list({ limit: 500 }); + const match = (list.members ?? []).find( + (m) => + !m.deleted && + !m.is_bot && + [m.name, m.profile?.display_name, m.profile?.real_name] + .filter(Boolean) + .some((n) => (n as string).toLowerCase() === wanted) + ); + return match?.id; + } catch { + return undefined; + } +} + async function whoisText( client: WebClient, teamId: string, @@ -380,14 +417,21 @@ async function groupText( return `Usage: \`/hawkmod group ${action} @user @group\`.`; } + // A roster row is not a precondition here, and requiring one was a deadlock: + // the sync only creates rows for people already in a role group, so the + // command meant to put somebody in their first group refused everybody who + // needed it. Slack's account is enough — `subteam_members_changed` fires on + // the write and the sync creates the roster row moments later. const person = await resolvePerson(client, mention); - if (!person) { + const slackId = + person?.slack_user_id ?? (await resolveSlackId(client, mention)); + if (!slackId) { return ( - `Couldn't find \`${mention}\` on the roster. hawk-mod only edits groups ` + - `for people it already knows, so the edit is never the first thing it ` + - `learns about someone.` + `Couldn't work out who \`${mention}\` is. Mention them with @ so Slack ` + + `sends their account, or paste their Slack member ID.` ); } + const who = person?.full_name ?? `<@${slackId}>`; const reason = reasonWords.join(" ").trim(); @@ -396,7 +440,7 @@ async function groupText( actor: caller, groupRef: ref, action, - subject: person, + subject: person ?? { slackUserId: slackId }, reason: reason || null, source: "command", }); @@ -407,27 +451,36 @@ async function groupText( // where the caller's own words are still to hand. if ("needsReason" in outcome) { return ( - `*${person.full_name}* is a student. ${outcome.reason}\n` + + `*${who}* is a student. ${outcome.reason}\n` + `\`/hawkmod group add ${mention} ${group} \`` ); } return outcome.reason; } if (outcome.noop) { - return `*${person.full_name}* was already ${ + return `*${who}* was already ${ action === "add" ? "in" : "out of" } @${outcome.handle}. Nothing changed.`; } const lines = [ - `${action === "add" ? "Added" : "Removed"} *${person.full_name}* ` + + `${action === "add" ? "Added" : "Removed"} *${who}* ` + `${action === "add" ? "to" : "from"} @${outcome.handle}.`, ]; + // Says out loud that the roster is about to catch up, so a caller who runs + // `whois` a second later and sees nothing knows to wait rather than to worry. + if (!person && action === "add") { + lines.push( + `_hawk-mod had no roster entry for them. The user group sync creates one ` + + `within a few seconds; \`/hawkmod whois\` will show it._` + ); + } + // The honest half. Group membership declares a role; it does not end // monitoring, and saying otherwise here would be the quiet failure this // project exists to avoid. - if (action === "remove") { + if (action === "remove" && person) { lines.push( `_${person.full_name} is still a ${person.role} on the roster and still ` + `monitored. Leaving a group never ends monitoring — use ` + @@ -436,7 +489,7 @@ async function groupText( } if (outcome.reducedMonitoring) { lines.push( - `_${person.full_name} is no longer monitored as a student. Recorded ` + + `_${who} is no longer monitored as a student. Recorded ` + `against your name: ${reason}_` ); } From 5d7880c60a4a1e2319e9495c4fec4f2a551832cf Mon Sep 17 00:00:00 2001 From: Ty Tremblay Date: Thu, 20 Aug 2026 22:39:25 -0400 Subject: [PATCH 2/3] Name the real user groups: @students and @mentors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The adults group is @mentors in production. Nothing was broken by the placeholder — both handles come from STUDENT_USERGROUP/ADULT_USERGROUP at runtime — but every example, prompt and fallback said "adults", which is the wrong thing to type and the wrong thing to look for in Slack. Changed where the text names a handle somebody types: .env.example, the setup script's prompt, the README's example, the `?? "adults"` fallbacks, and the test fixtures, so the regression test around moving a student into the mentors group reads the way production does. Left alone where "adults" is the role rather than the group. The two-adult rule, `isAdult`, screened adults, and the guidance text are about people, not about what a Slack group is called. `domain/rules/rosterSync.ts` stays handle-agnostic too — it is handed two sets and never learns what anyone named them, which is what keeps it a pure rule. Co-Authored-By: Claude Opus 5 --- .env.example | 2 +- CLAUDE.md | 2 +- CONTEXT.md | 2 +- README.md | 9 +++++---- scripts/setup-local.sh | 4 ++-- src/domain/rules/rosterSync.ts | 2 +- src/slack/app.ts | 2 +- src/slack/commands.ts | 6 +++--- src/slack/groupAdmin.ts | 4 ++-- test/groupMembership.test.ts | 32 ++++++++++++++++---------------- 10 files changed, 33 insertions(+), 32 deletions(-) diff --git a/.env.example b/.env.example index f031836..9929f32 100644 --- a/.env.example +++ b/.env.example @@ -28,7 +28,7 @@ LOG_MODE=full # exist on the free plan). Leave unset to maintain the roster purely by CSV. # Restrict "Create and edit user groups" to Owners/Admins before using these. STUDENT_USERGROUP=students -ADULT_USERGROUP=adults +ADULT_USERGROUP=mentors # Schedules (cron, in TZ). Defaults shown. TZ=America/New_York diff --git a/CLAUDE.md b/CLAUDE.md index fa8b0c6..2059708 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -104,7 +104,7 @@ is idempotent, which is what makes the duplicate events harmless). The reconciliation in `domain/rules/rosterSync.ts` is pure and **may only ever add monitoring, never subtract it**: a person dropped from the students group stays a student, since the alternative is silently ending someone's monitoring. Only -an explicit move into the adults group leaves `student`, and that raises +an explicit move into the mentors group leaves `student`, and that raises `roster_drift`. Every change lands in `role_changes` — Slack's audit log API is Grid-only, so that table is the only trail. Do not make this bidirectional. diff --git a/CONTEXT.md b/CONTEXT.md index e23b703..e1bb3e1 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -38,7 +38,7 @@ _Avoid_: Lead coach, admin role, superuser These two are the distinction most easily lost, and the one that matters most. **Declared**: -Present in the Slack user group that names a role — `@students` or `@adults`. +Present in the Slack user group that names a role — `@students` or `@mentors`. Cheap, reversible, and edited by hand in Slack or through hawk-mod. A declaration is a statement of intent, not a fact about monitoring. _Avoid_: Enrolled, rostered, assigned diff --git a/README.md b/README.md index fed266b..695ae01 100644 --- a/README.md +++ b/README.md @@ -192,8 +192,9 @@ npm run cli -- import-consents consents.csv ### Roles from Slack user groups -Set `STUDENT_USERGROUP` and `ADULT_USERGROUP` to user group handles (e.g. -`students`, `adults`) and each sweep reconciles roles from them, so membership +Set `STUDENT_USERGROUP` and `ADULT_USERGROUP` to user group handles — this +team uses `students` and `mentors` — and each sweep reconciles roles from them, +so membership is managed in Slack rather than by editing a CSV. Group membership is by Slack user ID, which removes the email-matching failure below entirely: a group member with no roster row gets one created from their Slack profile instead of @@ -203,8 +204,8 @@ Two properties make this safe to rely on: - **Membership is only ever added, never subtracted.** Dropping someone from the students group does _not_ un-student them — that would silently end their - monitoring. The only way out of `student` is being put in the adults group, - which is deliberate and raises a `roster_drift` finding. + monitoring. The only way out of `student` is being put in the mentors + group, which is deliberate and raises a `roster_drift` finding. - **Every role change is recorded** in `role_changes` with who, when, and from what. Slack's audit log API is Enterprise Grid only, so on Business+ this table is the sole durable trail of who was monitored when. diff --git a/scripts/setup-local.sh b/scripts/setup-local.sh index ea2e3a7..99ebca1 100755 --- a/scripts/setup-local.sh +++ b/scripts/setup-local.sh @@ -554,11 +554,11 @@ stage "Slack — user groups that declare roles" say "hawk-mod reads two user groups to decide who is a student and who is an" say "adult. Give it the TEST groups." step "In Slack: create the groups if they don't exist, and put ONLY accounts" -note " you control in them — you in the adults one, your second account" +note " you control in them — you in the mentors one, your second account" note " in the students one." say "" ask STUDENT_USERGROUP "Students group handle (without @):" -ask ADULT_USERGROUP "Adults group handle (without @):" +ask ADULT_USERGROUP "Mentors group handle (without @):" write_env STUDENT_USERGROUP "$STUDENT_USERGROUP" write_env ADULT_USERGROUP "$ADULT_USERGROUP" note "Membership is only ever added to the roster, never subtracted — dropping" diff --git a/src/domain/rules/rosterSync.ts b/src/domain/rules/rosterSync.ts index c31e31a..8ad2fd1 100644 --- a/src/domain/rules/rosterSync.ts +++ b/src/domain/rules/rosterSync.ts @@ -3,7 +3,7 @@ import { type Person, type Role } from "../people.js"; export type GroupMembership = { /** Slack ids in the group designating students. */ students: ReadonlySet; - /** Slack ids in the group designating adults/adults. */ + /** Slack ids in the group designating adults — @mentors, in this team. */ adults: ReadonlySet; }; diff --git a/src/slack/app.ts b/src/slack/app.ts index ae743a3..3e732e2 100644 --- a/src/slack/app.ts +++ b/src/slack/app.ts @@ -23,7 +23,7 @@ export const BOT_SCOPES = [ "team:read", "users:read", "users:read.email", - // Reads the @students / @adults groups that declare roles. + // Reads the @students / @mentors groups that declare roles. "usergroups:read", ]; diff --git a/src/slack/commands.ts b/src/slack/commands.ts index baa68f4..af42503 100644 --- a/src/slack/commands.ts +++ b/src/slack/commands.ts @@ -46,7 +46,7 @@ const HELP = [ "`/hawkmod backfill` — walk enrolled adults' DM history now", "", "_Roles come from Slack user groups. To add someone to the roster, add them", - "to the students or adults group — it applies straight away._", + "to the @students or @mentors group — it applies straight away._", ].join("\n"); export function registerCommands(app: App): void { @@ -101,7 +101,7 @@ export function registerCommands(app: App): void { `Couldn't find \`${rest.join(" ") || "(nobody)"}\` on the roster.\n` + `Usage: \`/hawkmod ${sub} @user\`. Roster membership comes from ` + `the user groups, so add them to @${config().STUDENT_USERGROUP ?? "students"} ` + - `or @${config().ADULT_USERGROUP ?? "adults"} first.`, + `or @${config().ADULT_USERGROUP ?? "mentors"} first.`, }); return; } @@ -395,7 +395,7 @@ function groupRef(raw: string): string | null { /** * `/hawkmod group add|remove @user @group`. * - * Moving a student into the adults group requires a written reason. Refusing it + * Moving a student into the mentors group requires a written reason. Refusing it * outright would be worse than allowing it: the action would simply happen in * Slack's own UI instead, where hawk-mod learns of it from an event carrying no * reason and no author. Requiring a sentence keeps the most consequential edit diff --git a/src/slack/groupAdmin.ts b/src/slack/groupAdmin.ts index 60d5a99..43e9346 100644 --- a/src/slack/groupAdmin.ts +++ b/src/slack/groupAdmin.ts @@ -142,7 +142,7 @@ export async function applyGroupEdit( }; } - // Moving a student into the adults group ends their monitoring as a + // Moving a student into the mentors group ends their monitoring as a // student. Allowed — refusing would only push the same act into Slack's own // UI, where hawk-mod learns of it from an event with no author and no // reason — but never silently, and never by typo. @@ -152,7 +152,7 @@ export async function applyGroupEdit( action: req.action, subjectRole: req.subject.role, handle: group.handle, - adultHandle: config().ADULT_USERGROUP ?? "adults", + adultHandle: config().ADULT_USERGROUP ?? "mentors", }); if (reduces && !req.reason) { diff --git a/test/groupMembership.test.ts b/test/groupMembership.test.ts index fe6161e..df5d13b 100644 --- a/test/groupMembership.test.ts +++ b/test/groupMembership.test.ts @@ -98,27 +98,27 @@ describe("group membership plans", () => { * It is keyed on the resolved handle for a reason — see the regression below. */ describe("edits that end a student's monitoring", () => { - const adults = "adults"; + const mentors = "mentors"; - it("flags a student being added to the adults group", () => { + it("flags a student being added to the mentors group", () => { assert.equal( reducesMonitoring({ action: "add", subjectRole: "student", - handle: "adults", - adultHandle: adults, + handle: "mentors", + adultHandle: mentors, }), true ); }); - it("ignores an adult being added to the adults group", () => { + it("ignores an adult being added to the mentors group", () => { assert.equal( reducesMonitoring({ action: "add", subjectRole: "adult", - handle: "adults", - adultHandle: adults, + handle: "mentors", + adultHandle: mentors, }), false ); @@ -130,7 +130,7 @@ describe("edits that end a student's monitoring", () => { action: "add", subjectRole: "student", handle: "programming", - adultHandle: adults, + adultHandle: mentors, }), false ); @@ -141,8 +141,8 @@ describe("edits that end a student's monitoring", () => { reducesMonitoring({ action: "remove", subjectRole: "student", - handle: "adults", - adultHandle: adults, + handle: "mentors", + adultHandle: mentors, }), false ); @@ -153,8 +153,8 @@ describe("edits that end a student's monitoring", () => { reducesMonitoring({ action: "add", subjectRole: "student", - handle: "Adults", - adultHandle: "@adults", + handle: "mentors", + adultHandle: "@Mentors", }), true ); @@ -163,9 +163,9 @@ describe("edits that end a student's monitoring", () => { /** * Regression. The first version of this compared the *raw slash-command * argument* to the configured handle. The app sets `should_escape: true`, so - * Slack sends `` and the raw argument is an opaque - * id — which never equals "adults", so the gate never fired and a student - * could be moved into the adults group by typo, with no reason recorded. + * Slack sends `` and the raw argument is an opaque + * id — which never equals "mentors", so the gate never fired and a student + * could be moved into the mentors group by typo, with no reason recorded. * Only the resolved handle is a safe input here. */ it("is not fooled by a group id, because it never sees one", () => { @@ -174,7 +174,7 @@ describe("edits that end a student's monitoring", () => { action: "add", subjectRole: "student", handle: "S0614TY5A", - adultHandle: adults, + adultHandle: mentors, }), false ); From 1d6b3180ea612e8bd5ef0bbbb236a84dd770676b Mon Sep 17 00:00:00 2001 From: Ty Tremblay Date: Thu, 20 Aug 2026 22:48:49 -0400 Subject: [PATCH 3/3] Configure hawk-mod from Slack, not from a file on the host MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Which user group declares students is a decision a Slack admin makes, and until now only an SSH session could change it. `authz.ts` already rejected that shape of problem once — "an app whose first-run instruction is 'SSH into the server' is broken" — and then it was left true of the settings that decide who is monitored. Four settings move: the student group, the mentor group, the editable-group allowlist, and the alert channel. `src/settings.ts` resolves each one database → environment → unset, so the env var becomes a seed rather than the source of truth. Nothing breaks for a running host and there is no flag day; a value set from Slack simply wins. `/hawkmod config` shows every value *and where it came from*. That second half is the point: "which group is it actually using, and who set it" is the question asked when the roster looks wrong, and it cost two rounds of guessing to answer before this existed. What cannot move is an allowlist, not an oversight. Slack credentials cannot be configured from Slack, and TOKEN_ENCRYPTION_KEY must never be reachable — changing it makes every stored token undecryptable and every enrolled adult invisible, with coverage still reporting 100%. A test asserts those keys stay unreachable. Handles are validated against Slack before they are stored, because a stored typo reads exactly like an empty group: nobody rostered, nobody monitored, no complaint. Changing a role group re-syncs immediately rather than at 3am, so the setting cannot look applied while it is not. Making the alert channel optional meant every caller had to face what an unset one means: a finding recorded and announced to nobody. It is an error at boot and an error at every use, naming the finding going unreported and the command that fixes it. `settings.ts` reads process.env directly rather than through `config()`, which would throw at import time in the CLI. Verified: the CLI still runs on an empty environment. Co-Authored-By: Claude Opus 5 --- .env.example | 3 + CLAUDE.md | 16 ++++ README.md | 9 +- migrations/0007_settings.sql | 35 +++++++ src/config.ts | 35 ++----- src/db/repo.ts | 76 +++++++++++++++ src/index.ts | 24 ++++- src/jobs/syncRoles.ts | 22 +++-- src/monitor/remediation.ts | 5 +- src/settings.ts | 121 +++++++++++++++++++++++ src/slack/alerts.ts | 39 +++++++- src/slack/commands.ts | 181 ++++++++++++++++++++++++++++++++++- src/slack/groupAdmin.ts | 5 +- test/settings.test.ts | 113 ++++++++++++++++++++++ 14 files changed, 632 insertions(+), 52 deletions(-) create mode 100644 migrations/0007_settings.sql create mode 100644 src/settings.ts create mode 100644 test/settings.test.ts diff --git a/.env.example b/.env.example index 9929f32..d459423 100644 --- a/.env.example +++ b/.env.example @@ -27,6 +27,9 @@ LOG_MODE=full # Slack user groups that declare roles (Business+ only; user groups do not # exist on the free plan). Leave unset to maintain the roster purely by CSV. # Restrict "Create and edit user groups" to Owners/Admins before using these. +# Seeds only. Once hawk-mod is running these four are set from Slack with +# `/hawkmod config`, which stores them in the database and shows you where each +# value came from. A value set there wins over anything here. STUDENT_USERGROUP=students ADULT_USERGROUP=mentors diff --git a/CLAUDE.md b/CLAUDE.md index 2059708..9217bc7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -80,6 +80,22 @@ that has shipped. All SQL lives in `src/db/repo.ts` and nowhere else, and better-sqlite3 is synchronous — repo functions are not `async`, so anything `await`ed in this codebase is Slack, not the database. +**Settings a Slack admin owns live in the database, not the environment.** +`src/settings.ts` resolves each one **database → environment → unset**, so the +env var is a _seed_ rather than the source of truth and an existing host keeps +working with no flag day. `/hawkmod config` shows every value **and where it +came from**, which is the question actually asked when the roster looks wrong. +`SETTINGS` is an allowlist and must stay one: Slack credentials cannot be set +from Slack, and `TOKEN_ENCRYPTION_KEY` must never be reachable — changing it +makes every stored token undecryptable and every enrolled adult invisible while +coverage still reads 100%. A user group handle is validated against Slack before +it is stored, because a stored typo reads exactly like an empty group. Changing +a role group re-syncs immediately; leaving it until 3am would mean the setting +looked applied and was not. Every change lands in `setting_changes`. + +Note `settings.ts` reads `process.env` directly rather than through `config()` — +it is reachable from the CLI, and `config()` there would throw at import time. + **`config()` is all or nothing.** One zod parse of the whole environment, on first use; a missing `SLACK_*` var throws for every caller. That is why `dataDir()` and `logMode()` exist as separate readers — the CLI runs diff --git a/README.md b/README.md index 695ae01..7571ffc 100644 --- a/README.md +++ b/README.md @@ -192,9 +192,9 @@ npm run cli -- import-consents consents.csv ### Roles from Slack user groups -Set `STUDENT_USERGROUP` and `ADULT_USERGROUP` to user group handles — this -team uses `students` and `mentors` — and each sweep reconciles roles from them, -so membership +Set the student and mentor groups with `/hawkmod config` — this team uses +`students` and `mentors` — and each sweep reconciles roles from them, so +membership is managed in Slack rather than by editing a CSV. Group membership is by Slack user ID, which removes the email-matching failure below entirely: a group member with no roster row gets one created from their Slack profile instead of @@ -206,6 +206,9 @@ Two properties make this safe to rely on: the students group does _not_ un-student them — that would silently end their monitoring. The only way out of `student` is being put in the mentors group, which is deliberate and raises a `roster_drift` finding. +- **The handle is checked before it is stored.** A user group that does not + resolve is refused outright, because a stored typo reads exactly like an + empty group: nobody rostered, nobody monitored, no complaint. - **Every role change is recorded** in `role_changes` with who, when, and from what. Slack's audit log API is Enterprise Grid only, so on Business+ this table is the sole durable trail of who was monitored when. diff --git a/migrations/0007_settings.sql b/migrations/0007_settings.sql new file mode 100644 index 0000000..9f12ffb --- /dev/null +++ b/migrations/0007_settings.sql @@ -0,0 +1,35 @@ +-- Settings a Slack admin can change without a shell on the host. +-- +-- `authz.ts` already made this argument once, about who may administer +-- hawk-mod: "an app whose first-run instruction is 'SSH into the server' is +-- broken." The same was still true of which user group declares students — a +-- decision a Slack admin makes, that only a Linode login could change, and that +-- reads identically to "the group is empty" when it is wrong. +-- +-- Credentials stay in the environment: you cannot configure from Slack the +-- things that let hawk-mod reach Slack, and TOKEN_ENCRYPTION_KEY changing would +-- make every stored token undecryptable. What moves here is policy, not +-- identity. +-- +-- Lives in the same SQLite file as everything else, so it survives a redeploy +-- and is already covered by the backup step in docs/deploy.md. +CREATE TABLE settings ( + key TEXT PRIMARY KEY, + value TEXT NOT NULL, + updated_by TEXT NOT NULL, + updated_at TEXT NOT NULL +); + +-- Which user group declares who is monitored is a youth-protection fact, so +-- changing it leaves the same kind of trail as changing somebody's role does. +-- Slack's audit log API is Enterprise Grid only; this is the trail. +CREATE TABLE setting_changes ( + id INTEGER PRIMARY KEY, + key TEXT NOT NULL, + from_value TEXT, -- NULL when the setting was previously unset + to_value TEXT NOT NULL, + actor TEXT NOT NULL, + actor_name TEXT NOT NULL, + changed_at TEXT NOT NULL +); +CREATE INDEX setting_changes_key_idx ON setting_changes (key, changed_at); diff --git a/src/config.ts b/src/config.ts index 3526f18..45bdd2e 100644 --- a/src/config.ts +++ b/src/config.ts @@ -7,23 +7,20 @@ const schema = z.object({ SLACK_STATE_SECRET: z.string().min(16), PUBLIC_URL: z.string().url(), PORT: z.coerce.number().int().positive().default(3000), - ALERT_CHANNEL_ID: z.string().min(1), + // Seeds only, all four of them. A value set from Slack wins — see + // src/settings.ts. Kept optional so a fresh install can be configured + // entirely from Slack, and so an existing host keeps working unchanged. + ALERT_CHANNEL_ID: z.string().optional(), DATA_DIR: z.string().default("./data"), TOKEN_ENCRYPTION_KEY: z.string().min(1), LOG_MODE: z.enum(["full", "metadata"]).default("full"), - // Slack user groups that declare roles. Both optional: leave them unset and - // the roster is maintained purely by CSV import. + // Slack user groups that declare roles. Seeds for the settings of the same + // name; leave both unset and unconfigured and the roster is maintained purely + // by CSV import. STUDENT_USERGROUP: z.string().optional(), ADULT_USERGROUP: z.string().optional(), - // Further user group handles hawk-mod may edit, comma separated. The two - // role groups above are always editable; this widens the allowlist to - // subteams (@programming, @drive-team) without widening it to everything. - // - // The allowlist is not authorization — every caller is already a Workspace - // Owner or Admin who could edit any group in Slack's own UI. It is blast - // radius. `usergroups.users.update` replaces a group's entire membership, so - // a bad plan does not corrupt a group, it empties one, and this bounds how - // many groups a single bug can reach. + // Seed for the `managed-groups` setting; see src/settings.ts for what the + // allowlist is actually for. MANAGED_USERGROUPS: z.string().optional(), TZ: z.string().default("America/New_York"), SWEEP_CRON: z.string().default("0 3 * * *"), @@ -62,17 +59,3 @@ export function dataDir(): string { export function logMode(): "full" | "metadata" { return process.env.LOG_MODE === "metadata" ? "metadata" : "full"; } - -/** Handles hawk-mod is permitted to edit, lowercased and without the `@`. */ -export function managedGroupHandles(): Set { - const cfg = config(); - const extra = (cfg.MANAGED_USERGROUPS ?? "") - .split(",") - .map((h) => h.trim()) - .filter(Boolean); - return new Set( - [cfg.STUDENT_USERGROUP, cfg.ADULT_USERGROUP, ...extra] - .filter((h): h is string => Boolean(h)) - .map((h) => h.replace(/^@/, "").toLowerCase()) - ); -} diff --git a/src/db/repo.ts b/src/db/repo.ts index 519cc06..f62fa37 100644 --- a/src/db/repo.ts +++ b/src/db/repo.ts @@ -373,6 +373,82 @@ export function revokeConsent(consentId: number, on: string): void { .run(on, consentId); } +/* -------------------------------------------------------------- settings */ + +/** A setting a Slack admin has changed, or undefined if none has. */ +export function getSetting(key: string): string | undefined { + const row = db() + .prepare("SELECT value FROM settings WHERE key = ?") + .get(key) as { value: string } | undefined; + return row?.value; +} + +export function listSettingRows(): Array<{ + key: string; + value: string; + updated_by: string; + updated_at: string; +}> { + return db().prepare("SELECT * FROM settings ORDER BY key").all() as Array<{ + key: string; + value: string; + updated_by: string; + updated_at: string; + }>; +} + +/** + * Changes a setting and records who changed it, in one transaction. + * + * Which user group declares who is monitored is a youth-protection fact, so it + * leaves the same kind of trail as changing somebody's role does. Slack's audit + * log API is Enterprise Grid only; `setting_changes` is the trail. + */ +export function setSetting(args: { + key: string; + value: string; + actor: string; + actorName: string; +}): void { + const now = nowIso(); + const previous = getSetting(args.key) ?? null; + db().transaction(() => { + db() + .prepare( + `INSERT INTO settings (key, value, updated_by, updated_at) + VALUES (?, ?, ?, ?) + ON CONFLICT (key) DO UPDATE SET + value = excluded.value, + updated_by = excluded.updated_by, + updated_at = excluded.updated_at` + ) + .run(args.key, args.value, args.actorName, now); + db() + .prepare( + `INSERT INTO setting_changes (key, from_value, to_value, actor, + actor_name, changed_at) + VALUES (?, ?, ?, ?, ?, ?)` + ) + .run(args.key, previous, args.value, args.actor, args.actorName, now); + })(); +} + +export function listSettingChanges(limit = 100) { + return db() + .prepare( + `SELECT * FROM setting_changes ORDER BY changed_at DESC, id DESC LIMIT ?` + ) + .all(limit) as Array<{ + id: number; + key: string; + from_value: string | null; + to_value: string; + actor: string; + actor_name: string; + changed_at: string; + }>; +} + /* --------------------------------------------------------- group changes */ export type GroupChangeInput = { diff --git a/src/index.ts b/src/index.ts index 5e79a44..6ebe4b5 100644 --- a/src/index.ts +++ b/src/index.ts @@ -2,6 +2,7 @@ import { config } from "./config.js"; import { db } from "./db/client.js"; import { startSchedules } from "./jobs/schedule.js"; import { log } from "./logger.js"; +import { setting } from "./settings.js"; import { createApp } from "./slack/app.js"; async function main() { @@ -10,16 +11,33 @@ async function main() { const app = createApp(); await app.start(cfg.PORT); startSchedules(); + const students = setting("student-group"); + const mentors = setting("mentor-group"); + const alerts = setting("alert-channel"); + log.info("hawk-mod started", { port: cfg.PORT, logMode: cfg.LOG_MODE, installUrl: `${cfg.PUBLIC_URL}/slack/install`, - // Silence here used to look identical to "the groups are empty". + // Silence here used to look identical to "the groups are empty". The + // source matters as much as the value: "which group?" and "who set it?" + // are the two questions asked when the roster looks wrong. roleSource: - cfg.STUDENT_USERGROUP || cfg.ADULT_USERGROUP - ? `user groups (@${cfg.STUDENT_USERGROUP ?? "-"} / @${cfg.ADULT_USERGROUP ?? "-"})` + students.value || mentors.value + ? `user groups (@${students.value ?? "-"} [${students.source}] / ` + + `@${mentors.value ?? "-"} [${mentors.source}])` : "CSV import only — no user groups configured", + alertChannel: alerts.value ? `${alerts.value} [${alerts.source}]` : "unset", }); + + // A finding nobody is told about is the failure this project is built to + // avoid, so an unset alert channel is an error at boot rather than a surprise + // the first time something goes wrong. + if (!alerts.value) { + log.error("no alert channel configured; findings will not be announced", { + fix: "/hawkmod config set alert-channel #channel", + }); + } } main().catch((err) => { diff --git a/src/jobs/syncRoles.ts b/src/jobs/syncRoles.ts index bf198f5..524913d 100644 --- a/src/jobs/syncRoles.ts +++ b/src/jobs/syncRoles.ts @@ -1,5 +1,4 @@ import type { WebClient } from "@slack/web-api"; -import { config } from "../config.js"; import { createPersonFromSlack, peopleBySlackId, @@ -10,6 +9,7 @@ import { import { dedupeKey } from "../domain/findings.js"; import { reconcileRoles } from "../domain/rules/rosterSync.js"; import { log } from "../logger.js"; +import { settingValue } from "../settings.js"; import { raise } from "../raise.js"; import { fetchProfiles, resolveGroup } from "../slack/userGroups.js"; @@ -49,7 +49,9 @@ const SOURCE = "usergroup_sync"; export async function syncRolesFromUserGroups( client: WebClient ): Promise { - const cfg = config(); + // From Slack if an admin has set it, from the environment otherwise. + const studentHandle = settingValue("student-group"); + const adultHandle = settingValue("mentor-group"); const stats: RoleSyncStats = { enabled: false, studentsInGroup: 0, @@ -62,22 +64,22 @@ export async function syncRolesFromUserGroups( missingGroups: 0, }; - if (!cfg.STUDENT_USERGROUP && !cfg.ADULT_USERGROUP) return stats; + if (!studentHandle && !adultHandle) return stats; stats.enabled = true; - const studentGroup = cfg.STUDENT_USERGROUP - ? await resolveGroup(client, cfg.STUDENT_USERGROUP) + const studentGroup = studentHandle + ? await resolveGroup(client, studentHandle) : null; - const adultGroup = cfg.ADULT_USERGROUP - ? await resolveGroup(client, cfg.ADULT_USERGROUP) + const adultGroup = adultHandle + ? await resolveGroup(client, adultHandle) : null; // A configured group that does not exist is a typo, and it reads exactly // like an empty group: nobody rostered, nothing monitored, no complaint. // Being quieter by being blinder is the failure this project must not have. for (const [handle, group] of [ - [cfg.STUDENT_USERGROUP, studentGroup], - [cfg.ADULT_USERGROUP, adultGroup], + [studentHandle, studentGroup], + [adultHandle, adultGroup], ] as const) { if (!handle || group) continue; stats.missingGroups += 1; @@ -88,7 +90,7 @@ export async function syncRolesFromUserGroups( summary: `Configured user group @${handle} does not exist in this workspace. ` + `Nobody is being rostered from it, so nobody is being monitored ` + - `through it either.`, + `through it either. Fix it with \`/hawkmod config\`.`, subjectRef: handle, }); } diff --git a/src/monitor/remediation.ts b/src/monitor/remediation.ts index f3f69cf..1692710 100644 --- a/src/monitor/remediation.ts +++ b/src/monitor/remediation.ts @@ -14,6 +14,7 @@ import { } from "../domain/rules/remediation.js"; import type { DmVerdict } from "../domain/rules/dmPolicy.js"; import { log } from "../logger.js"; +import { settingValue } from "../settings.js"; import { botClient } from "../slack/tokens.js"; type FindingDetail = { @@ -81,8 +82,10 @@ export async function remediateOneOnOnes( // to the alarm, rather than leaving a violation that looks unanswered. if (finding.alert_ts) { try { + const channel = settingValue("alert-channel"); + if (!channel) throw new Error("no alert channel configured"); await botClient().chat.postMessage({ - channel: config().ALERT_CHANNEL_ID, + channel, thread_ts: finding.alert_ts, text: `:white_check_mark: Put right — ${note} Acknowledged automatically; the 1:1 messages remain on record.`, }); diff --git a/src/settings.ts b/src/settings.ts new file mode 100644 index 0000000..49e72f4 --- /dev/null +++ b/src/settings.ts @@ -0,0 +1,121 @@ +import { getSetting } from "./db/repo.js"; + +/** + * Settings a Slack admin can change from Slack. + * + * Deliberately reads `process.env` directly rather than going through + * `config()`. `config()` is an all-or-nothing parse of the whole Slack + * environment, and this module is reachable from the CLI, which runs + * `import-roster` and `findings` with no Slack credentials present. Touching + * `config()` from here would break those commands at import time. + * + * The environment is no longer the source of truth for these — it is the seed. + * A value set in Slack wins; the env var is what a fresh install starts from, + * so nothing breaks for a host that already has one and there is no flag day. + */ + +export type SettingKind = "usergroup" | "usergroup_list" | "channel"; + +export type SettingSpec = { + /** The environment variable this used to live in, and still falls back to. */ + env: string; + label: string; + kind: SettingKind; + /** Shown by `/hawkmod config` when nothing is set anywhere. */ + hint: string; +}; + +/** + * The allowlist, and it is an allowlist on purpose. Slack credentials cannot be + * here — you cannot configure from Slack the thing that lets hawk-mod reach + * Slack — and `TOKEN_ENCRYPTION_KEY` must never be, because changing it makes + * every stored token undecryptable and every enrolled adult silently invisible. + */ +export const SETTINGS = { + "student-group": { + env: "STUDENT_USERGROUP", + label: "Student user group", + kind: "usergroup", + hint: "the group whose members are monitored as students", + }, + "mentor-group": { + env: "ADULT_USERGROUP", + label: "Mentor user group", + kind: "usergroup", + hint: "the group whose members are rostered as adults", + }, + "managed-groups": { + env: "MANAGED_USERGROUPS", + label: "Other editable groups", + kind: "usergroup_list", + hint: "comma separated; the two role groups are always editable", + }, + "alert-channel": { + env: "ALERT_CHANNEL_ID", + label: "Alert channel", + kind: "channel", + hint: "where findings are posted", + }, +} as const satisfies Record; + +export type SettingKey = keyof typeof SETTINGS; + +export const SETTING_KEYS = Object.keys(SETTINGS) as SettingKey[]; + +export function isSettingKey(key: string): key is SettingKey { + return Object.hasOwn(SETTINGS, key); +} + +export type Resolved = { + value: string | undefined; + /** Where the value came from — the thing that makes a wrong one debuggable. */ + source: "slack" | "env" | "unset"; +}; + +/** + * Pure so it can be tested without a database: the precedence rule is the whole + * point of this module and is worth pinning down on its own. + */ +export function resolveSetting( + fromDb: string | undefined, + fromEnv: string | undefined +): Resolved { + const db = fromDb?.trim(); + if (db) return { value: db, source: "slack" }; + const env = fromEnv?.trim(); + if (env) return { value: env, source: "env" }; + return { value: undefined, source: "unset" }; +} + +/** Splits a comma-separated setting into handles, without `@` and lowercased. */ +export function parseHandles(raw: string | undefined): string[] { + return (raw ?? "") + .split(",") + .map((h) => h.trim().replace(/^@/, "").toLowerCase()) + .filter(Boolean); +} + +export function setting(key: SettingKey): Resolved { + return resolveSetting(getSetting(key), process.env[SETTINGS[key].env]); +} + +/** The value alone, for the many callers that do not care where it came from. */ +export function settingValue(key: SettingKey): string | undefined { + return setting(key).value; +} + +/** + * Handles `/hawkmod group` is permitted to edit. + * + * The two role groups are always editable — they are the ones hawk-mod is for. + * Everything else has to be named, because `usergroups.users.update` replaces a + * group's whole membership, so a bad plan does not corrupt a group, it empties + * one. This bounds how many groups a single bug can reach. + */ +export function managedGroupHandles(): Set { + return new Set([ + ...parseHandles(settingValue("student-group")), + ...parseHandles(settingValue("mentor-group")), + ...parseHandles(settingValue("managed-groups")), + ]); +} diff --git a/src/slack/alerts.ts b/src/slack/alerts.ts index 9dd52ba..e3eae55 100644 --- a/src/slack/alerts.ts +++ b/src/slack/alerts.ts @@ -1,10 +1,31 @@ -import { config } from "../config.js"; import { getFinding, setFindingAlertTs } from "../db/repo.js"; import type { Finding } from "../domain/findings.js"; import { severityEmoji } from "../domain/findings.js"; import { log } from "../logger.js"; +import { settingValue } from "../settings.js"; import { botClient } from "./tokens.js"; +/** + * Where findings go, or `null` if nobody has said. + * + * Unset is not a quiet state. A finding with nowhere to be announced is + * recorded and invisible, which is the exact failure this project defines + * itself against — so every attempt to use it logs at error level, naming the + * finding that is going unreported and the command that fixes it. `/hawkmod + * config` shows it too, and so does startup. + */ +function alertChannel(context: string): string | null { + const channel = settingValue("alert-channel"); + if (!channel) { + log.error("no alert channel configured; nobody is being told", { + context, + fix: "/hawkmod config set alert-channel #channel", + }); + return null; + } + return channel; +} + export const ACK_ACTION = "hawkmod_finding_ack"; export const RESOLVE_ACTION = "hawkmod_finding_resolve"; @@ -84,8 +105,10 @@ export function findingBlocks(f: Finding): { */ async function supersede(previousTs: string, finding: Finding): Promise { try { + const channel = alertChannel("supersede"); + if (!channel) return; await botClient().chat.update({ - channel: config().ALERT_CHANNEL_ID, + channel, ts: previousTs, text: `${severityEmoji(finding.severity)} ${finding.kind} — happened again`, blocks: [ @@ -123,8 +146,10 @@ export async function postFinding(findingId: number): Promise { const previousTs = finding.alert_ts; const { text, blocks } = findingBlocks(finding); try { + const channel = alertChannel(`finding ${findingId}`); + if (!channel) return; const res = await botClient().chat.postMessage({ - channel: config().ALERT_CHANNEL_ID, + channel, text, blocks: blocks as never, }); @@ -146,8 +171,10 @@ export async function refreshFinding(findingId: number): Promise { if (!finding?.alert_ts) return; const { text, blocks } = findingBlocks(finding); try { + const channel = alertChannel(`finding ${finding.id}`); + if (!channel) return; await botClient().chat.update({ - channel: config().ALERT_CHANNEL_ID, + channel, ts: finding.alert_ts, text, blocks: blocks as never, @@ -162,8 +189,10 @@ export async function refreshFinding(findingId: number): Promise { export async function postToAlertChannel(text: string): Promise { try { + const channel = alertChannel("digest"); + if (!channel) return; await botClient().chat.postMessage({ - channel: config().ALERT_CHANNEL_ID, + channel, text, }); } catch (err) { diff --git a/src/slack/commands.ts b/src/slack/commands.ts index af42503..26a6974 100644 --- a/src/slack/commands.ts +++ b/src/slack/commands.ts @@ -5,6 +5,7 @@ import { closeFinding } from "../close.js"; import { config } from "../config.js"; import { countOpenByKind, + setSetting, getFinding, getInstallation, listConsents, @@ -21,9 +22,18 @@ import { requiresEnrollment, type Person } from "../domain/people.js"; import { consentStatus } from "../domain/rules/consent.js"; import { screeningStatus } from "../domain/rules/screening.js"; import { log } from "../logger.js"; +import { + isSettingKey, + SETTING_KEYS, + SETTINGS, + setting, + settingValue, + type SettingKey, +} from "../settings.js"; import { backfillAll } from "../monitor/backfill.js"; import { administrator, type Actor, NOT_PERMITTED } from "./authz.js"; import { applyGroupEdit } from "./groupAdmin.js"; +import { resolveGroup } from "./userGroups.js"; import { openConsent, openScreening } from "./modals.js"; import { runSweep } from "../jobs/sweep.js"; import { syncRolesFromUserGroups } from "../jobs/syncRoles.js"; @@ -37,6 +47,7 @@ const HELP = [ "`/hawkmod group add @user @group` — put someone in a user group", "`/hawkmod group remove @user @group` — take someone out of a user group", "`/hawkmod deactivate @user ` — stop monitoring someone", + "`/hawkmod config` — show settings; `config set ` to change one", "`/hawkmod screening @user` — record YPP / Mentor Ready / CORI dates", "`/hawkmod consent @user` — record a signed parental consent", "`/hawkmod ack ` — acknowledge without closing", @@ -100,8 +111,8 @@ export function registerCommands(app: App): void { text: `Couldn't find \`${rest.join(" ") || "(nobody)"}\` on the roster.\n` + `Usage: \`/hawkmod ${sub} @user\`. Roster membership comes from ` + - `the user groups, so add them to @${config().STUDENT_USERGROUP ?? "students"} ` + - `or @${config().ADULT_USERGROUP ?? "mentors"} first.`, + `the user groups, so add them to @${settingValue("student-group") ?? "students"} ` + + `or @${settingValue("mentor-group") ?? "mentors"} first.`, }); return; } @@ -151,6 +162,14 @@ export function registerCommands(app: App): void { return; } + case "config": { + await respond({ + response_type: "ephemeral", + text: await configText(client, caller, rest), + }); + return; + } + case "group": { await respond({ response_type: "ephemeral", @@ -542,3 +561,161 @@ async function deactivateText( } return lines.join("\n"); } + +/** + * `/hawkmod config` and `/hawkmod config set `. + * + * Everything here used to live in a `.env` file on the host, which meant a + * Slack admin could not change which user group declares students without an + * SSH session. `authz.ts` already rejected that shape of problem once, for + * administrative authority; this is the same argument applied to the settings + * that decide who is monitored. + * + * Credentials are deliberately absent, and cannot be added: `SETTINGS` is an + * allowlist. You cannot configure from Slack the things that let hawk-mod reach + * Slack, and changing the encryption key would make every stored token + * undecryptable. + */ +async function configText( + client: WebClient, + caller: Actor, + rest: string[] +): Promise { + const [verb, key, ...valueWords] = rest; + + if (!verb) return configListing(); + + if (verb !== "set") { + return ( + "Usage: `/hawkmod config` to show, " + + "`/hawkmod config set ` to change one." + ); + } + + if (!key || !isSettingKey(key)) { + return ( + `Unknown setting \`${key ?? "(none)"}\`. Settable: ` + + SETTING_KEYS.map((k) => `\`${k}\``).join(", ") + + ".\nSlack credentials and the encryption key are deliberately not " + + "settable from here." + ); + } + + const raw = valueWords.join(" ").trim(); + if (!raw) return `Usage: \`/hawkmod config set ${key} \`.`; + + const cleaned = await validateSetting(client, key, raw); + if ("error" in cleaned) return cleaned.error; + + const before = setting(key); + setSetting({ + key, + value: cleaned.value, + actor: caller.slackUserId, + actorName: caller.name, + }); + + const lines = [ + `*${SETTINGS[key].label}* is now \`${cleaned.value}\`` + + (before.value + ? ` (was \`${before.value}\`, from ${before.source})` + : "") + + ".", + ]; + + // Roles are read from these groups by everything downstream, so leaving the + // roster stale until 3am would mean the setting looked applied and was not. + if (key === "student-group" || key === "mentor-group") { + const stats = await syncRolesFromUserGroups(client); + lines.push( + `_Re-synced: ${stats.created} rostered, ${stats.changed} changed, ` + + `${stats.reactivated} resumed._` + ); + } + + return lines.join("\n"); +} + +function configListing(): string { + const rows = SETTING_KEYS.map((key) => { + const { value, source } = setting(key); + const where = + source === "slack" + ? "set here" + : source === "env" + ? `from ${SETTINGS[key].env}` + : "*not set*"; + return `• \`${key}\` — ${value ?? "—"} _(${where})_`; + }); + + const unset = SETTING_KEYS.filter((k) => setting(k).source === "unset"); + + return [ + "*Settings*", + ...rows, + "", + "`/hawkmod config set `", + ...(unset.length + ? ["", ...unset.map((k) => `_\`${k}\` is unset — ${SETTINGS[k].hint}._`)] + : []), + "_Slack credentials and the token encryption key stay in the environment " + + "and cannot be changed from here._", + ].join("\n"); +} + +/** + * Checks a value against Slack before storing it. + * + * A user group handle that does not resolve is a typo, and a stored typo reads + * exactly like an empty group: nobody rostered, nobody monitored, no complaint. + * The sweep would raise that eventually; refusing it here turns tomorrow's + * finding into an error message the person who caused it is still reading. + */ +async function validateSetting( + client: WebClient, + key: SettingKey, + raw: string +): Promise<{ value: string } | { error: string }> { + const kind = SETTINGS[key].kind; + + if (kind === "channel") { + // `<#C123|name>` when escaping is on, a bare id or #name when it is not. + const id = raw.match(/^<#([A-Z0-9]+)/i)?.[1] ?? raw.replace(/^#/, ""); + try { + const info = await client.conversations.info({ channel: id }); + if (!info.channel?.id) return { error: `No channel \`${raw}\`.` }; + return { value: info.channel.id }; + } catch (err) { + return { + error: + `Couldn't read \`${raw}\`: ${String(err)}\n` + + `hawk-mod must be a member of the channel it posts findings to.`, + }; + } + } + + const handles = raw + .split(",") + .map((h) => h.trim()) + .filter(Boolean) + .map((h) => h.match(/^]+)>$/i)?.[1] ?? h) + .map((h) => h.replace(/^@/, "")); + + if (kind === "usergroup" && handles.length !== 1) { + return { error: `\`${key}\` takes exactly one user group.` }; + } + + for (const handle of handles) { + const group = await resolveGroup(client, handle); + if (!group) { + return { + error: + `No user group @${handle} in this workspace. Nothing was changed — ` + + `a stored typo looks exactly like an empty group, which is why this ` + + `is checked before saving.`, + }; + } + } + + return { value: handles.join(",") }; +} diff --git a/src/slack/groupAdmin.ts b/src/slack/groupAdmin.ts index 43e9346..f888896 100644 --- a/src/slack/groupAdmin.ts +++ b/src/slack/groupAdmin.ts @@ -1,5 +1,5 @@ import { WebClient } from "@slack/web-api"; -import { config, managedGroupHandles } from "../config.js"; +import { config } from "../config.js"; import { getInstallation, insertGroupChange } from "../db/repo.js"; import type { Person } from "../domain/people.js"; import { @@ -9,6 +9,7 @@ import { type GroupPlan, } from "../domain/rules/groupMembership.js"; import { log } from "../logger.js"; +import { managedGroupHandles, settingValue } from "../settings.js"; import type { Actor } from "./authz.js"; import { resolveGroup, setGroupMembership } from "./userGroups.js"; @@ -152,7 +153,7 @@ export async function applyGroupEdit( action: req.action, subjectRole: req.subject.role, handle: group.handle, - adultHandle: config().ADULT_USERGROUP ?? "mentors", + adultHandle: settingValue("mentor-group") ?? "mentors", }); if (reduces && !req.reason) { diff --git a/test/settings.test.ts b/test/settings.test.ts new file mode 100644 index 0000000..41495d0 --- /dev/null +++ b/test/settings.test.ts @@ -0,0 +1,113 @@ +import assert from "node:assert/strict"; +import { describe, it } from "node:test"; +import { + isSettingKey, + parseHandles, + resolveSetting, + SETTINGS, + SETTING_KEYS, +} from "../src/settings.js"; + +describe("setting resolution", () => { + it("prefers a value set from Slack", () => { + assert.deepEqual(resolveSetting("mentors", "adults"), { + value: "mentors", + source: "slack", + }); + }); + + it("falls back to the environment", () => { + // The point of the fallback: an existing host keeps working unchanged, so + // moving these settings needs no flag day and no coordinated deploy. + assert.deepEqual(resolveSetting(undefined, "adults"), { + value: "adults", + source: "env", + }); + }); + + it("reports unset rather than guessing", () => { + assert.deepEqual(resolveSetting(undefined, undefined), { + value: undefined, + source: "unset", + }); + }); + + it("treats blank as unset on both sides", () => { + // A variable set to "" in a compose file is the common shape of this, and + // it must not beat a real value stored in Slack. + assert.deepEqual(resolveSetting(" ", "adults"), { + value: "adults", + source: "env", + }); + assert.deepEqual(resolveSetting("", ""), { + value: undefined, + source: "unset", + }); + }); + + it("trims, because a trailing space in a handle finds no group", () => { + assert.deepEqual(resolveSetting(" mentors ", undefined), { + value: "mentors", + source: "slack", + }); + }); +}); + +describe("handle parsing", () => { + it("splits a comma separated list", () => { + assert.deepEqual(parseHandles("programming, drive-team"), [ + "programming", + "drive-team", + ]); + }); + + it("strips @ and lowercases, so stored and typed forms match", () => { + assert.deepEqual(parseHandles("@Mentors"), ["mentors"]); + }); + + it("survives empty entries and trailing commas", () => { + assert.deepEqual(parseHandles("a,,b,"), ["a", "b"]); + assert.deepEqual(parseHandles(undefined), []); + assert.deepEqual(parseHandles(""), []); + }); +}); + +describe("the settable allowlist", () => { + it("recognises only known keys", () => { + assert.equal(isSettingKey("student-group"), true); + assert.equal(isSettingKey("nonsense"), false); + }); + + /** + * The load-bearing property. Slack credentials cannot be configured from + * Slack, and TOKEN_ENCRYPTION_KEY changing would make every stored token + * undecryptable — every enrolled adult silently invisible, with coverage + * still reading 100%. + */ + it("cannot reach credentials or the encryption key", () => { + const forbidden = [ + "SLACK_SIGNING_SECRET", + "SLACK_CLIENT_ID", + "SLACK_CLIENT_SECRET", + "SLACK_STATE_SECRET", + "TOKEN_ENCRYPTION_KEY", + "DATA_DIR", + "PUBLIC_URL", + "PORT", + ]; + const reachable = SETTING_KEYS.map((k) => SETTINGS[k].env); + for (const env of forbidden) { + assert.ok( + !reachable.includes(env as never), + `${env} must not be settable from Slack` + ); + } + }); + + it("every key names an env var to fall back to", () => { + for (const key of SETTING_KEYS) { + assert.ok(SETTINGS[key].env, `${key} has no env fallback`); + assert.ok(SETTINGS[key].label, `${key} has no label`); + } + }); +});