search

OWASP / threat-dragon

An open source threat modeling tool from OWASP
https://owasp.org/www-project-threat-dragon/
Apache License 2.0
902 stars 244 forks source link

Undo not fully functional #428

Closed github-itsec-oculavis closed 5 months ago

github-itsec-oculavis commented 2 years ago

Describe the bug While the undo operation works for deleted processes, actors, and data storages, it doesn´t work for arrows (data flows). If you delete one of the aforementioned diagrams which were previously connected with arrows, after "undo", the diagrams will show up again, but the arrows are gone, together with the threats defined for them. Undo also doesn´t work if you just delete an arrow and want to undo it.

Expected behaviour After accidental deletion of an arrow, or diagrams connected with arrows, the undo button should make the arrows reappear. It is particularly important when threats were already defined for those data flows.

Environment

To Reproduce

  1. Create two actors.
  2. Connect them with a data flow arrow.
  3. Delete one of the actors. (The arrow should also disappear.)
  4. Undo the operation. (The actor reappears but the connection arrow doesn´t.)

Further information

jgadsden commented 2 years ago

Thanks for finding this @github-itsec-oculavis

@lreading I remember that one of the companies we talked to who were using Threat Dragon a lot, they did not like the way the arrows / data-flows were deleted if the diagram component was deleted. I think if we alter that behavious then this may also be corrected?

I can experiment ...

lreading commented 2 years ago

I do remember having that discussion! I think we may have done something a bit custom with the undo feature, and IIRC it works off of the history stack at the graph level. Related: https://github.com/OWASP/threat-dragon/issues/201

This might have been due to how the data flows that are connected work, as they reference other objects rather than absolute coordinates, and antv handles finding the best path for them. My memory is a bit fuzzy, but I 100% agree with the current behavior not being the best user-experience! I'd love to find a better solution for it! :smile:

jgadsden commented 2 years ago

In addition a user reports "Undo button does not undo deletion of objects. This is a big deal because objects are too easy to remove accidentally. If it is too hard to undo deletion, perhaps an “are you really sure” button would help with accidental deletes?"

jgadsden commented 1 year ago

Unlikely to get this into the imminent version 2.0 release, so pushing it back to version 2.x

ChristopherHackett commented 1 year ago

I would be interested in taking a look at this so scanned for existing tests that look at the undo behaviour.

I found tests re the binding and correctness of things like canUndo but nothing dedicated to something like "place x, y and z, connect x & y, assert z and y connected".

If something like the above exists please can I be pointed in the direction of them. Alternatively, do existing maintainers have any helpful pointers about in adding this type of test in before I start to look into more detail at the undo functionality.