diff --git a/app/client/modules/auth/routes/__tests__/login.test.tsx b/app/client/modules/auth/routes/__tests__/login.test.tsx index 88bf7faa..f8a6e7aa 100644 --- a/app/client/modules/auth/routes/__tests__/login.test.tsx +++ b/app/client/modules/auth/routes/__tests__/login.test.tsx @@ -69,7 +69,7 @@ describe("LoginPage", () => { expect( await screen.findByText( - "Your account exists but is not linked to this SSO provider. Sign in with username/password first, then enable auto linking in your provider settings or contact your administrator.", + "SSO sign-in was blocked because this email already belongs to another user in this instance. Contact your administrator to resolve the account conflict.", ), ).toBeTruthy(); }); diff --git a/app/lib/sso-errors.ts b/app/lib/sso-errors.ts index a50c8115..df3f8a48 100644 --- a/app/lib/sso-errors.ts +++ b/app/lib/sso-errors.ts @@ -11,7 +11,7 @@ export type LoginErrorCode = (typeof LOGIN_ERROR_CODES)[number]; export function getLoginErrorDescription(errorCode: LoginErrorCode): string { switch (errorCode) { case "ACCOUNT_LINK_REQUIRED": - return "Your account exists but is not linked to this SSO provider. Sign in with username/password first, then enable auto linking in your provider settings or contact your administrator."; + return "SSO sign-in was blocked because this email already belongs to another user in this instance. Contact your administrator to resolve the account conflict."; case "EMAIL_NOT_VERIFIED": return "Your identity provider did not mark your email as verified."; case "INVITE_REQUIRED": diff --git a/app/server/lib/auth.ts b/app/server/lib/auth.ts index a8df5ed9..66c7451e 100644 --- a/app/server/lib/auth.ts +++ b/app/server/lib/auth.ts @@ -5,6 +5,7 @@ import { type MiddlewareContext, type MiddlewareOptions, } from "better-auth"; +import { APIError } from "better-auth/api"; import { drizzleAdapter } from "better-auth/adapters/drizzle"; import { admin, twoFactor, username, organization, testUtils } from "better-auth/plugins"; import { createAuthMiddleware } from "better-auth/api"; @@ -58,6 +59,20 @@ export const auth = betterAuth({ provider: "sqlite", }), databaseHooks: { + account: { + create: { + before: async (account, ctx) => { + if (ssoIntegration.isSsoCallback(ctx)) { + const allowed = await ssoIntegration.canLinkSsoAccount(account.userId, account.providerId); + if (!allowed) { + throw new APIError("FORBIDDEN", { + message: "SSO account linking is not permitted for users outside this organization", + }); + } + } + }, + }, + }, user: { delete: { before: async (user) => { diff --git a/app/server/modules/sso/__tests__/sso.errors.test.ts b/app/server/modules/sso/__tests__/sso.errors.test.ts new file mode 100644 index 00000000..4f3d2223 --- /dev/null +++ b/app/server/modules/sso/__tests__/sso.errors.test.ts @@ -0,0 +1,32 @@ +import { describe, expect, test } from "bun:test"; +import { mapAuthErrorToCode } from "../sso.errors"; + +describe("mapAuthErrorToCode", () => { + test("maps account-not-linked errors to ACCOUNT_LINK_REQUIRED", () => { + expect(mapAuthErrorToCode(encodeURIComponent("account not linked"))).toBe("ACCOUNT_LINK_REQUIRED"); + }); + + test("maps unable-to-link-account errors to ACCOUNT_LINK_REQUIRED", () => { + expect(mapAuthErrorToCode(encodeURIComponent("unable to link account"))).toBe("ACCOUNT_LINK_REQUIRED"); + }); + + test("maps security account-linking denial to ACCOUNT_LINK_REQUIRED", () => { + expect( + mapAuthErrorToCode( + encodeURIComponent("SSO account linking is not permitted for users outside this organization"), + ), + ).toBe("ACCOUNT_LINK_REQUIRED"); + }); + + test("maps invite-required errors to INVITE_REQUIRED", () => { + expect( + mapAuthErrorToCode( + encodeURIComponent("Access denied. You must be invited to this organization before you can sign in with SSO."), + ), + ).toBe("INVITE_REQUIRED"); + }); + + test("maps unknown errors to SSO_LOGIN_FAILED", () => { + expect(mapAuthErrorToCode(encodeURIComponent("some random error"))).toBe("SSO_LOGIN_FAILED"); + }); +}); diff --git a/app/server/modules/sso/__tests__/sso.integration.test.ts b/app/server/modules/sso/__tests__/sso.integration.test.ts index 142c0284..a471a97c 100644 --- a/app/server/modules/sso/__tests__/sso.integration.test.ts +++ b/app/server/modules/sso/__tests__/sso.integration.test.ts @@ -38,6 +38,22 @@ async function createUser(email: string, username: string) { return userId; } +async function createUserWithCredentialAccount(email: string, username: string) { + const userId = await createUser(email, username); + + await db.insert(account).values({ + id: randomId(), + accountId: username, + providerId: "credential", + userId, + password: "test-password-hash", + createdAt: new Date(), + updatedAt: new Date(), + }); + + return userId; +} + describe("ssoIntegration.resolveOrgMembership", () => { beforeEach(async () => { await db.delete(member); @@ -329,3 +345,141 @@ describe("ssoIntegration.resolveOrgMembership", () => { expect(updatedInvitation.status).toBe("accepted"); }); }); + +describe("ssoIntegration.canLinkSsoAccount", () => { + beforeEach(async () => { + await db.delete(member); + await db.delete(account); + await db.delete(invitation); + await db.delete(ssoProvider); + await db.delete(organization); + await db.delete(usersTable); + }); + + async function setupOrgWithProvider(providerId: string, autoLinkMatchingEmails = false) { + const orgId = randomId(); + const ownerId = await createUser(`owner-${randomSlug("u")}@example.com`, randomSlug("owner")); + await db.insert(organization).values({ id: orgId, name: "Acme", slug: randomSlug("acme"), createdAt: new Date() }); + await db.insert(ssoProvider).values({ + id: randomId(), + providerId, + organizationId: orgId, + userId: ownerId, + issuer: "https://issuer.example.com", + domain: "example.com", + autoLinkMatchingEmails, + }); + return { orgId, ownerId }; + } + + const autoLinkingStates = [false, true] as const; + + test("allows linking for a user who is already a member of the org", async () => { + const { orgId } = await setupOrgWithProvider("oidc-acme"); + const userId = await createUserWithCredentialAccount("alice@example.com", randomSlug("alice")); + await db.insert(member).values({ + id: randomId(), + userId, + organizationId: orgId, + role: "member", + createdAt: new Date(), + }); + + const allowed = await ssoIntegration.canLinkSsoAccount(userId, "oidc-acme"); + + expect(allowed).toBe(true); + }); + + for (const autoLinkingEnabled of autoLinkingStates) { + test(`allows linking for a new SSO user with a valid pending invitation (auto-linking ${autoLinkingEnabled ? "enabled" : "disabled"})`, async () => { + const providerId = `oidc-acme-${autoLinkingEnabled ? "on" : "off"}`; + const { orgId, ownerId } = await setupOrgWithProvider(providerId, autoLinkingEnabled); + const userId = await createUser("alice@example.com", randomSlug("alice")); + await db.insert(invitation).values({ + id: randomId(), + organizationId: orgId, + email: "alice@example.com", + role: "member", + status: "pending", + expiresAt: new Date(Date.now() + 60 * 60 * 1000), + createdAt: new Date(), + inviterId: ownerId, + }); + + const allowed = await ssoIntegration.canLinkSsoAccount(userId, providerId); + + expect(allowed).toBe(true); + }); + + test(`blocks linking for an existing user account even with a pending invitation (auto-linking ${autoLinkingEnabled ? "enabled" : "disabled"})`, async () => { + const providerId = `oidc-acme-${autoLinkingEnabled ? "on" : "off"}`; + const { orgId, ownerId } = await setupOrgWithProvider(providerId, autoLinkingEnabled); + const userId = await createUserWithCredentialAccount("alice@example.com", randomSlug("alice")); + await db.insert(invitation).values({ + id: randomId(), + organizationId: orgId, + email: "alice@example.com", + role: "member", + status: "pending", + expiresAt: new Date(Date.now() + 60 * 60 * 1000), + createdAt: new Date(), + inviterId: ownerId, + }); + + const allowed = await ssoIntegration.canLinkSsoAccount(userId, providerId); + + expect(allowed).toBe(false); + }); + } + + test("blocks linking for a user with no membership and no invitation in the org", async () => { + await setupOrgWithProvider("oidc-acme"); + const aliceId = await createUserWithCredentialAccount("alice@example.com", randomSlug("alice")); + + const allowed = await ssoIntegration.canLinkSsoAccount(aliceId, "oidc-acme"); + + expect(allowed).toBe(false); + }); + + test("blocks linking when the user only has an invitation to a different org", async () => { + const { ownerId } = await setupOrgWithProvider("oidc-acme"); + const otherOrgId = randomId(); + await db + .insert(organization) + .values({ id: otherOrgId, name: "Other", slug: randomSlug("other"), createdAt: new Date() }); + const aliceId = await createUserWithCredentialAccount("alice@example.com", randomSlug("alice")); + await db.insert(invitation).values({ + id: randomId(), + organizationId: otherOrgId, + email: "alice@example.com", + role: "member", + status: "pending", + expiresAt: new Date(Date.now() + 60 * 60 * 1000), + createdAt: new Date(), + inviterId: ownerId, + }); + + const allowed = await ssoIntegration.canLinkSsoAccount(aliceId, "oidc-acme"); + + expect(allowed).toBe(false); + }); + + test("blocks linking when the user's invitation to the org has expired", async () => { + const { orgId, ownerId } = await setupOrgWithProvider("oidc-acme"); + const aliceId = await createUserWithCredentialAccount("alice@example.com", randomSlug("alice")); + await db.insert(invitation).values({ + id: randomId(), + organizationId: orgId, + email: "alice@example.com", + role: "member", + status: "pending", + expiresAt: new Date(Date.now() - 60 * 60 * 1000), // expired + createdAt: new Date(), + inviterId: ownerId, + }); + + const allowed = await ssoIntegration.canLinkSsoAccount(aliceId, "oidc-acme"); + + expect(allowed).toBe(false); + }); +}); diff --git a/app/server/modules/sso/middlewares/__tests__/trust-provider-for-linking.test.ts b/app/server/modules/sso/middlewares/__tests__/trust-provider-for-linking.test.ts index 7d124e64..ae9e7d34 100644 --- a/app/server/modules/sso/middlewares/__tests__/trust-provider-for-linking.test.ts +++ b/app/server/modules/sso/middlewares/__tests__/trust-provider-for-linking.test.ts @@ -76,17 +76,25 @@ describe("resolveTrustedProvidersForRequest", () => { expect(await resolveTrustedProvidersForRequest(createRequest("/sso/callback/missing-provider"))).toEqual([]); }); - test("returns auto-link-enabled providers from the callback provider organization", async () => { - const { organizationId, userId } = await createSsoProviderRecord("pocket-id", true); + test.each([ + { callbackAutoLinking: true, sameOrgAutoLinking: true, expected: ["acme-saml", "pocket-id"] }, + { callbackAutoLinking: false, sameOrgAutoLinking: true, expected: ["acme-saml"] }, + { callbackAutoLinking: true, sameOrgAutoLinking: false, expected: ["pocket-id"] }, + { callbackAutoLinking: false, sameOrgAutoLinking: false, expected: [] }, + ])( + "returns trusted providers matrix (callback: $callbackAutoLinking, same-org: $sameOrgAutoLinking)", + async ({ callbackAutoLinking, sameOrgAutoLinking, expected }) => { + const { organizationId, userId } = await createSsoProviderRecord("pocket-id", callbackAutoLinking); - await createSsoProviderRecord("acme-saml", true, { organizationId, userId }); - await createSsoProviderRecord("acme-disabled", false, { organizationId, userId }); - await createSsoProviderRecord("other-org-provider", true); + await createSsoProviderRecord("acme-saml", sameOrgAutoLinking, { organizationId, userId }); + await createSsoProviderRecord("acme-disabled", false, { organizationId, userId }); + await createSsoProviderRecord("other-org-provider", true); - const trustedProviders = await resolveTrustedProvidersForRequest(createRequest("/sso/callback/pocket-id")); + const trustedProviders = await resolveTrustedProvidersForRequest(createRequest("/sso/callback/pocket-id")); - expect([...trustedProviders].sort()).toEqual(["acme-saml", "pocket-id"]); - }); + expect([...trustedProviders].sort()).toEqual([...expected]); + }, + ); test("supports /sso/saml2/callback/:providerId paths", async () => { await createSsoProviderRecord("saml-provider", true); @@ -112,7 +120,7 @@ describe("resolveTrustedProvidersForRequest", () => { ]); }); - test("removes providers from the result when auto-linking is disabled", async () => { + test("updates trusted providers immediately when callback provider auto-linking is toggled", async () => { await createSsoProviderRecord("pocket-id", true); expect(await resolveTrustedProvidersForRequest(createRequest("/sso/callback/pocket-id"))).toEqual(["pocket-id"]); diff --git a/app/server/modules/sso/sso.errors.ts b/app/server/modules/sso/sso.errors.ts index 42434449..1fbb1c9f 100644 --- a/app/server/modules/sso/sso.errors.ts +++ b/app/server/modules/sso/sso.errors.ts @@ -6,6 +6,12 @@ const INVITE_REQUIRED_ERRORS = new Set([ "unable to create session", ]); +const ACCOUNT_LINK_REQUIRED_ERRORS = new Set([ + "account not linked", + "unable to link account", + "SSO account linking is not permitted for users outside this organization", +]); + export function mapAuthErrorToCode(error: string): LoginErrorCode { let decoded: string; @@ -15,7 +21,7 @@ export function mapAuthErrorToCode(error: string): LoginErrorCode { return "SSO_LOGIN_FAILED"; } - if (decoded === "account not linked") { + if (ACCOUNT_LINK_REQUIRED_ERRORS.has(decoded)) { return "ACCOUNT_LINK_REQUIRED"; } diff --git a/app/server/modules/sso/sso.integration.ts b/app/server/modules/sso/sso.integration.ts index 4e644d30..01932147 100644 --- a/app/server/modules/sso/sso.integration.ts +++ b/app/server/modules/sso/sso.integration.ts @@ -47,7 +47,7 @@ async function resolveOrgMembership(userId: string, ctx: GenericEndpointContext throw new APIError("FORBIDDEN", { message: "SSO sign-in is invite-only for this organization" }); } - await db.transaction(async (tx) => { + db.transaction((tx) => { tx.insert(member) .values({ id: Bun.randomUUIDv7(), @@ -82,6 +82,39 @@ async function onUserCreate( user.hasDownloadedResticPassword = true; } +async function canLinkSsoAccount(userId: string, providerId: string): Promise { + const ssoProviderRecord = await ssoService.getSsoProviderById(providerId); + if (!ssoProviderRecord) { + return false; + } + + const existingMembership = await findMembershipWithOrganization(userId, ssoProviderRecord.organizationId); + if (existingMembership) { + return true; + } + + const existingAccount = await db.query.account.findFirst({ + where: { userId }, + columns: { id: true }, + }); + + if (existingAccount) { + return false; + } + + const user = await db.query.usersTable.findFirst({ where: { id: userId } }); + if (!user) { + return false; + } + + const pendingInvitation = await ssoService.getPendingInvitation( + ssoProviderRecord.organizationId, + normalizeEmail(user.email), + ); + + return !!pendingInvitation; +} + async function resolveOrgMembershipOrThrow(userId: string, ctx: GenericEndpointContext | null) { const membership = await resolveOrgMembership(userId, ctx); if (!membership) { @@ -122,5 +155,7 @@ export const ssoIntegration = { resolveOrgMembership, + canLinkSsoAccount, + resolveTrustedProviders: resolveTrustedProvidersForRequest, }; diff --git a/e2e/0003-oidc.spec.ts b/e2e/0003-oidc.spec.ts index a06d37b7..14629e8e 100644 --- a/e2e/0003-oidc.spec.ts +++ b/e2e/0003-oidc.spec.ts @@ -23,7 +23,7 @@ const autoLinkUninvitedLocalUsername = "sso-link-guard"; const inviteOnlyMessage = "Access is invite-only. Ask an organization admin to send you an invitation before signing in with SSO."; const accountLinkRequiredMessage = - "Your account exists but is not linked to this SSO provider. Sign in with username/password first, then enable auto linking in your provider settings or contact your administrator."; + "SSO sign-in was blocked because this email already belongs to another user in this instance. Contact your administrator to resolve the account conflict."; type OrgMembersResponse = { members: { @@ -226,18 +226,27 @@ async function withOidcLoginAttempt( } } +function isLoginPath(url: string): boolean { + const pathname = new URL(url).pathname; + return pathname === "/login" || pathname === "/login/error"; +} + +function isSsoCallbackPath(url: string): boolean { + return new URL(url).pathname.startsWith("/api/auth/sso/callback/"); +} + async function expectInviteOnlyLoginError(page: Page) { await expect .poll( () => { const url = page.url(); - return /\/login(\/error)?/.test(url) || /\/api\/auth\/sso\/callback\//.test(url); + return isLoginPath(url) || isSsoCallbackPath(url); }, { timeout: 30000 }, ) .toBe(true); - if (/\/login(\/error)?/.test(page.url())) { + if (isLoginPath(page.url())) { await waitForAppReady(page); await expect(page.getByText(inviteOnlyMessage)).toBeVisible(); return; @@ -251,19 +260,23 @@ async function expectAccountLinkRequiredLoginError(page: Page) { .poll( () => { const url = page.url(); - return /\/login(\/error)?/.test(url) || /\/api\/auth\/sso\/callback\//.test(url); + return isLoginPath(url) || isSsoCallbackPath(url); }, { timeout: 30000 }, ) .toBe(true); - if (/\/login(\/error)?/.test(page.url())) { + if (isLoginPath(page.url())) { await waitForAppReady(page); await expect(page.getByText(accountLinkRequiredMessage)).toBeVisible(); return; } - await expect(page.getByText(/account exists but is not linked/i)).toBeVisible(); + await expect( + page.getByText( + /(account not linked|unable to link account|already belongs to another user|outside this organization)/i, + ), + ).toBeVisible(); } test("uninvited OIDC users are blocked", async ({ page, browser }) => { @@ -321,7 +334,7 @@ test("auto-link policy enforces invitation and controls account linking", async await setProviderAutoLinking(page, providerIds.autoLinkNoInvite, true); await withOidcLoginAttempt(browser, providerIds.autoLinkNoInvite, autoLinkUninvitedLocalEmail, async (ssoPage) => { - await expectInviteOnlyLoginError(ssoPage); + await expectAccountLinkRequiredLoginError(ssoPage); }); await registerOidcProvider(page, providerIds.autoLink); @@ -336,8 +349,6 @@ test("auto-link policy enforces invitation and controls account linking", async await setProviderAutoLinking(page, providerIds.autoLink, true); await withOidcLoginAttempt(browser, providerIds.autoLink, autoLinkTargetEmail, async (ssoPage) => { - await ssoPage.waitForURL(/\/volumes/, { timeout: 30000 }); - await waitForAppReady(ssoPage); - await expect(ssoPage).toHaveURL(/\/volumes/); + await expectAccountLinkRequiredLoginError(ssoPage); }); });