revoke access token issued for auth token if auth token is used more …

…than once

@see https://datatracker.ietf.org/doc/html/rfc6749.html#section-4.1.2

e2e/oauth2.spec.ts

@@ -11,7 +11,7 @@ import {
 	DUMMY_CLIENT_SECRET,
 } from "../database/createDummyData.js";
 import { query } from "../database/database.js";
-import { createAccessTokenForAuthorizationCode } from "../library/oauth2/accessToken.js";
+import { createAccessTokenForAuthorizationToken } from "../library/oauth2/accessToken.js";
 import { createAuthorizationToken } from "../library/oauth2/authorizationToken.js";
 import { type UserData, findUserByUsername } from "../library/user.js";
 import type { AccessTokenRequestQueryParams } from "../routes/api.js";
@@ -779,7 +779,6 @@ test("/token endpoint should respond with 400 status code and invalid_grant erro
 }) => {
 	const codeVerifier = generateCodeVerifier();
 	const codeChallenge = createHash("sha256").update(codeVerifier).digest("base64url");
-
 	const userId = ((await findUserByUsername("MarkS")) as UserData).id;
 	const authorizationCode = cryptoRandomString({ length: 64, characters: "alphanumeric" });
 	// Set up so the user has already approved the authorization request and we manually create the
@@ -817,6 +816,69 @@ test("/token endpoint should respond with 400 status code and invalid_grant erro
 	expectTokenEndpointHeadersAreCorrect(response.headers());
 });
 
+test("/token endpoint should revoke all access tokens previously issued from an authorization code if that code is used twice", async ({
+	request,
+}) => {
+	const codeVerifier = generateCodeVerifier();
+	const codeChallenge = createHash("sha256").update(codeVerifier).digest("base64url");
+	const userId = ((await findUserByUsername("MarkS")) as UserData).id;
+	// Create an authorization code that was already used to get an access token.
+	const authorizationCode = await createAuthorizationToken({
+		clientId: DUMMY_CLIENT_ID,
+		codeChallenge,
+		userId,
+	});
+	const accessToken = await createAccessTokenForAuthorizationToken(authorizationCode);
+	// Guard assertion that checks that access token initially works for fetching user info.
+	await assertUserinfoEndpointWorks(accessToken.value, {
+		sub: userId,
+		preferred_username: "MarkS",
+		given_name: "Mark",
+		family_name: "Scout",
+	});
+
+	const tokenRequestParameters = {
+		grant_type: "authorization_code",
+		redirect_uri: DUMMY_CLIENT_REDIRECT_URI,
+		code: authorizationCode,
+		code_verifier: codeVerifier,
+	};
+	const tokenResponse = await request.post("/api/v1/token", {
+		headers: {
+			"Content-Type": "application/x-www-form-urlencoded",
+			Authorization: `Basic ${base64encode(`${DUMMY_CLIENT_ID}:${DUMMY_CLIENT_SECRET}`)}`,
+		},
+		form: tokenRequestParameters,
+	});
+
+	expect(tokenResponse.status()).toEqual(400);
+	expect(tokenResponse.statusText()).toEqual("Bad Request");
+	expect(await tokenResponse.json()).toEqual({ error: "invalid_grant" });
+	expectTokenEndpointHeadersAreCorrect(tokenResponse.headers());
+
+	// Trying to use old access token to fetch user info will not work now.
+	const response = await request.get("/api/v1/userinfo", {
+		headers: { Authorization: `Bearer ${base64encode(accessToken.value)}` },
+	});
+	expect(response.status()).toEqual(401);
+	expect(response.statusText()).toEqual("Unauthorized");
+	expect(response.headers()["www-authenticate"]).toEqual("Bearer");
+	expect(await response.json()).toEqual({ error: "invalid_token" });
+
+	// Requesting the access token with the same authorization token will also not work.
+	const secondTokenResponse = await request.post("/api/v1/token", {
+		headers: {
+			"Content-Type": "application/x-www-form-urlencoded",
+			Authorization: `Basic ${base64encode(`${DUMMY_CLIENT_ID}:${DUMMY_CLIENT_SECRET}`)}`,
+		},
+		form: tokenRequestParameters,
+	});
+	expect(secondTokenResponse.status()).toEqual(400);
+	expect(secondTokenResponse.statusText()).toEqual("Bad Request");
+	expect(await secondTokenResponse.json()).toEqual({ error: "invalid_grant" });
+	expectTokenEndpointHeadersAreCorrect(secondTokenResponse.headers());
+});
+
 test("/userinfo endpoint should respond with 401 error code if access token is invalid", async ({
 	request,
 }) => {
@@ -857,7 +919,7 @@ test("/userinfo endpoint should respond with 401 error code if access token has
 		userId: user?.id,
 		codeChallenge,
 	});
-	const { value: accessToken } = await createAccessTokenForAuthorizationCode(authorizationCode);
+	const { value: accessToken } = await createAccessTokenForAuthorizationToken(authorizationCode);
 
 	// First test that endpoint still works minutes before expiration.
 	await query("UPDATE access_tokens SET created_at = $1 WHERE value = $2", [
@@ -886,11 +948,6 @@ test("/userinfo endpoint should respond with 401 error code if access token has
 	expect(await response.json()).toEqual({ error: "invalid_token" });
 });
 
-/**
- * TODO validation for POST /token tests:
- * - if access token is requested twice for the same auth code, access_token is invalidated and user must sign in again
- */
-
 function generateCodeVerifier() {
 	return cryptoRandomString({
 		length: 64,

library/oauth2/accessToken.test.ts

@@ -1,18 +1,19 @@
 import { createHash } from "node:crypto";
 import cryptoRandomString from "crypto-random-string";
 import { differenceInSeconds, subHours } from "date-fns";
-import { describe, expect, it } from "vitest";
+import { afterEach, describe, expect, it, vi } from "vitest";
 import { DUMMY_CLIENT_ID } from "../../database/createDummyData.js";
 import { query } from "../../database/database.js";
 import { findUserByUsername } from "../user.js";
 import {
 	type AccessTokenData,
-	createAccessTokenForAuthorizationCode,
+	createAccessTokenForAuthorizationToken,
 	extractAccessTokenFromHeader,
 	findAccessTokenByValue,
 	hasTokenExpired,
+	revokeAccessTokenIssuedByAuthorizationToken,
 } from "./accessToken.js";
-import { createAuthorizationToken } from "./authorizationToken.js";
+import { createAuthorizationToken, findAuthorizationTokenByCode } from "./authorizationToken.js";
 
 describe("fetching access token from database by code", () => {
 	it("if token with specified code is not found, return null", async () => {
@@ -30,7 +31,7 @@ describe("fetching access token from database by code", () => {
 			userId,
 			codeChallenge,
 		});
-		const accessToken = await createAccessTokenForAuthorizationCode(authorizationToken);
+		const accessToken = await createAccessTokenForAuthorizationToken(authorizationToken);
 		const expectedTokenCreationDate = new Date();
 
 		const tokenData = (await findAccessTokenByValue(accessToken.value)) as AccessTokenData;
@@ -62,7 +63,7 @@ describe("generating access token", () => {
 		});
 
 		const { value, scope, expiresIn } =
-			await createAccessTokenForAuthorizationCode(authorizationToken);
+			await createAccessTokenForAuthorizationToken(authorizationToken);
 
 		expect(value).not.toBeFalsy();
 		expect(value.length).toEqual(64);
@@ -72,7 +73,7 @@ describe("generating access token", () => {
 
 	it("generating access token should throw error if provided authorization token does not exist", async () => {
 		await expect(
-			async () => await createAccessTokenForAuthorizationCode("this auth code does not exist"),
+			async () => await createAccessTokenForAuthorizationToken("this auth code does not exist"),
 		).rejects.toThrowError("Authorization code not found.");
 	});
 
@@ -87,10 +88,10 @@ describe("generating access token", () => {
 			codeChallenge,
 		});
 
-		await createAccessTokenForAuthorizationCode(authorizationToken);
+		await createAccessTokenForAuthorizationToken(authorizationToken);
 
 		await expect(
-			async () => await createAccessTokenForAuthorizationCode(authorizationToken),
+			async () => await createAccessTokenForAuthorizationToken(authorizationToken),
 		).rejects.toThrowError("Authorization code already has an access token.");
 	});
 });
@@ -161,6 +162,58 @@ describe("hasTokenExpired", () => {
 	});
 });
 
+describe("revokeAccessTokenForCode", () => {
+	afterEach(() => {
+		vi.restoreAllMocks();
+	});
+
+	it("should delete access token issued for the code and revoke the authorization code", async () => {
+		const userId = (await findUserByUsername("HellyR"))?.id as string;
+		const codeChallenge = createHash("sha256")
+			.update(generateRandomString({ length: 64 }))
+			.digest("base64url");
+		const authorizationToken = await createAuthorizationToken({
+			clientId: DUMMY_CLIENT_ID,
+			userId,
+			codeChallenge,
+		});
+		const accessToken = await createAccessTokenForAuthorizationToken(authorizationToken);
+
+		await revokeAccessTokenIssuedByAuthorizationToken(authorizationToken);
+
+		expect(await findAccessTokenByValue(accessToken.value)).toBeNull();
+		expect((await findAuthorizationTokenByCode(authorizationToken))?.revoked).toStrictEqual(true);
+	});
+
+	it("should log a warning if authorization token does not exist", async () => {
+		const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
+
+		await revokeAccessTokenIssuedByAuthorizationToken("some_nonexistent_authorization_token");
+
+		expect(consoleWarnSpy).toHaveBeenCalledOnce();
+		expect(consoleWarnSpy).toHaveBeenCalledWith(
+			"Revocation error: authorization token does not exist",
+		);
+	});
+
+	it("should do nothing if access token does not exist for authorization token", async () => {
+		const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
+		const userId = (await findUserByUsername("HellyR"))?.id as string;
+		const codeChallenge = createHash("sha256")
+			.update(generateRandomString({ length: 64 }))
+			.digest("base64url");
+		const authorizationToken = await createAuthorizationToken({
+			clientId: DUMMY_CLIENT_ID,
+			userId,
+			codeChallenge,
+		});
+
+		await revokeAccessTokenIssuedByAuthorizationToken(authorizationToken);
+
+		expect(consoleWarnSpy).not.toHaveBeenCalled();
+	});
+});
+
 function base64encode(text: string): string {
 	return Buffer.from(text).toString("base64");
 }

library/oauth2/accessToken.ts

@@ -1,7 +1,7 @@
 import cryptoRandomString from "crypto-random-string";
 import { addSeconds, isFuture } from "date-fns";
 import { query } from "../../database/database.js";
-import { findAuthorizationTokenByCode } from "./authorizationToken.js";
+import { findAuthorizationTokenByCode, revokeAuthorizationToken } from "./authorizationToken.js";
 
 export interface AccessTokenData {
 	id: number;
@@ -18,7 +18,7 @@ export interface AccessTokenData {
 /**
  * Generates an access token that lasts for 24 hours.
  */
-export async function createAccessTokenForAuthorizationCode(
+export async function createAccessTokenForAuthorizationToken(
 	authorizationToken: string,
 ): Promise<{ value: string; expiresIn: number; scope: string }> {
 	const value = cryptoRandomString({
@@ -91,3 +91,17 @@ export function extractAccessTokenFromHeader(authorizationHeader: string): strin
 export function hasTokenExpired(accessTokenData: AccessTokenData): boolean {
 	return !isFuture(addSeconds(accessTokenData.createdAt, accessTokenData.expiresIn));
 }
+
+export async function revokeAccessTokenIssuedByAuthorizationToken(authorizationToken: string) {
+	const authorizationCodeData = await findAuthorizationTokenByCode(authorizationToken);
+
+	if (authorizationCodeData === null) {
+		console.warn("Revocation error: authorization token does not exist");
+		return;
+	}
+
+	await revokeAuthorizationToken(authorizationToken);
+	await query("DELETE FROM access_tokens WHERE authorization_token_id = $1", [
+		authorizationCodeData.id,
+	]);
+}

library/oauth2/authorizationToken.test.ts

@@ -37,6 +37,7 @@ describe("fetching authorization token from database by code", () => {
 		expect(token?.scope).toEqual("openid");
 		expect(token?.codeChallenge).toEqual(codeChallenge);
 		expect(token?.codeChallengeMethod).toEqual("S256");
+		expect(token?.revoked).toStrictEqual(false);
 		expect(differenceInSeconds(token.createdAt, expectedTokenCreationDate)).toBeLessThan(2);
 	});
 });

library/oauth2/authorizationToken.ts

@@ -11,6 +11,7 @@ export interface AuthorizationTokenData {
 	userId: string;
 	codeChallenge: string;
 	codeChallengeMethod: string;
+	revoked: boolean;
 }
 
 interface CreateAuthorizationTokenArguments {
@@ -47,7 +48,7 @@ export async function findAuthorizationTokenByCode(
 	code: string,
 ): Promise<AuthorizationTokenData | null> {
 	const result = await query(
-		"SELECT id, value, scope, created_at, client_id, user_id, code_challenge, code_challenge_method FROM authorization_tokens WHERE value = $1",
+		"SELECT id, value, scope, created_at, client_id, user_id, revoked, code_challenge, code_challenge_method FROM authorization_tokens WHERE value = $1",
 		[code],
 	);
 
@@ -64,11 +65,18 @@ export async function findAuthorizationTokenByCode(
 		userId: tokenData.user_id,
 		value: tokenData.value,
 		scope: tokenData.scope,
+		revoked: tokenData.revoked,
 		codeChallenge: tokenData.code_challenge,
 		codeChallengeMethod: tokenData.code_challenge_method,
 	};
 }
 
+export async function revokeAuthorizationToken(authorizationToken: string) {
+	await query("UPDATE authorization_tokens SET revoked = true WHERE value = $1", [
+		authorizationToken,
+	]);
+}
+
 export function hasAuthorizationTokenExpired(authorizationToken: AuthorizationTokenData): boolean {
 	if (isAfter(authorizationToken.createdAt, new Date())) {
 		return true;

migrations/20240924001.do.add_revoked_column_for_auth_tokens.sql

@@ -0,0 +1,2 @@
+ALTER TABLE authorization_tokens
+ADD COLUMN revoked boolean NOT NULL DEFAULT false

routes/api.ts

@@ -1,10 +1,11 @@
 import * as argon2 from "argon2";
 import type { FastifyInstance } from "fastify";
 import {
-	createAccessTokenForAuthorizationCode,
+	createAccessTokenForAuthorizationToken,
 	extractAccessTokenFromHeader,
 	findAccessTokenByValue,
 	hasTokenExpired,
+	revokeAccessTokenIssuedByAuthorizationToken,
 } from "../library/oauth2/accessToken.js";
 import {
 	findAuthorizationTokenByCode,
@@ -62,6 +63,7 @@ export default async function frontend(fastify: FastifyInstance) {
 		if (
 			authorizationTokenData === null ||
 			authorizationTokenData.clientId !== clientId ||
+			authorizationTokenData.revoked === true ||
 			hasAuthorizationTokenExpired(authorizationTokenData)
 		) {
 			return reply.code(400).send({ error: "invalid_grant" });
@@ -71,13 +73,29 @@ export default async function frontend(fastify: FastifyInstance) {
 			return reply.code(400).send({ error: "invalid_request" });
 		}
 
-		const { value, scope, expiresIn } = await createAccessTokenForAuthorizationCode(code);
+		let accessTokenData: {
+			value: string;
+			expiresIn: number;
+			scope: string;
+		};
+
+		try {
+			accessTokenData = await createAccessTokenForAuthorizationToken(code);
+		} catch (error: any) {
+			if (error.message === "Authorization code already has an access token.") {
+				await revokeAccessTokenIssuedByAuthorizationToken(code);
+
+				return reply.code(400).send({ error: "invalid_grant" });
+			}
+
+			throw error;
+		}
 
 		return reply.send({
-			access_token: value,
+			access_token: accessTokenData.value,
 			token_type: "Bearer",
-			expires_in: expiresIn,
-			scope,
+			expires_in: accessTokenData.expiresIn,
+			scope: accessTokenData.scope,
 		});
 	});