From b674bb85008e75ae52067006fd51a519cfe2b6ba Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Fri, 12 Jun 2020 12:06:19 +0100
Subject: [PATCH 01/16] Move utility methods from login handler to auth handler

---
 synapse/handlers/auth.py        | 81 +++++++++++++++++++++++++++++++++
 synapse/rest/client/v1/login.py | 46 +++----------------
 2 files changed, 88 insertions(+), 39 deletions(-)

diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index 119678e67ba9..98d8a86d146d 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -45,12 +45,93 @@
 from synapse.module_api import ModuleApi
 from synapse.push.mailer import load_jinja2_templates
 from synapse.types import Requester, UserID
+from synapse.util.msisdn import phone_number_to_msisdn
 
 from ._base import BaseHandler
 
 logger = logging.getLogger(__name__)
 
 
+def client_dict_convert_legacy_fields_to_identifier(
+    submission: Dict[str, Union[str, Dict]]
+):
+    """Take a legacy-formatted login submission or User-Interactive Authentication dict and
+    updates it to feature an identifier dict instead.
+    Providing user-identifying information at the top-level of a login or UIA submission is
+    now deprecated and replaced with identifiers:
+    https://matrix.org/docs/spec/client_server/r0.6.1#identifier-types
+    Args:
+        submission: The client dict to convert. Passed by reference and modified
+    Raises:
+        SynapseError: if the dict contains a "medium" parameter that is anything other than
+            "email"
+    """
+    if "user" in submission:
+        submission["identifier"] = {"type": "m.id.user", "user": submission["user"]}
+        del submission["user"]
+
+    if "medium" in submission and "address" in submission:
+        submission["identifier"] = {
+            "type": "m.id.thirdparty",
+            "medium": submission["medium"],
+            "address": submission["address"],
+        }
+        del submission["medium"]
+        del submission["address"]
+
+    # We've converted valid, legacy login submissions to an identifier. If the
+    # dict still doesn't have an identifier, it's invalid
+    if "identifier" not in submission:
+        raise SynapseError(
+            400,
+            "Missing 'identifier' parameter in login submission",
+            errcode=Codes.MISSING_PARAM,
+        )
+
+    # Ensure the identifier has a type
+    if "type" not in submission["identifier"]:
+        raise SynapseError(
+            400, "'identifier' dict has no key 'type'", errcode=Codes.MISSING_PARAM,
+        )
+
+
+def login_id_phone_to_thirdparty(identifier: Dict[str, str]):
+    """Convert a phone login identifier type to a generic threepid identifier. Modifies
+    the identifier dict in place
+    Args:
+        identifier: Login identifier dict of type 'm.id.phone'
+    """
+    if "type" not in identifier:
+        raise SynapseError(
+            400, "Invalid phone-type identifier", errcode=Codes.MISSING_PARAM
+        )
+
+    if "country" not in identifier or (
+        # XXX: We used to require `number` instead of `phone`. The spec
+        # defines `phone`. So accept both
+        "phone" not in identifier
+        and "number" not in identifier
+    ):
+        raise SynapseError(
+            400, "Invalid phone-type identifier", errcode=Codes.INVALID_PARAM
+        )
+
+    # Accept both "phone" and "number" as valid keys in m.id.phone
+    phone_number = identifier.get("phone", identifier.get("number"))
+
+    # Convert user-provided phone number to a consistent representation
+    msisdn = phone_number_to_msisdn(identifier["country"], phone_number)
+
+    # Modify the passed dictionary by reference
+    del identifier["country"]
+    identifier.pop("number", None)
+    identifier.pop("phone", None)
+
+    identifier["type"] = "m.id.thirdparty"
+    identifier["medium"] = "msisdn"
+    identifier["address"] = msisdn
+
+
 class AuthHandler(BaseHandler):
     SESSION_EXPIRE_MS = 48 * 60 * 60 * 1000
 
diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py
index dceb2792fa7a..e106e109ee4b 100644
--- a/synapse/rest/client/v1/login.py
+++ b/synapse/rest/client/v1/login.py
@@ -17,6 +17,10 @@
 
 from synapse.api.errors import Codes, LoginError, SynapseError
 from synapse.api.ratelimiting import Ratelimiter
+from synapse.handlers.auth import (
+    client_dict_convert_legacy_fields_to_identifier,
+    login_id_phone_to_thirdparty,
+)
 from synapse.http.server import finish_request
 from synapse.http.servlet import (
     RestServlet,
@@ -27,47 +31,10 @@
 from synapse.rest.client.v2_alpha._base import client_patterns
 from synapse.rest.well_known import WellKnownBuilder
 from synapse.types import UserID
-from synapse.util.msisdn import phone_number_to_msisdn
 
 logger = logging.getLogger(__name__)
 
 
-def login_submission_legacy_convert(submission):
-    """
-    If the input login submission is an old style object
-    (ie. with top-level user / medium / address) convert it
-    to a typed object.
-    """
-    if "user" in submission:
-        submission["identifier"] = {"type": "m.id.user", "user": submission["user"]}
-        del submission["user"]
-
-    if "medium" in submission and "address" in submission:
-        submission["identifier"] = {
-            "type": "m.id.thirdparty",
-            "medium": submission["medium"],
-            "address": submission["address"],
-        }
-        del submission["medium"]
-        del submission["address"]
-
-
-def login_id_thirdparty_from_phone(identifier):
-    """
-    Convert a phone login identifier type to a generic threepid identifier
-    Args:
-        identifier(dict): Login identifier dict of type 'm.id.phone'
-
-    Returns: Login identifier dict of type 'm.id.threepid'
-    """
-    if "country" not in identifier or "number" not in identifier:
-        raise SynapseError(400, "Invalid phone-type identifier")
-
-    msisdn = phone_number_to_msisdn(identifier["country"], identifier["number"])
-
-    return {"type": "m.id.thirdparty", "medium": "msisdn", "address": msisdn}
-
-
 class LoginRestServlet(RestServlet):
     PATTERNS = client_patterns("/login$", v1=True)
     CAS_TYPE = "m.login.cas"
@@ -174,7 +141,8 @@ async def _do_other_login(self, login_submission):
             login_submission.get("address"),
             login_submission.get("user"),
         )
-        login_submission_legacy_convert(login_submission)
+        # Convert deprecated authdict formats to the current scheme
+        client_dict_convert_legacy_fields_to_identifier(login_submission)
 
         if "identifier" not in login_submission:
             raise SynapseError(400, "Missing param: identifier")
@@ -185,7 +153,7 @@ async def _do_other_login(self, login_submission):
 
         # convert phone type identifiers to generic threepids
         if identifier["type"] == "m.id.phone":
-            identifier = login_id_thirdparty_from_phone(identifier)
+            identifier = login_id_phone_to_thirdparty(identifier)
 
         # convert threepid identifiers to user IDs
         if identifier["type"] == "m.id.thirdparty":

From 7044c1f4fbba02ac39f936936538a5a082985055 Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Fri, 12 Jun 2020 13:09:52 +0100
Subject: [PATCH 02/16] Factor out identifier -> username conversion into its
 own method

We then use this in both login and authhandler, the latter being where we process m.login.password
User Interactive Authentication responses, which can now include identifiers
---
 synapse/handlers/auth.py        |  81 ++++++++++++++++++++++
 synapse/rest/client/v1/login.py | 115 +++++++++-----------------------
 2 files changed, 112 insertions(+), 84 deletions(-)

diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index 98d8a86d146d..372429fa48c9 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -603,6 +603,87 @@ async def _check_auth_dict(
         (canonical_id, callback) = await self.validate_login(user_id, authdict)
         return canonical_id
 
+    async def username_from_identifier(
+        self, identifier: Dict[str, str], password: Optional[str] = None
+    ) -> Optional[str]:
+        """Given a dictionary containing an identifier from a client, extract the
+        possibly unqualified username of the user that it identifies. Does *not*
+        guarantee that the user exists.
+        If this identifier dict contains a threepid, we attempt to ask password
+        auth providers about it or, failing that, look up an associated user in
+        the database.
+        Args:
+            identifier: The identifier dictionary provided by the client
+            password: The user provided password if one exists. Used for asking
+                password auth providers for usernames from 3pid+password combos.
+        Returns:
+            A username if one was found, or None otherwise
+        Raises:
+            SynapseError: If the identifier dict is invalid
+        """
+
+        # Convert phone type identifiers to generic threepid identifiers, which
+        # will be handled in the next step
+        if identifier["type"] == "m.id.phone":
+            login_id_phone_to_thirdparty(identifier)
+
+        # Convert a threepid identifier to an user identifier
+        if identifier["type"] == "m.id.thirdparty":
+            address = identifier.get("address")
+            medium = identifier.get("medium")
+
+            if not medium or not address:
+                # An error would've already been raised in
+                # `login_id_thirdparty_from_phone` if the original submission
+                # was a phone identifier
+                raise SynapseError(
+                    400, "Invalid thirdparty identifier", errcode=Codes.INVALID_PARAM,
+                )
+
+            if medium == "email":
+                # For emails, transform the address to lowercase.
+                # We store all email addresses as lowercase in the DB.
+                # (See add_threepid in synapse/handlers/auth.py)
+                address = address.lower()
+
+            # Check for auth providers that support 3pid login types
+            if password is not None:
+                canonical_user_id, _ = await self.check_password_provider_3pid(
+                    medium, address, password,
+                )
+                if canonical_user_id:
+                    # Authentication through password provider and 3pid succeeded
+                    return canonical_user_id
+
+            # Check local store
+            user_id = await self.hs.get_datastore().get_user_id_by_threepid(
+                medium, address
+            )
+            if not user_id:
+                # We were unable to find a user_id that belonged to the threepid returned
+                # by the password auth provider
+                return None
+
+            identifier = {"type": "m.id.user", "user": user_id}
+
+        # By this point, the identifier should be a `m.id.user`: if it's anything
+        # else, we haven't understood it.
+        if identifier["type"] != "m.id.user":
+            raise SynapseError(
+                400, "Unknown login identifier type", errcode=Codes.INVALID_PARAM,
+            )
+
+        # User identifiers have a "user" key
+        user = identifier.get("user")
+        if user is None:
+            raise SynapseError(
+                400,
+                "User identifier is missing 'user' key",
+                errcode=Codes.INVALID_PARAM,
+            )
+
+        return user
+
     def _get_params_recaptcha(self) -> dict:
         return {"public_key": self.hs.config.recaptcha_public_key}
 
diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py
index e106e109ee4b..509925dd7c02 100644
--- a/synapse/rest/client/v1/login.py
+++ b/synapse/rest/client/v1/login.py
@@ -17,10 +17,7 @@
 
 from synapse.api.errors import Codes, LoginError, SynapseError
 from synapse.api.ratelimiting import Ratelimiter
-from synapse.handlers.auth import (
-    client_dict_convert_legacy_fields_to_identifier,
-    login_id_phone_to_thirdparty,
-)
+from synapse.handlers.auth import client_dict_convert_legacy_fields_to_identifier
 from synapse.http.server import finish_request
 from synapse.http.servlet import (
     RestServlet,
@@ -144,99 +141,49 @@ async def _do_other_login(self, login_submission):
         # Convert deprecated authdict formats to the current scheme
         client_dict_convert_legacy_fields_to_identifier(login_submission)
 
-        if "identifier" not in login_submission:
-            raise SynapseError(400, "Missing param: identifier")
-
-        identifier = login_submission["identifier"]
-        if "type" not in identifier:
-            raise SynapseError(400, "Login identifier has no type")
-
-        # convert phone type identifiers to generic threepids
-        if identifier["type"] == "m.id.phone":
-            identifier = login_id_phone_to_thirdparty(identifier)
-
-        # convert threepid identifiers to user IDs
-        if identifier["type"] == "m.id.thirdparty":
-            address = identifier.get("address")
-            medium = identifier.get("medium")
-
-            if medium is None or address is None:
-                raise SynapseError(400, "Invalid thirdparty identifier")
-
-            if medium == "email":
-                # For emails, transform the address to lowercase.
-                # We store all email addreses as lowercase in the DB.
-                # (See add_threepid in synapse/handlers/auth.py)
-                address = address.lower()
-
-            # We also apply account rate limiting using the 3PID as a key, as
-            # otherwise using 3PID bypasses the ratelimiting based on user ID.
-            self._failed_attempts_ratelimiter.ratelimit((medium, address), update=False)
-
-            # Check for login providers that support 3pid login types
-            (
-                canonical_user_id,
-                callback_3pid,
-            ) = await self.auth_handler.check_password_provider_3pid(
-                medium, address, login_submission["password"]
+        # Check whether this attempt uses a threepid, if so, check if our failed attempt
+        # ratelimiter allows another attempt at this time
+        medium, address = (
+            login_submission.get("medium"),
+            login_submission.get("address"),
+        )
+        if medium and address:
+            self._failed_attempts_ratelimiter.ratelimit(
+                (medium.lower(), address.lower()), update=False
             )
-            if canonical_user_id:
-                # Authentication through password provider and 3pid succeeded
 
-                result = await self._complete_login(
-                    canonical_user_id, login_submission, callback_3pid
-                )
-                return result
+        # Extract a localpart or user ID from the values in the identifier
+        username = await self.auth_handler.username_from_identifier(
+            login_submission["identifier"], login_submission.get("password")
+        )
 
-            # No password providers were able to handle this 3pid
-            # Check local store
-            user_id = await self.hs.get_datastore().get_user_id_by_threepid(
-                medium, address
-            )
-            if not user_id:
-                logger.warning(
-                    "unknown 3pid identifier medium %s, address %r", medium, address
+        if not username:
+            if medium and address:
+                # The user attempted to login via threepid and failed
+                # Record this failed attempt
+                self._failed_attempts_ratelimiter.can_do_action(
+                    (medium.lower(), address.lower())
                 )
-                # We mark that we've failed to log in here, as
-                # `check_password_provider_3pid` might have returned `None` due
-                # to an incorrect password, rather than the account not
-                # existing.
-                #
-                # If it returned None but the 3PID was bound then we won't hit
-                # this code path, which is fine as then the per-user ratelimit
-                # will kick in below.
-                self._failed_attempts_ratelimiter.can_do_action((medium, address))
-                raise LoginError(403, "", errcode=Codes.FORBIDDEN)
-
-            identifier = {"type": "m.id.user", "user": user_id}
-
-        # by this point, the identifier should be an m.id.user: if it's anything
-        # else, we haven't understood it.
-        if identifier["type"] != "m.id.user":
-            raise SynapseError(400, "Unknown login identifier type")
-        if "user" not in identifier:
-            raise SynapseError(400, "User identifier is missing 'user' key")
-
-        if identifier["user"].startswith("@"):
-            qualified_user_id = identifier["user"]
-        else:
-            qualified_user_id = UserID(identifier["user"], self.hs.hostname).to_string()
-
-        # Check if we've hit the failed ratelimit (but don't update it)
-        self._failed_attempts_ratelimiter.ratelimit(
-            qualified_user_id.lower(), update=False
-        )
+
+                raise LoginError(403, "Unauthorized threepid", errcode=Codes.FORBIDDEN)
+
+            # The login failed for another reason
+            raise LoginError(403, "Invalid login", errcode=Codes.FORBIDDEN)
+
+        # We were able to extract a username successfully
+        # Check if we've hit the failed ratelimit for this user ID
+        self._failed_attempts_ratelimiter.ratelimit(username.lower(), update=False)
 
         try:
             canonical_user_id, callback = await self.auth_handler.validate_login(
-                identifier["user"], login_submission
+                username, login_submission
             )
         except LoginError:
             # The user has failed to log in, so we need to update the rate
             # limiter. Using `can_do_action` avoids us raising a ratelimit
             # exception and masking the LoginError. The actual ratelimiting
             # should have happened above.
-            self._failed_attempts_ratelimiter.can_do_action(qualified_user_id.lower())
+            self._failed_attempts_ratelimiter.can_do_action(username.lower())
             raise
 
         result = await self._complete_login(

From f240a8d18268c1c8cd1573c2f2128d2732a75249 Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Fri, 12 Jun 2020 13:21:37 +0100
Subject: [PATCH 03/16] Reconfigure m.login.password authdict checker to
 process identifiers

---
 synapse/handlers/auth.py | 40 ++++++++++++++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index 372429fa48c9..08b16895c9fd 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -593,14 +593,42 @@ async def _check_auth_dict(
             res = await checker.check_auth(authdict, clientip=clientip)
             return res
 
-        # build a v1-login-style dict out of the authdict and fall back to the
-        # v1 code
-        user_id = authdict.get("user")
+        # We don't have a checker for the auth type provided by the client
+        # Assume that it is `m.login.password`.
+        if login_type != LoginType.PASSWORD:
+            raise SynapseError(
+                400, "Unknown authentication type", errcode=Codes.INVALID_PARAM,
+            )
+
+        password = authdict.get("password")
+        if password is None:
+            raise SynapseError(
+                400,
+                "Missing parameter for m.login.password dict: 'password'",
+                errcode=Codes.INVALID_PARAM,
+            )
+
+        # Retrieve the user ID using details provided in the authdict
+
+        # Deprecation notice: Clients used to be able to simply provide a
+        # `user` field which pointed to a user_id or localpart. This has
+        # been deprecated in favour of an `identifier` key, which is a
+        # dictionary providing information on how to identify a single
+        # user.
+        # https://matrix.org/docs/spec/client_server/r0.6.1#identifier-types
+        #
+        # We convert old-style dicts to new ones here
+        client_dict_convert_legacy_fields_to_identifier(authdict)
+
+        # Extract a user ID from the values in the identifier
+        username = await self.username_from_identifier(authdict["identifier"], password)
+
+        if username is None:
+            raise SynapseError(400, "Valid username not found")
 
-        if user_id is None:
-            raise SynapseError(400, "", Codes.MISSING_PARAM)
+        # Now that we've found the username, validate that the password is correct
+        canonical_id, _ = await self.validate_login(username, authdict)
 
-        (canonical_id, callback) = await self.validate_login(user_id, authdict)
         return canonical_id
 
     async def username_from_identifier(

From cb64c956f0bd9667aeb0298cd371eb900934371d Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Fri, 12 Jun 2020 13:34:01 +0100
Subject: [PATCH 04/16] Comment cleanups, log on KeyError during login

---
 synapse/handlers/auth.py        | 13 +++++++------
 synapse/rest/client/v1/login.py | 13 +++++++------
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index 08b16895c9fd..03da1a51355f 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -396,7 +396,7 @@ async def check_auth(
             # otherwise use whatever was last provided.
             #
             # This was designed to allow the client to omit the parameters
-            # and just supply the session in subsequent calls so it split
+            # and just supply the session in subsequent calls. So it splits
             # auth between devices by just sharing the session, (eg. so you
             # could continue registration from your phone having clicked the
             # email auth link on there). It's probably too open to abuse
@@ -876,7 +876,8 @@ async def validate_login(
         m.login.password auth types.
 
         Args:
-            username: username supplied by the user
+            username: a localpart or fully qualified user ID - what is provided by the
+                client
             login_submission: the whole of the login submission
                 (including 'type' and other relevant fields)
         Returns:
@@ -888,10 +889,10 @@ async def validate_login(
             LoginError if there was an authentication problem.
         """
 
-        if username.startswith("@"):
-            qualified_user_id = username
-        else:
-            qualified_user_id = UserID(username, self.hs.hostname).to_string()
+        # We need a fully qualified User ID for some method calls here
+        qualified_user_id = username
+        if not qualified_user_id.startswith("@"):
+            qualified_user_id = UserID(qualified_user_id, self.hs.hostname).to_string()
 
         login_type = login_submission.get("type")
         known_login_type = False
diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py
index 509925dd7c02..14189828ff28 100644
--- a/synapse/rest/client/v1/login.py
+++ b/synapse/rest/client/v1/login.py
@@ -111,7 +111,8 @@ async def on_POST(self, request):
                 result = await self.do_token_login(login_submission)
             else:
                 result = await self._do_other_login(login_submission)
-        except KeyError:
+        except KeyError as e:
+            logger.debug("KeyError during login: %s", e)
             raise SynapseError(400, "Missing JSON keys.")
 
         well_known_data = self._well_known_builder.get_well_known()
@@ -181,8 +182,8 @@ async def _do_other_login(self, login_submission):
         except LoginError:
             # The user has failed to log in, so we need to update the rate
             # limiter. Using `can_do_action` avoids us raising a ratelimit
-            # exception and masking the LoginError. The actual ratelimiting
-            # should have happened above.
+            # exception and masking the LoginError. This just records the attempt.
+            # The actual rate-limiting happens above
             self._failed_attempts_ratelimiter.can_do_action(username.lower())
             raise
 
@@ -195,10 +196,10 @@ async def _complete_login(
         self, user_id, login_submission, callback=None, create_non_existent_users=False
     ):
         """Called when we've successfully authed the user and now need to
-        actually login them in (e.g. create devices). This gets called on
-        all succesful logins.
+        actually log them in (e.g. create devices). This gets called on
+        all successful logins.
 
-        Applies the ratelimiting for succesful login attempts against an
+        Applies the ratelimiting for successful login attempts against an
         account.
 
         Args:

From 18071156e49d49847b87a418db09d04346ee9b16 Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Fri, 12 Jun 2020 13:44:53 +0100
Subject: [PATCH 05/16] Remove placeholders/dummy classes for supporting
 identifiers in existing tests

---
 tests/rest/client/v1/test_login.py      | 4 +---
 tests/rest/client/v2_alpha/test_auth.py | 8 --------
 2 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/tests/rest/client/v1/test_login.py b/tests/rest/client/v1/test_login.py
index 9033f09fd2e3..c6d8f24fe924 100644
--- a/tests/rest/client/v1/test_login.py
+++ b/tests/rest/client/v1/test_login.py
@@ -273,9 +273,7 @@ def _delete_device(self, access_token, user_id, password, device_id):
 
         auth = {
             "type": "m.login.password",
-            # https://github.com/matrix-org/synapse/issues/5665
-            # "identifier": {"type": "m.id.user", "user": user_id},
-            "user": user_id,
+            "identifier": {"type": "m.id.user", "user": user_id},
             "password": password,
             "session": channel.json_body["session"],
         }
diff --git a/tests/rest/client/v2_alpha/test_auth.py b/tests/rest/client/v2_alpha/test_auth.py
index 293ccfba2bb1..8f97dd0dd216 100644
--- a/tests/rest/client/v2_alpha/test_auth.py
+++ b/tests/rest/client/v2_alpha/test_auth.py
@@ -38,11 +38,6 @@ def check_auth(self, authdict, clientip):
         return succeed(True)
 
 
-class DummyPasswordChecker(UserInteractiveAuthChecker):
-    def check_auth(self, authdict, clientip):
-        return succeed(authdict["identifier"]["user"])
-
-
 class FallbackAuthTests(unittest.HomeserverTestCase):
 
     servlets = [
@@ -166,9 +161,6 @@ class UIAuthTests(unittest.HomeserverTestCase):
     ]
 
     def prepare(self, reactor, clock, hs):
-        auth_handler = hs.get_auth_handler()
-        auth_handler.checkers[LoginType.PASSWORD] = DummyPasswordChecker(hs)
-
         self.user_pass = "pass"
         self.user = self.register_user("test", self.user_pass)
         self.user_tok = self.login("test", self.user_pass)

From 358e51be869c5dfe1ea641a7a37bb5d764b4242f Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Fri, 12 Jun 2020 13:55:49 +0100
Subject: [PATCH 06/16] Add some tests for m.id.phone and m.id.thirdparty

---
 tests/rest/client/v2_alpha/test_register.py | 92 +++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/tests/rest/client/v2_alpha/test_register.py b/tests/rest/client/v2_alpha/test_register.py
index 7deaf5b24a48..7e3dc22f64c5 100644
--- a/tests/rest/client/v2_alpha/test_register.py
+++ b/tests/rest/client/v2_alpha/test_register.py
@@ -593,6 +593,89 @@ def test_deactivated_user(self):
 
         self.assertEqual(len(self.email_attempts), 0)
 
+    def test_deactivated_user_using_user_identifier(self):
+        self.email_attempts = []
+
+        (user_id, tok) = self.create_user()
+
+        request_data = json.dumps(
+            {
+                "auth": {
+                    "type": "m.login.password",
+                    "identifier": {"type": "m.id.user", "user": user_id},
+                    "password": "monkey",
+                },
+                "erase": False,
+            }
+        )
+        request, channel = self.make_request(
+            "POST", "account/deactivate", request_data, access_token=tok
+        )
+        self.render(request)
+        self.assertEqual(request.code, 200)
+
+        self.reactor.advance(datetime.timedelta(days=8).total_seconds())
+
+        self.assertEqual(len(self.email_attempts), 0)
+
+    def test_deactivated_user_using_thirdparty_identifier(self):
+        self.email_attempts = []
+
+        (user_id, tok) = self.create_user()
+
+        request_data = json.dumps(
+            {
+                "auth": {
+                    "type": "m.login.password",
+                    "identifier": {
+                        "type": "m.id.thirdparty",
+                        "medium": "email",
+                        "address": "kermit@example.com",
+                    },
+                    "password": "monkey",
+                },
+                "erase": False,
+            }
+        )
+        request, channel = self.make_request(
+            "POST", "account/deactivate", request_data, access_token=tok
+        )
+        self.render(request)
+        self.assertEqual(request.code, 200)
+
+        self.reactor.advance(datetime.timedelta(days=8).total_seconds())
+
+        self.assertEqual(len(self.email_attempts), 0)
+
+    def test_deactivated_user_using_phone_identifier(self):
+        self.email_attempts = []
+
+        (user_id, tok) = self.create_user()
+
+        request_data = json.dumps(
+            {
+                "auth": {
+                    "type": "m.login.password",
+                    "identifier": {
+                        "type": "m.id.phone",
+                        "country": "GB",
+                        "phone": "077-009-00001",
+                    },
+                    "password": "monkey",
+                },
+                "erase": False,
+            }
+        )
+        request, channel = self.make_request(
+            "POST", "account/deactivate", request_data, access_token=tok
+        )
+        self.render(request)
+        self.assertEqual(request.code, 200)
+
+        self.reactor.advance(datetime.timedelta(days=8).total_seconds())
+
+        self.assertEqual(len(self.email_attempts), 0)
+
     def create_user(self):
         user_id = self.register_user("kermit", "monkey")
         tok = self.login("kermit", "monkey")
@@ -608,6 +691,15 @@ def create_user(self):
                 added_at=now,
             )
         )
+        self.get_success(
+            self.store.user_add_threepid(
+                user_id=user_id,
+                medium="msisdn",
+                address="447700900001",
+                validated_at=now,
+                added_at=now,
+            )
+        )
         return user_id, tok
 
     def test_manual_email_send_expired_account(self):

From 699904c9d8312bfffde56300579579bb2817f8ef Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Fri, 12 Jun 2020 11:23:40 +0100
Subject: [PATCH 07/16] Changelog

---
 changelog.d/7438.feature | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 changelog.d/7438.feature

diff --git a/changelog.d/7438.feature b/changelog.d/7438.feature
new file mode 100644
index 000000000000..b00529790e13
--- /dev/null
+++ b/changelog.d/7438.feature
@@ -0,0 +1 @@
+Support `identifier` dictionary fields in User-Interactive Authentication flows. Relax requirement of the `user` parameter.

From 7184c16f95d7f856105e50b8ac6ea7e51b23ba3e Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Tue, 16 Jun 2020 11:03:49 +0100
Subject: [PATCH 08/16] Change login_id_phone_to_thirdparty to return a dict
 again

---
 synapse/handlers/auth.py | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index 03da1a51355f..803322a432e5 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -95,11 +95,14 @@ def client_dict_convert_legacy_fields_to_identifier(
         )
 
 
-def login_id_phone_to_thirdparty(identifier: Dict[str, str]):
-    """Convert a phone login identifier type to a generic threepid identifier. Modifies
-    the identifier dict in place
+def login_id_phone_to_thirdparty(identifier: Dict[str, str]) -> Dict[str, str]:
+    """Convert a phone login identifier type to a generic threepid identifier.
+
     Args:
         identifier: Login identifier dict of type 'm.id.phone'
+
+    Returns:
+        An equivalent m.id.thirdparty identifier dict.
     """
     if "type" not in identifier:
         raise SynapseError(
@@ -122,14 +125,12 @@ def login_id_phone_to_thirdparty(identifier: Dict[str, str]):
     # Convert user-provided phone number to a consistent representation
     msisdn = phone_number_to_msisdn(identifier["country"], phone_number)
 
-    # Modify the passed dictionary by reference
-    del identifier["country"]
-    identifier.pop("number", None)
-    identifier.pop("phone", None)
-
-    identifier["type"] = "m.id.thirdparty"
-    identifier["medium"] = "msisdn"
-    identifier["address"] = msisdn
+    # Return the new dictionary
+    return {
+        "type": "m.id.thirdparty",
+        "medium": "msisdn",
+        "address": msisdn,
+    }
 
 
 class AuthHandler(BaseHandler):
@@ -653,7 +654,7 @@ async def username_from_identifier(
         # Convert phone type identifiers to generic threepid identifiers, which
         # will be handled in the next step
         if identifier["type"] == "m.id.phone":
-            login_id_phone_to_thirdparty(identifier)
+            identifier = login_id_phone_to_thirdparty(identifier)
 
         # Convert a threepid identifier to an user identifier
         if identifier["type"] == "m.id.thirdparty":

From 187623517b49678bfd551d119144b34bca7de5e2 Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Tue, 16 Jun 2020 11:09:16 +0100
Subject: [PATCH 09/16] pop() instead of pull then del

---
 synapse/handlers/auth.py | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index 803322a432e5..57dead89b5a0 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -67,17 +67,14 @@ def client_dict_convert_legacy_fields_to_identifier(
             "email"
     """
     if "user" in submission:
-        submission["identifier"] = {"type": "m.id.user", "user": submission["user"]}
-        del submission["user"]
+        submission["identifier"] = {"type": "m.id.user", "user": submission.pop("user")}
 
     if "medium" in submission and "address" in submission:
         submission["identifier"] = {
             "type": "m.id.thirdparty",
-            "medium": submission["medium"],
-            "address": submission["address"],
+            "medium": submission.pop("medium"),
+            "address": submission.pop("address"),
         }
-        del submission["medium"]
-        del submission["address"]
 
     # We've converted valid, legacy login submissions to an identifier. If the
     # dict still doesn't have an identifier, it's invalid

From b8f4b0c27c1ed2b7d1b429e69985afb6ee4b8b7f Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Tue, 16 Jun 2020 11:10:34 +0100
Subject: [PATCH 10/16] Use assert_param_in_dict

---
 synapse/handlers/auth.py | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index 57dead89b5a0..fd4e4148e3b5 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -39,6 +39,7 @@
 from synapse.handlers.ui_auth import INTERACTIVE_AUTH_CHECKERS
 from synapse.handlers.ui_auth.checkers import UserInteractiveAuthChecker
 from synapse.http.server import finish_request
+from synapse.http.servlet import assert_params_in_dict
 from synapse.http.site import SynapseRequest
 from synapse.logging.context import defer_to_thread
 from synapse.metrics.background_process_metrics import run_as_background_process
@@ -78,12 +79,7 @@ def client_dict_convert_legacy_fields_to_identifier(
 
     # We've converted valid, legacy login submissions to an identifier. If the
     # dict still doesn't have an identifier, it's invalid
-    if "identifier" not in submission:
-        raise SynapseError(
-            400,
-            "Missing 'identifier' parameter in login submission",
-            errcode=Codes.MISSING_PARAM,
-        )
+    assert_params_in_dict(submission, required=["identifier"])
 
     # Ensure the identifier has a type
     if "type" not in submission["identifier"]:

From efb5670845a87e69ff77b3991018b9af59e7dc68 Mon Sep 17 00:00:00 2001
From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Date: Tue, 16 Jun 2020 11:19:35 +0100
Subject: [PATCH 11/16] Update synapse/handlers/auth.py

Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
---
 synapse/handlers/auth.py | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index fd4e4148e3b5..e46918ee569e 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -56,11 +56,14 @@
 def client_dict_convert_legacy_fields_to_identifier(
     submission: Dict[str, Union[str, Dict]]
 ):
-    """Take a legacy-formatted login submission or User-Interactive Authentication dict and
-    updates it to feature an identifier dict instead.
-    Providing user-identifying information at the top-level of a login or UIA submission is
-    now deprecated and replaced with identifiers:
+    """
+    Convert a legacy-formatted login submission to an identifier dict.
+
+    Legacy login submissions (used in both login and user-interactive authentication)
+    provide user-identifying information at the top-level instead of in an `indentifier`
+    property. This is now deprecated and replaced with identifiers:
     https://matrix.org/docs/spec/client_server/r0.6.1#identifier-types
+
     Args:
         submission: The client dict to convert. Passed by reference and modified
     Raises:

From 53981c31e915239736accb50e9e99f168b03fee9 Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Tue, 16 Jun 2020 11:32:08 +0100
Subject: [PATCH 12/16] Change SynapseError comment

---
 synapse/handlers/auth.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index e46918ee569e..39f92a06fb5b 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -66,9 +66,9 @@ def client_dict_convert_legacy_fields_to_identifier(
 
     Args:
         submission: The client dict to convert. Passed by reference and modified
+
     Raises:
-        SynapseError: if the dict contains a "medium" parameter that is anything other than
-            "email"
+        SynapseError: If the format of the client dict is invalid
     """
     if "user" in submission:
         submission["identifier"] = {"type": "m.id.user", "user": submission.pop("user")}

From b1c0eb317880cf8ab67a5ba041f1eb3a266dd436 Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Tue, 16 Jun 2020 11:39:19 +0100
Subject: [PATCH 13/16] Docstring spacing

---
 synapse/handlers/auth.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index 39f92a06fb5b..f687a6803116 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -634,15 +634,19 @@ async def username_from_identifier(
         """Given a dictionary containing an identifier from a client, extract the
         possibly unqualified username of the user that it identifies. Does *not*
         guarantee that the user exists.
+
         If this identifier dict contains a threepid, we attempt to ask password
         auth providers about it or, failing that, look up an associated user in
         the database.
+
         Args:
             identifier: The identifier dictionary provided by the client
             password: The user provided password if one exists. Used for asking
                 password auth providers for usernames from 3pid+password combos.
+
         Returns:
             A username if one was found, or None otherwise
+
         Raises:
             SynapseError: If the identifier dict is invalid
         """

From d9277e94f3740b15f8bcede9695d83090a86a749 Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Tue, 16 Jun 2020 12:00:57 +0100
Subject: [PATCH 14/16] Don't lowercase medium in this PR

---
 synapse/rest/client/v1/login.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py
index 14189828ff28..602785fe2221 100644
--- a/synapse/rest/client/v1/login.py
+++ b/synapse/rest/client/v1/login.py
@@ -150,7 +150,7 @@ async def _do_other_login(self, login_submission):
         )
         if medium and address:
             self._failed_attempts_ratelimiter.ratelimit(
-                (medium.lower(), address.lower()), update=False
+                (medium, address.lower()), update=False
             )
 
         # Extract a localpart or user ID from the values in the identifier
@@ -163,7 +163,7 @@ async def _do_other_login(self, login_submission):
                 # The user attempted to login via threepid and failed
                 # Record this failed attempt
                 self._failed_attempts_ratelimiter.can_do_action(
-                    (medium.lower(), address.lower())
+                    (medium, address.lower())
                 )
 
                 raise LoginError(403, "Unauthorized threepid", errcode=Codes.FORBIDDEN)

From cb272bcfe89974ba575841e31b45b9edba293887 Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Thu, 25 Jun 2020 11:03:10 +0100
Subject: [PATCH 15/16] Explain why we rate-limit using a threepid

---
 synapse/rest/client/v1/login.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py
index 602785fe2221..98a3365f05fc 100644
--- a/synapse/rest/client/v1/login.py
+++ b/synapse/rest/client/v1/login.py
@@ -161,7 +161,8 @@ async def _do_other_login(self, login_submission):
         if not username:
             if medium and address:
                 # The user attempted to login via threepid and failed
-                # Record this failed attempt
+                # Record this failed attempt using the threepid as a key, as otherwise
+                # the user could bypass the ratelimiter by not providing a username
                 self._failed_attempts_ratelimiter.can_do_action(
                     (medium, address.lower())
                 )

From af21fbb33824977a392ffc4c7875b800901a3328 Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Thu, 25 Jun 2020 11:05:52 +0100
Subject: [PATCH 16/16] Simplify medium and address assignment

---
 synapse/rest/client/v1/login.py | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py
index 98a3365f05fc..b90ad6d79eb0 100644
--- a/synapse/rest/client/v1/login.py
+++ b/synapse/rest/client/v1/login.py
@@ -144,10 +144,8 @@ async def _do_other_login(self, login_submission):
 
         # Check whether this attempt uses a threepid, if so, check if our failed attempt
         # ratelimiter allows another attempt at this time
-        medium, address = (
-            login_submission.get("medium"),
-            login_submission.get("address"),
-        )
+        medium = login_submission.get("medium")
+        address = login_submission.get("address")
         if medium and address:
             self._failed_attempts_ratelimiter.ratelimit(
                 (medium, address.lower()), update=False