Support signed urls by DeepDiver1975 · Pull Request #37634 · owncloud/core

DeepDiver1975 · 2020-07-02T18:37:13Z

Description

OCS API /cloud/user/signing-key to get the user's key for url signing
capability to state that signed urls are supported
allow signed url access for webdav endpoints

Related Issue

Refs https://github.com/owncloud/enterprise/issues/3912

How Has This Been Tested?

Integrate owncloud-sign-url owncloud-sdk#504

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:

update-docs · 2020-07-02T18:37:16Z

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

codecov · 2020-07-03T11:48:07Z

Codecov Report

Merging #37634 into master will decrease coverage by 0.03%.
The diff coverage is 42.99%.

@@             Coverage Diff              @@
##             master   #37634      +/-   ##
============================================
- Coverage     64.74%   64.71%   -0.04%     
- Complexity    19360    19383      +23     
============================================
  Files          1281     1283       +2     
  Lines         75622    75724     +102     
  Branches       1333     1333              
============================================
+ Hits          48959    49001      +42     
- Misses        26271    26331      +60     
  Partials        392      392

Flag	Coverage Δ	Complexity Δ
#javascript	`54.03% <ø> (ø)`	`0.00 <ø> (ø)`
#phpunit	`65.88% <42.99%> (-0.04%)`	`19383.00 <24.00> (+23.00)`	⬇️

Impacted Files	Coverage Δ	Complexity Δ
apps/dav/appinfo/v1/caldav.php	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
apps/dav/appinfo/v1/carddav.php	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
apps/dav/appinfo/v1/webdav.php	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
core/Command/Security/CreateSignKey.php	`0.00% <0.00%> (ø)`	`6.00 <6.00> (?)`
core/Controller/CloudController.php	`0.00% <0.00%> (ø)`	`5.00 <2.00> (+2.00)`
core/register_command.php	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
core/routes.php	`50.00% <ø> (ø)`	`0.00 <0.00> (ø)`
lib/private/OCS/CoreCapabilities.php	`0.00% <0.00%> (ø)`	`2.00 <0.00> (ø)`
apps/dav/lib/Connector/Sabre/Auth.php	`78.18% <25.00%> (-11.18%)`	`43.00 <5.00> (+4.00)`	⬇️
lib/private/Security/SignedUrl/Verifier.php	`95.12% <95.12%> (ø)`	`11.00 <11.00> (?)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9687d77...e32cbef. Read the comment docs.

butonic · 2020-07-03T12:48:23Z

apps/dav/lib/Connector/Sabre/Auth.php

 				$uid = $user->getUID();
 				\OC_Util::setupFS($uid);
-				$this->currentUser = $uid;
+				$currentUser = $uid;


I don't see $currentUser being used anywhere (neither was there a $this->currentUser to begin with). Remove completely?

core/Controller/CloudController.php

PVince81

Some minor picky stuff.

Otherwise this looks good from the code perspective, I haven't tested 👍

apps/dav/lib/Connector/Sabre/Auth.php

PVince81 · 2020-07-03T15:00:34Z

apps/dav/lib/Files/FileLocksBackend.php


 		// in case the timeout has not been accepted, adjust in lock info
 		$lockInfo->timeout = $lock->getTimeout();
+		$lockInfo->owner = $lock->getOwner();


PR mix up ?

core/Controller/CloudController.php

lib/private/Security/SignedUrl/Verifier.php

PVince81 · 2020-07-03T15:07:03Z

tests/lib/Security/SignedUrl/VerifierTest.php

+		}
+	}
+
+	public function providesUrls() {


add cases where some of the required attrs are missing

lib/private/Security/SignedUrl/Verifier.php

PVince81 · 2020-07-03T15:10:30Z

lib/private/Security/SignedUrl/Verifier.php

+		return $this->request->getQueryParameters();
+	}
+
+	public function getUserId(): string {


rename to getUrlCredential for consistency ? everywhere you used it you always stored it in a var getUrlCredential. or alternatively rename those vars to $userId ?

DeepDiver1975 · 2020-07-06T08:11:05Z

codecov is nuts .... most likly because of the rebase .....

C0rby

👍 Looks good to me.

PVince81

All fine, just some minor text change 👍

PVince81 · 2020-07-08T12:51:08Z

core/Command/Security/CreateSignKey.php

+	protected function configure() {
+		$this
+			->setName('security:sign-key:create')
+			->setDescription('Create and recreate a users sign key for signed urls')


"Create or recreate a users signing key"

"sign key" doesn't sound good, I believe it should be "signing key" as it's used for signing

PVince81 · 2020-07-08T12:51:40Z

core/Command/Security/CreateSignKey.php

+		}
+		$signingKey = $this->config->getUserValue($uid, 'core', 'signing-key', null);
+		if ($signingKey !== null) {
+			$output->writeln('This user already has a sign key. Recreating the key will invalidate all existing signed urls.');


"signing key"

lib/private/Security/SignedUrl/Verifier.php

micbar · 2020-08-05T10:11:13Z

@DeepDiver1975 Why was this merged without a changelog?

DeepDiver1975 force-pushed the feature/pre-signed-url branch 2 times, most recently from 4c6c090 to e04d2b8 Compare July 3, 2020 10:21

DeepDiver1975 self-assigned this Jul 3, 2020

DeepDiver1975 marked this pull request as ready for review July 3, 2020 12:42

DeepDiver1975 requested review from C0rby, PVince81 and butonic July 3, 2020 12:43

butonic reviewed Jul 3, 2020

View reviewed changes

DeepDiver1975 force-pushed the feature/pre-signed-url branch from e04d2b8 to 6c2c952 Compare July 3, 2020 13:51

PVince81 approved these changes Jul 3, 2020

View reviewed changes

DeepDiver1975 force-pushed the feature/pre-signed-url branch from 6c2c952 to db2cad6 Compare July 6, 2020 06:37

C0rby approved these changes Jul 6, 2020

View reviewed changes

DeepDiver1975 force-pushed the feature/pre-signed-url branch from db2cad6 to 9377864 Compare July 8, 2020 08:47

PVince81 approved these changes Jul 8, 2020

View reviewed changes

Support signed urls

e32cbef

DeepDiver1975 force-pushed the feature/pre-signed-url branch from 9377864 to e32cbef Compare July 8, 2020 13:05

DeepDiver1975 merged commit 4a68319 into master Jul 9, 2020

delete-merged-branch bot deleted the feature/pre-signed-url branch July 9, 2020 15:09

DeepDiver1975 added a commit to owncloud/owncloud-sdk that referenced this pull request Jul 9, 2020

Test with core master after merging PR: owncloud/core#37634

d2bf099

butonic mentioned this pull request Jul 16, 2020

implement middleware to impersonate a user for presigned urls owncloud/ocis-proxy#73

Closed

dpakach mentioned this pull request Jul 24, 2020

Use pre-signed URLs to download files owncloud/web#3797

Merged

5 tasks

Conversation

DeepDiver1975 commented Jul 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

update-docs bot commented Jul 2, 2020

Uh oh!

codecov bot commented Jul 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

butonic Jul 3, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PVince81 Jul 3, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PVince81 Jul 3, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PVince81 Jul 3, 2020

Choose a reason for hiding this comment

Uh oh!

DeepDiver1975 commented Jul 6, 2020

Uh oh!

C0rby left a comment

Choose a reason for hiding this comment

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

PVince81 Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

PVince81 Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

micbar commented Aug 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DeepDiver1975 commented Jul 2, 2020 •

edited

Loading

codecov bot commented Jul 3, 2020 •

edited

Loading