Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementing secrets rm RPC handler #803

Merged
merged 1 commit into from
Sep 16, 2024
Merged

Implementing secrets rm RPC handler #803

merged 1 commit into from
Sep 16, 2024

Conversation

aryanjassal
Copy link
Contributor

@aryanjassal aryanjassal commented Sep 10, 2024

Description

Implement a RPC handler to delete multiple files. If an option is set, then also delete the specified directory and the contents of that directory from the vaults.

Issues Fixed

Tasks

  • 1. Add new VaultsSecretsRemove handler (functionally similar to VaultsSecretsDelete)
  • 2. Add support for specifying and deleting multiple files
  • 3. Add recursive flag to delete directories and their contents
  • 4. Add tests

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@aryanjassal aryanjassal marked this pull request as draft September 10, 2024 03:20
@aryanjassal aryanjassal self-assigned this Sep 10, 2024
Copy link

linear bot commented Sep 10, 2024

Copy link
Contributor

@tegefaulkes tegefaulkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small changes.

src/client/handlers/VaultsSecretsRemove.ts Outdated Show resolved Hide resolved
src/client/handlers/VaultsSecretsRemove.ts Outdated Show resolved Hide resolved
src/client/types.ts Outdated Show resolved Hide resolved
tests/client/handlers/vaults.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@tegefaulkes tegefaulkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, one last thing to add.

tests/vaults/VaultOps.test.ts Outdated Show resolved Hide resolved
@tegefaulkes
Copy link
Contributor

Looking good now. Prep it for merge but I wouldn't merge it until the CLI side is done and ready to merge as well.

@aryanjassal aryanjassal force-pushed the feature-unix-rm branch 2 times, most recently from ae52289 to 04f0df0 Compare September 10, 2024 08:09
@CMCDragonkai
Copy link
Member

Even when removing a secret, it will exist in the vault history which can be checked out.

@aryanjassal
Copy link
Contributor Author

Even when removing a secret, it will exist in the vault history which can be checked out.

Yes, I believe this is correct. Do I need to remove the file from vault history too?

If the user has access to a secret, they can always just copy the secret and back it up somewhere else, so removing the file from vault history doesn't make much sense to me.

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Sep 13, 2024 via email

@aryanjassal
Copy link
Contributor Author

feat: deleted old secrets delete handlers

feat: added tests

feat: performed requested changes

chore: added more tests for vaultOps.deleteSecret

chore: handles removing secrets of multiple vaults

chore: added tests for deleting multiple secrets
@aryanjassal
Copy link
Contributor Author

Everything for this PR has been done and approved. Merging.

@aryanjassal aryanjassal merged commit 3dd7f22 into staging Sep 16, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants