From 8a72ee99687f957adc785f56c3ab64d315f3d664 Mon Sep 17 00:00:00 2001 From: "Calum H." Date: Sun, 3 May 2026 19:29:05 +0100 Subject: [PATCH] fix(backend): moderation locking logic fix (#5979) * fix(backend): moderation locking logic fix * fix: clippy --- apps/labrinth/src/routes/v3/projects.rs | 28 ++++++++++-------------- apps/labrinth/src/test/api_v2/project.rs | 2 +- apps/labrinth/src/test/api_v3/project.rs | 3 +-- apps/labrinth/tests/moderation_lock.rs | 24 +++++++++++++------- 4 files changed, 29 insertions(+), 28 deletions(-) diff --git a/apps/labrinth/src/routes/v3/projects.rs b/apps/labrinth/src/routes/v3/projects.rs index 3c9d978d5..a40f93305 100644 --- a/apps/labrinth/src/routes/v3/projects.rs +++ b/apps/labrinth/src/routes/v3/projects.rs @@ -424,28 +424,22 @@ pub async fn project_edit_internal( )); } - // If a moderator (non-admin) is completing a review (changing from Processing to another - // status), they must hold an active non-expired lock on this project. + // If a moderator (non-admin) is completing a review while another moderator holds an + // active checklist lock, block them from changing the project status. if user.role.is_mod() && !user.role.is_admin() && project_item.inner.status == ProjectStatus::Processing && status != &ProjectStatus::Processing - { - let lock = + && let Some(lock) = DBModerationLock::get_with_user(project_item.inner.id, &pool) - .await?; - let owns = lock.as_ref().is_some_and(|l| { - l.moderator_id == db_ids::DBUserId::from(user.id) && !l.expired - }); - if !owns { - return Err(ApiError::CustomAuthentication(match lock { - Some(l) => format!( - "This project is currently being moderated by @{}. Please wait for them to finish or for the lock to expire.", - l.moderator_username - ), - None => "You must hold an active moderation lock to complete this review. Open the project in the moderation checklist to acquire one.".to_string(), - })); - } + .await? + && lock.moderator_id != db_ids::DBUserId::from(user.id) + && !lock.expired + { + return Err(ApiError::CustomAuthentication(format!( + "This project is currently being moderated by @{}. Please wait for them to finish or for the lock to expire.", + lock.moderator_username + ))); } if status == &ProjectStatus::Processing { diff --git a/apps/labrinth/src/test/api_v2/project.rs b/apps/labrinth/src/test/api_v2/project.rs index fade15bee..665d5b109 100644 --- a/apps/labrinth/src/test/api_v2/project.rs +++ b/apps/labrinth/src/test/api_v2/project.rs @@ -95,7 +95,7 @@ impl ApiProject for ApiV2 { let resp = self.create_project(creation_data, pat).await; assert_status!(&resp, StatusCode::OK); - // Approve as admin so fixture setup is not blocked by the moderation-lock guard. + // Approve as admin so fixture setup is not affected by moderation-lock contention. let req = TestRequest::patch() .uri(&format!("/v2/project/{slug}")) .append_pat(ADMIN_USER_PAT) diff --git a/apps/labrinth/src/test/api_v3/project.rs b/apps/labrinth/src/test/api_v3/project.rs index fc26cdd60..2899bd095 100644 --- a/apps/labrinth/src/test/api_v3/project.rs +++ b/apps/labrinth/src/test/api_v3/project.rs @@ -49,8 +49,7 @@ impl ApiProject for ApiV3 { let resp = self.create_project(creation_data, pat).await; assert_status!(&resp, StatusCode::OK); - // Approve as admin so fixture setup is not blocked by the moderation-lock guard - // (non-admin moderators must hold a lock to move a project out of processing). + // Approve as admin so fixture setup is not affected by moderation-lock contention. let req = TestRequest::patch() .uri(&format!("/v3/project/{slug}")) .append_pat(ADMIN_USER_PAT) diff --git a/apps/labrinth/tests/moderation_lock.rs b/apps/labrinth/tests/moderation_lock.rs index 57cfe26f8..04e3ee8ec 100644 --- a/apps/labrinth/tests/moderation_lock.rs +++ b/apps/labrinth/tests/moderation_lock.rs @@ -769,9 +769,9 @@ async fn test_http_delete_all_locks_admin() { .await; } -/// Moderator without a lock cannot move a project out of processing (401 + lock message). +/// Moderator without a lock can move a project out of processing when nobody else has a lock. #[actix_rt::test] -async fn test_complete_review_requires_lock() { +async fn test_complete_review_without_lock_succeeds() { with_test_environment(None, |test_env: TestEnvironment| async move { let api = &test_env.api; let alpha_id = &test_env.dummy.project_alpha.project_id; @@ -793,8 +793,8 @@ async fn test_complete_review_requires_lock() { let resp = api.call(req).await; assert_eq!( resp.status(), - StatusCode::UNAUTHORIZED, - "moderator without lock should not be able to complete review: {}", + StatusCode::NO_CONTENT, + "moderator without lock should be able to complete review when no active lock exists: {}", String::from_utf8_lossy(test::read_body(resp).await.as_ref()) ); }) @@ -849,7 +849,7 @@ async fn test_complete_review_blocked_when_other_holds_lock() { } #[actix_rt::test] -async fn test_complete_review_fails_when_lock_expired() { +async fn test_complete_review_succeeds_when_other_lock_expired() { with_test_environment( None, |test_env: TestEnvironment| async move { @@ -858,7 +858,6 @@ async fn test_complete_review_fails_when_lock_expired() { let project_id = DBProjectId( test_env.dummy.project_alpha.project_id_parsed.0 as i64, ); - let mod_id = DBUserId(MOD_USER_ID_PARSED); let pool = &test_env.db.pool; set_project_processing( @@ -868,7 +867,11 @@ async fn test_complete_review_fails_when_lock_expired() { &test_env.db.redis_pool, ) .await; - DBModerationLock::acquire(project_id, mod_id, pool) + DBModerationLock::acquire( + project_id, + DBUserId(ADMIN_USER_ID_PARSED), + pool, + ) .await .unwrap() .unwrap(); @@ -881,7 +884,12 @@ async fn test_complete_review_fails_when_lock_expired() { .to_request(); let resp = api.call(req).await; - assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); + assert_eq!( + resp.status(), + StatusCode::NO_CONTENT, + "moderator should be able to complete review when another moderator's lock expired: {}", + String::from_utf8_lossy(test::read_body(resp).await.as_ref()) + ); }, ) .await;