synapse.api.auth.Auth cleanup: make permission-related methods use Requester instead of the UserID

synapse/api/auth.py

@@ -37,7 +37,6 @@
  start_active_span,
  trace,
 )
-from synapse.storage.databases.main.registration import TokenLookupResult
 from synapse.types import Requester, create_requester
 
 if TYPE_CHECKING:
@@ -183,41 +182,29 @@ async def _wrapped_get_user_by_req(
 
  access_token = self.get_access_token_from_request(request)
 
- (
- user_id,
- device_id,
- app_service,
- ) = await self._get_appservice_user_id_and_device_id(request)
- if user_id and app_service:
+ # First check if it could be a request from an appservice
+ requester = await self._get_appservice_user(request)
+ if requester:
  if ip_addr and self._track_appservice_user_ips:
  await self.store.insert_client_ip(
- user_id=user_id,
+ user_id=requester.user.to_string(),
  access_token=access_token,
  ip=ip_addr,
  user_agent=user_agent,
- device_id="dummy-device"
- if device_id is None
- else device_id, # stubbed
+ device_id=requester.device_id,
  )
 
- requester = create_requester(
- user_id, app_service=app_service, device_id=device_id
- )
-
- request.requester = user_id
+ request.requester = requester
  return requester
 
- user_info = await self.get_user_by_access_token(
- access_token, allow_expired=allow_expired
+ requester = await self.get_user_by_access_token(
+ access_token, allow_expired=allow_expired, mark_as_used=True
  )
- 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 not allow_expired:
  if await self._account_validity_handler.is_user_expired(
- user_info.user_id
+ requester.user.to_string()
  ):
  # Raise the error if either an account validity module has determined
  # the account has expired, or the legacy account validity
@@ -228,51 +215,34 @@ async def _wrapped_get_user_by_req(
  errcode=Codes.EXPIRED_ACCOUNT,
  )
 
- device_id = user_info.device_id
-
- if access_token and ip_addr:
+ if ip_addr:
  await self.store.insert_client_ip(
- user_id=user_info.token_owner,
+ user_id=requester.authenticated_entity,
  access_token=access_token,
  ip=ip_addr,
  user_agent=user_agent,
- device_id=device_id,
+ device_id=requester.device_id,
  )
  # Track also the puppeted user client IP if enabled and the user is puppeting
  if (
- user_info.user_id != user_info.token_owner
+ requester.user.to_string() != requester.authenticated_entity
  and self._track_puppeted_user_ips
  ):
  await self.store.insert_client_ip(
- user_id=user_info.user_id,
+ user_id=requester.user.to_string(),
  access_token=access_token,
  ip=ip_addr,
  user_agent=user_agent,
- device_id=device_id,
+ device_id=requester.device_id,
  )
 
- if is_guest and not allow_guest:
+ if requester.is_guest and not allow_guest:
  raise AuthError(
  403,
  "Guest access not allowed",
  errcode=Codes.GUEST_ACCESS_FORBIDDEN,
  )
 
- # Mark the token as used. This is used to invalidate old refresh
- # tokens after some time.
- if not user_info.token_used and token_id is not None:
- await self.store.mark_access_token_as_used(token_id)
-
- requester = create_requester(
- user_info.user_id,
- token_id,
- is_guest,
- shadow_banned,
- device_id,
- app_service=app_service,
- authenticated_entity=user_info.token_owner,
- )
-
  request.requester = requester
  return requester
  except KeyError:
@@ -309,9 +279,7 @@ async def validate_appservice_can_control_user_id(
  403, "Application service has not registered this user (%s)" % user_id
  )
 
- async def _get_appservice_user_id_and_device_id(
- self, request: Request
- ) -> Tuple[Optional[str], Optional[str], Optional[ApplicationService]]:
+ async def _get_appservice_user(self, request: Request) -> Optional[Requester]:
  """
  Given a request, reads the request parameters to determine:
  - whether it's an application service that's making this request
@@ -326,8 +294,7 @@ async def _get_appservice_user_id_and_device_id(
  Must use `org.matrix.msc3202.device_id` in place of `device_id` for now.
 
  Returns:
- 3-tuple of
- (user ID?, device ID?, application service?)
+ the application service ``Requester`` of that request
 
  Postconditions:
  - If an application service is returned, so is a user ID
@@ -344,12 +311,12 @@ async def _get_appservice_user_id_and_device_id(
  self.get_access_token_from_request(request)
  )
  if app_service is None:
- return None, None, None
+ return None
 
  if app_service.ip_range_whitelist:
  ip_address = IPAddress(request.getClientAddress().host)
  if ip_address not in app_service.ip_range_whitelist:
- return None, None, None
+ return None
 
  # This will always be set by the time Twisted calls us.
  assert request.args is not None
@@ -383,19 +350,24 @@ async def _get_appservice_user_id_and_device_id(
  Codes.EXCLUSIVE,
  )
 
- return effective_user_id, effective_device_id, app_service
+ return create_requester(
+ effective_user_id, app_service=app_service, device_id=effective_device_id
+ )
 
  async def get_user_by_access_token(
  self,
  token: str,
  allow_expired: bool = False,
- ) -> TokenLookupResult:
+ mark_as_used: bool = False,
+ ) -> Requester:
  """Validate access token and get user_id from it
 
  Args:
  token: The access token to get the user by
  allow_expired: If False, raises an InvalidClientTokenError
  if the token is expired
+ mark_as_used: Mark the token as used, if it was used to
+ authenticate a regular C-S API request
 
  Raises:
  InvalidClientTokenError if a user by that token exists, but the token is
@@ -406,9 +378,9 @@ async def get_user_by_access_token(
 
  # First look in the database to see if the access token is present
  # as an opaque token.
- r = await self.store.get_user_by_access_token(token)
- if r:
- valid_until_ms = r.valid_until_ms
+ user_info = await self.store.get_user_by_access_token(token)
+ if user_info:
+ valid_until_ms = user_info.valid_until_ms
  if (
  not allow_expired
  and valid_until_ms is not None
@@ -420,7 +392,21 @@ async def get_user_by_access_token(
  msg="Access token has expired", soft_logout=True
  )
 
- return r
+ # Mark the token as used. This is used to invalidate old refresh
+ # tokens after some time.
+ if mark_as_used and not user_info.token_used:
+ await self.store.mark_access_token_as_used(user_info.token_id)
+
+ requester = create_requester(
+ user_id=user_info.user_id,
+ access_token_id=user_info.token_id,
+ is_guest=user_info.is_guest,
+ shadow_banned=user_info.shadow_banned,
+ device_id=user_info.device_id,
+ authenticated_entity=user_info.token_owner,
+ )
+
+ return requester
 
  # If the token isn't found in the database, then it could still be a
  # macaroon for a guest, so we check that here.
@@ -446,7 +432,7 @@ async def get_user_by_access_token(
  "Guest access token used for regular user"
  )
 
- return TokenLookupResult(
+ return create_requester(
  user_id=user_id,
  is_guest=True,
  # all guests get the same device id

synapse/handlers/auth.py

@@ -1435,20 +1435,24 @@ async def delete_access_token(self, access_token: str) -> None:
  access_token: access token to be deleted
 
  """
- user_info = await self.auth.get_user_by_access_token(access_token)
+ token = await self.store.get_user_by_access_token(access_token)
+ if not token:
+ # At this point, the token should already have been fetched once by
+ # the caller, so this should not happen
+ raise SynapseError(HTTPStatus.UNAUTHORIZED, "Unrecognised access token")
  await self.store.delete_access_token(access_token)
 
  # see if any modules want to know about this
  await self.password_auth_provider.on_logged_out(
- user_id=user_info.user_id,
- device_id=user_info.device_id,
+ user_id=token.user_id,
+ device_id=token.device_id,
  access_token=access_token,
  )
 
  # delete pushers associated with this access token
- if user_info.token_id is not None:
+ if token.token_id is not None:
  await self.hs.get_pusherpool().remove_pushers_by_access_token(
- user_info.user_id, (user_info.token_id,)
+ token.user_id, (token.token_id,)
  )
 
  async def delete_access_tokens_for_user(

synapse/handlers/register.py

@@ -180,10 +180,7 @@ async def check_username(
  )
  if guest_access_token:
  user_data = await self.auth.get_user_by_access_token(guest_access_token)
- if (
- not user_data.is_guest
- or UserID.from_string(user_data.user_id).localpart != localpart
- ):
+ if not user_data.is_guest or user_data.user.localpart != localpart:
  raise AuthError(
  403,
  "Cannot register taken user ID without valid guest "
@@ -618,7 +615,7 @@ async def appservice_register(self, user_localpart: str, as_token: str) -> str:
  user_id = user.to_string()
  service = self.store.get_app_service_by_token(as_token)
  if not service:
- raise AuthError(403, "Invalid application service token.")
+ raise AuthError(401, "Invalid application service token.")
  if not service.is_interested_in_user(user_id):
  raise SynapseError(
  400,

synapse/http/site.py

@@ -226,7 +226,7 @@ def get_authenticated_entity(self) -> Tuple[Optional[str], Optional[str]]:
 
  # 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 return both.
- if self._requester.user.to_string() != authenticated_entity:
+ if requester != authenticated_entity:
  return requester, authenticated_entity
 
  return requester, None

synapse/rest/client/register.py

@@ -484,9 +484,6 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
  "Appservice token must be provided when using a type of m.login.application_service",
  )
 
- # Verify the AS
- self.auth.get_appservice_by_req(request)
-
  # Set the desired user according to the AS API (which uses the
  # 'user' key not 'username'). Since this is a new addition, we'll
  # fallback to 'username' if they gave one.

synapse/storage/databases/main/registration.py

@@ -69,9 +69,9 @@ class TokenLookupResult:
  """
 
  user_id: str
+ token_id: int
  is_guest: bool = False
  shadow_banned: bool = False
- token_id: Optional[int] = None
  device_id: Optional[str] = None
  valid_until_ms: Optional[int] = None
  token_owner: str = attr.ib()

tests/api/test_auth.py

@@ -284,7 +284,9 @@ def test_get_user_by_req__puppeted_token__not_tracking_puppeted_mau(self):
  TokenLookupResult(
  user_id="@baldrick:matrix.org",
  device_id="device",
+ token_id=5,
  token_owner="@admin:matrix.org",
+ token_used=True,
  )
  )
  self.store.insert_client_ip = simple_async_mock(None)
@@ -301,7 +303,9 @@ def test_get_user_by_req__puppeted_token__tracking_puppeted_mau(self):
  TokenLookupResult(
  user_id="@baldrick:matrix.org",
  device_id="device",
+ token_id=5,
  token_owner="@admin:matrix.org",
+ token_used=True,
  )
  )
  self.store.insert_client_ip = simple_async_mock(None)
@@ -347,7 +351,7 @@ def test_get_guest_user_from_macaroon(self):
  serialized = macaroon.serialize()
 
  user_info = self.get_success(self.auth.get_user_by_access_token(serialized))
- self.assertEqual(user_id, user_info.user_id)
+ self.assertEqual(user_id, user_info.user.to_string())
  self.assertTrue(user_info.is_guest)
  self.store.get_user_by_id.assert_called_with(user_id)