-
Notifications
You must be signed in to change notification settings - Fork 76
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 #270
[SVCS-133] Reject complicated self-overwrites #270
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.
I ran into an issue while testing that I think will require a significant change. If I do a move with a conflict=keep. Then the original file is deleted, contrary to the meaning of "keep". In order to catch this, the test for sameness would need to occur before the call to handle_naming
in core.provider.move . With that in mind, can we make the if src_path.* == dest_path.*:
tests in each provider be it's own method.. So that it can be called from core.provider? Or better yet, can it be generalized for all providers in a method inside of core.provider?
With that in hand, we can catch before getting to handle_naming
If same path and copy/keep: let it go, or error
If same path and copy/replace: error
If same path and move/keep: switch it to copy/keep, or error
If same path and move/replace: error
@@ -1,4 +1,5 @@ | |||
import aiohttp | |||
from http import HTTPStatus |
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.
I think http
goes in a section to itself at the top for "Standard Library" libs.
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.
👍
Fixing an issue where when a child component is hooked up To a project, and they share the same provider, it is possible to silently delete files
950f3d9
to
5db8f08
Compare
CRR completed: |
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.
Looking good. Three comments regarding the location of the override check. I think it needs to occur regardless of whether handle_naming
is True
.
Tried to address the confusion you mentioned in one of your comments.
Suggestion for adding to one of your doc strings.
waterbutler/core/provider.py
Outdated
# In handle_naming so we don't run it multiple times | ||
# This does not mean the `dest_path` is a folder, at this point it could just be the parent | ||
# of the actual dest_path, or have a blank name | ||
if not dest_path.is_file: |
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.
I'm wondering if this shouldn't be checked regardless of whether or not handle_naming
is True
.
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.
Done for all 3 👍
waterbutler/core/provider.py
Outdated
# In handle_naming so we don't run it multiple times | ||
# This does not mean the `dest_path` is a folder, at this point it could just be the parent | ||
# of the actual dest_path, or have a blank name | ||
if not dest_path.is_file: |
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.
I'm wondering if this shouldn't be checked regardless of whether or not handle_naming
is True
.
waterbutler/core/provider.py
Outdated
@@ -269,7 +281,29 @@ def request(self, *args, **kwargs): | |||
'conflict': conflict, | |||
'got_rename': rename is not None, | |||
}) | |||
|
|||
if not dest_path.is_file: |
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.
This looks like the right spot to me, Does this need to be duplicated in if handle_naming:
?
if src_path.is_dir and dest_path.is_file: | ||
# Cant copy a directory to a file | ||
raise ValueError('Destination must be a directory if the source is') | ||
|
||
# This is confusing. `dest_path` at this point can refer to the parent or root of | ||
# the file we want to move/copy. So even if moving/copying a file, this code will run |
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.
I can think of at least one case where dest_path.is_file
will be True
. In the case of overwriting an existing file with new content. Something that cannot be done through osf.io but can be directly through the API.
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.
This made me realize that I should do some more testing on that sort of situation. All looked good though.
""" Return wether a move or copy operation will result in a self-overwrite. | ||
|
||
.. note:: | ||
Defaults to False |
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.
Maybe a comment, noting that this is meant to be overridden by providers that need to check?
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.
complete
waterbutler/core/provider.py
Outdated
rename or src_path.name, | ||
folder=src_path.is_dir | ||
) | ||
if self.will_self_overwrite(dest_provider, src_path, temp_path): |
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.
Consider if this isn't it's own method but just src_path.NAME == dest_provider.NAME and dest_path.full_path == src_path.full_path
or some other general comparison like src_path.identifier == dest_path.identifier
you avoid having to modify individual providers. Some combination of identifier, name and path comparisons should probably stop this in the handler before it gets to the provider.
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.
The problem is that we don't know if there will be a conflict before it gets to the provider. dest_path
is generic sometimes when it hits move
or copy
, and we do not have an idea of what the real path will be until we run revalidate_path
on the dest_path
with src_path.name
or rename
.
The reason it was done as a new function in the providers is that many providers do not suffer from this bug, and running a general check on those could return True
and reject a perfectly valid operation.
This is mostly due to the nature of the bug, where often the file is
src_path = "/folder1/folder2/file.txt"
and
dest_path = "/folder2/file.txt"
While they refer to the same file, oftentimes they will not look like the same file due to which folder a provider is attached to.
@AddisonSchiller commits messages should have some indication of the changes being made and should generally be written in the imperative. |
@Johnetordoff Its going to get rebased in a little bit with a different message? |
Well remember that for the future, it doesn't have to be elaborate just vaguely searchable. People might pick this up before you can rebase it. |
@Johnetordoff Next time a message like this is better sent over flow dock. |
@TomBaxter is taking this ticket over. There seems to be a lot of overlap between this and #274 so they may end up getting merged into one super-PR. Leaving this open for now for reference until a final PR is merged. |
New PR #341 |
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.
PR replaced and closed 🚫
refs: https://openscience.atlassian.net/browse/SVCS-133
Purpose
As per the instructions on the linked ticket, it is possible to silently cause files to delete themselves during intra_copy/moves. This ticket is to fix this issue on the providers it affects.
Effected providers:
*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 tests to the affected providers.
Corrected tests that were improperly erring due to the addition.
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.