From bc23d4eb271756c55c2210306f9d92fec5fc7caf Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 25 Mar 2021 23:12:41 +0100 Subject: [PATCH] (WIP) consolidate download utilities - move registration and cleanup code to new DownloadManager - adjust files and files_sharing controllers accordingly - TODO: js code of files_sharing - TODO: tests Signed-off-by: Arthur Schiwon --- apps/files/appinfo/info.xml | 1 - apps/files/lib/Controller/AjaxController.php | 48 ++------ .../lib/Controller/ShareController.php | 86 +++++++++---- .../BackgroundJobs}/CleanupDownloadTokens.php | 27 ++-- lib/private/Files/Utils/DownloadManager.php | 116 ++++++++++++++++++ .../NC21/AddCleanupDownloadTokenJob.php | 47 +++++++ lib/public/Files/Utils/IDownloadManager.php | 64 ++++++++++ 7 files changed, 307 insertions(+), 82 deletions(-) rename {apps/files/lib/BackgroundJob => core/BackgroundJobs}/CleanupDownloadTokens.php (59%) create mode 100644 lib/private/Files/Utils/DownloadManager.php create mode 100644 lib/private/Repair/NC21/AddCleanupDownloadTokenJob.php create mode 100644 lib/public/Files/Utils/IDownloadManager.php diff --git a/apps/files/appinfo/info.xml b/apps/files/appinfo/info.xml index 504de16303..cefdfbae34 100644 --- a/apps/files/appinfo/info.xml +++ b/apps/files/appinfo/info.xml @@ -26,7 +26,6 @@ OCA\Files\BackgroundJob\ScanFiles OCA\Files\BackgroundJob\DeleteOrphanedItems OCA\Files\BackgroundJob\CleanupFileLocks - OCA\Files\BackgroundJob\CleanupDownloadTokens OCA\Files\BackgroundJob\CleanupDirectEditingTokens diff --git a/apps/files/lib/Controller/AjaxController.php b/apps/files/lib/Controller/AjaxController.php index 8e0db6e4dd..d3cea96116 100644 --- a/apps/files/lib/Controller/AjaxController.php +++ b/apps/files/lib/Controller/AjaxController.php @@ -27,18 +27,14 @@ declare(strict_types=1); namespace OCA\Files\Controller; use OC_Files; -use OCA\Files\AppInfo\Application; use OCA\Files\Helper; use OCP\AppFramework\Controller; use OCP\AppFramework\Http\JSONResponse; -use OCP\AppFramework\Http\NotFoundResponse; use OCP\Files\NotFoundException; +use OCP\Files\Utils\IDownloadManager; use OCP\IConfig; use OCP\IRequest; use OCP\ISession; -use OCP\Security\ISecureRandom; -use function json_decode; -use function json_encode; class AjaxController extends Controller { /** @var ISession */ @@ -46,21 +42,21 @@ class AjaxController extends Controller { /** @var IConfig */ private $config; - /** @var ISecureRandom */ - private $secureRandom; + /** @var IDownloadManager */ + private $downloadManager; public function __construct( string $appName, IRequest $request, ISession $session, IConfig $config, - ISecureRandom $secureRandom + IDownloadManager $downloadManager ) { parent::__construct($appName, $request); $this->session = $session; $this->request = $request; $this->config = $config; - $this->secureRandom = $secureRandom; + $this->downloadManager = $downloadManager; } /** @@ -93,25 +89,11 @@ class AjaxController extends Controller { throw new \InvalidArgumentException('Invalid argument for files'); } - $attempts = 0; - do { - if ($attempts === 10) { - throw new \RuntimeException('Failed to create unique download token'); - } - $token = $this->secureRandom->generate(15); - $attempts++; - } while ($this->config->getAppValue(Application::APP_ID, Application::DL_TOKEN_PREFIX . $token, '') !== ''); - - $this->config->setAppValue( - Application::APP_ID, - Application::DL_TOKEN_PREFIX . $token, - json_encode([ - 'files' => $files, - 'dir' => $dir, - 'downloadStartSecret' => $downloadStartSecret, - 'lastActivity' => time() - ]) - ); + $token = $this->downloadManager->register([ + 'files' => $files, + 'dir' => $dir, + 'downloadStartSecret' => $downloadStartSecret, + ]); return new JSONResponse(['token' => $token]); } @@ -121,15 +103,9 @@ class AjaxController extends Controller { * @NoCSRFRequired */ public function download(string $token) { - $dataStr = $this->config->getAppValue(Application::APP_ID, Application::DL_TOKEN_PREFIX . $token, ''); - if ($dataStr === '') { - return new NotFoundResponse(); - } - $this->session->close(); - $data = json_decode($dataStr, true); - $data['lastActivity'] = time(); - $this->config->setAppValue(Application::APP_ID, Application::DL_TOKEN_PREFIX . $token, json_encode($data)); + $data = $this->downloadManager->retrieve($token); + $this->session->close(); if (strlen($data['downloadStartSecret']) <= 32 && (preg_match('!^[a-zA-Z0-9]+$!', $data['downloadStartSecret']) === 1) diff --git a/apps/files_sharing/lib/Controller/ShareController.php b/apps/files_sharing/lib/Controller/ShareController.php index 7e83ffaa7d..79b000466b 100644 --- a/apps/files_sharing/lib/Controller/ShareController.php +++ b/apps/files_sharing/lib/Controller/ShareController.php @@ -52,17 +52,23 @@ use OCA\Files_Sharing\Event\BeforeTemplateRenderedEvent; use OCA\Viewer\Event\LoadViewer; use OCP\Accounts\IAccountManager; use OCP\AppFramework\AuthPublicShareController; +use OCP\AppFramework\Http; +use OCP\AppFramework\Http\DataResponse; +use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\Http\NotFoundResponse; +use OCP\AppFramework\Http\Response; use OCP\AppFramework\Http\Template\ExternalShareMenuAction; use OCP\AppFramework\Http\Template\LinkMenuAction; use OCP\AppFramework\Http\Template\PublicTemplateResponse; use OCP\AppFramework\Http\Template\SimpleMenuAction; use OCP\AppFramework\Http\TemplateResponse; +use OCP\Constants; use OCP\Defaults; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\Folder; use OCP\Files\IRootFolder; use OCP\Files\NotFoundException; +use OCP\Files\Utils\IDownloadManager; use OCP\IConfig; use OCP\IL10N; use OCP\ILogger; @@ -112,6 +118,8 @@ class ShareController extends AuthPublicShareController { /** @var Share\IShare */ protected $share; + /** @var IDownloadManager */ + private $downloadManager; /** * @param string $appName @@ -146,7 +154,9 @@ class ShareController extends AuthPublicShareController { IAccountManager $accountManager, IEventDispatcher $eventDispatcher, IL10N $l10n, - Defaults $defaults) { + Defaults $defaults, + IDownloadManager $downloadManager + ) { parent::__construct($appName, $request, $session, $urlGenerator); $this->config = $config; @@ -161,6 +171,7 @@ class ShareController extends AuthPublicShareController { $this->l10n = $l10n; $this->defaults = $defaults; $this->shareManager = $shareManager; + $this->downloadManager = $downloadManager; } /** @@ -387,14 +398,14 @@ class ShareController extends AuthPublicShareController { $freeSpace = (INF > 0) ? INF: PHP_INT_MAX; // work around https://bugs.php.net/bug.php?id=69188 } - $hideFileList = !($share->getPermissions() & \OCP\Constants::PERMISSION_READ); + $hideFileList = !($share->getPermissions() & Constants::PERMISSION_READ); $maxUploadFilesize = $freeSpace; $folder = new Template('files', 'list', ''); $folder->assign('dir', $shareNode->getRelativePath($folderNode->getPath())); $folder->assign('dirToken', $this->getToken()); - $folder->assign('permissions', \OCP\Constants::PERMISSION_READ); + $folder->assign('permissions', Constants::PERMISSION_READ); $folder->assign('isPublic', true); $folder->assign('hideFileList', $hideFileList); $folder->assign('publicUploadEnabled', 'no'); @@ -462,6 +473,7 @@ class ShareController extends AuthPublicShareController { \OCP\Util::addScript('files', 'fileactionsmenu'); \OCP\Util::addScript('files', 'jquery.fileupload'); \OCP\Util::addScript('files_sharing', 'files_drop'); + \OCP\Util::addScript('files', 'dist/download'); if (isset($shareTmpl['folder'])) { // JS required for folders @@ -502,7 +514,7 @@ class ShareController extends AuthPublicShareController { $response->setHeaderDetails($this->l10n->t('shared by %s', [$shareTmpl['shareOwner']])); } - $isNoneFileDropFolder = $shareIsFolder === false || $share->getPermissions() !== \OCP\Constants::PERMISSION_CREATE; + $isNoneFileDropFolder = $shareIsFolder === false || $share->getPermissions() !== Constants::PERMISSION_CREATE; if ($isNoneFileDropFolder && !$share->getHideDownload()) { \OCP\Util::addScript('files_sharing', 'public_note'); @@ -542,29 +554,54 @@ class ShareController extends AuthPublicShareController { * @PublicPage * @NoCSRFRequired * @NoSameSiteCookieRequired - * - * @param string $token - * @param string $files - * @param string $path - * @param string $downloadStartSecret - * @return void|\OCP\AppFramework\Http\Response - * @throws NotFoundException */ - public function downloadShare($token, $files = null, $path = '', $downloadStartSecret = '') { + public function registerDownload(string $token, string $files = '', string $path = '', string $downloadStartSecret = ''): Response { + \OC_User::setIncognitoMode(true); $share = $this->shareManager->getShareByToken($token); + if (!($share->getPermissions() & Constants::PERMISSION_READ)) { + return new DataResponse('Share has no read permission', Http::STATUS_FORBIDDEN); + } + if (!$this->validateShare($share)) { + return new DataResponse('Share has not been found', Http::STATUS_NOT_FOUND); + } - if (!($share->getPermissions() & \OCP\Constants::PERMISSION_READ)) { - return new \OCP\AppFramework\Http\DataResponse('Share has no read permission'); + $token = $this->downloadManager->register([ + 'token' => $token, + 'files' => $files, + 'path' => $path, + 'downloadStartSecret' => $downloadStartSecret, + ]); + + return new JSONResponse(['token' => $token]); + } + + /** + * @PublicPage + * @NoCSRFRequired + * @NoSameSiteCookieRequired + * + * @return void|\OCP\AppFramework\Http\Response + * @throws NotFoundException + */ + public function downloadShare(string $downloadToken) { + $data = $this->downloadManager->retrieve($downloadToken); + + \OC_User::setIncognitoMode(true); + + $share = $this->shareManager->getShareByToken($data['token']); + + if (!($share->getPermissions() & Constants::PERMISSION_READ)) { + return new DataResponse('Share has no read permission'); } $files_list = null; - if (!is_null($files)) { // download selected files - $files_list = json_decode($files); + if (!is_null($data['files'])) { // download selected files + $files_list = json_decode($data['files']); // in case we get only a single file if ($files_list === null) { - $files_list = [$files]; + $files_list = [$data['files']]; } // Just in case $files is a single int like '1234' if (!is_array($files_list)) { @@ -579,7 +616,6 @@ class ShareController extends AuthPublicShareController { $userFolder = $this->rootFolder->getUserFolder($share->getShareOwner()); $originalSharePath = $userFolder->getRelativePath($share->getNode()->getPath()); - // Single file share if ($share->getNode() instanceof \OCP\Files\File) { // Single file download @@ -591,9 +627,9 @@ class ShareController extends AuthPublicShareController { $node = $share->getNode(); // Try to get the path - if ($path !== '') { + if ($data['path'] !== '') { try { - $node = $node->get($path); + $node = $node->get($data['path']); } catch (NotFoundException $e) { $this->emitAccessShareHook($share, 404, 'Share not found'); return new NotFoundResponse(); @@ -628,12 +664,12 @@ class ShareController extends AuthPublicShareController { * the content must not be longer than 32 characters and must only contain * alphanumeric characters */ - if (!empty($downloadStartSecret) - && !isset($downloadStartSecret[32]) - && preg_match('!^[a-zA-Z0-9]+$!', $downloadStartSecret) === 1) { + if (!empty($data['downloadStartSecret']) + && !isset($data['downloadStartSecret'][32]) + && preg_match('!^[a-zA-Z0-9]+$!', $data['downloadStartSecret']) === 1) { // FIXME: set on the response once we use an actual app framework response - setcookie('ocDownloadStarted', $downloadStartSecret, time() + 20, '/'); + setcookie('ocDownloadStarted', $data['downloadStartSecret'], time() + 20, '/'); } $this->emitAccessShareHook($share); @@ -648,7 +684,7 @@ class ShareController extends AuthPublicShareController { } // download selected files - if (!is_null($files) && $files !== '') { + if (!is_null($data['files']) && $data['files'] !== '') { // FIXME: The exit is required here because otherwise the AppFramework is trying to add headers as well // after dispatching the request which results in a "Cannot modify header information" notice. OC_Files::get($originalSharePath, $files_list, $server_params); diff --git a/apps/files/lib/BackgroundJob/CleanupDownloadTokens.php b/core/BackgroundJobs/CleanupDownloadTokens.php similarity index 59% rename from apps/files/lib/BackgroundJob/CleanupDownloadTokens.php rename to core/BackgroundJobs/CleanupDownloadTokens.php index 900cd46dcf..f9fb9a61cf 100644 --- a/apps/files/lib/BackgroundJob/CleanupDownloadTokens.php +++ b/core/BackgroundJobs/CleanupDownloadTokens.php @@ -22,39 +22,26 @@ declare(strict_types=1); * */ -namespace OCA\files\lib\BackgroundJob; +namespace OC\Core\BackgroundJobs; use OC\BackgroundJob\TimedJob; -use OCA\Files\AppInfo\Application; +use OC\Files\Utils\DownloadManager; use OCP\IConfig; class CleanupDownloadTokens extends TimedJob { private const INTERVAL_MINUTES = 24 * 60; /** @var IConfig */ private $config; + /** @var DownloadManager */ + private $downloadManager; - public function __construct(IConfig $config) { + public function __construct(IConfig $config, DownloadManager $downloadManager) { $this->interval = self::INTERVAL_MINUTES; $this->config = $config; + $this->downloadManager = $downloadManager; } protected function run($argument) { - $appKeys = $this->config->getAppKeys(Application::APP_ID); - foreach ($appKeys as $key) { - if (strpos($key, Application::DL_TOKEN_PREFIX) !== 0) { - continue; - } - $dataStr = $this->config->getAppValue(Application::APP_ID, $key, ''); - if ($dataStr === '') { - $this->config->deleteAppValue(Application::APP_ID, $key); - continue; - } - $data = \json_decode($dataStr, true); - if (!isset($data['lastActivity']) || (time() - $data['lastActivity']) > 24 * 60 * 2) { - // deletes tokens that have not seen activity for 2 days - // the period is chosen to allow continue of downloads with network interruptions in minde - $this->config->deleteAppValue(Application::APP_ID, $key); - } - } + $this->downloadManager->cleanupTokens(); } } diff --git a/lib/private/Files/Utils/DownloadManager.php b/lib/private/Files/Utils/DownloadManager.php new file mode 100644 index 0000000000..f5c7991b94 --- /dev/null +++ b/lib/private/Files/Utils/DownloadManager.php @@ -0,0 +1,116 @@ + + * + * @author Arthur Schiwon + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OC\Files\Utils; + +use OCP\Files\NotFoundException; +use OCP\Files\Utils\IDownloadManager; +use OCP\IConfig; +use OCP\Security\ISecureRandom; +use RuntimeException; +use function json_decode; +use function json_encode; + +class DownloadManager implements IDownloadManager { + /** + * Lifetime of tokens. Period of 2 days are chosen to allow continuation + * of downloads with network interruptions in mind + */ + protected const TOKEN_TTL = 24 * 60 * 2; + protected const TOKEN_PREFIX = 'dl_token_'; + + protected const FIELD_DATA = 'downloadData'; + protected const FIELD_ACTIVITY = 'lastActivity'; + + /** @var IConfig */ + private $config; + /** @var ISecureRandom */ + private $secureRandom; + + public function __construct(IConfig $config, ISecureRandom $secureRandom) { + $this->config = $config; + $this->secureRandom = $secureRandom; + } + + /** + * @inheritDoc + */ + public function register(array $data): string { + $attempts = 0; + do { + if ($attempts === 10) { + throw new RuntimeException('Failed to create unique download token'); + } + $token = $this->secureRandom->generate(15); + $attempts++; + } while ($this->config->getAppValue('core', self::TOKEN_PREFIX . $token, '') !== ''); + + $this->config->setAppValue( + 'core', + self::TOKEN_PREFIX . $token, + json_encode([ + self::FIELD_DATA => $data, + self::FIELD_ACTIVITY => time() + ]) + ); + + return $token; + } + + /** + * @inheritDoc + */ + public function retrieve(string $token): array { + $dataStr = $this->config->getAppValue('core', self::TOKEN_PREFIX . $token, ''); + if ($dataStr === '') { + throw new NotFoundException(); + } + + $data = json_decode($dataStr, true); + $data[self::FIELD_ACTIVITY] = time(); + $this->config->setAppValue('core', self::TOKEN_PREFIX . $token, json_encode($data)); + + return $data[self::FIELD_DATA]; + } + + public function cleanupTokens(): void { + $appKeys = $this->config->getAppKeys('core'); + foreach ($appKeys as $key) { + if (strpos($key, self::TOKEN_PREFIX) !== 0) { + continue; + } + $dataStr = $this->config->getAppValue('core', $key, ''); + if ($dataStr === '') { + $this->config->deleteAppValue('core', $key); + continue; + } + $data = json_decode($dataStr, true); + if (!isset($data[self::FIELD_ACTIVITY]) + || (time() - $data[self::FIELD_ACTIVITY]) > self::TOKEN_TTL + ) { + $this->config->deleteAppValue('core', $key); + } + } + } +} diff --git a/lib/private/Repair/NC21/AddCleanupDownloadTokenJob.php b/lib/private/Repair/NC21/AddCleanupDownloadTokenJob.php new file mode 100644 index 0000000000..1be3b76a52 --- /dev/null +++ b/lib/private/Repair/NC21/AddCleanupDownloadTokenJob.php @@ -0,0 +1,47 @@ + + * + * @author Arthur Schiwon + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OC\Repair\NC21; + +use OC\Core\BackgroundJobs\CleanupDownloadTokens; +use OCP\BackgroundJob\IJobList; +use OCP\Migration\IOutput; +use OCP\Migration\IRepairStep; + +class AddCleanupDownloadTokenJob implements IRepairStep { + /** @var IJobList */ + protected $jobList; + + public function __construct(IJobList $jobList) { + $this->jobList = $jobList; + } + + public function getName(): string { + return 'Add background job to cleanup download tokens'; + } + + public function run(IOutput $output) { + $this->jobList->add(CleanupDownloadTokens::class); + } +} diff --git a/lib/public/Files/Utils/IDownloadManager.php b/lib/public/Files/Utils/IDownloadManager.php new file mode 100644 index 0000000000..b2ac2fadc8 --- /dev/null +++ b/lib/public/Files/Utils/IDownloadManager.php @@ -0,0 +1,64 @@ + + * + * @author Arthur Schiwon + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OCP\Files\Utils; + +use OCP\Files\NotFoundException; +use RuntimeException; + +/** + * Interface IDownloadManager + * + * The usual process is to prepare a file download via POST request. Follow + * up with a GET request providing the token for best browser integration. + * Announcing the download via POST enables to hide the payload in the + * request body rather then the URL, which also lifts limitations on + * data length. + * + * @package OCP\Files + * + * @since 22.0.0 + */ +interface IDownloadManager { + + /** + * Register download data and receive a token to access it later on. + * + * The provided data will be returned on retrieve() again. The structure + * is up to the consumer, it is not being processed, but only stored by + * the manager. + * + * @throws RuntimeException + * @since 22.0.0 + */ + public function register(array $data): string; + + /** + * Retrieves the download data for the provided token. + * + * @throws NotFoundException + * @since 22.0.0 + */ + public function retrieve(string $token): array; +}