[knQzx]

Creating a datablock only checked the caller supplied ownerGroup, so a user could attach a datablock to another owner's dataset and inflate its size and file count. This looks up the real dataset, enforces the datablock create permission on it, and takes ownerGroup and access groups from the dataset.

Summary by Sourcery

Secure datablock creation and update by validating access to the target dataset and using its authoritative ownership metadata.

Bug Fixes:

• Authorize datablock creation and update against the target dataset and derive datablock ownership and access groups from that dataset.

Enhancements:

• Return a not-found error when the target dataset does not exist before creating a datablock.

[sourcery-ai]

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

• The new generateDatasetInstanceForPermissions helper duplicates the pattern in generateDatablockInstanceForPermissions; consider reusing a common abstraction or clarifying why datasets require a separate helper to avoid drift between the two.
• You now perform both a dataset-level permission check (DatasetDatablockCreate) and a datablock-level check (DatablockCreateInstance); it would be helpful to clarify or centralize the distinction between these checks to avoid redundant or inconsistent authorization logic.
• The created datablock now unconditionally takes ownerGroup, accessGroups, and instrumentGroup from the dataset; if there are legitimate cases where the client should override these, consider making that behavior explicit or adding validation to surface misconfigurations earlier.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The new `generateDatasetInstanceForPermissions` helper duplicates the pattern in `generateDatablockInstanceForPermissions`; consider reusing a common abstraction or clarifying why datasets require a separate helper to avoid drift between the two.
- You now perform both a dataset-level permission check (`DatasetDatablockCreate`) and a datablock-level check (`DatablockCreateInstance`); it would be helpful to clarify or centralize the distinction between these checks to avoid redundant or inconsistent authorization logic.
- The created datablock now unconditionally takes `ownerGroup`, `accessGroups`, and `instrumentGroup` from the dataset; if there are legitimate cases where the client should override these, consider making that behavior explicit or adding validation to surface misconfigurations earlier.

## Individual Comments

### Comment 1
<location path="src/datablocks/datablocks.controller.ts" line_range="73" />
<code_context>
+    instance.accessGroups = dataset.accessGroups || [];
+    instance.ownerGroup = dataset.ownerGroup;
+    instance.sharedWith = dataset.sharedWith;
+    instance.isPublished = dataset.isPublished || false;
+
+    return instance;
</code_context>
<issue_to_address>
**🚨 issue (security):** Dataset instance for permissions omits `instrumentGroup`, potentially causing mismatches with later access checks.

`generateDatasetInstanceForPermissions` omits `instrumentGroup`, while `datablock` later reads `instrumentGroup` from `dataset`. If any CASL rules for `Action.DatasetDatablockCreate` use `instrumentGroup`, the permission check may be evaluated against different data, leading to incorrect allow/deny decisions. Please include `instrumentGroup` in the permission instance so it matches the data used for access control.
</issue_to_address>

### Comment 2
<location path="src/datablocks/datablocks.controller.ts" line_range="64" />
<code_context>
     return instance;
   }

+  private generateDatasetInstanceForPermissions(
+    dataset: DatasetClass,
+  ): DatasetClass {
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the create flow to centralize permission checks and move dataset permission mapping into a dedicated helper to keep the controller focused on orchestration.

The controller is now doing low-level permission-object construction and orchestrating two separate checks. You can keep the current behavior but reduce complexity by:

1. **Centralizing the permission checks into one helper**
2. **Moving the dataset permission projection out of the controller**

### 1. Centralize permission checks

Instead of doing `datasetAbility.cannot(...)` and `this.checkPermission(...)` separately in the `create` handler, encapsulate the logic in a single method. This keeps the controller focused on orchestration and hides the CASL details:

```ts
// inside DatablocksController

private checkCreatePermissions(
  req: Request,
  dataset: DatasetClass,
  datablock: Datablock | CreateDatablockDto,
) {
  const user: JWTUser = req.user as JWTUser;

  const datasetAbility = this.caslAbilityFactory.datasetAccess(user);
  if (
    datasetAbility.cannot(
      Action.DatasetDatablockCreate,
      this.toDatasetPermissionView(dataset),
    )
  ) {
    throw new ForbiddenException("Unauthorized access");
  }

  const datablockAbility = this.caslAbilityFactory.datablockInstanceAccess(user);
  if (
    !datablockAbility.can(
      Action.DatablockCreateInstance,
      this.generateDatablockInstanceForPermissions(datablock),
    )
  ) {
    throw new ForbiddenException("Unauthorized access");
  }
}
```

Then `create` becomes simpler:

```ts
@Post()
async create(
  @Req() req: Request,
  @Body() createDatablockDto: CreateDatablockDto,
): Promise<Datablock> {
  const dataset = await this.datasetsService.findOne({
    where: { pid: createDatablockDto.datasetId },
  });
  if (!dataset) {
    throw new NotFoundException(`dataset: ${createDatablockDto.datasetId} not found`);
  }

  const datablock = {
    ...createDatablockDto,
    ownerGroup: dataset.ownerGroup,
    accessGroups: dataset.accessGroups,
    instrumentGroup: dataset.instrumentGroup,
  };

  this.checkCreatePermissions(req, dataset, datablock);

  try {
    return await this.datablocksService.createAndUpdateDatasetSizeAndFileCount(datablock);
  } catch (error) {
    // unchanged error handling
  }
}
```

This keeps both checks but makes the controller logic easier to follow and reduces the need to reason about when each ability is invoked.

### 2. Move dataset permission projection out of the controller

`generateDatasetInstanceForPermissions` partially duplicates `DatasetClass` construction and lives in the controller. You can turn it into a small mapper, either in the CASL ability factory or in a dedicated helper, and have the controller call that.

For example, in a separate file (or in `casl-ability.factory.ts`):

```ts
// src/datasets/dataset-permission.mapper.ts
import { DatasetClass } from "src/datasets/schemas/dataset.schema";

export type DatasetPermissionView = Pick<
  DatasetClass,
  "pid" | "accessGroups" | "ownerGroup" | "sharedWith" | "isPublished"
>;

export function toDatasetPermissionView(dataset: DatasetClass): DatasetPermissionView {
  return {
    pid: dataset.pid || "",
    accessGroups: dataset.accessGroups || [],
    ownerGroup: dataset.ownerGroup,
    sharedWith: dataset.sharedWith,
    isPublished: dataset.isPublished || false,
  };
}
```

Then in the controller:

```ts
import { toDatasetPermissionView } from "src/datasets/dataset-permission.mapper";

// ...

datasetAbility.cannot(
  Action.DatasetDatablockCreate,
  toDatasetPermissionView(dataset),
);
```

This removes the need to instantiate a synthetic `DatasetClass` in the controller, reduces coupling to the entity shape, and keeps the permission-specific mapping in a single, reusable place.
</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.}

[omkar-ethz]

Thanks @HayenNico for cleaning up the PR!
I also implemented the fix for PATCH, as the same issue was present there. And added an e2e test each for the new behavior.

[omkar-ethz]

Seems to duplicate method of the same name from DatasetsController. To be refactored: #2901