From 233e0e7aa06be06c8e25eda6abba407041c69089 Mon Sep 17 00:00:00 2001 From: Serge Latyntsev Date: Thu, 12 Sep 2019 14:34:06 +1200 Subject: [PATCH] ENH PasswordExpirationMiddleware implementation (#9207) --- _config/security.yml | 9 + docs/en/04_Changelogs/4.5.0.md | 31 +- .../MemberAuthenticator/CMSLoginHandler.php | 13 +- .../MemberAuthenticator/LoginHandler.php | 4 +- src/Security/PasswordExpirationMiddleware.php | 247 ++++++++++ .../PasswordExpirationMiddlewareTest.php | 446 ++++++++++++++++++ 6 files changed, 746 insertions(+), 4 deletions(-) create mode 100644 src/Security/PasswordExpirationMiddleware.php create mode 100644 tests/php/Security/PasswordExpirationMiddlewareTest.php diff --git a/_config/security.yml b/_config/security.yml index d4161b62f..1dfd8ef92 100644 --- a/_config/security.yml +++ b/_config/security.yml @@ -27,6 +27,7 @@ SilverStripe\Core\Injector\Injector: Middlewares: AuthenticationMiddleware: '%$SilverStripe\Security\AuthenticationMiddleware' BasicAuthMiddleware: '%$SilverStripe\Security\BasicAuthMiddleware' + PasswordExpirationMiddleware: '%$SilverStripe\Security\PasswordExpirationMiddleware' SilverStripe\Security\AuthenticationMiddleware: properties: AuthenticationHandler: '%$SilverStripe\Security\AuthenticationHandler' @@ -42,3 +43,11 @@ SilverStripe\Core\Injector\Injector: Authenticators: cms: '%$SilverStripe\Security\MemberAuthenticator\CMSMemberAuthenticator' SilverStripe\Security\IdentityStore: '%$SilverStripe\Security\AuthenticationHandler' + +SilverStripe\Security\PasswordExpirationMiddleware: + default_redirect: Security/changepassword + whitelisted_url_startswith: + - Security/basicauthlogin/ + - Security/changepassword/ + - Security/login/ + - Security/logout/ diff --git a/docs/en/04_Changelogs/4.5.0.md b/docs/en/04_Changelogs/4.5.0.md index 8b3de6735..fb9555645 100644 --- a/docs/en/04_Changelogs/4.5.0.md +++ b/docs/en/04_Changelogs/4.5.0.md @@ -4,7 +4,7 @@ * [Installer UI has been removed](#installer-ui) and offered as a separate module. * [Generic login form styling](#login-forms) - * Removed `use_gzip` option on `HtmlEditorField` which used to compress the rich text editor dependency. + * Removed `use_gzip` option on `HtmlEditorField` which used to compress the rich text editor dependency. No longer required since compression is performed as part of the CMS build automatically. See (#832)(https://github.com/silverstripe/silverstripe-admin/issues/832) @@ -48,7 +48,34 @@ composer require silverstripe/login-forms Note that any customisations you might have in `Page.ss` or `Layout/Security.ss` no longer apply when this module is installed. If you have customised the login process by adding form fields, or through custom handlers such as SAML or LDAP, -you'll need to review those before starting to use the module. +you'll need to review those before starting to use the module. + + +## New PasswordExpirationMiddleware now proactively invalidates members with expired passwords + +A new PasswordExpirationMiddleware has been implemented. +It checks passwords of authenticated users for expiration and either enforces a redirection +to a change password form, or resets the user for a request being processed (sets current user to null). + +This is considered to be a security enhancement, but potentially might interfere with some custom logic +around password expiration if you have it implemented. + +Ideally you should test your setup when upgrading if you use the password expiration functionality. + +If you'd like to deactivate the middleware, you can unregister it in your application config like this: + +```yml +--- +Name: disable-passwordExpirationMiddleware +After: + - '#coresecurity' +--- +SilverStripe\Core\Injector\Injector: + SilverStripe\Control\Director: + properties: + Middlewares: + PasswordExpirationMiddleware: null +``` ## Deprecation diff --git a/src/Security/MemberAuthenticator/CMSLoginHandler.php b/src/Security/MemberAuthenticator/CMSLoginHandler.php index ff85f8b0a..9a61fa8d3 100644 --- a/src/Security/MemberAuthenticator/CMSLoginHandler.php +++ b/src/Security/MemberAuthenticator/CMSLoginHandler.php @@ -2,9 +2,12 @@ namespace SilverStripe\Security\MemberAuthenticator; +use SilverStripe\Control\Controller; use SilverStripe\Control\Director; use SilverStripe\Control\HTTPResponse; use SilverStripe\Core\Convert; +use SilverStripe\Core\Injector\Injector; +use SilverStripe\Security\PasswordExpirationMiddleware; use SilverStripe\Security\CMSSecurity; use SilverStripe\Security\Security; @@ -65,7 +68,15 @@ class CMSLoginHandler extends LoginHandler ); // Get redirect url - $changePasswordURL = $this->addBackURLParam(Security::singleton()->Link('changepassword')); + $changedPasswordLink = Security::singleton()->Link('changepassword'); + $changePasswordURL = $this->addBackURLParam($changedPasswordLink); + + if (Injector::inst()->has(PasswordExpirationMiddleware::class)) { + $session = $this->getRequest()->getSession(); + $passwordExpirationMiddleware = Injector::inst()->get(PasswordExpirationMiddleware::class); + $passwordExpirationMiddleware->allowCurrentRequest($session); + } + $changePasswordURLATT = Convert::raw2att($changePasswordURL); $changePasswordURLJS = Convert::raw2js($changePasswordURL); $message = _t( diff --git a/src/Security/MemberAuthenticator/LoginHandler.php b/src/Security/MemberAuthenticator/LoginHandler.php index f97538810..e3728393f 100644 --- a/src/Security/MemberAuthenticator/LoginHandler.php +++ b/src/Security/MemberAuthenticator/LoginHandler.php @@ -9,6 +9,7 @@ use SilverStripe\Control\RequestHandler; use SilverStripe\Core\Injector\Injector; use SilverStripe\ORM\ValidationResult; use SilverStripe\Security\Authenticator; +use SilverStripe\Security\PasswordExpirationMiddleware; use SilverStripe\Security\IdentityStore; use SilverStripe\Security\Member; use SilverStripe\Security\Security; @@ -263,7 +264,8 @@ class LoginHandler extends RequestHandler 'good' ); $changedPasswordLink = Security::singleton()->Link('changepassword'); + $changePasswordUrl = $this->addBackURLParam($changedPasswordLink); - return $this->redirect($this->addBackURLParam($changedPasswordLink)); + return $this->redirect($changePasswordUrl); } } diff --git a/src/Security/PasswordExpirationMiddleware.php b/src/Security/PasswordExpirationMiddleware.php new file mode 100644 index 000000000..050673000 --- /dev/null +++ b/src/Security/PasswordExpirationMiddleware.php @@ -0,0 +1,247 @@ +checkForExpiredPassword($request)) { + return $response; + } + } catch (DatabaseException $e) { + // Database isn't ready, carry on. + } + + return $delegate($request); + } + + /** + * Check if the just authenticated member has the password expired. + * Returns a response if the current request should not be + * processed as usual. + * + * @param HTTPRequest $request + * + * @return HTTPResponse|null + */ + protected function checkForExpiredPassword(HTTPRequest $request): ?HTTPResponse + { + $session = $request->getSession(); + + if ($session && $session->get(static::SESSION_KEY_ALLOW_CURRENT_REQUEST)) { + // allow current request and skip the expiration check, but for only the current + // request, so we're deleting the flag from the session so it's not affecting other + // requests. + // This flag would usually be set from within $handler->authenticateRequest() + $session->clear(static::SESSION_KEY_ALLOW_CURRENT_REQUEST); + + return null; + } + + $user = Security::getCurrentUser(); + if ($user && $user->isPasswordExpired()) { + if ($response = $this->handleExpiredPassword($request)) { + return $response; + } + } + + return null; + } + + /** + * Check if we have a redirect to a password change form registered + * and redirect there if possible. + * Otherwise, deauthenticate the user by resetting it for this request, + * since we should treat ones with expired passwords as unauthorised. + * + * @param HTTPRequest $request + * + * @return HTTPResponse|null + */ + protected function handleExpiredPassword(HTTPRequest $request): ?HTTPResponse + { + $session = $request->getSession(); + + $sessionRedirectUrl = $session->get(static::SESSION_KEY_REDIRECT); + $defaultRedirectUrl = static::config()->get('default_redirect'); + + if ($sessionRedirectUrl || $defaultRedirectUrl) { + $redirectUrl = $this->absoluteUrl($sessionRedirectUrl ?? $defaultRedirectUrl); + } else { + $redirectUrl = null; + } + + if (!$session || !$redirectUrl) { + Security::setCurrentUser(null); + return null; + } + + $currentUrl = $this->absoluteUrl($request->getURL(true)); + if ($currentUrl === $redirectUrl) { + return null; + } + + $allowedStartswith = static::config()->get('whitelisted_url_startswith'); + if (is_array($allowedStartswith)) { + foreach ($allowedStartswith as $pattern) { + $startswith = $this->absoluteUrl($pattern); + + if (strncmp($currentUrl, $startswith, strlen($startswith)) === 0) { + return null; + } + } + } + + return $this->redirectOrForbid($request, $redirectUrl); + } + + /** + * Builds an absolute URL for the given path, adds base url + * if the path configured as absolute + * + * @param string $url + * + * @return string + */ + protected static function absoluteUrl($url): string + { + if (substr($url, 0, 1) === '/' && substr($url, 1, 1) !== '/') { + // add BASE_URL explicitly if not absolute + $url = Controller::join_links(Director::absoluteBaseURL(), $url); + } else { + $url = Director::absoluteURL($url) ?: Controller::join_links(Director::absoluteBaseURL(), $url); + } + + if (substr($url, -1) === '/') { + $url = substr($url, 0, -1); + } + + return $url; + } + + /** + * Returns a redirect to the URL if text/html is acceptable, otherwise + * deauthenticates the current request by Security::setCurrentUser(null) + * + * @param HTTPRequest $request + * @param string $redirectUrl + * + * @return HTTPResponse|null + */ + private function redirectOrForbid(HTTPRequest $request, $redirectUrl): ?HTTPResponse + { + $acceptableTypes = $request->getAcceptMimetypes(); + + $allowedTypes = static::config()->get('mimetypes_allowing_redirect') ?? []; + + if (count(array_intersect($allowedTypes, $acceptableTypes)) > 0) { + $redirectAllowed = true; + } else { + // if browser didn't send the Accept header + // with mimetypes, let's redirect anyway + $redirectAllowed = count($acceptableTypes) === 0; + } + + if ($redirectAllowed) { + $response = new HTTPResponse(); + $response->redirect($redirectUrl); + return $response; + } + + Security::setCurrentUser(null); + + return null; + } + + /** + * Preserve the password change URL in the session + * That URL is to be redirected to to force users change expired passwords + * + * @param Session $session Session where we persist the redirect URL + * @param string $url change password form address + */ + public static function setRedirect(Session $session, $url) + { + $session->set(static::SESSION_KEY_REDIRECT, $url); + } + + /** + * Allow the current request to be finished without password expiration check + * + * @param Session $session Session where we persist the redirect URL + * @param string $url change password form address + */ + public static function allowCurrentRequest(Session $session) + { + $session->set(static::SESSION_KEY_ALLOW_CURRENT_REQUEST, true); + } +} diff --git a/tests/php/Security/PasswordExpirationMiddlewareTest.php b/tests/php/Security/PasswordExpirationMiddlewareTest.php new file mode 100644 index 000000000..53136b92c --- /dev/null +++ b/tests/php/Security/PasswordExpirationMiddlewareTest.php @@ -0,0 +1,446 @@ +set('alternate_base_url', 'http://localhost/custom-base/'); + PasswordExpirationMiddleware::config()->set('default_redirect', null); + PasswordExpirationMiddleware::config()->set('whitelisted_url_startswith', []); + } + + /** + * Returns Member mock object + * + * @param bool $isPasswordExpired result of the function {@see SilverStripe\Security\Member::isPasswordExpired} + * + * @return Member + */ + private function getMemberMock($isPasswordExpired) : Member + { + $mock = $this->createMock(Member::class); + $mock->method('isPasswordExpired')->will($this->returnValue($isPasswordExpired)); + + return $mock; + } + + public function test200() + { + $member = $this->getMemberMock(false); + + $a = new PasswordExpirationMiddleware(); + + $request = $this->buildRequestMock('/'); + $executed = false; + Security::setCurrentUser($member); + $response = $a->process($request, static function () use (&$executed) { + $executed = true; + return "delegated"; + }); + + $this->assertEquals($member, Security::getCurrentUser()); + $this->assertTrue($executed); + $this->assertEquals("delegated", $response); + } + + /** + * Check a member with an expired password is allowed to process the request in + * deauthorised mode (Security::getCurrentUser() === null) if there are no + * change password redirects registered + * + * @depends test200 + */ + public function testDeauthorised() + { + $member = $this->getMemberMock(true); + $this->assertTrue($member->isPasswordExpired()); + + $a = new PasswordExpirationMiddleware(); + + $request = $this->buildRequestMock('/'); + $executed = false; + $activeMember = null; + Security::setCurrentUser($member); + $response = $a->process($request, static function () use (&$executed, &$activeMember) { + $executed = true; + $activeMember = Security::getCurrentUser(); + }); + + $this->assertTrue($executed); + $this->assertNull($activeMember); + } + + /** + * Check a member with an expired password is redirected to a change password form + * instead of processing its original request + * + * @depends test200 + */ + public function testRedirected() + { + $member = $this->getMemberMock(true); + $this->assertTrue($member->isPasswordExpired()); + + $a = new PasswordExpirationMiddleware(); + + $request = $this->buildRequestMock('/'); + $request->method('getAcceptMimetypes')->will($this->returnValue(['*/*'])); + $session = $request->getSession(); + + $a->setRedirect($session, '/redirect-address-custom'); + + $executed = false; + $activeMember = null; + Security::setCurrentUser($member); + $response = $a->process($request, static function () use (&$executed, &$activeMember) { + $executed = true; + $activeMember = Security::getCurrentUser(); + }); + + $this->assertFalse($executed); + $this->assertNull($activeMember); + + $this->assertEquals(302, $response->getStatusCode()); + $this->assertEquals(Director::absoluteURL('redirect-address-custom'), $response->getHeader('Location')); + } + + /** + * Check we handle network locations correctly (the relative urls starting with //) + * + * @depends testRedirected + */ + public function testNetworkLocationRedirect() + { + $member = $this->getMemberMock(true); + $this->assertTrue($member->isPasswordExpired()); + + $a = new PasswordExpirationMiddleware(); + + $request = $this->buildRequestMock('/'); + $request->method('getAcceptMimetypes')->will($this->returnValue(['*/*'])); + $session = $request->getSession(); + + $a->setRedirect($session, '//localhost/custom-base/redirect-address-custom'); + + $executed = false; + $activeMember = null; + Security::setCurrentUser($member); + $response = $a->process($request, static function () use (&$executed, &$activeMember) { + $executed = true; + $activeMember = Security::getCurrentUser(); + }); + + $this->assertFalse($executed); + $this->assertNull($activeMember); + + $this->assertEquals(302, $response->getStatusCode()); + $this->assertEquals(Director::absoluteURL('redirect-address-custom'), $response->getHeader('Location')); + } + + /** + * Check we can allow the current request handling even with an expired password + * + * @depends test200 + * @depends testDeauthorised + */ + public function testAllowRequest() + { + $member = $this->getMemberMock(true); + $this->assertTrue($member->isPasswordExpired()); + + $a = new PasswordExpirationMiddleware(); + + $request = $this->buildRequestMock('/'); + $session = $request->getSession(); + PasswordExpirationMiddleware::allowCurrentRequest($session); + + $executed = false; + $activeMember = null; + Security::setCurrentUser($member); + $response = $a->process($request, static function () use (&$executed, &$activeMember) { + $executed = true; + $activeMember = Security::getCurrentUser(); + }); + + $this->assertTrue($executed); + $this->assertEquals($member, $activeMember); + } + + /** + * Check a member with an expired password is redirected to a default change password form + * if a custom not set + * + * @depends testRedirected + */ + public function testDefaultRedirect() + { + $member = $this->getMemberMock(true); + $this->assertTrue($member->isPasswordExpired()); + + PasswordExpirationMiddleware::config()->set('default_redirect', 'redirect-address-default'); + + $a = new PasswordExpirationMiddleware(); + + $request = $this->buildRequestMock('/'); + $request->method('getAcceptMimetypes')->will($this->returnValue(['*/*'])); + $session = $request->getSession(); + + $executed = false; + $activeMember = null; + Security::setCurrentUser($member); + $response = $a->process($request, static function () use (&$executed, &$activeMember) { + $executed = true; + $activeMember = Security::getCurrentUser(); + }); + + $this->assertFalse($executed); + $this->assertNull($activeMember); + + $this->assertEquals(302, $response->getStatusCode()); + $this->assertEquals(Director::absoluteURL('redirect-address-default'), $response->getHeader('Location')); + } + + /** + * Check a member with an expired password is redirected to a default change password form + * if a custom not set + * + * @depends testDefaultRedirect + */ + public function testCustomRedirect() + { + $member = $this->getMemberMock(true); + $this->assertTrue($member->isPasswordExpired()); + + PasswordExpirationMiddleware::config()->set('default_redirect', '/redirect-address-default'); + + $a = new PasswordExpirationMiddleware(); + + $request = $this->buildRequestMock('/'); + $request->method('getAcceptMimetypes')->will($this->returnValue(['*/*'])); + + $executed = false; + $activeMember = null; + Security::setCurrentUser($member); + $response = $a->process($request, static function () use (&$executed, &$activeMember) { + $executed = true; + $activeMember = Security::getCurrentUser(); + }); + + $this->assertFalse($executed); + $this->assertNull($activeMember); + + $this->assertEquals(302, $response->getStatusCode()); + $this->assertEquals(Director::absoluteURL('redirect-address-default'), $response->getHeader('Location')); + } + + /** + * Check a custom redirect URL overrides the default one + * + * @depends testCustomRedirect + */ + public function testCustomOverDefaultRedirect() + { + $member = $this->getMemberMock(true); + $this->assertTrue($member->isPasswordExpired()); + + PasswordExpirationMiddleware::config()->set('default_redirect', '/redirect-address-default'); + + $a = new PasswordExpirationMiddleware(); + + $request = $this->buildRequestMock('/'); + $request->method('getAcceptMimetypes')->will($this->returnValue(['*/*'])); + $session = $request->getSession(); + $a->setRedirect($session, '/redirect-address-custom'); + + $executed = false; + $activeMember = null; + Security::setCurrentUser($member); + $response = $a->process($request, static function () use (&$executed, &$activeMember) { + $executed = true; + $activeMember = Security::getCurrentUser(); + }); + + $this->assertFalse($executed); + $this->assertNull($activeMember); + + $this->assertEquals(302, $response->getStatusCode()); + $this->assertEquals(Director::absoluteURL('redirect-address-custom'), $response->getHeader('Location')); + } + + /** + * Test we can allow URLs to be visited without redirections through config + * + * @depends testRedirected + */ + public function testAllowedUrlStartswithNegative() + { + $member = $this->getMemberMock(true); + $this->assertTrue($member->isPasswordExpired()); + + PasswordExpirationMiddleware::config()->set('whitelisted_url_startswith', [ + '/allowed-address-configured/' + ]); + + $a = new PasswordExpirationMiddleware(); + + $request = $this->buildRequestMock('/not-allowed'); + $request->method('getAcceptMimetypes')->will($this->returnValue(['*/*'])); + $session = $request->getSession(); + + $a->setRedirect($session, '/redirect-address-custom'); + + $executed = false; + $activeMember = null; + Security::setCurrentUser($member); + $response = $a->process($request, static function () use (&$executed, &$activeMember) { + $executed = true; + $activeMember = Security::getCurrentUser(); + }); + + $this->assertFalse($executed); + $this->assertNull($activeMember); + + $this->assertEquals(302, $response->getStatusCode()); + $this->assertEquals(Director::absoluteURL('redirect-address-custom'), $response->getHeader('Location')); + } + + /** + * Test we can allow URLs to be visited without redirections through config + * + * @depends testRedirected + */ + public function testAllowedUrlStartswithPositivePattern() + { + $member = $this->getMemberMock(true); + $this->assertTrue($member->isPasswordExpired()); + + PasswordExpirationMiddleware::config()->set('whitelisted_url_startswith', [ + '/allowed-address-configured/' + ]); + + $a = new PasswordExpirationMiddleware(); + + $request = $this->buildRequestMock('/allowed-address-configured/subsection1/subsection2/'); + $request->method('getAcceptMimetypes')->will($this->returnValue(['*/*'])); + $session = $request->getSession(); + + $a->setRedirect($session, '/redirect-address-custom'); + + $executed = false; + $activeMember = null; + Security::setCurrentUser($member); + $response = $a->process($request, static function () use (&$executed, &$activeMember) { + $executed = true; + $activeMember = Security::getCurrentUser(); + }); + + $this->assertTrue($executed); + $this->assertEquals($member, $activeMember); + } + + public function testAllowedUrlStartswithPositiveTrailingSlash() + { + $member = $this->getMemberMock(true); + $this->assertTrue($member->isPasswordExpired()); + + PasswordExpirationMiddleware::config()->set('whitelisted_url_startswith', [ + '/allowed-address-configured/' + ]); + + $a = new PasswordExpirationMiddleware(); + + $request = $this->buildRequestMock('/allowed-address-configured?foo=bar'); + $request->method('getAcceptMimetypes')->will($this->returnValue(['*/*'])); + $session = $request->getSession(); + + $a->setRedirect($session, '/redirect-address-custom'); + + $executed = false; + $activeMember = null; + Security::setCurrentUser($member); + $response = $a->process($request, static function () use (&$executed, &$activeMember) { + $executed = true; + $activeMember = Security::getCurrentUser(); + }); + + $this->assertTrue($executed); + $this->assertEquals($member, $activeMember); + } + + public function testAllowedUrlStartswithPositiveRelativeUrl() + { + $member = $this->getMemberMock(true); + $this->assertTrue($member->isPasswordExpired()); + + PasswordExpirationMiddleware::config()->set('whitelisted_url_startswith', [ + 'allowed-address-configured/' + ]); + + $a = new PasswordExpirationMiddleware(); + + $request = $this->buildRequestMock('/allowed-address-configured?foo=bar'); + $request->method('getAcceptMimetypes')->will($this->returnValue(['*/*'])); + $session = $request->getSession(); + + $a->setRedirect($session, 'redirect-address-custom'); + + $executed = false; + $activeMember = null; + Security::setCurrentUser($member); + $response = $a->process($request, static function () use (&$executed, &$activeMember) { + $executed = true; + $activeMember = Security::getCurrentUser(); + }); + + $this->assertTrue($executed); + $this->assertEquals($member, $activeMember); + } + + /** + * Test we can allow URLs to be visited without redirections through config + * + * @depends testRedirected + */ + public function testAllowedUrlStartswithPositiveExactUrl() + { + $member = $this->getMemberMock(true); + $this->assertTrue($member->isPasswordExpired()); + + PasswordExpirationMiddleware::config()->set('whitelisted_url_startswith', [ + '/allowed-address-configured/' + ]); + + $a = new PasswordExpirationMiddleware(); + + $request = $this->buildRequestMock('/allowed-address-configured/'); + $request->method('getAcceptMimetypes')->will($this->returnValue(['*/*'])); + $session = $request->getSession(); + + $a->setRedirect($session, '/redirect-address-custom'); + + $executed = false; + $activeMember = null; + Security::setCurrentUser($member); + $response = $a->process($request, static function () use (&$executed, &$activeMember) { + $executed = true; + $activeMember = Security::getCurrentUser(); + }); + + $this->assertTrue($executed); + $this->assertEquals($member, $activeMember); + } +}