Fix the FreeBusy request handling for proper scheduling support by go2sh · Pull Request #6715 · nextcloud/server

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

+ $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 0c2d5c4Compare 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 b25eaaaCompare 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 8a074a2Compare 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 f87d1dcCompare 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

go2shand 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 728248bCompare 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:masterDec 11, 2017

Uh oh!

Uh oh!

Conversation

go2sh commented Oct 1, 2017• edited by MorrisJobke Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

georgehrke commented Oct 1, 2017

Uh oh!

georgehrke commented Oct 1, 2017

Uh oh!

go2sh commented Oct 1, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

go2sh commented Oct 2, 2017

Uh oh!

georgehrke commented Oct 2, 2017

Uh oh!

go2sh commented Oct 2, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

georgehrke commented Oct 3, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

georgehrke commented Oct 31, 2017

Uh oh!

codecovBot commented Nov 3, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

georgehrke commented Nov 3, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

georgehrkeDec 7, 2017

Choose a reason for hiding this comment

Uh oh!

rullzerDec 11, 2017

Choose a reason for hiding this comment

Uh oh!

LukasReschke commented Dec 7, 2017

Uh oh!

LukasReschke left a comment

Choose a reason for hiding this comment

Uh oh!

LukasReschke commented Dec 7, 2017

Uh oh!

georgehrke commented Dec 7, 2017

Uh oh!

georgehrke commented Dec 7, 2017

Uh oh!

MorrisJobke commented Dec 8, 2017

Uh oh!

georgehrke commented Dec 9, 2017

Uh oh!

MorrisJobke commented Dec 10, 2017

Uh oh!

MorrisJobke commented Dec 10, 2017

Uh oh!

danxuliu commented Dec 10, 2017

Uh oh!

rullzer commented Dec 11, 2017

Uh oh!

rullzer commented Dec 11, 2017

Uh oh!

rullzer left a comment

Choose a reason for hiding this comment

Uh oh!

rullzer commented Dec 11, 2017

Uh oh!

MorrisJobke commented Dec 11, 2017

Uh oh!

georgehrke commented Dec 11, 2017

Uh oh!

rullzer commented Dec 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

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

codecovBot commented Nov 3, 2017•
edited
Loading

georgehrke commented Nov 3, 2017•
edited
Loading