IMPORTANT
Every PR MUST have exactly one of these labels, the merge is blocked otherwise:
major/minor/patchpublishes a new release on merge with the corresponding semver bump.skip-releasemerges without publishing a release (use for docs, CI changes, refactors, etc.).
📝 Description
An auditor who approved a review received 403 on PUT /api/reviews/dcdf3b10-8fbb-4813-a114-81dcb5ace66e. An admin on the same review succeeded. The gateway logs were:
forbidden access: PUT /api/reviews/dcdf3b10-8fbb-4813-a114-81dcb5ace66e, roles=[], email=auditor@example.com
access allowed: PUT /api/reviews/dcdf3b10-8fbb-4813-a114-81dcb5ace66e, roles=[], email=admin@example.com, isadmin=true, isauditor=falseroles=[] is the cause. PUT /reviews/:id and PUT /sessions/:session_id/review registered only AuthMiddleware, so no role was set on the route. isGroupAllowed allows a standard user when the role list is empty and denies an auditor. The admin passed because that function always allows the admin group.
GET /reviews and GET /reviews/:id already use ReadOnlyAccessRole, which allows a standard user, an admin, and an auditor. Both PUT routes now use the same middleware. An auditor who is not a member of the review's groups still receives 400 not eligible for review from doIndividualReview.
📣 User-facing impact
- An auditor can approve or deny a review they are eligible for. Before this change, that action returned 403.
🔗 Related Issue
Fixes #
🚀 Type of Change
- 🐛 Bug fix (non-breaking change which fixes an issue)
- ✨ New feature (non-breaking change which adds functionality)
- 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
- 📚 Documentation update
- 🎨 Style/UI update
- ♻️ Code refactor
- ⚡ Performance improvement
- ✅ Test update
- 🔧 Build configuration change
- 🧹 Chore
📋 Changes Made
- Add ReadOnlyAccessRole to PUT /reviews/:id.
- Add ReadOnlyAccessRole to PUT /sessions/:session_id/review.
🧪 Testing
Test Configuration:
- Browser(s):
- OS:
Tests performed:
- Unit tests pass
- Integration tests pass
- Manual testing completed
📸 Screenshots (if applicable)
✅ Checklist
- My code follows the style guidelines of this project
- I have performed a self-review of my own code
- I have commented my code, particularly in hard-to-understand areas
- My changes generate no new warnings
- New and existing unit tests pass locally with my changes
- I have checked my code and corrected any misspellings
📄 Additional Notes
Activity
6
qodo-code-review[bot]19d agoPR Summary by Qodo
Allow eligible auditors to approve or deny reviews
🐞 Bug fix 🕐 Less than 10 minutes
AI Description
-
• Allow auditors through both review update routes, which previously returned 403. • Preserve reviewer eligibility checks so auditors cannot decide reviews outside their groups.
Diagram
-
graph TD A["Review PUT routes"] --> B["ReadOnlyAccessRole"] --> C["AuthMiddleware"] --> D["Review handler"] --> E["Reviewer eligibility"]
High-Level Assessment
-
Reuse the existing ReadOnlyAccessRole middleware on both PUT routes. It matches the review read routes' access policy while leaving per-review eligibility enforcement in the handler; a new role policy is unnecessary.
Files changed (1) +2 / -0
-
Bug fix (1)
+2 / -0-
server.go
Grant auditors route access to review updates+2/-0
Grant auditors route access to review updates
• Adds ReadOnlyAccessRole before authentication on PUT /reviews/:id and PUT /sessions/:session_id/review. Eligible auditors can reach the existing review handler instead of being rejected at the route authorization check.
-
ducminhle19d agoI have read the CLA Document and I hereby sign the CLA
qodo-code-review[bot]19d agoCode Review by Qodo
🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (1) 📜 Skill insights (0)
1. Auditor approval contradicts access guidance 🔗 Cross-repo conflict ⛨ Security
Description
Adding ReadOnlyAccessRole to PUT /sessions/:session_id/review lets eligible auditors approve or reject a review, while the documentation repository describes the auditor group as having read-only access to session resources. Operators relying on that guidance may overlook this decision-making permission when assigning the auditor group.
Evidence
The changed route admits auditors to a review-writing handler; the documentation characterizes auditor session permissions as read-only.
hoop -> documentationgateway/api/server.go[1049-1054]gateway/api/review/review.go[356-363]External repo: hoophq/documentation, setup/configuration/idp/get-started.mdx [73-77]External repo: hoophq/documentation, setup/configuration/access-control-configuration.mdx [83-87]
Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution ## Issue description The auditor group can now submit session review decisions, but the documentation describes its session access as read-only. ## Fix Focus Areas - gateway/api/server.go[1049-1054] - /cross_repos/documentation/setup/configuration/idp/get-started.mdx[73-77] - /cross_repos/documentation/setup/configuration/access-control-configuration.mdx[83-87] ## Recommended Fix Coordinate a documentation update that distinguishes auditors' read access from their ability to approve or reject reviews when eligible.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Context sources
53 rules✅ Cross-repo context — repo relationships
repo: hoophq/documentation (sha: 10fc5a5b) — View relationshiprepo: hoophq/internal-runtime-control (sha: f9e2bc6f) — View relationshiprepo: hoophq/hoop-skills (sha: 372cfc31) — View relationshipAuto: ⚖️ Balanced: Authorization middleware changes access control on review mutation endpoints.Tip of the day
💡 Did you know, you can import rules and skills only from the repositories your team follows
All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.