feat: implement MTA hooks for quota enforcement#82
Open
jzunigax2 wants to merge 2 commits into
Open
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds an MTA Hooks feature with a guarded NestJS RCPT endpoint that resolves recipients, checks Bridge mailbox usage against quota, and returns accept or SMTP rejection responses. It also adds protocol types, configuration, Bridge usage access, application wiring, and tests. ChangesMTA Hooks feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Stalwart
participant MtaHooksController
participant MtaHooksAuthGuard
participant MtaHooksService
participant AccountService
participant BridgeClient
Stalwart->>MtaHooksController: POST /mta-hooks/rcpt
MtaHooksController->>MtaHooksAuthGuard: canActivate(request)
MtaHooksAuthGuard-->>MtaHooksController: allowed or UnauthorizedException
MtaHooksController->>MtaHooksService: handleRcpt(request)
MtaHooksService->>AccountService: findUserIdByAddress(recipient)
AccountService-->>MtaHooksService: userUuid
MtaHooksService->>BridgeClient: getUserUsage(userUuid)
BridgeClient-->>MtaHooksService: usage and quota
MtaHooksService-->>MtaHooksController: accept or reject response
MtaHooksController-->>Stalwart: MtaHookResponse
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.env.template (1)
33-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuedotenv-linter flags key ordering.
MTA_HOOKS_SECRETshould appear beforeMTA_HOOKS_USERNAMEper the linter's alphabetical ordering rule.🔧 Fix key ordering
# MTA hooks (RCPT) -MTA_HOOKS_USERNAME=stalwart MTA_HOOKS_SECRET= +MTA_HOOKS_USERNAME=stalwart🤖 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 @.env.template around lines 33 - 36, The dotenv linter is flagging the MTA hooks entries for incorrect alphabetical ordering. Reorder the variables in the .env.template section so MTA_HOOKS_SECRET appears before MTA_HOOKS_USERNAME, keeping the existing values unchanged and preserving the surrounding comment and section structure.Source: Linters/SAST tools
src/modules/mta-hooks/mta-hooks-auth.guard.spec.ts (1)
65-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test case for empty-secret credentials.
This would guard against the empty-secret bypass (see comment on the guard). When the configured secret is non-empty, sending
stalwart:(empty password) should reject.♻️ Suggested additional test
+ it('when the secret is empty, then throws Unauthorized', () => { + const context = contextWithAuth(basic(username, '')); + + expect(() => guard.canActivate(context)).toThrow(UnauthorizedException); + });🤖 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 `@src/modules/mta-hooks/mta-hooks-auth.guard.spec.ts` around lines 65 - 71, Add a spec in mta-hooks-auth.guard.spec for the empty-secret bypass case by extending the existing Basic auth parsing tests around guard.canActivate and contextWithAuth. Verify that when the configured secret is non-empty, a Basic credential decoding to a username with an empty password such as stalwart: is rejected with UnauthorizedException, alongside the existing no-colon test. Use the guard’s credential validation path in AuthGuard to locate the behavior being exercised.
🤖 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 `@src/config/configuration.ts`:
- Around line 57-60: The MTA hooks Basic auth check in configuration handling
currently allows an empty secret, which can let `stalwart:` pass as valid
credentials. Update the logic around `mtaHooks` in `configuration.ts` and the
related startup/request guard so `MTA_HOOKS_SECRET` must be non-empty: fail
startup or reject access when the secret resolves to an empty string, and ensure
the check uses the `MTA_HOOKS_SECRET`/`mtaHooks` values consistently.
In `@src/modules/mta-hooks/mta-hooks-auth.guard.ts`:
- Around line 16-20: The auth guard in MTAHooksAuthGuard is accepting an empty
secret because ConfigService.getOrThrow only rejects undefined, not ''. Update
the constructor to validate that both mtaHooks.username and mtaHooks.secret are
non-empty strings before storing them, and fail fast if the secret is missing or
blank. Use the existing MTAHooksAuthGuard constructor and
expectedSecret/expectedUsername fields to locate the change, and keep the
validation aligned with the mtaHooks configuration contract.
---
Nitpick comments:
In @.env.template:
- Around line 33-36: The dotenv linter is flagging the MTA hooks entries for
incorrect alphabetical ordering. Reorder the variables in the .env.template
section so MTA_HOOKS_SECRET appears before MTA_HOOKS_USERNAME, keeping the
existing values unchanged and preserving the surrounding comment and section
structure.
In `@src/modules/mta-hooks/mta-hooks-auth.guard.spec.ts`:
- Around line 65-71: Add a spec in mta-hooks-auth.guard.spec for the
empty-secret bypass case by extending the existing Basic auth parsing tests
around guard.canActivate and contextWithAuth. Verify that when the configured
secret is non-empty, a Basic credential decoding to a username with an empty
password such as stalwart: is rejected with UnauthorizedException, alongside the
existing no-colon test. Use the guard’s credential validation path in AuthGuard
to locate the behavior being exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26106eeb-7d02-47e8-93d7-20a9ea34f049
📒 Files selected for processing (12)
.env.templatesrc/app.module.tssrc/config/configuration.tssrc/modules/infrastructure/bridge/bridge.service.spec.tssrc/modules/infrastructure/bridge/bridge.service.tssrc/modules/mta-hooks/mta-hooks-auth.guard.spec.tssrc/modules/mta-hooks/mta-hooks-auth.guard.tssrc/modules/mta-hooks/mta-hooks.controller.tssrc/modules/mta-hooks/mta-hooks.module.tssrc/modules/mta-hooks/mta-hooks.service.spec.tssrc/modules/mta-hooks/mta-hooks.service.tssrc/modules/mta-hooks/mta-hooks.types.ts
- Added MTA hooks module with authentication guard, controller, and service. - Implemented `handleRcpt` method in `MtaHooksService` to manage recipient quota checks. - Introduced `getUserUsage` method in `BridgeClient` for fetching user storage usage. - Updated `.env.template` to include MTA hooks credentials. - Added unit tests for MTA hooks functionality and authentication guard to ensure reliability.
…uard - Added unit tests to verify that the MtaHooksAuthGuard throws UnauthorizedException when the secret is empty. - Enhanced the guard's constructor to throw an error if the configured secret is empty, ensuring proper configuration validation.
jzunigax2
force-pushed
the
feat/mta-hooks-read-only-quota
branch
from
cede383 to
5ffcffe
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/modules/mta-hooks/mta-hooks-auth.guard.spec.ts (1)
22-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider renaming
basichelper parameters to avoid shadowing.The
usernameandsecretparameters shadow the module-level constants of the same name (lines 7-8). While functionally correct, this can cause confusion during future edits.♻️ Optional rename
- const basic = (username: string, secret: string): string => - `Basic ${Buffer.from(`${username}:${secret}`).toString('base64')}`; + const basic = (user: string, pass: string): string => + `Basic ${Buffer.from(`${user}:${pass}`).toString('base64')}`;🤖 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 `@src/modules/mta-hooks/mta-hooks-auth.guard.spec.ts` around lines 22 - 23, Rename the parameters of the basic helper to avoid shadowing the module-level username and secret constants, and update the template expression to use the new parameter names while preserving the generated Basic authentication header.
🤖 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 @.env.template:
- Around line 35-37: Replace the empty MTA_HOOKS_SECRET value in the deployment
template with an explicit placeholder, and ensure the startup configuration
validation rejects the placeholder or any missing secret before enabling RCPT
hooks. Update the relevant MTA hooks configuration/validation symbols and
document that operators must replace the placeholder before deployment.
---
Nitpick comments:
In `@src/modules/mta-hooks/mta-hooks-auth.guard.spec.ts`:
- Around line 22-23: Rename the parameters of the basic helper to avoid
shadowing the module-level username and secret constants, and update the
template expression to use the new parameter names while preserving the
generated Basic authentication header.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f80a3a76-8d40-4ce1-a2b2-3641fd5400d2
📒 Files selected for processing (12)
.env.templatesrc/app.module.tssrc/config/configuration.tssrc/modules/infrastructure/bridge/bridge.service.spec.tssrc/modules/infrastructure/bridge/bridge.service.tssrc/modules/mta-hooks/mta-hooks-auth.guard.spec.tssrc/modules/mta-hooks/mta-hooks-auth.guard.tssrc/modules/mta-hooks/mta-hooks.controller.tssrc/modules/mta-hooks/mta-hooks.module.tssrc/modules/mta-hooks/mta-hooks.service.spec.tssrc/modules/mta-hooks/mta-hooks.service.tssrc/modules/mta-hooks/mta-hooks.types.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- src/modules/mta-hooks/mta-hooks.module.ts
- src/config/configuration.ts
- src/app.module.ts
- src/modules/mta-hooks/mta-hooks.types.ts
- src/modules/mta-hooks/mta-hooks.service.ts
- src/modules/mta-hooks/mta-hooks.controller.ts
- src/modules/mta-hooks/mta-hooks-auth.guard.ts
- src/modules/infrastructure/bridge/bridge.service.ts
- src/modules/mta-hooks/mta-hooks.service.spec.ts
- src/modules/infrastructure/bridge/bridge.service.spec.ts
Comment on lines
+35
to
+37
| # MTA hooks (RCPT) | ||
| MTA_HOOKS_USERNAME=stalwart | ||
| MTA_HOOKS_SECRET= |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not leave the MTA hooks secret empty in the deployment template.
With MTA_HOOKS_SECRET= and the configuration fallback to '', deployments that copy this file unchanged will have unusable RCPT authentication because the guard requires non-empty credentials. Use an explicit placeholder and validate the secret at startup, or document that it must be replaced before deployment.
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 37-37: [UnorderedKey] The MTA_HOOKS_SECRET key should go before the MTA_HOOKS_USERNAME key
(UnorderedKey)
🤖 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 @.env.template around lines 35 - 37, Replace the empty MTA_HOOKS_SECRET value
in the deployment template with an explicit placeholder, and ensure the startup
configuration validation rejects the placeholder or any missing secret before
enabling RCPT hooks. Update the relevant MTA hooks configuration/validation
symbols and document that operators must replace the placeholder before
deployment.
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
handleRcptmethod inMtaHooksServiceto manage recipient quota checks.getUserUsagemethod inBridgeClientfor fetching user storage usage..env.templateto include MTA hooks credentials.Summary by CodeRabbit
New Features
Bug Fixes
Tests