From 96544df7d5a79cfa3ce87f8410e3ac321b98adb3 Mon Sep 17 00:00:00 2001 From: Ali BARIN Date: Mon, 28 Oct 2024 14:57:33 +0100 Subject: [PATCH] refactor(role): remove transactions and tidy up logic in model (#2141) * refactor(role): remove returning this in model methods * refactor(role): assert altering admin in model before update and delete * refactor(role): rename overridePermissions with updatePermissions in model * refactor(role): remove transactions in model * refactor(role): remove transactions in model * refactor(role): return with permissions upon update in model * fix(role): assert admin check on old instance in model * refactor(role): fetch and use current role in preventAlteringAdmin --- .../api/v1/admin/roles/delete-role.ee.test.js | 17 -- packages/backend/src/models/role.js | 68 ++++--- packages/backend/src/models/role.test.js | 175 ++---------------- 3 files changed, 48 insertions(+), 212 deletions(-) diff --git a/packages/backend/src/controllers/api/v1/admin/roles/delete-role.ee.test.js b/packages/backend/src/controllers/api/v1/admin/roles/delete-role.ee.test.js index a9396bf0..839acc19 100644 --- a/packages/backend/src/controllers/api/v1/admin/roles/delete-role.ee.test.js +++ b/packages/backend/src/controllers/api/v1/admin/roles/delete-role.ee.test.js @@ -92,21 +92,4 @@ describe('DELETE /api/v1/admin/roles/:roleId', () => { }, }); }); - - it('should not delete role and permissions on unsuccessful response', async () => { - const role = await createRole(); - const permission = await createPermission({ roleId: role.id }); - await createUser({ roleId: role.id }); - - await request(app) - .delete(`/api/v1/admin/roles/${role.id}`) - .set('Authorization', token) - .expect(422); - - const refetchedRole = await role.$query(); - const refetchedPermission = await permission.$query(); - - expect(refetchedRole).toStrictEqual(role); - expect(refetchedPermission).toStrictEqual(permission); - }); }); diff --git a/packages/backend/src/models/role.js b/packages/backend/src/models/role.js index a0a7cf7c..9e0404a1 100644 --- a/packages/backend/src/models/role.js +++ b/packages/backend/src/models/role.js @@ -52,17 +52,19 @@ class Role extends Base { return await this.query().findOne({ name: 'Admin' }); } - preventAlteringAdmin() { - if (this.isAdmin) { + async preventAlteringAdmin() { + const currentRole = await Role.query().findById(this.id); + + if (currentRole.isAdmin) { throw new NotAuthorizedError('The admin role cannot be altered!'); } } - async deletePermissions(trx) { - return await this.$relatedQuery('permissions', trx).delete(); + async deletePermissions() { + return await this.$relatedQuery('permissions').delete(); } - async createPermissions(permissions, trx) { + async createPermissions(permissions) { if (permissions?.length) { const validPermissions = Permission.filter(permissions).map( (permission) => ({ @@ -71,50 +73,40 @@ class Role extends Base { }) ); - await Permission.query(trx).insert(validPermissions); + await Permission.query().insert(validPermissions); } - - return this; } - async overridePermissions(permissions, trx) { - await this.deletePermissions(trx); + async updatePermissions(permissions) { + await this.deletePermissions(); - await this.createPermissions(permissions, trx); - - return this; + await this.createPermissions(permissions); } async updateWithPermissions(data) { - this.preventAlteringAdmin(); - const { name, description, permissions } = data; - return await Role.transaction(async (trx) => { - await this.overridePermissions(permissions, trx); + await this.updatePermissions(permissions); - await this.$query(trx).patch({ - name, - description, - }); - - // TODO: consider removing returning permissions as they're not utilized - return await this.$query(trx) - .leftJoinRelated({ - permissions: true, - }) - .withGraphFetched({ - permissions: true, - }); + await this.$query().patchAndFetch({ + id: this.id, + name, + description, }); + + return await this.$query() + .leftJoinRelated({ + permissions: true, + }) + .withGraphFetched({ + permissions: true, + }); } async deleteWithPermissions() { - return await Role.transaction(async (trx) => { - await this.deletePermissions(trx); + await this.deletePermissions(); - return await this.$query(trx).delete(); - }); + return await this.$query().delete(); } async assertNoRoleUserExists() { @@ -164,10 +156,16 @@ class Role extends Base { await this.assertNoConfigurationUsage(); } + async $beforeUpdate(opt, queryContext) { + await super.$beforeUpdate(opt, queryContext); + + await this.preventAlteringAdmin(); + } + async $beforeDelete(queryContext) { await super.$beforeDelete(queryContext); - this.preventAlteringAdmin(); + await this.preventAlteringAdmin(); await this.assertRoleIsNotUsed(); } diff --git a/packages/backend/src/models/role.test.js b/packages/backend/src/models/role.test.js index d272d5a7..780c8f0d 100644 --- a/packages/backend/src/models/role.test.js +++ b/packages/backend/src/models/role.test.js @@ -71,47 +71,28 @@ describe('Role model', () => { }); describe('preventAlteringAdmin', () => { - it('preventAlteringAdmin should throw an error when altering admin role', () => { - const role = new Role(); - role.name = 'Admin'; + it('preventAlteringAdmin should throw an error when altering admin role', async () => { + const role = await createRole({ name: 'Admin' }); - expect(() => role.preventAlteringAdmin()).toThrowError( + await expect(() => role.preventAlteringAdmin()).rejects.toThrowError( 'The admin role cannot be altered!' ); }); - it('preventAlteringAdmin should not throw an error when altering non-admin roles', () => { - const role = new Role(); - role.name = 'User'; + it('preventAlteringAdmin should not throw an error when altering non-admin roles', async () => { + const role = await createRole({ name: 'User' }); - expect(() => role.preventAlteringAdmin()).not.toThrowError(); + expect(await role.preventAlteringAdmin()).toBe(undefined); }); }); - describe('deletePermissions', () => { - it("should delete role's permissions", async () => { - const role = await createRole({ name: 'User' }); - await createPermission({ roleId: role.id }); + it("deletePermissions should delete role's permissions", async () => { + const role = await createRole({ name: 'User' }); + await createPermission({ roleId: role.id }); - await role.deletePermissions(); + await role.deletePermissions(); - expect(await role.$relatedQuery('permissions')).toStrictEqual([]); - }); - - it('should accept transaction', async () => { - const transaction = vi.fn(); - - const relatedQuerySpy = vi - .spyOn(Role.prototype, '$relatedQuery') - .mockReturnValue({ delete: () => {} }); - - const role = await createRole({ name: 'User' }); - - await role.deletePermissions(transaction); - - expect(relatedQuerySpy).toHaveBeenCalledWith('permissions', transaction); - 1; - }); + expect(await role.$relatedQuery('permissions')).toStrictEqual([]); }); describe('createPermissions', () => { @@ -144,32 +125,13 @@ describe('Role model', () => { expect(permissionFilterSpy).toHaveBeenCalledWith(permissions); }); - - it('should accept transaction', async () => { - const transaction = vi.fn(); - - const permissionQuerySpy = vi - .spyOn(Permission, 'query') - .mockReturnValue({ insert: () => {} }); - - const role = await createRole({ name: 'User' }); - - await role.createPermissions( - [{ action: 'read', subject: 'Flow', conditions: [] }], - transaction - ); - - expect(permissionQuerySpy).toHaveBeenCalledWith(transaction); - 1; - }); }); - it('overridePermissions should delete existing permissions and create new permissions', async () => { + it('updatePermissions should delete existing permissions and create new permissions', async () => { const permissionsData = [ { action: 'read', subject: 'Flow', conditions: [] }, ]; - const transaction = vi.fn(); const deletePermissionsSpy = vi .spyOn(Role.prototype, 'deletePermissions') .mockResolvedValueOnce(); @@ -179,18 +141,14 @@ describe('Role model', () => { const role = await createRole({ name: 'User' }); - await role.overridePermissions(permissionsData, transaction); + await role.updatePermissions(permissionsData); expect(deletePermissionsSpy.mock.invocationCallOrder[0]).toBeLessThan( createPermissionsSpy.mock.invocationCallOrder[0] ); - expect(deletePermissionsSpy).toHaveBeenNthCalledWith(1, transaction); - expect(createPermissionsSpy).toHaveBeenNthCalledWith( - 1, - permissionsData, - transaction - ); + expect(deletePermissionsSpy).toHaveBeenNthCalledWith(1); + expect(createPermissionsSpy).toHaveBeenNthCalledWith(1, permissionsData); }); describe('updateWithPermissions', () => { @@ -224,83 +182,6 @@ describe('Role model', () => { expect(roleWithPermissions).toMatchObject(newRoleData); }); - - it('should throw an error while updating the admin role', async () => { - const role = new Role(); - role.name = 'Admin'; - - await expect(() => role.updateWithPermissions()).rejects.toThrowError( - 'The admin role cannot be altered!' - ); - }); - - it('should use transaction', async () => { - const transaction = vi.fn(); - const overridePermissionsSpy = vi - .spyOn(Role.prototype, 'overridePermissions') - .mockResolvedValue(); - - const querySpy = vi.spyOn(Role.prototype, '$query').mockReturnValue({ - patch: vi.fn().mockReturnValue(Promise.resolve()), - leftJoinRelated: vi.fn().mockReturnThis(), - withGraphFetched: vi.fn().mockResolvedValue({}), - }); - - const transactionSpy = vi - .spyOn(Role, 'transaction') - .mockImplementation(async (callback) => { - return await callback(transaction); - }); - const role = await createRole({ name: 'User' }); - - const newRoleData = { - name: 'New user', - description: 'Updated user role', - permissions: [{ action: 'read', subject: 'Flow', conditions: [] }], - }; - - await role.updateWithPermissions(newRoleData); - - expect(transactionSpy).toHaveBeenCalledOnce(); - - expect(overridePermissionsSpy).toHaveBeenCalledWith( - newRoleData.permissions, - transaction - ); - - expect(querySpy).toHaveBeenNthCalledWith(2, transaction); - }); - - it('should revert changes when an error occurs', async () => { - const role = await createRole({ name: 'User' }); - await createPermission({ - roleId: role.id, - subject: 'Flow', - action: 'read', - conditions: [], - }); - - const roleWithPermissions = await role - .$query() - .leftJoinRelated({ permissions: true }) - .withGraphFetched({ permissions: true }); - - await expect(() => - role.updateWithPermissions({ - name: false, - description: 123, - }) - ).rejects.toThrowError( - 'name: must be string, description: must be string,null' - ); - - const refetchedRoleWithPermissions = await role - .$query() - .leftJoinRelated({ permissions: true }) - .withGraphFetched({ permissions: true }); - - expect(roleWithPermissions).toStrictEqual(refetchedRoleWithPermissions); - }); }); describe('deleteWithPermissions', () => { @@ -323,32 +204,6 @@ describe('Role model', () => { expect(refetchedRole).toBe(undefined); expect(rolePermissions).toStrictEqual([]); }); - - it('should use transaction', async () => { - const transaction = vi.fn(); - const deletePermissionsSpy = vi - .spyOn(Role.prototype, 'deletePermissions') - .mockResolvedValue(); - - const querySpy = vi.spyOn(Role.prototype, '$query').mockReturnValue({ - delete: vi.fn().mockReturnValue(Promise.resolve()), - }); - - const transactionSpy = vi - .spyOn(Role, 'transaction') - .mockImplementation(async (callback) => { - return await callback(transaction); - }); - const role = await createRole({ name: 'User' }); - - await role.deleteWithPermissions(); - - expect(transactionSpy).toHaveBeenCalledOnce(); - - expect(deletePermissionsSpy).toHaveBeenCalledWith(transaction); - - expect(querySpy).toHaveBeenCalledWith(transaction); - }); }); describe('assertNoRoleUserExists', () => {