From c2cea8cffc26b930bb5625872b7285f5390a5585 Mon Sep 17 00:00:00 2001 From: Carlos Monastyrski Date: Fri, 8 Aug 2025 18:20:47 -0300 Subject: [PATCH] Fix SAML duplicate accounts when signing in the first time on an existing account --- ...174003_add-user-alias-is-email-verified.ts | 37 +++++++++++++++++++ backend/src/db/schemas/user-aliases.ts | 3 +- .../saml-config/saml-config-service.ts | 29 +++++++-------- backend/src/server/routes/index.ts | 3 +- backend/src/server/routes/v2/user-router.ts | 5 ++- backend/src/services/user/user-service.ts | 26 +++++++++++-- 6 files changed, 81 insertions(+), 22 deletions(-) create mode 100644 backend/src/db/migrations/20250808174003_add-user-alias-is-email-verified.ts diff --git a/backend/src/db/migrations/20250808174003_add-user-alias-is-email-verified.ts b/backend/src/db/migrations/20250808174003_add-user-alias-is-email-verified.ts new file mode 100644 index 000000000..62322afb8 --- /dev/null +++ b/backend/src/db/migrations/20250808174003_add-user-alias-is-email-verified.ts @@ -0,0 +1,37 @@ +import { Knex } from "knex"; + +import { TableName } from "../schemas"; + +const BATCH_SIZE = 1000; + +export async function up(knex: Knex): Promise { + if (!(await knex.schema.hasColumn(TableName.UserAliases, "isEmailVerified"))) { + // Add the column + await knex.schema.alterTable(TableName.UserAliases, (t) => { + t.boolean("isEmailVerified").defaultTo(false); + }); + + const aliasesToUpdate: { aliasId: string; isEmailVerified: boolean }[] = await knex(TableName.UserAliases) + .join(TableName.Users, `${TableName.UserAliases}.userId`, `${TableName.Users}.id`) + .select([`${TableName.UserAliases}.id as aliasId`, `${TableName.Users}.isEmailVerified`]); + + for (let i = 0; i < aliasesToUpdate.length; i += BATCH_SIZE) { + const batch = aliasesToUpdate.slice(i, i + BATCH_SIZE); + + const trueIds = batch.filter((row) => row.isEmailVerified).map((row) => row.aliasId); + + if (trueIds.length > 0) { + // eslint-disable-next-line no-await-in-loop + await knex(TableName.UserAliases).whereIn("id", trueIds).update({ isEmailVerified: true }); + } + } + } +} + +export async function down(knex: Knex): Promise { + if (await knex.schema.hasColumn(TableName.UserAliases, "isEmailVerified")) { + await knex.schema.alterTable(TableName.UserAliases, (t) => { + t.dropColumn("isEmailVerified"); + }); + } +} diff --git a/backend/src/db/schemas/user-aliases.ts b/backend/src/db/schemas/user-aliases.ts index 14147abf8..428fa62ca 100644 --- a/backend/src/db/schemas/user-aliases.ts +++ b/backend/src/db/schemas/user-aliases.ts @@ -16,7 +16,8 @@ export const UserAliasesSchema = z.object({ emails: z.string().array().nullable().optional(), orgId: z.string().uuid().nullable().optional(), createdAt: z.date(), - updatedAt: z.date() + updatedAt: z.date(), + isEmailVerified: z.boolean().default(false).nullable().optional() }); export type TUserAliases = z.infer; diff --git a/backend/src/ee/services/saml-config/saml-config-service.ts b/backend/src/ee/services/saml-config/saml-config-service.ts index cbb99f7eb..270462165 100644 --- a/backend/src/ee/services/saml-config/saml-config-service.ts +++ b/backend/src/ee/services/saml-config/saml-config-service.ts @@ -246,7 +246,7 @@ export const samlConfigServiceFactory = ({ }); } - const userAlias = await userAliasDAL.findOne({ + let userAlias = await userAliasDAL.findOne({ externalId, orgId, aliasType: UserAliasType.SAML @@ -320,15 +320,13 @@ export const samlConfigServiceFactory = ({ user = await userDAL.transaction(async (tx) => { let newUser: TUsers | undefined; - if (serverCfg.trustSamlEmails) { - newUser = await userDAL.findOne( - { - email, - isEmailVerified: true - }, - tx - ); - } + newUser = await userDAL.findOne( + { + email, + isEmailVerified: true + }, + tx + ); if (!newUser) { const uniqueUsername = await normalizeUsername(`${firstName ?? ""}-${lastName ?? ""}`, userDAL); @@ -346,13 +344,14 @@ export const samlConfigServiceFactory = ({ ); } - await userAliasDAL.create( + userAlias = await userAliasDAL.create( { userId: newUser.id, aliasType: UserAliasType.SAML, externalId, emails: email ? [email] : [], - orgId + orgId, + isEmailVerified: serverCfg.trustSamlEmails }, tx ); @@ -410,13 +409,13 @@ export const samlConfigServiceFactory = ({ } await licenseService.updateSubscriptionOrgMemberCount(organization.id); - const isUserCompleted = Boolean(user.isAccepted && user.isEmailVerified); + const isUserCompleted = Boolean(user.isAccepted && user.isEmailVerified && userAlias.isEmailVerified); const providerAuthToken = crypto.jwt().sign( { authTokenType: AuthTokenType.PROVIDER_TOKEN, userId: user.id, username: user.username, - ...(user.email && { email: user.email, isEmailVerified: user.isEmailVerified }), + ...(user.email && { email: user.email, isEmailVerified: userAlias.isEmailVerified }), firstName, lastName, organizationName: organization.name, @@ -440,7 +439,7 @@ export const samlConfigServiceFactory = ({ await samlConfigDAL.update({ orgId }, { lastUsed: new Date() }); - if (user.email && !user.isEmailVerified) { + if (user.email && !userAlias.isEmailVerified) { const token = await tokenService.createTokenForUser({ type: TokenType.TOKEN_EMAIL_VERIFICATION, userId: user.id diff --git a/backend/src/server/routes/index.ts b/backend/src/server/routes/index.ts index 90e961c29..82911ff53 100644 --- a/backend/src/server/routes/index.ts +++ b/backend/src/server/routes/index.ts @@ -727,7 +727,8 @@ export const registerRoutes = async ( permissionService, groupProjectDAL, smtpService, - projectMembershipDAL + projectMembershipDAL, + userAliasDAL }); const totpService = totpServiceFactory({ diff --git a/backend/src/server/routes/v2/user-router.ts b/backend/src/server/routes/v2/user-router.ts index 92b4138f2..f4b02e7be 100644 --- a/backend/src/server/routes/v2/user-router.ts +++ b/backend/src/server/routes/v2/user-router.ts @@ -17,6 +17,9 @@ export const registerUserRouter = async (server: FastifyZodProvider) => { }) }, schema: { + headers: z.object({ + referer: z.string().trim() + }), body: z.object({ username: z.string().trim() }), @@ -25,7 +28,7 @@ export const registerUserRouter = async (server: FastifyZodProvider) => { } }, handler: async (req) => { - await server.services.user.sendEmailVerificationCode(req.body.username); + await server.services.user.sendEmailVerificationCode(req.body.username, req.headers.referer); return {}; } }); diff --git a/backend/src/services/user/user-service.ts b/backend/src/services/user/user-service.ts index 30bf750c3..55c9de185 100644 --- a/backend/src/services/user/user-service.ts +++ b/backend/src/services/user/user-service.ts @@ -2,6 +2,7 @@ import { ForbiddenError } from "@casl/ability"; import { OrgPermissionActions, OrgPermissionSubjects } from "@app/ee/services/permission/org-permission"; import { TPermissionServiceFactory } from "@app/ee/services/permission/permission-service-types"; +import { crypto } from "@app/lib/crypto"; import { BadRequestError, ForbiddenRequestError, NotFoundError } from "@app/lib/errors"; import { logger } from "@app/lib/logger"; import { TAuthTokenServiceFactory } from "@app/services/auth-token/auth-token-service"; @@ -12,6 +13,7 @@ import { SmtpTemplates, TSmtpService } from "@app/services/smtp/smtp-service"; import { AuthMethod } from "../auth/auth-type"; import { TGroupProjectDALFactory } from "../group-project/group-project-dal"; import { TProjectMembershipDALFactory } from "../project-membership/project-membership-dal"; +import { TUserAliasDALFactory } from "../user-alias/user-alias-dal"; import { TUserDALFactory } from "./user-dal"; import { TListUserGroupsDTO, TUpdateUserMfaDTO } from "./user-types"; @@ -37,6 +39,7 @@ type TUserServiceFactoryDep = { projectMembershipDAL: Pick; smtpService: Pick; permissionService: TPermissionServiceFactory; + userAliasDAL: Pick; }; export type TUserServiceFactory = ReturnType; @@ -48,17 +51,28 @@ export const userServiceFactory = ({ groupProjectDAL, tokenService, smtpService, - permissionService + permissionService, + userAliasDAL }: TUserServiceFactoryDep) => { - const sendEmailVerificationCode = async (username: string) => { + const sendEmailVerificationCode = async (username: string, referer: string) => { // akhilmhdh: case sensitive email resolution const users = await userDAL.findUserByUsername(username); const user = users?.length > 1 ? users.find((el) => el.username === username) : users?.[0]; if (!user) throw new NotFoundError({ name: `User with username '${username}' not found` }); + let { isEmailVerified } = user; + const url = new URL(referer); + const refererToken = url.searchParams.get("token"); + if (!refererToken) + throw new BadRequestError({ name: "Failed to send email verification code due to no token on referer" }); + const { authType } = crypto.jwt().decode(refererToken) as { authType: string }; + const userAlias = await userAliasDAL.findOne({ userId: user.id, aliasType: authType }); + if (userAlias) { + isEmailVerified = userAlias.isEmailVerified; + } if (!user.email) throw new BadRequestError({ name: "Failed to send email verification code due to no email on user" }); - if (user.isEmailVerified) + if (isEmailVerified) throw new BadRequestError({ name: "Failed to send email verification code due to email already verified" }); const token = await tokenService.createTokenForUser({ @@ -95,7 +109,9 @@ export const userServiceFactory = ({ if (!user) throw new NotFoundError({ name: `User with username '${username}' not found` }); if (!user.email) throw new BadRequestError({ name: "Failed to verify email verification code due to no email on user" }); - if (user.isEmailVerified) + + const userAliases = await userAliasDAL.find({ userId: user.id }); + if (user.isEmailVerified && userAliases?.every((alias) => alias.isEmailVerified)) throw new BadRequestError({ name: "Failed to verify email verification code due to email already verified" }); await tokenService.validateTokenForUser({ @@ -106,6 +122,8 @@ export const userServiceFactory = ({ const userEmails = user?.email ? await userDAL.find({ email: user.email }) : []; + await userAliasDAL.update({ userId: user.id }, { isEmailVerified: true }); + await userDAL.updateById(user.id, { isEmailVerified: true, username: userEmails?.length === 1 && userEmails?.[0]?.id === user.id ? user.email.toLowerCase() : undefined