diff --git a/docs/en/04_Changelogs/4.0.0.md b/docs/en/04_Changelogs/4.0.0.md index 8d07b6fb3..1d877e160 100644 --- a/docs/en/04_Changelogs/4.0.0.md +++ b/docs/en/04_Changelogs/4.0.0.md @@ -1324,7 +1324,17 @@ After (`mysite/_config/config.yml`): * `MODULES_PATH` removed * `MODULES_DIR` removed * `SS_HOST` removed. Use `SS_BASE_URL` instead. - + * `Member::canLogIn()` now returns boolean. Use `Member::validateCanLogin()` to get a `ValidationResult` +* `Security` methods deprecated: + * `has_default_admin` use `DefaultAdminService::hasDefaultAdmin()` instead + * `check_default_admin` use `DefaultAdminService::isDefaultAdminCredentials()` instead + * `default_admin_username` use `DefaultAdminService::getDefaultAdminUsername()` instead + * `default_admin_password` use `DefaultAdminService::getDefaultAdminPassword()` instead + * `setDefaultAdmin` use `DefaultAdminService::setDefaultAdmin()` instead + * `clearDefaultAdmin` use `DefaultAdminService::clearDefaultAdmin()` instead + * `findAnAdministrator` use `DefaultAdminService::findOrCreateDefaultAdmin()` instead +* `Member` methods deprecated: + * `checkPassword`. Use Authenticator::checkPassword() instead #### General and Core Removed API diff --git a/sh.exe.stackdump b/sh.exe.stackdump new file mode 100644 index 000000000..ebfac06e3 --- /dev/null +++ b/sh.exe.stackdump @@ -0,0 +1,4 @@ +Stack trace: +Frame Function Args +0081BF88 6106D69F (00000000, 01010000, 00000000, 00000190) +End of stack trace diff --git a/src/Dev/Install/install.php5 b/src/Dev/Install/install.php5 index f13633153..3baad35ed 100755 --- a/src/Dev/Install/install.php5 +++ b/src/Dev/Install/install.php5 @@ -15,6 +15,7 @@ use SilverStripe\Dev\Install\DatabaseConfigurationHelper; use SilverStripe\ORM\DatabaseAdmin; use SilverStripe\ORM\DB; use SilverStripe\Security\Security; +use SilverStripe\Security\DefaultAdminService; /** * SilverStripe CMS Installer @@ -1515,7 +1516,7 @@ PHP // Create default administrator user and group in database // (not using Security::setDefaultAdmin()) - $adminMember = Security::findAnAdministrator(); + $adminMember = DefaultAdminService::singleton()->findOrCreateDefaultAdmin(); $adminMember->Email = $config['admin']['username']; $adminMember->Password = $config['admin']['password']; $adminMember->PasswordEncryption = Security::config()->encryption_algorithm; diff --git a/src/Forms/ConfirmedPasswordField.php b/src/Forms/ConfirmedPasswordField.php index 886039803..6a9ba5aac 100644 --- a/src/Forms/ConfirmedPasswordField.php +++ b/src/Forms/ConfirmedPasswordField.php @@ -4,9 +4,8 @@ namespace SilverStripe\Forms; use SilverStripe\ORM\DataObject; use SilverStripe\ORM\DataObjectInterface; -use SilverStripe\Security\Member; +use SilverStripe\Security\Authenticator; use SilverStripe\Security\Security; -use SilverStripe\View\Requirements; /** * Two masked input fields, checks for matching passwords. @@ -519,17 +518,20 @@ class ConfirmedPasswordField extends FormField } // With a valid user and password, check the password is correct - $checkResult = $member->checkPassword($this->currentPasswordValue); - if (!$checkResult->isValid()) { - $validator->validationError( - $name, - _t( - 'SilverStripe\\Forms\\ConfirmedPasswordField.CURRENT_PASSWORD_ERROR', - "The current password you have entered is not correct." - ), - "validation" - ); - return false; + $authenticators = Security::singleton()->getApplicableAuthenticators(Authenticator::CHECK_PASSWORD); + foreach ($authenticators as $authenticator) { + $checkResult = $authenticator->checkPassword($member, $this->currentPasswordValue); + if (!$checkResult->isValid()) { + $validator->validationError( + $name, + _t( + 'SilverStripe\\Forms\\ConfirmedPasswordField.CURRENT_PASSWORD_ERROR', + "The current password you have entered is not correct." + ), + "validation" + ); + return false; + } } } diff --git a/src/ORM/DataObject.php b/src/ORM/DataObject.php index 7e5736329..2f6bb083b 100644 --- a/src/ORM/DataObject.php +++ b/src/ORM/DataObject.php @@ -96,10 +96,11 @@ use stdClass; * @todo Add instance specific removeExtension() which undos loadExtraStatics() * and defineMethods() * - * @property integer ID ID of the DataObject, 0 if the DataObject doesn't exist in database. - * @property string ClassName Class name of the DataObject - * @property string LastEdited Date and time of DataObject's last modification. - * @property string Created Date and time of DataObject creation. + * @property int $ID ID of the DataObject, 0 if the DataObject doesn't exist in database. + * @property int $OldID ID of object, if deleted + * @property string $ClassName Class name of the DataObject + * @property string $LastEdited Date and time of DataObject's last modification. + * @property string $Created Date and time of DataObject creation. */ class DataObject extends ViewableData implements DataObjectInterface, i18nEntityProvider, Resettable { diff --git a/src/Security/Authenticator.php b/src/Security/Authenticator.php index fe2d8a52a..c4f66b121 100644 --- a/src/Security/Authenticator.php +++ b/src/Security/Authenticator.php @@ -16,13 +16,36 @@ use SilverStripe\Security\MemberAuthenticator\LogoutHandler; */ interface Authenticator { - + /** + * Can log a user in + */ const LOGIN = 1; + + /** + * Can log user out + */ const LOGOUT = 2; + + /** + * Can change password (check + reset) + */ const CHANGE_PASSWORD = 4; + + /** + * Can modify password + */ const RESET_PASSWORD = 8; + + /** + * In-CMS authentication + */ const CMS_LOGIN = 16; + /** + * Can check password is valid without logging the user in or modifying the password + */ + const CHECK_PASSWORD = 32; + /** * Returns the services supported by this authenticator * @@ -85,5 +108,18 @@ interface Authenticator * @param ValidationResult $result A validationresult which is either valid or contains the error message(s) * @return Member The matched member, or null if the authentication fails */ - public function authenticate($data, &$result = null); + public function authenticate($data, ValidationResult &$result = null); + + /** + * Check if the passed password matches the stored one (if the member is not locked out). + * + * Note, we don't return early, to prevent differences in timings to give away if a member + * password is invalid. + * + * @param Member $member + * @param string $password + * @param ValidationResult $result + * @return ValidationResult + */ + public function checkPassword(Member $member, $password, ValidationResult &$result = null); } diff --git a/src/Security/DefaultAdminService.php b/src/Security/DefaultAdminService.php new file mode 100644 index 000000000..0569c13ea --- /dev/null +++ b/src/Security/DefaultAdminService.php @@ -0,0 +1,191 @@ +constructExtensions(); + } + + /** + * Set the default admin credentials + * + * @param string $username + * @param string $password + */ + public static function setDefaultAdmin($username, $password) + { + // don't overwrite if already set + if (static::hasDefaultAdmin()) { + throw new BadMethodCallException( + "Default admin already exists. Use clearDefaultAdmin() first." + ); + } + + if (empty($username) || empty($password)) { + throw new InvalidArgumentException("Default admin username / password cannot be empty"); + } + + static::$default_username = $username; + static::$default_password = $password; + static::$has_default_admin = true; + } + + /** + * @return string The default admin username + * @throws BadMethodCallException Throws exception if there is no default admin + */ + public static function getDefaultAdminUsername() + { + if (!static::hasDefaultAdmin()) { + throw new BadMethodCallException( + "No default admin configured. Please call hasDefaultAdmin() before getting default admin username" + ); + } + return static::$default_username; + } + + /** + * @return string The default admin password + * @throws BadMethodCallException Throws exception if there is no default admin + */ + public static function getDefaultAdminPassword() + { + if (!static::hasDefaultAdmin()) { + throw new BadMethodCallException( + "No default admin configured. Please call hasDefaultAdmin() before getting default admin password" + ); + } + return static::$default_password; + } + + /** + * Check if there is a default admin + * + * @return bool + */ + public static function hasDefaultAdmin() + { + return static::$has_default_admin; + } + + /** + * Flush the default admin credentials + */ + public static function clearDefaultAdmin() + { + static::$has_default_admin = false; + static::$default_username = null; + static::$default_password = null; + } + + /** + * @return Member|null + */ + public function findOrCreateDefaultAdmin() + { + $this->extend('beforeFindOrCreateDefaultAdmin'); + + // Check if we have default admins + if (!static::hasDefaultAdmin()) { + return null; + } + + // Find or create ADMIN group + Group::singleton()->requireDefaultRecords(); + $adminGroup = Permission::get_groups_by_permission('ADMIN')->first(); + + if (!$adminGroup) { + Group::singleton()->requireDefaultRecords(); + $adminGroup = Permission::get_groups_by_permission('ADMIN')->first(); + } + + // Find member + /** @skipUpgrade */ + $admin = Member::get() + ->filter('Email', static::getDefaultAdminUsername()) + ->first(); + // If no admin is found, create one + if (!$admin) { + // 'Password' is not set to avoid creating + // persistent logins in the database. See Security::setDefaultAdmin(). + // Set 'Email' to identify this as the default admin + $admin = Member::create(); + $admin->FirstName = _t(__CLASS__ . '.DefaultAdminFirstname', 'Default Admin'); + $admin->Email = static::getDefaultAdminUsername(); + $admin->write(); + } + + // Ensure this user is in the admin group + if (!$admin->inGroup($adminGroup)) { + // Add member to group instead of adding group to member + // This bypasses the privilege escallation code in Member_GroupSet + $adminGroup + ->DirectMembers() + ->add($admin); + } + + $this->extend('afterFindOrCreateDefaultAdmin', $admin); + + return $admin; + } + + /** + * Check if the user is a default admin. + * Returns false if there is no default admin. + * + * @param string $username + * @return bool + */ + public static function isDefaultAdmin($username) + { + return static::hasDefaultAdmin() + && $username + && $username === static::getDefaultAdminUsername(); + } + + /** + * Check if the user credentials match the default admin. + * Returns false if there is no default admin. + * + * @param string $username + * @param string $password + * @return bool + */ + public static function isDefaultAdminCredentials($username, $password) + { + return static::isDefaultAdmin($username) + && $password + && $password === static::getDefaultAdminPassword(); + } +} diff --git a/src/Security/Group.php b/src/Security/Group.php index 333ead2d5..4382366f5 100755 --- a/src/Security/Group.php +++ b/src/Security/Group.php @@ -33,14 +33,14 @@ use SilverStripe\ORM\UnsavedRelationList; /** * A security group. * - * @property string Title Name of the group - * @property string Description Description of the group - * @property string Code Group code - * @property string Locked Boolean indicating whether group is locked in security panel - * @property int Sort + * @property string $Title Name of the group + * @property string $Description Description of the group + * @property string $Code Group code + * @property string $Locked Boolean indicating whether group is locked in security panel + * @property int $Sort * @property string HtmlEditorConfig * - * @property int ParentID ID of parent group + * @property int $ParentID ID of parent group * * @method Group Parent() Return parent group * @method HasManyList Permissions() List of group permissions diff --git a/src/Security/Member.php b/src/Security/Member.php index e7895b665..e11c76d56 100644 --- a/src/Security/Member.php +++ b/src/Security/Member.php @@ -19,6 +19,8 @@ use SilverStripe\Forms\DropdownField; use SilverStripe\Forms\FieldList; use SilverStripe\Forms\HTMLEditor\HTMLEditorConfig; use SilverStripe\Forms\ListboxField; +use SilverStripe\Forms\Tab; +use SilverStripe\Forms\TabSet; use SilverStripe\i18n\i18n; use SilverStripe\ORM\ArrayList; use SilverStripe\ORM\DataList; @@ -259,87 +261,43 @@ class Member extends DataObject { parent::requireDefaultRecords(); // Default groups should've been built by Group->requireDefaultRecords() already - static::default_admin(); + $service = DefaultAdminService::singleton(); + $service->findOrCreateDefaultAdmin(); } /** * Get the default admin record if it exists, or creates it otherwise if enabled * + * @deprecated 4.0.0...5.0.0 Use DefaultAdminService::findOrCreateDefaultAdmin() instead * @return Member */ public static function default_admin() { - // Check if set - if (!Security::has_default_admin()) { - return null; - } - - // Find or create ADMIN group - Group::singleton()->requireDefaultRecords(); - $adminGroup = Permission::get_groups_by_permission('ADMIN')->first(); - - // Find member - /** @skipUpgrade */ - $admin = static::get() - ->filter('Email', Security::default_admin_username()) - ->first(); - if (!$admin) { - // 'Password' is not set to avoid creating - // persistent logins in the database. See Security::setDefaultAdmin(). - // Set 'Email' to identify this as the default admin - $admin = Member::create(); - $admin->FirstName = _t(__CLASS__ . '.DefaultAdminFirstname', 'Default Admin'); - $admin->Email = Security::default_admin_username(); - $admin->write(); - } - - // Ensure this user is in the admin group - if (!$admin->inGroup($adminGroup)) { - // Add member to group instead of adding group to member - // This bypasses the privilege escallation code in Member_GroupSet - $adminGroup - ->DirectMembers() - ->add($admin); - } - - return $admin; + Deprecation::notice('5.0', 'Use DefaultAdminService::findOrCreateDefaultAdmin() instead'); + return DefaultAdminService::singleton()->findOrCreateDefaultAdmin(); } /** * Check if the passed password matches the stored one (if the member is not locked out). * - * @param string $password + * @deprecated 4.0.0...5.0.0 Use Authenticator::checkPassword() instead + * + * @param string $password * @return ValidationResult */ public function checkPassword($password) { - $result = $this->canLogIn(); + Deprecation::notice('5.0', 'Use Authenticator::checkPassword() instead'); - // Short-circuit the result upon failure, no further checks needed. - if (!$result->isValid()) { - return $result; + // With a valid user and password, check the password is correct + $result = ValidationResult::create(); + $authenticators = Security::singleton()->getApplicableAuthenticators(Authenticator::CHECK_PASSWORD); + foreach ($authenticators as $authenticator) { + $authenticator->checkPassword($this, $password, $result); + if (!$result->isValid()) { + break; + } } - - // Allow default admin to login as self - if ($this->isDefaultAdmin() && Security::check_default_admin($this->Email, $password)) { - return $result; - } - - // Check a password is set on this member - if (empty($this->Password) && $this->exists()) { - $result->addError(_t(__CLASS__ . '.NoPassword', 'There is no password on this member.')); - - return $result; - } - - $e = PasswordEncryptor::create_for_algorithm($this->PasswordEncryption); - if (!$e->check($this->Password, $password, $this->Salt, $this)) { - $result->addError(_t( - __CLASS__ . '.ERRORWRONGCRED', - 'The provided details don\'t seem to be correct. Please try again.' - )); - } - return $result; } @@ -350,8 +308,17 @@ class Member extends DataObject */ public function isDefaultAdmin() { - return Security::has_default_admin() - && $this->Email === Security::default_admin_username(); + return DefaultAdminService::isDefaultAdmin($this->Email); + } + + /** + * Check if this user can login + * + * @return bool + */ + public function canLogin() + { + return $this->validateCanLogin()->isValid(); } /** @@ -360,12 +327,12 @@ class Member extends DataObject * * You can hook into this with a "canLogIn" method on an attached extension. * + * @param ValidationResult $result Optional result to add errors to * @return ValidationResult */ - public function canLogIn() + public function validateCanLogin(ValidationResult &$result = null) { - $result = ValidationResult::create(); - + $result = $result ?: ValidationResult::create(); if ($this->isLockedOut()) { $result->addError( _t( @@ -394,7 +361,9 @@ class Member extends DataObject return false; } - return DBDatetime::now()->getTimestamp() < $this->dbObject('LockedOutUntil')->getTimestamp(); + /** @var DBDatetime $lockedOutUntil */ + $lockedOutUntil = $this->dbObject('LockedOutUntil'); + return DBDatetime::now()->getTimestamp() < $lockedOutUntil->getTimestamp(); } /** @@ -1418,8 +1387,12 @@ class Member extends DataObject public function getCMSFields() { $this->beforeUpdateCMSFields(function (FieldList $fields) { + /** @var TabSet $rootTabSet */ + $rootTabSet = $fields->fieldByName("Root"); + /** @var Tab $mainTab */ + $mainTab = $rootTabSet->fieldByName("Main"); /** @var FieldList $mainFields */ - $mainFields = $fields->fieldByName("Root")->fieldByName("Main")->getChildren(); + $mainFields = $mainTab->getChildren(); // Build change password field $mainFields->replaceField('Password', $this->getMemberPasswordField()); @@ -1479,7 +1452,7 @@ class Member extends DataObject } } - $permissionsTab = $fields->fieldByName("Root")->fieldByName('Permissions'); + $permissionsTab = $rootTabSet->fieldByName('Permissions'); if ($permissionsTab) { $permissionsTab->addExtraClass('readonly'); } diff --git a/src/Security/MemberAuthenticator/CMSLoginHandler.php b/src/Security/MemberAuthenticator/CMSLoginHandler.php index bd588c231..f30cdb7db 100644 --- a/src/Security/MemberAuthenticator/CMSLoginHandler.php +++ b/src/Security/MemberAuthenticator/CMSLoginHandler.php @@ -41,7 +41,7 @@ class CMSLoginHandler extends LoginHandler protected function redirectToChangePassword() { // Since this form is loaded via an iframe, this redirect must be performed via javascript - $changePasswordForm = ChangePasswordForm::create($this->form->getController(), 'ChangePasswordForm'); + $changePasswordForm = ChangePasswordForm::create($this, 'ChangePasswordForm'); $changePasswordForm->sessionMessage( _t('SilverStripe\\Security\\Member.PASSWORDEXPIRED', 'Your password has expired. Please choose a new one.'), 'good' diff --git a/src/Security/MemberAuthenticator/CMSMemberAuthenticator.php b/src/Security/MemberAuthenticator/CMSMemberAuthenticator.php index 5f3cd3403..467823b4d 100644 --- a/src/Security/MemberAuthenticator/CMSMemberAuthenticator.php +++ b/src/Security/MemberAuthenticator/CMSMemberAuthenticator.php @@ -6,6 +6,9 @@ use SilverStripe\ORM\ValidationResult; use SilverStripe\Security\Authenticator as BaseAuthenticator; use SilverStripe\Security\Member; +/** + * Provides authentication for the user within the CMS + */ class CMSMemberAuthenticator extends MemberAuthenticator { @@ -20,14 +23,14 @@ class CMSMemberAuthenticator extends MemberAuthenticator * @param Member|null $member * @return Member */ - protected function authenticateMember($data, &$result = null, $member = null) + protected function authenticateMember($data, ValidationResult &$result = null, Member $member = null) { // Attempt to identify by temporary ID if (!empty($data['tempid'])) { // Find user by tempid, in case they are re-validating an existing session $member = Member::member_from_tempid($data['tempid']); if ($member) { - $data['email'] = $member->Email; + $data['Email'] = $member->Email; } } diff --git a/src/Security/MemberAuthenticator/ChangePasswordHandler.php b/src/Security/MemberAuthenticator/ChangePasswordHandler.php index 8023b0d2a..755153bcf 100644 --- a/src/Security/MemberAuthenticator/ChangePasswordHandler.php +++ b/src/Security/MemberAuthenticator/ChangePasswordHandler.php @@ -200,11 +200,8 @@ class ChangePasswordHandler extends RequestHandler { $member = Security::getCurrentUser(); // The user was logged in, check the current password - if ($member && ( - empty($data['OldPassword']) || - !$member->checkPassword($data['OldPassword'])->isValid() - ) - ) { + $oldPassword = isset($data['OldPassword']) ? $data['OldPassword'] : null; + if ($member && !$this->checkPassword($member, $oldPassword)) { $form->sessionMessage( _t( 'SilverStripe\\Security\\Member.ERRORPASSWORDNOTMATCH', @@ -274,8 +271,10 @@ class ChangePasswordHandler extends RequestHandler $member->AutoLoginExpired = DBDatetime::create()->now(); $member->write(); - if ($member->canLogIn()->isValid()) { - Injector::inst()->get(IdentityStore::class)->logIn($member, false, $this->getRequest()); + if ($member->canLogIn()) { + /** @var IdentityStore $identityStore */ + $identityStore = Injector::inst()->get(IdentityStore::class); + $identityStore->logIn($member, false, $this->getRequest()); } // TODO Add confirmation message to login redirect @@ -305,4 +304,26 @@ class ChangePasswordHandler extends RequestHandler return $this->redirect($url); } + + /** + * Check if password is ok + * + * @param Member $member + * @param string $password + * @return bool + */ + protected function checkPassword($member, $password) + { + if (empty($password)) { + return false; + } + // With a valid user and password, check the password is correct + $authenticators = Security::singleton()->getApplicableAuthenticators(Authenticator::CHECK_PASSWORD); + foreach ($authenticators as $authenticator) { + if (!$authenticator->checkPassword($member, $password)->isValid()) { + return false; + } + } + return true; + } } diff --git a/src/Security/MemberAuthenticator/LoginHandler.php b/src/Security/MemberAuthenticator/LoginHandler.php index 74e8b1425..31a2b00c4 100644 --- a/src/Security/MemberAuthenticator/LoginHandler.php +++ b/src/Security/MemberAuthenticator/LoginHandler.php @@ -113,6 +113,7 @@ class LoginHandler extends RequestHandler $this->extend('beforeLogin'); // Successful login + /** @var ValidationResult $result */ if ($member = $this->checkLogin($data, $result)) { $this->performLogin($member, $data, $form->getRequestHandler()->getRequest()); // Allow operations on the member after successful login @@ -209,7 +210,7 @@ class LoginHandler extends RequestHandler * @return Member Returns the member object on successful authentication * or NULL on failure. */ - public function checkLogin($data, &$result) + public function checkLogin($data, ValidationResult &$result = null) { $member = $this->authenticator->authenticate($data, $result); if ($member instanceof Member) { diff --git a/src/Security/MemberAuthenticator/MemberAuthenticator.php b/src/Security/MemberAuthenticator/MemberAuthenticator.php index d40f0b37c..0068b6270 100644 --- a/src/Security/MemberAuthenticator/MemberAuthenticator.php +++ b/src/Security/MemberAuthenticator/MemberAuthenticator.php @@ -5,11 +5,14 @@ namespace SilverStripe\Security\MemberAuthenticator; use InvalidArgumentException; use SilverStripe\Control\Controller; use SilverStripe\Control\Session; +use SilverStripe\Core\Extensible; use SilverStripe\ORM\ValidationResult; use SilverStripe\Security\Authenticator; use SilverStripe\Security\LoginAttempt; use SilverStripe\Security\Member; +use SilverStripe\Security\PasswordEncryptor; use SilverStripe\Security\Security; +use SilverStripe\Security\DefaultAdminService; /** * Authenticator for the default "member" method @@ -19,12 +22,13 @@ use SilverStripe\Security\Security; */ class MemberAuthenticator implements Authenticator { + use Extensible; public function supportedServices() { // Bitwise-OR of all the supported services in this Authenticator, to make a bitmask return Authenticator::LOGIN | Authenticator::LOGOUT | Authenticator::CHANGE_PASSWORD - | Authenticator::RESET_PASSWORD; + | Authenticator::RESET_PASSWORD | Authenticator::CHECK_PASSWORD; } /** @@ -32,7 +36,7 @@ class MemberAuthenticator implements Authenticator * @param null|ValidationResult $result * @return null|Member */ - public function authenticate($data, &$result = null) + public function authenticate($data, ValidationResult &$result = null) { // Find authenticated member $member = $this->authenticateMember($data, $result); @@ -52,45 +56,46 @@ class MemberAuthenticator implements Authenticator * * @param array $data Form submitted data * @param ValidationResult $result - * @param Member|null This third parameter is used in the CMSAuthenticator(s) + * @param Member $member This third parameter is used in the CMSAuthenticator(s) * @return Member Found member, regardless of successful login */ - protected function authenticateMember($data, &$result = null, $member = null) + protected function authenticateMember($data, ValidationResult &$result = null, Member $member = null) { - // Default success to false $email = !empty($data['Email']) ? $data['Email'] : null; - $result = new ValidationResult(); + $result = $result ?: ValidationResult::create(); // Check default login (see Security::setDefaultAdmin()) - $asDefaultAdmin = $email === Security::default_admin_username(); + $asDefaultAdmin = DefaultAdminService::isDefaultAdmin($email); if ($asDefaultAdmin) { // If logging is as default admin, ensure record is setup correctly - $member = Member::default_admin(); - $success = Security::check_default_admin($email, $data['Password']); - $result = $member->canLogIn(); - //protect against failed login - if ($success && $result->isValid()) { - return $member; - } else { - $result->addError(_t( - 'SilverStripe\\Security\\Member.ERRORWRONGCRED', - "The provided details don't seem to be correct. Please try again." - )); + $member = DefaultAdminService::singleton()->findOrCreateDefaultAdmin(); + $member->validateCanLogin($result); + if ($result->isValid()) { + // Check if default admin credentials are correct + if (DefaultAdminService::isDefaultAdminCredentials($email, $data['Password'])) { + return $member; + } else { + $result->addError(_t( + 'SilverStripe\\Security\\Member.ERRORWRONGCRED', + "The provided details don't seem to be correct. Please try again." + )); + } } } // Attempt to identify user by email if (!$member && $email) { // Find user by email + $identifierField = Member::config()->get('unique_identifier_field'); /** @var Member $member */ $member = Member::get() - ->filter([Member::config()->get('unique_identifier_field') => $email]) + ->filter([$identifierField => $email]) ->first(); } // Validate against member if possible if ($member && !$asDefaultAdmin) { - $result = $member->checkPassword($data['Password']); + $this->checkPassword($member, $data['Password'], $result); } // Emit failure to member and form (if available) @@ -98,22 +103,61 @@ class MemberAuthenticator implements Authenticator if ($member) { $member->registerFailedLogin(); } + } elseif ($member) { + $member->registerSuccessfulLogin(); } else { - if ($member) { - $member->registerSuccessfulLogin(); - } else { - // A non-existing member occurred. This will make the result "valid" so let's invalidate - $result->addError(_t( - 'SilverStripe\\Security\\Member.ERRORWRONGCRED', - "The provided details don't seem to be correct. Please try again." - )); - $member = null; - } + // A non-existing member occurred. This will make the result "valid" so let's invalidate + $result->addError(_t( + 'SilverStripe\\Security\\Member.ERRORWRONGCRED', + "The provided details don't seem to be correct. Please try again." + )); + return null; } return $member; } + /** + * Check if the passed password matches the stored one (if the member is not locked out). + * + * Note, we don't return early, to prevent differences in timings to give away if a member + * password is invalid. + * + * @param Member $member + * @param string $password + * @param ValidationResult $result + * @return ValidationResult + */ + public function checkPassword(Member $member, $password, ValidationResult &$result = null) + { + // Check if allowed to login + $result = $member->validateCanLogin($result); + if (!$result->isValid()) { + return $result; + } + + // Allow default admin to login as self + if (DefaultAdminService::isDefaultAdminCredentials($member->Email, $password)) { + return $result; + } + + // Check a password is set on this member + if (empty($member->Password) && $member->exists()) { + $result->addError(_t(__CLASS__ . '.NoPassword', 'There is no password on this member.')); + } + + $encryptor = PasswordEncryptor::create_for_algorithm($member->PasswordEncryption); + if (!$encryptor->check($member->Password, $password, $member->Salt, $member)) { + $result->addError(_t( + __CLASS__ . '.ERRORWRONGCRED', + 'The provided details don\'t seem to be correct. Please try again.' + )); + } + + return $result; + } + + /** * Log login attempt * TODO We could handle this with an extension @@ -142,7 +186,7 @@ class MemberAuthenticator implements Authenticator $attempt->Status = 'Success'; // Audit logging hook - $member->extend('authenticated'); + $member->extend('authenticationSucceeded'); } else { // Failed login - we're trying to see if a user exists with this email (disregarding wrong passwords) $attempt->Status = 'Failure'; diff --git a/src/Security/MemberAuthenticator/MemberLoginForm.php b/src/Security/MemberAuthenticator/MemberLoginForm.php index ef61dfc4f..b6b7e8809 100644 --- a/src/Security/MemberAuthenticator/MemberLoginForm.php +++ b/src/Security/MemberAuthenticator/MemberLoginForm.php @@ -77,8 +77,7 @@ class MemberLoginForm extends BaseLoginForm $actions = null, $checkCurrentUser = true ) { - - $this->controller = $controller; + $this->setController($controller); $this->authenticator_class = $authenticatorClass; $customCSS = project() . '/css/member_login.css'; @@ -125,13 +124,14 @@ class MemberLoginForm extends BaseLoginForm */ protected function getFormFields() { - if ($this->controller->request->getVar('BackURL')) { - $backURL = $this->controller->request->getVar('BackURL'); + $request = $this->getController()->getRequest(); + if ($request->getVar('BackURL')) { + $backURL = $request->getVar('BackURL'); } else { $backURL = Session::get('BackURL'); } - $label = Member::singleton()->fieldLabel(Member::config()->unique_identifier_field); + $label = Member::singleton()->fieldLabel(Member::config()->get('unique_identifier_field')); $fields = FieldList::create( HiddenField::create("AuthenticationMethod", null, $this->authenticator_class, $this), // Regardless of what the unique identifer field is (usually 'Email'), it will be held in the diff --git a/src/Security/MemberPassword.php b/src/Security/MemberPassword.php index e739a40c1..30b557c45 100644 --- a/src/Security/MemberPassword.php +++ b/src/Security/MemberPassword.php @@ -7,10 +7,10 @@ use SilverStripe\ORM\DataObject; /** * Keep track of users' previous passwords, so that we can check that new passwords aren't changed back to old ones. * - * @property string Password - * @property string Salt - * @property string PasswordEncryption - * @property int MemberID ID of the Member + * @property string $Password + * @property string $Salt + * @property string $PasswordEncryption + * @property int $MemberID ID of the Member * @method Member Member() Owner of the password */ class MemberPassword extends DataObject @@ -21,9 +21,9 @@ class MemberPassword extends DataObject 'PasswordEncryption' => 'Varchar(50)', ); - private static $has_one = array( - 'Member' => 'SilverStripe\\Security\\Member' - ); + private static $has_one = [ + 'Member' => Member::class, + ]; private static $table_name = "MemberPassword"; @@ -47,12 +47,12 @@ class MemberPassword extends DataObject * Check if the given password is the same as the one stored in this record. * See {@link Member->checkPassword()}. * - * @param String $password Cleartext password - * @return Boolean + * @param string $password Cleartext password + * @return bool */ public function checkPassword($password) { - $e = PasswordEncryptor::create_for_algorithm($this->PasswordEncryption); - return $e->check($this->Password, $password, $this->Salt, $this->Member()); + $encryptor = PasswordEncryptor::create_for_algorithm($this->PasswordEncryption); + return $encryptor->check($this->Password, $password, $this->Salt, $this->Member()); } } diff --git a/src/Security/Security.php b/src/Security/Security.php index 2fc3b1312..2c39d18c0 100644 --- a/src/Security/Security.php +++ b/src/Security/Security.php @@ -25,10 +25,10 @@ use SilverStripe\ORM\DB; use SilverStripe\ORM\FieldType\DBField; use SilverStripe\ORM\FieldType\DBHTMLText; use SilverStripe\ORM\ValidationResult; +use SilverStripe\Security\DefaultAdminService; use SilverStripe\View\ArrayData; use SilverStripe\View\SSViewer; use SilverStripe\View\TemplateGlobalProvider; -use Subsite; /** * Implements a basic security model @@ -47,22 +47,6 @@ class Security extends Controller implements TemplateGlobalProvider 'ping', ); - /** - * Default user name. {@link setDefaultAdmin()} - * - * @var string - * @see setDefaultAdmin() - */ - protected static $default_username; - - /** - * Default password. {@link setDefaultAdmin()} - * - * @var string - * @see setDefaultAdmin() - */ - protected static $default_password; - /** * If set to TRUE to prevent sharing of the session across several sites * in the domain. @@ -286,7 +270,7 @@ class Security extends Controller implements TemplateGlobalProvider */ public function getApplicableAuthenticators($service = Authenticator::LOGIN) { - $authenticators = $this->authenticators; + $authenticators = $this->getAuthenticators(); /** @var Authenticator $authenticator */ foreach ($authenticators as $name => $authenticator) { @@ -295,6 +279,10 @@ class Security extends Controller implements TemplateGlobalProvider } } + if (empty($authenticators)) { + throw new LogicException('No applicable authenticators found'); + } + return $authenticators; } @@ -957,57 +945,27 @@ class Security extends Controller implements TemplateGlobalProvider * purposes outside of any default credentials set through {@link Security::setDefaultAdmin()}. * * @return Member + * + * @deprecated 4.0.0..5.0.0 Please use DefaultAdminService::findOrCreateDefaultAdmin() */ public static function findAnAdministrator() { - static::singleton()->extend('beforeFindAdministrator'); + Deprecation::notice('5.0.0', 'Please use DefaultAdminService::findOrCreateDefaultAdmin()'); - /** @var Member $member */ - $member = null; - - // find a group with ADMIN permission - $adminGroup = Permission::get_groups_by_permission('ADMIN')->first(); - - if (!$adminGroup) { - Group::singleton()->requireDefaultRecords(); - $adminGroup = Permission::get_groups_by_permission('ADMIN')->first(); - } - - $member = $adminGroup->Members()->First(); - - if (!$member) { - Member::singleton()->requireDefaultRecords(); - $member = Permission::get_members_by_permission('ADMIN')->first(); - } - - if (!$member) { - $member = Member::default_admin(); - } - - if (!$member) { - // Failover to a blank admin - $member = Member::create(); - $member->FirstName = _t('SilverStripe\\Security\\Member.DefaultAdminFirstname', 'Default Admin'); - $member->write(); - // Add member to group instead of adding group to member - // This bypasses the privilege escallation code in Member_GroupSet - $adminGroup - ->DirectMembers() - ->add($member); - } - - static::singleton()->extend('afterFindAdministrator'); - - return $member; + $service = DefaultAdminService::singleton(); + return $service->findOrCreateDefaultAdmin(); } /** * Flush the default admin credentials + * + * @deprecated 4.0.0..5.0.0 Please use DefaultAdminService::clearDefaultAdmin() */ public static function clear_default_admin() { - self::$default_username = null; - self::$default_password = null; + Deprecation::notice('5.0.0', 'Please use DefaultAdminService::clearDefaultAdmin()'); + + DefaultAdminService::clearDefaultAdmin(); } @@ -1022,17 +980,14 @@ class Security extends Controller implements TemplateGlobalProvider * @param string $username The user name * @param string $password The password (in cleartext) * @return bool True if successfully set + * + * @deprecated 4.0.0..5.0.0 Please use DefaultAdminService::setDefaultAdmin($username, $password) */ public static function setDefaultAdmin($username, $password) { - // don't overwrite if already set - if (self::$default_username || self::$default_password) { - return false; - } - - self::$default_username = $username; - self::$default_password = $password; + Deprecation::notice('5.0.0', 'Please use DefaultAdminService::setDefaultAdmin($username, $password)'); + DefaultAdminService::setDefaultAdmin($username, $password); return true; } @@ -1043,42 +998,53 @@ class Security extends Controller implements TemplateGlobalProvider * @param string $username * @param string $password * @return bool + * + * @deprecated 4.0.0..5.0.0 Use DefaultAdminService::isDefaultAdminCredentials() instead */ public static function check_default_admin($username, $password) { - return ( - self::$default_username === $username - && self::$default_password === $password - && self::has_default_admin() - ); + Deprecation::notice('5.0.0', 'Please use DefaultAdminService::isDefaultAdminCredentials($username, $password)'); + + /** @var DefaultAdminService $service */ + return DefaultAdminService::isDefaultAdminCredentials($username, $password); } /** * Check that the default admin account has been set. + * + * @deprecated 4.0.0..5.0.0 Use DefaultAdminService::hasDefaultAdmin() instead */ public static function has_default_admin() { - return !empty(self::$default_username) && !empty(self::$default_password); + Deprecation::notice('5.0.0', 'Please use DefaultAdminService::hasDefaultAdmin()'); + + return DefaultAdminService::hasDefaultAdmin(); } /** * Get default admin username * + * @deprecated 4.0.0..5.0.0 Use DefaultAdminService::getDefaultAdminUsername() * @return string */ public static function default_admin_username() { - return self::$default_username; + Deprecation::notice('5.0.0', 'Please use DefaultAdminService::getDefaultAdminUsername()'); + + return DefaultAdminService::getDefaultAdminUsername(); } /** * Get default admin password * + * @deprecated 4.0.0..5.0.0 Use DefaultAdminService::getDefaultAdminPassword() * @return string */ public static function default_admin_password() { - return self::$default_password; + Deprecation::notice('5.0.0', 'Please use DefaultAdminService::getDefaultAdminPassword()'); + + return DefaultAdminService::getDefaultAdminPassword(); } /** @@ -1115,16 +1081,16 @@ class Security extends Controller implements TemplateGlobalProvider $algorithm = self::config()->get('password_encryption_algorithm'); } - $e = PasswordEncryptor::create_for_algorithm($algorithm); + $encryptor = PasswordEncryptor::create_for_algorithm($algorithm); // New salts will only need to be generated if the password is hashed for the first time - $salt = ($salt) ? $salt : $e->salt($password); + $salt = ($salt) ? $salt : $encryptor->salt($password); return array( - 'password' => $e->encrypt($password, $salt, $member), + 'password' => $encryptor->encrypt($password, $salt, $member), 'salt' => $salt, 'algorithm' => $algorithm, - 'encryptor' => $e + 'encryptor' => $encryptor ); } diff --git a/src/conf/ConfigureFromEnv.php b/src/conf/ConfigureFromEnv.php index bf3738e3c..1dc60af2a 100644 --- a/src/conf/ConfigureFromEnv.php +++ b/src/conf/ConfigureFromEnv.php @@ -46,14 +46,14 @@ * - SS_SEND_ALL_EMAILS_FROM: If you set this define, all emails will be send from this address. */ -use Monolog\Logger; use Monolog\Handler\StreamHandler; +use Monolog\Logger; use Psr\Log\LoggerInterface; use SilverStripe\Control\Email\Email; use SilverStripe\Core\Injector\Injector; use SilverStripe\Dev\Install\DatabaseAdapterRegistry; use SilverStripe\Security\BasicAuth; -use SilverStripe\Security\Security; +use SilverStripe\Security\DefaultAdminService; global $database; @@ -131,7 +131,7 @@ if ($defaultAdminUser = getenv('SS_DEFAULT_ADMIN_USERNAME')) { E_USER_ERROR ); } else { - Security::setDefaultAdmin($defaultAdminUser, $defaultAdminPass); + DefaultAdminService::setDefaultAdmin($defaultAdminUser, $defaultAdminPass); } } if ($useBasicAuth = getenv('SS_USE_BASIC_AUTH')) { diff --git a/tests/php/Security/MemberAuthenticatorTest.php b/tests/php/Security/MemberAuthenticatorTest.php index 699ccac8c..139128b59 100644 --- a/tests/php/Security/MemberAuthenticatorTest.php +++ b/tests/php/Security/MemberAuthenticatorTest.php @@ -5,6 +5,7 @@ namespace SilverStripe\Security\Tests; use SilverStripe\Core\Injector\Injector; use SilverStripe\ORM\DataModel; use SilverStripe\ORM\FieldType\DBDatetime; +use SilverStripe\ORM\ValidationResult; use SilverStripe\Security\Authenticator; use SilverStripe\Security\MemberAuthenticator\CMSMemberAuthenticator; use SilverStripe\Security\MemberAuthenticator\CMSMemberLoginForm; @@ -16,6 +17,7 @@ use SilverStripe\Security\IdentityStore; use SilverStripe\Core\Config\Config; use SilverStripe\Dev\SapphireTest; use SilverStripe\Control\HTTPRequest; +use SilverStripe\Security\DefaultAdminService; class MemberAuthenticatorTest extends SapphireTest { @@ -29,29 +31,35 @@ class MemberAuthenticatorTest extends SapphireTest { parent::setUp(); - $this->defaultUsername = Security::default_admin_username(); - $this->defaultPassword = Security::default_admin_password(); - Security::clear_default_admin(); - Security::setDefaultAdmin('admin', 'password'); + if (DefaultAdminService::hasDefaultAdmin()) { + $this->defaultUsername = DefaultAdminService::getDefaultAdminUsername(); + $this->defaultPassword = DefaultAdminService::getDefaultAdminPassword(); + DefaultAdminService::clearDefaultAdmin(); + } else { + $this->defaultUsername = null; + $this->defaultPassword = null; + } + DefaultAdminService::clearDefaultAdmin(); + DefaultAdminService::setDefaultAdmin('admin', 'password'); } protected function tearDown() { - Security::setDefaultAdmin($this->defaultUsername, $this->defaultPassword); + DefaultAdminService::clearDefaultAdmin(); + if ($this->defaultUsername) { + DefaultAdminService::setDefaultAdmin($this->defaultUsername, $this->defaultPassword); + } parent::tearDown(); } public function testCustomIdentifierField() { + Member::config()->set('unique_identifier_field', 'Username'); - $origField = Member::config()->unique_identifier_field; - Member::config()->unique_identifier_field = 'Username'; - - $label=singleton(Member::class)->fieldLabel(Member::config()->unique_identifier_field); + $label = Member::singleton() + ->fieldLabel(Member::config()->get('unique_identifier_field')); $this->assertEquals($label, 'Username'); - - Member::config()->unique_identifier_field = $origField; } public function testGenerateLoginForm() @@ -106,6 +114,7 @@ class MemberAuthenticatorTest extends SapphireTest $this->assertNotEmpty($tempID); // Test correct login + /** @var ValidationResult $message */ $result = $authenticator->authenticate( array( 'tempid' => $tempID, @@ -143,6 +152,7 @@ class MemberAuthenticatorTest extends SapphireTest $authenticator = new MemberAuthenticator(); // Test correct login + /** @var ValidationResult $message */ $result = $authenticator->authenticate( array( 'Email' => 'admin', @@ -151,7 +161,7 @@ class MemberAuthenticatorTest extends SapphireTest $message ); $this->assertNotEmpty($result); - $this->assertEquals($result->Email, Security::default_admin_username()); + $this->assertEquals($result->Email, DefaultAdminService::getDefaultAdminUsername()); $this->assertTrue($message->isValid()); // Test incorrect login @@ -174,8 +184,8 @@ class MemberAuthenticatorTest extends SapphireTest { $authenticator = new MemberAuthenticator(); - Config::inst()->update(Member::class, 'lock_out_after_incorrect_logins', 1); - Config::inst()->update(Member::class, 'lock_out_delay_mins', 10); + Config::modify()->set(Member::class, 'lock_out_after_incorrect_logins', 1); + Config::modify()->set(Member::class, 'lock_out_delay_mins', 10); DBDatetime::set_mock_now('2016-04-18 00:00:00'); // Test correct login @@ -186,7 +196,9 @@ class MemberAuthenticatorTest extends SapphireTest ] ); - $this->assertFalse(Member::default_admin()->canLogin()->isValid()); - $this->assertEquals('2016-04-18 00:10:00', Member::default_admin()->LockedOutUntil); + $defaultAdmin = DefaultAdminService::singleton()->findOrCreateDefaultAdmin(); + $this->assertNotNull($defaultAdmin); + $this->assertFalse($defaultAdmin->canLogin()); + $this->assertEquals('2016-04-18 00:10:00', $defaultAdmin->LockedOutUntil); } } diff --git a/tests/php/Security/MemberTest.php b/tests/php/Security/MemberTest.php index 4420a814c..a44b8aa3d 100644 --- a/tests/php/Security/MemberTest.php +++ b/tests/php/Security/MemberTest.php @@ -2,26 +2,27 @@ namespace SilverStripe\Security\Tests; +use SilverStripe\Control\Cookie; use SilverStripe\Core\Convert; use SilverStripe\Core\Injector\Injector; use SilverStripe\Dev\FunctionalTest; -use SilverStripe\Control\Cookie; use SilverStripe\i18n\i18n; use SilverStripe\ORM\DataObject; use SilverStripe\ORM\DB; use SilverStripe\ORM\FieldType\DBDatetime; -use SilverStripe\Security\Member; -use SilverStripe\Security\MemberAuthenticator\SessionAuthenticationHandler; -use SilverStripe\Security\Security; -use SilverStripe\Security\MemberPassword; +use SilverStripe\ORM\ValidationException; use SilverStripe\Security\Group; -use SilverStripe\Security\Permission; use SilverStripe\Security\IdentityStore; -use SilverStripe\Security\PasswordEncryptor_Blowfish; -use SilverStripe\Security\RememberLoginHash; +use SilverStripe\Security\Member; use SilverStripe\Security\Member_Validator; +use SilverStripe\Security\MemberAuthenticator\MemberAuthenticator; +use SilverStripe\Security\MemberAuthenticator\SessionAuthenticationHandler; +use SilverStripe\Security\MemberPassword; +use SilverStripe\Security\PasswordEncryptor_Blowfish; +use SilverStripe\Security\Permission; +use SilverStripe\Security\RememberLoginHash; +use SilverStripe\Security\Security; use SilverStripe\Security\Tests\MemberTest\FieldsExtension; -use SilverStripe\Control\HTTPRequest; class MemberTest extends FunctionalTest { @@ -50,24 +51,13 @@ class MemberTest extends FunctionalTest { parent::setUp(); - $this->orig['Member_unique_identifier_field'] = Member::config()->unique_identifier_field; - Member::config()->unique_identifier_field = 'Email'; + Member::config()->set('unique_identifier_field', 'Email'); Member::set_password_validator(null); } - protected function tearDown() - { - Member::config()->unique_identifier_field = $this->orig['Member_unique_identifier_field']; - parent::tearDown(); - } - - - - /** - * @expectedException \SilverStripe\ORM\ValidationException - */ public function testWriteDoesntMergeNewRecordWithExistingMember() { + $this->expectException(ValidationException::class); $m1 = new Member(); $m1->Email = 'member@test.com'; $m1->write(); @@ -101,7 +91,7 @@ class MemberTest extends FunctionalTest $memberWithPassword->write(); $this->assertEquals( $memberWithPassword->PasswordEncryption, - Security::config()->password_encryption_algorithm, + Security::config()->get('password_encryption_algorithm'), 'Password encryption is set for new member records on first write (with setting "Password")' ); @@ -120,8 +110,7 @@ class MemberTest extends FunctionalTest $member->PasswordEncryption = 'sha1_v2.4'; $member->write(); - $origAlgo = Security::config()->password_encryption_algorithm; - Security::config()->password_encryption_algorithm = 'none'; + Security::config()->set('password_encryption_algorithm', 'none'); $member->Password = 'mynewpassword'; $member->write(); @@ -130,10 +119,9 @@ class MemberTest extends FunctionalTest $member->PasswordEncryption, 'sha1_v2.4' ); - $result = $member->checkPassword('mynewpassword'); + $auth = new MemberAuthenticator(); + $result = $auth->checkPassword($member, 'mynewpassword'); $this->assertTrue($result->isValid()); - - Security::config()->password_encryption_algorithm = $origAlgo; } public function testKeepsEncryptionOnEmptyPasswords() @@ -150,16 +138,19 @@ class MemberTest extends FunctionalTest $member->PasswordEncryption, 'sha1_v2.4' ); - $result = $member->checkPassword(''); + $auth = new MemberAuthenticator(); + $result = $auth->checkPassword($member, ''); $this->assertTrue($result->isValid()); } public function testSetPassword() { + /** @var Member $member */ $member = $this->objFromFixture(Member::class, 'test'); $member->Password = "test1"; $member->write(); - $result = $member->checkPassword('test1'); + $auth = new MemberAuthenticator(); + $result = $auth->checkPassword($member, 'test1'); $this->assertTrue($result->isValid()); } @@ -168,6 +159,7 @@ class MemberTest extends FunctionalTest */ public function testPasswordChangeLogging() { + /** @var Member $member */ $member = $this->objFromFixture(Member::class, 'test'); $this->assertNotNull($member); $member->Password = "test1"; @@ -179,7 +171,7 @@ class MemberTest extends FunctionalTest $member->Password = "test3"; $member->write(); - $passwords = DataObject::get("SilverStripe\\Security\\MemberPassword", "\"MemberID\" = $member->ID", "\"Created\" DESC, \"ID\" DESC") + $passwords = DataObject::get(MemberPassword::class, "\"MemberID\" = $member->ID", "\"Created\" DESC, \"ID\" DESC") ->getIterator(); $this->assertNotNull($passwords); $passwords->rewind(); @@ -215,6 +207,7 @@ class MemberTest extends FunctionalTest $this->clearEmails(); + /** @var Member $member */ $member = $this->objFromFixture(Member::class, 'test'); $this->assertNotNull($member); $valid = $member->changePassword('32asDF##$$%%'); @@ -236,6 +229,7 @@ class MemberTest extends FunctionalTest $this->clearEmails(); $this->autoFollowRedirection = false; + /** @var Member $member */ $member = $this->objFromFixture(Member::class, 'test'); $this->assertNotNull($member); @@ -353,8 +347,9 @@ class MemberTest extends FunctionalTest */ public function testPasswordExpirySetting() { - Member::config()->password_expiry_days = 90; + Member::config()->set('password_expiry_days', 90); + /** @var Member $member */ $member = $this->objFromFixture(Member::class, 'test'); $this->assertNotNull($member); $valid = $member->changePassword("Xx?1234234"); @@ -363,7 +358,7 @@ class MemberTest extends FunctionalTest $expiryDate = date('Y-m-d', time() + 90*86400); $this->assertEquals($expiryDate, $member->PasswordExpiry); - Member::config()->password_expiry_days = null; + Member::config()->set('password_expiry_days', null); $valid = $member->changePassword("Xx?1234235"); $this->assertTrue($valid->isValid()); @@ -372,6 +367,7 @@ class MemberTest extends FunctionalTest public function testIsPasswordExpired() { + /** @var Member $member */ $member = $this->objFromFixture(Member::class, 'test'); $this->assertNotNull($member); $this->assertFalse($member->isPasswordExpired()); @@ -394,14 +390,13 @@ class MemberTest extends FunctionalTest } public function testInGroups() { + /** @var Member $staffmember */ $staffmember = $this->objFromFixture(Member::class, 'staffmember'); - $managementmember = $this->objFromFixture(Member::class, 'managementmember'); - $accountingmember = $this->objFromFixture(Member::class, 'accountingmember'); + /** @var Member $ceomember */ $ceomember = $this->objFromFixture(Member::class, 'ceomember'); $staffgroup = $this->objFromFixture(Group::class, 'staffgroup'); $managementgroup = $this->objFromFixture(Group::class, 'managementgroup'); - $accountinggroup = $this->objFromFixture(Group::class, 'accountinggroup'); $ceogroup = $this->objFromFixture(Group::class, 'ceogroup'); $this->assertTrue( @@ -420,7 +415,9 @@ class MemberTest extends FunctionalTest public function testAddToGroupByCode() { + /** @var Member $grouplessMember */ $grouplessMember = $this->objFromFixture(Member::class, 'grouplessmember'); + /** @var Group $memberlessGroup */ $memberlessGroup = $this->objFromFixture(Group::class, 'memberlessgroup'); $this->assertFalse($grouplessMember->Groups()->exists()); @@ -434,6 +431,7 @@ class MemberTest extends FunctionalTest $grouplessMember->addToGroupByCode('somegroupthatwouldneverexist', 'New Group'); $this->assertEquals($grouplessMember->Groups()->count(), 2); + /** @var Group $group */ $group = DataObject::get_one( Group::class, array( @@ -447,7 +445,9 @@ class MemberTest extends FunctionalTest public function testRemoveFromGroupByCode() { + /** @var Member $grouplessMember */ $grouplessMember = $this->objFromFixture(Member::class, 'grouplessmember'); + /** @var Group $memberlessGroup */ $memberlessGroup = $this->objFromFixture(Group::class, 'memberlessgroup'); $this->assertFalse($grouplessMember->Groups()->exists()); @@ -461,6 +461,7 @@ class MemberTest extends FunctionalTest $grouplessMember->addToGroupByCode('somegroupthatwouldneverexist', 'New Group'); $this->assertEquals($grouplessMember->Groups()->count(), 2); + /** @var Group $group */ $group = DataObject::get_one(Group::class, "\"Code\" = 'somegroupthatwouldneverexist'"); $this->assertNotNull($group); $this->assertEquals($group->Code, 'somegroupthatwouldneverexist'); @@ -476,14 +477,20 @@ class MemberTest extends FunctionalTest public function testInGroup() { + /** @var Member $staffmember */ $staffmember = $this->objFromFixture(Member::class, 'staffmember'); + /** @var Member $managementmember */ $managementmember = $this->objFromFixture(Member::class, 'managementmember'); + /** @var Member $accountingmember */ $accountingmember = $this->objFromFixture(Member::class, 'accountingmember'); + /** @var Member $ceomember */ $ceomember = $this->objFromFixture(Member::class, 'ceomember'); + /** @var Group $staffgroup */ $staffgroup = $this->objFromFixture(Group::class, 'staffgroup'); + /** @var Group $managementgroup */ $managementgroup = $this->objFromFixture(Group::class, 'managementgroup'); - $accountinggroup = $this->objFromFixture(Group::class, 'accountinggroup'); + /** @var Group $ceogroup */ $ceogroup = $this->objFromFixture(Group::class, 'ceogroup'); $this->assertTrue( @@ -614,6 +621,7 @@ class MemberTest extends FunctionalTest */ public function testName() { + /** @var Member $member */ $member = $this->objFromFixture(Member::class, 'test'); $member->setName('Test Some User'); $this->assertEquals('Test Some User', $member->getName()); @@ -645,8 +653,11 @@ class MemberTest extends FunctionalTest public function testOnChangeGroups() { + /** @var Group $staffGroup */ $staffGroup = $this->objFromFixture(Group::class, 'staffgroup'); + /** @var Member $staffMember */ $staffMember = $this->objFromFixture(Member::class, 'staffmember'); + /** @var Member $adminMember */ $adminMember = $this->objFromFixture(Member::class, 'admin'); $newAdminGroup = new Group(array('Title' => 'newadmin')); $newAdminGroup->write(); @@ -685,7 +696,9 @@ class MemberTest extends FunctionalTest */ public function testOnChangeGroupsByAdd() { + /** @var Member $staffMember */ $staffMember = $this->objFromFixture(Member::class, 'staffmember'); + /** @var Member $adminMember */ $adminMember = $this->objFromFixture(Member::class, 'admin'); // Setup new admin group @@ -736,6 +749,7 @@ class MemberTest extends FunctionalTest */ public function testOnChangeGroupsBySetIDList() { + /** @var Member $staffMember */ $staffMember = $this->objFromFixture(Member::class, 'staffmember'); // Setup new admin group @@ -865,7 +879,7 @@ class MemberTest extends FunctionalTest $m2 = new Member(); $m2->PasswordEncryption = 'blowfish'; $m2->Salt = $enc->salt('456'); - $m2Token = $m2->generateAutologinTokenAndStoreHash(); + $m2->generateAutologinTokenAndStoreHash(); $this->assertTrue($m1->validateAutoLoginToken($m1Token), 'Passes token validity test against matching member.'); $this->assertFalse($m2->validateAutoLoginToken($m1Token), 'Fails token validity test against other member.'); @@ -873,12 +887,14 @@ class MemberTest extends FunctionalTest public function testRememberMeHashGeneration() { + /** @var Member $m1 */ $m1 = $this->objFromFixture(Member::class, 'grouplessmember'); Injector::inst()->get(IdentityStore::class)->logIn($m1, true); $hashes = RememberLoginHash::get()->filter('MemberID', $m1->ID); $this->assertEquals($hashes->count(), 1); + /** @var RememberLoginHash $firstHash */ $firstHash = $hashes->first(); $this->assertNotNull($firstHash->DeviceID); $this->assertNotNull($firstHash->Hash); @@ -893,6 +909,7 @@ class MemberTest extends FunctionalTest Injector::inst()->get(IdentityStore::class)->logIn($m1, true); + /** @var RememberLoginHash $firstHash */ $firstHash = RememberLoginHash::get()->filter('MemberID', $m1->ID)->first(); $this->assertNotNull($firstHash); @@ -966,11 +983,10 @@ class MemberTest extends FunctionalTest public function testExpiredRememberMeHashAutologin() { - /** - * @var Member $m1 -*/ + /** @var Member $m1 */ $m1 = $this->objFromFixture(Member::class, 'noexpiry'); Injector::inst()->get(IdentityStore::class)->logIn($m1, true); + /** @var RememberLoginHash $firstHash */ $firstHash = RememberLoginHash::get()->filter('MemberID', $m1->ID)->first(); $this->assertNotNull($firstHash); @@ -1026,6 +1042,7 @@ class MemberTest extends FunctionalTest public function testRememberMeMultipleDevices() { + /** @var Member $m1 */ $m1 = $this->objFromFixture(Member::class, 'noexpiry'); // First device @@ -1035,10 +1052,12 @@ class MemberTest extends FunctionalTest Injector::inst()->get(IdentityStore::class)->logIn($m1, true); // Hash of first device + /** @var RememberLoginHash $firstHash */ $firstHash = RememberLoginHash::get()->filter('MemberID', $m1->ID)->first(); $this->assertNotNull($firstHash); // Hash of second device + /** @var RememberLoginHash $secondHash */ $secondHash = RememberLoginHash::get()->filter('MemberID', $m1->ID)->last(); $this->assertNotNull($secondHash); @@ -1093,7 +1112,7 @@ class MemberTest extends FunctionalTest // Logging out from the second device - only one device being logged out RememberLoginHash::config()->update('logout_across_devices', false); - $response = $this->get( + $this->get( 'Security/logout', $this->session(), null, @@ -1110,7 +1129,7 @@ class MemberTest extends FunctionalTest // Logging out from any device when all login hashes should be removed RememberLoginHash::config()->update('logout_across_devices', true); Injector::inst()->get(IdentityStore::class)->logIn($m1, true); - $response = $this->get('Security/logout', $this->session()); + $this->get('Security/logout', $this->session()); $this->assertEquals( RememberLoginHash::get()->filter('MemberID', $m1->ID)->count(), 0 @@ -1152,6 +1171,7 @@ class MemberTest extends FunctionalTest //set up the config variables to enable login lockouts Member::config()->update('lock_out_after_incorrect_logins', $maxFailedLoginsAllowed); + /** @var Member $member */ $member = $this->objFromFixture(Member::class, 'test'); $failedLoginCount = $member->FailedLoginCount; @@ -1165,7 +1185,7 @@ class MemberTest extends FunctionalTest ); $this->assertTrue( - $member->canLogin()->isValid(), + $member->canLogin(), "Member has been locked out too early" ); } @@ -1175,7 +1195,9 @@ class MemberTest extends FunctionalTest { // clear custom requirements for this test Member_Validator::config()->update('customRequired', null); + /** @var Member $memberA */ $memberA = $this->objFromFixture(Member::class, 'admin'); + /** @var Member $memberB */ $memberB = $this->objFromFixture(Member::class, 'test'); // create a blank form diff --git a/tests/php/Security/SecurityDefaultAdminTest.php b/tests/php/Security/SecurityDefaultAdminTest.php index 192566883..8e62f9110 100644 --- a/tests/php/Security/SecurityDefaultAdminTest.php +++ b/tests/php/Security/SecurityDefaultAdminTest.php @@ -2,17 +2,17 @@ namespace SilverStripe\Security\Tests; -use SilverStripe\Security\Security; -use SilverStripe\Security\Permission; -use SilverStripe\Security\Member; use SilverStripe\Dev\SapphireTest; +use SilverStripe\Security\Member; +use SilverStripe\Security\Permission; +use SilverStripe\Security\DefaultAdminService; class SecurityDefaultAdminTest extends SapphireTest { - protected $usesDatabase = true; protected $defaultUsername = null; + protected $defaultPassword = null; protected function setUp() @@ -26,33 +26,41 @@ class SecurityDefaultAdminTest extends SapphireTest } self::empty_temp_db(); - $this->defaultUsername = Security::default_admin_username(); - $this->defaultPassword = Security::default_admin_password(); - Security::clear_default_admin(); - Security::setDefaultAdmin('admin', 'password'); + if (DefaultAdminService::hasDefaultAdmin()) { + $this->defaultUsername = DefaultAdminService::getDefaultAdminUsername(); + $this->defaultPassword = DefaultAdminService::getDefaultAdminPassword(); + DefaultAdminService::clearDefaultAdmin(); + } else { + $this->defaultUsername = null; + $this->defaultPassword = null; + } + DefaultAdminService::setDefaultAdmin('admin', 'password'); Permission::reset(); } protected function tearDown() { - Security::setDefaultAdmin($this->defaultUsername, $this->defaultPassword); + DefaultAdminService::clearDefaultAdmin(); + if ($this->defaultUsername) { + DefaultAdminService::setDefaultAdmin($this->defaultUsername, $this->defaultPassword); + } Permission::reset(); parent::tearDown(); } public function testCheckDefaultAdmin() { - $this->assertTrue(Security::has_default_admin()); + $this->assertTrue(DefaultAdminService::hasDefaultAdmin()); $this->assertTrue( - Security::check_default_admin('admin', 'password'), + DefaultAdminService::isDefaultAdminCredentials('admin', 'password'), 'Succeeds with correct username and password' ); $this->assertFalse( - Security::check_default_admin('wronguser', 'password'), + DefaultAdminService::isDefaultAdminCredentials('wronguser', 'password'), 'Fails with incorrect username' ); $this->assertFalse( - Security::check_default_admin('admin', 'wrongpassword'), + DefaultAdminService::isDefaultAdminCredentials('admin', 'wrongpassword'), 'Fails with incorrect password' ); } @@ -62,29 +70,34 @@ class SecurityDefaultAdminTest extends SapphireTest $adminMembers = Permission::get_members_by_permission('ADMIN'); $this->assertEquals(0, $adminMembers->count()); - $admin = Security::findAnAdministrator(); + $admin = DefaultAdminService::singleton()->findOrCreateDefaultAdmin(); $this->assertInstanceOf(Member::class, $admin); $this->assertTrue(Permission::checkMember($admin, 'ADMIN')); - $this->assertEquals($admin->Email, Security::default_admin_username()); + $this->assertEquals($admin->Email, DefaultAdminService::getDefaultAdminUsername()); + $this->assertTrue(DefaultAdminService::isDefaultAdmin($admin->Email)); $this->assertNull($admin->Password); } public function testFindAnAdministratorWithoutDefaultAdmin() { // Clear default admin - Security::clear_default_admin(); + $service = DefaultAdminService::singleton(); + DefaultAdminService::clearDefaultAdmin(); $adminMembers = Permission::get_members_by_permission('ADMIN'); $this->assertEquals(0, $adminMembers->count()); - $admin = Security::findAnAdministrator(); + $admin = $service->findOrCreateDefaultAdmin(); + $this->assertNull($admin); - $this->assertInstanceOf(Member::class, $admin); + // When clearing the admin, it will not re-instate it anymore + DefaultAdminService::setDefaultAdmin('admin', 'password'); + $admin = $service->findOrCreateDefaultAdmin(); $this->assertTrue(Permission::checkMember($admin, 'ADMIN')); - // User should be blank - $this->assertEmpty($admin->Email); + // User should have Email but no Password + $this->assertEquals('admin', $admin->Email); $this->assertEmpty($admin->Password); } @@ -93,11 +106,11 @@ class SecurityDefaultAdminTest extends SapphireTest $adminMembers = Permission::get_members_by_permission('ADMIN'); $this->assertEquals(0, $adminMembers->count()); - $admin = Member::default_admin(); - + $admin = DefaultAdminService::singleton()->findOrCreateDefaultAdmin(); $this->assertInstanceOf(Member::class, $admin); $this->assertTrue(Permission::checkMember($admin, 'ADMIN')); - $this->assertEquals($admin->Email, Security::default_admin_username()); + $this->assertEquals($admin->Email, DefaultAdminService::getDefaultAdminUsername()); + $this->assertTrue(DefaultAdminService::isDefaultAdmin($admin->Email)); $this->assertNull($admin->Password); } } diff --git a/tests/php/Security/SecurityTest.php b/tests/php/Security/SecurityTest.php index adbf3ffc4..029565661 100644 --- a/tests/php/Security/SecurityTest.php +++ b/tests/php/Security/SecurityTest.php @@ -31,14 +31,6 @@ class SecurityTest extends FunctionalTest protected $autoFollowRedirection = false; - protected $priorAuthenticators = array(); - - protected $priorDefaultAuthenticator = null; - - protected $priorUniqueIdentifierField = null; - - protected $priorRememberUsername = null; - protected static $extra_controllers = [ SecurityTest\NullController::class, SecurityTest\SecuredController::class, @@ -50,9 +42,6 @@ class SecurityTest extends FunctionalTest Config::modify()->set(MemberAuthenticator::class, 'authenticators', []); Config::modify()->set(MemberAuthenticator::class, 'default_authenticator', MemberAuthenticator::class); - // And that the unique identified field is 'Email' - $this->priorUniqueIdentifierField = Member::config()->unique_identifier_field; - $this->priorRememberUsername = Security::config()->remember_username; /** * @skipUpgrade */ @@ -63,21 +52,6 @@ class SecurityTest extends FunctionalTest Config::modify()->merge('SilverStripe\\Control\\Director', 'alternate_base_url', '/'); } - protected function tearDown() - { - // Restore selected authenticator - - // MemberAuthenticator might not actually be present - // Config::modify()->set(Authenticator::class, 'authenticators', $this->priorAuthenticators); - // Config::modify()->set(Authenticator::class, 'default_authenticator', $this->priorDefaultAuthenticator); - - // Restore unique identifier field - Member::config()->unique_identifier_field = $this->priorUniqueIdentifierField; - Security::config()->remember_username = $this->priorRememberUsername; - - parent::tearDown(); - } - public function testAccessingAuthenticatedPageRedirectsToLoginForm() { $this->autoFollowRedirection = false;