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.

Sign up for GitHub

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

Jump to bottom

Unsafe options confirmation #1000

Merged
merged 21 commits into from
Mar 1, 2021
Merged

Unsafe options confirmation #1000

merged 21 commits into from
Mar 1, 2021

Conversation

mssalvatore
Copy link
Collaborator

What does this PR do?

Adds a modal dialog warning the user when unsafe options are selected. This prevents users from accidentally selecting unsafe options.

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?
  • Have you successfully tested your changes locally? Elaborate:

    Tested locally by submitting, exporting, and importing configs.

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

@ghost
Copy link

ghost commented Feb 26, 2021 •
edited by ghost
Loading

DeepCode failed to analyze this pull request

Something went wrong despite trying multiple times, sorry about that.
Please comment this pull request with "Retry DeepCode" to manually retry, or contact us so that a human can look into the issue.

@mssalvatore
Copy link
Collaborator Author

I'm considering showing this dialog when the user attempts to run the monkey instead of when config options are selected. I think the code will be cleaner and it will be a more effective safeguard for the user.

@codecov
Copy link

codecov bot commented Feb 26, 2021 •
edited
Loading

Codecov Report

Merging #1000 (5050f33) into develop (bc3283c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1000   +/-   ##
========================================
  Coverage    26.11%   26.11%           
========================================
  Files          402      402           
  Lines        12821    12821           
========================================
  Hits          3348     3348           
  Misses        9473     9473

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 bc3283c...5050f33. Read the comment docs.

mssalvatore and others added 2 commits February 26, 2021 08:10
@mssalvatore @VakarisZ
ui: minor change to unsafe modal dialog language
f094efb 
Co-authored-by: VakarisZ <36815064+VakarisZ@users.noreply.github.com>
@mssalvatore
ui: pass callback, not return value, to setState()
7079a6f
VakarisZ
VakarisZ requested changes Feb 26, 2021
View reviewed changes
...onkey_island/cc/ui/src/components/configuration-components/UnsafeOptionsConfirmationModal.js Outdated Show resolved Hide resolved
...onkey_island/cc/ui/src/components/configuration-components/UnsafeOptionsConfirmationModal.js Outdated Show resolved Hide resolved
...onkey_island/cc/ui/src/components/configuration-components/UnsafeOptionsConfirmationModal.js Outdated Show resolved Hide resolved
...onkey_island/cc/ui/src/components/configuration-components/UnsafeOptionsConfirmationModal.js Outdated Show resolved Hide resolved
...onkey_island/cc/ui/src/components/configuration-components/UnsafeOptionsConfirmationModal.js Show resolved Hide resolved
monkey/monkey_island/cc/ui/src/components/pages/ConfigurePage.js Outdated Show resolved Hide resolved
monkey/monkey_island/cc/ui/src/components/pages/ConfigurePage.js Outdated Show resolved Hide resolved
monkey/monkey_island/cc/ui/src/components/pages/ConfigurePage.js Outdated Show resolved Hide resolved
monkey/monkey_island/cc/ui/src/components/pages/ConfigurePage.js Outdated Show resolved Hide resolved
monkey/monkey_island/cc/ui/src/components/pages/ConfigurePage.js Outdated Show resolved Hide resolved
@mssalvatore mssalvatore force-pushed the unsafe-options-confirmation branch 2 times, most recently from fdb5c43 to 473091d Compare February 26, 2021 18:37
@mssalvatore
Copy link
Collaborator Author

Retry DeepCode

VakarisZ
VakarisZ reviewed Mar 1, 2021
View reviewed changes
monkey/monkey_island/cc/ui/src/components/pages/ConfigurePage.js Outdated
@@ -11,6 +11,8 @@ import {faExclamationCircle} from '@fortawesome/free-solid-svg-icons/faExclamati
import {formValidationFormats} from '../configuration-components/ValidationFormats';
import transformErrors from '../configuration-components/ValidationErrorMessages';
import InternalConfig from '../configuration-components/InternalConfig';
import UnsafeOptionsConfirmationModal from '../configuration-components/UnsafeOptionsConfirmationModal.js';
import isUnsafeOptionSelected from '../utils/SafeOptionValidator.js'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import isUnsafeOptionSelected from '../utils/SafeOptionValidator.js'
import isUnsafeOptionSelected from '../utils/SafeOptionValidator.js';

Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ that you extracted the logic

VakarisZ
VakarisZ reviewed Mar 1, 2021
View reviewed changes
monkey/monkey_island/cc/ui/src/components/pages/ConfigurePage.js Outdated
@@ -74,6 +78,20 @@ class ConfigurePageComponent extends AuthComponent {
});
};

onUnsafeConfirmationCancelClick = () => {
this.setState({showUnsafeOptionsConfirmation: false})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this.setState({showUnsafeOptionsConfirmation: false})
this.setState({showUnsafeOptionsConfirmation: false});

VakarisZ
VakarisZ reviewed Mar 1, 2021
View reviewed changes
monkey/monkey_island/cc/ui/src/components/pages/ConfigurePage.js Outdated
@@ -87,10 +105,14 @@ class ConfigurePageComponent extends AuthComponent {
if (this.state.selectedSection === 'attack') {
this.matrixSubmit()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this.matrixSubmit()
this.matrixSubmit();

VakarisZ
VakarisZ reviewed Mar 1, 2021
View reviewed changes
monkey/monkey_island/cc/ui/src/components/pages/ConfigurePage.js Outdated
@@ -87,10 +105,14 @@ class ConfigurePageComponent extends AuthComponent {
if (this.state.selectedSection === 'attack') {
this.matrixSubmit()
} else {
this.configSubmit()
this.attemptConfigSubmit()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this.attemptConfigSubmit()
this.attemptConfigSubmit();

VakarisZ
VakarisZ reviewed Mar 1, 2021
View reviewed changes
monkey/monkey_island/cc/ui/src/components/pages/ConfigurePage.js Outdated
this.updateConfigSection();
this.setState({lastAction: 'submit_attempt'}, () => {
if (this.canSafelySubmitConfig(this.state.configuration)) {
this.configSubmit()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this.configSubmit()
this.configSubmit();

VakarisZ
VakarisZ reviewed Mar 1, 2021
View reviewed changes
monkey/monkey_island/cc/ui/src/components/pages/ConfigurePage.js Outdated
onCancelClick={this.onUnsafeConfirmationCancelClick}
onContinueClick={this.onUnsafeConfirmationContinueClick}
/>
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
)
);

VakarisZ
VakarisZ reviewed Mar 1, 2021
View reviewed changes
monkey/monkey_island/cc/ui/src/components/pages/ConfigurePage.js Outdated
lastAction: 'import_success'
}, () => {
this.sendConfig();
this.setInitialConfig(this.state.importCandidateConfig)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this.setInitialConfig(this.state.importCandidateConfig)
this.setInitialConfig(this.state.importCandidateConfig);

VakarisZ
VakarisZ approved these changes Mar 1, 2021
View reviewed changes
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.

Approved, but we should strive to be consistent with semicolons

@mssalvatore mssalvatore merged commit cfaf4a1 into develop Mar 1, 2021
@mssalvatore mssalvatore deleted the unsafe-options-confirmation branch March 1, 2021 15:25
This was referenced Mar 1, 2021
Merged
Closed
@mssalvatore mssalvatore mentioned this pull request Aug 13, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VakarisZ VakarisZ VakarisZ approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mssalvatore @VakarisZ