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

Split "Enforce password protection" option for file drop and public links #29526

Closed
pmaier1 opened this issue Nov 9, 2017 · 22 comments
Closed
Assignees
Labels
enhancement p1-urgent Critical issue, need to consider hotfix with just that issue settings:admin
Milestone

Comments

@pmaier1
Copy link
Contributor

pmaier1 commented Nov 9, 2017

To fulfill the requirement of enforcing password protection for public links while not enforcing them for file drop links I suggest to split the "Enforce password protection" option into two:
a) Enforce password protection for regular public links
b) Enforce password protection for File Drop public links

Wording might not yet be perfect.

Another thing we need to take into account is the UI behavior when only one of the options is true:

Example:

  • User creates a file drop public link without password and changes permissions to a regular public link where password protection is enforced. At that point saving needs to be blocked unless a (policy-matching) password is entered. This of course needs to work the other way round as well
  • What if I have a password protection enforced public link and change permissions to a not password protection enforced file drop public link? Password will be kept but can be removed by the user!?

I hope there are no implications I might have missed.

@PVince81

@pmaier1 pmaier1 added enhancement p2-high Escalation, on top of current planning, release blocker settings:admin labels Nov 9, 2017
@pmaier1 pmaier1 added this to the planned milestone Nov 9, 2017
@PVince81 PVince81 modified the milestones: planned, development Nov 22, 2017
@hodyroff hodyroff added PM prio 1 and removed p2-high Escalation, on top of current planning, release blocker labels Dec 11, 2017
@pmaier1 pmaier1 added p1-urgent Critical issue, need to consider hotfix with just that issue and removed PM prio 1 labels Dec 11, 2017
@pmaier1
Copy link
Contributor Author

pmaier1 commented Dec 13, 2017

Actually I don't really see a need for enforced password-protecting of File Drop links so we could also just exclude them (and add a hint in settings that it doesn't apply there!) in order not to clutter config options even more. I would make it dependent on effort.

@PVince81
Copy link
Contributor

@pmaier1 some people might feel safer if there is a password to File Drop links in case the link gets leaked and people would spam the folder. This, assuming that the password was shared using different channels. Probably a rare case...

@felixboehm
Copy link
Contributor

@jvillafanez please contact @felixheidecke if any frontend help needed.

@PVince81 PVince81 modified the milestones: development, planned Jan 12, 2018
@PVince81
Copy link
Contributor

I think currently we trigger an event when password policy check needs to apply. We might want to add a "scope" or "context" attribute there to let password policy apps find out in what context the password will be used.

@jvillafanez
Copy link
Member

I'll need some clarifications...

screen shot 2018-01-12 at 15 34 44

That sharing part, is it controlled by the files_sharing app or by core? If it's controlled by the files_sharing app, we should clarify that the password protection there is just for the files_sharing app and not for any other.

I expect the files_drop app has a similar checkbox in its own configuration panel.

My point is that core doesn't enforce anything, it's each app which does it, and each app has different ways to enforce password protection. If this is true, we can just adjust the text there for the files_sharing and duplicate the functionality (more or less) for files_drop.

@PVince81
Copy link
Contributor

@jvillafanez files_drop app is dead. When we talk about it we mostly mean its replacement: go to the link share creation dialog and choose "upload only" mode. In upload only mode, one should be able to disable password enforcement as often times passwords for upload only don't make much sense as there is no data you can access.

@PVince81
Copy link
Contributor

so all this is either in core or files_sharing, depending where the current code actually lives

@jvillafanez
Copy link
Member

screen shot 2018-01-16 at 11 54 57

Shouldn't we add a switch per permissions mode (read-only, read & write, write only)?

The other option would be that the password is enforced and we need to add a option to disable that just for upload-only shares, which is aligned with #29526 (comment)

screen shot 2018-01-16 at 12 42 28

@pmaier1
Copy link
Contributor Author

pmaier1 commented Jan 16, 2018

Shouldn't we add a switch per permissions mode (read-only, read & write, write only)?

Not necessary, I think.

Should be perfectly fine if we display another checkbox below (default: true) when "Enforce password protection" is enabled: "Exclude Upload only (File Drop) links"

@felixheidecke
Copy link
Contributor

To complete the whole thing why don't we do this instead:

  • Enforce password for read-only
  • Enforce password for read & write
  • Enforce password for File drop

@pmaier1
Copy link
Contributor Author

pmaier1 commented Jan 20, 2018

To complete the whole thing why don't we do this instead:

@jvillafanez suggested the same above. It's a trade-off between configurability vs. cluttering settings / losing overview. I don't think this granularity is necessary but would leave it up to you to decide on proper design/UX. The basic request is only this: Enforce password protection for public links but not for File Drop.

@jvillafanez
Copy link
Member

We'll likely need to change the code regardless of the decision, so whatever @felixheidecke decides. On the other hand, the current solution shouldn't need migration, which might be needed if we use 3 different options.

@pmaier1
Copy link
Contributor Author

pmaier1 commented Jan 22, 2018

Thanks for explanation @jvillafanez. Let's work in iterative MVP-style and keep effort low.

@felixheidecke
Copy link
Contributor

Maybe this granularity isn't needed. So we can just do regular and drop links. Fine with me :-)

@cdamken
Copy link
Contributor

cdamken commented Jan 29, 2018

@felixboehm @patrickjahns We need this in 10.0.6 This is a Blocker.

@PVince81
Copy link
Contributor

10.0.6 is a hotfix release for 10.0.5. This is scheduled for 10.0.7, so blocker for 10.0.7... it's already p1

@jvillafanez
Copy link
Member

PR in #30168 in case someone missed it

@pmaier1
Copy link
Contributor Author

pmaier1 commented Feb 12, 2018

Nice job! I'll close here then.

@pmaier1 pmaier1 closed this as completed Feb 12, 2018
@PVince81
Copy link
Contributor

@jvillafanez please adjust to only show "regular" and "drop" checkboxes

cc @pmaier1 please correct if misunderstood

@PVince81 PVince81 reopened this Mar 13, 2018
@pmaier1
Copy link
Contributor Author

pmaier1 commented Mar 13, 2018

split_enforce_pw_prot

Hmm, I thought about it again and think it's not bad to have three options though I don't really see a use case in differentiating between "read only" and "read & write" (well, maybe someone really wants to distinguish between write permission and read only, ok ^^). But we should stay consistent in wording. In public link share dialog the last one is called "Upload only (File Drop)".

I suggest the following:

Enforce password protection for Read only links
Enforce password protection for Read & Write links
Enforce password protection for Upload only (File Drop) links

This was referenced Mar 14, 2018
@jvillafanez
Copy link
Member

Wording change in #30773
Backport to stable10 in #30774

@lock
Copy link

lock bot commented Jul 30, 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 Jul 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement p1-urgent Critical issue, need to consider hotfix with just that issue settings:admin
Projects
None yet
Development

No branches or pull requests

7 participants