feat(auth): implement Secure Remote Password (SRP) authentication #386
Conversation
|
Warning Rate limit exceeded@HuanCheng65 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes integrate Secure Remote Password (SRP) authentication into the backend. This includes new database migrations, updates to the Prisma schema, and adjustments to several DTOs to accommodate SRP fields and logic. New service and controller methods are introduced to handle SRP initialization, verification, and password changes. Error handling is enhanced with new SRP-related errors, and the test suite is updated accordingly. Additionally, support for temporary token generation in TOTP is added, and module configuration is updated to inject the new service. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant UC as UsersController
participant US as UsersService
participant SS as SrpService
participant DB as Database
C->>UC: POST /srpInit { username, clientPublicEphemeral }
UC->>SS: createServerSession(username, salt, verifier, clientPublicEphemeral)
SS->>DB: Retrieve user & generate server ephemeral
SS-->>UC: { serverPublicEphemeral, serverSecretEphemeral }
UC-->>C: { salt, serverPublicEphemeral, serverSecretEphemeral }
C->>UC: POST /srpVerify { username, clientProof }
UC->>SS: verifyClient(..., clientProof)
SS->>DB: Validate client proof/session derivation
SS-->>UC: { serverProof, success }
UC-->>C: { accessToken, requires2FA, tempToken? }
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (14)
src/users/totp.service.ts (1)
291-314: LGTM! Well-implemented security measures.The implementation correctly generates a temporary token with:
- Properly scoped permissions limited to TOTP verification
- Time-limited access (5 minutes)
- User-specific resource binding
Consider these maintainability improvements:
- Add input validation for userId
- Extract magic numbers to constants
- Enhance JSDoc documentation
+ /** Time in milliseconds for token validity */ + private readonly TEMP_TOKEN_VALIDITY_MS = 5 * 60 * 1000; + private readonly TEMP_TOKEN_VALIDITY_SEC = 300; + /** * 生成临时令牌，用于 2FA 验证 + * @param userId - The ID of the user to generate the token for + * @returns A temporary JWT token valid for 5 minutes + * @throws {Error} If userId is invalid */ generateTempToken(userId: number): string { + if (!userId || userId <= 0) { + throw new Error('Invalid userId'); + } + return this.authService.sign( { userId: userId, permissions: [ { authorizedActions: ['verify'], authorizedResource: { ownedByUser: userId, types: ['users/totp:verify'], resourceIds: undefined, data: { - validUntil: Date.now() + 5 * 60 * 1000, + validUntil: Date.now() + this.TEMP_TOKEN_VALIDITY_MS, }, }, }, ], }, - 300, + this.TEMP_TOKEN_VALIDITY_SEC, ); }src/users/srp.service.ts (2)
15-33: Validate input prior to SRP derivation
Generating salt and verifier is correct. However, consider ensuring password complexity before deriving the verifier.
53-87: Error-handling strategy
Returning{ success: false, serverProof: '' }on error ensures the client recognizes verification failure. Logging the error details could help debugging.src/users/users.controller.ts (2)
163-275: Combined SRP and legacy registration
Mixing both flows in a single method can inflate complexity. Consider splitting logic or using separate methods for SRP vs legacy.
1196-1210: SRP-based change password
Straightforward method. Double-check that Sudo mode is verified beforehand to maintain security.test/user.e2e-spec.ts (1)
20-31: Commented-out SRP mock
Remove or refactor if not used to keep the test code base clean.-// // Mock SRP client -// jest.mock('secure-remote-password/client', () => ({ -// ... -// }));src/users/users.service.ts (1)
555-654: Improve SRP registration flow clarity.The registration flow now supports both SRP-based and legacy authentication. Consider constraints:
- Confirm environment gating is robust to avoid accidental production usage of legacy authentication.
- Log or warn more explicitly when legacy auth is chosen.
- Ensure unit tests fully cover the fallback logic.
These refinements could prevent misconfigurations and improve maintainability.
src/users/DTO/register.dto.ts (1)
25-33: Translate Chinese comments to English for better accessibility.The comments indicate that these fields are for testing purposes only, but they are written in Chinese. Consider translating them to English:
- // 可选的传统密码字段，仅用于测试 + // Optional legacy password field, for testing only @IsString() @IsOptional() password?: string; - // 是否使用传统认证方式，仅用于测试 + // Whether to use legacy authentication, for testing only @IsBoolean() @IsOptional() isLegacyAuth?: boolean;src/users/DTO/sudo.dto.ts (2)
3-16: Type thepasskeyResponsefield and translate Chinese comments.
The
passkeyResponsefield is typed asany, which bypasses TypeScript's type checking. Consider importing and using the appropriate type from@simplewebauthn/server.Comments are in Chinese. Consider translating them to English:
method: 'password' | 'srp' | 'passkey' | 'totp'; credentials: { - // 传统密码凭据 + // Legacy password credentials password?: string; - // SRP 凭据 + // SRP credentials clientPublicEphemeral?: string; clientProof?: string; - // Passkey 凭据 + // Passkey credentials - passkeyResponse?: any; + passkeyResponse?: AuthenticationResponseJSON; - // TOTP 凭据 + // TOTP credentials code?: string; };Import the type from
@simplewebauthn/server:+ import { AuthenticationResponseJSON } from '@simplewebauthn/server';
18-28: Translate Chinese comments to English for better accessibility.The comments explain when the fields are returned but are written in Chinese. Consider translating them to English:
data: { accessToken: string; - // 如果是 SRP 方式，需要返回这些字段用于完成握手 + // For SRP authentication, these fields are returned to complete the handshake salt?: string; serverPublicEphemeral?: string; serverProof?: string; - // 如果是传统密码验证且触发了 SRP 升级，返回此字段 + // Returned when legacy password verification triggers an SRP upgrade srpUpgraded?: boolean; };src/auth/definitions.ts (1)
76-76: Maintain consistency in comment language.The comment is in Chinese while all other comments in the file are in English. Consider translating it to English for consistency:
- username?: string; // 用户名，用于密码重置等场景 + username?: string; // Username, used for password reset and similar scenariossrc/users/srp.service.spec.ts (3)
11-27: Consider using more explicit mock cleanup.While the mocks are well-structured, consider moving them to a separate file for reusability and cleaner test organization.
+// src/users/__mocks__/srp-mocks.ts +export const mockSrpClient = { + generateSalt: jest.fn(() => 'test-salt'), + derivePrivateKey: jest.fn(() => 'test-private-key'), + deriveVerifier: jest.fn(() => 'test-verifier'), +}; + +export const mockSrpServer = { + generateEphemeral: jest.fn(() => ({ + secret: 'server-secret', + public: 'server-public', + })), + deriveSession: jest.fn(() => ({ + key: 'shared-key', + proof: 'server-proof', + })), +};
153-155: Improve type safety in test mocks.Using
as anyreduces type safety. Consider creating proper mock types:+interface MockUser { + id: number; + srpUpgraded: boolean; + srpSalt?: string; + srpVerifier?: string; +} + jest .spyOn(srpService['prismaService'].user, 'findUnique') - .mockResolvedValueOnce({ - srpUpgraded: true, - } as any); + .mockResolvedValueOnce({ + id: 1, + srpUpgraded: true, + } as MockUser);
48-70: Add edge cases to credential generation tests.The current test suite could benefit from additional test cases:
it('should handle empty username', async () => { await expect( srpService.generateSrpCredentials('', 'password') ).rejects.toThrow(); }); it('should handle empty password', async () => { await expect( srpService.generateSrpCredentials('username', '') ).rejects.toThrow(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
package.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yamlsrc/auth/token-payload.schema.jsonis excluded by!**/*.json
📒 Files selected for processing (20)
prisma/migrations/20250219084632_add_srp_fields/migration.sql(1 hunks)prisma/migrations/20250219101440_make_hashed_password_optional/migration.sql(1 hunks)prisma/migrations/20250219164036_add_last_password_changed_at/migration.sql(1 hunks)prisma/schema.prisma(1 hunks)src/auth/definitions.ts(1 hunks)src/users/DTO/change-password.dto.ts(1 hunks)src/users/DTO/passkey.dto.ts(1 hunks)src/users/DTO/register.dto.ts(2 hunks)src/users/DTO/reset-password.dto.ts(1 hunks)src/users/DTO/srp.dto.ts(1 hunks)src/users/DTO/sudo.dto.ts(1 hunks)src/users/srp.service.spec.ts(1 hunks)src/users/srp.service.ts(1 hunks)src/users/totp.service.ts(1 hunks)src/users/users.controller.ts(11 hunks)src/users/users.error.ts(1 hunks)src/users/users.module.ts(2 hunks)src/users/users.prisma(1 hunks)src/users/users.service.ts(13 hunks)test/user.e2e-spec.ts(16 hunks)
✅ Files skipped from review due to trivial changes (1)
- prisma/migrations/20250219101440_make_hashed_password_optional/migration.sql
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-push-image
🔇 Additional comments (55)
src/users/srp.service.ts (5)
1-4: Doc comment is concise and informative
Good job providing a clear file-level overview.
6-10: Imports are consistent and appropriate
Using separate imports for client-side and server-side SRP is well-structured.
11-13: Dependency injection
Constructor injection of PrismaService is straightforward and aligns with NestJS best practices.
35-51: Consider wrappinggenerateEphemeralin try/catch
If invalid verifier or other errors occur, an exception might propagate. A catch block or input validation can guard against edge cases.
89-98: Handle nonexistent user
Returningfalseif the user is missing or not upgraded is a sensible default behavior.src/users/users.controller.ts (12)
14-14: Use of forwardRef
This is acceptable for circular dependencies between modules.
17-17: @httpcode import
Makes the intended response code explicit. Good practice.
22-22: @patch usage
No issues with adding the HTTP PATCH decorator here.
49-52: DTO imports for change password
Cleanly adds new DTOs for password changes.
66-66: PasskeyAuthenticationOptionsRequestDto
Enables typed requests for passkey-based authentication flow.
87-91: SRP DTO imports
Imports for SRP init/verify requests and responses properly separate SRP concerns.
119-127: Extending session interface
Well-defined approach to store SRP ephemeral/exchange data in session.
425-433: Reset password with SRP
AcceptingsrpSaltandsrpVerifieris consistent. Make sure to notify users that old credentials are invalidated.
813-845: Sudo verification expansions
Supporting multiple verification strategies (password/TOTP/SRP) in one endpoint is convenient. Implementation looks solid.
951-952: Explicit 200 response
Using@HttpCode(200)clarifies successful 2FA disable calls.
1045-1145: SRP Init & Verify
Storing ephemeral values in session and removing them upon verification is a standard approach. Implementation looks good.
1147-1195: Retrieving auth methods
Good approach for flexible front-end flows. The fallback (no methods available) is logical for nonexistent users.test/user.e2e-spec.ts (13)
12-13: Updated imports for SRP testing
AddingotplibandsrpClientis appropriate for the new authentication flow.
62-118: loginWithSRP helper
Defines a solid end-to-end SRP login sequence. Encourages DRY test code.
119-168: verifySudoWithSRP helper
Well-structured, ensures ephemeral keys are generated and verified for Sudo mode.
172-217: createLegacyUser helper
Properly sets up a user with legacy auth, assisting SRP upgrade tests.
222-223: Centralized test credentials
Defining user credentials at the top is a clear approach for reuse and readability.
264-274: Validate invalid email
Good test coverage for malformed email addresses.
596-603: Ensuring SRP login success
This test verifies SRP login flow is working. Great coverage.
606-621: SrpVerificationError test
Probes an incorrect SRP proof scenario, confirming proper error response.
624-633: Unknown username
Checks that an undefined user triggers a 404.
635-652: Refresh token coverage
Ensures that clients can refresh tokens after SRP login.
654-660: Sudo mode test
Verifies SRP-based Sudo activation. Comprehensive approach.
722-739: Requesting password reset
Covers partial flows to confirm email sending is triggered.
742-792: SRP password reset
Ensures a user can reset their password with SRP, verifying new salt usage. Thorough coverage.src/users/users.service.ts (12)
52-52: Confirm usage of SRP service import.The new import for
SrpServiceis clear and self-explanatory. No immediate issues spotted.
72-73: Check newly introduced SRP errors for coverage.These SRP-related errors appear necessary for robust error handling. Ensure they are also tested thoroughly.
87-90: Verify SRP session lifetime.Storing ephemeral server values in the session helps persist the SRP state between steps; however, ensure these are cleared when not needed to reduce security risks. Consider setting an explicit expiration policy.
112-112: Inject SRP service successfully.The constructor injection for
SrpServiceis properly defined and consistent with the new SRP features.
238-265: Validate logic when userId is omitted.When
userIdis not provided, a passkey challenge with an empty credential allowance is generated. This is a convenient fallback but confirm it aligns with the intended authentication flow (e.g., kiosk mode).
741-761: Check auto-upgrade logic on login.The code auto-upgrades legacy users to SRP if they have not been upgraded yet. Verify that:
- Concurrent logins do not cause race conditions.
- Any partially updated records are handled gracefully on errors.
956-1032: Clear hashed password on reset.Shifting to SRP credentials and blanking out
hashedPasswordis a strong security measure. The logic appears coherent.
1306-1435: Review SRP sudo verification flow carefully.This method conditions on
'srp', storing ephemeral data in the session, and finally issues a sudo token.
- Confirm that the ephemeral session data is always cleared, even on error.
- Potentially implement a timeout to avoid indefinite ephemeral data.
1437-1469: SRP init flow appears consistent.The first step of SRP (server ephemeral generation) is straightforward and seems to align with standard SRP usage.
1470-1554: SRP verify flow is logically sound.This second SRP phase properly verifies the proof, logs the user in, and optionally checks 2FA. Implementation looks good.
1545-1553: Dedicated session creation for new users.Providing a separate method for first login session creation is clean. No issues noted.
1554-1572: Validate old password before changing SRP credentials.Although this method is presumably guarded by prior verification, consider requiring the user’s existing SRP or password proof before finalizing the change to reinforce security.
src/users/DTO/change-password.dto.ts (2)
4-10: Check SRP properties in request DTO.Both
srpSaltandsrpVerifierare required strings, aligning with SRP. Ensure size or complexity validations if necessary.
12-12: Extend from base response DTO.The empty
ChangePasswordResponseDtoinherits standard response fields. No issues found here.src/users/DTO/reset-password.dto.ts (1)
16-19: Confirm shift to SRP fields in reset flow.Replacing the direct password with
srpSaltandsrpVerifieris aligned with secure remote password practices. Looks consistent with other files.src/users/DTO/register.dto.ts (1)
11-18: LGTM! SRP fields are properly defined.The
srpSaltandsrpVerifierfields are correctly marked as optional and validated as strings, which is essential for implementing SRP authentication.src/users/DTO/srp.dto.ts (2)
13-18: LGTM! Response DTO is properly structured.The class correctly extends
BaseResponseDtoand includes the necessary fields for the server's SRP response.
25-33: LGTM! Response DTO is properly structured.The class correctly extends
BaseResponseDtoand includes all necessary fields for the server's verification response.src/users/users.module.ts (1)
18-18: LGTM! Clean integration of SRP service.The SRP service is properly imported and registered as a provider in the module.
Also applies to: 44-44
src/users/users.error.ts (1)
217-237: LGTM! Well-structured error classes.The new SRP-related error classes are well-implemented with:
- Appropriate HTTP status codes (401 for authentication failures, 422 for invalid input)
- Clear error messages
- Consistent inheritance from BaseError
prisma/migrations/20250219164036_add_last_password_changed_at/migration.sql (1)
1-3: Migration for "last_password_changed_at" Column is Correct.The migration script properly adds the new column with the correct TIMESTAMPTZ(6) type, a NOT NULL constraint, and the default value using CURRENT_TIMESTAMP. Please ensure that the corresponding Prisma model (in both
src/users/users.prismaandprisma/schema.prisma) remains in sync with this change.prisma/migrations/20250219084632_add_srp_fields/migration.sql (1)
1-5: SRP Fields Addition Looks Solid.The script adds three new columns—
srp_salt(VARCHAR(500)),srp_upgraded(BOOLEAN with default false), andsrp_verifier(VARCHAR(1000))—to the "user" table. The choices for data types and default value for the boolean field are appropriate. Verify in your application logic that any fields which are nullable (likesrp_saltandsrp_verifier) are handled correctly.src/users/users.prisma (2)
16-19: User Model: Authentication Fields Update.The
Usermodel modifications correctly change thehashedPasswordfield to be nullable and add the SRP-related fields (srpSalt,srpVerifier, andsrpUpgraded) with proper mapping and database type annotations. Ensure that your service and controller layers differentiate legacy authentication from SRP flows, especially considering the possibility of a nullhashedPassword.
24-24: Tracking Password Changes with "lastPasswordChangedAt".The addition of the
lastPasswordChangedAtfield with a default value ofnow()is well implemented. Confirm that when users update their passwords, the application logic explicitly sets or updates this field to reflect the change accurately.prisma/schema.prisma (1)
612-622: Prisma Schema User Model Consistency Check.In the Prisma schema’s
Usermodel, the updates are consistent with the migration scripts—thehashedPasswordfield is now optional, and the new fieldssrpSalt,srpVerifier,srpUpgraded, andlastPasswordChangedAthave been added with the correct default settings and database mappings. Double-check that any legacy authentication routines have been refactored to accommodate the nullablehashedPasswordfield and the new SRP properties.
| /** | ||
| * 为现有用户升级到 SRP | ||
| */ | ||
| async upgradeUserToSrp( | ||
| userId: number, | ||
| username: string, | ||
| password: string, | ||
| ): Promise<void> { | ||
| const { salt, verifier } = await this.generateSrpCredentials( | ||
| username, | ||
| password, | ||
| ); | ||
|
|
||
| await this.prismaService.user.update({ | ||
| where: { id: userId }, | ||
| data: { | ||
| srpSalt: salt, | ||
| srpVerifier: verifier, | ||
| srpUpgraded: true, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Transactional upgrade
Upgrading a user might benefit from a database transaction or concurrency checks to avoid partial updates under heavy load.
| export class SrpVerifyRequestDto { | ||
| username: string; | ||
| clientProof: string; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation decorators for required fields.
The fields are required but lack validation decorators. Consider adding @IsString() decorators:
export class SrpVerifyRequestDto {
+ @IsString()
username: string;
+ @IsString()
clientProof: string;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export class SrpVerifyRequestDto { | |
| username: string; | |
| clientProof: string; | |
| } | |
| export class SrpVerifyRequestDto { | |
| @IsString() | |
| username: string; | |
| @IsString() | |
| clientProof: string; | |
| } |
| export class SrpInitRequestDto { | ||
| username: string; | ||
| clientPublicEphemeral: string; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation decorators for required fields.
The fields are required but lack validation decorators. Consider adding @IsString() decorators:
export class SrpInitRequestDto {
+ @IsString()
username: string;
+ @IsString()
clientPublicEphemeral: string;
}Import the decorator:
+ import { IsString } from 'class-validator';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export class SrpInitRequestDto { | |
| username: string; | |
| clientPublicEphemeral: string; | |
| } | |
| import { IsString } from 'class-validator'; | |
| export class SrpInitRequestDto { | |
| @IsString() | |
| username: string; | |
| @IsString() | |
| clientPublicEphemeral: string; | |
| } |
| export class PasskeyAuthenticationOptionsRequestDto { | ||
| userId?: number; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation decorator for optional number field.
The userId field should be validated as an optional number:
export class PasskeyAuthenticationOptionsRequestDto {
+ @IsNumber()
+ @IsOptional()
userId?: number;
}Import the decorator:
+ import { IsNumber, IsOptional } from 'class-validator';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export class PasskeyAuthenticationOptionsRequestDto { | |
| userId?: number; | |
| } | |
| import { IsNumber, IsOptional } from 'class-validator'; | |
| export class PasskeyAuthenticationOptionsRequestDto { | |
| @IsNumber() | |
| @IsOptional() | |
| userId?: number; | |
| } |
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
sample.env (2)
76-80: Enhance Redis security configuration.Consider the following security recommendations:
- Uncomment and set
REDIS_USERNAMEandREDIS_PASSWORDfor production environments- Verify if "valkey" is the correct hostname or should be replaced with a more standard value like "localhost" for local development
81-83: Update WebAuthn configuration for production environments.The current WebAuthn configuration is suitable for local development but needs adjustment for production:
- Use HTTPS origin for production environments (WebAuthn requires secure context)
- Update
WEB_AUTHN_RP_IDto match your production domainConsider adding comments to distinguish between development and production values:
-WEB_AUTHN_RP_NAME=Cheese Community -WEB_AUTHN_RP_ID=localhost -WEB_AUTHN_ORIGIN=http://localhost:3000 +# Development values (update for production): +WEB_AUTHN_RP_NAME=Cheese Community +WEB_AUTHN_RP_ID=localhost # Use your domain in production (e.g., example.com) +WEB_AUTHN_ORIGIN=http://localhost:3000 # Use https:// in production
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sample.env(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: test (12, 8.0.0, 20.x, 8, 2)
- GitHub Check: test (12, 8.0.0, 20.x, 8, 1)
- GitHub Check: test (12, 8.0.0, 18.x, 8, 2)
- GitHub Check: test (12, 8.0.0, 18.x, 8, 1)
- GitHub Check: test (12, 8.12.2, 20.x, 8, 2)
- GitHub Check: test (12, 8.12.2, 20.x, 8, 1)
- GitHub Check: test (12, 8.12.2, 18.x, 8, 2)
- GitHub Check: test (12, 8.12.2, 18.x, 8, 1)
- GitHub Check: test (12, 8.12.2, 20.x, 8, 2)
- GitHub Check: test (latest, 8.0.0, 20.x, 8, 2)
- GitHub Check: test (latest, 8.0.0, 20.x, 8, 1)
- GitHub Check: test (latest, 8.0.0, 18.x, 8, 2)
- GitHub Check: test (latest, 8.0.0, 18.x, 8, 1)
- GitHub Check: test (latest, 8.0.0, 20.x, 8, 2)
- GitHub Check: test (latest, 8.12.2, 18.x, 8, 1)
- GitHub Check: test (latest, 8.12.2, 20.x, 8, 2)
- GitHub Check: build-and-push-image
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: test (latest, 8.12.2, 18.x, 8, 1)
| WEB_AUTHN_RP_NAME=Cheese Community | ||
| WEB_AUTHN_RP_ID=localhost | ||
| WEB_AUTHN_ORIGIN=http://localhost:3000 | ||
| TOTP_ENCRYPTION_KEY=sm2vSXU3SudEuBd2r6ewGiap1LbqGbjf |
There was a problem hiding this comment.
Remove hardcoded TOTP encryption key.
The hardcoded encryption key in the sample environment file poses a security risk if accidentally used in production.
Replace the hardcoded key with a placeholder and add instructions for generating a secure key:
-TOTP_ENCRYPTION_KEY=sm2vSXU3SudEuBd2r6ewGiap1LbqGbjf
+# Generate a secure 32-byte key for production using:
+# node -e "console.log(require('crypto').randomBytes(32).toString('base64'))"
+TOTP_ENCRYPTION_KEY=your_secure_random_key_here📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TOTP_ENCRYPTION_KEY=sm2vSXU3SudEuBd2r6ewGiap1LbqGbjf | |
| # Generate a secure 32-byte key for production using: | |
| # node -e "console.log(require('crypto').randomBytes(32).toString('base64'))" | |
| TOTP_ENCRYPTION_KEY=your_secure_random_key_here |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
test/user.follow.e2e-spec.ts (1)
42-48: 🛠️ Refactor suggestionAdd
isLegacyAuthflag to auxiliary user creation.The
createAuxiliaryUserfunction should also include theisLegacyAuthflag for consistency.Apply this diff:
.send({ username: `TestUser-${Math.floor(Math.random() * 10000000000)}`, nickname: 'auxiliary_user', password: 'abc123456!!!', email, emailCode: verificationCode, + isLegacyAuth: true, });test/materialbundle.e2e-spec.ts (1)
40-46: 🛠️ Refactor suggestionAdd
isLegacyAuthflag to auxiliary user creation.The
createAuxiliaryUserfunction should also include theisLegacyAuthflag for consistency.Apply this diff:
.send({ username: `TestUser-${Math.floor(Math.random() * 10000000000)}`, nickname: 'auxiliary_user', password: 'abc123456!!!', email, emailCode: verificationCode, + isLegacyAuth: true, });test/comment.e2e-spec.ts (1)
40-46: 🛠️ Refactor suggestionAdd
isLegacyAuthflag to auxiliary user creation.The
createAuxiliaryUserfunction should also include theisLegacyAuthflag for consistency.Apply this diff:
.send({ username: `TestUser-${Math.floor(Math.random() * 10000000000)}`, nickname: 'auxiliary_user', password: 'abc123456!!!', email, emailCode: verificationCode, + isLegacyAuth: true, });
🧹 Nitpick comments (11)
test/avatars.e2e-spec.ts (1)
53-60: Add test coverage for SRP authentication.The test currently only covers the legacy authentication path. Consider adding a parallel test case that uses SRP authentication to ensure both authentication methods work correctly with the Avatar Module.
test/user.profile.e2e-spec.ts (2)
75-82: Add test coverage for SRP authentication.The test currently only covers the legacy authentication path. Consider adding a parallel test case that uses SRP authentication to ensure both authentication methods work correctly with the User Profile Module.
170-188: Address commented-out test case.There's a commented-out test case for getting user profile without a token. Either remove the commented code or implement the test case if it's still relevant.
test/materials.e2e-spec.ts (2)
68-75: Add test coverage for SRP authentication.The test currently only covers the legacy authentication path. Consider adding a parallel test case that uses SRP authentication to ensure both authentication methods work correctly with the Material Module.
91-91: Consider using constants for test file paths.The test uses hardcoded file paths in multiple places. Consider extracting these paths into constants at the top of the file for better maintainability.
Example:
const TEST_RESOURCES = { IMAGE: 'src/materials/resources/test.jpg', VIDEO: 'src/materials/resources/test.mp4', AUDIO: 'src/materials/resources/test.mp3', PDF: 'src/materials/resources/test.pdf' };Also applies to: 102-102, 113-113, 127-127, 138-138, 149-149, 160-160
test/attachments.e2e-spec.ts (2)
71-78: Add test coverage for SRP authentication.The test currently only covers the legacy authentication path. Consider adding a parallel test case that uses SRP authentication to ensure both authentication methods work correctly with the Attachment Module.
221-248: Fix duplicate test descriptions.There are multiple test cases with the same description "should get the uploaded file detail". Consider making the descriptions more specific to each test case:
- it('should get the uploaded file detail', async () => { + it('should get the uploaded image as file detail', async () => { // ... }); - it('should get the uploaded file detail', async () => { + it('should get the uploaded video as file detail', async () => { // ... }); - it('should get the uploaded file detail', async () => { + it('should get the uploaded audio as file detail', async () => { // ... }); - it('should get the uploaded file detail', async () => { + it('should get the uploaded PDF file detail', async () => { // ... });test/topic.e2e-spec.ts (1)
83-83: LGTM! Consider adding a test comment about the auth transition.The addition of
isLegacyAuth: truecorrectly identifies this test as using legacy authentication.Consider adding a comment above the registration payload to document that this test uses legacy auth during the SRP transition period:
+ // Using legacy auth during SRP transition .send({ username: TestUsername, nickname: 'test_user', password: 'abc123456!!!', email: TestEmail, emailCode: verificationCode, isLegacyAuth: true, });test/groups.e2e-spec.ts (1)
118-118: LGTM! Consider adding a comment explaining the legacy auth flag.The addition of
isLegacyAuth: truecorrectly identifies this test user as using legacy authentication, which is important during the transition to SRP.Consider adding a comment explaining why this flag is necessary:
.send({ username: TestUsername, nickname: 'test_user', password: 'abc123456!!!', email: TestEmail, emailCode: verificationCode, + // Using legacy authentication for test users during SRP transition isLegacyAuth: true, });test/answer.e2e-spec.ts (1)
111-111: LGTM! Consider adding a comment explaining the legacy auth flag.The addition of
isLegacyAuth: truecorrectly identifies this test user as using legacy authentication, which is important during the transition to SRP.Consider adding a comment explaining why this flag is necessary:
.send({ username: TestUsername, nickname: 'test_user', password: 'abc123456!!!', email: TestEmail, emailCode: verificationCode, + // Using legacy authentication for test users during SRP transition isLegacyAuth: true, });test/question.e2e-spec.ts (1)
118-118: LGTM! Consider adding a comment explaining the legacy auth flag.The addition of
isLegacyAuth: truecorrectly identifies this test user as using legacy authentication, which is important during the transition to SRP.Consider adding a comment explaining why this flag is necessary:
.send({ username: TestUsername, nickname: 'test_user', password: 'abc123456!!!', email: TestEmail, emailCode: verificationCode, + // Using legacy authentication for test users during SRP transition isLegacyAuth: true, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
test/answer.e2e-spec.ts(1 hunks)test/attachments.e2e-spec.ts(1 hunks)test/avatars.e2e-spec.ts(1 hunks)test/comment.e2e-spec.ts(1 hunks)test/groups.e2e-spec.ts(1 hunks)test/materialbundle.e2e-spec.ts(1 hunks)test/materials.e2e-spec.ts(1 hunks)test/question.e2e-spec.ts(1 hunks)test/topic.e2e-spec.ts(1 hunks)test/user.follow.e2e-spec.ts(1 hunks)test/user.profile.e2e-spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: test (12, 8.0.0, 20.x, 8, 2)
- GitHub Check: test (12, 8.0.0, 20.x, 8, 1)
- GitHub Check: test (12, 8.0.0, 18.x, 8, 2)
- GitHub Check: test (12, 8.0.0, 18.x, 8, 1)
- GitHub Check: test (12, 8.12.2, 20.x, 8, 2)
- GitHub Check: test (12, 8.12.2, 20.x, 8, 1)
- GitHub Check: test (12, 8.12.2, 18.x, 8, 2)
- GitHub Check: test (12, 8.12.2, 18.x, 8, 1)
- GitHub Check: test (latest, 8.0.0, 20.x, 8, 2)
- GitHub Check: test (latest, 8.0.0, 20.x, 8, 1)
- GitHub Check: test (latest, 8.0.0, 18.x, 8, 2)
- GitHub Check: test (latest, 8.0.0, 18.x, 8, 1)
- GitHub Check: test (latest, 8.12.2, 20.x, 8, 2)
- GitHub Check: test (latest, 8.12.2, 20.x, 8, 1)
- GitHub Check: test (latest, 8.12.2, 18.x, 8, 2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: test (latest, 8.12.2, 18.x, 8, 1)
- GitHub Check: build-and-push-image
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
test/user.follow.e2e-spec.ts (1)
108-108: LGTM! The change maintains consistency with other test files.The addition of
isLegacyAuth: truecorrectly identifies this test as using legacy authentication.test/materialbundle.e2e-spec.ts (1)
103-103: LGTM! The change maintains consistency with other test files.The addition of
isLegacyAuth: truecorrectly identifies this test as using legacy authentication.test/comment.e2e-spec.ts (1)
115-115: LGTM! The change maintains consistency with other test files.The addition of
isLegacyAuth: truecorrectly identifies this test as using legacy authentication.
HuanCheng65
force-pushed
the
feat/srp-authentication
branch
from
47a0ae2 to
d3013ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/users/users.spec.ts (2)
89-92: Mock RestorationThis mocking approach is fine. For thorough cleanup, consider using
jest.restoreAllMocks()in anafterEachhook to prevent cross-test side effects.
155-199: SRP Credentials Reset FlowThis test thoroughly verifies the database update and log creation. Setting
hashedPasswordto''is acceptable if the rest of the codebase expects an empty string upon SRP upgrade. Otherwise, consider removing the field entirely to reduce confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.github/workflows/test-coverage.ymlis excluded by!**/*.yml.github/workflows/test.ymlis excluded by!**/*.yml
📒 Files selected for processing (1)
src/users/users.spec.ts(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: test (12, 8.0.0, 20.x, 10, 2)
- GitHub Check: test (12, 8.0.0, 20.x, 10, 1)
- GitHub Check: test (12, 8.0.0, 18.x, 10, 2)
- GitHub Check: test (12, 8.0.0, 18.x, 10, 1)
- GitHub Check: test (12, 8.12.2, 20.x, 10, 2)
- GitHub Check: test (12, 8.12.2, 20.x, 10, 1)
- GitHub Check: test (12, 8.12.2, 18.x, 10, 2)
- GitHub Check: test (12, 8.12.2, 18.x, 10, 1)
- GitHub Check: test (latest, 8.0.0, 20.x, 10, 2)
- GitHub Check: test (latest, 8.0.0, 20.x, 10, 1)
- GitHub Check: test (latest, 8.0.0, 18.x, 10, 2)
- GitHub Check: test (latest, 8.0.0, 18.x, 10, 1)
- GitHub Check: test (latest, 8.12.2, 20.x, 10, 2)
- GitHub Check: test (latest, 8.12.2, 20.x, 10, 1)
- GitHub Check: test (latest, 8.12.2, 18.x, 10, 2)
- GitHub Check: test (latest, 8.12.2, 18.x, 10, 1)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build-and-push-image
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (16)
src/users/users.spec.ts (16)
15-19: Additions of SRP-Like Errors Look AlignedThese newly-imported error classes are coherent with the SRP authentication flow. No issues found.
86-86: Consider Verifying If 120 Seconds Is Truly NeededIncreasing the test timeout to 120 seconds is quite high. Ensure this extended duration is strictly necessary for the environment or heavy integration steps.
102-106: User Not Found Test Looks GoodThe mocking for
findUniquereturning null is logically consistent, ensuringUserIdNotFoundErroris thrown as expected.
118-125: beforeEach Setup is ClearMocking
isEmailSuffixSupportedglobally inbeforeEachis a neat approach. Looks solid.
126-153: Reset Password Email TestThe test flow for sending reset emails is logical. Checking for possible validations (e.g., verifying email content) might be helpful, but this is optional.
201-224: Expired Reset Token HandlingProperly checks for an expired token scenario by throwing an error. This is well-covered.
226-249: Invalid Reset Token HandlingCovers the invalid token path by throwing “Permission denied.” No concerns here.
251-277: SRP Password ChangeClearing the old
hashedPasswordand setting new SRP fields is consistent. The test ensures the updated fields are persisted correctly.
307-335: Sudo Mode Password TestMocking
hashedPasswordvia bcrypt and verifyingaccessTokenis correct. The test checks success for a valid password. Everything looks proper.
362-362: Sudo Mode Passkey TestValidates that the new sudo token is generated after successful passkey authentication. The assertion is correct.
384-401: Passkey Authentication beforeEachThis setup effectively mocks user data and userProfile for subsequent passkey tests. No issues.
537-569: SRP Init TestingMocks
createServerSessionand verifies the returned salt and ephemeral keys. The coverage is solid.
570-619: SRP Verification SuccessEnsures the test updates login logs, fetches user info, and generates a session. This aligns well with SRP flow.
620-670: 2FA Integration in SRPCorrectly checks when
totpEnabledis true, sets atempToken, and flagsrequires2FA. Good coverage for multi-factor flows.
672-684: Non-upgraded SRP ErrorThrows
SrpNotUpgradedErrorif the user is not upgraded. Straightforward logic.
686-714: Failed SRP VerificationProperly throws
SrpVerificationErroron SRP mismatch. Test scenario is handled cleanly.
HuanCheng65
force-pushed
the
feat/srp-authentication
branch
from
2f93a5a to
4640051
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #386 +/- ##
==========================================
- Coverage 89.10% 88.22% -0.88%
==========================================
Files 73 74 +1
Lines 3102 3278 +176
Branches 437 472 +35
==========================================
+ Hits 2764 2892 +128
- Misses 318 385 +67
+ Partials 20 1 -19 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
test/user.e2e-spec.ts (1)
1065-1168:⚠️ Potential issueFix failing passkey authentication tests.
The pipeline failures indicate that all passkey authentication endpoints are returning 401 Unauthorized errors. This suggests that the sudo mode verification might not be persisting correctly or the token is not being properly passed.
- Verify that the sudo mode token is valid and properly set
- Add debug logging to track token state
- Consider adding test for token verification before passkey operations
describe('Passkey Authentication Endpoints', () => { let testUserId: number; let validToken: string; beforeAll(async () => { const auth = await loginWithSRP( app.getHttpServer(), TestUsername, TestNewPassword, ); validToken = auth.accessToken; testUserId = auth.userId; // Enter sudo mode validToken = await verifySudoWithSRP( app.getHttpServer(), validToken, TestUsername, TestNewPassword, ); + + // Verify sudo mode is active + const verifyRes = await request(app.getHttpServer()) + .get('/users/auth/sudo/status') + .set('Authorization', `Bearer ${validToken}`) + .expect(200); + + expect(verifyRes.body.data.sudoMode).toBe(true); }); it('POST /users/{userId}/passkeys/options should return registration options', async () => { const res = await request(app.getHttpServer()) .post(`/users/${testUserId}/passkeys/options`) .set('Authorization', `Bearer ${validToken}`) .send() .expect(201); + + // Add more specific assertions + expect(res.body.data.options.user).toBeDefined(); + expect(res.body.data.options.challenge).toBeDefined(); expect(res.body.data.options).toBeDefined(); });
🧹 Nitpick comments (3)
test/user.e2e-spec.ts (3)
62-117: Consider enhancing theloginWithSRPhelper function.The function is well-structured but could benefit from:
- English documentation for better international collaboration
- Error handling for network failures
- Type safety for the response body
-/** - * 使用 SRP 进行登录 - */ +/** + * Performs user login using the Secure Remote Password (SRP) protocol. + * @param httpServer - The HTTP server instance + * @param username - The username to login with + * @param password - The password to login with + * @returns Promise containing access token, refresh token and user ID + * @throws {Error} If network request fails or server returns unexpected response + */ async function loginWithSRP( httpServer: HttpServer, username: string, password: string, ): Promise<{ accessToken: string; refreshToken: string; userId: number; }> { + try { const clientEphemeral = srpClient.generateEphemeral(); const agent = request.agent(httpServer); // 1. Initialize SRP login const initResponse = await agent .post('/users/auth/srp/init') .send({ username, clientPublicEphemeral: clientEphemeral.public, }) .expect(201); + + // Type guard for response body + if (!initResponse.body?.data?.salt || !initResponse.body?.data?.serverPublicEphemeral) { + throw new Error('Invalid server response: missing required SRP fields'); + } // Rest of the implementation... + } catch (error) { + throw new Error(`SRP login failed: ${error.message}`); + } }
119-167: Consider enhancing theverifySudoWithSRPhelper function.Similar improvements needed as
loginWithSRP:
- English documentation
- Error handling
- Type safety
-/** - * 使用 SRP 进行 sudo 验证 - */ +/** + * Verifies sudo mode using the Secure Remote Password (SRP) protocol. + * @param httpServer - The HTTP server instance + * @param token - The access token for authorization + * @param username - The username to verify + * @param password - The password to verify + * @returns Promise containing the new access token + * @throws {Error} If network request fails or server returns unexpected response + */ async function verifySudoWithSRP( httpServer: HttpServer, token: string, username: string, password: string, ): Promise<string> { + try { // Implementation... + } catch (error) { + throw new Error(`SRP sudo verification failed: ${error.message}`); + } }🧰 Tools
🪛 GitHub Actions: Automatic Test
[error] 164-164: Expected 201 'Created', got 401 'Unauthorized' in POST /users/{userId}/passkeys/options.
[error] 164-164: Expected 201 'Created', got 401 'Unauthorized' in POST /users/{userId}/passkeys.
[error] 164-164: Expected 201 'Created', got 401 'Unauthorized' in POST /users/auth/passkey/options.
[error] 164-164: Expected 201 'Created', got 401 'Unauthorized' in POST /users/auth/passkey/verify.
[error] 164-164: Expected 201 'Created', got 401 'Unauthorized' in GET /users/{userId}/passkeys.
[error] 164-164: Expected 201 'Created', got 401 'Unauthorized' in DELETE /users/{userId}/passkeys/{credentialId}.
20-31: Remove commented-out mock implementation.The commented-out mock implementation for the SRP client should be either used or removed to maintain code cleanliness.
-// // Mock SRP client -// jest.mock('secure-remote-password/client', () => ({ -// generateSalt: jest.fn(() => 'test-salt'), -// derivePrivateKey: jest.fn((salt, username, password) => 'test-private-key'), -// deriveVerifier: jest.fn((privateKey) => 'test-verifier'), -// generateEphemeral: jest.fn(() => ({ public: 'client-public', secret: 'client-secret' })), -// deriveSession: jest.fn((clientSecretEphemeral, serverPublicEphemeral, salt, username, privateKey) => ({ -// key: 'session-key', -// proof: 'client-proof' -// })), -// verifySession: jest.fn((serverProof, clientSession) => true) -// }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.github/workflows/test-coverage.ymlis excluded by!**/*.yml.github/workflows/test.ymlis excluded by!**/*.yml
📒 Files selected for processing (1)
test/user.e2e-spec.ts(16 hunks)
🧰 Additional context used
🪛 GitHub Actions: Automatic Test
test/user.e2e-spec.ts
[error] 164-164: Expected 201 'Created', got 401 'Unauthorized' in POST /users/{userId}/passkeys/options.
[error] 164-164: Expected 201 'Created', got 401 'Unauthorized' in POST /users/{userId}/passkeys.
[error] 164-164: Expected 201 'Created', got 401 'Unauthorized' in POST /users/auth/passkey/options.
[error] 164-164: Expected 201 'Created', got 401 'Unauthorized' in POST /users/auth/passkey/verify.
[error] 164-164: Expected 201 'Created', got 401 'Unauthorized' in GET /users/{userId}/passkeys.
[error] 164-164: Expected 201 'Created', got 401 'Unauthorized' in DELETE /users/{userId}/passkeys/{credentialId}.
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: test (12, 8.0.0, 20.x, 2)
- GitHub Check: test (12, 8.0.0, 20.x, 1)
- GitHub Check: test (12, 8.0.0, 18.x, 2)
- GitHub Check: test (12, 8.0.0, 18.x, 1)
- GitHub Check: test (12, 8.12.2, 20.x, 2)
- GitHub Check: test (12, 8.12.2, 20.x, 1)
- GitHub Check: test (12, 8.12.2, 18.x, 2)
- GitHub Check: test (12, 8.12.2, 18.x, 1)
- GitHub Check: test (latest, 8.0.0, 20.x, 2)
- GitHub Check: test (latest, 8.0.0, 20.x, 1)
- GitHub Check: test (latest, 8.0.0, 18.x, 2)
- GitHub Check: test (latest, 8.0.0, 18.x, 1)
- GitHub Check: test (latest, 8.12.2, 20.x, 2)
- GitHub Check: test (latest, 8.12.2, 20.x, 1)
- GitHub Check: test (latest, 8.12.2, 18.x, 2)
- GitHub Check: test (latest, 8.12.2, 18.x, 1)
- GitHub Check: build-and-push-image
HuanCheng65
force-pushed
the
feat/srp-authentication
branch
from
b47c612 to
ac2c989
Compare
There was a problem hiding this comment.
LGTM! Thanks @HuanCheng65 for the quick support. Leave some comments here.
BTW, is this a breaking change? Will the existing webpage logic be accepted when this new backend get deployed?
| @@ -101,7 +100,6 @@ jobs: | |||
| - name: Install pnpm | |||
| uses: pnpm/action-setup@v4 | |||
| with: | |||
| version: ${{ matrix.pnpm-version }} | |||
| export class PasskeyAuthenticationOptionsRequestDto { | ||
| userId?: number; | ||
| } |
| export class SrpInitRequestDto { | ||
| username: string; | ||
| clientPublicEphemeral: string; | ||
| } |
Add comprehensive SRP authentication features: