- Notifications
You must be signed in to change notification settings - Fork655
Add a basic user admin backend#10245
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This provides an admin-only view of a user, including otherwisesensitive information such as their e-mail address and account lockstatus.
| 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 twoaccount_lock_reason andaccount_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 aPATCH API instead (PATCH /api/v1/users/{user}/admin?) like we started for versions too and also to some degree already have for users (usingPUT 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": "foo@example.com", | ||
| "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 ofEncodablePrivateUserwith 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_idroute 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 actualfunctional 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.