Skip to content

Commit c5868e5

Browse files
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>
1 parent bb601cc commit c5868e5

7 files changed

Lines changed: 296 additions & 35 deletions

File tree

‎.github/actions/github-app-token/action.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ inputs:
3131
description: Repository to include in the installation token
3232
required: false
3333
permission-profile:
34-
description: Permission profile (issues, pull-requests, or devflow)
34+
description: Permission profile (issues, pull-requests, devflow, or team-check)
3535
required: true
3636
fallback-token:
3737
description: PAT used temporarily when app authentication is unavailable

‎.github/actions/github-app-token/create-token.js‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,10 @@ const PERMISSION_PROFILES = Object.freeze({
1616
issues: 'write',
1717
pull_requests: 'write',
1818
}),
19+
'team-check': Object.freeze({
20+
members: 'read',
21+
pull_requests: 'read',
22+
}),
1923
});
2024

2125
function base64Url(value) {
@@ -71,7 +75,7 @@ async function createInstallationToken(config, dependencies = {}) {
7175
throw new Error('TARGET_REPOSITORY must use the owner/repository format.');
7276
}
7377
if (!Object.hasOwn(PERMISSION_PROFILES, config.permissionProfile)) {
74-
throw new Error('PERMISSION_PROFILE must be issues, pull-requests, or devflow.');
78+
throw new Error('PERMISSION_PROFILE must be issues, pull-requests, devflow, or team-check.');
7579
}
7680
const permissions = PERMISSION_PROFILES[config.permissionProfile];
7781

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
// Copyright (c) Microsoft. All rights reserved.
2+
3+
async function checkTeamMembership({ github, context, teamSlug, prNumber, username = '' }) {
4+
if (!teamSlug) {
5+
throw new Error('DEVELOPER_TEAM must be configured with the developer team slug.');
6+
}
7+
8+
const number = Number(prNumber);
9+
if (!/^[1-9][0-9]*$/.test(prNumber) || !Number.isSafeInteger(number)) {
10+
throw new Error('A valid PR number is required to check team membership.');
11+
}
12+
13+
let author = username.trim() || context.payload?.pull_request?.user?.login;
14+
if (!author) {
15+
const { data: pr } = await github.rest.pulls.get({
16+
...context.repo,
17+
pull_number: number,
18+
});
19+
author = pr.user?.login;
20+
}
21+
if (!author) {
22+
throw new Error('Could not determine PR author (user may be deleted).');
23+
}
24+
25+
// Verify the team exists so a missing or inaccessible team is not mistaken for a nonmember.
26+
await github.rest.teams.getByName({
27+
org: context.repo.owner,
28+
team_slug: teamSlug,
29+
});
30+
31+
try {
32+
const { data: membership } = await github.rest.teams.getMembershipForUserInOrg({
33+
org: context.repo.owner,
34+
team_slug: teamSlug,
35+
username: author,
36+
});
37+
return { author, isTeamMember: membership.state === 'active' };
38+
} catch (error) {
39+
if (error.status === 404) {
40+
return { author, isTeamMember: false };
41+
}
42+
throw error;
43+
}
44+
}
45+
46+
module.exports = checkTeamMembership;
Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
// Copyright (c) Microsoft. All rights reserved.
2+
3+
const { describe, it } = require('node:test');
4+
const assert = require('node:assert/strict');
5+
6+
const checkTeamMembership = require('../scripts/check_team_membership.js');
7+
8+
const context = { repo: { owner: 'microsoft', repo: 'semantic-kernel' } };
9+
10+
function githubWithMembership(membership, username = 'contributor') {
11+
return {
12+
rest: {
13+
pulls: {
14+
get: async (params) => {
15+
assert.deepEqual(params, { ...context.repo, pull_number: 42 });
16+
return { data: { user: { login: 'contributor' } } };
17+
},
18+
},
19+
teams: {
20+
getByName: async (params) => {
21+
assert.deepEqual(params, { org: 'microsoft', team_slug: 'developers' });
22+
},
23+
getMembershipForUserInOrg: async (params) => {
24+
assert.deepEqual(params, {
25+
org: 'microsoft',
26+
team_slug: 'developers',
27+
username,
28+
});
29+
if (membership instanceof Error) throw membership;
30+
return { data: { state: membership } };
31+
},
32+
},
33+
},
34+
};
35+
}
36+
37+
describe('Review requester team membership', () => {
38+
const options = { context, teamSlug: 'developers', prNumber: '42' };
39+
40+
it('uses the PR author from the event on automatic runs', async () => {
41+
const github = githubWithMembership('active');
42+
github.rest.pulls.get = async () => {
43+
throw new Error('Should not look up the PR when the event includes its author');
44+
};
45+
assert.deepEqual(
46+
await checkTeamMembership({
47+
...options,
48+
context: { ...context, payload: { pull_request: { user: { login: 'contributor' } } } },
49+
github,
50+
}),
51+
{ author: 'contributor', isTeamMember: true },
52+
);
53+
});
54+
55+
it('uses the initiator on manual runs, even if the PR author is external', async () => {
56+
const github = githubWithMembership('active', 'maintainer');
57+
github.rest.pulls.get = async () => {
58+
throw new Error('Should not look up the PR author on manual runs');
59+
};
60+
assert.deepEqual(
61+
await checkTeamMembership({ ...options, github, username: 'maintainer' }),
62+
{ author: 'maintainer', isTeamMember: true },
63+
);
64+
assert.deepEqual(
65+
await checkTeamMembership({
66+
...options,
67+
github: githubWithMembership(Object.assign(new Error('Not Found'), { status: 404 }), 'outsider'),
68+
username: 'outsider',
69+
}),
70+
{ author: 'outsider', isTeamMember: false },
71+
);
72+
});
73+
74+
it('accepts an active team member', async () => {
75+
assert.deepEqual(
76+
await checkTeamMembership({ ...options, github: githubWithMembership('active') }),
77+
{ author: 'contributor', isTeamMember: true },
78+
);
79+
});
80+
81+
it('rejects a pending team member and an outsider', async () => {
82+
for (const membership of ['pending', Object.assign(new Error('Not Found'), { status: 404 })]) {
83+
assert.deepEqual(
84+
await checkTeamMembership({ ...options, github: githubWithMembership(membership) }),
85+
{ author: 'contributor', isTeamMember: false },
86+
);
87+
}
88+
});
89+
90+
it('does not treat API errors or a missing team as nonmembership', async () => {
91+
await assert.rejects(
92+
checkTeamMembership({
93+
...options,
94+
github: githubWithMembership(Object.assign(new Error('Forbidden'), { status: 403 })),
95+
}),
96+
/Forbidden/,
97+
);
98+
const github = githubWithMembership('active');
99+
github.rest.teams.getByName = async () => {
100+
throw Object.assign(new Error('Team not found'), { status: 404 });
101+
};
102+
await assert.rejects(checkTeamMembership({ ...options, github }), /Team not found/);
103+
});
104+
105+
it('rejects missing configuration and invalid PR numbers', async () => {
106+
await assert.rejects(
107+
checkTeamMembership({ ...options, github: githubWithMembership('active'), teamSlug: '' }),
108+
/DEVELOPER_TEAM/,
109+
);
110+
for (const prNumber of ['0', '1e2', '9007199254740992']) {
111+
await assert.rejects(
112+
checkTeamMembership({ ...options, github: githubWithMembership('active'), prNumber }),
113+
/valid PR number/,
114+
);
115+
}
116+
});
117+
});

‎.github/tests/test_create_github_app_token.js‎

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,28 @@ describe('GitHub App token creation', () => {
109109
});
110110
});
111111

112+
it('limits team-check tokens to member and pull-request reads', async () => {
113+
let request;
114+
await createInstallationToken(
115+
{ ...CONFIG, permissionProfile: 'team-check' },
116+
{
117+
execute: () => '+/8=\n',
118+
fetch: async (_url, options) => {
119+
request = options;
120+
return {
121+
ok: true,
122+
json: async () => ({ token: 'installation-token' }),
123+
};
124+
},
125+
},
126+
);
127+
128+
assert.deepEqual(JSON.parse(request.body).permissions, {
129+
members: 'read',
130+
pull_requests: 'read',
131+
});
132+
});
133+
112134
it('rejects unknown permission profiles before signing', async () => {
113135
let signed = false;
114136

@@ -122,7 +144,7 @@ describe('GitHub App token creation', () => {
122144
},
123145
},
124146
),
125-
/PERMISSION_PROFILE must be issues, pull-requests, or devflow/,
147+
/PERMISSION_PROFILE must be issues, pull-requests, devflow, or team-check/,
126148
);
127149
assert.equal(signed, false);
128150
});
@@ -140,7 +162,7 @@ describe('GitHub App token creation', () => {
140162
},
141163
},
142164
),
143-
/PERMISSION_PROFILE must be issues, pull-requests, or devflow/,
165+
/PERMISSION_PROFILE must be issues, pull-requests, devflow, or team-check/,
144166
);
145167
assert.equal(signed, false);
146168
});

0 commit comments

Comments
 (0)