diff --git a/src/auth/auth.service.spec.ts b/src/auth/auth.service.spec.ts index db6524f..27bbb0d 100644 --- a/src/auth/auth.service.spec.ts +++ b/src/auth/auth.service.spec.ts @@ -7,13 +7,9 @@ import { User } from '../users/users.entity'; describe('AuthService', () => { let service: AuthService; - const mockFindByKeycloakSub = jest.fn(); - const mockUpdateFromToken = jest.fn(); const mockCreateFromToken = jest.fn(); const mockUsersService = { - findByKeycloakSub: mockFindByKeycloakSub, - updateFromToken: mockUpdateFromToken, createFromToken: mockCreateFromToken, }; @@ -62,13 +58,11 @@ describe('AuthService', () => { describe('syncUserFromToken', () => { it('should create a new user if user does not exist', async () => { - mockFindByKeycloakSub.mockReturnValue(null); mockCreateFromToken.mockReturnValue(mockUser); const result = await service.syncUserFromToken(mockAuthUser); expect(result).toEqual(mockUser); - expect(mockFindByKeycloakSub).toHaveBeenCalledWith('f:realm:user123'); expect(mockCreateFromToken).toHaveBeenCalledWith({ keycloakSub: 'f:realm:user123', email: 'test@example.com', @@ -77,32 +71,23 @@ describe('AuthService', () => { picture: 'https://example.com/avatar.jpg', roles: ['user', 'premium'], }); - expect(mockUpdateFromToken).not.toHaveBeenCalled(); }); - it('should update existing user if user exists', async () => { + it('should handle existing user via upsert', async () => { const updatedUser = { ...mockUser, lastLoginAt: new Date('2024-02-01') }; - mockFindByKeycloakSub.mockReturnValue(mockUser); - mockUpdateFromToken.mockReturnValue(updatedUser); + mockCreateFromToken.mockReturnValue(updatedUser); const result = await service.syncUserFromToken(mockAuthUser); expect(result).toEqual(updatedUser); - expect(mockFindByKeycloakSub).toHaveBeenCalledWith('f:realm:user123'); - - expect(mockUpdateFromToken).toHaveBeenCalledWith( - 'f:realm:user123', - expect.objectContaining({ - email: 'test@example.com', - name: 'Test User', - username: 'testuser', - picture: 'https://example.com/avatar.jpg', - roles: ['user', 'premium'], - - lastLoginAt: expect.any(Date), - }), - ); - expect(mockCreateFromToken).not.toHaveBeenCalled(); + expect(mockCreateFromToken).toHaveBeenCalledWith({ + keycloakSub: 'f:realm:user123', + email: 'test@example.com', + name: 'Test User', + username: 'testuser', + picture: 'https://example.com/avatar.jpg', + roles: ['user', 'premium'], + }); }); it('should handle user with no email by using empty string', async () => { @@ -111,7 +96,6 @@ describe('AuthService', () => { name: 'No Email User', }; - mockFindByKeycloakSub.mockReturnValue(null); mockCreateFromToken.mockReturnValue({ ...mockUser, email: '', @@ -131,30 +115,28 @@ describe('AuthService', () => { it('should handle user with no name by using username or fallback', async () => { const authUserNoName: AuthenticatedUser = { keycloakSub: 'f:realm:user789', - username: 'someusername', + username: 'fallbackuser', }; - mockFindByKeycloakSub.mockReturnValue(null); mockCreateFromToken.mockReturnValue({ ...mockUser, - name: 'someusername', + name: 'fallbackuser', }); await service.syncUserFromToken(authUserNoName); expect(mockCreateFromToken).toHaveBeenCalledWith( expect.objectContaining({ - name: 'someusername', + name: 'fallbackuser', }), ); }); it('should use "Unknown User" when no name or username is available', async () => { const authUserMinimal: AuthenticatedUser = { - keycloakSub: 'f:realm:user000', + keycloakSub: 'f:realm:minimal', }; - mockFindByKeycloakSub.mockReturnValue(null); mockCreateFromToken.mockReturnValue({ ...mockUser, name: 'Unknown User', @@ -176,7 +158,6 @@ describe('AuthService', () => { name: 'Empty Sub User', }; - mockFindByKeycloakSub.mockReturnValue(null); mockCreateFromToken.mockReturnValue({ ...mockUser, keycloakSub: '', @@ -186,7 +167,6 @@ describe('AuthService', () => { const result = await service.syncUserFromToken(authUserEmptySub); - expect(mockFindByKeycloakSub).toHaveBeenCalledWith(''); expect(mockCreateFromToken).toHaveBeenCalledWith( expect.objectContaining({ keycloakSub: '', @@ -203,7 +183,6 @@ describe('AuthService', () => { name: 'Malformed User', }; - mockFindByKeycloakSub.mockReturnValue(null); mockCreateFromToken.mockReturnValue({ ...mockUser, keycloakSub: 'invalid-format', @@ -213,7 +192,6 @@ describe('AuthService', () => { const result = await service.syncUserFromToken(authUserMalformed); - expect(mockFindByKeycloakSub).toHaveBeenCalledWith('invalid-format'); expect(mockCreateFromToken).toHaveBeenCalledWith( expect.objectContaining({ keycloakSub: 'invalid-format', diff --git a/src/auth/auth.service.ts b/src/auth/auth.service.ts index ce692c8..6efcbfa 100644 --- a/src/auth/auth.service.ts +++ b/src/auth/auth.service.ts @@ -7,8 +7,23 @@ import { User } from '../users/users.entity'; * Authentication Service * * Handles authentication-related business logic including: - * - User synchronization from Keycloak tokens - * - Profile updates for authenticated users + * - User login tracking from Keycloak tokens + * - Profile synchronization from Keycloak + * - Role-based authorization checks + * + * ## User Sync Strategy + * + * On every authentication: + * - Creates new users with full profile data from Keycloak + * - For existing users, compares Keycloak data with local database: + * - If profile changed: Updates all fields (email, name, username, picture, roles, lastLoginAt) + * - If profile unchanged: Only updates lastLoginAt (lightweight operation) + * + * This optimizes database performance since reads are cheaper than writes in PostgreSQL. + * Most logins only update lastLoginAt, but profile changes sync automatically. + * + * For explicit profile sync (webhooks, admin operations), use: + * - UsersService.syncProfileFromToken() - force sync regardless of changes */ @Injectable() export class AuthService { @@ -17,42 +32,31 @@ export class AuthService { constructor(private readonly usersService: UsersService) {} /** - * Synchronizes a user from Keycloak token to local database. - * Creates a new user if they don't exist, or updates their last login time. + * Handles user login and profile synchronization from Keycloak token. + * Creates new users with full profile data. + * For existing users, intelligently syncs only changed fields to optimize performance. + * + * The service compares Keycloak data with local database and: + * - Updates profile fields only if they changed from Keycloak + * - Always updates lastLoginAt to track login activity * * @param authenticatedUser - User data extracted from JWT token - * @returns The synchronized user entity + * @returns The user entity */ async syncUserFromToken(authenticatedUser: AuthenticatedUser): Promise { const { keycloakSub, email, name, username, picture, roles } = authenticatedUser; - // Try to find existing user by Keycloak subject - let user = await this.usersService.findByKeycloakSub(keycloakSub); - - if (user) { - // User exists - update last login and sync profile data - this.logger.debug(`Syncing existing user: ${keycloakSub}`); - user = await this.usersService.updateFromToken(keycloakSub, { - email, - name, - username, - picture, - roles, - lastLoginAt: new Date(), - }); - } else { - // New user - create from token data - this.logger.log(`Creating new user from token: ${keycloakSub}`); - user = await this.usersService.createFromToken({ - keycloakSub, - email: email || '', - name: name || username || 'Unknown User', - username, - picture, - roles, - }); - } + // Use createFromToken which handles upsert atomically + // This prevents race conditions when multiple requests arrive simultaneously + const user = await this.usersService.createFromToken({ + keycloakSub, + email: email || '', + name: name || username || 'Unknown User', + username, + picture, + roles, + }); return user; } diff --git a/src/users/dto/update-user.dto.ts b/src/users/dto/update-user.dto.ts index 896a34d..943f0e2 100644 --- a/src/users/dto/update-user.dto.ts +++ b/src/users/dto/update-user.dto.ts @@ -1,25 +1,14 @@ -import { ApiProperty } from '@nestjs/swagger'; -import { IsOptional, IsString, MinLength, MaxLength } from 'class-validator'; - /** * DTO for updating user profile. - * Only allows updating safe, user-controlled fields. - * Security-sensitive fields (keycloakSub, roles, email, etc.) are managed by Keycloak. + * + * Currently all profile fields (name, email, username, picture, roles) are managed by Keycloak + * and cannot be updated locally. This DTO exists for future extensibility if local profile + * fields are added (e.g., preferences, settings, bio, etc.). + * + * To update profile data, users must update their profile in Keycloak. Changes will sync + * automatically on next login. */ export class UpdateUserDto { - /** - * User's display name - */ - @ApiProperty({ - description: "User's display name", - example: 'John Doe', - required: false, - minLength: 1, - maxLength: 100, - }) - @IsOptional() - @IsString() - @MinLength(1, { message: 'Name must not be empty' }) - @MaxLength(100, { message: 'Name must not exceed 100 characters' }) - name?: string; + // Currently no fields are updatable locally + // Reserved for future local profile extensions } diff --git a/src/users/users.service.spec.ts b/src/users/users.service.spec.ts index 85b4689..cca2121 100644 --- a/src/users/users.service.spec.ts +++ b/src/users/users.service.spec.ts @@ -1,9 +1,9 @@ import { Test, TestingModule } from '@nestjs/testing'; -import { NotFoundException, ForbiddenException } from '@nestjs/common'; import { UsersService } from './users.service'; import { PrismaService } from '../database/prisma.service'; -import type { UpdateUserDto } from './dto/update-user.dto'; -import type { User } from '@prisma/client'; +import { NotFoundException, ForbiddenException } from '@nestjs/common'; +import { User } from '@prisma/client'; +import { UpdateUserDto } from './dto/update-user.dto'; describe('UsersService', () => { let service: UsersService; @@ -12,14 +12,14 @@ describe('UsersService', () => { const mockUser: User = { id: '550e8400-e29b-41d4-a716-446655440000', keycloakSub: 'f:realm:user123', - email: 'john@example.com', - name: 'John Doe', - username: 'johndoe', + email: 'test@example.com', + name: 'Test User', + username: 'testuser', picture: 'https://example.com/avatar.jpg', roles: ['user', 'premium'], - createdAt: new Date('2024-01-15T10:30:00.000Z'), - updatedAt: new Date('2024-01-15T10:30:00.000Z'), lastLoginAt: new Date('2024-01-15T10:30:00.000Z'), + createdAt: new Date('2024-01-01T00:00:00.000Z'), + updatedAt: new Date('2024-01-15T10:30:00.000Z'), }; const mockPrismaService = { @@ -28,6 +28,7 @@ describe('UsersService', () => { findUnique: jest.fn(), update: jest.fn(), delete: jest.fn(), + upsert: jest.fn(), }, }; @@ -45,7 +46,6 @@ describe('UsersService', () => { service = module.get(UsersService); prismaService = module.get(PrismaService); - // Clear all mocks before each test jest.clearAllMocks(); }); @@ -54,9 +54,9 @@ describe('UsersService', () => { }); describe('createFromToken', () => { - it('should create a new user from Keycloak token data', async () => { + it('should create a new user when user does not exist', async () => { const tokenData = { - keycloakSub: 'f:realm:user123', + keycloakSub: 'f:realm:newuser', email: 'john@example.com', name: 'John Doe', username: 'johndoe', @@ -65,53 +65,146 @@ describe('UsersService', () => { }; mockPrismaService.user.findUnique.mockResolvedValue(null); - mockPrismaService.user.create.mockResolvedValue(mockUser); + mockPrismaService.user.upsert.mockResolvedValue({ + ...mockUser, + ...tokenData, + }); const user = await service.createFromToken(tokenData); expect(user).toBeDefined(); - expect(user.id).toBeDefined(); - expect(user.keycloakSub).toBe('f:realm:user123'); - expect(user.email).toBe('john@example.com'); - expect(user.name).toBe('John Doe'); - expect(user.username).toBe('johndoe'); - expect(user.picture).toBe('https://example.com/avatar.jpg'); - expect(user.roles).toEqual(['user', 'premium']); - expect(mockPrismaService.user.findUnique).toHaveBeenCalledWith({ where: { keycloakSub: tokenData.keycloakSub }, }); - expect(mockPrismaService.user.create).toHaveBeenCalledWith({ - data: expect.objectContaining({ + expect(mockPrismaService.user.upsert).toHaveBeenCalledWith({ + where: { keycloakSub: tokenData.keycloakSub }, + update: expect.objectContaining({ + email: tokenData.email, + name: tokenData.name, + username: tokenData.username, + picture: tokenData.picture, + roles: tokenData.roles, + lastLoginAt: expect.any(Date), + }), + create: expect.objectContaining({ keycloakSub: tokenData.keycloakSub, email: tokenData.email, name: tokenData.name, username: tokenData.username, picture: tokenData.picture, roles: tokenData.roles, + lastLoginAt: expect.any(Date), }), }); }); - it('should return existing user if keycloakSub already exists', async () => { + it('should update all fields when profile data changed', async () => { const tokenData = { keycloakSub: 'f:realm:user123', - email: 'john@example.com', - name: 'John Doe', + email: 'newemail@example.com', // Changed + name: 'New Name', // Changed + username: 'testuser', + picture: 'https://example.com/avatar.jpg', + roles: ['user', 'premium'], }; - mockPrismaService.user.findUnique.mockResolvedValue(mockUser); + const existingUser = { ...mockUser }; + const updatedUser = { + ...mockUser, + email: tokenData.email, + name: tokenData.name, + lastLoginAt: new Date(), + }; + + mockPrismaService.user.findUnique.mockResolvedValue(existingUser); + mockPrismaService.user.update.mockResolvedValue(updatedUser); const user = await service.createFromToken(tokenData); - expect(user).toEqual(mockUser); + expect(user).toEqual(updatedUser); expect(mockPrismaService.user.findUnique).toHaveBeenCalledWith({ where: { keycloakSub: tokenData.keycloakSub }, }); - expect(mockPrismaService.user.create).not.toHaveBeenCalled(); + expect(mockPrismaService.user.update).toHaveBeenCalledWith({ + where: { keycloakSub: tokenData.keycloakSub }, + data: { + email: tokenData.email, + name: tokenData.name, + username: tokenData.username, + picture: tokenData.picture, + roles: tokenData.roles, + lastLoginAt: expect.any(Date), + }, + }); }); - it('should handle optional fields', async () => { + it('should only update lastLoginAt when profile unchanged', async () => { + const tokenData = { + keycloakSub: 'f:realm:user123', + email: 'test@example.com', // Same + name: 'Test User', // Same + username: 'testuser', // Same + picture: 'https://example.com/avatar.jpg', // Same + roles: ['user', 'premium'], // Same + }; + + const existingUser = { ...mockUser }; + const updatedUser = { + ...mockUser, + lastLoginAt: new Date('2024-02-01T10:00:00.000Z'), + }; + + mockPrismaService.user.findUnique.mockResolvedValue(existingUser); + mockPrismaService.user.update.mockResolvedValue(updatedUser); + + const user = await service.createFromToken(tokenData); + + expect(user).toEqual(updatedUser); + expect(mockPrismaService.user.update).toHaveBeenCalledWith({ + where: { keycloakSub: tokenData.keycloakSub }, + data: { + lastLoginAt: expect.any(Date), + }, + }); + }); + + it('should detect role changes and update profile', async () => { + const tokenData = { + keycloakSub: 'f:realm:user123', + email: 'test@example.com', + name: 'Test User', + username: 'testuser', + picture: 'https://example.com/avatar.jpg', + roles: ['user', 'premium', 'admin'], // Changed: added admin + }; + + const existingUser = { ...mockUser }; + const updatedUser = { + ...mockUser, + roles: tokenData.roles, + lastLoginAt: new Date(), + }; + + mockPrismaService.user.findUnique.mockResolvedValue(existingUser); + mockPrismaService.user.update.mockResolvedValue(updatedUser); + + const user = await service.createFromToken(tokenData); + + expect(user).toEqual(updatedUser); + expect(mockPrismaService.user.update).toHaveBeenCalledWith({ + where: { keycloakSub: tokenData.keycloakSub }, + data: { + email: tokenData.email, + name: tokenData.name, + username: tokenData.username, + picture: tokenData.picture, + roles: tokenData.roles, + lastLoginAt: expect.any(Date), + }, + }); + }); + + it('should handle optional fields when creating new user', async () => { const tokenData = { keycloakSub: 'f:realm:user456', email: 'jane@example.com', @@ -120,13 +213,22 @@ describe('UsersService', () => { const newUser = { ...mockUser, username: null, picture: null, roles: [] }; mockPrismaService.user.findUnique.mockResolvedValue(null); - mockPrismaService.user.create.mockResolvedValue(newUser); + mockPrismaService.user.upsert.mockResolvedValue(newUser); const user = await service.createFromToken(tokenData); expect(user).toBeDefined(); - expect(mockPrismaService.user.create).toHaveBeenCalledWith({ - data: expect.objectContaining({ + expect(mockPrismaService.user.upsert).toHaveBeenCalledWith({ + where: { keycloakSub: tokenData.keycloakSub }, + update: expect.objectContaining({ + email: tokenData.email, + name: tokenData.name, + username: undefined, + picture: undefined, + roles: [], + lastLoginAt: expect.any(Date), + }), + create: expect.objectContaining({ keycloakSub: tokenData.keycloakSub, email: tokenData.email, name: tokenData.name, @@ -136,10 +238,89 @@ describe('UsersService', () => { }), }); }); + + it('should normalize empty roles array', async () => { + const tokenData = { + keycloakSub: 'f:realm:user123', + email: 'test@example.com', + name: 'Test User', + roles: undefined, + }; + + const existingUser = { ...mockUser, roles: ['user'] }; + const updatedUser = { ...mockUser, roles: [] }; + + mockPrismaService.user.findUnique.mockResolvedValue(existingUser); + mockPrismaService.user.update.mockResolvedValue(updatedUser); + + await service.createFromToken(tokenData); + + expect(mockPrismaService.user.update).toHaveBeenCalledWith({ + where: { keycloakSub: tokenData.keycloakSub }, + data: expect.objectContaining({ + roles: [], + }), + }); + }); + + it('should detect username change', async () => { + const tokenData = { + keycloakSub: 'f:realm:user123', + email: 'test@example.com', + name: 'Test User', + username: 'newusername', // Changed + picture: 'https://example.com/avatar.jpg', + roles: ['user', 'premium'], + }; + + const existingUser = { ...mockUser }; + const updatedUser = { ...mockUser, username: 'newusername' }; + + mockPrismaService.user.findUnique.mockResolvedValue(existingUser); + mockPrismaService.user.update.mockResolvedValue(updatedUser); + + await service.createFromToken(tokenData); + + expect(mockPrismaService.user.update).toHaveBeenCalledWith({ + where: { keycloakSub: tokenData.keycloakSub }, + data: expect.objectContaining({ + username: 'newusername', + }), + }); + }); + + it('should detect picture change', async () => { + const tokenData = { + keycloakSub: 'f:realm:user123', + email: 'test@example.com', + name: 'Test User', + username: 'testuser', + picture: 'https://example.com/new-avatar.jpg', // Changed + roles: ['user', 'premium'], + }; + + const existingUser = { ...mockUser }; + const updatedUser = { + ...mockUser, + picture: 'https://example.com/new-avatar.jpg', + }; + + mockPrismaService.user.findUnique.mockResolvedValue(existingUser); + mockPrismaService.user.update.mockResolvedValue(updatedUser); + + await service.createFromToken(tokenData); + + expect(mockPrismaService.user.update).toHaveBeenCalledWith({ + where: { keycloakSub: tokenData.keycloakSub }, + data: expect.objectContaining({ + picture: 'https://example.com/new-avatar.jpg', + }), + }); + }); }); describe('findByKeycloakSub', () => { - it('should return a user by keycloakSub', async () => { + it('should find a user by keycloakSub', async () => { mockPrismaService.user.findUnique.mockResolvedValue(mockUser); const user = await service.findByKeycloakSub('f:realm:user123'); @@ -156,14 +337,11 @@ describe('UsersService', () => { const user = await service.findByKeycloakSub('nonexistent'); expect(user).toBeNull(); - expect(mockPrismaService.user.findUnique).toHaveBeenCalledWith({ - where: { keycloakSub: 'nonexistent' }, - }); }); }); describe('findOne', () => { - it('should return a user by ID', async () => { + it('should find a user by ID', async () => { mockPrismaService.user.findUnique.mockResolvedValue(mockUser); const user = await service.findOne(mockUser.id); @@ -175,7 +353,7 @@ describe('UsersService', () => { }); it('should throw NotFoundException if user not found', async () => { - const nonexistentId = 'nonexistent-id'; + const nonexistentId = '550e8400-0000-0000-0000-000000000000'; mockPrismaService.user.findUnique.mockResolvedValue(null); await expect(service.findOne(nonexistentId)).rejects.toThrow( @@ -187,66 +365,12 @@ describe('UsersService', () => { }); }); - describe('updateFromToken', () => { - it('should update user from token data', async () => { - const updateData = { - email: 'updated@example.com', - name: 'Updated Name', - lastLoginAt: new Date('2024-01-20T14:45:00.000Z'), - }; - - const updatedUser = { ...mockUser, ...updateData }; - mockPrismaService.user.findUnique.mockResolvedValue(mockUser); - mockPrismaService.user.update.mockResolvedValue(updatedUser); - - const user = await service.updateFromToken('f:realm:user123', updateData); - - expect(user).toEqual(updatedUser); - expect(mockPrismaService.user.update).toHaveBeenCalledWith({ - where: { keycloakSub: 'f:realm:user123' }, - data: expect.objectContaining({ - email: updateData.email, - name: updateData.name, - lastLoginAt: updateData.lastLoginAt, - }), - }); - }); - - it('should throw NotFoundException if user not found', async () => { - mockPrismaService.user.findUnique.mockResolvedValue(null); - - await expect( - service.updateFromToken('nonexistent', { name: 'Test' }), - ).rejects.toThrow(NotFoundException); - }); - - it('should only update provided fields', async () => { - const updateData = { - name: 'New Name', - }; - - const updatedUser = { ...mockUser, ...updateData }; - mockPrismaService.user.findUnique.mockResolvedValue(mockUser); - mockPrismaService.user.update.mockResolvedValue(updatedUser); - - await service.updateFromToken('f:realm:user123', updateData); - - expect(mockPrismaService.user.update).toHaveBeenCalledWith({ - where: { keycloakSub: 'f:realm:user123' }, - data: { name: 'New Name' }, - }); - }); - }); - describe('update', () => { - it('should update user profile', async () => { - const updateDto: UpdateUserDto = { - name: 'Updated Name', - }; + it('should allow update but currently no fields are updatable', async () => { + const updateDto: UpdateUserDto = {}; - const updatedUser = { ...mockUser, name: updateDto.name }; mockPrismaService.user.findUnique.mockResolvedValue(mockUser); - mockPrismaService.user.update.mockResolvedValue(updatedUser); + mockPrismaService.user.update.mockResolvedValue(mockUser); const user = await service.update( mockUser.id, @@ -254,17 +378,15 @@ describe('UsersService', () => { mockUser.keycloakSub, ); - expect(user.name).toBe(updateDto.name); + expect(user).toEqual(mockUser); expect(mockPrismaService.user.update).toHaveBeenCalledWith({ where: { id: mockUser.id }, - data: { name: updateDto.name }, + data: {}, }); }); it('should throw ForbiddenException if user tries to update someone else', async () => { - const updateDto: UpdateUserDto = { - name: 'Hacker Name', - }; + const updateDto: UpdateUserDto = {}; mockPrismaService.user.findUnique.mockResolvedValue(mockUser); @@ -276,9 +398,7 @@ describe('UsersService', () => { }); it('should throw NotFoundException if user not found', async () => { - const updateDto: UpdateUserDto = { - name: 'Test', - }; + const updateDto: UpdateUserDto = {}; mockPrismaService.user.findUnique.mockResolvedValue(null); @@ -318,4 +438,153 @@ describe('UsersService', () => { ).rejects.toThrow(NotFoundException); }); }); + + describe('syncProfileFromToken', () => { + it('should sync profile data from Keycloak token for existing user', async () => { + const profileData = { + email: 'updated@example.com', + name: 'Updated Name', + username: 'updateduser', + picture: 'https://example.com/new-avatar.jpg', + roles: ['user', 'admin'], + }; + + const updatedUser = { ...mockUser, ...profileData }; + mockPrismaService.user.update.mockResolvedValue(updatedUser); + + const user = await service.syncProfileFromToken( + 'f:realm:user123', + profileData, + ); + + expect(user).toEqual(updatedUser); + expect(mockPrismaService.user.update).toHaveBeenCalledWith({ + where: { keycloakSub: 'f:realm:user123' }, + data: { + email: profileData.email, + name: profileData.name, + username: profileData.username, + picture: profileData.picture, + roles: profileData.roles, + }, + }); + }); + + it('should handle profile data with missing optional fields', async () => { + const profileData = { + email: 'minimal@example.com', + name: 'Minimal User', + }; + + const updatedUser = { + ...mockUser, + ...profileData, + username: null, + picture: null, + roles: [], + }; + mockPrismaService.user.update.mockResolvedValue(updatedUser); + + const user = await service.syncProfileFromToken( + 'f:realm:user123', + profileData, + ); + + expect(user).toBeDefined(); + expect(mockPrismaService.user.update).toHaveBeenCalledWith({ + where: { keycloakSub: 'f:realm:user123' }, + data: { + email: profileData.email, + name: profileData.name, + username: undefined, + picture: undefined, + roles: [], + }, + }); + }); + + it('should throw NotFoundException if user not found', async () => { + const profileData = { + email: 'test@example.com', + name: 'Test User', + }; + + mockPrismaService.user.update.mockRejectedValue({ + code: 'P2025', + message: 'Record not found', + }); + + await expect( + service.syncProfileFromToken('nonexistent', profileData), + ).rejects.toThrow(NotFoundException); + await expect( + service.syncProfileFromToken('nonexistent', profileData), + ).rejects.toThrow('User with keycloakSub nonexistent not found'); + }); + + it('should normalize empty roles array', async () => { + const profileData = { + email: 'test@example.com', + name: 'Test User', + roles: undefined, + }; + + const updatedUser = { ...mockUser, roles: [] }; + mockPrismaService.user.update.mockResolvedValue(updatedUser); + + await service.syncProfileFromToken('f:realm:user123', profileData); + + expect(mockPrismaService.user.update).toHaveBeenCalledWith({ + where: { keycloakSub: 'f:realm:user123' }, + data: expect.objectContaining({ + roles: [], + }), + }); + }); + + it('should overwrite all profile fields from Keycloak', async () => { + const profileData = { + email: 'keycloak@example.com', + name: 'Keycloak Name', + username: 'keycloakuser', + picture: 'https://keycloak.example.com/avatar.jpg', + roles: ['external-role'], + }; + + const updatedUser = { ...mockUser, ...profileData }; + mockPrismaService.user.update.mockResolvedValue(updatedUser); + + const user = await service.syncProfileFromToken( + 'f:realm:user123', + profileData, + ); + + expect(user.name).toBe('Keycloak Name'); + expect(user.email).toBe('keycloak@example.com'); + expect(mockPrismaService.user.update).toHaveBeenCalledWith({ + where: { keycloakSub: 'f:realm:user123' }, + data: { + email: profileData.email, + name: profileData.name, + username: profileData.username, + picture: profileData.picture, + roles: profileData.roles, + }, + }); + }); + + it('should rethrow non-P2025 errors', async () => { + const profileData = { + email: 'test@example.com', + name: 'Test User', + }; + + const dbError = new Error('Database connection failed'); + mockPrismaService.user.update.mockRejectedValue(dbError); + + await expect( + service.syncProfileFromToken('f:realm:user123', profileData), + ).rejects.toThrow('Database connection failed'); + }); + }); }); diff --git a/src/users/users.service.ts b/src/users/users.service.ts index d8384ce..2323403 100644 --- a/src/users/users.service.ts +++ b/src/users/users.service.ts @@ -7,6 +7,7 @@ import { import { PrismaService } from '../database/prisma.service'; import { User } from '@prisma/client'; import type { UpdateUserDto } from './dto/update-user.dto'; +import { PrismaClientKnownRequestError } from '@prisma/client/runtime/client'; /** * Interface for creating a user from Keycloak token @@ -20,24 +21,36 @@ export interface CreateUserFromTokenDto { roles?: string[]; } -/** - * Interface for updating a user from Keycloak token - */ -export interface UpdateUserFromTokenDto { - email?: string; - name?: string; - username?: string; - picture?: string; - roles?: string[]; - lastLoginAt?: Date; -} - /** * Users Service * * Manages user data synchronized from Keycloak OIDC using Prisma ORM. * Users are created automatically when they first authenticate via Keycloak. * Direct user creation is not allowed - users must authenticate via Keycloak first. + * + * ## Profile Data Ownership + * + * Keycloak is the single source of truth for authentication and profile data: + * - `keycloakSub`: Managed by Keycloak (immutable identifier) + * - `email`: Managed by Keycloak + * - `name`: Managed by Keycloak + * - `username`: Managed by Keycloak + * - `picture`: Managed by Keycloak + * - `roles`: Managed by Keycloak + * + * Local application data: + * - `lastLoginAt`: Tracked locally for analytics + * + * ## Sync Strategy + * + * - On every login: Compares Keycloak data with local data + * - If profile changed: Updates all fields (name, email, picture, roles) + * - If profile unchanged: Only updates `lastLoginAt` + * - Read operations are cheaper than writes in PostgreSQL + * - Explicit sync: Call `syncProfileFromToken()` for force sync (webhooks, manual refresh) + * + * This optimizes performance by avoiding unnecessary writes while keeping + * profile data in sync with Keycloak on every authentication. */ @Injectable() export class UsersService { @@ -46,38 +59,146 @@ export class UsersService { constructor(private readonly prisma: PrismaService) {} /** - * Creates a new user from Keycloak token data. + * Creates a new user or syncs/tracks login for existing users. * This method is called automatically during authentication flow. * + * For new users: Creates the user with full profile data from token. + * For existing users: Compares Keycloak data with local data and only updates if changed. + * This optimizes performance since reads are cheaper than writes in PostgreSQL. + * * @param createDto - User data extracted from Keycloak JWT token - * @returns The newly created user + * @returns The user entity */ async createFromToken(createDto: CreateUserFromTokenDto): Promise { - // Check if user already exists - const existingUser = await this.findByKeycloakSub(createDto.keycloakSub); + // Normalize roles once to avoid duplication + const roles = createDto.roles || []; + const now = new Date(); + + // Check if user exists first (read is cheaper than write) + const existingUser = await this.prisma.user.findUnique({ + where: { keycloakSub: createDto.keycloakSub }, + }); if (existingUser) { - this.logger.warn( - `Attempted to create duplicate user with keycloakSub: ${createDto.keycloakSub}`, - ); - return existingUser; + // Compare profile data to detect changes + const profileChanged = + existingUser.email !== createDto.email || + existingUser.name !== createDto.name || + existingUser.username !== createDto.username || + existingUser.picture !== createDto.picture || + JSON.stringify(existingUser.roles) !== JSON.stringify(roles); + + if (profileChanged) { + // Profile data changed - update everything + this.logger.debug( + `Profile changed for user: ${existingUser.id} (${createDto.keycloakSub})`, + ); + return await this.prisma.user.update({ + where: { keycloakSub: createDto.keycloakSub }, + data: { + email: createDto.email, + name: createDto.name, + username: createDto.username, + picture: createDto.picture, + roles, + lastLoginAt: now, + }, + }); + } else { + // Profile unchanged - only update lastLoginAt + this.logger.debug( + `Login tracked for user: ${existingUser.id} (${createDto.keycloakSub})`, + ); + return await this.prisma.user.update({ + where: { keycloakSub: createDto.keycloakSub }, + data: { + lastLoginAt: now, + }, + }); + } } - const newUser = await this.prisma.user.create({ - data: { + // New user - create with all profile data + // Use upsert to handle race condition if user was created between findUnique and here + this.logger.log(`Creating new user from token: ${createDto.keycloakSub}`); + const user = await this.prisma.user.upsert({ + where: { keycloakSub: createDto.keycloakSub }, + update: { + // If created by concurrent request, update with current data + email: createDto.email, + name: createDto.name, + username: createDto.username, + picture: createDto.picture, + roles, + lastLoginAt: now, + }, + create: { keycloakSub: createDto.keycloakSub, email: createDto.email, name: createDto.name, username: createDto.username, picture: createDto.picture, - roles: createDto.roles || [], - lastLoginAt: new Date(), + roles, + lastLoginAt: now, }, }); - this.logger.log(`Created new user: ${newUser.id} (${newUser.keycloakSub})`); + return user; + } - return newUser; + /** + * Force syncs user profile data from Keycloak token. + * This should be called when explicit profile sync is needed. + * + * Use cases: + * - Keycloak webhook notification of profile change + * - Manual profile refresh request + * - Administrative profile sync operations + * + * Note: createFromToken() already handles profile sync on login automatically. + * This method is for explicit, out-of-band sync operations. + * + * @param keycloakSub - The Keycloak subject identifier + * @param profileData - Profile data from Keycloak token + * @returns The updated user + * @throws NotFoundException if the user is not found + */ + async syncProfileFromToken( + keycloakSub: string, + profileData: Omit, + ): Promise { + // Normalize roles once + const roles = profileData.roles || []; + + try { + const updatedUser = await this.prisma.user.update({ + where: { keycloakSub }, + data: { + email: profileData.email, + name: profileData.name, + username: profileData.username, + picture: profileData.picture, + roles, + }, + }); + + this.logger.log( + `Profile synced from Keycloak for user: ${updatedUser.id} (${keycloakSub})`, + ); + + return updatedUser; + } catch (error) { + // Prisma throws P2025 when record is not found + if ( + error instanceof PrismaClientKnownRequestError && + error.code === 'P2025' + ) { + throw new NotFoundException( + `User with keycloakSub ${keycloakSub} not found`, + ); + } + throw error; + } } /** @@ -113,64 +234,13 @@ export class UsersService { return user; } - /** - * Updates a user's profile from Keycloak token data. - * This syncs the user's data from Keycloak during authentication. - * - * @param keycloakSub - The Keycloak subject identifier - * @param updateDto - Updated user data from token - * @returns The updated user - * @throws NotFoundException if the user is not found - */ - async updateFromToken( - keycloakSub: string, - updateDto: UpdateUserFromTokenDto, - ): Promise { - const user = await this.findByKeycloakSub(keycloakSub); - - if (!user) { - throw new NotFoundException( - `User with keycloakSub ${keycloakSub} not found`, - ); - } - - // Prepare update data - only include defined fields - const updateData: { - email?: string; - name?: string; - username?: string; - picture?: string; - roles?: string[]; - lastLoginAt?: Date; - } = {}; - - if (updateDto.email !== undefined) updateData.email = updateDto.email; - if (updateDto.name !== undefined) updateData.name = updateDto.name; - if (updateDto.username !== undefined) - updateData.username = updateDto.username; - if (updateDto.picture !== undefined) updateData.picture = updateDto.picture; - if (updateDto.roles !== undefined) updateData.roles = updateDto.roles; - if (updateDto.lastLoginAt !== undefined) - updateData.lastLoginAt = updateDto.lastLoginAt; - - const updatedUser = await this.prisma.user.update({ - where: { keycloakSub }, - data: updateData, - }); - - this.logger.debug( - `Synced user from token: ${updatedUser.id} (${keycloakSub})`, - ); - - return updatedUser; - } - /** * Updates a user's profile. - * Users can only update their own profile (enforced by controller). + * Currently, all profile fields are managed by Keycloak and cannot be updated locally. + * This method exists for future extensibility if local profile fields are added. * * @param id - The user's internal ID - * @param updateUserDto - The fields to update + * @param updateUserDto - The fields to update (currently none supported) * @param requestingUserKeycloakSub - The Keycloak sub of the requesting user * @returns The updated user * @throws NotFoundException if the user is not found @@ -191,22 +261,16 @@ export class UsersService { throw new ForbiddenException('You can only update your own profile'); } - // Only allow updating specific fields via the public API - // Security-sensitive fields (keycloakSub, roles, etc.) cannot be updated - const updateData: { - name?: string; - } = {}; - - if (updateUserDto.name !== undefined) { - updateData.name = updateUserDto.name; - } + // Currently no fields are updatable locally - all managed by Keycloak + // This structure allows for future extensibility if local fields are added + const updateData: Record = {}; const updatedUser = await this.prisma.user.update({ where: { id }, data: updateData, }); - this.logger.log(`User ${id} updated their profile`); + this.logger.log(`User ${id} profile update requested`); return updatedUser; }