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

Drop support for calling /_matrix/client/v3/account/3pid/bind without an id_access_token #13239

Merged
merged 7 commits into from
Jul 12, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
1 change: 1 addition & 0 deletions changelog.d/13239.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Drop v1 3pid bind support.
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a few things called 3pid/bind here. I think it is worth being more precise.

Suggested change
Drop v1 3pid bind support.
Drop support for calling `/_matrix/client/v3/account/3pid/bind` without an `id_access_token`.

Additionally, this should be 13239.removal, not .misc.

25 changes: 8 additions & 17 deletions synapse/handlers/identity.py
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,6 @@ async def bind_threepid(
mxid: str,
id_server: str,
id_access_token: Optional[str] = None,
use_v2: bool = True,
) -> JsonDict:
"""Bind a 3PID to an identity server

Expand All @@ -175,10 +174,10 @@ async def bind_threepid(
id_server: The domain of the identity server to query
id_access_token: The access token to authenticate to the identity
server with, if necessary. Required if use_v2 is true
use_v2: Whether to use v2 Identity Service API endpoints. Defaults to True

Raises:
SynapseError: On any of the following conditions
- no id_access_token was supplied
- the supplied id_server is not a valid identity server name
- we failed to contact the supplied identity server

Expand All @@ -187,9 +186,10 @@ async def bind_threepid(
"""
logger.debug("Proxying threepid bind request for %s to %s", mxid, id_server)

# If an id_access_token is not supplied, force usage of v1
if id_access_token is None:
use_v2 = False
raise SynapseError(
400, "id_access_token is required", errcode=Codes.MISSING_PARAM
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This sort of validation should be handled at the rest level, see here:

id_access_token = body.get("id_access_token") # optional

Additionally, the current function should be changed so that id_access_token: str, rather than id_access_token: Optional[str] = None


if not valid_id_server_location(id_server):
raise SynapseError(
Expand All @@ -198,13 +198,9 @@ async def bind_threepid(
)

# Decide which API endpoint URLs to use
headers = {}
bind_data = {"sid": sid, "client_secret": client_secret, "mxid": mxid}
if use_v2:
bind_url = "https://%s/_matrix/identity/v2/3pid/bind" % (id_server,)
headers["Authorization"] = create_id_access_token_header(id_access_token) # type: ignore
else:
bind_url = "https://%s/_matrix/identity/api/v1/3pid/bind" % (id_server,)
bind_url = "https://%s/_matrix/identity/v2/3pid/bind" % (id_server,)
headers = {"Authorization": create_id_access_token_header(id_access_token)}

try:
# Use the blacklisting http client as this call is only to identity servers
Expand All @@ -223,20 +219,15 @@ async def bind_threepid(

return data
except HttpResponseException as e:
if e.code != 404 or not use_v2:
if e.code != 404:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this line wants to be deleted? I.e. treat a 404 like any other error?

logger.error("3PID bind failed with Matrix error: %r", e)
raise e.to_synapse_error()
except RequestTimedOutError:
raise SynapseError(500, "Timed out contacting identity server")
except CodeMessageException as e:
data = json_decoder.decode(e.msg) # XXX WAT?
return data

logger.info("Got 404 when POSTing JSON %s, falling back to v1 URL", bind_url)
res = await self.bind_threepid(
client_secret, sid, mxid, id_server, id_access_token, use_v2=False
)
return res
return {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think returning an empty dict in the 404 case makes sense here. Instead, I think we should treat 404 like all other errors, which will make this line unreachable.


async def try_unbind_threepid(self, mxid: str, threepid: dict) -> bool:
"""Attempt to remove a 3PID from an identity server, or if one is not provided, all
Expand Down