From 902ccd4809e125cd5fb9b6a1793705b72dd927ce Mon Sep 17 00:00:00 2001 From: Viktor Chernodub Date: Mon, 10 Jun 2024 20:15:20 +0300 Subject: [PATCH] refactor: remove explicit calls to cookie storage --- modules/yandexIdSystem.js | 85 ++++++------ test/spec/modules/yandexIdSystem_spec.js | 170 ++++++++++------------- 2 files changed, 122 insertions(+), 133 deletions(-) diff --git a/modules/yandexIdSystem.js b/modules/yandexIdSystem.js index 181aeeeef56..f24e33a8c44 100644 --- a/modules/yandexIdSystem.js +++ b/modules/yandexIdSystem.js @@ -4,21 +4,26 @@ * @requires module:modules/userId */ +// @ts-check + import { MODULE_TYPE_UID } from '../src/activities/modules.js'; import { submodule } from '../src/hook.js'; import { getStorageManager } from '../src/storageManager.js'; -import { logInfo } from '../src/utils.js'; +import { logError, logInfo } from '../src/utils.js'; -const BIDDER_CODE = 'yandex'; +// .com suffix is just a convention for naming the bidder eids +// See https://github.com/prebid/Prebid.js/pull/11196#discussion_r1591165139 const BIDDER_EID_KEY = 'yandex.com'; const YANDEX_ID_KEY = 'yandexId'; +export const BIDDER_CODE = 'yandex'; +export const YANDEX_USER_ID_KEY = '_ym_uid'; +export const YANDEX_COOKIE_STORAGE_TYPE = 'cookie'; +export const YANDEX_MIN_EXPIRE_DAYS = 30; -const USER_ID_KEY = '_ym_uid'; -const USER_ID_COOKIE_EXP_MS = 31536000000; // 365 days - -export const storage = getStorageManager({ +export const PREBID_STORAGE = getStorageManager({ moduleType: MODULE_TYPE_UID, moduleName: BIDDER_CODE, + bidderCode: undefined }); export const yandexIdSubmodule = { @@ -36,15 +41,24 @@ export const yandexIdSubmodule = { return { [YANDEX_ID_KEY]: value }; }, - getId() { - const yandexUidStorage = new YandexUidStorage(storage); - - if (!yandexUidStorage.checkIsAvailable()) { + /** + * @param {import('./userId/index.js').SubmoduleConfig} submoduleConfig + * @param {unknown} [_consentData] + * @param {string} [storedId] Id that was saved by the core previously. + */ + getId(submoduleConfig, _consentData, storedId) { + if (checkConfigHasErrorsAndReport(submoduleConfig)) { return; } + if (storedId) { + return { + id: storedId + }; + } + return { - id: yandexUidStorage.getUid(), + id: new YandexUidGenerator().generateUid(), }; }, eids: { @@ -55,43 +69,36 @@ export const yandexIdSubmodule = { }, }; -class YandexUidStorage { - /** - * @param {typeof cookieStorage} cookieStorage - */ - constructor(cookieStorage) { - this._cookieStorage = cookieStorage; - } - - _generateUid() { - return new YandexUidGenerator().generateUid(); - } +/** + * @param {import('./userId/index.js').SubmoduleConfig} submoduleConfig + * @returns {boolean} `true` - when there are errors, `false` - otherwise. + */ +function checkConfigHasErrorsAndReport(submoduleConfig) { + let error = false; - _getUserIdFromStorage() { - const id = this._cookieStorage.getCookie(USER_ID_KEY); + const READABLE_MODULE_NAME = 'Yandex ID module'; - return id; + if (submoduleConfig.storage == null) { + logError(`Misconfigured ${READABLE_MODULE_NAME}. "storage" is required.`) + return true; } - _setUid(userId) { - if (this._cookieStorage.cookiesAreEnabled()) { - const expires = new Date(Date.now() + USER_ID_COOKIE_EXP_MS).toString(); - - this._cookieStorage.setCookie(USER_ID_KEY, userId, expires); - } + if (submoduleConfig.storage?.name !== YANDEX_USER_ID_KEY) { + logError(`Misconfigured ${READABLE_MODULE_NAME}, "storage.name" is required to be "${YANDEX_USER_ID_KEY}"`); + error = true; } - checkIsAvailable() { - return this._cookieStorage.cookiesAreEnabled(); + if (submoduleConfig.storage?.type !== YANDEX_COOKIE_STORAGE_TYPE) { + logError(`Misconfigured ${READABLE_MODULE_NAME}, "storage.type" is required to be "${YANDEX_COOKIE_STORAGE_TYPE}"`); + error = true; } - getUid() { - const id = this._getUserIdFromStorage() || this._generateUid(); - - this._setUid(id); - - return id; + if ((submoduleConfig.storage?.expires ?? 0) < YANDEX_MIN_EXPIRE_DAYS) { + logError(`Misconfigured ${READABLE_MODULE_NAME}, "storage.expires" is required to be not less than "${YANDEX_MIN_EXPIRE_DAYS}"`); + error = true; } + + return error; } /** diff --git a/test/spec/modules/yandexIdSystem_spec.js b/test/spec/modules/yandexIdSystem_spec.js index bd798f3a3f2..d5f614dafb9 100644 --- a/test/spec/modules/yandexIdSystem_spec.js +++ b/test/spec/modules/yandexIdSystem_spec.js @@ -1,43 +1,77 @@ -import { yandexIdSubmodule, storage } from '../../../modules/yandexIdSystem.js'; +// @ts-check -const MIN_METRICA_ID_LEN = 17; +import { yandexIdSubmodule, PREBID_STORAGE, BIDDER_CODE, YANDEX_USER_ID_KEY, YANDEX_COOKIE_STORAGE_TYPE, YANDEX_MIN_EXPIRE_DAYS } from '../../../modules/yandexIdSystem.js'; +import {createSandbox} from 'sinon' +import * as utils from '../../../src/utils.js'; /** * @typedef {import('sinon').SinonStub} SinonStub + * @typedef {import('sinon').SinonSpy} SinonSpy * @typedef {import('sinon').SinonSandbox} SinonSandbox */ +const MIN_METRICA_ID_LEN = 17; + +/** @satisfies {import('../../../modules/userId/index.js').SubmoduleConfig} */ +const CORRECT_SUBMODULE_CONFIG = { + name: BIDDER_CODE, + storage: { + expires: YANDEX_MIN_EXPIRE_DAYS, + name: YANDEX_USER_ID_KEY, + type: YANDEX_COOKIE_STORAGE_TYPE, + refreshInSeconds: undefined, + }, + params: undefined, + value: undefined, +}; + +/** @type {import('../../../modules/userId/index.js').SubmoduleConfig[]} */ +const INCORRECT_SUBMODULE_CONFIGS = [ + { + ...CORRECT_SUBMODULE_CONFIG, + storage: { + ...CORRECT_SUBMODULE_CONFIG.storage, + expires: 0, + } + }, + { + ...CORRECT_SUBMODULE_CONFIG, + storage: { + ...CORRECT_SUBMODULE_CONFIG.storage, + type: 'html5' + } + }, + { + ...CORRECT_SUBMODULE_CONFIG, + storage: { + ...CORRECT_SUBMODULE_CONFIG.storage, + name: 'custom_key' + } + }, +]; + describe('YandexId module', () => { /** @type {SinonSandbox} */ let sandbox; /** @type {SinonStub} */ - let setCookieStub; - /** @type {SinonStub} */ - let getCookieStub; - /** @type {SinonStub} */ - let getLocalStorageStub; - /** @type {SinonStub} */ - let setLocalStorageStub; - /** @type {SinonStub} */ - let cookiesAreEnabledStub; - /** @type {SinonStub} */ let getCryptoRandomValuesStub; /** @type {SinonStub} */ let randomStub; + /** @type {SinonSpy} */ + let logErrorSpy; beforeEach(() => { - sandbox = sinon.sandbox.create(); - getCookieStub = sandbox.stub(storage, 'getCookie'); - setCookieStub = sandbox.stub(storage, 'setCookie'); - getLocalStorageStub = sandbox.stub(storage, 'getDataFromLocalStorage'); - setLocalStorageStub = sandbox.stub(storage, 'setDataInLocalStorage'); - cookiesAreEnabledStub = sandbox.stub(storage, 'cookiesAreEnabled'); - cookiesAreEnabledStub.returns(true); + sandbox = createSandbox(); + logErrorSpy = sandbox.spy(utils, 'logError'); getCryptoRandomValuesStub = sandbox .stub(window.crypto, 'getRandomValues') .callsFake((bufferView) => { - bufferView[0] = 10000; + if (bufferView != null) { + bufferView[0] = 10000; + } + + return null; }); randomStub = sandbox.stub(window.Math, 'random').returns(0.555); }); @@ -47,97 +81,45 @@ describe('YandexId module', () => { }); describe('getId()', () => { - describe('user id format', () => { - it('matches Yandex Metrica format', () => { - const generatedId = yandexIdSubmodule.getId().id; - expect(isNaN(generatedId)).to.be.false; - expect(generatedId).to.have.length.greaterThanOrEqual( - MIN_METRICA_ID_LEN - ); - }); - }); - - describe('when user id is set', () => { - const dummyId = Array(MIN_METRICA_ID_LEN).fill(1).join(''); - - beforeEach(() => { - getCookieStub.returns(dummyId); - }); - - it('returns id', () => { - const { id } = yandexIdSubmodule.getId(); - - expect(id).to.equal(dummyId); - }); + it('user id matches Yandex Metrica format', () => { + const generatedId = yandexIdSubmodule.getId(CORRECT_SUBMODULE_CONFIG)?.id; - it('does not change existing id', () => { - yandexIdSubmodule.getId(); - - expect(setCookieStub.lastCall.args[1]).to.equal(dummyId); - }); - }); - - describe('when user id is not set', () => { - beforeEach(() => { - getCookieStub.returns(undefined); - }); - - it('returns id', () => { - const id = yandexIdSubmodule.getId(); - - expect(id).not.to.be.undefined; - }); - - it('sets id', () => { - yandexIdSubmodule.getId() - - expect(setCookieStub.calledOnce).to.be.true; - }); + expect(isNaN(Number(generatedId))).to.be.false; + expect(generatedId).to.have.length.greaterThanOrEqual( + MIN_METRICA_ID_LEN + ); }); - describe('storage interaction', () => { - describe('when cookie storage is disabled', () => { - it('should not use storage api', () => { - cookiesAreEnabledStub.returns(false); + it('uses stored id', () => { + const storedId = '11111111111111111'; + const generatedId = yandexIdSubmodule.getId(CORRECT_SUBMODULE_CONFIG, undefined, storedId)?.id; - yandexIdSubmodule.getId(); + expect(generatedId).to.be.equal(storedId); + }) - expect(cookiesAreEnabledStub.called).to.be.true; - expect(getCookieStub.called).to.be.false; - expect(setCookieStub.called).to.be.false; - expect(getLocalStorageStub.called).to.be.false; - expect(setLocalStorageStub.called).to.be.false; - }); - }); + describe('config validation', () => { + INCORRECT_SUBMODULE_CONFIGS.forEach((config, i) => { + it(`invalid config #${i} fails`, () => { + const generatedId = yandexIdSubmodule.getId(config)?.id; - it('should use cookie', () => { - yandexIdSubmodule.getId(); - - expect(cookiesAreEnabledStub.called).to.be.true; - expect(getCookieStub.calledOnce).to.be.true; - expect(setCookieStub.calledOnce).to.be.true; - }); - - it('should not use localStorage', () => { - yandexIdSubmodule.getId(); - - expect(getLocalStorageStub.called).to.be.false; - expect(setLocalStorageStub.called).to.be.false; - }); - }); + expect(generatedId).to.be.undefined; + expect(logErrorSpy.called).to.be.true; + }) + }) + }) describe('crypto', () => { it('uses Math.random when crypto is not available', () => { sandbox.stub(window, 'crypto').value(undefined); - yandexIdSubmodule.getId(); + yandexIdSubmodule.getId(CORRECT_SUBMODULE_CONFIG); expect(randomStub.calledOnce).to.be.true; expect(getCryptoRandomValuesStub.called).to.be.false; }); it('uses crypto when it is available', () => { - yandexIdSubmodule.getId(); + yandexIdSubmodule.getId(CORRECT_SUBMODULE_CONFIG); expect(randomStub.called).to.be.false; expect(getCryptoRandomValuesStub.calledOnce).to.be.true;