From 56e3578e443ebffca30b7a89b8b6b450da2dfda2 Mon Sep 17 00:00:00 2001 From: benjamin Date: Thu, 28 Nov 2024 16:04:34 +0000 Subject: [PATCH] standaloneusers - split User.PasswordResetEmail into public and private actions --- .../Standaloneusers/Page/ResetPassword.php | 4 +- .../WorkflowMessage/PasswordReset.php | 37 +++--- .../Civi/Api4/Action/User/PasswordReset.php | 96 +++++++++++++++- ...eset.php => RequestPasswordResetEmail.php} | 51 +-------- .../Action/User/SendPasswordResetEmail.php | 106 ++++++++++++++++++ ext/standaloneusers/Civi/Api4/User.php | 2 +- .../Civi/Standalone/Security.php | 99 ---------------- ext/standaloneusers/ang/crmResetPassword.js | 4 +- .../crmResetPassword/crmResetPassword.html | 2 +- .../phpunit/Civi/Standalone/SecurityTest.php | 20 ++-- 10 files changed, 234 insertions(+), 187 deletions(-) rename ext/standaloneusers/Civi/Api4/Action/User/{SendPasswordReset.php => RequestPasswordResetEmail.php} (53%) create mode 100644 ext/standaloneusers/Civi/Api4/Action/User/SendPasswordResetEmail.php diff --git a/ext/standaloneusers/CRM/Standaloneusers/Page/ResetPassword.php b/ext/standaloneusers/CRM/Standaloneusers/Page/ResetPassword.php index 6017a8f2843b..489d15f05a6c 100644 --- a/ext/standaloneusers/CRM/Standaloneusers/Page/ResetPassword.php +++ b/ext/standaloneusers/CRM/Standaloneusers/Page/ResetPassword.php @@ -1,7 +1,7 @@ checkPasswordResetToken($token, FALSE)) { + if (!PasswordReset::checkPasswordResetToken($token, FALSE)) { $token = 'invalid'; } $this->assign('token', $token); diff --git a/ext/standaloneusers/CRM/Standaloneusers/WorkflowMessage/PasswordReset.php b/ext/standaloneusers/CRM/Standaloneusers/WorkflowMessage/PasswordReset.php index 76f45c5d2412..6a9414cf7ff7 100644 --- a/ext/standaloneusers/CRM/Standaloneusers/WorkflowMessage/PasswordReset.php +++ b/ext/standaloneusers/CRM/Standaloneusers/WorkflowMessage/PasswordReset.php @@ -50,35 +50,24 @@ class CRM_Standaloneusers_WorkflowMessage_PasswordReset extends GenericWorkflowM public $usernameHtml; /** - * @var array + * Load the required tplParams */ - protected $logParams; - - /** - * Generate/regenerate a token for the user and load the tplParams - */ - public function setDataFromUser(array $user, string $token) { + public function setRequiredParams( + string $username, + string $email, + int $contactId, + string $token + ) { $resetUrlPlaintext = \CRM_Utils_System::url('civicrm/login/password', ['token' => $token], TRUE, NULL, FALSE); - $resetUrlHtml = htmlspecialchars($resetUrlPlaintext); - $this->logParams = [ - 'userID' => $user['id'], - 'contactID' => $user['contact_id'], - 'username' => $user['username'], - 'email' => $user['uf_name'], - 'url' => $resetUrlPlaintext, - ]; + $this ->setResetUrlPlaintext($resetUrlPlaintext) - ->setResetUrlHtml($resetUrlHtml) - ->setUsernamePlaintext($user['username']) - ->setUsernameHtml(htmlspecialchars($user['username'])) - ->setTo(['name' => $user['username'], 'email' => $user['uf_name']]) - ->setContactID($user['contact_id']); + ->setResetUrlHtml(htmlspecialchars($resetUrlPlaintext)) + ->setUsernamePlaintext($username) + ->setUsernameHtml(htmlspecialchars($username)) + ->setTo(['name' => $username, 'email' => $email]) + ->setContactID($contactId); return $this; } - public function getParamsForLog(): array { - return $this->logParams; - } - } diff --git a/ext/standaloneusers/Civi/Api4/Action/User/PasswordReset.php b/ext/standaloneusers/Civi/Api4/Action/User/PasswordReset.php index b4c01b9cfcd7..25ec0c83a467 100644 --- a/ext/standaloneusers/Civi/Api4/Action/User/PasswordReset.php +++ b/ext/standaloneusers/Civi/Api4/Action/User/PasswordReset.php @@ -1,19 +1,23 @@ checkPasswordResetToken($this->token); + $userID = self::checkPasswordResetToken($this->token); if (!$userID) { throw new API_Exception("Invalid token."); } @@ -53,4 +57,88 @@ public function _run(Result $result) { \Civi::log()->info("Changed password for user {userID} via User.PasswordReset", compact('userID')); } + /** + * Generate and store a token on the User record. + * @internal (only public for use in SecurityTest) + * + * @param int $userID + * @param int $minutes the number of minutes the token should be valid for + * + * @return string + * The token + */ + public static function updateToken(int $userID, int $minutes = 60): string { + // Generate a JWT that expires in 1 hour. + // We'll store this on the User record, that way invalidating any previous token that may have been generated. + $expires = time() + 60 * $minutes; + $token = \Civi::service('crypto.jwt')->encode([ + 'exp' => $expires, + 'sub' => "uid:$userID", + 'scope' => self::PASSWORD_RESET_SCOPE, + ]); + User::update(FALSE) + ->addValue('password_reset_token', $token) + ->addWhere('id', '=', $userID) + ->execute(); + + return $token; + } + + /** + * Check a password reset token matches for a User. + * + * @param string $token + * @param bool $spend + * If TRUE, and the token matches, the token is then reset; so it can only be used once. + * If FALSE no changes are made. + * + * @return NULL|int + * If int, it's the UserID + * + */ + public static function checkPasswordResetToken(string $token, bool $spend = TRUE): ?int { + try { + $decodedToken = \Civi::service('crypto.jwt')->decode($token); + } + catch (CryptoException $e) { + Civi::log()->warning('Exception while decoding JWT', ['exception' => $e]); + return NULL; + } + + $scope = $decodedToken['scope'] ?? ''; + if ($scope != self::PASSWORD_RESET_SCOPE) { + Civi::log()->warning('Expected JWT password reset, got ' . $scope); + return NULL; + } + + if (empty($decodedToken['sub']) || substr($decodedToken['sub'], 0, 4) !== 'uid:') { + Civi::log()->warning('Missing uid in JWT sub field'); + return NULL; + } + else { + $userID = substr($decodedToken['sub'], 4); + } + if (!$userID > 0) { + // Hacker + Civi::log()->warning("Rejected passwordResetToken with invalid userID.", compact('token', 'userID')); + return NULL; + } + + $matched = User::get(FALSE) + ->addWhere('id', '=', $userID) + ->addWhere('password_reset_token', '=', $token) + ->addWhere('is_active', '=', 1) + ->selectRowCount() + ->execute()->countMatched() === 1; + + if ($matched && $spend) { + $matched = User::update(FALSE) + ->addWhere('id', '=', $userID) + ->addValue('password_reset_token', NULL) + ->execute(); + } + Civi::log()->info(($matched ? 'Accepted' : 'Rejected') . " passwordResetToken for user $userID"); + return $matched ? $userID : NULL; + } + } diff --git a/ext/standaloneusers/Civi/Api4/Action/User/SendPasswordReset.php b/ext/standaloneusers/Civi/Api4/Action/User/RequestPasswordResetEmail.php similarity index 53% rename from ext/standaloneusers/Civi/Api4/Action/User/SendPasswordReset.php rename to ext/standaloneusers/Civi/Api4/Action/User/RequestPasswordResetEmail.php index 5c864d31213c..427978fd529f 100644 --- a/ext/standaloneusers/Civi/Api4/Action/User/SendPasswordReset.php +++ b/ext/standaloneusers/Civi/Api4/Action/User/RequestPasswordResetEmail.php @@ -6,11 +6,9 @@ // clicking button on form with proper token does nothing. // should redirect to login on success -use Civi; use Civi\Api4\Generic\Result; use API_Exception; use Civi\Api4\User; -use Civi\Standalone\Security; use Civi\Api4\Generic\AbstractAction; /** @@ -22,7 +20,7 @@ * * @method static setIdentifier(string $identifier) */ -class SendPasswordReset extends AbstractAction { +class RequestPasswordResetEmail extends AbstractAction { /** * Username or email of user to send email for. @@ -66,23 +64,11 @@ public function _run(Result $result) { } if ($userID) { - // (Re)generate token and store on User. - $token = static::updateToken($userID); - - $workflowMessage = Security::singleton()->preparePasswordResetWorkflow($user, $token); - if ($workflowMessage) { - // The template_params are used in the template like {$resetUrlHtml} and {$resetUrlHtml} {$usernamePlaintext} {$usernameHtml} - try { - [$sent, /*$subject, $text, $html*/] = $workflowMessage->sendTemplate(); - if (!$sent) { - throw new \RuntimeException("sendTemplate() returned unsent."); - } - Civi::log()->info("Successfully sent password reset to user {userID} ({username}) to {email}", $workflowMessage->getParamsForLog()); - } - catch (\Exception $e) { - Civi::log()->error("Failed to send password reset to user {userID} ({username}) to {email}", $workflowMessage->getParamsForLog() + ['exception' => $e]); - } - } + // we've got through all the guards - now use the + // internal API action to actually send the email + User::sendPasswordResetEmail(FALSE) + ->addWhere('id', '=', $userID) + ->execute(); } // Ensure we took at least 0.25s. The assumption is that it takes @@ -93,29 +79,4 @@ public function _run(Result $result) { usleep(1000000 * max(0, $endNoSoonerThan - microtime(TRUE))); } - /** - * Generate and store a token on the User record. - * - * @param int $userID - * - * @return string - * The token - */ - public static function updateToken(int $userID): string { - // Generate a JWT that expires in 1 hour. - // We'll store this on the User record, that way invalidating any previous token that may have been generated. - $expires = time() + 60 * 60; - $token = \Civi::service('crypto.jwt')->encode([ - 'exp' => $expires, - 'sub' => "uid:$userID", - 'scope' => Security::PASSWORD_RESET_SCOPE, - ]); - User::update(FALSE) - ->addValue('password_reset_token', $token) - ->addWhere('id', '=', $userID) - ->execute(); - - return $token; - } - } diff --git a/ext/standaloneusers/Civi/Api4/Action/User/SendPasswordResetEmail.php b/ext/standaloneusers/Civi/Api4/Action/User/SendPasswordResetEmail.php new file mode 100644 index 000000000000..8953abc9ab92 --- /dev/null +++ b/ext/standaloneusers/Civi/Api4/Action/User/SendPasswordResetEmail.php @@ -0,0 +1,106 @@ +timeout); + + $workflowMessage = self::preparePasswordResetWorkflow($user, $token); + if ($workflowMessage) { + // The template_params are used in the template like {$resetUrlHtml} and {$resetUrlHtml} {$usernamePlaintext} {$usernameHtml} + try { + [$sent, /*$subject, $text, $html*/] = $workflowMessage->sendTemplate(); + if (!$sent) { + throw new \RuntimeException("sendTemplate() returned unsent."); + } + Civi::log()->info("Successfully sent password reset to user {$user['id']} ({$user['username']}) to {$user['uf_name']}"); + } + catch (\Exception $e) { + Civi::log()->error("Failed to send password reset to user {$user['id']} ({$user['username']}) to {$user['uf_name']}"); + } + } + } + + /** + * Prepare a password reset workflow email for a user + * + * Includes some checks that we have all the necessary pieces + * in place + * + * @internal (only public for use in SecurityTest) + * + * @return \CRM_Standaloneusers_WorkflowMessage_PasswordReset|null + */ + public static function preparePasswordResetWorkflow(array $user, string $token): ?CRM_Standaloneusers_WorkflowMessage_PasswordReset { + // Find the message template + $tplID = MessageTemplate::get(FALSE) + ->setSelect(['id']) + ->addWhere('workflow_name', '=', 'password_reset') + ->addWhere('is_default', '=', TRUE) + ->addWhere('is_reserved', '=', FALSE) + ->addWhere('is_active', '=', TRUE) + ->execute()->first()['id']; + if (!$tplID) { + // Some sites may deliberately disable this, but it's unusual, so leave a notice in the log. + Civi::log()->notice("There is no active, default password_reset message template, which has prevented emailing a reset to {username}", ['username' => $user['username']]); + return NULL; + } + if (!filter_var($user['uf_name'] ?? '', \FILTER_VALIDATE_EMAIL)) { + Civi::log()->warning("User {$user['id']} has an invalid email. Failed to send password reset."); + return NULL; + } + + // get the required params from the user record + $username = $user['username']; + $email = $user['uf_name']; + $contactId = $user['contact_id']; + + // The template_params are used in the template like {$resetUrlHtml} and {$resetUrlHtml} {$usernamePlaintext} {$usernameHtml} + [$domainFromName, $domainFromEmail] = \CRM_Core_BAO_Domain::getNameAndEmail(TRUE); + $workflowMessage = (new CRM_Standaloneusers_WorkflowMessage_PasswordReset()) + ->setRequiredParams($username, $email, $contactId, $token) + ->setFrom("\"$domainFromName\" <$domainFromEmail>"); + + return $workflowMessage; + } + +} diff --git a/ext/standaloneusers/Civi/Api4/User.php b/ext/standaloneusers/Civi/Api4/User.php index d361d7cad5bb..bb1b29f1aafa 100644 --- a/ext/standaloneusers/Civi/Api4/User.php +++ b/ext/standaloneusers/Civi/Api4/User.php @@ -67,7 +67,7 @@ public static function permissions() { 'create' => ['cms:administer users'], 'delete' => ['cms:administer users'], 'passwordReset' => ['access password resets'], - 'sendPasswordReset' => ['access password resets'], + 'requestPasswordResetEmail' => ['access password resets'], 'login' => ['access password resets'], ]; } diff --git a/ext/standaloneusers/Civi/Standalone/Security.php b/ext/standaloneusers/Civi/Standalone/Security.php index a41f50828f77..62ab5f92d131 100644 --- a/ext/standaloneusers/Civi/Standalone/Security.php +++ b/ext/standaloneusers/Civi/Standalone/Security.php @@ -1,11 +1,7 @@ decode($token); - } - catch (CryptoException $e) { - Civi::log()->warning('Exception while decoding JWT', ['exception' => $e]); - return NULL; - } - - $scope = $decodedToken['scope'] ?? ''; - if ($scope != Security::PASSWORD_RESET_SCOPE) { - Civi::log()->warning('Expected JWT password reset, got ' . $scope); - return NULL; - } - - if (empty($decodedToken['sub']) || substr($decodedToken['sub'], 0, 4) !== 'uid:') { - Civi::log()->warning('Missing uid in JWT sub field'); - return NULL; - } - else { - $userID = substr($decodedToken['sub'], 4); - } - if (!$userID > 0) { - // Hacker - Civi::log()->warning("Rejected passwordResetToken with invalid userID.", compact('token', 'userID')); - return NULL; - } - - $matched = User::get(FALSE) - ->addWhere('id', '=', $userID) - ->addWhere('password_reset_token', '=', $token) - ->addWhere('is_active', '=', 1) - ->selectRowCount() - ->execute()->countMatched() === 1; - - if ($matched && $spend) { - $matched = User::update(FALSE) - ->addWhere('id', '=', $userID) - ->addValue('password_reset_token', NULL) - ->execute(); - } - Civi::log()->info(($matched ? 'Accepted' : 'Rejected') . " passwordResetToken for user $userID"); - return $matched ? $userID : NULL; - } - - /** - * Prepare a password reset workflow email, if configured. - * - * @return \CRM_Standaloneusers_WorkflowMessage_PasswordReset|null - */ - public function preparePasswordResetWorkflow(array $user, string $token): ?CRM_Standaloneusers_WorkflowMessage_PasswordReset { - // Find the message template - $tplID = MessageTemplate::get(FALSE) - ->setSelect(['id']) - ->addWhere('workflow_name', '=', 'password_reset') - ->addWhere('is_default', '=', TRUE) - ->addWhere('is_reserved', '=', FALSE) - ->addWhere('is_active', '=', TRUE) - ->execute()->first()['id']; - if (!$tplID) { - // Some sites may deliberately disable this, but it's unusual, so leave a notice in the log. - Civi::log()->notice("There is no active, default password_reset message template, which has prevented emailing a reset to {username}", ['username' => $user['username']]); - return NULL; - } - if (!filter_var($user['uf_name'] ?? '', \FILTER_VALIDATE_EMAIL)) { - Civi::log()->warning("User $user[id] has an invalid email. Failed to send password reset."); - return NULL; - } - - // The template_params are used in the template like {$resetUrlHtml} and {$resetUrlHtml} {$usernamePlaintext} {$usernameHtml} - [$domainFromName, $domainFromEmail] = \CRM_Core_BAO_Domain::getNameAndEmail(TRUE); - $workflowMessage = (new \CRM_Standaloneusers_WorkflowMessage_PasswordReset()) - ->setDataFromUser($user, $token) - ->setFrom("\"$domainFromName\" <$domainFromEmail>"); - - return $workflowMessage; - } - } diff --git a/ext/standaloneusers/ang/crmResetPassword.js b/ext/standaloneusers/ang/crmResetPassword.js index 017c6c6ff25c..659c279bec19 100644 --- a/ext/standaloneusers/ang/crmResetPassword.js +++ b/ext/standaloneusers/ang/crmResetPassword.js @@ -36,14 +36,14 @@ ctrl[prop] = newVal; }, 0); }; - ctrl.sendPasswordReset = () => { + ctrl.requestPasswordResetEmail = () => { updateAngular('busy', ts('Just a moment...')); updateAngular('formSubmitted', true); if (!ctrl.identifier) { alert(ts('Please provide your username/email.')); return; } - crmApi4('User', 'sendPasswordReset', { identifier: ctrl.identifier }) + crmApi4('User', 'requestPasswordResetEmail', { identifier: ctrl.identifier }) .then(r => { updateAngular('busy', ''); updateAngular('resetSuccessfullySubmitted', true); diff --git a/ext/standaloneusers/ang/crmResetPassword/crmResetPassword.html b/ext/standaloneusers/ang/crmResetPassword/crmResetPassword.html index f7e692d0a62d..3ac810a9ba19 100644 --- a/ext/standaloneusers/ang/crmResetPassword/crmResetPassword.html +++ b/ext/standaloneusers/ang/crmResetPassword/crmResetPassword.html @@ -16,7 +16,7 @@

Request Password Reset Link

/> - +
diff --git a/ext/standaloneusers/tests/phpunit/Civi/Standalone/SecurityTest.php b/ext/standaloneusers/tests/phpunit/Civi/Standalone/SecurityTest.php index d91db2d8ef58..f39d8c09d978 100644 --- a/ext/standaloneusers/tests/phpunit/Civi/Standalone/SecurityTest.php +++ b/ext/standaloneusers/tests/phpunit/Civi/Standalone/SecurityTest.php @@ -4,6 +4,8 @@ use Civi\Test\EndToEndInterface; use Civi\Test\TransactionalInterface; use Civi\Api4\User; +use Civi\Api4\Action\User\PasswordReset; +use Civi\Api4\Action\User\SendPasswordResetEmail; /** * Test Security flows in Standalone @@ -171,7 +173,7 @@ public static function storeFakePasswordResetToken(int $userID, int $expires): s $token = \Civi::service('crypto.jwt')->encode([ 'exp' => $expires, 'sub' => "uid:$userID", - 'scope' => Security::PASSWORD_RESET_SCOPE, + 'scope' => PasswordReset::PASSWORD_RESET_SCOPE, ]); User::update(FALSE) ->addValue('password_reset_token', $token) @@ -187,18 +189,18 @@ public function testForgottenPassword() { [$contactID, $userID, $security] = $this->createFixtureContactAndUser(); // Create token. - $token = \Civi\Api4\Action\User\SendPasswordReset::updateToken($userID); + $token = PasswordReset::updateToken($userID); $decodedToken = \Civi::service('crypto.jwt')->decode($token); $this->assertEquals('uid:' . $userID, $decodedToken['sub']); - $this->assertEquals(Security::PASSWORD_RESET_SCOPE, $decodedToken['scope']); + $this->assertEquals(PasswordReset::PASSWORD_RESET_SCOPE, $decodedToken['scope']); // Check it works, but only once. - $extractedUserID = $security->checkPasswordResetToken($token); + $extractedUserID = PasswordReset::checkPasswordResetToken($token); $this->assertEquals($userID, $extractedUserID); - $this->assertNull($security->checkPasswordResetToken($token)); + $this->assertNull(PasswordReset::checkPasswordResetToken($token)); // OK, let's change that password. - $token = \Civi\Api4\Action\User\SendPasswordReset::updateToken($userID); + $token = PasswordReset::updateToken($userID); // Attempt to change the user's password using this token to authenticate. $result = User::passwordReset(TRUE) @@ -223,8 +225,8 @@ public function testForgottenPassword() { } // Check the message template generation - $token = \Civi\Api4\Action\User\SendPasswordReset::updateToken($userID); - $workflow = $security->preparePasswordResetWorkflow($user, $token); + $token = PasswordReset::updateToken($userID); + $workflow = SendPasswordResetEmail::preparePasswordResetWorkflow($user, $token); $this->assertNotNull($workflow); $result = $workflow->renderTemplate(); @@ -234,7 +236,7 @@ public function testForgottenPassword() { // Fake an expired token $token = $this->storeFakePasswordResetToken($userID, time() - 1); - $this->assertNull($security->checkPasswordResetToken($token)); + $this->assertNull(PasswordReset::checkPasswordResetToken($token)); } protected function deleteStuffWeMade() {