From dfbc405a45f42acd1202d5f0649619388adbfb83 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Fri, 16 Jan 2015 14:31:02 +0100 Subject: [PATCH 1/2] Prioritise Basic Auth header over Cookie There are a lot of clients that support multiple WebDAV accounts in the same application. However, they resent all the cookies they received from one of the accounts also to the other one. In the case of ownCloud this means that we will always show the user from the session and not the user that is specified in the basic authentication header. This patch adds a workaround the following way: 1. If the user authenticates via the Sabre Auth Connector add a hint to the session that this was authorized via Basic Auth (this is to prevent logout CSRF) 2. If the request contains this hint and the username specified in the basic auth header differs from the one in the session relogin the user using basic auth Fixes https://github.com/owncloud/core/issues/11400 and https://github.com/owncloud/core/issues/13245 and probably some other issues as well. This requires proper testing also considering LDAP / Shibboleth and whatever instances. --- apps/files_encryption/tests/webdav.php | 13 ++++++-- lib/private/connector/sabre/auth.php | 43 +++++++++++++++++++------- 2 files changed, 43 insertions(+), 13 deletions(-) diff --git a/apps/files_encryption/tests/webdav.php b/apps/files_encryption/tests/webdav.php index 83f4c0a77d..bdbc9d7ef0 100755 --- a/apps/files_encryption/tests/webdav.php +++ b/apps/files_encryption/tests/webdav.php @@ -206,12 +206,17 @@ class Webdav extends TestCase { * handle webdav request * * @param bool $body - * * @note this init procedure is copied from /apps/files/appinfo/remote.php */ function handleWebdavRequest($body = false) { // Backends - $authBackend = new \OC_Connector_Sabre_Auth(); + $authBackend = $this->getMockBuilder('OC_Connector_Sabre_Auth') + ->setMethods(['validateUserPass']) + ->getMock(); + $authBackend->expects($this->any()) + ->method('validateUserPass') + ->will($this->returnValue(true)); + $lockBackend = new \OC_Connector_Sabre_Locks(); $requestBackend = new \OC_Connector_Sabre_Request(); @@ -236,6 +241,10 @@ class Webdav extends TestCase { $server->addPlugin(new \OC_Connector_Sabre_MaintenancePlugin()); $server->debugExceptions = true; + // Totally ugly hack to setup the FS + \OC::$server->getUserSession()->login($this->userId, $this->userId); + \OC_Util::setupFS($this->userId); + // And off we go! if ($body) { $server->httpRequest->setBody($body); diff --git a/lib/private/connector/sabre/auth.php b/lib/private/connector/sabre/auth.php index 6e1baca933..34ccd64444 100644 --- a/lib/private/connector/sabre/auth.php +++ b/lib/private/connector/sabre/auth.php @@ -22,25 +22,49 @@ */ class OC_Connector_Sabre_Auth extends \Sabre\DAV\Auth\Backend\AbstractBasic { + const DAV_AUTHENTICATED = 'AUTHENTICATED_TO_DAV_BACKEND'; + + /** + * Whether the user has initially authenticated via DAV + * + * This is required for WebDAV clients that resent the cookies even when the + * account was changed. + * + * @see https://github.com/owncloud/core/issues/13245 + * + * @param string $username + * @return bool + */ + protected function isDavAuthenticated($username) { + return !is_null(\OC::$server->getSession()->get(self::DAV_AUTHENTICATED)) && + \OC::$server->getSession()->get(self::DAV_AUTHENTICATED) === $username; + } + /** * Validates a username and password * * This method should return true or false depending on if login * succeeded. * + * @param string $username + * @param string $password * @return bool */ protected function validateUserPass($username, $password) { - if (OC_User::isLoggedIn()) { + if (OC_User::isLoggedIn() && + $this->isDavAuthenticated($username) + ) { OC_Util::setupFS(OC_User::getUser()); return true; } else { - OC_Util::setUpFS();//login hooks may need early access to the filesystem + OC_Util::setUpFS(); //login hooks may need early access to the filesystem if(OC_User::login($username, $password)) { OC_Util::setUpFS(OC_User::getUser()); + \OC::$server->getSession()->set(self::DAV_AUTHENTICATED, $username); + \OC::$server->getSession()->close(); return true; - } - else{ + } else { + \OC::$server->getSession()->close(); return false; } } @@ -55,10 +79,10 @@ class OC_Connector_Sabre_Auth extends \Sabre\DAV\Auth\Backend\AbstractBasic { */ public function getCurrentUser() { $user = OC_User::getUser(); - if(!$user) { - return null; + if($user && $this->isDavAuthenticated($user)) { + return $user; } - return $user; + return null; } /** @@ -77,9 +101,6 @@ class OC_Connector_Sabre_Auth extends \Sabre\DAV\Auth\Backend\AbstractBasic { $result = $this->auth($server, $realm); - // close the session - right after authentication there is not need to write to the session any more - \OC::$server->getSession()->close(); - return $result; } @@ -89,7 +110,7 @@ class OC_Connector_Sabre_Auth extends \Sabre\DAV\Auth\Backend\AbstractBasic { * @return bool */ private function auth(\Sabre\DAV\Server $server, $realm) { - if (OC_User::handleApacheAuth() || OC_User::isLoggedIn()) { + if (OC_User::handleApacheAuth()) { $user = OC_User::getUser(); OC_Util::setupFS($user); $this->currentUser = $user; From 730460c9fa758ae2968299d9dcfa82c23e83f5bb Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Mon, 19 Jan 2015 16:25:44 +0100 Subject: [PATCH 2/2] Close session properly --- lib/private/connector/sabre/auth.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/private/connector/sabre/auth.php b/lib/private/connector/sabre/auth.php index 34ccd64444..f40706b73e 100644 --- a/lib/private/connector/sabre/auth.php +++ b/lib/private/connector/sabre/auth.php @@ -55,6 +55,7 @@ class OC_Connector_Sabre_Auth extends \Sabre\DAV\Auth\Backend\AbstractBasic { $this->isDavAuthenticated($username) ) { OC_Util::setupFS(OC_User::getUser()); + \OC::$server->getSession()->close(); return true; } else { OC_Util::setUpFS(); //login hooks may need early access to the filesystem