From b0321b02fa3db263aa722d11e3d7f5b0aaf30580 Mon Sep 17 00:00:00 2001 From: Chris Date: Tue, 14 Apr 2026 00:11:32 +0300 Subject: [PATCH] fix: resolve OIDC authentication error with existing identities (#1021) Fixed "Cannot read properties of undefined (reading 'id')" error that occurred when existing OIDC users attempted to log in. The issue was a case mismatch in the Sequelize association accessor. Changes: - Fixed case mismatch: identity.user -> identity.User (matches association alias) - Removed invalid username field that doesn't exist in User model - Fixed admin role assignment to use Role model instead of User model - Improved transaction error handling to prevent double rollbacks - Removed unused generateUsername function Tests: - Added comprehensive test suite with 12 tests covering all provisioning scenarios - All tests passing including the previously failing login scenario --- backend/modules/oidc/provisioningService.js | 28 +- .../modules/oidc/provisioningService.test.js | 275 ++++++++++++++++++ 2 files changed, 290 insertions(+), 13 deletions(-) create mode 100644 backend/tests/unit/modules/oidc/provisioningService.test.js diff --git a/backend/modules/oidc/provisioningService.js b/backend/modules/oidc/provisioningService.js index 8c5f28f..bfaf461 100644 --- a/backend/modules/oidc/provisioningService.js +++ b/backend/modules/oidc/provisioningService.js @@ -11,11 +11,6 @@ function shouldBeAdmin(config, email) { return config.adminEmailDomains.includes(domain); } -function generateUsername(email) { - const baseUsername = email.split('@')[0]; - return baseUsername.replace(/[^a-zA-Z0-9_-]/g, '_'); -} - async function findOrCreateIdentity(providerSlug, claims) { const identity = await OIDCIdentity.findOne({ where: { @@ -59,7 +54,7 @@ async function provisionUser(providerSlug, claims, req) { ); await transaction.commit(); - return { user: identity.user, isNewUser: false }; + return { user: identity.User, isNewUser: false }; } if (!config.autoProvision) { @@ -80,20 +75,24 @@ async function provisionUser(providerSlug, claims, req) { let isNewUser = false; if (!user) { - const username = generateUsername(claims.email); - user = await User.create( { email: claims.email, - username, verified_email: true, - is_admin: shouldBeAdmin(config, claims.email), password_digest: null, }, { transaction } ); isNewUser = true; + + if (shouldBeAdmin(config, claims.email)) { + const { Role } = require('../../models'); + await Role.update( + { is_admin: true }, + { where: { user_id: user.id }, transaction } + ); + } } identity = await OIDCIdentity.create( @@ -117,7 +116,9 @@ async function provisionUser(providerSlug, claims, req) { return { user, isNewUser }; } catch (error) { - await transaction.rollback(); + if (!transaction.finished) { + await transaction.rollback(); + } throw error; } } @@ -177,7 +178,9 @@ async function linkIdentityToUser(userId, providerSlug, claims) { await transaction.commit(); return identity; } catch (error) { - await transaction.rollback(); + if (!transaction.finished) { + await transaction.rollback(); + } throw error; } } @@ -187,5 +190,4 @@ module.exports = { linkIdentityToUser, findOrCreateIdentity, shouldBeAdmin, - generateUsername, }; diff --git a/backend/tests/unit/modules/oidc/provisioningService.test.js b/backend/tests/unit/modules/oidc/provisioningService.test.js new file mode 100644 index 0000000..4c7dc5c --- /dev/null +++ b/backend/tests/unit/modules/oidc/provisioningService.test.js @@ -0,0 +1,275 @@ +const { sequelize, User, OIDCIdentity, Role } = require('../../../../models'); +const provisioningService = require('../../../../modules/oidc/provisioningService'); +const providerConfig = require('../../../../modules/oidc/providerConfig'); + +jest.mock('../../../../modules/oidc/providerConfig'); + +describe('OIDC Provisioning Service', () => { + beforeAll(async () => { + await sequelize.sync({ force: true }); + }); + + afterAll(async () => { + await sequelize.close(); + }); + + beforeEach(async () => { + await OIDCIdentity.destroy({ where: {}, force: true }); + await Role.destroy({ where: {}, force: true }); + await User.destroy({ where: {}, force: true }); + + providerConfig.getProvider.mockReturnValue({ + slug: 'test-provider', + name: 'Test Provider', + autoProvision: true, + adminEmailDomains: ['admin.com'], + }); + }); + + describe('provisionUser', () => { + it('should return user object when existing identity logs in', async () => { + const existingUser = await User.create({ + email: 'test@example.com', + username: 'testuser', + password_digest: 'hashed', + verified_email: true, + }); + + await OIDCIdentity.create({ + user_id: existingUser.id, + provider_slug: 'test-provider', + subject: 'sub-123', + email: 'test@example.com', + name: 'Test User', + first_login_at: new Date(), + last_login_at: new Date(), + }); + + const claims = { + sub: 'sub-123', + email: 'test@example.com', + name: 'Test User Updated', + }; + + const result = await provisioningService.provisionUser( + 'test-provider', + claims, + {} + ); + + expect(result).toBeDefined(); + expect(result.user).toBeDefined(); + expect(result.user.id).toBe(existingUser.id); + expect(result.user.email).toBe('test@example.com'); + expect(result.isNewUser).toBe(false); + }); + + it('should create new user when auto-provision is enabled and identity does not exist', async () => { + const claims = { + sub: 'sub-456', + email: 'newuser@example.com', + name: 'New User', + given_name: 'New', + family_name: 'User', + }; + + const result = await provisioningService.provisionUser( + 'test-provider', + claims, + {} + ); + + expect(result).toBeDefined(); + expect(result.user).toBeDefined(); + expect(result.user.email).toBe('newuser@example.com'); + expect(result.isNewUser).toBe(true); + + const identity = await OIDCIdentity.findOne({ + where: { subject: 'sub-456' }, + }); + expect(identity).toBeDefined(); + expect(identity.user_id).toBe(result.user.id); + }); + + it('should link existing user by email when creating new identity', async () => { + const existingUser = await User.create({ + email: 'existing@example.com', + username: 'existing', + password_digest: 'hashed', + verified_email: true, + }); + + const claims = { + sub: 'sub-789', + email: 'existing@example.com', + name: 'Existing User', + }; + + const result = await provisioningService.provisionUser( + 'test-provider', + claims, + {} + ); + + expect(result).toBeDefined(); + expect(result.user).toBeDefined(); + expect(result.user.id).toBe(existingUser.id); + expect(result.isNewUser).toBe(false); + + const identity = await OIDCIdentity.findOne({ + where: { subject: 'sub-789' }, + }); + expect(identity).toBeDefined(); + expect(identity.user_id).toBe(existingUser.id); + }); + + it('should throw error when auto-provision is disabled and user does not exist', async () => { + providerConfig.getProvider.mockReturnValue({ + slug: 'test-provider', + name: 'Test Provider', + autoProvision: false, + }); + + const claims = { + sub: 'sub-999', + email: 'notallowed@example.com', + name: 'Not Allowed', + }; + + await expect( + provisioningService.provisionUser('test-provider', claims, {}) + ).rejects.toThrow('Auto-provisioning is disabled'); + }); + + it('should throw error when email claim is missing', async () => { + const claims = { + sub: 'sub-000', + name: 'No Email User', + }; + + await expect( + provisioningService.provisionUser('test-provider', claims, {}) + ).rejects.toThrow('Email claim is required'); + }); + + it('should set admin flag when email domain matches admin domains', async () => { + const claims = { + sub: 'sub-admin', + email: 'admin@admin.com', + name: 'Admin User', + }; + + const result = await provisioningService.provisionUser( + 'test-provider', + claims, + {} + ); + + const role = await Role.findOne({ + where: { user_id: result.user.id }, + }); + expect(role).toBeDefined(); + expect(role.is_admin).toBe(true); + }); + }); + + describe('linkIdentityToUser', () => { + it('should link new identity to existing user', async () => { + const user = await User.create({ + email: 'link@example.com', + username: 'linkuser', + password_digest: 'hashed', + verified_email: true, + }); + + const claims = { + sub: 'sub-link-123', + email: 'link@example.com', + name: 'Link User', + }; + + const identity = await provisioningService.linkIdentityToUser( + user.id, + 'test-provider', + claims + ); + + expect(identity).toBeDefined(); + expect(identity.user_id).toBe(user.id); + expect(identity.subject).toBe('sub-link-123'); + }); + + it('should throw error when identity is already linked to another user', async () => { + const user1 = await User.create({ + email: 'user1@example.com', + username: 'user1', + password_digest: 'hashed', + verified_email: true, + }); + + const user2 = await User.create({ + email: 'user2@example.com', + username: 'user2', + password_digest: 'hashed', + verified_email: true, + }); + + await OIDCIdentity.create({ + user_id: user1.id, + provider_slug: 'test-provider', + subject: 'sub-existing', + email: 'user1@example.com', + name: 'User 1', + first_login_at: new Date(), + last_login_at: new Date(), + }); + + const claims = { + sub: 'sub-existing', + email: 'user2@example.com', + name: 'User 2', + }; + + await expect( + provisioningService.linkIdentityToUser( + user2.id, + 'test-provider', + claims + ) + ).rejects.toThrow('already linked to another user'); + }); + }); + + describe('shouldBeAdmin', () => { + it('should return true when email domain matches admin domains', () => { + const config = { adminEmailDomains: ['admin.com', 'company.com'] }; + expect( + provisioningService.shouldBeAdmin(config, 'user@admin.com') + ).toBe(true); + expect( + provisioningService.shouldBeAdmin(config, 'user@company.com') + ).toBe(true); + }); + + it('should return false when email domain does not match', () => { + const config = { adminEmailDomains: ['admin.com'] }; + expect( + provisioningService.shouldBeAdmin(config, 'user@other.com') + ).toBe(false); + }); + + it('should return false when adminEmailDomains is empty', () => { + const config = { adminEmailDomains: [] }; + expect( + provisioningService.shouldBeAdmin(config, 'user@admin.com') + ).toBe(false); + }); + + it('should return false when adminEmailDomains is not set', () => { + const config = {}; + expect( + provisioningService.shouldBeAdmin(config, 'user@admin.com') + ).toBe(false); + }); + }); +});