Prevent users being logged out incorrectly (#7171)

Updates API and Webapp to:

- Only consider 401s from CouchDB to be valid missing authentication
- Throw other _session errors without changes
- Malformed _session responses result in a 500
- Don't send 401s for missing userCtx, instead send the actual authentication error or a 500.
- Add a custom header to 401 responses that Webapp checks for, and only when matched it would log users out. (prevents man-in-the-middle from logging users out with 401s)

#7169

(cherry picked from commit 2486dc5)

Author: dianabarsan

api/src/auth.js

@@ -86,14 +86,17 @@ module.exports = {
   getUserCtx: req => {
     return get('/_session', req.headers)
       .catch(err => {
-        throw { code: 401, message: 'Not logged in', err: err };
+        if (err.statusCode === 401) {
+          throw { code: 401, message: 'Not logged in', err: err };
+        }
+        throw err;
       })
       .then(auth => {
         if (auth && auth.userCtx && auth.userCtx.name) {
           req.headers['X-Medic-User'] = auth.userCtx.name;
           return auth.userCtx;
         }
-        throw { code: 401, message: 'Not logged in' };
+        throw { code: 500, message: 'Failed to authenticate' };
       });
   },
 

api/src/middleware/authorization.js

@@ -31,27 +31,40 @@ module.exports = {
       .then(next);
   },
 
-  // blocks offline users not-authorized requests
-  offlineUserFirewall: (req, res, next) => {
+  handleAuthErrors: (req, res, next) => {
+    if (req.authErr) {
+      return serverUtils.error(req.authErr, req, res);
+    }
+
     if (!req.userCtx) {
-      return serverUtils.notLoggedIn(req, res);
+      return serverUtils.error('Authentication error', req, res);
+    }
+
+    next();
+  },
+
+  handleAuthErrorsAllowingAuthorized: (req, res, next) => {
+    if (req.authorized) {
+      return next();
     }
 
-    if (!auth.isOnlineOnly(req.userCtx) && !req.authorized) {
+    return module.exports.handleAuthErrors(req, res, next);
+  },
+
+  // blocks offline users not-authorized requests
+  offlineUserFirewall: (req, res, next) => {
+    if (req.userCtx && !auth.isOnlineOnly(req.userCtx) && !req.authorized) {
       res.status(FIREWALL_ERROR.code);
       return res.json(FIREWALL_ERROR);
     }
     next();
   },
 
+  // proxies unauthenticated requests to CouchDB
   // proxies online users requests to CouchDB
   // saves offline user-settings doc in the request object
   onlineUserProxy: (proxy, req, res, next) => {
-    if (!req.userCtx) {
-      return serverUtils.notLoggedIn(req, res);
-    }
-
-    if (auth.isOnlineOnly(req.userCtx)) {
+    if (!req.userCtx || auth.isOnlineOnly(req.userCtx)) {
       return proxy.web(req, res);
     }
 
@@ -62,10 +75,6 @@ module.exports = {
   // saves offline user-settings doc in the request object
   // used for audited endpoints
   onlineUserPassThrough:(req, res, next) => {
-    if (!req.userCtx) {
-      return serverUtils.notLoggedIn(req, res);
-    }
-
     if (auth.isOnlineOnly(req.userCtx)) {
       return next('route');
     }
@@ -80,10 +89,6 @@ module.exports = {
   },
 
   getUserSettings: (req, res, next) => {
-    if (!req.userCtx) {
-      return serverUtils.notLoggedIn(req, res);
-    }
-
     if (auth.isOnlineOnly(req.userCtx)) {
       return next();
     }

api/src/middleware/connected-user-log.js

@@ -8,6 +8,11 @@ module.exports = {
       .getUserCtx(req)
       .then(({ name }) => connectedUserLogService.save(name))
       .catch(err => {
+        if (err && err.code === 401) {
+          // don't spam the logs with authentication errors
+          return;
+        }
+
         logger.error('Error recording user connection:', err);
       })
       .then(() => next());

api/src/routing.js

@@ -265,7 +265,7 @@ const ONLINE_ONLY_ENDPOINTS = [
 
 // block offline users from accessing some unaudited CouchDB endpoints
 ONLINE_ONLY_ENDPOINTS.forEach(url =>
-  app.all(routePrefix + url, authorization.offlineUserFirewall)
+  app.all(routePrefix + url, authorization.handleAuthErrors, authorization.offlineUserFirewall)
 );
 
 // allow anyone to access their session
@@ -431,12 +431,36 @@ app.postJson('/api/v1/people', function(req, res) {
 app.postJson('/api/v1/bulk-delete', bulkDocs.bulkDelete);
 
 // offline users are not allowed to hydrate documents via the hydrate API
-app.get('/api/v1/hydrate', authorization.offlineUserFirewall, jsonQueryParser, hydration.hydrate);
-app.post('/api/v1/hydrate', authorization.offlineUserFirewall, jsonParser, jsonQueryParser, hydration.hydrate);
+app.get(
+  '/api/v1/hydrate',
+  authorization.handleAuthErrors,
+  authorization.offlineUserFirewall,
+  jsonQueryParser,
+  hydration.hydrate
+);
+app.post(
+  '/api/v1/hydrate',
+  authorization.handleAuthErrors,
+  authorization.offlineUserFirewall,
+  jsonParser,
+  jsonQueryParser,
+  hydration.hydrate
+);
 
 // offline users are not allowed to get contacts by phone
-app.get('/api/v1/contacts-by-phone', authorization.offlineUserFirewall, contactsByPhone.request);
-app.post('/api/v1/contacts-by-phone', authorization.offlineUserFirewall, jsonParser, contactsByPhone.request);
+app.get(
+  '/api/v1/contacts-by-phone',
+  authorization.handleAuthErrors,
+  authorization.offlineUserFirewall,
+  contactsByPhone.request
+);
+app.post(
+  '/api/v1/contacts-by-phone',
+  authorization.handleAuthErrors,
+  authorization.offlineUserFirewall,
+  jsonParser,
+  contactsByPhone.request
+);
 
 app.get(`${appPrefix}app_settings/${environment.ddoc}/:path?`, settings.getV0); // deprecated
 app.get('/api/v1/settings', settings.get);
@@ -447,9 +471,24 @@ app.putJson('/api/v1/settings', settings.put);
 
 app.get('/api/couch-config-attachments', couchConfigController.getAttachments);
 
-app.get('/purging', authorization.onlineUserPassThrough, purgedDocsController.info);
-app.get('/purging/changes', authorization.onlineUserPassThrough, purgedDocsController.getPurgedDocs);
-app.get('/purging/checkpoint', authorization.onlineUserPassThrough, purgedDocsController.checkpoint);
+app.get(
+  '/purging',
+  authorization.handleAuthErrors,
+  authorization.onlineUserPassThrough,
+  purgedDocsController.info
+);
+app.get(
+  '/purging/changes',
+  authorization.handleAuthErrors,
+  authorization.onlineUserPassThrough,
+  purgedDocsController.getPurgedDocs
+);
+app.get(
+  '/purging/checkpoint',
+  authorization.handleAuthErrors,
+  authorization.onlineUserPassThrough,
+  purgedDocsController.checkpoint
+);
 
 app.get('/api/v1/users-doc-count', replicationLimitLogController.get);
 
@@ -467,11 +506,13 @@ const changesPath = routePrefix + '_changes(/*)?';
 
 app.get(
   changesPath,
+  authorization.handleAuthErrors,
   onlineUserChangesProxy,
   changesHandler
 );
 app.post(
   changesPath,
+  authorization.handleAuthErrors,
   onlineUserChangesProxy,
   jsonParser,
   changesHandler
@@ -481,9 +522,16 @@ app.post(
 const allDocsHandler = require('./controllers/all-docs').request;
 const allDocsPath = routePrefix + '_all_docs(/*)?';
 
-app.get(allDocsPath, onlineUserProxy, jsonQueryParser, allDocsHandler);
+app.get(
+  allDocsPath,
+  authorization.handleAuthErrors,
+  onlineUserProxy,
+  jsonQueryParser,
+  allDocsHandler
+);
 app.post(
   allDocsPath,
+  authorization.handleAuthErrors,
   onlineUserProxy,
   jsonParser,
   jsonQueryParser,
@@ -494,6 +542,7 @@ app.post(
 const bulkGetHandler = require('./controllers/bulk-get').request;
 app.post(
   routePrefix + '_bulk_get(/*)?',
+  authorization.handleAuthErrors,
   onlineUserProxy,
   jsonParser,
   jsonQueryParser,
@@ -506,6 +555,7 @@ app.post(
   routePrefix + '_bulk_docs(/*)?',
   jsonParser,
   infodoc.mark,
+  authorization.handleAuthErrors,
   authorization.onlineUserPassThrough, // online user requests pass through to the next route
   jsonQueryParser,
   bulkDocs.request,
@@ -521,6 +571,7 @@ const ddocPath = routePrefix + '_design/+:ddocId*';
 
 app.get(
   ddocPath,
+  authorization.handleAuthErrors,
   onlineUserProxy,
   jsonQueryParser,
   _.partial(dbDocHandler.requestDdoc, environment.ddoc),
@@ -529,6 +580,7 @@ app.get(
 
 app.get(
   docPath,
+  authorization.handleAuthErrors,
   onlineUserProxy, // online user GET requests are proxied directly to CouchDB
   jsonQueryParser,
   dbDocHandler.request
@@ -537,6 +589,7 @@ app.post(
   `/+${environment.db}/?`,
   jsonParser,
   infodoc.mark,
+  authorization.handleAuthErrors,
   authorization.onlineUserPassThrough, // online user requests pass through to the next route
   jsonQueryParser,
   dbDocHandler.request,
@@ -546,20 +599,23 @@ app.put(
   docPath,
   jsonParser,
   infodoc.mark,
+  authorization.handleAuthErrors,
   authorization.onlineUserPassThrough, // online user requests pass through to the next route,
   jsonQueryParser,
   dbDocHandler.request,
   authorization.setAuthorized // adds the `authorized` flag to the `req` object, so it passes the firewall
 );
 app.delete(
   docPath,
+  authorization.handleAuthErrors,
   authorization.onlineUserPassThrough, // online user requests pass through to the next route,
   jsonQueryParser,
   dbDocHandler.request,
   authorization.setAuthorized // adds the `authorized` flag to the `req` object, so it passes the firewall
 );
 app.all(
   attachmentPath,
+  authorization.handleAuthErrors,
   authorization.onlineUserPassThrough, // online user requests pass through to the next route
   jsonQueryParser,
   dbDocHandler.request,
@@ -677,6 +733,10 @@ proxyForChanges.on('proxyReq', (proxyReq, req) => {
 
 // because these are longpolls, we need to manually flush the CouchDB heartbeats through compression
 proxyForChanges.on('proxyRes', (proxyRes, req, res) => {
+  if (proxyRes.statusCode === 401) {
+    return serverUtils.notLoggedIn(req, res);
+  }
+
   copyProxyHeaders(proxyRes, res);
 
   proxyRes.pipe(res);
@@ -700,6 +760,7 @@ app.all(appPrefix + '*', authorization.setAuthorized);
 // block offline users requests from accessing CouchDB directly, via Proxy
 // requests which are authorized (fe: by BulkDocsHandler or DbDocHandler) can pass through
 // unauthenticated requests will be redirected to login or given a meaningful error
+app.use(authorization.handleAuthErrorsAllowingAuthorized);
 app.use(authorization.offlineUserFirewall);
 
 const canEdit = function(req, res) {
@@ -742,8 +803,18 @@ proxyForAuth.on('proxyReq', function(proxyReq, req) {
   writeParsedBody(proxyReq, req);
 });
 
+proxy.on('proxyRes', (proxyRes, req, res) => {
+  if (proxyRes.statusCode === 401) {
+    serverUtils.notLoggedIn(req, res);
+  }
+});
+
 // intercept responses from filtered offline endpoints to fill in with forbidden docs stubs
 proxyForAuth.on('proxyRes', (proxyRes, req, res) => {
+  if (proxyRes.statusCode === 401) {
+    return serverUtils.notLoggedIn(req, res);
+  }
+
   copyProxyHeaders(proxyRes, res);
 
   if (res.interceptResponse) {

api/src/server-utils.js

@@ -8,8 +8,10 @@ const MEDIC_BASIC_AUTH = 'Basic realm="Medic Mobile Web Services"';
 const wantsJSON = req => req.get('Accept') === 'application/json';
 
 const writeJSON = (res, code, error, details) => {
-  res.status(code);
-  res.json({ code, error, details });
+  if (!res.headersSent && !res.ended) {
+    res.status(code);
+    res.json({ code, error, details });
+  }
 };
 
 const respond = (req, res, code, message, details) => {
@@ -72,6 +74,10 @@ module.exports = {
    * an authentication error.
    */
   notLoggedIn: (req, res, showPrompt) => {
+    if (!res.headersSent) {
+      res.setHeader('logout-authorization', 'CHT-Core API');
+    }
+
     if (showPrompt) {
       // api access - basic auth allowed
       promptForBasicAuth(res);

api/tests/mocha/auth.spec.js

@@ -24,7 +24,7 @@ describe('Auth', () => {
 
     it('returns error when not logged in', () => {
       environment.serverUrl = 'http://abc.com';
-      const get = sinon.stub(request, 'get').resolves();
+      const get = sinon.stub(request, 'get').rejects({ statusCode: 401 });
       return auth.check({ }).catch(err => {
         chai.expect(get.callCount).to.equal(1);
         chai.expect(get.args[0][0].url).to.equal('http://abc.com/_session');
@@ -33,24 +33,34 @@ describe('Auth', () => {
       });
     });
 
+    it('returns error with incomplete session', () => {
+      environment.serverUrl = 'http://abc.com';
+      const get = sinon.stub(request, 'get').resolves();
+      return auth.check({ }).catch(err => {
+        chai.expect(get.callCount).to.equal(1);
+        chai.expect(get.args[0][0].url).to.equal('http://abc.com/_session');
+        chai.expect(err.message).to.equal('Failed to authenticate');
+        chai.expect(err.code).to.equal(500);
+      });
+    });
+
     it('returns error when no user context', () => {
       environment.serverUrl = 'http://abc.com';
       const get = sinon.stub(request, 'get').resolves({ roles: [] });
       return auth.check({ }).catch(err => {
         chai.expect(get.callCount).to.equal(1);
-        chai.expect(err.message).to.equal('Not logged in');
-        chai.expect(err.code).to.equal(401);
+        chai.expect(err.message).to.equal('Failed to authenticate');
+        chai.expect(err.code).to.equal(500);
       });
     });
 
     it('returns error when request errors', () => {
       environment.serverUrl = 'http://abc.com';
-      const get = sinon.stub(request, 'get').rejects('boom');
+      const get = sinon.stub(request, 'get').rejects({ error: 'boom' });
       return auth.check({ }).catch(err => {
         chai.expect(get.callCount).to.equal(1);
         chai.expect(get.args[0][0].url).to.equal('http://abc.com/_session');
-        chai.expect(err.message).to.equal('Not logged in');
-        chai.expect(err.code).to.equal(401);
+        chai.expect(err).to.deep.equal({ error: 'boom' });
       });
     });