Repository: feathersjs/feathers-authentication
Pull request #353 (2016-11-25 14:25:00)

fixing redirect middleware
### Summary

Express redirect middleware were not actually redirecting because of 2 things:

- the `hook` object lives on `res.hook` not `req.hook`
- if the hook object is previously modified the modifications weren't passed along. Only modified `hook.data` gets passed along to express middleware.


Patch:
diff --git a/src/express/failure-redirect.js b/src/express/failure-redirect.js
index 290c3ab..d3d3e09 100644
--- a/src/express/failure-redirect.js
+++ b/src/express/failure-redirect.js
@@ -10,8 +10,8 @@ export default function failureRedirect(options = {}) {
       res.clearCookie(options.cookie.name);
     }
 
-    if (req.hook && req.hook.redirect) {
-      const { url, status } = req.hook.redirect;
+    if (res.hook && res.hook.data && res.hook.data.__redirect) {
+      const { url, status } = res.hook.data.__redirect;
       debug(`Redirecting to ${url} after failed authentication.`);
       
       res.status(status || 302);
diff --git a/src/express/success-redirect.js b/src/express/success-redirect.js
index 4a83b30..6f63ef9 100644
--- a/src/express/success-redirect.js
+++ b/src/express/success-redirect.js
@@ -4,9 +4,9 @@ const debug = Debug('feathers-authentication:middleware:success-redirect');
 export default function successRedirect() {
   debug('Registering successRedirect middleware');
 
-  return function(req, res, next) {    
-    if (req.hook && req.hook.redirect) {
-      const { url, status } = req.hook.redirect;
+  return function(req, res, next) {
+    if (res.hook && res.hook.data && res.hook.data.__redirect) {
+      const { url, status } = res.hook.data.__redirect;
       debug(`Redirecting to ${url} after successful authentication.`);
 
       res.status(status || 302);
diff --git a/src/hooks/authenticate.js b/src/hooks/authenticate.js
index dc7a02e..75162ec 100644
--- a/src/hooks/authenticate.js
+++ b/src/hooks/authenticate.js
@@ -41,11 +41,9 @@ export default function authenticate (strategy, options = {}) {
         // TODO (EK): Reject with something...
         // You get back result.challenge and result.status
         if (options.failureRedirect) {
+          // TODO (EK): Bypass the service?
           // hook.result = true
-          hook.redirect = {
-            status: 302,
-            url: options.failureRedirect
-          };
+          Object.defineProperty(hook.data, '__redirect', { value: { status: 302, url: options.failureRedirect } });
         }
 
         const { challenge, status = 401 } = result;
@@ -60,22 +58,15 @@ export default function authenticate (strategy, options = {}) {
 
       if (result.success) {
         hook.params = Object.assign({ authenticated: true }, hook.params, result.data);
-
         if (options.successRedirect) {
           // TODO (EK): Bypass the service?
           // hook.result = true
-          hook.redirect = {
-            status: 302,
-            url: options.successRedirect
-          };
+          Object.defineProperty(hook.data, '__redirect', { value: { status: 302, url: options.successRedirect } });
         }
       } else if (result.redirect) {
         // TODO (EK): Bypass the service?
         // hook.result = true
-        hook.redirect = {
-          status: result.status,
-          url: result.url
-        };
+        Object.defineProperty(hook.data, '__redirect', { value: { status: result.status, url: result.url } });
       }
 
       return Promise.resolve(hook);
diff --git a/src/passport/authenticate.js b/src/passport/authenticate.js
index 2da5d1d..aa23452 100644
--- a/src/passport/authenticate.js
+++ b/src/passport/authenticate.js
@@ -36,8 +36,8 @@ export default function authenticate (options = {}) {
         // It is not feasible to construct a chain of multiple strategies that involve
         // redirection (for example both Facebook and Twitter), since the first one to
         // redirect will halt the chain.
-        if (request.strategy) {
-          strategies = [request.strategy];
+        if (request.body && request.body.strategy) {
+          strategies = [request.body.strategy];
         } else if (Array.isArray(name)) {
           strategies = name;
         } else {


Test patch:
diff --git a/test/express/failure-redirect.test.js b/test/express/failure-redirect.test.js
index da019fa..e2de356 100644
--- a/test/express/failure-redirect.test.js
+++ b/test/express/failure-redirect.test.js
@@ -11,14 +11,15 @@ describe('express:failureRedirect', () => {
   let error;
 
   beforeEach(() => {
-    req = {
+    req = {};
+    res = {
       hook: {
-        redirect: {
-          url: '/app'
+        data: {
+          __redirect: {
+            url: '/app'
+          }
         }
-      }
-    };
-    res = {
+      },
       clearCookie: sinon.spy(),
       redirect: sinon.spy(),
       status: sinon.spy()
@@ -42,7 +43,7 @@ describe('express:failureRedirect', () => {
     });
 
     it('supports a custom status code', () => {
-      req.hook.redirect.status = 400;
+      res.hook.data.__redirect.status = 400;
       failureRedirect()(error, req, res);
       expect(res.status).to.have.been.calledOnce;
       expect(res.status).to.have.been.calledWith(400);
@@ -66,9 +67,9 @@ describe('express:failureRedirect', () => {
     });
   });
 
-  describe('when req.hook is not defined', done => {
+  describe('when res.hook is not defined', done => {
     it('calls next with error', done => {
-      delete req.hook;
+      delete res.hook;
       failureRedirect()(error, req, res, e => {
         expect(e).to.equal(error);
         done();
@@ -76,9 +77,9 @@ describe('express:failureRedirect', () => {
     });
   });
 
-  describe('when req.hook.redirect is not defined', done => {
+  describe('when res.hook.data.redirect is not defined', done => {
     it('calls next with error', done => {
-      delete req.hook.redirect;
+      delete res.hook.data.__redirect;
       failureRedirect()(error, req, res, e => {
         expect(e).to.equal(error);
         done();
diff --git a/test/express/success-redirect.test.js b/test/express/success-redirect.test.js
index e3462a4..bd0d925 100644
--- a/test/express/success-redirect.test.js
+++ b/test/express/success-redirect.test.js
@@ -10,14 +10,15 @@ describe('express:successRedirect', () => {
   let res;
 
   beforeEach(() => {
-    req = {
+    req = {};
+    res = {
       hook: {
-        redirect: {
-          url: '/app'
+        data: {
+          __redirect: {
+            url: '/app'
+          }
         }
-      }
-    };
-    res = {
+      },
       redirect: sinon.spy(),
       status: sinon.spy()
     };
@@ -38,7 +39,7 @@ describe('express:successRedirect', () => {
     });
 
     it('supports a custom status code', () => {
-      req.hook.redirect.status = 400;
+      res.hook.data.__redirect.status = 400;
       successRedirect()(req, res);
       expect(res.status).to.have.been.calledOnce;
       expect(res.status).to.have.been.calledWith(400);
@@ -47,16 +48,16 @@ describe('express:successRedirect', () => {
     });
   });
 
-  describe('when req.hook is not defined', () => {
+  describe('when res.hook is not defined', () => {
     it('calls next', next => {
-      delete req.hook;
+      delete res.hook;
       successRedirect()(req, res, next);
     });
   });
 
-  describe('when req.hook.redirect is not defined', () => {
+  describe('when res.hook.data.__redirect is not defined', () => {
     it('calls next', next => {
-      delete req.hook.redirect;
+      delete res.hook.data.__redirect;
       successRedirect()(req, res, next);
     });
   });
diff --git a/test/hooks/authenticate.test.js b/test/hooks/authenticate.test.js
index 73f9bf3..a4fe5fe 100644
--- a/test/hooks/authenticate.test.js
+++ b/test/hooks/authenticate.test.js
@@ -128,8 +128,8 @@ describe('hooks:authenticate', () => {
     it('supports redirecting', () => {
       const successRedirect = '/app';
       return authenticate('mock', { successRedirect })(hook).then(hook => {
-        expect(hook.redirect.status).to.equal(302);
-        expect(hook.redirect.url).to.equal(successRedirect);
+        expect(hook.data.__redirect.status).to.equal(302);
+        expect(hook.data.__redirect.url).to.equal(successRedirect);
       });
     });
   });
@@ -196,8 +196,8 @@ describe('hooks:authenticate', () => {
     it('supports redirecting', () => {
       const failureRedirect = '/login';
       return authenticate('mock', { failureRedirect })(hook).catch(() => {
-        expect(hook.redirect.status).to.equal(302);
-        expect(hook.redirect.url).to.equal(failureRedirect);
+        expect(hook.data.__redirect.status).to.equal(302);
+        expect(hook.data.__redirect.url).to.equal(failureRedirect);
       });
     });
   });
@@ -236,10 +236,10 @@ describe('hooks:authenticate', () => {
       };
     });
 
-    it('sets hook.redirect', () => {
+    it('sets hook.data.__redirect', () => {
       return authenticate('mock')(hook).then(hook => {
-        expect(hook.redirect.status).to.equal(response.status);
-        expect(hook.redirect.url).to.equal(response.url);
+        expect(hook.data.__redirect.status).to.equal(response.status);
+        expect(hook.data.__redirect.url).to.equal(response.url);
       });
     });
   });