[US-16.1 / OBT-252] Keep platform admins out of project access - #104
[US-16.1 / OBT-252] Keep platform admins out of project access#104levigtri wants to merge 5 commits into
Conversation
Platform admins already manage every project implicitly (all authorization guards short-circuit on is_platform_admin), so they must not hold a project_user_access row. Enforce this at the write choke point and the listing: - grant_user_access: load the target user and reject a platform admin with ValidationError (400); unknown user now raises NotFoundError (404). This also covers the oral-collector invite-acceptance path. - update_user_access_role: reject a platform-admin target with ValidationError (400). - create_project: grant the creator manager access only when the creator is not a platform admin, so admin-created projects have no admin member. - list_project_user_access: exclude platform admins from a project's members. No schema change.
Grant/role-update rejection (400), unknown-user 404, listing exclusion, and admin-creator getting no membership row.
|
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)
📝 WalkthroughWalkthroughProject access services now prevent platform admins from receiving project memberships or roles, exclude them from access listings, and avoid creating creator memberships for admin-created projects. New async tests cover these behaviors and unknown-user validation. ChangesPlatform admin project access
🚥 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.
🧹 Nitpick comments (2)
tests/test_platform_admin_project_access.py (1)
34-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unknown-user coverage for
update_user_access_role.Only
grant_user_accesshas a test for the unknown-user/NotFoundErrorpath (lines 23-31).update_user_access_roleresolves the user via the sameget_user_by_idhelper, so it should get a symmetric test.Suggested test addition
`@pytest.mark.asyncio` async def test_update_user_access_role_rejects_unknown_user(db_session) -> None: lang = await make_language(db_session, code="kos") project = await make_project(db_session, language_id=lang.id) with pytest.raises(NotFoundError, match=r"User .* not found"): await project_service.update_user_access_role( db_session, project.id, "00000000-0000-0000-0000-000000000000", "manager" )🤖 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 `@tests/test_platform_admin_project_access.py` around lines 34 - 41, Add a symmetric async test for project_service.update_user_access_role covering an unknown user ID: create the language and project, call the method with a nonexistent UUID, and assert NotFoundError with the existing “User .* not found” message pattern.app/services/project/grant_user_access.py (1)
15-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate platform-admin rejection logic across two services.
Both
grant_user_accessandupdate_user_access_rolefetch the target user and raiseValidationErrorwhenis_platform_adminis true, differing only in message text. Extracting a shared helper avoids the two copies drifting apart as this policy evolves.
app/services/project/grant_user_access.py#L15-L19: replace the inline check with a call to a shared helper (e.g.,assert_not_platform_admin(target, context="added to a project")).app/services/project/update_user_access_role.py#L15-L19: replace the inline check with the same shared helper (e.g.,assert_not_platform_admin(target, context="receive a project role")).Suggested shared helper
# e.g. app/services/user/guard_not_platform_admin.py from app.core.exceptions import ValidationError from app.db.models.auth import User def guard_not_platform_admin(user: User, action: str) -> None: if user.is_platform_admin: raise ValidationError( f"Platform admins cannot {action}; they already manage every project." )# grant_user_access.py - target = await get_user_by_id(db, user_id) - if target.is_platform_admin: - raise ValidationError( - "Platform admins cannot be added to a project; they already manage every project." - ) + target = await get_user_by_id(db, user_id) + guard_not_platform_admin(target, "be added to a project")# update_user_access_role.py - target = await get_user_by_id(db, user_id) - if target.is_platform_admin: - raise ValidationError( - "Platform admins cannot receive a project role; they already manage every project." - ) + target = await get_user_by_id(db, user_id) + guard_not_platform_admin(target, "receive a project role")🤖 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/grant_user_access.py` around lines 15 - 19, Extract the duplicated platform-admin validation into a shared helper, such as guard_not_platform_admin, preserving the existing ValidationError behavior while accepting an action/context for the message. In app/services/project/grant_user_access.py lines 15-19, replace the inline check with the helper using the project-addition context; apply the same replacement in app/services/project/update_user_access_role.py lines 15-19 using the project-role context.
🤖 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.
Nitpick comments:
In `@app/services/project/grant_user_access.py`:
- Around line 15-19: Extract the duplicated platform-admin validation into a
shared helper, such as guard_not_platform_admin, preserving the existing
ValidationError behavior while accepting an action/context for the message. In
app/services/project/grant_user_access.py lines 15-19, replace the inline check
with the helper using the project-addition context; apply the same replacement
in app/services/project/update_user_access_role.py lines 15-19 using the
project-role context.
In `@tests/test_platform_admin_project_access.py`:
- Around line 34-41: Add a symmetric async test for
project_service.update_user_access_role covering an unknown user ID: create the
language and project, call the method with a nonexistent UUID, and assert
NotFoundError with the existing “User .* not found” message pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: b5fcf480-835b-4533-8a41-7fa19278c20d
📒 Files selected for processing (5)
app/services/project/create_project.pyapp/services/project/grant_user_access.pyapp/services/project/list_project_user_access.pyapp/services/project/update_user_access_role.pytests/test_platform_admin_project_access.py
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai — decisions on this review:
|
|
Tip For best results, initiate chat on the files or code changes.
|
| .where(ProjectUserAccess.project_id == project_id) | ||
| .where( | ||
| ProjectUserAccess.project_id == project_id, | ||
| User.is_platform_admin.is_(False), |
There was a problem hiding this comment.
Your scope note says the team_size counter is not on main yet, but get_member_counts (oral_collector/project_member_counts.py) is there and counts the project_user_access rows raw, and it feeds member_count on GET /oral-collector/projects. So a legacy admin row stays inside the count while this line hides it from the list — the OC project card says 3 members and the members page shows 2. Could you verify that? Maybe it is another counter that you meant.
| role: str = "member", | ||
| ) -> ProjectUserAccess: | ||
| target = await get_user_by_id(db, user_id) | ||
| if target.is_platform_admin: |
There was a problem hiding this comment.
Just make sure here about the invite path: create_invite never looks at is_platform_admin, so an admin can still be invited, and now accept_invite raises this 400 before setting the status — the invite stays pending forever and only decline works. The body says the guard covers that path, but it covers it by failing. Is that correct? @levigtri
…t-of-project-access-not-listed
2 participants
[US-16.1 / OBT-252] Keep platform admins out of project access
Summary
Platform Admin accounts must never appear in the per-project access surface. They already manage every project implicitly — every authorization guard (
assert_project_access,assert_can_grant_access,list_projects_for_user,require_platform_admin) short-circuits on theusers.is_platform_adminboolean — so a platform admin does not need, and must not hold, aproject_user_accessrow. Today they leak in two ways: the project-creation flow writes the creator in asmanager(and creation is admin-driven), and the member listing/grant paths never look atis_platform_admin. This change makes the API the source of truth for the rule: admins can't be added to a project, can't be given a project role, and don't show up in a project's member list. Because admins keep full access through the boolean, nothing about their capabilities changes. No schema change → no Alembic migration.Enforcement lives at the
project_user_accesswrite choke point (grant_user_access), so it also covers the oral-collector invite-acceptance path, plus the direct role-update and listing services.Changes
app/services/project/grant_user_access.pyget_user_by_id, so an unknownuser_idnow raisesNotFoundError→ 404) and raisesValidationError(400) when the target is a platform admin, before any row is written. This is the single write path forproject_user_access, so the guard also covers oral-collector invite acceptance.app/services/project/update_user_access_role.pyValidationError(400) when the target user is a platform admin, before the access-row lookup.app/services/project/create_project.pycreator_user_idis set, the creator is grantedmanageraccess only if they are not a platform admin. A non-admin creator (e.g. a manager whose creation request was approved) still receivesmanager; an admin-created project has no admin member.app/services/project/list_project_user_access.pyUser.is_platform_admin.is_(False)to the join so any admin with a legacy access row is hidden from the project's member list.tests/test_platform_admin_project_access.py(new)Type of Change
Testing
uv run ruff check .anduv run ruff format --check .pass on the changed files.uv run --group dev python -m pytest tests/test_platform_admin_project_access.py— 5 passed (grant/role-update rejection, unknown-user 404, list exclusion, admin-creator no-membership).uv run --group dev python -m pytest tests/test_project_service.py tests/test_oc_invite_service.py— existing project-access and invite-acceptance behavior unchanged (non-admin grants, idempotency, listing, revoke, invite accept).Summary by CodeRabbit
Bug Fixes
Tests