From 3861d4beb41c7561a3c85a09d70118e5119e19c5 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 15:01:00 +0200 Subject: [PATCH 01/35] Enable TypeScript strict mode This enables strict mode, but sets strictPropertyInitialization to false, as "the class-validator DTO pattern described in the documentation is not compatible with strict property initialization" according to https://github.com/nestjs/typescript-starter/pull/192 Signed-off-by: David Mehren --- tsconfig.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tsconfig.json b/tsconfig.json index c925859f0..03bcdca01 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -14,6 +14,7 @@ "./types", "./node_modules/@types" ], - "strict": false + "strict": true, + "strictPropertyInitialization": false } } From a6e245c5513d6f986926351ca75949479758a892 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 15:03:44 +0200 Subject: [PATCH 02/35] RevisionsService: Throw NotInDBError on empty DB result This adds error handling to various getters, so they throw a NotInDBError instead of (illegally, according to the type) returning null. Signed-off-by: David Mehren --- src/revisions/revisions.service.spec.ts | 24 ++++++++++++++++++++++-- src/revisions/revisions.service.ts | 21 ++++++++++++++++++--- 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/src/revisions/revisions.service.spec.ts b/src/revisions/revisions.service.spec.ts index c509d5fd7..2d1226856 100644 --- a/src/revisions/revisions.service.spec.ts +++ b/src/revisions/revisions.service.spec.ts @@ -6,6 +6,8 @@ import { Test, TestingModule } from '@nestjs/testing'; import { getRepositoryToken } from '@nestjs/typeorm'; +import { Repository } from 'typeorm'; +import { NotInDBError } from '../errors/errors'; import { LoggerModule } from '../logger/logger.module'; import { AuthorColor } from '../notes/author-color.entity'; import { Note } from '../notes/note.entity'; @@ -25,6 +27,7 @@ import appConfigMock from '../config/mock/app.config.mock'; describe('RevisionsService', () => { let service: RevisionsService; + let revisionRepo: Repository; beforeEach(async () => { const module: TestingModule = await Test.createTestingModule({ @@ -32,7 +35,7 @@ describe('RevisionsService', () => { RevisionsService, { provide: getRepositoryToken(Revision), - useValue: {}, + useClass: Repository, }, ], imports: [ @@ -57,7 +60,7 @@ describe('RevisionsService', () => { .overrideProvider(getRepositoryToken(Note)) .useValue({}) .overrideProvider(getRepositoryToken(Revision)) - .useValue({}) + .useClass(Repository) .overrideProvider(getRepositoryToken(Tag)) .useValue({}) .overrideProvider(getRepositoryToken(NoteGroupPermission)) @@ -69,9 +72,26 @@ describe('RevisionsService', () => { .compile(); service = module.get(RevisionsService); + revisionRepo = module.get>( + getRepositoryToken(Revision), + ); }); it('should be defined', () => { expect(service).toBeDefined(); }); + + describe('getRevision', () => { + it('returns a revision', async () => { + const revision = Revision.create('', ''); + jest.spyOn(revisionRepo, 'findOne').mockResolvedValueOnce(revision); + expect(await service.getRevision({} as Note, 1)).toEqual(revision); + }); + it('throws if the revision is not in the databse', async () => { + jest.spyOn(revisionRepo, 'findOne').mockResolvedValueOnce(undefined); + await expect(service.getRevision({} as Note, 1)).rejects.toThrow( + NotInDBError, + ); + }); + }); }); diff --git a/src/revisions/revisions.service.ts b/src/revisions/revisions.service.ts index 22774b945..0655a441a 100644 --- a/src/revisions/revisions.service.ts +++ b/src/revisions/revisions.service.ts @@ -13,6 +13,7 @@ import { RevisionMetadataDto } from './revision-metadata.dto'; import { RevisionDto } from './revision.dto'; import { Revision } from './revision.entity'; import { Note } from '../notes/note.entity'; +import { NotInDBError } from '../errors/errors'; @Injectable() export class RevisionsService { @@ -34,16 +35,22 @@ export class RevisionsService { } async getRevision(note: Note, revisionId: number): Promise { - return await this.revisionRepository.findOne({ + const revision = await this.revisionRepository.findOne({ where: { id: revisionId, note: note, }, }); + if (revision === undefined) { + throw new NotInDBError( + `Revision with ID ${revisionId} for note ${note.id} not found.`, + ); + } + return revision; } async getLatestRevision(noteId: string): Promise { - return await this.revisionRepository.findOne({ + const revision = await this.revisionRepository.findOne({ where: { note: noteId, }, @@ -52,10 +59,14 @@ export class RevisionsService { id: 'DESC', }, }); + if (revision === undefined) { + throw new NotInDBError(`Revision for note ${noteId} not found.`); + } + return revision; } async getFirstRevision(noteId: string): Promise { - return await this.revisionRepository.findOne({ + const revision = await this.revisionRepository.findOne({ where: { note: noteId, }, @@ -63,6 +74,10 @@ export class RevisionsService { createdAt: 'ASC', }, }); + if (revision === undefined) { + throw new NotInDBError(`Revision for note ${noteId} not found.`); + } + return revision; } toRevisionMetadataDto(revision: Revision): RevisionMetadataDto { From 74bc9612cc3077d792ba33f9e10312fb102ba99e Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 15:07:50 +0200 Subject: [PATCH 03/35] AuthService: Remove null from toAuthTokenDto return type toAuthTokenDto won't return null, as TS's strict mode prevents authToken from being nullish Signed-off-by: David Mehren --- src/auth/auth.service.ts | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/auth/auth.service.ts b/src/auth/auth.service.ts index 833c24232..90fe642a7 100644 --- a/src/auth/auth.service.ts +++ b/src/auth/auth.service.ts @@ -173,14 +173,7 @@ export class AuthService { await this.authTokenRepository.remove(token); } - toAuthTokenDto(authToken: AuthToken): AuthTokenDto | null { - if (!authToken) { - this.logger.warn( - `Recieved ${String(authToken)} argument!`, - 'toAuthTokenDto', - ); - return null; - } + toAuthTokenDto(authToken: AuthToken): AuthTokenDto { const tokenDto: AuthTokenDto = { lastUsed: null, validUntil: null, From 2da9b76a31978c3216c35cac8077e8c09ff5f07f Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 15:26:15 +0200 Subject: [PATCH 04/35] Config Utils: Fix type of toArrayConfig configValue is checked for a nullish value, the type should reflect that. Signed-off-by: David Mehren --- src/config/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config/utils.ts b/src/config/utils.ts index e02ca8ded..cee786c09 100644 --- a/src/config/utils.ts +++ b/src/config/utils.ts @@ -6,7 +6,7 @@ import { Loglevel } from './loglevel.enum'; -export function toArrayConfig(configValue: string, separator = ','): string[] { +export function toArrayConfig(configValue?: string, separator = ','): string[] { if (!configValue) { return []; } From 6fd9d64ad7ce74cc0bb29451f894b3409f446c06 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 15:34:59 +0200 Subject: [PATCH 05/35] Safely parse numbers from environment vars This adds the function parseOptionalInt to help parse numbers from environment variables Signed-off-by: David Mehren --- src/config/app.config.ts | 7 +++---- src/config/database.config.ts | 4 ++-- src/config/hsts.config.ts | 4 ++-- src/config/utils.spec.ts | 9 +++++++++ src/config/utils.ts | 7 +++++++ 5 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/config/app.config.ts b/src/config/app.config.ts index 52e593032..c12c2e171 100644 --- a/src/config/app.config.ts +++ b/src/config/app.config.ts @@ -7,7 +7,7 @@ import { registerAs } from '@nestjs/config'; import * as Joi from 'joi'; import { Loglevel } from './loglevel.enum'; -import { buildErrorMessage, toArrayConfig } from './utils'; +import { buildErrorMessage, parseOptionalInt, toArrayConfig } from './utils'; export interface AppConfig { domain: string; @@ -46,11 +46,10 @@ export default registerAs('appConfig', () => { { domain: process.env.HD_DOMAIN, rendererOrigin: process.env.HD_RENDERER_ORIGIN, - port: parseInt(process.env.PORT) || undefined, + port: parseOptionalInt(process.env.PORT), loglevel: process.env.HD_LOGLEVEL, forbiddenNoteIds: toArrayConfig(process.env.HD_FORBIDDEN_NOTE_IDS, ','), - maxDocumentLength: - parseInt(process.env.HD_MAX_DOCUMENT_LENGTH) || undefined, + maxDocumentLength: parseOptionalInt(process.env.HD_MAX_DOCUMENT_LENGTH), }, { abortEarly: false, diff --git a/src/config/database.config.ts b/src/config/database.config.ts index da437faf5..94b8a5802 100644 --- a/src/config/database.config.ts +++ b/src/config/database.config.ts @@ -7,7 +7,7 @@ import * as Joi from 'joi'; import { DatabaseDialect } from './database-dialect.enum'; import { registerAs } from '@nestjs/config'; -import { buildErrorMessage } from './utils'; +import { buildErrorMessage, parseOptionalInt } from './utils'; export interface DatabaseConfig { username: string; @@ -62,7 +62,7 @@ export default registerAs('databaseConfig', () => { password: process.env.HD_DATABASE_PASS, database: process.env.HD_DATABASE_NAME, host: process.env.HD_DATABASE_HOST, - port: parseInt(process.env.HD_DATABASE_PORT) || undefined, + port: parseOptionalInt(process.env.HD_DATABASE_PORT), storage: process.env.HD_DATABASE_STORAGE, dialect: process.env.HD_DATABASE_DIALECT, }, diff --git a/src/config/hsts.config.ts b/src/config/hsts.config.ts index 0a1aa6916..de66729fc 100644 --- a/src/config/hsts.config.ts +++ b/src/config/hsts.config.ts @@ -6,7 +6,7 @@ import * as Joi from 'joi'; import { registerAs } from '@nestjs/config'; -import { buildErrorMessage } from './utils'; +import { buildErrorMessage, parseOptionalInt } from './utils'; export interface HstsConfig { enable: boolean; @@ -32,7 +32,7 @@ export default registerAs('hstsConfig', () => { const hstsConfig = hstsSchema.validate( { enable: process.env.HD_HSTS_ENABLE, - maxAgeSeconds: parseInt(process.env.HD_HSTS_MAX_AGE) || undefined, + maxAgeSeconds: parseOptionalInt(process.env.HD_HSTS_MAX_AGE), includeSubdomains: process.env.HD_HSTS_INCLUDE_SUBDOMAINS, preload: process.env.HD_HSTS_PRELOAD, }, diff --git a/src/config/utils.spec.ts b/src/config/utils.spec.ts index 551aed0c5..6e24a37e2 100644 --- a/src/config/utils.spec.ts +++ b/src/config/utils.spec.ts @@ -6,6 +6,7 @@ import { needToLog, + parseOptionalInt, replaceAuthErrorsWithEnvironmentVariables, toArrayConfig, } from './utils'; @@ -84,4 +85,12 @@ describe('config utils', () => { expect(needToLog(currentLevel, Loglevel.TRACE)).toBeTruthy(); }); }); + describe('parseOptionalInt', () => { + it('returns undefined on undefined parameter', () => { + expect(parseOptionalInt(undefined)).toEqual(undefined); + }); + it('correctly parses a string', () => { + expect(parseOptionalInt('42')).toEqual(42); + }); + }); }); diff --git a/src/config/utils.ts b/src/config/utils.ts index cee786c09..f9f935af9 100644 --- a/src/config/utils.ts +++ b/src/config/utils.ts @@ -113,3 +113,10 @@ function transformLoglevelToInt(loglevel: Loglevel): number { return 1; } } + +export function parseOptionalInt(value?: string): number | undefined { + if (value === undefined) { + return undefined; + } + return parseInt(value); +} From cfaa07806b32a1fe41c5deb43526bafb5ac249db Mon Sep 17 00:00:00 2001 From: David Mehren Date: Sun, 9 May 2021 18:27:03 +0200 Subject: [PATCH 06/35] AuthService: Throw NotInDBError on empty DB result This adds error handling to various functions, so they throw a NotInDBError instead of a TypeError Signed-off-by: David Mehren --- src/auth/auth.service.spec.ts | 12 ++++++++++++ src/auth/auth.service.ts | 6 ++++++ 2 files changed, 18 insertions(+) diff --git a/src/auth/auth.service.spec.ts b/src/auth/auth.service.spec.ts index 9e10f418a..4240afb4e 100644 --- a/src/auth/auth.service.spec.ts +++ b/src/auth/auth.service.spec.ts @@ -168,6 +168,12 @@ describe('AuthService', () => { ); await service.setLastUsedToken(authToken.keyId); }); + it('throws if the token is not in the database', async () => { + jest.spyOn(authTokenRepo, 'findOne').mockResolvedValueOnce(undefined); + await expect(service.setLastUsedToken(authToken.keyId)).rejects.toThrow( + NotInDBError, + ); + }); }); describe('validateToken', () => { @@ -227,6 +233,12 @@ describe('AuthService', () => { ); await service.removeToken(authToken.keyId); }); + it('throws if the token is not in the database', async () => { + jest.spyOn(authTokenRepo, 'findOne').mockResolvedValueOnce(undefined); + await expect(service.removeToken(authToken.keyId)).rejects.toThrow( + NotInDBError, + ); + }); }); describe('createTokenForUser', () => { diff --git a/src/auth/auth.service.ts b/src/auth/auth.service.ts index 90fe642a7..d81a8d5c5 100644 --- a/src/auth/auth.service.ts +++ b/src/auth/auth.service.ts @@ -127,6 +127,9 @@ export class AuthService { const accessToken = await this.authTokenRepository.findOne({ where: { keyId: keyId }, }); + if (accessToken === undefined) { + throw new NotInDBError(`AuthToken for key '${keyId}' not found`); + } accessToken.lastUsed = new Date(); await this.authTokenRepository.save(accessToken); } @@ -170,6 +173,9 @@ export class AuthService { const token = await this.authTokenRepository.findOne({ where: { keyId: keyId }, }); + if (token === undefined) { + throw new NotInDBError(`AuthToken for key '${keyId}' not found`); + } await this.authTokenRepository.remove(token); } From 72b545fec5745d3f6e936e08c71bccfe7f170ef0 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 15:55:11 +0200 Subject: [PATCH 07/35] AuthTokenDto: Make properties consistently optional validUntil and lastUsed already have a IsOptional decorator, this makes the properties themselves also optional Signed-off-by: David Mehren --- src/auth/auth-token.dto.ts | 4 ++-- src/auth/auth.service.spec.ts | 4 ++-- src/auth/auth.service.ts | 2 -- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/auth/auth-token.dto.ts b/src/auth/auth-token.dto.ts index 46af7d6be..bd3ef8970 100644 --- a/src/auth/auth-token.dto.ts +++ b/src/auth/auth-token.dto.ts @@ -15,8 +15,8 @@ export class AuthTokenDto { createdAt: Date; @IsDate() @IsOptional() - validUntil: Date; + validUntil: Date | null; @IsDate() @IsOptional() - lastUsed: Date; + lastUsed: Date | null; } diff --git a/src/auth/auth.service.spec.ts b/src/auth/auth.service.spec.ts index 4240afb4e..b79f7f0bf 100644 --- a/src/auth/auth.service.spec.ts +++ b/src/auth/auth.service.spec.ts @@ -251,7 +251,7 @@ describe('AuthService', () => { }); jest.spyOn(authTokenRepo, 'save').mockImplementationOnce( async (authTokenSaved: AuthToken, _): Promise => { - expect(authTokenSaved.lastUsed).toBeUndefined(); + expect(authTokenSaved.lastUsed).toBeNull(); return authTokenSaved; }, ); @@ -275,7 +275,7 @@ describe('AuthService', () => { }); jest.spyOn(authTokenRepo, 'save').mockImplementationOnce( async (authTokenSaved: AuthToken, _): Promise => { - expect(authTokenSaved.lastUsed).toBeUndefined(); + expect(authTokenSaved.lastUsed).toBeNull(); return authTokenSaved; }, ); diff --git a/src/auth/auth.service.ts b/src/auth/auth.service.ts index d81a8d5c5..76758e680 100644 --- a/src/auth/auth.service.ts +++ b/src/auth/auth.service.ts @@ -181,8 +181,6 @@ export class AuthService { toAuthTokenDto(authToken: AuthToken): AuthTokenDto { const tokenDto: AuthTokenDto = { - lastUsed: null, - validUntil: null, label: authToken.label, keyId: authToken.keyId, createdAt: authToken.createdAt, From 53a0c87a5321aa2a07c3664d762252cdcd753eb6 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 16:06:59 +0200 Subject: [PATCH 08/35] AuthService.randomString: Throw Error instead of returning null A string with a negative length is invalid, so we should throw here instead of complicating the type with a possible null return value. Signed-off-by: David Mehren --- src/auth/auth.service.spec.ts | 6 ++++++ src/auth/auth.service.ts | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/auth/auth.service.spec.ts b/src/auth/auth.service.spec.ts index b79f7f0bf..ab13f1141 100644 --- a/src/auth/auth.service.spec.ts +++ b/src/auth/auth.service.spec.ts @@ -319,4 +319,10 @@ describe('AuthService', () => { ); }); }); + describe('randomString', () => { + it('throws on invalid lenght parameter', () => { + expect(() => service.randomString(0)).toThrow(); + expect(() => service.randomString(-1)).toThrow(); + }); + }); }); diff --git a/src/auth/auth.service.ts b/src/auth/auth.service.ts index 76758e680..45fa70280 100644 --- a/src/auth/auth.service.ts +++ b/src/auth/auth.service.ts @@ -64,7 +64,7 @@ export class AuthService { randomString(length: number): Buffer { if (length <= 0) { - return null; + throw new Error('randomString cannot have a length < 1'); } return randomBytes(length); } From d9799717b5953114ec2bcdc42ab4f9f473077dd9 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 16:07:58 +0200 Subject: [PATCH 09/35] AuthService: Fix type of toAuthTokenWithSecretDto toAuthTokenDto does not return nor accept a nullish value anymore, so the types can be adjusted. Signed-off-by: David Mehren --- src/auth/auth.service.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/auth/auth.service.ts b/src/auth/auth.service.ts index 45fa70280..52eecf9ff 100644 --- a/src/auth/auth.service.ts +++ b/src/auth/auth.service.ts @@ -198,9 +198,9 @@ export class AuthService { } toAuthTokenWithSecretDto( - authToken: AuthToken | null | undefined, + authToken: AuthToken, secret: string, - ): AuthTokenWithSecretDto | null { + ): AuthTokenWithSecretDto { const tokenDto = this.toAuthTokenDto(authToken); return { ...tokenDto, From 99103ad2179148436335cc1748935bbcfd932a66 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 16:10:46 +0200 Subject: [PATCH 10/35] ConsoleLoggerService: Fix type of context properties Nullish values of functionContext and classContext are handled correctly, so the type can be adjusted Signed-off-by: David Mehren --- src/logger/console-logger.service.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/logger/console-logger.service.ts b/src/logger/console-logger.service.ts index cc8e6a392..c87460bce 100644 --- a/src/logger/console-logger.service.ts +++ b/src/logger/console-logger.service.ts @@ -14,7 +14,7 @@ import { needToLog } from '../config/utils'; @Injectable({ scope: Scope.TRANSIENT }) export class ConsoleLoggerService { - private classContext: string; + private classContext: string | undefined; private lastTimestamp: number; constructor( @@ -83,7 +83,7 @@ export class ConsoleLoggerService { } } - private makeContextString(functionContext: string): string { + private makeContextString(functionContext?: string): string { let context = this.classContext; if (!context) { context = 'HedgeDoc'; From 0573ce4e08eb111468104e30def622c0316a38aa Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 16:13:54 +0200 Subject: [PATCH 11/35] FrontendConfig DTOs: Make properties consistently optional Some properties already have a IsOptional decorator, this makes the properties themselves also optional Signed-off-by: David Mehren --- src/frontend-config/frontend-config.dto.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/frontend-config/frontend-config.dto.ts b/src/frontend-config/frontend-config.dto.ts index 057dacb32..e2b54ac07 100644 --- a/src/frontend-config/frontend-config.dto.ts +++ b/src/frontend-config/frontend-config.dto.ts @@ -84,7 +84,7 @@ export class BrandingDto { */ @IsString() @IsOptional() - name: string; + name?: string; /** * The logo to be displayed next to the HedgeDoc logo @@ -92,7 +92,7 @@ export class BrandingDto { */ @IsUrl() @IsOptional() - logo: URL; + logo?: URL; } export class CustomAuthEntry { @@ -148,7 +148,7 @@ export class SpecialUrlsDto { */ @IsUrl() @IsOptional() - privacy: URL; + privacy?: URL; /** * A link to the terms of use @@ -156,7 +156,7 @@ export class SpecialUrlsDto { */ @IsUrl() @IsOptional() - termsOfUse: URL; + termsOfUse?: URL; /** * A link to the imprint @@ -164,7 +164,7 @@ export class SpecialUrlsDto { */ @IsUrl() @IsOptional() - imprint: URL; + imprint?: URL; } export class IframeCommunicationDto { @@ -174,7 +174,7 @@ export class IframeCommunicationDto { */ @IsUrl() @IsOptional() - editorOrigin: URL; + editorOrigin?: URL; /** * The origin under which the renderer page will be served @@ -182,7 +182,7 @@ export class IframeCommunicationDto { */ @IsUrl() @IsOptional() - rendererOrigin: URL; + rendererOrigin?: URL; } export class FrontendConfigDto { @@ -239,7 +239,7 @@ export class FrontendConfigDto { */ @IsUrl() @IsOptional() - plantUmlServer: URL; + plantUmlServer?: URL; /** * The maximal length of each document From 994bd7ae64017013dd4326e6afbe451d48dd70a9 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 16:16:30 +0200 Subject: [PATCH 12/35] HistoryService: Throw NotInDBError on empty DB result This adds error handling to getEntryByNote, so it throws a NotInDBError instead of (illegally, according to the type) returning null. Signed-off-by: David Mehren --- src/history/history.service.ts | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/history/history.service.ts b/src/history/history.service.ts index fa4aed431..fd733f732 100644 --- a/src/history/history.service.ts +++ b/src/history/history.service.ts @@ -69,13 +69,19 @@ export class HistoryService { * @return {HistoryEntry} the requested history entry */ private async getEntryByNote(note: Note, user: User): Promise { - return await this.historyEntryRepository.findOne({ + const entry = await this.historyEntryRepository.findOne({ where: { note: note, user: user, }, relations: ['note', 'user'], }); + if (!entry) { + throw new NotInDBError( + `User '${user.userName}' has no HistoryEntry for Note with id '${note.id}'`, + ); + } + return entry; } /** @@ -89,13 +95,17 @@ export class HistoryService { note: Note, user: User, ): Promise { - let entry = await this.getEntryByNote(note, user); - if (!entry) { - entry = HistoryEntry.create(user, note); - } else { + try { + const entry = await this.getEntryByNote(note, user); entry.updatedAt = new Date(); + return await this.historyEntryRepository.save(entry); + } catch (e) { + if (e instanceof NotInDBError) { + const entry = HistoryEntry.create(user, note); + return await this.historyEntryRepository.save(entry); + } + throw e; } - return await this.historyEntryRepository.save(entry); } /** @@ -112,11 +122,6 @@ export class HistoryService { updateDto: HistoryEntryUpdateDto, ): Promise { const entry = await this.getEntryByNoteIdOrAlias(noteIdOrAlias, user); - if (!entry) { - throw new NotInDBError( - `User '${user.userName}' has no HistoryEntry for Note with id '${noteIdOrAlias}'`, - ); - } entry.pinStatus = updateDto.pinStatus; return await this.historyEntryRepository.save(entry); } @@ -130,11 +135,6 @@ export class HistoryService { */ async deleteHistoryEntry(noteIdOrAlias: string, user: User): Promise { const entry = await this.getEntryByNoteIdOrAlias(noteIdOrAlias, user); - if (!entry) { - throw new NotInDBError( - `User '${user.userName}' has no HistoryEntry for Note with id '${noteIdOrAlias}'`, - ); - } await this.historyEntryRepository.remove(entry); return; } From a04a111293fd9ac9446112c61336914ab828bd63 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 16:22:01 +0200 Subject: [PATCH 13/35] Handle config initialisation error on app bootstrap Signed-off-by: David Mehren --- src/main.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/main.ts b/src/main.ts index 589b01696..d2b75f3a9 100644 --- a/src/main.ts +++ b/src/main.ts @@ -26,6 +26,11 @@ async function bootstrap(): Promise { const appConfig = configService.get('appConfig'); const mediaConfig = configService.get('mediaConfig'); + if (!appConfig || !mediaConfig) { + logger.error('Could not initialize config, aborting.', 'AppBootstrap'); + process.exit(1); + } + setupPublicApiDocs(app); logger.log( `Serving OpenAPI docs for public api under '/apidoc'`, From 96f8284e64f46180b7ea4a46541371d6aba96d47 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 16:27:15 +0200 Subject: [PATCH 14/35] MarkdownBody: Handle error in getOwnPropertyDescriptor Signed-off-by: David Mehren --- src/api/utils/markdownbody-decorator.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/api/utils/markdownbody-decorator.ts b/src/api/utils/markdownbody-decorator.ts index 4c55256a4..7614d3f30 100644 --- a/src/api/utils/markdownbody-decorator.ts +++ b/src/api/utils/markdownbody-decorator.ts @@ -42,15 +42,20 @@ export const MarkdownBody = createParamDecorator( }, [ (target, key): void => { - ApiConsumes('text/markdown')( + const ownPropertyDescriptor = Object.getOwnPropertyDescriptor( target, key, - Object.getOwnPropertyDescriptor(target, key), ); + if (!ownPropertyDescriptor) { + throw new Error( + `Could not get property descriptor for target ${target.toString()} and key ${key.toString()}`, + ); + } + ApiConsumes('text/markdown')(target, key, ownPropertyDescriptor); ApiBody({ required: true, schema: { example: '# Markdown Body' }, - })(target, key, Object.getOwnPropertyDescriptor(target, key)); + })(target, key, ownPropertyDescriptor); }, ], ); From e18ee1f0fe5afe95800f10ad7c41ad71b2e5e5be Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 16:40:13 +0200 Subject: [PATCH 15/35] UsersService: Remove null from toUserDto return type toUserDto won't return null, as TS's strict mode prevents user from being nullish Signed-off-by: David Mehren --- src/users/users.service.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/users/users.service.ts b/src/users/users.service.ts index 7a10b8fac..f7ce84217 100644 --- a/src/users/users.service.ts +++ b/src/users/users.service.ts @@ -108,13 +108,9 @@ export class UsersService { /** * Build UserInfoDto from a user. * @param {User=} user - the user to use - * @return {(UserInfoDto|null)} the built UserInfoDto + * @return {(UserInfoDto)} the built UserInfoDto */ - toUserDto(user: User | null | undefined): UserInfoDto | null { - if (!user) { - this.logger.warn(`Recieved ${String(user)} argument!`, 'toUserDto'); - return null; - } + toUserDto(user: User): UserInfoDto { return { userName: user.userName, displayName: user.displayName, From ace1b7fad6b6875aa38fd7b265b14fcceb52dfbc Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 16:43:16 +0200 Subject: [PATCH 16/35] MeController: Double-check that req.user is defined TokenAuthGuard ensures that req.user is always defined, but thanks to strict mode we have to check again. In the future, we may add a custom Request type and a custom param decorator to centralize the check. Signed-off-by: David Mehren --- src/api/public/me/me.controller.ts | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/api/public/me/me.controller.ts b/src/api/public/me/me.controller.ts index 3bc2ae167..2ba887108 100644 --- a/src/api/public/me/me.controller.ts +++ b/src/api/public/me/me.controller.ts @@ -15,6 +15,7 @@ import { Put, UseGuards, Req, + InternalServerErrorException, } from '@nestjs/common'; import { HistoryEntryUpdateDto } from '../../../history/history-entry-update.dto'; import { HistoryService } from '../../../history/history.service'; @@ -65,6 +66,10 @@ export class MeController { }) @ApiUnauthorizedResponse({ description: unauthorizedDescription }) async getMe(@Req() req: Request): Promise { + if (!req.user) { + // We should never reach this, as the TokenAuthGuard handles missing user info + throw new InternalServerErrorException('Request did not specify user'); + } return this.usersService.toUserDto( await this.usersService.getUserByUsername(req.user.userName), ); @@ -79,6 +84,10 @@ export class MeController { }) @ApiUnauthorizedResponse({ description: unauthorizedDescription }) async getUserHistory(@Req() req: Request): Promise { + if (!req.user) { + // We should never reach this, as the TokenAuthGuard handles missing user info + throw new InternalServerErrorException('Request did not specify user'); + } const foundEntries = await this.historyService.getEntriesByUser(req.user); return await Promise.all( foundEntries.map((entry) => this.historyService.toHistoryEntryDto(entry)), @@ -97,6 +106,10 @@ export class MeController { @Req() req: Request, @Param('note') note: string, ): Promise { + if (!req.user) { + // We should never reach this, as the TokenAuthGuard handles missing user info + throw new InternalServerErrorException('Request did not specify user'); + } try { const foundEntry = await this.historyService.getEntryByNoteIdOrAlias( note, @@ -124,6 +137,10 @@ export class MeController { @Param('note') note: string, @Body() entryUpdateDto: HistoryEntryUpdateDto, ): Promise { + if (!req.user) { + // We should never reach this, as the TokenAuthGuard handles missing user info + throw new InternalServerErrorException('Request did not specify user'); + } // ToDo: Check if user is allowed to pin this history entry try { return this.historyService.toHistoryEntryDto( @@ -151,6 +168,10 @@ export class MeController { @Req() req: Request, @Param('note') note: string, ): Promise { + if (!req.user) { + // We should never reach this, as the TokenAuthGuard handles missing user info + throw new InternalServerErrorException('Request did not specify user'); + } // ToDo: Check if user is allowed to delete note try { await this.historyService.deleteHistoryEntry(note, req.user); @@ -171,6 +192,10 @@ export class MeController { }) @ApiUnauthorizedResponse({ description: unauthorizedDescription }) async getMyNotes(@Req() req: Request): Promise { + if (!req.user) { + // We should never reach this, as the TokenAuthGuard handles missing user info + throw new InternalServerErrorException('Request did not specify user'); + } const notes = this.notesService.getUserNotes(req.user); return await Promise.all( (await notes).map((note) => this.notesService.toNoteMetadataDto(note)), @@ -186,6 +211,10 @@ export class MeController { }) @ApiUnauthorizedResponse({ description: unauthorizedDescription }) async getMyMedia(@Req() req: Request): Promise { + if (!req.user) { + // We should never reach this, as the TokenAuthGuard handles missing user info + throw new InternalServerErrorException('Request did not specify user'); + } const media = await this.mediaService.listUploadsByUser(req.user); return media.map((media) => this.mediaService.toMediaUploadDto(media)); } From 16ed12bfd7cf5ca88baba28de00b47b78754df42 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 16:44:27 +0200 Subject: [PATCH 17/35] MediaController: Double-check that req.user is defined TokenAuthGuard ensures that req.user is always defined, but thanks to strict mode we have to check again. In the future, we may add a custom Request type and a custom param decorator to centralize the check. Signed-off-by: David Mehren --- src/api/public/media/media.controller.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/api/public/media/media.controller.ts b/src/api/public/media/media.controller.ts index 1472088b0..3436a1dcc 100644 --- a/src/api/public/media/media.controller.ts +++ b/src/api/public/media/media.controller.ts @@ -94,6 +94,10 @@ export class MediaController { @UploadedFile() file: MulterFile, @Headers('HedgeDoc-Note') noteId: string, ): Promise { + if (!req.user) { + // We should never reach this, as the TokenAuthGuard handles missing user info + throw new InternalServerErrorException('Request did not specify user'); + } const username = req.user.userName; this.logger.debug( `Recieved filename '${file.originalname}' for note '${noteId}' from user '${username}'`, @@ -130,6 +134,10 @@ export class MediaController { @Req() req: Request, @Param('filename') filename: string, ): Promise { + if (!req.user) { + // We should never reach this, as the TokenAuthGuard handles missing user info + throw new InternalServerErrorException('Request did not specify user'); + } const username = req.user.userName; try { this.logger.debug( From b93f01fe57d4454c14b5006e511906b2207fdd2f Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 16:54:57 +0200 Subject: [PATCH 18/35] Correctly type nullable columns TypeORM columns with `nullable: true` can be `null` at runtime. This commit ensures that the types of the corresponding properties reflect that. Signed-off-by: David Mehren --- src/auth/auth-token.entity.ts | 4 ++-- src/media/media-upload.entity.ts | 4 ++-- src/notes/note.entity.ts | 6 +++--- src/notes/notes.service.spec.ts | 4 ++-- src/users/identity.entity.ts | 6 +++--- src/users/user.entity.ts | 4 ++-- 6 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/auth/auth-token.entity.ts b/src/auth/auth-token.entity.ts index 79cb8b20f..6a3349899 100644 --- a/src/auth/auth-token.entity.ts +++ b/src/auth/auth-token.entity.ts @@ -38,12 +38,12 @@ export class AuthToken { @Column({ nullable: true, }) - validUntil: Date; + validUntil: Date | null; @Column({ nullable: true, }) - lastUsed: Date; + lastUsed: Date | null; public static create( user: User, diff --git a/src/media/media-upload.entity.ts b/src/media/media-upload.entity.ts index 341ad52ad..ffa04146a 100644 --- a/src/media/media-upload.entity.ts +++ b/src/media/media-upload.entity.ts @@ -26,7 +26,7 @@ export class MediaUpload { @ManyToOne((_) => Note, (note) => note.mediaUploads, { nullable: true, }) - note: Note; + note: Note | null; @ManyToOne((_) => User, (user) => user.mediaUploads, { nullable: false, @@ -44,7 +44,7 @@ export class MediaUpload { @Column({ nullable: true, }) - backendData: BackendData; + backendData: BackendData | null; @CreateDateColumn() createdAt: Date; diff --git a/src/notes/note.entity.ts b/src/notes/note.entity.ts index 5e4cf0ef9..a3fda2b75 100644 --- a/src/notes/note.entity.ts +++ b/src/notes/note.entity.ts @@ -36,7 +36,7 @@ export class Note { unique: true, nullable: true, }) - alias?: string; + alias: string | null; @OneToMany( (_) => NoteGroupPermission, (groupPermission) => groupPermission.note, @@ -70,11 +70,11 @@ export class Note { @Column({ nullable: true, }) - description?: string; + description: string | null; @Column({ nullable: true, }) - title?: string; + title: string | null; @ManyToMany((_) => Tag, (tag) => tag.notes, { eager: true, cascade: true }) @JoinTable() diff --git a/src/notes/notes.service.spec.ts b/src/notes/notes.service.spec.ts index 7192b5d96..bb2d3f20e 100644 --- a/src/notes/notes.service.spec.ts +++ b/src/notes/notes.service.spec.ts @@ -153,7 +153,7 @@ describe('NotesService', () => { expect(newNote.groupPermissions).toHaveLength(0); expect(newNote.tags).toHaveLength(0); expect(newNote.owner).toBeUndefined(); - expect(newNote.alias).toBeUndefined(); + expect(newNote.alias).toBeNull(); }); it('without alias, with owner', async () => { const newNote = await service.createNote(content, undefined, user); @@ -166,7 +166,7 @@ describe('NotesService', () => { expect(newNote.groupPermissions).toHaveLength(0); expect(newNote.tags).toHaveLength(0); expect(newNote.owner).toEqual(user); - expect(newNote.alias).toBeUndefined(); + expect(newNote.alias).toBeNull(); }); it('with alias, without owner', async () => { const newNote = await service.createNote(content, alias); diff --git a/src/users/identity.entity.ts b/src/users/identity.entity.ts index 462399d42..6f78714b2 100644 --- a/src/users/identity.entity.ts +++ b/src/users/identity.entity.ts @@ -39,15 +39,15 @@ export class Identity { @Column({ nullable: true, }) - providerUserId?: string; + providerUserId: string | null; @Column({ nullable: true, }) - oAuthAccessToken?: string; + oAuthAccessToken: string | null; @Column({ nullable: true, }) - passwordHash?: string; + passwordHash: string | null; } diff --git a/src/users/user.entity.ts b/src/users/user.entity.ts index c04350821..606068dc7 100644 --- a/src/users/user.entity.ts +++ b/src/users/user.entity.ts @@ -41,12 +41,12 @@ export class User { @Column({ nullable: true, }) - photo?: string; + photo: string | null; @Column({ nullable: true, }) - email?: string; + email: string | null; @OneToMany((_) => Note, (note) => note.owner) ownedNotes: Note[]; From dc7d8ab470b56daffa8a8d5a270d79d10702aefe Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 16:59:18 +0200 Subject: [PATCH 19/35] MediaService: Handle unexpected backend type Signed-off-by: David Mehren --- src/media/media.service.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/media/media.service.ts b/src/media/media.service.ts index 259c94829..5debe2d59 100644 --- a/src/media/media.service.ts +++ b/src/media/media.service.ts @@ -202,6 +202,10 @@ export class MediaService { return BackendType.S3; case 'webdav': return BackendType.WEBDAV; + default: + throw new Error( + `Unexpected media backend ${this.mediaConfig.backend.use}`, + ); } } From b08a314863885324da9ba5fa77cedc0fc86d55a0 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 16:59:40 +0200 Subject: [PATCH 20/35] MediaUploadDto: Make noteId optional Signed-off-by: David Mehren --- src/media/media-upload.dto.ts | 5 +++-- src/media/media.service.ts | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/media/media-upload.dto.ts b/src/media/media-upload.dto.ts index 0ba14cf33..da82d9096 100644 --- a/src/media/media-upload.dto.ts +++ b/src/media/media-upload.dto.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: AGPL-3.0-only */ -import { IsDate, IsString } from 'class-validator'; +import { IsDate, IsOptional, IsString } from 'class-validator'; import { ApiProperty } from '@nestjs/swagger'; export class MediaUploadDto { @@ -21,8 +21,9 @@ export class MediaUploadDto { * @example "noteId" TODO how looks a note id? */ @IsString() + @IsOptional() @ApiProperty() - noteId: string; + noteId?: string; /** * The date when the upload objects was created. diff --git a/src/media/media.service.ts b/src/media/media.service.ts index 5debe2d59..4054c5f0f 100644 --- a/src/media/media.service.ts +++ b/src/media/media.service.ts @@ -227,7 +227,7 @@ export class MediaService { toMediaUploadDto(mediaUpload: MediaUpload): MediaUploadDto { return { url: mediaUpload.fileUrl, - noteId: mediaUpload.note.id, + noteId: mediaUpload.note?.id, createdAt: mediaUpload.createdAt, userName: mediaUpload.user.userName, }; From 6aa1aa229a95337e5a2e7fa175e2533fab138360 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 18:09:22 +0200 Subject: [PATCH 21/35] NoteEntity: Allow anonymous notes Notes created by anonymous users don't have an owner. This commit updates the entity accordingly. Signed-off-by: David Mehren --- src/notes/note.entity.ts | 7 ++++--- src/notes/notes.service.spec.ts | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/notes/note.entity.ts b/src/notes/note.entity.ts index a3fda2b75..3f7da51c1 100644 --- a/src/notes/note.entity.ts +++ b/src/notes/note.entity.ts @@ -56,8 +56,9 @@ export class Note { viewCount: number; @ManyToOne((_) => User, (user) => user.ownedNotes, { onDelete: 'CASCADE', // This deletes the Note, when the associated User is deleted + nullable: true, }) - owner: User; + owner: User | null; @OneToMany((_) => Revision, (revision) => revision.note, { cascade: true }) revisions: Promise; @OneToMany((_) => AuthorColor, (authorColor) => authorColor.note) @@ -89,9 +90,9 @@ export class Note { } const newNote = new Note(); newNote.shortid = shortid; - newNote.alias = alias; + newNote.alias = alias ?? null; newNote.viewCount = 0; - newNote.owner = owner; + newNote.owner = owner ?? null; newNote.authorColors = []; newNote.userPermissions = []; newNote.groupPermissions = []; diff --git a/src/notes/notes.service.spec.ts b/src/notes/notes.service.spec.ts index bb2d3f20e..ab852a9e9 100644 --- a/src/notes/notes.service.spec.ts +++ b/src/notes/notes.service.spec.ts @@ -152,7 +152,7 @@ describe('NotesService', () => { expect(newNote.userPermissions).toHaveLength(0); expect(newNote.groupPermissions).toHaveLength(0); expect(newNote.tags).toHaveLength(0); - expect(newNote.owner).toBeUndefined(); + expect(newNote.owner).toBeNull(); expect(newNote.alias).toBeNull(); }); it('without alias, with owner', async () => { @@ -177,7 +177,7 @@ describe('NotesService', () => { expect(newNote.userPermissions).toHaveLength(0); expect(newNote.groupPermissions).toHaveLength(0); expect(newNote.tags).toHaveLength(0); - expect(newNote.owner).toBeUndefined(); + expect(newNote.owner).toBeNull(); expect(newNote.alias).toEqual(alias); }); it('with alias, with owner', async () => { From 664a64495f808e7b3aaf9f028c3c60e52faa8441 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 18:12:20 +0200 Subject: [PATCH 22/35] NotesController: Double-check that req.user is defined TokenAuthGuard ensures that req.user is always defined, but thanks to strict mode we have to check again. In the future, we may add a custom Request type and a custom param decorator to centralize the check. Signed-off-by: David Mehren --- src/api/public/notes/notes.controller.ts | 45 ++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/src/api/public/notes/notes.controller.ts b/src/api/public/notes/notes.controller.ts index f32ba7d02..c87957850 100644 --- a/src/api/public/notes/notes.controller.ts +++ b/src/api/public/notes/notes.controller.ts @@ -12,6 +12,7 @@ import { Get, Header, HttpCode, + InternalServerErrorException, NotFoundException, Param, Post, @@ -88,6 +89,10 @@ export class NotesController { @Req() req: Request, @MarkdownBody() text: string, ): Promise { + if (!req.user) { + // We should never reach this, as the TokenAuthGuard handles missing user info + throw new InternalServerErrorException('Request did not specify user'); + } // ToDo: provide user for createNoteDto if (!this.permissionsService.mayCreate(req.user)) { throw new UnauthorizedException('Creating note denied!'); @@ -111,6 +116,10 @@ export class NotesController { @Req() req: Request, @Param('noteIdOrAlias') noteIdOrAlias: string, ): Promise { + if (!req.user) { + // We should never reach this, as the TokenAuthGuard handles missing user info + throw new InternalServerErrorException('Request did not specify user'); + } let note: Note; try { note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); @@ -144,6 +153,10 @@ export class NotesController { @Param('noteAlias') noteAlias: string, @MarkdownBody() text: string, ): Promise { + if (!req.user) { + // We should never reach this, as the TokenAuthGuard handles missing user info + throw new InternalServerErrorException('Request did not specify user'); + } if (!this.permissionsService.mayCreate(req.user)) { throw new UnauthorizedException('Creating note denied!'); } @@ -175,6 +188,10 @@ export class NotesController { @Param('noteIdOrAlias') noteIdOrAlias: string, @Body() noteMediaDeletionDto: NoteMediaDeletionDto, ): Promise { + if (!req.user) { + // We should never reach this, as the TokenAuthGuard handles missing user info + throw new InternalServerErrorException('Request did not specify user'); + } try { const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); if (!this.permissionsService.isOwner(req.user, note)) { @@ -217,6 +234,10 @@ export class NotesController { @Param('noteIdOrAlias') noteIdOrAlias: string, @MarkdownBody() text: string, ): Promise { + if (!req.user) { + // We should never reach this, as the TokenAuthGuard handles missing user info + throw new InternalServerErrorException('Request did not specify user'); + } try { const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); if (!this.permissionsService.mayWrite(req.user, note)) { @@ -251,6 +272,10 @@ export class NotesController { @Req() req: Request, @Param('noteIdOrAlias') noteIdOrAlias: string, ): Promise { + if (!req.user) { + // We should never reach this, as the TokenAuthGuard handles missing user info + throw new InternalServerErrorException('Request did not specify user'); + } try { const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); if (!this.permissionsService.mayRead(req.user, note)) { @@ -281,6 +306,10 @@ export class NotesController { @Req() req: Request, @Param('noteIdOrAlias') noteIdOrAlias: string, ): Promise { + if (!req.user) { + // We should never reach this, as the TokenAuthGuard handles missing user info + throw new InternalServerErrorException('Request did not specify user'); + } try { const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); if (!this.permissionsService.mayRead(req.user, note)) { @@ -315,6 +344,10 @@ export class NotesController { @Param('noteIdOrAlias') noteIdOrAlias: string, @Body() updateDto: NotePermissionsUpdateDto, ): Promise { + if (!req.user) { + // We should never reach this, as the TokenAuthGuard handles missing user info + throw new InternalServerErrorException('Request did not specify user'); + } try { const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); if (!this.permissionsService.isOwner(req.user, note)) { @@ -348,6 +381,10 @@ export class NotesController { @Req() req: Request, @Param('noteIdOrAlias') noteIdOrAlias: string, ): Promise { + if (!req.user) { + // We should never reach this, as the TokenAuthGuard handles missing user info + throw new InternalServerErrorException('Request did not specify user'); + } try { const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); if (!this.permissionsService.mayRead(req.user, note)) { @@ -384,6 +421,10 @@ export class NotesController { @Param('noteIdOrAlias') noteIdOrAlias: string, @Param('revisionId') revisionId: number, ): Promise { + if (!req.user) { + // We should never reach this, as the TokenAuthGuard handles missing user info + throw new InternalServerErrorException('Request did not specify user'); + } try { const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); if (!this.permissionsService.mayRead(req.user, note)) { @@ -415,6 +456,10 @@ export class NotesController { @Req() req: Request, @Param('noteIdOrAlias') noteIdOrAlias: string, ): Promise { + if (!req.user) { + // We should never reach this, as the TokenAuthGuard handles missing user info + throw new InternalServerErrorException('Request did not specify user'); + } try { const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); if (!this.permissionsService.mayRead(req.user, note)) { From b3e01fff7f1d26c180ab04a41e879b38932d40ad Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 18:28:32 +0200 Subject: [PATCH 23/35] GroupsService: Remove null from toGroupDto return type toGroupDto won't return null, as TS's strict mode prevents group from being nullish Signed-off-by: David Mehren --- src/groups/groups.service.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/groups/groups.service.ts b/src/groups/groups.service.ts index ea2e7fd0d..56878effa 100644 --- a/src/groups/groups.service.ts +++ b/src/groups/groups.service.ts @@ -43,11 +43,7 @@ export class GroupsService { * @param {Group} group - the group to use * @return {GroupInfoDto} the built GroupInfoDto */ - toGroupDto(group: Group | null | undefined): GroupInfoDto | null { - if (!group) { - this.logger.warn(`Recieved ${String(group)} argument!`, 'toGroupDto'); - return null; - } + toGroupDto(group: Group): GroupInfoDto { return { name: group.name, displayName: group.displayName, From 3b0ffaca3031bf4fbcf45fc197f53cccefefc186 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 18:29:12 +0200 Subject: [PATCH 24/35] Consistently type properties as optional Signed-off-by: David Mehren --- src/notes/note-metadata.dto.ts | 7 ++++--- src/notes/note-permissions.dto.ts | 15 +++++++++++---- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/notes/note-metadata.dto.ts b/src/notes/note-metadata.dto.ts index 03f23011e..f23972eec 100644 --- a/src/notes/note-metadata.dto.ts +++ b/src/notes/note-metadata.dto.ts @@ -31,7 +31,7 @@ export class NoteMetadataDto { @IsString() @IsOptional() @ApiPropertyOptional() - alias: string; + alias?: string; /** * Title of the note @@ -72,8 +72,9 @@ export class NoteMetadataDto { * User that last edited the note */ @ValidateNested() - @ApiProperty({ type: UserInfoDto }) - updateUser: UserInfoDto; + @ApiPropertyOptional({ type: UserInfoDto }) + @IsOptional() + updateUser?: UserInfoDto; /** * Counts how many times the published note has been viewed diff --git a/src/notes/note-permissions.dto.ts b/src/notes/note-permissions.dto.ts index 11c7d3c62..829732ee3 100644 --- a/src/notes/note-permissions.dto.ts +++ b/src/notes/note-permissions.dto.ts @@ -4,10 +4,16 @@ * SPDX-License-Identifier: AGPL-3.0-only */ -import { IsArray, IsBoolean, IsString, ValidateNested } from 'class-validator'; +import { + IsArray, + IsBoolean, + IsOptional, + IsString, + ValidateNested, +} from 'class-validator'; import { UserInfoDto } from '../users/user-info.dto'; import { GroupInfoDto } from '../groups/group-info.dto'; -import { ApiProperty } from '@nestjs/swagger'; +import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger'; export class NoteUserPermissionEntryDto { /** @@ -84,8 +90,9 @@ export class NotePermissionsDto { * User this permission applies to */ @ValidateNested() - @ApiProperty({ type: UserInfoDto }) - owner: UserInfoDto; + @ApiPropertyOptional({ type: UserInfoDto }) + @IsOptional() + owner?: UserInfoDto; /** * List of users the note is shared with From f8efb9717ea2b1220deb809a6abcc14b91b42156 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 18:30:17 +0200 Subject: [PATCH 25/35] NotesService: Fix type errors Signed-off-by: David Mehren --- src/notes/notes.service.ts | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/notes/notes.service.ts b/src/notes/notes.service.ts index 05e82ce7d..865d6978f 100644 --- a/src/notes/notes.service.ts +++ b/src/notes/notes.service.ts @@ -102,14 +102,18 @@ export class NotesService { } try { return await this.noteRepository.save(newNote); - } catch { - this.logger.debug( - `A note with the alias '${alias}' already exists.`, - 'createNote', - ); - throw new AlreadyInDBError( - `A note with the alias '${alias}' already exists.`, - ); + } catch (e) { + if (alias) { + this.logger.debug( + `A note with the alias '${alias}' already exists.`, + 'createNote', + ); + throw new AlreadyInDBError( + `A note with the alias '${alias}' already exists.`, + ); + } else { + throw e; + } } } @@ -304,7 +308,7 @@ export class NotesService { * @param {Note} note - the note to use * @return {User} user to be used as updateUser in the NoteDto */ - async calculateUpdateUser(note: Note): Promise { + async calculateUpdateUser(note: Note): Promise { const lastRevision = await this.getLatestRevision(note); if (lastRevision && lastRevision.authorships) { // Sort the last Revisions Authorships by their updatedAt Date to get the latest one @@ -333,7 +337,7 @@ export class NotesService { */ toNotePermissionsDto(note: Note): NotePermissionsDto { return { - owner: this.usersService.toUserDto(note.owner), + owner: note.owner ? this.usersService.toUserDto(note.owner) : undefined, sharedToUsers: note.userPermissions.map((noteUserPermission) => ({ user: this.usersService.toUserDto(noteUserPermission.user), canEdit: noteUserPermission.canEdit, From f9a035374888d3d39b6d16fe6cadb7d9c08ea947 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 18:30:48 +0200 Subject: [PATCH 26/35] NotesService.toNoteMetadataDto: Handle undefined updateUser Signed-off-by: David Mehren --- src/notes/notes.service.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/notes/notes.service.ts b/src/notes/notes.service.ts index 865d6978f..6500682ac 100644 --- a/src/notes/notes.service.ts +++ b/src/notes/notes.service.ts @@ -356,10 +356,11 @@ export class NotesService { * @return {NoteMetadataDto} the built NoteMetadataDto */ async toNoteMetadataDto(note: Note): Promise { + const updateUser = await this.calculateUpdateUser(note); return { // TODO: Convert DB UUID to base64 id: note.id, - alias: note.alias, + alias: note.alias ?? undefined, title: note.title ?? '', createTime: (await this.getFirstRevision(note)).createdAt, description: note.description ?? '', @@ -369,9 +370,9 @@ export class NotesService { permissions: this.toNotePermissionsDto(note), tags: this.toTagList(note), updateTime: (await this.getLatestRevision(note)).createdAt, - updateUser: this.usersService.toUserDto( - await this.calculateUpdateUser(note), - ), + updateUser: updateUser + ? this.usersService.toUserDto(updateUser) + : undefined, viewCount: note.viewCount, }; } From d1e352d56c72065e9444be98770ce15b52f828cd Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 19:08:59 +0200 Subject: [PATCH 27/35] Add explicit type annotations to nullable columns TypeORM can't correctly infer the data type on properties with a `| null` type. This commit adds explicit type annotations. See also https://github.com/typeorm/typeorm/issues/2567#issuecomment-408599335 Signed-off-by: David Mehren --- src/auth/auth-token.entity.ts | 2 ++ src/media/media-upload.entity.ts | 1 + src/notes/note.entity.ts | 3 +++ src/users/identity.entity.ts | 3 +++ src/users/user.entity.ts | 2 ++ 5 files changed, 11 insertions(+) diff --git a/src/auth/auth-token.entity.ts b/src/auth/auth-token.entity.ts index 6a3349899..d65beba5a 100644 --- a/src/auth/auth-token.entity.ts +++ b/src/auth/auth-token.entity.ts @@ -37,11 +37,13 @@ export class AuthToken { @Column({ nullable: true, + type: 'date', }) validUntil: Date | null; @Column({ nullable: true, + type: 'date', }) lastUsed: Date | null; diff --git a/src/media/media-upload.entity.ts b/src/media/media-upload.entity.ts index ffa04146a..64918074b 100644 --- a/src/media/media-upload.entity.ts +++ b/src/media/media-upload.entity.ts @@ -43,6 +43,7 @@ export class MediaUpload { @Column({ nullable: true, + type: 'text', }) backendData: BackendData | null; diff --git a/src/notes/note.entity.ts b/src/notes/note.entity.ts index 3f7da51c1..db53f546e 100644 --- a/src/notes/note.entity.ts +++ b/src/notes/note.entity.ts @@ -35,6 +35,7 @@ export class Note { @Column({ unique: true, nullable: true, + type: 'text', }) alias: string | null; @OneToMany( @@ -70,10 +71,12 @@ export class Note { @Column({ nullable: true, + type: 'text', }) description: string | null; @Column({ nullable: true, + type: 'text', }) title: string | null; diff --git a/src/users/identity.entity.ts b/src/users/identity.entity.ts index 6f78714b2..8cbe0f230 100644 --- a/src/users/identity.entity.ts +++ b/src/users/identity.entity.ts @@ -38,16 +38,19 @@ export class Identity { @Column({ nullable: true, + type: 'text', }) providerUserId: string | null; @Column({ nullable: true, + type: 'text', }) oAuthAccessToken: string | null; @Column({ nullable: true, + type: 'text', }) passwordHash: string | null; } diff --git a/src/users/user.entity.ts b/src/users/user.entity.ts index 606068dc7..b3267db57 100644 --- a/src/users/user.entity.ts +++ b/src/users/user.entity.ts @@ -40,11 +40,13 @@ export class User { @Column({ nullable: true, + type: 'text', }) photo: string | null; @Column({ nullable: true, + type: 'text', }) email: string | null; From f0c4fbe3713c1f8edaf692ed4b85af757d62cc18 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 19:09:15 +0200 Subject: [PATCH 28/35] Disable strict mode for tests Signed-off-by: David Mehren --- jest-e2e.json | 7 ++++++- package.json | 7 ++++++- tsconfig.test.json | 6 ++++++ tsconfig.test.json.license | 3 +++ 4 files changed, 21 insertions(+), 2 deletions(-) create mode 100644 tsconfig.test.json create mode 100644 tsconfig.test.json.license diff --git a/jest-e2e.json b/jest-e2e.json index 7594b2068..4dccfdafa 100644 --- a/jest-e2e.json +++ b/jest-e2e.json @@ -13,5 +13,10 @@ "^.+\\.(t|j)s$": "ts-jest" }, "coverageDirectory": "./coverage-e2e", - "testTimeout": 10000 + "testTimeout": 10000, + "globals": { + "ts-jest": { + "tsconfig": "tsconfig.test.json" + } + } } diff --git a/package.json b/package.json index 23cd9d452..5d73538cc 100644 --- a/package.json +++ b/package.json @@ -98,6 +98,11 @@ "^.+\\.(t|j)s$": "ts-jest" }, "coverageDirectory": "../coverage", - "testEnvironment": "node" + "testEnvironment": "node", + "globals": { + "ts-jest": { + "tsconfig": "tsconfig.test.json" + } + } } } diff --git a/tsconfig.test.json b/tsconfig.test.json new file mode 100644 index 000000000..5787af45b --- /dev/null +++ b/tsconfig.test.json @@ -0,0 +1,6 @@ +{ + "extends": "./tsconfig.json", + "compilerOptions": { + "strict": false + } +} diff --git a/tsconfig.test.json.license b/tsconfig.test.json.license new file mode 100644 index 000000000..078e5a9ac --- /dev/null +++ b/tsconfig.test.json.license @@ -0,0 +1,3 @@ +SPDX-FileCopyrightText: 2021 The HedgeDoc developers (see AUTHORS file) + +SPDX-License-Identifier: CC0-1.0 From ea11fbff1204b3f5842ad05d7764aac91e277498 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 21:14:09 +0200 Subject: [PATCH 29/35] Ensure optional properties of AuthTokenDto are initialized Signed-off-by: David Mehren --- src/auth/auth-token.entity.ts | 1 + src/auth/auth.service.ts | 2 ++ 2 files changed, 3 insertions(+) diff --git a/src/auth/auth-token.entity.ts b/src/auth/auth-token.entity.ts index d65beba5a..996199fed 100644 --- a/src/auth/auth-token.entity.ts +++ b/src/auth/auth-token.entity.ts @@ -64,6 +64,7 @@ export class AuthToken { newToken.accessTokenHash = accessToken; newToken.createdAt = new Date(); newToken.validUntil = validUntil; + newToken.lastUsed = null; return newToken; } } diff --git a/src/auth/auth.service.ts b/src/auth/auth.service.ts index 52eecf9ff..c018f4d6c 100644 --- a/src/auth/auth.service.ts +++ b/src/auth/auth.service.ts @@ -184,6 +184,8 @@ export class AuthService { label: authToken.label, keyId: authToken.keyId, createdAt: authToken.createdAt, + validUntil: null, + lastUsed: null, }; if (authToken.validUntil) { From 0c89d8715e3e70f12763801eba4f4ec79dfa6329 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 21:52:20 +0200 Subject: [PATCH 30/35] UsersService: Remove test obsoleted by strict mode Signed-off-by: David Mehren --- src/users/users.service.spec.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/users/users.service.spec.ts b/src/users/users.service.spec.ts index db93576fa..da9e3d755 100644 --- a/src/users/users.service.spec.ts +++ b/src/users/users.service.spec.ts @@ -149,9 +149,5 @@ describe('UsersService', () => { expect(userDto.photo).toEqual(''); expect(userDto.email).toEqual(''); }); - it('fails if no user is provided', () => { - expect(service.toUserDto(null)).toBeNull(); - expect(service.toUserDto(undefined)).toBeNull(); - }); }); }); From 30712abe31c02b8f144c3a56be739bfc77583d8c Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 21:53:05 +0200 Subject: [PATCH 31/35] GroupsService: Remove test obsoleted by strict mode Signed-off-by: David Mehren --- src/groups/groups.service.spec.ts | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/groups/groups.service.spec.ts b/src/groups/groups.service.spec.ts index 16a16097e..bd7d1edad 100644 --- a/src/groups/groups.service.spec.ts +++ b/src/groups/groups.service.spec.ts @@ -69,13 +69,5 @@ describe('GroupsService', () => { expect(groupDto.name).toEqual(group.name); expect(groupDto.special).toBeFalsy(); }); - it('fails with null parameter', () => { - const groupDto = service.toGroupDto(null); - expect(groupDto).toBeNull(); - }); - it('fails with undefined parameter', () => { - const groupDto = service.toGroupDto(undefined); - expect(groupDto).toBeNull(); - }); }); }); From b518583fd2b9d383fd7d2c5d4da835d9953ca36d Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 22:06:45 +0200 Subject: [PATCH 32/35] E2E Tests: Fix ESLint errors Signed-off-by: David Mehren --- test/private-api/history.e2e-spec.ts | 4 ++-- test/public-api/media.e2e-spec.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/private-api/history.e2e-spec.ts b/test/private-api/history.e2e-spec.ts index 6e5b29890..79eb8d0e9 100644 --- a/test/private-api/history.e2e-spec.ts +++ b/test/private-api/history.e2e-spec.ts @@ -185,7 +185,7 @@ describe('History', () => { const entry = await historyService.createOrUpdateHistoryEntry(note2, user); expect(entry.pinStatus).toBeFalsy(); await request(app.getHttpServer()) - .put(`/me/history/${entry.note.alias}`) + .put(`/me/history/${entry.note.alias || 'undefined'}`) .send({ pinStatus: true }) .expect(200); const userEntries = await historyService.getEntriesByUser(user); @@ -199,7 +199,7 @@ describe('History', () => { const entry2 = await historyService.createOrUpdateHistoryEntry(note, user); const entryDto = historyService.toHistoryEntryDto(entry2); await request(app.getHttpServer()) - .delete(`/me/history/${entry.note.alias}`) + .delete(`/me/history/${entry.note.alias || 'undefined'}`) .expect(200); const userEntries = await historyService.getEntriesByUser(user); expect(userEntries.length).toEqual(1); diff --git a/test/public-api/media.e2e-spec.ts b/test/public-api/media.e2e-spec.ts index b2dc81b8a..af6f558e9 100644 --- a/test/public-api/media.e2e-spec.ts +++ b/test/public-api/media.e2e-spec.ts @@ -134,7 +134,7 @@ describe('Media', () => { 'hardcoded', 'test_upload_media', ); - const filename = url.split('/').pop(); + const filename = url.split('/').pop() || ''; await request(app.getHttpServer()) .delete('/media/' + filename) .expect(204); From 64f9a29f02d26635ff076db66ea677f47e583351 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 29 Apr 2021 16:59:40 +0200 Subject: [PATCH 33/35] MediaUploadDto: Make noteId optional Signed-off-by: David Mehren --- src/media/media-upload.dto.ts | 2 +- src/media/media.service.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/media/media-upload.dto.ts b/src/media/media-upload.dto.ts index da82d9096..8bf462371 100644 --- a/src/media/media-upload.dto.ts +++ b/src/media/media-upload.dto.ts @@ -23,7 +23,7 @@ export class MediaUploadDto { @IsString() @IsOptional() @ApiProperty() - noteId?: string; + noteId: string | null; /** * The date when the upload objects was created. diff --git a/src/media/media.service.ts b/src/media/media.service.ts index 4054c5f0f..2e68c4642 100644 --- a/src/media/media.service.ts +++ b/src/media/media.service.ts @@ -227,7 +227,7 @@ export class MediaService { toMediaUploadDto(mediaUpload: MediaUpload): MediaUploadDto { return { url: mediaUpload.fileUrl, - noteId: mediaUpload.note?.id, + noteId: mediaUpload.note?.id ?? null, createdAt: mediaUpload.createdAt, userName: mediaUpload.user.userName, }; From 980da1fa43cc97701f975a677cc08ae0f6796fc7 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Sun, 2 May 2021 18:33:07 +0200 Subject: [PATCH 34/35] Fix nullable property types in Note DTOs Signed-off-by: David Mehren --- src/notes/note-metadata.dto.ts | 4 ++-- src/notes/note-permissions.dto.ts | 2 +- src/notes/notes.service.ts | 8 +++----- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/notes/note-metadata.dto.ts b/src/notes/note-metadata.dto.ts index f23972eec..f28621340 100644 --- a/src/notes/note-metadata.dto.ts +++ b/src/notes/note-metadata.dto.ts @@ -31,7 +31,7 @@ export class NoteMetadataDto { @IsString() @IsOptional() @ApiPropertyOptional() - alias?: string; + alias: string | null; /** * Title of the note @@ -74,7 +74,7 @@ export class NoteMetadataDto { @ValidateNested() @ApiPropertyOptional({ type: UserInfoDto }) @IsOptional() - updateUser?: UserInfoDto; + updateUser: UserInfoDto | null; /** * Counts how many times the published note has been viewed diff --git a/src/notes/note-permissions.dto.ts b/src/notes/note-permissions.dto.ts index 829732ee3..a9a0d9389 100644 --- a/src/notes/note-permissions.dto.ts +++ b/src/notes/note-permissions.dto.ts @@ -92,7 +92,7 @@ export class NotePermissionsDto { @ValidateNested() @ApiPropertyOptional({ type: UserInfoDto }) @IsOptional() - owner?: UserInfoDto; + owner: UserInfoDto | null; /** * List of users the note is shared with diff --git a/src/notes/notes.service.ts b/src/notes/notes.service.ts index 6500682ac..481700186 100644 --- a/src/notes/notes.service.ts +++ b/src/notes/notes.service.ts @@ -337,7 +337,7 @@ export class NotesService { */ toNotePermissionsDto(note: Note): NotePermissionsDto { return { - owner: note.owner ? this.usersService.toUserDto(note.owner) : undefined, + owner: note.owner ? this.usersService.toUserDto(note.owner) : null, sharedToUsers: note.userPermissions.map((noteUserPermission) => ({ user: this.usersService.toUserDto(noteUserPermission.user), canEdit: noteUserPermission.canEdit, @@ -360,7 +360,7 @@ export class NotesService { return { // TODO: Convert DB UUID to base64 id: note.id, - alias: note.alias ?? undefined, + alias: note.alias ?? null, title: note.title ?? '', createTime: (await this.getFirstRevision(note)).createdAt, description: note.description ?? '', @@ -370,9 +370,7 @@ export class NotesService { permissions: this.toNotePermissionsDto(note), tags: this.toTagList(note), updateTime: (await this.getLatestRevision(note)).createdAt, - updateUser: updateUser - ? this.usersService.toUserDto(updateUser) - : undefined, + updateUser: updateUser ? this.usersService.toUserDto(updateUser) : null, viewCount: note.viewCount, }; } From a72b4b1eb13d3a900ae607058cd4b13a1e465599 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Sun, 2 May 2021 18:35:38 +0200 Subject: [PATCH 35/35] Add error handling in seed.ts Signed-off-by: David Mehren --- src/seed.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/seed.ts b/src/seed.ts index 0589a041f..b9f4b23bc 100644 --- a/src/seed.ts +++ b/src/seed.ts @@ -56,7 +56,16 @@ createConnection({ user.ownedNotes = [note]; await connection.manager.save([user, note, revision]); const foundUser = await connection.manager.findOne(User); + if (!foundUser) { + throw new Error('Could not find freshly seeded user. Aborting.'); + } const foundNote = await connection.manager.findOne(Note); + if (!foundNote) { + throw new Error('Could not find freshly seeded note. Aborting.'); + } + if (!foundNote.alias) { + throw new Error('Could not find alias of freshly seeded note. Aborting.'); + } const historyEntry = HistoryEntry.create(foundUser, foundNote); await connection.manager.save(historyEntry); console.log(`Created User '${foundUser.userName}'`);