Solutions-Nitriques / anti_brute_force

Secure your Symphony CMS login page against brute force attacks
http://symphonyextensions.com/extensions/anti_brute_force/
Other
10 stars 12 forks source link

Using authorLoginFailure from other extensions #30

Closed michael-e closed 10 years ago

michael-e commented 10 years ago

You may know that I released the Members Anti Brute Force extension which extends the functionality of Anti Brute Force to Members login and password reset.

At the moment my extension registers failures, but doesn't do anything with colored lists. I have two choices to make it work:

  1. Rebuild authorLoginFailure logic in my extension.
  2. Build a function to get the ABF driver and then do s.th. like $this->getABFDriver()->authorLoginFailure($context).

I would prefer the second solution, but unfortunately failures will be registered with the name of your extension, not mine. To overcome this, what would you think about a small change to your extension's authorLoginFailure function which allows to pass a name that should be registered? Like so:

public function authorLoginFailure($context, $ext_name = self::EXT_NAME) {
    // do not do anything is ip is white listed
    if (!ABF::instance()->isWhiteListed()) {

        // register failure in DB
        ABF::instance()->registerFailure($context['username'], $ext_name);

        // if user is now banned
        if (ABF::instance()->isCurrentlyBanned()) {
            // register into gray list
            ABF::instance()->registerToGrayList($ext_name);
            // move to black list if necessary
            ABF::instance()->moveGrayToBlack($ext_name);
        }
    }
}

Do you think that is too much of a hack?

michael-e commented 10 years ago

I forgot: Then I could simply do $this->getABFDriver()->authorLoginFailure($context, $my_extension_name).

nitriques commented 10 years ago

You may know that I released the Members Anti Brute Force

No I did not! AWESOME!!!

Yeah, I think https://github.com/Solutions-Nitriques/anti_brute_force/blob/master/extension.driver.php#L136-L151 should go into the Facade also. Copying code is rarely good.

Will update ASAP. Thanks Michael!

michael-e commented 10 years ago

As soon as you update yours, I will update mine. Please release a new version as well, so I can correct the dependency info for my extension.

Thanks a lot!

nitriques commented 10 years ago

Please release a new version as well, so I can correct the dependency info for my extension

For sure!

nitriques commented 10 years ago

@michael-e How does e56d335 looks like ?

I had to got the the 'convention' for the parameters.

Would you mind testing the dev branch please ?

nitriques commented 10 years ago

Please look at 8cc20b1 instead, e56 was amateur work. I had to rebase in order to not look stupid :smile:

dev is up to date with the rebased version.

michael-e commented 10 years ago

We are close. But you broke the colored lists completely. The reason is that, by default, the $ip is empty in ABF::instance()->authorLoginFailure. So $this->isCurrentlyBanned($ip) will not return true.

nitriques commented 10 years ago

Damn. I'll fix it. Thanks!

michael-e commented 10 years ago

Bump. Hihi.

nitriques commented 10 years ago

Sorry man, I am really really sick (serious cold). As soon as I get better, I'll do it.

michael-e commented 10 years ago

Get well soon! See you then!

nitriques commented 10 years ago

Get well soon! See you then!

Thanks, I am feeling way better now.

So $this->isCurrentlyBanned($ip) will not return true.

From what I've seen and understood, $this->isCurrentlyBanned($ip) acts just like any other calls that accepts an optinal ip parameter: if the $ip var is empty, it will use the current user one, by calling getIp. isCurrentlyBannded calls getFailureByIp with the $ip param, which then calls getIp.

As far as a quick test shows, multiple brute force moves the ip to the gray list and then to the black list.

Maybe I misunderstood something you told me ? Or is this something specific you need to do with different ips ?

michael-e commented 10 years ago

You are right, and I am sorry. I have absolutely no idea why it didn't work on my test server. I am always using rsync to copy extensions to the server, and I swear that the greylist was broken. Now I have removed the file in question from the server, rsynced again, et voilà, it works!

Please release the stuff. I will release a new version of my extension as well. Everything should be cool then.

Thanks again for your patience, and for all the work.

nitriques commented 10 years ago

Great! 1.4.6 is out!

Thanks again for your patience, and for all the work.

Same goes for you!

michael-e commented 10 years ago

Members Anti Brute Force 1.1 is out.

nitriques commented 10 years ago

Awesome!