Change from basic auth to form based login #562

Merged
merged 12 commits into from Nov 10, 2016

Projects

None yet

9 participants

@JeremyPlease
Contributor
JeremyPlease commented Oct 23, 2016 edited

Summary

  • Created page at /login
  • Uses passport and passport-local for authentication.
  • Stores session in cookie to work across servers and server restarts.
  • No configuration changes needed. Still uses users.
  • Redirects unauthenticated requests to /login

This resolves #493

@facebook-github-bot
Collaborator

By analyzing the blame information on this pull request, we identified @dvanwinkle, @flovilmart and @deada92 to be potential reviewers.

@dvanwinkle
Contributor

@JeremyPlease It doesn't look like you added any tests. It'd be really nice to see some, and also make sure that you didn't break any existing tests.

@flovilmart
Collaborator

Do we really need passport/passport-local? I'm kinda opinionated with dependency usage :) and you can just tell me it would take a very long time to implement without it :)

@SaifAlDilaimi
Contributor

I can review it if the tests that will be added pass..

@steven-supersolid
Collaborator

Suggest we remove the editor config and lint rules/setup until a decision has been made on these.

@flovilmart

Thanks @JeremyPlease for that one! Most wanted feature it seems. Can you address the few comments here and there and we'll be able to move forward with that!

+ <Icon width={80} height={80} name='infinity' fill='#093A59' />
+ <form method='post' ref='form' action={this.props.endpoint} className={styles.form}>
+ {
+ //<CSRFInput />
@flovilmart
flovilmart Oct 25, 2016 Collaborator

Can we have CSRF taken care of?

+// App entry point
+
+var path = window.PARSE_DASHBOARD_PATH || '/';
+ReactDOM.render(<Login path={path}/>, document.getElementById('login_mount'));
@flovilmart
flovilmart Oct 25, 2016 Collaborator

We want the trailing \n ;)

@JeremyPlease
Contributor

(sorry it looks like my email reply didn't get posted here)

Thank you everyone for all the feedback! I'll try to respond in detail and provide a meaningful update within the next 24 hours or so.

@flovilmart
Collaborator

No problemo!

@JeremyPlease
Contributor

Thank you everyone for all the feedback! I'll try to respond in detail and
provide a meaningful update within the next 24 hours or so.

@flovilmart
Collaborator

@JeremyPlease seems that your email came in :)

@facebook-github-bot
Collaborator

@JeremyPlease updated the pull request - view changes

@JeremyPlease
Contributor

Alrighty. Finally some time to make some updates!

@dvanwinkle Current tests pass and I added a couple more. The overall repo definitely needs more tests though.

@steven-supersolid I'll move eslint/editorconfig to a new PR.

@flovilmart:
• We could implement without passport, but it would take a bit of time that I don't think I'll have soon. Besides passport and passport-local have proven stable in my other work, are pretty light, and have nearly no external dependencies :)
• I added CSRF (pretty much the same way parse.com has it implemented).
• I cleaned up some code.


Please let me know if anyone has any other feedback, concerns, or requests.

Thanks again!

@flovilmart
Collaborator

I'll conduct some local testing, as that is a security feature :)

@dvanwinkle
Contributor
dvanwinkle commented Oct 31, 2016 edited

At least from a code perspective, 👍. Haven't ran locally though.

@JeremyPlease
Contributor

Anyone interested in testing locally, here are the steps:

  1. git clone -b form-login git@github.com:JeremyPlease/parse-dashboard.git
  2. cd parse-dashboard
  3. npm install
  4. Edit file at ./Parse-Dashboard/parse-dashboard-config.json to have "users" defined and apps pointed to a real Parse server (example config).
  5. npm run dev
  6. Open browser to http://localhost:4040
@JeremyPlease
Contributor

bump

Anyone have any questions or concerns?

@flovilmart anything you'd like to see changed?

@mario
mario commented Nov 9, 2016

Would love to see this merged if you're all ok with it.

@flovilmart
Collaborator

Sorry it's taking forever...

@SaifAlDilaimi
Contributor

any eta for the merge @flovilmart ?

@flovilmart
Collaborator

All changes look good, I'm a bit scared of merging but that should be fine :)

@steven-supersolid
Collaborator

Looks like there are a couple of existing tests for authentication and these still pass, plus a few tests added. So as long as we trust those tests then I think it should be OK too.

@steven-supersolid
Collaborator

One small optional change - since the review was created we're encouraging contributors to update the changelog with their changes to reduce overhead.

@flovilmart
Collaborator

I'll take care of adding it to the changelog for the next release ;)

@flovilmart flovilmart merged commit be9f498 into ParsePlatform:master Nov 10, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@yinanfang yinanfang referenced this pull request in yinanfang/DBDCapital-Node Nov 16, 2016
Closed

Parse dashboard form based login #30

@gowridev

I hope, this logic works well with parse server example . Can we extend this logic to apply authorization on parse classes?

@vkeepe
vkeepe commented Dec 4, 2016

Getting this error:

Error: Illegal arguments: undefined, string at Error (native) at Object.bcrypt.compareSync (/usr/lib/node_modules/parse-dashboard/node_modules/bcryptjs/dist/bcrypt.js:230:19) at userToTest.validUsers.validUsers.find.user (/usr/lib/node_modules/parse-dashboard/Parse-Dashboard/Authentication.js:91:65) at Array.find (native) at Authentication.authenticate (/usr/lib/node_modules/parse-dashboard/Parse-Dashboard/Authentication.js:88:21) at /usr/lib/node_modules/parse-dashboard/Parse-Dashboard/Authentication.js:40:21 at pass (/usr/lib/node_modules/parse-dashboard/node_modules/passport/lib/authenticator.js:347:9) at Authenticator.deserializeUser (/usr/lib/node_modules/parse-dashboard/node_modules/passport/lib/authenticator.js:352:5) at SessionStrategy.authenticate (/usr/lib/node_modules/parse-dashboard/node_modules/passport/lib/strategies/session.js:53:28) at attempt (/usr/lib/node_modules/parse-dashboard/node_modules/passport/lib/middleware/authenticate.js:348:16)

@JeremyPlease
Contributor

@vkeepe Check out issue #609

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment