fix(dav): Try basic auth for ajax WebDAV requests

Signed-off-by: Ferdinand Thiessen <[email protected]>
nextcloud · Aug 8, 2024 · e7547a9 · e7547a9
1 parent 598d501
commit e7547a9
Show file tree

Hide file tree

Showing 2 changed files with 72 additions and 33 deletions.
diff --git a/apps/dav/lib/Connector/Sabre/Auth.php b/apps/dav/lib/Connector/Sabre/Auth.php
@@ -221,18 +221,16 @@ private function auth(RequestInterface $request, ResponseInterface $response): a
 			}
 		}
 
-		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
@@ -35,6 +35,7 @@
 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;
@@ -47,17 +48,17 @@
  * @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 {
@@ -549,11 +550,11 @@ public function testAuthenticateNoBasicAuthenticateHeadersProvidedWithAjax(): vo
 		$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();
@@ -562,10 +563,59 @@ public function testAuthenticateNoBasicAuthenticateHeadersProvidedWithAjax(): vo
 			->method('isLoggedIn')
 			->willReturn(false);
 		$httpRequest
+			->expects($this->exactly(2))
+			->method('getHeader')
+			->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')
-			->with('X-Requested-With')
-			->willReturn('XMLHttpRequest');
+			->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);
 	}
 
@@ -619,16 +669,11 @@ public function testAuthenticateValidCredentials(): void {
 			->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();
@@ -661,14 +706,10 @@ public function testAuthenticateInvalidCredentials(): void {
 		$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();