🔁 fix: Recover Concurrent First SAML, LDAP and Social Logins - #16779
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
codegraph-librechat
Bot
added
🗺️ Backend Infra
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8532f3bbf5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const result = await createUserOnce({ | ||
| lookup, | ||
| strategyName: `${provider}Login`, | ||
| create: () => createUserIfAbsent(update, balanceConfig), | ||
| }); |
There was a problem hiding this comment.
This adds the social provisioning contract—calling the creation service, translating its failure, and deciding whether to refresh an existing user—directly to the legacy CJS layer. The same pattern is also added to the LDAP and SAML strategies, but /api is restricted to Express wiring; move this orchestration into a TypeScript auth service and leave these strategy files to pass dependencies and invoke it.
AGENTS.md reference: AGENTS.md:L77-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bb50ce2. The create / recover / refresh decision and its failure translation now live in TypeScript: provisionSamlUser (packages/api/src/auth/saml.ts), provisionLdapUser (ldap.ts) and provisionSocialUser (social.ts), all on top of createUserOnce. The strategies pass their models and services and keep only the existing mapping of a result to passport's done/cb. createSocialUser is now a single call that hands provisionSocialUser its services: handleExistingUser for a recovered account, and the new-user avatar step, moved unchanged into finishNewSocialUser.
Tests: provision.spec.ts covers the three provisioners against a real MongoDB (stale-lookup recovery, refresh with the losing login's values, other-provider conflict, first-user ADMIN for LDAP). With each provisioner's refresh of a recovered account removed, exactly its refresh test fails in provision.spec.ts and in its strategy spec. With createUserOnce's recovery removed, 13 packages/api tests and all 9 strategy race tests fail.
| export type SocialUserLookup = { | ||
| findUser: UserMethods['findUser']; | ||
| provider: string; |
There was a problem hiding this comment.
The exported SocialUserLookup contract exposes UserMethods['findUser'], whose argument is FilterQuery<IUser>, thereby widening the existing Mongoose API leak into this new auth module; the new LDAP and SAML helpers repeat the same dependency. Define a plain lookup interface or put the concrete query behind a data-schemas method so callers of packages/api do not depend on the storage engine.
AGENTS.md reference: AGENTS.md:L83-L88
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bb50ce2 and 286b582. The new lookups and provisioners take FindUserByFields ((fields: Record<string, string | undefined>) => Promise<UserRecord | null>) and CreateUserIfAbsent, both plain function types in provision.ts, instead of UserMethods['findUser']; countUsers and claimSamlIdentity are plain function types as well. They return UserRecord (the plain stored user from #16778) rather than the IUser document type. data-schemas' findUser still satisfies FindUserByFields, which tsc checks through provision.spec.ts, where the real methods are passed in. No UserMethods or FilterQuery remains in saml.ts, ldap.ts, social.ts or provision.ts.
| const created = await create(); | ||
| if (created.ok) return { user: created.value, error: null, created: true }; | ||
|
|
||
| const resolution = await lookup(); |
There was a problem hiding this comment.
When two first logins race with balances enabled, this retry can return the inserted user while the winning createUserIfAbsent is still awaiting creditStartBalance (packages/data-schemas/src/methods/user.ts inserts at line 366 but credits at line 374). The recovered LDAP or social request can then reach setBalanceConfig, create the still-missing balance with startBalance, and later have the winner's $inc add startBalance again, so the concurrency path can double-credit the account; coordinate completion or make balance initialization idempotent before returning the recovered user.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and it applied to #16778's OpenID recovery too, so it is fixed there: a62f59a, then 18f5c30 after the next round there. creditStartBalance writes tokenCredits with $set (and only user with $setOnInsert). When login balance sync initializes the record first, the creating request's configured start balance replaces it rather than being added on top. If the creator's write lands first, sync finds the record and leaves it. This branch is rebased onto 18f5c30.
Test (#16778, user.methods.spec.ts): a balance record with 300 already exists for the new user's id; createUserIfAbsent with startBalance: 500 leaves exactly one record with 500. On the original $inc it would be 800, and with an insert-only credit 300; both variants fail the test.
8532f3b to
286b582
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
31f54eb to
f2ba7f7
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
f2ba7f7 to
b64695b
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
b64695b to
cc21cbd
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
cc21cbd to
7d0694e
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d0694e551
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const accountConfig = resolution.user.tenantId | ||
| ? await resolveAppConfigForUser(getAppConfig, resolution.user) | ||
| : appConfig; |
There was a problem hiding this comment.
When a tenanted concurrent login loses the insert race and the base config enables a start balance, this waits for the tenant config lookup before findBalanceByUser at line 110 can begin, even though both reads are independent once resolution.user is known. On an uncached config this adds a full database round trip to the successful SAML, LDAP, social, and OpenID recovery path; start the config and conditional balance reads together, then await their results before applying the checks.
AGENTS.md reference: AGENTS.md:L186-L188
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4ddf500. With the barrier now reading the creation config (#16778's 336434f), the balance read no longer depends on the tenant config, so createUserOnce starts both with Promise.all: the tenant config resolution (for a tenant account) and the balance read (when the creation config sets a start balance). The checks then run in the same order as before: email-domain policy first, then the provisioning barrier.
Test (provision.spec.ts, real MongoDB in a tenant context): with a slow getAppConfig, the balance read starts before the tenant config resolves. With the reads made serial again, the test fails. #16778's own createOpenIDUser has the same two reads serially; this PR replaces that body with createUserOnce, so the OpenID path gets the parallel version here.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
SAML, LDAP and social logins looked the user up and then inserted it with nothing in between, so two first logins for one new identity both inserted and the unique index failed the second with E11000. Every strategy now provisions first-login users in packages/api: createUserOnce inserts through createUserIfAbsent and, on user_exists, repeats the strategy's own lookup and resolves to the account that won, once its creator finished provisioning it (its start balance exists when one is configured), admitted exactly as a found account (a tenant account resolves its tenant config and that config's email-domain policy applies). provisionSamlUser, provisionLdapUser and provisionSocialUser own the create / recover / refresh decision, so a recovered account gets the same refresh as any account the login finds (claimSamlIdentity, the LDAP directory overwrite, handleExistingUser), and the CJS strategies only pass their models and services and map the result to passport. The lookups take a plain FindUserByFields and the contracts carry UserRecord and NewUserData. createOpenIDUser now goes through createUserOnce. Related to #16766
After losing the insert race, createUserOnce resolved the tenant config and only then read the start balance, although the balance check now depends only on the creation config. Both reads start together.
4ddf500 to
eaa1d8d
Compare
1 participant
Pull Request
Summary
SAML, LDAP and social (Google, GitHub, Discord, Facebook, Apple) logins have the same race #16778 fixes for OpenID. Each one looks the user up, and when there is no account, inserts one with nothing in between. Two first logins for the same new identity both miss the lookup and both insert; the unique email or provider-identity index rejects the second, and that login fails with
E11000 duplicate key erroreven though the account now exists.Every strategy now inserts first-login users through
createUserOnceinpackages/api, on top of #16778'screateUserIfAbsent. When the insert reportsuser_exists, the request repeats the strategy's own lookup, with its provider and identity checks, and continues as the account the other request created once that request finished provisioning it (when the config new users are created under sets a start balance, the balance must exist; #16778 explains why). That account is admitted exactly as if the first lookup had found it (a tenant account resolves its tenant config and that config's email-domain policy applies), and gets the same refresh any account found at login gets: SAML claims the NameID binding and updates the profile throughclaimSamlIdentity, LDAP overwrites provider,ldapId, email, username and name from the directory, and social runshandleExistingUser(avatar and email). If the lookup rejects the account (another provider took the email, or a different NameID holds it), the login fails with the strategy's normalAUTH_FAILEDresponse; if the account's policy rejects the email, with its normalEmail domain not allowedresponse. If the lookup finds nothing that explains the conflict, the login fails with an error, as an unexpected duplicate did before.Stacked on #16778 (
danny-avila/librechat-issue-16766-a94100); merge that first.Related to #16766
How it works
Each strategy's lookup and the whole create / recover / refresh decision live in
packages/api. The CJS strategies pass their models and services and map the result to passport'sdone/cbas before:provisionLdapUseroverwrites provider,ldapId, email, username and name from the directory for found and recovered accounts alike, and creates the deployment's first user as ADMIN.provisionSocialUserhands a new account tofinishNewUser(the existing avatar step, moved unchanged intofinishNewSocialUserinprocess.js) and a recovered one torefreshExistingUser(handleExistingUser), and throws the codedAUTH_FAILEDthatsocialLoginpasses to its callback. After a lost race,createUserOncereads the recovered account's tenant config and its start balance in parallel. All contracts are plain: lookups takeFindUserByFieldsrather thanUserMethods['findUser'], and accounts areUserRecordrather than theIUserdocument type.Type of change
Testing
Tested environments/configuration:
createUserOnce; strategy callbacks with mocked modelsAutomated tests:
packages/api/src/auth/provision.spec.ts(real MongoDB).createUserOnce: two concurrent first logins create one account and resolve to it (createdtrue for one, false for the other); a lookup that rejects the account returns its error; an account found before its winner credited the start balance returnsAUTH_FAILED, and one whose balance exists recovers; inside a tenant context, a recovered tenant account whose config disallows the email returnsEmail domain not allowed, one whose tenant config alone sets a start balance continues (login balance sync initializes it insert-only), and one that admits it continues under the tenant's config; a conflict the lookup cannot explain throws; a validation failure throws without repeating the lookup.provisionSamlUserclaims the account a concurrent login created with the losing login's username and name, and fails on an other-provider email.provisionLdapUsercreates the first user as ADMIN and carries the losing login's directory values onto the recovered account.provisionSocialUserrefreshes a recovered account instead of finishing it as new, and throws the codedAUTH_FAILEDon an other-provider email.api/strategies/samlStrategy.spec.js›concurrent first login: the losing callback claims the winner's account with its own username and name throughclaimSamlIdentity; a concurrent other-provider account fails withAUTH_FAILEDand nothing is written.api/strategies/ldapStrategy.spec.js›concurrent first login: the losing login updates the winner's account with its LDAP values and keeps the stored role; a recovered account owned by another provider fails withAUTH_FAILED.samlStrategy.spec.js,ldapStrategy.spec.js,process.test.js: a recovered tenant account whose config allows only another domain fails with each strategy'sEmail domain not allowedresponse.api/strategies/process.test.js›createSocialUser › concurrent first login: the recovered account goes throughhandleExistingUser(a changed email is written) and is returned without new-user processing; a rejected lookup throws the codedAUTH_FAILEDthatsocialLoginpasses to the callback.api/strategies/socialLogin.test.js:createSocialUserreceives the same lookup parameters the login's first search used.createUserOncefailing onuser_existsinstead of recovering, 13 tests inpackages/apiand all 9 strategy race tests fail. With each provisioner's refresh of a recovered account removed, exactly the SAML, LDAP and social refresh tests fail in bothprovision.spec.tsand the strategy specs. With the provisioning check removed fromcreateUserOnce, the missing-balance tests inprovision.spec.ts,openid.spec.tsandopenidStrategy.spec.jsfail. With recovery keeping the base config, the 4 tenant-policy tests inprovision.spec.tsandopenid.spec.tsand the SAML, LDAP andcreateSocialUsertenant tests fail.openid.spec.ts,saml.spec.ts,provision.spec.ts,remoteAgentAuth.spec.ts,web/agent.spec.ts,domain.spec.ts(435 passed);AuthController.spec.js,appleStrategy.test.js,ldapStrategy.spec.js,openIdJwtStrategy.spec.js,openidStrategy.spec.js,process.test.js,samlStrategy.spec.js,socialLogin.test.js(389 passed).Screenshots / recordings
No user-facing change.
Risk / compatibility
Low. The normal path still does one lookup and one insert per first login; recovery only runs on
user_exists. The moved lookups keep their queries, checks and log lines. LDAP now creates throughcreateUserIfAbsent, which returns the created record rather than an id, so the follow-upupdateUserreceives the full record, as it already does for every returning LDAP user. Start balances are credited once, with the creating request's value, through #16778'screditStartBalance.samlStrategy.spec.jsnow mocks only the sixfs/pathfunctions it stubs (existsSync,statSync,readFileSync,isAbsolute,join,normalize, each defaulting to the real implementation) instead of automocking both modules, because loading the real@librechat/apihelpers under fully mockedfsandpathfails inside the AWS SDK.Same shape, not changed here:
ensurePrincipalExists(packages/api/src/acl/principals.ts) andPermissionServicecreate placeholder users for Entra ID principals during sharing with a lookup-then-insert. That is not a login path.Checklist