From 17b442aff1dc333e67cd663bb95f3c9b58573c8b Mon Sep 17 00:00:00 2001 From: Philip Molares Date: Thu, 1 Apr 2021 01:15:44 +0200 Subject: [PATCH 1/5] Notes: Add NoteMediaDeletionDto This is used to specify if the media uploads should be kept or deleted, when deleting a note. Signed-off-by: Philip Molares --- src/notes/note.media-deletion.dto.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 src/notes/note.media-deletion.dto.ts diff --git a/src/notes/note.media-deletion.dto.ts b/src/notes/note.media-deletion.dto.ts new file mode 100644 index 000000000..0779fdbce --- /dev/null +++ b/src/notes/note.media-deletion.dto.ts @@ -0,0 +1,18 @@ +/* + * SPDX-FileCopyrightText: 2021 The HedgeDoc developers (see AUTHORS file) + * + * SPDX-License-Identifier: AGPL-3.0-only + */ +import { IsBoolean } from 'class-validator'; +import { ApiProperty } from '@nestjs/swagger'; + +export class NoteMediaDeletionDto { + /** + * Should the associated mediaUploads be keept + * @default false + * @example false + */ + @IsBoolean() + @ApiProperty() + keepMedia: boolean; +} From e7c9a214dfbb2ede82fee432dbb5da442afb5ac7 Mon Sep 17 00:00:00 2001 From: Philip Molares Date: Thu, 1 Apr 2021 01:17:09 +0200 Subject: [PATCH 2/5] MediaUpload: Make note nullable As it is possible to delete a note without also deleting the associated media uploads this needs to changed in the media upload entity, too. Signed-off-by: Philip Molares --- src/media/media-upload.entity.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/media/media-upload.entity.ts b/src/media/media-upload.entity.ts index d25094d3b..341ad52ad 100644 --- a/src/media/media-upload.entity.ts +++ b/src/media/media-upload.entity.ts @@ -24,7 +24,7 @@ export class MediaUpload { id: string; @ManyToOne((_) => Note, (note) => note.mediaUploads, { - nullable: false, + nullable: true, }) note: Note; From c29ce7eed55c6774abeb56186651d3ba66640948 Mon Sep 17 00:00:00 2001 From: Philip Molares Date: Thu, 1 Apr 2021 01:18:24 +0200 Subject: [PATCH 3/5] MediaService: Add removeNoteFromMediaUpload method This method replaces the associated note of a media upload with null. Signed-off-by: Philip Molares --- src/media/media.service.spec.ts | 23 +++++++++++++++++++++++ src/media/media.service.ts | 14 ++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/src/media/media.service.spec.ts b/src/media/media.service.spec.ts index e7acf22b0..8664ab8a7 100644 --- a/src/media/media.service.spec.ts +++ b/src/media/media.service.spec.ts @@ -265,4 +265,27 @@ describe('MediaService', () => { }); }); }); + + describe('removeNoteFromMediaUpload', () => { + it('works', async () => { + const mockMediaUploadEntry = { + id: 'testMediaUpload', + backendData: 'testBackendData', + note: { + alias: 'test', + } as Note, + user: { + userName: 'hardcoded', + } as User, + } as MediaUpload; + jest + .spyOn(mediaRepo, 'save') + .mockImplementationOnce(async (entry: MediaUpload) => { + expect(entry.note).toBeNull(); + return entry; + }); + await service.removeNoteFromMediaUpload(mockMediaUploadEntry); + expect(mediaRepo.save).toHaveBeenCalled(); + }); + }); }); diff --git a/src/media/media.service.ts b/src/media/media.service.ts index b0a25442d..259c94829 100644 --- a/src/media/media.service.ts +++ b/src/media/media.service.ts @@ -176,6 +176,20 @@ export class MediaService { return mediaUploads; } + /** + * @async + * Set the note of a mediaUpload to null + * @param {MediaUpload} mediaUpload - the media upload to be changed + */ + async removeNoteFromMediaUpload(mediaUpload: MediaUpload): Promise { + this.logger.debug( + 'Setting note to null for mediaUpload: ' + mediaUpload.id, + 'removeNoteFromMediaUpload', + ); + mediaUpload.note = null; + await this.mediaUploadRepository.save(mediaUpload); + } + private chooseBackendType(): BackendType { switch (this.mediaConfig.backend.use) { case 'filesystem': From 6ac267a2263c9d43176af01084aa952954ad8b23 Mon Sep 17 00:00:00 2001 From: Philip Molares Date: Thu, 1 Apr 2021 01:22:34 +0200 Subject: [PATCH 4/5] PrivateApi: Add option to keep media to DELETE /notes/{note} This adds a body to the route DELETE /notes/{note} of the private api to specify if the associated media uploads of the note should be kept or deleted. Signed-off-by: Philip Molares --- src/api/private/notes/notes.controller.ts | 11 +++++ test/private-api/notes.e2e-spec.ts | 51 ++++++++++++++++++++--- 2 files changed, 56 insertions(+), 6 deletions(-) diff --git a/src/api/private/notes/notes.controller.ts b/src/api/private/notes/notes.controller.ts index 752a150bc..bd9f1ab35 100644 --- a/src/api/private/notes/notes.controller.ts +++ b/src/api/private/notes/notes.controller.ts @@ -6,6 +6,7 @@ import { BadRequestException, + Body, Controller, Delete, Get, @@ -33,6 +34,7 @@ import { RevisionMetadataDto } from '../../../revisions/revision-metadata.dto'; import { RevisionDto } from '../../../revisions/revision.dto'; import { RevisionsService } from '../../../revisions/revisions.service'; import { MarkdownBody } from '../../utils/markdownbody-decorator'; +import { NoteMediaDeletionDto } from '../../../notes/note.media-deletion.dto'; @Controller('notes') export class NotesController { @@ -140,6 +142,7 @@ export class NotesController { @HttpCode(204) async deleteNote( @Param('noteIdOrAlias') noteIdOrAlias: string, + @Body() noteMediaDeletionDto: NoteMediaDeletionDto, ): Promise { try { // ToDo: use actual user here @@ -148,6 +151,14 @@ export class NotesController { if (!this.permissionsService.isOwner(user, note)) { throw new UnauthorizedException('Deleting note denied!'); } + const mediaUploads = await this.mediaService.listUploadsByNote(note); + for (const mediaUpload of mediaUploads) { + if (!noteMediaDeletionDto.keepMedia) { + await this.mediaService.deleteFile(mediaUpload); + } else { + await this.mediaService.removeNoteFromMediaUpload(mediaUpload); + } + } this.logger.debug('Deleting note: ' + noteIdOrAlias, 'deleteNote'); await this.noteService.deleteNote(note); this.logger.debug('Successfully deleted ' + noteIdOrAlias, 'deleteNote'); diff --git a/test/private-api/notes.e2e-spec.ts b/test/private-api/notes.e2e-spec.ts index e8fc2fa3c..1c29682a2 100644 --- a/test/private-api/notes.e2e-spec.ts +++ b/test/private-api/notes.e2e-spec.ts @@ -38,6 +38,7 @@ describe('Notes', () => { let content: string; let forbiddenNoteId: string; let uploadPath: string; + let testImage: Buffer; beforeAll(async () => { const moduleRef = await Test.createTestingModule({ @@ -80,6 +81,7 @@ describe('Notes', () => { user = await userService.createUser('hardcoded', 'Testy'); user2 = await userService.createUser('hardcoded2', 'Max Mustermann'); content = 'This is a test note.'; + testImage = await fs.readFile('test/public-api/fixtures/test.png'); }); it('POST /notes', async () => { @@ -152,12 +154,49 @@ describe('Notes', () => { }); describe('DELETE /notes/{note}', () => { - it('works with an existing alias', async () => { - await notesService.createNote(content, 'test3', user); - await request(app.getHttpServer()).delete('/notes/test3').expect(204); - await expect(notesService.getNoteByIdOrAlias('test3')).rejects.toEqual( - new NotInDBError("Note with id/alias 'test3' not found."), - ); + describe('works', () => { + it('with an existing alias and keepMedia false', async () => { + const noteId = 'test3'; + await notesService.createNote(content, noteId, user); + await mediaService.saveFile(testImage, user.userName, noteId); + await request(app.getHttpServer()) + .delete(`/notes/${noteId}`) + .set('Content-Type', 'application/json') + .send({ + keepMedia: false, + }) + .expect(204); + await expect(notesService.getNoteByIdOrAlias(noteId)).rejects.toEqual( + new NotInDBError(`Note with id/alias '${noteId}' not found.`), + ); + expect(await mediaService.listUploadsByUser(user)).toHaveLength(0); + await fs.rmdir(uploadPath); + }); + it('with an existing alias and keepMedia true', async () => { + const noteId = 'test3a'; + await notesService.createNote(content, noteId, user); + const url = await mediaService.saveFile( + testImage, + user.userName, + noteId, + ); + await request(app.getHttpServer()) + .delete(`/notes/${noteId}`) + .set('Content-Type', 'application/json') + .send({ + keepMedia: true, + }) + .expect(204); + await expect(notesService.getNoteByIdOrAlias(noteId)).rejects.toEqual( + new NotInDBError(`Note with id/alias '${noteId}' not found.`), + ); + expect(await mediaService.listUploadsByUser(user)).toHaveLength(1); + // Remove /upload/ from path as we just need the filename. + const fileName = url.replace('/uploads/', ''); + // delete the file afterwards + await fs.unlink(join(uploadPath, fileName)); + await fs.rmdir(uploadPath); + }); }); it('fails with a forbidden alias', async () => { await request(app.getHttpServer()) From 1f897636bb76ed6d49e16f2f1ef5186893c6eff9 Mon Sep 17 00:00:00 2001 From: Philip Molares Date: Thu, 1 Apr 2021 01:23:12 +0200 Subject: [PATCH 5/5] PublicApi: Add option to keep media to DELETE /notes/{note} This adds a body to the route DELETE /notes/{note} of the public api to specify if the associated media uploads of the note should be kept or deleted. Signed-off-by: Philip Molares --- src/api/public/notes/notes.controller.ts | 10 +++++ test/public-api/notes.e2e-spec.ts | 49 +++++++++++++++++++++--- 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/src/api/public/notes/notes.controller.ts b/src/api/public/notes/notes.controller.ts index cf0a905b9..f32ba7d02 100644 --- a/src/api/public/notes/notes.controller.ts +++ b/src/api/public/notes/notes.controller.ts @@ -62,6 +62,7 @@ import { } from '../../utils/descriptions'; import { MediaUploadDto } from '../../../media/media-upload.dto'; import { MediaService } from '../../../media/media.service'; +import { NoteMediaDeletionDto } from '../../../notes/note.media-deletion.dto'; @ApiTags('notes') @ApiSecurity('token') @@ -172,12 +173,21 @@ export class NotesController { async deleteNote( @Req() req: Request, @Param('noteIdOrAlias') noteIdOrAlias: string, + @Body() noteMediaDeletionDto: NoteMediaDeletionDto, ): Promise { try { const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); if (!this.permissionsService.isOwner(req.user, note)) { throw new UnauthorizedException('Deleting note denied!'); } + const mediaUploads = await this.mediaService.listUploadsByNote(note); + for (const mediaUpload of mediaUploads) { + if (!noteMediaDeletionDto.keepMedia) { + await this.mediaService.deleteFile(mediaUpload); + } else { + await this.mediaService.removeNoteFromMediaUpload(mediaUpload); + } + } this.logger.debug('Deleting note: ' + noteIdOrAlias, 'deleteNote'); await this.noteService.deleteNote(note); this.logger.debug('Successfully deleted ' + noteIdOrAlias, 'deleteNote'); diff --git a/test/public-api/notes.e2e-spec.ts b/test/public-api/notes.e2e-spec.ts index 63c5e661d..456d41c3c 100644 --- a/test/public-api/notes.e2e-spec.ts +++ b/test/public-api/notes.e2e-spec.ts @@ -38,6 +38,7 @@ describe('Notes', () => { let content: string; let forbiddenNoteId: string; let uploadPath: string; + let testImage: Buffer; beforeAll(async () => { const moduleRef = await Test.createTestingModule({ @@ -77,6 +78,7 @@ describe('Notes', () => { user = await userService.createUser('hardcoded', 'Testy'); user2 = await userService.createUser('hardcoded2', 'Max Mustermann'); content = 'This is a test note.'; + testImage = await fs.readFile('test/public-api/fixtures/test.png'); }); it('POST /notes', async () => { @@ -149,12 +151,47 @@ describe('Notes', () => { }); describe('DELETE /notes/{note}', () => { - it('works with an existing alias', async () => { - await notesService.createNote(content, 'test3', user); - await request(app.getHttpServer()).delete('/notes/test3').expect(204); - await expect(notesService.getNoteByIdOrAlias('test3')).rejects.toEqual( - new NotInDBError("Note with id/alias 'test3' not found."), - ); + describe('works', () => { + it('with an existing alias and keepMedia false', async () => { + const noteId = 'test3'; + await notesService.createNote(content, noteId, user); + await mediaService.saveFile(testImage, user.userName, noteId); + await request(app.getHttpServer()) + .delete(`/notes/${noteId}`) + .set('Content-Type', 'application/json') + .send({ + keepMedia: false, + }) + .expect(204); + await expect(notesService.getNoteByIdOrAlias(noteId)).rejects.toEqual( + new NotInDBError(`Note with id/alias '${noteId}' not found.`), + ); + expect(await mediaService.listUploadsByUser(user)).toHaveLength(0); + }); + it('with an existing alias and keepMedia true', async () => { + const noteId = 'test3a'; + await notesService.createNote(content, noteId, user); + const url = await mediaService.saveFile( + testImage, + user.userName, + noteId, + ); + await request(app.getHttpServer()) + .delete(`/notes/${noteId}`) + .set('Content-Type', 'application/json') + .send({ + keepMedia: true, + }) + .expect(204); + await expect(notesService.getNoteByIdOrAlias(noteId)).rejects.toEqual( + new NotInDBError(`Note with id/alias '${noteId}' not found.`), + ); + expect(await mediaService.listUploadsByUser(user)).toHaveLength(1); + // Remove /upload/ from path as we just need the filename. + const fileName = url.replace('/uploads/', ''); + // delete the file afterwards + await fs.unlink(join(uploadPath, fileName)); + }); }); it('works with an existing alias with permissions', async () => { const note = await notesService.createNote(content, 'test3', user);