Clean up synapse.api.auth.Auth

[sandhose]

I'm trying to make sense to what is in synapse.api.auth.Auth.
The goal is to extract a clear interface for verifying access tokens, so we can override that more easily for the OIDC work.

I already cleaned up things related to access token verification & macaroons in #12986

I'm finding a few method types:

• things related to checking if a user can do something:
  • check_user_in_room_or_world_readable
  • check_can_change_room_list
  • check_user_in_room
  • is_server_admin
• static methods to get the access token in a request
  • has_access_token
  • get_access_token_from_request
• methods to get the requester of a request
  • get_user_by_access_token
  • get_user_by_req
  • get_appservice_by_req

There is also check_auth_blocking, which only acts as a proxy to AuthBlocking.check_auth_blocking.
Note that is_server_admin is also a simple proxy to RegistrationWorkerStore.is_server_admin.

The other interesting thing I found out is that check_user_in_room is called from three places:

• check_user_in_room_or_world_readable (which makes sense)
• in handlers related to typing notifications (which also makes sense, since you can't send typing notifications in rooms you're not in)
• in rest.client.room.TimestampLookupRestServlet, which looks like an oversight, and should probably call check_user_in_room_or_world_readable instead

Other interesting things about a method: get_appservice_by_req is called from three places:

• /register, which seems not necessary, since a few lines bellow it would raise anyway if the appservice does not exist
• DELETE /directory/room/<room_alias>, for some reason?
• /login, which looks like the only legit place where it is called

So, what I would like to do is:

• remove the check_auth_blocking method, and use directly AuthBlocking.check_auth_blocking where it makes sense Decouple synapse.api.auth_blocking.AuthBlocking from synapse.api.auth.Auth. #13021
• move the two static methods as regular functions outside of the class
• make the TimestampLookupRestServlet use check_user_in_room_or_world_readable Allow MSC3030 'timestamp_to_event' calls from anyone on world-readable rooms. #13062
• group and move the permissions checks to their own class
• remove the usage of get_appservice_by_req:
  • Simplify the alias deletion logic as an application service. #13093
  • Make the AS login method call Auth.get_user_by_req for checking the AS token. #13094

[sandhose]

Another thought: I think it would make sense to make the permissions-related functions pass a Requester instead of just a UserID. This would help lay the foundations for having restricted access tokens, as the Requester would probably have scope informations in it. This is done in #13024

[clokep]

@sandhose Is this all set? I don't see any remaining work.

[DMRobertson]

The only outstanding thing from @sandhose's list is

• group and move the permissions checks to their own class

which might still be worthwhile?

[erikjohnston]

Let's close, if the last remaining item is valuable then lets open a new issue.