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

update reva & add share prefix to ocs shared with me paths #994

Merged
merged 5 commits into from
Dec 3, 2020

Conversation

C0rby
Copy link
Contributor

@C0rby C0rby commented Nov 30, 2020

@C0rby
Copy link
Contributor Author

C0rby commented Nov 30, 2020

Ok the tests fail because the file_target has the /Shares/ prefix now.
Also I need to look into the currently skipped tests which test the behavior of the storage with the /Shares/ prefix e.g. owncloud/product#203

@C0rby C0rby force-pushed the fix-ocs-shared-with-me branch 3 times, most recently from b081dbb to 30500cf Compare December 2, 2020 13:01
@C0rby C0rby changed the title add share prefix to ocs shared with me paths update reva & add share prefix to ocs shared with me paths Dec 3, 2020
Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was expecting to find that the related core API test scenario started to pass. But https://drone.owncloud.com/owncloud/ocis/1962/21/6 and https://drone.owncloud.com/owncloud/ocis/1962/22/6

  Scenario: send PROPFIND requests to another user's webDav endpoints as normal user                          # /srv/app/testrunner/tests/acceptance/features/apiAuthWebDav/webDavPROPFINDAuth.feature:37
    When user "Brian" requests these endpoints with "PROPFIND" to get property "d:getetag" about user "Alice" # OCSContext::userSendsRequestToTheseEndpointsWithProperty()
      | endpoint                                           |
      | /remote.php/dav/files/%username%/textfile0.txt     |
      | /remote.php/dav/files/%username%/PARENT            |
      | /remote.php/dav/files/%username%/PARENT/parent.txt |
    Then the HTTP status code of responses on all endpoints should be "404"                                   # FeatureContext::theHTTPStatusCodeOfResponsesOnAllEndpointsShouldBe()
      Responses did not return expected http status code
      Failed asserting that 403 is identical to 404.

oC10 core responds with 404. But the OCIS code here is responding with 403 (the same issue on both storages)
What to do?

@@ -0,0 +1,6 @@
Bugfix: Fix path of files shared with me in ocs api

The path of files shared with me using the ocs api we pointing to an incorrect location.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The path of files shared with me using the ocs api we pointing to an incorrect location.
The path of files shared with me using the ocs api was pointing to an incorrect location.

@refs
Copy link
Member

refs commented Dec 3, 2020

I recommend doing make clean build locally so that ocis/go.sum updates reva as well

@C0rby
Copy link
Contributor Author

C0rby commented Dec 3, 2020

I was expecting to find that the related core API test scenario started to pass. But https://drone.owncloud.com/owncloud/ocis/1962/21/6 and https://drone.owncloud.com/owncloud/ocis/1962/22/6

  Scenario: send PROPFIND requests to another user's webDav endpoints as normal user                          # /srv/app/testrunner/tests/acceptance/features/apiAuthWebDav/webDavPROPFINDAuth.feature:37
    When user "Brian" requests these endpoints with "PROPFIND" to get property "d:getetag" about user "Alice" # OCSContext::userSendsRequestToTheseEndpointsWithProperty()
      | endpoint                                           |
      | /remote.php/dav/files/%username%/textfile0.txt     |
      | /remote.php/dav/files/%username%/PARENT            |
      | /remote.php/dav/files/%username%/PARENT/parent.txt |
    Then the HTTP status code of responses on all endpoints should be "404"                                   # FeatureContext::theHTTPStatusCodeOfResponsesOnAllEndpointsShouldBe()
      Responses did not return expected http status code
      Failed asserting that 403 is identical to 404.

oC10 core responds with 404. But the OCIS code here is responding with 403 (the same issue on both storages)
What to do?

I feel like we "fixed" that exact things multiple times and it changed everytime....
@refs @butonic, what response code do we want here?

@refs
Copy link
Member

refs commented Dec 3, 2020

@C0rby @butonic merged a PR on reva: cs3org/reva#1354 is all I'm aware of. I've been in my own auth bubble

@C0rby
Copy link
Contributor Author

C0rby commented Dec 3, 2020

Ok now I got it...
The response code was 207 before, now somebody changed it to 403 (which makes more sense) but in OC10 the same request would return a 404. Should we leave the 403 or should I open another PR in reva to change it to 404?

@C0rby
Copy link
Contributor Author

C0rby commented Dec 3, 2020

Ok, @refs and I decided to delete the test and leave the response code as it is for now.
If we need to change it we can do it in another PR

Copy link
Member

@refs refs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, we got ourselves in a catch 22 with that phoenix PR

@sonarcloud
Copy link

sonarcloud bot commented Dec 3, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@phil-davis phil-davis merged commit c905f6e into master Dec 3, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix-ocs-shared-with-me branch December 3, 2020 15:39
@phil-davis
Copy link
Contributor

we got ourselves in a catch 22 with that phoenix PR

There is some work happening to have an expected-failures file for the phoenix UI tests. I will chase that up tomorrow. That would have let us add some expected-failures with comments about what was going on, then remove them in a later PR, and always have CI showing green.

ownclouders pushed a commit that referenced this pull request Dec 3, 2020
Merge: a2cba46 96e75ce
Author: Phil Davis <phil@jankaritech.com>
Date:   Thu Dec 3 21:24:51 2020 +0545

    Merge pull request #994 from owncloud/fix-ocs-shared-with-me

    update reva & add share prefix to ocs shared with me paths
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.

Shared with me row path leading to wrong target
3 participants