refactor: casl factory - users

[HayenNico]

Description

Subsection of PR #2748 for users.

This refactors the userEndpointAccess function in CaslAbilityFactory, extracting the ability builder into a separate module and adding userAccess in CaslAbilityFactory. Instance-level Action elements are removed and the affected controllers adjusted to accommodate the change. The user-specific code is extracted into a separate module.

Changes:

• Replace CaslAbilityFactory.userEndpointAccess with CaslAbilityFactory.userAccess
• Code for CaslAbilityFactory.userAccess factored out into new module UserAbility
• Remove all instance-level users Action elements, rename endpoint-level actions
• Adjust endpoint auth logic in users and user-identities controllers
• Add tests for endpoint access to admin-only endpoint /users/:id/password

Tests included

• Included for each change/fix?
• Passing?

Documentation

• swagger documentation updated (required for API changes)
• official documentation updated

Summary by Sourcery

Refactor CASL user access control by introducing a dedicated UserAbility, simplifying user actions, and updating controllers and tests to use the new unified permissions model.

New Features:

• Add a UserAbility module to encapsulate CASL ability building for user-related authorization.

Enhancements:

• Replace endpoint-specific userEndpointAccess with a generic userAccess method in CaslAbilityFactory and wire it through the CASL module.
• Unify user authorization actions into generic UserCreate/UserRead/UserUpdate/UserDelete plus AccessAny, removing instance-level user action variants and updating controllers accordingly.

Tests:

• Extend users API tests to cover access control for the admin-only password change endpoint and adjust controller unit tests for the new userAccess behavior.

[sourcery-ai]

Hey - I've found 2 issues, and left some high level feedback:

• In Users.js the new tests for the admin password change endpoint use a URL with a trailing } (e.g. /api/v3/users/${userIdAdmin}/password}), which will hit the wrong route and should be corrected to /password.
• The refactor now mixes Action.AccessAny for guards with Action.UserRead/UserUpdate in checkUserAuthorization; consider making the admin vs non-admin semantics clearer by consistently using AccessAny for admin-only endpoint checks and the user actions for self-access, to avoid confusion around which action is meant to represent admin-level access.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In Users.js the new tests for the admin password change endpoint use a URL with a trailing `}` (e.g. `/api/v3/users/${userIdAdmin}/password}`), which will hit the wrong route and should be corrected to `/password`.
- The refactor now mixes `Action.AccessAny` for guards with `Action.UserRead`/`UserUpdate` in `checkUserAuthorization`; consider making the admin vs non-admin semantics clearer by consistently using `AccessAny` for admin-only endpoint checks and the user actions for self-access, to avoid confusion around which action is meant to represent admin-level access.

## Individual Comments

### Comment 1
<location path="test/Users.js" line_range="213-215" />
<code_context>
   });

-  it("0050: admin should fail to change password for user when new and confirmation passwords do not match", async () => {
+  it("0050: anonymous user should not be able to access admin password change endpoint", async () => {
+    return request(appUrl)
+      .patch(`/api/v3/users/${userIdAdmin}/password}`)
+      .send({
+        newPassword: "compromisedPassword",
</code_context>
<issue_to_address>
**issue (bug_risk):** The admin password change endpoint URL in this test has a trailing `}` which likely makes the test hit the wrong route.

The controller route is `@Patch('/:id/password')`, so this URL should be `/api/v3/users/${userIdAdmin}/password` (no trailing `}`). As written, the test calls a non-existent endpoint and won’t properly cover the real admin password change route. Please fix the URL here and in the corresponding authenticated-user test below to match the controller route.
</issue_to_address>

### Comment 2
<location path="test/Users.js" line_range="224-221" />
<code_context>
+      .expect(TestData.UnauthorizedStatusCode);
+  });
+
+  it("0060: authenticated user should not be able to access admin password change endpoint", async () => {
+    return request(appUrl)
+      .patch(`/api/v3/users/${userIdAdmin}/password}`)
+      .send({
+        newPassword: "compromisedPassword",
+        confirmPassword: "compromisedPassword",
+      })
+      .set({ Authorization: `Bearer ${accessTokenUser1}` })
+      .set("Accept", "application/json")
+      .expect(TestData.UnauthorizedStatusCode);
+  });
+
</code_context>
<issue_to_address>
**question (bug_risk):** The expected status code for an authenticated non-admin hitting an admin-only endpoint may be incorrect.

For this admin-only `PATCH /users/:id/password` route, an authenticated non-admin should typically receive `403 Forbidden` (authenticated but lacks permission), not `401 Unauthorized`. The test currently expects `TestData.UnauthorizedStatusCode`. Please verify the actual response from `AuthenticatedPoliciesGuard` and `@CheckPolicies` for this endpoint and update the expected status code so the test matches the intended authorization behavior.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[nitrosx]

Code Review Report: refactor-casl-factory-users Branch

---

Overview

1. Verify logic correctness: The refactoring maintains logical equivalence. The new UserAbility class encapsulates the user authorization logic previously embedded in CaslAbilityFactory.userEndpointAccess(). The authorization rules are preserved: unauthenticated users have no access, authenticated users can perform CRUD operations on their own records, and admin users have full access including JWT creation.

2. Check if all edge cases are handled: The changes handle all edge cases properly. The UserAbility class correctly handles:

   • Unauthenticated users (null user)
   • Authenticated regular users (restricted to own _id)
   • Admin users (full access via ADMIN_GROUPS configuration)
   • Undefined accessGroups configuration (safely defaults to empty object)

3. Summarize what the code touched by the changes does: The changes refactor the user authorization system from a monolithic method in CaslAbilityFactory to a modular UserAbility class following the same pattern as DatasetAbility. This improves code organization, maintainability, and consistency across the authorization subsystem.

4. Assess whether the changes make sense: The refactoring is well-justified. It:

   • Follows existing patterns (DatasetAbility)
   • Improves separation of concerns
   • Simplifies action naming (e.g., UserReadOwn → UserRead with _id condition)
   • Maintains backward compatibility in behavior
   • The commit 68a8f1e9 introduced the core refactoring, while subsequent commits fixed tests and edge cases

5. Identify any unreachable code: No unreachable code identified. The old userEndpointAccess method was completely removed and replaced with the new UserAbility class.

Commits responsible: 68a8f1e9 (core refactoring), b415bceb (undefined accessGroups fix), ac76788f (added security tests), a8bf6b27 (added documentation)

---

Code Changes

New Files

• src/casl/abilities/users.ability.ts: New dedicated ability class for user authorization
• docs/developer-guide/authorization/users.md: Documentation explaining the new authorization model

Modified Files

• src/casl/action.enum.ts: Removed granular user actions (UserReadOwn, UserReadAny, UserCreateOwn, UserCreateAny, UserUpdateOwn, UserUpdateAny, UserDeleteOwn, UserDeleteAny, UserListAll, UserListOwn) and replaced with simpler actions (UserCreate, UserRead, UserUpdate, UserDelete, UserCreateJwt)

• src/casl/casl-ability.factory.ts:

  • Added UserAbility injection
  • Added userAccess() method that delegates to UserAbility.buildAbility()
  • Removed userEndpointAccess() method (71 lines removed)
  • Updated endpointAccess mapping to use userAccess instead of userEndpointAccess

• src/casl/casl.module.ts: Added UserAbility to providers

• src/users/users.controller.ts:

  • Updated checkUserAuthorization() to accept single Action parameter instead of Action[]
  • Replaced all Action.User*Own/Action.User*Any references with simpler Action.User*
  • Replaced Action.UserListAll/Action.UserListOwn with Action.AccessAny and Action.UserRead
  • Updated policy checks to use new action names

• src/users/user-identities.controller.ts: Similar updates to use new action names

• src/users/users.controller.spec.ts: Updated all test cases to use new action names and method names

• test/Users.js: Added tests for admin password change endpoint security (0050, 0060)

Assessment

• Necessary: Yes. The refactoring improves code organization and follows established patterns.
• Improves maintainability: Yes. Separating user authorization into its own class makes the code more modular.
• Reduces complexity: Yes. The action naming is simplified from 11 user-specific actions to 5, with conditions handling the "own vs any" distinction.

Improvement Needed

• The AccessAny action usage in user-specific contexts could be confusing. Consider renaming to UserAccessAny for clarity, or document the pattern more explicitly.
• The UserAbility class duplicates the can() calls for admin users (lines 61-65). These are redundant since AccessAny already grants full access. However, this is intentional for explicit documentation.

Verdict

The changes are well-executed and maintain backward compatibility while improving code structure. The refactoring follows the established pattern from the datasets refactoring (commit 45153cb0). The security tests added are valuable.

---

Security Review

1. Are there any potential injection vulnerabilities?

   • No. The changes are purely authorization logic refactoring. No user input is directly interpolated into queries or commands. The CASL library handles conditions safely.

2. Does this code expose any sensitive user data?

   • No new exposure. The authorization logic remains equivalent. The refactoring maintains the same access controls: users can only access their own data unless they are admins.

3. Are there instances of insecure API usage?

   • No. The CASL ability builder is used correctly with proper type definitions.

4. Could this code lead to an authentication bypass?

   • Potential concern: The UserAbility.buildAbility() method for admin users grants AccessAny which could be interpreted broadly. However, this is intentional and matches the previous behavior. The AccessAny action is already used in other parts of the system for admin access.

   • New security tests: Commits ac76788f and a8c04ce0 added tests specifically for the admin password change endpoint (PATCH /users/:id/password), ensuring:

     • Anonymous users cannot access it (0050)
     • Authenticated non-admin users cannot access it (0060)
     • Only admin users can access it (0080)

Commits responsible: ac76788f (security tests), a8c04ce0 (test fixes)

---

Test Coverage

Existing Coverage

• Unit tests in src/casl/casl-ability.factory.spec.ts updated to include UserAbility
• Controller tests in src/users/users.controller.spec.ts comprehensively updated
• Integration tests in test/Users.js added for password change endpoint security

Coverage Assessment

• Unit tests: Good. The UserAbility class is tested through the factory tests.
• Controller tests: Good. All endpoints updated with new action names.
• Integration tests: Improved. New tests verify admin endpoint security.

Suggestions for Improvement

• Add explicit unit tests for UserAbility.buildAbility() with different user types (unauthenticated, regular user, admin user)
• Add tests for edge cases like undefined accessGroups configuration (fixed in b415bceb)
• Consider adding tests for the UserCreateJwt action which is admin-only

---

Security Examples

Vulnerability	Example	Status
Unauthorized admin password change	Non-admin user attempting to change another user's password via PATCH /users/:id/password	Mitigated - New tests (0050, 0060) verify this is blocked
Information disclosure	Regular user accessing admin-only user list	Mitigated - findAll() now checks AccessAny before returning all users

---

Testing for Security Use Cases

Test: Unauthorized admin password change endpoint access

• Code affected: src/users/users.controller.ts:304-308 (PATCH /:id/password endpoint)
• Test location: test/Users.js:213-222 (0050), test/Users.js:224-234 (0060)
• How to test:

  # Anonymous user
  curl -X PATCH /api/v3/users/<admin-id>/password \
    -H "Content-Type: application/json" \
    -d '{"newPassword":"x","confirmPassword":"x"}' \
    -v  # Should return 401
  
  # Authenticated non-admin user
  curl -X PATCH /api/v3/users/<admin-id>/password \
    -H "Authorization: Bearer <non-admin-token>" \
    -H "Content-Type: application/json" \
    -d '{"newPassword":"x","confirmPassword":"x"}' \
    -v  # Should return 403

Test: Regular user data access isolation

• Code affected: src/users/users.controller.ts:115-128 (GET /users endpoint)
• How to test:

  # Regular user should only see their own data
  curl -X GET /api/v3/users \
    -H "Authorization: Bearer <regular-user-token>" \
    -v  # Should return only the authenticated user's data

---

Summary

The refactor-casl-factory-users branch successfully refactors the user authorization system to follow the established modular pattern used for datasets. Key achievements:

1. Code Quality: Improved through separation of concerns and reduced action enum complexity
2. Security: Enhanced with new tests for admin endpoint protection
3. Maintainability: Better code organization following existing patterns
4. Documentation: Added comprehensive developer guide for the new authorization model

Critical improvements:

• New security tests prevent unauthorized access to admin password change endpoint
• Simplified action naming reduces cognitive complexity
• Modular design allows for easier future modifications

No breaking changes in functionality. All existing behavior is preserved while improving the codebase structure.

The changes are approved for merging, with a minor recommendation to consider more explicit naming for the AccessAny action in user contexts to avoid potential confusion.

---

Generated by Mistral Vibe
Co-Authored-By: Mistral Vibe vibe@mistral.ai