From bc422e1203baef0827eca39936080ff91b8f0c3f Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Wed, 21 Oct 2020 11:26:08 +0100
Subject: [PATCH 01/14] Make get_user_by_access_token return a proper type

---
 synapse/api/auth.py                           | 70 +++++--------------
 synapse/handlers/auth.py                      |  8 +--
 synapse/handlers/register.py                  |  7 +-
 .../storage/databases/main/registration.py    | 31 ++++++--
 tests/api/test_auth.py                        | 29 ++++----
 tests/handlers/test_device.py                 |  2 +-
 tests/handlers/test_message.py                |  2 +-
 tests/push/test_email.py                      |  2 +-
 tests/push/test_http.py                       | 10 +--
 tests/replication/test_pusher_shard.py        |  2 +-
 tests/storage/test_registration.py            | 10 ++-
 11 files changed, 81 insertions(+), 92 deletions(-)

diff --git a/synapse/api/auth.py b/synapse/api/auth.py
index bff87fabde75..a182ce22dbdd 100644
--- a/synapse/api/auth.py
+++ b/synapse/api/auth.py
@@ -33,6 +33,7 @@
 from synapse.api.room_versions import KNOWN_ROOM_VERSIONS
 from synapse.events import EventBase
 from synapse.logging import opentracing as opentracing
+from synapse.storage.databases.main.registration import TokenLookupResult
 from synapse.types import StateMap, UserID
 from synapse.util.caches.lrucache import LruCache
 from synapse.util.metrics import Measure
@@ -210,10 +211,10 @@ async def get_user_by_req(
             user_info = await self.get_user_by_access_token(
                 access_token, rights, allow_expired=allow_expired
             )
-            user = user_info["user"]
-            token_id = user_info["token_id"]
-            is_guest = user_info["is_guest"]
-            shadow_banned = user_info["shadow_banned"]
+            user = UserID.from_string(user_info.user_id)
+            token_id = user_info.token_id
+            is_guest = user_info.is_guest
+            shadow_banned = user_info.shadow_banned
 
             # Deny the request if the user account has expired.
             if self._account_validity.enabled and not allow_expired:
@@ -223,11 +224,9 @@ async def get_user_by_req(
                         403, "User account has expired", errcode=Codes.EXPIRED_ACCOUNT
                     )
 
-            # device_id may not be present if get_user_by_access_token has been
-            # stubbed out.
-            device_id = user_info.get("device_id")
+            device_id = user_info.device_id
 
-            if user and access_token and ip_addr:
+            if access_token and ip_addr:
                 await self.store.insert_client_ip(
                     user_id=user.to_string(),
                     access_token=access_token,
@@ -286,7 +285,7 @@ async def _get_appservice_user_id(self, request):
 
     async def get_user_by_access_token(
         self, token: str, rights: str = "access", allow_expired: bool = False,
-    ) -> dict:
+    ) -> TokenLookupResult:
         """ Validate access token and get user_id from it
 
         Args:
@@ -295,13 +294,7 @@ async def get_user_by_access_token(
                 allow this
             allow_expired: If False, raises an InvalidClientTokenError
                 if the token is expired
-        Returns:
-            dict that includes:
-               `user` (UserID)
-               `is_guest` (bool)
-               `shadow_banned` (bool)
-               `token_id` (int|None): access token id. May be None if guest
-               `device_id` (str|None): device corresponding to access token
+
         Raises:
             InvalidClientTokenError if a user by that token exists, but the token is
                 expired
@@ -311,9 +304,9 @@ async def get_user_by_access_token(
 
         if rights == "access":
             # first look in the database
-            r = await self._look_up_user_by_access_token(token)
+            r = await self.store.get_user_by_access_token(token)
             if r:
-                valid_until_ms = r["valid_until_ms"]
+                valid_until_ms = r.valid_until_ms
                 if (
                     not allow_expired
                     and valid_until_ms is not None
@@ -330,7 +323,6 @@ async def get_user_by_access_token(
         # otherwise it needs to be a valid macaroon
         try:
             user_id, guest = self._parse_and_validate_macaroon(token, rights)
-            user = UserID.from_string(user_id)
 
             if rights == "access":
                 if not guest:
@@ -356,23 +348,17 @@ async def get_user_by_access_token(
                     raise InvalidClientTokenError(
                         "Guest access token used for regular user"
                     )
-                ret = {
-                    "user": user,
-                    "is_guest": True,
-                    "shadow_banned": False,
-                    "token_id": None,
+
+                ret = TokenLookupResult(
+                    user_id=user_id,
+                    is_guest=True,
                     # all guests get the same device id
-                    "device_id": GUEST_DEVICE_ID,
-                }
+                    device_id=GUEST_DEVICE_ID,
+                )
             elif rights == "delete_pusher":
                 # We don't store these tokens in the database
-                ret = {
-                    "user": user,
-                    "is_guest": False,
-                    "shadow_banned": False,
-                    "token_id": None,
-                    "device_id": None,
-                }
+
+                ret = TokenLookupResult(user_id=user_id, is_guest=False)
             else:
                 raise RuntimeError("Unknown rights setting %s", rights)
             return ret
@@ -481,24 +467,6 @@ def _verify_expiry(self, caveat):
         now = self.hs.get_clock().time_msec()
         return now < expiry
 
-    async def _look_up_user_by_access_token(self, token):
-        ret = await self.store.get_user_by_access_token(token)
-        if not ret:
-            return None
-
-        # we use ret.get() below because *lots* of unit tests stub out
-        # get_user_by_access_token in a way where it only returns a couple of
-        # the fields.
-        user_info = {
-            "user": UserID.from_string(ret.get("name")),
-            "token_id": ret.get("token_id", None),
-            "is_guest": False,
-            "shadow_banned": ret.get("shadow_banned"),
-            "device_id": ret.get("device_id"),
-            "valid_until_ms": ret.get("valid_until_ms"),
-        }
-        return user_info
-
     def get_appservice_by_req(self, request):
         token = self.get_access_token_from_request(request)
         service = self.store.get_app_service_by_token(token)
diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index 8619fbb982f6..70c95361f18c 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -984,17 +984,17 @@ async def delete_access_token(self, access_token: str):
                 # This might return an awaitable, if it does block the log out
                 # until it completes.
                 result = provider.on_logged_out(
-                    user_id=str(user_info["user"]),
-                    device_id=user_info["device_id"],
+                    user_id=user_info.user_id,
+                    device_id=user_info.device_id,
                     access_token=access_token,
                 )
                 if inspect.isawaitable(result):
                     await result
 
         # delete pushers associated with this access token
-        if user_info["token_id"] is not None:
+        if user_info.token_id is not None:
             await self.hs.get_pusherpool().remove_pushers_by_access_token(
-                str(user_info["user"]), (user_info["token_id"],)
+                user_info.user_id, (user_info.token_id,)
             )
 
     async def delete_access_tokens_for_user(
diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py
index a6f1d21674b6..ed1ff62599ff 100644
--- a/synapse/handlers/register.py
+++ b/synapse/handlers/register.py
@@ -115,7 +115,10 @@ async def check_username(
                     400, "User ID already taken.", errcode=Codes.USER_IN_USE
                 )
             user_data = await self.auth.get_user_by_access_token(guest_access_token)
-            if not user_data["is_guest"] or user_data["user"].localpart != localpart:
+            if (
+                not user_data.is_guest
+                or UserID.from_string(user_data.user_id).localpart != localpart
+            ):
                 raise AuthError(
                     403,
                     "Cannot register taken user ID without valid guest "
@@ -741,7 +744,7 @@ async def _register_email_threepid(self, user_id, threepid, token):
             # up when the access token is saved, but that's quite an
             # invasive change I'd rather do separately.
             user_tuple = await self.store.get_user_by_access_token(token)
-            token_id = user_tuple["token_id"]
+            token_id = user_tuple.token_id
 
             await self.pusher_pool.add_pusher(
                 user_id=user_id,
diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py
index b0329e17ec6e..6bbf1d9d88bf 100644
--- a/synapse/storage/databases/main/registration.py
+++ b/synapse/storage/databases/main/registration.py
@@ -18,6 +18,8 @@
 import re
 from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple
 
+import attr
+
 from synapse.api.constants import UserTypes
 from synapse.api.errors import Codes, StoreError, SynapseError, ThreepidValidationError
 from synapse.metrics.background_process_metrics import wrap_as_background_process
@@ -38,6 +40,27 @@
 logger = logging.getLogger(__name__)
 
 
+@attr.s(frozen=True, slots=True)
+class TokenLookupResult:
+    """Result of looking up an access token.
+
+    Attributes:
+        user_id: The user that this token authenticates as
+        is_guest
+        shadow_banned
+        token_id: The ID of the access token looked up
+        device_id: The device associated with the token, if any.
+        valid_until_ms: The timestamp the token expires, if any.
+    """
+
+    user_id = attr.ib(type=str)
+    is_guest = attr.ib(type=bool, default=False)
+    shadow_banned = attr.ib(type=bool, default=False)
+    token_id = attr.ib(type=Optional[int], default=None)
+    device_id = attr.ib(type=Optional[str], default=None)
+    valid_until_ms = attr.ib(type=Optional[int], default=None)
+
+
 class RegistrationWorkerStore(CacheInvalidationWorkerStore):
     def __init__(self, database: DatabasePool, db_conn: Connection, hs: "HomeServer"):
         super().__init__(database, db_conn, hs)
@@ -102,7 +125,7 @@ async def is_trial_user(self, user_id: str) -> bool:
         return is_trial
 
     @cached()
-    async def get_user_by_access_token(self, token: str) -> Optional[dict]:
+    async def get_user_by_access_token(self, token: str) -> Optional[TokenLookupResult]:
         """Get a user from the given access token.
 
         Args:
@@ -331,9 +354,9 @@ def set_server_admin_txn(txn):
 
         await self.db_pool.runInteraction("set_server_admin", set_server_admin_txn)
 
-    def _query_for_auth(self, txn, token):
+    def _query_for_auth(self, txn, token: str) -> Optional[TokenLookupResult]:
         sql = """
-            SELECT users.name,
+            SELECT users.name as user_id,
                 users.is_guest,
                 users.shadow_banned,
                 access_tokens.id as token_id,
@@ -347,7 +370,7 @@ def _query_for_auth(self, txn, token):
         txn.execute(sql, (token,))
         rows = self.db_pool.cursor_to_dict(txn)
         if rows:
-            return rows[0]
+            return TokenLookupResult(**rows[0])
 
         return None
 
diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py
index cb6f29d6704b..0fd55f428a0d 100644
--- a/tests/api/test_auth.py
+++ b/tests/api/test_auth.py
@@ -29,6 +29,7 @@
     MissingClientTokenError,
     ResourceLimitError,
 )
+from synapse.storage.databases.main.registration import TokenLookupResult
 from synapse.types import UserID
 
 from tests import unittest
@@ -61,7 +62,9 @@ def setUp(self):
 
     @defer.inlineCallbacks
     def test_get_user_by_req_user_valid_token(self):
-        user_info = {"name": self.test_user, "token_id": "ditto", "device_id": "device"}
+        user_info = TokenLookupResult(
+            user_id=self.test_user, token_id=5, device_id="device"
+        )
         self.store.get_user_by_access_token = Mock(
             return_value=defer.succeed(user_info)
         )
@@ -84,7 +87,7 @@ def test_get_user_by_req_user_bad_token(self):
         self.assertEqual(f.errcode, "M_UNKNOWN_TOKEN")
 
     def test_get_user_by_req_user_missing_token(self):
-        user_info = {"name": self.test_user, "token_id": "ditto"}
+        user_info = TokenLookupResult(user_id=self.test_user, token_id=5)
         self.store.get_user_by_access_token = Mock(
             return_value=defer.succeed(user_info)
         )
@@ -221,7 +224,7 @@ def test_get_user_by_req_appservice_valid_token_bad_user_id(self):
     def test_get_user_from_macaroon(self):
         self.store.get_user_by_access_token = Mock(
             return_value=defer.succeed(
-                {"name": "@baldrick:matrix.org", "device_id": "device"}
+                TokenLookupResult(user_id="@baldrick:matrix.org", device_id="device")
             )
         )
 
@@ -237,12 +240,11 @@ def test_get_user_from_macaroon(self):
         user_info = yield defer.ensureDeferred(
             self.auth.get_user_by_access_token(macaroon.serialize())
         )
-        user = user_info["user"]
-        self.assertEqual(UserID.from_string(user_id), user)
+        self.assertEqual(user_id, user_info.user_id)
 
         # TODO: device_id should come from the macaroon, but currently comes
         # from the db.
-        self.assertEqual(user_info["device_id"], "device")
+        self.assertEqual(user_info.device_id, "device")
 
     @defer.inlineCallbacks
     def test_get_guest_user_from_macaroon(self):
@@ -264,10 +266,8 @@ def test_get_guest_user_from_macaroon(self):
         user_info = yield defer.ensureDeferred(
             self.auth.get_user_by_access_token(serialized)
         )
-        user = user_info["user"]
-        is_guest = user_info["is_guest"]
-        self.assertEqual(UserID.from_string(user_id), user)
-        self.assertTrue(is_guest)
+        self.assertEqual(user_id, user_info.user_id)
+        self.assertTrue(user_info.is_guest)
         self.store.get_user_by_id.assert_called_with(user_id)
 
     @defer.inlineCallbacks
@@ -289,12 +289,9 @@ def get_user(tok):
             if token != tok:
                 return defer.succeed(None)
             return defer.succeed(
-                {
-                    "name": USER_ID,
-                    "is_guest": False,
-                    "token_id": 1234,
-                    "device_id": "DEVICE",
-                }
+                TokenLookupResult(
+                    user_id=USER_ID, is_guest=False, token_id=1234, device_id="DEVICE",
+                )
             )
 
         self.store.get_user_by_access_token = get_user
diff --git a/tests/handlers/test_device.py b/tests/handlers/test_device.py
index 4512c513111c..875aaec2c693 100644
--- a/tests/handlers/test_device.py
+++ b/tests/handlers/test_device.py
@@ -289,7 +289,7 @@ def test_dehydrate_and_rehydrate_device(self):
         # make sure that our device ID has changed
         user_info = self.get_success(self.auth.get_user_by_access_token(access_token))
 
-        self.assertEqual(user_info["device_id"], retrieved_device_id)
+        self.assertEqual(user_info.device_id, retrieved_device_id)
 
         # make sure the device has the display name that was set from the login
         res = self.get_success(self.handler.get_device(user_id, retrieved_device_id))
diff --git a/tests/handlers/test_message.py b/tests/handlers/test_message.py
index 9f6f21a6e202..2e0fea04af8e 100644
--- a/tests/handlers/test_message.py
+++ b/tests/handlers/test_message.py
@@ -46,7 +46,7 @@ def prepare(self, reactor, clock, hs):
         self.info = self.get_success(
             self.hs.get_datastore().get_user_by_access_token(self.access_token,)
         )
-        self.token_id = self.info["token_id"]
+        self.token_id = self.info.token_id
 
         self.requester = create_requester(self.user_id, access_token_id=self.token_id)
 
diff --git a/tests/push/test_email.py b/tests/push/test_email.py
index 55545d93410d..476781daf0e4 100644
--- a/tests/push/test_email.py
+++ b/tests/push/test_email.py
@@ -100,7 +100,7 @@ def prepare(self, reactor, clock, hs):
         user_tuple = self.get_success(
             self.hs.get_datastore().get_user_by_access_token(self.access_token)
         )
-        token_id = user_tuple["token_id"]
+        token_id = user_tuple.token_id
 
         self.pusher = self.get_success(
             self.hs.get_pusherpool().add_pusher(
diff --git a/tests/push/test_http.py b/tests/push/test_http.py
index b567868b02da..8571924b29fe 100644
--- a/tests/push/test_http.py
+++ b/tests/push/test_http.py
@@ -69,7 +69,7 @@ def test_sends_http(self):
         user_tuple = self.get_success(
             self.hs.get_datastore().get_user_by_access_token(access_token)
         )
-        token_id = user_tuple["token_id"]
+        token_id = user_tuple.token_id
 
         self.get_success(
             self.hs.get_pusherpool().add_pusher(
@@ -181,7 +181,7 @@ def test_sends_high_priority_for_encrypted(self):
         user_tuple = self.get_success(
             self.hs.get_datastore().get_user_by_access_token(access_token)
         )
-        token_id = user_tuple["token_id"]
+        token_id = user_tuple.token_id
 
         self.get_success(
             self.hs.get_pusherpool().add_pusher(
@@ -297,7 +297,7 @@ def test_sends_high_priority_for_one_to_one_only(self):
         user_tuple = self.get_success(
             self.hs.get_datastore().get_user_by_access_token(access_token)
         )
-        token_id = user_tuple["token_id"]
+        token_id = user_tuple.token_id
 
         self.get_success(
             self.hs.get_pusherpool().add_pusher(
@@ -379,7 +379,7 @@ def test_sends_high_priority_for_mention(self):
         user_tuple = self.get_success(
             self.hs.get_datastore().get_user_by_access_token(access_token)
         )
-        token_id = user_tuple["token_id"]
+        token_id = user_tuple.token_id
 
         self.get_success(
             self.hs.get_pusherpool().add_pusher(
@@ -452,7 +452,7 @@ def test_sends_high_priority_for_atroom(self):
         user_tuple = self.get_success(
             self.hs.get_datastore().get_user_by_access_token(access_token)
         )
-        token_id = user_tuple["token_id"]
+        token_id = user_tuple.token_id
 
         self.get_success(
             self.hs.get_pusherpool().add_pusher(
diff --git a/tests/replication/test_pusher_shard.py b/tests/replication/test_pusher_shard.py
index 2bdc6edbb14f..67c27a089fab 100644
--- a/tests/replication/test_pusher_shard.py
+++ b/tests/replication/test_pusher_shard.py
@@ -55,7 +55,7 @@ def _create_pusher_and_send_msg(self, localpart):
         user_dict = self.get_success(
             self.hs.get_datastore().get_user_by_access_token(access_token)
         )
-        token_id = user_dict["token_id"]
+        token_id = user_dict.token_id
 
         self.get_success(
             self.hs.get_pusherpool().add_pusher(
diff --git a/tests/storage/test_registration.py b/tests/storage/test_registration.py
index 6b582771feaf..c8c7a90e5dd7 100644
--- a/tests/storage/test_registration.py
+++ b/tests/storage/test_registration.py
@@ -69,11 +69,9 @@ def test_add_tokens(self):
             self.store.get_user_by_access_token(self.tokens[1])
         )
 
-        self.assertDictContainsSubset(
-            {"name": self.user_id, "device_id": self.device_id}, result
-        )
-
-        self.assertTrue("token_id" in result)
+        self.assertEqual(result.user_id, self.user_id)
+        self.assertEqual(result.device_id, self.device_id)
+        self.assertIsNotNone(result.token_id)
 
     @defer.inlineCallbacks
     def test_user_delete_access_tokens(self):
@@ -105,7 +103,7 @@ def test_user_delete_access_tokens(self):
         user = yield defer.ensureDeferred(
             self.store.get_user_by_access_token(self.tokens[0])
         )
-        self.assertEqual(self.user_id, user["name"])
+        self.assertEqual(self.user_id, user.user_id)
 
         # now delete the rest
         yield defer.ensureDeferred(self.store.user_delete_access_tokens(self.user_id))

From 17a3d942c3bf06dc47d678b4e44f53e9512495bd Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Wed, 21 Oct 2020 14:07:59 +0100
Subject: [PATCH 02/14] Add concept of authenticated_entity vs target_user

---
 synapse/api/auth.py                           | 15 ++++++++++-----
 synapse/http/site.py                          |  6 ++++++
 .../storage/databases/main/registration.py    | 13 +++++++++++--
 .../main/schema/delta/58/22puppet_token.sql   | 17 +++++++++++++++++
 synapse/types.py                              | 19 ++++++++++++++++++-
 5 files changed, 62 insertions(+), 8 deletions(-)
 create mode 100644 synapse/storage/databases/main/schema/delta/58/22puppet_token.sql

diff --git a/synapse/api/auth.py b/synapse/api/auth.py
index a182ce22dbdd..39d4ac33b4db 100644
--- a/synapse/api/auth.py
+++ b/synapse/api/auth.py
@@ -195,6 +195,7 @@ async def get_user_by_req(
             if user_id:
                 request.authenticated_entity = user_id
                 opentracing.set_tag("authenticated_entity", user_id)
+                opentracing.set_tag("target_user", user_id)
                 opentracing.set_tag("appservice_id", app_service.id)
 
                 if ip_addr and self._track_appservice_user_ips:
@@ -218,8 +219,9 @@ async def get_user_by_req(
 
             # Deny the request if the user account has expired.
             if self._account_validity.enabled and not allow_expired:
-                user_id = user.to_string()
-                if await self.store.is_account_expired(user_id, self.clock.time_msec()):
+                if await self.store.is_account_expired(
+                    user_info.user_id, self.clock.time_msec()
+                ):
                     raise AuthError(
                         403, "User account has expired", errcode=Codes.EXPIRED_ACCOUNT
                     )
@@ -228,7 +230,7 @@ async def get_user_by_req(
 
             if access_token and ip_addr:
                 await self.store.insert_client_ip(
-                    user_id=user.to_string(),
+                    user_id=user_info.token_owner,
                     access_token=access_token,
                     ip=ip_addr,
                     user_agent=user_agent,
@@ -242,8 +244,10 @@ async def get_user_by_req(
                     errcode=Codes.GUEST_ACCESS_FORBIDDEN,
                 )
 
-            request.authenticated_entity = user.to_string()
-            opentracing.set_tag("authenticated_entity", user.to_string())
+            request.authenticated_entity = user_info.token_owner
+            request.target_user = user_info.user_id
+            opentracing.set_tag("authenticated_entity", user_info.token_owner)
+            opentracing.set_tag("target_user", user_info.user_id)
             if device_id:
                 opentracing.set_tag("device_id", device_id)
 
@@ -254,6 +258,7 @@ async def get_user_by_req(
                 shadow_banned,
                 device_id,
                 app_service=app_service,
+                authenticated_entity=user_info.token_owner,
             )
         except KeyError:
             raise MissingClientTokenError()
diff --git a/synapse/http/site.py b/synapse/http/site.py
index 6e79b4782801..480c4d5e2f68 100644
--- a/synapse/http/site.py
+++ b/synapse/http/site.py
@@ -55,6 +55,7 @@ def __init__(self, channel, *args, **kw):
         self.site = channel.site
         self._channel = channel  # this is used by the tests
         self.authenticated_entity = None
+        self.target_user = None
         self.start_time = 0.0
 
         # we can't yet create the logcontext, as we don't know the method.
@@ -269,6 +270,11 @@ def _finished_processing(self):
         if authenticated_entity is not None and isinstance(authenticated_entity, bytes):
             authenticated_entity = authenticated_entity.decode("utf-8", "replace")
 
+        if self.target_user:
+            authenticated_entity = "{} as {}".format(
+                authenticated_entity, self.target_user,
+            )
+
         # ...or could be raw utf-8 bytes in the User-Agent header.
         # N.B. if you don't do this, the logger explodes cryptically
         # with maximum recursion trying to log errors about
diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py
index 6bbf1d9d88bf..bb7b10a79332 100644
--- a/synapse/storage/databases/main/registration.py
+++ b/synapse/storage/databases/main/registration.py
@@ -51,6 +51,8 @@ class TokenLookupResult:
         token_id: The ID of the access token looked up
         device_id: The device associated with the token, if any.
         valid_until_ms: The timestamp the token expires, if any.
+        token_owner: The "owner" of the token. This is either the same as the
+            user, or a server admin who is logged in as the user.
     """
 
     user_id = attr.ib(type=str)
@@ -59,6 +61,12 @@ class TokenLookupResult:
     token_id = attr.ib(type=Optional[int], default=None)
     device_id = attr.ib(type=Optional[str], default=None)
     valid_until_ms = attr.ib(type=Optional[int], default=None)
+    token_owner = attr.ib(type=str)
+
+    # Make the token owner default to the user ID, which is the common case.
+    @token_owner.default
+    def _default_token_owner(self):
+        return self.user_id
 
 
 class RegistrationWorkerStore(CacheInvalidationWorkerStore):
@@ -361,9 +369,10 @@ def _query_for_auth(self, txn, token: str) -> Optional[TokenLookupResult]:
                 users.shadow_banned,
                 access_tokens.id as token_id,
                 access_tokens.device_id,
-                access_tokens.valid_until_ms
+                access_tokens.valid_until_ms,
+                access_tokens.user_id as token_owner
             FROM users
-            INNER JOIN access_tokens on users.name = access_tokens.user_id
+            INNER JOIN access_tokens on users.name = COALESCE(puppets_user_id, access_tokens.user_id)
             WHERE token = ?
         """
 
diff --git a/synapse/storage/databases/main/schema/delta/58/22puppet_token.sql b/synapse/storage/databases/main/schema/delta/58/22puppet_token.sql
new file mode 100644
index 000000000000..00a9431a97f9
--- /dev/null
+++ b/synapse/storage/databases/main/schema/delta/58/22puppet_token.sql
@@ -0,0 +1,17 @@
+/* Copyright 2020 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.
+ */
+
+-- Whether the access token is an admin token for controlling another user.
+ALTER TABLE access_tokens ADD COLUMN puppets_user_id TEXT;
diff --git a/synapse/types.py b/synapse/types.py
index 5bde67cc078a..7fb39038b3ed 100644
--- a/synapse/types.py
+++ b/synapse/types.py
@@ -74,6 +74,7 @@ class Requester(
             "shadow_banned",
             "device_id",
             "app_service",
+            "authenticated_entity",
         ],
     )
 ):
@@ -104,6 +105,7 @@ def serialize(self):
             "shadow_banned": self.shadow_banned,
             "device_id": self.device_id,
             "app_server_id": self.app_service.id if self.app_service else None,
+            "authenticated_entity": self.authenticated_entity,
         }
 
     @staticmethod
@@ -129,6 +131,7 @@ def deserialize(store, input):
             shadow_banned=input["shadow_banned"],
             device_id=input["device_id"],
             app_service=appservice,
+            authenticated_entity=input["authenticated_entity"],
         )
 
 
@@ -139,6 +142,7 @@ def create_requester(
     shadow_banned=False,
     device_id=None,
     app_service=None,
+    authenticated_entity=None,
 ):
     """
     Create a new ``Requester`` object
@@ -151,14 +155,27 @@ def create_requester(
         shadow_banned (bool):  True if the user making this request is shadow-banned.
         device_id (str|None):  device_id which was set at authentication time
         app_service (ApplicationService|None):  the AS requesting on behalf of the user
+        authenticated_entity: The entity that authenticatd when making the request,
+            this is different than the user_id when an admin user or the server is
+            "puppeting" the user.
 
     Returns:
         Requester
     """
     if not isinstance(user_id, UserID):
         user_id = UserID.from_string(user_id)
+
+    if authenticated_entity is None:
+        authenticated_entity = user_id.to_string()
+
     return Requester(
-        user_id, access_token_id, is_guest, shadow_banned, device_id, app_service
+        user_id,
+        access_token_id,
+        is_guest,
+        shadow_banned,
+        device_id,
+        app_service,
+        authenticated_entity,
     )
 
 

From 028f04b797d638b000a72a0cdfe9695f452b7a98 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Wed, 21 Oct 2020 17:21:46 +0100
Subject: [PATCH 03/14] Newsfile

---
 changelog.d/8616.misc | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 changelog.d/8616.misc

diff --git a/changelog.d/8616.misc b/changelog.d/8616.misc
new file mode 100644
index 000000000000..385b14063ef0
--- /dev/null
+++ b/changelog.d/8616.misc
@@ -0,0 +1 @@
+Change schema to support access tokens belonging to one user but granting access to another.

From 9b72dab566e0a93f809609c7f801db55a66a6d2b Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Fri, 23 Oct 2020 10:52:40 +0100
Subject: [PATCH 04/14] Update synapse/types.py

Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
---
 synapse/types.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/types.py b/synapse/types.py
index 7fb39038b3ed..87bb6e36bd29 100644
--- a/synapse/types.py
+++ b/synapse/types.py
@@ -155,7 +155,7 @@ def create_requester(
         shadow_banned (bool):  True if the user making this request is shadow-banned.
         device_id (str|None):  device_id which was set at authentication time
         app_service (ApplicationService|None):  the AS requesting on behalf of the user
-        authenticated_entity: The entity that authenticatd when making the request,
+        authenticated_entity: The entity that authenticated when making the request,
             this is different than the user_id when an admin user or the server is
             "puppeting" the user.
 

From 66ef6cd7cba8c9de024aa478a76cb91fe3201ec0 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Fri, 23 Oct 2020 10:53:18 +0100
Subject: [PATCH 05/14] Update docstring

---
 synapse/storage/databases/main/registration.py | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py
index bb7b10a79332..b20de4611641 100644
--- a/synapse/storage/databases/main/registration.py
+++ b/synapse/storage/databases/main/registration.py
@@ -139,9 +139,7 @@ async def get_user_by_access_token(self, token: str) -> Optional[TokenLookupResu
         Args:
             token: The access token of a user.
         Returns:
-            None, if the token did not match, otherwise dict
-            including the keys `name`, `is_guest`, `device_id`, `token_id`,
-            `valid_until_ms`.
+            None, if the token did not match, otherwise a `TokenLookupResult`
         """
         return await self.db_pool.runInteraction(
             "get_user_by_access_token", self._query_for_auth, token

From c87bf0d84b23d41bf8024773fba5ec67679607c8 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Tue, 27 Oct 2020 10:33:04 +0000
Subject: [PATCH 06/14] Update docstring

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
---
 synapse/types.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/synapse/types.py b/synapse/types.py
index 87bb6e36bd29..df0b7589481f 100644
--- a/synapse/types.py
+++ b/synapse/types.py
@@ -155,8 +155,8 @@ def create_requester(
         shadow_banned (bool):  True if the user making this request is shadow-banned.
         device_id (str|None):  device_id which was set at authentication time
         app_service (ApplicationService|None):  the AS requesting on behalf of the user
-        authenticated_entity: The entity that authenticated when making the request,
-            this is different than the user_id when an admin user or the server is
+        authenticated_entity: The entity that authenticated when making the request.
+            This is different to the user_id when an admin user or the server is
             "puppeting" the user.
 
     Returns:

From 6a063043e3eae56da4d422dcb51c75dad8fec765 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Tue, 27 Oct 2020 10:42:06 +0000
Subject: [PATCH 07/14] Pass requester to SynapseRequest

---
 synapse/api/auth.py                    | 35 ++++++++++++++-----------
 synapse/federation/transport/server.py |  2 +-
 synapse/http/site.py                   | 36 ++++++++++++++++----------
 synapse/replication/http/membership.py |  6 ++---
 synapse/replication/http/send_event.py |  3 +--
 5 files changed, 47 insertions(+), 35 deletions(-)

diff --git a/synapse/api/auth.py b/synapse/api/auth.py
index 39d4ac33b4db..44186b39ba17 100644
--- a/synapse/api/auth.py
+++ b/synapse/api/auth.py
@@ -193,11 +193,6 @@ async def get_user_by_req(
 
             user_id, app_service = await self._get_appservice_user_id(request)
             if user_id:
-                request.authenticated_entity = user_id
-                opentracing.set_tag("authenticated_entity", user_id)
-                opentracing.set_tag("target_user", user_id)
-                opentracing.set_tag("appservice_id", app_service.id)
-
                 if ip_addr and self._track_appservice_user_ips:
                     await self.store.insert_client_ip(
                         user_id=user_id,
@@ -207,7 +202,16 @@ async def get_user_by_req(
                         device_id="dummy-device",  # stubbed
                     )
 
-                return synapse.types.create_requester(user_id, app_service=app_service)
+                requester = synapse.types.create_requester(
+                    user_id, app_service=app_service
+                )
+
+                request.requester = user_id
+                opentracing.set_tag("authenticated_entity", user_id)
+                opentracing.set_tag("target_user", user_id)
+                opentracing.set_tag("appservice_id", app_service.id)
+
+                return requester
 
             user_info = await self.get_user_by_access_token(
                 access_token, rights, allow_expired=allow_expired
@@ -244,14 +248,7 @@ async def get_user_by_req(
                     errcode=Codes.GUEST_ACCESS_FORBIDDEN,
                 )
 
-            request.authenticated_entity = user_info.token_owner
-            request.target_user = user_info.user_id
-            opentracing.set_tag("authenticated_entity", user_info.token_owner)
-            opentracing.set_tag("target_user", user_info.user_id)
-            if device_id:
-                opentracing.set_tag("device_id", device_id)
-
-            return synapse.types.create_requester(
+            requester = synapse.types.create_requester(
                 user,
                 token_id,
                 is_guest,
@@ -260,6 +257,14 @@ async def get_user_by_req(
                 app_service=app_service,
                 authenticated_entity=user_info.token_owner,
             )
+
+            request.requester = requester
+            opentracing.set_tag("authenticated_entity", user_info.token_owner)
+            opentracing.set_tag("target_user", user_info.user_id)
+            if device_id:
+                opentracing.set_tag("device_id", device_id)
+
+            return requester
         except KeyError:
             raise MissingClientTokenError()
 
@@ -478,7 +483,7 @@ def get_appservice_by_req(self, request):
         if not service:
             logger.warning("Unrecognised appservice access token.")
             raise InvalidClientTokenError()
-        request.authenticated_entity = service.sender
+        request.requester = service.sender
         return service
 
     async def is_server_admin(self, user: UserID) -> bool:
diff --git a/synapse/federation/transport/server.py b/synapse/federation/transport/server.py
index 3a6b95631eae..a0933fae88c8 100644
--- a/synapse/federation/transport/server.py
+++ b/synapse/federation/transport/server.py
@@ -154,7 +154,7 @@ async def authenticate_request(self, request, content):
         )
 
         logger.debug("Request from %s", origin)
-        request.authenticated_entity = origin
+        request.requester = origin
 
         # If we get a valid signed request from the other side, its probably
         # alive
diff --git a/synapse/http/site.py b/synapse/http/site.py
index 480c4d5e2f68..9ddc7a5767d8 100644
--- a/synapse/http/site.py
+++ b/synapse/http/site.py
@@ -14,7 +14,7 @@
 import contextlib
 import logging
 import time
-from typing import Optional
+from typing import Optional, Union
 
 from twisted.python.failure import Failure
 from twisted.web.server import Request, Site
@@ -23,6 +23,7 @@
 from synapse.http import redact_uri
 from synapse.http.request_metrics import RequestMetrics, requests_counter
 from synapse.logging.context import LoggingContext, PreserveLoggingContext
+from synapse.types import Requester
 
 logger = logging.getLogger(__name__)
 
@@ -54,10 +55,12 @@ def __init__(self, channel, *args, **kw):
         Request.__init__(self, channel, *args, **kw)
         self.site = channel.site
         self._channel = channel  # this is used by the tests
-        self.authenticated_entity = None
-        self.target_user = None
         self.start_time = 0.0
 
+        # The requester, if authenticated. For federation requests this is the
+        # server name, for client requests this is the Requester object.
+        self.requester = None  # type: Optional[Union[Requester, str]]
+
         # we can't yet create the logcontext, as we don't know the method.
         self.logcontext = None  # type: Optional[LoggingContext]
 
@@ -264,16 +267,23 @@ def _finished_processing(self):
         # to the client (nb may be negative)
         response_send_time = self.finish_time - self._processing_finished_time
 
-        # need to decode as it could be raw utf-8 bytes
-        # from a IDN servname in an auth header
-        authenticated_entity = self.authenticated_entity
-        if authenticated_entity is not None and isinstance(authenticated_entity, bytes):
-            authenticated_entity = authenticated_entity.decode("utf-8", "replace")
-
-        if self.target_user:
-            authenticated_entity = "{} as {}".format(
-                authenticated_entity, self.target_user,
-            )
+        # Convert the requester into a string that we can log
+        authenticated_entity = None
+        if isinstance(self.requester, str):
+            authenticated_entity = self.requester
+        elif isinstance(self.requester, Requester):
+            authenticated_entity = self.requester.authenticated_entity
+
+            # If this is a request where the target user doesn't match the user who
+            # authenticated (e.g. and admin is puppetting a user) then we log both.
+            if self.requester.user.to_string() != authenticated_entity:
+                authenticated_entity = "{} as {}".format(
+                    authenticated_entity, self.requester.user.to_string(),
+                )
+        elif self.requester is not None:
+            # This shouldn't happen, but we log it so we don't lose information
+            # and can see that we're doing something wrong.
+            authenticated_entity = repr(self.requester)
 
         # ...or could be raw utf-8 bytes in the User-Agent header.
         # N.B. if you don't do this, the logger explodes cryptically
diff --git a/synapse/replication/http/membership.py b/synapse/replication/http/membership.py
index e7cc74a5d21c..f0c37eaf5e1b 100644
--- a/synapse/replication/http/membership.py
+++ b/synapse/replication/http/membership.py
@@ -77,8 +77,7 @@ async def _handle_request(self, request, room_id, user_id):
 
         requester = Requester.deserialize(self.store, content["requester"])
 
-        if requester.user:
-            request.authenticated_entity = requester.user.to_string()
+        request.requester = requester
 
         logger.info("remote_join: %s into room: %s", user_id, room_id)
 
@@ -142,8 +141,7 @@ async def _handle_request(self, request, invite_event_id):
 
         requester = Requester.deserialize(self.store, content["requester"])
 
-        if requester.user:
-            request.authenticated_entity = requester.user.to_string()
+        request.requester = requester
 
         # hopefully we're now on the master, so this won't recurse!
         event_id, stream_id = await self.member_handler.remote_reject_invite(
diff --git a/synapse/replication/http/send_event.py b/synapse/replication/http/send_event.py
index fc129dbaa7b7..8fa104c8d3b4 100644
--- a/synapse/replication/http/send_event.py
+++ b/synapse/replication/http/send_event.py
@@ -115,8 +115,7 @@ async def _handle_request(self, request, event_id):
             ratelimit = content["ratelimit"]
             extra_users = [UserID.from_string(u) for u in content["extra_users"]]
 
-        if requester.user:
-            request.authenticated_entity = requester.user.to_string()
+        request.requester = requester
 
         logger.info(
             "Got event to send with ID: %s into room: %s", event.event_id, event.room_id

From 3286c215dc04f9d524758f289c27d0709f13415b Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Tue, 27 Oct 2020 10:42:48 +0000
Subject: [PATCH 08/14] Change log format when puppetting user to use ','
 rather than ' as '

---
 synapse/http/site.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/http/site.py b/synapse/http/site.py
index 9ddc7a5767d8..dd0dd61c2881 100644
--- a/synapse/http/site.py
+++ b/synapse/http/site.py
@@ -277,7 +277,7 @@ def _finished_processing(self):
             # If this is a request where the target user doesn't match the user who
             # authenticated (e.g. and admin is puppetting a user) then we log both.
             if self.requester.user.to_string() != authenticated_entity:
-                authenticated_entity = "{} as {}".format(
+                authenticated_entity = "{},{}".format(
                     authenticated_entity, self.requester.user.to_string(),
                 )
         elif self.requester is not None:

From acf13140bfdc120e5465b4215c67c866edc829af Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Tue, 27 Oct 2020 10:47:54 +0000
Subject: [PATCH 09/14] Add types to create_requester

---
 synapse/types.py | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/synapse/types.py b/synapse/types.py
index df0b7589481f..66bb5bac8dc2 100644
--- a/synapse/types.py
+++ b/synapse/types.py
@@ -29,6 +29,7 @@
     Tuple,
     Type,
     TypeVar,
+    Union,
 )
 
 import attr
@@ -38,6 +39,7 @@
 from synapse.api.errors import Codes, SynapseError
 
 if TYPE_CHECKING:
+    from synapse.appservice.api import ApplicationService
     from synapse.storage.databases.main import DataStore
 
 # define a version of typing.Collection that works on python 3.5
@@ -136,13 +138,13 @@ def deserialize(store, input):
 
 
 def create_requester(
-    user_id,
-    access_token_id=None,
-    is_guest=False,
-    shadow_banned=False,
-    device_id=None,
-    app_service=None,
-    authenticated_entity=None,
+    user_id: Union[str, "UserID"],
+    access_token_id: Optional[int] = None,
+    is_guest: Optional[bool] = False,
+    shadow_banned: Optional[bool] = False,
+    device_id: Optional[str] = None,
+    app_service: Optional["ApplicationService"] = None,
+    authenticated_entity: Optional[str] = None,
 ):
     """
     Create a new ``Requester`` object

From 6f736b849ccc3448b555bd4098f9674b6d1c202f Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Tue, 27 Oct 2020 11:10:24 +0000
Subject: [PATCH 10/14] Change opentracing to use user_id not target_user

---
 synapse/api/auth.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/synapse/api/auth.py b/synapse/api/auth.py
index 44186b39ba17..60eacb1bce18 100644
--- a/synapse/api/auth.py
+++ b/synapse/api/auth.py
@@ -208,7 +208,7 @@ async def get_user_by_req(
 
                 request.requester = user_id
                 opentracing.set_tag("authenticated_entity", user_id)
-                opentracing.set_tag("target_user", user_id)
+                opentracing.set_tag("user_id", user_id)
                 opentracing.set_tag("appservice_id", app_service.id)
 
                 return requester
@@ -260,7 +260,7 @@ async def get_user_by_req(
 
             request.requester = requester
             opentracing.set_tag("authenticated_entity", user_info.token_owner)
-            opentracing.set_tag("target_user", user_info.user_id)
+            opentracing.set_tag("user_id", user_info.user_id)
             if device_id:
                 opentracing.set_tag("device_id", device_id)
 

From 9d919da7fffc69aa46f9ec997878ad23ebe84d8b Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Tue, 27 Oct 2020 11:59:40 +0000
Subject: [PATCH 11/14] Fix mypy

---
 synapse/http/site.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/http/site.py b/synapse/http/site.py
index dd0dd61c2881..6e83bfc529a2 100644
--- a/synapse/http/site.py
+++ b/synapse/http/site.py
@@ -283,7 +283,7 @@ def _finished_processing(self):
         elif self.requester is not None:
             # This shouldn't happen, but we log it so we don't lose information
             # and can see that we're doing something wrong.
-            authenticated_entity = repr(self.requester)
+            authenticated_entity = repr(self.requester)  # type: ignore[unreachable]
 
         # ...or could be raw utf-8 bytes in the User-Agent header.
         # N.B. if you don't do this, the logger explodes cryptically

From 49bb079d876c4b7c5a8f702f2a84e081b58963d3 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Wed, 28 Oct 2020 16:00:01 +0000
Subject: [PATCH 12/14] Review comments

---
 synapse/api/auth.py | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/synapse/api/auth.py b/synapse/api/auth.py
index 60eacb1bce18..1c55fcf8c234 100644
--- a/synapse/api/auth.py
+++ b/synapse/api/auth.py
@@ -216,7 +216,6 @@ async def get_user_by_req(
             user_info = await self.get_user_by_access_token(
                 access_token, rights, allow_expired=allow_expired
             )
-            user = UserID.from_string(user_info.user_id)
             token_id = user_info.token_id
             is_guest = user_info.is_guest
             shadow_banned = user_info.shadow_banned
@@ -249,7 +248,7 @@ async def get_user_by_req(
                 )
 
             requester = synapse.types.create_requester(
-                user,
+                user_info.user_id,
                 token_id,
                 is_guest,
                 shadow_banned,
@@ -483,7 +482,9 @@ def get_appservice_by_req(self, request):
         if not service:
             logger.warning("Unrecognised appservice access token.")
             raise InvalidClientTokenError()
-        request.requester = service.sender
+        request.requester = synapse.types.create_requester(
+            service.sender, app_service=service
+        )
         return service
 
     async def is_server_admin(self, user: UserID) -> bool:

From 492e15e3b633c568300afaa7d630dd4b4314d10c Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Thu, 29 Oct 2020 10:23:05 +0000
Subject: [PATCH 13/14] Fix tests by mandating AS have sender.

We require application services to be configured with a sender, so we
may as well require it when creating an ApplicationService object.
---
 synapse/appservice/__init__.py              | 5 +++--
 tests/api/test_ratelimiting.py              | 4 ++--
 tests/appservice/test_appservice.py         | 1 +
 tests/rest/client/v2_alpha/test_register.py | 1 +
 4 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/synapse/appservice/__init__.py b/synapse/appservice/__init__.py
index 3862d9c08f38..210dd1ccaf0d 100644
--- a/synapse/appservice/__init__.py
+++ b/synapse/appservice/__init__.py
@@ -52,11 +52,12 @@ def __init__(
         self,
         token,
         hostname,
+        *,
+        id,
+        sender,
         url=None,
         namespaces=None,
         hs_token=None,
-        sender=None,
-        id=None,
         protocols=None,
         rate_limited=True,
         ip_range_whitelist=None,
diff --git a/tests/api/test_ratelimiting.py b/tests/api/test_ratelimiting.py
index 1e1f30d790e0..fe504d0869c1 100644
--- a/tests/api/test_ratelimiting.py
+++ b/tests/api/test_ratelimiting.py
@@ -43,7 +43,7 @@ def test_allowed_user_via_can_requester_do_action(self):
 
     def test_allowed_appservice_ratelimited_via_can_requester_do_action(self):
         appservice = ApplicationService(
-            None, "example.com", id="foo", rate_limited=True,
+            None, "example.com", id="foo", rate_limited=True, sender="@as:example.com",
         )
         as_requester = create_requester("@user:example.com", app_service=appservice)
 
@@ -68,7 +68,7 @@ def test_allowed_appservice_ratelimited_via_can_requester_do_action(self):
 
     def test_allowed_appservice_via_can_requester_do_action(self):
         appservice = ApplicationService(
-            None, "example.com", id="foo", rate_limited=False,
+            None, "example.com", id="foo", rate_limited=False, sender="@as:example.com",
         )
         as_requester = create_requester("@user:example.com", app_service=appservice)
 
diff --git a/tests/appservice/test_appservice.py b/tests/appservice/test_appservice.py
index 236b608d5848..0bffeb115081 100644
--- a/tests/appservice/test_appservice.py
+++ b/tests/appservice/test_appservice.py
@@ -31,6 +31,7 @@ class ApplicationServiceTestCase(unittest.TestCase):
     def setUp(self):
         self.service = ApplicationService(
             id="unique_identifier",
+            sender="@as:test",
             url="some_url",
             token="some_token",
             hostname="matrix.org",  # only used by get_groups_for_user
diff --git a/tests/rest/client/v2_alpha/test_register.py b/tests/rest/client/v2_alpha/test_register.py
index 2fc3a60fc53d..98c3887bbf1f 100644
--- a/tests/rest/client/v2_alpha/test_register.py
+++ b/tests/rest/client/v2_alpha/test_register.py
@@ -55,6 +55,7 @@ def test_POST_appservice_registration_valid(self):
             self.hs.config.server_name,
             id="1234",
             namespaces={"users": [{"regex": r"@as_user.*", "exclusive": True}]},
+            sender="@as:test",
         )
 
         self.hs.get_datastore().services_cache.append(appservice)

From 05c7f7b383dee5cb1d552f1e46494ab58809560e Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Thu, 29 Oct 2020 13:37:44 +0000
Subject: [PATCH 14/14] py3.5 syntax

---
 synapse/appservice/__init__.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/synapse/appservice/__init__.py b/synapse/appservice/__init__.py
index 210dd1ccaf0d..66d008d2f4b9 100644
--- a/synapse/appservice/__init__.py
+++ b/synapse/appservice/__init__.py
@@ -52,7 +52,6 @@ def __init__(
         self,
         token,
         hostname,
-        *,
         id,
         sender,
         url=None,