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

WebDAV: Fix PUT to overwrite file if already exists #92

Conversation

adrienverge
Copy link
Contributor

The WebDAV specification 1 states that in case of a PUT /…/file, if the resource /…/file already exists and is not a collection (= a directory), then it should be replaced in place (= overwritten).

This commit fixes that.
It can be tested with a simple WebDAV client like curl:

# Write once:
curl -k -T my-file.txt https://webdav.local.internxt.com:3005
# Read:
curl -k https://webdav.local.internxt.com:3005/my-file.txt
# Overwrite (this should not fail):
curl -k -T my-file.txt https://webdav.local.internxt.com:3005

Note: I wrote this commit because otherwise the server from internxt webdav enable bugs with some WebDAV clients.

Footnotes

  1. http://www.webdav.org/specs/rfc4918.html#put-resources

@adrienverge adrienverge force-pushed the feat/webdav-put-should-overwrite-existing-file branch from cc3964f to 9da86c5 Compare August 14, 2024 09:18
The WebDAV specification [^1] states that in case of a `PUT /…/file`, if
the resource `/…/file` already exists and is not a collection (= a
directory), then it should be replaced in place (= overwritten).

This commit fixes that.
It can be tested with a simple WebDAV client like `curl`:

    # Write once:
    curl -k -T my-file.txt https://webdav.local.internxt.com:3005
    # Read:
    curl -k https://webdav.local.internxt.com:3005/my-file.txt
    # Overwrite (this should not fail):
    curl -k -T my-file.txt https://webdav.local.internxt.com:3005

Note: I wrote this commit because otherwise the server from `internxt
webdav enable` bugs with some WebDAV clients.

[^1]: http://www.webdav.org/specs/rfc4918.html#put-resources
@adrienverge adrienverge force-pushed the feat/webdav-put-should-overwrite-existing-file branch from 9da86c5 to 5dc622d Compare August 21, 2024 07:42
@adrienverge
Copy link
Contributor Author

Rebased on latest main branch.

@adrienverge
Copy link
Contributor Author

Hello @apsantiso, @PixoDev and @larryrider, I see you worked on the WebDAV server code recently.

Could you have a look at this pull request? It fixes a real bug that prevents the use of Internxt.

Thanks :)

PS: #98 and #99 also fix bugs, and are easy to review.

@larry-internxt
Copy link
Contributor

Hello @apsantiso, @PixoDev and @larryrider, I see you worked on the WebDAV server code recently.

Could you have a look at this pull request? It fixes a real bug that prevents the use of Internxt.

Thanks :)

PS: #98 and #99 also fix bugs, and are easy to review.

Hey! @adrienverge Thank you for your work, we will check it asap and if its ok it will be merged for sure!

@PixoDev
Copy link
Contributor

PixoDev commented Sep 10, 2024

Hey @adrienverge , thank you so much for your contribution!

I reviewed your implementation, however we don't replace file content that way (trash + delete + upload).

Instead we do the following on other platforms:

  1. Check if the file exists in the Internxt API, you can check it by the UUID, you can check our existing API methods for Drive here https://github.com/internxt/sdk/blob/master/src/drive/storage/index.ts
  2. In case the file exists, encrypt and reupload the file, then replace the fileId with this https://github.com/internxt/sdk/blob/master/src/drive/storage/index.ts#L630

If you need some more guidance with this, we'll be happy to help

Copy link
Contributor

@PixoDev PixoDev left a comment

Choose a reason for hiding this comment

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

Check our comment about replacing file content please

adrienverge added a commit to adrienverge/internxt-cli that referenced this pull request Sep 14, 2024
This is another implementation of my fix proposed at
internxt#92, with suggestions from @PixoDev.
Instead of trashing + uploading, it replaces the file in place.

The WebDAV specification [^1] states that in case of a `PUT /…/file`, if
the resource `/…/file` already exists and is not a collection (= a
directory), then it should be replaced in place (= overwritten).

This commit can be tested with a simple WebDAV client like `curl`:

    # Write once:
    curl -k -T my-file.txt https://webdav.local.internxt.com:3005
    # Read:
    curl -k https://webdav.local.internxt.com:3005/my-file.txt
    # Overwrite (this should not fail):
    curl -k -T my-file.txt https://webdav.local.internxt.com:3005

Note: I wrote this commit because otherwise the server from `internxt
webdav enable` bugs with some WebDAV clients.

[^1]: http://www.webdav.org/specs/rfc4918.html#put-resources
@adrienverge
Copy link
Contributor Author

Thanks @larry-internxt and @PixoDev!

 however we don't replace file content that way (trash + delete + upload)

Indeed, it makes more sense to just replace without trashing.

Please see my other implementation at #107 (I open a separated PR to keep this original code somewhere, because at the moment this is the only one that allows me to use Internxt).

adrienverge added a commit to adrienverge/internxt-cli that referenced this pull request Sep 30, 2024
This is another implementation of my fix proposed at
internxt#92, with suggestions from @PixoDev.
Instead of trashing + uploading, it replaces the file in place.

The WebDAV specification [^1] states that in case of a `PUT /…/file`, if
the resource `/…/file` already exists and is not a collection (= a
directory), then it should be replaced in place (= overwritten).

This commit can be tested with a simple WebDAV client like `curl`:

    # Write once:
    curl -k -T my-file.txt https://webdav.local.internxt.com:3005
    # Read:
    curl -k https://webdav.local.internxt.com:3005/my-file.txt
    # Overwrite (this should not fail):
    curl -k -T my-file.txt https://webdav.local.internxt.com:3005

Note: I wrote this commit because otherwise the server from `internxt
webdav enable` bugs with some WebDAV clients.

[^1]: http://www.webdav.org/specs/rfc4918.html#put-resources
@adrienverge
Copy link
Contributor Author

Hello @PixoDev, I've done the suggested implementation at #107. Could you please have a look?

@larry-internxt
Copy link
Contributor

Hello @PixoDev, I've done the suggested implementation at #107. Could you please have a look?

Hey, thanks for your work! I will take a look asap

adrienverge added a commit to adrienverge/internxt-cli that referenced this pull request Oct 9, 2024
This is another implementation of my fix proposed at
internxt#92, with suggestions from @PixoDev.
Instead of trashing + uploading, it replaces the file in place.

The WebDAV specification [^1] states that in case of a `PUT /…/file`, if
the resource `/…/file` already exists and is not a collection (= a
directory), then it should be replaced in place (= overwritten).

This commit can be tested with a simple WebDAV client like `curl`:

    # Write once:
    curl -k -T my-file.txt https://webdav.local.internxt.com:3005
    # Read:
    curl -k https://webdav.local.internxt.com:3005/my-file.txt
    # Overwrite (this should not fail):
    curl -k -T my-file.txt https://webdav.local.internxt.com:3005

Note: I wrote this commit because otherwise the server from `internxt
webdav enable` bugs with some WebDAV clients.

[^1]: http://www.webdav.org/specs/rfc4918.html#put-resources
@larryrider
Copy link
Contributor

Hi again @adrienverge!
This change is going to be included in the next release with some other improvements, thank you for your effort!

@larryrider larryrider closed this Nov 5, 2024
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.

4 participants