Skip to content

Commit 59c17bc

Browse files
CarlSchwanbackportbot-nextcloud[bot]
CarlSchwan
authored and
backportbot-nextcloud[bot]
committed
Multiple fixes
- 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 <[email protected]>
1 parent b09cf35 commit 59c17bc

25 files changed

Lines changed: 534 additions & 202 deletions

‎apps/dav/lib/Connector/Sabre/Node.php‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
use OC\Files\Mount\MoveableMount;
3939
use OC\Files\Node\File;
4040
use OC\Files\Node\Folder;
41+
use OC\Files\Storage\Wrapper\Wrapper;
4142
use OC\Files\View;
4243
use OCA\DAV\Connector\Sabre\Exception\InvalidPath;
4344
use OCP\Files\FileInfo;
@@ -334,9 +335,14 @@ public function getShareAttributes(): array {
334335
$storage = null;
335336
}
336337

337-
if ($storage && $storage->instanceOfStorage('\OCA\Files_Sharing\SharedStorage')) {
338+
if ($storage && $storage->instanceOfStorage(\OCA\Files_Sharing\SharedStorage::class)) {
338339
/** @var \OCA\Files_Sharing\SharedStorage $storage */
339-
$attributes = $storage->getShare()->getAttributes()->toArray();
340+
$attributes = $storage->getShare()->getAttributes();
341+
if ($attributes === null) {
342+
return [];
343+
} else {
344+
return $attributes->toArray();
345+
}
340346
}
341347

342348
return $attributes;

‎apps/dav/lib/Controller/DirectController.php‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
use OCP\AppFramework\Utility\ITimeFactory;
3737
use OCP\EventDispatcher\GenericEvent;
3838
use OCP\EventDispatcher\IEventDispatcher;
39+
use OCP\Files\Events\BeforeDirectFileDownloadEvent;
3940
use OCP\Files\File;
4041
use OCP\Files\IRootFolder;
4142
use OCP\IRequest;
@@ -106,10 +107,10 @@ public function getUrl(int $fileId, int $expirationTime = 60 * 60 * 8): DataResp
106107
throw new OCSBadRequestException('Direct download only works for files');
107108
}
108109

109-
$event = new GenericEvent(null, ['path' => $userFolder->getRelativePath($file->getPath())]);
110-
$this->eventDispatcher->dispatch('file.beforeGetDirect', $event);
110+
$event = new BeforeDirectFileDownloadEvent($userFolder->getRelativePath($file->getPath()));
111+
$this->eventDispatcher->dispatchTyped($event);
111112

112-
if ($event->getArgument('run') === false) {
113+
if ($event->isSuccessful() === false) {
113114
throw new OCSForbiddenException('Permission denied to download file');
114115
}
115116

‎apps/dav/lib/DAV/ViewOnlyPlugin.php‎

Lines changed: 16 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -37,18 +37,11 @@
3737
*/
3838
class ViewOnlyPlugin extends ServerPlugin {
3939

40-
/** @var Server $server */
41-
private $server;
40+
private ?Server $server = null;
41+
private LoggerInterface $logger;
4242

43-
/** @var LoggerInterface $logger */
44-
private $logger;
45-
46-
/**
47-
* @param LoggerInterface $logger
48-
*/
4943
public function __construct(LoggerInterface $logger) {
5044
$this->logger = $logger;
51-
$this->server = null;
5245
}
5346

5447
/**
@@ -58,11 +51,8 @@ public function __construct(LoggerInterface $logger) {
5851
* addPlugin is called.
5952
*
6053
* This method should set up the required event subscriptions.
61-
*
62-
* @param Server $server
63-
* @return void
6454
*/
65-
public function initialize(Server $server) {
55+
public function initialize(Server $server): void {
6656
$this->server = $server;
6757
//priority 90 to make sure the plugin is called before
6858
//Sabre\DAV\CorePlugin::httpGet
@@ -73,17 +63,14 @@ public function initialize(Server $server) {
7363
* Disallow download via DAV Api in case file being received share
7464
* and having special permission
7565
*
76-
* @param RequestInterface $request request object
77-
* @return boolean
7866
* @throws Forbidden
7967
* @throws NotFoundException
8068
*/
81-
public function checkViewOnly(
82-
RequestInterface $request
83-
) {
69+
public function checkViewOnly(RequestInterface $request): bool {
8470
$path = $request->getPath();
8571

8672
try {
73+
assert($this->server !== null);
8774
$davNode = $this->server->tree->getNodeForPath($path);
8875
if (!($davNode instanceof DavFile)) {
8976
return true;
@@ -92,21 +79,28 @@ public function checkViewOnly(
9279
$node = $davNode->getNode();
9380

9481
$storage = $node->getStorage();
95-
// using string as we have no guarantee that "files_sharing" app is loaded
96-
if (!$storage->instanceOfStorage('OCA\Files_Sharing\SharedStorage')) {
82+
83+
if (!$storage->instanceOfStorage(\OCA\Files_Sharing\SharedStorage::class)) {
9784
return true;
9885
}
9986
// Extract extra permissions
10087
/** @var \OCA\Files_Sharing\SharedStorage $storage */
10188
$share = $storage->getShare();
10289

90+
$attributes = $share->getAttributes();
91+
if ($attributes === null) {
92+
return true;
93+
}
94+
10395
// Check if read-only and on whether permission can download is both set and disabled.
104-
$canDownload = $share->getAttributes()->getAttribute('permissions', 'download');
96+
$canDownload = $attributes->getAttribute('permissions', 'download');
10597
if ($canDownload !== null && !$canDownload) {
10698
throw new Forbidden('Access to this resource has been denied because it is in view-only mode.');
10799
}
108100
} catch (NotFound $e) {
109-
$this->logger->warning($e->getMessage());
101+
$this->logger->warning($e->getMessage(), [
102+
'exception' => $e,
103+
]);
110104
}
111105

112106
return true;

‎apps/dav/tests/unit/Controller/DirectControllerTest.php‎

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,12 @@
3434
use OCP\AppFramework\OCS\OCSBadRequestException;
3535
use OCP\AppFramework\OCS\OCSNotFoundException;
3636
use OCP\AppFramework\Utility\ITimeFactory;
37+
use OCP\EventDispatcher\IEventDispatcher;
3738
use OCP\Files\File;
3839
use OCP\Files\Folder;
3940
use OCP\Files\IRootFolder;
4041
use OCP\IRequest;
41-
use OCP\IURLGenerator;
42+
use OCP\IUrlGenerator;
4243
use OCP\Security\ISecureRandom;
4344
use Test\TestCase;
4445

@@ -56,11 +57,13 @@ class DirectControllerTest extends TestCase {
5657
/** @var ITimeFactory|\PHPUnit\Framework\MockObject\MockObject */
5758
private $timeFactory;
5859

59-
/** @var IURLGenerator|\PHPUnit\Framework\MockObject\MockObject */
60+
/** @var IUrlGenerator|\PHPUnit\Framework\MockObject\MockObject */
6061
private $urlGenerator;
6162

62-
/** @var DirectController */
63-
private $controller;
63+
/** @var IEventDispatcher|\PHPUnit\Framework\MockObject\MockObject */
64+
private $eventDispatcher;
65+
66+
private DirectController $controller;
6467

6568
protected function setUp(): void {
6669
parent::setUp();
@@ -69,7 +72,8 @@ protected function setUp(): void {
6972
$this->directMapper = $this->createMock(DirectMapper::class);
7073
$this->random = $this->createMock(ISecureRandom::class);
7174
$this->timeFactory = $this->createMock(ITimeFactory::class);
72-
$this->urlGenerator = $this->createMock(IURLGenerator::class);
75+
$this->urlGenerator = $this->createMock(IUrlGenerator::class);
76+
$this->eventDispatcher = $this->createMock(IEventDispatcher::class);
7377

7478
$this->controller = new DirectController(
7579
'dav',
@@ -79,11 +83,12 @@ protected function setUp(): void {
7983
$this->directMapper,
8084
$this->random,
8185
$this->timeFactory,
82-
$this->urlGenerator
86+
$this->urlGenerator,
87+
$this->eventDispatcher
8388
);
8489
}
8590

86-
public function testGetUrlNonExistingFileId() {
91+
public function testGetUrlNonExistingFileId(): void {
8792
$userFolder = $this->createMock(Folder::class);
8893
$this->rootFolder->method('getUserFolder')
8994
->with('awesomeUser')
@@ -97,7 +102,7 @@ public function testGetUrlNonExistingFileId() {
97102
$this->controller->getUrl(101);
98103
}
99104

100-
public function testGetUrlForFolder() {
105+
public function testGetUrlForFolder(): void {
101106
$userFolder = $this->createMock(Folder::class);
102107
$this->rootFolder->method('getUserFolder')
103108
->with('awesomeUser')
@@ -113,7 +118,7 @@ public function testGetUrlForFolder() {
113118
$this->controller->getUrl(101);
114119
}
115120

116-
public function testGetUrlValid() {
121+
public function testGetUrlValid(): void {
117122
$userFolder = $this->createMock(Folder::class);
118123
$this->rootFolder->method('getUserFolder')
119124
->with('awesomeUser')
@@ -128,6 +133,9 @@ public function testGetUrlValid() {
128133
->with(101)
129134
->willReturn([$file]);
130135

136+
$userFolder->method('getRelativePath')
137+
->willReturn('/path');
138+
131139
$this->random->method('generate')
132140
->with(
133141
60,

‎apps/dav/tests/unit/DAV/ViewOnlyPluginTest.php‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,7 @@
3636

3737
class ViewOnlyPluginTest extends TestCase {
3838

39-
/** @var ViewOnlyPlugin */
40-
private $plugin;
39+
private ViewOnlyPlugin $plugin;
4140
/** @var Tree | \PHPUnit\Framework\MockObject\MockObject */
4241
private $tree;
4342
/** @var RequestInterface | \PHPUnit\Framework\MockObject\MockObject */
@@ -56,14 +55,14 @@ public function setUp(): void {
5655
$this->plugin->initialize($server);
5756
}
5857

59-
public function testCanGetNonDav() {
58+
public function testCanGetNonDav(): void {
6059
$this->request->expects($this->once())->method('getPath')->willReturn('files/test/target');
6160
$this->tree->method('getNodeForPath')->willReturn(null);
6261

6362
$this->assertTrue($this->plugin->checkViewOnly($this->request));
6463
}
6564

66-
public function testCanGetNonShared() {
65+
public function testCanGetNonShared(): void {
6766
$this->request->expects($this->once())->method('getPath')->willReturn('files/test/target');
6867
$davNode = $this->createMock(DavFile::class);
6968
$this->tree->method('getNodeForPath')->willReturn($davNode);
@@ -78,7 +77,7 @@ public function testCanGetNonShared() {
7877
$this->assertTrue($this->plugin->checkViewOnly($this->request));
7978
}
8079

81-
public function providesDataForCanGet() {
80+
public function providesDataForCanGet(): array {
8281
return [
8382
// has attribute permissions-download enabled - can get file
8483
[ $this->createMock(File::class), true, true],
@@ -92,7 +91,7 @@ public function providesDataForCanGet() {
9291
/**
9392
* @dataProvider providesDataForCanGet
9493
*/
95-
public function testCanGet($nodeInfo, $attrEnabled, $expectCanDownloadFile) {
94+
public function testCanGet(File $nodeInfo, ?bool $attrEnabled, bool $expectCanDownloadFile): void {
9695
$this->request->expects($this->once())->method('getPath')->willReturn('files/test/target');
9796

9897
$davNode = $this->createMock(DavFile::class);

‎apps/files_sharing/lib/AppInfo/Application.php‎

Lines changed: 25 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@
5050
use OCA\Files_Sharing\Notification\Notifier;
5151
use OCA\Files\Event\LoadAdditionalScriptsEvent;
5252
use OCA\Files\Event\LoadSidebar;
53+
use OCP\Files\Event\BeforeDirectGetEvent;
5354
use OCA\Files_Sharing\ShareBackend\File;
5455
use OCA\Files_Sharing\ShareBackend\Folder;
5556
use OCA\Files_Sharing\ViewOnly;
@@ -61,6 +62,8 @@
6162
use OCP\EventDispatcher\GenericEvent;
6263
use OCP\Federation\ICloudIdManager;
6364
use OCP\Files\Config\IMountProviderCollection;
65+
use OCP\Files\Events\BeforeDirectFileDownloadEvent;
66+
use OCP\Files\Events\BeforeZipCreatedEvent;
6467
use OCP\Files\IRootFolder;
6568
use OCP\Group\Events\UserAddedEvent;
6669
use OCP\IDBConnection;
@@ -156,59 +159,53 @@ public function registerEventsScripts(IEventDispatcher $dispatcher, EventDispatc
156159

157160
public function registerDownloadEvents(
158161
IEventDispatcher $dispatcher,
159-
?IUserSession $userSession,
162+
IUserSession $userSession,
160163
IRootFolder $rootFolder
161164
): void {
162165

163166
$dispatcher->addListener(
164-
'file.beforeGetDirect',
165-
function (GenericEvent $event) use ($userSession, $rootFolder) {
166-
$pathsToCheck = [$event->getArgument('path')];
167-
$event->setArgument('run', true);
168-
167+
BeforeDirectFileDownloadEvent::class,
168+
function (BeforeDirectFileDownloadEvent $event) use ($userSession, $rootFolder): void {
169+
$pathsToCheck = [$event->getPath()];
169170
// Check only for user/group shares. Don't restrict e.g. share links
170-
if ($userSession && $userSession->isLoggedIn()) {
171-
$uid = $userSession->getUser()->getUID();
171+
$user = $userSession->getUser();
172+
if ($user) {
172173
$viewOnlyHandler = new ViewOnly(
173-
$rootFolder->getUserFolder($uid)
174+
$rootFolder->getUserFolder($user->getUID())
174175
);
175176
if (!$viewOnlyHandler->check($pathsToCheck)) {
176-
$event->setArgument('run', false);
177-
$event->setArgument('errorMessage', 'Access to this resource or one of its sub-items has been denied.');
177+
$event->setSuccessful(false);
178+
$event->setErrorMessage('Access to this resource or one of its sub-items has been denied.');
178179
}
179180
}
180181
}
181182
);
182183

183184
$dispatcher->addListener(
184-
'file.beforeCreateZip',
185-
function (GenericEvent $event) use ($userSession, $rootFolder) {
186-
$dir = $event->getArgument('dir');
187-
$files = $event->getArgument('files');
185+
BeforeZipCreatedEvent::class,
186+
function (BeforeZipCreatedEvent $event) use ($userSession, $rootFolder): void {
187+
$dir = $event->getDirectory();
188+
$files = $event->getFiles();
188189

189190
$pathsToCheck = [];
190-
if (\is_array($files)) {
191-
foreach ($files as $file) {
192-
$pathsToCheck[] = $dir . '/' . $file;
193-
}
194-
} elseif (\is_string($files)) {
195-
$pathsToCheck[] = $dir . '/' . $files;
191+
foreach ($files as $file) {
192+
$pathsToCheck[] = $dir . '/' . $file;
196193
}
197194

198195
// Check only for user/group shares. Don't restrict e.g. share links
199-
if ($userSession && $userSession->isLoggedIn()) {
200-
$uid = $userSession->getUser()->getUID();
196+
$user = $userSession->getUser();
197+
if ($user) {
201198
$viewOnlyHandler = new ViewOnly(
202-
$rootFolder->getUserFolder($uid)
199+
$rootFolder->getUserFolder($user->getUID())
203200
);
204201
if (!$viewOnlyHandler->check($pathsToCheck)) {
205-
$event->setArgument('errorMessage', 'Access to this resource or one of its sub-items has been denied.');
206-
$event->setArgument('run', false);
202+
$event->setErrorMessage('Access to this resource or one of its sub-items has been denied.');
203+
$event->setSuccessful(false);
207204
} else {
208-
$event->setArgument('run', true);
205+
$event->setSuccessful(true);
209206
}
210207
} else {
211-
$event->setArgument('run', true);
208+
$event->setSuccessful(true);
212209
}
213210
}
214211
);

‎apps/files_sharing/lib/Controller/PublicPreviewController.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ public function getPreview(
136136
* @param $token
137137
* @return DataResponse|FileDisplayResponse
138138
*/
139-
public function directLink($token) {
139+
public function directLink(string $token) {
140140
// No token no image
141141
if ($token === '') {
142142
return new DataResponse([], Http::STATUS_BAD_REQUEST);

0 commit comments

Comments
 (0)