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

Jump to bottom

BC-6352 - Deleting the creator field causes the files not to be displayed anymore #4716

Merged
merged 11 commits into from
Mar 8, 2024
Merged

BC-6352 - Deleting the creator field causes the files not to be displayed anymore #4716

merged 11 commits into from
Mar 8, 2024

Conversation

bn-pass
Copy link
Contributor

@bn-pass bn-pass commented Jan 25, 2024 •
edited
Loading

…e teams' files to show all the teams' files

Description

Links to Tickets or other pull requests

https://ticketsystem.dbildungscloud.de/browse/BC-6352

Changes

Datasecurity

Deployment

New Repos, NPM pakages or vendor scripts

Approval for review

  • DEV: If api was changed - generate-client:server was executed in vue frontend and changes were tested and put in a PR with the same branch name.
  • QA: In addition to review, the code has been manually tested (if manual testing is possible)
  • All points were discussed with the ticket creator, support-team or product owner. The code upholds all quality guidelines from the PR-template.

Notice: Please remove the WIP label if the PR is ready to review, otherwise nobody will review it.

@bn-pass bn-pass changed the title remove no longer valid code that checked for the creator among all th… BC-6352 - Deleting the creator field causes the files not to be displayed anymore Jan 25, 2024
dyedwiper
dyedwiper reviewed Feb 13, 2024
View reviewed changes
src/services/fileStorage/utils/filePermissionHelper.js Outdated
@@ -104,11 +58,7 @@ const checkPermissions = (permission) => async (user, file) => {
return Promise.reject();
}

return checkTeamPermission({
Copy link
Contributor

Choose a reason for hiding this comment

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

That you removed this check altogether, does it mean, it was completely unnecessary?

This old code is really hard to read, so I'm confused, sorry...

Copy link
Contributor Author

@bn-pass bn-pass Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That's ok 😊 . Yes there was a pretty weird (in my opinion) permission check which required the author of a file to be marked as a file's creator or present in the first position of the permissions array. And none of them is true anymore for some cases (for some files) since we've had a requirement to completely get rid of all the user's data, including any references etc. (so we're removing any existing creator references and any personal user's reference occurrences within all the file's permissions arrays). That's why we've had to also delete the requirement from the discussed function - keeping it caused some files to not being accessible anymore (by anyone basically).

Copy link
Contributor

@dyedwiper dyedwiper Feb 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

All this code is pretty pretty weird, indeed! I tried to understand it again more thoroughly but I can't get my head around it. What irritates me in your change is just, that there was a function before that checked for a team permission (at least by its name) and that this check is now completely gone. If the name is correct, this would mean that now a user from another team could see a file, that he is not allowed to, wouldn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dyedwiper Hm when I was developing this change and testing it afterwards it felt like the whole function is not doing anything without this "creator check". But let me verify it step-by-step again and get back to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dyedwiper You were right. I was able to get rid only of this weird "author check", but keep the whole "team* role" check. Please re-review.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems better, but I still don't understand what this change means. I guess somebody might not be able to see a file like he used to. But I'll approve now from a code perspective. You have to test this or wait for complaints ;)

@bn-pass bn-pass requested a review from dyedwiper March 8, 2024 01:31
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Mar 8, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@bn-pass bn-pass merged commit 455a42a into main Mar 8, 2024
56 of 57 checks passed
@bn-pass bn-pass deleted the BC-6352-no-creator-ref-case branch March 8, 2024 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blazejpass blazejpass blazejpass approved these changes

@dyedwiper dyedwiper dyedwiper approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bn-pass @dyedwiper @blazejpass