From 27122b24e88d4d29f8ddc707968db371c7e04a22 Mon Sep 17 00:00:00 2001 From: Benjamin Petetot Date: Thu, 6 Mar 2025 14:16:38 +0100 Subject: [PATCH 01/12] chore(api): upgrade openid client to v6.3.3 --- api/package-lock.json | 65 ++++++++++++------------------------------- api/package.json | 2 +- 2 files changed, 19 insertions(+), 48 deletions(-) diff --git a/api/package-lock.json b/api/package-lock.json index 089c67036ce..18ce23350e9 100644 --- a/api/package-lock.json +++ b/api/package-lock.json @@ -59,7 +59,7 @@ "node-cache": "^5.1.2", "node-stream-zip": "^1.15.0", "nodemailer": "^6.9.6", - "openid-client": "^5.6.4", + "openid-client": "^6.3.3", "papaparse": "^5.3.2", "pdf-lib": "^1.17.1", "pg": "^8.7.3", @@ -8212,9 +8212,9 @@ } }, "node_modules/jose": { - "version": "4.15.9", - "resolved": "https://registry.npmjs.org/jose/-/jose-4.15.9.tgz", - "integrity": "sha512-1vUQX+IdDMVPj4k8kOxgUqlcK518yluMuGZwqlr44FS1ppZB/5GWh4rZG89erpOBOJjU/OBsnCVFfapsRz6nEA==", + "version": "6.0.8", + "resolved": "https://registry.npmjs.org/jose/-/jose-6.0.8.tgz", + "integrity": "sha512-EyUPtOKyTYq+iMOszO42eobQllaIjJnwkZ2U93aJzNyPibCy7CEvT9UQnaCVB51IAd49gbNdCew1c0LcLTCB2g==", "license": "MIT", "funding": { "url": "https://github.com/sponsors/panva" @@ -10057,6 +10057,15 @@ "node": "*" } }, + "node_modules/oauth4webapi": { + "version": "3.3.0", + "resolved": "https://registry.npmjs.org/oauth4webapi/-/oauth4webapi-3.3.0.tgz", + "integrity": "sha512-ZlozhPlFfobzh3hB72gnBFLjXpugl/dljz1fJSRdqaV2r3D5dmi5lg2QWI0LmUYuazmE+b5exsloEv6toUtw9g==", + "license": "MIT", + "funding": { + "url": "https://github.com/sponsors/panva" + } + }, "node_modules/object-assign": { "version": "4.1.1", "resolved": "https://registry.npmjs.org/object-assign/-/object-assign-4.1.1.tgz", @@ -10073,15 +10082,6 @@ "dev": true, "license": "MIT" }, - "node_modules/object-hash": { - "version": "2.2.0", - "resolved": "https://registry.npmjs.org/object-hash/-/object-hash-2.2.0.tgz", - "integrity": "sha512-gScRMn0bS5fH+IuwyIFgnh9zBdo4DV+6GhygmWM9HyNJSgS0hScp1f5vjtm7oIIOiT9trXrShAkLFSc2IqKNgw==", - "license": "MIT", - "engines": { - "node": ">= 6" - } - }, "node_modules/object-to-spawn-args": { "version": "2.0.1", "resolved": "https://registry.npmjs.org/object-to-spawn-args/-/object-to-spawn-args-2.0.1.tgz", @@ -10092,15 +10092,6 @@ "node": ">=8.0.0" } }, - "node_modules/oidc-token-hash": { - "version": "5.0.3", - "resolved": "https://registry.npmjs.org/oidc-token-hash/-/oidc-token-hash-5.0.3.tgz", - "integrity": "sha512-IF4PcGgzAr6XXSff26Sk/+P4KZFJVuHAJZj3wgO3vX2bMdNVp/QXTP3P7CEm9V1IdG8lDLY3HhiqpsE/nOwpPw==", - "license": "MIT", - "engines": { - "node": "^10.13.0 || >=12.0.0" - } - }, "node_modules/on-exit-leak-free": { "version": "2.1.2", "resolved": "https://registry.npmjs.org/on-exit-leak-free/-/on-exit-leak-free-2.1.2.tgz", @@ -10127,38 +10118,18 @@ "peer": true }, "node_modules/openid-client": { - "version": "5.7.1", - "resolved": "https://registry.npmjs.org/openid-client/-/openid-client-5.7.1.tgz", - "integrity": "sha512-jDBPgSVfTnkIh71Hg9pRvtJc6wTwqjRkN88+gCFtYWrlP4Yx2Dsrow8uPi3qLr/aeymPF3o2+dS+wOpglK04ew==", + "version": "6.3.3", + "resolved": "https://registry.npmjs.org/openid-client/-/openid-client-6.3.3.tgz", + "integrity": "sha512-lTK8AV8SjqCM4qznLX0asVESAwzV39XTVdfMAM185ekuaZCnkWdPzcxMTXNlsm9tsUAMa1Q30MBmKAykdT1LWw==", "license": "MIT", "dependencies": { - "jose": "^4.15.9", - "lru-cache": "^6.0.0", - "object-hash": "^2.2.0", - "oidc-token-hash": "^5.0.3" + "jose": "^6.0.6", + "oauth4webapi": "^3.3.0" }, "funding": { "url": "https://github.com/sponsors/panva" } }, - "node_modules/openid-client/node_modules/lru-cache": { - "version": "6.0.0", - "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-6.0.0.tgz", - "integrity": "sha512-Jo6dJ04CmSjuznwJSS3pUeWmd/H0ffTlkXXgwZi+eq1UCmqQwCh+eLsYOYCwY991i2Fah4h1BEMCx4qThGbsiA==", - "license": "ISC", - "dependencies": { - "yallist": "^4.0.0" - }, - "engines": { - "node": ">=10" - } - }, - "node_modules/openid-client/node_modules/yallist": { - "version": "4.0.0", - "resolved": "https://registry.npmjs.org/yallist/-/yallist-4.0.0.tgz", - "integrity": "sha512-3wdGidZyq5PB084XLES5TpOSRA3wjXAlIWMhum2kRcv/41Sn2emQ0dycQW4uZXLejwKvg6EsvbdlVL+FYEct7A==", - "license": "ISC" - }, "node_modules/optionator": { "version": "0.9.4", "resolved": "https://registry.npmjs.org/optionator/-/optionator-0.9.4.tgz", diff --git a/api/package.json b/api/package.json index 5b3a842473b..d1f33c02ffc 100644 --- a/api/package.json +++ b/api/package.json @@ -65,7 +65,7 @@ "node-cache": "^5.1.2", "node-stream-zip": "^1.15.0", "nodemailer": "^6.9.6", - "openid-client": "^5.6.4", + "openid-client": "^6.3.3", "papaparse": "^5.3.2", "pdf-lib": "^1.17.1", "pg": "^8.7.3", From 50e51f0a3eef0797195ec99236bd1e58c82169bf Mon Sep 17 00:00:00 2001 From: Benjamin Petetot Date: Thu, 6 Mar 2025 14:50:42 +0100 Subject: [PATCH 02/12] refactor(api): use openid-client v6 --- .../services/oidc-authentication-service.js | 46 ++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/api/src/identity-access-management/domain/services/oidc-authentication-service.js b/api/src/identity-access-management/domain/services/oidc-authentication-service.js index 7ec957e843f..7efc6e06f1c 100644 --- a/api/src/identity-access-management/domain/services/oidc-authentication-service.js +++ b/api/src/identity-access-management/domain/services/oidc-authentication-service.js @@ -20,6 +20,13 @@ const DEFAULT_SCOPE = 'openid profile'; const defaultSessionTemporaryStorage = temporaryStorage.withPrefix('oidc-session:'); +// TODO: A verifier si tous les parametre de la configuration sont bien utilisé +// +// On ne retrouve pas dans le code source de la plateforme les propriétés suivantes: +// "openidClientExtraMetadata": { +// "token_endpoint_auth_method": "client_secret_post" +// }, + export class OidcAuthenticationService { #isReady = false; #isReadyForPixAdmin = false; @@ -107,6 +114,14 @@ export class OidcAuthenticationService { async createClient() { try { + // TODO + // https://github.com/panva/openid-client/blob/main/docs/functions/discovery.md + // + // let config: client.Configuration = await client.discovery( + // server, + // clientId, + // clientSecret, + // ) const issuer = await Issuer.discover(this.openidConfigurationUrl); const metadata = { client_id: this.clientId, @@ -148,6 +163,16 @@ export class OidcAuthenticationService { let tokenSet; try { + // TODO + // https://github.com/panva/openid-client/blob/main/docs/functions/authorizationCodeGrant.md + // + // let currentUrl: URL = getCurrentUrl() + // let tokens = await client.authorizationCodeGrant(config, currentUrl, { + // pkceCodeVerifier: code_verifier, + // expectedNonce: nonce, + // idTokenExpected: true, + // }) + tokenSet = await this.client.callback(this.redirectUri, { code, state, iss }, { nonce, state: sessionState }); } catch (error) { _monitorOidcError(error.message, { @@ -175,7 +200,7 @@ export class OidcAuthenticationService { getAuthorizationUrl() { const state = randomUUID(); - const nonce = randomUUID(); + const nonce = randomUUID(); // TODO: client.randomNonce() ? const authorizationParameters = { nonce, redirect_uri: this.redirectUri, @@ -190,6 +215,16 @@ export class OidcAuthenticationService { let redirectTarget; try { + // https://github.com/panva/openid-client/blob/main/docs/functions/buildAuthorizationUrl.md + // + // let redirectTo = client.buildAuthorizationUrl(config, { + // redirect_uri, + // scope, + // code_challenge, + // code_challenge_method: 'S256', + // }) + + // TODO redirectTarget = this.client.authorizationUrl(authorizationParameters); } catch (error) { _monitorOidcError(error.message, { @@ -260,6 +295,13 @@ export class OidcAuthenticationService { } try { + // TODO + // https://github.com/panva/openid-client/blob/main/docs/functions/buildEndSessionUrl.md + // + // let redirectTo = client.buildEndSessionUrl(config, { + // post_logout_redirect_uri, + // id_token_hint: id_token, + // }) const endSessionUrl = this.client.endSessionUrl(parameters); await this.sessionTemporaryStorage.delete(key); @@ -279,6 +321,8 @@ export class OidcAuthenticationService { let userInfo; try { + // TODO + // https://github.com/panva/openid-client/blob/main/docs/functions/fetchUserInfo.md userInfo = await this.client.userinfo(accessToken); } catch (error) { _monitorOidcError(error.message, { From 62693d72db33548d859aeacd154abe2893ccbb94 Mon Sep 17 00:00:00 2001 From: Benjamin Petetot Date: Thu, 6 Mar 2025 15:12:33 +0100 Subject: [PATCH 03/12] refactor(api): change createClient to initializeClientConfig using discovery --- .../oidc-authentication-service-registry.js | 4 +-- .../services/oidc-authentication-service.js | 26 ++++++----------- ...dc-authentication-service-registry_test.js | 9 +++--- .../oidc-authentication-service_test.js | 28 +++++++++---------- ...emploi-oidc-authentication-service_test.js | 6 ++-- 5 files changed, 33 insertions(+), 40 deletions(-) diff --git a/api/src/identity-access-management/domain/services/oidc-authentication-service-registry.js b/api/src/identity-access-management/domain/services/oidc-authentication-service-registry.js index 7dd046d3c98..1ee77dc8351 100644 --- a/api/src/identity-access-management/domain/services/oidc-authentication-service-registry.js +++ b/api/src/identity-access-management/domain/services/oidc-authentication-service-registry.js @@ -23,9 +23,9 @@ export class OidcAuthenticationServiceRegistry { ); if (!oidcProviderService) return; - if (oidcProviderService.client) return; + if (oidcProviderService.isClientConfigInitialized) return; // TODO À mettre dans .initializeClientConfig() ? - await oidcProviderService.createClient(); + await oidcProviderService.initializeClientConfig(); return true; } diff --git a/api/src/identity-access-management/domain/services/oidc-authentication-service.js b/api/src/identity-access-management/domain/services/oidc-authentication-service.js index 7efc6e06f1c..a9dcfe9acef 100644 --- a/api/src/identity-access-management/domain/services/oidc-authentication-service.js +++ b/api/src/identity-access-management/domain/services/oidc-authentication-service.js @@ -3,7 +3,7 @@ import { randomUUID } from 'node:crypto'; import jsonwebtoken from 'jsonwebtoken'; import lodash from 'lodash'; import ms from 'ms'; -import { Issuer } from 'openid-client'; +import * as client from 'openid-client'; import { config } from '../../../shared/config.js'; import { OIDC_ERRORS } from '../../../shared/domain/constants.js'; @@ -30,6 +30,7 @@ const defaultSessionTemporaryStorage = temporaryStorage.withPrefix('oidc-session export class OidcAuthenticationService { #isReady = false; #isReadyForPixAdmin = false; + #oidcClientConfig = null; constructor( { @@ -112,28 +113,19 @@ export class OidcAuthenticationService { return this.#isReadyForPixAdmin; } - async createClient() { + get isClientConfigInitialized() { + return !!this.#oidcClientConfig; + } + + async initializeClientConfig() { try { - // TODO - // https://github.com/panva/openid-client/blob/main/docs/functions/discovery.md - // - // let config: client.Configuration = await client.discovery( - // server, - // clientId, - // clientSecret, - // ) - const issuer = await Issuer.discover(this.openidConfigurationUrl); const metadata = { - client_id: this.clientId, client_secret: this.clientSecret, redirect_uris: [this.redirectUri], + ...this.openidClientExtraMetadata, }; - if (this.openidClientExtraMetadata) { - Object.assign(metadata, this.openidClientExtraMetadata); - } - - this.client = new issuer.Client(metadata); + this.#oidcClientConfig = await client.discovery(this.openidConfigurationUrl, this.clientId, metadata); } catch (error) { logger.error(`OIDC Provider "${this.identityProvider}" is UNAVAILABLE: ${error}`); } diff --git a/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service-registry_test.js b/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service-registry_test.js index c38c8be3470..e62f73e7938 100644 --- a/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service-registry_test.js +++ b/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service-registry_test.js @@ -112,12 +112,13 @@ describe('Unit | Identity Access Management | Domain | Services | oidc-authentic context('when oidc provider service exists and loaded', function () { it('configures openid client for ready oidc provider service and returns true', async function () { // given - const createClient = sinon.stub().resolves(); + const initializeClientConfig = sinon.stub().resolves(); const oidcProviderServices = [ { code: 'OIDC', isReady: true, - createClient, + isClientConfigInitialized: false, + initializeClientConfig, }, ]; await oidcAuthenticationServiceRegistry.loadOidcProviderServices(oidcProviderServices); @@ -129,7 +130,7 @@ describe('Unit | Identity Access Management | Domain | Services | oidc-authentic // then expect(result).to.be.true; - expect(createClient).to.have.been.calledOnce; + expect(initializeClientConfig).to.have.been.calledOnce; }); context('when there is already a client instantiated', function () { @@ -139,7 +140,7 @@ describe('Unit | Identity Access Management | Domain | Services | oidc-authentic { code: 'OIDC', isReady: true, - client: {}, + isClientConfigInitialized: true, }, ]; await oidcAuthenticationServiceRegistry.loadOidcProviderServices(oidcProviderServices); diff --git a/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service_test.js b/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service_test.js index ff60514ec10..2a3cb3213fb 100644 --- a/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service_test.js +++ b/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service_test.js @@ -189,7 +189,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const oidcAuthenticationService = new OidcAuthenticationService(settings.oidcExampleNet, { sessionTemporaryStorage, }); - await oidcAuthenticationService.createClient(); + await oidcAuthenticationService.initializeClientConfig(); // when const result = await oidcAuthenticationService.saveIdToken({ idToken, userId }); @@ -219,7 +219,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const oidcAuthenticationService = new OidcAuthenticationService(settings.oidcExampleNet, { sessionTemporaryStorage, }); - await oidcAuthenticationService.createClient(); + await oidcAuthenticationService.initializeClientConfig(); // when const result = await oidcAuthenticationService.getRedirectLogoutUrl({ userId, logoutUrlUUID }); @@ -254,7 +254,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const oidcAuthenticationService = new OidcAuthenticationService(settings.oidcExampleNet, { sessionTemporaryStorage, }); - await oidcAuthenticationService.createClient(); + await oidcAuthenticationService.initializeClientConfig(); // when const error = await catchErr( @@ -316,7 +316,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { openidConfigurationUrl, tokenUrl, }); - await oidcAuthenticationService.createClient(); + await oidcAuthenticationService.initializeClientConfig(); // when const result = await oidcAuthenticationService.exchangeCodeForTokens({ @@ -360,7 +360,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { openidConfigurationUrl, organizationName: 'Oidc Example', }); - await oidcAuthenticationService.createClient(); + await oidcAuthenticationService.initializeClientConfig(); // when const error = await catchErr( @@ -411,7 +411,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const Client = sinon.stub().returns(clientInstance); sinon.stub(Issuer, 'discover').resolves({ Client }); - await oidcAuthenticationService.createClient(); + await oidcAuthenticationService.initializeClientConfig(); // when const { nonce, state } = oidcAuthenticationService.getAuthorizationUrl(); @@ -451,7 +451,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { openidConfigurationUrl, organizationName: 'Oidc Example', }); - await oidcAuthenticationService.createClient(); + await oidcAuthenticationService.initializeClientConfig(); // when const error = catchErrSync(oidcAuthenticationService.getAuthorizationUrl, oidcAuthenticationService)(); @@ -687,7 +687,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const Client = sinon.stub().returns(clientInstance); sinon.stub(Issuer, 'discover').resolves({ Client }); - await oidcAuthenticationService.createClient(); + await oidcAuthenticationService.initializeClientConfig(); const accessToken = 'thisIsSerializedInformation'; @@ -725,7 +725,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { openidConfigurationUrl, organizationName: 'Oidc Example', }); - await oidcAuthenticationService.createClient(); + await oidcAuthenticationService.initializeClientConfig(); // when const error = await catchErr(oidcAuthenticationService._getUserInfoFromEndpoint, oidcAuthenticationService)({}); @@ -769,7 +769,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const Client = sinon.stub().returns(clientInstance); sinon.stub(Issuer, 'discover').resolves({ Client }); - await oidcAuthenticationService.createClient(); + await oidcAuthenticationService.initializeClientConfig(); const accessToken = 'thisIsSerializedInformation'; const errorMessage = `Un ou des champs obligatoires (family_name) n'ont pas été renvoyés par votre fournisseur d'identité Example.`; @@ -830,7 +830,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const Client = sinon.stub().returns(clientInstance); sinon.stub(Issuer, 'discover').resolves({ Client }); - await oidcAuthenticationService.createClient(); + await oidcAuthenticationService.initializeClientConfig(); const accessToken = 'thisIsSerializedInformation'; const errorMessage = `Un ou des champs obligatoires (population) n'ont pas été renvoyés par votre fournisseur d'identité Example.`; @@ -993,7 +993,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { }); }); - describe('#createClient', function () { + describe('#initializeClientConfig', function () { it('creates an openid client', async function () { // given const clientId = Symbol('clientId'); @@ -1014,7 +1014,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { }); // when - await oidcAuthenticationService.createClient(); + await oidcAuthenticationService.initializeClientConfig(); // then expect(Issuer.discover).to.have.been.calledWithExactly(openidConfigurationUrl); @@ -1048,7 +1048,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { }); // when - await oidcAuthenticationService.createClient(); + await oidcAuthenticationService.initializeClientConfig(); // then expect(Issuer.discover).to.have.been.calledWithExactly(openidConfigurationUrl); diff --git a/api/tests/identity-access-management/unit/domain/services/pole-emploi-oidc-authentication-service_test.js b/api/tests/identity-access-management/unit/domain/services/pole-emploi-oidc-authentication-service_test.js index f1e33708ab8..130419332ed 100644 --- a/api/tests/identity-access-management/unit/domain/services/pole-emploi-oidc-authentication-service_test.js +++ b/api/tests/identity-access-management/unit/domain/services/pole-emploi-oidc-authentication-service_test.js @@ -26,8 +26,8 @@ describe('Unit | Identity Access Management | Domain | Services | pole-emploi-oi }); }); - describe('#createClient', function () { - it('creates an openid client with extra metadata', async function () { + describe('#initializeClientConfig', function () { + it('creates an openid client config with extra metadata', async function () { // given const Client = sinon.spy(); @@ -46,7 +46,7 @@ describe('Unit | Identity Access Management | Domain | Services | pole-emploi-oi }); // when - await poleEmploiOidcAuthenticationService.createClient(); + await poleEmploiOidcAuthenticationService.initializeClientConfig(); // then expect(Issuer.discover).to.have.been.calledWithExactly( From 6041570bd55b2752c78988275298d7fb67927370 Mon Sep 17 00:00:00 2001 From: Benjamin Petetot Date: Thu, 6 Mar 2025 15:19:54 +0100 Subject: [PATCH 04/12] refactor(api): change getAuthorisationUrl using buildAuthorizationUrl --- .../services/oidc-authentication-service.js | 28 ++++++------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/api/src/identity-access-management/domain/services/oidc-authentication-service.js b/api/src/identity-access-management/domain/services/oidc-authentication-service.js index a9dcfe9acef..8ba23dacce3 100644 --- a/api/src/identity-access-management/domain/services/oidc-authentication-service.js +++ b/api/src/identity-access-management/domain/services/oidc-authentication-service.js @@ -193,31 +193,19 @@ export class OidcAuthenticationService { getAuthorizationUrl() { const state = randomUUID(); const nonce = randomUUID(); // TODO: client.randomNonce() ? - const authorizationParameters = { - nonce, - redirect_uri: this.redirectUri, - scope: this.scope, - state, - }; - - if (this.extraAuthorizationUrlParameters) { - Object.assign(authorizationParameters, this.extraAuthorizationUrlParameters); - } let redirectTarget; try { - // https://github.com/panva/openid-client/blob/main/docs/functions/buildAuthorizationUrl.md - // - // let redirectTo = client.buildAuthorizationUrl(config, { - // redirect_uri, - // scope, - // code_challenge, - // code_challenge_method: 'S256', - // }) + const parameters = { + nonce, + redirect_uri: this.redirectUri, + scope: this.scope, + state, + ...this.extraAuthorizationUrlParameters, + }; - // TODO - redirectTarget = this.client.authorizationUrl(authorizationParameters); + redirectTarget = client.buildAuthorizationUrl(this.#oidcClientConfig, parameters); } catch (error) { _monitorOidcError(error.message, { data: { organizationName: this.organizationName }, From ba02e5eac75bdffffb597d69e55afdf8015255c9 Mon Sep 17 00:00:00 2001 From: Benjamin Petetot Date: Thu, 6 Mar 2025 15:25:04 +0100 Subject: [PATCH 05/12] refactor(api): change getRedirectLogoutUrl using buildEndSessionUrl --- .../domain/services/oidc-authentication-service.js | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/api/src/identity-access-management/domain/services/oidc-authentication-service.js b/api/src/identity-access-management/domain/services/oidc-authentication-service.js index 8ba23dacce3..952ef25235c 100644 --- a/api/src/identity-access-management/domain/services/oidc-authentication-service.js +++ b/api/src/identity-access-management/domain/services/oidc-authentication-service.js @@ -275,14 +275,7 @@ export class OidcAuthenticationService { } try { - // TODO - // https://github.com/panva/openid-client/blob/main/docs/functions/buildEndSessionUrl.md - // - // let redirectTo = client.buildEndSessionUrl(config, { - // post_logout_redirect_uri, - // id_token_hint: id_token, - // }) - const endSessionUrl = this.client.endSessionUrl(parameters); + const endSessionUrl = client.buildEndSessionUrl(this.#oidcClientConfig, parameters); await this.sessionTemporaryStorage.delete(key); From a222cd76052823af5659af275215ac53b23c33cf Mon Sep 17 00:00:00 2001 From: Benjamin Petetot Date: Thu, 6 Mar 2025 15:35:46 +0100 Subject: [PATCH 06/12] refactor(api): change _getUserInfoFromEndpoint using fetchUserInfo --- .../domain/services/oidc-authentication-service.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/api/src/identity-access-management/domain/services/oidc-authentication-service.js b/api/src/identity-access-management/domain/services/oidc-authentication-service.js index 952ef25235c..e83983ab7b2 100644 --- a/api/src/identity-access-management/domain/services/oidc-authentication-service.js +++ b/api/src/identity-access-management/domain/services/oidc-authentication-service.js @@ -222,7 +222,7 @@ export class OidcAuthenticationService { let userInfo = jsonwebtoken.decode(idToken); if (this.claimManager.hasMissingClaims(userInfo)) { - userInfo = await this._getUserInfoFromEndpoint({ accessToken }); + userInfo = await this._getUserInfoFromEndpoint({ accessToken, expectedSubject: userInfo.sub }); } return { @@ -290,13 +290,15 @@ export class OidcAuthenticationService { } } - async _getUserInfoFromEndpoint({ accessToken }) { + async _getUserInfoFromEndpoint({ accessToken, expectedSubject }) { let userInfo; try { - // TODO - // https://github.com/panva/openid-client/blob/main/docs/functions/fetchUserInfo.md - userInfo = await this.client.userinfo(accessToken); + userInfo = await client.fetchUserInfo( + this.#oidcClientConfig, + accessToken, + expectedSubject || client.skipSubjectCheck, + ); } catch (error) { _monitorOidcError(error.message, { data: { organizationName: this.organizationName }, From bddfb4aebadecb4998669ba5ea28b352f0f46baf Mon Sep 17 00:00:00 2001 From: Benjamin Petetot Date: Thu, 6 Mar 2025 15:43:28 +0100 Subject: [PATCH 07/12] refactor(api): change exchangeCodeForTokens using authorizationCodeGrant --- .../services/oidc-authentication-service.js | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/api/src/identity-access-management/domain/services/oidc-authentication-service.js b/api/src/identity-access-management/domain/services/oidc-authentication-service.js index e83983ab7b2..9e77b7bf613 100644 --- a/api/src/identity-access-management/domain/services/oidc-authentication-service.js +++ b/api/src/identity-access-management/domain/services/oidc-authentication-service.js @@ -155,17 +155,15 @@ export class OidcAuthenticationService { let tokenSet; try { - // TODO - // https://github.com/panva/openid-client/blob/main/docs/functions/authorizationCodeGrant.md - // - // let currentUrl: URL = getCurrentUrl() - // let tokens = await client.authorizationCodeGrant(config, currentUrl, { - // pkceCodeVerifier: code_verifier, - // expectedNonce: nonce, - // idTokenExpected: true, - // }) - - tokenSet = await this.client.callback(this.redirectUri, { code, state, iss }, { nonce, state: sessionState }); + const checks = { nonce, state: sessionState }; + const tokenEndpointParameters = { code, state, iss }; + + tokenSet = await client.authorizationCodeGrant( + this.#oidcClientConfig, + this.redirectUri, + checks, + tokenEndpointParameters, + ); } catch (error) { _monitorOidcError(error.message, { data: { code, nonce, organizationName: this.organizationName, sessionState, state, iss }, From 9f39809222d2fa720df47212bf42cf31e261c5ff Mon Sep 17 00:00:00 2001 From: Benjamin Petetot Date: Thu, 6 Mar 2025 16:17:53 +0100 Subject: [PATCH 08/12] refactor(api): fix unit tests for oidc-authentication-service --- .../services/oidc-authentication-service.js | 22 +- ...c-authentication-service-registry.test.js} | 0 ...js => oidc-authentication-service.test.js} | 379 +++++++++--------- ....js => pix-authentication-service.test.js} | 0 ...mploi-oidc-authentication-service.test.js} | 99 +++-- 5 files changed, 263 insertions(+), 237 deletions(-) rename api/tests/identity-access-management/unit/domain/services/{oidc-authentication-service-registry_test.js => oidc-authentication-service-registry.test.js} (100%) rename api/tests/identity-access-management/unit/domain/services/{oidc-authentication-service_test.js => oidc-authentication-service.test.js} (85%) rename api/tests/identity-access-management/unit/domain/services/{pix-authentication-service_test.js => pix-authentication-service.test.js} (100%) rename api/tests/identity-access-management/unit/domain/services/{pole-emploi-oidc-authentication-service_test.js => pole-emploi-oidc-authentication-service.test.js} (58%) diff --git a/api/src/identity-access-management/domain/services/oidc-authentication-service.js b/api/src/identity-access-management/domain/services/oidc-authentication-service.js index 9e77b7bf613..4361ea8eb5f 100644 --- a/api/src/identity-access-management/domain/services/oidc-authentication-service.js +++ b/api/src/identity-access-management/domain/services/oidc-authentication-service.js @@ -30,7 +30,8 @@ const defaultSessionTemporaryStorage = temporaryStorage.withPrefix('oidc-session export class OidcAuthenticationService { #isReady = false; #isReadyForPixAdmin = false; - #oidcClientConfig = null; + #openIdClient; + #oidcClientConfig; // TODO: À modifier en openIdClientConfig constructor( { @@ -55,7 +56,7 @@ export class OidcAuthenticationService { isVisible = true, claimMapping, }, - { sessionTemporaryStorage = defaultSessionTemporaryStorage } = {}, + { sessionTemporaryStorage = defaultSessionTemporaryStorage, openIdClient = client } = {}, ) { this.accessTokenLifespanMs = ms(accessTokenLifespan); this.additionalRequiredProperties = additionalRequiredProperties; @@ -76,6 +77,7 @@ export class OidcAuthenticationService { this.slug = slug; this.source = source; this.isVisible = isVisible; + this.#openIdClient = openIdClient; claimMapping = claimMapping || DEFAULT_CLAIM_MAPPING; @@ -125,7 +127,7 @@ export class OidcAuthenticationService { ...this.openidClientExtraMetadata, }; - this.#oidcClientConfig = await client.discovery(this.openidConfigurationUrl, this.clientId, metadata); + this.#oidcClientConfig = await this.#openIdClient.discovery(this.openidConfigurationUrl, this.clientId, metadata); } catch (error) { logger.error(`OIDC Provider "${this.identityProvider}" is UNAVAILABLE: ${error}`); } @@ -158,7 +160,7 @@ export class OidcAuthenticationService { const checks = { nonce, state: sessionState }; const tokenEndpointParameters = { code, state, iss }; - tokenSet = await client.authorizationCodeGrant( + tokenSet = await this.#openIdClient.authorizationCodeGrant( this.#oidcClientConfig, this.redirectUri, checks, @@ -190,7 +192,7 @@ export class OidcAuthenticationService { getAuthorizationUrl() { const state = randomUUID(); - const nonce = randomUUID(); // TODO: client.randomNonce() ? + const nonce = randomUUID(); // TODO: this.#openIdClient.randomNonce() ? let redirectTarget; @@ -203,7 +205,7 @@ export class OidcAuthenticationService { ...this.extraAuthorizationUrlParameters, }; - redirectTarget = client.buildAuthorizationUrl(this.#oidcClientConfig, parameters); + redirectTarget = this.#openIdClient.buildAuthorizationUrl(this.#oidcClientConfig, parameters); } catch (error) { _monitorOidcError(error.message, { data: { organizationName: this.organizationName }, @@ -273,7 +275,7 @@ export class OidcAuthenticationService { } try { - const endSessionUrl = client.buildEndSessionUrl(this.#oidcClientConfig, parameters); + const endSessionUrl = this.#openIdClient.buildEndSessionUrl(this.#oidcClientConfig, parameters); await this.sessionTemporaryStorage.delete(key); @@ -292,11 +294,7 @@ export class OidcAuthenticationService { let userInfo; try { - userInfo = await client.fetchUserInfo( - this.#oidcClientConfig, - accessToken, - expectedSubject || client.skipSubjectCheck, - ); + userInfo = await this.#openIdClient.fetchUserInfo(this.#oidcClientConfig, accessToken, expectedSubject); } catch (error) { _monitorOidcError(error.message, { data: { organizationName: this.organizationName }, diff --git a/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service-registry_test.js b/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service-registry.test.js similarity index 100% rename from api/tests/identity-access-management/unit/domain/services/oidc-authentication-service-registry_test.js rename to api/tests/identity-access-management/unit/domain/services/oidc-authentication-service-registry.test.js diff --git a/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service_test.js b/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service.test.js similarity index 85% rename from api/tests/identity-access-management/unit/domain/services/oidc-authentication-service_test.js rename to api/tests/identity-access-management/unit/domain/services/oidc-authentication-service.test.js index 2a3cb3213fb..83ef337c10c 100644 --- a/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service_test.js +++ b/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service.test.js @@ -1,6 +1,5 @@ import jsonwebtoken from 'jsonwebtoken'; import ms from 'ms'; -import { Issuer } from 'openid-client'; import { OidcAuthenticationService } from '../../../../../src/identity-access-management/domain/services/oidc-authentication-service.js'; import { config as settings } from '../../../../../src/shared/config.js'; @@ -17,8 +16,19 @@ import { catchErr, catchErrSync, expect, sinon } from '../../../../test-helper.j const uuidV4Regex = /^[0-9A-F]{8}-[0-9A-F]{4}-4[0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}$/i; +const MOCK_OPENID_CLIENT_CONFIG = Symbol('config'); + describe('Unit | Domain | Services | oidc-authentication-service', function () { + let openIdClient; + beforeEach(function () { + openIdClient = { + discovery: sinon.stub().resolves(MOCK_OPENID_CLIENT_CONFIG), + authorizationCodeGrant: sinon.stub(), + buildAuthorizationUrl: sinon.stub(), + buildEndSessionUrl: sinon.stub(), + fetchUserInfo: sinon.stub(), + }; sinon.stub(monitoringTools, 'logErrorWithCorrelationIds'); }); @@ -29,7 +39,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const args = {}; // when - const oidcAuthenticationService = new OidcAuthenticationService(args); + const oidcAuthenticationService = new OidcAuthenticationService(args, { openIdClient }); // then expect(oidcAuthenticationService.shouldCloseSession).to.be.false; @@ -44,7 +54,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const args = { claimMapping: null, claimsToStore: null }; // when - const { claimManager } = new OidcAuthenticationService(args); + const { claimManager } = new OidcAuthenticationService(args, { openIdClient }); const claims = claimManager.getMissingClaims(); // then @@ -58,7 +68,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const args = { claimMapping: { firstName: ['hello'] }, claimsToStore: null }; // when - const { claimManager } = new OidcAuthenticationService(args); + const { claimManager } = new OidcAuthenticationService(args, { openIdClient }); const claims = claimManager.getMissingClaims(); // then @@ -72,7 +82,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const args = { claimMapping: { firstName: ['hello'] }, claimsToStore: 'employeeNumber,studentGroup' }; // when - const { claimManager } = new OidcAuthenticationService(args); + const { claimManager } = new OidcAuthenticationService(args, { openIdClient }); const claims = claimManager.getMissingClaims(); // then @@ -85,16 +95,19 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { context('when enabled in config', function () { it('returns true', function () { // given - const oidcAuthenticationService = new OidcAuthenticationService({ - clientId: 'anId', - clientSecret: 'aSecret', - additionalRequiredProperties: { - aProperty: 'a property value', + const oidcAuthenticationService = new OidcAuthenticationService( + { + clientId: 'anId', + clientSecret: 'aSecret', + additionalRequiredProperties: { + aProperty: 'a property value', + }, + enabled: true, + openidConfigurationUrl: 'https://example.net/.well-known/openid-configuration', + redirectUri: 'https://example.net/connexion/redirect', }, - enabled: true, - openidConfigurationUrl: 'https://example.net/.well-known/openid-configuration', - redirectUri: 'https://example.net/connexion/redirect', - }); + { openIdClient }, + ); // when const isOidcAuthenticationServiceReady = oidcAuthenticationService.isReady; @@ -107,7 +120,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { context('when not enabled in config', function () { it('returns false', function () { // given - const oidcAuthenticationService = new OidcAuthenticationService({}); + const oidcAuthenticationService = new OidcAuthenticationService({}, { openIdClient }); // when const result = oidcAuthenticationService.isReady; @@ -131,7 +144,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { .withArgs(payload, settings.authentication.secret, jwtOptions) .returns(accessToken); - const oidcAuthenticationService = new OidcAuthenticationService(settings.oidcExampleNet); + const oidcAuthenticationService = new OidcAuthenticationService(settings.oidcExampleNet, { openIdClient }); // when const result = oidcAuthenticationService.createAccessToken({ userId, audience }); @@ -147,7 +160,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { // given const userInfo = {}; const identityProvider = 'genericOidcProviderCode'; - const oidcAuthenticationService = new OidcAuthenticationService({ identityProvider }); + const oidcAuthenticationService = new OidcAuthenticationService({ identityProvider }, { openIdClient }); // when const result = oidcAuthenticationService.createAuthenticationComplement({ userInfo }); @@ -166,7 +179,10 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const claimsToStoreWithValues = { family_name, given_name }; const userInfo = { ...claimsToStoreWithValues }; const identityProvider = 'genericOidcProviderCode'; - const oidcAuthenticationService = new OidcAuthenticationService({ identityProvider, claimsToStore }); + const oidcAuthenticationService = new OidcAuthenticationService( + { identityProvider, claimsToStore }, + { openIdClient }, + ); // when const result = oidcAuthenticationService.createAuthenticationComplement({ userInfo }); @@ -188,6 +204,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const oidcAuthenticationService = new OidcAuthenticationService(settings.oidcExampleNet, { sessionTemporaryStorage, + openIdClient, }); await oidcAuthenticationService.initializeClientConfig(); @@ -211,13 +228,11 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { }; const postLogoutRedirectUriEncoded = encodeURIComponent(settings.oidcExampleNet.postLogoutRedirectUri); const endSessionUrl = `https://example.net/logout?post_logout_redirect_uri=${postLogoutRedirectUriEncoded}&id_token_hint=some_dummy_id_token`; - const clientInstance = { endSessionUrl: sinon.stub().resolves(endSessionUrl) }; - const Client = sinon.stub().returns(clientInstance); - - sinon.stub(Issuer, 'discover').resolves({ Client }); + openIdClient.buildEndSessionUrl.resolves(endSessionUrl); const oidcAuthenticationService = new OidcAuthenticationService(settings.oidcExampleNet, { sessionTemporaryStorage, + openIdClient, }); await oidcAuthenticationService.initializeClientConfig(); @@ -225,7 +240,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const result = await oidcAuthenticationService.getRedirectLogoutUrl({ userId, logoutUrlUUID }); // then - expect(clientInstance.endSessionUrl).to.have.been.calledWith({ + expect(openIdClient.buildEndSessionUrl).to.have.been.calledWith(MOCK_OPENID_CLIENT_CONFIG, { id_token_hint: idToken, post_logout_redirect_uri: settings.oidcExampleNet.postLogoutRedirectUri, }); @@ -245,14 +260,11 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { delete: sinon.stub().resolves(), }; const errorThrown = new Error('Fails to generate endSessionUrl'); - - const clientInstance = { endSessionUrl: sinon.stub().throws(errorThrown) }; - const Client = sinon.stub().returns(clientInstance); - - sinon.stub(Issuer, 'discover').resolves({ Client }); + openIdClient.buildEndSessionUrl.throws(errorThrown); const oidcAuthenticationService = new OidcAuthenticationService(settings.oidcExampleNet, { sessionTemporaryStorage, + openIdClient, }); await oidcAuthenticationService.initializeClientConfig(); @@ -298,24 +310,23 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { expiresIn, refreshToken, }); - const tokenSet = { + openIdClient.authorizationCodeGrant.resolves({ access_token: accessToken, expires_in: expiresIn, id_token: idToken, refresh_token: refreshToken, - }; - const clientInstance = { callback: sinon.stub().resolves(tokenSet) }; - const Client = sinon.stub().returns(clientInstance); - - sinon.stub(Issuer, 'discover').resolves({ Client }); - - const oidcAuthenticationService = new OidcAuthenticationService({ - clientSecret, - clientId, - redirectUri, - openidConfigurationUrl, - tokenUrl, }); + + const oidcAuthenticationService = new OidcAuthenticationService( + { + clientSecret, + clientId, + redirectUri, + openidConfigurationUrl, + tokenUrl, + }, + { openIdClient }, + ); await oidcAuthenticationService.initializeClientConfig(); // when @@ -347,19 +358,19 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { errorThrown.error_uri = '/oauth2/token'; errorThrown.response = 'api call response here'; - const clientInstance = { callback: sinon.stub().rejects(errorThrown) }; - const Client = sinon.stub().returns(clientInstance); - - sinon.stub(Issuer, 'discover').resolves({ Client }); + openIdClient.authorizationCodeGrant.rejects(errorThrown); - const oidcAuthenticationService = new OidcAuthenticationService({ - clientId, - clientSecret, - identityProvider, - redirectUri, - openidConfigurationUrl, - organizationName: 'Oidc Example', - }); + const oidcAuthenticationService = new OidcAuthenticationService( + { + clientId, + clientSecret, + identityProvider, + redirectUri, + openidConfigurationUrl, + organizationName: 'Oidc Example', + }, + { openIdClient }, + ); await oidcAuthenticationService.initializeClientConfig(); // when @@ -401,16 +412,17 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const clientSecret = 'OIDC_CLIENT_SECRET'; const redirectUri = 'https://example.org/please-redirect-to-me'; - const oidcAuthenticationService = new OidcAuthenticationService({ - clientId, - clientSecret, - redirectUri, - }); + openIdClient.buildAuthorizationUrl.returns(''); - const clientInstance = { authorizationUrl: sinon.stub().returns('') }; - const Client = sinon.stub().returns(clientInstance); + const oidcAuthenticationService = new OidcAuthenticationService( + { + clientId, + clientSecret, + redirectUri, + }, + { openIdClient }, + ); - sinon.stub(Issuer, 'discover').resolves({ Client }); await oidcAuthenticationService.initializeClientConfig(); // when @@ -420,7 +432,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { expect(nonce).to.match(uuidV4Regex); expect(state).to.match(uuidV4Regex); - expect(clientInstance.authorizationUrl).to.have.been.calledWithExactly({ + expect(openIdClient.buildAuthorizationUrl).to.have.been.calledWithExactly(MOCK_OPENID_CLIENT_CONFIG, { nonce, redirect_uri: 'https://example.org/please-redirect-to-me', scope: 'openid profile', @@ -438,19 +450,19 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const openidConfigurationUrl = Symbol('openidConfigurationUrl'); const errorThrown = new Error('Fails to generate authorization url'); - const clientInstance = { authorizationUrl: sinon.stub().throws(errorThrown) }; - const Client = sinon.stub().returns(clientInstance); + openIdClient.buildAuthorizationUrl.throws(errorThrown); - sinon.stub(Issuer, 'discover').resolves({ Client }); - - const oidcAuthenticationService = new OidcAuthenticationService({ - clientId, - clientSecret, - identityProvider, - redirectUri, - openidConfigurationUrl, - organizationName: 'Oidc Example', - }); + const oidcAuthenticationService = new OidcAuthenticationService( + { + clientId, + clientSecret, + identityProvider, + redirectUri, + openidConfigurationUrl, + organizationName: 'Oidc Example', + }, + { openIdClient }, + ); await oidcAuthenticationService.initializeClientConfig(); // when @@ -484,7 +496,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { 'secret', ); - const oidcAuthenticationService = new OidcAuthenticationService({}); + const oidcAuthenticationService = new OidcAuthenticationService({}, { openIdClient }); // when const result = await oidcAuthenticationService.getUserInfo({ @@ -514,7 +526,10 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { 'secret', ); - const oidcAuthenticationService = new OidcAuthenticationService({ claimsToStore: 'employeeNumber' }); + const oidcAuthenticationService = new OidcAuthenticationService( + { claimsToStore: 'employeeNumber' }, + { openIdClient }, + ); // when const result = await oidcAuthenticationService.getUserInfo({ @@ -550,7 +565,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { lastName: ['usual_name'], externalIdentityId: ['sub'], }; - const oidcAuthenticationService = new OidcAuthenticationService({ claimMapping }); + const oidcAuthenticationService = new OidcAuthenticationService({ claimMapping }, { openIdClient }); // when const result = await oidcAuthenticationService.getUserInfo({ @@ -586,10 +601,10 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { lastName: ['usual_name'], externalIdentityId: ['sub'], }; - const oidcAuthenticationService = new OidcAuthenticationService({ - claimMapping, - claimsToStore: 'employeeNumber', - }); + const oidcAuthenticationService = new OidcAuthenticationService( + { claimMapping, claimsToStore: 'employeeNumber' }, + { openIdClient }, + ); // when const result = await oidcAuthenticationService.getUserInfo({ @@ -618,18 +633,16 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { 'secret', ); - const oidcAuthenticationService = new OidcAuthenticationService({}); + const oidcAuthenticationService = new OidcAuthenticationService({}, { openIdClient }); sinon.stub(oidcAuthenticationService, '_getUserInfoFromEndpoint').resolves({}); // when - await oidcAuthenticationService.getUserInfo({ - idToken, - accessToken: 'accessToken', - }); + await oidcAuthenticationService.getUserInfo({ idToken, accessToken: 'accessToken' }); // then expect(oidcAuthenticationService._getUserInfoFromEndpoint).to.have.been.calledOnceWithExactly({ accessToken: 'accessToken', + expectedSubject: 'sub-id', }); }); }); @@ -647,7 +660,10 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { 'secret', ); - const oidcAuthenticationService = new OidcAuthenticationService({ claimsToStore: 'employeeNumber' }); + const oidcAuthenticationService = new OidcAuthenticationService( + { claimsToStore: 'employeeNumber' }, + { openIdClient }, + ); sinon.stub(oidcAuthenticationService, '_getUserInfoFromEndpoint').resolves({}); // when @@ -659,6 +675,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { // then expect(oidcAuthenticationService._getUserInfoFromEndpoint).to.have.been.calledOnceWithExactly({ accessToken: 'accessToken', + expectedSubject: 'sub-id', }); }); }); @@ -671,31 +688,37 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const clientSecret = 'OIDC_CLIENT_SECRET'; const redirectUri = 'https://example.org/please-redirect-to-me'; - const oidcAuthenticationService = new OidcAuthenticationService({ - clientId, - clientSecret, - redirectUri, + openIdClient.fetchUserInfo.returns({ + sub: 'sub-id', + given_name: 'givenName', + family_name: 'familyName', }); - const clientInstance = { - userinfo: sinon.stub().resolves({ - sub: 'sub-id', - given_name: 'givenName', - family_name: 'familyName', - }), - }; - const Client = sinon.stub().returns(clientInstance); + const oidcAuthenticationService = new OidcAuthenticationService( + { + clientId, + clientSecret, + redirectUri, + }, + { openIdClient }, + ); - sinon.stub(Issuer, 'discover').resolves({ Client }); await oidcAuthenticationService.initializeClientConfig(); const accessToken = 'thisIsSerializedInformation'; // when - const pickedUserInfo = await oidcAuthenticationService._getUserInfoFromEndpoint({ accessToken }); + const pickedUserInfo = await oidcAuthenticationService._getUserInfoFromEndpoint({ + accessToken, + expectedSubject: 'sub-id', + }); // then - expect(clientInstance.userinfo).to.have.been.calledOnceWithExactly(accessToken); + expect(openIdClient.fetchUserInfo).to.have.been.calledOnceWithExactly( + MOCK_OPENID_CLIENT_CONFIG, + accessToken, + 'sub-id', + ); expect(pickedUserInfo).to.deep.equal({ sub: 'sub-id', given_name: 'givenName', @@ -712,19 +735,19 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const openidConfigurationUrl = Symbol('openidConfigurationUrl'); const errorThrown = new Error('Fails to get user info'); - const clientInstance = { userinfo: sinon.stub().rejects(errorThrown) }; - const Client = sinon.stub().returns(clientInstance); + openIdClient.fetchUserInfo.rejects(errorThrown); - sinon.stub(Issuer, 'discover').resolves({ Client }); - - const oidcAuthenticationService = new OidcAuthenticationService({ - clientId, - clientSecret, - identityProvider, - redirectUri, - openidConfigurationUrl, - organizationName: 'Oidc Example', - }); + const oidcAuthenticationService = new OidcAuthenticationService( + { + clientId, + clientSecret, + identityProvider, + redirectUri, + openidConfigurationUrl, + organizationName: 'Oidc Example', + }, + { openIdClient }, + ); await oidcAuthenticationService.initializeClientConfig(); // when @@ -752,23 +775,22 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const redirectUri = 'https://example.org/please-redirect-to-me'; const organizationName = 'Example'; - const oidcAuthenticationService = new OidcAuthenticationService({ - clientId, - clientSecret, - redirectUri, - organizationName, + openIdClient.fetchUserInfo.returns({ + sub: 'sub-id', + given_name: 'givenName', + family_name: undefined, }); - const clientInstance = { - userinfo: sinon.stub().resolves({ - sub: 'sub-id', - given_name: 'givenName', - family_name: undefined, - }), - }; - const Client = sinon.stub().returns(clientInstance); + const oidcAuthenticationService = new OidcAuthenticationService( + { + clientId, + clientSecret, + redirectUri, + organizationName, + }, + { openIdClient }, + ); - sinon.stub(Issuer, 'discover').resolves({ Client }); await oidcAuthenticationService.initializeClientConfig(); const accessToken = 'thisIsSerializedInformation'; @@ -778,9 +800,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const error = await catchErr( oidcAuthenticationService._getUserInfoFromEndpoint, oidcAuthenticationService, - )({ - accessToken, - }); + )({ accessToken, expectedSubject: 'sub-id' }); // then expect(error).to.be.instanceOf(OidcMissingFieldsError); @@ -811,25 +831,23 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const redirectUri = 'https://example.org/please-redirect-to-me'; const organizationName = 'Example'; - const oidcAuthenticationService = new OidcAuthenticationService({ - claimsToStore: 'population', - clientId, - clientSecret, - redirectUri, - organizationName, + openIdClient.fetchUserInfo.returns({ + sub: 'sub-id', + given_name: 'givenName', + family_name: 'familyName', + population: '', }); - const clientInstance = { - userinfo: sinon.stub().resolves({ - sub: 'sub-id', - given_name: 'givenName', - family_name: 'familyName', - population: '', - }), - }; - const Client = sinon.stub().returns(clientInstance); - - sinon.stub(Issuer, 'discover').resolves({ Client }); + const oidcAuthenticationService = new OidcAuthenticationService( + { + claimsToStore: 'population', + clientId, + clientSecret, + redirectUri, + organizationName, + }, + { openIdClient }, + ); await oidcAuthenticationService.initializeClientConfig(); const accessToken = 'thisIsSerializedInformation'; @@ -839,9 +857,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const error = await catchErr( oidcAuthenticationService._getUserInfoFromEndpoint, oidcAuthenticationService, - )({ - accessToken, - }); + )({ accessToken, expectedSubject: 'sub-id' }); // then expect(error).to.be.instanceOf(OidcMissingFieldsError); @@ -899,7 +915,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { externalIdentifier: externalIdentityId, userId, }); - const oidcAuthenticationService = new OidcAuthenticationService({ identityProvider }); + const oidcAuthenticationService = new OidcAuthenticationService({ identityProvider }, { openIdClient }); // when const result = await oidcAuthenticationService.createUserAccount({ @@ -935,7 +951,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { externalIdentifier: externalIdentityId, userId, }); - const oidcAuthenticationService = new OidcAuthenticationService({ identityProvider }); + const oidcAuthenticationService = new OidcAuthenticationService({ identityProvider }, { openIdClient }); // when await oidcAuthenticationService.createUserAccount({ @@ -974,7 +990,10 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { externalIdentifier: externalIdentityId, userId, }); - const oidcAuthenticationService = new OidcAuthenticationService({ identityProvider, claimsToStore }); + const oidcAuthenticationService = new OidcAuthenticationService( + { identityProvider, claimsToStore }, + { openIdClient }, + ); // when await oidcAuthenticationService.createUserAccount({ @@ -1001,26 +1020,23 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const identityProvider = Symbol('identityProvider'); const redirectUri = Symbol('redirectUri'); const openidConfigurationUrl = Symbol('openidConfigurationUrl'); - const Client = sinon.spy(); - - sinon.stub(Issuer, 'discover').resolves({ Client }); - const oidcAuthenticationService = new OidcAuthenticationService({ - clientId, - clientSecret, - identityProvider, - redirectUri, - openidConfigurationUrl, - }); + const oidcAuthenticationService = new OidcAuthenticationService( + { + clientId, + clientSecret, + identityProvider, + redirectUri, + openidConfigurationUrl, + }, + { openIdClient }, + ); // when await oidcAuthenticationService.initializeClientConfig(); // then - expect(Issuer.discover).to.have.been.calledWithExactly(openidConfigurationUrl); - expect(Client).to.have.been.calledWithNew; - expect(Client).to.have.been.calledWithExactly({ - client_id: clientId, + expect(openIdClient.discovery).to.have.been.calledWithExactly(openidConfigurationUrl, clientId, { client_secret: clientSecret, redirect_uris: [redirectUri], }); @@ -1034,27 +1050,24 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const redirectUri = Symbol('redirectUri'); const openidConfigurationUrl = Symbol('openidConfigurationUrl'); const openidClientExtraMetadata = { token_endpoint_auth_method: 'client_secret_post' }; - const Client = sinon.spy(); - - sinon.stub(Issuer, 'discover').resolves({ Client }); - const oidcAuthenticationService = new OidcAuthenticationService({ - clientId, - clientSecret, - identityProvider, - redirectUri, - openidConfigurationUrl, - openidClientExtraMetadata, - }); + const oidcAuthenticationService = new OidcAuthenticationService( + { + clientId, + clientSecret, + identityProvider, + redirectUri, + openidConfigurationUrl, + openidClientExtraMetadata, + }, + { openIdClient }, + ); // when await oidcAuthenticationService.initializeClientConfig(); // then - expect(Issuer.discover).to.have.been.calledWithExactly(openidConfigurationUrl); - expect(Client).to.have.been.calledWithNew; - expect(Client).to.have.been.calledWithExactly({ - client_id: clientId, + expect(openIdClient.discovery).to.have.been.calledWithExactly(openidConfigurationUrl, clientId, { client_secret: clientSecret, redirect_uris: [redirectUri], token_endpoint_auth_method: 'client_secret_post', diff --git a/api/tests/identity-access-management/unit/domain/services/pix-authentication-service_test.js b/api/tests/identity-access-management/unit/domain/services/pix-authentication-service.test.js similarity index 100% rename from api/tests/identity-access-management/unit/domain/services/pix-authentication-service_test.js rename to api/tests/identity-access-management/unit/domain/services/pix-authentication-service.test.js diff --git a/api/tests/identity-access-management/unit/domain/services/pole-emploi-oidc-authentication-service_test.js b/api/tests/identity-access-management/unit/domain/services/pole-emploi-oidc-authentication-service.test.js similarity index 58% rename from api/tests/identity-access-management/unit/domain/services/pole-emploi-oidc-authentication-service_test.js rename to api/tests/identity-access-management/unit/domain/services/pole-emploi-oidc-authentication-service.test.js index 130419332ed..a3c136f126e 100644 --- a/api/tests/identity-access-management/unit/domain/services/pole-emploi-oidc-authentication-service_test.js +++ b/api/tests/identity-access-management/unit/domain/services/pole-emploi-oidc-authentication-service.test.js @@ -1,24 +1,37 @@ -import { Issuer } from 'openid-client'; - import { AuthenticationMethod } from '../../../../../src/identity-access-management/domain/models/AuthenticationMethod.js'; import { PoleEmploiOidcAuthenticationService } from '../../../../../src/identity-access-management/domain/services/pole-emploi-oidc-authentication-service.js'; import { config as settings } from '../../../../../src/shared/config.js'; import { expect, sinon } from '../../../../test-helper.js'; describe('Unit | Identity Access Management | Domain | Services | pole-emploi-oidc-authentication-service', function () { + let openIdClient; + + beforeEach(function () { + openIdClient = { + discovery: sinon.stub().resolves(Symbol('config')), + authorizationCodeGrant: sinon.stub(), + buildAuthorizationUrl: sinon.stub(), + buildEndSessionUrl: sinon.stub(), + fetchUserInfo: sinon.stub(), + }; + }); + describe('#constructor', function () { describe('when additionalRequiredProperties is not defined', function () { it('is not ready', async function () { // when - const oidcAuthenticationService = new PoleEmploiOidcAuthenticationService({ - ...settings.oidcExampleNet, - openidClientExtraMetadata: { token_endpoint_auth_method: 'client_secret_post' }, - identityProvider: 'POLE_EMPLOI', - organizationName: 'France Travail', - shouldCloseSession: true, - slug: 'pole-emploi', - source: 'pole_emploi_connect', - }); + const oidcAuthenticationService = new PoleEmploiOidcAuthenticationService( + { + ...settings.oidcExampleNet, + openidClientExtraMetadata: { token_endpoint_auth_method: 'client_secret_post' }, + identityProvider: 'POLE_EMPLOI', + organizationName: 'France Travail', + shouldCloseSession: true, + slug: 'pole-emploi', + source: 'pole_emploi_connect', + }, + { openIdClient }, + ); // then expect(oidcAuthenticationService.isReady).to.be.false; @@ -29,36 +42,35 @@ describe('Unit | Identity Access Management | Domain | Services | pole-emploi-oi describe('#initializeClientConfig', function () { it('creates an openid client config with extra metadata', async function () { // given - const Client = sinon.spy(); - - sinon.stub(Issuer, 'discover').resolves({ Client }); sinon.stub(settings, 'poleEmploi').value(settings.oidcExampleNet); - const poleEmploiOidcAuthenticationService = new PoleEmploiOidcAuthenticationService({ - ...settings.oidcExampleNet, - additionalRequiredProperties: { logoutUrl: '', afterLogoutUrl: '', sendingUrl: '' }, - openidClientExtraMetadata: { token_endpoint_auth_method: 'client_secret_post' }, - identityProvider: 'POLE_EMPLOI', - organizationName: 'France Travail', - shouldCloseSession: true, - slug: 'pole-emploi', - source: 'pole_emploi_connect', - }); + const poleEmploiOidcAuthenticationService = new PoleEmploiOidcAuthenticationService( + { + ...settings.oidcExampleNet, + additionalRequiredProperties: { logoutUrl: '', afterLogoutUrl: '', sendingUrl: '' }, + openidClientExtraMetadata: { token_endpoint_auth_method: 'client_secret_post' }, + identityProvider: 'POLE_EMPLOI', + organizationName: 'France Travail', + shouldCloseSession: true, + slug: 'pole-emploi', + source: 'pole_emploi_connect', + }, + { openIdClient }, + ); // when await poleEmploiOidcAuthenticationService.initializeClientConfig(); // then - expect(Issuer.discover).to.have.been.calledWithExactly( + expect(openIdClient.discovery).to.have.been.calledWithExactly( 'https://oidc.example.net/.well-known/openid-configuration', + 'client', + { + client_secret: 'secret', + redirect_uris: ['https://app.dev.pix.local/connexion/oidc-example-net'], + token_endpoint_auth_method: 'client_secret_post', + }, ); - expect(Client).to.have.been.calledWithNew; - expect(Client).to.have.been.calledWithExactly({ - client_id: 'client', - client_secret: 'secret', - redirect_uris: ['https://app.dev.pix.local/connexion/oidc-example-net'], - token_endpoint_auth_method: 'client_secret_post', - }); }); }); @@ -71,16 +83,19 @@ describe('Unit | Identity Access Management | Domain | Services | pole-emploi-oi expiresIn: 10, refreshToken: 'refreshToken', }; - const poleEmploiOidcAuthenticationService = new PoleEmploiOidcAuthenticationService({ - ...settings.oidcExampleNet, - additionalRequiredProperties: { logoutUrl: '', afterLogoutUrl: '', sendingUrl: '' }, - openidClientExtraMetadata: { token_endpoint_auth_method: 'client_secret_post' }, - identityProvider: 'POLE_EMPLOI', - organizationName: 'France Travail', - shouldCloseSession: true, - slug: 'pole-emploi', - source: 'pole_emploi_connect', - }); + const poleEmploiOidcAuthenticationService = new PoleEmploiOidcAuthenticationService( + { + ...settings.oidcExampleNet, + additionalRequiredProperties: { logoutUrl: '', afterLogoutUrl: '', sendingUrl: '' }, + openidClientExtraMetadata: { token_endpoint_auth_method: 'client_secret_post' }, + identityProvider: 'POLE_EMPLOI', + organizationName: 'France Travail', + shouldCloseSession: true, + slug: 'pole-emploi', + source: 'pole_emploi_connect', + }, + { openIdClient }, + ); // when const result = poleEmploiOidcAuthenticationService.createAuthenticationComplement({ sessionContent }); From 3c947f559a2bab69e469a2d37b8c2863be80d3e8 Mon Sep 17 00:00:00 2001 From: Benjamin Petetot Date: Fri, 7 Mar 2025 08:49:25 +0100 Subject: [PATCH 09/12] refactor(api): use mock instead of nock for openid-client in acceptance tests --- .../oidc-authentication-service-registry.js | 6 + .../oidc-provider.admin.route.test.js | 2 + .../application/oidc-provider.route.test.js | 237 ++++-------------- api/tests/test-helper.js | 4 +- .../openid-client-mocks.js} | 67 +++-- 5 files changed, 107 insertions(+), 209 deletions(-) rename api/tests/tooling/{server/hapi-server-with-test-oidc-provider.js => openid-client/openid-client-mocks.js} (50%) diff --git a/api/src/identity-access-management/domain/services/oidc-authentication-service-registry.js b/api/src/identity-access-management/domain/services/oidc-authentication-service-registry.js index 1ee77dc8351..4e1ae1b5ecb 100644 --- a/api/src/identity-access-management/domain/services/oidc-authentication-service-registry.js +++ b/api/src/identity-access-management/domain/services/oidc-authentication-service-registry.js @@ -88,4 +88,10 @@ export class OidcAuthenticationServiceRegistry { ); return true; } + + testOnly_reset() { + this.#allOidcProviderServices = null; + this.#readyOidcProviderServices = null; + this.#readyOidcProviderServicesForPixAdmin = null; + } } diff --git a/api/tests/identity-access-management/acceptance/application/oidc-provider.admin.route.test.js b/api/tests/identity-access-management/acceptance/application/oidc-provider.admin.route.test.js index 7590781799e..1b82e08611d 100644 --- a/api/tests/identity-access-management/acceptance/application/oidc-provider.admin.route.test.js +++ b/api/tests/identity-access-management/acceptance/application/oidc-provider.admin.route.test.js @@ -10,11 +10,13 @@ import { insertUserWithRoleSuperAdmin, knex, } from '../../../test-helper.js'; +import { createMockedTestOidcProvider } from '../../../tooling/openid-client/openid-client-mocks.js'; describe('Acceptance | Identity Access Management | Route | Admin | oidc-provider', function () { let server; beforeEach(async function () { + await createMockedTestOidcProvider(); server = await createServer(); }); diff --git a/api/tests/identity-access-management/acceptance/application/oidc-provider.route.test.js b/api/tests/identity-access-management/acceptance/application/oidc-provider.route.test.js index 0aebc6b08c8..5533b89bda2 100644 --- a/api/tests/identity-access-management/acceptance/application/oidc-provider.route.test.js +++ b/api/tests/identity-access-management/acceptance/application/oidc-provider.route.test.js @@ -2,7 +2,6 @@ import querystring from 'node:querystring'; import jsonwebtoken from 'jsonwebtoken'; -import { oidcAuthenticationServiceRegistry } from '../../../../lib/domain/usecases/index.js'; import { authenticationSessionService } from '../../../../src/identity-access-management/domain/services/authentication-session.service.js'; import { AuthenticationSessionContent } from '../../../../src/shared/domain/models/index.js'; import { decodeIfValid } from '../../../../src/shared/domain/services/token-service.js'; @@ -12,15 +11,16 @@ import { expect, generateAuthenticatedUserRequestHeaders, knex, - sinon, } from '../../../test-helper.js'; +import { createMockedTestOidcProvider } from '../../../tooling/openid-client/openid-client-mocks.js'; const UUID_PATTERN = new RegExp(/^[0-9A-F]{8}-[0-9A-F]{4}-4[0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}$/i); describe('Acceptance | Identity Access Management | Application | Route | oidc-provider', function () { - let server; + let server, openIdClientMock; beforeEach(async function () { + openIdClientMock = await createMockedTestOidcProvider(); server = await createServer(); }); @@ -81,9 +81,7 @@ describe('Acceptance | Identity Access Management | Application | Route | oidc-p describe('GET /api/oidc/authorization-url', function () { it('returns an object which contains the authentication url with an HTTP status code 200', async function () { // given - const query = querystring.stringify({ - identity_provider: 'OIDC_EXAMPLE_NET', - }); + const query = querystring.stringify({ identity_provider: 'OIDC_EXAMPLE_NET' }); // when const response = await server.inject({ @@ -112,17 +110,10 @@ describe('Acceptance | Identity Access Management | Application | Route | oidc-p }); describe('POST /api/oidc/token', function () { - let clock, payload, cookies, oidcExampleNetProvider; + let payload, cookies; beforeEach(async function () { - clock = sinon.useFakeTimers({ - now: Date.now(), - toFake: ['Date'], - }); - - const query = querystring.stringify({ - identity_provider: 'OIDC_EXAMPLE_NET', - }); + const query = querystring.stringify({ identity_provider: 'OIDC_EXAMPLE_NET' }); const authUrlResponse = await server.inject({ method: 'GET', url: `/api/oidc/authorization-url?${query}`, @@ -144,58 +135,20 @@ describe('Acceptance | Identity Access Management | Application | Route | oidc-p }, }, }; - - oidcExampleNetProvider = oidcAuthenticationServiceRegistry.getOidcProviderServiceByCode({ - identityProviderCode: 'OIDC_EXAMPLE_NET', - }); - sinon.stub(oidcExampleNetProvider.client, 'callback'); - }); - - afterEach(async function () { - clock.restore(); }); context('When user does not have an account', function () { it('returns status code 401 with authentication key matching session content and error code to validate cgu', async function () { // given - const idToken = jsonwebtoken.sign( - { - given_name: 'John', - family_name: 'Doe', - sub: 'sub', - }, - 'secret', - ); + const idToken = jsonwebtoken.sign({ given_name: 'John', family_name: 'Doe', sub: 'sub' }, 'secret'); - const getAccessTokenResponse = { + openIdClientMock.authorizationCodeGrant.resolves({ access_token: 'access_token', id_token: idToken, expires_in: 60, refresh_token: 'refresh_token', - }; - - /* - Le code ci-dessous a été commenté parce qu'on utilise un fournisseur d'identité - non valide d'exemple et l'utilisation de nock n'est pas possible car la librairie - openid-client tentera de valider le token reçu avec une configuration de chiffrement - d'exemple. - */ - //nock('https://oidc.example.net').post('/ea5ac20c-5076-4806-860a-b0aeb01645d4/oauth2/v2.0/token').reply(200, getAccessTokenResponse); - oidcExampleNetProvider.client.callback.resolves(getAccessTokenResponse); + }); - const sessionContentAndUserInfo = { - sessionContent: new AuthenticationSessionContent({ - accessToken: 'access_token', - idToken, - expiresIn: 60, - refreshToken: 'refresh_token', - }), - userInfo: { - externalIdentityId: 'sub', - firstName: 'John', - lastName: 'Doe', - }, - }; const headers = generateAuthenticatedUserRequestHeaders(); headers.cookie = cookies[0]; @@ -219,8 +172,21 @@ describe('Acceptance | Identity Access Management | Application | Route | oidc-p const authenticationKey = error.meta.authenticationKey; expect(authenticationKey).to.exist; + const result = await authenticationSessionService.getByKey(authenticationKey); - expect(result).to.deep.equal(sessionContentAndUserInfo); + expect(result).to.deep.equal({ + sessionContent: new AuthenticationSessionContent({ + accessToken: 'access_token', + idToken, + expiresIn: 60, + refreshToken: 'refresh_token', + }), + userInfo: { + externalIdentityId: 'sub', + firstName: 'John', + lastName: 'Doe', + }, + }); }); }); @@ -231,43 +197,25 @@ describe('Acceptance | Identity Access Management | Application | Route | oidc-p const lastName = 'Doe'; const externalIdentifier = 'sub'; - const userId = databaseBuilder.factory.buildUser({ - firstName, - lastName, - }).id; - + const userId = databaseBuilder.factory.buildUser({ firstName, lastName }).id; databaseBuilder.factory.buildAuthenticationMethod.withIdentityProvider({ identityProvider: 'OIDC_EXAMPLE_NET', externalIdentifier, - accessToken: 'access_token', - refreshToken: 'refresh_token', - expiresIn: 1000, userId, }); await databaseBuilder.commit(); const idToken = jsonwebtoken.sign( - { - given_name: firstName, - family_name: lastName, - sub: externalIdentifier, - }, + { given_name: firstName, family_name: lastName, sub: externalIdentifier }, 'secret', ); - const getAccessTokenResponse = { + + openIdClientMock.authorizationCodeGrant.resolves({ access_token: 'access_token', id_token: idToken, expires_in: 60, refresh_token: 'refresh_token', - }; - /* - Le code ci-dessous a été commenté parce qu'on utilise un fournisseur d'identité - non valide d'exemple et l'utilisation de nock n'est pas possible car la librairie - openid-client tentera de valider le token reçu avec une configuration de chiffrement - d'exemple. - */ - // const getAccessTokenRequest = nock(settings.poleEmploi.tokenUrl).post('/').reply(200, getAccessTokenResponse); - oidcExampleNetProvider.client.callback.resolves(getAccessTokenResponse); + }); // when const response = await server.inject({ @@ -282,19 +230,11 @@ describe('Acceptance | Identity Access Management | Application | Route | oidc-p }); // then - /* - Le code ci-dessous a été commenté parce qu'on utilise un fournisseur d'identité - non valide d'exemple et l'utilisation de nock n'est pas possible car la librairie - openid-client tentera de valider le token reçu avec une configuration de chiffrement - d'exemple. - */ - // expect(getAccessTokenRequest.isDone()).to.be.true; - expect(oidcExampleNetProvider.client.callback).to.have.been.calledOnce; + expect(openIdClientMock.authorizationCodeGrant).to.have.been.calledOnce; expect(response.result.access_token).to.exist; + const decodedAccessToken = await decodeIfValid(response.result.access_token); - expect(decodedAccessToken).to.include({ - aud: 'https://orga.pix.fr', - }); + expect(decodedAccessToken).to.include({ aud: 'https://orga.pix.fr' }); expect(response.statusCode).to.equal(200); expect(response.result['logout_url_uuid']).to.match(UUID_PATTERN); }); @@ -308,43 +248,25 @@ describe('Acceptance | Identity Access Management | Application | Route | oidc-p const lastName = 'Doe'; const externalIdentifier = 'sub'; - const userId = databaseBuilder.factory.buildUser({ - firstName, - lastName, - }).id; - + const userId = databaseBuilder.factory.buildUser({ firstName, lastName }).id; databaseBuilder.factory.buildAuthenticationMethod.withIdentityProvider({ identityProvider: 'OIDC_EXAMPLE_NET', externalIdentifier, - accessToken: 'access_token', - refreshToken: 'refresh_token', - expiresIn: 1000, userId, }); await databaseBuilder.commit(); const idToken = jsonwebtoken.sign( - { - given_name: firstName, - family_name: lastName, - sub: externalIdentifier, - }, + { given_name: firstName, family_name: lastName, sub: externalIdentifier }, 'secret', ); - const getAccessTokenResponse = { + + openIdClientMock.authorizationCodeGrant.resolves({ access_token: 'access_token', id_token: idToken, expires_in: 60, refresh_token: 'refresh_token', - }; - /* - Le code ci-dessous a été commenté parce qu'on utilise un fournisseur d'identité - non valide d'exemple et l'utilisation de nock n'est pas possible car la librairie - openid-client tentera de valider le token reçu avec une configuration de chiffrement - d'exemple. - */ - // const getAccessTokenRequest = nock(settings.poleEmploi.tokenUrl).post('/').reply(200, getAccessTokenResponse); - oidcExampleNetProvider.client.callback.resolves(getAccessTokenResponse); + }); const headers = generateAuthenticatedUserRequestHeaders(); headers.cookie = cookies[0]; @@ -362,14 +284,7 @@ describe('Acceptance | Identity Access Management | Application | Route | oidc-p }); // then - /* - Le code ci-dessous a été commenté parce qu'on utilise un fournisseur d'identité - non valide d'exemple et l'utilisation de nock n'est pas possible car la librairie - openid-client tentera de valider le token reçu avec une configuration de chiffrement - d'exemple. - */ - // expect(getAccessTokenRequest.isDone()).to.be.true; - expect(oidcExampleNetProvider.client.callback).to.have.been.calledOnce; + expect(openIdClientMock.authorizationCodeGrant).to.have.been.calledOnce; expect(response.statusCode).to.equal(403); }); }); @@ -381,45 +296,25 @@ describe('Acceptance | Identity Access Management | Application | Route | oidc-p const lastName = 'Doe'; const externalIdentifier = 'sub'; - const userId = databaseBuilder.factory.buildUser.withRole({ - firstName, - lastName, - role: 'SUPER_ADMIN', - }).id; - + const userId = databaseBuilder.factory.buildUser.withRole({ firstName, lastName, role: 'SUPER_ADMIN' }).id; databaseBuilder.factory.buildAuthenticationMethod.withIdentityProvider({ identityProvider: 'OIDC_EXAMPLE_NET', externalIdentifier, - accessToken: 'access_token', - refreshToken: 'refresh_token', - expiresIn: 1000, userId, }); - await databaseBuilder.commit(); const idToken = jsonwebtoken.sign( - { - given_name: firstName, - family_name: lastName, - sub: externalIdentifier, - }, + { given_name: firstName, family_name: lastName, sub: externalIdentifier }, 'secret', ); - const getAccessTokenResponse = { + + openIdClientMock.authorizationCodeGrant.resolves({ access_token: 'access_token', id_token: idToken, expires_in: 60, refresh_token: 'refresh_token', - }; - /* - Le code ci-dessous a été commenté parce qu'on utilise un fournisseur d'identité - non valide d'exemple et l'utilisation de nock n'est pas possible car la librairie - openid-client tentera de valider le token reçu avec une configuration de chiffrement - d'exemple. - */ - // const getAccessTokenRequest = nock(settings.poleEmploi.tokenUrl).post('/').reply(200, getAccessTokenResponse); - oidcExampleNetProvider.client.callback.resolves(getAccessTokenResponse); + }); // when const response = await server.inject({ @@ -430,19 +325,11 @@ describe('Acceptance | Identity Access Management | Application | Route | oidc-p }); // then - /* - Le code ci-dessous a été commenté parce qu'on utilise un fournisseur d'identité - non valide d'exemple et l'utilisation de nock n'est pas possible car la librairie - openid-client tentera de valider le token reçu avec une configuration de chiffrement - d'exemple. - */ - // expect(getAccessTokenRequest.isDone()).to.be.true; - expect(oidcExampleNetProvider.client.callback).to.have.been.calledOnce; + expect(openIdClientMock.authorizationCodeGrant).to.have.been.calledOnce; expect(response.result.access_token).to.exist; + const decodedAccessToken = await decodeIfValid(response.result.access_token); - expect(decodedAccessToken).to.include({ - aud: 'https://admin.pix.fr', - }); + expect(decodedAccessToken).to.include({ aud: 'https://admin.pix.fr' }); expect(response.statusCode).to.equal(200); }); }); @@ -456,18 +343,11 @@ describe('Acceptance | Identity Access Management | Application | Route | oidc-p const lastName = 'Glace'; const externalIdentifier = 'sub'; const idToken = jsonwebtoken.sign( - { - given_name: firstName, - family_name: lastName, - nonce: 'nonce', - sub: externalIdentifier, - }, + { given_name: firstName, family_name: lastName, nonce: 'nonce', sub: externalIdentifier }, 'secret', ); - const sessionContent = new AuthenticationSessionContent({ - idToken, - }); + const sessionContent = new AuthenticationSessionContent({ idToken }); const userAuthenticationKey = await authenticationSessionService.save({ sessionContent, userInfo: { @@ -504,9 +384,7 @@ describe('Acceptance | Identity Access Management | Application | Route | oidc-p expect(response.statusCode).to.equal(200); expect(response.result.access_token).to.exist; const decodedAccessToken = await decodeIfValid(response.result.access_token); - expect(decodedAccessToken).to.include({ - aud: 'https://app.pix.fr', - }); + expect(decodedAccessToken).to.include({ aud: 'https://app.pix.fr' }); const createdUser = await knex('users').first(); expect(createdUser.firstName).to.equal('Brice'); @@ -555,12 +433,7 @@ describe('Acceptance | Identity Access Management | Application | Route | oidc-p await databaseBuilder.commit(); const idToken = jsonwebtoken.sign( - { - given_name: 'Brice', - family_name: 'Glace', - nonce: 'nonce', - sub: 'some-user-unique-id', - }, + { given_name: 'Brice', family_name: 'Glace', nonce: 'nonce', sub: 'some-user-unique-id' }, 'secret', ); const userAuthenticationKey = await authenticationSessionService.save({ @@ -609,12 +482,7 @@ describe('Acceptance | Identity Access Management | Application | Route | oidc-p await databaseBuilder.commit(); const idToken = jsonwebtoken.sign( - { - given_name: 'Brice', - family_name: 'Glace', - nonce: 'nonce', - sub: 'some-user-unique-id', - }, + { given_name: 'Brice', family_name: 'Glace', nonce: 'nonce', sub: 'some-user-unique-id' }, 'secret', ); const userAuthenticationKey = await authenticationSessionService.save({ @@ -649,10 +517,9 @@ describe('Acceptance | Identity Access Management | Application | Route | oidc-p // then expect(response.statusCode).to.equal(200); expect(response.result.access_token).to.exist; + const decodedAccessToken = await decodeIfValid(response.result.access_token); - expect(decodedAccessToken).to.include({ - aud: 'https://app.pix.fr', - }); + expect(decodedAccessToken).to.include({ aud: 'https://app.pix.fr' }); expect(response.result['logout_url_uuid']).to.match(UUID_PATTERN); }); }); diff --git a/api/tests/test-helper.js b/api/tests/test-helper.js index 18755c44f22..bc99a2ae94f 100644 --- a/api/tests/test-helper.js +++ b/api/tests/test-helper.js @@ -21,6 +21,7 @@ import { knex as datamartKnex } from '../datamart/knex-database-connection.js'; import { knex as datawarehouseKnex } from '../datawarehouse/knex-database-connection.js'; import { DatabaseBuilder } from '../db/database-builder/database-builder.js'; import { disconnect, knex } from '../db/knex-database-connection.js'; +import { createServer } from '../server.js'; import { createMaddoServer } from '../server.maddo.js'; import { PIX_ADMIN } from '../src/authorization/domain/constants.js'; import * as tutorialRepository from '../src/devcomp/infrastructure/repositories/tutorial-repository.js'; @@ -42,7 +43,6 @@ import * as domainBuilder from './tooling/domain-builder/factory/index.js'; import { jobChai } from './tooling/jobs/expect-job.js'; import { buildLearningContent as learningContentBuilder } from './tooling/learning-content-builder/index.js'; import { increaseCurrentTestTimeout } from './tooling/mocha-tools.js'; -import { createServerWithTestOidcProvider } from './tooling/server/hapi-server-with-test-oidc-provider.js'; import { HttpTestServer } from './tooling/server/http-test-server.js'; import { createTempFile, removeTempFile } from './tooling/temporary-file.js'; @@ -334,7 +334,7 @@ export { catchErr, catchErrSync, createMaddoServer, - createServerWithTestOidcProvider as createServer, + createServer, createTempFile, databaseBuilder, datamartBuilder, diff --git a/api/tests/tooling/server/hapi-server-with-test-oidc-provider.js b/api/tests/tooling/openid-client/openid-client-mocks.js similarity index 50% rename from api/tests/tooling/server/hapi-server-with-test-oidc-provider.js rename to api/tests/tooling/openid-client/openid-client-mocks.js index e527ee5b32a..41e079ca12a 100644 --- a/api/tests/tooling/server/hapi-server-with-test-oidc-provider.js +++ b/api/tests/tooling/openid-client/openid-client-mocks.js @@ -1,9 +1,10 @@ -import nock from 'nock'; - import { oidcAuthenticationServiceRegistry } from '../../../lib/domain/usecases/index.js'; -import { createServer } from '../../../server.js'; import { OidcAuthenticationService } from '../../../src/identity-access-management/domain/services/oidc-authentication-service.js'; +import { sinon } from '../../test-helper.js'; +const clientId = 'client'; +const redirectUri = 'https://app.dev.pix.org/connexion/oidc-example-net'; +const scope = 'openid profile'; const openIdConfigurationResponse = { token_endpoint: 'https://oidc.example.net/ea5ac20c-5076-4806-860a-b0aeb01645d4/oauth2/v2.0/token', token_endpoint_auth_methods_supported: ['client_secret_post', 'private_key_jwt', 'client_secret_basic'], @@ -40,28 +41,50 @@ const openIdConfigurationResponse = { ], }; -async function createServerWithTestOidcProvider() { - nock('https://oidc.example.net').get('/.well-known/openid-configuration').reply(200, openIdConfigurationResponse); +function createOpenIdClientMock() { + return { + discovery: sinon.stub(), + authorizationCodeGrant: sinon.stub(), + buildAuthorizationUrl: sinon.stub(), + buildEndSessionUrl: sinon.stub(), + fetchUserInfo: sinon.stub(), + }; +} + +async function createMockedTestOidcProvider() { + oidcAuthenticationServiceRegistry.testOnly_reset(); + + const openIdClientMock = createOpenIdClientMock(); + openIdClientMock.discovery.resolves(openIdConfigurationResponse); + + const authorizationUrl = `${openIdConfigurationResponse.authorization_endpoint}?client_id=${clientId}&redirect_uri=${redirectUri}&response_type=code&scope=${scope}&state=state&nonce=nonce`; + openIdClientMock.buildAuthorizationUrl.returns(authorizationUrl); + + const endSessionUrl = `${openIdConfigurationResponse.end_session_endpoint}?client_id=${clientId}`; + openIdClientMock.buildEndSessionUrl.returns(endSessionUrl); await oidcAuthenticationServiceRegistry.loadOidcProviderServices([ - new OidcAuthenticationService({ - accessTokenLifespanMs: 60000, - clientId: 'client', - clientSecret: 'secret', - enabled: true, - enabledForPixAdmin: true, - configKey: 'oidcExampleNet', - shouldCloseSession: true, - identityProvider: 'OIDC_EXAMPLE_NET', - openidConfigurationUrl: 'https://oidc.example.net/.well-known/openid-configuration', - organizationName: 'OIDC Example', - redirectUri: 'https://app.dev.pix.org/connexion/oidc-example-net', - slug: 'oidc-example-net', - source: 'oidcexamplenet', - }), + new OidcAuthenticationService( + { + accessTokenLifespanMs: 60000, + clientId, + clientSecret: 'secret', + enabled: true, + enabledForPixAdmin: true, + configKey: 'oidcExampleNet', + shouldCloseSession: true, + identityProvider: 'OIDC_EXAMPLE_NET', + openidConfigurationUrl: 'https://oidc.example.net/.well-known/openid-configuration', + organizationName: 'OIDC Example', + redirectUri, + slug: 'oidc-example-net', + source: 'oidcexamplenet', + }, + { openIdClient: openIdClientMock }, + ), ]); - return createServer(); + return openIdClientMock; } -export { createServerWithTestOidcProvider }; +export { createMockedTestOidcProvider }; From 5e8611fa4f1dbc927a727f5a2a15427eecbdff6f Mon Sep 17 00:00:00 2001 From: Benjamin Petetot Date: Fri, 7 Mar 2025 08:56:34 +0100 Subject: [PATCH 10/12] refactor(api): factorize openid-client mock --- .../oidc-authentication-service.test.js | 17 ++++++----------- ...e-emploi-oidc-authentication-service.test.js | 9 ++------- .../openid-client/openid-client-mocks.js | 9 ++++----- 3 files changed, 12 insertions(+), 23 deletions(-) diff --git a/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service.test.js b/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service.test.js index 83ef337c10c..8a3cb84ab31 100644 --- a/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service.test.js +++ b/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service.test.js @@ -13,22 +13,17 @@ import { } from '../../../../../src/shared/domain/models/index.js'; import { monitoringTools } from '../../../../../src/shared/infrastructure/monitoring-tools.js'; import { catchErr, catchErrSync, expect, sinon } from '../../../../test-helper.js'; +import { createOpenIdClientMock } from '../../../../tooling/openid-client/openid-client-mocks.js'; const uuidV4Regex = /^[0-9A-F]{8}-[0-9A-F]{4}-4[0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}$/i; -const MOCK_OPENID_CLIENT_CONFIG = Symbol('config'); +const MOCK_OIDC_PROVIDER_CONFIG = Symbol('config'); describe('Unit | Domain | Services | oidc-authentication-service', function () { let openIdClient; beforeEach(function () { - openIdClient = { - discovery: sinon.stub().resolves(MOCK_OPENID_CLIENT_CONFIG), - authorizationCodeGrant: sinon.stub(), - buildAuthorizationUrl: sinon.stub(), - buildEndSessionUrl: sinon.stub(), - fetchUserInfo: sinon.stub(), - }; + openIdClient = createOpenIdClientMock(MOCK_OIDC_PROVIDER_CONFIG); sinon.stub(monitoringTools, 'logErrorWithCorrelationIds'); }); @@ -240,7 +235,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const result = await oidcAuthenticationService.getRedirectLogoutUrl({ userId, logoutUrlUUID }); // then - expect(openIdClient.buildEndSessionUrl).to.have.been.calledWith(MOCK_OPENID_CLIENT_CONFIG, { + expect(openIdClient.buildEndSessionUrl).to.have.been.calledWith(MOCK_OIDC_PROVIDER_CONFIG, { id_token_hint: idToken, post_logout_redirect_uri: settings.oidcExampleNet.postLogoutRedirectUri, }); @@ -432,7 +427,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { expect(nonce).to.match(uuidV4Regex); expect(state).to.match(uuidV4Regex); - expect(openIdClient.buildAuthorizationUrl).to.have.been.calledWithExactly(MOCK_OPENID_CLIENT_CONFIG, { + expect(openIdClient.buildAuthorizationUrl).to.have.been.calledWithExactly(MOCK_OIDC_PROVIDER_CONFIG, { nonce, redirect_uri: 'https://example.org/please-redirect-to-me', scope: 'openid profile', @@ -715,7 +710,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { // then expect(openIdClient.fetchUserInfo).to.have.been.calledOnceWithExactly( - MOCK_OPENID_CLIENT_CONFIG, + MOCK_OIDC_PROVIDER_CONFIG, accessToken, 'sub-id', ); diff --git a/api/tests/identity-access-management/unit/domain/services/pole-emploi-oidc-authentication-service.test.js b/api/tests/identity-access-management/unit/domain/services/pole-emploi-oidc-authentication-service.test.js index a3c136f126e..a23cbd1b4f8 100644 --- a/api/tests/identity-access-management/unit/domain/services/pole-emploi-oidc-authentication-service.test.js +++ b/api/tests/identity-access-management/unit/domain/services/pole-emploi-oidc-authentication-service.test.js @@ -2,18 +2,13 @@ import { AuthenticationMethod } from '../../../../../src/identity-access-managem import { PoleEmploiOidcAuthenticationService } from '../../../../../src/identity-access-management/domain/services/pole-emploi-oidc-authentication-service.js'; import { config as settings } from '../../../../../src/shared/config.js'; import { expect, sinon } from '../../../../test-helper.js'; +import { createOpenIdClientMock } from '../../../../tooling/openid-client/openid-client-mocks.js'; describe('Unit | Identity Access Management | Domain | Services | pole-emploi-oidc-authentication-service', function () { let openIdClient; beforeEach(function () { - openIdClient = { - discovery: sinon.stub().resolves(Symbol('config')), - authorizationCodeGrant: sinon.stub(), - buildAuthorizationUrl: sinon.stub(), - buildEndSessionUrl: sinon.stub(), - fetchUserInfo: sinon.stub(), - }; + openIdClient = createOpenIdClientMock(); }); describe('#constructor', function () { diff --git a/api/tests/tooling/openid-client/openid-client-mocks.js b/api/tests/tooling/openid-client/openid-client-mocks.js index 41e079ca12a..831238f485b 100644 --- a/api/tests/tooling/openid-client/openid-client-mocks.js +++ b/api/tests/tooling/openid-client/openid-client-mocks.js @@ -41,9 +41,9 @@ const openIdConfigurationResponse = { ], }; -function createOpenIdClientMock() { +function createOpenIdClientMock(oidcProviderConfig = Symbol('oidcProviderConfig')) { return { - discovery: sinon.stub(), + discovery: sinon.stub().resolves(oidcProviderConfig), authorizationCodeGrant: sinon.stub(), buildAuthorizationUrl: sinon.stub(), buildEndSessionUrl: sinon.stub(), @@ -54,8 +54,7 @@ function createOpenIdClientMock() { async function createMockedTestOidcProvider() { oidcAuthenticationServiceRegistry.testOnly_reset(); - const openIdClientMock = createOpenIdClientMock(); - openIdClientMock.discovery.resolves(openIdConfigurationResponse); + const openIdClientMock = createOpenIdClientMock(openIdConfigurationResponse); const authorizationUrl = `${openIdConfigurationResponse.authorization_endpoint}?client_id=${clientId}&redirect_uri=${redirectUri}&response_type=code&scope=${scope}&state=state&nonce=nonce`; openIdClientMock.buildAuthorizationUrl.returns(authorizationUrl); @@ -87,4 +86,4 @@ async function createMockedTestOidcProvider() { return openIdClientMock; } -export { createMockedTestOidcProvider }; +export { createMockedTestOidcProvider, createOpenIdClientMock }; From ce2e594cbb3e15aec92e7aaacd7407fd5b5775c9 Mon Sep 17 00:00:00 2001 From: Benjamin Petetot Date: Fri, 7 Mar 2025 09:13:22 +0100 Subject: [PATCH 11/12] refactor(api): check openid config initialization --- .../oidc-authentication-service-registry.js | 2 +- .../services/oidc-authentication-service.js | 31 ++++++++----------- ...dc-authentication-service-registry.test.js | 20 ------------ 3 files changed, 14 insertions(+), 39 deletions(-) diff --git a/api/src/identity-access-management/domain/services/oidc-authentication-service-registry.js b/api/src/identity-access-management/domain/services/oidc-authentication-service-registry.js index 4e1ae1b5ecb..2829f7c10e1 100644 --- a/api/src/identity-access-management/domain/services/oidc-authentication-service-registry.js +++ b/api/src/identity-access-management/domain/services/oidc-authentication-service-registry.js @@ -23,9 +23,9 @@ export class OidcAuthenticationServiceRegistry { ); if (!oidcProviderService) return; - if (oidcProviderService.isClientConfigInitialized) return; // TODO À mettre dans .initializeClientConfig() ? await oidcProviderService.initializeClientConfig(); + return true; } diff --git a/api/src/identity-access-management/domain/services/oidc-authentication-service.js b/api/src/identity-access-management/domain/services/oidc-authentication-service.js index 4361ea8eb5f..3708b18d93d 100644 --- a/api/src/identity-access-management/domain/services/oidc-authentication-service.js +++ b/api/src/identity-access-management/domain/services/oidc-authentication-service.js @@ -20,18 +20,11 @@ const DEFAULT_SCOPE = 'openid profile'; const defaultSessionTemporaryStorage = temporaryStorage.withPrefix('oidc-session:'); -// TODO: A verifier si tous les parametre de la configuration sont bien utilisé -// -// On ne retrouve pas dans le code source de la plateforme les propriétés suivantes: -// "openidClientExtraMetadata": { -// "token_endpoint_auth_method": "client_secret_post" -// }, - export class OidcAuthenticationService { #isReady = false; #isReadyForPixAdmin = false; #openIdClient; - #oidcClientConfig; // TODO: À modifier en openIdClientConfig + #openIdClientConfig; constructor( { @@ -115,11 +108,9 @@ export class OidcAuthenticationService { return this.#isReadyForPixAdmin; } - get isClientConfigInitialized() { - return !!this.#oidcClientConfig; - } - async initializeClientConfig() { + if (this.#openIdClientConfig) return; + try { const metadata = { client_secret: this.clientSecret, @@ -127,7 +118,11 @@ export class OidcAuthenticationService { ...this.openidClientExtraMetadata, }; - this.#oidcClientConfig = await this.#openIdClient.discovery(this.openidConfigurationUrl, this.clientId, metadata); + this.#openIdClientConfig = await this.#openIdClient.discovery( + this.openidConfigurationUrl, + this.clientId, + metadata, + ); } catch (error) { logger.error(`OIDC Provider "${this.identityProvider}" is UNAVAILABLE: ${error}`); } @@ -161,7 +156,7 @@ export class OidcAuthenticationService { const tokenEndpointParameters = { code, state, iss }; tokenSet = await this.#openIdClient.authorizationCodeGrant( - this.#oidcClientConfig, + this.#openIdClientConfig, this.redirectUri, checks, tokenEndpointParameters, @@ -192,7 +187,7 @@ export class OidcAuthenticationService { getAuthorizationUrl() { const state = randomUUID(); - const nonce = randomUUID(); // TODO: this.#openIdClient.randomNonce() ? + const nonce = randomUUID(); let redirectTarget; @@ -205,7 +200,7 @@ export class OidcAuthenticationService { ...this.extraAuthorizationUrlParameters, }; - redirectTarget = this.#openIdClient.buildAuthorizationUrl(this.#oidcClientConfig, parameters); + redirectTarget = this.#openIdClient.buildAuthorizationUrl(this.#openIdClientConfig, parameters); } catch (error) { _monitorOidcError(error.message, { data: { organizationName: this.organizationName }, @@ -275,7 +270,7 @@ export class OidcAuthenticationService { } try { - const endSessionUrl = this.#openIdClient.buildEndSessionUrl(this.#oidcClientConfig, parameters); + const endSessionUrl = this.#openIdClient.buildEndSessionUrl(this.#openIdClientConfig, parameters); await this.sessionTemporaryStorage.delete(key); @@ -294,7 +289,7 @@ export class OidcAuthenticationService { let userInfo; try { - userInfo = await this.#openIdClient.fetchUserInfo(this.#oidcClientConfig, accessToken, expectedSubject); + userInfo = await this.#openIdClient.fetchUserInfo(this.#openIdClientConfig, accessToken, expectedSubject); } catch (error) { _monitorOidcError(error.message, { data: { organizationName: this.organizationName }, diff --git a/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service-registry.test.js b/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service-registry.test.js index e62f73e7938..747b0d805b2 100644 --- a/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service-registry.test.js +++ b/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service-registry.test.js @@ -132,26 +132,6 @@ describe('Unit | Identity Access Management | Domain | Services | oidc-authentic expect(result).to.be.true; expect(initializeClientConfig).to.have.been.calledOnce; }); - - context('when there is already a client instantiated', function () { - it('returns undefined', async function () { - // given - const oidcProviderServices = [ - { - code: 'OIDC', - isReady: true, - isClientConfigInitialized: true, - }, - ]; - await oidcAuthenticationServiceRegistry.loadOidcProviderServices(oidcProviderServices); - - // when - const result = await oidcAuthenticationServiceRegistry.configureReadyOidcProviderServiceByCode('OIDC'); - - // then - expect(result).to.be.undefined; - }); - }); }); }); }); From 238ccdeff9b464fe154d677bf576ab40495a4086 Mon Sep 17 00:00:00 2001 From: Benjamin Petetot Date: Fri, 7 Mar 2025 14:52:27 +0100 Subject: [PATCH 12/12] refactor(api): fix usage of authorizationCodeGrant --- .../services/oidc-authentication-service.js | 52 ++++------ .../oidc-authentication-service.test.js | 96 ++++++++++--------- ...emploi-oidc-authentication-service.test.js | 3 +- 3 files changed, 75 insertions(+), 76 deletions(-) diff --git a/api/src/identity-access-management/domain/services/oidc-authentication-service.js b/api/src/identity-access-management/domain/services/oidc-authentication-service.js index 3708b18d93d..941edc14f6e 100644 --- a/api/src/identity-access-management/domain/services/oidc-authentication-service.js +++ b/api/src/identity-access-management/domain/services/oidc-authentication-service.js @@ -114,12 +114,11 @@ export class OidcAuthenticationService { try { const metadata = { client_secret: this.clientSecret, - redirect_uris: [this.redirectUri], ...this.openidClientExtraMetadata, }; this.#openIdClientConfig = await this.#openIdClient.discovery( - this.openidConfigurationUrl, + new URL(this.openidConfigurationUrl), this.clientId, metadata, ); @@ -149,18 +148,26 @@ export class OidcAuthenticationService { } async exchangeCodeForTokens({ code, state, iss, nonce, sessionState }) { - let tokenSet; - try { - const checks = { nonce, state: sessionState }; - const tokenEndpointParameters = { code, state, iss }; + const currentUrl = new URL(this.redirectUri); + currentUrl.searchParams.append('code', code); + currentUrl.searchParams.append('state', state); + currentUrl.searchParams.append('session_state', sessionState); + + const checks = { expectedNonce: nonce, expectedState: sessionState }; - tokenSet = await this.#openIdClient.authorizationCodeGrant( + const tokenResponse = await this.#openIdClient.authorizationCodeGrant( this.#openIdClientConfig, - this.redirectUri, + currentUrl, checks, - tokenEndpointParameters, ); + + return new AuthenticationSessionContent({ + accessToken: tokenResponse.access_token, + expiresIn: tokenResponse.expires_in, + idToken: tokenResponse.id_token, + refreshToken: tokenResponse.refresh_token, + }); } catch (error) { _monitorOidcError(error.message, { data: { code, nonce, organizationName: this.organizationName, sessionState, state, iss }, @@ -169,29 +176,12 @@ export class OidcAuthenticationService { }); throw new OidcError({ message: error.message }); } - - const { - access_token: accessToken, - expires_in: expiresIn, - id_token: idToken, - refresh_token: refreshToken, - } = tokenSet; - - return new AuthenticationSessionContent({ - accessToken, - expiresIn, - idToken, - refreshToken, - }); } getAuthorizationUrl() { - const state = randomUUID(); - const nonce = randomUUID(); - - let redirectTarget; - try { + const state = randomUUID(); + const nonce = randomUUID(); const parameters = { nonce, redirect_uri: this.redirectUri, @@ -200,7 +190,9 @@ export class OidcAuthenticationService { ...this.extraAuthorizationUrlParameters, }; - redirectTarget = this.#openIdClient.buildAuthorizationUrl(this.#openIdClientConfig, parameters); + const redirectTarget = this.#openIdClient.buildAuthorizationUrl(this.#openIdClientConfig, parameters); + + return { redirectTarget, state, nonce }; } catch (error) { _monitorOidcError(error.message, { data: { organizationName: this.organizationName }, @@ -209,8 +201,6 @@ export class OidcAuthenticationService { }); throw new OidcError({ message: error.message }); } - - return { redirectTarget, state, nonce }; } async getUserInfo({ idToken, accessToken }) { diff --git a/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service.test.js b/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service.test.js index 8a3cb84ab31..ef637d21a2c 100644 --- a/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service.test.js +++ b/api/tests/identity-access-management/unit/domain/services/oidc-authentication-service.test.js @@ -296,9 +296,9 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { const expiresIn = Symbol(60); const idToken = Symbol('idToken'); const refreshToken = Symbol('refreshToken'); - const code = Symbol('AUTHORIZATION_CODE'); - const state = Symbol('STATE'); - const nonce = Symbol('NONCE'); + const code = 'AUTHORIZATION_CODE'; + const state = 'STATE'; + const nonce = 'NONCE'; const oidcAuthenticationSessionContent = new AuthenticationSessionContent({ idToken, accessToken, @@ -325,11 +325,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { await oidcAuthenticationService.initializeClientConfig(); // when - const result = await oidcAuthenticationService.exchangeCodeForTokens({ - code, - nonce, - state, - }); + const result = await oidcAuthenticationService.exchangeCodeForTokens({ code, nonce, state }); // then expect(result).to.be.an.instanceOf(AuthenticationSessionContent); @@ -338,11 +334,11 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { context('when OpenId Client callback fails', function () { it('throws an error and logs a message in datadog', async function () { - const clientId = Symbol('clientId'); - const clientSecret = Symbol('clientSecret'); - const identityProvider = Symbol('identityProvider'); - const redirectUri = Symbol('redirectUri'); - const openidConfigurationUrl = Symbol('openidConfigurationUrl'); + const clientId = 'clientId'; + const clientSecret = 'clientSecret'; + const identityProvider = 'identityProvider'; + const redirectUri = 'https://example.org/please-redirect-to-me'; + const openidConfigurationUrl = 'https://example.org/oidc-provider-configuration'; const code = 'code'; const nonce = 'nonce'; const iss = 'https://issuer.url'; @@ -405,7 +401,9 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { // given const clientId = 'OIDC_CLIENT_ID'; const clientSecret = 'OIDC_CLIENT_SECRET'; + const identityProvider = 'identityProvider'; const redirectUri = 'https://example.org/please-redirect-to-me'; + const openidConfigurationUrl = 'https://example.org/oidc-provider-configuration'; openIdClient.buildAuthorizationUrl.returns(''); @@ -413,7 +411,10 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { { clientId, clientSecret, + identityProvider, redirectUri, + openidConfigurationUrl, + organizationName: 'Oidc Example', }, { openIdClient }, ); @@ -438,11 +439,11 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { context('when generating the authorization url fails', function () { it('throws an error and logs a message in datadog', async function () { // given - const clientId = Symbol('clientId'); - const clientSecret = Symbol('clientSecret'); - const identityProvider = Symbol('identityProvider'); - const redirectUri = Symbol('redirectUri'); - const openidConfigurationUrl = Symbol('openidConfigurationUrl'); + const clientId = 'clientId'; + const clientSecret = 'clientSecret'; + const identityProvider = 'identityProvider'; + const redirectUri = 'https://example.org/please-redirect-to-me'; + const openidConfigurationUrl = 'https://example.org/oidc-provider-configuration'; const errorThrown = new Error('Fails to generate authorization url'); openIdClient.buildAuthorizationUrl.throws(errorThrown); @@ -681,7 +682,9 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { // given const clientId = 'OIDC_CLIENT_ID'; const clientSecret = 'OIDC_CLIENT_SECRET'; + const identityProvider = 'identityProvider'; const redirectUri = 'https://example.org/please-redirect-to-me'; + const openidConfigurationUrl = 'https://example.org/oidc-provider-configuration'; openIdClient.fetchUserInfo.returns({ sub: 'sub-id', @@ -693,7 +696,10 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { { clientId, clientSecret, + identityProvider, redirectUri, + openidConfigurationUrl, + organizationName: 'Oidc Example', }, { openIdClient }, ); @@ -723,11 +729,11 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { context('when OpenId Client userinfo fails', function () { it('throws an error and logs a message in datadog', async function () { - const clientId = Symbol('clientId'); - const clientSecret = Symbol('clientSecret'); - const identityProvider = Symbol('identityProvider'); - const redirectUri = Symbol('redirectUri'); - const openidConfigurationUrl = Symbol('openidConfigurationUrl'); + const clientId = 'OIDC_CLIENT_ID'; + const clientSecret = 'OIDC_CLIENT_SECRET'; + const identityProvider = 'identityProvider'; + const redirectUri = 'https://example.org/please-redirect-to-me'; + const openidConfigurationUrl = 'https://example.org/oidc-provider-configuration'; const errorThrown = new Error('Fails to get user info'); openIdClient.fetchUserInfo.rejects(errorThrown); @@ -767,8 +773,9 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { // given const clientId = 'OIDC_CLIENT_ID'; const clientSecret = 'OIDC_CLIENT_SECRET'; + const identityProvider = 'identityProvider'; const redirectUri = 'https://example.org/please-redirect-to-me'; - const organizationName = 'Example'; + const openidConfigurationUrl = 'https://example.org/oidc-provider-configuration'; openIdClient.fetchUserInfo.returns({ sub: 'sub-id', @@ -780,8 +787,10 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { { clientId, clientSecret, + identityProvider, redirectUri, - organizationName, + openidConfigurationUrl, + organizationName: 'Oidc Example', }, { openIdClient }, ); @@ -789,7 +798,7 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { await oidcAuthenticationService.initializeClientConfig(); const accessToken = 'thisIsSerializedInformation'; - const errorMessage = `Un ou des champs obligatoires (family_name) n'ont pas été renvoyés par votre fournisseur d'identité Example.`; + const errorMessage = `Un ou des champs obligatoires (family_name) n'ont pas été renvoyés par votre fournisseur d'identité Oidc Example.`; // when const error = await catchErr( @@ -823,8 +832,9 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { // given const clientId = 'OIDC_CLIENT_ID'; const clientSecret = 'OIDC_CLIENT_SECRET'; + const identityProvider = 'identityProvider'; const redirectUri = 'https://example.org/please-redirect-to-me'; - const organizationName = 'Example'; + const openidConfigurationUrl = 'https://example.org/oidc-provider-configuration'; openIdClient.fetchUserInfo.returns({ sub: 'sub-id', @@ -838,15 +848,17 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { claimsToStore: 'population', clientId, clientSecret, + identityProvider, redirectUri, - organizationName, + openidConfigurationUrl, + organizationName: 'Oidc Example', }, { openIdClient }, ); await oidcAuthenticationService.initializeClientConfig(); const accessToken = 'thisIsSerializedInformation'; - const errorMessage = `Un ou des champs obligatoires (population) n'ont pas été renvoyés par votre fournisseur d'identité Example.`; + const errorMessage = `Un ou des champs obligatoires (population) n'ont pas été renvoyés par votre fournisseur d'identité Oidc Example.`; // when const error = await catchErr( @@ -1010,11 +1022,11 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { describe('#initializeClientConfig', function () { it('creates an openid client', async function () { // given - const clientId = Symbol('clientId'); - const clientSecret = Symbol('clientSecret'); - const identityProvider = Symbol('identityProvider'); - const redirectUri = Symbol('redirectUri'); - const openidConfigurationUrl = Symbol('openidConfigurationUrl'); + const clientId = 'clientId'; + const clientSecret = 'clientSecret'; + const identityProvider = 'identityProvider'; + const redirectUri = 'https://example.org/please-redirect-to-me'; + const openidConfigurationUrl = 'https://example.org/oidc-provider-configuration'; const oidcAuthenticationService = new OidcAuthenticationService( { @@ -1031,19 +1043,18 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { await oidcAuthenticationService.initializeClientConfig(); // then - expect(openIdClient.discovery).to.have.been.calledWithExactly(openidConfigurationUrl, clientId, { + expect(openIdClient.discovery).to.have.been.calledWithExactly(new URL(openidConfigurationUrl), clientId, { client_secret: clientSecret, - redirect_uris: [redirectUri], }); }); it('creates an openid client with extra meatadata', async function () { // given - const clientId = Symbol('clientId'); - const clientSecret = Symbol('clientSecret'); - const identityProvider = Symbol('identityProvider'); - const redirectUri = Symbol('redirectUri'); - const openidConfigurationUrl = Symbol('openidConfigurationUrl'); + const clientId = 'clientId'; + const clientSecret = 'clientSecret'; + const identityProvider = 'identityProvider'; + const redirectUri = 'https://example.org/please-redirect-to-me'; + const openidConfigurationUrl = 'https://example.org/oidc-provider-configuration'; const openidClientExtraMetadata = { token_endpoint_auth_method: 'client_secret_post' }; const oidcAuthenticationService = new OidcAuthenticationService( @@ -1062,9 +1073,8 @@ describe('Unit | Domain | Services | oidc-authentication-service', function () { await oidcAuthenticationService.initializeClientConfig(); // then - expect(openIdClient.discovery).to.have.been.calledWithExactly(openidConfigurationUrl, clientId, { + expect(openIdClient.discovery).to.have.been.calledWithExactly(new URL(openidConfigurationUrl), clientId, { client_secret: clientSecret, - redirect_uris: [redirectUri], token_endpoint_auth_method: 'client_secret_post', }); }); diff --git a/api/tests/identity-access-management/unit/domain/services/pole-emploi-oidc-authentication-service.test.js b/api/tests/identity-access-management/unit/domain/services/pole-emploi-oidc-authentication-service.test.js index a23cbd1b4f8..100e8c16b05 100644 --- a/api/tests/identity-access-management/unit/domain/services/pole-emploi-oidc-authentication-service.test.js +++ b/api/tests/identity-access-management/unit/domain/services/pole-emploi-oidc-authentication-service.test.js @@ -58,11 +58,10 @@ describe('Unit | Identity Access Management | Domain | Services | pole-emploi-oi // then expect(openIdClient.discovery).to.have.been.calledWithExactly( - 'https://oidc.example.net/.well-known/openid-configuration', + new URL('https://oidc.example.net/.well-known/openid-configuration'), 'client', { client_secret: 'secret', - redirect_uris: ['https://app.dev.pix.local/connexion/oidc-example-net'], token_endpoint_auth_method: 'client_secret_post', }, );