Skip to content

Conversation

ilija-lazoroski
Copy link
Contributor

@ilija-lazoroski ilija-lazoroski commented Jul 13, 2021

What does this PR do?

Fixes #1241 Add API endpoint for setting the island mode.

Add any further explanations here.

PR Checklist

  • Have you added an explanation of what your changes do and why you'd like to include them?
  • Is the TravisCI build passing?
  • Was the CHANGELOG.md updated to reflect the changes?
  • Was the documentation framework updated to reflect the changes?

Testing Checklist

  • Added relevant unit tests?
  • Have you successfully tested your changes locally? Elaborate:

    Tested by {Running the Monkey locally with relevant config/running Island/...}

  • If applicable, add screenshots or log transcripts of the feature working

Explain Changes

Are the commit messages enough? If not, elaborate.

@ghost
Copy link

ghost commented Jul 13, 2021

Congratulations 🎉. DeepCode analyzed your code in 3.775 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

If you are using our plugins, you might be interested in their successors: Snyk's JetBrains plugin and Snyk's VS Code plugin.

@codecov
Copy link

codecov bot commented Jul 13, 2021

Codecov Report

Merging #1318 (c56ca37) into develop (af739b6) will increase coverage by 9.37%.
The diff coverage is 100.00%.

❗ Current head c56ca37 differs from pull request most recent head 84a78a5. Consider uploading reports for the commit 84a78a5 to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1318      +/-   ##
===========================================
+ Coverage    30.87%   40.24%   +9.37%     
===========================================
  Files          454      471      +17     
  Lines        13545    13820     +275     
===========================================
+ Hits          4182     5562    +1380     
+ Misses        9363     8258    -1105     
Impacted Files Coverage Δ
monkey/monkey_island/cc/app.py 79.10% <100.00%> (+79.10%) ⬆️
...onkey/monkey_island/cc/models/island_mode_model.py 100.00% <100.00%> (ø)
monkey/monkey_island/cc/resources/island_mode.py 100.00% <100.00%> (ø)
...key_island/cc/services/mode/island_mode_service.py 100.00% <100.00%> (ø)
monkey/monkey_island/cc/services/mode/mode_enum.py 100.00% <100.00%> (ø)
.../unit_tests/monkey_island/cc/resources/conftest.py 100.00% <100.00%> (ø)
...rust/scoutsuite/consts/scoutsuite_findings_list.py 100.00% <0.00%> (ø)
...ey_island/cc/resources/zero_trust/finding_event.py 87.50% <0.00%> (ø)
... and 127 more

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 af739b6...84a78a5. Read the comment docs.

Comment on lines 27 to 38
@pytest.mark.usefixtures("uses_database")
def test_island_mode_post__set_model(flask_client):
flask_client.post(
"/api/island-mode", data=json.dumps({"mode": "ransomware"}), follow_redirects=True
)
assert IslandMode.objects[0].mode == "ransomware"


@pytest.mark.usefixtures("uses_database")
def test_island_mode_post__set_invalid_model(flask_client):
flask_client.post(
"/api/island-mode", data=json.dumps({"mode": "bogus mode"}), follow_redirects=True
)
assert len(IslandMode.objects) == 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Theses tests unnecessarily couple the API to the storage mechanism. What's needed is a test that calls the API to set the mode, and then calls the API again to get the mode.

We can commit these for now, but when we add the GET endpoint to the API, these tests should be replaced.

@ilija-lazoroski ilija-lazoroski force-pushed the ransomware_quickstart_endpoint branch from 63101b1 to d135b4b Compare July 13, 2021 10:56
set_mode(mode)
return make_response({}, 200)
except ValueError:
return make_response({}, 404)
Copy link
Collaborator

@mssalvatore mssalvatore Jul 13, 2021

Choose a reason for hiding this comment

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

404 means that the URL was not found. 400 would be a better return code. Better yet, it should return 200 with an error in the response body. See configuation_import.py for an example.

Copy link
Contributor

@VakarisZ VakarisZ Jul 13, 2021

Choose a reason for hiding this comment

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

I'd say changing this to 400 is more than enough for now (or maybe to 422) There's no point in the error message, since this is not gonna be displayed anywhere and 200 will not throw a console error, thus once we add more modes, if the mode is miss-spelled on the UI it will be harder to notice

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like 422

@mssalvatore mssalvatore force-pushed the ransomware_quickstart_endpoint branch from 8471be4 to c56ca37 Compare July 13, 2021 14:26
Flask automatically traps exceptions, returns a 500, and logs a stack
trace. Since Flask will automatically return a 500, we don't need to
duplicate the functionality. Since it prints a stack trace, it provides
more useful information than catching it did.
@mssalvatore mssalvatore merged commit c89416f into develop Jul 13, 2021
@mssalvatore mssalvatore deleted the ransomware_quickstart_endpoint branch July 13, 2021 15:05
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.

Ransomware quickstart
3 participants