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

Printf debugging for MSISDN validation #11882

Merged
merged 5 commits into from
Feb 8, 2022
Merged

Printf debugging for MSISDN validation #11882

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
574b4ff
Printf-debugging around MSDIDN validation
Feb 1, 2022
cd51d5c
Why do I need to run `black` on another file?
Feb 1, 2022
f839ea1
Tweak logging
Feb 3, 2022
3a1a053
Sod off mypy
Feb 3, 2022
b447c42
Merge remote-tracking branch 'origin/matrix-org-hotfixes' into dmr/ms…
Feb 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions synapse/rest/client/account.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not so much used to deduplicate but rather to make sure the next request(s) come from the client that started the process. I'm not sure what amount of harm can be done if someone gets a hold of someone else's client secret, but my paranoid self would prefer being cautious and not logging it if we don't need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I didn't grok that it was to be sent in another request.

logger.info("Request to verify ownership of %s: %s", msisdn, body)

if not check_3pid_allowed(self.hs, "msisdn", msisdn):
raise SynapseError(
403,
Expand All @@ -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:
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look like it's in the right place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, looks like a copy paste fail. thanks.


return 200, ret

Expand Down
6 changes: 2 additions & 4 deletions synapse/storage/database.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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
Expand Down