Passport JWT strategy for mobile #110

JameelB · 2017-09-01T14:10:04Z

Motivation

The current strategy that Passport is using for mobile is not working properly due to the implications listed on RAINCATCH-1192.

Description

Add Passport's JWT strategy for mobile authentication instead of cookies.

Progress

Separate mobile and portal into two express Routers
Create relevant routes for each mobile/portal
Add passport JWT Strategy
Update Passport Auth's authenticate function to authenticate without redirection
Update Passport Auth's protect function to verify user authentication with a token
Clean up
Update unit tests
Update documentation

Additional Notes

Related JIRA - https://issues.jboss.org/browse/RAINCATCH-1209

wtrocki · 2017-09-01T15:06:10Z

cloud/passportauth/src/auth/PassportAuth.ts

@@ -28,12 +31,16 @@ export class PassportAuth implements EndpointSecurity {
   * @param app - An express application
   * @param sessionOpts - Session options to be used by express-session


Add note that when skipping this session is not being used.

wtrocki · 2017-09-01T15:12:00Z

cloud/passportauth/src/auth/PassportAuth.ts

@@ -111,7 +140,23 @@ export class PassportAuth implements EndpointSecurity {
   * @param passportApi - passport.js instance
   */
  protected setup(passportApi: passport.Passport) {
+    const options = {
+      jwtFromRequest: ExtractJwt.fromAuthHeader(),
+      secretOrKey: 'secret'


Remember to extract and document that as keeping this secret is crucial for general application security.

wtrocki · 2017-09-01T15:12:56Z

demo/server/src/modules/passport-auth/index.ts

+    }
+  });
+
+  router.post('/login-mobile', authService.authenticate('jwt'), (req, res, next) => {


Why this is needed? Probably forgot to remove it?

wtrocki · 2017-09-01T15:13:14Z

Checked changes locally. Great progress so far.

wtrocki · 2017-09-04T18:24:47Z

cloud/passportauth/src/auth/PassportAuth.ts

   */
-  public init(app: express.Router, sessionOpts?: SessionOptions) {
+  public init(app: express.Router, sessionOpts?: SessionOptions, secret?: string) {


secret?: any may be better => That's restrictive as people may pass also key (which is probably better to secure this)

wtrocki · 2017-09-04T18:25:30Z

cloud/passportauth/src/auth/PassportAuth.ts

-      };
-      this.userRepo.getUserByLogin(jwtPayload.username, callback);
-    }));
+  protected setup(passportApi: passport.Passport, jwtOpts?: JwtOptions) {


missing documentation for parameter.

wtrocki · 2017-09-04T18:27:14Z

cloud/passportauth/src/auth/PassportAuth.ts

+        this.userRepo.getUserByLogin(jwtPayload.username, callback);
+      }));
+    } else {
+      passportApi.use(new Strategy(defaultStrategy(this.userRepo, this.userService)));


btw. - wit this change our defaultStrategy may not be as accurate as before. WDYT to rename this to WebStrategy to reflect the use case?

wtrocki · 2017-09-04T18:27:52Z

cloud/passportauth/src/auth/PassportAuth.ts

-    }));
+  protected setup(passportApi: passport.Passport, jwtOpts?: JwtOptions) {
+    if (jwtOpts) {
+      passportApi.use(new JwtStrategy(jwtOpts, (jwtPayload, done) => {


[Optional] [style] If default strategy is extracted to separate file let's have this as well.

wtrocki · 2017-09-04T18:30:27Z

cloud/passportauth/src/auth/PassportAuth.ts

-      };
-      this.userRepo.getUserByLogin(jwtPayload.username, callback);
-    }));
+  protected setup(passportApi: passport.Passport, jwtOpts?: JwtOptions) {


[Question] Do we still using interface for this? I think we may actually not need
We can have two setup methods once with jwtOptions and second that will setup session based strategy? This will save you another if and will make code cleaner.
We can even add name to provide more context.

setupToken and setupCookie etc.

wtrocki · 2017-09-04T18:31:27Z

demo/server/config-dev.json

@@ -19,5 +19,9 @@
  },
  "redis": {
    "url": "redis://127.0.0.1:6379"
+  },
+  "jwtSecret": "raincatcher",


I will suggest here to put really long generated hash (min 250bits).
It's also important to document this properly.

wtrocki · 2017-09-04T18:34:03Z

demo/server/config-dev.json

+  },
+  "jwtSecret": "raincatcher",
+  "clientUrl": {
+    "portal": "http://localhost:9003/?url=http://localhost:8001"


[Bug] 💯 That's not goingto work for any other environment apart from local development.
Having it this way will also mean additional setup instruction to this.
Overall -1 for this parameter, but we probably need to talk about it to get proper intentions here.

wtrocki · 2017-09-04T18:35:08Z

demo/server/src/modules/index.ts

+  demoDataSetup(connectionPromise);
+
+  const portalApp = express.Router();
+  portalsecurityMiddleware = securitySetup(portalApp, sessionOpts);


[Proposition] As mentioned above we do not use interface. Let's just rename this to reflect what we really setting this up. This method is just helper to setup passport strategy (simplification, hiding complexity)

wtrocki · 2017-09-04T18:38:55Z

demo/server/src/modules/passport-auth/index.ts

    return res.render('login', {
      title: 'Portal Feedhenry Workforce Management'
    });
  });

-  router.post('/login', authService.authenticate('local', '/', '/loginError'));
+  router.post('/login', authService.authenticate('local', {
+    successReturnToOrRedirect: config.clientUrl.portal,


Same as above: Worth reading as inspiration: http://www.baeldung.com/spring-security-redirect-login

wtrocki · 2017-09-04T18:42:17Z

demo/server/src/modules/passport-auth/index.ts


  router.get('/loginError', (req: express.Request, res: express.Response) => {
    return res.render('login', {
-      title: 'Feedhenry Workforce Management',
+      title: 'Portal Feedhenry Workforce Management',


[Style][Optional] Portal Feedhenry Workforce Management => RainCatcher Portal
[Optional] - Externalize to config.
[Optional] - Move to passport (as they are now really connected to the actual strategies) not universal.

wtrocki · 2017-09-04T18:42:44Z

demo/server/src/modules/passport-auth/index.ts


  router.get('/loginError', (req: express.Request, res: express.Response) => {
    return res.render('login', {
-      title: 'Feedhenry Workforce Management',
+      title: 'Portal Feedhenry Workforce Management',
      message: 'Invalid credentials'});
  });

  router.get('/profile', authService.protect(), (req: express.Request, res: express.Response) => {


[Question] Is that used in portal only? Maybe comment.

wtrocki · 2017-09-04T18:43:33Z

demo/server/src/modules/passport-auth/index.ts


  router.get('/loginError', (req: express.Request, res: express.Response) => {
    return res.render('login', {
-      title: 'Feedhenry Workforce Management',
+      title: 'Portal Feedhenry Workforce Management',
      message: 'Invalid credentials'});
  });

  router.get('/profile', authService.protect(), (req: express.Request, res: express.Response) => {


Proper format will be user/profile

wtrocki · 2017-09-04T18:50:30Z

@JameelB - Exceptional work. There is one bug we can deal with and couple a minor improvements.
We can chat about that tommorow and approve this PR.

wtrocki · 2017-09-05T10:30:55Z

cloud/passportauth/src/auth/PassportAuth.ts

 import { UserRepository } from '../user/UserRepository';
 import { UserService } from '../user/UserService';

 import { EndpointSecurity } from '@raincatcher/express-auth';
-import { defaultStrategy } from './DefaultStrategy';
+import { jwtStrategy, webStrategy } from './DefaultStrategy';


[Very optional] DefaultStrategy may be plural here : DefaultStrategies.

wtrocki · 2017-09-05T10:32:45Z

demo/server/config-prod.json

-  "jwtSecret": "raincatcher",
-  "clientUrl": {
-    "portal": "http://localhost:9003/?url=http://localhost:8001"
+  "jwtSecret": "D837131FD17F62CB85FBD5919563086369691F4D42379C3596F811839A8992CBA1FBA88DF243BF2481940F112D339C33283BDFEF29A13612550EDAAAB7B5E061",


ping @austincunningham - that's update for demo passport auth setup.
Users will need to change that in order to make sure that solution is secure.

wtrocki · 2017-09-05T10:34:10Z

@JameelB - Marking as approved for now, but let's not merge until angularjs work will be reviewed and tested.

Unit tests needs minor tweak:

  1) Test Passport Auth should return an error if an error occurred when retrieving the user:
     TypeError: Cannot read property 'authorization' of undefined
      at src/auth/PassportAuth.ts:26:335
      at Context.<anonymous> (test/PassportAuthTest.ts:51:34)
  2) Test Passport Auth should not authenticate the user if the credentials provided are not valid:
     TypeError: Cannot read property 'authorization' of undefined
      at src/auth/PassportAuth.ts:26:335
      at Context.<anonymous> (test/PassportAuthTest.ts:72:34)
  3) Test Passport Auth should authenticate the user if the credentials provided are valid:
     TypeError: Cannot read property 'authorization' of undefined
      at src/auth/PassportAuth.ts:26:335
      at Context.<anonymous> (test/PassportAuthTest.ts:93:34)
  4) Test Passport Auth should not authenticate if the user is already authenticated:
     TypeError: Cannot read property 'authorization' of undefined
      at src/auth/PassportAuth.ts:26:335
      at Context.<anonymous> (test/PassportAuthTest.ts:222:34)

coveralls · 2017-09-08T10:55:07Z

Changes Unknown when pulling 8e531a9 on JameelB:offline-mobile-auth into ** on feedhenry-raincatcher:master**.

coveralls · 2017-09-08T10:55:07Z

Changes Unknown when pulling 8e531a9 on JameelB:offline-mobile-auth into ** on feedhenry-raincatcher:master**.

coveralls · 2017-09-08T11:00:02Z

Changes Unknown when pulling 86629f3 on JameelB:offline-mobile-auth into ** on feedhenry-raincatcher:master**.

coveralls · 2017-09-08T14:49:26Z

Changes Unknown when pulling 7c220c4 on JameelB:offline-mobile-auth into ** on feedhenry-raincatcher:master**.

coveralls · 2017-09-08T14:49:26Z

Changes Unknown when pulling 7c220c4 on JameelB:offline-mobile-auth into ** on feedhenry-raincatcher:master**.

coveralls · 2017-09-08T15:16:03Z

Changes Unknown when pulling 3ce0a2d on JameelB:offline-mobile-auth into ** on feedhenry-raincatcher:master**.

wtrocki · 2017-09-10T21:29:39Z

cloud/passportauth/src/auth/PassportAuth.ts

+        const token = req.headers.authorization.toString().substring(4);
+        try {
+          const user = jwt.verify(token, this.jwtOpts.secretOrKey);
+          hasRole = role ? this.userService.hasResourceRole(user, role) : true;


IMHO role ? A : B makes this code unreadable - try to avoid that in the future, especially in mission critical code like security implementation.

wtrocki · 2017-09-10T21:31:15Z

cloud/passportauth/src/auth/PassportAuth.ts

-          req.session.clientURL = req.session.returnTo;
+      let hasRole;
+
+      if (req.headers && req.headers.authorization) {


It will be wort to check if auth is JWT.

JameelB requested review from wtrocki and paolobueno September 1, 2017 14:22

wtrocki reviewed Sep 1, 2017

View reviewed changes

wtrocki added the enhancement label Sep 1, 2017

wtrocki reviewed Sep 4, 2017

View reviewed changes

wtrocki reviewed Sep 5, 2017

View reviewed changes

wtrocki previously approved these changes Sep 5, 2017

View reviewed changes

JameelB dismissed wtrocki’s stale review via 4766c5c September 6, 2017 11:58

JameelB added 5 commits September 8, 2017 09:39

add jwt strategy and routes for mobile login

d0e1a09

fix login for portal

ad99626

separate mobile and portal into different routes

ea5d573

fix authenticate function

1d2c1e1

remove unused securityMiddleware in imports

2a04522

JameelB added 4 commits September 8, 2017 09:39

clean up routes

0e4cc7b

clean up

4c2da13

address PR comments

c08e210

rename login routes and default strategy file

21f2060

wtrocki force-pushed the offline-mobile-auth branch from 4766c5c to 21f2060 Compare September 8, 2017 08:43

update unit test

8e531a9

fix tslint issues

86629f3

improve unit tests

7c220c4

Fix tslint error

3ce0a2d

wtrocki reviewed Sep 10, 2017

View reviewed changes

wtrocki approved these changes Sep 10, 2017

View reviewed changes

wtrocki changed the title ~~[WIP] Passport JWT strategy for mobile~~ Passport JWT strategy for mobile Sep 10, 2017

wtrocki merged commit 983cd8c into feedhenry-raincatcher:master Sep 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Passport JWT strategy for mobile #110

Passport JWT strategy for mobile #110

JameelB commented Sep 1, 2017 •

edited

Loading

wtrocki Sep 1, 2017

wtrocki Sep 1, 2017

wtrocki Sep 1, 2017

wtrocki commented Sep 1, 2017

wtrocki Sep 4, 2017

wtrocki Sep 4, 2017

wtrocki Sep 4, 2017

wtrocki Sep 4, 2017 •

edited

Loading

wtrocki Sep 4, 2017

wtrocki Sep 4, 2017

wtrocki Sep 4, 2017 •

edited

Loading

wtrocki Sep 4, 2017 •

edited

Loading

wtrocki Sep 4, 2017 •

edited

Loading

wtrocki Sep 4, 2017

wtrocki Sep 4, 2017

wtrocki Sep 4, 2017

wtrocki commented Sep 4, 2017

wtrocki Sep 5, 2017

wtrocki Sep 5, 2017

wtrocki commented Sep 5, 2017

coveralls commented Sep 8, 2017

coveralls commented Sep 8, 2017

coveralls commented Sep 8, 2017 •

edited

Loading

coveralls commented Sep 8, 2017

coveralls commented Sep 8, 2017

coveralls commented Sep 8, 2017 •

edited

Loading

wtrocki Sep 10, 2017

wtrocki Sep 10, 2017

		@@ -28,12 +31,16 @@ export class PassportAuth implements EndpointSecurity {
		* @param app - An express application
		* @param sessionOpts - Session options to be used by express-session

Passport JWT strategy for mobile #110

Passport JWT strategy for mobile #110

Conversation

JameelB commented Sep 1, 2017 • edited Loading

Motivation

Description

Progress

Additional Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtrocki commented Sep 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtrocki Sep 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtrocki Sep 4, 2017 • edited Loading

Choose a reason for hiding this comment

wtrocki Sep 4, 2017 • edited Loading

Choose a reason for hiding this comment

wtrocki Sep 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtrocki commented Sep 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtrocki commented Sep 5, 2017

coveralls commented Sep 8, 2017

coveralls commented Sep 8, 2017

coveralls commented Sep 8, 2017 • edited Loading

coveralls commented Sep 8, 2017

coveralls commented Sep 8, 2017

coveralls commented Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JameelB commented Sep 1, 2017 •

edited

Loading

wtrocki Sep 4, 2017 •

edited

Loading

wtrocki Sep 4, 2017 •

edited

Loading

wtrocki Sep 4, 2017 •

edited

Loading

wtrocki Sep 4, 2017 •

edited

Loading

coveralls commented Sep 8, 2017 •

edited

Loading

coveralls commented Sep 8, 2017 •

edited

Loading