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