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

[SVCS-133] Reject complicated self-overwrites #341

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

NyanHelsing
Copy link
Contributor

@NyanHelsing NyanHelsing commented Apr 27, 2018

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:

  • Box
  • Dropbox
  • GoogleDrive
  • OwnCloud

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.

@coveralls
Copy link

coveralls commented Apr 30, 2018

Coverage Status

Coverage increased (+0.02%) to 91.917% when pulling a2d4f2c on birdbrained:ft/reject-overwrites into 0e53579 on CenterForOpenScience:develop.

@cslzchen cslzchen self-requested a review May 23, 2018 16:41
Copy link
Contributor

@cslzchen cslzchen left a 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.

@cslzchen
Copy link
Contributor

cslzchen commented May 29, 2018

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:

orginal-commits

New Commits:

new-commits

@cslzchen cslzchen force-pushed the ft/reject-overwrites branch 2 times, most recently from 0d24d2a to 5aae000 Compare May 29, 2018 17:24
Copy link
Contributor

@cslzchen cslzchen left a 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.

complicated-self-overwrite

AddisonSchiller and others added 3 commits June 19, 2018 09:59
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants