feat: expand user role model to 4 tiers (admin/creator/operator/user)#150
feat: expand user role model to 4 tiers (admin/creator/operator/user)#150
Conversation
b8a9725 to
9c52191
Compare
9c52191 to
c9af39f
Compare
|
@vybe Ready for review |
vybe
left a comment
There was a problem hiding this comment.
PR Validation Report — REQUEST CHANGES
Validated by: Trinity PR Validator
Files Changed: 14 (+922/-138)
Summary
| Category | Status | Notes |
|---|---|---|
| Commit Messages | ✅ | Clear conventional feat: prefix |
| Roadmap | ✅ | Closes #143 (P1) |
| Requirements | ❌ | requirements.md not updated — ROLE-001 missing |
| Architecture | 2 new endpoints undocumented | |
| Feature Flows | role-model.md is thorough, but unrelated removals + doc regression |
|
| Feature Flow Format | ✅ | All required sections present |
| Security Check | ✅ | No leaked secrets |
| Code Quality | ❌ | Authorization regression + scope creep |
| Requirements Trace | ✅ | References #143 |
Critical — Block Merge
1. Authorization downgrade on destructive endpoints
Two owner-only endpoints lost their OwnedAgentByName access control:
| Endpoint | Before | After | Risk |
|---|---|---|---|
POST /{agent_name}/queue/clear |
OwnedAgentByName (owner-only) |
str + get_current_user |
Any authenticated user can clear any agent's queue |
POST /{agent_name}/queue/release |
OwnedAgentByName (owner-only) |
str + get_current_user |
Any authenticated user can force-release any agent |
Two read endpoints also lost AuthorizedAgentByName:
GET /{agent_name}/statsGET /{agent_name}/queue
These changes are unrelated to the role model feature and weaken authorization.
2. Scope creep — unrelated removals
The following removals are not related to the 4-tier role model and should be a separate PR:
voice_routerimport + registration (removes VOICE-001)event_subscriptions_routerimport + registration (removes EVT-001)VOICE_ENABLED+GEMINI_API_KEYconfig importsslack_transportshutdown logic in lifespan- Setup token printing in lifespan (SEC #177)
delete_agent_event_subscriptions()call in agent delete- Voice Chat, Agent Event Subscriptions, Slack Channel Routing entries removed from
feature-flows.md #209changelog content erased- MCP tool count changed 62 → 45+ without justification
3. Documentation regression in email-authentication.md
The PR removes documentation for OTP rate limiting, IP-based rate limiting on verification, and POST /api/public/verify/confirm. The OTP rate limiting code still exists in src/backend/routers/auth.py (check_otp_rate_limit, record_otp_attempt, lines 102-402). The documentation is now inaccurate.
Required Changes
- Restore
OwnedAgentByNameonPOST /{agent_name}/queue/clearandPOST /{agent_name}/queue/release(or replace with equivalent role-based auth) - Restore
AuthorizedAgentByNameonGET /{agent_name}/statsandGET /{agent_name}/queue - Split unrelated removals into a separate cleanup PR
- Restore OTP rate limiting documentation in
email-authentication.md - Add ROLE-001 entry to
docs/memory/requirements.md - Add new endpoints (
GET /api/users,PUT /api/users/{username}/role) todocs/memory/architecture.md
Suggestions (non-blocking)
datetime.utcnow()indb/users.py:233is deprecated in Python 3.12+ — usedatetime.now(timezone.utc)currentUsernamecomputed property uses fragile@localhostheuristic for admin detectiontest_update_role_invalid_valuetargets/api/users/admin/role— passes due to self-guard, not role validation. Target a different username.
The role model implementation itself (require_role factory, users router, Settings UI, tests) is solid. The issues above are about unrelated changes bundled into this PR.
- Add require_role(min_role) dependency factory with role hierarchy enforcement
- New GET /api/users and PUT /api/users/{username}/role endpoints (admin-only)
- Agent creation now requires creator role or above
- New email users default to creator role instead of user
- Settings UI: User Management section with role dropdowns per user
- Tests: test_role_model.py covering list users, role update, auth guards
- Docs: role-model.md feature flow, changelog entry
Closes #143
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Restore OwnedAgentByName on queue/clear and queue/release endpoints
- Restore AuthorizedAgentByName on stats and queue endpoints
- Restore voice_router, event_subscriptions_router, setup token printing,
VOICE_ENABLED config import, and event subscription cleanup on delete
- Restore OTP rate limiting documentation in email-authentication.md
- Add ROLE-001 entry to requirements.md (§2.5)
- Add GET /api/users and PUT /api/users/{username}/role to architecture.md
- Fix datetime.utcnow() → datetime.now() in update_user_role
- Improve currentUsername admin detection in Settings.vue
- Fix test_update_role_invalid_value to target non-admin user
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
c9af39f to
f83f76d
Compare
PR Validation Report — APPROVEPR: feature/143-role-model → main Summary
Documentation Checklist
Security Checklist
Prior Review Issues — All Resolved
Issues FoundCritical (Block Merge)None. Warnings (Review Required)
Suggestions (Optional)
RecommendationAPPROVE All critical issues from the prior review have been resolved. Authorization is intact, scope creep reverted, documentation is complete. The feature flow naming variance is cosmetic — all required content is present. |
Closes #143
Description
Brief description of changes.
Related Issue
Fixes #(issue number)
Type of Change
Testing
Checklist
Screenshots (if applicable)
Add screenshots to help explain your changes.