From 308beddb2428d030d1520d71e19e17938e81f28a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20No=C3=ABl?= Date: Wed, 27 May 2020 13:15:52 +0200 Subject: [PATCH] add DELETE /admin/roles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Pierre Noël --- packages/strapi-admin/ee/config/routes.json | 7 ++ packages/strapi-admin/ee/controllers/role.js | 20 +++- packages/strapi-admin/ee/validation/role.js | 17 +++ .../services/__tests__/role.test.js | 51 +++++++++ packages/strapi-admin/services/role.js | 29 +++++ .../test/admin-role-crud.test.e2e.js | 106 +++++++++++------- .../validation/common-validators.js | 3 + .../lib/relations.js | 6 +- .../lib/middlewares/parser/defaults.json | 3 +- 9 files changed, 196 insertions(+), 46 deletions(-) diff --git a/packages/strapi-admin/ee/config/routes.json b/packages/strapi-admin/ee/config/routes.json index cfa88a98e2..2475ceda1b 100644 --- a/packages/strapi-admin/ee/config/routes.json +++ b/packages/strapi-admin/ee/config/routes.json @@ -7,6 +7,13 @@ "config": { "policies": [] } + }, { + "method": "DELETE", + "path": "/roles", + "handler": "role.delete", + "config": { + "policies": [] + } } ] } diff --git a/packages/strapi-admin/ee/controllers/role.js b/packages/strapi-admin/ee/controllers/role.js index a6c409b31e..928ad25334 100644 --- a/packages/strapi-admin/ee/controllers/role.js +++ b/packages/strapi-admin/ee/controllers/role.js @@ -1,6 +1,10 @@ 'use strict'; -const { validateRoleCreateInput, validateRoleUpdateInput } = require('../validation/role'); +const { + validateRoleCreateInput, + validateRoleUpdateInput, + validateRoleDeleteInput, +} = require('../validation/role'); module.exports = { async create(ctx) { @@ -35,4 +39,18 @@ module.exports = { data: sanitizedRole, }; }, + async delete(ctx) { + try { + await validateRoleDeleteInput(ctx.request.body); + } catch (err) { + return ctx.badRequest('ValidationError', err); + } + + let roles = await strapi.admin.services.role.delete({ id_in: ctx.request.body.ids }); + const sanitizedRoles = roles.map(strapi.admin.services.role.sanitizeRole); + + ctx.body = { + data: sanitizedRoles, + }; + }, }; diff --git a/packages/strapi-admin/ee/validation/role.js b/packages/strapi-admin/ee/validation/role.js index ac9394a8f7..f734c2335d 100644 --- a/packages/strapi-admin/ee/validation/role.js +++ b/packages/strapi-admin/ee/validation/role.js @@ -1,6 +1,7 @@ 'use strict'; const { yup, formatYupErrors } = require('strapi-utils'); +const { intergerOrString } = require('../../validation/common-validators'); const handleReject = error => Promise.reject(formatYupErrors(error)); @@ -27,7 +28,23 @@ const validateRoleUpdateInput = async data => { .catch(handleReject); }; +const validateRoleDeleteInput = async data => { + const roleDeleteSchema = yup + .object() + .shape({ + ids: yup + .array() + .of(intergerOrString) + .min(1) + .required(), + }) + .noUnknown(); + + return roleDeleteSchema.validate(data, { strict: true, abortEarly: false }).catch(handleReject); +}; + module.exports = { validateRoleCreateInput, validateRoleUpdateInput, + validateRoleDeleteInput, }; diff --git a/packages/strapi-admin/services/__tests__/role.test.js b/packages/strapi-admin/services/__tests__/role.test.js index d5b5cb848f..a2ba87c0aa 100644 --- a/packages/strapi-admin/services/__tests__/role.test.js +++ b/packages/strapi-admin/services/__tests__/role.test.js @@ -123,4 +123,55 @@ describe('Role', () => { expect(updatedRole).toStrictEqual(expectedUpdatedRole); }); }); + describe('delete', () => { + test('Delete a role', async () => { + const role = { + id: 1, + name: 'admin', + description: 'Description', + users: [], + }; + const dbFind = jest.fn(() => Promise.resolve([role])); + const dbDelete = jest.fn(() => Promise.resolve(role)); + + global.strapi = { + query: () => ({ find: dbFind, delete: dbDelete }), + }; + + const deletedRoles = await roleService.delete({ id: role.id }); + + expect(dbFind).toHaveBeenCalledWith({ id: role.id }); + expect(dbDelete).toHaveBeenCalledWith({ id_in: [role.id] }); + expect(deletedRoles).toStrictEqual([role]); + }); + test('Delete two roles', async () => { + const roles = [ + { + id: 1, + name: 'admin 1', + description: 'Description', + users: [], + }, + { + id: 2, + name: 'admin 2', + description: 'Description', + users: [], + }, + ]; + const rolesIds = roles.map(r => r.id); + const dbFind = jest.fn(() => Promise.resolve(roles)); + const dbDelete = jest.fn(() => Promise.resolve(roles)); + + global.strapi = { + query: () => ({ find: dbFind, delete: dbDelete }), + }; + + const deletedRoles = await roleService.delete({ id_in: rolesIds }); + + expect(dbFind).toHaveBeenCalledWith({ id_in: rolesIds }); + expect(dbDelete).toHaveBeenCalledWith({ id_in: rolesIds }); + expect(deletedRoles).toStrictEqual(roles); + }); + }); }); diff --git a/packages/strapi-admin/services/role.js b/packages/strapi-admin/services/role.js index 360f342b58..e1c85b93bb 100644 --- a/packages/strapi-admin/services/role.js +++ b/packages/strapi-admin/services/role.js @@ -78,6 +78,34 @@ const exists = async params => { return foundCount > 0; }; +/** + * Delete roles in database if they have no user assigned + * @param params query params to find the roles + * @returns {Promise} + */ +const deleteRoles = async params => { + const foundRoles = await strapi.query('role', 'admin').find(params); + + if (foundRoles.some(r => r.users.length !== 0)) { + throw strapi.errors.badRequest('ValidationError', { + ids: ['Some roles are still assigned to some users.'], + }); + } + + const rolesToDeleteIds = foundRoles.map(role => role.id); + + // TODO: Waiting for permissions + // await strapi.admin.services.permission.delete({ roleId_in: rolesToDeleteIds }); + + let deletedRoles = await strapi.query('role', 'admin').delete({ id_in: rolesToDeleteIds }); + + if (!Array.isArray(deletedRoles)) { + deletedRoles = [deletedRoles]; + } + + return deletedRoles; +}; + module.exports = { sanitizeRole, create, @@ -86,4 +114,5 @@ module.exports = { findAll, update, exists, + delete: deleteRoles, }; diff --git a/packages/strapi-admin/test/admin-role-crud.test.e2e.js b/packages/strapi-admin/test/admin-role-crud.test.e2e.js index c5934f9fd9..6f94c083e2 100644 --- a/packages/strapi-admin/test/admin-role-crud.test.e2e.js +++ b/packages/strapi-admin/test/admin-role-crud.test.e2e.js @@ -1,4 +1,5 @@ -// Helpers. +const _ = require('lodash'); + const { registerAndLogin } = require('../../../test/helpers/auth'); const { createAuthRequest } = require('../../../test/helpers/request'); @@ -17,21 +18,22 @@ describe('Role CRUD End to End', () => { }, 60000); if (edition === 'EE') { - describe('Create a new role', () => { - test('Can create a role successfully', async () => { - const role = { - name: 'new role', - description: 'Description of new role', - }; - - const res = await rq({ + describe('Create some roles', () => { + const rolesToCreate = [ + [{ name: 'new role 0', description: 'description' }], + [{ name: 'new role 1', description: 'description' }], + [{ name: 'new role 2', description: 'description' }], + [{ name: 'new role 3', description: 'description' }], + [{ name: 'new role 4', description: 'description' }], + [{ name: 'new role 5', description: 'description' }], + ]; + test.each(rolesToCreate)('can create %p', async role => { + let res = await rq({ url: '/admin/roles', method: 'POST', body: role, }); - data.roles.push(res.body.data); - expect(res.statusCode).toBe(201); expect(res.body.data).toMatchObject({ id: expect.anything(), @@ -40,36 +42,10 @@ describe('Role CRUD End to End', () => { created_at: expect.anything(), updated_at: expect.anything(), }); - }); - test('Can create another role successfully', async () => { - const role = { - name: 'new role 2', - description: 'Description of new role 2', - }; - - const res = await rq({ - url: '/admin/roles', - method: 'POST', - body: role, - }); - data.roles.push(res.body.data); - - expect(res.statusCode).toBe(201); - expect(res.body.data).toMatchObject({ - id: expect.anything(), - name: role.name, - description: role.description, - created_at: expect.anything(), - updated_at: expect.anything(), - }); }); test('Cannot create a role already existing', async () => { - const role = { - name: 'new role', - description: 'Description of new role', - }; - + const role = _.pick(data.roles[0], ['name', 'description']); const res = await rq({ url: '/admin/roles', method: 'POST', @@ -78,7 +54,7 @@ describe('Role CRUD End to End', () => { expect(res.statusCode).toBe(400); expect(res.body.data).toMatchObject({ - name: ['The name must be unique and a role with name `new role` already exists.'], + name: [`The name must be unique and a role with name \`${role.name}\` already exists.`], }); }); }); @@ -183,6 +159,58 @@ describe('Role CRUD End to End', () => { }); }); }); + describe('Delete roles', () => { + test('Can delete a role', async () => { + let res = await rq({ + url: '/admin/roles', + method: 'DELETE', + body: { ids: [data.roles[0].id] }, + }); + expect(res.statusCode).toBe(200); + expect(res.body.data).toMatchObject([data.roles[0]]); + + res = await rq({ + url: `/admin/roles/${data.roles[0].id}`, + method: 'GET', + body: { ids: data.roles[0].id }, + }); + expect(res.statusCode).toBe(404); + + data.roles.shift(); + }); + test('Can delete two roles', async () => { + const roles = data.roles.slice(0, 2); + const rolesIds = roles.map(r => r.id); + + let res = await rq({ + url: '/admin/roles', + method: 'DELETE', + body: { ids: rolesIds }, + }); + expect(res.statusCode).toBe(200); + expect(res.body.data).toMatchObject(roles); + + for (let roleId of rolesIds) { + res = await rq({ + url: `/admin/roles/${roleId}`, + method: 'GET', + body: { ids: data.roles[0].id }, + }); + expect(res.statusCode).toBe(404); + data.roles.shift(); + } + }); + test("No error if deleting a role that doesn't exist", async () => { + const res = await rq({ + url: '/admin/roles', + method: 'DELETE', + body: { ids: ['id-that-doesnt-exist'] }, + }); + + expect(res.statusCode).toBe(200); + expect(res.body.data).toEqual([]); + }); + }); } if (edition === 'CE') { diff --git a/packages/strapi-admin/validation/common-validators.js b/packages/strapi-admin/validation/common-validators.js index fd215606e4..da558b1bec 100644 --- a/packages/strapi-admin/validation/common-validators.js +++ b/packages/strapi-admin/validation/common-validators.js @@ -15,6 +15,9 @@ const validators = { .matches(/[a-z]/, '${path} must contain at least one lowercase character') .matches(/[A-Z]/, '${path} must contain at least one uppercase character') .matches(/\d/, '${path} must contain at least one number'), + intergerOrString: yup.lazy(value => + typeof value === 'number' ? yup.number().integer() : yup.string() + ), // https://github.com/jquense/yup/issues/665 }; module.exports = validators; diff --git a/packages/strapi-connector-bookshelf/lib/relations.js b/packages/strapi-connector-bookshelf/lib/relations.js index 372856c092..bf4c70fa80 100644 --- a/packages/strapi-connector-bookshelf/lib/relations.js +++ b/packages/strapi-connector-bookshelf/lib/relations.js @@ -24,11 +24,7 @@ const transformToArrayID = array => { }; const getModel = (model, plugin) => { - return ( - _.get(strapi.plugins, [plugin, 'models', model]) || - _.get(strapi, ['models', model]) || - undefined - ); + return strapi.db.getModel(model, plugin) || strapi.db.getModel(model); }; const removeUndefinedKeys = obj => _.pickBy(obj, _.negate(_.isUndefined)); diff --git a/packages/strapi/lib/middlewares/parser/defaults.json b/packages/strapi/lib/middlewares/parser/defaults.json index 5ba1b76c99..5d7d0a920e 100644 --- a/packages/strapi/lib/middlewares/parser/defaults.json +++ b/packages/strapi/lib/middlewares/parser/defaults.json @@ -1,6 +1,7 @@ { "parser": { "enabled": true, - "multipart": true + "multipart": true, + "parsedMethods": ["POST", "PUT", "PATCH", "DELETE"] } }