From 9e7a946b65e50dbf39fcbb5e2736f43cbda52107 Mon Sep 17 00:00:00 2001 From: Piotr Galar Date: Thu, 16 Jul 2026 14:11:12 +0100 Subject: [PATCH 01/13] Add inactive member workflows --- .../workflows/finalize-membership-changes.yml | 99 ++++++ .github/workflows/update-inactive-members.yml | 119 +++++++ docs/SETUP.md | 12 +- .../__tests__/actions/access-summary.test.ts | 123 +++++++ .../finalize-membership-changes.test.ts | 43 +++ .../actions/update-inactive-members.test.ts | 157 +++++++++ .../actions/finalize-membership-changes.ts | 131 ++++++++ scripts/src/actions/fix-yaml-config.ts | 13 +- scripts/src/actions/shared/access-summary.ts | 299 +++++++++++++++++ .../actions/shared/describe-access-changes.ts | 227 ++++++------- .../src/actions/update-inactive-members.ts | 302 ++++++++++++++++++ scripts/src/github.ts | 26 ++ scripts/src/terraform/schema.ts | 2 + 13 files changed, 1416 insertions(+), 137 deletions(-) create mode 100644 .github/workflows/finalize-membership-changes.yml create mode 100644 .github/workflows/update-inactive-members.yml create mode 100644 scripts/__tests__/actions/access-summary.test.ts create mode 100644 scripts/__tests__/actions/finalize-membership-changes.test.ts create mode 100644 scripts/__tests__/actions/update-inactive-members.test.ts create mode 100644 scripts/src/actions/finalize-membership-changes.ts create mode 100644 scripts/src/actions/shared/access-summary.ts create mode 100644 scripts/src/actions/update-inactive-members.ts diff --git a/.github/workflows/finalize-membership-changes.yml b/.github/workflows/finalize-membership-changes.yml new file mode 100644 index 0000000..1aa3589 --- /dev/null +++ b/.github/workflows/finalize-membership-changes.yml @@ -0,0 +1,99 @@ +name: Finalize Membership Changes + +on: + workflow_dispatch: + inputs: + organization: + description: Organization whose membership changes should be finalized + required: true + mode: + description: Which potential member changes to apply + required: true + default: both + type: choice + options: + - both + - convert-potential-outside-collaborators + - remove-potential-no-members + ignore: + description: Comma, space, or newline separated usernames to ignore + required: false + only: + description: Comma, space, or newline separated usernames to target + required: false + +defaults: + run: + shell: bash + +jobs: + preview: + permissions: + contents: read + name: Preview membership changes + runs-on: ubuntu-latest + outputs: + affected-users-json: ${{ steps.preview.outputs.affected-users-json }} + env: + TF_WORKSPACE: ${{ github.event.inputs.organization }} + steps: + - name: Checkout + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - name: Install pnpm + uses: pnpm/action-setup@91ab88e2619ed1f46221f0ba42d1492c02baf788 # v6.0.6 + with: + version: 10 + - name: Use Node.js lts/* + uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 + with: + node-version: lts/* + cache: '' + - name: Initialize scripts + run: pnpm install --frozen-lockfile && pnpm run build + working-directory: scripts + - name: Preview membership changes + id: preview + run: node lib/actions/finalize-membership-changes.js + working-directory: scripts + env: + MODE: ${{ github.event.inputs.mode }} + IGNORE: ${{ github.event.inputs.ignore }} + ONLY: ${{ github.event.inputs.only }} + APPLY: 'false' + + apply: + needs: [preview] + if: needs.preview.outputs.affected-users-json != '[]' + permissions: + contents: read + name: Apply membership changes + runs-on: ubuntu-latest + environment: membership-write + env: + GITHUB_APP_ID: ${{ secrets.RW_GITHUB_APP_ID }} + GITHUB_APP_INSTALLATION_ID: ${{ secrets[format('RW_GITHUB_APP_INSTALLATION_ID_{0}', github.event.inputs.organization)] || secrets.RW_GITHUB_APP_INSTALLATION_ID }} + GITHUB_APP_PEM_FILE: ${{ secrets.RW_GITHUB_APP_PEM_FILE }} + TF_WORKSPACE: ${{ github.event.inputs.organization }} + steps: + - name: Checkout + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - name: Install pnpm + uses: pnpm/action-setup@91ab88e2619ed1f46221f0ba42d1492c02baf788 # v6.0.6 + with: + version: 10 + - name: Use Node.js lts/* + uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 + with: + node-version: lts/* + cache: '' + - name: Initialize scripts + run: pnpm install --frozen-lockfile && pnpm run build + working-directory: scripts + - name: Apply membership changes + run: node lib/actions/finalize-membership-changes.js + working-directory: scripts + env: + MODE: ${{ github.event.inputs.mode }} + IGNORE: ${{ github.event.inputs.ignore }} + ONLY: ${{ github.event.inputs.only }} + APPLY: 'true' diff --git a/.github/workflows/update-inactive-members.yml b/.github/workflows/update-inactive-members.yml new file mode 100644 index 0000000..678e402 --- /dev/null +++ b/.github/workflows/update-inactive-members.yml @@ -0,0 +1,119 @@ +name: Update Inactive Members + +on: + workflow_dispatch: + inputs: + organization: + description: Organization config to update + required: true + cutoff-date: + description: Inactive before this date (YYYY-MM-DD) + required: false + limit: + description: Remove up to this many longest-inactive members + required: false + ignore: + description: Comma, space, or newline separated usernames to ignore + required: false + only: + description: Comma, space, or newline separated usernames to target + required: false + public-repo-access: + description: Whether to retain effective access to public repositories + required: true + default: retain + type: choice + options: + - retain + - remove + +defaults: + run: + shell: bash + +jobs: + update: + permissions: + contents: write + pull-requests: write + name: Update inactive members + runs-on: ubuntu-latest + environment: push + env: + GITHUB_APP_ID: ${{ secrets.RO_GITHUB_APP_ID }} + GITHUB_APP_INSTALLATION_ID: ${{ secrets[format('RO_GITHUB_APP_INSTALLATION_ID_{0}', github.event.inputs.organization)] || secrets.RO_GITHUB_APP_INSTALLATION_ID }} + GITHUB_APP_PEM_FILE: ${{ secrets.RO_GITHUB_APP_PEM_FILE }} + TF_WORKSPACE: ${{ github.event.inputs.organization }} + steps: + - name: Checkout + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - name: Install pnpm + uses: pnpm/action-setup@91ab88e2619ed1f46221f0ba42d1492c02baf788 # v6.0.6 + with: + version: 10 + - name: Use Node.js lts/* + uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 + with: + node-version: lts/* + cache: '' + - name: Initialize scripts + run: pnpm install --frozen-lockfile && pnpm run build + working-directory: scripts + - name: Update inactive members + id: update + run: node lib/actions/update-inactive-members.js + working-directory: scripts + env: + CUTOFF_DATE: ${{ github.event.inputs['cutoff-date'] }} + LIMIT: ${{ github.event.inputs.limit }} + IGNORE: ${{ github.event.inputs.ignore }} + ONLY: ${{ github.event.inputs.only }} + PUBLIC_REPO_ACCESS: ${{ github.event.inputs['public-repo-access'] }} + - name: Check if organization config was modified + id: config-modified + env: + ORGANIZATION: ${{ github.event.inputs.organization }} + run: | + if [ -z "$(git status --porcelain -- "github/${ORGANIZATION}.yml")" ]; then + echo "this=false" >> $GITHUB_OUTPUT + else + echo "this=true" >> $GITHUB_OUTPUT + fi + - uses: ./.github/actions/git-config-user + if: steps.config-modified.outputs.this == 'true' + - name: Create draft pull request + if: steps.config-modified.outputs.this == 'true' + env: + ORGANIZATION: ${{ github.event.inputs.organization }} + CUTOFF_DATE: ${{ github.event.inputs['cutoff-date'] }} + LIMIT: ${{ github.event.inputs.limit }} + IGNORE: ${{ github.event.inputs.ignore }} + ONLY: ${{ github.event.inputs.only }} + PUBLIC_REPO_ACCESS: ${{ github.event.inputs['public-repo-access'] }} + AFFECTED_USERS: ${{ steps.update.outputs.affected-users }} + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + branch="inactive-members-${ORGANIZATION}-${GITHUB_RUN_ID}" + body="$(mktemp)" + { + echo 'The changes in this PR were made by a bot. Please review carefully.' + echo + echo "Organization: ${ORGANIZATION}" + echo "Cutoff date: ${CUTOFF_DATE:-not set}" + echo "Limit: ${LIMIT:-not set}" + echo "Ignore: ${IGNORE:-not set}" + echo "Only: ${ONLY:-not set}" + echo "Public repo access: ${PUBLIC_REPO_ACCESS}" + echo "Affected users: ${AFFECTED_USERS:-none}" + } > "${body}" + + git checkout -B "${branch}" + git add "github/${ORGANIZATION}.yml" + git commit -m "update-inactive-members@${GITHUB_RUN_ID} ${ORGANIZATION}" + git push origin "${branch}" --force + gh pr create \ + --draft \ + --title "Update inactive members for ${ORGANIZATION}" \ + --body-file "${body}" \ + --head "${branch}" \ + --base "${GITHUB_REF_NAME}" diff --git a/docs/SETUP.md b/docs/SETUP.md index 85cc586..03bedc6 100644 --- a/docs/SETUP.md +++ b/docs/SETUP.md @@ -110,13 +110,14 @@ - `Pull requests`: `Read & Write` - `Workflows`: `Read & Write` - `Organization permissions` - - `Members`: `Read & Write` + - `Members`: `Read & Write` (required for Terraform membership management and the `Finalize Membership Changes` workflow) - [ ] [Install the GitHub Apps](https://docs.github.com/en/developers/apps/managing-github-apps/installing-github-apps) in the GitHub organization for `All repositories` ## GitHub Actions Environments and Secrets -- [ ] Create GitHub Actions environments named `read`, `write`, and `push`, and configure protection rules such as required reviewers. Workflows that read organization state reference `read`; workflows that write organization state reference `write`; workflows that push generated changes to the GitHub Management repository reference `push`. +- [ ] Create GitHub Actions environments named `read`, `write`, `push`, and `membership-write`, and configure protection rules such as required reviewers. Workflows that read organization state reference `read`; workflows that write organization state reference `write`; workflows that push generated changes to the GitHub Management repository reference `push`; workflows that directly convert or remove organization members through the GitHub API reference `membership-write`. +- [ ] Configure the `membership-write` environment with required reviewers. Treat approval of this environment as approval to call the GitHub API for every user listed by the `Finalize Membership Changes` workflow preview job. - [ ] [Create encrypted secrets](https://docs.github.com/en/actions/security-guides/encrypted-secrets#creating-encrypted-secrets-for-an-organization) for the GitHub organization and allow the repository to access them (\*replace `$GITHUB_ORGANIZATION_NAME` with the GitHub organization name) - *these secrets are read by the GitHub Action workflows* - [ ] Go to `https://github.com/organizations/$GITHUB_ORGANIZATION_NAME/settings/apps/$GITHUB_APP_NAME` and copy the `App ID` - [ ] `RO_GITHUB_APP_ID` @@ -150,6 +151,13 @@ - [ ] Follow [How to synchronize GitHub Management with GitHub?](HOWTOS.md#synchronize-github-management-with-github) to commit the terraform lock and initialize terraform state +## Inactive Member Workflows + +- [ ] Use `Update Inactive Members` to create a draft PR that removes selected members from teams and repository collaborators in `github/$ORGANIZATION_NAME.yml`. The workflow requires either `cutoff-date` or `only`, supports `ignore` and `limit`, and can retain effective public repository access by converting that access to direct public repository collaborators in the YAML config. +- [ ] Review and merge the draft PR through the normal GitHub Management PR flow. +- [ ] Use `Finalize Membership Changes` only after the YAML PR has landed and the preview job lists the expected users. The `membership-write` environment approval gates API calls that convert potential outside collaborators or remove potential no members. +- [ ] After `Finalize Membership Changes` completes, run `Sync` for the same organization so Terraform state and YAML config reflect the membership changes made through the GitHub API. + ## GitHub Management Repository Protections *NOTE*: Advanced users might have to skip/adjust this step if they are not managing some of the arguments/attributes mentioned here with GitHub Management. diff --git a/scripts/__tests__/actions/access-summary.test.ts b/scripts/__tests__/actions/access-summary.test.ts new file mode 100644 index 0000000..a59aa6e --- /dev/null +++ b/scripts/__tests__/actions/access-summary.test.ts @@ -0,0 +1,123 @@ +import 'reflect-metadata' + +import assert from 'node:assert' +import {describe, it} from 'node:test' +import {Config} from '../../src/yaml/config.js' +import {State} from '../../src/terraform/state.js' +import { + categorizeAccessSummary, + getAccessSummaryFrom +} from '../../src/actions/shared/access-summary.js' +import { + describeAccessChanges, + describeAccessReport +} from '../../src/actions/shared/describe-access-changes.js' +import {StateSchema} from '../../src/terraform/schema.js' + +describe('access summaries', () => { + it('categorizes post-change users', () => { + const config = new Config(` +members: + member: + - alice + - carol + - dave + - kept # KEEP: manual exception +repositories: + private-repo: + collaborators: + pull: + - outside + visibility: private + public-repo: + collaborators: + pull: + - alice + visibility: public + team-repo: + teams: + push: + - maintainers + visibility: public +teams: + maintainers: + members: + member: + - dave +`) + + const categories = categorizeAccessSummary(getAccessSummaryFrom(config)) + + assert.deepEqual(categories.outsideCollaborators, ['outside']) + assert.deepEqual(categories.potentialOutsideCollaborators, ['alice']) + assert.deepEqual(categories.potentialNoMembers, ['carol']) + assert.deepEqual(categories.anyOtherMembers, ['dave', 'kept']) + }) + + it('annotates repository visibility in access changes and summaries', () => { + const state = new State( + JSON.stringify({ + values: { + root_module: { + resources: [ + { + mode: 'managed', + index: 'alice', + address: 'github_membership.this["alice"]', + type: 'github_membership', + values: { + username: 'alice', + role: 'member' + } + }, + { + mode: 'managed', + index: 'public-repo', + address: 'github_repository.this["public-repo"]', + type: 'github_repository', + values: { + name: 'public-repo', + visibility: 'public' + } + }, + { + mode: 'managed', + index: 'public-repo:alice', + address: + 'github_repository_collaborator.this["public-repo:alice"]', + type: 'github_repository_collaborator', + values: { + repository: 'public-repo', + username: 'alice', + permission: 'pull' + } + } + ] + } + } + } satisfies StateSchema) + ) + const config = new Config(` +members: + member: + - alice +repositories: + public-repo: + collaborators: + push: + - alice + visibility: public +`) + + const changes = describeAccessChanges(state, config) + const report = describeAccessReport(state, config) + + assert.match( + changes, + /will have the permission to public-repo \(public\) change from pull to push/ + ) + assert.match(report, /Potential outside collaborators<\/summary>/) + assert.match(report, /Affected users: alice/) + assert.match(report, /has push permission to public-repo \(public\)/) + }) +}) diff --git a/scripts/__tests__/actions/finalize-membership-changes.test.ts b/scripts/__tests__/actions/finalize-membership-changes.test.ts new file mode 100644 index 0000000..5ac2819 --- /dev/null +++ b/scripts/__tests__/actions/finalize-membership-changes.test.ts @@ -0,0 +1,43 @@ +import 'reflect-metadata' + +import assert from 'node:assert' +import {describe, it} from 'node:test' +import {Config} from '../../src/yaml/config.js' +import { + parseFinalizeMembershipMode, + planFinalizeMembershipChanges +} from '../../src/actions/finalize-membership-changes.js' + +describe('finalize membership changes', () => { + it('validates mode input', () => { + assert.equal(parseFinalizeMembershipMode('both'), 'both') + assert.throws(() => parseFinalizeMembershipMode('invalid')) + }) + + it('plans filtered conversion and removal targets', () => { + const config = new Config(` +members: + member: + - outside-candidate + - no-member-candidate + - ignored +repositories: + public-repo: + collaborators: + pull: + - outside-candidate + - ignored + visibility: public +`) + + const plan = planFinalizeMembershipChanges( + config, + 'both', + ['ignored'], + ['outside-candidate', 'no-member-candidate', 'ignored'] + ) + + assert.deepEqual(plan.potentialOutsideCollaborators, ['outside-candidate']) + assert.deepEqual(plan.potentialNoMembers, ['no-member-candidate']) + }) +}) diff --git a/scripts/__tests__/actions/update-inactive-members.test.ts b/scripts/__tests__/actions/update-inactive-members.test.ts new file mode 100644 index 0000000..a954997 --- /dev/null +++ b/scripts/__tests__/actions/update-inactive-members.test.ts @@ -0,0 +1,157 @@ +import 'reflect-metadata' + +import assert from 'node:assert' +import {describe, it} from 'node:test' +import {Config} from '../../src/yaml/config.js' +import { + parseCutoffDate, + parseLimit, + selectInactiveMembers, + updateInactiveMembersConfig +} from '../../src/actions/update-inactive-members.js' +import {TeamMember} from '../../src/resources/team-member.js' +import {RepositoryCollaborator} from '../../src/resources/repository-collaborator.js' + +describe('update inactive members', () => { + it('requires cutoff date or only list', () => { + const config = new Config('members:\n member:\n - alice\n') + + assert.throws(() => + selectInactiveMembers(config, [], { + ignore: [], + only: [], + publicRepoAccess: 'retain' + }) + ) + }) + + it('validates workflow inputs', () => { + assert.equal( + parseCutoffDate('2025-01-02')?.toISOString(), + '2025-01-02T00:00:00.000Z' + ) + assert.equal(parseLimit('2'), 2) + assert.throws(() => parseCutoffDate('01-02-2025')) + assert.throws(() => parseLimit('0')) + }) + + it('selects inactive members with only, ignore, limit, and KEEP handling', () => { + const config = new Config(` +members: + member: + - active + - ignored + - kept # KEEP: manual exception + - manual + - never-active + - old +`) + + const selected = selectInactiveMembers( + config, + [ + {username: 'active', latestActivity: new Date('2025-01-01T00:00:00Z')}, + {username: 'old', latestActivity: new Date('2023-01-01T00:00:00Z')} + ], + { + cutoffDate: new Date('2024-01-01T00:00:00Z'), + limit: 2, + ignore: ['ignored'], + only: ['active', 'ignored', 'kept', 'manual', 'never-active', 'old'], + publicRepoAccess: 'retain' + } + ) + + assert.deepEqual(selected, ['manual', 'never-active']) + }) + + it('retains effective public repository access when configured', () => { + const config = new Config(` +members: + member: + - alice +repositories: + private-repo: + collaborators: + pull: + - alice + teams: + admin: + - maintainers + visibility: private + public-repo: + collaborators: + pull: + - alice + teams: + push: + - maintainers + visibility: public +teams: + maintainers: + members: + member: + - alice +`) + + updateInactiveMembersConfig(config, ['alice'], 'retain') + + assert.equal( + config + .getResources(TeamMember) + .some(teamMember => teamMember.username === 'alice'), + false + ) + assert.equal( + config + .getResources(RepositoryCollaborator) + .some( + collaborator => + collaborator.username === 'alice' && + collaborator.repository === 'private-repo' + ), + false + ) + + const publicCollaborator = config + .getResources(RepositoryCollaborator) + .find( + collaborator => + collaborator.username === 'alice' && + collaborator.repository === 'public-repo' + ) + + assert.equal(publicCollaborator?.permission, 'push') + }) + + it('removes public repository access when configured', () => { + const config = new Config(` +members: + member: + - alice +repositories: + public-repo: + collaborators: + pull: + - alice + teams: + push: + - maintainers + visibility: public +teams: + maintainers: + members: + member: + - alice +`) + + updateInactiveMembersConfig(config, ['alice'], 'remove') + + assert.equal( + config + .getResources(RepositoryCollaborator) + .some(collaborator => collaborator.username === 'alice'), + false + ) + }) +}) diff --git a/scripts/src/actions/finalize-membership-changes.ts b/scripts/src/actions/finalize-membership-changes.ts new file mode 100644 index 0000000..b86bcd6 --- /dev/null +++ b/scripts/src/actions/finalize-membership-changes.ts @@ -0,0 +1,131 @@ +import 'reflect-metadata' +import * as core from '@actions/core' +import {pathToFileURL} from 'url' +import {Config} from '../yaml/config.js' +import {GitHub} from '../github.js' +import { + categorizeAccessSummary, + getAccessSummaryFrom, + parseUserList +} from './shared/access-summary.js' + +export type FinalizeMembershipMode = + | 'convert-potential-outside-collaborators' + | 'remove-potential-no-members' + | 'both' + +export type FinalizeMembershipPlan = { + potentialOutsideCollaborators: string[] + potentialNoMembers: string[] +} + +export function parseFinalizeMembershipMode( + source?: string +): FinalizeMembershipMode { + if ( + source === 'convert-potential-outside-collaborators' || + source === 'remove-potential-no-members' || + source === 'both' + ) { + return source + } + throw new Error( + 'mode must be convert-potential-outside-collaborators, remove-potential-no-members, or both' + ) +} + +export function planFinalizeMembershipChanges( + config: Config, + mode: FinalizeMembershipMode, + ignore: string[], + only: string[] +): FinalizeMembershipPlan { + const ignoredUsers = new Set(ignore) + const onlyUsers = new Set(only) + const categories = categorizeAccessSummary(getAccessSummaryFrom(config)) + + const filter = (users: string[]): string[] => + users + .filter(username => !ignoredUsers.has(username)) + .filter(username => onlyUsers.size === 0 || onlyUsers.has(username)) + .sort() + + return { + potentialOutsideCollaborators: + mode === 'remove-potential-no-members' + ? [] + : filter(categories.potentialOutsideCollaborators), + potentialNoMembers: + mode === 'convert-potential-outside-collaborators' + ? [] + : filter(categories.potentialNoMembers) + } +} + +export function formatFinalizeMembershipPlan( + plan: FinalizeMembershipPlan +): string { + return [ + 'Potential outside collaborators to convert:', + plan.potentialOutsideCollaborators.length > 0 + ? plan.potentialOutsideCollaborators + .map(username => `- ${username}`) + .join('\n') + : '- none', + '', + 'Potential no members to remove:', + plan.potentialNoMembers.length > 0 + ? plan.potentialNoMembers.map(username => `- ${username}`).join('\n') + : '- none' + ].join('\n') +} + +async function run(): Promise { + const mode = parseFinalizeMembershipMode(process.env.MODE || 'both') + const ignore = parseUserList(process.env.IGNORE) + const only = parseUserList(process.env.ONLY) + const shouldApply = process.env.APPLY === 'true' + const config = Config.FromPath() + + const plan = planFinalizeMembershipChanges(config, mode, ignore, only) + const affectedUsers = Array.from( + new Set([...plan.potentialOutsideCollaborators, ...plan.potentialNoMembers]) + ).sort() + + core.info(formatFinalizeMembershipPlan(plan)) + core.setOutput('affected-users', affectedUsers.join(', ')) + core.setOutput('affected-users-json', JSON.stringify(affectedUsers)) + core.setOutput( + 'potential-outside-collaborators-json', + JSON.stringify(plan.potentialOutsideCollaborators) + ) + core.setOutput( + 'potential-no-members-json', + JSON.stringify(plan.potentialNoMembers) + ) + + if (!shouldApply) { + return + } + + const github = await GitHub.getGitHub() + + for (const username of plan.potentialOutsideCollaborators) { + await github.convertMemberToOutsideCollaborator(username) + } + + for (const username of plan.potentialNoMembers) { + await github.removeOrganizationMembership(username) + } + + core.notice( + 'Membership changes are complete. Run the Sync workflow for this organization so Terraform state and YAML config reflect GitHub.' + ) +} + +if ( + process.argv[1] && + import.meta.url === pathToFileURL(process.argv[1]).href +) { + run() +} diff --git a/scripts/src/actions/fix-yaml-config.ts b/scripts/src/actions/fix-yaml-config.ts index a88b191..733d370 100644 --- a/scripts/src/actions/fix-yaml-config.ts +++ b/scripts/src/actions/fix-yaml-config.ts @@ -10,18 +10,7 @@ async function run(): Promise { const accessChangesDescription = await runDescribeAccessChanges() - core.setOutput( - 'comment', - `The following access changes will be introduced as a result of applying the plan: - -
Access Changes - -\`\`\` -${accessChangesDescription} -\`\`\` - -
` - ) + core.setOutput('comment', accessChangesDescription) } run() diff --git a/scripts/src/actions/shared/access-summary.ts b/scripts/src/actions/shared/access-summary.ts new file mode 100644 index 0000000..5c5c112 --- /dev/null +++ b/scripts/src/actions/shared/access-summary.ts @@ -0,0 +1,299 @@ +import {Config} from '../../yaml/config.js' +import {State} from '../../terraform/state.js' +import {RepositoryCollaborator} from '../../resources/repository-collaborator.js' +import {Member} from '../../resources/member.js' +import {TeamMember} from '../../resources/team-member.js' +import {RepositoryTeam} from '../../resources/repository-team.js' +import {Repository, Visibility} from '../../resources/repository.js' + +export type RepositoryAccess = { + permission: string + visibility: Visibility +} + +export type UserAccess = { + role?: string + repositories: Record + directRepositories: Record + teams: string[] + hasKeepComment: boolean +} + +export type AccessSummary = Record + +export type AccessCategory = + | 'outsideCollaborators' + | 'potentialOutsideCollaborators' + | 'potentialNoMembers' + | 'anyOtherMembers' + +export type AccessCategories = Record + +export const permissions = ['admin', 'maintain', 'push', 'triage', 'pull'] + +export function betterPermission(current: string, next: string): string { + return permissions.indexOf(next) < permissions.indexOf(current) + ? next + : current +} + +export function parseUserList(source?: string): string[] { + return Array.from( + new Set( + (source || '') + .split(/[\s,]+/) + .map(username => username.trim().toLowerCase()) + .filter(username => username !== '') + ) + ).sort() +} + +export function hasKeepComment(config: Config, member: Member): boolean { + const node = config.document.getIn( + member.getSchemaPath(config.get()), + true + ) as {comment?: string} | undefined + return node?.comment?.includes('KEEP:') ?? false +} + +export function getAccessSummaryFrom(source: State | Config): AccessSummary { + const members = source.getResources(Member) + const teamMembers = source.getResources(TeamMember) + const teamRepositories = source.getResources(RepositoryTeam) + const repositoryCollaborators = source.getResources(RepositoryCollaborator) + const repositories = source.getResources(Repository) + + const repositoryVisibility = new Map( + repositories.map(repository => [ + repository.name.toLowerCase(), + repository.visibility ?? Visibility.Private + ]) + ) + const archivedRepositories = repositories + .filter(repository => repository.archived) + .map(repository => repository.name.toLowerCase()) + + const usernames = new Set([ + ...members.map(member => member.username.toLowerCase()), + ...teamMembers.map(teamMember => teamMember.username.toLowerCase()), + ...repositoryCollaborators.map(collaborator => + collaborator.username.toLowerCase() + ) + ]) + + const accessSummary: AccessSummary = {} + + for (const username of Array.from(usernames).sort()) { + const member = members.find( + candidate => candidate.username.toLowerCase() === username + ) + const role = member?.role + const teams = teamMembers + .filter(teamMember => teamMember.username.toLowerCase() === username) + .map(teamMember => teamMember.team.toLowerCase()) + .sort() + const repositoryCollaborator = repositoryCollaborators + .filter(collaborator => collaborator.username.toLowerCase() === username) + .filter( + collaborator => + !archivedRepositories.includes(collaborator.repository.toLowerCase()) + ) + const teamRepository = teamRepositories + .filter(repository => teams.includes(repository.team.toLowerCase())) + .filter( + repository => + !archivedRepositories.includes(repository.repository.toLowerCase()) + ) + + const repositories: Record = {} + const directRepositories: Record = {} + + for (const rc of repositoryCollaborator) { + const repository = rc.repository.toLowerCase() + const access = { + permission: rc.permission, + visibility: repositoryVisibility.get(repository) ?? Visibility.Private + } + directRepositories[repository] = directRepositories[repository] + ? { + ...access, + permission: betterPermission( + directRepositories[repository].permission, + access.permission + ) + } + : access + repositories[repository] = repositories[repository] + ? { + ...access, + permission: betterPermission( + repositories[repository].permission, + access.permission + ) + } + : access + } + + for (const tr of teamRepository) { + const repository = tr.repository.toLowerCase() + const access = { + permission: tr.permission, + visibility: repositoryVisibility.get(repository) ?? Visibility.Private + } + repositories[repository] = repositories[repository] + ? { + ...access, + permission: betterPermission( + repositories[repository].permission, + access.permission + ) + } + : access + } + + const hasKeep = + source instanceof Config && member !== undefined + ? hasKeepComment(source, member) + : false + + if ( + role !== undefined || + teams.length > 0 || + Object.keys(repositories).length > 0 + ) { + accessSummary[username] = { + role, + repositories, + directRepositories, + teams, + hasKeepComment: hasKeep + } + } + } + + return deepSort(accessSummary) +} + +export function getComparableAccessSummary(source: State | Config): Record< + string, + { + role?: string + repositories: Record + } +> { + return Object.fromEntries( + Object.entries(getAccessSummaryFrom(source)).map(([username, access]) => [ + username, + { + role: access.role, + repositories: Object.fromEntries( + Object.entries(access.repositories).map(([repository, value]) => [ + repository, + {permission: value.permission} + ]) + ) + } + ]) + ) +} + +export function categorizeAccessSummary( + summary: AccessSummary +): AccessCategories { + const categories: AccessCategories = { + outsideCollaborators: [], + potentialOutsideCollaborators: [], + potentialNoMembers: [], + anyOtherMembers: [] + } + + for (const [username, access] of Object.entries(summary)) { + const repositories = Object.values(access.repositories) + if (access.role === undefined) { + if (repositories.length > 0) { + categories.outsideCollaborators.push(username) + } + } else if ( + !access.hasKeepComment && + access.teams.length === 0 && + repositories.length > 0 && + repositories.every( + repository => repository.visibility === Visibility.Public + ) + ) { + categories.potentialOutsideCollaborators.push(username) + } else if (!access.hasKeepComment && repositories.length === 0) { + categories.potentialNoMembers.push(username) + } else { + categories.anyOtherMembers.push(username) + } + } + + for (const users of Object.values(categories)) { + users.sort() + } + + return categories +} + +export function formatRepositoryAccess( + repository: string, + access: RepositoryAccess +): string { + return `${repository} (${access.visibility})` +} + +export function formatAccessSummarySection( + title: string, + users: string[], + summary: AccessSummary +): string { + const lines = [ + `
${title}`, + '', + `Affected users: ${users.length > 0 ? users.join(', ') : 'none'}`, + '', + '```' + ] + + if (users.length === 0) { + lines.push('No users in this section') + } + + for (const username of users) { + const access = summary[username] + lines.push(`User ${username}:`) + const repositories = Object.entries(access.repositories) + if (repositories.length === 0) { + lines.push(' - has no repository access') + } else { + for (const [repository, repositoryAccess] of repositories) { + lines.push( + ` - has ${repositoryAccess.permission} permission to ${formatRepositoryAccess( + repository, + repositoryAccess + )}` + ) + } + } + } + + lines.push('```', '', '
') + return lines.join('\n') +} + +// deep sort object +// eslint-disable-next-line @typescript-eslint/no-explicit-any +function deepSort(obj: any): any { + if (Array.isArray(obj)) { + return obj.map(deepSort) + } else if (obj !== null && typeof obj === 'object') { + const sorted: Record = {} + for (const key of Object.keys(obj).sort()) { + sorted[key] = deepSort(obj[key]) + } + return sorted + } else { + return obj + } +} diff --git a/scripts/src/actions/shared/describe-access-changes.ts b/scripts/src/actions/shared/describe-access-changes.ts index df76407..93f602d 100644 --- a/scripts/src/actions/shared/describe-access-changes.ts +++ b/scripts/src/actions/shared/describe-access-changes.ts @@ -1,128 +1,87 @@ import {Config} from '../../yaml/config.js' import {State} from '../../terraform/state.js' -import {RepositoryCollaborator} from '../../resources/repository-collaborator.js' -import {Member} from '../../resources/member.js' -import {TeamMember} from '../../resources/team-member.js' -import {RepositoryTeam} from '../../resources/repository-team.js' import diff from 'deep-diff' import * as core from '@actions/core' -import {Repository} from '../../resources/repository.js' - -type AccessSummary = Record< - string, - { - role?: string - repositories: Record - } -> - -function getAccessSummaryFrom(source: State | Config): AccessSummary { - const members = source.getResources(Member) - const teamMembers = source.getResources(TeamMember) - const teamRepositories = source.getResources(RepositoryTeam) - const repositoryCollaborators = source.getResources(RepositoryCollaborator) - - const archivedRepositories = source - .getResources(Repository) - .filter(repository => repository.archived) - .map(repository => repository.name.toLowerCase()) - - const usernames = new Set([ - ...members.map(member => member.username.toLowerCase()), - ...repositoryCollaborators.map(collaborator => - collaborator.username.toLowerCase() - ) - ]) - - const accessSummary: AccessSummary = {} - const permissions = ['admin', 'maintain', 'push', 'triage', 'pull'] - - for (const username of usernames) { - const role = members.find( - member => member.username.toLowerCase() === username - )?.role - const teams = teamMembers - .filter(teamMember => teamMember.username.toLowerCase() === username) - .map(teamMember => teamMember.team.toLowerCase()) - const repositoryCollaborator = repositoryCollaborators - .filter(collaborator => collaborator.username.toLowerCase() === username) - .filter( - collaborator => - !archivedRepositories.includes(collaborator.repository.toLowerCase()) - ) - const teamRepository = teamRepositories - .filter(repository => teams.includes(repository.team.toLowerCase())) - .filter( - repository => - !archivedRepositories.includes(repository.repository.toLowerCase()) - ) - - const repositories: Record = {} - - for (const rc of repositoryCollaborator) { - const repository = rc.repository.toLowerCase() - repositories[repository] = repositories[repository] ?? {} - if ( - !repositories[repository].permission || - permissions.indexOf(rc.permission) < - permissions.indexOf(repositories[repository].permission) - ) { - repositories[repository].permission = rc.permission - } - } - - for (const tr of teamRepository) { - const repository = tr.repository.toLowerCase() - repositories[repository] = repositories[repository] ?? {} - if ( - !repositories[repository].permission || - permissions.indexOf(tr.permission) < - permissions.indexOf(repositories[repository].permission) - ) { - repositories[repository].permission = tr.permission - } - } - - if (role !== undefined || Object.keys(repositories).length > 0) { - accessSummary[username] = { - role, - repositories - } - } - } - - return deepSort(accessSummary) -} - -// deep sort object -// eslint-disable-next-line @typescript-eslint/no-explicit-any -function deepSort(obj: any): any { - if (Array.isArray(obj)) { - return obj.map(deepSort) - } else if (typeof obj === 'object') { - const sorted: Record = {} - for (const key of Object.keys(obj).sort()) { - sorted[key] = deepSort(obj[key]) - } - return sorted - } else { - return obj - } +import { + categorizeAccessSummary, + formatAccessSummarySection, + formatRepositoryAccess, + getAccessSummaryFrom, + getComparableAccessSummary, + RepositoryAccess +} from './access-summary.js' + +function repositoryLabel( + repository: string, + afterSummary: ReturnType, + beforeSummary: ReturnType +): string { + const access = + Object.values(afterSummary) + .map(user => user.repositories[repository]) + .find(Boolean) ?? + Object.values(beforeSummary) + .map(user => user.repositories[repository]) + .find(Boolean) ?? + ({permission: 'pull', visibility: 'private'} as RepositoryAccess) + + return formatRepositoryAccess(repository, access) } export async function runDescribeAccessChanges(): Promise { const state = await State.New() const config = Config.FromPath() - return await describeAccessChanges(state, config) + return describeAccessReport(state, config) } -export async function describeAccessChanges( - state: State, - config: Config -): Promise { - const before = getAccessSummaryFrom(state) +export function describeAccessReport(state: State, config: Config): string { + const accessChangesDescription = describeAccessChanges(state, config) const after = getAccessSummaryFrom(config) + const categories = categorizeAccessSummary(after) + + return [ + 'The following access changes will be introduced as a result of applying the plan:', + '', + '
Access Changes', + '', + '```', + accessChangesDescription, + '```', + '', + '
', + '', + formatAccessSummarySection( + 'Outside collaborators', + categories.outsideCollaborators, + after + ), + '', + formatAccessSummarySection( + 'Potential outside collaborators', + categories.potentialOutsideCollaborators, + after + ), + '', + formatAccessSummarySection( + 'Potential no members', + categories.potentialNoMembers, + after + ), + '', + formatAccessSummarySection( + 'Any other members', + categories.anyOtherMembers, + after + ) + ].join('\n') +} + +export function describeAccessChanges(state: State, config: Config): string { + const before = getComparableAccessSummary(state) + const after = getComparableAccessSummary(config) + const beforeWithVisibility = getAccessSummaryFrom(state) + const afterWithVisibility = getAccessSummaryFrom(config) core.info(JSON.stringify({before, after}, null, 2)) @@ -136,11 +95,10 @@ export async function describeAccessChanges( throw new Error(`Change ${change.kind} has no path`) } const path = change.path - changesByUser[path[0]] = changesByUser[path[0]] || [] - changesByUser[path[0]].push(change) + changesByUser[String(path[0])] = changesByUser[String(path[0])] || [] + changesByUser[String(path[0])].push(change) } - // iterate over changesByUser and build a description const lines = [] for (const [username, userChanges] of Object.entries(changesByUser)) { lines.push(`User ${username}:`) @@ -157,15 +115,20 @@ export async function describeAccessChanges( ` - will join the organization as a ${change.rhs} (remind them to accept the email invitation)` ) } else if (change.rhs === undefined) { - lines.push(` - will leave the organization`) + lines.push(' - will leave the organization') } else { lines.push( ` - will have the role in the organization change from ${change.lhs} to ${change.rhs}` ) } } else { + const repository = String(path[2]) lines.push( - ` - will have the permission to ${path[2]} change from ${change.lhs} to ${change.rhs}` + ` - will have the permission to ${repositoryLabel( + repository, + afterWithVisibility, + beforeWithVisibility + )} change from ${change.lhs} to ${change.rhs}` ) } break @@ -173,7 +136,7 @@ export async function describeAccessChanges( if (path.length === 1) { if (change.rhs.role) { lines.push( - ` - will join the organization as a ${change.rhs} (remind them to accept the email invitation)` + ` - will join the organization as a ${change.rhs.role} (remind them to accept the email invitation)` ) } if (change.rhs.repositories) { @@ -185,20 +148,29 @@ export async function describeAccessChanges( repositories )) { lines.push( - ` - will gain ${permission} permission to ${repository}` + ` - will gain ${permission} permission to ${repositoryLabel( + repository, + afterWithVisibility, + beforeWithVisibility + )}` ) } } } else { + const repository = String(path[2]) lines.push( - ` - will gain ${change.rhs.permission} permission to ${path[2]}` + ` - will gain ${change.rhs.permission} permission to ${repositoryLabel( + repository, + afterWithVisibility, + beforeWithVisibility + )}` ) } break case 'D': if (path.length === 1) { if (change.lhs.role) { - lines.push(` - will leave the organization`) + lines.push(' - will leave the organization') } if (change.lhs.repositories) { const repositories = change.lhs.repositories as unknown as Record< @@ -209,13 +181,22 @@ export async function describeAccessChanges( repositories )) { lines.push( - ` - will lose ${permission} permission to ${repository}` + ` - will lose ${permission} permission to ${repositoryLabel( + repository, + afterWithVisibility, + beforeWithVisibility + )}` ) } } } else { + const repository = String(path[2]) lines.push( - ` - will lose ${change.lhs.permission} permission to ${path[2]}` + ` - will lose ${change.lhs.permission} permission to ${repositoryLabel( + repository, + afterWithVisibility, + beforeWithVisibility + )}` ) } break diff --git a/scripts/src/actions/update-inactive-members.ts b/scripts/src/actions/update-inactive-members.ts new file mode 100644 index 0000000..f8bfdc2 --- /dev/null +++ b/scripts/src/actions/update-inactive-members.ts @@ -0,0 +1,302 @@ +import 'reflect-metadata' +import * as core from '@actions/core' +import {pathToFileURL} from 'url' +import {Config} from '../yaml/config.js' +import {Member} from '../resources/member.js' +import {Repository, Visibility} from '../resources/repository.js' +import { + Permission as RepositoryCollaboratorPermission, + RepositoryCollaborator +} from '../resources/repository-collaborator.js' +import {TeamMember} from '../resources/team-member.js' +import {GitHub} from '../github.js' +import { + betterPermission, + getAccessSummaryFrom, + hasKeepComment, + parseUserList +} from './shared/access-summary.js' + +export type PublicRepoAccess = 'retain' | 'remove' + +export type MemberActivity = { + username: string + latestActivity?: Date +} + +export type UpdateInactiveMembersOptions = { + cutoffDate?: Date + limit?: number + ignore: string[] + only: string[] + publicRepoAccess: PublicRepoAccess +} + +type ActivityRecord = { + username: string + createdAt: Date +} + +export function parseCutoffDate(source?: string): Date | undefined { + if (source === undefined || source.trim() === '') { + return undefined + } + if (!/^\d{4}-\d{2}-\d{2}$/.test(source)) { + throw new Error('cutoff-date must use YYYY-MM-DD format') + } + const date = new Date(`${source}T00:00:00.000Z`) + if (Number.isNaN(date.valueOf())) { + throw new Error('cutoff-date must be a valid date') + } + return date +} + +export function parseLimit(source?: string): number | undefined { + if (source === undefined || source.trim() === '') { + return undefined + } + const limit = Number(source) + if (!Number.isInteger(limit) || limit <= 0) { + throw new Error('limit must be a positive integer') + } + return limit +} + +export function parsePublicRepoAccess(source?: string): PublicRepoAccess { + if (source === 'retain' || source === 'remove') { + return source + } + throw new Error('public-repo-access must be retain or remove') +} + +export function selectInactiveMembers( + config: Config, + activities: MemberActivity[], + options: UpdateInactiveMembersOptions +): string[] { + if (options.cutoffDate === undefined && options.only.length === 0) { + throw new Error('Either cutoff-date or only must be provided') + } + + const activityByUsername = new Map( + activities.map(activity => [activity.username.toLowerCase(), activity]) + ) + const ignore = new Set(options.ignore) + const only = new Set(options.only) + + const candidates = config + .getResources(Member) + .filter(member => !hasKeepComment(config, member)) + .map(member => member.username.toLowerCase()) + .filter(username => !ignore.has(username)) + .filter(username => only.size === 0 || only.has(username)) + .filter(username => { + if (options.cutoffDate === undefined) { + return true + } + const latestActivity = activityByUsername.get(username)?.latestActivity + return latestActivity === undefined || latestActivity < options.cutoffDate + }) + .sort((a, b) => { + const aActivity = activityByUsername.get(a)?.latestActivity?.valueOf() + const bActivity = activityByUsername.get(b)?.latestActivity?.valueOf() + if (aActivity === undefined && bActivity === undefined) { + return a.localeCompare(b) + } + if (aActivity === undefined) { + return -1 + } + if (bActivity === undefined) { + return 1 + } + return aActivity - bActivity || a.localeCompare(b) + }) + + return options.limit === undefined + ? candidates + : candidates.slice(0, options.limit) +} + +export function updateInactiveMembersConfig( + config: Config, + usernames: string[], + publicRepoAccess: PublicRepoAccess +): string[] { + const targets = new Set(usernames.map(username => username.toLowerCase())) + const repositories = new Map( + config + .getResources(Repository) + .map(repository => [repository.name.toLowerCase(), repository]) + ) + const accessBefore = getAccessSummaryFrom(config) + const retainedPublicAccess = new Map< + string, + Map + >() + + if (publicRepoAccess === 'retain') { + for (const username of targets) { + const userAccess = accessBefore[username] + if (userAccess === undefined) { + continue + } + for (const [repository, access] of Object.entries( + userAccess.repositories + )) { + const configRepository = repositories.get(repository) + if ( + configRepository?.archived || + access.visibility !== Visibility.Public + ) { + continue + } + const retainedRepositories = + retainedPublicAccess.get(username) ?? new Map() + const current = retainedRepositories.get(repository) + retainedRepositories.set( + repository, + (current === undefined + ? access.permission + : betterPermission( + current, + access.permission + )) as RepositoryCollaboratorPermission + ) + retainedPublicAccess.set(username, retainedRepositories) + } + } + } + + for (const teamMember of config.getResources(TeamMember)) { + if (targets.has(teamMember.username.toLowerCase())) { + core.info(`Removing ${teamMember.username} from ${teamMember.team} team`) + config.removeResource(teamMember) + } + } + + for (const collaborator of config.getResources(RepositoryCollaborator)) { + if (targets.has(collaborator.username.toLowerCase())) { + core.info( + `Removing ${collaborator.username} from ${collaborator.repository} repository` + ) + config.removeResource(collaborator) + } + } + + for (const [username, retainedRepositories] of retainedPublicAccess) { + for (const [repository, permission] of retainedRepositories) { + core.info( + `Retaining ${username} ${permission} access to public repository ${repository}` + ) + config.addResource( + new RepositoryCollaborator(repository, username, permission) + ) + } + } + + return Array.from(targets).sort() +} + +function latestActivityByUser(activities: ActivityRecord[]): MemberActivity[] { + const latest = new Map() + for (const activity of activities) { + const username = activity.username.toLowerCase() + const previous = latest.get(username) + if (previous === undefined || activity.createdAt > previous) { + latest.set(username, activity.createdAt) + } + } + return Array.from(latest.entries()).map(([username, latestActivity]) => ({ + username, + latestActivity + })) +} + +async function collectActivities(since: Date): Promise { + const github = await GitHub.getGitHub() + const [ + githubRepositoryActivities, + githubRepositoryIssues, + githubRepositoryPullRequestReviewComments, + githubRepositoryIssueComments, + githubRepositoryCommitComments + ] = await Promise.all([ + github.listRepositoryActivities(since), + github.listRepositoryIssues(since), + github.listRepositoryPullRequestReviewComments(since), + github.listRepositoryIssueComments(since), + github.listRepositoryCommitComments(since) + ]) + + return latestActivityByUser( + [ + ...githubRepositoryActivities.map(({activity}) => ({ + username: activity.actor?.login, + createdAt: new Date(activity.timestamp) + })), + ...githubRepositoryIssues.map(({issue}) => ({ + username: issue.user?.login, + createdAt: new Date(issue.created_at) + })), + ...githubRepositoryPullRequestReviewComments.map(({comment}) => ({ + username: comment.user?.login, + createdAt: new Date(comment.created_at) + })), + ...githubRepositoryIssueComments.map(({comment}) => ({ + username: comment.user?.login, + createdAt: new Date(comment.created_at) + })), + ...githubRepositoryCommitComments.map(({comment}) => ({ + username: comment.user?.login, + createdAt: new Date(comment.created_at) + })) + ] + .filter( + ( + activity + ): activity is { + username: string + createdAt: Date + } => activity.username !== undefined + ) + .filter(activity => !Number.isNaN(activity.createdAt.valueOf())) + ) +} + +async function run(): Promise { + const cutoffDate = parseCutoffDate(process.env.CUTOFF_DATE) + const limit = parseLimit(process.env.LIMIT) + const ignore = parseUserList(process.env.IGNORE) + const only = parseUserList(process.env.ONLY) + const publicRepoAccess = parsePublicRepoAccess(process.env.PUBLIC_REPO_ACCESS) + + const config = Config.FromPath() + const activities = + cutoffDate === undefined + ? [] + : await collectActivities(limit === undefined ? cutoffDate : new Date(0)) + const selectedMembers = selectInactiveMembers(config, activities, { + cutoffDate, + limit, + ignore, + only, + publicRepoAccess + }) + const affectedUsers = updateInactiveMembersConfig( + config, + selectedMembers, + publicRepoAccess + ) + + config.save() + + core.setOutput('affected-users', affectedUsers.join(', ')) + core.setOutput('affected-users-json', JSON.stringify(affectedUsers)) +} + +if ( + process.argv[1] && + import.meta.url === pathToFileURL(process.argv[1]).href +) { + run() +} diff --git a/scripts/src/github.ts b/scripts/src/github.ts index 882e477..5fac482 100644 --- a/scripts/src/github.ts +++ b/scripts/src/github.ts @@ -587,4 +587,30 @@ export class GitHub { } return commitComments } + + async convertMemberToOutsideCollaborator(username: string): Promise { + core.info(`Converting ${username} to outside collaborator...`) + await this.client.request( + 'PUT /orgs/{org}/outside_collaborators/{username}', + { + org: env.GITHUB_ORG, + username, + async: false, + headers: { + 'X-GitHub-Api-Version': '2026-03-10' + } + } + ) + } + + async removeOrganizationMembership(username: string): Promise { + core.info(`Removing organization membership for ${username}...`) + await this.client.request('DELETE /orgs/{org}/memberships/{username}', { + org: env.GITHUB_ORG, + username, + headers: { + 'X-GitHub-Api-Version': '2026-03-10' + } + }) + } } diff --git a/scripts/src/terraform/schema.ts b/scripts/src/terraform/schema.ts index fded2c2..0f7b027 100644 --- a/scripts/src/terraform/schema.ts +++ b/scripts/src/terraform/schema.ts @@ -53,6 +53,8 @@ type ResourceSchema = { type: 'github_repository' values: { name: string + archived?: boolean + visibility?: 'private' | 'public' pages?: { source?: object[] }[] From e32fa783297cc0b68ef25c83e6f526d058499e76 Mon Sep 17 00:00:00 2001 From: Piotr Galar Date: Thu, 16 Jul 2026 18:04:00 +0100 Subject: [PATCH 02/13] Refine member update workflow --- .../workflows/finalize-membership-changes.yml | 99 ------------- ...nactive-members.yml => update-members.yml} | 16 +-- docs/SETUP.md | 11 +- .../__tests__/actions/access-summary.test.ts | 17 ++- .../finalize-membership-changes.test.ts | 43 ------ ...members.test.ts => update-members.test.ts} | 16 +-- .../actions/finalize-membership-changes.ts | 131 ------------------ scripts/src/actions/shared/access-summary.ts | 36 +++-- ...-inactive-members.ts => update-members.ts} | 12 +- scripts/src/github.ts | 26 ---- 10 files changed, 66 insertions(+), 341 deletions(-) delete mode 100644 .github/workflows/finalize-membership-changes.yml rename .github/workflows/{update-inactive-members.yml => update-members.yml} (91%) delete mode 100644 scripts/__tests__/actions/finalize-membership-changes.test.ts rename scripts/__tests__/actions/{update-inactive-members.test.ts => update-members.test.ts} (90%) delete mode 100644 scripts/src/actions/finalize-membership-changes.ts rename scripts/src/actions/{update-inactive-members.ts => update-members.ts} (96%) diff --git a/.github/workflows/finalize-membership-changes.yml b/.github/workflows/finalize-membership-changes.yml deleted file mode 100644 index 1aa3589..0000000 --- a/.github/workflows/finalize-membership-changes.yml +++ /dev/null @@ -1,99 +0,0 @@ -name: Finalize Membership Changes - -on: - workflow_dispatch: - inputs: - organization: - description: Organization whose membership changes should be finalized - required: true - mode: - description: Which potential member changes to apply - required: true - default: both - type: choice - options: - - both - - convert-potential-outside-collaborators - - remove-potential-no-members - ignore: - description: Comma, space, or newline separated usernames to ignore - required: false - only: - description: Comma, space, or newline separated usernames to target - required: false - -defaults: - run: - shell: bash - -jobs: - preview: - permissions: - contents: read - name: Preview membership changes - runs-on: ubuntu-latest - outputs: - affected-users-json: ${{ steps.preview.outputs.affected-users-json }} - env: - TF_WORKSPACE: ${{ github.event.inputs.organization }} - steps: - - name: Checkout - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - - name: Install pnpm - uses: pnpm/action-setup@91ab88e2619ed1f46221f0ba42d1492c02baf788 # v6.0.6 - with: - version: 10 - - name: Use Node.js lts/* - uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 - with: - node-version: lts/* - cache: '' - - name: Initialize scripts - run: pnpm install --frozen-lockfile && pnpm run build - working-directory: scripts - - name: Preview membership changes - id: preview - run: node lib/actions/finalize-membership-changes.js - working-directory: scripts - env: - MODE: ${{ github.event.inputs.mode }} - IGNORE: ${{ github.event.inputs.ignore }} - ONLY: ${{ github.event.inputs.only }} - APPLY: 'false' - - apply: - needs: [preview] - if: needs.preview.outputs.affected-users-json != '[]' - permissions: - contents: read - name: Apply membership changes - runs-on: ubuntu-latest - environment: membership-write - env: - GITHUB_APP_ID: ${{ secrets.RW_GITHUB_APP_ID }} - GITHUB_APP_INSTALLATION_ID: ${{ secrets[format('RW_GITHUB_APP_INSTALLATION_ID_{0}', github.event.inputs.organization)] || secrets.RW_GITHUB_APP_INSTALLATION_ID }} - GITHUB_APP_PEM_FILE: ${{ secrets.RW_GITHUB_APP_PEM_FILE }} - TF_WORKSPACE: ${{ github.event.inputs.organization }} - steps: - - name: Checkout - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - - name: Install pnpm - uses: pnpm/action-setup@91ab88e2619ed1f46221f0ba42d1492c02baf788 # v6.0.6 - with: - version: 10 - - name: Use Node.js lts/* - uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 - with: - node-version: lts/* - cache: '' - - name: Initialize scripts - run: pnpm install --frozen-lockfile && pnpm run build - working-directory: scripts - - name: Apply membership changes - run: node lib/actions/finalize-membership-changes.js - working-directory: scripts - env: - MODE: ${{ github.event.inputs.mode }} - IGNORE: ${{ github.event.inputs.ignore }} - ONLY: ${{ github.event.inputs.only }} - APPLY: 'true' diff --git a/.github/workflows/update-inactive-members.yml b/.github/workflows/update-members.yml similarity index 91% rename from .github/workflows/update-inactive-members.yml rename to .github/workflows/update-members.yml index 678e402..5eb34ca 100644 --- a/.github/workflows/update-inactive-members.yml +++ b/.github/workflows/update-members.yml @@ -1,4 +1,4 @@ -name: Update Inactive Members +name: Update Members on: workflow_dispatch: @@ -36,7 +36,7 @@ jobs: permissions: contents: write pull-requests: write - name: Update inactive members + name: Update members runs-on: ubuntu-latest environment: push env: @@ -55,13 +55,13 @@ jobs: uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 with: node-version: lts/* - cache: '' + cache: "" - name: Initialize scripts run: pnpm install --frozen-lockfile && pnpm run build working-directory: scripts - - name: Update inactive members + - name: Update members id: update - run: node lib/actions/update-inactive-members.js + run: node lib/actions/update-members.js working-directory: scripts env: CUTOFF_DATE: ${{ github.event.inputs['cutoff-date'] }} @@ -93,7 +93,7 @@ jobs: AFFECTED_USERS: ${{ steps.update.outputs.affected-users }} GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | - branch="inactive-members-${ORGANIZATION}-${GITHUB_RUN_ID}" + branch="update-members-${ORGANIZATION}-${GITHUB_RUN_ID}" body="$(mktemp)" { echo 'The changes in this PR were made by a bot. Please review carefully.' @@ -109,11 +109,11 @@ jobs: git checkout -B "${branch}" git add "github/${ORGANIZATION}.yml" - git commit -m "update-inactive-members@${GITHUB_RUN_ID} ${ORGANIZATION}" + git commit -m "update-members@${GITHUB_RUN_ID} ${ORGANIZATION}" git push origin "${branch}" --force gh pr create \ --draft \ - --title "Update inactive members for ${ORGANIZATION}" \ + --title "Update members for ${ORGANIZATION}" \ --body-file "${body}" \ --head "${branch}" \ --base "${GITHUB_REF_NAME}" diff --git a/docs/SETUP.md b/docs/SETUP.md index 03bedc6..fdca08a 100644 --- a/docs/SETUP.md +++ b/docs/SETUP.md @@ -110,14 +110,13 @@ - `Pull requests`: `Read & Write` - `Workflows`: `Read & Write` - `Organization permissions` - - `Members`: `Read & Write` (required for Terraform membership management and the `Finalize Membership Changes` workflow) + - `Members`: `Read & Write` - [ ] [Install the GitHub Apps](https://docs.github.com/en/developers/apps/managing-github-apps/installing-github-apps) in the GitHub organization for `All repositories` ## GitHub Actions Environments and Secrets -- [ ] Create GitHub Actions environments named `read`, `write`, `push`, and `membership-write`, and configure protection rules such as required reviewers. Workflows that read organization state reference `read`; workflows that write organization state reference `write`; workflows that push generated changes to the GitHub Management repository reference `push`; workflows that directly convert or remove organization members through the GitHub API reference `membership-write`. -- [ ] Configure the `membership-write` environment with required reviewers. Treat approval of this environment as approval to call the GitHub API for every user listed by the `Finalize Membership Changes` workflow preview job. +- [ ] Create GitHub Actions environments named `read`, `write`, and `push`, and configure protection rules such as required reviewers. Workflows that read organization state reference `read`; workflows that write organization state reference `write`; workflows that push generated changes to the GitHub Management repository reference `push`. - [ ] [Create encrypted secrets](https://docs.github.com/en/actions/security-guides/encrypted-secrets#creating-encrypted-secrets-for-an-organization) for the GitHub organization and allow the repository to access them (\*replace `$GITHUB_ORGANIZATION_NAME` with the GitHub organization name) - *these secrets are read by the GitHub Action workflows* - [ ] Go to `https://github.com/organizations/$GITHUB_ORGANIZATION_NAME/settings/apps/$GITHUB_APP_NAME` and copy the `App ID` - [ ] `RO_GITHUB_APP_ID` @@ -151,12 +150,10 @@ - [ ] Follow [How to synchronize GitHub Management with GitHub?](HOWTOS.md#synchronize-github-management-with-github) to commit the terraform lock and initialize terraform state -## Inactive Member Workflows +## Member Update Workflows -- [ ] Use `Update Inactive Members` to create a draft PR that removes selected members from teams and repository collaborators in `github/$ORGANIZATION_NAME.yml`. The workflow requires either `cutoff-date` or `only`, supports `ignore` and `limit`, and can retain effective public repository access by converting that access to direct public repository collaborators in the YAML config. +- [ ] Use `Update Members` to create a draft PR that removes selected members from teams and repository collaborators in `github/$ORGANIZATION_NAME.yml`. The workflow requires either `cutoff-date` or `only`, supports `ignore` and `limit`, and can retain effective public repository access by converting that access to direct public repository collaborators in the YAML config. - [ ] Review and merge the draft PR through the normal GitHub Management PR flow. -- [ ] Use `Finalize Membership Changes` only after the YAML PR has landed and the preview job lists the expected users. The `membership-write` environment approval gates API calls that convert potential outside collaborators or remove potential no members. -- [ ] After `Finalize Membership Changes` completes, run `Sync` for the same organization so Terraform state and YAML config reflect the membership changes made through the GitHub API. ## GitHub Management Repository Protections diff --git a/scripts/__tests__/actions/access-summary.test.ts b/scripts/__tests__/actions/access-summary.test.ts index a59aa6e..14e5854 100644 --- a/scripts/__tests__/actions/access-summary.test.ts +++ b/scripts/__tests__/actions/access-summary.test.ts @@ -34,20 +34,34 @@ repositories: pull: - alice visibility: public + team-only-repo: + teams: + push: + - guests + visibility: public team-repo: teams: push: - maintainers visibility: public teams: + guests: + members: + member: + - team-only-non-member maintainers: members: member: - dave `) - const categories = categorizeAccessSummary(getAccessSummaryFrom(config)) + const summary = getAccessSummaryFrom(config) + const categories = categorizeAccessSummary(summary) + assert.equal(summary.outside.isMember, false) + assert.equal(summary.outside.isOutsideCollaborator, true) + assert.equal(summary['team-only-non-member'].isMember, false) + assert.equal(summary['team-only-non-member'].isOutsideCollaborator, false) assert.deepEqual(categories.outsideCollaborators, ['outside']) assert.deepEqual(categories.potentialOutsideCollaborators, ['alice']) assert.deepEqual(categories.potentialNoMembers, ['carol']) @@ -118,6 +132,7 @@ repositories: ) assert.match(report, /Potential outside collaborators<\/summary>/) assert.match(report, /Affected users: alice/) + assert.match(report, /User alice \(member\):/) assert.match(report, /has push permission to public-repo \(public\)/) }) }) diff --git a/scripts/__tests__/actions/finalize-membership-changes.test.ts b/scripts/__tests__/actions/finalize-membership-changes.test.ts deleted file mode 100644 index 5ac2819..0000000 --- a/scripts/__tests__/actions/finalize-membership-changes.test.ts +++ /dev/null @@ -1,43 +0,0 @@ -import 'reflect-metadata' - -import assert from 'node:assert' -import {describe, it} from 'node:test' -import {Config} from '../../src/yaml/config.js' -import { - parseFinalizeMembershipMode, - planFinalizeMembershipChanges -} from '../../src/actions/finalize-membership-changes.js' - -describe('finalize membership changes', () => { - it('validates mode input', () => { - assert.equal(parseFinalizeMembershipMode('both'), 'both') - assert.throws(() => parseFinalizeMembershipMode('invalid')) - }) - - it('plans filtered conversion and removal targets', () => { - const config = new Config(` -members: - member: - - outside-candidate - - no-member-candidate - - ignored -repositories: - public-repo: - collaborators: - pull: - - outside-candidate - - ignored - visibility: public -`) - - const plan = planFinalizeMembershipChanges( - config, - 'both', - ['ignored'], - ['outside-candidate', 'no-member-candidate', 'ignored'] - ) - - assert.deepEqual(plan.potentialOutsideCollaborators, ['outside-candidate']) - assert.deepEqual(plan.potentialNoMembers, ['no-member-candidate']) - }) -}) diff --git a/scripts/__tests__/actions/update-inactive-members.test.ts b/scripts/__tests__/actions/update-members.test.ts similarity index 90% rename from scripts/__tests__/actions/update-inactive-members.test.ts rename to scripts/__tests__/actions/update-members.test.ts index a954997..e2aeef3 100644 --- a/scripts/__tests__/actions/update-inactive-members.test.ts +++ b/scripts/__tests__/actions/update-members.test.ts @@ -6,18 +6,18 @@ import {Config} from '../../src/yaml/config.js' import { parseCutoffDate, parseLimit, - selectInactiveMembers, - updateInactiveMembersConfig -} from '../../src/actions/update-inactive-members.js' + selectMembersForUpdate, + updateMembersConfig +} from '../../src/actions/update-members.js' import {TeamMember} from '../../src/resources/team-member.js' import {RepositoryCollaborator} from '../../src/resources/repository-collaborator.js' -describe('update inactive members', () => { +describe('update members', () => { it('requires cutoff date or only list', () => { const config = new Config('members:\n member:\n - alice\n') assert.throws(() => - selectInactiveMembers(config, [], { + selectMembersForUpdate(config, [], { ignore: [], only: [], publicRepoAccess: 'retain' @@ -47,7 +47,7 @@ members: - old `) - const selected = selectInactiveMembers( + const selected = selectMembersForUpdate( config, [ {username: 'active', latestActivity: new Date('2025-01-01T00:00:00Z')}, @@ -94,7 +94,7 @@ teams: - alice `) - updateInactiveMembersConfig(config, ['alice'], 'retain') + updateMembersConfig(config, ['alice'], 'retain') assert.equal( config @@ -145,7 +145,7 @@ teams: - alice `) - updateInactiveMembersConfig(config, ['alice'], 'remove') + updateMembersConfig(config, ['alice'], 'remove') assert.equal( config diff --git a/scripts/src/actions/finalize-membership-changes.ts b/scripts/src/actions/finalize-membership-changes.ts deleted file mode 100644 index b86bcd6..0000000 --- a/scripts/src/actions/finalize-membership-changes.ts +++ /dev/null @@ -1,131 +0,0 @@ -import 'reflect-metadata' -import * as core from '@actions/core' -import {pathToFileURL} from 'url' -import {Config} from '../yaml/config.js' -import {GitHub} from '../github.js' -import { - categorizeAccessSummary, - getAccessSummaryFrom, - parseUserList -} from './shared/access-summary.js' - -export type FinalizeMembershipMode = - | 'convert-potential-outside-collaborators' - | 'remove-potential-no-members' - | 'both' - -export type FinalizeMembershipPlan = { - potentialOutsideCollaborators: string[] - potentialNoMembers: string[] -} - -export function parseFinalizeMembershipMode( - source?: string -): FinalizeMembershipMode { - if ( - source === 'convert-potential-outside-collaborators' || - source === 'remove-potential-no-members' || - source === 'both' - ) { - return source - } - throw new Error( - 'mode must be convert-potential-outside-collaborators, remove-potential-no-members, or both' - ) -} - -export function planFinalizeMembershipChanges( - config: Config, - mode: FinalizeMembershipMode, - ignore: string[], - only: string[] -): FinalizeMembershipPlan { - const ignoredUsers = new Set(ignore) - const onlyUsers = new Set(only) - const categories = categorizeAccessSummary(getAccessSummaryFrom(config)) - - const filter = (users: string[]): string[] => - users - .filter(username => !ignoredUsers.has(username)) - .filter(username => onlyUsers.size === 0 || onlyUsers.has(username)) - .sort() - - return { - potentialOutsideCollaborators: - mode === 'remove-potential-no-members' - ? [] - : filter(categories.potentialOutsideCollaborators), - potentialNoMembers: - mode === 'convert-potential-outside-collaborators' - ? [] - : filter(categories.potentialNoMembers) - } -} - -export function formatFinalizeMembershipPlan( - plan: FinalizeMembershipPlan -): string { - return [ - 'Potential outside collaborators to convert:', - plan.potentialOutsideCollaborators.length > 0 - ? plan.potentialOutsideCollaborators - .map(username => `- ${username}`) - .join('\n') - : '- none', - '', - 'Potential no members to remove:', - plan.potentialNoMembers.length > 0 - ? plan.potentialNoMembers.map(username => `- ${username}`).join('\n') - : '- none' - ].join('\n') -} - -async function run(): Promise { - const mode = parseFinalizeMembershipMode(process.env.MODE || 'both') - const ignore = parseUserList(process.env.IGNORE) - const only = parseUserList(process.env.ONLY) - const shouldApply = process.env.APPLY === 'true' - const config = Config.FromPath() - - const plan = planFinalizeMembershipChanges(config, mode, ignore, only) - const affectedUsers = Array.from( - new Set([...plan.potentialOutsideCollaborators, ...plan.potentialNoMembers]) - ).sort() - - core.info(formatFinalizeMembershipPlan(plan)) - core.setOutput('affected-users', affectedUsers.join(', ')) - core.setOutput('affected-users-json', JSON.stringify(affectedUsers)) - core.setOutput( - 'potential-outside-collaborators-json', - JSON.stringify(plan.potentialOutsideCollaborators) - ) - core.setOutput( - 'potential-no-members-json', - JSON.stringify(plan.potentialNoMembers) - ) - - if (!shouldApply) { - return - } - - const github = await GitHub.getGitHub() - - for (const username of plan.potentialOutsideCollaborators) { - await github.convertMemberToOutsideCollaborator(username) - } - - for (const username of plan.potentialNoMembers) { - await github.removeOrganizationMembership(username) - } - - core.notice( - 'Membership changes are complete. Run the Sync workflow for this organization so Terraform state and YAML config reflect GitHub.' - ) -} - -if ( - process.argv[1] && - import.meta.url === pathToFileURL(process.argv[1]).href -) { - run() -} diff --git a/scripts/src/actions/shared/access-summary.ts b/scripts/src/actions/shared/access-summary.ts index 5c5c112..9561a38 100644 --- a/scripts/src/actions/shared/access-summary.ts +++ b/scripts/src/actions/shared/access-summary.ts @@ -13,6 +13,8 @@ export type RepositoryAccess = { export type UserAccess = { role?: string + isMember: boolean + isOutsideCollaborator: boolean repositories: Record directRepositories: Record teams: string[] @@ -156,13 +158,15 @@ export function getAccessSummaryFrom(source: State | Config): AccessSummary { ? hasKeepComment(source, member) : false - if ( - role !== undefined || - teams.length > 0 || - Object.keys(repositories).length > 0 - ) { + const isMember = role !== undefined + const isOutsideCollaborator = + !isMember && Object.keys(directRepositories).length > 0 + + if (isMember || isOutsideCollaborator || teams.length > 0) { accessSummary[username] = { role, + isMember, + isOutsideCollaborator, repositories, directRepositories, teams, @@ -209,11 +213,11 @@ export function categorizeAccessSummary( for (const [username, access] of Object.entries(summary)) { const repositories = Object.values(access.repositories) - if (access.role === undefined) { - if (repositories.length > 0) { - categories.outsideCollaborators.push(username) - } + const directRepositories = Object.values(access.directRepositories) + if (access.isOutsideCollaborator) { + categories.outsideCollaborators.push(username) } else if ( + access.isMember && !access.hasKeepComment && access.teams.length === 0 && repositories.length > 0 && @@ -222,9 +226,14 @@ export function categorizeAccessSummary( ) ) { categories.potentialOutsideCollaborators.push(username) - } else if (!access.hasKeepComment && repositories.length === 0) { + } else if ( + access.isMember && + !access.hasKeepComment && + directRepositories.length === 0 && + access.teams.length === 0 + ) { categories.potentialNoMembers.push(username) - } else { + } else if (access.isMember) { categories.anyOtherMembers.push(username) } } @@ -262,7 +271,10 @@ export function formatAccessSummarySection( for (const username of users) { const access = summary[username] - lines.push(`User ${username}:`) + const kind = access.isOutsideCollaborator + ? 'outside collaborator' + : 'member' + lines.push(`User ${username} (${kind}):`) const repositories = Object.entries(access.repositories) if (repositories.length === 0) { lines.push(' - has no repository access') diff --git a/scripts/src/actions/update-inactive-members.ts b/scripts/src/actions/update-members.ts similarity index 96% rename from scripts/src/actions/update-inactive-members.ts rename to scripts/src/actions/update-members.ts index f8bfdc2..25d649c 100644 --- a/scripts/src/actions/update-inactive-members.ts +++ b/scripts/src/actions/update-members.ts @@ -24,7 +24,7 @@ export type MemberActivity = { latestActivity?: Date } -export type UpdateInactiveMembersOptions = { +export type UpdateMembersOptions = { cutoffDate?: Date limit?: number ignore: string[] @@ -69,10 +69,10 @@ export function parsePublicRepoAccess(source?: string): PublicRepoAccess { throw new Error('public-repo-access must be retain or remove') } -export function selectInactiveMembers( +export function selectMembersForUpdate( config: Config, activities: MemberActivity[], - options: UpdateInactiveMembersOptions + options: UpdateMembersOptions ): string[] { if (options.cutoffDate === undefined && options.only.length === 0) { throw new Error('Either cutoff-date or only must be provided') @@ -117,7 +117,7 @@ export function selectInactiveMembers( : candidates.slice(0, options.limit) } -export function updateInactiveMembersConfig( +export function updateMembersConfig( config: Config, usernames: string[], publicRepoAccess: PublicRepoAccess @@ -275,14 +275,14 @@ async function run(): Promise { cutoffDate === undefined ? [] : await collectActivities(limit === undefined ? cutoffDate : new Date(0)) - const selectedMembers = selectInactiveMembers(config, activities, { + const selectedMembers = selectMembersForUpdate(config, activities, { cutoffDate, limit, ignore, only, publicRepoAccess }) - const affectedUsers = updateInactiveMembersConfig( + const affectedUsers = updateMembersConfig( config, selectedMembers, publicRepoAccess diff --git a/scripts/src/github.ts b/scripts/src/github.ts index 5fac482..882e477 100644 --- a/scripts/src/github.ts +++ b/scripts/src/github.ts @@ -587,30 +587,4 @@ export class GitHub { } return commitComments } - - async convertMemberToOutsideCollaborator(username: string): Promise { - core.info(`Converting ${username} to outside collaborator...`) - await this.client.request( - 'PUT /orgs/{org}/outside_collaborators/{username}', - { - org: env.GITHUB_ORG, - username, - async: false, - headers: { - 'X-GitHub-Api-Version': '2026-03-10' - } - } - ) - } - - async removeOrganizationMembership(username: string): Promise { - core.info(`Removing organization membership for ${username}...`) - await this.client.request('DELETE /orgs/{org}/memberships/{username}', { - org: env.GITHUB_ORG, - username, - headers: { - 'X-GitHub-Api-Version': '2026-03-10' - } - }) - } } From 8e36bb959444c28ebd43766c102c2042fec35d2a Mon Sep 17 00:00:00 2001 From: Piotr Galar Date: Thu, 16 Jul 2026 18:18:03 +0100 Subject: [PATCH 03/13] Guard repo and membership deletes --- .github/workflows/apply.yml | 59 +++++- .github/workflows/plan.yml | 65 ++++++- .gitignore | 3 + CHANGELOG.md | 1 + docs/ABOUT.md | 2 + docs/SETUP.md | 7 +- .../actions/classify-allow-destroy.test.ts | 181 ++++++++++++++++++ scripts/src/actions/classify-allow-destroy.ts | 120 ++++++++++++ terraform/allow_destroy_override.tf.disabled | 11 ++ 9 files changed, 438 insertions(+), 11 deletions(-) create mode 100644 scripts/__tests__/actions/classify-allow-destroy.test.ts create mode 100644 scripts/src/actions/classify-allow-destroy.ts create mode 100644 terraform/allow_destroy_override.tf.disabled diff --git a/.github/workflows/apply.yml b/.github/workflows/apply.yml index 6dcaca9..21b3665 100644 --- a/.github/workflows/apply.yml +++ b/.github/workflows/apply.yml @@ -47,19 +47,65 @@ jobs: GITHUB_APP_PEM_FILE: ${{ secrets.RO_GITHUB_APP_PEM_FILE }} run: node lib/actions/find-sha-for-plan.js working-directory: scripts - apply: + classify: needs: [prepare] if: needs.prepare.outputs.sha != '' && needs.prepare.outputs.workspaces != '' + permissions: + contents: read + name: Classify + runs-on: ubuntu-latest + environment: read + outputs: + matrix: ${{ steps.classify.outputs.matrix }} + env: + TF_IN_AUTOMATION: 1 + TF_INPUT: 0 + AWS_ACCESS_KEY_ID: ${{ secrets.RO_AWS_ACCESS_KEY_ID }} + AWS_SECRET_ACCESS_KEY: ${{ secrets.RO_AWS_SECRET_ACCESS_KEY }} + WORKSPACES: ${{ needs.prepare.outputs.workspaces }} + defaults: + run: + shell: bash + steps: + - name: Checkout + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - name: Setup terraform + uses: hashicorp/setup-terraform@5e8dbf3c6d9deaf4193ca7a8fb23f2ac83bb6c85 # v4.0.0 + with: + terraform_version: 1.12.0 + terraform_wrapper: false + - name: Initialize terraform + run: terraform init + working-directory: terraform + - name: Install pnpm + uses: pnpm/action-setup@91ab88e2619ed1f46221f0ba42d1492c02baf788 # v6.0.6 + with: + version: 10 + - name: Use Node.js lts/* + uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 + with: + node-version: lts/* + cache: '' + - run: pnpm install --frozen-lockfile && pnpm run build + working-directory: scripts + - name: Classify workspaces + id: classify + env: + MODE: write + run: node lib/actions/classify-allow-destroy.js + working-directory: scripts + apply: + needs: [prepare, classify] + if: needs.prepare.outputs.sha != '' && needs.prepare.outputs.workspaces != '' permissions: actions: read contents: read strategy: fail-fast: false - matrix: - workspace: ${{ fromJson(needs.prepare.outputs.workspaces) }} + matrix: ${{ fromJson(needs.classify.outputs.matrix) }} name: Apply runs-on: ubuntu-latest - environment: write + environment: ${{ matrix.environment }} env: TF_IN_AUTOMATION: 1 TF_INPUT: 0 @@ -84,6 +130,9 @@ jobs: terraform_wrapper: false - name: Initialize terraform run: terraform init + - name: Allow destroy in guarded environment + if: matrix.environment == 'write-allow-destroy' + run: cp allow_destroy_override.tf.disabled allow_destroy_override.tf - name: Download reviewed terraform plan env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} @@ -91,7 +140,7 @@ jobs: run: gh run download -n "${TF_WORKSPACE}_${SHA}.tfplan" --repo "${GITHUB_REPOSITORY}" - name: Replan merged commit run: | - terraform show -json > $TF_WORKSPACE.tfstate.json + terraform show -json > "$TF_WORKSPACE.tfstate.json" terraform plan -refresh=false -lock=false -out="${TF_WORKSPACE}.merged.tfplan" -no-color - name: Compare reviewed and merged plans run: | diff --git a/.github/workflows/plan.yml b/.github/workflows/plan.yml index 8b95d94..3dae02f 100644 --- a/.github/workflows/plan.yml +++ b/.github/workflows/plan.yml @@ -47,18 +47,69 @@ jobs: env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} timeout-minutes: 10 - plan: + classify: needs: [prepare] + permissions: + contents: read + pull-requests: read + name: Classify + runs-on: ubuntu-latest + environment: read + outputs: + matrix: ${{ steps.classify.outputs.matrix }} + env: + TF_IN_AUTOMATION: 1 + TF_INPUT: 0 + AWS_ACCESS_KEY_ID: ${{ secrets.RO_AWS_ACCESS_KEY_ID }} + AWS_SECRET_ACCESS_KEY: ${{ secrets.RO_AWS_SECRET_ACCESS_KEY }} + WORKSPACES: ${{ needs.prepare.outputs.workspaces }} + steps: + - name: Checkout + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - if: github.event_name == 'pull_request_target' + env: + NUMBER: ${{ github.event.pull_request.number }} + SHA: ${{ github.event.pull_request.head.sha }} + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + git fetch origin "pull/${NUMBER}/head" + rm -rf github && git checkout "${SHA}" -- github + - name: Setup terraform + uses: hashicorp/setup-terraform@5e8dbf3c6d9deaf4193ca7a8fb23f2ac83bb6c85 # v4.0.0 + with: + terraform_version: 1.12.0 + terraform_wrapper: false + - name: Initialize terraform + run: terraform init + working-directory: terraform + - name: Install pnpm + uses: pnpm/action-setup@91ab88e2619ed1f46221f0ba42d1492c02baf788 # v6.0.6 + with: + version: 10 + - name: Use Node.js lts/* + uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 + with: + node-version: lts/* + cache: '' + - run: pnpm install --frozen-lockfile && pnpm run build + working-directory: scripts + - name: Classify workspaces + id: classify + env: + MODE: read + run: node lib/actions/classify-allow-destroy.js + working-directory: scripts + plan: + needs: [prepare, classify] permissions: contents: read pull-requests: read strategy: fail-fast: false - matrix: - workspace: ${{ fromJson(needs.prepare.outputs.workspaces || '[]') }} + matrix: ${{ fromJson(needs.classify.outputs.matrix) }} name: Plan runs-on: ubuntu-latest - environment: read + environment: ${{ matrix.environment }} env: TF_IN_AUTOMATION: 1 TF_INPUT: 0 @@ -88,9 +139,13 @@ jobs: - name: Initialize terraform run: terraform init working-directory: terraform + - name: Allow destroy in guarded environment + if: matrix.environment == 'read-allow-destroy' + run: cp allow_destroy_override.tf.disabled allow_destroy_override.tf + working-directory: terraform - name: Plan terraform run: | - terraform show -json > $TF_WORKSPACE.tfstate.json + terraform show -json > "$TF_WORKSPACE.tfstate.json" terraform plan -refresh=false -lock=false -out="${TF_WORKSPACE}.tfplan" -no-color working-directory: terraform - name: Upload terraform plan diff --git a/.gitignore b/.gitignore index 34adc7e..356c12f 100644 --- a/.gitignore +++ b/.gitignore @@ -28,3 +28,6 @@ crash.log # Include tfplan files to ignore the plan output of command: terraform plan -out=tfplan # example: *tfplan* *.tfplan + +# Enabled only by guarded allow-destroy workflow jobs +terraform/allow_destroy_override.tf diff --git a/CHANGELOG.md b/CHANGELOG.md index 5549e1d..59bfc4a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added +- allow-destroy workflow environments for guarded repository and membership deletion plans/applies - shared action for adding a collaborator to all repositories - clean workflow which removes resources from state - information on how to handle private GitHub Management repository diff --git a/docs/ABOUT.md b/docs/ABOUT.md index c7a44ef..d442054 100644 --- a/docs/ABOUT.md +++ b/docs/ABOUT.md @@ -27,6 +27,8 @@ The workflow for introducing changes to GitHub via YAML configuration file is as 1. Review the plan. 1. Merge the PR and wait for the GitHub Action workflow triggered on pushes to the default branch to apply it. +Plans that remove managed repositories or organization memberships are routed through the `read-allow-destroy` GitHub Actions environment before the PR plan is created. The matching apply is routed through `write-allow-destroy`. These environments must be protected separately from the normal `read` and `write` environments. Outside the allow-destroy environments, Terraform keeps `prevent_destroy` enabled for repository and membership resources. + Neither creating the terraform plan nor applying it refreshes the underlying terraform state i.e. going through this workflow does **NOT** ask GitHub if the actual GitHub configuration state has changed. This makes the workflow fast and rate limit friendly because the number of requests to GitHub is minimised. This can result in the plan failing to be applied, e.g. if the underlying resource has been deleted. This assumes that YAML configuration is the main source of truth for GitHub configuration state. The plans that are created during the PR GitHub Action workflow are compared against plans regenerated from the merged commit before applying. The workflow for synchronising the current GitHub configuration state with YAML configuration file is as follows: diff --git a/docs/SETUP.md b/docs/SETUP.md index 85cc586..32cbe5f 100644 --- a/docs/SETUP.md +++ b/docs/SETUP.md @@ -116,7 +116,12 @@ ## GitHub Actions Environments and Secrets -- [ ] Create GitHub Actions environments named `read`, `write`, and `push`, and configure protection rules such as required reviewers. Workflows that read organization state reference `read`; workflows that write organization state reference `write`; workflows that push generated changes to the GitHub Management repository reference `push`. +- [ ] Create GitHub Actions environments named `read`, `read-allow-destroy`, `write`, `write-allow-destroy`, and `push`, and configure protection rules such as required reviewers. Workflows that read organization state reference `read`; workflows that write organization state reference `write`; workflows that push generated changes to the GitHub Management repository reference `push`. +- [ ] Configure `read-allow-destroy` and `write-allow-destroy` with stricter protection rules for repository and membership deletion plans/applies: + - [ ] Require reviewers + - [ ] Prevent self-review + - [ ] Restrict deployment branches to `master` + - [ ] Disable administrator bypass where available - [ ] [Create encrypted secrets](https://docs.github.com/en/actions/security-guides/encrypted-secrets#creating-encrypted-secrets-for-an-organization) for the GitHub organization and allow the repository to access them (\*replace `$GITHUB_ORGANIZATION_NAME` with the GitHub organization name) - *these secrets are read by the GitHub Action workflows* - [ ] Go to `https://github.com/organizations/$GITHUB_ORGANIZATION_NAME/settings/apps/$GITHUB_APP_NAME` and copy the `App ID` - [ ] `RO_GITHUB_APP_ID` diff --git a/scripts/__tests__/actions/classify-allow-destroy.test.ts b/scripts/__tests__/actions/classify-allow-destroy.test.ts new file mode 100644 index 0000000..974911b --- /dev/null +++ b/scripts/__tests__/actions/classify-allow-destroy.test.ts @@ -0,0 +1,181 @@ +import 'reflect-metadata' + +import {describe, it} from 'node:test' +import assert from 'node:assert' +import { + getEnvironment, + hasAllowDestroyChange +} from '../../src/actions/classify-allow-destroy.js' +import {Config} from '../../src/yaml/config.js' +import {State} from '../../src/terraform/state.js' +import {Locals} from '../../src/terraform/locals.js' + +function setManagedResourceTypes(resourceTypes: string[]): void { + Locals.locals = { + resource_types: resourceTypes, + ignore: { + repositories: [], + teams: [], + users: [] + } + } +} + +function state(source: object): State { + return new State(JSON.stringify(source)) +} + +describe('allow destroy classification', () => { + it('routes repository deletes to allow-destroy environments', async () => { + setManagedResourceTypes(['github_repository', 'github_membership']) + + const allowDestroy = await hasAllowDestroyChange( + new Config(` +repositories: + kept: {} +`), + state({ + values: { + root_module: { + resources: [ + { + mode: 'managed', + type: 'github_repository', + values: {name: 'kept'} + }, + { + mode: 'managed', + type: 'github_repository', + values: {name: 'removed'} + } + ] + } + } + }) + ) + + assert.equal(allowDestroy, true) + assert.equal(getEnvironment('read', allowDestroy), 'read-allow-destroy') + assert.equal(getEnvironment('write', allowDestroy), 'write-allow-destroy') + }) + + it('routes membership deletes to allow-destroy environments', async () => { + setManagedResourceTypes(['github_repository', 'github_membership']) + + const allowDestroy = await hasAllowDestroyChange( + new Config(` +members: + admin: + - kept +`), + state({ + values: { + root_module: { + resources: [ + { + mode: 'managed', + type: 'github_membership', + values: {username: 'kept', role: 'admin'} + }, + { + mode: 'managed', + type: 'github_membership', + values: {username: 'removed', role: 'admin'} + } + ] + } + } + }) + ) + + assert.equal(allowDestroy, true) + }) + + it('keeps repository and membership updates in normal environments', async () => { + setManagedResourceTypes(['github_repository', 'github_membership']) + + const allowDestroy = await hasAllowDestroyChange( + new Config(` +members: + member: + - octocat +repositories: + github: + description: updated +`), + state({ + values: { + root_module: { + resources: [ + { + mode: 'managed', + type: 'github_repository', + values: {name: 'github', description: 'old'} + }, + { + mode: 'managed', + type: 'github_membership', + values: {username: 'octocat', role: 'member'} + } + ] + } + } + }) + ) + + assert.equal(allowDestroy, false) + assert.equal(getEnvironment('read', allowDestroy), 'read') + assert.equal(getEnvironment('write', allowDestroy), 'write') + }) + + it('ignores deletes for other resource types', async () => { + setManagedResourceTypes(['github_team']) + + const allowDestroy = await hasAllowDestroyChange( + new Config('{}'), + state({ + values: { + root_module: { + resources: [ + { + mode: 'managed', + type: 'github_team', + values: {name: 'removed'} + } + ] + } + } + }) + ) + + assert.equal(allowDestroy, false) + }) + + it('ignores repository and membership types that are not managed', async () => { + setManagedResourceTypes([]) + + const allowDestroy = await hasAllowDestroyChange( + new Config('{}'), + state({ + values: { + root_module: { + resources: [ + { + mode: 'managed', + type: 'github_repository', + values: {name: 'removed'} + }, + { + mode: 'managed', + type: 'github_membership', + values: {username: 'removed', role: 'member'} + } + ] + } + } + }) + ) + + assert.equal(allowDestroy, false) + }) +}) diff --git a/scripts/src/actions/classify-allow-destroy.ts b/scripts/src/actions/classify-allow-destroy.ts new file mode 100644 index 0000000..22fdc74 --- /dev/null +++ b/scripts/src/actions/classify-allow-destroy.ts @@ -0,0 +1,120 @@ +import * as core from '@actions/core' +import {pathToFileURL} from 'url' +import {Config} from '../yaml/config.js' +import {State} from '../terraform/state.js' +import { + Resource, + ResourceConstructor, + ResourceConstructors +} from '../resources/resource.js' +import {Member} from '../resources/member.js' +import {Repository} from '../resources/repository.js' + +const ALLOW_DESTROY_RESOURCE_CLASSES: ResourceConstructor[] = [ + Member, + Repository +] + +type Mode = 'read' | 'write' + +type Matrix = { + include: { + workspace: string + environment: string + }[] +} + +function getStateAddress(resource: Resource): string { + return resource.getStateAddress().toLowerCase() +} + +function hasMissingResources( + config: Config, + state: State, + resourceClass: ResourceConstructor +): boolean { + const desiredAddresses = new Set( + config.getResources(resourceClass).map(getStateAddress) + ) + return state + .getResources(resourceClass) + .some(resource => !desiredAddresses.has(getStateAddress(resource))) +} + +export async function hasAllowDestroyChange( + config: Config, + state: State +): Promise { + for (const resourceClass of ALLOW_DESTROY_RESOURCE_CLASSES) { + if ( + ResourceConstructors.includes(resourceClass) && + !(await state.isIgnored(resourceClass)) && + hasMissingResources(config, state, resourceClass) + ) { + return true + } + } + + return false +} + +export function getEnvironment(mode: Mode, allowDestroy: boolean): string { + return allowDestroy ? `${mode}-allow-destroy` : mode +} + +export async function classifyWorkspaces({ + mode, + workspaces, + githubDir +}: { + mode: Mode + workspaces: string[] + githubDir: string +}): Promise { + const include = [] + const originalWorkspace = process.env.TF_WORKSPACE + + try { + for (const workspace of workspaces) { + process.env.TF_WORKSPACE = workspace + const config = Config.FromPath(`${githubDir}/${workspace}.yml`) + const state = await State.New() + const allowDestroy = await hasAllowDestroyChange(config, state) + const environment = getEnvironment(mode, allowDestroy) + core.info(`${workspace}: ${environment}`) + include.push({workspace, environment}) + } + } finally { + if (originalWorkspace === undefined) { + delete process.env.TF_WORKSPACE + } else { + process.env.TF_WORKSPACE = originalWorkspace + } + } + + return {include} +} + +async function run(): Promise { + const mode = (process.env.MODE ?? 'read') as Mode + if (mode !== 'read' && mode !== 'write') { + throw new Error(`MODE must be one of "read" or "write", got "${mode}"`) + } + + const workspaces = JSON.parse(process.env.WORKSPACES?.trim() || '[]') + if (!Array.isArray(workspaces)) { + throw new Error('WORKSPACES must be a JSON array') + } + + const matrix = await classifyWorkspaces({ + mode, + workspaces, + githubDir: process.env.GITHUB_DIR ?? '../github' + }) + + core.setOutput('matrix', JSON.stringify(matrix)) +} + +if (import.meta.url === pathToFileURL(process.argv[1]).href) { + run().catch(error => core.setFailed(error)) +} diff --git a/terraform/allow_destroy_override.tf.disabled b/terraform/allow_destroy_override.tf.disabled new file mode 100644 index 0000000..012e584 --- /dev/null +++ b/terraform/allow_destroy_override.tf.disabled @@ -0,0 +1,11 @@ +resource "github_membership" "this" { + lifecycle { + prevent_destroy = false + } +} + +resource "github_repository" "this" { + lifecycle { + prevent_destroy = false + } +} From 34f200fbea77d191b001d5911949a9455f64e539 Mon Sep 17 00:00:00 2001 From: Piotr Galar Date: Thu, 16 Jul 2026 19:34:53 +0100 Subject: [PATCH 04/13] Make org membership updates configurable --- .github/workflows/update-members.yml | 11 ++++++ docs/SETUP.md | 2 +- .../__tests__/actions/update-members.test.ts | 34 ++++++++++++++++--- scripts/src/actions/update-members.ts | 32 +++++++++++++++-- 4 files changed, 71 insertions(+), 8 deletions(-) diff --git a/.github/workflows/update-members.yml b/.github/workflows/update-members.yml index 5eb34ca..6d9165a 100644 --- a/.github/workflows/update-members.yml +++ b/.github/workflows/update-members.yml @@ -26,6 +26,14 @@ on: options: - retain - remove + organization-membership: + description: Whether selected users remain organization members + required: true + default: keep + type: choice + options: + - keep + - remove defaults: run: @@ -69,6 +77,7 @@ jobs: IGNORE: ${{ github.event.inputs.ignore }} ONLY: ${{ github.event.inputs.only }} PUBLIC_REPO_ACCESS: ${{ github.event.inputs['public-repo-access'] }} + ORGANIZATION_MEMBERSHIP: ${{ github.event.inputs['organization-membership'] }} - name: Check if organization config was modified id: config-modified env: @@ -90,6 +99,7 @@ jobs: IGNORE: ${{ github.event.inputs.ignore }} ONLY: ${{ github.event.inputs.only }} PUBLIC_REPO_ACCESS: ${{ github.event.inputs['public-repo-access'] }} + ORGANIZATION_MEMBERSHIP: ${{ github.event.inputs['organization-membership'] }} AFFECTED_USERS: ${{ steps.update.outputs.affected-users }} GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | @@ -104,6 +114,7 @@ jobs: echo "Ignore: ${IGNORE:-not set}" echo "Only: ${ONLY:-not set}" echo "Public repo access: ${PUBLIC_REPO_ACCESS}" + echo "Organization membership: ${ORGANIZATION_MEMBERSHIP}" echo "Affected users: ${AFFECTED_USERS:-none}" } > "${body}" diff --git a/docs/SETUP.md b/docs/SETUP.md index fdca08a..0fdeb8f 100644 --- a/docs/SETUP.md +++ b/docs/SETUP.md @@ -152,7 +152,7 @@ ## Member Update Workflows -- [ ] Use `Update Members` to create a draft PR that removes selected members from teams and repository collaborators in `github/$ORGANIZATION_NAME.yml`. The workflow requires either `cutoff-date` or `only`, supports `ignore` and `limit`, and can retain effective public repository access by converting that access to direct public repository collaborators in the YAML config. +- [ ] Use `Update Members` to create a draft PR that removes selected members from teams and repository collaborators in `github/$ORGANIZATION_NAME.yml`. The workflow requires either `cutoff-date` or `only`, supports `ignore` and `limit`, can retain effective public repository access by converting that access to direct public repository collaborators, and can optionally remove selected users from organization membership in the YAML config. - [ ] Review and merge the draft PR through the normal GitHub Management PR flow. ## GitHub Management Repository Protections diff --git a/scripts/__tests__/actions/update-members.test.ts b/scripts/__tests__/actions/update-members.test.ts index e2aeef3..b115ef7 100644 --- a/scripts/__tests__/actions/update-members.test.ts +++ b/scripts/__tests__/actions/update-members.test.ts @@ -6,9 +6,11 @@ import {Config} from '../../src/yaml/config.js' import { parseCutoffDate, parseLimit, + parseOrganizationMembership, selectMembersForUpdate, updateMembersConfig } from '../../src/actions/update-members.js' +import {Member} from '../../src/resources/member.js' import {TeamMember} from '../../src/resources/team-member.js' import {RepositoryCollaborator} from '../../src/resources/repository-collaborator.js' @@ -20,7 +22,8 @@ describe('update members', () => { selectMembersForUpdate(config, [], { ignore: [], only: [], - publicRepoAccess: 'retain' + publicRepoAccess: 'retain', + organizationMembership: 'keep' }) ) }) @@ -31,8 +34,11 @@ describe('update members', () => { '2025-01-02T00:00:00.000Z' ) assert.equal(parseLimit('2'), 2) + assert.equal(parseOrganizationMembership('keep'), 'keep') + assert.equal(parseOrganizationMembership('remove'), 'remove') assert.throws(() => parseCutoffDate('01-02-2025')) assert.throws(() => parseLimit('0')) + assert.throws(() => parseOrganizationMembership('invalid')) }) it('selects inactive members with only, ignore, limit, and KEEP handling', () => { @@ -58,7 +64,8 @@ members: limit: 2, ignore: ['ignored'], only: ['active', 'ignored', 'kept', 'manual', 'never-active', 'old'], - publicRepoAccess: 'retain' + publicRepoAccess: 'retain', + organizationMembership: 'keep' } ) @@ -94,7 +101,7 @@ teams: - alice `) - updateMembersConfig(config, ['alice'], 'retain') + updateMembersConfig(config, ['alice'], 'retain', 'keep') assert.equal( config @@ -145,7 +152,7 @@ teams: - alice `) - updateMembersConfig(config, ['alice'], 'remove') + updateMembersConfig(config, ['alice'], 'remove', 'keep') assert.equal( config @@ -154,4 +161,23 @@ teams: false ) }) + + it('removes organization membership when configured', () => { + const config = new Config(` +members: + member: + - alice + - bob +repositories: + public-repo: + visibility: public +`) + + updateMembersConfig(config, ['alice'], 'remove', 'remove') + + assert.deepEqual( + config.getResources(Member).map(member => member.username), + ['bob'] + ) + }) }) diff --git a/scripts/src/actions/update-members.ts b/scripts/src/actions/update-members.ts index 25d649c..77613a4 100644 --- a/scripts/src/actions/update-members.ts +++ b/scripts/src/actions/update-members.ts @@ -18,6 +18,7 @@ import { } from './shared/access-summary.js' export type PublicRepoAccess = 'retain' | 'remove' +export type OrganizationMembership = 'keep' | 'remove' export type MemberActivity = { username: string @@ -30,6 +31,7 @@ export type UpdateMembersOptions = { ignore: string[] only: string[] publicRepoAccess: PublicRepoAccess + organizationMembership: OrganizationMembership } type ActivityRecord = { @@ -69,6 +71,15 @@ export function parsePublicRepoAccess(source?: string): PublicRepoAccess { throw new Error('public-repo-access must be retain or remove') } +export function parseOrganizationMembership( + source?: string +): OrganizationMembership { + if (source === 'keep' || source === 'remove') { + return source + } + throw new Error('organization-membership must be keep or remove') +} + export function selectMembersForUpdate( config: Config, activities: MemberActivity[], @@ -120,7 +131,8 @@ export function selectMembersForUpdate( export function updateMembersConfig( config: Config, usernames: string[], - publicRepoAccess: PublicRepoAccess + publicRepoAccess: PublicRepoAccess, + organizationMembership: OrganizationMembership ): string[] { const targets = new Set(usernames.map(username => username.toLowerCase())) const repositories = new Map( @@ -194,6 +206,15 @@ export function updateMembersConfig( } } + if (organizationMembership === 'remove') { + for (const member of config.getResources(Member)) { + if (targets.has(member.username.toLowerCase())) { + core.info(`Removing ${member.username} from the organization`) + config.removeResource(member) + } + } + } + return Array.from(targets).sort() } @@ -269,6 +290,9 @@ async function run(): Promise { const ignore = parseUserList(process.env.IGNORE) const only = parseUserList(process.env.ONLY) const publicRepoAccess = parsePublicRepoAccess(process.env.PUBLIC_REPO_ACCESS) + const organizationMembership = parseOrganizationMembership( + process.env.ORGANIZATION_MEMBERSHIP || 'keep' + ) const config = Config.FromPath() const activities = @@ -280,12 +304,14 @@ async function run(): Promise { limit, ignore, only, - publicRepoAccess + publicRepoAccess, + organizationMembership }) const affectedUsers = updateMembersConfig( config, selectedMembers, - publicRepoAccess + publicRepoAccess, + organizationMembership ) config.save() From 674f2b8026803002a7ff53ae0231e53bd9662e5b Mon Sep 17 00:00:00 2001 From: galargh Date: Sun, 9 Aug 2026 10:21:30 +0200 Subject: [PATCH 05/13] Refine access change reporting and guards --- .github/workflows/access-report.yml | 63 ++++ .github/workflows/apply.yml | 9 +- .github/workflows/fix.yml | 13 + .github/workflows/plan.yml | 9 +- CHANGELOG.md | 1 + docs/SETUP.md | 2 +- .../__tests__/actions/access-summary.test.ts | 106 ++++++- .../actions/classify-allow-destroy.test.ts | 106 ++++++- scripts/__tests__/workflows.test.ts | 60 ++++ scripts/src/actions/access-report.ts | 18 ++ scripts/src/actions/classify-allow-destroy.ts | 81 +++++- scripts/src/actions/fix-yaml-config.ts | 17 +- scripts/src/actions/shared/access-summary.ts | 171 ++++++++--- .../actions/shared/describe-access-changes.ts | 269 +++++++++--------- 14 files changed, 738 insertions(+), 187 deletions(-) create mode 100644 .github/workflows/access-report.yml create mode 100644 scripts/__tests__/workflows.test.ts create mode 100644 scripts/src/actions/access-report.ts diff --git a/.github/workflows/access-report.yml b/.github/workflows/access-report.yml new file mode 100644 index 0000000..b7907da --- /dev/null +++ b/.github/workflows/access-report.yml @@ -0,0 +1,63 @@ +name: Access Report + +on: + workflow_dispatch: + inputs: + organization: + description: Organization config to report on + required: true + +defaults: + run: + shell: bash + +jobs: + report: + permissions: + contents: read + name: Access report + runs-on: ubuntu-latest + environment: read + env: + TF_IN_AUTOMATION: 1 + TF_INPUT: 0 + TF_WORKSPACE: ${{ github.event.inputs.organization }} + AWS_ACCESS_KEY_ID: ${{ secrets.RO_AWS_ACCESS_KEY_ID }} + AWS_SECRET_ACCESS_KEY: ${{ secrets.RO_AWS_SECRET_ACCESS_KEY }} + steps: + - name: Checkout + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - name: Setup terraform + uses: hashicorp/setup-terraform@5e8dbf3c6d9deaf4193ca7a8fb23f2ac83bb6c85 # v4.0.0 + with: + terraform_version: 1.12.0 + terraform_wrapper: false + - name: Initialize terraform + run: terraform init + working-directory: terraform + - name: Install pnpm + uses: pnpm/action-setup@91ab88e2619ed1f46221f0ba42d1492c02baf788 # v6.0.6 + with: + version: 10 + - name: Use Node.js lts/* + uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 + with: + node-version: lts/* + cache: '' + - name: Initialize scripts + run: pnpm install --frozen-lockfile && pnpm run build + working-directory: scripts + - name: Generate access report + run: node lib/actions/access-report.js + working-directory: scripts + env: + ACCESS_REPORT_PATH: ../ACCESS_REPORT.md + - name: Publish access report summary + run: cat ACCESS_REPORT.md >> "$GITHUB_STEP_SUMMARY" + - name: Upload access report + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: access-report-${{ env.TF_WORKSPACE }} + path: ACCESS_REPORT.md + if-no-files-found: error + retention-days: 14 diff --git a/.github/workflows/apply.yml b/.github/workflows/apply.yml index 21b3665..4b619f5 100644 --- a/.github/workflows/apply.yml +++ b/.github/workflows/apply.yml @@ -132,7 +132,14 @@ jobs: run: terraform init - name: Allow destroy in guarded environment if: matrix.environment == 'write-allow-destroy' - run: cp allow_destroy_override.tf.disabled allow_destroy_override.tf + env: + ALLOW_DESTROY: ${{ vars.ALLOW_DESTROY }} + run: | + if [[ "${ALLOW_DESTROY}" != "true" ]]; then + echo "The write-allow-destroy environment must define ALLOW_DESTROY=true." + exit 1 + fi + cp allow_destroy_override.tf.disabled allow_destroy_override.tf - name: Download reviewed terraform plan env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/.github/workflows/fix.yml b/.github/workflows/fix.yml index fe3b7ef..dba81be 100644 --- a/.github/workflows/fix.yml +++ b/.github/workflows/fix.yml @@ -117,6 +117,19 @@ jobs: id: fix run: node lib/actions/fix-yaml-config.js working-directory: scripts + env: + ACCESS_REPORT_PATH: ../ACCESS_REPORT.md + - name: Publish access report summary + if: always() && hashFiles('ACCESS_REPORT.md') != '' + run: cat ACCESS_REPORT.md >> "$GITHUB_STEP_SUMMARY" + - name: Upload access report + if: always() && hashFiles('ACCESS_REPORT.md') != '' + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: access-report-${{ env.TF_WORKSPACE }} + path: ACCESS_REPORT.md + if-no-files-found: error + retention-days: 14 - name: Upload YAML config uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: diff --git a/.github/workflows/plan.yml b/.github/workflows/plan.yml index 3dae02f..8bbc433 100644 --- a/.github/workflows/plan.yml +++ b/.github/workflows/plan.yml @@ -141,7 +141,14 @@ jobs: working-directory: terraform - name: Allow destroy in guarded environment if: matrix.environment == 'read-allow-destroy' - run: cp allow_destroy_override.tf.disabled allow_destroy_override.tf + env: + ALLOW_DESTROY: ${{ vars.ALLOW_DESTROY }} + run: | + if [[ "${ALLOW_DESTROY}" != "true" ]]; then + echo "The read-allow-destroy environment must define ALLOW_DESTROY=true." + exit 1 + fi + cp allow_destroy_override.tf.disabled allow_destroy_override.tf working-directory: terraform - name: Plan terraform run: | diff --git a/CHANGELOG.md b/CHANGELOG.md index 59bfc4a..1fedcfd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - workflows: added separate GitHub Actions environments for reading organization state, writing organization state, and pushing repository changes +- **BREAKING**: access changes action now emits only the access change comment by default; update custom usage to avoid nesting the full access breakdown in PR comments - workflows: pin third-party actions to latest release SHAs and replan from the merged commit before applying - docs: update template repository references from `github-mgmt-template` to `github-as-code` - scripts: update dependencies with security advisories diff --git a/docs/SETUP.md b/docs/SETUP.md index be67265..123c6fb 100644 --- a/docs/SETUP.md +++ b/docs/SETUP.md @@ -117,7 +117,7 @@ ## GitHub Actions Environments and Secrets - [ ] Create GitHub Actions environments named `read`, `read-allow-destroy`, `write`, `write-allow-destroy`, and `push`, and configure protection rules such as required reviewers. Workflows that read organization state reference `read`; workflows that write organization state reference `write`; workflows that push generated changes to the GitHub Management repository reference `push`. -- [ ] Configure `read-allow-destroy` and `write-allow-destroy` with stricter protection rules for repository and membership deletion plans/applies: +- [ ] Configure `read-allow-destroy` and `write-allow-destroy` with stricter protection rules for repository and membership deletion plans/applies, and set an environment variable named `ALLOW_DESTROY` to `true` in each environment: - [ ] Require reviewers - [ ] Prevent self-review - [ ] Restrict deployment branches to `master` diff --git a/scripts/__tests__/actions/access-summary.test.ts b/scripts/__tests__/actions/access-summary.test.ts index 14e5854..6abe256 100644 --- a/scripts/__tests__/actions/access-summary.test.ts +++ b/scripts/__tests__/actions/access-summary.test.ts @@ -10,6 +10,7 @@ import { } from '../../src/actions/shared/access-summary.js' import { describeAccessChanges, + describeAccessChangesComment, describeAccessReport } from '../../src/actions/shared/describe-access-changes.js' import {StateSchema} from '../../src/terraform/schema.js' @@ -22,6 +23,7 @@ members: - alice - carol - dave + - frank - kept # KEEP: manual exception repositories: private-repo: @@ -45,6 +47,10 @@ repositories: - maintainers visibility: public teams: + empty: + members: + member: + - frank guests: members: member: @@ -64,11 +70,11 @@ teams: assert.equal(summary['team-only-non-member'].isOutsideCollaborator, false) assert.deepEqual(categories.outsideCollaborators, ['outside']) assert.deepEqual(categories.potentialOutsideCollaborators, ['alice']) - assert.deepEqual(categories.potentialNoMembers, ['carol']) + assert.deepEqual(categories.potentialNoMembers, ['carol', 'frank']) assert.deepEqual(categories.anyOtherMembers, ['dave', 'kept']) }) - it('annotates repository visibility in access changes and summaries', () => { + it('annotates repository visibility and access path in access changes and summaries', () => { const state = new State( JSON.stringify({ values: { @@ -128,11 +134,103 @@ repositories: assert.match( changes, - /will have the permission to public-repo \(public\) change from pull to push/ + /will change from having direct pull permission to public-repo \(public\) to having direct push permission to public-repo \(public\)/ ) assert.match(report, /Potential outside collaborators<\/summary>/) assert.match(report, /Affected users: alice/) assert.match(report, /User alice \(member\):/) - assert.match(report, /has push permission to public-repo \(public\)/) + assert.match(report, /has direct push permission to public-repo \(public\)/) + }) + + it('describes team and mixed repository access paths', () => { + const state = new State( + JSON.stringify({values: {root_module: {resources: []}}}) + ) + const config = new Config(` +members: + member: + - alice + - bob +repositories: + private-repo: + collaborators: + pull: + - bob + teams: + admin: + - owners + push: + - maintainers + visibility: private +teams: + maintainers: + members: + member: + - alice + owners: + members: + member: + - bob +`) + + const changes = describeAccessChanges(state, config) + const report = describeAccessReport(state, config) + + assert.match( + changes, + /will gain push permission to private-repo \(private\) through team maintainers/ + ) + assert.match( + changes, + /will gain effective admin permission to private-repo \(private\) through direct pull permission and team owners/ + ) + assert.match( + report, + /has push permission to private-repo \(private\) through team maintainers/ + ) + assert.match( + report, + /has effective admin permission to private-repo \(private\) through direct pull permission and team owners/ + ) + }) + + it('keeps routine comments to access changes only', () => { + const state = new State( + JSON.stringify({values: {root_module: {resources: []}}}) + ) + const config = new Config(` +members: + member: + - alice +`) + + const comment = describeAccessChangesComment(state, config) + + assert.match(comment, /Access Changes<\/summary>/) + assert.doesNotMatch(comment, /Potential no members/) + assert.doesNotMatch(comment, /Any other members/) + }) + + it('falls back to workflow output when access change comments are too long', () => { + const state = new State( + JSON.stringify({values: {root_module: {resources: []}}}) + ) + const config = new Config(` +members: + member: + - alice +`) + + const comment = describeAccessChangesComment( + state, + config, + 10, + 'https://github.example/runs/1' + ) + + assert.equal( + comment, + 'Access changes are too long to post as a comment. Please inspect [the Fix workflow summary or access report artifact](https://github.example/runs/1) instead.' + ) }) }) diff --git a/scripts/__tests__/actions/classify-allow-destroy.test.ts b/scripts/__tests__/actions/classify-allow-destroy.test.ts index 974911b..6d070a2 100644 --- a/scripts/__tests__/actions/classify-allow-destroy.test.ts +++ b/scripts/__tests__/actions/classify-allow-destroy.test.ts @@ -4,7 +4,8 @@ import {describe, it} from 'node:test' import assert from 'node:assert' import { getEnvironment, - hasAllowDestroyChange + hasAllowDestroyChange, + validateRemovedMembersHaveNoDanglingAccess } from '../../src/actions/classify-allow-destroy.js' import {Config} from '../../src/yaml/config.js' import {State} from '../../src/terraform/state.js' @@ -178,4 +179,107 @@ repositories: assert.equal(allowDestroy, false) }) + + it('fails when removing a member who remains in a team', async () => { + setManagedResourceTypes(['github_membership']) + + await assert.rejects( + validateRemovedMembersHaveNoDanglingAccess( + new Config(` +teams: + maintainers: + members: + member: + - removed +`), + state({ + values: { + root_module: { + resources: [ + { + mode: 'managed', + type: 'github_membership', + values: {username: 'removed', role: 'member'} + } + ] + } + } + }) + ), + /removed is still a member of team maintainers/ + ) + }) + + it('fails when removing a member who keeps direct private repository access', async () => { + setManagedResourceTypes(['github_membership']) + + await assert.rejects( + validateRemovedMembersHaveNoDanglingAccess( + new Config(` +repositories: + private-repo: + collaborators: + pull: + - removed + visibility: private +`), + state({ + values: { + root_module: { + resources: [ + { + mode: 'managed', + type: 'github_membership', + values: {username: 'removed', role: 'member'} + } + ] + } + } + }) + ), + /removed still has direct access to private repository private-repo/ + ) + }) + + it('allows member removal when team and private direct access are removed too', async () => { + setManagedResourceTypes(['github_membership']) + + await validateRemovedMembersHaveNoDanglingAccess( + new Config(` +repositories: + public-repo: + collaborators: + pull: + - removed + visibility: public +`), + state({ + values: { + root_module: { + resources: [ + { + mode: 'managed', + type: 'github_membership', + values: {username: 'removed', role: 'member'} + }, + { + mode: 'managed', + type: 'github_repository', + values: {name: 'private-repo', visibility: 'private'} + }, + { + mode: 'managed', + type: 'github_repository_collaborator', + values: { + username: 'removed', + repository: 'private-repo', + permission: 'pull' + } + } + ] + } + } + }) + ) + }) }) diff --git a/scripts/__tests__/workflows.test.ts b/scripts/__tests__/workflows.test.ts new file mode 100644 index 0000000..8ee388f --- /dev/null +++ b/scripts/__tests__/workflows.test.ts @@ -0,0 +1,60 @@ +import assert from 'node:assert' +import {readFileSync} from 'node:fs' +import {describe, it} from 'node:test' +import * as YAML from 'yaml' + +type WorkflowStep = { + name?: string + run?: string + env?: Record +} + +type Workflow = { + on: { + workflow_dispatch?: unknown + } + jobs: Record< + string, + { + environment?: string + steps: WorkflowStep[] + } + > +} + +function workflow(path: string): Workflow { + return YAML.parse(readFileSync(`../.github/workflows/${path}`, 'utf8')) +} + +describe('workflows', () => { + it('guards allow-destroy override steps with an environment variable', () => { + const plan = workflow('plan.yml') + const apply = workflow('apply.yml') + const planStep = plan.jobs.plan.steps.find( + step => step.name === 'Allow destroy in guarded environment' + ) + const applyStep = apply.jobs.apply.steps.find( + step => step.name === 'Allow destroy in guarded environment' + ) + + assert.ok(planStep) + assert.ok(applyStep) + assert.equal(planStep.env?.ALLOW_DESTROY, '${{ vars.ALLOW_DESTROY }}') + assert.match(planStep.run ?? '', /ALLOW_DESTROY.*true/) + assert.match(planStep.run ?? '', /allow_destroy_override\.tf\.disabled/) + assert.equal(applyStep.env?.ALLOW_DESTROY, '${{ vars.ALLOW_DESTROY }}') + assert.match(applyStep.run ?? '', /ALLOW_DESTROY.*true/) + assert.match(applyStep.run ?? '', /allow_destroy_override\.tf\.disabled/) + }) + + it('provides a manual access report workflow with summary and artifact output', () => { + const report = workflow('access-report.yml') + const steps = report.jobs.report.steps.map(step => step.name) + + assert.ok(report.on.workflow_dispatch) + assert.equal(report.jobs.report.environment, 'read') + assert.ok(steps.includes('Generate access report')) + assert.ok(steps.includes('Publish access report summary')) + assert.ok(steps.includes('Upload access report')) + }) +}) diff --git a/scripts/src/actions/access-report.ts b/scripts/src/actions/access-report.ts new file mode 100644 index 0000000..4dcc592 --- /dev/null +++ b/scripts/src/actions/access-report.ts @@ -0,0 +1,18 @@ +import 'reflect-metadata' + +import * as fs from 'fs' +import * as core from '@actions/core' +import {Config} from '../yaml/config.js' +import {State} from '../terraform/state.js' +import {describeAccessReport} from './shared/describe-access-changes.js' + +async function run(): Promise { + const state = await State.New() + const config = Config.FromPath() + const accessReport = describeAccessReport(state, config) + const accessReportPath = process.env.ACCESS_REPORT_PATH ?? 'ACCESS_REPORT.md' + + fs.writeFileSync(accessReportPath, accessReport) +} + +run().catch(error => core.setFailed(error)) diff --git a/scripts/src/actions/classify-allow-destroy.ts b/scripts/src/actions/classify-allow-destroy.ts index 22fdc74..a4828c5 100644 --- a/scripts/src/actions/classify-allow-destroy.ts +++ b/scripts/src/actions/classify-allow-destroy.ts @@ -1,3 +1,5 @@ +import 'reflect-metadata' + import * as core from '@actions/core' import {pathToFileURL} from 'url' import {Config} from '../yaml/config.js' @@ -8,7 +10,9 @@ import { ResourceConstructors } from '../resources/resource.js' import {Member} from '../resources/member.js' -import {Repository} from '../resources/repository.js' +import {Repository, Visibility} from '../resources/repository.js' +import {TeamMember} from '../resources/team-member.js' +import {RepositoryCollaborator} from '../resources/repository-collaborator.js' const ALLOW_DESTROY_RESOURCE_CLASSES: ResourceConstructor[] = [ Member, @@ -58,6 +62,80 @@ export async function hasAllowDestroyChange( return false } +export async function validateRemovedMembersHaveNoDanglingAccess( + config: Config, + state: State +): Promise { + if (await state.isIgnored(Member)) { + return + } + + const desiredMembers = new Set( + config.getResources(Member).map(member => member.username.toLowerCase()) + ) + const removedMembers = state + .getResources(Member) + .map(member => member.username.toLowerCase()) + .filter(username => !desiredMembers.has(username)) + + if (removedMembers.length === 0) { + return + } + + const repositoryVisibility = new Map( + [...state.getResources(Repository), ...config.getResources(Repository)].map( + repository => [ + repository.name.toLowerCase(), + repository.visibility ?? Visibility.Private + ] + ) + ) + const teamMembers = config.getResources(TeamMember) + const repositoryCollaborators = config.getResources(RepositoryCollaborator) + const errors: string[] = [] + + for (const username of removedMembers.sort()) { + const teams = teamMembers + .filter(teamMember => teamMember.username.toLowerCase() === username) + .map(teamMember => teamMember.team.toLowerCase()) + .sort() + const privateRepositories = repositoryCollaborators + .filter(collaborator => collaborator.username.toLowerCase() === username) + .filter( + collaborator => + (repositoryVisibility.get(collaborator.repository.toLowerCase()) ?? + Visibility.Private) === Visibility.Private + ) + .map(collaborator => collaborator.repository.toLowerCase()) + .sort() + + if (teams.length > 0) { + errors.push( + `${username} is still a member of ${teams.length === 1 ? 'team' : 'teams'} ${teams.join( + ', ' + )}` + ) + } + + if (privateRepositories.length > 0) { + errors.push( + `${username} still has direct access to private ${privateRepositories.length === 1 ? 'repository' : 'repositories'} ${privateRepositories.join( + ', ' + )}` + ) + } + } + + if (errors.length > 0) { + throw new Error( + [ + 'Cannot remove organization members while leaving dangling access:', + ...errors.map(error => `- ${error}`) + ].join('\n') + ) + } +} + export function getEnvironment(mode: Mode, allowDestroy: boolean): string { return allowDestroy ? `${mode}-allow-destroy` : mode } @@ -79,6 +157,7 @@ export async function classifyWorkspaces({ process.env.TF_WORKSPACE = workspace const config = Config.FromPath(`${githubDir}/${workspace}.yml`) const state = await State.New() + await validateRemovedMembersHaveNoDanglingAccess(config, state) const allowDestroy = await hasAllowDestroyChange(config, state) const environment = getEnvironment(mode, allowDestroy) core.info(`${workspace}: ${environment}`) diff --git a/scripts/src/actions/fix-yaml-config.ts b/scripts/src/actions/fix-yaml-config.ts index 733d370..0a163bf 100644 --- a/scripts/src/actions/fix-yaml-config.ts +++ b/scripts/src/actions/fix-yaml-config.ts @@ -1,16 +1,27 @@ import 'reflect-metadata' +import * as fs from 'fs' import {runToggleArchivedRepos} from './shared/toggle-archived-repos.js' -import {runDescribeAccessChanges} from './shared/describe-access-changes.js' +import { + describeAccessChangesComment, + describeAccessReport +} from './shared/describe-access-changes.js' +import {Config} from '../yaml/config.js' +import {State} from '../terraform/state.js' import * as core from '@actions/core' async function run(): Promise { await runToggleArchivedRepos() - const accessChangesDescription = await runDescribeAccessChanges() + const state = await State.New() + const config = Config.FromPath() + const accessChangesComment = describeAccessChangesComment(state, config) + const accessReport = describeAccessReport(state, config) + const accessReportPath = process.env.ACCESS_REPORT_PATH ?? 'ACCESS_REPORT.md' - core.setOutput('comment', accessChangesDescription) + fs.writeFileSync(accessReportPath, accessReport) + core.setOutput('comment', accessChangesComment) } run() diff --git a/scripts/src/actions/shared/access-summary.ts b/scripts/src/actions/shared/access-summary.ts index 9561a38..b3fc9a8 100644 --- a/scripts/src/actions/shared/access-summary.ts +++ b/scripts/src/actions/shared/access-summary.ts @@ -9,6 +9,13 @@ import {Repository, Visibility} from '../../resources/repository.js' export type RepositoryAccess = { permission: string visibility: Visibility + grants: RepositoryAccessGrant[] +} + +export type RepositoryAccessGrant = { + source: 'direct' | 'team' + permission: string + team?: string } export type UserAccess = { @@ -39,6 +46,45 @@ export function betterPermission(current: string, next: string): string { : current } +function betterGrant( + current: RepositoryAccessGrant | undefined, + next: RepositoryAccessGrant +): RepositoryAccessGrant { + return current === undefined || + permissions.indexOf(next.permission) < + permissions.indexOf(current.permission) + ? next + : current +} + +function addRepositoryGrant( + repositories: Record, + repository: string, + visibility: Visibility, + grant: RepositoryAccessGrant +): void { + const current = repositories[repository] + if (current === undefined) { + repositories[repository] = { + permission: grant.permission, + visibility, + grants: [grant] + } + } else { + current.permission = betterPermission(current.permission, grant.permission) + current.grants.push(grant) + } +} + +function sortRepositoryAccess(access: RepositoryAccess): RepositoryAccess { + return { + ...access, + grants: access.grants.sort((a, b) => + JSON.stringify(a).localeCompare(JSON.stringify(b)) + ) + } +} + export function parseUserList(source?: string): string[] { return Array.from( new Set( @@ -114,43 +160,49 @@ export function getAccessSummaryFrom(source: State | Config): AccessSummary { const repository = rc.repository.toLowerCase() const access = { permission: rc.permission, - visibility: repositoryVisibility.get(repository) ?? Visibility.Private + visibility: repositoryVisibility.get(repository) ?? Visibility.Private, + grants: [ + { + source: 'direct' as const, + permission: rc.permission + } + ] } - directRepositories[repository] = directRepositories[repository] - ? { - ...access, - permission: betterPermission( + directRepositories[repository] = { + ...access, + grants: [ + betterGrant( + directRepositories[repository]?.grants[0], + access.grants[0] + ) + ], + permission: directRepositories[repository] + ? betterPermission( directRepositories[repository].permission, access.permission ) - } - : access - repositories[repository] = repositories[repository] - ? { - ...access, - permission: betterPermission( - repositories[repository].permission, - access.permission - ) - } - : access + : access.permission + } + addRepositoryGrant( + repositories, + repository, + access.visibility, + access.grants[0] + ) } for (const tr of teamRepository) { const repository = tr.repository.toLowerCase() - const access = { - permission: tr.permission, - visibility: repositoryVisibility.get(repository) ?? Visibility.Private - } - repositories[repository] = repositories[repository] - ? { - ...access, - permission: betterPermission( - repositories[repository].permission, - access.permission - ) - } - : access + addRepositoryGrant( + repositories, + repository, + repositoryVisibility.get(repository) ?? Visibility.Private, + { + source: 'team', + permission: tr.permission, + team: tr.team.toLowerCase() + } + ) } const hasKeep = @@ -168,10 +220,20 @@ export function getAccessSummaryFrom(source: State | Config): AccessSummary { isMember, isOutsideCollaborator, repositories, - directRepositories, + directRepositories: Object.fromEntries( + Object.entries(directRepositories).map(([repository, access]) => [ + repository, + sortRepositoryAccess(access) + ]) + ), teams, hasKeepComment: hasKeep } + accessSummary[username].repositories = Object.fromEntries( + Object.entries(accessSummary[username].repositories).map( + ([repository, access]) => [repository, sortRepositoryAccess(access)] + ) + ) } } @@ -182,7 +244,10 @@ export function getComparableAccessSummary(source: State | Config): Record< string, { role?: string - repositories: Record + repositories: Record< + string, + {permission: string; grants: RepositoryAccessGrant[]} + > } > { return Object.fromEntries( @@ -193,7 +258,10 @@ export function getComparableAccessSummary(source: State | Config): Record< repositories: Object.fromEntries( Object.entries(access.repositories).map(([repository, value]) => [ repository, - {permission: value.permission} + { + permission: value.permission, + grants: value.grants + } ]) ) } @@ -213,7 +281,6 @@ export function categorizeAccessSummary( for (const [username, access] of Object.entries(summary)) { const repositories = Object.values(access.repositories) - const directRepositories = Object.values(access.directRepositories) if (access.isOutsideCollaborator) { categories.outsideCollaborators.push(username) } else if ( @@ -229,8 +296,7 @@ export function categorizeAccessSummary( } else if ( access.isMember && !access.hasKeepComment && - directRepositories.length === 0 && - access.teams.length === 0 + repositories.length === 0 ) { categories.potentialNoMembers.push(username) } else if (access.isMember) { @@ -252,6 +318,39 @@ export function formatRepositoryAccess( return `${repository} (${access.visibility})` } +function formatTeams(teams: string[]): string { + return teams.length === 1 ? `team ${teams[0]}` : `teams ${teams.join(', ')}` +} + +export function formatRepositoryAccessDescription( + repository: string, + access: RepositoryAccess +): string { + const directGrant = access.grants.find(grant => grant.source === 'direct') + const teamGrants = access.grants.filter(grant => grant.source === 'team') + const repositoryLabel = formatRepositoryAccess(repository, access) + + if (directGrant !== undefined && teamGrants.length === 0) { + return `direct ${directGrant.permission} permission to ${repositoryLabel}` + } + + if (directGrant === undefined) { + const teams = Array.from( + new Set(teamGrants.map(grant => grant.team).filter(Boolean) as string[]) + ).sort() + return `${access.permission} permission to ${repositoryLabel} through ${formatTeams( + teams + )}` + } + + const teams = Array.from( + new Set(teamGrants.map(grant => grant.team).filter(Boolean) as string[]) + ).sort() + return `effective ${access.permission} permission to ${repositoryLabel} through direct ${directGrant.permission} permission and ${formatTeams( + teams + )}` +} + export function formatAccessSummarySection( title: string, users: string[], @@ -281,7 +380,7 @@ export function formatAccessSummarySection( } else { for (const [repository, repositoryAccess] of repositories) { lines.push( - ` - has ${repositoryAccess.permission} permission to ${formatRepositoryAccess( + ` - has ${formatRepositoryAccessDescription( repository, repositoryAccess )}` diff --git a/scripts/src/actions/shared/describe-access-changes.ts b/scripts/src/actions/shared/describe-access-changes.ts index 93f602d..3e328d8 100644 --- a/scripts/src/actions/shared/describe-access-changes.ts +++ b/scripts/src/actions/shared/describe-access-changes.ts @@ -1,38 +1,67 @@ import {Config} from '../../yaml/config.js' import {State} from '../../terraform/state.js' -import diff from 'deep-diff' import * as core from '@actions/core' import { categorizeAccessSummary, formatAccessSummarySection, - formatRepositoryAccess, - getAccessSummaryFrom, - getComparableAccessSummary, - RepositoryAccess + formatRepositoryAccessDescription, + getAccessSummaryFrom } from './access-summary.js' -function repositoryLabel( - repository: string, - afterSummary: ReturnType, - beforeSummary: ReturnType -): string { - const access = - Object.values(afterSummary) - .map(user => user.repositories[repository]) - .find(Boolean) ?? - Object.values(beforeSummary) - .map(user => user.repositories[repository]) - .find(Boolean) ?? - ({permission: 'pull', visibility: 'private'} as RepositoryAccess) - - return formatRepositoryAccess(repository, access) -} +const GITHUB_COMMENT_LENGTH_LIMIT = 65000 export async function runDescribeAccessChanges(): Promise { const state = await State.New() const config = Config.FromPath() - return describeAccessReport(state, config) + return describeAccessChangesComment(state, config) +} + +export function workflowRunUrl(): string | undefined { + const serverUrl = process.env.GITHUB_SERVER_URL + const repository = process.env.GITHUB_REPOSITORY + const runId = process.env.GITHUB_RUN_ID + + if ( + serverUrl === undefined || + repository === undefined || + runId === undefined + ) { + return undefined + } + + return `${serverUrl}/${repository}/actions/runs/${runId}` +} + +export function describeAccessChangesComment( + state: State, + config: Config, + maxLength = GITHUB_COMMENT_LENGTH_LIMIT, + runUrl = workflowRunUrl() +): string { + const accessChangesDescription = describeAccessChanges(state, config) + const comment = [ + 'The following access changes will be introduced as a result of applying the plan:', + '', + '
Access Changes', + '', + '```', + accessChangesDescription, + '```', + '', + '
' + ].join('\n') + + if (Buffer.byteLength(comment, 'utf8') < maxLength) { + return comment + } + + const destination = + runUrl === undefined + ? 'the Fix workflow summary or the access report artifact' + : `[the Fix workflow summary or access report artifact](${runUrl})` + + return `Access changes are too long to post as a comment. Please inspect ${destination} instead.` } export function describeAccessReport(state: State, config: Config): string { @@ -78,133 +107,95 @@ export function describeAccessReport(state: State, config: Config): string { } export function describeAccessChanges(state: State, config: Config): string { - const before = getComparableAccessSummary(state) - const after = getComparableAccessSummary(config) - const beforeWithVisibility = getAccessSummaryFrom(state) - const afterWithVisibility = getAccessSummaryFrom(config) + const before = getAccessSummaryFrom(state) + const after = getAccessSummaryFrom(config) core.info(JSON.stringify({before, after}, null, 2)) - const changes = diff(before, after) || [] + const lines = [] + const usernames = Array.from( + new Set([...Object.keys(before), ...Object.keys(after)]) + ).sort() - core.debug(JSON.stringify(changes, null, 2)) + for (const username of usernames) { + const beforeAccess = before[username] + const afterAccess = after[username] + const userLines = [] - const changesByUser: Record = {} - for (const change of changes) { - if (change.path === undefined) { - throw new Error(`Change ${change.kind} has no path`) + if (beforeAccess?.role !== afterAccess?.role) { + if (beforeAccess?.role === undefined && afterAccess?.role !== undefined) { + userLines.push( + ` - will join the organization as a ${afterAccess.role} (remind them to accept the email invitation)` + ) + } else if ( + beforeAccess?.role !== undefined && + afterAccess?.role === undefined + ) { + userLines.push(' - will leave the organization') + } else { + userLines.push( + ` - will have the role in the organization change from ${beforeAccess?.role} to ${afterAccess?.role}` + ) + } } - const path = change.path - changesByUser[String(path[0])] = changesByUser[String(path[0])] || [] - changesByUser[String(path[0])].push(change) - } - const lines = [] - for (const [username, userChanges] of Object.entries(changesByUser)) { - lines.push(`User ${username}:`) - for (const change of userChanges) { - if (change.path === undefined) { - throw new Error(`Change ${change.kind} has no path`) + const repositories = Array.from( + new Set([ + ...Object.keys(beforeAccess?.repositories ?? {}), + ...Object.keys(afterAccess?.repositories ?? {}) + ]) + ).sort() + + for (const repository of repositories) { + const beforeRepositoryAccess = beforeAccess?.repositories[repository] + const afterRepositoryAccess = afterAccess?.repositories[repository] + if ( + JSON.stringify(beforeRepositoryAccess) === + JSON.stringify(afterRepositoryAccess) + ) { + continue } - const path = change.path - switch (change.kind) { - case 'E': - if (path[1] === 'role') { - if (change.lhs === undefined) { - lines.push( - ` - will join the organization as a ${change.rhs} (remind them to accept the email invitation)` - ) - } else if (change.rhs === undefined) { - lines.push(' - will leave the organization') - } else { - lines.push( - ` - will have the role in the organization change from ${change.lhs} to ${change.rhs}` - ) - } - } else { - const repository = String(path[2]) - lines.push( - ` - will have the permission to ${repositoryLabel( - repository, - afterWithVisibility, - beforeWithVisibility - )} change from ${change.lhs} to ${change.rhs}` - ) - } - break - case 'N': - if (path.length === 1) { - if (change.rhs.role) { - lines.push( - ` - will join the organization as a ${change.rhs.role} (remind them to accept the email invitation)` - ) - } - if (change.rhs.repositories) { - const repositories = change.rhs.repositories as unknown as Record< - string, - {permission: string} - > - for (const [repository, {permission}] of Object.entries( - repositories - )) { - lines.push( - ` - will gain ${permission} permission to ${repositoryLabel( - repository, - afterWithVisibility, - beforeWithVisibility - )}` - ) - } - } - } else { - const repository = String(path[2]) - lines.push( - ` - will gain ${change.rhs.permission} permission to ${repositoryLabel( - repository, - afterWithVisibility, - beforeWithVisibility - )}` - ) - } - break - case 'D': - if (path.length === 1) { - if (change.lhs.role) { - lines.push(' - will leave the organization') - } - if (change.lhs.repositories) { - const repositories = change.lhs.repositories as unknown as Record< - string, - {permission: string} - > - for (const [repository, {permission}] of Object.entries( - repositories - )) { - lines.push( - ` - will lose ${permission} permission to ${repositoryLabel( - repository, - afterWithVisibility, - beforeWithVisibility - )}` - ) - } - } - } else { - const repository = String(path[2]) - lines.push( - ` - will lose ${change.lhs.permission} permission to ${repositoryLabel( - repository, - afterWithVisibility, - beforeWithVisibility - )}` - ) - } - break + + if ( + beforeRepositoryAccess === undefined && + afterRepositoryAccess !== undefined + ) { + userLines.push( + ` - will gain ${formatRepositoryAccessDescription( + repository, + afterRepositoryAccess + )}` + ) + } else if ( + beforeRepositoryAccess !== undefined && + afterRepositoryAccess === undefined + ) { + userLines.push( + ` - will lose ${formatRepositoryAccessDescription( + repository, + beforeRepositoryAccess + )}` + ) + } else if ( + beforeRepositoryAccess !== undefined && + afterRepositoryAccess !== undefined + ) { + userLines.push( + ` - will change from having ${formatRepositoryAccessDescription( + repository, + beforeRepositoryAccess + )} to having ${formatRepositoryAccessDescription( + repository, + afterRepositoryAccess + )}` + ) } } + + if (userLines.length > 0) { + lines.push(`User ${username}:`, ...userLines) + } } - return changes.length > 0 - ? lines.join('\n') - : 'There will be no access changes' + return lines.length > 0 ? lines.join('\n') : 'There will be no access changes' } From 34be3af9ef5c6a03f282eb6894ef45849fba8023 Mon Sep 17 00:00:00 2001 From: galargh Date: Sun, 9 Aug 2026 11:31:58 +0200 Subject: [PATCH 06/13] Fix access change comments --- .github/workflows/access-report.yml | 63 ------------------- .../__tests__/actions/access-summary.test.ts | 4 +- scripts/__tests__/workflows.test.ts | 15 ++--- scripts/src/actions/access-report.ts | 18 ------ .../actions/shared/describe-access-changes.ts | 15 ++--- 5 files changed, 17 insertions(+), 98 deletions(-) delete mode 100644 .github/workflows/access-report.yml delete mode 100644 scripts/src/actions/access-report.ts diff --git a/.github/workflows/access-report.yml b/.github/workflows/access-report.yml deleted file mode 100644 index b7907da..0000000 --- a/.github/workflows/access-report.yml +++ /dev/null @@ -1,63 +0,0 @@ -name: Access Report - -on: - workflow_dispatch: - inputs: - organization: - description: Organization config to report on - required: true - -defaults: - run: - shell: bash - -jobs: - report: - permissions: - contents: read - name: Access report - runs-on: ubuntu-latest - environment: read - env: - TF_IN_AUTOMATION: 1 - TF_INPUT: 0 - TF_WORKSPACE: ${{ github.event.inputs.organization }} - AWS_ACCESS_KEY_ID: ${{ secrets.RO_AWS_ACCESS_KEY_ID }} - AWS_SECRET_ACCESS_KEY: ${{ secrets.RO_AWS_SECRET_ACCESS_KEY }} - steps: - - name: Checkout - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - - name: Setup terraform - uses: hashicorp/setup-terraform@5e8dbf3c6d9deaf4193ca7a8fb23f2ac83bb6c85 # v4.0.0 - with: - terraform_version: 1.12.0 - terraform_wrapper: false - - name: Initialize terraform - run: terraform init - working-directory: terraform - - name: Install pnpm - uses: pnpm/action-setup@91ab88e2619ed1f46221f0ba42d1492c02baf788 # v6.0.6 - with: - version: 10 - - name: Use Node.js lts/* - uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 - with: - node-version: lts/* - cache: '' - - name: Initialize scripts - run: pnpm install --frozen-lockfile && pnpm run build - working-directory: scripts - - name: Generate access report - run: node lib/actions/access-report.js - working-directory: scripts - env: - ACCESS_REPORT_PATH: ../ACCESS_REPORT.md - - name: Publish access report summary - run: cat ACCESS_REPORT.md >> "$GITHUB_STEP_SUMMARY" - - name: Upload access report - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 - with: - name: access-report-${{ env.TF_WORKSPACE }} - path: ACCESS_REPORT.md - if-no-files-found: error - retention-days: 14 diff --git a/scripts/__tests__/actions/access-summary.test.ts b/scripts/__tests__/actions/access-summary.test.ts index 6abe256..5d9699d 100644 --- a/scripts/__tests__/actions/access-summary.test.ts +++ b/scripts/__tests__/actions/access-summary.test.ts @@ -206,7 +206,9 @@ members: const comment = describeAccessChangesComment(state, config) - assert.match(comment, /Access Changes<\/summary>/) + assert.match(comment, /The following access changes/) + assert.match(comment, /For the full access breakdown/) + assert.doesNotMatch(comment, /
/) assert.doesNotMatch(comment, /Potential no members/) assert.doesNotMatch(comment, /Any other members/) }) diff --git a/scripts/__tests__/workflows.test.ts b/scripts/__tests__/workflows.test.ts index 8ee388f..fb19575 100644 --- a/scripts/__tests__/workflows.test.ts +++ b/scripts/__tests__/workflows.test.ts @@ -1,5 +1,5 @@ import assert from 'node:assert' -import {readFileSync} from 'node:fs' +import {existsSync, readFileSync} from 'node:fs' import {describe, it} from 'node:test' import * as YAML from 'yaml' @@ -47,13 +47,14 @@ describe('workflows', () => { assert.match(applyStep.run ?? '', /allow_destroy_override\.tf\.disabled/) }) - it('provides a manual access report workflow with summary and artifact output', () => { - const report = workflow('access-report.yml') - const steps = report.jobs.report.steps.map(step => step.name) + it('does not provide a manual access report workflow', () => { + assert.equal(existsSync('../.github/workflows/access-report.yml'), false) + }) + + it('publishes the full access report from the fix workflow', () => { + const fix = workflow('fix.yml') + const steps = fix.jobs.fix.steps.map(step => step.name) - assert.ok(report.on.workflow_dispatch) - assert.equal(report.jobs.report.environment, 'read') - assert.ok(steps.includes('Generate access report')) assert.ok(steps.includes('Publish access report summary')) assert.ok(steps.includes('Upload access report')) }) diff --git a/scripts/src/actions/access-report.ts b/scripts/src/actions/access-report.ts deleted file mode 100644 index 4dcc592..0000000 --- a/scripts/src/actions/access-report.ts +++ /dev/null @@ -1,18 +0,0 @@ -import 'reflect-metadata' - -import * as fs from 'fs' -import * as core from '@actions/core' -import {Config} from '../yaml/config.js' -import {State} from '../terraform/state.js' -import {describeAccessReport} from './shared/describe-access-changes.js' - -async function run(): Promise { - const state = await State.New() - const config = Config.FromPath() - const accessReport = describeAccessReport(state, config) - const accessReportPath = process.env.ACCESS_REPORT_PATH ?? 'ACCESS_REPORT.md' - - fs.writeFileSync(accessReportPath, accessReport) -} - -run().catch(error => core.setFailed(error)) diff --git a/scripts/src/actions/shared/describe-access-changes.ts b/scripts/src/actions/shared/describe-access-changes.ts index 3e328d8..58d7b7f 100644 --- a/scripts/src/actions/shared/describe-access-changes.ts +++ b/scripts/src/actions/shared/describe-access-changes.ts @@ -40,28 +40,25 @@ export function describeAccessChangesComment( runUrl = workflowRunUrl() ): string { const accessChangesDescription = describeAccessChanges(state, config) + const reportDestination = + runUrl === undefined + ? 'the Fix workflow summary or access report artifact' + : `[the Fix workflow summary or access report artifact](${runUrl})` const comment = [ 'The following access changes will be introduced as a result of applying the plan:', '', - '
Access Changes', - '', '```', accessChangesDescription, '```', '', - '
' + `For the full access breakdown, inspect ${reportDestination}.` ].join('\n') if (Buffer.byteLength(comment, 'utf8') < maxLength) { return comment } - const destination = - runUrl === undefined - ? 'the Fix workflow summary or the access report artifact' - : `[the Fix workflow summary or access report artifact](${runUrl})` - - return `Access changes are too long to post as a comment. Please inspect ${destination} instead.` + return `Access changes are too long to post as a comment. Please inspect ${reportDestination} instead.` } export function describeAccessReport(state: State, config: Config): string { From 7886c8b759c10bccbfeca69584ff47613b0dd139 Mon Sep 17 00:00:00 2001 From: galargh Date: Sun, 9 Aug 2026 12:00:22 +0200 Subject: [PATCH 07/13] Restore access comment details --- scripts/__tests__/actions/access-summary.test.ts | 2 +- scripts/src/actions/shared/describe-access-changes.ts | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/scripts/__tests__/actions/access-summary.test.ts b/scripts/__tests__/actions/access-summary.test.ts index 5d9699d..eeace16 100644 --- a/scripts/__tests__/actions/access-summary.test.ts +++ b/scripts/__tests__/actions/access-summary.test.ts @@ -208,7 +208,7 @@ members: assert.match(comment, /The following access changes/) assert.match(comment, /For the full access breakdown/) - assert.doesNotMatch(comment, /
/) + assert.match(comment, /
Access Changes<\/summary>/) assert.doesNotMatch(comment, /Potential no members/) assert.doesNotMatch(comment, /Any other members/) }) diff --git a/scripts/src/actions/shared/describe-access-changes.ts b/scripts/src/actions/shared/describe-access-changes.ts index 58d7b7f..b4120a4 100644 --- a/scripts/src/actions/shared/describe-access-changes.ts +++ b/scripts/src/actions/shared/describe-access-changes.ts @@ -47,10 +47,14 @@ export function describeAccessChangesComment( const comment = [ 'The following access changes will be introduced as a result of applying the plan:', '', + '
Access Changes', + '', '```', accessChangesDescription, '```', '', + '
', + '', `For the full access breakdown, inspect ${reportDestination}.` ].join('\n') From 1598fbf42a990ae08975d90d1e05011c5cf5534f Mon Sep 17 00:00:00 2001 From: galargh Date: Sun, 9 Aug 2026 12:19:46 +0200 Subject: [PATCH 08/13] Move access report side effect to shared action --- .../__tests__/actions/access-summary.test.ts | 29 ++++++++++++++++++- scripts/src/actions/fix-yaml-config.ts | 15 ++-------- .../actions/shared/describe-access-changes.ts | 4 +++ 3 files changed, 34 insertions(+), 14 deletions(-) diff --git a/scripts/__tests__/actions/access-summary.test.ts b/scripts/__tests__/actions/access-summary.test.ts index eeace16..3bed0e2 100644 --- a/scripts/__tests__/actions/access-summary.test.ts +++ b/scripts/__tests__/actions/access-summary.test.ts @@ -1,7 +1,10 @@ import 'reflect-metadata' import assert from 'node:assert' +import {mkdtempSync, readFileSync, rmSync} from 'node:fs' +import {join} from 'node:path' import {describe, it} from 'node:test' +import {tmpdir} from 'node:os' import {Config} from '../../src/yaml/config.js' import {State} from '../../src/terraform/state.js' import { @@ -11,7 +14,8 @@ import { import { describeAccessChanges, describeAccessChangesComment, - describeAccessReport + describeAccessReport, + runDescribeAccessChanges } from '../../src/actions/shared/describe-access-changes.js' import {StateSchema} from '../../src/terraform/schema.js' @@ -235,4 +239,27 @@ members: 'Access changes are too long to post as a comment. Please inspect [the Fix workflow summary or access report artifact](https://github.example/runs/1) instead.' ) }) + + it('writes the full access report as a side effect of the action helper', async () => { + const dir = mkdtempSync(join(tmpdir(), 'access-report-')) + const reportPath = join(dir, 'ACCESS_REPORT.md') + const originalPath = process.env.ACCESS_REPORT_PATH + + try { + process.env.ACCESS_REPORT_PATH = reportPath + const comment = await runDescribeAccessChanges() + const report = readFileSync(reportPath, 'utf8') + + assert.match(comment, /
Access Changes<\/summary>/) + assert.doesNotMatch(comment, /Potential no members/) + assert.match(report, /Potential no members<\/summary>/) + } finally { + if (originalPath === undefined) { + delete process.env.ACCESS_REPORT_PATH + } else { + process.env.ACCESS_REPORT_PATH = originalPath + } + rmSync(dir, {recursive: true, force: true}) + } + }) }) diff --git a/scripts/src/actions/fix-yaml-config.ts b/scripts/src/actions/fix-yaml-config.ts index 0a163bf..142297c 100644 --- a/scripts/src/actions/fix-yaml-config.ts +++ b/scripts/src/actions/fix-yaml-config.ts @@ -1,26 +1,15 @@ import 'reflect-metadata' -import * as fs from 'fs' import {runToggleArchivedRepos} from './shared/toggle-archived-repos.js' -import { - describeAccessChangesComment, - describeAccessReport -} from './shared/describe-access-changes.js' -import {Config} from '../yaml/config.js' -import {State} from '../terraform/state.js' +import {runDescribeAccessChanges} from './shared/describe-access-changes.js' import * as core from '@actions/core' async function run(): Promise { await runToggleArchivedRepos() - const state = await State.New() - const config = Config.FromPath() - const accessChangesComment = describeAccessChangesComment(state, config) - const accessReport = describeAccessReport(state, config) - const accessReportPath = process.env.ACCESS_REPORT_PATH ?? 'ACCESS_REPORT.md' + const accessChangesComment = await runDescribeAccessChanges() - fs.writeFileSync(accessReportPath, accessReport) core.setOutput('comment', accessChangesComment) } diff --git a/scripts/src/actions/shared/describe-access-changes.ts b/scripts/src/actions/shared/describe-access-changes.ts index b4120a4..3a163d7 100644 --- a/scripts/src/actions/shared/describe-access-changes.ts +++ b/scripts/src/actions/shared/describe-access-changes.ts @@ -1,6 +1,7 @@ import {Config} from '../../yaml/config.js' import {State} from '../../terraform/state.js' import * as core from '@actions/core' +import * as fs from 'fs' import { categorizeAccessSummary, formatAccessSummarySection, @@ -13,7 +14,10 @@ const GITHUB_COMMENT_LENGTH_LIMIT = 65000 export async function runDescribeAccessChanges(): Promise { const state = await State.New() const config = Config.FromPath() + const accessReport = describeAccessReport(state, config) + const accessReportPath = process.env.ACCESS_REPORT_PATH ?? 'ACCESS_REPORT.md' + fs.writeFileSync(accessReportPath, accessReport) return describeAccessChangesComment(state, config) } From beb7c27cfafba96c6cf705b1e8ae84f03741be8f Mon Sep 17 00:00:00 2001 From: galargh Date: Sun, 9 Aug 2026 17:52:41 +0200 Subject: [PATCH 09/13] Filter fix workflow config artifacts --- .github/workflows/fix.yml | 8 ++++---- scripts/__tests__/workflows.test.ts | 27 +++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/.github/workflows/fix.yml b/.github/workflows/fix.yml index dba81be..50248cd 100644 --- a/.github/workflows/fix.yml +++ b/.github/workflows/fix.yml @@ -133,7 +133,7 @@ jobs: - name: Upload YAML config uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: - name: ${{ env.TF_WORKSPACE }}.yml + name: fixed-config-${{ env.TF_WORKSPACE }} path: github/${{ env.TF_WORKSPACE }}.yml if-no-files-found: error retention-days: 1 @@ -177,10 +177,10 @@ jobs: uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: path: artifacts + pattern: fixed-config-* + merge-multiple: true - name: Copy YAML configs - run: | - shopt -s globstar - cp artifacts/**/*.yml head/github + run: cp artifacts/*.yml head/github - name: Check if github was modified id: github-modified run: | diff --git a/scripts/__tests__/workflows.test.ts b/scripts/__tests__/workflows.test.ts index fb19575..993d53e 100644 --- a/scripts/__tests__/workflows.test.ts +++ b/scripts/__tests__/workflows.test.ts @@ -7,6 +7,7 @@ type WorkflowStep = { name?: string run?: string env?: Record + with?: Record } type Workflow = { @@ -58,4 +59,30 @@ describe('workflows', () => { assert.ok(steps.includes('Publish access report summary')) assert.ok(steps.includes('Upload access report')) }) + + it('downloads only fixed YAML config artifacts before pushing fix changes', () => { + const fix = workflow('fix.yml') + const fixSteps = fix.jobs.fix.steps + const pushSteps = fix.jobs.push.steps + const uploadYamlStep = fixSteps.find( + step => step.name === 'Upload YAML config' + ) + const downloadYamlStep = pushSteps.find( + step => step.name === 'Download YAML configs' + ) + const copyYamlStep = pushSteps.find( + step => step.name === 'Copy YAML configs' + ) + + assert.ok(uploadYamlStep) + assert.ok(downloadYamlStep) + assert.ok(copyYamlStep) + assert.equal( + uploadYamlStep.with?.name, + 'fixed-config-${{ env.TF_WORKSPACE }}' + ) + assert.equal(downloadYamlStep.with?.pattern, 'fixed-config-*') + assert.equal(downloadYamlStep.with?.['merge-multiple'], true) + assert.equal(copyYamlStep.run, 'cp artifacts/*.yml head/github') + }) }) From 3fe55d32c832526c9036cc122071be755d2defbb Mon Sep 17 00:00:00 2001 From: galargh Date: Sun, 9 Aug 2026 18:56:24 +0200 Subject: [PATCH 10/13] Surface plan and apply summaries --- .github/workflows/apply.yml | 51 ++++++++++++- .github/workflows/plan.yml | 26 +++++++ CHANGELOG.md | 2 + .../__tests__/actions/access-summary.test.ts | 14 +++- scripts/__tests__/workflows.test.ts | 75 +++++++++++++++++++ .../actions/shared/describe-access-changes.ts | 10 ++- 6 files changed, 170 insertions(+), 8 deletions(-) diff --git a/.github/workflows/apply.yml b/.github/workflows/apply.yml index 4b619f5..840cfaa 100644 --- a/.github/workflows/apply.yml +++ b/.github/workflows/apply.yml @@ -140,19 +140,66 @@ jobs: exit 1 fi cp allow_destroy_override.tf.disabled allow_destroy_override.tf + - name: Summarize apply target + env: + REVIEWED_SHA: ${{ needs.prepare.outputs.sha }} + run: | + { + echo '## Apply target' + echo '' + echo "- Reviewed SHA: \`${REVIEWED_SHA}\`" + echo "- Workspace: \`${TF_WORKSPACE}\`" + echo "- Environment: \`${{ matrix.environment }}\`" + echo "- Reviewed plan artifact: \`${TF_WORKSPACE}_${REVIEWED_SHA}.tfplan\`" + } >> "$GITHUB_STEP_SUMMARY" - name: Download reviewed terraform plan env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} SHA: ${{ needs.prepare.outputs.sha }} run: gh run download -n "${TF_WORKSPACE}_${SHA}.tfplan" --repo "${GITHUB_REPOSITORY}" + - name: Show reviewed terraform plan + run: | + terraform show -no-color "${TF_WORKSPACE}.tfplan" > "${TF_WORKSPACE}.reviewed.txt" + { + echo '## Reviewed Terraform plan' + echo '' + echo "
${TF_WORKSPACE}.tfplan" + echo '' + echo '~~~~terraform' + sed 's/^~~~~/~~~~ /' "${TF_WORKSPACE}.reviewed.txt" + echo '~~~~' + echo '' + echo '
' + } >> "$GITHUB_STEP_SUMMARY" - name: Replan merged commit run: | terraform show -json > "$TF_WORKSPACE.tfstate.json" terraform plan -refresh=false -lock=false -out="${TF_WORKSPACE}.merged.tfplan" -no-color - - name: Compare reviewed and merged plans + - name: Show merged terraform plan run: | - terraform show -no-color "${TF_WORKSPACE}.tfplan" > "${TF_WORKSPACE}.reviewed.txt" terraform show -no-color "${TF_WORKSPACE}.merged.tfplan" > "${TF_WORKSPACE}.merged.txt" + { + echo '## Merged Terraform plan' + echo '' + echo "
${TF_WORKSPACE}.merged.tfplan" + echo '' + echo '~~~~terraform' + sed 's/^~~~~/~~~~ /' "${TF_WORKSPACE}.merged.txt" + echo '~~~~' + echo '' + echo '
' + } >> "$GITHUB_STEP_SUMMARY" + - name: Upload apply plan summaries + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: apply-plans-${{ env.TF_WORKSPACE }}-${{ needs.prepare.outputs.sha }} + path: | + terraform/${{ env.TF_WORKSPACE }}.reviewed.txt + terraform/${{ env.TF_WORKSPACE }}.merged.txt + if-no-files-found: error + retention-days: 14 + - name: Compare reviewed and merged plans + run: | diff -u "${TF_WORKSPACE}.reviewed.txt" "${TF_WORKSPACE}.merged.txt" - name: Terraform Apply run: | diff --git a/.github/workflows/plan.yml b/.github/workflows/plan.yml index 8bbc433..73da5da 100644 --- a/.github/workflows/plan.yml +++ b/.github/workflows/plan.yml @@ -150,6 +150,22 @@ jobs: fi cp allow_destroy_override.tf.disabled allow_destroy_override.tf working-directory: terraform + - name: Summarize plan target + env: + SOURCE_SHA: ${{ github.event.pull_request.head.sha || github.sha }} + PULL_REQUEST_NUMBER: ${{ github.event.pull_request.number || '' }} + run: | + { + echo '## Plan target' + echo '' + if [[ -n "${PULL_REQUEST_NUMBER}" ]]; then + echo "- Pull request: #${PULL_REQUEST_NUMBER}" + fi + echo "- Source SHA: \`${SOURCE_SHA}\`" + echo "- Workspace: \`${TF_WORKSPACE}\`" + echo "- Environment: \`${{ matrix.environment }}\`" + echo "- Terraform plan artifact: \`${TF_WORKSPACE}_${SOURCE_SHA}.tfplan\`" + } >> "$GITHUB_STEP_SUMMARY" - name: Plan terraform run: | terraform show -json > "$TF_WORKSPACE.tfstate.json" @@ -210,6 +226,16 @@ jobs: done cat TERRAFORM_PLANS.md working-directory: terraform + - name: Publish terraform plans summary + run: cat TERRAFORM_PLANS.md >> "$GITHUB_STEP_SUMMARY" + working-directory: terraform + - name: Upload terraform plans summary + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: terraform-plans-${{ github.event.pull_request.head.sha || github.sha }} + path: terraform/TERRAFORM_PLANS.md + if-no-files-found: error + retention-days: 14 - name: Prepare comment run: | delimiter="$(uuidgen)" diff --git a/CHANGELOG.md b/CHANGELOG.md index 1fedcfd..a00febb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - new args for repositories and branch protection rules ### Changed +- plan/apply workflows now publish planned/applied commit, workspace, environment, and rendered terraform plan details to workflow summaries and artifacts +- access report member classifications now explicitly describe the post-change access state - workflows: added separate GitHub Actions environments for reading organization state, writing organization state, and pushing repository changes - **BREAKING**: access changes action now emits only the access change comment by default; update custom usage to avoid nesting the full access breakdown in PR comments - workflows: pin third-party actions to latest release SHAs and replan from the merged commit before applying diff --git a/scripts/__tests__/actions/access-summary.test.ts b/scripts/__tests__/actions/access-summary.test.ts index 3bed0e2..a394835 100644 --- a/scripts/__tests__/actions/access-summary.test.ts +++ b/scripts/__tests__/actions/access-summary.test.ts @@ -140,7 +140,14 @@ repositories: changes, /will change from having direct pull permission to public-repo \(public\) to having direct push permission to public-repo \(public\)/ ) - assert.match(report, /Potential outside collaborators<\/summary>/) + assert.match( + report, + /The sections below describe effective access after these config changes are applied:/ + ) + assert.match( + report, + /Post-change potential outside collaborators<\/summary>/ + ) assert.match(report, /Affected users: alice/) assert.match(report, /User alice \(member\):/) assert.match(report, /has direct push permission to public-repo \(public\)/) @@ -252,7 +259,10 @@ members: assert.match(comment, /
Access Changes<\/summary>/) assert.doesNotMatch(comment, /Potential no members/) - assert.match(report, /Potential no members<\/summary>/) + assert.match( + report, + /Post-change potential no members<\/summary>/ + ) } finally { if (originalPath === undefined) { delete process.env.ACCESS_REPORT_PATH diff --git a/scripts/__tests__/workflows.test.ts b/scripts/__tests__/workflows.test.ts index 993d53e..8a99329 100644 --- a/scripts/__tests__/workflows.test.ts +++ b/scripts/__tests__/workflows.test.ts @@ -85,4 +85,79 @@ describe('workflows', () => { assert.equal(downloadYamlStep.with?.['merge-multiple'], true) assert.equal(copyYamlStep.run, 'cp artifacts/*.yml head/github') }) + + it('publishes planned terraform targets and rendered plan summaries', () => { + const plan = workflow('plan.yml') + const planSteps = plan.jobs.plan.steps + const commentSteps = plan.jobs.comment.steps + const targetStep = planSteps.find( + step => step.name === 'Summarize plan target' + ) + const publishStep = commentSteps.find( + step => step.name === 'Publish terraform plans summary' + ) + const uploadStep = commentSteps.find( + step => step.name === 'Upload terraform plans summary' + ) + + assert.ok(targetStep) + assert.match(targetStep.run ?? '', /## Plan target/) + assert.match(targetStep.run ?? '', /Pull request/) + assert.match(targetStep.run ?? '', /Source SHA/) + assert.match(targetStep.run ?? '', /Terraform plan artifact/) + assert.ok(publishStep) + assert.equal( + publishStep.run, + 'cat TERRAFORM_PLANS.md >> "$GITHUB_STEP_SUMMARY"' + ) + assert.ok(uploadStep) + assert.equal( + uploadStep.with?.name, + 'terraform-plans-${{ github.event.pull_request.head.sha || github.sha }}' + ) + assert.equal(uploadStep.with?.path, 'terraform/TERRAFORM_PLANS.md') + }) + + it('publishes apply targets and reviewed plan summaries', () => { + const apply = workflow('apply.yml') + const steps = apply.jobs.apply.steps + const targetStep = steps.find( + step => step.name === 'Summarize apply target' + ) + const reviewedStep = steps.find( + step => step.name === 'Show reviewed terraform plan' + ) + const mergedStep = steps.find( + step => step.name === 'Show merged terraform plan' + ) + const uploadStep = steps.find( + step => step.name === 'Upload apply plan summaries' + ) + const compareStep = steps.find( + step => step.name === 'Compare reviewed and merged plans' + ) + + assert.ok(targetStep) + assert.match(targetStep.run ?? '', /## Apply target/) + assert.match(targetStep.run ?? '', /Reviewed SHA/) + assert.match(targetStep.run ?? '', /Reviewed plan artifact/) + assert.ok(reviewedStep) + assert.match(reviewedStep.run ?? '', /## Reviewed Terraform plan/) + assert.match(reviewedStep.run ?? '', /\.reviewed\.txt/) + assert.ok(mergedStep) + assert.match(mergedStep.run ?? '', /## Merged Terraform plan/) + assert.match(mergedStep.run ?? '', /\.merged\.txt/) + assert.ok(uploadStep) + assert.equal( + uploadStep.with?.name, + 'apply-plans-${{ env.TF_WORKSPACE }}-${{ needs.prepare.outputs.sha }}' + ) + assert.match(String(uploadStep.with?.path), /\.reviewed\.txt/) + assert.match(String(uploadStep.with?.path), /\.merged\.txt/) + assert.ok(compareStep) + assert.equal( + compareStep.run, + 'diff -u "${TF_WORKSPACE}.reviewed.txt" "${TF_WORKSPACE}.merged.txt"\n' + ) + }) }) diff --git a/scripts/src/actions/shared/describe-access-changes.ts b/scripts/src/actions/shared/describe-access-changes.ts index 3a163d7..ab1b580 100644 --- a/scripts/src/actions/shared/describe-access-changes.ts +++ b/scripts/src/actions/shared/describe-access-changes.ts @@ -85,26 +85,28 @@ export function describeAccessReport(state: State, config: Config): string { '', '
', '', + 'The sections below describe effective access after these config changes are applied:', + '', formatAccessSummarySection( - 'Outside collaborators', + 'Post-change outside collaborators', categories.outsideCollaborators, after ), '', formatAccessSummarySection( - 'Potential outside collaborators', + 'Post-change potential outside collaborators', categories.potentialOutsideCollaborators, after ), '', formatAccessSummarySection( - 'Potential no members', + 'Post-change potential no members', categories.potentialNoMembers, after ), '', formatAccessSummarySection( - 'Any other members', + 'Post-change any other members', categories.anyOtherMembers, after ) From 24b2716a631b463b686db982dda1f79df6465382 Mon Sep 17 00:00:00 2001 From: galargh Date: Sun, 9 Aug 2026 19:18:39 +0200 Subject: [PATCH 11/13] Explain allow-destroy classifications --- .github/workflows/apply.yml | 7 ++ .github/workflows/plan.yml | 7 ++ CHANGELOG.md | 1 + .../__tests__/actions/access-summary.test.ts | 41 ++++++++++ .../actions/classify-allow-destroy.test.ts | 62 ++++++++++++++ scripts/__tests__/workflows.test.ts | 10 +++ scripts/src/actions/classify-allow-destroy.ts | 80 ++++++++++++++++--- .../actions/shared/describe-access-changes.ts | 6 +- 8 files changed, 201 insertions(+), 13 deletions(-) diff --git a/.github/workflows/apply.yml b/.github/workflows/apply.yml index 840cfaa..7087b02 100644 --- a/.github/workflows/apply.yml +++ b/.github/workflows/apply.yml @@ -143,6 +143,7 @@ jobs: - name: Summarize apply target env: REVIEWED_SHA: ${{ needs.prepare.outputs.sha }} + ENVIRONMENT_REASONS: ${{ toJson(matrix.environmentReasons) }} run: | { echo '## Apply target' @@ -150,6 +151,12 @@ jobs: echo "- Reviewed SHA: \`${REVIEWED_SHA}\`" echo "- Workspace: \`${TF_WORKSPACE}\`" echo "- Environment: \`${{ matrix.environment }}\`" + if [[ "$(jq 'length' <<< "${ENVIRONMENT_REASONS}")" == '0' ]]; then + echo "- Environment reason: no allow-destroy changes detected" + else + echo "- Environment reason:" + jq -r '.[] | " - " + .' <<< "${ENVIRONMENT_REASONS}" + fi echo "- Reviewed plan artifact: \`${TF_WORKSPACE}_${REVIEWED_SHA}.tfplan\`" } >> "$GITHUB_STEP_SUMMARY" - name: Download reviewed terraform plan diff --git a/.github/workflows/plan.yml b/.github/workflows/plan.yml index 73da5da..069c8e1 100644 --- a/.github/workflows/plan.yml +++ b/.github/workflows/plan.yml @@ -154,6 +154,7 @@ jobs: env: SOURCE_SHA: ${{ github.event.pull_request.head.sha || github.sha }} PULL_REQUEST_NUMBER: ${{ github.event.pull_request.number || '' }} + ENVIRONMENT_REASONS: ${{ toJson(matrix.environmentReasons) }} run: | { echo '## Plan target' @@ -164,6 +165,12 @@ jobs: echo "- Source SHA: \`${SOURCE_SHA}\`" echo "- Workspace: \`${TF_WORKSPACE}\`" echo "- Environment: \`${{ matrix.environment }}\`" + if [[ "$(jq 'length' <<< "${ENVIRONMENT_REASONS}")" == '0' ]]; then + echo "- Environment reason: no allow-destroy changes detected" + else + echo "- Environment reason:" + jq -r '.[] | " - " + .' <<< "${ENVIRONMENT_REASONS}" + fi echo "- Terraform plan artifact: \`${TF_WORKSPACE}_${SOURCE_SHA}.tfplan\`" } >> "$GITHUB_STEP_SUMMARY" - name: Plan terraform diff --git a/CHANGELOG.md b/CHANGELOG.md index a00febb..c1f8e46 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - plan/apply workflows now publish planned/applied commit, workspace, environment, and rendered terraform plan details to workflow summaries and artifacts +- allow-destroy workspace classification now reports which member or repository removals require guarded environments - access report member classifications now explicitly describe the post-change access state - workflows: added separate GitHub Actions environments for reading organization state, writing organization state, and pushing repository changes - **BREAKING**: access changes action now emits only the access change comment by default; update custom usage to avoid nesting the full access breakdown in PR comments diff --git a/scripts/__tests__/actions/access-summary.test.ts b/scripts/__tests__/actions/access-summary.test.ts index a394835..be6d7e8 100644 --- a/scripts/__tests__/actions/access-summary.test.ts +++ b/scripts/__tests__/actions/access-summary.test.ts @@ -205,6 +205,47 @@ teams: ) }) + it('describes member removal with retained public access as outside collaborator transition', () => { + const state = new State( + JSON.stringify({ + values: { + root_module: { + resources: [ + { + mode: 'managed', + index: 'alice', + address: 'github_membership.this["alice"]', + type: 'github_membership', + values: { + username: 'alice', + role: 'member' + } + } + ] + } + } + } satisfies StateSchema) + ) + const config = new Config(` +repositories: + public-repo: + collaborators: + pull: + - alice + visibility: public +`) + + const changes = describeAccessChanges(state, config) + + assert.match(changes, /User alice:/) + assert.match(changes, /will become an outside collaborator/) + assert.doesNotMatch(changes, /will leave the organization/) + assert.match( + changes, + /will gain direct pull permission to public-repo \(public\)/ + ) + }) + it('keeps routine comments to access changes only', () => { const state = new State( JSON.stringify({values: {root_module: {resources: []}}}) diff --git a/scripts/__tests__/actions/classify-allow-destroy.test.ts b/scripts/__tests__/actions/classify-allow-destroy.test.ts index 6d070a2..67969af 100644 --- a/scripts/__tests__/actions/classify-allow-destroy.test.ts +++ b/scripts/__tests__/actions/classify-allow-destroy.test.ts @@ -3,7 +3,9 @@ import 'reflect-metadata' import {describe, it} from 'node:test' import assert from 'node:assert' import { + describeWorkspaceClassification, getEnvironment, + getAllowDestroyReasons, hasAllowDestroyChange, validateRemovedMembersHaveNoDanglingAccess } from '../../src/actions/classify-allow-destroy.js' @@ -92,6 +94,66 @@ members: assert.equal(allowDestroy, true) }) + it('describes why allow-destroy environments are selected', async () => { + setManagedResourceTypes(['github_repository', 'github_membership']) + + const reasons = await getAllowDestroyReasons( + new Config(` +members: + admin: + - kept +repositories: + kept: {} +`), + state({ + values: { + root_module: { + resources: [ + { + mode: 'managed', + type: 'github_membership', + values: {username: 'kept', role: 'admin'} + }, + { + mode: 'managed', + type: 'github_membership', + values: {username: 'removed', role: 'admin'} + }, + { + mode: 'managed', + type: 'github_repository', + values: {name: 'kept'} + }, + { + mode: 'managed', + type: 'github_repository', + values: {name: 'removed'} + } + ] + } + } + }) + ) + const summary = describeWorkspaceClassification({ + include: [ + { + workspace: 'default', + environment: 'read-allow-destroy', + environmentReasons: reasons + } + ] + }) + + assert.deepEqual(reasons, [ + 'removes organization member removed', + 'removes repository removed' + ]) + assert.match(summary, /Workspace classification/) + assert.match(summary, /read-allow-destroy/) + assert.match(summary, /removes organization member removed/) + assert.match(summary, /removes repository removed/) + }) + it('keeps repository and membership updates in normal environments', async () => { setManagedResourceTypes(['github_repository', 'github_membership']) diff --git a/scripts/__tests__/workflows.test.ts b/scripts/__tests__/workflows.test.ts index 8a99329..ad24070 100644 --- a/scripts/__tests__/workflows.test.ts +++ b/scripts/__tests__/workflows.test.ts @@ -101,9 +101,14 @@ describe('workflows', () => { ) assert.ok(targetStep) + assert.equal( + targetStep.env?.ENVIRONMENT_REASONS, + '${{ toJson(matrix.environmentReasons) }}' + ) assert.match(targetStep.run ?? '', /## Plan target/) assert.match(targetStep.run ?? '', /Pull request/) assert.match(targetStep.run ?? '', /Source SHA/) + assert.match(targetStep.run ?? '', /Environment reason/) assert.match(targetStep.run ?? '', /Terraform plan artifact/) assert.ok(publishStep) assert.equal( @@ -138,8 +143,13 @@ describe('workflows', () => { ) assert.ok(targetStep) + assert.equal( + targetStep.env?.ENVIRONMENT_REASONS, + '${{ toJson(matrix.environmentReasons) }}' + ) assert.match(targetStep.run ?? '', /## Apply target/) assert.match(targetStep.run ?? '', /Reviewed SHA/) + assert.match(targetStep.run ?? '', /Environment reason/) assert.match(targetStep.run ?? '', /Reviewed plan artifact/) assert.ok(reviewedStep) assert.match(reviewedStep.run ?? '', /## Reviewed Terraform plan/) diff --git a/scripts/src/actions/classify-allow-destroy.ts b/scripts/src/actions/classify-allow-destroy.ts index a4828c5..bea8b79 100644 --- a/scripts/src/actions/classify-allow-destroy.ts +++ b/scripts/src/actions/classify-allow-destroy.ts @@ -2,6 +2,7 @@ import 'reflect-metadata' import * as core from '@actions/core' import {pathToFileURL} from 'url' +import * as fs from 'fs' import {Config} from '../yaml/config.js' import {State} from '../terraform/state.js' import { @@ -25,6 +26,7 @@ type Matrix = { include: { workspace: string environment: string + environmentReasons: string[] }[] } @@ -32,34 +34,58 @@ function getStateAddress(resource: Resource): string { return resource.getStateAddress().toLowerCase() } -function hasMissingResources( +function formatAllowDestroyReason(resource: Resource): string { + if (resource instanceof Member) { + return `removes organization member ${resource.username.toLowerCase()}` + } + + if (resource instanceof Repository) { + return `removes repository ${resource.name.toLowerCase()}` + } + + return `removes ${resource.getStateAddress().toLowerCase()}` +} + +function getMissingResources( config: Config, state: State, resourceClass: ResourceConstructor -): boolean { +): T[] { const desiredAddresses = new Set( config.getResources(resourceClass).map(getStateAddress) ) return state .getResources(resourceClass) - .some(resource => !desiredAddresses.has(getStateAddress(resource))) + .filter(resource => !desiredAddresses.has(getStateAddress(resource))) } -export async function hasAllowDestroyChange( +export async function getAllowDestroyReasons( config: Config, state: State -): Promise { +): Promise { + const reasons = [] + for (const resourceClass of ALLOW_DESTROY_RESOURCE_CLASSES) { if ( ResourceConstructors.includes(resourceClass) && - !(await state.isIgnored(resourceClass)) && - hasMissingResources(config, state, resourceClass) + !(await state.isIgnored(resourceClass)) ) { - return true + reasons.push( + ...getMissingResources(config, state, resourceClass).map( + formatAllowDestroyReason + ) + ) } } - return false + return reasons.sort() +} + +export async function hasAllowDestroyChange( + config: Config, + state: State +): Promise { + return (await getAllowDestroyReasons(config, state)).length > 0 } export async function validateRemovedMembersHaveNoDanglingAccess( @@ -140,6 +166,32 @@ export function getEnvironment(mode: Mode, allowDestroy: boolean): string { return allowDestroy ? `${mode}-allow-destroy` : mode } +export function describeWorkspaceClassification(matrix: Matrix): string { + const lines = [ + '## Workspace classification', + '', + '| Workspace | Environment | Reason |', + '| --- | --- | --- |' + ] + + for (const item of matrix.include) { + const reasons = + item.environmentReasons.length === 0 + ? 'No allow-destroy changes detected.' + : item.environmentReasons.join('
') + lines.push(`| ${item.workspace} | ${item.environment} | ${reasons} |`) + } + + return lines.join('\n') +} + +function writeStepSummary(markdown: string): void { + const summaryPath = process.env.GITHUB_STEP_SUMMARY + if (summaryPath !== undefined) { + fs.appendFileSync(summaryPath, `${markdown}\n`) + } +} + export async function classifyWorkspaces({ mode, workspaces, @@ -158,10 +210,13 @@ export async function classifyWorkspaces({ const config = Config.FromPath(`${githubDir}/${workspace}.yml`) const state = await State.New() await validateRemovedMembersHaveNoDanglingAccess(config, state) - const allowDestroy = await hasAllowDestroyChange(config, state) - const environment = getEnvironment(mode, allowDestroy) + const environmentReasons = await getAllowDestroyReasons(config, state) + const environment = getEnvironment(mode, environmentReasons.length > 0) core.info(`${workspace}: ${environment}`) - include.push({workspace, environment}) + for (const reason of environmentReasons) { + core.info(`- ${reason}`) + } + include.push({workspace, environment, environmentReasons}) } } finally { if (originalWorkspace === undefined) { @@ -191,6 +246,7 @@ async function run(): Promise { githubDir: process.env.GITHUB_DIR ?? '../github' }) + writeStepSummary(describeWorkspaceClassification(matrix)) core.setOutput('matrix', JSON.stringify(matrix)) } diff --git a/scripts/src/actions/shared/describe-access-changes.ts b/scripts/src/actions/shared/describe-access-changes.ts index ab1b580..ea3093c 100644 --- a/scripts/src/actions/shared/describe-access-changes.ts +++ b/scripts/src/actions/shared/describe-access-changes.ts @@ -138,7 +138,11 @@ export function describeAccessChanges(state: State, config: Config): string { beforeAccess?.role !== undefined && afterAccess?.role === undefined ) { - userLines.push(' - will leave the organization') + if (afterAccess?.isOutsideCollaborator) { + userLines.push(' - will become an outside collaborator') + } else { + userLines.push(' - will leave the organization') + } } else { userLines.push( ` - will have the role in the organization change from ${beforeAccess?.role} to ${afterAccess?.role}` From 55105e6eb07ced687dfb17298cde91302aaa07ab Mon Sep 17 00:00:00 2001 From: galargh Date: Sun, 9 Aug 2026 19:50:36 +0200 Subject: [PATCH 12/13] Restore manual access report workflow --- .github/workflows/access-report.yml | 63 +++++++++++++++++++++++++++++ CHANGELOG.md | 1 + scripts/__tests__/workflows.test.ts | 30 +++++++++++++- 3 files changed, 92 insertions(+), 2 deletions(-) create mode 100644 .github/workflows/access-report.yml diff --git a/.github/workflows/access-report.yml b/.github/workflows/access-report.yml new file mode 100644 index 0000000..ce30866 --- /dev/null +++ b/.github/workflows/access-report.yml @@ -0,0 +1,63 @@ +name: Access Report + +on: + workflow_dispatch: + inputs: + organization: + description: Organization config to report on + required: true + +defaults: + run: + shell: bash + +jobs: + report: + permissions: + contents: read + name: Access report + runs-on: ubuntu-latest + environment: read + env: + TF_IN_AUTOMATION: 1 + TF_INPUT: 0 + TF_WORKSPACE: ${{ inputs.organization }} + AWS_ACCESS_KEY_ID: ${{ secrets.RO_AWS_ACCESS_KEY_ID }} + AWS_SECRET_ACCESS_KEY: ${{ secrets.RO_AWS_SECRET_ACCESS_KEY }} + steps: + - name: Checkout + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - name: Setup terraform + uses: hashicorp/setup-terraform@5e8dbf3c6d9deaf4193ca7a8fb23f2ac83bb6c85 # v4.0.0 + with: + terraform_version: 1.12.0 + terraform_wrapper: false + - name: Initialize terraform + run: terraform init + working-directory: terraform + - name: Install pnpm + uses: pnpm/action-setup@91ab88e2619ed1f46221f0ba42d1492c02baf788 # v6.0.6 + with: + version: 10 + - name: Use Node.js lts/* + uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 + with: + node-version: lts/* + cache: '' + - name: Initialize scripts + run: pnpm install --frozen-lockfile && pnpm run build + working-directory: scripts + - name: Generate access report + run: node --input-type=module --eval "import {runDescribeAccessChanges} from './lib/actions/shared/describe-access-changes.js'; await runDescribeAccessChanges();" + working-directory: scripts + env: + ACCESS_REPORT_PATH: ../ACCESS_REPORT.md + - name: Publish access report summary + run: cat ACCESS_REPORT.md >> "$GITHUB_STEP_SUMMARY" + - name: Upload access report + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: access-report-${{ env.TF_WORKSPACE }} + path: ACCESS_REPORT.md + if-no-files-found: error + retention-days: 14 diff --git a/CHANGELOG.md b/CHANGELOG.md index c1f8e46..aaaabc8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - plan/apply workflows now publish planned/applied commit, workspace, environment, and rendered terraform plan details to workflow summaries and artifacts +- manual access report workflow for generating the full access breakdown on demand - allow-destroy workspace classification now reports which member or repository removals require guarded environments - access report member classifications now explicitly describe the post-change access state - workflows: added separate GitHub Actions environments for reading organization state, writing organization state, and pushing repository changes diff --git a/scripts/__tests__/workflows.test.ts b/scripts/__tests__/workflows.test.ts index ad24070..cbc28d2 100644 --- a/scripts/__tests__/workflows.test.ts +++ b/scripts/__tests__/workflows.test.ts @@ -48,8 +48,34 @@ describe('workflows', () => { assert.match(applyStep.run ?? '', /allow_destroy_override\.tf\.disabled/) }) - it('does not provide a manual access report workflow', () => { - assert.equal(existsSync('../.github/workflows/access-report.yml'), false) + it('provides a manual access report workflow through the shared formatter helper', () => { + assert.equal(existsSync('../.github/workflows/access-report.yml'), true) + + const accessReport = workflow('access-report.yml') + const reportJob = accessReport.jobs.report + const steps = reportJob.steps + const generateStep = steps.find( + step => step.name === 'Generate access report' + ) + const publishStep = steps.find( + step => step.name === 'Publish access report summary' + ) + const uploadStep = steps.find(step => step.name === 'Upload access report') + + assert.ok(accessReport.on.workflow_dispatch) + assert.equal(reportJob.environment, 'read') + assert.ok(generateStep) + assert.equal(generateStep.env?.ACCESS_REPORT_PATH, '../ACCESS_REPORT.md') + assert.match(generateStep.run ?? '', /runDescribeAccessChanges/) + assert.doesNotMatch(generateStep.run ?? '', /access-report\.js/) + assert.ok(publishStep) + assert.equal( + publishStep.run, + 'cat ACCESS_REPORT.md >> "$GITHUB_STEP_SUMMARY"' + ) + assert.ok(uploadStep) + assert.equal(uploadStep.with?.name, 'access-report-${{ env.TF_WORKSPACE }}') + assert.equal(uploadStep.with?.path, 'ACCESS_REPORT.md') }) it('publishes the full access report from the fix workflow', () => { From 018413b280aff00edb6c675003df8b4687ee6eaa Mon Sep 17 00:00:00 2001 From: galargh Date: Sun, 9 Aug 2026 20:00:17 +0200 Subject: [PATCH 13/13] Use app token for update member PRs --- .github/workflows/update-members.yml | 84 ++++++++++++++++++++++--- CHANGELOG.md | 2 + scripts/__tests__/workflows.test.ts | 91 ++++++++++++++++++++++++++++ 3 files changed, 170 insertions(+), 7 deletions(-) diff --git a/.github/workflows/update-members.yml b/.github/workflows/update-members.yml index 6d9165a..4f42046 100644 --- a/.github/workflows/update-members.yml +++ b/.github/workflows/update-members.yml @@ -34,6 +34,11 @@ on: options: - keep - remove + draft-run: + description: Only summarize the member update without creating a pull request + required: true + default: false + type: boolean defaults: run: @@ -42,18 +47,33 @@ defaults: jobs: update: permissions: - contents: write - pull-requests: write + contents: read + pull-requests: read name: Update members runs-on: ubuntu-latest - environment: push + environment: ${{ github.event.inputs['draft-run'] == 'true' && 'read' || 'push' }} env: GITHUB_APP_ID: ${{ secrets.RO_GITHUB_APP_ID }} GITHUB_APP_INSTALLATION_ID: ${{ secrets[format('RO_GITHUB_APP_INSTALLATION_ID_{0}', github.event.inputs.organization)] || secrets.RO_GITHUB_APP_INSTALLATION_ID }} GITHUB_APP_PEM_FILE: ${{ secrets.RO_GITHUB_APP_PEM_FILE }} TF_WORKSPACE: ${{ github.event.inputs.organization }} steps: + - name: Generate app token + if: github.event.inputs['draft-run'] != 'true' + id: token + uses: tibdex/github-app-token@3beb63f4bd073e61482598c45c71c1019b59b73a # v2.1.0 + with: + app_id: ${{ secrets.RW_GITHUB_APP_ID }} + installation_retrieval_mode: id + installation_retrieval_payload: ${{ secrets[format('RW_GITHUB_APP_INSTALLATION_ID_{0}', github.repository_owner)] || secrets.RW_GITHUB_APP_INSTALLATION_ID }} + private_key: ${{ secrets.RW_GITHUB_APP_PEM_FILE }} + - name: Checkout with app token + if: github.event.inputs['draft-run'] != 'true' + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + token: ${{ steps.token.outputs.token }} - name: Checkout + if: github.event.inputs['draft-run'] == 'true' uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - name: Install pnpm uses: pnpm/action-setup@91ab88e2619ed1f46221f0ba42d1492c02baf788 # v6.0.6 @@ -88,10 +108,60 @@ jobs: else echo "this=true" >> $GITHUB_OUTPUT fi - - uses: ./.github/actions/git-config-user - if: steps.config-modified.outputs.this == 'true' + - name: Summarize member update + env: + ORGANIZATION: ${{ github.event.inputs.organization }} + CUTOFF_DATE: ${{ github.event.inputs['cutoff-date'] }} + LIMIT: ${{ github.event.inputs.limit }} + IGNORE: ${{ github.event.inputs.ignore }} + ONLY: ${{ github.event.inputs.only }} + PUBLIC_REPO_ACCESS: ${{ github.event.inputs['public-repo-access'] }} + ORGANIZATION_MEMBERSHIP: ${{ github.event.inputs['organization-membership'] }} + DRAFT_RUN: ${{ github.event.inputs['draft-run'] }} + AFFECTED_USERS: ${{ steps.update.outputs.affected-users }} + CONFIG_MODIFIED: ${{ steps.config-modified.outputs.this }} + run: | + { + echo '## Update members' + echo + echo "- Organization: \`${ORGANIZATION}\`" + echo "- Cutoff date: \`${CUTOFF_DATE:-not set}\`" + echo "- Limit: \`${LIMIT:-not set}\`" + echo "- Ignore: \`${IGNORE:-not set}\`" + echo "- Only: \`${ONLY:-not set}\`" + echo "- Public repo access: \`${PUBLIC_REPO_ACCESS}\`" + echo "- Organization membership: \`${ORGANIZATION_MEMBERSHIP}\`" + echo "- Draft run: \`${DRAFT_RUN}\`" + echo "- Affected users: \`${AFFECTED_USERS:-none}\`" + echo "- Config modified: \`${CONFIG_MODIFIED}\`" + if [[ "${DRAFT_RUN}" == 'true' ]]; then + echo "- Pull request: not created because draft run is enabled" + elif [[ "${CONFIG_MODIFIED}" == 'true' ]]; then + echo "- Pull request: will be created" + else + echo "- Pull request: not created because there are no config changes" + fi + + if [[ "${CONFIG_MODIFIED}" == 'true' ]]; then + echo + echo '
Config diff' + echo + echo '```diff' + git diff -- "github/${ORGANIZATION}.yml" | sed 's/^```/``` /' + echo '```' + echo + echo '
' + fi + } >> "$GITHUB_STEP_SUMMARY" + - name: Configure git user + if: steps.config-modified.outputs.this == 'true' && github.event.inputs['draft-run'] != 'true' + env: + GITHUB_MGMT_APP_ID: ${{ secrets.RW_GITHUB_APP_ID }} + run: | + git config --global user.name "github-mgmt[bot]" + git config --global user.email "${GITHUB_MGMT_APP_ID}+github-mgmt[bot]@users.noreply.github.com" - name: Create draft pull request - if: steps.config-modified.outputs.this == 'true' + if: steps.config-modified.outputs.this == 'true' && github.event.inputs['draft-run'] != 'true' env: ORGANIZATION: ${{ github.event.inputs.organization }} CUTOFF_DATE: ${{ github.event.inputs['cutoff-date'] }} @@ -101,7 +171,7 @@ jobs: PUBLIC_REPO_ACCESS: ${{ github.event.inputs['public-repo-access'] }} ORGANIZATION_MEMBERSHIP: ${{ github.event.inputs['organization-membership'] }} AFFECTED_USERS: ${{ steps.update.outputs.affected-users }} - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GITHUB_TOKEN: ${{ steps.token.outputs.token }} run: | branch="update-members-${ORGANIZATION}-${GITHUB_RUN_ID}" body="$(mktemp)" diff --git a/CHANGELOG.md b/CHANGELOG.md index aaaabc8..b93a8c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,9 +26,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - plan/apply workflows now publish planned/applied commit, workspace, environment, and rendered terraform plan details to workflow summaries and artifacts - manual access report workflow for generating the full access breakdown on demand +- update members workflow draft run mode for summarizing member updates without creating a pull request - allow-destroy workspace classification now reports which member or repository removals require guarded environments - access report member classifications now explicitly describe the post-change access state - workflows: added separate GitHub Actions environments for reading organization state, writing organization state, and pushing repository changes +- update members workflow now creates branches and pull requests with the configured GitHub App token so follow-up workflows are triggered - **BREAKING**: access changes action now emits only the access change comment by default; update custom usage to avoid nesting the full access breakdown in PR comments - workflows: pin third-party actions to latest release SHAs and replan from the merged commit before applying - docs: update template repository references from `github-mgmt-template` to `github-as-code` diff --git a/scripts/__tests__/workflows.test.ts b/scripts/__tests__/workflows.test.ts index cbc28d2..1894acc 100644 --- a/scripts/__tests__/workflows.test.ts +++ b/scripts/__tests__/workflows.test.ts @@ -5,6 +5,8 @@ import * as YAML from 'yaml' type WorkflowStep = { name?: string + if?: string + uses?: string run?: string env?: Record with?: Record @@ -17,6 +19,7 @@ type Workflow = { jobs: Record< string, { + permissions?: Record environment?: string steps: WorkflowStep[] } @@ -196,4 +199,92 @@ describe('workflows', () => { 'diff -u "${TF_WORKSPACE}.reviewed.txt" "${TF_WORKSPACE}.merged.txt"\n' ) }) + + it('creates update-members pull requests with the GitHub App token', () => { + const updateMembers = workflow('update-members.yml') + const job = updateMembers.jobs.update + const steps = job.steps + const generateTokenStep = steps.find( + step => step.name === 'Generate app token' + ) + const checkoutStep = steps.find( + step => step.name === 'Checkout with app token' + ) + const configureGitStep = steps.find( + step => step.name === 'Configure git user' + ) + const createPullRequestStep = steps.find( + step => step.name === 'Create draft pull request' + ) + + assert.equal(job.permissions?.contents, 'read') + assert.equal(job.permissions?.['pull-requests'], 'read') + assert.ok(generateTokenStep) + assert.equal( + generateTokenStep.if, + "github.event.inputs['draft-run'] != 'true'" + ) + assert.equal( + generateTokenStep.with?.app_id, + '${{ secrets.RW_GITHUB_APP_ID }}' + ) + assert.ok(checkoutStep) + assert.equal(checkoutStep.if, "github.event.inputs['draft-run'] != 'true'") + assert.equal(checkoutStep.with?.token, '${{ steps.token.outputs.token }}') + assert.ok(configureGitStep) + assert.equal( + configureGitStep.if, + "steps.config-modified.outputs.this == 'true' && github.event.inputs['draft-run'] != 'true'" + ) + assert.equal( + configureGitStep.env?.GITHUB_MGMT_APP_ID, + '${{ secrets.RW_GITHUB_APP_ID }}' + ) + assert.match(configureGitStep.run ?? '', /github-mgmt\[bot\]/) + assert.ok(createPullRequestStep) + assert.equal( + createPullRequestStep.env?.GITHUB_TOKEN, + '${{ steps.token.outputs.token }}' + ) + }) + + it('supports update-members draft runs without creating pull requests', () => { + const updateMembers = workflow('update-members.yml') + const workflowDispatch = updateMembers.on.workflow_dispatch as { + inputs: Record + } + const job = updateMembers.jobs.update + const steps = job.steps + const checkoutStep = steps.find(step => step.name === 'Checkout') + const summaryStep = steps.find( + step => step.name === 'Summarize member update' + ) + const createPullRequestStep = steps.find( + step => step.name === 'Create draft pull request' + ) + + assert.equal(workflowDispatch.inputs['draft-run'].default, false) + assert.equal(workflowDispatch.inputs['draft-run'].type, 'boolean') + assert.equal( + job.environment, + "${{ github.event.inputs['draft-run'] == 'true' && 'read' || 'push' }}" + ) + assert.ok(checkoutStep) + assert.equal(checkoutStep.if, "github.event.inputs['draft-run'] == 'true'") + assert.ok(summaryStep) + assert.match(summaryStep.run ?? '', /## Update members/) + assert.match( + summaryStep.run ?? '', + /Pull request: not created because draft run is enabled/ + ) + assert.match( + summaryStep.run ?? '', + /git diff -- "github\/\$\{ORGANIZATION\}\.yml"/ + ) + assert.ok(createPullRequestStep) + assert.equal( + createPullRequestStep.if, + "steps.config-modified.outputs.this == 'true' && github.event.inputs['draft-run'] != 'true'" + ) + }) })