Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Password can not be changed from the user settings page #1375

Open
wants to merge 1 commit into
base: master
from

Conversation

@aharenaz
Copy link

aharenaz commented Jan 24, 2020

bug report #15769

The password it's never updated on user model so it's never saved on database making it impossible to change an users password.

When $newPassword is non empty, it never enters the else part so it doesn't updates the model.

Steps to reproduce this bug:
1- Make a new clean installation (i.e. using limesurvey4.0.1+200120.tar.gz, PHP 7, mariadb).
2- Login as admin, using "password" as password
3- Go to user account, and try to change the pasword to another one
-> The feedback the page gives is that changes were successfully applied
-> Password isn't really updated

} else {
// We can update
$oUserModel->setPassword($newPassword);
return $this->getController()->redirect(array("admin/user/sa/personalsettings"));

This comment has been minimized.

Copy link
@Shnoulle

Shnoulle Jan 24, 2020

Collaborator

redirect is a redirect, no return please

This comment has been minimized.

Copy link
@aharenaz

aharenaz Jan 24, 2020

Author

Here is needed so action returns immediately showing the appropiate flash message (https://www.yiiframework.com/doc/api/2.0/yii-web-controller#redirect()-detail).
Without a return, it would show "Your personal settings were successfully saved." leading to confusion.
Otherwise it wold require more changes on then follow up code to check if errors happened on password change for showing the appropiate flash message.
Any suggestion at this respect?

This comment has been minimized.

Copy link
@Shnoulle

Shnoulle Jan 24, 2020

Collaborator

I mean :

Since $this->getController()->redirect(array("admin/user/sa/personalsettings")); is a redirect : you CAN'T return it …

It's an end : no return

See https://www.yiiframework.com/doc/api/1.1/CHttpRequest#redirect-detail

return void …

This comment has been minimized.

Copy link
@aharenaz

aharenaz Jan 24, 2020

Author

OK, thanks, you are right!
I'll change that.

@Shnoulle

This comment has been minimized.

Copy link
Collaborator

Shnoulle commented Jan 24, 2020

A mantis issue please ? With a how to reproduce ?

Yii::app()->setFlashMessage(gT("The password can't be empty."), 'error');
$this->getController()->redirect(array("admin/user/sa/personalsettings"));
} else {

This comment has been minimized.

Copy link
@Shnoulle

Shnoulle Jan 24, 2020

Collaborator

In your method : seem this else must be remove

This comment has been minimized.

Copy link
@aharenaz

aharenaz Jan 24, 2020

Author

It's already removed in my code ... your quote is from the original one

This comment has been minimized.

Copy link
@Shnoulle

Shnoulle Jan 24, 2020

Collaborator

It's the one in this pull request …

This comment has been minimized.

Copy link
@Shnoulle

Shnoulle Jan 24, 2020

Collaborator

Right :)

@Shnoulle

This comment has been minimized.

Copy link
Collaborator

Shnoulle commented Jan 24, 2020

My opinion :

If password was not updated : add a error / alert, but don't redirect before saving other data.

Then when user save (with a bad password)

  1. Add a alert/error with error setFlashMessage
  2. tag it must come back to user edit.
  3. Save other data, and add success setFlashMessage
  4. If must redirect : check it here even if it's saveandclose

Because (for example) if user want to upadte is name at same time than his password : you must save is name …

Maybe : when save with success : put a different string

Yii::app()->setFlashMessage(gT("Your personal settings were successfully saved."));

“Password excepted, your personal settings were successfully saved.”

@Shnoulle

This comment has been minimized.

Copy link
Collaborator

Shnoulle commented Jan 24, 2020

PS :

public function setFlashMessage($message, $type = 'success')

Allow multiple message : then can have error + warning + success + success + … …

Just add message :)

@olleharstedt

This comment has been minimized.

Copy link
Contributor

olleharstedt commented Jan 24, 2020

As Denis said, please add a bug report in bugs.limesurvey.org with precise step-by-step instructions on how to reproduce.

@olleharstedt

This comment has been minimized.

Copy link
Contributor

olleharstedt commented Jan 24, 2020

Oh, yeah, just copy your "Steps to reproduce this bug:" to the ticket.

@aharenaz

This comment has been minimized.

Copy link
Author

aharenaz commented Jan 24, 2020

Oh, yeah, just copy your "Steps to reproduce this bug:" to the ticket.

Created bug report #15769

@aharenaz aharenaz closed this Jan 24, 2020
@aharenaz aharenaz reopened this Jan 24, 2020
@@ -643,28 +643,31 @@ public function personalsettings()
$error = $oUserModel->checkPasswordStrength($newPassword);
if ($error) {
Yii::app()->setFlashMessage(gT($error), 'error');
$this->getController()->redirect(array("admin/user/sa/personalsettings"));
return $this->getController()->redirect(array("admin/user/sa/personalsettings"));

This comment has been minimized.

Copy link
@olleharstedt

olleharstedt Jan 24, 2020

Contributor

Remove return, please.

// Always check password
Yii::app()->setFlashMessage(gT("Your new password was not saved because the old password was wrong."), 'error');
$this->getController()->redirect(array("admin/user/sa/personalsettings"));
} elseif (trim($oldPassword) === trim($newPassword)) {
return $this->getController()->redirect(array("admin/user/sa/personalsettings"));

This comment has been minimized.

Copy link
@olleharstedt

olleharstedt Jan 24, 2020

Contributor

Remove return.

//First test if old and new password are identical => no need to save it (or ?)
Yii::app()->setFlashMessage(gT("Your new password was not saved because it matches the old password."), 'error');
$this->getController()->redirect(array("admin/user/sa/personalsettings"));
} elseif (trim($newPassword) !== trim($repeatPassword)) {
return $this->getController()->redirect(array("admin/user/sa/personalsettings"));

This comment has been minimized.

Copy link
@olleharstedt

olleharstedt Jan 24, 2020

Contributor

Same.

//Then test the new password and the repeat password for identity
Yii::app()->setFlashMessage(gT("Your new password was not saved because the passwords did not match."), 'error');
$this->getController()->redirect(array("admin/user/sa/personalsettings"));
return $this->getController()->redirect(array("admin/user/sa/personalsettings"));

This comment has been minimized.

Copy link
@olleharstedt

olleharstedt Jan 24, 2020

Contributor

Same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.