[UNRESOLVED] JWT Authentication #1163

trainerbill · 2016-01-26T22:34:25Z

I took a stab at redoing the Authentication in favor of JWT. I am using passport-jwt to leverage the existing authentication structure. The Authentication service now holds a user and token and uses localStorage to store the token. I am only storing the token. If you refresh the page the AuthService will look for a token in local storage and if it exists hits the users/me route to get the user. The auth service also sets the $http header for Authorization if the token exists so you don't have to add it to other services. Currently working:

Login
Sign Up
Persistent on Refresh and users/me route
Edit Profile

I want to get buy in from contributors before going any further. This PR will touch a lot of files and I don't want to get all the way done and someone has a problem and this ends up in PR hell. If I can get some feedback and +1's I will have this done by Friday this week. Also would like a volunteer to help fix the tests when this is done and ready. Thanks to @mleanos for the server side kick start.

@mleanos @lirantal @ilanbiala @codydaig

Fixes #389
Fixes #1086

ilanbiala · 2016-01-27T00:17:21Z

config/env/default.js

+  },
+  jwt: {
+    secret: process.env.TOKEN_AUTH_SECRET || 'M3@N_R0CK5!',
+    expiresInSeconds: process.env.TOKEN_AUTH_EXPIRES_SECONDS || 200 * 60 * 60 // 200 hours


Why 200 hours?

ilanbiala · 2016-01-27T00:20:54Z

Did a quick read through, didn't comment on everything. @trainerbill can you give the pros and cons of JWT and any limitations of this implementation?

mleanos · 2016-01-27T02:28:07Z

@trainerbill This looks pretty good. I'm gonna pull it down and play around with it.

The other day, I started looking at using the passport-http-bearer module. I think the module you're using is probably better, but what are the advantages of this one over the bearer?

One thing I started playing around with, mostly in thought, was to think of MEAN Token-auth as just another Provider. If we use a passport module for authenticating the JWT Token, can we merely treat it as a new Provider, and have it behave the same (just as Google, Twitter, GitHub, etc)?

Think of having a MEAN.JS logo mixed in with the other providers at time of sign in or sign up. The project user could decide if they want to allow either authentication method, or both.

It think it could allow the project to be easily switched to and from JWT & Session based authentication. Any thoughts on that approach?

trainerbill · 2016-01-27T02:54:48Z

@ilanbiala Thanks for the feedback. This is more of a rough draft than a code review so don't worry about new lines etc. I am just looking for an overall discussion on the topic. My goal is to get away from the current AuthService that reads from window.user and JWT is kinda the norm right now. JWT is becoming the defacto standard for authenticating REST services. The biggest pro is that it will work with mobile. Session based auth will not. A mobile app could be coded to hit the auth/sigin URL store the token and then make additional calls to the MEAN backend. Most applications are going away from Session based authentications as they are not mobile friendly. I am sure there are other benefits but those are the 2 I can think of off hand. With JWT the client holds the keys instead of the server. As for limitations, I am not aware of any. Perfomance should be the same as it is the same amount of DB calls, just an added token decode which isn't much.

@mleanos The module you linked looks more like an oAuth strategy. I like the passport-jwt strategy as it works with our current auth procedure and you can make it route based. So if I have a public endpoint I don't do the passport.authenticate. How I implemented it really is just another provider.

https://github.com/trainerbill/mean/blob/53302087a0fc1d2d38a867ca342d707786ca2bd2/modules/users/server/config/strategies/jwt.js

As far as switching between session and JWT, I don't see the point. If you look at how passport and express currently work, only the _id is stored and there is a DB call to get the rest of the info:

https://github.com/meanjs/mean/blob/master/modules/users/server/config/users.server.config.js#L21

So the JWT approach is the same DB calls, as you decode the token, get the ID and find the user. Plus it has the added benefit of being client independent. Mobile devices, and any other client would be able to authenticate and use the REST interfaces.

For your question on if we can treat it as another provider, the answer would probably be no. You still need a user/pass combo to authenticate a user and the token is merely a reflection of that authentication. The "local" strategy now just checks the user/pass combo and returns the user info and the token. This will be how providers work when I get to them. The same steps as far as auth, but when it comes back we send a token to the AuthService and everything works as normal.

Like the discussion, lets keep it up. Want everything ironed out before doing more work!

codydaig · 2016-01-27T18:26:45Z

I'm totally onboard with this PR. I like token auth vs. session auth. It makes scaling the app to more than 1 server much easier.

trainerbill · 2016-01-27T19:07:04Z

@codydaig awesome, I will continue to work on this then. I think it will solve a lot of the other open issues as well

What do you all think of changing up the routes a bit:

https://github.com/trainerbill/mean/blob/JWTAuthorization/modules/users/server/routes/users.server.routes.js

Pros:

Can do router.all() and hit all of the routes on the specific router. Like the JWT auth
Currently using app.param like we are doing can cause collisions if a submodule wants to use say :userId
Keeps the route logic contained in the module instead of the app.

Cons:

Its different
have to require express and create a new router.
Probably going to need separate routes for admin. /api/admin/users etc.

I personally like it, but want some feedback.

codydaig · 2016-01-27T19:16:49Z

I personally am not usually a fan of the express router because it usually adds unnecessary complexity. However, I think it could be of benefit here to help keep things more modular. I think that is a huge undertaking though because I would like to keep it consistent throughout the framework.

trainerbill · 2016-01-27T19:18:39Z

@codydaig Yeah I know... but this whole PR is gonna be a huge undertaking... I know in one of my apps the app.param collision screwed me.

codydaig · 2016-01-27T19:20:54Z

@trainerbill Doing things in smaller PRs is what I usually like to go for. It makes it easier to review, discuss, and get merged. My vote would be to have the router refactor be its own PR.

trainerbill · 2016-01-27T19:22:06Z

@codydaig True. However adding passport.authenticate(jwt) to every route is going to be kind of a pain.

ilanbiala · 2016-01-27T19:53:35Z

@trainerbill if possible, @codydaig's suggestion of keeping changes in smaller PRs will definitely speed up the whole conversion process. @mleanos has a PR (#1121) that seems to also implement JWT instead of sessions and his is slightly smaller in impact. @trainerbill can you and @mleanos can combine PRs to create a better one that has less impact and incorporates the changes of both?

trainerbill · 2016-01-27T21:05:29Z

@ilanbiala I understand that smaller PR's are easier and will try to keep it as small as possible, but this is a large change. I have spoken with @mleanos and offered for him to assist. His current PR isn't close to being complete, and will need 90% of the changes in this PR.

On the plus side I am almost done. I need some help on the chat module. Anyone familiar enough with the module and socket.io to make the auth changes? I found this article but I am not at all familiar with this module or socket.io

http://wmyers.github.io/technical/nodejs/Simple-JWT-auth-for-SocketIO/

mleanos · 2016-01-27T21:10:11Z

@trainerbill I can help out with the Socket.IO changes

My intentions with my PR was to keep the server-side implementation independent from the client-side. Really those two things are separate. It looks like most of your server-side changes are the same that I implemented in my PR. My suggestion from the beginning of working on this, was to work on a shared branch; with client-side & server-side changes made independent of one another.

So shall I submit PR's to your branch?

trainerbill · 2016-01-27T21:27:14Z

@mleanos Absolutely. I don't think you can really keep client/server in independent PR's. In order to make sure that it is working you have to remove the session authentication. Otherwise the app will continue to use session and JWT so you really don't know if the changes you are making are working as expected with JWT.

I will be AWK till a little later tonight and will check back in. Can some of you pull the branch and take it for a spin? It should be in a semi workable state. Whats currently working:

Authentication. Signin/Signup/Signout
User Routes. Edit Profile, Password, Picture
Article Module. Add/Create/Update/List
User Admin
Social Login
Client/Server Tests
Password Reset
Chat Module

Whats not working:

~~Chat Module~~
~~Possibly user admin. Not sure on this if someone wants to check and see~~
~~Signout bug. When on a user profile and sign out it should be forbidden. Possibly just redirect to home on signout?~~
~~Social Accounts. I need some client/secrets if anyone has them.~~
~~Tests. REally need help here.~~
~~Password Reset~~
New tests for client Auth service

@mleanos If you are working on specific items let me know and I will stay away from them.

Thanks Everyone!

codydaig · 2016-01-27T23:47:32Z

@trainerbill socketio-jwt is what I've used in the past.

jameslegue · 2016-01-28T02:55:35Z

+1 on this. Have been waiting for token based auth for awhile now (main use case is sharing of REST APIs with mobile).

trainerbill · 2016-01-28T17:58:02Z

Can someone look into CSRF with JWT? I don't think we need the CSRF PR's after this goes in.

trainerbill · 2016-01-28T18:20:58Z

Anyone have any thoughts on the following file:

config/lib/jwtAuthentication.js

I think it should go in the users module since that is where the authentication is taking place. Just not sure where to put it, controllers?, and what to name it.

ilanbiala · 2016-01-28T19:30:06Z

config/lib/jwtAuthentication.js

+      var token = jwt.sign(payload, config.jwt.secret, options);
+
+      resolve(token);
+


extra newline

trainerbill · 2016-01-29T17:50:55Z

All. I have this PR about ready. I will do the Password Reset this afternoon:

@codydaig I made a frame of the auth service test in modules/users/tests/client/authentication.client.service.tests.js. The describes and its are there but I don't know enough about the client side tests to finish this. Can you do it or find someone who can?
@mleanos How is the chat module coming along?
@ilanbiala @rhutchison You can do a first round code review now if you want to.

The coverall tests decreased because it looks like there is a duplicate in the user tests

ilanbiala · 2016-01-29T18:13:40Z

config/env/development.js

@@ -60,8 +60,8 @@ module.exports = {
    callbackURL: '/api/auth/github/callback'
  },
  paypal: {
-    clientID: process.env.PAYPAL_ID || 'CLIENT_ID',
-    clientSecret: process.env.PAYPAL_SECRET || 'CLIENT_SECRET',
+    clientID: process.env.PAYPAL_ID || 'AVWxUS4uQVfXAzKdCQKrIkczdPxixgKQ-SA7Ewodn9UmLOjHSCkftB6D9Wj_lVjRZpF9VCanp7thMCeG',


These probably shouldn't be here

lol. good thing that wasn't production...

trendzetter · 2016-02-14T22:29:33Z

@trainerbill I managed to run the branch. As soon as nodemon has started got this error:

/meanTB/node_modules/mongoose/node_modules/mongodb/lib/utils.js:98
    process.nextTick(function() { throw err; });
                                        ^
TypeError: undefined is not a function
    at allFailed (/meanTB/node_modules/passport/lib/middleware/authenticate.js:154:11)
    at attempt (/meanTB/node_modules/passport/lib/middleware/authenticate.js:160:28)
    at JwtStrategy.strategy.fail (/meanTB/node_modules/passport/lib/middleware/authenticate.js:277:9)
    at JwtStrategy.authenticate (/meanTB/node_modules/passport-jwt/lib/strategy.js:114:21)
    at attempt (/meanTB/node_modules/passport/lib/middleware/authenticate.js:341:16)
    at authenticate (/meanTB/node_modules/passport/lib/middleware/authenticate.js:342:7)
    at /meanTB/config/lib/socket.io.js:88:59
..

Am I doing it wrong or did I find an issue to resolve?

trainerbill · 2016-02-15T15:09:44Z

@trendzetter Tests still pass so this should not be an issue. MY guess is that you have a dependency issue. Pull the branch and make sure you NPM Install

trendzetter · 2016-02-15T18:38:38Z

You are correct. I forgot in all the rush to get this going..

trendzetter · 2016-02-18T22:47:16Z

I tested this and hope to see this merged. It would improve greatly on security.
There is proposed to break this pr up in pieces. @lirantal Maybe you could do a suggestion? I propose to spit the social network accounts into a separate pr because that part still seems to have some bugs while the rest of the functionality is working fully now.

lirantal · 2016-02-20T11:24:22Z

@trainerbill ok, a lot has been going on in this PR so I'd need to free up some time to get it resolved in several PRs.

zargold · 2016-02-21T20:36:02Z

config/env/default.js

@@ -37,5 +37,12 @@ module.exports = {
        fileSize: 1*1024*1024 // Max file size in bytes (1 MB)
      }
    }
+  },


jwt: { secret: process.env.TOKEN_AUTH_SECRET || 'M3@N_R0CK5!', options: { //Anything From https://www.npmjs.com/package/jsonwebtoken expiresIn: process.env.TOKEN_EXPIRES || '1d' } } Basically not a big deal but gives a formatting warning.

zargold · 2016-02-22T17:35:19Z

@trainerbill maybe you know if you tested for this?/If I messed it up ?
Seems like even after logging out (read: JWT token is gone) session storage is still there. Not sure if I messed up his copy though so maybe someone should test more thoroughly.

Cookies are there probably how it knew my name and I was still able to post.
And yes if I delete cookie then try again I can't create/read/update 403 forbidden. AKA you can post with just JWT or just cookie, and cookie is not being deleted at least for me.

trendzetter · 2016-02-22T21:05:43Z

I am getting forbidden when trying to access articles/create when signed out.

trendzetter · 2016-02-22T21:11:23Z

If I use 2 tabs and log out in one I can continue my session with my token in the other tab as long as I do not refresh the page.

zargold · 2016-02-23T19:05:11Z

config/lib/socket.io.js

@@ -85,7 +85,7 @@ module.exports = function (app, db) {



The stuff above here talks about sessionSecret sessionId not sure if that's need/was causing my problem.

I removed the above code (everything about Session and cookies and everything worked just fine! Except the tests I guess... so might have to change those

trainerbill · 2016-02-23T20:07:19Z

FYI one issue is that signout has to kill the token in the $http header.

0xNacho · 2016-02-24T09:30:15Z

@trainerbill I've just read your comment about CSRF protection. If meanjs no longer use cookies, you don't need to worry about cross requests. In other words, with JWT you don't need CSRF protection.
Actually, JWT token replaces CSRF security token in each request.

http://stackoverflow.com/questions/21357182/csrf-token-necessary-when-using-stateless-sessionless-authentication

jameslegue · 2016-03-01T01:40:37Z

@trainerbill Saw your comment above you are working on a fork with JWTAuth and Material design. Any chance you're willing to share that branch? It's exactly what I'm after.

trainerbill · 2016-03-01T02:05:51Z

@jameslegue I do. Hit me up on gitter

ilanbiala · 2016-03-01T03:43:02Z

Let's rebase this and finalize everything. @mleanos @trainerbill @codydaig can we work out the rest of the kinks?

avingochea · 2016-04-07T17:55:58Z

Hey guys, I think I've found an issue. req.user is undefined in articles.server.policy.js so it always defaults to 'guest' role.

avingochea · 2016-04-07T18:47:06Z

Fixed it by adding this to the articles.server.routs.js file just before the routes are declared

// Set JWT Auth for all article routes router.all('*', passport.authenticate('jwt', { session: false }));

avingochea · 2016-04-07T18:57:21Z

Just realized that the get route intentionally doesn't have authentication. What do you guys think about authenticating if there's a token, but continuing if there isn't. This would leave the policy to decide whether the route should be accessible to guests

mleanos · 2016-04-09T19:25:01Z

@avingochea IMO, we should attempt to authenticate on each request. Thus, the responsibility of API access will solely reside in the server policies.

trainerbill · 2016-04-09T19:32:00Z

That adds to much logic to the policy. Just attached the passport Middleware to the route that you want to secure like it's designed to do.

lirantal · 2016-09-08T16:33:37Z

@trainerbill this by far adds so much code that I'm incapable of code reviewing that.
We can take a look at this in a new PR if there's an easier approach?

This was referenced Jan 26, 2016

(fix AuthenticationService) User /me server route instead of $window #1155

Closed

[WIP] feat(users): Initial server-side Token Authorization #1121

Closed

ilanbiala reviewed Jan 27, 2016
View reviewed changes

codydaig added the enhance:feature label Jan 27, 2016

codydaig added this to the 0.5.0 milestone Jan 27, 2016

codydaig self-assigned this Jan 27, 2016

trainerbill changed the title ~~JWT Authentication~~ [WIP] feat(JWT Authentication) Jan 28, 2016

ilanbiala reviewed Jan 28, 2016
View reviewed changes

config/lib/jwtAuthentication.js

var token = jwt.sign(payload, config.jwt.secret, options);

resolve(token);

Copy link

Member

ilanbiala Jan 28, 2016

Choose a reason for hiding this comment

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

extra newline

ilanbiala reviewed Jan 29, 2016
View reviewed changes

jloveland mentioned this pull request Feb 21, 2016

Ionic app gives 403 forbidden error with meanjs as server #1212

Closed

zargold reviewed Feb 21, 2016
View reviewed changes

zargold reviewed Feb 23, 2016
View reviewed changes

mleanos mentioned this pull request Apr 10, 2016

JWT Authentication simplified #1300

Open

simison mentioned this pull request Jul 14, 2016

How to authenticate mobile clients? Trustroots/trustroots#368

Closed

lirantal closed this Sep 8, 2016

lirantal changed the title ~~feat(core): JWT Authentication~~ [UNRESOLVED] JWT Authentication Sep 8, 2016

mleanos modified the milestones: Backlog, 0.5.0, q Oct 2, 2016

		var token = jwt.sign(payload, config.jwt.secret, options);

		resolve(token);

[UNRESOLVED] JWT Authentication #1163

[UNRESOLVED] JWT Authentication #1163

Conversation

trainerbill commented Jan 26, 2016

ilanbiala Jan 27, 2016

Choose a reason for hiding this comment

ilanbiala commented Jan 27, 2016

mleanos commented Jan 27, 2016

trainerbill commented Jan 27, 2016

codydaig commented Jan 27, 2016

trainerbill commented Jan 27, 2016

codydaig commented Jan 27, 2016

trainerbill commented Jan 27, 2016

codydaig commented Jan 27, 2016

trainerbill commented Jan 27, 2016

ilanbiala commented Jan 27, 2016

trainerbill commented Jan 27, 2016

mleanos commented Jan 27, 2016

trainerbill commented Jan 27, 2016

codydaig commented Jan 27, 2016

jameslegue commented Jan 28, 2016

trainerbill commented Jan 28, 2016

trainerbill commented Jan 28, 2016

ilanbiala Jan 28, 2016

Choose a reason for hiding this comment

trainerbill commented Jan 29, 2016

ilanbiala Jan 29, 2016

Choose a reason for hiding this comment

trainerbill Jan 29, 2016

Choose a reason for hiding this comment

trendzetter commented Feb 14, 2016

trainerbill commented Feb 15, 2016

trendzetter commented Feb 15, 2016

trendzetter commented Feb 18, 2016

lirantal commented Feb 20, 2016

zargold Feb 21, 2016

Choose a reason for hiding this comment

zargold commented Feb 22, 2016

trendzetter commented Feb 22, 2016

trendzetter commented Feb 22, 2016

zargold Feb 23, 2016

Choose a reason for hiding this comment

zargold Feb 24, 2016

Choose a reason for hiding this comment

trainerbill commented Feb 23, 2016

0xNacho commented Feb 24, 2016

jameslegue commented Mar 1, 2016

trainerbill commented Mar 1, 2016

ilanbiala commented Mar 1, 2016

avingochea commented Apr 7, 2016

avingochea commented Apr 7, 2016

avingochea commented Apr 7, 2016

mleanos commented Apr 9, 2016

trainerbill commented Apr 9, 2016

lirantal commented Sep 8, 2016