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

Add authentication to thirdparty bridge APIs #12746

Merged
merged 11 commits into from
May 24, 2022
Merged
1 change: 1 addition & 0 deletions changelog.d/12746.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Require an `access_token` in `/thirdparty/` requests to appservices, as required by the [Matrix specification](https://spec.matrix.org/v1.1/application-service-api/#third-party-networks).
Half-Shot marked this conversation as resolved.
Show resolved Hide resolved
15 changes: 12 additions & 3 deletions synapse/appservice/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
# limitations under the License.
import logging
import urllib.parse
from typing import TYPE_CHECKING, Dict, Iterable, List, Optional, Tuple
from typing import TYPE_CHECKING, Any, Dict, Iterable, List, Mapping, Optional, Tuple

from prometheus_client import Counter
from typing_extensions import TypeGuard
Expand Down Expand Up @@ -155,14 +155,21 @@ async def query_3pe(
if service.url is None:
return []

# 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:
response = await self.get_json(uri, fields)
args: Mapping[Any, Any] = {
**fields,
b"access_token": service.hs_token,
}
response = await self.get_json(uri, args=args)
if not isinstance(response, list):
logger.warning(
"query_3pe to %s returned an invalid response %r", uri, response
Expand Down Expand Up @@ -190,13 +197,15 @@ async def get_3pe_protocol(
return {}

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.get_json(uri, {"access_token": service.hs_token})

if not _is_valid_3pe_metadata(info):
logger.warning(
Expand Down
96 changes: 96 additions & 0 deletions tests/appservice/test_api.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
# Copyright 2022 The Matrix.org Foundation C.I.C.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
from typing import Any, List, Mapping
from unittest.mock import Mock

from twisted.internet.testing import MemoryReactor
Half-Shot marked this conversation as resolved.
Show resolved Hide resolved

from synapse.appservice import ApplicationService
from synapse.appservice.api import ApplicationServiceApi
from synapse.server import HomeServer
from synapse.types import JsonDict
from synapse.util import Clock

from tests import unittest

PROTOCOL = "myproto"
TOKEN = "myastoken"
URL = "http://mytestservice"
URL_USER = f"{URL}/_matrix/app/unstable/thirdparty/user/{PROTOCOL}"
URL_LOCATION = f"{URL}/_matrix/app/unstable/thirdparty/location/{PROTOCOL}"
SUCCESS_RESULT_USER = [
{
"protocol": PROTOCOL,
"userid": "@a:user",
"fields": {
"more": "fields",
},
}
]
SUCCESS_RESULT_LOCATION = [
{
"protocol": PROTOCOL,
"alias": "#a:room",
"fields": {
"more": "fields",
},
}
]


class ApplicationServiceApiTestCase(unittest.HomeserverTestCase):
def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer):
self.api = ApplicationServiceApi(hs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.api = ApplicationServiceApi(hs)
self.api = hs.get_application_service_api()

It probably doesn't make that much of a difference, but it's more consistent with how we access this kind of objects in tests.


self.request_url = None
self.fields = None

async def get_json(url: str, args: Mapping[Any, Any]) -> List[JsonDict]:
if not args.get(b"access_token"):
raise Exception("Access token not provided")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
raise Exception("Access token not provided")
raise RuntimeError("Access token not provided")

For consistency


self.assertEqual(args.get(b"access_token"), TOKEN)
self.request_url = url
self.fields = args
if url == URL_USER:
return SUCCESS_RESULT_USER
elif url == URL_LOCATION:
return SUCCESS_RESULT_LOCATION
else:
self.fail("URL provided was invalid")
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this is now a bit unnecessary since raising the RuntimeError will automatically fail the test.

return []
Half-Shot marked this conversation as resolved.
Show resolved Hide resolved

self.api.get_json = Mock(side_effect=get_json) # type: ignore[assignment] # We assign to a method.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to line wrap and explain a bit what the comment refers to.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this (and the definition of get_json should probably happen within the test? It feels pretty specific to what we want to test in test_query_3pe_authenticates_token.
In this case most of the constants used can become local to test_query_3pe_authenticates_token.

self.service = ApplicationService(
id="unique_identifier",
sender="@as:test",
url=URL,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like URL could just be inlined here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a bit tricky. It's used by template strings URL_USER and URL_LOCATION, but I suppose I could hardcode those to use URL. It just felt a bit more...safe?

token="unused",
hs_token=TOKEN,
hostname="myserver",
)

def test_query_3pe_authenticates_token(self):
result = self.get_success(
self.api.query_3pe(self.service, "user", PROTOCOL, {b"some": [b"field"]})
)
self.assertEqual(self.request_url, URL_USER)
self.assertEqual(result, SUCCESS_RESULT_USER)
result = self.get_success(
self.api.query_3pe(
self.service, "location", PROTOCOL, {b"some": [b"field"]}
)
)
self.assertEqual(self.request_url, URL_LOCATION)
self.assertEqual(result, SUCCESS_RESULT_LOCATION)