Conversation
06547da to
57d945d
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
57d945d to
5f3da98
Compare
976883e to
9fc9f9a
Compare
abe6eeb to
87575f3
Compare
| * | ||
| * @package OCA\DAV\Files\PublicFiles | ||
| */ | ||
| class ShareNode extends Collection { |
There was a problem hiding this comment.
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.
| } | ||
| } | ||
|
|
||
| if (!$this->userSession->isLoggedIn() && \in_array('XMLHttpRequest', \explode(',', $request->getHeader('X-Requested-With')))) { |
There was a problem hiding this comment.
What if the user isn't logged in?
There was a problem hiding this comment.
Please ignore this commit - only here for testing purpose - THX
| */ | ||
| public function check(RequestInterface $request, ResponseInterface $response) { | ||
| $node = $this->resolveShare($request->getPath()); | ||
| if (!$node instanceof ShareNode && !$node instanceof SharedFile && !$node instanceof SharedFolder) { |
There was a problem hiding this comment.
Checking for IPublicSharedNode seems better
| if (!$node instanceof ShareNode && !$node instanceof SharedFile && !$node instanceof SharedFolder) { | ||
| return [true, 'principals/system/public']; | ||
| } | ||
| $this->share = $node->getShare(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
the resolvePath method only returns ShareNode - only for this shall be checked - will adjust the code - THX for the review
There was a problem hiding this comment.
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.
|
|
||
| /** | ||
| * @param string $path | ||
| * @return INode |
| if ($this->share->getNodeType() === 'folder') { | ||
| $nodes = $this->share->getNode()->getDirectoryListing(); | ||
| } else { | ||
| $nodes = [$this->share->getNode()]; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
the idea is that the root is always a folder which holds all files of the shared folder or only the one shared file.
| throw new Forbidden('Permission denied to create directory'); | ||
| } | ||
| if ($this->share->getNodeType() !== 'folder') { | ||
| throw new Forbidden('Permission denied to create directory'); |
There was a problem hiding this comment.
Maybe we should change the exception message.
| try { | ||
| $file = $this->share->getNode()->newFile($name); | ||
| $file->putContent(data); | ||
| return $file->getEtag(); |
There was a problem hiding this comment.
I assume that returning the etag here is documented somewhere... I find strange that this method returns the etag but the createDirectory doesn't
There was a problem hiding this comment.
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 🤷♂️
| try { | ||
| $this->file->delete(); | ||
| } catch (NotPermittedException $ex) { | ||
| throw new Forbidden('Permission denied to create directory'); |
There was a problem hiding this comment.
you aren't creating a directory here.
| } | ||
| } | ||
|
|
||
| private function nodeFactory(Node $node) { |
There was a problem hiding this comment.
we should move this to a different place, maybe a different class that can be injected both here and in the ShareNode
1fc87f9 to
5ecf099
Compare
| if (!$node instanceof ShareNode && !$node instanceof SharedFile && !$node instanceof SharedFolder) { | ||
| return [true, 'principals/system/public']; | ||
| } | ||
| $this->share = $node->getShare(); |
There was a problem hiding this comment.
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.
| * | ||
| * @var bool | ||
| */ | ||
| public $disableListing = false; |
There was a problem hiding this comment.
personal preference: private attribute with get + set methods in order to give more versatility in the future.
There was a problem hiding this comment.
I understand that - but I'm just following the sabre way of doing it 🤷♂️
| if ($username !== 'public') { | ||
| return false; | ||
| } | ||
| return $this->shareManager->checkPassword($this->share, $password); |
| $p .= 'NV'; // Renameable, Moveable | ||
| } | ||
| if ($this->checkPermissions(Constants::PERMISSION_CREATE)) { | ||
| $p .= 'CK'; |
| $p .= 'D'; | ||
| } | ||
| if ($this->checkPermissions(Constants::PERMISSION_UPDATE)) { | ||
| $p .= 'NV'; // Renameable, Moveable |
There was a problem hiding this comment.
Constants::PERMISSION_UPDATE = who is allowed to update the share, not the resource.
What does NV stand for?
| if ($this->checkPermissions(Constants::PERMISSION_CREATE)) { | ||
| $p .= 'CK'; | ||
| } | ||
| return $p; |
There was a problem hiding this comment.
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.
|
|
||
| public function getPermissions() { | ||
| $p = ''; | ||
| if ($this->checkPermissions(Constants::PERMISSION_DELETE)) { |
There was a problem hiding this comment.
Constants::PERMISSION_DELETE = who is allowed to delete the share, not the resource.
| // 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); |
There was a problem hiding this comment.
add to config.sample.php please
|
@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> |
|
|
Thank you. |
|
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> |
Why this URL? You have to use a public-link url ... |
|
@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. |
|
@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? |
|
Actually, I hadn't enabled <?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>"a28785e285ce0de0738676814705c4e1"</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. |
|
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.
This relates to owncloud/core/pull/35932 and #1871.
* 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.
Description
Related Issue
How Has This Been Tested?
Types of changes
Checklist:
Open tasks: