Session management improvements: Multi sessions, renew on login/logout (#603)

* wip: update sessionId on every login

* comment out object.freeze

* only refresh cookies on post

* Add support for multiple sessions per user

* fix tests

* 🛂 Hash sessionId in DB

* 🐛 do not forget about event.locals.sessionId

* Update src/lib/server/auth.ts

Co-authored-by: Eliott C. <coyotte508@gmail.com>

* Add `expiresAt` field

* remove index causing errors

* Fix bug where sessions were not properly being deleted on logout

* Moved session refresh outside of form content check

---------

Co-authored-by: coyotte508 <coyotte508@gmail.com>

Author: nsarrazin

src/hooks.server.ts

@@ -7,18 +7,12 @@ import {
 } from "$env/static/public";
 import { collections } from "$lib/server/database";
 import { base } from "$app/paths";
-import { refreshSessionCookie, requiresUser } from "$lib/server/auth";
+import { findUser, refreshSessionCookie, requiresUser } from "$lib/server/auth";
 import { ERROR_MESSAGES } from "$lib/stores/errors";
+import { sha256 } from "$lib/utils/sha256";
+import { addWeeks } from "date-fns";
 
 export const handle: Handle = async ({ event, resolve }) => {
-	const token = event.cookies.get(COOKIE_NAME);
-
-	const user = token ? await collections.users.findOne({ sessionId: token }) : null;
-
-	if (user) {
-		event.locals.user = user;
-	}
-
 	function errorResponse(status: number, message: string) {
 		const sendJson =
 			event.request.headers.get("accept")?.includes("application/json") ||
@@ -31,17 +25,31 @@ export const handle: Handle = async ({ event, resolve }) => {
 		});
 	}
 
-	if (!token) {
-		const sessionId = crypto.randomUUID();
-		if (await collections.users.findOne({ sessionId })) {
-			return errorResponse(500, "Session ID collision");
+	const token = event.cookies.get(COOKIE_NAME);
+
+	let secretSessionId: string;
+	let sessionId: string;
+
+	if (token) {
+		secretSessionId = token;
+		sessionId = await sha256(token);
+
+		const user = await findUser(sessionId);
+
+		if (user) {
+			event.locals.user = user;
 		}
-		event.locals.sessionId = sessionId;
 	} else {
-		event.locals.sessionId = token;
+		// if the user doesn't have any cookie, we generate one for him
+		secretSessionId = crypto.randomUUID();
+		sessionId = await sha256(secretSessionId);
+
+		if (await collections.sessions.findOne({ sessionId })) {
+			return errorResponse(500, "Session ID collision");
+		}
 	}
 
-	Object.freeze(event.locals);
+	event.locals.sessionId = sessionId;
 
 	// CSRF protection
 	const requestContentType = event.request.headers.get("content-type")?.split(";")[0] ?? "";
@@ -73,13 +81,23 @@ export const handle: Handle = async ({ event, resolve }) => {
 		}
 	}
 
+	if (event.request.method === "POST") {
+		// if the request is a POST request we refresh the cookie
+		refreshSessionCookie(event.cookies, secretSessionId);
+
+		await collections.sessions.updateOne(
+			{ sessionId },
+			{ $set: { updatedAt: new Date(), expiresAt: addWeeks(new Date(), 2) } }
+		);
+	}
+
 	if (
 		!event.url.pathname.startsWith(`${base}/login`) &&
 		!event.url.pathname.startsWith(`${base}/admin`) &&
 		!["GET", "OPTIONS", "HEAD"].includes(event.request.method)
 	) {
 		if (
-			!user &&
+			!event.locals.user &&
 			requiresUser &&
 			!((MESSAGES_BEFORE_LOGIN ? parseInt(MESSAGES_BEFORE_LOGIN) : 0) > 0)
 		) {

src/lib/server/auth.ts

@@ -1,5 +1,5 @@
 import { Issuer, BaseClient, type UserinfoResponse, TokenSet, custom } from "openid-client";
-import { addHours, addYears } from "date-fns";
+import { addHours, addWeeks } from "date-fns";
 import {
 	COOKIE_NAME,
 	OPENID_CLIENT_ID,
@@ -14,6 +14,7 @@ import { sha256 } from "$lib/utils/sha256";
 import { z } from "zod";
 import { dev } from "$app/environment";
 import type { Cookies } from "@sveltejs/kit";
+import { collections } from "./database";
 
 export interface OIDCSettings {
 	redirectURI: string;
@@ -50,10 +51,19 @@ export function refreshSessionCookie(cookies: Cookies, sessionId: string) {
 		sameSite: dev ? "lax" : "none",
 		secure: !dev,
 		httpOnly: true,
-		expires: addYears(new Date(), 1),
+		expires: addWeeks(new Date(), 2),
 	});
 }
 
+export async function findUser(sessionId: string) {
+	const session = await collections.sessions.findOne({ sessionId: sessionId });
+
+	if (!session) {
+		return null;
+	}
+
+	return await collections.users.findOne({ _id: session.userId });
+}
 export const authCondition = (locals: App.Locals) => {
 	return locals.user
 		? { userId: locals.user._id }

src/lib/server/database.ts

@@ -7,6 +7,7 @@ import type { AbortedGeneration } from "$lib/types/AbortedGeneration";
 import type { Settings } from "$lib/types/Settings";
 import type { User } from "$lib/types/User";
 import type { MessageEvent } from "$lib/types/MessageEvent";
+import type { Session } from "$lib/types/Session";
 
 if (!MONGODB_URL) {
 	throw new Error(
@@ -27,6 +28,7 @@ const sharedConversations = db.collection<SharedConversation>("sharedConversatio
 const abortedGenerations = db.collection<AbortedGeneration>("abortedGenerations");
 const settings = db.collection<Settings>("settings");
 const users = db.collection<User>("users");
+const sessions = db.collection<Session>("sessions");
 const webSearches = db.collection<WebSearch>("webSearches");
 const messageEvents = db.collection<MessageEvent>("messageEvents");
 const bucket = new GridFSBucket(db, { bucketName: "files" });
@@ -38,6 +40,7 @@ export const collections = {
 	abortedGenerations,
 	settings,
 	users,
+	sessions,
 	webSearches,
 	messageEvents,
 	bucket,
@@ -65,4 +68,6 @@ client.on("open", () => {
 	users.createIndex({ hfUserId: 1 }, { unique: true }).catch(console.error);
 	users.createIndex({ sessionId: 1 }, { unique: true, sparse: true }).catch(console.error);
 	messageEvents.createIndex({ createdAt: 1 }, { expireAfterSeconds: 60 }).catch(console.error);
+	sessions.createIndex({ expiresAt: 1 }, { expireAfterSeconds: 0 }).catch(console.error);
+	sessions.createIndex({ sessionId: 1 }, { unique: true }).catch(console.error);
 });

src/lib/types/MessageEvent.ts

@@ -1,7 +1,8 @@
+import type { Session } from "./Session";
 import type { Timestamps } from "./Timestamps";
 import type { User } from "./User";
 
 export interface MessageEvent extends Pick<Timestamps, "createdAt"> {
-	userId: User["_id"] | User["sessionId"];
+	userId: User["_id"] | Session["sessionId"];
 	ip?: string;
 }

src/lib/types/Session.ts

@@ -0,0 +1,12 @@
+import type { ObjectId } from "bson";
+import type { Timestamps } from "./Timestamps";
+import type { User } from "./User";
+
+export interface Session extends Timestamps {
+	_id: ObjectId;
+	sessionId: string;
+	userId: User["_id"];
+	userAgent?: string;
+	ip?: string;
+	expiresAt: Date;
+}

src/lib/types/User.ts

@@ -9,7 +9,4 @@ export interface User extends Timestamps {
 	email?: string;
 	avatarUrl: string;
 	hfUserId: string;
-
-	// Session identifier, stored in the cookie
-	sessionId: string;
 }

src/routes/login/callback/+page.server.ts

@@ -4,7 +4,7 @@ import { z } from "zod";
 import { base } from "$app/paths";
 import { updateUser } from "./updateUser";
 
-export async function load({ url, locals, cookies }) {
+export async function load({ url, locals, cookies, request, getClientAddress }) {
 	const { error: errorName, error_description: errorDescription } = z
 		.object({
 			error: z.string().optional(),
@@ -33,7 +33,13 @@ export async function load({ url, locals, cookies }) {
 
 	const { userData } = await getOIDCUserData({ redirectURI: validatedToken.redirectUrl }, code);
 
-	await updateUser({ userData, locals, cookies });
+	await updateUser({
+		userData,
+		locals,
+		cookies,
+		userAgent: request.headers.get("user-agent") ?? undefined,
+		ip: getClientAddress(),
+	});
 
 	throw redirect(302, `${base}/`);
 }

src/routes/login/callback/updateUser.spec.ts

@@ -5,13 +5,15 @@ import { updateUser } from "./updateUser";
 import { ObjectId } from "mongodb";
 import { DEFAULT_SETTINGS } from "$lib/types/Settings";
 import { defaultModel } from "$lib/server/models";
+import { findUser } from "$lib/server/auth";
 
 const userData = {
 	preferred_username: "new-username",
 	name: "name",
 	picture: "https://example.com/avatar.png",
 	sub: "1234567890",
 };
+Object.freeze(userData);
 
 const locals = {
 	userId: "1234567890",
@@ -32,7 +34,6 @@ const insertRandomUser = async () => {
 		name: userData.name,
 		avatarUrl: userData.picture,
 		hfUserId: userData.sub,
-		sessionId: locals.sessionId,
 	});
 
 	return res.insertedId;
@@ -87,7 +88,7 @@ describe("login", () => {
 	it("should create default settings for new user", async () => {
 		await updateUser({ userData, locals, cookies: cookiesMock });
 
-		const user = await collections.users.findOne({ sessionId: locals.sessionId });
+		const user = await findUser(locals.sessionId);
 
 		assert.exists(user);
 
@@ -140,5 +141,9 @@ describe("login", () => {
 
 afterEach(async () => {
 	await collections.users.deleteMany({ hfUserId: userData.sub });
+	await collections.sessions.deleteMany({});
+
+	locals.userId = "1234567890";
+	locals.sessionId = "1234567890";
 	vi.clearAllMocks();
 });

src/routes/login/callback/updateUser.ts

@@ -1,17 +1,23 @@
-import { authCondition, refreshSessionCookie } from "$lib/server/auth";
+import { refreshSessionCookie } from "$lib/server/auth";
 import { collections } from "$lib/server/database";
 import { ObjectId } from "mongodb";
 import { DEFAULT_SETTINGS } from "$lib/types/Settings";
 import { z } from "zod";
 import type { UserinfoResponse } from "openid-client";
-import type { Cookies } from "@sveltejs/kit";
+import { error, type Cookies } from "@sveltejs/kit";
+import crypto from "crypto";
+import { sha256 } from "$lib/utils/sha256";
+import { addWeeks } from "date-fns";
 
 export async function updateUser(params: {
 	userData: UserinfoResponse;
 	locals: App.Locals;
 	cookies: Cookies;
+	userAgent?: string;
+	ip?: string;
 }) {
-	const { userData, locals, cookies } = params;
+	const { userData, locals, cookies, userAgent, ip } = params;
+
 	const {
 		preferred_username: username,
 		name,
@@ -31,17 +37,43 @@ export async function updateUser(params: {
 		})
 		.parse(userData);
 
+	// check if user already exists
 	const existingUser = await collections.users.findOne({ hfUserId });
 	let userId = existingUser?._id;
 
+	// update session cookie on login
+	const previousSessionId = locals.sessionId;
+	const secretSessionId = crypto.randomUUID();
+	const sessionId = await sha256(secretSessionId);
+
+	if (await collections.sessions.findOne({ sessionId })) {
+		throw error(500, "Session ID collision");
+	}
+
+	locals.sessionId = sessionId;
+
 	if (existingUser) {
 		// update existing user if any
 		await collections.users.updateOne(
 			{ _id: existingUser._id },
 			{ $set: { username, name, avatarUrl } }
 		);
+
+		// remove previous session if it exists and add new one
+		await collections.sessions.deleteOne({ sessionId: previousSessionId });
+		await collections.sessions.insertOne({
+			_id: new ObjectId(),
+			sessionId: locals.sessionId,
+			userId: existingUser._id,
+			createdAt: new Date(),
+			updatedAt: new Date(),
+			userAgent,
+			ip,
+			expiresAt: addWeeks(new Date(), 2),
+		});
+
 		// refresh session cookie
-		refreshSessionCookie(cookies, existingUser.sessionId);
+		refreshSessionCookie(cookies, secretSessionId);
 	} else {
 		// user doesn't exist yet, create a new one
 		const { insertedId } = await collections.users.insertOne({
@@ -53,19 +85,32 @@ export async function updateUser(params: {
 			email,
 			avatarUrl,
 			hfUserId,
-			sessionId: locals.sessionId,
 		});
 
 		userId = insertedId;
 
-		// update pre-existing settings
-		const { matchedCount } = await collections.settings.updateOne(authCondition(locals), {
-			$set: { userId, updatedAt: new Date() },
-			$unset: { sessionId: "" },
+		await collections.sessions.insertOne({
+			_id: new ObjectId(),
+			sessionId: locals.sessionId,
+			userId,
+			createdAt: new Date(),
+			updatedAt: new Date(),
+			userAgent,
+			ip,
+			expiresAt: addWeeks(new Date(), 2),
 		});
 
+		// move pre-existing settings to new user
+		const { matchedCount } = await collections.settings.updateOne(
+			{ sessionId: previousSessionId },
+			{
+				$set: { userId, updatedAt: new Date() },
+				$unset: { sessionId: "" },
+			}
+		);
+
 		if (!matchedCount) {
-			// create new default settings
+			// if no settings found for user, create default settings
 			await collections.settings.insertOne({
 				userId,
 				ethicsModalAcceptedAt: new Date(),
@@ -77,8 +122,11 @@ export async function updateUser(params: {
 	}
 
 	// migrate pre-existing conversations
-	await collections.conversations.updateMany(authCondition(locals), {
-		$set: { userId },
-		$unset: { sessionId: "" },
-	});
+	await collections.conversations.updateMany(
+		{ sessionId: previousSessionId },
+		{
+			$set: { userId },
+			$unset: { sessionId: "" },
+		}
+	);
 }

src/routes/logout/+page.server.ts

@@ -1,10 +1,13 @@
 import { dev } from "$app/environment";
 import { base } from "$app/paths";
 import { COOKIE_NAME } from "$env/static/private";
+import { collections } from "$lib/server/database";
 import { redirect } from "@sveltejs/kit";
 
 export const actions = {
-	default: async function ({ cookies }) {
+	default: async function ({ cookies, locals }) {
+		await collections.sessions.deleteOne({ sessionId: locals.sessionId });
+
 		cookies.delete(COOKIE_NAME, {
 			path: "/",
 			// So that it works inside the space's iframe