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

Remove support for legacy application service paths #15964

Merged
merged 4 commits into from
Jul 26, 2023
Merged

Remove support for legacy application service paths #15964

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4e202aa
remove legacy paths
H-Shay Jul 19, 2023
10f2696
newsfragment
H-Shay Jul 19, 2023
447d666
remove _send_with_fallbacks
H-Shay Jul 24, 2023
3274e51
add appservice prefix back in as constant
H-Shay Jul 26, 2023
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/15964.removal
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Remove support for legacy application service paths.
82 changes: 11 additions & 71 deletions synapse/appservice/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@
from typing import (
TYPE_CHECKING,
Any,
Awaitable,
Callable,
Dict,
Iterable,
List,
Expand All @@ -30,7 +28,7 @@
)

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

from synapse.api.constants import EventTypes, Membership, ThirdPartyEntityKind
from synapse.api.errors import CodeMessageException, HttpResponseException
Expand Down Expand Up @@ -80,9 +78,7 @@

HOUR_IN_MS = 60 * 60 * 1000


APP_SERVICE_PREFIX = "/_matrix/app/v1"
Copy link
Member

Choose a reason for hiding this comment

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

Keeping the prefix I think is still useful / more consistent with what we do in other places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For future reference, besides consistency are there any heuristics I should keep in mind about when to use a constant vs when to hardcode something like this case here?

Copy link
Member

Choose a reason for hiding this comment

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

Usually if you're putting the same text several times it is worth having a constant.

APP_SERVICE_UNSTABLE_PREFIX = "/_matrix/app/unstable"

P = ParamSpec("P")
R = TypeVar("R")
Expand Down Expand Up @@ -128,47 +124,6 @@ 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],
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.
"""
for i, prefix in enumerate(prefixes, start=1):
uri = f"{service.url}{prefix}{path}"
try:
return await func(uri, *args, **kwargs)
except HttpResponseException 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 i < len(prefixes) and is_unknown_endpoint(e):
continue
raise
except Exception:
# Unexpected exceptions get sent to the caller.
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
Expand All @@ -177,11 +132,8 @@ async def query_user(self, service: "ApplicationService", user_id: str) -> bool:
assert service.hs_token is not None

try:
response = await self._send_with_fallbacks(
service,
[APP_SERVICE_PREFIX, ""],
f"/users/{urllib.parse.quote(user_id)}",
self.get_json,
response = await self.get_json(
f"{service.url}{APP_SERVICE_PREFIX}/users/{urllib.parse.quote(user_id)}",
{"access_token": service.hs_token},
headers={"Authorization": [f"Bearer {service.hs_token}"]},
)
Expand All @@ -203,11 +155,8 @@ async def query_alias(self, service: "ApplicationService", alias: str) -> bool:
assert service.hs_token is not None

try:
response = await self._send_with_fallbacks(
service,
[APP_SERVICE_PREFIX, ""],
f"/rooms/{urllib.parse.quote(alias)}",
self.get_json,
response = await self.get_json(
f"{service.url}{APP_SERVICE_PREFIX}/rooms/{urllib.parse.quote(alias)}",
{"access_token": service.hs_token},
headers={"Authorization": [f"Bearer {service.hs_token}"]},
)
Expand Down Expand Up @@ -245,11 +194,8 @@ async def query_3pe(
**fields,
b"access_token": service.hs_token,
}
response = await self._send_with_fallbacks(
service,
[APP_SERVICE_PREFIX, APP_SERVICE_UNSTABLE_PREFIX],
f"/thirdparty/{kind}/{urllib.parse.quote(protocol)}",
self.get_json,
response = await self.get_json(
f"{service.url}{APP_SERVICE_PREFIX}/thirdparty/{kind}/{urllib.parse.quote(protocol)}",
args=args,
headers={"Authorization": [f"Bearer {service.hs_token}"]},
)
Expand Down Expand Up @@ -285,11 +231,8 @@ async def _get() -> Optional[JsonDict]:
# This is required by the configuration.
assert service.hs_token is not None
try:
info = await self._send_with_fallbacks(
service,
[APP_SERVICE_PREFIX, APP_SERVICE_UNSTABLE_PREFIX],
f"/thirdparty/protocol/{urllib.parse.quote(protocol)}",
self.get_json,
info = await self.get_json(
f"{service.url}{APP_SERVICE_PREFIX}/thirdparty/protocol/{urllib.parse.quote(protocol)}",
{"access_token": service.hs_token},
headers={"Authorization": [f"Bearer {service.hs_token}"]},
)
Expand Down Expand Up @@ -401,11 +344,8 @@ async def push_bulk(
}

try:
await self._send_with_fallbacks(
service,
[APP_SERVICE_PREFIX, ""],
f"/transactions/{urllib.parse.quote(str(txn_id))}",
self.put_json,
await self.put_json(
f"{service.url}{APP_SERVICE_PREFIX}/transactions/{urllib.parse.quote(str(txn_id))}",
json_body=body,
args={"access_token": service.hs_token},
headers={"Authorization": [f"Bearer {service.hs_token}"]},
Expand Down
53 changes: 0 additions & 53 deletions tests/appservice/test_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

from twisted.test.proto_helpers import MemoryReactor

from synapse.api.errors import HttpResponseException
from synapse.appservice import ApplicationService
from synapse.server import HomeServer
from synapse.types import JsonDict
Expand Down Expand Up @@ -107,58 +106,6 @@ async def get_json(
self.assertEqual(self.request_url, URL_LOCATION)
self.assertEqual(result, SUCCESS_RESULT_LOCATION)

def test_fallback(self) -> None:
"""
Tests that the fallback to legacy URLs works.
"""
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 HttpResponseException(404, "NOT_FOUND", b"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
Loading