diff options
| -rw-r--r-- | apps/dav/lib/connector/sabre/auth.php | 82 | ||||
| -rw-r--r-- | apps/dav/tests/unit/connector/sabre/auth.php | 193 | ||||
| -rw-r--r-- | lib/private/appframework/http/request.php | 3 |
3 files changed, 242 insertions, 36 deletions
diff --git a/apps/dav/lib/connector/sabre/auth.php b/apps/dav/lib/connector/sabre/auth.php index 8c09c9fdc1c..b63efa3a1ba 100644 --- a/apps/dav/lib/connector/sabre/auth.php +++ b/apps/dav/lib/connector/sabre/auth.php @@ -30,6 +30,7 @@ namespace OCA\DAV\Connector\Sabre; use Exception; +use OC\AppFramework\Http\Request; use OCP\IRequest; use OCP\ISession; use OCP\IUserSession; @@ -48,6 +49,8 @@ class Auth extends AbstractBasic { private $userSession; /** @var IRequest */ private $request; + /** @var string */ + private $currentUser; /** * @param ISession $session @@ -130,7 +133,46 @@ class Auth extends AbstractBasic { $msg = $e->getMessage(); throw new ServiceUnavailable("$class: $msg"); } - } + } + + /** + * Checks whether a CSRF check is required on the request + * + * @return bool + */ + private function requiresCSRFCheck() { + // GET requires no check at all + if($this->request->getMethod() === 'GET') { + return false; + } + + // Official ownCloud clients require no checks + if($this->request->isUserAgent([ + Request::USER_AGENT_OWNCLOUD_DESKTOP, + Request::USER_AGENT_OWNCLOUD_ANDROID, + Request::USER_AGENT_OWNCLOUD_IOS, + ])) { + return false; + } + + // If not logged-in no check is required + if(!$this->userSession->isLoggedIn()) { + return false; + } + + // POST always requires a check + if($this->request->getMethod() === 'POST') { + return true; + } + + // If logged-in AND DAV authenticated no check is required + if($this->userSession->isLoggedIn() && + $this->isDavAuthenticated($this->userSession->getUser()->getUID())) { + return false; + } + + return true; + } /** * @param RequestInterface $request @@ -139,27 +181,33 @@ class Auth extends AbstractBasic { * @throws NotAuthenticated */ private function auth(RequestInterface $request, ResponseInterface $response) { - // If request is not GET and not authenticated via WebDAV a requesttoken is required - if($this->userSession->isLoggedIn() && - $this->request->getMethod() !== 'GET' && - !$this->isDavAuthenticated($this->userSession->getUser()->getUID())) { - if(!$this->request->passesCSRFCheck()) { + $forcedLogout = false; + if(!$this->request->passesCSRFCheck() && + $this->requiresCSRFCheck()) { + // In case of a fail with POST we need to recheck the credentials + if($this->request->getMethod() === 'POST') { + $forcedLogout = true; + } else { $response->setStatus(401); throw new \Sabre\DAV\Exception\NotAuthenticated('CSRF check not passed.'); } } - if (\OC_User::handleApacheAuth() || - //Fix for broken webdav clients - ($this->userSession->isLoggedIn() && is_null($this->session->get(self::DAV_AUTHENTICATED))) || - //Well behaved clients that only send the cookie are allowed - ($this->userSession->isLoggedIn() && $this->session->get(self::DAV_AUTHENTICATED) === $this->userSession->getUser()->getUID() && $request->getHeader('Authorization') === null) - ) { - $user = $this->userSession->getUser()->getUID(); - \OC_Util::setupFS($user); - $this->currentUser = $user; - $this->session->close(); - return [true, $this->principalPrefix . $user]; + if($forcedLogout) { + $this->userSession->logout(); + } else { + if (\OC_User::handleApacheAuth() || + //Fix for broken webdav clients + ($this->userSession->isLoggedIn() && is_null($this->session->get(self::DAV_AUTHENTICATED))) || + //Well behaved clients that only send the cookie are allowed + ($this->userSession->isLoggedIn() && $this->session->get(self::DAV_AUTHENTICATED) === $this->userSession->getUser()->getUID() && $request->getHeader('Authorization') === null) + ) { + $user = $this->userSession->getUser()->getUID(); + \OC_Util::setupFS($user); + $this->currentUser = $user; + $this->session->close(); + return [true, $this->principalPrefix . $user]; + } } if (!$this->userSession->isLoggedIn() && in_array('XMLHttpRequest', explode(',', $request->getHeader('X-Requested-With')))) { diff --git a/apps/dav/tests/unit/connector/sabre/auth.php b/apps/dav/tests/unit/connector/sabre/auth.php index edb4073bdcb..b81a5e003b5 100644 --- a/apps/dav/tests/unit/connector/sabre/auth.php +++ b/apps/dav/tests/unit/connector/sabre/auth.php @@ -198,10 +198,7 @@ class Auth extends TestCase { $this->assertFalse($this->invokePrivate($this->auth, 'validateUserPass', ['MyTestUser', 'MyTestPassword'])); } - /** - * @expectedException \Sabre\DAV\Exception\NotAuthenticated - * @expectedExceptionMessage CSRF check not passed. - */ + public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenForNonGet() { $request = $this->getMockBuilder('Sabre\HTTP\RequestInterface') ->disableOriginalConstructor() @@ -210,30 +207,42 @@ class Auth extends TestCase { ->disableOriginalConstructor() ->getMock(); $this->userSession - ->expects($this->once()) + ->expects($this->any()) ->method('isLoggedIn') ->will($this->returnValue(true)); + $this->request + ->expects($this->any()) + ->method('getMethod') + ->willReturn('POST'); $this->session - ->expects($this->once()) + ->expects($this->any()) ->method('get') ->with('AUTHENTICATED_TO_DAV_BACKEND') ->will($this->returnValue(null)); $user = $this->getMockBuilder('\OCP\IUser') ->disableOriginalConstructor() ->getMock(); - $user->expects($this->once()) + $user->expects($this->any()) ->method('getUID') ->will($this->returnValue('MyWrongDavUser')); $this->userSession - ->expects($this->once()) + ->expects($this->any()) ->method('getUser') ->will($this->returnValue($user)); + $this->request + ->expects($this->once()) + ->method('passesCSRFCheck') + ->willReturn(false); + $expectedResponse = [ + false, + "No 'Authorization: Basic' header found. Either the client didn't send one, or the server is mis-configured", + ]; $response = $this->auth->check($request, $response); - $this->assertEquals([true, 'principals/users/MyWrongDavUser'], $response); + $this->assertSame($expectedResponse, $response); } - public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenForGet() { + public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenAndCorrectlyDavAuthenticated() { $request = $this->getMockBuilder('Sabre\HTTP\RequestInterface') ->disableOriginalConstructor() ->getMock(); @@ -241,26 +250,169 @@ class Auth extends TestCase { ->disableOriginalConstructor() ->getMock(); $this->userSession - ->expects($this->exactly(2)) + ->expects($this->any()) ->method('isLoggedIn') - ->will($this->returnValue(true)); + ->willReturn(true); + $this->request + ->expects($this->any()) + ->method('getMethod') + ->willReturn('PROPFIND'); + $this->request + ->expects($this->any()) + ->method('isUserAgent') + ->with([ + '/^Mozilla\/5\.0 \([A-Za-z ]+\) (mirall|csyncoC)\/.*$/', + '/^Mozilla\/5\.0 \(Android\) ownCloud\-android.*$/', + '/^Mozilla\/5\.0 \(iOS\) ownCloud\-iOS.*$/', + ]) + ->willReturn(false); $this->session + ->expects($this->any()) + ->method('get') + ->with('AUTHENTICATED_TO_DAV_BACKEND') + ->will($this->returnValue('LoggedInUser')); + $user = $this->getMockBuilder('\OCP\IUser') + ->disableOriginalConstructor() + ->getMock(); + $user->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('LoggedInUser')); + $this->userSession + ->expects($this->any()) + ->method('getUser') + ->will($this->returnValue($user)); + $this->request ->expects($this->once()) + ->method('passesCSRFCheck') + ->willReturn(false); + $this->auth->check($request, $response); + } + + /** + * @expectedException \Sabre\DAV\Exception\NotAuthenticated + * @expectedExceptionMessage CSRF check not passed. + */ + public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenAndIncorrectlyDavAuthenticated() { + $request = $this->getMockBuilder('Sabre\HTTP\RequestInterface') + ->disableOriginalConstructor() + ->getMock(); + $response = $this->getMockBuilder('Sabre\HTTP\ResponseInterface') + ->disableOriginalConstructor() + ->getMock(); + $this->userSession + ->expects($this->any()) + ->method('isLoggedIn') + ->willReturn(true); + $this->request + ->expects($this->any()) + ->method('getMethod') + ->willReturn('PROPFIND'); + $this->request + ->expects($this->any()) + ->method('isUserAgent') + ->with([ + '/^Mozilla\/5\.0 \([A-Za-z ]+\) (mirall|csyncoC)\/.*$/', + '/^Mozilla\/5\.0 \(Android\) ownCloud\-android.*$/', + '/^Mozilla\/5\.0 \(iOS\) ownCloud\-iOS.*$/', + ]) + ->willReturn(false); + $this->session + ->expects($this->any()) + ->method('get') + ->with('AUTHENTICATED_TO_DAV_BACKEND') + ->will($this->returnValue('AnotherUser')); + $user = $this->getMockBuilder('\OCP\IUser') + ->disableOriginalConstructor() + ->getMock(); + $user->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('LoggedInUser')); + $this->userSession + ->expects($this->any()) + ->method('getUser') + ->will($this->returnValue($user)); + $this->request + ->expects($this->once()) + ->method('passesCSRFCheck') + ->willReturn(false); + $this->auth->check($request, $response); + } + + public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenForNonGetAndDesktopClient() { + $request = $this->getMockBuilder('Sabre\HTTP\RequestInterface') + ->disableOriginalConstructor() + ->getMock(); + $response = $this->getMockBuilder('Sabre\HTTP\ResponseInterface') + ->disableOriginalConstructor() + ->getMock(); + $this->userSession + ->expects($this->any()) + ->method('isLoggedIn') + ->will($this->returnValue(true)); + $this->request + ->expects($this->any()) + ->method('getMethod') + ->willReturn('POST'); + $this->request + ->expects($this->any()) + ->method('isUserAgent') + ->with([ + '/^Mozilla\/5\.0 \([A-Za-z ]+\) (mirall|csyncoC)\/.*$/', + '/^Mozilla\/5\.0 \(Android\) ownCloud\-android.*$/', + '/^Mozilla\/5\.0 \(iOS\) ownCloud\-iOS.*$/', + ]) + ->willReturn(true); + $this->session + ->expects($this->any()) ->method('get') ->with('AUTHENTICATED_TO_DAV_BACKEND') ->will($this->returnValue(null)); $user = $this->getMockBuilder('\OCP\IUser') ->disableOriginalConstructor() ->getMock(); - $user->expects($this->once()) + $user->expects($this->any()) ->method('getUID') ->will($this->returnValue('MyWrongDavUser')); $this->userSession - ->expects($this->once()) + ->expects($this->any()) ->method('getUser') ->will($this->returnValue($user)); $this->request ->expects($this->once()) + ->method('passesCSRFCheck') + ->willReturn(false); + + $this->auth->check($request, $response); + } + + public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenForGet() { + $request = $this->getMockBuilder('Sabre\HTTP\RequestInterface') + ->disableOriginalConstructor() + ->getMock(); + $response = $this->getMockBuilder('Sabre\HTTP\ResponseInterface') + ->disableOriginalConstructor() + ->getMock(); + $this->userSession + ->expects($this->any()) + ->method('isLoggedIn') + ->will($this->returnValue(true)); + $this->session + ->expects($this->any()) + ->method('get') + ->with('AUTHENTICATED_TO_DAV_BACKEND') + ->will($this->returnValue(null)); + $user = $this->getMockBuilder('\OCP\IUser') + ->disableOriginalConstructor() + ->getMock(); + $user->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('MyWrongDavUser')); + $this->userSession + ->expects($this->any()) + ->method('getUser') + ->will($this->returnValue($user)); + $this->request + ->expects($this->any()) ->method('getMethod') ->willReturn('GET'); @@ -268,7 +420,6 @@ class Auth extends TestCase { $this->assertEquals([true, 'principals/users/MyWrongDavUser'], $response); } - public function testAuthenticateAlreadyLoggedInWithCsrfTokenForGet() { $request = $this->getMockBuilder('Sabre\HTTP\RequestInterface') ->disableOriginalConstructor() @@ -277,22 +428,22 @@ class Auth extends TestCase { ->disableOriginalConstructor() ->getMock(); $this->userSession - ->expects($this->exactly(2)) + ->expects($this->any()) ->method('isLoggedIn') ->will($this->returnValue(true)); $this->session - ->expects($this->exactly(2)) + ->expects($this->any()) ->method('get') ->with('AUTHENTICATED_TO_DAV_BACKEND') ->will($this->returnValue(null)); $user = $this->getMockBuilder('\OCP\IUser') ->disableOriginalConstructor() ->getMock(); - $user->expects($this->exactly(2)) + $user->expects($this->any()) ->method('getUID') ->will($this->returnValue('MyWrongDavUser')); $this->userSession - ->expects($this->exactly(2)) + ->expects($this->any()) ->method('getUser') ->will($this->returnValue($user)); $this->request @@ -368,6 +519,10 @@ class Auth extends TestCase { ->method('get') ->with('AUTHENTICATED_TO_DAV_BACKEND') ->will($this->returnValue('MyTestUser')); + $this->request + ->expects($this->once()) + ->method('getMethod') + ->willReturn('GET'); $httpRequest ->expects($this->atLeastOnce()) ->method('getHeader') diff --git a/lib/private/appframework/http/request.php b/lib/private/appframework/http/request.php index 2dc636fcff2..f4cbb6384ba 100644 --- a/lib/private/appframework/http/request.php +++ b/lib/private/appframework/http/request.php @@ -59,6 +59,9 @@ class Request implements \ArrayAccess, \Countable, IRequest { // Android Chrome user agent: https://developers.google.com/chrome/mobile/docs/user-agent const USER_AGENT_ANDROID_MOBILE_CHROME = '#Android.*Chrome/[.0-9]*#'; const USER_AGENT_FREEBOX = '#^Mozilla/5\.0$#'; + const USER_AGENT_OWNCLOUD_IOS = '/^Mozilla\/5\.0 \(iOS\) ownCloud\-iOS.*$/'; + const USER_AGENT_OWNCLOUD_ANDROID = '/^Mozilla\/5\.0 \(Android\) ownCloud\-android.*$/'; + const USER_AGENT_OWNCLOUD_DESKTOP = '/^Mozilla\/5\.0 \([A-Za-z ]+\) (mirall|csyncoC)\/.*$/'; const REGEX_LOCALHOST = '/^(127\.0\.0\.1|localhost)$/'; protected $inputStream; |