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

505 merging of multiple bundles into a single matrix #613

Merged

Conversation

adpare
Copy link
Contributor

@adpare adpare commented Feb 7, 2024

This PR addresses issue #505.

@adpare adpare requested a review from clemiller February 7, 2024 20:05
Comment on lines 36 to 38
public get_technique_domain(stixSDO: any): string {
return stixSDO.x_mitre_domains[0];
}
Copy link
Contributor

@clemiller clemiller Feb 21, 2024

Choose a reason for hiding this comment

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

  • For techniques in multiple domains, this function would only check the first listed domain and disregard all other domains. While ATT&CK doesn't currently have cross-domain techniques, this should support custom datasets that do.
  • I suggest removing the stixSDO parameter from this function, and instead update the Technique constructor (which already has access to the stixSDO) to store the x_mitre_domain field as a class property. This would enhance the accessibility of the data within the class.

Comment on lines 239 to 241
if(domain.techniques[j].get_technique_domain(techniqueSDOslist[j]) == matricesList[i].external_references[0].external_id) {
techniquesList.push(domain.techniques[j]);
}
Copy link
Contributor

@clemiller clemiller Feb 21, 2024

Choose a reason for hiding this comment

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

This logic should be updated to check if the matrix ID matches any of the domains listed in the technique's x_mitre_domains field. See other comment for more details.

matricesList[i].external_references[0].external_id == domain[techniques[j].x_mitre_domains

Copy link
Contributor

@clemiller clemiller left a comment

Choose a reason for hiding this comment

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

This looks great. The functionality to view multiple bundles/matrices is working. I have one comment on the logic for checking if a technique is part of a matrix/domain.

@clemiller clemiller self-requested a review February 22, 2024 00:15
Copy link
Contributor

@clemiller clemiller left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for making these updates. Please take a look at the two issues reported by SonarCloud and see if they can be addressed. Once that's done, we can merge this in!

Copy link

sonarcloud bot commented Feb 23, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@clemiller clemiller merged commit eb76a01 into develop Feb 25, 2024
2 checks passed
@clemiller clemiller deleted the 505-merging-of-multiple-bundles-into-a-single-matrix branch February 25, 2024 13:35
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