/synapse

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

Already on GitHub? Sign in to your account

Add LDAP authentication #701

Merged
merged 14 commits into from Apr 7, 2016

Conversation

Reviewers
No reviews
Assignees

@erikjohnston erikjohnston

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@DoubleMalt @matrixbot @erikjohnston @aperezdc
@DoubleMalt
Contributor

DoubleMalt commented Apr 6, 2016

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:

  • get email from LDAP server
  • get full name from LDAP server
@matrixbot

matrixbot commented Apr 6, 2016

Can one of the admins verify this patch?
@matrixbot

matrixbot commented Apr 6, 2016

Can one of the admins verify this patch?
@matrixbot

matrixbot commented Apr 6, 2016

Can one of the admins verify this patch?
@matrixbot

matrixbot commented Apr 6, 2016

Can one of the admins verify this patch?
@erikjohnston
Owner

erikjohnston commented Apr 6, 2016

@matrixbot ok to test

(Sorry about the matrixbot spam)
@erikjohnston
Owner

erikjohnston commented Apr 6, 2016

Oh, I'll need to add the new dependency manually to get the tests to run.
@erikjohnston
Owner

erikjohnston commented Apr 6, 2016

Although see: http://matrix.org/jenkins/job/SynapseFlake8Packaging/176/violations/file/synapse/handlers/auth.py/ for some code style violations :)

@erikjohnston erikjohnston and 1 other commented on an outdated diff Apr 6, 2016

View full outdated diff
synapse/handlers/auth.py
@@ -407,11 +423,51 @@ def _find_user_id_and_pwd_hash(self, user_id):
else:
defer.returnValue(user_infos.popitem())
- def _check_password(self, user_id, password, stored_hash):
- """Checks that user_id has passed password, raises LoginError if not."""
- if not self.validate_hash(password, stored_hash):
- logger.warn("Failed password login for user %s", user_id)
- 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))))
@erikjohnston

erikjohnston Apr 6, 2016

Owner

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.

@DoubleMalt

DoubleMalt Apr 6, 2016

Contributor

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 :)

@erikjohnston erikjohnston and 1 other commented on an outdated diff Apr 6, 2016

View full outdated diff
synapse/handlers/auth.py
- def _check_password(self, user_id, password, stored_hash):
- """Checks that user_id has passed password, raises LoginError if not."""
- if not self.validate_hash(password, stored_hash):
- logger.warn("Failed password login for user %s", user_id)
- 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))))
+
+
+ @defer.inlineCallbacks
+ def _check_local_password(self, user_id, password):
+ try:
+ user_id, password_hash = yield self._find_user_id_and_pwd_hash(user_id)
+ defer.returnValue(not self.validate_hash(password, password_hash))
+ except:
@erikjohnston

erikjohnston Apr 6, 2016

Owner

I'd prefer if this only caught login failures, as otherwise it masks programmer/logic errors.

@DoubleMalt

DoubleMalt Apr 6, 2016

Contributor

Good point. Will fix.

@erikjohnston erikjohnston commented on an outdated diff Apr 6, 2016

View full outdated diff
synapse/handlers/auth.py
+
+ local_name = UserID.from_string(user_id).localpart
+
+ dn = "%s=%s, %s" % (self.ldap_search_property, local_name, self.ldap_search_base)
+ logger.debug("DN for LDAP authentication: %s" % dn)
+
+ l.simple_bind_s(dn.encode('utf-8'), password.encode('utf-8'))
+
+ if not (yield self.does_user_exist(user_id)):
+ user_id, access_token = (
+ yield self.hs.get_handlers().registration_handler.register(localpart=local_name)
+ )
+
+ defer.returnValue(True)
+ except ldap.LDAPError, e:
+ logger.info("LDAP error: %s" % e)
@erikjohnston

erikjohnston Apr 6, 2016

Owner

Maybe make this a warning? Also, instead of string formatting you can do: logger.info("LDAP error: %s", e), which saves us doing unnecessary string formatting.
@erikjohnston
Owner

erikjohnston commented Apr 6, 2016

Does python-ldap have a system dependency? If it does then I'd probably want to make the python-ldap dependency optional somehow, otherwise its going to break everyone's synapse on upgrade.
@DoubleMalt
Contributor

DoubleMalt commented Apr 6, 2016

Unfortunately it depends on openldap-dev. How could I make it optional in the python-dependencies file?
@erikjohnston
Owner

erikjohnston commented Apr 6, 2016

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 __init__ (to catch early if ldap is enabled, but they didn't add the dependency) and then just reimport it in _check_ldap_password

@erikjohnston erikjohnston self-assigned this Apr 6, 2016

@erikjohnston
Owner

erikjohnston commented Apr 6, 2016

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 if self.ldap_enabled is True and if self.ldap_enabled is not True

@erikjohnston erikjohnston commented on the diff Apr 6, 2016

View full changes
synapse/handlers/auth.py
@@ -407,11 +425,60 @@ def _find_user_id_and_pwd_hash(self, user_id):
else:
defer.returnValue(user_infos.popitem())
- def _check_password(self, user_id, password, stored_hash):
- """Checks that user_id has passed password, raises LoginError if not."""
- if not self.validate_hash(password, stored_hash):
- logger.warn("Failed password login for user %s", user_id)
- 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))
+ ))
@erikjohnston

erikjohnston Apr 6, 2016

Owner

Line 234 needs to be updated now that this no longer raises but instead returns a boolean.

DoubleMalt added some commits Apr 5, 2016

@DoubleMalt
Introduce LDAP authentication
3d95405
@DoubleMalt
move LDAP authentication to AuthenticationHandler
7b9319b
@DoubleMalt
add tls property
92767dd
@DoubleMalt
add tls property and twist my head around twisted
823b8be
@DoubleMalt
cleanup
8f0e47f
@DoubleMalt
code style
afff321
@DoubleMalt
fix exception handling
67f3a50
@DoubleMalt
fix pep8
875ed05
@DoubleMalt
conditionally import ldap
4c5e8ad
@DoubleMalt
output ldap version for info and to pacify pep8
3555a65
@DoubleMalt
make tests for ldap more specific to not be fooled by Mocks
27a0c21
@DoubleMalt
remove line

Some checks were not successful

2 failing and 2 successful checks
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
@matrixbot
Flake8 + Packaging (Merged PR) — Build finished.
Details
@matrixbot
Unit Tests (Merged PR) — Build finished.
Details
9c62fcd
@DoubleMalt
fix check for failed authentication

All checks have passed

4 successful checks
@matrixbot
Flake8 + Packaging (Merged PR) — Build finished.
Details
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
@matrixbot
Unit Tests (Merged PR) — Build finished.
Details
ed4d18f
@DoubleMalt
Contributor

DoubleMalt commented Apr 6, 2016

All checks pass :)
@erikjohnston
Owner

erikjohnston commented Apr 7, 2016

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 Signed-off-by: Your Name <your@email.example.org>, is sufficient.

Also, feel free to add yourself to the AUTHORS.rst!
@DoubleMalt
Add myself to AUTHORS.rst 
Signed-off-by: Christoph Witzany <christoph@web.crofting.com>

Some checks were not successful

1 failing and 3 successful checks
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Flake8 + Packaging (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
@matrixbot
Unit Tests (Merged PR) — Build finished.
Details
674379e
@erikjohnston
Owner

erikjohnston commented Apr 7, 2016

Woo! Thanks :)

(The failure seems to be due to jenkins failing to set the commit status on github, intriguingly. All the tests pass though)

@erikjohnston erikjohnston merged commit f942980 into matrix-org:develop Apr 7, 2016
3 of 4 checks passed

3 of 4 checks passed

Sytest Postgres (Merged PR) Build finished.
Details
Flake8 + Packaging (Merged PR) Build finished.
Details
Sytest SQLite (Merged PR) Build finished.
Details
Unit Tests (Merged PR) Build finished.
Details

@DoubleMalt DoubleMalt deleted the DoubleMalt:ldap-auth branch Apr 11, 2016

@simsasaile simsasaile referenced this pull request Apr 18, 2016

Closed

LDAP Support #670

@aperezdc
Contributor

aperezdc commented May 2, 2016

JFTR if you want to avoid the dependency of OpenLDAP installed in the system, a pure-Python option would be the ldap3 package.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment