From f5bc9694094786cce732568991bd09e8927d4594 Mon Sep 17 00:00:00 2001 From: Caleb Bae Date: Thu, 10 Sep 2026 05:24:44 +0000 Subject: [PATCH] fix: drop stale pending reviews and scope assignment toggle to super admins --- cmd/api/reviews.go | 16 ++++- cmd/api/reviews_test.go | 16 +++++ docs/docs.go | 4 +- internal/store/reviews.go | 41 ++++++++--- .../reviews_assignment_integration_test.go | 72 ++++++++++++++++++- 5 files changed, 133 insertions(+), 16 deletions(-) diff --git a/cmd/api/reviews.go b/cmd/api/reviews.go index 5f8d9430..c2fc09d6 100644 --- a/cmd/api/reviews.go +++ b/cmd/api/reviews.go @@ -167,12 +167,12 @@ func (app *application) batchAssignReviews(w http.ResponseWriter, r *http.Reques // getNextReview assigns and returns the next application needing review // // @Summary Get next review assignment (Admin) -// @Description Automatically assigns the next submitted application needing review to the current admin and returns it +// @Description Automatically assigns the next submitted application needing review to the current admin and returns it. Super admins who have disabled their review assignment toggle are refused. // @Tags admin/reviews // @Produce json // @Success 200 {object} ReviewResponse // @Failure 401 {object} object{error=string} -// @Failure 403 {object} object{error=string} +// @Failure 403 {object} object{error=string} "Review assignment disabled for this super admin" // @Failure 404 {object} object{error=string} "No applications need review" // @Failure 500 {object} object{error=string} // @Security CookieAuth @@ -180,6 +180,18 @@ func (app *application) batchAssignReviews(w http.ResponseWriter, r *http.Reques func (app *application) getNextReview(w http.ResponseWriter, r *http.Request) { user := getUserFromContext(r.Context()) + if user.Role == store.RoleSuperAdmin { + enabled, err := app.store.Settings.GetReviewAssignmentToggle(r.Context(), user.ID) + if err != nil { + app.internalServerError(w, r, err) + return + } + if !enabled { + app.forbiddenResponse(w, r, errors.New("review assignment is disabled for this account")) + return + } + } + reviewsPerApp, err := app.store.Settings.GetReviewsPerApplication(r.Context()) if err != nil { app.internalServerError(w, r, err) diff --git a/cmd/api/reviews_test.go b/cmd/api/reviews_test.go index f85f9b2a..64b03fc9 100644 --- a/cmd/api/reviews_test.go +++ b/cmd/api/reviews_test.go @@ -341,6 +341,22 @@ func TestGetNextReview(t *testing.T) { mockReviews.AssertExpectations(t) mockSettings.AssertExpectations(t) }) + + t.Run("should return 403 for a super admin with assignment disabled", func(t *testing.T) { + superAdmin := newSuperAdminUser() + + mockSettings.On("GetReviewAssignmentToggle", superAdmin.ID).Return(false, nil).Once() + + req, err := http.NewRequest(http.MethodGet, "/", nil) + require.NoError(t, err) + req = setUserContext(req, superAdmin) + + rr := executeRequest(req, http.HandlerFunc(app.getNextReview)) + checkResponseCode(t, http.StatusForbidden, rr.Code) + + mockReviews.AssertNotCalled(t, "AssignNextForAdmin", superAdmin.ID, 3) + mockSettings.AssertExpectations(t) + }) } func TestBatchAssignReviews(t *testing.T) { diff --git a/docs/docs.go b/docs/docs.go index 4d9e6eb2..92568bd4 100644 --- a/docs/docs.go +++ b/docs/docs.go @@ -1101,7 +1101,7 @@ const docTemplate = `{ "CookieAuth": [] } ], - "description": "Automatically assigns the next submitted application needing review to the current admin and returns it", + "description": "Automatically assigns the next submitted application needing review to the current admin and returns it. Super admins who have disabled their review assignment toggle are refused.", "produces": [ "application/json" ], @@ -1128,7 +1128,7 @@ const docTemplate = `{ } }, "403": { - "description": "Forbidden", + "description": "Review assignment disabled for this super admin", "schema": { "type": "object", "properties": { diff --git a/internal/store/reviews.go b/internal/store/reviews.go index d23225f6..ff6d581b 100644 --- a/internal/store/reviews.go +++ b/internal/store/reviews.go @@ -131,7 +131,8 @@ func (s *ApplicationReviewsStore) GetTravelStatusByReviewID(ctx context.Context, } // GetPendingByAdminID returns all reviews assigned to an admin that haven't been voted on yet, -// including application details for display +// including application details for display. Reviews on applications that have +// already been decided are omitted; the next BatchAssign removes them. func (s *ApplicationReviewsStore) GetPendingByAdminID(ctx context.Context, adminID string) ([]ApplicationReviewWithDetails, error) { ctx, cancel := context.WithTimeout(ctx, QueryTimeoutDuration) defer cancel() @@ -155,7 +156,7 @@ func (s *ApplicationReviewsStore) GetPendingByAdminID(ctx context.Context, admin FROM application_reviews ar JOIN applications a ON ar.application_id = a.id JOIN users u ON a.user_id = u.id - WHERE ar.admin_id = $1 AND ar.vote IS NULL + WHERE ar.admin_id = $1 AND ar.vote IS NULL AND a.status = 'submitted' ORDER BY ar.assigned_at ASC ` @@ -295,8 +296,11 @@ type BatchAssignmentResult struct { ReviewsUnfilled int `json:"reviews_unfilled"` } -// BatchAssign recovers inaccessible pending reviews and fills submitted +// BatchAssign recovers pending reviews that can no longer be acted on (reviewer +// disabled or demoted, application already decided) and fills submitted // applications' assignment targets with distinct, currently eligible reviewers. +// The assignment toggle only applies to super admins; entries for users who +// no longer hold that role are dropped so a demoted user is a regular admin. func (s *ApplicationReviewsStore) BatchAssign(ctx context.Context, reviewsPerApp int) (*BatchAssignmentResult, error) { ctx, cancel := context.WithTimeout(ctx, QueryTimeoutDuration*2) defer cancel() @@ -336,10 +340,14 @@ func (s *ApplicationReviewsStore) BatchAssign(ctx context.Context, reviewsPerApp result := &BatchAssignmentResult{ReviewsPerApplication: reviewsPerApp} removed, err := tx.ExecContext(ctx, ` DELETE FROM application_reviews ar - WHERE ar.vote IS NULL AND ( - ar.admin_id::text = ANY($1::text[]) OR NOT EXISTS ( + USING applications a + WHERE a.id = ar.application_id AND ar.vote IS NULL AND ( + a.status <> 'submitted' OR NOT EXISTS ( SELECT 1 FROM users u - WHERE u.id = ar.admin_id AND u.role IN ('admin', 'super_admin') + WHERE u.id = ar.admin_id AND ( + u.role = 'admin' OR + (u.role = 'super_admin' AND NOT (u.id::text = ANY($1::text[]))) + ) ) ) `, disabledIDs) @@ -354,7 +362,8 @@ func (s *ApplicationReviewsStore) BatchAssign(ctx context.Context, reviewsPerApp // Read workloads after cleanup. Creation time and ID provide stable ties. adminRows, err := tx.QueryContext(ctx, ` - SELECT u.id, u.role, COUNT(ar.id), NOT (u.id::text = ANY($1::text[])) + SELECT u.id, u.role, COUNT(ar.id), + NOT (u.role = 'super_admin' AND u.id::text = ANY($1::text[])) FROM users u LEFT JOIN application_reviews ar ON ar.admin_id = u.id AND ar.vote IS NULL WHERE u.role IN ('admin', 'super_admin') @@ -370,6 +379,7 @@ func (s *ApplicationReviewsStore) BatchAssign(ctx context.Context, reviewsPerApp Pending int } var admins []reviewer + superAdmins := make(map[string]bool) for adminRows.Next() { var admin reviewer var role UserRole @@ -377,8 +387,11 @@ func (s *ApplicationReviewsStore) BatchAssign(ctx context.Context, reviewsPerApp if err := adminRows.Scan(&admin.ID, &role, &admin.Pending, &enabled); err != nil { return nil, err } - if role == RoleSuperAdmin && !listed[admin.ID] { - entries = append(entries, ReviewAssignmentEntry{ID: admin.ID, Enabled: true}) + if role == RoleSuperAdmin { + superAdmins[admin.ID] = true + if !listed[admin.ID] { + entries = append(entries, ReviewAssignmentEntry{ID: admin.ID, Enabled: true}) + } } if enabled { admins = append(admins, admin) @@ -389,7 +402,15 @@ func (s *ApplicationReviewsStore) BatchAssign(ctx context.Context, reviewsPerApp } adminRows.Close() - // Normalize legacy settings and retain the super-admin backfill. + // Normalize legacy settings, retain the super-admin backfill, and drop + // entries for users who are no longer super admins. + current := entries[:0] + for _, entry := range entries { + if superAdmins[entry.ID] { + current = append(current, entry) + } + } + entries = current encoded, err := json.Marshal(entries) if err != nil { return nil, err diff --git a/internal/store/reviews_assignment_integration_test.go b/internal/store/reviews_assignment_integration_test.go index ae51c939..010742a7 100644 --- a/internal/store/reviews_assignment_integration_test.go +++ b/internal/store/reviews_assignment_integration_test.go @@ -292,8 +292,11 @@ func TestIntegrationBatchAssign(t *testing.T) { if _, err := (&UsersStore{db: db}).UpdateRole(ctx, admins[0], RoleHacker); err != nil { t.Fatal(err) } - } else if err := (&SettingsStore{db: db}).SetReviewAssignmentToggle(ctx, admins[0], false); err != nil { - t.Fatal(err) + } else { + batchTestExec(t, db, "UPDATE users SET role='super_admin' WHERE id=$1", admins[0]) + if err := (&SettingsStore{db: db}).SetReviewAssignmentToggle(ctx, admins[0], false); err != nil { + t.Fatal(err) + } } r, err := s.BatchAssign(ctx, 2) if err != nil { @@ -352,6 +355,71 @@ func TestIntegrationBatchAssign(t *testing.T) { } } }) + t.Run("decided_application_pending_reviews_removed", func(t *testing.T) { + db, s, admins, apps := batchTestSeed(t, 2, 2) + batchTestBatch(t, s, 2) + pending, err := s.GetPendingByAdminID(ctx, admins[0]) + if err != nil || len(pending) != 2 { + t.Fatalf("pending=%d err=%v", len(pending), err) + } + var decidedReview string + for _, p := range pending { + if p.ApplicationID == apps[0] { + decidedReview = p.ID + } + } + if _, err := s.SubmitVote(ctx, decidedReview, admins[0], ReviewVoteAccept, nil, nil); err != nil { + t.Fatal(err) + } + if _, err := (&ApplicationsStore{db: db}).SetStatus(ctx, apps[0], StatusAccepted); err != nil { + t.Fatal(err) + } + // The queue hides the decided application before any batch runs. + for _, admin := range admins { + pending, err := s.GetPendingByAdminID(ctx, admin) + if err != nil || len(pending) != 1 || pending[0].ApplicationID != apps[1] { + t.Fatalf("admin queue after decision=%+v err=%v", pending, err) + } + } + r, err := s.BatchAssign(ctx, 2) + if err != nil { + t.Fatal(err) + } + want := BatchAssignmentResult{ReviewsRemoved: 1, ReviewsPerApplication: 2} + if *r != want { + t.Errorf("result=%+v, want %+v", *r, want) + } + if n := batchTestCount(t, db, "SELECT count(*) FROM application_reviews WHERE application_id=$1 AND vote IS NOT NULL", apps[0]); n != 1 { + t.Errorf("completed reviews on decided application=%d, want 1", n) + } + if n := batchTestCount(t, db, "SELECT count(*) FROM application_reviews WHERE application_id=$1 AND vote IS NULL", apps[0]); n != 0 { + t.Errorf("pending reviews on decided application=%d, want 0", n) + } + }) + t.Run("demoted_disabled_super_admin_becomes_eligible_admin", func(t *testing.T) { + db, s, admins, _ := batchTestSeed(t, 2, 2) + batchTestExec(t, db, "UPDATE users SET role='super_admin' WHERE id=$1", admins[0]) + settings := &SettingsStore{db: db} + if err := settings.SetReviewAssignmentToggle(ctx, admins[0], false); err != nil { + t.Fatal(err) + } + if n := batchTestBatch(t, s, 2); n != 2 { + t.Fatalf("created=%d, want 2 (only the enabled admin)", n) + } + if _, err := (&UsersStore{db: db}).UpdateRole(ctx, admins[0], RoleAdmin); err != nil { + t.Fatal(err) + } + if n := batchTestBatch(t, s, 2); n != 2 { + t.Errorf("created=%d, want 2 for the demoted reviewer", n) + } + if n := batchTestCount(t, db, "SELECT count(*) FROM application_reviews WHERE admin_id=$1", admins[0]); n != 2 { + t.Errorf("demoted reviewer assignments=%d, want 2", n) + } + entries, err := settings.GetAllReviewAssignmentToggles(ctx) + if err != nil || len(entries) != 0 { + t.Errorf("stale toggle entries=%+v err=%v", entries, err) + } + }) t.Run("simultaneous_batches", func(t *testing.T) { for _, absent := range []bool{false, true} { t.Run(fmt.Sprint(absent), func(t *testing.T) {