#6744 fix worker setPassword #2027

Merged
jorgep merged 18 commits from 6744-fixWorkerSetPassword into dev 2024-03-15 09:48:14 +00:00
7 changed files with 23 additions and 36 deletions
Showing only changes of commit 744dd61561 - Show all commits

View File

@ -209,5 +209,6 @@
"You cannot update these fields": "You cannot update these fields", "You cannot update these fields": "You cannot update these fields",
"CountryFK cannot be empty": "Country cannot be empty", "CountryFK cannot be empty": "Country cannot be empty",
"You are not allowed to modify the alias": "You are not allowed to modify the alias", "You are not allowed to modify the alias": "You are not allowed to modify the alias",
"You already have the mailAlias": "You already have the mailAlias" "You already have the mailAlias": "You already have the mailAlias",
"The email has been already verified": "The email has been already verified"
} }

View File

@ -344,5 +344,6 @@
"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",
"No tickets to invoice": "No hay tickets para facturar" "No tickets to invoice": "No hay tickets para facturar",
"The email has been already verified": "El correo ya ha sido verificado"
} }

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,12 @@ 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 user = await models.VnUser.findById(id, null, options);
if (user.emailVerified) throw new ForbiddenError('The email has been already verified');

Si alguien intenta cambiar un password y le mostramos este mensaje de error, no va a saber relacionarlo..
Pensando en Norman yo mostraria algo así como "Esta contraseña solo puede ser modificada por el propio usuario"

Y asegurate que VnUser.setPassword tiene los permisos correctos y nadie lo puede llamar.

Si alguien intenta cambiar un password y le mostramos este mensaje de error, no va a saber relacionarlo.. Pensando en Norman yo mostraria algo así como "Esta contraseña solo puede ser modificada por el propio usuario" Y asegurate que VnUser.setPassword tiene los permisos correctos y nadie lo puede llamar.

En principio lo reviso con isSubordinate en worker->setPassword y dentro de account->setUnverifiedPassword compruebo si el email está verificado. Hay que cambiar el enfoque? @juan @jgallego . setUnverifiedPassword no se puede llamar desde el exterior(http request), solo internamente.

En principio lo reviso con isSubordinate en worker->setPassword y dentro de account->setUnverifiedPassword compruebo si el email está verificado. Hay que cambiar el enfoque? @juan @jgallego . setUnverifiedPassword no se puede llamar desde el exterior(http request), solo internamente.

Traducción cambiada

Traducción cambiada
await models.VnUser.setPassword(id, pass, options);
await user.updateAttribute('emailVerified', true, 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

@ -19,8 +19,7 @@ module.exports = Self => {
verb: 'PATCH' verb: 'PATCH'
} }
}); });
Self.setPassword = async(ctx, workerId, newPass, options) => { Self.setPassword = async(ctx, id, newPass, options) => {
const userId = ctx.req.accessToken.userId;
const models = Self.app.models; const models = Self.app.models;
const myOptions = {}; const myOptions = {};
let tx; let tx;
@ -31,17 +30,11 @@ module.exports = Self => {
tx = await Self.beginTransaction({}); tx = await Self.beginTransaction({});
myOptions.transaction = tx; myOptions.transaction = tx;
} }
try { try {
const isHimself = userId === workerId; const isSubordinate = await Self.isSubordinate(ctx, id, myOptions);
const isSubordinate = await Self.isSubordinate(ctx, workerId, myOptions); if (!isSubordinate) throw new UserError('You don\'t have enough privileges.');
jorgep marked this conversation as resolved Outdated
Outdated
Review

Lanzar ForbiddenError indicando en el mensaje que no es subordinado.

Lanzar ForbiddenError indicando en el mensaje que no es subordinado.
const {emailVerified} = await models.VnUser.findById(workerId, {fields: ['emailVerified']}, myOptions);
jorgep marked this conversation as resolved Outdated

isHimself

isHimself
Outdated
Review

No pondría aquí isHimself, para cambiarse la contraseña uno mismo que se utilice el método tradicional que yahace las comprobaciones de seguridad correspondientes

No pondría aquí `isHimself`, para cambiarse la contraseña uno mismo que se utilice el método tradicional que yahace las comprobaciones de seguridad correspondientes
if (isHimself || (isSubordinate && !emailVerified)) { 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.setPassword(workerId, newPass, myOptions);
await models.VnUser.updateAll({id: workerId}, {emailVerified: true}, myOptions);
} else
throw new UserError('You don\'t have enough privileges.');
if (tx) await tx.commit(); if (tx) await tx.commit();
} catch (e) { } catch (e) {

View File

@ -42,23 +42,7 @@ describe('worker setPassword()', () => {
await tx.rollback(); await tx.rollback();
} catch (e) { } catch (e) {
expect(e.message).toEqual(`You don't have enough privileges.`); expect(e.message).toEqual(`The email has been already verified`);
await tx.rollback();
}
});
it('should change the password if it is himself', async() => {
const tx = await models.Worker.beginTransaction({});
try {
const options = {transaction: tx};
await models.VnUser.updateAll({id: managerId}, {emailVerified: true}, options);
await models.Worker.setPassword(ctx, managerId, newPass, options);
const isNewPass = await passHasBeenChanged(managerId, newPass, options);
expect(isNewPass).toBeTrue();
await tx.rollback();
} catch (e) {
await tx.rollback(); 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 || $ctrl.id == $ctrl.userFk" 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

@ -5,9 +5,6 @@ class Controller extends Descriptor {
constructor($element, $, $rootScope) { constructor($element, $, $rootScope) {
super($element, $); super($element, $);
this.$rootScope = $rootScope; this.$rootScope = $rootScope;
this.$http.get(`UserConfigs/getUserConfig`)
.then(res => this.userFk = res.data.userFk);
} }
get worker() { get worker() {
@ -93,11 +90,11 @@ class Controller extends Descriptor {
`Workers/${this.entity.id}/setPassword`, {newPass: this.newPassword} `Workers/${this.entity.id}/setPassword`, {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'),