From ef1953210911238406b64fea0e37f25601e8fbbe Mon Sep 17 00:00:00 2001 From: nandan-bhat <167290944+nandan-bhat@users.noreply.github.com> Date: Fri, 19 Jul 2024 18:12:09 +0530 Subject: [PATCH] Adding fixes for permission errors (403) (#925) * Adding fixes for permission errors * Updating the warning message * Adding fixes for too_many_requests (429) error --------- Co-authored-by: KunalOfficial <35455566+developerkunal@users.noreply.github.com> --- src/tools/auth0/handlers/connections.ts | 5 + src/tools/auth0/handlers/scimHandler.ts | 176 +++++++++++++----- .../tools/auth0/handlers/scimHandler.tests.js | 19 +- 3 files changed, 152 insertions(+), 48 deletions(-) diff --git a/src/tools/auth0/handlers/connections.ts b/src/tools/auth0/handlers/connections.ts index 10b2587f..bb94d928 100644 --- a/src/tools/auth0/handlers/connections.ts +++ b/src/tools/auth0/handlers/connections.ts @@ -183,6 +183,11 @@ export default class ConnectionsHandler extends DefaultAPIHandler { config: this.config, }); + // We add the expected changes to the scimHandler so it can track the progress of the SCIM changes. + // This is necessary because import / deploy actions are concurrent and we need to know when all updates are complete. + const { update, create, conflicts } = proposedChangesWithExcludedProperties; + this.scimHandler.expectedChanges = update.length + create.length + conflicts.length; + return proposedChangesWithExcludedProperties; } diff --git a/src/tools/auth0/handlers/scimHandler.ts b/src/tools/auth0/handlers/scimHandler.ts index 064b1168..a5ae0142 100644 --- a/src/tools/auth0/handlers/scimHandler.ts +++ b/src/tools/auth0/handlers/scimHandler.ts @@ -14,9 +14,11 @@ interface scimRequestParams { interface scimBodyParams { user_id_attribute: string; - mapping: { scim: string; auth0: string; }[]; + mapping: { scim: string; auth0: string }[]; } +interface ScimScopes { read: boolean; create: boolean; update: boolean; delete: boolean } + /** * The current version of this sdk use `node-auth0` v3. But `SCIM` features are not natively supported by v3. * This is a workaround to make this SDK support SCIM without `node-auth0` upgrade. @@ -27,6 +29,18 @@ export default class ScimHandler { private tokenProvider: any; private config: any; private connectionsManager: any; + private updateQueue: any[] = []; + private isExecuting = false; + private scimScopes: ScimScopes = { read: true, create: true, update: true, delete: true }; + private scopeMethodMap = { + get: 'read', + post: 'create', + patch: 'update', + delete: 'delete' + } + + expectedChanges: number = 0; + completedChanges: number = 0; constructor(config, tokenProvider, connectionsManager) { this.config = config; @@ -54,20 +68,18 @@ export default class ScimHandler { this.idMap.clear(); for (let connection of connections) { + if (!this.scimScopes.read) return; if (!this.isScimStrategy(connection.strategy)) continue; - - try { - this.idMap.set(connection.id, { strategy: connection.strategy, hasConfig: false }); - await this.getScimConfiguration({ id: connection.id }); - this.idMap.set(connection.id, { ...this.idMap.get(connection.id)!, hasConfig: true }); - - // To avoid rate limiter error, we making API requests with a small delay. - // TODO: However, this logic needs to be re-worked. - await sleep(500); - } catch (err) { - // Skip the connection if it returns 404. This can happen if `SCIM` is not enabled on a `SCIM` connection. - if (err !== 'SCIM_NOT_FOUND') throw err; - } + + // To avoid rate limiter error, we making API requests with a small delay. + // TODO: However, this logic needs to be re-worked. + await sleep(250); + + this.idMap.set(connection.id, { strategy: connection.strategy, hasConfig: false }); + const scimConfiguration = await this.getScimConfiguration({ id: connection.id }); + if (!scimConfiguration) continue; + + this.idMap.set(connection.id, { ...this.idMap.get(connection.id)!, hasConfig: true }); } } @@ -81,30 +93,26 @@ export default class ScimHandler { */ async applyScimConfiguration(connections: Asset[]) { for (let connection of connections) { + if (!this.scimScopes.read) return connections; if (!this.isScimStrategy(connection.strategy)) continue; + + // To avoid rate limiter error, we making API requests with a small delay. + // TODO: However, this logic needs to be re-worked. + await sleep(250); - try { - const { user_id_attribute, mapping } = await this.getScimConfiguration({ id: connection.id }); - connection.scim_configuration = { user_id_attribute, mapping } - - // To avoid rate limiter error, we making API requests with a small delay. - // TODO: However, this logic needs to be re-worked. - await sleep(500); - } catch (err) { - // Skip the connection if it returns 404. This can happen if `SCIM` is not enabled on a `SCIM` connection. - if (err !== 'SCIM_NOT_FOUND') throw err; - - const warningMessage = `SCIM configuration not found on connection \"${connection.id}\".`; - log.warn(warningMessage); - } + const scimConfiguration = await this.getScimConfiguration({ id: connection.id }); + if (!scimConfiguration) continue; + + const { user_id_attribute, mapping } = scimConfiguration; + connection.scim_configuration = { user_id_attribute, mapping }; } } - + /** - * HTTP request wrapper on axios. - */ - private async scimHttpRequest(method: string, options: [string, ...Record[]]): Promise { - return await this.withErrorHandling(async () => { + * HTTP request wrapper on axios. + */ + private async scimHttpRequest(method: string, options: [string, ...Record[]]): Promise { + return await this.withErrorHandling(async () => { // @ts-ignore const accessToken = await this.tokenProvider?.getAccessToken(); const headers = { @@ -112,20 +120,49 @@ export default class ScimHandler { 'Authorization': `Bearer ${ accessToken }` } options = [...options, { headers }]; - return await axios[method](...options); - }); + }, method, options[0]); } /** * Error handler wrapper. */ - async withErrorHandling(callback) { + async withErrorHandling(callback, method: string, url: string) { try { return await callback(); } catch (error) { + // Extract connection_id from the url. + const regex = /api\/v2\/connections\/(.*?)\/scim-configuration/; + const match = url.match(regex); + const connectionId = match ? match[1] : null; + + // Extract error data const errorData = error?.response?.data; - if (errorData?.statusCode === 404) throw "SCIM_NOT_FOUND"; + + // Skip the connection if it returns 404. This can happen if `SCIM` is not enabled on a `SCIM` connection. + if (errorData?.statusCode === 404) { + const warningMessage = `SCIM configuration is not enabled on connection \"${ connectionId }\".`; + log.warn(warningMessage); + return { data: null }; + }; + + // Skip the connection if it returns 403. Looks like "scim_config" permissions are not enabled on Management API. + if (errorData?.statusCode === 403) { + const scope = this.scopeMethodMap[method]; + this.scimScopes[scope] = false; + const warningMessage = `Insufficient scope, "${ scope }:scim_config". Looks like "scim_config" permissions are not enabled your application.`; + log.warn(warningMessage); + return { data: null }; + } + + // Skip the connection if it returns 400. This can happen if `SCIM` configuration already exists on a `SCIM` connection. + // When `read:scim_config` is disabled and `create:scim_config` is enabled, we cannot check if `SCIM` configuration exists on a connection. + // So, it'll run into 400 error. + if (errorData?.statusCode === 400 && errorData?.message?.includes('already exists')) { + const warningMessage = `SCIM configuration already exists on connection \"${ connectionId }\".`; + log.warn(warningMessage); + return { data: null }; + } const statusCode = errorData?.statusCode || error?.response?.status; const errorCode = errorData?.errorCode || errorData?.error || error?.response?.statusText; @@ -149,7 +186,7 @@ export default class ScimHandler { * Creates a new `SCIM` configuration. */ async createScimConfiguration({ id: connection_id }: scimRequestParams, { user_id_attribute, mapping }: scimBodyParams): Promise { - log.debug(`Creating SCIM configuration for connection ${ connection_id }`); + log.debug(`Creating SCIM configuration on connection ${ connection_id }`); const url = this.getScimEndpoint(connection_id); return (await this.scimHttpRequest('post', [ url, { user_id_attribute, mapping } ])).data; } @@ -176,7 +213,7 @@ export default class ScimHandler { * Deletes an existing `SCIM` configuration. */ async deleteScimConfiguration({ id: connection_id }: scimRequestParams): Promise { - log.debug(`Deleting SCIM configuration of connection ${ connection_id }`); + log.debug(`Deleting SCIM configuration on connection ${ connection_id }`); const url = this.getScimEndpoint(connection_id); return (await this.scimHttpRequest('delete', [ url ])).data; } @@ -190,6 +227,7 @@ export default class ScimHandler { // First, update `connections`. const updated = await this.connectionsManager.update(requestParams, bodyParams); const idMapEntry = this.idMap.get(requestParams.id); + this.completedChanges ++; // Now, update `scim_configuration` inside the updated connection. // If `scim_configuration` exists in both local and remote -> updateScimConfiguration(...) @@ -197,17 +235,21 @@ export default class ScimHandler { // If `scim_configuration` exists in local but remote -> createScimConfiguration(...) if (idMapEntry?.hasConfig) { if (scimBodyParams) { - await this.updateScimConfiguration(requestParams, scimBodyParams); + this.updateQueue.push({action: 'update', requestParams, scimBodyParams}); } else { if (this.config('AUTH0_ALLOW_DELETE')) { - log.warn(`Deleting scim_configuration on connection ${ requestParams.id }.`); - await this.deleteScimConfiguration(requestParams); + this.updateQueue.push({action: 'delete', requestParams}); } else { log.warn('Skipping DELETE scim_configuration. Enable deletes by setting AUTH0_ALLOW_DELETE to true in your config.'); } } } else if (scimBodyParams) { - await this.createScimConfiguration(requestParams, scimBodyParams); + this.updateQueue.push({action: 'create', requestParams, scimBodyParams}); + } + + // Execute the queue. + if (this.completedChanges >= this.expectedChanges) { + await this.executeQueue(); } // Return response from connections.update(...). @@ -222,12 +264,56 @@ export default class ScimHandler { // First, create the new `connection`. const created = await this.connectionsManager.create(bodyParams); + this.completedChanges ++; + if (scimBodyParams) { // Now, create the `scim_configuration` for newly created `connection`. - await this.createScimConfiguration({ id: created.id }, scimBodyParams); + this.updateQueue.push({action: 'create', requestParams: {id: created.id}, scimBodyParams}); + } + + // Execute the queue. + if (this.completedChanges >= this.expectedChanges) { + await this.executeQueue(); } // Return response from connections.create(...). return created; } -} \ No newline at end of file + + /** + * If we perform `connectionsManager.update` and `updateScimConfiguration` together, they may result in rate limit error. + * The reason is that both of them make API requests to the same `api/v2/connections` endpoint. + * We cannot control it with delay because both `updateOverride` and `createOverride` are async functions. And being called concurrently by `PromisePoolExecutor`. + * To avoid this, we are queuing the `SCIM` actions and executing them one by one separately, only after `connectionsManager.update` is completed. + * + * This is true for both `create` and `update` actions. + * @returns {Promise} + */ + async executeQueue() { + if (this.isExecuting) return; + + this.isExecuting = true; + while (this.updateQueue.length > 0) { + // Rate limit error handling + await sleep(250); + const { action, requestParams, scimBodyParams } = this.updateQueue.shift(); + + switch (action) { + case 'create': + if (this.scimScopes.create) await this.createScimConfiguration(requestParams, scimBodyParams); + break; + case 'update': + if (this.scimScopes.update) await this.updateScimConfiguration(requestParams, scimBodyParams); + break; + case 'delete': + if (this.scimScopes.delete) await this.deleteScimConfiguration(requestParams); + break; + } + } + + this.isExecuting = false; + this.expectedChanges = 0; + this.completedChanges = 0; + } +} + diff --git a/test/tools/auth0/handlers/scimHandler.tests.js b/test/tools/auth0/handlers/scimHandler.tests.js index db98afcc..96592ddd 100644 --- a/test/tools/auth0/handlers/scimHandler.tests.js +++ b/test/tools/auth0/handlers/scimHandler.tests.js @@ -46,7 +46,7 @@ describe('ScimHandler', () => { const connections = [ { id: 'con_KYp633cmKtnEQ31C', strategy: 'samlp' }, // SCIM connection. { id: 'con_Njd1bxE3QTqTRwAk', strategy: 'auth0' }, // Non-SCIM connection. - { id: 'con_d3tmuoAkaUQgxN1f', strategy: 'gmail' } // Connection which doesn't exist. + { id: 'con_d3tmuoAkaUQgxN1f', strategy: 'gmail' }, // Connection which doesn't exist. ]; const getScimConfigurationStub = sinon.stub(scimHandler, 'getScimConfiguration'); getScimConfigurationStub.withArgs({ id: 'con_KYp633cmKtnEQ31C' }).resolves({ user_id_attribute: 'externalId-115', mapping: [{ auth0: 'auth0_key', scim: 'scim_key' }] }); @@ -77,7 +77,7 @@ describe('ScimHandler', () => { const getScimConfigurationStub = sinon.stub(scimHandler, 'getScimConfiguration'); getScimConfigurationStub.withArgs({ id: 'con_KYp633cmKtnEQ31C' }).resolves({ user_id_attribute: 'externalId-1', mapping: [{ auth0: 'auth0_key', scim: 'scim_key' }] }); getScimConfigurationStub.withArgs({ id: 'con_Njd1bxE3QTqTRwAk' }).resolves({ user_id_attribute: 'externalId-2', mapping: [{ auth0: 'auth0_key', scim: 'scim_key' }] }); - getScimConfigurationStub.withArgs({ id: 'con_d3tmuoAkaUQgxN1f' }).rejects({ response: { status: 404 } }); + getScimConfigurationStub.withArgs({ id: 'con_d3tmuoAkaUQgxN1f' }).rejects({ response: { data: { statusCode: 404 } } }); await scimHandler.applyScimConfiguration(connections); @@ -132,6 +132,7 @@ describe('ScimHandler', () => { const axiosStub = sinon.stub(axios, 'get').resolves({ data: scimConfiguration, status: 201 }); const response = await scimHandler.getScimConfiguration(requestParams); + // eslint-disable-next-line no-unused-expressions expect(response).to.deep.equal(scimConfiguration); axiosStub.restore(); @@ -139,17 +140,29 @@ describe('ScimHandler', () => { it('should throw error for non-existing SCIM configuration', async () => { const requestParams = { id: 'con_KYp633cmKtnEQ31C' }; - const axiosStub = sinon.stub(axios, 'get').rejects({ response: { status: 404, errorCode: 'not-found', statusText: 'The connection does not exist' } }); + const axiosStub = sinon.stub(axios, 'get').rejects({ response: { status: 404, errorCode: 'not_found', statusText: 'The connection does not exist.' } }); try { await scimHandler.getScimConfiguration(requestParams); expect.fail('Expected getScimConfiguration to throw an error'); } catch (error) { + // eslint-disable-next-line no-unused-expressions expect(error.response.status).to.equal(404); } axiosStub.restore(); }); + + it('should not throw error for scim connections when SCIM permissions disabled.', async () => { + const requestParams = { id: 'con_KYp633cmKtnEQ31C' }; + const axiosStub = sinon.stub(axios, 'get').rejects({ response: { data: { statusCode: 403, errorCode: 'insufficient_scope', statusText: 'Insufficient scope, expected any of: read:scim_config.' } } }); + + const data = await scimHandler.getScimConfiguration(requestParams); + // eslint-disable-next-line no-unused-expressions + expect(data).to.be.null; + + axiosStub.restore(); + }); }); describe('#createScimConfiguration', () => {