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

Cannot reshare a file when Secure view is enabled #36751

Closed
davitol opened this issue Jan 13, 2020 · 15 comments
Closed

Cannot reshare a file when Secure view is enabled #36751

davitol opened this issue Jan 13, 2020 · 15 comments

Comments

@davitol
Copy link
Contributor

davitol commented Jan 13, 2020

Steps to reproduce

  1. Having oC server 10.4.0. RC1 and richdocuments: 2.2.0 and Secure View enabled
  2. Admin user shares a file with user1 with can share permission granted
  3. user1 tries to reshare with user2 the previous file

Expected behaviour

The file is reshared

Actual behaviour

An error is shown in Files View Cannot set the requested share attributes. No logs are spotted in owncloud.log
Screen Shot 2020-01-13 at 12 01 28

Screen Shot 2020-01-13 at 12 06 24

Note Disabling Secure View the reshare works fine

Server configuration

ownCloud 10.4.0. RC1 and richdocuments: 2.2.0

@pmaier1 the error message needs to be more user friendly
@mrow4a maybe for verifying the behavior It looks like a regression. Works in oC 10.3.2. Maybe related to #36265 ?

@pmaier1
Copy link
Contributor

pmaier1 commented Jan 13, 2020

For shares with Secure View permissions, disabled resharing is expected. To allow resharing there needs to be some more logic in place to prevent privilege escalations. For regular shares, resharing should be possible, though.

@phil-davis
Copy link
Contributor

What error text should be displayed?
It could give more reason why the reshare could not be done. But that will require more code in core (or the hooks in rich documents) that can "decode" the various "error" paths and insert suitable text.

@phil-davis
Copy link
Contributor

It is a bit unfortunate that the webUI by default attempts to reshare with all the permssions of the received share. If the webUI "understood" what are the maximum available resharing permissions in this context, then it could request only those by default. That would avoid the user getting this error message, and then having to understand and manually reduce the permission of the reshare attempt.

@davitol after getting the error message, can you then open the sharing settings, reduce the resharing permissions, and get a successful reshare? Or d you get stuck?

@davitol
Copy link
Contributor Author

davitol commented Jan 13, 2020

For shares with Secure View permissions, disabled resharing is expected. To allow resharing there needs to be some more logic in place to prevent privilege escalations. For regular shares, resharing should be possible, though.

Then I think we have to avoid user can mark 'Can share' textbox in Sharing menu when Secure View is enabled

Screen Shot 2020-01-13 at 13 50 42

@davitol
Copy link
Contributor Author

davitol commented Jan 13, 2020

@davitol after getting the error message, can you then open the sharing settings, reduce the resharing permissions, and get a successful reshare? Or d you get stuck?

Screen Shot 2020-01-13 at 12 01 28

@phil-davis Due to share is not deployed it is not possible to reduce the resharing permissions. If I reduce the permission in user1 then I get 'Sharing is not allowed' (expected behaviour and no issue)

@mrow4a
Copy link
Contributor

mrow4a commented Jan 13, 2020

@davitol I am not sure how it was possible, but generally "can share" should not be visible when "Secure View" is enabled. Can you give steps to reproduce showing "can share" with "secure view" ? what richdocuments version you have installed?

@davitol
Copy link
Contributor Author

davitol commented Jan 13, 2020

what richdocuments version you have installed?

@mrow4a ownCloud 10.4.0. RC1 and richdocuments: 2.2.0

Steps just the ones mentioned on the ticket. Anyway I will try again from scratch

@davitol
Copy link
Contributor Author

davitol commented Jan 13, 2020

Steps just the ones mentioned on the ticket. Anyway I will try again from scratch

Retested from scratch and reproduced again with ownCloud 10.4.0. RC1 and richdocuments: 2.2.0

Screen Shot 2020-01-13 at 14 58 20

@mrow4a
Copy link
Contributor

mrow4a commented Jan 13, 2020

@davitol what happens when you click on "can share" ? Isn't "secure view" disabled? I think we have have acceptance test for this - https://github.com/owncloud/richdocuments/blob/master/tests/acceptance/features/webUISecureView/main.feature

@micbar
Copy link
Contributor

micbar commented Jan 13, 2020

@felixheidecke Could you check, if we can change the field order with secure view enabled?

@davitol
Copy link
Contributor Author

davitol commented Jan 14, 2020

@davitol what happens when you click on "can share" ? Isn't "secure view" disabled? I think we have have acceptance test for this

SecureVPt

In the following gif it is shown what happens when clicking on "can share" with Secure View enabled. Also it shows how are ordered the secure view checkboxes

@mrow4a
Copy link
Contributor

mrow4a commented Jan 14, 2020

@davitol I start wonder how the acceptance tests in core and in richdocuments (https://drone.owncloud.com/owncloud/richdocuments/645) are passing, if this is possible. Something has been changed that broke the logic of adding share attributes (maybe while doing expiration, dunno). I will have a look.

Anyways, this is just UX. On backend we are safe with privilege escalation, e.g. due to #36265 and secure view logic in richdocuments.

@mrow4a
Copy link
Contributor

mrow4a commented Jan 14, 2020

@pmaier1 @micbar I will have a look today/tomorrow, maybe it is something easy

@mrow4a mrow4a self-assigned this Jan 14, 2020
@mrow4a
Copy link
Contributor

mrow4a commented Jan 14, 2020

@davitol @pmaier1 @jvillafanez this commit 4319548 broke advanced share attributes code. The "js code" we promised to onlyoffice and collabora guys we wont brake (we had build intensive acceptance tests). But that commit managed to circumvent it, and thus this issue.

Side note 1: we depreciated the code that exactly protected against this kind of issues, but it was not flexible enough for all "secure view" usecases and we went for "please do not touch me js code" approach.

Side note 2: @davitol @PVince81 I would also check if guest app has not been broken, it relied on similar mechanism

@mrow4a
Copy link
Contributor

mrow4a commented Jan 15, 2020

fix #36766

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants