Add LDAP authentication #701
Conversation
|
Can one of the admins verify this patch? |
3 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
@matrixbot ok to test (Sorry about the matrixbot spam) |
|
Oh, I'll need to add the new dependency manually to get the tests to run. |
|
Although see: http://matrix.org/jenkins/job/SynapseFlake8Packaging/176/violations/file/synapse/handlers/auth.py/ for some code style violations :) |
| raise LoginError(403, "", errcode=Codes.FORBIDDEN) | ||
| @defer.inlineCallbacks | ||
| def _check_password(self, user_id, password): | ||
| defer.returnValue(not ((yield self._check_ldap_password(user_id, password)) or (yield self._check_local_password(user_id, password)))) |
There was a problem hiding this comment.
This is a bit hard to follow. Maybe something like the following is clearer?
if self.ldap_enabled:
valid_ldap = yield self._check_ldap_password(user_id, password))
if valid_ldap:
defer.returnValue(True)
valid_password = yield self._check_local_password(user_id, password)
defer.returnValue(valid_password)This way we don't attempt (and log) if we've tried to do LDAP each time someone logs in to a HS with LDAP disabled.
There was a problem hiding this comment.
the first line of _check_ldap_password bails immediately if ldap_enabled is false. I can put the log level for the message on debug.
I honestly have a harder time parsing your code to see that it returns true if any of the two methods return true, but that's probably just my personal preference. I don't have a problem to change the code to you suggestion if you feel strongly about it :)
|
Does |
|
Unfortunately it depends on openldap-dev. How could I make it optional in the python-dependencies file? |
I think the easiest is just to omit it from the dependency list entirely, and only try and import it if ldap is enabled. Since importing is cheap after its done initially, I'd probably import it in |
|
The unit tests are failing due to the fact that Mocks evaluate to True. I propose being slightly evil and changing the ldap enabled tests to |
| (yield self._check_ldap_password(user_id, password)) | ||
| or | ||
| (yield self._check_local_password(user_id, password)) | ||
| )) |
There was a problem hiding this comment.
Line 234 needs to be updated now that this no longer raises but instead returns a boolean.
|
All checks pass :) |
|
Excellent! If you could just sign off on this contribution we can land it: https://github.com/matrix-org/synapse/blob/master/CONTRIBUTING.rst#sign-off Simply replying here with Also, feel free to add yourself to the |
Signed-off-by: Christoph Witzany <[email protected]>
|
Woo! Thanks :) (The failure seems to be due to jenkins failing to set the commit status on github, intriguingly. All the tests pass though) |
|
JFTR if you want to avoid the dependency of OpenLDAP installed in the system, a pure-Python option would be the ldap3 package. |
This pul request enables LDAP authentication. If a users authenticate successfully via the configured LDAP server and are not yet in the local database, they are created.
Missing parts: