fix(organizations): allow org owners and admins to edit their own use…#24380
fix(organizations): allow org owners and admins to edit their own use…#24380antcybersec wants to merge 14 commits intocalcom:mainfrom
Conversation
|
@antcybersec is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
bot
added
organizations
dosubot
bot
added
teams
WalkthroughAdds an acceptOrLeave mutation and a leave-team confirmation dialog in the team member UI; expands per-row permission checks (isSelf now compares id/email/username, adds viewerIsAdminOrOwner, rowIsOwnerSelf, canChangeRoleEffective, and adjusts editMode) and gates dropdown actions (leave, resend invite, remove) using an ownersCount derived from current data. Server-side leaveTeamMembership now runs in a transaction, fetches the departing membership role, counts OWNER memberships, and prevents the last OWNER from leaving. Public page links use /{username} when present. Minor local variable renames (_-prefixed). Adds .babelrc and makes SalesforceGraphQLClient optionally require retryExchange. Possibly related PRs
Pre-merge checks and finishing touches❌ 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 |
can you please check now ...i have attached the video .. |
dosubot
bot
added
the
🧹 Improvements
![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
🧹 Nitpick comments (3)
packages/features/ee/teams/components/MemberList.tsx (3)
448-457: Avoid unintentionally enabling role edits via self-edit gating.
canChangeRoleEffective = canChangeRole || (isSelf && viewerIsAdminOrOwner)may surface the Edit sheet for self even whencanChangeRoleis false. Ensure EditMemberSheet disables role changes unlesscanChangeRoleis true (open sheet for username only).Would you pass an explicit prop (e.g.,
allowRoleEdit={canChangeRole}) to EditMemberSheet to enforce this?
587-605: Consider hiding “Remove” for any self row and prefer “Leave team” label.Currently removal is hidden only for owner-self. For admin/member self-rows, surfacing “Remove” is confusing; prefer a unified “Leave team” action for any self-row.
738-744: Underscored fetchMore hook — OK.If the variable isn’t used, the underscore prefix is fine to satisfy lint rules. Alternatively, call the hook without assignment if your lint config allows it.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/features/ee/teams/components/MemberList.tsx(11 hunks)packages/features/users/components/UserTable/UserListTable.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/features/users/components/UserTable/UserListTable.tsx
🧰 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/ee/teams/components/MemberList.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/ee/teams/components/MemberList.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/ee/teams/components/MemberList.tsx
🧬 Code graph analysis (1)
packages/features/ee/teams/components/MemberList.tsx (3)
packages/trpc/react/trpc.ts (1)
trpc(54-138)packages/ui/components/button/Button.tsx (1)
Button(221-349)packages/ui/components/dropdown/Dropdown.tsx (2)
DropdownMenuItem(63-71)DropdownItem(161-181)
🔇 Additional comments (4)
packages/features/ee/teams/components/MemberList.tsx (4)
438-443: Robust self-check — LGTM.Expanding isSelf to id/email/username reduces false negatives for self rows.
516-533: Edit menu visibility based oncanChangeRoleEffective— OK, but ensure field-level enforcement.Showing the sheet is fine; just ensure the sheet itself respects
canChangeRoleand does not expose role changes when disallowed.
571-586: Owner-self exclusions for “Resend invitation” — LGTM.Matches intent; owner shouldn’t resend invite to self.
720-729: PBAC role facet case wrapped in braces — LGTM.No behavior change; improves readability for future additions.
| const acceptOrLeaveMutation = trpc.viewer.teams.acceptOrLeave.useMutation({ | ||
| onSuccess: () => { | ||
| showToast(t("success"), "success"); | ||
| }, | ||
| onError: (error) => { | ||
| showToast(error.message, "error"); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Mutation lacks cache invalidation and UX safeguards (stale UI, no redirect, double-click risk).
- Add invalidations after accept/leave to refresh lists and team state.
- Consider redirecting after leaving (user will likely lose access to this page).
- Disable the menu item while the mutation is pending.
Suggested diff:
- const acceptOrLeaveMutation = trpc.viewer.teams.acceptOrLeave.useMutation({
- onSuccess: () => {
- showToast(t("success"), "success");
- },
- onError: (error) => {
- showToast(error.message, "error");
- },
- });
+ const acceptOrLeaveMutation = trpc.viewer.teams.acceptOrLeave.useMutation({
+ onSuccess: async () => {
+ await Promise.all([
+ utils.viewer.teams.get.invalidate(),
+ utils.viewer.teams.listMembers.invalidate(),
+ utils.viewer.organizations.listMembers.invalidate(),
+ utils.viewer.organizations.getMembers.invalidate(),
+ ]);
+ showToast(t("success"), "success");
+ // Optional: navigate away if the viewer left this team/org.
+ // router.push("/teams"); // or a safe landing page
+ },
+ onError: (error) => {
+ showToast(error.message, "error");
+ },
+ });Please also confirm whether the server prevents the “last owner” from leaving; if not, we should guard this client-side too.
---
🏁 Script executed:
```shell
#!/bin/bash
# Inspect MemberList.tsx for imports, utils setup, and how acceptOrLeaveMutation is used
head -100 packages/features/ee/teams/components/MemberList.tsx
Length of output: 3078
🏁 Script executed:
#!/bin/bash
# Find usage of acceptOrLeaveMutation in the file
rg -n "acceptOrLeaveMutation" packages/features/ee/teams/components/MemberList.tsx -A5 -B2Length of output: 1164
🏁 Script executed:
#!/bin/bash
# Check if utils is imported and how cache invalidation is done in similar mutations nearby
rg -n "utils\." packages/features/ee/teams/components/MemberList.tsx -C2 | head -50Length of output: 794
🏁 Script executed:
#!/bin/bash
# Search for patterns of isPending usage in this component
rg -n "isPending" packages/features/ee/teams/components/MemberList.tsxLength of output: 107
🏁 Script executed:
#!/bin/bash
# Look for menu/button usage that might need the disable pattern
rg -n "menu|Menu|Button|button" packages/features/ee/teams/components/MemberList.tsx -i | head -20Length of output: 942
🏁 Script executed:
#!/bin/bash
# Check the server-side implementation of acceptOrLeave for last-owner validation
fd -t f "acceptOrLeave" packages/trpc/server --type=tsLength of output: 169
🏁 Script executed:
#!/bin/bash
# If found, examine the server handler
fd -t f "teams" packages/trpc/server --type=ts | xargs rg -l "acceptOrLeave"Length of output: 558
🏁 Script executed:
#!/bin/bash
# Look for how other team mutations handle cache invalidation
rg -n "viewer\.teams" packages/features/ee/teams/components/MemberList.tsx | grep -E "invalidate|useMutation" -A3Length of output: 440
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/features/ee/teams/components/MemberList.tsx (2)
569-584: Gate “Resend invitation” to pending invites onlyCurrently shows for accepted users too. Add the accepted check.
- {!rowIsOwnerSelf && canResendInvitation && ( + {!rowIsOwnerSelf && canResendInvitation && !user.accepted && (
689-690: Fix stale closures in columns: expand deps and memoize ownersCount before columns
- Columns use session email/username in isSelf but deps only include id → stale UI after username/email change.
- ownersCount is read inside column cells but declared after useMemo → stale reference across renders.
Apply dep expansion:
- }, [props.isOrgAdminOrOwner, dispatch, totalRowCount, session?.user.id]); + }, [ + props.isOrgAdminOrOwner, + dispatch, + totalRowCount, + session?.user?.id, + session?.user?.email, + (session?.user as { username?: string } | undefined)?.username, + // add ownersCount after moving it above this useMemo (see below) + ownersCount, + ]);Then move ownersCount above the columns useMemo and memoize it:
// Place this above memorisedColumns const ownersCount = useMemo( () => flatData.filter((m) => m.role === "OWNER" && m.accepted).length, [flatData] );This ensures columns re-compute when session identity or ownersCount changes.
Also applies to: 693-693
♻️ Duplicate comments (1)
packages/features/ee/teams/components/MemberList.tsx (1)
290-297: Add cache invalidation (and optional redirect) after accept/leaveUI will go stale without invalidations; optionally redirect after leave.
Apply:
- const acceptOrLeaveMutation = trpc.viewer.teams.acceptOrLeave.useMutation({ - onSuccess: () => { - showToast(t("success"), "success"); - }, + const acceptOrLeaveMutation = trpc.viewer.teams.acceptOrLeave.useMutation({ + onSuccess: async (_data, variables) => { + await Promise.all([ + utils.viewer.teams.get.invalidate(), + utils.viewer.teams.listMembers.invalidate(), + utils.viewer.organizations.listMembers.invalidate(), + utils.viewer.organizations.getMembers.invalidate(), + ]); + showToast(t("success"), "success"); + // Optional: redirect after a successful leave + // if (variables?.accept === false) router.push("/teams"); + },If you add redirect, also import/use useRouter from next/navigation and instantiate router.
Please confirm the correct landing path post-leave (team vs org).
🧹 Nitpick comments (1)
packages/features/ee/teams/components/MemberList.tsx (1)
495-505: Public-page links unified/guarded — minor nit on targetLooks good. Consider making target conditional to avoid invalid target on non-links.
- target="_blank" + target={user.username ? "_blank" : undefined}And similarly on mobile:
- target="_blank" + target={user.username ? "_blank" : undefined}Also applies to: 618-625
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- 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/ee/teams/components/MemberList.tsx(14 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/ee/teams/components/MemberList.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/ee/teams/components/MemberList.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/ee/teams/components/MemberList.tsx
🧬 Code graph analysis (1)
packages/features/ee/teams/components/MemberList.tsx (4)
packages/trpc/react/trpc.ts (1)
trpc(54-138)packages/ui/components/button/Button.tsx (1)
Button(221-349)packages/ui/components/dropdown/Dropdown.tsx (2)
DropdownMenuItem(63-71)DropdownItem(161-181)packages/ui/components/dialog/Dialog.tsx (2)
Dialog(35-38)DialogFooter(150-164)
⏰ 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/ee/teams/components/MemberList.tsx (4)
212-212: Leave confirmation state — OKState wiring looks good.
439-444: Self detection and edit gating — aligns with objectiveExpanded isSelf and editMode correctly enable self-edit for admins/owners.
Also applies to: 450-455, 456-459
585-603: Mobile “Remove” permission parity — OKNow matches desktop guards (no self-remove for owner, requires canRemove).
Also applies to: 656-674
736-741: Rename to _fetchMoreOnBottomReached — OKNo behavior change; avoids name clash.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/features/ee/teams/components/MemberList.tsx(14 hunks)packages/lib/server/service/teamService.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/features/ee/teams/components/MemberList.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*Service.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Service files must include
Servicesuffix, use PascalCase matching exported class, and avoid generic names (e.g.,MembershipService.ts)
Files:
packages/lib/server/service/teamService.ts
**/*.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/lib/server/service/teamService.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/lib/server/service/teamService.ts
**/*.{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/lib/server/service/teamService.ts
⏰ 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). (3)
- GitHub Check: Type check / check-types
- GitHub Check: Tests / Unit
- GitHub Check: Linters / lint
dhairyashiil
left a comment
dhairyashiil
left a comment
There was a problem hiding this comment.
Could you please check the following?
- Can you change the role of the owner to admin or member?
- Also, could you click on “Leave Team”? I mean, is the owner able to leave the team? Please check this as well. ( I think you have added new check for this in your code, right?)
Record both scenarios and please attach a video here.
Additionally, the tests are failing, and there’s a merge conflict. Please fix both.
hey i have attached the new video with the changes u have said ..also fix the merge conflicts issuess....please check it once |
…rnames - Fix UserListTable to show edit option for org owners/admins on themselves - Fix MemberList to show edit option for org owners/admins on themselves - Resolves issue where org owners/admins couldn't see triple dot menu to edit their own usernames - Maintains existing security by only allowing edit for users with proper permissions Fixes calcom#24377
…for self; show public link for pending members
…uards - Add confirmation dialog for Leave team action - Prevent last owner from leaving team - Add proper guards for mobile Remove action - Fix public page links with username validation - Disable buttons during pending operations
- Use ConfirmationDialogContent for leave dialog consistency - Add server-side last owner guard in leaveTeamMembership - Prevent last owner from leaving team at database level
… issues - Use transaction to prevent race condition in leaveTeamMembership - Use MembershipRole.OWNER enum instead of string literals - Use TRPCError for consistent error handling - Use logger instead of console.log
- Remove ownersCount > 1 restriction for leave team button - Remove server-side last owner restriction for leave team - Allow owners to change their own role (even if last owner) - Update both legacy and PBAC role managers for self-role changes - Remove unused ownersCount variable
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/app-store/salesforce/lib/graphql/SalesforceGraphQLClient.ts (1)
28-42: Consider logging when the retry exchange is unavailable.While the graceful fallback is appropriate, the empty catch block makes it difficult to diagnose whether the module is missing when expected. Consider logging at debug or info level to aid troubleshooting.
Apply this diff to add observability:
} catch { - // Fallback: proceed without retry exchange if the module is not installed + // Fallback: proceed without retry exchange if the module is not installed + this.log.debug("@urql/exchange-retry not installed; proceeding without retry logic"); }Note: Since
this.logis assigned after the Client is created (line 55), you would need to use the baseloggerinstead:} catch { - // Fallback: proceed without retry exchange if the module is not installed + // Fallback: proceed without retry exchange if the module is not installed + logger.debug("@urql/exchange-retry not installed; proceeding without retry logic"); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.babelrc(1 hunks)packages/app-store/salesforce/lib/graphql/SalesforceGraphQLClient.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .babelrc
🧰 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/app-store/salesforce/lib/graphql/SalesforceGraphQLClient.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/app-store/salesforce/lib/graphql/SalesforceGraphQLClient.ts
**/*.{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/app-store/salesforce/lib/graphql/SalesforceGraphQLClient.ts
| maxRetries: 3, | ||
| initialDelayMs: Number(process.env.SALESFORCE_GRAPHQL_DELAY_MS), | ||
| maxDelayMs: Number(process.env.SALESFORCE_GRAPHQL_MAX_DELAY_MS), | ||
| randomDelay: true, | ||
| maxNumberAttempts: Number(process.env.SALESFORCE_GRAPHQL_MAX_RETRIES), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inconsistent retry configuration: hardcoded vs. environment variable.
Line 33 hardcodes maxRetries: 3, while line 37 reads maxNumberAttempts from an environment variable. This inconsistency makes the configuration less predictable. Consider either reading both from environment variables or clarifying why one is hardcoded.
Apply this diff to align the configuration:
const retryOptions = {
- maxRetries: 3,
+ maxRetries: Number(process.env.SALESFORCE_GRAPHQL_MAX_RETRIES) || 3,
initialDelayMs: Number(process.env.SALESFORCE_GRAPHQL_DELAY_MS),
maxDelayMs: Number(process.env.SALESFORCE_GRAPHQL_MAX_DELAY_MS),
randomDelay: true,
maxNumberAttempts: Number(process.env.SALESFORCE_GRAPHQL_MAX_RETRIES),
};Alternatively, if maxRetries and maxNumberAttempts serve different purposes in the retryExchange API, document the distinction to avoid confusion.
📝 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.
| maxRetries: 3, | |
| initialDelayMs: Number(process.env.SALESFORCE_GRAPHQL_DELAY_MS), | |
| maxDelayMs: Number(process.env.SALESFORCE_GRAPHQL_MAX_DELAY_MS), | |
| randomDelay: true, | |
| maxNumberAttempts: Number(process.env.SALESFORCE_GRAPHQL_MAX_RETRIES), | |
| maxRetries: Number(process.env.SALESFORCE_GRAPHQL_MAX_RETRIES) || 3, | |
| initialDelayMs: Number(process.env.SALESFORCE_GRAPHQL_DELAY_MS), | |
| maxDelayMs: Number(process.env.SALESFORCE_GRAPHQL_MAX_DELAY_MS), | |
| randomDelay: true, | |
| maxNumberAttempts: Number(process.env.SALESFORCE_GRAPHQL_MAX_RETRIES), |
🤖 Prompt for AI Agents
In packages/app-store/salesforce/lib/graphql/SalesforceGraphQLClient.ts around
lines 33 to 37, the retry configuration is inconsistent: maxRetries is hardcoded
to 3 while maxNumberAttempts is read from an env var; update the code so both
values are driven consistently (either both read from environment variables or
both set to constants) — e.g., replace maxRetries: 3 with maxRetries:
Number(process.env.SALESFORCE_GRAPHQL_MAX_RETRIES) (or vice versa), ensure you
parse Numbers safely with a fallback default if the env var is missing, and
remove or rename duplicate/ambiguous fields so the retryExchange API receives a
single clear setting (or add a short inline comment documenting if they have
distinct meanings).
There was a problem hiding this comment.
@antcybersec, I’m not sure why you made changes to this file. Salesforce app have nothing to do with the current scope of PR, right?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@antcybersec, I’m not sure why you made changes to this file. Salesforce app have nothing to do with the current scope of PR, right?
sorry i will remove that ...is there anything more i need to check ??
There was a problem hiding this comment.
@dhairyashiil hey please check once i have removed the unnnecesary files ...
antcybersec
force-pushed
the
fix/org-owners-edit-own-username
branch
from
48ea187 to
3d0f335
Compare
- Mock prisma.$transaction to handle transaction-based calls - Update test setup to mock tx.membership methods instead of direct prismaMock - Ensure tests properly verify transaction behavior - Add eslint-disable comments for necessary any types
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 8 files
Prompt for AI agents (all 4 issues)
Understand the root cause of the following 4 issues and fix them.
<file name="packages/lib/server/service/teamService.ts">
<violation number="1" location="packages/lib/server/service/teamService.ts:463">
Rule violated: **Avoid Logging Sensitive Information**
Remove the username from this debug log to avoid writing PII to the logs.</violation>
</file>
<file name="packages/features/ee/teams/services/teamService.ts">
<violation number="1" location="packages/features/ee/teams/services/teamService.ts:292">
This last-owner guard should only fire for accepted owners; pending owner invitations are currently blocked from leaving even when another accepted owner remains. Please include the member’s accepted status in the select and require it to be true before throwing.</violation>
</file>
<file name="packages/features/pbac/services/legacy-role-manager.service.ts">
<violation number="1" location="packages/features/pbac/services/legacy-role-manager.service.ts:48">
This change lets the only owner demote themselves, leaving the team without any owner. Keep blocking self-demotion unless the new role stays OWNER.</violation>
</file>
<file name="packages/features/pbac/services/pbac-role-manager.service.ts">
<violation number="1" location="packages/features/pbac/services/pbac-role-manager.service.ts:90">
This change lets the last remaining owner demote themself, leaving the organization without any owners. Please keep the last-owner protection in place even for self-initiated role changes.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| }); | ||
|
|
||
| if (user.username && user.movedToProfileId === profileToDelete?.id) { | ||
| log.debug("Cleaning up tempOrgRedirect for user", user.username); |
There was a problem hiding this comment.
Rule violated: Avoid Logging Sensitive Information
Remove the username from this debug log to avoid writing PII to the logs.
Prompt for AI agents
Address the following comment on packages/lib/server/service/teamService.ts at line 463:
<comment>Remove the username from this debug log to avoid writing PII to the logs.</comment>
<file context>
@@ -0,0 +1,564 @@
+ });
+
+ if (user.username && user.movedToProfileId === profileToDelete?.id) {
+ log.debug("Cleaning up tempOrgRedirect for user", user.username);
+ await prisma.tempOrgRedirect.deleteMany({
+ where: {
</file context>
| log.debug("Cleaning up tempOrgRedirect for user", user.username); | |
| log.debug("Cleaning up tempOrgRedirect for user"); |
✅ Addressed in 6b0d0d6
| }); | ||
| } | ||
|
|
||
| if (userMembership?.role === MembershipRole.OWNER && ownerCount <= 1) { |
There was a problem hiding this comment.
This last-owner guard should only fire for accepted owners; pending owner invitations are currently blocked from leaving even when another accepted owner remains. Please include the member’s accepted status in the select and require it to be true before throwing.
Prompt for AI agents
Address the following comment on packages/features/ee/teams/services/teamService.ts at line 292:
<comment>This last-owner guard should only fire for accepted owners; pending owner invitations are currently blocked from leaving even when another accepted owner remains. Please include the member’s accepted status in the select and require it to be true before throwing.</comment>
<file context>
@@ -269,32 +269,53 @@ export class TeamService {
});
- }
+
+ if (userMembership?.role === MembershipRole.OWNER && ownerCount <= 1) {
+ throw new TRPCError({
+ code: "FORBIDDEN",
</file context>
| // Cannot change the role of the only owner | ||
| if (targetMembership?.role === MembershipRole.OWNER && !teamHasMoreThanOneOwner) { | ||
| // Cannot change the role of the only owner (unless it's self-changing) | ||
| if (targetMembership?.role === MembershipRole.OWNER && !teamHasMoreThanOneOwner && memberId !== userId) { |
There was a problem hiding this comment.
This change lets the only owner demote themselves, leaving the team without any owner. Keep blocking self-demotion unless the new role stays OWNER.
Prompt for AI agents
Address the following comment on packages/features/pbac/services/legacy-role-manager.service.ts at line 48:
<comment>This change lets the only owner demote themselves, leaving the team without any owner. Keep blocking self-demotion unless the new role stays OWNER.</comment>
<file context>
@@ -44,8 +44,8 @@ export class LegacyRoleManager implements IRoleManager {
- // Cannot change the role of the only owner
- if (targetMembership?.role === MembershipRole.OWNER && !teamHasMoreThanOneOwner) {
+ // Cannot change the role of the only owner (unless it's self-changing)
+ if (targetMembership?.role === MembershipRole.OWNER && !teamHasMoreThanOneOwner && memberId !== userId) {
throw new RoleManagementError(
"You can not change the role of the only owner of a team.",
</file context>
| if (targetMembership?.role === MembershipRole.OWNER && !teamHasMoreThanOneOwner && memberId !== userId) { | |
| if (targetMembership?.role === MembershipRole.OWNER && !teamHasMoreThanOneOwner && (memberId !== userId || newRole !== MembershipRole.OWNER)) { |
✅ Addressed in ba06d45
| // If this is the last owner, prevent the role change | ||
| if (ownerCount <= 1) { | ||
| // If this is the last owner and it's not a self-change, prevent the role change | ||
| if (ownerCount <= 1 && currentMembership.userId !== userId) { |
There was a problem hiding this comment.
This change lets the last remaining owner demote themself, leaving the organization without any owners. Please keep the last-owner protection in place even for self-initiated role changes.
Prompt for AI agents
Address the following comment on packages/features/pbac/services/pbac-role-manager.service.ts at line 90:
<comment>This change lets the last remaining owner demote themself, leaving the organization without any owners. Please keep the last-owner protection in place even for self-initiated role changes.</comment>
<file context>
@@ -84,8 +86,8 @@ export class PBACRoleManager implements IRoleManager {
- // If this is the last owner, prevent the role change
- if (ownerCount <= 1) {
+ // If this is the last owner and it's not a self-change, prevent the role change
+ if (ownerCount <= 1 && currentMembership.userId !== userId) {
throw new RoleManagementError(
"Cannot change the role of the last owner in the organization",
</file context>
| if (ownerCount <= 1 && currentMembership.userId !== userId) { | |
| if (ownerCount <= 1) { |
✅ Addressed in 22b76e9
|
This PR has been marked as stale due to inactivity. If you're still working on it or need any help, please let us know or update the PR to keep it active. |
|
This PR has been closed due to inactivity. Please feel free to reopen it if you'd like to continue the work. |
|
Hey @dhairyashiil , thanks for reopening the PR. I see there are new merge conflicts in MemberList.tsx and UserListTable.tsx. Before I start resolving them, just wanted to confirm: → Should I rebase my branch on top of the latest main and push the updated version? Once I get your confirmation, I’ll resolve the conflicts and re-run the tests so the checks can pass. Let me know — happy to fix everything needed. |
There was a problem hiding this comment.
4 issues found across 8 files
Prompt for AI agents (all 4 issues)
Understand the root cause of the following 4 issues and fix them.
<file name="packages/features/ee/teams/components/MemberList.tsx">
<violation number="1" location="packages/features/ee/teams/components/MemberList.tsx:501">
The desktop “View public page” button now drops user.bookerUrl and links to a dashboard-relative path, so it opens the dashboard host instead of the member’s actual booking domain.</violation>
</file>
<file name="packages/features/pbac/services/legacy-role-manager.service.ts">
<violation number="1" location="packages/features/pbac/services/legacy-role-manager.service.ts:48">
The updated guard now lets the last remaining owner demote themselves, leaving the team with zero owners and no way to recover ownership. Keep blocking role changes that would remove the final owner, even for self-edits, while still allowing no-op edits.</violation>
</file>
<file name="packages/features/pbac/services/pbac-role-manager.service.ts">
<violation number="1" location="packages/features/pbac/services/pbac-role-manager.service.ts:90">
Allowing the last remaining owner to bypass the owner-count guard means they can demote themselves and leave the organization without any owner, violating the invariant that each org must always retain at least one owner.</violation>
</file>
<file name="packages/lib/server/service/teamService.ts">
<violation number="1" location="packages/lib/server/service/teamService.ts:463">
Rule violated: **Avoid Logging Sensitive Information**
Debug logging must not include PII. This line writes each member’s username into the logs, violating the “Avoid Logging Sensitive Information” rule. Remove or anonymize the identifier before logging.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
|
@cubic-dev-ai review the PR |
@pallava-joshi I've started the AI code review. It'll take a few minutes to complete. |
There was a problem hiding this comment.
3 issues found across 8 files
Prompt for AI agents (all 3 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/lib/server/service/teamService.ts">
<violation number="1" location="packages/lib/server/service/teamService.ts:132">
Await the asynchronous deleteDomain call so domain cleanup errors are handled and the promise chain stays intact.</violation>
</file>
<file name="packages/features/pbac/services/pbac-role-manager.service.ts">
<violation number="1" location="packages/features/pbac/services/pbac-role-manager.service.ts:104">
validateNotLastOwner now accepts and receives userId but never uses it, so the “last owner” guard still throws for self-changes and owners are still unable to edit their own membership.</violation>
</file>
<file name="packages/features/ee/teams/components/MemberList.tsx">
<violation number="1" location="packages/features/ee/teams/components/MemberList.tsx:501">
The desktop "View public page" button now drops `user.bookerUrl`, so it opens `/username` on the admin host instead of the member’s booking domain and 404s on branded domains. Restore the original `${user.bookerUrl}/${user.username}` URL construction.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
1 issue found across 2 files (reviewed changes from recent commits).
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/lib/server/service/teamService.ts">
<violation number="1" location="packages/lib/server/service/teamService.ts:132">
Awaiting `deleteDomain` without handling errors turns the “non-critical” cleanup step into a hard failure; wrap the call in a catch/log so domain API glitches don’t abort `TeamService.delete`.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
|
|
||
| // Step 4: Clean up any final, non-critical external state. | ||
| if (deletedTeam && deletedTeam.isOrganization && deletedTeam.slug) { | ||
| await deleteDomain(deletedTeam.slug); |
There was a problem hiding this comment.
Awaiting deleteDomain without handling errors turns the “non-critical” cleanup step into a hard failure; wrap the call in a catch/log so domain API glitches don’t abort TeamService.delete.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/lib/server/service/teamService.ts, line 132:
<comment>Awaiting `deleteDomain` without handling errors turns the “non-critical” cleanup step into a hard failure; wrap the call in a catch/log so domain API glitches don’t abort `TeamService.delete`.</comment>
<file context>
@@ -129,7 +129,7 @@ export class TeamService {
// Step 4: Clean up any final, non-critical external state.
if (deletedTeam && deletedTeam.isOrganization && deletedTeam.slug) {
- deleteDomain(deletedTeam.slug);
+ await deleteDomain(deletedTeam.slug);
}
</file context>
Devin AI is resolving merge conflictsThis PR has merge conflicts with the Devin will:
If you prefer to resolve conflicts manually, you can close the Devin session and handle it yourself. |
Devin AI is resolving merge conflictsThis PR has merge conflicts with the Devin will:
If you prefer to resolve conflicts manually, you can close the Devin session and handle it yourself. |
Ryukemeister
left a comment
Ryukemeister
left a comment
There was a problem hiding this comment.
hi @antcybersec, thank for you contribution. i'm gonna close this in favour of #24407 since we think the fix is implemented in a much better way there, thanks again!
5 participants
Allows org owners/admins to see the edit (ellipsis) menu on their own row by permitting self-edit when user is admin/owner. Changes limited to UserListTable.tsx and MemberList.tsx. Fixes #24377.
Screen.Recording.2025-10-15.at.11.49.07.PM.mov
here is the new video ...
Screen.Recording.2025-10-16.at.1.34.17.PM.1.mov