Skip to content

feat(files_sharing): allow viewing files with download disabled #53671

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
skjnldsv merged 2 commits into from
Jul 1, 2025
Merged

feat(files_sharing): allow viewing files with download disabled #53671

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9806a98
feat(files_sharing): allow viewing files with download disabled
skjnldsv Jun 25, 2025
20a50ea
fix: invalid bool return
skjnldsv Jun 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions apps/dav/lib/DAV/ViewOnlyPlugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,18 +84,25 @@ public function checkViewOnly(RequestInterface $request): bool {
if (!$storage->instanceOfStorage(ISharedStorage::class)) {
return true;
}

// Extract extra permissions
/** @var ISharedStorage $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.
// We have two options here, if download is disabled, but viewing is allowed,
// we still allow the GET request to return the file content.
$canDownload = $attributes->getAttribute('permissions', 'download');
if ($canDownload !== null && !$canDownload) {
if (!$share->canSeeContent()) {
throw new Forbidden('Access to this shared resource has been denied because its download permission is disabled.');
}

// If download is disabled, we disable the COPY and MOVE methods even if the
// shareapi_allow_view_without_download is set to true.
if ($request->getMethod() !== 'GET' && ($canDownload !== null && !$canDownload)) {
throw new Forbidden('Access to this shared resource has been denied because its download permission is disabled.');
}
} catch (NotFound $e) {
Expand Down
28 changes: 19 additions & 9 deletions apps/dav/tests/unit/DAV/ViewOnlyPluginTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,24 +74,30 @@ public function testCanGetNonShared(): void {
public static function providesDataForCanGet(): array {
return [
// has attribute permissions-download enabled - can get file
[false, true, true],
[false, true, true, true],
// has no attribute permissions-download - can get file
[false, null, true],
// has attribute permissions-download disabled- cannot get the file
[false, false, false],
[false, null, true, true],
// has attribute permissions-download enabled - can get file version
[true, true, true],
[true, true, true, true],
// has no attribute permissions-download - can get file version
[true, null, true],
// has attribute permissions-download disabled- cannot get the file version
[true, false, false],
[true, null, true, true],
// has attribute permissions-download disabled - cannot get the file
[false, false, false, false],
// has attribute permissions-download disabled - cannot get the file version
[true, false, false, false],

// Has global allowViewWithoutDownload option enabled
// has attribute permissions-download disabled - can get file
[false, false, false, true],
// has attribute permissions-download disabled - can get file version
[true, false, false, true],
];
}

/**
* @dataProvider providesDataForCanGet
*/
public function testCanGet(bool $isVersion, ?bool $attrEnabled, bool $expectCanDownloadFile): void {
public function testCanGet(bool $isVersion, ?bool $attrEnabled, bool $expectCanDownloadFile, bool $allowViewWithoutDownload): void {
$nodeInfo = $this->createMock(File::class);
if ($isVersion) {
$davPath = 'versions/alice/versions/117/123456';
Expand Down Expand Up @@ -150,6 +156,10 @@ public function testCanGet(bool $isVersion, ?bool $attrEnabled, bool $expectCanD
->method('getAttribute')
->with('permissions', 'download')
->willReturn($attrEnabled);

$share->expects($this->once())
->method('canSeeContent')
->willReturn($allowViewWithoutDownload);

if (!$expectCanDownloadFile) {
$this->expectException(Forbidden::class);
Expand Down
7 changes: 4 additions & 3 deletions apps/files/lib/Controller/ApiController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,11 +105,12 @@ public function getThumbnail($x, $y, $file) {
}

// Validate the user is allowed to download the file (preview is some kind of download)
/** @var ISharedStorage $storage */
$storage = $file->getStorage();
if ($storage->instanceOfStorage(ISharedStorage::class)) {
/** @var ISharedStorage $storage */
$attributes = $storage->getShare()->getAttributes();
if ($attributes !== null && $attributes->getAttribute('permissions', 'download') === false) {
/** @var IShare $share */
Copy link
Contributor

@come-nc come-nc Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** @var IShare $share */

Copy link
Member Author

@skjnldsv skjnldsv Jun 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, weirdly it doesn't fully understand the real instance of $storage for me 🤷
So I prefer to keep it

$share = $storage->getShare();
if (!$share->canSeeContent()) {
throw new NotFoundException();
}
}
Expand Down
15 changes: 4 additions & 11 deletions apps/files/tests/Controller/ApiControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
use OCP\IRequest;
use OCP\IUser;
use OCP\IUserSession;
use OCP\Share\IAttributes;
use OCP\Share\IManager;
use OCP\Share\IShare;
use PHPUnit\Framework\MockObject\MockObject;
Expand Down Expand Up @@ -183,16 +182,10 @@ public function testGetThumbnailInvalidPartFile(): void {
}

public function testGetThumbnailSharedNoDownload(): void {
$attributes = $this->createMock(IAttributes::class);
$attributes->expects(self::once())
->method('getAttribute')
->with('permissions', 'download')
->willReturn(false);

$share = $this->createMock(IShare::class);
$share->expects(self::once())
->method('getAttributes')
->willReturn($attributes);
->method('canSeeContent')
->willReturn(false);

$storage = $this->createMock(ISharedStorage::class);
$storage->expects(self::once())
Expand Down Expand Up @@ -221,8 +214,8 @@ public function testGetThumbnailSharedNoDownload(): void {
public function testGetThumbnailShared(): void {
$share = $this->createMock(IShare::class);
$share->expects(self::once())
->method('getAttributes')
->willReturn(null);
->method('canSeeContent')
->willReturn(true);

$storage = $this->createMock(ISharedStorage::class);
$storage->expects(self::once())
Expand Down
7 changes: 3 additions & 4 deletions apps/files_sharing/lib/Controller/PublicPreviewController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,9 @@ public function getPreview(
return new DataResponse([], Http::STATUS_FORBIDDEN);
}

$attributes = $share->getAttributes();
// Only explicitly set to false will forbid the download!
$downloadForbidden = $attributes?->getAttribute('permissions', 'download') === false;
$downloadForbidden = !$share->canSeeContent();

// Is this header is set it means our UI is doing a preview for no-download shares
// we check a header so we at least prevent people from using the link directly (obfuscation)
$isPublicPreview = $this->request->getHeader('x-nc-preview') === 'true';
Expand Down Expand Up @@ -181,8 +181,7 @@ public function directLink(string $token) {
return new DataResponse([], Http::STATUS_FORBIDDEN);
}

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

Expand Down
28 changes: 12 additions & 16 deletions apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
use OCP\ISession;
use OCP\Preview\IMimeIconProvider;
use OCP\Share\Exceptions\ShareNotFound;
use OCP\Share\IAttributes;
use OCP\Share\IManager;
use OCP\Share\IShare;
use PHPUnit\Framework\MockObject\MockObject;
Expand Down Expand Up @@ -114,12 +113,8 @@ public function testShareNoDownload() {
$share->method('getPermissions')
->willReturn(Constants::PERMISSION_READ);

$attributes = $this->createMock(IAttributes::class);
$attributes->method('getAttribute')
->with('permissions', 'download')
$share->method('canSeeContent')
->willReturn(false);
$share->method('getAttributes')
->willReturn($attributes);

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

$attributes = $this->createMock(IAttributes::class);
$attributes->method('getAttribute')
->with('permissions', 'download')
$share->method('canSeeContent')
->willReturn(false);
$share->method('getAttributes')
->willReturn($attributes);

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

$attributes = $this->createMock(IAttributes::class);
$attributes->method('getAttribute')
->with('permissions', 'download')
$share->method('canSeeContent')
->willReturn(true);
$share->method('getAttributes')
->willReturn($attributes);

$this->request->method('getHeader')
->with('x-nc-preview')
Expand Down Expand Up @@ -220,6 +207,9 @@ public function testPreviewFile() {
$share->method('getNode')
->willReturn($file);

$share->method('canSeeContent')
->willReturn(true);

$preview = $this->createMock(ISimpleFile::class);
$preview->method('getName')->willReturn('name');
$preview->method('getMTime')->willReturn(42);
Expand Down Expand Up @@ -249,6 +239,9 @@ public function testPreviewFolderInvalidFile(): void {
$share->method('getNode')
->willReturn($folder);

$share->method('canSeeContent')
->willReturn(true);

$folder->method('get')
->with($this->equalTo('file'))
->willThrowException(new NotFoundException());
Expand All @@ -272,6 +265,9 @@ public function testPreviewFolderValidFile(): void {
$share->method('getNode')
->willReturn($folder);

$share->method('canSeeContent')
->willReturn(true);

$file = $this->createMock(File::class);
$folder->method('get')
->with($this->equalTo('file'))
Expand Down
1 change: 1 addition & 0 deletions apps/settings/lib/Settings/Admin/Sharing.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ public function getForm() {
'remoteExpireAfterNDays' => $this->config->getAppValue('core', 'shareapi_remote_expire_after_n_days', '7'),
'enforceRemoteExpireDate' => $this->getHumanBooleanConfig('core', 'shareapi_enforce_remote_expire_date'),
'allowCustomTokens' => $this->shareManager->allowCustomTokens(),
'allowViewWithoutDownload' => $this->shareManager->allowViewWithoutDownload(),
];

$this->initialState->provideInitialState('sharingAppEnabled', $this->appManager->isEnabledForUser('files_sharing'));
Expand Down
10 changes: 10 additions & 0 deletions apps/settings/src/components/AdminSettingsSharingForm.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,15 @@
:label="t('settings', 'Ignore the following groups when checking group membership')"
style="width: 100%" />
</div>
<NcCheckboxRadioSwitch :checked.sync="settings.allowViewWithoutDownload">
{{ t('settings', 'Allow users to preview files even if download is disabled') }}
</NcCheckboxRadioSwitch>
<NcNoteCard v-show="settings.allowViewWithoutDownload"
id="settings-sharing-api-view-without-download-hint"
class="sharing__note"
type="warning">
{{ t('settings', 'Users will still be able to screenshot or record the screen. This does not provide any definitive protection.') }}
</NcNoteCard>
</div>

<div v-show="settings.enabled" id="settings-sharing-api" class="sharing__section">
Expand Down Expand Up @@ -258,6 +267,7 @@ interface IShareSettings {
remoteExpireAfterNDays: string
enforceRemoteExpireDate: boolean
allowCustomTokens: boolean
allowViewWithoutDownload: boolean
}

export default defineComponent({
Expand Down
1 change: 1 addition & 0 deletions build/integration/features/bootstrap/SharingContext.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ protected function resetAppConfigs() {
$this->deleteServerConfig('core', 'shareapi_expire_after_n_days');
$this->deleteServerConfig('core', 'link_defaultExpDays');
$this->deleteServerConfig('files_sharing', 'outgoing_server2server_share_enabled');
$this->deleteServerConfig('core', 'shareapi_allow_view_without_download');

$this->runOcc(['config:system:delete', 'share_folder']);
}
Expand Down
34 changes: 31 additions & 3 deletions build/integration/sharing_features/sharing-v1-part2.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1265,7 +1265,9 @@ Feature: sharing
|{http://open-collaboration-services.org/ns}share-permissions |
Then the single response should contain a property "{http://open-collaboration-services.org/ns}share-permissions" with value "19"

Scenario: Cannot download a file when it's shared view-only
Scenario: Cannot download a file when it's shared view-only without shareapi_allow_view_without_download
Given As an "admin"
And parameter "shareapi_allow_view_without_download" of app "core" is set to "no"
Given user "user0" exists
And user "user1" exists
And User "user0" moves file "/textfile0.txt" to "/document.odt"
Expand All @@ -1274,8 +1276,15 @@ Feature: sharing
When As an "user1"
And Downloading file "/document.odt"
Then the HTTP status code should be "403"
Then As an "admin"
And parameter "shareapi_allow_view_without_download" of app "core" is set to "yes"
Then As an "user1"
And Downloading file "/document.odt"
Then the HTTP status code should be "200"

Scenario: Cannot download a file when its parent is shared view-only
Scenario: Cannot download a file when its parent is shared view-only without shareapi_allow_view_without_download
Given As an "admin"
And parameter "shareapi_allow_view_without_download" of app "core" is set to "no"
Given user "user0" exists
And user "user1" exists
And User "user0" created a folder "/sharedviewonly"
Expand All @@ -1285,17 +1294,31 @@ Feature: sharing
When As an "user1"
And Downloading file "/sharedviewonly/document.odt"
Then the HTTP status code should be "403"
Then As an "admin"
And parameter "shareapi_allow_view_without_download" of app "core" is set to "yes"
Then As an "user1"
And Downloading file "/sharedviewonly/document.odt"
Then the HTTP status code should be "200"

Scenario: Cannot copy a file when it's shared view-only
Scenario: Cannot copy a file when it's shared view-only even with shareapi_allow_view_without_download enabled
Given As an "admin"
And parameter "shareapi_allow_view_without_download" of app "core" is set to "no"
Given user "user0" exists
And user "user1" exists
And User "user0" moves file "/textfile0.txt" to "/document.odt"
And file "document.odt" of user "user0" is shared with user "user1" view-only
And user "user1" accepts last share
When User "user1" copies file "/document.odt" to "/copyforbidden.odt"
Then the HTTP status code should be "403"
Then As an "admin"
And parameter "shareapi_allow_view_without_download" of app "core" is set to "yes"
Then As an "user1"
And User "user1" copies file "/document.odt" to "/copyforbidden.odt"
Then the HTTP status code should be "403"

Scenario: Cannot copy a file when its parent is shared view-only
Given As an "admin"
And parameter "shareapi_allow_view_without_download" of app "core" is set to "no"
Given user "user0" exists
And user "user1" exists
And User "user0" created a folder "/sharedviewonly"
Expand All @@ -1304,5 +1327,10 @@ Feature: sharing
And user "user1" accepts last share
When User "user1" copies file "/sharedviewonly/document.odt" to "/copyforbidden.odt"
Then the HTTP status code should be "403"
Then As an "admin"
And parameter "shareapi_allow_view_without_download" of app "core" is set to "yes"
Then As an "user1"
And User "user1" copies file "/sharedviewonly/document.odt" to "/copyforbidden.odt"
Then the HTTP status code should be "403"

# See sharing-v1-part3.feature
5 changes: 1 addition & 4 deletions core/Controller/PreviewController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,7 @@ private function fetchPreview(
if ($isNextcloudPreview === false && $storage->instanceOfStorage(ISharedStorage::class)) {
/** @var ISharedStorage $storage */
$share = $storage->getShare();
$attributes = $share->getAttributes();
// No "allow preview" header set, so we must check if
// the share has not explicitly disabled download permissions
if ($attributes?->getAttribute('permissions', 'download') === false) {
if (!$share->canSeeContent()) {
return new DataResponse([], Http::STATUS_FORBIDDEN);
}
}
Expand Down
Loading
Loading