From: Rich Lott / Artful Robot Date: Fri, 29 Sep 2023 17:58:22 +0000 (+0100) Subject: standalone: password reset APIs X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=3ce861b9b37a598c8f0904f2cc86327767c1b826;p=civicrm-core.git standalone: password reset APIs --- diff --git a/ext/standaloneusers/CRM/Standaloneusers/DAO/User.php b/ext/standaloneusers/CRM/Standaloneusers/DAO/User.php index 92343ccf1d..0cc7da598c 100644 --- a/ext/standaloneusers/CRM/Standaloneusers/DAO/User.php +++ b/ext/standaloneusers/CRM/Standaloneusers/DAO/User.php @@ -6,7 +6,7 @@ * * Generated from standaloneusers/xml/schema/CRM/Standaloneusers/User.xml * DO NOT EDIT. Generated by CRM_Core_CodeGen - * (GenCodeChecksum:5769c2469482e66ebeec050ea3e82a97) + * (GenCodeChecksum:d8ea39f007e0f4c4de6225227bcfda7b) */ use CRM_Standaloneusers_ExtensionUtil as E; @@ -164,6 +164,15 @@ class CRM_Standaloneusers_DAO_User extends CRM_Core_DAO { */ public $language; + /** + * The unspent token + * + * @var string|null + * (SQL type: varchar(40)) + * Note that values will be retrieved from the database as a string. + */ + public $password_reset_token; + /** * Class constructor. */ @@ -518,6 +527,27 @@ class CRM_Standaloneusers_DAO_User extends CRM_Core_DAO { 'localizable' => 0, 'add' => '2.1', ], + 'password_reset_token' => [ + 'name' => 'password_reset_token', + 'type' => CRM_Utils_Type::T_STRING, + 'title' => E::ts('Password Reset Token'), + 'description' => E::ts('The unspent token'), + 'maxlength' => 40, + 'size' => CRM_Utils_Type::BIG, + 'usage' => [ + 'import' => FALSE, + 'export' => FALSE, + 'duplicate_matching' => FALSE, + 'token' => FALSE, + ], + 'where' => 'civicrm_uf_match.password_reset_token', + 'table_name' => 'civicrm_uf_match', + 'entity' => 'User', + 'bao' => 'CRM_Standaloneusers_DAO_User', + 'localizable' => 0, + 'readonly' => TRUE, + 'add' => NULL, + ], ]; CRM_Core_DAO_AllCoreTables::invoke(__CLASS__, 'fields_callback', Civi::$statics[__CLASS__]['fields']); } diff --git a/ext/standaloneusers/Civi/Api4/Action/User/PasswordReset.php b/ext/standaloneusers/Civi/Api4/Action/User/PasswordReset.php new file mode 100644 index 0000000000..e5307360d2 --- /dev/null +++ b/ext/standaloneusers/Civi/Api4/Action/User/PasswordReset.php @@ -0,0 +1,57 @@ +password)) { + throw new API_Exception("Invalid password"); + } + + // todo: some minimum password quality check? + + // Only valid change here is password, for a known ID. + $security = Security::singleton(); + $userID = $security->checkPasswordResetToken($this->token); + if (!$userID) { + throw new API_Exception("Invalid token."); + } + + User::update(FALSE) + ->addWhere('id', '=', $userID) + ->addValue('password', $this->password) + ->execute(); + + $result['success'] = 1; + $result[] = ['success' => 1]; + } + +} diff --git a/ext/standaloneusers/Civi/Api4/Action/User/SendPasswordReset.php b/ext/standaloneusers/Civi/Api4/Action/User/SendPasswordReset.php index 42de24a867..ce0d2c3555 100644 --- a/ext/standaloneusers/Civi/Api4/Action/User/SendPasswordReset.php +++ b/ext/standaloneusers/Civi/Api4/Action/User/SendPasswordReset.php @@ -10,13 +10,15 @@ use Civi\Api4\Generic\AbstractAction; /** * This is designed to be a public API + * + * @method static setIdentifier(string $identifier) */ class SendPasswordReset extends AbstractAction { /** * Username or email of user to send email for. * - * @param string $identifier + * @var string * @default '' */ protected string $identifier; @@ -85,18 +87,9 @@ class SendPasswordReset extends AbstractAction { return; } - // Generate a once-use token 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 = dechex($expires) . substr(preg_replace('@[/+=]+@', '', base64_encode(random_bytes(64))), 0, 32); - - User::update(FALSE) - ->setValue('password_reset_token', $token) - ->addWhere('id', '=', $user['id']) - ->execute(); + $token = static::updateToken($user['id']); list($domainFromName, $domainFromEmail) = \CRM_Core_BAO_Domain::getNameAndEmail(TRUE); - $resetUrlPlaintext = \CRM_Utils_System::url('civicrm/password-reset', ['token' => $token], TRUE, NULL, FALSE, TRUE); $resetUrlHtml = htmlspecialchars($resetUrlPlaintext); // The template_params are used in the template like {$resetUrlHtml} and {$resetUrlHtml} @@ -119,4 +112,29 @@ class SendPasswordReset extends AbstractAction { } } + /** + * 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 once-use token that expires in 1 hour. + // We'll store this on the User record, that way invalidating any previous token that may have been generated. + // The format is + // The UserID shouldn't need to be secret. + // We only store on the User record. + $expires = time() + 60 * 60; + $token = dechex($expires) . substr(preg_replace('@[/+=]+@', '', base64_encode(random_bytes(64))), 0, 32); + + User::update(FALSE) + ->addValue('password_reset_token', $token) + ->addWhere('id', '=', $userID) + ->execute(); + + return $token . dechex($userID); + } + } diff --git a/ext/standaloneusers/Civi/Api4/Action/User/WriteTrait.php b/ext/standaloneusers/Civi/Api4/Action/User/WriteTrait.php index ec35fe65e0..7b7b998329 100644 --- a/ext/standaloneusers/Civi/Api4/Action/User/WriteTrait.php +++ b/ext/standaloneusers/Civi/Api4/Action/User/WriteTrait.php @@ -14,13 +14,6 @@ trait WriteTrait { */ protected $actorPassword; - /** - * Password reset token. - * - * @var string - */ - protected $passwordResetToken; - /** * At this point we don't have the records we're going to update, we just have the * API values we're going to SET on (each) record that gets processed. @@ -40,12 +33,7 @@ trait WriteTrait { // We must have a logged in user if we're checking permissions. $loggedInUserID = \CRM_Utils_System::getLoggedInUfID(); if (!$loggedInUserID) { - // Only valid situation where a not-logged-in user is asking to change a User record - // is a password reset. - ksort($record); - if (empty($this->passwordResetToken) || array_keys($record) !== ['id', 'password']) { - throw new UnauthorizedException("Unauthorized"); - } + throw new UnauthorizedException("Unauthorized"); } // We never allow one user to directly change the hashed password of another. @@ -101,8 +89,6 @@ trait WriteTrait { } $authenticatedAsLoggedInUser = TRUE; } - // @todo check password_reset_token - // elseif () { if (tokenSuppliedAndValid) $authenticatedAsLoggedInUser = TRUE; } $records = ($this->getActionName() === 'save') ? $this->records : [$this->getValues()]; foreach ($records as $values) { @@ -115,10 +101,7 @@ trait WriteTrait { $changingPassword = array_key_exists('password', $values); if (!$loggedInUserID) { - // Only valid change here is password, for a known ID. - if (empty($values['id']) || $security->checkPasswordResetToken($values['id'], $this->passwordResetToken)) { - throw new UnauthorizedException("Unauthorized"); - } + throw new UnauthorizedException("Unauthorized"); } else { $changingOtherUser = ($values['id'] ?? FALSE) !== $loggedInUserID; diff --git a/ext/standaloneusers/Civi/Api4/User.php b/ext/standaloneusers/Civi/Api4/User.php index 301bf0df0c..87b002ff25 100644 --- a/ext/standaloneusers/Civi/Api4/User.php +++ b/ext/standaloneusers/Civi/Api4/User.php @@ -4,6 +4,7 @@ namespace Civi\Api4; use Civi\Api4\Action\User\Create; use Civi\Api4\Action\User\Save; use Civi\Api4\Action\User\Update; +use Civi\Api4\Action\User\SendPasswordReset; /** * User entity. @@ -41,11 +42,24 @@ class User extends Generic\DAOEntity { ->setCheckPermissions($checkPermissions); } + /** + * @param bool $checkPermissions + * @return \Civi\Api4\Action\User\SendPasswordReset + */ + public static function sendPasswordReset($checkPermissions = TRUE): SendPasswordReset { + return (new SendPasswordReset(static::getEntityName(), __FUNCTION__)) + ->setCheckPermissions($checkPermissions); + } + /** * Permissions are wide on this but are checked in validateValues. */ public static function permissions() { - return ['default' => ['access CiviCRM']]; + return [ + 'default' => ['access CiviCRM'], + 'PasswordReset' => ['access password resets'], + 'SendPasswordreset' => ['access password resets'], + ]; } } diff --git a/ext/standaloneusers/Civi/Standalone/Security.php b/ext/standaloneusers/Civi/Standalone/Security.php index e1e4ef87e5..9fdb3a4ef0 100644 --- a/ext/standaloneusers/Civi/Standalone/Security.php +++ b/ext/standaloneusers/Civi/Standalone/Security.php @@ -369,27 +369,41 @@ class Security { /** * Check a password reset token matches for a User. * - * @param int $userID * @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 bool TRUE if it was valid. + * @return NULL|int + * If int, it's the UserID + * */ - public function checkPasswordResetToken(int $userID, string $token, bool $spend = TRUE): bool { - if (!preg_match('/^([a-f0-9]{8})(.{32})$/', $token, $matches)) { - Civi::log()->warning("Rejected passwordResetToken with invalid syntax for user $userID.", compact('token')); - return FALSE; + public function checkPasswordResetToken(string $token, bool $spend = TRUE): ?int { + if (!preg_match('/^([0-9a-f]{8})([a-zA-Z0-9]{32})([0-9a-f]+)$/', $token, $matches)) { + // Hacker + Civi::log()->warning("Rejected passwordResetToken with invalid syntax.", compact('token')); + return NULL; + } + + $userID = hexdec($matches[3]); + if (!$userID > 0) { + // Hacker + Civi::log()->warning("Rejected passwordResetToken with invalid userID.", compact('token', 'userID')); + return NULL; } + $expiry = hexdec($matches[1]); if (time() > $expiry) { + // Less serious Civi::log()->info("Rejected expired passwordResetToken for user $userID"); - return FALSE; + return NULL; } + + $storedToken = $matches[1] . $matches[2]; $matched = User::get(FALSE) ->addWhere('id', '=', $userID) - ->addWhere('password_reset_token', '=', $token) + ->addWhere('password_reset_token', '=', $storedToken) + ->addWhere('is_active', '=', 1) ->selectRowCount() ->execute()->countMatched() === 1; @@ -400,7 +414,7 @@ class Security { ->execute(); } Civi::log()->info(($matched ? 'Accepted' : 'Rejected') . " passwordResetToken for user $userID"); - return $matched; + return $matched ? $userID : NULL; } } diff --git a/ext/standaloneusers/sql/auto_install.sql b/ext/standaloneusers/sql/auto_install.sql index f00d390197..d0c4c5c561 100644 --- a/ext/standaloneusers/sql/auto_install.sql +++ b/ext/standaloneusers/sql/auto_install.sql @@ -66,6 +66,7 @@ CREATE TABLE `civicrm_uf_match` ( `is_active` tinyint NOT NULL DEFAULT 1, `timezone` varchar(32) NULL COMMENT 'User\'s timezone', `language` varchar(5) COMMENT 'UI language preferred by the given user/contact', + `password_reset_token` varchar(40) COMMENT 'The unspent token', PRIMARY KEY (`id`), INDEX `I_civicrm_uf_match_uf_id`(uf_id), UNIQUE INDEX `UI_username`(username), diff --git a/ext/standaloneusers/standaloneusers.php b/ext/standaloneusers/standaloneusers.php index 9bdb964792..1af490bbd9 100644 --- a/ext/standaloneusers/standaloneusers.php +++ b/ext/standaloneusers/standaloneusers.php @@ -31,3 +31,10 @@ function standaloneusers_civicrm_install() { function standaloneusers_civicrm_enable() { _standaloneusers_civix_civicrm_enable(); } + +/** + * Implements hook_civicrm_permission(). + */ +function standalone_civicrm_permission(&$permissions) { + $permissions['access password resets'] = ts('Allow users to access the reset password system'); +} diff --git a/ext/standaloneusers/tests/phpunit/Civi/Standalone/SecurityTest.php b/ext/standaloneusers/tests/phpunit/Civi/Standalone/SecurityTest.php index b96330fc2b..025bb93b8a 100644 --- a/ext/standaloneusers/tests/phpunit/Civi/Standalone/SecurityTest.php +++ b/ext/standaloneusers/tests/phpunit/Civi/Standalone/SecurityTest.php @@ -74,15 +74,17 @@ class SecurityTest extends \PHPUnit\Framework\TestCase implements EndToEndInterf [$contactID, $userID, $security] = $this->createFixtureContactAndUser(); $user = \Civi\Api4\User::get(FALSE) - ->addSelect('*', 'uf_match.*') ->addWhere('id', '=', $userID) - ->addJoin('UFMatch AS uf_match', 'INNER', ['uf_match.uf_id', '=', 'id']) ->execute()->single(); $this->assertEquals('user_one', $user['username']); + $this->assertEquals($contactID, $user['contact_id']); + $this->assertEquals($userID, $user['id']); + $this->assertEquals($userID, $user['uf_id']); $this->assertEquals('user_one@example.org', $user['uf_name']); $this->assertStringStartsWith('$', $user['hashed_password']); + // Test that the password can be checked ok. $this->assertTrue($security->checkPassword('secret1', $user['hashed_password'])); $this->assertFalse($security->checkPassword('some other password', $user['hashed_password'])); } @@ -123,26 +125,26 @@ class SecurityTest extends \PHPUnit\Framework\TestCase implements EndToEndInterf // $this->switchBackFromOurUFClasses(); } - protected function switchToOurUFClasses() { - return; - if (!empty($this->originalUFPermission)) { - throw new \RuntimeException("are you calling switchToOurUFClasses twice?"); - } - $this->originalUFPermission = \CRM_Core_Config::singleton()->userPermissionClass; - $this->originalUF = \CRM_Core_Config::singleton()->userSystem; - \CRM_Core_Config::singleton()->userPermissionClass = new \CRM_Core_Permission_Standalone(); - \CRM_Core_Config::singleton()->userSystem = new \CRM_Utils_System_Standalone(); - } - - protected function switchBackFromOurUFClasses($justInCase = FALSE) { - return; - if (!$justInCase && empty($this->originalUFPermission)) { - throw new \RuntimeException("are you calling switchBackFromOurUFClasses() twice?"); - } - \CRM_Core_Config::singleton()->userPermissionClass = $this->originalUFPermission; - \CRM_Core_Config::singleton()->userSystem = $this->originalUF; - $this->originalUFPermission = $this->originalUF = NULL; - } + // protected function switchToOurUFClasses() { + // return; + // if (!empty($this->originalUFPermission)) { + // throw new \RuntimeException("are you calling switchToOurUFClasses twice?"); + // } + // $this->originalUFPermission = \CRM_Core_Config::singleton()->userPermissionClass; + // $this->originalUF = \CRM_Core_Config::singleton()->userSystem; + // \CRM_Core_Config::singleton()->userPermissionClass = new \CRM_Core_Permission_Standalone(); + // \CRM_Core_Config::singleton()->userSystem = new \CRM_Utils_System_Standalone(); + // } + // + // protected function switchBackFromOurUFClasses($justInCase = FALSE) { + // return; + // if (!$justInCase && empty($this->originalUFPermission)) { + // throw new \RuntimeException("are you calling switchBackFromOurUFClasses() twice?"); + // } + // \CRM_Core_Config::singleton()->userPermissionClass = $this->originalUFPermission; + // \CRM_Core_Config::singleton()->userSystem = $this->originalUF; + // $this->originalUFPermission = $this->originalUF = NULL; + // } /** * Temporary debugging function @@ -367,6 +369,53 @@ class SecurityTest extends \PHPUnit\Framework\TestCase implements EndToEndInterf $this->deleteStuffWeMade(); } + public function testForgottenPassword() { + + /** @var Security $security */ + [$contactID, $userID, $security] = $this->createFixtureContactAndUser(); + + // Create token. + $token = \Civi\Api4\Action\User\SendPasswordReset::updateToken($userID); + $this->assertMatchesRegularExpression('/^([0-9a-f]{8}[a-zA-Z0-9]{32})([0-9a-f]+)$/', $token); + + // Fake an expired token + $old = dechex(time() - 1); + $this->assertNull($security->checkPasswordResetToken($old . substr($token, 9))); + + // Check token fails if contact ID is different. + $this->assertNull($security->checkPasswordResetToken($token . '0')); + + // Check it works, but only once. + $extractedUserID = $security->checkPasswordResetToken($token); + $this->assertEquals($userID, $extractedUserID); + $this->assertNull($security->checkPasswordResetToken($token)); + + // OK, let's change that password. + $token = \Civi\Api4\Action\User\SendPasswordReset::updateToken($userID); + + // Attempt to change the user's password using this token to authenticate. + $result = User::PasswordReset(TRUE) + ->setToken($token) + ->setPassword('fingersCrossed') + ->execute(); + + $this->assertEquals(1, $result['success']); + $user = User::get(FALSE)->addWhere('id', '=', $userID)->execute()->single(); + $this->assertTrue($security->checkPassword('fingersCrossed', $user['hashed_password'])); + + // Should not work a 2nd time with same token. + try { + User::PasswordReset(TRUE) + ->setToken($token) + ->setPassword('oooh') + ->execute(); + $this->fail("Should not have been able to reuse token"); + } + catch (\Exception $e) { + $this->assertEquals('Invalid token.', $e->getMessage()); + } + } + protected function deleteStuffWeMade() { User::delete(FALSE)->addWhere('username', '=', 'testuser1')->execute(); } diff --git a/ext/standaloneusers/xml/schema/CRM/Standaloneusers/User.xml b/ext/standaloneusers/xml/schema/CRM/Standaloneusers/User.xml index d980630249..ba5c9b0b6e 100644 --- a/ext/standaloneusers/xml/schema/CRM/Standaloneusers/User.xml +++ b/ext/standaloneusers/xml/schema/CRM/Standaloneusers/User.xml @@ -198,4 +198,12 @@ true 1.6 + + password_reset_token + Password Reset Token + The unspent token + varchar + 40 + true + diff --git a/setup/plugins/init/StandaloneUsers.civi-setup.php b/setup/plugins/init/StandaloneUsers.civi-setup.php index 573c4b777a..55eb46afbe 100644 --- a/setup/plugins/init/StandaloneUsers.civi-setup.php +++ b/setup/plugins/init/StandaloneUsers.civi-setup.php @@ -49,7 +49,13 @@ if (!defined('CIVI_SETUP')) { 'name' => 'everyone', 'label' => ts('Everyone, including anonymous users'), // Provide default open permissions - 'permissions' => ['CiviMail subscribe/unsubscribe pages', 'make online contributions', 'view event info', 'register for events'], + 'permissions' => [ + 'CiviMail subscribe/unsubscribe pages', + 'make online contributions', + 'view event info', + 'register for events', + 'access password resets', + ], ], [ 'name' => 'admin',