diff --git a/lang/en.yml b/lang/en.yml index ce663fad6..a1f0dfa3e 100644 --- a/lang/en.yml +++ b/lang/en.yml @@ -259,6 +259,7 @@ en: ADDGROUP: 'Add group' BUTTONCHANGEPASSWORD: 'Change Password' BUTTONLOGIN: 'Log in' + BUTTONLOGOUT: 'Log out' BUTTONLOGINOTHER: 'Log in as someone else' BUTTONLOSTPASSWORD: 'I''ve lost my password' CONFIRMNEWPASSWORD: 'Confirm New Password' @@ -354,9 +355,11 @@ en: BUTTONSEND: 'Send me the password reset link' CHANGEPASSWORDBELOW: 'You can change your password below.' CHANGEPASSWORDHEADER: 'Change your password' + CONFIRMLOGOUT: 'Please click the button below to confirm that you wish to log out.' ENTERNEWPASSWORD: 'Please enter a new password.' ERRORPASSWORDPERMISSION: 'You must be logged in in order to change your password!' LOGIN: 'Log in' + LOGOUT: 'Log out' LOSTPASSWORDHEADER: 'Lost Password' NOTEPAGESECURED: 'That page is secured. Enter your credentials below and we will send you right along.' NOTERESETLINKINVALID: '

The password reset link is invalid or expired.

You can request a new one here or change your password after you logged in.

' diff --git a/src/Security/CMSSecurity.php b/src/Security/CMSSecurity.php index 16cdcb6cf..9ed9c0b95 100644 --- a/src/Security/CMSSecurity.php +++ b/src/Security/CMSSecurity.php @@ -89,9 +89,9 @@ class CMSSecurity extends Security return $this; } - protected function getLoginMessage(&$messageType = null) + protected function getSessionMessage(&$messageType = null) { - $message = parent::getLoginMessage($messageType); + $message = parent::getSessionMessage($messageType); if ($message) { return $message; } diff --git a/src/Security/LogoutForm.php b/src/Security/LogoutForm.php new file mode 100644 index 000000000..e8851c51e --- /dev/null +++ b/src/Security/LogoutForm.php @@ -0,0 +1,81 @@ +setController($controller); + + if (!$fields) { + $fields = $this->getFormFields(); + } + if (!$actions) { + $actions = $this->getFormActions(); + } + + parent::__construct($controller, $name, $fields, $actions); + + $this->setFormAction(Security::logout_url()); + } + + /** + * Build the FieldList for the logout form + * + * @return FieldList + */ + protected function getFormFields() + { + $fields = FieldList::create(); + + $controller = $this->getController(); + $backURL = $controller->getBackURL() + ?: $controller->getReturnReferer(); + + // Protect against infinite redirection back to the logout URL after logging out + if (!$backURL || Director::makeRelative($backURL) === $controller->getRequest()->getURL()) { + $backURL = Director::baseURL(); + } + + $fields->push(HiddenField::create('BackURL', 'BackURL', $backURL)); + + return $fields; + } + + /** + * Build default logout form action FieldList + * + * @return FieldList + */ + protected function getFormActions() + { + $actions = FieldList::create( + FormAction::create('doLogout', _t('SilverStripe\\Security\\Member.BUTTONLOGOUT', "Log out")) + ); + + return $actions; + } +} diff --git a/src/Security/MemberAuthenticator/LoginHandler.php b/src/Security/MemberAuthenticator/LoginHandler.php index 31a2b00c4..8bc108cfb 100644 --- a/src/Security/MemberAuthenticator/LoginHandler.php +++ b/src/Security/MemberAuthenticator/LoginHandler.php @@ -195,7 +195,7 @@ class LoginHandler extends RequestHandler 'Welcome Back, {firstname}', ['firstname' => $member->FirstName] ); - Security::singleton()->setLoginMessage($message, ValidationResult::TYPE_GOOD); + Security::singleton()->setSessionMessage($message, ValidationResult::TYPE_GOOD); } // Redirect back diff --git a/src/Security/MemberAuthenticator/LogoutHandler.php b/src/Security/MemberAuthenticator/LogoutHandler.php index 21b2a4381..22909aeaf 100644 --- a/src/Security/MemberAuthenticator/LogoutHandler.php +++ b/src/Security/MemberAuthenticator/LogoutHandler.php @@ -2,11 +2,15 @@ namespace SilverStripe\Security\MemberAuthenticator; +use SilverStripe\Control\Director; use SilverStripe\Control\RequestHandler; use SilverStripe\Core\Injector\Injector; +use SilverStripe\ORM\ValidationResult; use SilverStripe\Security\IdentityStore; +use SilverStripe\Security\LogoutForm; use SilverStripe\Security\Member; use SilverStripe\Security\Security; +use SilverStripe\Security\SecurityToken; /** * Class LogoutHandler handles logging out Members from their session and/or cookie. @@ -27,7 +31,8 @@ class LogoutHandler extends RequestHandler * @var array */ private static $allowed_actions = [ - 'logout' + 'logout', + 'LogoutForm' ]; @@ -45,20 +50,64 @@ class LogoutHandler extends RequestHandler { $member = Security::getCurrentUser(); + // If the user doesn't have a security token, show them a form where they can get one. + // This protects against nuisance CSRF attacks to log out users. + if ($member && !SecurityToken::inst()->checkRequest($this->getRequest())) { + Security::singleton()->setSessionMessage( + _t( + 'SilverStripe\\Security\\Security.CONFIRMLOGOUT', + "Please click the button below to confirm that you wish to log out." + ), + ValidationResult::TYPE_WARNING + ); + + return [ + 'Form' => $this->logoutForm() + ]; + } + return $this->doLogOut($member); } /** - * + * @return LogoutForm + */ + public function logoutForm() + { + return LogoutForm::create($this); + } + + /** * @param Member $member - * @return bool|Member Return a member if something goes wrong + * @return HTTPResponse */ public function doLogOut($member) { + $this->extend('beforeLogout'); + if ($member instanceof Member) { Injector::inst()->get(IdentityStore::class)->logOut($this->getRequest()); } - return true; + if (Security::getCurrentUser()) { + $this->extend('failedLogout'); + } else { + $this->extend('afterLogout'); + } + + return $this->redirectAfterLogout(); + } + + /** + * @return HTTPResponse + */ + protected function redirectAfterLogout() + { + $backURL = $this->getBackURL(); + if ($backURL) { + return $this->redirect($backURL); + } + + return $this->redirect(Director::absoluteBaseURL()); } } diff --git a/src/Security/Security.php b/src/Security/Security.php index 3d1bff936..ff008dd30 100644 --- a/src/Security/Security.php +++ b/src/Security/Security.php @@ -4,6 +4,7 @@ namespace SilverStripe\Security; use LogicException; use Page; +use ReflectionClass; use SilverStripe\CMS\Controllers\ModelAsController; use SilverStripe\Control\Controller; use SilverStripe\Control\Director; @@ -391,7 +392,7 @@ class Security extends Controller implements TemplateGlobalProvider $message = $messageSet['default']; } - static::singleton()->setLoginMessage($message, ValidationResult::TYPE_WARNING); + static::singleton()->setSessionMessage($message, ValidationResult::TYPE_WARNING); $loginResponse = static::singleton()->login(); if ($loginResponse instanceof HTTPResponse) { return $loginResponse; @@ -406,7 +407,7 @@ class Security extends Controller implements TemplateGlobalProvider $message = $messageSet['default']; } - static::singleton()->setLoginMessage($message, ValidationResult::TYPE_WARNING); + static::singleton()->setSessionMessage($message, ValidationResult::TYPE_WARNING); Session::set("BackURL", $_SERVER['REQUEST_URI']); @@ -481,54 +482,6 @@ class Security extends Controller implements TemplateGlobalProvider return 1; } - /** - * 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 - * they should go. - * @return HTTPResponse|null - */ - public function logout($redirect = true) - { - $this->extend('beforeMemberLoggedOut'); - $member = static::getCurrentUser(); - - if ($member) { // If we don't have a member, there's not much to log out. - /** @var array|Authenticator[] $authenticators */ - $authenticators = $this->getApplicableAuthenticators(Authenticator::LOGOUT); - - /** @var Authenticator[] $authenticator */ - foreach ($authenticators as $name => $authenticator) { - $handler = $authenticator->getLogOutHandler(Controller::join_links($this->Link(), 'logout')); - $this->delegateToHandler($handler, $name); - } - // In the rare case, but plausible with e.g. an external IdentityStore, the user is not logged out. - if (static::getCurrentUser() !== null) { - $this->extend('failureMemberLoggedOut', $authenticator); - - 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; - } - /** * Perform pre-login checking and prepare a response if available prior to login * @@ -558,7 +511,7 @@ class Security extends Controller implements TemplateGlobalProvider // This step is necessary in cases such as automatic redirection where a user is authenticated // upon landing on an SSL secured site and is automatically logged in, or some other case // where the user has permissions to continue but is not given the option. - if (!$this->getLoginMessage() + if (!$this->getSessionMessage() && ($member = static::getCurrentUser()) && $member->exists() && $this->getRequest()->requestVar('BackURL') @@ -605,18 +558,18 @@ class Security extends Controller implements TemplateGlobalProvider * @param array|Form[] $forms * @return string */ - protected function generateLoginFormSet($forms) + protected function generateTabbedFormSet($forms) { if (count($forms) === 1) { return $forms; } - $viewData = new ArrayData(array( + $viewData = new ArrayData([ 'Forms' => new ArrayList($forms), - )); + ]); return $viewData->renderWith( - $this->getTemplatesFor('MultiAuthenticatorLogin') + $this->getTemplatesFor('MultiAuthenticatorTabbedForms') ); } @@ -626,7 +579,7 @@ class Security extends Controller implements TemplateGlobalProvider * @param string &$messageType Type of message, if available, passed back to caller * @return string Message in HTML format */ - protected function getLoginMessage(&$messageType = null) + protected function getSessionMessage(&$messageType = null) { $message = Session::get('Security.Message.message'); $messageType = null; @@ -650,7 +603,7 @@ class Security extends Controller implements TemplateGlobalProvider * @param string $messageType Message type. One of ValidationResult::TYPE_* * @param string $messageCast Message cast. One of ValidationResult::CAST_* */ - public function setLoginMessage( + public function setSessionMessage( $message, $messageType = ValidationResult::TYPE_WARNING, $messageCast = ValidationResult::CAST_TEXT @@ -663,7 +616,7 @@ class Security extends Controller implements TemplateGlobalProvider /** * Clear login message */ - public static function clearLoginMessage() + public static function clearSessionMessage() { Session::clear('Security.Message'); } @@ -678,7 +631,6 @@ class Security extends Controller implements TemplateGlobalProvider * @param null|HTTPRequest $request * @param int $service * @return HTTPResponse|string Returns the "login" page as HTML code. - * @throws HTTPResponse_Exception */ public function login($request = null, $service = Authenticator::LOGIN) { @@ -692,30 +644,9 @@ class Security extends Controller implements TemplateGlobalProvider $request = $this->getRequest(); } - if ($request && $request->param('ID')) { - $authName = $request->param('ID'); - } + $handlers = $this->getServiceAuthenticatorsFromRequest($service, $request); $link = $this->Link('login'); - - // Delegate to a single handler - Security/login//... - if ($authName && $this->hasAuthenticator($authName)) { - if ($request) { - $request->shift(); - } - - $authenticator = $this->getAuthenticator($authName); - - if (!$authenticator->supportedServices() & $service) { - throw new HTTPResponse_Exception('Invalid Authenticator "' . $authName . '" for login action', 418); - } - - $handlers = [$authName => $authenticator]; - } else { - // Delegate to all of them, building a tabbed view - Security/login - $handlers = $this->getApplicableAuthenticators($service); - } - array_walk( $handlers, function (Authenticator &$auth, $name) use ($link) { @@ -726,22 +657,137 @@ class Security extends Controller implements TemplateGlobalProvider return $this->delegateToMultipleHandlers( $handlers, _t('Security.LOGIN', 'Log in'), - $this->getTemplatesFor('login') + $this->getTemplatesFor('login'), + [$this, 'aggregateTabbedForms'] ); } /** - * Delegate to an number of handlers, extracting their forms and rendering a tabbed form-set. - * This is used to built the log-in page where there are multiple authenticators active. + * 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. + * + * @param null|HTTPRequest $request + * @param int $service + * @return HTTPResponse|string + */ + public function logout($request = null, $service = Authenticator::LOGOUT) + { + $authName = null; + + if (!$request) { + $request = $this->getRequest(); + } + + $handlers = $this->getServiceAuthenticatorsFromRequest($service, $request); + + $link = $this->Link('logout'); + array_walk( + $handlers, + function (Authenticator &$auth, $name) use ($link) { + $auth = $auth->getLogoutHandler(Controller::join_links($link, $name)); + } + ); + + return $this->delegateToMultipleHandlers( + $handlers, + _t('Security.LOGOUT', 'Log out'), + $this->getTemplatesFor('logout'), + [$this, 'aggregateTabbedForms'] + ); + } + + /** + * Get authenticators for the given service, optionally filtered by the ID parameter + * of the current request + * + * @param int $service + * @param HTTPRequest $request + * @throws HTTPResponse_Exception + */ + protected function getServiceAuthenticatorsFromRequest($service, HTTPRequest $request) + { + $authName = null; + + if ($request->param('ID')) { + $authName = $request->param('ID'); + } + + // Delegate to a single named handler - e.g. Security/login// + if ($authName && $this->hasAuthenticator($authName)) { + if ($request) { + $request->shift(); + } + + $authenticator = $this->getAuthenticator($authName); + + if (!$authenticator->supportedServices() & $service) { + // Try to be helpful and show the service constant name, e.g. Authenticator::LOGIN + $constants = array_flip((new ReflectionClass(Authenticator::class))->getConstants()); + + $message = 'Invalid Authenticator "' . $authName . '" for '; + if (array_key_exists($service, $constants)) { + $message .= 'service: Authenticator::' . $constants[$service]; + } else { + $message .= 'unknown authenticator service'; + } + + throw new HTTPResponse_Exception($message, 400); + } + + $handlers = [$authName => $authenticator]; + } else { + // Delegate to all of them, building a tabbed view - e.g. Security/login/ + $handlers = $this->getApplicableAuthenticators($service); + } + + return $handlers; + } + + /** + * Aggregate tabbed forms from each handler to fragments ready to be rendered. + * + * @param array $results + * @return array + */ + protected function aggregateTabbedForms(array $results) + { + $forms = []; + foreach ($results as $authName => $singleResult) { + // The result *must* be an array with a Form key + if (!is_array($singleResult) || !isset($singleResult['Form'])) { + user_error('Authenticator "' . $authName . '" doesn\'t support tabbed forms', E_USER_WARNING); + continue; + } + + $forms[] = $singleResult['Form']; + } + + if (!$forms) { + throw new \LogicException('No authenticators found compatible with tabbed forms'); + } + + return [ + 'Forms' => ArrayList::create($forms), + 'Form' => $this->generateTabbedFormSet($forms) + ]; + } + + /** + * Delegate to a number of handlers and aggregate the results. This is used, for example, to + * build the log-in page where there are multiple authenticators active. * * If a single handler is passed, delegateToHandler() will be called instead * * @param array|RequestHandler[] $handlers * @param string $title The title of the form * @param array $templates + * @param callable $aggregator * @return array|HTTPResponse|RequestHandler|DBHTMLText|string */ - protected function delegateToMultipleHandlers(array $handlers, $title, array $templates) + protected function delegateToMultipleHandlers(array $handlers, $title, array $templates, callable $aggregator) { // Simpler case for a single authenticator @@ -757,30 +803,8 @@ class Security extends Controller implements TemplateGlobalProvider $handlers ); - // Aggregate all their forms, assuming they all return - $forms = []; - foreach ($results as $authName => $singleResult) { - // The result *must* be an array with a Form key - if (!is_array($singleResult) || !isset($singleResult['Form'])) { - user_error('Authenticator "' . $authName . '" doesn\'t support a tabbed login', E_USER_WARNING); - continue; - } - - $forms[] = $singleResult['Form']; - } - - if (!$forms) { - throw new \LogicException('No authenticators found compatible with a tabbed login'); - } - - return $this->renderWrappedController( - $title, - [ - 'Forms' => ArrayList::create($forms), - 'Form' => $this->generateLoginFormSet($forms), - ], - $templates - ); + $fragments = call_user_func_array($aggregator, [$results]); + return $this->renderWrappedController($title, $fragments, $templates); } /** @@ -796,8 +820,7 @@ class Security extends Controller implements TemplateGlobalProvider { $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 + // Return the customised controller - may be used to render a Form (e.g. login form) if (is_array($result)) { $result = $this->renderWrappedController($title, $result, $templates); } @@ -807,6 +830,7 @@ class Security extends Controller implements TemplateGlobalProvider /** * Render the given fragments into a security page controller with the given title. + * * @param string $title string The title to give the security page * @param array $fragments A map of objects to render into the page, e.g. "Form" * @param array $templates An array of templates to use for the render @@ -823,10 +847,10 @@ class Security extends Controller implements TemplateGlobalProvider // Handle any form messages from validation, etc. $messageType = ''; - $message = $this->getLoginMessage($messageType); + $message = $this->getSessionMessage($messageType); // We've displayed the message in the form output, so reset it for the next run. - static::clearLoginMessage(); + static::clearSessionMessage(); if ($message) { $messageResult = [ @@ -865,7 +889,8 @@ class Security extends Controller implements TemplateGlobalProvider return $this->delegateToMultipleHandlers( $handlers, _t('SilverStripe\\Security\\Security.LOSTPASSWORDHEADER', 'Lost Password'), - $this->getTemplatesFor('lostpassword') + $this->getTemplatesFor('lostpassword'), + [$this, 'aggregateTabbedForms'] ); } @@ -893,7 +918,8 @@ class Security extends Controller implements TemplateGlobalProvider return $this->delegateToMultipleHandlers( $handlers, _t('SilverStripe\\Security\\Security.CHANGEPASSWORDHEADER', 'Change your password'), - $this->getTemplatesFor('changepassword') + $this->getTemplatesFor('changepassword'), + [$this, 'aggregateTabbedForms'] ); } @@ -1220,7 +1246,8 @@ class Security extends Controller implements TemplateGlobalProvider */ public static function logout_url() { - return Controller::join_links(Director::baseURL(), self::config()->get('logout_url')); + $logoutUrl = Controller::join_links(Director::baseURL(), self::config()->get('logout_url')); + return SecurityToken::inst()->addToUrl($logoutUrl); } /** diff --git a/templates/SilverStripe/Security/Security_MultiAuthenticatorLogin.ss b/templates/SilverStripe/Security/Security_MultiAuthenticatorTabbedForms.ss similarity index 100% rename from templates/SilverStripe/Security/Security_MultiAuthenticatorLogin.ss rename to templates/SilverStripe/Security/Security_MultiAuthenticatorTabbedForms.ss diff --git a/tests/behat/features/login.feature b/tests/behat/features/login.feature index 5dbaf8f95..29ba09d5d 100644 --- a/tests/behat/features/login.feature +++ b/tests/behat/features/login.feature @@ -17,3 +17,11 @@ Feature: Log in # disable automatic redirection so we can use the profiler When I go to "/admin/" And I should see a log-in form + + Scenario: Logout without token + Given I am logged in with "ADMIN" permissions + When I go to "/Security/logout" + Then I should see a log-out form + When I press the "Log out" button + And I go to "/admin/" + Then I should see a log-in form diff --git a/tests/php/Security/SecurityTest.php b/tests/php/Security/SecurityTest.php index 029565661..677f06641 100644 --- a/tests/php/Security/SecurityTest.php +++ b/tests/php/Security/SecurityTest.php @@ -12,6 +12,7 @@ use SilverStripe\Security\LoginAttempt; use SilverStripe\Security\Member; use SilverStripe\Security\MemberAuthenticator\MemberAuthenticator; use SilverStripe\Security\Security; +use SilverStripe\Security\SecurityToken; use SilverStripe\Core\Config\Config; use SilverStripe\Core\Convert; use SilverStripe\Dev\FunctionalTest; @@ -277,6 +278,60 @@ class SecurityTest extends FunctionalTest $this->assertNotEquals('off', (string)$form[0]->attributes()->autocomplete); } + public function testLogout() + { + /* Enable SecurityToken */ + $securityTokenWasEnabled = SecurityToken::is_enabled(); + SecurityToken::enable(); + + $member = DataObject::get_one(Member::class); + + /* Log in with any user that we can find */ + Security::setCurrentUser($member); + + /* Visit the Security/logout page with a test referer, but without a security token */ + $response = $this->get( + Config::inst()->get(Security::class, 'logout_url'), + null, + ['Referer' => Director::absoluteBaseURL() . 'testpage'] + ); + + /* Make sure the user is still logged in */ + $this->assertNotNull(Security::getCurrentUser(), 'User is still logged in.'); + + $token = $this->cssParser()->getBySelector('#LogoutForm_Form #LogoutForm_Form_SecurityID'); + $actions = $this->cssParser()->getBySelector('#LogoutForm_Form input.action'); + + /* We have a security token, and an action to allow the user to log out */ + $this->assertCount(1, $token, 'There is a hidden field containing a security token.'); + $this->assertCount(1, $actions, 'There is 1 action, allowing the user to log out.'); + + /* Submit the form, using the logout action */ + $response = $this->submitForm( + 'LogoutForm_Form', + null, + array( + 'action_doLogout' => 1, + ) + ); + + /* We get a good response */ + $this->assertEquals(302, $response->getStatusCode()); + $this->assertRegExp( + '/testpage/', + $response->getHeader('Location'), + "Logout form redirects to back to referer." + ); + + /* User is logged out successfully */ + $this->assertNull(Security::getCurrentUser(), 'User is logged out.'); + + /* Re-disable SecurityToken */ + if (!$securityTokenWasEnabled) { + SecurityToken::disable(); + } + } + public function testExternalBackUrlRedirectionDisallowed() { // Test internal relative redirect