Add configurable user lockout #1127
Conversation
update lockoutfix to latest for testing
|
Line endings got messed up again, going to fix before someone tries to review in pain. |
| var maxCount = user.Policy.IsAdministrator ? 3 : 5; | ||
| // Check for users without a value here and then fill in the default value | ||
| // also protect from an always lockout if misconfigured | ||
| if (user.Policy.LoginAttemptsBeforeLockout == null || user.Policy.LoginAttemptsBeforeLockout == 0) |
There was a problem hiding this comment.
I feel like this value should've been set prior to reaching this
There was a problem hiding this comment.
So this is for users that currently exist. They should (theoretically) have a value of null here. This will keep the old expected behavior in play until their accounts get the setting updated. The only other place I could think to set this was in the load code for the Policies and I'd rather not do it there.
| @@ -104,6 +105,8 @@ public UserPolicy() | |||
|
|
|||
| AccessSchedules = Array.Empty<AccessSchedule>(); | |||
|
|
|||
| LoginAttemptsBeforeLockout = -1; | |||
There was a problem hiding this comment.
Shouldn't this be null? I take it this is the default value when creating a new User Policy.
There was a problem hiding this comment.
From the other PR,
Because I didn't have a better idea for what number should represent "this hasn't been set because legacy.
So it makes total sense to me that -1 is "off", and "0" is "old defaults" for compatibility, with a default of "0". I'm not sure if Null == 0 in C#, but even if so numbers are consistent and then the user can disable it with -1 too, which is a prety common convention I've seen.
Changes
This adds a configurable user option for the lockout attempts, and changes the default for new accounts to -1 (lockout disabled)
Issues
#187 is addressed by this, though not fully solved
Note: This change also necessitates a PR in jellyfin web, link: jellyfin/jellyfin-web#190