From faa231f2a7972abbcc219e337d8d759e45737891 Mon Sep 17 00:00:00 2001 From: BenoitSerrano Date: Tue, 21 Apr 2026 16:11:15 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=97=91=EF=B8=8F=20remove=20eidas1=20legac?= =?UTF-8?q?y=20handling?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Problem** Handling `acr_values=eidas1` introduces unexpected behavior when combined with required claims. **Proposal** Remove the legacy behavior to ensure consistent handling of `acr_values` and required claims. --- .github/workflows/end-to-end.yml | 1 - cypress/e2e/signin_with_legacy_acr/env.conf | 0 .../e2e/signin_with_legacy_acr/fixtures.sql | 153 ------------------ .../e2e/signin_with_legacy_acr/index.cy.ts | 43 ----- src/controllers/interaction.ts | 16 +- src/services/acr-checks.ts | 14 -- test/acr-checks.test.ts | 70 -------- 7 files changed, 1 insertion(+), 296 deletions(-) delete mode 100644 cypress/e2e/signin_with_legacy_acr/env.conf delete mode 100644 cypress/e2e/signin_with_legacy_acr/fixtures.sql delete mode 100644 cypress/e2e/signin_with_legacy_acr/index.cy.ts diff --git a/.github/workflows/end-to-end.yml b/.github/workflows/end-to-end.yml index 2c7262e77..52a3a5b98 100644 --- a/.github/workflows/end-to-end.yml +++ b/.github/workflows/end-to-end.yml @@ -48,7 +48,6 @@ jobs: - signin_with_certification_dirigeant - signin_with_email_verification - signin_with_email_verification_renewal - - signin_with_legacy_acr - signin_with_legacy_scope - signin_with_magic_link - signin_with_right_acr diff --git a/cypress/e2e/signin_with_legacy_acr/env.conf b/cypress/e2e/signin_with_legacy_acr/env.conf deleted file mode 100644 index e69de29bb..000000000 diff --git a/cypress/e2e/signin_with_legacy_acr/fixtures.sql b/cypress/e2e/signin_with_legacy_acr/fixtures.sql deleted file mode 100644 index 77cd29c5c..000000000 --- a/cypress/e2e/signin_with_legacy_acr/fixtures.sql +++ /dev/null @@ -1,153 +0,0 @@ -INSERT INTO - users ( - id, - email, - email_verified, - email_verified_at, - encrypted_password, - created_at, - updated_at, - given_name, - family_name, - phone_number, - job, - encrypted_totp_key, - totp_key_verified_at, - force_2fa - ) -VALUES - ( - 1, - 'ial2-aal2@yopmail.com', - true, - CURRENT_TIMESTAMP, - '$2a$10$kzY3LINL6..50Fy9shWCcuNlRfYq0ft5lS.KCcJ5PzrhlWfKK4NIO', - CURRENT_TIMESTAMP, - CURRENT_TIMESTAMP, - 'Jean', - 'IAL2 AAL2', - '0123456789', - 'Sbire', - 'kuOSXGk68H2B3pYnph0uyXAHrmpbWaWyX/iX49xVaUc=.VMPBZSO+eAng7mjS.cI2kRY9rwhXchcKiiaMZIg==', - CURRENT_TIMESTAMP, - false - ), - ( - 2, - 'ial1-aal2@yopmail.com', - true, - CURRENT_TIMESTAMP, - '$2a$10$kzY3LINL6..50Fy9shWCcuNlRfYq0ft5lS.KCcJ5PzrhlWfKK4NIO', - CURRENT_TIMESTAMP, - CURRENT_TIMESTAMP, - 'Jean', - 'IAL1 AAL2', - '0123456789', - 'Sbire', - 'kuOSXGk68H2B3pYnph0uyXAHrmpbWaWyX/iX49xVaUc=.VMPBZSO+eAng7mjS.cI2kRY9rwhXchcKiiaMZIg==', - CURRENT_TIMESTAMP, - false - ), - ( - 3, - 'ial2-aal1@yopmail.com', - true, - CURRENT_TIMESTAMP, - '$2a$10$kzY3LINL6..50Fy9shWCcuNlRfYq0ft5lS.KCcJ5PzrhlWfKK4NIO', - CURRENT_TIMESTAMP, - CURRENT_TIMESTAMP, - 'Jean', - 'IAL2 AAL1', - '0123456789', - 'Sbire', - null, - null, - false - ), - ( - 4, - 'ial1-aal1@yopmail.com', - true, - CURRENT_TIMESTAMP, - '$2a$10$kzY3LINL6..50Fy9shWCcuNlRfYq0ft5lS.KCcJ5PzrhlWfKK4NIO', - CURRENT_TIMESTAMP, - CURRENT_TIMESTAMP, - 'Jean', - 'IAL1 AAL1', - '0123456789', - 'Sbire', - null, - null, - false - ); - -INSERT INTO - organizations (id, cached_libelle, siret, created_at, updated_at) -VALUES - ( - 1, - 'Commune de lamalou-les-bains', - '21340126800130', - CURRENT_TIMESTAMP, - CURRENT_TIMESTAMP - ); - -INSERT INTO - users_organizations ( - user_id, - organization_id, - is_external, - verified_at, - verification_type, - has_been_greeted - ) -VALUES - (1, 1, false, null, 'domain', true), - ( - 2, - 1, - false, - null, - 'domain_not_verified_yet', - true - ), - (3, 1, false, null, 'domain', true), - ( - 4, - 1, - false, - null, - 'domain_not_verified_yet', - true - ); - -INSERT INTO - oidc_clients ( - client_name, - client_id, - client_secret, - redirect_uris, - post_logout_redirect_uris, - scope, - client_uri, - client_description, - userinfo_signed_response_alg, - id_token_signed_response_alg, - authorization_signed_response_alg, - introspection_signed_response_alg - ) -VALUES - ( - 'Oidc Test Client', - 'standard_client_id', - 'standard_client_secret', - ARRAY['http://localhost:4000/login-callback'], - ARRAY[]::varchar[], - 'openid email profile organization', - 'http://localhost:4000/', - 'ProConnect test client. More info: https://github.com/proconnect-gouv/proconnect-test-client.', - null, - null, - null, - null - ); diff --git a/cypress/e2e/signin_with_legacy_acr/index.cy.ts b/cypress/e2e/signin_with_legacy_acr/index.cy.ts deleted file mode 100644 index c456f14f7..000000000 --- a/cypress/e2e/signin_with_legacy_acr/index.cy.ts +++ /dev/null @@ -1,43 +0,0 @@ -describe("sign-in with a client requiring legacy acr : eidas1", () => { - before(cy.seed); - beforeEach(() => { - cy.visit("http://localhost:4000"); - - cy.updateCustomParams((customParams) => ({ - ...customParams, - acr_values: "eidas1", - })); - }); - - it("should return eidas1 for a user without 2FA and unverified domain", function () { - cy.get("button#custom-connection").click({ force: true }); - - cy.login("ial1-aal1@yopmail.com"); - - cy.contains('"acr": "eidas1"'); - }); - - it("should return eidas1 for a user without 2FA and verified domain", function () { - cy.get("button#custom-connection").click({ force: true }); - - cy.login("ial2-aal1@yopmail.com"); - - cy.contains('"acr": "eidas1"'); - }); - - it("should return eidas1 for a user with 2FA and unverified domain", function () { - cy.get("button#custom-connection").click({ force: true }); - - cy.login("ial1-aal2@yopmail.com"); - - cy.contains('"acr": "eidas1"'); - }); - - it("should return eidas1 for a user with 2FA and verified domain", function () { - cy.get("button#custom-connection").click({ force: true }); - - cy.login("ial2-aal2@yopmail.com"); - - cy.contains('"acr": "eidas1"'); - }); -}); diff --git a/src/controllers/interaction.ts b/src/controllers/interaction.ts index 4ad56fde2..629be325e 100644 --- a/src/controllers/interaction.ts +++ b/src/controllers/interaction.ts @@ -19,7 +19,6 @@ import { findByClientId } from "../repositories/oidc-client"; import { certificationDirigeantRequested, isAcrSatisfied, - isThereAnyRequestedAcr, twoFactorsAuthRequested, } from "../services/acr-checks"; import { oidcErrorSchema, siretSchema } from "../services/custom-zod-schemas"; @@ -115,20 +114,7 @@ export const interactionEndControllerFactory = ? epochTime(user.last_sign_in_at) : undefined; - const { prompt, params } = await oidcProvider.interactionDetails( - req, - res, - ); - - // Previously, OIDC clients were required to include `acr_values=eidas1` as a query parameter in the /authorize request. - // Some clients may still expect the returned ACR to be "eidas1" for successful authentication. - // We maintain this legacy behavior until all OIDC clients have been properly migrated. - if ( - params?.["acr_values"] === "eidas1" && - !isThereAnyRequestedAcr(prompt) - ) { - currentAcr = "eidas1"; - } + const { prompt } = await oidcProvider.interactionDetails(req, res); let result: OidcInteractionResults = { login: { diff --git a/src/services/acr-checks.ts b/src/services/acr-checks.ts index 23f3847b5..16bcff7d4 100644 --- a/src/services/acr-checks.ts +++ b/src/services/acr-checks.ts @@ -86,20 +86,6 @@ export const certificationDirigeantRequested = (prompt: PromptDetail) => { ); }; -export const isThereAnyRequestedAcr = (prompt: PromptDetail) => { - return areAcrsRequestedInPrompt({ - prompt, - acrs: [ - ACR_VALUE_FOR_IAL1_AAL1, - ACR_VALUE_FOR_IAL1_AAL2, - ACR_VALUE_FOR_IAL2_AAL1, - ACR_VALUE_FOR_IAL2_AAL2, - ACR_VALUE_FOR_CERTIFICATION_DIRIGEANT, - ACR_VALUE_FOR_IAL3_AAL2, - ], - }); -}; - export const isAcrSatisfied = (prompt: PromptDetail, currentAcr: string) => { // if no acr is required it is satisfied if (!containsEssentialAcrs(prompt)) { diff --git a/test/acr-checks.test.ts b/test/acr-checks.test.ts index 17b2a6cb6..2a7b4a924 100644 --- a/test/acr-checks.test.ts +++ b/test/acr-checks.test.ts @@ -3,7 +3,6 @@ import { describe, it } from "node:test"; import { certificationDirigeantRequested, isAcrSatisfied, - isThereAnyRequestedAcr, twoFactorsAuthRequested, } from "../src/services/acr-checks"; @@ -177,75 +176,6 @@ describe("isAcrSatisfied", () => { }); }); -describe("isThereAnyRequestedAcr", () => { - it("should return false for acr non-related prompt", () => { - const prompt = { - name: "random", - reasons: ["random"], - details: { random: "random" }, - }; - - assert.equal(isThereAnyRequestedAcr(prompt), false); - }); - - it("should return true for prompt with no acr required", () => { - const prompt = { name: "login", reasons: ["no_session"], details: {} }; - - assert.equal(isThereAnyRequestedAcr(prompt), false); - }); - - it("should return false for legacy acr", () => { - const prompt = { - name: "login", - reasons: ["essential_acrs"], - details: { - acr: { - essential: true, - value: "eidas1", - }, - }, - }; - - assert.equal(isThereAnyRequestedAcr(prompt), false); - }); - - it("should return true for non legacy acr", () => { - const prompt = { - name: "login", - reasons: ["essential_acrs"], - details: { - acr: { - essential: true, - values: [ - "eidas1", - "https://proconnect.gouv.fr/assurance/consistency-checked-2fa", - ], - }, - }, - }; - - assert.equal(isThereAnyRequestedAcr(prompt), true); - }); - - it("should return true for mfa requested identity", () => { - const prompt = { - name: "login", - reasons: ["essential_acrs"], - details: { - acr: { - essential: true, - values: [ - "https://proconnect.gouv.fr/assurance/self-asserted-2fa", - "https://proconnect.gouv.fr/assurance/consistency-checked-2fa", - ], - }, - }, - }; - - assert.equal(isThereAnyRequestedAcr(prompt), true); - }); -}); - describe("certificationDirigeantRequested", () => { it("should return true for certification dirigeant acr", () => { const prompt = {