From 2cfce47b3015292f73d6c13cd3480177ba75c4e7 Mon Sep 17 00:00:00 2001 From: till_schuetze Date: Mon, 21 Oct 2024 15:02:19 +0200 Subject: [PATCH 1/4] fix: don't allow to update usage status or work status if not masterEditor on current or future workgroup of the edited asset, even for admins --- .../asset-edit/asset-edit.controller.ts | 55 +++++++++++++++++-- .../src/features/asset-edit/asset-edit.http | 3 +- .../asset-editor-tab-usage.component.ts | 6 +- libs/shared/v2/src/lib/models/user.ts | 3 - 4 files changed, 54 insertions(+), 13 deletions(-) diff --git a/apps/server-asset-sg/src/features/asset-edit/asset-edit.controller.ts b/apps/server-asset-sg/src/features/asset-edit/asset-edit.controller.ts index 1197eb44..1593ac2c 100644 --- a/apps/server-asset-sg/src/features/asset-edit/asset-edit.controller.ts +++ b/apps/server-asset-sg/src/features/asset-edit/asset-edit.controller.ts @@ -1,9 +1,8 @@ import { PatchAsset } from '@asset-sg/shared'; -import { User } from '@asset-sg/shared/v2'; -import { Role } from '@asset-sg/shared/v2'; -import { AssetEditPolicy } from '@asset-sg/shared/v2'; +import { AssetEditPolicy, Role, User } from '@asset-sg/shared/v2'; import { Controller, Get, HttpException, HttpStatus, Param, ParseIntPipe, Post, Put } from '@nestjs/common'; import * as E from 'fp-ts/Either'; +import * as O from 'fp-ts/Option'; import { authorize } from '@/core/authorize'; import { CurrentUser } from '@/core/decorators/current-user.decorator'; import { ParseBody } from '@/core/decorators/parse.decorator'; @@ -69,16 +68,60 @@ const validatePatch = (user: User, patch: PatchAsset, record?: AssetEditDetail) ); } + const checkStatusChange = ( + hasChanged: boolean, + newStatus: string, + allowedStatus: string, + errorMessage: string, + record: AssetEditDetail | undefined, + policy: AssetEditPolicy, + patchWorkgroupId: number + ) => { + if ( + hasChanged && + newStatus !== allowedStatus && + ((record != null && !policy.hasRole(Role.MasterEditor, record.workgroupId)) || + !policy.hasRole(Role.MasterEditor, patchWorkgroupId)) + ) { + throw new HttpException(errorMessage, HttpStatus.FORBIDDEN); + } + }; + // Specialization of the policy where we disallow the internal status to be changed to anything else than `tobechecked` // if the current user is not a master-editor for the asset's current or future workgroup. const hasInternalUseChanged = record == null || record.internalUse.statusAssetUseItemCode !== patch.internalUse.statusAssetUseItemCode; + checkStatusChange( + hasInternalUseChanged, + patch.internalUse.statusAssetUseItemCode, + 'tobechecked', + "Changing the asset's status is not allowed", + record, + policy, + patch.workgroupId + ); + + // Specialization of the policy where we disallow the public status to be changed to anything else than `tobechecked` + // if the current user is not a master-editor for the asset's current or future workgroup. + const hasPublicUseChanged = + record == null || record.publicUse.statusAssetUseItemCode !== patch.publicUse.statusAssetUseItemCode; + checkStatusChange( + hasPublicUseChanged, + patch.publicUse.statusAssetUseItemCode, + 'tobechecked', + "Changing the asset's status is not allowed", + record, + policy, + patch.workgroupId + ); + + // Specialization of the policy where we disallow the status work item code to be changed to `published` + // if the current user is not a master-editor for the asset's current or future workgroup. if ( - hasInternalUseChanged && - patch.internalUse.statusAssetUseItemCode !== 'tobechecked' && + O.toNullable(patch.newStatusWorkItemCode) === 'published' && ((record != null && !policy.hasRole(Role.MasterEditor, record.workgroupId)) || !policy.hasRole(Role.MasterEditor, patch.workgroupId)) ) { - throw new HttpException("Changing the asset's status is not allowed", HttpStatus.FORBIDDEN); + throw new HttpException("Changing the asset's working status is not allowed", HttpStatus.FORBIDDEN); } }; diff --git a/apps/server-asset-sg/src/features/asset-edit/asset-edit.http b/apps/server-asset-sg/src/features/asset-edit/asset-edit.http index b11027c7..c421a664 100644 --- a/apps/server-asset-sg/src/features/asset-edit/asset-edit.http +++ b/apps/server-asset-sg/src/features/asset-edit/asset-edit.http @@ -71,5 +71,6 @@ Content-Type: application/json "titleOriginal": "My Cool Asset", "titlePublic": "Our Cool Asset", "typeNatRels": [], - "workgroupId": 1 + "workgroupId": 1, + "assetFiles": [] } diff --git a/libs/asset-editor/src/lib/components/asset-editor-tab-usage/asset-editor-tab-usage.component.ts b/libs/asset-editor/src/lib/components/asset-editor-tab-usage/asset-editor-tab-usage.component.ts index 0c7da25a..b8499200 100644 --- a/libs/asset-editor/src/lib/components/asset-editor-tab-usage/asset-editor-tab-usage.component.ts +++ b/libs/asset-editor/src/lib/components/asset-editor-tab-usage/asset-editor-tab-usage.component.ts @@ -1,8 +1,8 @@ import { Dialog, DialogRef } from '@angular/cdk/dialog'; -import { ChangeDetectionStrategy, Component, Input, OnInit, TemplateRef, ViewChild, inject } from '@angular/core'; +import { ChangeDetectionStrategy, Component, inject, Input, OnInit, TemplateRef, ViewChild } from '@angular/core'; import { FormGroupDirective } from '@angular/forms'; import { MatCheckboxChange } from '@angular/material/checkbox'; -import { LifecycleHooks, LifecycleHooksDirective, fromAppShared } from '@asset-sg/client-shared'; +import { fromAppShared, LifecycleHooks, LifecycleHooksDirective } from '@asset-sg/client-shared'; import { isNotNull } from '@asset-sg/core'; import { isMasterEditor } from '@asset-sg/shared/v2'; import * as RD from '@devexperts/remote-data-ts'; @@ -12,7 +12,7 @@ import { TranslateService } from '@ngx-translate/core'; import { RxState } from '@rx-angular/state'; import * as O from 'fp-ts/Option'; -import { Observable, map, of, startWith, switchMap, withLatestFrom, filter } from 'rxjs'; +import { filter, map, Observable, of, startWith, switchMap, withLatestFrom } from 'rxjs'; import { AssetEditDetailVM } from '../../models'; import { AssetEditorFormGroup, AssetEditorUsageFormGroup } from '../asset-editor-form-group'; diff --git a/libs/shared/v2/src/lib/models/user.ts b/libs/shared/v2/src/lib/models/user.ts index 2213d554..adeb31e0 100644 --- a/libs/shared/v2/src/lib/models/user.ts +++ b/libs/shared/v2/src/lib/models/user.ts @@ -19,9 +19,6 @@ const hasRole = (role: Role) => (user: User | null | undefined, workgroupId?: Wo if (user == null) { return false; } - if (user.isAdmin) { - return true; - } const roleIndex = getRoleIndex(role); if (workgroupId != null) { const role = user.roles.get(workgroupId); From e43f2682b31a9e7a992d545a15727816e4851d83 Mon Sep 17 00:00:00 2001 From: till_schuetze Date: Thu, 24 Oct 2024 14:46:21 +0200 Subject: [PATCH 2/4] Fix: refactor inner function --- .../asset-edit/asset-edit.controller.ts | 49 +++++-------------- 1 file changed, 12 insertions(+), 37 deletions(-) diff --git a/apps/server-asset-sg/src/features/asset-edit/asset-edit.controller.ts b/apps/server-asset-sg/src/features/asset-edit/asset-edit.controller.ts index 1593ac2c..74d70940 100644 --- a/apps/server-asset-sg/src/features/asset-edit/asset-edit.controller.ts +++ b/apps/server-asset-sg/src/features/asset-edit/asset-edit.controller.ts @@ -68,52 +68,27 @@ const validatePatch = (user: User, patch: PatchAsset, record?: AssetEditDetail) ); } - const checkStatusChange = ( - hasChanged: boolean, - newStatus: string, - allowedStatus: string, - errorMessage: string, - record: AssetEditDetail | undefined, - policy: AssetEditPolicy, - patchWorkgroupId: number - ) => { - if ( + const checkStatusChange = (key: 'internalUse' | 'publicUse', newStatus: string) => { + const hasChanged = record == null || record[key].statusAssetUseItemCode !== patch[key].statusAssetUseItemCode; + return ( hasChanged && - newStatus !== allowedStatus && + newStatus !== 'tobechecked' && ((record != null && !policy.hasRole(Role.MasterEditor, record.workgroupId)) || - !policy.hasRole(Role.MasterEditor, patchWorkgroupId)) - ) { - throw new HttpException(errorMessage, HttpStatus.FORBIDDEN); - } + !policy.hasRole(Role.MasterEditor, patch.workgroupId)) + ); }; // Specialization of the policy where we disallow the internal status to be changed to anything else than `tobechecked` // if the current user is not a master-editor for the asset's current or future workgroup. - const hasInternalUseChanged = - record == null || record.internalUse.statusAssetUseItemCode !== patch.internalUse.statusAssetUseItemCode; - checkStatusChange( - hasInternalUseChanged, - patch.internalUse.statusAssetUseItemCode, - 'tobechecked', - "Changing the asset's status is not allowed", - record, - policy, - patch.workgroupId - ); + const hasInternalUSeChanged = checkStatusChange('internalUse', patch.internalUse.statusAssetUseItemCode); // Specialization of the policy where we disallow the public status to be changed to anything else than `tobechecked` // if the current user is not a master-editor for the asset's current or future workgroup. - const hasPublicUseChanged = - record == null || record.publicUse.statusAssetUseItemCode !== patch.publicUse.statusAssetUseItemCode; - checkStatusChange( - hasPublicUseChanged, - patch.publicUse.statusAssetUseItemCode, - 'tobechecked', - "Changing the asset's status is not allowed", - record, - policy, - patch.workgroupId - ); + const hasPublicUSeChanged = checkStatusChange('publicUse', patch.publicUse.statusAssetUseItemCode); + + if (hasInternalUSeChanged || hasPublicUSeChanged) { + throw new HttpException("Changing the asset's status is not allowed", HttpStatus.FORBIDDEN); + } // Specialization of the policy where we disallow the status work item code to be changed to `published` // if the current user is not a master-editor for the asset's current or future workgroup. From bf1c4620a848185bdbbebcc6443c5f9a4ede80a7 Mon Sep 17 00:00:00 2001 From: till_schuetze Date: Tue, 29 Oct 2024 08:33:53 +0100 Subject: [PATCH 3/4] refactor status change methode --- .../features/asset-edit/asset-edit.controller.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/apps/server-asset-sg/src/features/asset-edit/asset-edit.controller.ts b/apps/server-asset-sg/src/features/asset-edit/asset-edit.controller.ts index 74d70940..fb29d153 100644 --- a/apps/server-asset-sg/src/features/asset-edit/asset-edit.controller.ts +++ b/apps/server-asset-sg/src/features/asset-edit/asset-edit.controller.ts @@ -68,11 +68,11 @@ const validatePatch = (user: User, patch: PatchAsset, record?: AssetEditDetail) ); } - const checkStatusChange = (key: 'internalUse' | 'publicUse', newStatus: string) => { + const hasStatusChanged = (key: 'internalUse' | 'publicUse') => { const hasChanged = record == null || record[key].statusAssetUseItemCode !== patch[key].statusAssetUseItemCode; return ( hasChanged && - newStatus !== 'tobechecked' && + patch[key].statusAssetUseItemCode !== 'tobechecked' && ((record != null && !policy.hasRole(Role.MasterEditor, record.workgroupId)) || !policy.hasRole(Role.MasterEditor, patch.workgroupId)) ); @@ -80,14 +80,14 @@ const validatePatch = (user: User, patch: PatchAsset, record?: AssetEditDetail) // Specialization of the policy where we disallow the internal status to be changed to anything else than `tobechecked` // if the current user is not a master-editor for the asset's current or future workgroup. - const hasInternalUSeChanged = checkStatusChange('internalUse', patch.internalUse.statusAssetUseItemCode); + if (hasStatusChanged('internalUse')) { + throw new HttpException("Changing the asset's internalUse status is not allowed", HttpStatus.FORBIDDEN); + } // Specialization of the policy where we disallow the public status to be changed to anything else than `tobechecked` // if the current user is not a master-editor for the asset's current or future workgroup. - const hasPublicUSeChanged = checkStatusChange('publicUse', patch.publicUse.statusAssetUseItemCode); - - if (hasInternalUSeChanged || hasPublicUSeChanged) { - throw new HttpException("Changing the asset's status is not allowed", HttpStatus.FORBIDDEN); + if (hasStatusChanged('publicUse')) { + throw new HttpException("Changing the asset's public use status is not allowed", HttpStatus.FORBIDDEN); } // Specialization of the policy where we disallow the status work item code to be changed to `published` From 339befec5a6aa3dee66dbe161c910975caa40f7a Mon Sep 17 00:00:00 2001 From: till_schuetze Date: Wed, 30 Oct 2024 14:47:49 +0100 Subject: [PATCH 4/4] Empty-Commit