fix(attachmentV4): allow elevated users to access attchment endpoint

[Junjiequan]

Description

This PR adds back the missing CASL ability check for elevated attachment group users to access attachment endpoints.

Motivation

Fixes

• Bug fixed (#X)

Changes:

• changes made

Tests included

• Included for each change/fix?
• Passing?

Documentation

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

official documentation info

Summary by Sourcery

Bug Fixes:

• Allow elevated attachment group users to access attachment create, update, and delete endpoints via CASL ability checks.

[sourcery-ai]

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

• The repeated can(Action.Attachment*Endpoint, Attachment) calls in each branch could be consolidated (e.g., compute a single boolean for attachment endpoint access and then apply the three can rules once) to reduce duplication and risk of divergence between branches.
• The condition this.accessGroups?.attachment.includes("#all") will throw if accessGroups exists but attachment is undefined; consider using optional chaining on attachment as well (e.g., this.accessGroups?.attachment?.includes("#all")) or normalizing attachment to an empty array.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The repeated `can(Action.Attachment*Endpoint, Attachment)` calls in each branch could be consolidated (e.g., compute a single boolean for attachment endpoint access and then apply the three `can` rules once) to reduce duplication and risk of divergence between branches.
- The condition `this.accessGroups?.attachment.includes("#all")` will throw if `accessGroups` exists but `attachment` is undefined; consider using optional chaining on `attachment` as well (e.g., `this.accessGroups?.attachment?.includes("#all")`) or normalizing `attachment` to an empty array.

## Individual Comments

### Comment 1
<location path="src/casl/casl-ability.factory.ts" line_range="426" />
<code_context>
         can(Action.AttachmentCreateEndpoint, Attachment);
         can(Action.AttachmentUpdateEndpoint, Attachment);
         can(Action.AttachmentDeleteEndpoint, Attachment);
+      } else if (
+        user.currentGroups.some((g) =>
+          this.accessGroups?.attachmentPrivileged.includes(g),
</code_context>
<issue_to_address>
**issue (complexity):** Consider consolidating the three repeated group-check branches into a single attachment-access predicate and shared capability block to simplify the authorization logic and remove duplication.

You can reduce branching complexity by factoring the three equivalent branches into a single predicate and a single capability block.

Currently:

```ts
if (user.currentGroups.some((g) => this.accessGroups?.admin.includes(g))) {
  can(Action.AttachmentCreateEndpoint, Attachment);
  can(Action.AttachmentUpdateEndpoint, Attachment);
  can(Action.AttachmentDeleteEndpoint, Attachment);
} else if (
  user.currentGroups.some((g) =>
    this.accessGroups?.attachmentPrivileged.includes(g),
  )
) {
  can(Action.AttachmentCreateEndpoint, Attachment);
  can(Action.AttachmentUpdateEndpoint, Attachment);
  can(Action.AttachmentDeleteEndpoint, Attachment);
} else if (
  user.currentGroups.some((g) =>
    this.accessGroups?.attachment.includes(g),
  ) ||
  this.accessGroups?.attachment.includes("#all")
) {
  can(Action.AttachmentCreateEndpoint, Attachment);
  can(Action.AttachmentUpdateEndpoint, Attachment);
  can(Action.AttachmentDeleteEndpoint, Attachment);
}
```

Suggested refactor (same behavior, less duplication):

```ts
const hasAttachmentAccess =
  user.currentGroups.some((g) => this.accessGroups?.admin.includes(g)) ||
  user.currentGroups.some((g) =>
    this.accessGroups?.attachmentPrivileged.includes(g),
  ) ||
  user.currentGroups.some((g) =>
    this.accessGroups?.attachment.includes(g),
  ) ||
  this.accessGroups?.attachment.includes("#all");

if (hasAttachmentAccess) {
  can(Action.AttachmentCreateEndpoint, Attachment);
  can(Action.AttachmentUpdateEndpoint, Attachment);
  can(Action.AttachmentDeleteEndpoint, Attachment);
}
```

If you want to keep the comments about which groups grant access, you can move that into a small helper for readability:

```ts
private hasAttachmentAccess(user: User): boolean {
  const { admin, attachmentPrivileged, attachment } = this.accessGroups ?? {};

  return (
    user.currentGroups.some((g) => admin?.includes(g)) ||           // ADMIN_GROUPS
    user.currentGroups.some((g) => attachmentPrivileged?.includes(g)) || // ATTACHMENT_PRIVILEGED_GROUPS
    user.currentGroups.some((g) => attachment?.includes(g)) ||      // ATTACHMENT_GROUPS
    attachment?.includes("#all")
  );
}

// usage:
if (this.hasAttachmentAccess(user)) {
  can(Action.AttachmentCreateEndpoint, Attachment);
  can(Action.AttachmentUpdateEndpoint, Attachment);
  can(Action.AttachmentDeleteEndpoint, Attachment);
}
```

This keeps all existing functionality while making the authorization logic easier to scan and maintain.
</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.}

[jl-wynen]

Are there tests for these endpoints with normal user accounts? It seems like you should have caught this bug without my help.

[Junjiequan]

@jl-wynen
All the attachmentV4 endpoint tests were using admin account, I think I forgot to add tests for privileged accounts
Allowing normal users to upload attachments needs to be discussed, I will get back to you once there's decision.

[nitrosx]

Shouldn't this be cannot?
As it is now, attachment privileged and attachment do not have difference.

[Junjiequan]

Endpoint wise there shouldnt be differences, both group users should be able to access the endpoint. The difference should be in the instance level

[nitrosx]

OK.
I will consolidate the two if branches.

[Junjiequan]

I suggeste we open a new PR and consolidates all of the ifs that has same permission logic. Instead of just do it for the attachmentEndpointAccess