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

pre-signed download urls for password protected public links #38376

Merged
merged 1 commit into from Mar 5, 2021

Conversation

C0rby
Copy link
Contributor

@C0rby C0rby commented Feb 3, 2021

Description

When a propfind request is done for a password protected public link share and the property '{http://owncloud.org/ns}downloadURL' is requested, then a signed url is created and sent to the client as the value of that property.
The downloadURL property was added some time ago but haven't been used since because the demand disappeared. So I think using this property should be fine.

Related Issue

Motivation and Context

To support clients which don't use cookies I implemented pre-signed urls
for password protected public links. The share password is used as the
signing key and the signed url is then added to the propfind response in
the field downloadURL which was added before but never used. This
change allows owncloud Web to implement a more efficient download
mechanism for password protected link shares.

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@C0rby C0rby self-assigned this Feb 3, 2021
@C0rby C0rby force-pushed the owncloud-web-password-public-share branch 3 times, most recently from 27c4700 to 9fc9ea8 Compare February 3, 2021 15:23
apps/dav/lib/Files/PublicFiles/PublicFilesPlugin.php Outdated Show resolved Hide resolved
apps/dav/lib/Files/PublicFiles/PublicFilesPlugin.php Outdated Show resolved Hide resolved
return \hash_hmac('sha512/256', \implode('|', [$this->token, $this->fileName, $this->validUntil]), $this->signingKey, false);
}

public function verify($signature) {
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't seem to add much value... I mean, you could just call the "get" method and compare it outside. Any future plan to make this verification more complex? That could justify this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you are right. The idea behind it was just to have a nice API.

$signature = new PublicShareSignature($share->getToken(), $node->getName(), $validUntil, $key);
if ($signature->verify($urlSignature)) {
...

vs.

$signature = new PublicShareSignature($share->getToken(), $node->getName(), $validUntil, $key);
if ($signature->get() == $urlSignature) {
...

It doesn't add much value and won't get any more complex in the future so I'm good with deleting the method and comparing the hash outside.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, just remember that === is always preferred. If you use ==, include a comment explaining the reason in order to prevent changing it later.

$this->signingKey = $signingKey;
}

public function get() {
Copy link
Member

Choose a reason for hiding this comment

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

In general, I prefer to avoid too generic method names. It will be more difficult to find where this method is being used. getSignature or getShareSignature will be easier to find

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with it potentially being hard to find.
Though if we change the name from get to getSignature then I would change the class as well.
Currently the usage looks like:

$signature = new PublicShareSignature($share->getToken(), $node->getName(), $validUntil, $key);
$signature->get();

By changing the method to getSignature it would look wrong.

$signature = new PublicShareSignature($share->getToken(), $node->getName(), $validUntil, $key);
$signature->getSignature();

So how do you find this:

$signer = new PublicShareSigner($share->getToken(), $node->getName(), $validUntil, $key);
$signer->createSignature();

Copy link
Member

Choose a reason for hiding this comment

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

"create" doesn't necessarily imply returning a value. You could create the signature, keep it inside the class, and return "true" or "false" to indicate if the signature was created successfully.
Anyway, I don't want to overcomplicate this, so I'm fine if you want to keep the "createSignature" name as long as it's properly documented and explained in the PHPDoc.

By changing the method to getSignature it would look wrong.

I partially agree. It looks wrong because that's the only method the object has. However, it doesn't look bad if you have a signature object with multiple methods, being one of them a method to return the raw or stringified signature.

@C0rby C0rby force-pushed the owncloud-web-password-public-share branch from 9fc9ea8 to 901d87e Compare February 11, 2021 10:30
@C0rby C0rby marked this pull request as draft February 11, 2021 11:25
@C0rby
Copy link
Contributor Author

C0rby commented Feb 11, 2021

I converted the PR to a draft because I noticed a bug when sharing a folder.
The signature check is not working correctly because when checking the signature we use the shared resource name (In case of a folder, the folder name) not the filename.

@C0rby C0rby force-pushed the owncloud-web-password-public-share branch 3 times, most recently from b9fa135 to 759b637 Compare February 17, 2021 17:36
@C0rby C0rby marked this pull request as ready for review February 18, 2021 08:33
@C0rby C0rby force-pushed the owncloud-web-password-public-share branch from 759b637 to 3c29599 Compare February 18, 2021 09:29
@C0rby
Copy link
Contributor Author

C0rby commented Feb 18, 2021

So folder shares work now.

apps/dav/lib/Files/PublicFiles/PublicFilesPlugin.php Outdated Show resolved Hide resolved
apps/dav/lib/Files/PublicFiles/PublicFilesPlugin.php Outdated Show resolved Hide resolved
apps/dav/lib/Files/PublicFiles/PublicFilesPlugin.php Outdated Show resolved Hide resolved
apps/dav/lib/Files/PublicFiles/PublicFilesPlugin.php Outdated Show resolved Hide resolved
$shared_resource_path = '/' . $share->getNode()->getName();
}
$s = new PublicShareSigner($share->getToken(), $shared_resource_path, $validUntil, $key);
return $path . '?signature=' . $s->getSignature() . '&expires=' . \urlencode($validUntil->format(\DateTime::ATOM));
Copy link
Member

Choose a reason for hiding this comment

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

I assume the date format is consistent with the rest of the code. If not, there are 2 options: either change it for consistency or include a comment explaining.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, there is no 'one' date format. In the code I found usages of ATOM, ISO8601, RFC2822.
I chose ATOM because of this: https://www.php.net/manual/en/class.datetimeinterface.php#datetime.constants.iso8601

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we should have only one, but it seems we don't... at least we can change this one later without problems.

@C0rby C0rby force-pushed the owncloud-web-password-public-share branch from 3c29599 to 30802b0 Compare March 4, 2021 16:27
To support clients which don't use cookies I implemented pre-signed urls
for password protected public links. The share password is used as the
signing key and the signed url is then added to the propfind response in
the field `downloadURL` which was added before but never used. This
change allows owncloud Web to implement a more efficient download
mechanism for password protected link shares.
@C0rby C0rby force-pushed the owncloud-web-password-public-share branch from 30802b0 to 6ea3fb0 Compare March 4, 2021 17:06
@sonarcloud
Copy link

sonarcloud bot commented Mar 4, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@micbar micbar merged commit 33ebff6 into master Mar 5, 2021
@delete-merged-branch delete-merged-branch bot deleted the owncloud-web-password-public-share branch March 5, 2021 20:07
@C0rby C0rby mentioned this pull request Mar 17, 2021
11 tasks
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.

None yet

3 participants