From 2b19fd2e941b6b3a00abc96a519597c40e290a68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Sat, 8 Jan 2022 16:30:45 +0100 Subject: [PATCH] deep clone data read out of config, to avoid pollution / mutation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .changeset/fuzzy-llamas-collect.md | 5 + .../src/ldap/config.test.ts | 84 +++++++++ .../src/ldap/config.ts | 13 +- .../src/ldap/read.test.ts | 169 +++++++++++++++++- .../src/ldap/read.ts | 5 +- 5 files changed, 271 insertions(+), 5 deletions(-) create mode 100644 .changeset/fuzzy-llamas-collect.md diff --git a/.changeset/fuzzy-llamas-collect.md b/.changeset/fuzzy-llamas-collect.md new file mode 100644 index 0000000000..17ee925e56 --- /dev/null +++ b/.changeset/fuzzy-llamas-collect.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend-module-ldap': patch +--- + +Make sure to avoid accidental data sharing / mutation of `set` values diff --git a/plugins/catalog-backend-module-ldap/src/ldap/config.test.ts b/plugins/catalog-backend-module-ldap/src/ldap/config.test.ts index 9fb536a87f..06ac24d3ae 100644 --- a/plugins/catalog-backend-module-ldap/src/ldap/config.test.ts +++ b/plugins/catalog-backend-module-ldap/src/ldap/config.test.ts @@ -209,4 +209,88 @@ describe('readLdapConfig', () => { const expected = '(|(cn=foo bar)(cn=bar))'; expect(actual[0].users.options.filter).toEqual(expected); }); + + it('supports a dot nested set structure', () => { + const config = { + providers: [ + { + target: 'target', + users: { + dn: 'udn', + options: { + filter: 'f', + }, + set: { + 'metadata.annotations': { + a: 'b', + }, + }, + }, + groups: { + dn: 'gdn', + options: { + filter: 'f', + }, + set: { + x: { a: 'b' }, + }, + }, + }, + ], + }; + const actual = readLdapConfig(new ConfigReader(config)); + + expect(actual[0].users.set).toEqual({ 'metadata.annotations': { a: 'b' } }); + }); + + it('throws on attempts to modify the set structure', () => { + const config = { + providers: [ + { + target: 'target', + users: { + dn: 'udn', + options: { + filter: 'f', + }, + set: { + x: { a: 'b' }, + }, + }, + groups: { + dn: 'gdn', + options: { + filter: 'f', + }, + set: { + x: { a: 'b' }, + }, + }, + }, + ], + }; + const actual = readLdapConfig(new ConfigReader(config)); + + expect(() => { + (actual[0].users.set as any).y = 2; + }).toThrowErrorMatchingInlineSnapshot( + `"Cannot add property y, object is not extensible"`, + ); + expect(() => { + (actual[0].users.set as any).x.b = 2; + }).toThrowErrorMatchingInlineSnapshot( + `"Cannot add property b, object is not extensible"`, + ); + + expect(() => { + (actual[0].groups.set as any).y = 2; + }).toThrowErrorMatchingInlineSnapshot( + `"Cannot add property y, object is not extensible"`, + ); + expect(() => { + (actual[0].groups.set as any).x.b = 2; + }).toThrowErrorMatchingInlineSnapshot( + `"Cannot add property b, object is not extensible"`, + ); + }); }); diff --git a/plugins/catalog-backend-module-ldap/src/ldap/config.ts b/plugins/catalog-backend-module-ldap/src/ldap/config.ts index dc1ec4910d..b12c7c9fee 100644 --- a/plugins/catalog-backend-module-ldap/src/ldap/config.ts +++ b/plugins/catalog-backend-module-ldap/src/ldap/config.ts @@ -166,6 +166,15 @@ const defaultConfig = { * @param config The root of the LDAP config hierarchy */ export function readLdapConfig(config: Config): LdapProviderConfig[] { + function freeze(data: T): T { + return JSON.parse(JSON.stringify(data), (_key, value) => { + if (typeof value === 'object' && value !== null) { + Object.freeze(value); + } + return value; + }); + } + function readBindConfig( c: Config | undefined, ): LdapProviderConfig['bind'] | undefined { @@ -217,7 +226,7 @@ export function readLdapConfig(config: Config): LdapProviderConfig[] { if (!c) { return undefined; } - return Object.fromEntries(c.keys().map(path => [path, c.get(path)])); + return c.get(); } function readUserMapConfig( @@ -297,6 +306,6 @@ export function readLdapConfig(config: Config): LdapProviderConfig[] { // Replace arrays instead of merging, otherwise default behavior return Array.isArray(from) ? from : undefined; }); - return merged as LdapProviderConfig; + return freeze(merged) as LdapProviderConfig; }); } diff --git a/plugins/catalog-backend-module-ldap/src/ldap/read.test.ts b/plugins/catalog-backend-module-ldap/src/ldap/read.test.ts index c4f203c5db..875ffb7029 100644 --- a/plugins/catalog-backend-module-ldap/src/ldap/read.test.ts +++ b/plugins/catalog-backend-module-ldap/src/ldap/read.test.ts @@ -25,7 +25,13 @@ import { LDAP_RDN_ANNOTATION, LDAP_UUID_ANNOTATION, } from './constants'; -import { readLdapGroups, readLdapUsers, resolveRelations } from './read'; +import { + defaultGroupTransformer, + defaultUserTransformer, + readLdapGroups, + readLdapUsers, + resolveRelations, +} from './read'; import { ActiveDirectoryVendor, DefaultLdapVendor } from './vendors'; function user(data: RecursivePartial): UserEntity { @@ -264,6 +270,7 @@ describe('readLdapGroups', () => { new Map([['dn-value', new Set(['x', 'y', 'z'])]]), ); }); + it('transfers all attributes from Microsoft Active Directory', async () => { client.getVendor.mockResolvedValue(ActiveDirectoryVendor); client.searchStreaming.mockImplementation(async (_dn, _opts, fn) => { @@ -358,6 +365,7 @@ describe('resolveRelations', () => { expect(parent.spec.children).toEqual(['child']); expect(child.spec.parent).toEqual('parent'); }); + it('matches by UUID', () => { const parent = group({ metadata: { @@ -539,3 +547,162 @@ describe('resolveRelations', () => { }); }); }); + +describe('defaultUserTransformer', () => { + it('can set things safely', async () => { + const config: UserConfig = { + dn: 'ddd', + options: {}, + map: { + rdn: 'uid', + name: 'uid', + displayName: 'cn', + email: 'mail', + memberOf: 'memberOf', + }, + set: { + 'metadata.annotations.a': 1, + 'metadata.annotations': { a: 2, b: 3 }, + }, + }; + + const entry = searchEntry({ + uid: ['uid-value'], + description: ['description-value'], + cn: ['cn-value'], + mail: ['mail-value'], + avatarUrl: ['avatarUrl-value'], + memberOf: ['x', 'y', 'z'], + entryDN: ['dn-value'], + entryUUID: ['uuid-value'], + }); + + let output = await defaultUserTransformer(DefaultLdapVendor, config, entry); + expect(output).toEqual({ + apiVersion: 'backstage.io/v1beta1', + kind: 'User', + metadata: { + annotations: { + 'backstage.io/ldap-dn': 'dn-value', + 'backstage.io/ldap-rdn': 'uid-value', + 'backstage.io/ldap-uuid': 'uuid-value', + a: 2, + b: 3, + }, + name: 'uid-value', + }, + spec: { + memberOf: [], + profile: { displayName: 'cn-value', email: 'mail-value' }, + }, + }); + + (output!.metadata.annotations as any).c = 7; + + // exact same inputs again + output = await defaultUserTransformer(DefaultLdapVendor, config, entry); + expect(output).toEqual({ + apiVersion: 'backstage.io/v1beta1', + kind: 'User', + metadata: { + annotations: { + 'backstage.io/ldap-dn': 'dn-value', + 'backstage.io/ldap-rdn': 'uid-value', + 'backstage.io/ldap-uuid': 'uuid-value', + a: 2, + b: 3, + }, + name: 'uid-value', + }, + spec: { + memberOf: [], + profile: { displayName: 'cn-value', email: 'mail-value' }, + }, + }); + }); +}); + +describe('defaultGroupTransformer', () => { + it('can set things safely', async () => { + const config: GroupConfig = { + dn: 'ddd', + options: {}, + map: { + rdn: 'uid', + name: 'uid', + displayName: 'cn', + email: 'mail', + description: 'description', + type: 'type', + members: 'members', + memberOf: 'memberOf', + }, + set: { + 'metadata.annotations.a': 1, + 'metadata.annotations': { a: 2, b: 3 }, + }, + }; + + const entry = searchEntry({ + uid: ['uid-value'], + description: ['description-value'], + cn: ['cn-value'], + mail: ['mail-value'], + avatarUrl: ['avatarUrl-value'], + memberOf: ['x', 'y', 'z'], + entryDN: ['dn-value'], + entryUUID: ['uuid-value'], + }); + + let output = await defaultGroupTransformer( + DefaultLdapVendor, + config, + entry, + ); + expect(output).toEqual({ + apiVersion: 'backstage.io/v1beta1', + kind: 'Group', + metadata: { + annotations: { + 'backstage.io/ldap-dn': 'dn-value', + 'backstage.io/ldap-rdn': 'uid-value', + 'backstage.io/ldap-uuid': 'uuid-value', + a: 2, + b: 3, + }, + description: 'description-value', + name: 'uid-value', + }, + spec: { + type: 'unknown', + children: [], + profile: { displayName: 'cn-value', email: 'mail-value' }, + }, + }); + + (output!.metadata.annotations as any).c = 7; + + // exact same inputs again + output = await defaultGroupTransformer(DefaultLdapVendor, config, entry); + expect(output).toEqual({ + apiVersion: 'backstage.io/v1beta1', + kind: 'Group', + metadata: { + annotations: { + 'backstage.io/ldap-dn': 'dn-value', + 'backstage.io/ldap-rdn': 'uid-value', + 'backstage.io/ldap-uuid': 'uuid-value', + a: 2, + b: 3, + }, + description: 'description-value', + name: 'uid-value', + }, + spec: { + type: 'unknown', + children: [], + profile: { displayName: 'cn-value', email: 'mail-value' }, + }, + }); + }); +}); diff --git a/plugins/catalog-backend-module-ldap/src/ldap/read.ts b/plugins/catalog-backend-module-ldap/src/ldap/read.ts index 2d6e3c1608..513d899304 100644 --- a/plugins/catalog-backend-module-ldap/src/ldap/read.ts +++ b/plugins/catalog-backend-module-ldap/src/ldap/read.ts @@ -17,6 +17,7 @@ import { GroupEntity, UserEntity } from '@backstage/catalog-model'; import { SearchEntry } from 'ldapjs'; import lodashSet from 'lodash/set'; +import cloneDeep from 'lodash/cloneDeep'; import { buildOrgHierarchy } from './org'; import { LdapClient } from './client'; import { GroupConfig, UserConfig } from './config'; @@ -52,7 +53,7 @@ export async function defaultUserTransformer( if (set) { for (const [path, value] of Object.entries(set)) { - lodashSet(entity, path, value); + lodashSet(entity, path, cloneDeep(value)); } } @@ -146,7 +147,7 @@ export async function defaultGroupTransformer( if (set) { for (const [path, value] of Object.entries(set)) { - lodashSet(entity, path, value); + lodashSet(entity, path, cloneDeep(value)); } }