From 57f9826a3566accbe2d97162c9b0c6edd9f04061 Mon Sep 17 00:00:00 2001 From: Rich Lott / Artful Robot Date: Thu, 28 Sep 2023 21:18:21 +0100 Subject: [PATCH] standalone: User password storage and api --- .../CRM/Standaloneusers/DAO/User.php | 17 +- .../Standaloneusers/Page/ChangePassword.php | 19 ++ .../Civi/Api4/Action/User/Create.php | 12 + .../Civi/Api4/Action/User/Save.php | 9 + .../Civi/Api4/Action/User/Update.php | 9 + .../Civi/Api4/Action/User/WriteTrait.php | 129 ++++++++++ .../Spec/Provider/UserSpecProvider.php | 42 ++++ ext/standaloneusers/Civi/Api4/User.php | 38 +++ .../PasswordAlgorithms/AlgorithmInterface.php | 21 ++ .../Standalone/PasswordAlgorithms/Drupal7.php | 184 ++++++++++++++ .../Civi/Standalone/Security.php | 217 +++++------------ ...formX.aff.html => afformEditRole.aff.html} | 0 ...afformX.aff.php => afformEditRole.aff.php} | 0 .../ang/crmChangePassword.ang.php | 17 ++ ext/standaloneusers/ang/crmChangePassword.js | 115 +++++++++ .../crmChangePassword/crmChangePassword.html | 49 ++++ ext/standaloneusers/info.xml | 2 + ext/standaloneusers/sql/auto_install.sql | 2 +- .../Standaloneusers/Page/ChangePassword.tpl | 3 + .../phpunit/Civi/Standalone/SecurityTest.php | 225 +++++++++++++++++- .../xml/Menu/standaloneusers.xml | 6 + .../xml/schema/CRM/Standaloneusers/User.xml | 5 +- templates/CRM/common/standalone.tpl | 2 +- 23 files changed, 952 insertions(+), 171 deletions(-) create mode 100644 ext/standaloneusers/CRM/Standaloneusers/Page/ChangePassword.php create mode 100644 ext/standaloneusers/Civi/Api4/Action/User/Create.php create mode 100644 ext/standaloneusers/Civi/Api4/Action/User/Save.php create mode 100644 ext/standaloneusers/Civi/Api4/Action/User/Update.php create mode 100644 ext/standaloneusers/Civi/Api4/Action/User/WriteTrait.php create mode 100644 ext/standaloneusers/Civi/Api4/Service/Spec/Provider/UserSpecProvider.php create mode 100644 ext/standaloneusers/Civi/Standalone/PasswordAlgorithms/AlgorithmInterface.php create mode 100644 ext/standaloneusers/Civi/Standalone/PasswordAlgorithms/Drupal7.php rename ext/standaloneusers/ang/{afformX.aff.html => afformEditRole.aff.html} (100%) rename ext/standaloneusers/ang/{afformX.aff.php => afformEditRole.aff.php} (100%) create mode 100644 ext/standaloneusers/ang/crmChangePassword.ang.php create mode 100644 ext/standaloneusers/ang/crmChangePassword.js create mode 100644 ext/standaloneusers/ang/crmChangePassword/crmChangePassword.html create mode 100644 ext/standaloneusers/templates/CRM/Standaloneusers/Page/ChangePassword.tpl diff --git a/ext/standaloneusers/CRM/Standaloneusers/DAO/User.php b/ext/standaloneusers/CRM/Standaloneusers/DAO/User.php index d814534a78..df4d4a3bce 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:44014e10073e5e7f63059f09956e77ca) + * (GenCodeChecksum:12f08a726067ec0e3cc7f45411d9526a) */ use CRM_Standaloneusers_ExtensionUtil as E; @@ -74,13 +74,13 @@ class CRM_Standaloneusers_DAO_User extends CRM_Core_DAO { public $username; /** - * Hashed password + * Hashed, not plaintext password * * @var string * (SQL type: varchar(128)) * Note that values will be retrieved from the database as a string. */ - public $password; + public $hashed_password; /** * Email (e.g. for password resets) @@ -252,11 +252,11 @@ class CRM_Standaloneusers_DAO_User extends CRM_Core_DAO { ], 'add' => NULL, ], - 'password' => [ - 'name' => 'password', + 'hashed_password' => [ + 'name' => 'hashed_password', 'type' => CRM_Utils_Type::T_STRING, - 'title' => E::ts('Password'), - 'description' => E::ts('Hashed password'), + 'title' => E::ts('Hashed Password'), + 'description' => E::ts('Hashed, not plaintext password'), 'required' => TRUE, 'maxlength' => 128, 'size' => CRM_Utils_Type::HUGE, @@ -266,11 +266,12 @@ class CRM_Standaloneusers_DAO_User extends CRM_Core_DAO { 'duplicate_matching' => FALSE, 'token' => FALSE, ], - 'where' => 'civicrm_user.password', + 'where' => 'civicrm_user.hashed_password', 'table_name' => 'civicrm_user', 'entity' => 'User', 'bao' => 'CRM_Standaloneusers_DAO_User', 'localizable' => 0, + 'readonly' => TRUE, 'add' => NULL, ], 'email' => [ diff --git a/ext/standaloneusers/CRM/Standaloneusers/Page/ChangePassword.php b/ext/standaloneusers/CRM/Standaloneusers/Page/ChangePassword.php new file mode 100644 index 0000000000..1f897dc482 --- /dev/null +++ b/ext/standaloneusers/CRM/Standaloneusers/Page/ChangePassword.php @@ -0,0 +1,19 @@ +assign('hibp', CIVICRM_HIBP_URL); + $this->assign('loggedInUserID', CRM_Utils_System::getLoggedInUfID()); + Civi::service('angularjs.loader')->addModules('crmChangePassword'); + // @todo specify the user in the URL somehow. + parent::run(); + } + +} diff --git a/ext/standaloneusers/Civi/Api4/Action/User/Create.php b/ext/standaloneusers/Civi/Api4/Action/User/Create.php new file mode 100644 index 0000000000..b28fcf6b59 --- /dev/null +++ b/ext/standaloneusers/Civi/Api4/Action/User/Create.php @@ -0,0 +1,12 @@ +getCheckPermissions()) { + // We must have a logged in user if we're checking permissions. + $loggedInUserID = \CRM_Utils_System::getLoggedInUfID(); + if (!$loggedInUserID) { + // Something weird going on. This codepath should never happen. + throw new UnauthorizedException("Not logged on"); + } + + // We never allow one user to directly change the hashed password of another. + // We assume that directly setting hashed_password would only ever be done by + // integration/migration scripts which would not use checkPermissions() + // Some other things should also not be changed by permissioned API call. + $disallowChanging = ['hashed_password', 'when_created', 'when_last_accessed', 'when_updated']; + $forbidden = array_intersect_key($record, array_flip($disallowChanging)); + if ($forbidden) { + throw new UnauthorizedException("Not allowed to change " . implode(' or ', array_keys($forbidden))); + } + + $requireAdminPermissionToChange = ['contact_id', 'roles', 'is_active']; + $forbidden = array_intersect_key($record, array_flip($requireAdminPermissionToChange)); + if (!\CRM_Core_Permission::check(['cms:administer users']) && $forbidden) { + throw new UnauthorizedException("Not allowed to change " . implode(' or ', array_keys($forbidden))); + } + } + if (isset($record['plaintext_password'])) { + if (!empty($record['hashed_password'])) { + throw new API_Exception("Ambiguous password parameters: Cannot pass plaintext_password AND hashed_password."); + } + if (empty($record['plaintext_password'])) { + throw new API_Exception("Disallowing empty password."); + } + } + parent::formatWriteValues($record); + } + + /** + * This is called with the values for a record fully loaded. + * + * Note that we will now have hashed_password, as well as possibly plaintext_password. + * + */ + protected function validateValues() { + if (!$this->getCheckPermissions()) { + return; + } + $loggedInUserID = \CRM_Utils_System::getLoggedInUfID() ?? FALSE; + $hasAdminPermission = \CRM_Core_Permission::check(['cms:administer users']); + $hasAuthenticated = FALSE; + // Check that we have the logged-in-user's password. + if ($this->actorPassword) { + $storedHashedPassword = \Civi\Api4\User::get(FALSE) + ->addWhere('id', '=', $loggedInUserID) + ->addSelect('hashed_password') + ->execute() + ->first()['hashed_password']; + $security = \Civi\Standalone\Security::singleton(); + if (!$security->checkPassword($this->actorPassword, $storedHashedPassword)) { + throw new UnauthorizedException("Incorrect password"); + } + $hasAuthenticated = TRUE; + } + // @todo check password_reset_token + // elseif () { if (tokenSuppliedAndValid) $hasAuthenticated = TRUE; } + + $records = ($this->getActionName() === 'save') ? $this->records : [$this->getValues()]; + foreach ($records as $values) { + // If changing someone else's account, require 'cms:administer users' + if (($values['id'] ?? NULL) !== $loggedInUserID && !$hasAdminPermission) { + throw new UnauthorizedException("You are not permitted to change other users' accounts."); + } + + // If changing a password, require user to re-authenticate as themself. + if (isset(($values['plaintext_password'])) && !$hasAuthenticated) { + throw new UnauthorizedException("Unauthorized"); + } + } + } + + /** + * Overrideable function to save items using the appropriate BAO function + * + * @param array[] $items + * Items already formatted by self::writeObjects + * @return \CRM_Core_DAO[] + * Array of saved DAO records + */ + protected function write(array $items) { + foreach ($items as &$item) { + // If given, convert plaintext_password to hashed_password now. + if (isset($item['plaintext_password'])) { + $item['hashed_password'] = Security::singleton()->hashPassword($item['plaintext_password']); + unset($item['plaintext_password']); + } + } + return parent::write($items); + } + +} diff --git a/ext/standaloneusers/Civi/Api4/Service/Spec/Provider/UserSpecProvider.php b/ext/standaloneusers/Civi/Api4/Service/Spec/Provider/UserSpecProvider.php new file mode 100644 index 0000000000..42e7344885 --- /dev/null +++ b/ext/standaloneusers/Civi/Api4/Service/Spec/Provider/UserSpecProvider.php @@ -0,0 +1,42 @@ +setTitle(ts('New password')); + $plaintextPassword->setDescription('Provide a new password for this user.'); + $plaintextPassword->setInputType('Text'); + $spec->addFieldSpec($plaintextPassword); + } + + /** + * @inheritDoc + */ + public function applies($entity, $action) { + return $entity === 'User' && in_array($action, ['create', 'update', 'save']); + } + +} diff --git a/ext/standaloneusers/Civi/Api4/User.php b/ext/standaloneusers/Civi/Api4/User.php index ddb629fadd..301bf0df0c 100644 --- a/ext/standaloneusers/Civi/Api4/User.php +++ b/ext/standaloneusers/Civi/Api4/User.php @@ -1,6 +1,10 @@ setCheckPermissions($checkPermissions); + } + + /** + * @param bool $checkPermissions + * @return \Civi\Api4\Action\User\Create + */ + public static function create($checkPermissions = TRUE): Create { + return (new Create(static::getEntityName(), __FUNCTION__)) + ->setCheckPermissions($checkPermissions); + } + + /** + * @param bool $checkPermissions + * @return \Civi\Api4\Action\User\Update + */ + public static function update($checkPermissions = TRUE): Update { + return (new Update(static::getEntityName(), __FUNCTION__)) + ->setCheckPermissions($checkPermissions); + } + + /** + * Permissions are wide on this but are checked in validateValues. + */ + public static function permissions() { + return ['default' => ['access CiviCRM']]; + } + } diff --git a/ext/standaloneusers/Civi/Standalone/PasswordAlgorithms/AlgorithmInterface.php b/ext/standaloneusers/Civi/Standalone/PasswordAlgorithms/AlgorithmInterface.php new file mode 100644 index 0000000000..edfdde795c --- /dev/null +++ b/ext/standaloneusers/Civi/Standalone/PasswordAlgorithms/AlgorithmInterface.php @@ -0,0 +1,21 @@ +_d7_password_crypt('sha512', $plaintext, $storedPassword)); + } + + /** + * Creates a hashed value to store for given password. + * + * Responsible for loading any of its configurable settings. + * + */ + public function hashPassword(string $plaintext): string { + return $this->_d7_password_crypt('sha512', $plaintext, $this->_d7_password_generate_salt()); + } + + /** + * This is taken from Drupal 7.91 + * + * Encodes bytes into printable base 64 using the *nix standard from crypt(). + * + * @param $input + * The string containing bytes to encode. + * @param $count + * The number of characters (bytes) to encode. + * + * @return string + * Encoded string + */ + public function _d7_password_base64_encode($input, $count): string { + $output = ''; + $i = 0; + $itoa64 = self::ITOA64; + do { + $value = ord($input[$i++]); + $output .= $itoa64[$value & 0x3f]; + if ($i < $count) { + $value |= ord($input[$i]) << 8; + } + $output .= $itoa64[($value >> 6) & 0x3f]; + if ($i++ >= $count) { + break; + } + if ($i < $count) { + $value |= ord($input[$i]) << 16; + } + $output .= $itoa64[($value >> 12) & 0x3f]; + if ($i++ >= $count) { + break; + } + $output .= $itoa64[($value >> 18) & 0x3f]; + } while ($i < $count); + + return $output; + } + + /** + * This is taken from Drupal 7.91 + * + * Generates a random base 64-encoded salt prefixed with settings for the hash. + * + * Proper use of salts may defeat a number of attacks, including: + * - The ability to try candidate passwords against multiple hashes at once. + * - The ability to use pre-hashed lists of candidate passwords. + * - The ability to determine whether two users have the same (or different) + * password without actually having to guess one of the passwords. + * + * @param $count_log2 + * Integer that determines the number of iterations used in the hashing + * process. A larger value is more secure, but takes more time to complete. + * + * @return string + * A 12 character string containing the iteration count and a random salt. + */ + public function _d7_password_generate_salt($count_log2 = NULL): string { + + // Standalone: D7 has this stored as a CMS variable setting. + // @todo use global setting that can be changed in civicrm.settings.php + // For now, we just pick a value half way between our hard-coded min and max. + if ($count_log2 === NULL) { + $count_log2 = (int) ((static::$maxHashCount + static::$minHashCount) / 2); + } + $output = '$S$'; + // Ensure that $count_log2 is within set bounds. + $count_log2 = max(static::$minHashCount, min(static::$maxHashCount, $count_log2)); + // We encode the final log2 iteration count in base 64. + $output .= self::ITOA64[$count_log2]; + // 6 bytes is the standard salt for a portable phpass hash. + $output .= $this->_d7_password_base64_encode(random_bytes(6), 6); + return $output; + } + + /** + * This is taken from Drupal 7.91 + * + * Hash a password using a secure stretched hash. + * + * By using a salt and repeated hashing the password is "stretched". Its + * security is increased because it becomes much more computationally costly + * for an attacker to try to break the hash by brute-force computation of the + * hashes of a large number of plain-text words or strings to find a match. + * + * @param $algo + * The string name of a hashing algorithm usable by hash(), like 'sha256'. + * @param $password + * Plain-text password up to 512 bytes (128 to 512 UTF-8 characters) to hash. + * @param $setting + * An existing hash or the output of _d7_password_generate_salt(). Must be + * at least 12 characters (the settings and salt). + * + * @return string|bool + * A string containing the hashed password (and salt) or FALSE on failure. + * The return string will be truncated at DRUPAL_HASH_LENGTH characters max. + */ + protected function _d7_password_crypt($algo, $password, $setting) { + // Prevent DoS attacks by refusing to hash large passwords. + if (strlen($password) > 512) { + return FALSE; + } + // The first 12 characters of an existing hash are its setting string. + $setting = substr($setting, 0, 12); + + if ($setting[0] != '$' || $setting[2] != '$') { + return FALSE; + } + + $count_log2 = strpos(self::ITOA64, $setting[3]); + + // Hashes may be imported from elsewhere, so we allow != DRUPAL_HASH_COUNT + if ($count_log2 < self::$minHashCount || $count_log2 > self::$maxHashCount) { + return FALSE; + } + $salt = substr($setting, 4, 8); + // Hashes must have an 8 character salt. + if (strlen($salt) != 8) { + return FALSE; + } + + // Convert the base 2 logarithm into an integer. + $count = 1 << $count_log2; + $hash = hash($algo, $password, TRUE); + do { + $hash = hash($algo, $hash . $password, TRUE); + } while (--$count); + + $len = strlen($hash); + $output = $setting . $this->_d7_password_base64_encode($hash, $len); + // _d7_password_base64_encode() of a 16 byte MD5 will always be 22 characters. + // _d7_password_base64_encode() of a 64 byte sha512 will always be 86 characters. + $expected = 12 + ceil((8 * $len) / 6); + return (strlen($output) == $expected) ? substr($output, 0, self::$hashLength) : FALSE; + } + +} diff --git a/ext/standaloneusers/Civi/Standalone/Security.php b/ext/standaloneusers/Civi/Standalone/Security.php index a6ddaa0792..2c90dcccdd 100644 --- a/ext/standaloneusers/Civi/Standalone/Security.php +++ b/ext/standaloneusers/Civi/Standalone/Security.php @@ -2,6 +2,7 @@ namespace Civi\Standalone; use CRM_Core_Session; +use Civi; /** * This is a single home for security related functions for Civi Standalone. @@ -12,13 +13,6 @@ use CRM_Core_Session; */ class Security { - public const ITOA64 = './0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz'; - - public static $minHashCount = 7; - public static $maxHashCount = 30; - public static $hashLength = 55; - public static $hashMethod = 'sha512'; - /** * @return static */ @@ -29,24 +23,6 @@ class Security { return \Civi::$statics[__METHOD__]; } - /** - * Check whether a password matches a hashed version. - */ - public function checkPassword(string $plaintextPassword, string $storedHashedPassword): bool { - $type = substr($storedHashedPassword, 0, 3); - switch ($type) { - case '$S$': - // A normal Drupal 7 password. - $hash = $this->_password_crypt(static::$hashMethod, $plaintextPassword, $storedHashedPassword); - break; - - default: - // Invalid password - return FALSE; - } - return hash_equals($storedHashedPassword, $hash); - } - /** * CRM_Core_Permission_Standalone::check() delegates here. * @@ -162,14 +138,11 @@ class Security { */ public function createUser(&$params, $mailParam) { try { - // Q. should this be in the api for User.create? - $hashedPassword = $this->_password_crypt(static::$hashMethod, $params['cms_pass'], $this->_password_generate_salt()); $mail = $params[$mailParam]; - $userID = \Civi\Api4\User::create(FALSE) ->addValue('username', $params['cms_name']) ->addValue('email', $mail) - ->addValue('password', $hashedPassword) + ->addValue('plaintext_password', $params['cms_pass']) ->execute()->single()['id']; } catch (\Exception $e) { @@ -314,140 +287,80 @@ class Security { } /** - * This is taken from Drupal 7.91 - * - * Hash a password using a secure stretched hash. - * - * By using a salt and repeated hashing the password is "stretched". Its - * security is increased because it becomes much more computationally costly - * for an attacker to try to break the hash by brute-force computation of the - * hashes of a large number of plain-text words or strings to find a match. - * - * @param $algo - * The string name of a hashing algorithm usable by hash(), like 'sha256'. - * @param $password - * Plain-text password up to 512 bytes (128 to 512 UTF-8 characters) to hash. - * @param $setting - * An existing hash or the output of _password_generate_salt(). Must be - * at least 12 characters (the settings and salt). - * - * @return string|bool - * A string containing the hashed password (and salt) or FALSE on failure. - * The return string will be truncated at DRUPAL_HASH_LENGTH characters max. + * High level function to encrypt password using the site-default mechanism. */ - public function _password_crypt($algo, $password, $setting) { - // Prevent DoS attacks by refusing to hash large passwords. - if (strlen($password) > 512) { - return FALSE; - } - // The first 12 characters of an existing hash are its setting string. - $setting = substr($setting, 0, 12); + public function hashPassword(string $plaintext): string { + // For now, we just implement D7's but this should be configurable. + // Sites should be able to move from one password hashing algo to another + // e.g. if a vulnerability is discovered. + $algo = new \Civi\Standalone\PasswordAlgorithms\Drupal7(); + return $algo->hashPassword($plaintext); + } - if ($setting[0] != '$' || $setting[2] != '$') { - return FALSE; - } + /** + * Check whether a password matches a hashed version. + */ + public function checkPassword(string $plaintextPassword, string $storedHashedPassword): bool { - $count_log2 = strpos(self::ITOA64, $setting[3]); + if (preg_match('@^\$S\$[A-Za-z./0-9]{52}$@', $storedHashedPassword)) { + // Looks like a default D7 password. + $algo = new \Civi\Standalone\PasswordAlgorithms\Drupal7(); + return $algo->checkPassword($plaintextPassword, $storedHashedPassword); + } - // Hashes may be imported from elsewhere, so we allow != DRUPAL_HASH_COUNT - if ($count_log2 < self::$minHashCount || $count_log2 > self::$maxHashCount) { + if (preg_match('@^\$P\$B[a-zA-Z0-9./]{30}$@', $storedHashedPassword)) { + Civi::log()->warning("Denying access to user whose password looks like a WordPress one because we haven't coded support for that."); return FALSE; } - $salt = substr($setting, 4, 8); - // Hashes must have an 8 character salt. - if (strlen($salt) != 8) { + + // See if we can parse it against this spec... + // One day we might like to support this format because it allows all sorts of hashing algorithms. + // https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md + // $[$v=][$=(,=)*][$[$]] + if (!preg_match('/ + ^ + \$([a-z0-9-]{1,32}) # Match 1 algorithm identifier + (\$v=[0-9+])? # Match 2 optional version + (\$[a-z0-9-]{1,32}=[a-zA-Z0-9/+.-]*(?:,[a-z0-9-]{1,32}=[a-zA-Z0-9/+.-]*)*)? # 3: optional parameters + \$([a-zA-Z0-9/+.-]+) # Match 4 salt + \$([a-zA-Z0-9/+]+) # Match 5 B64 encoded hash + $/x', $storedHashedPassword, $matches)) { + + Civi::log()->warning("Denying access to user whose stored password is not in a format we can parse."); return FALSE; } - - // Convert the base 2 logarithm into an integer. - $count = 1 << $count_log2; - $hash = hash($algo, $password, TRUE); - do { - $hash = hash($algo, $hash . $password, TRUE); - } while (--$count); - - $len = strlen($hash); - $output = $setting . $this->_password_base64_encode($hash, $len); - // _password_base64_encode() of a 16 byte MD5 will always be 22 characters. - // _password_base64_encode() of a 64 byte sha512 will always be 86 characters. - $expected = 12 + ceil((8 * $len) / 6); - return (strlen($output) == $expected) ? substr($output, 0, self::$hashLength) : FALSE; - } - - /** - * This is taken from Drupal 7.91 - * - * Generates a random base 64-encoded salt prefixed with settings for the hash. - * - * Proper use of salts may defeat a number of attacks, including: - * - The ability to try candidate passwords against multiple hashes at once. - * - The ability to use pre-hashed lists of candidate passwords. - * - The ability to determine whether two users have the same (or different) - * password without actually having to guess one of the passwords. - * - * @param $count_log2 - * Integer that determines the number of iterations used in the hashing - * process. A larger value is more secure, but takes more time to complete. - * - * @return string - * A 12 character string containing the iteration count and a random salt. - */ - public function _password_generate_salt($count_log2 = NULL): string { - - // Standalone: D7 has this stored as a CMS variable setting. - // @todo use global setting that can be changed in civicrm.settings.php - // For now, we just pick a value half way between our hard-coded min and max. - if ($count_log2 === NULL) { - $count_log2 = (int) ((static::$maxHashCount + static::$minHashCount) / 2); + [, $identifier, $version, $params, $salt, $hash] = $matches; + + // Map type to algorithm name. Some common ones here, but we don't implement them all. + $algo = [ + '1' => 'md5', + '5' => 'sha256_crypt', + '6' => 'sha512_crypt', + '2' => 'bcrypt', + '2b' => 'bcrypt', + '2x' => 'bcrypt', + '2y' => 'bcrypt', + ][$identifier] ?? ''; + + $version = ltrim($version, '$'); + $parsedParams = []; + if (!empty($params)) { + $parsedParams = []; + foreach (explode(',', (ltrim($params, '$'))) as $kv) { + [$k, $v] = explode('=', $kv); + $parsedParams[$k] = $v; + } } - $output = '$S$'; - // Ensure that $count_log2 is within set bounds. - $count_log2 = max(static::$minHashCount, min(static::$maxHashCount, $count_log2)); - // We encode the final log2 iteration count in base 64. - $output .= self::ITOA64[$count_log2]; - // 6 bytes is the standard salt for a portable phpass hash. - $output .= $this->_password_base64_encode(random_bytes(6), 6); - return $output; - } + $params = $parsedParams; - /** - * This is taken from Drupal 7.91 - * - * Encodes bytes into printable base 64 using the *nix standard from crypt(). - * - * @param $input - * The string containing bytes to encode. - * @param $count - * The number of characters (bytes) to encode. - * - * @return string - * Encoded string - */ - public function _password_base64_encode($input, $count): string { - $output = ''; - $i = 0; - $itoa64 = self::ITOA64; - do { - $value = ord($input[$i++]); - $output .= $itoa64[$value & 0x3f]; - if ($i < $count) { - $value |= ord($input[$i]) << 8; - } - $output .= $itoa64[($value >> 6) & 0x3f]; - if ($i++ >= $count) { - break; - } - if ($i < $count) { - $value |= ord($input[$i]) << 16; - } - $output .= $itoa64[($value >> 12) & 0x3f]; - if ($i++ >= $count) { - break; - } - $output .= $itoa64[($value >> 18) & 0x3f]; - } while ($i < $count); + // salt and hash should be base64 encoded. + $salt = base64_decode(ltrim($salt, '$'), TRUE); + $hash = base64_decode(ltrim($hash, '$'), TRUE); - return $output; + // @todo + // Implement a pluggable interface here to handle some of these password types or more. + Civi::log()->warning("Denying access to user whose stored password relies on '$algo' which we have not implemented yet."); + return FALSE; } } diff --git a/ext/standaloneusers/ang/afformX.aff.html b/ext/standaloneusers/ang/afformEditRole.aff.html similarity index 100% rename from ext/standaloneusers/ang/afformX.aff.html rename to ext/standaloneusers/ang/afformEditRole.aff.html diff --git a/ext/standaloneusers/ang/afformX.aff.php b/ext/standaloneusers/ang/afformEditRole.aff.php similarity index 100% rename from ext/standaloneusers/ang/afformX.aff.php rename to ext/standaloneusers/ang/afformEditRole.aff.php diff --git a/ext/standaloneusers/ang/crmChangePassword.ang.php b/ext/standaloneusers/ang/crmChangePassword.ang.php new file mode 100644 index 0000000000..427f19a9dd --- /dev/null +++ b/ext/standaloneusers/ang/crmChangePassword.ang.php @@ -0,0 +1,17 @@ + 'standaloneusers', + 'js' => [ + 'ang/crmChangePassword.js', + // 'ang/crmQueueMonitor/*.js', + // 'ang/crmQueueMonitor/*/*.js', + ], + // 'css' => ['ang/crmQueueMonitor.css'], + 'partials' => ['ang/crmChangePassword'], + 'requires' => ['crmUi', 'crmUtil', 'api4'], + 'basePages' => ['civicrm/admin/user/password'], + 'exports' => [ + // Export the module as an [E]lement + 'crm-change-password' => 'E', + ], +]; diff --git a/ext/standaloneusers/ang/crmChangePassword.js b/ext/standaloneusers/ang/crmChangePassword.js new file mode 100644 index 0000000000..c74273825d --- /dev/null +++ b/ext/standaloneusers/ang/crmChangePassword.js @@ -0,0 +1,115 @@ +(function (angular, $, _) { + "use strict"; + + angular.module('crmChangePassword', CRM.angRequires('crmChangePassword')); + + // "crmChangePassword" displays the status of a queue + // Example usage:
+ // If "queue" is omitted, then inherit `CRM.vars.crmChangePassword.default`. + angular.module('crmChangePassword').component('crmChangePassword', { + templateUrl: '~/crmChangePassword/crmChangePassword.html', + bindings: { + // things listed here become properties on the controller using values from attributes. + hibp: '@', + userId: '@' + }, + controller: function($scope, $timeout, crmApi4) { + var ts = $scope.ts = CRM.ts(null), + ctrl = this; + + console.log('init crmChangePassword component starting'); + // $onInit gets run after the this controller is called, and after the bindings have been applied. + // this.$onInit = function() { console.log('user', ctrl.userId); }; + ctrl.actorPassword = ''; + ctrl.newPassword = ''; + ctrl.newPasswordAgain = ''; + ctrl.busy = ''; + ctrl.pwnd = false; + + let updateAngular = (prop, newVal) => { + $timeout(() => { + console.log("Setting", prop, "to", newVal); + ctrl[prop] = newVal; + }, 0); + }; + + ctrl.attemptChange = () => { + updateAngular('busy', ''); + updateAngular('pwnd', false); + updateAngular('formInactive', true); + if (ctrl.newPassword.length < 8) { + alert(ts("C'mon now, you can do better than that.")); + return; + } + if (ctrl.newPassword != ctrl.newPasswordAgain) { + alert(ts("Passwords do not match")); + return; + } + + let promises = Promise.resolve(null); + if (ctrl.hibp) { + promises = promises.then(() => { + updateAngular('busy', ts('Checking password is not known to have been involved in data breach...')); + return sha1(ctrl.newPassword) + .then(hash => { + if (!hash.match(/^[a-f0-9]+$/)) { + updateAngular('busy', ts('Could not check. Is your browser up-to-date?')); + } + else { + hash = hash.toUpperCase(); + hashPrefix = hash.substring(0, 5); + return fetch(ctrl.hibp + hashPrefix) + .then(r => r.text()) + .then(hibpResult => { + if (hibpResult && + hibpResult.split(/\r\n/).find(line => hashPrefix + line.replace(/:\d+$/, '') === hash)) { + // e.g. Password123 + updateAngular('pwn', true); + return; + } + updateAngular('busy', ''); + }) + .catch( () => { + updateAngular('busy', ts('Could not perform check; service error.')); + }); + } + }); + }); + } + + promises = promises.then(() => { + updateAngular('busy', ctrl.busy + ts('Changing...')); + // Now submit api request. + const userUpdateParams = { + actorPassword: ctrl.actorPassword, + values: {plaintext_password: ctrl.newPassword}, + where: [['id', '=', ctrl.userId]] + }; + return crmApi4('User', 'Update', userUpdateParams) + .then(r => updateAngular('busy', ts('Password successfully updated'))) + .catch(e => { + let msg = (e.error_message === 'Authorization failed') ? + ts("Sorry, that didn't work. Perhaps you mistyped your password?") : + e.error_message; + updateAngular('busy', msg); + }); + }); + }; + + // Generate SHA-1 digest for given text. Returns Promise + function sha1(message) { + const encoder = new TextEncoder(); + const data = encoder.encode(message); + // const hashBuffer = + return crypto.subtle.digest('SHA-1', data) + .then(hashBuffer => { + const hashArray = Array.from(new Uint8Array(hashBuffer)); // convert buffer to byte array + const hashHex = hashArray + .map((b) => b.toString(16).padStart(2, "0")) + .join(""); // convert bytes to hex string + return hashHex; + }); + } + } + }); +})(angular, CRM.$, CRM._); diff --git a/ext/standaloneusers/ang/crmChangePassword/crmChangePassword.html b/ext/standaloneusers/ang/crmChangePassword/crmChangePassword.html new file mode 100644 index 0000000000..d2604837fd --- /dev/null +++ b/ext/standaloneusers/ang/crmChangePassword/crmChangePassword.html @@ -0,0 +1,49 @@ +
+
+ +
+ +
+ +
+ +
+ +
+ + + {{ts('Passwords do not match')}} + +
+ + + +
+
{{$ctrl.busy}}
+
+

{{ts('Known compromised password')}}

+

{{ts('The password you entered is known to have been included in data breaches. This means it can no longer be considered secure.')}}

+

{{ts('Please choose a different password.')}}

+

{{ts('Also, if you have used that password anywhere else, you should change it as soon as you can.')}}

+

{{ts('For more information, see:')}} haveibeenpwned.com

+
+ +
diff --git a/ext/standaloneusers/info.xml b/ext/standaloneusers/info.xml index 5d392fd36f..373475d44e 100644 --- a/ext/standaloneusers/info.xml +++ b/ext/standaloneusers/info.xml @@ -36,7 +36,9 @@ crmStandaloneusers + scan-classes@1.0.0 mgd-php@1.0.0 + ang-php@1.0.0 setting-php@1.0.0 menu-xml@1.0.0 smarty-v2@1.0.1 diff --git a/ext/standaloneusers/sql/auto_install.sql b/ext/standaloneusers/sql/auto_install.sql index 6bec096b99..44ee4c4998 100644 --- a/ext/standaloneusers/sql/auto_install.sql +++ b/ext/standaloneusers/sql/auto_install.sql @@ -55,7 +55,7 @@ CREATE TABLE `civicrm_user` ( `id` int unsigned NOT NULL AUTO_INCREMENT COMMENT 'Unique User ID', `contact_id` int unsigned COMMENT 'FK to Contact - possibly redundant', `username` varchar(60) NOT NULL, - `password` varchar(128) NOT NULL COMMENT 'Hashed password', + `hashed_password` varchar(128) NOT NULL COMMENT 'Hashed, not plaintext password', `email` varchar(255) NOT NULL COMMENT 'Email (e.g. for password resets)', `roles` varchar(128) COMMENT 'FK to Role', `when_created` timestamp DEFAULT CURRENT_TIMESTAMP, diff --git a/ext/standaloneusers/templates/CRM/Standaloneusers/Page/ChangePassword.tpl b/ext/standaloneusers/templates/CRM/Standaloneusers/Page/ChangePassword.tpl new file mode 100644 index 0000000000..0f2035da6f --- /dev/null +++ b/ext/standaloneusers/templates/CRM/Standaloneusers/Page/ChangePassword.tpl @@ -0,0 +1,3 @@ + + + diff --git a/ext/standaloneusers/tests/phpunit/Civi/Standalone/SecurityTest.php b/ext/standaloneusers/tests/phpunit/Civi/Standalone/SecurityTest.php index e41312be06..0c1ee14885 100644 --- a/ext/standaloneusers/tests/phpunit/Civi/Standalone/SecurityTest.php +++ b/ext/standaloneusers/tests/phpunit/Civi/Standalone/SecurityTest.php @@ -1,9 +1,9 @@ deleteStuffWeMade(); $this->switchBackFromOurUFClasses(TRUE); parent::tearDown(); } + protected function loginUser($userID) { + $security = Security::singleton(); + $user = \Civi\Api4\User::get(FALSE) + ->addWhere('id', '=', $userID) + ->execute()->first(); + + $contactID = civicrm_api3('UFMatch', 'get', [ + 'sequential' => 1, + 'return' => ['contact_id'], + 'uf_id' => $user['id'], + ])['values'][0]['contact_id'] ?? NULL; + $this->assertNotNull($contactID); + /** @var \Civi\Standalone\Security $security */ + $security->loginAuthenticatedUserRecord($user, FALSE); + } + public function testCreateUser():void { [$contactID, $userID, $security] = $this->createFixtureContactAndUser(); @@ -64,10 +81,10 @@ class SecurityTest extends \PHPUnit\Framework\TestCase implements EndToEndInterf $this->assertEquals('user_one', $user['username']); $this->assertEquals('user_one@example.org', $user['email']); - $this->assertStringStartsWith('$', $user['password']); + $this->assertStringStartsWith('$', $user['hashed_password']); - $this->assertTrue($security->checkPassword('secret1', $user['password'])); - $this->assertFalse($security->checkPassword('some other password', $user['password'])); + $this->assertTrue($security->checkPassword('secret1', $user['hashed_password'])); + $this->assertFalse($security->checkPassword('some other password', $user['hashed_password'])); } public function testPerms() { @@ -123,6 +140,9 @@ class SecurityTest extends \PHPUnit\Framework\TestCase implements EndToEndInterf $this->originalUFPermission = $this->originalUF = NULL; } + /** + * @return Array[int, int, \Civi\Standalone\Security] + */ public function createFixtureContactAndUser(): array { $contactID = \Civi\Api4\Contact::create(FALSE) @@ -131,18 +151,209 @@ class SecurityTest extends \PHPUnit\Framework\TestCase implements EndToEndInterf 'display_name' => 'Admin McDemo', ])->execute()->first()['id']; - $security = Security::singleton(); $params = ['cms_name' => 'user_one', 'cms_pass' => 'secret1', 'notify' => FALSE, 'contactID' => $contactID, 'email' => 'user_one@example.org']; - $this->switchToOurUFClasses(); $userID = \CRM_Core_BAO_CMSUser::create($params, 'email'); $this->switchBackFromOurUFClasses(); - $this->assertGreaterThan(0, $userID); $this->contactID = $contactID; $this->userID = $userID; + $security = Security::singleton(); return [$contactID, $userID, $security]; } + public function ensureStaffRoleExists() { + $staffRole = \Civi\Api4\Role::get(FALSE) + ->addWhere('name', '=', 'staffRole') + ->execute()->first(); + if (!$staffRole) { + \Civi\Api4\Role::create(FALSE) + ->setValues([ + 'name' => 'staff', + 'label' => 'General staff', + 'permissions' => [ + "access CiviCRM", + "access Contact Dashboard", + "view my contact", + "edit my contact", + "make online contributions", + "view event info", + "register for events", + "authenticate with password", + ], + ])->execute(); + } + } + + public function testUserApi() { + [$contactID, $adminUserID, $security] = $this->createFixtureContactAndUser(); + // Make our main user an admin and log them in. + User::update(FALSE)->addWhere('id', '=', $adminUserID)->addValue('roles:name', ['admin'])->execute(); + $this->loginUser($adminUserID); + $this->ensureStaffRoleExists(); + + // Create a 2nd contact and linked user. + $stafferContactID = \Civi\Api4\Contact::create(FALSE) + ->setValues(['display_name' => 'Test Staffer']) + ->execute()->first()['id']; + /** @var \Civi\Api4\Action\User\Create */ + $userID = User::create(FALSE) + ->setValues([ + 'username' => 'testuser1', + 'plaintext_password' => 'shhh', + 'contact_id' => $stafferContactID, + 'roles:name' => ['staff'], + 'email' => 'testuser1@example.org', + ]) + ->execute()->first()['id']; + $user = User::get(FALSE)->addWhere('id', '=', $userID)->execute()->first(); + \Civi\Api4\UFMatch::create(FALSE) + ->setValues([ + 'contact_id' => $stafferContactID, + 'uf_id' => $user['id'], + ]) + ->execute(); + ; + $userId = \CRM_Core_BAO_UFMatch::getUFId($stafferContactID); + $this->assertNotNull($userId); + + $this->assertArrayNotHasKey('plaintext_password', $user); + $this->assertMatchesRegularExpression('/^[$].+[$].+/', $user['hashed_password']); + + // Update to the loaded values should NOT result in the password being changed. + $updatedUser = User::update(FALSE) + ->setValues($user) + ->addWhere('id', '=', $user['id']) + ->setReload(TRUE) + ->execute()->first(); + $this->assertEquals($user['hashed_password'], $updatedUser['hashed_password']); + + // Ditto save + $updated = User::save(FALSE) + ->setRecords([$user]) + ->setReload(TRUE) + ->execute()->first(); + $updatedUser = User::get(FALSE)->addWhere('id', '=', $user['id'])->execute()->first(); + $this->assertEquals($user['hashed_password'], $updatedUser['hashed_password']); + + // Test we can force saving a raw password + $updatedUser = User::update(FALSE) + ->setReload(TRUE) + ->addValue('hashed_password', '$shhh') + ->addWhere('id', '=', $user['id']) + ->execute()->first(); + $this->assertEquals('$shhh', $updatedUser['hashed_password']); + + // Now move on to tests with checkPermissions:TRUE + + // Check we are allowed to update this user's password if we provide our own, since we have 'cms:administer users' + // ...by plaintext_password + $previousHash = $updatedUser['hashed_password']; + $updatedUser = User::update(TRUE) + ->addValue('plaintext_password', 'topSecret') + ->addWhere('id', '=', $user['id']) + ->setActorPassword('secret1') + ->setReload(TRUE) + ->execute()->first(); + $this->assertNotEquals($previousHash, $updatedUser['hashed_password'], "Expected that the password was changed, but it wasn't."); + $previousHash = $updatedUser['hashed_password']; + + // ...but NOT by hashed_password + $previousHash = $updatedUser['hashed_password']; + try { + $updatedUser = User::update(TRUE) + ->addValue('hashed_password', '$someNefariousHash') + ->addWhere('id', '=', $user['id']) + ->setActorPassword('secret1') + ->execute(); + $this->fail("Expected UnauthorizedException got none."); + } + catch (\Civi\API\Exception\UnauthorizedException $e) { + $this->assertEquals('Not allowed to change hashed_password', $e->getMessage()); + } + + // Check that if we don't supply OUR correct password, we're not allowed to update the user's password. + try { + User::update(TRUE) + ->addValue('plaintext_password', 'anotherNewPassword') + ->addWhere('id', '=', $user['id']) + ->setActorPassword('wrong pass') + ->execute(); + $this->fail("Expected UnauthorizedException got none."); + } + catch (\Civi\API\Exception\UnauthorizedException $e) { + $this->assertEquals('Incorrect password', $e->getMessage()); + } + + // Check that if we don't supply OUR password at all, we're not allowed to update the user's password. + try { + User::update(TRUE) + ->addValue('plaintext_password', 'anotherNewPassword') + ->addWhere('id', '=', $user['id']) + ->execute(); + $this->fail("Expected UnauthorizedException got none."); + } + catch (\Civi\API\Exception\UnauthorizedException $e) { + $this->assertEquals('Unauthorized', $e->getMessage()); + } + + // Now login as the user in question who only has the 'staff' role. + $this->loginUser($user['id']); + + // Check we are allowed to update our own password if we provide the current one. + $updatedUser = User::update(TRUE) + ->setActorPassword('topSecret') + ->addValue('plaintext_password', 'ourNewSecret') + ->addWhere('id', '=', $user['id']) + ->setReload(TRUE) + ->execute()->first(); + $this->assertNotEquals($previousHash, $updatedUser['hashed_password'], "Expected that the password was changed, but it wasn't."); + $previousHash = $updatedUser['hashed_password']; + + // Check that if we don't supply OUR correct password, we're not allowed to update our password. + try { + User::update(TRUE) + ->addValue('plaintext_password', 'anotherNewPassword') + ->addWhere('id', '=', $user['id']) + ->setActorPassword('wrong pass') + ->execute(); + $this->fail("Expected UnauthorizedException got none."); + } + catch (\Civi\API\Exception\UnauthorizedException $e) { + $this->assertEquals('Incorrect password', $e->getMessage()); + } + + // Check that if we don't supply OUR password at all, we're not allowed to update the user's password. + try { + User::update(TRUE) + ->addValue('plaintext_password', 'anotherNewPassword') + ->addWhere('id', '=', $user['id']) + ->execute(); + $this->fail("Expected UnauthorizedException got none."); + } + catch (\Civi\API\Exception\UnauthorizedException $e) { + $this->assertEquals('Unauthorized', $e->getMessage()); + } + + // Check that we're not allowed to update the admin user's password, since we are not an admin. + try { + User::update(TRUE) + ->addValue('plaintext_password', 'anotherNewPassword') + ->addWhere('id', '=', $adminUserID) + ->setActorPassword('ourNewSecret') + ->execute(); + $this->fail("Expected UnauthorizedException got none."); + } + catch (\Civi\API\Exception\UnauthorizedException $e) { + $this->assertEquals("You are not permitted to change other users' accounts.", $e->getMessage()); + } + + $this->deleteStuffWeMade(); + } + + protected function deleteStuffWeMade() { + User::delete(FALSE)->addWhere('username', '=', 'testuser1')->execute(); + } + } diff --git a/ext/standaloneusers/xml/Menu/standaloneusers.xml b/ext/standaloneusers/xml/Menu/standaloneusers.xml index e82ee26f44..c4f7c94f3f 100644 --- a/ext/standaloneusers/xml/Menu/standaloneusers.xml +++ b/ext/standaloneusers/xml/Menu/standaloneusers.xml @@ -11,4 +11,10 @@ CRM_Standaloneusers_Page_Login::logout *always allow* + + civicrm/admin/user/password + CRM_Standaloneusers_Page_ChangePassword + Change Password + access CiviCRM + diff --git a/ext/standaloneusers/xml/schema/CRM/Standaloneusers/User.xml b/ext/standaloneusers/xml/schema/CRM/Standaloneusers/User.xml index 4cb23909f5..1684bced10 100644 --- a/ext/standaloneusers/xml/schema/CRM/Standaloneusers/User.xml +++ b/ext/standaloneusers/xml/schema/CRM/Standaloneusers/User.xml @@ -57,11 +57,12 @@ - password + hashed_password varchar true 128 - Hashed password + true + Hashed, not plaintext password diff --git a/templates/CRM/common/standalone.tpl b/templates/CRM/common/standalone.tpl index 58acc88223..e84e85a765 100644 --- a/templates/CRM/common/standalone.tpl +++ b/templates/CRM/common/standalone.tpl @@ -21,7 +21,7 @@ {include file="CRM/common/Navigation.tpl"} {/if} - {$docTitle} + {if isset($docTitle)}{$docTitle}{else}CiviCRM{/if} {if $config->debug} -- 2.25.1