Skip to content

Commit 73dad8a

Browse files
susnuxsusnux
committed
fix: Adjust preview for view-only shares
Previously there was a different behavior for public shares (link-shares) and internal shares, if the user disabled the view permission. The legacy UI for public shares simply "disabled" the context menu and hided all download actions. With Nextcloud 31 all share types use the consistent permissions attributes, which simplifies code, but caused a regression: Images can no longer been viewed. Because on 30 and before the attribute was not set, previews for view-only files were still allowed. Now with 31 we need a new way to allow "viewing" shares. So this is allowing previews for those files, but only for internal usage. This is done by settin a special header, which only works with custom requests, and not by opening the URL directly. Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
1 parent 926bd29 commit 73dad8a

5 files changed

Lines changed: 310 additions & 42 deletions

File tree

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

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,8 @@ public function getPreview(
8585
int $y = 32,
8686
$a = false
8787
) {
88+
$cacheForSeconds = 60 * 60 * 24; // 1 day
89+
8890
if ($token === '' || $x === 0 || $y === 0) {
8991
return new DataResponse([], Http::STATUS_BAD_REQUEST);
9092
}
@@ -100,7 +102,17 @@ public function getPreview(
100102
}
101103

102104
$attributes = $share->getAttributes();
103-
if ($attributes !== null && $attributes->getAttribute('permissions', 'download') === false) {
105+
// Only explicitly set to false will forbid the download!
106+
$downloadForbidden = $attributes?->getAttribute('permissions', 'download') === false;
107+
// Is this header is set it means our UI is doing a preview for no-download shares
108+
// we check a header so we at least prevent people from using the link directly (obfuscation)
109+
$isPublicPreview = $this->request->getHeader('X-NC-Preview') === 'true';
110+
111+
if ($isPublicPreview && $downloadForbidden) {
112+
// Only cache for 15 minutes on public preview requests to quickly remove from cache
113+
$cacheForSeconds = 15 * 60;
114+
} elseif ($downloadForbidden) {
115+
// This is not a public share preview so we only allow a preview if download permissions are granted
104116
return new DataResponse([], Http::STATUS_FORBIDDEN);
105117
}
106118

@@ -114,7 +126,7 @@ public function getPreview(
114126

115127
$f = $this->previewManager->getPreview($file, $x, $y, !$a);
116128
$response = new FileDisplayResponse($f, Http::STATUS_OK, ['Content-Type' => $f->getMimeType()]);
117-
$response->cacheFor(3600 * 24);
129+
$response->cacheFor($cacheForSeconds);
118130
return $response;
119131
} catch (NotFoundException $e) {
120132
return new DataResponse([], Http::STATUS_NOT_FOUND);

‎apps/files_sharing/lib/ViewOnly.php‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -91,17 +91,10 @@ private function checkFileInfo(Node $fileInfo): bool {
9191
/** @var \OCA\Files_Sharing\SharedStorage $storage */
9292
$share = $storage->getShare();
9393

94-
$canDownload = true;
95-
96-
// Check if read-only and on whether permission can download is both set and disabled.
94+
// Check whether download-permission was denied (granted if not set)
9795
$attributes = $share->getAttributes();
98-
if ($attributes !== null) {
99-
$canDownload = $attributes->getAttribute('permissions', 'download');
100-
}
96+
$canDownload = $attributes?->getAttribute('permissions', 'download');
10197

102-
if ($canDownload !== null && !$canDownload) {
103-
return false;
104-
}
105-
return true;
98+
return $canDownload !== false;
10699
}
107100
}

‎apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php‎

Lines changed: 110 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,29 +19,28 @@
1919
use OCP\IRequest;
2020
use OCP\ISession;
2121
use OCP\Share\Exceptions\ShareNotFound;
22+
use OCP\Share\IAttributes;
2223
use OCP\Share\IManager;
2324
use OCP\Share\IShare;
2425
use PHPUnit\Framework\MockObject\MockObject;
2526
use Test\TestCase;
2627

2728
class PublicPreviewControllerTest extends TestCase {
2829

29-
/** @var IPreview|\PHPUnit\Framework\MockObject\MockObject */
30-
private $previewManager;
31-
/** @var IManager|\PHPUnit\Framework\MockObject\MockObject */
32-
private $shareManager;
33-
/** @var ITimeFactory|MockObject */
34-
private $timeFactory;
30+
private IPreview&MockObject $previewManager;
31+
private IManager&MockObject $shareManager;
32+
private ITimeFactory&MockObject $timeFactory;
33+
private IRequest&MockObject $request;
3534

36-
/** @var PublicPreviewController */
37-
private $controller;
35+
private PublicPreviewController $controller;
3836

3937
protected function setUp(): void {
4038
parent::setUp();
4139

4240
$this->previewManager = $this->createMock(IPreview::class);
4341
$this->shareManager = $this->createMock(IManager::class);
4442
$this->timeFactory = $this->createMock(ITimeFactory::class);
43+
$this->request = $this->createMock(IRequest::class);
4544

4645
$this->timeFactory->method('getTime')
4746
->willReturn(1337);
@@ -50,7 +49,7 @@ protected function setUp(): void {
5049

5150
$this->controller = new PublicPreviewController(
5251
'files_sharing',
53-
$this->createMock(IRequest::class),
52+
$this->request,
5453
$this->shareManager,
5554
$this->createMock(ISession::class),
5655
$this->previewManager
@@ -104,6 +103,108 @@ public function testShareNotAccessable() {
104103
$this->assertEquals($expected, $res);
105104
}
106105

106+
public function testShareNoDownload() {
107+
$share = $this->createMock(IShare::class);
108+
$this->shareManager->method('getShareByToken')
109+
->with($this->equalTo('token'))
110+
->willReturn($share);
111+
112+
$share->method('getPermissions')
113+
->willReturn(Constants::PERMISSION_READ);
114+
115+
$attributes = $this->createMock(IAttributes::class);
116+
$attributes->method('getAttribute')
117+
->with('permissions', 'download')
118+
->willReturn(false);
119+
$share->method('getAttributes')
120+
->willReturn($attributes);
121+
122+
$res = $this->controller->getPreview('token', 'file', 10, 10);
123+
$expected = new DataResponse([], Http::STATUS_FORBIDDEN);
124+
125+
$this->assertEquals($expected, $res);
126+
}
127+
128+
public function testShareNoDownloadButPreviewHeader() {
129+
$share = $this->createMock(IShare::class);
130+
$this->shareManager->method('getShareByToken')
131+
->with($this->equalTo('token'))
132+
->willReturn($share);
133+
134+
$share->method('getPermissions')
135+
->willReturn(Constants::PERMISSION_READ);
136+
137+
$attributes = $this->createMock(IAttributes::class);
138+
$attributes->method('getAttribute')
139+
->with('permissions', 'download')
140+
->willReturn(false);
141+
$share->method('getAttributes')
142+
->willReturn($attributes);
143+
144+
$this->request->method('getHeader')
145+
->with('X-NC-Preview')
146+
->willReturn('true');
147+
148+
$file = $this->createMock(File::class);
149+
$share->method('getNode')
150+
->willReturn($file);
151+
152+
$preview = $this->createMock(ISimpleFile::class);
153+
$preview->method('getName')->willReturn('name');
154+
$preview->method('getMTime')->willReturn(42);
155+
$this->previewManager->method('getPreview')
156+
->with($this->equalTo($file), 10, 10, false)
157+
->willReturn($preview);
158+
159+
$preview->method('getMimeType')
160+
->willReturn('myMime');
161+
162+
$res = $this->controller->getPreview('token', 'file', 10, 10, true);
163+
$expected = new FileDisplayResponse($preview, Http::STATUS_OK, ['Content-Type' => 'myMime']);
164+
$expected->cacheFor(15 * 60);
165+
$this->assertEquals($expected, $res);
166+
}
167+
168+
public function testShareWithAttributes() {
169+
$share = $this->createMock(IShare::class);
170+
$this->shareManager->method('getShareByToken')
171+
->with($this->equalTo('token'))
172+
->willReturn($share);
173+
174+
$share->method('getPermissions')
175+
->willReturn(Constants::PERMISSION_READ);
176+
177+
$attributes = $this->createMock(IAttributes::class);
178+
$attributes->method('getAttribute')
179+
->with('permissions', 'download')
180+
->willReturn(true);
181+
$share->method('getAttributes')
182+
->willReturn($attributes);
183+
184+
$this->request->method('getHeader')
185+
->with('X-NC-Preview')
186+
->willReturn('true');
187+
188+
$file = $this->createMock(File::class);
189+
$share->method('getNode')
190+
->willReturn($file);
191+
192+
$preview = $this->createMock(ISimpleFile::class);
193+
$preview->method('getName')->willReturn('name');
194+
$preview->method('getMTime')->willReturn(42);
195+
$this->previewManager->method('getPreview')
196+
->with($this->equalTo($file), 10, 10, false)
197+
->willReturn($preview);
198+
199+
$preview->method('getMimeType')
200+
->willReturn('myMime');
201+
202+
$res = $this->controller->getPreview('token', 'file', 10, 10, true);
203+
$expected = new FileDisplayResponse($preview, Http::STATUS_OK, ['Content-Type' => 'myMime']);
204+
$expected->cacheFor(3600 * 24);
205+
$this->assertEquals($expected, $res);
206+
}
207+
107208
public function testPreviewFile() {
108209
$share = $this->createMock(IShare::class);
109210
$this->shareManager->method('getShareByToken')

‎core/Controller/PreviewController.php‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
*/
99
namespace OC\Core\Controller;
1010

11-
use OCA\Files_Sharing\SharedStorage;
1211
use OCP\AppFramework\Controller;
1312
use OCP\AppFramework\Http;
1413
use OCP\AppFramework\Http\Attribute\FrontpageRoute;
@@ -21,6 +20,7 @@
2120
use OCP\Files\IRootFolder;
2221
use OCP\Files\Node;
2322
use OCP\Files\NotFoundException;
23+
use OCP\Files\Storage\ISharedStorage;
2424
use OCP\IPreview;
2525
use OCP\IRequest;
2626
use OCP\Preview\IMimeIconProvider;
@@ -145,12 +145,17 @@ private function fetchPreview(
145145
return new DataResponse([], Http::STATUS_NOT_FOUND);
146146
}
147147

148+
// Is this header is set it means our UI is doing a preview for no-download shares
149+
// we check a header so we at least prevent people from using the link directly (obfuscation)
150+
$isNextcloudPreview = $this->request->getHeader('X-NC-Preview') === 'true';
148151
$storage = $node->getStorage();
149-
if ($storage->instanceOfStorage(SharedStorage::class)) {
150-
/** @var SharedStorage $storage */
152+
if ($isNextcloudPreview === false && $storage->instanceOfStorage(ISharedStorage::class)) {
153+
/** @var ISharedStorage $storage */
151154
$share = $storage->getShare();
152155
$attributes = $share->getAttributes();
153-
if ($attributes !== null && $attributes->getAttribute('permissions', 'download') === false) {
156+
// No "allow preview" header set, so we must check if
157+
// the share has not explicitly disabled download permissions
158+
if ($attributes?->getAttribute('permissions', 'download') === false) {
154159
return new DataResponse([], Http::STATUS_FORBIDDEN);
155160
}
156161
}

0 commit comments

Comments
 (0)