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

BC-8092 - implement config validation #21

Merged
merged 10 commits into from
Oct 16, 2024
35 changes: 35 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
"@nestjs/passport": "^10.0.3",
"@nestjs/platform-express": "^10.4.1",
"@y/redis": "github:hpi-schul-cloud/y-redis#7d48e08d18ec78c9ab90063a7d867ec7f191319c",
"class-transformer": "^0.5.1",
"class-validator": "^0.14.1",
"ioredis": "^5.4.1",
"passport": "^0.7.0",
"passport-headerapikey": "^1.2.2",
Expand Down
4 changes: 3 additions & 1 deletion src/infra/auth-guard/auth-guard.module.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import { Module } from '@nestjs/common';
import { PassportModule } from '@nestjs/passport';
import { ConfigurationModule } from '../configuration/configuration.module.js';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

configuration und config sind sich zu ähnlich um ausdrücken zu können was für was ist.

import { XApiKeyConfig } from './config/x-api-key.config.js';
import { XApiKeyStrategy } from './strategy/index.js';

@Module({
imports: [PassportModule],
imports: [PassportModule, ConfigurationModule.register(XApiKeyConfig)],
providers: [XApiKeyStrategy],
exports: [],
})
Expand Down
9 changes: 7 additions & 2 deletions src/infra/auth-guard/config/x-api-key.config.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
export interface XApiKeyConfig {
ADMIN_API__ALLOWED_API_KEYS: string[];
import { Transform } from 'class-transformer';
import { IsArray } from 'class-validator';

export class XApiKeyConfig {
@Transform(({ value }) => value.split(',').map((part: string) => (part.split(':').pop() ?? '').trim()))
@IsArray()
public ADMIN_API__ALLOWED_API_KEYS = ['randomString'];
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sollte der default Wert wirklich bei der Anforderung festgelegt werden? Ich finde das der bessere Ort die Validierung, bzw. das lesen/zuweisen ist.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isDefined() ?

19 changes: 9 additions & 10 deletions src/infra/auth-guard/strategy/x-api-key.strategy.spec.ts
Original file line number Diff line number Diff line change
@@ -1,31 +1,28 @@
import { createMock } from '@golevelup/ts-jest';
import { UnauthorizedException } from '@nestjs/common';
import { ConfigService } from '@nestjs/config';
import { Test, TestingModule } from '@nestjs/testing';
import { XApiKeyConfig } from '../config/x-api-key.config.js';
import { XApiKeyStrategy } from './x-api-key.strategy.js';

describe('XApiKeyStrategy', () => {
let module: TestingModule;
let strategy: XApiKeyStrategy;
let configService: ConfigService<XApiKeyConfig, true>;
let config: XApiKeyConfig;

beforeAll(async () => {
module = await Test.createTestingModule({
imports: [],
providers: [
XApiKeyStrategy,
{
provide: ConfigService,
useValue: createMock<ConfigService<XApiKeyConfig, true>>({
get: () => ['7ccd4e11-c6f6-48b0-81eb-cccf7922e7a4'],
}),
provide: XApiKeyConfig,
useValue: createMock<XApiKeyConfig>(),
},
],
}).compile();

strategy = module.get(XApiKeyStrategy);
configService = module.get(ConfigService<XApiKeyConfig, true>);
config = module.get(XApiKeyConfig);
});

afterAll(async () => {
Expand All @@ -41,33 +38,35 @@ describe('XApiKeyStrategy', () => {
describe('when a valid api key is provided', () => {
const setup = () => {
const CORRECT_API_KEY = '7ccd4e11-c6f6-48b0-81eb-cccf7922e7a4';
config.ADMIN_API__ALLOWED_API_KEYS = [CORRECT_API_KEY];

return { CORRECT_API_KEY, done };
};
it('should do nothing', () => {
const { CORRECT_API_KEY } = setup();
strategy.validate(CORRECT_API_KEY, done);
expect(done).toBeCalledWith(null, true);
expect(done).toHaveBeenCalledWith(null, true);
});
});

describe('when a invalid api key is provided', () => {
const setup = () => {
const INVALID_API_KEY = '7ccd4e11-c6f6-48b0-81eb-cccf7922e7a4BAD';
config.ADMIN_API__ALLOWED_API_KEYS = [INVALID_API_KEY];

return { INVALID_API_KEY, done };
};
it('should throw error', () => {
const { INVALID_API_KEY } = setup();
strategy.validate(INVALID_API_KEY, done);
expect(done).toBeCalledWith(new UnauthorizedException(), null);
expect(done).toHaveBeenCalledWith(new UnauthorizedException(), null);
});
});
});

describe('constructor', () => {
it('should create strategy', () => {
const ApiKeyStrategy = new XApiKeyStrategy(configService);
const ApiKeyStrategy = new XApiKeyStrategy(config);
expect(ApiKeyStrategy).toBeDefined();
expect(ApiKeyStrategy).toBeInstanceOf(XApiKeyStrategy);
});
Expand Down
5 changes: 2 additions & 3 deletions src/infra/auth-guard/strategy/x-api-key.strategy.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { Injectable, UnauthorizedException } from '@nestjs/common';
import { ConfigService } from '@nestjs/config';
import { PassportStrategy } from '@nestjs/passport';
import { Strategy } from 'passport-headerapikey/lib/Strategy.js';
import { XApiKeyConfig } from '../config/index.js';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index ist hier unnötig.

Expand All @@ -9,9 +8,9 @@ import { StrategyType } from '../interface/index.js';
export class XApiKeyStrategy extends PassportStrategy(Strategy, StrategyType.API_KEY) {
private readonly allowedApiKeys: string[];

public constructor(private readonly configService: ConfigService<XApiKeyConfig, true>) {
public constructor(private readonly config: XApiKeyConfig) {
super({ header: 'X-API-KEY' }, false);
this.allowedApiKeys = this.configService.get<string[]>('ADMIN_API__ALLOWED_API_KEYS');
this.allowedApiKeys = this.config.ADMIN_API__ALLOWED_API_KEYS;
}

public validate(apiKey: string, done: (error: Error | null, data: boolean | null) => void): void {
Expand Down
6 changes: 6 additions & 0 deletions src/infra/authorization/authorization.config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { IsUrl } from 'class-validator';

export class AuthorizationConfig {
@IsUrl({ require_tld: false })
public API_HOST = 'http://localhost:3030';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Die verschiedenen Werte für prod, test, local sollten aus einer anderen Quelle kommen. Den default als local zu nutzen finde ich nicht gut. Man könnte sicher argumentieren das test eh im test file definiert werden muss. Nur gibt es einzelne envs die immer wieder gesetzt werden müssten und einmalig definiert einfach praktischer sind.

}
4 changes: 3 additions & 1 deletion src/infra/authorization/authorization.module.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import { Module } from '@nestjs/common';
import { ConfigurationModule } from '../configuration/configuration.module.js';
import { LoggerModule } from '../logging/logger.module.js';
import { AuthorizationConfig } from './authorization.config.js';
import { AuthorizationService } from './authorization.service.js';

@Module({
imports: [LoggerModule],
imports: [LoggerModule, ConfigurationModule.register(AuthorizationConfig)],
providers: [AuthorizationService],
exports: [AuthorizationService],
})
Expand Down
11 changes: 5 additions & 6 deletions src/infra/authorization/authorization.service.spec.ts
Original file line number Diff line number Diff line change
@@ -1,22 +1,23 @@
import { createMock, DeepMocked } from '@golevelup/ts-jest';
import { ConfigService } from '@nestjs/config';
import { Test, TestingModule } from '@nestjs/testing';
import { HttpRequest } from 'uws';
import { Logger } from '../logging/logger.js';
import { AuthorizationConfig } from './authorization.config.js';
import { AuthorizationService } from './authorization.service.js';

describe(AuthorizationService.name, () => {
let module: TestingModule;
let service: AuthorizationService;
let configService: DeepMocked<ConfigService>;

beforeAll(async () => {
module = await Test.createTestingModule({
providers: [
AuthorizationService,
{
provide: ConfigService,
useValue: createMock<ConfigService>(),
provide: AuthorizationConfig,
useValue: createMock<AuthorizationConfig>({
API_HOST: 'http://localhost:3000',
}),
},
{
provide: Logger,
Expand All @@ -26,7 +27,6 @@ describe(AuthorizationService.name, () => {
}).compile();

service = module.get<AuthorizationService>(AuthorizationService);
configService = module.get(ConfigService);
});

afterAll(async () => {
Expand All @@ -41,7 +41,6 @@ describe(AuthorizationService.name, () => {
const req: DeepMocked<HttpRequest> = createMock<HttpRequest>();
jest.spyOn(req, 'getParameter').mockReturnValue(roomId);
jest.spyOn(req, 'getHeader').mockReturnValue(cookies);
configService.getOrThrow.mockReturnValue('API_HOST');
const fetchSpy = jest.spyOn(global, 'fetch');

return { req, fetchSpy };
Expand Down
8 changes: 4 additions & 4 deletions src/infra/authorization/authorization.service.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
import { Injectable } from '@nestjs/common';
import { ConfigService } from '@nestjs/config';
import { HttpRequest } from 'uws';
import { Logger } from '../logging/logger.js';
import { AuthorizationConfig } from './authorization.config.js';
import { ResponsePayload } from './interfaces/response.payload.js';
import { ResponsePayloadBuilder } from './response.builder.js';

@Injectable()
export class AuthorizationService {
public constructor(
private configService: ConfigService,
private logger: Logger,
private readonly config: AuthorizationConfig,
private readonly logger: Logger,
) {
logger.setContext(AuthorizationService.name);
}
Expand Down Expand Up @@ -49,7 +49,7 @@ export class AuthorizationService {
}

private async fetchAuthorization(room: string, token: string): Promise<ResponsePayload> {
const apiHost = this.configService.getOrThrow<string>('API_HOST');
const apiHost = this.config.API_HOST;
const requestOptions = this.createAuthzRequestOptions(room, token);

const response = await fetch(`${apiHost}/api/v3/authorization/by-reference`, requestOptions);
Expand Down
22 changes: 22 additions & 0 deletions src/infra/configuration/configuration.module.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { DynamicModule, Module } from '@nestjs/common';
import { ConfigModule } from '@nestjs/config';
import { Configuration } from './configuration.service.js';

@Module({})
export class ConfigurationModule {
public static register<T extends object>(Constructor: new () => T): DynamicModule {
return {
imports: [ConfigModule.forRoot({ cache: true })],
providers: [
Configuration,
{
provide: Constructor,
useFactory: (config: Configuration): T => config.getAllValidConfigsByType(Constructor),
inject: [Configuration],
},
],
CeEv marked this conversation as resolved.
Show resolved Hide resolved
exports: [Configuration, Constructor],
module: ConfigurationModule,
};
}
}
59 changes: 59 additions & 0 deletions src/infra/configuration/configuration.service.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import { createMock } from '@golevelup/ts-jest';
import { ConfigService } from '@nestjs/config';
import { Test, TestingModule } from '@nestjs/testing';
import { IsString } from 'class-validator';
import { Configuration } from './configuration.service.js';

class TestConfig {
@IsString()
public TEST_VALUE!: string;
}

describe(Configuration.name, () => {
let module: TestingModule;
let service: Configuration;
let configService: ConfigService;

beforeAll(async () => {
module = await Test.createTestingModule({
providers: [
Configuration,
{
provide: ConfigService,
useValue: createMock<ConfigService>(),
},
],
}).compile();

service = module.get<Configuration>(Configuration);
configService = module.get<ConfigService>(ConfigService);
});

afterAll(async () => {
await module.close();
});

afterEach(() => {
jest.resetAllMocks();
});

describe('getAllValidConfigsByType', () => {
describe('when value is valid', () => {
it('should return valid configs', () => {
jest.spyOn(configService, 'get').mockReturnValueOnce('test');

const result = service.getAllValidConfigsByType(TestConfig);

expect(result).toEqual({ TEST_VALUE: 'test' });
});
});

describe('when value is not valid', () => {
it('should throw error', () => {
jest.spyOn(configService, 'get').mockReturnValueOnce(123);

expect(() => service.getAllValidConfigsByType(TestConfig)).toThrow(/isString/);
});
});
});
});
Loading
Loading