CUMULUS-1748: support relative paths in recursion.js

bamboo/bootstrap-unit-tests.sh

@@ -27,7 +27,7 @@ while ! docker container inspect ${container_id}\_build_env_1; do
 done
 
 ## Setup the build env container once it's started
-$docker_command "npm install --error --no-progress -g nyc; cd $UNIT_TEST_BUILD_DIR; git fetch --all; git checkout $GIT_SHA; npm install --error --no-progress; npm run bootstrap-no-build-quiet || true; npm run bootstrap-no-build-quiet; chmod 0400 -R $UNIT_TEST_BUILD_DIR/packages/test-data/keys"
+$docker_command "npm install --error --no-progress -g nyc; cd $UNIT_TEST_BUILD_DIR; git fetch --all; git checkout $GIT_SHA; npm install --error --no-progress; npm run bootstrap-no-build-quiet || true; npm run bootstrap-no-build-quiet"
 
 # Wait for the FTP server to be available
 while ! $docker_command  'curl --connect-timeout 5 -sS -o /dev/null ftp://testuser:[email protected]/README.md'; do
@@ -45,10 +45,11 @@ while ! $docker_command  'curl --connect-timeout 5 -sS -o /dev/null http://127.0
 done
 echo 'HTTP service is available'
 
+$docker_command "mkdir /keys;cp $UNIT_TEST_BUILD_DIR/packages/test-data/keys/ssh_client_rsa_key /keys/; chmod -R 400 /keys"
 # Wait for the SFTP server to be available
 while ! $docker_command "sftp \
   -P 2222\
-  -i $UNIT_TEST_BUILD_DIR/packages/test-data/keys/ssh_client_rsa_key\
+  -i /keys/ssh_client_rsa_key\
   -o 'ConnectTimeout=5'\
   -o 'StrictHostKeyChecking=no'\
   -o 'UserKnownHostsFile=/dev/null'\

bamboo/deploy-dev-integration-test-stack.sh

@@ -1,8 +1,8 @@
 #!/bin/bash
 set -ex
+. ./bamboo/set-bamboo-env-variables.sh
 . ./bamboo/abort-if-not-pr-or-redeployment.sh
 . ./bamboo/abort-if-skip-integration-tests.sh
-. ./bamboo/set-bamboo-env-variables.sh
 
 npm config set unsafe-perm true
 

packages/common/sftp.js

@@ -163,10 +163,9 @@ class Sftp {
    * @returns {Promise.<Object>} - list of file object
    */
   async list(remotePath) {
-    const normalizedPath = (remotePath.trim() === '' ? '/' : remotePath);
     if (!this.connected) await this.connect();
     return new Promise((resolve, reject) => {
-      this.sftp.readdir(normalizedPath, (err, list) => {
+      this.sftp.readdir(remotePath, (err, list) => {
         if (err) {
           if (err.message.includes('No such file')) {
             return resolve([]);
@@ -175,7 +174,7 @@ class Sftp {
         }
         return resolve(list.map((i) => ({
           name: i.filename,
-          path: normalizedPath,
+          path: remotePath,
           type: i.longname.substr(0, 1),
           size: i.attrs.size,
           time: i.attrs.mtime * 1000

packages/ingest/FtpProviderClient.js

@@ -8,6 +8,7 @@ const log = require('@cumulus/common/log');
 const omit = require('lodash.omit');
 const S3 = require('@cumulus/aws-client/S3');
 const { S3KeyPairProvider } = require('@cumulus/common/key-pair-provider');
+const { isNil } = require('@cumulus/common/util');
 const recursion = require('./recursion');
 const { lookupMimeType } = require('./util');
 
@@ -51,13 +52,31 @@ class FtpProviderClient {
   }
 
   async buildFtpClient() {
-    return new JSFtp({
-      host: this.host,
-      port: get(this.providerConfig, 'port', 21),
-      user: await this.getUsername(),
-      pass: await this.getPassword(),
-      useList: get(this.providerConfig, 'useList', false)
-    });
+    if (isNil(this.ftpClient)) {
+      this.ftpClient = new JSFtp({
+        host: this.host,
+        port: get(this.providerConfig, 'port', 21),
+        user: await this.getUsername(),
+        pass: await this.getPassword(),
+        useList: get(this.providerConfig, 'useList', false)
+      });
+    }
+    return this.ftpClient;
+  }
+
+  errorHandler(rejectFn, e) {
+    let err;
+    if (!e.message && e.text) {
+      const message = (e.code
+      ? `FTP Code ${e.code}: ${e.text}`
+      : `FTP error: ${e.text}`) + ' This may be caused by user permissions disallowing the listing.';
+      err = new Error(message);
+    }
+    if (!isNil(this.ftpClient)) {
+      this.ftpClient.destroy();
+    }
+    log.error('FtpProviderClient encountered error: ', err);
+    return rejectFn(err);
   }
 
   /**
@@ -74,15 +93,13 @@ class FtpProviderClient {
     const client = await this.buildFtpClient();
 
     return new Promise((resolve, reject) => {
-      client.on('error', reject);
+      client.on('error', this.errorHandler.bind(this, reject));
       client.get(remotePath, localPath, (err) => {
-        client.destroy();
-
-        if (err) reject(err);
-        else {
-          log.info(`Finishing downloading ${remoteUrl}`);
-          resolve(localPath);
+        if (err) {
+          return this.errorHandler(reject, err);
         }
+        log.info(`Finishing downloading ${remoteUrl}`);
+        return resolve(localPath);
       });
     });
   }
@@ -91,19 +108,19 @@ class FtpProviderClient {
     let counter = _counter;
     const client = await this.buildFtpClient();
     return new Promise((resolve, reject) => {
-      client.on('error', reject);
+      client.on('error', this.errorHandler.bind(this, reject));
       client.ls(path, (err, data) => {
-        client.destroy();
         if (err) {
-          if (err.message.includes('Timed out') && counter < 3) {
+          const message = err.message || err.text;
+          if (message && message.includes('Timed out') && counter < 3) {
             log.error(`Connection timed out while listing ${path}. Retrying...`);
             counter += 1;
             return this._list(path, counter).then((r) => {
-              log.info(`${counter} retry suceeded`);
+              log.info(`${counter} retry succeeded`);
               return resolve(r);
-            }).catch((e) => reject(e));
+            }).catch(this.errorHandler.bind(this, reject));
           }
-          return reject(err);
+          return this.errorHandler(reject, err);
         }
 
         return resolve(data.map((d) => ({
@@ -153,8 +170,7 @@ class FtpProviderClient {
     const readable = await new Promise((resolve, reject) => {
       client.get(remotePath, (err, socket) => {
         if (err) {
-          client.destroy();
-          return reject(err);
+          return this.errorHandler(reject, err);
         }
         return resolve(socket);
       });
@@ -172,8 +188,6 @@ class FtpProviderClient {
     await S3.promiseS3Upload(params);
     log.info('Uploading to s3 is complete(ftp)', s3uri);
 
-    client.destroy();
-
     return s3uri;
   }
 }

packages/ingest/S3ProviderClient.js

@@ -4,7 +4,12 @@ const aws = require('@cumulus/common/aws');
 const log = require('@cumulus/common/log');
 const errors = require('@cumulus/common/errors');
 const isString = require('lodash.isstring');
-const { basename, dirname } = require('path');
+const {
+  basename,
+  dirname,
+  isAbsolute,
+  normalize
+} = require('path');
 
 class S3ProviderClient {
   constructor({ bucket }) {
@@ -41,11 +46,13 @@ class S3ProviderClient {
    * @returns {Promise<Array>} a list of files
    * @private
    */
-  async list(path) {
+  async list(path = '') {
+    // absolute paths are not valid S3 prefixes
+    const prefix = isAbsolute(path) ? normalize(path.slice(1)) : normalize(path);
     const objects = await aws.listS3ObjectsV2({
       Bucket: this.bucket,
       FetchOwner: true,
-      Prefix: path
+      Prefix: prefix
     });
 
     return objects.map(({ Key, Size, LastModified }) => ({

packages/ingest/SftpProviderClient.js

@@ -85,7 +85,6 @@ class SftpProviderClient {
         privateKey: await this.getPrivateKey()
       });
     }
-
     return this.sftpClientInstance;
   }
 

packages/ingest/recursion.js

@@ -1,20 +1,8 @@
 'use strict';
 
+const path = require('path');
 const log = require('@cumulus/common/log');
 
-/**
- * Insert leading and remove terminating slashes into/from the path string
- *
- * @param {string} path - path string
- * @returns {string} normalized path
- */
-const normalizeSlashes = (path) => {
-  let output = path.replace(/[\/]{2,}/g, '/');
-  if (!output.startsWith('/')) output = `/${output}`;
-  if (output.endsWith('/')) output = output.slice(0, -1);
-  return output;
-};
-
 /**
  * Recur on directory, list all files, and recur into any further directories,
  * as specified regex segments allow.
@@ -40,7 +28,7 @@ async function recurOnDirectory(fn, currentPath, segments, position) {
       if (['-', 0].includes(item.type)) {
         files.push(item);
       } else if (['d', 1].includes(item.type)) {
-        const nextDir = (currentPath === '' ? item.name : `${currentPath}/${item.name}`);
+        const nextDir = (currentPath === '/' ? `/${item.name}` : `${currentPath}/${item.name}`);
         // eslint-disable-next-line no-await-in-loop
         files = files.concat(await recurOnDirectory(fn, nextDir, segments, position + 1));
       }
@@ -54,28 +42,37 @@ async function recurOnDirectory(fn, currentPath, segments, position) {
  * It requests a promisified list function that returns contents of
  * a directory on a server, filtering on provided regex segments.
  *
- * Note that calls to the list function will not have leading or terminating slashes.
- * Initially an empty string is passed as the path to list the default directory. Following calls
- * based on items discovered will be of the format `fn('path/to/files')`, again with no leading or
- * terminating slashes.
+ * Note that calls to the list function will use either a relative or absolute path, corresponding
+ * to the `configuredPath` passed into this function. The list function will initially be called
+ * with '.' for a relative path or '/' for an absolute path. List functions will need to be able to
+ * normalize or correct these paths as appropriate for their protocol.
+ *
+ * Further calls to the list functions will append the current path to that starting path, such
+ * that all calls will start with either '.' or '/', regardless of additional characters, e.g.
+ * `fn('./path')` vs. `fn('path')`.
  *
- * List functions will need to be able to normalize or correct these paths as appropriate for their
- * protocol.
+ * In the case of failure during the recursive list, this function will only apply `path.normalize`
+ * to the `configuredPath` and then call the list function with the entire normalizedPath.
  *
  * @param {function} fn - the promisified function for listing a directory
- * @param {string} originalPath - path string which may contain regexes for filtering
+ * @param {string} configuredPath - path string configured by operator, which may contain
+ *                                  regexes for filtering
  * @returns {Promise} the promise of an object that has the path is the key and
  *   list of files as values
  */
-async function recursion(fn, originalPath) {
-  const normalizedPath = normalizeSlashes(originalPath);
+async function recursion(fn, configuredPath) {
+  const normalizedPath = path.normalize(configuredPath);
+  const isAbsolutePath = path.isAbsolute(normalizedPath);
   try {
-    const segments = normalizedPath.split('/'); // split on divider
-    return await recurOnDirectory(fn, segments[0], segments, 0);
+    const segments = normalizedPath
+      .split('/') // split on divider
+      .filter((segment) => segment.trim() !== ''); // filter out empty strings from split
+    const startingPath = isAbsolutePath ? '/' : '.';
+    return await recurOnDirectory(fn, startingPath, segments, -1);
   } catch (e) {
     log.error(`Encountered error during recursive list filtering: ${e}`);
     log.info('Falling back to unfiltered directory listing...');
-    return recurOnDirectory(fn, normalizedPath.slice(1), [], 0);
+    return recurOnDirectory(fn, normalizedPath, [], 0);
   }
 }
 

packages/ingest/test/test-FtpProviderClient.js

@@ -87,21 +87,22 @@ test('FtpProviderClient supports plaintext usernames and passwords', async (t) =
   t.true(fileNames.includes('index.html'));
 });
 
-test('FtpProviderClient supports S3-keypair-encrypted usernames and passwords', async (t) => {
-  const ftpClient = new FtpProviderClient({
-    host: '127.0.0.1',
-    encrypted: true,
-    username: await S3KeyPairProvider.encrypt('testuser'),
-    password: await S3KeyPairProvider.encrypt('testpass'),
-    useList: true
+test('FtpProviderClient supports S3-keypair-encrypted usernames and passwords',
+  async (t) => {
+    const ftpClient = new FtpProviderClient({
+      host: '127.0.0.1',
+      encrypted: true,
+      username: await S3KeyPairProvider.encrypt('testuser'),
+      password: await S3KeyPairProvider.encrypt('testpass'),
+      useList: true
+    });
+
+    const files = await ftpClient.list('/');
+    const fileNames = files.map((f) => f.name);
+
+    t.true(fileNames.includes('index.html'));
   });
 
-  const files = await ftpClient.list('/');
-  const fileNames = files.map((f) => f.name);
-
-  t.true(fileNames.includes('index.html'));
-});
-
 test('FtpProviderClient supports KMS-encrypted usernames and passwords', async (t) => {
   const ftpClient = new FtpProviderClient({
     host: '127.0.0.1',
@@ -132,7 +133,7 @@ test('useList is present and true when assigned', async (t) => {
 
   await myFtpProviderClient.list('');
 
-  t.true(jsftpSpy.callCount > 1);
+  t.true(jsftpSpy.callCount > 0);
   t.is(jsftpSpy.getCall(0).args[0].useList, true);
 });
 
@@ -150,11 +151,11 @@ test('useList defaults to false when not assigned', async (t) => {
 
   await myFtpProviderClient.list('');
 
-  // TODO figure out why STAT does not list any results on our local FTP server
-  t.is(jsftpSpy.callCount, 1);
+  t.true(jsftpSpy.callCount > 0);
   t.is(jsftpSpy.getCall(0).args[0].useList, false);
 });
 
+
 test('Download remote file to s3 with correct content-type', async (t) => {
   const myFtpProviderClient = new FtpProviderClient({
     host: '127.0.0.1',

packages/ingest/test/test-FtpProviderClientFailure.js

@@ -0,0 +1,45 @@
+'use strict';
+
+const test = require('ava');
+const JSFtp = require('jsftp');
+
+const FtpProviderClient = require('../FtpProviderClient');
+
+const executeRawFtpCommand = async (cmd) => {
+  const jsftp = new JSFtp({
+    host: '127.0.0.1',
+    port: 21,
+    user: 'testuser',
+    pass: 'testpass'
+  });
+  return new Promise((resolve, reject) => {
+    jsftp.raw(cmd, (err, data) => {
+      if (err) reject(err);
+      resolve(data);
+    });
+  });
+};
+
+test.before(async () => {
+  // await when fixed
+  executeRawFtpCommand('chmod 400 -R forbidden/file.txt');
+});
+
+test.after.always(async () => {
+  // await when fixed
+  executeRawFtpCommand('chmod 644 -R forbidden/file.txt');
+});
+
+test.skip('FtpProviderClient throws an error when listing a non-permitted directory', async (t) => {
+  // TODO: update cumuluss/vsftpd to allow `chmod` to work in the setup for this test.
+  const myFtpProviderClient = new FtpProviderClient({
+    host: '127.0.0.1',
+    username: 'testuser',
+    password: 'testpass',
+    useList: true
+  });
+
+  await t.throwsAsync(myFtpProviderClient.list('forbidden/file.txt'),
+    'FTP Code 451: Could not retrieve a file listing for forbidden/file.txt.'
+    + ' This may be caused by user permissions disallowing the listing.');
+});