Skip to content

Commit

Permalink
fix: redirect handler creates new session in a way that is compatible…
Browse files Browse the repository at this point in the history
… with session & csrf middleware
  • Loading branch information
activescott committed Jan 14, 2021
1 parent ce2e4f7 commit a6d5e72
Show file tree
Hide file tree
Showing 15 changed files with 145 additions and 174 deletions.
9 changes: 5 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,14 @@ Some super helpful references to keep handy:
- [+] chore: code separated into clean `/client` and `/server` root directories
- [+] feat: bundle static assets (js, css, images) instead of using PUBLIC_URL as described at https://create-react-app.dev/docs/using-the-public-folder/#when-to-use-the-public-folder

- Allow adding multiple OAuth Authorization servers to allow a user to authenticate:
- [ ] Allow adding multiple OAuth Authorization servers to allow a user to authenticate:

- [+] feat: CSRF tokens to protect against login attacks: https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html
- [ ] feat(authentication): configuration for client ID & secret for google
- [ ] feat: DDB tables to store user and table to store tokens by provider
- [ ] feat: google OAuth working (with unit tests that mock google & user interactions)
- [+] feat(authentication): configuration for client ID & secret for google
- [+] feat: DDB tables to store user and table to store tokens by provider
- [+] feat: google OAuth working (with unit tests that mock google & user interactions)
- [ ] feat: user can use one or more OAuth providers with simple configuration
- [ ] feat: logout endpoint (clears the session)

- [ ] feat: CSRF token middleware in all state-changing APIs:

Expand Down
10 changes: 9 additions & 1 deletion server/src/http/get-auth-redirect/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
import * as arc from "@architect/functions"
import oAuthRedirectHandlerFactory from "../../lib/architect/oauth/handlers/redirect"
import { tokenRepositoryFactory } from "../../lib/architect/oauth/repository/TokenRepository"
import userRepositoryFactory from "../../lib/architect/oauth/repository/UserRepository"
import { fetchJson } from "../../lib/fetch"

const impl = oAuthRedirectHandlerFactory(
fetchJson,
userRepositoryFactory(),
tokenRepositoryFactory()
)

const impl = oAuthRedirectHandlerFactory()
export const handler = arc.http.async(impl)
4 changes: 2 additions & 2 deletions server/src/lib/architect/middleware/csrf.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
expectCsrfTokenWithRequest,
CSRF_HEADER_NAME,
} from "./csrf"
import { SESSION_ID_KEY } from "./session"
import { writeSessionID } from "./session"

describe("csrf", () => {
describe("response middleware", () => {
Expand All @@ -27,7 +27,7 @@ describe("csrf", () => {
// CSRF tokens are bound to a session id, so we mock that here and add it to the mock request:
const req: ArchitectHttpRequestPayload = createMockRequest()
const sessionID = "fooID"
req.session[SESSION_ID_KEY] = sessionID
writeSessionID(req, sessionID)
// get a valid token
const tempResponse: ArchitectHttpResponsePayload = {}
await addCsrfTokenToResponse(sessionID, tempResponse)
Expand Down
22 changes: 11 additions & 11 deletions server/src/lib/architect/middleware/csrf.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,17 +35,6 @@ export function expectCsrfTokenWithRequest(
// token exists, is valid, and matched to the session so just exit without returning an error response.
}

function createErrorResponse(
errorMessage: string
): ArchitectHttpResponsePayload {
return {
statusCode: HTTP_STATUS_ERROR,
json: {
message: errorMessage,
},
}
}

/**
* Creates a CSRF token that is matched to the specified session ID.
* @param sessionID The session id that the token should be matched to
Expand Down Expand Up @@ -118,6 +107,17 @@ export async function addCsrfTokenToResponse(
response.headers[CSRF_HEADER_NAME] = await createCSRFToken(sessionID)
}

function createErrorResponse(
errorMessage: string
): ArchitectHttpResponsePayload {
return {
statusCode: HTTP_STATUS_ERROR,
json: {
message: errorMessage,
},
}
}

const createTokenater = (): Tokenater =>
new Tokenater(getSecret(), Tokenater.DAYS_IN_MS * 1)

Expand Down
34 changes: 12 additions & 22 deletions server/src/lib/architect/middleware/session.spec.ts
Original file line number Diff line number Diff line change
@@ -1,36 +1,26 @@
import { addRequestSessionID, SESSION_ID_KEY } from "./session"
import { writeSessionID, readSessionID } from "./session"
import { createMockRequest } from "../../../../test/support/architect"
import { randomInt } from "../../../../test/support"

describe("session", () => {
// note process.env.SESSION_SECRET just avois some warnings
beforeAll(() => (process.env.SESSION_SECRET = "session secretish"))
afterAll(() => delete process.env.SESSION_SECRET)
let testSessionID = beforeEach(() => {
testSessionID = "session-id-" + randomInt().toString()
})

it("should require http.async session on request", async () => {
const req = createMockRequest()
delete req.session
expect(await addRequestSessionID(req)).toHaveProperty("statusCode", 500)
expect(() => writeSessionID(req, testSessionID)).toThrowError(
/missing session middleware/
)
})

it("add a session-id if not exists", async () => {
it("should add and read the same session id", async () => {
const req = createMockRequest()
await addRequestSessionID(req)
writeSessionID(req, testSessionID)
expect(req).toHaveProperty("session")
expect(req.session).toHaveProperty(SESSION_ID_KEY)
const id = req.session[SESSION_ID_KEY]
expect(typeof id).toStrictEqual("string")
expect(id.length).toBeGreaterThan(10)
})

it("should reuse existing session-id if it exists", async () => {
// create a session with SESSION_ID_HEADER_NAME:
const req = createMockRequest()
await addRequestSessionID(req)
expect(req).toHaveProperty("session")
const session1 = req.session[SESSION_ID_KEY]
// now call it again and make sure it doesn't replace it:
await addRequestSessionID(req)
const session2 = req.session[SESSION_ID_KEY]
expect(session1).toEqual(session2)
const found = readSessionID(req)
expect(found).toStrictEqual(testSessionID)
})
})
40 changes: 15 additions & 25 deletions server/src/lib/architect/middleware/session.ts
Original file line number Diff line number Diff line change
@@ -1,39 +1,29 @@
import {
ArchitectHttpRequestPayload,
ArchitectHttpResponsePayload,
} from "../../../types/http"
import { v4 as uuidv4 } from "uuid"

/** The name of the session key to get the session ID value */
export const SESSION_ID_KEY = "SESS-ID-KEY"
const SESSION_ID_KEY = "SESS-ID-KEY"

type HttpResponseLike = { session?: Record<string, string> }
type HttpRequestLike = { session?: Record<string, string> }

/**
* If no request.session.SESSION_ID_TOKEN_HEADER_NAME exists in the session it adds it to request
\ */
export async function addRequestSessionID(
req: ArchitectHttpRequestPayload
): Promise<ArchitectHttpResponsePayload | null> {
if (!req.session) {
* Writes the specified session ID to the specified response.
* @param response This is generally a response, but for testing purposes we also use it to write to requests (and ultimately we just need something with a session so...)
*/
export function writeSessionID(
response: HttpResponseLike,
sessionID: string
): void {
if (!response.session) {
// The request probably didn't use arc.http.async middleware https://arc.codes/docs/en/reference/runtime/node#arc.http.async to automatically add the parse `session` field to req.
return {
statusCode: 500,
json: { message: "missing session middleware" },
}
throw new Error("missing session middleware")
}

// NOTE: arc.http.session is already *signed* so we don't have to worry about tampering of this session ID
const sessionID = req.session[SESSION_ID_KEY] || newSessionID()

// now ensure token and session ID are on the request
req.session[SESSION_ID_KEY] = sessionID
response.session[SESSION_ID_KEY] = sessionID
}

/** Returns the session id for the given request. Assumes the request already has a session id */
export function readSessionID(req: ArchitectHttpRequestPayload): string {
export function readSessionID(req: HttpRequestLike): string {
if (!req.session || !req.session[SESSION_ID_KEY]) {
return null
}
return req.session[SESSION_ID_KEY]
}

const newSessionID = (): string => uuidv4()
4 changes: 2 additions & 2 deletions server/src/lib/architect/oauth/handlers/login.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { createMockRequest } from "../../../../../test/support/architect"
import { addRequestSessionID } from "../../middleware/session"
import { writeSessionID } from "../../middleware/session"
import login from "./login"

describe("login handler", () => {
Expand Down Expand Up @@ -86,7 +86,7 @@ describe("login handler", () => {
const req = createMockRequest()

// to properly create a state token, the handler needs a session id
await addRequestSessionID(req)
writeSessionID(req, "test-session-id")
req.queryStringParameters["provider"] = "GOO"

process.env.OAUTH_GOO_CLIENT_ID = "googcid"
Expand Down
54 changes: 27 additions & 27 deletions server/src/lib/architect/oauth/handlers/redirect.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { randomBytes } from "crypto"
import { createMockRequest } from "../../../../../test/support/architect"
import { ArchitectHttpRequestPayload } from "../../../../types/http"
import { createCSRFToken } from "../../middleware/csrf"
import { addRequestSessionID, readSessionID } from "../../middleware/session"
import { readSessionID, writeSessionID } from "../../middleware/session"
import { tokenRepositoryFactory } from "../repository/TokenRepository"
import userRepositoryFactory from "../repository/UserRepository"
import oAuthRedirectHandlerFactory from "./redirect"
Expand All @@ -22,9 +22,6 @@ afterAll(() => {

afterEach(() => {
process.env = OLD_ENV
})

beforeEach(() => {
// as we're mocking fetch below
jest.resetModules()
})
Expand All @@ -35,8 +32,8 @@ describe("redirect", () => {
it("should display an error if auth server provided unknown/any error query param", async () => {
const oauthRedirectHandler = oAuthRedirectHandlerFactory(
mockFetchJson(),
await userRepositoryFactory(),
await tokenRepositoryFactory()
userRepositoryFactory(),
tokenRepositoryFactory()
)
const req = await mockAuthorizationCodeResponseRequest()
req.queryStringParameters.error = "unknown"
Expand All @@ -50,8 +47,8 @@ describe("redirect", () => {
async (errorCode) => {
const oauthRedirectHandler = oAuthRedirectHandlerFactory(
mockFetchJson(),
await userRepositoryFactory(),
await tokenRepositoryFactory()
userRepositoryFactory(),
tokenRepositoryFactory()
)
const req = await mockAuthorizationCodeResponseRequest()
req.queryStringParameters.error = errorCode
Expand All @@ -69,8 +66,8 @@ describe("redirect", () => {
it("should reject missing state", async () => {
const oauthRedirectHandler = oAuthRedirectHandlerFactory(
mockFetchJson(),
await userRepositoryFactory(),
await tokenRepositoryFactory()
userRepositoryFactory(),
tokenRepositoryFactory()
)
const req = await mockAuthorizationCodeResponseRequest()

Expand All @@ -91,8 +88,8 @@ describe("redirect", () => {
it("should reject invalid state", async () => {
const oauthRedirectHandler = oAuthRedirectHandlerFactory(
mockFetchJson(),
await userRepositoryFactory(),
await tokenRepositoryFactory()
userRepositoryFactory(),
tokenRepositoryFactory()
)
const req = await mockAuthorizationCodeResponseRequest()

Expand Down Expand Up @@ -121,8 +118,8 @@ describe("redirect", () => {
const fetchJson = mockFetchJson()
const oauthRedirectHandler = oAuthRedirectHandlerFactory(
fetchJson,
await userRepositoryFactory(),
await tokenRepositoryFactory()
userRepositoryFactory(),
tokenRepositoryFactory()
)

// invoke handler
Expand Down Expand Up @@ -150,8 +147,8 @@ describe("redirect", () => {
mockProviderConfigInEnvironment()

// setup mocks:
const userRepo = await userRepositoryFactory()
const tokenRepo = await tokenRepositoryFactory()
const userRepo = userRepositoryFactory()
const tokenRepo = tokenRepositoryFactory()
const email = randomEmail()
const fetchJson = mockFetchJsonWithEmail(email)
const oauthRedirectHandler = oAuthRedirectHandlerFactory(
Expand All @@ -175,8 +172,8 @@ describe("redirect", () => {
mockProviderConfigInEnvironment()

// setup mocks:
const tokenRepo = await tokenRepositoryFactory()
const userRepo = await userRepositoryFactory()
const tokenRepo = tokenRepositoryFactory()
const userRepo = userRepositoryFactory()
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 })
Expand Down Expand Up @@ -204,8 +201,8 @@ describe("redirect", () => {
mockProviderConfigInEnvironment()

// setup mocks:
const userRepo = await userRepositoryFactory()
const tokenRepo = await tokenRepositoryFactory()
const userRepo = userRepositoryFactory()
const tokenRepo = tokenRepositoryFactory()
const email = randomEmail()
const fetchJson = mockFetchJsonWithEmail(email)
const oauthRedirectHandler = oAuthRedirectHandlerFactory(
Expand All @@ -232,11 +229,11 @@ describe("redirect", () => {
expect(actualToken.expires_at).toBeGreaterThan(Date.now())
})

it("should write session cookie to indicate the user is indeed logged in", async () => {
it("should create a session to indicate the user is indeed logged in", async () => {
const oauthRedirectHandler = oAuthRedirectHandlerFactory(
mockFetchJson(),
await userRepositoryFactory(),
await tokenRepositoryFactory()
userRepositoryFactory(),
tokenRepositoryFactory()
)
const req = await mockAuthorizationCodeResponseRequest()

Expand All @@ -246,7 +243,10 @@ describe("redirect", () => {
// invoke handler
const res = await oauthRedirectHandler(req)
expect(res).toHaveProperty("statusCode", 302)
expect(res).toHaveProperty("headers.Set-Cookie", expect.anything())
// make sure it created a session
const foundSession = readSessionID(res)
expect(typeof foundSession).toStrictEqual("string")
expect(foundSession.length).toBeGreaterThan(0)
})

it.todo(
Expand All @@ -256,8 +256,8 @@ describe("redirect", () => {
it("should redirect the user to the default after-login redirect page", async () => {
const oauthRedirectHandler = oAuthRedirectHandlerFactory(
mockFetchJson(),
await userRepositoryFactory(),
await tokenRepositoryFactory()
userRepositoryFactory(),
tokenRepositoryFactory()
)
const req = await mockAuthorizationCodeResponseRequest()

Expand All @@ -283,7 +283,7 @@ describe("redirect", () => {
async function mockAuthorizationCodeResponseRequest(): Promise<ArchitectHttpRequestPayload> {
const req = createMockRequest()
// because for state validation we need a session ID
await addRequestSessionID(req)
writeSessionID(req, "test-sess-id")
const sessionID = readSessionID(req)
const csrfToken = await createCSRFToken(sessionID)

Expand Down
Loading

0 comments on commit a6d5e72

Please sign in to comment.