From c2682d63db459ab855ff7c19188bf70b266d47fe Mon Sep 17 00:00:00 2001 From: Aryan Jassal Date: Tue, 27 Aug 2024 17:03:26 +1000 Subject: [PATCH] feat: 'secrets edit' now makes a new secret if it doesnt exist already feat: added tests for secrets edit chore: fixed logic errors and added review changes --- src/secrets/CommandEdit.ts | 110 ++++++++++++++++------- tests/secrets/edit.test.ts | 178 +++++++++++++++++++++++++++++++++++++ tests/secrets/get.test.ts | 4 +- tests/secrets/list.test.ts | 4 +- 4 files changed, 259 insertions(+), 37 deletions(-) create mode 100644 tests/secrets/edit.test.ts diff --git a/src/secrets/CommandEdit.ts b/src/secrets/CommandEdit.ts index aa7abac4..8705d14e 100644 --- a/src/secrets/CommandEdit.ts +++ b/src/secrets/CommandEdit.ts @@ -1,8 +1,8 @@ import type PolykeyClient from 'polykey/dist/PolykeyClient'; import fs from 'fs'; import path from 'path'; -import * as errors from '../errors'; import CommandPolykey from '../CommandPolykey'; +import * as errors from '../errors'; import * as binUtils from '../utils'; import * as binOptions from '../utils/options'; import * as binParsers from '../utils/parsers'; @@ -24,6 +24,7 @@ class CommandEdit extends CommandPolykey { this.action(async (secretPath, options) => { const os = await import('os'); const { execSync } = await import('child_process'); + const vaultsErrors = await import('polykey/dist/vaults/errors'); const { default: PolykeyClient } = await import( 'polykey/dist/PolykeyClient' ); @@ -44,6 +45,10 @@ class CommandEdit extends CommandPolykey { this.exitHandlers.handlers.push(async () => { if (pkClient != null) await pkClient.stop(); }); + + const tmpDir = await fs.promises.mkdtemp( + path.join(os.tmpdir(), 'polykey-'), + ); try { pkClient = await PolykeyClient.createPolykeyClient({ nodeId: clientOptions.nodeId, @@ -54,45 +59,86 @@ class CommandEdit extends CommandPolykey { }, logger: this.logger.getChild(PolykeyClient.name), }); - const response = await binUtils.retryAuthentication( - (auth) => - pkClient.rpcClient.methods.vaultsSecretsGet({ - metadata: auth, - nameOrId: secretPath[0], - secretName: secretPath[1], - }), + const tmpFile = path.join(tmpDir, path.basename(secretPath[1])); + const secretExists = await binUtils.retryAuthentication( + async (auth) => { + let exists: boolean = true; + try { + const response = + await pkClient.rpcClient.methods.vaultsSecretsGet({ + metadata: auth, + nameOrId: secretPath[0], + secretName: secretPath[1], + }); + await this.fs.promises.writeFile(tmpFile, response.secretContent); + } catch (e) { + const [cause, _] = binUtils.remoteErrorCause(e); + if (cause instanceof vaultsErrors.ErrorSecretsSecretUndefined) { + exists = false; + } else { + throw e; + } + } + return exists; + }, meta, ); - const secretContent = response.secretContent; - const tmpDir = await fs.promises.mkdtemp( - path.join(os.tmpdir(), 'polykey-'), - ); - const tmpFile = path.join(tmpDir, 'pksecret'); - await this.fs.promises.writeFile(tmpFile, secretContent); - execSync(`$EDITOR \"${tmpFile}\"`, { stdio: 'inherit' }); - let content: Buffer; + // If the editor exited with a code other than zero, then execSync + // will throw an error. So, in the case of saving the file but the + // editor crashing, the program won't save the updated secret. + execSync(`${process.env.EDITOR} \"${tmpFile}\"`, { stdio: 'inherit' }); + let content: string; try { - content = await this.fs.promises.readFile(tmpFile); + content = (await this.fs.promises.readFile(tmpFile)).toString( + 'binary', + ); } catch (e) { - throw new errors.ErrorPolykeyCLIFileRead(e.message, { - data: { - errno: e.errno, - syscall: e.syscall, - code: e.code, - path: e.path, - }, - cause: e, - }); + if (e.code === 'ENOENT') { + // If the secret exists but the file doesn't, then something went + // wrong, and the file cannot be read anymore. This is bad. + if (secretExists) { + throw new errors.ErrorPolykeyCLIFileRead(e.message, { + data: { + errno: e.errno, + syscall: e.syscall, + code: e.code, + path: e.path, + }, + cause: e, + }); + // If the secret didn't exist before and we can't read the file, + // then the secret was never actually created or saved. The user + // doesn't want to make the secret anymore, so abort mision! + } else { + return; + } + } + throw e; } - await pkClient.rpcClient.methods.vaultsSecretsEdit({ - nameOrId: secretPath[0], - secretName: secretPath[1], - secretContent: content.toString('binary'), - }); - await this.fs.promises.rm(tmpDir, { recursive: true, force: true }); + await binUtils.retryAuthentication(async (auth) => { + // This point will never be reached if the temp file doesn't exist. + // As such, if the secret didn't exist before, then we want to make it. + // Otherwise, if the secret existed before, then we want to edit it. + if (secretExists) { + await pkClient.rpcClient.methods.vaultsSecretsEdit({ + metadata: auth, + nameOrId: secretPath[0], + secretName: secretPath[1], + secretContent: content, + }); + } else { + await pkClient.rpcClient.methods.vaultsSecretsNew({ + metadata: auth, + nameOrId: secretPath[0], + secretName: secretPath[1], + secretContent: content, + }); + } + }, meta); // Windows // TODO: complete windows impl } finally { + await this.fs.promises.rm(tmpDir, { recursive: true, force: true }); if (pkClient! != null) await pkClient.stop(); } }); diff --git a/tests/secrets/edit.test.ts b/tests/secrets/edit.test.ts new file mode 100644 index 00000000..e8f3c983 --- /dev/null +++ b/tests/secrets/edit.test.ts @@ -0,0 +1,178 @@ +import type { VaultName } from 'polykey/dist/vaults/types'; +import path from 'path'; +import fs from 'fs'; +import Logger, { LogLevel, StreamHandler } from '@matrixai/logger'; +import PolykeyAgent from 'polykey/dist/PolykeyAgent'; +import { vaultOps } from 'polykey/dist/vaults'; +import * as keysUtils from 'polykey/dist/keys/utils'; +import * as testUtils from '../utils'; + +describe('commandEditSecret', () => { + const password = 'password'; + const logger = new Logger('CLI Test', LogLevel.WARN, [new StreamHandler()]); + const editedContent = 'edited secret contents'; + let dataDir: string; + let editorEdit: string; + let editorExit: string; + let editorFail: string; + let editorView: string; + let polykeyAgent: PolykeyAgent; + let command: Array; + + beforeEach(async () => { + dataDir = await fs.promises.mkdtemp( + path.join(globalThis.tmpDir, 'polykey-test-'), + ); + editorEdit = path.join(dataDir, 'editorEdit.sh'); + editorExit = path.join(dataDir, 'editorExit.sh'); + editorFail = path.join(dataDir, 'editorFail.sh'); + editorView = path.join(dataDir, 'editorView.sh'); + await fs.promises.writeFile(editorExit, `#!/usr/bin/env bash\nexit`); + await fs.promises.chmod(editorExit, '755'); + await fs.promises.writeFile( + editorView, + `#!/usr/bin/env bash\ncp $1 ${dataDir}/secret; echo "${editedContent}" > $1`, + ); + await fs.promises.chmod(editorView, '755'); + await fs.promises.writeFile( + editorEdit, + `#!/usr/bin/env bash\necho "${editedContent}" > $1`, + ); + await fs.promises.chmod(editorEdit, '755'); + await fs.promises.writeFile( + editorFail, + `#!/usr/bin/env bash\necho "${editedContent}" > $1; exit 1`, + ); + await fs.promises.chmod(editorFail, '755'); + polykeyAgent = await PolykeyAgent.createPolykeyAgent({ + password, + options: { + nodePath: dataDir, + agentServiceHost: '127.0.0.1', + clientServiceHost: '127.0.0.1', + keys: { + passwordOpsLimit: keysUtils.passwordOpsLimits.min, + passwordMemLimit: keysUtils.passwordMemLimits.min, + strictMemoryLock: false, + }, + }, + logger: logger, + }); + }); + afterEach(async () => { + await polykeyAgent.stop(); + await fs.promises.rm(dataDir, { + force: true, + recursive: true, + }); + }); + + test('should edit secret', async () => { + const vaultName = 'Vault10' as VaultName; + const vaultId = await polykeyAgent.vaultManager.createVault(vaultName); + const secretName = 'secret'; + + await polykeyAgent.vaultManager.withVaults([vaultId], async (vault) => { + await vaultOps.addSecret(vault, secretName, 'original secret'); + }); + + command = ['secrets', 'edit', '-np', dataDir, `${vaultName}:${secretName}`]; + + const result = await testUtils.pkStdio([...command], { + env: { PK_PASSWORD: password, EDITOR: editorEdit }, + cwd: dataDir, + }); + expect(result.exitCode).toBe(0); + + await polykeyAgent.vaultManager.withVaults([vaultId], async (vault) => { + const contents = await vaultOps.getSecret(vault, secretName); + expect(contents.toString()).toStrictEqual(`${editedContent}\n`); + }); + }); + + test('should create secret if it does not exist', async () => { + const vaultName = 'Vault10' as VaultName; + const vaultId = await polykeyAgent.vaultManager.createVault(vaultName); + const secretName = 'secret'; + + command = ['secrets', 'edit', '-np', dataDir, `${vaultName}:${secretName}`]; + + const result = await testUtils.pkStdio([...command], { + env: { PK_PASSWORD: password, EDITOR: editorEdit }, + cwd: dataDir, + }); + expect(result.exitCode).toBe(0); + + await polykeyAgent.vaultManager.withVaults([vaultId], async (vault) => { + const contents = await vaultOps.getSecret(vault, secretName); + expect(contents.toString()).toStrictEqual(`${editedContent}\n`); + }); + }); + + test('should not create secret if editor crashes', async () => { + const vaultName = 'Vault10' as VaultName; + const vaultId = await polykeyAgent.vaultManager.createVault(vaultName); + const secretName = 'secret'; + + command = ['secrets', 'edit', '-np', dataDir, `${vaultName}:${secretName}`]; + + const result = await testUtils.pkStdio([...command], { + env: { PK_PASSWORD: password, EDITOR: editorFail }, + cwd: dataDir, + }); + expect(result.exitCode).not.toBe(0); + + await polykeyAgent.vaultManager.withVaults([vaultId], async (vault) => { + const list = await vaultOps.listSecrets(vault); + expect(list.sort()).toStrictEqual([]); + }); + }); + + test('should not create secret if editor does not write to file', async () => { + const vaultName = 'Vault10' as VaultName; + const vaultId = await polykeyAgent.vaultManager.createVault(vaultName); + const secretName = 'secret'; + + command = ['secrets', 'edit', '-np', dataDir, `${vaultName}:${secretName}`]; + + const result = await testUtils.pkStdio([...command], { + env: { PK_PASSWORD: password, EDITOR: editorExit }, + cwd: dataDir, + }); + expect(result.exitCode).toBe(0); + + await polykeyAgent.vaultManager.withVaults([vaultId], async (vault) => { + const list = await vaultOps.listSecrets(vault); + expect(list.sort()).toStrictEqual([]); + }); + }); + + test('file contents should be fetched correctly', async () => { + const vaultName = 'Vault10' as VaultName; + const vaultId = await polykeyAgent.vaultManager.createVault(vaultName); + const secretName = 'secret'; + const secretContent = 'original secret'; + + await polykeyAgent.vaultManager.withVaults([vaultId], async (vault) => { + await vaultOps.addSecret(vault, secretName, secretContent); + }); + + command = ['secrets', 'edit', '-np', dataDir, `${vaultName}:${secretName}`]; + + const result = await testUtils.pkStdio([...command], { + env: { PK_PASSWORD: password, EDITOR: editorView }, + cwd: dataDir, + }); + expect(result.exitCode).toBe(0); + + const fetchedSecret = await fs.promises.readFile( + path.join(dataDir, 'secret'), + ); + expect(fetchedSecret.toString()).toEqual(secretContent); + + await polykeyAgent.vaultManager.withVaults([vaultId], async (vault) => { + const contents = await vaultOps.getSecret(vault, secretName); + expect(contents.toString()).toStrictEqual(`${editedContent}\n`); + }); + }); +}); diff --git a/tests/secrets/get.test.ts b/tests/secrets/get.test.ts index 21cbe3b7..ca2171fa 100644 --- a/tests/secrets/get.test.ts +++ b/tests/secrets/get.test.ts @@ -52,9 +52,7 @@ describe('commandGetSecret', () => { command = ['secrets', 'get', '-np', dataDir, `${vaultName}:MySecret`]; const result = await testUtils.pkStdio([...command], { - env: { - PK_PASSWORD: password, - }, + env: { PK_PASSWORD: password }, cwd: dataDir, }); expect(result.stdout).toBe('this is the secret'); diff --git a/tests/secrets/list.test.ts b/tests/secrets/list.test.ts index 648aed70..3e8513c6 100644 --- a/tests/secrets/list.test.ts +++ b/tests/secrets/list.test.ts @@ -84,7 +84,7 @@ describe('commandListSecrets', () => { test( 'should fail when path is not a directory', async () => { - const vaultName = 'Vault5' as VaultName; + const vaultName = 'Vault4' as VaultName; const vaultId = await polykeyAgent.vaultManager.createVault(vaultName); await polykeyAgent.vaultManager.withVaults([vaultId], async (vault) => { @@ -124,7 +124,7 @@ describe('commandListSecrets', () => { test( 'should list secrets within directories', async () => { - const vaultName = 'Vault6' as VaultName; + const vaultName = 'Vault4' as VaultName; const vaultId = await polykeyAgent.vaultManager.createVault(vaultName); await polykeyAgent.vaultManager.withVaults([vaultId], async (vault) => {