ATO-745: Identity Support For User Info #111

BenjaminWCO · 2024-09-19T16:20:08Z

Added config controller request body validation
Added support to configure claim data from config controller
Added support to return claim data from user info controller
Added CoreIdentityJWT generation (vot and vc fields can be configured via config controller)
Removed HMRC specific InheritedIdentityJWT claim

BAU:

Removed unnecessary dependancy on "body-parser"
Fixed typos

CarlyG55 · 2024-09-23T11:31:03Z

Can you remove the return codes validation? Since we definitely won't be validating these

Ryan-Andrews99 · 2024-09-20T11:20:44Z

src/config.ts

+    this.clientId = process.env.CLIENT_ID ?? "HGIOgho9HIRhgoepdIOPFdIUWgewi0jw";
+    this.publicKey = process.env.PUBLIC_KEY ?? defaultPublicKey;
+    this.scopes = process.env.SCOPES
+      ? process.env.SCOPES.split(",")
+      : ["openid", "email", "phone"];
+    this.redirectUrls = process.env.REDIRECT_URLS
+      ? process.env.REDIRECT_URLS.split(",")
+      : ["http://localhost:8080/authorization-code/callback"];
+    this.claims = process.env.CLAIMS
+      ? (process.env.CLAIMS.split(",") as (keyof UserIdentity)[])
+      : ["https://vocab.account.gov.uk/v1/coreIdentityJWT"];
+    this.identityVerificationSupported =
+      process.env.IDENTITY_VERIFICATION_SUPPORTED !== "false";
+    this.idTokenSigningAlgorithm =
+      process.env.ID_TOKEN_SIGNING_ALGORITHM ?? "ES256";
+    this.clientLoCs = process.env.CLIENT_LOCS
+      ? process.env.CLIENT_LOCS.split(",")
+      : ["P0", "P2"];
+
+    this.sub =
+      process.env.SUB ??
+      "urn:fdc:gov.uk:2022:56P4CMsGh_02YOlWpd8PAOI-2sVlB2nsNU7mcLZYhYw=";
+    this.email = process.env.EMAIL ?? "john.smith@gmail.com";
+    this.emailVerified = process.env.EMAIL_VERIFIED !== "false";
+    this.phoneNumber = process.env.PHONE_NUMBER ?? "07123456789";
+    this.phoneNumberVerified = process.env.PHONE_NUMBER_VERIFIED !== "false";
+


Is there a reason we moved from the more nested structure that was introduced here?https://github.com/govuk-one-login/simulator/pull/46/files

The reason is that some properties may be optional in the config request, but must have a value (i.e. some default) so the types don't necessarily align. I suppose an alternative might be to fall back to the defaults from the setters rather than setting them in the constructor. Thoughts?

Reverted this. Using "!" assertion in getters when needed.

src/components/user-info/user-info-controller.ts

Ryan-Andrews99 · 2024-09-23T12:52:24Z

src/types/driving-permit-details.ts

+export const generateDrivingPermitPropertyValidators = (
+  parent?: string
+): ValidationChain[] => {
+  const prefix = parent ? `${parent}.` : "";


Do these property validators ever have the value of parent passed in? It looks like they don't, if so we could simplify the code a bit. Appreciate I may be missing something here, so happy to be told otherwise 🙂

I don't know if this is a good pattern, but it was the best way I could think of. Most of the time it is set, because you have to describe the full chain of properties from the root object. The only case it's unset is for the root object (so ConfigRequest). ConfigRequest could simply not take the optional parameter and all the other types could require it. The reason for duplicating the prefix logic for all the types is just for a consistent pattern. Potentially useful If you had a type that could appear in different locations in a request body.

tests/integration/config.test.ts

Ryan-Andrews99 · 2024-09-23T13:18:31Z

src/types/client-configuration.ts

 }
+
+export const generateClientConfigurationPropertyValidators = (


I'm not super familiar with these validators, they seem pretty readable, but some tests for these might be nice 🙂

I've added a few examples in the config integration test. We could add some more thorough tests, tough I'm not sure what the best way to do that would be. Given the way express-validator runs as a middleware, the simplest way would be to add a bunch more test cases to the config endpoint integration test, but given it's an endpoint that's in a state of flux at the moment that might be quite brittle.

We could come up with some tests helper function that spins up an express server with some test endpoint and pass in the validator to that. That might be a nice way to test validating each type individually.

I've adde the helper function and some individual tests for validated types.

Ryan-Andrews99 · 2024-09-23T13:20:06Z

src/types/identity.ts

+      .isURL()
+      .optional({ values: "null" }),
+    body(`${prefix}${nameof<Identity>("sub")}`)
+      .isURL()


Quite a few of these the isURL method called, I wouldn't expect most of them to be valid URLs. Is this correct?

I think you're right - iss and sub should just validated to be strings.

Removed this type as coreIdentityJWT is being generated now

ethanmills · 2024-09-26T14:13:51Z

src/components/user-info/user-info-controller.ts

+  if (
+    validationResult.claims?.includes(
+      "https://vocab.account.gov.uk/v1/inheritedIdentityJWT"
+    )
+  ) {
+    const inheritedIdentity = config.getInheritedIdentityDetails();
+    userInfo["https://vocab.account.gov.uk/v1/inheritedIdentityJWT"] =
+      await new SignJWT(inheritedIdentity as any)
+        .setProtectedHeader({
+          alg: "ES256",
+        })
+        .sign(signingKey);
+  }


This is not something that is returned in the user info endpoint. This is a slight abuse of the spec for HMRC to pass us additional information about a user

ethanmills

I'm really hesitant about the amount of validation we're doing here. We don't own the schema for the validations we're performing, and there's no way to ensure this stays correct and up to date. I happen to know, for example, that new fields will soon be valid in address objects. There's no real way for us to test that this validation matches the information returned by One Login. In general I think we should avoid providing functionality unless we can be confident that we can ensure it stays correct.

ethanmills · 2024-09-27T10:10:44Z

src/components/user-info/user-info-controller.ts

+    userInfo["https://vocab.account.gov.uk/v1/coreIdentityJWT"] =
+      await new SignJWT(coreIdentity as any)
+        .setProtectedHeader({
+          alg: "ES256",


Think we're missing a kid here?

ethanmills · 2024-09-27T10:29:32Z

src/types/identity.ts

+  vot?: string;
+  vtm?: string;
+  iss?: string;
+  sub?: string;
+  nbf?: number;
+  exp?: number;
+  aud?: string;
+  iat?: number;
+  jti?: string;


Are we missing vc?

Added - vot and vc are configurable. The other fields are now generated.

ethanmills · 2024-09-27T10:30:21Z

src/types/identity.ts

+  vot?: string;
+  vtm?: string;
+  iss?: string;
+  sub?: string;
+  nbf?: number;
+  exp?: number;
+  aud?: string;
+  iat?: number;
+  jti?: string;


Feels like we should be setting some sensible defaults for some of these?

As above - vot and vc are configurable. The other fields are now generated.

sonarcloud · 2024-10-01T17:19:00Z

Quality Gate failed

Failed conditions
4.7% Duplication on New Code (required ≤ 3%)
1 New Code Smells (required ≤ 0)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

BenjaminWCO requested review from a team as code owners September 19, 2024 16:20

BenjaminWCO force-pushed the ATO-745-Identity-For-User-Info branch 7 times, most recently from db5fc9d to 64f59e4 Compare September 19, 2024 16:58

CarlyG55 force-pushed the ATO-745-Identity-For-User-Info branch from 64f59e4 to 0a6d93c Compare September 19, 2024 17:08

BenjaminWCO marked this pull request as draft September 23, 2024 09:30

BenjaminWCO force-pushed the ATO-745-Identity-For-User-Info branch from 0a6d93c to fc41225 Compare September 23, 2024 11:01

Ryan-Andrews99 reviewed Sep 23, 2024

View reviewed changes

ethanmills reviewed Sep 26, 2024

View reviewed changes

ethanmills reviewed Sep 27, 2024

View reviewed changes

isaac-GDS mentioned this pull request Sep 30, 2024

Ato 1009/identity for authorize endpoint #119

Closed

BenjaminWCO force-pushed the ATO-745-Identity-For-User-Info branch 3 times, most recently from 2e110ac to 9ef2f0c Compare October 1, 2024 10:07

BenjaminWCO requested review from ethanmills and Ryan-Andrews99 October 1, 2024 10:16

BenjaminWCO marked this pull request as ready for review October 1, 2024 10:16

BenjaminWCO mentioned this pull request Oct 1, 2024

ATO-1058: Add Support For Core Identity Error Scenarios #135

Draft

BenjaminWCO added 2 commits October 1, 2024 17:45

BAU: Remove Unnecessary body-parser Dependency

b1d0ce4

BAU: Fix Typos

faaae5a

BenjaminWCO force-pushed the ATO-745-Identity-For-User-Info branch 2 times, most recently from 8b1ffb1 to ddd20b9 Compare October 1, 2024 17:17

BenjaminWCO added 6 commits October 1, 2024 18:18

ATO-745: Add Config Handler Request Type Validation

5b3c487

ATO-745: Remove Inherited Identity Claim

a4d83c6

ATO-745: Add Support To Configure Returned Claims

9cb7a3e

ATO-745: Add Config Validation Tests

21c89b7

ATO-745: Rename Key IDs

cb7fffd

ATO-745: Return Configured Claims From User Info Controller

1c5dd66

BenjaminWCO force-pushed the ATO-745-Identity-For-User-Info branch from ddd20b9 to 1c5dd66 Compare October 1, 2024 17:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ATO-745: Identity Support For User Info #111

ATO-745: Identity Support For User Info #111

BenjaminWCO commented Sep 19, 2024 •

edited

Loading

CarlyG55 commented Sep 23, 2024

Ryan-Andrews99 Sep 20, 2024

BenjaminWCO Sep 23, 2024 •

edited

Loading

BenjaminWCO Oct 1, 2024

Ryan-Andrews99 Sep 23, 2024

BenjaminWCO Sep 23, 2024

Ryan-Andrews99 Sep 23, 2024

BenjaminWCO Sep 23, 2024

BenjaminWCO Sep 23, 2024 •

edited

Loading

BenjaminWCO Oct 1, 2024

Ryan-Andrews99 Sep 23, 2024 •

edited

Loading

BenjaminWCO Sep 23, 2024

BenjaminWCO Oct 1, 2024 •

edited

Loading

ethanmills Sep 26, 2024

BenjaminWCO Oct 1, 2024

ethanmills left a comment

ethanmills Sep 27, 2024

BenjaminWCO Oct 1, 2024

ethanmills Sep 27, 2024

BenjaminWCO Oct 1, 2024 •

edited

Loading

ethanmills Sep 27, 2024

BenjaminWCO Oct 1, 2024 •

edited

Loading

sonarcloud bot commented Oct 1, 2024

		}

		export const generateClientConfigurationPropertyValidators = (

ATO-745: Identity Support For User Info #111

Are you sure you want to change the base?

ATO-745: Identity Support For User Info #111

Conversation

BenjaminWCO commented Sep 19, 2024 • edited Loading

CarlyG55 commented Sep 23, 2024

Choose a reason for hiding this comment

BenjaminWCO Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenjaminWCO Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ryan-Andrews99 Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenjaminWCO Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanmills left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenjaminWCO Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenjaminWCO Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Oct 1, 2024

Quality Gate failed

BenjaminWCO commented Sep 19, 2024 •

edited

Loading

BenjaminWCO Sep 23, 2024 •

edited

Loading

BenjaminWCO Sep 23, 2024 •

edited

Loading

Ryan-Andrews99 Sep 23, 2024 •

edited

Loading

BenjaminWCO Oct 1, 2024 •

edited

Loading

BenjaminWCO Oct 1, 2024 •

edited

Loading

BenjaminWCO Oct 1, 2024 •

edited

Loading