refactor: casl factory - publisheddata

[HayenNico]

Description

Subsection of PR #2748 for published-data.

This unifies the publishedDataEndpointAccess and publishedDataInstanceAccess functions in CaslAbilityFactory, and adjusts the affected controller to accommodate the change. The publisheddata-specific code is extracted into a separate module.

Changes:

• Replace CaslAbilityFactory.publishedDataInstanceAccess and CaslAbilityFactory.publishedDataEndpointAccess with one function CaslAbilityFactory.publishedDataAccess
• Code for CaslAbilityFactory.publishedDataAccess is factored out into new module PublishedDataAbility
• Adjust endpoint and instance auth logic in publisheddata v4 controller (v3 needed no adjustments)

Tests included

• Included for each change/fix?
• Passing?

Documentation

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

Summary by Sourcery

Unify published data CASL ability handling by delegating to a dedicated PublishedDataAbility and update the published-data v4 controller to use the new access entry point.

Enhancements:

• Extract published data authorization rules into a new PublishedDataAbility class and inject it into CaslAbilityFactory.
• Replace separate published data endpoint and instance accessors in CaslAbilityFactory with a single publishedDataAccess method wired into the CASL subjects map.
• Update the v4 published-data controller to use the unified publishedDataAccess method for all ability checks.

Build:

• Register PublishedDataAbility as a provider in the CASL module.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• Consider updating PublishedDataAbility.buildAbility’s signature to accept JWTUser | undefined (or make the parameter optional) to reflect the current usage from CaslAbilityFactory.publishedDataAccess and avoid misleading type expectations for unauthenticated calls.
• It might be helpful to give accessGroups in PublishedDataAbility an explicit AccessGroupsType | undefined type instead of leaving it implicit, to make the expected shape and potential undefined handling clearer at call sites using admin/delete.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Consider updating `PublishedDataAbility.buildAbility`’s signature to accept `JWTUser | undefined` (or make the parameter optional) to reflect the current usage from `CaslAbilityFactory.publishedDataAccess` and avoid misleading type expectations for unauthenticated calls.
- It might be helpful to give `accessGroups` in `PublishedDataAbility` an explicit `AccessGroupsType | undefined` type instead of leaving it implicit, to make the expected shape and potential `undefined` handling clearer at call sites using `admin`/`delete`.

## Individual Comments

### Comment 1
<location path="src/casl/abilities/published-data.ability.ts" line_range="21-25" />
<code_context>
+    this.accessGroups =
+      this.configService.get<AccessGroupsType>("accessGroups");
+  }
+  private accessGroups;
+
+  buildAbility(user: JWTUser): MongoAbility<PossibleAbilities, Conditions> {
</code_context>
<issue_to_address>
**suggestion:** Consider typing `accessGroups` explicitly and making it readonly.

`accessGroups` is currently implicitly `any` and mutable. To align with the rest of the config-based access logic and reduce accidental mutation, consider declaring it as `private readonly accessGroups?: AccessGroupsType;`, which also clarifies its use with optional chaining later.

```suggestion
  constructor(private configService: ConfigService) {
    this.accessGroups =
      this.configService.get<AccessGroupsType>("accessGroups");
  }
  private readonly accessGroups?: AccessGroupsType;
```
</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: Branch refactor-casl-factory-publisheddata

---

Overview

1. Verify logic correctness: The refactoring maintains the original authorization logic while improving code structure. The extraction of PublishedDataAbility into a separate class follows the same pattern used for DatasetAbility. All permission checks remain functionally equivalent.
2. Check if all edge cases are handled: The changes properly handle edge cases including unauthenticated users (null user), undefined accessGroups configuration, and proper type safety with JWTUser | null.
3. Summarize what the code touched by the changes does: The changes refactor the CASL (Authorization) layer for published data by extracting authorization logic from the monolithic CaslAbilityFactory into a dedicated PublishedDataAbility service class, following the same pattern established for datasets.
4. Assess whether the changes make sense: The changes are well-structured, follow existing patterns in the codebase, and improve maintainability without changing functionality.
5. Identify any unreachable code: No unreachable code identified. The removed methods (publishedDataEndpointAccess and publishedDataInstanceAccess) were replaced by the new unified publishedDataAccess method.

Commits responsible: 04dc7c2e (main refactor), 6b89e5bb (typing fix), 286d4ba5 (undefined accessGroups handling), 2a1f0314 (added docs), e9f6446d (moved docs to correct location), 16b3a691 (fixed test initialization).

---

Code Changes

Files Modified:

• src/casl/abilities/published-data.ability.ts - NEW: Extracted authorization logic for published data
• src/casl/casl-ability.factory.ts - Refactored to delegate to PublishedDataAbility
• src/casl/casl.module.ts - Added PublishedDataAbility to providers
• src/casl/casl-ability.factory.spec.ts - Updated tests to inject PublishedDataAbility
• src/published-data/published-data.v4.controller.ts - Updated method calls from publishedDataInstanceAccess to publishedDataAccess
• docs/developer-guide/authorization/published-data.md - NEW: Comprehensive authorization documentation

Assessment: The changes are necessary for maintaining a consistent architecture. The CASL factory was growing monolithic, and extracting domain-specific abilities into separate classes improves:

• Separation of concerns
• Testability
• Code reusability
• Maintainability

Improvement Needed:

1. The PublishedDataAbility class lacks dedicated unit tests. Currently, it's only tested indirectly through CaslAbilityFactory tests.
2. The documentation file lacks a trailing newline (minor formatting issue).
3. Consider adding a dedicated test file for PublishedDataAbility to verify all permission scenarios.

Verdict: The refactoring is well-executed and follows established patterns. The changes maintain functional equivalence while improving architecture.

---

Security Review

1. Are there any potential injection vulnerabilities?

   • No. The changes are purely structural refactoring of authorization logic. No new user input handling or query construction was introduced. The existing CASL library properly sanitizes conditions.

2. Does this code expose any sensitive user data?

   • No. The authorization logic only determines what users can access, not how data is retrieved or exposed. No new data exposure paths were created.

3. Are there instances of insecure API usage?

   • No. The CASL library is used correctly with proper ability building and checking patterns.

4. Could this code lead to an authentication bypass?

   • No. The refactoring maintains the same authorization checks. In fact, it improves consistency by:
     • Properly handling null user cases (commit 286d4ba5)
     • Safely handling undefined accessGroups configuration (commit 286d4ba5)
     • Using consistent type signatures (JWTUser | null)

Commits responsible: All commits maintain security posture; 286d4ba5 specifically improved null safety.

---

Test Coverage

Current State: Inadequate for the new PublishedDataAbility class.

Findings:

• PublishedDataAbility has no dedicated unit tests
• Only tested indirectly through CaslAbilityFactory instantiation tests
• The factory tests verify construction but not the actual buildAbility() logic

Suggestions for Improvement:

1. Create src/casl/abilities/published-data.ability.spec.ts with tests for:
   • Unauthenticated user returns empty ability
   • Authenticated user has Create, Read, Update permissions
   • User in ADMIN_GROUPS has AccessAny permission
   • User in DELETE_GROUPS has Delete permission
   • Proper handling when accessGroups configuration is undefined
   • Proper handling when accessGroups.admin/delete are undefined

Relevant commits: 16b3a691 updated factory tests but didn't add ability-specific tests.

---

Security Examples

Empty - No security vulnerabilities were identified in the changes.

---

Testing for Security Use Cases

Authorization Bypass Testing:
To verify the refactoring didn't introduce authorization bypasses:

1. Test unauthenticated access to protected endpoints:

   // Verify unauthenticated users cannot create/update/delete
   const ability = publishedDataAbility.buildAbility(null);
   expect(ability.cannot(Action.Create, PublishedData)).toBe(true);
   expect(ability.cannot(Action.Update, PublishedData)).toBe(true);
   expect(ability.cannot(Action.Delete, PublishedData)).toBe(true);

2. Test authenticated user without special groups:

   const user: JWTUser = { username: 'test', currentGroups: [] };
   const ability = publishedDataAbility.buildAbility(user);
   expect(ability.can(Action.Create, PublishedData)).toBe(true);
   expect(ability.can(Action.Read, PublishedData)).toBe(true);
   expect(ability.can(Action.Update, PublishedData)).toBe(true);
   expect(ability.cannot(Action.Delete, PublishedData)).toBe(true);
   expect(ability.cannot(Action.AccessAny, PublishedData)).toBe(true);

3. Test ADMIN_GROUPS user:

   const user: JWTUser = { username: 'admin', currentGroups: ['admin-group'] };
   const configService = { get: () => ({ admin: ['admin-group'], delete: [] }) };
   const ability = new PublishedDataAbility(configService).buildAbility(user);
   expect(ability.can(Action.AccessAny, PublishedData)).toBe(true);

4. Test DELETE_GROUPS user:

   const user: JWTUser = { username: 'deleter', currentGroups: ['delete-group'] };
   const configService = { get: () => ({ admin: [], delete: ['delete-group'] }) };
   const ability = new PublishedDataAbility(configService).buildAbility(user);
   expect(ability.can(Action.Delete, PublishedData)).toBe(true);

Files affected: src/casl/abilities/published-data.ability.ts (lines 28-72)

---

---

Summary

Architecture: The refactoring successfully extracts published data authorization logic into a dedicated, injectable service (PublishedDataAbility), following the established pattern from DatasetAbility. This improves code organization and maintainability.

Functionality: All authorization behavior is preserved. The changes maintain the same permission matrix as documented.

Security: No security issues introduced. The refactoring actually improves null safety with proper handling of undefined configurations and null users.

Testing: The primary gap is the lack of dedicated unit tests for PublishedDataAbility. The existing factory tests only verify instantiation, not the actual authorization logic.

Documentation: Comprehensive documentation was added in docs/developer-guide/authorization/published-data.md, clearly explaining the permission model.

Recommendation: Add dedicated unit tests for PublishedDataAbility before merging. The architectural changes are sound and the code is ready for production once test coverage is addressed.

---

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

[nitrosx]

This PR might be effected by #2886 or vice versa.

[omkar-ethz]

Just a reminder we should update the newly added authorization docs for publisheddata to reflect the casl refactor: #2886 (comment) in this / another PR