implement usual user login functionality with validation and auditing#1851
Merged
Conversation
📝 WalkthroughWalkthroughAdds a ChangesSaaS Usual Login Bridge
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a SaaS “usual login” bridge endpoint (POST /saas/user/login) to the backend microservice layer. This lets the SaaS control plane verify email/password (including request-domain validation and sign-in auditing) and return FoundUserDto without minting an end-user token/cookie (left to rocketadmin-saas).
Changes:
- Introduces
SaasUsualLoginUseCasethat mirrors OSSUsualLoginUseCaselogin verification + domain validation + sign-in audit recording, but returnsFoundUserDtoinstead of signing JWTs. - Wires the new use case into
SaasModule/DI and exposesPOST /saas/user/logininSaasController. - Adds AVA e2e coverage for the new SaaS login bridge behavior (success, 2FA flag surfaced, wrong password, unknown user, missing microservice JWT).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/test/ava-tests/saas-tests/saas-user-login-e2e.test.ts | New e2e tests for the SaaS login bridge endpoint behavior (no token/cookie, 2FA flag, auth required). |
| backend/src/microservices/saas-microservice/use-cases/saas-usual-login.use.case.ts | New SaaS login use case that verifies credentials, validates request domain, and records sign-in audits, returning FoundUserDto. |
| backend/src/microservices/saas-microservice/use-cases/saas-use-cases.interface.ts | Adds ISaasUsualLoginUser interface for the new use case. |
| backend/src/microservices/saas-microservice/saas.module.ts | Registers the new use case provider and applies SaaS auth middleware to the new route. |
| backend/src/microservices/saas-microservice/saas.controller.ts | Adds POST /saas/user/login handler delegating to the new use case. |
| backend/src/common/data-injection.tokens.ts | Adds a new UseCaseType token for SaaS usual login. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+176
to
+181
| @ApiOperation({ summary: 'User login webhook' }) | ||
| @ApiResponse({ | ||
| status: 200, | ||
| description: 'Credentials verified; user info returned (no token is signed here — the caller signs the cookie).', | ||
| type: FoundUserDto, | ||
| }) |
| @@ -0,0 +1,183 @@ | |||
| import { faker } from '@faker-js/faker'; | |||
![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
🧹 Nitpick comments (3)
backend/test/ava-tests/saas-tests/saas-user-login-e2e.test.ts (3)
95-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a template literal for the error message.
As per coding guidelines,
**/*.{js,ts,jsx,tsx}should “Use template literals instead of string concatenation.”Proposed fix
- console.error('After tests error ' + e); + console.error(`After tests error ${e}`);🤖 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 `@backend/test/ava-tests/saas-tests/saas-user-login-e2e.test.ts` at line 95, The error logging in saas-user-login-e2e.test.ts uses string concatenation instead of the required template literal. Update the console.error call in the test’s after-tests error handling to use a template literal while preserving the same message and error value, following the project guideline for all js/ts/jsx/tsx code.Source: Coding guidelines
32-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign test helpers with the TypeScript style rules.
Use arrow-function helpers and named interfaces for the options/fixture shapes. As per coding guidelines,
**/*.{js,ts,jsx,tsx}should “Prefer arrow functions over function declarations” and**/*.{ts,tsx}should “Use interfaces for object shapes and type for unions and primitives.”Proposed refactor
-function microserviceAuthHeader(): string { +const microserviceAuthHeader = (): string => { const token = jwt.sign({ request_id: faker.string.uuid() }, appConfig.auth.microserviceJwtSecret); return `Bearer ${token}`; -} +}; -async function createCoreUser(opts: { isOTPEnabled?: boolean } = {}): Promise<{ +interface CoreUserOptions { + isOTPEnabled?: boolean; +} + +interface CoreUserFixture { email: string; password: string; companyId: string; userId: string; -}> { +} + +const createCoreUser = async (opts: CoreUserOptions = {}): Promise<CoreUserFixture> => {🤖 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 `@backend/test/ava-tests/saas-tests/saas-user-login-e2e.test.ts` around lines 32 - 43, The test helper definitions are not following the TypeScript style rules: `microserviceAuthHeader` is a function declaration and the inline object shapes in `createCoreUser` use anonymous types. Refactor these helpers to use arrow functions, and introduce named interfaces for the options and returned fixture shapes used by `createCoreUser` so the test file matches the project’s TS conventions.Source: Coding guidelines
101-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the non-
companyIdresolution paths.The new use case has separate branches for custom-domain lookup and single-company email fallback, but the e2e suite only exercises the explicit
companyIdpath. Add success/failure coverage for those branches to protect the login bridge contract.🤖 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 `@backend/test/ava-tests/saas-tests/saas-user-login-e2e.test.ts` around lines 101 - 166, The saas user login e2e suite only covers the explicit companyId branch, so the custom-domain lookup and single-company email fallback paths in the login flow are untested. Add test coverage in saas-user-login-e2e.test.ts around the existing test.serial cases for /saas/user/login to verify both successful and failing resolution when companyId is omitted, using createCoreUser and the current login request setup. Include assertions for the resolved user response on success and the expected rejection behavior on failure so the bridge contract is protected across all resolution branches.
🤖 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 `@backend/src/microservices/saas-microservice/saas.controller.ts`:
- Around line 183-194: Validate the request body in usualUserLogin before
calling usualLoginUserUseCase.execute by introducing a DTO or explicit guards
for required fields like email, password, and request_domain. Update the
controller method to accept the validated object instead of raw `@Body` fields,
and ensure missing/invalid inputs are rejected early so
userData.email.toLowerCase() is never reached with undefined.
---
Nitpick comments:
In `@backend/test/ava-tests/saas-tests/saas-user-login-e2e.test.ts`:
- Line 95: The error logging in saas-user-login-e2e.test.ts uses string
concatenation instead of the required template literal. Update the console.error
call in the test’s after-tests error handling to use a template literal while
preserving the same message and error value, following the project guideline for
all js/ts/jsx/tsx code.
- Around line 32-43: The test helper definitions are not following the
TypeScript style rules: `microserviceAuthHeader` is a function declaration and
the inline object shapes in `createCoreUser` use anonymous types. Refactor these
helpers to use arrow functions, and introduce named interfaces for the options
and returned fixture shapes used by `createCoreUser` so the test file matches
the project’s TS conventions.
- Around line 101-166: The saas user login e2e suite only covers the explicit
companyId branch, so the custom-domain lookup and single-company email fallback
paths in the login flow are untested. Add test coverage in
saas-user-login-e2e.test.ts around the existing test.serial cases for
/saas/user/login to verify both successful and failing resolution when companyId
is omitted, using createCoreUser and the current login request setup. Include
assertions for the resolved user response on success and the expected rejection
behavior on failure so the bridge contract is protected across all resolution
branches.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10ccf2a5-0847-4b73-915d-766b9c91d5a9
📒 Files selected for processing (6)
backend/src/common/data-injection.tokens.tsbackend/src/microservices/saas-microservice/saas.controller.tsbackend/src/microservices/saas-microservice/saas.module.tsbackend/src/microservices/saas-microservice/use-cases/saas-use-cases.interface.tsbackend/src/microservices/saas-microservice/use-cases/saas-usual-login.use.case.tsbackend/test/ava-tests/saas-tests/saas-user-login-e2e.test.ts
Comment on lines
+183
to
+194
| async usualUserLogin( | ||
| @Body('email') email: string, | ||
| @Body('password') password: string, | ||
| @Body('companyId') companyId: string, | ||
| @Body('request_domain') request_domain: string, | ||
| @Body('ipAddress') ipAddress: string, | ||
| @Body('userAgent') userAgent: string, | ||
| ): Promise<FoundUserDto> { | ||
| return await this.usualLoginUserUseCase.execute( | ||
| { email, password, companyId, request_domain, ipAddress, userAgent, gclidValue: null }, | ||
| InTransactionEnum.OFF, | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate the login body before invoking the use case.
The handler passes raw body fields into the use case; a missing email reaches userData.email.toLowerCase() and can turn a bad request into a 500. Use a DTO with runtime validators or explicit checks for required email, password, and request_domain.
Proposed direction
- async usualUserLogin(
- `@Body`('email') email: string,
- `@Body`('password') password: string,
- `@Body`('companyId') companyId: string,
- `@Body`('request_domain') request_domain: string,
- `@Body`('ipAddress') ipAddress: string,
- `@Body`('userAgent') userAgent: string,
- ): Promise<FoundUserDto> {
+ async usualUserLogin(`@Body`() loginData: SaasUsualLoginDto): Promise<FoundUserDto> {
return await this.usualLoginUserUseCase.execute(
- { email, password, companyId, request_domain, ipAddress, userAgent, gclidValue: null },
+ { ...loginData, gclidValue: null },
InTransactionEnum.OFF,
);
}📝 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.
Suggested change
| async usualUserLogin( | |
| @Body('email') email: string, | |
| @Body('password') password: string, | |
| @Body('companyId') companyId: string, | |
| @Body('request_domain') request_domain: string, | |
| @Body('ipAddress') ipAddress: string, | |
| @Body('userAgent') userAgent: string, | |
| ): Promise<FoundUserDto> { | |
| return await this.usualLoginUserUseCase.execute( | |
| { email, password, companyId, request_domain, ipAddress, userAgent, gclidValue: null }, | |
| InTransactionEnum.OFF, | |
| ); | |
| async usualUserLogin(`@Body`() loginData: SaasUsualLoginDto): Promise<FoundUserDto> { | |
| return await this.usualLoginUserUseCase.execute( | |
| { ...loginData, gclidValue: null }, | |
| InTransactionEnum.OFF, | |
| ); |
🤖 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 `@backend/src/microservices/saas-microservice/saas.controller.ts` around lines
183 - 194, Validate the request body in usualUserLogin before calling
usualLoginUserUseCase.execute by introducing a DTO or explicit guards for
required fields like email, password, and request_domain. Update the controller
method to accept the validated object instead of raw `@Body` fields, and ensure
missing/invalid inputs are rejected early so userData.email.toLowerCase() is
never reached with undefined.
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
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.
Summary by CodeRabbit
New Features
Bug Fixes
Tests