feat: redirect handler looks up user by sub claim not email claim

README.md

@@ -87,7 +87,8 @@ Some super helpful references to keep handy:
     - [+] Write session cookie as a separate cookie from architect session obj (assume lambda proxy types directly)
     - [ ] Ensure that accounts are linked by sessionid (rather than email address)
       - [x] Store `sub` claim as part of token.
-      - [ ] redirect handler should lookup by `sub` claim not `email` claim.
+      - [x] redirect handler should lookup by `sub` claim not `email` claim.
+      - [ ] fix: sign the session id cookie
     - [ ] should be tested with at least two (for the same user)
     - [ ] Ensures User's ID is preserved with multiple providers (multiple tokens for a single user)
     - [ ] Ensure `email_verified` claim: https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims

server/src/http/get-auth-me/index.ts

@@ -1,4 +1,3 @@
-import * as arc from "@architect/functions"
 import userRepositoryFactory from "@architect/shared/lambda/oauth/repository/UserRepository"
 import identityRepositoryFactory from "@architect/shared/lambda/oauth/repository/IdentityRepository"
 import meHandlerFactory from "@architect/shared/lambda/oauth/handlers/me"
@@ -7,4 +6,4 @@ const handlerImp = meHandlerFactory(
   userRepositoryFactory(),
   identityRepositoryFactory()
 )
-export const handler = arc.http.async(handlerImp)
+export const handler = handlerImp

server/src/shared/lambda.ts

@@ -27,7 +27,7 @@ export type LambdaHttpHandler = (
  */
 export function JsonResponse(
   httpStatusCode: number,
-  body: any
+  body: HttpJsonBody
 ): LambdaHttpResponse {
   return {
     statusCode: httpStatusCode,
@@ -37,3 +37,6 @@ export function JsonResponse(
     body: JSON.stringify(body),
   }
 }
+
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+type HttpJsonBody = any

server/src/shared/lambda/middleware/csrf.ts

@@ -9,8 +9,6 @@ import { readSessionID } from "./session"
 
 export const CSRF_HEADER_NAME = "X-CSRF-TOKEN-X"
 
-const HTTP_STATUS_ERROR = 403
-
 /**
  * Creates a CSRF token that is matched to the specified session ID.
  * @param sessionID The session id that the token should be matched to
@@ -89,15 +87,6 @@ export async function addCsrfTokenToResponse(
   response.headers[CSRF_HEADER_NAME] = await createCSRFToken(sessionID)
 }
 
-function createErrorResponse(errorMessage: string): LambdaHttpResponse {
-  return {
-    statusCode: HTTP_STATUS_ERROR,
-    body: JSON.stringify({
-      message: errorMessage,
-    }),
-  }
-}
-
 const createTokenater = (): Tokenater =>
   new Tokenater(getSecret(), Tokenater.DAYS_IN_MS * 1)
 

server/src/shared/lambda/middleware/session.ts

@@ -52,6 +52,13 @@ export function injectSessionToRequest(
   assert(response.cookies)
   const stolenCookie = response.cookies ? response.cookies[0] : ""
   req.cookies = req.cookies || []
+  if (req.cookies.length > 0) {
+    // if there is already a session cookie here, remove it:
+    const notSessionCookies = req.cookies.filter(
+      (cookieHeader) => !cookieHeader.startsWith(`${SESSION_COOKIE_NAME}=`)
+    )
+    req.cookies = notSessionCookies
+  }
   req.cookies.push(stolenCookie)
 }
 

server/src/shared/lambda/oauth/handlers/httpStatus.ts

@@ -3,5 +3,10 @@
 export const OK = 200
 export const BAD_REQUEST = 400
 export const UNAUTHENTICATED = 401
+/**
+ * The HTTP 403 Forbidden client error status response code indicates that the server understood the request but refuses to authorize it.
+ * This status is similar to 401, but in this case, re-authenticating will make no difference. The access is permanently forbidden and tied to the application logic, such as insufficient rights to a resource.
+ */
+export const FORBIDDEN = 403
 export const NOT_FOUND = 404
 export const INTERNAL_SERVER_ERROR = 500

server/src/shared/lambda/oauth/handlers/me.spec.ts

@@ -5,16 +5,13 @@ import { injectSessionToRequest } from "../../middleware/session"
 import identityRepositoryFactory, {
   StoredIdentity,
 } from "../repository/IdentityRepository"
-import userRepositoryFactory, {
-  UserRepository,
-} from "../repository/UserRepository"
+import userRepositoryFactory from "../repository/UserRepository"
 import meHandlerFactory from "./me"
 
 describe("me handler", () => {
   it("should return a valid user", async () => {
     const req = createMockRequest()
     const testUserID = `user:foo-${randomInt()}`
-    const testUserEmail = `${randomInt()}@testmail.com`
 
     injectSessionToRequest(req, testUserID)
 

server/src/shared/lambda/oauth/handlers/redirect.spec.ts

@@ -6,8 +6,10 @@ import {
   injectSessionToRequest,
   readSessionID,
 } from "../../middleware/session"
-import identityRepositoryFactory from "../repository/IdentityRepository"
-import userRepositoryFactory from "../repository/UserRepository"
+import identityRepositoryFactory, {
+  StoredIdentityProposal,
+} from "../repository/IdentityRepository"
+import userRepositoryFactory, { StoredUser } from "../repository/UserRepository"
 import oAuthRedirectHandlerFactory from "./redirect"
 import * as jwt from "node-webtokens"
 import {
@@ -24,6 +26,7 @@ import { APIGatewayProxyEventQueryStringParameters } from "aws-lambda"
 // note to self: Jest's auto-mocking voodoo wastes more time than it saves. Just inject dependencies (e.g. w/ oAuthRedirectHandlerFactory)
 
 const PROVIDER_NAME = "GOO"
+const PROVIDER_ALTERNATE_NAME = "AAP"
 
 // preserve environment
 const OLD_ENV = process.env
@@ -154,7 +157,11 @@ describe("redirect", () => {
 
     // setup mocks:
     const userRepo = userRepositoryFactory()
+    const userRepoCreateSpy = sinon.spy(userRepo, "create")
+
     const identityRepo = identityRepositoryFactory()
+    const identityRepoUpsertSpy = sinon.spy(identityRepo, "upsert")
+
     const email = randomEmail()
     const fetchJson = mockFetchJsonWithEmail(email)
     const oauthRedirectHandler = oAuthRedirectHandlerFactory(
@@ -165,39 +172,113 @@ describe("redirect", () => {
 
     // invoke handler
     await oauthRedirectHandler(req)
-    await expect(userRepo.getFromEmail(email)).resolves.toHaveProperty(
-      "email",
-      email
-    )
+    expect(userRepoCreateSpy.callCount).toEqual(1)
+    expect(identityRepoUpsertSpy.callCount).toEqual(1)
+
+    const upsertArg = identityRepoUpsertSpy.firstCall.firstArg
+    expect(upsertArg).toHaveProperty("provider", PROVIDER_NAME)
+    expect(upsertArg).toHaveProperty("subject", email)
   })
 
-  it("should NOT create a user that already exists (by email address)", async () => {
-    const req = await mockAuthorizationCodeResponseRequest()
+  it("should NOT recreate existing user", async () => {
+    let req = await mockAuthorizationCodeResponseRequest()
     req.queryStringParameters.provider = PROVIDER_NAME
 
     mockProviderConfigInEnvironment()
 
     // setup mocks:
     const identityRepo = identityRepositoryFactory()
     const userRepo = userRepositoryFactory()
+    const userRepoCreateSpy = sinon.spy(userRepo, "create")
+
+    // setup token response for redirect:
     const email = randomEmail()
-    // now create the user in the user repo so that we can ensure it isn't re-created:
-    await userRepo.add({ email })
+    const tokenResponse = mockFetchJsonWithEmail(email)
 
-    // now make sure no more suers are added:
-    const userRepoMock = sinon.mock(userRepo)
-    userRepoMock.expects("add").never()
+    // setup the handler:
+    const oauthRedirectHandler = oAuthRedirectHandlerFactory(
+      tokenResponse,
+      userRepo,
+      identityRepo
+    )
+    // FIRST redirect/auth:
+    const response = await oauthRedirectHandler(req)
+    const foundSession = readSessionID(response)
+    expect(userRepoCreateSpy.callCount).toEqual(1)
+
+    // SECOND redirect/auth:
+    //   add the session for the last created user and do the auth again:
+    req = await mockAuthorizationCodeResponseRequest(foundSession)
+    await oauthRedirectHandler(req)
+    // here we don't want a second user created for the same authentication info from token response, so make sure it wasn't created:
+    expect(userRepoCreateSpy.callCount).toEqual(1)
+  })
 
-    const fetchJson = mockFetchJsonWithEmail(email)
+  it("should allow user to login with multiple providers", async () => {
+    let req = await mockAuthorizationCodeResponseRequest()
+
+    mockProviderConfigInEnvironment()
+
+    // setup mocks:
+    const identityRepo = identityRepositoryFactory()
+    const identityRepoUpsertSpy = sinon.spy(identityRepo, "upsert")
+    const userRepo = userRepositoryFactory()
+    const userRepoCreateSpy = sinon.spy(userRepo, "create")
+
+    // setup token response for redirect:
+    const email = randomEmail()
+    const tokenResponse = mockFetchJsonWithEmail(email)
+
+    // setup the handler:
     const oauthRedirectHandler = oAuthRedirectHandlerFactory(
-      fetchJson,
+      tokenResponse,
       userRepo,
       identityRepo
     )
 
-    // invoke handler
-    await oauthRedirectHandler(req)
-    userRepoMock.verify()
+    // FIRST redirect/auth:
+    let response = await oauthRedirectHandler(req)
+    expect(response).toHaveProperty("statusCode", 302)
+    const foundSession = readSessionID(response)
+    // should have created 1 user with 1 identity
+    expect(userRepoCreateSpy.callCount).toEqual(1)
+    expect(identityRepoUpsertSpy.callCount).toEqual(1)
+    // get the created user:
+    const createdUser: StoredUser = await userRepoCreateSpy.firstCall
+      .returnValue
+
+    // SECOND redirect/auth with a different provider:
+    req = await mockAuthorizationCodeResponseRequest(foundSession)
+    req.pathParameters = {
+      provider: PROVIDER_ALTERNATE_NAME,
+    }
+    mockProviderConfigInEnvironment(PROVIDER_ALTERNATE_NAME)
+
+    response = await oauthRedirectHandler(req)
+    expect(response).toHaveProperty("statusCode", 302)
+    // expect no new user to be created, so callCount still 1:
+    expect(userRepoCreateSpy.callCount).toEqual(1)
+
+    // expect a SECOND identity to have been created:
+    expect(identityRepoUpsertSpy.callCount).toEqual(2)
+
+    // expect both identities to be for the same user:
+    const upsertCalls = identityRepoUpsertSpy
+      .getCalls()
+      .map((up) => up.firstArg)
+    expect(upsertCalls).toHaveLength(2)
+    // first lets make sure that the two identities were created for different providers
+    const identityProviders = upsertCalls.map(
+      (arg) => (arg as StoredIdentityProposal).provider
+    )
+    expect(identityProviders[0]).not.toEqual(identityProviders[1])
+
+    const identityUserIDs = upsertCalls.map(
+      (arg) => (arg as StoredIdentityProposal).userID
+    )
+    identityUserIDs.forEach((actualUserID) =>
+      expect(actualUserID).toEqual(createdUser.id)
+    )
   })
 
   it("should save access/refresh token into DB", async () => {
@@ -207,6 +288,7 @@ describe("redirect", () => {
 
     // setup mocks:
     const userRepo = userRepositoryFactory()
+    const userRepoCreateSpy = sinon.spy(userRepo, "create")
     const identityRepo = identityRepositoryFactory()
     const email = randomEmail()
     const fetchJson = mockFetchJsonWithEmail(email)
@@ -221,10 +303,7 @@ describe("redirect", () => {
     // invoke handler
     await oauthRedirectHandler(req)
 
-    // NOTE: could mock the userRepo and just return a new user with an ID, to not need a functioning userRepo, but this works for now.
-    const newUser = await userRepo.getFromEmail(email)
-    if (!newUser) throw new Error("newUser must not be null")
-
+    const newUser = await userRepoCreateSpy.firstCall.returnValue
     expect(identityRepoUpsert.callCount).toEqual(1)
     const actualToken = identityRepoUpsert.firstCall.args[0]
     expect(actualToken).toHaveProperty("provider", PROVIDER_NAME)
@@ -316,15 +395,17 @@ type LambdaHttpRequestMock = LambdaHttpRequest &
 /**
  * Mocks out a request to the app from the authorization server
  */
-async function mockAuthorizationCodeResponseRequest(): Promise<LambdaHttpRequestMock> {
+async function mockAuthorizationCodeResponseRequest(
+  userID: string = createAnonymousSessionID()
+): Promise<LambdaHttpRequestMock> {
   const req = createMockRequest()
   // we expect a path param that specifies the provider name:
   req.pathParameters = {
     provider: PROVIDER_NAME,
   }
 
   // because for state validation we need a session ID. Since no user is logged in we can kinda create anything, but we'll create an anonymous one:
-  injectSessionToRequest(req, createAnonymousSessionID())
+  injectSessionToRequest(req, userID)
   const sessionID = readSessionID(req)
   const csrfToken = await createCSRFToken(sessionID)
 
@@ -335,7 +416,7 @@ async function mockAuthorizationCodeResponseRequest(): Promise<LambdaHttpRequest
   return req as LambdaHttpRequestMock
 }
 
-function createIDToken(email: string = "foo@bar.com"): string {
+function createIDToken(email: string = `foo-${randomInt()}@bar.com`): string {
   const key = randomBytes(32)
   return jwt.generate("HS256", { sub: email, email: email }, key)
 }

server/src/shared/lambda/oauth/handlers/redirect.ts

@@ -8,6 +8,7 @@ import {
   BAD_REQUEST,
   INTERNAL_SERVER_ERROR,
   UNAUTHENTICATED,
+  FORBIDDEN,
 } from "./httpStatus"
 import * as jwt from "node-webtokens"
 import { addResponseSession, errorResponse, getProviderName } from "./common"
@@ -96,18 +97,47 @@ export default function oAuthRedirectHandlerFactory(
       return claimsError
     }
 
-    // TODO: Below we need to see if we already have this provider + subject rather than this email.
+    let user: StoredUser | null = null
+    // lookup an existing user for the current session (the user may have an active session by signing in with a different identity provider):
+    const sessionID = readSessionID(req)
+    if (sessionID) {
+      // user has a valid session (it could be an anonymous session though):
+      user = await userRepository.get(sessionID)
+    }
 
-    // create user (if they don't exist already):
-    let user: StoredUser | null = await userRepository.getFromEmail(
-      parsed.payload.email
+    // see if any user has logged in and authenticated with this external identity before:
+    const existingIdentity = await identityRepository.getByProviderSubject(
+      providerName,
+      parsed.payload.sub
     )
+
+    // if the current session is an authenticated/non-anonymous user then we must make sure no other user is already logged in with this external identity:
+    if (user && existingIdentity && user.id !== existingIdentity.userID) {
+      // eslint-disable-next-line no-console
+      console.error(
+        "The user",
+        user.id,
+        "attempted to link to external identity from provider",
+        existingIdentity.provider,
+        "with subject",
+        existingIdentity.subject,
+        "but that identity is already linked to different user id",
+        existingIdentity.userID
+      )
+      return errorResponse(
+        FORBIDDEN,
+        "This identity is already linked to another user in this application. Please login with that user and unlink it, then login again with this user to link it to this user."
+      )
+    }
+
+    // create user (if they don't exist already):
     if (!user) {
-      user = await userRepository.add({ email: parsed.payload.email })
+      user = await userRepository.create()
     }
+
     assert(user != null, "user was not found and was not created?")
 
-    // save access/refresh tokens
+    // save identity & access/refresh tokens
     await identityRepository.upsert({
       userID: user.id,
       provider: providerName,

server/src/shared/lambda/oauth/repository/IdentityRepository.ts

@@ -144,7 +144,10 @@ class IdentityRepositoryImpl
     if (!result.Items || result.Items.length === 0) {
       return null
     }
-    assert(result.Items.length <= 1)
+    assert(
+      result.Items.length === 0 || result.Items.length === 1,
+      `unexpected number of items returned (${result.Items.length}) for provider ${provider} and subject ${subject}.`
+    )
     const item = result.Items[0] as StoredIdentity
     assert(!("provider_subject" in item), "unexpected provider_subject")
     return item