diff --git a/.changeset/clerk-auth-signup-race-fix.md b/.changeset/clerk-auth-signup-race-fix.md new file mode 100644 index 0000000000..d9b39a41b9 --- /dev/null +++ b/.changeset/clerk-auth-signup-race-fix.md @@ -0,0 +1,6 @@ +--- +"jazz-tools": patch +--- + +Bugfix: fixed a race condition in Clerk auth where the signup flow could trigger a duplicate login attempt + diff --git a/CHANGELOG.md b/CHANGELOG.md index 20d2f9ba8d..c29d22c172 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ Released Jazz 0.19.16: - Improved sync timeout error messages to include known state, peer state, and any error information when waiting for sync times out +- Bugfix: fixed a race condition in Clerk auth where the signup flow could trigger a duplicate login attempt Released Jazz 0.19.15: - Added a locking system for session IDs in React Native to make mounting multiple JazzProviders safer (still not advised as duplicate the data loading effort) diff --git a/packages/jazz-tools/src/tools/auth/clerk/index.ts b/packages/jazz-tools/src/tools/auth/clerk/index.ts index 7808c68cd3..95a722bcb0 100644 --- a/packages/jazz-tools/src/tools/auth/clerk/index.ts +++ b/packages/jazz-tools/src/tools/auth/clerk/index.ts @@ -53,18 +53,21 @@ export class JazzClerkAuth { } private isFirstCall = true; + private previousUser: Pick< + NonNullable, + "unsafeMetadata" + > | null = null; registerListener(clerkClient: MinimalClerkClient) { - let previousUser: MinimalClerkClient["user"] | null = - clerkClient.user ?? null; + this.previousUser = clerkClient.user ?? null; // Need to use addListener because the clerk user object is not updated when the user logs in return clerkClient.addListener((event) => { const user = (event as Pick).user ?? null; - if (!isClerkAuthStateEqual(previousUser, user) || this.isFirstCall) { + if (!isClerkAuthStateEqual(this.previousUser, user) || this.isFirstCall) { + this.previousUser = user; this.onClerkUserChange({ user }); - previousUser = user; this.isFirstCall = false; } }); @@ -137,13 +140,20 @@ export class JazzClerkAuth { ? Array.from(credentials.secretSeed) : undefined; - await clerkClient.user?.update({ - unsafeMetadata: { - jazzAccountID: credentials.accountID, - jazzAccountSecret: credentials.accountSecret, - jazzAccountSeed, - } satisfies ClerkCredentials, - }); + const clerkCredentials = { + jazzAccountID: credentials.accountID, + jazzAccountSecret: credentials.accountSecret, + jazzAccountSeed, + }; + // user.update will cause the Clerk user change listener to fire; updating this.previousUser beforehand + // ensures the listener sees the new credentials and does not trigger an unnecessary logIn operation + this.previousUser = { unsafeMetadata: clerkCredentials }; + + if (clerkClient.user) { + await clerkClient.user.update({ + unsafeMetadata: clerkCredentials, + }); + } const currentAccount = await Account.getMe().$jazz.ensureLoaded({ resolve: { diff --git a/packages/jazz-tools/src/tools/auth/clerk/tests/JazzClerkAuth.test.ts b/packages/jazz-tools/src/tools/auth/clerk/tests/JazzClerkAuth.test.ts index 7e1812bb7d..a26bc03dc0 100644 --- a/packages/jazz-tools/src/tools/auth/clerk/tests/JazzClerkAuth.test.ts +++ b/packages/jazz-tools/src/tools/auth/clerk/tests/JazzClerkAuth.test.ts @@ -282,6 +282,83 @@ describe("JazzClerkAuth", () => { expect(onClerkUserChangeSpy).toHaveBeenCalledTimes(1); }); + + it("should complete signup flow when new Clerk user is detected", async () => { + // 1. Setup local credentials (simulating anonymous user) + await authSecretStorage.set({ + accountID: "test-account-id" as ID, + secretSeed: new Uint8Array([1, 2, 3]), + accountSecret: "test-secret" as AgentSecret, + provider: "anonymous", + }); + + const { client, triggerUserChange } = setupMockClerk(null); + + const auth = new JazzClerkAuth( + mockAuthenticate, + mockLogOut, + authSecretStorage, + ); + + // 2. Register listener with null user (no one logged in yet) + auth.registerListener(client); + + // Initial trigger with no user + triggerUserChange(null); + + // 3. Trigger event with new Clerk user (no Jazz credentials yet) + const mockUserUpdate = vi.fn((data) => { + triggerUserChange(data); + }); + + const signInSpy = vi.spyOn(auth, "signIn"); + const logInSpy = vi.spyOn(auth, "logIn"); + + const newClerkUser = { + fullName: "Test User", + firstName: "Test", + lastName: "User", + username: "testuser", + id: "clerk-user-123", + primaryEmailAddress: { emailAddress: "test@example.com" }, + unsafeMetadata: {}, // No Jazz credentials yet + update: mockUserUpdate, + }; + + triggerUserChange(newClerkUser); + + // Wait for async operations to complete + await vi.waitFor(() => { + expect(mockUserUpdate).toHaveBeenCalled(); + }); + + // 4. Verify credentials synced to Clerk + expect(mockUserUpdate).toHaveBeenCalledWith({ + unsafeMetadata: { + jazzAccountID: "test-account-id", + jazzAccountSecret: "test-secret", + jazzAccountSeed: [1, 2, 3], + }, + }); + + // Verify profile name was updated from Clerk username + const me = await Account.getMe().$jazz.ensureLoaded({ + resolve: { profile: true }, + }); + expect(me.profile.name).toBe("Test User"); + + // Verify authSecretStorage is updated with provider "clerk" + const storedCredentials = await authSecretStorage.get(); + expect(storedCredentials).toEqual({ + accountID: "test-account-id", + accountSecret: "test-secret", + secretSeed: new Uint8Array([1, 2, 3]), + provider: "clerk", + }); + + expect(signInSpy).toHaveBeenCalled(); + expect(logInSpy).not.toHaveBeenCalled(); + }); }); describe("initializeAuth", () => { diff --git a/packages/jazz-tools/src/tools/auth/clerk/tests/isClerkAuthStateEqual.test.ts b/packages/jazz-tools/src/tools/auth/clerk/tests/isClerkAuthStateEqual.test.ts new file mode 100644 index 0000000000..56a42ad148 --- /dev/null +++ b/packages/jazz-tools/src/tools/auth/clerk/tests/isClerkAuthStateEqual.test.ts @@ -0,0 +1,124 @@ +import { describe, expect, it } from "vitest"; +import { isClerkAuthStateEqual } from "../types"; + +describe("isClerkAuthStateEqual", () => { + const validCredentials = { + jazzAccountID: "account-123", + jazzAccountSecret: "secret-123", + jazzAccountSeed: [1, 2, 3], + }; + + const differentCredentials = { + jazzAccountID: "account-456", + jazzAccountSecret: "secret-456", + jazzAccountSeed: [4, 5, 6], + }; + + describe("both users null/undefined", () => { + it.each([ + { previous: null, next: null, description: "both null" }, + { previous: undefined, next: undefined, description: "both undefined" }, + { previous: null, next: undefined, description: "null and undefined" }, + { previous: undefined, next: null, description: "undefined and null" }, + ])("returns true when $description", ({ previous, next }) => { + expect(isClerkAuthStateEqual(previous, next)).toBe(true); + }); + }); + + describe("one user null, other exists", () => { + it.each([ + { + previous: null, + next: { unsafeMetadata: validCredentials }, + description: "previous null, next exists", + }, + { + previous: { unsafeMetadata: validCredentials }, + next: null, + description: "previous exists, next null", + }, + { + previous: undefined, + next: { unsafeMetadata: validCredentials }, + description: "previous undefined, next exists", + }, + { + previous: { unsafeMetadata: validCredentials }, + next: undefined, + description: "previous exists, next undefined", + }, + ])("returns false when $description", ({ previous, next }) => { + expect(isClerkAuthStateEqual(previous, next)).toBe(false); + }); + }); + + describe("same jazzAccountID", () => { + it("returns true when both users have the same jazzAccountID", () => { + const previous = { unsafeMetadata: validCredentials }; + const next = { + unsafeMetadata: { + ...validCredentials, + jazzAccountSecret: "different-secret", + }, + }; + expect(isClerkAuthStateEqual(previous, next)).toBe(true); + }); + }); + + describe("different jazzAccountID", () => { + it("returns false when users have different jazzAccountID", () => { + const previous = { unsafeMetadata: validCredentials }; + const next = { unsafeMetadata: differentCredentials }; + expect(isClerkAuthStateEqual(previous, next)).toBe(false); + }); + }); + + describe("neither user has valid credentials", () => { + it.each([ + { + previous: { unsafeMetadata: {} }, + next: { unsafeMetadata: {} }, + description: "both have empty metadata", + }, + { + previous: { unsafeMetadata: { someOtherField: "value" } }, + next: { unsafeMetadata: { anotherField: "value" } }, + description: "both have non-credential metadata", + }, + { + previous: { unsafeMetadata: { jazzAccountID: "123" } }, + next: { unsafeMetadata: { jazzAccountSecret: "456" } }, + description: "both have incomplete credentials", + }, + ])("returns true when $description", ({ previous, next }) => { + expect(isClerkAuthStateEqual(previous, next)).toBe(true); + }); + }); + + describe("one has credentials, other doesn't", () => { + it.each([ + { + previous: { unsafeMetadata: validCredentials }, + next: { unsafeMetadata: {} }, + description: "previous has credentials, next empty", + }, + { + previous: { unsafeMetadata: {} }, + next: { unsafeMetadata: validCredentials }, + description: "previous empty, next has credentials", + }, + { + previous: { unsafeMetadata: validCredentials }, + next: { unsafeMetadata: { jazzAccountID: "123" } }, + description: "previous has credentials, next has incomplete", + }, + { + previous: { unsafeMetadata: { jazzAccountSecret: "456" } }, + next: { unsafeMetadata: validCredentials }, + description: "previous has incomplete, next has credentials", + }, + ])("returns false when $description", ({ previous, next }) => { + expect(isClerkAuthStateEqual(previous, next)).toBe(false); + }); + }); +}); diff --git a/packages/jazz-tools/src/tools/auth/clerk/types.ts b/packages/jazz-tools/src/tools/auth/clerk/types.ts index 84ea2b87c6..ecbe5910b3 100644 --- a/packages/jazz-tools/src/tools/auth/clerk/types.ts +++ b/packages/jazz-tools/src/tools/auth/clerk/types.ts @@ -36,21 +36,38 @@ export type ClerkCredentials = { * **Note**: It does not validate the credentials, only checks if the necessary fields are present in the metadata object. */ export function isClerkCredentials( - data: NonNullable["unsafeMetadata"] | undefined, + data: + | NonNullable["unsafeMetadata"] + | null + | undefined, ): data is ClerkCredentials { return !!data && "jazzAccountID" in data && "jazzAccountSecret" in data; } export function isClerkAuthStateEqual( - previousUser: MinimalClerkClient["user"] | null | undefined, - newUser: MinimalClerkClient["user"] | null | undefined, + previousUser: + | Pick, "unsafeMetadata"> + | null + | undefined, + newUser: + | Pick, "unsafeMetadata"> + | null + | undefined, ) { if (Boolean(previousUser) !== Boolean(newUser)) { return false; } - const previousCredentials = isClerkCredentials(previousUser?.unsafeMetadata); - const newCredentials = isClerkCredentials(newUser?.unsafeMetadata); + const previousCredentials = isClerkCredentials(previousUser?.unsafeMetadata) + ? previousUser?.unsafeMetadata + : null; + const newCredentials = isClerkCredentials(newUser?.unsafeMetadata) + ? newUser?.unsafeMetadata + : null; + + if (!previousCredentials || !newCredentials) { + return previousCredentials === newCredentials; + } - return previousCredentials === newCredentials; + return previousCredentials.jazzAccountID === newCredentials.jazzAccountID; }