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 2598e6a..a4828c5 100644 --- a/scripts/src/actions/classify-allow-destroy.ts +++ b/scripts/src/actions/classify-allow-destroy.ts @@ -1,4 +1,5 @@ import 'reflect-metadata' + import * as core from '@actions/core' import {pathToFileURL} from 'url' import {Config} from '../yaml/config.js' @@ -9,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, @@ -59,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 } @@ -80,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/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' }