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

Conversation

artonge
Copy link
Contributor

@artonge artonge commented Nov 3, 2022

This PR allows applications to respond to file download requests by registering providers.

Another way of doing it would be to get the content from our DAV API, but I am not sure that I can get the content of a file without doing an HTTP request.

  • Apps can register download providers like so:
// AppInfo/Application.php
class Application extends App implements IBootstrap {
	// ...
	public function register(IRegistrationContext $context): void {
		$context->registerFileDownloadProvider(MyFileDownloadProvider::class);
	}
	// ...
}
// Provider/MyFileDownloadProvider
use OCP\Files\IFileDownloadProvider;

class MyFileDownloadProvider implements IFileDownloadProvider {
	public function getNode(string $davPath): ?Node {
		return $theFileContent;
	}
}
  • The new DownloadController will then query all the providers looking for a content to put in a zip file.
  • As an example, I add a download provider for /files/$userId/... URLs in FileDownloadProvider.php
  • Other implementation example for photos: Register a download provider photos#1450
  • The frontend can the request for an array of files by running a GET on https://server.example/apps/files/api/v1/download?files=${stringified_json_array}

Notes:

  • The DownloadController is accessible from a logged-out user. (@PublicPage)
  • The endpoint support heterogeneous DAV paths. Meaning, I can do ?files=['files/alice/hello.txt','photos/sharedalbums/bob/img.png']

Todo:

@artonge artonge force-pushed the artonge/feat/download_providers branch 3 times, most recently from 6843627 to 6cd7733 Compare November 7, 2022 15:32
@artonge artonge force-pushed the artonge/feat/download_providers branch from 6cd7733 to edd4b86 Compare November 7, 2022 15:49
@artonge artonge force-pushed the artonge/feat/download_providers branch from edd4b86 to 8448fcb Compare November 7, 2022 17:17
@artonge artonge marked this pull request as ready for review November 7, 2022 17:20
@artonge artonge requested review from a team, icewind1991, blizzz and CarlSchwan and removed request for a team November 7, 2022 17:20
apps/files/lib/Controller/DownloadController.php Outdated Show resolved Hide resolved
/**
* @NoAdminRequired
* @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:/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.

apps/files/lib/Controller/DownloadController.php Outdated Show resolved Hide resolved
/** @var string[] */
$files = json_decode($files);

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;
}

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?

apps/files/lib/Provider/FileDownloadProvider.php Outdated Show resolved Hide resolved
apps/files/lib/Provider/FileDownloadProvider.php Outdated Show resolved Hide resolved
lib/public/AppFramework/Bootstrap/IRegistrationContext.php Outdated Show resolved Hide resolved
lib/public/Files/IFileDownloadProvider.php Show resolved Hide resolved
@artonge artonge force-pushed the artonge/feat/download_providers branch 4 times, most recently from 8d5c2b2 to fdfd552 Compare November 8, 2022 17:27
return $response;
}

[$firstPrefix,] = \Sabre\Uri\split($files[0]);

Check failure

Code scanning / Psalm

UndefinedFunction

Function Sabre\Uri\split does not exist
@artonge artonge force-pushed the artonge/feat/download_providers branch 2 times, most recently from 3dbdf6d to a9d9d50 Compare November 8, 2022 17:50
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
@blizzz blizzz mentioned this pull request Mar 7, 2023
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
@blizzz blizzz mentioned this pull request May 17, 2023
@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
@artonge artonge marked this pull request as draft November 2, 2023 10:35
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
This was referenced Mar 12, 2024
@Altahrim Altahrim mentioned this pull request Mar 20, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 6, 2024
@skjnldsv skjnldsv closed this Aug 14, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 31 milestone Aug 14, 2024
@skjnldsv skjnldsv deleted the artonge/feat/download_providers branch August 30, 2024 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants