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

Clean up Desktop and Temporary Files #747

Merged
merged 1 commit into from
Nov 21, 2023
Merged

Conversation

emtuls
Copy link
Member

@emtuls emtuls commented Nov 16, 2023

This resolves mandiant/flare-vm#517 by adding each command to a separate function. These functions will need to be called from somewhere in order for them to work.

The desktop function is run against both the Current User and 'Public' desktops due to some cases where desktop icons showing on Current User desktop that are only located in Public/Desktop.

@emtuls emtuls requested a review from Ana06 November 16, 2023 04:29
@emtuls emtuls self-assigned this Nov 16, 2023
@emtuls emtuls force-pushed the Cleanup-Desktop-and-Files branch 2 times, most recently from 4fa0631 to 81ee509 Compare November 16, 2023 05:00
Copy link
Member

@Ana06 Ana06 left a comment

Choose a reason for hiding this comment

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

This is great work @emtuls!

We need a single wrapper function (called something like VM-Clean-Up) that executes the 4 steps (the 4 functions you have created) as we may need to call this function manually often. I would keep the functions that have more logic, but I would get rid of the ones that just execute just one command and just call the command directly.

I like to be able to provide items to ignore for deletion (as you are doing) and I think we should have the ignore arguments in the wrapper function as well.

[nitpick] Do not include the issue number in the commit message header and limit the header to 50 (commit message good practice). In this case the commit number is referring the wrong issue (because it is from another repo). In general I like to add the whole GitHub url as you can also use from outside GitHub. As an example, I would write:

common.vm: Add clean up helper function

Add helper function than cleans up space including deleting desktop files and
temporary files.

Resolves https://github.com/mandiant/flare-vm/issues/517

packages/common.vm/tools/vm.common/vm.common.psm1 Outdated Show resolved Hide resolved
packages/common.vm/tools/vm.common/vm.common.psm1 Outdated Show resolved Hide resolved
packages/common.vm/tools/vm.common/vm.common.psm1 Outdated Show resolved Hide resolved
packages/common.vm/tools/vm.common/vm.common.psm1 Outdated Show resolved Hide resolved
packages/common.vm/tools/vm.common/vm.common.psm1 Outdated Show resolved Hide resolved
packages/common.vm/tools/vm.common/vm.common.psm1 Outdated Show resolved Hide resolved
packages/common.vm/tools/vm.common/vm.common.psm1 Outdated Show resolved Hide resolved
@emtuls emtuls force-pushed the Cleanup-Desktop-and-Files branch 3 times, most recently from 31df720 to c6e9494 Compare November 16, 2023 20:34
Copy link
Member

@Ana06 Ana06 left a comment

Choose a reason for hiding this comment

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

This looks great! We need to discuss what to do with the linter complain that seems to not improve the code and then we are ready to go 😃

# Ensure that "PS_Transcripts", "Desktop\Tools", and "fakenet_logs" folders are not to be deleted.
$defaultExcludedFolders = @("PS_Transcripts", ${Env:TOOL_LIST_DIR}, "fakenet_logs")
$excludeFolders = $excludeFolders + $defaultExcludedFolders
$excludeFiles = $excludeFiles # required for lint.ps1 to pass
Copy link
Member

Choose a reason for hiding this comment

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

why is this exactly needed? What does the linter complain about?

Copy link
Member Author

Choose a reason for hiding this comment

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

The linter says that the $excludeFiles variable isn't used, even though it is being used in the if statement. I think this might be a bug in how they are parsing things.

It said the same for $excludeFolders as well, but now that we add + $defaultExcludedFolders, this is a little less obvious that I was doing the same thing for that variable as well.

packages/common.vm/tools/vm.common/vm.common.psm1 Outdated Show resolved Hide resolved
packages/common.vm/tools/vm.common/vm.common.psm1 Outdated Show resolved Hide resolved
Add helper function than cleans up space including deleting desktop files and
temporary files.

Resolves mandiant/flare-vm#517
Copy link
Member

@Ana06 Ana06 left a comment

Choose a reason for hiding this comment

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

Thanks for all the work @emtuls!!!

@Ana06 Ana06 merged commit bd86739 into main Nov 21, 2023
6 checks passed
Ana06 added a commit to Ana06/VM-Packages that referenced this pull request Nov 23, 2023
We originally decided to prevent that `PS_Transcripts` is deleted in:
mandiant#747
But after installing FLARE-VM I would like to delete the files generated
by the installation in the `PS_Transcripts` directory. Remove the
protection, allowing to delete this folder. It is still possible to keep
it using the `excludedFolders` function argument.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configuration: Free up space
2 participants