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

Improve advanced multiselect #920

Merged

Conversation

mssalvatore
Copy link
Collaborator

@mssalvatore mssalvatore commented Jan 12, 2021

What does this PR do?

Fixes #891.

Master checkboxes can now have a mixed state.

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 documentation framework updated to reflect the changes?

Testing Checklist

  • Added relevant unit tests? (N/A)
  • Have you successfully tested your changes locally? Elaborate:

    Tested by running the Monkey Island locally

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

Testing GIF

checkbox

Explain Changes

The AdvancedMultiSelect should adhere to some set of human interface guidelines. In the absence of a formal, agreed upon set of guidelines for Infection Monkey, this commit uses KDE's guidelines for checkboxes: https://hig.kde.org/components/editing/checkbox.html

When child checkboxes are not all checked, the master checkbox displays a mixed-state icon, instead of a checked icon. Clicking the mixed-state icon checks all child checkboxes. Clicking an unchecked master checkbox also enables all child checkboxes.

In the past, clicking an unchecked master checkbox checked only the default child checkboxes. While this may seem desirable so that unsafe exploits do not accidentally get selected by the user, it will confuse and frustrate users, as master/child checkboxes do not normally function this way. If there is concern that users may unknowingly select unsafe exploits/options, we should pop up a warning to inform the user when the config is saved/submitted.

AdvancedMultiSelect can be broken up and composed of smaller, more
focused components. This commit refactors AdvancedMultiSelect from a
functional component to a class component.
…elect

The AdvancedMultiSelect should adhere to some set of human interface
guidelines. In the absence of a formal, agreed upon set of guidelines
for Infection Monkey, this commit uses KDE's guidelines for checkboxes:
https://hig.kde.org/components/editing/checkbox.html

When child checkboxes are not all checked, the master checkbox displays
a mixed-state icon, instead of a checked icon. Clicking the mixed-state
icon checks all child checkboxes. Clicking an unchecked master checkbox
also enables all child checkboxes.

In the past, clicking an unchecked master checkbox checked only the
*default* child checkboxes. While this may seem desirable so that unsafe
exploits do not accidentally get selected by the user, it will confuse
and frustrate users, as master/child checkboxes do not normally function
this way. If there is concern that users may unknowingly select unsafe
exploits/options, we should pop up a warning to inform the user when the
config is saved/submitted.

Issue guardicore#891
@mssalvatore mssalvatore changed the title Refactor advanced multiselect WIP: Refactor advanced multiselect Jan 12, 2021
@codecov
Copy link

codecov bot commented Jan 12, 2021

Codecov Report

Merging #920 (09a8415) into develop (816c108) will decrease coverage by 41.49%.
The diff coverage is 7.57%.

Impacted file tree graph

@@             Coverage Diff              @@
##           develop     #920       +/-   ##
============================================
- Coverage    60.56%   19.07%   -41.50%     
============================================
  Files          166      338      +172     
  Lines         4953    11482     +6529     
============================================
- Hits          3000     2190      -810     
- Misses        1953     9292     +7339     
Impacted Files Coverage Δ
monkey/common/cloud/azure/azure_instance.py 0.00% <0.00%> (ø)
monkey/common/cloud/gcp/gcp_instance.py 0.00% <0.00%> (ø)
monkey/infection_monkey/exploit/drupal.py 0.00% <0.00%> (ø)
monkey/infection_monkey/exploit/hadoop.py 0.00% <0.00%> (ø)
monkey/infection_monkey/exploit/shellshock.py 0.00% <ø> (ø)
monkey/infection_monkey/exploit/smbexec.py 0.00% <0.00%> (ø)
monkey/infection_monkey/exploit/web_rce.py 0.00% <0.00%> (ø)
monkey/infection_monkey/monkey.py 0.00% <0.00%> (ø)
monkey/infection_monkey/network/network_scanner.py 0.00% <0.00%> (ø)
monkey/infection_monkey/network/tools.py 17.44% <0.00%> (ø)
... and 528 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 17ee823...09a8415. Read the comment docs.

@mssalvatore mssalvatore changed the title WIP: Refactor advanced multiselect Refactor advanced multiselect Jan 12, 2021
Copy link
Contributor

@VakarisZ VakarisZ left a comment

Choose a reason for hiding this comment

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

Nice touch of adding mixed state to master checkbox component! Also, did some good refactoring, but there are some things that I don't like/understand.

@mssalvatore
Copy link
Collaborator Author

This PR is getting fairly long and I think it has accomplished it's mission:

  1. Resolve issue Plugin selector has wrong inital state. #891
  2. Make the master checkbox behavior consistent with a user's expectations
  3. More clearly indicate to the user which options are unsafe.

This PR is missing a modal dialog that pops up and warns the user when they import or attempt to submit an unsafe config. #946 has been created to track that feature.

Given that, now, when unsafe options are selected, users have more warning than they did before, I think this PR can proceed without the inclusion of the modal dialog.

advanced_multiselect

Copy link
Contributor

@VakarisZ VakarisZ left a comment

Choose a reason for hiding this comment

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

Preem quality stuff. But look at my suggestions before merging

@mssalvatore mssalvatore merged commit 13af101 into guardicore:develop Feb 1, 2021
@mssalvatore mssalvatore deleted the refactor-advanced-multiselect branch February 1, 2021 13:47
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.

Plugin selector has wrong inital state.
3 participants