chore: Use enums instead of hardcoded strings#24132
Conversation
WalkthroughThis change replaces string literal role checks with enum-based comparisons across auth, bookings, impersonation, organizations, and webhook modules. It imports MembershipRole and UserPermissionRole from "@calcom/prisma/enums" and updates conditionals to use these enum values at runtime. Several type-only imports are converted to value imports to enable runtime comparisons. No function signatures or public interfaces are modified; logic flow remains the same while aligning role checks to enums. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
dosubot
bot
added
the
🧹 Improvements
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/lib/server/repository/webhook.ts (1)
62-88: Avoid returning webhook.secret; restrict selects on user.webhooks.You fetch user.webhooks without a select, likely pulling secret too. This risks accidental leakage to callers and violates the “select only what you need” guideline. Select explicit non‑secret fields.
const user = await prisma.user.findUnique({ where: { id: userId }, select: { id: true, username: true, name: true, avatarUrl: true, - webhooks: true, + webhooks: { + select: { + id: true, + appId: true, + subscriberUrl: true, + payloadTemplate: true, + active: true, + eventTriggers: true, + time: true, + timeUnit: true, + platform: true, + teamId: true, + }, + }, teams: { where: { accepted: true, }, select: { role: true, team: { select: { id: true, name: true, slug: true, logoUrl: true, - webhooks: true, + webhooks: { + select: { + id: true, + appId: true, + subscriberUrl: true, + payloadTemplate: true, + active: true, + eventTriggers: true, + time: true, + timeUnit: true, + platform: true, + teamId: true, + }, + }, }, }, }, }, }, });packages/features/auth/lib/next-auth-options.ts (3)
148-148: Remove sensitive credential logging in authorize() handlers.Logging credentials (even at debug with safeStringify) is a security risk and contradicts past guidance for this file. Remove or redact these logs.
- log.debug("CredentialsProvider:credentials:authorize", safeStringify({ credentials })); + log.debug("CredentialsProvider:credentials:authorize"); @@ - log.debug("CredentialsProvider:saml-idp:authorize", safeStringify({ credentials })); + log.debug("CredentialsProvider:saml-idp:authorize");Based on learnings
Also applies to: 352-352
519-542: Use select instead of include for Prisma queries (teams).Guideline: always select only what you need. Replace include with select and trim nested fields.
- const existingUser = await prisma.user.findFirst({ - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - where: { email: token.email! }, - select: { - id: true, - username: true, - avatarUrl: true, - name: true, - email: true, - role: true, - locale: true, - movedToProfileId: true, - teams: { - include: { - team: { - select: { - id: true, - metadata: true, - }, - }, - }, - }, - }, - }); + const existingUser = await prisma.user.findFirst({ + where: { email: token.email! }, + select: { + id: true, + username: true, + avatarUrl: true, + name: true, + email: true, + role: true, + locale: true, + movedToProfileId: true, + teams: { + select: { + team: { select: { id: true, metadata: true } }, + }, + }, + }, + });As per coding guidelines
425-431: Comment/value mismatch for email maxAge.The comment says “10 min” but maxAge is 10 hours (10 * 60 * 60). Fix the comment or value.
- maxAge: 10 * 60 * 60, // Magic links are valid for 10 min only + maxAge: 10 * 60, // Magic links are valid for 10 min onlypackages/features/ee/organizations/lib/OrganizationPermissionService.ts (1)
1-35: Finish migrating this service to enum-based role checks.
hasPermissionToCreateForEmailstill compares against the raw"ADMIN"string, and we’re pullingUserPermissionRolefrom the Kysely type bundle instead of the runtime enum module. To stay consistent with the rest of this PR (and to avoid falling back to string literals), importUserPermissionRolefrom@calcom/prisma/enumsand compare againstUserPermissionRole.ADMIN.-import { UserPermissionRole } from "@calcom/kysely/types"; +import { UserPermissionRole } from "@calcom/prisma/enums"; … - return this.user.email === targetEmail || this.user.role === "ADMIN"; + return this.user.email === targetEmail || this.user.role === UserPermissionRole.ADMIN;
🧹 Nitpick comments (5)
packages/features/ee/organizations/lib/OrganizationPaymentService.ts (1)
120-130: Replace BillingPeriod string literals with enum values.Stay consistent with the PR theme and prevent drift by using BillingPeriod enum values instead of "MONTHLY"/"ANNUALLY".
- import { UserPermissionRole, type BillingPeriod } from "@calcom/prisma/enums"; + import { UserPermissionRole, BillingPeriod } from "@calcom/prisma/enums"; protected normalizePaymentConfig(input: { billingPeriod?: BillingPeriod; @@ }): PaymentConfig { return { - billingPeriod: input.billingPeriod || "MONTHLY", + billingPeriod: input.billingPeriod || BillingPeriod.MONTHLY, seats: input.seats || Number(ORGANIZATION_SELF_SERVE_MIN_SEATS), pricePerSeat: input.pricePerSeat || Number(ORGANIZATION_SELF_SERVE_PRICE), }; } @@ - const { interval, occurrence } = (() => { - if (config.billingPeriod === "MONTHLY") return { interval: "month" as const, occurrence: 1 }; - if (config.billingPeriod === "ANNUALLY") return { interval: "year" as const, occurrence: 12 }; + const { interval, occurrence } = (() => { + if (config.billingPeriod === BillingPeriod.MONTHLY) return { interval: "month" as const, occurrence: 1 }; + if (config.billingPeriod === BillingPeriod.ANNUALLY) return { interval: "year" as const, occurrence: 12 }; throw new Error(`Invalid billing period: ${config.billingPeriod}`); })();Also applies to: 223-227
packages/features/auth/lib/next-auth-options.ts (3)
821-833: Replace include with select (accounts/password).Prefer select; avoid over‑fetching. Also only fetch the fields you actually read.
- let existingUser = await prisma.user.findFirst({ - include: { - password: { - select: { - hash: true, - }, - }, - accounts: { - where: { - provider: account.provider, - }, - }, - }, - where: { - identityProvider: idP, - identityProviderId: account.providerAccountId, - }, - }); + let existingUser = await prisma.user.findFirst({ + where: { + identityProvider: idP, + identityProviderId: account.providerAccountId, + }, + select: { + id: true, + email: true, + emailVerified: true, + username: true, + twoFactorEnabled: true, + password: { select: { hash: true } }, + accounts: { + where: { provider: account.provider }, + select: { id: true, provider: true, providerAccountId: true }, + }, + }, + });As per coding guidelines
839-870: Legacy fallback query: also convert include → select.Mirror the above select‑based shape to keep consistency and avoid over‑fetching.
60-70: Type cast to UserPermissionRole may mask "INACTIVE_ADMIN".AdapterUserPresenter casts role as UserPermissionRole even when it can be "INACTIVE_ADMIN". Consider widening the field type or mapping “INACTIVE_ADMIN” to a separate property to avoid type/semantic drift.
- fromCalUser: ( - user: UserWithProfiles, - role: UserPermissionRole | "INACTIVE_ADMIN", - hasActiveTeams: boolean - ) => ({ + fromCalUser: ( + user: UserWithProfiles, + role: UserPermissionRole | "INACTIVE_ADMIN", + hasActiveTeams: boolean + ) => ({ ...user, - role: role as UserPermissionRole, + role,Audit downstream uses of user.role expecting strict UserPermissionRole; if needed, model
inactiveAdmin: booleanseparately.packages/features/bookings/lib/payment/processNoShowFeeOnCancellation.ts (1)
47-49: Replace literal "HOLD" with enumPaymentOption.HOLD
Add an import forPaymentOptionat the top ofprocessNoShowFeeOnCancellation.ts(alongsidePayment) and update the finder accordingly:-import type { Payment } from "@calcom/prisma/client"; +import type { Payment, PaymentOption } from "@calcom/prisma/client"; const paymentToCharge = payments.find( - (payment) => payment.paymentOption === "HOLD" && payment.success === false + (payment) => payment.paymentOption === PaymentOption.HOLD && payment.success === false );
📜 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 (7)
packages/features/auth/lib/checkAdminOrOwner.ts(1 hunks)packages/features/auth/lib/next-auth-options.ts(2 hunks)packages/features/bookings/lib/payment/processNoShowFeeOnCancellation.ts(2 hunks)packages/features/ee/impersonation/lib/ImpersonationProvider.ts(6 hunks)packages/features/ee/organizations/lib/OrganizationPaymentService.ts(2 hunks)packages/features/ee/organizations/lib/OrganizationPermissionService.ts(3 hunks)packages/lib/server/repository/webhook.ts(2 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:
packages/features/auth/lib/next-auth-options.tspackages/features/auth/lib/checkAdminOrOwner.tspackages/features/ee/organizations/lib/OrganizationPermissionService.tspackages/features/ee/organizations/lib/OrganizationPaymentService.tspackages/features/ee/impersonation/lib/ImpersonationProvider.tspackages/features/bookings/lib/payment/processNoShowFeeOnCancellation.tspackages/lib/server/repository/webhook.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/auth/lib/next-auth-options.tspackages/features/auth/lib/checkAdminOrOwner.tspackages/features/ee/organizations/lib/OrganizationPermissionService.tspackages/features/ee/organizations/lib/OrganizationPaymentService.tspackages/features/ee/impersonation/lib/ImpersonationProvider.tspackages/features/bookings/lib/payment/processNoShowFeeOnCancellation.tspackages/lib/server/repository/webhook.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/features/auth/lib/next-auth-options.tspackages/features/auth/lib/checkAdminOrOwner.tspackages/features/ee/organizations/lib/OrganizationPermissionService.tspackages/features/ee/organizations/lib/OrganizationPaymentService.tspackages/features/ee/impersonation/lib/ImpersonationProvider.tspackages/features/bookings/lib/payment/processNoShowFeeOnCancellation.tspackages/lib/server/repository/webhook.ts
**/*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/features/ee/organizations/lib/OrganizationPermissionService.tspackages/features/ee/organizations/lib/OrganizationPaymentService.ts
🧠 Learnings (2)
📚 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:
packages/features/auth/lib/next-auth-options.ts
📚 Learning: 2025-08-23T13:45:16.529Z
Learnt from: shaun-ak
PR: calcom/cal.com#23233
File: packages/features/users/components/UserTable/EditSheet/EditUserForm.tsx:100-101
Timestamp: 2025-08-23T13:45:16.529Z
Learning: In Cal.com's team structure, the organization team (root team) is identified by having no parentId (!team.parentId), while sub-teams within an organization have a parentId. Use selectedUser?.teams?.find((team) => !team.parentId) to get the organization team, not by matching org?.id.
Applied to files:
packages/features/ee/impersonation/lib/ImpersonationProvider.ts
🧬 Code graph analysis (4)
packages/features/auth/lib/checkAdminOrOwner.ts (1)
packages/platform/libraries/index.ts (1)
MembershipRole(34-34)
packages/features/ee/organizations/lib/OrganizationPermissionService.ts (1)
packages/platform/libraries/index.ts (1)
MembershipRole(34-34)
packages/features/ee/impersonation/lib/ImpersonationProvider.ts (2)
packages/features/auth/lib/next-auth-options.ts (1)
session(746-771)packages/platform/libraries/index.ts (1)
MembershipRole(34-34)
packages/features/bookings/lib/payment/processNoShowFeeOnCancellation.ts (1)
packages/platform/libraries/index.ts (1)
MembershipRole(34-34)
🔇 Additional comments (6)
packages/features/ee/organizations/lib/OrganizationPaymentService.ts (2)
13-13: Enum value import LGTM.Using UserPermissionRole as a value for runtime checks is correct and aligns with the PR goal.
297-305: TrpcSessionUser.role is correctly typed as UserPermissionRole.packages/features/bookings/lib/payment/processNoShowFeeOnCancellation.ts (1)
5-5: Enum swap for role checks looks good.Using MembershipRole.OWNER/ADMIN for the cancellation bypass is correct and consistent with the PR objective.
Also applies to: 36-39
packages/lib/server/repository/webhook.ts (1)
6-6: Enum‑based admin check approved.Switching to UserPermissionRole.ADMIN is correct.
Also applies to: 165-166
packages/features/auth/lib/next-auth-options.ts (1)
45-45: Enum usage for admin checks looks good.Switching to UserPermissionRole.ADMIN is correct and consistent across the PR.
Also applies to: 252-267
packages/features/auth/lib/checkAdminOrOwner.ts (1)
1-5: Ignore suggestion to widen the predicate toMembershipRole
The current guardrole is "OWNER" | "ADMIN"correctly narrows to those two enum values; changing it torole is MembershipRolewould wrongly include all roles. If you want to reference the enum directly, define a narrow alias—e.g.type AdminOrOwner = MembershipRole.OWNER | MembershipRole.ADMIN; function checkAdminOrOwner( role: MembershipRole | null | undefined ): role is AdminOrOwner { … }—but no changes are needed here.
Likely an incorrect or invalid review comment.
E2E results are ready! |
3 participants
What does this PR do?
Uses enums across the app instead of hard coded strings. Makes this easier for PBAC migration