refactor(core): add dedicated signUp strategy function - #244
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThe PR adds ChangesSession strategy sign-up
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SignUpAPI as signUp API
participant SessionStrategy as SessionStrategy.signUp
participant StatefulHandler as stateful signUp handler
participant SessionStore as database session store
SignUpAPI->>SessionStrategy: pass payload and request
SessionStrategy->>StatefulHandler: create stateful session token
StatefulHandler->>SessionStore: persist user, account, device, and session
SessionStore-->>StatefulHandler: persist records
StatefulHandler-->>SignUpAPI: return session token
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 8
🧹 Nitpick comments (3)
packages/core/src/shared/errors.ts (1)
931-938: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider the account-enumeration tradeoff of a distinct 409 response.
The sign-up endpoint now returns a unique code and message when an email already exists. An unauthenticated caller can use this to test which emails are registered. If enumeration resistance matters for this library, return a generic success response and send a "you already have an account" email instead, or gate the distinct response behind a configuration flag.
The current behavior may be intentional. Document the tradeoff if you keep it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/shared/errors.ts` around lines 931 - 938, Review the EMAIL_ALREADY_REGISTERED error behavior for account-enumeration exposure. Either replace the distinct unauthenticated 409 response with the library’s generic success flow and an existing-account email, or gate it behind a configuration flag; if retaining the current behavior, document the intentional security tradeoff near the error definition or signup flow.packages/core/test/api/stateful/signUp.test.ts (1)
393-406: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated mock setup and assertions into helpers.
Six redirect scenarios repeat the same six mock definitions and the same
createUserandcreateAccountassertion pair: Lines 393-406 and 431-449, 473-485 and 511-529, 553-566 and 591-609, 633-645 and 671-689, 715-730 and 753-771, plus the base case at Lines 31-43 and 66-84.When the handler changes its write set, every block needs the same edit. Extract two helpers, for example
createSignUpAdapterMocks()andexpectSignUpPersistence(mocks), and keep only the scenario-specific parts inline.The stale mock at Line 726 shows the cost of the duplication.
getUserByIdis registered there, but the handler in packages/core/src/session/stateful/signUp.ts never calls it. Remove it during the extraction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/test/api/stateful/signUp.test.ts` around lines 393 - 406, The repeated sign-up mock setup and persistence assertions in the test scenarios should be centralized. Add helpers such as createSignUpAdapterMocks() and expectSignUpPersistence(mocks), update the base and redirect scenarios to reuse them while keeping only scenario-specific setup inline, and remove the unused getUserById mock from the affected scenario.packages/core/src/session/stateful/signUp.ts (1)
100-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract stateful session 15-day expiry from one source of truth.
signUp.ts,createSession.ts, andcookie.tseach hardcode60 * 60 * 24 * 15. Stateful DB sessions and the session cookie can diverge if only one expression changes, and the log keeps reading its own magic15. Use one configured/default value for the DB expiry, cookie max age, and loggedmax_age_days.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/session/stateful/signUp.ts` around lines 100 - 106, Update the stateful session expiry flow around the sign-up expiration calculation, createSession, and cookie handling to use one shared configured/default 15-day value instead of hardcoded expressions. Reuse that same source for the database expiration, cookie max age, and the logger’s max_age_days field, preserving the existing expiration behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/`@types/session.ts:
- Around line 280-284: Update createStatelessStrategy so its returned
SessionStrategy implements the required signUp method, using an appropriate
stateless/no-op behavior consistent with unsupported signup. Keep the
SessionStrategy contract and existing stateful implementations unchanged.
In `@packages/core/src/api/signUp.ts`:
- Around line 72-77: Update the SIGN_UP_ERROR logging in signUp to stop
including the raw adapter error message; replace error_message with the stable
error code, using AuraAuthError’s public code field when available and an
appropriate stable fallback for other errors. Preserve the existing error_type
logging and do not add free-form adapter messages to structuredData.
In `@packages/core/src/session/stateful/signUp.ts`:
- Around line 26-30: Remove the direct console.log debug statements in the
sign-up flow, including the payload log near the payload destructuring and the
password log around the later sign-up logic. Do not replace them with logging;
preserve the existing payload handling and schema-mismatch `@todo`.
- Around line 41-46: Normalize the submitted email once by trimming and
lowercasing it, then reuse that value for both getUserByEmail and createUser in
the sign-up flow. Require adapters to enforce a unique email constraint, and
catch the resulting createUser constraint violation to throw AuraAuthError with
code EMAIL_ALREADY_REGISTERED, preserving the API’s 409 response for concurrent
duplicates.
- Around line 48-118: Make the writes in signUp atomic across createUser,
createAccount, optional createCredentialAccount, createDevice, and createSession
by executing them within a database transaction. Extend DatabaseAdapter with the
required transaction primitive and use it through sessionConfig.adapter,
ensuring any failure rolls back all previously created records and preserves the
existing successful flow.
- Around line 30-31: Update the sign-up flow around the password extraction and
credential account creation branch to preserve and forward the raw payload’s
password independently of the user object returned by onCreateUser. Ensure
createCredentialAccount receives the submitted password whenever one is
provided, including with the default onCreateUser, while retaining validated
identity fields for user creation.
- Around line 77-83: Update the password handling in the sign-up flow around
hashPassword and createCredentialAccount to require typeof password === "string"
rather than casting it. When a truthy non-string password is provided, reject
the request with the existing validation-error mechanism and an appropriate
invalid sign-up password error code; continue hashing valid string passwords
unchanged.
In `@packages/core/test/api/stateful/signUp.test.ts`:
- Around line 367-370: Strengthen the assertion in the sign-up test around
createCredentialsAccountMock so passwordHash matches the pbkdf2-sha256 format
and differs from the submitted plaintext password, rather than only being any
string. After resolving the payload contract issue in the sign-up handler, add a
case using the default onCreateUser behavior instead of relying solely on the
payload passthrough override.
---
Nitpick comments:
In `@packages/core/src/session/stateful/signUp.ts`:
- Around line 100-106: Update the stateful session expiry flow around the
sign-up expiration calculation, createSession, and cookie handling to use one
shared configured/default 15-day value instead of hardcoded expressions. Reuse
that same source for the database expiration, cookie max age, and the logger’s
max_age_days field, preserving the existing expiration behavior.
In `@packages/core/src/shared/errors.ts`:
- Around line 931-938: Review the EMAIL_ALREADY_REGISTERED error behavior for
account-enumeration exposure. Either replace the distinct unauthenticated 409
response with the library’s generic success flow and an existing-account email,
or gate it behind a configuration flag; if retaining the current behavior,
document the intentional security tradeoff near the error definition or signup
flow.
In `@packages/core/test/api/stateful/signUp.test.ts`:
- Around line 393-406: The repeated sign-up mock setup and persistence
assertions in the test scenarios should be centralized. Add helpers such as
createSignUpAdapterMocks() and expectSignUpPersistence(mocks), update the base
and redirect scenarios to reuse them while keeping only scenario-specific setup
inline, and remove the unused getUserById mock from the affected scenario.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4436a033-28d6-46f4-9556-04a9d79deddc
📒 Files selected for processing (8)
packages/core/src/@types/session.tspackages/core/src/api/signUp.tspackages/core/src/session/stateful/index.tspackages/core/src/session/stateful/signUp.tspackages/core/src/session/strategy.tspackages/core/src/shared/errors.tspackages/core/src/shared/logger.tspackages/core/test/api/stateful/signUp.test.ts
💤 Files with no reviewable changes (1)
- packages/core/src/session/strategy.ts
halvaradop
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/test/actions/signUp/stateful.test.ts (2)
70-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten
createAccountMockuserIdassertions to match the created user's id.Every
createAccountMockassertion in this file checksuserId: expect.any(String), while the parallelcreateSessionMockassertion (for example line 340-342) checks the literaluserId: "user-123". SincecreateUserMockalways resolves to the fixeduserEntitywithid: "user-123"(perpackages/core/test/presets.ts:86-98), the account-creation call site can assert the same literal value. As written,expect.any(String)would still pass even if the implementation wired the wrong user id into the created account, so the test does not verify that the account is actually linked to the newly created user.🔧 Proposed tightening (applies to each occurrence)
expect(createAccountMock).toHaveBeenCalledWith({ id: expect.any(String), - userId: expect.any(String), + userId: "user-123", provider: "credentials", providerUserId: expect.any(String), type: "credentials", status: "active", })Also applies to: 320-342, 416-440, 486-505, 554-573, 622-641, 690-709, 758-777
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/test/actions/signUp/stateful.test.ts` around lines 70 - 89, Update every createAccountMock assertion in the stateful sign-up tests to require userId: "user-123" instead of expect.any(String), matching the fixed userEntity returned by createUserMock. Apply this consistently to the occurrences near the initial assertion and the additional createAccountMock checks, while leaving unrelated fields unchanged.
32-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated sign-up mock setup into a shared helper.
The same block (
getUserByEmailMock,createUserMock,createAccountMock,createDeviceMock,createSessionMock,getDeviceByFingerprintMock) repeats near-verbatim across at least nine test cases. Extract a factory function, for examplecreateSignUpMocks(overrides), that returns the mock set with sensible defaults. Each test then overrides only what it needs (for examplegetUserByEmailMockreturninguserEntityfor the duplicate-email case). This reduces duplication and lowers the risk that a future contract change (for example a new field onaccountEntity) gets updated in some call sites but missed in others.♻️ Example helper
const createSignUpMocks = (overrides: Partial<Record<string, ReturnType<typeof vi.fn>>> = {}) => ({ getUserByEmailMock: vi.fn().mockReturnValue(null), createUserMock: vi.fn().mockReturnValue(userEntity), createAccountMock: vi.fn().mockReturnValue({ ...accountEntity, provider: "credentials" }), createDeviceMock: vi.fn().mockResolvedValue(deviceEntity), createSessionMock: vi.fn().mockReturnValue(sessionEntityWithUser), getDeviceByFingerprintMock: vi.fn().mockReturnValue(null), ...overrides, })Also applies to: 110-125, 264-278, 354-368, 452-465, 520-533, 588-601, 656-669, 724-737
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/test/actions/signUp/stateful.test.ts` around lines 32 - 47, Extract the repeated mock initialization in the sign-up tests into a shared createSignUpMocks factory with the current defaults for getUserByEmailMock, createUserMock, createAccountMock, createDeviceMock, createSessionMock, and getDeviceByFingerprintMock. Update each affected test to use the factory and pass only the mocks it overrides, preserving cases such as duplicate-email tests that return userEntity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/test/actions/signUp/stateful.test.ts`:
- Around line 70-89: Update every createAccountMock assertion in the stateful
sign-up tests to require userId: "user-123" instead of expect.any(String),
matching the fixed userEntity returned by createUserMock. Apply this
consistently to the occurrences near the initial assertion and the additional
createAccountMock checks, while leaving unrelated fields unchanged.
- Around line 32-47: Extract the repeated mock initialization in the sign-up
tests into a shared createSignUpMocks factory with the current defaults for
getUserByEmailMock, createUserMock, createAccountMock, createDeviceMock,
createSessionMock, and getDeviceByFingerprintMock. Update each affected test to
use the factory and pass only the mocks it overrides, preserving cases such as
duplicate-email tests that return userEntity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b37f1f16-2077-446a-8733-e40480106033
📒 Files selected for processing (7)
packages/core/src/api/signUp.tspackages/core/src/session/stateful/index.tspackages/core/src/session/stateful/signUp.tspackages/core/src/session/stateless/index.tspackages/core/src/session/stateless/signUp.tspackages/core/test/actions/signUp/stateful.test.tspackages/core/test/api/stateful/signUp.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core/src/session/stateful/index.ts
- packages/core/src/api/signUp.ts
- packages/core/src/session/stateful/signUp.ts
- packages/core/test/api/stateful/signUp.test.ts
1 participant
Description
This pull request refactors the sign-up flow by introducing a dedicated
signUpstrategy for both the Stateless (JWT) and Stateful (Database) session strategies.Previously, the sign-up flow relied on the shared
createSessionimplementation. While this approach worked for creating authenticated sessions, it coupled sign-up behavior with generic session creation logic. As a result, it was difficult to introduce validations and behaviors that are specific to account registration.By separating the sign-up flow into its own strategy, the authentication process now has full control over the sign-up lifecycle, making it easier to implement registration-specific validations and future features.
One immediate benefit of this refactor is the ability to validate whether an email address is already registered before creating a new account, preventing duplicate registrations and enabling more robust sign-up workflows.
Key Changes
signUpstrategy for the Stateless (JWT) session strategy.signUpstrategy for the Stateful (Database) session strategy.createSessionimplementation.Note
This PR is primarily an internal refactor. While it introduces support for email existence validation, its main purpose is to establish a dedicated sign-up strategy that enables future enhancements without affecting the generic session creation flow.
Related PRs