diff --git a/docs/sample-settings/settings.yml b/docs/sample-settings/settings.yml index 1ede6a079..9b6427a93 100644 --- a/docs/sample-settings/settings.yml +++ b/docs/sample-settings/settings.yml @@ -250,7 +250,9 @@ rulesets: # - pull_request bypass_mode: pull_request - - actor_id: 1 + # actor_id must be null for OrganizationAdmin: GitHub ignores the id for + # this actor type and returns null, so any other value never converges + - actor_id: null actor_type: OrganizationAdmin bypass_mode: always diff --git a/lib/mergeDeep.js b/lib/mergeDeep.js index ab278e5c2..c6b75bcbc 100644 --- a/lib/mergeDeep.js +++ b/lib/mergeDeep.js @@ -2,7 +2,8 @@ const mergeBy = require('./mergeArrayBy') const DeploymentConfig = require('./deploymentConfig') const NAME_FIELDS = ['name', 'username', 'actor_id', 'login', 'type', 'key_prefix', 'context'] -const NAME_USERNAME_PROPERTY = item => NAME_FIELDS.find(prop => Object.prototype.hasOwnProperty.call(item, prop)) +const IDENTITY_FIELDS = ['name', 'username', 'actor_id', 'actor_type', 'login', 'type', 'key_prefix', 'context'] +const NAME_USERNAME_PROPERTY = item => IDENTITY_FIELDS.find(prop => Object.prototype.hasOwnProperty.call(item, prop) && item[prop] != null) const GET_NAME_USERNAME_PROPERTY = item => { if (NAME_USERNAME_PROPERTY(item)) return item[NAME_USERNAME_PROPERTY(item)] } class MergeDeep { @@ -92,8 +93,8 @@ class MergeDeep { // So any property in the target that is not in the source is not treated as a deletion for (const key in source) { // Skip prototype pollution vectors - if (key === "__proto__" || key === "constructor") { - continue; + if (key === '__proto__' || key === 'constructor') { + continue } // Logic specific for Github // API response includes urls for resources, or other ignorable fields; we can ignore them diff --git a/test/unit/lib/mergeDeep.test.js b/test/unit/lib/mergeDeep.test.js index dd8bd60ba..57f152b0a 100644 --- a/test/unit/lib/mergeDeep.test.js +++ b/test/unit/lib/mergeDeep.test.js @@ -1882,4 +1882,44 @@ branches: expect(same.additions).toEqual({}) expect(same.modifications).toEqual({}) }) + + describe('CompareDeep bypass actors with a null actor_id (OrganizationAdmin, DeployKey)', () => { + // GitHub ignores actor_id for these actor types and always returns + // actor_id: null, so the config should carry null and still diff correctly. + const mergeDeep = new MergeDeep(log, jest.fn(), []) + const orgAdmin = { actor_id: null, actor_type: 'OrganizationAdmin', bypass_mode: 'pull_request' } + const deployKey = { actor_id: null, actor_type: 'DeployKey', bypass_mode: 'always' } + const app = { actor_id: 210920, actor_type: 'Integration', bypass_mode: 'always' } + + it('deploys a config-only actor from an empty ruleset', () => { + const result = mergeDeep.compareDeep({ bypass_actors: [] }, { bypass_actors: [orgAdmin] }) + expect(result.hasChanges).toEqual(true) + expect(result.additions.bypass_actors).toEqual([orgAdmin]) + }) + + it('deploys a config-only actor alongside an existing app actor', () => { + const result = mergeDeep.compareDeep({ bypass_actors: [app] }, { bypass_actors: [app, orgAdmin] }) + expect(result.hasChanges).toEqual(true) + expect(result.additions.bypass_actors).toEqual([orgAdmin]) + }) + + it('converges once the actor is live', () => { + const result = mergeDeep.compareDeep({ bypass_actors: [orgAdmin] }, { bypass_actors: [orgAdmin] }) + expect(result.hasChanges).toEqual(false) + }) + + it('converges with mixed actors and still identifies real ids by actor_id', () => { + const result = mergeDeep.compareDeep({ bypass_actors: [app, orgAdmin] }, { bypass_actors: [app, orgAdmin] }) + expect(result.hasChanges).toEqual(false) + }) + + it('handles a DeployKey actor the same way, distinct from other null-id actors', () => { + const deploys = mergeDeep.compareDeep({ bypass_actors: [orgAdmin] }, { bypass_actors: [orgAdmin, deployKey] }) + expect(deploys.hasChanges).toEqual(true) + expect(deploys.additions.bypass_actors).toEqual([deployKey]) + + const converged = mergeDeep.compareDeep({ bypass_actors: [orgAdmin, deployKey] }, { bypass_actors: [orgAdmin, deployKey] }) + expect(converged.hasChanges).toEqual(false) + }) + }) })