Skip to content
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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

LawnGnome
Copy link
Contributor

This PR adds three new routes, all of them admin only:

  • GET /api/v1/users/:user_id/admin: returns an extended version of EncodablePrivateUser with information on whether the account is locked
  • PUT /api/v1/users/:user_id/lock: locks the given account
  • DELETE /api/v1/users/:user_id/unlock: unlocks the given account

The 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.

@LawnGnome LawnGnome added C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works A-backend ⚙️ labels Dec 19, 2024
@LawnGnome LawnGnome requested a review from a team December 19, 2024 03:00
@LawnGnome LawnGnome self-assigned this Dec 19, 2024
pub struct EncodableAdminUser {
#[serde(flatten)]
pub user: EncodablePrivateUser,
pub lock: Option<EncodableUserLock>,
Copy link
Member

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 😅

Comment on lines +55 to +73
/// 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>> {
Copy link
Member

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.

Comment on lines +5 to +17
{
"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"
}
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants