Change from basic auth to form based login #562

JeremyPlease · 2016-10-23T19:56:32Z

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

…session

facebook-github-bot · 2016-10-23T19:57:07Z

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

dvanwinkle · 2016-10-23T22:02:58Z

@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 · 2016-10-23T22:08:12Z

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 · 2016-10-24T07:53:32Z

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

steven-supersolid · 2016-10-24T08:32:18Z

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!

flovilmart · 2016-10-23T22:09:35Z

src/components/LoginForm/LoginForm.react.js

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


Can we have CSRF taken care of?

flovilmart · 2016-10-23T22:10:29Z

src/login/index.js

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


We want the trailing \n ;)

JeremyPlease · 2016-10-25T14:52:42Z

(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 · 2016-10-25T16:15:53Z

No problemo!

JeremyPlease · 2016-10-25T18:23:37Z

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 · 2016-10-26T20:43:10Z

@JeremyPlease seems that your email came in :)

This reverts commit 008092b.

…components

facebook-github-bot · 2016-10-29T15:41:00Z

@JeremyPlease updated the pull request - view changes

JeremyPlease · 2016-10-29T15:48:05Z

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 · 2016-10-29T15:56:22Z

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

dvanwinkle · 2016-10-31T00:06:29Z

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

JeremyPlease · 2016-10-31T17:23:54Z

Anyone interested in testing locally, here are the steps:

git clone -b form-login [email protected]:JeremyPlease/parse-dashboard.git
cd parse-dashboard
npm install
Edit file at ./Parse-Dashboard/parse-dashboard-config.json to have "users" defined and apps pointed to a real Parse server (example config).
npm run dev
Open browser to http://localhost:4040

JeremyPlease · 2016-11-04T23:00:22Z

bump

Anyone have any questions or concerns?

@flovilmart anything you'd like to see changed?

mario · 2016-11-09T14:05:40Z

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

flovilmart · 2016-11-09T17:55:06Z

Sorry it's taking forever...

SaifAlDilaimi · 2016-11-10T08:25:02Z

any eta for the merge @flovilmart ?

flovilmart · 2016-11-10T14:29:08Z

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

steven-supersolid · 2016-11-10T14:37:28Z

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 · 2016-11-10T14:43:25Z

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

flovilmart · 2016-11-10T15:03:33Z

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

gowridev · 2016-11-21T10:08:49Z

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

vkeepe · 2016-12-04T02:34:41Z

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 · 2016-12-04T21:36:53Z

@vkeepe Check out issue #609

JeremyPlease added 8 commits October 23, 2016 11:05

Restore login related files (with some tweaks) from 9afc03b

681dc27

Add eslint and editorconfig for linting and consistency

008092b

Clean up the login page and unused scss

40f6ca0

Update server to respond to use passport users and server /login

96ef6cf

Implement form based login with passport, passport-local, and cookie-…

858344c

…session

Add log out button to footer

284eaf3

Disable CSRFInput for now as it isn't properly implemented

501cb6d

Remove unused basic-auth module

ccd4921

facebook-github-bot added GH Review: review-needed labels Oct 23, 2016

facebook-github-bot added the CLA Signed label Oct 23, 2016

flovilmart suggested changes Oct 25, 2016

View reviewed changes

JeremyPlease added 4 commits October 29, 2016 10:29

Revert "Add eslint and editorconfig for linting and consistency"

7f8d095

This reverts commit 008092b.

Add two more test cases for Authentication.authenticate

e0068e4

Code clean up

b635765

Add proper CSRF handling to login through express and existing React …

014ed41

…components

flovilmart approved these changes Nov 9, 2016

View reviewed changes

flovilmart merged commit be9f498 into parse-community:master Nov 10, 2016

yinanfang mentioned this pull request Nov 16, 2016

Parse dashboard form based login yinanfang/DBDCapital-Node#30

Closed

Uh oh!

Change from basic auth to form based login #562

Change from basic auth to form based login #562

Uh oh!

Conversation

JeremyPlease commented Oct 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

facebook-github-bot commented Oct 23, 2016

Uh oh!

dvanwinkle commented Oct 23, 2016

Uh oh!

flovilmart commented Oct 23, 2016

Uh oh!

SaifAlDilaimi commented Oct 24, 2016

Uh oh!

steven-supersolid commented Oct 24, 2016

Uh oh!

flovilmart left a comment

Choose a reason for hiding this comment

Uh oh!

flovilmart Oct 23, 2016

Choose a reason for hiding this comment

Uh oh!

flovilmart Oct 23, 2016

Choose a reason for hiding this comment

Uh oh!

JeremyPlease commented Oct 25, 2016

Uh oh!

flovilmart commented Oct 25, 2016

Uh oh!

JeremyPlease commented Oct 25, 2016

Uh oh!

flovilmart commented Oct 26, 2016

Uh oh!

facebook-github-bot commented Oct 29, 2016

Uh oh!

JeremyPlease commented Oct 29, 2016

Uh oh!

flovilmart commented Oct 29, 2016

Uh oh!

dvanwinkle commented Oct 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JeremyPlease commented Oct 31, 2016

Uh oh!

JeremyPlease commented Nov 4, 2016

Uh oh!

mario commented Nov 9, 2016

Uh oh!

flovilmart commented Nov 9, 2016

Uh oh!

SaifAlDilaimi commented Nov 10, 2016

Uh oh!

flovilmart commented Nov 10, 2016

Uh oh!

steven-supersolid commented Nov 10, 2016

Uh oh!

steven-supersolid commented Nov 10, 2016

Uh oh!

flovilmart commented Nov 10, 2016

Uh oh!

gowridev commented Nov 21, 2016

Uh oh!

vkeepe commented Dec 4, 2016

Uh oh!

JeremyPlease commented Dec 4, 2016

Uh oh!

Uh oh!

JeremyPlease commented Oct 23, 2016 •

edited

Loading

dvanwinkle commented Oct 31, 2016 •

edited

Loading