From f65d06fc7db84905be5543c7a1f7e34ebece9ce9 Mon Sep 17 00:00:00 2001 From: alexm Date: Wed, 26 Oct 2022 08:27:28 +0200 Subject: [PATCH 1/4] fixes #4073 --- back/methods/account/privileges.js | 26 +++++++++------ back/methods/account/specs/privileges.spec.js | 14 ++++---- db/changes/10490-august/00-user_hasGrant.sql | 3 ++ db/dump/fixtures.sql | 8 ++--- e2e/paths/14-account/07_ldap.spec.js | 9 ++++++ e2e/paths/14-account/08_samba.spec.js | 9 ++++++ e2e/paths/14-account/09_privileges.spec.js | 32 +++++++++++++++++-- loopback/locale/en.json | 6 ++-- loopback/locale/es.json | 6 ++-- .../account/front/privileges/locale/es.yml | 2 +- modules/client/back/models/client.js | 9 ++++-- 11 files changed, 93 insertions(+), 31 deletions(-) diff --git a/back/methods/account/privileges.js b/back/methods/account/privileges.js index df421125e..6510779f2 100644 --- a/back/methods/account/privileges.js +++ b/back/methods/account/privileges.js @@ -29,6 +29,8 @@ module.exports = Self => { }); Self.privileges = async function(ctx, id, roleFk, hasGrant, options) { + if (!(hasGrant != null || roleFk)) return; + const models = Self.app.models; const userId = ctx.req.accessToken.userId; @@ -37,22 +39,26 @@ module.exports = Self => { if (typeof options == 'object') Object.assign(myOptions, options); - const user = await models.Account.findById(userId, null, myOptions); + const user = await models.Account.findById(userId, {fields: ['hasGrant']}, myOptions); if (!user.hasGrant) - throw new UserError(`You don't have enough privileges`); + throw new UserError(`You don't have grant privilege`); - const userToUpdate = await models.Account.findById(id); + const userToUpdate = await models.Account.findById(id, ['name', 'hasGrant', 'roleFk'], myOptions); if (hasGrant != null) - return await userToUpdate.updateAttribute('hasGrant', hasGrant, myOptions); - if (!roleFk) return; + userToUpdate.hasGrant = hasGrant; - const role = await models.Role.findById(roleFk, null, myOptions); - const hasRole = await models.Account.hasRole(userId, role.name, myOptions); + if (roleFk) { + const role = await models.Role.findById(roleFk, {fields: ['name']}, myOptions); + const hasRole = await models.Account.hasRole(userId, role.name, myOptions); - if (!hasRole) - throw new UserError(`You don't have enough privileges`); + if (!hasRole) + throw new UserError(`You don't own the role and you can't assign it to another user`); - await userToUpdate.updateAttribute('roleFk', roleFk, myOptions); + userToUpdate.roleFk = roleFk; + } + + await userToUpdate.save(userToUpdate); + await models.UserAccount.sync(userToUpdate.name); }; }; diff --git a/back/methods/account/specs/privileges.spec.js b/back/methods/account/specs/privileges.spec.js index 137c08671..959130e8b 100644 --- a/back/methods/account/specs/privileges.spec.js +++ b/back/methods/account/specs/privileges.spec.js @@ -4,7 +4,7 @@ describe('account privileges()', () => { const employeeId = 1; const developerId = 9; const sysadminId = 66; - const bruceWayneId = 1101; + const clarkKent = 1103; it('should throw an error when user not has privileges', async() => { const ctx = {req: {accessToken: {userId: developerId}}}; @@ -22,7 +22,7 @@ describe('account privileges()', () => { await tx.rollback(); } - expect(error.message).toContain(`You don't have enough privileges`); + expect(error.message).toContain(`You don't have grant privilege`); }); it('should throw an error when user has privileges but not has the role', async() => { @@ -46,7 +46,7 @@ describe('account privileges()', () => { await tx.rollback(); } - expect(error.message).toContain(`You don't have enough privileges`); + expect(error.message).toContain(`You don't own the role and you can't assign it to another user`); }); it('should change role', async() => { @@ -63,8 +63,8 @@ describe('account privileges()', () => { let error; let result; try { - await models.Account.privileges(ctx, bruceWayneId, agency.id, null, options); - result = await models.Account.findById(bruceWayneId, null, options); + await models.Account.privileges(ctx, clarkKent, agency.id, null, options); + result = await models.Account.findById(clarkKent, null, options); await tx.rollback(); } catch (e) { @@ -84,8 +84,8 @@ describe('account privileges()', () => { let result; try { const options = {transaction: tx}; - await models.Account.privileges(ctx, bruceWayneId, null, true, options); - result = await models.Account.findById(bruceWayneId, null, options); + await models.Account.privileges(ctx, clarkKent, null, true, options); + result = await models.Account.findById(clarkKent, null, options); await tx.rollback(); } catch (e) { diff --git a/db/changes/10490-august/00-user_hasGrant.sql b/db/changes/10490-august/00-user_hasGrant.sql index 60d1273d8..05a09f87b 100644 --- a/db/changes/10490-august/00-user_hasGrant.sql +++ b/db/changes/10490-august/00-user_hasGrant.sql @@ -1 +1,4 @@ ALTER TABLE `account`.`user` ADD hasGrant TINYINT(1) NOT NULL; + +INSERT INTO `salix`.`ACL` (model, property, accessType, permission, principalType, principalId) + VALUES('Account', 'privileges', '*', 'ALLOW', 'ROLE', '$authenticated'); diff --git a/db/dump/fixtures.sql b/db/dump/fixtures.sql index 7e59c1a54..c4079adc0 100644 --- a/db/dump/fixtures.sql +++ b/db/dump/fixtures.sql @@ -14,10 +14,10 @@ INSERT INTO `salix`.`AccessToken` (`id`, `ttl`, `created`, `userId`) ('DEFAULT_TOKEN', '1209600', util.VN_CURDATE(), 66); INSERT INTO `salix`.`printConfig` (`id`, `itRecipient`, `incidencesEmail`) - VALUES + VALUES (1, 'it@gotamcity.com', 'incidences@gotamcity.com'); -INSERT INTO `vn`.`ticketConfig` (`id`, `scopeDays`) +INSERT INTO `vn`.`ticketConfig` (`id`, `scopeDays`) VALUES ('1', '6'); @@ -45,8 +45,8 @@ INSERT INTO `account`.`roleConfig`(`id`, `mysqlPassword`, `rolePrefix`, `userPre CALL `account`.`role_sync`; -INSERT INTO `account`.`user`(`id`,`name`, `nickname`, `password`,`role`,`active`,`email`, `lang`, `image`) - SELECT id, name, CONCAT(name, 'Nick'),MD5('nightmare'), id, 1, CONCAT(name, '@mydomain.com'), 'en', '4fa3ada0-3ac4-11eb-9ab8-27f6fc3b85fd' +INSERT INTO `account`.`user`(`id`,`name`, `nickname`, `password`,`role`,`active`,`email`, `lang`, `image`, `bcryptPassword`) + SELECT id, name, CONCAT(name, 'Nick'),MD5('nightmare'), id, 1, CONCAT(name, '@mydomain.com'), 'en', '4fa3ada0-3ac4-11eb-9ab8-27f6fc3b85fd', '$2b$10$UzQHth.9UUQ1T5aiQJ21lOU0oVlbxoqH4PFM9V8T90KNSAcg0eEL2' FROM `account`.`role` WHERE id <> 20 ORDER BY id; diff --git a/e2e/paths/14-account/07_ldap.spec.js b/e2e/paths/14-account/07_ldap.spec.js index a3b8137d3..eb22f695c 100644 --- a/e2e/paths/14-account/07_ldap.spec.js +++ b/e2e/paths/14-account/07_ldap.spec.js @@ -29,4 +29,13 @@ describe('Account LDAP path', () => { expect(message.text).toContain('Data saved!'); }); + + it('should reset data', async() => { + await page.waitToClick(selectors.accountLdap.checkEnable); + await page.waitToClick(selectors.accountLdap.save); + + const message = await page.waitForSnackbar(); + + expect(message.text).toContain('Data saved!'); + }); }); diff --git a/e2e/paths/14-account/08_samba.spec.js b/e2e/paths/14-account/08_samba.spec.js index c3db026dc..6e7ef9bbf 100644 --- a/e2e/paths/14-account/08_samba.spec.js +++ b/e2e/paths/14-account/08_samba.spec.js @@ -29,4 +29,13 @@ describe('Account Samba path', () => { expect(message.text).toContain('Data saved!'); }); + + it('should reset data', async() => { + await page.waitToClick(selectors.accountSamba.checkEnable); + await page.waitToClick(selectors.accountSamba.save); + + const message = await page.waitForSnackbar(); + + expect(message.text).toContain('Data saved!'); + }); }); diff --git a/e2e/paths/14-account/09_privileges.spec.js b/e2e/paths/14-account/09_privileges.spec.js index 71e9345a8..e4b8fb24c 100644 --- a/e2e/paths/14-account/09_privileges.spec.js +++ b/e2e/paths/14-account/09_privileges.spec.js @@ -24,7 +24,7 @@ describe('Account privileges path', () => { const message = await page.waitForSnackbar(); - expect(message.text).toContain(`You don't have enough privileges`); + expect(message.text).toContain(`You don't have grant privilege`); }); it('should throw error when change role', async() => { @@ -33,7 +33,7 @@ describe('Account privileges path', () => { const message = await page.waitForSnackbar(); - expect(message.text).toContain(`You don't have enough privileges`); + expect(message.text).toContain(`You don't have grant privilege`); }); }); @@ -56,7 +56,16 @@ describe('Account privileges path', () => { expect(result).toBe('checked'); }); - it('should change role', async() => { + it('should throw error when change role and not own role', async() => { + await page.autocompleteSearch(selectors.accountPrivileges.role, 'itBoss'); + await page.waitToClick(selectors.accountPrivileges.save); + + const message = await page.waitForSnackbar(); + + expect(message.text).toContain(`You don't own the role and you can't assign it to another user`); + }); + + it('should change role to employee', async() => { await page.autocompleteSearch(selectors.accountPrivileges.role, 'employee'); await page.waitToClick(selectors.accountPrivileges.save); const message = await page.waitForSnackbar(); @@ -67,6 +76,18 @@ describe('Account privileges path', () => { expect(message.text).toContain(`Data saved!`); expect(result).toContain('employee'); }); + + it('should return role to developer', async() => { + await page.autocompleteSearch(selectors.accountPrivileges.role, 'developer'); + await page.waitToClick(selectors.accountPrivileges.save); + const message = await page.waitForSnackbar(); + + await page.reloadSection('account.card.privileges'); + const result = await page.waitToGetProperty(selectors.accountPrivileges.role, 'value'); + + expect(message.text).toContain(`Data saved!`); + expect(result).toContain('developer'); + }); }); describe('as developer again', () => { @@ -76,7 +97,12 @@ describe('Account privileges path', () => { await page.waitToClick(selectors.accountPrivileges.checkHasGrant); await page.waitToClick(selectors.accountPrivileges.save); + const message = await page.waitForSnackbar(); + expect(message.text).toContain(`Data saved!`); + }); + + it('should logIn in developer', async() => { await page.reloadSection('account.card.privileges'); const result = await page.checkboxState(selectors.accountPrivileges.checkHasGrant); diff --git a/loopback/locale/en.json b/loopback/locale/en.json index e5a0fae32..1e151294f 100644 --- a/loopback/locale/en.json +++ b/loopback/locale/en.json @@ -133,5 +133,7 @@ "Descanso semanal 36h. / 72h.": "Weekly rest 36h. / 72h.", "Password does not meet requirements": "Password does not meet requirements", "You don't have privileges to change the zone": "You don't have privileges to change the zone or for these parameters there are more than one shipping options, talk to agencies", - "Not enough privileges to edit a client": "Not enough privileges to edit a client" -} \ No newline at end of file + "Not enough privileges to edit a client": "Not enough privileges to edit a client", + "You don't have grant privilege": "You don't have grant privilege", + "You don't own the role and you can't assign it to another user": "You don't own the role and you can't assign it to another user" +} diff --git a/loopback/locale/es.json b/loopback/locale/es.json index 67370b343..a41315dd1 100644 --- a/loopback/locale/es.json +++ b/loopback/locale/es.json @@ -235,5 +235,7 @@ "Dirección incorrecta": "Dirección incorrecta", "Modifiable user details only by an administrator": "Detalles de usuario modificables solo por un administrador", "Modifiable password only via recovery or by an administrator": "Contraseña modificable solo a través de la recuperación o por un administrador", - "Not enough privileges to edit a client": "No tienes suficientes privilegios para editar un cliente" -} \ No newline at end of file + "Not enough privileges to edit a client": "No tienes suficientes privilegios para editar un cliente", + "You don't have grant privilege": "No tienes privilegios para dar privilegios", + "You don't own the role and you can't assign it to another user": "No eres el propietario del rol y no puedes asignarlo a otro usuario" +} diff --git a/modules/account/front/privileges/locale/es.yml b/modules/account/front/privileges/locale/es.yml index f7330e1be..17f1ef29e 100644 --- a/modules/account/front/privileges/locale/es.yml +++ b/modules/account/front/privileges/locale/es.yml @@ -1,2 +1,2 @@ Privileges: Privilegios -Has grant: Tiene privilegios +Has grant: Puede dar privilegios diff --git a/modules/client/back/models/client.js b/modules/client/back/models/client.js index 3bd89eff1..e66cdb83f 100644 --- a/modules/client/back/models/client.js +++ b/modules/client/back/models/client.js @@ -425,14 +425,19 @@ module.exports = Self => { account.observe('before save', async ctx => { if (ctx.isNewInstance) return; - ctx.hookState.oldInstance = JSON.parse(JSON.stringify(ctx.currentInstance)); + if (ctx.currentInstance) + ctx.hookState.oldInstance = JSON.parse(JSON.stringify(ctx.currentInstance)); }); account.observe('after save', async ctx => { const changes = ctx.data || ctx.instance; if (!ctx.isNewInstance && changes) { const oldData = ctx.hookState.oldInstance; - const hasChanges = oldData.name != changes.name || oldData.active != changes.active; + let hasChanges; + + if (oldData) + hasChanges = oldData.name != changes.name || oldData.active != changes.active; + if (!hasChanges) return; const isClient = await Self.app.models.Client.count({id: oldData.id}); From 3ffc098b56d8ee9724edfe2c26a8e5813ab11857 Mon Sep 17 00:00:00 2001 From: alexm Date: Thu, 27 Oct 2022 09:01:09 +0200 Subject: [PATCH 2/4] feat(privileges): check if user has role from userToUpdate --- back/methods/account/privileges.js | 17 +++++++++++++++-- back/models/account.json | 7 +++++++ db/changes/10490-august/00-user_hasGrant.sql | 3 --- modules/account/front/privileges/locale/es.yml | 2 +- package.json | 2 +- 5 files changed, 24 insertions(+), 7 deletions(-) diff --git a/back/methods/account/privileges.js b/back/methods/account/privileges.js index 6510779f2..f3f500e7d 100644 --- a/back/methods/account/privileges.js +++ b/back/methods/account/privileges.js @@ -44,15 +44,28 @@ module.exports = Self => { if (!user.hasGrant) throw new UserError(`You don't have grant privilege`); - const userToUpdate = await models.Account.findById(id, ['name', 'hasGrant', 'roleFk'], myOptions); + const [userToUpdate] = await models.Account.find({ + fields: ['id', 'name', 'hasGrant', 'roleFk', 'password'], + include: { + relation: 'role', + scope: { + fields: ['name'] + } + }, + where: { + id: id + } + }, myOptions); + if (hasGrant != null) userToUpdate.hasGrant = hasGrant; if (roleFk) { const role = await models.Role.findById(roleFk, {fields: ['name']}, myOptions); const hasRole = await models.Account.hasRole(userId, role.name, myOptions); + const hasRoleFromUser = await models.Account.hasRole(userId, userToUpdate.role().name, myOptions); - if (!hasRole) + if (!hasRole || !hasRoleFromUser) throw new UserError(`You don't own the role and you can't assign it to another user`); userToUpdate.roleFk = roleFk; diff --git a/back/models/account.json b/back/models/account.json index c25cd532d..d0c17e70f 100644 --- a/back/models/account.json +++ b/back/models/account.json @@ -102,6 +102,13 @@ "principalType": "ROLE", "principalId": "$authenticated", "permission": "ALLOW" + }, + { + "property": "privileges", + "accessType": "*", + "principalType": "ROLE", + "principalId": "$authenticated", + "permission": "ALLOW" } ] } diff --git a/db/changes/10490-august/00-user_hasGrant.sql b/db/changes/10490-august/00-user_hasGrant.sql index 05a09f87b..60d1273d8 100644 --- a/db/changes/10490-august/00-user_hasGrant.sql +++ b/db/changes/10490-august/00-user_hasGrant.sql @@ -1,4 +1 @@ ALTER TABLE `account`.`user` ADD hasGrant TINYINT(1) NOT NULL; - -INSERT INTO `salix`.`ACL` (model, property, accessType, permission, principalType, principalId) - VALUES('Account', 'privileges', '*', 'ALLOW', 'ROLE', '$authenticated'); diff --git a/modules/account/front/privileges/locale/es.yml b/modules/account/front/privileges/locale/es.yml index 17f1ef29e..d66a7a6cf 100644 --- a/modules/account/front/privileges/locale/es.yml +++ b/modules/account/front/privileges/locale/es.yml @@ -1,2 +1,2 @@ Privileges: Privilegios -Has grant: Puede dar privilegios +Has grant: Puede delegar privilegios diff --git a/package.json b/package.json index 92ca13e45..573e42335 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,7 @@ "node-ssh": "^11.0.0", "object-diff": "0.0.4", "object.pick": "^1.3.0", - "puppeteer": "^18.0.5", + "puppeteer": "^19.0.0", "read-chunk": "^3.2.0", "require-yaml": "0.0.1", "sharp": "^0.27.1", From 5c65314162a8e3d821cf9346ad454828306a7de1 Mon Sep 17 00:00:00 2001 From: alexm Date: Thu, 27 Oct 2022 09:27:50 +0200 Subject: [PATCH 3/4] use findById --- back/methods/account/privileges.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/back/methods/account/privileges.js b/back/methods/account/privileges.js index f3f500e7d..d3aa9bf59 100644 --- a/back/methods/account/privileges.js +++ b/back/methods/account/privileges.js @@ -44,16 +44,13 @@ module.exports = Self => { if (!user.hasGrant) throw new UserError(`You don't have grant privilege`); - const [userToUpdate] = await models.Account.find({ + const userToUpdate = await models.Account.findById(id, { fields: ['id', 'name', 'hasGrant', 'roleFk', 'password'], include: { relation: 'role', scope: { fields: ['name'] } - }, - where: { - id: id } }, myOptions); From ad1b429d10d1d0d66373f54fbc9c6fafbae57600 Mon Sep 17 00:00:00 2001 From: alexm Date: Fri, 28 Oct 2022 08:09:47 +0200 Subject: [PATCH 4/4] check if user has role from userToUpdate --- back/methods/account/privileges.js | 14 ++++++---- back/methods/account/specs/privileges.spec.js | 28 +++++++++++++++---- 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/back/methods/account/privileges.js b/back/methods/account/privileges.js index d3aa9bf59..5c5e7409d 100644 --- a/back/methods/account/privileges.js +++ b/back/methods/account/privileges.js @@ -41,9 +41,6 @@ module.exports = Self => { const user = await models.Account.findById(userId, {fields: ['hasGrant']}, myOptions); - if (!user.hasGrant) - throw new UserError(`You don't have grant privilege`); - const userToUpdate = await models.Account.findById(id, { fields: ['id', 'name', 'hasGrant', 'roleFk', 'password'], include: { @@ -54,15 +51,22 @@ module.exports = Self => { } }, myOptions); + if (!user.hasGrant) + throw new UserError(`You don't have grant privilege`); + + const hasRoleFromUser = await models.Account.hasRole(userId, userToUpdate.role().name, myOptions); + + if (!hasRoleFromUser) + throw new UserError(`You don't own the role and you can't assign it to another user`); + if (hasGrant != null) userToUpdate.hasGrant = hasGrant; if (roleFk) { const role = await models.Role.findById(roleFk, {fields: ['name']}, myOptions); const hasRole = await models.Account.hasRole(userId, role.name, myOptions); - const hasRoleFromUser = await models.Account.hasRole(userId, userToUpdate.role().name, myOptions); - if (!hasRole || !hasRoleFromUser) + if (!hasRole) throw new UserError(`You don't own the role and you can't assign it to another user`); userToUpdate.roleFk = roleFk; diff --git a/back/methods/account/specs/privileges.spec.js b/back/methods/account/specs/privileges.spec.js index 959130e8b..edfe0f03f 100644 --- a/back/methods/account/specs/privileges.spec.js +++ b/back/methods/account/specs/privileges.spec.js @@ -4,6 +4,8 @@ describe('account privileges()', () => { const employeeId = 1; const developerId = 9; const sysadminId = 66; + const itBossId = 104; + const rootId = 100; const clarkKent = 1103; it('should throw an error when user not has privileges', async() => { @@ -33,12 +35,26 @@ describe('account privileges()', () => { try { const options = {transaction: tx}; - const root = await models.Role.findOne({ - where: { - name: 'root' - } - }, options); - await models.Account.privileges(ctx, employeeId, root.id, null, options); + await models.Account.privileges(ctx, employeeId, rootId, null, options); + + await tx.rollback(); + } catch (e) { + error = e; + await tx.rollback(); + } + + expect(error.message).toContain(`You don't own the role and you can't assign it to another user`); + }); + + it('should throw an error when user has privileges but not has the role from user', async() => { + const ctx = {req: {accessToken: {userId: sysadminId}}}; + const tx = await models.Account.beginTransaction({}); + + let error; + try { + const options = {transaction: tx}; + + await models.Account.privileges(ctx, itBossId, developerId, null, options); await tx.rollback(); } catch (e) {