From 95c162ef0d645a2af9ad355ef195ab426a881fd4 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Tue, 31 Mar 2015 18:06:00 +1300 Subject: [PATCH] API Security better respects BackURL on login BUG Restore missing authentication message not appearing in the login form $Content area (regression from #1807) --- security/Security.php | 18 +++- tests/security/MemberTest.yml | 150 ++++++++++++++++---------------- tests/security/SecurityTest.php | 57 ++++++++++++ 3 files changed, 149 insertions(+), 76 deletions(-) diff --git a/security/Security.php b/security/Security.php index 97edc0f5c..77f6ed4c7 100644 --- a/security/Security.php +++ b/security/Security.php @@ -413,6 +413,20 @@ class Security extends Controller implements TemplateGlobalProvider { if($result instanceof SS_HTTPResponse) return $result; } } + + // If arriving on the login page already logged in, with no security error, and a ReturnURL then redirect + // back. The login message check is neccesary to prevent infinite loops where BackURL links to + // an action that triggers Security::permissionFailure. + // 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->request->requestVar('BackURL') + && !$this->getLoginMessage() + && ($member = Member::currentUser()) + && $member->exists() + ) { + return $this->redirectBack(); + } } /** @@ -523,6 +537,7 @@ class Security extends Controller implements TemplateGlobalProvider { // Finally, customise the controller to add any form messages and the form. $customisedController = $controller->customise(array( + "Content" => $message, "Message" => $message, "MessageType" => $messageType, "Form" => $content, @@ -572,7 +587,8 @@ class Security extends Controller implements TemplateGlobalProvider { * @return Form Returns the lost password form */ public function LostPasswordForm() { - return MemberLoginForm::create( $this, + return MemberLoginForm::create( + $this, 'LostPasswordForm', new FieldList( new EmailField('Email', _t('Member.EMAIL', 'Email')) diff --git a/tests/security/MemberTest.yml b/tests/security/MemberTest.yml index 3ea9a90f1..223dba5c2 100644 --- a/tests/security/MemberTest.yml +++ b/tests/security/MemberTest.yml @@ -1,78 +1,78 @@ Permission: - admin: - Code: ADMIN - security-admin: - Code: CMS_ACCESS_SecurityAdmin + admin: + Code: ADMIN + security-admin: + Code: CMS_ACCESS_SecurityAdmin Group: - admingroup: - Title: Admin - Code: admin - Permissions: =>Permission.admin - securityadminsgroup: - Title: securityadminsgroup - Code: securityadminsgroup - Permissions: =>Permission.security-admin - staffgroup: - Title: staffgroup - Code: staffgroup - managementgroup: - Title: managementgroup - Code: managementgroup - Parent: =>Group.staffgroup - accountinggroup: - Title: accountinggroup - Code: accountinggroup - Parent: =>Group.staffgroup - ceogroup: - Title: ceogroup - Code: ceogroup - Parent: =>Group.managementgroup - memberlessgroup: - Title: Memberless Group - code: memberless + admingroup: + Title: Admin + Code: admin + Permissions: =>Permission.admin + securityadminsgroup: + Title: securityadminsgroup + Code: securityadminsgroup + Permissions: =>Permission.security-admin + staffgroup: + Title: staffgroup + Code: staffgroup + managementgroup: + Title: managementgroup + Code: managementgroup + Parent: =>Group.staffgroup + accountinggroup: + Title: accountinggroup + Code: accountinggroup + Parent: =>Group.staffgroup + ceogroup: + Title: ceogroup + Code: ceogroup + Parent: =>Group.managementgroup + memberlessgroup: + Title: Memberless Group + code: memberless Member: - admin: - FirstName: Admin - Email: admin@silverstripe.com - Groups: =>Group.admingroup - other-admin: - FirstName: OtherAdmin - Email: other-admin@silverstripe.com - Groups: =>Group.admingroup - test: - FirstName: Test - Surname: User - Email: sam@silverstripe.com - Password: 1nitialPassword - PasswordExpiry: 2030-01-01 - Groups: =>Group.securityadminsgroup - expiredpassword: - FirstName: Test - Surname: User - Email: expired@silverstripe.com - Password: 1nitialPassword - PasswordExpiry: 2006-01-01 - noexpiry: - FirstName: Test - Surname: User - Email: noexpiry@silverstripe.com - Password: 1nitialPassword - staffmember: - Email: staffmember@test.com - Groups: =>Group.staffgroup - managementmember: - Email: managementmember@test.com - Groups: =>Group.managementgroup - accountingmember: - Email: accountingmember@test.com - Groups: =>Group.accountinggroup - ceomember: - Email: ceomember@test.com - Groups: =>Group.ceogroup - grouplessmember: - FirstName: Groupless Member - noformatmember: - Email: noformat@test.com - delocalemember: - Email: delocalemember@test.com - Locale: de_DE \ No newline at end of file + admin: + FirstName: Admin + Email: admin@silverstripe.com + Groups: =>Group.admingroup + other-admin: + FirstName: OtherAdmin + Email: other-admin@silverstripe.com + Groups: =>Group.admingroup + test: + FirstName: Test + Surname: User + Email: sam@silverstripe.com + Password: 1nitialPassword + PasswordExpiry: 2030-01-01 + Groups: =>Group.securityadminsgroup + expiredpassword: + FirstName: Test + Surname: User + Email: expired@silverstripe.com + Password: 1nitialPassword + PasswordExpiry: 2006-01-01 + noexpiry: + FirstName: Test + Surname: User + Email: noexpiry@silverstripe.com + Password: 1nitialPassword + staffmember: + Email: staffmember@test.com + Groups: =>Group.staffgroup + managementmember: + Email: managementmember@test.com + Groups: =>Group.managementgroup + accountingmember: + Email: accountingmember@test.com + Groups: =>Group.accountinggroup + ceomember: + Email: ceomember@test.com + Groups: =>Group.ceogroup + grouplessmember: + FirstName: Groupless Member + noformatmember: + Email: noformat@test.com + delocalemember: + Email: delocalemember@test.com + Locale: de_DE diff --git a/tests/security/SecurityTest.php b/tests/security/SecurityTest.php index be163dadb..c44aefef7 100644 --- a/tests/security/SecurityTest.php +++ b/tests/security/SecurityTest.php @@ -114,7 +114,64 @@ class SecurityTest extends FunctionalTest { Config::unnest(); } + + /** + * Follow all redirects recursively + * + * @param string $url + * @param int $limit Max number of requests + * @return SS_HTTPResponse + */ + protected function getRecursive($url, $limit = 10) { + $this->cssParser = null; + $response = $this->mainSession->get($url); + while(--$limit > 0 && $response instanceof SS_HTTPResponse && $response->getHeader('Location')) { + $response = $this->mainSession->followRedirection(); + } + return $response; + } + public function testAutomaticRedirectionOnLogin() { + // BackURL with permission error (not authenticated) should not redirect + if($member = Member::currentUser()) $member->logOut(); + $response = $this->getRecursive('SecurityTest_SecuredController'); + $this->assertContains(Convert::raw2xml("That page is secured."), $response->getBody()); + $this->assertContains('getBody()); + + // Non-logged in user should not be redirected, but instead shown the login form + // No message/context is available as the user has not attempted to view the secured controller + $response = $this->getRecursive('Security/login?BackURL=SecurityTest_SecuredController/'); + $this->assertNotContains(Convert::raw2xml("That page is secured."), $response->getBody()); + $this->assertNotContains(Convert::raw2xml("You don't have access to this page"), $response->getBody()); + $this->assertContains('getBody()); + + // BackURL with permission error (wrong permissions) should not redirect + $this->logInAs('grouplessmember'); + $response = $this->getRecursive('SecurityTest_SecuredController'); + $this->assertContains(Convert::raw2xml("You don't have access to this page"), $response->getBody()); + $this->assertContains( + 'getBody() + ); + + // Directly accessing this page should attempt to follow the BackURL, but stop when it encounters the error + $response = $this->getRecursive('Security/login?BackURL=SecurityTest_SecuredController/'); + $this->assertContains(Convert::raw2xml("You don't have access to this page"), $response->getBody()); + $this->assertContains( + 'getBody() + ); + + // Check correctly logged in admin doesn't generate the same errors + $this->logInAs('admin'); + $response = $this->getRecursive('SecurityTest_SecuredController'); + $this->assertContains(Convert::raw2xml("Success"), $response->getBody()); + + // Directly accessing this page should attempt to follow the BackURL and succeed + $response = $this->getRecursive('Security/login?BackURL=SecurityTest_SecuredController/'); + $this->assertContains(Convert::raw2xml("Success"), $response->getBody()); + } + public function testLogInAsSomeoneElse() { $member = DataObject::get_one('Member');