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

bump reva #8649

Merged
Merged

Conversation

dragonchaser
Copy link
Member

@dragonchaser dragonchaser commented Mar 14, 2024

Bugfix: Prevent copying a file to a parent folder

When copying a file to its parent folder, the file would be copied onto the parent folder, moving the parent to the trash-bin.

#1230

@dragonchaser dragonchaser force-pushed the ocis-1230-do-not-overwrite-parent branch from ef3a5eb to 2bd0191 Compare March 14, 2024 10:16
@saw-jan
Copy link
Member

saw-jan commented Mar 14, 2024

Findings:

  1. Some test failed because the expecetd statuscode was different (should be 409 now) - can be fixed in the test scenarios

  2. Cannot do MOVE using old/new webdav endpoints in the Shares - (works from spaces) ❌

    demo is a sharee here:

    curl -XMOVE "https://localhost:9200/remote.php/webdav/Shares/parent" \                                                                
    -H"Destination: https://localhost:9200/remote.php/webdav/Shares/sharedParent" \        
    -udemo:demo -vk
    
    < HTTP/1.1 404 Not Found
    curl -XMOVE "https://localhost:9200/remote.php/dav/files/demo/Shares/parent" \                                                                
    -H"Destination: https://localhost:9200/remote.php/dav/files/demo/Shares/sharedParent" \        
    -udemo:demo -vk
    
    < HTTP/1.1 404 Not Found

@saw-jan
Copy link
Member

saw-jan commented Mar 15, 2024

I get the 404 from this line

isParent, err := s.referenceIsChildOf(ctx, s.gatewaySelector, src, dst)
if err != nil {
switch err.(type) {
case errtypes.IsNotFound:
w.WriteHeader(http.StatusNotFound)

with old/new webdav, dst returns 404

parentStatRes, err := client.Stat(ctx, &provider.StatRequest{Ref: parent})
if err != nil {
return false, err

@dragonchaser
Copy link
Member Author

This is weird, this is not supposed to happen, almost the same code is running in line 144 of move.go....

@saw-jan
Copy link
Member

saw-jan commented Mar 15, 2024

This is weird, this is not supposed to happen, almost the same code is running in line 144 of move.go....

Yeah, only difference is that the src and dst are exchanged. May be client.Stat( is not handling them properly while using old/new webdav

@dragonchaser
Copy link
Member Author

@saw-jan yes but the same code is running in copy.go properly....

@dragonchaser
Copy link
Member Author

@saw-jan cs3org/reva#4582 @aduffeck saw the issue :)

@dragonchaser dragonchaser force-pushed the ocis-1230-do-not-overwrite-parent branch from 2bd0191 to 10bc55e Compare March 18, 2024 08:53
@saw-jan
Copy link
Member

saw-jan commented Mar 18, 2024

https://drone.owncloud.com/owncloud/ocis/32956/18/6
COPY between spaces now give 409. It should be possible to COPY cross spaces

Scenario Outline: user copies a file from a project space with a different role to a project space with the manager role              # /drone/src/tests/acceptance/features/apiSpacesShares/copySpaces.feature:47
    Given the administrator has assigned the role "Space Admin" to user "Brian" using the Graph API                                     # GraphContext::theAdministratorHasGivenTheRoleUsingTheGraphApi()
    And user "Brian" has created a space "Project1" with the default quota using the Graph API                                          # SpacesContext::theUserHasCreatedASpaceByDefaultUsingTheGraphApi()
    And user "Brian" has created a space "Project2" with the default quota using the Graph API                                          # SpacesContext::theUserHasCreatedASpaceByDefaultUsingTheGraphApi()
    And user "Brian" has uploaded a file inside space "Project1" with content "Project1 content" to "/project1.txt"                     # SpacesContext::userHasUploadedFile()
    And user "Brian" has shared a space "Project2" with settings:                                                                       # SpacesContext::userHasSharedSpace()
      | shareWith | Alice     |
      | role      | <to_role> |
    And user "Brian" has shared a space "Project1" with settings:                                                                       # SpacesContext::userHasSharedSpace()
      | shareWith | Alice       |
      | role      | <from_role> |
    When user "Alice" copies file "/project1.txt" from space "Project1" to "/project1.txt" inside space "Project2" using the WebDAV API # SpacesContext::userCopiesFileFromAndToSpaceBetweenSpaces()
    Then the HTTP status code should be "201"                                                                                           # FeatureContext::thenTheHTTPStatusCodeShouldBe()
    And for user "Alice" the space "Project2" should contain these entries:                                                             # SpacesContext::userTheSpaceShouldContainEntries()
      | /project1.txt |
    And for user "Alice" the content of the file "/project1.txt" of the space "Project2" should be "Project1 content"                   # SpacesContext::checkFileContent()

    Examples:
      | from_role | to_role |
      | manager   | manager |
        Failed step: Then the HTTP status code should be "201"
        HTTP status code 409 is not the expected value 201
        Failed asserting that 409 matches expected '201'.

@dragonchaser dragonchaser force-pushed the ocis-1230-do-not-overwrite-parent branch from 10bc55e to 767dbfa Compare March 18, 2024 13:20
Signed-off-by: Christian Richter <crichter@owncloud.com>
@dragonchaser dragonchaser force-pushed the ocis-1230-do-not-overwrite-parent branch 2 times, most recently from 2b07b18 to aef8661 Compare March 18, 2024 14:33
@saw-jan
Copy link
Member

saw-jan commented Mar 18, 2024

These two tests also need statuscode adjustment:

apiSpacesDavOperation/moveByFileId.feature:741
apiSpacesDavOperation/moveByFileId.feature:742

@dragonchaser dragonchaser force-pushed the ocis-1230-do-not-overwrite-parent branch from aef8661 to 21fb1c1 Compare March 18, 2024 14:52
Signed-off-by: Christian Richter <crichter@owncloud.com>
@dragonchaser dragonchaser force-pushed the ocis-1230-do-not-overwrite-parent branch from 21fb1c1 to 6ddac23 Compare March 18, 2024 15:06
Copy link

sonarcloud bot commented Mar 18, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@dragonchaser dragonchaser marked this pull request as ready for review March 18, 2024 15:37
@dragonchaser dragonchaser merged commit e8ab328 into owncloud:master Mar 18, 2024
4 checks passed
@dragonchaser dragonchaser deleted the ocis-1230-do-not-overwrite-parent branch March 18, 2024 15:39
ownclouders pushed a commit that referenced this pull request Mar 18, 2024
@micbar micbar mentioned this pull request Jun 19, 2024
24 tasks
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.

3 participants