Add end-to-end tests for SaaS email triggers and user account bridges - #1868
Merged
Conversation
- Implemented e2e tests for email trigger inversion scenarios in `saas-email-trigger-inversion-e2e.test.ts`, covering user registration, email verification, password reset requests, and email change requests with and without the suppressEmail flag. - Created comprehensive tests for user account management in `saas-user-account-bridges-e2e.test.ts`, including user profile retrieval, password changes, name updates, settings management, 2FA enrollment and verification, and account deletion. - Ensured proper handling of microservice JWT authentication across all new tests.
📝 WalkthroughWalkthroughThe PR adds suppressed-email responses for SaaS bridge flows, routes email delivery through a SaaS gateway, adds SaaS account and OTP endpoints, supports scoped token validation, and adds guarded self-hosted administrator endpoints for user password and email updates. ChangesEmail delivery and bridge flows
SaaS account and OTP bridges
Self-hosted administrator updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 comprehensive SaaS end-to-end coverage for the new “email trigger inversion” behavior (returning raw-token payloads when suppressEmail is set) and for new internal user-account “bridge” endpoints, while also extending the self-hosted operations module with admin-managed user email/password updates and introducing an allowlisted-scope option for JWT validation.
Changes:
- Added AVA e2e tests for SaaS email flows and for internal SaaS user-account bridge endpoints (profile/settings/password/name/2FA/delete/validate-token).
- Implemented trigger inversion across SaaS bridge email flows via
suppressEmail+emailPayloadreturn shapes, and refactored core email sending to route through the SaaS email webhook (or suppress entirely self-hosted). - Added self-hosted-only admin endpoints to update a user’s password/email, with cookie auth middleware applied only when the self-hosted module variant is registered.
Reviewed changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/test/ava-tests/saas-tests/saas-user-account-bridges-e2e.test.ts | New SaaS e2e coverage for internal user-account bridge endpoints and microservice JWT auth. |
| backend/test/ava-tests/saas-tests/saas-email-trigger-inversion-e2e.test.ts | New SaaS e2e coverage for suppressEmail trigger inversion and backward compatibility. |
| backend/test/ava-tests/non-saas-tests/non-saas-selfhosted-user-admin-e2e.test.ts | New self-hosted e2e coverage for admin-managed invite flow and admin user management endpoints. |
| backend/src/selfhosted-operations/selhosted-operations.module.ts | Adds self-hosted admin route auth middleware wiring and new self-hosted use-case providers. |
| backend/src/selfhosted-operations/selfhosted-operations.controller.ts | Adds self-hosted admin endpoints to update a user’s password/email. |
| backend/src/selfhosted-operations/application/use-cases/update-user-password-as-admin.use.case.ts | New use case to set a user password (self-hosted, same-company constraint). |
| backend/src/selfhosted-operations/application/use-cases/update-user-email-as-admin.use.case.ts | New use case to set a user email (self-hosted, uniqueness + same-company constraint). |
| backend/src/selfhosted-operations/application/use-cases/selfhosted-use-cases.interfaces.ts | Extends self-hosted use-case interfaces for new admin operations. |
| backend/src/selfhosted-operations/application/dto/update-user-password-as-admin.dto.ts | DTO for self-hosted admin password update. |
| backend/src/selfhosted-operations/application/dto/update-user-email-as-admin.dto.ts | DTO for self-hosted admin email update. |
| backend/src/selfhosted-operations/application/data-structures/update-user-as-admin.ds.ts | Data structures for self-hosted admin update operations. |
| backend/src/microservices/saas-microservice/use-cases/saas-usual-register-user.use.case.ts | Adds suppressEmail trigger inversion to SaaS register flow and returns payload when suppressed. |
| backend/src/microservices/saas-microservice/use-cases/saas-use-cases.interface.ts | Updates SaaS use-case interfaces for new RO types and OTP login bridge. |
| backend/src/microservices/saas-microservice/use-cases/saas-otp-login.use.case.ts | New SaaS OTP login bridge use case validating temporary tokens + delegating OTP verification. |
| backend/src/microservices/saas-microservice/saas.module.ts | Wires new SaaS bridge use cases and secures new bridge routes with microservice JWT middleware. |
| backend/src/microservices/saas-microservice/saas.controller.ts | Adds SaaS internal user-account bridge endpoints and extends email-flow bridges for suppressEmail. |
| backend/src/microservices/saas-microservice/data-structures/saas-user-account.dtos.ts | DTOs for new SaaS user-account bridge endpoints (password/name/settings/2FA/etc.). |
| backend/src/microservices/saas-microservice/data-structures/saas-otp-login.ds.ts | DS for SaaS OTP login bridge use case input. |
| backend/src/microservices/saas-microservice/data-structures/saas-email-flows.dtos.ts | Adds suppressEmail fields and defines the SaasRegisteredUserRO with optional emailPayload. |
| backend/src/microservices/gateways/saas-gateway.ts/saas-gateway.module.ts | Exports the new SaaS email gateway service. |
| backend/src/microservices/gateways/saas-gateway.ts/saas-email-gateway.service.ts | New gateway to POST composed email requests to the SaaS webhook with timeout + swallow-on-failure. |
| backend/src/microservices/agents-microservice/use-cases/validate-user-token.use.case.ts | Extends JWT validation to optionally allow scopes (e.g. 2fa_enable) while adjusting suspension/scope checks. |
| backend/src/microservices/agents-microservice/use-cases/agents-use-cases.interface.ts | Updates IValidateUserToken signature to accept an input DS. |
| backend/src/microservices/agents-microservice/dto/agents-auth.dtos.ts | Extends token validation DTO with allowScopes. |
| backend/src/microservices/agents-microservice/data-structures/agents.ds.ts | Adds ValidateUserTokenDs for the updated token validation use case signature. |
| backend/src/microservices/agents-microservice/agents.controller.ts | Updates validate-token endpoint to call the updated use case signature. |
| backend/src/exceptions/text/messages.ts | Removes the unused EMAIL_SEND_FAILED message. |
| backend/src/entities/user/user.controller.ts | Hardens public reset-request route by explicitly copying allowed fields (prevents suppressEmail smuggling). |
| backend/src/entities/user/use-cases/verify-change-user-email.use.case.ts | Adds trigger inversion for “email changed” notice via optional emailPayload. |
| backend/src/entities/user/use-cases/user-use-cases.interfaces.ts | Updates user use-case interfaces to return payload-aware message structures and new DS types. |
| backend/src/entities/user/use-cases/request-reset-user-password.use.case.ts | Adds trigger inversion for password reset request flow and user-enumeration-safe behavior when suppressed. |
| backend/src/entities/user/use-cases/request-email-verification.use.case.ts | Adds trigger inversion for email verification resend flow. |
| backend/src/entities/user/use-cases/request-change-user-email.use.case.ts | Adds trigger inversion for email change request flow. |
| backend/src/entities/user/application/data-structures/usual-register-user.ds.ts | Adds suppressEmail to SaaS register DS. |
| backend/src/entities/user/application/data-structures/request-password-reset.ds.ts | New DS to prevent forwarding untrusted request bodies (bridge-only suppressEmail). |
| backend/src/entities/user/application/data-structures/request-email-change.ds.ts | Adds suppressEmail to email-change and email-verification request DS. |
| backend/src/entities/user/application/data-structures/operation-result-message.ds.ts | Adds OperationResultMessageWithEmailPayloadDs. |
| backend/src/entities/user/application/data-structures/change-user-email.ds.ts | Adds suppressEmail to email-change verification DS. |
| backend/src/entities/email/repository/email-verification-custom-repository-extension.ts | Fixes create/update logic to avoid relation-loading assumptions and unique constraint collisions. |
| backend/src/entities/email/email/email.service.ts | Refactors outgoing email to dispatch via SaaS webhook (or suppress self-hosted), keeping transporter path as dead code. |
| backend/src/entities/email/application/data-structures/outgoing-email-payload.ds.ts | Defines the common OutgoingEmailPayloadDs type returned when trigger inversion is enabled. |
| backend/src/entities/company-info/use-cases/invite-user-in-company.use.case.ts | Adds trigger inversion for invite + re-invite inactive flow; self-hosted logs links instead of sending. |
| backend/src/entities/company-info/application/data-structures/invited-user-in-company-and-connection-group.ds.ts | Extends invite response DS with optional emailPayload and inactive-user marker. |
| backend/src/entities/company-info/application/data-structures/invite-user-in-company-and-connection-group.ds.ts | Adds suppressEmail flag to invite DS (bridge-only). |
| backend/src/common/data-injection.tokens.ts | Adds new use-case tokens for SaaS OTP login and self-hosted admin user updates. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
88
to
91
| @Post('/auth/validate-user-token') | ||
| public async validateUserToken(@Body() body: ValidateUserTokenDto): Promise<ValidatedUserTokenRO> { | ||
| return await this.validateUserTokenUseCase.execute(body.token, InTransactionEnum.OFF); | ||
| return await this.validateUserTokenUseCase.execute({ token: body.token }, InTransactionEnum.OFF); | ||
| } |
Comment on lines
1
to
+9
| import { Response } from 'express'; | ||
|
|
||
| export class ValidateUserTokenDs { | ||
| token: string; | ||
| // Scopes the caller is willing to accept on the token (plan 15 Phase 5). When it includes | ||
| // '2fa_enable', validation mirrors the core's NonScopedAuthMiddleware instead of AuthMiddleware. | ||
| // Absent/empty = current strict behavior (backward compatible). | ||
| allowScopes?: Array<string>; | ||
| } |
Comment on lines
57
to
61
| const foundUser = await this._dbContext.userRepository.findOneUserById(userId); | ||
| if (!foundUser) { | ||
| throw new UnauthorizedException('JWT verification failed'); | ||
| } | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/entities/email/repository/email-verification-custom-repository-extension.ts (1)
26-38: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake verification replacement atomic.
findOne,remove, andsaveare separate operations. Concurrent requests can both observe no row and then onesavecan violate the uniqueuserIdconstraint. Use an atomic upsert, or serialize replacement per user and handle unique conflicts.🤖 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/entities/email/repository/email-verification-custom-repository-extension.ts` around lines 26 - 38, The verification replacement flow around findOne, remove, and save is not atomic and can race on the userId unique constraint. Update the repository method to use an atomic upsert keyed by the user id, or serialize replacement per user and handle unique-conflict retries, while preserving generation and return of the new verification token.
🧹 Nitpick comments (11)
backend/src/microservices/agents-microservice/dto/agents-auth.dtos.ts (1)
14-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a template literal for the Swagger description.
Lines 15-17 concatenate string literals. Use one template literal instead.
Proposed fix
- description: - "Scopes the caller is willing to accept on the token (plan 15 Phase 5). When it includes '2fa_enable', " + - 'validation mirrors NonScopedAuthMiddleware (no 2fa-scope rejection, no suspension check). ' + - 'Absent = current strict behavior.', + description: `Scopes the caller is willing to accept on the token (plan 15 Phase 5). When it includes '2fa_enable', validation mirrors NonScopedAuthMiddleware (no 2fa-scope rejection, no suspension check). Absent = current strict behavior.`,As per coding guidelines, "Use template literals instead of string concatenation."
🤖 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/agents-microservice/dto/agents-auth.dtos.ts` around lines 14 - 17, Update the Swagger description in the agents-auth DTO to use a single template literal instead of concatenated string literals, preserving the existing text and formatting.Source: Coding guidelines
backend/test/ava-tests/saas-tests/saas-user-account-bridges-e2e.test.ts (1)
40-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConvert the six test helpers to arrow functions. Update
microserviceAuthHeader,registerUser,activateUser,getProfile,enrollUserIntoOtp, andmintTemporaryToken.🤖 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-account-bridges-e2e.test.ts` around lines 40 - 43, Convert the six test helpers to arrow-function assignments: microserviceAuthHeader at backend/test/ava-tests/saas-tests/saas-user-account-bridges-e2e.test.ts:40-43, registerUser at :45-65, activateUser at :67-73, getProfile at :75-80, enrollUserIntoOtp at :83-107, and mintTemporaryToken at :111-114. Preserve each helper’s existing parameters, return values, and behavior.Source: Coding guidelines
backend/src/selfhosted-operations/application/use-cases/update-user-password-as-admin.use.case.ts (2)
41-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the return type annotation.
findTargetUserInCallerCompanyhas no declared return type. The coding guidelines require type annotations on function parameters and return types in TypeScript. DeclarePromise<UserEntity>.🤖 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/selfhosted-operations/application/use-cases/update-user-password-as-admin.use.case.ts` around lines 41 - 45, Update the findTargetUserInCallerCompany method signature to explicitly declare Promise<UserEntity> as its return type, while preserving its existing parameters and implementation.Source: Coding guidelines
41-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated company-scoping helper in both administrator use cases.
findTargetUserInCallerCompanyis implemented identically in both files, including the comment. The shared root cause is missing extraction of the company-membership check. Extract it into one shared helper or a small base class, and give it an explicitPromise<UserEntity>return type as required by the coding guidelines.
backend/src/selfhosted-operations/application/use-cases/update-user-password-as-admin.use.case.ts#L41-L53: replace the private method with a call to the shared helper.backend/src/selfhosted-operations/application/use-cases/update-user-email-as-admin.use.case.ts#L52-L64: replace the private method with a call to the same shared helper.The helper also takes
dbContextas a parameter while both callers passthis._dbContext. Drop the parameter if the helper stays inside the classes.🤖 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/selfhosted-operations/application/use-cases/update-user-password-as-admin.use.case.ts` around lines 41 - 53, Extract the duplicated findTargetUserInCallerCompany logic into one shared helper or base class, giving it an explicit Promise<UserEntity> return type and using the helper’s existing database context instead of passing this._dbContext as an argument. Replace the private helper and its call sites in backend/src/selfhosted-operations/application/use-cases/update-user-password-as-admin.use.case.ts#L41-L53 and backend/src/selfhosted-operations/application/use-cases/update-user-email-as-admin.use.case.ts#L52-L64 with the shared implementation; both sites require the same change.Source: Coding guidelines
backend/src/selfhosted-operations/application/use-cases/update-user-email-as-admin.use.case.ts (1)
52-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the return type annotation.
findTargetUserInCallerCompanyhas no declared return type. The coding guidelines require type annotations on function parameters and return types in TypeScript. DeclarePromise<UserEntity>.🤖 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/selfhosted-operations/application/use-cases/update-user-email-as-admin.use.case.ts` around lines 52 - 56, Annotate the findTargetUserInCallerCompany method with the required Promise<UserEntity> return type, leaving its parameters and implementation unchanged.Source: Coding guidelines
backend/test/ava-tests/non-saas-tests/non-saas-selfhosted-user-admin-e2e.test.ts (2)
78-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCheck the response status before parsing.
getUserProfileparsesfoundUser.textwithout a status check. If/user/returns an error,JSON.parseeither throws a syntax error or produces an error body, and the test fails with a message that does not identify the cause. The other helpers in this file check the status first. Add the same check here.♻️ Proposed change
const foundUser = await request(app.getHttpServer()) .get('/user/') .set('Cookie', userToken) .set('Content-Type', 'application/json') .set('Accept', 'application/json'); + if (foundUser.status !== 200) { + throw new Error(`Get user profile failed: ${foundUser.text}`); + } return JSON.parse(foundUser.text);🤖 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/non-saas-tests/non-saas-selfhosted-user-admin-e2e.test.ts` around lines 78 - 85, Update getUserProfile to validate the /user/ response status before calling JSON.parse, matching the status-check pattern used by the other helpers in this file; preserve parsing only for successful responses and report failures through the existing test error path.
291-322: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a cross-company test for the email endpoint.
The password endpoint has a foreign-admin test at Lines 248-263, but the email endpoint has none. Add the same case for
PUT /selfhosted/users/:userId/email. Assert404. This test also locks in the check ordering discussed inbackend/src/selfhosted-operations/application/use-cases/update-user-email-as-admin.use.case.ts: a foreign admin must receive404, not400 CANNOT_SET_THIS_EMAIL, when the submitted address already exists.🤖 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/non-saas-tests/non-saas-selfhosted-user-admin-e2e.test.ts` around lines 291 - 322, Add a serial cross-company test alongside the existing email-update tests, using the foreign-admin setup pattern from the password endpoint test and targeting PUT /selfhosted/users/:userId/email with an already-used email. Assert that the response status is 404, ensuring the foreign-user check occurs before the email uniqueness validation.backend/src/selfhosted-operations/selhosted-operations.module.ts (1)
76-81: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winThe authentication route paths are duplicated as string literals across the module and the controller.
configure()hardcodes the absolute paths, while the controller declares only the relative paths plus a prefix. The two must stay in agreement. If a route or the controller prefix changes,AuthMiddlewarestops matching and the endpoints lose cookie authentication with no compile-time error.
backend/src/selfhosted-operations/selhosted-operations.module.ts#L76-L81: derive the middleware targets fromSelfHostedOperationsController(for exampleforRoutes(SelfHostedOperationsController)combined with method filtering) or define the path segments as shared constants.backend/src/selfhosted-operations/selfhosted-operations.controller.ts#L67-L67: use the same shared path constants in the@Putdecorators so both sites change together.🤖 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/selfhosted-operations/selhosted-operations.module.ts` around lines 76 - 81, Eliminate duplicated authentication route literals by introducing shared path constants and using them in both SelfHostedOperationsModule.configure() at backend/src/selfhosted-operations/selhosted-operations.module.ts:76-81 and SelfHostedOperationsController at backend/src/selfhosted-operations/selfhosted-operations.controller.ts:67. Update the AuthMiddleware route targets and the controller’s `@Put` decorators to consume those constants, preserving the existing PUT endpoints and controller prefix.backend/test/ava-tests/saas-tests/saas-email-trigger-inversion-e2e.test.ts (2)
30-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConvert the helpers to arrow functions.
microserviceAuthHeaderandrandomEmailare function declarations.registerUserat Line 39 is also a function declaration.♻️ Proposed change
-function microserviceAuthHeader(): string { +const microserviceAuthHeader = (): string => { const token = jwt.sign({ request_id: faker.string.uuid() }, appConfig.auth.microserviceJwtSecret); return `Bearer ${token}`; -} +}; -function randomEmail(): string { +const randomEmail = (): string => { return `${faker.lorem.word()}_${faker.string.alphanumeric(6)}_${faker.internet.email()}`.toLowerCase(); -} +};As per coding guidelines: "Prefer arrow functions over function declarations".
🤖 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-email-trigger-inversion-e2e.test.ts` around lines 30 - 37, Convert the helper declarations microserviceAuthHeader, randomEmail, and registerUser to const-assigned arrow functions while preserving their existing return types, parameters, and behavior.Source: Coding guidelines
397-409: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a test for a missing or invalid microservice JWT.
The suite proves that a public route ignores
suppressEmail. It does not prove that an unauthenticated request to a/saas/*bridge is rejected. The microservice JWT is the only authorization boundary protecting these endpoints, and a suppressed response returns a raw account-takeover token. Add a case that posts to/saas/user/password/reset/requestwithsuppressEmail: trueand noAuthorizationheader, then asserts a 401 or 403 and the absence ofemailPayload.Do you want me to generate this test?
🤖 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-email-trigger-inversion-e2e.test.ts` around lines 397 - 409, Add a test alongside the existing password-reset inversion test that posts to the /saas/user/password/reset/request bridge with suppressEmail: true and no Authorization header, then assert the response is unauthorized (401 or 403) and does not contain emailPayload. Reuse the existing user setup and request conventions.backend/src/entities/email/email/email.service.ts (1)
197-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
type: stringwith a literal union.
dispatchEmailaccepts any string. A typo in a letter type reaches the SaaS composer at runtime and the webhook rejects it silently, because the gateway swallows non-2xx responses. Define a union of the dispatch catalog and use it here and inSaasEmailGatewayService.sendEmail.♻️ Proposed refactor
+export type DispatchedEmailType = + | 'table_action' + | 'reminder' + | 'company_2fa_enabled' + | 'group_invite' + | 'company_invite' + | 'email_confirmation' + | 'email_changed' + | 'email_change_request' + | 'password_reset_request'; + private async dispatchEmail( - type: string, + type: DispatchedEmailType, to: string, params: Record<string, unknown>, ): Promise<SMTPTransport.SentMessageInfo | null> {🤖 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/entities/email/email/email.service.ts` around lines 197 - 201, Replace the type parameter in EmailService.dispatchEmail with a literal union containing the supported dispatch catalog values, and apply the same union to SaasEmailGatewayService.sendEmail. Reuse a shared type or existing catalog definition so both methods accept only valid email types and reject typos at compile time.
🤖 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/entities/company-info/use-cases/invite-user-in-company.use.case.ts`:
- Around line 109-114: Restrict the two self-hosted confirmation-link log calls
in the invite-user flow, including the nearby call around the company invitation
path, behind an explicit configuration flag that is disabled by default.
Preserve link delivery only when the flag is enabled, and document the required
secure log retention for operators who enable it.
In
`@backend/src/microservices/gateways/saas-gateway.ts/saas-email-gateway.service.ts`:
- Around line 44-57: Update the warning logs in the webhook response handling
and catch block of the email dispatch method to stop interpolating the recipient
variable “to”. Retain the letter type, HTTP status where available, and error
details, while omitting the address or replacing it with a non-reversible
identifier.
In `@backend/src/microservices/saas-microservice/saas.controller.ts`:
- Line 551: Update the Slack notification construction in the account-deletion
handler to stop passing deleteResult.email to Messages.USER_DELETED_ACCOUNT.
Pass the user id or an established non-reversible identifier instead, while
preserving the existing reason and message arguments.
- Around line 550-553: Update the notification flow after
deleteUserAccountUseCase.execute in the account-deletion handler so
slackPostMessage does not remain awaited on the response path. Dispatch the
Slack notification asynchronously with appropriate error isolation, while
returning deleteResult immediately and preserving the committed deletion
behavior.
In
`@backend/src/microservices/saas-microservice/use-cases/saas-usual-register-user.use.case.ts`:
- Around line 86-93: Update the suppressEmail branch in the user registration
use case to include the already-destructured companyId in
registeredUserRO.emailPayload alongside type, to, and rawToken, preserving the
existing return behavior.
In
`@backend/src/selfhosted-operations/application/use-cases/update-user-email-as-admin.use.case.ts`:
- Around line 30-48: Move the findTargetUserInCallerCompany call before
findExistingUsersWithThisEmail in the update flow, so target-user company
membership is validated before querying email uniqueness. Preserve the existing
404 behavior for unauthorized or missing targets and only perform the global
email lookup after a valid targetUser is returned.
- Around line 34-48: Add a database-level unique constraint for UserEntity.email
through a new migration, then update the admin email-update flow around
findTargetUserInCallerCompany and saveUserEntity to catch unique-constraint
violations and translate them into HttpException with
Messages.CANNOT_SET_THIS_EMAIL, while preserving the existing pre-check for
normal conflicts.
In
`@backend/src/selfhosted-operations/application/use-cases/update-user-password-as-admin.use.case.ts`:
- Around line 35-38: Update the password reset flow in the relevant use-case
method to revoke all active sessions for targetUser after saving the new
password and before returning success. Use the existing user-wide session
revocation mechanism rather than exact-token logout, preserving the successful
response only after revocation completes.
In `@backend/test/ava-tests/saas-tests/saas-email-trigger-inversion-e2e.test.ts`:
- Around line 39-44: Replace the any-based responseBody type in registerUser
with a specific shape containing the fields accessed by these tests, or use
Record<string, unknown> and narrow values at each access site. Preserve the
existing registerUser return contract while removing all explicit any usage.
In `@backend/test/ava-tests/saas-tests/saas-user-account-bridges-e2e.test.ts`:
- Around line 137-143: Update the test.after teardown handler to rethrow the
caught cleanup error after logging it, so failures from Cacher.clearAllCache()
or app.close() propagate to AVA. Preserve the existing cleanup sequence and
change the concatenated log message to use a template literal.
---
Outside diff comments:
In
`@backend/src/entities/email/repository/email-verification-custom-repository-extension.ts`:
- Around line 26-38: The verification replacement flow around findOne, remove,
and save is not atomic and can race on the userId unique constraint. Update the
repository method to use an atomic upsert keyed by the user id, or serialize
replacement per user and handle unique-conflict retries, while preserving
generation and return of the new verification token.
---
Nitpick comments:
In `@backend/src/entities/email/email/email.service.ts`:
- Around line 197-201: Replace the type parameter in EmailService.dispatchEmail
with a literal union containing the supported dispatch catalog values, and apply
the same union to SaasEmailGatewayService.sendEmail. Reuse a shared type or
existing catalog definition so both methods accept only valid email types and
reject typos at compile time.
In `@backend/src/microservices/agents-microservice/dto/agents-auth.dtos.ts`:
- Around line 14-17: Update the Swagger description in the agents-auth DTO to
use a single template literal instead of concatenated string literals,
preserving the existing text and formatting.
In
`@backend/src/selfhosted-operations/application/use-cases/update-user-email-as-admin.use.case.ts`:
- Around line 52-56: Annotate the findTargetUserInCallerCompany method with the
required Promise<UserEntity> return type, leaving its parameters and
implementation unchanged.
In
`@backend/src/selfhosted-operations/application/use-cases/update-user-password-as-admin.use.case.ts`:
- Around line 41-45: Update the findTargetUserInCallerCompany method signature
to explicitly declare Promise<UserEntity> as its return type, while preserving
its existing parameters and implementation.
- Around line 41-53: Extract the duplicated findTargetUserInCallerCompany logic
into one shared helper or base class, giving it an explicit Promise<UserEntity>
return type and using the helper’s existing database context instead of passing
this._dbContext as an argument. Replace the private helper and its call sites in
backend/src/selfhosted-operations/application/use-cases/update-user-password-as-admin.use.case.ts#L41-L53
and
backend/src/selfhosted-operations/application/use-cases/update-user-email-as-admin.use.case.ts#L52-L64
with the shared implementation; both sites require the same change.
In `@backend/src/selfhosted-operations/selhosted-operations.module.ts`:
- Around line 76-81: Eliminate duplicated authentication route literals by
introducing shared path constants and using them in both
SelfHostedOperationsModule.configure() at
backend/src/selfhosted-operations/selhosted-operations.module.ts:76-81 and
SelfHostedOperationsController at
backend/src/selfhosted-operations/selfhosted-operations.controller.ts:67. Update
the AuthMiddleware route targets and the controller’s `@Put` decorators to consume
those constants, preserving the existing PUT endpoints and controller prefix.
In
`@backend/test/ava-tests/non-saas-tests/non-saas-selfhosted-user-admin-e2e.test.ts`:
- Around line 78-85: Update getUserProfile to validate the /user/ response
status before calling JSON.parse, matching the status-check pattern used by the
other helpers in this file; preserve parsing only for successful responses and
report failures through the existing test error path.
- Around line 291-322: Add a serial cross-company test alongside the existing
email-update tests, using the foreign-admin setup pattern from the password
endpoint test and targeting PUT /selfhosted/users/:userId/email with an
already-used email. Assert that the response status is 404, ensuring the
foreign-user check occurs before the email uniqueness validation.
In `@backend/test/ava-tests/saas-tests/saas-email-trigger-inversion-e2e.test.ts`:
- Around line 30-37: Convert the helper declarations microserviceAuthHeader,
randomEmail, and registerUser to const-assigned arrow functions while preserving
their existing return types, parameters, and behavior.
- Around line 397-409: Add a test alongside the existing password-reset
inversion test that posts to the /saas/user/password/reset/request bridge with
suppressEmail: true and no Authorization header, then assert the response is
unauthorized (401 or 403) and does not contain emailPayload. Reuse the existing
user setup and request conventions.
In `@backend/test/ava-tests/saas-tests/saas-user-account-bridges-e2e.test.ts`:
- Around line 40-43: Convert the six test helpers to arrow-function assignments:
microserviceAuthHeader at
backend/test/ava-tests/saas-tests/saas-user-account-bridges-e2e.test.ts:40-43,
registerUser at :45-65, activateUser at :67-73, getProfile at :75-80,
enrollUserIntoOtp at :83-107, and mintTemporaryToken at :111-114. Preserve each
helper’s existing parameters, return values, and behavior.
🪄 Autofix
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 Plus
Run ID: 8929ac40-3666-439e-ad55-6ffb72641ac7
📒 Files selected for processing (45)
backend/src/common/data-injection.tokens.tsbackend/src/entities/company-info/application/data-structures/invite-user-in-company-and-connection-group.ds.tsbackend/src/entities/company-info/application/data-structures/invited-user-in-company-and-connection-group.ds.tsbackend/src/entities/company-info/use-cases/invite-user-in-company.use.case.tsbackend/src/entities/email/application/data-structures/outgoing-email-payload.ds.tsbackend/src/entities/email/email/email.service.tsbackend/src/entities/email/repository/email-verification-custom-repository-extension.tsbackend/src/entities/user/application/data-structures/change-user-email.ds.tsbackend/src/entities/user/application/data-structures/operation-result-message.ds.tsbackend/src/entities/user/application/data-structures/request-email-change.ds.tsbackend/src/entities/user/application/data-structures/request-password-reset.ds.tsbackend/src/entities/user/application/data-structures/usual-register-user.ds.tsbackend/src/entities/user/use-cases/request-change-user-email.use.case.tsbackend/src/entities/user/use-cases/request-email-verification.use.case.tsbackend/src/entities/user/use-cases/request-reset-user-password.use.case.tsbackend/src/entities/user/use-cases/user-use-cases.interfaces.tsbackend/src/entities/user/use-cases/verify-change-user-email.use.case.tsbackend/src/entities/user/user.controller.tsbackend/src/exceptions/text/messages.tsbackend/src/microservices/agents-microservice/agents.controller.tsbackend/src/microservices/agents-microservice/data-structures/agents.ds.tsbackend/src/microservices/agents-microservice/dto/agents-auth.dtos.tsbackend/src/microservices/agents-microservice/use-cases/agents-use-cases.interface.tsbackend/src/microservices/agents-microservice/use-cases/validate-user-token.use.case.tsbackend/src/microservices/gateways/saas-gateway.ts/saas-email-gateway.service.tsbackend/src/microservices/gateways/saas-gateway.ts/saas-gateway.module.tsbackend/src/microservices/saas-microservice/data-structures/saas-email-flows.dtos.tsbackend/src/microservices/saas-microservice/data-structures/saas-otp-login.ds.tsbackend/src/microservices/saas-microservice/data-structures/saas-user-account.dtos.tsbackend/src/microservices/saas-microservice/saas.controller.tsbackend/src/microservices/saas-microservice/saas.module.tsbackend/src/microservices/saas-microservice/use-cases/saas-otp-login.use.case.tsbackend/src/microservices/saas-microservice/use-cases/saas-use-cases.interface.tsbackend/src/microservices/saas-microservice/use-cases/saas-usual-register-user.use.case.tsbackend/src/selfhosted-operations/application/data-structures/update-user-as-admin.ds.tsbackend/src/selfhosted-operations/application/dto/update-user-email-as-admin.dto.tsbackend/src/selfhosted-operations/application/dto/update-user-password-as-admin.dto.tsbackend/src/selfhosted-operations/application/use-cases/selfhosted-use-cases.interfaces.tsbackend/src/selfhosted-operations/application/use-cases/update-user-email-as-admin.use.case.tsbackend/src/selfhosted-operations/application/use-cases/update-user-password-as-admin.use.case.tsbackend/src/selfhosted-operations/selfhosted-operations.controller.tsbackend/src/selfhosted-operations/selhosted-operations.module.tsbackend/test/ava-tests/non-saas-tests/non-saas-selfhosted-user-admin-e2e.test.tsbackend/test/ava-tests/saas-tests/saas-email-trigger-inversion-e2e.test.tsbackend/test/ava-tests/saas-tests/saas-user-account-bridges-e2e.test.ts
💤 Files with no reviewable changes (1)
- backend/src/exceptions/text/messages.ts
Comment on lines
+109
to
114
| } else { | ||
| // Plan 15 Phase 6 (rev 5): self-hosted sends no email — the admin takes the full | ||
| // confirmation link from the server logs and hands it to the user. | ||
| this.logger.printTechString( | ||
| `Email confirmation link: ${Constants.APP_DOMAIN_ADDRESS}/external/user/email/verify/${rawToken}`, | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Raw single-use tokens are written to the application logs in self-hosted mode.
Both printTechString calls embed the raw verification token in a complete link. The token grants email confirmation or company-invitation acceptance. Log files are usually retained, rotated, and shipped to external transports, so the exposure is wider than the local administrator console.
The comments state this is the intended self-hosted delivery mechanism. If that decision stands, restrict the log level for these two lines and document the retention requirement. Consider gating the output behind an explicit configuration flag so operators who ship logs off-host can disable it.
Also applies to: 164-170
🤖 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/entities/company-info/use-cases/invite-user-in-company.use.case.ts`
around lines 109 - 114, Restrict the two self-hosted confirmation-link log calls
in the invite-user flow, including the nearby call around the company invitation
path, behind an explicit configuration flag that is disabled by default.
Preserve link delivery only when the flag is enabled, and document the required
secure log retention for operators who enable it.
Comment on lines
+44
to
+57
| if (res.status > 299) { | ||
| this.logger.warn(`Email webhook rejected "${type}" letter to "${to}": status ${res.status}`); | ||
| return null; | ||
| } | ||
| const body = await this.bodyToJSON(res); | ||
| return { | ||
| messageId: typeof body.messageId === 'string' ? body.messageId : undefined, | ||
| accepted: Array.isArray(body.accepted) ? (body.accepted as Array<string>) : [], | ||
| rejected: Array.isArray(body.rejected) ? (body.rejected as Array<string>) : [], | ||
| }; | ||
| } catch (error) { | ||
| this.logger.warn(`Email webhook dispatch of "${type}" letter to "${to}" failed: ${getErrorMessage(error)}`); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not write the recipient address to the logs.
Both warn calls interpolate to, which is a user email address. Every webhook failure then persists a user identifier in the application logs. Log the letter type and the status, and identify the recipient by a non-reversible value or omit it.
🛡️ Proposed fix to remove the address from the log lines
if (res.status > 299) {
- this.logger.warn(`Email webhook rejected "${type}" letter to "${to}": status ${res.status}`);
+ this.logger.warn(`Email webhook rejected "${type}" letter: status ${res.status}`);
return null;
}
@@
} catch (error) {
- this.logger.warn(`Email webhook dispatch of "${type}" letter to "${to}" failed: ${getErrorMessage(error)}`);
+ this.logger.warn(`Email webhook dispatch of "${type}" letter failed: ${getErrorMessage(error)}`);
return null;
}📝 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
| if (res.status > 299) { | |
| this.logger.warn(`Email webhook rejected "${type}" letter to "${to}": status ${res.status}`); | |
| return null; | |
| } | |
| const body = await this.bodyToJSON(res); | |
| return { | |
| messageId: typeof body.messageId === 'string' ? body.messageId : undefined, | |
| accepted: Array.isArray(body.accepted) ? (body.accepted as Array<string>) : [], | |
| rejected: Array.isArray(body.rejected) ? (body.rejected as Array<string>) : [], | |
| }; | |
| } catch (error) { | |
| this.logger.warn(`Email webhook dispatch of "${type}" letter to "${to}" failed: ${getErrorMessage(error)}`); | |
| return null; | |
| } | |
| if (res.status > 299) { | |
| this.logger.warn(`Email webhook rejected "${type}" letter: status ${res.status}`); | |
| return null; | |
| } | |
| const body = await this.bodyToJSON(res); | |
| return { | |
| messageId: typeof body.messageId === 'string' ? body.messageId : undefined, | |
| accepted: Array.isArray(body.accepted) ? (body.accepted as Array<string>) : [], | |
| rejected: Array.isArray(body.rejected) ? (body.rejected as Array<string>) : [], | |
| }; | |
| } catch (error) { | |
| this.logger.warn(`Email webhook dispatch of "${type}" letter failed: ${getErrorMessage(error)}`); | |
| return null; | |
| } |
🤖 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/gateways/saas-gateway.ts/saas-email-gateway.service.ts`
around lines 44 - 57, Update the warning logs in the webhook response handling
and catch block of the email dispatch method to stop interpolating the recipient
variable “to”. Retain the letter type, HTTP status where available, and error
details, while omitting the address or replacing it with a non-reversible
identifier.
Comment on lines
+550
to
+553
| const deleteResult = await this.deleteUserAccountUseCase.execute(body.userId, InTransactionEnum.ON); | ||
| const slackMessage = Messages.USER_DELETED_ACCOUNT(deleteResult.email, body.reason ?? '', body.message ?? ''); | ||
| await slackPostMessage(slackMessage); | ||
| return deleteResult; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
An awaited Slack call after a committed deletion can fail the request.
deleteUserAccountUseCase.execute commits the deletion. slackPostMessage then runs on the request thread with no try/catch and no timeout visible at this call site. If Slack is slow or returns an error, this endpoint returns a failure for an operation that already succeeded. The SaaS caller can then retry a delete for a user that no longer exists.
Detach the notification from the response path.
🛡️ Proposed fix
const deleteResult = await this.deleteUserAccountUseCase.execute(body.userId, InTransactionEnum.ON);
const slackMessage = Messages.USER_DELETED_ACCOUNT(deleteResult.email, body.reason ?? '', body.message ?? '');
- await slackPostMessage(slackMessage);
+ void slackPostMessage(slackMessage).catch(() => undefined);
return deleteResult;📝 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
| const deleteResult = await this.deleteUserAccountUseCase.execute(body.userId, InTransactionEnum.ON); | |
| const slackMessage = Messages.USER_DELETED_ACCOUNT(deleteResult.email, body.reason ?? '', body.message ?? ''); | |
| await slackPostMessage(slackMessage); | |
| return deleteResult; | |
| const deleteResult = await this.deleteUserAccountUseCase.execute(body.userId, InTransactionEnum.ON); | |
| const slackMessage = Messages.USER_DELETED_ACCOUNT(deleteResult.email, body.reason ?? '', body.message ?? ''); | |
| void slackPostMessage(slackMessage).catch(() => undefined); | |
| return deleteResult; |
🤖 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
550 - 553, Update the notification flow after deleteUserAccountUseCase.execute
in the account-deletion handler so slackPostMessage does not remain awaited on
the response path. Dispatch the Slack notification asynchronously with
appropriate error isolation, while returning deleteResult immediately and
preserving the committed deletion behavior.
| // When the deleted user is the last one in the company, the use case calls back into the SaaS | ||
| // service (`saasCompanyGatewayService.deleteCompany`) — plain sequential HTTP, no deadlock. | ||
| const deleteResult = await this.deleteUserAccountUseCase.execute(body.userId, InTransactionEnum.ON); | ||
| const slackMessage = Messages.USER_DELETED_ACCOUNT(deleteResult.email, body.reason ?? '', body.message ?? ''); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
The deleted user's email address is sent to Slack.
Messages.USER_DELETED_ACCOUNT receives deleteResult.email. Account deletion is the point at which a user most often exercises an erasure right, and this call copies the identifier to a third-party service that the deletion cannot reach. Send a non-reversible identifier or the user id instead of the address.
🤖 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` at line 551,
Update the Slack notification construction in the account-deletion handler to
stop passing deleteResult.email to Messages.USER_DELETED_ACCOUNT. Pass the user
id or an established non-reversible identifier instead, while preserving the
existing reason and message arguments.
Comment on lines
+86
to
+93
| if (suppressEmail) { | ||
| registeredUserRO.emailPayload = { | ||
| type: 'email_confirmation', | ||
| to: savedUser.email, | ||
| rawToken, | ||
| }; | ||
| return registeredUserRO; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Add companyId to the suppressed registration payload.
Every other suppressed email_confirmation payload carries companyId. RequestEmailVerificationUseCase sets companyId: foundUserCompany.id, and the invite flow sets companyId on its re-confirmation payload. This branch omits it, so the SaaS composer receives inconsistent context for the same letter type. companyId is already destructured in this method.
🐛 Proposed fix
if (suppressEmail) {
registeredUserRO.emailPayload = {
type: 'email_confirmation',
to: savedUser.email,
rawToken,
+ companyId,
};
return registeredUserRO;
}📝 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
| if (suppressEmail) { | |
| registeredUserRO.emailPayload = { | |
| type: 'email_confirmation', | |
| to: savedUser.email, | |
| rawToken, | |
| }; | |
| return registeredUserRO; | |
| } | |
| if (suppressEmail) { | |
| registeredUserRO.emailPayload = { | |
| type: 'email_confirmation', | |
| to: savedUser.email, | |
| rawToken, | |
| companyId, | |
| }; | |
| return registeredUserRO; | |
| } |
🤖 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/use-cases/saas-usual-register-user.use.case.ts`
around lines 86 - 93, Update the suppressEmail branch in the user registration
use case to include the already-destructured companyId in
registeredUserRO.emailPayload alongside type, to, and rawToken, preserving the
existing return behavior.
Comment on lines
+30
to
+48
| const { callerUserId, targetUserId } = inputData; | ||
| const newEmail = inputData.newEmail.toLowerCase(); | ||
| ValidationHelper.validateOrThrowHttpExceptionEmail(newEmail); | ||
|
|
||
| const foundExistingUsersWithThisEmail = await this._dbContext.userRepository.find({ | ||
| where: { email: newEmail }, | ||
| }); | ||
| if (foundExistingUsersWithThisEmail.length > 0) { | ||
| throw new HttpException( | ||
| { | ||
| message: Messages.CANNOT_SET_THIS_EMAIL, | ||
| }, | ||
| HttpStatus.BAD_REQUEST, | ||
| ); | ||
| } | ||
|
|
||
| const targetUser = await this.findTargetUserInCallerCompany(this._dbContext, callerUserId, targetUserId); | ||
| targetUser.email = newEmail; | ||
| await this._dbContext.userRepository.saveUserEntity(targetUser); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Perform the company membership check before the global email lookup.
The use case queries all users for newEmail before it verifies that targetUserId belongs to the caller's company. The two failure paths return different results: 400 CANNOT_SET_THIS_EMAIL when the email exists anywhere in the deployment, and 404 USER_NOT_FOUND when it does not. A company admin can therefore call the endpoint with an arbitrary targetUserId and learn whether any given email address is registered, including addresses in other companies. Move findTargetUserInCallerCompany above the uniqueness query so that unauthorized callers always receive 404.
🔒 Proposed fix to check authorization first
const { callerUserId, targetUserId } = inputData;
const newEmail = inputData.newEmail.toLowerCase();
ValidationHelper.validateOrThrowHttpExceptionEmail(newEmail);
+ const targetUser = await this.findTargetUserInCallerCompany(this._dbContext, callerUserId, targetUserId);
+
const foundExistingUsersWithThisEmail = await this._dbContext.userRepository.find({
where: { email: newEmail },
});
if (foundExistingUsersWithThisEmail.length > 0) {
throw new HttpException(
{
message: Messages.CANNOT_SET_THIS_EMAIL,
},
HttpStatus.BAD_REQUEST,
);
}
- const targetUser = await this.findTargetUserInCallerCompany(this._dbContext, callerUserId, targetUserId);
targetUser.email = newEmail;📝 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
| const { callerUserId, targetUserId } = inputData; | |
| const newEmail = inputData.newEmail.toLowerCase(); | |
| ValidationHelper.validateOrThrowHttpExceptionEmail(newEmail); | |
| const foundExistingUsersWithThisEmail = await this._dbContext.userRepository.find({ | |
| where: { email: newEmail }, | |
| }); | |
| if (foundExistingUsersWithThisEmail.length > 0) { | |
| throw new HttpException( | |
| { | |
| message: Messages.CANNOT_SET_THIS_EMAIL, | |
| }, | |
| HttpStatus.BAD_REQUEST, | |
| ); | |
| } | |
| const targetUser = await this.findTargetUserInCallerCompany(this._dbContext, callerUserId, targetUserId); | |
| targetUser.email = newEmail; | |
| await this._dbContext.userRepository.saveUserEntity(targetUser); | |
| const { callerUserId, targetUserId } = inputData; | |
| const newEmail = inputData.newEmail.toLowerCase(); | |
| ValidationHelper.validateOrThrowHttpExceptionEmail(newEmail); | |
| const targetUser = await this.findTargetUserInCallerCompany(this._dbContext, callerUserId, targetUserId); | |
| const foundExistingUsersWithThisEmail = await this._dbContext.userRepository.find({ | |
| where: { email: newEmail }, | |
| }); | |
| if (foundExistingUsersWithThisEmail.length > 0) { | |
| throw new HttpException( | |
| { | |
| message: Messages.CANNOT_SET_THIS_EMAIL, | |
| }, | |
| HttpStatus.BAD_REQUEST, | |
| ); | |
| } | |
| targetUser.email = newEmail; | |
| await this._dbContext.userRepository.saveUserEntity(targetUser); |
🤖 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/selfhosted-operations/application/use-cases/update-user-email-as-admin.use.case.ts`
around lines 30 - 48, Move the findTargetUserInCallerCompany call before
findExistingUsersWithThisEmail in the update flow, so target-user company
membership is validated before querying email uniqueness. Preserve the existing
404 behavior for unauthorized or missing targets and only perform the global
email lookup after a valid targetUser is returned.
Comment on lines
+34
to
+48
| const foundExistingUsersWithThisEmail = await this._dbContext.userRepository.find({ | ||
| where: { email: newEmail }, | ||
| }); | ||
| if (foundExistingUsersWithThisEmail.length > 0) { | ||
| throw new HttpException( | ||
| { | ||
| message: Messages.CANNOT_SET_THIS_EMAIL, | ||
| }, | ||
| HttpStatus.BAD_REQUEST, | ||
| ); | ||
| } | ||
|
|
||
| const targetUser = await this.findTargetUserInCallerCompany(this._dbContext, callerUserId, targetUserId); | ||
| targetUser.email = newEmail; | ||
| await this._dbContext.userRepository.saveUserEntity(targetUser); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the user entity email column and any unique index on it.
fd -i 'user.entity.ts' backend/src | xargs -r rg -n -C6 'email'
# Look for unique constraints on user email in migrations.
rg -n -i --glob '**/migration*/**' -C3 'user.*email|UNIQUE.*email'Repository: rocket-admin/rocketadmin
Length of output: 3762
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- user entity indexes and email declarations ---'
user_entity="$(fd -i -t f 'user.entity.ts' backend/src | head -n 1 || true)"
if [ -n "$user_entity" ]; then
rg -n -C8 '(`@Index`|`@Column`|email)' "$user_entity"
fi
echo '--- migration files ---'
fd -i -t f 'migration' . | head -n 200
echo '--- email-related schema operations ---'
rg -n -i -C4 --glob '*.ts' --glob '*.sql' \
'createIndex|dropIndex|unique|user.*email|email.*user|CREATE TABLE.*user|ALTER TABLE.*user' \
backend 2>/dev/null | head -n 300
echo '--- TypeORM schema configuration ---'
rg -n -i -C4 --glob '*.ts' --glob '*.js' --glob '*.json' \
'synchronize|migrations|typeorm|postgres|mysql|sqlite' \
backend 2>/dev/null | head -n 300Repository: rocket-admin/rocketadmin
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- user table creation ---'
rg -n -A3 -B3 'CREATE TABLE "user"' backend/src/migrations
echo '--- migrations that alter user.email or add user indexes ---'
rg -n -i -C3 \
'ALTER TABLE "user".*(email|INDEX|CONSTRAINT)|CREATE( UNIQUE)? INDEX.*(user|email)|ADD CONSTRAINT.*(user|email)|UNIQUE.*email' \
backend/src/migrations || true
echo '--- datasource schema settings ---'
datasource="$(fd -i -t f 'datasource.config.ts' backend/src | head -n 1 || true)"
if [ -n "$datasource" ]; then
cat -n "$datasource"
fi
echo '--- transaction mode at the use-case call site ---'
rg -n -C8 \
'update-user-email-as-admin|InTransactionEnum\.OFF|implementation\(.*InTransactionEnum' \
backend/src | head -n 160Repository: rocket-admin/rocketadmin
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- all schema declarations containing email and uniqueness ---'
rg -n -i --glob '!backend/test/**' \
'email|unique|create.*index|add.*constraint' . \
| rg -i 'user|email|unique|index' \
| head -n 250
echo '--- datasource transaction and schema options ---'
datasource="$(fd -i -t f 'datasource.config.ts' backend/src | head -n 1 || true)"
if [ -n "$datasource" ]; then
rg -n -C5 'synchronize|migrations|transaction|DataSource|TypeOrmModule' "$datasource"
fi
echo '--- update-email controller and use-case invocation ---'
rg -n -C12 \
'update-user-email-as-admin|InTransactionEnum\.OFF|UpdateUserEmailAsAdmin' \
backend/src | head -n 220Repository: rocket-admin/rocketadmin
Length of output: 50381
Add a database-level unique constraint for user.email. UserEntity.email and the migrations do not define one. With InTransactionEnum.OFF, concurrent requests can pass the read and save duplicate email addresses. Add a unique migration and map constraint violations to Messages.CANNOT_SET_THIS_EMAIL.
🤖 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/selfhosted-operations/application/use-cases/update-user-email-as-admin.use.case.ts`
around lines 34 - 48, Add a database-level unique constraint for
UserEntity.email through a new migration, then update the admin email-update
flow around findTargetUserInCallerCompany and saveUserEntity to catch
unique-constraint violations and translate them into HttpException with
Messages.CANNOT_SET_THIS_EMAIL, while preserving the existing pre-check for
normal conflicts.
Comment on lines
+35
to
+38
| const targetUser = await this.findTargetUserInCallerCompany(this._dbContext, callerUserId, targetUserId); | ||
| targetUser.password = await Encryptor.hashUserPassword(newPassword); | ||
| await this._dbContext.userRepository.saveUserEntity(targetUser); | ||
| return { success: true }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the logout entity and the use case that revokes user sessions.
fd -i 'log-out' backend/src --exec rg -n -C4 'class|export'
# Find how other flows revoke sessions after credential changes.
rg -n -C4 'LOG_OUT|logOutRepository|saveLogOutEntity' backend/srcRepository: rocket-admin/rocketadmin
Length of output: 19522
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target use case ---'
sed -n '1,140p' backend/src/selfhosted-operations/application/use-cases/update-user-password-as-admin.use.case.ts
printf '%s\n' '--- logout use case and entity/repository ---'
sed -n '1,120p' backend/src/entities/user/use-cases/log-out.use.case.ts
sed -n '1,100p' backend/src/entities/log-out/log-out.entity.ts
sed -n '1,120p' backend/src/entities/log-out/repository/log-out-custom-repository-extension.ts
sed -n '1,80p' backend/src/entities/log-out/repository/log-out-repository.interface.ts
printf '%s\n' '--- token creation and password-change flows ---'
rg -n -C5 'signAsync|sign\\(|jwtToken|saveLogOutUserToken|CHANGE_USUAL_PASSWORD|hashUserPassword' backend/src \
-g '*.ts'Repository: rocket-admin/rocketadmin
Length of output: 5516
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target use case ---'
sed -n '1,140p' backend/src/selfhosted-operations/application/use-cases/update-user-password-as-admin.use.case.ts
printf '%s\n' '--- logout use case and entity/repository ---'
sed -n '1,120p' backend/src/entities/user/use-cases/log-out.use.case.ts
sed -n '1,100p' backend/src/entities/log-out/log-out.entity.ts
sed -n '1,120p' backend/src/entities/log-out/repository/log-out-custom-repository-extension.ts
sed -n '1,80p' backend/src/entities/log-out/repository/log-out-repository.interface.ts
printf '%s\n' '--- token creation and password-change flows ---'
rg -n -C5 'signAsync|sign\(|jwtToken|saveLogOutUserToken|CHANGE_USUAL_PASSWORD|hashUserPassword' backend/src -g '*.ts'Repository: rocket-admin/rocketadmin
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- JWT generation ---'
rg -l 'generateGwtToken|generateJwtToken|generate.*Token' backend/src -g '*.ts' |
while IFS= read -r file; do
printf '\n### %s\n' "$file"
rg -n -C8 'generateGwtToken|generateJwtToken|generate.*Token|JwtService|jwt\\.sign' "$file"
done
printf '%s\n' '--- authentication validation ---'
sed -n '1,130p' backend/src/authorization/auth.middleware.ts
sed -n '1,110p' backend/src/authorization/non-scoped-auth.middleware.ts
sed -n '1,100p' backend/src/authorization/temporary-auth.middleware.ts
printf '%s\n' '--- token lifetime and cookie handling ---'
rg -n -C3 'JWT_EXPIRES|expiresIn|jwt.*expiration|cookie.*token|setHeader.*cookie|res\\.cookie|access_token|authorization' backend/src -g '*.ts' |
head -n 260
printf '%s\n' '--- password/session coupling ---'
rg -n -C3 'password.*(token|session|version)|token.*password|session.*version|passwordVersion|passwordChanged' backend/src -g '*.ts'Repository: rocket-admin/rocketadmin
Length of output: 49301
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
root = Path("backend/src")
terms = (
"generateGwtToken",
"generateJwtToken",
"passwordVersion",
"passwordChanged",
"sessionVersion",
)
for path in root.rglob("*.ts"):
text = path.read_text(errors="replace")
if any(term in text for term in terms):
print(path)
for i, line in enumerate(text.splitlines(), 1):
if any(term in line for term in terms):
print(f"{i}: {line.strip()}")
PYRepository: rocket-admin/rocketadmin
Length of output: 2028
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- JWT generation ---'
rg -l 'generateGwtToken|generateJwtToken|generate.*Token' backend/src -g '*.ts' |
while IFS= read -r file; do
printf '\n### %s\n' "$file"
rg -n -C8 'generateGwtToken|generateJwtToken|generate.*Token|JwtService|jwt\.sign' "$file"
done
printf '%s\n' '--- authentication validation ---'
sed -n '1,130p' backend/src/authorization/auth.middleware.ts
sed -n '1,110p' backend/src/authorization/non-scoped-auth.middleware.ts
sed -n '1,100p' backend/src/authorization/temporary-auth.middleware.ts
printf '%s\n' '--- token lifetime and cookie handling ---'
rg -n -C3 'JWT_EXPIRES|expiresIn|jwt.*expiration|cookie.*token|setHeader.*cookie|res\.cookie|access_token|authorization' backend/src -g '*.ts' |
head -n 260
printf '%s\n' '--- password/session coupling ---'
rg -n -C3 'password.*(token|session|version)|token.*password|session.*version|passwordVersion|passwordChanged' backend/src -g '*.ts' |
head -n 200Repository: rocket-admin/rocketadmin
Length of output: 49148
Invalidate the target user's active sessions after the password reset.
The seven-day JWT remains valid after its password hash changes. The logout table revokes only exact token strings, not all tokens for a user. Add user-wide session revocation before returning success.
🤖 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/selfhosted-operations/application/use-cases/update-user-password-as-admin.use.case.ts`
around lines 35 - 38, Update the password reset flow in the relevant use-case
method to revoke all active sessions for targetUser after saving the new
password and before returning success. Use the existing user-wide session
revocation mechanism rather than exact-token logout, preserving the successful
response only after revocation completes.
Comment on lines
+39
to
+44
| async function registerUser(suppressEmail = false): Promise<{ | ||
| userId: string; | ||
| email: string; | ||
| companyId: string; | ||
| responseBody: Record<string, any>; | ||
| }> { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace Record<string, any> with a typed shape.
The coding guidelines prohibit any. Declare the response fields the tests read, or use Record<string, unknown> with narrowing at the access sites.
♻️ Proposed change
+type EmailPayloadRO = {
+ type: string;
+ to: string;
+ rawToken?: string;
+ companyId?: string;
+ companyName?: string | null;
+};
+
+type RegisterRO = {
+ id: string;
+ email: string;
+ emailPayload?: EmailPayloadRO;
+};
+
async function registerUser(suppressEmail = false): Promise<{
userId: string;
email: string;
companyId: string;
- responseBody: Record<string, any>;
+ responseBody: RegisterRO;
}> {As per coding guidelines: "Avoid any types - use specific types or generics instead".
📝 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 function registerUser(suppressEmail = false): Promise<{ | |
| userId: string; | |
| email: string; | |
| companyId: string; | |
| responseBody: Record<string, any>; | |
| }> { | |
| type EmailPayloadRO = { | |
| type: string; | |
| to: string; | |
| rawToken?: string; | |
| companyId?: string; | |
| companyName?: string | null; | |
| }; | |
| type RegisterRO = { | |
| id: string; | |
| email: string; | |
| emailPayload?: EmailPayloadRO; | |
| }; | |
| async function registerUser(suppressEmail = false): Promise<{ | |
| userId: string; | |
| email: string; | |
| companyId: string; | |
| responseBody: RegisterRO; | |
| }> { |
🤖 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-email-trigger-inversion-e2e.test.ts`
around lines 39 - 44, Replace the any-based responseBody type in registerUser
with a specific shape containing the fields accessed by these tests, or use
Record<string, unknown> and narrow values at each access site. Preserve the
existing registerUser return contract while removing all explicit any usage.
Source: Coding guidelines
Comment on lines
+137
to
+143
| test.after(async () => { | ||
| try { | ||
| await Cacher.clearAllCache(); | ||
| await app.close(); | ||
| } catch (e) { | ||
| console.error('After tests error ' + e); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Rethrow teardown failures.
The catch block logs cleanup errors and then completes successfully. AVA can report a passing teardown after Cacher.clearAllCache() or app.close() fails. Log the error and rethrow it.
Proposed fix
} catch (e) {
- console.error('After tests error ' + e);
+ console.error(`After tests error ${String(e)}`);
+ throw e;
}As per coding guidelines, "Ensure all error handling is explicit - use try/catch blocks appropriately" and "Use template literals instead of string concatenation."
📝 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
| test.after(async () => { | |
| try { | |
| await Cacher.clearAllCache(); | |
| await app.close(); | |
| } catch (e) { | |
| console.error('After tests error ' + e); | |
| } | |
| test.after(async () => { | |
| try { | |
| await Cacher.clearAllCache(); | |
| await app.close(); | |
| } catch (e) { | |
| console.error(`After tests error ${String(e)}`); | |
| throw 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-account-bridges-e2e.test.ts`
around lines 137 - 143, Update the test.after teardown handler to rethrow the
caught cleanup error after logging it, so failures from Cacher.clearAllCache()
or app.close() propagate to AVA. Preserve the existing cleanup sequence and
change the concatenated log message to use a template literal.
Source: Coding guidelines
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.
saas-email-trigger-inversion-e2e.test.ts, covering user registration, email verification, password reset requests, and email change requests with and without the suppressEmail flag.saas-user-account-bridges-e2e.test.ts, including user profile retrieval, password changes, name updates, settings management, 2FA enrollment and verification, and account deletion.Summary by CodeRabbit