-
Notifications
You must be signed in to change notification settings - Fork 606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a basic user admin backend #10245
base: main
Are you sure you want to change the base?
Conversation
This provides an admin-only view of a user, including otherwise sensitive information such as their e-mail address and account lock status.
pub struct EncodableAdminUser { | ||
#[serde(flatten)] | ||
pub user: EncodablePrivateUser, | ||
pub lock: Option<EncodableUserLock>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the nested object instead of directly adding the two account_lock_reason
and account_lock_until
fields? seems a bit easier to handle to me 😅
/// Lock the given user. | ||
/// | ||
/// Only site admins can use this endpoint. | ||
#[utoipa::path( | ||
put, | ||
path = "/api/v1/users/{user}/lock", | ||
params( | ||
("user" = String, Path, description = "Login name of the user"), | ||
), | ||
request_body = LockRequest, | ||
tags = ["admin", "users"], | ||
responses((status = 200, description = "Successful Response")), | ||
)] | ||
pub async fn lock( | ||
state: AppState, | ||
Path(user_name): Path<String>, | ||
req: Parts, | ||
Json(LockRequest { reason, until }): Json<LockRequest>, | ||
) -> AppResult<Json<EncodableAdminUser>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for new APIs I would prefer to avoid such "custom action" API endpoints and use a PATCH
API instead (PATCH /api/v1/users/{user}/admin
?) like we started for versions too and also to some degree already have for users (using PUT
unfortunately...).
this will allow us to "easily" extend the functionality of this endpoint in the future instead of having to add new endpoints for basically every field we want to change.
{ | ||
"avatar": null, | ||
"email": "[email protected]", | ||
"email_verification_sent": true, | ||
"email_verified": true, | ||
"id": 1, | ||
"is_admin": false, | ||
"lock": null, | ||
"login": "foo", | ||
"name": null, | ||
"publish_notifications": true, | ||
"url": "https://github.com/foo" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the rest of the API is using e.g. { user: { ... } }
, which allows us to side-load other data from the same request in some cases. we should probably stay consistent with the other API endpoints.
This PR adds three new routes, all of them admin only:
GET /api/v1/users/:user_id/admin
: returns an extended version ofEncodablePrivateUser
with information on whether the account is lockedPUT /api/v1/users/:user_id/lock
: locks the given accountDELETE /api/v1/users/:user_id/unlock
: unlocks the given accountThe only part I expect to be slightly controversial here is the first one: in theory, we could extend the existing
GET /api/v1/users/:user_id
route to return the extra fields only to admins. But, practically, it feels cleaner and less error prone to add a new route that is entirely its own thing, although it reuses existing view and model infrastructure as much as it can.Unsurprisingly, this will be followed by some frontend PRs.
Notes for reviewers
This is probably best reviewed commit-by-commit. (I thought about splitting it into three or four PRs, but I think that's just increasing the review overhead, honestly.)
I will also note — slightly defensively — that the actual functional diff is only 247 lines; the remainder is the update to the OpenAPI snapshot, integration tests for the new controllers, and snapshots related to those tests.