From 576eee72dc9eb50a50f8ddc428c3ccd3274256fc Mon Sep 17 00:00:00 2001 From: Simon Erkelens Date: Tue, 13 Jun 2017 21:04:43 +1200 Subject: [PATCH] Remove DefaultAdmin things from Security and Member into the MemberAuthenticator, unifying and removing duplicate code. --- sh.exe.stackdump | 4 + src/Forms/ConfirmedPasswordField.php | 26 +-- src/Security/Member.php | 5 +- .../CMSMemberAuthenticator.php | 2 +- .../MemberAuthenticator.php | 54 +++++- src/Security/Security.php | 103 ++++------- src/Security/Service/DefaultAdminService.php | 165 ++++++++++++++++++ src/conf/ConfigureFromEnv.php | 4 +- .../php/Security/MemberAuthenticatorTest.php | 14 +- .../php/Security/SecurityDefaultAdminTest.php | 13 +- 10 files changed, 287 insertions(+), 103 deletions(-) create mode 100644 sh.exe.stackdump create mode 100644 src/Security/Service/DefaultAdminService.php 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/Forms/ConfirmedPasswordField.php b/src/Forms/ConfirmedPasswordField.php index 886039803..b6ecab65f 100644 --- a/src/Forms/ConfirmedPasswordField.php +++ b/src/Forms/ConfirmedPasswordField.php @@ -4,6 +4,7 @@ namespace SilverStripe\Forms; use SilverStripe\ORM\DataObject; use SilverStripe\ORM\DataObjectInterface; +use SilverStripe\Security\Authenticator; use SilverStripe\Security\Member; use SilverStripe\Security\Security; use SilverStripe\View\Requirements; @@ -519,17 +520,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::CHANGE_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/Security/Member.php b/src/Security/Member.php index e7895b665..5adab44b1 100644 --- a/src/Security/Member.php +++ b/src/Security/Member.php @@ -31,6 +31,7 @@ use SilverStripe\ORM\Map; use SilverStripe\ORM\SS_List; use SilverStripe\ORM\ValidationException; use SilverStripe\ORM\ValidationResult; +use SilverStripe\Security\Service\DefaultAdminService; /** * The member class which represents the users of the system @@ -259,7 +260,9 @@ class Member extends DataObject { parent::requireDefaultRecords(); // Default groups should've been built by Group->requireDefaultRecords() already - static::default_admin(); + $service = Injector::inst()->get(DefaultAdminService::class); + + $service->findOrCreateDefaultAdmin(); } /** diff --git a/src/Security/MemberAuthenticator/CMSMemberAuthenticator.php b/src/Security/MemberAuthenticator/CMSMemberAuthenticator.php index 5f3cd3403..92bc3acca 100644 --- a/src/Security/MemberAuthenticator/CMSMemberAuthenticator.php +++ b/src/Security/MemberAuthenticator/CMSMemberAuthenticator.php @@ -27,7 +27,7 @@ class CMSMemberAuthenticator extends MemberAuthenticator // 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/MemberAuthenticator.php b/src/Security/MemberAuthenticator/MemberAuthenticator.php index d40f0b37c..3085579d8 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\Core\Injector\Injector; use SilverStripe\ORM\ValidationResult; use SilverStripe\Security\Authenticator; use SilverStripe\Security\LoginAttempt; use SilverStripe\Security\Member; use SilverStripe\Security\Security; +use SilverStripe\Security\Service\DefaultAdminService; /** * Authenticator for the default "member" method @@ -19,6 +22,7 @@ use SilverStripe\Security\Security; */ class MemberAuthenticator implements Authenticator { + use Extensible; public function supportedServices() { @@ -53,23 +57,25 @@ 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) - * @return Member Found member, regardless of successful login + * @return Member|null Found member, regardless of successful login */ protected function authenticateMember($data, &$result = null, $member = null) { - // Default success to false $email = !empty($data['Email']) ? $data['Email'] : null; + // Default success to false $result = new ValidationResult(); // Check default login (see Security::setDefaultAdmin()) - $asDefaultAdmin = $email === Security::default_admin_username(); + $asDefaultAdmin = $email === DefaultAdminService::getDefaultAdminUsername(); 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']); + /** @var Member $member */ + $service = Injector::inst()->get(DefaultAdminService::class); + $member = $service->findOrCreateDefaultAdmin(); + $validAdmin = $service->validateDefaultAdmin($email, $data['Password']); $result = $member->canLogIn(); //protect against failed login - if ($success && $result->isValid()) { + if ($validAdmin->isValid() && $result->isValid()) { return $member; } else { $result->addError(_t( @@ -82,9 +88,10 @@ class MemberAuthenticator implements Authenticator // 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(); } @@ -114,6 +121,37 @@ class MemberAuthenticator implements Authenticator 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 + * @return ValidationResult + */ + public function checkPassword($member, $password) + { + $result = $member->canLogIn(); + + // 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 +180,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/Security.php b/src/Security/Security.php index 2fc3b1312..c891a6848 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\Service\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. @@ -957,57 +941,28 @@ class Security extends Controller implements TemplateGlobalProvider * purposes outside of any default credentials set through {@link Security::setDefaultAdmin()}. * * @return Member + * + * @deprecated 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; + $service = Injector::inst()->get(DefaultAdminService::class); - // 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; + return $service->findOrCreateDefaultAdmin(); } /** * Flush the default admin credentials + * + * @deprecated 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,18 +977,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 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; - } + Deprecation::notice('5.0.0', 'Please use DefaultAdminService::setDefaultAdmin($username, $password)'); - self::$default_username = $username; - self::$default_password = $password; - - return true; + return DefaultAdminService::setDefaultAdmin($username, $password); } /** @@ -1043,14 +994,16 @@ class Security extends Controller implements TemplateGlobalProvider * @param string $username * @param string $password * @return bool + * + * @deprecated 5.0.0 */ 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::validateDefaultAdmin($username, $password)'); + + $service = Injector::inst()->get(DefaultAdminService::class); + + return $service->validateDefaultAdmin($username, $password)->isValid(); } /** @@ -1058,7 +1011,9 @@ class Security extends Controller implements TemplateGlobalProvider */ 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(); } /** @@ -1068,7 +1023,9 @@ class Security extends Controller implements TemplateGlobalProvider */ public static function default_admin_username() { - return self::$default_username; + Deprecation::notice('5.0.0', 'Please use DefaultAdminService::getDefaultAdminUsername()'); + + return DefaultAdminService::getDefaultAdminUsername(); } /** @@ -1078,7 +1035,9 @@ class Security extends Controller implements TemplateGlobalProvider */ public static function default_admin_password() { - return self::$default_password; + Deprecation::notice('5.0.0', 'Please use DefaultAdminService::getDefaultAdminPassword()'); + + return DefaultAdminService::getDefaultAdminPassword(); } /** diff --git a/src/Security/Service/DefaultAdminService.php b/src/Security/Service/DefaultAdminService.php new file mode 100644 index 000000000..8cd0997d6 --- /dev/null +++ b/src/Security/Service/DefaultAdminService.php @@ -0,0 +1,165 @@ +extend('beforeFindAdministrator'); + + // Check if we have default admins + if ( + !static::$has_default_admin || + empty(static::$default_username) || + empty(static::$default_password) + ) { + 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('afterFindAnAdministrator'); + + return $admin; + } + + /** + * @param string $username + * @param string $password + * @return ValidationResult + */ + public function validateDefaultAdmin($username, $password) + { + $result = new ValidationResult(); + if ( + static::$default_username === $username + && static::$default_password === $password + && static::$has_default_admin + ) { + return $result; + } + + $result->addError('No valid default admin found'); + + return $result; + } +} \ No newline at end of file diff --git a/src/conf/ConfigureFromEnv.php b/src/conf/ConfigureFromEnv.php index bf3738e3c..c3def19cb 100644 --- a/src/conf/ConfigureFromEnv.php +++ b/src/conf/ConfigureFromEnv.php @@ -51,9 +51,11 @@ use Monolog\Handler\StreamHandler; use Psr\Log\LoggerInterface; use SilverStripe\Control\Email\Email; use SilverStripe\Core\Injector\Injector; +use SilverStripe\Dev\Debug; use SilverStripe\Dev\Install\DatabaseAdapterRegistry; use SilverStripe\Security\BasicAuth; use SilverStripe\Security\Security; +use SilverStripe\Security\Service\DefaultAdminService; global $database; @@ -131,7 +133,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..60d702039 100644 --- a/tests/php/Security/MemberAuthenticatorTest.php +++ b/tests/php/Security/MemberAuthenticatorTest.php @@ -16,6 +16,7 @@ use SilverStripe\Security\IdentityStore; use SilverStripe\Core\Config\Config; use SilverStripe\Dev\SapphireTest; use SilverStripe\Control\HTTPRequest; +use SilverStripe\Security\Service\DefaultAdminService; class MemberAuthenticatorTest extends SapphireTest { @@ -29,15 +30,16 @@ 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'); + $this->defaultUsername = DefaultAdminService::getDefaultAdminUsername(); + $this->defaultPassword = DefaultAdminService::getDefaultAdminPassword(); + DefaultAdminService::clearDefaultAdmin(); + DefaultAdminService::setDefaultAdmin('admin', 'password'); } protected function tearDown() { - Security::setDefaultAdmin($this->defaultUsername, $this->defaultPassword); + DefaultAdminService::clearDefaultAdmin(); + DefaultAdminService::setDefaultAdmin($this->defaultUsername, $this->defaultPassword); parent::tearDown(); } @@ -151,7 +153,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 diff --git a/tests/php/Security/SecurityDefaultAdminTest.php b/tests/php/Security/SecurityDefaultAdminTest.php index 192566883..205f5462e 100644 --- a/tests/php/Security/SecurityDefaultAdminTest.php +++ b/tests/php/Security/SecurityDefaultAdminTest.php @@ -2,10 +2,12 @@ namespace SilverStripe\Security\Tests; +use SilverStripe\Core\Injector\Injector; use SilverStripe\Security\Security; use SilverStripe\Security\Permission; use SilverStripe\Security\Member; use SilverStripe\Dev\SapphireTest; +use SilverStripe\Security\Service\DefaultAdminService; class SecurityDefaultAdminTest extends SapphireTest { @@ -35,6 +37,7 @@ class SecurityDefaultAdminTest extends SapphireTest protected function tearDown() { + Security::clear_default_admin(); Security::setDefaultAdmin($this->defaultUsername, $this->defaultPassword); Permission::reset(); parent::tearDown(); @@ -72,15 +75,19 @@ class SecurityDefaultAdminTest extends SapphireTest public function testFindAnAdministratorWithoutDefaultAdmin() { + $service = Injector::inst()->get(DefaultAdminService::class); // Clear default admin - Security::clear_default_admin(); + DefaultAdminService::clearDefaultAdmin(); $adminMembers = Permission::get_members_by_permission('ADMIN'); $this->assertEquals(0, $adminMembers->count()); - $admin = Security::findAnAdministrator(); + $admin = $service->findOrCreateDefaultAdmin(); - $this->assertInstanceOf(Member::class, $admin); + $this->assertNull($admin); + // When clearing the admin, it will not re-instate it anymore + DefaultAdminService::setDefaultAdmin('admin', 'password'); + $admin = $service->findAnAdministrator(); $this->assertTrue(Permission::checkMember($admin, 'ADMIN')); // User should be blank