diff --git a/.github/actions/github-app-token/action.yml b/.github/actions/github-app-token/action.yml index 1447b4ea43..04ab15c50a 100644 --- a/.github/actions/github-app-token/action.yml +++ b/.github/actions/github-app-token/action.yml @@ -31,7 +31,7 @@ inputs: description: Repository to include in the installation token required: false permission-profile: - description: Permission profile (issues, pull-requests, or devflow) + description: Permission profile (issues, pull-requests, devflow, or team-check) required: true fallback-token: description: PAT used temporarily when app authentication is unavailable diff --git a/.github/actions/github-app-token/create-token.js b/.github/actions/github-app-token/create-token.js index 1caedf70a8..9fd23addd9 100644 --- a/.github/actions/github-app-token/create-token.js +++ b/.github/actions/github-app-token/create-token.js @@ -16,6 +16,10 @@ const PERMISSION_PROFILES = Object.freeze({ issues: 'write', pull_requests: 'write', }), + 'team-check': Object.freeze({ + members: 'read', + pull_requests: 'read', + }), }); function base64Url(value) { @@ -71,7 +75,7 @@ async function createInstallationToken(config, dependencies = {}) { throw new Error('TARGET_REPOSITORY must use the owner/repository format.'); } if (!Object.hasOwn(PERMISSION_PROFILES, config.permissionProfile)) { - throw new Error('PERMISSION_PROFILE must be issues, pull-requests, or devflow.'); + throw new Error('PERMISSION_PROFILE must be issues, pull-requests, devflow, or team-check.'); } const permissions = PERMISSION_PROFILES[config.permissionProfile]; diff --git a/.github/scripts/check_team_membership.js b/.github/scripts/check_team_membership.js new file mode 100644 index 0000000000..e64fbaf852 --- /dev/null +++ b/.github/scripts/check_team_membership.js @@ -0,0 +1,46 @@ +// Copyright (c) Microsoft. All rights reserved. + +async function checkTeamMembership({ github, context, teamSlug, prNumber, username = '' }) { + if (!teamSlug) { + throw new Error('DEVELOPER_TEAM must be configured with the developer team slug.'); + } + + const number = Number(prNumber); + if (!/^[1-9][0-9]*$/.test(prNumber) || !Number.isSafeInteger(number)) { + throw new Error('A valid PR number is required to check team membership.'); + } + + let author = username.trim() || context.payload?.pull_request?.user?.login; + if (!author) { + const { data: pr } = await github.rest.pulls.get({ + ...context.repo, + pull_number: number, + }); + author = pr.user?.login; + } + if (!author) { + throw new Error('Could not determine PR author (user may be deleted).'); + } + + // Verify the team exists so a missing or inaccessible team is not mistaken for a nonmember. + await github.rest.teams.getByName({ + org: context.repo.owner, + team_slug: teamSlug, + }); + + try { + const { data: membership } = await github.rest.teams.getMembershipForUserInOrg({ + org: context.repo.owner, + team_slug: teamSlug, + username: author, + }); + return { author, isTeamMember: membership.state === 'active' }; + } catch (error) { + if (error.status === 404) { + return { author, isTeamMember: false }; + } + throw error; + } +} + +module.exports = checkTeamMembership; diff --git a/.github/tests/test_check_team_membership.js b/.github/tests/test_check_team_membership.js new file mode 100644 index 0000000000..6b6863c200 --- /dev/null +++ b/.github/tests/test_check_team_membership.js @@ -0,0 +1,117 @@ +// Copyright (c) Microsoft. All rights reserved. + +const { describe, it } = require('node:test'); +const assert = require('node:assert/strict'); + +const checkTeamMembership = require('../scripts/check_team_membership.js'); + +const context = { repo: { owner: 'microsoft', repo: 'semantic-kernel' } }; + +function githubWithMembership(membership, username = 'contributor') { + return { + rest: { + pulls: { + get: async (params) => { + assert.deepEqual(params, { ...context.repo, pull_number: 42 }); + return { data: { user: { login: 'contributor' } } }; + }, + }, + teams: { + getByName: async (params) => { + assert.deepEqual(params, { org: 'microsoft', team_slug: 'developers' }); + }, + getMembershipForUserInOrg: async (params) => { + assert.deepEqual(params, { + org: 'microsoft', + team_slug: 'developers', + username, + }); + if (membership instanceof Error) throw membership; + return { data: { state: membership } }; + }, + }, + }, + }; +} + +describe('Review requester team membership', () => { + const options = { context, teamSlug: 'developers', prNumber: '42' }; + + it('uses the PR author from the event on automatic runs', async () => { + const github = githubWithMembership('active'); + github.rest.pulls.get = async () => { + throw new Error('Should not look up the PR when the event includes its author'); + }; + assert.deepEqual( + await checkTeamMembership({ + ...options, + context: { ...context, payload: { pull_request: { user: { login: 'contributor' } } } }, + github, + }), + { author: 'contributor', isTeamMember: true }, + ); + }); + + it('uses the initiator on manual runs, even if the PR author is external', async () => { + const github = githubWithMembership('active', 'maintainer'); + github.rest.pulls.get = async () => { + throw new Error('Should not look up the PR author on manual runs'); + }; + assert.deepEqual( + await checkTeamMembership({ ...options, github, username: 'maintainer' }), + { author: 'maintainer', isTeamMember: true }, + ); + assert.deepEqual( + await checkTeamMembership({ + ...options, + github: githubWithMembership(Object.assign(new Error('Not Found'), { status: 404 }), 'outsider'), + username: 'outsider', + }), + { author: 'outsider', isTeamMember: false }, + ); + }); + + it('accepts an active team member', async () => { + assert.deepEqual( + await checkTeamMembership({ ...options, github: githubWithMembership('active') }), + { author: 'contributor', isTeamMember: true }, + ); + }); + + it('rejects a pending team member and an outsider', async () => { + for (const membership of ['pending', Object.assign(new Error('Not Found'), { status: 404 })]) { + assert.deepEqual( + await checkTeamMembership({ ...options, github: githubWithMembership(membership) }), + { author: 'contributor', isTeamMember: false }, + ); + } + }); + + it('does not treat API errors or a missing team as nonmembership', async () => { + await assert.rejects( + checkTeamMembership({ + ...options, + github: githubWithMembership(Object.assign(new Error('Forbidden'), { status: 403 })), + }), + /Forbidden/, + ); + const github = githubWithMembership('active'); + github.rest.teams.getByName = async () => { + throw Object.assign(new Error('Team not found'), { status: 404 }); + }; + await assert.rejects(checkTeamMembership({ ...options, github }), /Team not found/); + }); + + it('rejects missing configuration and invalid PR numbers', async () => { + await assert.rejects( + checkTeamMembership({ ...options, github: githubWithMembership('active'), teamSlug: '' }), + /DEVELOPER_TEAM/, + ); + for (const prNumber of ['0', '1e2', '9007199254740992']) { + await assert.rejects( + checkTeamMembership({ ...options, github: githubWithMembership('active'), prNumber }), + /valid PR number/, + ); + } + }); +}); diff --git a/.github/tests/test_create_github_app_token.js b/.github/tests/test_create_github_app_token.js index 04cf8099b6..7508dcdc69 100644 --- a/.github/tests/test_create_github_app_token.js +++ b/.github/tests/test_create_github_app_token.js @@ -109,6 +109,28 @@ describe('GitHub App token creation', () => { }); }); + it('limits team-check tokens to member and pull-request reads', async () => { + let request; + await createInstallationToken( + { ...CONFIG, permissionProfile: 'team-check' }, + { + execute: () => '+/8=\n', + fetch: async (_url, options) => { + request = options; + return { + ok: true, + json: async () => ({ token: 'installation-token' }), + }; + }, + }, + ); + + assert.deepEqual(JSON.parse(request.body).permissions, { + members: 'read', + pull_requests: 'read', + }); + }); + it('rejects unknown permission profiles before signing', async () => { let signed = false; @@ -122,7 +144,7 @@ describe('GitHub App token creation', () => { }, }, ), - /PERMISSION_PROFILE must be issues, pull-requests, or devflow/, + /PERMISSION_PROFILE must be issues, pull-requests, devflow, or team-check/, ); assert.equal(signed, false); }); @@ -140,7 +162,7 @@ describe('GitHub App token creation', () => { }, }, ), - /PERMISSION_PROFILE must be issues, pull-requests, or devflow/, + /PERMISSION_PROFILE must be issues, pull-requests, devflow, or team-check/, ); assert.equal(signed, false); }); diff --git a/.github/workflows/devflow-pr-review.yml b/.github/workflows/devflow-pr-review.yml index 59be07ae43..8458cfb6a3 100644 --- a/.github/workflows/devflow-pr-review.yml +++ b/.github/workflows/devflow-pr-review.yml @@ -30,37 +30,14 @@ env: MODEL_CONFIG_PATH: ${{ github.workspace }}/devflow/config.ci.yaml jobs: - review: + prepare_pr: runs-on: ubuntu-latest - environment: github-app-auth - timeout-minutes: 60 - if: ${{ github.event_name != 'pull_request_target' || !github.event.pull_request.draft }} - + permissions: {} + outputs: + pr_number: ${{ steps.pr.outputs.pr_number }} + pr_url: ${{ steps.pr.outputs.pr_url }} + repo: ${{ steps.pr.outputs.repo }} steps: - - name: Checkout GitHub automation - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - with: - ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.sha || github.sha }} - sparse-checkout: .github/actions/github-app-token - fetch-depth: 1 - persist-credentials: false - - - name: Get GitHub automation token - id: github-auth - uses: ./.github/actions/github-app-token - with: - mode: ${{ vars.GH_APP_AUTH_MODE }} - azure-client-id: ${{ secrets.GH_APP_AZURE_CLIENT_ID }} - azure-tenant-id: ${{ secrets.GH_APP_AZURE_TENANT_ID }} - azure-subscription-id: ${{ secrets.GH_APP_AZURE_SUBSCRIPTION_ID }} - key-vault-name: ${{ secrets.GH_APP_KEY_VAULT_NAME }} - key-name: ${{ secrets.GH_APP_KEY_NAME }} - github-app-client-id: ${{ secrets.GH_APP_CLIENT_ID }} - github-app-installation-id: ${{ secrets.GH_APP_INSTALLATION_ID }} - repository: ${{ github.repository }} - permission-profile: devflow - fallback-token: ${{ secrets.GH_ACTIONS_PR_WRITE }} - - name: Resolve PR metadata id: pr shell: bash @@ -88,6 +65,99 @@ jobs: echo "pr_number=${pr_number}" >> "$GITHUB_OUTPUT" echo "repo=${GITHUB_REPOSITORY}" >> "$GITHUB_OUTPUT" + team_check: + needs: prepare_pr + runs-on: ubuntu-latest + permissions: + contents: read + id-token: write + environment: github-app-auth + outputs: + is_team_member: ${{ steps.check.outputs.is_team_member }} + steps: + - name: Checkout GitHub automation + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.sha || github.sha }} + sparse-checkout: | + .github/actions/github-app-token + .github/scripts/check_team_membership.js + fetch-depth: 1 + persist-credentials: false + + - name: Get GitHub automation token + id: github-auth + uses: ./.github/actions/github-app-token + with: + mode: ${{ vars.GH_APP_AUTH_MODE }} + azure-client-id: ${{ secrets.GH_APP_AZURE_CLIENT_ID }} + azure-tenant-id: ${{ secrets.GH_APP_AZURE_TENANT_ID }} + azure-subscription-id: ${{ secrets.GH_APP_AZURE_SUBSCRIPTION_ID }} + key-vault-name: ${{ secrets.GH_APP_KEY_VAULT_NAME }} + key-name: ${{ secrets.GH_APP_KEY_NAME }} + github-app-client-id: ${{ secrets.GH_APP_CLIENT_ID }} + github-app-installation-id: ${{ secrets.GH_APP_INSTALLATION_ID }} + repository: ${{ github.repository }} + permission-profile: team-check + fallback-token: ${{ secrets.GH_ACTIONS_PR_WRITE }} + + - name: Check review requester team membership + id: check + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + env: + MEMBERSHIP_USER: ${{ github.event_name == 'workflow_dispatch' && github.actor || '' }} + TEAM_NAME: ${{ secrets.DEVELOPER_TEAM }} + PR_NUMBER: ${{ needs.prepare_pr.outputs.pr_number }} + with: + github-token: ${{ steps.github-auth.outputs.token }} + script: | + const checkTeamMembership = require('./.github/scripts/check_team_membership.js'); + const { author, isTeamMember } = await checkTeamMembership({ + github, + context, + teamSlug: process.env.TEAM_NAME, + prNumber: process.env.PR_NUMBER, + username: process.env.MEMBERSHIP_USER, + }); + core.setOutput('is_team_member', isTeamMember ? 'true' : 'false'); + if (isTeamMember) { + core.info(`User ${author} is a team member; proceeding with review.`); + } else { + core.info(`User ${author} is not a member of ${process.env.TEAM_NAME}; skipping review.`); + } + + review: + runs-on: ubuntu-latest + needs: [prepare_pr, team_check] + if: ${{ needs.team_check.outputs.is_team_member == 'true' }} + environment: github-app-auth + timeout-minutes: 60 + + steps: + - name: Checkout GitHub automation + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.sha || github.sha }} + sparse-checkout: .github/actions/github-app-token + fetch-depth: 1 + persist-credentials: false + + - name: Get GitHub automation token + id: github-auth + uses: ./.github/actions/github-app-token + with: + mode: ${{ vars.GH_APP_AUTH_MODE }} + azure-client-id: ${{ secrets.GH_APP_AZURE_CLIENT_ID }} + azure-tenant-id: ${{ secrets.GH_APP_AZURE_TENANT_ID }} + azure-subscription-id: ${{ secrets.GH_APP_AZURE_SUBSCRIPTION_ID }} + key-vault-name: ${{ secrets.GH_APP_KEY_VAULT_NAME }} + key-name: ${{ secrets.GH_APP_KEY_NAME }} + github-app-client-id: ${{ secrets.GH_APP_CLIENT_ID }} + github-app-installation-id: ${{ secrets.GH_APP_INSTALLATION_ID }} + repository: ${{ github.repository }} + permission-profile: devflow + fallback-token: ${{ secrets.GH_ACTIONS_PR_WRITE }} + # Safe checkout: base repo only, not the untrusted PR head. - name: Checkout target repo base uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -131,8 +201,8 @@ jobs: GITHUB_TOKEN: ${{ github.token }} SK_REPO_PATH: ${{ env.TARGET_REPO_PATH }} AGENT_REPO_PATH: ${{ env.TARGET_REPO_PATH }} - PR_REPO: ${{ steps.pr.outputs.repo }} - PR_NUMBER: ${{ steps.pr.outputs.pr_number }} + PR_REPO: ${{ needs.prepare_pr.outputs.repo }} + PR_NUMBER: ${{ needs.prepare_pr.outputs.pr_number }} run: | uv run python scripts/classify_pr_spam.py \ --repo "$PR_REPO" \ @@ -158,7 +228,7 @@ jobs: GITHUB_TOKEN: ${{ github.token }} SK_REPO_PATH: ${{ env.TARGET_REPO_PATH }} AGENT_REPO_PATH: ${{ env.TARGET_REPO_PATH }} - PR_URL: ${{ steps.pr.outputs.pr_url }} + PR_URL: ${{ needs.prepare_pr.outputs.pr_url }} run: | uv run python scripts/trigger_pr_review.py \ --pr-url "$PR_URL" \ diff --git a/.github/workflows/github-automation-tests.yml b/.github/workflows/github-automation-tests.yml index d96f9180ff..a7b3e003f3 100644 --- a/.github/workflows/github-automation-tests.yml +++ b/.github/workflows/github-automation-tests.yml @@ -4,6 +4,7 @@ on: pull_request: paths: - ".github/actions/**" + - ".github/scripts/check_team_membership.js" - ".github/tests/**" - ".github/workflows/devflow-pr-review.yml" - ".github/workflows/github-automation-tests.yml" @@ -14,6 +15,7 @@ on: - main paths: - ".github/actions/**" + - ".github/scripts/check_team_membership.js" - ".github/tests/**" - ".github/workflows/devflow-pr-review.yml" - ".github/workflows/github-automation-tests.yml"