From f3568f60d651ae4b393592b932359af7095fdad0 Mon Sep 17 00:00:00 2001 From: Vivswan Shah <58091053+Vivswan@users.noreply.github.com> Date: Wed, 11 Nov 2026 20:19:45 -0500 Subject: [PATCH] fix(sections): never plan the owner, hedge a concealed team 404, and advise the grant the writes need A `collaborators` entry naming the repository owner was settled like any other: on a user-owned repository it planned a PUT re-granting the owner at the declared role, and an organization's own login planned an invitation. The declared walk now skips the owner with a note saying to remove the entry, so no write is ever planned for it. Check mode read a 404 on a declared team's access probe as "no access" even when the repository's team list named the team, the state a fine-grained token without the Members organization permission produces. The drift line now names both readings of the 404 and the grant that settles it, in the words the snapshot's note already used. The grant is still planned. A denied team list advised "Administration" (read), the list's own gate. That passes the next apply preflight and is denied again at the grant PUT. A denial on an endpoint whose permission the section's grant covers now advises the section's grant, so the first advice names what the writes need. `LiveCollaborator.permissions` was parsed and never read, and is gone. The collaborators page said a custom role named "push" leaves a partial snapshot: the section fails, which fails the target with no file written, and the page now says so. --- docs/reference/coverage.md | 6 +- docs/sections/collaborators.docs.yml | 9 ++- docs/sections/teams.docs.yml | 4 + src/sections/collaborators/index.ts | 23 ++++-- src/sections/contract/errors.ts | 22 ++++-- src/sections/contract/permissions.ts | 11 +++ src/sections/teams/index.ts | 48 ++++++++---- .../collaborators/collaborators.test.ts | 19 +++++ .../collaborators-declared-owner-note.yml | 31 ++++++++ test/sections/contract/contract.test.ts | 35 +++++++++ .../scenarios/teams-check-concealed-probe.yml | 31 ++++++++ .../scenarios/teams-list-denied-advice.yml | 23 ++++++ test/sections/teams/teams.test.ts | 74 ++++++++++++++++++- 13 files changed, 304 insertions(+), 32 deletions(-) create mode 100644 test/sections/collaborators/scenarios/collaborators-declared-owner-note.yml create mode 100644 test/sections/teams/scenarios/teams-check-concealed-probe.yml create mode 100644 test/sections/teams/scenarios/teams-list-denied-advice.yml diff --git a/docs/reference/coverage.md b/docs/reference/coverage.md index cccba8831..4b6beb2f8 100644 --- a/docs/reference/coverage.md +++ b/docs/reference/coverage.md @@ -408,9 +408,10 @@ The side-by-side comparison with the Probot Settings app lives in the migration - Direct collaborators only. PUT/DELETE /repos/{owner}/{repo}/collaborators/{username} (affiliation=direct); new users get invitations. - Vocabulary mapping push<->write, pull<->read for check mode; custom org role names pass through. -- Undeclared direct collaborators are REMOVED by default (kept as notes under the wrapped `_undeclared: keep` form); the owner is never touched. +- Undeclared direct collaborators are REMOVED by default (kept as notes under the wrapped `_undeclared: keep` form). +- The repository owner is never touched: it is never removed as undeclared, and an entry naming it (an organization's own login included) plans no write and earns a note saying to remove the entry. - Snapshot reads the direct collaborators and the pending username invitations back as entries under the delete default, each role through the inverse mapping (write -> push, read -> pull, custom role names verbatim). -- The snapshot leaves out the repository owner, email invitations, and expired invitations with a note; a live role no declaration plans as (a custom role named "push" or "pull") fails the collaborators section, and the snapshot is written as partial without it. +- The snapshot leaves out the repository owner, email invitations, and expired invitations with a note; a live role no declaration plans as (a custom role named "push" or "pull") fails the collaborators section, and a failed section fails the target: the result is `failed` and no file is written for it. **Repository invitations** (`collaborators`) @@ -423,6 +424,7 @@ The side-by-side comparison with the Probot Settings app lives in the migration - Org repos only. PUT/DELETE /orgs/{org}/teams/{slug}/repos/{owner}/{repo}; the section probes GET /orgs/{owner} and no-ops with a note on personal accounts (404 only; 403/5xx still fail). - Both modes probe each declared team's access with the v3.repository media type to read role_name, and the grant is issued only when the team lacks access or its live role diverges. +- A declared team the repository's team list names, whose probe answers 404, is still planned as a grant, and its check line names both readings of the 404 (no access, or a fine-grained token missing the grant), as the snapshot's note does. - The repository's team list (GET /repos/{owner}/{repo}/teams) names the undeclared teams: kept by default and noted (a team is often granted by the org for reasons outside one repository's file). - Under the wrapped `_undeclared: delete` form a team's DIRECT grant is revoked, while access granted at the organization or enterprise level is noted as beyond the repository's reach. - A declared child team whose access comes through an undeclared parent's grant loses it with that revocation (the probe reads the inherited role as converged): declare the parent too, or keep. diff --git a/docs/sections/collaborators.docs.yml b/docs/sections/collaborators.docs.yml index 6b26e9881..d82232fa1 100644 --- a/docs/sections/collaborators.docs.yml +++ b/docs/sections/collaborators.docs.yml @@ -15,7 +15,11 @@ coverage: through. - >- Undeclared direct collaborators are REMOVED by default (kept as notes under the wrapped - `_undeclared: keep` form); the owner is never touched. + `_undeclared: keep` form). + - >- + The repository owner is never touched: it is never removed as undeclared, and an entry + naming it (an organization's own login included) plans no write and earns a note saying to + remove the entry. - >- Snapshot reads the direct collaborators and the pending username invitations back as entries under the delete default, each role through the inverse mapping (write -> push, read -> pull, @@ -23,7 +27,8 @@ coverage: - >- The snapshot leaves out the repository owner, email invitations, and expired invitations with a note; a live role no declaration plans as (a custom role named "push" or "pull") fails the - collaborators section, and the snapshot is written as partial without it. + collaborators section, and a failed section fails the target: the result is `failed` and no + file is written for it. - area: "[Repository invitations](https://docs.github.com/en/rest/collaborators/invitations)" endpoints: [listInvitations, updateInvitation, cancelInvitation] notes: diff --git a/docs/sections/teams.docs.yml b/docs/sections/teams.docs.yml index c7cbf0ab9..305f39498 100644 --- a/docs/sections/teams.docs.yml +++ b/docs/sections/teams.docs.yml @@ -13,6 +13,10 @@ coverage: - >- Both modes probe each declared team's access with the v3.repository media type to read role_name, and the grant is issued only when the team lacks access or its live role diverges. + - >- + A declared team the repository's team list names, whose probe answers 404, is still planned + as a grant, and its check line names both readings of the 404 (no access, or a fine-grained + token missing the grant), as the snapshot's note does. - >- The repository's team list (GET /repos/{owner}/{repo}/teams) names the undeclared teams: kept by default and noted (a team is often granted by the org for reasons outside one repository's diff --git a/src/sections/collaborators/index.ts b/src/sections/collaborators/index.ts index f8384fab5..539eb4a4b 100644 --- a/src/sections/collaborators/index.ts +++ b/src/sections/collaborators/index.ts @@ -1,7 +1,7 @@ /** * `collaborators:` section: direct collaborators by username plus their pending invitations; the owner is - * never removed. Bespoke, not on listSection: one declared username resolves against two live pools (the - * collaborator list and the pending invitations), each with its own writes. + * never touched, declared or not. Bespoke, not on listSection: one declared username resolves against two + * live pools (the collaborator list and the pending invitations), each with its own writes. */ import { err, ok, type Result, safeTry } from "neverthrow"; @@ -32,7 +32,6 @@ import { CollaboratorConfig } from "./schema.js"; const LiveCollaborator = z.looseObject({ login: z.string(), - permissions: z.record(z.string(), z.boolean()).optional(), role_name: z.string().optional(), }); type LiveCollaborator = z.infer; @@ -256,20 +255,30 @@ function inviteDeclared( }); } -/** One declared entry settled against the collaborator it already is, else its pending invitation, else invited. */ +/** + * One declared entry settled against the collaborator it already is, else its pending invitation, else invited. + * The owner is none of the three: an entry naming it earns a note, never a write. + */ function settleDeclared( + ctx: CollaboratorsContext, collaborator: CollaboratorConfig, access: LiveAccess, plan: CollaboratorsPlan, ): void { + const login = loginKey(collaborator.username); + if (isOwner(ctx, login)) { + plan.notes.push( + `collaborator "${collaborator.username}" is the repository owner, whose access is implicit and never managed; the entry is ignored - remove it from the settings file to silence this note`, + ); + return; + } + const permission = collaborator.permission ?? DEFAULT_ROLE; const wanted: Wanted = { username: collaborator.username, permission, role: roleForPermission(permission), }; - const login = loginKey(collaborator.username); - const existing = access.liveByLogin.get(login); if (existing) { // On GitHub a user is never a collaborator AND an invitee at once, so this branch settles the entry. @@ -465,7 +474,7 @@ export const collaboratorsSection = { const declaredKeys = new Set(); for (const collaborator of desired) { declaredKeys.add(loginKey(collaborator.username)); - settleDeclared(collaborator, access, plan); + settleDeclared(ctx, collaborator, access, plan); } const undeclared: Undeclared = { declaredKeys, policy, listDefault }; diff --git a/src/sections/contract/errors.ts b/src/sections/contract/errors.ts index d0817623c..9fcefa09d 100644 --- a/src/sections/contract/errors.ts +++ b/src/sections/contract/errors.ts @@ -8,7 +8,12 @@ import { sectionGrant, sectionOperations, } from "./operations.js"; -import { grantFor, type SectionPermission, samePermission } from "./permissions.js"; +import { + coversPermission, + grantFor, + type SectionPermission, + samePermission, +} from "./permissions.js"; /** * What ends a section's work, as a value. `message` is the whole line the loops report; `kind` is read for policy @@ -87,9 +92,9 @@ function sentence(clause: string): string { /** * `operation` says WHAT was being done in settings-file terms. `op` supplies the hints and denial hint and resolves - * the EFFECTIVE permission: an override renders its own grant advice, and a public operation ("none") cannot be a - * missing-grant failure, so its 403/404 takes the generic branch; a GraphQL failure renders `GRAPHQL ` in - * the method/path slot. + * the EFFECTIVE permission: an override the section's grant does not cover renders its own grant advice, and a + * public operation ("none") cannot be a missing-grant failure, so its 403/404 takes the generic branch; a GraphQL + * failure renders `GRAPHQL ` in the method/path slot. */ interface FailureContext { readonly operation?: string; @@ -117,14 +122,19 @@ function requestLines( }; } -/** The denial's advice: the grant of the EFFECTIVE permission at the level the section needs, or the section's own, then the endpoint's hint. */ +/** + * The denial's advice: the section's own grant whenever it covers the EFFECTIVE permission, else that permission's + * grant at the level the section needs; then the endpoint's hint. An override the section's grant covers (the team + * list, gated at the repository half alone) is no second grant to ask for: advising only its own read would pass + * the preflight and be denied at the section's first write. + */ function denialAdvice( section: SectionMeta, effective: SectionPermission | undefined, op: FailingOp | undefined, ): string { const grant = - effective !== undefined && !samePermission(effective, section.permission) + effective !== undefined && !coversPermission(section.permission, effective) ? grantFor(effective, undefined, overrideAdviceLevel(section, effective)) : sectionGrant(section); const denialHint = op?.denialHint ? `. Note: ${op.denialHint}` : ""; diff --git a/src/sections/contract/permissions.ts b/src/sections/contract/permissions.ts index f2f95aad6..6b99ecbd3 100644 --- a/src/sections/contract/permissions.ts +++ b/src/sections/contract/permissions.ts @@ -39,6 +39,17 @@ export function samePermission( return a.org === b.org && resources.size === others.size && resources.isSubsetOf(others); } +/** + * Whether a token granted `grant` passes `asked`: whichever alternative of `grant` the user picks is one `asked` + * accepts, and `asked` wants no organization permission `grant` lacks. The teams grant covers its own list read + * this way, which GitHub gates at the repository half alone. + */ +export function coversPermission(grant: SectionPermission, asked: SectionPermission): boolean { + const offered = new Set(grant.repo); + const accepted = new Set(asked.repo); + return (asked.org === undefined || asked.org === grant.org) && offered.isSubsetOf(accepted); +} + /** Human-facing label for each PAT resource, as shown in the token UI. */ export const RESOURCE_LABEL: Record = { administration: "Administration", diff --git a/src/sections/teams/index.ts b/src/sections/teams/index.ts index e42138be2..9740f524d 100644 --- a/src/sections/teams/index.ts +++ b/src/sections/teams/index.ts @@ -133,15 +133,39 @@ function teamsBySlug( ); } -/** The drift line a declared team's probe earns, or undefined once its live role matches. */ +/** + * A probe 404 for a team the repository's list names, in the words plan() and snapshot() share. Neither can tell + * no access from a probe the token is denied, and check mode and a snapshot write nothing that would surface the + * denial, so both readings are named. `so` is what the caller does under the first reading. + */ +function listedProbe404( + ctx: TeamsContext, + section: SectionMeta, + opts: { so?: string; rerun: "check" | "snapshot" }, +): string { + const so = opts.so === undefined ? "" : `, so ${opts.so}`; + return ( + `listed with access to ${ctx.repo.slug}, but the access probe answered 404, read here as no access${so}. ` + + "A fine-grained token missing the grant gets the same answer; if the team does have access, " + + `${sectionGrant(section)}, then ${opts.rerun} again` + ); +} + +/** The drift line a declared team's probe earns, or undefined once its live role matches; `listed` says the repository's list names the team, which a probe 404 contradicts. */ function declaredTeamDrift( - repo: string, + ctx: TeamsContext, + section: SectionMeta, team: TeamConfig, role: string, - probe: TeamAccess, + found: { probe: TeamAccess; listed: boolean }, ): string | undefined { + const label = `teams[${team.name}]`; + const { probe } = found; if (!probe.access) { - return `teams[${team.name}]: no access to ${repo}; apply will grant "${role}"`; + const grant = `apply will grant "${role}"`; + return found.listed + ? `${label}: ${listedProbe404(ctx, section, { so: grant, rerun: "check" })}` + : `${label}: no access to ${ctx.repo.slug}; ${grant}`; } const wantRole = roleForPermission(role); @@ -150,7 +174,7 @@ function declaredTeamDrift( return undefined; } - return valueDrift(`teams[${team.name}]`, JSON.stringify(wantRole), JSON.stringify(liveRole), { + return valueDrift(label, JSON.stringify(wantRole), JSON.stringify(liveRole), { remedy: "apply will set the declared permission", }); } @@ -221,13 +245,7 @@ function snapshotRole(ctx: TeamsContext, section: SectionMeta, team: LiveTeam): return probeTeamRole(ctx, team.slug).map((probe): SnapshotRole => { if (!probe.access) { - // No write follows to surface a denial, so the note names both readings of the 404. - return { - leftOut: - `listed with access to ${ctx.repo.slug}, but the access probe answered 404, read here as no access. ` + - "A fine-grained token missing the grant gets the same answer; if the team does have access, " + - `${sectionGrant(section)}, then snapshot again`, - }; + return { leftOut: listedProbe404(ctx, section, { rerun: "snapshot" }) }; } if (probe.role === undefined) { return { @@ -260,7 +278,8 @@ export const teamsSection = { ); const plan: SectionPlan = { ops: [], notes: [], drift: [] }; // The listing is read BEFORE the declared walk, so the undeclared teams are judged against the state the grants - // below start from; a declared team's role still comes from the probe, which names a custom role. + // below start from; a declared team's role still comes from the probe, which names a custom role, and the + // listing only says whether a probe 404 contradicts it. const live = yield* ctx.read.list .listAll(LiveTeam) .andThen((teams) => teamsBySlug(section, teams)); @@ -268,7 +287,8 @@ export const teamsSection = { for (const team of desired) { const role = team.permission ?? DEFAULT_ROLE; const probe = yield* probeTeamRole(ctx, team.name); - const drift = declaredTeamDrift(ctx.repo.slug, team, role, probe); + const listed = live.has(teamKey(team.name)); + const drift = declaredTeamDrift(ctx, section, team, role, { probe, listed }); if (drift === undefined) { continue; } diff --git a/test/sections/collaborators/collaborators.test.ts b/test/sections/collaborators/collaborators.test.ts index 9d4d34be3..f46b93f8b 100644 --- a/test/sections/collaborators/collaborators.test.ts +++ b/test/sections/collaborators/collaborators.test.ts @@ -82,6 +82,25 @@ describe("collaborators", () => { expect(api.calls.map((c) => `${c.method} ${c.path}`)).toEqual([LIST, INVITATIONS]); }); + test.each<[label: string, username: string, live: Array<{ login: string; role_name: string }>]>([ + // Settled like anyone else, the listed owner would be re-granted at the declared role. + ["a user-owned repository's owner, listed as admin", "o", [{ login: "o", role_name: "admin" }]], + // No list carries an organization, so its login would read as a missing collaborator and be invited. + ["an organization's own login in another case, which no list carries", "O", []], + ])( + "a declared repository owner plans no write and is noted with the fix: %s", + async (_label, username, live) => { + const api = new MockApi({ [LIST]: { data: live }, [INVITATIONS]: { data: [] } }); + expect(await plan(api, [{ username, permission: "push" }])).toEqual({ + ops: [], + notes: [ + `collaborator "${username}" is the repository owner, whose access is implicit and never managed; the entry is ignored - remove it from the settings file to silence this note`, + ], + drift: [], + }); + }, + ); + test("a matching role and a matching pending invitation plan nothing; keep turns the undeclared ones into notes", async () => { const api = new MockApi({ [LIST]: { diff --git a/test/sections/collaborators/scenarios/collaborators-declared-owner-note.yml b/test/sections/collaborators/scenarios/collaborators-declared-owner-note.yml new file mode 100644 index 000000000..0186f0db8 --- /dev/null +++ b/test/sections/collaborators/scenarios/collaborators-declared-owner-note.yml @@ -0,0 +1,31 @@ +name: collaborators-declared-owner-note +description: >- + The settings file names the repository owner beside a collaborator already + at the declared role, on a user-owned repository whose list carries the + owner as admin. Settled like any other entry the owner would be re-granted + at the declared "push"; instead apply plans no write for it, the entry + earns a note saying to remove it, and a check rerun converges. +settings: + collaborators: + - username: e2e-owner + permission: push + - username: e2e-keeper + permission: push +owner_kind: user +live_state: + collaborators: + - login: e2e-owner + role_name: admin + - login: e2e-keeper + role_name: write +expect: + exit_code: 0 + result: applied + outcomes: + collaborators: applied + never: + - "PUT /repos/{repo}/collaborators/" + - "DELETE /repos/{repo}/collaborators/" + stdout_contains: + - 'collaborator "e2e-owner" is the repository owner, whose access is implicit and never managed; the entry is ignored - remove it from the settings file to silence this note' + fixpoint: converges diff --git a/test/sections/contract/contract.test.ts b/test/sections/contract/contract.test.ts index a2b149042..3192c09ef 100644 --- a/test/sections/contract/contract.test.ts +++ b/test/sections/contract/contract.test.ts @@ -30,6 +30,7 @@ import { writeGatedReads, } from "../../../src/sections/contract/operations.js"; import { + coversPermission, type SectionPermission, samePermission, } from "../../../src/sections/contract/permissions.js"; @@ -1548,6 +1549,40 @@ describe("DenialPolicy", () => { }); }); +describe("coversPermission", () => { + // Only the first row is a shipped section (teams): the others keep the direction and the alternatives honest, where a + // symmetric or reversed comparison would advise a section grant that leaves the denied endpoint denied. + test.each<[label: string, grant: SectionPermission, asked: SectionPermission, covers: boolean]>([ + [ + "a read gated at the repository half of a grant that also carries an organization half", + { repo: ["administration"], org: "members" }, + { repo: ["administration"] }, + true, + ], + [ + "an organization half the grant lacks", + { repo: ["administration"] }, + { repo: ["administration"], org: "members" }, + false, + ], + [ + "a grant alternative the asked permission does not accept, which the user may be the one to pick", + { repo: ["administration", "code_scanning_alerts"] }, + { repo: ["administration"] }, + false, + ], + [ + "a grant whose every alternative the asked permission accepts", + { repo: ["administration"] }, + { repo: ["code_scanning_alerts", "administration"] }, + true, + ], + ["a different resource", { repo: ["administration"] }, { repo: ["contents"] }, false], + ])("%s: %p against %p covers %p", (_label, grant, asked, covers) => { + expect(coversPermission(grant, asked)).toBe(covers); + }); +}); + describe("samePermission", () => { test.each< [label: string, a: SectionPermission | "none", b: SectionPermission | "none", same: boolean] diff --git a/test/sections/teams/scenarios/teams-check-concealed-probe.yml b/test/sections/teams/scenarios/teams-check-concealed-probe.yml new file mode 100644 index 000000000..f19d60cd0 --- /dev/null +++ b/test/sections/teams/scenarios/teams-check-concealed-probe.yml @@ -0,0 +1,31 @@ +name: teams-check-concealed-probe +description: >- + A fine-grained token holds Administration but not the Members organization + permission, so the repository's team list names the declared team while its + access probe is answered 404. The team already holds the declared role, and + check mode cannot tell: the drift line names both readings of the 404 and + the grant that settles it, where a plain "no access" line would be false. +settings: + teams: + - name: platform + permission: push +inputs: + mode: check +owner_kind: org +token_permissions: + org_members: none +live_state: + teams: + platform: + role_name: write +expect: + exit_code: 1 + result: drift + outcomes: + teams: drift + never: + - "PUT /orgs/e2e-owner/teams/platform/repos/{repo}" + stdout_contains: + - "teams[platform]: listed with access to" + - 'but the access probe answered 404, read here as no access, so apply will grant "push". A fine-grained token missing the grant gets the same answer' + - 'grant "Members" (read) under the PAT''s Organization permissions and "Administration" (read and write) under its Repository permissions, then check again' diff --git a/test/sections/teams/scenarios/teams-list-denied-advice.yml b/test/sections/teams/scenarios/teams-list-denied-advice.yml new file mode 100644 index 000000000..879c2a0da --- /dev/null +++ b/test/sections/teams/scenarios/teams-list-denied-advice.yml @@ -0,0 +1,23 @@ +name: teams-list-denied-advice +description: >- + A token without Administration is denied the repository's team list, the + section's first gated read, so the apply preflight fails the run before + any write. The advice names the whole grant the section's writes need: + advice for the list's own gate alone (Administration, read) would pass the + next preflight and be denied at the grant PUT. +settings: + teams: + - name: platform + permission: push +owner_kind: org +token_permissions: + administration: none +denial_style: 403 +expect: + exit_code: 1 + result: failed + never: + - "PUT /orgs/e2e-owner/teams/platform/repos/{repo}" + stdout_contains: + - "teams: the token was denied GET /repos/" + - 'To fix, grant "Members" (read) under the PAT''s Organization permissions and "Administration" (read and write) under its Repository permissions' diff --git a/test/sections/teams/teams.test.ts b/test/sections/teams/teams.test.ts index 0db70ce6b..bf36b938d 100644 --- a/test/sections/teams/teams.test.ts +++ b/test/sections/teams/teams.test.ts @@ -5,6 +5,7 @@ import { describe, expect, test } from "bun:test"; import type { SectionInput } from "../../../src/sections/contract/declared.js"; +import { failureFor } from "../../../src/sections/contract/errors.js"; import type { SectionModule } from "../../../src/sections/contract/module.js"; import { planContext, snapshotContext } from "../../../src/sections/contract/plan.js"; import { sectionModule } from "../../../src/sections/registry.js"; @@ -12,7 +13,7 @@ import { teamsSection } from "../../../src/sections/teams/index.js"; import { MockApi } from "../../mock-api.js"; import { fragmentFake } from "../fragment-fake.js"; import { provePlanIdempotent } from "../plan-idempotence.js"; -import { REPO, unwrap } from "../section-run.js"; +import { REPO, rejectsDenied, unwrap } from "../section-run.js"; import { validatedInput } from "../validated-input.js"; import { teamsMockHandlers } from "./mock.js"; @@ -27,6 +28,13 @@ const plan = async (api: MockApi, desired: SectionInput<"teams">) => const snapshot = async (api: MockApi) => unwrap(await gated.snapshot(snapshotContext(gated, api, REPO, "fail"))); +const SECTION_GRANT = + 'grant "Members" (read) under the PAT\'s Organization permissions and "Administration" (read and write) under its Repository permissions'; +/** What plan() and snapshot() both say of a team the list names and the probe answers 404 for. */ +const LISTED_PROBE_404 = (reading: string) => + `listed with access to o/r, but the access probe answered 404, ${reading}. ` + + `A fine-grained token missing the grant gets the same answer; if the team does have access, ${SECTION_GRANT}`; + describe("teams", () => { test("a personal account no-ops with a note after the org probe alone", async () => { const api = new MockApi({}); @@ -86,6 +94,70 @@ describe("teams", () => { ]); }); + test("a listed team whose probe answers 404 drifts under both readings of the 404, as snapshot() notes the same state; unlisted, it is plain no access", async () => { + // Without the listing, check calls a probe the token may be denied "no access" and promises a grant, while snapshot() hedges the same answers. + const notFound = { error: { status: 404, message: "Not Found", body: "" } }; + const api = new MockApi({ + [ORG]: { data: { login: "o" } }, + [LIST]: { + data: [ + { slug: "core", access_source: "direct" }, + { slug: "everyone", access_source: "organization" }, + ], + }, + // The declared spelling and the listed slug differ in case, as GitHub folds them. + [probeOf("Core")]: notFound, + [probeOf("core")]: notFound, + [probeOf("everyone")]: notFound, + [probeOf("new")]: notFound, + }); + const result = await plan(api, [ + { name: "Core", permission: "push" }, + { name: "everyone" }, + { name: "new" }, + ]); + expect(result.ops.map((op) => op.drift)).toEqual([ + [ + `teams[Core]: ${LISTED_PROBE_404('read here as no access, so apply will grant "push"')}, then check again`, + ], + // Access granted above the repository is listed access all the same, so its 404 is as unproven. + [ + `teams[everyone]: ${LISTED_PROBE_404('read here as no access, so apply will grant "push"')}, then check again`, + ], + ['teams[new]: no access to o/r; apply will grant "push"'], + ]); + expect(result.ops.map((op) => [op.role, op.params?.team_slug])).toEqual([ + ["grant", "Core"], + ["grant", "everyone"], + ["grant", "new"], + ]); + expect([result.notes, result.drift]).toEqual([[], []]); + expect((await snapshot(api)).notes).toEqual([ + `teams[core]: left out of the snapshot - ${LISTED_PROBE_404("read here as no access")}, then snapshot again`, + "teams[everyone]: left out of the snapshot - access to o/r is granted at the organization level, not on the repository, and declaring it would grant direct access", + ]); + }); + + test("a denied team list advises the grant the section's writes need, so the fix costs one round trip", async () => { + // Advice naming only the list's own gate (Administration, read) passes the next preflight, and the grant PUT is denied in turn. + const denied = { + status: 403, + message: "Resource not accessible by personal access token", + body: "", + }; + const api = new MockApi({ [ORG]: { data: { login: "o" } }, [LIST]: { error: denied } }); + expect(await rejectsDenied(plan(api, [{ name: "core" }]))).toBe( + `the token was denied GET /repos/o/r/teams: 403 Resource not accessible by personal access token. To fix, ${SECTION_GRANT}`, + ); + const grant = teamsSection.endpoints.grant; + expect( + failureFor(gated, "PUT", "/orgs/o/teams/core/repos/o/r", denied, { op: grant }), + ).toMatchObject({ + kind: "permission-denied", + detail: `the token was denied PUT /orgs/o/teams/core/repos/o/r: 403 Resource not accessible by personal access token. To fix, ${SECTION_GRANT}`, + }); + }); + // A converged declared team beside an undeclared direct team and a team whose access is granted above the repository. const mixedAccess = { [ORG]: { data: { login: "o" } },