Fix the FreeBusy request handling for proper scheduling support #6715

go2sh · 2017-10-01T14:18:24Z

Currently the FreeBusy Request are answered with a 404 Status. and it is not possible to receive a proper FreeBusy status with e.g. Thunderbird. This is due to a function missing in the NC Principal Backend. Further the ACL for User Principals need to be adjusted to allow propFinds to find the scheduling boxes for principles others than the owner.

I know that especially the ACL fix is not perfect at all and it is just a test get the FreeBusy stuff working. I just saw, that NC subclasses the Calendar to generate a proper ACL based on the sharing options. Maybe subclassing the User class would be a better way. But I'am not sure about the right solution. So this is open for discussion. :)

See: https://help.nextcloud.com/t/future-of-sabre-dav/21739
~~Requires: nextcloud/3rdparty#68~~

PS.: My first NC PR. 🙈

cc @georgehrke

georgehrke · 2017-10-01T14:58:11Z

Maybe subclassing the User class would be a better way.

I think this would be the way to go.
The constructor of \Sabre\CalDAV\Principal\User is only ever called in \Sabre\CalDAV\Principal\Collection::getChildForPrincipal. The constructor of \Sabre\CalDAV\Principal\Collection is only called by Nextcloud and not Sabre/Dav itself, so we can simply subclass \Sabre\CalDAV\Principal\Collection and make it return our very own \Sabre\CalDAV\Principal\User.

georgehrke · 2017-10-01T14:58:56Z

Did you check if there is a bug report about this upstream in Sabre/Dav?

go2sh · 2017-10-01T17:03:52Z

Did you check if there is a bug report about this upstream in Sabre/Dav?

Yes I did. There is no Issue in that direction. I'am not sure how they handle it there.

I will have a look and try to create a solution with subclassing. Stay tuned. :-)

go2sh · 2017-10-02T13:24:58Z

@georgehrke Done. Now authenticated users are allowed to execute a PROPFIND on user principles through nextcloud. Is that enough? Or should we restrict it to some thing like the same group eg?

georgehrke · 2017-10-02T13:30:31Z

Or should we restrict it to some thing like the same group e.g.?

Yes, I think we should connect this to the settings of the Share API.
If the Share API is disabled => no Free/Busy
If the Share API is restricted to groups => Free/Busy only for users from the same group

Do you mind looking into the failing unit tests and adding a unit test for your changes? :)
An integration test would also be great!

cc @LukasReschke about the ACL changes

go2sh · 2017-10-02T13:51:26Z

I'll fix and add some tests, no problem

One more thing. I just patched our server and a found out, that shared calendars also being taken into account... :( They should be excluded otherwise the FreeBusy status is not usable.

Edit: This is a real big problem... Due to the way shared calenders are handle in NC, I think there is no easy way to fix this...

georgehrke · 2017-10-03T09:19:29Z

Edit: This is a real big problem... Due to the way shared calenders are handle in NC, I think there is no easy way to fix this...

I will try to look into this soon.

Maybe we should distinguish between read only and read write shared calendars. This could be tricky. We should research how other CalDAV servers handle this.

georgehrke · 2017-10-31T19:25:23Z

@go2sh The shared calendar issue should be fixable by making all shared calendars transparent here: https://github.com/nextcloud/server/blob/master/apps/dav/lib/CalDAV/CalDavBackend.php#L339

(https://tools.ietf.org/html/rfc6638#section-9.1)

Can you confirm this?

codecov · 2017-11-03T13:34:08Z

Codecov Report

Merging #6715 into master will increase coverage by <.01%.
The diff coverage is 62.96%.

@@             Coverage Diff              @@
##             master    #6715      +/-   ##
============================================
+ Coverage     51.07%   51.07%   +<.01%     
- Complexity    24830    24852      +22     
============================================
  Files          1594     1596       +2     
  Lines         94485    94554      +69     
  Branches       1365     1365              
============================================
+ Hits          48256    48298      +42     
- Misses        46229    46256      +27

Impacted Files	Coverage Δ	Complexity Δ
apps/dav/composer/composer/autoload_static.php	`0% <ø> (ø)`	`1 <0> (ø)`	⬇️
apps/dav/appinfo/v1/carddav.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
apps/dav/lib/Command/CreateCalendar.php	`46.66% <0%> (-3.34%)`	`4 <0> (ø)`
apps/dav/composer/composer/autoload_classmap.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
apps/dav/appinfo/v1/caldav.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
apps/dav/lib/CalDAV/Principal/Collection.php	`0% <0%> (ø)`	`1 <1> (?)`
apps/dav/lib/CalDAV/Principal/User.php	`0% <0%> (ø)`	`1 <1> (?)`
apps/dav/lib/CalDAV/CalDavBackend.php	`84.21% <100%> (ø)`	`243 <0> (-1)`	⬇️
apps/dav/lib/RootCollection.php	`90.41% <50%> (-2.45%)`	`1 <0> (ø)`
apps/dav/lib/Connector/Sabre/Principal.php	`90.82% <82.75%> (-7.51%)`	`46 <14> (+21)`
... and 4 more

georgehrke · 2017-11-03T20:15:20Z

Todo:

disable when sharing API is disabled
only allow groups when sharing API is restricted to groups
unit tests

georgehrke · 2017-12-07T12:23:45Z

apps/dav/lib/CalDAV/Principal/User.php

+		$acl = parent::getACL();
+		$acl[] = [
+			'privilege' => '{DAV:}read',
+			'principal' => '{DAV:}authenticated',


@LukasReschke @rullzer Is it safe to do this? Does this result in any kind of authentication bypass?

I think this is save to do. It just requests if the currently authenticated user has read access.

If you could somehow change the current authenticated user then we'd have bigger problems :P

LukasReschke · 2017-12-07T14:58:28Z

Requires: nextcloud/3rdparty#68

Is this still the case? :)

LukasReschke

Wasn't aware of using ... for unpacking on arrays but neat :)

LukasReschke · 2017-12-07T15:04:58Z

@georgehrke Can you fix the tests? :)

georgehrke · 2017-12-07T16:44:07Z

@georgehrke Can you fix the tests? :)

Yes, will fix the tomorrow. Just wanted to get feedback on the ACL changes for the user principal :)

Wasn't aware of using ... for unpacking on arrays but neat :)

Me neither, it's PHP 5.6+, so it's fine for 13

georgehrke · 2017-12-07T16:44:30Z

Requires: nextcloud/3rdparty#68

Is this still the case? :)

No

MorrisJobke · 2017-12-08T15:30:18Z

@georgehrke What about the unit tests? I would like to get this in or move it to 14.

georgehrke · 2017-12-09T20:50:17Z

There are ocs-api related integration tests failing. Can someone with more knowledge into our integration test suite look into them?

MorrisJobke · 2017-12-10T09:31:04Z

Those are the 4 failing assertions:

sharees_features/sharees.feature:41
    ...
    And "users" sharees returned is empty                                                # ShareesContext::thenListOfSharees()
      Failed asserting that an array is empty.
sharees_features/sharees.feature:75
    ...
    And "users" sharees returned is empty                                                # ShareesContext::thenListOfSharees()
      Failed asserting that an array is empty.
sharees_features/sharees_provisioningapiv2.feature:41
    ...
    And "users" sharees returned is empty                                                # ShareesContext::thenListOfSharees()
      Failed asserting that an array is empty.
sharees_features/sharees_provisioningapiv2.feature:75
    ...
    And "users" sharees returned is empty                                                # ShareesContext::thenListOfSharees()
      Failed asserting that an array is empty.

MorrisJobke · 2017-12-10T09:31:17Z

cc @rullzer @danxuliu

danxuliu · 2017-12-10T13:34:41Z

@MorrisJobke The failing integration tests are not related to this pull request; they fail in current master too (see, for example, https://drone.nextcloud.com/nextcloud/server/3305/219). The first offending commit seems to be ba648ee @rullzer

rullzer · 2017-12-11T14:49:48Z

Test will be fixed once #7342 is in

rullzer · 2017-12-11T15:00:32Z

Really awesome stuff here @go2sh !!

So just as a note this will allow on a 'standard' configured cloud to have all users see the free/busy schedule of all the others. I'm fine with that. But it might be something we need to communicate on release. (cc @MorrisJobke)

@georgehrke @go2sh would (for a followup PR). it be a lot of work to be able to set a calendar property to now have this exposed? As I can imagine use cases where users don't want this.

rullzer

Awesome stuff!

Add a "searchPrincipals" function to the NC principal backend. Fix the "findByUri" function to respect the prefixPath. Signed-off-by: Christoph Seitz <christoph.seitz@posteo.de>

Signed-off-by: Georg Ehrke <developer@georgehrke.com>

rullzer · 2017-12-11T15:05:24Z

Rebased to get a green CI

MorrisJobke · 2017-12-11T15:33:05Z

So just as a note this will allow on a 'standard' configured cloud to have all users see the free/busy schedule of all the others. I'm fine with that. But it might be something we need to communicate on release. (cc @MorrisJobke)

cc @jospoortvliet @karlitschek For this

georgehrke · 2017-12-11T15:48:37Z

In the standard configuration, yes. But if the ShareAPI is disabled, FreeBusy will also stop working and if the ShareAPI is restricted to group sharing only, you will only be able to query freeBusy reports for people from the same groups.

So that’s the expected behavior imho:)

rullzer · 2017-12-11T15:50:03Z

@georgehrke yep I agree

go2sh force-pushed the fix-freebusy branch from 3aa22d5 to 0c2d5c4 Compare October 2, 2017 13:21

georgehrke added the 2. developing Work in progress label Oct 22, 2017

georgehrke assigned go2sh and georgehrke Oct 22, 2017

georgehrke added this to the Nextcloud 13 milestone Oct 22, 2017

georgehrke force-pushed the fix-freebusy branch from 6244fc5 to b25eaaa Compare November 3, 2017 13:34

georgehrke requested a review from LukasReschke November 3, 2017 13:34

georgehrke removed the request for review from LukasReschke November 7, 2017 22:44

georgehrke added the bug label Nov 13, 2017

georgehrke force-pushed the fix-freebusy branch 3 times, most recently from 63a3e72 to 8a074a2 Compare December 4, 2017 16:09

georgehrke reviewed Dec 7, 2017

View reviewed changes

LukasReschke approved these changes Dec 7, 2017

View reviewed changes

MorrisJobke mentioned this pull request Dec 8, 2017

13.0.0 beta 2 #7425

Merged

28 tasks

georgehrke force-pushed the fix-freebusy branch 2 times, most recently from e032776 to f87d1dc Compare December 9, 2017 19:58

MorrisJobke mentioned this pull request Dec 10, 2017

Always generate avatar #6876

Merged

2 tasks

rullzer approved these changes Dec 11, 2017

View reviewed changes

go2sh and others added 4 commits December 11, 2017 16:04

Fix functions to search for principals in the backend.

787e3d5

Add a "searchPrincipals" function to the NC principal backend. Fix the "findByUri" function to respect the prefixPath. Signed-off-by: Christoph Seitz <christoph.seitz@posteo.de>

exclude shared calendars from freeBusy

f543039

Signed-off-by: Georg Ehrke <developer@georgehrke.com>

Principal search: Take sharing settings into account

6802e2b

Signed-off-by: Georg Ehrke <developer@georgehrke.com>

updated autoloaders for dav

728248b

Signed-off-by: Georg Ehrke <developer@georgehrke.com>

rullzer force-pushed the fix-freebusy branch from e3d26db to 728248b Compare December 11, 2017 15:05

blizzz added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Dec 11, 2017

MorrisJobke merged commit d0ec0ce into nextcloud:master Dec 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the FreeBusy request handling for proper scheduling support #6715

Fix the FreeBusy request handling for proper scheduling support #6715

go2sh commented Oct 1, 2017 •

edited by MorrisJobke

Loading

georgehrke commented Oct 1, 2017

georgehrke commented Oct 1, 2017

go2sh commented Oct 1, 2017 •

edited

Loading

go2sh commented Oct 2, 2017

georgehrke commented Oct 2, 2017

go2sh commented Oct 2, 2017 •

edited

Loading

georgehrke commented Oct 3, 2017 •

edited

Loading

georgehrke commented Oct 31, 2017

codecov bot commented Nov 3, 2017 •

edited

Loading

georgehrke commented Nov 3, 2017 •

edited

Loading

georgehrke Dec 7, 2017

rullzer Dec 11, 2017

LukasReschke commented Dec 7, 2017

LukasReschke left a comment

LukasReschke commented Dec 7, 2017

georgehrke commented Dec 7, 2017

georgehrke commented Dec 7, 2017

MorrisJobke commented Dec 8, 2017

georgehrke commented Dec 9, 2017

MorrisJobke commented Dec 10, 2017

MorrisJobke commented Dec 10, 2017

danxuliu commented Dec 10, 2017

rullzer commented Dec 11, 2017

rullzer commented Dec 11, 2017

rullzer left a comment

rullzer commented Dec 11, 2017

MorrisJobke commented Dec 11, 2017

georgehrke commented Dec 11, 2017

rullzer commented Dec 11, 2017

Fix the FreeBusy request handling for proper scheduling support #6715

Fix the FreeBusy request handling for proper scheduling support #6715

Conversation

go2sh commented Oct 1, 2017 • edited by MorrisJobke Loading

georgehrke commented Oct 1, 2017

georgehrke commented Oct 1, 2017

go2sh commented Oct 1, 2017 • edited Loading

go2sh commented Oct 2, 2017

georgehrke commented Oct 2, 2017

go2sh commented Oct 2, 2017 • edited Loading

georgehrke commented Oct 3, 2017 • edited Loading

georgehrke commented Oct 31, 2017

codecov bot commented Nov 3, 2017 • edited Loading

Codecov Report

georgehrke commented Nov 3, 2017 • edited Loading

georgehrke Dec 7, 2017

Choose a reason for hiding this comment

rullzer Dec 11, 2017

Choose a reason for hiding this comment

LukasReschke commented Dec 7, 2017

LukasReschke left a comment

Choose a reason for hiding this comment

LukasReschke commented Dec 7, 2017

georgehrke commented Dec 7, 2017

georgehrke commented Dec 7, 2017

MorrisJobke commented Dec 8, 2017

georgehrke commented Dec 9, 2017

MorrisJobke commented Dec 10, 2017

MorrisJobke commented Dec 10, 2017

danxuliu commented Dec 10, 2017

rullzer commented Dec 11, 2017

rullzer commented Dec 11, 2017

rullzer left a comment

Choose a reason for hiding this comment

rullzer commented Dec 11, 2017

MorrisJobke commented Dec 11, 2017

georgehrke commented Dec 11, 2017

rullzer commented Dec 11, 2017

go2sh commented Oct 1, 2017 •

edited by MorrisJobke

Loading

go2sh commented Oct 1, 2017 •

edited

Loading

go2sh commented Oct 2, 2017 •

edited

Loading

georgehrke commented Oct 3, 2017 •

edited

Loading

codecov bot commented Nov 3, 2017 •

edited

Loading

georgehrke commented Nov 3, 2017 •

edited

Loading