From c4194f0ed27a8e6b6b60796beed83e711c670898 Mon Sep 17 00:00:00 2001 From: Simon Erkelens Date: Sun, 30 Apr 2017 15:17:26 +1200 Subject: [PATCH] CMS Login Handling Move to canLogin in the authentication check. Protected isLockedOut Enable login to be called with a different login service (CMSLogin), enabling CMS Log in. Seems the styling and/or output is still broken. logOut could be managed from the Authenticator instead of the member --- _config/security.yml | 4 + src/Security/Authenticator.php | 18 ++-- src/Security/BasicAuth.php | 9 +- src/Security/CMSMemberLoginForm.php | 42 ++++---- src/Security/CMSSecurity.php | 60 ++++++------ src/Security/Member.php | 54 ++--------- .../MemberAuthenticator/Authenticator.php | 78 +++++++++------ .../MemberAuthenticator/CMSAuthenticator.php | 41 ++++++++ .../MemberAuthenticator/CMSLoginHandler.php | 33 ++++--- .../MemberAuthenticator/LoginForm.php | 3 +- .../MemberAuthenticator/LoginHandler.php | 5 +- src/Security/Security.php | 97 ++++++++++++------- .../php/Security/MemberAuthenticatorTest.php | 5 +- tests/php/Security/MemberTest.php | 4 +- 14 files changed, 258 insertions(+), 195 deletions(-) create mode 100644 src/Security/MemberAuthenticator/CMSAuthenticator.php diff --git a/_config/security.yml b/_config/security.yml index 02f960fa0..039e089b1 100644 --- a/_config/security.yml +++ b/_config/security.yml @@ -6,3 +6,7 @@ SilverStripe\Security\MemberAuthenticator\LoginForm: SilverStripe\Security\Security: authenticators: default: SilverStripe\Security\MemberAuthenticator\Authenticator + cms: SilverStripe\Security\MemberAuthenticator\CMSAuthenticator + +SilverStripe\Security\MemberAuthenticator\CMSSecurity: + reauth_enabled: true \ No newline at end of file diff --git a/src/Security/Authenticator.php b/src/Security/Authenticator.php index beee5158c..884893624 100644 --- a/src/Security/Authenticator.php +++ b/src/Security/Authenticator.php @@ -45,15 +45,10 @@ interface Authenticator * URL-handling methods may return an array [ "Form" => (form-object) ] which can then * be merged into a default controller. * - * @param $link The base link to use for this RequestHnadler + * @param string $link The base link to use for this RequestHnadler */ public function getLoginHandler($link); - /** - * @todo - */ - public function getCMSLoginHandler($link); - /** * Return RequestHandler to manage the change-password process. * @@ -63,7 +58,7 @@ interface Authenticator * URL-handling methods may return an array [ "Form" => (form-object) ] which can then * be merged into a default controller. * - * @param $link The base link to use for this RequestHnadler + * @param string $link The base link to use for this RequestHnadler */ public function getChangePasswordHandler($link); @@ -86,4 +81,13 @@ interface Authenticator * @return array */ // public function getAuthenticateFields(); + + /** + * Log the member out of this Authentication method. + * + * @param Member $member by reference, to allow for multiple actions on the member with a single write + * @return boolean|Member if logout was unsuccessfull, return true, otherwise, the member is returned + */ + public function doLogOut(&$member); + } diff --git a/src/Security/BasicAuth.php b/src/Security/BasicAuth.php index cf4c5d8b3..82fbab687 100644 --- a/src/Security/BasicAuth.php +++ b/src/Security/BasicAuth.php @@ -85,6 +85,7 @@ class BasicAuth $member = null; if (isset($_SERVER['PHP_AUTH_USER']) && isset($_SERVER['PHP_AUTH_PW'])) { + /** @var Authenticator $authenticator */ $authenticator = Injector::inst()->get(Authenticator::class); $member = $authenticator->authenticate([ @@ -151,9 +152,9 @@ class BasicAuth */ public static function protect_entire_site($protect = true, $code = 'ADMIN', $message = null) { - Config::inst()->update('SilverStripe\\Security\\BasicAuth', 'entire_site_protected', $protect); - Config::inst()->update('SilverStripe\\Security\\BasicAuth', 'entire_site_protected_code', $code); - Config::inst()->update('SilverStripe\\Security\\BasicAuth', 'entire_site_protected_message', $message); + Config::inst()->update(self::class, 'entire_site_protected', $protect); + Config::inst()->update(self::class, 'entire_site_protected_code', $code); + Config::inst()->update(self::class, 'entire_site_protected_message', $message); } /** @@ -165,7 +166,7 @@ class BasicAuth */ public static function protect_site_if_necessary() { - $config = Config::forClass('SilverStripe\\Security\\BasicAuth'); + $config = Config::forClass(BasicAuth::class); if ($config->entire_site_protected) { self::requireLogin($config->entire_site_protected_message, $config->entire_site_protected_code, false); } diff --git a/src/Security/CMSMemberLoginForm.php b/src/Security/CMSMemberLoginForm.php index 4f1c602a1..e71865b50 100644 --- a/src/Security/CMSMemberLoginForm.php +++ b/src/Security/CMSMemberLoginForm.php @@ -1,38 +1,30 @@ Link($action); - } /** * CMSMemberLoginForm constructor. - * @param Controller $controller + * @param RequestHandler $controller * @param string $authenticatorClass * @param FieldList $name */ - public function __construct(Controller $controller, $authenticatorClass, $name) + public function __construct(RequestHandler $controller, $authenticatorClass, $name) { $this->controller = $controller; @@ -42,7 +34,7 @@ class CMSMemberLoginForm extends LoginForm $actions = $this->getFormActions(); - parent::__construct($controller, $name, $fields, $actions); + parent::__construct($controller, $authenticatorClass, $name, $fields, $actions); } /** @@ -51,7 +43,7 @@ class CMSMemberLoginForm extends LoginForm public function getFormFields() { // Set default fields - $fields = new FieldList( + $fields = FieldList::create([ HiddenField::create("AuthenticationMethod", null, $this->authenticator_class, $this), HiddenField::create('tempid', null, $this->controller->getRequest()->requestVar('tempid')), PasswordField::create("Password", _t('SilverStripe\\Security\\Member.PASSWORD', 'Password')), @@ -63,9 +55,9 @@ class CMSMemberLoginForm extends LoginForm _t('SilverStripe\\Security\\CMSMemberLoginForm.BUTTONFORGOTPASSWORD', "Forgot password?") ) ) - ); + ]); - if (Security::config()->autologin_enabled) { + if (Security::config()->get('autologin_enabled')) { $fields->push(CheckboxField::create( "Remember", _t('SilverStripe\\Security\\Member.REMEMBERME', "Remember me next time?") @@ -88,8 +80,8 @@ class CMSMemberLoginForm extends LoginForm } // Make actions - $actions = new FieldList( - FormAction::create('dologin', _t('SilverStripe\\Security\\CMSMemberLoginForm.BUTTONLOGIN', "Log back in")), + $actions = FieldList::create([ + FormAction::create('doLogin', _t('SilverStripe\\Security\\CMSMemberLoginForm.BUTTONLOGIN', "Log back in")), LiteralField::create( 'doLogout', sprintf( @@ -98,14 +90,20 @@ class CMSMemberLoginForm extends LoginForm _t('SilverStripe\\Security\\CMSMemberLoginForm.BUTTONLOGOUT', "Log out") ) ) - ); + ]); return $actions; } - protected function buildRequestHandler() + /** + * Get link to use for external security actions + * + * @param string $action Action + * @return string + */ + public function getExternalLink($action = null) { - return CMSMemberLoginHandler::create($this); + return Security::singleton()->Link($action); } /** diff --git a/src/Security/CMSSecurity.php b/src/Security/CMSSecurity.php index 38d9cd5f5..1a89f1c9e 100644 --- a/src/Security/CMSSecurity.php +++ b/src/Security/CMSSecurity.php @@ -4,11 +4,13 @@ namespace SilverStripe\Security; use SilverStripe\Admin\AdminRootController; use SilverStripe\Control\HTTPResponse; +use SilverStripe\Control\Session; use SilverStripe\Core\Convert; use SilverStripe\Control\Director; use SilverStripe\Control\Controller; -use SilverStripe\Control\Session; +use SilverStripe\Core\Injector\Injector; use SilverStripe\ORM\FieldType\DBField; +use SilverStripe\Security\MemberAuthenticator\CMSAuthenticator; use SilverStripe\View\Requirements; /** @@ -22,6 +24,7 @@ class CMSSecurity extends Security ); private static $allowed_actions = array( + 'login', 'LoginForm', 'success' ); @@ -41,12 +44,27 @@ class CMSSecurity extends Security Requirements::javascript(FRAMEWORK_ADMIN_DIR . '/client/dist/js/vendor.js'); } + public function login($request, $service = Authenticator::CMS_LOGIN) + { + return parent::login($request, Authenticator::CMS_LOGIN); + } + public function Link($action = null) { /** @skipUpgrade */ return Controller::join_links(Director::baseURL(), "CMSSecurity", $action); } + protected function getAuthenticator($name = 'cms') + { + return parent::getAuthenticator($name); + } + + public static function getAuthenticators($service = Authenticator::CMS_LOGIN) + { + return parent::getAuthenticators($service); + } + /** * Get known logged out member * @@ -57,6 +75,7 @@ class CMSSecurity extends Security if ($tempid = $this->getRequest()->requestVar('tempid')) { return Member::member_from_tempid($tempid); } + return null; } @@ -129,6 +148,7 @@ setTimeout(function(){top.location.href = "$loginURLJS";}, 0); PHP ); $this->setResponse($response); + return $response; } @@ -142,19 +162,6 @@ PHP return parent::preLogin(); } - public function GetLoginForms() - { - $forms = array(); - $authenticators = Authenticator::get_authenticators(); - foreach ($authenticators as $authenticator) { - // Get only CMS-supporting authenticators - if ($authenticator::supports_cms()) { - $forms[] = $authenticator::get_cms_login_form($this); - } - } - return $forms; - } - /** * Determine if CMSSecurity is enabled * @@ -163,28 +170,23 @@ PHP public static function enabled() { // Disable shortcut - if (!static::config()->reauth_enabled) { + if (!static::config()->get('reauth_enabled')) { return false; } - // Count all cms-supported methods - $authenticators = Authenticator::get_authenticators(); - foreach ($authenticators as $authenticator) { + /** @var [] $authenticators */ + $authenticators = Security::config()->get('authenticators'); + foreach ($authenticators as $name => $authenticator) { // Supported if at least one authenticator is supported - if ($authenticator::supports_cms()) { + $authenticator = Injector::inst()->get($authenticator); + if (($authenticator->supportedServices() & Authenticator::CMS_LOGIN) + && Security::hasAuthenticator($name) + ) { return true; } } - return false; - } - public function LoginForm() - { - $authenticator = $this->getAuthenticator('default'); - if ($authenticator && $authenticator::supports_cms()) { - return $authenticator::get_cms_login_form($this); - } - user_error('Passed invalid authentication method', E_USER_ERROR); + return false; } /** @@ -217,7 +219,7 @@ PHP $controller = $controller->customise(array( 'Content' => _t( 'SilverStripe\\Security\\CMSSecurity.SUCCESSCONTENT', - '

Login success. If you are not automatically redirected '. + '

Login success. If you are not automatically redirected ' . 'click here

', 'Login message displayed in the cms popup once a user has re-authenticated themselves', array('link' => Convert::raw2att($backURL)) diff --git a/src/Security/Member.php b/src/Security/Member.php index 6dba32a06..fa26fdabe 100644 --- a/src/Security/Member.php +++ b/src/Security/Member.php @@ -368,7 +368,7 @@ class Member extends DataObject implements TemplateGlobalProvider 'Your account has been temporarily disabled because of too many failed attempts at ' . 'logging in. Please try again in {count} minutes.', null, - array('count' => $this->config()->lock_out_delay_mins) + array('count' => static::config()->get('lock_out_delay_mins')) ) ); } @@ -382,7 +382,7 @@ class Member extends DataObject implements TemplateGlobalProvider * * @return bool */ - public function isLockedOut() + protected function isLockedOut() { if (!$this->LockedOutUntil) { return false; @@ -499,7 +499,7 @@ class Member extends DataObject implements TemplateGlobalProvider $this->write(); // Audit logging hook - $this->extend('memberLoggedIn'); + $this->extend('afterMemberLoggedIn'); } /** @@ -626,40 +626,6 @@ class Member extends DataObject implements TemplateGlobalProvider } } - /** - * Logs this member out. - */ - public function logOut() - { - $this->extend('beforeMemberLoggedOut'); - - Session::clear("loggedInAs"); - if (Member::config()->login_marker_cookie) { - Cookie::set(Member::config()->login_marker_cookie, null, 0); - } - - Session::destroy(); - - $this->extend('memberLoggedOut'); - - // Clears any potential previous hashes for this member - RememberLoginHash::clear($this, Cookie::get('alc_device')); - - Cookie::set('alc_enc', null); // // Clear the Remember Me cookie - Cookie::force_expiry('alc_enc'); - Cookie::set('alc_device', null); - Cookie::force_expiry('alc_device'); - - // Switch back to live in order to avoid infinite loops when - // redirecting to the login screen (if this login screen is versioned) - Session::clear('readingMode'); - - $this->write(); - - // Audit logging hook - $this->extend('memberLoggedOut'); - } - /** * Utility for generating secure password hashes for this member. * @@ -762,7 +728,7 @@ class Member extends DataObject implements TemplateGlobalProvider ->filter('TempIDHash', $tempid); // Exclude expired - if (static::config()->temp_id_lifetime) { + if (static::config()->get('temp_id_lifetime')) { $members = $members->filter('TempIDExpired:GreaterThan', DBDatetime::now()->getValue()); } @@ -788,7 +754,7 @@ class Member extends DataObject implements TemplateGlobalProvider i18n::getSources()->getKnownLocales() )); - $fields->removeByName(static::config()->hidden_fields); + $fields->removeByName(static::config()->get('hidden_fields')); $fields->removeByName('FailedLoginCount'); @@ -988,7 +954,7 @@ class Member extends DataObject implements TemplateGlobalProvider if ((Director::isLive() || Injector::inst()->get(Mailer::class) instanceof TestMailer) && $this->isChanged('Password') && $this->record['Password'] - && $this->config()->notify_password_change + && static::config()->get('notify_password_change') ) { Email::create() ->setHTMLTemplate('SilverStripe\\Control\\Email\\ChangePasswordEmail') @@ -1219,7 +1185,7 @@ class Member extends DataObject implements TemplateGlobalProvider */ public function getTitle() { - $format = $this->config()->title_format; + $format = static::config()->get('title_format'); if ($format) { $values = array(); foreach ($format['columns'] as $col) { @@ -1254,7 +1220,7 @@ class Member extends DataObject implements TemplateGlobalProvider $op = (DB::get_conn() instanceof MSSQLDatabase) ? " + " : " || "; // Get title_format with fallback to default - $format = static::config()->title_format; + $format = static::config()->get('title_format'); if (!$format) { $format = [ 'columns' => ['Surname', 'FirstName'], @@ -1542,9 +1508,9 @@ class Member extends DataObject implements TemplateGlobalProvider _t(__CLASS__.'.INTERFACELANG', "Interface Language", 'Language of the CMS'), i18n::getSources()->getKnownLocales() )); - $mainFields->removeByName($this->config()->hidden_fields); + $mainFields->removeByName(static::config()->get('hidden_fields')); - if (! $this->config()->lock_out_after_incorrect_logins) { + if (! static::config()->get('lock_out_after_incorrect_logins')) { $mainFields->removeByName('FailedLoginCount'); } diff --git a/src/Security/MemberAuthenticator/Authenticator.php b/src/Security/MemberAuthenticator/Authenticator.php index c86cabe04..4e9c836e1 100644 --- a/src/Security/MemberAuthenticator/Authenticator.php +++ b/src/Security/MemberAuthenticator/Authenticator.php @@ -3,11 +3,12 @@ namespace SilverStripe\Security\MemberAuthenticator; use SilverStripe\Control\Controller; +use SilverStripe\Control\Cookie; use SilverStripe\Control\Session; -use SilverStripe\Forms\Form; use SilverStripe\ORM\ValidationResult; use InvalidArgumentException; use SilverStripe\Security\Authenticator as BaseAuthenticator; +use SilverStripe\Security\RememberLoginHash; use SilverStripe\Security\Security; use SilverStripe\Security\Member; use SilverStripe\Security\LoginAttempt; @@ -24,7 +25,7 @@ class Authenticator implements BaseAuthenticator { // Bitwise-OR of all the supported services, to make a bitmask return BaseAuthenticator::LOGIN | BaseAuthenticator::LOGOUT | BaseAuthenticator::CHANGE_PASSWORD - | BaseAuthenticator::RESET_PASSWORD | BaseAuthenticator::CMS_LOGIN; + | BaseAuthenticator::RESET_PASSWORD; } /** @@ -50,39 +51,24 @@ class Authenticator implements BaseAuthenticator /** * Attempt to find and authenticate member if possible from the given data * - * @param array $data - * @param Form $form + * @param array $data Form submitted data + * @param $message * @param bool &$success Success flag + * @param null|Member $member If the parent method already identified the member, it can be passed in * @return Member Found member, regardless of successful login */ - protected function authenticateMember($data, &$message, &$success) + protected function authenticateMember($data, &$message, &$success, $member = null) { // Default success to false $success = false; - - // Attempt to identify by temporary ID - $member = null; - $email = null; - 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) { - $email = $member->Email; - } - } - - // Otherwise, get email from posted value instead - /** @skipUpgrade */ - if (!$member && !empty($data['Email'])) { - $email = $data['Email']; - } - + $email = !empty($data['Email']) ? $data['Email'] : null ; + // Check default login (see Security::setDefaultAdmin()) $asDefaultAdmin = $email === Security::default_admin_username(); if ($asDefaultAdmin) { // If logging is as default admin, ensure record is setup correctly $member = Member::default_admin(); - $success = !$member->isLockedOut() && Security::check_default_admin($email, $data['Password']); + $success = $member->canLogin()->isValid() && Security::check_default_admin($email, $data['Password']); //protect against failed login if ($success) { return $member; @@ -92,8 +78,9 @@ class Authenticator implements BaseAuthenticator // Attempt to identify user by email if (!$member && $email) { // Find user by email + /** @var Member $member */ $member = Member::get() - ->filter(Member::config()->unique_identifier_field, $email) + ->filter([Member::config()->get('unique_identifier_field') => $email]) ->first(); } @@ -120,7 +107,7 @@ class Authenticator implements BaseAuthenticator $result->getMessages() )); } else { - if ($member) { + if ($member) { // How can success be true and member false? $member->registerSuccessfulLogin(); } } @@ -137,7 +124,7 @@ class Authenticator implements BaseAuthenticator */ protected function recordLoginAttempt($data, $member, $success) { - if (!Security::config()->login_recording) { + if (!Security::config()->get('login_recording')) { return; } @@ -148,7 +135,7 @@ class Authenticator implements BaseAuthenticator throw new InvalidArgumentException("Bad email passed to MemberAuthenticator::authenticate(): $email"); } - $attempt = new LoginAttempt(); + $attempt = LoginAttempt::create(); if ($success) { // successful login (member is existing with matching password) $attempt->MemberID = $member->ID; @@ -198,8 +185,39 @@ class Authenticator implements BaseAuthenticator return LoginHandler::create($link, $this); } - public function getCMSLoginHandler($link) + /** + * + * @param Member $member + * @return bool|Member + */ + public function doLogOut(&$member) { - return CMSMemberLoginHandler::create($controller, self::class, "LoginForm"); + if($member instanceof Member) { + Session::clear("loggedInAs"); + if (Member::config()->login_marker_cookie) { + Cookie::set(Member::config()->login_marker_cookie, null, 0); + } + + Session::destroy(); + + // Clears any potential previous hashes for this member + RememberLoginHash::clear($member, Cookie::get('alc_device')); + + Cookie::set('alc_enc', null); // // Clear the Remember Me cookie + Cookie::force_expiry('alc_enc'); + Cookie::set('alc_device', null); + Cookie::force_expiry('alc_device'); + + // Switch back to live in order to avoid infinite loops when + // redirecting to the login screen (if this login screen is versioned) + Session::clear('readingMode'); + + // Log out unsuccessful. Useful for 3rd-party logins that return failure. Shouldn't happen + // on the default authenticator though. + if(Member::currentUserID()) { + return Member::currentUser(); + } + } + return true; } } diff --git a/src/Security/MemberAuthenticator/CMSAuthenticator.php b/src/Security/MemberAuthenticator/CMSAuthenticator.php new file mode 100644 index 000000000..43946398f --- /dev/null +++ b/src/Security/MemberAuthenticator/CMSAuthenticator.php @@ -0,0 +1,41 @@ +Email; + } + } + + return parent::authenticateMember($data, $message, $success, $member); + } + + public function getLoginHandler($link) + { + return CMSLoginHandler::create($link, $this); + } + +} \ No newline at end of file diff --git a/src/Security/MemberAuthenticator/CMSLoginHandler.php b/src/Security/MemberAuthenticator/CMSLoginHandler.php index ca469e23e..7cf751d52 100644 --- a/src/Security/MemberAuthenticator/CMSLoginHandler.php +++ b/src/Security/MemberAuthenticator/CMSLoginHandler.php @@ -4,24 +4,26 @@ namespace SilverStripe\Security\MemberAuthenticator; use SilverStripe\Control\HTTPResponse; use SilverStripe\Core\Convert; +use SilverStripe\Security\CMSSecurity; +use SilverStripe\Security\Member; +use SilverStripe\Security\Security; class CMSLoginHandler extends LoginHandler { - /** - * Login form handler method - * - * This method is called when the user clicks on "Log in" - * - * @param array $data Submitted data - * @return HTTPResponse - */ - public function dologin($data, $formHandler) - { - if ($this->performLogin($data)) { - return $this->logInUserAndRedirect($data); - } + private static $allowed_actions = [ + 'LoginForm' + ]; - return $this->redirectBackToForm(); + /** + * Return the CMSMemberLoginForm form + */ + public function loginForm() + { + return CMSMemberLoginForm::create( + $this, + get_class($this->authenticator), + 'LoginForm' + ); } public function redirectBackToForm() @@ -75,10 +77,9 @@ PHP /** * Send user to the right location after login * - * @param array $data * @return HTTPResponse */ - protected function logInUserAndRedirect($data, $formHandler) + protected function redirectAfterSuccessfulLogin() { // Check password expiry if (Member::currentUser()->isPasswordExpired()) { diff --git a/src/Security/MemberAuthenticator/LoginForm.php b/src/Security/MemberAuthenticator/LoginForm.php index ff1a69f77..e59bc9d98 100644 --- a/src/Security/MemberAuthenticator/LoginForm.php +++ b/src/Security/MemberAuthenticator/LoginForm.php @@ -3,6 +3,7 @@ namespace SilverStripe\Security\MemberAuthenticator; use SilverStripe\Control\Director; +use SilverStripe\Control\RequestHandler; use SilverStripe\Control\Session; use SilverStripe\Control\Controller; use SilverStripe\Forms\HiddenField; @@ -49,7 +50,7 @@ class LoginForm extends BaseLoginForm * Constructor * * @skipUpgrade - * @param Controller $controller The parent controller, necessary to + * @param RequestHandler $controller The parent controller, necessary to * create the appropriate form action tag. * @param string $authenticatorClass Authenticator for this LoginForm * @param string $name The method on the controller that will return this diff --git a/src/Security/MemberAuthenticator/LoginHandler.php b/src/Security/MemberAuthenticator/LoginHandler.php index 59e55e43c..c09b61fd6 100644 --- a/src/Security/MemberAuthenticator/LoginHandler.php +++ b/src/Security/MemberAuthenticator/LoginHandler.php @@ -7,7 +7,6 @@ use SilverStripe\Control\HTTPResponse; use SilverStripe\Control\Session; use SilverStripe\Control\RequestHandler; use SilverStripe\ORM\ValidationResult; -use SilverStripe\Security\MemberAuthenticator\Authenticator; use SilverStripe\Security\Security; use SilverStripe\Security\Member; @@ -91,7 +90,7 @@ class LoginHandler extends RequestHandler * This method is called when the user clicks on "Log in" * * @param array $data Submitted data - * @param LoginHandler $formHandler + * @param LoginForm $form * @return HTTPResponse */ public function doLogin($data, $form) @@ -223,7 +222,7 @@ class LoginHandler extends RequestHandler */ public function performLogin($member, $data) { - $member->LogIn(isset($data['Remember'])); + $member->logIn(isset($data['Remember'])); return $member; } /** diff --git a/src/Security/Security.php b/src/Security/Security.php index dff7e5e42..068cf6ed0 100644 --- a/src/Security/Security.php +++ b/src/Security/Security.php @@ -238,13 +238,13 @@ class Security extends Controller implements TemplateGlobalProvider parent::init(); // Prevent clickjacking, see https://developer.mozilla.org/en-US/docs/HTTP/X-Frame-Options - $frameOptions = $this->config()->get('frame_options'); + $frameOptions = static::config()->get('frame_options'); if ($frameOptions) { $this->getResponse()->addHeader('X-Frame-Options', $frameOptions); } // Prevent search engines from indexing the login page - $robotsTag = $this->config()->get('robots_tag'); + $robotsTag = static::config()->get('robots_tag'); if ($robotsTag) { $this->getResponse()->addHeader('X-Robots-Tag', $robotsTag); } @@ -267,9 +267,9 @@ class Security extends Controller implements TemplateGlobalProvider */ protected function getAuthenticator($name) { - $authenticators = self::config()->authenticators; + $authenticators = self::config()->get('authenticators'); - if (!$name) $name = 'default'; + $name = $name ?: 'default'; if (isset($authenticators[$name])) { return Injector::inst()->get($authenticators[$name]); @@ -283,13 +283,20 @@ class Security extends Controller implements TemplateGlobalProvider * * @return array Return an array of Authenticator objects */ - public static function getAuthenticators() + public static function getAuthenticators($service = Authenticator::LOGIN) { - $authenticators = self::config()->authenticators; + $authenticators = self::config()->get('authenticators'); - return array_map(function ($class) { - return Injector::inst()->get($class); - }, $authenticators); + foreach($authenticators as $name => &$class) { + /** @var Authenticator $authenticator */ + $authenticator = Injector::inst()->get($class); + if($authenticator->supportedServices() & $service) { + $class = $authenticator; + } else { + unset($authenticators[$name]); + } + } + return $authenticators; } /** @@ -421,22 +428,6 @@ class Security extends Controller implements TemplateGlobalProvider )); } - /** - * Get the login form to process according to the submitted data - * - * @return Form - * @throws Exception - */ - public function LoginForm() - { - $authenticator = $this->getAuthenticator('default'); - if ($authenticator) { - $handler = $authenticator->getLoginHandler($this->Link()); - return $handler->handleRequest($this->request, DataModel::inst()); - } - throw new Exception('Passed invalid authentication method'); - } - /** * Get the login forms for all available authentication methods * @@ -480,6 +471,12 @@ class Security extends Controller implements TemplateGlobalProvider /** * Log the currently logged in user out * + * Logging out without ID-parameter in the URL, will log the user out of all applicable Authenticators. + * + * Adding an ID will only log the user out of that Authentication method. + * + * Logging out of Default will always completely log out the user. + * * @param bool $redirect Redirect the user back to where they came. * - If it's false, the code calling logout() is * responsible for sending the user where-ever @@ -488,14 +485,43 @@ class Security extends Controller implements TemplateGlobalProvider */ public function logout($redirect = true) { + $this->extend('beforeMemberLoggedOut'); + $request = $this->getRequest(); $member = Member::currentUser(); - if ($member) { - $member->logOut(); + // Reasoning for (now) to not go with a full LoginHandler call, is to not make it circular + // re-sending the request forward to the authenticator. In the case of logout, I think it would be + // overkill. + if (($name = $request->param('ID')) && self::hasAuthenticator($request->param('ID'))){ + /** @var Authenticator $authenticator */ + $authenticator = $this->getAuthenticator($request->param('ID')); + if($authenticator->doLogOut($member) !== true) { + $this->extend('failureMemberLoggedOut', $authenticator); + return $this->redirectBack(); + } + $this->extend('successMemberLoggedOut', $authenticator); + } else { + $authenticators = static::getAuthenticators(Authenticator::LOGOUT); + /** + * @var string $name + * @var Authenticator $authenticator + */ + foreach ($authenticators as $name => $authenticator) { + if ($authenticator->logOut($member) !== true) { + $this->extend('failureMemberLoggedOut', $authenticator); + // Break on first log out failure(?) + return $this->redirectBack(); + } + $this->extend('successMemberLoggedOut', $authenticator); + } } + // Member is successfully logged out. Write possible changes to the database. + $member->write(); + $this->extend('afterMemberLoggedOut'); if ($redirect && (!$this->getResponse()->isFinished())) { return $this->redirectBack(); } + return null; } @@ -644,7 +670,7 @@ class Security extends Controller implements TemplateGlobalProvider * @return HTTPResponse|string Returns the "login" page as HTML code. * @throws HTTPResponse_Exception */ - public function login($request) + public function login($request, $service = Authenticator::LOGIN) { // Check pre-login process if ($response = $this->preLogin()) { @@ -654,19 +680,20 @@ class Security extends Controller implements TemplateGlobalProvider $link = $this->link("login"); // Delegate to a single handler - Security/login//... - if ($name = $request->param('ID')) { + if (($name = $request->param('ID')) && self::hasAuthenticator($request->param('ID'))) { $request->shift(); $authenticator = $this->getAuthenticator($name); - if (!$authenticator) { - throw new HTTPResponse_Exception(404, 'No authenticator "' . $name . '"'); + // @todo handle different Authenticator situations + if (!$authenticator->supportedServices() & $service) { + throw new HTTPResponse_Exception('Invalid Authenticator "' . $name . '" for login action', 418); } $authenticators = [ $name => $authenticator ]; // Delegate to all of them, building a tabbed view - Security/login } else { - $authenticators = $this->getAuthenticators(); + $authenticators = static::getAuthenticators($service); } $handlers = $authenticators; @@ -746,7 +773,7 @@ class Security extends Controller implements TemplateGlobalProvider */ protected function delegateToHandler(RequestHandler $handler, $title, array $templates) { - $result = $handler->handleRequest($this->getRequest(), \SilverStripe\ORM\DataModel::inst()); + $result = $handler->handleRequest($this->getRequest(), DataModel::inst()); // Return the customised controller - used to render in a Form // Post requests are expected to be login posts, so they'll be handled downstairs @@ -927,7 +954,7 @@ class Security extends Controller implements TemplateGlobalProvider * Factory method for the lost password form * * @skipUpgrade - * @return ChangePasswordForm Returns the lost password form + * @return MemberAuthenticator\ChangePasswordForm */ public function ChangePasswordForm() { @@ -1076,7 +1103,7 @@ class Security extends Controller implements TemplateGlobalProvider */ public static function has_default_admin() { - return !empty(self::$default_username) && !empty(self::$default_password); + return !empty(self::$default_username) && !empty(self::$default_password) && (Director::get_environment_type() === 'dev'); } /** diff --git a/tests/php/Security/MemberAuthenticatorTest.php b/tests/php/Security/MemberAuthenticatorTest.php index af9bbba1d..75af65aa0 100644 --- a/tests/php/Security/MemberAuthenticatorTest.php +++ b/tests/php/Security/MemberAuthenticatorTest.php @@ -5,6 +5,7 @@ namespace SilverStripe\Security\Tests; use SilverStripe\ORM\DataObject; use SilverStripe\ORM\FieldType\DBDatetime; use SilverStripe\ORM\ValidationResult; +use SilverStripe\Security\MemberAuthenticator\CMSAuthenticator; use SilverStripe\Security\PasswordEncryptor; use SilverStripe\Security\PasswordEncryptor_PHPHash; use SilverStripe\Security\Security; @@ -89,7 +90,7 @@ class MemberAuthenticatorTest extends SapphireTest */ public function testAuthenticateByTempID() { - $authenticator = new Authenticator(); + $authenticator = new CMSAuthenticator(); $member = new Member(); $member->Email = 'test1@test.com'; @@ -186,7 +187,7 @@ class MemberAuthenticatorTest extends SapphireTest $dummy ); - $this->assertTrue(Member::default_admin()->isLockedOut()); + $this->assertFalse(Member::default_admin()->canLogin()->isValid()); $this->assertEquals('2016-04-18 00:10:00', Member::default_admin()->LockedOutUntil); } } diff --git a/tests/php/Security/MemberTest.php b/tests/php/Security/MemberTest.php index 4b33939cf..f3f7348f7 100644 --- a/tests/php/Security/MemberTest.php +++ b/tests/php/Security/MemberTest.php @@ -1155,8 +1155,8 @@ class MemberTest extends FunctionalTest 'Failed to increment $member->FailedLoginCount' ); - $this->assertFalse( - $member->isLockedOut(), + $this->assertTrue( + $member->canLogin()->isValid(), "Member has been locked out too early" ); }