Public: anyone can read this; the crew can change it.
Removing a member from a team goes through the confirm page, which names the person and what they lose #8009
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/7980-removing-a-member-from-a-team-goes-throu → main 1 commit fork
3 files
changed +125
−5
src/web/pages/org.ts +12 −5
| @@ -550,10 +550,11 @@ export function teamPage(ctx: Ctx): Response { | ||
| 550 | 550 | trail: when( |
| 551 | 551 | !protectedTeam || members.length > 1, |
| 552 | 552 | () => |
| 553 | html`<form method="post" action="${teamBase}/remove"> | |
| 554 | <input type="hidden" name="user" value="${m.username}" /> | |
| 555 | ${btn("Remove", { variant: "danger", size: "sm" })} | |
| 556 | </form>`, | |
| 553 | confirmAction({ | |
| 554 | action: `${teamBase}/remove?user=${encodeURIComponent(m.username)}`, | |
| 555 | verb: "Remove", | |
| 556 | size: "sm", | |
| 557 | }), | |
| 557 | 558 | ), |
| 558 | 559 | }), |
| 559 | 560 | ), |
| @@ -641,7 +642,13 @@ export async function teamMemberAdd(ctx: Ctx): Promise<Response> { | ||
| 641 | 642 | export async function teamMemberRemove(ctx: Ctx): Promise<Response> { |
| 642 | 643 | const r = await teamAction(ctx); |
| 643 | 644 | if (r instanceof Response) return r; |
| 644 | const username = field(r.form, "user").trim(); | |
| 645 | // The confirm page posts to its `to` path exactly, so the member arrives | |
| 646 | // in the query; a form that names one still wins. | |
| 647 | const username = ( | |
| 648 | field(r.form, "user") || | |
| 649 | ctx.url.searchParams.get("user") || | |
| 650 | "" | |
| 651 | ).trim(); | |
| 645 | 652 | const dest = `/${r.org.username}/-/teams/${r.team.id}`; |
| 646 | 653 | const removed = removeTeamMember( |
| 647 | 654 | ctx.app, |
src/web/pages/search.ts +9
| @@ -680,6 +680,15 @@ const DESTRUCTIVE: { | ||
| 680 | 680 | phrase: "", |
| 681 | 681 | }), |
| 682 | 682 | }, |
| 683 | { | |
| 684 | pattern: /^\/([^/]+)\/-\/teams\/([^/]+)\/remove\?user=([A-Za-z0-9_.-]+)$/, | |
| 685 | describe: (m) => ({ | |
| 686 | verb: "Remove from team", | |
| 687 | title: `Remove @${m[3]} from this team?`, | |
| 688 | detail: `They lose whatever access this team grants them in ${m[1]}. Their account is untouched.`, | |
| 689 | phrase: "", | |
| 690 | }), | |
| 691 | }, | |
| 683 | 692 | { |
| 684 | 693 | pattern: /^\/([^/]+)\/([^/]+)\/settings\/app\/disable$/, |
| 685 | 694 | describe: (m) => ({ |
test/confirm-team-remove.test.ts added +104
| @@ -0,0 +1,104 @@ | ||
| 1 | /** Removing a member from a team asks first, on the confirm page, which names the person and what they lose (#7980). */ | |
| 2 | import { afterAll, beforeAll, describe, 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 { createOrg } from "../src/actions/orgs.ts"; | |
| 7 | import { createApp, type App } from "../src/app.ts"; | |
| 8 | import { addTeamMember, listTeamMembers, listTeams, OWNERS_TEAM, type Team } from "../src/core/orgs.ts"; | |
| 9 | import { createSession } from "../src/core/sessions.ts"; | |
| 10 | import { createUser, type User } from "../src/core/users.ts"; | |
| 11 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 12 | import { proveAll } from "./helpers/verified.ts"; | |
| 13 | import { quietLogs } from "./helpers/globals.ts"; | |
| 14 | ||
| 15 | quietLogs(); | |
| 16 | ||
| 17 | describe("Remove on a team page goes through the confirm page", () => { | |
| 18 | let dataDir = ""; | |
| 19 | let app: App; | |
| 20 | let servers: Servers; | |
| 21 | let cookie = ""; | |
| 22 | let owners: Team; | |
| 23 | let soloOwners: Team; | |
| 24 | const confirmHref = (to: string) => `/-/confirm?to=${encodeURIComponent(to)}`; | |
| 25 | const get = (path: string) => fetch(`${servers.url}${path}`, { redirect: "manual", headers: { cookie } }); | |
| 26 | const post = (path: string) => | |
| 27 | fetch(`${servers.url}${path}`, { method: "POST", redirect: "manual", headers: { cookie, origin: servers.url } }); | |
| 28 | const members = (team: Team) => listTeamMembers(app.db, team.id).map((u) => u.username).sort(); | |
| 29 | const person = async (name: string): Promise<User> => { | |
| 30 | const made = await createUser(app.db, { | |
| 31 | origin: "seed", | |
| 32 | trusted: true, | |
| 33 | username: name, | |
| 34 | email: `${name}@forge.local`, | |
| 35 | password: "a-good-passphrase", | |
| 36 | adminIfFirst: name === "alice", | |
| 37 | }); | |
| 38 | if (!made.ok) throw new Error(made.error); | |
| 39 | return made.value; | |
| 40 | }; | |
| 41 | const ownersOf = (org: User) => listTeams(app.db, org.id).find((t) => t.name === OWNERS_TEAM)!; | |
| 42 | ||
| 43 | beforeAll(async () => { | |
| 44 | dataDir = await mkdtemp(join(tmpdir(), "forge-confirm-team-remove-")); | |
| 45 | app = createApp({ dataDir, port: 0 }); | |
| 46 | const alice = await person("alice"); | |
| 47 | const bob = await person("bob"); | |
| 48 | proveAll(app); | |
| 49 | const acme = createOrg(app, alice, "acme"); | |
| 50 | if (!acme.ok) throw new Error(acme.error); | |
| 51 | owners = ownersOf(acme.value); | |
| 52 | expect(addTeamMember(app.db, owners, bob.id)).toBe(true); | |
| 53 | const solo = createOrg(app, alice, "solo"); | |
| 54 | if (!solo.ok) throw new Error(solo.error); | |
| 55 | soloOwners = ownersOf(solo.value); | |
| 56 | cookie = `forge_session=${createSession(app.db, alice.id).token}`; | |
| 57 | servers = await serve(app); | |
| 58 | }, 60_000); | |
| 59 | afterAll(async () => { | |
| 60 | servers.stop(); | |
| 61 | app.close(); | |
| 62 | await rm(dataDir, { recursive: true, force: true }); | |
| 63 | }); | |
| 64 | ||
| 65 | test("the team page links Remove to the confirm page, not a form that posts at once", async () => { | |
| 66 | const page = await (await get(`/acme/-/teams/${owners.id}`)).text(); | |
| 67 | const to = `/acme/-/teams/${owners.id}/remove?user=bob`; | |
| 68 | expect(page).toContain(`href="${confirmHref(to)}"`); | |
| 69 | expect(page).toContain("remove%3Fuser%3Dbob"); | |
| 70 | expect(page).not.toContain(`action="/acme/-/teams/${owners.id}/remove"`); | |
| 71 | }); | |
| 72 | ||
| 73 | test("the confirm page names the person and what they lose, and posts to the path", async () => { | |
| 74 | const to = `/acme/-/teams/${owners.id}/remove?user=bob`; | |
| 75 | const res = await get(confirmHref(to)); | |
| 76 | expect(res.status).toBe(200); | |
| 77 | const body = await res.text(); | |
| 78 | expect(body).toContain("Remove @bob from this team?"); | |
| 79 | expect(body).toContain("They lose whatever access this team grants them in acme. Their account is untouched."); | |
| 80 | expect(body).toContain(`action="/acme/-/teams/${owners.id}/remove?user=bob"`); | |
| 81 | expect(body).toContain("Remove from team"); | |
| 82 | expect(members(owners)).toEqual(["alice", "bob"]); | |
| 83 | }); | |
| 84 | ||
| 85 | test("a crafted query is not a target the confirm page words", async () => { | |
| 86 | const res = await get(confirmHref(`/acme/-/teams/${owners.id}/remove?user=bob&next=x`)); | |
| 87 | expect(res.status).toBe(404); | |
| 88 | }); | |
| 89 | ||
| 90 | test("the POST from the confirm page removes the member", async () => { | |
| 91 | const res = await post(`/acme/-/teams/${owners.id}/remove?user=bob`); | |
| 92 | expect(res.status).toBe(303); | |
| 93 | expect(members(owners)).toEqual(["alice"]); | |
| 94 | }); | |
| 95 | ||
| 96 | test("the last owner is still refused", async () => { | |
| 97 | const res = await post(`/solo/-/teams/${soloOwners.id}/remove?user=alice`); | |
| 98 | expect(res.status).toBe(303); | |
| 99 | expect(members(soloOwners)).toEqual(["alice"]); | |
| 100 | const flash = (res.headers.get("set-cookie") ?? "").split(";")[0]!; | |
| 101 | const back = await fetch(`${servers.url}${res.headers.get("location")}`, { headers: { cookie: `${cookie}; ${flash}` } }); | |
| 102 | expect(await back.text()).toContain("must keep at least one person who owns solo"); | |
| 103 | }); | |
| 104 | }); |
new above
offline