From c5868e5e3bae1028d479563effcf9328dd2a1371 Mon Sep 17 00:00:00 2001 From: SergeyMenshykh <68852919+SergeyMenshykh@users.noreply.github.com> Date: Tue, 29 Sep 2026 15:25:55 +0100 Subject: [PATCH] Restrict DevFlow PR reviews to internal contributors (#14503) ### Motivation and Context DevFlow currently starts PR reviews for all contributors. Align its review eligibility with Microsoft Agent Framework by checking membership in the internal developer team before running the review. ### Description - Split the workflow into `prepare_pr`, `team_check`, and `review` jobs. The review runs only when the team check succeeds. - Check the **PR author** for automatic runs and the **person starting the workflow** for manual runs. Draft PRs follow the same eligibility check as other PRs. - Add a team-membership helper that uses the team slug supplied by `DEVELOPER_TEAM`, plus a `team-check` GitHub App token profile with member and PR read permissions. - Keep SK's existing spam/relevance check and DevFlow review steps. - Add tests for the membership helper and token profile, and include the helper in the automation test workflow's path filters. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/actions/github-app-token/action.yml | 2 +- .../actions/github-app-token/create-token.js | 6 +- .github/scripts/check_team_membership.js | 46 ++++++ .github/tests/test_check_team_membership.js | 117 +++++++++++++++ .github/tests/test_create_github_app_token.js | 26 +++- .github/workflows/devflow-pr-review.yml | 134 +++++++++++++----- .github/workflows/github-automation-tests.yml | 2 + 7 files changed, 297 insertions(+), 36 deletions(-) create mode 100644 .github/scripts/check_team_membership.js create mode 100644 .github/tests/test_check_team_membership.js 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"