From bb9f3a135fae5d06615452c7bf7ff29db97ff182 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 26 Aug 2020 16:14:39 +0100 Subject: [PATCH 1/9] Move login_submission_legacy_convert, login_id_thirdparty_from_phone methods We'll be using them in AuthHandler in the separate PR, so move them to AuthHandler instead of importing them from LoginRestServlet --- synapse/handlers/auth.py | 45 ++++++++++++++++++++++++++++++ synapse/rest/client/v1/login.py | 49 +++------------------------------ 2 files changed, 49 insertions(+), 45 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 654f58ddaefe..fc032fa9521d 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -44,6 +44,7 @@ from synapse.module_api import ModuleApi from synapse.types import Requester, UserID from synapse.util import stringutils as stringutils +from synapse.util.msisdn import phone_number_to_msisdn from synapse.util.threepids import canonicalise_email from ._base import BaseHandler @@ -51,6 +52,50 @@ 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 ( + # The specification requires a "phone" field, while Synapse used to require a "number" + # field. Accept both for backwards compatibility. + "phone" not in identifier + and "number" not in identifier + ): + raise SynapseError(400, "Invalid phone-type identifier") + + # Accept both "phone" and "number" as valid keys in m.id.phone + phone_number = identifier.get("phone", identifier["number"]) + + msisdn = phone_number_to_msisdn(identifier["country"], phone_number) + + return {"type": "m.id.thirdparty", "medium": "msisdn", "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 379f668d6f8a..208504fb062e 100644 --- a/synapse/rest/client/v1/login.py +++ b/synapse/rest/client/v1/login.py @@ -18,6 +18,10 @@ from synapse.api.errors import Codes, LoginError, SynapseError from synapse.api.ratelimiting import Ratelimiter +from synapse.handlers.auth import ( + login_id_thirdparty_from_phone, + login_submission_legacy_convert, +) from synapse.http.server import finish_request from synapse.http.servlet import ( RestServlet, @@ -28,56 +32,11 @@ from synapse.rest.client.v2_alpha._base import client_patterns from synapse.rest.well_known import WellKnownBuilder from synapse.types import JsonDict, UserID -from synapse.util.msisdn import phone_number_to_msisdn from synapse.util.threepids import canonicalise_email 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 ( - # The specification requires a "phone" field, while Synapse used to require a "number" - # field. Accept both for backwards compatibility. - "phone" not in identifier - and "number" not in identifier - ): - raise SynapseError(400, "Invalid phone-type identifier") - - # Accept both "phone" and "number" as valid keys in m.id.phone - phone_number = identifier.get("phone", identifier["number"]) - - msisdn = phone_number_to_msisdn(identifier["country"], phone_number) - - return {"type": "m.id.thirdparty", "medium": "msisdn", "address": msisdn} - - class LoginRestServlet(RestServlet): PATTERNS = client_patterns("/login$", v1=True) CAS_TYPE = "m.login.cas" From 6cd0a4420245ed1df4b31a8c3d021edbb5fb3746 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 26 Aug 2020 16:32:44 +0100 Subject: [PATCH 2/9] Refactor and rename login_submission_legacy_convert Additionally moves some checks that were in `LoginRestServlet._do_other_login` to `convert_client_dict_legacy_fields_to_identifier` as they would be applicable to any other callers of this function. Additionally, there is a functional change here, in that we now return M_MISSING_PARAM's instead of M_UNKNOWN errcodes for when `identifier` or `type` parameters are missing. --- synapse/handlers/auth.py | 40 ++++++++++++++++++++++++--------- synapse/rest/client/v1/login.py | 10 ++------- 2 files changed, 32 insertions(+), 18 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index fc032fa9521d..606981e2b605 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -38,6 +38,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, respond_with_html +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 @@ -52,24 +53,43 @@ logger = logging.getLogger(__name__) -def login_submission_legacy_convert(submission): +def convert_client_dict_legacy_fields_to_identifier( + submission: Dict[str, Union[str, Dict]] +): """ - If the input login submission is an old style object - (ie. with top-level user / medium / address) convert it - to a typed object. + 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. + + 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: + SynapseError: If the format of the client dict is invalid """ 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 + assert_params_in_dict(submission, required=["identifier"]) + + # 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_thirdparty_from_phone(identifier): diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py index 208504fb062e..e09624fba3b1 100644 --- a/synapse/rest/client/v1/login.py +++ b/synapse/rest/client/v1/login.py @@ -19,8 +19,8 @@ from synapse.api.errors import Codes, LoginError, SynapseError from synapse.api.ratelimiting import Ratelimiter from synapse.handlers.auth import ( + convert_client_dict_legacy_fields_to_identifier, login_id_thirdparty_from_phone, - login_submission_legacy_convert, ) from synapse.http.server import finish_request from synapse.http.servlet import ( @@ -153,14 +153,8 @@ async def _do_other_login(self, login_submission: JsonDict) -> Dict[str, str]: login_submission.get("address"), login_submission.get("user"), ) - login_submission_legacy_convert(login_submission) - - if "identifier" not in login_submission: - raise SynapseError(400, "Missing param: identifier") - + convert_client_dict_legacy_fields_to_identifier(login_submission) 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": From ed30684765eb7facd332fedb9d6ba9c7a0c3bb22 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 26 Aug 2020 16:59:42 +0100 Subject: [PATCH 3/9] Refactor and rename login_id_thirdparty_from_phone This function changes the errcode of a SynapseError from M_UNKNOWN to M_INVALID_PARAM. I think it fits better, but this is changing behaviour... Also corrected docstring. There is no 'm.id.threepid' identifier type. --- synapse/handlers/auth.py | 26 ++++++++++++++++++++------ synapse/rest/client/v1/login.py | 4 ++-- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 606981e2b605..cc44c88ac6c8 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -92,28 +92,42 @@ def convert_client_dict_legacy_fields_to_identifier( ) -def login_id_thirdparty_from_phone(identifier): +def login_id_phone_to_thirdparty(identifier: Dict[str, str]) -> Dict[str, str]: """ - Convert a phone login identifier type to a generic threepid identifier + Convert a phone login identifier type to a generic threepid identifier. + Args: - identifier(dict): Login identifier dict of type 'm.id.phone' + identifier: Login identifier dict of type 'm.id.phone' - Returns: Login identifier dict of type 'm.id.threepid' + Returns: + An equivalent m.id.thirdparty identifier dict """ + if "type" not in identifier: + raise SynapseError( + 400, "Invalid phone-type identifier", errcode=Codes.MISSING_PARAM + ) + if "country" not in identifier or ( # The specification requires a "phone" field, while Synapse used to require a "number" # field. Accept both for backwards compatibility. "phone" not in identifier and "number" not in identifier ): - raise SynapseError(400, "Invalid phone-type 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["number"]) + # Convert user-provided phone number to a consistent representation msisdn = phone_number_to_msisdn(identifier["country"], phone_number) - return {"type": "m.id.thirdparty", "medium": "msisdn", "address": msisdn} + return { + "type": "m.id.thirdparty", + "medium": "msisdn", + "address": msisdn, + } class AuthHandler(BaseHandler): diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py index e09624fba3b1..87a42bcb2f77 100644 --- a/synapse/rest/client/v1/login.py +++ b/synapse/rest/client/v1/login.py @@ -20,7 +20,7 @@ from synapse.api.ratelimiting import Ratelimiter from synapse.handlers.auth import ( convert_client_dict_legacy_fields_to_identifier, - login_id_thirdparty_from_phone, + login_id_phone_to_thirdparty, ) from synapse.http.server import finish_request from synapse.http.servlet import ( @@ -158,7 +158,7 @@ async def _do_other_login(self, login_submission: JsonDict) -> Dict[str, str]: # 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 69f2b3085d55f9910756f8b1aadd631e695d952f Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 26 Aug 2020 17:38:01 +0100 Subject: [PATCH 4/9] Changelog --- changelog.d/8182.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/8182.misc diff --git a/changelog.d/8182.misc b/changelog.d/8182.misc new file mode 100644 index 000000000000..4fcdf1c45213 --- /dev/null +++ b/changelog.d/8182.misc @@ -0,0 +1 @@ +Refactor some of `LoginRestServlet`'s helper methods, and move them to `AuthHandler` for easier reuse. \ No newline at end of file From b768dd0a824cf2d07840d6d66c2055c68021c01e Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 27 Aug 2020 11:52:14 +0100 Subject: [PATCH 5/9] JsonDicts --- synapse/handlers/auth.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index cc44c88ac6c8..4b61dd7888f0 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -43,7 +43,7 @@ from synapse.logging.context import defer_to_thread from synapse.metrics.background_process_metrics import run_as_background_process from synapse.module_api import ModuleApi -from synapse.types import Requester, UserID +from synapse.types import JsonDict, Requester, UserID from synapse.util import stringutils as stringutils from synapse.util.msisdn import phone_number_to_msisdn from synapse.util.threepids import canonicalise_email @@ -53,9 +53,7 @@ logger = logging.getLogger(__name__) -def convert_client_dict_legacy_fields_to_identifier( - submission: Dict[str, Union[str, Dict]] -): +def convert_client_dict_legacy_fields_to_identifier(submission: JsonDict): """ Convert a legacy-formatted login submission to an identifier dict. @@ -92,7 +90,7 @@ def convert_client_dict_legacy_fields_to_identifier( ) -def login_id_phone_to_thirdparty(identifier: Dict[str, str]) -> Dict[str, str]: +def login_id_phone_to_thirdparty(identifier: JsonDict) -> Dict[str, str]: """ Convert a phone login identifier type to a generic threepid identifier. From 3a98400647e8707de596d498f9ecd4ebc9dc99e9 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 27 Aug 2020 11:53:13 +0100 Subject: [PATCH 6/9] Remove duplicate 'type' param check --- synapse/handlers/auth.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 4b61dd7888f0..b7add8007b67 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -100,11 +100,6 @@ def login_id_phone_to_thirdparty(identifier: JsonDict) -> Dict[str, str]: Returns: An equivalent m.id.thirdparty identifier dict """ - if "type" not in identifier: - raise SynapseError( - 400, "Invalid phone-type identifier", errcode=Codes.MISSING_PARAM - ) - if "country" not in identifier or ( # The specification requires a "phone" field, while Synapse used to require a "number" # field. Accept both for backwards compatibility. From 11329e8306ae5f78727ec3422d7db6f09a72b444 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 27 Aug 2020 12:13:18 +0100 Subject: [PATCH 7/9] Return identifier instead of modified submission dict --- synapse/handlers/auth.py | 40 +++++++++++++++++++++------------ synapse/rest/client/v1/login.py | 3 +-- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index b7add8007b67..d783e66b09b8 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -38,7 +38,6 @@ from synapse.handlers.ui_auth import INTERACTIVE_AUTH_CHECKERS from synapse.handlers.ui_auth.checkers import UserInteractiveAuthChecker from synapse.http.server import finish_request, respond_with_html -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 @@ -53,42 +52,55 @@ logger = logging.getLogger(__name__) -def convert_client_dict_legacy_fields_to_identifier(submission: JsonDict): +def convert_client_dict_legacy_fields_to_identifier(submission: JsonDict) -> Dict[str, str]: """ 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. - This is now deprecated and replaced with identifiers: + These are 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 + submission: The client dict to convert + + Returns: + The matching identifier dict Raises: SynapseError: If the format of the client dict is invalid """ - if "user" in submission: - submission["identifier"] = {"type": "m.id.user", "user": submission.pop("user")} - - if "medium" in submission and "address" in submission: - submission["identifier"] = { + identifier = submission.get("identifier", {}) + + # Generate an m.id.user identifier if "user" parameter is present + user = submission.get("user") + if user: + identifier = {"type": "m.id.user", "user": user} + + # Generate an m.id.thirdparty identifier if "medium" and "address" parameters are present + medium = submission.get("medium") + address = submission.get("address") + if medium and address: + identifier = { "type": "m.id.thirdparty", - "medium": submission.pop("medium"), - "address": submission.pop("address"), + "medium": medium, + "address": address, } # We've converted valid, legacy login submissions to an identifier. If the - # dict still doesn't have an identifier, it's invalid - assert_params_in_dict(submission, required=["identifier"]) + # submission still doesn't have an identifier, it's invalid + if not identifier: + raise SynapseError(400, "Invalid login submission", Codes.INVALID_PARAM) # Ensure the identifier has a type - if "type" not in submission["identifier"]: + if "type" not in ["identifier"]: raise SynapseError( 400, "'identifier' dict has no key 'type'", errcode=Codes.MISSING_PARAM, ) + return identifier + def login_id_phone_to_thirdparty(identifier: JsonDict) -> Dict[str, str]: """ diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py index 87a42bcb2f77..a14618ac84fb 100644 --- a/synapse/rest/client/v1/login.py +++ b/synapse/rest/client/v1/login.py @@ -153,8 +153,7 @@ async def _do_other_login(self, login_submission: JsonDict) -> Dict[str, str]: login_submission.get("address"), login_submission.get("user"), ) - convert_client_dict_legacy_fields_to_identifier(login_submission) - identifier = login_submission["identifier"] + identifier = convert_client_dict_legacy_fields_to_identifier(login_submission) # convert phone type identifiers to generic threepids if identifier["type"] == "m.id.phone": From e3e48f07906129277f0bbea57844fc1df285b02b Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 27 Aug 2020 13:30:41 +0100 Subject: [PATCH 8/9] lint --- synapse/handlers/auth.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index d783e66b09b8..8691bd3a4e7f 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -52,7 +52,9 @@ logger = logging.getLogger(__name__) -def convert_client_dict_legacy_fields_to_identifier(submission: JsonDict) -> Dict[str, str]: +def convert_client_dict_legacy_fields_to_identifier( + submission: JsonDict, +) -> Dict[str, str]: """ Convert a legacy-formatted login submission to an identifier dict. From 47e8d841140330c0ccbf15f27a26f0454c9a12bc Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 27 Aug 2020 13:47:28 +0100 Subject: [PATCH 9/9] Fix typo --- synapse/handlers/auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 8691bd3a4e7f..f0b0a4d76ab7 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -96,7 +96,7 @@ def convert_client_dict_legacy_fields_to_identifier( raise SynapseError(400, "Invalid login submission", Codes.INVALID_PARAM) # Ensure the identifier has a type - if "type" not in ["identifier"]: + if "type" not in identifier: raise SynapseError( 400, "'identifier' dict has no key 'type'", errcode=Codes.MISSING_PARAM, )