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

Clean up synapse.api.auth.Auth #13019

Closed
sandhose opened this issue Jun 10, 2022 · 4 comments
Closed

Clean up synapse.api.auth.Auth #13019

sandhose opened this issue Jun 10, 2022 · 4 comments
Assignees
Labels
T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.

Comments

@sandhose
Copy link
Member

sandhose commented Jun 10, 2022

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:

So, what I would like to do is:

@sandhose
Copy link
Member Author

sandhose commented Jun 10, 2022

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

@reivilibre reivilibre added the T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. label Jun 13, 2022
richvdh pushed a commit that referenced this issue Aug 22, 2022
…`Requester` instead of the `UserID` (#13024)

Part of #13019

This changes all the permission-related methods to rely on the Requester instead of the UserID. This is a first step towards enabling scoped access tokens at some point, since I expect the Requester to have scope-related informations in it.

It also changes methods which figure out the user/device/appservice out of the access token to return a Requester instead of something else. This avoids having store-related objects in the methods signatures.
@clokep
Copy link
Member

clokep commented Nov 28, 2022

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

@DMRobertson
Copy link
Contributor

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
Copy link
Member

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.
Projects
None yet
Development

No branches or pull requests

5 participants