refactor(modules): decoupling — constants, removeSensitive, audit req, last owner#3403
refactor(modules): decoupling — constants, removeSensitive, audit req, last owner#3403PierreBrisorgueil merged 10 commits intomasterfrom
Conversation
…ole magic strings (#3400)
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 15 minutes and 9 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughMiddleware now extracts identity and client-context (userId, organizationId, ip, userAgent) and passes them to AuditService.log; AuditService.log signature changed to accept explicit context fields. Organization membership magic strings were moved to constants, last-owner validation was extracted to a helper, sanitizeUser.removeSensitive was added and imports updated, and some middleware import paths were renamed. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuditMiddleware as Audit Middleware
participant AuditService
participant AuditRepo as Audit Repository
Client->>AuditMiddleware: HTTP request (req)
AuditMiddleware->>AuditMiddleware: extract userId, organizationId, ip, userAgent (config-controlled)
AuditMiddleware->>AuditService: log({ action, userId, organizationId, ip, userAgent, ... })
AuditService->>AuditRepo: persist audit record with provided fields
AuditRepo-->>AuditService: write result
AuditService-->>AuditMiddleware: completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3403 +/- ##
==========================================
+ Coverage 85.22% 85.53% +0.30%
==========================================
Files 112 114 +2
Lines 2883 2896 +13
Branches 788 792 +4
==========================================
+ Hits 2457 2477 +20
+ Misses 337 333 -4
+ Partials 89 86 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors several modules to reduce coupling and duplication by introducing shared constants, relocating user sanitization logic, and decoupling audit logging from Express request objects.
Changes:
- Refactors
AuditService.log()to accept plain context fields (user/org/ip/UA) and updates audit middleware + unit tests accordingly. - Introduces
modules/organizations/lib/constants.js, applies it across key organizations services/repositories/policies, and extracts a shared “last owner” protection helper. - Moves
removeSensitive()behavior intoUsersServiceand updates callers; renames organizationsmiddleware/→middlewares/and updates import paths.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| package-lock.json | Lockfile metadata updates (peer flags) reflecting dependency tree changes. |
| modules/users/services/users.service.js | Adds and exports removeSensitive() and switches internal callers to use it. |
| modules/home/services/home.service.js | Switches sanitization to UserService.removeSensitive. |
| modules/tasks/routes/tasks.routes.js | Updates organizations middleware import path to middlewares/. |
| modules/billing/routes/billing.routes.js | Updates organizations middleware import path to middlewares/. |
| modules/organizations/lib/constants.js | Adds membership role/status constants for reuse across the module. |
| modules/organizations/middlewares/organizations.middleware.js | Uses constants for synthetic admin membership role. |
| modules/organizations/policies/organizations.policy.js | Uses role constants in CASL role switch. |
| modules/organizations/services/organizations.service.js | Uses role constants when creating owner membership. |
| modules/organizations/services/organizations.crud.service.js | Uses role/status constants for membership lookups/creation. |
| modules/organizations/services/organizations.membership.service.js | Extracts validateLastOwnerProtection() and replaces magic strings with constants. |
| modules/organizations/repositories/organizations.membership.repository.js | Uses status constants in aggregate/find filters. |
| modules/organizations/tests/organizations.middleware.unit.tests.js | Updates import path for renamed middlewares/ folder. |
| modules/audit/services/audit.service.js | Refactors log() signature to remove req dependency; maps provided context fields. |
| modules/audit/middlewares/audit.middleware.js | Extracts context from req and passes plain fields to AuditService.log(). |
| modules/audit/tests/audit.service.unit.tests.js | Updates unit tests to pass plain context fields instead of req. |
| modules/audit/tests/audit.middleware.unit.tests.js | Updates expectations to reflect no req passed into AuditService.log(). |
modules/organizations/services/organizations.membership.service.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
modules/organizations/tests/organizations.middleware.unit.tests.js (1)
130-130: 🧹 Nitpick | 🔵 TrivialConsider using the constant in test assertions for consistency.
The test asserts against the hardcoded string
'owner'. While functionally correct, usingMEMBERSHIP_ROLES.OWNERwould improve maintainability if the constant value ever changes.♻️ Optional: Use constant in assertion
+import { MEMBERSHIP_ROLES } from '../lib/constants.js'; + // ... in the test assertion: - expect(req.membership).toEqual(expect.objectContaining({ role: 'owner', organizationId: fakeOrgId })); + expect(req.membership).toEqual(expect.objectContaining({ role: MEMBERSHIP_ROLES.OWNER, organizationId: fakeOrgId }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/organizations/tests/organizations.middleware.unit.tests.js` at line 130, Replace the hardcoded 'owner' string in the assertion with the MEMBERSHIP_ROLES.OWNER constant: update the expect on req.membership (currently expect.objectContaining({ role: 'owner', organizationId: fakeOrgId })) to use role: MEMBERSHIP_ROLES.OWNER and ensure MEMBERSHIP_ROLES is imported or referenced in the test file so the constant is available.modules/home/services/home.service.js (1)
88-91:⚠️ Potential issue | 🟡 MinorUpdate
team()JSDoc to use@returns.The modified async function still uses
@return; align with the required@returnsconvention.As per coding guidelines,
**/*.js: Every function must include JSDoc header with@paramand@returnsdocumentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/home/services/home.service.js` around lines 88 - 91, Update the JSDoc for the async function team() in home.service.js to use `@returns` (plural) instead of `@return` and ensure the header includes any required `@param` entries; locate the team() function declaration and replace the `@return` tag with `@returns` describing the Promise return type and value (e.g., Promise<Array|Object> or similar) and add `@param` tags for any parameters the function accepts so the JSDoc follows the repo convention.modules/users/services/users.service.js (1)
94-100:⚠️ Potential issue | 🟡 MinorFix stale
update()JSDoc parameter contract.Line 94 documents
adminas a boolean, but the function now takesoptionand branches on string values ('admin','recover'). Update the JSDoc to match the actual API.As per coding guidelines,
**/*.js: Every function must include JSDoc header with@paramand@returnsdocumentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/users/services/users.service.js` around lines 94 - 100, The JSDoc for the update function is stale: replace the boolean "admin" param with an "option" param that documents the string values used by the function ('admin', 'recover') and update `@param` entries for user and body and the `@return` to reflect it returns a Promise resolving to the updated user; specifically update the JSDoc above the const update = async (user, body, option) => { ... } to list `@param` {Object} user, `@param` {Object} body, `@param` {string} [option] - optional mode ('admin'|'recover'), and `@return` {Promise<Object>} user, and keep references to the behavior that uses removeSensitive and config.whitelists.users.update / updateAdmin / recover in the description.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/audit/tests/audit.middleware.unit.tests.js`:
- Around line 151-153: Update the unit assertions in the audit middleware test
to assert exact expected values rather than using toBeDefined(); keep the
existing expect(call.req).toBeUndefined() but replace
expect(call.userId).toBeDefined() and expect(call.ip).toBeDefined() with strict
equality checks against the known test fixtures (e.g., assert call.userId ===
<expectedUserId> and call.ip === <expectedIp>), and add assertions for the new
fields call.organizationId === <expectedOrganizationId> and call.userAgent ===
<expectedUserAgent> so the test verifies the full middleware → service contract
(look for the test that inspects the "call" object in
audit.middleware.unit.tests.js).
In `@modules/audit/tests/audit.service.unit.tests.js`:
- Around line 123-127: The tests for handling undefined ip/userAgent are
inconsistent: update both test titles and assertions to assert the strict
expected value (choose undefined per the suggestion). Specifically, in the tests
that call AuditService.log and inspect mockCreate.mock.calls[0][0], change the
descriptions from "empty string when undefined is passed" to "undefined when
undefined is passed" and replace expect(arg.ip).toBeFalsy() /
expect(arg.userAgent).toBeFalsy() with strict assertions like
expect(arg.ip).toBeUndefined() and expect(arg.userAgent).toBeUndefined(); apply
the same change to the second test that checks userAgent (the tests referencing
mockCreate and arg).
In `@modules/home/services/home.service.js`:
- Line 11: HomeService currently imports and calls UserService (import
UserService from '../../users/services/users.service.js') to sanitize user data
which breaks layer boundaries; extract the sanitizer logic into a lower/shared
utility or the users repository (e.g., create a users/utils/sanitizeUser or add
sanitizeUser to UsersRepository), replace direct calls to UserService in
HomeService (and at the other occurrences around the mentioned lines 92-95) with
an import of that new utility/repository method, and update HomeService to call
the new sanitizeUser helper so services no longer depend on other services.
In `@modules/users/services/users.service.js`:
- Around line 12-17: Update the JSDoc blocks in
modules/users/services/users.service.js that currently use `@return` to use
`@returns` and include the resolved value/type for async functions; specifically
change the JSDoc for the "Remove sensitive data from user object" header (and
the other JSDoc blocks at the listed ranges 25-31, 37-41, 60-64, 70-74, 90-96,
106-110) to use `@param` entries as-is and replace `@return` with `@returns`,
documenting the exact returned type/value (e.g., "Object|null" or
"Promise<Object|null>" for async functions) so each function has a `@returns`
describing the resolved value.
---
Outside diff comments:
In `@modules/home/services/home.service.js`:
- Around line 88-91: Update the JSDoc for the async function team() in
home.service.js to use `@returns` (plural) instead of `@return` and ensure the
header includes any required `@param` entries; locate the team() function
declaration and replace the `@return` tag with `@returns` describing the Promise
return type and value (e.g., Promise<Array|Object> or similar) and add `@param`
tags for any parameters the function accepts so the JSDoc follows the repo
convention.
In `@modules/organizations/tests/organizations.middleware.unit.tests.js`:
- Line 130: Replace the hardcoded 'owner' string in the assertion with the
MEMBERSHIP_ROLES.OWNER constant: update the expect on req.membership (currently
expect.objectContaining({ role: 'owner', organizationId: fakeOrgId })) to use
role: MEMBERSHIP_ROLES.OWNER and ensure MEMBERSHIP_ROLES is imported or
referenced in the test file so the constant is available.
In `@modules/users/services/users.service.js`:
- Around line 94-100: The JSDoc for the update function is stale: replace the
boolean "admin" param with an "option" param that documents the string values
used by the function ('admin', 'recover') and update `@param` entries for user and
body and the `@return` to reflect it returns a Promise resolving to the updated
user; specifically update the JSDoc above the const update = async (user, body,
option) => { ... } to list `@param` {Object} user, `@param` {Object} body, `@param`
{string} [option] - optional mode ('admin'|'recover'), and `@return`
{Promise<Object>} user, and keep references to the behavior that uses
removeSensitive and config.whitelists.users.update / updateAdmin / recover in
the description.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c68da846-8745-4c8b-880d-904760999c04
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
modules/audit/middlewares/audit.middleware.jsmodules/audit/services/audit.service.jsmodules/audit/tests/audit.middleware.unit.tests.jsmodules/audit/tests/audit.service.unit.tests.jsmodules/billing/routes/billing.routes.jsmodules/home/services/home.service.jsmodules/organizations/lib/constants.jsmodules/organizations/middlewares/organizations.middleware.jsmodules/organizations/policies/organizations.policy.jsmodules/organizations/repositories/organizations.membership.repository.jsmodules/organizations/services/organizations.crud.service.jsmodules/organizations/services/organizations.membership.service.jsmodules/organizations/services/organizations.service.jsmodules/organizations/tests/organizations.middleware.unit.tests.jsmodules/tasks/routes/tasks.routes.jsmodules/users/services/users.service.js
…util, test precision, JSDoc
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
modules/audit/tests/audit.service.unit.tests.js (1)
81-91: 🧹 Nitpick | 🔵 TrivialMinor: Consider using
toBeUndefined()for consistency with other tests.Lines 89-90 use
toBeFalsy()while similar assertions elsewhere (Lines 127, 141) usetoBeUndefined(). SinceAuditService.log({ action: 'system.startup' })doesn't passiporuserAgent, the service won't set these properties on the entry object (per theif (ip !== undefined)/if (userAgent !== undefined)guards), so they should beundefined.Suggested fix for consistency
expect(arg.action).toBe('system.startup'); expect(arg.userId).toBeUndefined(); expect(arg.orgId).toBeUndefined(); - expect(arg.ip).toBeFalsy(); - expect(arg.userAgent).toBeFalsy(); + expect(arg.ip).toBeUndefined(); + expect(arg.userAgent).toBeUndefined();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/audit/tests/audit.service.unit.tests.js` around lines 81 - 91, In the test "should log an audit entry without request context", replace the two assertions using toBeFalsy() for arg.ip and arg.userAgent with toBeUndefined() to match other tests; locate the assertions after the AuditService.log({ action: 'system.startup' }) call where mockCreate is inspected (mockCreate.mock.calls[0][0]) and change the expectations for arg.ip and arg.userAgent to use toBeUndefined().modules/audit/tests/audit.middleware.unit.tests.js (1)
132-156: 🧹 Nitpick | 🔵 TrivialAdd unit tests for
captureIpandcaptureUserAgentconfig flags in middleware.The middleware handles
config.audit.captureIpandconfig.audit.captureUserAgentat lines 130-131, but there are no middleware unit tests verifying this behavior. While the service tests cover how it handles undefined values, they do not test that the middleware correctly passes undefined when these flags are false.Add test cases like:
Example test cases
test('should pass undefined ip when captureIp config is false', () => { const middleware = createAuditMiddleware(); const req = createReq({ method: 'POST', originalUrl: '/api/test', baseUrl: '/api', route: { path: '/test' }, }); const res = createRes(200); jest.spyOn(config.audit, 'captureIp', 'get').mockReturnValue(false); middleware(req, res, jest.fn()); res.emit('finish'); expect(mockLog).toHaveBeenCalledWith(expect.objectContaining({ ip: undefined })); }); test('should pass undefined userAgent when captureUserAgent config is false', () => { const middleware = createAuditMiddleware(); const req = createReq({ method: 'POST', originalUrl: '/api/test', baseUrl: '/api', route: { path: '/test' }, }); const res = createRes(200); jest.spyOn(config.audit, 'captureUserAgent', 'get').mockReturnValue(false); middleware(req, res, jest.fn()); res.emit('finish'); expect(mockLog).toHaveBeenCalledWith(expect.objectContaining({ userAgent: undefined })); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/audit/tests/audit.middleware.unit.tests.js` around lines 132 - 156, Add unit tests to verify that createAuditMiddleware respects config.audit.captureIp and config.audit.captureUserAgent flags: write two tests that mock the getters jest.spyOn(config.audit, 'captureIp', 'get').mockReturnValue(false) and similarly for captureUserAgent, invoke middleware created by createAuditMiddleware with a POST request (use createReq/createRes), emit 'finish' on the response, and assert mockLog was called with an objectContaining where ip is undefined in the first test and userAgent is undefined in the second; ensure tests use jest.fn() for next and restore/reset mocks between tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/organizations/controllers/organizations.membership.controller.js`:
- Around line 35-40: In updateRole, ensure the controller fails closed if
req.membership is missing by adding an explicit check that req.membership exists
before allowing role changes: if req.membership is falsy, immediately call
responses.error(res, 403, 'Forbidden', 'Only owners can change member roles')();
keep the existing owner-only check using MEMBERSHIP_ROLES.OWNER so the logic
becomes: reject when no req.membership OR when req.membership.role !==
MEMBERSHIP_ROLES.OWNER, preventing accidental updates when resolveOrganization
didn't set req.membership.
- Around line 55-60: The remove handler's predicate allows a requesting MEMBER
to delete other members and doesn't guard missing req.membership; update remove
to first require req.membership (return 403 if absent), then if
req.membership.role === MEMBERSHIP_ROLES.MEMBER return a 403 (members cannot
remove anyone), and keep the existing admin check but tighten it to: if
requester is ADMIN and req.membershipDoc.role !== MEMBERSHIP_ROLES.MEMBER return
403 (admins may only remove MEMBERs); reference the remove function,
req.membership, req.membershipDoc, MEMBERSHIP_ROLES and resolveOrganization
which populates req.organization/req.membership.
In `@modules/organizations/services/organizations.membership.service.js`:
- Around line 23-31: The current validateLastOwnerProtection function (and
similar checks at the other noted spots) uses a separate count() call before
mutating, which is racy; change the logic to perform the check-and-mutate
atomically by moving the invariant into a transactional repository operation or
a conditional mutation method (e.g., add a
MembershipRepository.removeOrDemoteIfOtherOwnersExist or
updateStatusIfOtherOwnersExist) that runs inside a DB transaction or uses a
SELECT ... FOR UPDATE to lock rows, counts active OWNERs, and only performs the
delete/update when another active owner exists; replace calls to
validateLastOwnerProtection and the separate count+mutate sequences (including
the code around validateLastOwnerProtection, the blocks at lines ~94-97,
~109-112, and ~272-273) to use that new atomic repository method so concurrent
operations cannot leave zero owners.
In `@modules/users/services/users.service.js`:
- Around line 118-121: The owner-count check in the last-owner protection path
uses MembershipService.count filtered only by role, which can include inactive
memberships; update the count call used in the block that checks membership.role
=== MEMBERSHIP_ROLES.OWNER to also filter for active memberships (e.g., include
the active/status flag used in your model) so ownerCount only reflects active
owners for orgId; modify the MembershipService.count invocation (the one that
sets ownerCount) to include both organizationId: orgId and the active membership
condition alongside role: MEMBERSHIP_ROLES.OWNER.
---
Outside diff comments:
In `@modules/audit/tests/audit.middleware.unit.tests.js`:
- Around line 132-156: Add unit tests to verify that createAuditMiddleware
respects config.audit.captureIp and config.audit.captureUserAgent flags: write
two tests that mock the getters jest.spyOn(config.audit, 'captureIp',
'get').mockReturnValue(false) and similarly for captureUserAgent, invoke
middleware created by createAuditMiddleware with a POST request (use
createReq/createRes), emit 'finish' on the response, and assert mockLog was
called with an objectContaining where ip is undefined in the first test and
userAgent is undefined in the second; ensure tests use jest.fn() for next and
restore/reset mocks between tests.
In `@modules/audit/tests/audit.service.unit.tests.js`:
- Around line 81-91: In the test "should log an audit entry without request
context", replace the two assertions using toBeFalsy() for arg.ip and
arg.userAgent with toBeUndefined() to match other tests; locate the assertions
after the AuditService.log({ action: 'system.startup' }) call where mockCreate
is inspected (mockCreate.mock.calls[0][0]) and change the expectations for
arg.ip and arg.userAgent to use toBeUndefined().
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 86d7102c-47ee-4159-8453-e0fc06b97a9e
📒 Files selected for processing (9)
modules/audit/tests/audit.integration.tests.jsmodules/audit/tests/audit.middleware.unit.tests.jsmodules/audit/tests/audit.service.unit.tests.jsmodules/home/services/home.service.jsmodules/organizations/controllers/organizations.membership.controller.jsmodules/organizations/controllers/organizations.membershipRequest.controller.jsmodules/organizations/services/organizations.membership.service.jsmodules/users/services/users.service.jsmodules/users/utils/sanitizeUser.js
modules/organizations/controllers/organizations.membership.controller.js
Show resolved
Hide resolved
modules/organizations/controllers/organizations.membership.controller.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/organizations/controllers/organizations.membership.controller.js`:
- Line 63: The error message in the remove branch of the remove method is too
specific; update the responses.error call in
organizations.membership.controller.js (the remove handler) to use a neutral
forbidden message (e.g., "You do not have permission to perform this action" or
"Action forbidden") so it correctly covers missing membership, actor-not-member,
and admin-targeting-non-member cases; keep the 403 status and existing response
shape but replace the descriptive string used now ("Only owners can remove
admins or other owners") with the neutral message.
In
`@modules/organizations/tests/organizations.membership.controller.unit.tests.js`:
- Around line 36-37: Replace hard-coded role string literals in the test
fixtures with the shared MEMBERSHIP_ROLES constants to avoid drift; update
occurrences such as the objects named membership and membershipDoc (and the
other similar fixtures around the file) to use MEMBERSHIP_ROLES.OWNER /
MEMBERSHIP_ROLES.MEMBER (or the appropriate enum values) instead of
'owner'/'member' so the tests reference the centralized MEMBERSHIP_ROLES
constant for all role values.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e45f1fbe-8d61-44a4-a15d-0a971c6c6371
📒 Files selected for processing (3)
modules/organizations/controllers/organizations.membership.controller.jsmodules/organizations/tests/organizations.membership.controller.unit.tests.jsmodules/users/services/users.service.js
Closes #3398, closes #3399, closes #3400, closes #3401, closes #3402
Summary by CodeRabbit
New Features
Improvements
Tests