N21-1398 review changes

hpi-schul-cloud · Nov 8, 2023 · e887922 · e887922
1 parent d1d23c4
commit e887922
Show file tree

Hide file tree

Showing 8 changed files with 77 additions and 80 deletions.
diff --git a/apps/server/src/modules/authentication/controllers/api-test/login.api.spec.ts b/apps/server/src/modules/authentication/controllers/api-test/login.api.spec.ts
@@ -192,6 +192,7 @@ describe('Login Controller (api)', () => {
 				expect(decodedToken).toHaveProperty('accountId');
 				expect(decodedToken).toHaveProperty('schoolId');
 				expect(decodedToken).toHaveProperty('roles');
+				expect(decodedToken).toHaveProperty('isExternalUser');
 				expect(decodedToken).not.toHaveProperty('externalIdToken');
 			});
 		});

diff --git a/apps/server/src/modules/authentication/controllers/login.controller.ts b/apps/server/src/modules/authentication/controllers/login.controller.ts
@@ -3,7 +3,7 @@ import { AuthGuard } from '@nestjs/passport';
 import { ApiOperation, ApiResponse, ApiTags } from '@nestjs/swagger';
 import { ForbiddenOperationError, ValidationError } from '@shared/common';
 import { CurrentUser } from '../decorator';
-import type { ICurrentUser, LdapCurrentUser, OauthCurrentUser } from '../interface';
+import type { ICurrentUser, OauthCurrentUser } from '../interface';
 import { LoginDto } from '../uc/dto';
 import { LoginUc } from '../uc/login.uc';
 import {
@@ -27,11 +27,8 @@ export class LoginController {
 	@ApiResponse({ status: 200, type: LoginResponse, description: 'Login was successful.' })
 	@ApiResponse({ status: 400, type: ValidationError, description: 'Request data has invalid format.' })
 	@ApiResponse({ status: 403, type: ForbiddenOperationError, description: 'Invalid user credentials.' })
-	async loginLdap(
-		@CurrentUser() user: LdapCurrentUser,
-		// eslint-disable-next-line @typescript-eslint/no-unused-vars
-		@Body() _: LdapAuthorizationBodyParams
-	): Promise<LoginResponse> {
+	// eslint-disable-next-line @typescript-eslint/no-unused-vars
+	async loginLdap(@CurrentUser() user: ICurrentUser, @Body() _: LdapAuthorizationBodyParams): Promise<LoginResponse> {
 		const loginDto: LoginDto = await this.loginUc.getLoginData(user);
 
 		const mapped: LoginResponse = LoginResponseMapper.mapToLoginResponse(loginDto);

diff --git a/apps/server/src/modules/authentication/interface/index.ts b/apps/server/src/modules/authentication/interface/index.ts
@@ -1,3 +1,2 @@
 export * from './user';
-export * from './ldap-current-user';
 export * from './oauth-current-user';
diff --git a/apps/server/src/modules/authentication/interface/ldap-current-user.ts b/apps/server/src/modules/authentication/interface/ldap-current-user.ts
diff --git a/apps/server/src/modules/authentication/mapper/current-user.mapper.spec.ts b/apps/server/src/modules/authentication/mapper/current-user.mapper.spec.ts
@@ -15,42 +15,78 @@ describe('CurrentUserMapper', () => {
 
 	describe('userToICurrentUser', () => {
 		describe('when mapping from a user entity to the current user object', () => {
-			it('should map with roles', () => {
-				const teacherRole = roleFactory.buildWithId({ name: RoleName.TEACHER, permissions: [Permission.STUDENT_EDIT] });
-				const user = userFactory.buildWithId({
-					roles: [teacherRole],
-				});
-				const currentUser: ICurrentUser = CurrentUserMapper.userToICurrentUser(accountId, user);
-				expect(currentUser).toMatchObject({
-					accountId,
-					systemId: undefined,
-					roles: [teacherRole.id],
-					schoolId: null,
+			describe('when user has roles', () => {
+				const setup = () => {
+					const teacherRole = roleFactory.buildWithId({
+						name: RoleName.TEACHER,
+						permissions: [Permission.STUDENT_EDIT],
+					});
+					const user = userFactory.buildWithId({
+						roles: [teacherRole],
+					});
+
+					return {
+						teacherRole,
+						user,
+					};
+				};
+
+				it('should map with roles', () => {
+					const { teacherRole, user } = setup();
+
+					const currentUser: ICurrentUser = CurrentUserMapper.userToICurrentUser(accountId, user, false);
+
+					expect(currentUser).toMatchObject({
+						accountId,
+						systemId: undefined,
+						roles: [teacherRole.id],
+						schoolId: null,
+						isExternalUser: false,
+					});
 				});
 			});
 
-			it('should map without roles', () => {
-				const user = userFactory.buildWithId();
-				const currentUser: ICurrentUser = CurrentUserMapper.userToICurrentUser(accountId, user);
-				expect(currentUser).toMatchObject({
-					accountId,
-					systemId: undefined,
-					roles: [],
-					schoolId: null,
+			describe('when user has no roles', () => {
+				it('should map without roles', () => {
+					const user = userFactory.buildWithId();
+
+					const currentUser: ICurrentUser = CurrentUserMapper.userToICurrentUser(accountId, user, true);
+
+					expect(currentUser).toMatchObject({
+						accountId,
+						systemId: undefined,
+						roles: [],
+						schoolId: null,
+						isExternalUser: true,
+					});
 				});
 			});
 
-			it('should map system and school', () => {
-				const user = userFactory.buildWithId({
-					school: schoolFactory.buildWithId(),
-				});
-				const systemId = 'mockSystemId';
-				const currentUser: ICurrentUser = CurrentUserMapper.userToICurrentUser(accountId, user, systemId);
-				expect(currentUser).toMatchObject({
-					accountId,
-					systemId,
-					roles: [],
-					schoolId: user.school.id,
+			describe('when systemId is provided', () => {
+				const setup = () => {
+					const user = userFactory.buildWithId({
+						school: schoolFactory.buildWithId(),
+					});
+					const systemId = 'mockSystemId';
+
+					return {
+						user,
+						systemId,
+					};
+				};
+
+				it('should map system and school', () => {
+					const { user, systemId } = setup();
+
+					const currentUser: ICurrentUser = CurrentUserMapper.userToICurrentUser(accountId, user, false, systemId);
+
+					expect(currentUser).toMatchObject({
+						accountId,
+						systemId,
+						roles: [],
+						schoolId: user.school.id,
+						isExternalUser: false,
+					});
 				});
 			});
 		});
@@ -304,33 +340,4 @@ describe('CurrentUserMapper', () => {
 			});
 		});
 	});
-
-	describe('mapToLdapCurrentUser', () => {
-		const setup = () => {
-			const user = userFactory.buildWithId({
-				school: schoolFactory.buildWithId(),
-			});
-			const systemId = 'mockSystemId';
-
-			return {
-				user,
-				systemId,
-			};
-		};
-
-		it('should map to ldap current user', () => {
-			const { user, systemId } = setup();
-
-			const currentUser: ICurrentUser = CurrentUserMapper.mapToLdapCurrentUser(accountId, user, systemId);
-
-			expect(currentUser).toMatchObject({
-				accountId,
-				systemId,
-				roles: [],
-				schoolId: user.school.id,
-				userId: user.id,
-				isExternalUser: true,
-			});
-		});
-	});
 });
diff --git a/apps/server/src/modules/authentication/mapper/current-user.mapper.ts b/apps/server/src/modules/authentication/mapper/current-user.mapper.ts
@@ -2,25 +2,21 @@ import { ValidationError } from '@shared/common';
 import { Role, User } from '@shared/domain';
 import { RoleReference } from '@shared/domain/domainobject';
 import { UserDO } from '@shared/domain/domainobject/user.do';
-import { ICurrentUser, LdapCurrentUser, OauthCurrentUser } from '../interface';
+import { ICurrentUser, OauthCurrentUser } from '../interface';
 import { CreateJwtPayload, JwtPayload } from '../interface/jwt-payload';
 
 export class CurrentUserMapper {
-	static userToICurrentUser(accountId: string, user: User, systemId?: string): ICurrentUser {
+	static userToICurrentUser(accountId: string, user: User, isExternalUser: boolean, systemId?: string): ICurrentUser {
 		return {
 			accountId,
 			systemId,
 			roles: user.roles.getItems().map((role: Role) => role.id),
 			schoolId: user.school.id,
 			userId: user.id,
-			isExternalUser: false,
+			isExternalUser,
 		};
 	}
 
-	static mapToLdapCurrentUser(accountId: string, user: User, systemId?: string): LdapCurrentUser {
-		return { ...CurrentUserMapper.userToICurrentUser(accountId, user, systemId), isExternalUser: true };
-	}
-
 	static mapToOauthCurrentUser(
 		accountId: string,
 		user: UserDO,

diff --git a/apps/server/src/modules/authentication/strategy/ldap.strategy.ts b/apps/server/src/modules/authentication/strategy/ldap.strategy.ts
@@ -1,13 +1,13 @@
+import { AccountDto } from '@modules/account/services/dto';
 import { Injectable, UnauthorizedException } from '@nestjs/common';
 import { PassportStrategy } from '@nestjs/passport';
 import { LegacySchoolDo, SystemEntity, User } from '@shared/domain';
 import { LegacySchoolRepo, SystemRepo, UserRepo } from '@shared/repo';
 import { ErrorLoggable } from '@src/core/error/loggable/error.loggable';
 import { Logger } from '@src/core/logger';
-import { AccountDto } from '@modules/account/services/dto';
 import { Strategy } from 'passport-custom';
 import { LdapAuthorizationBodyParams } from '../controllers/dto';
-import { ICurrentUser, LdapCurrentUser } from '../interface';
+import { ICurrentUser } from '../interface';
 import { CurrentUserMapper } from '../mapper';
 import { AuthenticationService } from '../services/authentication.service';
 import { LdapService } from '../services/ldap.service';
@@ -48,7 +48,7 @@ export class LdapStrategy extends PassportStrategy(Strategy, 'ldap') {
 
 		await this.checkCredentials(account, system, ldapDn, password);
 
-		const currentUser: LdapCurrentUser = CurrentUserMapper.mapToLdapCurrentUser(account.id, user, systemId);
+		const currentUser: ICurrentUser = CurrentUserMapper.userToICurrentUser(account.id, user, true, systemId);
 
 		return currentUser;
 	}

diff --git a/apps/server/src/modules/authentication/strategy/local.strategy.ts b/apps/server/src/modules/authentication/strategy/local.strategy.ts
@@ -39,7 +39,7 @@ export class LocalStrategy extends PassportStrategy(Strategy) {
 			new Error(`login failing, because account ${account.id} has no userId`)
 		);
 		const user = await this.userRepo.findById(accountUserId, true);
-		const currentUser = CurrentUserMapper.userToICurrentUser(account.id, user);
+		const currentUser = CurrentUserMapper.userToICurrentUser(account.id, user, false);
 		return currentUser;
 	}