From e6193d34327a209cbe7887ce55fed18546749d2c Mon Sep 17 00:00:00 2001 From: sszafGCA <116172610+sszafGCA@users.noreply.github.com> Date: Tue, 9 Jan 2024 12:06:45 +0100 Subject: [PATCH] BC-5832- Add deleting creator ID reference from the files entities (the legacy files, rewritten from FeathersJS) in the main user deletion use case (#4662) --- .../deletion/uc/deletion-request.uc.spec.ts | 8 +- .../deletion/uc/deletion-request.uc.ts | 4 +- .../modules/files/entity/file.entity.spec.ts | 44 ++++++++ .../src/modules/files/entity/file.entity.ts | 16 ++- .../src/modules/files/repo/files.repo.spec.ts | 53 +++++++-- .../src/modules/files/repo/files.repo.ts | 23 +++- .../files/service/files.service.spec.ts | 104 +++++++++++------- .../modules/files/service/files.service.ts | 13 ++- 8 files changed, 200 insertions(+), 65 deletions(-) diff --git a/apps/server/src/modules/deletion/uc/deletion-request.uc.spec.ts b/apps/server/src/modules/deletion/uc/deletion-request.uc.spec.ts index cd7f0a00443..9ebee1e77ef 100644 --- a/apps/server/src/modules/deletion/uc/deletion-request.uc.spec.ts +++ b/apps/server/src/modules/deletion/uc/deletion-request.uc.spec.ts @@ -218,7 +218,7 @@ describe(DeletionRequestUc.name, () => { courseGroupService.deleteUserDataFromCourseGroup.mockResolvedValueOnce(2); courseService.deleteUserDataFromCourse.mockResolvedValueOnce(2); filesService.markFilesOwnedByUserForDeletion.mockResolvedValueOnce(2); - filesService.removeUserPermissionsToAnyFiles.mockResolvedValueOnce(2); + filesService.removeUserPermissionsOrCreatorReferenceToAnyFiles.mockResolvedValueOnce(2); lessonService.deleteUserDataFromLessons.mockResolvedValueOnce(2); pseudonymService.deleteByUserId.mockResolvedValueOnce(2); teamService.deleteUserDataFromTeams.mockResolvedValueOnce(2); @@ -336,7 +336,9 @@ describe(DeletionRequestUc.name, () => { await uc.executeDeletionRequests(); - expect(filesService.removeUserPermissionsToAnyFiles).toHaveBeenCalledWith(deletionRequestToExecute.targetRefId); + expect(filesService.removeUserPermissionsOrCreatorReferenceToAnyFiles).toHaveBeenCalledWith( + deletionRequestToExecute.targetRefId + ); }); it('should call filesStorageClientAdapterService.removeCreatorIdFromFileRecords to remove cratorId to any files in fileRecords module', async () => { @@ -482,7 +484,7 @@ describe(DeletionRequestUc.name, () => { courseGroupService.deleteUserDataFromCourseGroup.mockResolvedValueOnce(2); courseService.deleteUserDataFromCourse.mockResolvedValueOnce(2); filesService.markFilesOwnedByUserForDeletion.mockResolvedValueOnce(2); - filesService.removeUserPermissionsToAnyFiles.mockResolvedValueOnce(2); + filesService.removeUserPermissionsOrCreatorReferenceToAnyFiles.mockResolvedValueOnce(2); lessonService.deleteUserDataFromLessons.mockResolvedValueOnce(2); pseudonymService.deleteByUserId.mockResolvedValueOnce(2); teamService.deleteUserDataFromTeams.mockResolvedValueOnce(2); diff --git a/apps/server/src/modules/deletion/uc/deletion-request.uc.ts b/apps/server/src/modules/deletion/uc/deletion-request.uc.ts index 97d2284a4cc..16973af5a2c 100644 --- a/apps/server/src/modules/deletion/uc/deletion-request.uc.ts +++ b/apps/server/src/modules/deletion/uc/deletion-request.uc.ts @@ -204,14 +204,14 @@ export class DeletionRequestUc { this.logger.debug({ action: 'removeUsersFilesAndPermissions', deletionRequest }); const filesDeleted: number = await this.filesService.markFilesOwnedByUserForDeletion(deletionRequest.targetRefId); - const filePermissionsUpdated: number = await this.filesService.removeUserPermissionsToAnyFiles( + const filesUpdated: number = await this.filesService.removeUserPermissionsOrCreatorReferenceToAnyFiles( deletionRequest.targetRefId ); await this.logDeletion( deletionRequest, DomainModel.FILE, DeletionOperationModel.UPDATE, - filesDeleted + filePermissionsUpdated, + filesDeleted + filesUpdated, 0 ); } diff --git a/apps/server/src/modules/files/entity/file.entity.spec.ts b/apps/server/src/modules/files/entity/file.entity.spec.ts index ea9649f4c66..bf52769f32b 100644 --- a/apps/server/src/modules/files/entity/file.entity.spec.ts +++ b/apps/server/src/modules/files/entity/file.entity.spec.ts @@ -94,6 +94,50 @@ describe(FileEntity.name, () => { }); }); + describe('removCreatorId', () => { + describe('when called on a file that contains matching creatorId', () => { + const setup = () => { + const file = fileEntityFactory.build({ + ownerId: mainUserId, + creatorId: mainUserId, + }); + + const expectedFile = copyFile(file); + expectedFile._creatorId = undefined; + + return { file, expectedFile }; + }; + it('should properly remove this creatorId', () => { + const { file, expectedFile } = setup(); + + file.removeCreatorId(mainUserId); + + expect(file).toEqual(expectedFile); + }); + }); + + describe("when called on a file that doesn't have any permission with given refId", () => { + const setup = () => { + const file = fileEntityFactory.build({ + ownerId: mainUserId, + creatorId: mainUserId, + }); + + const originalFile = copyFile(file); + + const randomUserId = new ObjectId().toHexString(); + return { file, originalFile, randomUserId }; + }; + it('should not modify the file in any way (including the other present permissions)', () => { + const { file, originalFile, randomUserId } = setup(); + + file.removeCreatorId(randomUserId); + + expect(file).toEqual(originalFile); + }); + }); + }); + describe('markForDeletion', () => { describe('when called on some typical file', () => { it('should properly mark the file for deletion', () => { diff --git a/apps/server/src/modules/files/entity/file.entity.ts b/apps/server/src/modules/files/entity/file.entity.ts index 68b667fe7c3..335e0f4d4b1 100644 --- a/apps/server/src/modules/files/entity/file.entity.ts +++ b/apps/server/src/modules/files/entity/file.entity.ts @@ -27,7 +27,7 @@ export interface FileEntityProps { parentId?: EntityId; ownerId: EntityId; refOwnerModel: FileOwnerModel; - creatorId: EntityId; + creatorId?: EntityId; permissions: FilePermissionEntity[]; lockId?: EntityId; versionKey?: number; @@ -95,12 +95,12 @@ export class FileEntity extends BaseEntityWithTimestamps { @Enum({ nullable: false }) refOwnerModel: FileOwnerModel; - @Property({ fieldName: 'creator' }) + @Property({ fieldName: 'creator', nullable: true }) @Index() - _creatorId: ObjectId; + _creatorId?: ObjectId; - get creatorId(): EntityId { - return this._creatorId.toHexString(); + get creatorId(): EntityId | undefined { + return this._creatorId?.toHexString(); } @Embedded(() => FilePermissionEntity, { array: true, nullable: false }) @@ -141,6 +141,12 @@ export class FileEntity extends BaseEntityWithTimestamps { return this.deleted && this.deletedAt !== undefined && !Number.isNaN(this.deletedAt.getTime()); } + public removeCreatorId(creatorId: EntityId): void { + if (creatorId === this._creatorId?.toHexString()) { + this._creatorId = undefined; + } + } + constructor(props: FileEntityProps) { super(); diff --git a/apps/server/src/modules/files/repo/files.repo.spec.ts b/apps/server/src/modules/files/repo/files.repo.spec.ts index d96fb27cf04..b6654384698 100644 --- a/apps/server/src/modules/files/repo/files.repo.spec.ts +++ b/apps/server/src/modules/files/repo/files.repo.spec.ts @@ -228,13 +228,19 @@ describe(FilesRepo.name, () => { }); }); - describe('findByPermissionRefId', () => { - describe('when searching for a files to which the user with given userId has access', () => { + describe('findByPermissionRefIdOrCreatorId', () => { + describe('when searching for a files to which the user with given userId has access or is creator', () => { const setup = async () => { const mainUserId = new ObjectId().toHexString(); const otherUserId = new ObjectId().toHexString(); // Test files created, owned and accessible only by the other user. + const otherUserFileWithMainUserCreator = fileEntityFactory.build({ + ownerId: otherUserId, + creatorId: mainUserId, + permissions: [filePermissionEntityFactory.build({ refId: otherUserId })], + }); + const otherUserFile = fileEntityFactory.build({ ownerId: otherUserId, creatorId: otherUserId, @@ -268,9 +274,35 @@ describe(FilesRepo.name, () => { permissions: [filePermissionEntityFactory.build({ refId: mainUserId })], }); - await em.persistAndFlush([otherUserFile, mainUserSharedFile, otherUserSharedFile, mainUserFile]); + await em.persistAndFlush([ + otherUserFileWithMainUserCreator, + mainUserSharedFile, + otherUserSharedFile, + mainUserFile, + otherUserFile, + ]); em.clear(); + const expectedOtherUserFileWithMainUserCreatorProps = { + id: otherUserFileWithMainUserCreator.id, + createdAt: otherUserFileWithMainUserCreator.createdAt, + updatedAt: otherUserFileWithMainUserCreator.updatedAt, + deleted: false, + isDirectory: false, + name: otherUserFileWithMainUserCreator.name, + size: otherUserFileWithMainUserCreator.size, + type: otherUserFileWithMainUserCreator.type, + storageFileName: otherUserFileWithMainUserCreator.storageFileName, + bucket: otherUserFileWithMainUserCreator.bucket, + thumbnail: otherUserFileWithMainUserCreator.thumbnail, + thumbnailRequestToken: otherUserFileWithMainUserCreator.thumbnailRequestToken, + securityCheck: otherUserFileWithMainUserCreator.securityCheck, + shareTokens: [], + refOwnerModel: otherUserFileWithMainUserCreator.refOwnerModel, + permissions: otherUserFileWithMainUserCreator.permissions, + versionKey: 0, + }; + const expectedMainUserSharedFileProps = { id: mainUserSharedFile.id, createdAt: mainUserSharedFile.createdAt, @@ -336,6 +368,8 @@ describe(FilesRepo.name, () => { mainUserSharedFile, otherUserSharedFile, mainUserFile, + otherUserFileWithMainUserCreator, + expectedOtherUserFileWithMainUserCreatorProps, expectedMainUserSharedFileProps, expectedOtherUserSharedFileProps, expectedMainUserFileProps, @@ -349,18 +383,21 @@ describe(FilesRepo.name, () => { mainUserSharedFile, otherUserSharedFile, mainUserFile, + otherUserFileWithMainUserCreator, expectedMainUserSharedFileProps, expectedOtherUserSharedFileProps, expectedMainUserFileProps, + expectedOtherUserFileWithMainUserCreatorProps, } = await setup(); - const results = await repo.findByPermissionRefId(mainUserId); + const results = await repo.findByPermissionRefIdOrCreatorId(mainUserId); - expect(results).toHaveLength(3); + expect(results).toHaveLength(4); // Verify explicit fields. expect(results).toEqual( expect.arrayContaining([ + expect.objectContaining(expectedOtherUserFileWithMainUserCreatorProps), expect.objectContaining(expectedMainUserSharedFileProps), expect.objectContaining(expectedOtherUserSharedFileProps), expect.objectContaining(expectedMainUserFileProps), @@ -370,6 +407,7 @@ describe(FilesRepo.name, () => { // Verify storage provider id. expect(results.map((result) => result.storageProvider?.id)).toEqual( expect.arrayContaining([ + otherUserFileWithMainUserCreator.storageProvider?.id, mainUserSharedFile.storageProvider?.id, otherUserSharedFile.storageProvider?.id, mainUserFile.storageProvider?.id, @@ -384,6 +422,7 @@ describe(FilesRepo.name, () => { // Verify implicit creatorId field. expect(results.map((result) => result.creatorId)).toEqual( expect.arrayContaining([ + otherUserFileWithMainUserCreator.creatorId, mainUserSharedFile.creatorId, otherUserSharedFile.creatorId, mainUserFile.creatorId, @@ -397,7 +436,7 @@ describe(FilesRepo.name, () => { await em.persistAndFlush([fileEntityFactory.build(), fileEntityFactory.build(), fileEntityFactory.build()]); em.clear(); - const results = await repo.findByPermissionRefId(new ObjectId().toHexString()); + const results = await repo.findByPermissionRefIdOrCreatorId(new ObjectId().toHexString()); expect(results).toHaveLength(0); }); @@ -407,7 +446,7 @@ describe(FilesRepo.name, () => { it('should return an empty array', async () => { const testPermissionRefId = new ObjectId().toHexString(); - const results = await repo.findByPermissionRefId(testPermissionRefId); + const results = await repo.findByPermissionRefIdOrCreatorId(testPermissionRefId); expect(results).toHaveLength(0); }); diff --git a/apps/server/src/modules/files/repo/files.repo.ts b/apps/server/src/modules/files/repo/files.repo.ts index 4b30386d6d4..87cf26ec28e 100644 --- a/apps/server/src/modules/files/repo/files.repo.ts +++ b/apps/server/src/modules/files/repo/files.repo.ts @@ -41,15 +41,28 @@ export class FilesRepo extends BaseRepo { return files as FileEntity[]; } - public async findByPermissionRefId(permissionRefId: EntityId): Promise { + public async findByPermissionRefIdOrCreatorId(userId: EntityId): Promise { + const refId = new ObjectId(userId); + const pipeline = [ { $match: { - permissions: { - $elemMatch: { - refId: new ObjectId(permissionRefId), + $and: [ + { + $or: [ + { + permissions: { + $elemMatch: { + refId, + }, + }, + }, + { creator: refId }, + ], }, - }, + { deleted: false }, + { deletedAt: undefined }, + ], }, }, ]; diff --git a/apps/server/src/modules/files/service/files.service.spec.ts b/apps/server/src/modules/files/service/files.service.spec.ts index 98ddc788d34..4a65adaefc6 100644 --- a/apps/server/src/modules/files/service/files.service.spec.ts +++ b/apps/server/src/modules/files/service/files.service.spec.ts @@ -30,7 +30,7 @@ describe(FilesService.name, () => { }); afterEach(() => { - repo.findByPermissionRefId.mockClear(); + repo.findByPermissionRefIdOrCreatorId.mockClear(); repo.findByOwnerUserId.mockClear(); repo.save.mockClear(); }); @@ -39,82 +39,105 @@ describe(FilesService.name, () => { await module.close(); }); - describe('findFilesAccessibleByUser', () => { + describe('findFilesAccessibleOrCreatedByUser', () => { describe('when called with a userId of a user that', () => { const setup = () => { const userId = new ObjectId().toHexString(); - const accessibleFiles: FileEntity[] = []; + const userId2 = new ObjectId().toHexString(); + const userId3 = new ObjectId().toHexString(); + const accessibleOrCreatedFiles: FileEntity[] = []; for (let i = 0; i < 5; i += 1) { - accessibleFiles.push( + accessibleOrCreatedFiles.push( fileEntityFactory.build({ permissions: [filePermissionEntityFactory.build({ refId: userId })], + }), + fileEntityFactory.build({ + creatorId: userId2, + }), + fileEntityFactory.build({ + permissions: [filePermissionEntityFactory.build({ refId: userId })], + creatorId: userId3, + }), + fileEntityFactory.build({ + permissions: [filePermissionEntityFactory.build({ refId: userId3 })], + creatorId: userId, }) ); } - return { userId, accessibleFiles }; + return { userId, accessibleOrCreatedFiles }; }; - describe("doesn't have an access to any files", () => { + describe("doesn't have an access or is creator of any files", () => { it('should return an empty array', async () => { const { userId } = setup(); - repo.findByPermissionRefId.mockResolvedValueOnce([]); + repo.findByPermissionRefIdOrCreatorId.mockResolvedValueOnce([]); - const result = await service.findFilesAccessibleByUser(userId); + const result = await service.findFilesAccessibleOrCreatedByUser(userId); - expect(repo.findByPermissionRefId).toBeCalledWith(userId); + expect(repo.findByPermissionRefIdOrCreatorId).toBeCalledWith(userId); expect(result).toEqual([]); }); }); - describe('does have an access to some files', () => { + describe('does have an access or is creator of some files', () => { it('should return an array containing proper file entities', async () => { - const { userId, accessibleFiles } = setup(); + const { userId, accessibleOrCreatedFiles } = setup(); - repo.findByPermissionRefId.mockResolvedValueOnce(accessibleFiles); + repo.findByPermissionRefIdOrCreatorId.mockResolvedValueOnce(accessibleOrCreatedFiles); - const result = await service.findFilesAccessibleByUser(userId); + const result = await service.findFilesAccessibleOrCreatedByUser(userId); - expect(repo.findByPermissionRefId).toBeCalledWith(userId); - expect(result).toEqual(accessibleFiles); + expect(repo.findByPermissionRefIdOrCreatorId).toBeCalledWith(userId); + expect(result).toEqual(accessibleOrCreatedFiles); }); }); }); }); - describe('removeUserPermissionsToAnyFiles', () => { - it('should not modify any files if there are none that user has permission to access', async () => { + describe('removeUserPermissionsOrCreatorReferenceToAnyFiles', () => { + it('should not modify any files if there are none that user has permission to access or is creator', async () => { const userId = new ObjectId().toHexString(); - repo.findByPermissionRefId.mockResolvedValueOnce([]); + repo.findByPermissionRefIdOrCreatorId.mockResolvedValueOnce([]); - const result = await service.removeUserPermissionsToAnyFiles(userId); + const result = await service.removeUserPermissionsOrCreatorReferenceToAnyFiles(userId); expect(result).toEqual(0); - expect(repo.findByPermissionRefId).toBeCalledWith(userId); + expect(repo.findByPermissionRefIdOrCreatorId).toBeCalledWith(userId); expect(repo.save).not.toBeCalled(); }); - describe('should properly remove user permissions', () => { - it('in case of just a single file accessible by given user', async () => { + describe('should properly remove user permissions, creatorId reference', () => { + const setup = () => { const userId = new ObjectId().toHexString(); const userPermission = filePermissionEntityFactory.build({ refId: userId }); - const entity = fileEntityFactory.buildWithId({ permissions: [userPermission] }); - repo.findByPermissionRefId.mockResolvedValueOnce([entity]); + const entity = fileEntityFactory.buildWithId({ permissions: [userPermission], creatorId: userId }); + const entity2 = fileEntityFactory.buildWithId({ creatorId: userId }); + const entity3 = fileEntityFactory.buildWithId({ permissions: [userPermission] }); - const result = await service.removeUserPermissionsToAnyFiles(userId); + repo.findByPermissionRefIdOrCreatorId.mockResolvedValueOnce([entity, entity2, entity3]); + return { userId, userPermission, entity, entity2, entity3 }; + }; + it('in case of just a single file (permission) accessible by given user and couple of files created', async () => { + const { userId, userPermission, entity, entity2, entity3 } = setup(); - expect(result).toEqual(1); - expect(entity.permissions).not.toContain(userPermission); + const result = await service.removeUserPermissionsOrCreatorReferenceToAnyFiles(userId); - expect(repo.findByPermissionRefId).toBeCalledWith(userId); - expect(repo.save).toBeCalledWith([entity]); + expect(result).toEqual(3); + expect(entity3.permissions).not.toContain(userPermission); + expect(entity._creatorId).toBe(undefined); + expect(entity3.permissions).not.toContain(userPermission); + expect(entity2._creatorId).toBe(undefined); + + expect(repo.findByPermissionRefIdOrCreatorId).toBeCalledWith(userId); + expect(repo.save).toBeCalledWith([entity, entity2, entity3]); }); - it('in case of many files accessible by given user', async () => { + it('in case of many files accessible or created by given user', async () => { const userId = new ObjectId().toHexString(); const userPermission = filePermissionEntityFactory.build({ refId: userId }); const anotherUserPermission = filePermissionEntityFactory.build(); @@ -125,31 +148,36 @@ describe(FilesService.name, () => { }), fileEntityFactory.buildWithId({ permissions: [yetAnotherUserPermission, userPermission, anotherUserPermission], + creatorId: userId, }), fileEntityFactory.buildWithId({ permissions: [anotherUserPermission, yetAnotherUserPermission, userPermission], }), fileEntityFactory.buildWithId({ permissions: [userPermission, yetAnotherUserPermission, anotherUserPermission], + creatorId: userId, }), fileEntityFactory.buildWithId({ permissions: [yetAnotherUserPermission, anotherUserPermission, userPermission], }), ]; - repo.findByPermissionRefId.mockResolvedValueOnce(entities); + repo.findByPermissionRefIdOrCreatorId.mockResolvedValueOnce(entities); - const result = await service.removeUserPermissionsToAnyFiles(userId); + const result = await service.removeUserPermissionsOrCreatorReferenceToAnyFiles(userId); expect(result).toEqual(5); - entities.forEach((entity) => { - expect(entity.permissions).not.toContain(userPermission); - expect(entity.permissions).toContain(anotherUserPermission); - expect(entity.permissions).toContain(yetAnotherUserPermission); - }); + for (let i = 0; i < entities.length; i += 1) { + expect(entities[i].permissions).not.toContain(userPermission); + if (i === 1 || i === 3) { + expect(entities[i]._creatorId).toBe(undefined); + } + expect(entities[i].permissions).toContain(anotherUserPermission); + expect(entities[i].permissions).toContain(yetAnotherUserPermission); + } - expect(repo.findByPermissionRefId).toBeCalledWith(userId); + expect(repo.findByPermissionRefIdOrCreatorId).toBeCalledWith(userId); expect(repo.save).toBeCalledWith(entities); }); }); diff --git a/apps/server/src/modules/files/service/files.service.ts b/apps/server/src/modules/files/service/files.service.ts index 7f7666edbc3..c665e196f46 100644 --- a/apps/server/src/modules/files/service/files.service.ts +++ b/apps/server/src/modules/files/service/files.service.ts @@ -7,18 +7,21 @@ import { FilesRepo } from '../repo'; export class FilesService { constructor(private readonly repo: FilesRepo) {} - async findFilesAccessibleByUser(userId: EntityId): Promise { - return this.repo.findByPermissionRefId(userId); + async findFilesAccessibleOrCreatedByUser(userId: EntityId): Promise { + return this.repo.findByPermissionRefIdOrCreatorId(userId); } - async removeUserPermissionsToAnyFiles(userId: EntityId): Promise { - const entities = await this.repo.findByPermissionRefId(userId); + async removeUserPermissionsOrCreatorReferenceToAnyFiles(userId: EntityId): Promise { + const entities = await this.repo.findByPermissionRefIdOrCreatorId(userId); if (entities.length === 0) { return 0; } - entities.forEach((entity) => entity.removePermissionsByRefId(userId)); + entities.forEach((entity) => { + entity.removePermissionsByRefId(userId); + entity.removeCreatorId(userId); + }); await this.repo.save(entities);