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

Expose Actor and Asset lists in report template #150

Merged
merged 3 commits into from
Apr 7, 2021
Merged

Conversation

nozmore
Copy link
Collaborator

@nozmore nozmore commented Apr 2, 2021

I was trying to use the report framework to mimc existing TM output where annotations for components (Assets in pytm) are separate from dataflows. I couldn't do this currently as Assets were only included in Elements so I replicated what was done for boundaries, flows and elements for assets.

Everything works locally in my manual tests, also updated json test data to reflect inclusion of the asset list and confirm all tests now pass.

One difference here is that the other TM._lists are actually used in the code for logic purposes, boundaries, flows, elements are looped thru in various points to process the TM. Currently this not needed for Assets but seems reasonable there may be a use case in the future.

I implemented the same for Actors.

Alternatively if we don't want to include additional TM._lists the report issue could be addressed by filtering elements in the TM.report method.

`assets = [ obj for obj in TM._elements if not isinstance(obj, Asset) ]

     data = {
         "tm": self,
         "dataflows": TM._flows,
         "threats": TM._threats,
         "findings": self.findings,
         "elements": TM._elements,
         "assets": assets,
         "boundaries": TM._boundaries,
         "data": TM._data,

`

@nozmore nozmore requested a review from izar as a code owner April 2, 2021 17:48
@nozmore
Copy link
Collaborator Author

nozmore commented Apr 2, 2021

If we want to proceed with this approach I can do the same for Actors in this PR, otherwise I can revert and just filter elements in the report method.

@ghost
Copy link

ghost commented Apr 2, 2021

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.

@@ -869,6 +871,7 @@ def report(self, template_path):
"threats": TM._threats,
"findings": self.findings,
"elements": TM._elements,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder should we replace _elements with _assets. It's easy to combine both lists and it'll be more explicit. The word elements is ambiguous anyway. @nozmore @izar WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't have a strong opinion either way as for the existence of _elements in the code. Obviously I agree with using assets since I raised the PR : ) I would probably go a step further and remove elements from json output and as mentioned add an Actor list so that objects are only referenced once. You could hold off on that for a major version to avoid breaking changes or do so now but add a bool flag on TM to remove elements and maintain existing functionality.

@nozmore nozmore changed the title Expose Asset list in report template Expose Actor and Asset lists in report template Apr 6, 2021
@nozmore
Copy link
Collaborator Author

nozmore commented Apr 6, 2021

I updated the PR with a similar change for Actors so its ready to merge if desired.

@izar izar merged commit fd01a1f into master Apr 7, 2021
@izar izar deleted the tm_asset_list branch April 12, 2021 20:55
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.

4 participants