Skip to content
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.

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

Block file access for secure view #2776

Merged
merged 3 commits into from
Dec 21, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 15 additions & 0 deletions cypress/e2e/share.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import { randUser } from '../utils/index.js'

const user = randUser()
const recipient = randUser()

describe('Open test.md in viewer', function() {
before(function() {
Expand All @@ -37,6 +38,8 @@ describe('Open test.md in viewer', function() {
cy.uploadFile('test.md', 'text/markdown', 'folder/Readme.md')
cy.uploadFile('test.md', 'text/markdown', 'test2.md')
cy.uploadFile('test.md', 'text/markdown')

cy.createUser(recipient)
})
beforeEach(function() {
cy.login(user)
Expand Down Expand Up @@ -120,4 +123,16 @@ describe('Open test.md in viewer', function() {
})
})

it('Share a file with download disabled shows an error', function() {
cy.shareFileToUser('test.md', recipient, {
attributes: '[{"scope":"permissions","key":"download","enabled":false}]',
}).then(() => {
cy.login(recipient)
cy.visit('/apps/files')
cy.openFile('test.md')
cy.getModal().find('.empty-content__title').should('contain', 'Failed to load file')
cy.getModal().getContent().should('not.exist')
})
})

})
3 changes: 2 additions & 1 deletion cypress/support/commands.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ Cypress.Commands.add('createFile', (target, content, mimeType = 'text/markdown')

})

Cypress.Commands.add('shareFileToUser', (path, targetUser) => {
Cypress.Commands.add('shareFileToUser', (path, targetUser, shareData = {}) => {
cy.clearCookies()
cy.ocsRequest({
method: 'POST',
Expand All @@ -104,6 +104,7 @@ Cypress.Commands.add('shareFileToUser', (path, targetUser) => {
path,
shareType: 0,
shareWith: targetUser.userId,
...shareData,
},
}).then(response => {
cy.log(`${auth.user} shared ${path} with ${targetUser.userId}`, response.status)
Expand Down
4 changes: 2 additions & 2 deletions js/editor.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion js/editor.js.map

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions js/files-modal.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion js/files-modal.js.map

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions js/text-files.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion js/text-files.js.map

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions js/text-public.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion js/text-public.js.map

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions js/text-text.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion js/text-text.js.map

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions js/text-viewer.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion js/text-viewer.js.map

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions js/vendors.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion js/vendors.js.map

Large diffs are not rendered by default.

23 changes: 22 additions & 1 deletion lib/Service/ApiService.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

use Exception;
use OC\Files\Node\File;
use OCA\Files_Sharing\SharedStorage;
use OCA\Text\AppInfo\Application;
use OCA\Text\Exception\DocumentHasUnsavedChangesException;
use OCA\Text\Exception\DocumentSaveConflictException;
Expand All @@ -41,8 +42,11 @@
use OCP\Constants;
use OCP\Files\Lock\ILock;
use OCP\Files\NotFoundException;
use OCP\Files\NotPermittedException;
use OCP\IL10N;
use OCP\IRequest;
use OCP\Lock\LockedException;
use OCP\Share\IShare;
use Psr\Log\LoggerInterface;

class ApiService {
Expand All @@ -52,19 +56,23 @@ class ApiService {
private LoggerInterface $logger;
private AttachmentService $attachmentService;
private EncodingService $encodingService;
private IL10N $l10n;

public function __construct(IRequest $request,
SessionService $sessionService,
DocumentService $documentService,
AttachmentService $attachmentService,
EncodingService $encodingService,
LoggerInterface $logger) {
LoggerInterface $logger,
IL10N $l10n
) {
$this->request = $request;
$this->sessionService = $sessionService;
$this->documentService = $documentService;
$this->logger = $logger;
$this->attachmentService = $attachmentService;
$this->encodingService = $encodingService;
$this->l10n = $l10n;
}

public function create($fileId = null, $filePath = null, $token = null, $guestName = null, bool $forceRecreate = false): DataResponse {
Expand All @@ -81,13 +89,26 @@ public function create($fileId = null, $filePath = null, $token = null, $guestNa
$this->documentService->checkSharePermissions($token, Constants::PERMISSION_READ);
} catch (NotFoundException $e) {
return new DataResponse([], Http::STATUS_NOT_FOUND);
} catch (NotPermittedException $e) {
return new DataResponse($this->l10n->t('This file cannot be displayed as download is disabled by the share'), 404);
}
} elseif ($fileId) {
$file = $this->documentService->getFileById($fileId);
} else {
return new DataResponse('No valid file argument provided', 500);
}

$storage = $file->getStorage();

// Block using text for disabled download internal shares
if ($storage->instanceOfStorage(SharedStorage::class)) {
/** @var IShare $share */
$share = $storage->getShare();
if ($share->getAttributes()->getAttribute('permissions', 'download') === false) {
return new DataResponse($this->l10n->t('This file cannot be displayed as download is disabled by the share'), 403);
}
}

$readOnly = $this->documentService->isReadOnly($file, $token);

$this->sessionService->removeInactiveSessions($file->getId());
Expand Down
6 changes: 5 additions & 1 deletion lib/Service/DocumentService.php
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,7 @@ public function getLockInfo($file): ?ILock {
/**
* @param $shareToken
* @return void
* @throws NotFoundException
* @throws NotFoundException|NotPermittedException
*/
public function checkSharePermissions($shareToken, $permission = Constants::PERMISSION_READ): void {
try {
Expand All @@ -494,6 +494,10 @@ public function checkSharePermissions($shareToken, $permission = Constants::PERM
if (($share->getPermissions() & $permission) === 0) {
throw new NotFoundException();
}

if ($share->getHideDownload()) {
throw new NotPermittedException();
}
}

public function hasUnsavedChanges(Document $document) {
Expand Down
20 changes: 17 additions & 3 deletions src/components/Editor.vue
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,11 @@
-->

<template>
<div data-text-el="editor-container" class="text-editor" @keydown.esc.stop.prevent="() => {}">
<DocumentStatus v-if="displayed"
<div id="editor-container"
data-text-el="editor-container"
class="text-editor"
@keydown.esc.stop.prevent="() => {}">
<DocumentStatus v-if="displayedStatus"
:idle="idle"
:lock="lock"
:sync-error="syncError"
Expand Down Expand Up @@ -276,6 +279,9 @@ export default {
displayed() {
return this.currentSession && this.active
},
displayedStatus() {
return this.displayed || !!this.syncError
},
renderRichEditorMenus() {
return this.contentLoaded
&& this.isRichEditor
Expand Down Expand Up @@ -611,12 +617,19 @@ export default {
},

onError({ type, data }) {
this.$editor.setOptions({ editable: false })

this.$nextTick(() => {
this.$editor?.setEditable(false)
this.$emit('sync-service:error')
})

if (type === ERROR_TYPE.LOAD_ERROR) {
this.syncError = {
type,
data,
}
}

if (type === ERROR_TYPE.SAVE_COLLISSION && (!this.syncError || this.syncError.type !== ERROR_TYPE.SAVE_COLLISSION)) {
this.contentLoaded = true
this.syncError = {
Expand All @@ -635,6 +648,7 @@ export default {
if (type === ERROR_TYPE.SOURCE_NOT_FOUND) {
this.hasConnectionIssue = true
}

this.$emit('ready')
},

Expand Down
18 changes: 17 additions & 1 deletion src/components/Editor/DocumentStatus.vue
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,24 @@

<template>
<div class="document-status">
<p v-if="idle" class="msg">
<NcEmptyContent v-if="isLoadingError" :title="t('text', 'Failed to load file')" :description="syncError.data.data">
<template #icon>
<AlertOctagonOutline />
</template>
</NcEmptyContent>

<p v-else-if="idle" class="msg">
{{ t('text', 'Document idle for {timeout} minutes, click to continue editing', { timeout: IDLE_TIMEOUT }) }} <a class="button primary" @click="reconnect">{{ t('text', 'Reconnect') }}</a>
</p>

<p v-else-if="hasSyncCollission" class="msg icon-error">
{{ t('text', 'The document has been changed outside of the editor. The changes cannot be applied.') }}
</p>

<p v-else-if="hasConnectionIssue" class="msg">
{{ t('text', 'File could not be loaded. Please check your internet connection.') }} <a class="button primary" @click="reconnect">{{ t('text', 'Reconnect') }}</a>
</p>

<p v-if="lock" class="msg msg-locked">
<Lock /> {{ t('text', 'This file is opened read-only as it is currently locked by {user}.', { user: lock.displayName }) }}
</p>
Expand All @@ -40,13 +49,17 @@
<script>

import { ERROR_TYPE, IDLE_TIMEOUT } from './../../services/SyncService.js'
import AlertOctagonOutline from 'vue-material-design-icons/AlertOctagonOutline.vue'
import Lock from 'vue-material-design-icons/Lock.vue'
import { NcEmptyContent } from '@nextcloud/vue'

export default {
name: 'DocumentStatus',

components: {
AlertOctagonOutline,
Lock,
NcEmptyContent,
},

props: {
Expand Down Expand Up @@ -78,6 +91,9 @@ export default {
hasSyncCollission() {
return this.syncError && this.syncError.type === ERROR_TYPE.SAVE_COLLISSION
},
isLoadingError() {
return this.syncError && this.syncError.type === ERROR_TYPE.LOAD_ERROR
},
},

methods: {
Expand Down
2 changes: 1 addition & 1 deletion src/services/SyncService.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ class SyncService {
if (!error.response || error.code === 'ECONNABORTED') {
this.emit('error', { type: ERROR_TYPE.CONNECTION_FAILED, data: {} })
} else {
this.emit('error', { type: ERROR_TYPE.LOAD_ERROR, data: error.response.status })
this.emit('error', { type: ERROR_TYPE.LOAD_ERROR, data: error.response })
}
throw error
})
Expand Down
5 changes: 5 additions & 0 deletions tests/stub.phpstub
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,8 @@ namespace OC\User {
class NoUserException extends \Exception {}
}

namespace OCA\Files_Sharing {
abstract class SharedStorage implements \OCP\Files\Storage\IStorage {
abstract public function getShare(): \OCP\Share\IShare;
}
}