diff --git a/.env.example b/.env.example index d972f49d..7d334427 100644 --- a/.env.example +++ b/.env.example @@ -104,6 +104,8 @@ JWT_EXPIRES_IN=15m JWT_REFRESH_SECRET=your-super-secret-refresh-key-change-this-in-production # Refresh token expiration time +# IMPORTANT: JWT_REFRESH_EXPIRES_IN should be <= AUTH_SESSION_TTL_SECONDS. +# A startup warning will be logged if the session TTL is shorter than this value. JWT_REFRESH_EXPIRES_IN=7d # Encryption secret [REQUIRED, exactly 32 characters] @@ -237,8 +239,16 @@ SESSION_COOKIE_NAME=teachlink.sid SESSION_PREFIX=sess: # Session time-to-live in seconds (default: 604800 = 7 days) +# NOTE: The session service reads AUTH_SESSION_TTL_SECONDS. Both should be set identically. +# IMPORTANT: This value should be >= the JWT refresh token lifetime (JWT_REFRESH_EXPIRES_IN). +# If the session TTL is shorter than the refresh token lifetime, sessions may expire while +# refresh tokens are still valid, causing authentication state inconsistencies. +# A startup warning will be logged if this misalignment is detected. SESSION_TTL_SECONDS=604800 +# Auth session TTL (used by SessionService, should match SESSION_TTL_SECONDS) +AUTH_SESSION_TTL_SECONDS=604800 + # Browser cookie max age in milliseconds (should match SESSION_TTL_SECONDS) SESSION_COOKIE_MAX_AGE_MS=604800000 diff --git a/src/auth/auth.controller.ts b/src/auth/auth.controller.ts index 1f21a6e0..76661f02 100644 --- a/src/auth/auth.controller.ts +++ b/src/auth/auth.controller.ts @@ -33,7 +33,10 @@ export class AuthController { @ApiResponse({ status: 200, description: 'Successfully authenticated' }) @ApiResponse({ status: 401, description: 'Invalid credentials' }) async login(@Body() loginDto: LoginDto) { - const user = await this.userRepository.findOneBy({ email: loginDto.email }); + const user = await this.userRepository.findOne({ + where: { email: loginDto.email }, + relations: ['roles'], + }); if (!user) { throw new UnauthorizedException('Invalid credentials'); } diff --git a/src/auth/auth.service.spec.ts b/src/auth/auth.service.spec.ts index 95feb6b4..55fc7a07 100644 --- a/src/auth/auth.service.spec.ts +++ b/src/auth/auth.service.spec.ts @@ -22,11 +22,13 @@ function makeUser(overrides: Partial = {}): User { status: UserStatus.ACTIVE, refreshToken: 'old-hash', passwordHistory: [], + roles: [{ name: 'student' }], ...overrides, } as User; } const mockUserRepo = { + findOne: jest.fn(), findOneBy: jest.fn(), update: jest.fn(), }; @@ -112,21 +114,21 @@ describe('AuthService', () => { it('throws UnauthorizedException when the user does not exist', async () => { mockJwtService.verify.mockReturnValue(validDecoded); - mockUserRepo.findOneBy.mockResolvedValue(null); + mockUserRepo.findOne.mockResolvedValue(null); await expect(service.refreshTokens('token')).rejects.toThrow(UnauthorizedException); }); it('throws UnauthorizedException when user has no stored refresh token', async () => { mockJwtService.verify.mockReturnValue(validDecoded); - mockUserRepo.findOneBy.mockResolvedValue(makeUser({ refreshToken: undefined })); + mockUserRepo.findOne.mockResolvedValue(makeUser({ refreshToken: undefined })); await expect(service.refreshTokens('token')).rejects.toThrow(UnauthorizedException); }); it('revokes all tokens and throws when a blacklisted token is reused', async () => { mockJwtService.verify.mockReturnValue(validDecoded); - mockUserRepo.findOneBy.mockResolvedValue(makeUser()); + mockUserRepo.findOne.mockResolvedValue(makeUser()); mockBlacklistService.isBlacklisted.mockResolvedValue(true); mockUserRepo.update.mockResolvedValue(undefined); @@ -136,21 +138,21 @@ describe('AuthService', () => { it('throws UnauthorizedException when the user status is SUSPENDED', async () => { mockJwtService.verify.mockReturnValue(validDecoded); - mockUserRepo.findOneBy.mockResolvedValue(makeUser({ status: UserStatus.SUSPENDED })); + mockUserRepo.findOne.mockResolvedValue(makeUser({ status: UserStatus.SUSPENDED })); await expect(service.refreshTokens('token')).rejects.toThrow(UnauthorizedException); }); it('throws UnauthorizedException when the user status is INACTIVE', async () => { mockJwtService.verify.mockReturnValue(validDecoded); - mockUserRepo.findOneBy.mockResolvedValue(makeUser({ status: UserStatus.INACTIVE })); + mockUserRepo.findOne.mockResolvedValue(makeUser({ status: UserStatus.INACTIVE })); await expect(service.refreshTokens('token')).rejects.toThrow(UnauthorizedException); }); it('issues new tokens when the refresh token is valid and not blacklisted', async () => { mockJwtService.verify.mockReturnValue(validDecoded); - mockUserRepo.findOneBy.mockResolvedValue(makeUser()); + mockUserRepo.findOne.mockResolvedValue(makeUser()); mockBlacklistService.isBlacklisted.mockResolvedValue(false); mockBlacklistService.addToBlacklist.mockResolvedValue(undefined); mockJwtService.signAsync diff --git a/src/auth/auth.service.ts b/src/auth/auth.service.ts index c7f75826..57c952ea 100644 --- a/src/auth/auth.service.ts +++ b/src/auth/auth.service.ts @@ -45,7 +45,10 @@ export class AuthService { } const userId = decoded.sub; - const user = await this.userRepository.findOneBy({ id: userId }); + const user = await this.userRepository.findOne({ + where: { id: userId }, + relations: ['roles'], + }); if (!user || !user.refreshToken) { throw new UnauthorizedException('Access Denied'); diff --git a/src/courses/courses.service.ts b/src/courses/courses.service.ts index a4bc7165..de404da1 100644 --- a/src/courses/courses.service.ts +++ b/src/courses/courses.service.ts @@ -12,7 +12,7 @@ import { BulkOperationStatus, BulkOperationType, } from './entities/bulk-operation.entity'; -import { User, UserRole } from '../users/entities/user.entity'; +import { User } from '../users/entities/user.entity'; import { CreateCourseDto } from './dto/create-course.dto'; import { UpdateCourseDto } from './dto/update-course.dto'; import { SubmitForReviewDto } from './dto/submit-for-review.dto'; @@ -108,7 +108,10 @@ export class CoursesService { const page = query?.page ?? 1; const limit = query?.limit ?? 20; const isPrivileged = - requestingUser && [UserRole.ADMIN, UserRole.MODERATOR].includes(requestingUser.role); + requestingUser && + requestingUser.roles?.some((role) => + ['admin', 'moderator'].includes(typeof role === 'string' ? role : role.name), + ); const where = isPrivileged ? {} : { status: CourseStatus.PUBLISHED }; const [data, total] = await this.courseRepo.findAndCount({ @@ -387,14 +390,19 @@ export class CoursesService { } private assertPrivileged(user: User): void { - if (![UserRole.ADMIN, UserRole.MODERATOR].includes(user.role)) { + const isPrivileged = user.roles?.some((role) => + ['admin', 'moderator'].includes(typeof role === 'string' ? role : role.name), + ); + if (!isPrivileged) { throw new ForbiddenOperationException('Only admins or moderators may perform this action.'); } } private assertOwnerOrPrivileged(course: Course, user: User): void { const isOwner = course.instructorId === user.id; - const isPrivileged = [UserRole.ADMIN, UserRole.MODERATOR].includes(user.role); + const isPrivileged = user.roles?.some((role) => + ['admin', 'moderator'].includes(typeof role === 'string' ? role : role.name), + ); if (!isOwner && !isPrivileged) { throw new ForbiddenOperationException('Insufficient permissions.'); } diff --git a/src/courses/enrollments.service.ts b/src/courses/enrollments.service.ts index d7421ddc..34eab8da 100644 --- a/src/courses/enrollments.service.ts +++ b/src/courses/enrollments.service.ts @@ -11,7 +11,7 @@ import { EventEmitter2 } from '@nestjs/event-emitter'; import { Course, CourseStatus } from './entities/course.entity'; import { Enrollment } from './entities/enrollment.entity'; -import { User, UserRole } from '../users/entities/user.entity'; +import { User } from '../users/entities/user.entity'; import { CACHE_EVENTS } from '../caching/caching.constants'; import { APP_EVENTS } from '../common/constants/event.constants'; @@ -272,7 +272,11 @@ export class EnrollmentsService { * Check admin/moderator role. */ private isPrivileged(user: User): boolean { - return [UserRole.ADMIN, UserRole.MODERATOR].includes(user.role); + return ( + user.roles?.some((role) => + ['admin', 'moderator'].includes(typeof role === 'string' ? role : role.name), + ) ?? false + ); } /** diff --git a/src/session/session.service.spec.ts b/src/session/session.service.spec.ts index 5b256361..63c0ba96 100644 --- a/src/session/session.service.spec.ts +++ b/src/session/session.service.spec.ts @@ -243,4 +243,96 @@ describe('SessionService', () => { ); }); }); + + describe('parseDurationToSeconds', () => { + it('should parse days correctly', () => { + expect(SessionService.parseDurationToSeconds('7d')).toBe(604800); + expect(SessionService.parseDurationToSeconds('30d')).toBe(2592000); + }); + + it('should parse hours correctly', () => { + expect(SessionService.parseDurationToSeconds('1h')).toBe(3600); + expect(SessionService.parseDurationToSeconds('24h')).toBe(86400); + }); + + it('should parse minutes correctly', () => { + expect(SessionService.parseDurationToSeconds('15m')).toBe(900); + expect(SessionService.parseDurationToSeconds('60m')).toBe(3600); + }); + + it('should parse seconds correctly', () => { + expect(SessionService.parseDurationToSeconds('3600s')).toBe(3600); + }); + + it('should parse bare numeric strings', () => { + expect(SessionService.parseDurationToSeconds('604800')).toBe(604800); + }); + + it('should handle whitespace', () => { + expect(SessionService.parseDurationToSeconds(' 7d ')).toBe(604800); + }); + + it('should return 0 for unrecognized formats', () => { + expect(SessionService.parseDurationToSeconds('invalid')).toBe(0); + expect(SessionService.parseDurationToSeconds('')).toBe(0); + }); + }); + + describe('constructor session TTL validation', () => { + it('should warn when session TTL (3600s) is shorter than refresh token lifetime (7d)', async () => { + const configWithShortSession = { + get: jest.fn((key: string, defaultVal?: string) => { + const values: Record = { + AUTH_SESSION_PREFIX: 'auth:sess:', + AUTH_SESSION_LEGACY_PREFIX: 'session:', + AUTH_SESSION_TTL_SECONDS: '3600', + SESSION_LOCK_TTL_MS: '5000', + SESSION_LOCK_MAX_RETRIES: '5', + SESSION_LOCK_RETRY_DELAY_MS: '120', + JWT_REFRESH_EXPIRES_IN: '7d', + }; + return values[key] ?? defaultVal ?? ''; + }), + }; + + const module = await Test.createTestingModule({ + providers: [ + SessionService, + { provide: SESSION_REDIS_CLIENT, useValue: mockRedis }, + { provide: ConfigService, useValue: configWithShortSession }, + ], + }).compile(); + + const svc = module.get(SessionService); + expect(svc).toBeDefined(); + }); + + it('should not warn when session TTL matches refresh token lifetime', async () => { + const configWithMatchingSession = { + get: jest.fn((key: string, defaultVal?: string) => { + const values: Record = { + AUTH_SESSION_PREFIX: 'auth:sess:', + AUTH_SESSION_LEGACY_PREFIX: 'session:', + AUTH_SESSION_TTL_SECONDS: '604800', + SESSION_LOCK_TTL_MS: '5000', + SESSION_LOCK_MAX_RETRIES: '5', + SESSION_LOCK_RETRY_DELAY_MS: '120', + JWT_REFRESH_EXPIRES_IN: '7d', + }; + return values[key] ?? defaultVal ?? ''; + }), + }; + + const module = await Test.createTestingModule({ + providers: [ + SessionService, + { provide: SESSION_REDIS_CLIENT, useValue: mockRedis }, + { provide: ConfigService, useValue: configWithMatchingSession }, + ], + }).compile(); + + const svc = module.get(SessionService); + expect(svc).toBeDefined(); + }); + }); }); diff --git a/src/session/session.service.ts b/src/session/session.service.ts index fbb52086..aaa195ed 100644 --- a/src/session/session.service.ts +++ b/src/session/session.service.ts @@ -46,6 +46,44 @@ export class SessionService implements OnModuleDestroy { this.configService.get('SESSION_LOCK_RETRY_DELAY_MS') || '120', 10, ); + + const jwtRefreshExpiresIn = this.configService.get('JWT_REFRESH_EXPIRES_IN') || '7d'; + const jwtRefreshExpirySeconds = SessionService.parseDurationToSeconds(jwtRefreshExpiresIn); + + if (this.sessionTtlSeconds < jwtRefreshExpirySeconds) { + this.logger.warn( + `Session TTL (${this.sessionTtlSeconds}s from AUTH_SESSION_TTL_SECONDS) is shorter ` + + `than refresh token lifetime (${jwtRefreshExpirySeconds}s from JWT_REFRESH_EXPIRES_IN="${jwtRefreshExpiresIn}"). ` + + 'Sessions may expire while refresh tokens are still valid, causing authentication state inconsistencies.', + ); + } + } + + /** + * Parses a duration string (e.g. "7d", "30d", "1h", "60m", "3600s") into seconds. + * Falls back to parseInt for bare numeric strings. + */ + static parseDurationToSeconds(duration: string): number { + const trimmed = duration.trim().toLowerCase(); + const match = trimmed.match(/^(\d+)\s*(d|h|m|s)$/); + if (match) { + const value = parseInt(match[1], 10); + switch (match[2]) { + case 'd': + return value * 86400; + case 'h': + return value * 3600; + case 'm': + return value * 60; + case 's': + return value; + } + } + const numeric = parseInt(trimmed, 10); + if (!Number.isNaN(numeric)) { + return numeric; + } + return 0; } /** diff --git a/src/users/entities/user.entity.spec.ts b/src/users/entities/user.entity.spec.ts new file mode 100644 index 00000000..5517df35 --- /dev/null +++ b/src/users/entities/user.entity.spec.ts @@ -0,0 +1,36 @@ +import { User, UserRole } from './user.entity'; + +describe('User entity - role getter', () => { + it('should throw when roles relation is not loaded (undefined)', () => { + const user = new User(); + expect(() => user.role).toThrow( + 'User.roles relation not loaded. Include relations: ["roles"] in the query.', + ); + }); + + it('should return the first role name when roles are loaded with values', () => { + const user = new User(); + (user as any).roles = [{ name: 'teacher' }, { name: 'moderator' }]; + expect(user.role).toBe(UserRole.TEACHER); + }); + + it('should return STUDENT when roles array is loaded but empty', () => { + const user = new User(); + (user as any).roles = []; + expect(user.role).toBe(UserRole.STUDENT); + }); + + it('should throw when roles is explicitly set to undefined', () => { + const user = new User(); + (user as any).roles = undefined; + expect(() => user.role).toThrow( + 'User.roles relation not loaded. Include relations: ["roles"] in the query.', + ); + }); + + it('should not throw when roles is null (edge case)', () => { + const user = new User(); + (user as any).roles = null; + expect(user.role).toBe(UserRole.STUDENT); + }); +}); diff --git a/src/users/entities/user.entity.ts b/src/users/entities/user.entity.ts index e833c943..75020c63 100644 --- a/src/users/entities/user.entity.ts +++ b/src/users/entities/user.entity.ts @@ -118,6 +118,9 @@ export class User { roles: Role[]; get role(): UserRole { + if (this.roles === undefined) { + throw new Error('User.roles relation not loaded. Include relations: ["roles"] in the query.'); + } if (this.roles && this.roles.length > 0) { return this.roles[0].name as UserRole; }