AUTHLIB-180 Lockout Cooldown - #97
Conversation
Added a scheduled job that looks for locked user accounts and, if a configured cooldown period has elapsed, unlocks those accounts. Added configuration includes: * `octri.authentication.lockout-cooldown-period`: Defaults to 15 minutes. * `octri.authentication.lockout-polling-schedule`: Defaults to 30 minutes. Controls frequency with which the unlock job runs. Implementation notes: * The current implementation checks that `user.enabled` is true but, otherwise, does not check things such as account or credential expiration. * Unlocking the account resets the number of login failures to 0.
Removed commend about null configuration, due to issues testing the configured value is null.
| /** | ||
| * Schedule for cron task to check cooldown on locked accounts. Defaults to every 15 minutes. | ||
| */ | ||
| private String lockoutPollingSchedule = "0 */15 * * * *"; |
There was a problem hiding this comment.
I would default this to every minute to minimize the time that an account might remain locked after the cooldown period ends.
| * Minimum time (in minutes) that must elapse between the most recent failed login and automatic account unlock. | ||
| * Defaults to 30 minutes. | ||
| */ | ||
| private Integer lockoutCooldownPeriod = 30; |
There was a problem hiding this comment.
This would be more natural as a Duration. See passwordTokenValidFor in this file for an example.
There was a problem hiding this comment.
One other thought: I might group the auto-unlock properties in their own configuration property class, which would make adding an explicit feature flag easier.
| if (cooldownPeriod == null) | ||
| return; |
There was a problem hiding this comment.
We always include curly braces in if tests to prevent bugs when updating code. Our code formatting rules should have prevented this.
The cleanest way to implement conditional behavior is to prevent bean creation using one of the In this case, you could add an
I would probably initially default to disabling automatic unlock to preserve existing behavior, but we might want to revisit the default the next time we do a major version release. |
Implement changes based on feedback: * Convert cooldown from Integer to Duration - updated naming from `lockoutCooldownPeriod` to `lockoutCooldownDuration` to match. * Update the polling default to every minute, to minimize default lockout time.
heathharrelson
left a comment
There was a problem hiding this comment.
This is pretty close, so I expect to approve this once you've reworked the configuration to make the LockoutCooldownJob bean conditional.
Don't forget to document your configuration properties in docs/CONFIGURATION_PROPERTIES.md.
Use the `@ConditionalOnProperty` annotation to make `LockoutCooldownJob` a conditional bean, branching off of whether octri.authentication.lockout-cooldown.enabled` is set. With the feature flag in place, the code no longer uses `duration=null` to control behavior. Additionally, isolates properties in new `LockoutCooldownProperties` class.
| private Duration duration = DEFAULT_COOLDOWN_DURATION; | ||
|
|
||
| /** | ||
| * Schedule for cron task to check cooldown on locked accounts. Defaults to every 15 minutes. |
There was a problem hiding this comment.
This comment needs to be updated to reflect the new default.
| | octri.authentication.enable-password-visibility-toggle | OCTRI_AUTHENTICATION_ENABLE_PASSWORD_VISIBILITY_TOGGLE | boolean | true | Whether to enable the password visibility toggle button. | | ||
| | octri.authentication.lockout-cooldown.enabled | OCTRI_AUTHENTICATION_LOCKOUTCOOLDOWN_ENABLED | boolean | false | Whether to enable automatic account unlocking with configurable cooldown. | | ||
| | octri.authentication.lockout-cooldown.duration | OCTRI_AUTHENTICATION_LOCKOUTCOOLDOWN_DURATION | duration | 30m | Minimum lockout duration before account is unlocked. | | ||
| | octri.authentication.lockout-cooldown.pollingSchedule | OCTRI_AUTHENTICATION_LOCKOUTCOOLDOWN_POLLINGSCHEDULE | string | "0 */1 * * * *" | Cron schedule to unlock eligible accounts. | |
There was a problem hiding this comment.
Let's save ourselves from having to mentally parse cron expressions.
| | octri.authentication.lockout-cooldown.pollingSchedule | OCTRI_AUTHENTICATION_LOCKOUTCOOLDOWN_POLLINGSCHEDULE | string | "0 */1 * * * *" | Cron schedule to unlock eligible accounts. | | |
| | octri.authentication.lockout-cooldown.pollingSchedule | OCTRI_AUTHENTICATION_LOCKOUTCOOLDOWN_POLLINGSCHEDULE | string | "0 */1 * * * *" | Cron schedule to unlock eligible accounts. Defaults to every minute. | |
heathharrelson
left a comment
There was a problem hiding this comment.
Thanks for refining this!
Update comment to reflect accurate polling schedule default, and state the default schedule behavior in plain English in CONFIGURATION_PROPERTIES.md
Overview
Added a scheduled job that looks for locked user accounts and, if a configured cooldown period has elapsed, unlocks those accounts. Added configuration includes:
octri.authentication.lockout-cooldown.enabled: Feature flag, defaults to false.octri.authentication.lockout-cooldown.duration: Defaults to 30 minutes.octri.authentication.lockout-cooldown.polling-schedule: Defaults to every minute. Controls frequency with which the unlock job runs.Issues
AUTHLIB-180
[x] Added to CHANGELOG.md
Discussion
Issue with implementation
Already addressed in discussion.
Other notes
user.enabledis true but, otherwise, does not check things such as account or credential expiration.