Skip to content

Commit

Permalink
feat: Gracefully handle expired JWTs (#4195)
Browse files Browse the repository at this point in the history
  • Loading branch information
DafyddLlyr authored Jan 24, 2025
1 parent 1c3b7a8 commit 24fdb28
Show file tree
Hide file tree
Showing 16 changed files with 142 additions and 55 deletions.
2 changes: 1 addition & 1 deletion api.planx.uk/.env.test.example
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ FILE_API_KEY_GATESHEAD=👻
FILE_API_KEY_DONCASTER=👻

# Editor
EDITOR_URL_EXT=example.com
EDITOR_URL_EXT=https://www.example.com

# Hasura
HASURA_GRAPHQL_URL=http://hasura:8080/v1/graphql
Expand Down
4 changes: 2 additions & 2 deletions api.planx.uk/client/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { CoreDomainClient } from "@opensystemslab/planx-core";
import { getClient } from "./index.js";
import { userContext } from "../modules/auth/middleware.js";
import { getJWT } from "../tests/mockJWT.js";
import { getTestJWT } from "../tests/mockJWT.js";

test("getClient() throws an error if a store is not set", () => {
expect(() => getClient()).toThrow();
Expand All @@ -12,7 +12,7 @@ test("getClient() returns a client if store is set", () => {
getStoreMock.mockReturnValue({
user: {
sub: "123",
jwt: getJWT({ role: "teamEditor" }),
jwt: getTestJWT({ role: "teamEditor" }),
},
});

Expand Down
53 changes: 53 additions & 0 deletions api.planx.uk/errors/requestHandlers.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import type { ErrorRequestHandler } from "express";
import { ServerError } from "./serverError.js";
import airbrake from "../airbrake.js";

/**
* Check for expired JWTs, redirect to /logout if found
*/
export const expiredJWTHandler: ErrorRequestHandler = (
errorObject,
_res,
res,
next,
) => {
const isJWTExpiryError =
errorObject?.name === "UnauthorizedError" &&
errorObject?.message === "jwt expired";

if (!isJWTExpiryError) return next(errorObject);

const logoutPage = new URL("/logout", process.env.EDITOR_URL_EXT!).toString();
return res.redirect(logoutPage);
};

/**
* Fallback error handler
* Must be final Express middleware
*/
export const errorHandler: ErrorRequestHandler = (
errorObject,
_req,
res,
_next,
) => {
const { status = 500, message = "Something went wrong" } = (() => {
if (
airbrake &&
(errorObject instanceof Error || errorObject instanceof ServerError)
) {
airbrake.notify(errorObject);
return {
...errorObject,
message: errorObject.message.concat(", this error has been logged"),
};
} else {
console.log(errorObject);
return errorObject;
}
})();

res.status(status).send({
error: message,
});
};
8 changes: 6 additions & 2 deletions api.planx.uk/modules/auth/middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,9 @@ type UseRoleAuth = (authRoles: Role[]) => RequestHandler;
*/
export const useRoleAuth: UseRoleAuth =
(authRoles) => async (req, res, next) => {
useJWT(req, res, () => {
useJWT(req, res, (err) => {
if (err) return next(err);

if (!req?.user)
return next({
status: 401,
Expand Down Expand Up @@ -273,7 +275,9 @@ export const usePlatformAdminAuth = useRoleAuth(["platformAdmin"]);
* Allow any logged in user to access route, without checking roles
*/
export const useLoginAuth: RequestHandler = (req, res, next) =>
useJWT(req, res, () => {
useJWT(req, res, (err) => {
if (err) return next(err);

if (req?.user?.sub) {
userContext.run(
{
Expand Down
4 changes: 2 additions & 2 deletions api.planx.uk/modules/flows/publish/publish.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import supertest from "supertest";

import { queryMock } from "../../../tests/graphqlQueryMock.js";
import { authHeader, getJWT } from "../../../tests/mockJWT.js";
import { authHeader, getTestJWT } from "../../../tests/mockJWT.js";
import app from "../../../server.js";
import { userContext } from "../../auth/middleware.js";
import { mockFlowData } from "../../../tests/mocks/validateAndPublishMocks.js";
Expand All @@ -11,7 +11,7 @@ beforeAll(() => {
getStoreMock.mockReturnValue({
user: {
sub: "123",
jwt: getJWT({ role: "teamEditor" }),
jwt: getTestJWT({ role: "teamEditor" }),
},
});
});
Expand Down
4 changes: 2 additions & 2 deletions api.planx.uk/modules/flows/validate/validate.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import supertest from "supertest";

import { queryMock } from "../../../tests/graphqlQueryMock.js";
import { authHeader, getJWT } from "../../../tests/mockJWT.js";
import { authHeader, getTestJWT } from "../../../tests/mockJWT.js";
import app from "../../../server.js";
import { flowWithInviteToPay } from "../../../tests/mocks/inviteToPayData.js";
import { userContext } from "../../auth/middleware.js";
Expand All @@ -13,7 +13,7 @@ beforeAll(() => {
getStoreMock.mockReturnValue({
user: {
sub: "123",
jwt: getJWT({ role: "teamEditor" }),
jwt: getTestJWT({ role: "teamEditor" }),
},
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ describe("buildContentFromSessions function", () => {
Address: 1 High Street
Project type: New offices
Expiry Date: 29 May 2026
Link: example.com/team/apply-for-a-lawful-development-certificate/published?sessionId=123`;
Link: https://www.example.com/team/apply-for-a-lawful-development-certificate/published?sessionId=123`;
expect(
await buildContentFromSessions(
sessions as LowCalSession[],
Expand Down Expand Up @@ -110,15 +110,15 @@ describe("buildContentFromSessions function", () => {
Address: 1 High Street
Project type: New offices
Expiry Date: 29 May 2026
Link: example.com/team/apply-for-a-lawful-development-certificate/published?sessionId=123\n\nService: Apply for a Lawful Development Certificate
Link: https://www.example.com/team/apply-for-a-lawful-development-certificate/published?sessionId=123\n\nService: Apply for a Lawful Development Certificate
Address: 2 High Street
Project type: New offices
Expiry Date: 29 May 2026
Link: example.com/team/apply-for-a-lawful-development-certificate/published?sessionId=456\n\nService: Apply for a Lawful Development Certificate
Link: https://www.example.com/team/apply-for-a-lawful-development-certificate/published?sessionId=456\n\nService: Apply for a Lawful Development Certificate
Address: 3 High Street
Project type: New offices
Expiry Date: 29 May 2026
Link: example.com/team/apply-for-a-lawful-development-certificate/published?sessionId=789`;
Link: https://www.example.com/team/apply-for-a-lawful-development-certificate/published?sessionId=789`;
expect(
await buildContentFromSessions(
sessions as LowCalSession[],
Expand Down Expand Up @@ -170,7 +170,7 @@ describe("buildContentFromSessions function", () => {
Address: 1 High Street
Project type: New offices
Expiry Date: 29 May 2026
Link: example.com/team/apply-for-a-lawful-development-certificate/published?sessionId=123`;
Link: https://www.example.com/team/apply-for-a-lawful-development-certificate/published?sessionId=123`;
expect(
await buildContentFromSessions(
sessions as LowCalSession[],
Expand Down Expand Up @@ -203,7 +203,7 @@ describe("buildContentFromSessions function", () => {
Address: Address not submitted
Project type: New offices
Expiry Date: 29 May 2026
Link: example.com/team/apply-for-a-lawful-development-certificate/published?sessionId=123`;
Link: https://www.example.com/team/apply-for-a-lawful-development-certificate/published?sessionId=123`;
expect(
await buildContentFromSessions(
sessions as LowCalSession[],
Expand Down Expand Up @@ -237,7 +237,7 @@ describe("buildContentFromSessions function", () => {
Address: 1 High Street
Project type: Project type not submitted
Expiry Date: 29 May 2026
Link: example.com/team/apply-for-a-lawful-development-certificate/published?sessionId=123`;
Link: https://www.example.com/team/apply-for-a-lawful-development-certificate/published?sessionId=123`;
expect(
await buildContentFromSessions(
sessions as LowCalSession[],
Expand Down
3 changes: 2 additions & 1 deletion api.planx.uk/modules/saveAndReturn/service/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ describe("getResumeLink util function", () => {
propertyType: "house",
};
const testCase = getResumeLink(session, { slug: "team" } as Team, "flow");
const expectedResult = "example.com/team/flow/published?sessionId=123";
const expectedResult =
"https://www.example.com/team/flow/published?sessionId=123";
expect(testCase).toEqual(expectedResult);
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
} from "../../../tests/mocks/saveAndReturnMocks.js";
import type { Node, Flow, Breadcrumb } from "../../../types.js";
import { userContext } from "../../auth/middleware.js";
import { getJWT } from "../../../tests/mockJWT.js";
import { getTestJWT } from "../../../tests/mockJWT.js";

const validateSessionPath = "/validate-session";
const getStoreMock = vi.spyOn(userContext, "getStore");
Expand All @@ -28,7 +28,7 @@ describe("Validate Session endpoint", () => {
getStoreMock.mockReturnValue({
user: {
sub: "123",
jwt: getJWT({ role: "teamEditor" }),
jwt: getTestJWT({ role: "teamEditor" }),
},
});
});
Expand Down
4 changes: 2 additions & 2 deletions api.planx.uk/modules/team/service.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { getJWT } from "../../tests/mockJWT.js";
import { getTestJWT } from "../../tests/mockJWT.js";
import { userContext } from "../auth/middleware.js";

import {
Expand Down Expand Up @@ -40,7 +40,7 @@ describe("TeamService", () => {
getStoreMock.mockReturnValue({
user: {
sub: "123",
jwt: getJWT({ role: "teamEditor" }),
jwt: getTestJWT({ role: "teamEditor" }),
},
});
});
Expand Down
21 changes: 18 additions & 3 deletions api.planx.uk/modules/user/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import supertest from "supertest";
import app from "../../server.js";
import { authHeader, getJWT } from "../../tests/mockJWT.js";
import {
authHeader,
expiredAuthHeader,
getTestJWT,
} from "../../tests/mockJWT.js";
import { userContext } from "../auth/middleware.js";

const getStoreMock = vi.spyOn(userContext, "getStore");
Expand Down Expand Up @@ -38,7 +42,7 @@ describe("/me endpoint", () => {
getStoreMock.mockReturnValue({
user: {
sub: "123",
jwt: getJWT({ role: "teamEditor" }),
jwt: getTestJWT({ role: "teamEditor" }),
},
});
});
Expand All @@ -58,7 +62,7 @@ describe("/me endpoint", () => {
getStoreMock.mockReturnValue({
user: {
sub: undefined,
jwt: getJWT({ role: "teamEditor" }),
jwt: getTestJWT({ role: "teamEditor" }),
},
});

Expand Down Expand Up @@ -87,6 +91,17 @@ describe("/me endpoint", () => {
});
});

it("returns a redirect for an expired JWT", async () => {
await supertest(app)
.get("/user/me")
.set(expiredAuthHeader({ role: "teamEditor" }))
.expect(302)
.then((res) => {
expect(res.redirect).toBe(true);
expect(res.header.location).toMatch(/logout/);
});
});

it("returns user details for a logged in user", async () => {
await supertest(app)
.get("/user/me")
Expand Down
29 changes: 3 additions & 26 deletions api.planx.uk/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,14 @@ import cookieParser from "cookie-parser";
import cookieSession from "cookie-session";
import type { CorsOptions } from "cors";
import cors from "cors";
import type { ErrorRequestHandler } from "express";
import express from "express";
import pinoLogger from "express-pino-logger";
import helmet from "helmet";
import { Server, type IncomingMessage } from "http";
import "isomorphic-fetch";
import noir from "pino-noir";
import airbrake from "./airbrake.js";
import { useSwaggerDocs } from "./docs/index.js";
import { ServerError } from "./errors/index.js";
import { errorHandler, expiredJWTHandler } from "./errors/requestHandlers.js";
import adminRoutes from "./modules/admin/routes.js";
import analyticsRoutes from "./modules/analytics/routes.js";
import getPassport from "./modules/auth/passport.js";
Expand Down Expand Up @@ -151,30 +149,9 @@ app.use(testRoutes);
app.use(userRoutes);
app.use(webhookRoutes);

const errorHandler: ErrorRequestHandler = (errorObject, _req, res, _next) => {
const { status = 500, message = "Something went wrong" } = (() => {
if (
airbrake &&
(errorObject instanceof Error || errorObject instanceof ServerError)
) {
airbrake.notify(errorObject);
return {
...errorObject,
message: errorObject.message.concat(", this error has been logged"),
};
} else {
console.log(errorObject);
return errorObject;
}
})();

res.status(status).send({
error: message,
});
};

// Handle any server errors that were passed with next(err)
// Order is significant, this should be the final app.use()
// Order is significant, these should be the final app.use()
app.use(expiredJWTHandler);
app.use(errorHandler);

const server = new Server(app);
Expand Down
24 changes: 20 additions & 4 deletions api.planx.uk/tests/mockJWT.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
import type { Role } from "@opensystemslab/planx-core/types";
import jwt from "jsonwebtoken";

function getJWT({ role }: { role: Role }) {
function getTestJWT({
role,
isExpired = false,
}: {
role: Role;
isExpired?: boolean;
}) {
const data = {
sub: "123",
email: "[email protected]",
Expand All @@ -10,13 +16,23 @@ function getJWT({ role }: { role: Role }) {
"x-hasura-default-role": role,
"x-hasura-user-id": "123",
},
// 1st Jan, 1970
...(isExpired && { exp: 0 }),
};

return jwt.sign(data, process.env.JWT_SECRET!, { expiresIn: "24h" });
return isExpired
? // Expiry hardcoded to token
jwt.sign(data, process.env.JWT_SECRET!)
: // Generate standard 24hr expiry
jwt.sign(data, process.env.JWT_SECRET!, { expiresIn: "24h" });
}

function authHeader({ role }: { role: Role }) {
return { Authorization: `Bearer ${getJWT({ role })}` };
return { Authorization: `Bearer ${getTestJWT({ role })}` };
}

export { authHeader, getJWT };
function expiredAuthHeader({ role }: { role: Role }) {
return { Authorization: `Bearer ${getTestJWT({ role, isExpired: true })}` };
}

export { authHeader, getTestJWT, expiredAuthHeader };
2 changes: 1 addition & 1 deletion e2e/tests/ui-driven/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export const gqlAdmin = async (query, variables = {}) => {
return json;
};

export const getJWT = (userId) => {
export const getTestJWT = (userId) => {
const data = {
sub: String(userId),
"https://hasura.io/jwt/claims": {
Expand Down
Loading

0 comments on commit 24fdb28

Please sign in to comment.