From 2430079f78de002c079d9e98e96eeda3b15f42bc Mon Sep 17 00:00:00 2001 From: kushalshit27 <43465488+kushalshit27@users.noreply.github.com> Date: Wed, 7 May 2025 12:21:41 +0530 Subject: [PATCH 1/5] fix: enhance error handling in guardian handlers for forbidden deprecated feature --- .../auth0/handlers/guardianFactorProviders.ts | 50 ++++++++++++------- .../auth0/handlers/guardianFactorTemplates.ts | 39 +++++++++------ src/tools/auth0/handlers/guardianFactors.ts | 22 ++++++-- .../guardianPhoneFactorMessageTypes.ts | 18 ++++--- .../guardianPhoneFactorSelectedProvider.ts | 18 ++++--- src/tools/utils.ts | 8 +++ 6 files changed, 102 insertions(+), 53 deletions(-) diff --git a/src/tools/auth0/handlers/guardianFactorProviders.ts b/src/tools/auth0/handlers/guardianFactorProviders.ts index a6c7c152c..d2e5a9d60 100644 --- a/src/tools/auth0/handlers/guardianFactorProviders.ts +++ b/src/tools/auth0/handlers/guardianFactorProviders.ts @@ -1,6 +1,7 @@ import DefaultHandler from './default'; import constants from '../../constants'; import { Asset, Assets } from '../../../types'; +import { isForbiddenFeatureError } from '../../utils'; const mappings = Object.entries(constants.GUARDIAN_FACTOR_PROVIDERS).reduce( (accum: { name: string; provider: string }[], [name, providers]) => { @@ -35,29 +36,40 @@ export default class GuardianFactorProvidersHandler extends DefaultHandler { }); } - async getType(): Promise { + async getType(): Promise { if (this.existing) return this.existing; - const data = await Promise.all( - mappings.map(async (m) => { - let provider; - // TODO: This is quite a change, needs to be validated for sure. - if (m.name === 'phone' && m.provider === 'twilio') { - provider = await this.client.guardian.getPhoneFactorProviderTwilio(); - } else if (m.name === 'sms' && m.provider === 'twilio') { - provider = await this.client.guardian.getSmsFactorProviderTwilio(); - } else if (m.name === 'push-notification' && m.provider === 'apns') { - provider = await this.client.guardian.getPushNotificationProviderAPNS(); - } else if (m.name === 'push-notification' && m.provider === 'sns') { - provider = await this.client.guardian.getPushNotificationProviderSNS(); - } + try { + const data = await Promise.all( + mappings.map(async (m) => { + let provider; + // TODO: This is quite a change, needs to be validated for sure. + if (m.name === 'phone' && m.provider === 'twilio') { + provider = await this.client.guardian.getPhoneFactorProviderTwilio(); + } else if (m.name === 'sms' && m.provider === 'twilio') { + provider = await this.client.guardian.getSmsFactorProviderTwilio(); + } else if (m.name === 'push-notification' && m.provider === 'apns') { + provider = await this.client.guardian.getPushNotificationProviderAPNS(); + } else if (m.name === 'push-notification' && m.provider === 'sns') { + provider = await this.client.guardian.getPushNotificationProviderSNS(); + } - return { ...m, ...provider.data }; - }) - ); + return { ...m, ...provider.data }; + }) + ); + + // Filter out empty, should have more then 2 keys (name, provider) + return data.filter((d) => Object.keys(d).length > 2); + } catch (err) { + if (err.statusCode === 404 || err.statusCode === 501) { + return null; + } + if (isForbiddenFeatureError(err, this.type)) { + return null; + } - // Filter out empty, should have more then 2 keys (name, provider) - return data.filter((d) => Object.keys(d).length > 2); + throw err; + } } async processChanges(assets: Assets): Promise { diff --git a/src/tools/auth0/handlers/guardianFactorTemplates.ts b/src/tools/auth0/handlers/guardianFactorTemplates.ts index 3c8267222..ffef826b5 100644 --- a/src/tools/auth0/handlers/guardianFactorTemplates.ts +++ b/src/tools/auth0/handlers/guardianFactorTemplates.ts @@ -1,7 +1,8 @@ +import { TemplateMessages } from 'auth0'; import DefaultHandler from './default'; import constants from '../../constants'; import { Assets, Asset } from '../../../types'; -import { TemplateMessages } from 'auth0'; +import { isForbiddenFeatureError } from '../../utils'; export const schema = { type: 'array', @@ -25,25 +26,33 @@ export default class GuardianFactorTemplatesHandler extends DefaultHandler { }); } - async getType(): Promise { + async getType(): Promise { if (this.existing) return this.existing; + try { + const data = await Promise.all( + constants.GUARDIAN_FACTOR_TEMPLATES.map(async (name) => { + if (name === 'sms') { + const { data: templates } = await this.client.guardian.getSmsFactorTemplates(); + return { name, ...templates }; + } - const data = await Promise.all( - constants.GUARDIAN_FACTOR_TEMPLATES.map(async (name) => { - // TODO: This is quite a change, needs to be validated for sure. - if (name === 'sms') { - const { data: templates } = await this.client.guardian.getSmsFactorTemplates(); - return { name, ...templates }; - // TODO: GUARDIAN_FACTOR_TEMPLATES only contains 'sms'. Is that expected? We also have 'phone'. - } else { const { data: templates } = await this.client.guardian.getPhoneFactorTemplates(); return { name, ...templates }; - } - }) - ); + }) + ); + + // Filter out empty, should have more then 1 keys (name) + return data.filter((d) => Object.keys(d).length > 1); + } catch (err) { + if (err.statusCode === 404 || err.statusCode === 501) { + return null; + } + if (isForbiddenFeatureError(err, this.type)) { + return null; + } - // Filter out empty, should have more then 1 keys (name) - return data.filter((d) => Object.keys(d).length > 1); + throw err; + } } async processChanges(assets: Assets): Promise { diff --git a/src/tools/auth0/handlers/guardianFactors.ts b/src/tools/auth0/handlers/guardianFactors.ts index 53e370310..aa2b9a153 100644 --- a/src/tools/auth0/handlers/guardianFactors.ts +++ b/src/tools/auth0/handlers/guardianFactors.ts @@ -1,7 +1,8 @@ +import { Factor, FactorNameEnum } from 'auth0'; import DefaultHandler from './default'; import constants from '../../constants'; import { Asset, Assets } from '../../../types'; -import { Factor, FactorNameEnum } from 'auth0'; +import { isForbiddenFeatureError } from '../../utils'; export const schema = { type: 'array', @@ -25,11 +26,22 @@ export default class GuardianFactorsHandler extends DefaultHandler { }); } - async getType(): Promise { + async getType(): Promise { if (this.existing) return this.existing; - const { data } = await this.client.guardian.getFactors(); - this.existing = data; - return this.existing; + try { + const { data } = await this.client.guardian.getFactors(); + this.existing = data; + return this.existing; + } catch (err) { + if (err.statusCode === 404 || err.statusCode === 501) { + return null; + } + if (isForbiddenFeatureError(err, this.type)) { + return null; + } + + throw err; + } } async processChanges(assets: Assets): Promise { diff --git a/src/tools/auth0/handlers/guardianPhoneFactorMessageTypes.ts b/src/tools/auth0/handlers/guardianPhoneFactorMessageTypes.ts index 45c1f032f..639e2ec6c 100644 --- a/src/tools/auth0/handlers/guardianPhoneFactorMessageTypes.ts +++ b/src/tools/auth0/handlers/guardianPhoneFactorMessageTypes.ts @@ -1,7 +1,8 @@ +import { GetMessageTypes200Response } from 'auth0'; import DefaultHandler from './default'; import constants from '../../constants'; import { Asset, Assets } from '../../../types'; -import { GetMessageTypes200Response } from 'auth0'; +import { isForbiddenFeatureError } from '../../utils'; export const schema = { type: 'object', @@ -45,13 +46,13 @@ export default class GuardianPhoneMessageTypesHandler extends DefaultHandler { }); } - async getType(): Promise { + async getType(): Promise { // in case client version does not support the operation if ( !this.client.guardian || typeof this.client.guardian.getPhoneFactorMessageTypes !== 'function' ) { - return {}; + return null; } if (this.existing) return this.existing; @@ -59,12 +60,15 @@ export default class GuardianPhoneMessageTypesHandler extends DefaultHandler { try { const { data } = await this.client.guardian.getPhoneFactorMessageTypes(); this.existing = data; - } catch (e) { - if (isFeatureUnavailableError(e)) { + } catch (err) { + if (isFeatureUnavailableError(err)) { // Gracefully skip processing this configuration value. - return {}; + return null; + } + if (isForbiddenFeatureError(err, this.type)) { + return null; } - throw e; + throw err; } return this.existing; diff --git a/src/tools/auth0/handlers/guardianPhoneFactorSelectedProvider.ts b/src/tools/auth0/handlers/guardianPhoneFactorSelectedProvider.ts index 6d98cefb7..cb1197a2c 100644 --- a/src/tools/auth0/handlers/guardianPhoneFactorSelectedProvider.ts +++ b/src/tools/auth0/handlers/guardianPhoneFactorSelectedProvider.ts @@ -1,7 +1,8 @@ +import { GetPhoneProviders200Response } from 'auth0'; import DefaultHandler from './default'; import constants from '../../constants'; import { Asset, Assets } from '../../../types'; -import { GetPhoneProviders200Response } from 'auth0'; +import { isForbiddenFeatureError } from '../../utils'; export const schema = { type: 'object', @@ -42,13 +43,13 @@ export default class GuardianPhoneSelectedProviderHandler extends DefaultHandler }); } - async getType() { + async getType(): Promise { // in case client version does not support the operation if ( !this.client.guardian || typeof this.client.guardian.getPhoneFactorSelectedProvider !== 'function' ) { - return {}; + return null; } if (this.existing) return this.existing; @@ -56,12 +57,15 @@ export default class GuardianPhoneSelectedProviderHandler extends DefaultHandler try { const { data } = await this.client.guardian.getPhoneFactorSelectedProvider(); this.existing = data; - } catch (e) { - if (isFeatureUnavailableError(e)) { + } catch (err) { + if (isFeatureUnavailableError(err)) { // Gracefully skip processing this configuration value. - return {}; + return null; + } + if (isForbiddenFeatureError(err, this.type)) { + return null; } - throw e; + throw err; } return this.existing; diff --git a/src/tools/utils.ts b/src/tools/utils.ts index 787b05c2d..cd6429a93 100644 --- a/src/tools/utils.ts +++ b/src/tools/utils.ts @@ -275,3 +275,11 @@ export const isDeprecatedError = (err: { message: string; statusCode: number }): if (!err) return false; return !!(err.statusCode === 403 || err.message?.includes('deprecated feature')); }; + +export const isForbiddenFeatureError = (err, type): boolean => { + if (err.statusCode === 403) { + log.warn(`${err.message};${err.errorCode ?? ''} - Skipping ${type}`); + return true; + } + return false; +}; From bd6524888d90af2c96cabad7afbd250519ce6f07 Mon Sep 17 00:00:00 2001 From: kushalshit27 <43465488+kushalshit27@users.noreply.github.com> Date: Wed, 7 May 2025 13:32:50 +0530 Subject: [PATCH 2/5] fix: update tests to expect null instead of empty object for guardian phone factor handlers --- .../handlers/guardianPhoneFactorMessageTypes.tests.js | 8 ++++---- .../handlers/guardianPhoneFactorSelectedProvider.tests.js | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/test/tools/auth0/handlers/guardianPhoneFactorMessageTypes.tests.js b/test/tools/auth0/handlers/guardianPhoneFactorMessageTypes.tests.js index ada7a506c..861547260 100644 --- a/test/tools/auth0/handlers/guardianPhoneFactorMessageTypes.tests.js +++ b/test/tools/auth0/handlers/guardianPhoneFactorMessageTypes.tests.js @@ -12,7 +12,7 @@ describe('#guardianPhoneFactorMessageTypes handler', () => { const handler = new guardianPhoneFactorMessageTypes.default({ client: auth0 }); const data = await handler.getType(); - expect(data).to.deep.equal({}); + expect(data).to.deep.equal(null); }); it('should support when endpoint does not exist (older installations)', async () => { @@ -42,7 +42,7 @@ describe('#guardianPhoneFactorMessageTypes handler', () => { const handler = new guardianPhoneFactorMessageTypes.default({ client: auth0 }); const data = await handler.getType(); - expect(data).to.deep.equal({}); + expect(data).to.deep.equal(null); }); it('should support when endpoint is disabled for tenant', async () => { @@ -73,7 +73,7 @@ describe('#guardianPhoneFactorMessageTypes handler', () => { const handler = new guardianPhoneFactorMessageTypes.default({ client: auth0 }); const data = await handler.getType(); - expect(data).to.deep.equal({}); + expect(data).to.deep.equal(null); }); it('should get guardian phone factor message types', async () => { @@ -140,7 +140,7 @@ describe('#guardianPhoneFactorMessageTypes handler', () => { const handler = new guardianPhoneFactorMessageTypes.default({ client: auth0 }); const stageFn = Object.getPrototypeOf(handler).processChanges; - await stageFn.apply(handler, [{ guardianPhoneFactorMessageTypes: {} }]); + await stageFn.apply(handler, [{ guardianPhoneFactorMessageTypes: null }]); }); }); }); diff --git a/test/tools/auth0/handlers/guardianPhoneFactorSelectedProvider.tests.js b/test/tools/auth0/handlers/guardianPhoneFactorSelectedProvider.tests.js index 68ff576de..cff0d015d 100644 --- a/test/tools/auth0/handlers/guardianPhoneFactorSelectedProvider.tests.js +++ b/test/tools/auth0/handlers/guardianPhoneFactorSelectedProvider.tests.js @@ -12,7 +12,7 @@ describe('#guardianPhoneFactorSelectedProvider handler', () => { const handler = new guardianPhoneFactorSelectedProvider.default({ client: auth0 }); const data = await handler.getType(); - expect(data).to.deep.equal({}); + expect(data).to.deep.equal(null); }); it('should support when endpoint does not exist (older installations)', async () => { @@ -42,7 +42,7 @@ describe('#guardianPhoneFactorSelectedProvider handler', () => { const handler = new guardianPhoneFactorSelectedProvider.default({ client: auth0 }); const data = await handler.getType(); - expect(data).to.deep.equal({}); + expect(data).to.deep.equal(null); }); it('should support when endpoint is disabled for tenant', async () => { @@ -73,7 +73,7 @@ describe('#guardianPhoneFactorSelectedProvider handler', () => { const handler = new guardianPhoneFactorSelectedProvider.default({ client: auth0 }); const data = await handler.getType(); - expect(data).to.deep.equal({}); + expect(data).to.deep.equal(null); }); it('should get guardian phone factor selected provider', async () => { @@ -142,7 +142,7 @@ describe('#guardianPhoneFactorSelectedProvider handler', () => { const handler = new guardianPhoneFactorSelectedProvider.default({ client: auth0 }); const stageFn = Object.getPrototypeOf(handler).processChanges; - await stageFn.apply(handler, [{ guardianPhoneFactorSelectedProvider: {} }]); + await stageFn.apply(handler, [{ guardianPhoneFactorSelectedProvider: null }]); }); }); }); From a2a648d02c4d2a78ee4bb3e4a87a243522f0b5e6 Mon Sep 17 00:00:00 2001 From: kushalshit27 <43465488+kushalshit27@users.noreply.github.com> Date: Wed, 7 May 2025 14:38:57 +0530 Subject: [PATCH 3/5] test: add unit tests for isForbiddenFeatureError utility function --- test/tools/utils.test.js | 75 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 68 insertions(+), 7 deletions(-) diff --git a/test/tools/utils.test.js b/test/tools/utils.test.js index 9af0b02ab..99a415966 100644 --- a/test/tools/utils.test.js +++ b/test/tools/utils.test.js @@ -4,6 +4,7 @@ import { expect } from 'chai'; import jsYaml from 'js-yaml'; import * as utils from '../../src/tools/utils'; import constants from '../../src/tools/constants'; +import log from '../../src/logger'; const mappings = { string: 'some string', @@ -246,10 +247,10 @@ describe('#keywordReplacement', () => { STRING_REPLACEMENT: 'string-replace-value', ARRAY_REPLACEMENT: ['##STRING_REPLACEMENT##', 'other-array-replace-value'], }; - const inputJSON = `{ - "arrayReplace": "@@ARRAY_REPLACEMENT@@", - "stringReplace": "##STRING_REPLACEMENT##", - "noReplace": "NO_REPLACEMENT" + const inputJSON = `{ + "arrayReplace": "@@ARRAY_REPLACEMENT@@", + "stringReplace": "##STRING_REPLACEMENT##", + "noReplace": "NO_REPLACEMENT" }`; const output = utils.keywordReplace(inputJSON, mapping); @@ -274,8 +275,8 @@ describe('#keywordReplacement', () => { propertyShouldNotStringReplace: 'this should not be replaced', }, }; - const inputJSON = `{ - "stringReplace": "##STRING_REPLACEMENT##", + const inputJSON = `{ + "stringReplace": "##STRING_REPLACEMENT##", "noReplace": "NO_REPLACEMENT", "objectReplace": "@@OBJECT_REPLACEMENT@@" }`; @@ -303,7 +304,7 @@ describe('#keywordReplacement', () => { GLOBAL_WEB_ORIGINS: "\"http://local.me:8080\", \"http://localhost\", \"http://localhost:3000\"", // eslint-disable-line }; - const inputJSON = `{ + const inputJSON = `{ "web_origins": [ ##GLOBAL_WEB_ORIGINS##, "http://production-app.com", @@ -650,3 +651,63 @@ describe('#isDeprecatedError', () => { expect(utils.isDeprecatedError({})).to.be.false; }); }); + +describe('#isForbiddenFeatureError', () => { + it('should return true and log warning for 403 status code', () => { + let warnMessage; + const originalWarn = log.warn; + // Mock the log.warn function + log.warn = (msg) => { + warnMessage = msg; + }; + + const error = { + message: 'Forbidden resource access', + statusCode: 403, + }; + const resourceType = 'connections'; + + // eslint-disable-next-line no-unused-expressions + expect(utils.isForbiddenFeatureError(error, resourceType)).to.be.true; + expect(warnMessage).to.equal('Forbidden resource access; - Skipping connections'); + + // Restore original warn function + log.warn = originalWarn; + }); + + it('should include errorCode in log message when available', () => { + let warnMessage; + const originalWarn = log.warn; + // Mock the log.warn function + log.warn = (msg) => { + warnMessage = msg; + }; + + const error = { + message: 'Forbidden resource access', + statusCode: 403, + errorCode: 'forbidden_resource_error', + }; + const resourceType = 'actions'; + + // eslint-disable-next-line no-unused-expressions + expect(utils.isForbiddenFeatureError(error, resourceType)).to.be.true; + expect(warnMessage).to.equal( + 'Forbidden resource access;forbidden_resource_error - Skipping actions' + ); + + // Restore original warn function + log.warn = originalWarn; + }); + + it('should return false for non-403 status code', () => { + const error = { + message: 'Not found', + statusCode: 404, + }; + const resourceType = 'rules'; + + // eslint-disable-next-line no-unused-expressions + expect(utils.isForbiddenFeatureError(error, resourceType)).to.be.false; + }); +}); From 1549c90f0c3f6510d6574fcb1041f484039c9148 Mon Sep 17 00:00:00 2001 From: kushalshit27 <43465488+kushalshit27@users.noreply.github.com> Date: Wed, 7 May 2025 15:08:08 +0530 Subject: [PATCH 4/5] test: add handling for forbidden errors in guardian factor and template tests --- .../handlers/guardianFactorProviders.tests.js | 22 +++++++++++++++++++ .../handlers/guardianFactorTemplates.tests.js | 17 ++++++++++++++ .../auth0/handlers/guardianFactors.tests.js | 17 ++++++++++++++ 3 files changed, 56 insertions(+) diff --git a/test/tools/auth0/handlers/guardianFactorProviders.tests.js b/test/tools/auth0/handlers/guardianFactorProviders.tests.js index ca807d4f5..4d74b07ed 100644 --- a/test/tools/auth0/handlers/guardianFactorProviders.tests.js +++ b/test/tools/auth0/handlers/guardianFactorProviders.tests.js @@ -59,6 +59,28 @@ describe('#guardianFactorProviders handler', () => { }); describe('#guardianFactorProviders process', () => { + it('should handle forbidden error', async () => { + const throwForbidden = () => { + const error = new Error('Forbidden resource access'); + error.statusCode = 403; + throw error; + }; + + const auth0 = { + guardian: { + getPhoneFactorProviderTwilio: throwForbidden, + getSmsFactorProviderTwilio: throwForbidden, + getPushNotificationProviderAPNS: throwForbidden, + getPushNotificationProviderSNS: throwForbidden + }, + pool, + }; + + const handler = new guardianFactorProvidersTests.default({ client: auth0, config }); + const data = await handler.getType(); + expect(data).to.equal(null); + }); + it('should get guardianFactorProviders', async () => { const auth0 = { guardian: { diff --git a/test/tools/auth0/handlers/guardianFactorTemplates.tests.js b/test/tools/auth0/handlers/guardianFactorTemplates.tests.js index 0fb3f3631..fc2a9bc7f 100644 --- a/test/tools/auth0/handlers/guardianFactorTemplates.tests.js +++ b/test/tools/auth0/handlers/guardianFactorTemplates.tests.js @@ -56,6 +56,23 @@ describe('#guardianFactorTemplates handler', () => { }); describe('#guardianFactorTemplates process', () => { + it('should handle forbidden error', async () => { + const auth0 = { + guardian: { + getSmsFactorTemplates: () => { + const error = new Error('Forbidden resource access'); + error.statusCode = 403; + throw error; + }, + }, + pool, + }; + + const handler = new guardianFactorTemplatesTests.default({ client: auth0, config }); + const data = await handler.getType(); + expect(data).to.equal(null); + }); + it('should get guardianFactorTemplates', async () => { const auth0 = { guardian: { diff --git a/test/tools/auth0/handlers/guardianFactors.tests.js b/test/tools/auth0/handlers/guardianFactors.tests.js index 5ff88161b..a9f097932 100644 --- a/test/tools/auth0/handlers/guardianFactors.tests.js +++ b/test/tools/auth0/handlers/guardianFactors.tests.js @@ -55,6 +55,23 @@ describe('#guardianFactors handler', () => { }); describe('#guardianFactors process', () => { + it('should handle forbidden error', async () => { + const auth0 = { + guardian: { + getFactors: () => { + const error = new Error('Forbidden resource access'); + error.statusCode = 403; + throw error; + }, + }, + pool, + }; + + const handler = new guardianFactorsTests.default({ client: auth0, config }); + const data = await handler.getType(); + expect(data).to.equal(null); + }); + it('should get guardianFactors', async () => { const factors = [ { name: 'sms', enabled: true }, From 6b2bec5809e3f0cb61b8fc69c08ba234f8984a29 Mon Sep 17 00:00:00 2001 From: kushalshit27 <43465488+kushalshit27@users.noreply.github.com> Date: Wed, 7 May 2025 15:08:49 +0530 Subject: [PATCH 5/5] test: add handling for forbidden errors in guardian factor and template tests --- test/tools/auth0/handlers/guardianFactorProviders.tests.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/tools/auth0/handlers/guardianFactorProviders.tests.js b/test/tools/auth0/handlers/guardianFactorProviders.tests.js index 4d74b07ed..c607f3c85 100644 --- a/test/tools/auth0/handlers/guardianFactorProviders.tests.js +++ b/test/tools/auth0/handlers/guardianFactorProviders.tests.js @@ -71,7 +71,7 @@ describe('#guardianFactorProviders handler', () => { getPhoneFactorProviderTwilio: throwForbidden, getSmsFactorProviderTwilio: throwForbidden, getPushNotificationProviderAPNS: throwForbidden, - getPushNotificationProviderSNS: throwForbidden + getPushNotificationProviderSNS: throwForbidden, }, pool, };