From 9f1c2d9b4607c6bc83b80bfde5cf3eb1ce558fe7 Mon Sep 17 00:00:00 2001 From: kridai Date: Fri, 5 Aug 2022 20:58:05 +0530 Subject: [PATCH] fix: cleanup keytar (#209) --- README.md | 4 - package.json | 3 - src/base-commands/base-command.js | 2 - src/base-commands/twilio-client-command.js | 13 +-- src/index.js | 1 - src/services/cli-http-client.js | 4 +- src/services/secure-storage.js | 81 ------------------- .../twilio-client-command.test.js | 35 ++++---- test/services/cli-http-client.test.js | 20 ++--- test/services/secure-storage.test.js | 62 -------------- 10 files changed, 30 insertions(+), 195 deletions(-) delete mode 100644 src/services/secure-storage.js delete mode 100644 test/services/secure-storage.test.js diff --git a/README.md b/README.md index e29e6446..fdb5ad3a 100644 --- 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 index f9768307..ef00752e 100644 --- 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 index 750ff9b4..543f7681 100644 --- 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 index fca3da45..24d7f1c2 100644 --- 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 index 21804397..76b23bca 100644 --- 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 index b02c7728..978e5ccd 100644 --- 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 deleted file mode 100644 index 38bd7146..00000000 --- a/src/services/secure-storage.js +++ /dev/null @@ -1,81 +0,0 @@ -const { CLI_NAME } = require('./config'); -const { TwilioCliError } = require('./error'); -const { HELP_ENVIRONMENT_VARIABLES } = require('./messaging/help-messages'); -const { logger } = require('./messaging/logging'); - -const STORAGE_LOCATIONS = { - KEYCHAIN: 'keychain', - WIN_CRED_VAULT: 'win_cred_vault', - LIBSECRET: 'libsecret', -}; - -const PLATFORM_TO_LOCATION = { - darwin: STORAGE_LOCATIONS.KEYCHAIN, - win32: STORAGE_LOCATIONS.WIN_CRED_VAULT, - linux: STORAGE_LOCATIONS.LIBSECRET, -}; - -class SecureStorage { - constructor({ command, platform } = {}) { - this.command = command; - this.platform = platform || process.platform; - this.keytar = null; - } - - async loadKeytar() { - if (!this.keytar) { - try { - this.keytar = await this.command.install('keytar'); - - // Also sanity-check that we can retrieve passwords (just use a dummy profile ID). - await this.keytar.getPassword(CLI_NAME, CLI_NAME); - } catch (error) { - logger.debug(`Error loading keytar: ${error}`); - // If we can't load up keytar, tell the user that maybe they should just stick to env vars. - throw new TwilioCliError(`Secure credential storage failed to load.\n\n${HELP_ENVIRONMENT_VARIABLES}`); - } - } - - return this.keytar; - } - - async saveCredentials(profileId, username, password) { - await this.loadKeytar(); - await this.keytar.setPassword(CLI_NAME, profileId, `${username}|${password}`); - } - - async removeCredentials(profileId) { - await this.loadKeytar(); - return this.keytar.deletePassword(CLI_NAME, profileId); - } - - async getCredentials(profileId) { - let credentials = null; - try { - await this.loadKeytar(); - credentials = await this.keytar.getPassword(CLI_NAME, profileId); - } catch (error) { - if (error instanceof TwilioCliError) { - throw error; - } - - return { apiKey: 'error', apiSecret: error.message }; - } - - const [apiKey, apiSecret] = credentials ? credentials.split('|') : ['error', 'error']; - - return { - apiKey, - apiSecret, - }; - } - - get storageLocation() { - return PLATFORM_TO_LOCATION[this.platform]; - } -} - -module.exports = { - SecureStorage, - STORAGE_LOCATIONS, -}; diff --git a/test/base-commands/twilio-client-command.test.js b/test/base-commands/twilio-client-command.test.js index 0a212663..ce003015 100644 --- 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,8 +63,21 @@ 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'); } }) @@ -73,15 +85,6 @@ describe('base-commands', () => { .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 index 324c6846..31e34bab 100644 --- 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,20 +71,17 @@ 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); }); @@ -95,17 +89,17 @@ describe('services', () => { 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 deleted file mode 100644 index 121f2ef7..00000000 --- a/test/services/secure-storage.test.js +++ /dev/null @@ -1,62 +0,0 @@ -const { expect, test } = require('@twilio/cli-test'); - -const { SecureStorage, STORAGE_LOCATIONS } = require('../../src/services/secure-storage'); -const { TwilioCliError } = require('../../src/services/error'); - -describe('services', () => { - describe('secure-storage', () => { - test.it('storageLocation (Native)', () => { - const secureStorage = new SecureStorage(); - expect(secureStorage.storageLocation).to.not.eq(undefined); - }); - - test.it('storageLocation (Mac)', () => { - const secureStorage = new SecureStorage({ platform: 'darwin' }); - expect(secureStorage.storageLocation).to.eq(STORAGE_LOCATIONS.KEYCHAIN); - }); - - test.it('storageLocation (Windows)', () => { - const secureStorage = new SecureStorage({ platform: 'win32' }); - expect(secureStorage.storageLocation).to.eq(STORAGE_LOCATIONS.WIN_CRED_VAULT); - }); - - test.it('storageLocation (Linux)', () => { - const secureStorage = new SecureStorage({ platform: 'linux' }); - expect(secureStorage.storageLocation).to.eq(STORAGE_LOCATIONS.LIBSECRET); - }); - - test.it('storageLocation (OpenBSD)', () => { - const secureStorage = new SecureStorage({ platform: 'openbsd' }); - expect(secureStorage.storageLocation).to.eq(undefined); - }); - - describe('getCredentials', () => { - const setup = (getPassword) => - test.do((ctx) => { - ctx.secureStorage = new SecureStorage(); - Object.defineProperty(ctx.secureStorage, 'keytar', { - get: () => ({ getPassword }), - }); - }); - - setup(async () => 'key|password').it('handles a key-password pair', async (ctx) => { - const expected = { apiKey: 'key', apiSecret: 'password' }; - expect(await ctx.secureStorage.getCredentials('Pro File')).to.eql(expected); - }); - - setup(async () => { - throw new Error('WHOA!'); - }).it('handles a keytar error', async (ctx) => { - const expected = { apiKey: 'error', apiSecret: 'WHOA!' }; - expect(await ctx.secureStorage.getCredentials('No File')).to.eql(expected); - }); - - setup(async () => { - throw new TwilioCliError('WOE!'); - }) - .do((ctx) => ctx.secureStorage.getCredentials('Woe File')) - .catch(/WOE!/) - .it('passes a TwilioCliError error through'); - }); - }); -});