feat(permissions): align project roles with UI access - #2095
Conversation
Make project roles usable across backend authorization and frontend navigation without granting system scopes.\n\n- Publish effective project scopes and use them for routes and controls.\n- Bind invitations to validated project roles and assign them atomically at registration.\n- Separate project Playground model availability from system Channel management.\n- Hide mutation controls from read-only resource views.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds project role presets and effective scopes, binds invitations to validated project roles, persists invitation ChangesProject permissions and role assignment
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant InviteDialog
participant InvitationAPI
participant InvitationService
participant Database
User->>InviteDialog: select project role
InviteDialog->>InvitationAPI: create invitation with roleID
InvitationAPI->>InvitationService: validate role and grant permission
InvitationService->>Database: store invitation roleID
User->>InvitationAPI: register invitation
InvitationAPI->>InvitationService: validate invitation role
InvitationService->>Database: assign role to created user
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThis PR aligns project-role permissions across the backend and frontend.
Confidence Score: 3/5The PR is not yet safe to merge because legacy invitations can still inherit customized Developer permissions and role replacement can permanently discard existing user assignments. The beta7 migration binds invitations to an existing mutable Developer role without normalizing its scopes, while its soft-deleted-role replacement path deletes surviving UserRole rows and does not attach those users to the replacement. Files Needing Attention: internal/ent/migrate/datamigrate/v1.0.0-beta7.go and internal/ent/migrate/datamigrate/v1.0.0-beta7_test.go
|
| Filename | Overview |
|---|---|
| internal/ent/migrate/datamigrate/v1.0.0-beta7.go | Backfills legacy invitation roles, but existing customized Developer roles and surviving assignments on replaced soft-deleted roles remain merge-safety concerns. |
| internal/ent/migrate/datamigrate/v1.0.0-beta8.go | Updates only Developer roles matching the prior preset while preserving customized roles. |
| internal/server/biz/invitation.go | Validates invitation roles and atomically creates membership and role assignment during registration. |
| internal/server/biz/user.go | Publishes effective project scopes assembled from direct membership and project-role scopes. |
| frontend/src/hooks/usePermissions.ts | Uses effective scopes and limits project-owner bypasses to the selected project. |
| frontend/src/features/playground/index.tsx | Separates project-approved channel models from system-level Model Gateway access. |
Sequence Diagram
sequenceDiagram
participant Migration as beta7 migration
participant DB as Database
participant Registration as Invitation registration
Migration->>DB: Find redeemable role-less invitations
Migration->>DB: Resolve or create Developer role
Migration->>DB: Persist role_id on invitation
Registration->>DB: Validate invitation and role
Registration->>DB: Create membership and assign stored role atomically
Reviews (24): Last reviewed commit: "fix(apikeys): avoid unauthorized project..." | Re-trigger Greptile
Keep invitations issued before role assignment usable and preserve stable React hook ordering in read-only status cells.\n\n- Backfill a legacy invitation with its project's Developer role during registration.\n- Run status-cell callbacks before permission-based rendering branches.\n- Align migration role-scope assertions with the new presets.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/features/channels/components/channels-columns.tsx (1)
60-84: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winHoist
handleSwitchClickabove the permission guard.
StatusSwitchCellcallsuseCallbackafterreturn !channelPermissions.canWrite ? ... : null. If permissions change between renders for the same memoized row, the hook order changes and React throws “Rendered fewer hooks than expected”. Move theuseCallbackbefore the early return, as done inOrderingWeightCell.🐛 Proposed fix
const { channelPermissions } = usePermissions(); const isEnabled = channel.status === 'enabled'; const isArchived = channel.status === 'archived'; + const handleSwitchClick = useCallback(() => { + if (!isArchived) { + setDialogOpen(true); + } + }, [isArchived]); + if (!channelPermissions.canWrite) { return <Badge variant='outline'>{channel.status}</Badge>; } - const handleSwitchClick = useCallback(() => { - if (!isArchived) { - setDialogOpen(true); - } - }, [isArchived]); - return (🤖 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 `@frontend/src/features/channels/components/channels-columns.tsx` around lines 60 - 84, Move the handleSwitchClick useCallback in StatusSwitchCell above the !channelPermissions.canWrite early return, keeping its existing isArchived guard and dependency array unchanged so hook order remains consistent across permission changes.frontend/src/features/models/components/models-columns.tsx (1)
21-45: 🩺 Stability & Availability | 🟠 Major | 💤 Low valueReturn the read-only badge after the callback definition in
StatusSwitchCell.
StatusSwitchCellcallsuseCallbackinside the!channelPermissions.canWritepath and skips that hook whencanWriteis true. Move the badge-return branch after defininghandleSwitchClick, or make this a row-level conditional renderer instead of a component.🤖 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 `@frontend/src/features/models/components/models-columns.tsx` around lines 21 - 45, Move the !channelPermissions.canWrite badge return in StatusSwitchCell below the handleSwitchClick useCallback so hooks execute consistently on every render. Preserve the existing read-only Badge output and the switch/dialog behavior for writable users.
🧹 Nitpick comments (1)
internal/server/biz/invitation_test.go (1)
32-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative-path coverage for the new role-grant checks.
Coverage now covers the happy path (role persisted, granted, and reflected in effective scopes) but the two new security-relevant branches introduced in
invitation.goare untested:CanGrantRolepermission denial inCreateInvitation, and the "invitation role is no longer available" branch inRegisterInvitation(role deleted after invitation creation, before registration).Also applies to: 53-55, 71-77
🤖 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 `@internal/server/biz/invitation_test.go` around lines 32 - 43, Extend the invitation tests around CreateInvitation and RegisterInvitation to cover both negative paths: assert that a caller lacking CanGrantRole is denied when creating an invitation, and assert that registration fails with the expected error when the invited role is deleted after invitation creation. Reuse createInvitationRole and the existing invitation setup, preserving the current happy-path assertions.
🤖 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 `@frontend/src/features/prompts/components/prompts-columns.tsx`:
- Around line 15-24: Move the useCallback call in StatusSwitchCell above the
!canWrite early return so hooks execute unconditionally on every render. Keep
the returned handler usage behind the canWrite guard, preserving the read-only
Badge behavior.
In `@internal/server/biz/user.go`:
- Around line 376-394: Update AddUserToProject so UserProject.Create and the
user.Update role assignment execute within a single RunInTransaction
transaction. Propagate any role-assignment error to roll back the membership
creation, and invalidate the user cache only after the transaction commits
successfully.
- Around line 383-389: Update the role-loading error handling around
RunWithSystemBypass: use ent.IsNotFound(err) to report the existing “project
role not found” error only when the query finds no role. For all other query
failures, return an error that preserves the original cause through wrapping,
rather than replacing it with the missing-role message.
In `@internal/server/gql/axonhub.resolvers.go`:
- Around line 754-765: Update the no-project branch of allChannelSummarys to
enforce the same canReadChannels and ScopeWriteRequests authorization checks as
the project-scoped path before querying channels. Ensure any fallback
ScopeWriteRequests query runs through authz.RunWithSystemBypass(...) when system
privileges are required, and deny or return appropriately when neither scope
permits access.
---
Outside diff comments:
In `@frontend/src/features/channels/components/channels-columns.tsx`:
- Around line 60-84: Move the handleSwitchClick useCallback in StatusSwitchCell
above the !channelPermissions.canWrite early return, keeping its existing
isArchived guard and dependency array unchanged so hook order remains consistent
across permission changes.
In `@frontend/src/features/models/components/models-columns.tsx`:
- Around line 21-45: Move the !channelPermissions.canWrite badge return in
StatusSwitchCell below the handleSwitchClick useCallback so hooks execute
consistently on every render. Preserve the existing read-only Badge output and
the switch/dialog behavior for writable users.
---
Nitpick comments:
In `@internal/server/biz/invitation_test.go`:
- Around line 32-43: Extend the invitation tests around CreateInvitation and
RegisterInvitation to cover both negative paths: assert that a caller lacking
CanGrantRole is denied when creating an invitation, and assert that registration
fails with the expected error when the invited role is deleted after invitation
creation. Reuse createInvitationRole and the existing invitation setup,
preserving the current happy-path assertions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b596455a-2ee5-4980-9732-47bbe49a7910
📒 Files selected for processing (33)
PERMISSION_UI_PLAN.mdfrontend/src/features/apikeys/components/apikeys-primary-buttons.tsxfrontend/src/features/channels/components/channels-columns.tsxfrontend/src/features/models/components/models-columns.tsxfrontend/src/features/playground/index.tsxfrontend/src/features/proejct-users/components/users-invite-dialog.tsxfrontend/src/features/project-roles/data/roles.tsfrontend/src/features/prompts/components/prompts-columns.tsxfrontend/src/gql/users.tsfrontend/src/hooks/usePermissions.tsfrontend/src/hooks/useRoutePermissions.tsfrontend/src/lib/permission-utils.tsfrontend/src/stores/authStore.tsinternal/ent/entql.gointernal/ent/internal/schema.gointernal/ent/invitation.gointernal/ent/invitation/invitation.gointernal/ent/invitation/where.gointernal/ent/invitation_create.gointernal/ent/invitation_update.gointernal/ent/migrate/schema.gointernal/ent/mutation.gointernal/ent/runtime/runtime.gointernal/ent/schema/invitation.gointernal/objects/user.gointernal/server/api/invitation.gointernal/server/biz/invitation.gointernal/server/biz/invitation_test.gointernal/server/biz/project.gointernal/server/biz/user.gointernal/server/gql/axonhub.resolvers.gointernal/server/gql/generated.gointernal/server/gql/me.graphql
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/ent/migrate/datamigrate/v0.3.0_test.go`:
- Around line 351-354: Extend the Viewer role assertions in the migration test
to verify its scopes exclude write access: add a NotContains assertion for
"write_prompts" alongside the existing read_prompts check, while preserving the
current query and error validation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 160e9588-22e9-4f29-a0f5-5475f8e21c0f
📒 Files selected for processing (6)
frontend/src/features/channels/components/channels-columns.tsxfrontend/src/features/models/components/models-columns.tsxfrontend/src/features/prompts/components/prompts-columns.tsxinternal/ent/migrate/datamigrate/v0.3.0_test.gointernal/server/biz/invitation.gointernal/server/biz/invitation_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/features/prompts/components/prompts-columns.tsx
- frontend/src/features/channels/components/channels-columns.tsx
- internal/server/biz/invitation.go
Keep role-less legacy invitations at their original membership-only access level and harden project membership and channel-summary authorization.\n\n- Make member creation and role assignment transactional.\n- Require system channel read scope without project context.\n- Cover invitation grant denial, deleted roles, and unauthenticated channel summaries.
Assert that the default Viewer role excludes write_prompts, preventing a migration regression from granting prompt write access to a read-only role.
Give role-less legacy invitations the Viewer-equivalent read scopes directly on their project membership. This preserves usable project access without synthesizing a role or granting write permissions.
|
The confidence score on a Greptile comment can't be updated after it's posted because GitHub review comments are immutable once created — we post them as a snapshot at review time and cannot edit them retroactively. What happens instead when you apply a fix:
Your fix to assign That new comment is the current unresolved one to address. The original 3/5 confidence score on the resolved thread is simply a historical artifact of when the comment was first generated. |
Migrate role-less invitations to their project's Viewer role before registration, preserving a usable explicit access path without inferring direct membership scopes. Reject unmigrated invitation rows so they cannot create partially authorized members.
Match legacy invitation backfills to the Developer role selected by default in the invitation UI. Recreate the standard project Developer role during migration when it is absent so upgrades do not reject existing invitations.
|
The Developer backfill is intentional product policy, not a registration-time privilege inference. Before role-bound invitations existed, a legacy invitation stored only project membership and no authorization metadata. In the current authorization model, that membership alone is not a meaningful project grant: backend resource policies require a direct project scope or a project-role scope, and the frontend derives navigation and controls from The invitation UI now requires an explicit role and selects This is the confirmed compatibility contract for non-revoked legacy invitations: they receive the new default Developer authorization. Please treat the role assignment as an intentional migration of missing legacy metadata rather than an unauthorized privilege expansion. |
|
That context makes the design completely clear — the migration is the authoritative source of the role assignment, I'll withdraw the concern. Thanks for the detailed explanation. |
Add GoDoc for the invitation API, invitation service, and user permission objects so the PR meets the repository's documentation coverage check.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/ent/migrate/datamigrate/v1.0.0-beta7.go`:
- Line 42: The legacy invitation query in the v1.0.0-beta7 migration should
select only roleless, non-revoked invitations. Update the Invitation.Query
filter by adding the existing revocation predicate, and add a regression test
verifying revoked roleless invitations are excluded from the backfill.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f88108b2-db43-448f-8833-d5f0b38b5f86
📒 Files selected for processing (8)
internal/build/VERSIONinternal/ent/migrate/datamigrate/migrator.gointernal/ent/migrate/datamigrate/v1.0.0-beta7.gointernal/ent/migrate/datamigrate/v1.0.0-beta7_test.gointernal/objects/user.gointernal/server/api/invitation.gointernal/server/biz/invitation.gointernal/server/biz/invitation_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/objects/user.go
- internal/server/api/invitation.go
|
@coderabbitai The previous Docstring Coverage warning was generated before commit |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Current commit and target commit =="
git rev-parse HEAD
git show -s --format='%H%n%P%n%s' a2ae73f7
echo
echo "== Files changed by a2ae73f7 =="
git diff-tree --no-commit-id --name-status -r a2ae73f7
echo
echo "== Documentation-related automation/configuration =="
rg -n -i -C 2 'docstring|godoc|golint|golangci|revive|comment.*coverage|coverage.*comment' \
-g '!*vendor*' -g '!internal/ent/**' . \
|| true
echo
echo "== Exported declarations and immediately preceding comments in affected Go files =="
for file in internal/server/api/invitation.go internal/server/biz/invitation.go internal/objects/user.go; do
if [ -f "$file" ]; then
echo "--- $file ---"
rg -n -B 2 -A 1 '^(type|func|var|const) [A-Z][A-Za-z0-9_]*' "$file" || true
fi
done
echo
echo "== Current diff after a2ae73f7 affecting these files =="
git diff --unified=3 a2ae73f7..HEAD -- \
internal/server/api/invitation.go \
internal/server/biz/invitation.go \
internal/objects/user.go \
|| trueLength of output: 153809 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== GoDoc additions in a2ae73f7 =="
git show --format=fuller --find-renames --unified=1 a2ae73f7 -- \
internal/objects/user.go \
internal/server/api/invitation.go \
internal/server/biz/invitation.go
echo
echo "== PR comments mentioning documentation coverage =="
gh api --paginate 'repos/looplj/axonhub/issues/2095/comments?per_page=100' \
--jq '.[] | select((.body // "") | test("Docstring Coverage|docstring coverage|68\\.57%"; "i")) | {user: .user.login, created_at, body, url: .html_url}' \
|| true
echo
echo "== Inline review comments mentioning documentation coverage =="
gh api --paginate 'repos/looplj/axonhub/pulls/2095/comments?per_page=100' \
--jq '.[] | select((.body // "") | test("Docstring Coverage|docstring coverage|68\\.57%"; "i")) | {user: .user.login, path, line, original_line, body, url: .html_url}' \
|| true
echo
echo "== Refreshed declaration count for the three reviewed files =="
python3 - <<'PY'
from pathlib import Path
import re
files = [
Path("internal/server/api/invitation.go"),
Path("internal/server/biz/invitation.go"),
Path("internal/objects/user.go"),
]
decl = re.compile(r'^(?:type|func|var|const)\s+([A-Z]\w*)\b')
total = documented = 0
for path in files:
lines = path.read_text().splitlines()
for i, line in enumerate(lines):
match = decl.match(line)
if not match:
continue
total += 1
name = match.group(1)
j = i - 1
while j >= 0 and not lines[j].strip():
j -= 1
if j >= 0 and lines[j].lstrip().startswith("//"):
block = []
while j >= 0 and lines[j].lstrip().startswith("//"):
block.append(lines[j].lstrip()[2:].strip())
j -= 1
if " ".join(reversed(block)).startswith(name):
documented += 1
print(f"{path}:{i + 1}: {name}")
print(f"GoDoc coverage: {documented}/{total} ({documented / total * 100:.2f}%)")
PYLength of output: 18403
At the current PR head ( |
Backfill only active role-less invitations and preserve customized Developer roles when applying the legacy default. Add regression coverage for both behaviors.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@greptile-apps The customized-Developer finding is a design misclassification. Project roles are mutable authorization policy by design: if an administrator customized the project's The beta7 migration binds active role-less invitations to the project's current CodeRabbit's separate finding is fixed in 52810ec: the migration now filters |
|
Acknowledged. Having reviewed 52810ec, both points are correct:
Both findings are resolved. The approach is sound. |
|
@greptile-apps Please re-run the full PR review now that both findings are resolved and publish a new Confidence Score for this PR. The customized-Developer concern was withdrawn as an intentional design decision, and the revoked-invitation backfill issue was fixed and confirmed in 52810ec. Please update the PR rating explicitly rather than only acknowledging the resolved comments. |
|
@greptile-apps Is this PR safe to merge? Please give your score in comment. |
|
Score: 8 / 10 — Safe to merge with awareness of one known trade-off SummaryThis is a thorough, well-iterated permissions PR. It went through 9+ committed fixups during review, and every P1/critical issue raised by both Greptile and CodeRabbit has been resolved. Here's the breakdown: ✅ Issues caught and fixed
|
|
这个PR我需要实际部署看效果再merge。 |
Ensure project owners receive project-level UI access and role mutations carry their selected project context. - Validate role edits against the target role project scopes. - Revoke pending invitations bound to roles in the same deletion transaction. - Cover project role permission validation and invitation revocation.
Limit beta7 Developer-role cleanup to projects with active role-less invitations. - Avoid deleting soft-deleted roles and UserRole history from unrelated projects. - Continue replacing stale project roles when invitation backfill requires it. - Add regression coverage for unrelated soft-deleted Developer roles.
Hide the project API key type from users without project administration scopes and enforce the same rule in the service layer. Add regression coverage for explicit and omitted project key types.
Update the default Developer role to read request records without project prompt management, and migrate unchanged legacy presets while preserving customized roles. Allow project request and API key managers to load model metadata with the selected project context so API key profiles remain usable.
Limit legacy invitation backfills to active, usable invitations and process each project once. Preserve customized and soft-deleted Developer roles and their assignments while creating a standard role for new invitees, preventing permission elevation, unique-key failures, and unrelated role cleanup.
This reverts commit cb285ed.
Expired and exhausted legacy invitations cannot be redeemed, so exclude them from the role backfill. This prevents invalid invitation rows from triggering Developer role cleanup.
|
The
The beta7 migration intentionally clears those orphaned Please withdraw this finding and re-evaluate the current PR head. The invalid-invitation finding has also been fixed: expired and exhausted invitations are excluded before the backfill can touch a project's roles, with a regression test. Given the resolved review findings and focused migration coverage, the current |
Regenerate Ent and GraphQL artifacts after merging the upstream schema and settings changes.
Remove the unused nested project selection from API key profile template queries. Project roles with read_api_keys can access templates without requiring read_projects.
|
@looplj 我觉得可以merge了。这个PR做了很多UI和权限一致性的工作。并且让邀请来的人都可以自动拥有指定角色（默认Developer）。这也让Developer这个角色不能随便删了，所以我在设计上做了限定。 我经过测试是能用的。避免了很多权限错误提示。邀请来的人也可以顺利登录系统，查看少量几个菜单，并且整个操作都不会有权限问题了。他们可以创建API key，创建或者加载profile，并且使用自己的API key。这一串流程就通了。 |
1 participant
Summary
Verification
Summary by CodeRabbit