From 4f15d87cbebeec954a76b91ffa0206312941767b Mon Sep 17 00:00:00 2001 From: Maurice Date: Fri, 28 Aug 2026 19:37:22 +0200 Subject: [PATCH] fix(oidc): drop the old provider's avatar and identity when the IdP changes Pointing the instance at a different OIDC provider left two things from the old one on every account. The avatar kept a URL on a host this instance no longer talks to, so it rendered as a broken image and stayed that way: the sync only ran when the new provider sent a picture claim, and having no picture for a user is a perfectly normal answer. The account also stayed pinned to the old provider. Login still worked, because the verified-email lookup finds the row, but the relink sat behind a check for a missing sub, so oidc_sub and oidc_issuer never moved. The SSO note in settings went on naming a provider that was gone. An uploaded avatar is a bare filename rather than a URL and is left alone on both paths, exactly as before. Fixes #2110 --- server/src/nest/oidc/oidc.service.ts | 18 +++++++- server/tests/unit/nest/oidc.service.test.ts | 48 +++++++++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/server/src/nest/oidc/oidc.service.ts b/server/src/nest/oidc/oidc.service.ts index 8263f0f25..94d934997 100644 --- a/server/src/nest/oidc/oidc.service.ts +++ b/server/src/nest/oidc/oidc.service.ts @@ -525,6 +525,14 @@ export class OidcService implements OnModuleDestroy { return { error: 'email_not_verified' }; } this.db.prepare('UPDATE users SET oidc_sub = ?, oidc_issuer = ? WHERE id = ?').run(sub, config.issuer, user.id); + user = { ...user, oidc_sub: sub, oidc_issuer: config.issuer } as User; + } else if (user.oidc_issuer !== config.issuer || user.oidc_sub !== sub) { + // The admin pointed the instance at a different IdP. We got here through the + // verified-email lookup, so this is the same person arriving from the new + // provider; leaving the old sub and issuer on the row would keep the account + // pinned to a provider that no longer exists (#2110). + this.db.prepare('UPDATE users SET oidc_sub = ?, oidc_issuer = ? WHERE id = ?').run(sub, config.issuer, user.id); + user = { ...user, oidc_sub: sub, oidc_issuer: config.issuer } as User; } // Update role based on OIDC claims on every login (if claim mapping is configured) if (readEnv().oidc.adminValue) { @@ -547,10 +555,16 @@ export class OidcService implements OnModuleDestroy { } } // Keep the avatar in sync with the OIDC picture, but never clobber a custom - // upload: only fill it when empty or when the current value is itself an OIDC + // upload: only touch it when empty or when the current value is itself an OIDC // picture URL, so the picture refreshes on each login without overriding an // uploaded one. #1399 - if (picture && picture !== user.avatar && (!user.avatar || /^https:\/\//i.test(user.avatar))) { + // + // "In sync" includes the provider having no picture for this user any more. That + // is what a provider switch looks like from here, and the old value points at a + // host this instance no longer talks to, so it renders as a broken image forever + // (#2110). An uploaded avatar is a bare filename and stays untouched either way. + const avatarIsOidc = !!user.avatar && /^https:\/\//i.test(user.avatar); + if (picture ? picture !== user.avatar && (!user.avatar || avatarIsOidc) : avatarIsOidc) { this.db.prepare('UPDATE users SET avatar = ? WHERE id = ?').run(picture, user.id); user = { ...user, avatar: picture } as User; } diff --git a/server/tests/unit/nest/oidc.service.test.ts b/server/tests/unit/nest/oidc.service.test.ts index 30f8dc056..189189391 100644 --- a/server/tests/unit/nest/oidc.service.test.ts +++ b/server/tests/unit/nest/oidc.service.test.ts @@ -678,6 +678,54 @@ describe('findOrCreateUser', () => { expect(row.avatar).toBe('https://idp.example.com/u/new.png'); }); + // #2110 — the admin repoints the instance at a different IdP. The user still logs in, + // because the verified-email lookup finds the row, but everything the old provider put + // on it used to survive: an avatar URL on a host this instance no longer talks to, and + // a sub/issuer pair pinning the account to a provider that is gone. + it('OIDC-SVC-060: a provider without a picture claim clears the previous provider avatar', () => { + const { user } = createUser(testDb, { email: 'switch1@example.com' }); + testDb.prepare('UPDATE users SET oidc_sub = ?, oidc_issuer = ?, avatar = ? WHERE id = ?') + .run('sub-old-1', 'https://old-idp.example.com', 'https://old-idp.example.com/u/me.png', user.id); + + svc.findOrCreateUser( + { sub: 'sub-new-1', email: 'switch1@example.com', name: 'Switcher', email_verified: true }, + { ...MOCK_CONFIG, issuer: 'https://new-idp.example.com' }, + ); + + const row = testDb.prepare('SELECT avatar FROM users WHERE id = ?').get(user.id) as any; + expect(row.avatar).toBeNull(); + }); + + it('OIDC-SVC-061: an uploaded avatar survives the same switch', () => { + const { user } = createUser(testDb, { email: 'switch2@example.com' }); + // A local upload is a bare filename, not a URL, and belongs to the user. + testDb.prepare('UPDATE users SET oidc_sub = ?, oidc_issuer = ?, avatar = ? WHERE id = ?') + .run('sub-old-2', 'https://old-idp.example.com', 'uploaded-abc.jpg', user.id); + + svc.findOrCreateUser( + { sub: 'sub-new-2', email: 'switch2@example.com', name: 'Switcher', email_verified: true }, + { ...MOCK_CONFIG, issuer: 'https://new-idp.example.com' }, + ); + + const row = testDb.prepare('SELECT avatar FROM users WHERE id = ?').get(user.id) as any; + expect(row.avatar).toBe('uploaded-abc.jpg'); + }); + + it('OIDC-SVC-062: the account is relinked to the new provider sub and issuer', () => { + const { user } = createUser(testDb, { email: 'switch3@example.com' }); + testDb.prepare('UPDATE users SET oidc_sub = ?, oidc_issuer = ? WHERE id = ?') + .run('sub-old-3', 'https://old-idp.example.com', user.id); + + svc.findOrCreateUser( + { sub: 'sub-new-3', email: 'switch3@example.com', name: 'Switcher', email_verified: true }, + { ...MOCK_CONFIG, issuer: 'https://new-idp.example.com' }, + ); + + const row = testDb.prepare('SELECT oidc_sub, oidc_issuer FROM users WHERE id = ?').get(user.id) as any; + expect(row.oidc_sub).toBe('sub-new-3'); + expect(row.oidc_issuer).toBe('https://new-idp.example.com'); + }); + it('OIDC-SVC-053: returns no_email when the email claim is missing (no throw)', () => { const result = svc.findOrCreateUser({ sub: 'sub-no-email', name: 'No Email' }, MOCK_CONFIG); expect('error' in result).toBe(true);