diff --git a/src/backend/database/routes/user-external-account-routes.ts b/src/backend/database/routes/user-external-account-routes.ts index 40e6c17f8..b1b77f89d 100644 --- a/src/backend/database/routes/user-external-account-routes.ts +++ b/src/backend/database/routes/user-external-account-routes.ts @@ -20,6 +20,30 @@ function isNonEmptyString(val: unknown): val is string { return typeof val === "string" && val.trim().length > 0; } +/** + * 2.9.0 could not match a 2.8 OIDC account to its provider and made a second + * account on first sign-in. The original keeps only its "legacy-oidc" link + * with the same subject, so the duplicate may be merged back into it even + * though it has no password. + */ +async function isLegacyDuplicate( + externalUserId: string, + targetLinks: Array<{ providerId: string; subject: string }>, +): Promise { + if (targetLinks.length === 0) return false; + const sourceSubjects = new Set( + ( + await createCurrentUserAuthRepository().listIdentitiesForUser( + externalUserId, + ) + ).map((link) => link.subject), + ); + return targetLinks.every( + (link) => + link.providerId === "legacy-oidc" && sourceSubjects.has(link.subject), + ); +} + /** * Merging an account that only signs in externally (SSO, LDAP or any login * plugin) into a password account, and taking the external sign-in away @@ -34,7 +58,7 @@ export function registerUserExternalAccountRoutes( * /users/link-external-to-password: * post: * summary: Merge an external account into a password account - * description: Moves an external-only account's sign-in identities onto a password account and deletes the external-only account (admin only). The 2.8 path /users/link-oidc-to-password and its oidcUserId field are accepted too. + * description: Moves an external-only account's sign-in identities onto a password account and deletes the external-only account (admin only). A 2.8 SSO account whose only sign-in is the matching legacy OIDC link is accepted as the target too, to merge a duplicate made by 2.9.0. The 2.8 path /users/link-oidc-to-password and its oidcUserId field are accepted too. * tags: * - Users * requestBody: @@ -98,15 +122,18 @@ export function registerUserExternalAccountRoutes( .status(404) .json({ error: "Target password user not found" }); } - if (isExternalAccount(targetUser) || !targetUser.passwordHash) { - return res.status(400).json({ - error: "Target user must be a password-based account", - }); - } - if ((await identities.listIdentitiesForUser(targetUser.id)).length > 0) { - return res.status(400).json({ - error: "Target user already has an external sign-in", - }); + const targetLinks = await identities.listIdentitiesForUser(targetUser.id); + if (!(await isLegacyDuplicate(externalUserId, targetLinks))) { + if (isExternalAccount(targetUser) || !targetUser.passwordHash) { + return res.status(400).json({ + error: "Target user must be a password-based account", + }); + } + if (targetLinks.length > 0) { + return res.status(400).json({ + error: "Target user already has an external sign-in", + }); + } } authLogger.info("Linking an external account to a password account", { @@ -118,7 +145,8 @@ export function registerUserExternalAccountRoutes( await userRepository.update(targetUser.id, { isOidc: true, - oidcIdentifier: externalUser.oidcIdentifier, + oidcIdentifier: + externalUser.oidcIdentifier ?? targetUser.oidcIdentifier, }); await identities.moveIdentities(externalUserId, targetUser.id); diff --git a/src/backend/tests/database/routes/user-external-account-routes.test.ts b/src/backend/tests/database/routes/user-external-account-routes.test.ts index 19f9fad77..7c62e979b 100644 --- a/src/backend/tests/database/routes/user-external-account-routes.test.ts +++ b/src/backend/tests/database/routes/user-external-account-routes.test.ts @@ -201,6 +201,65 @@ describe("linking an external account to a password account", () => { }); expect(response.status).toBe(400); }); + + it("merges a 2.9.0 duplicate back into its 2.8 SSO account", async () => { + h.users.set("orig", { + id: "orig", + username: "bob", + isAdmin: false, + isOidc: true, + passwordHash: "", + oidcIdentifier: "sub-9", + }); + h.users.set("dup", { + id: "dup", + username: "bob-1", + isAdmin: false, + isOidc: true, + passwordHash: "", + oidcIdentifier: null, + }); + h.identities.push( + { userId: "orig", providerId: "legacy-oidc", subject: "sub-9" }, + { userId: "dup", providerId: "1", subject: "sub-9" }, + ); + + const response = await post("/link-external-to-password", { + externalUserId: "dup", + targetUsername: "bob", + }); + + expect(response.status).toBe(200); + expect(h.deleted).toEqual(["dup"]); + expect(h.users.get("orig")!.oidcIdentifier).toBe("sub-9"); + expect( + h.identities + .filter((row) => row.userId === "orig") + .map((row) => row.providerId), + ).toEqual(["legacy-oidc", "1"]); + }); + + it("does not treat an unrelated SSO account as a duplicate target", async () => { + h.users.set("other", { + id: "other", + username: "carol", + isAdmin: false, + isOidc: true, + passwordHash: "", + oidcIdentifier: "sub-x", + }); + h.identities.push({ + userId: "other", + providerId: "legacy-oidc", + subject: "sub-x", + }); + + const response = await post("/link-external-to-password", { + externalUserId: "sso", + targetUsername: "carol", + }); + expect(response.status).toBe(400); + }); }); describe("unlinking", () => { diff --git a/src/backend/tests/upgrade/external-identity-migration.test.ts b/src/backend/tests/upgrade/external-identity-migration.test.ts index 96eca533a..a7acbed1d 100644 --- a/src/backend/tests/upgrade/external-identity-migration.test.ts +++ b/src/backend/tests/upgrade/external-identity-migration.test.ts @@ -111,4 +111,20 @@ describe("runExternalIdentityMigration", () => { expect(second).toEqual({ identities: 0, skipped: 1 }); expect(h.state.identities).toHaveLength(1); }); + + it("does not recreate a legacy-oidc link that was moved to its provider", async () => { + user("oidc-user", { oidcIdentifier: "sub-1" }); + h.state.identities.push({ + id: 1, + userId: "oidc-user", + providerId: "1", + subject: "sub-1", + email: null, + } as never); + + const result = await runExternalIdentityMigration(); + + expect(result).toEqual({ identities: 0, skipped: 1 }); + expect(h.state.identities).toHaveLength(1); + }); }); diff --git a/src/backend/tests/upgrade/sso-legacy-identity-migration.test.ts b/src/backend/tests/upgrade/sso-legacy-identity-migration.test.ts new file mode 100644 index 000000000..c71d6f6fb --- /dev/null +++ b/src/backend/tests/upgrade/sso-legacy-identity-migration.test.ts @@ -0,0 +1,200 @@ +/** + * 2.8 OIDC accounts with no provider id were filed under "legacy-oidc", but + * the sso plugin signs in through the migrated provider row (#1381). They move + * to that row when it is the only one from before 2.9 and the env provider is + * not in use; anything ambiguous is left alone. + */ + +import Database from "better-sqlite3"; +import { drizzle } from "drizzle-orm/better-sqlite3"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const h = vi.hoisted(() => ({ + db: null as unknown, + warn: null as unknown as ReturnType, +})); + +vi.mock("../../database/db/index.js", () => ({ getDb: () => h.db })); +vi.mock("../../utils/logger.js", () => { + const log = { info: vi.fn(), warn: vi.fn(), error: vi.fn() }; + h.warn = log.warn; + return { databaseLogger: log }; +}); + +const { runSsoLegacyIdentityMigration } = + await import("../../upgrade/sso-legacy-identity-migration.js"); + +const ENV = [ + "OIDC_CLIENT_ID", + "OIDC_CLIENT_SECRET", + "OIDC_ISSUER_URL", + "OIDC_AUTHORIZATION_URL", + "OIDC_TOKEN_URL", + "OIDC_ENV_OVERRIDE", +]; + +let sqlite: Database.Database; + +function setup(providers: Array<[number, string, number, number]>) { + sqlite = new Database(":memory:"); + sqlite.exec(` + CREATE TABLE users (id TEXT PRIMARY KEY, oidc_identifier TEXT); + CREATE TABLE p_sso_providers ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + name TEXT NOT NULL DEFAULT 'x', + type TEXT NOT NULL, + enabled INTEGER NOT NULL DEFAULT 1, + legacy_callback INTEGER NOT NULL DEFAULT 0 + ); + CREATE TABLE user_external_identities ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + user_id TEXT NOT NULL, + provider_id TEXT NOT NULL, + subject TEXT NOT NULL, + email TEXT + ); + `); + const insert = sqlite.prepare( + "INSERT INTO p_sso_providers (id, type, enabled, legacy_callback) VALUES (?, ?, ?, ?)", + ); + for (const row of providers) insert.run(...row); + h.db = drizzle(sqlite); +} + +function addUser(id: string, identifier: string, providerId: string) { + sqlite + .prepare("INSERT INTO users (id, oidc_identifier) VALUES (?, ?)") + .run(id, identifier); + const subject = identifier.replace(/^github:[^:]+:/, ""); + sqlite + .prepare( + "INSERT INTO user_external_identities (user_id, provider_id, subject) VALUES (?, ?, ?)", + ) + .run(id, providerId, subject); +} + +function links() { + return sqlite + .prepare( + "SELECT user_id, provider_id, subject FROM user_external_identities ORDER BY id", + ) + .all(); +} + +beforeEach(() => { + for (const name of ENV) delete process.env[name]; +}); + +afterEach(() => { + for (const name of ENV) delete process.env[name]; +}); + +describe("runSsoLegacyIdentityMigration", () => { + it("moves legacy OIDC sign-ins to the one migrated provider", async () => { + setup([ + [1, "oidc", 1, 1], + [2, "github", 1, 1], + ]); + addUser("alice", "sub-a", "legacy-oidc"); + addUser("gh", "github:null:42", "legacy-oidc"); + + const result = await runSsoLegacyIdentityMigration(); + + expect(result).toEqual({ moved: 1, conflicts: 0 }); + expect(links()).toEqual([ + { user_id: "alice", provider_id: "1", subject: "sub-a" }, + { user_id: "gh", provider_id: "legacy-oidc", subject: "42" }, + ]); + }); + + it("ignores OIDC providers added after the upgrade", async () => { + setup([ + [1, "oidc", 1, 1], + [5, "oidc", 1, 0], + ]); + addUser("alice", "sub-a", "legacy-oidc"); + + await runSsoLegacyIdentityMigration(); + + expect(links()).toEqual([ + { user_id: "alice", provider_id: "1", subject: "sub-a" }, + ]); + }); + + it("leaves them when two providers are from before 2.9", async () => { + setup([ + [1, "oidc", 1, 1], + [2, "oidc", 1, 1], + ]); + addUser("alice", "sub-a", "legacy-oidc"); + + expect(await runSsoLegacyIdentityMigration()).toEqual({ + moved: 0, + conflicts: 0, + }); + expect(links()[0]).toMatchObject({ provider_id: "legacy-oidc" }); + }); + + it("leaves them when the env provider signs in", async () => { + setup([[1, "oidc", 1, 1]]); + addUser("alice", "sub-a", "legacy-oidc"); + for (const name of ENV.slice(0, 5)) process.env[name] = "x"; + process.env.OIDC_ENV_OVERRIDE = "true"; + + await runSsoLegacyIdentityMigration(); + + expect(links()[0]).toMatchObject({ provider_id: "legacy-oidc" }); + }); + + it("still moves them when env settings exist but the row is what signs in", async () => { + setup([[1, "oidc", 1, 1]]); + addUser("alice", "sub-a", "legacy-oidc"); + for (const name of ENV.slice(0, 5)) process.env[name] = "x"; + + await runSsoLegacyIdentityMigration(); + + expect(links()[0]).toMatchObject({ provider_id: "1" }); + }); + + it("skips a subject a 2.9.0 duplicate account already holds", async () => { + setup([[1, "oidc", 1, 1]]); + addUser("alice", "sub-a", "legacy-oidc"); + sqlite + .prepare( + "INSERT INTO user_external_identities (user_id, provider_id, subject) VALUES ('dup', '1', 'sub-a')", + ) + .run(); + + const result = await runSsoLegacyIdentityMigration(); + + expect(result).toEqual({ moved: 0, conflicts: 1 }); + expect(h.warn).toHaveBeenCalled(); + expect(links()).toHaveLength(2); + }); + + it("drops the legacy link once the same account holds the real one", async () => { + setup([[1, "oidc", 1, 1]]); + addUser("alice", "sub-a", "legacy-oidc"); + sqlite + .prepare( + "INSERT INTO user_external_identities (user_id, provider_id, subject) VALUES ('alice', '1', 'sub-a')", + ) + .run(); + + await runSsoLegacyIdentityMigration(); + + expect(links()).toEqual([ + { user_id: "alice", provider_id: "1", subject: "sub-a" }, + ]); + }); + + it("is idempotent", async () => { + setup([[1, "oidc", 1, 1]]); + addUser("alice", "sub-a", "legacy-oidc"); + await runSsoLegacyIdentityMigration(); + expect(await runSsoLegacyIdentityMigration()).toEqual({ + moved: 0, + conflicts: 0, + }); + }); +}); diff --git a/src/backend/upgrade/boot-migrations.ts b/src/backend/upgrade/boot-migrations.ts index cac8c7cd2..e517fab03 100644 --- a/src/backend/upgrade/boot-migrations.ts +++ b/src/backend/upgrade/boot-migrations.ts @@ -37,6 +37,7 @@ import { runTotpMigration } from "./totp-migration.js"; import { runNotificationChannelMigration } from "./notification-channel-migration.js"; import { runTermixIdentityCaMigration } from "./termix-identity-ca-migration.js"; import { runSsoSettingsMigration } from "./sso-settings-migration.js"; +import { runSsoLegacyIdentityMigration } from "./sso-legacy-identity-migration.js"; import { runHostDefaultsMigration } from "./host-defaults-migration.js"; const MIGRATIONS: Array<[string, () => Promise]> = [ @@ -69,6 +70,7 @@ const MIGRATIONS: Array<[string, () => Promise]> = [ ["runNotificationChannelMigration", runNotificationChannelMigration], ["runTermixIdentityCaMigration", runTermixIdentityCaMigration], ["runSsoSettingsMigration", runSsoSettingsMigration], + ["runSsoLegacyIdentityMigration", runSsoLegacyIdentityMigration], ["runTelemetrySettingsMigration", runTelemetrySettingsMigration], // Last: it reads what the moves above wrote. ["runHostDefaultsMigration", runHostDefaultsMigration], diff --git a/src/backend/upgrade/external-identity-migration.ts b/src/backend/upgrade/external-identity-migration.ts index 1a25e1c37..dae127155 100644 --- a/src/backend/upgrade/external-identity-migration.ts +++ b/src/backend/upgrade/external-identity-migration.ts @@ -79,7 +79,15 @@ export async function runExternalIdentityMigration(): Promise link.subject === parsed.subject, + ); + if (existing || moved) { result.skipped++; } else { await auth.linkIdentity({ diff --git a/src/backend/upgrade/sso-legacy-identity-migration.ts b/src/backend/upgrade/sso-legacy-identity-migration.ts new file mode 100644 index 000000000..93134d41f --- /dev/null +++ b/src/backend/upgrade/sso-legacy-identity-migration.ts @@ -0,0 +1,160 @@ +/** + * Points 2.8 OIDC sign-ins at the provider row they came from. + * + * external-identity-migration.ts files a bare OIDC subject with no + * users.sso_provider_id under "legacy-oidc", the id the sso plugin gives its + * env-configured provider. Those accounts came from the single 2.8 OIDC + * config, which 2.8 copied into an sso_providers row, and the plugin signs in + * through that row with its id. Without this they could not sign in at all + * (#1381). + * + * Only runs when the answer is clear: the env provider is not what signs in, + * and exactly one OIDC row is from before 2.9. An identity whose target is + * already taken (a duplicate account made by 2.9.0) is left for an admin. + * Idempotent. + */ + +import { sql } from "drizzle-orm"; +import { databaseLogger } from "../utils/logger.js"; +import { + runStatement, + selectRows, +} from "../utils/crypto-migration/raw-rows.js"; + +const LEGACY_OIDC_PROVIDER_ID = "legacy-oidc"; +const SOURCES = ["p_sso_providers", "sso_providers"]; + +export interface SsoLegacyIdentityMigrationResult { + moved: number; + conflicts: number; +} + +async function tableExists(name: string): Promise { + try { + await selectRows(sql`SELECT 1 FROM ${sql.identifier(name)} WHERE 1 = 0`); + return true; + } catch { + return false; + } +} + +function isOn(value: unknown): boolean { + return value === true || value === 1 || value === "1" || value === "t"; +} + +function envProviderConfigured(): boolean { + return [ + "OIDC_CLIENT_ID", + "OIDC_CLIENT_SECRET", + "OIDC_ISSUER_URL", + "OIDC_AUTHORIZATION_URL", + "OIDC_TOKEN_URL", + ].every((name) => !!process.env[name]); +} + +interface ProviderRow { + id: number; + type: string; + enabled: unknown; + legacy_callback?: unknown; +} + +async function readProviders(table: string): Promise { + try { + return await selectRows( + sql`SELECT id, type, enabled, legacy_callback FROM ${sql.identifier(table)}`, + ); + } catch { + return await selectRows( + sql`SELECT id, type, enabled FROM ${sql.identifier(table)}`, + ); + } +} + +/** The row "legacy-oidc" sign-ins belong to, or null when that is unclear. */ +async function findTargetProvider(): Promise { + for (const table of SOURCES) { + if (!(await tableExists(table))) continue; + const rows = await readProviders(table); + + // The env provider signs in as "legacy-oidc" when it overrides the rows + // or no row is enabled, so those identities are already right. + const enabled = rows.filter((row) => isOn(row.enabled)); + if ( + envProviderConfigured() && + (process.env.OIDC_ENV_OVERRIDE?.toLowerCase() === "true" || + enabled.length === 0) + ) { + return null; + } + + const oidc = rows.filter((row) => row.type === "oidc"); + const legacy = oidc.filter( + (row) => row.legacy_callback === undefined || isOn(row.legacy_callback), + ); + return legacy.length === 1 ? Number(legacy[0].id) : null; + } + return null; +} + +export async function runSsoLegacyIdentityMigration(): Promise { + const result: SsoLegacyIdentityMigrationResult = { moved: 0, conflicts: 0 }; + try { + const target = await findTargetProvider(); + if (target === null) return result; + const providerId = String(target); + + // "github:null:" also landed on "legacy-oidc", but its identifier + // kept the prefix, so matching the bare column picks only OIDC ones. + const identities = await selectRows<{ + id: number; + user_id: string; + subject: string; + }>( + sql`SELECT i.id, i.user_id, i.subject FROM user_external_identities i JOIN users u ON u.id = i.user_id WHERE i.provider_id = ${LEGACY_OIDC_PROVIDER_ID} AND u.oidc_identifier = i.subject`, + ); + + for (const identity of identities) { + const [taken] = await selectRows<{ user_id: string }>( + sql`SELECT user_id FROM user_external_identities WHERE provider_id = ${providerId} AND subject = ${identity.subject}`, + ); + if (taken) { + if (taken.user_id !== identity.user_id) { + result.conflicts++; + databaseLogger.warn( + "An SSO subject is linked to two accounts; merge or delete the duplicate", + { + operation: "sso_legacy_identity_conflict", + userId: identity.user_id, + duplicateUserId: taken.user_id, + providerId, + }, + ); + continue; + } + await runStatement( + sql`DELETE FROM user_external_identities WHERE id = ${identity.id}`, + ); + result.moved++; + continue; + } + await runStatement( + sql`UPDATE user_external_identities SET provider_id = ${providerId} WHERE id = ${identity.id}`, + ); + result.moved++; + } + + if (result.moved > 0) { + databaseLogger.info("Moved 2.8 OIDC sign-ins to their provider", { + operation: "sso_legacy_identity_migration", + providerId, + ...result, + }); + } + } catch (error) { + databaseLogger.error("SSO legacy identity migration failed", error, { + operation: "sso_legacy_identity_migration_failed", + }); + } + return result; +}