From e1e6698e83128b86924affae97b7e3ab78dd4421 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Fri, 25 Mar 2022 19:06:33 -0300 Subject: [PATCH 1/5] implementation and tests --- package-lock.json | 8 ++-- package.json | 2 +- .../__tests__/sdkClientMethodCS.spec.ts | 25 ------------- src/sdkClient/sdkClientMethodCS.ts | 7 +--- src/sdkClient/sdkClientMethodCSWithTT.ts | 13 +------ .../__tests__/index.spec.ts | 37 +++++++++++++++++++ src/utils/settingsValidation/index.ts | 24 ++++++++++-- src/utils/settingsValidation/types.ts | 4 +- 8 files changed, 68 insertions(+), 52 deletions(-) diff --git a/package-lock.json b/package-lock.json index 0a014819..46d193a6 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "@splitsoftware/splitio-commons", - "version": "1.2.1-rc.8", + "version": "1.2.1-rc.9", "lockfileVersion": 1, "requires": true, "dependencies": { @@ -4984,9 +4984,9 @@ } }, "minimist": { - "version": "1.2.5", - "resolved": "https://registry.npmjs.org/minimist/-/minimist-1.2.5.tgz", - "integrity": "sha512-FM9nNUYrRBAELZQT3xeZQ7fmMOBg6nWNmJKTcgsJeaLstP/UODVpGsr5OhXhhXg6f+qtJ8uiZ+PUxkDWcgIXLw==", + "version": "1.2.6", + "resolved": "https://registry.npmjs.org/minimist/-/minimist-1.2.6.tgz", + "integrity": "sha512-Jsjnk4bw3YJqYzbdyBiNsPWHPfO++UGG749Cxs6peCu5Xg4nrena6OVxOYxrQTqww0Jmwt+Ref8rggumkTLz9Q==", "dev": true }, "ms": { diff --git a/package.json b/package.json index 63b45dc7..609120dc 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@splitsoftware/splitio-commons", - "version": "1.2.1-rc.8", + "version": "1.2.1-rc.9", "description": "Split Javascript SDK common components", "main": "cjs/index.js", "module": "esm/index.js", diff --git a/src/sdkClient/__tests__/sdkClientMethodCS.spec.ts b/src/sdkClient/__tests__/sdkClientMethodCS.spec.ts index b5b6b21e..4e445d6d 100644 --- a/src/sdkClient/__tests__/sdkClientMethodCS.spec.ts +++ b/src/sdkClient/__tests__/sdkClientMethodCS.spec.ts @@ -220,31 +220,6 @@ describe('sdkClientMethodCSFactory', () => { if (!ignoresTT) expect(() => sdkClientMethod('valid-key', ['invalid-TT'])).toThrow('Shared Client needs a valid traffic type or no traffic type at all.'); }); - test.each(testTargets)('invalid key/TT binds a false key/TT in the default client', (sdkClientMethodCSFactory, ignoresTT) => { - const paramsWithInvalidKeyAndTT = { - ...params, - settings: { - ...params.settings, - core: { - key: true, // invalid key - trafficType: '' // invalid TT - } - } - }; - - (clientCSDecoratorSpy as jest.Mock).mockClear(); - // @ts-expect-error - const sdkClientMethod = sdkClientMethodCSFactory(paramsWithInvalidKeyAndTT); - - // calling the function should return a client instance - const client = sdkClientMethod(); - assertClientApi(client, params.sdkReadinessManager.sdkStatus); - - // but with false as binded key and TT - if (ignoresTT) expect(clientCSDecoratorSpy).toHaveBeenCalledWith(expect.anything(), expect.anything(), false); - else expect(clientCSDecoratorSpy).toHaveBeenCalledWith(expect.anything(), expect.anything(), false, false); - }); - test.each(testTargets)('attributes binding - main client', (sdkClientMethodCSFactory) => { // @ts-expect-error const sdkClientMethod = sdkClientMethodCSFactory(params); diff --git a/src/sdkClient/sdkClientMethodCS.ts b/src/sdkClient/sdkClientMethodCS.ts index 2abd7a20..32f6f3fb 100644 --- a/src/sdkClient/sdkClientMethodCS.ts +++ b/src/sdkClient/sdkClientMethodCS.ts @@ -23,15 +23,10 @@ const method = 'Client instantiation'; export function sdkClientMethodCSFactory(params: ISdkClientFactoryParams): (key?: SplitIO.SplitKey) => SplitIO.ICsClient { const { storage, syncManager, sdkReadinessManager, settings: { core: { key }, startup: { readyTimeout }, log } } = params; - // Keeping similar behaviour as in the isomorphic JS SDK: if settings key is invalid, - // `false` value is used as binded key of the default client, but trafficType is ignored - // @TODO handle as a non-recoverable error - const validKey = validateKey(log, key, method); - const mainClientInstance = clientCSDecorator( log, sdkClientFactory(params) as SplitIO.IClient, // @ts-ignore - validKey + key ); const parsedDefaultKey = keyParser(key); diff --git a/src/sdkClient/sdkClientMethodCSWithTT.ts b/src/sdkClient/sdkClientMethodCSWithTT.ts index 7c5427e2..22a49ac6 100644 --- a/src/sdkClient/sdkClientMethodCSWithTT.ts +++ b/src/sdkClient/sdkClientMethodCSWithTT.ts @@ -25,20 +25,11 @@ const method = 'Client instantiation'; export function sdkClientMethodCSFactory(params: ISdkClientFactoryParams): (key?: SplitIO.SplitKey, trafficType?: string) => SplitIO.ICsClient { const { storage, syncManager, sdkReadinessManager, settings: { core: { key, trafficType }, startup: { readyTimeout }, log } } = params; - // Keeping the behaviour as in the isomorphic JS SDK: if settings key or TT are invalid, - // `false` value is used as binded key/TT of the default client, which leads to several issues. - // @TODO update when supporting non-recoverable errors - const validKey = validateKey(log, key, method); - let validTrafficType; - if (trafficType !== undefined) { - validTrafficType = validateTrafficType(log, trafficType, method); - } - const mainClientInstance = clientCSDecorator( log, sdkClientFactory(params) as SplitIO.IClient, // @ts-ignore - validKey, - validTrafficType + key, + trafficType ); const parsedDefaultKey = keyParser(key); diff --git a/src/utils/settingsValidation/__tests__/index.spec.ts b/src/utils/settingsValidation/__tests__/index.spec.ts index 50b960a9..7f110c86 100644 --- a/src/utils/settingsValidation/__tests__/index.spec.ts +++ b/src/utils/settingsValidation/__tests__/index.spec.ts @@ -189,6 +189,43 @@ describe('settingsValidation', () => { expect(settings.integrations).toBe(integrationsValidatorResult); expect(integrationsValidatorMock).toBeCalledWith(settings); }); + + test('validates and sanitizes key and traffic type in client-side', () => { + const clientSideValidationParams = { ...minimalSettingsParams, isClientSide: true }; + + const samples = [{ + key: ' valid-key ', settingsKey: 'valid-key', // key string is trimmed + trafficType: 'VALID-TT', settingsTrafficType: 'valid-tt', // TT is converted to lowercase + }, { + key: undefined, settingsKey: false, // undefined key is not valid in client-side + trafficType: undefined, settingsTrafficType: undefined, + }, { + key: null, settingsKey: false, + trafficType: null, settingsTrafficType: false, + }, { + key: true, settingsKey: false, + trafficType: true, settingsTrafficType: false, + }, { + key: 1.5, settingsKey: '1.5', // finite number as key is parsed + trafficType: 100, settingsTrafficType: false, + }, { + key: { matchingKey: 100, bucketingKey: ' BUCK ' }, settingsKey: { matchingKey: '100', bucketingKey: 'BUCK' }, + trafficType: {}, settingsTrafficType: false, + }]; + + samples.forEach(({ key, trafficType, settingsKey, settingsTrafficType }) => { + const settings = settingsValidation({ + core: { + authorizationKey: 'dummy token', + key, + trafficType + } + }, clientSideValidationParams); + + expect(settings.core.key).toEqual(settingsKey); + expect(settings.core.trafficType).toEqual(settingsTrafficType); + }); + }); }); test('SETTINGS / urls should be correctly assigned', () => { diff --git a/src/utils/settingsValidation/index.ts b/src/utils/settingsValidation/index.ts index d7a31c10..c4de8fbc 100644 --- a/src/utils/settingsValidation/index.ts +++ b/src/utils/settingsValidation/index.ts @@ -5,6 +5,8 @@ import { STANDALONE_MODE, OPTIMIZED, LOCALHOST_MODE } from '../constants'; import { validImpressionsMode } from './impressionsMode'; import { ISettingsValidationParams } from './types'; import { ISettings } from '../../types'; +import { validateKey } from '../inputValidation/key'; +import { validateTrafficType } from '../inputValidation/trafficType'; const base = { // Define which kind of object you want to retrieve from SplitFactory @@ -97,7 +99,7 @@ function fromSecondsToMillis(n: number) { */ export function settingsValidation(config: unknown, validationParams: ISettingsValidationParams) { - const { defaults, runtime, storage, integrations, logger, localhost, consent } = validationParams; + const { defaults, isClientSide, runtime, storage, integrations, logger, localhost, consent } = validationParams; // creates a settings object merging base, defaults and config objects. const withDefaults = merge({}, base, defaults, config) as ISettings; @@ -129,9 +131,23 @@ export function settingsValidation(config: unknown, validationParams: ISettingsV // @ts-ignore, modify readonly prop if (storage) withDefaults.storage = storage(withDefaults); - // Although `key` is mandatory according to TS declaration files, it can be omitted in LOCALHOST mode. In that case, the value `localhost_key` is used. - if (withDefaults.mode === LOCALHOST_MODE && withDefaults.core.key === undefined) { - withDefaults.core.key = 'localhost_key'; + // In client-side, validate key and TT + if (isClientSide) { + const maybeKey = withDefaults.core.key; + // Although `key` is required in client-side, it can be omitted in LOCALHOST mode. In that case, the value `localhost_key` is used. + if (withDefaults.mode === LOCALHOST_MODE && maybeKey === undefined) { + withDefaults.core.key = 'localhost_key'; + } else { + // Keeping same behaviour than JS SDK: if settings key or TT are invalid, + // `false` value is used as binded key/TT of the default client, which leads to some issues. + // @ts-ignore, @TODO handle invalid keys as a non-recoverable error? + withDefaults.core.key = validateKey(log, maybeKey, 'Client instantiation'); + } + + const maybeTT = withDefaults.core.trafficType; + if (maybeTT !== undefined) { // @ts-ignore, assigning false + withDefaults.core.trafficType = validateTrafficType(log, maybeTT, 'Client instantiation'); + } } // Current ip/hostname information diff --git a/src/utils/settingsValidation/types.ts b/src/utils/settingsValidation/types.ts index 40cd155c..f6ead8e4 100644 --- a/src/utils/settingsValidation/types.ts +++ b/src/utils/settingsValidation/types.ts @@ -10,7 +10,9 @@ export interface ISettingsValidationParams { * Version and startup properties are required, because they are not defined in the base settings. */ defaults: Partial & { version: string } & { startup: ISettings['startup'] }, - /** Function to define runtime values (`settings.runtime`) */ + /** If true, validates core.key and core.trafficType */ + isClientSide?: boolean, + /** Define runtime values (`settings.runtime`) */ runtime: (settings: ISettings) => ISettings['runtime'], /** Storage validator (`settings.storage`) */ storage?: (settings: ISettings) => ISettings['storage'], From c5aee70c4a58512583587d520519854648fb321e Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Fri, 25 Mar 2022 19:13:08 -0300 Subject: [PATCH 2/5] fixed type issue --- src/sdkClient/__tests__/sdkClientMethodCS.spec.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/sdkClient/__tests__/sdkClientMethodCS.spec.ts b/src/sdkClient/__tests__/sdkClientMethodCS.spec.ts index 4e445d6d..7eabeb7c 100644 --- a/src/sdkClient/__tests__/sdkClientMethodCS.spec.ts +++ b/src/sdkClient/__tests__/sdkClientMethodCS.spec.ts @@ -2,10 +2,6 @@ import { sdkClientMethodCSFactory as sdkClientMethodCSWithTTFactory } from '../s import { sdkClientMethodCSFactory } from '../sdkClientMethodCS'; import { assertClientApi } from './testUtils'; -/** Mocks */ -import * as clientCS from '../clientCS'; -const clientCSDecoratorSpy = jest.spyOn(clientCS, 'clientCSDecorator'); - import { settingsWithKey, settingsWithKeyAndTT, settingsWithKeyObject } from '../../utils/settingsValidation/__tests__/settings.mocks'; const partialStorages: { destroy: jest.Mock }[] = []; From 5020990f796239893ddb40ba99cb9c7918be941b Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Tue, 29 Mar 2022 15:13:58 -0300 Subject: [PATCH 3/5] handle key and TT validation separately --- package-lock.json | 2 +- package.json | 2 +- .../settingsValidation/__tests__/index.spec.ts | 15 ++++++++++++++- src/utils/settingsValidation/index.ts | 14 ++++++++------ src/utils/settingsValidation/types.ts | 6 ++++-- 5 files changed, 28 insertions(+), 11 deletions(-) diff --git a/package-lock.json b/package-lock.json index 46d193a6..1e0f7f3d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "@splitsoftware/splitio-commons", - "version": "1.2.1-rc.9", + "version": "1.2.1-rc.10", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index 609120dc..c74fbf0e 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@splitsoftware/splitio-commons", - "version": "1.2.1-rc.9", + "version": "1.2.1-rc.10", "description": "Split Javascript SDK common components", "main": "cjs/index.js", "module": "esm/index.js", diff --git a/src/utils/settingsValidation/__tests__/index.spec.ts b/src/utils/settingsValidation/__tests__/index.spec.ts index 7f110c86..3f799970 100644 --- a/src/utils/settingsValidation/__tests__/index.spec.ts +++ b/src/utils/settingsValidation/__tests__/index.spec.ts @@ -191,7 +191,7 @@ describe('settingsValidation', () => { }); test('validates and sanitizes key and traffic type in client-side', () => { - const clientSideValidationParams = { ...minimalSettingsParams, isClientSide: true }; + const clientSideValidationParams = { ...minimalSettingsParams, acceptKey: true, acceptTT: true }; const samples = [{ key: ' valid-key ', settingsKey: 'valid-key', // key string is trimmed @@ -226,6 +226,19 @@ describe('settingsValidation', () => { expect(settings.core.trafficType).toEqual(settingsTrafficType); }); }); + + test('validates and sanitizes key, while traffic type is ignored', () => { + const settings = settingsValidation({ + core: { + authorizationKey: 'dummy token', + key: true, + trafficType: true + } + }, { ...minimalSettingsParams, acceptKey: true }); + + expect(settings.core.key).toEqual(false); // key is validated + expect(settings.core.trafficType).toEqual(true); // traffic type is ignored + }); }); test('SETTINGS / urls should be correctly assigned', () => { diff --git a/src/utils/settingsValidation/index.ts b/src/utils/settingsValidation/index.ts index c4de8fbc..5bb1125b 100644 --- a/src/utils/settingsValidation/index.ts +++ b/src/utils/settingsValidation/index.ts @@ -99,7 +99,7 @@ function fromSecondsToMillis(n: number) { */ export function settingsValidation(config: unknown, validationParams: ISettingsValidationParams) { - const { defaults, isClientSide, runtime, storage, integrations, logger, localhost, consent } = validationParams; + const { defaults, runtime, storage, integrations, logger, localhost, consent } = validationParams; // creates a settings object merging base, defaults and config objects. const withDefaults = merge({}, base, defaults, config) as ISettings; @@ -131,8 +131,8 @@ export function settingsValidation(config: unknown, validationParams: ISettingsV // @ts-ignore, modify readonly prop if (storage) withDefaults.storage = storage(withDefaults); - // In client-side, validate key and TT - if (isClientSide) { + // Validate key and TT (for client-side) + if (validationParams.acceptKey) { const maybeKey = withDefaults.core.key; // Although `key` is required in client-side, it can be omitted in LOCALHOST mode. In that case, the value `localhost_key` is used. if (withDefaults.mode === LOCALHOST_MODE && maybeKey === undefined) { @@ -144,9 +144,11 @@ export function settingsValidation(config: unknown, validationParams: ISettingsV withDefaults.core.key = validateKey(log, maybeKey, 'Client instantiation'); } - const maybeTT = withDefaults.core.trafficType; - if (maybeTT !== undefined) { // @ts-ignore, assigning false - withDefaults.core.trafficType = validateTrafficType(log, maybeTT, 'Client instantiation'); + if (validationParams.acceptTT) { + const maybeTT = withDefaults.core.trafficType; + if (maybeTT !== undefined) { // @ts-ignore + withDefaults.core.trafficType = validateTrafficType(log, maybeTT, 'Client instantiation'); + } } } diff --git a/src/utils/settingsValidation/types.ts b/src/utils/settingsValidation/types.ts index f6ead8e4..2322d042 100644 --- a/src/utils/settingsValidation/types.ts +++ b/src/utils/settingsValidation/types.ts @@ -10,8 +10,10 @@ export interface ISettingsValidationParams { * Version and startup properties are required, because they are not defined in the base settings. */ defaults: Partial & { version: string } & { startup: ISettings['startup'] }, - /** If true, validates core.key and core.trafficType */ - isClientSide?: boolean, + /** If true, validates core.key */ + acceptKey?: boolean, + /** If true, validates core.trafficType */ + acceptTT?: boolean, /** Define runtime values (`settings.runtime`) */ runtime: (settings: ISettings) => ISettings['runtime'], /** Storage validator (`settings.storage`) */ From 0ca9ff5fca2a36a367a66f53838acfde9b8cd78b Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Thu, 31 Mar 2022 13:01:03 -0300 Subject: [PATCH 4/5] fixed issue with a info log --- package-lock.json | 2 +- package.json | 2 +- src/sdkFactory/userConsentProps.ts | 5 ++--- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/package-lock.json b/package-lock.json index 1e0f7f3d..ebe428e0 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "@splitsoftware/splitio-commons", - "version": "1.2.1-rc.10", + "version": "1.2.1-rc.11", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index c74fbf0e..8338e84a 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@splitsoftware/splitio-commons", - "version": "1.2.1-rc.10", + "version": "1.2.1-rc.11", "description": "Split Javascript SDK common components", "main": "cjs/index.js", "module": "esm/index.js", diff --git a/src/sdkFactory/userConsentProps.ts b/src/sdkFactory/userConsentProps.ts index da715c33..893a10ca 100644 --- a/src/sdkFactory/userConsentProps.ts +++ b/src/sdkFactory/userConsentProps.ts @@ -19,13 +19,12 @@ export function userConsentProps(settings: ISettings, syncManager?: ISyncManager const newConsentStatus = consent ? CONSENT_GRANTED : CONSENT_DECLINED; - if (settings.userConsent !== newConsentStatus) { // @ts-ignore, modify readonly prop + if (settings.userConsent !== newConsentStatus) { + log.info(USER_CONSENT_UPDATED, [settings.userConsent, newConsentStatus]); // @ts-ignore, modify readonly prop settings.userConsent = newConsentStatus; if (consent) syncManager?.submitter?.start(); // resumes submitters if transitioning to GRANTED else syncManager?.submitter?.stop(); // pauses submitters if transitioning to DECLINED - - log.info(USER_CONSENT_UPDATED, [settings.userConsent, newConsentStatus]); } else { log.info(USER_CONSENT_NOT_UPDATED, [newConsentStatus]); } From f053eaf4163ac2e96378702758d6cba75787a77e Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Thu, 31 Mar 2022 13:40:46 -0300 Subject: [PATCH 5/5] added info log for initial user consent --- src/logger/constants.ts | 1 + src/logger/messages/info.ts | 1 + src/sdkFactory/userConsentProps.ts | 5 ++++- 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/logger/constants.ts b/src/logger/constants.ts index 68259646..10b45254 100644 --- a/src/logger/constants.ts +++ b/src/logger/constants.ts @@ -70,6 +70,7 @@ export const EVENTS_TRACKER_SUCCESS = 120; export const IMPRESSIONS_TRACKER_SUCCESS = 121; export const USER_CONSENT_UPDATED = 122; export const USER_CONSENT_NOT_UPDATED = 123; +export const USER_CONSENT_INITIAL = 124; export const ENGINE_VALUE_INVALID = 200; export const ENGINE_VALUE_NO_ATTRIBUTES = 201; diff --git a/src/logger/messages/info.ts b/src/logger/messages/info.ts index 9f4a4254..96703540 100644 --- a/src/logger/messages/info.ts +++ b/src/logger/messages/info.ts @@ -16,6 +16,7 @@ export const codesInfo: [number, string][] = codesWarn.concat([ [c.IMPRESSIONS_TRACKER_SUCCESS, c.LOG_PREFIX_IMPRESSIONS_TRACKER + 'Successfully stored %s impression(s).'], [c.USER_CONSENT_UPDATED, 'setUserConsent: consent status changed from %s to %s.'], [c.USER_CONSENT_NOT_UPDATED, 'setUserConsent: call had no effect because it was the current consent status (%s).'], + [c.USER_CONSENT_INITIAL, 'Starting the SDK with %s user consent. No data will be sent.'], // synchronizer [c.POLLING_SMART_PAUSING, c.LOG_PREFIX_SYNC_POLLING + 'Turning segments data polling %s.'], diff --git a/src/sdkFactory/userConsentProps.ts b/src/sdkFactory/userConsentProps.ts index 893a10ca..b275406c 100644 --- a/src/sdkFactory/userConsentProps.ts +++ b/src/sdkFactory/userConsentProps.ts @@ -1,6 +1,7 @@ -import { ERROR_NOT_BOOLEAN, USER_CONSENT_UPDATED, USER_CONSENT_NOT_UPDATED } from '../logger/constants'; +import { ERROR_NOT_BOOLEAN, USER_CONSENT_UPDATED, USER_CONSENT_NOT_UPDATED, USER_CONSENT_INITIAL } from '../logger/constants'; import { ISyncManager } from '../sync/types'; import { ISettings } from '../types'; +import { isConsentGranted } from '../utils/consent'; import { CONSENT_GRANTED, CONSENT_DECLINED } from '../utils/constants'; import { isBoolean } from '../utils/lang'; @@ -9,6 +10,8 @@ export function userConsentProps(settings: ISettings, syncManager?: ISyncManager const log = settings.log; + if (!isConsentGranted(settings)) log.info(USER_CONSENT_INITIAL, [settings.userConsent]); + return { setUserConsent(consent: unknown) { // validate input param