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

Removes pre-ATT&CK domain #218

Merged
merged 27 commits into from
Sep 2, 2020
Merged

Conversation

clemiller
Copy link
Contributor

Removes the pre-ATT&CK domain from the application, including:

  • Removal of pre-ATT&CK urls from the config file
  • Removal of "stages" in the "filters" interface and the layer format documentation
  • Removal of data loading for pre-ATT&CK domain when updating outdated layers with the update-layers script
  • Addition of option to select/deselect all techniques in a tactic, which can be done in both the context menu or by clicking the tactic name. These options will follow the user's selection preference under "selection behavior".

Resolves Issue #207

@clemiller clemiller requested a review from isaisabel August 17, 2020 17:23
Copy link
Contributor

@isaisabel isaisabel 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 pretty good.

PRE-ATT&CK removal comments

  • See below about layer upgrade script
  • Help page links to v3 of the layer file despite your adding the v4 pdf. Hyperlink just needs to be updated to point to the new file. The link is at the end of the 4th paragraph of the layers section.

Tactic selection comments

There's a bit of leftover logic from the old tactic highlighting UX which produces odd behavior. The old UX had a "toggle" behavior when the tactic header was clicked on. Because the new behavior involves techniques selections, the toggle doesn't really work anymore and produces unintuitive behavior.

In a viewModel, the selectedTactic field keeps track of the "tactic selection" even if the user deselects the techniques manually. For instance:

  1. click "initial access" header to select techniques in that tactic.
  2. click "native API" in execution, or really any technique not in initial access. Initial access techniques are now deselected.
  3. Click the "initial access" header again. The App now is trying to deselect techniques in that tactic because it thinks that tactic is selected, but it is not selected according to the UI. So it looks like nothing happens (though internally selectedTactic is being reset and techniques in that tactic deselected, though none were selected in the first place).

There are two possible ways to fix this.

  1. Remove the "toggle selection" feature altogether; remove the ability to deselect tactics via clicking on the tactic header. Clicking the tactic header will always apply the selection to the techniques in the tactic.
  2. Modify isTacticSelected to check if all the techniques in the tactic are selected. This should allow the old toggle "click the header again to deselect" UX to work provided all the techniques in the tactic are selected when it's clicked. Essentially this will mean the "selection status" of the tactic is determined dynamically, and not from the value of a single boolean.

Either way, the selectedTactic property should be removed.

layers/update-layers.py Show resolved Hide resolved
@clemiller
Copy link
Contributor Author

Hey Isabel, thank you for the suggestions for the tactic selection behavior issue! I've opted for solution 2: dynamically determining if all techniques in a tactic are selected or not, and cleaned up some of that code in the process.

@clemiller clemiller requested a review from isaisabel August 25, 2020 18:45
Copy link
Contributor

@isaisabel isaisabel left a comment

Choose a reason for hiding this comment

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

Resolves all the previously identified issues.

One possible improvement that I could go either way on:

Selecting a tactic doesn't select sub-techniques within the tactic unless "select sub-techniques with parent" is enabled. This can be unintuitive since the sub-techniques are under the tactic regardless of whether that control is active. We might want to consider always selecting sub-techniques when selecting a tactic for more logically consistent behavior ("everything in that column, technique or subtechnique, gets selected").

@clemiller
Copy link
Contributor Author

clemiller commented Aug 27, 2020

Would there be a use case for not selecting the sub-techniques under the tactic? Changing the current logic would remove users' ability to choose the "sub-technique selection" behavior when selecting a tactic, since it would always select the sub-techniques under the tactic whether the control has been selected or not. If that's not a concern, I can definitely make that logic change if it's more intuitive that way.

@isaisabel
Copy link
Contributor

Yeah that's why I said "I could go either way on [it]" -- I haven't thought of a use case where a user would want to select techniques in a tactic but not sub-techniques, but there could be one? There's certainly more user control over the behavior if we honor the "select sub-techniques with parent" control.

One more argument in favor of keeping it the way it is (and not forcing sub-technique selection) is that the context menu prompt says "techniques" and not "techniques and sub-techniques."

I guess my recommendation is that we can keep it the way it is, but make it clear in the documentation for the tactic selection behavior that it only selects sub-techniques if "select with parents" is enabled.

@isaisabel
Copy link
Contributor

The help page already says:

This action follows the behavior preference under selection behavior in the selection controls.

I think this needs to be more explicit about what "follows" means with respect to cross-tactic techniques and techniques with sub-techniques.

@isaisabel isaisabel merged commit 1a68270 into feature/v4.0 Sep 2, 2020
@clemiller clemiller deleted the feature/remove-pre-attack branch February 17, 2022 16:46
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