✨ server: support factory selection in credential creation - #1089
Conversation
🦋 Changeset detectedLatest commit: c4a808f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis PR adds optional ChangesFactory Selection Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
There was a problem hiding this comment.
Code Review
This pull request introduces support for factory selection during credential creation and authentication. It adds validation for the factory query parameter, updates credential creation logic to handle the selected factory, and includes a utility to derive valid factories. The review feedback highlights a potential issue where legacy credentials with a null or undefined factory field in the database could cause a validation error when parsed. To prevent this, the reviewer suggests falling back to exaAccountFactoryAddress and importing it in authentication.ts.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
View the full list of 1 ❄️ flaky test(s)
|
dce468d to
c4a808f
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/utils/createCredential.ts (1)
27-45: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winValidate
factoryat the persistence boundary.
createCredentialnow derives the account fromoptions.factoryand stores that value incredentials.factory, but the whitelist check only exists inserver/api/auth/authentication.tsandserver/api/auth/registration.ts. Any other caller of this utility can bypass the whitelist and persist an unsupported factory. Guard it here too, and keep the route-level checks only as early request validation.♻️ Proposed fix
import { exaAccountFactoryAddress } from "`@exactly/common/generated/chain`"; import { Address } from "`@exactly/common/validation`"; import { updateWebhookAddresses } from "./alchemy"; import authSecret from "./authSecret"; import decodePublicKey from "./decodePublicKey"; import { customer } from "./sardine"; import { identify } from "./segment"; +import validFactories from "./validFactories"; import database from "../database"; import { credentials } from "../database/schema"; import { webhookId } from "../hooks/activity"; @@ ) { const factory = options?.factory ?? exaAccountFactoryAddress; + if (!validFactories.has(factory)) throw new Error("bad factory"); const publicKey = options?.webauthn?.publicKey ?? (isAddress(credentialId) ? new Uint8Array(hexToBytes(credentialId)) : undefined);As per coding guidelines, "extract and abstract only with reuse (two or more uses) or foot-gun encapsulation."
Source: Coding guidelines
server/test/api/auth.test.ts (1)
463-658: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRegistration's new factory path still has no coverage.
server/api/auth/registration.tsnow parses, whitelists, and persistsquery.factory, but the registration suite never sends afactoryquery on either the success or rejection path. Add one non-default factory registration case and one invalid-factory case that assert the storedcredentials.factory, so auth and registration stay aligned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4ccc70bd-8079-4cf3-99b8-c0ce8f5c6663
📒 Files selected for processing (6)
.changeset/brave-foxes-beam.mdserver/api/auth/authentication.tsserver/api/auth/registration.tsserver/test/api/auth.test.tsserver/utils/createCredential.tsserver/utils/validFactories.ts
Summary by CodeRabbit
New Features
Tests