fix: cleanup keytar (#209)

twilio · Aug 5, 2022 · 9f1c2d9 · 9f1c2d9
1 parent b5150dd
commit 9f1c2d9
Show file tree

Hide file tree

Showing 10 changed files with 30 additions and 195 deletions.
diff --git a/README.md b/README.md
@@ -41,7 +41,3 @@ Manages the CLI configuration options, such as Twilio profiles and credentials.
 ### Logger
 
 Standardizes logging output of debug, info, warning, and error messages to stderr (all go to stderr to allow differentiation between command output and logging messages).
-
-### SecureStorage
-
-An abstraction around the keytar npm package which further abstracts platform-level data encryption services for storing Twilio credentials securely.
diff --git a/package.json b/package.json
@@ -64,9 +64,6 @@
     "sinon": "^9.0.2",
     "tmp": "^0.2.1"
   },
-  "optionalDependencies": {
-    "keytar": "^7.6.0"
-  },
   "engines": {
     "node": ">=14.0.0"
   }

diff --git a/src/base-commands/base-command.js b/src/base-commands/base-command.js
@@ -8,7 +8,6 @@ const { TwilioCliError } = require('../services/error');
 const { logger, LoggingLevel } = require('../services/messaging/logging');
 const { OutputFormats } = require('../services/output-formats');
 const { getCommandPlugin, requireInstall } = require('../services/require-install');
-const { SecureStorage } = require('../services/secure-storage');
 const { instanceOf } = require('../services/javascript-utilities');
 
 let inquirer; // We'll lazy-load this only when it's needed.
@@ -21,7 +20,6 @@ class BaseCommand extends Command {
     super(argv, config);
     this.configFile = new Config('');
     this.userConfig = new ConfigData();
-    this.secureStorage = new SecureStorage({ command: this });
   }
 
   get inquirer() {

diff --git a/src/base-commands/twilio-client-command.js b/src/base-commands/twilio-client-command.js
@@ -31,7 +31,6 @@ class TwilioClientCommand extends BaseCommand {
       );
     }
     this.currentProfile = this.userConfig.getProfileById(this.flags.profile);
-    let keytarFlag = false;
     const pluginName = (this.config.userAgent || ' ').split(' ')[0];
 
     const reportUnconfigured = (verb, message = '', commandName = 'create') => {
@@ -55,17 +54,11 @@ class TwilioClientCommand extends BaseCommand {
     this.logger.debug(`Using profile: ${this.currentProfile.id}`);
 
     if (!this.currentProfile.apiKey || !this.currentProfile.apiSecret) {
-      const creds = await this.secureStorage.getCredentials(this.currentProfile.id);
-      if (creds.apiKey === 'error') {
-        this.logger.error(`Could not get credentials for profile "${this.currentProfile.id}".`);
-        reportUnconfigured('reconfigure');
-      }
-      this.currentProfile.apiKey = creds.apiKey;
-      this.currentProfile.apiSecret = creds.apiSecret;
-      keytarFlag = true;
+      this.logger.error(`Could not get credentials for profile "${this.currentProfile.id}".`);
+      reportUnconfigured('reconfigure');
     }
 
-    this.httpClient = new CliRequestClient(this.id, this.logger, undefined, keytarFlag, pluginName);
+    this.httpClient = new CliRequestClient(this.id, this.logger, undefined, pluginName);
   }
 
   async catch(error) {

diff --git a/src/index.js b/src/index.js
@@ -13,7 +13,6 @@ module.exports = {
     templating: require('./services/messaging/templating'),
     namingConventions: require('./services/naming-conventions'),
     outputFormats: require('./services/output-formats'),
-    secureStorage: require('./services/secure-storage'),
   },
   configureEnv: require('./services/env'),
   releaseScripts: {

diff --git a/src/services/cli-http-client.js b/src/services/cli-http-client.js
@@ -14,7 +14,7 @@ const NETWORK_ERROR_CODES = new Set(['ETIMEDOUT', 'ESOCKETTIMEDOUT', 'ECONNABORT
 const STANDARD_HEADERS = ['user-agent', 'accept-charset', 'connection', 'authorization', 'accept', 'content-type'];
 
 class CliRequestClient {
-  constructor(commandName, logger, http, keytarFlag = false, extensions = ' ') {
+  constructor(commandName, logger, http, extensions = ' ') {
     this.commandName = commandName;
     this.logger = logger;
     this.pluginName = extensions;
@@ -27,7 +27,6 @@ class CliRequestClient {
       this.http.defaults.proxy = false;
       this.http.defaults.httpsAgent = new HttpsProxyAgent(process.env.HTTP_PROXY);
     }
-    this.keytarWord = keytarFlag ? 'keytar' : '';
   }
 
   /**
@@ -72,7 +71,6 @@ class CliRequestClient {
     componentInfo.push(userAgentArr[0]); // Api client version
     componentInfo.push(userAgentArr[3]); // nodejs version
     componentInfo.push(this.commandName); // cli-command
-    componentInfo.push(this.keytarWord); // keytar flag
     headers['User-Agent'] = `${this.pluginName} ${pkg.name}/${pkg.version} ${componentInfo.filter(Boolean).join(' ')}`;
 
     const options = {

diff --git a/src/services/secure-storage.js b/src/services/secure-storage.js
diff --git a/test/base-commands/twilio-client-command.test.js b/test/base-commands/twilio-client-command.test.js
@@ -36,7 +36,6 @@ describe('base-commands', () => {
       args = [],
       {
         setUpUserConfig = undefined,
-        mockSecureStorage = true,
         commandClass: CommandClass = TestClientCommand,
         envRegion,
         envEdge,
@@ -64,24 +63,28 @@ describe('base-commands', () => {
           if (setUpUserConfig) {
             setUpUserConfig(ctx.userConfig);
           } else {
-            ctx.userConfig.addProfile('MyFirstProfile', constants.FAKE_ACCOUNT_SID);
-            ctx.userConfig.addProfile('region-edge-testing', constants.FAKE_ACCOUNT_SID, configRegion);
+            ctx.userConfig.addProfile(
+              'MyFirstProfile',
+              constants.FAKE_ACCOUNT_SID,
+              undefined,
+              constants.FAKE_API_KEY,
+              `${constants.FAKE_API_SECRET}MyFirstProfile`,
+            );
+            ctx.userConfig.addProfile(
+              'region-edge-testing',
+              constants.FAKE_ACCOUNT_SID,
+              configRegion,
+              constants.FAKE_API_KEY,
+              `${constants.FAKE_API_SECRET}region-edge-testing`,
+            );
+            ctx.userConfig.addProfile('no-credentials-profile', constants.FAKE_ACCOUNT_SID, configRegion);
             ctx.userConfig.setActiveProfile('MyFirstProfile');
           }
         })
         .twilioCliEnv(Config)
         .stderr()
         .do(async (ctx) => {
           ctx.testCmd = new CommandClass(args, ctx.fakeConfig);
-          ctx.testCmd.secureStorage = {
-            async getCredentials(profileId) {
-              return {
-                apiKey: mockSecureStorage ? constants.FAKE_API_KEY : 'error',
-                apiSecret: constants.FAKE_API_SECRET + profileId,
-              };
-            },
-          };
-
           // This is essentially what oclif does behind the scenes.
           try {
             await ctx.testCmd.run();
@@ -169,12 +172,12 @@ describe('base-commands', () => {
       expect(ctx.testCmd.twilioClient.region).to.equal('configRegion');
     });
 
-    setUpTest(['-p', 'region-edge-testing'], { mockSecureStorage: false })
+    setUpTest(['-p', 'no-credentials-profile'])
       .exit(1)
-      .it('should handle a secure storage error', (ctx) => {
-        expect(ctx.stderr).to.contain('Could not get credentials for profile "region-edge-testing"');
+      .it('should handle no profile error', (ctx) => {
+        expect(ctx.stderr).to.contain('Could not get credentials for profile "no-credentials-profile"');
         expect(ctx.stderr).to.contain('To reconfigure the profile, run:');
-        expect(ctx.stderr).to.contain('twilio profiles:create --profile "region-edge-testing"');
+        expect(ctx.stderr).to.contain('twilio profiles:create --profile "no-credentials-profile"');
       });
 
     setUpTest([], { commandClass: ThrowingUnknownClientCommand })

diff --git a/test/services/cli-http-client.test.js b/test/services/cli-http-client.test.js
@@ -21,11 +21,9 @@ describe('services', () => {
           expect(options.url).to.equal('https://foo.com/bar');
           return { status: 200, data: 'foo', headers: {} };
         },
-        true,
         'blah',
       );
       expect(client.commandName).to.equal('blah');
-      expect(client.keytarWord).to.equal('keytar');
       expect(client.pluginName).to.equal('blah');
       const response = await client.request({
         method: 'POST',
@@ -43,9 +41,8 @@ describe('services', () => {
 
     test.it('should add the correct http agent for proxy', async () => {
       process.env.HTTP_PROXY = 'http://someproxy.com:8080';
-      const client = new CliRequestClient('blah', logger, { defaults: {} }, false, 'blah');
+      const client = new CliRequestClient('blah', logger, { defaults: {} }, 'blah');
       const httpAgent = client.http.defaults.httpsAgent;
-      expect(client.keytarWord).to.equal('');
       expect(client.pluginName).to.equal('blah');
       expect(httpAgent.proxy.host).to.equal('someproxy.com');
       expect(httpAgent.proxy.port).to.equal(8080);
@@ -74,38 +71,35 @@ describe('services', () => {
     test
       .nock('https://foo.com', (api) => {
         api.get('/bar').reply(200, '', {
-          'User-Agent': `twilio-cli/2.27.1 ${pkg.name}\/${
-            pkg.version
-          } \(${os.platform()} ${os.arch()}\) dummyCommand keytar`,
+          'User-Agent': `twilio-cli/2.27.1 ${pkg.name}\/${pkg.version} \(${os.platform()} ${os.arch()}\) dummyCommand`,
         });
       })
       .it('correctly sets user-agent', async () => {
-        const client = new CliRequestClient('dummyCommand', logger, '', true, 'twilio-cli/2.27.1');
+        const client = new CliRequestClient('dummyCommand', logger, '', 'twilio-cli/2.27.1');
         const response = await client.request({
           method: 'GET',
           uri: 'https://foo.com/bar',
         });
-        expect(client.keytarWord).to.equal('keytar');
         expect(client.lastRequest.headers['User-Agent']).to.equal(
-          `twilio-cli/2.27.1 ${pkg.name}\/${pkg.version} \(${os.platform()} ${os.arch()}\) dummyCommand keytar`,
+          `twilio-cli/2.27.1 ${pkg.name}\/${pkg.version} \(${os.platform()} ${os.arch()}\) dummyCommand`,
         );
         expect(response.statusCode).to.equal(200);
       });
 
     test
       .nock('https://foo.com', (api) => {
         api.get('/bar').reply(200, '', {
-          'User-Agent': ` ${pkg.name}\/${pkg.version} \(${os.platform()} ${os.arch()}\) dummyCommand keytar`,
+          'User-Agent': ` ${pkg.name}\/${pkg.version} \(${os.platform()} ${os.arch()}\) dummyCommand`,
         });
       })
       .it('correctly sets user-agent with empty plugin value', async () => {
-        const client = new CliRequestClient('dummyCommand', logger, '', true, '');
+        const client = new CliRequestClient('dummyCommand', logger, '', '');
         const response = await client.request({
           method: 'GET',
           uri: 'https://foo.com/bar',
         });
         expect(client.lastRequest.headers['User-Agent']).to.equal(
-          ` ${pkg.name}\/${pkg.version} \(${os.platform()} ${os.arch()}\) dummyCommand keytar`,
+          ` ${pkg.name}\/${pkg.version} \(${os.platform()} ${os.arch()}\) dummyCommand`,
         );
         expect(response.statusCode).to.equal(200);
       });

diff --git a/test/services/secure-storage.test.js b/test/services/secure-storage.test.js