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

2024-11-19 15:21:38 -05:00
parent 97b920744e
commit 16573717b4
11 changed files with 150 additions and 47 deletions
@@ -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}>`,
@@ -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>;
 }
@@ -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;
+  }
+}
@@ -15,51 +15,50 @@ export class FeaturesRepository implements IFeaturesRepository {
      throw err;
    }
  }
-  async checkIfTeamHasFeature(teamId: number, slug: string) {
-    try {
-      const teamFeature = await db.teamFeatures.findUnique({
-        where: {
-          teamId_featureId: { teamId, featureId: slug },
-        },
-      });
-      return !!teamFeature;
-    } catch (err) {
-      captureException(err);
-      throw err;
-    }
-  }
-
  async checkIfUserHasFeature(userId: number, slug: string) {
    try {
-      const userFeature = await db.userFeatures.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: {
-          userId_featureId: { userId, featureId: slug },
+          userId,
+          featureId: slug,
        },
      });
-      return !!userFeature;
+      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 checkIfTeamOrUserHasFeature(
-    args: {
-      teamId?: number;
-      userId?: number;
-    },
-    slug: string
-  ) {
-    const { teamId, userId } = args;
+  private async checkIfUserBelongsToTeamWithFeature(userId: number, slug: string) {
    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;
-      }
+      const user = await db.user.findUnique({
+        where: {
+          id: userId,
+          teams: {
+            some: {
+              team: {
+                features: {
+                  some: {
+                    featureId: slug,
+                  },
+                },
+              },
+            },
+          },
+        },
+        select: { id: true },
+      });
+      if (user) return true;
      return false;
    } catch (err) {
      captureException(err);
@@ -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);
+});
@@ -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);
+  });
+}
@@ -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";
@@ -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);
+});
@@ -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);
+  });
+}
@@ -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");
@@ -1236,6 +1236,7 @@ model UserFeatures {
  updatedAt  DateTime @updatedAt

  @@id([userId, featureId])
+  @@index([userId, featureId])
 }

 model TeamFeatures {
@@ -1248,6 +1249,7 @@ model TeamFeatures {
  updatedAt  DateTime @updatedAt

  @@id([teamId, featureId])
+  @@index([teamId, featureId])
 }

 enum FeatureType {