Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX] Le InMemoryTemporaryStorage ne suit pas la TemporaryStorage API et est utilisé à tort dans les tests d'intégration (PIX-12551) #8954

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -11,55 +11,55 @@ class InMemoryTemporaryStorage extends TemporaryStorage {
this._client = new NodeCache();
}

save({ key, value, expirationDelaySeconds }) {
async save({ key, value, expirationDelaySeconds }) {
const storageKey = trim(key) || InMemoryTemporaryStorage.generateKey();
this._client.set(storageKey, value, expirationDelaySeconds);
return storageKey;
}

update(key, value) {
async update(key, value) {
const storageKey = trim(key);
const timeoutMs = this._client.getTtl(storageKey);
const expirationDelaySeconds = timeoutMs === 0 ? 0 : (timeoutMs - Date.now()) / 1000;
this._client.set(storageKey, value, expirationDelaySeconds);
}

get(key) {
async get(key) {
return this._client.get(key);
}

delete(key) {
async delete(key) {
return this._client.del(key);
}

quit() {
noop;
}

expire({ key, expirationDelaySeconds }) {
async expire({ key, expirationDelaySeconds }) {
return this._client.ttl(key, expirationDelaySeconds);
}

ttl(key) {
async ttl(key) {
return this._client.getTtl(key);
}

lpush(key, value) {
async lpush(key, value) {
let list = this._client.get(key) || [];
list = [value, ...list];
this._client.set(key, list);
return list.length;
}

lrem(key, value) {
async lrem(key, value) {
const list = this._client.get(key) || [];
const filtered = list.filter((item) => item !== value);
const removed = list.filter((item) => item === value);
this._client.set(key, filtered);
return removed.length;
}

lrange(key) {
async lrange(key) {
return this._client.get(key) || [];
}
}
Expand Down
7 changes: 4 additions & 3 deletions api/lib/infrastructure/temporary-storage/index.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import { config } from '../../config.js';
const REDIS_URL = config.temporaryStorage.redisUrl;

const redisUrl = config.temporaryStorage.redisUrl;

import { InMemoryTemporaryStorage } from './InMemoryTemporaryStorage.js';
import { RedisTemporaryStorage } from './RedisTemporaryStorage.js';

function _createTemporaryStorage() {
if (REDIS_URL) {
return new RedisTemporaryStorage(REDIS_URL);
if (redisUrl) {
return new RedisTemporaryStorage(redisUrl);
} else {
return new InMemoryTemporaryStorage();
}
Expand Down
2 changes: 1 addition & 1 deletion api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@
"test:api": "for testType in 'unit' 'integration' 'acceptance'; do npm run test:api:$testType || status=1 ; done ; exit $status",
"test:api:path": "NODE_ENV=test mocha --exit --recursive --reporter=${MOCHA_REPORTER:-dot}",
"test:api:scripts": "npm run test:api:path -- tests/integration/scripts",
"test:api:unit": "TEST_DATABASE_URL=postgres://should.not.reach.db.in.unit.tests REDIS_URL= npm run test:api:path -- 'tests/**/unit/**/*test.js'",
"test:api:unit": "TEST_DATABASE_URL=postgres://should.not.reach.db.in.unit.tests TEST_REDIS_URL= npm run test:api:path -- 'tests/**/unit/**/*test.js'",
"test:api:integration": "npm run test:api:path -- 'tests/**/integration/**/*test.js'",
"test:api:acceptance": "npm run test:api:path -- 'tests/**/acceptance/**/*test.js'",
"test:api:debug": "NODE_ENV=test mocha --inspect-brk=9229 --recursive --exit --reporter dot tests",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export class FwbOidcAuthenticationService extends OidcAuthenticationService {
async getRedirectLogoutUrl({ userId, logoutUrlUUID }) {
const redirectTarget = new URL(this.logoutUrl);
const key = `${userId}:${logoutUrlUUID}`;
const idToken = this.sessionTemporaryStorage.get(key);
const idToken = await this.sessionTemporaryStorage.get(key);

const params = [{ key: 'id_token_hint', value: idToken }];

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export class PoleEmploiOidcAuthenticationService extends OidcAuthenticationServi
async getRedirectLogoutUrl({ userId, logoutUrlUUID }) {
const redirectTarget = new URL(this.logoutUrl);
const key = `${userId}:${logoutUrlUUID}`;
const idToken = this.sessionTemporaryStorage.get(key);
const idToken = await this.sessionTemporaryStorage.get(key);

const params = [
{ key: 'id_token_hint', value: idToken },
Expand Down
4 changes: 2 additions & 2 deletions api/src/shared/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ const configuration = (function () {

config.temporaryKey.secret = 'the-password-must-be-at-least-32-characters-long';

config.temporaryStorage.redisUrl = null;
config.temporaryStorage.redisUrl = process.env.TEST_REDIS_URL;

config.saml.accessTokenLifespanMs = 1000;

Expand Down Expand Up @@ -448,6 +448,7 @@ const configuration = (function () {
config.logging.enableLogStartingEventDispatch = false;
config.logging.enableLogEndingEventDispatch = false;

// TODO: Rather use config.caching.redisUrl = process.env.TEST_REDIS_URL;
config.caching.redisUrl = null;
config.caching.redisCacheKeyLockTTL = 100;
config.caching.redisCacheLockedWaitBeforeRetry = 1;
Expand All @@ -456,7 +457,6 @@ const configuration = (function () {

config.redis = {
url: process.env.TEST_REDIS_URL,
database: 1,
};

config.import = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ describe('Integration | Identity Access Management | Domain | Service | fwb-oidc

// then
const expectedResult = await defaultSessionTemporaryStorage.get(key);
expect(expectedResult).to.be.undefined;
expect(expectedResult).to.be.null;

expect(redirectTarget).to.equal(
'https://logout-url.org/?id_token_hint=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ describe('Integration | Identity Access Management | Domain | Services | pole-em

// then
const expectedResult = await defaultSessionTemporaryStorage.get(key);
expect(expectedResult).to.be.undefined;
expect(expectedResult).to.be.null;

expect(redirectTarget).to.equal(
'https://logout-url.fr/?id_token_hint=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c&redirect_uri=https%3A%2F%2Fafter-logout.fr',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,20 +19,20 @@ describe('Unit | Infrastructure | temporary-storage | InMemoryTemporaryStorage',
clock.restore();
});

it('should resolve with the generated key', function () {
it('should resolve with the generated key', async function () {
// when
const key = inMemoryTemporaryStorage.save({ value: {}, expirationDelaySeconds: 1000 });
const key = await inMemoryTemporaryStorage.save({ value: {}, expirationDelaySeconds: 1000 });

// then
expect(key).to.exist;
expect(key).to.be.a.string;
});

it('should return a key from passed key parameter if valid', function () {
it('should return a key from passed key parameter if valid', async function () {
// given
const keyParameter = 'KEY-PARAMETER';

// when
const returnedKey = inMemoryTemporaryStorage.save({
const returnedKey = await inMemoryTemporaryStorage.save({
key: keyParameter,
value: {},
expirationDelaySeconds: 1000,
Expand All @@ -42,12 +42,12 @@ describe('Unit | Infrastructure | temporary-storage | InMemoryTemporaryStorage',
expect(returnedKey).to.be.equal(keyParameter);
});

it('should return a generated key if key parameter is not valid', function () {
it('should return a generated key if key parameter is not valid', async function () {
// given
const keyParameter = ' ';

// when
const returnedKey = inMemoryTemporaryStorage.save({
const returnedKey = await inMemoryTemporaryStorage.save({
key: keyParameter,
value: {},
expirationDelaySeconds: 1000,
Expand All @@ -57,87 +57,87 @@ describe('Unit | Infrastructure | temporary-storage | InMemoryTemporaryStorage',
expect(returnedKey).not.be.equal(keyParameter);
});

it('should save key value with a defined ttl in seconds', function () {
it('should save key value with a defined ttl in seconds', async function () {
// given
const TWO_MINUTES_IN_SECONDS = 2 * 60;
const TWO_MINUTES_IN_MILLISECONDS = 2 * 60 * 1000;

// when
const key = inMemoryTemporaryStorage.save({
const key = await inMemoryTemporaryStorage.save({
value: { name: 'name' },
expirationDelaySeconds: TWO_MINUTES_IN_SECONDS,
});

// then
const expirationKeyInTimestamp = inMemoryTemporaryStorage._client.getTtl(key);
const expirationKeyInTimestamp = await inMemoryTemporaryStorage._client.getTtl(key);
expect(expirationKeyInTimestamp).to.equal(TWO_MINUTES_IN_MILLISECONDS);
});
});

describe('#get', function () {
it('should retrieve the value if it exists', function () {
it('should retrieve the value if it exists', async function () {
// given
const value = { name: 'name' };
const expirationDelaySeconds = 1000;

const key = inMemoryTemporaryStorage.save({ value, expirationDelaySeconds });
const key = await inMemoryTemporaryStorage.save({ value, expirationDelaySeconds });

// when
const result = inMemoryTemporaryStorage.get(key);
const result = await inMemoryTemporaryStorage.get(key);

// then
expect(result).to.deep.equal(value);
});
});

describe('#update', function () {
it('should set a new value', function () {
it('should set a new value', async function () {
// given
const key = inMemoryTemporaryStorage.save({
const key = await inMemoryTemporaryStorage.save({
value: { name: 'name' },
});

// when
inMemoryTemporaryStorage.update(key, { url: 'url' });
await inMemoryTemporaryStorage.update(key, { url: 'url' });

// then
const result = inMemoryTemporaryStorage.get(key);
const result = await inMemoryTemporaryStorage.get(key);
expect(result).to.deep.equal({ url: 'url' });
});

it('should not change the time to live', async function () {
// given
const keyWithTtl = inMemoryTemporaryStorage.save({
const keyWithTtl = await inMemoryTemporaryStorage.save({
value: {},
expirationDelaySeconds: 1,
});
const keyWithoutTtl = inMemoryTemporaryStorage.save({ value: {} });
const keyWithoutTtl = await inMemoryTemporaryStorage.save({ value: {} });

// when
await new Promise((resolve) => setTimeout(resolve, 500));
inMemoryTemporaryStorage.update(keyWithTtl, {});
inMemoryTemporaryStorage.update(keyWithoutTtl, {});
await inMemoryTemporaryStorage.update(keyWithTtl, {});
await inMemoryTemporaryStorage.update(keyWithoutTtl, {});
await new Promise((resolve) => setTimeout(resolve, 600));

// then
expect(inMemoryTemporaryStorage.get(keyWithTtl)).to.be.undefined;
expect(inMemoryTemporaryStorage.get(keyWithoutTtl)).not.to.be.undefined;
expect(await inMemoryTemporaryStorage.get(keyWithTtl)).to.be.undefined;
expect(await inMemoryTemporaryStorage.get(keyWithoutTtl)).not.to.be.undefined;
});
});

describe('#delete', function () {
it('should delete the value if it exists', function () {
it('should delete the value if it exists', async function () {
// given
const value = { name: 'name' };
const expirationDelaySeconds = 1000;

const key = inMemoryTemporaryStorage.save({ value, expirationDelaySeconds });
const key = await inMemoryTemporaryStorage.save({ value, expirationDelaySeconds });

// when
inMemoryTemporaryStorage.delete(key);
await inMemoryTemporaryStorage.delete(key);

// then
const savedKey = inMemoryTemporaryStorage.get(key);
const savedKey = await inMemoryTemporaryStorage.get(key);
expect(savedKey).to.be.undefined;
});
});
Expand Down Expand Up @@ -168,7 +168,7 @@ describe('Unit | Infrastructure | temporary-storage | InMemoryTemporaryStorage',
const key = 'key:lpush';
await inMemoryTemporaryStorage.lpush(key, 'value');
await inMemoryTemporaryStorage.expire({ key, expirationDelaySeconds: 120 });
const remainingExpirationSeconds = inMemoryTemporaryStorage.ttl(key);
const remainingExpirationSeconds = await inMemoryTemporaryStorage.ttl(key);

// then
expect(remainingExpirationSeconds).to.be.above(Date.now());
Expand Down