Initial implementation of admin tokens #6222
Conversation
hawkowl
changed the title
There was a problem hiding this comment.
a few bits and pieces below, but generally seems like a good idea.
One thing that I'd like to see asap is a bit of a design doc for it - what is the intention for what an admin token looks like, how is it used, how does the permissions model work. I don't think it's terribly complicated, but the act of explaining it in words would help ensure everyone is on the same page and that there aren't any obvious thinkos. I think that doc could either be a text file in the repo or a google doc.
| @@ -28,7 +28,7 @@ | |||
|
|
|||
| # Remember to update this number every time a change is made to database | |||
| # schema files, so the users will be informed on server restarts. | |||
| SCHEMA_VERSION = 56 | |||
| SCHEMA_VERSION = 57 | |||
There was a problem hiding this comment.
I don't think there's any need to bump the schema version?
There was a problem hiding this comment.
the comment implies I should?
There was a problem hiding this comment.
unfortunately I think the comment is wrong. If you read it at face value, we would bump it for every single delta file, which clearly we don't do.
The number should be updated when there is a backward-incompatible change, so that attempts to roll back will be met with a refusal to start rather than a subtle failure
| @@ -0,0 +1 @@ | |||
| Synapse now supports the use of API tokens when interfacing with administration APIs. These tokens can be namespaced to allow access to particular endpoints, and actions on those endpoints. | |||
There was a problem hiding this comment.
As per the company sync this week: we should avoid announcing new features until they are ready to be used. Could this just say "lay the groundwork for" or something and be a .misc?
| @@ -198,6 +198,11 @@ def read_config(self, config, **kwargs): | |||
| # due to resource constraints | |||
| self.admin_contact = config.get("admin_contact", None) | |||
|
|
|||
| # Admin user, a (possibly fake) MXID to use when an administrator token has been used. | |||
There was a problem hiding this comment.
please can this have doc in the sample config
| self.storage = hs.get_storage() | ||
| self.state_store = self.storage.state | ||
|
|
||
| async def validate_admin_token( |
There was a problem hiding this comment.
this should go in synapse.api.auth alongside the existing functions that handle auth tokens.
| self, servlet, request, raise_if_missing: bool = True | ||
| ) -> bool: | ||
| """ | ||
| Validate that there is an admin token on the request, and that it can |
There was a problem hiding this comment.
can you expand on what it returns, what the params mean, etc?
| authorised_by_token = await self.check_authorized_admin_token_in_use(request) | ||
| if not authorised_by_token: | ||
| await assert_requester_is_admin(self.auth, request) |
There was a problem hiding this comment.
can we just make check_authorized_admin_token_in_use also do the hoop-jumping to allow requests from admin users so we don't have to repeat this boilerplate every time?
There was a problem hiding this comment.
ideally, I'd like to remove matrix users entirely from being able to call things on /admin, since it's both inconvenient (none of this is stuff we want to expose through clients, so you have to do the login dance as an API client, and then you use that like an opaque token anyway) and also doesn't make sense when you think of administrators not being the same as the users (e.g. hosted solutions)
There was a problem hiding this comment.
well, that's a separate discussion. In the meantime, better to have it in one place than hundreds?
| @@ -287,6 +288,8 @@ class attribute containing a pre-compiled regular expression. The automatic | |||
| into the appropriate HTTP response. | |||
| """ | |||
|
|
|||
| PERMISSION_CODE = None | |||
There was a problem hiding this comment.
I feel like the whole PERMISSION_CODE thing is a bit opaque, compared with just passing the desired permission code into validate_admin_token.
| token_state = attr.ib(type=TokenState, default=TokenState.NON_EXISTANT) | ||
|
|
||
|
|
||
| class AdminTokenWorkerStore(SQLBaseStore): |
There was a problem hiding this comment.
why is this called WorkerStore ?
There was a problem hiding this comment.
er, i was intending to split this into two, so you can load the read-only parts in worker datastores
| @@ -122,11 +122,10 @@ def inject_redaction(self, room, event_id, user, reason): | |||
| return event | |||
|
|
|||
| def test_redact(self): | |||
| self.get_success( | |||
There was a problem hiding this comment.
not quite following why this is necessary or a good idea?
| * limitations under the License. | ||
| */ | ||
|
|
||
| CREATE TABLE IF NOT EXISTS admin_tokens ( |
There was a problem hiding this comment.
some comments about what is in these tables and columns would be good.
|
This branch has bitrotted rather badly and has not been updated in ~6 months. I'm going to close this. If this is still wanted, we can rebase it and reopen the PR. |
Admin tokens are a mechanism of doing admin-related tasks in Synapse without requiring an admin user. This makes sense when the application doing the admin tasks has no need to actually be a Matrix user, and when one wants to delegate portions of the admin API without giving over the whole shebang (which an admin user right now cannot do).
This PR adds the mechanisms under which admin tokens can be created and used, as well as adds token support for a number of admin endpoints (but not all of them). There is not yet a method for creating tokens via the REST API, that will follow separately.