From 1107316235ea457b566570a3a1ffaa49de4f4ecf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Madis=20K=C3=B5osaar?= Date: Thu, 12 Feb 2026 10:37:24 +0200 Subject: [PATCH 1/4] fix: apply org-level settings before loading repository configurations --- lib/plugins/branches.js | 20 ++++++++++++++------ lib/settings.js | 3 +++ 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/lib/plugins/branches.js b/lib/plugins/branches.js index d28e2f905..28bb09cc9 100644 --- a/lib/plugins/branches.js +++ b/lib/plugins/branches.js @@ -5,10 +5,18 @@ const Overrides = require('./overrides') const ignorableFields = [] const previewHeaders = { accept: 'application/vnd.github.hellcat-preview+json,application/vnd.github.luke-cage-preview+json,application/vnd.github.zzzax-preview+json' } const overrides = { - 'contexts': { - 'action': 'reset', - 'type': 'array' - }, + contexts: { + action: 'reset', + type: 'array' + } +} + +// GitHub API requires these fields to be present in updateBranchProtection calls +// See: https://docs.github.com/rest/branches/branch-protection#update-branch-protection +const requiredBranchProtectionDefaults = { + required_status_checks: null, + enforce_admins: null, + restrictions: null } module.exports = class Branches extends ErrorStash { @@ -73,7 +81,7 @@ module.exports = class Branches extends ErrorStash { resArray.push(new NopCommand(this.constructor.name, this.repo, null, results)) } - Object.assign(params, branch.protection, { headers: previewHeaders }) + Object.assign(params, requiredBranchProtectionDefaults, branch.protection, { headers: previewHeaders }) if (this.nop) { resArray.push(new NopCommand(this.constructor.name, this.repo, this.github.repos.updateBranchProtection.endpoint(params), 'Add Branch Protection')) @@ -83,7 +91,7 @@ module.exports = class Branches extends ErrorStash { return this.github.repos.updateBranchProtection(params).then(res => this.log.debug(`Branch protection applied successfully ${JSON.stringify(res.url)}`)).catch(e => { this.logError(`Error applying branch protection ${JSON.stringify(e)}`); return [] }) }).catch((e) => { if (e.status === 404) { - Object.assign(params, Overrides.removeOverrides(overrides, branch.protection, {}), { headers: previewHeaders }) + Object.assign(params, requiredBranchProtectionDefaults, Overrides.removeOverrides(overrides, branch.protection, {}), { headers: previewHeaders }) if (this.nop) { resArray.push(new NopCommand(this.constructor.name, this.repo, this.github.repos.updateBranchProtection.endpoint(params), 'Add Branch Protection')) return Promise.resolve(resArray) diff --git a/lib/settings.js b/lib/settings.js index 8d9e07b2b..9a6d37bf1 100644 --- a/lib/settings.js +++ b/lib/settings.js @@ -46,6 +46,9 @@ class Settings { const settings = new Settings(nop, context, context.repo(), config, ref) try { + // Apply org-level settings (e.g., rulesets) first, matching syncAll behavior + await settings.updateOrg() + for (const repo of repos) { settings.repo = repo await settings.loadConfigs(repo) From 0051d0e26024567f1d081d060fd2529e1b7ce30c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Madis=20K=C3=B5osaar?= Date: Thu, 12 Feb 2026 10:37:50 +0200 Subject: [PATCH 2/4] fix: enhance descriptions and add new properties for security features in settings.json --- schema/dereferenced/settings.json | 229 +++++++++++++++++------------- 1 file changed, 132 insertions(+), 97 deletions(-) diff --git a/schema/dereferenced/settings.json b/schema/dereferenced/settings.json index e94a66e57..fc9a45c3e 100644 --- a/schema/dereferenced/settings.json +++ b/schema/dereferenced/settings.json @@ -39,7 +39,17 @@ "properties": { "advanced_security": { "type": "object", - "description": "Use the `status` property to enable or disable GitHub Advanced Security for this repository. For more information, see \"[About GitHub Advanced Security](/github/getting-started-with-github/learning-about-github/about-github-advanced-security).\"", + "description": "Use the `status` property to enable or disable GitHub Advanced Security for this repository.\nFor more information, see \"[About GitHub Advanced\nSecurity](/github/getting-started-with-github/learning-about-github/about-github-advanced-security).\"\n\nFor standalone Code Scanning or Secret Protection products, this parameter cannot be used.", + "properties": { + "status": { + "type": "string", + "description": "Can be `enabled` or `disabled`." + } + } + }, + "code_security": { + "type": "object", + "description": "Use the `status` property to enable or disable GitHub Code Security for this repository.", "properties": { "status": { "type": "string", @@ -67,6 +77,16 @@ } } }, + "secret_scanning_ai_detection": { + "type": "object", + "description": "Use the `status` property to enable or disable secret scanning AI detection for this repository. For more information, see \"[Responsible detection of generic secrets with AI](https://docs.github.com/code-security/secret-scanning/using-advanced-secret-scanning-and-push-protection-features/generic-secret-detection/responsible-ai-generic-secrets).\"", + "properties": { + "status": { + "type": "string", + "description": "Can be `enabled` or `disabled`." + } + } + }, "secret_scanning_non_provider_patterns": { "type": "object", "description": "Use the `status` property to enable or disable secret scanning non-provider patterns for this repository. For more information, see \"[Supported secret scanning patterns](/code-security/secret-scanning/introduction/supported-secret-scanning-patterns#supported-secrets).\"", @@ -135,7 +155,7 @@ }, "use_squash_pr_title_as_default": { "type": "boolean", - "description": "Either `true` to allow squash-merge commits to use pull request title, or `false` to use commit message. **This property has been deprecated. Please use `squash_merge_commit_title` instead.", + "description": "Either `true` to allow squash-merge commits to use pull request title, or `false` to use commit message. **This property is closing down. Please use `squash_merge_commit_title` instead.", "default": false, "deprecated": true }, @@ -338,7 +358,7 @@ }, "maintainers": { "type": "array", - "description": "List GitHub IDs for organization members who will become team maintainers.", + "description": "List GitHub usernames for organization members who will become team maintainers.", "items": { "type": "string" } @@ -368,7 +388,7 @@ }, "permission": { "type": "string", - "description": "**Deprecated**. The permission that new repositories will be added to the team with when none is specified.", + "description": "**Closing down notice**. The permission that new repositories will be added to the team with when none is specified.", "enum": [ "pull", "push" @@ -409,7 +429,7 @@ "contexts": { "type": "array", "deprecated": true, - "description": "**Deprecated**: The list of status checks to require in order to merge into this branch. If any of these checks have recently been set by a particular GitHub App, they will be required to come from that app in future for the branch to merge. Use `checks` instead of `contexts` for more fine-grained control.", + "description": "**Closing down notice**: The list of status checks to require in order to merge into this branch. If any of these checks have recently been set by a particular GitHub App, they will be required to come from that app in future for the branch to merge. Use `checks` instead of `contexts` for more fine-grained control.", "items": { "type": "string" } @@ -663,7 +683,8 @@ "enum": [ "branch", "tag", - "push" + "push", + "repository" ], "default": "branch" }, @@ -690,7 +711,7 @@ "actor_id": { "type": "integer", "nullable": true, - "description": "The ID of the actor that can bypass a ruleset. If `actor_type` is `OrganizationAdmin`, this should be `1`. If `actor_type` is `DeployKey`, this should be null. `OrganizationAdmin` is not applicable for personal repositories." + "description": "The ID of the actor that can bypass a ruleset. Required for `Integration`, `RepositoryRole`, and `Team` actor types. If `actor_type` is `OrganizationAdmin`, `actor_id` is ignored. If `actor_type` is `DeployKey`, this should be null. `OrganizationAdmin` is not applicable for personal repositories." }, "actor_type": { "type": "string", @@ -705,10 +726,11 @@ }, "bypass_mode": { "type": "string", - "description": "When the specified actor can bypass the ruleset. `pull_request` means that an actor can only bypass rules on pull requests. `pull_request` is not applicable for the `DeployKey` actor type. Also, `pull_request` is only applicable to branch rulesets.", + "description": "When the specified actor can bypass the ruleset. `pull_request` means that an actor can only bypass rules on pull requests. `pull_request` is not applicable for the `DeployKey` actor type. Also, `pull_request` is only applicable to branch rulesets. When `bypass_mode` is `exempt`, rules will not be run for that actor and a bypass audit entry will not be created.", "enum": [ "always", - "pull_request" + "pull_request", + "exempt" ], "default": "always" } @@ -718,7 +740,7 @@ "conditions": { "title": "Organization ruleset conditions", "type": "object", - "description": "Conditions for an organization ruleset.\nThe branch and tag rulesets conditions object should contain both `repository_name` and `ref_name` properties, or both `repository_id` and `ref_name` properties, or both `repository_property` and `ref_name` properties.\nThe push rulesets conditions object does not require the `ref_name` property.", + "description": "Conditions for an organization ruleset.\nThe branch and tag rulesets conditions object should contain both `repository_name` and `ref_name` properties, or both `repository_id` and `ref_name` properties, or both `repository_property` and `ref_name` properties.\nThe push rulesets conditions object does not require the `ref_name` property.\nFor repository policy rulesets, the conditions object should only contain the `repository_name`, the `repository_id`, or the `repository_property`.", "oneOf": [ { "type": "object", @@ -1043,83 +1065,6 @@ } } }, - { - "title": "merge_queue", - "description": "Merges must be performed via a merge queue.", - "type": "object", - "required": [ - "type" - ], - "properties": { - "type": { - "type": "string", - "enum": [ - "merge_queue" - ] - }, - "parameters": { - "type": "object", - "properties": { - "check_response_timeout_minutes": { - "type": "integer", - "description": "Maximum time for a required status check to report a conclusion. After this much time has elapsed, checks that have not reported a conclusion will be assumed to have failed", - "minimum": 1, - "maximum": 360 - }, - "grouping_strategy": { - "type": "string", - "description": "When set to ALLGREEN, the merge commit created by merge queue for each PR in the group must pass all required checks to merge. When set to HEADGREEN, only the commit at the head of the merge group, i.e. the commit containing changes from all of the PRs in the group, must pass its required checks to merge.", - "enum": [ - "ALLGREEN", - "HEADGREEN" - ] - }, - "max_entries_to_build": { - "type": "integer", - "description": "Limit the number of queued pull requests requesting checks and workflow runs at the same time.", - "minimum": 0, - "maximum": 100 - }, - "max_entries_to_merge": { - "type": "integer", - "description": "The maximum number of PRs that will be merged together in a group.", - "minimum": 0, - "maximum": 100 - }, - "merge_method": { - "type": "string", - "description": "Method to use when merging changes from queued pull requests.", - "enum": [ - "MERGE", - "SQUASH", - "REBASE" - ] - }, - "min_entries_to_merge": { - "type": "integer", - "description": "The minimum number of PRs that will be merged together in a group.", - "minimum": 0, - "maximum": 100 - }, - "min_entries_to_merge_wait_minutes": { - "type": "integer", - "description": "The time merge queue should wait after the first PR is added to the queue for the minimum group size to be met. After this time has elapsed, the minimum group size will be ignored and a smaller group will be merged.", - "minimum": 0, - "maximum": 360 - } - }, - "required": [ - "check_response_timeout_minutes", - "grouping_strategy", - "max_entries_to_build", - "max_entries_to_merge", - "merge_method", - "min_entries_to_merge", - "min_entries_to_merge_wait_minutes" - ] - } - } - }, { "title": "required_deployments", "description": "Choose which environments must be successfully deployed to before refs can be pushed into a ref that matches this rule.", @@ -1184,6 +1129,18 @@ "parameters": { "type": "object", "properties": { + "allowed_merge_methods": { + "type": "array", + "description": "Array of allowed merge methods. Allowed values include `merge`, `squash`, and `rebase`. At least one option must be enabled.", + "items": { + "type": "string", + "enum": [ + "merge", + "squash", + "rebase" + ] + } + }, "dismiss_stale_reviews_on_push": { "type": "boolean", "description": "New, reviewable commits pushed will dismiss previous pull request review approvals." @@ -1205,6 +1162,55 @@ "required_review_thread_resolution": { "type": "boolean", "description": "All conversations on code must be resolved before a pull request can be merged." + }, + "required_reviewers": { + "type": "array", + "description": "> [!NOTE]\n> `required_reviewers` is in beta and subject to change.\n\nA collection of reviewers and associated file patterns. Each reviewer has a list of file patterns which determine the files that reviewer is required to review.", + "items": { + "title": "RequiredReviewerConfiguration", + "description": "A reviewing team, and file patterns describing which files they must approve changes to.", + "type": "object", + "properties": { + "file_patterns": { + "type": "array", + "description": "Array of file patterns. Pull requests which change matching files must be approved by the specified team. File patterns use fnmatch syntax.", + "items": { + "type": "string" + } + }, + "minimum_approvals": { + "type": "integer", + "description": "Minimum number of approvals required from the specified team. If set to zero, the team will be added to the pull request but approval is optional." + }, + "reviewer": { + "title": "Reviewer", + "description": "A required reviewing team", + "type": "object", + "properties": { + "id": { + "type": "integer", + "description": "ID of the reviewer which must review changes to matching files." + }, + "type": { + "type": "string", + "description": "The type of the reviewer", + "enum": [ + "Team" + ] + } + }, + "required": [ + "id", + "type" + ] + } + }, + "required": [ + "file_patterns", + "minimum_approvals", + "reviewer" + ] + } } }, "required": [ @@ -1307,7 +1313,7 @@ "properties": { "name": { "type": "string", - "description": "How this rule will appear to users." + "description": "How this rule appears when configuring it." }, "negate": { "type": "boolean", @@ -1354,7 +1360,7 @@ "properties": { "name": { "type": "string", - "description": "How this rule will appear to users." + "description": "How this rule appears when configuring it." }, "negate": { "type": "boolean", @@ -1401,7 +1407,7 @@ "properties": { "name": { "type": "string", - "description": "How this rule will appear to users." + "description": "How this rule appears when configuring it." }, "negate": { "type": "boolean", @@ -1448,7 +1454,7 @@ "properties": { "name": { "type": "string", - "description": "How this rule will appear to users." + "description": "How this rule appears when configuring it." }, "negate": { "type": "boolean", @@ -1495,7 +1501,7 @@ "properties": { "name": { "type": "string", - "description": "How this rule will appear to users." + "description": "How this rule appears when configuring it." }, "negate": { "type": "boolean", @@ -1525,7 +1531,7 @@ }, { "title": "file_path_restriction", - "description": "Prevent commits that include changes in specified file paths from being pushed to the commit graph.", + "description": "Prevent commits that include changes in specified file and folder paths from being pushed to the commit graph. This includes absolute paths that contain file names.", "type": "object", "required": [ "type" @@ -1556,7 +1562,7 @@ }, { "title": "max_file_path_length", - "description": "Prevent commits that include file paths that exceed a specified character limit from being pushed to the commit graph.", + "description": "Prevent commits that include file paths that exceed the specified character limit from being pushed to the commit graph.", "type": "object", "required": [ "type" @@ -1573,9 +1579,9 @@ "properties": { "max_file_path_length": { "type": "integer", - "description": "The maximum amount of characters allowed in file paths", + "description": "The maximum amount of characters allowed in file paths.", "minimum": 1, - "maximum": 256 + "maximum": 32767 } }, "required": [ @@ -1617,7 +1623,7 @@ }, { "title": "max_file_size", - "description": "Prevent commits that exceed a specified file size limit from being pushed to the commit.", + "description": "Prevent commits with individual files that exceed the specified limit from being pushed to the commit graph.", "type": "object", "required": [ "type" @@ -1768,6 +1774,35 @@ ] } } + }, + { + "title": "copilot_code_review", + "description": "Request Copilot code review for new pull requests automatically if the author has access to Copilot code review and their premium requests quota has not reached the limit.", + "type": "object", + "required": [ + "type" + ], + "properties": { + "type": { + "type": "string", + "enum": [ + "copilot_code_review" + ] + }, + "parameters": { + "type": "object", + "properties": { + "review_draft_pull_requests": { + "type": "boolean", + "description": "Copilot automatically reviews draft pull requests before they are marked as ready for review." + }, + "review_on_push": { + "type": "boolean", + "description": "Copilot automatically reviews each new push to the pull request." + } + } + } + } } ] } From bb1b033f9b9a9b4463554ce731fa4b602155bba1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Madis=20K=C3=B5osaar?= Date: Thu, 12 Feb 2026 17:59:10 +0200 Subject: [PATCH 3/4] fix: update description for deprecated squash-merge commit title property in settings.json --- schema/dereferenced/settings.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/schema/dereferenced/settings.json b/schema/dereferenced/settings.json index fc9a45c3e..df3d9231a 100644 --- a/schema/dereferenced/settings.json +++ b/schema/dereferenced/settings.json @@ -155,7 +155,7 @@ }, "use_squash_pr_title_as_default": { "type": "boolean", - "description": "Either `true` to allow squash-merge commits to use pull request title, or `false` to use commit message. **This property is closing down. Please use `squash_merge_commit_title` instead.", + "description": "Either `true` to allow squash-merge commits to use pull request title, or `false` to use commit message. **This property is closing down. Please use `squash_merge_commit_title` instead.**", "default": false, "deprecated": true }, From 3a7c254edab0b19022bfcf95b99e39766a7e0ba4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Madis=20K=C3=B5osaar?= Date: Thu, 25 Jun 2026 12:22:32 +0300 Subject: [PATCH 4/4] feat: enhance organization role management in teams plugin --- app.yml | 4 + docs/deploy.md | 1 + lib/plugins/teams.js | 109 +++++++++++----- test/integration/plugins/teams.test.js | 15 ++- test/unit/lib/plugins/teams.test.js | 169 ++++++++++++++++++++++++- 5 files changed, 261 insertions(+), 37 deletions(-) diff --git a/app.yml b/app.yml index 1a72906d7..4b3685d27 100644 --- a/app.yml +++ b/app.yml @@ -94,6 +94,10 @@ default_permissions: # https://developer.github.com/v3/apps/permissions/#permission-on-members members: write + # View organization roles used to identify security manager teams. + # https://docs.github.com/en/rest/orgs/organization-roles + organization_custom_roles: read + # View and manage users blocked by the organization. # https://developer.github.com/v3/apps/permissions/#permission-on-organization-user-blocking # organization_user_blocking: read diff --git a/docs/deploy.md b/docs/deploy.md index 31a0bea31..5962c9dd7 100644 --- a/docs/deploy.md +++ b/docs/deploy.md @@ -298,6 +298,7 @@ Every deployment will need an [App](https://developer.github.com/apps/). #### Organization Permissions - Administration: **Read & Write** +- Custom organization roles: **Read-only** - Custom properties: **Admin** - Members: **Read & Write** diff --git a/lib/plugins/teams.js b/lib/plugins/teams.js index da97a2d2e..93248c659 100644 --- a/lib/plugins/teams.js +++ b/lib/plugins/teams.js @@ -2,8 +2,11 @@ const Diffable = require('./diffable') const NopCommand = require('../nopcommand') const teamRepoEndpoint = '/orgs/:owner/teams/:team_slug/repos/:owner/:repo' +const securityManagerRoleName = 'security_manager' +const safeSecurityManagerStatuses = [403, 404, 422] module.exports = class Teams extends Diffable { async find () { + this.skipTeamDeletion = false this.log.debug(`Finding teams for ${this.repo.owner}/${this.repo.repo}`) return this.github.paginate(this.github.rest.repos.listTeams, this.repo).then(res => { this.log.debug(`Found teams ${JSON.stringify(res)}`) @@ -14,47 +17,90 @@ module.exports = class Teams extends Diffable { // remove all security manager teams async checkSecurityManager (teams) { try { - // Uncomment the following lines to handle the deprecation of the teams api https://gh.io/security-managers-rest-api-sunset - // but this would require a new permission on the app - // - // const roles = await this.github.paginate('GET /orgs/{org}/roles', { org: this.repo.owner }) - // const securityManagerRole = roles.find(role => role.name === 'security_manager') - // - // this.log.debug(`Calling API to get security managers ${JSON.stringify(this.github.request.endpoint('GET /orgs/{org}/roles/{role_id}/teams', - // { - // org: this.repo.owner, - // role_id: securityManagerRole.id - // }))} `) - // const resp = await this.github.paginate('GET /orgs/{org}/roles/{role_id}/teams', - // { - // org: this.repo.owner, - // role_id: securityManagerRole.id - // }) - this.log.debug('Removing all security manager teams since they should not be handled here') - this.log.debug(`Calling API to get security managers ${JSON.stringify(this.github.request.endpoint('GET /orgs/{org}/security-managers', - { - org: this.repo.owner - }))} `) - const resp = await this.github.paginate('GET /orgs/{org}/security-managers', + this.log.debug(`Calling API to get organization roles ${JSON.stringify(this.github.request.endpoint('GET /orgs/{org}/organization-roles', + { + org: this.repo.owner + }))} `) + const rolesResp = await this.github.paginate('GET /orgs/{org}/organization-roles', { org: this.repo.owner }) + const roles = this.toArray(rolesResp, 'roles') + const securityManagerRole = roles.find(role => this.isSecurityManagerRole(role)) + + if (!securityManagerRole || !securityManagerRole.id) { + this.log.debug(`${this.repo.owner} Org does not have a security manager organization role set up`) + return teams + } + + const params = { + org: this.repo.owner, + role_id: securityManagerRole.id + } + this.log.debug(`Calling API to get security manager teams ${JSON.stringify(this.github.request.endpoint('GET /orgs/{org}/organization-roles/{role_id}/teams', params))} `) + const resp = await this.github.paginate('GET /orgs/{org}/organization-roles/{role_id}/teams', params) this.log.debug(`Response from the call is ${JSON.stringify(resp)}`) - return teams.filter(team => !resp.some(sec => sec.name === team.name)) + const securityManagerTeams = this.toArray(resp, 'teams') + const securityManagerTeamIdentifiers = new Set(securityManagerTeams.flatMap(team => [team.slug, team.name].map(name => this.normalizeTeamIdentifier(name))).filter(Boolean)) + + return teams.filter(team => !this.isSecurityManagerTeam(team, securityManagerTeamIdentifiers)) } catch (e) { - if (e.status === 404) { - this.log.debug(`${this.repo.owner} Org does not have Security manager teams set up ${e}`) + this.skipTeamDeletion = true + const status = e && e.status + if (safeSecurityManagerStatuses.includes(status)) { + this.log.debug(`${this.repo.owner} Org security manager teams could not be fetched with status ${status}; keeping repository teams unchanged ${e}`) } else { this.log.error( - `Unexpected error when fetching for security manager teams org ${this.repo.owner} = ${e}` + `Unexpected error when fetching security manager teams for org ${this.repo.owner}; keeping repository teams unchanged ${e}` ) } return teams } } + toArray (resp, propertyName) { + if (Array.isArray(resp)) { + return resp + } + + if (resp && Array.isArray(resp[propertyName])) { + return resp[propertyName] + } + + return [] + } + + isSecurityManagerRole (role) { + return [role && role.name, role && role.slug] + .map(name => this.normalizeRoleName(name)) + .includes(securityManagerRoleName) + } + + normalizeRoleName (name) { + if (typeof name !== 'string') { + return '' + } + + return name.trim().toLowerCase().replace(/[\s-]+/g, '_') + } + + normalizeTeamIdentifier (name) { + if (typeof name !== 'string') { + return '' + } + + return name.trim().toLowerCase().replace(/['’]/g, '').replace(/[^a-z0-9]+/g, '-').replace(/^-+|-+$/g, '') + } + + isSecurityManagerTeam (team, securityManagerTeamIdentifiers) { + return [team.slug, team.name] + .map(name => this.normalizeTeamIdentifier(name)) + .filter(Boolean) + .some(name => securityManagerTeamIdentifiers.has(name)) + } + comparator (existing, attrs) { - return existing.slug === attrs.name.toLowerCase() + return this.normalizeTeamIdentifier(existing.slug || existing.name) === this.normalizeTeamIdentifier(attrs.name) } changed (existing, attrs) { @@ -73,7 +119,7 @@ module.exports = class Teams extends Diffable { add (attrs) { let existing = { team_id: 1 } this.log.debug(`Getting team with the parms ${JSON.stringify(attrs)}`) - return this.github.rest.teams.getByName({ org: this.repo.owner, team_slug: attrs.name }).then(res => { + return this.github.rest.teams.getByName({ org: this.repo.owner, team_slug: this.normalizeTeamIdentifier(attrs.name) }).then(res => { existing = res.data this.log.debug(`adding team ${attrs.name} to repo ${this.repo.repo}`) if (this.nop) { @@ -114,6 +160,11 @@ module.exports = class Teams extends Diffable { } remove (existing) { + if (this.skipTeamDeletion) { + this.log.debug(`Skipping deletion of team ${existing.slug} from repo ${this.repo.repo} because security manager team discovery failed`) + return Promise.resolve() + } + if (this.nop) { return Promise.resolve([ new NopCommand(this.constructor.name, this.repo, this.github.request.endpoint( @@ -132,7 +183,7 @@ module.exports = class Teams extends Diffable { return { team_id: existing.id, org: this.repo.owner, - team_slug: attrs.name, + team_slug: existing.slug || this.normalizeTeamIdentifier(attrs.name), owner: this.repo.owner, repo: this.repo.repo, permission: attrs.permission diff --git a/test/integration/plugins/teams.test.js b/test/integration/plugins/teams.test.js index 4fde0637f..535a510f2 100644 --- a/test/integration/plugins/teams.test.js +++ b/test/integration/plugins/teams.test.js @@ -24,6 +24,8 @@ describe('teams plugin', function () { const probotTeamId = any.integer() const greenkeeperKeeperTeamId = any.integer() const formationTeamId = any.integer() + const securityManagerRoleId = any.integer() + const securityManagerTeamId = any.integer() githubScope .get(`/repos/${repository.owner.name}/${repository.name}/contents/${settings.FILE_PATH}`) .reply(OK, { content: encodedConfig, name: 'settings.yml', type: 'file' }) @@ -33,9 +35,20 @@ describe('teams plugin', function () { OK, [ { slug: 'greenkeeper-keeper', id: greenkeeperKeeperTeamId, permission: 'pull' }, - { slug: 'form8ion', id: formationTeamId, permission: 'push' } + { slug: 'form8ion', id: formationTeamId, permission: 'push' }, + { slug: 'security-managers', id: securityManagerTeamId, permission: 'push' } ] ) + githubScope + .get(`/orgs/${repository.owner.name}/organization-roles`) + .reply(OK, { + roles: [{ id: securityManagerRoleId, slug: 'security_manager', name: 'Security Manager' }] + }) + githubScope + .get(`/orgs/${repository.owner.name}/organization-roles/${securityManagerRoleId}/teams`) + .reply(OK, { + teams: [{ id: securityManagerTeamId, slug: 'security-managers', name: 'Security Managers' }] + }) githubScope .get(`/orgs/${repository.owner.name}/teams/probot`) .reply(OK, { id: probotTeamId }) diff --git a/test/unit/lib/plugins/teams.test.js b/test/unit/lib/plugins/teams.test.js index de16965a6..ab5dd1556 100644 --- a/test/unit/lib/plugins/teams.test.js +++ b/test/unit/lib/plugins/teams.test.js @@ -6,6 +6,9 @@ describe('Teams', () => { let github const addedTeamName = 'added' const addedTeamId = any.integer() + const securityManagerRoleId = any.integer() + const securityManagerTeamName = 'security-managers' + const securityManagerTeamId = any.integer() const updatedTeamName = 'updated-permission' const updatedTeamId = any.integer() const removedTeamName = 'removed' @@ -13,9 +16,18 @@ describe('Teams', () => { const unchangedTeamName = 'unchanged' const unchangedTeamId = any.integer() const org = 'bkeepers' + const organizationRolesRoute = 'GET /orgs/{org}/organization-roles' + const organizationRoleTeamsRoute = 'GET /orgs/{org}/organization-roles/{role_id}/teams' + const roleFailureStatuses = [403, 404, 422, 500] + const repoTeams = [ + { id: securityManagerTeamId, slug: securityManagerTeamName, name: 'Security Managers', permission: 'admin' }, + { id: unchangedTeamId, slug: unchangedTeamName, permission: 'push' }, + { id: removedTeamId, slug: removedTeamName, permission: 'push' }, + { id: updatedTeamId, slug: updatedTeamName, permission: 'pull' } + ] function configure (config) { - const log = { debug: jest.fn(), error: console.error } + const log = { debug: jest.fn(), error: jest.fn() } const errors = [] return new Teams(undefined, github, { owner: 'bkeepers', repo: 'test' }, config, log, errors) } @@ -38,11 +50,7 @@ describe('Teams', () => { }, repos: { listTeams: jest.fn().mockResolvedValue({ - data: [ - { id: unchangedTeamId, slug: unchangedTeamName, permission: 'push' }, - { id: removedTeamId, slug: removedTeamName, permission: 'push' }, - { id: updatedTeamId, slug: updatedTeamName, permission: 'pull' } - ] + data: repoTeams }) } }, @@ -53,13 +61,21 @@ describe('Teams', () => { }) describe('sync', () => { - it('syncs teams', async () => { + it('syncs non-security-manager teams and leaves security manager teams untouched', async () => { const plugin = configure([ { name: unchangedTeamName, permission: 'push' }, { name: updatedTeamName, permission: 'admin' }, { name: addedTeamName, permission: 'pull' } ]) + when(github.paginate) + .calledWith(organizationRolesRoute, { org }) + .mockResolvedValue({ roles: [{ id: securityManagerRoleId, name: 'Security Manager' }] }) + + when(github.paginate) + .calledWith(organizationRoleTeamsRoute, { org, role_id: securityManagerRoleId }) + .mockResolvedValue({ teams: [{ slug: securityManagerTeamName, name: 'Security Managers' }] }) + when(github.rest.teams.getByName) .defaultResolvedValue({}) .calledWith({ org: 'bkeepers', team_slug: addedTeamName }) @@ -88,7 +104,127 @@ describe('Teams', () => { permission: 'pull' }) + expect(github.paginate).toHaveBeenCalledWith(organizationRolesRoute, { org }) + expect(github.paginate).toHaveBeenCalledWith(organizationRoleTeamsRoute, { org, role_id: securityManagerRoleId }) expectTeamDeleted(removedTeamName) + expectTeamNotDeleted(securityManagerTeamName) + }) + + it.each(roleFailureStatuses)('skips deletions when organization role lookup fails with %s', async status => { + const plugin = configure([ + { name: unchangedTeamName, permission: 'push' } + ]) + + when(github.paginate) + .calledWith(organizationRolesRoute, { org }) + .mockRejectedValue({ status }) + + await plugin.sync() + + expectNoTeamsDeleted() + }) + + it.each(roleFailureStatuses)('skips deletions when organization role team lookup fails with %s', async status => { + const plugin = configure([ + { name: unchangedTeamName, permission: 'push' } + ]) + + when(github.paginate) + .calledWith(organizationRolesRoute, { org }) + .mockResolvedValue({ roles: [{ id: securityManagerRoleId, slug: 'security_manager' }] }) + + when(github.paginate) + .calledWith(organizationRoleTeamsRoute, { org, role_id: securityManagerRoleId }) + .mockRejectedValue({ status }) + + await plugin.sync() + + expectNoTeamsDeleted() + }) + + it('matches configured team names to existing slugs without add or remove churn', async () => { + const formattedTeamName = 'Platform & Security!' + + github.rest.repos.listTeams.mockResolvedValue({ + data: [{ id: unchangedTeamId, slug: 'platform-security', name: formattedTeamName, permission: 'push' }] + }) + + const plugin = configure([ + { name: formattedTeamName, permission: 'push' } + ]) + + await plugin.sync() + + expect(github.rest.teams.getByName).not.toHaveBeenCalled() + expectNoTeamsDeleted() + }) + + it('matches security manager team names against repository team slugs', async () => { + github.rest.repos.listTeams.mockResolvedValue({ + data: [{ id: securityManagerTeamId, slug: securityManagerTeamName, permission: 'admin' }] + }) + + when(github.paginate) + .calledWith(organizationRolesRoute, { org }) + .mockResolvedValue({ roles: [{ id: securityManagerRoleId, name: 'Security Manager' }] }) + + when(github.paginate) + .calledWith(organizationRoleTeamsRoute, { org, role_id: securityManagerRoleId }) + .mockResolvedValue({ teams: [{ name: 'Security Managers' }] }) + + const plugin = configure([]) + + await expect(plugin.find()).resolves.toEqual([]) + }) + + it('uses normalized team slugs when adding configured team names', async () => { + const formattedTeamName = 'Platform & Security!' + + github.rest.repos.listTeams.mockResolvedValue({ data: [] }) + + when(github.rest.teams.getByName) + .calledWith({ org, team_slug: 'platform-security' }) + .mockResolvedValue({ data: { id: addedTeamId, slug: 'platform-security' } }) + + const plugin = configure([ + { name: formattedTeamName, permission: 'pull' } + ]) + + await plugin.sync() + + expect(github.rest.teams.addOrUpdateRepoPermissionsInOrg).toHaveBeenCalledWith({ + org, + team_id: addedTeamId, + team_slug: 'platform-security', + owner: org, + repo: 'test', + permission: 'pull' + }) + }) + + it('returns original teams when the security manager role is absent', async () => { + const plugin = configure([]) + + when(github.paginate) + .calledWith(organizationRolesRoute, { org }) + .mockResolvedValue({ roles: [{ id: any.integer(), name: 'compliance_manager' }] }) + + await expect(plugin.find()).resolves.toEqual(repoTeams) + expect(github.paginate).not.toHaveBeenCalledWith(organizationRoleTeamsRoute, { org, role_id: securityManagerRoleId }) + }) + + it('returns original teams when organization role team lookup fails', async () => { + const plugin = configure([]) + + when(github.paginate) + .calledWith(organizationRolesRoute, { org }) + .mockResolvedValue({ roles: [{ id: securityManagerRoleId, slug: 'security_manager' }] }) + + when(github.paginate) + .calledWith(organizationRoleTeamsRoute, { org, role_id: securityManagerRoleId }) + .mockRejectedValue({ status: 500 }) + + await expect(plugin.find()).resolves.toEqual(repoTeams) }) function expectTeamDeleted (teamSlug) { @@ -102,5 +238,24 @@ describe('Teams', () => { } ) } + + function expectTeamNotDeleted (teamSlug) { + expect(github.request).not.toHaveBeenCalledWith( + 'DELETE /orgs/:owner/teams/:team_slug/repos/:owner/:repo', + { + org, + owner: org, + repo: 'test', + team_slug: teamSlug + } + ) + } + + function expectNoTeamsDeleted () { + expect(github.request).not.toHaveBeenCalledWith( + 'DELETE /orgs/:owner/teams/:team_slug/repos/:owner/:repo', + expect.any(Object) + ) + } }) })