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

feat: enable dataset access for shared emails #485

Merged
merged 4 commits into from
May 11, 2023

Conversation

martin-trajanovski
Copy link
Collaborator

@martin-trajanovski martin-trajanovski commented May 8, 2023

Description

This PR aims to make shared datasets available for users. Also prevents all users for being able to update datasets and leave that ability only for owners.

Motivation

Anyone that has access to dataset could update or share it and people in sharedWith field were not able to access the datasets.

Fixes:

Changes:

  • changes made

Tests included/Docs Updated?

  • Included for each change/fix?
  • Passing? (Merge will not be approved unless this is checked)
  • Docs updated?
  • New packages used/requires npm install?
  • Toggle added for new features?

@martin-trajanovski martin-trajanovski self-assigned this May 9, 2023
@martin-trajanovski martin-trajanovski marked this pull request as ready for review May 9, 2023 11:13
@martin-trajanovski martin-trajanovski changed the title [WIP]feat: enable dataset access for shared emails feat: enable dataset access for shared emails May 9, 2023
Copy link
Member

@nitrosx nitrosx left a comment

Choose a reason for hiding this comment

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

Please use the casl facility to check if the user can update the dataset.

Comment on lines 956 to 965
// NOTE: Not sure if email is the best way to do this but since we don't have userId or something like that email is acceptable.
const couldUpdateDataset =
datasetToUpdate?.ownerEmail === loggedInUser.email;
// $addToSet is necessary to append to the field and not overwrite
// $each is necessary as data is an array of values

if (!couldUpdateDataset) {
throw new ForbiddenException();
}

Copy link
Member

Choose a reason for hiding this comment

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

In order to allow the update on the dataset, we should use the casl factory.
The rules is the following:

  • if the user logged in belongs to the group that owns the dataset, he/she is allowed to update the dataset.
  • if the user belongs to any of the admin groups, he/she can update the dataset.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahaaa... I see now but do we have these rules set up already or I need to fix that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nitrosx I fixed it and now it uses casl to decide if user is able to update something or not.

@nitrosx nitrosx merged commit 14645c1 into master May 11, 2023
@nitrosx nitrosx deleted the SWAP-3274-access-shared-datasets branch May 11, 2023 14:24
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.

2 participants