User data providers and cookie expiration setting by MatteoPiovanelli · Pull Request #7945 · OrchardCMS/Orchard

MatteoPiovanelli · 2018-01-18T11:37:01Z

AS mentioned in #6014 I have implemented providers for the UserData string that is used in authentication cookies. Adding this, any extant cookie wil immeditely be considered invalid. Upon first sign in, the new UserData will be used.
I have also made the life span of said cookies configurable (the default is still 30 days).

I'll add comments to the PR to explain/discuss my changes.

…ntication cookies

…d last: this means that changing the password invalidates all extant authentication cookies

… rather than always using a constant. The default implementation returns the same constant.

MatteoPiovanelli · 2018-01-18T11:37:42Z

        private readonly IMembershipValidationService _membershipValidationService;
        private readonly IEnumerable<IOpenIdProvider> _openIdProviders;
+        private readonly IEnumerable<IUserDataProvider> _userDataProviders;
+        private readonly ISecurityService _securityService;


I had to make changes here because of the new services being injected in FormsAuthenticationService

MatteoPiovanelli · 2018-01-18T11:40:17Z

+using Orchard.Users.Models;
+
+namespace Orchard.Users.Services {
+    public class PasswordChangedDateUserDataProvider : BaseUserDataProvider {


With this provider the auth cookies is considered invalid on password changes.
Because of this, when we change password we are automatically logged out of the tenant (from all clients, including the one from which we changed password), and have to sign in anew. I prefer this to having the user remain logged in, but it is different than the current UX.

Note: in this PR right now (2018/01/18 15.30) I am not firing the LoggedOut event when this happens.

MatteoPiovanelli · 2018-01-18T11:42:12Z

            return cookiePath;
        }
+
+        #region Serialization of UserData Dictionary


I wrote this serialization/deserialization to avoid introducing new dependencies in Orchard.Framework.

Don't even try, otherwise there is a risk that user data conflict with the format if you are not escaping correctly.

The way it's written there should be no escaping concerns.
To safely serialize to json, I would feel safer to use JsonSerializer, or Newtonsoft.Json, or something like that. I haven't done that directly to avoid introducing new dependencies in Orchard.Framework. However, if you tell me that is fine, I would go that route.

I think there is already a dependency on it.

there should be no escaping concerns

Until there is

Ok. I will use Newtonsoft.Json

MatteoPiovanelli · 2018-01-18T11:45:50Z

+        /// This is the key that will be used in the UserData dictionary
+        /// </summary>
+        public virtual string Key {
+            get { return GetType().FullName; }


This should make the Key unique to each provider, right?

MatteoPiovanelli · 2018-01-18T11:53:46Z

Note:
I'm changing the code a bit so that extant auth cookies arent't automatically invalid

…very client, or none. Added code to prevent users that were authenticated with "old" cookies to be suddenly logged out when deploying this

sebastienros · 2018-01-18T20:17:12Z

 <p>@T("Use the form below to change your password.")</p>
 <p>@T.Plural("The password can't be empty.", "New passwords are required to be a minimum of {0} characters in length.", (int)ViewData["PasswordLength"])</p>
+@if ((bool)ViewData["InvalidateOnPasswordChange"]) {
+    <p>@T("After changing the password you will be required to login anew.")</p>


sebastienros · 2018-01-18T20:18:00Z

 @model dynamic
 <h1>@Html.TitleForPage(T("Change Password").ToString())</h1>
 <p>@T("Your password has been changed successfully.")</p>
+@if ((bool)ViewData["InvalidateOnPasswordChange"]) {


Unnecessary. Users are redirected to the login page, it's obvious they need to use the new one.

While trying the feature, I was not being redirected anywhere, so I added this paragraph. Could be the redirect is not in place anymore?

sebastienros · 2018-01-18T20:18:42Z

+        <label>@T("Life span of authentication cookies")</label>
+        @Html.TextBoxFor(m => m.AuthCookieLifeSpan, new { @class = "text medium" })
+        @Html.ValidationMessageFor(m => m.AuthCookieLifeSpan)
+        <p class="hint">@T("The time after signing in after which authentication cookies will be invalidated, in standard TimeSpan format (dd.hh:mm:ss).")</p>


Don't mention TimeSpan, just the format.

For reference https://msdn.microsoft.com/library/1d3t3c61(v=vs.100).aspx

I went to the URL, but I am not sure what I am looking for in relation with the TimeSpan

sebastienros · 2018-01-18T20:21:52Z

+        @Html.CheckBoxFor(m => m.ShouldInvalidateAuthOnPasswordChanged)
+        <label class="forcheckbox" for="@Html.FieldIdFor(m => m.ShouldInvalidateAuthOnPasswordChanged)">@T("Invalidate authentication cookies on password change")</label>
+        @Html.ValidationMessageFor(m => m.ShouldInvalidateAuthOnPasswordChanged)
+        <p class="hint">@T("Select this flag if you want that changing a user's password will require them to sign in anew from all clients.")</p>


When checked, the user will be disconnected from all clients when their password is changed.

sebastienros · 2018-01-18T20:24:42Z

        public ILogger Logger { get; set; }

-        public TimeSpan ExpirationTimeSpan { get; set; }
+        public TimeSpan ExpirationTimeSpan {


This is breaking what users currently have to do to change this value (host.config).
Not sure it's wise to make it editable in the UI. More code, not usually used, accessible to more users, ... lots of reasons to keep it the way it is today.

The value for this previously was contant and 30 days. I don't see where it was being read from host.config. Unless writing the form authentication settings there directly overrides anything that is done in code here for the auth cookie.

In this PR, the value is in a setting that is editable from the UI (that is the implementation I put in Orchard.Users, not the default one from Orchard.Framework, even though I realize that close to 100% tenants have Orchard.Users enabled), but here in the service we are getting it from a service, rather than from the setting. It's straightforward to write a service that reads from a config file, or that does something else entirely. DIfferent calls could lead to different values for this.

The value is assigned automatically by autofac when the service is resolve. It's called "Property Injection". I don't think we need a service for this.

I did not know that.
I am trying now to see how to set this up in a per-tenant way using the .config files ("Sites.MyTenant.config", right?)

We could have both the services and property injection:

If I moved that ExpirationTimeSpan to DefaultSecurityService I could still do Property Injection into there.

Alternatively, in the constructor of FormsAuthenticationService I could initialize the value of ExpirationTimeSpan to a negative value. Then rather than using the property in the rest of the class, I could have a method like:

public TimeSpan GetExpirationTime() { if (ExpirationTimeSpan < TimeSpan.Zero) { // We have not set expiration from Sites.Tenant.config return _securityService.GetAuthenticationCookieLifeSpan(); } // we have set the expiration from the config file return ExpirationTimeSpan; }

As long as the UI for each of those services clearly stated that having a value in the config file would take precedence, that behaviour would not be obscure.

The question, I guess, is more of a "should we" than a "could we" nature, at this point, right?

Personally I think there may be value in having the option, for example, to set different ExpirationTimeSpan depending on the "source" of the SignIn. For example, if you SignIn from a browser I may set a short validity, but if you SignIn from my app on your phone I may decide that that may be ok with a longer-lived cookie (of course that requires me to identify the source in my service, but that is a separate issue).

Another scenario may be:

Cookie expires in a bunch of days for admin

Cookie expires in a shorter timeframe for "normal" users

sebastienros · 2018-01-18T20:26:09Z

+            var userDataDictionary = new Dictionary<string, string>();
+
+            if (formsIdentity.Ticket.Version == 3) {
+                var userDataSegments = userData.Split(';');


Just revoke the cookie.

It's what I initially thought of doing. However that would have meant that deploying this update to how the cookies are processed would have caused all users to effectively be logged out.
That would have been jarring in terms of UX, and no LoggedOut event would have been fired for anyone.

Something else I thought would be adding at the end of this method a condition to replace V3 cookies with new ones by releasing it (I'd just call CreateAndAddCookie)

I like the idea of replacing as a good compromise.

sebastienros · 2018-01-18T20:27:26Z

            return cookiePath;
        }
+
+        #region Serialization of UserData Dictionary


sebastienros · 2018-01-18T20:28:11Z

            return cookiePath;
        }
+
+        #region Serialization of UserData Dictionary


Don't even try, otherwise there is a risk that user data conflict with the format if you are not escaping correctly.

sebastienros · 2018-01-18T20:29:35Z

+using Orchard.Users.Models;
+
+namespace Orchard.Users.Services {
+    public class PasswordChangedDateUserDataProvider : BaseUserDataProvider {


Does it need to be extensible? Isn't the setting already available in the form authentication service?

Could you clarify what you mean? I don't understand what you are referring to.

I don't see why we need to be able to extend this. The forms auth logic should be self sufficient, and store in the auth cookie only what it needs. If other modules need to store stuff for the user in a cookie, they are free to create their own.

The point is not to store information in the auth cookie per se, but to stamp it with information to be able to invalidate it if/when required by something. In the specific case of PasswordChangedDateUserDataProvider, the idea is that a password change can make all auth cookies, for the user and from any client, invalid. That information we are storing in the cookie (the datetime in this case) is not for the user/client.

sebastienros · 2018-02-01T20:23:12Z

I'll let you do all the changes and compromises that you deem necessary, and trust your judgement as you are the main user of this feature and it's not breaking.
Once you are happy just let me know and I will merge.

Changed FormsAuthenticationService and DefaultSecurityService so that property injection configured in sites.config still works the same as before, and takes priority over ISecurityService implementations. Changed serialization of UserData dictionary to use Newtonsoft.Json. Added check to upgrade the authentication cookie whenever an "old" one is received.

…ep the same expiration

MatteoPiovanelli · 2018-02-05T16:33:08Z

Thanks for the trust in my code.

I think with the last couple commits this feature is good for me.
I removed the service implementations I previously added in Orchard.Users to override the cookie ExpirationTimeSpan. I also wrote the code in FormsAuthenticationService so that it should behave exactly as it does right now (wrt the ExpirationTimeSpan): if autofac injects a value on that property, it will take priority on services trying to affect that; otherwise, the value form services will be used (with the default implementation giving 30days, that is the current default).
I used Newtonsoft.Json to serialize/deserialize the userdata dictionary. I had missed the fact that the dependency on that was already in place.
A call to GetAuthenticatedUser for a request that carries an "old" cookie will cause an upgrade of the cookie: the way it's set up, the new cookie that will be released will have the same expiration as the "old" cookie from the request.

As it is, nothing should break. Let me know what do you think.

sebastienros · 2018-02-05T17:28:52Z

I trust you more than your code.

MatteoPiovanelli added 5 commits January 17, 2018 17:33

Added providers to compute and validate the userdata string for authe…

9bd0887

…ntication cookies

Added UserDataProvider based on the date when the password was change…

3976702

…d last: this means that changing the password invalidates all extant authentication cookies

Added a service that allows configuring the lifespan of auth cookies,…

abf8d92

… rather than always using a constant. The default implementation returns the same constant.

Added setting to control the life span of authentication cookies

6ff8ac6

cleanup

383761e

MatteoPiovanelli commented Jan 18, 2018

View reviewed changes

Added setting to control the behaviour on changing password: logout e…

96de0fb

…very client, or none. Added code to prevent users that were authenticated with "old" cookies to be suddenly logged out when deploying this

sebastienros reviewed Jan 18, 2018

View reviewed changes

Changed some labels

c843bee

MatteoPiovanelli added 2 commits February 5, 2018 15:20

On cookie upgrade we do not give a fresh new cookie, but rather we ke…

d141da8

…ep the same expiration

MatteoPiovanelli mentioned this pull request Feb 5, 2018

Authentication cookie remains valid even after password change - potential security issue #6014

Closed

sebastienros merged commit 0754e7b into OrchardCMS:dev Feb 5, 2018

MatteoPiovanelli mentioned this pull request May 25, 2021

Actually signout user on change password. #8478

Merged

Uh oh!

Conversation

MatteoPiovanelli commented Jan 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MatteoPiovanelli Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MatteoPiovanelli commented Jan 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MatteoPiovanelli Jan 18, 2018 •

edited

Loading