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

Login Attempts object are duplicated #421

Open
alex-dna opened this issue Feb 1, 2021 · 8 comments
Open

Login Attempts object are duplicated #421

alex-dna opened this issue Feb 1, 2021 · 8 comments

Comments

@alex-dna
Copy link

alex-dna commented Feb 1, 2021

Hi,

I've recently encountered an issue where every time a user tries to login (regardless of success or failure), 2 LoginAttempt objects are created virtually at the same time.

I have tested it on a clean SilverStripe install with the MFA module installed (no extra config), with and with the TOPT module,
and get consistent duplication of the LoginAttempt.

This is causing an issue when a user has been locked out of the system for 15 minutes because of too many failed attempts.

I spent a few hours trying to pin point what could be the root cause, without success.
As far as I can tell, the LoginAttempt are created as part of the Authenticate method on the SilverStripe\Security\Authenticator class. I can't seem to find any instance where this method would be called twice or independently.

Perhaps someone else has experienced the same issue (you wouldn't know until looking in the DB really).

Thanks.
Alex

Screen Shot 2021-02-01 at 5 17 12 PM

@ScopeyNZ
Copy link
Contributor

ScopeyNZ commented Feb 1, 2021

Do you have any custom code that might be calling Member::registerFailedLogin? This module works by subbing out the LoginHandler from core, and we only call this method once:

$store->getMember()->registerFailedLogin();

@alex-dna
Copy link
Author

alex-dna commented Feb 1, 2021

HI, no I don't have any custom code as I have been re-testing on a clean install.
I've tracked it down to the doLogin method:
If MFA is step is not being displayed because not active or user has skipped, the parent::doLogin method is called.
However, this doLogin calls for checkLogin which will create a LoginAttempt, and so does the parent::doLogin.

public function doLogin($data, MemberLoginForm $form, HTTPRequest $request)
{
/** @var Member&MemberExtension $member */
$member = $this->checkLogin($data, $request, $result);
$enforcementManager = EnforcementManager::singleton();
// If:
// - there's no member it's an invalid login, or
// - the enforcement manager determines that MFA should not be shown
// then we can delegate to the parent as this will just be the normal login flow (without MFA)
if (!$member || !$enforcementManager->shouldRedirectToMFA($member)) {
return parent::doLogin($data, $form, $request);
}

@brynwhyman
Copy link

Hey @alex-dna, a little late to this one but I think you're seeing the following issue surface: silverstripe/silverstripe-framework#9474

As a workaround you might want to look at extending the rate limits on your project: https://docs.silverstripe.org/en/4/developer_guides/security/rate_limiting/#rate-limiting.

@dhensby
Copy link

dhensby commented Mar 25, 2021

This doesn't look to be anything to do with rate limiting; but I'm not entirely following why the parent doLogin is called twice

@alex-dna
Copy link
Author

SilverStripe\Security\MemberAuthenticator\MemberAuthenticator::authenticateMember is called by SilverStripe\Security\MemberAuthenticator\MemberAuthenticator::authenticate which is called by SilverStripe\Security\MemberAuthenticator\LoginHandler::checkLogin

When SilverStripe\MFA\Authenticator\LoginHandler::doLogin is called, the checkLogin is called first to find a member, creating a LoginAttempt. If no member is found, then parent::doLogin which is essentially SilverStripe\Security\MemberAuthenticator\LoginHandler::doLogin is called, calling checkLogin again, creating the duplicate LoginAttempt

@JamesDPC
Copy link

I'm seeing this as well. The workaround is to double the lock_out_after_incorrect_logins config value.

The failed login user/pass handling before any MFA action occurs is:

  1. MFA LoginHandler doLogin called
  2. calls parent::checkLogin
  3. Attempts to authenticate the member, fails (records a failed login attempt)
  4. MFA LoginHandler calls parent::doLogin
  5. checkLogin hit
  6. Attempts to authenticate the member, fails (records a failed login attempt)
  7. 2x failed Login attempts recorded

If the initial checkLogin attempt fails @ https://github.com/silverstripe/silverstripe-mfa/blob/4/src/Authenticator/LoginHandler.php#L90 it should fail immediately & record 1 failed login attempt. Looks like checking the result of the first checkLogin and shouldRedirectToMFA should be separated.

Cheers
James

@kinglozzer
Copy link
Member

Spotted this on a new site, so just confirming this is still an issue in SS5

@blueo
Copy link

blueo commented Jan 18, 2024

I ran into this today and found it coming from the LoginHandler::doLogin. It first seems to call checkLogin which will call authenticate on the Authenticator and record an attempt - if no user is found (failed login) it then calls parent::doLogin which will call checkLogin again which records a second attempt

public function doLogin($data, MemberLoginForm $form, HTTPRequest $request)
    {
        /** @var Member&MemberExtension $member */
        $member = $this->checkLogin($data, $request, $result);
        $enforcementManager = EnforcementManager::singleton();

        // If:
        //  - there's no member it's an invalid login, or
        //  - the enforcement manager determines that MFA should not be shown
        // then we can delegate to the parent as this will just be the normal login flow (without MFA)
        if (!$member || !$enforcementManager->shouldRedirectToMFA($member)) {
            return parent::doLogin($data, $form, $request);
        }

will see if there is a way to avoid this double record

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

No branches or pull requests

8 participants