Merge pull request #339 from hms-dbmi-cellenics/patch-cs

[BIOMAGE-1824] Missing clusters issue

package.json

@@ -41,6 +41,7 @@
     "express-openapi-validator": "^4.10.8",
     "github-download-directory": "^2.0.0",
     "ioredis": "^4.17.3",
+    "ipaddr.js": "^2.0.1",
     "js-yaml": "^3.13.1",
     "json-merger": "^1.1.7",
     "jsonwebtoken": "^8.5.1",

src/utils/authMiddlewares.js

@@ -8,7 +8,6 @@ const jwt = require('jsonwebtoken');
 const jwtExpress = require('express-jwt');
 const jwkToPem = require('jwk-to-pem');
 const util = require('util');
-const dns = require('dns').promises;
 
 const config = require('../config');
 
@@ -18,6 +17,7 @@ const { CacheMissError } = require('../cache/cache-utils');
 const { UnauthorizedError, UnauthenticatedError } = require('./responses');
 const ProjectsService = require('../api/route-services/projects');
 const AccessService = require('../api/route-services/access');
+const { isReqFromLocalhost, isReqFromCluster } = require('./isReqFrom');
 
 const accessService = new AccessService();
 const projectService = new ProjectsService();
@@ -82,32 +82,7 @@ const authenticationMiddlewareExpress = async (app) => {
   });
 };
 
-
-// eslint-disable-next-line no-useless-escape
-const INTERNAL_DOMAINS_REGEX = new RegExp('((\.compute\.internal)|(\.svc\.local))$');
-
 const checkAuthExpiredMiddleware = (req, res, next) => {
-  const isReqFromLocalhost = async () => {
-    const ip = req.connection.remoteAddress;
-    const host = req.get('host');
-
-    if (ip === '127.0.0.1' || ip === '::ffff:127.0.0.1' || ip === '::1' || host.indexOf('localhost') !== -1) {
-      return true;
-    }
-
-    throw new Error('ip address is not localhost');
-  };
-
-  const isReqFromCluster = async () => {
-    const domains = await dns.reverse(req.ip);
-
-    if (!domains.some((domain) => INTERNAL_DOMAINS_REGEX.test(domain))) {
-      throw new Error('ip address does not come from internal sources');
-    }
-
-    return true;
-  };
-
   if (!req.user) {
     return next();
   }
@@ -125,8 +100,9 @@ const checkAuthExpiredMiddleware = (req, res, next) => {
     return next(new UnauthenticatedError('token has expired'));
   }
 
+
   // check if we should ignore expired jwt token for this path and request type
-  const longTimeoutEndpoints = [{ urlMatcher: /^\/v1\/experiments\/.{32}\/cellSets$/, method: 'PATCH' }];
+  const longTimeoutEndpoints = [{ urlMatcher: /experiments\/.{32}\/cellSets$/, method: 'PATCH' }];
   const isEndpointIgnored = longTimeoutEndpoints.some(
     ({ urlMatcher, method }) => (
       req.method.toLowerCase() === method.toLowerCase() && urlMatcher.test(req.url)
@@ -135,15 +111,13 @@ const checkAuthExpiredMiddleware = (req, res, next) => {
 
   // if endpoint is not in ignore list, the JWT is too old, send an error accordingly
   if (!isEndpointIgnored) {
-    return next(new UnauthenticatedError('token has expired'));
+    return next(new UnauthenticatedError('token has expired for non-ignored endpoint'));
   }
 
-  promiseAny([isReqFromCluster(), isReqFromLocalhost()])
-    .then(() => {
-      next();
-    })
-    .catch(() => {
-      next(new UnauthenticatedError('token has expired'));
+  promiseAny([isReqFromCluster(req), isReqFromLocalhost(req)])
+    .then(() => next())
+    .catch((e) => {
+      next(new UnauthenticatedError(`invalid request origin ${e}`));
     });
 
   return null;

src/utils/isReqFrom.js

@@ -0,0 +1,41 @@
+const dns = require('dns').promises;
+const ipaddr = require('ipaddr.js');
+
+// eslint-disable-next-line no-useless-escape
+const INTERNAL_DOMAINS_REGEX = new RegExp('((\.compute\.internal)|(\.svc\.local))$');
+
+const isReqFromLocalhost = async (req) => {
+  const ip = req.connection.remoteAddress;
+  const host = req.get('host');
+
+  if (ip === '127.0.0.1' || ip === '::ffff:127.0.0.1' || ip === '::1' || host.indexOf('localhost') !== -1) {
+    return true;
+  }
+
+  throw new Error('ip address is not localhost');
+};
+
+const isReqFromCluster = async (req) => {
+  let remoteAddress = req.ip;
+  const addr = ipaddr.parse(req.ip);
+  // req.ip returns IPv4 addresses mapped to IPv6, e.g.:
+  // 127.0.0.1 (IPv4) -> ::ffff:127.0.0.1 (IPv6)
+  // dns.reverse is not capable of dealing with them,
+  // it either uses IPv4 or IPv6, so we need to map those
+  // IPs back to IPv4 before.
+  if (addr.kind() === 'ipv6' && addr.isIPv4MappedAddress()) {
+    remoteAddress = addr.toIPv4Address().toString();
+  }
+
+  const domains = await dns.reverse(remoteAddress);
+  if (domains.some((domain) => INTERNAL_DOMAINS_REGEX.test(domain))) {
+    return true;
+  }
+
+  throw new Error('ip address does not come from internal sources');
+};
+
+module.exports = {
+  isReqFromCluster,
+  isReqFromLocalhost,
+};

tests/utils/authMiddlewares.test.js

@@ -1,5 +1,7 @@
 const AWSMock = require('aws-sdk-mock');
+
 const {
+  checkAuthExpiredMiddleware,
   expressAuthorizationMiddleware,
   authorize,
 } = require('../../src/utils/authMiddlewares');
@@ -52,6 +54,22 @@ describe('Tests for authorization/authentication middlewares', () => {
     expect(next).toBeCalledWith();
   });
 
+  it('checkAuth accepts expired tokens for patch cellsets', async () => {
+    mockDynamoGetItem(data);
+
+    const req = {
+      params: { experimentId: fake.EXPERIMENT_ID },
+      user: fake.USER,
+      url: `/v1/experiments/${fake.EXPERIMENT_ID}/cellSets`,
+      method: 'PATCH',
+      ip: '::ffff:127.0.0.1',
+    };
+    const next = jest.fn();
+
+    const ret = checkAuthExpiredMiddleware(req, {}, next);
+    expect(ret).toBe(null);
+  });
+
   it('Express middleware can reject incorrect users', async () => {
     mockDynamoGetItem({});
 

tests/utils/isReqFrom.test.js

@@ -0,0 +1,77 @@
+
+
+jest.mock('dns', () => ({
+  promises: {
+    reverse: jest.fn((ip) => {
+      if (ip === '127.0.0.1') return [];
+      return ['ip-192-168-905-123.region.compute.internal'];
+    }),
+  },
+}));
+
+const {
+  isReqFromCluster,
+  isReqFromLocalhost,
+} = require('../../src/utils/isReqFrom');
+const fake = require('../test-utils/constants');
+
+describe('Tests for isRequestFromCluster and isRequestFromLocalhost', () => {
+  it('isReqFromLocalhost returns true for localhost', async () => {
+    const req = {
+      params: { experimentId: fake.EXPERIMENT_ID },
+      user: fake.USER,
+      url: `/v1/experiments/${fake.EXPERIMENT_ID}/cellSets`,
+      method: 'PATCH',
+      connection: {
+        address: '::ffff:127.0.0.1',
+      },
+      get: () => 'localhost',
+    };
+
+    expect(await isReqFromLocalhost(req)).toEqual(true);
+  });
+
+  it('isRequestFromLocalhost throws with cluster addr', async () => {
+    const req = {
+      params: { experimentId: fake.EXPERIMENT_ID },
+      user: fake.USER,
+      url: `/v1/experiments/${fake.EXPERIMENT_ID}/cellSets`,
+      method: 'PATCH',
+      ip: '::ffff:192.0.0.1',
+      connection: {
+        address: '::ffff:192.0.0.1',
+      },
+      get: () => 'whatever.internal.compute',
+    };
+
+    await expect(isReqFromLocalhost(req))
+      .rejects
+      .toThrow();
+  });
+
+  it('isReqFromCluster returns true for cluster addr', async () => {
+    const req = {
+      params: { experimentId: fake.EXPERIMENT_ID },
+      user: fake.USER,
+      url: `/v1/experiments/${fake.EXPERIMENT_ID}/cellSets`,
+      method: 'PATCH',
+      ip: '::ffff:192.0.0.1',
+    };
+
+    expect(await isReqFromCluster(req)).toEqual(true);
+  });
+
+  it('isRequestFromCluster throws with localhost', async () => {
+    const req = {
+      params: { experimentId: fake.EXPERIMENT_ID },
+      user: fake.USER,
+      url: `/v1/experiments/${fake.EXPERIMENT_ID}/cellSets`,
+      method: 'PATCH',
+      ip: '::ffff:127.0.0.1',
+    };
+
+    await expect(isReqFromCluster(req))
+      .rejects
+      .toThrow();
+  });
+});