Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: CardDAV support #1284

Merged
merged 55 commits into from
Oct 29, 2018
Merged

feat: CardDAV support #1284

merged 55 commits into from
Oct 29, 2018

Conversation

kidk
Copy link

@kidk kidk commented May 6, 2018

WIP for #582

This is what I have after spending a couple of hours on it, the basic UI of Sabre works, but the two clients I've tested give me errors.

  • Evolution because of a misconfiguration in the client, so it sends an empty request size. Ref
  • IOS 12 still unknown. I've reverted to doing own authentication, instead of using the Laravel system, but that hasn't resolved the issue. Next up is looking at the technical details.

There is still a lot of debug code in there, but I wanted to already share this to see if people have comments or requests. I've added a TODO below to give you an idea on the work that is still needed.

  • Adding more Contacts testing data
  • First working client (Evolution and IOS)
  • Read only CardDAV access
  • Add remaining fields

Copy link
Member

@djaiss djaiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing. Really amazing. I could not have done this myself. Can't wait to see how it evolves.

\Debugbar::disable();

// TODO: Not sure if this is needed, check later
date_default_timezone_set('Europe/Berlin');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If anything, probably should be set to UTC?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

* Returns the list of addressbooks for a specific user.
*
* Every addressbook should have the following properties:
* id - an arbitrary unique id
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use our HashID here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try, need to see if we can use non-numeric values.

* This method should work identical to getCard, but instead return all the
* cards in the list as an array.
*
* If the backend supports this, it may allow for some speed-ups.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why wouldn't it?

*/
function check(\Sabre\HTTP\RequestInterface $request, \Sabre\HTTP\ResponseInterface $response) {
Log::debug(__CLASS__.' check', func_get_args());
if (Auth::check() || Auth::attempt(['email' => $request->getRawServerValue('PHP_AUTH_USER'), 'password' => $request->getRawServerValue('PHP_AUTH_PW')])) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be easier to put Auth::attempt(['email' => $request->getRawServerValue('PHP_AUTH_USER'), 'password' => $request->getRawServerValue('PHP_AUTH_PW')]) in a variable so this line is more readable.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I might revert back to using https://laravel.com/docs/5.6/authentication#stateless-http-basic-authentication I prefer using the framework systems whenever possible, but I need to figure out the issue with sync first.

@kidk
Copy link
Author

kidk commented May 11, 2018

It currently works for IOS and Evolution. This means the basic building blocks for CardDAV are in place and all that remains is polishing. This means adding other fields, data, and setting up the ReadOnly permissions.

@djaiss
Copy link
Member

djaiss commented May 14, 2018

@kidk awesome! Let me know when you need a review!

@cweagans
Copy link

I'm super excited for this to land. Having a contacts database that's shared with my wife + a nice UI + a way to record extra notes and stuff is gonna be amazing.

Copy link

@sonarcloud sonarcloud bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SonarQube analysis found issues:
Bug Bugs: 0
Vulnerability Vulnerabilities: 0
Code Smell Code Smells: 1

See all issues in SonarCloud

@asbiin
Copy link
Member

asbiin commented Oct 28, 2018

Thank you so much @kidk for your help!!
I will merge this soon. I think we should improve this little by little instead of one big PR.
This is BETA feature, not enabled by default: you'll have to enable it with CARDDAV_ENABLED=true.
For now it's only readonly, which is very nice!

@asbiin asbiin changed the title [WIP] CalDAV / CardDAV feat: CardDAV support Oct 28, 2018
@kidk
Copy link
Author

kidk commented Oct 28, 2018

No problem, just glad to help on such an amazing project. Thanks for cleaning up the code.

I have some time the next couple of weeks to test and improve the code where needed. So feel free to tag me on any bugs or feature requests.

CHANGELOG Outdated Show resolved Hide resolved
// Disable debugger for caldav output
Debugbar::disable();

// Initiate custom backends for link between Sabra and Monica
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Initiate custom backends for link between Sabra and Monica
// Initiate custom backends for link between Sabre and Monica

}

/**
* Returns a specific principal, specified by it's path.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Returns a specific principal, specified by it's path.
* Returns a specific principal, specified by its path.

CHANGELOG Outdated
@@ -18,6 +18,7 @@ UNRELEASED CHANGES:
* API breaking change: Remove 'PUT /contacts/:contact_id/pets/:id' in favor of 'PUT /pets/:id' with a 'contact_id'
* API breaking change: Every validator fails now send a HTTP 400 code (was 200) with the error 32
* API breaking change: Every Invald Parameters errors now send a HTTP 400 (was 500 or 200) code with the error 41
* Add CardDAV support (readonly)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Add CardDAV support (readonly)
* Add CardDAV support (readonly - disabled by default). To enable it, toggle the `CARDDAV_ENABLED` env variable.

@asbiin asbiin merged commit c7eea2b into monicahq:master Oct 29, 2018
@pqhf5kd pqhf5kd mentioned this pull request Jan 12, 2019
@dosch
Copy link

dosch commented May 13, 2019

I am unable to see the options to sync over CardDav. Is there already some documentation, or where can find this? Thnx!

@asbiin
Copy link
Member

asbiin commented May 14, 2019

@dosch The feature is not yet available on app.monicahq.com, we have to validate it yet, but it will be activated soon.

@asbiin
Copy link
Member

asbiin commented Sep 16, 2019

Hi folks! Carddav is now activated on all account on app.monicahq.com!
Let me now if it's working ... or not.

@dosch
Copy link

dosch commented Oct 1, 2019

I (finally) added the Monica Carddav to my Contacts.app in MacOS.
But the contacts I already have in Monica do not appear in the Contacts.app and new contacts I add to the app do not appear in Monica.

Am I missing something?

@github-actions
Copy link

This pull request has been automatically locked since there
has not been any recent activity after it was closed.
Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants