Set can_request_admin API #3082
Conversation
Deploying matrix-authentication-service-docs with
|
| Latest commit: |
77713c7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f25110d9.matrix-authentication-service-docs.pages.dev |
| Branch Preview URL: | https://quenting-admin-user-can-requ.matrix-authentication-service-docs.pages.dev |
crates/handlers/src/admin/v1/mod.rs
Outdated
| @@ -45,6 +45,13 @@ where | |||
| "/users/by-username/:username", | |||
| get_with(self::users::by_username, self::users::by_username_doc), | |||
| ) | |||
| .api_route( | |||
| "/users/:id/set-can-request-admin", | |||
There was a problem hiding this comment.
not exactly major, but this feels like a mouthful of a name. Would it not be enough to call it set-admin or something?
I think it would make sense to call these users 'admin users' rather than 'can request admin users'. I think 'can request admin' gives the wrong vibe, that the user doesn't have any power, they can just request it from someone.
In actuality I think this just means they can request it at login time and will be given the power with no questions asked?
In fact, should this really be a separate endpoint set-admin or should this just be a POST or I guess PATCH on the user resource itself, similar to how I think Synapse does it?
to be honest, I think I find it slightly on the weird side to have each attribute its own API request, but I'm not a frequent user of the API either
There was a problem hiding this comment.
this feels like a mouthful of a name
It is, I guess I'm being pedantic here? Because before, in Synapse the admin flag really defined whether all the sessions of the users had admin access or not, whereas in MAS it's really 'can this user have admin access', if that makes sense?
In fact, should this really be a separate endpoint set-admin or should this just be a POST or I guess PATCH on the user resource itself, similar to how I think Synapse does it?
I did think the same, but I'm worried we'll end up with the horrible 'do everything' endpoint that Synapse has… https://github.com/element-hq/synapse/blob/develop/synapse/rest/admin/users.py#L254
We can always get a PUT/PATCH endpoint on user in later?
There was a problem hiding this comment.
putting off PUT/PATCH until later is fine with me, though I don't actually mind the 'everything' endpoint in Synapse so much TBH, maybe worth asking an actual user of the API though, but I suspect it suits some use cases better. One for later though!
I think it would be nice to just call it set-admin. I think the mental model of users being 'admins' is a little bit more suitable for the community etc, I suspect even if we called them can-request-admins, the community (and others) will abbreviate this to 'admin users' in everyday speech.
Closest comparison I can think of is the concept of admins on most Linux distros: their powers are latent, but they can escalate them with sudo. This feels a bit like that. But I'd still claim these users are called admins, rather than can-be-admins or so :).
I'd also say that 'admin user' sounds more powerful/dangerous than 'can-request-admin user'. If there's a misunderstanding here, the system fails closed/safe, which seems to me to be the preferable direction to fail in.
TBH I am applying my own connotations here that 'requests' means something that isn't guaranteed. If a request is guaranteed, it's not a request, it's a demand (new Python HTTP client name anyone? :D) Happy for someone else to have an opinion if wanted, we are going to be supporting this API for a while after all :)
| operation | ||
| .id("setCanRequestAdmin") | ||
| .summary("Set whether a user can request admin") | ||
| .description("Calling this endpoint will not have any effect on existing sessions, meaning that their existing sessions will keep admin access if they were granted it.") |
There was a problem hiding this comment.
this feels like a dangerous caveat and I somewhat think it would be better if it didn't exist, but I guess there is a good reason for it
There was a problem hiding this comment.
Same reason than above: 'can request admin' and admin scope on the tokens are completely disjoint. Maybe that design should be challenged at some point, but I'd rather call out the caveat here, and expect the API consumer to work around it themselves
sandhose
force-pushed
the
quenting/admin/user-can-request-admin
branch
from
b8884c2 to
07a80b7
Compare
sandhose
force-pushed
the
quenting/admin/user-can-request-admin
branch
from
07a80b7 to
77713c7
Compare
This adds a new API to flip the can_request_admin flag on a user.