-
Notifications
You must be signed in to change notification settings - Fork 182
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
Unexpected error sharing with users from mobile clients #9681
Comments
What was the resource Id? This error normally happens when somebody uses the resource id of an already shared file. E.g in the shares jail. |
These are the remote ids in Android DB (iOS also reproduces the problem): In text: 997aa698-6dbb-4fd4-84dc-166bde274a91$4c510ada-c86b-4815-8820-42cdf82c3d51!2fbb52ba-aa5d-4711-b707-ee62de6ca3ab Remote ids are the resource ids in the backend, so the ids that the client receives from endpoint. Is this what you asked for? |
Yes. Thanks. |
This is an ocs request. You know that ocs is deprecated? I need to check the web request but I think web sends another parameter for the space id. @kulmann @JammingBen can you help? |
Mobile clients still trusting ocs. Need some scalation? |
yes. I think so. We are building a lot of new features like secure view and transparent shares which will never be available on the OCS api. Deprecation happened with 5.0.0 in may. Web has already moved away from ocs So we need to move together „at some point“ |
web is the only client that moved away from supporting oc10. all other clients support both oc10 and ocis. moving on to SharingNG would also mean supporting both SharingNG and OCS at the same time. I know from experience that that's ugly... Any chance that dropping support for oc10 in desktop/android/ios is on the horizon? I'd strongly recommend to only move to SharingNG when dropping oc10 support at the same time... |
We could do it like web: maintain a „LTS“ branch for hotfixing and only do new features on sharing NG. |
any ETA for this? And, going back to the initial problem, what was missing in the initial request? i guess that next oCIS release will be out before oC10 is dropped, so, Android and iOS will need a fix for this. Any other new parameter in requests that we should mind? |
I guess you share a resource with (sharing permission) and you get 400, which is correct because we no longer support resharing #8842 here is tests which check this case: https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiReshare/resharing.feature#L138-L156 |
@ScharfViktor that's it,
31 includes share. I need clear confirmation because that issue will require changes in both mobile clients. |
yes, you need send 15 if you want to share with editor role
https://doc.owncloud.com/ocis/5.0/migration/upgrading_4.0.0_5.0.0.html#manage-breaking-changes you can also see here #8842 or wait @micbar @kulmann confirmation |
thanks a lot @ScharfViktor ETA for these changes to be consolidated? just to know when the clients must be ready for those changes @micbar @kulmann |
@micbar please have a look |
Having a deeper look into the current issue in ocis.ocis.master.owncloud.works, where the problem is reproducible: Assuming that In other words: if the client propfinds the server for the list of files, and the server responses for every item in the list:
I'm not clear with the expected behaviour. Please, mind again that clients have to work consistently against any oCIS and oC10 version. am i wrong somewhere? |
are you sure that |
No, you're right. if I found it in the right place https://github.com/cs3org/reva/blob/dde65a44013db3c4a8e8e5219a7707674838e410/internal/http/services/owncloud/ocs/conversions/role.go#L91-L107 |
I think there is a misunderstanding between server and client here: The |
So, is that wrong? in that case, the source of truth for resharing is the capability and only the capability? |
No that is not wrong. It is standing in this line exactly: https://github.com/cs3org/reva/blob/dde65a44013db3c4a8e8e5219a7707674838e410/internal/http/services/owncloud/ocs/conversions/role.go#L97
Exactly. Only capability decides if "share" can be added as permission on Shares. |
thanks for clarifying @kobergj , we will follow that path. |
Closing this one. |
Please correct me if I'm wrong, but this still breaks ever single client(ios&android) out there? |
Well yes. As far as I understood we still have a bug in ios and android clients which is allowing you to share with share permission even if you can't. But I'm not sure what we can do against it. (Except fix it in the client.) The server behaves correctly in rejecting the share request as resharing is not permitted. I don't think we should magically reduce permissions as this will confuse the user even more. The We can also not reactivate resharing as ocis doesn't support it any more. Any other ideas how to fix this? |
@hodyroff as experienced live in the EOSC demo... |
@kobergj is this still being worked on? (sharing in the mobile clients with rolling ocis is currently not possible and the mobile clients won't implement Sharing NG until the next production release in Nov. 2024) |
@TheOneRing reopened this ticket. From my side same as stated before: works as expected. No work needs to be done on server side. |
Unfortunately we need to be "bug compatible", as we can not update all clients at once, and we can not afford to have broken sharing in all mobiles. |
@jesmrec during implementation of the fix I recognized your curl request is broken. I hope you handcrafted it because if that is what the client is sending we might have bigger problem.
|
@dragotin @jesmrec We need to adjust a lot of tests so we need to know what exactly we need to allow. See this comment: cs3org/reva#4816 (comment) Should we only allow ignore the resharing when Please note that now if a user explicitly selects |
i didn't. Will review, anyway. The attention of that curl to be paid in the payload, not in the headers.
If the value is greater than 16, then If value is
that's right. |
@kobergj the difference could be:
It might be confusing the What would you expect in that case? (this is out of scope of that issue, if you think something is wrong, feel free to open issue in client repository). |
I would disagree. The content-length header tells the server how many bytes to read. It doesn't care if there are encoded strings in there. This is standard expected behaviour. |
i will move this to the android dev team |
Desktop client redirects sharing to the webUI and doesn't allow resharing. So, I think desktop-client is unaffected |
Yes you are right, the issue landed in the "Needs Testing" column by accident. |
Describe the bug
Using mobile clients Android and iOS, sharing with other users returns an error (reproduced in
ocis.ocis.master.owncloud.works
, in stable 5.0.6 works). But, it works in web client. Something different somewhereSteps to reproduce
Let's curlizy it (Android request):
Expected behavior
Content shared
Actual behavior
Error:
action was not a resharing, just a direct share of an item.
Setup
reproducible in
ocis.ocis.master.owncloud.works
ownCloud Web UI 9.2.0-alpha.1
Infinite Scale 6.1.0+baa0c23c3 Community
not reproducible in stable 5.0.6.... something is missing in the middle. I created the 5.0.6 instance with the following
docker-compose-yml
file:The text was updated successfully, but these errors were encountered: