diff --git a/CLAUDE.md b/CLAUDE.md index 644e0b3..fa8b0c6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -45,7 +45,8 @@ code that carries it, and records which controls are deliberately manual. ## Architecture -**Two token planes.** The bot token posts alerts and reads channel membership. +**Two token planes, and a third narrow one.** The bot token posts alerts and +reads channel membership. Each adult's _user_ token is what makes DMs visible at all — Slack exposes no other way below Enterprise Grid. `src/slack/installStore.ts` keeps one `bot` row per workspace and one `user` row per enrolled adult, and `fetchInstallation` @@ -54,6 +55,18 @@ stripped so one adult's token never rides along to a request that did not ask for it. Tokens are encrypted at rest (`src/crypto.ts`); the DB file alone is not enough to read anyone's DMs. +The third is an `admin` row: an administrator's token scoped to +`usergroups:write` and nothing else, granted at `/slack/authorize-groups`. It +exists because Slack accepts a _bot_ token for `usergroups.users.update` only +when the workspace lets everyone edit user groups — which §6 forbids — so group +edits go out as a named person. It is a separate row rather than extra scopes on +that person's `user` row because Slack issues one token per authorization +carrying only that authorization's scopes: sharing a row would mean an +administrator who is also an enrolled mentor losing their DM token the moment +they authorized group editing, with coverage still reading 100%. +`saveInstallation` refuses a `user` write whose scopes lack `im:history` for +exactly that reason. Never merge these two grants. + **Students may not enroll.** `storeInstallation` throws on a student's user token, and the sweep revokes and deletes one that appears later (a person can be moved into the students group after enrolling). Their token would expose @@ -88,12 +101,42 @@ policy that lands in a handler is policy nothing covers. in the sweep, because everything after it reads roles, and again on `subteam_*` events so an edit applies immediately rather than at 3am (the sync is idempotent, which is what makes the duplicate events harmless). The -reconciliation in `domain/rules/rosterSync.ts` is pure and **add-only by -design**: 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 `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. +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 +`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. + +The invariant is about _direction_, not about writes, which is why `reactivate` +belongs there: a deactivated person who reappears in a role group is monitored +again immediately, for the same reason `create` needs nobody's approval. Ending +monitoring is `/hawkmod deactivate`, which demands a person and a reason — the +only operation in hawk-mod that makes it see less, and the only one no rule, job +or sync can reach. + +**Group membership is declaration; the roster is monitoring.** `/hawkmod group +add|remove` edits the Slack user group and nothing else. Removing someone from +`@students` leaves them a student on the roster, and the command says so in its +reply rather than letting the caller assume otherwise. `CONTEXT.md` keeps the +two words apart; conflating them is how a graduated student ends up monitored +forever, or a returning one ends up invisible. + +**Group edits go through a plan.** `domain/rules/groupMembership.ts` is pure and +diffs intended membership against actual, because `usergroups.users.update` +_replaces_ a group's whole member list — there is no add-one endpoint. So a bad +input does not corrupt a group, it empties one. The plan carries its own refusal +(over-large removals, emptying a group) so a caller cannot apply a bad one by +forgetting to check a flag elsewhere. `slack/groupAdmin.ts` serializes writes and +re-reads membership inside the lock. The single-user command is the degenerate +case of the spreadsheet-driven sync this was built for — one planner, two +callers. Editable groups are an allowlist (`MANAGED_USERGROUPS`), which is blast +radius rather than authorization: every caller is already a Slack admin who could +edit any group by hand. + +`group_changes` records who asked for an edit. `role_changes` cannot: it is +written by the sync, which runs from a Slack event long after the human is gone, +and most group edits will change no role at all once subteams are managed here. **Findings are the output, and have exactly one door each way.** `src/raise.ts` persists then alerts, and only alerts when the finding is new or has recurred. @@ -221,7 +264,7 @@ Building locally on macOS can trip the "access data from other apps" prompt; detectable without message text; only investigation needs the text. - Non-students with no screening on file do not count toward the two-adult rule, whatever their role. Do not loosen `isScreenedAdult`. -- **Every Slack entry point is gated on `mayAdministerWorkspace`**, and each one +- **Every Slack entry point is gated on `administrator()`**, and each one checks for itself: the slash command (`commands.ts`), the alert buttons and their note modal (`actions.ts`), and the screening and consent submissions (`modals.ts`). There is no middleware doing this centrally — anyone who can diff --git a/CONTEXT.md b/CONTEXT.md new file mode 100644 index 0000000..e23b703 --- /dev/null +++ b/CONTEXT.md @@ -0,0 +1,142 @@ +# hawk-mod + +Youth-protection auditing for a FIRST team's Slack workspace: it records +adult–student direct messages so a human can review them, because Slack cannot +prevent those messages below Enterprise Grid. + +This glossary exists because several words in this domain look like synonyms and +are not. Confusing any pair of them ends someone's monitoring quietly, which is +the failure this project is built to avoid. + +## People + +**Person**: +Someone the roster knows about, identified by their Slack account. +_Avoid_: User, member, account + +**Student**: +A person the roster records as a minor, whose direct messages with adults are +recorded. See **Declared** vs **Monitored** — the word alone is ambiguous. +_Avoid_: Kid, child, minor + +**Adult**: +Any person on the roster who is not a student, a district observer included. +Seniority is never an exemption. +_Avoid_: Mentor, coach, grown-up, leader + +**Screened adult**: +An adult with current Youth Protection Screening, Youth Protection Training, and +CORI on file. Only screened adults count toward the two-adult rule. + +**Administrator**: +Someone Slack records as a Workspace Owner or Admin. It is not a roster role and +is never stored — the roster says who is monitored, never who is in charge. +_Avoid_: Lead coach, admin role, superuser + +## Declaration vs monitoring + +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`. +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 + +**Monitored**: +Carried on the roster with a role and marked active. Sticky by design: gained +automatically when someone is declared, and lost only by an explicit, +attributed act. Removing a declaration never removes monitoring. +_Avoid_: Tracked, watched, covered + +**Active**: +The state of a person whose monitoring is in force. The opposite is +**deactivated** — a person the roster remembers but no longer monitors. + +**Deactivation**: +The deliberate act of ending a person's monitoring. The only operation in +hawk-mod that makes it see less, and therefore the only one that always names +who performed it and why. +_Avoid_: Removal, deletion, archiving, offboarding + +**Reactivation**: +Restoring monitoring to a deactivated person. Happens automatically when they +are declared again, because gaining protection never needs permission. + +## Enrollment + +**Enrollment**: +An adult's own authorization letting hawk-mod read their direct messages. It is +what makes DMs visible at all, and it is granted by that adult, never on their +behalf. Students may never enroll. +_Avoid_: Onboarding, signup, opting in, installation + +**Coverage**: +The proportion of adults requiring enrollment who have enrolled. An unenrolled +adult is a gap, and a gap is a finding rather than a silence. + +**Group-editing authorization**: +An administrator's separate authorization letting hawk-mod edit Slack user +groups as them. Distinct from enrollment in purpose, lifetime, and consent, and +held separately so that neither can revoke the other. + +## Conversations + +**Conversation**: +A Slack direct message or group direct message that hawk-mod has classified. +Classification depends only on who is present, never on what was said. + +**Verdict**: +What the rules conclude about a conversation from its participants alone. + +**Two-adult rule**: +The requirement that a student's conversation include at least two screened +adults. Unknown accounts never satisfy it. + +**Remedy**: +Moving a one-to-one conversation into a channel or adding a second adult. A +remedy acknowledges a finding; it never resolves one, because the one-to-one +still happened. +_Avoid_: Fix, resolution, correction + +## Findings + +**Finding**: +A recorded policy problem. Findings name people and conversations, never +message content. +_Avoid_: Alert, violation, issue, incident + +**Condition finding**: +A finding describing something currently true, such as lapsed screening. +Re-detecting one is not news. + +**Occurrence finding**: +A finding describing something that happened, such as a one-to-one message. Only +a newer event can repeat it. + +**Dedupe key**: +The identity of a problem across sweeps. It is what separates an alert channel +someone reads from one nobody does. + +**Guidance**: +An advisory note sent privately to the adults in a conversation that raised a +finding. It never reaches the student, and it never replaces the finding. + +## Reconciliation + +**Sweep**: +The scheduled pass that re-checks conditions and closes what it owns. + +**Backfill**: +The hourly re-walk of enrolled adults' message history, catching what predates +enrollment or was missed while the process was down. + +**Plan**: +The set of additions and removals that would bring a user group to an intended +membership. A plan is inspectable before it is applied, and is refused outright +when it would remove too much of a group at once. +_Avoid_: Diff, changeset, patch + +**Drift**: +A disagreement between what Slack declares and what the roster monitors. Drift +is reported, never silently reconciled in the direction that reduces monitoring. diff --git a/docker-compose.yml b/docker-compose.yml index 45613f3..a6e42bc 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -46,6 +46,9 @@ services: # is told to, and a missing one silently reads as "feature off". STUDENT_USERGROUP: ${STUDENT_USERGROUP:-} ADULT_USERGROUP: ${ADULT_USERGROUP:-} + # Further group handles `/hawkmod group` may edit, comma separated. The + # two above are always editable; this bounds how far a bad plan reaches. + MANAGED_USERGROUPS: ${MANAGED_USERGROUPS:-} # --- Schedules ------------------------------------------------------- TZ: ${TZ:-America/New_York} diff --git a/docs/policy-mapping.md b/docs/policy-mapping.md index 10ec6cb..e9da3a5 100644 --- a/docs/policy-mapping.md +++ b/docs/policy-mapping.md @@ -5,33 +5,33 @@ Each control from and what carries it. "Manual" means hawk-mod cannot do it and does not pretend to. -| § | Control | Carried by | -| --- | ----------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| 2 | Parental consent on file before a student account exists | `consent.ts`; `team_join` event raises `unconsented_account` the moment an account appears; nightly sweep re-checks | -| 2 | Consent re-collected annually | `CONSENT_VALID_YEARS = 1`; consents expire rather than linger | -| 2 | Consents kept on file and producible | `consents` table records `document_ref`; the signed copies themselves live wherever the team files them — **manual** | -| 2 | Parents notified of the PII collected and shared (Slack CSS §IV) | [`consent-form.md`](consent-form.md) — **manual**. The form is the artifact; hawk-mod records only that a version of it was signed (`form_version`) | -| 2 | Consent withdrawable on request | `revokeConsent()` exists in `repo.ts` but **has no caller** — no CLI, no modal. The form promises this; the tool cannot yet do it | -| 3 | Two YPP-screened Lead Coaches | `workspace_config` `screened_admins`: two of the workspace's Owners/Admins must be screened adults on the roster, plus `screening_lapsed` findings | -| 3 | Written communications copied to a second adult | `dmPolicy` — two screened adults required in any student conversation | -| 4.1 | No 1:1 adult–student DMs, ever | `dmPolicy` `one_to_one_adult_student`, raised on each new message and on backfill; a message after a finding is closed raises it again (`recurrence.ts`) | -| 4.2 | Two screened adults in every channel students are in | `twoAdults.ts`; re-evaluated on every join/leave, plus nightly | -| 4.3 | Students and parents told DMs are subject to audit | [`consent-form.md`](consent-form.md) — **manual**, and a precondition of deploying this. Both halves: the guardian signature block and the student acknowledgment below it | -| 4.4 | Quarterly export actually run and spot-checked | continuous instead: DMs are recorded as they happen. The quarterly reminder and runbook keep the human review honest | -| 5 | Business+ / Corporate Export | procurement — **manual**. Note hawk-mod does not depend on Corporate Export; it is the backstop for adults who never enroll | -| 6 | Retention | append-only while retained: deletions are tombstoned, edits keep prior text. No longer "keep everything" — `consent-form.md` promises deletion two years after a student's last day, and **nothing purges yet** | -| 6 | Slack Connect external DMs disabled | workspace setting, not API-readable — **manual** | -| 6 | Invites restricted to Owners/Admins | workspace setting — **manual**; `unknown_account` catches the consequence | -| 6 | Two Workspace Owners minimum, never a student | `workspace_config` findings | -| 6 | User group editing restricted to Owners/Admins | workspace setting — **manual**. Only matters when `STUDENT_USERGROUP`/`ADULT_USERGROUP` are set, since group membership then declares who is monitored | -| 6 | Real names enforced | workspace setting — **manual** | -| 6 | Huddles off | no API to observe huddles — **manual**, and the reason it matters is in the README's gap list | -| 7 | Youth Protection Training annually (the part FIRST requires for clearance) | `screening.ts`, `YPT_VALID_YEARS = 1` | -| 7 | CORI + national fingerprints every 3 years (M.G.L. c. 71 §38R, 603 CMR 51.00) | `screening.ts`, `CORI_VALID_YEARS = 3` | -| 8 | MPS administrator added to the workspace | roster role `district_observer`; counts as an adult only with screening dates recorded. Voluntary — IJNDD does not require it (see the correction below) | -| 8 | Employee-mentor keeps to channels, no student DMs (IJNDD clause k) | **manual**, and deliberately so: the employee manual binds that one person more tightly than §4.1 does, and hawk-mod is not the enforcer of it | -| 8 | Public-records retention for the employee-mentor (M.G.L. c. 66 §10) | substantively covered — messages are retained and producible via `export-conversation`; IJNDD's forward-to-school-e-mail expectation is **manual** | -| 8 | Written approval from the principal before launch | **manual**, and blocking | +| § | Control | Carried by | +| --- | ----------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| 2 | Parental consent on file before a student account exists | `consent.ts`; `team_join` event raises `unconsented_account` the moment an account appears; nightly sweep re-checks | +| 2 | Consent re-collected annually | `CONSENT_VALID_YEARS = 1`; consents expire rather than linger | +| 2 | Consents kept on file and producible | `consents` table records `document_ref`; the signed copies themselves live wherever the team files them — **manual** | +| 2 | Parents notified of the PII collected and shared (Slack CSS §IV) | [`consent-form.md`](consent-form.md) — **manual**. The form is the artifact; hawk-mod records only that a version of it was signed (`form_version`) | +| 2 | Consent withdrawable on request | `revokeConsent()` exists in `repo.ts` but **has no caller** — no CLI, no modal. The form promises this; the tool cannot yet do it | +| 3 | Two YPP-screened Lead Coaches | `workspace_config` `screened_admins`: two of the workspace's Owners/Admins must be screened adults on the roster, plus `screening_lapsed` findings | +| 3 | Written communications copied to a second adult | `dmPolicy` — two screened adults required in any student conversation | +| 4.1 | No 1:1 adult–student DMs, ever | `dmPolicy` `one_to_one_adult_student`, raised on each new message and on backfill; a message after a finding is closed raises it again (`recurrence.ts`) | +| 4.2 | Two screened adults in every channel students are in | `twoAdults.ts`; re-evaluated on every join/leave, plus nightly | +| 4.3 | Students and parents told DMs are subject to audit | [`consent-form.md`](consent-form.md) — **manual**, and a precondition of deploying this. Both halves: the guardian signature block and the student acknowledgment below it | +| 4.4 | Quarterly export actually run and spot-checked | continuous instead: DMs are recorded as they happen. The quarterly reminder and runbook keep the human review honest | +| 5 | Business+ / Corporate Export | procurement — **manual**. Note hawk-mod does not depend on Corporate Export; it is the backstop for adults who never enroll | +| 6 | Retention | append-only while retained: deletions are tombstoned, edits keep prior text. No longer "keep everything" — `consent-form.md` promises deletion two years after a student's last day, and **nothing purges yet** | +| 6 | Slack Connect external DMs disabled | workspace setting, not API-readable — **manual** | +| 6 | Invites restricted to Owners/Admins | workspace setting — **manual**; `unknown_account` catches the consequence | +| 6 | Two Workspace Owners minimum, never a student | `workspace_config` findings | +| 6 | User group editing restricted to Owners/Admins | workspace setting — **manual**, and now load-bearing for more than itself: Slack accepts a _bot_ token for `usergroups.users.update` only when editing is open to everyone, so keeping this restricted is what forces `slack/groupAdmin.ts` to write as a named administrator. `/hawkmod group` is gated on `slack/authz.ts` — the same Owner/Admin population, plus a refusal for students | +| 6 | Real names enforced | workspace setting — **manual** | +| 6 | Huddles off | no API to observe huddles — **manual**, and the reason it matters is in the README's gap list | +| 7 | Youth Protection Training annually (the part FIRST requires for clearance) | `screening.ts`, `YPT_VALID_YEARS = 1` | +| 7 | CORI + national fingerprints every 3 years (M.G.L. c. 71 §38R, 603 CMR 51.00) | `screening.ts`, `CORI_VALID_YEARS = 3` | +| 8 | MPS administrator added to the workspace | roster role `district_observer`; counts as an adult only with screening dates recorded. Voluntary — IJNDD does not require it (see the correction below) | +| 8 | Employee-mentor keeps to channels, no student DMs (IJNDD clause k) | **manual**, and deliberately so: the employee manual binds that one person more tightly than §4.1 does, and hawk-mod is not the enforcer of it | +| 8 | Public-records retention for the employee-mentor (M.G.L. c. 66 §10) | substantively covered — messages are retained and producible via `export-conversation`; IJNDD's forward-to-school-e-mail expectation is **manual** | +| 8 | Written approval from the principal before launch | **manual**, and blocking | ## Corrections worth keeping straight diff --git a/docs/slack-app-manifest.yaml b/docs/slack-app-manifest.yaml index dae3d14..a8d00a1 100644 --- a/docs/slack-app-manifest.yaml +++ b/docs/slack-app-manifest.yaml @@ -22,7 +22,7 @@ features: - command: /hawkmod url: https://hawk-mod.example.org/slack/events description: Coverage, findings, and audits - usage_hint: status | findings | whois @user | resolve + usage_hint: status | findings | whois @user | group add @user @group should_escape: true oauth_config: @@ -42,14 +42,28 @@ oauth_config: - users:read - users:read.email - usergroups:read - # Granted per adult, by that adult. This is the only way to see DMs - # without Enterprise Grid. + # Two separate per-person grants live in this one list, and Slack has no + # way to say so: an authorization asks for whichever subset it needs. + # + # im:*/mpim:* — enrolment. Granted per adult, by that adult, at + # /slack/install. The only way to see DMs without + # Enterprise Grid. + # usergroups:write + # — group editing. Granted per administrator at + # /slack/authorize-groups, and asked for on its own so + # nobody hands over their DMs to manage the roster. A bot + # token cannot do this while user group editing is + # restricted to Owners/Admins, which §6 requires it to be. + # + # Both must be listed here or the matching authorization link is rejected + # by Slack before anyone sees a consent screen. user: - im:read - im:history - mpim:read - mpim:history - users:read + - usergroups:write settings: interactivity: diff --git a/migrations/0006_group_membership.sql b/migrations/0006_group_membership.sql new file mode 100644 index 0000000..6c1b199 --- /dev/null +++ b/migrations/0006_group_membership.sql @@ -0,0 +1,72 @@ +-- Group membership management: hawk-mod can now edit the Slack user groups it +-- reads, and needs somewhere to record who asked for each edit. +-- +-- Two changes, for two reasons. + +-- 1. A third kind of installation row. +-- +-- Slack refuses a *bot* token for usergroups.users.update unless the workspace +-- lets everyone edit user groups — which §6 forbids. So group edits ride on an +-- administrator's own user token, granted at a separate authorization that asks +-- for `usergroups:write` and nothing else. +-- +-- That token cannot share a row with an enrolled adult's DM token. Slack issues +-- one token per authorization carrying only the scopes granted at that moment, +-- and `installations` is keyed on (team, kind, slack_user_id) with an upsert — +-- so an administrator who is also an enrolled mentor would have their DM token +-- overwritten by their group-editing token, silently ending their monitoring +-- while /hawkmod status still reported them enrolled. A separate `kind` keeps +-- the two grants in separate rows with independent lifetimes. +-- +-- SQLite cannot alter a CHECK constraint, so the table is rebuilt. This is safe +-- here in a way it was not in 0005: nothing references `installations`, so +-- there are no cascades to fire and no rows anywhere else to lose. + +CREATE TABLE installations_new ( + id INTEGER PRIMARY KEY, + team_id TEXT NOT NULL, + enterprise_id TEXT, + slack_user_id TEXT NOT NULL, + kind TEXT NOT NULL CHECK (kind IN ('bot','user','admin')), + payload_enc TEXT NOT NULL, -- encrypted JSON Installation + scopes TEXT, + installed_at TEXT NOT NULL, + updated_at TEXT NOT NULL, + revoked_at TEXT, + UNIQUE (team_id, kind, slack_user_id) +); + +INSERT INTO installations_new + (id, team_id, enterprise_id, slack_user_id, kind, payload_enc, scopes, + installed_at, updated_at, revoked_at) +SELECT id, team_id, enterprise_id, slack_user_id, kind, payload_enc, scopes, + installed_at, updated_at, revoked_at +FROM installations; + +DROP TABLE installations; +ALTER TABLE installations_new RENAME TO installations; + +-- 2. The trail of who asked. +-- +-- A group edit and its effect on monitoring are different facts, and they need +-- different records. `role_changes` says what happened to someone's monitoring; +-- it is written by the user-group sync, which runs from a Slack event and has +-- no idea a human was involved. This table says who asked for what. +-- +-- It is also the only record of edits that change no role at all — which will +-- be most of them once subteams (@programming, @drive-team) are managed here. +CREATE TABLE group_changes ( + id INTEGER PRIMARY KEY, + usergroup_id TEXT NOT NULL, + handle TEXT NOT NULL, + action TEXT NOT NULL CHECK (action IN ('add','remove')), + subject TEXT NOT NULL, -- Slack id of the person added or removed + person_id INTEGER REFERENCES people(id) ON DELETE SET NULL, + actor TEXT NOT NULL, -- Slack id of the administrator who asked + actor_name TEXT NOT NULL, + reason TEXT, -- required when the edit reduces monitoring + source TEXT NOT NULL, -- 'command' | 'sheet_sync' + changed_at TEXT NOT NULL +); +CREATE INDEX group_changes_group_idx ON group_changes (usergroup_id, changed_at); +CREATE INDEX group_changes_subject_idx ON group_changes (subject, changed_at); diff --git a/src/config.ts b/src/config.ts index fac8fa5..3526f18 100644 --- a/src/config.ts +++ b/src/config.ts @@ -15,6 +15,16 @@ const schema = z.object({ // 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. + MANAGED_USERGROUPS: z.string().optional(), TZ: z.string().default("America/New_York"), SWEEP_CRON: z.string().default("0 3 * * *"), BACKFILL_CRON: z.string().default("15 * * * *"), @@ -52,3 +62,17 @@ 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 6f0931f..519cc06 100644 --- a/src/db/repo.ts +++ b/src/db/repo.ts @@ -142,6 +142,55 @@ export function setPersonRole(args: { })(); } +/** + * Starts or stops monitoring a person, and records who decided and why. + * + * Deactivation is the only operation in hawk-mod that makes it see less. There + * is no sweep, no sync and no rule that reaches it — a person does, by name, + * with a reason, which is why both are required rather than optional. + * + * The trail goes in `role_changes` with `from_role` and `to_role` equal. That + * reads oddly until you remember what the table is for: it is not a log of role + * strings, it is the answer to "who was monitored, when" — the only such answer + * that exists below Enterprise Grid. A deactivation changes that answer, so it + * belongs in the same place as the changes that alter someone's role. + */ +export function setPersonActive(args: { + personId: number; + active: boolean; + source: string; + actor: string; + reason: string; +}): void { + const now = nowIso(); + const person = personById(args.personId); + if (!person) throw new Error(`No person ${args.personId}`); + db().transaction(() => { + db() + .prepare("UPDATE people SET active = ?, updated_at = ? WHERE id = ?") + .run(args.active ? 1 : 0, now, args.personId); + db() + .prepare( + `INSERT INTO role_changes (person_id, slack_user_id, from_role, to_role, + source, detail, changed_at) + VALUES (?, ?, ?, ?, ?, ?, ?)` + ) + .run( + args.personId, + person.slack_user_id, + person.role, + person.role, + args.source, + JSON.stringify({ + active: args.active, + actor: args.actor, + reason: args.reason, + }), + now + ); + })(); +} + /** * Creates a roster row for a Slack account that is in a user group but was * never imported. Email may be absent, so the row is keyed on the Slack id — @@ -324,13 +373,89 @@ export function revokeConsent(consentId: number, on: string): void { .run(on, consentId); } +/* --------------------------------------------------------- group changes */ + +export type GroupChangeInput = { + usergroupId: string; + handle: string; + action: "add" | "remove"; + subject: string; + personId: number | null; + actor: string; + actorName: string; + reason: string | null; + source: string; +}; + +/** + * Records that somebody edited a user group. + * + * Separate from `role_changes` on purpose. That table is written by the + * user-group sync, which runs from a Slack event and cannot know a human was + * involved — by the time it fires, whoever ran the command is long gone. This + * one answers "who asked for this", and it is the only record of edits that + * change nobody's role at all, which most of them will be once subteams are + * managed here too. + */ +export function insertGroupChange(input: GroupChangeInput): void { + db() + .prepare( + `INSERT INTO group_changes (usergroup_id, handle, action, subject, + person_id, actor, actor_name, reason, + source, changed_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)` + ) + .run( + input.usergroupId, + input.handle, + input.action, + input.subject, + input.personId, + input.actor, + input.actorName, + input.reason, + input.source, + nowIso() + ); +} + +export function listGroupChanges(limit = 100) { + return db() + .prepare( + `SELECT * FROM group_changes ORDER BY changed_at DESC, id DESC LIMIT ?` + ) + .all(limit) as Array<{ + id: number; + usergroup_id: string; + handle: string; + action: "add" | "remove"; + subject: string; + person_id: number | null; + actor: string; + actor_name: string; + reason: string | null; + source: string; + changed_at: string; + }>; +} + /* ----------------------------------------------------------- installations */ +/** + * 'bot' — one per workspace; posts alerts and reads membership. + * 'user' — one per enrolled adult; their DM-reading token. + * 'admin' — one per administrator who authorized group editing. Separate from + * 'user' because Slack issues one token per authorization carrying + * only that authorization's scopes, so sharing a row would mean the + * second grant destroying the first. + */ +export type InstallationKind = "bot" | "user" | "admin"; + export type StoredInstallation = { teamId: string; enterpriseId: string | null; slackUserId: string; - kind: "bot" | "user"; + kind: InstallationKind; payload: Record; scopes: string | null; installedAt: string; @@ -341,7 +466,7 @@ type InstallationRow = { team_id: string; enterprise_id: string | null; slack_user_id: string; - kind: "bot" | "user"; + kind: InstallationKind; payload_enc: string; scopes: string | null; installed_at: string; @@ -351,14 +476,46 @@ type InstallationRow = { /** Bot installations share one row per team; '-' keeps the UNIQUE index honest. */ export const BOT_USER_KEY = "-"; +/** + * Scopes an enrolled adult's token must carry to be worth storing. Without + * these hawk-mod cannot read their DMs, which is the entire reason the row + * exists. + */ +const DM_SCOPES = ["im:history", "mpim:history"]; + +/** + * Thrown rather than swallowed: a caller about to destroy DM monitoring should + * fail loudly at the authorization, not succeed and go quiet for a month. + */ +export class ScopeDowngradeError extends Error {} + export function saveInstallation(args: { teamId: string; enterpriseId?: string | null; slackUserId: string; - kind: "bot" | "user"; + kind: InstallationKind; payload: unknown; scopes?: string | null; }): void { + // Belt and braces on the one failure nobody would notice. A 'user' row is + // what makes an adult's DMs visible; overwriting it with a token that cannot + // read DMs would end their monitoring while /hawkmod status still counted + // them as enrolled — an absent alert, not a wrong one. The routing in + // installStore should mean this never fires; it exists because if the + // routing ever breaks, nothing else in the system would tell us. + if (args.kind === "user") { + const granted = (args.scopes ?? "").split(",").filter(Boolean); + const missing = DM_SCOPES.filter((s) => !granted.includes(s)); + const existing = getInstallation(args.teamId, "user", args.slackUserId); + if (missing.length && existing && !existing.revokedAt) { + throw new ScopeDowngradeError( + `Refusing to replace ${args.slackUserId}'s enrolment token with one ` + + `missing ${missing.join(", ")}; that would silently end their DM ` + + `monitoring.` + ); + } + } + const now = nowIso(); db() .prepare( @@ -399,7 +556,7 @@ function hydrate(row: InstallationRow): StoredInstallation { export function getInstallation( teamId: string, - kind: "bot" | "user", + kind: InstallationKind, slackUserId: string ): StoredInstallation | undefined { const row = db() @@ -432,7 +589,7 @@ export function listUserInstallations( export function markInstallationRevoked( teamId: string, - kind: "bot" | "user", + kind: InstallationKind, slackUserId: string ): void { db() @@ -446,7 +603,7 @@ export function markInstallationRevoked( export function deleteInstallation( teamId: string, - kind: "bot" | "user", + kind: InstallationKind, slackUserId: string ): void { db() diff --git a/src/domain/rules/groupMembership.ts b/src/domain/rules/groupMembership.ts new file mode 100644 index 0000000..9c22628 --- /dev/null +++ b/src/domain/rules/groupMembership.ts @@ -0,0 +1,145 @@ +/** + * Plans an edit to a Slack user group's membership. + * + * Slack sells no add-one/remove-one endpoint: `usergroups.users.update` + * *replaces* the entire member list. So every edit is really "here is the + * complete new membership", and the interesting question is what a caller is + * about to overwrite. This module answers that question purely, from two sets, + * so the answer can be shown to a human before anything is applied. + * + * Adding one person to a group is the degenerate case — `desired` is `current` + * plus one — which is why the single-user command and the eventual + * spreadsheet-driven sync share this and not just a naming convention. + */ + +export type GroupPlan = { + /** Slack ids to be added, sorted. */ + add: string[]; + /** Slack ids to be removed, sorted. */ + remove: string[]; + /** Slack ids already correct, sorted. */ + unchanged: string[]; + /** The full membership to send to Slack if this plan is applied, sorted. */ + result: string[]; + /** + * Why this plan must not be applied, or `null` if it may be. A refusal is a + * property of the plan itself, so a caller cannot apply one by forgetting to + * check a separate flag. + */ + refusal: string | null; +}; + +export type PlanLimits = { + /** + * The largest share of a group a single plan may remove, as a fraction of + * current membership. Removing exactly one person is always allowed however + * small the group, since that is a person deliberately naming a person. + */ + maxRemovedFraction: number; +}; + +export const DEFAULT_LIMITS: PlanLimits = { maxRemovedFraction: 0.25 }; + +/** + * Diffs intended membership against actual, and refuses plans that look like + * an accident rather than an intention. + * + * The refusal exists because of what is on the other end of this: a Google + * Sheet. A shifted header row, a filtered view someone forgot to clear, a + * column of blanks — none of these read as errors, they read as "the roster is + * now these four people", and applying that empties @students. A bulk plan that + * removes most of a group is indistinguishable from a bad spreadsheet, so it is + * refused and shown to a human rather than applied and reported. + */ +export function planGroupMembership( + current: ReadonlySet, + desired: ReadonlySet, + limits: PlanLimits = DEFAULT_LIMITS +): GroupPlan { + const add = [...desired].filter((id) => !current.has(id)).sort(); + const remove = [...current].filter((id) => !desired.has(id)).sort(); + const unchanged = [...current].filter((id) => desired.has(id)).sort(); + const result = [...desired].sort(); + + return { + add, + remove, + unchanged, + result, + refusal: refuse(current, result, remove, limits), + }; +} + +function refuse( + current: ReadonlySet, + result: string[], + remove: string[], + limits: PlanLimits +): string | null { + if (result.length === 0) { + // Slack rejects an empty `users` list outright (`no_users_provided`) — a + // user group must keep at least one member. Saying so here turns an opaque + // API error into a sentence that explains the situation. + return current.size === 0 + ? "That group is empty and Slack will not accept an empty membership." + : "Slack does not allow a user group to be emptied. Remove the group " + + "itself in Slack if it is finished with."; + } + + if (remove.length <= 1 || current.size === 0) return null; + + const fraction = remove.length / current.size; + if (fraction > limits.maxRemovedFraction) { + return ( + `This would remove ${remove.length} of ${current.size} members ` + + `(${Math.round(fraction * 100)}%), which is more than a plan is allowed ` + + `to remove at once. Check the source of this change before applying it.` + ); + } + + return null; +} + +/** The plan for adding one person — what the slash command builds. */ +export function planAdd( + current: ReadonlySet, + slackId: string, + limits?: PlanLimits +): GroupPlan { + return planGroupMembership(current, new Set([...current, slackId]), limits); +} + +/** The plan for removing one person — what the slash command builds. */ +export function planRemove( + current: ReadonlySet, + slackId: string, + limits?: PlanLimits +): GroupPlan { + const desired = new Set(current); + desired.delete(slackId); + return planGroupMembership(current, desired, limits); +} + +/** + * Whether an edit ends someone's monitoring as a student, and so must carry a + * written reason. + * + * Pure, and keyed on the group's *handle* rather than whatever the caller + * typed. Slack sends an escaped mention as ``, so a + * command handler comparing the raw argument to a configured handle compares an + * opaque id to a word and quietly never matches — which would mean the one gate + * standing in front of the most consequential edit never fires. + */ +export function reducesMonitoring(args: { + action: "add" | "remove"; + subjectRole: string; + handle: string; + adultHandle: string; +}): boolean { + return ( + args.action === "add" && + args.subjectRole === "student" && + args.handle.replace(/^@/, "").toLowerCase() === + args.adultHandle.replace(/^@/, "").toLowerCase() + ); +} diff --git a/src/domain/rules/rosterSync.ts b/src/domain/rules/rosterSync.ts index 05644b8..c31e31a 100644 --- a/src/domain/rules/rosterSync.ts +++ b/src/domain/rules/rosterSync.ts @@ -11,6 +11,12 @@ export type RoleDecision = | { kind: "unchanged"; slackId: string } /** In a group but not on the roster — the row gets created from Slack. */ | { kind: "create"; slackId: string; role: Role } + /** + * Deactivated, but declared again by a group. Monitoring resumes: someone + * back in @students is a student again, and requiring a human to confirm + * that would leave a real student unmonitored until somebody got round to it. + */ + | { kind: "reactivate"; personId: number; slackId: string; role: Role } | { kind: "change"; personId: number; @@ -19,6 +25,8 @@ export type RoleDecision = to: Role; /** Moving out of `student` removes someone from monitoring. */ reducesProtection: boolean; + /** Deactivated when the group declared them; monitoring resumes too. */ + reactivates: boolean; } /** In both groups at once; too ambiguous to act on. */ | { kind: "conflict"; slackId: string; personId: number | null }; @@ -32,12 +40,18 @@ function isAdultRole(role: Role): boolean { /** * Reconciles Slack user groups against roster roles. * - * Deliberately one-directional in the safe sense: **membership of a group can - * only ever be added to the roster, never subtracted from it.** Someone dropped - * from the students group is left a student, because the alternative — silently - * ending their monitoring — is the failure this whole system exists to avoid. - * The only way out of `student` is an explicit move into the adults group, - * which is a deliberate admin action and is reported as one. + * Deliberately one-directional in the safe sense: **this reconciliation may only + * ever add monitoring, never subtract it.** Someone dropped from the students + * group is left a student, because the alternative — silently ending their + * monitoring — is the failure this whole system exists to avoid. The only way + * out of `student` is an explicit move into the adults group, which is a + * deliberate admin action and is reported as one. + * + * That invariant is about direction, not about writes. `reactivate` adds + * monitoring back to someone who was deactivated and has since been declared + * again, so it belongs here for the same reason `create` does: gaining + * protection never needs anyone's permission. Losing it always does — which is + * why there is no decision here that deactivates anybody. * * Pure: the caller does the Slack reads and the writes. */ @@ -71,13 +85,21 @@ export function reconcileRoles( // A district observer in the adults group is agreement, not a demotion to // plain `adult`. - if (target === "adult" && isAdultRole(person.role)) { - decisions.push({ kind: "unchanged", slackId }); - continue; - } + const roleAlreadyRight = + person.role === target || + (target === "adult" && isAdultRole(person.role)); - if (person.role === target) { - decisions.push({ kind: "unchanged", slackId }); + if (roleAlreadyRight) { + decisions.push( + person.active === 1 + ? { kind: "unchanged", slackId } + : { + kind: "reactivate", + personId: person.id, + slackId, + role: person.role, + } + ); continue; } @@ -88,6 +110,7 @@ export function reconcileRoles( from: person.role, to: target, reducesProtection: person.role === "student" && target !== "student", + reactivates: person.active !== 1, }); } diff --git a/src/jobs/syncRoles.ts b/src/jobs/syncRoles.ts index 45fdbfd..bf198f5 100644 --- a/src/jobs/syncRoles.ts +++ b/src/jobs/syncRoles.ts @@ -4,6 +4,7 @@ import { createPersonFromSlack, peopleBySlackId, personBySlackId, + setPersonActive, setPersonRole, } from "../db/repo.js"; import { dedupeKey } from "../domain/findings.js"; @@ -18,6 +19,7 @@ export type RoleSyncStats = { adultsInGroup: number; created: number; changed: number; + reactivated: number; conflicts: number; reducedProtection: number; missingGroups: number; @@ -28,15 +30,21 @@ const SOURCE = "usergroup_sync"; /** * Pulls role declarations from Slack user groups into the roster. * - * Membership is only ever *added*. Dropping someone from the students group + * Monitoring is only ever *added*. Dropping someone from the students group * does not un-student them — that would silently end their monitoring, which is * the failure this whole system exists to prevent. Moving out of `student` * requires putting them in the adults group, which is deliberate, and is - * reported as a finding either way. + * reported as a finding either way. Ending monitoring altogether is + * `/hawkmod deactivate`, which names a person and demands a reason. + * + * The same asymmetry runs the other way: a deactivated person who reappears in + * a group is reactivated here without ceremony, because gaining protection back + * never needs approval. * * Group editing is restricted to Workspace Admins in the workspace settings. - * That is not readable through any API, so it lives on the manual §6 checklist - * alongside retention and huddles. + * That is not readable through any API, so it stays on the manual §6 checklist + * alongside retention and huddles — and it is precisely why `slack/groupAdmin.ts` + * edits groups with an administrator's own token rather than the bot's. */ export async function syncRolesFromUserGroups( client: WebClient @@ -48,6 +56,7 @@ export async function syncRolesFromUserGroups( adultsInGroup: 0, created: 0, changed: 0, + reactivated: 0, conflicts: 0, reducedProtection: 0, missingGroups: 0, @@ -122,7 +131,36 @@ export async function syncRolesFromUserGroups( break; } + case "reactivate": { + // Declared again after being deactivated. Monitoring resumes without + // anyone's approval, for the same reason `create` needs none: this + // direction only ever adds protection. + setPersonActive({ + personId: decision.personId, + active: true, + source: SOURCE, + actor: SOURCE, + reason: "declared by a Slack user group", + }); + stats.reactivated += 1; + log.info("monitoring resumed from user group", { + user: decision.slackId, + role: decision.role, + }); + break; + } + case "change": { + if (decision.reactivates) { + setPersonActive({ + personId: decision.personId, + active: true, + source: SOURCE, + actor: SOURCE, + reason: "declared by a Slack user group", + }); + stats.reactivated += 1; + } setPersonRole({ personId: decision.personId, toRole: decision.to, diff --git a/src/slack/app.ts b/src/slack/app.ts index c92369f..ae743a3 100644 --- a/src/slack/app.ts +++ b/src/slack/app.ts @@ -1,4 +1,4 @@ -import { App } from "@slack/bolt"; +import { App, type InstallURLOptions } from "@slack/bolt"; import { APP_NAME, BRAND, ICON_SVG } from "../brand.js"; import { config } from "../config.js"; import { healthHandler } from "../health.js"; @@ -7,7 +7,7 @@ import { registerActions } from "./actions.js"; import { registerCommands } from "./commands.js"; import { registerEvents } from "./events.js"; import { registerViews } from "./modals.js"; -import { installationStore } from "./installStore.js"; +import { GROUP_ADMIN_METADATA, installationStore } from "./installStore.js"; /** Read-only apart from posting alerts. hawk-mod never needs to act as a user. */ export const BOT_SCOPES = [ @@ -41,9 +41,73 @@ export const USER_SCOPES = [ "users:read", ]; +/** + * Scopes for the group-editing grant, and deliberately nothing else. + * + * Slack issues one token per authorization carrying only what that + * authorization asked for, so this list is exactly what an administrator hands + * over: permission to edit user groups. It asks for no DM access, because an + * administrator is not necessarily a monitored mentor and the two consents + * should never be bundled — enrolment is a deliberate, named act, and burying + * it inside "let me edit user groups" would be a trick. + */ +export const GROUP_ADMIN_USER_SCOPES = ["usergroups:write"]; + export function createApp(): App { const cfg = config(); + /** + * Bolt builds an `InstallProvider` inside its receiver but does not put + * either on `App`'s public type, so reaching the URL generator needs this + * shape. `receiver` is private, hence the double assertion; keeping the + * target structural rather than `any` still means a Bolt upgrade that changes + * `generateInstallUrl` breaks here at compile time rather than at an + * administrator's first click. + */ + type WithInstaller = { + receiver?: { + installer?: { + generateInstallUrl(options: InstallURLOptions): Promise; + }; + }; + }; + + // Assigned immediately after construction; the route handler below only runs + // once a request arrives, long after that. + let self: App; + + /** + * Sends an administrator to Slack to grant group-editing permission. + * + * A second authorization route rather than a wider `/slack/install`, because + * the two grants must land in different rows. `metadata` is what tells the + * callback which one came back. + */ + const authorizeGroups = async ( + _req: import("http").IncomingMessage, + res: import("http").ServerResponse + ): Promise => { + try { + const installer = (self as unknown as WithInstaller).receiver?.installer; + if (!installer) throw new Error("no install provider on the receiver"); + const url = await installer.generateInstallUrl({ + // No bot scopes: this authorization adds a permission to one person's + // token and must not re-grant or alter the workspace installation. + scopes: [], + userScopes: GROUP_ADMIN_USER_SCOPES, + metadata: GROUP_ADMIN_METADATA, + }); + res.writeHead(302, { location: url }); + res.end(); + } catch (err) { + log.error("could not build group authorization url", { + error: String(err), + }); + res.writeHead(500, { "content-type": "text/plain; charset=utf-8" }); + res.end("Could not start authorization. Tell a coach."); + } + }; + const app = new App({ signingSecret: cfg.SLACK_SIGNING_SECRET, clientId: cfg.SLACK_CLIENT_ID, @@ -53,6 +117,11 @@ export function createApp(): App { installationStore, customRoutes: [ { path: "/health", method: ["GET"], handler: healthHandler }, + { + path: "/slack/authorize-groups", + method: ["GET"], + handler: authorizeGroups, + }, ], redirectUri: `${cfg.PUBLIC_URL}/slack/oauth_redirect`, installerOptions: { @@ -61,10 +130,27 @@ export function createApp(): App { installPath: "/slack/install", directInstall: true, callbackOptions: { - success: (_installation, _options, _req, res) => { + success: (installation, _options, _req, res) => { + const groupAdmin = installation.metadata === GROUP_ADMIN_METADATA; res.writeHead(200, { "content-type": "text/html; charset=utf-8" }); res.end( - ` + groupAdmin + ? ` + + ${APP_NAME} +
+ ${ICON_SVG} +

You can manage user groups.

+

${APP_NAME}

+

${APP_NAME} can now edit Slack user groups on your behalf when + you run /hawkmod group. Changes will show in Slack as + made by you, which is the point — every roster change has a + name against it.

+

This grants no access to your messages. If you are also an + enrolled mentor, that is a separate permission and this has not + touched it.

+
` + : ` ${APP_NAME}
@@ -89,6 +175,8 @@ export function createApp(): App { }, }); + self = app; + registerEvents(app); registerCommands(app); registerViews(app); diff --git a/src/slack/commands.ts b/src/slack/commands.ts index d29c4eb..d301a21 100644 --- a/src/slack/commands.ts +++ b/src/slack/commands.ts @@ -11,7 +11,9 @@ import { listFindings, listPeople, personByEmail, + personById, personBySlackId, + setPersonActive, } from "../db/repo.js"; import { today } from "../domain/dates.js"; import { severityEmoji } from "../domain/findings.js"; @@ -20,7 +22,8 @@ import { consentStatus } from "../domain/rules/consent.js"; import { screeningStatus } from "../domain/rules/screening.js"; import { log } from "../logger.js"; import { backfillAll } from "../monitor/backfill.js"; -import { administrator, NOT_PERMITTED } from "./authz.js"; +import { administrator, type Actor, NOT_PERMITTED } from "./authz.js"; +import { applyGroupEdit } from "./groupAdmin.js"; import { openConsent, openScreening } from "./modals.js"; import { runSweep } from "../jobs/sweep.js"; import { syncRolesFromUserGroups } from "../jobs/syncRoles.js"; @@ -31,6 +34,9 @@ const HELP = [ "`/hawkmod enroll` — link for a adult to authorize monitoring", "`/hawkmod findings [kind]` — open findings", "`/hawkmod whois @user` — role, consent, screening, enrollment", + "`/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 screening @user` — record YPP / Mentor Ready / CORI dates", "`/hawkmod consent @user` — record a signed parental consent", "`/hawkmod ack ` — acknowledge without closing", @@ -145,6 +151,22 @@ export function registerCommands(app: App): void { return; } + case "group": { + await respond({ + response_type: "ephemeral", + text: await groupText(client, teamId, caller, rest), + }); + return; + } + + case "deactivate": { + await respond({ + response_type: "ephemeral", + text: await deactivateText(client, caller, rest), + }); + return; + } + case "sync": { const stats = await syncRolesFromUserGroups(client); await respond({ @@ -319,3 +341,151 @@ async function whoisText( return lines.join("\n"); } + +/** + * Pulls the group out of a mention. With link escaping on, Slack sends a user + * group as ``; with it off, the raw `@students`. Both + * reach `resolveGroup`, which accepts an id or a handle. + */ +function groupRef(raw: string): string | null { + const trimmed = raw.trim(); + if (!trimmed) return null; + const escaped = trimmed.match(/^ { + const [action, mention, group, ...reasonWords] = rest; + if (action !== "add" && action !== "remove") { + return "Usage: `/hawkmod group add|remove @user @group`."; + } + const ref = groupRef(group ?? ""); + if (!mention || !ref) { + return `Usage: \`/hawkmod group ${action} @user @group\`.`; + } + + const person = await resolvePerson(client, mention); + if (!person) { + 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.` + ); + } + + const reason = reasonWords.join(" ").trim(); + + const outcome = await applyGroupEdit({ + teamId, + actor: caller, + groupRef: ref, + action, + subject: person, + reason: reason || null, + source: "command", + }); + + if (!outcome.ok) { + // The group's handle is only known once it has been resolved, so the + // needs-a-reason refusal comes back from there and is dressed up here, + // where the caller's own words are still to hand. + if ("needsReason" in outcome) { + return ( + `*${person.full_name}* is a student. ${outcome.reason}\n` + + `\`/hawkmod group add ${mention} ${group} \`` + ); + } + return outcome.reason; + } + if (outcome.noop) { + return `*${person.full_name}* was already ${ + action === "add" ? "in" : "out of" + } @${outcome.handle}. Nothing changed.`; + } + + const lines = [ + `${action === "add" ? "Added" : "Removed"} *${person.full_name}* ` + + `${action === "add" ? "to" : "from"} @${outcome.handle}.`, + ]; + + // 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") { + lines.push( + `_${person.full_name} is still a ${person.role} on the roster and still ` + + `monitored. Leaving a group never ends monitoring — use ` + + `\`/hawkmod deactivate\` for that._` + ); + } + if (outcome.reducedMonitoring) { + lines.push( + `_${person.full_name} is no longer monitored as a student. Recorded ` + + `against your name: ${reason}_` + ); + } + return lines.join("\n"); +} + +/** + * `/hawkmod deactivate @user ` — the only thing here that makes hawk-mod + * see less, which is why it names a person and demands a reason in the same + * breath, exactly as `ack` and `resolve` do. + */ +async function deactivateText( + client: WebClient, + caller: Actor, + rest: string[] +): Promise { + const [mention, ...reasonWords] = rest; + const reason = reasonWords.join(" ").trim(); + if (!mention || !reason) { + return ( + "Usage: `/hawkmod deactivate @user ` — the reason is required.\n" + + "This is the one command that stops hawk-mod watching somebody." + ); + } + + const person = await resolvePerson(client, mention); + if (!person) return `No roster entry for \`${mention}\`.`; + if (person.active !== 1) { + return `*${person.full_name}* is already deactivated.`; + } + + setPersonActive({ + personId: person.id, + active: false, + source: "command", + actor: caller.name, + reason, + }); + + const after = personById(person.id); + const lines = [ + `*${person.full_name}* is no longer monitored. Recorded against your name, ` + + `with the reason you gave.`, + ]; + if (after?.role === "student") { + lines.push( + `_This was a student. Their recorded messages are kept; nothing new will ` + + `be recorded. Adding them back to a role user group resumes monitoring._` + ); + } + return lines.join("\n"); +} diff --git a/src/slack/groupAdmin.ts b/src/slack/groupAdmin.ts new file mode 100644 index 0000000..60d5a99 --- /dev/null +++ b/src/slack/groupAdmin.ts @@ -0,0 +1,217 @@ +import { WebClient } from "@slack/web-api"; +import { config, managedGroupHandles } from "../config.js"; +import { getInstallation, insertGroupChange } from "../db/repo.js"; +import type { Person } from "../domain/people.js"; +import { + planAdd, + planRemove, + reducesMonitoring, + type GroupPlan, +} from "../domain/rules/groupMembership.js"; +import { log } from "../logger.js"; +import type { Actor } from "./authz.js"; +import { resolveGroup, setGroupMembership } from "./userGroups.js"; + +export type GroupEditOutcome = + | { + ok: true; + plan: GroupPlan; + handle: string; + noop: boolean; + /** This edit ended someone's monitoring as a student. */ + reducedMonitoring: boolean; + } + | { ok: false; reason: string; needsAuthorization?: boolean } + /** + * Refused for want of a reason. Raised here rather than in the command + * handler because only this side knows the group's handle: an escaped + * mention arrives as an opaque id. + */ + | { ok: false; needsReason: true; handle: string; reason: string }; + +/** + * Serializes group writes within this process. + * + * Every edit is a read-modify-write against an endpoint that replaces the whole + * member list, so two overlapping edits lose one of them. hawk-mod is a single + * container, so one lock genuinely closes the door on hawk-mod racing itself — + * a command running while an event-driven resync or a sheet sync is in flight. + * + * It cannot close the door on hawk-mod racing a human in Slack's own UI; Slack + * offers no compare-and-swap on this endpoint. Re-reading membership inside the + * lock, immediately before writing, keeps that window to about one round trip. + */ +let queue: Promise = Promise.resolve(); + +function serialize(fn: () => Promise): Promise { + const run = queue.then(fn, fn); + queue = run.catch(() => undefined); + return run; +} + +/** + * The administrator's own Slack client for group edits. + * + * Not the bot's. Slack accepts a bot token for `usergroups.users.update` only + * when the workspace lets everyone edit user groups, which §6 forbids — so the + * write goes out as the administrator who asked for it, which also means Slack + * attributes the change to a real person. + */ +function adminClient(teamId: string, slackUserId: string): WebClient | null { + const row = getInstallation(teamId, "admin", slackUserId); + if (!row || row.revokedAt) return null; + const payload = row.payload as { user?: { token?: string } }; + const token = payload.user?.token; + return token ? new WebClient(token) : null; +} + +/** Where an administrator goes to grant group-editing permission. */ +export function authorizeUrl(): string { + return `${config().PUBLIC_URL}/slack/authorize-groups`; +} + +export type GroupEditRequest = { + teamId: string; + actor: Actor; + /** `@handle`, a raw handle, or a Slack group id from an escaped mention. */ + groupRef: string; + action: "add" | "remove"; + subject: Person | { slackUserId: string }; + /** Required only when the edit reduces someone's monitoring. */ + reason: string | null; + source: "command" | "sheet_sync"; +}; + +function subjectId(s: GroupEditRequest["subject"]): string { + return "id" in s ? (s.slack_user_id ?? "") : s.slackUserId; +} + +function subjectPersonId(s: GroupEditRequest["subject"]): number | null { + return "id" in s ? s.id : null; +} + +/** + * Applies one membership edit, or explains why it did not. + * + * Deliberately does not touch the roster. Slack fires `subteam_members_changed` + * on a successful write, and the existing sync reconciles from that — so there + * is still exactly one path from a group to a role, whether the group was + * edited here or by hand in Slack. Writing the roster here as well would give + * that fact two doors, and they would disagree the first time one of them + * failed. + */ +export async function applyGroupEdit( + req: GroupEditRequest +): Promise { + const target = subjectId(req.subject); + if (!target) { + return { ok: false, reason: "That person has no linked Slack account." }; + } + + const client = adminClient(req.teamId, req.actor.slackUserId); + if (!client) { + return { + ok: false, + needsAuthorization: true, + reason: + `hawk-mod needs your permission to edit user groups on your behalf. ` + + `Slack will not let it do this as itself while group editing is ` + + `restricted to admins, which is the correct setting.\n` + + `Authorize once here: ${authorizeUrl()}`, + }; + } + + return serialize(async () => { + // Read inside the lock so the plan reflects membership as it is now, not + // as it was when the command was typed. + const group = await resolveGroup(client, req.groupRef); + if (!group) { + return { + ok: false as const, + reason: `No user group \`${req.groupRef}\`.`, + }; + } + + if (!managedGroupHandles().has(group.handle.toLowerCase())) { + return { + ok: false as const, + reason: + `hawk-mod is not configured to edit @${group.handle}. Add it to ` + + `MANAGED_USERGROUPS if it should be manageable here, or edit it in ` + + `Slack directly.`, + }; + } + + // Moving a student into the adults 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. + const reduces = + "role" in req.subject && + reducesMonitoring({ + action: req.action, + subjectRole: req.subject.role, + handle: group.handle, + adultHandle: config().ADULT_USERGROUP ?? "adults", + }); + + if (reduces && !req.reason) { + return { + ok: false as const, + needsReason: true as const, + handle: group.handle, + reason: + `Adding a student to @${group.handle} ends their monitoring as a ` + + `student, so this one needs a reason.`, + }; + } + + const plan = + req.action === "add" + ? planAdd(group.members, target) + : planRemove(group.members, target); + + if (plan.refusal) { + return { ok: false as const, reason: plan.refusal }; + } + + if (plan.add.length === 0 && plan.remove.length === 0) { + return { + ok: true as const, + plan, + handle: group.handle, + noop: true, + reducedMonitoring: false, + }; + } + + await setGroupMembership(client, group.id, plan.result); + + insertGroupChange({ + usergroupId: group.id, + handle: group.handle, + action: req.action, + subject: target, + personId: subjectPersonId(req.subject), + actor: req.actor.slackUserId, + actorName: req.actor.name, + reason: req.reason, + source: req.source, + }); + + log.info("user group edited", { + handle: group.handle, + action: req.action, + subject: target, + actor: req.actor.slackUserId, + }); + + return { + ok: true as const, + plan, + handle: group.handle, + noop: false, + reducedMonitoring: reduces, + }; + }); +} diff --git a/src/slack/installStore.ts b/src/slack/installStore.ts index 786fe1c..a5ebc5b 100644 --- a/src/slack/installStore.ts +++ b/src/slack/installStore.ts @@ -28,16 +28,30 @@ function teamKey( return id; } +/** Marks an authorization that came from the group-editing flow. */ +export const GROUP_ADMIN_METADATA = "group-admin"; + /** - * Two kinds of row live here: + * Three kinds of row live here: + * + * 'bot' — one per workspace, installed once by a workspace admin. Posts alerts. + * 'user' — one per enrolled adult, holding that adult's user token. This is + * what lets hawk-mod see DMs at all; Slack exposes no other way to + * read them below Enterprise Grid. + * 'admin' — one per administrator who authorized group editing, holding a + * token scoped to `usergroups:write` and nothing else. * - * 'bot' — one per workspace, installed once by a workspace admin. Posts alerts. - * 'user' — one per enrolled adult, holding that adult's user token. This is - * what lets hawk-mod see DMs at all; Slack exposes no other way to - * read them below Enterprise Grid. + * A fetch merges bot and user so a single event can carry both a bot token (to + * reply) and the observing adult's user token (to read the conversation). + * 'admin' rows are deliberately *not* part of that merge: they are fetched by + * name, by the one code path that edits groups, and never ride along on + * anything else. * - * A fetch merges the two so a single event can carry both a bot token (to reply) - * and the observing adult's user token (to read the conversation). + * The reason 'admin' is a separate row rather than extra scopes on 'user' is + * that Slack issues one token per authorization carrying only the scopes + * granted at that moment. An administrator who is also an enrolled mentor would + * otherwise have their DM token overwritten the first time they authorized + * group editing, ending their monitoring while coverage still read 100%. */ export const installationStore: InstallationStore = { async storeInstallation(installation) { @@ -56,6 +70,25 @@ export const installationStore: InstallationStore = { } if (installation.user?.token) { + // The group-editing grant is a different consent for a different purpose + // and gets its own row. Critically it must not fall through to the + // enrolment branch below, which would overwrite a mentor's DM token. + if (installation.metadata === GROUP_ADMIN_METADATA) { + saveInstallation({ + teamId, + enterpriseId, + slackUserId: installation.user.id, + kind: "admin", + payload: installation, + scopes: (installation.user.scopes ?? []).join(","), + }); + log.info("administrator authorized group editing", { + user: installation.user.id, + teamId, + }); + return; + } + // Enrolling grants hawk-mod that account's DM history. For a student // that would expose student-to-student conversations, which are // deliberately never recorded — so the enrolment is refused outright diff --git a/src/slack/userGroups.ts b/src/slack/userGroups.ts index ffd0955..0a94367 100644 --- a/src/slack/userGroups.ts +++ b/src/slack/userGroups.ts @@ -8,24 +8,35 @@ export type ResolvedGroup = { members: Set; }; +/** Slack ids for user groups are `S` followed by uppercase alphanumerics. */ +const GROUP_ID = /^S[A-Z0-9]{4,}$/; + +export function isGroupId(ref: string): boolean { + return GROUP_ID.test(ref.toUpperCase()); +} + /** - * Resolves a user group by its @handle. Handles are what people actually type - * and see, so they are what the config names; ids are opaque. + * Resolves a user group by its @handle or its id. Handles are what people + * actually type and see, so they are what the config names; ids are opaque — + * but an id is what Slack sends when a slash command has link escaping on and + * somebody types `@students`, which arrives as ``. * * Requires the `usergroups:read` bot scope, and User Groups themselves require * a Standard/Business+ plan — they do not exist on the free tier. */ export async function resolveGroup( client: WebClient, - handle: string + ref: string ): Promise { - const wanted = handle.replace(/^@/, "").toLowerCase(); + const raw = ref.replace(/^@/, ""); + const wanted = raw.toLowerCase(); + const byId = isGroupId(raw) ? raw.toUpperCase() : null; const list = await client.usergroups.list({ include_disabled: false }); - const group = (list.usergroups ?? []).find( - (g) => (g.handle ?? "").toLowerCase() === wanted + const group = (list.usergroups ?? []).find((g) => + byId ? g.id === byId : (g.handle ?? "").toLowerCase() === wanted ); if (!group?.id) { - log.warn("user group not found", { handle }); + log.warn("user group not found", { ref }); return null; } @@ -66,3 +77,32 @@ export async function fetchProfiles( } return out; } + +/** + * Replaces a user group's membership. + * + * `usergroups.users.update` takes the *complete* new member list — Slack sells + * no add-one or remove-one endpoint — which is why every caller goes through a + * plan rather than mutating a set in place. + * + * This must be called with an administrator's user token, not the bot token. + * Slack accepts a bot token here only when the workspace lets *everyone* edit + * user groups, and §6 requires that be restricted to Owners and Admins. The + * restriction is the point, so the token is what changes. + */ +export async function setGroupMembership( + client: WebClient, + usergroupId: string, + userIds: readonly string[] +): Promise { + if (userIds.length === 0) { + throw new Error( + "Refusing to send an empty membership; Slack rejects it and a plan " + + "should have caught this." + ); + } + await client.usergroups.users.update({ + usergroup: usergroupId, + users: userIds.join(","), + }); +} diff --git a/test/groupMembership.test.ts b/test/groupMembership.test.ts new file mode 100644 index 0000000..fe6161e --- /dev/null +++ b/test/groupMembership.test.ts @@ -0,0 +1,182 @@ +import assert from "node:assert/strict"; +import { describe, it } from "node:test"; +import { + planAdd, + planGroupMembership, + planRemove, + reducesMonitoring, +} from "../src/domain/rules/groupMembership.js"; + +const set = (...ids: string[]) => new Set(ids); + +describe("group membership plans", () => { + it("adds one person without disturbing the rest", () => { + const plan = planAdd(set("U1", "U2"), "U3"); + assert.deepEqual(plan.add, ["U3"]); + assert.deepEqual(plan.remove, []); + assert.deepEqual(plan.result, ["U1", "U2", "U3"]); + assert.equal(plan.refusal, null); + }); + + it("treats adding someone already present as a no-op", () => { + const plan = planAdd(set("U1", "U2"), "U2"); + assert.deepEqual(plan.add, []); + assert.deepEqual(plan.remove, []); + assert.equal(plan.refusal, null); + }); + + it("removes one person however small the group", () => { + // A person naming a person. Never refused for being proportionally large, + // or removing someone from a group of three would be impossible. + const plan = planRemove(set("U1", "U2", "U3"), "U2"); + assert.deepEqual(plan.remove, ["U2"]); + assert.deepEqual(plan.result, ["U1", "U3"]); + assert.equal(plan.refusal, null); + }); + + it("refuses to empty a group, because Slack rejects it anyway", () => { + const plan = planRemove(set("U1"), "U1"); + assert.match( + plan.refusal ?? "", + /does not allow a user group to be emptied/ + ); + }); + + it("refuses a bulk plan that removes most of a group", () => { + // The shape of a bad spreadsheet: a handful of survivors, everyone else + // silently dropped. + const current = set("U1", "U2", "U3", "U4", "U5", "U6", "U7", "U8"); + const plan = planGroupMembership(current, set("U1", "U2")); + assert.deepEqual(plan.remove, ["U3", "U4", "U5", "U6", "U7", "U8"]); + assert.match(plan.refusal ?? "", /would remove 6 of 8 members \(75%\)/); + }); + + it("allows a bulk plan within the removal limit", () => { + const current = set("U1", "U2", "U3", "U4", "U5", "U6", "U7", "U8"); + const plan = planGroupMembership( + current, + set("U1", "U2", "U3", "U4", "U5", "U6") + ); + assert.deepEqual(plan.remove, ["U7", "U8"]); + assert.equal(plan.refusal, null); + }); + + it("counts additions and removals in the same plan", () => { + const plan = planGroupMembership( + set("U1", "U2", "U3", "U4"), + set("U1", "U2", "U3", "U9") + ); + assert.deepEqual(plan.add, ["U9"]); + assert.deepEqual(plan.remove, ["U4"]); + assert.deepEqual(plan.unchanged, ["U1", "U2", "U3"]); + assert.equal(plan.refusal, null); + }); + + it("carries the refusal on the plan rather than beside it", () => { + // A caller cannot apply a refused plan by forgetting to check a flag + // somewhere else; the refusal travels with the thing being applied. + const plan = planRemove(set("U1"), "U1"); + assert.ok(plan.refusal); + assert.deepEqual(plan.result, []); + }); + + it("respects a caller-supplied removal limit", () => { + const current = set("U1", "U2", "U3", "U4"); + const strict = planGroupMembership(current, set("U1", "U2"), { + maxRemovedFraction: 0.1, + }); + assert.ok(strict.refusal); + const loose = planGroupMembership(current, set("U1", "U2"), { + maxRemovedFraction: 0.9, + }); + assert.equal(loose.refusal, null); + }); +}); + +/** + * The gate in front of the single most consequential edit hawk-mod can make. + * 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"; + + it("flags a student being added to the adults group", () => { + assert.equal( + reducesMonitoring({ + action: "add", + subjectRole: "student", + handle: "adults", + adultHandle: adults, + }), + true + ); + }); + + it("ignores an adult being added to the adults group", () => { + assert.equal( + reducesMonitoring({ + action: "add", + subjectRole: "adult", + handle: "adults", + adultHandle: adults, + }), + false + ); + }); + + it("ignores a student being added to any other group", () => { + assert.equal( + reducesMonitoring({ + action: "add", + subjectRole: "student", + handle: "programming", + adultHandle: adults, + }), + false + ); + }); + + it("ignores removals, which never end monitoring", () => { + assert.equal( + reducesMonitoring({ + action: "remove", + subjectRole: "student", + handle: "adults", + adultHandle: adults, + }), + false + ); + }); + + it("matches regardless of @ prefix or case on either side", () => { + assert.equal( + reducesMonitoring({ + action: "add", + subjectRole: "student", + handle: "Adults", + adultHandle: "@adults", + }), + true + ); + }); + + /** + * 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. + * Only the resolved handle is a safe input here. + */ + it("is not fooled by a group id, because it never sees one", () => { + assert.equal( + reducesMonitoring({ + action: "add", + subjectRole: "student", + handle: "S0614TY5A", + adultHandle: adults, + }), + false + ); + }); +}); diff --git a/test/rosterSync.test.ts b/test/rosterSync.test.ts index 7e6ec5f..08be616 100644 --- a/test/rosterSync.test.ts +++ b/test/rosterSync.test.ts @@ -24,6 +24,11 @@ function person(slackId: string, role: Role): Person { }; } +/** Someone the roster remembers but no longer monitors. */ +function deactivated(p: Person): Person { + return { ...p, active: 0 }; +} + function roster(...people: Person[]): Map { return new Map(people.map((p) => [p.slack_user_id!, p])); } @@ -110,4 +115,85 @@ describe("user group reconciliation", () => { { kind: "conflict", slackId: "U9", personId: null }, ]); }); + + /** + * The mirror of the property above. Monitoring is sticky on the way out and + * automatic on the way back in: a deactivated student who is declared again + * is monitored again, without waiting for anyone to notice and approve it. + */ + describe("returning after deactivation", () => { + it("resumes monitoring when a deactivated student is declared again", () => { + const p = deactivated(person("U1", "student")); + const decisions = reconcileRoles(roster(p), groups(["U1"], [])); + assert.deepEqual(decisions, [ + { kind: "reactivate", personId: p.id, slackId: "U1", role: "student" }, + ]); + }); + + it("resumes monitoring for a deactivated adult too", () => { + const p = deactivated(person("U1", "adult")); + const decisions = reconcileRoles(roster(p), groups([], ["U1"])); + assert.deepEqual(decisions, [ + { kind: "reactivate", personId: p.id, slackId: "U1", role: "adult" }, + ]); + }); + + it("keeps a deactivated district observer's role while resuming them", () => { + const p = deactivated(person("U1", "district_observer")); + const decisions = reconcileRoles(roster(p), groups([], ["U1"])); + assert.deepEqual(decisions, [ + { + kind: "reactivate", + personId: p.id, + slackId: "U1", + role: "district_observer", + }, + ]); + }); + + it("reactivates and changes role in one decision", () => { + const p = deactivated(person("U1", "student")); + const decisions = reconcileRoles(roster(p), groups([], ["U1"])); + const d = decisions[0]!; + assert.equal(d.kind, "change"); + if (d.kind !== "change") return; + assert.equal(d.to, "adult"); + assert.equal(d.reactivates, true); + assert.equal(d.reducesProtection, true); + }); + + it("does not reactivate someone who is in no group", () => { + const p = deactivated(person("U1", "student")); + assert.deepEqual(reconcileRoles(roster(p), groups([], [])), []); + }); + + it("says nothing about an active person who is already right", () => { + const decisions = reconcileRoles( + roster(person("U1", "student")), + groups(["U1"], []) + ); + assert.deepEqual(decisions, [{ kind: "unchanged", slackId: "U1" }]); + }); + + /** + * There is no decision that deactivates anybody. If one ever appears here, + * the sync has gained the power to end monitoring on its own. + */ + it("never produces a decision that ends monitoring", () => { + const people = [ + person("U1", "student"), + person("U2", "adult"), + deactivated(person("U3", "student")), + deactivated(person("U4", "adult")), + ]; + const decisions = reconcileRoles( + roster(...people), + groups(["U1", "U3"], ["U2", "U4"]) + ); + for (const d of decisions) { + assert.notEqual(d.kind, "deactivate"); + if (d.kind === "change") assert.notEqual(d.reactivates, undefined); + } + }); + }); });