Pass authUserId to createOrUpdateUser for secure account linking#306
Pass authUserId to createOrUpdateUser for secure account linking#306rorhug wants to merge 5 commits into
Conversation
|
@rorhug is attempting to deploy a commit to the Convex Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdd tracking of the currently authenticated user ID ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as Server (auth handler)
participant DB as Database
participant Callback as createOrUpdateUser callback
Client->>Server: Start OAuth flow (existingSessionId present)
Server->>DB: get(existingSessionId)
DB-->>Server: session { userId: authUserId }
Server->>Callback: createOrUpdateUser({ existingUserId, authUserId, profile, ... })
Callback-->>Server: returns userId
Server->>DB: create/update auth_accounts (link to returned userId)
Server-->>Client: OAuth complete (tokens / redirect)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/pages/advanced.mdx (1)
81-108:⚠️ Potential issue | 🟡 MinorMissing
convexAuth()function call in example.The code example assigns an object literal directly instead of calling
convexAuth(). This will cause a runtime error if copied verbatim.📝 Proposed fix
-export const { auth, signIn, signOut, store, isAuthenticated } = { +export const { auth, signIn, signOut, store, isAuthenticated } = convexAuth({ providers: [GitHub, Password], callbacks: { // ... }, -}; +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/pages/advanced.mdx` around lines 81 - 108, The example incorrectly destructures from a plain object instead of calling convexAuth(), so replace the object literal assignment for export const { auth, signIn, signOut, store, isAuthenticated } = { ... } with a call to convexAuth(...) returning that config; specifically, keep the callbacks.createOrUpdateUser implementation (MutationCtx, args, findUserByEmail, etc.) but pass it into convexAuth() so the exported symbols (auth, signIn, signOut, store, isAuthenticated) come from convexAuth(...) rather than a raw object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@docs/pages/advanced.mdx`:
- Around line 81-108: The example incorrectly destructures from a plain object
instead of calling convexAuth(), so replace the object literal assignment for
export const { auth, signIn, signOut, store, isAuthenticated } = { ... } with a
call to convexAuth(...) returning that config; specifically, keep the
callbacks.createOrUpdateUser implementation (MutationCtx, args, findUserByEmail,
etc.) but pass it into convexAuth() so the exported symbols (auth, signIn,
signOut, store, isAuthenticated) come from convexAuth(...) rather than a raw
object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e23b6ef-5900-4d5d-a331-8ffb2f2ccf52
📒 Files selected for processing (3)
docs/pages/advanced.mdxsrc/server/implementation/users.tssrc/server/types.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/convex/createOrUpdateUser.test.ts (2)
95-103: Prevent cross-test leakage from global env mutation.
setupEnvoverwritesprocess.envand never restores prior values, which can make other tests order-dependent.Proposed fix
test("createOrUpdateUser links a new OAuth account to the signed-in user via authUserId", async () => { - setupEnv(); + const restoreEnv = setupEnv(); + try { const modules = import.meta.glob("./**/*.*s"); const overriddenModules = { ...modules, "./auth.ts": async () => createOrUpdateUserAuth, }; const t = convexTest(schema, overriddenModules); @@ await t.run(async (ctx) => { @@ }); + } finally { + restoreEnv(); + } }); @@ function setupEnv() { + const previous = { + SITE_URL: process.env.SITE_URL, + CONVEX_SITE_URL: process.env.CONVEX_SITE_URL, + JWT_PRIVATE_KEY: process.env.JWT_PRIVATE_KEY, + JWKS: process.env.JWKS, + AUTH_GITHUB_ID: process.env.AUTH_GITHUB_ID, + AUTH_GITHUB_SECRET: process.env.AUTH_GITHUB_SECRET, + AUTH_LOG_LEVEL: process.env.AUTH_LOG_LEVEL, + }; + process.env.SITE_URL = "http://localhost:5173"; process.env.CONVEX_SITE_URL = CONVEX_SITE_URL; process.env.JWT_PRIVATE_KEY = JWT_PRIVATE_KEY; process.env.JWKS = JWKS; process.env.AUTH_GITHUB_ID = "githubClientId"; process.env.AUTH_GITHUB_SECRET = "githubClientSecret"; process.env.AUTH_LOG_LEVEL = "ERROR"; + + return () => { + for (const [k, v] of Object.entries(previous)) { + if (v === undefined) delete process.env[k]; + else process.env[k] = v; + } + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/convex/createOrUpdateUser.test.ts` around lines 95 - 103, The setupEnv function mutates process.env without restoring original values causing cross-test leakage; update setupEnv (or the test harness around it) to capture the current values of each environment variable it touches (SITE_URL, CONVEX_SITE_URL, JWT_PRIVATE_KEY, JWKS, AUTH_GITHUB_ID, AUTH_GITHUB_SECRET, AUTH_LOG_LEVEL) before setting them and restore those saved originals after the test (e.g., in an afterEach/teardown) so the global environment is returned to its prior state, ensuring tests are order-independent.
46-93: Add test for unauthenticated OAuth with nullauthUserId.The current test covers the signed-in user linking path via
authUserId, but the callback also handles the unauthenticated OAuth flow whereauthUserIdisnull(line 36-40: creates new user without inserting a message). This case needs explicit test coverage—callsignInViaGitHubwithoutt.withIdentity()and verify that a new user is created without a message being inserted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/convex/createOrUpdateUser.test.ts` around lines 46 - 93, Add a new test that exercises the unauthenticated OAuth path where authUserId is null: reuse the convexTest setup (calls to setupEnv, import.meta.glob override of "./auth.ts" with createOrUpdateUserAuth, and convexTest(schema, overriddenModules)), call signInViaGitHub without wrapping the client in t.withIdentity (so no subject/claims), then inside t.run verify that a new user was created (ctx.db.query("users").collect() length increased) and confirm no message was inserted (ctx.db.query("messages").collect() returns empty), and that the new authAccounts entry for provider "github" has userId pointing to the newly created user; reference signInViaGitHub, t.withIdentity, createOrUpdateUserAuth, and authUserId to locate related logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/convex/createOrUpdateUser.test.ts`:
- Around line 95-103: The setupEnv function mutates process.env without
restoring original values causing cross-test leakage; update setupEnv (or the
test harness around it) to capture the current values of each environment
variable it touches (SITE_URL, CONVEX_SITE_URL, JWT_PRIVATE_KEY, JWKS,
AUTH_GITHUB_ID, AUTH_GITHUB_SECRET, AUTH_LOG_LEVEL) before setting them and
restore those saved originals after the test (e.g., in an afterEach/teardown) so
the global environment is returned to its prior state, ensuring tests are
order-independent.
- Around line 46-93: Add a new test that exercises the unauthenticated OAuth
path where authUserId is null: reuse the convexTest setup (calls to setupEnv,
import.meta.glob override of "./auth.ts" with createOrUpdateUserAuth, and
convexTest(schema, overriddenModules)), call signInViaGitHub without wrapping
the client in t.withIdentity (so no subject/claims), then inside t.run verify
that a new user was created (ctx.db.query("users").collect() length increased)
and confirm no message was inserted (ctx.db.query("messages").collect() returns
empty), and that the new authAccounts entry for provider "github" has userId
pointing to the newly created user; reference signInViaGitHub, t.withIdentity,
createOrUpdateUserAuth, and authUserId to locate related logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d046e41-a294-4673-8795-35923aca9393
📒 Files selected for processing (1)
test/convex/createOrUpdateUser.test.ts
|
Hey @erquhart, could you take a look at this? |
|
@rorhug thanks for this! It would definitely be good to better enable account linking, and the approach looks viable, but I would need to take some time ensuring its fully secure. Given the maintenance mode status of Convex Auth that's unlikely to happen immediately, but I want to leave this open if that's alright for future consideration. |
2 participants
Problem
In
createOrUpdateUserit's not currently possible to know who the currently logged in user is.Given an app with Github auth and Google auth, without customisation if you first log in with Github and then Google, the Google signin creates a second user and the session becomes that. Therefore we need custom account linking logic.
Current docs for account linking: https://labs.convex.dev/auth/advanced#controlling-user-creation-and-account-linking-behavior
However, although the docs recommend using
createOrUpdateUser(ctx, args)the ctx does not have the current session because it's being ran from a convex HTTP*.siteURL which doesn't have the cookies. Therefore the following code prints "my user id is null" while having clicked "connect Google" even though we are logged in already.Email based account linking is not good enough.
Solution
authUserIdto thecreateOrUpdateUsercallback when an OAuth flow is started while another user is already signed in.authUserIdfor OAuth account linking, fix a small typo in the callback docs.Why
Custom account-linking logic needs access to the currently authenticated user during signed-in OAuth flows. Without that,
createOrUpdateUsercallbacks can't reliably attach a new OAuth account to the existing logged in user who initiated the flow.authUserIdis distinct fromexistingUserIdexistingUserId: Id<"users"> | nullrefers to the user that may already be linked to the account being processedauthUserId: Id<"users"> | nullNEW refers to the user who may currently be logged in.Test plan
createOrUpdateUsercallback receivesauthUserIdduring an OAuth flow started while signed inauthUserIdisnullfor flows without an authenticated sessionBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Summary by CodeRabbit
New Features
Documentation
Tests
Chore