fix: Use Memberships with OWNER role for platform owner lookup #22475
Conversation
|
@sahitya-chandra is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
|
|
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (07/14/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (07/14/25)1 label was added to this PR based on Keith Williams's automation. "Add ready-for-e2e label" took an action on this PR • (08/13/25)1 label was added to this PR based on Keith Williams's automation. |
There was a problem hiding this comment.
cubic reviewed 2 files and found no issues. Review PR in cubic.dev.
WalkthroughApiAuthStrategy now injects MembershipsRepository and uses membershipsRepository.findPlatformOwnerUserId(client.organizationId) with a fallback to findPlatformAdminUserId(client.organizationId) in oAuthClientStrategy instead of profilesRepository.getPlatformOwnerUserId(...). MembershipsRepository gained two methods: findPlatformOwnerUserId and findPlatformAdminUserId, each querying memberships for teamId = organizationId, role = OWNER/ADMIN and accepted = true and returning userId or null. AuthModule no longer imports ProfilesModule. The ApiAuthStrategy constructor parameter order changed and the owner-not-found error message was updated. Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Out-of-scope changes
Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/v2/src/modules/auth/strategies/api-auth/api-auth.strategy.ts(3 hunks)apps/api/v2/src/modules/memberships/memberships.repository.ts(1 hunks)
⏰ 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). (2)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Security Check
🔇 Additional comments (3)
apps/api/v2/src/modules/memberships/memberships.repository.ts (1)
23-36: LGTM! The method correctly implements platform owner lookup.The implementation properly queries the membership table with the correct filters (teamId, role, accepted) and handles the null case appropriately. This addresses the issue where the previous approach could incorrectly identify managed users as platform owners.
apps/api/v2/src/modules/auth/strategies/api-auth/api-auth.strategy.ts (2)
7-7: LGTM! Dependency injection implemented correctly.The MembershipsRepository is properly imported and injected following the established patterns in the codebase.
Also applies to: 62-62
188-188: LGTM! Method call change correctly implements the fix.The change from
profilesRepository.getPlatformOwnerUserIdtomembershipsRepository.findPlatformOwnerUserIdproperly addresses the issue where managed users could be incorrectly identified as platform owners. The new approach correctly validates actual ownership through membership roles.
There was a problem hiding this comment.
hey @sahitya-chandra thanks for the pr, can you please address the unit test failing. Also it will be good to add a loom.
|
@kart1ka sir, I am unable to create a loom video of this fix because locally running the api needs x_cal_secret which I don't have... |
|
other than the one small change looks good to me, @supalarry can you have a look at this as well? |
| @@ -172,7 +174,7 @@ export class ApiAuthStrategy extends PassportStrategy(BaseStrategy, "api-auth") | |||
| throw new UnauthorizedException("ApiAuthStrategy - oAuth client - Invalid client secret"); | |||
| } | |||
|
|
|||
| const platformCreatorId = await this.profilesRepository.getPlatformOwnerUserId(client.organizationId); | |||
| const platformCreatorId = await this.membershipsRepository.findPlatformOwnerUserId(client.organizationId); | |||
There was a problem hiding this comment.
I would add a new function 'findPlatformAdminUserId' that does the same thing as 'findPlatformOwnerUserId' but with role 'MembershipRole.ADMIN' and then have
const platformCreatorId = await this.membershipsRepository.findPlatformOwnerUserId(client.organizationId) || await this.membershipsRepository.findPlatformAdminUserId(client.organizationId;
as a fallback just in case owner of platform is not part of the platform anymore and rest of people who were added were added as admins.
There was a problem hiding this comment.
@supalarry added the admin fallback
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
apps/api/v2/src/modules/auth/strategies/api-auth/api-auth.strategy.ts (1)
7-12: Import MembershipsRepository: good direction; remove now-unused ProfilesRepository importSwitching to MembershipsRepository aligns with the PR objective. ProfilesRepository is no longer used in this file—clean it up to avoid dead dependencies and potential lint failures.
Apply this diff to drop the unused import:
@@ -import { ProfilesRepository } from "@/modules/profiles/profiles.repository";
🧹 Nitpick comments (1)
apps/api/v2/src/modules/auth/strategies/api-auth/api-auth.strategy.ts (1)
177-185: Use nullish coalescing for fallback and update error message to reflect admin fallback
- Prefer
??over||to avoid treating a legitimate userId0as falsy (defensive correctness).- Error message should mention owner/admin since we now fallback to admin.
Apply this diff:
@@ - const platformCreatorId = - (await this.membershipsRepository.findPlatformOwnerUserId(client.organizationId)) || - (await this.membershipsRepository.findPlatformAdminUserId(client.organizationId)); + const platformCreatorId = + (await this.membershipsRepository.findPlatformOwnerUserId(client.organizationId)) ?? + (await this.membershipsRepository.findPlatformAdminUserId(client.organizationId)); @@ - if (!platformCreatorId) { - throw new UnauthorizedException( - "ApiAuthStrategy - oAuth client - No owner ID found for this OAuth client" - ); - } + if (!platformCreatorId) { + throw new UnauthorizedException( + "ApiAuthStrategy - oAuth client - No owner or admin found for this OAuth client" + ); + }Happy to add unit tests covering:
- Owner present
- Owner absent, admin present (fallback)
- Owner/admin both absent (Unauthorized)
- Regression: managed user not selected
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/v2/src/modules/auth/strategies/api-auth/api-auth.strategy.ts(3 hunks)apps/api/v2/src/modules/memberships/memberships.repository.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/v2/src/modules/memberships/memberships.repository.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/api/v2/src/modules/auth/strategies/api-auth/api-auth.strategy.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/api/v2/src/modules/auth/strategies/api-auth/api-auth.strategy.ts
⏰ 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: Install dependencies / Yarn install & cache
🔇 Additional comments (3)
apps/api/v2/src/modules/auth/strategies/api-auth/api-auth.strategy.ts (3)
177-179: LGTM: Owner/Admin lookup via Memberships fixes the managed-user bugUsing Memberships with OWNER (and ADMIN as fallback) correctly resolves the platform owner deterministically and prevents managed users from being misidentified.
177-179: Repository methods comply with Prisma select-only guideline
Both findPlatformOwnerUserId and findPlatformAdminUserId in apps/api/v2/src/modules/memberships/memberships.repository.ts already:
- Filter by accepted: true and the correct MembershipRole
- Use select: { userId: true }
- Do not use any include
No changes required.
7-12: AuthModule correctly imports MembershipsModuleAuthModule (apps/api/v2/src/modules/auth/auth.module.ts) imports MembershipsModule, which exports MembershipsRepository. Dependency injection for MembershipsRepository in ApiAuthStrategy will work at runtime—no changes needed.
apps/api/v2/src/modules/auth/strategies/api-auth/api-auth.strategy.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
apps/api/v2/src/modules/auth/strategies/api-auth/api-auth.strategy.ts (1)
175-183: Use nullish coalescing and update exception message in ApiAuthStrategy
- File: apps/api/v2/src/modules/auth/strategies/api-auth/api-auth.strategy.ts (lines 175–183)
Switch the fallback from
||to??so a valid userId (even if ever 0) isn’t treated as falsy, and tweak the error text to mention both owner and admin.- const platformCreatorId = - (await this.membershipsRepository.findPlatformOwnerUserId(client.organizationId)) || - (await this.membershipsRepository.findPlatformAdminUserId(client.organizationId)); + const platformCreatorId = + (await this.membershipsRepository.findPlatformOwnerUserId(client.organizationId)) ?? + (await this.membershipsRepository.findPlatformAdminUserId(client.organizationId)); - if (!platformCreatorId) { - throw new UnauthorizedException( - "ApiAuthStrategy - oAuth client - No owner ID found for this OAuth client" - ); - } + if (!platformCreatorId) { + throw new UnauthorizedException( + "ApiAuthStrategy - oAuth client - No owner or admin found for this OAuth client" + ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/v2/src/modules/auth/strategies/api-auth/api-auth.strategy.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/api/v2/src/modules/auth/strategies/api-auth/api-auth.strategy.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/api/v2/src/modules/auth/strategies/api-auth/api-auth.strategy.ts
⏰ 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: Install dependencies / Yarn install & cache
🔇 Additional comments (1)
apps/api/v2/src/modules/auth/strategies/api-auth/api-auth.strategy.ts (1)
7-7: Switch to MembershipsRepository import is appropriateImporting MembershipsRepository aligns with the new owner resolution logic and prevents false-positives from Profiles-based lookups. No concerns here.
stale
E2E results are ready! |
|
@sahitya-chandra Could you please fix the failing tests? |
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/api/v2/src/modules/slots/slots-2024-09-04/services/slots.service.spec.ts (1)
14-18: Remove redundant jest.mock for AvailableSlotsServiceYou already provide AvailableSlotsService via Nest DI with a useValue mock below. The top-level jest.mock here is redundant and risks diverging behavior if the service’s shape changes. Rely on the DI-provided mock only.
-jest.mock("@/lib/services/available-slots.service", () => ({ - AvailableSlotsService: jest.fn().mockImplementation(() => ({ - getAvailableSlots: jest.fn(), - })), -}));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/v2/src/modules/auth/guards/billing/platform-plan.guard.spec.ts(1 hunks)apps/api/v2/src/modules/slots/slots-2024-09-04/services/slots.service.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/api/v2/src/modules/auth/guards/billing/platform-plan.guard.spec.tsapps/api/v2/src/modules/slots/slots-2024-09-04/services/slots.service.spec.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/api/v2/src/modules/auth/guards/billing/platform-plan.guard.spec.tsapps/api/v2/src/modules/slots/slots-2024-09-04/services/slots.service.spec.ts
⏰ 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). (2)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Codacy Static Code Analysis
| jest.mock("@calcom/prisma/client", () => ({ | ||
| MembershipRole: { | ||
| OWNER: "OWNER", | ||
| ADMIN: "ADMIN", | ||
| MEMBER: "MEMBER", | ||
| }, | ||
| })); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Prefer partial mocking over full module override for @calcom/prisma/client
Fully overriding the Prisma client module with just MembershipRole risks breaking any transitive imports that rely on other exports. Use a partial mock that preserves the original exports and only overrides MembershipRole. Also consider centralizing this mock in a shared Jest setup to avoid duplication across specs.
Apply this safer partial mock:
-jest.mock("@calcom/prisma/client", () => ({
- MembershipRole: {
- OWNER: "OWNER",
- ADMIN: "ADMIN",
- MEMBER: "MEMBER",
- },
-}));
+jest.mock("@calcom/prisma/client", () => {
+ const actual = jest.requireActual("@calcom/prisma/client");
+ return {
+ ...actual,
+ MembershipRole: {
+ OWNER: "OWNER",
+ ADMIN: "ADMIN",
+ MEMBER: "MEMBER",
+ },
+ };
+});If you want to deduplicate this across specs, add the following to a global Jest setup file (e.g., jest.setup.ts) and reference it in your Jest config:
// jest.setup.ts
jest.mock("@calcom/prisma/client", () => {
const actual = jest.requireActual("@calcom/prisma/client");
return {
...actual,
MembershipRole: { OWNER: "OWNER", ADMIN: "ADMIN", MEMBER: "MEMBER" },
};
});Run this to find all occurrences to consolidate:
🏁 Script executed:
#!/bin/bash
rg -n -A 2 $'jest\\.mock\\(["\\\']@calcom/prisma/client["\\\']'Length of output: 188
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Looking for jest.mock uses of @calcom/prisma/client (double- and single-quoted)..."
rg -n -A2 -S "jest.mock('@calcom/prisma/client'" || true
rg -n -A2 -S 'jest.mock("@calcom/prisma/client"' || true
echo
echo "Fallback: searching for any reference to @calcom/prisma/client..."
rg -n -S "@calcom/prisma/client" || trueLength of output: 36773
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== platform-plan.guard.spec.ts =="
sed -n '1,240p' apps/api/v2/src/modules/auth/guards/billing/platform-plan.guard.spec.ts || true
echo
echo "== slots.service.spec.ts =="
sed -n '1,240p' apps/api/v2/src/modules/slots/slots-2024-09-04/services/slots.service.spec.ts || true
echo
echo "== Files that import @calcom/prisma/client and contain jest.mock =="
rg -n -l "@calcom/prisma/client" | while read -r f; do
if rg -n "jest.mock" "$f" >/dev/null 2>&1; then
echo
echo "File: $f"
rg -n -A2 "jest.mock" "$f" || true
fi
doneLength of output: 40221
Prefer partial mocking + centralize @calcom/prisma/client mock
Full-module overrides of @calcom/prisma/client are present and can break transitive imports. Replace with a partial mock (preserve the real exports, override only MembershipRole) and centralize the mock in a Jest setup file.
Files that need changes:
- apps/api/v2/src/modules/auth/guards/billing/platform-plan.guard.spec.ts
- apps/api/v2/src/modules/slots/slots-2024-09-04/services/slots.service.spec.ts
Apply this replacement in each spec (example diff):
-jest.mock("@calcom/prisma/client", () => ({
- MembershipRole: {
- OWNER: "OWNER",
- ADMIN: "ADMIN",
- MEMBER: "MEMBER",
- },
-}));
+jest.mock("@calcom/prisma/client", () => {
+ const actual = jest.requireActual("@calcom/prisma/client");
+ return {
+ ...actual,
+ MembershipRole: {
+ OWNER: "OWNER",
+ ADMIN: "ADMIN",
+ MEMBER: "MEMBER",
+ },
+ };
+});To deduplicate, add a global setup file and reference it from your Jest config:
// jest.setup.ts
jest.mock("@calcom/prisma/client", () => {
const actual = jest.requireActual("@calcom/prisma/client");
return {
...actual,
MembershipRole: { OWNER: "OWNER", ADMIN: "ADMIN", MEMBER: "MEMBER" },
};
});Add it to your Jest config (setupFiles or setupFilesAfterEnv), e.g.:
// jest.config.js
module.exports = {
setupFiles: ["<rootDir>/jest.setup.ts"],
};To find other occurrences to consolidate, run:
rg -n -F -A2 -e "jest.mock('@calcom/prisma/client'" -e 'jest.mock("@calcom/prisma/client"' || true| jest.mock("@calcom/prisma/client", () => ({ | ||
| MembershipRole: { | ||
| OWNER: "OWNER", | ||
| ADMIN: "ADMIN", | ||
| MEMBER: "MEMBER", | ||
| }, | ||
| })); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Use partial mock for @calcom/prisma/client and consider centralizing it
Same rationale as in PlatformPlanGuard spec: retain all original exports and override only MembershipRole. This avoids accidental breakage of other Prisma exports used by transitive imports. Also consider moving this to a shared Jest setup.
-jest.mock("@calcom/prisma/client", () => ({
- MembershipRole: {
- OWNER: "OWNER",
- ADMIN: "ADMIN",
- MEMBER: "MEMBER",
- },
-}));
+jest.mock("@calcom/prisma/client", () => {
+ const actual = jest.requireActual("@calcom/prisma/client");
+ return {
+ ...actual,
+ MembershipRole: {
+ OWNER: "OWNER",
+ ADMIN: "ADMIN",
+ MEMBER: "MEMBER",
+ },
+ };
+});To consolidate across specs:
🏁 Script executed:
#!/bin/bash
rg -n -A 2 $'jest\\.mock\\(["\\\']@calcom/prisma/client["\\\']'Length of output: 188
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching repository for occurrences of '@calcom/prisma/client' with surrounding context..."
rg -n --hidden --no-ignore -S "@calcom/prisma/client" -g '!node_modules' -A 3 -B 3 || trueLength of output: 209304
Use a partial mock for @calcom/prisma/client and centralize it
Replace full-module mocks with a partial mock (jest.requireActual) so only MembershipRole is overridden — this preserves all other Prisma exports transitive imports may rely on. Prefer moving the override into a shared Jest setup file.
Files to update:
- apps/api/v2/src/modules/slots/slots-2024-09-04/services/slots.service.spec.ts (jest.mock at lines ~20–26)
- apps/api/v2/src/modules/auth/guards/billing/platform-plan.guard.spec.ts (jest.mock at lines ~9–12)
Proposed change (replace the existing jest.mock in each spec):
-jest.mock("@calcom/prisma/client", () => ({
- MembershipRole: {
- OWNER: "OWNER",
- ADMIN: "ADMIN",
- MEMBER: "MEMBER",
- },
-}));
+jest.mock("@calcom/prisma/client", () => {
+ const actual = jest.requireActual("@calcom/prisma/client");
+ return {
+ ...actual,
+ MembershipRole: {
+ OWNER: "OWNER",
+ ADMIN: "ADMIN",
+ MEMBER: "MEMBER",
+ },
+ };
+});To find all spec files that mock the prisma client, run:
#!/bin/bash
rg -n --hidden --no-ignore -S -e "jest\\.mock\\([\"']@calcom/prisma/client[\"']" -g '!node_modules' || true|
@sahitya-chandra We still have a test failing. |
|
@kart1ka I'm still working on it sir... |
What does this PR do?
Replaced the logic for identifying the platform owner from the Profiles table (ordered by createdAt) with a direct lookup in the Memberships table using the 'OWNER' role. This prevents managed users from being mistakenly treated as platform owners if the original owner is deleted.
Visual Demo (For contributors especially)
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist
Summary by cubic
Changed platform owner lookup to use the Memberships table with the OWNER role instead of Profiles, ensuring only valid owners are identified.