Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented account lockout policy backend #20087

Merged
merged 2 commits into from
May 4, 2020

Conversation

skateman
Copy link
Member

@skateman skateman commented Apr 21, 2020

So according to the parent issue I added a new column to the users table for counting failed login attempts. I introduced two new parametres under Settings.authentication for setting the maximum number of failed login attempts before a lockout and the timeout after the account gets unlocked by a background job.

I did some changes in the Authenticator::Base class for having a way to retrieve the actual reason of a login failure. If there's no failure provider, it would fall back to the original Authentication failed message. This message is further consumed by the UI and also to the API.

I have this feeling that @abellotti might not like the idea of returning with two things from the _authenticate method instead of one, but I tried to limit the scope of my changes to just the DB authentication as we discussed. If you have a better idea of how do to it, I am willing to change my second commit.

@miq-bot add_reviewer @abellotti
@miq-bot add_reviewer @Fryguy
@miq-bot add_reviewer @jvlcek
@miq-bot add_label enhancement, question

@skateman skateman changed the title Implemented account lockout policy backend [WIP] Implemented account lockout policy backend Apr 21, 2020
@miq-bot miq-bot requested review from abellotti and jvlcek April 21, 2020 11:45
app/models/user.rb Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
config/settings.yml Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
@Fryguy
Copy link
Member

Fryguy commented Apr 21, 2020

This is coming along really nice. Thanks @skateman !

@skateman skateman force-pushed the locked-account branch 2 times, most recently from fb122dc to f02da36 Compare April 22, 2020 14:55
@skateman skateman requested a review from bdunne as a code owner April 22, 2020 14:55
Copy link
Member

@jvlcek jvlcek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I am in favor of this PR as long as there is an easy mechanism in place to re-enable any locked out users. When a system admin is setting up a new appliance I want to make sure they can quickly troubleshoot any issues.
It might be best to have this feature disabled by default and require the admin user enable it once the system is configured properly.

app/models/authenticator/base.rb Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
@Fryguy Fryguy removed the question label Apr 24, 2020
@skateman skateman force-pushed the locked-account branch 2 times, most recently from 63cf5b0 to ef18dd7 Compare April 27, 2020 06:02
@Fryguy
Copy link
Member

Fryguy commented May 3, 2020

If a user succeeds, we should set the failed back to 0. Otherwise, they can get 2 fails, succeed, then log out, fail once, and it will say "too many failed attempts", which is weird.

@Fryguy
Copy link
Member

Fryguy commented May 3, 2020

Can we also get some specs?

@skateman skateman requested a review from gtanzillo as a code owner May 4, 2020 10:42
app/models/user.rb Outdated Show resolved Hide resolved
@skateman
Copy link
Member Author

skateman commented May 4, 2020

This also works out-of-the box with the API:

{
  "error": {
    "kind": "unauthorized",
    "message": "Your account has been locked due to too many failed login attempts, please contact the administrator.",
    "klass": "Api::BaseController::Authentication::AuthenticationError"
  }
}

@skateman skateman changed the title [WIP] Implemented account lockout policy backend Implemented account lockout policy backend May 4, 2020
@skateman
Copy link
Member Author

skateman commented May 4, 2020

@Fryguy created cross repo tests ManageIQ/manageiq-cross_repo-tests#117 they're green

@miq-bot miq-bot removed the wip label May 4, 2020
@Fryguy
Copy link
Member

Fryguy commented May 4, 2020

Couple of minor things, but this is just about good to go.

@kbrock
Copy link
Member

kbrock commented May 4, 2020

Can the admin user get locked? what do you do in that case?

@skateman
Copy link
Member Author

skateman commented May 4, 2020

@kbrock this has been discussed at the issue, the locking is just temporary and it can be unlocked from the rails console

@miq-bot
Copy link
Member

miq-bot commented May 4, 2020

Checked commits skateman/manageiq@303c3c6~...beac7c9 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
5 files checked, 1 offense detected

spec/models/authenticator/database_spec.rb

@Fryguy Fryguy merged commit 927b99f into ManageIQ:master May 4, 2020
@dmetzger57 dmetzger57 mentioned this pull request May 4, 2020
4 tasks
@skateman skateman deleted the locked-account branch May 4, 2020 20:13
simaishi pushed a commit that referenced this pull request May 8, 2020
Implemented account lockout policy backend

(cherry picked from commit 927b99f)
@simaishi
Copy link
Contributor

simaishi commented May 8, 2020

Jansa backport details:

$ git log -1
commit b74002bab48a9f9d9a2d9850eb8c9e26e90d606d
Author: Jason Frey <[email protected]>
Date:   Mon May 4 16:11:13 2020 -0400

    Merge pull request #20087 from skateman/locked-account

    Implemented account lockout policy backend

    (cherry picked from commit 927b99f526e130cd4e1fdbf07ad66156111c05d8)

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

Successfully merging this pull request may close these issues.

8 participants