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

Allow app to register a download provider #34956

Closed
wants to merge 3 commits into from
Closed
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
6 changes: 6 additions & 0 deletions apps/files/appinfo/routes.php
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,12 @@
'url' => '/api/v1/openlocaleditor/{token}',
'verb' => 'POST',
],
[
/** @see DownloadController::index() */
'name' => 'Download#index',
'url' => '/api/v1/download',
'verb' => 'GET',
],
],
]
);
2 changes: 2 additions & 0 deletions apps/files/composer/composer/autoload_classmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
'OCA\\Files\\Controller\\ApiController' => $baseDir . '/../lib/Controller/ApiController.php',
'OCA\\Files\\Controller\\DirectEditingController' => $baseDir . '/../lib/Controller/DirectEditingController.php',
'OCA\\Files\\Controller\\DirectEditingViewController' => $baseDir . '/../lib/Controller/DirectEditingViewController.php',
'OCA\\Files\\Controller\\DownloadController' => $baseDir . '/../lib/Controller/DownloadController.php',
'OCA\\Files\\Controller\\OpenLocalEditorController' => $baseDir . '/../lib/Controller/OpenLocalEditorController.php',
'OCA\\Files\\Controller\\TemplateController' => $baseDir . '/../lib/Controller/TemplateController.php',
'OCA\\Files\\Controller\\TransferOwnershipController' => $baseDir . '/../lib/Controller/TransferOwnershipController.php',
Expand All @@ -53,6 +54,7 @@
'OCA\\Files\\Migration\\Version11301Date20191205150729' => $baseDir . '/../lib/Migration/Version11301Date20191205150729.php',
'OCA\\Files\\Migration\\Version12101Date20221011153334' => $baseDir . '/../lib/Migration/Version12101Date20221011153334.php',
'OCA\\Files\\Notification\\Notifier' => $baseDir . '/../lib/Notification/Notifier.php',
'OCA\\Files\\Provider\\FileDownloadProvider' => $baseDir . '/../lib/Provider/FileDownloadProvider.php',
'OCA\\Files\\Search\\FilesSearchProvider' => $baseDir . '/../lib/Search/FilesSearchProvider.php',
'OCA\\Files\\Service\\DirectEditingService' => $baseDir . '/../lib/Service/DirectEditingService.php',
'OCA\\Files\\Service\\OwnershipTransferService' => $baseDir . '/../lib/Service/OwnershipTransferService.php',
Expand Down
2 changes: 2 additions & 0 deletions apps/files/composer/composer/autoload_static.php
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ class ComposerStaticInitFiles
'OCA\\Files\\Controller\\ApiController' => __DIR__ . '/..' . '/../lib/Controller/ApiController.php',
'OCA\\Files\\Controller\\DirectEditingController' => __DIR__ . '/..' . '/../lib/Controller/DirectEditingController.php',
'OCA\\Files\\Controller\\DirectEditingViewController' => __DIR__ . '/..' . '/../lib/Controller/DirectEditingViewController.php',
'OCA\\Files\\Controller\\DownloadController' => __DIR__ . '/..' . '/../lib/Controller/DownloadController.php',
'OCA\\Files\\Controller\\OpenLocalEditorController' => __DIR__ . '/..' . '/../lib/Controller/OpenLocalEditorController.php',
'OCA\\Files\\Controller\\TemplateController' => __DIR__ . '/..' . '/../lib/Controller/TemplateController.php',
'OCA\\Files\\Controller\\TransferOwnershipController' => __DIR__ . '/..' . '/../lib/Controller/TransferOwnershipController.php',
Expand All @@ -68,6 +69,7 @@ class ComposerStaticInitFiles
'OCA\\Files\\Migration\\Version11301Date20191205150729' => __DIR__ . '/..' . '/../lib/Migration/Version11301Date20191205150729.php',
'OCA\\Files\\Migration\\Version12101Date20221011153334' => __DIR__ . '/..' . '/../lib/Migration/Version12101Date20221011153334.php',
'OCA\\Files\\Notification\\Notifier' => __DIR__ . '/..' . '/../lib/Notification/Notifier.php',
'OCA\\Files\\Provider\\FileDownloadProvider' => __DIR__ . '/..' . '/../lib/Provider/FileDownloadProvider.php',
'OCA\\Files\\Search\\FilesSearchProvider' => __DIR__ . '/..' . '/../lib/Search/FilesSearchProvider.php',
'OCA\\Files\\Service\\DirectEditingService' => __DIR__ . '/..' . '/../lib/Service/DirectEditingService.php',
'OCA\\Files\\Service\\OwnershipTransferService' => __DIR__ . '/..' . '/../lib/Service/OwnershipTransferService.php',
Expand Down
3 changes: 3 additions & 0 deletions apps/files/lib/AppInfo/Application.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
use OCA\Files\Listener\LegacyLoadAdditionalScriptsAdapter;
use OCA\Files\Listener\LoadSidebarListener;
use OCA\Files\Notification\Notifier;
use OCA\Files\Provider\FileDownloadProvider;
use OCA\Files\Search\FilesSearchProvider;
use OCA\Files\Service\TagService;
use OCA\Files\Service\UserConfig;
Expand Down Expand Up @@ -122,6 +123,8 @@ public function register(IRegistrationContext $context): void {
$context->registerSearchProvider(FilesSearchProvider::class);

$context->registerNotifierService(Notifier::class);

$context->registerFileDownloadProvider(FileDownloadProvider::class);
Fixed Show fixed Hide fixed
}

public function boot(IBootContext $context): void {
Expand Down
154 changes: 154 additions & 0 deletions apps/files/lib/Controller/DownloadController.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
<?php

declare(strict_types=1);

/**
* @copyright Copyright (c) 2022 Louis Chmn <louis@chmn.me>
*
* @author Louis Chmn <louis@chmn.me>
*
* @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 <http://www.gnu.org/licenses/>.
*
*/

namespace OCA\Files\Controller;

use OC\AppFramework\Bootstrap\Coordinator;
use OCP\AppFramework\Http\ZipResponse;
use OCP\AppFramework\Controller;
use OCP\Files\File;
use OCP\Files\Folder;
use OCP\Files\IFileDownloadProvider;
use OCP\Files\Node;
use OCP\IRequest;
use Psr\Log\LoggerInterface;

class DownloadController extends Controller {
private Coordinator $coordinator;
private LoggerInterface $logger;

public function __construct(
string $appName,
IRequest $request,
Coordinator $coordinator,
LoggerInterface $logger
) {
parent::__construct($appName, $request);

$this->request = $request;
$this->coordinator = $coordinator;
$this->logger = $logger;
}

/**
* @NoCSRFRequired
* @PublicPage
Copy link
Member

Choose a reason for hiding this comment

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

The DownloadController is accessible from a logged-out user. (@publicpage)

Please make use of the rate limit then, so guests can not DDoS the server:

* @UserRateThrottle(limit=5, period=100)
* @AnonRateThrottle(limit=1, period=100)

Should also add brute force protection and log attempts for guessing share tokens

* that are annotated with @BruteForceProtection(action=$action) whereas $action

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't there be a way to indicate failed guess attempts for brute force protection ? Like in:

https://github.com/nextcloud/photos/blob/05c3c2f2271b5ee58c1a0b9a7b3f55573230d20b/lib/Sabre/PublicAlbumAuthBackend.php#L53-L60

Copy link
Member

Choose a reason for hiding this comment

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

Yes for brute force protection is $response->throttle();

For rate limiting this is not needed as rate limiting always kicks in.

* @UserRateThrottle(limit=5, period=100)
* @AnonRateThrottle(limit=1, period=100)
* @BruteForceProtection(action='download_files')
*/
public function index(string $files): ZipResponse {
$response = new ZipResponse($this->request, 'download');

/** @var string[] */
$files = json_decode($files);
artonge marked this conversation as resolved.
Show resolved Hide resolved

if (count($files) === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we have an upper limit as well? Server can easily run into a timeout anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Difficult to predict from the number of files. Also, what to do if we reach a timeout ? Because this prevent users from downloading their files. We can maybe create a background job that will make the file available at a specific URL.

Would checking for the execTime = currentTime - startTime in the loop make sense? If execTime is bigger than timeoutTime, we can return with an message saying, "Download will be available later at url".

Same thing with the size. Not sure how the zip is created, but if this is in memory, then this will lead to OOM. So when the size reaches 2Gb, we can also return the same message.

return $response;
}

[$firstPrefix,] = explode('/', $files[0], 2);
$commonPrefix = $firstPrefix;
foreach ($files as $filePath) {
$commonPrefix = $this->getCommonPrefix($filePath, $commonPrefix);
}

foreach ($files as $filePath) {
$node = null;

foreach ($this->getProviders() as $provider) {
try {
$node = $provider->getNode($filePath);
if ($node !== null) {
break;
}
} catch (\Throwable $ex) {
$providerClass = $provider::class;
Fixed Show fixed Hide fixed
$this->logger->warning("Error while getting file content from $providerClass", ['exception' => $ex]);
Fixed Show fixed Hide fixed
}
}

if ($node === null) {
continue;
}

$this->addNode($response, $node, substr($filePath, strlen($commonPrefix)));
}

return $response;
}

private function getCommonPrefix(string $str1, string $str2): string {
Copy link
Member

Choose a reason for hiding this comment

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

What is the goal of this function? It looks very dangerous and I don't think it does what you think it does?

?files=['photos/sharedalbums/bob/img.png','photos/sharedalbums/bar/img.png'] has commonPrefix of photos/sharedalbums/b and then try to find notes like ob/img.png and ar/img.png?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's to prevent having a zip file with this kind of hierarchy:

files/
├─ userId/
│  ├─ folderName/
│  │  ├─ theFileIWant.txt

But only

theFileIWant.txt

Note that $commonPrefix is not used for searching, but only for building the zip.

However, your concern is still valid. The best way would be to be able to split the path and then compare the parts. But I am not sure that we have a proper way to do that in PHP. Any tips ?

Copy link
Member

Choose a reason for hiding this comment

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

explode('/', $path) and then compare the segments?

$explodedStr1 = explode('/', $str1);
$explodedStr2 = explode('/', $str2);

for ($i = 0; $i < count($explodedStr1); $i++) {
if (!isset($explodedStr2[$i]) || $explodedStr1[$i] !== $explodedStr2[$i]) {
$i--;
break;
}
}

if ($i < 0) {
return '';
} else {
return implode(array_slice($explodedStr1, 0, $i));
}
}

private function addNode(ZipResponse $response, Node $node, string $path): void {
if ($node instanceof File) {
$response->addResource($node->fopen('r'), $path, $node->getSize());
}

if ($node instanceof Folder) {
foreach ($node->getDirectoryListing() as $subnode) {
$this->addNode($response, $subnode, $path.'/'.$subnode->getName());
}
}
}

/**
* @return IFileDownloadProvider[]
*/
private function getProviders() {
/** @var IFileDownloadProvider[] */
$providers = [];

$context = $this->coordinator->getRegistrationContext();
if ($context === null) {
throw new \Exception("Can't get download providers");
}

$providerRegistrations = $context->getFileDownloadProviders();

foreach ($providerRegistrations as $registration) {
$providers[] = \OCP\Server::get($registration->getService());
}

return $providers;
}
}
60 changes: 60 additions & 0 deletions apps/files/lib/Provider/FileDownloadProvider.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
<?php

declare(strict_types=1);

/**
* @copyright Copyright (c) 2022 Louis Chmn <louis@chmn.me>
*
* @author Louis Chmn <louis@chmn.me>
*
* @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 <http://www.gnu.org/licenses/>.
*
*/
namespace OCA\Files\Provider;

use OCP\Files\Folder;
use OCP\Files\Node;
use OCP\Files\IFileDownloadProvider;

class FileDownloadProvider implements IFileDownloadProvider {
private ?Folder $userFolder;

public function __construct(
?Folder $userFolder
) {
$this->userFolder = $userFolder;
}

public function getNode(string $davPath): ?Node {
if (!str_starts_with($davPath, "files/")) {
return null;
}

if ($this->userFolder === null) {
return null;
}

/** @var ?string */
$userId = explode('/', $davPath, 3)[1] ?? null;
if (is_null($userId) || $userId !== $this->userFolder->getOwner()->getUID()) {
Fixed Show fixed Hide fixed
Fixed Show fixed Hide fixed
return null;
}

$filePath = substr($davPath, strlen("files/$userId"));

return $this->userFolder->get($filePath);
}
}
1 change: 1 addition & 0 deletions lib/composer/composer/autoload_classmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,7 @@
'OCP\\Files\\ForbiddenException' => $baseDir . '/lib/public/Files/ForbiddenException.php',
'OCP\\Files\\GenericFileException' => $baseDir . '/lib/public/Files/GenericFileException.php',
'OCP\\Files\\IAppData' => $baseDir . '/lib/public/Files/IAppData.php',
'OCP\\Files\\IFileDownloadProvider' => $baseDir . '/lib/public/Files/IFileDownloadProvider.php',
'OCP\\Files\\IHomeStorage' => $baseDir . '/lib/public/Files/IHomeStorage.php',
'OCP\\Files\\IMimeTypeDetector' => $baseDir . '/lib/public/Files/IMimeTypeDetector.php',
'OCP\\Files\\IMimeTypeLoader' => $baseDir . '/lib/public/Files/IMimeTypeLoader.php',
Expand Down
1 change: 1 addition & 0 deletions lib/composer/composer/autoload_static.php
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
'OCP\\Files\\ForbiddenException' => __DIR__ . '/../../..' . '/lib/public/Files/ForbiddenException.php',
'OCP\\Files\\GenericFileException' => __DIR__ . '/../../..' . '/lib/public/Files/GenericFileException.php',
'OCP\\Files\\IAppData' => __DIR__ . '/../../..' . '/lib/public/Files/IAppData.php',
'OCP\\Files\\IFileDownloadProvider' => __DIR__ . '/../../..' . '/lib/public/Files/IFileDownloadProvider.php',
'OCP\\Files\\IHomeStorage' => __DIR__ . '/../../..' . '/lib/public/Files/IHomeStorage.php',
'OCP\\Files\\IMimeTypeDetector' => __DIR__ . '/../../..' . '/lib/public/Files/IMimeTypeDetector.php',
'OCP\\Files\\IMimeTypeLoader' => __DIR__ . '/../../..' . '/lib/public/Files/IMimeTypeLoader.php',
Expand Down
22 changes: 22 additions & 0 deletions lib/private/AppFramework/Bootstrap/RegistrationContext.php
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
use OCP\Dashboard\IManager;
use OCP\Dashboard\IWidget;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\Files\IFileDownloadProvider;
use OCP\Files\Template\ICustomTemplateProvider;
use OCP\Http\WellKnown\IHandler;
use OCP\Notification\INotifier;
Expand Down Expand Up @@ -127,6 +128,9 @@ class RegistrationContext {
/** @var ParameterRegistration[] */
private $sensitiveMethods = [];

/** @var ServiceRegistration<IFileDownloadProvider>[] */
private array $fileDownloadProviders = [];

/** @var LoggerInterface */
private $logger;

Expand Down Expand Up @@ -326,6 +330,13 @@ public function registerSensitiveMethods(string $class, array $methods): void {
$methods
);
}

public function registerFileDownloadProvider(string $class): void {
$this->context->registerFileDownloadProvider(
$this->appId,
$class
);
}
};
}

Expand Down Expand Up @@ -461,6 +472,10 @@ public function registerSensitiveMethods(string $appId, string $class, array $me
$this->sensitiveMethods[] = new ParameterRegistration($appId, $class, $methods);
}

public function registerFileDownloadProvider(string $appId, string $class): void {
artonge marked this conversation as resolved.
Show resolved Hide resolved
$this->fileDownloadProviders[] = new ServiceRegistration($appId, $class);
}

/**
* @param App[] $apps
*/
Expand Down Expand Up @@ -738,4 +753,11 @@ public function getUserMigrators(): array {
public function getSensitiveMethods(): array {
return $this->sensitiveMethods;
}

/**
* @return ServiceRegistration<IFileDownloadProvider>[]
*/
public function getFileDownloadProviders(): array {
return $this->fileDownloadProviders;
}
}
10 changes: 10 additions & 0 deletions lib/public/AppFramework/Bootstrap/IRegistrationContext.php
Original file line number Diff line number Diff line change
Expand Up @@ -328,4 +328,14 @@ public function registerUserMigrator(string $migratorClass): void;
* @since 25.0.0
*/
public function registerSensitiveMethods(string $class, array $methods): void;

/**
* Register a backend to provide file based on a dav path.
*
* @param string $class
* @param string[] $methods
* @return void
* @since 26.0.0
*/
public function registerFileDownloadProvider(string $class): void;
}
Loading