fix: 2.8 oidc accounts unable to sign in after upgrading (#1381)

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<boolean> {
+  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);
 

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", () => {

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);
+  });
 });

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<typeof vi.fn>,
+}));
+
+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,
+    });
+  });
+});

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<unknown>]> = [
@@ -69,6 +70,7 @@ const MIGRATIONS: Array<[string, () => Promise<unknown>]> = [
   ["runNotificationChannelMigration", runNotificationChannelMigration],
   ["runTermixIdentityCaMigration", runTermixIdentityCaMigration],
   ["runSsoSettingsMigration", runSsoSettingsMigration],
+  ["runSsoLegacyIdentityMigration", runSsoLegacyIdentityMigration],
   ["runTelemetrySettingsMigration", runTelemetrySettingsMigration],
   // Last: it reads what the moves above wrote.
   ["runHostDefaultsMigration", runHostDefaultsMigration],

src/backend/upgrade/external-identity-migration.ts

@@ -79,7 +79,15 @@ export async function runExternalIdentityMigration(): Promise<ExternalIdentityMi
             parsed.providerId,
             parsed.subject,
           );
-          if (existing) {
+          // sso-legacy-identity-migration.ts moves "legacy-oidc" rows to
+          // their provider; recreating one would undo that on every boot.
+          const moved =
+            !existing &&
+            parsed.providerId === LEGACY_OIDC_PROVIDER_ID &&
+            (await auth.listIdentitiesForUser(user.id)).some(
+              (link) => link.subject === parsed.subject,
+            );
+          if (existing || moved) {
             result.skipped++;
           } else {
             await auth.linkIdentity({

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<boolean> {
+  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<ProviderRow[]> {
+  try {
+    return await selectRows<ProviderRow>(
+      sql`SELECT id, type, enabled, legacy_callback FROM ${sql.identifier(table)}`,
+    );
+  } catch {
+    return await selectRows<ProviderRow>(
+      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<number | null> {
+  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<SsoLegacyIdentityMigrationResult> {
+  const result: SsoLegacyIdentityMigrationResult = { moved: 0, conflicts: 0 };
+  try {
+    const target = await findTargetProvider();
+    if (target === null) return result;
+    const providerId = String(target);
+
+    // "github:null:<id>" 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;
+}