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 <git@herrmehren.de>
Signed-off-by: Erik Michelson <github@erik.michelson.eu>

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<Alias>(
+      '',
+      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> => note);
-        jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(null);
+        jest.spyOn(noteRepo, 'existsBy').mockResolvedValueOnce(false);
-        jest.spyOn(aliasRepo, 'findOne').mockResolvedValueOnce(null);
+        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> => note);
-        jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(null);
+        jest.spyOn(noteRepo, 'existsBy').mockResolvedValueOnce(false);
-        jest.spyOn(aliasRepo, 'findOne').mockResolvedValueOnce(null);
+        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
+        jest.spyOn(noteRepo, 'existsBy').mockResolvedValueOnce(false);
-          .spyOn(aliasRepo, 'findOne')
+        jest.spyOn(aliasRepo, 'existsBy').mockResolvedValueOnce(true);
-          .mockResolvedValueOnce(Alias.create(alias2, note, false) as Alias);
         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

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<Alias> {
-    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<Note> {
-    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(
+    const filteredAliases: Alias[] = [];
-      (anAlias) => anAlias.name !== alias,
+    let aliasToDelete: Alias | null = null;
-    );
+    let aliasFound = false;
-    if ((await note.aliases).length === filteredAliases.length) {
+
+    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 !== null) {
-    );
-    if (aliasToDelete !== undefined) {
       await this.aliasRepository.remove(aliasToDelete);
     }
+
     note.aliases = Promise.resolve(filteredAliases);
     return await this.noteRepository.save(note);
   }

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<Note>;
   let userRepo: Repository<User>;
+  let aliasRepo: Repository<Alias>;
   let groupRepo: Repository<Group>;
   let forbiddenNoteId: string;
   let everyoneDefaultAccessPermission: string;
@@ -108,6 +109,16 @@ describe('NotesService', () => {
       ),
       undefined,
     );
+    aliasRepo = new Repository<Alias>(
+      '',
+      new EntityManager(
+        new DataSource({
+          type: 'sqlite',
+          database: ':memory:',
+        }),
+      ),
+      undefined,
+    );
     groupRepo = new Repository<Group>(
       '',
       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>(NotesService);
     noteRepo = module.get<Repository<Note>>(getRepositoryToken(Note));
+    aliasRepo = module.get<Repository<Alias>>(getRepositoryToken(Alias));
     eventEmitter = module.get<EventEmitter2>(EventEmitter2);
   });
 
@@ -354,11 +366,15 @@ describe('NotesService', () => {
           // eslint-disable-next-line @typescript-eslint/ban-ts-comment
           // @ts-ignore
           .mockImplementation(async (note: Note): Promise<Note> => 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();
           });

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<Note> {
     // 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<Note> {
+    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 {
+  async ensureNoteIdOrAliasIsAvailable(noteIdOrAlias: string): Promise<void> {
-    if (this.noteConfig.forbiddenNoteIds.includes(noteIdOrAlias)) {
+    const isForbidden = this.noteIdOrAliasIsForbidden(noteIdOrAlias);
-      this.logger.debug(
+    if (isForbidden) {
-        `A note with the alias '${noteIdOrAlias}' is forbidden by the administrator.`,
-        'checkNoteIdOrAlias',
-      );
       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<boolean> {
+    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;
   }
 
   /**

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}', () => {

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}', () => {