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

allow automatic ordering of dataflows #66

Merged
merged 1 commit into from
Feb 18, 2020
Merged

Conversation

nineinchnick
Copy link
Collaborator

Allow automatic ordering of dataflows by setting the TM.isOrdered property to True. Only Dataflows without an order assigned are automatically enumerated. This might be slightly confusing if any/not all Dataflows are ordered manually but should not be an issue since isOrdered defaults to False.

@ghost
Copy link

ghost commented Feb 8, 2020

DeepCode's analysis on #ba4d14 found:

  • 0 critical issues. ⚠️ 0 warnings and 0 minor issues. ✔️ 0 issues were fixed.

💬 This comment has been generated by the DeepCode bot, installed by the owner of the repository. The DeepCode bot protects your repository by detecting and commenting on security vulnerabilities or other critical issues.


☺️ If you want to provide feedback on our bot, here is how to contact us.

@izar
Copy link
Collaborator

izar commented Feb 11, 2020

I'm not quite sure on this one - it creates a situation where the developer has to keep dataflow state in mind when writing the threat model. Using just element.order they are free to describe flows in any order and then apply order. Can you convince me otherwise?

@nineinchnick
Copy link
Collaborator Author

I'll certainly try!

I think there are 3 main workflows:

  1. Only caring about the data flow diagram without having flows ordered at all (numbers attached in labels)
  2. Having custom/partial order
  3. Having everything ordered to either have the data flow diagram ordered and/or using the sequence diagram.

For 1 and 2, auto ordering is disabled by default. Turning it on with having only some Dataflows manually ordered would result in having duplicates. This is on purpose to avoid adding complex logic.

For 3, I found that I do keep Dataflow instances ordered in code as code is sequential and manually managing the order field is tedious when rearranging the code. If I care about the sequence diagram I do want my code to be arranged in the same order so it's easier to read.

As a workaround, I used something like this:

current_order = 0

a = Dataflow("a")
a.order = current_order = current_order + 1

b = Dataflow("b")
b.order = current_order = current_order + 1

but this is unnecessarily verbose.

I admit I have not tried combining Dataflows from multiple files but again, the option for auto numbering is off by default.

@izar
Copy link
Collaborator

izar commented Feb 12, 2020

You make a very good point. Could you perhaps summarize it in the README.md ? Perhaps a section on "TM global attributes" is in order. Also, would you say it should be flippable by command line argument?

@nineinchnick
Copy link
Collaborator Author

Updating the README warrants a whole new MR. I do noticed that most of the properties are not self-explanatory and they badly need a description. I'd add it to varXXX classes but first I do have to go through all threat rules to figure out the meaning of the props.

For this MR I only updated the example in the README. I don't think this should be added as a cli param since it's part of the model and changing it on the fly would make the output less deterministic.

@nineinchnick
Copy link
Collaborator Author

This is ready to be merged

@izar izar merged commit 0e5863f into OWASP:master Feb 18, 2020
@nineinchnick nineinchnick deleted the ordered branch March 12, 2020 17:28
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