Sessions with more security (#12208)

* Modifying "cookie authentication" to be more secure.

Instead of authenticating by token auth if it exists in the cookie, validate an existing session. If the session
has the user name stored as a session var, it has been authenticated. If the request has the same IP address and
user agent as the request that created the session, the request is from the user that created the session. If
both of these are true, then the session is valid, and we don't need a token auth to authenticate.

If the session is deleted before the Piwik auth cookie expires (due to garbage collection), we attempt to
re-authenticate using a secure hash of the token auth. We don't do this on every request since password_verify()
will, at BEST, add 3.5ms to every request.

* Invalidate existing sessions after user password change.

Invalidation is accomplished w/o having to individually touch sessions by:

1. Using the password hash as the piwik_auth key secret, instead of the token auth. So when a password changes, existing piwik_auth keys are no longer valid. This affects session re-authentication.
2. Saving the session start time & the last time a user's password was modified, and checking that the session start time is always newer than the password modification time.

* Set session.gc_maxlifetime to login_cookie_expire time so session data does not disappear, remove session re-auth functionality & tie cookie hash to password modified time instead of password hash to retain automatic session invalidation on password change.

* In SessionInitializer, clear other cookie values so previously stored token auths will be removed.

* Make sure anonymous user is still default user whan authenticating.

* fixing test failures

* Remove hash checking in piwik_auth cookie.

piwik_auth cookie still required since it's presence indicates we should use SessionAuth instead of the normal authentication mechanism. Since there's always a session, even if you're not logged in, PIWIK_SESSID can't be used by itself to determine this.

* Make sure session auth doesnt break in edge case where ts_password_modified column does not exist.

* Clarify session destruction/invalidation logic in SessionAuth.

* Make UsersManagerTest slightly more comprehensive.

* Use Date::now()->getTimestampUTC() instead of time() in SessionFingerprint::initialize().

* Check getUser returns correct user info in SessionAuth for sanity.

* Add SessionInitializer::getAuthCookie() back since it is @api.

* Remove IP address from session auth info + check.

* Refactor session start changes so it is started in one place only.

* Remove SessionAuthCookieFactory & deprecate auth cookie INI config vars (still needed for SessionInitializer deprectaed method).

* Make sure user can still login if ts_password_modified column is not present in database.

* Rename ts_password_modified Update class.

* Update comment in SessionAuth to include why Piwik tries to create another session.

* Restore 3.x-dev SessionInitializer for BC (deprecated), move new SessionInitializer to core, add tests for both SessionInitializers.

* Change update to 3.5 version.

* Make sure normal auth implementation is used if sessionauth fails so anonymous user can be logged in.

* On logout clear session fingerprint so same session cannot be used to login.

* Change update name + bump version, and make sure Session::rememberMe() is called before session is started (otherwise it has no effect).

* Fixing tests.

* apply review fixes

* remove test

12 files changed, 42 insertions, 3 deletions

diff --git a/plugins/UsersManager/Controller.php b/plugins/UsersManager/Controller.php
index 0be04abda4..6b59d05437 100644
--- a/plugins/UsersManager/Controller.php
+++ b/plugins/UsersManager/Controller.php
@@ -19,7 +19,6 @@ use Piwik\Piwik;
 use Piwik\Plugin\ControllerAdmin;
 use Piwik\Plugins\LanguagesManager\API as APILanguagesManager;
 use Piwik\Plugins\LanguagesManager\LanguagesManager;
-use Piwik\Plugins\Login\SessionInitializer;
 use Piwik\Plugins\UsersManager\API as APIUsersManager;
 use Piwik\SettingsPiwik;
 use Piwik\Site;
@@ -27,6 +26,7 @@ use Piwik\Tracker\IgnoreCookie;
 use Piwik\Translation\Translator;
 use Piwik\Url;
 use Piwik\View;
+use Piwik\Session\SessionInitializer;
 
 class Controller extends ControllerAdmin
 {
diff --git a/plugins/UsersManager/Model.php b/plugins/UsersManager/Model.php
index d33c808c48..f986333bc0 100644
--- a/plugins/UsersManager/Model.php
+++ b/plugins/UsersManager/Model.php
@@ -10,6 +10,7 @@ namespace Piwik\Plugins\UsersManager;
 
 use Piwik\Auth\Password;
 use Piwik\Common;
+use Piwik\Date;
 use Piwik\Db;
 use Piwik\Piwik;
 
@@ -199,7 +200,8 @@ class Model
             'email'            => $email,
             'token_auth'       => $tokenAuth,
             'date_registered'  => $dateRegistered,
-            'superuser_access' => 0
+            'superuser_access' => 0,
+            'ts_password_modified' => Date::now()->getDatetime(),
         );
 
         $db = $this->getDb();
@@ -223,6 +225,11 @@ class Model
             $bind[] = $val;
         }
 
+        if (!empty($fields['password'])) {
+            $set[] = "ts_password_modified = ?";
+            $bind[] = Date::now()->getDatetime();
+        }
+
         $bind[] = $userLogin;
 
         $db = $this->getDb();
diff --git a/plugins/UsersManager/tests/Integration/UsersManagerTest.php b/plugins/UsersManager/tests/Integration/UsersManagerTest.php
index 790387ce8b..8bc80b436a 100644
--- a/plugins/UsersManager/tests/Integration/UsersManagerTest.php
+++ b/plugins/UsersManager/tests/Integration/UsersManagerTest.php
@@ -26,6 +26,8 @@ use Exception;
  */
 class UsersManagerTest extends IntegrationTestCase
 {
+    const DATETIME_REGEX = '/\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}/';
+
     /**
      * @var API
      */
@@ -74,8 +76,20 @@ class UsersManagerTest extends IntegrationTestCase
         if (is_null($newAlias)) {
             $newAlias = $user['alias'];
         }
+
         $userAfter = $this->api->getUser($user["login"]);
+
+        $this->assertArrayHasKey('date_registered', $userAfter);
+        $this->assertRegExp(self::DATETIME_REGEX, $userAfter['date_registered']);
+
+        $this->assertArrayHasKey('ts_password_modified', $userAfter);
+        $this->assertRegExp(self::DATETIME_REGEX, $userAfter['date_registered']);
+
+        $this->assertArrayHasKey('password', $userAfter);
+        $this->assertNotEmpty($userAfter['password']);
+
         unset($userAfter['date_registered']);
+        unset($userAfter['ts_password_modified']);
         unset($userAfter['password']);
 
         // implicitly checks password!
@@ -425,6 +439,7 @@ class UsersManagerTest extends IntegrationTestCase
             unset($user['password']);
             unset($user['token_auth']);
             unset($user['date_registered']);
+            unset($user['ts_password_modified']);
         }
         return $users;
     }
diff --git a/plugins/UsersManager/tests/System/ApiTest.php b/plugins/UsersManager/tests/System/ApiTest.php
index 4192b62d0c..8c1baaec80 100644
--- a/plugins/UsersManager/tests/System/ApiTest.php
+++ b/plugins/UsersManager/tests/System/ApiTest.php
@@ -38,7 +38,7 @@ class ApiTest extends SystemTestCase
         // login1 = super user, login2 = some admin access, login4 = only view access
         foreach ($logins as $login => $appendix) {
             $params['token_auth'] = self::$fixture->users[$login]['token'];
-            $xmlFieldsToRemove    = array('date_registered', 'password', 'token_auth');
+            $xmlFieldsToRemove    = array('date_registered', 'password', 'token_auth', 'ts_password_modified');
 
             $this->runAnyApiTest($api, $apiId . '_' . $appendix, $params, array('xmlFieldsToRemove' => $xmlFieldsToRemove));
         }
diff --git a/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login1_when_superuseraccess.xml b/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login1_when_superuseraccess.xml
index e233776f69..4b1a43a0ee 100644
--- a/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login1_when_superuseraccess.xml
+++ b/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login1_when_superuseraccess.xml
@@ -7,5 +7,6 @@
 		<email>login1@example.com</email>
 		<superuser_access>1</superuser_access>
 		
+		
 	</row>
 </result>
\ No newline at end of file
diff --git a/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login2_when_adminaccess.xml b/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login2_when_adminaccess.xml
index 7b9a2b37ca..b394489b94 100644
--- a/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login2_when_adminaccess.xml
+++ b/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login2_when_adminaccess.xml
@@ -7,5 +7,6 @@
 		<email>login2@example.com</email>
 		<superuser_access>0</superuser_access>
 		
+		
 	</row>
 </result>
\ No newline at end of file
diff --git a/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login2_when_superuseraccess.xml b/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login2_when_superuseraccess.xml
index 7b9a2b37ca..b394489b94 100644
--- a/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login2_when_superuseraccess.xml
+++ b/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login2_when_superuseraccess.xml
@@ -7,5 +7,6 @@
 		<email>login2@example.com</email>
 		<superuser_access>0</superuser_access>
 		
+		
 	</row>
 </result>
\ No newline at end of file
diff --git a/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login4_when_superuseraccess.xml b/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login4_when_superuseraccess.xml
index 28d1197732..caa6b7267e 100644
--- a/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login4_when_superuseraccess.xml
+++ b/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login4_when_superuseraccess.xml
@@ -7,5 +7,6 @@
 		<email>login4@example.com</email>
 		<superuser_access>0</superuser_access>
 		
+		
 	</row>
 </result>
\ No newline at end of file
diff --git a/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login4_when_viewaccess.xml b/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login4_when_viewaccess.xml
index 28d1197732..caa6b7267e 100644
--- a/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login4_when_viewaccess.xml
+++ b/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login4_when_viewaccess.xml
@@ -7,5 +7,6 @@
 		<email>login4@example.com</email>
 		<superuser_access>0</superuser_access>
 		
+		
 	</row>
 </result>
\ No newline at end of file
diff --git a/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login6_when_superuseraccess.xml b/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login6_when_superuseraccess.xml
index ebf69209da..beeca3f033 100644
--- a/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login6_when_superuseraccess.xml
+++ b/plugins/UsersManager/tests/System/expected/test___UsersManager.getUser_login6_when_superuseraccess.xml
@@ -7,5 +7,6 @@
 		<email>login6@example.com</email>
 		<superuser_access>0</superuser_access>
 		
+		
 	</row>
 </result>
\ No newline at end of file
diff --git a/plugins/UsersManager/tests/System/expected/test___UsersManager.getUsersWithSiteAccess_3_admin_when_superuseraccess.xml b/plugins/UsersManager/tests/System/expected/test___UsersManager.getUsersWithSiteAccess_3_admin_when_superuseraccess.xml
index aa93d367ad..df237acece 100644
--- a/plugins/UsersManager/tests/System/expected/test___UsersManager.getUsersWithSiteAccess_3_admin_when_superuseraccess.xml
+++ b/plugins/UsersManager/tests/System/expected/test___UsersManager.getUsersWithSiteAccess_3_admin_when_superuseraccess.xml
@@ -7,6 +7,7 @@
 		<email>login5@example.com</email>
 		<superuser_access>0</superuser_access>
 		
+		
 	</row>
 	<row>
 		<login>login6</login>
@@ -15,5 +16,6 @@
 		<email>login6@example.com</email>
 		<superuser_access>0</superuser_access>
 		
+		
 	</row>
 </result>
\ No newline at end of file
diff --git a/plugins/UsersManager/tests/System/expected/test___UsersManager.getUsers__when_superuseraccess.xml b/plugins/UsersManager/tests/System/expected/test___UsersManager.getUsers__when_superuseraccess.xml
index e8eb10f4b1..35351a9999 100644
--- a/plugins/UsersManager/tests/System/expected/test___UsersManager.getUsers__when_superuseraccess.xml
+++ b/plugins/UsersManager/tests/System/expected/test___UsersManager.getUsers__when_superuseraccess.xml
@@ -7,6 +7,7 @@
 		<email>login1@example.com</email>
 		<superuser_access>1</superuser_access>
 		
+		
 	</row>
 	<row>
 		<login>login2</login>
@@ -15,6 +16,7 @@
 		<email>login2@example.com</email>
 		<superuser_access>0</superuser_access>
 		
+		
 	</row>
 	<row>
 		<login>login3</login>
@@ -23,6 +25,7 @@
 		<email>login3@example.com</email>
 		<superuser_access>0</superuser_access>
 		
+		
 	</row>
 	<row>
 		<login>login4</login>
@@ -31,6 +34,7 @@
 		<email>login4@example.com</email>
 		<superuser_access>0</superuser_access>
 		
+		
 	</row>
 	<row>
 		<login>login5</login>
@@ -39,6 +43,7 @@
 		<email>login5@example.com</email>
 		<superuser_access>0</superuser_access>
 		
+		
 	</row>
 	<row>
 		<login>login6</login>
@@ -47,6 +52,7 @@
 		<email>login6@example.com</email>
 		<superuser_access>0</superuser_access>
 		
+		
 	</row>
 	<row>
 		<login>login7</login>
@@ -55,6 +61,7 @@
 		<email>login7@example.com</email>
 		<superuser_access>0</superuser_access>
 		
+		
 	</row>
 	<row>
 		<login>login8</login>
@@ -63,6 +70,7 @@
 		<email>login8@example.com</email>
 		<superuser_access>0</superuser_access>
 		
+		
 	</row>
 	<row>
 		<login>superUserLogin</login>
@@ -71,5 +79,6 @@
 		<email>hello@example.org</email>
 		<superuser_access>1</superuser_access>
 		
+		
 	</row>
 </result>
\ No newline at end of file