Merge pull request #869 from govuk-one-login/OLH-4194-move-required-claims-check

OLH-4194: move required claims check

Author: liegeandlief

solutions/frontend/open-api-spec.yaml

@@ -3,7 +3,7 @@ openapi: 3.0.3
 
 info:
   title: Account Management Components OAuth Provider Public API
-  version: 0.0.14
+  version: 0.0.15
   description: |
     OAuth 2.0 provider endpoints, including:
     - GET /authorize
@@ -96,6 +96,8 @@ paths:
                   value: https://signin.account.gov.uk/callback?error=invalid_request&error_description=E1011&state=aRandomString
                 user_agent_mismatch:
                   value: https://signin.account.gov.uk/callback?error=invalid_request&error_description=E1012&state=aRandomString
+                required_claims_missing:
+                  value: https://signin.account.gov.uk/callback?error=invalid_request&error_description=E1013&state=aRandomString
                 jwks_timeout:
                   value: https://signin.account.gov.uk/callback?error=unauthorized_client&error_description=E2001&state=aRandomString
                 jwks_invalid:

solutions/frontend/src/handlers/authorize/utils/verifyJwt.test.ts

@@ -64,6 +64,18 @@ vi.mock(import("../../../utils/getClaimsSchema.js"), () => ({
   getClaimsSchema: mockGetClaimsSchema,
 }));
 
+const mockJourneys = vi.fn();
+
+// @ts-expect-error
+vi.mock(import("../../../journeys/utils/config.js"), () => ({
+  journeys: new Proxy(
+    {},
+    {
+      get: () => mockJourneys,
+    },
+  ),
+}));
+
 let verifyJwt: typeof verifyJwtForType;
 
 describe("verifyJwt", () => {
@@ -102,11 +114,16 @@ describe("verifyJwt", () => {
   });
 
   it("verifies JWT with ES256 algorithm", async () => {
-    const payload = { sub: "user123", aud: "test-client" };
-    const claimsSchema = v.object({ sub: v.string(), aud: v.string() });
+    const payload = { sub: "user123", aud: "test-client", scope: "openid" };
+    const claimsSchema = v.object({
+      sub: v.string(),
+      aud: v.string(),
+      scope: v.string(),
+    });
 
     mockJwtVerify.mockResolvedValue({ payload });
     mockGetClaimsSchema.mockReturnValue(claimsSchema);
+    mockJourneys.mockResolvedValue({ requiredClaims: [] });
 
     const result = await verifyJwt(
       reply,
@@ -441,12 +458,55 @@ describe("verifyJwt", () => {
     );
   });
 
+  it("returns ErrorResponse when required claims are missing", async () => {
+    const payload = { sub: "user123", aud: "test-client", scope: "openid" };
+    const claimsSchema = v.object({
+      sub: v.string(),
+      aud: v.string(),
+      scope: v.string(),
+    });
+
+    mockJwtVerify.mockResolvedValue({ payload });
+    mockGetClaimsSchema.mockReturnValue(claimsSchema);
+    mockJourneys.mockResolvedValue({
+      requiredClaims: ["account_management_api_access_token"],
+    });
+
+    const result = await verifyJwt(
+      reply,
+      signedJwt,
+      client,
+      redirectUri,
+      scope,
+      state,
+    );
+
+    expect(result).toBeInstanceOf(ErrorResponse);
+
+    assert.ok(result instanceof ErrorResponse);
+
+    expect(vi.mocked(result.reply.redirect).mock.calls[0]?.[0]).toContain(
+      "error=invalid_request",
+    );
+    expect(vi.mocked(result.reply.redirect).mock.calls[0]?.[0]).toContain(
+      "error_description=E1013",
+    );
+    expect(addAuthorizeErrorMetric).toHaveBeenCalledWith(
+      "RequiredClaimsMissing",
+    );
+  });
+
   it("works without state parameter", async () => {
-    const payload = { sub: "user123", aud: "test-client" };
-    const claimsSchema = v.object({ sub: v.string(), aud: v.string() });
+    const payload = { sub: "user123", aud: "test-client", scope: "openid" };
+    const claimsSchema = v.object({
+      sub: v.string(),
+      aud: v.string(),
+      scope: v.string(),
+    });
 
     mockJwtVerify.mockResolvedValue({ payload });
     mockGetClaimsSchema.mockReturnValue(claimsSchema);
+    mockJourneys.mockResolvedValue({ requiredClaims: [] });
 
     const result = await verifyJwt(
       reply,

solutions/frontend/src/handlers/authorize/utils/verifyJwt.ts

@@ -17,6 +17,7 @@ import assert from "node:assert";
 import { getAppConfig } from "../../../../../commons/utils/getAppConfig/index.js";
 import type { FastifyReply } from "fastify";
 import { jwtVerifyAlgorithms } from "../../../../../commons/utils/constants.js";
+import { journeys } from "../../../journeys/utils/config.js";
 
 const handleJwtError = (
   reply: FastifyReply,
@@ -291,6 +292,29 @@ const checkClaims = async (
     );
   }
 
+  const journey = await journeys[claimsResult.output.scope]();
+
+  const missingRequiredClaims = journey.requiredClaims.filter((claim) => {
+    return claimsResult.output[claim] === undefined;
+  });
+
+  if (missingRequiredClaims.length) {
+    logger.warn("RequiredClaimsMissing", {
+      client_id: client.client_id,
+      missing_required_claims: missingRequiredClaims,
+    });
+    addAuthorizeErrorMetric("RequiredClaimsMissing");
+
+    return new ErrorResponse(
+      getRedirectToClientRedirectUriResponse(
+        reply,
+        redirectUri,
+        authorizeErrors.requiredClaimsMissing,
+        state,
+      ),
+    );
+  }
+
   return claimsResult.output;
 };
 

solutions/frontend/src/journeys/utils/onRequest.test.ts

@@ -63,7 +63,6 @@ vi.mock(import("./config.js"), () => ({
       stateMachine: {
         resolveState: vi.fn().mockReturnValue({}),
       },
-      requiredClaims: [],
     }),
   },
 }));
@@ -147,7 +146,6 @@ describe("onRequest", () => {
     vi.mocked(journeys["test-scope" as Scope]).mockResolvedValue({
       translations: { en: { key: "value" } },
       stateMachine: { resolveState: vi.fn().mockReturnValue({}) },
-      requiredClaims: [],
     } as any);
   });
 
@@ -211,40 +209,6 @@ describe("onRequest", () => {
     });
   });
 
-  describe("when required claims are missing", () => {
-    beforeEach(() => {
-      mockSession.claims = {
-        client_id: "test-client-id",
-        scope: "test-scope",
-      } as unknown as Claims;
-      // eslint-disable-next-line @typescript-eslint/no-unsafe-argument
-      vi.mocked(journeys["test-scope" as Scope]).mockResolvedValue({
-        translations: { en: { key: "value" } },
-        stateMachine: { resolveState: vi.fn().mockReturnValue({}) },
-        requiredClaims: ["account_management_api_access_token"],
-      } as any);
-    });
-
-    it("should redirect to error page and log warning", async () => {
-      await onRequest(mockRequest as FastifyRequest, mockReply as FastifyReply);
-
-      expect(mockRequest.log?.warn).toHaveBeenCalledWith(
-        { missingRequiredClaims: ["account_management_api_access_token"] },
-        "RequiredClaimsMissing",
-      );
-      expect(metrics.addMetadata).toHaveBeenCalledWith(
-        "error_type",
-        "RequiredClaimsMissing",
-      );
-      expect(metrics.addMetric).toHaveBeenCalledWith(
-        "JourneyRequestError",
-        "Count",
-        1,
-      );
-      expect(mockReply.redirect).toHaveBeenCalledWith("/error");
-    });
-  });
-
   describe("successful flow", () => {
     beforeEach(() => {
       mockSession.claims = {

solutions/frontend/src/journeys/utils/onRequest.ts

@@ -85,23 +85,6 @@ export const onRequest = async (
 
   const journey = await journeys[claims.scope]();
 
-  const missingRequiredClaims = journey.requiredClaims.filter((claim) => {
-    return claims[claim] === undefined;
-  });
-
-  if (missingRequiredClaims.length) {
-    request.log.warn(
-      {
-        missingRequiredClaims,
-      },
-      "RequiredClaimsMissing",
-    );
-    addErrorMetric("RequiredClaimsMissing");
-
-    reply.redirect(paths.others.authorizeError.path);
-    return reply;
-  }
-
   Object.entries(journey.translations).forEach(([lang, translations]) => {
     request.i18n.addResourceBundle(lang, "journey", translations);
   });

solutions/frontend/src/utils/authorizeErrors.ts

@@ -60,6 +60,10 @@ export const authorizeErrors = {
     description: "E1012",
     type: "invalid_request",
   },
+  requiredClaimsMissing: {
+    description: "E1013",
+    type: "invalid_request",
+  },
   jwksTimeout: {
     description: "E2001",
     type: "unauthorized_client",