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.

Sign up for GitHub

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

Jump to bottom

Let user specify a new password (#13973) #15461

Closed
wants to merge 2 commits into from
Closed

Let user specify a new password (#13973) #15461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c06a3d0
Added option to let user generate password via email
sdrenth Mar 5, 2021
43833e2
Use hashing service for generating the hashes
sdrenth Mar 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions core/lexicon/en/user.inc.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
$_lang['password_gen_gen'] = 'Let MODX generate a password.';
$_lang['password_gen_method'] = 'New password method';
$_lang['password_gen_specify'] = 'Let me specify the password:';
$_lang['password_gen_user_email_specify'] = 'Let the user choose their own password via email';
$_lang['notify_new_user'] = 'Email this user about their new login for this website.';
$_lang['password_new'] = 'New Password';
$_lang['password_notification'] = 'Password Notification';
Expand Down Expand Up @@ -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>';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's avoid using HTML tags inside lexicons. It becomes a hell while translating such stuff.

47 changes: 47 additions & 0 deletions core/src/Revolution/Processors/Security/User/Create.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@


use Exception;
use MODX\Revolution\Hashing\modHashing;
use MODX\Revolution\Processors\Model\CreateProcessor;
use MODX\Revolution\Processors\Processor;
use MODX\Revolution\modUser;
Expand Down Expand Up @@ -232,6 +233,52 @@ public function sendNotificationEmail() {
'html' => true,
]);
}

if ($this->getProperty('passwordgenmethod') === 'user_email_specify' && $this->modx->getService('hashing', modHashing::class)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be refactored, to move this implementation into a method to avoid such long if statement.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sdrenth Please make changes to promote PR

$activationHash = $this->modx->hashing->getHash('md5', 'hashing.modMD5', [])->hash($this->object->get('email') . '/' . $this->object->get('id'));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/** @var modRegistry $registry */
$registry = $this->modx->getService('registry', 'registry.modRegistry');
/** @var modRegister $register */
$register = $registry->getRegister('user', 'registry.modDbRegister');
$register->connect();
$register->subscribe('/pwd/change/');
$register->send('/pwd/change/', [$activationHash => $this->object->get('username')], ['ttl' => 86400]);

// Send activation email
$message = $this->modx->lexicon('user_password_email');
$placeholders = array_merge($this->modx->config, $this->object->toArray());
$placeholders['hash'] = $activationHash;

// Store previous placeholders
$ph = $this->modx->placeholders;
// now set those useful for modParser
$this->modx->setPlaceholders($placeholders);
$this->modx->getParser()->processElementTags('', $message, true, false, '[[', ']]', [], 10);
$this->modx->getParser()->processElementTags('', $message, true, true, '[[', ']]', [], 10);
// Then restore previous placeholders to prevent any breakage
$this->modx->placeholders = $ph;

$this->modx->getService('smarty', 'smarty.modSmarty', '', ['template_dir' => $this->modx->getOption('manager_path') . 'templates/default/']);

$this->modx->smarty->assign('_config', $this->modx->config);
$this->modx->smarty->assign('content', $message, true);

$sent = $this->object->sendEmail(
$this->modx->smarty->fetch('email/default.tpl'),
[
'from' => $this->modx->getOption('emailsender'),
'fromName' => $this->modx->getOption('site_name'),
'sender' => $this->modx->getOption('emailsender'),
'subject' => $this->modx->lexicon('user_password_email_subject'),
'html' => true,
]
);

if (!$sent) {
return $this->failure($this->modx->lexicon('error_sending_email_to') . $this->object->get('email'));
}
}
}

/**
Expand Down
76 changes: 68 additions & 8 deletions core/src/Revolution/Processors/Security/User/Update.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
namespace MODX\Revolution\Processors\Security\User;


use MODX\Revolution\Hashing\modHashing;
use MODX\Revolution\Processors\Model\UpdateProcessor;
use MODX\Revolution\Processors\Processor;
use MODX\Revolution\modSystemEvent;
Expand Down Expand Up @@ -285,6 +286,7 @@ public function setUserGroups() {
*/
public function afterSave() {
$this->setUserGroups();
$this->sendNotificationEmail();
if ($this->activeStatusChanged) {
$this->fireAfterActiveStatusChange();
}
Expand All @@ -305,20 +307,78 @@ public function fireAfterActiveStatusChange() {
);
}

/**
* Send the password notification email, if specified
*
* @return void
* @throws Exception
*/
public function sendNotificationEmail() {
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'));

/** @var modRegistry $registry */
$registry = $this->modx->getService('registry', 'registry.modRegistry');
/** @var modRegister $register */
$register = $registry->getRegister('user', 'registry.modDbRegister');
$register->connect();
$register->subscribe('/pwd/change/');
$register->send('/pwd/change/', [$activationHash => $this->object->get('username')], ['ttl' => 86400]);

$this->modx->lexicon->load('core:login');

// Send activation email
$message = $this->modx->lexicon('user_password_email');
$placeholders = array_merge($this->modx->config, $this->object->toArray());
$placeholders['hash'] = $activationHash;

// Store previous placeholders
$ph = $this->modx->placeholders;
// now set those useful for modParser
$this->modx->setPlaceholders($placeholders);
$this->modx->getParser()->processElementTags('', $message, true, false, '[[', ']]', [], 10);
$this->modx->getParser()->processElementTags('', $message, true, true, '[[', ']]', [], 10);
// Then restore previous placeholders to prevent any breakage
$this->modx->placeholders = $ph;

$this->modx->getService('smarty', 'smarty.modSmarty', '', ['template_dir' => $this->modx->getOption('manager_path') . 'templates/default/']);

$this->modx->smarty->assign('_config', $this->modx->config);
$this->modx->smarty->assign('content', $message, true);

$sent = $this->object->sendEmail(
$this->modx->smarty->fetch('email/default.tpl'),
[
'from' => $this->modx->getOption('emailsender'),
'fromName' => $this->modx->getOption('site_name'),
'sender' => $this->modx->getOption('emailsender'),
'subject' => $this->modx->lexicon('user_password_email_subject'),
'html' => true,
]
);

if (!$sent) {
return $this->failure($this->modx->lexicon('error_sending_email_to') . $this->object->get('email'));
}
}
}

/**
* {@inheritDoc}
* @return array|string
*/
public function cleanup() {
$passwordNotifyMethod = $this->getProperty('passwordnotifymethod');
if (!empty($passwordNotifyMethod) && !empty($this->newPassword) && $passwordNotifyMethod == 's') {
return $this->success($this->modx->lexicon('user_updated_password_message',
[
'password' => $this->newPassword,
]
), $this->object);
$passwordGenerationMethod = $this->getProperty('passwordgenmethod');
if (!empty($passwordGenerationMethod) && !empty($this->newPassword)) {
return $this->success(
$this->modx->lexicon('user_updated_password_message',
array(
'password' => $this->newPassword,
)
),
$this->object);
} else {
return $this->success('',$this->object);
return $this->success('', $this->object);
}
}
}
6 changes: 4 additions & 2 deletions core/src/Revolution/Processors/Security/User/Validation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,14 @@ public function alreadyExists($name) {
public function checkPassword() {
$newPassword = $this->processor->getProperty('newpassword',null);
$id = $this->processor->getProperty('id');
if ($newPassword !== null && $newPassword != 'false' || empty($id)) {

$passwordGenerationMethod = $this->processor->getProperty('passwordgenmethod','g');
if ($passwordGenerationMethod !== 'user_email_specify' && ($newPassword !== null && $newPassword != 'false' || empty($id))) {
$passwordNotifyMethod = $this->processor->getProperty('passwordnotifymethod',null);
if (empty($passwordNotifyMethod)) {
$this->processor->addFieldError('password_notify_method',$this->modx->lexicon('user_err_not_specified_notification_method'));
}
$passwordGenerationMethod = $this->processor->getProperty('passwordgenmethod','g');

if ($passwordGenerationMethod == 'g') {
$autoPassword = $this->user->generatePassword();
$this->user->set('password', $autoPassword);
Expand Down
7 changes: 7 additions & 0 deletions manager/assets/modext/widgets/security/modx.panel.user.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,13 @@ Ext.extend(MODx.panel.User,MODx.FormPanel,{
,xtype: 'radio'
,inputValue: 'spec'
,value: 'spec'
},{
id: 'modx-user-password-genmethod-user-email-specify'
,name: 'passwordgenmethod'
,boxLabel: _('password_gen_user_email_specify')
,xtype: 'radio'
,inputValue: 'user_email_specify'
,value: 'user_email_specify'
}]
},{
id: 'modx-user-panel-newpassword'
Expand Down