Switch to using zod for validation for account recovery endpoints. (#…

…1555) * add zod schemas for account recovery * use schemas for validation * requested changes * requested changes * fix tests * requested changes
dyc3 · Mar 25, 2024 · dd9dc30 · dd9dc30
1 parent 16c059a
commit dd9dc30
Show file tree

Hide file tree

Showing 4 changed files with 54 additions and 31 deletions.
diff --git a/common/models/rest-api.ts b/common/models/rest-api.ts
@@ -8,6 +8,8 @@ import {
 	OttApiRequestVoteSchema,
 	OttApiRequestAddToQueueSchema,
 	OttApiRequestRemoveFromQueueSchema,
+	OttApiRequestAccountRecoveryStartSchema,
+	OttApiRequestAccountRecoveryVerifySchema,
 } from "./zod-schemas";
 import { z } from "zod";
 
@@ -83,12 +85,10 @@ export type OttApiResponseAddPreview = {
 
 export type OttApiRequestVote = z.infer<typeof OttApiRequestVoteSchema>;
 
-export type OttApiRequestAccountRecoveryStart = {
-	email?: string;
-	username?: string;
-};
+export type OttApiRequestAccountRecoveryStart = z.infer<
+	typeof OttApiRequestAccountRecoveryStartSchema
+>;
 
-export type OttApiRequestAccountRecoveryVerify = {
-	verifyKey: string;
-	newPassword: string;
-};
+export type OttApiRequestAccountRecoveryVerify = z.infer<
+	typeof OttApiRequestAccountRecoveryVerifySchema
+>;
diff --git a/common/models/zod-schemas.ts b/common/models/zod-schemas.ts
@@ -41,3 +41,17 @@ export const OttApiRequestAddToQueueSchema = z.union([
 export const OttApiRequestRemoveFromQueueSchema = z.object({
 	...VideoIdSchema.shape,
 });
+
+export const OttApiRequestAccountRecoveryStartSchema = z.union([
+	z.object({
+		email: z.string().email().min(3),
+	}),
+	z.object({
+		username: z.string(),
+	}),
+]);
+
+export const OttApiRequestAccountRecoveryVerifySchema = z.object({
+	verifyKey: z.string(),
+	newPassword: z.string(),
+});
diff --git a/server/tests/unit/api/accountrecovery.spec.ts b/server/tests/unit/api/accountrecovery.spec.ts
@@ -29,7 +29,7 @@ describe("Account Recovery", () => {
 		app = (await main()).app;
 
 		emailUser = await usermanager.registerUser({
-			email: "email@localhost",
+			email: "email@localhost.com",
 			username: "email user",
 			password: "test1234",
 		});
@@ -60,7 +60,7 @@ describe("Account Recovery", () => {
 	});
 
 	it.each([
-		["email", "email@localhost"],
+		["email", "email@localhost.com"],
 		["username", "email user"],
 	])(
 		"should send a recovery email when using %s field in request",
@@ -77,7 +77,7 @@ describe("Account Recovery", () => {
 			const mailer: MockMailer = usermanager.mailer as MockMailer;
 			expect(mailer.sentEmails).toHaveLength(1);
 			expect(mailer.sentEmails[0]).toMatchObject({
-				to: "email@localhost",
+				to: "email@localhost.com",
 			});
 		}
 	);
@@ -115,7 +115,7 @@ describe("Account Recovery", () => {
 	);
 
 	it("should change the password when the verify key is valid", async () => {
-		await redisClient.set("accountrecovery:foo", "email@localhost");
+		await redisClient.set("accountrecovery:foo", "email@localhost.com");
 
 		const body: OttApiRequestAccountRecoveryVerify = {
 			verifyKey: "foo",
@@ -139,7 +139,7 @@ describe("Account Recovery", () => {
 		{ verifyKey: "foo", newPassword: "asdf" },
 		{ verifyKey: "bar", newPassword: "asdf1234" },
 	])("should not change the password when the request is bad: %s", async (body: any) => {
-		await redisClient.set("accountrecovery:foo", "email@localhost");
+		await redisClient.set("accountrecovery:foo", "email@localhost.com");
 
 		const resp = await request(app)
 			.post("/api/user/recover/verify")

diff --git a/server/usermanager.ts b/server/usermanager.ts
@@ -34,6 +34,12 @@ import type {
 } from "ott-common/models/rest-api";
 import { counterHttpErrors } from "./metrics";
 import { OttException } from "ott-common/exceptions";
+import {
+	OttApiRequestAccountRecoveryStartSchema,
+	OttApiRequestAccountRecoveryVerifySchema,
+} from "ott-common/models/zod-schemas";
+import { ZodError } from "zod";
+import { fromZodError } from "zod-validation-error";
 
 const pwd = securePassword();
 const log = getLogger("usermanager");
@@ -739,25 +745,25 @@ async function isEmailTaken(email: string): Promise<boolean> {
 		.then(room => (room ? true : false))
 		.catch(() => false);
 }
+
+function isEmailRequest(request: OttApiRequestAccountRecoveryStart): request is { email: string } {
+	return "email" in request;
+}
+
 const accountRecoveryStart: RequestHandler<
 	unknown,
 	OttResponseBody<{}>,
 	OttApiRequestAccountRecoveryStart
 > = async (req, res) => {
+	const body = OttApiRequestAccountRecoveryStartSchema.parse(req.body);
 	if (conf.get("rate_limit.enabled")) {
 		await consumeRateLimitPoints(res, req.ip, 100);
 	}
 
-	if (!("email" in req.body) && !("username" in req.body)) {
-		throw new BadApiArgumentException(
-			"email,username",
-			"Either email or username is required."
-		);
-	}
-
 	const query = {
-		user: req.body.email ?? req.body.username,
+		user: isEmailRequest(body) ? body.email : body.username,
 	};
+
 	const user = await getUser(query);
 
 	if (!user.email) {
@@ -776,22 +782,17 @@ const accountRecoveryVerify: RequestHandler<
 	OttResponseBody<{}>,
 	OttApiRequestAccountRecoveryVerify
 > = async (req, res) => {
-	if (!("verifyKey" in req.body)) {
-		throw new BadApiArgumentException("verifyKey", "missing");
-	}
-	if (!("newPassword" in req.body)) {
-		throw new BadApiArgumentException("newPassword", "missing");
-	}
+	const body = OttApiRequestAccountRecoveryVerifySchema.parse(req.body);
 
 	if (conf.get("rate_limit.enabled")) {
 		await consumeRateLimitPoints(res, req.ip, 10);
 	}
 
-	if (!isPasswordValid(req.body.newPassword)) {
+	if (!isPasswordValid(body.newPassword)) {
 		throw new BadPasswordError();
 	}
 
-	const accountEmail = await redisClient.get(`accountrecovery:${req.body.verifyKey}`);
+	const accountEmail = await redisClient.get(`accountrecovery:${body.verifyKey}`);
 	if (!accountEmail) {
 		throw new InvalidVerifyKey();
 	}
@@ -801,12 +802,12 @@ const accountRecoveryVerify: RequestHandler<
 		throw new UserNotFound();
 	}
 
-	await changeUserPassword(user, req.body.newPassword, {
+	await changeUserPassword(user, body.newPassword, {
 		// we already validated the password above
 		validatePassword: false,
 	});
 
-	await redisClient.del(`accountrecovery:${req.body.verifyKey}`);
+	await redisClient.del(`accountrecovery:${body.verifyKey}`);
 
 	if (req.token) {
 		await tokens.setSessionInfo(req.token, { isLoggedIn: true, user_id: user.id });
@@ -893,6 +894,14 @@ const errorHandler: ErrorRequestHandler = (err: Error, req, res) => {
 			success: false,
 			error: err,
 		});
+	} else if (err instanceof ZodError) {
+		err = fromZodError(err);
+		res.status(400).json({
+			success: false,
+			error: {
+				name: err.name,
+			},
+		});
 	} else {
 		log.error(`Unhandled exception: path=${req.path} ${err.name} ${err.message} ${err.stack}`);
 		res.status(500).json({