SignInResult extensibility for SignInManager

[RoySalisbury]

The current implementation of the SignInManager does not allow for extending the SignInResult that can get returned from something like PasswordSignInAsync.

For example, I need to add expiring passwords to my users. This means I need to add a new "result" to the call. Just like "LockedOut", "RequiresTwofactor", ect. I will be adding a "RequiresPasswordChange". I can override PasswordSignInAsync and add my logic, but there is no way to return the status. What I propose is the following:

// Existing class logic goes here
public class SignInResult<T> where T : SiginInResult<T>, new()

// backwards compatibility signature
public class SignInResult : SignInResult<SignInResult> {}

And

// Existing class logic goes here
public class SignInManager<TUser, TSignInResult>
    where TUser : class
    where TSignInResult : SignInResult<TSignInResult>
{
    public virtual async Task<TSignInResult> PasswordSignInAsync(TUser user, string password, bool isPersistent, bool lockoutOnFailure)
    {
        return SignInResult.Success as TSignInResult;
    }
}

// backwards compatibility signature
public class SignInManager<TUser> : SignInManager<TUser, SignInResult> {}

Now, if I wanted to extend the SignInManager ....

public class ApplicationSignInResult : SignInResult<ApplicationSignInResult>
{
    private static readonly ApplicationSignInResult _passwordChangeRequired = new ApplicationSignInResult { RequiresPasswordChange = true };

    public bool RequiresPasswordChange { get; protected set; }

    public static ApplicationSignInResult PasswordChangeRequired => _passwordChangeRequired;

    public override string ToString()
    {
        return IsLockedOut ? "Lockedout" :
            IsNotAllowed ? "NotAllowed" :
            RequiresTwoFactor ? "RequiresTwoFactor" :
            RequiresPasswordChange ? "RequiresPasswordChange" :
            Succeeded ? "Succeeded" :
            "Failed";
    }
}

public class ApplicationSignInManager : SignInManager<ApplicationUser, ApplicationSignInResult>
{
    public override async Task<ApplicationSignInResult> PasswordSignInAsync(ApplicationUser user, string password, bool isPersistent, bool lockoutOnFailure)
    {
        //return base.PasswordSignInAsync(user, password, isPersistent, lockoutOnFailure);
        return ApplicationSignInResult.PasswordChangeRequired;
    }
}   

I'm going to have to completly replace the SignInManager with my own Application specific version (not very good), or have something like what I am proposing here... I'm going to do the work either way, I just need to know if the approach will work. And what it will mess up internally. As far as I can tell, it looks like a solid approach.

[RoySalisbury]

OK.. So I forged the Identity repository, and created a new branch with the potential updates. If someone could review them before I do the whole pull request process I would appreciate it.

https://github.com/RoySalisbury/Identity/tree/SignInManagerGenerics

I'm sure I will need to figure out some type of tests to create, but before I go that far I need to get the current fork running its own tests.

[RoySalisbury]

OK.. Found and issue... < doh! >

In the base class I used a stupid shortcut so I did not have to refactor the code.

return SignInResult.Failure as TSignInResult;

However, I'm trying to cast UP and not down. I'll need to refactor that a bit.

It was a fun exercise to get this all working so far ... from forking, branch, build, package, publish, and use in my own project... Took a while, but had fun.

BTW, the extension that was added for AddSignInManager in another branch will need to be updated with this as well. I had written it as a standalone extension awhile back and had to update it as follows:

public static void AddIdentitySignInManager<TSignInManager, TUser, TSignInResult>(this IServiceCollection services)

var managerType = typeof(SignInManager<,>).MakeGenericType(typeof(TUser), typeof(TSignInResult));

[RoySalisbury]

Well, beginning to think this may not be possible. Its probably why having static properties/methods in generic types is discouraged. No matter what I come up with, it always means a breaking change. Basically, it would be easier to leave the SignInManger signature like it is, and just deal with creating a new (or updated) SignInResult.

A better solution might be to model SignInResult after IdentityResult.

With IdentityResult you basically have "Success", and "Failed". If "Failed" you can provide a list of IdentityError's { Code, Description }. This allows it to be extensible, just not strongly typed (what I originally going for). And In most cases in the source, you use the "IdentityErrorDescriber" as a way to localize the strings in the "description".

I like this approach. You can still have the current SignInResult properties so the existing code stays as is, but if "Failed" is true, then there may be further information in the "Errors" property. (e.g. { Code = "PasswordChangeRequired", "Description = Resources.PasswordChangeRequired }). I always hated branching code based string comparison, but sometimes its just how it has to be.

So we wind up with:

public class SignInResult 
{
    // All Existing stuff still here.....

    // New Stuff
    private List<IdentityError> _errors = new List<IdentityError>();
    public IEnumerable<IdentityError> Errors => _errors;
}

We reuse the IdentityError class since it appears to be designed to be extended for the Identity system and is even setup for DI. Its also used in the PasswordValidator, RoleManager, UserManager, ect).

I'll modify my branch to try this structure.

[RoySalisbury]

Whats better .. a minor breaking change, or an addition that is semi-lame.

Breaking (modify the existing "Failed" property:

public static SignInResult Failed(params IdentityError[] errors)

Or ugly ..

public static SignInResult Failed => _failed;
public static SignInResult FailedWithError(params IdentityError[] errors)

I'm going to go with the 2nd option until someone can chime in on what shuld be done about the existing "Failed" results (NotAllowed, TwofactorRequired, ect) that do not have an IdentityError assigned to them.

[HaoK]

So why can't you just add a new RequiresPasswordChangeSignInResult : SignInResult, and override whatever sign in methods you want to return this result in the cases you need.

You can add an extension method to SignInResult.RequiresPasswordChange to return true if its an instance of your type.

[RoySalisbury]

Because you cant change the return type of the overridden method.

For example...

public class MyIdentityResult : IdentityResult { }

public override async Task<MyIdentityResult> ChangePhoneNumberAsync(Guid userId, string phoneNumber, string token)
    {
        return new MyIdentityResult();
    }

This will error with:

'ApplicationUserManager.ChangePhoneNumberAsync(Guid, string, string)': return type must be 'Task<IdentityResult>' to match overridden member 'UserManager<ApplicationUser, Guid>.ChangePhoneNumberAsync(Guid, string, string)'  

I could create a NEW one, and possibly hide the parent, but then that's not very good.

Roy

[HaoK]

Why are you trying to change the return type, return your derived new result for the cases where you need in your method, there's no data on it, so what's wrong with what I suggested above, create a type for your new RequiresPasswordChangeSignInResult, update your calling code to contain a check for that result, alongside all the old cases. That's basically no different than how you use the other built in results today.

Leave the signatures alone

[RoySalisbury]

Or are you suggesting I treat the SignInResult like an abstract class. Always have to check the type so see what it is before I cast it. That seems a bit heavy handed.

public class MySignInResult : SignInResult 
{
    public static MySignInResult RequiresPasswordChange => _requiresPasswordChange;     
}

public override async Task<SignInResult> PasswordSignInAsync(ApplicationUser user, string password, bool isPersistent, bool lockoutOnFailure)
{
    return MySignInResult.RequiresPasswordChange;
}

And then in my code

var signInResult = await PasswordSignInAsync(user, "password", false, true);

// Do normal checks .. is success, lockout, ect

// Now do my checks 
if (SignInResult.RequiresPasswordChange) {}
else if (SignInResult.RequiresSomeOtherThing) {}

Is that what your suggesting?

I could also put better logic in my class so that I just do a single cast, check it, and then switch on the results:

var mySignInResult = signInResult as MySignInResult;
if (mySignInResult != null)
{
    switch (mySignInResult.ErrorCode)
    {
         case 1: { break; }
         case 2: { break; }
         case 3: { break; }
    }
}

[RoySalisbury]

So I have implemented it the way you suggested, and while it does work, its not something that lends itself to an inclusion into the base code. So I wont need to provide all the PasswordHistory, LastLogin and other changes for inclusion into the framework. If I run into other things that might require internal changes, I will re-branch and see what I can contribute.

Thanks

BTW: The added CheckPasswordSignInAsync in the SignInManager helped a lot. Thanks.

[HaoK]

Cool