From 9597ac54227f46d5a49255cbbf9ed5f1688a7891 Mon Sep 17 00:00:00 2001 From: Erik Michelson Date: Fri, 12 Apr 2024 11:49:05 +0200 Subject: [PATCH] feat(notes): check for equal alias or note id When creating a new note or adding a new alias to one, it is checked that the new name is neither forbidden nor already in use. Co-authored-by: David Mehren Signed-off-by: Erik Michelson --- backend/src/notes/alias.service.spec.ts | 31 +++++---- backend/src/notes/alias.service.ts | 64 +++++++------------ backend/src/notes/notes.service.spec.ts | 46 ++++++++++++-- backend/src/notes/notes.service.ts | 73 ++++++++++++++++++---- backend/test/private-api/notes.e2e-spec.ts | 15 ++++- backend/test/public-api/notes.e2e-spec.ts | 16 ++++- 6 files changed, 170 insertions(+), 75 deletions(-) diff --git a/backend/src/notes/alias.service.spec.ts b/backend/src/notes/alias.service.spec.ts index bf5674268..876990a8f 100644 --- a/backend/src/notes/alias.service.spec.ts +++ b/backend/src/notes/alias.service.spec.ts @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022 The HedgeDoc developers (see AUTHORS file) + * SPDX-FileCopyrightText: 2024 The HedgeDoc developers (see AUTHORS file) * * SPDX-License-Identifier: AGPL-3.0-only */ @@ -60,6 +60,16 @@ describe('AliasService', () => { ), undefined, ); + aliasRepo = new Repository( + '', + new EntityManager( + new DataSource({ + type: 'sqlite', + database: ':memory:', + }), + ), + undefined, + ); const module: TestingModule = await Test.createTestingModule({ providers: [ AliasService, @@ -70,7 +80,7 @@ describe('AliasService', () => { }, { provide: getRepositoryToken(Alias), - useClass: Repository, + useValue: aliasRepo, }, { provide: getRepositoryToken(Tag), @@ -105,7 +115,7 @@ describe('AliasService', () => { .overrideProvider(getRepositoryToken(Tag)) .useClass(Repository) .overrideProvider(getRepositoryToken(Alias)) - .useClass(Repository) + .useValue(aliasRepo) .overrideProvider(getRepositoryToken(User)) .useClass(Repository) .overrideProvider(getRepositoryToken(AuthToken)) @@ -144,8 +154,8 @@ describe('AliasService', () => { jest .spyOn(noteRepo, 'save') .mockImplementationOnce(async (note: Note): Promise => note); - jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(null); - jest.spyOn(aliasRepo, 'findOne').mockResolvedValueOnce(null); + jest.spyOn(noteRepo, 'existsBy').mockResolvedValueOnce(false); + jest.spyOn(aliasRepo, 'existsBy').mockResolvedValueOnce(false); const savedAlias = await service.addAlias(note, alias); expect(savedAlias.name).toEqual(alias); expect(savedAlias.primary).toBeTruthy(); @@ -155,8 +165,8 @@ describe('AliasService', () => { jest .spyOn(noteRepo, 'save') .mockImplementationOnce(async (note: Note): Promise => note); - jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(null); - jest.spyOn(aliasRepo, 'findOne').mockResolvedValueOnce(null); + jest.spyOn(noteRepo, 'existsBy').mockResolvedValueOnce(false); + jest.spyOn(aliasRepo, 'existsBy').mockResolvedValueOnce(false); const savedAlias = await service.addAlias(note, alias2); expect(savedAlias.name).toEqual(alias2); expect(savedAlias.primary).toBeFalsy(); @@ -165,9 +175,8 @@ describe('AliasService', () => { describe('does not create an alias', () => { const note = Note.create(user, alias2) as Note; it('with an already used name', async () => { - jest - .spyOn(aliasRepo, 'findOne') - .mockResolvedValueOnce(Alias.create(alias2, note, false) as Alias); + jest.spyOn(noteRepo, 'existsBy').mockResolvedValueOnce(false); + jest.spyOn(aliasRepo, 'existsBy').mockResolvedValueOnce(true); await expect(service.addAlias(note, alias2)).rejects.toThrow( AlreadyInDBError, ); @@ -254,7 +263,7 @@ describe('AliasService', () => { it('mark the alias as primary', async () => { jest - .spyOn(aliasRepo, 'findOneBy') + .spyOn(aliasRepo, 'findOneByOrFail') .mockResolvedValueOnce(alias) .mockResolvedValueOnce(alias2); jest diff --git a/backend/src/notes/alias.service.ts b/backend/src/notes/alias.service.ts index f3fa588d7..2b2a7e055 100644 --- a/backend/src/notes/alias.service.ts +++ b/backend/src/notes/alias.service.ts @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022 The HedgeDoc developers (see AUTHORS file) + * SPDX-FileCopyrightText: 2024 The HedgeDoc developers (see AUTHORS file) * * SPDX-License-Identifier: AGPL-3.0-only */ @@ -8,7 +8,6 @@ import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; import { - AlreadyInDBError, NotInDBError, PrimaryAliasDeletionForbiddenError, } from '../errors/errors'; @@ -40,28 +39,8 @@ export class AliasService { * @return {Alias} the new alias */ async addAlias(note: Note, alias: string): Promise { - this.notesService.checkNoteIdOrAlias(alias); + await this.notesService.ensureNoteIdOrAliasIsAvailable(alias); - const foundAlias = await this.aliasRepository.findOne({ - where: { name: alias }, - }); - if (foundAlias !== null) { - this.logger.debug(`The alias '${alias}' is already used.`, 'addAlias'); - throw new AlreadyInDBError(`The alias '${alias}' is already used.`); - } - - const foundNote = await this.noteRepository.findOne({ - where: { publicId: alias }, - }); - if (foundNote !== null) { - this.logger.debug( - `The alias '${alias}' is already a public id.`, - 'addAlias', - ); - throw new AlreadyInDBError( - `The alias '${alias}' is already a public id.`, - ); - } let newAlias; if ((await note.aliases).length === 0) { // the first alias is automatically made the primary alias @@ -89,8 +68,6 @@ export class AliasService { let oldPrimaryId = 0; let newPrimaryId = 0; - this.notesService.checkNoteIdOrAlias(alias); - for (const anAlias of await note.aliases) { // found old primary if (anAlias.primary) { @@ -113,17 +90,13 @@ export class AliasService { throw new NotInDBError(`The alias '${alias}' is not used by this note.`); } - const oldPrimary = await this.aliasRepository.findOneBy({ + const oldPrimary = await this.aliasRepository.findOneByOrFail({ id: oldPrimaryId, }); - const newPrimary = await this.aliasRepository.findOneBy({ + const newPrimary = await this.aliasRepository.findOneByOrFail({ id: newPrimaryId, }); - if (!oldPrimary || !newPrimary) { - throw new Error('This should not happen!'); - } - oldPrimary.primary = false; newPrimary.primary = true; @@ -143,10 +116,10 @@ export class AliasService { * @throws {PrimaryAliasDeletionForbiddenError} the primary alias can only be deleted if it's the only alias */ async removeAlias(note: Note, alias: string): Promise { - this.notesService.checkNoteIdOrAlias(alias); const primaryAlias = await getPrimaryAlias(note); + const noteAliases = await note.aliases; - if (primaryAlias === alias && (await note.aliases).length !== 1) { + if (primaryAlias === alias && noteAliases.length !== 1) { this.logger.debug( `The alias '${alias}' is the primary alias, which can only be removed if it's the only alias.`, 'removeAlias', @@ -156,10 +129,20 @@ export class AliasService { ); } - const filteredAliases = (await note.aliases).filter( - (anAlias) => anAlias.name !== alias, - ); - if ((await note.aliases).length === filteredAliases.length) { + const filteredAliases: Alias[] = []; + let aliasToDelete: Alias | null = null; + let aliasFound = false; + + for (const anAlias of noteAliases) { + if (anAlias.name === alias) { + aliasFound = true; + aliasToDelete = anAlias; + } else { + filteredAliases.push(anAlias); + } + } + + if (!aliasFound) { this.logger.debug( `The alias '${alias}' is not used by this note or is the primary alias, which can't be removed.`, 'removeAlias', @@ -168,12 +151,11 @@ export class AliasService { `The alias '${alias}' is not used by this note or is the primary alias, which can't be removed.`, ); } - const aliasToDelete = (await note.aliases).find( - (anAlias) => anAlias.name === alias, - ); - if (aliasToDelete !== undefined) { + + if (aliasToDelete !== null) { await this.aliasRepository.remove(aliasToDelete); } + note.aliases = Promise.resolve(filteredAliases); return await this.noteRepository.save(note); } diff --git a/backend/src/notes/notes.service.spec.ts b/backend/src/notes/notes.service.spec.ts index 73663d826..e10d126a8 100644 --- a/backend/src/notes/notes.service.spec.ts +++ b/backend/src/notes/notes.service.spec.ts @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022 The HedgeDoc developers (see AUTHORS file) + * SPDX-FileCopyrightText: 2024 The HedgeDoc developers (see AUTHORS file) * * SPDX-License-Identifier: AGPL-3.0-only */ @@ -63,6 +63,7 @@ describe('NotesService', () => { const noteMockConfig: NoteConfig = createDefaultMockNoteConfig(); let noteRepo: Repository; let userRepo: Repository; + let aliasRepo: Repository; let groupRepo: Repository; let forbiddenNoteId: string; let everyoneDefaultAccessPermission: string; @@ -108,6 +109,16 @@ describe('NotesService', () => { ), undefined, ); + aliasRepo = new Repository( + '', + new EntityManager( + new DataSource({ + type: 'sqlite', + database: ':memory:', + }), + ), + undefined, + ); groupRepo = new Repository( '', new EntityManager( @@ -146,7 +157,7 @@ describe('NotesService', () => { }, { provide: getRepositoryToken(Alias), - useClass: Repository, + useValue: aliasRepo, }, { provide: getRepositoryToken(User), @@ -180,7 +191,7 @@ describe('NotesService', () => { .overrideProvider(getRepositoryToken(Tag)) .useClass(Repository) .overrideProvider(getRepositoryToken(Alias)) - .useClass(Repository) + .useValue(aliasRepo) .overrideProvider(getRepositoryToken(User)) .useValue(userRepo) .overrideProvider(getRepositoryToken(AuthToken)) @@ -210,6 +221,7 @@ describe('NotesService', () => { loggedinDefaultAccessPermission = noteConfig.permissions.default.loggedIn; service = module.get(NotesService); noteRepo = module.get>(getRepositoryToken(Note)); + aliasRepo = module.get>(getRepositoryToken(Alias)); eventEmitter = module.get(EventEmitter2); }); @@ -354,11 +366,15 @@ describe('NotesService', () => { // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore .mockImplementation(async (note: Note): Promise => note); + jest.spyOn(noteRepo, 'existsBy').mockResolvedValueOnce(false); + jest.spyOn(aliasRepo, 'existsBy').mockResolvedValueOnce(false); mockGroupRepo(); createRevisionSpy = jest .spyOn(revisionsService, 'createRevision') .mockResolvedValue(newRevision); + + mockSelectQueryBuilderInRepo(noteRepo, null); }); it('without alias, without owner', async () => { const newNote = await service.createNote(content, null); @@ -528,14 +544,34 @@ describe('NotesService', () => { }); }); describe('fails:', () => { + beforeEach(() => { + mockSelectQueryBuilderInRepo(noteRepo, null); + }); + it('alias is forbidden', async () => { + jest.spyOn(noteRepo, 'existsBy').mockResolvedValueOnce(false); + jest.spyOn(aliasRepo, 'existsBy').mockResolvedValueOnce(false); await expect( service.createNote(content, null, forbiddenNoteId), ).rejects.toThrow(ForbiddenIdError); }); - it('alias is already used', async () => { + it('alias is already used (as another alias)', async () => { mockGroupRepo(); + jest.spyOn(noteRepo, 'existsBy').mockResolvedValueOnce(false); + jest.spyOn(aliasRepo, 'existsBy').mockResolvedValueOnce(true); + jest.spyOn(noteRepo, 'save').mockImplementationOnce(async () => { + throw new Error(); + }); + await expect(service.createNote(content, null, alias)).rejects.toThrow( + AlreadyInDBError, + ); + }); + + it('alias is already used (as publicId)', async () => { + mockGroupRepo(); + jest.spyOn(noteRepo, 'existsBy').mockResolvedValueOnce(true); + jest.spyOn(aliasRepo, 'existsBy').mockResolvedValueOnce(false); jest.spyOn(noteRepo, 'save').mockImplementationOnce(async () => { throw new Error(); }); @@ -547,6 +583,8 @@ describe('NotesService', () => { beforeEach(() => (noteMockConfig.maxDocumentLength = 1000)); it('document is too long', async () => { mockGroupRepo(); + jest.spyOn(noteRepo, 'existsBy').mockResolvedValueOnce(false); + jest.spyOn(aliasRepo, 'existsBy').mockResolvedValueOnce(false); jest.spyOn(noteRepo, 'save').mockImplementationOnce(async () => { throw new Error(); }); diff --git a/backend/src/notes/notes.service.ts b/backend/src/notes/notes.service.ts index 43b1521a2..0bc8e3691 100644 --- a/backend/src/notes/notes.service.ts +++ b/backend/src/notes/notes.service.ts @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022 The HedgeDoc developers (see AUTHORS file) + * SPDX-FileCopyrightText: 2024 The HedgeDoc developers (see AUTHORS file) * * SPDX-License-Identifier: AGPL-3.0-only */ @@ -93,7 +93,7 @@ export class NotesService { ): Promise { // Check if new note doesn't violate application constraints if (alias) { - this.checkNoteIdOrAlias(alias); + await this.ensureNoteIdOrAliasIsAvailable(alias); } if (noteContent.length > this.noteConfig.maxDocumentLength) { throw new MaximumDocumentLengthExceededError(); @@ -201,13 +201,18 @@ export class NotesService { * @throws {ForbiddenIdError} the requested id or alias is forbidden */ async getNoteByIdOrAlias(noteIdOrAlias: string): Promise { + const isForbidden = this.noteIdOrAliasIsForbidden(noteIdOrAlias); + if (isForbidden) { + throw new ForbiddenIdError( + `The note id or alias '${noteIdOrAlias}' is forbidden by the administrator.`, + ); + } + this.logger.debug( `Trying to find note '${noteIdOrAlias}'`, 'getNoteByIdOrAlias', ); - this.checkNoteIdOrAlias(noteIdOrAlias); - /** * This query gets the note's aliases, owner, groupPermissions (and the groups), userPermissions (and the users) and tags and * then only gets the note, that either has a publicId :noteIdOrAlias or has any alias with this name. @@ -263,20 +268,62 @@ export class NotesService { } /** - * Check if the provided note id or alias is not forbidden + * Check if the provided note id or alias is available for notes * @param noteIdOrAlias - the alias or id in question - * @throws {ForbiddenIdError} the requested id or alias is forbidden + * @throws {ForbiddenIdError} the requested id or alias is not available */ - checkNoteIdOrAlias(noteIdOrAlias: string): void { - if (this.noteConfig.forbiddenNoteIds.includes(noteIdOrAlias)) { - this.logger.debug( - `A note with the alias '${noteIdOrAlias}' is forbidden by the administrator.`, - 'checkNoteIdOrAlias', - ); + async ensureNoteIdOrAliasIsAvailable(noteIdOrAlias: string): Promise { + const isForbidden = this.noteIdOrAliasIsForbidden(noteIdOrAlias); + if (isForbidden) { throw new ForbiddenIdError( - `A note with the alias '${noteIdOrAlias}' is forbidden by the administrator.`, + `The note id or alias '${noteIdOrAlias}' is forbidden by the administrator.`, ); } + const isUsed = await this.noteIdOrAliasIsUsed(noteIdOrAlias); + if (isUsed) { + throw new AlreadyInDBError( + `A note with the id or alias '${noteIdOrAlias}' already exists.`, + ); + } + } + + /** + * Check if the provided note id or alias is forbidden + * @param noteIdOrAlias - the alias or id in question + * @return {boolean} true if the id or alias is forbidden, false otherwise + */ + noteIdOrAliasIsForbidden(noteIdOrAlias: string): boolean { + const forbidden = this.noteConfig.forbiddenNoteIds.includes(noteIdOrAlias); + if (forbidden) { + this.logger.debug( + `A note with the alias '${noteIdOrAlias}' is forbidden by the administrator.`, + 'noteIdOrAliasIsForbidden', + ); + } + return forbidden; + } + + /** + * @async + * Check if the provided note id or alias is already used + * @param noteIdOrAlias - the alias or id in question + * @return {boolean} true if the id or alias is already used, false otherwise + */ + async noteIdOrAliasIsUsed(noteIdOrAlias: string): Promise { + const noteWithPublicIdExists = await this.noteRepository.existsBy({ + publicId: noteIdOrAlias, + }); + const noteWithAliasExists = await this.aliasRepository.existsBy({ + name: noteIdOrAlias, + }); + if (noteWithPublicIdExists || noteWithAliasExists) { + this.logger.debug( + `A note with the id or alias '${noteIdOrAlias}' already exists.`, + 'noteIdOrAliasIsUsed', + ); + return true; + } + return false; } /** diff --git a/backend/test/private-api/notes.e2e-spec.ts b/backend/test/private-api/notes.e2e-spec.ts index 8dc321076..9d8e5dce8 100644 --- a/backend/test/private-api/notes.e2e-spec.ts +++ b/backend/test/private-api/notes.e2e-spec.ts @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022 The HedgeDoc developers (see AUTHORS file) + * SPDX-FileCopyrightText: 2024 The HedgeDoc developers (see AUTHORS file) * * SPDX-License-Identifier: AGPL-3.0-only */ @@ -25,7 +25,7 @@ describe('Notes', () => { let agent: request.SuperAgentTest; beforeAll(async () => { - testSetup = await TestSetupBuilder.create().build(); + testSetup = await TestSetupBuilder.create().withNotes().build(); forbiddenNoteId = testSetup.configService.get('noteConfig').forbiddenNoteIds[0]; @@ -139,12 +139,21 @@ describe('Notes', () => { .maxDocumentLength as number) + 1, ); await agent - .post('/api/private/notes/test2') + .post('/api/private/notes/test3') .set('Content-Type', 'text/markdown') .send(content) .expect('Content-Type', /json/) .expect(413); }); + + it('cannot create an alias equal to a note publicId', async () => { + await agent + .post(`/api/private/notes/${testSetup.anonymousNotes[0].publicId}`) + .set('Content-Type', 'text/markdown') + .send(content) + .expect('Content-Type', /json/) + .expect(409); + }); }); describe('DELETE /notes/{note}', () => { diff --git a/backend/test/public-api/notes.e2e-spec.ts b/backend/test/public-api/notes.e2e-spec.ts index d7aafb489..514b212fb 100644 --- a/backend/test/public-api/notes.e2e-spec.ts +++ b/backend/test/public-api/notes.e2e-spec.ts @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022 The HedgeDoc developers (see AUTHORS file) + * SPDX-FileCopyrightText: 2024 The HedgeDoc developers (see AUTHORS file) * * SPDX-License-Identifier: AGPL-3.0-only */ @@ -20,7 +20,7 @@ describe('Notes', () => { let testImage: Buffer; beforeAll(async () => { - testSetup = await TestSetupBuilder.create().withUsers().build(); + testSetup = await TestSetupBuilder.create().withUsers().withNotes().build(); forbiddenNoteId = testSetup.configService.get('noteConfig').forbiddenNoteIds[0]; @@ -129,13 +129,23 @@ describe('Notes', () => { .maxDocumentLength as number) + 1, ); await request(testSetup.app.getHttpServer()) - .post('/api/v2/notes/test2') + .post('/api/v2/notes/test3') .set('Authorization', `Bearer ${testSetup.authTokens[0].secret}`) .set('Content-Type', 'text/markdown') .send(content) .expect('Content-Type', /json/) .expect(413); }); + + it('cannot create an alias equal to a note publicId', async () => { + await request(testSetup.app.getHttpServer()) + .post(`/api/v2/notes/${testSetup.anonymousNotes[0].publicId}`) + .set('Authorization', `Bearer ${testSetup.authTokens[0].secret}`) + .set('Content-Type', 'text/markdown') + .send(content) + .expect('Content-Type', /json/) + .expect(409); + }); }); describe('DELETE /notes/{note}', () => {