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

Adds ability to set an account lockout policy #2601

Merged
merged 2 commits into from
Sep 3, 2016
Merged

Adds ability to set an account lockout policy #2601

merged 2 commits into from
Sep 3, 2016

Conversation

cherukumilli
Copy link
Contributor

@cherukumilli cherukumilli commented Aug 28, 2016

This is a fix for issue #2508

Account lockout policy:

Someone who attempts to use more than a few unsuccessful passwords while trying to log on to your system might be a malicious user who is attempting to determine an account password by trial and error.

Update parse-server to track logon attempts and respond to this type of potential attack by disabling the account for a preset period of time

This setting will have 2 parameters:

Account lockout duration

The Account lockout duration policy setting determines the number of minutes that a locked-out account remains locked out before automatically becoming unlocked. The available range is from 1 through 99,999 minutes.

Account lockout threshold

The Account lockout threshold policy setting determines the number of failed sign-in attempts that will cause a user account to be locked. You can set a value from 1 through 999 failed sign-in attempts.

@cherukumilli
Copy link
Contributor Author

@flovilmart
If you get a few mins then can you please restart travis CI?


describe("Account Lockout Policy: ", () => {

it_exclude_dbs(['postgres'])('account should not be locked even after failed login attempts if account lockout policy is not set', done => {
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the problem with PG?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was under the impression that PG is not ready yet for testing and we are excluding test cases for PG.
Let me give the PG tests a shot and submit another pull request.

BTW: do you know if there is a workflow doc for contributing? i.e., a document that walks the contributor about the setup process, etc...

Copy link
Contributor

Choose a reason for hiding this comment

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

Not really, for PG, you need postgres > 9.4 with POSTGIS extensions loaded, and create the tables (see .travis.yml for the commands)

@codecov-io
Copy link

codecov-io commented Aug 29, 2016

Current coverage is 92.11% (diff: 91.01%)

Merging #2601 into master will increase coverage by 0.02%

@@             master      #2601   diff @@
==========================================
  Files            99        100     +1   
  Lines         12113      12282   +169   
  Methods        1488       1522    +34   
  Messages          0          0          
  Branches       1996       2020    +24   
==========================================
+ Hits          11154      11313   +159   
- Misses          959        969    +10   
  Partials          0          0          

Powered by Codecov. Last update f6516a1...67b5942

@facebook-github-bot
Copy link

@cherukumilli updated the pull request - view changes

@ghost
Copy link

ghost commented Aug 29, 2016

@cherukumilli updated the pull request - view changes

@flovilmart
Copy link
Contributor

Seems that there is an issue with PG on your PR. I'd still like to get it up and running for that DB.

you can debug the 3 failing tests

  • Account Lockout Policy: lock account if failed login attempts are above threshold
  • Account Lockout Policy: lock account for accountPolicy.duration minutes if failed login attempts are above threshold
  • Account Lockout Policy: allow login for locked account after accountPolicy.duration minutes

by replacing the it  with a fit and also setting PARSE_SERVER_LOG_LEVEL=debug before running npm test like that:

PARSE_SERVER_LOG_LEVEL=debug PARSE_SERVER_TEST_DB=postgres npm test

@cherukumilli
Copy link
Contributor Author

@flovilmart
Many thanks for the feedback and setting the environment variables. I will push a PR tomorrow after fixing the code to work with PG.

Is there a Contributor Workflow document that has the instructions like setting the environment variables? I wonder if I should add the instructions you provided above to the existing contributions document (https://github.com/ParsePlatform/parse-server/blob/master/CONTRIBUTING.md). It will be useful for other contributors.

@flovilmart
Copy link
Contributor

@cherukumilli you're 100% right! We should add that to this file. I'll add it

@ghost
Copy link

ghost commented Aug 31, 2016

@cherukumilli updated the pull request - view changes

if (fieldName == '_perishable_token') {
valuesArray.push(object[fieldName].iso);
valuesArray.push(object[fieldName].iso); // TODO: check with @flovilmart about why .iso is needed here
Copy link
Contributor

@flovilmart flovilmart Aug 31, 2016

Choose a reason for hiding this comment

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

this is probably an error...

@cherukumilli
Copy link
Contributor Author

cherukumilli commented Aug 31, 2016

@flovilmart

Updating it to fit in the new AccountLockoutPolicy.spec.js file is causing only tests (10) in this file to be executed by npm test on my pc (locally).

Also I don't see any of the other test cases using fit.

@ghost
Copy link

ghost commented Aug 31, 2016

@cherukumilli updated the pull request - view changes

@ghost
Copy link

ghost commented Aug 31, 2016

@cherukumilli updated the pull request - view changes

@nitrag
Copy link

nitrag commented Aug 31, 2016

Nice PR! Was just thinking about the need for this last night!

@ghost
Copy link

ghost commented Aug 31, 2016

@cherukumilli updated the pull request - view changes

@flovilmart
Copy link
Contributor

Looking good, but change fit for it (otherwise only fit tests run :P)

@ghost
Copy link

ghost commented Sep 2, 2016

@cherukumilli updated the pull request - view changes

@flovilmart
Copy link
Contributor

Very neat feature as always @cherukumilli ! Thanks for your commitment to make it more secure!

@cherukumilli
Copy link
Contributor Author

Thanks @flovilmart

btw: I see warnings and errors like the following in my local setup for PG GeoPoint tests in file ParseGeoPoint.spec.js. Do I need to do anything special to my local setup to get the GeoPoint tests working?

[33mwarn�[39m: Unable to ensure uniqueness for usernames:  code=107, message=schema class name does not revalidate
�[33mwarn�[39m: Unable to ensure uniqueness for user email addresses:  code=107, message=schema class name does not revalidate
F�[33mwarn�[39m: Unable to ensure uniqueness for usernames:  error: type "_SCHEMA" already exists
    at [object Object].Connection.parseE (/Users/diwakarcherukumilli/projects/parse/parse-server/node_modules/pg-promise/node_modules/pg/lib/connection.js:539:11)
    at [object Object].Connection.parseMessage (/Users/diwakarcherukumilli/projects/parse/parse-server/node_modules/pg-promise/node_modules/pg/lib/connection.js:366:17)
    at Socket.<anonymous> (/Users/diwakarcherukumilli/projects/parse/parse-server/node_modules/pg-promise/node_modules/pg/lib/connection.js:105:22)
    at emitOne (events.js:77:13)
    at Socket.emit (events.js:169:7)
    at readableAddChunk (_stream_readable.js:153:18)
    at Socket.Readable.push (_stream_readable.js:111:10)
    at TCP.onread (net.js:531:20)

Please see the attached file for error details after running command npm test ParseGeoPoint.spec.js

ParseGeoPoint.Errors.Errors.zip

@flovilmart
Copy link
Contributor

Seems ok on Travis. Did you enable the postgis extension?

@flovilmart
Copy link
Contributor

Looking very nice!! Thanks again!

@flovilmart flovilmart merged commit 28bd378 into parse-community:master Sep 3, 2016
@cherukumilli cherukumilli deleted the account-lockout-policy branch September 3, 2016 16:17
@cherukumilli
Copy link
Contributor Author

@flovilmart

Is this going to be targeted for v2.3.0?
If so, is there a target date for 2.30?

@flovilmart
Copy link
Contributor

We don't have a target date for the next release, as soon as all Pr are merged for 2.2.19 we'll release.

@ederelk
Copy link

ederelk commented May 11, 2017

Hi all, @cherukumilli . It seems like a locked out account although not having access to the login api still can access other classes/objects with a valid session token. Why don't we remove all the user's session token when the user is locked out?

@ederelk
Copy link

ederelk commented May 11, 2017

@flovilmart @cherukumilli @facebook-github-bot Is there a way to actually lock the user out in cloud code?

@cherukumilli
Copy link
Contributor Author

@ederelk
IMO, allowing valid session tokens access to the api other than login api is acceptable behavior.

For example:
User A logins from System X
User B (unauthorized user) tries to login from System Y unsuccessfully and ends up locking the account.

In the scenario above, IMO,

  1. we should let User A continue to use their valid session token to use the application
  2. lock the account so that User B is unable to login again for the lockout duration

Please let me know if you are seeing a security vulnerability that I am not seeing here.

@ahmedu007
Copy link

@cherukumilli @facebook-github-bot @flavionegrao
Is there any documentation explaining this please?

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.

None yet

7 participants