From cadc9628a6fe31a8a76098fe545c5851a1e02489 Mon Sep 17 00:00:00 2001 From: Aryan Jassal Date: Fri, 1 Nov 2024 18:00:30 +1100 Subject: [PATCH] chore: updated secrets remove to properly operate on multiple paths --- src/client/callers/vaultsSecretsRemove.ts | 4 +- src/client/handlers/VaultsSecretsRemove.ts | 80 ++++++++----- src/vaults/VaultOps.ts | 50 ++++----- tests/client/handlers/vaults.test.ts | 124 +++++++++++++++------ tests/vaults/VaultOps/deleteSecret.test.ts | 40 ++----- tests/vaults/VaultOps/mkdir.test.ts | 1 - 6 files changed, 178 insertions(+), 121 deletions(-) diff --git a/src/client/callers/vaultsSecretsRemove.ts b/src/client/callers/vaultsSecretsRemove.ts index e2bda1e28..8362ce46a 100644 --- a/src/client/callers/vaultsSecretsRemove.ts +++ b/src/client/callers/vaultsSecretsRemove.ts @@ -1,10 +1,10 @@ import type { HandlerTypes } from '@matrixai/rpc'; import type VaultsSecretsRemove from '../handlers/VaultsSecretsRemove'; -import { ClientCaller } from '@matrixai/rpc'; +import { DuplexCaller } from '@matrixai/rpc'; type CallerTypes = HandlerTypes; -const vaultsSecretsRemove = new ClientCaller< +const vaultsSecretsRemove = new DuplexCaller< CallerTypes['input'], CallerTypes['output'] >(); diff --git a/src/client/handlers/VaultsSecretsRemove.ts b/src/client/handlers/VaultsSecretsRemove.ts index c06807639..25832ae1f 100644 --- a/src/client/handlers/VaultsSecretsRemove.ts +++ b/src/client/handlers/VaultsSecretsRemove.ts @@ -2,26 +2,25 @@ import type { DB } from '@matrixai/db'; import type { ClientRPCRequestParams, ClientRPCResponseResult, - SuccessMessage, SecretIdentifierMessage, + SuccessOrErrorMessage, } from '../types'; import type VaultManager from '../../vaults/VaultManager'; -import { ClientHandler } from '@matrixai/rpc'; +import { DuplexHandler } from '@matrixai/rpc'; import * as vaultsUtils from '../../vaults/utils'; import * as vaultsErrors from '../../vaults/errors'; -import * as vaultOps from '../../vaults/VaultOps'; -class VaultsSecretsRemove extends ClientHandler< +class VaultsSecretsRemove extends DuplexHandler< { db: DB; vaultManager: VaultManager; }, ClientRPCRequestParams, - ClientRPCResponseResult + ClientRPCResponseResult > { - public handle = async ( + public handle = async function* ( input: AsyncIterable>, - ): Promise> => { + ): AsyncGenerator> { const { db, vaultManager }: { db: DB; vaultManager: VaultManager } = this.container; // Create a record of secrets to be removed, grouped by vault names @@ -41,25 +40,54 @@ class VaultsSecretsRemove extends ClientHandler< } vaultGroups[vaultName].push(secretName); }); - - await db.withTransactionF(async (tran) => { - for (const [vaultName, secretNames] of Object.entries(vaultGroups)) { - const vaultIdFromName = await vaultManager.getVaultId(vaultName, tran); - const vaultId = vaultIdFromName ?? vaultsUtils.decodeVaultId(vaultName); - if (vaultId == null) throw new vaultsErrors.ErrorVaultsVaultUndefined(); - await vaultManager.withVaults( - [vaultId], - async (vault) => { - await vaultOps.deleteSecret(vault, secretNames, { - recursive: metadata?.options?.recursive, - }); - }, - tran, - ); - } - }); - - return { type: 'success', success: true }; + // Now, all the paths will be removed for a vault within a single commit + yield* db.withTransactionG( + async function* (tran): AsyncGenerator { + for (const [vaultName, secretNames] of Object.entries(vaultGroups)) { + const vaultIdFromName = await vaultManager.getVaultId( + vaultName, + tran, + ); + const vaultId = + vaultIdFromName ?? vaultsUtils.decodeVaultId(vaultName); + if (vaultId == null) { + throw new vaultsErrors.ErrorVaultsVaultUndefined(); + } + yield* vaultManager.withVaultsG( + [vaultId], + async function* (vault): AsyncGenerator { + yield* vault.writeG( + async function* (efs): AsyncGenerator { + for (const secretName of secretNames) { + try { + const stat = await efs.stat(secretName); + if (stat.isDirectory()) { + await efs.rmdir(secretName, { + recursive: metadata?.options?.recursive, + }); + } else { + await efs.unlink(secretName); + } + yield { + type: 'success', + success: true, + }; + } catch (e) { + yield { + type: 'error', + code: e.code, + reason: secretName, + }; + } + } + }, + ); + }, + tran, + ); + } + }, + ); }; } diff --git a/src/vaults/VaultOps.ts b/src/vaults/VaultOps.ts index 120d9663c..6d2ea6079 100644 --- a/src/vaults/VaultOps.ts +++ b/src/vaults/VaultOps.ts @@ -129,37 +129,35 @@ async function statSecret(vault: Vault, secretName: string): Promise { */ async function deleteSecret( vault: Vault, - secretNames: Array, + secretName: string, fileOptions?: FileOptions, logger?: Logger, ): Promise { await vault.writeF(async (efs) => { - for (const secretName of secretNames) { - try { - const stat = await efs.stat(secretName); - if (stat.isDirectory()) { - await efs.rmdir(secretName, fileOptions); - logger?.info(`Deleted directory at '${secretName}'`); - } else { - // Remove the specified file - await efs.unlink(secretName); - logger?.info(`Deleted secret at '${secretName}'`); - } - } catch (e) { - if (e.code === 'ENOENT') { - throw new vaultsErrors.ErrorSecretsSecretUndefined( - `Secret with name: ${secretName} does not exist`, - { cause: e }, - ); - } - if (e.code === 'ENOTEMPTY') { - throw new vaultsErrors.ErrorVaultsRecursive( - `Could not delete directory '${secretName}' without recursive option`, - { cause: e }, - ); - } - throw e; + try { + const stat = await efs.stat(secretName); + if (stat.isDirectory()) { + await efs.rmdir(secretName, fileOptions); + logger?.info(`Deleted directory at '${secretName}'`); + } else { + // Remove the specified file + await efs.unlink(secretName); + logger?.info(`Deleted secret at '${secretName}'`); + } + } catch (e) { + if (e.code === 'ENOENT') { + throw new vaultsErrors.ErrorSecretsSecretUndefined( + `Secret with name: ${secretName} does not exist`, + { cause: e }, + ); } + if (e.code === 'ENOTEMPTY') { + throw new vaultsErrors.ErrorVaultsRecursive( + `Could not delete directory '${secretName}' without recursive option`, + { cause: e }, + ); + } + throw e; } }); } diff --git a/tests/client/handlers/vaults.test.ts b/tests/client/handlers/vaults.test.ts index 377f1c3a2..8eb154662 100644 --- a/tests/client/handlers/vaults.test.ts +++ b/tests/client/handlers/vaults.test.ts @@ -1008,7 +1008,7 @@ describe('vaultsSecretsWriteFile', () => { // Make the directory await vaultManager.withVaults([vaultId], async (vault) => { await vault.writeF(async (efs) => { - efs.mkdir(dirName); + await efs.mkdir(dirName); }); }); // Try writing to directory @@ -1056,7 +1056,7 @@ describe('vaultsSecretsWriteFile', () => { // Make the directory await vaultManager.withVaults([vaultId], async (vault) => { await vault.writeF(async (efs) => { - efs.mkdir(dirName); + await efs.mkdir(dirName); }); }); // Try writing @@ -1789,7 +1789,9 @@ describe('vaultsSecretsNew and vaultsSecretsDelete, vaultsSecretsGet', () => { secretName: secretName, }); await deleteWriter.close(); - expect((await deleteStream.output).success).toBeTruthy(); + for await (const data of deleteStream.readable) { + expect(data.type).toStrictEqual('success'); + } // Check secret was deleted const deleteGetStream = await rpcClient.methods.vaultsSecretsGet(); const deleteGetWriter = deleteGetStream.writable.getWriter(); @@ -1839,7 +1841,7 @@ describe('vaultsSecretsNew and vaultsSecretsDelete, vaultsSecretsGet', () => { `${secretName1}${secretName2}${secretName3}`, ); }); - test('should not fail to get secrets on error when continueOnError is set', async () => { + test('should continue on error if option is set', async () => { // Create secrets const secretName1 = 'test-secret1'; const secretName2 = 'test-secret2'; @@ -1894,7 +1896,52 @@ describe('vaultsSecretsNew and vaultsSecretsDelete, vaultsSecretsGet', () => { secretName: secretName2, }); await deleteWriter.close(); - expect((await deleteStream.output).success).toBeTruthy(); + for await (const data of deleteStream.readable) { + expect(data.type).toStrictEqual('success'); + } + // Check each secret was deleted + await checkSecretIsDeleted(vaultId, secretName1); + await checkSecretIsDeleted(vaultId, secretName2); + }); + test('continues deleting secrets if invalid secret is present', async () => { + // Create secrets + const secretName1 = 'test-secret1'; + const secretName2 = 'test-secret2'; + const invalidName = 'invalid-name'; + const vaultId = await vaultManager.createVault('test-vault'); + const vaultIdEncoded = vaultsUtils.encodeVaultId(vaultId); + await createVaultSecret(vaultId, secretName1, secretName1); + await createVaultSecret(vaultId, secretName2, secretName2); + // Delete secrets + const deleteStream = await rpcClient.methods.vaultsSecretsRemove(); + const deleteWriter = deleteStream.writable.getWriter(); + await deleteWriter.write({ + nameOrId: vaultIdEncoded, + secretName: secretName1, + }); + await deleteWriter.write({ + nameOrId: vaultIdEncoded, + secretName: invalidName, + }); + await deleteWriter.write({ + nameOrId: vaultIdEncoded, + secretName: secretName2, + }); + await deleteWriter.close(); + let errorCount = 0; + for await (const data of deleteStream.readable) { + if (data.type === 'error') { + // TS cannot properly evaluate a type as nested as this, so we use the + // as keyword to help it. Inside this block, the type of data is 'error'. + const error = data as ErrorMessage; + // No other file name should raise this error + expect(error.reason).toStrictEqual(invalidName); + errorCount++; + continue; + } + expect(data.type).toStrictEqual('success'); + } + expect(errorCount).toEqual(1); // Check each secret was deleted await checkSecretIsDeleted(vaultId, secretName1); await checkSecretIsDeleted(vaultId, secretName2); @@ -1973,7 +2020,9 @@ describe('vaultsSecretsNew and vaultsSecretsDelete, vaultsSecretsGet', () => { secretName: secretName3, }); await deleteWriter.close(); - expect((await deleteStream.output).success).toBeTruthy(); + for await (const data of deleteStream.readable) { + expect(data.type).toStrictEqual('success'); + } // Ensure single log message for deleting the secrets await vaultManager.withVaults( [vaultId1, vaultId2], @@ -1988,13 +2037,16 @@ describe('vaultsSecretsNew and vaultsSecretsDelete, vaultsSecretsGet', () => { const vaultId = await vaultManager.createVault('test-vault'); const vaultIdEncoded = vaultsUtils.encodeVaultId(vaultId); const secretDir = 'secret-dir'; - const secretName1 = `${secretDir}/test-secret1`; - const secretName2 = `${secretDir}/test-secret2`; - const secretName3 = `${secretDir}/test-secret3`; + const secretName1 = 'test-secret1'; + const secretName2 = 'test-secret2'; + const secretName3 = 'test-secret3'; + const secretPath1 = path.join(secretDir, secretName1); + const secretPath2 = path.join(secretDir, secretName2); + const secretPath3 = path.join(secretDir, secretName3); await createVaultDir(vaultId, secretDir); - await createVaultSecret(vaultId, secretName1, secretName1); - await createVaultSecret(vaultId, secretName2, secretName2); - await createVaultSecret(vaultId, secretName3, secretName3); + await createVaultSecret(vaultId, secretPath1, secretPath1); + await createVaultSecret(vaultId, secretPath2, secretPath2); + await createVaultSecret(vaultId, secretPath3, secretPath3); // Deleting directory with recursive set should not fail const deleteStream = await rpcClient.methods.vaultsSecretsRemove(); await (async () => { @@ -2006,11 +2058,13 @@ describe('vaultsSecretsNew and vaultsSecretsDelete, vaultsSecretsGet', () => { }); await writer.close(); })(); - expect((await deleteStream.output).success).toBeTruthy(); + for await (const data of deleteStream.readable) { + expect(data.type).toStrictEqual('success'); + } // Check each secret and the secret directory were deleted - await checkSecretIsDeleted(vaultId, secretName1); - await checkSecretIsDeleted(vaultId, secretName2); - await checkSecretIsDeleted(vaultId, secretName3); + await checkSecretIsDeleted(vaultId, secretPath1); + await checkSecretIsDeleted(vaultId, secretPath2); + await checkSecretIsDeleted(vaultId, secretPath3); await testsUtils.expectRemoteError( rpcClient.methods.vaultsSecretsStat({ nameOrId: vaultIdEncoded, @@ -2024,28 +2078,28 @@ describe('vaultsSecretsNew and vaultsSecretsDelete, vaultsSecretsGet', () => { const vaultId = await vaultManager.createVault('test-vault'); const vaultIdEncoded = vaultsUtils.encodeVaultId(vaultId); const secretDir = 'secret-dir'; - const secretName1 = `${secretDir}/test-secret1`; - const secretName2 = `${secretDir}/test-secret2`; - const secretName3 = `${secretDir}/test-secret3`; + const secretName1 = 'test-secret1'; + const secretName2 = 'test-secret2'; + const secretName3 = 'test-secret3'; + const secretPath1 = path.join(secretDir, secretName1); + const secretPath2 = path.join(secretDir, secretName2); + const secretPath3 = path.join(secretDir, secretName3); await createVaultDir(vaultId, secretDir); - await createVaultSecret(vaultId, secretName1, secretName1); - await createVaultSecret(vaultId, secretName2, secretName2); - await createVaultSecret(vaultId, secretName3, secretName3); + await createVaultSecret(vaultId, secretPath1, secretPath1); + await createVaultSecret(vaultId, secretPath2, secretPath2); + await createVaultSecret(vaultId, secretPath3, secretPath3); // Deleting directory with recursive unset should fail - const failDeleteStream = await rpcClient.methods.vaultsSecretsRemove(); - await (async () => { - const writer = failDeleteStream.writable.getWriter(); - await writer.write({ nameOrId: vaultIdEncoded, secretName: secretDir }); - await writer.close(); - })(); - await testsUtils.expectRemoteError( - failDeleteStream.output, - vaultsErrors.ErrorVaultsRecursive, - ); + const response = await rpcClient.methods.vaultsSecretsRemove(); + const writer = response.writable.getWriter(); + await writer.write({ nameOrId: vaultIdEncoded, secretName: secretDir }); + await writer.close(); + for await (const data of response.readable) { + expect(data.type).toStrictEqual('error'); + } // Check each secret and the secret directory exist - await checkSecretExists(vaultId, secretName1); - await checkSecretExists(vaultId, secretName2); - await checkSecretExists(vaultId, secretName3); + await checkSecretExists(vaultId, secretPath1); + await checkSecretExists(vaultId, secretPath2); + await checkSecretExists(vaultId, secretPath3); await checkSecretExists(vaultId, secretDir); }); }); diff --git a/tests/vaults/VaultOps/deleteSecret.test.ts b/tests/vaults/VaultOps/deleteSecret.test.ts index 4ffbe1cdd..f2061ca72 100644 --- a/tests/vaults/VaultOps/deleteSecret.test.ts +++ b/tests/vaults/VaultOps/deleteSecret.test.ts @@ -91,76 +91,54 @@ describe('deleteSecret', () => { test('deleting a secret', async () => { await testVaultsUtils.writeSecret(vault, secretName, secretContent); - await vaultOps.deleteSecret(vault, [secretName]); + await vaultOps.deleteSecret(vault, secretName); await testVaultsUtils.expectSecretNot(vault, secretName); }); test('deleting a secret in a directory', async () => { const secretPath = path.join(dirName, secretName); await testVaultsUtils.writeSecret(vault, secretPath, secretContent); - await vaultOps.deleteSecret(vault, [secretPath]); + await vaultOps.deleteSecret(vault, secretPath); await testVaultsUtils.expectSecretNot(vault, secretPath); await testVaultsUtils.expectDirExists(vault, dirName); }); test('deleting a directory', async () => { await testVaultsUtils.mkdir(vault, dirName); - await vaultOps.deleteSecret(vault, [dirName]); + await vaultOps.deleteSecret(vault, dirName); await testVaultsUtils.expectDirExistsNot(vault, dirName); }); test('deleting a directory with a file should fail', async () => { const secretPath = path.join(dirName, secretName); await testVaultsUtils.writeSecret(vault, secretPath, secretContent); - await expect(vaultOps.deleteSecret(vault, [dirName])).rejects.toThrow( + await expect(vaultOps.deleteSecret(vault, dirName)).rejects.toThrow( vaultsErrors.ErrorVaultsRecursive, ); }); test('deleting a directory with force', async () => { const secretPath = path.join(dirName, secretName); await testVaultsUtils.writeSecret(vault, secretPath, secretContent); - await vaultOps.deleteSecret(vault, [dirName], { recursive: true }); + await vaultOps.deleteSecret(vault, dirName, { recursive: true }); await testVaultsUtils.expectDirExistsNot(vault, dirName); }); test('deleting a secret that does not exist should fail', async () => { - await expect(vaultOps.deleteSecret(vault, [secretName])).rejects.toThrow( + await expect(vaultOps.deleteSecret(vault, secretName)).rejects.toThrow( vaultsErrors.ErrorSecretsSecretUndefined, ); }); - test('deleting multiple secrets', async () => { - const secretNames = ['secret1', 'secret2', 'secret3']; - for (const secretName of secretNames) { - await testVaultsUtils.writeSecret(vault, secretName, secretName); - } - await vaultOps.deleteSecret(vault, secretNames); - for (const secretName of secretNames) { - await testVaultsUtils.expectSecretNot(vault, secretName); - } - }); - test('deleting multiple secrets should add only one new log message', async () => { - const secretNames = ['secret1', 'secret2', 'secret3']; - for (const secretName of secretNames) { - await testVaultsUtils.writeSecret(vault, secretName, secretName); - } - const logLength = (await vault.log()).length; - await vaultOps.deleteSecret(vault, secretNames); - for (const secretName of secretNames) { - await testVaultsUtils.expectSecretNot(vault, secretName); - } - expect((await vault.log()).length).toBe(logLength + 1); - }); test('deleting a hidden secret', async () => { await testVaultsUtils.writeSecret(vault, secretNameHidden, secretContent); - await vaultOps.deleteSecret(vault, [secretNameHidden]); + await vaultOps.deleteSecret(vault, secretNameHidden); await testVaultsUtils.expectSecretNot(vault, secretNameHidden); }); test('deleting a hidden secret in a hidden directory', async () => { const secretPathHidden = path.join(dirNameHidden, secretNameHidden); await testVaultsUtils.writeSecret(vault, secretPathHidden, secretContent); - await vaultOps.deleteSecret(vault, [secretPathHidden]); + await vaultOps.deleteSecret(vault, secretPathHidden); await testVaultsUtils.expectSecretNot(vault, secretPathHidden); await testVaultsUtils.expectDirExists(vault, dirNameHidden); }); test('deleting a hidden directory', async () => { await testVaultsUtils.mkdir(vault, dirNameHidden); - await vaultOps.deleteSecret(vault, [dirNameHidden]); + await vaultOps.deleteSecret(vault, dirNameHidden); await testVaultsUtils.expectDirExistsNot(vault, dirNameHidden); }); }); diff --git a/tests/vaults/VaultOps/mkdir.test.ts b/tests/vaults/VaultOps/mkdir.test.ts index b8869da44..ea97be492 100644 --- a/tests/vaults/VaultOps/mkdir.test.ts +++ b/tests/vaults/VaultOps/mkdir.test.ts @@ -2,7 +2,6 @@ import type { VaultId } from '@/vaults/types'; import type { Vault } from '@/vaults/Vault'; import type KeyRing from '@/keys/KeyRing'; import type { LevelPath } from '@matrixai/db'; -import type { ErrorMessage } from '@/client/types'; import fs from 'fs'; import path from 'path'; import os from 'os';