Support email-based authentication #58
Conversation
Makes use of a new check_3pid_auth method which allows for authentication via any 3PID method.
…nto anoa/email_auth * 'master' of github.com:matrix-org/matrix-synapse-ldap3: Update LDAP dependency to fix broken unittests
This PR allows password provider modules to bind email addresses when a user is registering and is motivated by matrix-org/matrix-synapse-ldap3#58
ldap_auth_provider.py
Outdated
| @@ -50,7 +50,7 @@ class LDAPMode(object): | |||
|
|
|||
|
|
|||
| class LdapAuthProvider(object): | |||
| __version__ = "0.1" | |||
| __version__ = "0.2" | |||
There was a problem hiding this comment.
I thought since we were adding a relatively big feature, and we should get into the habit of bumping the version number when we do that.
Maybe that doesn't make sense though for this code.
There was a problem hiding this comment.
I think if we do a release that should be done separately from this PR tbh.
There was a problem hiding this comment.
Ah ok, good point.
There was a problem hiding this comment.
Also not sure why we have this line and this:
ldap_auth_provider.py
Outdated
| "User authenticated against LDAP server: %s", | ||
| conn | ||
| ) | ||
| except NameError: |
There was a problem hiding this comment.
What's this doing? We don't expect logging to fail? And we've already logged conn in the debug logging above?
There was a problem hiding this comment.
Ah, this was copied from
matrix-synapse-ldap3/ldap_auth_provider.py
Line 136 in 3c0b342
I assume if conn logs correctly then connection was successfully established. Not sure if this is the recommended way though.
There was a problem hiding this comment.
I think the code before was checking whether we had managed to even define conn as a variable, rather than checking if conn can be stringified.
There was a problem hiding this comment.
I've set an option to the connection function to raise an exception if there's an issue connecting which should solve this?
ldap_auth_provider.py
Outdated
|
|
||
| except ldap3.core.exceptions.LDAPException as e: | ||
| logger.warning("Error during ldap authentication: %s", e) | ||
| defer.returnValue(None) |
There was a problem hiding this comment.
I'd have thought we wouldn't want to silently continue, as that is going to result in the wrong thing?
There was a problem hiding this comment.
Again taken from elsewhere in the file. I agree silently failing isn't great for a client's perspective (they just get "invalid username/password"). Alternative is to actually fail so a M_UNKNOWN is returned which may actually prompt someone to poke their server admin.
Our only options for password providers are to return a user_id or None, either meaning authentication success or fail. We could just raise here though?
There was a problem hiding this comment.
Hmm, ok. raise was what I vote for but we should be consistent.
There was a problem hiding this comment.
So do I. I'll edit all occurrences.
|
Sorry erik, one last time. |
|
Is it possible to add some tests to this if you managed to break it? |
ldap_auth_provider.py
Outdated
| """ | ||
| if self.ldap_mode != LDAPMode.SEARCH: | ||
| logger.warn("3PID LDAP login/register attempted but LDAP search mode " | ||
| "not enabled. Bailing.") |
There was a problem hiding this comment.
Isn't this expected if you haven't enabled search? If so it shouldn't be a warn
There was a problem hiding this comment.
True, just want to make sure people don't get confused if they expect email login to work. Debug instead?
Required Synapse PR: matrix-org/synapse#4931
Adds support for logging in/registering using your email address associated with LDAP. Additionally adds some new functionality to the LDAP search method to ask it for whatever search terms you'd like.