Password confirmation for some actions #1447

nickvergessen · 2016-09-19T14:35:33Z

when performing certain actions, we want to make sure, that the user is actually the user.
Therefor we require a password confirmation, e.g. when changing the email address.

ToDo

@LukasReschke @MorrisJobke

MorrisJobke · 2016-09-19T15:49:35Z

@nickvergessen What state is this? 2 or 3?

nickvergessen · 2016-09-19T15:58:01Z

Both, because @LukasReschke should already have a look at it, while I develop the rest 😛

nickvergessen · 2016-10-25T13:48:40Z

@MorrisJobke can you have a look?

Currently when you open the user management, then the settings section and toggle "show storage location" that doesn't work.

For easier testing set down the confirmation time to 10 seconds instead of 30 minutes:

diff --git a/core/js/js.js b/core/js/js.js
index e423b6e..89426cc 100644
--- a/core/js/js.js
+++ b/core/js/js.js
@@ -1536,6 +1536,7 @@ OC.PasswordConfirmation = {

        requiresPasswordConfirmation: function() {
                var timeSinceLogin = moment.now() - nc_lastLogin * 1000;
+               return timeSinceLogin > 10 * 1000; // 30 minutes
                return timeSinceLogin > 30 * 60 * 1000; // 30 minutes
        },

diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php
index 981232f..7e385b9 100644
--- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php
+++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php
@@ -147,7 +147,8 @@ class SecurityMiddleware extends Middleware {

                if ($this->reflector->hasAnnotation('PasswordConfirmationRequired')) {
                        $lastConfirm = (int) $this->session->get('last-password-confirm');
-                       if ($lastConfirm < (time() - 30 * 60 + 15)) { // allow 15 seconds delay
+                       if ($lastConfirm < (time() - 10)) { // allow 15 seconds delay
+                       //if ($lastConfirm < (time() - 30 * 60 + 15)) { // allow 15 seconds delay
                                throw new NotConfirmedException();
                        }
                }
diff --git a/settings/ajax/togglegroups.php b/settings/ajax/togglegroups.php
index b9958be..35ac36c 100644
--- a/settings/ajax/togglegroups.php
+++ b/settings/ajax/togglegroups.php
@@ -29,7 +29,8 @@ OC_JSON::checkSubAdminUser();
 OCP\JSON::callCheck();

 $lastConfirm = (int) \OC::$server->getSession()->get('last-password-confirm');
-if ($lastConfirm < (time() - 30 * 60 + 15)) { // allow 15 seconds delay
+if ($lastConfirm < (time() - 10)) { // allow 15 seconds delay
+//if ($lastConfirm < (time() - 30 * 60 + 15)) { // allow 15 seconds delay
        $l = \OC::$server->getL10N('core');
        OC_JSON::error(array( 'data' => array( 'message' => $l->t('Password confirmation is required'))));
        exit();
diff --git a/settings/ajax/togglesubadmins.php b/settings/ajax/togglesubadmins.php
index 5658a38..eb79ced 100644
--- a/settings/ajax/togglesubadmins.php
+++ b/settings/ajax/togglesubadmins.php
@@ -25,7 +25,8 @@ OC_JSON::checkAdminUser();
 OCP\JSON::callCheck();

 $lastConfirm = (int) \OC::$server->getSession()->get('last-password-confirm');
-if ($lastConfirm < (time() - 30 * 60 + 15)) { // allow 15 seconds delay
+if ($lastConfirm < (time() - 10)) { // allow 15 seconds delay
+//if ($lastConfirm < (time() - 30 * 60 + 15)) { // allow 15 seconds delay
        $l = \OC::$server->getL10N('core');
        OC_JSON::error(array( 'data' => array( 'message' => $l->t('Password confirmation is required'))));
        exit();

LukasReschke · 2016-11-01T19:34:54Z

core/js/config.php

 	$dataLocation = false;
 }

+$lastConfirmTimestamp = $user->getLastLogin();
+$sessionTime = \OC::$server->getSession()->get('last-password-confirm');


This is kinda often used and seems error-prone. Maybe let's add that to OCP\IUser as new method? isSudoMode() : bool or so?

Would introduce a new dependency on ISession in the User class though. Anyhow, I think we should abstract that away in some other class. Also it's own helper class is fine in the OC\Security namespace.

Well it's used 3 times and in legacy code (ajax endpoints which don't use controller).
Once the legacy is gone, only the annotation is used, so I think the 3 appearances are fine.

Fair enough

nickvergessen · 2016-11-03T09:26:21Z

@MorrisJobke I fail to get the quota and the email thing covered, can you try it?

MorrisJobke · 2016-11-03T10:30:20Z

~~BUG: as an admin I need to enter my password when changing my email address on the personal page (this is not the case for a non-admin)~~

LukasReschke · 2016-11-03T10:30:44Z

BUG: as an admin I need to enter my password when changing my email address on the personal page (this is not the case for a non-admin)

BUG: As an normal user I should also have to enter my password 😉

codecov-io · 2016-11-10T16:59:09Z

Current coverage is 57.97% (diff: 13.00%)

Merging #1447 into master will increase coverage by 0.39%

@@             master      #1447   diff @@
==========================================
  Files          1169       1170      +1   
  Lines         70226      71253   +1027   
  Methods        7178       7374    +196   
  Messages          0          0           
  Branches       1205       1237     +32   
==========================================
+ Hits          40433      41309    +876   
- Misses        29793      29944    +151   
  Partials          0          0

Powered by Codecov. Last update c241539...3ffd9a7

nickvergessen · 2016-11-11T09:00:39Z

Should be ready to review now, only thing left is that you can't submit with enter, but @ChristophWurst will take care of that soon.

rullzer · 2016-11-16T08:58:23Z

Submit on enter now works

jancborchardt · 2016-11-16T16:12:48Z

Please use the modal we use everywhere else. This doesn’t work on mobile at all, and we don’t need a second implementation to maintain & style. ;)

nickvergessen · 2016-11-17T09:37:58Z

core/css/jquery.ocdialog.css

@@ -22,7 +22,6 @@
 .oc-dialog-buttonrow {
 	display: block;
 	background: transparent;
-	position: absolute;


@jancborchardt this was required because the password field was not clickable since the button row was overlaying it. Maybe you can have a look at the design a bit.

Eeeh this will mess with other dialogs like the move dialog. 👎

Will need to check it out

MorrisJobke · 2016-11-17T15:40:02Z

@nickvergessen conflicts 🙈

Signed-off-by: Joas Schilling <coding@schilljs.com>

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

Signed-off-by: Joas Schilling <coding@schilljs.com>

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Signed-off-by: Joas Schilling <coding@schilljs.com>

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

* properly handle user management actions like display name, email or password field change Signed-off-by: Morris Jobke <hey@morrisjobke.de>

MorrisJobke · 2016-11-18T13:19:59Z

Tested and works 👍

ChristophWurst · 2016-11-19T18:22:28Z

core/js/js.js

 	},

 	requiresPasswordConfirmation: function() {
 		var timeSinceLogin = moment.now() - nc_lastLogin * 1000;
+		return timeSinceLogin > 10 * 1000; // 30 minutes


liar 🙈

FF reports "unreachable code after return statement" on master

nickvergessen added 2. developing Work in progress 3. to review Waiting for reviews security labels Sep 19, 2016

nickvergessen added this to the Nextcloud 11.0 milestone Sep 19, 2016

nickvergessen force-pushed the password-confirmation-for-some-actions branch from a8ae639 to 875af94 Compare September 19, 2016 14:37

MorrisJobke removed the 3. to review Waiting for reviews label Sep 19, 2016

nickvergessen mentioned this pull request Sep 19, 2016

Appconfig endpoint #1452

Merged

MorrisJobke mentioned this pull request Oct 24, 2016

Nextcloud 11 overview #745

Closed

47 tasks

nickvergessen force-pushed the password-confirmation-for-some-actions branch from 55b717f to 616e840 Compare October 25, 2016 11:05

LukasReschke requested changes Nov 1, 2016

View reviewed changes

nickvergessen force-pushed the password-confirmation-for-some-actions branch from 739cbe1 to 4526f3c Compare November 3, 2016 08:59

nickvergessen force-pushed the password-confirmation-for-some-actions branch from 91ba596 to 00da391 Compare November 10, 2016 16:09

nickvergessen assigned nickvergessen and ChristophWurst Nov 11, 2016

nickvergessen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 11, 2016

rullzer force-pushed the password-confirmation-for-some-actions branch from 81fb5b7 to 4118be3 Compare November 16, 2016 08:52

nickvergessen commented Nov 17, 2016

View reviewed changes

ChristophWurst removed their assignment Nov 17, 2016

nickvergessen and others added 16 commits November 18, 2016 12:10

Require password confirmation when changing workflow rules

410e0fc

Signed-off-by: Joas Schilling <coding@schilljs.com>

Require password confirmation for user management

2fd2e45

Signed-off-by: Joas Schilling <coding@schilljs.com>

Hide and show after updating

47f9574

Signed-off-by: Joas Schilling <coding@schilljs.com>

use proper method call for _.bind()

71c4edf

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

More fixes

3dc3887

Signed-off-by: Joas Schilling <coding@schilljs.com>

Update class map

922f51e

Signed-off-by: Joas Schilling <coding@schilljs.com>

Fix unit tests

b2d9c20

Signed-off-by: Joas Schilling <coding@schilljs.com>

Confirm on create/delete group

a7feb91

Signed-off-by: Joas Schilling <coding@schilljs.com>

Require confirmation to change the displayname

309b21f

Signed-off-by: Joas Schilling <coding@schilljs.com>

Empty the password field on submission of the form

05df523

Signed-off-by: Joas Schilling <coding@schilljs.com>

Require password confirmation to change the Quota

a53c313

Signed-off-by: Joas Schilling <coding@schilljs.com>

Fix error displaying on email and add confirmation

02ea134

Signed-off-by: Joas Schilling <coding@schilljs.com>

Add the 15 seconds to the window, instead of removing

bb7787a

Signed-off-by: Joas Schilling <coding@schilljs.com>

Show a little explanation above the input field

80abb69

Signed-off-by: Joas Schilling <coding@schilljs.com>

Submit on enter

e80d3c2

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Use the existing prompt

8d33d76

Signed-off-by: Joas Schilling <coding@schilljs.com>

LukasReschke force-pushed the password-confirmation-for-some-actions branch from 7bcea46 to 8d33d76 Compare November 18, 2016 11:12

LukasReschke approved these changes Nov 18, 2016

View reviewed changes

fix displayname

8ef356a

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

MorrisJobke force-pushed the password-confirmation-for-some-actions branch from e3f99b6 to 8ef356a Compare November 18, 2016 12:50

Fix user management callbacks

3ffd9a7

* properly handle user management actions like display name, email or password field change Signed-off-by: Morris Jobke <hey@morrisjobke.de>

MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Nov 18, 2016

MorrisJobke merged commit 332eaec into master Nov 18, 2016

MorrisJobke deleted the password-confirmation-for-some-actions branch November 18, 2016 14:42

ChristophWurst reviewed Nov 19, 2016

View reviewed changes

nickvergessen mentioned this pull request Nov 21, 2016

Make sure the password is okay with 30 minutes #2217

Closed

juliushaertl mentioned this pull request Nov 29, 2016

Cancel password confirmation on settings toggles checkbox state #2395

Open

ShGKme mentioned this pull request Jan 16, 2024

refactor(core): remove sudo-login-form layout #42837

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Password confirmation for some actions #1447

Password confirmation for some actions #1447

nickvergessen commented Sep 19, 2016 •

edited by rullzer

Loading

MorrisJobke commented Sep 19, 2016

nickvergessen commented Sep 19, 2016

nickvergessen commented Oct 25, 2016

LukasReschke Nov 1, 2016

LukasReschke Nov 1, 2016

nickvergessen Nov 2, 2016

LukasReschke Nov 2, 2016

nickvergessen commented Nov 3, 2016

MorrisJobke commented Nov 3, 2016 •

edited

Loading

LukasReschke commented Nov 3, 2016 •

edited by MorrisJobke

Loading

codecov-io commented Nov 10, 2016 •

edited

Loading

nickvergessen commented Nov 11, 2016

rullzer commented Nov 16, 2016

jancborchardt commented Nov 16, 2016

nickvergessen Nov 17, 2016

jancborchardt Nov 18, 2016

MorrisJobke commented Nov 17, 2016

MorrisJobke commented Nov 18, 2016

ChristophWurst Nov 19, 2016

Password confirmation for some actions #1447

Password confirmation for some actions #1447

Conversation

nickvergessen commented Sep 19, 2016 • edited by rullzer Loading

ToDo

MorrisJobke commented Sep 19, 2016

nickvergessen commented Sep 19, 2016

nickvergessen commented Oct 25, 2016

LukasReschke Nov 1, 2016

Choose a reason for hiding this comment

LukasReschke Nov 1, 2016

Choose a reason for hiding this comment

nickvergessen Nov 2, 2016

Choose a reason for hiding this comment

LukasReschke Nov 2, 2016

Choose a reason for hiding this comment

nickvergessen commented Nov 3, 2016

MorrisJobke commented Nov 3, 2016 • edited Loading

LukasReschke commented Nov 3, 2016 • edited by MorrisJobke Loading

codecov-io commented Nov 10, 2016 • edited Loading

Current coverage is 57.97% (diff: 13.00%)

nickvergessen commented Nov 11, 2016

rullzer commented Nov 16, 2016

jancborchardt commented Nov 16, 2016

nickvergessen Nov 17, 2016

Choose a reason for hiding this comment

jancborchardt Nov 18, 2016

Choose a reason for hiding this comment

MorrisJobke commented Nov 17, 2016

MorrisJobke commented Nov 18, 2016

ChristophWurst Nov 19, 2016

Choose a reason for hiding this comment

nickvergessen commented Sep 19, 2016 •

edited by rullzer

Loading

MorrisJobke commented Nov 3, 2016 •

edited

Loading

LukasReschke commented Nov 3, 2016 •

edited by MorrisJobke

Loading

codecov-io commented Nov 10, 2016 •

edited

Loading