Rewriting to use ldap3 #13
Conversation
Currently blocked because MOCK_ASYNC strategy seems to have todo's in ldap3
|
Note that opened this just for reference. Even when tests pass, still needs testing against a real server |
|
Yeah, better communication than work 1m in own repo and then push gigantit pull request ;) To me it looks good, i bit hesitate about the connection hook (or should one use rather unittest.mock) but as that would be the only mock i guess current method (that might be usefull also in real life) is fine. About the testing; i have lost the real server test enviroment. Combined with lack of time it pretty much means that i am unable to do any real testing. If you have knowledge of a) open ldap servers that one could use for testing or b) good virtual machine images 'ready to serve' please let me know. |
|
In general yeah, good call. I'll try to work a bit to make backend not have anything test-related but instead override/mock ldap_open_connection and add test context through that. That particular unittest.mock doesn't exist in Python2 but I get the idea I'm tbh not worrying too much about real servers at this point. I think there was some address that looked like a real one in ldap3 documentation but didn't test. Btw, one option to avoid waiting for MOCK_ASYNC is to rewrite code so it works with sync ones. Difference is basically queries block and you get result in connection.result directly instead of having to check with result id |
|
I think I largely got the tests working. The only test that fails is Python3.4 on Django 1.4 which apparently is not a supported combination. 1.5 is the oldest Django that has Python3 support |
|
https://www.djangoproject.com/weblog/2015/jun/25/roadmap/ that said, both 1.4 and 1.8 are EOL already |
django_auth_ldap_ad/backend.py
Outdated
| else: # end up here with mock | ||
| ldap_connection = self.ldap_connection | ||
| server_urls = self.ldap_settings.SERVER_URI | ||
| self.servers = [] |
There was a problem hiding this comment.
Style: Extra variable?
EDIT: line 27, i did not see the linenumber here on this thread.
django_auth_ldap_ad/backend.py
Outdated
| if not hasattr(self, "ldap_settings"): | ||
| self.ldap_settings = LDAPSettings() | ||
| def __init__(self): | ||
| self.connection = Connection |
There was a problem hiding this comment.
Better than before, nice! Extra comment reasoning why would not hurt.
EDIT: line 19, same reasoning as above
|
Sounds good, looks good, nice work. I added few silly comments when glancing through the code. Have tested on any real server? Do you have any available (for yourself - i guess since you had motivation to do this:) ? Dropping django versions before 1.8 is ok imho, since the release 1.0 can serve those needs anyway. |
|
@susundberg Actually no, I don't have a real server available currently. Will need to figure something out soon |
|
Hi again, when you find time and server to tests, could you write down the test setup as much as you can with ease; at least python version, django version and ldap-ad server software/version. But yeah, no rush, i just wanted to write this in hope of i did it before you did the testing :) Cheers, |
|
Any progress on this issue? Anyone else have servers & time to check the pull request functionality on real server? I am not going to merge this before some indication "it works with real server" appears . |
|
Sorry, not yet. I've been overwhelmed with work recently and haven't had a chance to investigate free LDAP sandboxes |
|
Hi, no worries. I was just eager to check if it would be "oh, yeah, i have been using it for weeks and its fine, but i have been busy" - situation, but it seems like i had only half guessed correctly :) Cheers, |
Rewriting codebase to use ldap3 instead of python-ldap. Some old options for temporarily removed until replacements can be figured out.