refactor: casl factory - history

[HayenNico]

Description

Subsection of PR #2748 for attchments. PR depends on #2759 to be merged first.

This unifies the historyEndpointAccess and historyInstanceAccess functions in CaslAbilityFactory, and adjusts the history controller to accommodate the change. The history-specific auth code is extracted into a separate module.

Changes:

• Replace CaslAbilityFactory.historyInstanceAccess and CaslAbilityFactory.historyEndpointAccess with one function CaslAbilityFactory.historyAccess
• Code for CaslAbilityFactory.hsitoryAccess is factored out into new module HistoryAbility
• Rename endpoint-level action from HistoryReadEndpoint to HistoryRead
• Adjust endpoint and instance auth logic in attachment controller

Tests included

• Included for each change/fix?
• Passing?

Documentation

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

Summary by Sourcery

Unify history authorization into a single CASL history ability and update history endpoints to use subject-based HistoryRead permissions.

Enhancements:

• Extract history authorization logic into a dedicated HistoryAbility service and inject it into CaslAbilityFactory.
• Replace separate history endpoint and instance access methods with a unified historyAccess method in CaslAbilityFactory.
• Refactor history controller to use subject-based HistoryRead checks instead of per-subsystem action mappings and GenericHistory strings.
• Simplify CASL action enum by consolidating history endpoint and instance actions under a single HistoryRead action while preserving per-collection differentiation via subjects.

Build:

• Register the new HistoryAbility provider in the CASL module for dependency injection.

[sourcery-ai]

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

• In HistoryAbility.buildAbility, all the group checks assume user.currentGroups is a defined array; consider adding a defensive check (e.g., Array.isArray) as in the previous implementation to avoid runtime errors when currentGroups is missing or malformed.
• HistoryAbility.buildAbility contains a lot of repetitive currentGroups.some and can(Action.HistoryRead, ...) blocks per subsystem; you could simplify this by driving it from a configuration map of group keys to subjects and looping, which would make it easier to extend or change the history access rules.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In HistoryAbility.buildAbility, all the group checks assume user.currentGroups is a defined array; consider adding a defensive check (e.g., Array.isArray) as in the previous implementation to avoid runtime errors when currentGroups is missing or malformed.
- HistoryAbility.buildAbility contains a lot of repetitive `currentGroups.some` and `can(Action.HistoryRead, ...)` blocks per subsystem; you could simplify this by driving it from a configuration map of group keys to subjects and looping, which would make it easier to extend or change the history access rules.

## Individual Comments

### Comment 1
<location path="src/casl/abilities/history.ability.ts" line_range="50-51" />
<code_context>
+      });
+    }
+
+    if (
+      user.currentGroups.some((g) =>
+        this.accessGroups?.historyAttachments.includes(g),
+      )
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against missing or non-array `currentGroups` to avoid runtime errors

This change assumes `user.currentGroups` is always a defined array. If it’s `undefined`, `null`, or not an array (e.g. older tokens or partially populated users), `user.currentGroups.some(...)` will throw. Please add back a guard (or normalize `currentGroups` earlier) before calling `some`.
</issue_to_address>

### Comment 2
<location path="src/history/history.controller.ts" line_range="117-122" />
<code_context>
-        this.subsystemActionMap[
-          filter.subsystem as keyof typeof this.subsystemActionMap
-        ];
+    const subject =
+      this.subsystemSubjectMap[
+        filter.subsystem as keyof typeof this.subsystemSubjectMap
+      ];

-      if (!requiredAction || !ability.can(requiredAction, "GenericHistory")) {
-        throw new ForbiddenException(
-          `You don't have permission to access history for ${filter.subsystem} collection`,
-        );
-      }
+    if (!subject || !ability.can(Action.HistoryRead, subject)) {
+      throw new ForbiddenException(
+        `You don't have permission to access history for ${filter.subsystem} collection`,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Differentiate between missing/invalid `subsystem` and lack of permission for clearer API errors

This change now returns `ForbiddenException` both when `filter.subsystem` is missing/invalid and when the user truly lacks permission. Previously, missing `subsystem` for non-admins was a `BadRequestException`, and admins could omit it. If that behavior is still desired, consider: (1) using `BadRequestException` when `filter.subsystem` is falsy or not in `subsystemSubjectMap`, and (2) reserving `ForbiddenException` for cases where a valid subject exists but `ability.can(...)` fails, to keep error messages accurate and avoid cases like `... for undefined collection`.
</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.}

[HayenNico]

Re: Admin level access

There appears to be a mismatch between the GET /history and GET /history/count endpoints - the former allows admins to omit the subsystem filter, the latter does not. Should this be fixed here (will make this breaking instead of refactor)?