-
Notifications
You must be signed in to change notification settings - Fork 78
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
[SVCS-133] Reject complicated self-overwrites #341
base: develop
Are you sure you want to change the base?
[SVCS-133] Reject complicated self-overwrites #341
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎆
@birdbrained I will rebase/squash/reword commits, remove three merge ones and then push back to the PR.
Update: given that your changes are only style fixes (including fixing issues introduced by yourself), I cherry-picked and rebased the full PR into 2 commits, one from Addison and one from you. I removed several unnecessary style updates as well. Your original PR is backed up in this branch. Original Commits:New Commits: |
0d24d2a
to
5aae000
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as expected locally. Commits are cherry-picked and rebased to the latest develop branch 🎆 🎆 . Ready for final review @felliott . The ticket has a detailed config of local integration tests. Below is a screenshot of my local one with the Box provider.
9a45b46
to
353c348
Compare
Fixing an issue where it is possible to silently delete files when a child component is hooked up to a project and they share the same provider.
5802ec9
to
a2d4f2c
Compare
Update: PR description modified by @cslzchen
Ticket
https://openscience.atlassian.net/browse/SVCS-133
Purpose
Replaces: #270
As per the instructions on the linked ticket, it is possible to silently cause files to delete themselves during intra copy/move actions. This ticket fixes this issue on the following providers it affects:
Summary of Changes
Added Code to check the full path, or identifiers of the source and destination. If they overlap (are the same) the move/copy is rejected with a 409 and an error message that "files cannot overwrite themselves".
Added and updated tests to the affected providers.
Tests and QA Notes
There are testing notes on the ticket for how to properly recreate the issue.
For the 4 providers listed, copy/move with replace/keep both and rename should all be tested extensively. They should be tested with and without the setup for the bug in place
Light testing on other addons/providers should also be done.