Add ldap storage #500

paulfariello · 2016-09-13T14:21:43Z

Current implementation is read only.
Not sure if ldap storage is supposed to do vcard conversion.

I haven't added tests nor documentation but will if @pimutils thinks this patch is meaningful.

Other enhancement would be to allow for attributes configuration.

edit by @untitaker: Fix #283

Current implementation is read only. Not sure if ldap storage is supposed to do vcard conversion.

untitaker · 2016-09-13T14:55:18Z

Cool! This looks reasonable, but:

for VCard conversion please use https://github.com/eventable/vobject instead of your own thing, which actually has some bugs with values containing backslashes or newlines. This will also make it possible to parse VCards for uploading.
Please specifiy ldap and vobject as dependencies here: https://github.com/pimutils/vdirsyncer/blob/master/setup.py#L69 For example, if I run pip install vdirsyncer[remotestorage], requests-oauthlib will be installed. ldap and vobject should be installed when I run pip install vdirsyncer[ldap].

untitaker · 2016-09-13T14:56:09Z

vdirsyncer/storage/ldap.py

+
+class LDAPStorage(Storage):
+
+    __doc__ = '''


When implementing upload and update, we have to make it very clear which properties are even parsed and which ones are discarded.

Also the __doc__ is only necessary if we need to include variables from other strings.

untitaker · 2016-09-13T14:56:57Z

And yes, please add tests for this, this is very meaningful! Do you have an idea how to do this? I.e., is there a simple ldap server we could use?

paulfariello · 2016-09-13T16:01:43Z

Thanks for the quick reply.

I'm now using vobject which is clearly cleaner. Thanks for the hint.
I've also added support for title and fax.

I changed the default filter to one that make sense since vobject
clearly states that F and FN are mandatory fields. On some dirty
ldap directory sn or givenName could be missing and I'm not sure how
to
handle it cleanly without filtering it. Maybe just provide the available
family or givenname for N.

About tests, I'm not sure how we could test it against a real ldap
server. I don't know any open ldap server but I can check.

Note that the ldap attributes I'm using are from Active Directory. Any
other ldap directory could also use it but this is not guaranteed. The
most problematic attribute is whenChanged. I've never seen it on other
classic ldap directory and it's used for etag.

Paul Fariello

PGP: 0x672CDD2031AAF49B

untitaker · 2016-09-13T16:02:19Z

It appears that this would be appropriate for testing: https://pypi.python.org/pypi/mockldap

See tests/storage/test_filesystem.py for how to write tests for the LDAP backend. In particular if you subclass StorageTests your storage will already be thoroughly tested, but get_storage_args has to be implemented.

EDIT: However, this would require support of update and upload.

untitaker · 2016-09-13T16:06:58Z

Note that the ldap attributes I'm using are from Active Directory. Any other ldap directory could also use it but this is not guaranteed. The most problematic attribute is whenChanged. I've never seen it on other classic ldap directory and it's used for etag.

Yeah I think we should make as much as possible configurable there, and also provide examples for other major LDAP providers.

untitaker · 2016-09-13T16:07:58Z

Regarding whenChanged, if it's not there, you'll probably have to download the item and use item.hash or something similar.

untitaker · 2016-09-13T18:36:34Z

vdirsyncer/storage/ldap.py

+        super(LDAPStorage, self).__init__(**kwargs)
+        self.search_base = search_base
+        self.filter = filter
+        self.server = ldap3.Server(uri)


I think you could just use ldap.initialize here, no?

Ah, I see, you're using https://github.com/cannatag/ldap3, but I was reading the docs for https://github.com/pyldap/pyldap

Could you switch to the latter? This would make testing easier as mockldap appears to support pyldap only.

untitaker · 2016-09-13T18:40:36Z

Am I correct to assume that this storage type does not support multiple addressbooks?

untitaker · 2016-09-13T18:43:42Z

vdirsyncer/storage/ldap.py

+
+class LDAPStorage(Storage):
+    '''
+    :param uri: LDAP URI


Why URI instead of URL?

URI is the right term for this since it can be an URL, a simple ip or a
hostname.

Paul Fariello

PGP: 0x672CDD2031AAF49B

An IP or hostname is not a URI. URIs always have a scheme. https://tools.ietf.org/html/rfc3986#section-1.1.1

untitaker · 2016-09-13T19:04:19Z

I added a LDAP testcase that should work if you use pyldap instead of ldap3.

paulfariello · 2016-09-14T07:23:24Z

Thanks for the review. I'll fix today.

But first I have two questions:

how am I supposed to run the test suites ? I'm running make -e test but got a bunch of errors
how did you managed to push a commit on my fork ?

untitaker · 2016-09-14T09:03:35Z

You need to run "make install-test" as well. In the documentation there is a page "Contributing" that goes into details.

Regarding how my commit landed on your fork, see https://github.com/blog/2247-improving-collaboration-with-forks

On 14 September 2016 09:23:24 CEST, Paul Fariello [email protected] wrote:

Thanks for the review. I'll fix today.

But first I have two questions:

how am I supposed to run the test suites ? I'm running make -e test
but got a bunch of errors

how did you managed to push a commit on my fork ?

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#500 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

paulfariello · 2016-09-14T13:57:37Z

I've been trying to switch to pyldap but it doesn't natively support generator for ldap result. It makes it more complicated to cleanly integrate it.
Meanwhile I just discovered that ldap3 has a mocking component. I'll try to use it instead.

paulfariello · 2016-09-14T16:52:02Z

Tests with ldap3 mocking are still WIP.

untitaker · 2016-09-14T20:42:21Z

Ah, you need to add the new dependencies to test-requirements.txt as well.

untitaker · 2016-09-29T15:38:19Z

@paulfariello BTW let me know if I can help with anything or if anything is unclear.

untitaker · 2016-10-08T12:24:03Z

@paulfariello Any update on this? Adding tests is really important! Are you using this already for yourself?

paulfariello · 2016-10-08T19:01:33Z

Sorry I've been quite busy lately. I'll try to add tests by the end of next week.

Le 8 octobre 2016 14:24:04 GMT+02:00, Markus Unterwaditzer [email protected] a écrit :

@paulfariello Any update on this? Adding tests is really important! Are
you using this already for yourself?

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#500 (comment)

Paul Fariello

PGP: 0x672CDD2031AAF49B

paulfariello · 2016-10-18T20:35:11Z

I tried to add upload and update method. But I'm facing an issue with VCALENDAR. What should I do with it ? They doesn't really belong to an LDAP directory.

untitaker · 2016-10-18T20:42:04Z

Fantastic, please push what you have, I'll do the changes such that VCALENDARs are not used in the tests.

paulfariello · 2016-10-19T20:49:00Z

My last push is just a first attempt on update and upload implementation. It push to mocked server.
I also have an issue with start tls on mocked server.

untitaker · 2016-10-19T21:27:59Z

Are ldap://-urls SSL by default?

untitaker · 2016-10-19T21:42:01Z

I believe I've fixed the issue with ssl, let's just expect that the connection is fully set up if it's passed into the storage. I think ldap doesn't deal with SSL very well when mocked, but in our case SSL is not of any use anyway.

untitaker

Just got around to reviewing all of the LDAP storage. Unfortunately upload and update still need a lot of work.

untitaker · 2016-10-22T14:56:11Z

vdirsyncer/storage/ldap.py

+
+    def upload(self, item):
+        vcard = vobject.readOne(item.raw)
+        self.conn.strategy.add_entry('cn={},ou=test,o=lab'.format(vcard.fn),


Upload needs to return the assigned href and etag. It appears that your href is dn, but the etag has to be fetched like in list and get.

Also add_entry returns a boolean that indicates whether the entry has been added, you should check for that.

untitaker · 2016-10-22T14:56:50Z

vdirsyncer/storage/ldap.py

+            raise exceptions.NotFoundError(href)
+
+        entry = self.conn.entries[0]
+        etag = str(entry.whenChanged)


Here you have no fallback from whenChanged to item.hash like you have in list. I suggest a separate helper function (outside of the storage class) for this.

untitaker · 2016-10-22T14:57:56Z

vdirsyncer/storage/ldap.py

+        vo = vcard.add('n')
+        vo.value = vobject.vcard.Name(family=str(entry.sn),
+                                      given=str(entry.givenName))
+        if getattr(entry, 'telephoneNumber', None):


This conversion from entry to item could be its own function (outside of storage class)

untitaker · 2016-10-22T15:01:15Z

vdirsyncer/storage/ldap.py

+
+    def update(self, href, item, etag):
+        vcard = vobject.readOne(item.raw)
+        self.conn.strategy.add_entry(href, vcard)


add_entry will do nothing if the item already exists, but if I call update, the item always exists. update is also expected to fail if the passed etag does not match the etag of the item on the server.

I think modify or how it's called is more appropriate than add_entry, but I'm not sure since I couldn't find much docs :(

untitaker · 2016-10-22T15:04:05Z

Also please contact me if there are any questions at all, PM, direct email, whatever too.

untitaker · 2017-01-20T21:10:30Z

@paulfariello Ping!

untitaker · 2017-06-20T13:55:15Z

Closing this due to inactivity, but for reference, this is a good start for a successor PR.

Paul Fariello added 2 commits September 13, 2016 16:17

Add ldap storage

413c0f7

Current implementation is read only. Not sure if ldap storage is supposed to do vcard conversion.

pep8 conformance

69698da

untitaker reviewed Sep 13, 2016
View reviewed changes

untitaker mentioned this pull request Sep 13, 2016

LDAP support #283

Open

Paul Fariello added 3 commits September 13, 2016 17:44

Use vobject for vcard conversion

3b0c4a6

Add ldap3 and vobject dependencies

55e5ff6

Remove superfluous __doc__ attribute

0959a16

untitaker reviewed Sep 13, 2016
View reviewed changes

Add simple LDAP test

df3b6ff

untitaker force-pushed the master branch from de5683a to df3b6ff Compare September 13, 2016 19:04

Paul Fariello added 4 commits September 14, 2016 18:12

Use ldap3 internal mocking for tests

3a08a00

Fallback on item hash if whenChanged is not available

400ba90

Add default value for ldap uri

9299f11

Add default value for base dn

88cc171

untitaker force-pushed the master branch 3 times, most recently from 3e375a1 to 6aedd62 Compare September 26, 2016 20:38

untitaker force-pushed the master branch from 9d690b2 to fe3f8ea Compare October 3, 2016 17:26

Rename ldap URI into URL

51ec702

Add update and upload method

626dcf0

paulfariello force-pushed the master branch from bf11cf6 to 626dcf0 Compare October 19, 2016 20:48

ldap: Hide test args from docs, fix testing

52eb20e

untitaker force-pushed the master branch from dc2d8ff to 52eb20e Compare October 19, 2016 21:39

untitaker added 3 commits October 19, 2016 23:43

Only test vcards

f4e5f04

Fix test requirements

3a5a5d6

Stylefixes

2ff778b

untitaker requested changes Oct 22, 2016

View reviewed changes

untitaker mentioned this pull request Dec 29, 2016

Support for other single-file-based local storage formats #531

Closed

untitaker force-pushed the master branch from 3be94eb to c87848f Compare February 11, 2017 19:25

untitaker added the stalled label Feb 18, 2017

untitaker closed this Jun 20, 2017

untitaker removed the stalled label Jun 20, 2017

nblock mentioned this pull request Apr 11, 2023

Add ldap storage #1061

Draft

11 tasks

Add ldap storage #500

Add ldap storage #500

Conversation

paulfariello commented Sep 13, 2016 • edited by untitaker Loading

untitaker commented Sep 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untitaker commented Sep 13, 2016

paulfariello commented Sep 13, 2016

untitaker commented Sep 13, 2016 • edited Loading

untitaker commented Sep 13, 2016

untitaker commented Sep 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untitaker commented Sep 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untitaker commented Sep 13, 2016

paulfariello commented Sep 14, 2016

untitaker commented Sep 14, 2016

paulfariello commented Sep 14, 2016

paulfariello commented Sep 14, 2016

untitaker commented Sep 14, 2016

untitaker commented Sep 29, 2016

untitaker commented Oct 8, 2016

paulfariello commented Oct 8, 2016

paulfariello commented Oct 18, 2016

untitaker commented Oct 18, 2016

paulfariello commented Oct 19, 2016

untitaker commented Oct 19, 2016

untitaker commented Oct 19, 2016

untitaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untitaker Oct 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untitaker commented Oct 22, 2016

untitaker commented Jan 20, 2017

untitaker commented Jun 20, 2017

paulfariello commented Sep 13, 2016 •

edited by untitaker

Loading

untitaker commented Sep 13, 2016 •

edited

Loading

untitaker Oct 22, 2016 •

edited

Loading