From d4b1f1da4be92120461ca0190dff4efec2b01d47 Mon Sep 17 00:00:00 2001 From: HayenNico <141850426+HayenNico@users.noreply.github.com> Date: Thu, 4 Jun 2026 00:02:45 +0200 Subject: [PATCH 1/2] refactor datablocks casl & controller --- src/casl/abilities/datablocks.ability.ts | 96 ++++++++++++++++++++++++ src/casl/action.enum.ts | 22 ++---- src/casl/casl-ability.factory.ts | 93 ++--------------------- src/casl/casl.module.ts | 4 +- src/datablocks/datablocks.controller.ts | 44 +++++------ 5 files changed, 132 insertions(+), 127 deletions(-) create mode 100644 src/casl/abilities/datablocks.ability.ts diff --git a/src/casl/abilities/datablocks.ability.ts b/src/casl/abilities/datablocks.ability.ts new file mode 100644 index 000000000..93ec218f4 --- /dev/null +++ b/src/casl/abilities/datablocks.ability.ts @@ -0,0 +1,96 @@ +import { + AbilityBuilder, + ExtractSubjectType, + MongoAbility, + createMongoAbility, +} from "@casl/ability"; +import { Injectable } from "@nestjs/common"; +import { ConfigService } from "@nestjs/config"; +import { AccessGroupsType } from "src/config/configuration"; +import { Action } from "../action.enum"; +import { + Subjects, + PossibleAbilities, + Conditions, +} from "../types/casl-subjects"; +import { JWTUser } from "src/auth/interfaces/jwt-user.interface"; +import { Datablock } from "src/datablocks/schemas/datablock.schema"; + +@Injectable() +export class DatablockAbility { + constructor(private configService: ConfigService) { + this.accessGroups = + this.configService.get("accessGroups"); + } + private accessGroups; + + buildAbility(user: JWTUser): MongoAbility { + const { can, build } = new AbilityBuilder( + createMongoAbility, + ); + const ifPublished = { isPublished: true }; + + /** + * Unauthenticated user + */ + can(Action.DatablockRead, Datablock, ifPublished); + + if (!user) { + return build({ + detectSubjectType: (item) => + item.constructor as ExtractSubjectType, + }); + } + + const ifOwner = { ownerGroup: { $in: user.currentGroups } }; + const ifAccess = { accessGroups: { $in: user.currentGroups } }; + + /** + * Authenticated user + */ + can(Action.DatablockRead, Datablock, ifOwner); + can(Action.DatablockRead, Datablock, ifAccess); + can(Action.DatablockRead, Datablock, ifPublished); + + 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), + ) + ) { + /** + * User belonging to CREATE_DATASET_PRIVILEGED_GROUPS, + * CREATE_DATASET_WITH_PID_GROUPS or CREATE_DATASET_GROUPS + */ + can(Action.DatablockCreate, Datablock); + can(Action.DatablockUpdate, Datablock); + } + + if (user.currentGroups.some((g) => this.accessGroups?.admin.includes(g))) { + /** + * User belonging to ADMIN_GROUPS + */ + can(Action.DatablockCreate, Datablock); + can(Action.DatablockRead, Datablock); + can(Action.DatablockUpdate, Datablock); + } + + if (user.currentGroups.some((g) => this.accessGroups?.delete.includes(g))) { + /** + * User belonging to DELETE_GROUPS + */ + can(Action.DatablockRead, Datablock); + can(Action.DatablockUpdate, Datablock); + can(Action.DatablockDelete, Datablock); + } + + return build({ + detectSubjectType: (item) => + item.constructor as ExtractSubjectType, + }); + } +} diff --git a/src/casl/action.enum.ts b/src/casl/action.enum.ts index c1e6f3baa..6ea7e30aa 100644 --- a/src/casl/action.enum.ts +++ b/src/casl/action.enum.ts @@ -13,6 +13,12 @@ export enum Action { // Currently used by addAccessBasedFilters for admin/special group users AccessAny = "access_any", + // Datablock + DatablockCreate = "datablock_create", + DatablockRead = "datablock_read", + DatablockUpdate = "datablock_update", + DatablockDelete = "datablock_delete", + // --------------- // Datasets // endpoint authorization actions @@ -105,22 +111,6 @@ export enum Action { OrigdatablockDeleteOwner = "origdatablock_delete_owner", OrigdatablockDeleteAny = "origdatablock_delete_any", - // ------------- - // Datablock - // endpoint authorization actions - DatablockCreateEndpoint = "datablock_create_endpoint", - DatablockReadEndpoint = "datablock_read_endpoint", - DatablockUpdateEndpoint = "datablock_update_endpoint", - DatablockDeleteEndpoint = "datablock_delete_endpoint", - // individual actions - DatablockCreateInstance = "datablock_create_instance", - DatablockReadInstance = "datablock_read_instance", - DatablockUpdateInstance = "datablock_update_instance", - // admin actions - DatablockReadAny = "datablock_read_any", - DatablockUpdateAny = "datablock_update_any", - DatablockDeleteAny = "datablock_delete_any", - // Proposals // endpoint authorization actions ProposalsCreate = "proposals_create", diff --git a/src/casl/casl-ability.factory.ts b/src/casl/casl-ability.factory.ts index dc048de16..2c6047aa7 100644 --- a/src/casl/casl-ability.factory.ts +++ b/src/casl/casl-ability.factory.ts @@ -11,7 +11,6 @@ import { JobConfigService } from "src/config/job-config/jobconfig.service"; import { JWTUser } from "src/auth/interfaces/jwt-user.interface"; import { AccessGroupsType } from "src/config/configuration"; import { Attachment } from "src/attachments/schemas/attachment.schema"; -import { Datablock } from "src/datablocks/schemas/datablock.schema"; import { DatasetClass } from "src/datasets/schemas/dataset.schema"; import { Instrument } from "src/instruments/schemas/instrument.schema"; import { JobClass } from "src/jobs/schemas/job.schema"; @@ -29,6 +28,7 @@ import { SampleClass } from "src/samples/schemas/sample.schema"; import { User } from "src/users/schemas/user.schema"; import { Action } from "./action.enum"; import { Subjects, PossibleAbilities, Conditions } from "./types/casl-subjects"; +import { DatablockAbility } from "./abilities/datablocks.ability"; export type AppAbility = MongoAbility; @@ -37,6 +37,7 @@ export class CaslAbilityFactory { constructor( private configService: ConfigService, private jobConfigService: JobConfigService, + private datablockAbility: DatablockAbility, ) { this.accessGroups = this.configService.get("accessGroups"); @@ -47,7 +48,7 @@ export class CaslAbilityFactory { [endpoint: string]: (user: JWTUser) => AppAbility; } = { attachments: this.attachmentEndpointAccess, - datablocks: this.datablockEndpointAccess, + datablocks: this.datablockAccess, datasets: this.datasetEndpointAccess, history: this.historyEndpointAccess, instruments: this.instrumentEndpointAccess, @@ -74,6 +75,10 @@ export class CaslAbilityFactory { return accessFunction.call(this, user); } + datablockAccess(user: JWTUser) { + return this.datablockAbility.buildAbility(user); + } + datasetEndpointAccess(user: JWTUser) { const { can, cannot, build } = new AbilityBuilder( createMongoAbility, @@ -883,34 +888,6 @@ export class CaslAbilityFactory { }); } - datablockEndpointAccess(user: JWTUser) { - const { can, cannot, build } = new AbilityBuilder( - createMongoAbility, - ); - if (user) { - can(Action.DatablockCreateEndpoint, Datablock); - can(Action.DatablockReadEndpoint, Datablock); - can(Action.DatablockUpdateEndpoint, Datablock); - - if ( - user.currentGroups.some((g) => this.accessGroups?.delete.includes(g)) - ) { - can(Action.DatablockDeleteEndpoint, Datablock); - } else { - cannot(Action.DatablockDeleteEndpoint, Datablock); - } - } else { - cannot(Action.DatablockCreateEndpoint, Datablock); - cannot(Action.DatablockReadEndpoint, Datablock); - cannot(Action.DatablockUpdateEndpoint, Datablock); - cannot(Action.DatablockDeleteEndpoint, Datablock); - } - - return build({ - detectSubjectType: (item) => - item.constructor as ExtractSubjectType, - }); - } runtimeConfigEndpointAccess(user: JWTUser) { const { can, build } = new AbilityBuilder( createMongoAbility, @@ -2379,62 +2356,6 @@ export class CaslAbilityFactory { }); } - datablockInstanceAccess(user: JWTUser) { - const { can, build } = new AbilityBuilder( - createMongoAbility, - ); - if (user) { - // Can read if user is in ownerGroup/accessGroup or if published - can(Action.DatablockReadInstance, Datablock, { - ownerGroup: { $in: user.currentGroups }, - }); - can(Action.DatablockReadInstance, Datablock, { - accessGroups: { $in: user.currentGroups }, - }); - can(Action.DatablockReadInstance, Datablock, { isPublished: true }); - - // Can update if in ownerGroup - can(Action.DatablockUpdateInstance, Datablock, { - accessGroups: { $in: user.currentGroups }, - }); - - // Ingestor group is allowed to create/update - if ( - user.currentGroups.some((g) => - this.accessGroups?.createDataset.includes(g), - ) || - user.currentGroups.some((g) => - this.accessGroups?.createDatasetPrivileged.includes(g), - ) || - user.currentGroups.some((g) => - this.accessGroups?.createDatasetWithPid.includes(g), - ) - ) { - can(Action.DatablockCreateInstance, Datablock); - can(Action.DatablockUpdateAny, Datablock); - } - - if ( - user.currentGroups.some((g) => this.accessGroups?.delete.includes(g)) - ) { - can(Action.DatablockReadAny, Datablock); - can(Action.DatablockUpdateAny, Datablock); - can(Action.DatablockDeleteAny, Datablock); - } - if ( - user.currentGroups.some((g) => this.accessGroups?.admin.includes(g)) - ) { - can(Action.DatablockCreateInstance, Datablock); - can(Action.DatablockReadAny, Datablock); - can(Action.DatablockUpdateAny, Datablock); - } - } - return build({ - detectSubjectType: (item) => - item.constructor as ExtractSubjectType, - }); - } - metadataKeyInstanceAccess(user: JWTUser) { const { can, build } = new AbilityBuilder( createMongoAbility, diff --git a/src/casl/casl.module.ts b/src/casl/casl.module.ts index 3c8e1f2d5..c0862867c 100644 --- a/src/casl/casl.module.ts +++ b/src/casl/casl.module.ts @@ -2,9 +2,11 @@ import { Module } from "@nestjs/common"; import { ConfigModule } from "@nestjs/config"; import { CaslAbilityFactory } from "./casl-ability.factory"; import { JobConfigModule } from "src/config/job-config/jobconfig.module"; +import { DatablockAbility } from "./abilities/datablocks.ability"; + @Module({ imports: [JobConfigModule, ConfigModule], - providers: [CaslAbilityFactory], + providers: [CaslAbilityFactory, DatablockAbility], exports: [CaslAbilityFactory], }) export class CaslModule {} diff --git a/src/datablocks/datablocks.controller.ts b/src/datablocks/datablocks.controller.ts index a474323a6..c10155b31 100644 --- a/src/datablocks/datablocks.controller.ts +++ b/src/datablocks/datablocks.controller.ts @@ -67,7 +67,7 @@ export class DatablocksController { action: Action, ) { const user: JWTUser = request.user as JWTUser; - const ability = this.caslAbilityFactory.datablockInstanceAccess(user); + const ability = this.caslAbilityFactory.datablockAccess(user); if ( !ability.can( @@ -81,18 +81,14 @@ export class DatablocksController { @UseGuards(PoliciesGuard) @CheckPolicies("datablocks", (ability: AppAbility) => - ability.can(Action.DatablockCreateEndpoint, Datablock), + ability.can(Action.DatablockCreate, Datablock), ) @Post() async create( @Req() req: Request, @Body() createDatablockDto: CreateDatablockDto, ): Promise { - this.checkPermission( - req, - createDatablockDto, - Action.DatablockCreateInstance, - ); + this.checkPermission(req, createDatablockDto, Action.DatablockCreate); try { const dataset = await this.datasetsService.findOne({ @@ -127,7 +123,7 @@ export class DatablocksController { @UseGuards(PoliciesGuard) @CheckPolicies("datablocks", (ability: AppAbility) => - ability.can(Action.DatablockReadEndpoint, Datablock), + ability.can(Action.DatablockRead, Datablock), ) @ApiQuery({ name: "filter", @@ -145,9 +141,9 @@ export class DatablocksController { ): Promise { let datablockFilter: IFilters = 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); } @@ -156,7 +152,7 @@ export class DatablocksController { @UseGuards(PoliciesGuard) @CheckPolicies("datablocks", (ability: AppAbility) => - ability.can(Action.DatablockReadEndpoint, Datablock), + ability.can(Action.DatablockRead, Datablock), ) @Get("/count") @ApiOperation({ @@ -213,9 +209,9 @@ export class DatablocksController { let datablockFilter: IFilters = 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); } @@ -224,7 +220,7 @@ export class DatablocksController { @UseGuards(PoliciesGuard) @CheckPolicies("datablocks", (ability: AppAbility) => - ability.can(Action.DatablockReadEndpoint, Datablock), + ability.can(Action.DatablockRead, Datablock), ) @Get(":id") async findById( @@ -232,7 +228,7 @@ export class DatablocksController { @Param("id") id: string, ): Promise { const user: JWTUser = request.user as JWTUser; - const abilities = this.caslAbilityFactory.datablockInstanceAccess(user); + const abilities = this.caslAbilityFactory.datablockAccess(user); const instance = await this.datablocksService.findOne({ where: { _id: id }, @@ -242,8 +238,8 @@ export class DatablocksController { } if ( - abilities.cannot(Action.DatablockReadInstance, instance) && - abilities.cannot(Action.DatablockReadAny, Datablock) + abilities.cannot(Action.DatablockRead, instance) && + abilities.cannot(Action.DatablockRead, Datablock) ) { throw new UnauthorizedException(); } @@ -253,7 +249,7 @@ export class DatablocksController { @UseGuards(PoliciesGuard) @CheckPolicies("datablocks", (ability: AppAbility) => - ability.can(Action.DatablockUpdateEndpoint, Datablock), + ability.can(Action.DatablockUpdate, Datablock), ) @Patch(":id") async update( @@ -266,15 +262,15 @@ export class DatablocksController { where: { _id: id }, }); const user: JWTUser = request.user as JWTUser; - const ability = this.caslAbilityFactory.datablockInstanceAccess(user); + const ability = this.caslAbilityFactory.datablockAccess(user); if (!instance) { throw new NotFoundException(); } if ( - ability.cannot(Action.DatablockUpdateInstance, instance) && - ability.cannot(Action.DatablockUpdateAny, Datablock) + ability.cannot(Action.DatablockUpdate, instance) && + ability.cannot(Action.DatablockUpdate, Datablock) ) { throw new ForbiddenException("Unauthorized to update this datablock"); } @@ -297,7 +293,7 @@ export class DatablocksController { @UseGuards(PoliciesGuard) @CheckPolicies("datablocks", (ability: AppAbility) => - ability.can(Action.DatablockDeleteEndpoint, Datablock), + ability.can(Action.DatablockDelete, Datablock), ) @Delete(":id") async remove( @@ -315,8 +311,8 @@ export class DatablocksController { throw new NotFoundException(`dataset: ${datablock.datasetId} not found`); const user: JWTUser = request.user as JWTUser; - const ability = this.caslAbilityFactory.datablockInstanceAccess(user); - if (ability.cannot(Action.DatablockDeleteAny, Datablock)) { + const ability = this.caslAbilityFactory.datablockAccess(user); + if (ability.cannot(Action.DatablockDelete, Datablock)) { throw new ForbiddenException("Unauthorized to delete this datablock"); } From 6fce9bf1d096b9a1a913d8edfc10f5307fb4d0c7 Mon Sep 17 00:00:00 2001 From: HayenNico <141850426+HayenNico@users.noreply.github.com> Date: Thu, 4 Jun 2026 02:15:25 +0200 Subject: [PATCH 2/2] fix security gaps in datablocks controller --- src/casl/abilities/datablocks.ability.ts | 2 + src/datablocks/datablocks.controller.ts | 156 +++++++++++------------ 2 files changed, 77 insertions(+), 81 deletions(-) diff --git a/src/casl/abilities/datablocks.ability.ts b/src/casl/abilities/datablocks.ability.ts index 93ec218f4..e2a8166ab 100644 --- a/src/casl/abilities/datablocks.ability.ts +++ b/src/casl/abilities/datablocks.ability.ts @@ -74,6 +74,8 @@ export class DatablockAbility { /** * User belonging to ADMIN_GROUPS */ + can(Action.AccessAny, Datablock); + can(Action.DatablockCreate, Datablock); can(Action.DatablockRead, Datablock); can(Action.DatablockUpdate, Datablock); diff --git a/src/datablocks/datablocks.controller.ts b/src/datablocks/datablocks.controller.ts index c10155b31..8a5bc47e1 100644 --- a/src/datablocks/datablocks.controller.ts +++ b/src/datablocks/datablocks.controller.ts @@ -14,7 +14,6 @@ import { Post, Query, Req, - UnauthorizedException, UseGuards, } from "@nestjs/common"; import { @@ -67,16 +66,17 @@ export class DatablocksController { action: Action, ) { const user: JWTUser = request.user as JWTUser; + const datablockInstance = + this.generateDatablockInstanceForPermissions(datablock); + const ability = this.caslAbilityFactory.datablockAccess(user); + const canDoAction = ability.can(action, datablockInstance); - if ( - !ability.can( - action, - this.generateDatablockInstanceForPermissions(datablock), - ) - ) { + if (!canDoAction) { throw new ForbiddenException(); } + + return canDoAction; } @UseGuards(PoliciesGuard) @@ -141,10 +141,11 @@ export class DatablocksController { ): Promise { let datablockFilter: IFilters = filter ?? {}; const user: JWTUser = request.user as JWTUser; - const abilities = this.caslAbilityFactory.datablockAccess(user); + const ability = this.caslAbilityFactory.datablockAccess(user); + const canViewAny = ability.can(Action.AccessAny, Datablock); - if (abilities.cannot(Action.DatablockRead, Datablock)) { - datablockFilter = addAccessControlFilters(datablockFilter, user); + if (!canViewAny) { + datablockFilter = this.addAccessControlFilters(datablockFilter, user); } return this.datablocksService.findAll(datablockFilter); @@ -207,12 +208,13 @@ export class DatablocksController { filter = where; } - let datablockFilter: IFilters = filter ?? {}; + const datablockFilter: IFilters = filter ?? {}; const user: JWTUser = request.user as JWTUser; - const abilities = this.caslAbilityFactory.datablockAccess(user); + const ability = this.caslAbilityFactory.datablockAccess(user); + const canViewAny = ability.can(Action.AccessAny, Datablock); - if (abilities.cannot(Action.DatablockRead, Datablock)) { - datablockFilter = addAccessControlFilters(datablockFilter, user); + if (!canViewAny) { + this.addAccessControlFilters(datablockFilter, user); } return this.datablocksService.count(datablockFilter); @@ -227,24 +229,17 @@ export class DatablocksController { @Req() request: Request, @Param("id") id: string, ): Promise { - const user: JWTUser = request.user as JWTUser; - const abilities = this.caslAbilityFactory.datablockAccess(user); - - const instance = await this.datablocksService.findOne({ + const datablockInstance = await this.datablocksService.findOne({ where: { _id: id }, }); - if (!instance) { + + if (!datablockInstance) { throw new NotFoundException(); } - if ( - abilities.cannot(Action.DatablockRead, instance) && - abilities.cannot(Action.DatablockRead, Datablock) - ) { - throw new UnauthorizedException(); - } + this.checkPermission(request, datablockInstance, Action.DatablockRead); - return instance; + return datablockInstance; } @UseGuards(PoliciesGuard) @@ -257,38 +252,21 @@ export class DatablocksController { @Req() request: Request, @Body() updateDatablockDto: PartialUpdateDatablockDto, ): Promise { - try { - const instance = await this.datablocksService.findOne({ - where: { _id: id }, - }); - const user: JWTUser = request.user as JWTUser; - const ability = this.caslAbilityFactory.datablockAccess(user); + const datablockInstance = await this.datablocksService.findOne({ + where: { _id: id }, + }); - if (!instance) { - throw new NotFoundException(); - } + if (!datablockInstance) { + throw new NotFoundException(); + } - if ( - ability.cannot(Action.DatablockUpdate, instance) && - ability.cannot(Action.DatablockUpdate, Datablock) - ) { - throw new ForbiddenException("Unauthorized to update this datablock"); - } + this.checkPermission(request, datablockInstance, Action.DatablockUpdate); - const datablock = await this.datablocksService.update( - { _id: id }, - updateDatablockDto, - ); - return datablock; - } catch (error) { - if ((error as MongoError).code === 11000) { - throw new ConflictException( - "A datablock with this this unique key already exists!", - ); - } else { - throw new InternalServerErrorException(error); - } - } + const datablock = await this.datablocksService.update( + { _id: id }, + updateDatablockDto, + ); + return datablock; } @UseGuards(PoliciesGuard) @@ -300,19 +278,29 @@ export class DatablocksController { @Req() request: Request, @Param("id") id: string, ): Promise { + const user: JWTUser = request.user as JWTUser; + const datablock = await this.datablocksService.findOne({ where: { _id: id }, }); - if (!datablock) throw new NotFoundException(`datablock: ${id} not found`); + if (!datablock) { + throw new NotFoundException(`datablock: ${id} not found`); + } + const dataset = await this.datasetsService.findOne({ where: { pid: datablock?.datasetId }, }); - if (!dataset) + if (!dataset) { throw new NotFoundException(`dataset: ${datablock.datasetId} not found`); + } + + const datablockInstance = + this.generateDatablockInstanceForPermissions(datablock); - const user: JWTUser = request.user as JWTUser; const ability = this.caslAbilityFactory.datablockAccess(user); - if (ability.cannot(Action.DatablockDelete, Datablock)) { + const canDelete = ability.can(Action.DatablockDelete, datablockInstance); + + if (!canDelete) { throw new ForbiddenException("Unauthorized to delete this datablock"); } @@ -326,29 +314,35 @@ export class DatablocksController { return res; } -} -function addAccessControlFilters( - datablockFilter: IFilters, - user: JWTUser, -): IFilters { - if (!datablockFilter.where) { - datablockFilter.where = {}; - } + private addAccessControlFilters( + datablockFilter: IFilters, + user: JWTUser, + ): IFilters { + datablockFilter.where = datablockFilter.where ?? {}; - const accessControlFilters = { - $or: [ - { ownerGroup: { $in: user.currentGroups || [] } }, - { accessGroups: { $in: user.currentGroups || [] } }, - { isPublished: true }, - ], - }; - - if (datablockFilter.where["$and"]) { - datablockFilter.where["$and"].push(accessControlFilters); - } else { - datablockFilter.where["$and"] = [accessControlFilters]; - } + if (!user) { + if (datablockFilter.where["$and"]) { + datablockFilter.where["$and"].push({ isPublished: true }); + } else { + datablockFilter.where["$and"] = [{ isPublished: true }]; + } + } else { + const accessControlFilters = { + $or: [ + { ownerGroup: { $in: user.currentGroups } }, + { accessGroups: { $in: user.currentGroups } }, + { isPublished: true }, + ], + }; - return datablockFilter; + if (datablockFilter.where["$and"]) { + datablockFilter.where["$and"].push(accessControlFilters); + } else { + datablockFilter.where["$and"] = [accessControlFilters]; + } + } + + return datablockFilter; + } }