feat: Implement initial API structure with core modules, authenticati…

[JahnaviVeera]

…on, and Prisma schema.

[github-actions]

🤖 AI Code Review

🤖 AI Code Review

📌 Summary

• The diff introduces new functionality for tracking createdBy and updatedBy fields across multiple models and services, adds support for the accountant role, and enhances filtering capabilities in expense-related endpoints.
• Overall, the changes are well-structured, but there are some areas that could benefit from improvements in security, maintainability, and performance.

---

💬 Inline Comments (File-wise)

prisma/schema.prisma

• Line 15: Adding the accountant role to the UserRole enum is a good addition. However, ensure that this role is properly integrated into authorization checks throughout the application.
• Lines 174, 261, 359, 378, 410, 484, 525: The addition of createdBy and updatedBy fields is useful for tracking changes. However, storing full names might not be ideal for scalability and consistency. Consider storing user IDs instead and resolving full names dynamically when needed.
  • ✅ Suggestion: Replace String? with userId (foreign key to User model) for better relational integrity.

src/app.ts

• Line 88: Adding the /api/reports route is straightforward. Ensure that the reportRoutes module includes proper authentication and authorization checks.

src/middleware/auth.middleware.ts

• Line 9: Adding fullName to the AuthRequest interface is fine, but ensure that this field is consistently populated and validated during authentication.
• Line 46: The inclusion of fullName in the req.user object is acceptable, but consider whether this is necessary for every request or if it can be resolved dynamically when needed.

src/modules/auth/auth.routes.ts

• Line 26: Adding an endpoint for accountant/login is logical. Ensure that the userLogin controller properly handles the accountant role and its permissions.

src/modules/auth/auth.services.ts

• Lines 56, 108, 221, 300, 372: Passing userName (full name) to token generation functions is potentially problematic for token size and security. Tokens should ideally contain minimal information.
  • ✅ Suggestion: Use user ID and role only in tokens, and resolve additional details like userName from the database when needed.

src/modules/daily-updates/daily-updates.routes.ts

• Lines 18-81: Adding accountant to the authorized roles for various endpoints is consistent. Ensure that the accountant role's permissions are clearly defined and tested.

src/modules/documents/documents.controller.ts

• Lines 116, 601: Using authReq.user?.fullName || "System" for createdBy and updatedBy fields is functional but could lead to inconsistencies if fullName is not reliably populated.
  • ✅ Suggestion: Use userId instead and resolve fullName dynamically when displaying data.

src/modules/documents/documents.services.ts

• Lines 22, 66, 307, 323: The addition of createdBy and updatedBy fields is consistent with the controller changes. However, storing full names directly in the database can lead to redundancy and data integrity issues.
  • ✅ Suggestion: Store userId instead and resolve fullName dynamically.

src/modules/expense/expense.controller.ts

• Lines 91, 209, 237: The changes to support filtering by projectId, startDate, and endDate are useful for enhanced querying. Ensure that these query parameters are validated to prevent SQL injection or other security issues.

---

⚠️ High-Risk Issues

1. Token Payload Size and Security

• Including userName in tokens increases their size unnecessarily and could expose sensitive information.
  • ✅ Suggestion: Limit token payloads to essential fields like userId and role. Resolve additional details from the database when needed.

2. Data Integrity for createdBy and updatedBy

• Storing full names directly in the database can lead to inconsistencies if a user's name changes. This also makes relational queries harder.
  • ✅ Suggestion: Store userId instead and resolve fullName dynamically when displaying data.

3. Authorization for accountant Role

• The accountant role has been added to multiple endpoints, but its permissions and access levels are not clearly defined.
  • ✅ Suggestion: Audit all endpoints to ensure the accountant role has appropriate access and restrictions.

4. Query Parameter Validation

• The addition of projectId, startDate, and endDate query parameters in expense-related endpoints introduces potential risks for SQL injection.
  • ✅ Suggestion: Validate and sanitize all query parameters before using them in database queries.

---

🛠️ Suggested Improvements

1. Replace createdBy and updatedBy fields with userId (foreign key to User model) for better relational integrity.
2. Limit token payloads to essential fields (userId, role) and resolve additional details dynamically.
3. Define and document the permissions for the accountant role to avoid unintended access.
4. Implement validation and sanitization for all query parameters in expense-related endpoints.

---

✅ Final Notes

The changes are well-structured and align with the application's goals of improving auditability and role-based access control. Addressing the highlighted issues will enhance security, maintainability, and scalability.

---

null

---

🤖 AI Code Review

📌 Summary

• The changes introduce audit tracking (createdBy, updatedBy) and role-based access control enhancements across multiple modules.
• Improvements in maintainability and security are evident, but some areas need attention for better robustness and clarity.
• Potential issues with type safety, error handling, and performance optimization were identified.

---

💬 Inline Comments (File-wise)

src/modules/project/project.routes.ts

• Line 11-22: The addition of the "accountant" role to various endpoints is appropriate for expanding access control. However, ensure this aligns with business requirements and does not inadvertently expose sensitive data to unauthorized roles.

src/modules/project/project.services.ts

• Line 43: Adding createdBy is a good enhancement for audit tracking. Consider validating the createdBy field to ensure it contains valid user information.
• Line 370: Similarly, updatedBy is a useful addition. Ensure this field is consistently populated across all update operations.

src/modules/purchases/purchases.controller.ts

• Line 3-37: The inclusion of createdBy and updatedBy fields is beneficial for tracking changes. However, casting req as any (authReq = req as any) can lead to type safety issues.
  • ✅ Suggestion: Define a custom type for Request that includes the user property to avoid unsafe type casting.
• Line 55: The query parameters (projectId, startDate, endDate) are directly cast to strings without validation. This could lead to unexpected behavior if invalid values are passed.
  • ✅ Suggestion: Validate and sanitize query parameters before using them.

src/modules/purchases/purchases.routes.ts

• Line 9-12: Adding authorizeRoles middleware improves security. Ensure the roles and permissions are thoroughly tested to prevent unauthorized access.

src/modules/purchases/purchases.services.ts

• Line 7-62: The addition of new fields (transportAmt, vendorPay, etc.) is well-implemented. However, the repeated use of ?? null for default values can be simplified.
  • ✅ Suggestion: Use a utility function to handle default values for multiple fields to reduce redundancy.
• Line 89: The whereClause construction for filtering purchases is effective but could benefit from stricter validation of startDate and endDate formats.
  • ✅ Suggestion: Validate date formats using a library like date-fns or moment.

src/modules/quotations/quotations.controller.ts

• Line 61-473: Masking sensitive data for the "accountant" role is a good security measure. However, the logic for masking (q.totalAmount = "••••••") is repeated multiple times.
  • ✅ Suggestion: Extract the masking logic into a reusable function to improve maintainability.
• Line 253: The addition of createdBy is useful for audit tracking. Similar to other files, avoid casting req as any.
  • ✅ Suggestion: Use a custom type for Request with the user property.

---

⚠️ High-Risk Issues

1. Type Safety Concerns:

   • The repeated use of req as any for accessing user properties can lead to runtime errors if the user object is undefined or malformed.
   • ✅ Suggestion: Define a custom AuthenticatedRequest type that extends Request and includes the user property.

2. Date Validation:

   • The startDate and endDate query parameters in getAllPurchases are not validated. Invalid dates could cause unexpected behavior or errors.
   • ✅ Suggestion: Validate date formats using a library like date-fns or moment.

3. Role-Based Data Masking:

   • The masking logic for the "accountant" role is repeated across multiple endpoints. Any future changes to masking rules will require updates in multiple places, increasing the risk of inconsistencies.
   • ✅ Suggestion: Centralize the masking logic into a utility function.

4. Error Handling:

   • Error handling in controllers (e.g., createPurchase, updatePurchase) is generic and does not differentiate between validation errors, database errors, or other issues.
   • ✅ Suggestion: Implement more granular error handling to provide clearer feedback to the client.

---

🚀 Recommendations

• Refactor repeated logic (e.g., data masking, default value handling) into reusable utility functions.
• Improve type safety by defining custom types for Request objects with user properties.
• Validate and sanitize all user inputs, including query parameters and request bodies, to prevent unexpected behavior or security vulnerabilities.
• Enhance error handling to provide more specific and actionable feedback to clients.

Let me know if you need further clarification or assistance!

---

🤖 AI Code Review

📌 Summary

• The changes introduce new functionality for handling "accountant" roles, including data masking and role-based access control.
• The updates are generally well-structured but include some potential issues related to type safety, maintainability, and security.
• Suggestions are provided to improve code quality and address potential risks.

---

💬 Inline Comments (File-wise)

src/modules/quotations/quotations.controller.ts

• Line 746: The use of authReq as any bypasses TypeScript's type safety.
  • ✅ Suggestion: Define a proper interface for authReq to include the user property with role and fullName fields.
• Line 805: Masking totalAmount for accountants is a good addition, but the hardcoded "••••••" could be replaced with a constant for better maintainability.
  • ✅ Suggestion: Define a constant like MASKED_VALUE = "••••••" and reuse it across the codebase.

src/modules/quotations/quotations.routes.ts

• Line 4: The addition of the "accountant" role to various routes is consistent, but ensure this role is documented in the API documentation for clarity.

src/modules/quotations/quotations.services.ts

• Line 210: Adding updatedBy is a good enhancement, but ensure that this field is validated before being passed to the database.
  • ✅ Suggestion: Validate updatedBy to ensure it is a non-empty string before updating the database.

src/modules/reports/reports.controller.ts

• Line 8: Query parameters like projectId, startDate, and endDate are cast as strings without validation.
  • ✅ Suggestion: Validate these parameters to ensure they meet expected formats (e.g., startDate and endDate should be valid dates).

src/modules/reports/reports.services.ts

• Line 10: The whereExpense and wherePurchase objects are dynamically constructed but lack type safety.
  • ✅ Suggestion: Define a type or interface for these objects to ensure consistency and avoid runtime errors.
• Line 77: Parsing category as JSON without validation could lead to runtime errors if the data is malformed.
  • ✅ Suggestion: Use a try-catch block to handle JSON parsing errors gracefully and log any issues for debugging.

src/modules/supervisor/supervisor.controller.ts

• Line 224: Masking totalBudget for accountants is a good addition, but the logic is repeated in multiple places.
  • ✅ Suggestion: Extract the masking logic into a reusable utility function to improve maintainability.
• Line 636: Similar to above, the masking logic for totalBudget is repeated.
  • ✅ Suggestion: Use the same utility function for consistency.

src/modules/supervisor/supervisor.routes.ts

• Line 14: Adding the "accountant" role to routes is consistent, but ensure this role's permissions are clearly documented.

---

⚠️ High-Risk Issues

1. Type Safety Issues:

   • The frequent use of any (e.g., authReq in multiple files) bypasses TypeScript's type checking, which can lead to runtime errors.
   • ✅ Suggestion: Define proper interfaces for authReq and other objects to ensure type safety.

2. Unvalidated Input:

   • Query parameters like startDate and endDate in reports.controller.ts are not validated, which could lead to unexpected behavior or security vulnerabilities.
   • ✅ Suggestion: Validate all user inputs, especially query parameters, to ensure they meet expected formats.

3. Hardcoded Masking Values:

   • The hardcoded "••••••" for masking sensitive data is not maintainable and could lead to inconsistencies.
   • ✅ Suggestion: Use a constant or configuration value for masked data representation.

4. JSON Parsing Without Validation:

   • Parsing category as JSON in reports.services.ts without validation could lead to runtime errors if the data is malformed.
   • ✅ Suggestion: Add error handling for JSON parsing and log any issues for debugging.

---

⚠️ Additional Notes

• No critical security vulnerabilities were identified, but the lack of input validation and type safety could lead to potential issues in the future.
• Consider adding unit tests for the new functionality, especially for role-based access control and data masking, to ensure correctness and prevent regressions.

By addressing the above issues, the codebase will be more robust, maintainable, and secure.

---

🤖 AI Code Review

📌 Summary

• The changes introduce a new role, accountant, across multiple routes and services.
• Enhancements include tracking createdBy and updatedBy fields for user creation and updates.
• JWT token generation now includes a fullName field.
• Overall, the changes improve functionality but raise concerns about security, maintainability, and potential role misuse.

---

💬 Inline Comments (File-wise)

src/routes/supervisor.routes.ts

• Line 1-20: The addition of the accountant role to multiple routes increases flexibility but may inadvertently grant excessive permissions. Ensure that the accountant role is intended to access these endpoints.
  • ✅ Suggestion: Review the business logic and confirm that accountant should have access to sensitive operations like updating or deleting supervisors.

src/modules/user/user.controller.ts

• Line 68: The authReq cast to any introduces type safety issues.
  • ✅ Suggestion: Define a proper TypeScript interface for the request object that includes the user property with fullName.

src/modules/user/user.routes.ts

• Line 1-50: Similar to the supervisor routes, the addition of the accountant role to admin-specific routes (e.g., /admin/account-settings) might expose sensitive data or operations.
  • ✅ Suggestion: Reassess the role's permissions and ensure proper authorization checks are in place.

src/modules/user/user.services.ts

• Line 23-67: The createdBy and updatedBy fields are useful for auditing but are optional. This could lead to inconsistent data if not always provided.
  • ✅ Suggestion: Consider making these fields mandatory or adding fallback values (e.g., "System") to ensure consistency.

src/utils/jwt.ts

• Line 11-22: Adding fullName to the JWT payload increases token size and might expose unnecessary user information.

  • ✅ Suggestion: Avoid including sensitive or non-essential data in JWT payloads. Instead, fetch fullName from the database when needed.

• Line 55-60: The role validation now includes accountant. Ensure that this role addition aligns with the application's security model.

  • ✅ Suggestion: Centralize role validation logic to avoid duplication and ensure consistency.

---

⚠️ High-Risk Issues

1. Excessive Permissions for accountant Role:

   • The accountant role is granted access to several admin and supervisor routes, including sensitive operations like updating and deleting supervisors or users. This could lead to privilege escalation or data breaches if the role is misused.
   • ✅ Suggestion: Perform a thorough review of the accountant role's responsibilities and restrict access to only necessary endpoints.

2. Sensitive Data in JWT Payload:

   • Including fullName in the JWT payload could expose personal information if the token is intercepted or logged.
   • ✅ Suggestion: Remove fullName from the JWT payload and retrieve it from the database when required.

3. Type Safety Issues in user.controller.ts:

   • Casting req to any bypasses TypeScript's type-checking, increasing the risk of runtime errors.
   • ✅ Suggestion: Define a proper interface for the request object to ensure type safety.

---

⚠️ Additional Notes

• No performance concerns were identified in the changes.
• Ensure that all new role-based access control changes are thoroughly tested, especially for edge cases like unauthorized access attempts.
• Consider adding unit tests for the new createdBy and updatedBy fields to validate their behavior under different scenarios.

---