From 574b4ffddf17006e8ec054703648e44b83042422 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 1 Feb 2022 19:43:22 +0000 Subject: [PATCH 1/4] Printf-debugging around MSDIDN validation --- synapse/rest/client/account.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/synapse/rest/client/account.py b/synapse/rest/client/account.py index 6b272658fc3c..9c46aae6bd55 100644 --- a/synapse/rest/client/account.py +++ b/synapse/rest/client/account.py @@ -468,6 +468,12 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: msisdn = phone_number_to_msisdn(country, phone_number) + # Didn't like the sound of logging `client_secret`, but the spec says it is + # "A unique string generated by the client, and used to identify the validation + # attempt." I.e. something to facilitate deduplication. I don't think it's a + # sensitive secret per se. + logger.info("Request to verify ownership of %s: %s", msisdn, body) + if not check_3pid_allowed(self.hs, "msisdn", msisdn): raise SynapseError( 403, @@ -494,6 +500,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: await self.hs.get_clock().sleep(random.randint(1, 10) / 10) return 200, {"sid": random_string(16)} + logger.info("MSISDN %s is already in use by %s", msisdn, existing_user_id) raise SynapseError(400, "MSISDN is already in use", Codes.THREEPID_IN_USE) if not self.hs.config.registration.account_threepid_delegate_msisdn: @@ -518,6 +525,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: threepid_send_requests.labels(type="msisdn", reason="add_threepid").observe( send_attempt ) + logger.info("MSISDN %s is already in use by %s", msisdn, existing_user_id) return 200, ret From cd51d5c607cf866f24f9db6e94f410783578feb7 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 1 Feb 2022 19:51:48 +0000 Subject: [PATCH 2/4] Why do I need to run `black` on another file? --- synapse/storage/database.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/synapse/storage/database.py b/synapse/storage/database.py index f1d975a8ff9a..29423db86037 100644 --- a/synapse/storage/database.py +++ b/synapse/storage/database.py @@ -97,7 +97,7 @@ def __init__(self, connection): self._synapse_parent_context = None def commit(self, *args, **kwargs): - with LoggingContext("db_commit", parent_context = self._synapse_parent_context): + with LoggingContext("db_commit", parent_context=self._synapse_parent_context): with opentracing.start_active_span("db.conn.commit"): self._connection.commit(*args, **kwargs) @@ -120,9 +120,7 @@ def _on_new_connection(conn): # etc. with LoggingContext("db.on_new_connection"): engine.on_new_connection( - LoggingDatabaseConnection( - conn, engine, "on_new_connection" - ) + LoggingDatabaseConnection(conn, engine, "on_new_connection") ) # HACK Patch the connection's commit function so that we can see From f839ea1b69eefa4c77785ba55d01ead174a6b8b9 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 3 Feb 2022 18:34:44 +0000 Subject: [PATCH 3/4] Tweak logging --- synapse/rest/client/account.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/synapse/rest/client/account.py b/synapse/rest/client/account.py index 9c46aae6bd55..d9cb55e35b87 100644 --- a/synapse/rest/client/account.py +++ b/synapse/rest/client/account.py @@ -467,12 +467,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: next_link = body.get("next_link") # Optional param msisdn = phone_number_to_msisdn(country, phone_number) - - # Didn't like the sound of logging `client_secret`, but the spec says it is - # "A unique string generated by the client, and used to identify the validation - # attempt." I.e. something to facilitate deduplication. I don't think it's a - # sensitive secret per se. - logger.info("Request to verify ownership of %s: %s", msisdn, body) + logger.info("Request #%s to verify ownership of %s", send_attempt, msisdn) if not check_3pid_allowed(self.hs, "msisdn", msisdn): raise SynapseError( @@ -525,7 +520,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: threepid_send_requests.labels(type="msisdn", reason="add_threepid").observe( send_attempt ) - logger.info("MSISDN %s is already in use by %s", msisdn, existing_user_id) + logger.info("MSISDN %s: got response from identity server: %s", msisdn, ret) return 200, ret From 3a1a0532489feed82c929773ea88fc9019e7169c Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 3 Feb 2022 18:36:01 +0000 Subject: [PATCH 4/4] Sod off mypy --- synapse/handlers/message.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 4411d901ac41..8f44af2d9264 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -277,8 +277,8 @@ async def get_joined_members(self, requester: Requester, room_id: str) -> dict: # If this is an AS, double check that they are allowed to see the members. # This can either be because the AS user is in the room or because there # is a user in the room that the AS is "interested in" - if False and requester.app_service and user_id not in users_with_profile: - for uid in users_with_profile: + if False and requester.app_service and user_id not in users_with_profile: # type: ignore[unreachable] + for uid in users_with_profile: # type: ignore[unreachable] if requester.app_service.is_interested_in_user(uid): break else: