#6744 fix worker setPassword #2027

Merged
jorgep merged 18 commits from 6744-fixWorkerSetPassword into dev 2024-03-15 09:48:14 +00:00
8 changed files with 89 additions and 59 deletions

View File

@ -220,5 +220,7 @@
"Shelving not valid": "Shelving not valid", "Shelving not valid": "Shelving not valid",
"printerNotExists": "The printer does not exist", "printerNotExists": "The printer does not exist",
"There are not picking tickets": "There are not picking tickets", "There are not picking tickets": "There are not picking tickets",
"ticketCommercial": "The ticket {{ ticket }} for the salesperson {{ salesMan }} is in preparation. (automatically generated message)" "ticketCommercial": "The ticket {{ ticket }} for the salesperson {{ salesMan }} is in preparation. (automatically generated message)",
"This password can only be changed by the user themselves": "This password can only be changed by the user themselves",
"They're not your subordinate": "They're not your subordinate"
} }

View File

@ -346,5 +346,7 @@
"CountryFK cannot be empty": "El país no puede estar vacío", "CountryFK cannot be empty": "El país no puede estar vacío",
"Cmr file does not exist": "El archivo del cmr no existe", "Cmr file does not exist": "El archivo del cmr no existe",
"You are not allowed to modify the alias": "No estás autorizado a modificar el alias", "You are not allowed to modify the alias": "No estás autorizado a modificar el alias",
"The address of the customer must have information about Incoterms and Customs Agent": "El consignatario del cliente debe tener informado Incoterms y Agente de aduanas" "The address of the customer must have information about Incoterms and Customs Agent": "El consignatario del cliente debe tener informado Incoterms y Agente de aduanas",
"This password can only be changed by the user themselves": "Esta contraseña solo puede ser modificada por el propio usuario",
"They're not your subordinate": "No es tu subordinado/a."
} }

View File

@ -1,4 +1,7 @@
const ForbiddenError = require('vn-loopback/util/forbiddenError');
const {models} = require('vn-loopback/server/server');
module.exports = Self => { module.exports = Self => {
require('../methods/account/sync')(Self); require('../methods/account/sync')(Self);
require('../methods/account/sync-by-id')(Self); require('../methods/account/sync-by-id')(Self);
@ -7,4 +10,11 @@ module.exports = Self => {
require('../methods/account/logout')(Self); require('../methods/account/logout')(Self);
require('../methods/account/change-password')(Self); require('../methods/account/change-password')(Self);
require('../methods/account/set-password')(Self); require('../methods/account/set-password')(Self);
Self.setUnverifiedPassword = async(id, pass, options) => {
const {emailVerified} = await models.VnUser.findById(id, {fields: ['emailVerified']}, options);
if (emailVerified) throw new ForbiddenError('This password can only be changed by the user themselves');
await models.VnUser.setPassword(id, pass, options);
};
jorgep marked this conversation as resolved
Review

porque activas el correo aqui? si el usuario no consigue entrar por cualquier razón no debería verificarse el mail

porque activas el correo aqui? si el usuario no consigue entrar por cualquier razón no debería verificarse el mail
Review

me lo pidio expresamente @juan . A que te refieres con que no consigue entrar?

me lo pidio expresamente @juan . A que te refieres con que no consigue entrar?
Review

Considero que és un mal enfoque.
El hecho de cambiar la contraseña no es lo mismo que hacer un login satisfactorio.
Bajo mi punto de vista poner el email verificado a true sólo se debería hacer cuando el usuario ha conseguido hacer un login satisfactorio

Considero que és un mal enfoque. El hecho de cambiar la contraseña no es lo mismo que hacer un login satisfactorio. Bajo mi punto de vista poner el email verificado a true sólo se debería hacer cuando el usuario ha conseguido hacer un login satisfactorio
Review

Paso la tarea a feedback pues

Paso la tarea a feedback pues
Review

después de hablar con Juan quitar await user.updateAttribute('emailVerified', true, options);
poner el email verificado a true sólo se debería hacer cuando el usuario ha conseguido hacer un login satisfactorio

después de hablar con Juan quitar await user.updateAttribute('emailVerified', true, options); poner el email verificado a true sólo se debería hacer cuando el usuario ha conseguido hacer un login satisfactorio
}; };

View File

@ -1,31 +1,29 @@
const UserError = require('vn-loopback/util/user-error'); const ForbiddenError = require('vn-loopback/util/forbiddenError');
module.exports = Self => { module.exports = Self => {
Self.remoteMethodCtx('setPassword', { Self.remoteMethodCtx('setPassword', {
description: 'Set a new password', description: 'Set a new password',
accepts: [ accepts: [{
{ arg: 'id',
arg: 'workerFk',
type: 'number', type: 'number',
required: true, required: true,
description: 'The worker id', description: 'The worker id',
}, http: {source: 'path'}
{ }, {
arg: 'newPass', arg: 'newPass',
type: 'String', type: 'String',
required: true, required: true,
description: 'The new worker password' description: 'The new worker password'
} }],
],
http: { http: {
path: `/:id/setPassword`, path: `/:id/setPassword`,
verb: 'PATCH' verb: 'PATCH'
} }
}); });
Self.setPassword = async(ctx, options) => { Self.setPassword = async(ctx, id, newPass, options) => {
const models = Self.app.models; const models = Self.app.models;
const myOptions = {}; const myOptions = {};
const {args} = ctx;
let tx; let tx;
if (typeof options == 'object') if (typeof options == 'object')
Object.assign(myOptions, options); Object.assign(myOptions, options);
if (!myOptions.transaction) { if (!myOptions.transaction) {
@ -33,11 +31,10 @@ module.exports = Self => {
myOptions.transaction = tx; myOptions.transaction = tx;
} }
try { try {
const isSubordinate = await models.Worker.isSubordinate(ctx, args.workerFk, myOptions); const isSubordinate = await Self.isSubordinate(ctx, id, myOptions);
if (!isSubordinate) throw new UserError('You don\'t have enough privileges.'); if (!isSubordinate) throw new ForbiddenError('They\'re not your subordinate');
await models.VnUser.setPassword(args.workerFk, args.newPass, myOptions); await models.Account.setUnverifiedPassword(id, newPass, myOptions);
jorgep marked this conversation as resolved
Review

no podemos poner aquí el contenido de setUnverifiedPassword?
es necesario crear ese método?

no podemos poner aquí el contenido de setUnverifiedPassword? es necesario crear ese método?
Review

me lo pidio exprasemente @juan

me lo pidio exprasemente @juan
await models.VnUser.updateAll({id: args.workerFk}, {emailVerified: true}, myOptions);
if (tx) await tx.commit(); if (tx) await tx.commit();
} catch (e) { } catch (e) {

View File

@ -1,31 +1,30 @@
const UserError = require('vn-loopback/util/user-error'); const {models} = require('vn-loopback/server/server');
const models = require('vn-loopback/server/server').models;
describe('worker setPassword()', () => { describe('worker setPassword()', () => {
let ctx; let ctx;
const newPass = 'H3rn4d3z#';
const employeeId = 1;
const managerId = 20;
const administrativeId = 5;
beforeAll(() => { beforeAll(() => {
ctx = { ctx = {
req: { req: {
accessToken: {}, accessToken: {userId: managerId},
headers: {origin: 'http://localhost'} headers: {origin: 'http://localhost'}
}, },
args: {workerFk: 9}
}; };
}); });
beforeEach(() => { it('should change the password if it is a subordinate and the email is not verified', async() => {
ctx.req.accessToken.userId = 20;
ctx.args.newPass = 'H3rn4d3z#';
});
it('should change the password', async() => {
const tx = await models.Worker.beginTransaction({}); const tx = await models.Worker.beginTransaction({});
try { try {
const options = {transaction: tx}; const options = {transaction: tx};
await models.Worker.setPassword(ctx, options); await models.Worker.setPassword(ctx, employeeId, newPass, options);
const isNewPass = await passHasBeenChanged(employeeId, newPass, options);
expect(isNewPass).toBeTrue();
await tx.rollback(); await tx.rollback();
} catch (e) { } catch (e) {
await tx.rollback(); await tx.rollback();
@ -33,29 +32,48 @@ describe('worker setPassword()', () => {
} }
}); });
it('should throw an error: Password does not meet requirements', async() => { it('should not change the password if it is a subordinate and the email is verified', async() => {
const tx = await models.Collection.beginTransaction({}); const tx = await models.Worker.beginTransaction({});
ctx.args.newPass = 'Hi';
try { try {
const options = {transaction: tx}; const options = {transaction: tx};
await models.Worker.setPassword(ctx, options); await models.VnUser.updateAll({id: employeeId}, {emailVerified: true}, options);
await models.Worker.setPassword(ctx, employeeId, newPass, options);
await tx.rollback();
} catch (e) {
expect(e.message).toEqual(`This password can only be changed by the user themselves`);
await tx.rollback();
}
});
it('should not change the password if it is not a subordinate', async() => {
const tx = await models.Worker.beginTransaction({});
try {
const options = {transaction: tx};
await models.Worker.setPassword(ctx, administrativeId, newPass, options);
await tx.rollback();
} catch (e) {
expect(e.message).toEqual(`They're not your subordinate`);
await tx.rollback();
}
});
it('should throw an error: Password does not meet requirements', async() => {
const tx = await models.Worker.beginTransaction({});
const newPass = 'Hi';
try {
const options = {transaction: tx};
await models.Worker.setPassword(ctx, employeeId, newPass, options);
await tx.rollback(); await tx.rollback();
} catch (e) { } catch (e) {
expect(e.sqlMessage).toEqual('Password does not meet requirements'); expect(e.sqlMessage).toEqual('Password does not meet requirements');
await tx.rollback(); await tx.rollback();
} }
}); });
});
it('should throw an error: You don\'t have enough privileges.', async() => { const passHasBeenChanged = async(userId, pass, options) => {
ctx.req.accessToken.userId = 5; const user = await models.VnUser.findById(userId, null, options);
const tx = await models.Collection.beginTransaction({}); return user.hasPassword(pass);
try { };
const options = {transaction: tx};
await models.Worker.setPassword(ctx, options);
await tx.rollback();
} catch (e) {
expect(e).toEqual(new UserError(`You don't have enough privileges.`));
await tx.rollback();
}
});
});

View File

@ -11,7 +11,7 @@
? 'Click to allow the user to be disabled' ? 'Click to allow the user to be disabled'
: 'Click to exclude the user from getting disabled'}} : 'Click to exclude the user from getting disabled'}}
</vn-item> </vn-item>
<vn-item ng-if="!$ctrl.worker.user.emailVerified" ng-click="setPassword.show()" translate> <vn-item ng-if="!$ctrl.worker.user.emailVerified && $ctrl.vnConfig.storage.currentUserWorkerId !=$ctrl.worker.id" ng-click="setPassword.show()" translate>
Review

creo que esta mal esta comprobación, en que casos quieres que se muestre?

creo que esta mal esta comprobación, en que casos quieres que se muestre?
Review

Solo si el email no está verificado y no es el mismo. Tras lo hablado con Juan, si eres tú mismo, te cambiarás la contraseña desde otro lado. @jgallego

Solo si el email no está verificado y no es el mismo. Tras lo hablado con Juan, si eres tú mismo, te cambiarás la contraseña desde otro lado. @jgallego
Change password Change password
</vn-item> </vn-item>
</slot-menu> </slot-menu>

View File

@ -69,6 +69,7 @@ class Controller extends Descriptor {
} }
] ]
}; };
return this.getData(`Workers/${this.id}`, {filter}) return this.getData(`Workers/${this.id}`, {filter})
.then(res => this.entity = res.data); .then(res => this.entity = res.data);
} }
@ -86,15 +87,14 @@ class Controller extends Descriptor {
if (this.newPassword != this.repeatPassword) if (this.newPassword != this.repeatPassword)
throw new UserError(`Passwords don't match`); throw new UserError(`Passwords don't match`);
this.$http.patch( this.$http.patch(
`Workers/${this.entity.id}/setPassword`, `Workers/${this.entity.id}/setPassword`, {newPass: this.newPassword}
{workerFk: this.entity.id, newPass: this.newPassword}
) .then(() => { ) .then(() => {
this.vnApp.showSuccess(this.$translate.instant('Password changed!')); this.vnApp.showSuccess(this.$translate.instant('Password changed!'));
}); }).then(() => this.loadData());
} }
} }
Controller.$inject = ['$element', '$scope', '$rootScope']; Controller.$inject = ['$element', '$scope', '$rootScope', 'vnConfig'];
ngModule.vnComponent('vnWorkerDescriptor', { ngModule.vnComponent('vnWorkerDescriptor', {
template: require('./index.html'), template: require('./index.html'),

View File

@ -16,6 +16,7 @@ describe('vnWorkerDescriptor', () => {
const id = 1; const id = 1;
const response = 'foo'; const response = 'foo';
$httpBackend.whenGET('UserConfigs/getUserConfig').respond({});
$httpBackend.expectRoute('GET', `Workers/${id}`).respond(response); $httpBackend.expectRoute('GET', `Workers/${id}`).respond(response);
controller.id = id; controller.id = id;
$httpBackend.flush(); $httpBackend.flush();