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

Sharing a file does set while to rw for short time #11707

Closed
LukasReschke opened this issue Oct 22, 2014 · 76 comments
Closed

Sharing a file does set while to rw for short time #11707

LukasReschke opened this issue Oct 22, 2014 · 76 comments

Comments

@LukasReschke
Copy link
Member

LukasReschke commented Oct 22, 2014

When sharing a file via the sharing button to other users the file is automatically shared with "can edit" permissions. - While permissions can be changed then in the next window within a short timeframe the receiving user has permissions to edit the shared file which is an unexpected behaviour.

We should make sure that always the least permissive permissions are set per default and/or make this editable within the same step.

@jancborchardt Your opinion?

\cc @guruz @ogoffart

@LukasReschke LukasReschke added this to the 2014-sprint-06-current milestone Oct 22, 2014
@LukasReschke
Copy link
Member Author

What currently happens is:

  1. User enters username
  2. User clicks enter
  3. File is shared
  4. Permissions can be edited

We have to make sure that step 4 either happens before step 3 or set non destructive permissions per default.

@jancborchardt
Copy link
Member

Then the file should maybe only be shared once the popup is closed?

@LukasReschke
Copy link
Member Author

That would be another approach that I'd welcome.

@PVince81 Is that easily possible?

@guruz
Copy link
Contributor

guruz commented Oct 23, 2014

But that's a behaviour change and it might not be obvious that people have to close it ("i clicked share but my friend didnt get the mail"). I vote for having non destructive permissions as default.

@jancborchardt
Copy link
Member

I agree with that, but having read-only as default is an even bigger change in behavior. Sharing with someone on your same instance should by default be read/write of course.

@PVince81
Copy link
Contributor

The other alternative would be to only add the share after the dropdown is closed (or the window is closed). Basically: defer changes made in the dropdown until it's closed.

@jancborchardt
Copy link
Member

@PVince81 that’s what I said above. ;)

@PVince81
Copy link
Contributor

Ah right, missed that.

@craigpg craigpg modified the milestones: 2014-sprint-07-current, 2014-sprint-06 Oct 27, 2014
@schiessle
Copy link
Contributor

Not sure If I want to wait until the drop down was closed before the shares are stored for various reasons:

  • If the user created multiple shares and one of the share operation fails we would need to inform the user somehow, pop-up -> ugly. Then the user needs to open the dialog again, try to share the file again,...
  • We would probably need quite some extra code. Not only for the sharing and failure handling but also for additional options, like the option to inform recipients about the share by mail. Not sure if this efforts and the additional complexity is worth the possible benefits.

If we agree that we need to do something I would vote for minimal permissions by default.

But honestly I'm not sure if we really need to do something here. We are talking about seconds, right? How likely is it that at the time I shared a file with a user, the user is logged in, refresh his browser directly after I clicked the share button, realize the new share within the 2-3 seconds time frame and clicks on edit, edits the file and save it again? Is this really a problem?

@LukasReschke
Copy link
Member Author

If we agree that we need to do something I would vote for minimal permissions by default. But honestly I'm not sure if we really need to do something here. We are talking about seconds, right? How likely is it that at the time I shared a file with a user, the user is logged in. He refresh his browser directly after I clicked the share button, realize the new share within the 2-3 seconds time frame and clicks on edit, edits the file and save it again? Is this really a problem?

Well, this is something that you can easily automate and therefore: yes it is. There is a reason that no other platform implements this in the way we do :-)

@schiessle
Copy link
Contributor

Well, this is something that you can easily automate and therefore: yes it is. There is a reason that no other platform implements this in the way we do :-)

Well, but then it is clearly a destructive behaviour and nothing which happens by accident. Also we are talking about internal shares with a known user with whom I actually want to share stuff, not about a random person getting access and go crazy. Let's say someone in your company does this, running a script to destroy all documents shared with him. In this case you would handle this person on a personal level anyway. The same way he could run in your office and run all your paper documents through a document shredder. 😉

@LukasReschke
Copy link
Member Author

Well, there are actually use-cases where you want to prevent such destructive behaviour ;-)

Easiest example: Teacher wants to share documents with the scores with the students, in a short moment they are able to modify them.

We either have to:

  1. Ask for the proper permission in the sharing dropbox or make a popup out of it (as does Google or Dropbox do)
  2. Limit the privileges

@schiessle
Copy link
Contributor

Then I would go with 2. Everything else is imho to much extra complexity with respect to the problem we want to solve.

@LukasReschke
Copy link
Member Author

I agree.

@craigpg craigpg modified the milestones: 2014-sprint-08-current, 2014-sprint-07 Nov 10, 2014
@MorrisJobke
Copy link
Contributor

@LukasReschke @jancborchardt @schiesbn @PVince81 So the default permission should be read-only?

@karlitschek Is this still topic of 8.0? Otherwise move this to the 8.1 milestone and I will have a look later on this.

@MorrisJobke MorrisJobke self-assigned this Jan 5, 2015
@LukasReschke
Copy link
Member Author

Yes.

I'd vouch for 8.1. Nothing did explode in the past due to that and as we're already in the beta let's keep it that way.

@jancborchardt
Copy link
Member

I’d vouch for 8.1 if anything as well.

So the default permission should be read-only?

I’m really not too fond about that. We didn’t have real problems in practice before because of this. And I think we would get more complaints about people not noticing sharing is read-only by default and others can’t edit it.

@LukasReschke
Copy link
Member Author

Well - fact is that we do it differently than everybody else and this really a problem in serious environments. You simply don't want to have it that way.

So we either set it to read-only or we come up with a proper sharing popup dialog (as does Dropbox or Google Drive) but the current solution is a mess at best.

@LukasReschke
Copy link
Member Author

The current approach can be kinda compared to creating a user and give it admin rights per default and then have a small timeframe where the user has administrative privileges.

A best practise is to obey the least-privilege principle: Give the user only what he needs at start and if he needs more give him more permissions.

Honestly, if you want to have a file editable we have the checkbox right there in the sharing dropdown and doing it this way would not result in any potentially unintentional behaviour.

Then again we also have public shared files only readable by default. For the regular user it is in no way understandable why sharing a file with other users should make it editable but sharing a file publicly will make it read-only.

@karlitschek
Copy link
Contributor

Hmm. I see pros and cons for both options. I would leave it as it is for now because we are in feature freeze and no one complained about the current behavior -> 8.1

@AnrDaemon
Copy link

Well what are "sane" defaults?

Sane defaults are the defaults that administrator set for this object type.

Administrator. Not developer.

What users may or may not forget, is not yours to call. You can't hold everyone's hand in the world. There's roughly 3 billion internet users in the world, and you only have two hands.

If you are going to intentionally create administration headache, you are limiting the potential of your product, and encouraging its users to look for alternatives.

@MTRichards
Copy link
Contributor

Most important is to cover the most likely scenario with the defaults.

Resharing should be off by default. That should be an act of commission - you want someone to get a copy of a copy, therefore you turn on resharing. Otherwise, it should be off.

Editing should be on, that is why you share with someone more often than not. Delete should be off inside edit, so we don't have people losing data by accident.

That covers the most likely path for most users, and then the others can be configured at the time of share.

@AnrDaemon
Copy link

@MTRichards , I think we're leaning to discuss apples and oranges.
Nobody question the necessity of the installation defaults.
What I'm saying is that the installation defaults shouldn't have a final say in each instance of the application.
The administrator of an instance must be able to set defaults of an object to suit the workflow of their organization, which will save their users time and effort and require less administrative intervention over the lifetime of an application. See my example above.

@MTRichards
Copy link
Contributor

Sure, I understand - I needed to clarify the defaults, which I did.

As for the issue of defaults, and being able to change them in a config switch or something, this is tricky. Every time we add a config file option, we increase the test matrix, complicate the code, and make it harder to maintain. We also increase confusion, the number of support cases, and make it harder for an admin to easily get what they want out of ownCloud. Eventually we will get bogged down and unusable. We have a TON of these sorts of cases out there, just look at the sample config file - it is daunting and awesome at the same time.

So the question becomes balance - to balance the needs of most against the needs of few and come up with the right answer. In this situation, if a read only folder is needed more than a read/write folder - I am just not yet convinced that there are enough use cases or need to warrant a switch. Are there use cases? You bet. Are there enough to warrant the addition of these switches? Not convinced.

@karlitschek
Copy link
Contributor

@AnrDaemon There might be a way for you to override this default with a custom theme or app. But unfortunately we have to limit the number of configuration options as @MTRichards said. In a lot of cases we have to look at what 95% or all users want and what 95% or all other sync and share solutions out there do. And this is RW. We really understand the desire and need for an non standard behavior.
But this can and should be done by a patch, or theme or app. ownCloud is open source after all which means it is possible. But this doesn't man that we can do config options for every non standard use-case because this would kill our software in the long run.
Thanks a lot for your understanding.

@rullzer
Copy link
Contributor

rullzer commented Feb 10, 2016

So it seems we are still devided what the default behaviour has to be. As I think we have seen all possible combinations of. READ/WRITE/SHARE come along here. And for each combination there are very valid use cases.

If we would eventually move to 'simpler' permissions (#12970) so only can edit and can share. Maybe the approach from @LukasReschke in #11707 (comment) is best.

So next to the input field for the sharee you have a dropdown menu where you can select the behaviour your want.

@karlitschek
Copy link
Contributor

I actually think that the current behavior is fine because it is the most common behavior and it also the same that all our competitors do. Which means that this is also what users expect.
Let's close this for now.

@LukasReschke
Copy link
Member Author

I actually think that the current behavior is fine because it is the most common behavior and it also the same that all our competitors do.

It's not really, see #11707 (comment)

@LukasReschke LukasReschke reopened this Feb 17, 2016
@LukasReschke
Copy link
Member Author

All others: Set to "edit" per default BUT allow changing this BEFORE sharing. Thus not setting potential insecure defaults.
We: Set to "edit" per default and do not allow changing this until the files have already been shared.

The screenshots in #11707 (comment) show this pretty well. It is possible to set the permissions before anything is shared, and this is the default sharing screen.

Let's set this to backlog…

@LukasReschke LukasReschke modified the milestones: 9.1-next, 9.0-current, backlog Feb 17, 2016
@butonic
Copy link
Member

butonic commented Feb 17, 2016

I see two possible solutions:

  1. Add a confirmation button. Clicking it creates the share. Might need UI rework to make it look appealing and less confusing.
  2. Change defaults to readonly. Adding a share then does not need a confirmation action. Creating a writable share needs an extra click on the corresponding checkbox.

Hope this clarifies that it is not the defaults that are wrong, but the moment we create the share.

@ogoffart
Copy link

The security issue is this one: imagine a funny student who configured his client to automatically delete all new share. The proffessor want to share a folder containing the assignement data and instruction with all the students (the students group) When he does so, he better be quick to remove the write permission before it's too late and all the data is gone.

@MorrisJobke
Copy link
Contributor

The security issue is this one: imagine a funny student who configured his client to automatically delete all new share. The proffessor want to share a folder containing the assignement data and instruction with all the students (the students group) When he does so, he better be quick to remove the write permission before it's too late and all the data is gone.

🙈 That's a weird scenario.

@AnrDaemon
Copy link

Weird, may be.
But I've seen people doing that "because it is too much spam".

@jancborchardt
Copy link
Member

configured his client to automatically delete all new share.

Well – this should simply result in that person unsharing the share from themselves, right? Not in deleting all the files.

@guruz
Copy link
Contributor

guruz commented Feb 17, 2016

Well – this should simply result in that person unsharing the share from themselves, right? Not in deleting all the files.

Not if the student is fast enough with deleting before the professor sets R-only.

I still think it is a security issue.

@LukasReschke
Copy link
Member Author

I still think it is a security issue.

👍

@PVince81
Copy link
Contributor

It is now possible to set default sharing permissions since OC 10.0.3, closing.

@lock
Copy link

lock bot commented Aug 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests