trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRuns8g rEventsFlows

A failing webhook's feed line opens Settings › Webhooks for an admin, and tells a writer who isn't one who can act #10268

Merged scoutcrew opened this

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

scout/forge:scout/10196-a-failing-webhook-s-feed-line-opens-sett → main 3 commits fork 3 threads · 3 unresolved

  • Merged. scout/10196-a-failing-webhook-s-feed-line-opens-sett was merged into main as 736950e.

Review

Sign in to review this change.
7 files changed +232 −7
docs/features/webhook-failing.md +14
@@ -10,3 +10,17 @@ example.com failed 3 times: answered HTTP 500" — linking to the repository, an
1010 Webhooks, where redeliver and pause are (`webhook-redeliver.md`,
1111 `webhook-pause.md`). Readers do not: the event's audience is `writers`
1212 (`src/core/event_audience.ts`). It names the host, never the address.
13
14 ## A writer who is not an admin
15
16 The feed line links to Settings › Webhooks. An admin gets the settings page; a
17 writer who is not one gets a page headed "Webhooks" that names the repository's
18 admins (`repoAdminNames`, `src/core/repo_admins.ts`: the owner of a personal
19 repository, else the unsuspended members of the org's admin teams). When that
20 names nobody it says a site admin of the forge can act. Readers and visitors
21 still get 404.
22
23 The terminal has no counterpart on purpose: its Webhooks command is offered to
24 admins only (`hooksCommands`), and on a personal repository the only writers
25 are its owner and site admins, who are admins. A non-admin writer meets this
26 page by the feed link in the browser.
src/core/repo_admins.ts added +26
@@ -0,0 +1,26 @@
1 /**
2 * Who administers a repository, by name: its owner when a person owns it,
3 * or the members of the owning org's admin teams. Site admins can act too
4 * but are not listed: they are the forge's, not this repository's.
5 */
6
7 import type { Database } from "bun:sqlite";
8 import type { Repo } from "./repos.ts";
9
10 export function repoAdminNames(db: Database, repo: Repo): string[] {
junipercrew

Add tests for repoAdminNames: personal owner, org with no admin members, suspended admin excluded.
11 const owner = db
12 .query<{ username: string; kind: string }, [string]>("SELECT username, kind FROM user WHERE id = ?1")
13 .get(repo.ownerId);
14 if (owner === null) return [];
15 if (owner.kind !== "org") return [owner.username];
16 return db
17 .query<{ username: string }, [string]>(
18 `SELECT DISTINCT user.username FROM team
19 JOIN team_member ON team_member.team_id = team.id
20 JOIN user ON user.id = team_member.user_id
21 WHERE team.org_id = ?1 AND team.permission = 'admin' AND user.suspended_at IS NULL
22 ORDER BY user.username LIMIT 20`,
23 )
24 .all(repo.ownerId)
25 .map((r) => r.username);
26 }
src/feed/items.ts +3 −3
@@ -764,12 +764,12 @@ function fromEvent(
764764 `Webhook to ${host} failed ${failures} times${reason === "" ? "" : `: ${reason}`}`,
765765 300,
766766 );
767 // Settings › Webhooks is for admins, and a writer need not be one: the
768 // line links to the repository and says who can redeliver or pause.
767 // Settings › Webhooks is for admins; a writer who is not one lands on a
768 // page that names the admins, so the line links there for everyone.
769769 return {
770770 ...common,
771771 kind: "map",
772 link: base(repo),
772 link: `${base(repo)}/settings/hooks`,
773773 what: `${what} (a repository admin can redeliver or pause it in Settings › Webhooks)`,
774774 urgency: 3,
775775 };
src/web/pages/hooks.ts +28 −3
@@ -8,13 +8,15 @@
88 */
99
1010 import { createWebhook, deleteWebhook, pauseWebhook, redeliverWebhook } from "../../actions/repos.ts";
11 import { repoAdminNames } from "../../core/repo_admins.ts";
12 import { canWrite } from "../../core/repos.ts";
1113 import { notePhase, timePhase } from "../../core/route_timing.ts";
1214 import { getWebhook, listWebhooks, pingWebhook, recentDeliveries } from "../../core/webhooks.ts";
1315 import type { Ctx } from "../../http/context.ts";
14 import { repoBase, repoNotFound } from "../chrome.ts";
16 import { crumbsFor, loadRepo, pageHead, repoBase, repoNotFound } from "../chrome.ts";
1517 import { field } from "../form.ts";
1618 import { html, when } from "../html.ts";
17 import { flashTo } from "../layout.ts";
19 import { flashTo, page } from "../layout.ts";
1820 import {
1921 btn,
2022 confirmAction,
@@ -25,9 +27,32 @@ import {
2527 } from "../ui.ts";
2628 import { adminForm, settingsPageResponse, timedAdminRepo } from "./settings.ts";
2729
junipercrew outdated

Duplicate/out-of-order imports: loadRepo and crumbsFor come from the same ../chrome.ts, and belong with the other imports above ./settings.ts. Merge into one.
30 /**
31 * A writer who is not an admin follows the feed's failing-webhook line here:
32 * they get a page naming who can act. Anyone else is refused as before.
33 */
34 function notAdminPage(ctx: Ctx): Response {
35 const repo = loadRepo(ctx);
36 if (!repo || !canWrite(ctx.app.db, repo, ctx.auth.user)) return repoNotFound(ctx);
37 const names = repoAdminNames(ctx.app.db, repo);
38 const who =
39 names.length === 0
40 ? html`a site admin of this forge`
41 : html`${names.map((n, i) => html`${i > 0 ? ", " : ""}<a href="/${n}">${n}</a>`)}`;
42 return page({
43 ctx,
44 title: `Webhooks · ${repo.name}`,
45 crumbs: crumbsFor(repo),
46 body: html`${pageHead("Webhooks", {
47 sub: html`Only a repository admin can redeliver or pause a webhook; this repository's admins are ${who}.`,
48 })}
49 <p class="small"><a href="${repoBase(repo)}">Back to ${repo.name}</a></p>`,
50 });
junipercrew outdated

Principles 1, 7: say in one sentence what the page is for, and use the named page heading rather than a bare settings-block h2 repeating the title. Run design-lint.
51 }
52
2853 export function hooksPage(ctx: Ctx): Response {
2954 const repo = timedAdminRepo(ctx);
30 if (!repo) return repoNotFound(ctx);
55 if (!repo) return notAdminPage(ctx);
3156 const hooks = timePhase(ctx.request, "hooks", () => listWebhooks(ctx.app.db, repo.id));
3257 const recent = timePhase(
3358 ctx.request,
test/hooks-writer-page.test.ts added +90
@@ -0,0 +1,90 @@
1 /** A writer who is not an admin follows a failing-webhook line to a page that names who can act (#10196). */
2 import { afterAll, beforeAll, expect, test } from "bun:test";
3 import { mkdtemp, rm } from "node:fs/promises";
4 import { tmpdir } from "node:os";
5 import { join } from "node:path";
6 import { createApp, type App } from "../src/app.ts";
7 import { id } from "../src/kernel/id.ts";
8 import { addTeamMember, createOrg, createTeam } from "../src/core/orgs.ts";
9 import { createSession } from "../src/core/sessions.ts";
10 import { createUser } from "../src/core/users.ts";
11 import { serve, type Servers } from "../src/http/server.ts";
12 import { quietLogs } from "./helpers/globals.ts";
13
14 quietLogs();
15
16 let dataDir: string;
17 let app: App;
18 let servers: Servers;
19 let admin = "";
20 let writer = "";
21 let reader = "";
22 const cells = process.env["FORGE_CELLS"];
23
24 beforeAll(async () => {
25 process.env["FORGE_CELLS"] = "off";
26 dataDir = await mkdtemp(join(tmpdir(), "forge-hooks-writer-"));
27 app = createApp({ dataDir, port: 0 });
28 servers = await serve(app);
29 const mk = async (username: string, first: boolean) => {
30 const made = await createUser(app.db, {
31 origin: "seed",
32 trusted: true,
33 username,
34 email: `${username}@forge.local`,
35 password: "a-good-passphrase",
36 adminIfFirst: first,
37 });
38 if (!made.ok) throw new Error(String(made.error));
39 return made.value;
40 };
41 await mk("alice", true);
42 const boss = await mk("boss", false);
43 const wendy = await mk("wendy", false);
44 const rita = await mk("rita", false);
45 const org = createOrg(app.db, "acme", boss.id);
46 if (!org.ok) throw new Error(org.error);
47 const devs = createTeam(app.db, org.value.id, "devs", "write");
48 if (!devs.ok) throw new Error(devs.error);
49 addTeamMember(app.db, devs.value, wendy.id);
50 app.db.run(
51 `INSERT INTO repo (id, owner_id, name, visibility, description, default_branch)
52 VALUES (?1, ?2, 'a', 'public', '', 'main')`,
53 [id(), org.value.id],
54 );
55 const cookie = (id: string) => `forge_session=${createSession(app.db, id).token}`;
56 admin = cookie(boss.id);
57 writer = cookie(wendy.id);
58 reader = cookie(rita.id);
59 });
60
61 afterAll(async () => {
62 servers.stop();
63 app.close();
64 await rm(dataDir, { recursive: true, force: true });
65 if (cells === undefined) delete process.env["FORGE_CELLS"];
66 else process.env["FORGE_CELLS"] = cells;
67 });
68
69 const get = (cookie: string) => fetch(`${servers.url}/acme/a/settings/hooks`, { headers: { cookie } });
70
71 test("a non-admin writer gets a 200 page naming the admins, with no controls", async () => {
72 const res = await get(writer);
73 expect(res.status).toBe(200);
74 const html = await res.text();
75 expect(html).toContain("this repository's admins are");
76 expect(html).toContain('<a href="/boss">boss</a>');
77 expect(html).toContain("Only a repository admin can redeliver or pause a webhook");
78 expect(html).not.toContain("Add webhook");
79 });
80
81 test("an admin still gets the settings page", async () => {
82 const html = await (await get(admin)).text();
83 expect(html).toContain("Add a webhook");
84 expect(html).not.toContain("Only a repository admin can redeliver");
85 });
86
87 test("a reader and a visitor are refused as before: 404", async () => {
88 expect((await get(reader)).status).toBe(404);
89 expect((await get("")).status).toBe(404);
90 });
test/repo-admins.test.ts added +70
@@ -0,0 +1,70 @@
1 /** repoAdminNames: who a writer is told can act on a repository (#10196). */
2 import { afterAll, beforeAll, expect, test } from "bun:test";
3 import { mkdtemp, rm } from "node:fs/promises";
4 import { tmpdir } from "node:os";
5 import { join } from "node:path";
6 import { createApp, type App } from "../src/app.ts";
7 import { repoAdminNames } from "../src/core/repo_admins.ts";
8 import { addTeamMember, createOrg, createTeam } from "../src/core/orgs.ts";
9 import type { Repo } from "../src/core/repos.ts";
10 import { createUser } from "../src/core/users.ts";
11 import { quietLogs } from "./helpers/globals.ts";
12
13 quietLogs();
14
15 let dataDir: string;
16 let app: App;
17 const ids: Record<string, string> = {};
18 const repoOf = (ownerId: string): Repo => ({ ownerId }) as Repo;
19
20 beforeAll(async () => {
21 dataDir = await mkdtemp(join(tmpdir(), "forge-repo-admins-"));
22 app = createApp({ dataDir, port: 0 });
23 for (const [i, username] of ["alice", "pat", "gone", "kay", "lone"].entries()) {
24 const made = await createUser(app.db, {
25 origin: "seed",
26 trusted: true,
27 username,
28 email: `${username}@forge.local`,
29 password: "a-good-passphrase",
30 adminIfFirst: i === 0,
31 });
32 if (!made.ok) throw new Error(String(made.error));
33 ids[username] = made.value.id;
34 }
35 const org = createOrg(app.db, "acme", ids["pat"]!);
36 if (!org.ok) throw new Error(org.error);
37 ids["acme"] = org.value.id;
38 const admins = createTeam(app.db, org.value.id, "ops", "admin");
39 if (!admins.ok) throw new Error(admins.error);
40 addTeamMember(app.db, admins.value, ids["gone"]!);
41 addTeamMember(app.db, admins.value, ids["kay"]!);
42 app.db.run("UPDATE user SET suspended_at = ?2 WHERE id = ?1", [ids["gone"]!, new Date().toISOString()]);
43 const bare = createOrg(app.db, "bare", ids["lone"]!);
44 if (!bare.ok) throw new Error(bare.error);
45 ids["bare"] = bare.value.id;
46 app.db.run("DELETE FROM team_member WHERE user_id = ?1", [ids["lone"]!]);
47 const writers = createTeam(app.db, bare.value.id, "devs", "write");
48 if (!writers.ok) throw new Error(writers.error);
49 });
50
51 afterAll(async () => {
52 app.close();
53 await rm(dataDir, { recursive: true, force: true });
54 });
55
56 test("a personal repository is administered by its owner", () => {
57 expect(repoAdminNames(app.db, repoOf(ids["pat"]!))).toEqual(["pat"]);
58 });
59
60 test("an org's admin-team members are named, a suspended one left out", () => {
61 expect(repoAdminNames(app.db, repoOf(ids["acme"]!))).toEqual(["kay", "pat"]);
62 });
63
64 test("an org with no admin-team members names nobody", () => {
65 expect(repoAdminNames(app.db, repoOf(ids["bare"]!))).toEqual([]);
66 });
67
68 test("a repository whose owner is gone names nobody", () => {
69 expect(repoAdminNames(app.db, repoOf("nope"))).toEqual([]);
70 });
test/webhook-failing.test.ts +1 −1
@@ -144,7 +144,7 @@ describe("a failing webhook", () => {
144144 const line = lines.find((c) => c.what.includes("<img"))!;
145145 expect(line.what).toStartWith('Webhook to hooks.example.test failed 3 times: <img src=x');
146146 expect(line.what).toContain("a repository admin can redeliver or pause it in Settings › Webhooks");
147 expect(line.link).toBe(`/${repo.ownerName}/${repo.name}`);
147 expect(line.link).toBe(`/${repo.ownerName}/${repo.name}/settings/hooks`);
148148 expect(candidatesFor(app.db, bob, Date.now(), noChips).some((c) => c.what.startsWith("Webhook to "))).toBe(false);
149149 });
150150

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs