refactor(core): add dedicated signInCredentials strategy function - #245
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughCredential sign-in now uses dedicated stateful and stateless session strategy handlers. Stateful sign-in validates users, creates sessions, and logs progress. The API records structured error metadata. Tests now use existing users and verify session resolution. ChangesCredential Sign-In
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SignInAPI
participant SessionStrategy
participant StatefulSignInCredentials
participant UserLookup
participant SessionStore
SignInAPI->>SessionStrategy: signInCredentials(payload, request)
SessionStrategy->>StatefulSignInCredentials: validate credentials
StatefulSignInCredentials->>UserLookup: find user by sub
UserLookup-->>StatefulSignInCredentials: return user entity
StatefulSignInCredentials->>SessionStore: create device and credentials session
SessionStore-->>SignInAPI: return hashed 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/api/signInCredentials.ts (1)
74-107: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not log arbitrary credential authorization error messages.
credentials.authorizecan throw an error that includes submitted credentials or provider response data. Lines 95 and 106 persisterror_messageto the configured logger. Keeperror_typeand the stable error code, but remove the raw exception message from authentication logs.Proposed fix
- const error_message = error instanceof Error ? error.message : String(error) const headers = new Headers(secureApiHeaders) ... - error_message, }, ... - error_message, },🤖 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/api/signInCredentials.ts` around lines 74 - 107, Remove the raw error_message field from both authentication logger structuredData payloads in the invalid-credentials and failed sign-in branches. Preserve logging of error_type and the stable error_code, and leave the response behavior unchanged.
🧹 Nitpick comments (1)
packages/core/test/actions/signIn/signInCredentials/stateful.test.ts (1)
23-28: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse identifier-sensitive user lookup mocks.
Both core suites return the same user for every lookup key. This can allow an incorrect
subor an unverified existing-user path to pass.
packages/core/test/actions/signIn/signInCredentials/stateful.test.ts#L23-L28: MakegetUserByIdMockreturnuserEntityonly for"user-123"and assert that argument.packages/core/test/actions/signIn/signInCredentials/stateful.test.ts#L186-L191: Apply the same conditional lookup and argument assertion.packages/core/test/actions/signIn/signInCredentials/stateful.test.ts#L245-L250: Apply the same conditional lookup and argument assertion.packages/core/test/actions/signIn/signInCredentials/stateful.test.ts#L304-L309: Apply the same conditional lookup and argument assertion.packages/core/test/actions/signIn/signInCredentials/stateful.test.ts#L363-L368: Apply the same conditional lookup and argument assertion.packages/core/test/api/stateful/signInCredentials.test.ts#L23-L28: MakegetUserByIdMockreturnuserEntityonly for"user-123"and assert that argument.packages/core/test/api/stateful/signInCredentials.test.ts#L232-L237: Apply the same conditional lookup and argument assertion.packages/core/test/api/stateful/signInCredentials.test.ts#L293-L298: Apply the same conditional lookup and argument assertion.packages/core/test/api/stateful/signInCredentials.test.ts#L354-L359: Apply the same conditional lookup and argument assertion.packages/core/test/api/stateful/signInCredentials.test.ts#L415-L420: Apply the same conditional lookup and argument assertion.🤖 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/signIn/signInCredentials/stateful.test.ts` around lines 23 - 28, Update each getUserByIdMock setup in packages/core/test/actions/signIn/signInCredentials/stateful.test.ts at lines 23-28, 186-191, 245-250, 304-309, and 363-368, and packages/core/test/api/stateful/signInCredentials.test.ts at lines 23-28, 232-237, 293-298, 354-359, and 415-420, so the mock returns userEntity only when called with "user-123" and otherwise does not match; add assertions verifying getUserByIdMock received the expected "user-123" identifier in every affected test.
🤖 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/session/stateful/signInCredentials.ts`:
- Around line 43-94: Update the return value at the end of the sign-in flow to
return the raw secret token generated as secretValue, while continuing to
persist tokenHash in createSession. Keep the existing hashing, logging, and
session creation behavior unchanged so cookie-based lookup can resolve the
stored hash.
---
Outside diff comments:
In `@packages/core/src/api/signInCredentials.ts`:
- Around line 74-107: Remove the raw error_message field from both
authentication logger structuredData payloads in the invalid-credentials and
failed sign-in branches. Preserve logging of error_type and the stable
error_code, and leave the response behavior unchanged.
---
Nitpick comments:
In `@packages/core/test/actions/signIn/signInCredentials/stateful.test.ts`:
- Around line 23-28: Update each getUserByIdMock setup in
packages/core/test/actions/signIn/signInCredentials/stateful.test.ts at lines
23-28, 186-191, 245-250, 304-309, and 363-368, and
packages/core/test/api/stateful/signInCredentials.test.ts at lines 23-28,
232-237, 293-298, 354-359, and 415-420, so the mock returns userEntity only when
called with "user-123" and otherwise does not match; add assertions verifying
getUserByIdMock received the expected "user-123" identifier in every affected
test.
🪄 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: 881da655-38f9-4471-a0ef-79cddf4b7a78
📒 Files selected for processing (10)
packages/core/src/@types/session.tspackages/core/src/api/signInCredentials.tspackages/core/src/session/stateful/index.tspackages/core/src/session/stateful/signInCredentials.tspackages/core/src/session/stateless/index.tspackages/core/src/session/stateless/signInCredentials.tspackages/core/src/shared/logger.tspackages/core/test/actions/signIn/signInCredentials/stateful.test.tspackages/core/test/api/stateful/signInCredentials.test.tspackages/elysia/test/stateful/index.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This pull request refactors the credentials sign-in flow by introducing a dedicated
signInCredentialsstrategy for both the Stateless (JWT) and Stateful (Database) session strategies.Previously, the credentials sign-in flow relied on the shared
createSessionimplementation. While this approach was sufficient for creating authenticated sessions, it coupled credentials authentication with generic session creation logic, making it difficult to introduce validations and behaviors specific to credentials-based authentication.By separating the credentials flow into its own strategy, the authentication process now has full control over the sign-in lifecycle, enabling cleaner implementations and making it easier to introduce additional features and validations in the future.
Key Changes
signInCredentialsstrategy for the Stateless (JWT) session strategy.signInCredentialsstrategy for the Stateful (Database) session strategy.createSessionimplementation.Warning
In the Stateful session strategy, the authenticated user is identified by the
subvalue returned from thecredentials.authorize()callback. Within this callback,subis treated as the unique identifier of the user and is used to retrieve the corresponding database record.The remaining fields returned by
authorize()(such asname,email,image, or custom identity fields) are not persisted or synchronized with the database. They are only used during the authentication process.This behavior differs from the Stateless strategy, where the returned identity is stored directly in the session token.
Usage
@coderabbitai ignore