Skip to content
This repository has been archived by the owner on Dec 20, 2018. It is now read-only.

LockoutEnabled not enable after fail login attempt #1764

Closed
ngohungphuc opened this issue Apr 19, 2018 · 18 comments
Closed

LockoutEnabled not enable after fail login attempt #1764

ngohungphuc opened this issue Apr 19, 2018 · 18 comments
Labels

Comments

@ngohungphuc
Copy link

I have turn on user login attempt in controller

  var result = await _signInManager.PasswordSignInAsync(model.Username, model.Password,model.RememberMe, true);

Startup

 services
                .AddIdentity<User, ApplicationRole>(options =>
                {
                    options.Password.RequireDigit = false;
                    options.Password.RequiredLength = 4;
                    options.Password.RequireLowercase = false;
                    options.Password.RequireNonAlphanumeric = false;
                    options.Password.RequireUppercase = false;

                    //lock out attempt
                    options.Lockout.AllowedForNewUsers = true;
                    options.Lockout.DefaultLockoutTimeSpan = TimeSpan.FromMinutes(30);
                    options.Lockout.MaxFailedAccessAttempts = 2;
                 })
                .AddEntityFrameworkStores<ApplicationDbContext>()
                .AddDefaultTokenProviders();

But when user login fail only the LockoutEnd have data. The LockoutEnabled column is always false so user can keep login back again. Any idea about this. Thank you

@HaoK
Copy link
Member

HaoK commented Apr 19, 2018

It should get set on your user after you CreateAsync the user, see: https://github.com/aspnet/Identity/blob/dev/src/Core/UserManager.cs#L477

@ngohungphuc
Copy link
Author

@HaoK I mean when I already have account I want to login to system and the LockoutEnabled is set to true after 2 times login fail

@HaoK
Copy link
Member

HaoK commented May 1, 2018

I don't understand what you are asking.

So there are two lockout related properties on the user.

LockoutEnabled must be set to true for the feature to be enabled for the user at all.
If lockoutEnabled = True and LockoutEnd date is set to a time later than now, the user should be considered locked out.

@dviry
Copy link

dviry commented May 31, 2018

In this specific case, if a user fails the login for options.Lockout.MaxFailedAccessAttempts = 2; the code is only setting the lockoutEnd date but not the lockoutEnabled so he can basically still login.

Regarding your code quote, that also does not work since it only sets the lockoutEnabled but not the lockoutEnd -> the check in IsLockedoutAsync() will return False if the lockoutEnd is not set.

All this also fails in the VisualStudio Identity.Core sample template as well - in fact, it's even worse there as the AccessFailedCount column never gets incremented.

Cheers

@HaoK
Copy link
Member

HaoK commented May 31, 2018

That's correct, if lockout isn't enabled for the user, it won't do anything. Whether lockout is enabled for a user or not, is a separate field, typically you turn it on via AllowedForNewUsers which is default to on, so typically when a user is registered that flag is set to true.

https://github.com/aspnet/Identity/blob/dev/src/Core/LockoutOptions.cs#L19

@HaoK
Copy link
Member

HaoK commented May 31, 2018

Its used in Create by the user manager to set LockoutEnabled here:

await GetUserLockoutStore().SetLockoutEnabledAsync(user, true, CancellationToken);

@dviry
Copy link

dviry commented Jun 1, 2018

IsLockedoutAsync() expects BOTH LockoutEnabled AND LockoutEnd, otherwise it will always return FALSE (not locked out).

Please read again my comment!

@HaoK
Copy link
Member

HaoK commented Jun 1, 2018

Yes that's how the code is supposed to work, the user must have lock out enabled, and there must be a lock out end date that's in the future.

@dviry
Copy link

dviry commented Jun 1, 2018

I'll try again ;)

If that's how it supposed to work, then why:
a. CreateAsync for new users only sets the LockoutEnabled but not LockoutEnd?
b. failed logins only sets the LockoutEnd and not LockoutEnabled?

In both of those cases the user should be locked out but instead the method IsLockedoutAsync() returns false (so the user can still login).

I'd urge you to read again the whole thread...

Cheers

@HaoK
Copy link
Member

HaoK commented Jun 1, 2018

a. because unless you want your newly registered user to be locked out, you don't set lockout end.
b. failed logins should set login end to trigger lockout if its enabled.

I'm pretty sure I'm not the one that's confused...

Anyways this behavior is by design

@HaoK HaoK closed this as completed Jun 1, 2018
@HaoK HaoK added the question label Jun 1, 2018
@dviry
Copy link

dviry commented Jun 1, 2018

The default SigninManager provided with Identity.Core does not do that, and neither does the sample provided by the Visual Studio template.

I am pretty sure it's at least 2 people on this thread trying to tell you the code is NOT working correctly (UserManager in combination with SignInManager), so I suggest you try it out and then we'll see who is confused ;)

Cheers

@HaoK
Copy link
Member

HaoK commented Jun 1, 2018

This code hasn't changed in a long time, since before core, and there are unit tests so while its possible there's an issue, I'd assume it would have been discovered earlier.

If I were to guess what is going on, users were created with lockout disabled (allowedForNewUsers = false, or by some other mechanism) so their lockoutEnabled = false. If that's the case, irrespective of what lockoutEnd value is there, that user will never be locked out.

The templates indeed do not have this enabled by default as its no longer recommended practice as it allows people to lock out other users.

@dviry
Copy link

dviry commented Jun 4, 2018

I understand that things change and designs/templates need to be adjusted, but in the short term at least an update to the documentation would probably be helpful for all sides.
In it's current state the following configuration has absolutely no effect, which I am sure was not the original intent...

//lock out attempt options.Lockout.AllowedForNewUsers = true; options.Lockout.DefaultLockoutTimeSpan = TimeSpan.FromMinutes(30); options.Lockout.MaxFailedAccessAttempts = 2;

Cheers

@HaoK
Copy link
Member

HaoK commented Jun 4, 2018

The new default UI might be making it harder to turn on, did you scaffold any of your pages?

@HaoK
Copy link
Member

HaoK commented Jun 4, 2018

I don't see any issues with the current default UI templates, I tried by just setting

        services.AddDefaultIdentity<IdentityUser>(o => o.Lockout.MaxFailedAccessAttempts = 1)

And then the user on first failure is immediately locked out.

@dviry
Copy link

dviry commented Jun 4, 2018

Thanks, but it seems we are using different UI templates :(
I've used the latest on VisualStudio 2017, but I don't have .AddDefaultIdentity, instead only .AddIdentity:

        services.AddIdentity<ApplicationUser, IdentityRole>(options =>
        {
            options.Lockout.MaxFailedAccessAttempts = 3;
            options.Lockout.DefaultLockoutTimeSpan = TimeSpan.FromMinutes(3);

            options.SignIn.RequireConfirmedEmail = true;
        })
            .AddEntityFrameworkStores<ApplicationDbContext>()
            .AddDefaultTokenProviders();

I am using the latest NuGet:

        <PackageReference Include="Microsoft.AspNetCore.All" Version="2.0.6" />

Cheers

@HaoK
Copy link
Member

HaoK commented Jun 4, 2018

Did you turn the lockout flag on in your PasswordSignIn call in the controller/login action?

@dviry
Copy link

dviry commented Jun 4, 2018

Yes of course ;)

But really just by looking at the sourcecode (linked in my other replies) this can't work - unless I am looking at the wrong files or missing something...

Cheers

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants