allow admin to enforce password on mail shares #4303

schiessle · 2017-04-11T13:11:12Z

Allow the admin to enforce passwords on share by mails.

That's my conclusion out if this discussion for now: #2357 (comment)

Possible scenarios and the corresponding workflow

Scenario 1:

Admin checks the "enforce password" in the mail share settings, everything else stays the default:

The user creates a share
We create a random password on the server and set it for the share
We send out the share
We send out a separate mail with the random password to the recipient

Users can change the password at any time but they can't disable it. Changing the password will trigger a new mail to the recipient with the new password.

Scenario 2

Admin checks "enforce password" in the mail share settings and disables the password mail:

The user creates a share
We create a random password on the server and set it for the share
We send out the share
We send a mail to the user who just created the share. Tell him that according to the security policy on his server all shares needs to be password protected and tell him the auto-generated password. Now it is the job of the user to get the password to the recipient.

Users can change the password at any time but they can't disable it, no additional mail will be send.

Scenario 3

Admin checks "enforce password" in the mail share settings, disables the password mail and the user who creates the share has no email address (really unlikely combination but it can still happen):

The user creates a share
operation fails, and the user gets a warning that according to the security policy on the server he needs a valid email address to create a mail share. He should set one in the personal settings and try again.

I like the workflow, especially in scenario 1 which is probably ~70% or all use cases, maybe 29% will have a setup as in scenario 2 which I think works still quite well and probably around 1% will be confronted with scenario 3. In this rare case you could even argue that it is good to remember people to set a valid email address because they will also need it in case they lose their password or for other tasks.

Reviews? Opinions?

Note: that I don't have a lot of extra time I can put into this feature. Complete re-designs are out-of-scope for now. Either we take this one with small adjustments or nothing for Nc12 (at least nothing I implement 😉 )

cc @rullzer @MorrisJobke @jancborchardt

mention-bot · 2017-04-11T13:11:16Z

@schiessle, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rullzer, @blizzz and @ChristophWurst to be potential reviewers.

ChristophWurst · 2017-04-11T13:42:02Z

apps/sharebymail/appinfo/app.php

@@ -19,6 +19,7 @@
 *
 */

-$settings = new \OCA\ShareByMail\Settings();
+$settings = new \OCA\ShareByMail\Settings(new \OCA\ShareByMail\Settings\SettingsManager(\OC::$server->getConfig()));


I would prefer to let the DI container build the object if possible and use it like OC::$server->query(SettingsManager::class).

ChristophWurst · 2017-04-11T15:24:58Z

conflicting

schiessle · 2017-04-11T15:37:34Z

I just did a rebase and resolved the merge conflicts but couldn't test it afterwards. Hope that nothing breaks... Will test tomorrow.

...Tested everything still works 😃

codecov-io · 2017-04-11T21:29:00Z

Codecov Report

❗ No coverage uploaded for pull request base (master@140580f). Click here to learn what that means.
The diff coverage is 31.25%.

@@            Coverage Diff            @@
##             master    #4303   +/-   ##
=========================================
  Coverage          ?   54.15%           
  Complexity        ?    21629           
=========================================
  Files             ?     1327           
  Lines             ?    82710           
  Branches          ?     1305           
=========================================
  Hits              ?    44790           
  Misses            ?    37920           
  Partials          ?        0

Impacted Files	Coverage Δ	Complexity Δ
core/js/sharedialogshareelistview.js	`43.92% <ø> (ø)`	`0 <0> (?)`
core/js/shareconfigmodel.js	`69.56% <ø> (ø)`	`0 <0> (?)`
apps/sharebymail/lib/Settings/SettingsManager.php	`25% <0%> (ø)`	`3 <1> (?)`
apps/sharebymail/lib/AppInfo/Application.php	`0% <0%> (ø)`	`1 <0> (?)`
apps/sharebymail/lib/Activity.php	`0% <0%> (ø)`	`31 <0> (?)`
apps/sharebymail/lib/Settings/Admin.php	`0% <0%> (ø)`	`4 <0> (?)`
apps/sharebymail/templates/settings-admin.php	`0% <0%> (ø)`	`0 <0> (?)`
lib/private/Share20/ProviderFactory.php	`84.61% <100%> (ø)`	`29 <0> (?)`
apps/sharebymail/lib/Settings.php	`100% <100%> (ø)`	`3 <2> (?)`
apps/sharebymail/tests/ShareByMailProviderTest.php	`93.08% <100%> (ø)`	`27 <0> (?)`
... and 2 more

MorrisJobke · 2017-04-13T06:09:48Z

@schiessle Could you please fix the emails, thanks :)

schiessle · 2017-04-13T10:30:10Z

changed to new mail template... please review. Thanks!

supremesyntax · 2017-04-18T08:18:38Z

I have checked out the branch and tested it a minute ago. Looks good to me great work!
Is it possible to create a activity for the password mail? (Just a nice to have)

Though i can't see anything in admin>Security section - just a blank page. Does anyone else have this problem for this branch?

EDIT: seems to be related to master...

MorrisJobke · 2017-04-18T20:49:51Z

Though i can't see anything in admin>Security section - just a blank page. Does anyone else have this problem for this branch?

You need the bruteforcesettings app cloned in you apps folder. It is shipped in the release tar ball and automatically enabled. ;)

supremesyntax · 2017-04-19T07:26:52Z

@MorrisJobke Thanks for the hint :D I can't find the password policy settings, thought they might have been shifted from additional settings to security but i was wrong... do i need some other apps enabled? Have these settings been removed for NC12?
Right now the auto-generated password is 8 chars upper-/lower-case but i have advanced requirements.

schiessle · 2017-04-19T08:31:00Z

You need the bruteforcesettings app cloned in you apps folder. It is shipped in the release tar ball and automatically enabled. ;)

But of course we shouldn't show a empty security section, even if the app is shipped and enabled by default. Admins can disable it at any time. @supremesyntax can you create a separate issue for it? Thank.

schiessle · 2017-04-19T14:05:49Z

@rullzer @MorrisJobke @nickvergessen @ChristophWurst can we please do a final review and get this in? I don't want to rebase, adjust the code and test it over and over again (which will happen again once #4384 is merged)

Thanks a lot! 😃

MorrisJobke · 2017-04-19T22:54:48Z

@rullzer @MorrisJobke @nickvergessen @ChristophWurst can we please do a final review and get this in? I don't want to rebase, adjust the code and test it over and over again (which will happen again once #4384 is merged)

Don't worry - I fixed it for you 😉 Let me test it now.

MorrisJobke

Rebased, fixed a typo, and tested 👍 Works

supremesyntax · 2017-04-20T06:54:10Z

Could somebody please answer my question? Maybe y'all overlooked it 😄
I want to test if the auto-generated password respects the password policy...

I can't find the password policy settings, thought they might have been shifted from additional settings to security but i was wrong... do i need some other apps enabled? Have these settings been removed for NC12?

rullzer · 2017-04-20T14:02:24Z

@supremesyntax if you have the password policy app enabled they should show up (in 12 they will be moved to the security settings btw).

rullzer

Ok lets do this!

schiessle · 2017-04-20T14:20:53Z

@supremesyntax it will, but only with the password policy app we will release together with Nextcloud 12, the current version in Nextcloud 11 doesn't expose the policy to other apps.

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>

…word before the admin started to enforce the password Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>

Signed-off-by: Morris Jobke <hey@morrisjobke.de>

schiessle added 3. to review Waiting for reviews feature: sharing labels Apr 11, 2017

schiessle mentioned this pull request Apr 11, 2017

Sharing a file via email and password protect the link #2357

Closed

schiessle requested review from rullzer, nickvergessen, ChristophWurst and blizzz April 11, 2017 15:11

ChristophWurst reviewed Apr 11, 2017

View reviewed changes

schiessle force-pushed the enforce-password-mailshare branch 2 times, most recently from b11610b to 0de0d15 Compare April 11, 2017 15:34

schiessle force-pushed the enforce-password-mailshare branch 3 times, most recently from 3428027 to bd90ac4 Compare April 11, 2017 20:54

schiessle force-pushed the enforce-password-mailshare branch from bd90ac4 to 21e8db4 Compare April 12, 2017 13:02

MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Apr 13, 2017

schiessle force-pushed the enforce-password-mailshare branch from 21e8db4 to 3730fb7 Compare April 13, 2017 10:28

schiessle added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 13, 2017

supremesyntax mentioned this pull request Apr 19, 2017

Empty 'Security' site when 'bruteforcesettings' app missing or disabled on NC12 #4393

Closed

schiessle force-pushed the enforce-password-mailshare branch 2 times, most recently from 5eb0cda to 0ac94b1 Compare April 19, 2017 15:09

schiessle added this to the Nextcloud 12.0 milestone Apr 19, 2017

MorrisJobke force-pushed the enforce-password-mailshare branch from 0ac94b1 to 48d56cb Compare April 19, 2017 22:54

MorrisJobke approved these changes Apr 19, 2017

View reviewed changes

rullzer approved these changes Apr 20, 2017

View reviewed changes

schiessle and others added 9 commits April 20, 2017 16:33

allow admin to enforce password on mail shares

d8dcd72

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>

templates no longer needed

7a8f8cb

allow to set a password for shares which where created without a pass…

f0651cb

…word before the admin started to enforce the password Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>

create activity if a password was send by mail

f00c1ec

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>

improve mail text

428d7cd

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>

respect password policy for auto generated passwords

972b4c0

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>

fix unit tests

aa54d31

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>

Fix casing

9d07ec5

Signed-off-by: Morris Jobke <hey@morrisjobke.de>

fix typo

6c294c3

Signed-off-by: Morris Jobke <hey@morrisjobke.de>

schiessle force-pushed the enforce-password-mailshare branch 2 times, most recently from 613c85b to 6c294c3 Compare April 20, 2017 14:42

schiessle merged commit a4086a6 into master Apr 20, 2017

schiessle deleted the enforce-password-mailshare branch April 20, 2017 14:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow admin to enforce password on mail shares #4303

allow admin to enforce password on mail shares #4303

schiessle commented Apr 11, 2017 •

edited

Loading

mention-bot commented Apr 11, 2017

ChristophWurst Apr 11, 2017

schiessle Apr 11, 2017

ChristophWurst commented Apr 11, 2017

schiessle commented Apr 11, 2017 •

edited

Loading

codecov-io commented Apr 11, 2017 •

edited by codecov bot

Loading

MorrisJobke commented Apr 13, 2017

schiessle commented Apr 13, 2017

supremesyntax commented Apr 18, 2017 •

edited

Loading

MorrisJobke commented Apr 18, 2017

supremesyntax commented Apr 19, 2017 •

edited

Loading

schiessle commented Apr 19, 2017

schiessle commented Apr 19, 2017 •

edited

Loading

MorrisJobke commented Apr 19, 2017

MorrisJobke left a comment

supremesyntax commented Apr 20, 2017

rullzer commented Apr 20, 2017

rullzer left a comment

schiessle commented Apr 20, 2017

allow admin to enforce password on mail shares #4303

allow admin to enforce password on mail shares #4303

Conversation

schiessle commented Apr 11, 2017 • edited Loading

Possible scenarios and the corresponding workflow

Scenario 1:

Scenario 2

Scenario 3

mention-bot commented Apr 11, 2017

ChristophWurst Apr 11, 2017

Choose a reason for hiding this comment

schiessle Apr 11, 2017

Choose a reason for hiding this comment

ChristophWurst commented Apr 11, 2017

schiessle commented Apr 11, 2017 • edited Loading

codecov-io commented Apr 11, 2017 • edited by codecov bot Loading

Codecov Report

MorrisJobke commented Apr 13, 2017

schiessle commented Apr 13, 2017

supremesyntax commented Apr 18, 2017 • edited Loading

MorrisJobke commented Apr 18, 2017

supremesyntax commented Apr 19, 2017 • edited Loading

schiessle commented Apr 19, 2017

schiessle commented Apr 19, 2017 • edited Loading

MorrisJobke commented Apr 19, 2017

MorrisJobke left a comment

Choose a reason for hiding this comment

supremesyntax commented Apr 20, 2017

rullzer commented Apr 20, 2017

rullzer left a comment

Choose a reason for hiding this comment

schiessle commented Apr 20, 2017

schiessle commented Apr 11, 2017 •

edited

Loading

schiessle commented Apr 11, 2017 •

edited

Loading

codecov-io commented Apr 11, 2017 •

edited by codecov bot

Loading

supremesyntax commented Apr 18, 2017 •

edited

Loading

supremesyntax commented Apr 19, 2017 •

edited

Loading

schiessle commented Apr 19, 2017 •

edited

Loading