Add calendar sharing #21964

DeepDiver1975 · 2016-01-27T18:37:43Z

Test Scenario

create a calendar (./occ dav:create-calendar user shared-calendar)
POST a sharing xml request to the calendar url https://github.com/owncloud/core/blob/calendar-sharing-3/apps/dav/tests/travis/caldavtest/data/Resource/CalDAV/sharing/calendars/read-write/1.xml
see the calendar popping up with the other user

You can use cadaver to check for the calendar resources.

mention-bot · 2016-01-27T18:37:44Z

By analyzing the blame information on this pull request, we identified @schiesbn, @nickvergessen, @blizzz and @DeepDiver1975 to be potential reviewers

georgehrke · 2016-01-28T15:15:57Z

The response body for the post request is empty and the response code a 200. Shouldn't it be a 204 instead?

If I share a calendar with a user and a group the user is also a member of the calendar appears twice:

Listing collection `/remote.php/dav/calendars/user3/': succeeded.
Coll:   inbox                                  0  Jan  1  1970
Coll:   outbox                                 0  Jan  1  1970
Coll:   uni_shared_by_user1                    0  Jan  1  1970
Coll:   uni_shared_by_user1                    0  Jan  1  1970
dav:/remote.php/dav/calendars/user3/>

georgehrke · 2016-01-28T15:19:31Z

deleting events succeeds in a read-only calendar.

DeepDiver1975 · 2016-01-28T15:22:03Z

If I share a calendar with a user and a group the user is also a member of the calendar appears twice:

thx - note to self: use distinct

DeepDiver1975 · 2016-01-28T15:22:32Z

deleting events succeeds in a read-only calendar.

I'll have a look - THX

georgehrke · 2016-01-28T15:23:10Z

deleting events succeeds in a read-only calendar.

I used <CS:read/>. Is that actually the right thing to do?

DeepDiver1975 · 2016-01-28T19:33:50Z

I used CS:read/. Is that actually the right thing to do?

by default sharing is read only - you have to add CS:read-write/ to share it writable

DeepDiver1975 · 2016-01-28T19:34:10Z

@georgehrke your issues should be addressed now - thanks for testing!

georgehrke · 2016-01-28T19:59:11Z

apps/dav/lib/caldav/caldavbackend.php

+			list(, $name) = URLUtil::splitPath($row['principaluri']);
+			$uri = $row['uri'] . '_shared_by_' . $name;
+			$row['displayname'] = $row['displayname'] . "($name)";
+			$readOnly = ($row['access'] == Backend::ACCESS_READ);


maybe add a comment to the code on why you only use == instead of ===

georgehrke · 2016-01-28T19:59:54Z

Awesome, will retest asap.
Do you have XML examples for unsharing and updating permission for me?

georgehrke · 2016-01-28T20:06:39Z

apps/dav/lib/caldav/caldavbackend.php

@@ -153,10 +172,63 @@ function getCalendarsForUser($principalUri) {
 				$calendar[$xmlName] = $row[$dbName];
 			}

-			$calendars[] = $calendar;
+			$calendars[$calendar['id']] = $calendar;


e3ef67b#commitcomment-15744097

DeepDiver1975 · 2016-01-29T11:55:02Z

Do you have XML examples for unsharing and updating permission for me?

unshare

<?xml version="1.0" encoding="utf-8" ?>
<CS:share xmlns:D="DAV:" xmlns:CS="http://owncloud.org/ns">
    <CS:remove>
        <D:href>principal:principals/users/user02</D:href>
    </CS:remove>
</CS:share>

DeepDiver1975 · 2016-01-29T11:55:35Z

Updating permissions is done by just POSTing the set xml again

DeepDiver1975 · 2016-01-29T12:14:59Z

Will the user the calendar is shared with end up with a read-only or a read-write calendar?

this will all happen via the acls which are generated based on the shares which are attached to the calendar. These acls contain e.g. write privilege for the user and read for the group.

As soon as a write operation is issued on the calendar alls acl entires are evaluated.

So this should work already properly - I'll remove the access property within getCalendarsForUser.

blizzz · 2016-01-29T18:00:02Z

apps/dav/lib/caldav/caldavbackend.php

 		$this->db = $db;
+		$this->principalBackend = $principalBackend;
+		$this->sharingBackend = new Backend($this->db, 'calendar');


no DI necessary?

Looking at rest of the code it's ok

not really from my pov since this is there to share common code between the two backends

but I'm happy to discuss this

If I did not miss anything, they were onliners in methods, without further logic, so per se it's sufficient to test the Backend itself. OTOH it leaves a heritage, if the code would change in future.

DeepDiver1975 · 2016-02-02T12:08:46Z

delete has been fixed

DeepDiver1975 · 2016-02-02T12:10:51Z

How can I query a list of all sharees?

we have to implement something like https://trac.calendarserver.org/browser/CalendarServer/trunk/doc/Extensions/caldav-sharing.txt#L396

georgehrke · 2016-02-02T13:16:29Z

IMHO this is ready to be merged once the CS:invite property was taken care of

DeepDiver1975 · 2016-02-03T15:11:28Z

@georgehrke please have a look

georgehrke · 2016-02-03T15:13:50Z

Roger that!

georgehrke · 2016-02-03T15:34:22Z

user1 shared the calendar owncloud with group group1. user3 is a member of group1

dav:/remote.php/dav/calendars/user3/> ls
Listing collection `/remote.php/dav/calendars/user3/': succeeded.
Coll:   inbox                                  0  Jan  1  1970
Coll:   outbox                                 0  Jan  1  1970
Coll:   owncloud_shared_by_user1               0  Jan  1  1970
dav:/remote.php/dav/calendars/user3/> rmcol owncloud_shared_by_user1
Deleting collection `owncloud_shared_by_user1': succeeded.
dav:/remote.php/dav/calendars/user3/> ls
Listing collection `/remote.php/dav/calendars/user3/': succeeded.
Coll:   inbox                                  0  Jan  1  1970
Coll:   outbox                                 0  Jan  1  1970
Coll:   owncloud_shared_by_user1               0  Jan  1  1970
dav:/remote.php/dav/calendars/user3/>

It seems to respond with a success message, it should return an error code instead.

georgehrke · 2016-02-03T15:36:18Z

I think the behavior that user3 can't unshare the calendar for all members of group1 is correct though.

DeepDiver1975 · 2016-02-03T15:57:28Z

Since the share operation works on a list of people to be shared and unshared with error feedback does not really exist ..... anyway - let's see

What would be the appropriate status code? 403/Forbidden ?

georgehrke · 2016-02-03T15:58:42Z

What would be the appropriate status code? 403/Forbidden ?

Yes, 403 sounds just about right.

…part of the group

…is part of the group

…source

DeepDiver1975 · 2016-02-03T16:38:47Z

@georgehrke ...

georgehrke · 2016-02-03T17:23:47Z

Deleting a calendar still leaves dead entries in oc_dav_shares

georgehrke · 2016-02-03T17:24:42Z

looks good otherwise

georgehrke · 2016-02-03T19:58:16Z

👍

georgehrke · 2016-02-03T19:58:51Z

@blizzz could you review again? :)

LukasReschke · 2016-02-04T09:48:20Z

Works 👍

Add calendar sharing

DeepDiver1975 added the 2 - Developing label Jan 27, 2016

DeepDiver1975 added this to the 9.0-current milestone Jan 27, 2016

DeepDiver1975 added the in progress label Jan 27, 2016

DeepDiver1975 force-pushed the calendar-sharing-3 branch 2 times, most recently from 0dcdb3d to 3f88c20 Compare January 28, 2016 09:03

DeepDiver1975 mentioned this pull request Jan 28, 2016

Calendar sharing 2 #21910

Closed

1 task

DeepDiver1975 added 3 - To Review and removed 2 - Developing labels Jan 28, 2016

DeepDiver1975 self-assigned this Jan 28, 2016

DeepDiver1975 added 2 - Developing and removed 3 - To Review labels Jan 28, 2016

georgehrke reviewed Jan 28, 2016
View reviewed changes

DeepDiver1975 force-pushed the calendar-sharing-3 branch from 16b682a to 82ca5bd Compare January 29, 2016 13:36

DeepDiver1975 added 3 - To Review and removed 2 - Developing labels Jan 29, 2016

blizzz reviewed Jan 29, 2016
View reviewed changes

DeepDiver1975 added 8 commits February 3, 2016 17:18

Add calendar sharing

9106595

No duplicate calendars if shared with user and group and the user is …

18c35bf

…part of the group

No duplicate address book if shared with user and group and the user …

0753067

…is part of the group

Fix ACLs on shared calendars

4b14ca6

Disallow to share with the owner of the resource

6f933fd

Deleting a shared calendar results in unshare

ef06d6b

Deleting a shared address book results in unshare

8c14ce0

Adding propfind property to retrieve sharee information from a dav re…

a3cc448

…source

DeepDiver1975 force-pushed the calendar-sharing-3 branch from 7d60e3b to a3cc448 Compare February 3, 2016 16:18

Remove entries in dav_shares on deletion of a calendar

81b9721

DeepDiver1975 added a commit that referenced this pull request Feb 4, 2016

Merge pull request #21964 from owncloud/calendar-sharing-3

e64044d

Add calendar sharing

DeepDiver1975 merged commit e64044d into master Feb 4, 2016

DeepDiver1975 deleted the calendar-sharing-3 branch February 4, 2016 09:48

DeepDiver1975 removed the in progress label Feb 4, 2016

lock bot locked as resolved and limited conversation to collaborators Aug 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add calendar sharing #21964

Add calendar sharing #21964

DeepDiver1975 commented Jan 27, 2016

mention-bot commented Jan 27, 2016

georgehrke commented Jan 28, 2016

georgehrke commented Jan 28, 2016

DeepDiver1975 commented Jan 28, 2016

DeepDiver1975 commented Jan 28, 2016

georgehrke commented Jan 28, 2016

DeepDiver1975 commented Jan 28, 2016

DeepDiver1975 commented Jan 28, 2016

georgehrke Jan 28, 2016

georgehrke commented Jan 28, 2016

georgehrke Jan 28, 2016

DeepDiver1975 commented Jan 29, 2016

DeepDiver1975 commented Jan 29, 2016

DeepDiver1975 commented Jan 29, 2016

blizzz Jan 29, 2016

blizzz Jan 29, 2016

DeepDiver1975 Jan 29, 2016

blizzz Jan 29, 2016

DeepDiver1975 commented Feb 2, 2016

DeepDiver1975 commented Feb 2, 2016

georgehrke commented Feb 2, 2016

DeepDiver1975 commented Feb 3, 2016

georgehrke commented Feb 3, 2016

georgehrke commented Feb 3, 2016

georgehrke commented Feb 3, 2016

DeepDiver1975 commented Feb 3, 2016

georgehrke commented Feb 3, 2016

DeepDiver1975 commented Feb 3, 2016

georgehrke commented Feb 3, 2016

georgehrke commented Feb 3, 2016

georgehrke commented Feb 3, 2016

georgehrke commented Feb 3, 2016

LukasReschke commented Feb 4, 2016

Add calendar sharing #21964

Add calendar sharing #21964

Conversation

DeepDiver1975 commented Jan 27, 2016

Test Scenario

mention-bot commented Jan 27, 2016

georgehrke commented Jan 28, 2016

georgehrke commented Jan 28, 2016

DeepDiver1975 commented Jan 28, 2016

DeepDiver1975 commented Jan 28, 2016

georgehrke commented Jan 28, 2016

DeepDiver1975 commented Jan 28, 2016

DeepDiver1975 commented Jan 28, 2016

georgehrke Jan 28, 2016

Choose a reason for hiding this comment

georgehrke commented Jan 28, 2016

georgehrke Jan 28, 2016

Choose a reason for hiding this comment

DeepDiver1975 commented Jan 29, 2016

DeepDiver1975 commented Jan 29, 2016

DeepDiver1975 commented Jan 29, 2016

blizzz Jan 29, 2016

Choose a reason for hiding this comment

blizzz Jan 29, 2016

Choose a reason for hiding this comment

DeepDiver1975 Jan 29, 2016

Choose a reason for hiding this comment

blizzz Jan 29, 2016

Choose a reason for hiding this comment

DeepDiver1975 commented Feb 2, 2016

DeepDiver1975 commented Feb 2, 2016

georgehrke commented Feb 2, 2016

DeepDiver1975 commented Feb 3, 2016

georgehrke commented Feb 3, 2016

georgehrke commented Feb 3, 2016

georgehrke commented Feb 3, 2016

DeepDiver1975 commented Feb 3, 2016

georgehrke commented Feb 3, 2016

DeepDiver1975 commented Feb 3, 2016

georgehrke commented Feb 3, 2016

georgehrke commented Feb 3, 2016

georgehrke commented Feb 3, 2016

georgehrke commented Feb 3, 2016

LukasReschke commented Feb 4, 2016