fix downloadURL of public links by C0rby · Pull Request #38532 · owncloud/core

C0rby · 2021-03-16T15:44:32Z

Description

Make the downloadURL always relative to the owncloud base path.

Related Issue

download password protected public shared files using pre-signed url web#4689 (review)

Motivation and Context

If ownCloud is setup in a subdirectory like https://example.com/owncloud/ then we don't wan't to include the subdirectory in the relative downloadURL.

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:

kulmann

👍

jnweiger · 2021-03-17T13:42:44Z

@C0rby when and how can the double prefix occur? I don't understand the context of owncloud/web#4689 (review)

jnweiger · 2021-03-17T13:48:22Z

apps/dav/lib/Files/PublicFiles/PublicFilesPlugin.php

 					}

-					$path = $server->getBaseUri() . $server->getRequestUri();
+					$path = '/remote.php/dav/' . $server->getRequestUri();


This results in a partial URL starting with a /.
So it is not relative, but rooted.
I'd normally object that, as it looks like it should break, when installing owncloud in a subdirectory.

But as you introduce that exactly to fix something with subdirectories, I assume you know what you are doing -- there are other places in the propFind() code that also returns paths starting with a '/'. That seems to be consistent.

Good spot. In fact the leading slash gets removed again in ownCloud Web. So it'd be fine to have a relative URL here.

Also note that legacy "/remote.php/webdav/" endpoint is still supported I think. It doesn't seem a good idea to mix both endpoints. I don't know if some webdav clients could throw errors if you change the endpoint; at least I would be suspicious

This results in a partial URL starting with a /.
So it is not relative, but rooted.

Yes, you're right.

I'd normally object that, as it looks like it should break, when installing owncloud in a subdirectory.

It's the client responsibility to prepend the base path to this. Including the subdirectory if owncloud was intalled in one.
But as you commented here: owncloud/web#4689 (comment)

Why do we go for flexibility? I'd argue that a full URL is more robust, as no errors can be made during URL-assembly.

I was thinking that maybe in a loadbalanced setup having the full URL could break things. After thinking about it again I don't think so anymore.
The instances would have to configure the overwrite.cli.url anyways right? So I could just use that as the base url and it should point to the load balancer/public address.

Also note that legacy "/remote.php/webdav/" endpoint is still supported I think. It doesn't seem a good idea to mix both endpoints. I don't know if some webdav clients could throw errors if you change the endpoint; at least I would be suspicious

I see your point but will a generic webdav client even use this owncloud proprietary attribute?
Or am I wrong and this isn't a proprietary attribute?

Anyways this feature is especially implemented to solve a problem of webclients or more specifically of ownCloud web.
Our other clients don't even need this.

jnweiger

I lack enough insights to review the fix itself.
It affects the behavior of the propFind() method. Not sure how widely used that method is.

Please provide a clear example how to make a PROPFIND call that would benefit from that.
Is this a problem with new code, or is the same problem also present in released versions?

C0rby · 2021-03-17T15:25:45Z

I lack enough insights to review the fix itself.
It affects the behavior of the propFind() method. Not sure how widely used that method is.

Please provide a clear example how to make a PROPFIND call that would benefit from that.
Is this a problem with new code, or is the same problem also present in released versions?

It's not a problem in released versions. This PR fixes a problem which was introduced with a new feature for 10.7 #38376

To test this you can send this PROPFIND request:

curl -s -u public:<share_password>  -k -X PROPFIND -d '<?xml version="1.0"?>
  <d:propfind  xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns">
    <d:prop>
      <oc:permissions />
      <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:resourcetype />
      <oc:downloadURL />
    </d:prop>
  </d:propfind>' https://localhost:9200/remote.php/dav/public-files/rUenhLAqWpfHPXJ | xmllint --format -

You have to do propfind requests for public shares with and without passwords.

jnweiger

Tested the patch in https://188.34.186.241/owncloud (Classic UI only!)
Looks good. Thanks!

Or maybe not: Could that be related? #38543

C0rby · 2021-03-18T09:26:13Z

@micbar @jnweiger, regarding putting the full URL into downloadURL. Can I assume that the config option overwrite.cli.url will always be set? If not I'd leave it at the rooted url.

Or is there any other reliable way to get the owncloud base path like https://owncloud.example.com/ or https://example.com/owncloud/.

C0rby · 2021-03-18T10:04:02Z

I found a way. And now the downloadURL is a full URL not a rooted/relative URL. So the handling for the frontend should be easier now. :)

If owncloud was setup under a subdirectory it would return the path including the subdirectory which we don't want. Now we put the full URL into the downloadURL.

sonarqubecloud · 2021-03-18T12:52:48Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

C0rby added the 3 - To Review label Mar 16, 2021

C0rby requested review from jvillafanez, kulmann and micbar March 16, 2021 15:44

C0rby self-assigned this Mar 16, 2021

kulmann approved these changes Mar 16, 2021

View reviewed changes

C0rby changed the base branch from master to release-10.7.0 March 16, 2021 15:54

C0rby force-pushed the fix-public-link-downloads branch from 0fd331f to 496ab61 Compare March 16, 2021 15:59

micbar approved these changes Mar 16, 2021

View reviewed changes

micbar requested a review from jnweiger March 16, 2021 16:01

jnweiger reviewed Mar 17, 2021

View reviewed changes

jnweiger suggested changes Mar 17, 2021

View reviewed changes

jnweiger approved these changes Mar 17, 2021

View reviewed changes

C0rby force-pushed the fix-public-link-downloads branch from 496ab61 to 16ab8db Compare March 18, 2021 10:03

fix downloadURL of public links

55aa8b7

If owncloud was setup under a subdirectory it would return the path including the subdirectory which we don't want. Now we put the full URL into the downloadURL.

C0rby force-pushed the fix-public-link-downloads branch from 16ab8db to 55aa8b7 Compare March 18, 2021 10:41

C0rby merged commit 45d9b49 into release-10.7.0 Mar 18, 2021

delete-merged-branch bot deleted the fix-public-link-downloads branch March 18, 2021 14:14

jnweiger mentioned this pull request Apr 6, 2021

[QA] 2.1.0 Testplan owncloud/web#4873

Closed

2 tasks

Conversation

C0rby commented Mar 16, 2021

Description

Related Issue

Motivation and Context

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

kulmann left a comment

Choose a reason for hiding this comment

Uh oh!

jnweiger commented Mar 17, 2021

Uh oh!

jnweiger Mar 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kulmann Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

jvillafanez Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

C0rby Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

C0rby Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

jnweiger left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

C0rby commented Mar 17, 2021

Uh oh!

jnweiger left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

C0rby commented Mar 18, 2021

Uh oh!

C0rby commented Mar 18, 2021

Uh oh!

sonarqubecloud bot commented Mar 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jnweiger Mar 17, 2021 •

edited

Loading

jnweiger left a comment •

edited

Loading

jnweiger left a comment •

edited

Loading