rcsheets commented

Owner

The /admin/reviews list landed in #49 but every clickthrough 403'd
because the review-detail and rate handlers gated on
canUserSeeReview, which only lets the PR author or a requested
reviewer through. Admins viewing the cross-system list could see
that a review existed but never open it — inconsistent and
actionably broken.

Add an isAdmin parameter to canUserSeeReview that short-circuits the
involvement check, and a small sessionIsAdmin(r) helper so the two
existing call sites (review-detail GET and rate POST) can derive it
from request context. Behaviour for non-admins is unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com

The /admin/reviews list landed in #49 but every clickthrough 403'd because the review-detail and rate handlers gated on canUserSeeReview, which only lets the PR author or a requested reviewer through. Admins viewing the cross-system list could see that a review existed but never open it — inconsistent and actionably broken. Add an isAdmin parameter to canUserSeeReview that short-circuits the involvement check, and a small sessionIsAdmin(r) helper so the two existing call sites (review-detail GET and rate POST) can derive it from request context. Behaviour for non-admins is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

rcsheets added 1 commit

2026-05-01 10:33:42 +00:00

fix: dashboard admins can drill into any review from /admin/reviews

ci / check (pull_request) Successful in 43s

Details

e17d93e6de

The /admin/reviews list landed in #49 but every clickthrough 403'd
because the review-detail and rate handlers gated on
canUserSeeReview, which only lets the PR author or a requested
reviewer through. Admins viewing the cross-system list could see
that a review existed but never open it — inconsistent and
actionably broken.

Add an isAdmin parameter to canUserSeeReview that short-circuits the
involvement check, and a small sessionIsAdmin(r) helper so the two
existing call sites (review-detail GET and rate POST) can derive it
from request context. Behaviour for non-admins is unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

pr-reviewer-bot commented

2026-05-01 10:33:43 +00:00

Collaborator

Automated review by pr-reviewer v0.21.0 | Full Review | Claude Sonnet 4 | tracking id r-f48187-094f8b
This is an AI-generated review and may contain mistakes.

Status: ✅ Completed

✅ Verdict: Approve — The fix correctly addresses the access control inconsistency for admin users with clean implementation and proper security considerations.
Complexity: trivial

Code Review

Summary

This PR fixes a legitimate access control bug where dashboard admins could see review listings in /admin/reviews but couldn't actually view individual reviews due to overly restrictive permission checks. The solution is clean and maintains security boundaries appropriately.

What's Good ✅

Correct Problem Identification: The issue description accurately identifies the inconsistency - admins had a system-wide review list but couldn't drill down into individual reviews, which is broken UX.

Clean Implementation:

The sessionIsAdmin(r) helper is well-designed and reusable
Adding the isAdmin parameter to canUserSeeReview is the right approach
Early return pattern in canUserSeeReview maintains clean control flow

Security Considerations:

Admin bypass is appropriately scoped - only affects review visibility, not other operations
Non-admin behavior is completely unchanged (backwards compatible)
Uses existing admin group configuration (s.cfg.AdminGroup)

Documentation: The updated comment on canUserSeeReview clearly explains the admin bypass logic and provides good context about the broader access control design.

Implementation Details

The changes are minimal and focused:

Function signature update: canUserSeeReview now takes an isAdmin bool parameter
Admin short-circuit: Early return true if isAdmin is set
Helper function: sessionIsAdmin() extracts admin check logic for reuse
Call site updates: Both handleReviewDetail and handleRateReview now pass the admin flag

Architecture Alignment

This aligns well with the existing two-tier system mentioned in the project guidance - admins have elevated access to system-wide data, and this fix ensures that access is consistent across both listing and detail views.

Minor Observations

No tests are included, but this aligns with the project's stated approach of having no test suite
The fix is consistent with the pattern already established in admin_handlers.go where admins have broader access
Error messages remain user-friendly and don't leak sensitive information

Conclusion

This is a straightforward bug fix that resolves a real usability issue for admin users while maintaining proper security boundaries. The implementation is clean, well-documented, and follows existing patterns in the codebase.

📚 Context used in this review

Project guidance files:

CLAUDE.md

Repository fragments:

internal/dashboard/admin_handlers.go (internal/dashboard/admin_handlers.go:15-45) — extracted in answer to: "admin access patterns, admin group checking, and any existing admin review functionality"

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.21.0 | Full Review | Claude Sonnet 4 | tracking id `r-f48187-094f8b`* *This is an AI-generated review and may contain mistakes.* **Status:** ✅ Completed --- **✅ Verdict: Approve** — The fix correctly addresses the access control inconsistency for admin users with clean implementation and proper security considerations. **Complexity:** trivial # Code Review ## Summary This PR fixes a legitimate access control bug where dashboard admins could see review listings in `/admin/reviews` but couldn't actually view individual reviews due to overly restrictive permission checks. The solution is clean and maintains security boundaries appropriately. ## What's Good ✅ **Correct Problem Identification**: The issue description accurately identifies the inconsistency - admins had a system-wide review list but couldn't drill down into individual reviews, which is broken UX. **Clean Implementation**: - The `sessionIsAdmin(r)` helper is well-designed and reusable - Adding the `isAdmin` parameter to `canUserSeeReview` is the right approach - Early return pattern in `canUserSeeReview` maintains clean control flow **Security Considerations**: - Admin bypass is appropriately scoped - only affects review visibility, not other operations - Non-admin behavior is completely unchanged (backwards compatible) - Uses existing admin group configuration (`s.cfg.AdminGroup`) **Documentation**: The updated comment on `canUserSeeReview` clearly explains the admin bypass logic and provides good context about the broader access control design. ## Implementation Details The changes are minimal and focused: 1. **Function signature update**: `canUserSeeReview` now takes an `isAdmin bool` parameter 2. **Admin short-circuit**: Early return `true` if `isAdmin` is set 3. **Helper function**: `sessionIsAdmin()` extracts admin check logic for reuse 4. **Call site updates**: Both `handleReviewDetail` and `handleRateReview` now pass the admin flag ## Architecture Alignment This aligns well with the existing two-tier system mentioned in the project guidance - admins have elevated access to system-wide data, and this fix ensures that access is consistent across both listing and detail views. ## Minor Observations - No tests are included, but this aligns with the project's stated approach of having no test suite - The fix is consistent with the pattern already established in `admin_handlers.go` where admins have broader access - Error messages remain user-friendly and don't leak sensitive information ## Conclusion This is a straightforward bug fix that resolves a real usability issue for admin users while maintaining proper security boundaries. The implementation is clean, well-documented, and follows existing patterns in the codebase. <details> <summary>📚 Context used in this review</summary> **Project guidance files:** - `CLAUDE.md` **Repository fragments:** - `internal/dashboard/admin_handlers.go` (internal/dashboard/admin_handlers.go:15-45) — extracted in answer to: "admin access patterns, admin group checking, and any existing admin review functionality" </details>

rcsheets requested review from pr-reviewer-bot

2026-05-01 10:33:55 +00:00

rcsheets merged commit 5e3e46ac23 into main

2026-05-01 10:40:45 +00:00

rcsheets deleted branch fix/admin-can-view-all-reviews

2026-05-01 10:40:45 +00:00

rcsheets referenced this pull request from a commit

2026-05-01 10:40:47 +00:00

Merge pull request 'fix: dashboard admins can drill into any review from /admin/reviews' (#51) from fix/admin-can-view-all-reviews into main