From 67a0e0138261f455af258d92d5c9a3f4caddc8af Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Mon, 6 May 2024 20:17:24 +0200 Subject: [PATCH] fix(dav): Try basic auth for ajax WebDAV requests Signed-off-by: Ferdinand Thiessen --- apps/dav/lib/Connector/Sabre/Auth.php | 12 +-- .../tests/unit/Connector/Sabre/AuthTest.php | 95 +++++++++++++------ 2 files changed, 73 insertions(+), 34 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/Auth.php b/apps/dav/lib/Connector/Sabre/Auth.php index 09495d27976..ee84da49d7d 100644 --- a/apps/dav/lib/Connector/Sabre/Auth.php +++ b/apps/dav/lib/Connector/Sabre/Auth.php @@ -195,18 +195,16 @@ class Auth extends AbstractBasic { } } - if (!$this->userSession->isLoggedIn() && in_array('XMLHttpRequest', explode(',', $request->getHeader('X-Requested-With') ?? ''))) { - // do not re-authenticate over ajax, use dummy auth name to prevent browser popup - $response->addHeader('WWW-Authenticate', 'DummyBasic realm="' . $this->realm . '"'); - $response->setStatus(401); - throw new \Sabre\DAV\Exception\NotAuthenticated('Cannot authenticate over ajax calls'); - } - $data = parent::check($request, $response); if ($data[0] === true) { $startPos = strrpos($data[1], '/') + 1; $user = $this->userSession->getUser()->getUID(); $data[1] = substr_replace($data[1], $user, $startPos); + } elseif (in_array('XMLHttpRequest', explode(',', $request->getHeader('X-Requested-With') ?? ''))) { + // For ajax requests use dummy auth name to prevent browser popup in case of invalid creditials + $response->addHeader('WWW-Authenticate', 'DummyBasic realm="' . $this->realm . '"'); + $response->setStatus(401); + throw new \Sabre\DAV\Exception\NotAuthenticated('Cannot authenticate over ajax calls'); } return $data; } diff --git a/apps/dav/tests/unit/Connector/Sabre/AuthTest.php b/apps/dav/tests/unit/Connector/Sabre/AuthTest.php index c9868debb43..dccb1edd0ae 100644 --- a/apps/dav/tests/unit/Connector/Sabre/AuthTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/AuthTest.php @@ -13,6 +13,7 @@ use OCP\IRequest; use OCP\ISession; use OCP\IUser; use OCP\Security\Bruteforce\IThrottler; +use PHPUnit\Framework\MockObject\MockObject; use Sabre\DAV\Server; use Sabre\HTTP\RequestInterface; use Sabre\HTTP\ResponseInterface; @@ -25,17 +26,17 @@ use Test\TestCase; * @group DB */ class AuthTest extends TestCase { - /** @var ISession */ + /** @var ISession&MockObject */ private $session; /** @var \OCA\DAV\Connector\Sabre\Auth */ private $auth; - /** @var Session */ + /** @var Session&MockObject */ private $userSession; - /** @var IRequest */ + /** @var IRequest&MockObject */ private $request; - /** @var Manager */ + /** @var Manager&MockObject */ private $twoFactorManager; - /** @var IThrottler */ + /** @var IThrottler&MockObject */ private $throttler; protected function setUp(): void { @@ -527,11 +528,11 @@ class AuthTest extends TestCase { $this->expectException(\Sabre\DAV\Exception\NotAuthenticated::class); $this->expectExceptionMessage('Cannot authenticate over ajax calls'); - /** @var \Sabre\HTTP\RequestInterface $httpRequest */ + /** @var \Sabre\HTTP\RequestInterface&MockObject $httpRequest */ $httpRequest = $this->getMockBuilder(RequestInterface::class) ->disableOriginalConstructor() ->getMock(); - /** @var \Sabre\HTTP\ResponseInterface $httpResponse */ + /** @var \Sabre\HTTP\ResponseInterface&MockObject $httpResponse */ $httpResponse = $this->getMockBuilder(ResponseInterface::class) ->disableOriginalConstructor() ->getMock(); @@ -540,10 +541,59 @@ class AuthTest extends TestCase { ->method('isLoggedIn') ->willReturn(false); $httpRequest - ->expects($this->once()) + ->expects($this->exactly(2)) ->method('getHeader') - ->with('X-Requested-With') - ->willReturn('XMLHttpRequest'); + ->willReturnMap([ + ['X-Requested-With', 'XMLHttpRequest'], + ['Authorization', null], + ]); + + $this->auth->check($httpRequest, $httpResponse); + } + + public function testAuthenticateWithBasicAuthenticateHeadersProvidedWithAjax(): void { + // No CSRF + $this->request + ->expects($this->once()) + ->method('passesCSRFCheck') + ->willReturn(false); + + /** @var \Sabre\HTTP\RequestInterface&MockObject $httpRequest */ + $httpRequest = $this->getMockBuilder(RequestInterface::class) + ->disableOriginalConstructor() + ->getMock(); + /** @var \Sabre\HTTP\ResponseInterface&MockObject $httpResponse */ + $httpResponse = $this->getMockBuilder(ResponseInterface::class) + ->disableOriginalConstructor() + ->getMock(); + $httpRequest + ->expects($this->any()) + ->method('getHeader') + ->willReturnMap([ + ['X-Requested-With', 'XMLHttpRequest'], + ['Authorization', 'basic dXNlcm5hbWU6cGFzc3dvcmQ='], + ]); + + $user = $this->getMockBuilder(IUser::class) + ->disableOriginalConstructor() + ->getMock(); + $user->expects($this->any()) + ->method('getUID') + ->willReturn('MyDavUser'); + $this->userSession + ->expects($this->any()) + ->method('isLoggedIn') + ->willReturn(false); + $this->userSession + ->expects($this->once()) + ->method('logClientIn') + ->with('username', 'password') + ->willReturn(true); + $this->userSession + ->expects($this->any()) + ->method('getUser') + ->willReturn($user); + $this->auth->check($httpRequest, $httpResponse); } @@ -597,16 +647,11 @@ class AuthTest extends TestCase { ->disableOriginalConstructor() ->getMock(); $server->httpRequest - ->expects($this->exactly(2)) + ->expects($this->once()) ->method('getHeader') - ->withConsecutive( - ['X-Requested-With'], - ['Authorization'], - ) - ->willReturnOnConsecutiveCalls( - null, - 'basic dXNlcm5hbWU6cGFzc3dvcmQ=', - ); + ->with('Authorization') + ->willReturn('basic dXNlcm5hbWU6cGFzc3dvcmQ='); + $server->httpResponse = $this->getMockBuilder(ResponseInterface::class) ->disableOriginalConstructor() ->getMock(); @@ -639,14 +684,10 @@ class AuthTest extends TestCase { $server->httpRequest ->expects($this->exactly(2)) ->method('getHeader') - ->withConsecutive( - ['X-Requested-With'], - ['Authorization'], - ) - ->willReturnOnConsecutiveCalls( - null, - 'basic dXNlcm5hbWU6cGFzc3dvcmQ=', - ); + ->willReturnMap([ + ['Authorization', 'basic dXNlcm5hbWU6cGFzc3dvcmQ='], + ['X-Requested-With', null], + ]); $server->httpResponse = $this->getMockBuilder(ResponseInterface::class) ->disableOriginalConstructor() ->getMock();