feat: PBAC on team members page + org members page to use specific permissi… #23004
Conversation
…ons with fallbacks
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
WalkthroughIntroduces PBAC-first permission handling across frontend and backend. Adds MemberPermissions and SettingsPermissions types, a new getSpecificPermissions API for per-action checks (mixing CrudAction and CustomAction), scope-aware usePermissions, and new PBAC actions (ListMembers, Impersonate) in the permission registry. Wires permissions objects through settings layout, sidebar/tabs, members/team views, user/member tables, impersonation provider, and multiple server handlers (teams/orgs) with membership lookups and role-based fallbacks. Adds related Prisma migrations and locale keys for impersonation. Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
| ]); | ||
|
|
||
| // Get specific PBAC permissions for team member actions | ||
| const permissions = await getSpecificPermissions({ |
There was a problem hiding this comment.
New function to do the same as getResourcePermissions but we do not limit to basic CRUD permissions
|
|
||
| if (!team) return false; | ||
|
|
||
| if (isOrgAdminOrOwner && team?.parentId === orgId) { |
There was a problem hiding this comment.
isOrgAdminOrOwner just checks if youre admin of any org :O
dosubot
bot
added
organizations
Graphite Automations"Add platform team as reviewer" took an action on this PR • (08/12/25)1 reviewer was added to this PR based on Keith Williams's automation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (2)
apps/web/modules/members/members-view.tsx (1)
24-26: Add missing localization key across translation filesThe key
"only_admin_can_see_members_of_org"isn’t defined in any of our locale JSON/TS files. Please add it to each supported language pack. For example:• locales/en.json
{ …, "only_admin_can_see_members_of_org": "Only admins can see members of this organization." }• locales/es.json
{ …, "only_admin_can_see_members_of_org": "Sólo los administradores pueden ver los miembros de esta organización." }• (locales/fr.json, locales/de.json, etc.)
packages/features/ee/teams/components/MemberList.tsx (1)
639-639: Incorrect memorized columns dependencies.The
useMemodependency array includesprops.isOrgAdminOrOwnerbut this prop is no longer used within the memorized columns logic since it was replaced with PBAC permissions. The dependencies should includeprops.permissionsinstead.- }, [props.isOrgAdminOrOwner, dispatch, totalRowCount, session?.user.id]); + }, [props.permissions, dispatch, totalRowCount, session?.user.id]);
♻️ Duplicate comments (1)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsx (1)
84-110: LGTM! Well-implemented PBAC permission retrievalThe implementation correctly utilizes the new
getSpecificPermissionsfunction with proper fallback roles. The permission structure is well-organized for team member management.
🧹 Nitpick comments (12)
packages/features/users/components/UserTable/types.ts (1)
56-61: MemberPermissions type looks good; consider clarifying scope and immutability
- Nicely aligns with PBAC actions. Consider documenting whether these flags refer to Organization, Team, or context-derived scope to avoid misuse in consumers.
- Optional: mark fields as readonly to discourage mutation in UI code.
Example:
-export interface MemberPermissions { - canListMembers: boolean; - canInvite: boolean; - canChangeMemberRole: boolean; - canRemove: boolean; -} +export interface MemberPermissions { + /** Scope is context-dependent (org or team); document at call sites. */ + readonly canListMembers: boolean; + readonly canInvite: boolean; + readonly canChangeMemberRole: boolean; + readonly canRemove: boolean; +}packages/features/pbac/lib/resource-permissions.ts (2)
7-7: Minor: consolidate imports from permission-registryCombine Resource, CrudAction, and CustomAction into a single import for consistency.
-import type { Resource } from "../domain/types/permission-registry"; -import { CrudAction, CustomAction } from "../domain/types/permission-registry"; +import type { Resource } from "../domain/types/permission-registry"; +import { CrudAction, CustomAction } from "../domain/types/permission-registry";Or:
-import type { Resource } from "../domain/types/permission-registry"; -import { CrudAction, CustomAction } from "../domain/types/permission-registry"; +import { type Resource, CrudAction, CustomAction } from "../domain/types/permission-registry";
105-142: De-dupe actions and strengthen action map indexing
- If actions contains duplicates, we perform duplicate work. De-duplicate via Set.
- Indexing roleActions[action] can be typed more safely by asserting to a compatible map.
export const getSpecificPermissions = async ({ @@ -}: SpecificPermissionsOptions): Promise<Record<string, boolean>> => { +}: SpecificPermissionsOptions): Promise<Partial<Record<ActionType, boolean>>> => { @@ - if (!pbacEnabled) { - const permissions: Record<string, boolean> = {}; - for (const action of actions) { + if (!pbacEnabled) { + const permissions: Partial<Record<ActionType, boolean>> = {}; + for (const action of new Set(actions)) { permissions[action] = checkRoleAccess(userRole, fallbackRoles[action]); } return permissions; } @@ - const roleActions = PermissionMapper.toActionMap(resourcePermissions, resource); + const roleActions = PermissionMapper.toActionMap(resourcePermissions, resource) as Partial< + Record<ActionType, boolean> + >; @@ - const permissions: Record<string, boolean> = {}; - for (const action of actions) { + const permissions: Partial<Record<ActionType, boolean>> = {}; + for (const action of new Set(actions)) { permissions[action] = roleActions[action] ?? false; }packages/trpc/server/routers/viewer/organizations/bulkDeleteUsers.handler.ts (2)
37-39: Unauthorized message is fine; consider using a consistent error shapeCurrent message is explicit and helpful. Ensure consistency with other handlers’ UNAUTHORIZED messaging.
41-54: Fallback roles and PBAC check look correct; consider typing and reuse
- Mapping Remove to ADMIN/OWNER covers non-PBAC environments.
- Optionally, extract a shared helper for “get current org membership role (accepted only)” to avoid duplication across handlers.
packages/features/users/components/UserTable/UserListTable.tsx (1)
180-183: Consider renamingtablePermissionsto avoid confusion with thepermissionspropThe variable name
tablePermissionscould be confusing since it's actually the resolved permissions (PBAC or fallback), not specifically table-related permissions. Consider renaming it toresolvedPermissionsoreffectivePermissionsfor clarity.- const tablePermissions = { + const effectivePermissions = { canEdit: permissions?.canChangeMemberRole ?? adminOrOwner, canRemove: permissions?.canRemove ?? adminOrOwner, canResendInvitation: permissions?.canInvite ?? adminOrOwner,Then update all references from
tablePermissionstoeffectivePermissionsthroughout the component.packages/trpc/server/routers/viewer/teams/removeMember.handler.ts (1)
31-62: Good PBAC implementation but consider extracting permission checks to a helperThe PBAC permission checking logic is correctly implemented but could benefit from being extracted to a reusable helper function since similar patterns appear in other handlers.
Consider creating a shared helper function like
checkUserPermissionsForTeamsthat can be reused across different handlers to reduce code duplication and improve maintainability.packages/trpc/server/routers/viewer/teams/listMembers.handler.ts (1)
184-185: Document why Invite permission is used as a proxy for listing membersWhile the comment mentions using Invite as a proxy, it would be helpful to explain why
CustomAction.ListMembersisn't available for teams and why Invite was chosen as the proxy permission.- actions: [CustomAction.Invite], // Using Invite as proxy for member access since teams don't have ListMembers + actions: [CustomAction.Invite], // Using Invite as proxy for member access since teams don't have ListMembers action in the permission registry. Invite is used because users who can invite typically need to see existing members.packages/trpc/server/routers/viewer/organizations/listMembers.handler.test.ts (1)
39-51: Consider adding test cases for permission-denied scenarios.While the mock setup correctly returns
listMembers: true, the test suite would benefit from additional test cases that verify behavior when PBAC permissions are denied (e.g.,listMembers: false). This would ensure proper handling of unauthorized access attempts.Would you like me to generate additional test cases that cover:
- When
listMemberspermission is false- When the user has no membership in the organization
- Different permission combinations for comprehensive coverage?
packages/features/ee/teams/components/MemberList.tsx (1)
434-435: Track the impersonation permission TODO.The comment indicates that impersonation will get its own custom permission in a follow-up PR. This is a good approach for incremental migration.
Would you like me to create a GitHub issue to track the implementation of the custom impersonation permission for organizations and teams?
apps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsx (1)
46-50: Consider standardizing authentication redirects across pagesThe authentication check redirects to
/settings/profilewhen session/organization details are missing, while the teams page redirects to/auth/login. Consider implementing a consistent authentication strategy.Apply this diff to align with the team page's authentication pattern:
const session = await getServerSession({ req: buildLegacyRequest(await headers(), await cookies()) }); if (!session?.user.id || !session?.user.profile?.organizationId || !session?.user.org) { - return redirect("/settings/profile"); + return redirect("/auth/login"); }apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsx (1)
106-108: Consider broader access for listing team membersThe ListMembers fallback currently restricts to ADMIN and OWNER only. Team members typically expect to see their teammates for collaboration purposes.
Consider allowing MEMBER role to list team members, similar to the organization page:
[CustomAction.ListMembers]: { - roles: [MembershipRole.ADMIN, MembershipRole.OWNER], + roles: [MembershipRole.MEMBER, MembershipRole.ADMIN, MembershipRole.OWNER], },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx(9 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/layout.tsx(1 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsx(4 hunks)apps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsx(3 hunks)apps/web/modules/members/members-view.tsx(1 hunks)apps/web/modules/teams/team-members-view.tsx(3 hunks)packages/features/ee/teams/components/MemberList.tsx(4 hunks)packages/features/pbac/domain/types/permission-registry.ts(1 hunks)packages/features/pbac/lib/resource-permissions.ts(2 hunks)packages/features/users/components/UserTable/UserListTable.tsx(9 hunks)packages/features/users/components/UserTable/types.ts(1 hunks)packages/prisma/migrations/20250811123707_add_team_list_members_permission_to_admin_role/migration.sql(1 hunks)packages/trpc/server/routers/viewer/organizations/bulkDeleteUsers.handler.ts(2 hunks)packages/trpc/server/routers/viewer/organizations/getUser.handler.ts(2 hunks)packages/trpc/server/routers/viewer/organizations/listMembers.handler.test.ts(2 hunks)packages/trpc/server/routers/viewer/organizations/listMembers.handler.ts(2 hunks)packages/trpc/server/routers/viewer/teams/listMembers.handler.ts(3 hunks)packages/trpc/server/routers/viewer/teams/removeMember.handler.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/trpc/server/routers/viewer/organizations/bulkDeleteUsers.handler.tspackages/features/users/components/UserTable/types.tspackages/features/pbac/domain/types/permission-registry.tspackages/trpc/server/routers/viewer/organizations/getUser.handler.tspackages/trpc/server/routers/viewer/teams/listMembers.handler.tspackages/trpc/server/routers/viewer/organizations/listMembers.handler.tspackages/features/pbac/lib/resource-permissions.tspackages/trpc/server/routers/viewer/organizations/listMembers.handler.test.tspackages/trpc/server/routers/viewer/teams/removeMember.handler.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/trpc/server/routers/viewer/organizations/bulkDeleteUsers.handler.tspackages/features/users/components/UserTable/types.tspackages/features/pbac/domain/types/permission-registry.tsapps/web/modules/members/members-view.tsxpackages/trpc/server/routers/viewer/organizations/getUser.handler.tspackages/trpc/server/routers/viewer/teams/listMembers.handler.tsapps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/layout.tsxpackages/trpc/server/routers/viewer/organizations/listMembers.handler.tspackages/features/pbac/lib/resource-permissions.tsapps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsxpackages/trpc/server/routers/viewer/organizations/listMembers.handler.test.tspackages/features/users/components/UserTable/UserListTable.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsxpackages/features/ee/teams/components/MemberList.tsxapps/web/modules/teams/team-members-view.tsxpackages/trpc/server/routers/viewer/teams/removeMember.handler.ts
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
apps/web/modules/members/members-view.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/layout.tsxapps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsxpackages/features/users/components/UserTable/UserListTable.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsxpackages/features/ee/teams/components/MemberList.tsxapps/web/modules/teams/team-members-view.tsx
🧬 Code Graph Analysis (13)
packages/trpc/server/routers/viewer/organizations/bulkDeleteUsers.handler.ts (1)
packages/features/pbac/lib/resource-permissions.ts (1)
getSpecificPermissions(105-142)
apps/web/modules/members/members-view.tsx (2)
packages/features/users/components/UserTable/UserListTable.tsx (2)
UserListTableProps(110-126)UserListTable(128-134)packages/features/users/components/UserTable/types.ts (1)
MemberPermissions(56-61)
packages/trpc/server/routers/viewer/organizations/getUser.handler.ts (1)
packages/features/pbac/lib/resource-permissions.ts (1)
getSpecificPermissions(105-142)
packages/trpc/server/routers/viewer/teams/listMembers.handler.ts (3)
apps/api/v2/src/modules/organizations/memberships/services/organizations-membership.service.ts (1)
isOrgAdminOrOwner(30-39)packages/features/pbac/lib/resource-permissions.ts (1)
getSpecificPermissions(105-142)packages/platform/libraries/index.ts (1)
MembershipRole(98-98)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsx (4)
packages/features/auth/lib/getServerSession.ts (1)
getServerSession(32-136)packages/features/pbac/lib/resource-permissions.ts (1)
getSpecificPermissions(105-142)packages/platform/libraries/index.ts (1)
MembershipRole(98-98)apps/web/modules/teams/team-members-view.tsx (1)
TeamMembersView(30-70)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/layout.tsx (1)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx (1)
SettingsLayoutAppDirClient(802-856)
packages/trpc/server/routers/viewer/organizations/listMembers.handler.ts (1)
packages/features/pbac/lib/resource-permissions.ts (1)
getSpecificPermissions(105-142)
packages/features/pbac/lib/resource-permissions.ts (5)
packages/platform/libraries/index.ts (1)
MembershipRole(98-98)packages/features/flags/features.repository.ts (1)
FeaturesRepository(18-254)packages/platform/libraries/repositories.ts (1)
FeaturesRepository(9-9)packages/features/pbac/services/permission-check.service.ts (1)
PermissionCheckService(20-370)packages/features/pbac/domain/mappers/PermissionMapper.ts (1)
PermissionMapper(7-120)
apps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsx (6)
packages/features/auth/lib/getServerSession.ts (1)
getServerSession(32-136)apps/web/app/_trpc/context.ts (1)
createRouterCaller(18-22)packages/trpc/server/routers/viewer/organizations/_router.tsx (1)
viewerOrganizationsRouter(33-166)packages/features/pbac/lib/resource-permissions.ts (1)
getSpecificPermissions(105-142)packages/platform/libraries/index.ts (1)
MembershipRole(98-98)apps/web/modules/members/members-view.tsx (1)
MembersView(10-30)
packages/features/users/components/UserTable/UserListTable.tsx (3)
packages/features/users/components/UserTable/types.ts (1)
MemberPermissions(56-61)scripts/prepare-local-for-delegation-credentials-testing.js (1)
org(42-44)packages/features/users/components/UserTable/BulkActions/MassAssignAttributes.tsx (1)
MassAssignAttributesBulkAction(335-341)
packages/features/ee/teams/components/MemberList.tsx (1)
packages/features/users/components/UserTable/types.ts (1)
MemberPermissions(56-61)
apps/web/modules/teams/team-members-view.tsx (1)
packages/features/users/components/UserTable/types.ts (1)
MemberPermissions(56-61)
packages/trpc/server/routers/viewer/teams/removeMember.handler.ts (3)
packages/features/pbac/lib/resource-permissions.ts (1)
getSpecificPermissions(105-142)packages/platform/libraries/index.ts (2)
MembershipRole(98-98)TRPCError(56-56)packages/lib/server/queries/teams/index.ts (1)
isTeamOwner(409-418)
🔇 Additional comments (18)
packages/trpc/server/routers/viewer/organizations/bulkDeleteUsers.handler.ts (1)
2-4: PBAC imports and usage LGTMSwitching to Resource/CustomAction and getSpecificPermissions aligns with the new PBAC model.
packages/features/users/components/UserTable/UserListTable.tsx (2)
456-466: Good implementation of fine-grained permissions for user actionsThe permissions logic correctly handles the nuanced requirements for each action (edit, remove, impersonate, leave, resend invitation) by combining PBAC permissions with contextual checks like
isSelfanduser.accepted.
630-644: LGTM! Proper PBAC gating for bulk actionsThe bulk action visibility is correctly gated using the PBAC permissions with appropriate fallback to the
adminOrOwnercheck when permissions are not provided.packages/trpc/server/routers/viewer/organizations/getUser.handler.ts (1)
39-53: LGTM! Well-structured PBAC permission check with appropriate fallback rolesThe implementation correctly:
- Fetches PBAC permissions for viewing/editing organization members
- Provides sensible fallback roles for non-PBAC scenarios
- Uses the appropriate resource type (
Resource.Organization)packages/trpc/server/routers/viewer/teams/listMembers.handler.ts (1)
151-195: LGTM! Clean PBAC implementation with appropriate access control logicThe implementation correctly handles both organization-level and team-level access with proper PBAC checks and sensible fallbacks for private vs public teams.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx (2)
184-186: Good architectural decision to introduce SettingsPermissions typeCreating a dedicated
SettingsPermissionsinterface provides good extensibility for future permission additions and maintains clean separation of concerns.
234-234: LGTM! Proper PBAC gating for roles and permissions menuThe condition correctly checks both PBAC enablement and the user's permission to view roles before showing the menu item, preventing unauthorized access.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/layout.tsx (1)
70-74: LGTM! Clean permissions object structure for PBAC integration.The change from a boolean
canViewRolesprop to a nestedpermissionsobject follows good architectural patterns for extensibility. This allows for future permission additions without modifying the component's prop signature.packages/trpc/server/routers/viewer/organizations/listMembers.handler.test.ts (1)
12-13: LGTM! Comprehensive test setup for PBAC-enabled flows.The test correctly mocks the required Prisma methods (
findFirstfor membership lookup andfindManyfor attributes) and installs the necessary spies to support the new PBAC authentication flow.Also applies to: 21-22, 27-29
packages/trpc/server/routers/viewer/organizations/listMembers.handler.ts (3)
75-88: Good security improvement with membership-based access control.The implementation correctly validates membership before checking permissions, preventing unauthorized access attempts. The error messages are appropriately generic to avoid information leakage.
90-104: Well-structured PBAC permission check with appropriate fallback logic.The permission check correctly:
- Uses the new PBAC system when available
- Falls back to role-based access for backward compatibility
- Handles both private and public organization scenarios
The separation of concerns between membership validation and permission checking follows security best practices.
106-114: Confirm Frontend Handling of Permission-Denied Response
I ran searches forcanUserGetMembersandtrpc.viewer.organizations.listMembersin our.ts/.tsxfiles but didn’t find any matches. Please manually verify that every component calling this endpoint handles the{ canUserGetMembers: false, rows: [], meta: { totalRowCount: 0 } }case correctly. For example:
- Ensure any
useQuery(trpc.viewer.organizations.listMembers…)hooks checkcanUserGetMembersbefore rendering member data.- Show an appropriate empty state or disable list actions when permission is denied.
- Add or update frontend tests to cover this scenario.
apps/web/modules/members/members-view.tsx (1)
14-18: Clean implementation of PBAC permissions with backward compatibility.The implementation elegantly handles the transition to PBAC by:
- Checking PBAC permissions first when available
- Falling back to the existing role-based logic for backward compatibility
- Using the nullish coalescing operator appropriately
The comment clearly documents the dual-path logic.
packages/features/ee/teams/components/MemberList.tsx (2)
430-433: PBAC permissions correctly integrated for edit mode.The implementation properly uses PBAC permissions (
canChangeMemberRoleandcanRemove) to determine edit capabilities, replacing the previous role-based checks. The self-edit prevention (!isSelf) is correctly maintained.
715-724: Clean permission-based gating for the invite button.The Add Member button is now correctly gated by the
canInvitepermission instead of role checks, maintaining consistency with the PBAC approach throughout the component.apps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsx (1)
58-84: LGTM! Well-structured PBAC permission retrievalThe implementation correctly uses the new
getSpecificPermissionsfunction with appropriate fallback roles for backward compatibility. The permission structure aligns well with the standard member management actions.apps/web/modules/teams/team-members-view.tsx (1)
35-37: LGTM! Clean implementation of permission fallback logicThe implementation properly uses PBAC permissions when available and gracefully falls back to role-based checks. The comment clearly documents the behavior.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsx (1)
62-66: LGTM! Consistent authentication check implementationThe authentication check properly validates the session and redirects to the login page when authentication is missing.
apps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsx
Show resolved
Hide resolved
...risma/migrations/20250811123707_add_team_list_members_permission_to_admin_role/migration.sql
Show resolved
Hide resolved
packages/trpc/server/routers/viewer/organizations/bulkDeleteUsers.handler.ts
Show resolved
Hide resolved
| if (!permissions[CustomAction.Remove]) { | ||
| throw new TRPCError({ code: "UNAUTHORIZED" }); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Guard rails: prevent self-deletion and protect owners
Independent of PBAC, consider:
- Disallow deleting the acting user’s own account in bulk operations.
- Prevent deleting the last OWNER in an org.
Would you like me to propose a guarded implementation that filters input.userIds and validates owner counts before proceeding?
🏁 Script executed:
#!/bin/bash
# Locate the handler file and print the first 200 lines to inspect existing guards
fd bulkDeleteUsers.handler.ts --exec sed -n '1,200p' {} \;Length of output: 4103
Add guards to prevent self-deletion and preserve at least one OWNER
After the PBAC check, ensure that the acting user cannot delete themselves and that you never remove the last OWNER from an organization.
Locations to update:
- packages/trpc/server/routers/viewer/organizations/bulkDeleteUsers.handler.ts, immediately after the
if (!permissions[CustomAction.Remove])block.
Suggested diff:
if (!permissions[CustomAction.Remove]) {
throw new TRPCError({ code: "UNAUTHORIZED" });
}
+ // Prevent self-deletion
+ if (input.userIds.includes(currentUser.id)) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: "Cannot bulk‐delete the current user",
+ });
+ }
+
+ // Prevent deleting the last OWNER in the organization
+ const totalOwners = await prisma.membership.count({
+ where: { teamId: currentUserOrgId, role: MembershipRole.OWNER },
+ });
+ const ownersToRemove = await prisma.membership.count({
+ where: {
+ teamId: currentUserOrgId,
+ role: MembershipRole.OWNER,
+ userId: { in: input.userIds },
+ },
+ });
+ if (totalOwners - ownersToRemove < 1) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: "Cannot delete the last OWNER of the organization",
+ });
+ }These checks will filter out the current user’s ID and guard against removing the final OWNER before running the deletion transaction.
🤖 Prompt for AI Agents
In packages/trpc/server/routers/viewer/organizations/bulkDeleteUsers.handler.ts
around lines 55 to 57, after the existing PBAC guard (if
(!permissions[CustomAction.Remove]) ...), add two safeguards: first, filter the
incoming list of user IDs to remove to exclude ctx.session.user.id so the acting
user cannot delete themselves; second, before performing the delete transaction,
query the org membership roles for the target users and the current count of
OWNERs and ensure the operation will not remove the last OWNER (i.e., compute
remainingOwnerCount = totalOwnerCount - ownersInDeleteList and throw a
TRPCError({ code: "BAD_REQUEST" }) if remainingOwnerCount < 1). Apply these
checks prior to starting the deletion transaction and return a clear error when
the request would self-delete or remove the final OWNER.
| if (isRemovingSelf && hasRemovePermission) { | ||
| // Additional check: ensure they're not an owner trying to remove themselves | ||
| const isOwnerOfAnyTeam = await Promise.all( | ||
| teamIds.map(async (teamId) => await isTeamOwner(ctx.user.id, teamId)) | ||
| ).then((results) => results.some((result) => result)); | ||
|
|
||
| if (isOwnerOfAnyTeam) { | ||
| throw new TRPCError({ | ||
| code: "FORBIDDEN", | ||
| message: "You can not remove yourself from a team you own.", | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential logical issue: Self-removal check may be incorrect
The condition if (isRemovingSelf && hasRemovePermission) on line 103 within the non-PBAC block seems incorrect. If a user is removing themselves and has removal permission, why would you then check if they're an owner? The logic should prevent owners from removing themselves regardless of having permission.
// Check if user is trying to remove themselves from a team they own (prevent this)
- if (isRemovingSelf && hasRemovePermission) {
+ if (isRemovingSelf) {
// Additional check: ensure they're not an owner trying to remove themselves
const isOwnerOfAnyTeam = await Promise.all(
teamIds.map(async (teamId) => await isTeamOwner(ctx.user.id, teamId))
).then((results) => results.some((result) => result));
if (isOwnerOfAnyTeam) {
throw new TRPCError({
code: "FORBIDDEN",
message: "You can not remove yourself from a team you own.",
});
}
}🤖 Prompt for AI Agents
In packages/trpc/server/routers/viewer/teams/removeMember.handler.ts around
lines 103 to 115, the self-removal guard is wrongly gated by
hasRemovePermission; change the conditional so owners are prevented from
removing themselves regardless of permission (i.e., check if isRemovingSelf,
then check isTeamOwner for each teamId and throw FORBIDDEN if any true). Keep
the existing Promise.all owner checks and error details, but remove
hasRemovePermission from the outer if condition so owner-self-removal is always
blocked.
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
packages/features/ee/teams/components/MemberList.tsx (1)
638-639: Update useMemo dependencies to include permissions.Columns depend on
props.permissions(e.g., actions column behavior), but the dependency array omits it. This risks stale UI when permissions change.- }, [props.isOrgAdminOrOwner, dispatch, totalRowCount, session?.user.id]); + }, [props.isOrgAdminOrOwner, props.permissions, dispatch, totalRowCount, session?.user.id]);apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx (1)
232-239: Missing useMemo dependencies: include isPbacEnabled and permissions.The tabs list is memoized without
isPbacEnabledandpermissions?.canViewRolesin the dependency array. Sidebar won’t update if PBAC toggles or permissions change.Apply this diff to the dependency array:
- }, [isAdmin, orgBranding, isOrgAdminOrOwner, user, isDelegationCredentialEnabled]); + }, [ + isAdmin, + orgBranding, + isOrgAdminOrOwner, + user, + isDelegationCredentialEnabled, + isPbacEnabled, + permissions?.canViewRoles, + ]);
♻️ Duplicate comments (2)
packages/trpc/server/routers/viewer/teams/listMembers.handler.ts (1)
151-157: Security concern:isOrgAdminOrOwnercheck is overly permissive.The current implementation of
isOrgAdminOrOwner(Line 151) only checksctx.user.organization?.isOrgAdmin, which doesn't verify if the user is an admin of this specific organization. This could allow an admin from any organization to access members of teams in other organizations.Consider verifying the user's admin status for the specific organization:
- const isOrgAdminOrOwner = ctx.user.organization?.isOrgAdmin; - const isTargetingOrg = teamId === ctx.user.organizationId; - - // If targeting org and user is org admin, allow access - if (isTargetingOrg && isOrgAdminOrOwner) { - return true; - } + const isTargetingOrg = teamId === ctx.user.organizationId; + + // If targeting org and user is org admin of THIS org, allow access + if (isTargetingOrg && ctx.user.organization?.isOrgAdmin) { + // Additional check: verify this is the correct org + const orgMembership = await prisma.membership.findFirst({ + where: { + teamId: ctx.user.organizationId, + userId: ctx.user.id, + role: { in: [MembershipRole.ADMIN, MembershipRole.OWNER] }, + accepted: true, + }, + }); + if (orgMembership) return true; + }apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsx (1)
85-110: Well-structured PBAC permission configuration.The implementation correctly:
- Uses the new
getSpecificPermissionsfunction as documented- Provides appropriate fallback roles for non-PBAC scenarios
- Maps all necessary team member actions (Invite, ChangeMemberRole, Remove, ListMembers)
🧹 Nitpick comments (12)
packages/trpc/server/routers/viewer/teams/removeMember.handler.ts (2)
34-42: Add null check for user membership.The code fetches the user's membership but doesn't handle the case where the user might not be a member of the team. While Line 44 returns
falsefor this case, it would be clearer to handle this explicitly.Consider adding an explicit check and error message:
const membership = await prisma.membership.findFirst({ where: { userId: ctx.user.id, teamId: teamId, }, select: { role: true, }, }); - if (!membership) return false; + if (!membership) { + // User is not a member of this team + return false; + }
47-58: Consider caching PBAC permissions for multiple teams.When removing members from multiple teams, the code makes separate PBAC permission checks for each team. Consider batching these checks or implementing a caching strategy if this becomes a performance bottleneck.
For future optimization, consider implementing a batch permission check method that can retrieve permissions for multiple teams in a single query, especially if users frequently remove members from multiple teams simultaneously.
packages/trpc/server/routers/viewer/organizations/listMembers.handler.test.ts (2)
12-12: Mock should return a more complete membership object.The
findFirstmock only returns{ role: "ADMIN" }, but the actual code might expect additional fields liketeamId,userId, etc.Consider returning a more complete mock object:
- findFirst: vi.fn().mockResolvedValue({ role: "ADMIN" }), + findFirst: vi.fn().mockResolvedValue({ + role: "ADMIN", + userId: 1, + teamId: ORGANIZATION_ID, + accepted: true + }),
39-44: Consider making the PBAC mock more flexible.The PBAC permissions mock always returns
{ listMembers: true }. For more comprehensive testing, consider making it configurable to test both permitted and denied scenarios.Make the mock more flexible for different test scenarios:
// Mock PBAC permissions +const mockGetSpecificPermissions = vi.fn().mockResolvedValue({ + listMembers: true, +}); + vi.mock("@calcom/features/pbac/lib/resource-permissions", () => ({ - getSpecificPermissions: vi.fn().mockResolvedValue({ - listMembers: true, - }), + getSpecificPermissions: mockGetSpecificPermissions, }));This allows individual tests to override the behavior:
mockGetSpecificPermissions.mockResolvedValueOnce({ listMembers: false });apps/web/modules/members/members-view.tsx (2)
7-7: Consider using a more specific import pathInstead of importing from the general types file, consider importing
MemberPermissionsfrom a more specific location if it exists, to improve code organization and reduce coupling.
14-18: Clarify the complex permission logic with better variable namingThe permission check logic is complex with multiple conditions. Consider extracting the fallback logic into a separate variable for better readability.
- // Use PBAC permissions if available, otherwise fall back to role-based check - const isOrgAdminOrOwner = props.org && checkAdminOrOwner(props.org.user.role); - const canLoggedInUserSeeMembers = - permissions?.canListMembers ?? - ((props.org?.isPrivate && isOrgAdminOrOwner) || isOrgAdminOrOwner || !props.org?.isPrivate); + // Use PBAC permissions if available, otherwise fall back to role-based check + const isOrgAdminOrOwner = props.org && checkAdminOrOwner(props.org.user.role); + const roleBasedCanSeeMembers = props.org?.isPrivate + ? isOrgAdminOrOwner + : true; + const canLoggedInUserSeeMembers = permissions?.canListMembers ?? roleBasedCanSeeMembers;packages/trpc/server/routers/viewer/organizations/getUser.handler.ts (1)
34-36: Consider using a constant for the error messageThe error message "User is not a member of this organization." is hardcoded. Consider extracting it to a constant or using localization for consistency across the codebase.
packages/trpc/server/routers/viewer/organizations/listMembers.handler.ts (1)
106-114: Consider extracting the empty response to a constantThe empty response object with
canUserGetMembers: falsecould be extracted to a constant for reusability and consistency.+const UNAUTHORIZED_RESPONSE = { + canUserGetMembers: false, + rows: [], + meta: { + totalRowCount: 0, + }, +}; if (!permissions[CustomAction.ListMembers]) { - return { - canUserGetMembers: false, - rows: [], - meta: { - totalRowCount: 0, - }, - }; + return UNAUTHORIZED_RESPONSE; }packages/features/users/components/UserTable/UserListTable.tsx (1)
456-456: Consider renamingpermissionsRawfor clarityThe variable name
permissionsRawdoesn't clearly indicate its purpose. Consider renaming it to better reflect that it contains the table-level permissions.- const permissionsRaw = tablePermissions; + const tablePermissionsForRow = tablePermissions;apps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsx (2)
46-51: Avoid unnecessary awaits on headers() and cookies().
headers()andcookies()are synchronous in the Next.js app router. Droppingawaitavoids confusion and slightly reduces overhead.- const session = await getServerSession({ req: buildLegacyRequest(await headers(), await cookies()) }); + const session = await getServerSession({ req: buildLegacyRequest(headers(), cookies()) });
58-85: Keepsession.user.profile.organizationIdasteamIdand confirm ListMembers fallback semanticsOur search shows that most pages (general, dsync, sso, privacy, attributes, members, etc.) consistently use
session.user.profile.organizationIdfor the organization/team ID. Switching tosession.user.org.idhere would be inconsistent with the rest of the codebase.However, the updated fallback for
CustomAction.ListMembersnow grants members the ability to list org members when PBAC is disabled, which differs from the previous privacy-based gating inMembersView. Please confirm that this broader access forMEMBERroles in private orgs is intentional.• Location to review:
apps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsx (lines 61–69)
• Fallback mapping in question:fallbackRoles: { [CustomAction.ListMembers]: { roles: [MembershipRole.MEMBER, MembershipRole.ADMIN, MembershipRole.OWNER], }, // … },No change needed for
teamId. Please verify the intended behavior for theListMembersfallback.packages/features/ee/teams/components/MemberList.tsx (1)
165-166: Prop surface extension looks good, but consider deduplicating Props.You now declare
interface Propstwice in this file (Lines 66-70 and 150-166). TypeScript merges them, but it’s a code smell. Prefer consolidating into a single interface for maintainability.Example consolidated interface (apply outside the shown hunk):
interface Props { team: NonNullable<RouterOutputs["viewer"]["teams"]["get"]>; isOrgAdminOrOwner: boolean | undefined; setShowMemberInvitationModal: Dispatch<SetStateAction<boolean>>; facetedTeamValues?: { roles: { id: string; name: string }[]; teams: RouterOutputs["viewer"]["teams"]["get"][]; attributes: { id: string; name: string; options: { value: string }[] }[]; }; permissions: MemberPermissions; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx(9 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/layout.tsx(1 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsx(4 hunks)apps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsx(3 hunks)apps/web/modules/members/members-view.tsx(1 hunks)apps/web/modules/teams/team-members-view.tsx(3 hunks)packages/features/ee/teams/components/MemberList.tsx(4 hunks)packages/features/pbac/domain/types/permission-registry.ts(1 hunks)packages/features/pbac/lib/resource-permissions.ts(2 hunks)packages/features/users/components/UserTable/UserListTable.tsx(9 hunks)packages/features/users/components/UserTable/types.ts(1 hunks)packages/prisma/migrations/20250811123707_add_team_list_members_permission_to_admin_role/migration.sql(1 hunks)packages/trpc/server/routers/viewer/organizations/bulkDeleteUsers.handler.ts(2 hunks)packages/trpc/server/routers/viewer/organizations/getUser.handler.ts(2 hunks)packages/trpc/server/routers/viewer/organizations/listMembers.handler.test.ts(2 hunks)packages/trpc/server/routers/viewer/organizations/listMembers.handler.ts(2 hunks)packages/trpc/server/routers/viewer/teams/listMembers.handler.ts(3 hunks)packages/trpc/server/routers/viewer/teams/removeMember.handler.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/trpc/server/routers/viewer/organizations/bulkDeleteUsers.handler.tspackages/features/users/components/UserTable/types.tspackages/trpc/server/routers/viewer/organizations/getUser.handler.tspackages/features/pbac/domain/types/permission-registry.tspackages/trpc/server/routers/viewer/organizations/listMembers.handler.test.tspackages/trpc/server/routers/viewer/organizations/listMembers.handler.tspackages/trpc/server/routers/viewer/teams/listMembers.handler.tspackages/features/pbac/lib/resource-permissions.tspackages/trpc/server/routers/viewer/teams/removeMember.handler.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/trpc/server/routers/viewer/organizations/bulkDeleteUsers.handler.tspackages/features/users/components/UserTable/types.tspackages/trpc/server/routers/viewer/organizations/getUser.handler.tsapps/web/modules/members/members-view.tsxpackages/features/pbac/domain/types/permission-registry.tspackages/trpc/server/routers/viewer/organizations/listMembers.handler.test.tspackages/trpc/server/routers/viewer/organizations/listMembers.handler.tspackages/trpc/server/routers/viewer/teams/listMembers.handler.tsapps/web/app/(use-page-wrapper)/settings/(settings-layout)/layout.tsxapps/web/modules/teams/team-members-view.tsxapps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsxpackages/features/users/components/UserTable/UserListTable.tsxpackages/features/pbac/lib/resource-permissions.tspackages/trpc/server/routers/viewer/teams/removeMember.handler.tsapps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsxpackages/features/ee/teams/components/MemberList.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
apps/web/modules/members/members-view.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/layout.tsxapps/web/modules/teams/team-members-view.tsxapps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsxpackages/features/users/components/UserTable/UserListTable.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsxpackages/features/ee/teams/components/MemberList.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx
🧬 Code Graph Analysis (15)
packages/trpc/server/routers/viewer/organizations/bulkDeleteUsers.handler.ts (1)
packages/features/pbac/lib/resource-permissions.ts (1)
getSpecificPermissions(105-142)
packages/trpc/server/routers/viewer/organizations/getUser.handler.ts (1)
packages/features/pbac/lib/resource-permissions.ts (1)
getSpecificPermissions(105-142)
apps/web/modules/members/members-view.tsx (2)
packages/features/users/components/UserTable/UserListTable.tsx (2)
UserListTableProps(110-126)UserListTable(128-134)packages/features/users/components/UserTable/types.ts (1)
MemberPermissions(56-61)
packages/trpc/server/routers/viewer/organizations/listMembers.handler.test.ts (1)
scripts/prepare-local-for-delegation-credentials-testing.js (1)
user(15-17)
packages/trpc/server/routers/viewer/organizations/listMembers.handler.ts (1)
packages/features/pbac/lib/resource-permissions.ts (1)
getSpecificPermissions(105-142)
packages/trpc/server/routers/viewer/teams/listMembers.handler.ts (3)
apps/api/v2/src/modules/organizations/memberships/services/organizations-membership.service.ts (1)
isOrgAdminOrOwner(30-39)packages/features/pbac/lib/resource-permissions.ts (1)
getSpecificPermissions(105-142)packages/platform/libraries/index.ts (1)
MembershipRole(98-98)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/layout.tsx (1)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx (1)
SettingsLayoutAppDirClient(802-856)
apps/web/modules/teams/team-members-view.tsx (1)
packages/features/users/components/UserTable/types.ts (1)
MemberPermissions(56-61)
apps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsx (5)
apps/web/app/_trpc/context.ts (1)
createRouterCaller(18-22)packages/trpc/server/routers/viewer/organizations/_router.tsx (1)
viewerOrganizationsRouter(33-166)packages/features/pbac/lib/resource-permissions.ts (1)
getSpecificPermissions(105-142)packages/platform/libraries/index.ts (1)
MembershipRole(98-98)apps/web/modules/members/members-view.tsx (1)
MembersView(10-30)
packages/features/users/components/UserTable/UserListTable.tsx (2)
packages/features/users/components/UserTable/types.ts (1)
MemberPermissions(56-61)packages/features/users/components/UserTable/BulkActions/MassAssignAttributes.tsx (1)
MassAssignAttributesBulkAction(335-341)
packages/features/pbac/lib/resource-permissions.ts (5)
packages/platform/libraries/index.ts (1)
MembershipRole(98-98)packages/features/flags/features.repository.ts (1)
FeaturesRepository(18-254)packages/platform/libraries/repositories.ts (1)
FeaturesRepository(9-9)packages/features/pbac/services/permission-check.service.ts (1)
PermissionCheckService(20-370)packages/features/pbac/domain/mappers/PermissionMapper.ts (1)
PermissionMapper(7-120)
packages/trpc/server/routers/viewer/teams/removeMember.handler.ts (2)
packages/features/pbac/lib/resource-permissions.ts (1)
getSpecificPermissions(105-142)packages/lib/server/queries/teams/index.ts (1)
isTeamOwner(409-418)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsx (3)
packages/features/auth/lib/getServerSession.ts (1)
getServerSession(32-136)packages/features/pbac/lib/resource-permissions.ts (1)
getSpecificPermissions(105-142)apps/web/modules/teams/team-members-view.tsx (1)
TeamMembersView(30-70)
packages/features/ee/teams/components/MemberList.tsx (1)
packages/features/users/components/UserTable/types.ts (1)
MemberPermissions(56-61)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx (1)
packages/features/shell/Shell.tsx (1)
Shell(108-121)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (27)
packages/features/users/components/UserTable/types.ts (1)
55-61: LGTM! Well-structured permission interface.The
MemberPermissionsinterface provides a clean abstraction for PBAC-driven permission checks across member management UIs. The boolean flags align well with the specific actions needed for team and organization member management.packages/prisma/migrations/20250811123707_add_team_list_members_permission_to_admin_role/migration.sql (1)
1-9: Migration depends on existingadmin_role– please verifyThe migration correctly adds the
team.listMemberspermission to the admin role with conflict-safe inserts, but I didn’t find any prior migration that creates a role with ID'admin_role'. Before deploying, please confirm that a record withid = 'admin_role'exists (for example, in your seed script or an earlier migration).• File:
packages/prisma/migrations/20250811123707_add_team_list_members_permission_to_admin_role/migration.sql (lines 1–9)packages/trpc/server/routers/viewer/organizations/listMembers.handler.test.ts (1)
47-51: UserRepository mock implementation looks good.The mock correctly returns the user object without modification, which is appropriate for unit testing where we want to isolate the handler logic from the enrichment logic.
apps/web/modules/members/members-view.tsx (1)
10-22: Good implementation of PBAC permissions with backward compatibilityThe implementation correctly prioritizes PBAC permissions when available and falls back to role-based checks, ensuring backward compatibility for organizations without PBAC enabled.
packages/trpc/server/routers/viewer/organizations/getUser.handler.ts (1)
39-53: Well-structured PBAC permission checkThe implementation correctly uses
getSpecificPermissionswith appropriate fallback roles, properly mapping actions to membership roles. The separation between ListMembers (MEMBER+) and ChangeMemberRole (ADMIN+) permissions is appropriate.packages/trpc/server/routers/viewer/organizations/bulkDeleteUsers.handler.ts (2)
26-39: Good consistent authorization patternThe membership check and error handling pattern is consistent with other handlers in the PR, which improves maintainability.
41-53: Appropriate PBAC permission configuration for delete operationsThe permission check correctly restricts the Remove action to ADMIN and OWNER roles only, which is appropriate for a destructive operation like bulk deletion.
packages/trpc/server/routers/viewer/organizations/listMembers.handler.ts (2)
75-88: Consistent membership validation patternGood consistency with the membership validation pattern used across other handlers in this PR.
99-102: Verify fallback role logic for private organizations
I didn’t find any tests or documentation defining expected member‐visibility rules for private vs. public organizations. Please confirm that private orgs should indeed only expose ADMIN and OWNER roles when listing members.• File: packages/trpc/server/routers/viewer/organizations/listMembers.handler.ts
• Lines 99–102:roles: ctx.user.organization.isPrivate ? [MembershipRole.ADMIN, MembershipRole.OWNER] : [MembershipRole.MEMBER, MembershipRole.ADMIN, MembershipRole.OWNER],packages/features/users/components/UserTable/UserListTable.tsx (4)
179-185: Good implementation of permission-based table controlsThe tablePermissions object correctly maps PBAC permissions to table-specific actions with appropriate fallbacks to maintain backward compatibility.
630-630: Verify permission checks are applied consistentlyThe permission checks using nullish coalescing (
??) are applied consistently across all bulk actions. This ensures PBAC permissions take precedence when available.Also applies to: 639-644, 647-652
675-691: Correct permission check for the Add buttonThe Add button correctly uses the
canInvitepermission, maintaining consistency with the permission model.
481-481: Good dependency management in useMemoThe addition of
permissionsto the dependency array ensures the columns are properly recalculated when permissions change.packages/features/pbac/lib/resource-permissions.ts (1)
88-142: LGTM! The newgetSpecificPermissionsfunction provides flexible PBAC support.The implementation correctly:
- Reuses the existing FeaturesRepository and PermissionCheckService infrastructure
- Provides proper fallback to role-based access when PBAC is disabled
- Maintains consistency with the existing
getResourcePermissionspattern- Uses the same permission mapping logic via
PermissionMapper.toActionMapapps/web/app/(use-page-wrapper)/settings/(settings-layout)/layout.tsx (1)
70-74: LGTM! Clean migration to permissions object pattern.The change from
canViewRolestopermissions={{ canViewRoles }}properly encapsulates permissions in an object, providing better extensibility for future permission additions.apps/web/modules/teams/team-members-view.tsx (1)
30-37: LGTM! Clean implementation of PBAC permissions with proper fallback.The implementation correctly:
- Uses optional chaining to safely access
permissions?.canListMembers- Falls back to the existing role-based logic when permissions are not available
- Maintains backward compatibility while enabling PBAC
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsx (2)
62-66: LGTM! Proper session validation and redirect.The session check correctly validates user authentication before proceeding with permission checks.
113-118: LGTM! Clean mapping of PBAC permissions to UI requirements.The permissions are properly mapped to a consistent
memberPermissionsobject that aligns with theMemberPermissionsinterface used throughout the UI components.apps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsx (2)
87-93: LGTM: Clear mapping from PBAC results to UI permissions.The projection into
MemberPermissionsis straightforward and consistent with the UI contract.
107-113: LGTM: Wiring permissions into MembersView.Passing the computed
memberPermissionsenables consistent UI gating downstream.packages/features/ee/teams/components/MemberList.tsx (2)
32-32: LGTM: MemberPermissions imported at source.Type import keeps bundle clean on the client.
715-724: LGTM: Add-member CTA gated by canInvite.This aligns UI affordances with PBAC.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx (5)
184-187: LGTM: Centralized SettingsPermissions type.A single surface for layout permissions reduces prop proliferation.
191-196: Hook signature accepts permissions — good.Threading
permissionsintouseTabsenables PBAC-aware sidebar composition.
521-525: LGTM: Passing permissions into useTabs.Keeps tab gating concentrated in one place.
840-841: LGTM: Propagating permissions into SidebarContainer.Consistent with the new API surface.
888-889: LGTM: Final propagation to SettingsSidebarContainer.Completes the flow.
| // Use PBAC permissions for edit mode, with role-based fallback | ||
| const canChangeRole = props.permissions?.canChangeMemberRole ?? false; | ||
| const canRemove = props.permissions?.canRemove ?? false; | ||
| const editMode = (canChangeRole || canRemove) && !isSelf; | ||
| // For now impersonation just lives on the canChangeRole OR canRemove permission (since without pbac these check admin or owner) | ||
| // In my next PR on this stack i will implement a custom impersonation permission that lives on orgs + teams. | ||
| const impersonationMode = |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Gate edit actions per permission; avoid showing “Remove” and “Resend invitation” when not allowed.
Currently, editMode becomes true if either canChangeRole or canRemove is true, and the menu always shows “remove” and “resend invitation” options. Users who cannot remove still see “remove,” and users who cannot invite can see “resend invitation”. Gate each action individually.
Refactor local flags:
- // Use PBAC permissions for edit mode, with role-based fallback
- const canChangeRole = props.permissions?.canChangeMemberRole ?? false;
- const canRemove = props.permissions?.canRemove ?? false;
- const editMode = (canChangeRole || canRemove) && !isSelf;
+ // Use PBAC permissions for granular gating
+ const canChangeRole = props.permissions?.canChangeMemberRole ?? false;
+ const canRemove = props.permissions?.canRemove ?? false;
+ const canInvite = props.permissions?.canInvite ?? false;
+ const editMode = (canChangeRole || canRemove || canInvite) && !isSelf;Apply outside this hunk (non-diff example) to gate items:
// Desktop menu
{impersonationMode && ...}
{resendInvitation && canInvite && (
/* resend invitation item */
)}
{canRemove && (
<DropdownMenuItem>
<DropdownItem
type="button"
onClick={() => dispatch({ type: "SET_DELETE_ID", payload: { user, showModal: true } })}
color="destructive"
StartIcon="user-x">
{t("remove")}
</DropdownItem>
</DropdownMenuItem>
)}And similarly for the mobile menu’s “remove” item.
Also, consider gating the bulk remove bar (DeleteBulkTeamMembers) behind props.permissions.canRemove to keep UX consistent.
🤖 Prompt for AI Agents
In packages/features/ee/teams/components/MemberList.tsx around lines 430-436,
editMode is currently derived from canChangeRole || canRemove and the menu
always renders “Remove” and “Resend invitation”; update this to gate each action
individually: read explicit flags from props.permissions (e.g., const
canChangeRole = props.permissions?.canChangeMemberRole ?? false; const canRemove
= props.permissions?.canRemove ?? false; const canInvite =
props.permissions?.canInvite ?? false), keep editMode only for whether any edit
UI should be shown, but replace unconditional menu items so each action is
wrapped with its own conditional (resend invitation only when canInvite, remove
only when canRemove, impersonation UI only when its specific flag), apply the
same conditionals in the mobile menu, and gate the bulk remove bar
(DeleteBulkTeamMembers) behind props.permissions.canRemove so users who lack
remove permission never see remove/resend options.
| }); | ||
|
|
||
| // User needs either ListMembers (to view) or ChangeMemberRole (to edit) permission | ||
| if (!permissions[CustomAction.ListMembers] || !permissions[CustomAction.ChangeMemberRole]) { |
There was a problem hiding this comment.
Fix incorrect comment - permission logic uses AND not OR
The comment states "either...or" but the condition uses AND (&&), requiring both permissions.
- // User needs either ListMembers (to view) or ChangeMemberRole (to edit) permission
+ // User needs both ListMembers (to view) and ChangeMemberRole (to edit) permissions📝 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.
| if (!permissions[CustomAction.ListMembers] || !permissions[CustomAction.ChangeMemberRole]) { | |
| // User needs both ListMembers (to view) and ChangeMemberRole (to edit) permissions | |
| if (!permissions[CustomAction.ListMembers] || !permissions[CustomAction.ChangeMemberRole]) { |
🤖 Prompt for AI Agents
In packages/trpc/server/routers/viewer/organizations/getUser.handler.ts around
line 56, the inline comment describing the permission check is wrong: the code
uses a logical AND requiring both CustomAction.ListMembers and
CustomAction.ChangeMemberRole, but the comment says "either...or"; update the
comment to state that both permissions are required (or if the intended behavior
is to allow either permission, change the condition to use || instead of &&) so
the code and comment match.
packages/trpc/server/routers/viewer/teams/listMembers.handler.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/web/playwright/fixtures/users.ts (1)
982-994: Use correct NextAuth payload keys and form-encoded body (carryover from earlier review)Use callbackUrl (not callbackURL), booleans for redirect/json, and send x-www-form-urlencoded via form to align with NextAuth’s credentials callback.
- // Make the login request - const loginData = { - email: user.email ?? `${user.username}@example.com`, - password: user.password ?? user.username, - callbackURL: WEBAPP_URL, - redirect: "false", - json: "true", - csrfToken, - }; - - const response = await page.context().request.post("/api/auth/callback/credentials", { - data: loginData, - }); + // Make the login request (NextAuth expects form-encoded body and specific keys) + const form = { + email: user.email ?? `${user.username}@example.com`, + password: user.password ?? user.username, + callbackUrl: "/", + redirect: false, + json: true, + csrfToken, + }; + + const response = await page.context().request.post("/api/auth/callback/credentials", { + form, + });apps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsx (1)
59-64: Revisit fallback visibility for member listing (matches prior thread).To keep least-privilege when PBAC is off, consider restricting ListMembers to ADMIN/OWNER regardless of org privacy. This mirrors the team page and avoids exposing member data to regular members.
Apply if desired:
- const fallbackRolesThatCanSeeMembers: MembershipRole[] = [MembershipRole.ADMIN, MembershipRole.OWNER]; - - if (!org?.isPrivate) { - fallbackRolesThatCanSeeMembers.push(MembershipRole.MEMBER); - } + const fallbackRolesThatCanSeeMembers: MembershipRole[] = [MembershipRole.ADMIN, MembershipRole.OWNER];
🧹 Nitpick comments (3)
apps/web/playwright/fixtures/users.ts (1)
1007-1008: Harden session wait to reduce flakesMatch the exact session request and ensure it succeeded before proceeding.
- // Wait for the session API call to complete to ensure session is fully established - await page.waitForResponse("/api/auth/session"); + // Wait for the successful session response to ensure session is fully established + await page.waitForResponse((res) => res.url().endsWith("/api/auth/session") && res.ok());apps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsx (2)
78-95: DRY up repeated ADMIN/OWNER role arrays in fallbacks.Reduces duplication and drift.
Apply:
const permissions = await getSpecificPermissions({ @@ - fallbackRoles: { + fallbackRoles: { + // Avoid repetition: ADMIN/OWNER used across multiple actions + // eslint-disable-next-line @typescript-eslint/consistent-type-assertions + // (Keep it inline or lift to a top-level const if preferred) + const ADMIN_OWNER_ROLES: MembershipRole[] = [MembershipRole.ADMIN, MembershipRole.OWNER]; [CustomAction.ListMembers]: { roles: fallbackRolesThatCanSeeMembers, }, [CustomAction.Invite]: { - roles: [MembershipRole.ADMIN, MembershipRole.OWNER], + roles: ADMIN_OWNER_ROLES, }, [CustomAction.ChangeMemberRole]: { - roles: [MembershipRole.ADMIN, MembershipRole.OWNER], + roles: ADMIN_OWNER_ROLES, }, [CustomAction.Remove]: { - roles: [MembershipRole.ADMIN, MembershipRole.OWNER], + roles: ADMIN_OWNER_ROLES, }, [CustomAction.Impersonate]: { - roles: [MembershipRole.ADMIN, MembershipRole.OWNER], + roles: ADMIN_OWNER_ROLES, }, }, });If placing a const inside the object literal isn’t allowed by your lint rules, hoist this line just above the getSpecificPermissions call:
const ADMIN_OWNER_ROLES: MembershipRole[] = [MembershipRole.ADMIN, MembershipRole.OWNER];
119-125: Prop wiring OK; consider avoiding redundant “teams” passing.facetedTeamValues already includes teams; if MembersView doesn’t need raw teams separately, drop the extra prop to reduce prop surface.
Confirm by checking MembersView’s props usage in apps/web/modules/members/members-view.tsx and UserListTable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsx(3 hunks)apps/web/playwright/fixtures/users.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
apps/web/playwright/fixtures/users.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/playwright/fixtures/users.tsapps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
apps/web/playwright/fixtures/users.tsapps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
apps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsx
🧠 Learnings (1)
📚 Learning: 2025-08-08T09:12:08.280Z
Learnt from: hariombalhara
PR: calcom/cal.com#22968
File: packages/features/auth/lib/next-auth-options.ts:327-327
Timestamp: 2025-08-08T09:12:08.280Z
Learning: In packages/features/auth/lib/next-auth-options.ts, do not log credentials in authorize() handlers (e.g., the "saml-idp" CredentialsProvider). Remove accidental console.log statements and avoid including credential contents in logs; prefer either no logging or structured logs without sensitive data.
Applied to files:
apps/web/playwright/fixtures/users.ts
🧬 Code graph analysis (1)
apps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsx (4)
packages/features/auth/lib/getServerSession.ts (1)
getServerSession(32-136)apps/web/lib/buildLegacyCtx.ts (1)
buildLegacyRequest(47-49)packages/features/pbac/lib/resource-permissions.ts (1)
getSpecificPermissions(105-142)apps/web/modules/members/members-view.tsx (1)
MembersView(10-30)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Tests / Unit
- GitHub Check: Production builds / Build Docs
- GitHub Check: Production builds / Build Web App
- GitHub Check: Production builds / Build Atoms
- GitHub Check: Linters / lint
🔇 Additional comments (7)
apps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsx (7)
4-5: LGTM on App Router primitives.Correct use of headers/cookies with server component.
7-9: PBAC imports look correct.Types and helpers wired appropriately.
12-13: Prisma imports OK.Enums and client usage align with downstream code.
16-17: Legacy request bridge OK.buildLegacyRequest is the right adapter for getServerSession.
49-53: Auth guard + redirect is sound.Early exit prevents rendering without an org-scoped session.
97-105: Permission mapping looks correct.Keys align with MemberPermissions and Action enums.
71-77: Server-side PBAC enforcement present
ImpersonationProvider invokes getSpecificPermissions with CustomAction.Impersonate to enforce permissions on the server (packages/features/ee/impersonation/lib/ImpersonationProvider.ts). No further changes needed.
apps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsx
Show resolved
Hide resolved
| if (!org?.isPrivate) { | ||
| fallbackRolesThatCanSeeMembers.push(MembershipRole.MEMBER); | ||
| } |
There was a problem hiding this comment.
If the org is not private - members can see it too
(Fall back roles if PBAC is disabled)
This logic is also replicated in the handler -> We just return []
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
packages/features/users/components/UserTable/UserListTable.tsx (5)
651-656: Delete bulk users should require list permission too.
Aligns with PBAC consistency.- {(permissions?.canRemove ?? adminOrOwner) && ( + {(permissions?.canListMembers ?? adminOrOwner) && + (permissions?.canRemove ?? adminOrOwner) && ( <DeleteBulkUsers users={table.getSelectedRowModel().flatRows.map((row) => row.original)} onRemove={() => table.toggleAllPageRowsSelected(false)} /> )}
679-695: “Add” button should be gated by list permission + invite.
Prevents invites when the user can’t view the list.- {(permissions?.canInvite ?? adminOrOwner) && ( + {(permissions?.canListMembers ?? adminOrOwner) && + (permissions?.canInvite ?? adminOrOwner) && ( <DataTableToolbar.CTA type="button" color="primary" StartIcon="plus" onClick={() => dispatch({ type: "INVITE_MEMBER", payload: { showModal: true, }, }) } data-testid="new-organization-member-button"> {t("add")} </DataTableToolbar.CTA> )}
179-185: Gate all actions by canListMembers to avoid inconsistent UI states.
Without gating, users lacking list permission can still see edit/remove/impersonate affordances if role fallback is true. Normalize via canList first.Apply:
- // Use PBAC permissions if available, otherwise fall back to role-based check - const tablePermissions = { - canEdit: permissions?.canChangeMemberRole ?? adminOrOwner, - canRemove: permissions?.canRemove ?? adminOrOwner, - canResendInvitation: permissions?.canInvite ?? adminOrOwner, - canImpersonate: permissions?.canImpersonate ?? adminOrOwner, - }; + // Normalize: gate by canListMembers, then apply PBAC with role fallback + const canList = permissions?.canListMembers ?? adminOrOwner; + const tablePermissions = { + canEdit: canList && (permissions?.canChangeMemberRole ?? adminOrOwner), + canRemove: canList && (permissions?.canRemove ?? adminOrOwner), + canResendInvitation: canList && (permissions?.canInvite ?? adminOrOwner), + canImpersonate: canList && (permissions?.canImpersonate ?? adminOrOwner), + };If upstream already folds role fallbacks into permissions, we can drop the redundant
?? adminOrOwner. Verify with:#!/bin/bash # Find MemberPermissions producers and any fallbacks rg -nC3 'MemberPermissions|canListMembers|canChangeMemberRole|canInvite|canRemove|canImpersonate|adminOrOwner' --type=ts --type=tsx
634-634: Bulk action needs list gating.
Users without list permission shouldn’t see team assignment.- {permissions?.canChangeMemberRole && <TeamListBulkAction table={table} />} + {(permissions?.canListMembers ?? adminOrOwner) && + (permissions?.canChangeMemberRole ?? adminOrOwner) && ( + <TeamListBulkAction table={table} /> + )}
643-648: Mass-assign and Event Types should also require list permission.
Prevent action exposure when listing is denied.- {(permissions?.canChangeMemberRole ?? adminOrOwner) && ( + {(permissions?.canListMembers ?? adminOrOwner) && + (permissions?.canChangeMemberRole ?? adminOrOwner) && ( <MassAssignAttributesBulkAction table={table} filters={columnFilters} /> )} - {(permissions?.canChangeMemberRole ?? adminOrOwner) && ( + {(permissions?.canListMembers ?? adminOrOwner) && + (permissions?.canChangeMemberRole ?? adminOrOwner) && ( <EventTypesList table={table} orgTeams={teams} /> )}
🧹 Nitpick comments (3)
packages/features/users/components/UserTable/UserListTable.tsx (3)
485-485: Stabilize memo deps (add t, drop dispatch).
t is used within the memo; dispatch from useReducer is stable and need not be a dep.- }, [session?.user.id, adminOrOwner, dispatch, domain, attributes, org?.canAdminImpersonate, permissions]); + }, [session?.user.id, adminOrOwner, domain, attributes, org?.canAdminImpersonate, permissions, t]);
354-358: Fix stray data-testid2 attribute.
Looks like a typo; keep a single data-testid to avoid selector ambiguity.- <Badge - data-testid2={`member-${username}-pending`} + <Badge variant="red" className="text-xs" data-testid={`email-${email.replace("@", "")}-pending`}
267-279: Localize aria-labels to meet i18n guidelines.
Direct strings violate the repo rule to always use t() in TSX.- aria-label="Select all" + aria-label={t("select_all")} ... - aria-label="Select row" + aria-label={t("select_row")}Also consider localizing “No username” (Line 304) and error toasts in handleDownload (Lines 550, 555, 582, 588, 594).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/features/users/components/UserTable/UserListTable.tsx(9 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/features/users/components/UserTable/UserListTable.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/users/components/UserTable/UserListTable.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/users/components/UserTable/UserListTable.tsx
🧬 Code graph analysis (1)
packages/features/users/components/UserTable/UserListTable.tsx (1)
packages/features/users/components/UserTable/types.ts (1)
MemberPermissions(56-62)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (4)
packages/features/users/components/UserTable/UserListTable.tsx (4)
53-53: Type import looks good.
Importing MemberPermissions locally keeps coupling minimal.
125-126: Prop addition is fine and backwards compatible.
Optional permissions with sane fallbacks preserves pre-PBAC behavior.
136-142: Props destructuring OK.
No functional concerns.
456-468: Row-level permissions derivation looks correct.
Self-guards and org.canAdminImpersonate are correctly applied. This will inherit list gating from tablePermissions after the fix above.
sean-brydon
requested review from
CarinaWolli and
eunjae-lee
and removed request for
CarinaWolli
| const memberPermissions = { | ||
| canListMembers: permissions[CustomAction.ListMembers], | ||
| canInvite: permissions[CustomAction.Invite], | ||
| canChangeMemberRole: permissions[CustomAction.ChangeMemberRole], |
There was a problem hiding this comment.
Change Member Role: Change role of organization members
ChangeMemberRole also includes updating other member details like username or the "about" section, is this intended? If yes we should rename the setting
| dependsOn: ["team.read"], | ||
| dependsOn: ["team.read", "team.listMembers"], | ||
| }, | ||
| [CustomAction.Impersonate]: { |
There was a problem hiding this comment.
I can have impersonation turned on but nothing else. Do we want that?
There was a problem hiding this comment.
Can't do that for the team settings it automatically turns on "List Members" which seems like the correct behaviour
There was a problem hiding this comment.
Nice spot Will fix in a follow up if this gets merged before tomorrow (unlikely)
Visual Demo (For contributors especially)
Organization Members page demo: https://cap.so/s/qv2v71db6bn20x5
Teams member page demo: https://cap.so/s/ztcg5wxj3a8eys8
How should this be tested?
Seed your database with yarn seed-pbac
Create a role in organization with no permissions.
Assign a user this role.
Visit organization members page
Assign permissions
Work your way through assigning all members permissions - invite,remove,canListMembers,changeRole etc
Notice they all reflect in the UI and on the backend
Do the same for teams roles
Logout of pbac - login as teampro - test everything works as expected on a team without PBAC
Login to a plain org (or remove feature flag) and test everything works as expected for each role
Checklist