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) + ) + } }) })