Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: don't allow to update usage status or work status if not masterEditor #311

Merged
merged 4 commits into from
Oct 30, 2024
Merged

Fix: don't allow to update usage status or work status if not masterEditor #311

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2cfce47
fix: don't allow to update usage status or work status if not masterE…
TIL-EBP Oct 21, 2024
e43f268
Fix: refactor inner function
TIL-EBP Oct 24, 2024
bf1c462
refactor status change methode
TIL-EBP Oct 29, 2024
339befe
Empty-Commit
TIL-EBP Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 49 additions & 6 deletions apps/server-asset-sg/src/features/asset-edit/asset-edit.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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);
}
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some issues with this function. Note that these are just general observations - if and how you apply them is up to you. Solving some of these issues even makes others obsolete.

  • There is currently no reason for this to be a nested function (a function within another function). I like nested functions, as long as they reuse their outer scope, making their parameter list more readable. Here, the last three parameters are unnecessary - they can be replaced by accessing the outer function's scope.

  • As a general rule, I like to keep a function's parameter count at a maximum of 3 or 4. Any more than that and it becomes unclear what each argument passed to the function does. If there is no way to reduce the parameter count, use an options object, e.g:

    const checkStatusChange = (hasChanged: boolean, options: {
  newStatus: string;
  allowedStatus: string;
  errorMessage: string;
}) => { ... }

    This makes function calls much more readable:

    checkStatusChange(hasInternalUseChanged, {
  newStatus: patch.internalUse.statusAssetUseItemCode,
  allowedStatus: 'tobechecked',
  errorMessage: 'Changing the asset's status is not allowed',
})

  • There is no reason for allowedStatus and errorMessage to be parameters, as they are the same for all function calls.

  • Instead of passing hasChanged and newStatus, you could just pass the key 'internalUse' | 'publicUse'. This would also enable you to compute hasChanged within the function instead of duplicating it outside of it.

  • Reading a function starting with check, I would expect it to return a value (a boolean, most likely). Here, I would prefer validateStatus to keep with the outer function's name, or even validateStatusOrThrow if you want to be explicit about what it does.
    If you want to keep it as a check*, I personally would prefer to make the function return a boolean and then make the function caller throw the exception themself. This would also enable you to remove the errorMessage parameter from the function, while keeping the function pure.


// 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);
}
};
3 changes: 2 additions & 1 deletion apps/server-asset-sg/src/features/asset-edit/asset-edit.http
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,5 +71,6 @@ Content-Type: application/json
"titleOriginal": "My Cool Asset",
"titlePublic": "Our Cool Asset",
"typeNatRels": [],
"workgroupId": 1
"workgroupId": 1,
"assetFiles": []
}
6 changes: 3 additions & 3 deletions ...sset-editor/src/lib/components/asset-editor-tab-usage/asset-editor-tab-usage.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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';

Expand Down
3 changes: 0 additions & 3 deletions libs/shared/v2/src/lib/models/user.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down