Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Support 3PID login in password providers #4931

Merged
merged 14 commits into from
Mar 26, 2019
Merged

Conversation

anoadragon453
Copy link
Member

This change allows for 3PID login via password provider modules and is motivated by matrix-org/matrix-synapse-ldap3#58

Additionally adds the ability for password provider modules to set the default displayname and any number of email addresses for a user.

There are also some little fixes littered about that I found about the codebase locations I touched, and hope they aren't too annoying look through.

Adds a new method, check_3pid_auth, which gives password providers
the chance to allow authentication with third-party identifiers such
as email or msisdn.
@codecov
Copy link

codecov bot commented Mar 25, 2019

Codecov Report

Merging #4931 into develop will increase coverage by <.01%.
The diff coverage is 45.45%.

@@             Coverage Diff             @@
##           develop    #4931      +/-   ##
===========================================
+ Coverage    78.03%   78.04%   +<.01%     
===========================================
  Files          328      328              
  Lines        34072    34090      +18     
  Branches      5622     5627       +5     
===========================================
+ Hits         26589    26604      +15     
- Misses        5868     5870       +2     
- Partials      1615     1616       +1

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally seems sane. a few nits

docs/password_auth_providers.rst Outdated Show resolved Hide resolved
@@ -97,3 +97,14 @@ Password auth provider classes may optionally provide the following methods.

It may return a Twisted ``Deferred`` object; the logout request will wait
for the deferred to complete but the result is ignored.

``someprovider.check_3pid_auth``\(*medium*, *address*, *password*)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest putting this up with check_auth

synapse/handlers/auth.py Outdated Show resolved Hide resolved
synapse/handlers/auth.py Show resolved Hide resolved
synapse/module_api/__init__.py Outdated Show resolved Hide resolved
changelog.d/4931.feature Outdated Show resolved Hide resolved
)

# Bind email address with the registered identity service
unix_secs = int(time.time())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you want to use hs.get_clock(), mostly because it makes testing easier if anyone ever comes to unit test this

* develop: (141 commits)
  Make federation endpoints more tolerant of trailing slashes v2 (#4935)
  Fix ClientReplicationStreamProtocol.__str__ (#4929)
  Fix bug where read-receipts lost their timestamps (#4927)
  Use an explicit dbname for postgres connections in the tests. (#4928)
  Fix nginx example in ACME doc. (#4923)
  Refactor out state delta handling into its own class (#4917)
  Newsfile
  Use yaml safe_load
  Allow newsfragments to end with exclamation marks! (#4912)
  Some more porting to HomeserverTestCase and remove old RESTHelper (#4913)
  Clean up backoff_on_404 and metehod calls
  Update changelog.d/4908.bugfix
  Update Apache Setup To Remove Location Syntax (#4870)
  isort
  Newsfile
  Fix typo and add description
  Deny peeking into rooms that have been blocked
  Rejig testcase to make it more extensible
  Remove debug
  Add tests
  ...
@anoadragon453 anoadragon453 dismissed richvdh’s stale review March 26, 2019 12:34

Addressed changes and moved email out-of-scope

@anoadragon453 anoadragon453 requested a review from richvdh March 26, 2019 12:35

The method should return a Twisted ``Deferred`` object, which resolves to
a ``str`` containing the user's (canonical) User ID if authentication was
successful, and ``None`` if not. The ``Deferred`` can also instead
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"also instead"

How about:

As with check_auth, the Deferred may alternatively resolve to a (user_id, callback) tuple.

... rather than duplicating the whole thing. Suggest a separate paragraph for clarity too.

password (str): The password of the user.

Returns:
Deferred[(str|None, None)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does the result mean?


Args:
localpart (str): The localpart of the new user.
displayname (str|None): The displayname of the new user.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if it is set to None?

@anoadragon453 anoadragon453 dismissed richvdh’s stale review March 26, 2019 14:40

Addressed changes

@anoadragon453 anoadragon453 requested a review from richvdh March 26, 2019 14:40
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@anoadragon453 anoadragon453 merged commit bbd244c into develop Mar 26, 2019
@anoadragon453 anoadragon453 deleted the anoa/ldap_email_support branch March 26, 2019 17:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants