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

Prevent recursive copy/move operations on the API level #3009

Merged
merged 9 commits into from
Jul 5, 2022

Conversation

aduffeck
Copy link
Contributor

@aduffeck aduffeck commented Jun 24, 2022

Fixes owncloud/ocis#3992

and owncloud/ocis#3023

and owncloud/ocis#4098

(people keep reporting this!)

@aduffeck aduffeck force-pushed the prevent-recursive-copy-move branch from f59eaf0 to ca09131 Compare June 24, 2022 09:27
@aduffeck aduffeck closed this Jun 27, 2022
@aduffeck aduffeck force-pushed the prevent-recursive-copy-move branch from 3b5e1b9 to f44e9e8 Compare July 4, 2022 09:23
Copy link
Contributor

@butonic butonic left a comment

Choose a reason for hiding this comment

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

I see now why you were unhappy. Doing a GetPath request as the owner requires ocdav to know the machine auth api key ...

what would happen if we send the GetPath as the current user?

In theory the storage driver should give us a path relative to the same root when calling GetPath with two different references. Each call should look up the highest resourceid the user has access to and we should be able to compare the path. We can also check thet the references resourceid that we get in GetPath is the same ... dang ... GetPath has no resource id ... fck

I remember discussing having to extend GetPath with the space root for exactly this reason ... maybe in #1721 (comment)

Then again, we extended ResourceInfo with Name and could use the filedmask to query Path and Space in a Stat request instead of a GetPath request ... 🤔

Comment on lines +277 to +283
if isShareJailRoot(req.ResourceId) {
return &provider.GetPathResponse{
Status: status.NewOK(ctx),
Path: "/",
}, nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 minimal implementation so we can get the path to the share jail root

@@ -690,7 +703,8 @@ func (s *service) Stat(ctx context.Context, req *provider.StatRequest) (*provide
PermissionSet: &provider.ResourcePermissions{
// TODO
},
Etag: shareMd[earliestShare.Id.OpaqueId].ETag,
Etag: shareMd[earliestShare.Id.OpaqueId].ETag,
Owner: owner.Id,
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 yes, the share jail is always owned by the current user

}
if isChild {
w.WriteHeader(http.StatusBadRequest)
b, err := errors.Marshal(http.StatusBadRequest, "can not copy a folder into one of its children", "")
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@phil-davis
Copy link
Contributor

phil-davis commented Jul 5, 2022

@aduffeck this is good and fixes the loop. cc: @butonic

Now the related test fails because the API request returns HTTP status 400 in this case. But oC10 returns 409 in this case.

Is it easy to make another PR that adjusts the return status to 409?

See my updated issue owncloud/ocis#3023 and PR #3030 that enables the test again.

@aduffeck
Copy link
Contributor Author

aduffeck commented Jul 5, 2022

@phil-davis changing the response code will be a trivial change. I'll prepare a PR soon.

@phil-davis
Copy link
Contributor

@phil-davis changing the response code will be a trivial change. I'll prepare a PR soon.

Done in #3031 and works.

@aduffeck what happens about reva master branch? Do we backport this type of fix?

reva master branch also has the ~@issue-ocis-3023 sspecified in CI to skip the test scenarios related to this.

kobergj pushed a commit that referenced this pull request Jul 7, 2022
* Prevent recursive copy/move operations on the API level

* Add changelog

* Enable machine auth in ocdav

* Fix error message

* Also detect recursive operations when the SSP is involved

* Fix hound issues

* Set the owner on the stat response to the share jail

* Implement GetPath() for the share jail root

* Return a better error when the ocdav machine auth config is missing
kobergj pushed a commit to kobergj/reva that referenced this pull request Jul 11, 2022
* Prevent recursive copy/move operations on the API level

* Add changelog

* Enable machine auth in ocdav

* Fix error message

* Also detect recursive operations when the SSP is involved

* Fix hound issues

* Set the owner on the stat response to the share jail

* Implement GetPath() for the share jail root

* Return a better error when the ocdav machine auth config is missing
kobergj added a commit to kobergj/reva that referenced this pull request Jul 14, 2022
@micbar micbar mentioned this pull request Jul 19, 2022
36 tasks
@kobergj kobergj mentioned this pull request Dec 19, 2022
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