fix: setUserId lets users override the previous userId (#7035)

packages/js-core/src/lib/user/tests/user.test.ts

@@ -1,7 +1,7 @@
-import { type Mock, type MockInstance, beforeEach, describe, expect, test, vi } from "vitest";
+import { type MockInstance, beforeEach, describe, expect, test, vi } from "vitest";
 import { Config } from "@/lib/common/config";
 import { Logger } from "@/lib/common/logger";
-import { setup, tearDown } from "@/lib/common/setup";
+import { tearDown } from "@/lib/common/setup";
 import { UpdateQueue } from "@/lib/user/update-queue";
 import { logout, setUserId } from "@/lib/user/user";
 
@@ -34,13 +34,10 @@ vi.mock("@/lib/user/update-queue", () => ({
 
 vi.mock("@/lib/common/setup", () => ({
   tearDown: vi.fn(),
-  setup: vi.fn(),
 }));
 
 describe("user.ts", () => {
   const mockUserId = "test-user-123";
-  const mockEnvironmentId = "env-123";
-  const mockAppUrl = "https://test.com";
 
   let getInstanceConfigMock: MockInstance<() => Config>;
   let getInstanceLoggerMock: MockInstance<() => Logger>;
@@ -54,7 +51,40 @@ describe("user.ts", () => {
   });
 
   describe("setUserId", () => {
-    test("returns error if userId is already set", async () => {
+    test("returns ok without updating when same userId is already set", async () => {
+      const mockConfig = {
+        get: vi.fn().mockReturnValue({
+          user: {
+            data: {
+              userId: mockUserId,
+            },
+          },
+        }),
+      };
+
+      const mockLogger = {
+        debug: vi.fn(),
+        error: vi.fn(),
+      };
+
+      const mockUpdateQueue = {
+        updateUserId: vi.fn(),
+        processUpdates: vi.fn(),
+      };
+
+      getInstanceConfigMock.mockReturnValue(mockConfig as unknown as Config);
+      getInstanceLoggerMock.mockReturnValue(mockLogger as unknown as Logger);
+      getInstanceUpdateQueueMock.mockReturnValue(mockUpdateQueue as unknown as UpdateQueue);
+
+      const result = await setUserId(mockUserId);
+
+      expect(result.ok).toBe(true);
+      expect(mockLogger.debug).toHaveBeenCalledWith("UserId is already set to the same value, skipping");
+      expect(mockUpdateQueue.updateUserId).not.toHaveBeenCalled();
+      expect(mockUpdateQueue.processUpdates).not.toHaveBeenCalled();
+    });
+
+    test("tears down previous state and sets new userId when different userId is set", async () => {
       const mockConfig = {
         get: vi.fn().mockReturnValue({
           user: {
@@ -70,17 +100,24 @@ describe("user.ts", () => {
         error: vi.fn(),
       };
 
+      const mockUpdateQueue = {
+        updateUserId: vi.fn(),
+        processUpdates: vi.fn(),
+      };
+
       getInstanceConfigMock.mockReturnValue(mockConfig as unknown as Config);
       getInstanceLoggerMock.mockReturnValue(mockLogger as unknown as Logger);
+      getInstanceUpdateQueueMock.mockReturnValue(mockUpdateQueue as unknown as UpdateQueue);
 
       const result = await setUserId(mockUserId);
 
-      expect(result.ok).toBe(false);
-      if (!result.ok) {
-        expect(result.error.code).toBe("forbidden");
-        expect(result.error.status).toBe(403);
-      }
-      expect(mockLogger.error).toHaveBeenCalled();
+      expect(result.ok).toBe(true);
+      expect(mockLogger.debug).toHaveBeenCalledWith(
+        "Different userId is being set, cleaning up previous user state"
+      );
+      expect(tearDown).toHaveBeenCalled();
+      expect(mockUpdateQueue.updateUserId).toHaveBeenCalledWith(mockUserId);
+      expect(mockUpdateQueue.processUpdates).toHaveBeenCalled();
     });
 
     test("successfully sets userId when none exists", async () => {
@@ -110,44 +147,41 @@ describe("user.ts", () => {
       const result = await setUserId(mockUserId);
 
       expect(result.ok).toBe(true);
+      expect(tearDown).not.toHaveBeenCalled();
       expect(mockUpdateQueue.updateUserId).toHaveBeenCalledWith(mockUserId);
       expect(mockUpdateQueue.processUpdates).toHaveBeenCalled();
     });
   });
 
   describe("logout", () => {
-    test("successfully sets up formbricks after logout", () => {
-      const mockConfig = {
-        get: vi.fn().mockReturnValue({
-          environmentId: mockEnvironmentId,
-          appUrl: mockAppUrl,
-          user: { data: { userId: mockUserId } },
-        }),
+    test("successfully logs out and cleans state when userId is set", () => {
+      const mockLogger = {
+        debug: vi.fn(),
+        error: vi.fn(),
       };
 
-      getInstanceConfigMock.mockReturnValue(mockConfig as unknown as Config);
-
-      (setup as Mock).mockResolvedValue(undefined);
+      getInstanceLoggerMock.mockReturnValue(mockLogger as unknown as Logger);
 
       const result = logout();
 
+      expect(mockLogger.debug).toHaveBeenCalledWith("Logging out and cleaning user state");
       expect(tearDown).toHaveBeenCalled();
       expect(result.ok).toBe(true);
     });
 
-    test("returns error if appConfig.get fails", () => {
-      const mockConfig = {
-        get: vi.fn().mockReturnValue(null),
+    test("successfully logs out and cleans state even when no userId is set", () => {
+      const mockLogger = {
+        debug: vi.fn(),
+        error: vi.fn(),
       };
 
-      getInstanceConfigMock.mockReturnValue(mockConfig as unknown as Config);
+      getInstanceLoggerMock.mockReturnValue(mockLogger as unknown as Logger);
 
       const result = logout();
 
-      expect(result.ok).toBe(false);
-      if (!result.ok) {
-        expect(result.error).toEqual(new Error("Failed to logout"));
-      }
+      expect(mockLogger.debug).toHaveBeenCalledWith("Logging out and cleaning user state");
+      expect(tearDown).toHaveBeenCalled();
+      expect(result.ok).toBe(true);
     });
   });
 });

packages/js-core/src/lib/user/user.ts

@@ -2,7 +2,7 @@ import { Config } from "@/lib/common/config";
 import { Logger } from "@/lib/common/logger";
 import { tearDown } from "@/lib/common/setup";
 import { UpdateQueue } from "@/lib/user/update-queue";
-import { type ApiErrorResponse, type Result, err, okVoid } from "@/types/error";
+import { type ApiErrorResponse, type Result, okVoid } from "@/types/error";
 
 // eslint-disable-next-line @typescript-eslint/require-await -- we want to use promises here
 export const setUserId = async (userId: string): Promise<Result<void, ApiErrorResponse>> => {
@@ -14,16 +14,16 @@ export const setUserId = async (userId: string): Promise<Result<void, ApiErrorRe
     data: { userId: currentUserId },
   } = appConfig.get().user;
 
+  // If the same userId is already set, no-op
+  if (currentUserId === userId) {
+    logger.debug("UserId is already set to the same value, skipping");
+    return okVoid();
+  }
+
+  // If a different userId is set, clean up the previous user state first
   if (currentUserId) {
-    logger.error(
-      "A userId is already set in formbricks, please first call the logout function and then set a new userId"
-    );
-    return err({
-      code: "forbidden",
-      message: "User already set",
-      responseMessage: "User already set",
-      status: 403,
-    });
+    logger.debug("Different userId is being set, cleaning up previous user state");
+    tearDown();
   }
 
   updateQueue.updateUserId(userId);
@@ -34,15 +34,8 @@ export const setUserId = async (userId: string): Promise<Result<void, ApiErrorRe
 export const logout = (): Result<void> => {
   try {
     const logger = Logger.getInstance();
-    const appConfig = Config.getInstance();
-
-    const { userId } = appConfig.get().user.data;
-
-    if (!userId) {
-      logger.error("No userId is set, please use the setUserId function to set a userId first");
-      return okVoid();
-    }
 
+    logger.debug("Logging out and cleaning user state");
     tearDown();
 
     return okVoid();