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

Call appservices on modern paths, falling back to legacy paths. #15317

Merged
merged 10 commits into from
Apr 3, 2023
1 change: 1 addition & 0 deletions changelog.d/15317.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a long-standing bug that Synpase only used the [legacy appservice routes](https://spec.matrix.org/v1.6/application-service-api/#legacy-routes).
16 changes: 16 additions & 0 deletions docs/upgrade.md
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,22 @@ process, for example:
dpkg -i matrix-synapse-py3_1.3.0+stretch1_amd64.deb
```

# Upgrading to v1.81.0

## Application service path & authentication deprecations

Synapse now attempts the versioned appservice paths before falling back to the
[legacy paths](https://spec.matrix.org/v1.6/application-service-api/#legacy-routes).
Usage of the legacy routes should be considered deprecated.

Additionally, Synapse has supported sending the application service access token
via [the `Authorization` header](https://spec.matrix.org/v1.6/application-service-api/#authorization)
since v1.70.0. For backwards compatibility it is *also* sent as the `access_token`
query parameter. This is insecure and should be considered deprecated.
clokep marked this conversation as resolved.
Show resolved Hide resolved

A future version of Synapse (v1.88.0 or later) will remove support for legacy
application service routes and query parameter authorization.

# Upgrading to v1.80.0

## Reporting events error code change
Expand Down
131 changes: 91 additions & 40 deletions synapse/appservice/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,20 @@
from typing import (
TYPE_CHECKING,
Any,
Awaitable,
Callable,
Dict,
Iterable,
List,
Mapping,
Optional,
Sequence,
Tuple,
TypeVar,
)

from prometheus_client import Counter
from typing_extensions import TypeGuard
from typing_extensions import Concatenate, ParamSpec, TypeGuard

from synapse.api.constants import EventTypes, Membership, ThirdPartyEntityKind
from synapse.api.errors import CodeMessageException
Expand Down Expand Up @@ -78,7 +81,11 @@
HOUR_IN_MS = 60 * 60 * 1000


APP_SERVICE_PREFIX = "/_matrix/app/unstable"
APP_SERVICE_PREFIX = "/_matrix/app/v1"
APP_SERVICE_UNSTABLE_PREFIX = "/_matrix/app/unstable"

P = ParamSpec("P")
R = TypeVar("R")


def _is_valid_3pe_metadata(info: JsonDict) -> bool:
Expand Down Expand Up @@ -121,17 +128,58 @@ def __init__(self, hs: "HomeServer"):
hs.get_clock(), "as_protocol_meta", timeout_ms=HOUR_IN_MS
)

async def _send_with_fallbacks(
self,
service: "ApplicationService",
prefixes: List[str],
clokep marked this conversation as resolved.
Show resolved Hide resolved
path: str,
func: Callable[Concatenate[str, P], Awaitable[R]],
*args: P.args,
**kwargs: P.kwargs,
) -> R:
"""
Attempt to call an application service with multiple paths, falling back
until one succeeds.

Args:
service: The appliacation service, this provides the base URL.
prefixes: A last of paths to try in order for the requests.
path: A suffix to append to each prefix.
func: The function to call, the first argument will be the full
endpoint to fetch. Other arguments are provided by args/kwargs.

Returns:
The return value of func.
"""
while prefixes:
prefix = prefixes.pop(0)
uri = f"{service.url}{prefix}{path}"
try:
return await func(uri, *args, **kwargs)
except CodeMessageException as e:
# If an error is received that is due to an unrecognised path,
# fallback to next path (if one exists). Otherwise, consider it
# a legitimate error and raise.
if prefixes and (e.code == 404 or e.code == 405):
continue
clokep marked this conversation as resolved.
Show resolved Hide resolved
raise

# The function should always exit via the return or raise above this.
raise RuntimeError("Unexpected fallback behaviour. This should never be seen.")

async def query_user(self, service: "ApplicationService", user_id: str) -> bool:
if service.url is None:
return False

# This is required by the configuration.
assert service.hs_token is not None

uri = service.url + ("/users/%s" % urllib.parse.quote(user_id))
try:
response = await self.get_json(
uri,
response = await self._send_with_fallbacks(
service,
[APP_SERVICE_PREFIX, ""],
f"/users/{urllib.parse.quote(user_id)}",
self.get_json,
{"access_token": service.hs_token},
headers={"Authorization": [f"Bearer {service.hs_token}"]},
)
Expand All @@ -140,9 +188,9 @@ async def query_user(self, service: "ApplicationService", user_id: str) -> bool:
except CodeMessageException as e:
if e.code == 404:
return False
logger.warning("query_user to %s received %s", uri, e.code)
logger.warning("query_user to %s received %s", service.url, e.code)
except Exception as ex:
logger.warning("query_user to %s threw exception %s", uri, ex)
logger.warning("query_user to %s threw exception %s", service.url, ex)
return False

async def query_alias(self, service: "ApplicationService", alias: str) -> bool:
Expand All @@ -152,21 +200,23 @@ async def query_alias(self, service: "ApplicationService", alias: str) -> bool:
# This is required by the configuration.
assert service.hs_token is not None

uri = service.url + ("/rooms/%s" % urllib.parse.quote(alias))
try:
response = await self.get_json(
uri,
response = await self._send_with_fallbacks(
service,
[APP_SERVICE_PREFIX, ""],
f"/rooms/{urllib.parse.quote(alias)}",
self.get_json,
{"access_token": service.hs_token},
headers={"Authorization": [f"Bearer {service.hs_token}"]},
)
if response is not None: # just an empty json object
return True
except CodeMessageException as e:
logger.warning("query_alias to %s received %s", uri, e.code)
logger.warning("query_alias to %s received %s", service.url, e.code)
if e.code == 404:
return False
except Exception as ex:
logger.warning("query_alias to %s threw exception %s", uri, ex)
logger.warning("query_alias to %s threw exception %s", service.url, ex)
return False

async def query_3pe(
Expand All @@ -188,25 +238,24 @@ async def query_3pe(
# This is required by the configuration.
assert service.hs_token is not None

uri = "%s%s/thirdparty/%s/%s" % (
service.url,
APP_SERVICE_PREFIX,
kind,
urllib.parse.quote(protocol),
)
try:
args: Mapping[Any, Any] = {
**fields,
b"access_token": service.hs_token,
}
response = await self.get_json(
uri,
response = await self._send_with_fallbacks(
service,
[APP_SERVICE_PREFIX, APP_SERVICE_UNSTABLE_PREFIX],
f"/thirdparty/{kind}/{urllib.parse.quote(protocol)}",
self.get_json,
args=args,
headers={"Authorization": [f"Bearer {service.hs_token}"]},
)
if not isinstance(response, list):
logger.warning(
"query_3pe to %s returned an invalid response %r", uri, response
"query_3pe to %s returned an invalid response %r",
service.url,
response,
)
return []

Expand All @@ -216,12 +265,12 @@ async def query_3pe(
ret.append(r)
else:
logger.warning(
"query_3pe to %s returned an invalid result %r", uri, r
"query_3pe to %s returned an invalid result %r", service.url, r
)

return ret
except Exception as ex:
logger.warning("query_3pe to %s threw exception %s", uri, ex)
logger.warning("query_3pe to %s threw exception %s", service.url, ex)
return []

async def get_3pe_protocol(
Expand All @@ -233,21 +282,20 @@ async def get_3pe_protocol(
async def _get() -> Optional[JsonDict]:
# This is required by the configuration.
assert service.hs_token is not None
uri = "%s%s/thirdparty/protocol/%s" % (
service.url,
APP_SERVICE_PREFIX,
urllib.parse.quote(protocol),
)
try:
info = await self.get_json(
uri,
info = await self._send_with_fallbacks(
service,
[APP_SERVICE_PREFIX, APP_SERVICE_UNSTABLE_PREFIX],
f"/thirdparty/protocol/{urllib.parse.quote(protocol)}",
self.get_json,
{"access_token": service.hs_token},
headers={"Authorization": [f"Bearer {service.hs_token}"]},
)

if not _is_valid_3pe_metadata(info):
logger.warning(
"query_3pe_protocol to %s did not return a valid result", uri
"query_3pe_protocol to %s did not return a valid result",
service.url,
)
return None

Expand All @@ -260,7 +308,9 @@ async def _get() -> Optional[JsonDict]:

return info
except Exception as ex:
logger.warning("query_3pe_protocol to %s threw exception %s", uri, ex)
logger.warning(
"query_3pe_protocol to %s threw exception %s", service.url, ex
)
return None

key = (service.id, protocol)
Expand All @@ -274,7 +324,7 @@ async def ping(self, service: "ApplicationService", txn_id: Optional[str]) -> No
assert service.hs_token is not None

await self.post_json_get_json(
uri=service.url + "/_matrix/app/unstable/fi.mau.msc2659/ping",
uri=f"{service.url}{APP_SERVICE_UNSTABLE_PREFIX}/fi.mau.msc2659/ping",
post_json={"transaction_id": txn_id},
headers={"Authorization": [f"Bearer {service.hs_token}"]},
)
Expand Down Expand Up @@ -318,8 +368,6 @@ async def push_bulk(
)
txn_id = 0

uri = service.url + ("/transactions/%s" % urllib.parse.quote(str(txn_id)))

# Never send ephemeral events to appservices that do not support it
body: JsonDict = {"events": serialized_events}
if service.supports_ephemeral:
Expand Down Expand Up @@ -351,16 +399,19 @@ async def push_bulk(
}

try:
await self.put_json(
uri=uri,
await self._send_with_fallbacks(
service,
[APP_SERVICE_PREFIX, ""],
f"/transactions/{urllib.parse.quote(str(txn_id))}",
self.put_json,
json_body=body,
args={"access_token": service.hs_token},
headers={"Authorization": [f"Bearer {service.hs_token}"]},
)
if logger.isEnabledFor(logging.DEBUG):
logger.debug(
"push_bulk to %s succeeded! events=%s",
uri,
service.url,
[event.get("event_id") for event in events],
)
sent_transactions_counter.labels(service.id).inc()
Expand All @@ -371,15 +422,15 @@ async def push_bulk(
except CodeMessageException as e:
logger.warning(
"push_bulk to %s received code=%s msg=%s",
uri,
service.url,
e.code,
e.msg,
exc_info=logger.isEnabledFor(logging.DEBUG),
)
except Exception as ex:
logger.warning(
"push_bulk to %s threw exception(%s) %s args=%s",
uri,
service.url,
type(ex).__name__,
ex,
ex.args,
Expand Down
57 changes: 55 additions & 2 deletions tests/appservice/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

from twisted.test.proto_helpers import MemoryReactor

from synapse.api.errors import CodeMessageException
from synapse.appservice import ApplicationService
from synapse.server import HomeServer
from synapse.types import JsonDict
Expand Down Expand Up @@ -64,8 +65,8 @@ def test_query_3pe_authenticates_token(self) -> None:
}
]

URL_USER = f"{URL}/_matrix/app/unstable/thirdparty/user/{PROTOCOL}"
URL_LOCATION = f"{URL}/_matrix/app/unstable/thirdparty/location/{PROTOCOL}"
URL_USER = f"{URL}/_matrix/app/v1/thirdparty/user/{PROTOCOL}"
URL_LOCATION = f"{URL}/_matrix/app/v1/thirdparty/location/{PROTOCOL}"

self.request_url = None

Expand Down Expand Up @@ -106,6 +107,58 @@ async def get_json(
self.assertEqual(self.request_url, URL_LOCATION)
self.assertEqual(result, SUCCESS_RESULT_LOCATION)

def test_fallback(self) -> None:
"""
Tests that Synapse fallsback to legacy URLs.
"""
SUCCESS_RESULT_USER = [
{
"protocol": PROTOCOL,
"userid": "@a:user",
"fields": {
"more": "fields",
},
}
]

URL_USER = f"{URL}/_matrix/app/v1/thirdparty/user/{PROTOCOL}"
FALLBACK_URL_USER = f"{URL}/_matrix/app/unstable/thirdparty/user/{PROTOCOL}"

self.request_url = None
self.v1_seen = False

async def get_json(
url: str,
args: Mapping[Any, Any],
headers: Mapping[Union[str, bytes], Sequence[Union[str, bytes]]],
) -> List[JsonDict]:
# Ensure the access token is passed as both a header and query arg.
if not headers.get("Authorization") or not args.get(b"access_token"):
raise RuntimeError("Access token not provided")

self.assertEqual(headers.get("Authorization"), [f"Bearer {TOKEN}"])
self.assertEqual(args.get(b"access_token"), TOKEN)
self.request_url = url
if url == URL_USER:
self.v1_seen = True
raise CodeMessageException(404, "NOT_FOUND")
elif url == FALLBACK_URL_USER:
return SUCCESS_RESULT_USER
else:
raise RuntimeError(
"URL provided was invalid. This should never be seen."
)

# We assign to a method, which mypy doesn't like.
self.api.get_json = Mock(side_effect=get_json) # type: ignore[assignment]

result = self.get_success(
self.api.query_3pe(self.service, "user", PROTOCOL, {b"some": [b"field"]})
)
self.assertTrue(self.v1_seen)
self.assertEqual(self.request_url, FALLBACK_URL_USER)
self.assertEqual(result, SUCCESS_RESULT_USER)

def test_claim_keys(self) -> None:
"""
Tests that the /keys/claim response is properly parsed for missing
Expand Down