[US-11.9 / OBT-237] Let project managers manage member roles (not other managers) - #92
Conversation
71b39f7 to
6fb1756
Compare
Rework project access-role authorization to the role hierarchy: a manager of the project may grant a user with a role, promote a member to manager, and revoke a member, but may never demote or remove another manager (403); platform admins may act on anyone. Grant uses assert_can_grant_access; role-update and revoke use assert_can_modify_member_role. The grant role is restricted to member/manager.
6fb1756 to
b425778
Compare
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughProject access endpoints now use manager-specific authorization checks. Project roles are constrained to ChangesProject access authorization
Sequence Diagram(s)sequenceDiagram
participant Client
participant AccessEndpoints
participant ProjectAuthorization
participant ProjectUserAccess
Client->>AccessEndpoints: grant, update, or revoke access
AccessEndpoints->>ProjectAuthorization: validate actor and target
ProjectAuthorization->>ProjectUserAccess: query project role or access
ProjectUserAccess-->>ProjectAuthorization: access record or no record
ProjectAuthorization-->>AccessEndpoints: allow or raise authorization error
AccessEndpoints-->>Client: operation response or error
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/services/project/assert_can_grant_access.py`:
- Around line 8-13: Replace the separate assert_can_grant_access and
grant_user_access calls with a single transactional service operation that
performs authorization and grants access atomically. In that service, lock the
actor’s direct-access row before validating manager permissions, while
preserving the platform-admin path; update the route to delegate entirely to
this service and keep database/business-rule orchestration out of app/api.
In `@app/services/project/is_project_manager.py`:
- Line 8: Add a concise docstring to the public is_project_manager function
describing that it determines whether the specified user manages the specified
project and returns a boolean result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 3a9a5f98-7ebe-40a8-b490-7ccae2ce7692
📒 Files selected for processing (9)
app/api/projects/access.pyapp/models/project.pyapp/services/project/__init__.pyapp/services/project/assert_can_grant_access.pyapp/services/project/assert_can_modify_member_role.pyapp/services/project/get_user_project_access.pyapp/services/project/is_project_manager.pytests/baker.pytests/test_project_access_roles.py
| async def assert_can_grant_access(db: AsyncSession, actor: User, project_id: str) -> None: | ||
| """Only a platform admin or a manager of the project may grant project access.""" | ||
| if actor.is_platform_admin: | ||
| return | ||
| if not await is_project_manager(db, actor.id, project_id): | ||
| raise AuthorizationError("You must be a manager of this project") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Eliminate the authorization TOCTOU before granting access.
The shown route checks this helper and subsequently calls grant_user_access in a separate awaited operation. A manager can be revoked or demoted after Line 12 succeeds but before the grant is committed, allowing a no-longer-authorized actor to grant access. Move authorization and the grant into one transactional service operation and lock the actor’s direct-access row while validating it.
As per coding guidelines, app/api must not orchestrate business rules and database access must live in app/services.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/services/project/assert_can_grant_access.py` around lines 8 - 13, Replace
the separate assert_can_grant_access and grant_user_access calls with a single
transactional service operation that performs authorization and grants access
atomically. In that service, lock the actor’s direct-access row before
validating manager permissions, while preserving the platform-admin path; update
the route to delegate entirely to this service and keep database/business-rule
orchestration out of app/api.
Source: Coding guidelines
| from app.db.models.project import ProjectUserAccess | ||
|
|
||
|
|
||
| async def is_project_manager(db: AsyncSession, user_id: str, project_id: str) -> bool: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Document this public service function.
Proposed fix
async def is_project_manager(db: AsyncSession, user_id: str, project_id: str) -> bool:
+ """Return whether a user has the manager role for a project."""As per coding guidelines, public service functions need concise docstrings.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def is_project_manager(db: AsyncSession, user_id: str, project_id: str) -> bool: | |
| async def is_project_manager(db: AsyncSession, user_id: str, project_id: str) -> bool: | |
| """Return whether a user has the manager role for a project.""" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/services/project/is_project_manager.py` at line 8, Add a concise
docstring to the public is_project_manager function describing that it
determines whether the specified user manages the specified project and returns
a boolean result.
Source: Coding guidelines
|
@coderabbitai — decisions on this review:
|
levigtri
changed the title
|
Tip For best results, initiate chat on the files or code changes.
I'll keep these conventions in mind for future reviews in this repo. (=^･ω･^=) noted, thanks for walking through the reasoning! ✏️ Learnings added
|
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
Follow-up: stripped the existing docstrings from this PR's service files (assert_can_grant_access, assert_can_modify_member_role, get_user_project_access) to match the codebase's no-docstring convention. |
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
| ProjectUserAccessRoleUpdate, | ||
| ) | ||
| from app.services import project_service | ||
| from app.services.project import assert_can_grant_access, assert_can_modify_member_role |
There was a problem hiding this comment.
Maybe here you could call these two via project_service. like the rest of the file already does — you exported them on the __init__ in this same PR, and your own tests call them that way. I am not too sure we have direct imports like this on other routers, please verify that and if the namespaced usage is majority adapt this file.
…-role-changes-to-platform
2 participants
Summary
Reworks project access-role authorization to the 2026-07 role hierarchy: a manager of the project may now manage members — grant a user with a role, promote a member to manager, and revoke a member — but may never demote or remove another manager (that stays platform-admin-only). Platform admins may change or revoke any user's role on any project.
Changes
app/services/project/is_project_manager.py: true when the user holds themanagerrole on the project.app/services/project/get_user_project_access.py: returns a user's access row on a project (or None).app/services/project/assert_can_grant_access.py(admin or project-manager) andapp/services/project/assert_can_modify_member_role.py(admin, or project-manager acting on amember;403on a manager target,404on an unknown target).app/api/projects/access.py: grant usesassert_can_grant_access; role update and revoke useassert_can_modify_member_role.app/models/project.py:ProjectGrantUserAccess.roletightened toLiteral["member","manager"].tests/test_project_access_roles.py,tests/baker.py(role onmake_project_user_access): admin/manager/member × member/manager-target matrix.Type of Change
Testing
uv run ruff check ./uv run ruff format --check .— clean.uv run --group dev python -m pytest tests/test_project_access_roles.py tests/test_project_service.py— 37 passed.Summary by CodeRabbit
New Features
Tests