Show sharees via propfind by tobiasKaminsky · Pull Request #14429 · nextcloud/server

tobiasKaminsky · 2019-02-28T09:30:51Z

❗ TODO: once this is merged, change/check OCFileListAdapter: showShareAvatar boolean ❗

Steps to test

create a file "share"
share this file to userA and userB
do a propfind with nc:sharees/, e.g.:

curl --request PROPFIND \
  --url http://localhost/nc/remote.php/webdav/share \
  --header 'authorization: Basic dG9iaTplOGVrei1SbVpaVC1DMkdDTi01clk2My1BeTl5QQ==' \
  --header 'content-type: application/xml' \
  --header 'ocs-apirequest: true' \
  --cookie 'ocjycgrudn78=vgovrq3bgraie6rs6ai2sl42ag; oc_sessionPassphrase=oyqxSU8b6n4N8Aj0yfObGeheppYvsdzgMhEkMgHw76nWBRLVRRM%252FZcjxqQ5QHtm3knScFxIKIjCzDasJYhLEYxRsvabseMjOqDSa%252BNSkI%252Fi%252FDrm6cKlWmA4QBr3NtcGI; nc_sameSiteCookielax=true; nc_sameSiteCookiestrict=true' \
  --data '<?xml version="1.0" encoding="UTF-8"?>
<D:propfind xmlns:D="DAV:">
<D:prop>
	
<sharees xmlns="http://nextcloud.org/ns" />
</D:prop></D:propfind>
'

see:
- Federated sharee: <nc:sharees>tobi@http://localhost/nc15</nc:sharees>
- Multiple sharees on same server: <nc:sharees>admin, Tobi Kaminsky</nc:sharees>
- One sharee on same server: <nc:sharees>admin</nc:sharees>

Please note that this shows only the user you have created & shared a file to.
The other way around (receiving a share) is already exists via < owner-id> and < owner-display-name>.

Signed-off-by: tobiasKaminsky tobias@kaminsky.me

blizzz

too ambiguous maybe

blizzz · 2019-02-28T10:41:36Z

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

+			Share::SHARE_TYPE_ROOM
+		];
+
+		if ($this->getPath() === "/") {


or-connect it with the condition in line 303

blizzz · 2019-02-28T10:47:29Z

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

+				}
+			}
+		}
+		return implode(', ', $sharees);


ain't it better return the array and format it where the output is being handled (the callee)? This also only works as long as "," is prohibited as identifier. Further, you have all the types mixed in and identifiers are not unique across those types. → What do you want to achieve eventually?

If I simply return $sharees, I do get that as plain string, e.g. "adminuser1user2".
If you have a better way, please feel free to change it or give me a hint.
I do not "care" as long as the client has a list of userIds of all sharees.

I removed Circle and Rooms, so that now only user, remote user and groups are returned.

blizzz · 2019-02-28T16:38:33Z

as discussed on irc, we agreed about not returning grouped by type and and each user, group etc. is inside an id-tag within the appropriate element.

rullzer

So I knew this looked familiar.

Because the way this is implemented it won't scale. If you request this property for a user on a share with 200 entries. It will fire off 300 SQL requests.

We should bundle it with https://github.com/nextcloud/server/blob/master/apps/dav/lib/Connector/Sabre/SharesPlugin.php

rullzer · 2019-03-01T09:39:49Z

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

+				$path = $userFolder->get($path);
+				$this->lock($path);
+			} catch (\OCP\Files\NotFoundException $e) {
+				throw new OCSNotFoundException($this->l->t('Wrong path, file/folder doesn\'t exist'));


This should not be here

rullzer · 2019-03-01T09:39:59Z

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

+			} catch (\OCP\Files\NotFoundException $e) {
+				throw new OCSNotFoundException($this->l->t('Wrong path, file/folder doesn\'t exist'));
+			} catch (LockedException $e) {
+				throw new OCSNotFoundException($this->l->t('Could not lock path'));


Same. This is not an OCS endpoint

tobiasKaminsky · 2019-07-02T06:58:49Z

Bump, can anyone help me here? Or direct me in the right direction? :-)

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

tobiasKaminsky · 2019-07-23T08:35:22Z

I really would like to have this in.

I changed it as good as I can and can help/rewrite if needed.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

This makes the XML parsing more sane ;) Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer · 2019-07-31T11:12:12Z

Let me fix the tests this evening

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

rullzer · 2019-07-31T20:39:44Z

Ok so the test require some more work as they were/are not the cleanest. I'll have a look tomorrow.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer · 2019-08-02T09:36:34Z

Test all look good. Time for reviews.

rullzer

All good from my part

rullzer · 2019-08-02T09:37:31Z

Small note. Once we have this we could also enhance the server UI.
We could show who a file is shared to more easily etc.

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

georgehrke · 2019-08-06T08:09:45Z

Besides those two remarks it looks good and works

no camelCase Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

tobiasKaminsky · 2019-08-06T08:34:01Z

Can you test this please with an admin account:

<nc:sharees>
          <nc:sharee>
            <nc:id>admin</nc:id>
            <nc:display-name/>
            <nc:type>1</nc:type>
          </nc:sharee>
          <nc:sharee>
            <nc:id>user1</nc:id>
            <nc:display-name>User One</nc:display-name>
            <nc:type>0</nc:type>
          </nc:sharee>
          <nc:sharee>
            <nc:id>user2</nc:id>
            <nc:display-name>user2</nc:display-name>
            <nc:type>0</nc:type>
          </nc:sharee>
        </nc:sharees>

For some reason the display name of an admin is missing, but it should be at least userId.
It is working for other users…
I just want to be sure if this is a problem of my local system…

georgehrke · 2019-08-06T08:41:37Z

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

+		foreach ($this->shares as $share) {
+			$writer->startElement('{' . self::NS_NEXTCLOUD . '}sharee');
+			$writer->writeElement('{' . self::NS_NEXTCLOUD . '}id', $share->getSharedWith());
+			$writer->writeElement('{' . self::NS_NEXTCLOUD . '}display-name', $share->getSharedWithDisplayName());


Suggested change

$writer->writeElement('{' . self::NS_NEXTCLOUD . '}display-name', $share->getSharedWithDisplayName());

$writer->writeElement('{' . self::NS_NEXTCLOUD . '}display-name', $share->getSharedWithDisplayName() ?? $share->getSharedWith());

I wrote a test for android library and there the problem also exists.
It seems that always one of the display names is set to userId.

tobiasKaminsky · 2019-08-06T12:04:58Z

BUG:

create folder "testFolder"
share it to user1, user2
query "/remote.php/webdav/", see

 <d:response>
    <d:href>/remote.php/webdav/testFolder/</d:href>
    <d:propstat>
      <d:prop>
        <nc:sharees>
          <nc:sharee>
            <nc:id>user1</nc:id>
            <nc:display-name>User One</nc:display-name>
            <nc:type>0</nc:type>
          </nc:sharee>
          <nc:sharee>
            <nc:id>user2</nc:id>
            <nc:display-name>user2</nc:display-name>
            <nc:type>0</nc:type>
          </nc:sharee>
        </nc:sharees>
      </d:prop>
      <d:status>HTTP/1.1 200 OK</d:status>
    </d:propstat>
  </d:response>

query "/remote.php/webdav/testFolder", see:

 <d:response>
    <d:href>/remote.php/webdav/testFolder/</d:href>
    <d:propstat>
      <d:prop>
        <nc:sharees>
          <nc:sharee>
            <nc:id>user1</nc:id>
            <nc:display-name>User One</nc:display-name>
            <nc:type>0</nc:type>
          </nc:sharee>
        </nc:sharees>
      </d:prop>
      <d:status>HTTP/1.1 200 OK</d:status>
    </d:propstat>
  </d:response>

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

tobiasKaminsky · 2019-08-08T09:01:39Z

Bug fixed via latest commit.

tobiasKaminsky · 2019-08-09T06:32:54Z

From my side this is ready to get in

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer · 2019-08-11T19:35:01Z

Fixed the failing tests

georgehrke

Seems to do what its supposed do and webdav compliance wise I'm also happy with it 👍

tobiasKaminsky requested review from blizzz and rullzer February 28, 2019 09:31

tobiasKaminsky added enhancement 3. to review Waiting for reviews feature: sharing feature: dav labels Feb 28, 2019

tobiasKaminsky added this to the Nextcloud 16 milestone Feb 28, 2019

blizzz reviewed Feb 28, 2019

View reviewed changes

tobiasKaminsky force-pushed the shareesOnDav branch from 40f086b to 3ff5424 Compare February 28, 2019 13:52

rullzer requested changes Mar 1, 2019

View reviewed changes

MorrisJobke mentioned this pull request Mar 4, 2019

16.0.0 Alpha 1 #14501

Merged

45 tasks

rullzer mentioned this pull request Mar 4, 2019

Add note to dav endpoint #12978

Merged

rullzer modified the milestones: Nextcloud 16, Nextcloud 17 Mar 4, 2019

tobiasKaminsky force-pushed the shareesOnDav branch from 3ff5424 to 221a8c6 Compare March 18, 2019 07:57

tobiasKaminsky mentioned this pull request Mar 18, 2019

Show sharees nextcloud/android#3765

Merged

6 tasks

MorrisJobke mentioned this pull request Jul 17, 2019

17.0.0 Beta 1 #16408

Merged

28 tasks

tobiasKaminsky added 2 commits July 22, 2019 13:59

Show sharees via propfind

e3991fb

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

wip

b81fb18

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

tobiasKaminsky force-pushed the shareesOnDav branch 2 times, most recently from b7c0bdb to b965c79 Compare July 23, 2019 08:33

wip

efa0733

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

tobiasKaminsky force-pushed the shareesOnDav branch from b965c79 to efa0733 Compare July 23, 2019 08:34

tobiasKaminsky requested review from MorrisJobke, blizzz and rullzer July 23, 2019 08:34

Use proper caching

06a9ade

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer added 2 commits July 30, 2019 22:51

Use proper ShareeList

2a45763

This makes the XML parsing more sane ;) Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Unify share fetching

21477bc

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

add display name to sharee list

42f00e8

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

This was referenced Jul 31, 2019

Change sharees format nextcloud/android-library#325

Merged

Sharees will be in NC17 nextcloud/android#4284

Merged

Fix tests

85d5400

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer requested review from georgehrke and juliusknorr August 2, 2019 09:36

rullzer approved these changes Aug 2, 2019

View reviewed changes

georgehrke requested changes Aug 6, 2019

View reviewed changes

apps/dav/lib/Connector/Sabre/ShareeList.php Outdated Show resolved Hide resolved

apps/dav/lib/Connector/Sabre/ShareeList.php Outdated Show resolved Hide resolved

use namespace everywhere

82ae6fb

no camelCase Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

georgehrke reviewed Aug 6, 2019

View reviewed changes

get all shares, not only first one per type

eb564e8

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

tobiasKaminsky requested a review from georgehrke August 9, 2019 06:32

Fix tests

a853926

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

georgehrke approved these changes Aug 13, 2019

View reviewed changes

rullzer merged commit 34868f0 into nextcloud:master Aug 13, 2019

tobiasKaminsky deleted the shareesOnDav branch August 14, 2019 04:47

	$writer->writeElement('{' . self::NS_NEXTCLOUD . '}display-name', $share->getSharedWithDisplayName());
	$writer->writeElement('{' . self::NS_NEXTCLOUD . '}display-name', $share->getSharedWithDisplayName() ?? $share->getSharedWith());

Uh oh!

Conversation

tobiasKaminsky commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blizzz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blizzz commented Feb 28, 2019

Uh oh!

rullzer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasKaminsky commented Jul 2, 2019

Uh oh!

tobiasKaminsky commented Jul 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rullzer commented Jul 31, 2019

Uh oh!

rullzer commented Jul 31, 2019

Uh oh!

rullzer commented Aug 2, 2019

Uh oh!

rullzer left a comment

Choose a reason for hiding this comment

Uh oh!

rullzer commented Aug 2, 2019

Uh oh!

Uh oh!

Uh oh!

georgehrke commented Aug 6, 2019

Uh oh!

tobiasKaminsky commented Aug 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasKaminsky commented Aug 6, 2019

Uh oh!

tobiasKaminsky commented Aug 8, 2019

Uh oh!

tobiasKaminsky commented Aug 9, 2019

Uh oh!

rullzer commented Aug 11, 2019

Uh oh!

georgehrke left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tobiasKaminsky commented Feb 28, 2019 •

edited

Loading

tobiasKaminsky commented Jul 23, 2019 •

edited

Loading