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

[Bugfix] CacheLoginThrottleService->isThrottled #412

Merged
merged 3 commits into from
Sep 20, 2014
Merged

[Bugfix] CacheLoginThrottleService->isThrottled #412

merged 3 commits into from
Sep 20, 2014

Conversation

freezy-sk
Copy link
Contributor

When login fails on too many login attempts, it's not possible to check it because isThrottled() is passing different identity into countThrottle() method.
Now it will be the same and it should fix #387 and #392

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.97%) when pulling 36271f2 on freezy-sk:bugfix/isThrottled into bdfbcff on Zizaco:master.

@ghost
Copy link

ghost commented Sep 19, 2014

@freezy-sk I think you should still do the serialize in case username and email don't exist.

    public function isThrottled($identity)
    {
        $identity = $this->parseIdentity($identity);

        // Retuns the current count
        $count = $this->countThrottle($identity, 0);

    }
    protected function countThrottle($identityString, $increments = 1)
    {
        $count = $this->app['cache']
            ->get('login_throttling:'.md5($identityString), 0);

       ....
    }

md5($identityString)

string md5 ( string $str [, bool $raw_output = false ] )

@freezy-sk
Copy link
Contributor Author

@WPPD yes you're right but the best would be using same function for isThrottled and loginAttempt but it needs bigger refactoring

@@ -1,2 +1,3 @@
/vendor
composer.lock
.idea
Copy link
Owner

Choose a reason for hiding this comment

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

Can you please remove .idea from .gitignore.
It's better to put this into your global git config instead.

@Zizaco
Copy link
Owner

Zizaco commented Sep 19, 2014

Thanks. But can you fix the gitignore first?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.96%) when pulling 1644f09 on freezy-sk:bugfix/isThrottled into bdfbcff on Zizaco:master.

@freezy-sk
Copy link
Contributor Author

@Zizaco It's removed right now. I have it in my global gitignore but it's my (bad)habit to add it also to project just to be sure that other developers don't commit their .idea if they don't have it globally ignored.

Zizaco added a commit that referenced this pull request Sep 20, 2014
[Bugfix] CacheLoginThrottleService->isThrottled
@Zizaco Zizaco merged commit c27d251 into Zizaco:master Sep 20, 2014
@Zizaco
Copy link
Owner

Zizaco commented Sep 20, 2014

😄

@freezy-sk freezy-sk deleted the bugfix/isThrottled branch September 20, 2014 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Login throttling doesn't work as expected for me
3 participants