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

[full-ci] bump reva to c33c803283ee #10172

Merged
merged 4 commits into from
Sep 30, 2024
Merged

[full-ci] bump reva to c33c803283ee #10172

merged 4 commits into from
Sep 30, 2024

Conversation

butonic
Copy link
Member

@butonic butonic commented Sep 26, 2024

pull in cs3org/reva#4835 to test a fix for #9933

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
Copy link

update-docs bot commented Sep 26, 2024

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.

@butonic
Copy link
Member Author

butonic commented Sep 26, 2024

ok, this fails:

  @issue-4421

  Scenario Outline: sharee PROPFIND same name shares shared by multiple users using new dav path                     # /dronesrc/tests/acceptance/features/apiSharingNg1/propfindShares.feature:53
    Given using <dav-path-version> DAV path                                                                          #FeatureContext::usingOldOrNewDavPath()
    And user "Alice" has uploaded file with content "to share" to "textfile.txt"                                     #FeatureContext::userHasUploadedAFileWithContentTo()
    And user "Alice" has created folder "folderToShare"                                                              #FeatureContext::userHasCreatedFolder()
    And user "Carol" has uploaded file with content "to share" to "textfile.txt"                                     #FeatureContext::userHasUploadedAFileWithContentTo()
    And user "Carol" has created folder "folderToShare"                                                              #FeatureContext::userHasCreatedFolder()
    And user "Alice" has sent the following resource share invitation:                                               #SharingNgContext::userHasSentTheFollowingResourceShareInvitation()
    | resource        | <resource> |
    | space           | Personal   |
    | sharee          | Brian      |
    | shareType       | user       |
    | permissionsRole | Viewer     |
    And user "Brian" has a share "<resource>" synced                                                                 #SharingNgContext::userHasShareSynced()
    And user "Carol" has sent the following resource share invitation:                                               #SharingNgContext::userHasSentTheFollowingResourceShareInvitation()
    | resource        | <resource> |
    | space           | Personal   |
    | sharee          | Brian      |
    | shareType       | user       |
    | permissionsRole | Viewer     |
    And user "Brian" has a share "<resource-2>" synced                                                               #SharingNgContext::userHasShareSynced()
    When user "Brian" sends PROPFIND request from the space "Shares" to the resource "Shares" using the WebDAV API   #SpacesContext::userSendsPropfindRequestFromTheSpaceToTheResourceWithDepthUsingTheWebdavApi()
    Then the HTTP status code should be "207"                                                                        #FeatureContext::thenTheHTTPStatusCodeShouldBe()
    And the "PROPFIND" response to user "Brian" should contain a space "Shares" with these key and value pairs:      #SpacesContext::theResponseShouldContainMountPoint()
    | key       | value         |
    | oc:fileid | UUIDof:Shares |
    | oc:name   | Shares        |
    And the "PROPFIND" response to user "Brian" should contain a mountpoint "Shares" with these key and value pairs: #SpacesContext::theResponseShouldContainMountPoint()
    | key            | value             |
    | oc:fileid      | UUIDof:<resource> |
    | oc:name        | <resource>        |
    | oc:permissions | S                 |
    And the "PROPFIND" response to user "Brian" should contain a mountpoint "Shares" with these key and value pairs: #SpacesContext::theResponseShouldContainMountPoint()
    | key            | value               |
    | oc:fileid      | UUIDof:<resource-2> |
    | oc:name        | <resource-2>        |
    | oc:permissions | S                   |
  Examples:
    | dav-path-version | resource      | resource-2        |
    | old              | textfile.txt  | textfile (1).txt  |
        Failed step: And the "PROPFIND" response to user "Brian" should contain a mountpoint "Shares" with these key and valuepairs:
      wrong fileId in the response
        Failed asserting that an array contains 'a0ca6a90-a365-4782-871e-d44447bbc668$a0ca6a90-a365-4782-871e-d44447bbc668!a4fd2951-56cf-4bd4-88b7-d05f299384d0:9ddc3a0-055d-46f3-b47a-222cf50f01d0:526c46a9-7ded-4e54-ae6d-b57433a9a419'.
    | old              | folderToShare | folderToShare (1) |
        Failed step: And the "PROPFIND" response to user "Brian" should contain a mountpoint "Shares" with these key and valuepairs:
      wrong fileId in the response
        Failed asserting that an array contains 'a0ca6a90-a365-4782-871e-d44447bbc668$a0ca6a90-a365-4782-871e-d44447bbc668!a4fd2951-56cf-4bd4-88b7-d05f299384d0:38e0f3d-1bbf-4964-92e0-ea5d4cdc8fd5:b13cdcfa-5eb8-44bb-ba08-ac763e52f3ae'.
    | new              | textfile.txt  | textfile (1).txt  |
        Failed step: And the "PROPFIND" response to user "Brian" should contain a mountpoint "Shares" with these key and valuepairs:
      wrong fileId in the response
        Failed asserting that an array contains 'a0ca6a90-a365-4782-871e-d44447bbc668$a0ca6a90-a365-4782-871e-d44447bbc668!a4fd2951-56cf-4bd4-88b7-d05f299384d0:40676d9-9256-4b21-9c4d-aceeede012e4:e0f82b48-8997-4e0d-b18c-3ef16ca57983'.
    | new              | folderToShare | folderToShare (1) |
        Failed step: And the "PROPFIND" response to user "Brian" should contain a mountpoint "Shares" with these key and valuepairs:
      wrong fileId in the response
        Failed asserting that an array contains 'a0ca6a90-a365-4782-871e-d44447bbc668$a0ca6a90-a365-4782-871e-d44447bbc668!a4fd2951-56cf-4bd4-88b7-d05f299384d0:d39f88a-f1f2-4792-93b3-688a4b1d049b:fc5efc5d-94d4-4b53-be5e-d83f352ab56f'.

@issue-4421

🤔

ok ... the test is wrong, it assumes the resource id of the shared resource is the same as the mountpoint. unfortunately that is no longer a valid assumption.

@butonic
Copy link
Member Author

butonic commented Sep 26, 2024

That leaves

Scenario Outline: recipient of the shared resource tries to remove a tag                                   # /drone/src/tests/acceptance/features/apiSpaces/tag.feature:153
  Given user "Alice" has sent the following resource share invitation:                                     # SharingNgContext::userHasSentTheFollowingResourceShareInvitation()
    | resource        | folderMain         |
    | space           | use-tag            |
    | sharee          | Brian              |
    | shareType       | user               |
    | permissionsRole | <permissions-role> |
  And user "Brian" has a share "folderMain" synced                                                         # SharingNgContext::userHasShareSynced()
  And user "Alice" has created the following tags for <resource-type> "<resource>" of the space "use-tag": # TagContext::theUserHasCreatedFollowingTags()
    | tag in a shared resource |
    | second tag               |
  When user "Brian" removes the following tags for <resource-type> "<resource>" of space "Shares":         # TagContext::userRemovesTagsFromResourceOfTheSpace()
    | tag in a shared resource |
    | second tag               |
  Then the HTTP status code should be "<http-status-code>"                                                 # FeatureContext::thenTheHTTPStatusCodeShouldBe()
  When user "Alice" lists all available tags via the Graph API                                             # TagContext::theUserGetsAllAvailableTags()
  Then the HTTP status code should be "200"                                                                # FeatureContext::thenTheHTTPStatusCodeShouldBe()
  And the response <should-or-not> contain following tags:                                                 # TagContext::theFollowingTagsShouldExistForUser()
    | tag in a shared resource |
    | second tag               |
  Examples:
    | permissions-role | resource-type | resource                       | http-status-code | should-or-not |
    | Viewer           | file          | folderMain/insideTheFolder.txt | 403              | should        |
    | Editor           | file          | folderMain/insideTheFolder.txt | 200              | should not    |
    | Viewer           | folder        | folderMain                     | 403              | should        |
    | Editor           | folder        | folderMain                     | 200              | should not    |        Failed step: And the response should not contain following tags:        the response should not contain the tag tag in a shared resource.        Response        Array        (
          [0] => second tag
          [1] => tag in a shared resource        )                Failed asserting that true is false.

@ScharfViktor
Copy link
Contributor

ok ... the test is wrong, it assumes the resource id of the shared resource is the same as the mountpoint. unfortunately that is no longer a valid assumption.

after debugging I see that:

PROPFIND https://ocis-server:9200/remote.php/dav/spaces/a0ca6a90-a365-4782-871e-d44447bbc668$a0ca6a90-a365-4782-871e-d44447bbc668/folderToShare (1) virtualSpaceUUID/folderName doesn't contain folderUUid "4b371513-f50c-4f21-b071-ab596d38a246$2c7ab090-26ec-4845-823a-80cd51248cbd!f84f399f-37c5-49be-b2ed-b847fbcb2141"

but in the same case when folder wasn't renamed (prefics (1)) -> that works fine

In the process, I honestly got confused with these tons of uuid and what should be there and what should not.

here is my screens:
Screenshot 2024-09-27 at 12 49 08

Screenshot 2024-09-27 at 13 01 27

@butonic
Copy link
Member Author

butonic commented Sep 30, 2024

Correct. #9933 is about a mountpoint having different ids, depending on which href you use:

When listing the space root the <oc:id> shows an id from the share jail (indicated by it starting with a0ca6a90-a365-4782-871e-d44447bbc668$a0ca6a90-a365-4782-871e-d44447bbc668)

PROPFIND https://cloud.ocis.test/dav/spaces/a0ca6a90-a365-4782-871e-d44447bbc668$a0ca6a90-a365-4782-871e-d44447bbc668/

<oc:id>a0ca6a90-a365-4782-871e-d44447bbc668$a0ca6a90-a365-4782-871e-d44447bbc668!166d1210-cdb9-50ab-9f1e-ecb9ef12a304:4c510ada-c86b-4815-8820-42cdf82c3d51:3ef59385-34ba-4055-b705-f4f05f1388a4</oc:id>

But when using a PROPFIND against the same resource, but directly referencing it it shows the id from the shared resource:

PROPFIND https://cloud.ocis.test/dav/spaces/a0ca6a90-a365-4782-871e-d44447bbc668$a0ca6a90-a365-4782-871e-d44447bbc668/Universe/

<oc:id>166d1210-cdb9-50ab-9f1e-ecb9ef12a304$4c510ada-c86b-4815-8820-42cdf82c3d51!17634536-719e-47c4-bf8b-e8b97188259a</oc:id>

That confuses clients who remember the fileid: Android, iOS and deskop as well, but I cannot find an issue right now. @TheOneRing might be able to provide a link.

In your screenshots, the

PROPFIND https://ocis-server:9200/remote.php/dav/spaces/a0ca6a90-a365-4782-871e-d44447bbc668$a0ca6a90-a365-4782-871e-d44447bbc668/folderToShare (1)

response contains multiple resource ids:

  1. the id of the containing folder folderToShare (1) which, with this PR, matches the id returned when PROPFINDing the share jail.
  2. the id of the child textfile.txt which points to the shared spaces.

That is how it should be, because the folderToShare (1) in the share jail is a different resoucre than the resource that was shared (folderToShare in Alice home space).

The share jail of Brian should only contain resource ids from the share jail when listing the root: the root directory in the share jail always has the id a0ca6a90-a365-4782-871e-d44447bbc668$a0ca6a90-a365-4782-871e-d44447bbc668!a0ca6a90-a365-4782-871e-d44447bbc668, the children - all of them are mountpoints - all have an id from the sharejail as well (in the form of a0ca6a90-a365-4782-871e-d44447bbc668$a0ca6a90-a365-4782-871e-d44447bbc668!{shareid}).

When listing these sub directories, the child id should become the root id and the children will have the resource id from the actual space.

Before this PR, the root id would also wrongly change to the resource id from the actual space.

I hope this clarifies the expectations.

@ScharfViktor
Copy link
Contributor

regarding

Scenario Outline: recipient of the shared resource tries to remove a tag                                   # /drone/src/tests/acceptance/features/apiSpaces/tag.feature:153
    Given user "Alice" has sent the following resource share invitation:                                     # SharingNgContext::userHasSentTheFollowingResourceShareInvitation()
      | resource        | folderMain         |
      | space           | use-tag            |
      | sharee          | Brian              |
      | shareType       | user               |
      | permissionsRole | <permissions-role> |
    And user "Brian" has a share "folderMain" synced                                                         # SharingNgContext::userHasShareSynced()
    And user "Alice" has created the following tags for <resource-type> "<resource>" of the space "use-tag": # TagContext::theUserHasCreatedFollowingTags()
      | tag in a shared resource |
      | second tag               |
    When user "Brian" removes the following tags for <resource-type> "<resource>" of space "Shares":         # TagContext::userRemovesTagsFromResourceOfTheSpace()
      | tag in a shared resource |
      | second tag               |
    Then the HTTP status code should be "<http-status-code>"                                                 # FeatureContext::thenTheHTTPStatusCodeShouldBe()
    When user "Alice" lists all available tags via the Graph API                                             # TagContext::theUserGetsAllAvailableTags()
    Then the HTTP status code should be "200"                                                                # FeatureContext::thenTheHTTPStatusCodeShouldBe()
    And the response <should-or-not> contain following tags:                                                 # TagContext::theFollowingTagsShouldExistForUser()
      | tag in a shared resource |
      | second tag               |

    Examples:
      | permissions-role | resource-type | resource                       | http-status-code | should-or-not |
      | Viewer           | file          | folderMain/insideTheFolder.txt | 403              | should        |
      | Editor           | file          | folderMain/insideTheFolder.txt | 200              | should not    |
      | Viewer           | folder        | folderMain                     | 403              | should        |
      | Editor           | folder        | folderMain                     | 200              | should not    |
        Failed step: And the response should not contain following tags:
        the response should not contain the tag tag in a shared resource.
        Response
        Array
        (
            [0] => second tag
            [1] => tag in a shared resource
        )
        
        Failed asserting that true is false.
  1. tests used PROPFIND "https://ocis-server:9200/dav/spaces/a0ca6a90-a365-4782-871e-d44447bbc668$a0ca6a90-a365-4782-871e-d44447bbc668/folderMain" to get folderId -> folderID is "a0ca6a90-a365-4782-871e-d44447bbc668$a0ca6a90-a365-4782-871e-d44447bbc668!4b371513-f50c-4f21-b071-ab596d38a246:7b277db6-7f0d-4b21-a98a-41253295349d:4cb75799-8708-426d-a382-d93cbb7560c2"

  2. using this folderUUid tries to delete tag
    curl 'https://ocis-server:9200/graph/v1.0/extensions/org.libregraph/tags' -XDELETE -ubrian:1234 -d'{"resourceId":"a0ca6a90-a365-4782-871e-d44447bbc668$a0ca6a90-a365-4782-871e-d44447bbc668!4b371513-f50c-4f21-b071-ab596d38a246:7b277db6-7f0d-4b21-a98a-41253295349d:4cb75799-8708-426d-a382-d93cbb7560c2","tags":["second tag"]}' -vk

  3. user gets 200 but tags are exist: GET https://ocis-server:9200/graph/v1.0/extensions/org.libregraph/tags ["second tag", "tag in a shared resource"]

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
@butonic
Copy link
Member Author

butonic commented Sep 30, 2024

we were using the resource id from the share jail instead of the actual resource id. fixed with 0f525e9

there might be other code paths were the share jail id is used. all clients should try to look up the actual resource id and not rely on the id returned by the sharejail.

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
@butonic butonic changed the title [full-ci] bump reva to 02af5a266 [full-ci] bump reva to c33c803283ee Sep 30, 2024
@butonic
Copy link
Member Author

butonic commented Sep 30, 2024

I merged the reva PR as @ScharfViktor and fixed the remaining bug and added the sharee PROPFIND same name shares shared by multiple users using new dav path scenario to expected failures for now.

enabling auto merge

Copy link

sonarcloud bot commented Sep 30, 2024

@butonic butonic merged commit ecad57a into master Sep 30, 2024
3 checks passed
ownclouders pushed a commit that referenced this pull request Sep 30, 2024
@butonic butonic deleted the fix-sharejail-stat-id branch September 30, 2024 13:23
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