Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EW-650 use Loggable instead of LegacyLogger #5015

Merged
merged 8 commits into from
May 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -218,8 +218,6 @@ describe('Login Controller (api)', () => {
const decodedToken = jwt.decode(token);
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
expect(response.body.accessToken).toBeDefined();
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
expect(response.body.accessToken).toBeDefined();
expect(decodedToken).toHaveProperty('userId');
expect(decodedToken).toHaveProperty('accountId');
expect(decodedToken).toHaveProperty('schoolId');
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { AccountNotFoundLoggableException } from './account-not-found.loggable-exception';

describe(AccountNotFoundLoggableException.name, () => {
describe('getLogMessage', () => {
const setup = () => {
const exception = new AccountNotFoundLoggableException();

return {
exception,
};
};

it('should return the correct log message', () => {
const { exception } = setup();

const message = exception.getLogMessage();

expect(message).toEqual({
type: 'UNAUTHORIZED_EXCEPTION',
stack: expect.any(String),
data: {},
});
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { HttpStatus } from '@nestjs/common';
import { BusinessError } from '@shared/common';
import { Loggable } from '@src/core/logger/interfaces';
import { ErrorLogMessage } from '@src/core/logger/types';

export class AccountNotFoundLoggableException extends BusinessError implements Loggable {
constructor() {
super(
{
type: 'UNAUTHORIZED_EXCEPTION',
title: 'Login has failed because account not found',
defaultMessage: 'Login has failed because account not found',
},
HttpStatus.UNAUTHORIZED
);
}

getLogMessage(): ErrorLogMessage {
const message: ErrorLogMessage = {
type: this.type,
stack: this.stack,
data: {},
};

return message;
}
}
3 changes: 3 additions & 0 deletions apps/server/src/modules/authentication/loggable/index.ts
Original file line number Diff line number Diff line change
@@ -1 +1,4 @@
export * from './school-in-migration.loggable-exception';
export * from './account-not-found.loggable-exception';
export * from './user-could-not-be-authenticated.loggable.exception';
export * from './user-authenticated.loggable';
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { UserAuthenticatedLoggable } from './user-authenticated.loggable';

describe(UserAuthenticatedLoggable.name, () => {
describe('getLogMessage', () => {
const setup = () => {
const loggable = new UserAuthenticatedLoggable();

return {
loggable,
};
};

it('should return the correct log message', () => {
const { loggable } = setup();

const message = loggable.getLogMessage();

expect(message).toEqual({
message: 'SUCCESSFULLY_AUTHENTICATED',
data: {},
});
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { Loggable } from '@src/core/logger/interfaces';
import { LogMessage } from '@src/core/logger/types';

export class UserAuthenticatedLoggable implements Loggable {
getLogMessage(): LogMessage {
const message: LogMessage = {
message: 'SUCCESSFULLY_AUTHENTICATED',
data: {},
};

return message;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { LdapUserCouldNotBeAuthenticatedLoggableException } from './user-could-not-be-authenticated.loggable.exception';

describe(LdapUserCouldNotBeAuthenticatedLoggableException.name, () => {
describe('getLogMessage', () => {
const setup = () => {
const err = new Error('test');
const exception = new LdapUserCouldNotBeAuthenticatedLoggableException(err);

return {
exception,
err,
};
};

it('should return the correct log message', () => {
const { err, exception } = setup();

const message = exception.getLogMessage();

expect(message).toEqual({
type: 'UNAUTHORIZED_EXCEPTION',
stack: expect.any(String),
data: { error: JSON.stringify(err) },
});
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { HttpStatus } from '@nestjs/common';
import { BusinessError } from '@shared/common';
import { Loggable } from '@src/core/logger/interfaces';
import { ErrorLogMessage } from '@src/core/logger/types';

export class LdapUserCouldNotBeAuthenticatedLoggableException extends BusinessError implements Loggable {
constructor(private readonly error: Error) {
super(
{
type: 'UNAUTHORIZED_EXCEPTION',
title: 'User could not be authenticated',
defaultMessage: 'LdapService connection failed because User could not be authenticated',
},
HttpStatus.UNAUTHORIZED
);
}

getLogMessage(): ErrorLogMessage {
const message: ErrorLogMessage = {
type: this.type,
stack: this.stack,
data: {
error: JSON.stringify(this.error),
},
};

return message;
}
}
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { createMock } from '@golevelup/ts-jest';
import { UnauthorizedException } from '@nestjs/common';
import { Test, TestingModule } from '@nestjs/testing';
import { SystemEntity } from '@shared/domain/entity';
import { systemEntityFactory } from '@shared/testing';
import { LegacyLogger } from '@src/core/logger';
import { Logger } from '@src/core/logger';
import { LdapUserCouldNotBeAuthenticatedLoggableException } from '../loggable';
import { LdapService } from './ldap.service';

const mockClient = {
Expand Down Expand Up @@ -47,8 +47,8 @@ describe('LdapService', () => {
providers: [
LdapService,
{
provide: LegacyLogger,
useValue: createMock<LegacyLogger>(),
provide: Logger,
useValue: createMock<Logger>(),
},
],
}).compile();
Expand Down Expand Up @@ -76,19 +76,10 @@ describe('LdapService', () => {
});

describe('when user is not authorized', () => {
it('should throw unauthorized error', async () => {
it('should throw UserCouldNotAuthenticateLoggableException', async () => {
const system: SystemEntity = systemEntityFactory.withLdapConfig().buildWithId();
await expect(ldapService.checkLdapCredentials(system, 'mockUsername', 'mockPassword')).rejects.toThrow(
new UnauthorizedException('User could not authenticate')
);
});
});

describe('when connected flag is not set', () => {
it('should throw unauthorized error', async () => {
const system: SystemEntity = systemEntityFactory.withLdapConfig().buildWithId();
await expect(ldapService.checkLdapCredentials(system, 'connectWithoutFlag', 'mockPassword')).rejects.toThrow(
new UnauthorizedException('User could not authenticate')
LdapUserCouldNotBeAuthenticatedLoggableException
);
});
});
Expand Down
16 changes: 5 additions & 11 deletions apps/server/src/modules/authentication/services/ldap.service.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import { Injectable, UnauthorizedException } from '@nestjs/common';
import { SystemEntity } from '@shared/domain/entity';
import { ErrorUtils } from '@src/core/error/utils';
import { LegacyLogger } from '@src/core/logger';
import { Logger } from '@src/core/logger';
import { Client, createClient } from 'ldapjs';
import { LdapConnectionError } from '../errors/ldap-connection.error';
import { UserAuthenticatedLoggable, LdapUserCouldNotBeAuthenticatedLoggableException } from '../loggable';

@Injectable()
export class LdapService {
constructor(private readonly logger: LegacyLogger) {
constructor(private readonly logger: Logger) {
this.logger.setContext(LdapService.name);
}

Expand Down Expand Up @@ -37,15 +37,9 @@ export class LdapService {
client.on('connect', () => {
client.bind(username, password, (err) => {
if (err) {
MajedAlaitwniCap marked this conversation as resolved.
Show resolved Hide resolved
this.logger.debug(err);
reject(
new UnauthorizedException(
'User could not authenticate',
ErrorUtils.createHttpExceptionOptions(err, 'LdapService:connect')
)
);
reject(new LdapUserCouldNotBeAuthenticatedLoggableException(err));
} else {
this.logger.debug('[LDAP] Bind successful');
this.logger.debug(new UserAuthenticatedLoggable());
resolve(client);
}
});
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { createMock, DeepMocked } from '@golevelup/ts-jest';
import { AccountService, Account } from '@modules/account';
import { Account, AccountService } from '@modules/account';
import { OAuthService, OAuthTokenDto } from '@modules/oauth';
import { UnauthorizedException } from '@nestjs/common';
import { Test, TestingModule } from '@nestjs/testing';
import { UserDO } from '@shared/domain/domainobject/user.do';
import { RoleName } from '@shared/domain/interface';
Expand All @@ -12,8 +11,9 @@ import { ICurrentUser, OauthCurrentUser } from '../interface';

import { SchoolInMigrationLoggableException } from '../loggable';

import { Oauth2Strategy } from './oauth2.strategy';
import { AccountNotFoundLoggableException } from '../loggable/account-not-found.loggable-exception';
import { UserAccountDeactivatedLoggableException } from '../loggable/user-account-deactivated-exception';
import { Oauth2Strategy } from './oauth2.strategy';

describe('Oauth2Strategy', () => {
let module: TestingModule;
Expand Down Expand Up @@ -133,15 +133,16 @@ describe('Oauth2Strategy', () => {
accountService.findByUserId.mockResolvedValue(null);
};

it('should throw an UnauthorizedException', async () => {
it('should throw an AccountNotFoundLoggableException', async () => {
setup();

const func = async () =>
strategy.validate({
body: { code: 'code', redirectUri: 'redirectUri', systemId: 'systemId' },
});

await expect(func).rejects.toThrow(new UnauthorizedException('no account found'));
const loggableException = new AccountNotFoundLoggableException();
await expect(func).rejects.toThrow(loggableException);
});
});

Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import { AccountService, Account } from '@modules/account';
import { OAuthService, OAuthTokenDto } from '@modules/oauth';
import { Injectable, UnauthorizedException } from '@nestjs/common';
import { Injectable } from '@nestjs/common';
import { PassportStrategy } from '@nestjs/passport';
import { UserDO } from '@shared/domain/domainobject/user.do';
import { Strategy } from 'passport-custom';
import { Oauth2AuthorizationBodyParams } from '../controllers/dto';
import { ICurrentUser, OauthCurrentUser } from '../interface';
import { SchoolInMigrationLoggableException } from '../loggable';
import { AccountNotFoundLoggableException, SchoolInMigrationLoggableException } from '../loggable';
import { CurrentUserMapper } from '../mapper';
import { UserAccountDeactivatedLoggableException } from '../loggable/user-account-deactivated-exception';

Expand All @@ -29,7 +29,7 @@ export class Oauth2Strategy extends PassportStrategy(Strategy, 'oauth2') {

const account: Account | null = await this.accountService.findByUserId(user.id);
if (!account) {
throw new UnauthorizedException('no account found');
throw new AccountNotFoundLoggableException();
}

if (account.deactivatedAt !== undefined && account.deactivatedAt.getTime() <= Date.now()) {
Expand Down
Loading