U4-10268 - 7.7 Beta - Changing password should be safe by default

Created by Jeffrey Schoemaker 08 Aug 2017, 08:49:24 Updated by Sebastiaan Janssen 08 Sep 2017, 22:15:28

Subtask of: UAASSCRUM-1073

In my opinion changing the password should be as follows:

  • Click on your avatar, click change password. Always specify your old password and then put in the new password (twice). In that way there's no option that if your still logged in on Umbraco but away from your computer anybody can change your password. Currently by default you only have to specify the new password twice.

  • User-section, change password. You would always only add in the new password twice. I don't see why there should be a button "Reset password". In my opinion there's no use for that.

I know in previous versions of Umbraco we had all kinds of settings possible on the UsersMemberProvider like "allowManuallyChangingPassword", "enablePasswordReset", "enablePasswordRetrieval", but I think this wouldn't have to be an configuration setting for the user section. It should be just like this :)

3 Attachments

Comments

Shannon Deminick 21 Aug 2017, 06:34:00

Most of those settings are still relevant and it all comes down to backward compatibility since we still have active Membership providers which ASP.NET Identity exists alongside and extends. Both auth mechanisms share the exact same logic ... because backwards compatibility.

FYI, you do have to specify your old password to change your own, see screenshot If that is not the case for you, then you have allowManuallyChangingPassword set to true. This is another compat setting because many admins want the ability to manually change a user's password. By default in both ASP.NET Identity and Membership providers, you cannot just do that so this is a special switch to allow that.

If you don't want to have the password reset thing, then set enablePasswordReset = false


Shannon Deminick 21 Aug 2017, 06:36:22

I've had a look though and yes both AllowManuallyChangingPassword and EnablePasswordReset is true by default, so that can be changed to false, i'll have a look


Shannon Deminick 21 Aug 2017, 06:50:09

There's also this special case too: https://github.com/umbraco/Umbraco-CMS/blob/dev-v7.7/src/Umbraco.Core/Security/MembershipProviderExtensions.cs#L29


Shannon Deminick 21 Aug 2017, 07:02:56

PR: https://github.com/umbraco/Umbraco-CMS/pull/2140


Mads Rasmussen 22 Aug 2017, 08:20:23

@Shandem A question:

I have created a new user with Editor rights and extra access to the user section. When editing the user I see the rest password checkbox but in the user dialog, it is gone. The model I get from the "GetMembershipProbiderConfig" endpoint tells me that "enableReset" is true in both cases.

I found out that we hardcode this to false for the user dialog. Do you know the reason other than what the comment says? It doesn't make much sense when we have a setting.

https://github.com/umbraco/Umbraco-CMS/blob/dev-v7/src/Umbraco.Web.UI.Client/src/views/common/overlays/user/user.controller.js#L128


Jeffrey Schoemaker 24 Aug 2017, 06:57:18

Hi guys,

maybe I'm getting crazy from these settings, but IMHO you can't get it to work like it 'supposed' to be. You can only change your own password when you specify the old password.

If you have access to the User section you can always changes everyone's password (except your own) by only specifying the new password twice.

If tried all kinds of combinations but didn't get this output.


Sebastiaan Janssen 28 Aug 2017, 07:48:45

@jeffrey.schoemaker@perplex.nl To be clear, you expect:

  1. Only to be able to change your own password by specifying the old password (in both the content section flyout and the users section)
  2. If you have access to the user's section you can change everyone's password except your own (not by doing the weird checkbox-reset thing)

And this doesn't currently work for you?


Jeffrey Schoemaker 28 Aug 2017, 09:57:17

Exactly. And I can't get a combination of settings where this works


Sebastiaan Janssen 28 Aug 2017, 09:58:09

Cool, I'll look at it today!


Shannon Deminick 04 Sep 2017, 10:16:19

OK, here's the info.

AllowManuallyChangingPassword is now false by default, i thought i had changed it but I didn't. This should be false otherwise it allows for less secure API usage. This setting has always existed as a hack for legacy purposes. I've also fixed editing your own user in the user section so that you cannot reset your own password (just like in the change password dialog)

In both ASP.NET Identity and ASP.NET MembershipProvider APIs there is no API that allows you to arbitrarily change a user's password without first knowing their existing password. (hence the need for the AllowManuallyChangingPassword hack mentioned above). The only APIs that exist in these 2 frameworks to do something similar is to reset the password - which is a hack in itself since this is meant to be used for emails to reset their own passwords.

By default, the user's membership provider config will simply be:

<add name="UsersMembershipProvider" type="Umbraco.Web.Security.Providers.UsersMembershipProvider, Umbraco"  />

Which will mean:

  • AllowManuallyChangingPassword = false
  • EnablePasswordReset = true

These settings are what control the UI password changing functionality. Therefore this means that

  1. Only to be able to change your own password by specifying the old password (in both the content section flyout and the users section)
  2. If you have access to the user's section you can change everyone's password except your own (--not-- by doing the weird checkbox-reset thing)

Regarding the "weird checkbox-reset thing" - our options for this are:

  1. Update the UI to be more clear about what you are doing, as sebastiaan says:

It should be a button saying: Do you want to

  1. Reset password (press this button) or
  2. provide the old password and a new password (click here to show the three input fields to do that)
  1. OR ... we implement our own APIs on the BackOfficeUserManager that allows arbitrarily setting a password for a given user without knowing their old one. This is essentially putting the AllowManuallyChangingPassword hack back but into the BackOfficeUserManager for ASP.NET Identity. This means creating an API that isn't very secure... that said, it is still possible to achieve this by doing the password reset trick with APIs so perhaps that argument is moot?

What do you guys think?


Shannon Deminick 04 Sep 2017, 12:05:03

Righto, the changes were easier than i thought so here's what you get now which is what Seb mentioned before:

  1. Only to be able to change your own password by specifying the old password (in both the content section flyout and the users section)
  2. If you have access to the user's section you can change everyone's password except your own (not by doing the weird checkbox-reset thing)

I've tested these scenarios and all works. The changes can be found here:

  • 0e0ab811558247b7ce3bf457a2647b0378f671ef
  • a0306f9d550d69c5aab60778b4034ac3ffa362f9
  • 7926a0c6768383aa667556fa6979f64cbf184f46

Should be good now :)


Sebastiaan Janssen 06 Sep 2017, 21:26:47

Almost! :-)

  1. Now, when you enter a 1 character password, Umbraco will just generate a password instead of accepting the input that's too short. It should refuse to save.
  2. When you go to a member and try to save them without changing the password you get a js error:

angular.min.js?cdv=1:63 TypeError: Cannot read property 'oldPassword' of undefined at c.save (http://localhost:7700/umbraco/js/umbraco.controllers.js?cdv=1:20849:68) at http://localhost:7700/umbraco/lib/angular/1.1.5/angular.min.js?cdv=1:74:98 at Object.d.(anonymous function) [as action] (http://localhost:7700/umbraco/lib/angular/1.1.5/angular.min.js?cdv=1:44:33) at http://localhost:7700/umbraco/lib/angular/1.1.5/angular.min.js?cdv=1:74:98 at http://localhost:7700/umbraco/lib/angular/1.1.5/angular-mobile.js?cdv=1:270:9 at Object.$eval (http://localhost:7700/umbraco/lib/angular/1.1.5/angular.min.js?cdv=1:92:272) at Object.$apply (http://localhost:7700/umbraco/lib/angular/1.1.5/angular.min.js?cdv=1:92:379) at HTMLButtonElement. (http://localhost:7700/umbraco/lib/angular/1.1.5/angular-mobile.js?cdv=1:269:13) at HTMLButtonElement.dispatch (http://localhost:7700/umbraco/lib/jquery/jquery.min.js?cdv=1:3:7537) at HTMLButtonElement.r.handle (http://localhost:7700/umbraco/lib/jquery/jquery.min.js?cdv=1:3:5620)

I think #1 is okay for now, we can update in 7.7.1, it generates a new password and shows it. If it's easy to fix then it would be great to fix of course!


Sebastiaan Janssen 06 Sep 2017, 21:27:40

Should we still merge https://github.com/umbraco/Umbraco-CMS/pull/2140 ?

I couldn't get a breakpoint to trigger in that code, so not sure what it does. If you want to merge then go ahead, seems harmless.


Shannon Deminick 08 Sep 2017, 03:45:39

Righto... i think this should be good now :)

Thanks for all the testing as this revealed a few other odd quirks with password changing (that have existed for some time too)

I've pushed all code changes and merged that branch in. There was some issues with (for example), resetting a members password and then pressing save again without changing anything would cause an error. Also when you changed your own password, the password values weren't cleared from the text boxes. The 1 character thing was actually caused by a different bug/quirk with the reset password switch in js so that's fixed too.


Sebastiaan Janssen 08 Sep 2017, 14:34:54

Whoops! Commented on the wrong issue!


Sebastiaan Janssen 08 Sep 2017, 22:15:28

Works great now!


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions: 7.7.0

Due in version: 7.7.0

Sprint: Sprint 67

Story Points: 1

Cycle: