[OC storage] file operations not working as share recipient #205
Comments
exalate-issue-sync
bot
added
bug
|
Vincent Petry commented: Reproducible locally. Upload and download as share recipient is broken. Bisecting... |
|
Vincent Petry commented: also broken in ocis v1.0.0-beta9 so it's not a regression... |
|
Vincent Petry commented: broken also in beta8, beta7 and beta6 but in a different way. it feels like it maybe was never working. |
|
Benedikt Kulmann commented: |
|
Vincent Petry commented: it seems the problem is specific to OC storage. this is on ocis master (ba3eb488e39b54a45ef40fd9608fb82b2e2b78c6) |
|
Vincent Petry commented: strange thing that we observed that kind of behavior before with [~jfd] |
|
Benedikt Kulmann commented: I can confirm that it works fine with EOS storage. |
|
Vincent Petry commented: Seems this was already known before: owncloud/ocis-reva#359 |
|
Remote key is https://jira.owncloud.com/browse/OCIS-446 |
PVince81
changed the title
|
creating folders also broken. likely related to that strange issue with ocfs's it seems that it's passing in an absolute path to ocfs when the path should be relative |
|
when creating a directory outside of shares, the "fn" is relative: when creating it inside the shared folder, it becomes absolute: so the issue is likely not in the owncloud storage itself but could be outside, in the caller. |
|
something already wrong on storage provider level: fn should be relative here as well... |
|
Deletion also not working: interesting are these lines in the middle: resolving storage by id which will pick 9154, which is the "storage home". somehow this reminds me of the problem we had with EOS: #129 @butonic I think we'll need the same kind of config gymnastics with the storage id cheat also for OC storage, and later for OCIS storage... |
|
I had a look at the EOS way. The difference is that EOS has both "eos" and "eoshome" drivers where "eos" is the root folder and "eoshome" points to user homes. However for the OC storage driver (and also later OCIS storage in owncloud/ocis-reva#461), we only have the home one (because we need to set to enable_home=true for everything to work correctly). So either we need to create a second driver "ochome" like "eoshome" and use the same mount id overlapping tricks like in #129. Or change the implementation to be always able to work with the root storage. It feels like this would be more work. |
|
Just tried a very naive attempt, starting ocis with but I only managed to make FindProvider switch to the other one, but the unwrap is still receiving the full path as before: Maybe the problem is a different one... |
|
seems it's still aiming to the wrong storage despite the override above: From my understanding the path should become "p=/oc/4c510ada-c86b-4815-8820-42cdf82c3d51/test/abc". And I found this FindProvider that just appends the absolute path after "/oc": |
|
I do find it strange that doing a PROPFIND there (in "Shares/test/sub") works fine, it seems ListContainer is not doing the same kind of things that CreateContainer does. |
|
I think I've got something, still with the above env change: I see that the prefix from which to trim the home is based on the current user but should actually be based on the share owner. From what I see the code would always be working with the currently logged in user, so |
|
I tried to compare with the EOS wrap/unwrap code but it isn't that different, they both rely on the user in the context. But the EOS approach works (with its special config). I'm not clear yet where in the code the share would be resolved from Marie's "Shares/test/abc" to the Einstein's "/test/abc". |
|
I've switched back to the original scenario, no overrides. Trying to look into why listing folders work but not other operations. This is some of the flow (with local log debug statements): from what I see the reference that is passed to For CreateDir there is no reference passed in, only a string... |
|
so it seems that and it does so for CreateContainer as well! even CreateDir receives the correct path. but somehow it seems the code insists on still trying to wrap it, while it doesn't wrap it with ListFolder. |
|
... and, got it: https://github.com/cs3org/reva/blob/master/pkg/storage/fs/owncloud/owncloud.go#L1582 the only reason the PROPFIND code path works if before of the above referenced line: it checks if the path is an absolute one and then does some special handling that skips the we could in theory add this logic elsewhere. BUT: it feels wrong to have a difference between relative and absolute paths there |
|
it feel redundant that there is some share splitting / resolving logic in the storage provider but also some similar logic in the ownCloud storage implementation, and potentially all of them. |
|
furthermore, having the complete absolute path visible here feels like a security issue: if someone would be able to modify that path they could potentially access any folder on the storage, not only the actual one (for example access "/var/log" instead of just "/var/tmp/reva/..." so the proper solution would likely involve having the root storage vs home storage, at least in a way like it works with EOS. |
|
next up: checking how it's done for EOS storage |
|
as far as I can see, with EOS, only the relative path is given: ... |
|
and here I see a difference happening how the storage is resolving the opaque id: EOS: => resolution happens on the root and so we find the correct user and path relative to the storage ownCloud storage: => the resolve call is using when looking at redis, I found absolute paths: so perhaps here the solution is to make the paths stored in redis relative to the storage... |
|
could the getOwner implementation be at fault? https://github.com/cs3org/reva/blob/master/pkg/storage/fs/owncloud/owncloud.go#L502 it always takes the owner from the path ... 🤔 |
|
from what I see now we'd trim the path prefix anyway, the problem is that the path prefix is using Marie's user id (currently logged in user) but the input path is the one from Einstein, so it cannot be trimmed. Back to square one. we should look together into aligning the OC storage config to the one we made for EOS @butonic |
|
I've pushed my branch where I added a lot more logging which can be seen above: cs3org/reva#1147 |
|
@phil-davis what you see looks like a different issue. I grepped my logs and didn't find any issues related to accounts. |
4 tasks
|
fix is here, some config changes: owncloud/ocis-reva#461 |
This was referenced Sep 9, 2020
Closed
|
Vincent Petry commented: Fix was merged into ocis with owncloud/ocis-reva#466 Needs verification |
|
Vincent Petry commented: Tested on ocis.owncloud.works, works fine 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Steps to reproduce
Expected result
Creation works
Actual result
Creation fails with log entries with wrongly concatenated absolute paths (see analysis below)
Additional info
Same issue with upload, download, delete
The text was updated successfully, but these errors were encountered: