diff --git a/apps/dav/appinfo/v2/publicremote.php b/apps/dav/appinfo/v2/publicremote.php index fbb3ddd2cb3..e089fa7bb62 100644 --- a/apps/dav/appinfo/v2/publicremote.php +++ b/apps/dav/appinfo/v2/publicremote.php @@ -29,6 +29,7 @@ use OCP\IPreview; use OCP\IRequest; use OCP\ISession; use OCP\ITagManager; +use OCP\IURLGenerator; use OCP\IUserSession; use OCP\L10N\IFactory; use OCP\Security\Bruteforce\IThrottler; @@ -56,7 +57,8 @@ $authBackend = new PublicAuth( Server::get(IManager::class), $session, Server::get(IThrottler::class), - Server::get(LoggerInterface::class) + Server::get(LoggerInterface::class), + Server::get(IURLGenerator::class), ); $authPlugin = new \Sabre\DAV\Auth\Plugin($authBackend); diff --git a/apps/dav/lib/Connector/Sabre/PublicAuth.php b/apps/dav/lib/Connector/Sabre/PublicAuth.php index b5d9ce3db72..2ca1c25e2f6 100644 --- a/apps/dav/lib/Connector/Sabre/PublicAuth.php +++ b/apps/dav/lib/Connector/Sabre/PublicAuth.php @@ -14,6 +14,7 @@ namespace OCA\DAV\Connector\Sabre; use OCP\Defaults; use OCP\IRequest; use OCP\ISession; +use OCP\IURLGenerator; use OCP\Security\Bruteforce\IThrottler; use OCP\Security\Bruteforce\MaxDelayReached; use OCP\Share\Exceptions\ShareNotFound; @@ -23,6 +24,7 @@ use Psr\Log\LoggerInterface; use Sabre\DAV\Auth\Backend\AbstractBasic; use Sabre\DAV\Exception\NotAuthenticated; use Sabre\DAV\Exception\NotFound; +use Sabre\DAV\Exception\PreconditionFailed; use Sabre\DAV\Exception\ServiceUnavailable; use Sabre\HTTP; use Sabre\HTTP\RequestInterface; @@ -45,6 +47,7 @@ class PublicAuth extends AbstractBasic { private ISession $session, private IThrottler $throttler, private LoggerInterface $logger, + private IURLGenerator $urlGenerator, ) { // setup realm $defaults = new Defaults(); @@ -52,10 +55,6 @@ class PublicAuth extends AbstractBasic { } /** - * @param RequestInterface $request - * @param ResponseInterface $response - * - * @return array * @throws NotAuthenticated * @throws MaxDelayReached * @throws ServiceUnavailable @@ -64,6 +63,10 @@ class PublicAuth extends AbstractBasic { try { $this->throttler->sleepDelayOrThrowOnMax($this->request->getRemoteAddress(), self::BRUTEFORCE_ACTION); + if (count($_COOKIE) > 0 && !$this->request->passesStrictCookieCheck() && $this->getShare()->getPassword() !== null) { + throw new PreconditionFailed('Strict cookie check failed'); + } + $auth = new HTTP\Auth\Basic( $this->realm, $request, @@ -80,6 +83,15 @@ class PublicAuth extends AbstractBasic { } catch (NotAuthenticated|MaxDelayReached $e) { $this->throttler->registerAttempt(self::BRUTEFORCE_ACTION, $this->request->getRemoteAddress()); throw $e; + } catch (PreconditionFailed $e) { + $response->setHeader( + 'Location', + $this->urlGenerator->linkToRoute( + 'files_sharing.share.showShare', + [ 'token' => $this->getToken() ], + ), + ); + throw $e; } catch (\Exception $e) { $class = get_class($e); $msg = $e->getMessage(); @@ -90,7 +102,6 @@ class PublicAuth extends AbstractBasic { /** * Extract token from request url - * @return string * @throws NotFound */ private function getToken(): string { @@ -107,7 +118,7 @@ class PublicAuth extends AbstractBasic { /** * Check token validity - * @return array + * * @throws NotFound * @throws NotAuthenticated */ @@ -155,15 +166,13 @@ class PublicAuth extends AbstractBasic { protected function validateUserPass($username, $password) { $this->throttler->sleepDelayOrThrowOnMax($this->request->getRemoteAddress(), self::BRUTEFORCE_ACTION); - $token = $this->getToken(); try { - $share = $this->shareManager->getShareByToken($token); + $share = $this->getShare(); } catch (ShareNotFound $e) { $this->throttler->registerAttempt(self::BRUTEFORCE_ACTION, $this->request->getRemoteAddress()); return false; } - $this->share = $share; \OC_User::setIncognitoMode(true); // check if the share is password protected @@ -206,7 +215,13 @@ class PublicAuth extends AbstractBasic { } public function getShare(): IShare { - assert($this->share !== null); + $token = $this->getToken(); + + if ($this->share === null) { + $share = $this->shareManager->getShareByToken($token); + $this->share = $share; + } + return $this->share; } } diff --git a/apps/dav/tests/unit/Connector/Sabre/PublicAuthTest.php b/apps/dav/tests/unit/Connector/Sabre/PublicAuthTest.php index 00bddf2e69c..67e7f6af675 100644 --- a/apps/dav/tests/unit/Connector/Sabre/PublicAuthTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/PublicAuthTest.php @@ -10,10 +10,12 @@ namespace OCA\DAV\Tests\unit\Connector; use OCA\DAV\Connector\Sabre\PublicAuth; use OCP\IRequest; use OCP\ISession; +use OCP\IURLGenerator; use OCP\Security\Bruteforce\IThrottler; use OCP\Share\Exceptions\ShareNotFound; use OCP\Share\IManager; use OCP\Share\IShare; +use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; /** @@ -25,21 +27,15 @@ use Psr\Log\LoggerInterface; */ class PublicAuthTest extends \Test\TestCase { - /** @var ISession|MockObject */ - private $session; - /** @var IRequest|MockObject */ - private $request; - /** @var IManager|MockObject */ - private $shareManager; - /** @var PublicAuth */ - private $auth; - /** @var IThrottler|MockObject */ - private $throttler; - /** @var LoggerInterface|MockObject */ - private $logger; + private ISession&MockObject $session; + private IRequest&MockObject $request; + private IManager&MockObject $shareManager; + private PublicAuth $auth; + private IThrottler&MockObject $throttler; + private LoggerInterface&MockObject $logger; + private IURLGenerator&MockObject $urlGenerator; - /** @var string */ - private $oldUser; + private string $oldUser; protected function setUp(): void { parent::setUp(); @@ -49,6 +45,7 @@ class PublicAuthTest extends \Test\TestCase { $this->shareManager = $this->createMock(IManager::class); $this->throttler = $this->createMock(IThrottler::class); $this->logger = $this->createMock(LoggerInterface::class); + $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->auth = new PublicAuth( $this->request, @@ -56,6 +53,7 @@ class PublicAuthTest extends \Test\TestCase { $this->session, $this->throttler, $this->logger, + $this->urlGenerator, ); // Store current user @@ -137,7 +135,7 @@ class PublicAuthTest extends \Test\TestCase { $this->session->method('exists')->with('public_link_authenticated')->willReturn(true); $this->session->method('get')->with('public_link_authenticated')->willReturn('42'); - + $result = $this->invokePrivate($this->auth, 'checkToken'); $this->assertSame([true, 'principals/GX9HSGQrGE'], $result); } @@ -158,7 +156,7 @@ class PublicAuthTest extends \Test\TestCase { ->willReturn($share); $this->session->method('exists')->with('public_link_authenticated')->willReturn(false); - + $this->expectException(\Sabre\DAV\Exception\NotAuthenticated::class); $this->invokePrivate($this->auth, 'checkToken'); } @@ -180,7 +178,7 @@ class PublicAuthTest extends \Test\TestCase { $this->session->method('exists')->with('public_link_authenticated')->willReturn(false); $this->session->method('get')->with('public_link_authenticated')->willReturn('43'); - + $this->expectException(\Sabre\DAV\Exception\NotAuthenticated::class); $this->invokePrivate($this->auth, 'checkToken'); } diff --git a/lib/base.php b/lib/base.php index 45058db1600..2aa833df3b5 100644 --- a/lib/base.php +++ b/lib/base.php @@ -550,10 +550,10 @@ class OC { $processingScript = explode('/', $requestUri); $processingScript = $processingScript[count($processingScript) - 1]; - // index.php routes are handled in the middleware - // and cron.php does not need any authentication at all - if ($processingScript === 'index.php' - || $processingScript === 'cron.php') { + if ($processingScript === 'index.php' // index.php routes are handled in the middleware + || $processingScript === 'cron.php' // and cron.php does not need any authentication at all + || $processingScript === 'public.php' // For public.php, auth for password protected shares is done in the PublicAuth plugin + ) { return; }