JWT Authentication simplified #1300

mleanos · 2016-04-10T23:44:18Z

I've simplified the JWT Authentication implementation from #1163. Everything seems to be working correctly, but this definitely needs more eyes & manual testing.

I've managed to keep the changes to a minimum, so that it's easier to review. Also, I added separate commits for the changes to the test coverage.

Let me know how it looks, or if there's something missing/broken.

mleanos · 2016-04-10T23:54:36Z

It looks like the Social Accounts OAuth isn't working. Does someone want to take a look at that as well?

Anyone, feel free to submit PR's to my branch if you'd like to contribute here.

coveralls · 2016-04-10T23:56:16Z

Coverage increased (+1.1%) to 71.717% when pulling ae9955734891a0a796dceec5b2d44ee598fedbb5 on mleanos:feature/JWT-Auth-Lib into bc0b4a6 on meanjs:master.

ilanbiala · 2016-04-11T00:05:08Z

config/lib/socket.io.js

  passport = require('passport'),
  socketio = require('socket.io'),
-  session = require('express-session'),
-  MongoStore = require('connect-mongo')(session);
+  ExtractJwt = require('passport-jwt').ExtractJwt;


Where do you use this?

I'm not. It was left over from my attempts to get SocketIO working with the JWT. I'll remove this.

ilanbiala · 2016-04-11T00:08:08Z

Looks like a good start. I'll look again tomorrow afernoon/Tuesday because I have some things I need to take care of for tomorrow.

coveralls · 2016-04-12T02:07:17Z

Coverage increased (+1.05%) to 71.669% when pulling bf78f16ebb8641a1c4f8c7bb34d743a21bd23766 on mleanos:feature/JWT-Auth-Lib into eaead7a on meanjs:master.

mleanos · 2016-04-12T03:46:26Z

@ilanbiala @simison I modified the SocketIO configuration to authenticate using the JWT Token. Thanks for leading me to the socketio-auth package @simison. I like their implementation, and it's super simple. Now the SocketIO configuration is much simpler :)

However, the client-side SocketIO tests are failing. If someone wants to help out here, that would be much appreciated.

coveralls · 2016-04-13T08:20:19Z

Coverage increased (+1.05%) to 71.669% when pulling b14a5bf5b657e05d62899f752cf6a9913ae49a44 on mleanos:feature/JWT-Auth-Lib into f2a6bf9 on meanjs:master.

mleanos · 2016-04-13T09:03:25Z

Tests are passing. I haven't looked at the OAuth issues yet. I'll try to work on that in the next few days.

In the meantime, please feel free to review & provide feedback.

mleanos · 2016-04-13T23:00:59Z

config/lib/express.js

@@ -89,6 +90,21 @@ module.exports.initMiddleware = function (app) {
  // Add the cookie parser and flash middleware
  app.use(cookieParser());
  app.use(flash());
+
+  // Authorize JWT
+  app.use(function (req, res, next) {


I think it would be better to use the new config/lib/authorization service as the middleware here.

Something like:

app.use(authorization.authorize);

Any objections? Or other input on this part of the implementation?

So you move the authorize method out of express into it's own authorization service. Seems a good idea to separate those
Also it makes the file a little shorter, not that it is too big now but it could easily become too big as someone needs to add something more.

trendzetter · 2016-04-14T21:36:53Z

I am having a hard time trying to understand what happens exactly, not that it's bad, it's just complicated and elaborate. I would consider spitting up the tests in a separate pr so just reading what changes is less unwieldy.
One consideration that came to my mind is why signing only the id as payload? Shouldn't the roles also be signed as "claims" so no fiddling with that by enterprising users? Maybe I am just not far enough in my understanding of what happens and how jwt is used, just a first thought.
Does it work, should I test the functionality?

trendzetter · 2016-04-14T21:41:25Z

config/lib/authorization.js

+module.exports = auth;
+
+// Sign the Token
+function signToken(user, options) {


Are there any planned uses for the options parameter?

this parameter is there for anyone that wants to add/override any options coming from the env configuration. We just don't have any use for it at this time. For instance, one auth method might need slightly different options.

See below where we're merging the two objects.

mleanos · 2016-04-14T22:39:33Z

@trendzetter I broke things down into more focused commits. While looking at the changes here, you can specify a single commit to view. That should makes things easier for you to review.

Yes. Everything appears to be working, except for the Social OAuth functionality. I haven't looked into why it's broken, but my guess is the difference in the request handling. If you would like to look into that I would appreciate it. Otherwise, feel free to take this for a test drive.

mleanos · 2016-04-14T22:42:33Z

One consideration that came to my mind is why signing only the id as payload? Shouldn't the roles also be signed as "claims" so no fiddling with that by enterprising users?

I think it may be a good idea to include the roles with the payload. Also, adding the expiration date might be a good idea as well, to further secure the payload by adding a dynamic field.

Anyone else have thoughts on that idea?

trendzetter · 2016-04-15T11:05:20Z

I will take another look to the separate commits

simison · 2016-04-15T11:08:41Z

@trendzetter Check also https://github.com/sindresorhus/refined-github — it has some features that ease reading longer PR's.

PierreBrisorgueil · 2016-06-08T09:39:54Z

Hello @mleanos (sorry for my english :))

For me, we have a little problem from this comit
trainerbill@09389f0

on modules/core/client/app/init.js

Authentication.ready
    .then(function (auth) {

In your pull request it's this

    Authentication.ready
      .then(function (auth) {
        $rootScope.$on('$stateChangeStart', stateChangeStart);
      });

Authentication.ready return readyPromise.promise, bu you never go in 'then' so you never go in 'stateChangeStart' and never check roles on my side. I have a look on this.

new state of promise is not spread to .ready

mleanos · 2016-06-10T02:47:05Z

@PierreBrisorgueil I'm not quite following where this issue is. Is the issue with the implementation of the Authentication.ready method in this PR?

What do you mean by "never go in 'then'"? Maybe provide inline comments to the commits in this PR to clearly point out where/what the issue is.

Thank you for your feedback. I'd like to get this PR back on track, so I appreciate any contribution.

PierreBrisorgueil · 2016-06-10T08:06:06Z

@mleanos Yes it's this,

in fact in this PR (https://github.com/meanjs/mean/tree/b14a5bf5b657e05d62899f752cf6a9913ae49a44)
based on trainerbill@09389f0

If we add a console.log :

  Authentication.ready
      .then(function (auth) {
        console.log("MeanJS");
        $rootScope.$on('$stateChangeStart', stateChangeStart);
      });

In my test with @farhatmo, if you are sign in, after you will never see the console.log("MeanJs"), when you change state. That's what I meant by you will "never go in then", and never check roles in front part (when user is connected).

In fact, in https://github.com/meanjs/mean/blob/b14a5bf5b657e05d62899f752cf6a9913ae49a44/modules/users/client/services/authentication.client.service.js

    var readyPromise = $q.defer();

    var auth = {
      user: null,
      token: null,
      login: login,
      signout: signout,
      refresh: refresh,
      ready: readyPromise.promise
    };

var auth.ready never get the new status and update from readyPromise.promise. So the
information is not propagated to core.client.route-filter.js and Authentication.ready var is never update by promise. So you never go in "then". You never get the resolved status of promise in auth.ready.

For us (me and @farhatmo), you need to make something like this :

authentication.client.service.js

    //var readyPromise = $q.defer();

    var auth = {
      user: null,
      token: null,
      login: login,
      signout: signout,
      safe: safe,
      refresh: refresh,
      readyPromise: $q.defer(),
    };

readyPromise.resolve(auth);
=>

auth.readyPromise.resolve(auth);

core.client.route-filter


    Authentication.readyPromise.promise
    .then(function(auth) {
      $rootScope.$on('$stateChangeStart', stateChangeStart);
    });

mleanos · 2016-06-11T01:31:54Z

@PierreBrisorgueil Thank you for your in depth explanation. I'll implement your suggested fix for this.

PierreBrisorgueil · 2016-06-14T09:39:06Z

@mleanos perfect 👍 :)

We have one last problem, now (with this modifications) we tcheck users role in order to protect front part display. But we do this only if we execute "stateChangeStart".

This event is called, only if we change state. If you refresh the page, actually, you never go in stateChangeStart, and don't tcheck roles. So the user will see the template. It's not critical, it's only in client side ..

But i think it's not really clean, we probably can protect this case. ( create a function to check roles from two events, not only stateChangeStart ... i d'ont know)

I have a look on this quickly if possible.

simison · 2016-06-14T09:46:43Z

BTW, some food for thought: http://cryto.net/~joepie91/blog/2016/06/13/stop-using-jwt-for-sessions/

simison · 2016-11-03T14:56:45Z

FYI, some reading: https://angularjs.blogspot.com/2016/11/easy-angular-authentication-with-json.html

Ghalleb · 2016-12-11T21:15:31Z

First, thanks for the good job. I'm using 0.4.2 and it works like a charm.

But I was expecting JWT authentification to be in 0.5.0 so I can integrate my project with a mobileapp...
Is there a chance for this feature to be in 0.5.0 release or in near future?

Mean.js is a great framework, but with JWT integration and the ability to use it as a pure back end for mobile app, it would be perfect.

PierreBrisorgueil · 2016-12-12T08:28:16Z

Hello @Ghalleb you can use it with an app without jwt auth, with basic auth and https :)
I have a sample like this with iOS (swift1.2 or 2 don't remember) if you have some questions

But if we switch to JWT, always agree for my side, done in my fork, I will try to push samples Apps switft, iOS & tvOS maybe :)

Always in the idea of having a better and quicker starter for projects :)

Ghalleb · 2016-12-12T16:52:19Z

@PierreBrisorgueil
If I can have access to your sample, it would be nice.
Does it work with social login as well?
It can do the job while waiting for JWT Authentication...

PierreBrisorgueil · 2016-12-14T07:15:58Z

@Ghalleb no, only basic inscription, and login, i will have a look on this quickly an share you a quote of code here if i can, as soon as possible :)

This require https for security, with Let's Encrypt, it's perfect.

can't make a sample for the moment, to much work, but i will try after ;)

tomasfse · 2017-01-04T13:47:13Z

Please don't forget this

JWT is necessary for better scaling and for reusing the api for mobile applications (yet discussed some comments ago)

Thanks for the work @mleanos

Implements JWT Authentication, and removes dependency of session storage. Closes meanjs#389

Updates the necessary server-side tests to accommodate the JWT Authentication.

Not really sure about the $httpBackend expects & flushes that had to be added. It seems like we shouldn't have to do this for every client-side tests. The issue is that the Authentication service now makes a call to the backend (`api/users/me`) to retrieve the user info. Could use some eyes on this aspect of the implementation.

Fixed the E2E tests for the JWT implementation.

Adds the "button" role to the signout link in the header.

Adds authentication to the SocketIO server using the JWT Token. Added the socketio-auth package to the project which provides hooks for handling authentication with SocketIO. Changed the Passport JWT Strategy to use the `fromAuthHeader()` method for getting the Token from the request.

Adds a listener to the mock Socket.IO server used with the Karma tests.

Fixed lingering issues after rebasing. Fixed client-side tests that were failing. Added authorization header to server-side tests for the Admin Articles module Added Authentication mocking for edit-profile client controller test

Added missing dependencies after manual merge conflict resolution. Also, updated test to include JWT auth header.

Added back the passport serialization callbacks, due to an error with the multer uploads.

The user's profile image upload was throwing an error stating that the "user wasn't serialized". This was caused by the attempt to "login" using the req.login which uses Passport & expects the User to be serialized with the request. The fix was to remove the `req.login` attempt, since there shouldn't be a need to log the user in again (not even sure why that was happening in the first place). As a result, the Passport serialization in the User server configuration is no longer needed. This also removes that functionality.

Moved the express middleware for authorizing the User (on each request) to the new Authorization service.

Fixes merge issues, after rebasing from upstream. Fixes broken tests that weren't setup to handle new JWT authentication logic.

Fixes the client-side UsersService.me() api route, that was missing the leading "/". Also cleaned up the client-side Authentication's `refresh()` method by removing ununsed parameters.

Modified the client-side Authentication service's ready promise to ensure we can handle the resolve externally from the service. This is critical in the Angular route filter for changing state, in order to check that the user has sufficient permissions to access the client-side route. Fixes an issue with the redirect url not getting set properly after removing sessions. Fixes Notification message after user signs in, to correctly show the user's first name. Upgraded version of passport-jwt library.

Fixes broken client-side tests after changing the url reference of the /api/users/me endpoint, in the UsersService.

mleanos · 2017-01-21T04:18:21Z

I've managed to get everything working, except for the social logins. For some reason, the callback is not receiving the req object that contains the user. It could be that since sessions aren't being used any longer, that passport isn't able to retrieve the user when the social provider responds to the callback. Although, it seems that the request should still have the user embedded. Any help here would be much appreciated; I could use a second set of eyes on it.

As for why the build is failing, I'm unclear since the build & tests pass locally (Windows 10 x64, and Node 6.6.0).

Ghalleb · 2017-03-22T15:00:37Z

Hi,

Any news on JWT integration and Social Login?
It doesn't seems to have a lot of commits lately

mleanos changed the title ~~Feature/jwt auth lib~~ JWT Authentication simplified Apr 10, 2016

mleanos mentioned this pull request Apr 10, 2016

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

Closed

ilanbiala reviewed Apr 11, 2016
View reviewed changes

mleanos force-pushed the feature/JWT-Auth-Lib branch from ae99557 to bf78f16 Compare April 12, 2016 01:57

mleanos force-pushed the feature/JWT-Auth-Lib branch from bf78f16 to b14a5bf Compare April 13, 2016 08:05

mleanos reviewed Apr 13, 2016
View reviewed changes

trendzetter reviewed Apr 14, 2016
View reviewed changes

mleanos mentioned this pull request Apr 28, 2016

fix(redirect url): on login from social #1327

Closed

mleanos removed this from the 0.5.0 milestone Sep 10, 2016

mleanos mentioned this pull request Oct 19, 2016

Upgrading to use Material Design #369

Closed

mleanos added this to the Backlog milestone Nov 3, 2016

mleanos added 14 commits January 16, 2017 16:32

feat(core): JWT Authentication simplified

f2dac09

Implements JWT Authentication, and removes dependency of session storage. Closes meanjs#389

feat(tests): Server tests with JWT Auth

683df15

Updates the necessary server-side tests to accommodate the JWT Authentication.

feat(tests): E2E Test for JWT

e489336

Fixed the E2E tests for the JWT implementation.

signout link style

86a5ae4

Adds the "button" role to the signout link in the header.

Mock Socket server authentication

546cf02

Adds a listener to the mock Socket.IO server used with the Karma tests.

Lingering issues after rebase

98e27c3

Fixed lingering issues after rebasing. Fixed client-side tests that were failing. Added authorization header to server-side tests for the Admin Articles module Added Authentication mocking for edit-profile client controller test

Missing dependency after merge & update test

72ad45b

Added missing dependencies after manual merge conflict resolution. Also, updated test to include JWT auth header.

Add back passport serialization

692d183

Added back the passport serialization callbacks, due to an error with the multer uploads.

Authorization middleware

140974f

Moved the express middleware for authorizing the User (on each request) to the new Authorization service.

fix(tests): Broken tests & merge issues

ee60df4

Fixes merge issues, after rebasing from upstream. Fixes broken tests that weren't setup to handle new JWT authentication logic.

fix(user): Client Users Service me() method

661958f

Fixes the client-side UsersService.me() api route, that was missing the leading "/". Also cleaned up the client-side Authentication's `refresh()` method by removing ununsed parameters.

farhatmo mentioned this pull request Jan 19, 2017

Front-End Part Angular lirantal/Riess.js#2

Open

mleanos added 2 commits January 20, 2017 11:22

fix(tests): Broken client-side tests

c7a6ea8

Fixes broken client-side tests after changing the url reference of the /api/users/me endpoint, in the UsersService.

mleanos force-pushed the feature/JWT-Auth-Lib branch from 181cc93 to c7a6ea8 Compare January 20, 2017 19:56

mleanos mentioned this pull request Mar 12, 2017

Relevance of Social Providers #1732

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JWT Authentication simplified #1300

JWT Authentication simplified #1300

mleanos commented Apr 10, 2016

mleanos commented Apr 10, 2016

coveralls commented Apr 10, 2016

ilanbiala Apr 11, 2016

mleanos Apr 11, 2016

ilanbiala commented Apr 11, 2016

coveralls commented Apr 12, 2016

mleanos commented Apr 12, 2016

coveralls commented Apr 13, 2016

mleanos commented Apr 13, 2016

mleanos Apr 13, 2016

trendzetter Apr 15, 2016 •

edited

Loading

trendzetter commented Apr 14, 2016 •

edited

Loading

trendzetter Apr 14, 2016 •

edited

Loading

mleanos Apr 14, 2016 •

edited

Loading

mleanos commented Apr 14, 2016

mleanos commented Apr 14, 2016 •

edited

Loading

trendzetter commented Apr 15, 2016

simison commented Apr 15, 2016

PierreBrisorgueil commented Jun 8, 2016 •

edited

Loading

mleanos commented Jun 10, 2016

PierreBrisorgueil commented Jun 10, 2016

mleanos commented Jun 11, 2016

PierreBrisorgueil commented Jun 14, 2016

simison commented Jun 14, 2016

simison commented Nov 3, 2016 •

edited

Loading

Ghalleb commented Dec 11, 2016

PierreBrisorgueil commented Dec 12, 2016

Ghalleb commented Dec 12, 2016

PierreBrisorgueil commented Dec 14, 2016 •

edited

Loading

tomasfse commented Jan 4, 2017

mleanos commented Jan 21, 2017

Ghalleb commented Mar 22, 2017

JWT Authentication simplified #1300

Are you sure you want to change the base?

JWT Authentication simplified #1300

Conversation

mleanos commented Apr 10, 2016

mleanos commented Apr 10, 2016

coveralls commented Apr 10, 2016

ilanbiala Apr 11, 2016

Choose a reason for hiding this comment

mleanos Apr 11, 2016

Choose a reason for hiding this comment

ilanbiala commented Apr 11, 2016

coveralls commented Apr 12, 2016

mleanos commented Apr 12, 2016

coveralls commented Apr 13, 2016

mleanos commented Apr 13, 2016

mleanos Apr 13, 2016

Choose a reason for hiding this comment

trendzetter Apr 15, 2016 • edited Loading

Choose a reason for hiding this comment

trendzetter commented Apr 14, 2016 • edited Loading

trendzetter Apr 14, 2016 • edited Loading

Choose a reason for hiding this comment

mleanos Apr 14, 2016 • edited Loading

Choose a reason for hiding this comment

mleanos commented Apr 14, 2016

mleanos commented Apr 14, 2016 • edited Loading

trendzetter commented Apr 15, 2016

simison commented Apr 15, 2016

PierreBrisorgueil commented Jun 8, 2016 • edited Loading

mleanos commented Jun 10, 2016

PierreBrisorgueil commented Jun 10, 2016

mleanos commented Jun 11, 2016

PierreBrisorgueil commented Jun 14, 2016

simison commented Jun 14, 2016

simison commented Nov 3, 2016 • edited Loading

Ghalleb commented Dec 11, 2016

PierreBrisorgueil commented Dec 12, 2016

Ghalleb commented Dec 12, 2016

PierreBrisorgueil commented Dec 14, 2016 • edited Loading

tomasfse commented Jan 4, 2017

mleanos commented Jan 21, 2017

Ghalleb commented Mar 22, 2017

trendzetter Apr 15, 2016 •

edited

Loading

trendzetter commented Apr 14, 2016 •

edited

Loading

trendzetter Apr 14, 2016 •

edited

Loading

mleanos Apr 14, 2016 •

edited

Loading

mleanos commented Apr 14, 2016 •

edited

Loading

PierreBrisorgueil commented Jun 8, 2016 •

edited

Loading

simison commented Nov 3, 2016 •

edited

Loading

PierreBrisorgueil commented Dec 14, 2016 •

edited

Loading