fix: conflation of a user's Sub and their Username

BREAKING CHANGE: potential -- the autogenerated Sub and user-supplied Username were treated interchangeably before, but now are independent. Previously lookups by the Sub attribute were possible, but it now doesn't appear necessary so has been removed. Databases should be unaffected.
jagregory · Jul 28, 2021 · ece63b6 · kadyrleev · Jul 29, 2021 · ece63b6
1 parent 51056dc
commit ece63b6
Show file tree

Hide file tree

Showing 12 changed files with 121 additions and 88 deletions.
diff --git a/integration-tests/userPoolClient.test.ts b/integration-tests/userPoolClient.test.ts
@@ -45,15 +45,18 @@ describe("User Pool Client", () => {
   });
 
   describe("saveUser", () => {
-    it("saves a user with their username as an additional attribute", async () => {
+    it("saves the user", async () => {
       const now = new Date().getTime();
       const userPool = await cognitoClient.getUserPool("local");
 
       await userPool.saveUser({
         Username: "1",
         Password: "hunter3",
         UserStatus: "UNCONFIRMED",
-        Attributes: [{ Name: "email", Value: "[email protected]" }],
+        Attributes: [
+          { Name: "sub", Value: "uuid-1234" },
+          { Name: "email", Value: "[email protected]" },
+        ],
         UserLastModifiedDate: now,
         UserCreateDate: now,
         Enabled: true,
@@ -69,7 +72,7 @@ describe("User Pool Client", () => {
             Password: "hunter3",
             UserStatus: "UNCONFIRMED",
             Attributes: [
-              { Name: "sub", Value: "1" },
+              { Name: "sub", Value: "uuid-1234" },
               { Name: "email", Value: "[email protected]" },
             ],
             UserLastModifiedDate: now,
@@ -89,7 +92,10 @@ describe("User Pool Client", () => {
         Password: "hunter3",
         UserStatus: "UNCONFIRMED",
         ConfirmationCode: "1234",
-        Attributes: [{ Name: "email", Value: "[email protected]" }],
+        Attributes: [
+          { Name: "sub", Value: "uuid-1234" },
+          { Name: "email", Value: "[email protected]" },
+        ],
         UserLastModifiedDate: now,
         UserCreateDate: now,
         Enabled: true,
@@ -106,7 +112,7 @@ describe("User Pool Client", () => {
             UserStatus: "UNCONFIRMED",
             ConfirmationCode: "1234",
             Attributes: [
-              { Name: "sub", Value: "1" },
+              { Name: "sub", Value: "uuid-1234" },
               { Name: "email", Value: "[email protected]" },
             ],
             UserLastModifiedDate: now,
@@ -120,7 +126,10 @@ describe("User Pool Client", () => {
         Username: "1",
         Password: "hunter3",
         UserStatus: "CONFIRMED",
-        Attributes: [{ Name: "email", Value: "[email protected]" }],
+        Attributes: [
+          { Name: "sub", Value: "uuid-1234" },
+          { Name: "email", Value: "[email protected]" },
+        ],
         UserLastModifiedDate: now,
         UserCreateDate: now,
         Enabled: true,
@@ -136,7 +145,7 @@ describe("User Pool Client", () => {
             Password: "hunter3",
             UserStatus: "CONFIRMED",
             Attributes: [
-              { Name: "sub", Value: "1" },
+              { Name: "sub", Value: "uuid-1234" },
               { Name: "email", Value: "[email protected]" },
             ],
             UserLastModifiedDate: now,
@@ -158,6 +167,7 @@ describe("User Pool Client", () => {
         Password: "hunter2",
         UserStatus: "UNCONFIRMED",
         Attributes: [
+          { Name: "sub", Value: "uuid-1234" },
           { Name: "email", Value: "[email protected]" },
           { Name: "phone_number", Value: "0411000111" },
         ],
@@ -194,6 +204,7 @@ describe("User Pool Client", () => {
         Password: "hunter2",
         UserStatus: "UNCONFIRMED",
         Attributes: [
+          { Name: "sub", Value: "uuid-1234" },
           { Name: "email", Value: "[email protected]" },
           { Name: "phone_number", Value: "0411000111" },
         ],
@@ -206,7 +217,7 @@ describe("User Pool Client", () => {
         Username: "2",
         Password: "password1",
         UserStatus: "UNCONFIRMED",
-        Attributes: [],
+        Attributes: [{ Name: "sub", Value: "uuid-5678" }],
         UserCreateDate: now.getTime(),
         UserLastModifiedDate: now.getTime(),
         Enabled: true,
@@ -222,7 +233,7 @@ describe("User Pool Client", () => {
           Password: "hunter2",
           UserStatus: "UNCONFIRMED",
           Attributes: [
-            { Name: "sub", Value: "1" },
+            { Name: "sub", Value: "uuid-1234" },
             { Name: "email", Value: "[email protected]" },
             { Name: "phone_number", Value: "0411000111" },
           ],
@@ -234,7 +245,7 @@ describe("User Pool Client", () => {
           Username: "2",
           Password: "password1",
           UserStatus: "UNCONFIRMED",
-          Attributes: [{ Name: "sub", Value: "2" }],
+          Attributes: [{ Name: "sub", Value: "uuid-5678" }],
           UserCreateDate: now.getTime(),
           UserLastModifiedDate: now.getTime(),
           Enabled: true,

diff --git a/src/__tests__/patterns.ts b/src/__tests__/patterns.ts
@@ -0,0 +1 @@
+export const UUID = /[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/i;
diff --git a/src/services/tokens.ts b/src/services/tokens.ts
@@ -1,7 +1,7 @@
 import jwt from "jsonwebtoken";
 import * as uuid from "uuid";
 import PrivateKey from "../keys/cognitoLocal.private.json";
-import { User } from "./userPoolClient";
+import { attributeValue, User } from "./userPoolClient";
 
 export interface Token {
   client_id: string;
@@ -23,10 +23,12 @@ export function generateTokens(
   const eventId = uuid.v4();
   const authTime = Math.floor(new Date().getTime() / 1000);
 
+  const sub = attributeValue("sub", user.Attributes);
+
   return {
     AccessToken: jwt.sign(
       {
-        sub: user.Username,
+        sub,
         event_id: eventId,
         token_use: "access",
         scope: "aws.cognito.signin.user.admin", // TODO: scopes
@@ -45,15 +47,13 @@ export function generateTokens(
     ),
     IdToken: jwt.sign(
       {
-        sub: user.Username,
+        sub,
         email_verified: true,
         event_id: eventId,
         token_use: "id",
         auth_time: authTime,
         "cognito:username": user.Username,
-        email: user.Attributes.filter((x) => x.Name === "email").map(
-          (x) => x.Value
-        )[0],
+        email: attributeValue("email", user.Attributes),
       },
       PrivateKey.pem,
       {

diff --git a/src/services/triggers/userMigration.test.ts b/src/services/triggers/userMigration.test.ts
@@ -1,11 +1,10 @@
+import { UUID } from "../../__tests__/patterns";
 import { NotAuthorizedError } from "../../errors";
 import { CognitoClient } from "../cognitoClient";
 import { Lambda } from "../lambda";
 import { UserPoolClient } from "../userPoolClient";
 import { UserMigration, UserMigrationTrigger } from "./userMigration";
 
-const UUID = /[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/i;
-
 describe("UserMigration trigger", () => {
   let mockLambda: jest.Mocked<Lambda>;
   let mockCognitoClient: jest.Mocked<CognitoClient>;

diff --git a/src/services/userPoolClient.test.ts b/src/services/userPoolClient.test.ts
@@ -6,6 +6,7 @@ import {
   attributesInclude,
   attributesIncludeMatch,
   attributesToRecord,
+  User,
   UserAttribute,
   UserPoolClient,
   UserPoolClientService,
@@ -82,7 +83,7 @@ describe("User Pool Client", () => {
   });
 
   describe("saveUser", () => {
-    it("saves a user with their username as an additional attribute", async () => {
+    it("saves the user", async () => {
       const now = new Date().getTime();
       const set = jest.fn();
 
@@ -99,21 +100,24 @@ describe("User Pool Client", () => {
       );
 
       await userPool.saveUser({
-        Username: "1",
+        Username: "user-supplied",
         Password: "hunter3",
         UserStatus: "UNCONFIRMED",
-        Attributes: [{ Name: "email", Value: "[email protected]" }],
+        Attributes: [
+          { Name: "sub", Value: "uuid-1234" },
+          { Name: "email", Value: "[email protected]" },
+        ],
         UserLastModifiedDate: now,
         UserCreateDate: now,
         Enabled: true,
       });
 
-      expect(set).toHaveBeenCalledWith("Users.1", {
-        Username: "1",
+      expect(set).toHaveBeenCalledWith(["Users", "user-supplied"], {
+        Username: "user-supplied",
         Password: "hunter3",
         UserStatus: "UNCONFIRMED",
         Attributes: [
-          { Name: "sub", Value: "1" },
+          { Name: "sub", Value: "uuid-1234" },
           { Name: "email", Value: "[email protected]" },
         ],
         UserLastModifiedDate: now,
@@ -140,13 +144,13 @@ describe("User Pool Client", () => {
             Id: "local",
             UsernameAttributes: username_attributes,
           };
-          const users = {
-            "1": {
-              Username: "1",
+          const users: Record<string, User> = {
+            "user-supplied": {
+              Username: "user-supplied",
               Password: "hunter3",
               UserStatus: "UNCONFIRMED",
               Attributes: [
-                { Name: "sub", Value: "1" },
+                { Name: "sub", Value: "uuid-1234" },
                 { Name: "email", Value: "[email protected]" },
                 { Name: "phone_number", Value: "0411000111" },
               ],
@@ -161,6 +165,8 @@ describe("User Pool Client", () => {
               return Promise.resolve(users);
             } else if (key === "Options") {
               return Promise.resolve(options);
+            } else if (Array.isArray(key) && key[0] === "Users") {
+              return Promise.resolve(users[key[1]]);
             }
 
             return Promise.resolve(null);
@@ -185,11 +191,11 @@ describe("User Pool Client", () => {
           expect(user).toBeNull();
         });
 
-        it("returns existing user by their sub attribute", async () => {
-          const user = await userPool.getUserByUsername("1");
+        it("returns existing user by their username", async () => {
+          const user = await userPool.getUserByUsername("user-supplied");
 
           expect(user).not.toBeNull();
-          expect(user?.Username).toEqual("1");
+          expect(user?.Username).toEqual("user-supplied");
         });
 
         if (find_by_email) {
@@ -199,7 +205,7 @@ describe("User Pool Client", () => {
             );
 
             expect(user).not.toBeNull();
-            expect(user?.Username).toEqual("1");
+            expect(user?.Username).toEqual("user-supplied");
           });
         } else {
           it("does not return the user by their email", async () => {
@@ -216,7 +222,7 @@ describe("User Pool Client", () => {
             const user = await userPool.getUserByUsername("0411000111");
 
             expect(user).not.toBeNull();
-            expect(user?.Username).toEqual("1");
+            expect(user?.Username).toEqual("user-supplied");
           });
         } else {
           it("does not return the user by their phone number", async () => {

diff --git a/src/services/userPoolClient.ts b/src/services/userPoolClient.ts
@@ -140,13 +140,14 @@ export class UserPoolClientService implements UserPoolClient {
       "phone_number"
     );
 
+    const userByUsername = await this.dataStore.get<User>(["Users", username]);
+    if (userByUsername) {
+      return userByUsername;
+    }
+
     const users = await this.dataStore.get<Record<string, User>>("Users", {});
 
     for (const user of Object.values(users)) {
-      if (attributesIncludeMatch("sub", username, user.Attributes)) {
-        return user;
-      }
-
       if (
         aliasEmailEnabled &&
         attributesIncludeMatch("email", username, user.Attributes)
@@ -175,13 +176,6 @@ export class UserPoolClientService implements UserPoolClient {
   public async saveUser(user: User): Promise<void> {
     this.logger.debug("saveUser", user);
 
-    const attributes = attributesInclude("sub", user.Attributes)
-      ? user.Attributes
-      : [{ Name: "sub", Value: user.Username }, ...user.Attributes];
-
-    await this.dataStore.set<User>(`Users.${user.Username}`, {
-      ...user,
-      Attributes: attributes,
-    });
+    await this.dataStore.set<User>(["Users", user.Username], user);
   }
 }
diff --git a/src/targets/confirmForgotPassword.ts b/src/targets/confirmForgotPassword.ts
@@ -19,7 +19,6 @@ export const ConfirmForgotPassword = ({
 >): ConfirmForgotPasswordTarget => async (body) => {
   const userPool = await cognitoClient.getUserPoolForClientId(body.ClientId);
   const user = await userPool.getUserByUsername(body.Username);
-
   if (!user) {
     throw new UserNotFoundError();
   }

diff --git a/src/targets/confirmSignUp.ts b/src/targets/confirmSignUp.ts
@@ -18,7 +18,6 @@ export const ConfirmSignUp = ({
 ) => {
   const userPool = await cognitoClient.getUserPoolForClientId(body.ClientId);
   const user = await userPool.getUserByUsername(body.Username);
-
   if (!user) {
     throw new NotAuthorizedError();
   }

diff --git a/src/targets/initiateAuth.test.ts b/src/targets/initiateAuth.test.ts
@@ -1,5 +1,6 @@
 import { advanceTo } from "jest-date-mock";
 import jwt from "jsonwebtoken";
+import { UUID } from "../__tests__/patterns";
 import {
   InvalidPasswordError,
   NotAuthorizedError,
@@ -21,8 +22,6 @@ import {
   SmsMfaOutput,
 } from "./initiateAuth";
 
-const UUID = /[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/i;
-
 describe("InitiateAuth target", () => {
   let initiateAuth: InitiateAuthTarget;
   let mockCognitoClient: jest.Mocked<CognitoClient>;

diff --git a/src/targets/respondToAuthChallenge.test.ts b/src/targets/respondToAuthChallenge.test.ts
@@ -1,5 +1,6 @@
 import { advanceTo } from "jest-date-mock";
 import jwt from "jsonwebtoken";
+import { UUID } from "../__tests__/patterns";
 import { CodeMismatchError, NotAuthorizedError } from "../errors";
 import PublicKey from "../keys/cognitoLocal.public.json";
 import { CognitoClient, UserPoolClient } from "../services";
@@ -8,8 +9,6 @@ import {
   RespondToAuthChallengeTarget,
 } from "./respondToAuthChallenge";
 
-const UUID = /[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/i;
-
 describe("RespondToAuthChallenge target", () => {
   let respondToAuthChallenge: RespondToAuthChallengeTarget;
   let mockCognitoClient: jest.Mocked<CognitoClient>;