fix: identify org-wide features per user (#17010)

Author: zomars

packages/emails/templates/organizer-request-email.ts

@@ -1,4 +1,4 @@
-import { FeaturesRepository } from "@calcom/features/flags/features.repository";
+import { checkIfUserHasFeatureController } from "@calcom/features/flags/operations/check-if-user-has-feature.controller";
 import { EMAIL_FROM_NAME } from "@calcom/lib/constants";
 
 import { renderEmail } from "../";
@@ -7,22 +7,15 @@ import OrganizerScheduledEmail from "./organizer-scheduled-email";
 /**
  * TODO: Remove once fully migrated to V2
  */
-async function getOrganizerRequestTemplate(args: { teamId?: number; userId?: number }) {
-  const featuresRepository = new FeaturesRepository();
-  const hasNewTemplate = await featuresRepository.checkIfTeamOrUserHasFeature(
-    args,
-    "organizer-request-email-v2"
-  );
+async function getOrganizerRequestTemplate(userId?: number) {
+  const hasNewTemplate = await checkIfUserHasFeatureController(userId, "organizer-request-email-v2");
   return hasNewTemplate ? ("OrganizerRequestEmailV2" as const) : ("OrganizerRequestEmail" as const);
 }
 
 export default class OrganizerRequestEmail extends OrganizerScheduledEmail {
   protected async getNodeMailerPayload(): Promise<Record<string, unknown>> {
     const toAddresses = [this.teamMember?.email || this.calEvent.organizer.email];
-    const template = await getOrganizerRequestTemplate({
-      userId: this.calEvent.organizer.id,
-      teamId: this.calEvent.team?.id,
-    });
+    const template = await getOrganizerRequestTemplate(this.calEvent.organizer.id);
 
     return {
       from: `${EMAIL_FROM_NAME} <${this.getMailerOptions().from}>`,

packages/features/flags/features.repository.interface.ts

@@ -3,6 +3,4 @@ import type { AppFlags } from "./config";
 export interface IFeaturesRepository {
   checkIfFeatureIsEnabledGlobally(slug: keyof AppFlags): Promise<boolean>;
   checkIfUserHasFeature(userId: number, slug: string): Promise<boolean>;
-  checkIfTeamHasFeature(teamId: number, slug: string): Promise<boolean>;
-  checkIfTeamOrUserHasFeature(args: { teamId?: number; userId?: number }, slug: string): Promise<boolean>;
 }

packages/features/flags/features.repository.mock.ts

@@ -0,0 +1,10 @@
+import type { IFeaturesRepository } from "./features.repository.interface";
+
+export class MockFeaturesRepository implements IFeaturesRepository {
+  async checkIfUserHasFeature(userId: number, slug: string) {
+    return slug === "mock-feature";
+  }
+  async checkIfFeatureIsEnabledGlobally() {
+    return true;
+  }
+}

packages/features/flags/features.repository.ts

@@ -15,51 +15,50 @@ export class FeaturesRepository implements IFeaturesRepository {
       throw err;
     }
   }
-  async checkIfTeamHasFeature(teamId: number, slug: string) {
+  async checkIfUserHasFeature(userId: number, slug: string) {
     try {
-      const teamFeature = await db.teamFeatures.findUnique({
+      /**
+       * findUnique was failing in prismock tests, so I'm using findFirst instead
+       * FIXME refactor when upgrading prismock
+       * https://github.com/morintd/prismock/issues/592
+       */
+      const userHasFeature = await db.userFeatures.findFirst({
         where: {
-          teamId_featureId: { teamId, featureId: slug },
+          userId,
+          featureId: slug,
         },
       });
-      return !!teamFeature;
+      if (userHasFeature) return true;
+      // If the user doesn't have the feature, check if they belong to a team with the feature.
+      // This also covers organizations, which are teams.
+      const userBelongsToTeamWithFeature = await this.checkIfUserBelongsToTeamWithFeature(userId, slug);
+      if (userBelongsToTeamWithFeature) return true;
+      return false;
     } catch (err) {
       captureException(err);
       throw err;
     }
   }
-
-  async checkIfUserHasFeature(userId: number, slug: string) {
+  private async checkIfUserBelongsToTeamWithFeature(userId: number, slug: string) {
     try {
-      const userFeature = await db.userFeatures.findUnique({
+      const user = await db.user.findUnique({
         where: {
-          userId_featureId: { userId, featureId: slug },
+          id: userId,
+          teams: {
+            some: {
+              team: {
+                features: {
+                  some: {
+                    featureId: slug,
+                  },
+                },
+              },
+            },
+          },
         },
+        select: { id: true },
       });
-      return !!userFeature;
-    } catch (err) {
-      captureException(err);
-      throw err;
-    }
-  }
-
-  async checkIfTeamOrUserHasFeature(
-    args: {
-      teamId?: number;
-      userId?: number;
-    },
-    slug: string
-  ) {
-    const { teamId, userId } = args;
-    try {
-      if (teamId) {
-        const teamHasFeature = await this.checkIfTeamHasFeature(teamId, slug);
-        if (teamHasFeature) return true;
-      }
-      if (userId) {
-        const userHasFeature = await this.checkIfUserHasFeature(userId, slug);
-        if (userHasFeature) return true;
-      }
+      if (user) return true;
       return false;
     } catch (err) {
       captureException(err);

packages/features/flags/operations/check-if-user-has-feature.controller.test.ts

@@ -0,0 +1,23 @@
+import prismock from "../../../../tests/libs/__mocks__/prisma";
+
+import { expect, it } from "vitest";
+
+import { checkIfUserHasFeatureController } from "./check-if-user-has-feature.controller";
+
+/**
+ * Since our current controller doesn't run any authentication checks or input validation,
+ * this test is identical to the test in the use case.
+ */
+it("checks if user has access to feature", async () => {
+  const userId = 1;
+  await prismock.userFeatures.create({
+    data: {
+      userId,
+      featureId: "mock-feature",
+      assignedBy: "1",
+      updatedAt: new Date(),
+    },
+  });
+  await expect(checkIfUserHasFeatureController(userId, "nonexistent-feature")).resolves.toBe(false);
+  await expect(checkIfUserHasFeatureController(userId, "mock-feature")).resolves.toBe(true);
+});

packages/features/flags/operations/check-if-user-has-feature.controller.ts

@@ -0,0 +1,32 @@
+import { startSpan } from "@sentry/nextjs";
+
+import { checkIfUserHasFeatureUseCase } from "./check-if-user-has-feature.use-case";
+
+/**
+ * Controllers use Presenters to convert the data to a UI-friendly format just before
+ * returning it to the "consumer". This helps us ship less JavaScript to the client (logic
+ * and libraries to convert the data), helps prevent leaking any sensitive properties, like
+ * emails or hashed passwords, and also helps us slim down the amount of data we're sending
+ * back to the client.
+ */
+function presenter(userHasFeature: boolean) {
+  return startSpan({ name: "checkIfUserHasFeature Presenter", op: "serialize" }, () => {
+    return userHasFeature;
+  });
+}
+
+/**
+ * Controllers perform authentication checks and input validation before passing the input
+ * to the specific use cases. Controllers orchestrate Use Cases. They don't implement any
+ * logic, but define the whole operations using use cases.
+ */
+export async function checkIfUserHasFeatureController(
+  userId: number | undefined,
+  slug: string
+): Promise<ReturnType<typeof presenter>> {
+  return await startSpan({ name: "checkIfUserHasFeature Controller" }, async () => {
+    if (!userId) throw new Error("Missing userId in checkIfUserHasFeatureController");
+    const userHasFeature = await checkIfUserHasFeatureUseCase(userId, slug);
+    return presenter(userHasFeature);
+  });
+}

packages/features/flags/operations/check-if-user-has-feature.ts

@@ -0,0 +1,2 @@
+/** We export the controller since it's the main entry point for this operation in the app */
+export { checkIfUserHasFeatureController } from "./check-if-user-has-feature.controller";

packages/features/flags/operations/check-if-user-has-feature.use-case.test.ts

@@ -0,0 +1,21 @@
+import prismock from "../../../../tests/libs/__mocks__/prisma";
+
+import { expect, it } from "vitest";
+
+import { checkIfUserHasFeatureUseCase } from "./check-if-user-has-feature.use-case";
+
+// This is identical to the test in the controller since the controller currently
+// doesn't run any authentication checks or input validation.
+it("returns if user has access to feature", async () => {
+  const userId = 1;
+  await prismock.userFeatures.create({
+    data: {
+      userId,
+      featureId: "mock-feature",
+      assignedBy: "1",
+      updatedAt: new Date(),
+    },
+  });
+  await expect(checkIfUserHasFeatureUseCase(userId, "nonexistent-feature")).resolves.toBe(false);
+  await expect(checkIfUserHasFeatureUseCase(userId, "mock-feature")).resolves.toBe(true);
+});

packages/features/flags/operations/check-if-user-has-feature.use-case.ts

@@ -0,0 +1,18 @@
+import { startSpan } from "@sentry/nextjs";
+
+import { FeaturesRepository } from "../features.repository";
+
+/**
+ * Use Cases represent individual operations, like "Create Feature" or "Sign In" or "Toggle Feature".
+ * Accept pre-validated input (from controllers) and handle authorization checks.
+ * Use Repositories and Services to access data sources and communicate with external systems.
+ * Use cases should not use other use cases. That's a code smell. It means the use case
+ * does multiple things and should be broken down into multiple use cases.
+ */
+export function checkIfUserHasFeatureUseCase(userId: number, slug: string): Promise<boolean> {
+  return startSpan({ name: "checkIfUserHasFeature UseCase", op: "function" }, async () => {
+    const featuresRepository = new FeaturesRepository();
+
+    return await featuresRepository.checkIfUserHasFeature(userId, slug);
+  });
+}

packages/prisma/migrations/20241119185538_add_index_for_user_and_team_features/migration.sql

@@ -0,0 +1,5 @@
+-- CreateIndex
+CREATE INDEX "TeamFeatures_teamId_featureId_idx" ON "TeamFeatures"("teamId", "featureId");
+
+-- CreateIndex
+CREATE INDEX "UserFeatures_userId_featureId_idx" ON "UserFeatures"("userId", "featureId");