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.

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

Conversation

TIL-EBP
Copy link
Contributor

@TIL-EBP TIL-EBP commented Oct 21, 2024

resolves #309

…ditor on current or future workgroup of the edited asset, even for admins
@TIL-EBP TIL-EBP requested a review from daniel-va October 21, 2024 13:04
Comment on lines 71 to 88
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.

Copy link

@TIL-EBP TIL-EBP merged commit 8a554c8 into develop Oct 30, 2024
8 checks passed
@TIL-EBP TIL-EBP deleted the bug/assets-309-restrict-editing-permissions-for-admins branch October 30, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Admins sollen keine MasterEditor Berechtigungen haben, wenn sie nur Editor sind
2 participants