Feature/public links #35932

DeepDiver1975 · 2019-07-26T13:12:29Z

Description

Allow access to public links via webdav
redirect public links to phoenix

Related Issue

How Has This Been Tested?

🚧

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:

Open tasks:

Backport (if applicable set "backport-request" label and remove when the backport was done)

codecov · 2019-07-29T12:02:44Z

Codecov Report

Merging #35932 into stable10 will decrease coverage by 0.09%.
The diff coverage is 11.85%.

@@              Coverage Diff              @@
##             stable10   #35932     +/-   ##
=============================================
- Coverage        65.1%      65%   -0.1%     
- Complexity      20197    20244     +47     
=============================================
  Files            1302     1307      +5     
  Lines           77158    77291    +133     
  Branches         1301     1301             
=============================================
+ Hits            50232    50246     +14     
- Misses          26541    26660    +119     
  Partials          385      385

Flag	Coverage Δ	Complexity Δ
#javascript	`53.85% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`66.18% <11.85%> (-0.11%)`	`20244 <47> (+47)`

Impacted Files	Coverage Δ	Complexity Δ
apps/dav/lib/DAV/PublicAuth.php	`16.66% <ø> (ø)`	`5 <0> (ø)`	⬇️
core/routes.php	`48.71% <0%> (-7.17%)`	`0 <0> (ø)`
apps/dav/lib/Files/PublicFiles/SharedFolder.php	`0% <0%> (ø)`	`13 <13> (?)`
apps/dav/lib/Files/PublicFiles/SharedFile.php	`0% <0%> (ø)`	`10 <10> (?)`
lib/private/Files/Node/Node.php	`86.95% <0%> (-1.28%)`	`63 <1> (+1)`
apps/dav/lib/Files/PublicFiles/ShareNode.php	`0% <0%> (ø)`	`8 <8> (?)`
apps/dav/lib/RootCollection.php	`100% <100%> (ø)`	`1 <0> (ø)`	⬇️
apps/dav/lib/Server.php	`53.74% <100%> (+0.31%)`	`24 <0> (ø)`	⬇️
apps/dav/lib/Files/PublicFiles/RootCollection.php	`27.77% <27.77%> (ø)`	`7 <7> (?)`
...ps/dav/lib/Files/PublicFiles/PublicSharingAuth.php	`27.77% <27.77%> (ø)`	`8 <8> (?)`
... and 5 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 c688803...57d945d. Read the comment docs.

codecov · 2019-07-29T12:02:45Z

Codecov Report

Merging #35932 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #35932   +/-   ##
=======================================
  Coverage   53.85%   53.85%           
=======================================
  Files          63       63           
  Lines        7377     7377           
  Branches     1301     1301           
=======================================
  Hits         3973     3973           
  Misses       3019     3019           
  Partials      385      385

Flag	Coverage Δ
#javascript	`53.85% <ø> (ø)`	⬆️

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 1c27f4f...5ecf099. Read the comment docs.

CLAassistant · 2019-08-06T12:19:42Z

All committers have signed the CLA.

jvillafanez · 2019-08-08T12:45:34Z

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

+ *
+ * @package OCA\DAV\Files\PublicFiles
+ */
+class ShareNode extends Collection {


Maybe we should change the name of the class to PublicSharedNodeRoot. This should make more clear than it's expected to contain PublicSharedNodes.
I'm not sure if it should also implement the IPublicSharedNode.

jvillafanez · 2019-08-08T12:47:33Z

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

@@ -237,13 +236,6 @@ private function auth(RequestInterface $request, ResponseInterface $response) {
 			}
 		}

-		if (!$this->userSession->isLoggedIn() && \in_array('XMLHttpRequest', \explode(',', $request->getHeader('X-Requested-With')))) {


What if the user isn't logged in?

Please ignore this commit - only here for testing purpose - THX

jvillafanez · 2019-08-08T12:50:59Z

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

+	 */
+	public function check(RequestInterface $request, ResponseInterface $response) {
+		$node = $this->resolveShare($request->getPath());
+		if (!$node instanceof ShareNode && !$node instanceof SharedFile && !$node instanceof SharedFolder) {


Checking for IPublicSharedNode seems better

jvillafanez · 2019-08-08T12:56:31Z

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

+		if (!$node instanceof ShareNode && !$node instanceof SharedFile && !$node instanceof SharedFolder) {
+			return [true, 'principals/system/public'];
+		}
+		$this->share = $node->getShare();


Taking into account that $node can be 3 different classes, I think this is a risky call. There is no common interface to ensure that the method will exists.
It's easy to assume that $node is of one specific instance neglecting the other 2, which will cause problems in the long run.

the resolvePath method only returns ShareNode - only for this shall be checked - will adjust the code - THX for the review

Another question around here: why do we need the $this->share stored as a class attribute?. I only see the reference here, so I'd be very tempted of using just a local variable instead of an attribute.
If this is need, please document the reason in order to prevent unwanted changes.

jvillafanez · 2019-08-08T12:58:27Z

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

+
+	/**
+	 * @param string $path
+	 * @return INode


it can also return null

jvillafanez · 2019-08-08T13:05:59Z

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

+		if ($this->share->getNodeType() === 'folder') {
+			$nodes = $this->share->getNode()->getDirectoryListing();
+		} else {
+			$nodes = [$this->share->getNode()];


hmmm, if the share is a file, the children list contains the file, but if the share is a folder, the folder doesn't appear. I think it's better to return an empty list if the share is a file, if we don't want to throw an exception.

the idea is that the root is always a folder which holds all files of the shared folder or only the one shared file.

jvillafanez · 2019-08-08T13:07:38Z

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

+			throw new Forbidden('Permission denied to create directory');
+		}
+		if ($this->share->getNodeType() !== 'folder') {
+			throw new Forbidden('Permission denied to create directory');


Maybe we should change the exception message.

jvillafanez · 2019-08-08T13:10:38Z

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

+		try {
+			$file = $this->share->getNode()->newFile($name);
+			$file->putContent(data);
+			return $file->getEtag();


I assume that returning the etag here is documented somewhere... I find strange that this method returns the etag but the createDirectory doesn't

will add some docs on this - basically copy over the base class phpdoc.

createDir cannot return an etag because in plain webdav a directory has no etag 🤷‍♂️

jvillafanez · 2019-08-08T13:13:24Z

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

+		try {
+			$this->file->delete();
+		} catch (NotPermittedException $ex) {
+			throw new Forbidden('Permission denied to create directory');


you aren't creating a directory here.

jvillafanez · 2019-08-08T13:21:25Z

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

+		}
+	}
+
+	private function nodeFactory(Node $node) {


we should move this to a different place, maybe a different class that can be injected both here and in the ShareNode

jvillafanez · 2019-08-09T07:36:46Z

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

+		if (!$node instanceof ShareNode && !$node instanceof SharedFile && !$node instanceof SharedFolder) {
+			return [true, 'principals/system/public'];
+		}
+		$this->share = $node->getShare();


Another question around here: why do we need the $this->share stored as a class attribute?. I only see the reference here, so I'd be very tempted of using just a local variable instead of an attribute.
If this is need, please document the reason in order to prevent unwanted changes.

jvillafanez · 2019-08-09T07:40:02Z

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

+	 *
+	 * @var bool
+	 */
+	public $disableListing = false;


personal preference: private attribute with get + set methods in order to give more versatility in the future.

I understand that - but I'm just following the sabre way of doing it 🤷‍♂️

DeepDiver1975 · 2019-08-09T08:06:22Z

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

+		if ($username !== 'public') {
+			return false;
+		}
+		return $this->shareManager->checkPassword($this->share, $password);


@jvillafanez this->share is used down here

butonic · 2019-08-09T09:04:41Z

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

+			$p .= 'NV'; // Renameable, Moveable
+		}
+		if ($this->checkPermissions(Constants::PERMISSION_CREATE)) {
+			$p .= 'CK';


What does CK stand for?

butonic · 2019-08-09T09:04:51Z

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

+			$p .= 'D';
+		}
+		if ($this->checkPermissions(Constants::PERMISSION_UPDATE)) {
+			$p .= 'NV'; // Renameable, Moveable


Constants::PERMISSION_UPDATE = who is allowed to update the share, not the resource.
What does NV stand for?

butonic · 2019-08-09T09:06:05Z

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

+		if ($this->checkPermissions(Constants::PERMISSION_CREATE)) {
+			$p .= 'CK';
+		}
+		return $p;


What about the Reshare permission?
What about extended share attributes?
Not relevant here because the permissions are used to restrict share management, not file resource access.

butonic · 2019-08-09T09:22:37Z

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

+
+	public function getPermissions() {
+		$p = '';
+		if ($this->checkPermissions(Constants::PERMISSION_DELETE)) {


Constants::PERMISSION_DELETE = who is allowed to delete the share, not the resource.

butonic · 2019-08-09T09:30:00Z

core/routes.php

@@ -92,7 +92,13 @@
 });

 // Sharing routes
-$this->create('files_sharing.sharecontroller.showShare', '/s/{token}')->action(function ($urlParams) {
+$this->create('files_sharing.sharecontroller.showShare', '/s/{token}')->action(static function ($urlParams) {
+	$phoenixBaseUrl = \OC::$server->getConfig()->getSystemValue('phoenix.baseUrl', null);


add to config.sample.php please

settermjd · 2019-08-19T09:59:36Z

@DeepDiver1975, wrt documenting this, please provide a sample request body for a PROPFIND request. I'm using the following, with limited success:

<?xml version="1.0"?>
<d:propfind xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns">
    <d:prop>
        <d:getexpirationdate />
        <d:getnodetype />
        <d:getpermissions />
        <d:getshareowner />
        <d:getsharetime />
    </d:prop>
</d:propfind>

DeepDiver1975 · 2019-08-19T10:14:50Z

use namespace oc - not d
use property names as in the code - e.g. public-link-item-type

settermjd · 2019-08-19T11:17:50Z

Thank you.

settermjd · 2019-08-19T11:29:44Z

Can you shed light on why the following request results in no results found?

REQUEST_BODY=$(cat <<EOF
<?xml version="1.0"?>
<d:propfind xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns">
    <d:prop>
        <oc:public-link-item-type />
        <oc:public-link-item-permission />
        <oc:public-link-item-expiration />
        <oc:public-link-item-share-datetime />
        <oc:public-link-item-owner />
    </d:prop>
</d:propfind>
EOF
)

curl -s --silent 'https://test.owncloud.domain/remote.php/dav/principals' \
  -H 'Content-Type: application/xml; charset=UTF-8' \
  -H 'Depth: 1' \
  -X PROPFIND \
  --data-binary "${REQUEST_BODY}" \
  --user "admin:password"

<?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/principals/</d:href>
		<d:propstat>
			<d:prop>
				<oc:public-link-item-type/>
				<oc:public-link-item-permission/>
				<oc:public-link-item-expiration/>
				<oc:public-link-item-share-datetime/>
				<oc:public-link-item-owner/>
			</d:prop>
			<d:status>HTTP/1.1 404 Not Found</d:status>
		</d:propstat>
	</d:response>
	<d:response>
		<d:href>/remote.php/dav/principals/users/</d:href>
		<d:propstat>
			<d:prop>
				<oc:public-link-item-type/>
				<oc:public-link-item-permission/>
				<oc:public-link-item-expiration/>
				<oc:public-link-item-share-datetime/>
				<oc:public-link-item-owner/>
			</d:prop>
			<d:status>HTTP/1.1 404 Not Found</d:status>
		</d:propstat>
	</d:response>
	<d:response>
		<d:href>/remote.php/dav/principals/groups/</d:href>
		<d:propstat>
			<d:prop>
				<oc:public-link-item-type/>
				<oc:public-link-item-permission/>
				<oc:public-link-item-expiration/>
				<oc:public-link-item-share-datetime/>
				<oc:public-link-item-owner/>
			</d:prop>
			<d:status>HTTP/1.1 404 Not Found</d:status>
		</d:propstat>
	</d:response>
	<d:response>
		<d:href>/remote.php/dav/principals/system/</d:href>
		<d:propstat>
			<d:prop>
				<oc:public-link-item-type/>
				<oc:public-link-item-permission/>
				<oc:public-link-item-expiration/>
				<oc:public-link-item-share-datetime/>
				<oc:public-link-item-owner/>
			</d:prop>
			<d:status>HTTP/1.1 404 Not Found</d:status>
		</d:propstat>
	</d:response>
</d:multistatus>

DeepDiver1975 · 2019-08-19T11:31:52Z

'https://test.owncloud.domain/remote.php/dav/principals'

Why this URL? You have to use a public-link url ...
https://test.owncloud.domain/remote.php/dav/public-files/${SHARE_TOKEN}

settermjd · 2019-10-02T09:53:20Z

@DeepDiver1975, thanks for the update. It took a bit of fiddling and some research, but I'm making progress. I hope to have this documented this week.

settermjd · 2019-10-02T10:40:57Z

@DeepDiver1975, hoping that you can shed some light on this.

When attempting to view a public file, I get the following response:

<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAVACL\Exception\NeedPrivileges</s:exception>
  <s:message>User did not have the required privileges ({DAV:}read) for path "public-files/GbgdLgcoqYv8RF5"</s:message>
  <d:need-privileges>
    <d:resource>
      <d:href>/remote.php/dav/public-files/GbgdLgcoqYv8RF5</d:href>
      <d:privilege>
        <d:read/>
      </d:privilege>
    </d:resource>
  </d:need-privileges>
</d:error>

However, if I retrieve the details of that file, read permission appears to be set, as the following response shows:

{
  "ocs": {
    "meta": {
      "status": "ok",
      "statuscode": 100,
      "message": null,
      "totalitems": "",
      "itemsperpage": ""
    },
    "data": [
      {
        "id": "3",
        "share_type": 3,
        "uid_owner": "admin",
        "displayname_owner": "admin",
        "permissions": 1,
        "stime": 1569930985,
        "parent": null,
        "expiration": null,
        "token": "GbgdLgcoqYv8RF5",
        "uid_file_owner": "admin",
        "displayname_file_owner": "admin",
        "path": "/welcome.txt",
        "item_type": "file",
        "mimetype": "text/plain",
        "storage_id": "home::admin",
        "storage": 1,
        "item_source": 4,
        "file_source": 4,
        "file_parent": 3,
        "file_target": "/welcome.txt",
        "name": "welcome.txt",
        "url": "http://owncloud.localdomain/index.php/s/GbgdLgcoqYv8RF5",
        "mail_send": 0,
        "attributes": null
      }
    ]
  }
}

Any suggestions?

settermjd · 2019-10-02T10:55:25Z

Actually, I hadn't enabled 'dav.enable.tech_preview' => true, in config.php. Now that I have, that error's gone and the following appears:

<?xml version="1.0"?>
<d:multistatus xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns" xmlns:s="http://sabredav.org/ns">
  <d:response>
    <d:href>/remote.php/dav/public-files/GbgdLgcoqYv8RF5/</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/GbgdLgcoqYv8RF5/welcome.txt</d:href>
    <d:propstat>
      <d:prop>
        <d:getlastmodified>Mon, 30 Sep 2019 12:13:02 GMT</d:getlastmodified>
        <d:getcontentlength>0</d:getcontentlength>
        <d:resourcetype/>
        <d:getetag>&quot;a28785e285ce0de0738676814705c4e1&quot;</d:getetag>
        <d:getcontenttype>text/plain</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>

I may just have a typo in the token.

settermjd · 2019-10-02T10:58:00Z

I've double-checked the token and it's correct. I also tried a few others and received the same response. Any ideas?

This relates to owncloud/core/pull/35932 and #1871.

@deepdiver

* Document the public-files WebDAV API endpoint This relates to owncloud/core/pull/35932 and #1871. * Remove username and password in PHP and Curl examples These are added as per @deepdiver's comments https://github.com/owncloud/docs/pull/1879/files#r332913897. An admonition's been added to document how to access the public links when they are password protected.

DeepDiver1975 added the 2 - Developing label Jul 26, 2019

DeepDiver1975 requested review from PVince81 and butonic July 26, 2019 13:12

DeepDiver1975 self-assigned this Jul 26, 2019

DeepDiver1975 force-pushed the feature/public-links branch from 06547da to 57d945d Compare July 29, 2019 12:02

DeepDiver1975 force-pushed the feature/public-links branch from 57d945d to 5f3da98 Compare July 29, 2019 12:26

DeepDiver1975 changed the base branch from stable10 to master July 30, 2019 08:20

DeepDiver1975 mentioned this pull request Jul 31, 2019

Add public link display owncloud/web#1670

Merged

18 tasks

DeepDiver1975 force-pushed the feature/public-links branch 9 times, most recently from 976883e to 9fc9f9a Compare August 6, 2019 09:29

DeepDiver1975 force-pushed the feature/public-links branch 2 times, most recently from abe6eeb to 87575f3 Compare August 8, 2019 11:02

DeepDiver1975 added 3 - To Review and removed 2 - Developing labels Aug 8, 2019

DeepDiver1975 requested a review from jvillafanez August 8, 2019 12:09

jvillafanez reviewed Aug 8, 2019

View reviewed changes

DeepDiver1975 added 4 commits August 8, 2019 15:52

Add public-files to DAV including ACL handling

76b04ab

Add password verification for public shares

4c72bc4

Redirect public link to phoenix

e9c2fa6

Add some properties to public files resource

6a563c7

DeepDiver1975 force-pushed the feature/public-links branch from 1fc87f9 to 5ecf099 Compare August 8, 2019 15:47

jvillafanez reviewed Aug 9, 2019

View reviewed changes

DeepDiver1975 requested a review from jvillafanez August 9, 2019 08:04

DeepDiver1975 commented Aug 9, 2019

View reviewed changes

jvillafanez approved these changes Aug 9, 2019

View reviewed changes

DeepDiver1975 merged commit 6cefd5e into master Aug 9, 2019

delete-merged-branch bot deleted the feature/public-links branch August 9, 2019 09:26

DeepDiver1975 mentioned this pull request Aug 9, 2019

Acceptance Tests for Public-Links-API #36006

Closed

butonic reviewed Aug 9, 2019

View reviewed changes

DeepDiver1975 mentioned this pull request Aug 9, 2019

Document phoenix.baseUrl in config.sample.php #36007

Merged

11 tasks

phil-davis added the release:10.3.0 label Aug 13, 2019

patrickjahns added the changelog-added label Aug 15, 2019

individual-it mentioned this pull request Aug 20, 2019

all results that work in the old public link webDAV API should also work in the new one #36051

Merged

11 tasks

davitol mentioned this pull request Sep 3, 2019

10.3 - Changelog test Results #36142

Closed

13 tasks

pmaier1 mentioned this pull request Sep 30, 2019

Document new public link endpoints owncloud/docs#1871

Closed

12 tasks

butonic mentioned this pull request Oct 1, 2019

[reva] public links webdav API owncloud/ocis#18

Closed

settermjd added a commit to owncloud/docs that referenced this pull request Oct 2, 2019

Document the public-files WebDAV API endpoint

e3501dc

This relates to owncloud/core/pull/35932 and #1871.

settermjd mentioned this pull request Oct 2, 2019

Document the public-files WebDAV API endpoint owncloud/docs#1879

Merged

settermjd added a commit to owncloud/docs that referenced this pull request Oct 4, 2019

Document the public-files WebDAV API endpoint

0660804

This relates to owncloud/core/pull/35932 and #1871.

butonic mentioned this pull request Nov 11, 2019

implement /remote.php/dav/public-files endpoint owncloud/ocis#46

Closed

Feature/public links #35932

Feature/public links #35932

Conversation

DeepDiver1975 commented Jul 26, 2019 • edited Loading

Description

Related Issue

How Has This Been Tested?

Types of changes

Checklist:

Open tasks:

codecov bot commented Jul 29, 2019

Codecov Report

codecov bot commented Jul 29, 2019 • edited Loading

Codecov Report

CLAassistant commented Aug 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

settermjd commented Aug 19, 2019

DeepDiver1975 commented Aug 19, 2019

settermjd commented Aug 19, 2019

settermjd commented Aug 19, 2019

DeepDiver1975 commented Aug 19, 2019

settermjd commented Oct 2, 2019

settermjd commented Oct 2, 2019 • edited Loading

settermjd commented Oct 2, 2019

settermjd commented Oct 2, 2019

DeepDiver1975 commented Jul 26, 2019 •

edited

Loading

codecov bot commented Jul 29, 2019 •

edited

Loading

CLAassistant commented Aug 6, 2019 •

edited

Loading

settermjd commented Oct 2, 2019 •

edited

Loading