From 50a5348b792f4c5e2e31eafa616705308c00efa4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Tue, 18 May 2021 15:04:50 +0200 Subject: [PATCH 1/2] Fix error handling in `LdapOrgReaderProcessor`, and support complex paging options MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .changeset/afraid-rockets-yell.md | 5 ++++ .../processors/LdapOrgReaderProcessor.ts | 6 ++++- .../src/ingestion/processors/ldap/client.ts | 21 ++++++++++++++-- .../ingestion/processors/ldap/config.test.ts | 10 ++++++-- .../src/ingestion/processors/ldap/config.ts | 25 +++++++++++++++++-- 5 files changed, 60 insertions(+), 7 deletions(-) create mode 100644 .changeset/afraid-rockets-yell.md diff --git a/.changeset/afraid-rockets-yell.md b/.changeset/afraid-rockets-yell.md new file mode 100644 index 0000000000..363c4dcd9f --- /dev/null +++ b/.changeset/afraid-rockets-yell.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend': patch +--- + +Fix error handling in `LdapOrgReaderProcessor`, and support complex paging options diff --git a/plugins/catalog-backend/src/ingestion/processors/LdapOrgReaderProcessor.ts b/plugins/catalog-backend/src/ingestion/processors/LdapOrgReaderProcessor.ts index d863142169..474e813590 100644 --- a/plugins/catalog-backend/src/ingestion/processors/LdapOrgReaderProcessor.ts +++ b/plugins/catalog-backend/src/ingestion/processors/LdapOrgReaderProcessor.ts @@ -69,7 +69,11 @@ export class LdapOrgReaderProcessor implements CatalogProcessor { // Be lazy and create the client each time; even though it's pretty // inefficient, we usually only do this once per entire refresh loop and // don't have to worry about timeouts and reconnects etc. - const client = await LdapClient.create(provider.target, provider.bind); + const client = await LdapClient.create( + this.logger, + provider.target, + provider.bind, + ); const { users, groups } = await readLdapOrg( client, provider.users, diff --git a/plugins/catalog-backend/src/ingestion/processors/ldap/client.ts b/plugins/catalog-backend/src/ingestion/processors/ldap/client.ts index bf0d0f740e..5ac71f4b50 100644 --- a/plugins/catalog-backend/src/ingestion/processors/ldap/client.ts +++ b/plugins/catalog-backend/src/ingestion/processors/ldap/client.ts @@ -15,6 +15,7 @@ */ import ldap, { Client, SearchEntry, SearchOptions } from 'ldapjs'; +import { Logger } from 'winston'; import { BindConfig } from './config'; import { errorString } from './util'; import { @@ -31,8 +32,20 @@ import { export class LdapClient { private vendor: Promise | undefined; - static async create(target: string, bind?: BindConfig): Promise { + static async create( + logger: Logger, + target: string, + bind?: BindConfig, + ): Promise { const client = ldap.createClient({ url: target }); + + // We want to have a catch-all error handler at the top, since the default + // behavior of the client is to blow up the entire process when it fails, + // unless an error handler is set. + client.on('error', (err: ldap.Error) => { + logger.warn(`LDAP client threw an error, ${errorString(err)}`); + }); + if (!bind) { return new LdapClient(client); } @@ -62,6 +75,10 @@ export class LdapClient { return await new Promise((resolve, reject) => { const output: SearchEntry[] = []; + this.client.on('error', (err: ldap.Error) => { + reject(new Error(errorString(err))); + }); + this.client.search(dn, options, (err, res) => { if (err) { reject(new Error(errorString(err))); @@ -92,7 +109,7 @@ export class LdapClient { }); }); } catch (e) { - throw new Error(`LDAP search at ${dn} failed, ${e.message}`); + throw new Error(`LDAP search at DN "${dn}" failed, ${e.message}`); } } diff --git a/plugins/catalog-backend/src/ingestion/processors/ldap/config.test.ts b/plugins/catalog-backend/src/ingestion/processors/ldap/config.test.ts index 607581e153..755c173d19 100644 --- a/plugins/catalog-backend/src/ingestion/processors/ldap/config.test.ts +++ b/plugins/catalog-backend/src/ingestion/processors/ldap/config.test.ts @@ -105,7 +105,10 @@ describe('readLdapConfig', () => { scope: 'base', attributes: ['*'], filter: 'f', - paged: true, + paged: { + pageSize: 7, + pagePause: true, + }, }, set: { p: 'v' }, map: { @@ -153,7 +156,10 @@ describe('readLdapConfig', () => { scope: 'base', attributes: ['*'], filter: 'f', - paged: true, + paged: { + pageSize: 7, + pagePause: true, + }, }, set: { p: 'v' }, map: { diff --git a/plugins/catalog-backend/src/ingestion/processors/ldap/config.ts b/plugins/catalog-backend/src/ingestion/processors/ldap/config.ts index aca02e6e10..417657b244 100644 --- a/plugins/catalog-backend/src/ingestion/processors/ldap/config.ts +++ b/plugins/catalog-backend/src/ingestion/processors/ldap/config.ts @@ -180,11 +180,32 @@ export function readLdapConfig(config: Config): LdapProviderConfig[] { if (!c) { return {}; } + + const paged = readOptionsPagedConfig(c); + return { scope: c.getOptionalString('scope') as SearchOptions['scope'], filter: formatFilter(c.getOptionalString('filter')), attributes: c.getOptionalStringArray('attributes'), - paged: c.getOptionalBoolean('paged'), + ...(paged !== undefined ? { paged } : undefined), + }; + } + + function readOptionsPagedConfig(c: Config): SearchOptions['paged'] { + const pagedConfig = c.getOptional('paged'); + if (pagedConfig === undefined) { + return undefined; + } + + if (pagedConfig === true || pagedConfig === false) { + return pagedConfig; + } + + const pageSize = c.getOptionalNumber('paged.pageSize'); + const pagePause = c.getOptionalBoolean('paged.pagePause'); + return { + ...(pageSize !== undefined ? { pageSize } : undefined), + ...(pagePause !== undefined ? { pagePause } : undefined), }; } @@ -258,7 +279,7 @@ export function readLdapConfig(config: Config): LdapProviderConfig[] { } function formatFilter(filter?: string): string | undefined { - // Remove extra whitespaces between blocks to support multiline filters from the configuration + // Remove extra whitespace between blocks to support multiline filters from the configuration return filter?.replace(/\s*(\(|\))/g, '$1')?.trim(); } From 61a9c71bdcece0a86ef2abf968c08ae98435c1a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Thu, 20 May 2021 11:40:36 +0200 Subject: [PATCH 2/2] remove the extra error handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .../catalog-backend/src/ingestion/processors/ldap/client.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/plugins/catalog-backend/src/ingestion/processors/ldap/client.ts b/plugins/catalog-backend/src/ingestion/processors/ldap/client.ts index 5ac71f4b50..0557f21003 100644 --- a/plugins/catalog-backend/src/ingestion/processors/ldap/client.ts +++ b/plugins/catalog-backend/src/ingestion/processors/ldap/client.ts @@ -75,10 +75,6 @@ export class LdapClient { return await new Promise((resolve, reject) => { const output: SearchEntry[] = []; - this.client.on('error', (err: ldap.Error) => { - reject(new Error(errorString(err))); - }); - this.client.search(dn, options, (err, res) => { if (err) { reject(new Error(errorString(err)));