Skip to content

Commit

Permalink
EW-650 use Loggable instead of LegacyLogger (#5015)
Browse files Browse the repository at this point in the history
  • Loading branch information
MajedAlaitwniCap authored May 23, 2024
1 parent 33cba70 commit 0f5455f
Show file tree
Hide file tree
Showing 12 changed files with 168 additions and 36 deletions.
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) {
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

0 comments on commit 0f5455f

Please sign in to comment.