refactor: permission-check mocking behaviour#23046
Conversation
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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
CodeRabbit Configuration File (
|
Graphite Automations"Add foundation 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: 0
🔭 Outside diff range comments (4)
packages/features/flags/features.repository.ts (1)
70-82: Replace Prisma include with select to comply with org-wide guideline.Per guidelines, avoid include and select only the data you need. Refactor teamFeatures query to use select with nested selects.
Apply this diff:
- const result = await this.prismaClient.teamFeatures.findMany({ - where: { - teamId, - }, - include: { - feature: { - select: { - slug: true, - enabled: true, - }, - }, - }, - }); + const result = await this.prismaClient.teamFeatures.findMany({ + where: { teamId }, + select: { + feature: { + select: { + slug: true, + enabled: true, + }, + }, + }, + });packages/lib/server/repository/membership.ts (1)
306-315: DI regression: instance method bypasses injected prismaClient.findUniqueByUserIdAndTeamId is an instance method but calls the global prisma, defeating DI and making tests harder to isolate. Also, select only needed fields per guideline.
Apply this diff:
- async findUniqueByUserIdAndTeamId({ userId, teamId }: { userId: number; teamId: number }) { - return await prisma.membership.findUnique({ - where: { - userId_teamId: { - userId, - teamId, - }, - }, - }); - } + async findUniqueByUserIdAndTeamId({ userId, teamId }: { userId: number; teamId: number }) { + return await this.prismaClient.membership.findUnique({ + where: { + userId_teamId: { + userId, + teamId, + }, + }, + select: { + id: true, + teamId: true, + userId: true, + accepted: true, + role: true, + customRoleId: true, + }, + }); + }packages/features/pbac/services/__tests__/permission-check.service.test.ts (1)
697-735: Add a test for org-level explicit permission in checkPermission.Currently, hasPermission checks explicit permission at team level but only checks manage at org level. Once fixed (see service change), add a test ensuring org-level explicit permission grants access when team-level fails.
Example test to add:
it("should return true when org has explicit permission (no manage)", async () => { const membership = { id: 1, teamId: 1, userId: 1, accepted: true, role: "MEMBER" as MembershipRole, customRoleId: "team_role", disableImpersonation: false, createdAt: new Date(), updatedAt: new Date() }; vi.spyOn(mockMembershipRepository, "findUniqueByUserIdAndTeamId").mockResolvedValueOnce(membership); vi.spyOn(mockFeaturesRepository, "checkIfTeamHasFeature").mockResolvedValueOnce(true); vi.spyOn(mockRepository, "getMembershipByMembershipId").mockResolvedValueOnce({ id: membership.id, teamId: membership.teamId, userId: membership.userId, customRoleId: membership.customRoleId, team: { parentId: 2 }, }); vi.spyOn(mockRepository, "getOrgMembership").mockResolvedValueOnce({ id: 2, teamId: 2, userId: 1, customRoleId: "org_role", }); // Team explicit + manage both fail; org explicit succeeds vi.spyOn(mockRepository, "checkRolePermission") .mockResolvedValueOnce(false) // team explicit .mockResolvedValueOnce(false) // team manage .mockResolvedValueOnce(true); // org explicit const result = await service.checkPermission({ userId: 1, teamId: 1, permission: "eventType.delete", fallbackRoles: ["ADMIN", "OWNER"], }); expect(result).toBe(true); });packages/features/pbac/services/permission-check.service.ts (1)
217-246: Bug: org-level explicit permission isn’t checked in hasPermission.Team-level checks verify explicit permission and manage; org-level only checks manage. This is inconsistent with hasPermissions (which checks explicit org permissions first) and can incorrectly deny access when the org grants an explicit permission.
Apply this diff:
private async hasPermission(query: PermissionCheck, permission: PermissionString): Promise<boolean> { const { membership, orgMembership } = await this.getMembership(query); @@ // If no team permission, check org-level permissions if (orgMembership?.customRoleId) { - const [resource] = permission.split("."); - const managePermission = `${resource}.manage` as PermissionString; - return this.dependencies.repository.checkRolePermission(orgMembership.customRoleId, managePermission); + // First check explicit permission at org level + const hasOrgPermission = await this.dependencies.repository.checkRolePermission( + orgMembership.customRoleId, + permission + ); + if (hasOrgPermission) return true; + + // Fallback: check manage at org level + const [resource] = permission.split("."); + const managePermission = `${resource}.manage` as PermissionString; + return this.dependencies.repository.checkRolePermission(orgMembership.customRoleId, managePermission); }
🧹 Nitpick comments (3)
packages/features/flags/features.repository.ts (1)
38-41: Consider selecting only needed fields in getAllFeatures.You only use slug and enabled downstream. To reduce payload and adhere to “only select what you need”, consider selecting those fields. Verify external consumers of getAllFeatures won’t break.
Proposed change:
- const features = await this.prismaClient.feature.findMany({ - orderBy: { slug: "asc" }, - cacheStrategy: { swr: 300, ttl: 300 }, - }); + const features = await this.prismaClient.feature.findMany({ + select: { slug: true, enabled: true }, + orderBy: { slug: "asc" }, + cacheStrategy: { swr: 300, ttl: 300 }, + });If other callers require additional columns, consider introducing a dedicated method (e.g., getAllFeatureFlagsLight) instead of changing this one.
packages/lib/server/repository/membership.ts (1)
317-320: Static wrapper: add deprecation note to steer callers to DI.The static delegator is fine for back-compat, but it encourages non-DI usage. Consider annotating as deprecated to nudge migrations.
Suggested JSDoc:
/** * @deprecated Prefer creating an instance of MembershipRepository (with injected PrismaClient) and * calling instance.findUniqueByUserIdAndTeamId for testability and DI compliance. */ static async findUniqueByUserIdAndTeamId(passThroughProps: { userId: number; teamId: number }) { const instance = new MembershipRepository(); return instance.findUniqueByUserIdAndTeamId(passThroughProps); }packages/features/pbac/services/__tests__/permission-check.service.test.ts (1)
20-33: Minor: you can configure stubs directly without spyOn for brevity.Since mockRepository methods are already vi.fn(), prefer setting return values directly (e.g., mockRepository.getUserMemberships!.mockResolvedValueOnce(...)) to reduce boilerplate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/features/flags/features.repository.ts(2 hunks)packages/features/pbac/services/__tests__/permission-check.service.test.ts(38 hunks)packages/features/pbac/services/permission-check.service.ts(17 hunks)packages/lib/server/repository/membership.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{service,repository}.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Avoid dot-suffixes like
.service.tsor.repository.tsfor new files; reserve.test.ts,.spec.ts,.types.tsfor their specific purposes
Files:
packages/features/flags/features.repository.tspackages/features/pbac/services/permission-check.service.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/features/flags/features.repository.tspackages/features/pbac/services/permission-check.service.tspackages/features/pbac/services/__tests__/permission-check.service.test.tspackages/lib/server/repository/membership.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/features/flags/features.repository.tspackages/features/pbac/services/permission-check.service.tspackages/features/pbac/services/__tests__/permission-check.service.test.tspackages/lib/server/repository/membership.ts
🧬 Code Graph Analysis (2)
packages/features/flags/features.repository.ts (1)
packages/prisma/index.ts (1)
PrismaClient(83-83)
packages/features/pbac/services/__tests__/permission-check.service.test.ts (5)
packages/features/pbac/domain/repositories/IPermissionRepository.ts (1)
IPermissionRepository(4-66)packages/features/flags/features.repository.ts (1)
FeaturesRepository(19-255)packages/features/pbac/services/permission.service.ts (1)
PermissionService(17-115)packages/lib/server/repository/membership.ts (1)
MembershipRepository(83-468)packages/features/pbac/services/permission-check.service.ts (1)
PermissionCheckService(25-385)
⏰ 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: Detect changes
🔇 Additional comments (4)
packages/features/flags/features.repository.ts (1)
22-22: Good DI improvement: default-inject the Prisma singleton.Constructor defaulting to the prisma singleton simplifies wiring and aligns with the DI move. No concerns here.
packages/features/pbac/services/__tests__/permission-check.service.test.ts (1)
45-50: Nice shift to DI-based construction in tests.Passing a dependencies object into PermissionCheckService and using spies on injected mocks makes tests cleaner and removes brittle module-level mocking.
packages/features/pbac/services/permission-check.service.ts (2)
31-45: Static build() wiring looks solid.Dynamic imports avoid hard runtime coupling and keep constructor DI-friendly. This aligns with the new test strategy.
137-141: Confirm MembershipRepository select covers required fields.checkPermission(s) dereferences membership.id, membership.role, and membership.customRoleId. Ensure MembershipRepository.findUniqueByUserIdAndTeamId selects these fields (and accepted if needed). See repository fix suggestion.
Also applies to: 186-190
|
This PR is being marked as stale due to inactivity. |
| import { captureException } from "@sentry/nextjs"; | ||
|
|
||
| import type { PrismaClient } from "@calcom/prisma"; | ||
| import prisma from "@calcom/prisma"; |
There was a problem hiding this comment.
We should use import {prisma} from "@calcom/prisma";
|
Is this still relevant? If so, can you please merge conflicts and resolve type-check errors? Otherwise let's close it :) |
|
This PR is being marked as stale due to inactivity. |
3 participants
What does this PR do?