Let user specify a new password (#13973) #15461
Conversation
|
@sdrenth please change target branch |
|
@JoshuaLuckers Thanks! :) |
sdrenth
changed the title
Mark-H
changed the title
|
Maybe the sendNotificationEmail method can be moved to a helper class instead of duplicating the code. |
|
@Jako I agree with your comment but think that should be a separate issue, because the duplicate code was already in here (and there's more). Also where should the helper class be placed? |
|
The user needs access to the manager context, otherwise an access denied error will show up (only after you try to set your own password). |
|
@JoshuaLuckers Is that just an FYI? I would think that for frontend users you would have a registration form and a forgot password form. You don't want to guide those users to the manager part of the application. I could also rename that option to "Let the user choose their own password via email (Needs manager access)" to clarify this? What do you think? |
|
First of all, I like this new feature! I'm having some doubts about the flow of this feature. If a user doesn't have access to the manager it still receives the email and the option to set the password. Only after setting your password you get the access denied message. |
Ibochkarev
added
proposal
| @@ -200,3 +201,5 @@ | |||
| $_lang['users'] = 'Users'; | |||
| $_lang['user_createdon'] = 'Created On'; | |||
| $_lang['user_createdon_desc'] = 'The date the user was created.'; | |||
| $_lang['user_password_email_subject'] = 'Set up your password'; | |||
| $_lang['user_password_email'] = '<h2>Set up your password</h2><p>We received a request to set up your MODX Revolution password. You can set up your password by clicking the button below and following the instructions on screen.</p><p class="center"><a href="[[+url_scheme]][[+http_host]][[+manager_url]]?modhash=[[+hash]]" class="btn">Set up my password</a></p><p class="small">If you did not send this request, please ignore this email.</p>'; | |||
There was a problem hiding this comment.
Let's avoid using HTML tags inside lexicons. It becomes a hell while translating such stuff.
| @@ -232,6 +233,52 @@ public function sendNotificationEmail() { | |||
| 'html' => true, | |||
| ]); | |||
| } | |||
|
|
|||
| if ($this->getProperty('passwordgenmethod') === 'user_email_specify' && $this->modx->getService('hashing', modHashing::class)) { | |||
There was a problem hiding this comment.
It should be refactored, to move this implementation into a method to avoid such long if statement.
There was a problem hiding this comment.
@sdrenth Please make changes to promote PR
| @@ -232,6 +233,52 @@ public function sendNotificationEmail() { | |||
| 'html' => true, | |||
| ]); | |||
| } | |||
|
|
|||
| if ($this->getProperty('passwordgenmethod') === 'user_email_specify' && $this->modx->getService('hashing', modHashing::class)) { | |||
| $activationHash = $this->modx->hashing->getHash('md5', 'hashing.modMD5', [])->hash($this->object->get('email') . '/' . $this->object->get('id')); | |||
There was a problem hiding this comment.
This is not a secure hash/tokens and introduces a potential vulnerability. The scope would be limited because the "tokens" have a limited lifetime after manually being created by an admin, however it makes the hash very predictable.
In 3.x we can use random_bytes, so I would suggest:
$activationHash = bin2hex(random_bytes(32));
for a 64-character secure random token.
|
As this PR has gone stale but this is still something that would be great to have, I've replaced it with a new one over at #16519. |
What does it do?
This adds the possibility to let a user specify their own MODX password, using the same flow as the current manager Forgot password flow by sending a hashed link in the email which allows users to setup their new password.
Credits to @jonleverrier for coming up with this solution.
New password options:
Set up password mail:
Why is it needed?
There used to be an option to send an email to the MODX user which contains their MODX account password. This has been removed (for security purposes). This basically readds that functionality but in a saver way.
Related issue(s)/PR(s)
Sterc#22
#13973
Sterc#31