Broken tests in drop zone in reva master

[gmgigi96]

in cs3org/reva#3469, to not override files having the same names in a drop zone, we are appending an uuid at the end of a file name.
This is unfortunately breaking some acceptance tests, like for example this scenario (from https://drone.cernbox.cern.ch/cs3org/reva/9673/10/6):

  Scenario: Uploading same file to a public upload-only share multiple times via new API
    # The new API does the autorename automatically in upload-only folders
    Given user "Alice" has created a public link share with settings
      | path        | FOLDER |
      | permissions | create |
    When the public uploads file "test.txt" with content "test" using the new public WebDAV API
    And the public uploads file "test.txt" with content "test2" using the new public WebDAV API
    Then the HTTP status code of responses on all endpoints should be "201"
    And the following headers should match these regular expressions
      | ETag | /^"[a-f0-9:\.]{1,32}"$/ |
    And the content of file "/FOLDER/test.txt" for user "Alice" should be "test"
    And the content of file "/FOLDER/test (2).txt" for user "Alice" should be "test2"

Now, this test is supposed to check for a file containing a number (2), that didn't override the original file. But even testing in https://ocis.owncloud.com this does not happen, and the original file is overwritten.
So which is the expected behavior in OCIS?
We would like than to modify the test for reva master to pass the PR
@phil-davis

[labkode]

@micbar can someone please look into it?

[phil-davis]

Note: issue #5094 - some time we are thinking to move the API test code into the ocis repo. That will make it a bit easier to maintain ocis-specific small differences in behavior.

In this case here, the behavior in reva master is becoming quite different to what is expected in ocis.

The test scenario currently fails in ocis and the expected-failures file links to issue #1267 which has a link to the discussion in cs3org/reva#2480

The test scenario also currently fails in reva master - see https://github.com/cs3org/reva/blob/master/tests/acceptance/expected-failures-on-OCIS-storage.md and search for uploadToPublicLinkShare.feature:24

From memory, I thought that some implementations were now making names starting with (1) (instead of (2) that oC10 does). And I thought that we had modified the tests so that they would allow the second upload to have either (1) or (2)` appended to its file name. That has not happened for the test scenario that you mention - I will have a look to see what happened there.

Regardless of the (1) or (2)` thing - if some implementations (for example, reva master) are going to have a different automatic-naming scheme then we need to sort out how we want to test such behavior differences:

1. skip the test scenario(s)
2. make the scenarios much more flexible, to "auto detect" somehow that an extra file appeared, and not to care what is the particular file name construction
3. make the scenarios know what to expect for each implementation (oC10 core, ocis, reva edge, reva master)
   or?

And if there are going to be long-term behavior differences between reva master and reva edge, then how will we ever get them merged into a single "base product"? I suppose that, in this example, the duplicate-file-name-resolution behavior would need to be able to be specified in config somewhere.

[labkode]

@phil-davis thanks Phil for the explanation, so this means that this functionality is just broken, this is a serious security issue, we're disabling the tests for now to include this PR merged.

[phil-davis]

@phil-davis thanks Phil for the explanation, so this means that this functionality is just broken, this is a serious security issue, we're disabling the tests for now to include this PR merged.

The related test scenarios are already in expected-failures. So I don't understand how this effects day-to-day CI. The test scenarios will fail in a different way. But there should not be anything to actually do - we know that the test fails, there are lots of tests in expected-failures that we know are failing, and gradually things get fixed and the entries in expected-failures reduce.

I don't know what you mean by "disabling the tests for now" - maybe you can can add a test tag to the tags to be ignored. But I don't understand what benefit that would be.

[micbar]

this is a serious security issue

Why is the difference between (1) or (2) a serious security issue?

[phil-davis]

Why is the difference between (1) or (2) a serious security issue?

Note: the current implementation in ocis (and I presume in reva master and edge) does not do any renaming at all. There is no (1) or (2) happening. The 2nd upload overwrites the first upload. If version history is available, then the previous content should be in the version history, but otherwise the first version content would be lost. That is an unexpected data loss.

Issue #1267 is still open and needs to be addressed some day.

[labkode]

Exactly, today overwrite happens both on master and edge. We have a fix for master, that can be easily ported to edge but the tests are failing, that's why we need to disable them unless this fix makes sense for both of branches and the tests will be adapted.

[phil-davis]

@micbar this would fix #1267

If it is implemented by adding a uuid at the end of a file name, is that OK for oCIS? Or does oCIS/ownCloud want a different algorithm for generating the extra filename(s)?

If so, then I can sort out the test expectations. Otherwise we all need to discuss what solution or range of solutions are to be implemented, and then I can make the tests expect different things depending on the implementation.

[micbar]

yes. I agree. I retested that on ocis master. Seems that this is just not implemented.

Needs qualification if this is a GA Blocker.

[micbar]

@tbsbdr FYI

[phil-davis]

I made a comment on the code here: https://github.com/cs3org/reva/pull/3469/files#r1030330906
As I read it, the proposed implementation will always add "_uuid" to the uploaded file name, even for the first file uploaded.
Is that the intended implementation?

[micbar]

@labkode I spoke with @tbsbdr and @dragotin again. We need to achieve a compromise.
A uuidv4 is just too long and looks "nerdy". That will never be accepted by non-phsicists.

We could come together with a short 5 character string. Would that be possible?

[phil-davis]

Maybe use some "tinyURL"-style string. Some use base62 (characters 0-9, A-Z and a-z. But we don't want to get into the issues with mixed-case file names that happen to be seen as a conflict when synced to some clients ("aB4xZ" and "AB4xz" would be different base62 strings but look like very much the same file name).

base36 is another option 0-9 and A-Z (or a-z - pick which is preferred for rendering). if people care about it being readable accurately for humans, then there is the issue with zero 0 and the letter O, and one 1 and the letter L when rendered in various fonts. 5-characters is 36^5 = 60,466,176. 6 characters is 2,176,782,336 combinations.

Or could drop down to base26 and use just the 26 letters of the alphabet. 6 characters gives 308,915,776 combinations. If multiple people drop the same filename form.pdf then they become form_atycoq.pdf form_dwftvj.pdf etc. A random generator is going to rarely have a collision, but the code should still check, and if the generated filename already exists, then generate another string.

The other decision is the separatore - use underscore _ or dash - or?

[individual-it]

Is this a duplicate of owncloud/web#7643 ?

[gmgigi96]

@phil-davis Maybe having only the 26 letters is enough. Only, even if the code checks before uploading that the file already exists, this does not guarantee to not have collisions. I propose instead to enforce the if_not_exist flag in the InitiateFileUpload (https://cs3org.github.io/cs3apis/#cs3.storage.provider.v1beta1.InitiateFileUploadRequest), and report to the user to retry again the upload. In fact, some storage providers (see EOS), cache locally the file, before finally uploading to the remote storage, making it impossible to retry with a different file name (as the stream has been already consumed).

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 10 days if no further activity occurs. Thank you for your contributions.

[phil-davis]

The naming scheme for this still needs to be decided, then implemented.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 10 days if no further activity occurs. Thank you for your contributions.

[ScharfViktor]

fixed via #8340