diff options
author | Carl Schwan <carl@carlschwan.eu> | 2022-07-15 18:11:54 +0300 |
---|---|---|
committer | Carl Schwan <carl@carlschwan.eu> | 2022-07-31 20:37:59 +0300 |
commit | 7b723813cef60e744ab14ab418c82e5ec67a9f2e (patch) | |
tree | eda4a044c9defc883c35d98e1ba5a9e261a3ecdf /apps/dav | |
parent | ab1a20522b1b2730398869a764f672175a1abcb8 (diff) |
Multiple fixesenh/noid/share-attributes
- Fix tests
- Use non deprecated event stuff
- Add a bit of type hinting to the new stuff
- More safe handling of instanceOfStorage (share might not be the first
wrapper)
- Fix resharing
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
Diffstat (limited to 'apps/dav')
-rw-r--r-- | apps/dav/lib/Connector/Sabre/Node.php | 10 | ||||
-rw-r--r-- | apps/dav/lib/Controller/DirectController.php | 7 | ||||
-rw-r--r-- | apps/dav/lib/DAV/ViewOnlyPlugin.php | 38 | ||||
-rw-r--r-- | apps/dav/tests/unit/Controller/DirectControllerTest.php | 26 | ||||
-rw-r--r-- | apps/dav/tests/unit/DAV/ViewOnlyPluginTest.php | 11 |
5 files changed, 50 insertions, 42 deletions
diff --git a/apps/dav/lib/Connector/Sabre/Node.php b/apps/dav/lib/Connector/Sabre/Node.php index a55a799a81f..87f2fea394f 100644 --- a/apps/dav/lib/Connector/Sabre/Node.php +++ b/apps/dav/lib/Connector/Sabre/Node.php @@ -38,6 +38,7 @@ namespace OCA\DAV\Connector\Sabre; use OC\Files\Mount\MoveableMount; use OC\Files\Node\File; use OC\Files\Node\Folder; +use OC\Files\Storage\Wrapper\Wrapper; use OC\Files\View; use OCA\DAV\Connector\Sabre\Exception\InvalidPath; use OCP\Files\FileInfo; @@ -334,9 +335,14 @@ abstract class Node implements \Sabre\DAV\INode { $storage = null; } - if ($storage && $storage->instanceOfStorage('\OCA\Files_Sharing\SharedStorage')) { + if ($storage && $storage->instanceOfStorage(\OCA\Files_Sharing\SharedStorage::class)) { /** @var \OCA\Files_Sharing\SharedStorage $storage */ - $attributes = $storage->getShare()->getAttributes()->toArray(); + $attributes = $storage->getShare()->getAttributes(); + if ($attributes === null) { + return []; + } else { + return $attributes->toArray(); + } } return $attributes; diff --git a/apps/dav/lib/Controller/DirectController.php b/apps/dav/lib/Controller/DirectController.php index 260ef3bae04..f9c83488935 100644 --- a/apps/dav/lib/Controller/DirectController.php +++ b/apps/dav/lib/Controller/DirectController.php @@ -36,6 +36,7 @@ use OCP\AppFramework\OCSController; use OCP\AppFramework\Utility\ITimeFactory; use OCP\EventDispatcher\GenericEvent; use OCP\EventDispatcher\IEventDispatcher; +use OCP\Files\Events\BeforeDirectFileDownloadEvent; use OCP\Files\File; use OCP\Files\IRootFolder; use OCP\IRequest; @@ -106,10 +107,10 @@ class DirectController extends OCSController { throw new OCSBadRequestException('Direct download only works for files'); } - $event = new GenericEvent(null, ['path' => $userFolder->getRelativePath($file->getPath())]); - $this->eventDispatcher->dispatch('file.beforeGetDirect', $event); + $event = new BeforeDirectFileDownloadEvent($userFolder->getRelativePath($file->getPath())); + $this->eventDispatcher->dispatchTyped($event); - if ($event->getArgument('run') === false) { + if ($event->isSuccessful() === false) { throw new OCSForbiddenException('Permission denied to download file'); } diff --git a/apps/dav/lib/DAV/ViewOnlyPlugin.php b/apps/dav/lib/DAV/ViewOnlyPlugin.php index 17b0823b7d7..1504969b5b4 100644 --- a/apps/dav/lib/DAV/ViewOnlyPlugin.php +++ b/apps/dav/lib/DAV/ViewOnlyPlugin.php @@ -37,18 +37,11 @@ use Sabre\DAV\Exception\NotFound; */ class ViewOnlyPlugin extends ServerPlugin { - /** @var Server $server */ - private $server; + private ?Server $server = null; + private LoggerInterface $logger; - /** @var LoggerInterface $logger */ - private $logger; - - /** - * @param LoggerInterface $logger - */ public function __construct(LoggerInterface $logger) { $this->logger = $logger; - $this->server = null; } /** @@ -58,11 +51,8 @@ class ViewOnlyPlugin extends ServerPlugin { * addPlugin is called. * * This method should set up the required event subscriptions. - * - * @param Server $server - * @return void */ - public function initialize(Server $server) { + public function initialize(Server $server): void { $this->server = $server; //priority 90 to make sure the plugin is called before //Sabre\DAV\CorePlugin::httpGet @@ -73,17 +63,14 @@ class ViewOnlyPlugin extends ServerPlugin { * Disallow download via DAV Api in case file being received share * and having special permission * - * @param RequestInterface $request request object - * @return boolean * @throws Forbidden * @throws NotFoundException */ - public function checkViewOnly( - RequestInterface $request - ) { + public function checkViewOnly(RequestInterface $request): bool { $path = $request->getPath(); try { + assert($this->server !== null); $davNode = $this->server->tree->getNodeForPath($path); if (!($davNode instanceof DavFile)) { return true; @@ -92,21 +79,28 @@ class ViewOnlyPlugin extends ServerPlugin { $node = $davNode->getNode(); $storage = $node->getStorage(); - // using string as we have no guarantee that "files_sharing" app is loaded - if (!$storage->instanceOfStorage('OCA\Files_Sharing\SharedStorage')) { + + if (!$storage->instanceOfStorage(\OCA\Files_Sharing\SharedStorage::class)) { return true; } // Extract extra permissions /** @var \OCA\Files_Sharing\SharedStorage $storage */ $share = $storage->getShare(); + $attributes = $share->getAttributes(); + if ($attributes === null) { + return true; + } + // Check if read-only and on whether permission can download is both set and disabled. - $canDownload = $share->getAttributes()->getAttribute('permissions', 'download'); + $canDownload = $attributes->getAttribute('permissions', 'download'); if ($canDownload !== null && !$canDownload) { throw new Forbidden('Access to this resource has been denied because it is in view-only mode.'); } } catch (NotFound $e) { - $this->logger->warning($e->getMessage()); + $this->logger->warning($e->getMessage(), [ + 'exception' => $e, + ]); } return true; diff --git a/apps/dav/tests/unit/Controller/DirectControllerTest.php b/apps/dav/tests/unit/Controller/DirectControllerTest.php index 00771e7f7a6..fe6d4ea8f24 100644 --- a/apps/dav/tests/unit/Controller/DirectControllerTest.php +++ b/apps/dav/tests/unit/Controller/DirectControllerTest.php @@ -34,11 +34,12 @@ use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\OCS\OCSBadRequestException; use OCP\AppFramework\OCS\OCSNotFoundException; use OCP\AppFramework\Utility\ITimeFactory; +use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\File; use OCP\Files\Folder; use OCP\Files\IRootFolder; use OCP\IRequest; -use OCP\IURLGenerator; +use OCP\IUrlGenerator; use OCP\Security\ISecureRandom; use Test\TestCase; @@ -56,11 +57,13 @@ class DirectControllerTest extends TestCase { /** @var ITimeFactory|\PHPUnit\Framework\MockObject\MockObject */ private $timeFactory; - /** @var IURLGenerator|\PHPUnit\Framework\MockObject\MockObject */ + /** @var IUrlGenerator|\PHPUnit\Framework\MockObject\MockObject */ private $urlGenerator; - /** @var DirectController */ - private $controller; + /** @var IEventDispatcher|\PHPUnit\Framework\MockObject\MockObject */ + private $eventDispatcher; + + private DirectController $controller; protected function setUp(): void { parent::setUp(); @@ -69,7 +72,8 @@ class DirectControllerTest extends TestCase { $this->directMapper = $this->createMock(DirectMapper::class); $this->random = $this->createMock(ISecureRandom::class); $this->timeFactory = $this->createMock(ITimeFactory::class); - $this->urlGenerator = $this->createMock(IURLGenerator::class); + $this->urlGenerator = $this->createMock(IUrlGenerator::class); + $this->eventDispatcher = $this->createMock(IEventDispatcher::class); $this->controller = new DirectController( 'dav', @@ -79,11 +83,12 @@ class DirectControllerTest extends TestCase { $this->directMapper, $this->random, $this->timeFactory, - $this->urlGenerator + $this->urlGenerator, + $this->eventDispatcher ); } - public function testGetUrlNonExistingFileId() { + public function testGetUrlNonExistingFileId(): void { $userFolder = $this->createMock(Folder::class); $this->rootFolder->method('getUserFolder') ->with('awesomeUser') @@ -97,7 +102,7 @@ class DirectControllerTest extends TestCase { $this->controller->getUrl(101); } - public function testGetUrlForFolder() { + public function testGetUrlForFolder(): void { $userFolder = $this->createMock(Folder::class); $this->rootFolder->method('getUserFolder') ->with('awesomeUser') @@ -113,7 +118,7 @@ class DirectControllerTest extends TestCase { $this->controller->getUrl(101); } - public function testGetUrlValid() { + public function testGetUrlValid(): void { $userFolder = $this->createMock(Folder::class); $this->rootFolder->method('getUserFolder') ->with('awesomeUser') @@ -128,6 +133,9 @@ class DirectControllerTest extends TestCase { ->with(101) ->willReturn([$file]); + $userFolder->method('getRelativePath') + ->willReturn('/path'); + $this->random->method('generate') ->with( 60, diff --git a/apps/dav/tests/unit/DAV/ViewOnlyPluginTest.php b/apps/dav/tests/unit/DAV/ViewOnlyPluginTest.php index 3bd7a2d6dde..f86a60fb4bf 100644 --- a/apps/dav/tests/unit/DAV/ViewOnlyPluginTest.php +++ b/apps/dav/tests/unit/DAV/ViewOnlyPluginTest.php @@ -36,8 +36,7 @@ use OCA\DAV\Connector\Sabre\Exception\Forbidden; class ViewOnlyPluginTest extends TestCase { - /** @var ViewOnlyPlugin */ - private $plugin; + private ViewOnlyPlugin $plugin; /** @var Tree | \PHPUnit\Framework\MockObject\MockObject */ private $tree; /** @var RequestInterface | \PHPUnit\Framework\MockObject\MockObject */ @@ -56,14 +55,14 @@ class ViewOnlyPluginTest extends TestCase { $this->plugin->initialize($server); } - public function testCanGetNonDav() { + public function testCanGetNonDav(): void { $this->request->expects($this->once())->method('getPath')->willReturn('files/test/target'); $this->tree->method('getNodeForPath')->willReturn(null); $this->assertTrue($this->plugin->checkViewOnly($this->request)); } - public function testCanGetNonShared() { + public function testCanGetNonShared(): void { $this->request->expects($this->once())->method('getPath')->willReturn('files/test/target'); $davNode = $this->createMock(DavFile::class); $this->tree->method('getNodeForPath')->willReturn($davNode); @@ -78,7 +77,7 @@ class ViewOnlyPluginTest extends TestCase { $this->assertTrue($this->plugin->checkViewOnly($this->request)); } - public function providesDataForCanGet() { + public function providesDataForCanGet(): array { return [ // has attribute permissions-download enabled - can get file [ $this->createMock(File::class), true, true], @@ -92,7 +91,7 @@ class ViewOnlyPluginTest extends TestCase { /** * @dataProvider providesDataForCanGet */ - public function testCanGet($nodeInfo, $attrEnabled, $expectCanDownloadFile) { + public function testCanGet(File $nodeInfo, ?bool $attrEnabled, bool $expectCanDownloadFile): void { $this->request->expects($this->once())->method('getPath')->willReturn('files/test/target'); $davNode = $this->createMock(DavFile::class); |