diff --git a/docs/deploy.md b/docs/deploy.md index d61377bed..7767899a8 100644 --- a/docs/deploy.md +++ b/docs/deploy.md @@ -295,6 +295,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 0df459a2f..5e789526a 100644 --- a/lib/plugins/teams.js +++ b/lib/plugins/teams.js @@ -4,6 +4,8 @@ const NopCommand = require('../nopcommand') const teamRepoEndpoint = '/orgs/:owner/teams/:team_slug/repos/:owner/:repo' const listExternalGroupsEndpoint = 'GET /orgs/{org}/external-groups' const teamExternalGroupsEndpoint = '/orgs/{org}/teams/{team_slug}/external-groups' +const securityManagerRoleName = 'security_manager' +const safeSecurityManagerStatuses = [403, 404, 422] module.exports = class Teams extends Diffable { // Override Diffable.sync to also reconcile the optional `external_group` @@ -27,6 +29,8 @@ module.exports = class Teams extends Diffable { } async find () { + this.skipTeamDeletion = false + this.securityManagerTeamIdentifiers = new Set() this.log.debug(`Finding teams for ${this.repo.owner}/${this.repo.repo}`) return this.github.paginate(this.github.repos.listTeams, this.repo).then(res => { this.log.debug(`Found teams ${JSON.stringify(res)}`) @@ -37,47 +41,117 @@ 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)) + // Persist the identifiers so add()/update()/remove() can no-op for + // security manager teams even when they appear in the config. Without + // this, a configured security manager team would look "missing" from the + // filtered existing list and Diffable.sync() would (re)add it here. + this.securityManagerTeamIdentifiers = securityManagerTeamIdentifiers + + 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)) + } + + // True when the given attrs/record refers to a discovered security manager + // team. Security manager teams are intentionally not managed by this plugin, + // so add()/update()/remove() must no-op for them even if they are present in + // the config file. + isConfiguredSecurityManagerTeam (attrs) { + if (!this.securityManagerTeamIdentifiers || this.securityManagerTeamIdentifiers.size === 0) { + return false + } + return this.isSecurityManagerTeam(attrs, this.securityManagerTeamIdentifiers) + } + + skipSecurityManagerTeam (attrs, verb) { + const teamName = (attrs && (attrs.name || attrs.slug)) || 'unknown' + this.log.debug(`Skipping ${verb} of security manager team ${teamName} for repo ${this.repo.repo}; security manager teams are not managed here`) + if (this.nop) { + return Promise.resolve([ + new NopCommand(this.constructor.name, this.repo, null, `Skipping ${verb} of security manager team ${teamName}; security manager teams are not managed by safe-settings`, 'INFO') + ]) + } + return Promise.resolve() + } + comparator (existing, attrs) { - return existing.slug === attrs.name.toLowerCase() + return this.normalizeTeamIdentifier(existing.slug || existing.name) === this.normalizeTeamIdentifier(attrs.name) } changed (existing, attrs) { @@ -85,6 +159,9 @@ module.exports = class Teams extends Diffable { } update (existing, attrs) { + if (this.isConfiguredSecurityManagerTeam(attrs)) { + return this.skipSecurityManagerTeam(attrs, 'update') + } if (this.nop) { return Promise.resolve([ new NopCommand(this.constructor.name, this.repo, this.github.request.endpoint(`PUT ${teamRepoEndpoint}`, this.toParams(existing, attrs)), 'Add Teams to Repo') @@ -94,9 +171,12 @@ module.exports = class Teams extends Diffable { } add (attrs) { + if (this.isConfiguredSecurityManagerTeam(attrs)) { + return this.skipSecurityManagerTeam(attrs, 'add') + } let existing = { team_id: 1 } this.log.debug(`Getting team with the parms ${JSON.stringify(attrs)}`) - return this.github.teams.getByName({ org: this.repo.owner, team_slug: attrs.name }).then(res => { + return this.github.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) { @@ -137,6 +217,21 @@ module.exports = class Teams extends Diffable { } remove (existing) { + if (this.isConfiguredSecurityManagerTeam(existing)) { + return this.skipSecurityManagerTeam(existing, 'removal') + } + + if (this.skipTeamDeletion) { + const msg = `Skipping deletion of team ${existing.slug} from repo ${this.repo.repo} because security manager team discovery failed` + this.log.debug(msg) + if (this.nop) { + return Promise.resolve([ + new NopCommand(this.constructor.name, this.repo, null, msg, 'INFO') + ]) + } + return Promise.resolve() + } + if (this.nop) { return Promise.resolve([ new NopCommand(this.constructor.name, this.repo, this.github.request.endpoint( @@ -155,7 +250,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 f7c599d8b..5e46a3654 100644 --- a/test/unit/lib/plugins/teams.test.js +++ b/test/unit/lib/plugins/teams.test.js @@ -23,9 +23,12 @@ describe('Teams', () => { beforeEach(() => { github = { paginate: jest.fn() - .mockImplementation(async (fetch) => { - const response = await fetch() - return response.data + .mockImplementation(async (fetchOrRoute) => { + if (typeof fetchOrRoute === 'function') { + const response = await fetchOrRoute() + return response.data + } + return [] }), teams: { create: jest.fn().mockResolvedValue(), @@ -41,7 +44,7 @@ describe('Teams', () => { ] }) }, - request: jest.fn().mockResolvedValue() + request: Object.assign(jest.fn().mockResolvedValue(), { endpoint: jest.fn().mockReturnValue({ url: 'endpoint-stub', body: {} }) }) } }) @@ -97,6 +100,266 @@ describe('Teams', () => { } }) + describe('security manager teams', () => { + const securityManagerRoleId = any.integer() + const securityManagerTeamName = 'security-managers' + const securityManagerTeamId = any.integer() + 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' } + ] + + beforeEach(() => { + github.repos.listTeams.mockResolvedValue({ data: repoTeams }) + }) + + function expectTeamDeleted (teamSlug) { + expect(github.request).toHaveBeenCalledWith( + 'DELETE /orgs/:owner/teams/:team_slug/repos/:owner/:repo', + { + org, + owner: org, + repo: 'test', + team_slug: teamSlug + } + ) + } + + 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) + ) + } + + 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.teams.getByName) + .defaultResolvedValue({}) + .calledWith({ org, team_slug: addedTeamName }) + .mockResolvedValue({ data: { id: addedTeamId } }) + + await plugin.sync() + + expect(github.paginate).toHaveBeenCalledWith(organizationRolesRoute, { org }) + expect(github.paginate).toHaveBeenCalledWith(organizationRoleTeamsRoute, { org, role_id: securityManagerRoleId }) + expectTeamDeleted(removedTeamName) + expectTeamNotDeleted(securityManagerTeamName) + }) + + it('does not add or update a security manager team even when it is listed in the config', async () => { + const plugin = configure([ + { name: securityManagerTeamName, permission: 'pull' }, + { name: unchangedTeamName, permission: 'push' } + ]) + + 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' }] }) + + await plugin.sync() + + expect(github.teams.getByName).not.toHaveBeenCalledWith({ org, team_slug: securityManagerTeamName }) + expect(github.teams.addOrUpdateRepoPermissionsInOrg).not.toHaveBeenCalled() + expect(github.request).not.toHaveBeenCalledWith( + 'PUT /orgs/:owner/teams/:team_slug/repos/:owner/:repo', + expect.objectContaining({ team_slug: securityManagerTeamName }) + ) + expectTeamNotDeleted(securityManagerTeamName) + }) + + it('emits an INFO nop command instead of managing a configured security manager team in nop mode', async () => { + const log = { debug: jest.fn(), error: jest.fn(), warn: jest.fn() } + const plugin = new Teams(true, github, { owner: org, repo: 'test' }, [ + { name: securityManagerTeamName, permission: 'pull' } + ], log, []) + + 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' }] }) + + const result = await plugin.sync() + + expect(Array.isArray(result)).toBe(true) + const flattened = result.flat(Infinity) + expect(flattened.some(c => c && c.type === 'INFO' && /security manager team/i.test(JSON.stringify(c)))).toBe(true) + expect(github.teams.addOrUpdateRepoPermissionsInOrg).not.toHaveBeenCalled() + }) + + 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('emits an INFO nop command when skipping deletion in nop mode after discovery failure', async () => { + const log = { debug: jest.fn(), error: jest.fn(), warn: jest.fn() } + const plugin = new Teams(true, github, { owner: org, repo: 'test' }, [ + { name: unchangedTeamName, permission: 'push' } + ], log, []) + + when(github.paginate) + .calledWith(organizationRolesRoute, { org }) + .mockRejectedValue({ status: 500 }) + + const result = await plugin.sync() + + expect(Array.isArray(result)).toBe(true) + const flattened = result.flat(Infinity) + expect(flattened.some(c => c && c.type === 'INFO' && /security manager team discovery failed/i.test(JSON.stringify(c)))).toBe(true) + expectNoTeamsDeleted() + }) + + it('matches configured team names to existing slugs without add or remove churn', async () => { + const formattedTeamName = 'Platform & Security!' + + github.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.teams.getByName).not.toHaveBeenCalled() + expectNoTeamsDeleted() + }) + + it('matches security manager team names against repository team slugs', async () => { + github.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.repos.listTeams.mockResolvedValue({ data: [] }) + + when(github.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.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) + }) + }) + describe('filtering teams by include/exclude', () => { beforeEach(() => { github.repos.listTeams.mockResolvedValue({ data: [] }) @@ -158,7 +421,7 @@ describe('Teams', () => { } return Promise.resolve({ data: {} }) }) - github.request.endpoint = jest.fn().mockReturnValue('endpoint-stub') + github.request.endpoint = jest.fn().mockReturnValue({ url: 'endpoint-stub', body: {} }) // paginate: route the external-groups list call to a single page; keep // the original implementation for other paginated endpoints. The real @@ -221,7 +484,7 @@ describe('Teams', () => { } return Promise.resolve({ data: {} }) }) - github.request.endpoint = jest.fn().mockReturnValue('endpoint-stub') + github.request.endpoint = jest.fn().mockReturnValue({ url: 'endpoint-stub', body: {} }) const plugin = configure([ { name: unchangedTeamName, permission: 'push', external_group: externalGroupName }