0-byte uploads are completed after the InitiateFileUploads call already

[aduffeck]

This PR changes the wopiserver to treat 0-byte uploads as finished after the InitiateFileUploadRequest already to match the behavior of reva edge. I suppose this might break compatibility with reva master so it'll likely need some discussion.

@glpatcern In reva edge we changed the behavior for 0-byte uploads so that they are finalized as part of the InitiateFileUploadRequest request already. This is analogous to what tusd does in the TUS POST requests and fixes a bug with 0-byte TUS uploads where uploads were not finalized because reva doesn't use the TUS POST handler and subsequent PATCH requests, which would finalize the upload, are not sent in this case.

So, do you know if this actually causes a problem for reva master? Or would you consider changing the behavior in master as well?

/cc @micbar @butonic

[glpatcern]

AFAICT we need to change reva master as well, I remember back then that an empty PUT was/is needed to complete a touch operation. @labkode can you confirm? and what would it entail to change this behavior for reva master?

That said, I'd still log at INFO level a success, as it is done at the end of the method, rather than a short debug message.

[aduffeck]

That said, I'd still log at INFO level a success, as it is done at the end of the method, rather than a short debug message.

👍 done

[glpatcern]

👍 done

Thanks. With further thinking, we know that Office apps never store a 0-byte file, as "empty documents" do contain a few KB of metadata, therefore the chance of this code path to be triggered is only if you have an app that manages .md or .txt files, and the user attempts to delete the content of a file and closes it: did you have such a case?

[aduffeck]

No, we didn't see it in real life yet or at least I'm not aware of it, but our ci fails in the wopi validator pipeline with this testcase, which does try to upload an empty file: https://github.com/microsoft/wopi-validator-core/blob/main/TestCases.xml#L974-L996C18

[glpatcern]

OK, understood, also to set the priority for this. Agreed that it's not good to fail a test case, but the way Reva (master/edge) implements creating a new file is by touching it prior to passing its Ref to the wopiserver, therefore we are sure we never go through that MS validator's scenario.

Now it remains the scenario I mentioned above: an app that may trigger a save of a 0-byte file I know of is CodiMD, which we do run and AFAICT you do not propose in your offer. Not sure if Onlyoffice offers to edit .txt files. As such, I'll wait to merge this until we adapt Reva master.

[aduffeck]

Right. While I can't seem to make OnlyOffice to upload a 0-byte file (it automatically adds a newline before saving an empty .txt file), there still might be other current or future apps which do that of course.

Is there any chance we could merge this already since you are planning to adjust master anyway? Or - if that's not possible - could you give an ETA for when this could be merged so we can decide how we move forward?

[glpatcern]

As you've just proved you can't trigger this code path in real life (whereas we could, via CodiMD), let's please hold. I'll check with @labkode about adapting Reva master, but the priority of this patch is "low" compared to other developments, so can't promise an ETA for now.

[micbar]

@glpatcern This is currently blocking our release.

[micbar]

The test pipelines are failing, even if we have no "real world" impact.

[glpatcern]

We have proven that the test pipelines are failing on a non-real-life scenario, so they ought to be overridden.

On the other side, IMHO it seems the new 0-byte upload implementation in Reva edge assumes that all clients behave "correctly": I guess you have implemented proper protections for 0-byte PUT uploads from WebDAV clients, but pure CS3 clients appear to not be protected. Is that the case? Arguably, a 0-byte PUT is legitimate in all cases and should not fail in the first place...

[aduffeck]

Yes, we currently do not handle the case, when an upload is tried even though it shouldn't, very well and actually fail with an internal error. That needs to be changed - maybe something like a 404 might be the way to go - but in any case we can't just OK these requests because the according upload isn't available anymore. While 0-byte PUTs could be considered legitimate in general, PUTs against already finalized uploads aren't.

[labkode]

@micbar @aduffeck the implementation in Reva edge is a hack that relies on the internal of the Go TUS library creating an in-memory object that is re-used across calls for the TUS protocol.

I understand that is a way to reduce one call from the client, however that is not semantically correct, the InitiateFileUpload method does not create files.

A Touch method needs to be added to the CS3APIs, feel free to open a PR, that will help with your use-case.
The WebDAV layer as well as WOPI can intercept this case and call the appropriate method based on file size.

Can you add this for your release?

[glpatcern]

@micbar @aduffeck as we realized it may be hard at this time to extend the CS3APIs (you're running on the latest tag, not on the top of the main branch), a workaround I can think about to keep compatibility with both Reva master and edge would be the following.

wopiserver attempts to execute the PUT as usual. If that PUT is not successful AND the size == 0, then the failure is ignored (logged as INFO) and the method returns success.

[micbar]

A Touch method needs to be added to the CS3APIs, feel free to open a PR, that will help with your use-case.
The WebDAV layer as well as WOPI can intercept this case and call the appropriate method based on file size.

This is already present on the storage provider and implemented in the DecomposedFS. We are using Touch already when we are getting empty PUT requests via ocdav.

The issue here is, like you described already, the tusd library which is limited and somehow dictates what we can do without a major refactoring.

@butonic @aduffeck @labkode please feel free to suggest a different approach. The only thing that i do not have much is time 🕐 😄 This needs to be addressed within the next few days because we have a very tight schedule for the pentest, release and rollout dates and cannot hold the release.

[aduffeck]

Alright, I would suggest to implement the workaround @glpatcern suggested for now to unblock the release (try the PUT unconditionally but ignore failures if the size equals 0) and discuss this in more detail when @butonic is back. Does that makes sense?

I'll adapt this PR accordingly in a bit.

[glpatcern]

This is already present on the storage provider and implemented in the DecomposedFS. We are using Touch already when we are getting empty PUT requests via ocdav.

Indeed, I completely forgot that we do have a TouchFile CS3API method. So I agree with @aduffeck for now and the proper implementation should be relatively straightforward:

if size == 0 {
    cs3.TouchFile(...)
} else {
    cs3.InitiateFileUpload(...)
    requests.Put(...)
}

[aduffeck]

I changed the PR to implement the described workaround instead.

I also tried quickly to do implement the TouchFile approach in https://github.com/aduffeck/wopiserver/tree/0-byte-uploads-touchfile but unfortunately it doesn't work right away because TouchFile doesn't support setting the lock token so I run into

{"time": "2024-01-18T09:35:29.926", "host": "1f8731a05806", "level": "ERROR", "process": "wopiserver", "module": "cs3iface", "msg": "Failed to touch file", "filepath": "da1b4b4f-eeb2-436c-be66-d272fbbf68fb/test.wopitest", "trace": "415a45f27d1e7295f912fa601fa52c7b", "code": "20", "reason": "touch file: error: locked by opaquelocktoken:797356a8-0500-4ceb-a8a0-c94c8cde7eba TG9ja1N0cmluZw=="}

Also I'm not sure what the expected behavior of TouchFile-ing an existing file would be. Would it be truncated? Or just change the mtime?

Anyway, I hope the workaround lets us move forward with the release for now :)

[glpatcern]

OK, let's move forward indeed, I've just added a more extended comment.

TouchFile for an existing file should only modify the mtime. And I'd argue it should not be prevented on a locked file as it's modifying the metadata only, not the file itself (in old-good-POSIX terms, a touch alters the content of the parent folder).

[aduffeck]

Thanks a lot, I applied your suggestion 👍