[full-ci] update reva to v2.6.1-...

[butonic]

update reva to v2.6.1-...

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[ownclouders]

💥 Acceptance test Core-API-Tests-ocis-storage-1 failed. Further test are cancelled...

[phil-davis]

I added full-ci to the title. If you push again, drone will take notice.

[butonic]

@phil-davis thx, we have an unexpected pass ;-)

[phil-davis]

https://drone.owncloud.com/owncloud/ocis/12787/38/6

Scenario: A user cannot create a folder in a Space if they do not have permission                                     # /drone/src/tests/acceptance/features/apiSpaces/uploadSpaces.feature:33
    Given user "Alice" has created a space "Project Merkur" of type "project" with quota "2000"                         # SpacesContext::userHasCreatedSpace()
    And user "Bob" creates a folder "forAlice" in space "Project Merkur" owned by the user "Alice" using the WebDav Api # SpacesContext::theUserCreatesAFolderToAnotherOwnerSpaceUsingTheGraphApi()
    Then the HTTP status code should be "404"                                                                           # FeatureContext::thenTheHTTPStatusCodeShouldBe()
      HTTP status code 403 is not the expected value 404
      Failed asserting that 403 matches expected '404'.

  Scenario: A user cannot upload a file in a Space if they do not have permission                                                                # /drone/src/tests/acceptance/features/apiSpaces/uploadSpaces.feature:52
    Given user "Alice" has created a space "Project Pluto" of type "project" with quota "2000"                                                   # SpacesContext::userHasCreatedSpace()
    When user "Bob" uploads a file inside space "Project Pluto" owned by the user "Alice" with content "Test" to "test.txt" using the WebDAV API # SpacesContext::theUserUploadsAFileToAnotherOwnerSpace()
    Then the HTTP status code should be "404"                                                                                                    # FeatureContext::thenTheHTTPStatusCodeShouldBe()
      HTTP status code 403 is not the expected value 404
      Failed asserting that 403 matches expected '404'.

It looks like those scenarios should now be edited to change the expectation of 404 to 403

[butonic]

apiSpaces/uploadSpaces.feature:33
apiSpaces/uploadSpaces.feature:52

Mkcol and put return 403 instead of 404 when targeting another users space

[butonic]

hmmm ocis apiSpaces/uploadSpaces.feature:52

  Scenario: A user cannot upload a file in a Space if they do not have permission                                                                # /drone/src/tests/acceptance/features/apiSpaces/uploadSpaces.feature:52
    Given user "Alice" has created a space "Project Pluto" of type "project" with quota "2000"                                                   # SpacesContext::userHasCreatedSpace()
    When user "Bob" uploads a file inside space "Project Pluto" owned by the user "Alice" with content "Test" to "test.txt" using the WebDAV API # SpacesContext::theUserUploadsAFileToAnotherOwnerSpace()
    Then the HTTP status code should be "404"                                                                                                    # FeatureContext::thenTheHTTPStatusCodeShouldBe()
      HTTP status code 403 is not the expected value 404
      Failed asserting that 403 matches expected '404'.
    And for user "Alice" the space "Project Pluto" should not contain these entries:                                                             # SpacesContext::userTheSpaceShouldContainEntries()
      | test.txt |

seems to contradict reva apiAuthWebDav/webDavPUTAuth.feature:70

2315 | Scenario: send PUT requests to another user's webDav endpoints as normal user using the spaces WebDAV API # /drone/src/tmp/testrunner/tests/acceptance/features/apiAuthWebDav/webDavPUTAuth.feature:70 | 203s
2316 | When user "Brian" requests these endpoints with "PUT" including body "doesnotmatter" about user "Alice" # OCSContext::userSendsRequestToTheseEndpointsWithBody() | 204s
2317 | \| endpoint                                       \| | 204s
2318 | \| /remote.php/dav/spaces/%spaceid%/textfile0.txt \| | 204s
2319 | \| /remote.php/dav/spaces/%spaceid%/PARENT        \| | 204s
2320 | Then the HTTP status code of responses on all endpoints should be "403"                                 # FeatureContext::theHTTPStatusCodeOfResponsesOnAllEndpointsShouldBe() | 204s
2321 | Responses did not return expected http status code | 204s
2322 | Failed asserting that 404 is identical to 403. | 204s
2323 | When user "Brian" requests these endpoints with "PUT" including body "doesnotmatter" about user "Alice" # OCSContext::userSendsRequestToTheseEndpointsWithBody() | 204s
2324 | \| endpoint                                           \| | 204s
2325 | \| /remote.php/dav/spaces/%spaceid%/PARENT/parent.txt \| | 204s
2326 | Then the HTTP status code of responses on all endpoints should be "403"

In both cases Brian is PUTing a file into the space of another user. In the first example a project space, in the second example a personal space.

Taking a step back, we are trying to prevent leaking the existence of a space / file by returning 404 instead of 403. But to access a file you need to know the space id in advance ... hm, which for users is guessable because the userid is not secret and currently equals the personal space id.

IMO we should always return 404 if the user has no access to a space, which would make the reva apiAuthWebDav/webDavPUTAuth.feature:70 test the wrong status code.

[butonic]

currently points to my branch, needs merge in reva

[butonic]

  Scenario: A user with role viewer cannot create a folder in shared Space via the Graph API                  # /drone/src/tests/acceptance/features/apiSpaces/uploadSpaces.feature:95
    Given user "Alice" has created a space "Viewer cannot create folder" of type "project" with quota "2000"  # SpacesContext::userHasCreatedSpace()
    And user "Alice" has shared a space "Viewer cannot create folder" to user "Bob" with role "viewer"        # SpacesContext::userHasSharedSpace()
    When user "Bob" creates a folder "mainFolder" in space "Viewer cannot create folder" using the WebDav Api # SpacesContext::theUserCreatesAFolderUsingTheGraphApi()
    Then the HTTP status code should be "403"                                                                 # FeatureContext::thenTheHTTPStatusCodeShouldBe()
      HTTP status code 404 is not the expected value 403
      Failed asserting that 404 matches expected '403'.
    And for user "Bob" the space "Viewer cannot create folder" should not contain these entries:              # SpacesContext::userTheSpaceShouldContainEntries()
      | mainFolder |
    And for user "Alice" the space "Viewer cannot create folder" should not contain these entries:            # SpacesContext::userTheSpaceShouldContainEntries()
      | mainFolder |

[butonic]

  @skipOnOcV10.6 @skipOnOcV10.7 @skipOnOcV10.8.0
  Scenario Outline: as share receiver renaming a file inside a folder changes its etag for all collaborators    # /srv/app/testrunner/tests/acceptance/features/apiWebdavEtagPropagation1/moveFileFolder.feature:158
    Given user "Brian" has been created with default attributes and without skeleton files                      # FeatureContext::userHasBeenCreatedWithDefaultAttributesAndWithoutSkeletonFiles()
    And the administrator has set the default folder for received shares to "Shares"                            # OccContext::theAdministratorHasSetTheDefaultFolderForReceivedSharesTo()
    And parameter "shareapi_auto_accept_share" of app "core" has been set to "no"                               # AppConfigurationContext::serverParameterHasBeenSetTo()
    And using <dav_version> DAV path                                                                            # FeatureContext::usingOldOrNewDavPath()
    And user "Alice" has created folder "/upload"                                                               # FeatureContext::userHasCreatedFolder()
    And user "Alice" has uploaded file with content "uploaded content" to "/upload/file.txt"                    # FeatureContext::userHasUploadedAFileWithContentTo()
    And user "Alice" has shared folder "/upload" with user "Brian"                                              # FeatureContext::userHasSharedFileWithUserUsingTheSharingApi()
    And user "Brian" has accepted share "/upload" offered by user "Alice"                                       # FeatureContext::userHasReactedToShareOfferedBy()
    And user "Alice" has stored etag of element "/"                                                             # WebDavPropertiesContext::userHasStoredEtagOfElement()
    And user "Alice" has stored etag of element "/upload"                                                       # WebDavPropertiesContext::userHasStoredEtagOfElement()
    And user "Brian" has stored etag of element "/"                                                             # WebDavPropertiesContext::userHasStoredEtagOfElement()
    And user "Brian" has stored etag of element "/Shares"                                                       # WebDavPropertiesContext::userHasStoredEtagOfElement()
    And user "Brian" has stored etag of element "/Shares/upload"                                                # WebDavPropertiesContext::userHasStoredEtagOfElement()
    When user "Brian" moves file "/Shares/upload/file.txt" to "/Shares/upload/renamed.txt" using the WebDAV API # FeatureContext::userMovesFileUsingTheAPI()
    Then the HTTP status code should be "201"                                                                   # FeatureContext::thenTheHTTPStatusCodeShouldBe()
    And these etags should have changed:                                                                        # WebDavPropertiesContext::theseEtagsShouldHaveChanged()
      | user  | path           |
      | Alice | /              |
      | Alice | /upload        |
      | Brian | /              |
      | Brian | /Shares        |
      | Brian | /Shares/upload |

    Examples:
      | dav_version |
      | old         |
        WebDavPropertiesContext::theseEtagsShouldHaveChanged
        The etag '"d9723673601f444f347d78776272b41d"' of element '/upload' of user 'Alice' did not change.
        Failed asserting that 1 matches expected 0.
      | new         |
      | spaces      |
        Expected stored etag to be some string but found null! (Exception)

[phil-davis]

seems to contradict reva apiAuthWebDav/webDavPUTAuth.feature:70

https://github.com/owncloud/core/pull/39718/files#diff-6f66858efef616c8d86267ddb370262dabe13b0e8a63f175b734777f5db66945

Those scenarios were added with the same "403" expectation as for the /remote.php/dav/files/%username% endpoint. In oC10 it gives 403 "forbidden". And I think that is a problem/feature in oC10 also. I get a different HTTP 4xx status if I try to access another real user, or if I try to access a non-existent user.

The requirements need to be thought about and defined - probably a consistent 404 for all these combinations of trying to access something that may or may not "really" exist, but it does not exist from the point-of-view of the authenticated user. Then make the test scenarios expect the proper requirements. Then make both oC10 and oCIS meet the requirements. Part of the problem here is that we have oC10 core tests that demonstrate current oC10 behavior, but actually the current behavior is not correct/optimal.

[butonic]

@phil-davis I agree. Regardless of the error, it you don't have read access to the storage all you get should be a 404.

Currently, only some error paths of the ocdav service do a stat request to determine if a user can read file.

This actually differs from oc10 where you get a 403.

The initial spaces implementation required a space lookup, when you didn't have access you would always get a 404. For performance reasons we moved the space lookup from the storage registry to the storage provider.

That ocdav does a stat after an error is technical debt. The storage providers would need to return 404 when the user has no access to the root. That could be done in the storage provider and is not necessary required to be done by the storage drivers.

[butonic]

In any case this pr can now point to Reva edge, as both cis are green and the Reva pr has been merged.

[phil-davis]

In any case this pr can now point to Reva edge, as both cis are green and the Reva pr has been merged.

Agree - are you going to update this PR?

And after you do, and CI goes green, IMO it can be merged.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information