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

Fix error when removing uploaded post breach file on config reset #1054

Merged
merged 3 commits into from
Apr 26, 2021

Conversation

shreyamalviya
Copy link
Contributor

@shreyamalviya shreyamalviya commented Mar 25, 2021

Fixes:
image
os.remove(file_path) in monkey_island/cc/resources/pba_file_upload.py attempts to delete a directory which throws an error when the PBA_linux_filename/PBA_windows_filename field in the database is empty.

To reproduce:

  1. Start Monkey Island.
  2. Go to the configuration page.
  3. Make sure that nothing is uploaded in the custom PBA file field.
  4. Reset the config.

@codecov
Copy link

codecov bot commented Mar 25, 2021

Codecov Report

Merging #1054 (1ad8249) into develop (753c058) will decrease coverage by 0.12%.
The diff coverage is 0.00%.

❗ Current head 1ad8249 differs from pull request most recent head 9b38303. Consider uploading reports for the commit 9b38303 to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1054      +/-   ##
===========================================
- Coverage    29.13%   29.01%   -0.13%     
===========================================
  Files          410      409       -1     
  Lines        12934    12912      -22     
===========================================
- Hits          3768     3746      -22     
  Misses        9166     9166              
Impacted Files Coverage Δ
...nkey/monkey_island/cc/resources/pba_file_upload.py 0.00% <0.00%> (ø)
monkey/tests/conftest.py
monkey/tests/monkey_island/cc/conftest.py
...onkey/tests/infection_monkey/telemetry/conftest.py
monkey/monkey_island/cc/conftest.py 100.00% <0.00%> (ø)
...onkey/infection_monkey/telemetry/tests/conftest.py 100.00% <0.00%> (ø)
monkey/__init__.py 100.00% <0.00%> (+100.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8121f08...9b38303. Read the comment docs.

@shreyamalviya shreyamalviya changed the base branch from release/1.10.0 to develop March 25, 2021 16:52
Copy link
Collaborator

@mssalvatore mssalvatore left a comment

Choose a reason for hiding this comment

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

It would be nice, but not mandatory, to add some tests for this function. I'm not sure if pytest plays nicely with the @jwt_required decorator.

monkey/monkey_island/cc/resources/pba_file_upload.py Outdated Show resolved Hide resolved
monkey/monkey_island/cc/resources/pba_file_upload.py Outdated Show resolved Hide resolved
@shreyamalviya
Copy link
Contributor Author

@mssalvatore I'm not sure how much value we'll get out of adding tests for this.

@mssalvatore
Copy link
Collaborator

@mssalvatore I'm not sure how much value we'll get out of adding tests for this.

Had we had the tests in the first place, this bug might have been prevented. Where possible, we should try to first write a test that reproduces the issue, then fix it and see that the test passes. A set of tests for this function seems feasible. It's also a good opportunity to investigate the best way to test our API endpoints.

image
https://flask.palletsprojects.com/en/1.1.x/testing/

@@ -96,3 +93,11 @@ def upload_pba_file(request_, is_linux=True):
(PBA_LINUX_FILENAME_PATH if is_linux else PBA_WINDOWS_FILENAME_PATH), filename
)
return filename

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would recommend putting this method immediately after the one that calls it so as to follow the Stepdown rule

image

@@ -69,13 +69,10 @@ def delete(self, file_type):
PBA_LINUX_FILENAME_PATH if file_type == "PBAlinux" else PBA_WINDOWS_FILENAME_PATH
Copy link
Collaborator

Choose a reason for hiding this comment

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

Regarding the value of adding tests, does this function behave in a sane way if I pass an invalid value for file_type?

@mssalvatore mssalvatore merged commit c6fd04e into guardicore:develop Apr 26, 2021
mssalvatore added a commit that referenced this pull request Apr 26, 2021
shreyamalviya pushed a commit that referenced this pull request Apr 27, 2021
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.

2 participants