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. |
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) (falling back to findPlatformAdminUserId) in oAuthClientStrategy instead of profilesRepository.getPlatformOwnerUserId(...). MembershipsRepository gained two methods: findPlatformOwnerUserId(organizationId) and findPlatformAdminUserId(organizationId), each querying memberships for teamId = organizationId, role = OWNER/ADMIN and accepted = true and returning the userId or null. 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
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ 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)
✨ 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.
|
@Devanshusharma2005 unit tests have been passed, and the changes are also not complex. |
|
@anikdhabal can you review this sir... |
|
@sahitya-chandra We will review it. Can you pls add a loom video? |
|
@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.
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.