search

signalapp / Flock

Private contact and calendar sync for Android.
https://signal.org/blog/flock
357 stars 80 forks source link

Unable to login to ownCloud #19

Open gellenburg opened 10 years ago

gellenburg commented 10 years ago

Update: 2014-07-25 - This affects ownCloud 7.0.0-stable as well.

Flock 0.51 is unable to login/ connect to ownCloud 6.0.4 stable.

HTTP Access Logs reveal the following connection attempts:

We see the Flock client query ownCloud for the well-known URI of ownCloud's CardDAV server, and receive an HTTP/302 redirect to /remote.php/carddav/.

We know that ownCloud is working properly in this instance as we can test the response with curl:

XXXXX:~ XXXXX$ curl --request PROPFIND https://XXXXX.XXXXX.XXXXX/.well-known/carddav <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">

302 Found

Found

The document has moved here\.

XXXXX:~ XXXXX$

Yet Flock is attempting to append yet another well-known-URI request operation to the end of the returned URI from the first operation.

heikoengel commented 10 years ago

Haven't fully understood what's happening here, but CardDavStore::getCurrentUserPrincipal() is checking for SC_MOVED_PERMANENTLY, where OwnCloud is responding with SC_MOVED_TEMPORARILY. When adding SC_MOVED_TEMPORARILY to flock/src/main/java/org/anhonesteffort/flock/webdav/carddav/CardDavStore.java#101, the request is handed over to AbstractDavComponentStore::getCurrentUserPrincipal(uri) with the new location. However propFindMethod.getResponseBodyAsMultiStatus() in flock/src/main/java/org/anhonesteffort/flock/webdav/AbstractDavComponentStore.java#L122 still throws an exception with code 302, whereas the command line query with curl on the new location seems to return the expected multistatus:

$ curl --request PROPFIND --user username:passwd https://OWNCLOUD_SERVER/remote.php/carddav/
<?xml version="1.0" encoding="utf-8"?>
<d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:card="urn:ietf:params:xml:ns:carddav"><d:response><d:href>/remote.php/carddav/</d:href><d:propstat><d:prop><d:getlastmodified>Thu, 24 Jul 2014 10:37:49 GMT</d:getlastmodified><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/carddav/principals/</d:href><d:propstat><d:prop><d:getlastmodified>Thu, 24 Jul 2014 10:37:49 GMT</d:getlastmodified><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/carddav/addressbooks/</d:href><d:propstat><d:prop><d:getlastmodified>Thu, 24 Jul 2014 10:37:49 GMT</d:getlastmodified><d:resourcetype><d:collection/></d:resourcetype></d:prop><d:status>HTTP/1.1 200 OK</d:status></d:propstat></d:response></d:multistatus>
rhodey commented 10 years ago

@gellenburg @rndhro

I just installed owncloud 7 in a lighttpd configuration and added the following rules to my /etc/lighttpd/lighttpd.conf file

url.redirect = (
  "^/.well-known/carddav" => "/owncloud/remote.php/carddav",
  "^/.well-known/caldav" => "/owncloud/remote.php/caldav",
)

This get's me all the way to half-way through the "handleCalDavTestCreateEditCollectionProperties()" test. Here I get an error from SabreDAV complaining about the "org.anhonesteffort.flock" namespace. Switching the namespace to "http://org.anhonesteffort.flock/ns" resolves this issue.

Unfortunately then I run into the error I was experiencing months ago where OwnCloud only retains a subset of calendar properties and if you try to use a custom property (ie X-CUSTOM-PROP-NAME) it silently fails to set the property and when you check for it next tells you it is not there. This is not in spec with WebDAV and should be resolved in the OwnCloud codebase.

Until OwnCloud starts maintaining collection properties correctly there isn't anything I can do to make the Flock client play nicely with OwnCloud. I will however patch Flock to use the "http://org.anhonesteffort.flock/ns" namespace so that we can get closer to OwnCloud friendly.

heikoengel commented 10 years ago

@rhodey thanks for looking into this! Closing ticket #21 made me think the ownloud side was already fixed. I stubled upon this conversation: http://owncloud.10557.n7.nabble.com/Impressionist-App-for-ownCloud-td5688.html This thread is already 2 years old, but the users seems to have similar issues and could resolv them. Namespace adjustments are also mentioned - is this maybe related?

rhodey commented 10 years ago

@rndhro sorry for the confusion. I'm positive this is an issue with the OwnCloud server and not the Flock client, until someone patches OwnCloud Flock just isn't going to pass the server tests.

Here is the DB structure for calendars in OwnCloud:

CREATE TABLE calendars (
    id SERIAL NOT NULL,
    principaluri VARCHAR(100),
    displayname VARCHAR(100),
    uri VARCHAR(200),
    ctag INTEGER NOT NULL DEFAULT 0,
    description TEXT,
    calendarorder INTEGER NOT NULL DEFAULT 0,
    calendarcolor VARCHAR(10),
    timezone TEXT,
    components VARCHAR(20),
    transparent SMALLINT NOT NULL DEFAULT '0'
);

The issue here is that the WebDAV spec requires that arbitrary properties can be set on WebDAV collections (in CalDAV calendars are modelled as WebDAV collections) while the OwnCloud database structure only allows for a static set of properties, ie "displayname", "description", "calendarorder", etc.

To properly conform to WebDAV, OwnCloud's calendar database structure should be more like...

CREATE TABLE calendars (
    id SERIAL NOT NULL,
    principaluri VARCHAR(100),
    uri VARCHAR(200),
    components VARCHAR(20),
    transparent SMALLINT NOT NULL DEFAULT '0'
);

CREATE TABLE calendar_properties (
    id_calendar SERIAL,
    property_name VARCHAR(1024) NOT NULL,
    property_value VARCHAR(1024) NOT NULL,
    FOREIGN KEY (id_calendar) REFERENCES calendars(id)
);

This would still allow for the "displayname", "description", "calendarcolor", etc properties but would also allow for arbitrary properties to be defined on calendars. I'd be psyched to see something like this patched into OwnCloud but I guess if that doesn't happen for another couple months maybe I'll have to write the patch myself :(

rhodey commented 10 years ago

even though this is not an issue with the Flock client I've decided to keep this issue open for now under the label "wontfix" for purposes of documentation.

devurandom commented 10 years ago

Is there an ownCloud Calendar issue for this?

jaromil commented 10 years ago

just did https://github.com/owncloud/calendar/issues/569

evert commented 9 years ago

Supporting storage of arbitrary properties is not something that is explicitly demanded by the specification. The WebDAV framework that CalDAV builds upon also does not specifically require it, and many servers don't allow it. It's a bit presumptuous to assume that everyone else is wrong.

Many clients do store custom properties on caldav servers, but I haven't seen a single client that does not have adequate fallback behavior. iCal specifically assumes that the server may reject one or more properties in PROPPATCH requests, and therefore actually does 1 PROPPATCH per property, so that if one property is accepted by the server, and others aren't, it doesn't block the entire operation.

However:

  1. This is just talking about WebDAV properties, retention of custom iCalendar properties in calendar objects should be supported. And if a server drops X- properties, it should be treated as a violation of the spec.
  2. Since sabredav 2.0 there is now a facility that does allow defining arbitrary webdav properties on any resource. Documentation. Neither owncloud nor baikal support sabredav 2.0 yet though, as far as I can tell they are both still on 1.8.

Note that the property storage system only allows storing simple string-based properties at the moment. Storing complex xml structures in properties is something we'll work on in the future.

rhodey commented 9 years ago

Thanks for the background @evert

I will admit that I have been very surprised by the lack of support for custom WebDAV collection properties, retrospectively it makes sense that I should have done some more research on existing WebDAV server implementations. Section 9.2 of RFC 4918 states that "DAV-compliant resources SHOULD support the setting of arbitrary dead properties" so you are correct in stating that it is not demanded.

Unfortunately there really isn't any acceptable fallback behavior that Flock can take when arbitrary collection properties are not supported because 4 new "arbitrary dead properties" are defined and required, namely:

  1. X-FLOCK-COLLECTION - collections without this property are ignored by Flock.
  2. X-FLOCK-HIDDEN-CALENDAR-COLOR - calendar color is traditionally defined as an integer, this property allows Flock to store color as base64 encoded ciphertext.
  3. X-KEY-MATERIAL-SALT - defined on the "key collection", the salt for all cryptographic opertations.
  4. X-ENCRYPTED-KEY-MATERIAL - defined on the "key collection", encrypted key material used to bootstrap new clients.

There are some things that could be done to hack these properties into webdav servers that reject arbitrary dead properties, eg packing them into a special-case .ical or .vcard resource and storing that within the collection-- but IMO this is a really ugly hack and I'd rather WebDAV servers be patched to support the recommended behavior.

There is a really and dirty SabreDAV (and thus ownCloud) patch that would make Flock happy:

ALTER TABLE calendars ADD COLUMN x_flock_collection INTEGER;
ALTER TABLE calendars ADD COLUMN x_hidden_calendar_color VARCHAR(1024);
ALTER TABLE calendars ADD COLUMN x_key_material_salt VARCHAR(1024);
ALTER TABLE calendars ADD COLUMN x_encrypted_key_material VARCHAR(1024);

The default for all of these new columns would be null so this alone wouldn't break or affect the server. All that would be needed after this is to patch whatever code applies the displayname, description, etc. properties to include these 4 new props. I might end up working on this today but would prefer that arbitrary collection properties be supported instead of just these 4 because I would like the option of defining more properties in the future.

Quick note on iCal: the response to a PROPPATCH is multi-status meaning you can update multiple properties in a single PROPPATCH and then check the response to see which succeeded, each property has its own http status result code. Flock checks each response code within the multi-status but still fails because it can't do without these 4.

evert commented 9 years ago

Hi!

The properties you are talking about, sound more like iCalendar properties as opposed to WebDAV properties to me. Is there some confusion going on here?

You uppercase them and prefix them with X- which is typical for embedding custom data in iCalendar, and CalDAV servers definitely should retain these.

rhodey commented 9 years ago

@evert If I had the chance to rename these properties I would have dropped the X- prefix :) it is in-fact due to a prior misunderstanding of property naming conventions. However, these properties are being set on WebDAV collections (calendars in CalDAV) and not resources (.ical files in CalDAV).

There are additional properties which Flock sets on WebDAV resources (.vcard & .ical files) but thankfully there are no issues with these:

If I had the chance to drop the X- prefix in the new Flock WebDAV properties I would also take the chance to drop X-FLOCK-HIDDEN-CALENDAR and use X-FLOCK-HIDDEN in both VCard and iCal to store ciphertext.

evert commented 9 years ago

Makes sense then! Unfortunately my earlier comment does stand... We added support for storing any arbitrary properties, but the wait is for Baikal and Owncloud to upgrade to SabreDAV 2 and add the plugin:

http://sabre.io/dav/property-storage

Crossing fingers that they will for their next releases :)

rhodey commented 9 years ago

@evert I remember testing Flock with ownCloud many months ago, running into this arbitrary property problem, and thinking "damn, well I've still got a couple months till release-- hope they fix by then!" haha so yeah, looks like unfortunately no relief presently.

Thanks for your work on SabreDAV :) property-storage looks great!

jaromil commented 9 years ago

Good work on this guys, thanks for following up. One more reason to get property-storage into SabreDAV 2: it will fix so many webcalendars! not just Owncloud, but also Roundcube for instance. This is nailing down a long term problem that prevented many people around the world from using their own synced calendar on their own servers.

timeggleston commented 9 years ago

I've created a bounty for this over at OwnCloud:

https://www.bountysource.com/issues/4482683-support-arbitrary-properties-conforming-to-caldav-spec

Although I suppose that may be a little optimistic if they need to switch out their SabreDAV version...

rhodey commented 9 years ago

@timeggleston thanks for organizing this :) I'll throw into the bounty as soon as I get an account setup.

untitaker commented 9 years ago

@rhodey Fallback behavior that comes to mind: You could create a special resource with a very specific, not randomly generated href. I.e. for CalDAV you upload a resource called flock-properties.ics, which could contain a valid VCALENDAR, but also any data you wanted to store inside a collection property.

EDIT: nvm didn't read that you already thought about this

monolithonadmin commented 9 years ago

What about owncloud 8?

untitaker commented 9 years ago

@Muvuk ownCloud uses SabreDAV 1.8, support for arbitrary props was introduced in 2.0.

devurandom commented 9 years ago

@untitaker So when does OC plan to update to SabreDAV 2?

untitaker commented 9 years ago

@devurandom I don't know, I am not affiliated with them.

joeykrim commented 9 years ago

Latest I could find on ownCloud and SabreDev 2.0 is from a blog post on the ownCloud blog on April 14th 2015: "Vincent will be part of the work to update sabre/dav in ownCloud to version 2. It requires some “cleanup and re-factoring,” but stands to make “our code more beautiful.” The upgrade will be preceded by “adding unit tests everywhere to make sure the upgrade is smooth.”"

scroom commented 9 years ago

Does OC 8.1 solve the issue?

ichEben commented 9 years ago

I'm using OC 8.1 and the problem still exists