Skip to content

Commit bd00b75

Browse files
skjnldsvskjnldsv
authored
Merge pull request #53671 from nextcloud/fix/read-only-share-download
2 parents a9e3f68 + 20a50ea commit bd00b75

File tree

20 files changed

+233
-74
lines changed

20 files changed

+233
-74
lines changed

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

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -84,18 +84,25 @@ public function checkViewOnly(RequestInterface $request): bool {
8484
if (!$storage->instanceOfStorage(ISharedStorage::class)) {
8585
return true;
8686
}
87+
8788
// Extract extra permissions
8889
/** @var ISharedStorage $storage */
8990
$share = $storage->getShare();
90-
9191
$attributes = $share->getAttributes();
9292
if ($attributes === null) {
9393
return true;
9494
}
9595

96-
// Check if read-only and on whether permission can download is both set and disabled.
96+
// We have two options here, if download is disabled, but viewing is allowed,
97+
// we still allow the GET request to return the file content.
9798
$canDownload = $attributes->getAttribute('permissions', 'download');
98-
if ($canDownload !== null && !$canDownload) {
99+
if (!$share->canSeeContent()) {
100+
throw new Forbidden('Access to this shared resource has been denied because its download permission is disabled.');
101+
}
102+
103+
// If download is disabled, we disable the COPY and MOVE methods even if the
104+
// shareapi_allow_view_without_download is set to true.
105+
if ($request->getMethod() !== 'GET' && ($canDownload !== null && !$canDownload)) {
99106
throw new Forbidden('Access to this shared resource has been denied because its download permission is disabled.');
100107
}
101108
} catch (NotFound $e) {

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

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -74,24 +74,30 @@ public function testCanGetNonShared(): void {
7474
public static function providesDataForCanGet(): array {
7575
return [
7676
// has attribute permissions-download enabled - can get file
77-
[false, true, true],
77+
[false, true, true, true],
7878
// has no attribute permissions-download - can get file
79-
[false, null, true],
80-
// has attribute permissions-download disabled- cannot get the file
81-
[false, false, false],
79+
[false, null, true, true],
8280
// has attribute permissions-download enabled - can get file version
83-
[true, true, true],
81+
[true, true, true, true],
8482
// has no attribute permissions-download - can get file version
85-
[true, null, true],
86-
// has attribute permissions-download disabled- cannot get the file version
87-
[true, false, false],
83+
[true, null, true, true],
84+
// has attribute permissions-download disabled - cannot get the file
85+
[false, false, false, false],
86+
// has attribute permissions-download disabled - cannot get the file version
87+
[true, false, false, false],
88+
89+
// Has global allowViewWithoutDownload option enabled
90+
// has attribute permissions-download disabled - can get file
91+
[false, false, false, true],
92+
// has attribute permissions-download disabled - can get file version
93+
[true, false, false, true],
8894
];
8995
}
9096

9197
/**
9298
* @dataProvider providesDataForCanGet
9399
*/
94-
public function testCanGet(bool $isVersion, ?bool $attrEnabled, bool $expectCanDownloadFile): void {
100+
public function testCanGet(bool $isVersion, ?bool $attrEnabled, bool $expectCanDownloadFile, bool $allowViewWithoutDownload): void {
95101
$nodeInfo = $this->createMock(File::class);
96102
if ($isVersion) {
97103
$davPath = 'versions/alice/versions/117/123456';
@@ -150,6 +156,10 @@ public function testCanGet(bool $isVersion, ?bool $attrEnabled, bool $expectCanD
150156
->method('getAttribute')
151157
->with('permissions', 'download')
152158
->willReturn($attrEnabled);
159+
160+
$share->expects($this->once())
161+
->method('canSeeContent')
162+
->willReturn($allowViewWithoutDownload);
153163

154164
if (!$expectCanDownloadFile) {
155165
$this->expectException(Forbidden::class);

‎apps/files/lib/Controller/ApiController.php‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -105,11 +105,12 @@ public function getThumbnail($x, $y, $file) {
105105
}
106106

107107
// Validate the user is allowed to download the file (preview is some kind of download)
108+
/** @var ISharedStorage $storage */
108109
$storage = $file->getStorage();
109110
if ($storage->instanceOfStorage(ISharedStorage::class)) {
110-
/** @var ISharedStorage $storage */
111-
$attributes = $storage->getShare()->getAttributes();
112-
if ($attributes !== null && $attributes->getAttribute('permissions', 'download') === false) {
111+
/** @var IShare $share */
112+
$share = $storage->getShare();
113+
if (!$share->canSeeContent()) {
113114
throw new NotFoundException();
114115
}
115116
}

‎apps/files/tests/Controller/ApiControllerTest.php‎

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929
use OCP\IRequest;
3030
use OCP\IUser;
3131
use OCP\IUserSession;
32-
use OCP\Share\IAttributes;
3332
use OCP\Share\IManager;
3433
use OCP\Share\IShare;
3534
use PHPUnit\Framework\MockObject\MockObject;
@@ -183,16 +182,10 @@ public function testGetThumbnailInvalidPartFile(): void {
183182
}
184183

185184
public function testGetThumbnailSharedNoDownload(): void {
186-
$attributes = $this->createMock(IAttributes::class);
187-
$attributes->expects(self::once())
188-
->method('getAttribute')
189-
->with('permissions', 'download')
190-
->willReturn(false);
191-
192185
$share = $this->createMock(IShare::class);
193186
$share->expects(self::once())
194-
->method('getAttributes')
195-
->willReturn($attributes);
187+
->method('canSeeContent')
188+
->willReturn(false);
196189

197190
$storage = $this->createMock(ISharedStorage::class);
198191
$storage->expects(self::once())
@@ -221,8 +214,8 @@ public function testGetThumbnailSharedNoDownload(): void {
221214
public function testGetThumbnailShared(): void {
222215
$share = $this->createMock(IShare::class);
223216
$share->expects(self::once())
224-
->method('getAttributes')
225-
->willReturn(null);
217+
->method('canSeeContent')
218+
->willReturn(true);
226219

227220
$storage = $this->createMock(ISharedStorage::class);
228221
$storage->expects(self::once())

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

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -102,9 +102,9 @@ public function getPreview(
102102
return new DataResponse([], Http::STATUS_FORBIDDEN);
103103
}
104104

105-
$attributes = $share->getAttributes();
106105
// Only explicitly set to false will forbid the download!
107-
$downloadForbidden = $attributes?->getAttribute('permissions', 'download') === false;
106+
$downloadForbidden = !$share->canSeeContent();
107+
108108
// Is this header is set it means our UI is doing a preview for no-download shares
109109
// we check a header so we at least prevent people from using the link directly (obfuscation)
110110
$isPublicPreview = $this->request->getHeader('x-nc-preview') === 'true';
@@ -181,8 +181,7 @@ public function directLink(string $token) {
181181
return new DataResponse([], Http::STATUS_FORBIDDEN);
182182
}
183183

184-
$attributes = $share->getAttributes();
185-
if ($attributes !== null && $attributes->getAttribute('permissions', 'download') === false) {
184+
if (!$share->canSeeContent()) {
186185
return new DataResponse([], Http::STATUS_FORBIDDEN);
187186
}
188187

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

Lines changed: 12 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
use OCP\ISession;
2121
use OCP\Preview\IMimeIconProvider;
2222
use OCP\Share\Exceptions\ShareNotFound;
23-
use OCP\Share\IAttributes;
2423
use OCP\Share\IManager;
2524
use OCP\Share\IShare;
2625
use PHPUnit\Framework\MockObject\MockObject;
@@ -114,12 +113,8 @@ public function testShareNoDownload() {
114113
$share->method('getPermissions')
115114
->willReturn(Constants::PERMISSION_READ);
116115

117-
$attributes = $this->createMock(IAttributes::class);
118-
$attributes->method('getAttribute')
119-
->with('permissions', 'download')
116+
$share->method('canSeeContent')
120117
->willReturn(false);
121-
$share->method('getAttributes')
122-
->willReturn($attributes);
123118

124119
$res = $this->controller->getPreview('token', 'file', 10, 10);
125120
$expected = new DataResponse([], Http::STATUS_FORBIDDEN);
@@ -136,12 +131,8 @@ public function testShareNoDownloadButPreviewHeader() {
136131
$share->method('getPermissions')
137132
->willReturn(Constants::PERMISSION_READ);
138133

139-
$attributes = $this->createMock(IAttributes::class);
140-
$attributes->method('getAttribute')
141-
->with('permissions', 'download')
134+
$share->method('canSeeContent')
142135
->willReturn(false);
143-
$share->method('getAttributes')
144-
->willReturn($attributes);
145136

146137
$this->request->method('getHeader')
147138
->with('x-nc-preview')
@@ -176,12 +167,8 @@ public function testShareWithAttributes() {
176167
$share->method('getPermissions')
177168
->willReturn(Constants::PERMISSION_READ);
178169

179-
$attributes = $this->createMock(IAttributes::class);
180-
$attributes->method('getAttribute')
181-
->with('permissions', 'download')
170+
$share->method('canSeeContent')
182171
->willReturn(true);
183-
$share->method('getAttributes')
184-
->willReturn($attributes);
185172

186173
$this->request->method('getHeader')
187174
->with('x-nc-preview')
@@ -220,6 +207,9 @@ public function testPreviewFile() {
220207
$share->method('getNode')
221208
->willReturn($file);
222209

210+
$share->method('canSeeContent')
211+
->willReturn(true);
212+
223213
$preview = $this->createMock(ISimpleFile::class);
224214
$preview->method('getName')->willReturn('name');
225215
$preview->method('getMTime')->willReturn(42);
@@ -249,6 +239,9 @@ public function testPreviewFolderInvalidFile(): void {
249239
$share->method('getNode')
250240
->willReturn($folder);
251241

242+
$share->method('canSeeContent')
243+
->willReturn(true);
244+
252245
$folder->method('get')
253246
->with($this->equalTo('file'))
254247
->willThrowException(new NotFoundException());
@@ -272,6 +265,9 @@ public function testPreviewFolderValidFile(): void {
272265
$share->method('getNode')
273266
->willReturn($folder);
274267

268+
$share->method('canSeeContent')
269+
->willReturn(true);
270+
275271
$file = $this->createMock(File::class);
276272
$folder->method('get')
277273
->with($this->equalTo('file'))

‎apps/settings/lib/Settings/Admin/Sharing.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ public function getForm() {
7272
'remoteExpireAfterNDays' => $this->config->getAppValue('core', 'shareapi_remote_expire_after_n_days', '7'),
7373
'enforceRemoteExpireDate' => $this->getHumanBooleanConfig('core', 'shareapi_enforce_remote_expire_date'),
7474
'allowCustomTokens' => $this->shareManager->allowCustomTokens(),
75+
'allowViewWithoutDownload' => $this->shareManager->allowViewWithoutDownload(),
7576
];
7677

7778
$this->initialState->provideInitialState('sharingAppEnabled', $this->appManager->isEnabledForUser('files_sharing'));

‎apps/settings/src/components/AdminSettingsSharingForm.vue‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,15 @@
2727
:label="t('settings', 'Ignore the following groups when checking group membership')"
2828
style="width: 100%" />
2929
</div>
30+
<NcCheckboxRadioSwitch :checked.sync="settings.allowViewWithoutDownload">
31+
{{ t('settings', 'Allow users to preview files even if download is disabled') }}
32+
</NcCheckboxRadioSwitch>
33+
<NcNoteCard v-show="settings.allowViewWithoutDownload"
34+
id="settings-sharing-api-view-without-download-hint"
35+
class="sharing__note"
36+
type="warning">
37+
{{ t('settings', 'Users will still be able to screenshot or record the screen. This does not provide any definitive protection.') }}
38+
</NcNoteCard>
3039
</div>
3140

3241
<div v-show="settings.enabled" id="settings-sharing-api" class="sharing__section">
@@ -258,6 +267,7 @@ interface IShareSettings {
258267
remoteExpireAfterNDays: string
259268
enforceRemoteExpireDate: boolean
260269
allowCustomTokens: boolean
270+
allowViewWithoutDownload: boolean
261271
}
262272
263273
export default defineComponent({

‎build/integration/features/bootstrap/SharingContext.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ protected function resetAppConfigs() {
2929
$this->deleteServerConfig('core', 'shareapi_expire_after_n_days');
3030
$this->deleteServerConfig('core', 'link_defaultExpDays');
3131
$this->deleteServerConfig('files_sharing', 'outgoing_server2server_share_enabled');
32+
$this->deleteServerConfig('core', 'shareapi_allow_view_without_download');
3233

3334
$this->runOcc(['config:system:delete', 'share_folder']);
3435
}

‎build/integration/sharing_features/sharing-v1-part2.feature‎

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1265,7 +1265,9 @@ Feature: sharing
12651265
|{http://open-collaboration-services.org/ns}share-permissions |
12661266
Then the single response should contain a property "{http://open-collaboration-services.org/ns}share-permissions" with value "19"
12671267

1268-
Scenario: Cannot download a file when it's shared view-only
1268+
Scenario: Cannot download a file when it's shared view-only without shareapi_allow_view_without_download
1269+
Given As an "admin"
1270+
And parameter "shareapi_allow_view_without_download" of app "core" is set to "no"
12691271
Given user "user0" exists
12701272
And user "user1" exists
12711273
And User "user0" moves file "/textfile0.txt" to "/document.odt"
@@ -1274,8 +1276,15 @@ Feature: sharing
12741276
When As an "user1"
12751277
And Downloading file "/document.odt"
12761278
Then the HTTP status code should be "403"
1279+
Then As an "admin"
1280+
And parameter "shareapi_allow_view_without_download" of app "core" is set to "yes"
1281+
Then As an "user1"
1282+
And Downloading file "/document.odt"
1283+
Then the HTTP status code should be "200"
12771284

1278-
Scenario: Cannot download a file when its parent is shared view-only
1285+
Scenario: Cannot download a file when its parent is shared view-only without shareapi_allow_view_without_download
1286+
Given As an "admin"
1287+
And parameter "shareapi_allow_view_without_download" of app "core" is set to "no"
12791288
Given user "user0" exists
12801289
And user "user1" exists
12811290
And User "user0" created a folder "/sharedviewonly"
@@ -1285,17 +1294,31 @@ Feature: sharing
12851294
When As an "user1"
12861295
And Downloading file "/sharedviewonly/document.odt"
12871296
Then the HTTP status code should be "403"
1297+
Then As an "admin"
1298+
And parameter "shareapi_allow_view_without_download" of app "core" is set to "yes"
1299+
Then As an "user1"
1300+
And Downloading file "/sharedviewonly/document.odt"
1301+
Then the HTTP status code should be "200"
12881302

1289-
Scenario: Cannot copy a file when it's shared view-only
1303+
Scenario: Cannot copy a file when it's shared view-only even with shareapi_allow_view_without_download enabled
1304+
Given As an "admin"
1305+
And parameter "shareapi_allow_view_without_download" of app "core" is set to "no"
12901306
Given user "user0" exists
12911307
And user "user1" exists
12921308
And User "user0" moves file "/textfile0.txt" to "/document.odt"
12931309
And file "document.odt" of user "user0" is shared with user "user1" view-only
12941310
And user "user1" accepts last share
12951311
When User "user1" copies file "/document.odt" to "/copyforbidden.odt"
12961312
Then the HTTP status code should be "403"
1313+
Then As an "admin"
1314+
And parameter "shareapi_allow_view_without_download" of app "core" is set to "yes"
1315+
Then As an "user1"
1316+
And User "user1" copies file "/document.odt" to "/copyforbidden.odt"
1317+
Then the HTTP status code should be "403"
12971318

12981319
Scenario: Cannot copy a file when its parent is shared view-only
1320+
Given As an "admin"
1321+
And parameter "shareapi_allow_view_without_download" of app "core" is set to "no"
12991322
Given user "user0" exists
13001323
And user "user1" exists
13011324
And User "user0" created a folder "/sharedviewonly"
@@ -1304,5 +1327,10 @@ Feature: sharing
13041327
And user "user1" accepts last share
13051328
When User "user1" copies file "/sharedviewonly/document.odt" to "/copyforbidden.odt"
13061329
Then the HTTP status code should be "403"
1330+
Then As an "admin"
1331+
And parameter "shareapi_allow_view_without_download" of app "core" is set to "yes"
1332+
Then As an "user1"
1333+
And User "user1" copies file "/sharedviewonly/document.odt" to "/copyforbidden.odt"
1334+
Then the HTTP status code should be "403"
13071335

13081336
# See sharing-v1-part3.feature

0 commit comments

Comments
 (0)