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

Added hashing of passwords for its storage in the database #113

Merged
merged 7 commits into from
Feb 15, 2019

Conversation

himanshukumar660
Copy link
Contributor

Hey @yochannah . Just added a hashing mechanism so that the password could be stored in its hashed form rather than a plain text. Please review it and suggest any modification, if any. Thanks :)

@yochannah
Copy link
Member

hey @himanshukumar660! I love the prompts you've added in the createUser.js! I'm getting an error though - any suggestions?

  1. git pull your branch
  2. npm install (to get bcrypt etc.)
  3. node createUser.js
Please enter the username : yo
Please enter the password : SOMEPASSWORD
node: symbol lookup error: /home/FILESERVER5/micklem/yy406/projects/intermine-registry/node_modules/bcrypt/lib/binding/bcrypt_lib.node: undefined symbol: _ZN4node19GetCurrentEventLoopEPN2v87IsolateE

@himanshukumar660
Copy link
Contributor Author

himanshukumar660 commented Jan 23, 2019

@yochannah Have you tried the above script after running the mongodb server.

@yochannah
Copy link
Member

yochannah commented Jan 23, 2019

@himanshukumar660 yes!

export MONGODB_URL="mongodb://127.0.0.1/registry"
mongod

When I run createUser.js, I can see the connection attempt in the mongo logs:

2019-01-23T13:23:56.848+0000 I NETWORK  [thread1] connection accepted from 127.0.0.1:37406 #4 (2 connections now open)
2019-01-23T13:23:56.860+0000 I NETWORK  [conn4] received client metadata from 127.0.0.1:37406 conn4: { driver: { name: "nodejs", version: "2.2.27" }, os: { type: "Linux", name: "linux", architecture: "x64", version: "4.4.0-141-generic" }, platform: "Node.js v9.2.0, LE, mongodb-core: 2.1.11" }
2019-01-23T13:23:57.224+0000 I INDEX    [conn4] build index on: registry.users properties: { v: 2, unique: true, key: { user: 1 }, name: "user_1", ns: "registry.users", background: true }
2019-01-23T13:23:57.224+0000 I INDEX    [conn4] build index done.  scanned 0 total records. 0 secs
2019-01-23T13:23:57.224+0000 I COMMAND  [conn4] command registry.$cmd command: createIndexes { createIndexes: "users", indexes: [ { name: "user_1", key: { user: 1 }, unique: true, background: true } ], writeConcern: { w: 1 } } numYields:0 reslen:113 locks:{ Global: { acquireCount: { r: 1, w: 1 } }, Database: { acquireCount: { w: 1, W: 2 } }, Collection: { acquireCount: { w: 1 } } } protocol:op_query 355ms
2019-01-23T13:24:27.708+0000 I -        [conn4] end connection 127.0.0.1:37406 (2 connections now open)

and the users collection is created but there is nothing inside itL

> show collections
test
users
> db.users.find({})

@himanshukumar660
Copy link
Contributor Author

himanshukumar660 commented Jan 23, 2019

@yochannah I think this issue has been addressed and solved here

@yochannah
Copy link
Member

@himanshukumar660 thanks, changing it to version 3.0.0 fixed the issue. Could you update package.json in your PR to specify 3.0.0?

@himanshukumar660
Copy link
Contributor Author

@yochannah Please Check if everything is fine.

@yochannah
Copy link
Member

@himanshukumar660 you were right - the bcrypt version rollback fixed things.

I think we still need a tweak to initRegistry.js, though - now, in order to run it I need to paste the hashed password into the password field, not the plaintext.

@himanshukumar660
Copy link
Contributor Author

@yochannah Uncommited last 3 commits because I had to change the entire logic. This commit solves the problem related to the tweak required for running initRegistry.js correctly.

@yochannah yochannah self-assigned this Feb 15, 2019
@yochannah
Copy link
Member

yochannah commented Feb 15, 2019

@himanshukumar660 this is looking good! There is one small bit that needs to be fixed, but otherwise everything I tested worked nicely.

  1. pull code from your fork, dev branch
  2. node createUser.js >> all worked! 👍
    • The password is hashed when I check in mongo!
  3. Started the server with npm start and ran some checks:
    • successfully logged in with the password and user from step 2 🎉

    • deleted an instance successfully

    • tried to press the yellow synchronise all button (bottom right) - this gives a popup asking for re-auth,
      image but does work. I'd prefer no pop-up but that's not a big thing. Oddly, it didn't ask for this the next time I tried it...

    • tried to add instance. 😿 This caused the server to error out and stop running:

    • tried to update. This also failed, I assume for the same reason the add instance didn't work. Stack trace:

POST /service/instances 401 64.820 ms - -
undefined:1
Unauthorized
^

SyntaxError: Unexpected token U in JSON at position 0
    at JSON.parse (<anonymous>)
    at Request._callback (/home/FILESERVER5/micklem/yy406/projects/intermine-registry/routes/index.js:216:21)
    at Request.self.callback (/home/FILESERVER5/micklem/yy406/projects/intermine-registry/node_modules/request/request.js:186:22)
    at Request.emit (events.js:189:13)
    at Request.<anonymous> (/home/FILESERVER5/micklem/yy406/projects/intermine-registry/node_modules/request/request.js:1163:10)
    at Request.emit (events.js:189:13)
    at IncomingMessage.<anonymous> (/home/FILESERVER5/micklem/yy406/projects/intermine-registry/node_modules/request/request.js:1085:12)
    at Object.onceWrapper (events.js:277:13)
    at IncomingMessage.emit (events.js:194:15)
    at endReadableNT (_stream_readable.js:1103:12)
    at process._tickCallback (internal/process/next_tick.js:63:19)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] start: `node ./bin/www`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the [email protected] start script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

@yochannah
Copy link
Member

apparently this is working nicely on @himanshukumar660's computer 😆 it might be handy to have someone else try this out as well.

@yochannah
Copy link
Member

okay, it's all working nicely now - thanks so much @himanshukumar660 for all your hard work on this - it's been a bit of a journey getting there! 🏆

@yochannah yochannah merged commit 3341fff into intermine:dev Feb 15, 2019
@yochannah
Copy link
Member

One small comment might be to try avoiding force-pushes in the future if possible. Even so, nice job :)

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.

2 participants