middle-ware-added #175
middle-ware-added #175
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis change refactors admin entity deletion to enforce tenant scoping and centralizes admin role validation in a new middleware. The admin role check is removed from the controller and instead applied as middleware in the routing layer. Deletion methods and audit logging are updated to include tenantId, and method names are aligned for consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant checkAdminRole Middleware
participant Authenticator Middleware
participant AdminController
participant AdminHelper
participant Database
Client->>Router: DELETE /admin/deleteResource
Router->>checkAdminRole Middleware: Validate admin role via JWT
alt Valid admin
checkAdminRole Middleware->>Authenticator Middleware: Authenticate user
Authenticator Middleware->>AdminController: Call deleteEntity
AdminController->>AdminHelper: allowRecursiveDelete (with tenantId)
AdminHelper->>Database: Delete entity, filter by tenantId
AdminHelper->>Database: pullEntityFromGroups (with tenantId)
AdminHelper->>Database: create deletion audit log
AdminHelper->>AdminController: Return deletion result
AdminController->>Router: Respond to client
else Invalid admin
checkAdminRole Middleware->>Router: Respond with forbidden error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
src/databaseQueries/admin.js (1)
36-42: Update JSDoc to document the new tenantId parameter.The method signature was correctly updated to include tenant scoping, but the JSDoc comment needs to reflect this change.
/** * Unlink an entity ID from all parent documents that reference it in their groups. * * @param {String} entityType - Type of the entity (e.g., 'block', 'cluster'). * @param {ObjectId} entityId - MongoDB ObjectId of the entity to remove from groups. + * @param {String} tenantId - Tenant ID to scope the operation. * @returns {Promise<Object>} - MongoDB updateMany result containing modified count. */
🧹 Nitpick comments (1)
src/generics/middleware/checkAdminRole.js (1)
75-77: Remove unnecessary return statement.The
returnafternext()is redundant.next() -return
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/controllers/v1/admin.js(1 hunks)src/databaseQueries/admin.js(2 hunks)src/databaseQueries/deletionAuditLogs.js(1 hunks)src/generics/middleware/checkAdminRole.js(1 hunks)src/module/admin/helper.js(6 hunks)src/routes/index.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-01T11:20:44.813Z
Learnt from: MallanagoudaB
PR: ELEVATE-Project/entity-management#172
File: src/generics/kafka/producers.js:22-37
Timestamp: 2025-08-01T11:20:44.813Z
Learning: In the ELEVATE-Project/entity-management service, the team consistently uses the async Promise executor pattern `return new Promise(async (resolve, reject) => { ... })` throughout the entire codebase including controllers, helpers, database queries, and services. This is an established coding standard that should be maintained for consistency.
Applied to files:
src/controllers/v1/admin.jssrc/routes/index.jssrc/module/admin/helper.js
📚 Learning: 2025-08-01T12:12:13.317Z
Learnt from: MallanagoudaB
PR: ELEVATE-Project/entity-management#172
File: src/module/admin/helper.js:222-224
Timestamp: 2025-08-01T12:12:13.317Z
Learning: In the ELEVATE-Project/entity-management repository, the Kafka implementation already includes built-in retry and error handling mechanisms at the client level, so additional retry logic in individual methods like pushEntityDeleteKafkaEvent is not necessary.
Applied to files:
src/module/admin/helper.js
🧬 Code Graph Analysis (2)
src/routes/index.js (2)
src/generics/middleware/checkAdminRole.js (1)
checkAdminRole(16-16)src/generics/middleware/validator/index.js (1)
fs(10-10)
src/generics/middleware/checkAdminRole.js (1)
src/routes/index.js (1)
checkAdminRole(10-10)
🪛 Biome (2.1.2)
src/routes/index.js
[error] 77-77: This property is later overwritten by an object member with the same name.
Overwritten with this property.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property.
(lint/suspicious/noDuplicateObjectKeys)
🔇 Additional comments (11)
src/databaseQueries/deletionAuditLogs.js (1)
13-13: LGTM! Clean method rename for better semantics.The rename from
deletionAuditLogstocreateimproves the method's semantic clarity and aligns with standard CRUD naming conventions.Also applies to: 21-21
src/controllers/v1/admin.js (1)
76-81: LGTM! Clean separation of concerns.Moving admin role validation from the controller to middleware is a good architectural decision. The controller now focuses on business logic while authentication/authorization is handled at the middleware layer.
src/databaseQueries/admin.js (1)
52-52: LGTM! Essential tenant scoping added.Adding
tenantIdto the filter query is crucial for multi-tenant data isolation and prevents cross-tenant data access issues.src/routes/index.js (1)
10-10: LGTM! Proper middleware integration.The checkAdminRole middleware is correctly integrated into the middleware stack with proper ordering (authentication → admin role check → pagination).
Also applies to: 18-18
src/module/admin/helper.js (7)
82-83: LGTM! Proper tenant scoping added to entity filter.The addition of
tenantIdto the filter ensures that entity lookups are properly scoped by tenant, which is essential for multi-tenant security and data isolation.
115-118: LGTM! Consistent tenant scoping in recursive deletion.The
tenantIdfilter is correctly applied to the bulk deletion operation for related entities, maintaining tenant isolation during recursive deletions.
139-142: LGTM! Tenant scoping applied to single entity deletion.The single entity deletion now properly includes
tenantIdin the filter, ensuring consistent tenant scoping across both recursive and non-recursive deletion paths.
199-199: Improved promise resolution pattern.The change from
resolve()toreturn resolve()is consistent with good practices and provides clearer control flow, though both forms are functionally equivalent in this context.
188-188: Signature matches usage of pullEntityFromGroups
The static method insrc/databaseQueries/admin.jsis defined asstatic pullEntityFromGroups(entityType, entityId, tenantId) { … }which aligns with the three-argument call in
src/module/admin/helper.js. No further changes needed.
191-191: Deletion audit log method rename validatedThe
deletionAuditLogsclass insrc/databaseQueries/deletionAuditLogs.jsnow exposes a staticcreatemethod (no remaininglogDeletion), matching the call insrc/module/admin/helper.js. No further changes needed.
209-209: Confirmed:logDeletionfully replaced bycreateAll references to the old
logDeletionmethod have been removed, and the database query class implementsstatic create(logs)as expected. No further changes are needed.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
VISHNUDAS-tunerlabs
left a comment
VISHNUDAS-tunerlabs
left a comment
There was a problem hiding this comment.
Reviewed on aug 8 - 4:21
|
|
||
| module.exports = async function (req, res, next) { | ||
| // Define paths that require admin role validation | ||
| let adminPath = ['admin/deleteResource'] |
There was a problem hiding this comment.
@MallanagoudaB is this the only api in entity service which require admin role?
| } | ||
|
|
||
| let decodedToken | ||
| try { |
There was a problem hiding this comment.
@MallanagoudaB we should also take the scenario of keycloak usage as well
VISHNUDAS-tunerlabs
left a comment
VISHNUDAS-tunerlabs
left a comment
There was a problem hiding this comment.
reviewed onaug 8, 5:10 PM
Summary by CodeRabbit
New Features
Bug Fixes
Refactor