refactor: casl factory - datablocks

[HayenNico]

Description

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

This unifies the datablockEndpointAccess and datablockInstanceAccess functions in CaslAbilityFactory, removes instance-level Action elements and adjusts the affected controller to accommodate the change. The datablock-specific auth code is extracted into a separate module.

Changes:

• Replace CaslAbilityFactory.datablockInstanceAccess and CaslAbilityFactory.datablockEndpointAccess with one function CaslAbilityFactory.datablockAccess
• Code for CaslAbilityFactory.datablockAccessis factored out into new module DatablockAbility
• Remove all instance-level attachment Action elements, rename endpoint-level actions to cover both
• 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 datablock authorization into a single CASL ability and update the datablocks controller to use consolidated actions.

Enhancements:

• Extract datablock authorization rules into a dedicated DatablockAbility service and integrate it with CaslAbilityFactory via a unified datablockAccess method.
• Simplify datablock CASL actions by replacing separate endpoint/instance/admin variants with a single set of CRUD actions.
• Adjust datablocks controller authorization checks and policies to rely on the new unified datablock ability and actions.

[sourcery-ai]

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

• The change from DatablockReadAny to a single DatablockRead action in the controller filter logic (e.g. deciding whether to apply addAccessControlFilters) may inadvertently treat any conditional read ability as a global read; consider explicitly distinguishing between scoped vs unrestricted read (e.g. by using a separate admin/unrestricted action or a helper that inspects rule conditions) before skipping access-control filters.
• In DatablockAbility, accessGroups is left untyped and initialized via the constructor; consider giving it an explicit type (e.g. private accessGroups: AccessGroupsType | undefined) and possibly extracting the shared build({ detectSubjectType: ... }) pattern into a small helper to reduce repetition with other ability builders.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The change from `DatablockReadAny` to a single `DatablockRead` action in the controller filter logic (e.g. deciding whether to apply `addAccessControlFilters`) may inadvertently treat any conditional read ability as a global read; consider explicitly distinguishing between scoped vs unrestricted read (e.g. by using a separate admin/unrestricted action or a helper that inspects rule conditions) before skipping access-control filters.
- In `DatablockAbility`, `accessGroups` is left untyped and initialized via the constructor; consider giving it an explicit type (e.g. `private accessGroups: AccessGroupsType | undefined`) and possibly extracting the shared `build({ detectSubjectType: ... })` pattern into a small helper to reduce repetition with other ability builders.

## Individual Comments

### Comment 1
<location path="src/datablocks/datablocks.controller.ts" line_range="144-146" />
<code_context>
     let datablockFilter: IFilters<DatablockDocument> = filter ?? {};
     const user: JWTUser = request.user as JWTUser;
-    const abilities = this.caslAbilityFactory.datablockInstanceAccess(user);
+    const abilities = this.caslAbilityFactory.datablockAccess(user);

-    if (abilities.cannot(Action.DatablockReadAny, Datablock)) {
+    if (abilities.cannot(Action.DatablockRead, Datablock)) {
       datablockFilter = addAccessControlFilters(datablockFilter, user);
     }
</code_context>
<issue_to_address>
**🚨 issue (security):** Using `cannot(Action.DatablockRead, Datablock)` to decide if filters should be applied likely grants unfiltered access to many users.

With `DatablockAbility`, most authenticated users now have conditional `can(Action.DatablockRead, Datablock, conditions)` rules (e.g. `ifOwner`, `ifAccess`, `ifPublished`). In CASL, `can(action, SubjectClass)` ignores conditions and returns `true` if any rule exists for that subject, so `cannot(Action.DatablockRead, Datablock)` will be `false` for regular users too. That means `addAccessControlFilters` will never run and all datablocks become visible.

To keep only privileged groups bypassing filters, either:
- Reintroduce a separate unrestricted action (e.g. `DatablockReadAny`) and check that, or
- Change the check to detect an unconditional rule (e.g. evaluate against an instance or use an explicit flag/group check) instead of testing with the `Datablock` class.
</issue_to_address>

### Comment 2
<location path="src/datablocks/datablocks.controller.ts" line_range="70-73" />
<code_context>
   ) {
     const user: JWTUser = request.user as JWTUser;
-    const ability = this.caslAbilityFactory.datablockInstanceAccess(user);
+    const ability = this.caslAbilityFactory.datablockAccess(user);

     if (
</code_context>
<issue_to_address>
**🚨 issue (security):** Update authorization now effectively treats most users as globally allowed to update, due to the `cannot(Action.DatablockUpdate, Datablock)` fallback.

Because `DatablockAbility` defines `can(Action.DatablockUpdate, Datablock, ifOwner)` for normal users, `ability.can(Action.DatablockUpdate, Datablock)` becomes `true` for them in CASL (conditional rules still satisfy the `can` check). That makes `ability.cannot(Action.DatablockUpdate, Datablock)` `false`, so the combined condition

```ts
if (
  ability.cannot(Action.DatablockUpdate, instance) &&
  ability.cannot(Action.DatablockUpdate, Datablock)
) {
  ...forbid...
}
```

will pass (and allow updates) even when the instance-level check fails, effectively over-granting updates.

To prevent this, either:
- Use a distinct `DatablockUpdateAny` (or similar) action for elevated users as the fallback, or
- Drop the fallback and rely solely on the instance-level check if you don’t need global overrides.
</issue_to_address>

### Comment 3
<location path="src/casl/abilities/datablocks.ability.ts" line_range="55" />
<code_context>
+    can(Action.DatablockRead, Datablock, ifAccess);
+    can(Action.DatablockRead, Datablock, ifPublished);
+
+    can(Action.DatablockUpdate, Datablock, ifOwner);
+
+    if (
</code_context>
<issue_to_address>
**question (bug_risk):** The update condition changed from `accessGroups`-based to `ownerGroup`-based, which may unintentionally restrict who can update a datablock.

Previously, updates were allowed for any user whose group was in `accessGroups`:

```ts
can(Action.DatablockUpdateInstance, Datablock, {
  accessGroups: { $in: user.currentGroups },
});
```

Now updates require membership in `ownerGroup` only:

```ts
const ifOwner = { ownerGroup: { $in: user.currentGroups } };
...
can(Action.DatablockUpdate, Datablock, ifOwner);
```

This removes update rights from users who have `accessGroups` membership but are not in `ownerGroup`. If that’s not intended, consider adding an `ifAccess` condition (or combining owner + access) for the update rule to keep prior permissions.
</issue_to_address>

### Comment 4
<location path="src/casl/abilities/datablocks.ability.ts" line_range="36" />
<code_context>
+    /**
+     * Unauthenticated user
+     */
+    can(Action.DatablockRead, Datablock, ifPublished);
+
+    if (!user) {
</code_context>
<issue_to_address>
**🚨 question (security):** Unauthenticated users are now allowed to read published datablocks; confirm if this exposure is desired.

This change comes from the unconditional `can(Action.DatablockRead, Datablock, ifPublished);` placed before the `if (!user)` early return, so even a falsy `user` now gets `DatablockRead` on published items. If this public exposure is not intentional, move this rule into the authenticated-user branch or protect it behind a configuration flag/feature toggle.
</issue_to_address>

### Comment 5
<location path="src/casl/abilities/datablocks.ability.ts" line_range="57-62" />
<code_context>
+
+    can(Action.DatablockUpdate, Datablock, ifOwner);
+
+    if (
+      user.currentGroups.some(
+        (g) =>
+          this.accessGroups?.createDatasetPrivileged.includes(g) ||
+          this.accessGroups?.createDatasetWithPid.includes(g) ||
+          this.accessGroups?.createDataset.includes(g),
+      )
+    ) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Optional chaining is only applied at the first level, so calling `.includes` may throw if `accessGroups` or the nested arrays are undefined.

Here, `this.accessGroups?.createDatasetPrivileged` (and the other arrays) become `undefined` when `accessGroups` is missing, and then `.includes` is called on `undefined`, causing a runtime error.

Consider normalizing `accessGroups` so these properties are always arrays, or use optional chaining/defaults at the array level, e.g. `(this.accessGroups?.createDatasetPrivileged ?? []).includes(g)` (and similarly for the other properties).

Suggested implementation:

```typescript
    if (
      user.currentGroups.some(
        (g) =>
          (this.accessGroups?.createDatasetPrivileged ?? []).includes(g) ||
          (this.accessGroups?.createDatasetWithPid ?? []).includes(g) ||
          (this.accessGroups?.createDataset ?? []).includes(g),
      )
    ) {

```

```typescript
    if (
      user.currentGroups.some(
        (g) => (this.accessGroups?.admin ?? []).includes(g),
      )
    ) {

```
</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.}