From 4ee237c20dd88b2ce643c855379bb53c87bb2cce Mon Sep 17 00:00:00 2001 From: dcshzj <27919917+dcshzj@users.noreply.github.com> Date: Thu, 7 Nov 2024 13:11:22 +0800 Subject: [PATCH 1/4] feat: move to whitelist database table from growthbook --- .../studio/prisma/generated/generatedTypes.ts | 8 ++ .../prisma/generated/selectableTypes.ts | 1 + .../migration.sql | 19 +++ apps/studio/prisma/schema.prisma | 10 ++ apps/studio/prisma/seed.ts | 12 ++ .../auth/email/__tests__/email.router.test.ts | 7 +- .../server/modules/auth/email/email.router.ts | 19 +-- .../__tests__/whitelist.service.test.ts | 125 ++++++++++++++++++ .../modules/whitelist/whitelist.service.ts | 66 +++++++++ apps/studio/src/server/trpc.ts | 17 +-- apps/studio/tests/integration/helpers/auth.ts | 12 ++ .../helpers/growthbook/mockFeatureFlags.ts | 3 - .../tests/integration/helpers/seed/index.ts | 17 +++ 13 files changed, 286 insertions(+), 30 deletions(-) create mode 100644 apps/studio/prisma/migrations/20241107033200_add_whitelist_table/migration.sql create mode 100644 apps/studio/src/server/modules/whitelist/__tests__/whitelist.service.test.ts create mode 100644 apps/studio/src/server/modules/whitelist/whitelist.service.ts diff --git a/apps/studio/prisma/generated/generatedTypes.ts b/apps/studio/prisma/generated/generatedTypes.ts index aa9e169ff4..c4720a3e52 100644 --- a/apps/studio/prisma/generated/generatedTypes.ts +++ b/apps/studio/prisma/generated/generatedTypes.ts @@ -114,6 +114,13 @@ export interface Version { publishedBy: string updatedAt: Generated } +export interface Whitelist { + id: GeneratedAlways + email: string + expiry: Timestamp | null + createdAt: Generated + updatedAt: Generated +} export interface DB { Blob: Blob Footer: Footer @@ -126,4 +133,5 @@ export interface DB { User: User VerificationToken: VerificationToken Version: Version + Whitelist: Whitelist } diff --git a/apps/studio/prisma/generated/selectableTypes.ts b/apps/studio/prisma/generated/selectableTypes.ts index cd5f102f6c..a0dddc122d 100644 --- a/apps/studio/prisma/generated/selectableTypes.ts +++ b/apps/studio/prisma/generated/selectableTypes.ts @@ -14,3 +14,4 @@ export type SiteMember = Selectable export type User = Selectable export type VerificationToken = Selectable export type Version = Selectable +export type Whitelist = Selectable diff --git a/apps/studio/prisma/migrations/20241107033200_add_whitelist_table/migration.sql b/apps/studio/prisma/migrations/20241107033200_add_whitelist_table/migration.sql new file mode 100644 index 0000000000..2851c64bd1 --- /dev/null +++ b/apps/studio/prisma/migrations/20241107033200_add_whitelist_table/migration.sql @@ -0,0 +1,19 @@ +-- CreateTable +CREATE TABLE "Whitelist" ( + "id" SERIAL NOT NULL, + "email" TEXT NOT NULL, + "expiry" TIMESTAMP(3), + "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, + "updatedAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, + + CONSTRAINT "Whitelist_pkey" PRIMARY KEY ("id") +); + +-- CreateIndex +CREATE UNIQUE INDEX "Whitelist_email_key" ON "Whitelist"("email"); + +-- CreateIndex +CREATE INDEX "Whitelist_email_idx" ON "Whitelist"("email"); + +-- AlterTable +CREATE TRIGGER update_timestamp BEFORE UPDATE ON "Whitelist" FOR EACH ROW EXECUTE PROCEDURE moddatetime("updatedAt"); diff --git a/apps/studio/prisma/schema.prisma b/apps/studio/prisma/schema.prisma index 37e070cf24..4f74b88fd7 100644 --- a/apps/studio/prisma/schema.prisma +++ b/apps/studio/prisma/schema.prisma @@ -212,3 +212,13 @@ model RateLimiterFlexible { points Int expire DateTime? } + +model Whitelist { + id Int @id @default(autoincrement()) + email String @unique + expiry DateTime? + createdAt DateTime @default(now()) + updatedAt DateTime @default(now()) @updatedAt + + @@index([email]) +} diff --git a/apps/studio/prisma/seed.ts b/apps/studio/prisma/seed.ts index 12907bd464..030756385f 100644 --- a/apps/studio/prisma/seed.ts +++ b/apps/studio/prisma/seed.ts @@ -259,6 +259,18 @@ async function main() { ) .executeTakeFirstOrThrow() + await db + .insertInto("Whitelist") + .values({ + email: "@open.gov.sg", + }) + .onConflict((oc) => + oc + .column("email") + .doUpdateSet((eb) => ({ email: eb.ref("excluded.email") })), + ) + .executeTakeFirstOrThrow() + await Promise.all( [...ISOMER_ADMINS, ...ISOMER_MIGRATORS, EDITOR_USER, PUBLISHER_USER].map( async (name) => { diff --git a/apps/studio/src/server/modules/auth/email/__tests__/email.router.test.ts b/apps/studio/src/server/modules/auth/email/__tests__/email.router.test.ts index 3c5c093773..55fdb7617a 100644 --- a/apps/studio/src/server/modules/auth/email/__tests__/email.router.test.ts +++ b/apps/studio/src/server/modules/auth/email/__tests__/email.router.test.ts @@ -3,6 +3,7 @@ import { applySession, createMockRequest, } from "tests/integration/helpers/iron-session" +import { setUpWhitelist } from "tests/integration/helpers/seed" import { describe, expect, it } from "vitest" import { env } from "~/env.mjs" @@ -15,16 +16,17 @@ import { getIpFingerprint, LOCALHOST } from "../utils" describe("auth.email", () => { let caller: Awaited> let session: ReturnType + const TEST_VALID_EMAIL = "test@open.gov.sg" beforeEach(async () => { - await resetTables("User", "VerificationToken") + await resetTables("User", "VerificationToken", "Whitelist") + await setUpWhitelist({ email: TEST_VALID_EMAIL }) session = applySession() const ctx = createMockRequest(session) caller = emailSessionRouter.createCaller(ctx) }) describe("login", () => { - const TEST_VALID_EMAIL = "test@open.gov.sg" it("should throw if email is not provided", async () => { // Act const result = caller.login({ email: "" }) @@ -72,7 +74,6 @@ describe("auth.email", () => { }) describe("verifyOtp", () => { - const TEST_VALID_EMAIL = "test@open.gov.sg" const VALID_OTP = "123456" const VALID_TOKEN_HASH = createTokenHash(VALID_OTP, TEST_VALID_EMAIL) const INVALID_OTP = "987643" diff --git a/apps/studio/src/server/modules/auth/email/email.router.ts b/apps/studio/src/server/modules/auth/email/email.router.ts index 797371025c..da69c271c1 100644 --- a/apps/studio/src/server/modules/auth/email/email.router.ts +++ b/apps/studio/src/server/modules/auth/email/email.router.ts @@ -10,6 +10,7 @@ import { import { publicProcedure, router } from "~/server/trpc" import { getBaseUrl } from "~/utils/getBaseUrl" import { defaultMeSelect } from "../../me/me.select" +import { isEmailWhitelisted } from "../../whitelist/whitelist.service" import { VerificationError } from "../auth.error" import { verifyToken } from "../auth.service" import { createTokenHash, createVfnPrefix, createVfnToken } from "../auth.util" @@ -21,19 +22,13 @@ export const emailSessionRouter = router({ .input(emailSignInSchema) .meta({ rateLimitOptions: {} }) .mutation(async ({ ctx, input: { email } }) => { - if (env.NODE_ENV === "production") { - // check if whitelisted email on Growthbook - const defaultWhitelist: string[] = [] - const whitelistedUsers = ctx.gb.getFeatureValue("whitelisted_users", { - whitelist: defaultWhitelist, - }) + const isWhitelisted = await isEmailWhitelisted(email) - if (!whitelistedUsers.whitelist.includes(email)) { - throw new TRPCError({ - code: "UNAUTHORIZED", - message: "Unauthorized. Contact Isomer support.", - }) - } + if (!isWhitelisted) { + throw new TRPCError({ + code: "UNAUTHORIZED", + message: "Unauthorized. Contact Isomer support.", + }) } // TODO: instead of storing expires, store issuedAt to calculate when the next otp can be re-issued diff --git a/apps/studio/src/server/modules/whitelist/__tests__/whitelist.service.test.ts b/apps/studio/src/server/modules/whitelist/__tests__/whitelist.service.test.ts new file mode 100644 index 0000000000..559d4d0992 --- /dev/null +++ b/apps/studio/src/server/modules/whitelist/__tests__/whitelist.service.test.ts @@ -0,0 +1,125 @@ +import { resetTables } from "tests/integration/helpers/db" +import { setUpWhitelist } from "tests/integration/helpers/seed" + +import { isEmailWhitelisted } from "../whitelist.service" + +describe("whitelist.service", () => { + beforeAll(async () => { + const oneYearFromNow = new Date() + oneYearFromNow.setFullYear(oneYearFromNow.getFullYear() + 1) + const oneYearAgo = new Date() + oneYearAgo.setFullYear(oneYearAgo.getFullYear() - 1) + + await resetTables("Whitelist") + await setUpWhitelist({ email: "whitelisted@example.com" }) + await setUpWhitelist({ + email: "vendor-whitelisted@example.com", + expiry: oneYearFromNow, + }) + await setUpWhitelist({ + email: "vendor-expired@example.com", + expiry: oneYearAgo, + }) + await setUpWhitelist({ email: ".gov.sg" }) + await setUpWhitelist({ + email: "@vendor.com.sg", + }) + await setUpWhitelist({ + email: "@whitelisted.com.sg", + expiry: oneYearFromNow, + }) + await setUpWhitelist({ email: "@expired.sg", expiry: oneYearAgo }) + await setUpWhitelist({ + email: "expired@whitelisted.com.sg", + expiry: oneYearAgo, + }) + }) + + it("should show email as whitelisted if the exact email address is whitelisted and expiry is NULL", async () => { + // Arrange + const email = "whitelisted@example.com" + + // Act + const result = await isEmailWhitelisted(email) + + // Assert + expect(result).toBe(true) + }) + + it("should show email as whitelisted if the exact email address is whitelisted and expiry is in the future", async () => { + // Arrange + const email = "vendor-whitelisted@example.com" + + // Act + const result = await isEmailWhitelisted(email) + + // Assert + expect(result).toBe(true) + }) + + it("should show email as not whitelisted if the exact email address is whitelisted and expiry is in the past", async () => { + // Arrange + const email = "vendor-expired@example.com" + + // Act + const result = await isEmailWhitelisted(email) + + // Assert + expect(result).toBe(false) + }) + + it("should show email as whitelisted if the exact email domain is whitelisted and expiry is NULL", async () => { + // Arrange + const email = "user@vendor.com.sg" + + // Act + const result = await isEmailWhitelisted(email) + + // Assert + expect(result).toBe(true) + }) + + it("should show email as whitelisted if the exact email domain is whitelisted and expiry is in the future", async () => { + // Arrange + const email = "user@whitelisted.com.sg" + + // Act + const result = await isEmailWhitelisted(email) + + // Assert + expect(result).toBe(true) + }) + + it("should show email as not whitelisted if the exact email domain is whitelisted and expiry is in the past", async () => { + // Arrange + const email = "user@expired.sg" + + // Act + const result = await isEmailWhitelisted(email) + + // Assert + expect(result).toBe(false) + }) + + it("should show email as whitelisted if the suffix of the email domain is whitelisted and expiry is NULL", async () => { + // Arrange + const email = "user@agency.gov.sg" + + // Act + const result = await isEmailWhitelisted(email) + + // Assert + expect(result).toBe(true) + }) + + it("should show email as whitelisted if the exact email address is expired, but the domain's expiry is in the future", async () => { + // Arrange + const email = "expired@whitelisted.com.sg" + + // Act + const result = await isEmailWhitelisted(email) + + // Assert + expect(result).toBe(true) + }) +}) diff --git a/apps/studio/src/server/modules/whitelist/whitelist.service.ts b/apps/studio/src/server/modules/whitelist/whitelist.service.ts new file mode 100644 index 0000000000..f4a005a660 --- /dev/null +++ b/apps/studio/src/server/modules/whitelist/whitelist.service.ts @@ -0,0 +1,66 @@ +import { TRPCError } from "@trpc/server" + +import { db } from "../database" + +export const isEmailWhitelisted = async (email: string) => { + const lowercaseEmail = email.toLowerCase() + + // Step 1: Check if the exact email address is whitelisted + const exactMatch = await db + .selectFrom("Whitelist") + .where("email", "=", lowercaseEmail) + .where(({ eb }) => + eb.or([eb("expiry", "is", null), eb("expiry", ">", new Date())]), + ) + .select(["id"]) + .executeTakeFirst() + + if (exactMatch) { + return true + } + + // Step 2: Check if the exact email domain is whitelisted + const emailParts = lowercaseEmail.split("@") + if (emailParts.length !== 2 && !emailParts[1]) { + throw new TRPCError({ + code: "BAD_REQUEST", + message: "An invalid email was provided", + }) + } + + const emailDomain = `@${emailParts[1]}` + const domainMatch = await db + .selectFrom("Whitelist") + .where("email", "=", emailDomain) + .where(({ eb }) => + eb.or([eb("expiry", "is", null), eb("expiry", ">", new Date())]), + ) + .select(["id"]) + .executeTakeFirst() + + if (domainMatch) { + return true + } + + // Step 3: Check if the suffix of the email domain is whitelisted + const domainParts = emailDomain.split(".") + for (let i = 1; i < domainParts.length; i++) { + // Suffices should start with a dot (e.g. ".gov.sg") + const suffix = `.${domainParts.slice(i).join(".")}` + + const suffixMatch = await db + .selectFrom("Whitelist") + .where("email", "=", suffix) + .where(({ eb }) => + eb.or([eb("expiry", "is", null), eb("expiry", ">", new Date())]), + ) + .select(["id"]) + .executeTakeFirst() + + if (suffixMatch) { + return true + } + } + + return false +} diff --git a/apps/studio/src/server/trpc.ts b/apps/studio/src/server/trpc.ts index b990d1ab18..262d74f759 100644 --- a/apps/studio/src/server/trpc.ts +++ b/apps/studio/src/server/trpc.ts @@ -20,6 +20,7 @@ import getIP from "~/utils/getClientIp" import { type Context } from "./context" import { defaultMeSelect } from "./modules/me/me.select" import { checkRateLimit } from "./modules/rate-limit/rate-limit.service" +import { isEmailWhitelisted } from "./modules/whitelist/whitelist.service" import { prisma } from "./prisma" interface Meta { @@ -124,13 +125,6 @@ const baseMiddleware = t.middleware(async ({ ctx, next }) => { }) const authMiddleware = t.middleware(async ({ next, ctx }) => { - const defaultWhitelist: string[] = [] - const whitelistedUsers = ctx.gb - .getFeatureValue("whitelisted_users", { - whitelist: defaultWhitelist, - }) - .whitelist.map((email) => email.toLowerCase()) - if (!ctx.session?.userId) { throw new TRPCError({ code: "UNAUTHORIZED" }) } @@ -145,11 +139,10 @@ const authMiddleware = t.middleware(async ({ next, ctx }) => { throw new TRPCError({ code: "UNAUTHORIZED" }) } - // check against Growthbook if user is whitelisted for prod/stg - if (env.NODE_ENV === "production") { - if (!whitelistedUsers.includes(user.email.toLowerCase())) { - throw new TRPCError({ code: "UNAUTHORIZED" }) - } + // Ensure that the user is whitelisted to use the app + const isWhitelisted = await isEmailWhitelisted(user.email) + if (!isWhitelisted) { + throw new TRPCError({ code: "UNAUTHORIZED" }) } return next({ diff --git a/apps/studio/tests/integration/helpers/auth.ts b/apps/studio/tests/integration/helpers/auth.ts index 2ebd56fc6c..df12f9ecb6 100644 --- a/apps/studio/tests/integration/helpers/auth.ts +++ b/apps/studio/tests/integration/helpers/auth.ts @@ -5,6 +5,18 @@ import { db } from "~server/db" import type { User } from "~server/db" export const auth = async ({ id, ...user }: SetOptional) => { + await db + .insertInto("Whitelist") + .values({ + email: user.email, + }) + .onConflict((oc) => + oc + .column("email") + .doUpdateSet((eb) => ({ email: eb.ref("excluded.email") })), + ) + .executeTakeFirstOrThrow() + if (id !== undefined) { return db .updateTable("User") diff --git a/apps/studio/tests/integration/helpers/growthbook/mockFeatureFlags.ts b/apps/studio/tests/integration/helpers/growthbook/mockFeatureFlags.ts index 37abc28a2b..f042312007 100644 --- a/apps/studio/tests/integration/helpers/growthbook/mockFeatureFlags.ts +++ b/apps/studio/tests/integration/helpers/growthbook/mockFeatureFlags.ts @@ -1,6 +1,3 @@ const mockFeatureFlags = new Map() -mockFeatureFlags.set("whitelisted_users", { - whitelist: ["test@open.gov.sg"], -}) export { mockFeatureFlags } diff --git a/apps/studio/tests/integration/helpers/seed/index.ts b/apps/studio/tests/integration/helpers/seed/index.ts index d9c18920f8..4dbeb4cdd9 100644 --- a/apps/studio/tests/integration/helpers/seed/index.ts +++ b/apps/studio/tests/integration/helpers/seed/index.ts @@ -293,3 +293,20 @@ export const setupFolder = async ({ folder, } } + +export const setUpWhitelist = async ({ + email, + expiry, +}: { + email: string + expiry?: Date +}) => { + return db + .insertInto("Whitelist") + .values({ + email, + expiry: expiry ?? null, + }) + .returningAll() + .executeTakeFirstOrThrow() +} From 0dfe78cb1490905d8b31ef106fabff47434f9517 Mon Sep 17 00:00:00 2001 From: dcshzj <27919917+dcshzj@users.noreply.github.com> Date: Thu, 7 Nov 2024 16:30:14 +0800 Subject: [PATCH 2/4] fix: use lowercase email in tests --- apps/studio/tests/integration/helpers/auth.ts | 13 ++----------- apps/studio/tests/integration/helpers/seed/index.ts | 7 ++++++- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/apps/studio/tests/integration/helpers/auth.ts b/apps/studio/tests/integration/helpers/auth.ts index df12f9ecb6..c84e263cb3 100644 --- a/apps/studio/tests/integration/helpers/auth.ts +++ b/apps/studio/tests/integration/helpers/auth.ts @@ -3,19 +3,10 @@ import cuid2 from "@paralleldrive/cuid2" import { db } from "~server/db" import type { User } from "~server/db" +import { setUpWhitelist } from "./seed" export const auth = async ({ id, ...user }: SetOptional) => { - await db - .insertInto("Whitelist") - .values({ - email: user.email, - }) - .onConflict((oc) => - oc - .column("email") - .doUpdateSet((eb) => ({ email: eb.ref("excluded.email") })), - ) - .executeTakeFirstOrThrow() + await setUpWhitelist({ email: user.email }) if (id !== undefined) { return db diff --git a/apps/studio/tests/integration/helpers/seed/index.ts b/apps/studio/tests/integration/helpers/seed/index.ts index 4dbeb4cdd9..5ba82f1462 100644 --- a/apps/studio/tests/integration/helpers/seed/index.ts +++ b/apps/studio/tests/integration/helpers/seed/index.ts @@ -304,9 +304,14 @@ export const setUpWhitelist = async ({ return db .insertInto("Whitelist") .values({ - email, + email: email.toLowerCase(), expiry: expiry ?? null, }) + .onConflict((oc) => + oc + .column("email") + .doUpdateSet((eb) => ({ email: eb.ref("excluded.email") })), + ) .returningAll() .executeTakeFirstOrThrow() } From 805eebf4aa60387f9df7f8d26c09d0c42e876531 Mon Sep 17 00:00:00 2001 From: dcshzj <27919917+dcshzj@users.noreply.github.com> Date: Thu, 7 Nov 2024 21:56:07 +0800 Subject: [PATCH 3/4] chore: add additional guard clause for email validation --- .../src/server/modules/whitelist/whitelist.service.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/apps/studio/src/server/modules/whitelist/whitelist.service.ts b/apps/studio/src/server/modules/whitelist/whitelist.service.ts index f4a005a660..2546d14b36 100644 --- a/apps/studio/src/server/modules/whitelist/whitelist.service.ts +++ b/apps/studio/src/server/modules/whitelist/whitelist.service.ts @@ -1,10 +1,19 @@ import { TRPCError } from "@trpc/server" +import { isValidEmail } from "~/utils/email" import { db } from "../database" export const isEmailWhitelisted = async (email: string) => { const lowercaseEmail = email.toLowerCase() + // Extra guard even if Zod validation has already checked + if (!isValidEmail(lowercaseEmail)) { + throw new TRPCError({ + code: "BAD_REQUEST", + message: "Please sign in with a valid email address.", + }) + } + // Step 1: Check if the exact email address is whitelisted const exactMatch = await db .selectFrom("Whitelist") @@ -24,7 +33,7 @@ export const isEmailWhitelisted = async (email: string) => { if (emailParts.length !== 2 && !emailParts[1]) { throw new TRPCError({ code: "BAD_REQUEST", - message: "An invalid email was provided", + message: "Please sign in with a valid email address.", }) } From b10205433eeb5e3965a6e8dd5f4befa58f603569 Mon Sep 17 00:00:00 2001 From: dcshzj <27919917+dcshzj@users.noreply.github.com> Date: Fri, 8 Nov 2024 18:38:53 +0800 Subject: [PATCH 4/4] fix: enforce using last part of email address --- apps/studio/src/server/modules/whitelist/whitelist.service.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/studio/src/server/modules/whitelist/whitelist.service.ts b/apps/studio/src/server/modules/whitelist/whitelist.service.ts index 2546d14b36..5a356b10b4 100644 --- a/apps/studio/src/server/modules/whitelist/whitelist.service.ts +++ b/apps/studio/src/server/modules/whitelist/whitelist.service.ts @@ -30,14 +30,14 @@ export const isEmailWhitelisted = async (email: string) => { // Step 2: Check if the exact email domain is whitelisted const emailParts = lowercaseEmail.split("@") - if (emailParts.length !== 2 && !emailParts[1]) { + if (emailParts.length !== 2) { throw new TRPCError({ code: "BAD_REQUEST", message: "Please sign in with a valid email address.", }) } - const emailDomain = `@${emailParts[1]}` + const emailDomain = `@${emailParts.pop()}` const domainMatch = await db .selectFrom("Whitelist") .where("email", "=", emailDomain)