From faca30f23fd98143314ca89293915aca9a0325a5 Mon Sep 17 00:00:00 2001 From: diosmosis Date: Sun, 23 Sep 2018 15:08:33 -0700 Subject: Remove user-agent checking code in SessionAuth. (#13470) * Remove user-agent checking code in SessionAuth. * Fixing test. * Fixing couple more tests. --- core/Session/SessionAuth.php | 5 -- core/Session/SessionFingerprint.php | 20 +------- .../Integration/Session/SessionAuthTest.php | 49 +++---------------- .../Unit/Session/SessionFingerprintTest.php | 56 +--------------------- .../Unit/Session/SessionInitializerTest.php | 6 +-- 5 files changed, 11 insertions(+), 125 deletions(-) diff --git a/core/Session/SessionAuth.php b/core/Session/SessionAuth.php index 0c23f13cc2..e185600100 100644 --- a/core/Session/SessionAuth.php +++ b/core/Session/SessionAuth.php @@ -99,11 +99,6 @@ class SessionAuth implements Auth return $this->makeAuthFailure(); } - if (!$sessionFingerprint->isMatchingCurrentRequest()) { - $this->initNewBlankSession($sessionFingerprint); - return $this->makeAuthFailure(); - } - $tsPasswordModified = !empty($user['ts_password_modified']) ? $user['ts_password_modified'] : null; if ($this->isSessionStartedBeforePasswordChange($sessionFingerprint, $tsPasswordModified)) { $this->destroyCurrentSession($sessionFingerprint); diff --git a/core/Session/SessionFingerprint.php b/core/Session/SessionFingerprint.php index 2239661fb9..8b61c62817 100644 --- a/core/Session/SessionFingerprint.php +++ b/core/Session/SessionFingerprint.php @@ -56,12 +56,11 @@ class SessionFingerprint return null; } - public function initialize($userName, $time = null, $userAgent = null) + public function initialize($userName, $time = null) { $_SESSION[self::USER_NAME_SESSION_VAR_NAME] = $userName; $_SESSION[self::SESSION_INFO_SESSION_VAR_NAME] = [ 'ts' => $time ?: Date::now()->getTimestampUTC(), - 'ua' => $userAgent ?: $this->getUserAgent(), ]; } @@ -71,18 +70,6 @@ class SessionFingerprint unset($_SESSION[self::SESSION_INFO_SESSION_VAR_NAME]); } - public function isMatchingCurrentRequest() - { - $requestUa = $this->getUserAgent(); - - $userInfo = $this->getUserInfo(); - if (empty($userInfo)) { - return false; - } - - return $userInfo['ua'] == $requestUa; - } - public function getSessionStartTime() { $userInfo = $this->getUserInfo(); @@ -94,9 +81,4 @@ class SessionFingerprint return $userInfo['ts']; } - - private function getUserAgent() - { - return array_key_exists('HTTP_USER_AGENT', $_SERVER) ? $_SERVER['HTTP_USER_AGENT'] : null; - } } diff --git a/tests/PHPUnit/Integration/Session/SessionAuthTest.php b/tests/PHPUnit/Integration/Session/SessionAuthTest.php index cc4790b7b4..00392f2bc7 100644 --- a/tests/PHPUnit/Integration/Session/SessionAuthTest.php +++ b/tests/PHPUnit/Integration/Session/SessionAuthTest.php @@ -20,7 +20,6 @@ use Piwik\Plugins\UsersManager\Model as UsersModel; class SessionAuthTest extends IntegrationTestCase { - const TEST_UA = 'test-user-agent'; const TEST_OTHER_USER = 'testuser'; /** @@ -37,37 +36,9 @@ class SessionAuthTest extends IntegrationTestCase $this->testInstance = StaticContainer::get(SessionAuth::class); } - public function test_authenticate_ReturnsFailure_IfRequestUserAgentDiffersFromSessionUserAgent() - { - $this->initializeSession(self::TEST_UA, Fixture::ADMIN_USER_LOGIN); - $this->initializeRequest('some-other-user-agent'); - - $result = $this->testInstance->authenticate(); - $this->assertEquals(AuthResult::FAILURE, $result->getCode()); - } - - public function test_authenticate_ReturnsSuccess_IfRequestUserAgentMatchSession() - { - $this->initializeSession(self::TEST_UA, self::TEST_OTHER_USER); - $this->initializeRequest(self::TEST_UA); - - $result = $this->testInstance->authenticate(); - $this->assertEquals(AuthResult::SUCCESS, $result->getCode()); - } - - public function test_authenticate_ReturnsSuperUserSuccess_IfRequestUserAgentMatchSession() - { - $this->initializeSession(self::TEST_UA, Fixture::ADMIN_USER_LOGIN); - $this->initializeRequest(self::TEST_UA); - - $result = $this->testInstance->authenticate(); - $this->assertEquals(AuthResult::SUCCESS_SUPERUSER_AUTH_CODE, $result->getCode()); - } - public function test_authenticate_ReturnsFailure_IfNoSessionExists() { - $this->initializeSession(self::TEST_UA, Fixture::ADMIN_USER_LOGIN); - $this->initializeRequest(self::TEST_UA); + $this->initializeSession(Fixture::ADMIN_USER_LOGIN); $this->destroySession(); @@ -77,8 +48,7 @@ class SessionAuthTest extends IntegrationTestCase public function test_authenticate_ReturnsFailure_IfAuthenticatedSession_AndPasswordChangedAfterSessionCreated() { - $this->initializeSession(self::TEST_UA, self::TEST_OTHER_USER); - $this->initializeRequest(self::TEST_UA); + $this->initializeSession(self::TEST_OTHER_USER); sleep(1); @@ -92,8 +62,7 @@ class SessionAuthTest extends IntegrationTestCase public function test_authenticate_ReturnsFailure_IfUsersModelReturnsIncorrectUser() { - $this->initializeSession(self::TEST_UA, self::TEST_OTHER_USER); - $this->initializeRequest(self::TEST_UA); + $this->initializeSession(self::TEST_OTHER_USER); $sessionAuth = new SessionAuth(new MockUsersModel([ 'login' => 'wronguser', @@ -105,8 +74,7 @@ class SessionAuthTest extends IntegrationTestCase public function test_authenticate_ReturnsSuccess_IfUserDataHasNoPasswordModifiedTimestamp() { - $this->initializeSession(self::TEST_UA, self::TEST_OTHER_USER); - $this->initializeRequest(self::TEST_UA); + $this->initializeSession(self::TEST_OTHER_USER); $usersModel = new UsersModel(); $user = $usersModel->getUser(self::TEST_OTHER_USER); @@ -118,15 +86,10 @@ class SessionAuthTest extends IntegrationTestCase $this->assertEquals(AuthResult::SUCCESS, $result->getCode()); } - private function initializeRequest($userAgent) - { - $_SERVER['HTTP_USER_AGENT'] = $userAgent; - } - - private function initializeSession($userAgent, $userLogin) + private function initializeSession($userLogin) { $sessionFingerprint = new SessionFingerprint(); - $sessionFingerprint->initialize($userLogin, $time = null, $userAgent); + $sessionFingerprint->initialize($userLogin, $time = null); } protected static function configureFixture($fixture) diff --git a/tests/PHPUnit/Unit/Session/SessionFingerprintTest.php b/tests/PHPUnit/Unit/Session/SessionFingerprintTest.php index 047fdd80df..e9bbdd9f38 100644 --- a/tests/PHPUnit/Unit/Session/SessionFingerprintTest.php +++ b/tests/PHPUnit/Unit/Session/SessionFingerprintTest.php @@ -43,7 +43,6 @@ class SessionFingerprintTest extends \PHPUnit_Framework_TestCase { $sessionVarValue = [ 'ip' => 'someip', - 'ua' => 'someua', ]; $_SESSION[SessionFingerprint::SESSION_INFO_SESSION_VAR_NAME] = $sessionVarValue; @@ -57,68 +56,15 @@ class SessionFingerprintTest extends \PHPUnit_Framework_TestCase public function test_initialize_SetsSessionVarsToCurrentRequest() { - $_SERVER['HTTP_USER_AGENT'] = 'test-user-agent'; $this->testInstance->initialize('testuser', self::TEST_TIME_VALUE); $this->assertEquals('testuser', $_SESSION[SessionFingerprint::USER_NAME_SESSION_VAR_NAME]); $this->assertEquals( - ['ts' => self::TEST_TIME_VALUE, 'ua' => 'test-user-agent'], + ['ts' => self::TEST_TIME_VALUE], $_SESSION[SessionFingerprint::SESSION_INFO_SESSION_VAR_NAME] ); } - public function test_initialize_DoesNotSetUserAgent_IfUserAgentIsNotInHttpRequest() - { - unset($_SERVER['HTTP_USER_AGENT']); - $this->testInstance->initialize('testuser', self::TEST_TIME_VALUE); - - $this->assertEquals('testuser', $_SESSION[SessionFingerprint::USER_NAME_SESSION_VAR_NAME]); - $this->assertEquals( - ['ts' => self::TEST_TIME_VALUE, 'ua' => null], - $_SESSION[SessionFingerprint::SESSION_INFO_SESSION_VAR_NAME] - ); - } - - /** - * @dataProvider getTestDataForIsMatchingCurrentRequest - */ - public function test_isMatchingCurrentRequest_ChecksIfSessionVarsMatchRequest( - $sessionUa, $requestUa, $expectedResult - ) { - $_SESSION[SessionFingerprint::SESSION_INFO_SESSION_VAR_NAME] = [ - 'ua' => $sessionUa, - ]; - - $_SERVER['HTTP_USER_AGENT'] = $requestUa; - - $this->assertEquals($expectedResult, $this->testInstance->isMatchingCurrentRequest()); - } - - public function getTestDataForIsMatchingCurrentRequest() - { - return [ - ['test ua', 'test ua', true], - ['nontest ua', 'test ua', false], - [null, 'test ua', false], - ]; - } - - public function test_isMatchingCurrentRequest_ReturnsFalse_IfUserInfoSessionVarDoesNotExist() - { - $_SERVER['HTTP_USER_AGENT'] = 'test-ua'; - - $this->assertEquals(false, $this->testInstance->isMatchingCurrentRequest()); - } - - public function test_isMatchingCurrentRequest_ReturnsFalse_IfRequestDetailsDoNotExist() - { - $_SESSION[SessionFingerprint::SESSION_INFO_SESSION_VAR_NAME] = [ - 'ua' => 'test-ua', - ]; - - $this->assertEquals(false, $this->testInstance->isMatchingCurrentRequest()); - } - public function test_getSessionStartTime_() { $_SESSION[SessionFingerprint::SESSION_INFO_SESSION_VAR_NAME] = [ diff --git a/tests/PHPUnit/Unit/Session/SessionInitializerTest.php b/tests/PHPUnit/Unit/Session/SessionInitializerTest.php index 7b3c2a206a..1275cb340a 100644 --- a/tests/PHPUnit/Unit/Session/SessionInitializerTest.php +++ b/tests/PHPUnit/Unit/Session/SessionInitializerTest.php @@ -40,7 +40,7 @@ class SessionInitializerTest extends \PHPUnit_Framework_TestCase public function test_initSession_Throws_IfAuthenticationFailed($rememberMe) { $sessionInitializer = new TestSessionInitializer(); - $sessionInitializer->initSession($this->makeMockAuth(AuthResult::SUCCESS), $rememberMe); + $sessionInitializer->initSession($this->makeMockAuth(AuthResult::SUCCESS)); } /** @@ -49,7 +49,7 @@ class SessionInitializerTest extends \PHPUnit_Framework_TestCase public function test_initSession_InitializesTheSessionCorrectly_IfAuthenticationSucceeds($rememberMe) { $sessionInitializer = new TestSessionInitializer(); - $sessionInitializer->initSession($this->makeMockAuth(AuthResult::SUCCESS), $rememberMe); + $sessionInitializer->initSession($this->makeMockAuth(AuthResult::SUCCESS)); $this->assertSessionCreatedCorrectly(); } @@ -72,7 +72,7 @@ class SessionInitializerTest extends \PHPUnit_Framework_TestCase $fingerprint = new SessionFingerprint(); $this->assertEquals('testlogin', $fingerprint->getUser()); $this->assertNotEmpty($fingerprint->getSessionStartTime()); - $this->assertEquals(['ts', 'ua'], array_keys($fingerprint->getUserInfo())); + $this->assertEquals(['ts'], array_keys($fingerprint->getUserInfo())); } } -- cgit v1.2.3