Align href url encoding with oc10 #1425

butonic · 2021-01-28T20:52:40Z

We now use the same percent encoding for URLs in WebDAV href properties as ownCloud 10.

fixes owncloud/ocis#1120
fixes owncloud/ocis#1296
fixes owncloud/ocis#1307

ownCloud 10 relies on sabre/dav to encode the href in xml like this:

https://github.com/sabre-io/http/blob/bb27d1a8c92217b34e778ee09dcf79d9a2936e84/lib/functions.php#L369-L379

I ported the function to go. If anyone wants to go crazy about optimizing it feel free after this PR was merged. I want to make the tests green first.

update-docs · 2021-01-28T20:52:43Z

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.

butonic · 2021-01-29T15:44:44Z

@phil-davis how can this Scenario: Get the size of a file shared by public link work:
https://drone.cernbox.cern.ch/cs3org/reva/315/16/7

@skipOnOcV10.3 | 580s
6126 -- | --
6127 | Scenario: Get the size of a file shared by public link                                     # /drone/src/tmp/testrunner/tests/acceptance/features/apiSharePublicLink1/createPublicLinkShare.feature:755 | 580s
6128 | Given the administrator has enabled DAV tech_preview                                     # OccContext::theAdministratorHasEnabledDAVTechPreview() | 580s
6129 | And user "Alice" has uploaded file with content "This is a test file" to "test-file.txt" # FeatureContext::userHasUploadedAFileWithContentTo() | 580s
6130 | And user "Alice" has created a public link share with settings                           # FeatureContext::userHasCreatedAPublicLinkShareWithSettings() | 580s
6131 | \| path        \| test-file.txt \| | 580s
6132 | \| permissions \| read          \| | 580s
6133 | When the public gets the size of the last shared public link using the WebDAV API        # FeatureContext::publicGetsSizeOfLastSharedPublicLinkUsingTheWebdavApi() | 580s
6134 | Then the HTTP status code should be "207"                                                # FeatureContext::thenTheHTTPStatusCodeShouldBe() | 580s
6135 | And the size of the file should be "19"                                                  # FeatureContext::theSizeOfTheFileShouldBe() | 580s
6136 | WebDav::theSizeOfTheFileShouldBe Expected size of the file was '19', but got '' instead. | 580s
6137 | Failed asserting that two strings are equal. | 580s
6138 | --- Expected | 580s
6139 | +++ Actual | 580s
6140 | @@ @@ | 580s
6141 | -'19' | 580s
6142 | +''

On oc10 the xml of a publicly shared file looks like this:

<?xml version="1.0"?>
<d:multistatus
	xmlns:d="DAV:"
	xmlns:s="http://sabredav.org/ns"
	xmlns:oc="http://owncloud.org/ns">
	<d:response>
		<d:href>/remote.php/dav/public-files/AM3BHmiAWvnxBCX/</d:href>
		<d:propstat>
			<d:prop>
				<oc:permissions></oc:permissions>
				<d:resourcetype>
					<d:collection/>
				</d:resourcetype>
			</d:prop>
			<d:status>HTTP/1.1 200 OK</d:status>
		</d:propstat>
		<d:propstat>
			<d:prop>
				<oc:favorite/>
				<oc:fileid/>
				<oc:owner-id/>
				<oc:owner-display-name/>
				<oc:share-types/>
				<oc:privatelink/>
				<d:getcontentlength/>
				<oc:size/>
				<d:getlastmodified/>
				<d:getetag/>
			</d:prop>
			<d:status>HTTP/1.1 404 Not Found</d:status>
		</d:propstat>
	</d:response>
	<d:response>
		<d:href>/remote.php/dav/public-files/AM3BHmiAWvnxBCX/ownCloud%20Manual.pdf</d:href>
		<d:propstat>
			<d:prop>
				<oc:permissions></oc:permissions>
				<oc:fileid>19</oc:fileid>
				<oc:owner-id>admin</oc:owner-id>
				<oc:owner-display-name>admin</oc:owner-display-name>
				<d:getcontentlength>6662575</d:getcontentlength>
				<oc:size>6662575</oc:size>
				<d:getlastmodified>Fri, 29 Jan 2021 15:18:49 GMT</d:getlastmodified>
				<d:getetag>&quot;e4985b9306b021210b456b88b1a21ad1&quot;</d:getetag>
				<d:resourcetype/>
			</d:prop>
			<d:status>HTTP/1.1 200 OK</d:status>
		</d:propstat>
		<d:propstat>
			<d:prop>
				<oc:favorite/>
				<oc:share-types/>
				<oc:privatelink/>
			</d:prop>
			<d:status>HTTP/1.1 404 Not Found</d:status>
		</d:propstat>
	</d:response>
</d:multistatus>

The path expression //d:prop/d:getcontentlength used in the test evaluates to

Element='<d:getcontentlength xmlns:d="DAV:"/>'
Element='<d:getcontentlength xmlns:d="DAV:">6662575</d:getcontentlength>'

on ocis it looks like this:

<?xml version="1.0" encoding="utf-8"?>
<d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns">
  <d:response>
    <d:href>/remote.php/dav/public-files/wCQgXMEmktssxAH/</d:href>
    <d:propstat>
      <d:prop>
        <oc:permissions>S</oc:permissions>
        <oc:favorite>0</oc:favorite>
        <oc:fileid>MTI4NGQyMzgtYWE5Mi00MmNlLWJkYzQtMGIwMDAwMDA5MTU3OmU1NzYxZmFiLTYxZjEtNGYyZC04YTI5LWU1NDQ5NTlkZmI5NA==</oc:fileid>
        <oc:size>737</oc:size>
        <d:getlastmodified>Fri, 29 Jan 2021 15:24:42 +0000</d:getlastmodified>
        <d:getetag>"1ba16dcb2464582770a72e52a7297ab7"</d:getetag>
        <d:resourcetype>
          <d:collection/>
        </d:resourcetype>
      </d:prop>
      <d:status>HTTP/1.1 200 OK</d:status>
    </d:propstat>
    <d:propstat>
      <d:prop>
        <oc:owner-id/>
        <oc:owner-display-name/>
        <oc:share-types/>
        <oc:privatelink/>
        <d:getcontentlength/>
      </d:prop>
      <d:status>HTTP/1.1 404 Not Found</d:status>
    </d:propstat>
  </d:response>
  <d:response>
    <d:href>/remote.php/dav/public-files/wCQgXMEmktssxAH/ChatLog%20All%20hands%20call%202021_01_27%2010_03.rtf</d:href>
    <d:propstat>
      <d:prop>
        <oc:permissions>S</oc:permissions>
        <oc:favorite>0</oc:favorite>
        <oc:fileid>MTI4NGQyMzgtYWE5Mi00MmNlLWJkYzQtMGIwMDAwMDA5MTU3OmU1NzYxZmFiLTYxZjEtNGYyZC04YTI5LWU1NDQ5NTlkZmI5NA==</oc:fileid>
        <d:getcontentlength>737</d:getcontentlength>
        <oc:size>737</oc:size>
        <d:getlastmodified>Fri, 29 Jan 2021 15:24:42 +0000</d:getlastmodified>
        <d:getetag>"1ba16dcb2464582770a72e52a7297ab7"</d:getetag>
        <d:resourcetype/>
      </d:prop>
      <d:status>HTTP/1.1 200 OK</d:status>
    </d:propstat>
    <d:propstat>
      <d:prop>
        <oc:owner-id/>
        <oc:owner-display-name/>
        <oc:share-types/>
        <oc:privatelink/>
      </d:prop>
      <d:status>HTTP/1.1 404 Not Found</d:status>
    </d:propstat>
  </d:response>
</d:multistatus>

which evaluates to

Element='<d:getcontentlength xmlns:d="DAV:"/>'
Element='<d:getcontentlength xmlns:d="DAV:">737</d:getcontentlength>'

So both return two entries, which makes sense, because webdave requires the parent to be part of the collection of a propfind.

Both requests were made against the public-url endpoint:

https://demo.owncloud.com/remote.php/dav/public-files/AM3BHmiAWvnxBCX
https://cloud.ocis.test/remote.php/dav/public-files/wCQgXMEmktssxAH

I did not find any magic that would descend int the dir and do a PROPFIND directly on the file... or that would append the name of the file to the propfind.

🤔

doesnt the test test the wrong entry when used for a file (vs a folder) shared via public link?

	public function theSizeOfTheFileShouldBe($size) {
		$responseXml = HttpRequestHelper::getResponseXml(
			$this->response,
			__METHOD__
		);
		$xmlPart = $responseXml->xpath("//d:prop/d:getcontentlength");
		$actualSize = (string) $xmlPart[0];
		Assert::assertEquals(
			$size,
			$actualSize,
			__METHOD__
			. " Expected size of the file was '$size', but got '$actualSize' instead."
		);
	}

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

butonic · 2021-01-29T20:27:35Z

might be a Depth: 0 header ...

butonic · 2021-01-29T21:45:11Z

hm the testsuite does not send a depth 0 ... does oc10 default to depth 0?

butonic · 2021-01-29T21:47:59Z

no ... bit it does not return any properties as 404 on the root collection of a shared public file:

<?xml version="1.0"?>
<d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns">
  <d:response>
    <d:href>/remote.php/dav/public-files/KNghJEjp2XqZI3n/</d:href>
    <d:propstat>
      <d:prop>
        <d:resourcetype>
          <d:collection/>
        </d:resourcetype>
      </d:prop>
      <d:status>HTTP/1.1 200 OK</d:status>
    </d:propstat>
  </d:response>
  <d:response>
    <d:href>/remote.php/dav/public-files/KNghJEjp2XqZI3n/ownCloud%20Manual.pdf</d:href>
    <d:propstat>
      <d:prop>
        <d:getlastmodified>Fri, 29 Jan 2021 21:27:32 GMT</d:getlastmodified>
        <d:getcontentlength>6662575</d:getcontentlength>
        <d:resourcetype/>
        <d:getetag>"abc05683012996d0c96249586539a740"</d:getetag>
        <d:getcontenttype>application/pdf</d:getcontenttype>
      </d:prop>
      <d:status>HTTP/1.1 200 OK</d:status>
    </d:propstat>
    <d:propstat>
      <d:prop>
        <d:quota-used-bytes/>
        <d:quota-available-bytes/>
      </d:prop>
      <d:status>HTTP/1.1 404 Not Found</d:status>
    </d:propstat>
  </d:response>
</d:multistatus>

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

butonic · 2021-01-30T18:21:02Z

@phil-davis I don't know why oc 10 renders the href with different casing. Percent encoding is specified as treating the different caseing of a-f as equivalent: https://tools.ietf.org/html/rfc3986#section-2.1

General URL Handling https://tools.ietf.org/html/rfc4918#section-8.3

Path production rules https://tools.ietf.org/html/rfc3986#section-3.3

Can we make the tests ignore case? I think we should.

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

butonic · 2021-01-30T20:58:41Z

Ok so ... Rendering the percent encoded bytes as lowercase seems to work now. I guess I confused the different results at some point... @phil-davis nevermind... I hope ...

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

phil-davis · 2021-02-01T07:44:37Z

Can we make the tests ignore case? I think we should.

I agree. The core API tests are too strict, because when they were written they were just regression tests for oC10. Now they are being used as "gold" tests against other implementations (OCIS and reva), so they should be given a "correct" amount of flexibility. I raised issue owncloud/core#38367

phil-davis

Changes to expected-failures and removal of out-of-date test scenarios LGTM. Someone else can review the actual Golang code here!

phil-davis · 2021-02-01T09:15:37Z

tests/acceptance/features/apiOcisSpecific/apiWebdavProperties2-getFileProperties.feature

-    Examples:
-      | dav_version | folder_name     | expected_href                                                             |
-      | old         | /folder ?2.txt  | webdav\/folder%20%3F2\.txt                                                |
-      | new         | /folder ?2.txt  | dav\/files\/%username%\/folder%20%3F2\.txt                                     |


Looks good to delete these "local" test scenarios. Various core scenarios are now passing, which is great.
Requiring exact values like %3F is problematic in tests like this - both %3F and %3f are valid. I raised issue owncloud/core#38367 to get the core API tests sorted out so that they have the needed flexibility. That way, in future, we can have implementations that return either case and the test suite will allow it.

ishank011 · 2021-02-01T11:01:13Z

internal/http/services/owncloud/ocdav/ocdav.go

+
+// replaceAllStringSubmatchFunc is taken from 'Go: Replace String with Regular Expression Callback'
+// see: https://elliotchance.medium.com/go-replace-string-with-regular-expression-callback-f89948bad0bb
+func replaceAllStringSubmatchFunc(re *regexp.Regexp, str string, repl func([]string) string) string {


I'd like to look into this. Will create an issue

butonic requested a review from labkode as a code owner January 28, 2021 20:52

butonic force-pushed the align-href-url-encoding-with-oc10 branch 4 times, most recently from 1bed621 to fa19fa6 Compare January 29, 2021 10:33

butonic force-pushed the align-href-url-encoding-with-oc10 branch from fa19fa6 to 8846ffe Compare January 29, 2021 16:03

Align href url encoding with oc10

9fd63cd

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

butonic force-pushed the align-href-url-encoding-with-oc10 branch from 8846ffe to bf6fbc6 Compare January 29, 2021 19:15

fix trash href, location and filename, correctly escape property values

6554e2c

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

butonic force-pushed the align-href-url-encoding-with-oc10 branch from bf6fbc6 to 6554e2c Compare January 29, 2021 19:27

butonic force-pushed the align-href-url-encoding-with-oc10 branch from a5bfc0e to 08d5c13 Compare January 29, 2021 21:54

align public file link root collection rendering

1acb45e

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

butonic force-pushed the align-href-url-encoding-with-oc10 branch from 08d5c13 to 1acb45e Compare January 30, 2021 11:58

render percent encoded bytes lowercased

a6a3c27

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

finish pr

6a346fa

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

butonic force-pushed the align-href-url-encoding-with-oc10 branch from 22d4bba to 6a346fa Compare January 30, 2021 23:02

phil-davis mentioned this pull request Feb 1, 2021

Make tests of escaped file and folder names more flexible. owncloud/core#38367

Closed

butonic requested a review from phil-davis February 1, 2021 09:00

phil-davis approved these changes Feb 1, 2021

View reviewed changes

phil-davis requested a review from ishank011 February 1, 2021 09:37

ishank011 approved these changes Feb 1, 2021

View reviewed changes

ishank011 merged commit e3510ca into cs3org:master Feb 1, 2021

butonic deleted the align-href-url-encoding-with-oc10 branch February 1, 2021 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Align href url encoding with oc10 #1425

Align href url encoding with oc10 #1425

butonic commented Jan 28, 2021 •

edited

Loading

update-docs bot commented Jan 28, 2021

butonic commented Jan 29, 2021 •

edited

Loading

butonic commented Jan 29, 2021

butonic commented Jan 29, 2021

butonic commented Jan 29, 2021

butonic commented Jan 30, 2021

butonic commented Jan 30, 2021

phil-davis commented Feb 1, 2021

phil-davis left a comment

phil-davis Feb 1, 2021

ishank011 Feb 1, 2021

Align href url encoding with oc10 #1425

Align href url encoding with oc10 #1425

Conversation

butonic commented Jan 28, 2021 • edited Loading

update-docs bot commented Jan 28, 2021

butonic commented Jan 29, 2021 • edited Loading

butonic commented Jan 29, 2021

butonic commented Jan 29, 2021

butonic commented Jan 29, 2021

butonic commented Jan 30, 2021

butonic commented Jan 30, 2021

phil-davis commented Feb 1, 2021

phil-davis left a comment

Choose a reason for hiding this comment

phil-davis Feb 1, 2021

Choose a reason for hiding this comment

ishank011 Feb 1, 2021

Choose a reason for hiding this comment

butonic commented Jan 28, 2021 •

edited

Loading

butonic commented Jan 29, 2021 •

edited

Loading