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

Add brute force protection for login #54

Merged
merged 1 commit into from Aug 12, 2016

Conversation

Projects
None yet
7 participants
@butlerx
Copy link
Contributor

commented Jul 14, 2016

Account gets locked after number of failed logins thats specified in the config, default 3. The lock is reset after the user changes the password or by calling the set_user_lock and changing the value.

@coveralls

This comment has been minimized.

Copy link

commented Jul 14, 2016

Coverage Status

Coverage increased (+0.2%) to 82.412% when pulling afb3541 on butlerx:enchanment/bruteforce-protection into ab94a7d on senecajs:master.

@thekemkid

This comment has been minimized.

Copy link
Member

commented Jul 14, 2016

Two minor suggestions: It might be nice to add a lock timeout to this (e.g. you can try again after an hour) and give devs the ability to remove the brute force protection (e.g. if locktries is set to null there is no lock protection added)

@butlerx butlerx force-pushed the butlerx:enchanment/bruteforce-protection branch from afb3541 to 0826cf6 Jul 14, 2016

@Wardormeur

This comment has been minimized.

Copy link
Member

commented Jul 14, 2016

Timeout requires us an extra-work as an integration with node-cron. We considered the retrieval process is sortof related to your own workflow and shouldn't be enforced, likely what has been done with mailing on seneca-user. In our case @CoderDojo, we prefer an email reset, which wouldn't be implemented here ( seneca-user ) but through our own µs, cd-users, since seneca-user doesn't do emailing anymore.
tldr : We're happy to participate to seneca's growth, but we don't have the workforce to develop feature we won't use, alas :)

@butlerx

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2016

ive added the null check to disable it

@coveralls

This comment has been minimized.

Copy link

commented Jul 14, 2016

Coverage Status

Coverage increased (+0.2%) to 82.412% when pulling 0826cf6 on butlerx:enchanment/bruteforce-protection into ab94a7d on senecajs:master.

@thekemkid

This comment has been minimized.

Copy link
Member

commented Jul 14, 2016

@Wardormeur Fair enough, that's a valid reason 👍

I'm not the maintainer of this, so Its up to @mihaidma or @mirceaalexandru to merge, but this lgtm :)

@mirceaalexandru

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2016

Hey @Wardormeur,
first great job, thank you for this PR.

Now I do have two comments:

  • I would like this to be disabled by default as we do want to preserve the current functionality in case that somebody use the new version, don't see this feature and did not implement the reset/notify using email. I do not want them to have locked accounts. For this reason I think is best to have this feature turn off by default.
  • This will require, I think, a major version change. This is because you added a new property to user and on some DB engines this means to change the sys_user table structure. Because of this I think we should consider this as a breaking change.

@mihaidma any thoughts?

user.js Outdated

userent.load$({id: args.id}, function (err, user) {
if (err) return done(err)
console.log(user.lockTry, args);

This comment has been minimized.

Copy link
@mirceaalexandru

mirceaalexandru Jul 14, 2016

Contributor

Please do not use console.log, use seneca.log

This comment has been minimized.

Copy link
@butlerx

butlerx Jul 15, 2016

Author Contributor

i actually meant to remove those sorry

user.js Outdated
var ok

userent.load$({id: args.id}, function (err, user) {
if (err) return done(err)

This comment has been minimized.

Copy link
@mirceaalexandru

mirceaalexandru Jul 14, 2016

Contributor

this done(err) do not match the description of the function that say returns {ok: false, why: err}

This comment has been minimized.

Copy link
@Wardormeur

Wardormeur Jul 14, 2016

Member

not sure about seneca coding style, but i'd prefer var x, y ,z instead of var x var y var z

This comment has been minimized.

Copy link
@mirceaalexandru

mirceaalexandru Jul 15, 2016

Contributor

@Wardormeur well here the coding standard requires one var/line so its OK like it is now.

user.js Outdated
why = 'account-unlocked'
}
user.save$(function (err, user) {
if (err) return done(err)

This comment has been minimized.

Copy link
@mirceaalexandru

mirceaalexandru Jul 14, 2016

Contributor

same as above (done(err))

This comment has been minimized.

Copy link
@Wardormeur

Wardormeur Jul 14, 2016

Member

and console log ;)
I'd even say you could += instead of x = x +1, but that's just me :)

@mirceaalexandru

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2016

One more note: can you please add to this PR some tests? We need tests for all new features.

Thank you

@mirceaalexandru

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2016

Hey @butlerx really sorry, I just realize is your PR actually. 👍

@mirceaalexandru

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2016

It seems to me that after a successful login the counter should be reset to 0.

This seems to be the desired feature. If you try x times to login without success the account is locked. But if I failed 1 time and then login with success it should reset the counter back to 0.

Only x consecutive logins should lock the account.

@butlerx @Wardormeur @mihaidma @thekemkid Thoughts?

@butlerx

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2016

yep thats what i was goign for and a reset to the user password would unlock the account or by calling the unlock. ill get on to the other parts now aswell

@Wardormeur

This comment has been minimized.

Copy link
Member

commented Jul 15, 2016

Very neat to see that going, keep up @butlerx :)

Add brute force protection for login
Account gets locked after number of failed logins thats specified in the
config, default 3. The lock is reset after the user changes the password
or by calling the set_user_lock and changing the value.

@butlerx butlerx force-pushed the butlerx:enchanment/bruteforce-protection branch from 0826cf6 to 2c6e3c9 Jul 15, 2016

@coveralls

This comment has been minimized.

Copy link

commented Jul 15, 2016

Coverage Status

Coverage increased (+0.4%) to 82.649% when pulling 2c6e3c9 on butlerx:enchanment/bruteforce-protection into ab94a7d on senecajs:master.

1 similar comment
@coveralls

This comment has been minimized.

Copy link

commented Jul 15, 2016

Coverage Status

Coverage increased (+0.4%) to 82.649% when pulling 2c6e3c9 on butlerx:enchanment/bruteforce-protection into ab94a7d on senecajs:master.

@butlerx

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2016

tests added and disabled by default
it does require a modification to user so would require a migration for dbs so the version number is up too you

@mcdonnelldean

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2016

LGTM @mirceaalexandru / @mihaidma I'd say major version to be sure?

@mcdonnelldean

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2016

@mihaidma Can we review this and get it in?


Sets the lock on users account

#### Argumants:

This comment has been minimized.

Copy link
@mihaidma

mihaidma Aug 12, 2016

Contributor

typo

@@ -526,6 +533,7 @@ module.exports = function user (options) {
user.name = args.name || ''
user.active = void 0 === args.active ? true : args.active
user.when = new Date().toISOString()
user.lockTry = 0;

This comment has been minimized.

Copy link
@mihaidma

mihaidma Aug 12, 2016

Contributor

linting will fail with this


#### Provides:

* `ok`: true if the account was unloocked or false if the lock was incremented

This comment has been minimized.

Copy link
@mihaidma

mihaidma Aug 12, 2016

Contributor

typo unlooked

})

for(var i=0; i<3; i++) {
test('user/login user test', function (done) {

This comment has been minimized.

Copy link
@mihaidma

mihaidma Aug 12, 2016

Contributor

this loop makes and attempt to login with wrong password for user1 then checks user lock with user2ID? Seems incorrect.
Also these tests should be grouped under the same test.

This comment has been minimized.

Copy link
@mihaidma

mihaidma Aug 12, 2016

Contributor

Test is ok-ish, I got tricked by the test arrangement.

@mihaidma

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2016

I'll merge, the implemented functionality is ok. After merge will do the fixes asked in comments.
i'll not publish yet the plugin, planning to review the set_user_lock 'interface', it might change

@mihaidma mihaidma merged commit 67f3ad8 into senecajs:master Aug 12, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@butlerx butlerx deleted the butlerx:enchanment/bruteforce-protection branch Aug 13, 2016

@mihaidma

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2016

I made the specs for brute force protection: #62
Your implementation remains more or less the same, the changes are:

  • renamed the value stored in db,
  • the set_usor_lock command is not exposed,
  • only the unlock functionality is exposed

I'll work at modifying this implementation.

@Wardormeur

This comment has been minimized.

Copy link
Member

commented Aug 15, 2016

Very nice, thanks both of you :)

@mcdonnelldean

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2016

Good work @butlerx and @mihaidma ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.