Bug/issue-903

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

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**
 

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

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