diff --git a/docs/api-reference.md b/docs/api-reference.md index 5be9ffd..e64f173 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -41,6 +41,7 @@ and `/openapi` are matched first. | `POST /v1/model/threat-register` | Model | Split the register by origin and standing: manual, current-generated, stale-generated, and entries whose rule was not part of the run. | | `POST /v1/model/read` | Model | Parse uploaded bytes (base64) into the canonical model. | | `POST /v1/model/manifest` | Model | Materialize a declarative authoring manifest into a model (the `tmforge apply` build). | +| `POST /v1/model/layout` | Model | Return geometry-only updates after preserving every boundary membership and actual flow crossing; unsafe candidates are refused atomically. | | `POST /v1/model/convert?to=` | Model | Convert a model to another format. | | `POST /v1/model/export/tm7` | Model | Export a model as a `.tm7` file. | | `POST /v1/model/report?format=` | Report | Render a model to an HTML or SVG report. | @@ -93,6 +94,45 @@ curl -s -X POST http://localhost:8080/v1/model/analysis \ combined action returns for their half; they exist for callers that genuinely need only one projection, and they do not materialize the other. +## Shared arrangement + +`POST /v1/model/layout` accepts a `LayoutRequestDto`: `model` is the **original** canonical model; +optional `page` selects a name or one-based index (otherwise every page); and optional `options` +supplies layout metrics such as +`nodeSpacing`, `layerSpacing`, `maxWidth`, and `boundaryHeaderHeight`. + +Without `positions`, the engine generates a bounded arrangement, as used by the CLI's explicit +layout command. `maxWidth` defaults to `1760` for its MTMT-oriented layout. + +For in-place cleanup, supply `positions: [{id,x,y,width,height}, ...]` for every element on the +selected pages. The engine validates those exact rectangles against the original +model's memberships and crossings, recomputing connector endpoints but never re-layering the +candidate. Partial, duplicate, unknown or out-of-bounds positions are refused. +Studio's one-click Tidy uses only this validation path with the +released cleanup algorithm; Studio does not request engine-generated rearrangement. + +Include text fitting in the proposed positions, not by resizing the input model first: the original geometry is +the baseline against which memberships and crossings must be preserved. Unknown or duplicate ids, +cross-page/dangling flows, boundary-ended flows, invalid geometry and excessive work are refused rather +than normalized into a different model. No analysis rules are evaluated, and property bags and +triage are not hydrated into the geometry candidate. + +The response is `{success,error,elements,pages,components,labelOverlaps}`. On success, `elements` +contains only `{id,x,y,width,height}` keyed by the original author ids. Apply those rectangles to the +existing document; do not replace it. Pages, topology, properties, rule selections, triage and view +state remain owned by the caller. A refusal returns HTTP `200` with `success: false`, a reason, and +an empty `elements` array; malformed request JSON still receives the ordinary `400` problem response. + +All requested pages succeed or none do. The engine checks complete component membership, both +connector endpoints' boundary sides, and crossing sets using the analyzer's boundary implementation. +Overlapping regions are not assumed to be invalid; an arrangement that would drop a claim is refused. +There is no force bypass. See the [CLI layout limits](cli-reference.md#layout) for computation bounds; +canonical model dimensions and proposed rectangles must be at least 20 units in width and height. + +The WASM `Layout(requestJson)` export returns the identical contract. Studio retains client-side +text measurement when computing the candidate, then visual routing and label deconfliction after +the shared validation step. + ## Custom rule packs Custom rules are deployment configuration, not request input: this host never loads rules from a diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 4534f59..0dd6c88 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -521,6 +521,24 @@ arrangement that moved a component out of its boundary would change what the mod how it looks. Columns wrap onto a new row instead of running past the right-hand edge, because the Microsoft Threat Modeling Tool's drawing surface is bounded and taller than it is wide. +Arrangement is **validated before it is committed**. Every component keeps its complete set of +boundary memberships, each connector endpoint stays on the same side of every boundary, and actual +crossing sets (including line trust boundaries) must be unchanged. If any selected page fails, the +command exits `1` and writes nothing. Partially overlapping claims are not discarded, and flows +attached to a boundary rather than a component are refused. Use `--labels` to keep such geometry, +or resolve the placement explicitly; there is no force flag that silently changes trust claims. + +Spacing arguments must be integers from `1` to `4096`. An arrangement request is limited to 32 pages, +512 shapes and 1,024 lines, with a separate 25-million-work-unit bound for dense graphs and label +placement. Shape sizes must be positive and at most 100,000 units, and input/output coordinates must +be within ±1,000,000. These are computation limits, not a promise that an oversized diagram fits +the MTMT drawing surface. + +On canonical JSON, layout patches only the selected rectangles and retains author ids, flow +aliases, properties, analysis settings, triage, and unknown extension/view fields. JSON does not +persist the engine's connector handles: `--labels` leaves it unchanged, and `labelsPersisted: false` +in JSON output distinguishes that from a persisted label edit. Studio handles its own label offsets. + ```text tmforge layout [--page ] [--node-spacing ] [--layer-spacing ] [--labels] [--check] [--json] ``` @@ -529,6 +547,7 @@ tmforge layout [--page ] [--node-spacing ] [--layer-spacing ] | --- | --- | | `--labels` | Place only the flow labels and leave every shape exactly where it is. Use this when the geometry is hand-placed or comes from a manifest and only the labels need sorting out. | | `--check` | Report obstructed flow labels and write nothing. Exits `1` when any remain, so a publishing gate can require a legible diagram. | +| `--page` | Arrange one page by name or one-based index. Omitted means every page; refusal is atomic across the selected pages. | ```bash tmforge layout payments.tm7 diff --git a/docs/studio-guide.md b/docs/studio-guide.md index dc792f6..66a71eb 100644 --- a/docs/studio-guide.md +++ b/docs/studio-guide.md @@ -54,11 +54,36 @@ connection, not the geometry. | Select several objects | Hold `Cmd` (`Ctrl` on Windows/Linux) and click, or drag a selection box with `Shift`. | | Delete the selection | `Delete` key. Deleting an element takes its flows with it, and the whole deletion is a single undo step. | | Resize a trust boundary | Drag its handles (it's a resizable region). | -| Tidy the diagram | Click **Tidy** to fit labels, separate overlapping shapes and peer trust boundaries, route flows, and deconflict flow labels. Each boundary also keeps a clear strip at its top so nothing covers its own name. Nested boundaries remain nested, and each boundary moves with its members. | +| Tidy the diagram | Click **Tidy** to clean up the existing arrangement: fit text, separate overlaps, route flows and deconflict labels. The adjacent **Tidy options** menu offers offline **Labels only** with fixed rectangles. | | Pan / zoom | Drag the canvas / scroll; use the minimap and **fit** control to navigate. | | Step through the flows | `Alt+↓` / `Alt+↑` selects the next / previous flow in the outline's order. | | Undo / redo | `Cmd+Z` / `Shift+Cmd+Z` (covers every edit). | +Arrangement applies to the **active page**, as one undo step. A failed or unsafe candidate changes +nothing and consumes no undo step. If an edit or page switch occurs while the engine is working, +the response is discarded rather than overwriting newer work. Repeating an unchanged arrangement is +a no-op. + +**Tidy preserves the author's arrangement**, using the released Studio cleanup algorithm. It does +not move an already horizontal sketch into graph layers or shrink boundaries around newly ordered +groups. Objects move only as text fitting and overlap separation require. The engine validates the +proposed rectangles without rearranging them again, so the boundary-safety checks still apply. + +Studio does not offer full layout rearrangement. The CLI's explicit `layout` command remains +available for callers that want to generate placement rather than tidy an existing drawing. + +The engine preserves the **actual geometric trust claims**, including all memberships when regions +overlap; it never chooses one claim and discards another. If the candidate cannot preserve them, +Studio explains the refusal. Use **Labels only**, or edit the boundary placement explicitly before +trying again. Opening a file now preserves its shape and boundary rectangles: automatic import +cleanup is limited to visual routing and label offsets. Text-fit sizing requires an explicit +arrangement so importing a model cannot silently change analysis. + +A single-page model can still contain a detached flow. An imported connector whose source or target +id is all zeros is not attached to an element. Tidy names the affected flow and endpoint; reconnect +it in the source model, or remove the flow if it is unintended. Tidy never guesses the missing target +or silently drops the connection. **Labels only** remains available without rearranging shapes. + ### Pages A model can hold several diagrams. The **page tab strip** below the canvas lets you work across them: @@ -104,9 +129,10 @@ the listed order, wrapping at either end, so a review can be worked through flow hunted for. Each step also opens that flow in the inspector, so its properties are right there while you read it. -An object is placed in a boundary by its authored `Boundary` property when it has one, and otherwise -by the smallest boundary region it sits inside — the same rule **Tidy** uses, so the list and the -drawing always agree. +The outline groups an object by its authored `Boundary` property when it has one, and otherwise by +the smallest boundary region containing its center. Arrangement instead preserves the complete +geometry-derived membership set used for analysis. If a declared property disagrees with the +drawing, resolve that disagreement explicitly rather than relying on Tidy to change the trust claim. ### The inspector diff --git a/src/ThreatModelForge.Api/Program.cs b/src/ThreatModelForge.Api/Program.cs index f114e3f..c52dff5 100644 --- a/src/ThreatModelForge.Api/Program.cs +++ b/src/ThreatModelForge.Api/Program.cs @@ -108,6 +108,11 @@ public static void Main(string[] args) request.Theirs ?? new TmForgeModelDto()))) .WithName("MergeModels") .WithTags("Model"); + app.MapPost( + "/v1/model/layout", + (LayoutRequestDto request) => TypedResults.Ok(EngineService.Layout(request))) + .WithName("LayoutModel") + .WithTags("Model"); app.MapPost( "/v1/model/export/tm7", (TmForgeModelDto model) => TypedResults.File(EngineService.ExportTm7(model, rules), "application/xml", "model.tm7")) diff --git a/src/ThreatModelForge.Api/openapi/v1.json b/src/ThreatModelForge.Api/openapi/v1.json index 92a9e63..6b444a3 100644 --- a/src/ThreatModelForge.Api/openapi/v1.json +++ b/src/ThreatModelForge.Api/openapi/v1.json @@ -369,6 +369,36 @@ } } }, + "/v1/model/layout": { + "post": { + "tags": [ + "Model" + ], + "operationId": "LayoutModel", + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/LayoutRequestDto" + } + } + }, + "required": true + }, + "responses": { + "200": { + "description": "OK", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/LayoutResultDto" + } + } + } + } + } + } + }, "/v1/model/export/tm7": { "post": { "tags": [ @@ -853,6 +883,235 @@ } } }, + "LayoutElementDto": { + "type": "object", + "properties": { + "id": { + "type": "string" + }, + "x": { + "pattern": "^-?(?:0|[1-9]\\d*)$", + "type": [ + "integer", + "string" + ], + "format": "int32" + }, + "y": { + "pattern": "^-?(?:0|[1-9]\\d*)$", + "type": [ + "integer", + "string" + ], + "format": "int32" + }, + "width": { + "pattern": "^-?(?:0|[1-9]\\d*)$", + "type": [ + "integer", + "string" + ], + "format": "int32" + }, + "height": { + "pattern": "^-?(?:0|[1-9]\\d*)$", + "type": [ + "integer", + "string" + ], + "format": "int32" + } + } + }, + "LayoutOptions": { + "type": "object", + "properties": { + "originX": { + "pattern": "^-?(?:0|[1-9]\\d*)$", + "type": [ + "integer", + "string" + ], + "description": "Gets or sets the x coordinate of the top-left origin of the laid-out region.", + "format": "int32" + }, + "originY": { + "pattern": "^-?(?:0|[1-9]\\d*)$", + "type": [ + "integer", + "string" + ], + "description": "Gets or sets the y coordinate of the top-left origin of the laid-out region.", + "format": "int32" + }, + "layerSpacing": { + "pattern": "^-?(?:0|[1-9]\\d*)$", + "type": [ + "integer", + "string" + ], + "description": "Gets or sets the horizontal gap between adjacent layers (columns).", + "format": "int32" + }, + "nodeSpacing": { + "pattern": "^-?(?:0|[1-9]\\d*)$", + "type": [ + "integer", + "string" + ], + "description": "Gets or sets the vertical gap between adjacent nodes within a layer.", + "format": "int32" + }, + "maxWidth": { + "pattern": "^-?(?:0|[1-9]\\d*)$", + "type": [ + "integer", + "string" + ], + "description": "Gets or sets the width a row of columns may occupy before the next column wraps onto a new\nrow. The Microsoft Threat Modeling Tool's drawing surface is bounded and taller than it is\nwide, so a wide model has to grow downwards; anything drawn past the right-hand limit is\nclamped by the tool on load, which would pile elements on top of each other.", + "format": "int32" + }, + "boundaryPadding": { + "pattern": "^-?(?:0|[1-9]\\d*)$", + "type": [ + "integer", + "string" + ], + "description": "Gets or sets the padding between a trust boundary's edge and the members inside it.", + "format": "int32" + }, + "boundaryHeaderHeight": { + "pattern": "^-?(?:0|[1-9]\\d*)$", + "type": [ + "integer", + "string" + ], + "description": "Gets or sets the extra headroom reserved at the top of a trust boundary for its title,\nwhich is drawn inside the box and would otherwise print over the topmost member.", + "format": "int32" + }, + "labelCharacterWidth": { + "pattern": "^-?(?:0|[1-9]\\d*)$", + "type": [ + "integer", + "string" + ], + "description": "Gets or sets the width one character of a data-flow label occupies. Labels are drawn as a\nsingle unwrapped line, so this is what converts a flow's name into the space it needs.", + "format": "int32" + }, + "labelHeight": { + "pattern": "^-?(?:0|[1-9]\\d*)$", + "type": [ + "integer", + "string" + ], + "description": "Gets or sets the height of a data-flow label.", + "format": "int32" + }, + "labelLaneSpacing": { + "pattern": "^-?(?:0|[1-9]\\d*)$", + "type": [ + "integer", + "string" + ], + "description": "Gets or sets the perpendicular distance between adjacent label lanes.", + "format": "int32" + }, + "labelLanes": { + "pattern": "^-?(?:0|[1-9]\\d*)$", + "type": [ + "integer", + "string" + ], + "description": "Gets or sets how many lanes either side of a connector a label may be pushed into before\nthe least-covered position is accepted. Larger values clear more labels at the cost of\nbowing connectors further from a straight line.", + "format": "int32" + } + }, + "description": "Tunable spacing parameters for DiagramLayout. Defaults produce a readable\nlayered diagram; callers may widen the gaps for larger stencils." + }, + "LayoutRequestDto": { + "type": "object", + "properties": { + "model": { + "oneOf": [ + { + "type": "null" + }, + { + "$ref": "#/components/schemas/TmForgeModelDto" + } + ] + }, + "page": { + "type": [ + "null", + "string" + ] + }, + "options": { + "oneOf": [ + { + "type": "null" + }, + { + "$ref": "#/components/schemas/LayoutOptions" + } + ] + }, + "positions": { + "type": [ + "null", + "array" + ], + "items": { + "$ref": "#/components/schemas/LayoutElementDto" + } + } + } + }, + "LayoutResultDto": { + "type": "object", + "properties": { + "success": { + "type": "boolean" + }, + "error": { + "type": [ + "null", + "string" + ] + }, + "elements": { + "type": "array", + "items": { + "$ref": "#/components/schemas/LayoutElementDto" + } + }, + "pages": { + "pattern": "^-?(?:0|[1-9]\\d*)$", + "type": [ + "integer", + "string" + ], + "format": "int32" + }, + "components": { + "pattern": "^-?(?:0|[1-9]\\d*)$", + "type": [ + "integer", + "string" + ], + "format": "int32" + }, + "labelOverlaps": { + "pattern": "^-?(?:0|[1-9]\\d*)$", + "type": [ + "integer", + "string" + ], + "format": "int32" + } + } + }, "ManifestRequestDto": { "type": "object", "properties": { diff --git a/src/ThreatModelForge.Cli/LayoutCommand.cs b/src/ThreatModelForge.Cli/LayoutCommand.cs index a8802fd..5f49495 100644 --- a/src/ThreatModelForge.Cli/LayoutCommand.cs +++ b/src/ThreatModelForge.Cli/LayoutCommand.cs @@ -5,6 +5,8 @@ namespace ThreatModelForge.Cli using System.Globalization; using System.IO; using System.Linq; + using System.Text.Json; + using System.Text.Json.Nodes; using ThreatModelForge.Editing; using ThreatModelForge.Engine; using ThreatModelForge.Formats; @@ -63,14 +65,17 @@ public static int Run(string[] args) return 1; } - (ThreatModel model, IThreatModelFormat? format) = CliModelLoader.Load(input!); - if (format == null || (!check && !format.Capabilities.CanWrite)) + LayoutOptions options = new LayoutOptions(); + foreach (string name in new[] { "node-spacing", "layer-spacing" }) { - Console.Error.WriteLine("The model's format does not support writing."); - return 1; + string? value = parsed.Get(name); + if (value != null && (!TryGetInt(parsed, name, out int spacing) || spacing < 1 || spacing > 4096)) + { + Console.Error.WriteLine("--" + name + " must be an integer between 1 and 4096."); + return 1; + } } - LayoutOptions options = new LayoutOptions(); if (TryGetInt(parsed, "node-spacing", out int nodeSpacing)) { options.NodeSpacing = nodeSpacing; @@ -82,6 +87,18 @@ public static int Run(string[] args) } string? pageSpec = parsed.Get("page"); + if (!check && !labelsOnly && IsCanonicalJson(input!)) + { + return ArrangeJson(input!, pageSpec, options, parsed.Json); + } + + (ThreatModel model, IThreatModelFormat? format) = CliModelLoader.Load(input!); + if (format == null || (!check && !format.Capabilities.CanWrite)) + { + Console.Error.WriteLine("The model's format does not support writing."); + return 1; + } + List targets = new List(); if (string.IsNullOrEmpty(pageSpec)) { @@ -99,7 +116,14 @@ public static int Run(string[] args) int components = 0; int labelled = 0; + bool persistLabels = format is not TmForgeJsonFormat; List overlaps = new List(); + if (!check && !labelsOnly && !LayoutOperations.TryApply(targets, options, out labelled, out string? layoutError)) + { + Console.Error.WriteLine(layoutError); + return 1; + } + foreach (DrawingSurfaceModel diagram in targets) { components += diagram.Borders.Values.OfType().Count(element => !(element is BorderBoundary)); @@ -109,14 +133,10 @@ public static int Run(string[] args) continue; } - if (labelsOnly) + if (labelsOnly && persistLabels) { labelled += DiagramLabels.Deconflict(diagram, options); } - else - { - labelled += DiagramLayout.Apply(diagram, options); - } overlaps.AddRange(DiagramLabels.Inspect(diagram, options)); } @@ -126,7 +146,12 @@ public static int Run(string[] args) return Report(parsed, targets.Count, components, overlaps); } - AuthoringSupport.Save(model, input!, format); + // Canonical JSON has no persisted connector handles. Labels-only is a no-op there; + // writing through its format provider would discard unrelated author-owned metadata. + if (persistLabels) + { + AuthoringSupport.Save(model, input!, format); + } if (parsed.Json) { @@ -136,11 +161,14 @@ public static int Run(string[] args) components, labelsMoved = labelled, labelOverlaps = overlaps.Count, + labelsPersisted = persistLabels, }); } else { - Console.Error.WriteLine((labelsOnly + Console.Error.WriteLine(!persistLabels + ? "Canonical JSON does not store engine label positions. Use Studio's Labels only action; no file was changed." + : (labelsOnly ? "Placed " + labelled + " flow label(s)" : "Laid out " + components + " component(s)") + " across " + targets.Count + " page(s) in " + input + "."); @@ -150,6 +178,85 @@ public static int Run(string[] args) return 0; } + private static bool IsCanonicalJson(string path) + { + IThreatModelFormat? format = ThreatModelFormatRegistry.CreateDefault().FindByExtension(path); + if (format != null) + { + return format is TmForgeJsonFormat; + } + + using FileStream stream = File.OpenRead(path); + return new TmForgeJsonFormat().CanRead(stream); + } + + private static int ArrangeJson(string path, string? page, LayoutOptions options, bool json) + { + // Canonical JSON is already the author's document. A format write would re-key aliases, + // drop analysis settings and omit client view state. Patch only rectangles instead. + JsonNode document = JsonNode.Parse(File.ReadAllText(path), new JsonNodeOptions { PropertyNameCaseInsensitive = true }) + ?? throw new JsonException("A model document is required."); + LayoutResultDto result = EngineService.Layout(new LayoutRequestDto + { + Model = document.Deserialize(new JsonSerializerOptions { PropertyNameCaseInsensitive = true }), + Page = page, + Options = options, + }); + if (!result.Success) + { + Console.Error.WriteLine(result.Error); + return 1; + } + + Dictionary geometry = result.Elements.ToDictionary(element => element.Id, StringComparer.Ordinal); + void Update(JsonNode? node) + { + if (node?["elements"] is not JsonArray elements) + { + return; + } + + foreach (JsonNode? element in elements) + { + string? id = element?["id"]?.GetValue(); + if (id == null || !geometry.TryGetValue(id, out LayoutElementDto? placed)) + { + continue; + } + + element!["x"] = placed.X; + element["y"] = placed.Y; + element["width"] = placed.Width; + element["height"] = placed.Height; + } + } + + Update(document); + if (document["diagrams"] is JsonArray diagrams) + { + foreach (JsonNode? diagram in diagrams) + { + Update(diagram); + } + } + + AuthoringSupport.WriteAtomically(path, stream => + { + using Utf8JsonWriter writer = new Utf8JsonWriter(stream, new JsonWriterOptions { Indented = true }); + document.WriteTo(writer); + }); + if (json) + { + CliJson.WriteEnvelope("layout", new { pages = result.Pages, components = result.Components, labelsMoved = 0, labelOverlaps = result.LabelOverlaps, labelsPersisted = false }); + } + else + { + Console.Error.WriteLine($"Laid out {result.Components} component(s) across {result.Pages} page(s) in {path}. JSON retains its author-owned state; labels are rendered by the consuming host."); + } + + return 0; + } + /// /// Reports the label collisions found by --check without writing the model, and fails /// when any remain so a publishing gate can depend on the diagram being legible. @@ -232,6 +339,7 @@ private static void PrintUsage() Console.Error.WriteLine(); Console.Error.WriteLine("Arranges components by their data flows so you need not hand-place coordinates."); Console.Error.WriteLine("Components keep the trust boundary they were in; boundaries are resized around them."); + Console.Error.WriteLine("Refuses layout without writing if any page's boundary memberships or flow crossings would change."); Console.Error.WriteLine(" --labels Place only the flow labels, leaving hand-placed shapes exactly where they are."); Console.Error.WriteLine(" --check Report obstructed flow labels without writing; exits 1 when any remain."); } diff --git a/src/ThreatModelForge.Editing/DiagramGeometry.cs b/src/ThreatModelForge.Editing/DiagramGeometry.cs index d508711..b59e80f 100644 --- a/src/ThreatModelForge.Editing/DiagramGeometry.cs +++ b/src/ThreatModelForge.Editing/DiagramGeometry.cs @@ -10,6 +10,38 @@ namespace ThreatModelForge.Editing /// public static class DiagramGeometry { + /// Reattaches component-to-component flows to their facing edges after placement. + /// The diagram whose component rectangles were placed. + public static void RerouteConnectors(DrawingSurfaceModel diagram) + { + if (diagram == null) + { + throw new ArgumentNullException(nameof(diagram)); + } + + foreach (Connector connector in diagram.Lines.Values.OfType()) + { + if (!diagram.Borders.TryGetValue(connector.SourceGuid, out object? source) + || source is not DrawingElement || source is BorderBoundary + || !diagram.Borders.TryGetValue(connector.TargetGuid, out object? target) + || target is not DrawingElement || target is BorderBoundary) + { + continue; + } + + (int sourceCenterX, int sourceCenterY) = CenterOf(diagram, connector.SourceGuid); + (int targetCenterX, int targetCenterY) = CenterOf(diagram, connector.TargetGuid); + (int sourceX, int sourceY) = EdgePoint(diagram, connector.SourceGuid, targetCenterX, targetCenterY); + (int targetX, int targetY) = EdgePoint(diagram, connector.TargetGuid, sourceCenterX, sourceCenterY); + connector.SourceX = sourceX; + connector.SourceY = sourceY; + connector.TargetX = targetX; + connector.TargetY = targetY; + connector.HandleX = (sourceX + targetX) / 2; + connector.HandleY = (sourceY + targetY) / 2; + } + } + /// /// Computes the bounding box that encloses every element in a diagram. Returns a default /// 100x100 box when the diagram is empty. diff --git a/src/ThreatModelForge.Editing/DiagramLayout.cs b/src/ThreatModelForge.Editing/DiagramLayout.cs index b2aa479..3b60045 100644 --- a/src/ThreatModelForge.Editing/DiagramLayout.cs +++ b/src/ThreatModelForge.Editing/DiagramLayout.cs @@ -25,9 +25,10 @@ namespace ThreatModelForge.Editing /// /// One case cannot be preserved: a component sitting in the intersection of two boundaries that /// merely overlap. Nesting is representable and is kept, but a partial overlap is not, so such a - /// component keeps only the innermost boundary and the arrangement drops the other claim on it. - /// Overlapping boundaries are a modelling defect rather than a drawing one — they assert two - /// unrelated trust claims over one shape — so resolve them in the model before laying it out. + /// component keeps only the innermost boundary in this coarse placement candidate. Hosts must + /// validate all memberships and actual connector crossings before committing the candidate; + /// the engine's shared layout operation does so atomically. Overlapping trust claims may be + /// intentional and must never be discarded just to improve a drawing. /// /// public static class DiagramLayout @@ -63,7 +64,7 @@ public static int Apply(DrawingSurfaceModel diagram, LayoutOptions? options = nu Measure(root, edges, effectiveOptions, effectiveOptions.MaxWidth); Place(root, effectiveOptions.OriginX, effectiveOptions.OriginY, effectiveOptions); - RerouteConnectors(diagram, componentGuids); + DiagramGeometry.RerouteConnectors(diagram); return DiagramLabels.Deconflict(diagram, effectiveOptions); } @@ -333,29 +334,6 @@ private static void Place(Group group, int x, int y, LayoutOptions options) } } - private static void RerouteConnectors(DrawingSurfaceModel diagram, HashSet componentGuids) - { - foreach (Connector connector in diagram.Lines.Values.OfType()) - { - if (!componentGuids.Contains(connector.SourceGuid) || !componentGuids.Contains(connector.TargetGuid)) - { - continue; - } - - (int sourceCenterX, int sourceCenterY) = DiagramGeometry.CenterOf(diagram, connector.SourceGuid); - (int targetCenterX, int targetCenterY) = DiagramGeometry.CenterOf(diagram, connector.TargetGuid); - (int sourceX, int sourceY) = DiagramGeometry.EdgePoint(diagram, connector.SourceGuid, targetCenterX, targetCenterY); - (int targetX, int targetY) = DiagramGeometry.EdgePoint(diagram, connector.TargetGuid, sourceCenterX, sourceCenterY); - - connector.SourceX = sourceX; - connector.SourceY = sourceY; - connector.TargetX = targetX; - connector.TargetY = targetY; - connector.HandleX = (sourceX + targetX) / 2; - connector.HandleY = (sourceY + targetY) / 2; - } - } - private static long Area(DrawingElement element) => (long)element.Width * element.Height; private static bool Contains(DrawingElement outer, int x, int y) diff --git a/src/ThreatModelForge.Engine/Authoring/AuthoringOperations.cs b/src/ThreatModelForge.Engine/Authoring/AuthoringOperations.cs index c4e1174..388afdf 100644 --- a/src/ThreatModelForge.Engine/Authoring/AuthoringOperations.cs +++ b/src/ThreatModelForge.Engine/Authoring/AuthoringOperations.cs @@ -127,6 +127,14 @@ public static bool Add(ThreatModel model, AddRequest request, out Guid id, out I : (string.IsNullOrWhiteSpace(boundaryName) ? request.Boundary! : boundaryName); int memberIndex = AuthoringSupport.CountBoundaryMembers(diagram, membershipKey); (int insideLeft, int insideTop) = AuthoringSupport.PositionInsideBoundary(boundaryBox, memberIndex); + if ((long)insideLeft + placedComponent.Width >= (long)boundaryBox.Left + boundaryBox.Width + || (long)insideTop + placedComponent.Height >= (long)boundaryBox.Top + boundaryBox.Height) + { + editor.RemoveElement(diagram, id); + error = "The component does not fit inside boundary '" + membershipKey + "'. Enlarge the boundary explicitly before adding this member."; + return false; + } + editor.ResizeElement(diagram, id, insideLeft, insideTop, placedComponent.Width, placedComponent.Height); DiagramElementHelper.SetCustomProperty(added, AuthoringSupport.BoundaryPropertyName, membershipKey); } diff --git a/src/ThreatModelForge.Engine/Authoring/AuthoringSupport.cs b/src/ThreatModelForge.Engine/Authoring/AuthoringSupport.cs index f80db45..0087e4b 100644 --- a/src/ThreatModelForge.Engine/Authoring/AuthoringSupport.cs +++ b/src/ThreatModelForge.Engine/Authoring/AuthoringSupport.cs @@ -623,6 +623,19 @@ public static void Save(ThreatModel model, string path, IThreatModelFormat forma } } + WriteAtomically(path, stream => format.Write(model, stream)); + } + + /// Writes document content via a same-directory temporary file and atomic rename. + /// The destination path. + /// The content writer, called before the destination is replaced. + public static void WriteAtomically(string path, Action write) + { + if (write == null) + { + throw new ArgumentNullException(nameof(write)); + } + string fullPath = Path.GetFullPath(path); string directory = Path.GetDirectoryName(fullPath) ?? "."; string temp = Path.Join(directory, "." + Path.GetFileName(fullPath) + "." + Guid.NewGuid().ToString("N") + ".tmp"); @@ -630,7 +643,7 @@ public static void Save(ThreatModel model, string path, IThreatModelFormat forma { using (FileStream stream = File.Create(temp)) { - format.Write(model, stream); + write(stream); } File.Move(temp, fullPath, overwrite: true); diff --git a/src/ThreatModelForge.Engine/Authoring/LayoutElementDto.cs b/src/ThreatModelForge.Engine/Authoring/LayoutElementDto.cs new file mode 100644 index 0000000..aa6905b --- /dev/null +++ b/src/ThreatModelForge.Engine/Authoring/LayoutElementDto.cs @@ -0,0 +1,21 @@ +namespace ThreatModelForge.Engine +{ + /// A geometry-only update, keyed by the original author-assigned element identity. + public sealed class LayoutElementDto + { + /// Gets the unchanged author-assigned element id. + public string Id { get; init; } = string.Empty; + + /// Gets the arranged left coordinate. + public int X { get; init; } + + /// Gets the arranged top coordinate. + public int Y { get; init; } + + /// Gets the arranged width. + public int Width { get; init; } + + /// Gets the arranged height. + public int Height { get; init; } + } +} diff --git a/src/ThreatModelForge.Engine/Authoring/LayoutOperations.cs b/src/ThreatModelForge.Engine/Authoring/LayoutOperations.cs new file mode 100644 index 0000000..dfa33f1 --- /dev/null +++ b/src/ThreatModelForge.Engine/Authoring/LayoutOperations.cs @@ -0,0 +1,310 @@ +namespace ThreatModelForge.Engine +{ + using System; + using System.Collections.Generic; + using System.Linq; + using ThreatModelForge.Analysis; + using ThreatModelForge.Editing; + using ThreatModelForge.Model; + using ThreatModelForge.Model.Abstracts; + + /// + /// Arranges geometry on a candidate, checks it against the analyzer's actual trust-boundary + /// semantics, and commits only when every requested page is safe. No rules are evaluated and no + /// identities, properties, or threat-register entries are rewritten. + /// + public static class LayoutOperations + { + /// The maximum number of shapes in one arrangement request. + public const int MaximumElements = 512; + + /// The maximum number of lines in one arrangement request. + public const int MaximumLines = 1024; + + /// The maximum number of pages in one arrangement request. + public const int MaximumPages = 32; + + private const int MaximumCoordinate = 1000000; + private const int MaximumSize = 100000; + private const long MaximumWork = 25000000; + + /// + /// Tries to arrange all supplied pages atomically, or validates the supplied cleanup + /// positions without rearranging them. A refused candidate leaves every page untouched. + /// + /// The pages to arrange. + /// Spacing and label metrics, or the defaults. + /// The number of labels moved on success. + /// The reason the arrangement was refused. + /// Optional complete proposed placement to validate instead of generating a layout. + /// Whether every page could be arranged without changing its meaning. + public static bool TryApply( + IReadOnlyList diagrams, + LayoutOptions? options, + out int labelsMoved, + out string? error, + IReadOnlyDictionary? positions = null) + { + if (diagrams == null) + { + throw new ArgumentNullException(nameof(diagrams)); + } + + labelsMoved = 0; + LayoutOptions effective = options ?? new LayoutOptions(); + error = Validate(diagrams, effective); + if (error != null) + { + return false; + } + + if (positions != null && (positions.Count != diagrams.Sum(diagram => diagram.Borders.Count) + || diagrams.SelectMany(diagram => diagram.Borders.Keys).Any(id => !positions.ContainsKey(id)))) + { + error = "Proposed positions must name every selected element exactly once."; + return false; + } + + List candidates = new List(); + int moved = 0; + foreach (DrawingSurfaceModel diagram in diagrams) + { + DrawingSurfaceModel candidate = CopyGeometry(diagram); + if (positions == null) + { + moved += DiagramLayout.Apply(candidate, effective); + } + else + { + foreach (DrawingElement element in candidate.Borders.Values.OfType()) + { + LayoutElementDto position = positions[element.Guid]; + element.Left = position.X; + element.Top = position.Y; + element.Width = position.Width; + element.Height = position.Height; + } + + error = Validate(new[] { candidate }, effective); + if (error != null) + { + return false; + } + + DiagramGeometry.RerouteConnectors(candidate); + } + + error = Validate(new[] { candidate }, effective) ?? FindSemanticChange(diagram, candidate); + if (error != null) + { + error += " No pages were changed. Keep the geometry and use label-only cleanup, or resolve the boundary placement explicitly."; + return false; + } + + candidates.Add(candidate); + } + + for (int index = 0; index < diagrams.Count; index++) + { + CommitGeometry(candidates[index], diagrams[index]); + } + + labelsMoved = moved; + return true; + } + + private static string? Validate( + IReadOnlyList diagrams, + LayoutOptions options) + { + if (diagrams.Count > MaximumPages + || diagrams.Sum(diagram => (long)diagram.Borders.Count) > MaximumElements + || diagrams.Sum(diagram => (long)diagram.Lines.Count) > MaximumLines) + { + return $"Arrangement is limited to {MaximumPages} pages, {MaximumElements} shapes and {MaximumLines} lines per request."; + } + + if (!Within(options.OriginX, -MaximumCoordinate, MaximumCoordinate) + || !Within(options.OriginY, -MaximumCoordinate, MaximumCoordinate) + || !Within(options.NodeSpacing, 1, 4096) + || !Within(options.LayerSpacing, 1, 4096) + || !Within(options.BoundaryPadding, 1, 4096) + || !Within(options.BoundaryHeaderHeight, 1, 4096) + || !Within(options.MaxWidth, 120, MaximumSize) + || !Within(options.LabelCharacterWidth, 1, 64) + || !Within(options.LabelHeight, 1, 512) + || !Within(options.LabelLaneSpacing, 1, 4096) + || !Within(options.LabelLanes, 0, 3)) + { + return "Invalid arrangement metrics: spacing, boundary padding and header height must be between 1 and 4096; all other metrics must be within their documented limits."; + } + + HashSet ids = new HashSet(); + long work = 0; + foreach (DrawingSurfaceModel diagram in diagrams) + { + // Bound the quadratic label search as well as the graph size. Its candidate count + // is at most seven positions in each of seven lanes (DiagramLabels). + long lines = diagram.Lines.Count; + long shapes = diagram.Borders.Count; + work += (49 * lines * (lines + shapes)) + (shapes * shapes) + (shapes * lines); + if (work > MaximumWork) + { + return "Arrangement exceeds the layout work budget. Split the diagram into smaller pages."; + } + + foreach (KeyValuePair entry in diagram.Borders) + { + if (entry.Value is not DrawingElement element || entry.Key != element.Guid || !ids.Add(element.Guid)) + { + return "Arrangement requires uniquely identified drawing elements."; + } + + if (!Within(element.Left, -MaximumCoordinate, MaximumCoordinate) + || !Within(element.Top, -MaximumCoordinate, MaximumCoordinate) + || !Within(element.Width, 1, MaximumSize) + || !Within(element.Height, 1, MaximumSize) + || DiagramElementHelper.GetName(element).Length > 4096) + { + return "Arrangement requires positive shape sizes up to 100000, coordinates within +/-1000000 and names of at most 4096 characters."; + } + } + + foreach (KeyValuePair entry in diagram.Lines) + { + if (entry.Value is not LineElement line + || (line is not Connector && line is not LineBoundary) + || entry.Key != line.Guid || !ids.Add(line.Guid)) + { + return "Arrangement requires uniquely identified connectors or trust-boundary lines."; + } + + if (!Within(line.SourceX, -MaximumCoordinate, MaximumCoordinate) + || !Within(line.SourceY, -MaximumCoordinate, MaximumCoordinate) + || !Within(line.TargetX, -MaximumCoordinate, MaximumCoordinate) + || !Within(line.TargetY, -MaximumCoordinate, MaximumCoordinate) + || DiagramElementHelper.GetName(line).Length > 4096) + { + return "Arrangement requires line endpoints within +/-1000000 and names of at most 4096 characters."; + } + + if (line is Connector && (!diagram.Borders.ContainsKey(line.SourceGuid) || !diagram.Borders.ContainsKey(line.TargetGuid))) + { + return "Arrangement requires both endpoints of every flow to exist on its page."; + } + + if (line is Connector && (diagram.Borders[line.SourceGuid] is BorderBoundary || diagram.Borders[line.TargetGuid] is BorderBoundary)) + { + return "Arrangement requires flows to connect components, not trust boundaries. Use label-only cleanup for this page."; + } + } + } + + return null; + } + + private static bool Within(int value, int minimum, int maximum) => value >= minimum && value <= maximum; + + private static DrawingSurfaceModel CopyGeometry(DrawingSurfaceModel source) + { + DrawingSurfaceModel copy = new DrawingSurfaceModel { Guid = source.Guid, Header = source.Header }; + foreach (DrawingElement original in source.Borders.Values.OfType()) + { + // Layout uses bounding rectangles, not stencil rendering. These are geometry-only + // proxies: the originals' types and properties are never replaced on commit. + DrawingElement element = original is BorderBoundary ? new BorderBoundary() : new StencilRectangle(); + element.Guid = original.Guid; + element.Left = original.Left; + element.Top = original.Top; + element.Width = original.Width; + element.Height = original.Height; + DiagramElementHelper.SetName(element, DiagramElementHelper.GetName(original)); + copy.Borders.Add(element.Guid, element); + } + + foreach (LineElement original in source.Lines.Values.OfType()) + { + LineElement line = original is LineBoundary ? new LineBoundary() : new Connector(); + line.Guid = original.Guid; + line.SourceGuid = original.SourceGuid; + line.TargetGuid = original.TargetGuid; + line.SourceX = original.SourceX; + line.SourceY = original.SourceY; + line.TargetX = original.TargetX; + line.TargetY = original.TargetY; + line.HandleX = original.HandleX; + line.HandleY = original.HandleY; + DiagramElementHelper.SetName(line, DiagramElementHelper.GetName(original)); + copy.Lines.Add(line.Guid, line); + } + + return copy; + } + + private static string? FindSemanticChange(DrawingSurfaceModel before, DrawingSurfaceModel after) + { + foreach (BorderBoundary boundary in before.Borders.Values.OfType()) + { + BorderBoundary placedBoundary = (BorderBoundary)after.Borders[boundary.Guid]; + foreach (DrawingElement element in before.Borders.Values.OfType().Where(element => element is not BorderBoundary)) + { + DrawingElement placed = (DrawingElement)after.Borders[element.Guid]; + bool wasMember = boundary.Contains(element.Left + (element.Width / 2), element.Top + (element.Height / 2)); + bool isMember = placedBoundary.Contains(placed.Left + (placed.Width / 2), placed.Top + (placed.Height / 2)); + if (wasMember != isMember) + { + return $"Layout would change boundary membership of '{DiagramElementHelper.GetName(element)}' ({element.Guid}) in '{DiagramElementHelper.GetName(boundary)}' on page '{before.Header}'."; + } + } + + foreach (Connector flow in before.Lines.Values.OfType()) + { + Connector placed = (Connector)after.Lines[flow.Guid]; + if (boundary.Contains(flow.SourceX, flow.SourceY) != placedBoundary.Contains(placed.SourceX, placed.SourceY) + || boundary.Contains(flow.TargetX, flow.TargetY) != placedBoundary.Contains(placed.TargetX, placed.TargetY)) + { + return $"Layout would change the boundary side of an endpoint of flow '{DiagramElementHelper.GetName(flow)}' ({flow.Guid}) on page '{before.Header}'."; + } + } + } + + // Reuse the analysis implementation, including line-boundary intersections. Comparing + // component centers alone misses both detached connector endpoints and boundary lines. + ThreatModel original = new ThreatModel(); + original.DrawingSurfaceList.Add(before); + ThreatModel candidate = new ThreatModel(); + candidate.DrawingSurfaceList.Add(after); + CrossingDifference crossings = BoundaryCrossingDiff.Compare(original, candidate); + if (!crossings.IsEmpty) + { + CrossingChange changed = crossings.Changes[0]; + return $"Layout would change trust-boundary crossings of flow '{changed.FlowName}' ({changed.FlowId}) on page '{before.Header}'."; + } + + return null; + } + + private static void CommitGeometry(DrawingSurfaceModel source, DrawingSurfaceModel target) + { + foreach (DrawingElement placed in source.Borders.Values.OfType()) + { + DrawingElement original = (DrawingElement)target.Borders[placed.Guid]; + original.Left = placed.Left; + original.Top = placed.Top; + original.Width = placed.Width; + original.Height = placed.Height; + } + + foreach (Connector placed in source.Lines.Values.OfType()) + { + Connector original = (Connector)target.Lines[placed.Guid]; + original.SourceX = placed.SourceX; + original.SourceY = placed.SourceY; + original.TargetX = placed.TargetX; + original.TargetY = placed.TargetY; + original.HandleX = placed.HandleX; + original.HandleY = placed.HandleY; + } + } + } +} diff --git a/src/ThreatModelForge.Engine/Authoring/LayoutRequestDto.cs b/src/ThreatModelForge.Engine/Authoring/LayoutRequestDto.cs new file mode 100644 index 0000000..7c21043 --- /dev/null +++ b/src/ThreatModelForge.Engine/Authoring/LayoutRequestDto.cs @@ -0,0 +1,24 @@ +namespace ThreatModelForge.Engine +{ + using System.Collections.Generic; + using ThreatModelForge.Editing; + + /// Inputs to a semantic-preserving arrangement of all pages in a model. + public sealed class LayoutRequestDto + { + /// Gets the original model, before any client-side resizing or placement. + public TmForgeModelDto? Model { get; init; } + + /// Gets an optional page name or one-based index. Omitted means every page. + public string? Page { get; init; } + + /// Gets optional spacing and label metrics. Omitted metrics use engine defaults. + public LayoutOptions? Options { get; init; } + + /// + /// Gets optional proposed rectangles for every element on the selected pages. When present, + /// validates this placement without rearranging it. + /// + public IReadOnlyList? Positions { get; init; } + } +} diff --git a/src/ThreatModelForge.Engine/Authoring/LayoutResultDto.cs b/src/ThreatModelForge.Engine/Authoring/LayoutResultDto.cs new file mode 100644 index 0000000..85720cb --- /dev/null +++ b/src/ThreatModelForge.Engine/Authoring/LayoutResultDto.cs @@ -0,0 +1,30 @@ +namespace ThreatModelForge.Engine +{ + using System; + using System.Collections.Generic; + + /// + /// The result of a guarded arrangement. A refusal contains no geometry to apply. On success only + /// rectangles are returned, so a client retains its own topology, pages, properties and triage. + /// + public sealed class LayoutResultDto + { + /// Gets whether all pages passed membership and crossing-preservation checks. + public bool Success { get; init; } + + /// Gets the reason the arrangement was refused. + public string? Error { get; init; } + + /// Gets every arranged rectangle, or an empty list on refusal. + public IReadOnlyList Elements { get; init; } = Array.Empty(); + + /// Gets the number of pages arranged. + public int Pages { get; init; } + + /// Gets the number of components arranged, excluding trust boundaries. + public int Components { get; init; } + + /// Gets the remaining label overlaps under the engine's label metrics. + public int LabelOverlaps { get; init; } + } +} diff --git a/src/ThreatModelForge.Engine/EngineService.cs b/src/ThreatModelForge.Engine/EngineService.cs index 284db8b..4c3fedd 100644 --- a/src/ThreatModelForge.Engine/EngineService.cs +++ b/src/ThreatModelForge.Engine/EngineService.cs @@ -685,6 +685,225 @@ public static MergeResultDto Merge(TmForgeModelDto? baseModel, TmForgeModelDto o return new MergeResultDto { Merged = ToDto(result.Merged), Conflicts = conflicts }; } + /// + /// Arranges a model's pages without changing trust-boundary membership or connector + /// crossings. Returns only geometry keyed by original author ids; never rewrites author + /// state or evaluates detection rules. Unsafe candidates are refused without partial output. + /// + /// The original model, optional proposed positions and layout metrics. + /// Geometry updates on success, or a reason with no updates on refusal. + public static LayoutResultDto Layout(LayoutRequestDto request) + { + if (request == null) + { + throw new ArgumentNullException(nameof(request)); + } + + string? error = ValidateLayoutInput(request); + if (error != null) + { + return new LayoutResultDto { Error = error }; + } + + // Properties and triage cannot influence placement. Do not build them only to discard + // them: the canonical reader types properties and hydrates the register, which can be + // expensive for untrusted property bags and is unrelated to a geometry-only operation. + ThreatModel model = BuildModel(LayoutStructure(request.Model!), out _, out Dictionary originalIds); + IReadOnlyList targets = model.DrawingSurfaceList; + if (!string.IsNullOrEmpty(request.Page)) + { + if (!AuthoringSupport.TryResolveDiagram(model, request.Page, out DrawingSurfaceModel? selected, out error)) + { + return new LayoutResultDto { Error = error }; + } + + targets = new[] { selected! }; + } + + Dictionary internalIds = targets + .SelectMany(diagram => diagram.Borders.Values.OfType()) + .ToDictionary(element => originalIds[element.Guid], element => element.Guid, StringComparer.Ordinal); + Dictionary? positions = null; + if (request.Positions != null) + { + positions = new Dictionary(); + foreach (LayoutElementDto position in request.Positions) + { + if (!internalIds.TryGetValue(position.Id, out Guid id) || !positions.TryAdd(id, position)) + { + return new LayoutResultDto { Error = "Proposed positions must name each selected element exactly once." }; + } + } + } + + if (!LayoutOperations.TryApply(targets, request.Options, out _, out error, positions)) + { + return new LayoutResultDto { Error = error }; + } + + List elements = targets + .SelectMany(diagram => diagram.Borders.Values.OfType()) + .Select(element => new LayoutElementDto + { + Id = originalIds[element.Guid], + X = element.Left, + Y = element.Top, + Width = element.Width, + Height = element.Height, + }) + .ToList(); + return new LayoutResultDto + { + Success = true, + Elements = elements, + Pages = targets.Count, + Components = targets.Sum(diagram => diagram.Borders.Values.OfType().Count(element => element is not BorderBoundary)), + LabelOverlaps = targets.Sum(diagram => DiagramLabels.Inspect(diagram, request.Options).Count), + }; + } + + private static TmForgeModelDto LayoutStructure(TmForgeModelDto model) + { + IReadOnlyList? Elements(IReadOnlyList? elements) => elements?.Select(element => new TmForgeElementDto + { + Id = element.Id, Kind = element.Kind, Name = element.Name, + X = element.X, Y = element.Y, Width = element.Width, Height = element.Height, + }).ToArray(); + IReadOnlyList? Flows(IReadOnlyList? flows) => flows?.Select(flow => new TmForgeFlowDto + { + Id = flow.Id, Source = flow.Source, Target = flow.Target, Name = flow.Name, + }).ToArray(); + bool paged = model.Diagrams is { Count: > 0 }; + return new TmForgeModelDto + { + Elements = paged ? null : Elements(model.Elements), + Flows = paged ? null : Flows(model.Flows), + Diagrams = paged ? model.Diagrams!.Select(page => new TmForgeDiagramDto + { + Id = page.Id, Name = page.Name, Elements = Elements(page.Elements), Flows = Flows(page.Flows), + }).ToArray() : null, + }; + } + + private static string? ValidateLayoutInput(LayoutRequestDto request) + { + if (request.Model == null) + { + return "A model is required for arrangement."; + } + + TmForgeModelDto model = request.Model; + IReadOnlyList pages = model.Diagrams is { Count: > 0 } + ? model.Diagrams + : new[] { new TmForgeDiagramDto { Elements = model.Elements, Flows = model.Flows } }; + if (pages.Count > LayoutOperations.MaximumPages + || pages.Sum(page => (long)(page?.Elements?.Count ?? 0)) > LayoutOperations.MaximumElements + || pages.Sum(page => (long)(page?.Flows?.Count ?? 0)) > LayoutOperations.MaximumLines) + { + return $"Arrangement is limited to {LayoutOperations.MaximumPages} pages, {LayoutOperations.MaximumElements} shapes and {LayoutOperations.MaximumLines} lines per request."; + } + + HashSet ids = new HashSet(StringComparer.Ordinal); + HashSet pageIds = new HashSet(StringComparer.Ordinal); + HashSet elementKeys = new HashSet(); + HashSet pageKeys = new HashSet(); + foreach (TmForgeDiagramDto page in pages) + { + if (page == null || (model.Diagrams is { Count: > 0 } + && (string.IsNullOrWhiteSpace(page.Id) || page.Id.Length > 256 || page.Name?.Length > 4096 || !pageIds.Add(page.Id) + || !pageKeys.Add(Guid.TryParse(page.Id, out Guid pageId) ? pageId : DeterministicGuid.FromPageId(page.Id))))) + { + return "Arrangement requires unique non-empty page ids."; + } + + HashSet endpoints = new HashSet(StringComparer.Ordinal); + foreach (TmForgeElementDto element in page.Elements ?? Array.Empty()) + { + if (element == null || string.IsNullOrWhiteSpace(element.Id) || element.Id.Length > 256 || !ids.Add(element.Id) + || !elementKeys.Add(Guid.TryParse(element.Id, out Guid id) ? id : DeterministicGuid.FromElementId(element.Id))) + { + return "Arrangement requires unique non-empty element and flow ids."; + } + + if ((element.Kind != "process" && element.Kind != "external" && element.Kind != "datastore" && element.Kind != "boundary") + || element.Width.HasValue != element.Height.HasValue + || element.Width < 20 || element.Height < 20 || element.Name?.Length > 4096) + { + return "Arrangement requires supported element kinds and either omitted sizes or width and height of at least 20."; + } + + endpoints.Add(element.Id); + } + + foreach (TmForgeFlowDto flow in page.Flows ?? Array.Empty()) + { + if (flow == null) + { + return "Arrangement found a null flow entry. No pages were changed."; + } + + if (string.IsNullOrWhiteSpace(flow.Id) || flow.Id.Length > 256) + { + return "Arrangement requires a non-empty id of at most 256 characters for every flow. No pages were changed."; + } + + if (flow.Name?.Length > 4096) + { + return $"Flow '{flow.Id}' has a label longer than 4096 characters. No pages were changed."; + } + + string description = string.IsNullOrWhiteSpace(flow.Name) ? $"Flow '{flow.Id}'" : $"Flow '{flow.Name}' ({flow.Id})"; + if (!ids.Add(flow.Id)) + { + return description + " has a duplicate id shared with another element or flow. No pages were changed."; + } + + if (!elementKeys.Add(Guid.TryParse(flow.Id, out Guid id) ? id : DeterministicGuid.FromElementId(flow.Id))) + { + return description + " resolves to the same internal GUID as another element or flow. No pages were changed."; + } + + if (!endpoints.Contains(flow.Source)) + { + return LayoutEndpointError(description, "source", flow.Source); + } + + if (!endpoints.Contains(flow.Target)) + { + return LayoutEndpointError(description, "target", flow.Target); + } + } + } + + if (pageKeys.Overlaps(elementKeys)) + { + return "A page and an element cannot use the same internal GUID for arrangement."; + } + + if (request.Positions != null && (request.Positions.Count > LayoutOperations.MaximumElements + || request.Positions.Any(position => position == null || string.IsNullOrWhiteSpace(position.Id) + || position.Id.Length > 256 || position.Width < 20 || position.Height < 20))) + { + return "Proposed positions require valid element ids and sizes of at least 20."; + } + + return null; + } + + private static string LayoutEndpointError(string flow, string endpoint, string? id) + { + if (id?.Length > 256) + { + return flow + $" has a {endpoint} endpoint id longer than 256 characters. No pages were changed."; + } + + bool unattached = string.IsNullOrWhiteSpace(id) || (Guid.TryParse(id, out Guid parsed) && parsed == Guid.Empty); + string problem = unattached + ? $" has an unattached {endpoint} endpoint ('{id}')." + : $" references {endpoint} element '{id}', which is not present on this page."; + return flow + problem + " Reconnect that endpoint to an element before arranging. No pages were changed."; + } + /// /// The one place the effective rule set is evaluated for an analysis action. It builds the model /// once, loads the effective bundle once, evaluates every enabled rule once, and then projects diff --git a/src/ThreatModelForge.Studio/src/dfd/Editor.tsx b/src/ThreatModelForge.Studio/src/dfd/Editor.tsx index fd6c816..a37db8f 100644 --- a/src/ThreatModelForge.Studio/src/dfd/Editor.tsx +++ b/src/ThreatModelForge.Studio/src/dfd/Editor.tsx @@ -35,8 +35,8 @@ import { ThreatsPanel, type NewThreatDraft, type ThreatEdit, type ThreatScopeOpt import { CanvasSearch, type SearchItem } from './CanvasSearch'; import { ModelOutline } from './ModelOutline'; import { buildOutline, type OutlineOrder } from './outline'; -import { DEFAULT_NODE_SIZE, modelFromPages, pagesFromModel, type PageGraph } from './mapping'; -import { tidyGraph } from './autosize'; +import { DEFAULT_NODE_SIZE, modelFromPages, pagesFromModel, toModel, type PageGraph } from './mapping'; +import { applyLayoutGeometry, tidyGraph, tidyLabels } from './autosize'; import { cloneGraph, type Clipboard } from './clipboard'; import { useUndoRedo } from './useUndoRedo'; import { FlowEdge } from './edges/FlowEdge'; @@ -654,6 +654,12 @@ export function Editor() { () => JSON.stringify({ v: 2, activePageId, model: currentModel }), [activePageId, currentModel], ); + const layoutStateRef = useRef({ workspaceJson, nodes, edges }); + layoutStateRef.current = { workspaceJson, nodes, edges }; + const layoutRequestRef = useRef(0); + const layoutPendingRef = useRef(false); + const [tidying, setTidying] = useState(false); + useEffect(() => () => { layoutRequestRef.current += 1; }, []); useEffect(() => { const id = window.setTimeout(() => { try { @@ -1538,12 +1544,10 @@ export function Editor() { const loadModel = useCallback( (model: TmForgeModel) => { - // Auto-fit each page as it loads: grow shapes so a name never overruns its boundary, route - // flows through the ports that face their endpoints, and pull apart overlapping flow labels, so - // an imported (for example, CLI-authored) model is readable without manual clean-up. `grow` - // never shrinks a hand-tuned size, and labels already dragged aside are left as they are. + // Import never changes trust claims. Routing and label offsets are presentation-only; shape + // sizing and arrangement require an explicit Tidy action and the engine's preservation guard. const nextPages = pagesFromModel(model).map((p) => { - const tidied = tidyGraph(p.nodes, p.edges, 'grow'); + const tidied = tidyLabels(p.nodes, p.edges); return { ...p, nodes: tidied.nodes, edges: tidied.edges }; }); const nextPacks = model.analysis?.disabledPacks ?? []; @@ -1679,20 +1683,49 @@ export function Editor() { reset(); }, [setNodes, setEdges, reset]); - // Auto-layout the active page: fit every shape to its label (so a name can't overrun its boundary), - // route each flow through the ports that face its endpoints, and separate flow labels that overlap. - // One undo snapshot covers the whole tidy, then the view is re-framed. This is the on-demand form - // of the grow-only pass that runs when a model is loaded. - const tidyActivePage = useCallback(() => { - if (nodes.length === 0) { + // Preserve the author's arrangement and validate the cleanup before creating an undoable edit. + const tidyActivePage = useCallback(async (mode: 'tidy' | 'labels' = 'tidy') => { + if (nodes.length === 0 || layoutPendingRef.current) { return; } - takeSnapshot(); - const tidied = tidyGraph(nodes, edges, 'exact'); - setNodes(tidied.nodes); - setEdges(tidied.edges); - window.setTimeout(() => fitView({ padding: 0.25, maxZoom: 1.15, duration: 300 }), 0); - }, [nodes, edges, setNodes, setEdges, takeSnapshot, fitView]); + const version = ++layoutRequestRef.current; + const baseline = layoutStateRef.current.workspaceJson; + layoutPendingRef.current = true; + setTidying(true); + try { + let positioned = nodes; + if (mode !== 'labels') { + const original = toModel(nodes, edges); + const candidate = tidyGraph(nodes, edges, 'exact'); + const positions = toModel(candidate.nodes, candidate.edges).elements.map((element) => ({ + id: element.id, x: element.x, y: element.y, width: element.width!, height: element.height!, + })); + const geometry = await engine.layout(original, positions); + if (version !== layoutRequestRef.current) return; + if (layoutStateRef.current.workspaceJson !== baseline) { + toast('The model changed while tidying. The cleanup was not applied.', 'info'); + return; + } + positioned = applyLayoutGeometry(layoutStateRef.current.nodes, geometry); + } + const tidied = tidyLabels(positioned, layoutStateRef.current.edges); + if (tidied.nodes.every((node, index) => node === layoutStateRef.current.nodes[index]) + && tidied.edges.every((edge, index) => edge === layoutStateRef.current.edges[index])) return; + takeSnapshot(); + setNodes(tidied.nodes); + setEdges(tidied.edges); + window.setTimeout(() => fitView({ padding: 0.25, maxZoom: 1.15, duration: 300 }), 0); + } catch (err) { + if (version === layoutRequestRef.current) { + toast(err instanceof Error ? err.message : 'Could not tidy this page. Nothing was changed.', 'error'); + } + } finally { + if (version === layoutRequestRef.current) { + layoutPendingRef.current = false; + setTidying(false); + } + } + }, [engine, nodes, edges, setNodes, setEdges, takeSnapshot, fitView]); const actions = useMemo( () => ({ beginEdit: takeSnapshot, renameNode, renameEdge, setEdgeLabelOffset }), @@ -1727,6 +1760,7 @@ export function Editor() { onClear={clearAll} onFit={() => fitView({ padding: 0.25, maxZoom: 1.15, duration: 300 })} onTidy={tidyActivePage} + tidying={tidying} onUndo={undo} onRedo={redo} canUndo={canUndo} diff --git a/src/ThreatModelForge.Studio/src/dfd/EditorHarness.test.tsx b/src/ThreatModelForge.Studio/src/dfd/EditorHarness.test.tsx index 280bc4c..42414f7 100644 --- a/src/ThreatModelForge.Studio/src/dfd/EditorHarness.test.tsx +++ b/src/ThreatModelForge.Studio/src/dfd/EditorHarness.test.tsx @@ -3,6 +3,17 @@ import { describe, it, expect, beforeAll, beforeEach, vi } from 'vitest'; import { render, screen, fireEvent, waitFor, within, act } from '@testing-library/react'; import { ReactFlowProvider, useReactFlow, type ReactFlowInstance } from '@xyflow/react'; import { STORAGE_KEY } from './Editor'; +import type { IEngineClient, LayoutElement } from './engineClient'; + +const engineState = vi.hoisted(() => ({ current: undefined as IEngineClient | undefined })); +vi.mock('./engineClient', async (importOriginal) => { + const original = await importOriginal(); + return { + ...original, + probeEngine: async () => false, + loadWasmEngine: async () => engineState.current ?? null, + }; +}); /** * Drives the whole editor — React Flow canvas, Inspector, toolbar, page strip — the way a person @@ -182,6 +193,118 @@ function addCustomProperty(key: string, value: string): void { beforeEach(() => { window.localStorage.clear(); + engineState.current = undefined; +}); + +describe('Editor — guarded Tidy', () => { + const geometry: LayoutElement[] = ['a', 'b', 'c'].map((id, index) => ({ + id, x: 40 + index * 300, y: 100, width: 160, height: 96, + })); + + async function useLayout(layout: IEngineClient['layout']): Promise { + const { offlineEngine } = await import('./engineClient'); + engineState.current = Object.assign(Object.create(offlineEngine) as IEngineClient, { label: 'layout test engine', layout }); + } + + async function tidy(): Promise { + await waitFor(() => expect(document.querySelector('.engine-pill')).toHaveTextContent('layout test engine')); + fireEvent.click(screen.getByRole('button', { name: 'Tidy' })); + } + + it('applies a successful response with the original ids and exactly one undo step', async () => { + const layout = vi.fn(async () => geometry); + await useLayout(layout); + await mountEditor(chain()); + + await tidy(); + + await waitFor(() => expect(flow!.getNode('a')?.position).toEqual({ x: 40, y: 100 })); + expect(layout).toHaveBeenCalledTimes(1); + expect(canvasNodeIds()).toEqual(['a', 'b', 'c']); + expect(await undoToExhaustion()).toBe(1); + await waitFor(() => expect(flow!.getNode('a')?.position).toEqual({ x: 0, y: 0 })); + await expectPersisted({ elements: ['a', 'b', 'c'], flows: ['ab', 'bc'] }); + }); + + it('tidies the existing horizontal arrangement instead of replacing it with graph layers', async () => { + const layout = vi.fn(async (...args: unknown[]) => args[1] as LayoutElement[]); + await useLayout(layout); + await mountEditor(chain()); + await waitFor(() => expect(document.querySelector('.engine-pill')).toHaveTextContent('layout test engine')); + + fireEvent.click(screen.getByRole('button', { name: 'Tidy' })); + + await waitFor(() => expect(layout).toHaveBeenCalledTimes(1)); + const candidate = layout.mock.calls[0][1] as LayoutElement[]; + expect(candidate).toHaveLength(3); + for (const [index, id] of ['a', 'b', 'c'].entries()) { + const placed = candidate.find((element) => element.id === id)!; + expect(Math.abs(placed.x + placed.width / 2 - (index * 200 + 60))).toBeLessThanOrEqual(0.5); + expect(Math.abs(placed.y + placed.height / 2 - 30)).toBeLessThanOrEqual(0.5); + } + await waitFor(() => expect(flow!.getNode('a')?.position.x).toBe(candidate[0].x)); + expect(await undoToExhaustion()).toBe(1); + }); + + it('leaves geometry and undo history untouched after a refusal', async () => { + await useLayout(vi.fn(async () => { throw new Error('Unsafe crossing; no pages were changed.'); })); + await mountEditor(chain()); + + await tidy(); + + await waitFor(() => expect(screen.getByText(/Unsafe crossing/)).toBeInTheDocument()); + expect(flow!.getNode('a')?.position).toEqual({ x: 0, y: 0 }); + expect(undoButton()).toBeDisabled(); + }); + + it('discards a late response instead of overwriting a newer property edit', async () => { + let finish!: (elements: LayoutElement[]) => void; + const layout = vi.fn(() => new Promise((resolve) => { finish = resolve; })); + await useLayout(layout); + await mountEditor(chain()); + await tidy(); + await waitFor(() => expect(layout).toHaveBeenCalledTimes(1)); + expect(screen.getByRole('button', { name: /Tidying/ })).toBeDisabled(); + + selectNodes('a'); + addCustomProperty('Owner', 'newer-edit'); + await act(async () => { finish(geometry); }); + + await waitFor(() => expect(screen.getByText(/model changed while tidying/i)).toBeInTheDocument()); + expect(flow!.getNode('a')?.position).toEqual({ x: 0, y: 0 }); + expect(flow!.getNode('a')?.data.properties).toMatchObject({ Owner: 'newer-edit' }); + expect(await undoToExhaustion()).toBe(1); + }); + + it('does not apply a late response to a different active page', async () => { + let finish!: (elements: LayoutElement[]) => void; + const layout = vi.fn(() => new Promise((resolve) => { finish = resolve; })); + await useLayout(layout); + const first = chain(); + const second = seedModel([{ id: 'other', kind: 'process', name: 'Other', x: 900, y: 500, width: 120, height: 60 }], []); + const model = { ...first, diagrams: [{ ...first, id: 'one', name: 'First' }, { ...second, id: 'two', name: 'Second' }] }; + await mountEditor(model); + await tidy(); + await waitFor(() => expect(layout).toHaveBeenCalledTimes(1)); + + fireEvent.click(screen.getByRole('tab', { name: /^Second/ })); + await act(async () => { finish(geometry); }); + + await waitFor(() => expect(canvasNodeIds()).toEqual(['other'])); + expect(flow!.getNode('other')?.position).toEqual({ x: 900, y: 500 }); + expect(undoButton()).toBeDisabled(); + }); + + it('keeps labels-only cleanup available without invoking an offline layout engine', async () => { + await mountEditor(chain()); + expect(screen.getByRole('button', { name: 'Tidy' })).toBeDisabled(); + fireEvent.click(screen.getByRole('button', { name: 'Tidy options' })); + expect(screen.queryByRole('menuitem', { name: /Arrange/ })).not.toBeInTheDocument(); + fireEvent.click(screen.getByRole('menuitem', { name: /Labels only/ })); + + expect(flow!.getNode('a')?.position).toEqual({ x: 0, y: 0 }); + expect(flow!.getNode('b')?.position).toEqual({ x: 200, y: 0 }); + }); }); describe('Editor — deleting from the canvas', () => { diff --git a/src/ThreatModelForge.Studio/src/dfd/Toolbar.test.tsx b/src/ThreatModelForge.Studio/src/dfd/Toolbar.test.tsx index 6ebf3cd..f8487ea 100644 --- a/src/ThreatModelForge.Studio/src/dfd/Toolbar.test.tsx +++ b/src/ThreatModelForge.Studio/src/dfd/Toolbar.test.tsx @@ -3,7 +3,7 @@ import { render, screen, fireEvent } from '@testing-library/react'; import { Toolbar, REPORT_OPTIONS } from './Toolbar'; import { REPORT_DOWNLOADS } from './Editor'; -function renderToolbar(onReport: (id: string) => void) { +function renderToolbar(onReport: (id: string) => void, onTidy = vi.fn(), engineOnline = true, tidying = false) { render( void) { onUndo={vi.fn()} onRedo={vi.fn()} onFit={vi.fn()} - onTidy={vi.fn()} + onTidy={onTidy} + tidying={tidying} onToggleTheme={vi.fn()} canUndo={false} canRedo={false} dirty={false} fileName={null} exportFormats={[]} - engineOnline + engineOnline={engineOnline} engineLabel="test" theme="light" demo={false} @@ -90,3 +91,41 @@ describe('report menu', () => { expect(screen.queryAllByRole('menuitem')).toHaveLength(0); }); }); + +describe('Tidy menu', () => { + it('tidies the existing arrangement with one click', () => { + const onTidy = vi.fn(); + renderToolbar(vi.fn(), onTidy); + + fireEvent.click(screen.getByRole('button', { name: 'Tidy' })); + + expect(onTidy).toHaveBeenCalledWith('tidy'); + expect(screen.queryAllByRole('menuitem')).toHaveLength(0); + }); + + it('offers only labels-only cleanup in the secondary menu, not rearrangement', () => { + const onTidy = vi.fn(); + renderToolbar(vi.fn(), onTidy); + + fireEvent.click(screen.getByRole('button', { name: 'Tidy options' })); + expect(screen.getAllByRole('menuitem')).toHaveLength(1); + expect(screen.queryByRole('menuitem', { name: /Arrange/ })).not.toBeInTheDocument(); + fireEvent.click(screen.getByRole('menuitem', { name: /Labels only/ })); + expect(onTidy).toHaveBeenCalledWith('labels'); + }); + + it('keeps labels-only cleanup available offline without promising a safe arrangement', () => { + renderToolbar(vi.fn(), vi.fn(), false); + + expect(screen.getByRole('button', { name: 'Tidy' })).toBeDisabled(); + fireEvent.click(screen.getByRole('button', { name: 'Tidy options' })); + + expect(screen.getByRole('menuitem', { name: /Labels only/ })).toBeEnabled(); + }); + + it('disables repeated requests while tidying', () => { + renderToolbar(vi.fn(), vi.fn(), true, true); + + expect(screen.getByRole('button', { name: /Tidying/ })).toBeDisabled(); + }); +}); diff --git a/src/ThreatModelForge.Studio/src/dfd/Toolbar.tsx b/src/ThreatModelForge.Studio/src/dfd/Toolbar.tsx index fcda13c..d0f1320 100644 --- a/src/ThreatModelForge.Studio/src/dfd/Toolbar.tsx +++ b/src/ThreatModelForge.Studio/src/dfd/Toolbar.tsx @@ -23,11 +23,15 @@ function ToolbarMenu({ title, options, onSelect, + disabled = false, + ariaLabel, }: { label: string; title: string; options: MenuOption[]; onSelect: (id: string) => void; + disabled?: boolean; + ariaLabel?: string; }) { const [open, setOpen] = useState(false); const ref = useRef(null); @@ -58,8 +62,9 @@ function ToolbarMenu({
+ props.onTidy('labels')} + /> diff --git a/src/ThreatModelForge.Studio/src/dfd/autosize.test.ts b/src/ThreatModelForge.Studio/src/dfd/autosize.test.ts index c8fb2cd..f31f5e5 100644 --- a/src/ThreatModelForge.Studio/src/dfd/autosize.test.ts +++ b/src/ThreatModelForge.Studio/src/dfd/autosize.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect } from 'vitest'; import { + applyLayoutGeometry, boundaryTitleRect, deconflictEdgeLabels, edgeLabelBox, @@ -10,11 +11,72 @@ import { routeEdges, separateNodes, tidyGraph, + tidyLabels, wrapLabel, } from './autosize'; import { DEFAULT_NODE_SIZE } from './mapping'; import type { DfdEdge, DfdNode } from './types'; +describe('shared layout integration', () => { + function nodes(): DfdNode[] { + return [ + { id: 'tb', type: 'boundary', position: { x: 0, y: 0 }, width: 400, height: 250, data: { label: 'A long boundary title that will wrap on a narrow region' } }, + { id: 'p', type: 'process', position: { x: 50, y: 50 }, width: 100, height: 60, selected: true, data: { label: 'Gateway', properties: { Boundary: 'tb', Owner: 'security' } } }, + ]; + } + + it('applies only geometry and retains all author and selection state', () => { + const input = nodes(); + const placed = applyLayoutGeometry(input, [ + { id: 'tb', x: 40, y: 40, width: 240, height: 280 }, + { id: 'p', x: 64, y: 160, width: 160, height: 96 }, + ]); + + expect(placed[1].position).toEqual({ x: 64, y: 160 }); + expect(placed[1].data).toBe(input[1].data); + expect(placed[1].selected).toBe(true); + expect(input[1].position).toEqual({ x: 50, y: 50 }); + expect(placed[1].style).toEqual({ width: 160, height: 96 }); + }); + + it('refuses missing, duplicate and foreign rectangles instead of partially applying a response', () => { + const input = nodes(); + const patch = { id: 'p', x: 1, y: 2, width: 100, height: 60 }; + + expect(() => applyLayoutGeometry(input, [patch])).toThrow(/does not match/); + expect(() => applyLayoutGeometry(input, [patch, patch])).toThrow(/does not match/); + expect(() => applyLayoutGeometry(input, [patch, { ...patch, id: 'foreign' }])).toThrow(/does not match/); + expect(input).toEqual(nodes()); + }); + + it('labels-only cleanup leaves every rectangle untouched, including overlapping trust claims', () => { + const input = nodes(); + const edges: DfdEdge[] = [{ id: 'f', source: 'p', target: 'p', label: 'loop', data: { properties: { Protocol: 'TLS' } } }]; + const cleaned = tidyLabels(input, edges); + + expect(cleaned.nodes).toBe(input); + expect(cleaned.edges[0].source).toBe(edges[0].source); + expect(cleaned.edges[0].target).toBe(edges[0].target); + expect(cleaned.edges[0].data?.properties).toEqual(edges[0].data?.properties); + }); + + it('refuses cyclic declared boundaries without recursing or changing the input', () => { + const input = [ + { ...nodes()[0], id: 'a', data: { label: 'A', properties: { Boundary: 'b' } } }, + { ...nodes()[0], id: 'b', data: { label: 'B', properties: { Boundary: 'a' } } }, + ]; + const before = structuredClone(input); + + expect(() => tidyGraph(input, [])).toThrow(/cyclic boundary membership/); + expect(input).toEqual(before); + }); + + it('bounds cleanup before performing overlap searches or text measurement', () => { + expect(() => tidyGraph(Array.from({ length: 513 }, () => nodes()[1]), [])).toThrow(/512 shapes/); + expect(() => tidyGraph([{ ...nodes()[1], data: { label: 'x'.repeat(4097) } }], [])).toThrow(/4096 characters/); + }); +}); + describe('wrapLabel', () => { it('keeps a short name on a single line', () => { expect(wrapLabel('kube-apiserver', 150)).toEqual(['kube-apiserver']); diff --git a/src/ThreatModelForge.Studio/src/dfd/autosize.ts b/src/ThreatModelForge.Studio/src/dfd/autosize.ts index 54a1d9c..162cb8f 100644 --- a/src/ThreatModelForge.Studio/src/dfd/autosize.ts +++ b/src/ThreatModelForge.Studio/src/dfd/autosize.ts @@ -1,4 +1,5 @@ import { DEFAULT_NODE_SIZE } from './mapping'; +import type { LayoutElement } from './engineClient'; import type { DfdEdge, DfdKind, DfdNode } from './types'; /** @@ -237,6 +238,33 @@ export function fitNodeSize(kind: DfdKind, label: string, hasCaption = false): N }; } +/** Merge validated rectangles onto current nodes, retaining selection, properties, and view state. */ +export function applyLayoutGeometry(nodes: DfdNode[], elements: LayoutElement[]): DfdNode[] { + const byId = new Map(elements.map((element) => [element.id, element])); + if (elements.length !== nodes.length || byId.size !== elements.length || nodes.some((node) => !byId.has(node.id))) { + throw new Error('The layout response does not match the page. Nothing was changed.'); + } + return nodes.map((node) => { + const element = byId.get(node.id)!; + const current = rectOf(node); + if (current.x === element.x && current.y === element.y && current.w === element.width && current.h === element.height) { + return node; + } + return { + ...node, + position: { x: element.x, y: element.y }, + width: element.width, + height: element.height, + style: { ...node.style, width: element.width, height: element.height }, + }; + }); +} + +/** Presentation-only cleanup, safe on import and offline: no shape or boundary geometry changes. */ +export function tidyLabels(nodes: DfdNode[], edges: DfdEdge[]): { nodes: DfdNode[]; edges: DfdEdge[] } { + return { nodes, edges: deconflictEdgeLabels(nodes, routeEdges(nodes, edges)) }; +} + export type FitMode = 'grow' | 'exact'; /** @@ -828,6 +856,18 @@ export function separateNodes(nodes: DfdNode[]): DfdNode[] { parentOf.set(boundary.id, parent?.id ?? null); } + for (const boundary of boundaries) { + const visited = new Set(); + let current: string | null = boundary.id; + while (current !== null) { + if (visited.has(current)) { + throw new Error('Tidy cannot resolve cyclic boundary membership. No changes were made.'); + } + visited.add(current); + current = parentOf.get(current) ?? null; + } + } + const childrenOf = new Map(); for (const boundary of boundaries) { const parent = parentOf.get(boundary.id) ?? null; @@ -994,6 +1034,11 @@ export function tidyGraph( edges: DfdEdge[], mode: FitMode = 'exact', ): { nodes: DfdNode[]; edges: DfdEdge[] } { + if (nodes.length > 512 || edges.length > 1024 + || nodes.some((node) => node.data.label.length > 4096) + || edges.some((edge) => typeof edge.label === 'string' && edge.label.length > 4096)) { + throw new Error('Tidy supports up to 512 shapes, 1024 flows and labels of at most 4096 characters. No changes were made.'); + } const separated = separateNodes(resizeNodesToFit(nodes, mode)); const routed = routeEdges(separated, edges); return { nodes: separated, edges: deconflictEdgeLabels(separated, routed) }; diff --git a/src/ThreatModelForge.Studio/src/dfd/engine/schema.d.ts b/src/ThreatModelForge.Studio/src/dfd/engine/schema.d.ts index 3f4e1f5..aaeb709 100644 --- a/src/ThreatModelForge.Studio/src/dfd/engine/schema.d.ts +++ b/src/ThreatModelForge.Studio/src/dfd/engine/schema.d.ts @@ -228,6 +228,22 @@ export interface paths { patch?: never; trace?: never; }; + "/v1/model/layout": { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + get?: never; + put?: never; + post: operations["LayoutModel"]; + delete?: never; + options?: never; + head?: never; + patch?: never; + trace?: never; + }; "/v1/model/export/tm7": { parameters: { query?: never; @@ -412,6 +428,102 @@ export interface components { HealthStatusDto: { status?: string; }; + LayoutElementDto: { + id?: string; + /** Format: int32 */ + x?: number | string; + /** Format: int32 */ + y?: number | string; + /** Format: int32 */ + width?: number | string; + /** Format: int32 */ + height?: number | string; + }; + /** + * @description Tunable spacing parameters for DiagramLayout. Defaults produce a readable + * layered diagram; callers may widen the gaps for larger stencils. + */ + LayoutOptions: { + /** + * Format: int32 + * @description Gets or sets the x coordinate of the top-left origin of the laid-out region. + */ + originX?: number | string; + /** + * Format: int32 + * @description Gets or sets the y coordinate of the top-left origin of the laid-out region. + */ + originY?: number | string; + /** + * Format: int32 + * @description Gets or sets the horizontal gap between adjacent layers (columns). + */ + layerSpacing?: number | string; + /** + * Format: int32 + * @description Gets or sets the vertical gap between adjacent nodes within a layer. + */ + nodeSpacing?: number | string; + /** + * Format: int32 + * @description Gets or sets the width a row of columns may occupy before the next column wraps onto a new + * row. The Microsoft Threat Modeling Tool's drawing surface is bounded and taller than it is + * wide, so a wide model has to grow downwards; anything drawn past the right-hand limit is + * clamped by the tool on load, which would pile elements on top of each other. + */ + maxWidth?: number | string; + /** + * Format: int32 + * @description Gets or sets the padding between a trust boundary's edge and the members inside it. + */ + boundaryPadding?: number | string; + /** + * Format: int32 + * @description Gets or sets the extra headroom reserved at the top of a trust boundary for its title, + * which is drawn inside the box and would otherwise print over the topmost member. + */ + boundaryHeaderHeight?: number | string; + /** + * Format: int32 + * @description Gets or sets the width one character of a data-flow label occupies. Labels are drawn as a + * single unwrapped line, so this is what converts a flow's name into the space it needs. + */ + labelCharacterWidth?: number | string; + /** + * Format: int32 + * @description Gets or sets the height of a data-flow label. + */ + labelHeight?: number | string; + /** + * Format: int32 + * @description Gets or sets the perpendicular distance between adjacent label lanes. + */ + labelLaneSpacing?: number | string; + /** + * Format: int32 + * @description Gets or sets how many lanes either side of a connector a label may be pushed into before + * the least-covered position is accepted. Larger values clear more labels at the cost of + * bowing connectors further from a straight line. + */ + labelLanes?: number | string; + }; + LayoutRequestDto: { + model?: null | components["schemas"]["TmForgeModelDto"]; + page?: null | string; + options?: null | components["schemas"]["LayoutOptions"]; + positions?: null | components["schemas"]["LayoutElementDto"][]; + }; + LayoutResultDto: { + success?: boolean; + error?: null | string; + elements?: components["schemas"]["LayoutElementDto"][]; + /** Format: int32 */ + pages?: number | string; + /** Format: int32 */ + components?: number | string; + /** Format: int32 */ + labelOverlaps?: number | string; + }; ManifestRequestDto: { manifest?: null | string; force?: boolean; @@ -935,6 +1047,30 @@ export interface operations { }; }; }; + LayoutModel: { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + requestBody: { + content: { + "application/json": components["schemas"]["LayoutRequestDto"]; + }; + }; + responses: { + /** @description OK */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["LayoutResultDto"]; + }; + }; + }; + }; ExportModelTm7: { parameters: { query?: never; diff --git a/src/ThreatModelForge.Studio/src/dfd/engineClient.test.ts b/src/ThreatModelForge.Studio/src/dfd/engineClient.test.ts index a6314a5..2300c2a 100644 --- a/src/ThreatModelForge.Studio/src/dfd/engineClient.test.ts +++ b/src/ThreatModelForge.Studio/src/dfd/engineClient.test.ts @@ -1,5 +1,5 @@ -import { describe, it, expect } from 'vitest'; -import { looksLikeManifest, offlineEngine, toModel } from './engineClient'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { createHttpEngine, looksLikeManifest, offlineEngine, toModel, WasmEngineClient } from './engineClient'; import type { components } from './engine/schema'; import type { TmForgeModel } from './types'; @@ -56,6 +56,10 @@ describe('OfflineEngineClient — the honest fallback contract', () => { ); }); + it('refuses unvalidated offline arrangement while directing the user to labels-only cleanup', async () => { + await expect(offlineEngine.layout(emptyModel(), [])).rejects.toThrow(/requires the .NET engine.*Labels only/i); + }); + it('converts to tmforge-json client-side but rejects engine-only target formats', async () => { const blob = await offlineEngine.convert(emptyModel(), 'tmforge-json'); expect(blob).toBeInstanceOf(Blob); @@ -114,6 +118,22 @@ describe('looksLikeManifest — routing an unidentified document', () => { }); describe('engine model normalization', () => { + it('preserves every imported page and the expected rule fingerprints before layout', () => { + const dto: components['schemas']['TmForgeModelDto'] = { + diagrams: [ + { id: 'first', name: 'First', elements: [{ id: 'a', kind: 'process', x: 170, y: 130 }], flows: [] }, + { id: 'second', name: 'Second', elements: [{ id: 'b', kind: 'boundary', x: 280, y: 190, width: 700, height: 400 }], flows: [] }, + ], + analysis: { expectedPacks: [{ id: 'policy', fingerprint: 'sha256:unchanged' }] }, + }; + + const model = toModel(dto); + + expect(model.diagrams?.map((page) => page.id)).toEqual(['first', 'second']); + expect(model.diagrams?.[1].elements[0]).toMatchObject({ id: 'b', x: 280, y: 190, width: 700, height: 400 }); + expect(model.analysis?.expectedPacks).toEqual(dto.analysis?.expectedPacks); + }); + it('preserves accepted, priority-edited, and manual threat overlays returned by the engine', () => { const dto: components['schemas']['TmForgeModelDto'] = { threats: [ @@ -165,3 +185,58 @@ describe('engine model normalization', () => { ]); }); }); + +describe('layout transports', () => { + afterEach(() => vi.unstubAllGlobals()); + + const geometry = [{ id: 'author-id', x: 40, y: 64, width: 140, height: 100 }]; + + it('requests only validation of proposed geometry through HTTP and WASM, never automatic placement', async () => { + let httpBody: unknown; + vi.stubGlobal('fetch', vi.fn(async (request: Request) => { + expect(request.url).toBe('http://localhost/v1/model/layout'); + httpBody = await request.json(); + return new Response(JSON.stringify({ success: true, elements: geometry }), { headers: { 'Content-Type': 'application/json' } }); + })); + let wasmBody: unknown; + const wasm = new WasmEngineClient({ + Layout: (json: string) => { + wasmBody = JSON.parse(json); + return JSON.stringify({ success: true, elements: geometry }); + }, + } as unknown as ConstructorParameters[0]); + const model = emptyModel(); + expect(await createHttpEngine('http://localhost').layout(model, geometry)).toEqual(geometry); + expect(await wasm.layout(model, geometry)).toEqual(geometry); + expect(httpBody).toEqual({ model, positions: geometry }); + expect(wasmBody).toEqual(httpBody); + }); + + it.each([ + { success: false, error: 'Layout would change trust-boundary crossings.', elements: geometry }, + { success: true, elements: [{ id: 'a', x: 40, y: 40 }] }, + { success: true, elements: [{ id: 'a', x: 40, y: 40, width: -1, height: 50 }] }, + {}, + ])('rejects refused or incomplete layout results on both transports: %j', async (result) => { + vi.stubGlobal('fetch', vi.fn(async () => new Response(JSON.stringify(result), { headers: { 'Content-Type': 'application/json' } }))); + const wasm = new WasmEngineClient({ Layout: () => JSON.stringify(result) } as unknown as ConstructorParameters[0]); + + await expect(createHttpEngine('http://localhost').layout(emptyModel(), geometry)).rejects.toThrow(); + await expect(wasm.layout(emptyModel(), geometry)).rejects.toThrow(); + }); + + it('refuses an older engine that ignores Tidy positions and rearranges the model', async () => { + const rearranged = geometry.map((element) => ({ ...element, x: element.x + 100 })); + vi.stubGlobal('fetch', vi.fn(async () => new Response(JSON.stringify({ success: true, elements: rearranged }), { headers: { 'Content-Type': 'application/json' } }))); + const wasm = new WasmEngineClient({ Layout: () => JSON.stringify({ success: true, elements: rearranged }) } as unknown as ConstructorParameters[0]); + + await expect(createHttpEngine('http://localhost').layout(emptyModel(), geometry)).rejects.toThrow(/did not preserve/); + await expect(wasm.layout(emptyModel(), geometry)).rejects.toThrow(/did not preserve/); + }); + + it('reports an HTTP failure instead of substituting a client arrangement', async () => { + vi.stubGlobal('fetch', vi.fn(async () => new Response('{}', { status: 503, headers: { 'Content-Type': 'application/json' } }))); + + await expect(createHttpEngine('http://localhost').layout(emptyModel(), geometry)).rejects.toThrow(/503.*Nothing was changed/); + }); +}); diff --git a/src/ThreatModelForge.Studio/src/dfd/engineClient.ts b/src/ThreatModelForge.Studio/src/dfd/engineClient.ts index 9883bb5..dd6874c 100644 --- a/src/ThreatModelForge.Studio/src/dfd/engineClient.ts +++ b/src/ThreatModelForge.Studio/src/dfd/engineClient.ts @@ -84,6 +84,15 @@ export interface MergeResult { conflicts: MergeConflict[]; } +/** An engine-validated rectangle update. All semantic and author-owned data remains client-owned. */ +export interface LayoutElement { + id: string; + x: number; + y: number; + width: number; + height: number; +} + /** A file format the engine can read and/or write. */ export interface FormatInfo { id: string; @@ -263,6 +272,8 @@ export interface IEngineClient { * is unavailable for a two-way merge, where any overlapping difference is reported as a conflict. */ merge(base: TmForgeModel | null, ours: TmForgeModel, theirs: TmForgeModel): Promise; + /** Validates proposed Tidy geometry without rearranging it or changing trust claims. */ + layout(model: TmForgeModel, positions: LayoutElement[]): Promise; } /** @@ -316,6 +327,30 @@ function modelFromApplyResult(dto: components['schemas']['ApplyResultDto'] | und return toModel(dto.model); } +/** A refused layout never falls back to an unvalidated client-side arrangement. */ +function layoutElements(dto: components['schemas']['LayoutResultDto'] | undefined, positions: LayoutElement[]): LayoutElement[] { + if (!dto?.success || !dto.elements) { + throw new Error(dto?.error ?? 'The engine did not return a validated arrangement.'); + } + const elements = dto.elements.map((element) => { + if (!element.id || ![element.x, element.y, element.width, element.height].every((value) => typeof value === 'number' && Number.isSafeInteger(value)) + || Number(element.width) < 20 || Number(element.height) < 20) { + throw new Error('The engine returned incomplete layout geometry. Nothing was changed.'); + } + return { id: element.id, x: Number(element.x), y: Number(element.y), width: Number(element.width), height: Number(element.height) }; + }); + const proposed = new Map(positions.map((element) => [element.id, element])); + if (elements.length !== positions.length || new Set(elements.map((element) => element.id)).size !== elements.length + || elements.some((element) => { + const expected = proposed.get(element.id); + return !expected || element.x !== expected.x || element.y !== expected.y + || element.width !== expected.width || element.height !== expected.height; + })) { + throw new Error('The engine did not preserve the proposed Tidy layout. Update the engine; nothing was changed.'); + } + return elements; +} + /** Encodes bytes as base64 for the engine's read/detect payloads. */ function toBase64(bytes: Uint8Array): string { let binary = ''; @@ -457,30 +492,39 @@ function toPropertyDescriptor(dto: components['schemas']['PropertyDescriptor']): /** Normalizes a generated TmForgeModelDto (all fields optional/nullable) onto the UI model. */ export function toModel(dto: components['schemas']['TmForgeModelDto']): TmForgeModel { + const elements = (items: components['schemas']['TmForgeModelDto']['elements']) => (items ?? []).map((e) => ({ + id: e.id ?? '', + kind: (e.kind ?? 'process') as DfdKind, + name: e.name ?? '', + x: Number(e.x ?? 0), + y: Number(e.y ?? 0), + width: e.width == null ? undefined : Number(e.width), + height: e.height == null ? undefined : Number(e.height), + properties: e.properties ?? {}, + })); + const flows = (items: components['schemas']['TmForgeModelDto']['flows']) => (items ?? []).map((f) => ({ + id: f.id ?? '', + source: f.source ?? '', + target: f.target ?? '', + name: f.name ?? '', + properties: f.properties ?? {}, + })); return { schema: 'tmforge-json', version: '0.1', - elements: (dto.elements ?? []).map((e) => ({ - id: e.id ?? '', - kind: (e.kind ?? 'process') as DfdKind, - name: e.name ?? '', - x: Number(e.x ?? 0), - y: Number(e.y ?? 0), - width: e.width == null ? undefined : Number(e.width), - height: e.height == null ? undefined : Number(e.height), - properties: e.properties ?? {}, - })), - flows: (dto.flows ?? []).map((f) => ({ - id: f.id ?? '', - source: f.source ?? '', - target: f.target ?? '', - name: f.name ?? '', - properties: f.properties ?? {}, + elements: elements(dto.elements), + flows: flows(dto.flows), + diagrams: dto.diagrams?.map((page) => ({ + id: page.id ?? '', + name: page.name ?? '', + elements: elements(page.elements), + flows: flows(page.flows), })), analysis: dto.analysis ? { disabledPacks: dto.analysis.disabledPacks ?? undefined, disabledRuleIds: dto.analysis.disabledRuleIds ?? undefined, + expectedPacks: dto.analysis.expectedPacks?.map((pack) => ({ id: pack.id ?? '', fingerprint: pack.fingerprint ?? '' })), } : undefined, threats: dto.threats?.map(toThreatTriage), @@ -709,6 +753,10 @@ class OfflineEngineClient implements IEngineClient { new Error('Three-way merge requires the .NET engine. Start the API (or use the hosted app), then reload.'), ); } + + public layout(): Promise { + return Promise.reject(new Error('Tidy requires the .NET engine. Use Labels only until the engine is available.')); + } } class HttpEngineClient implements IEngineClient { @@ -910,6 +958,16 @@ class HttpEngineClient implements IEngineClient { } return toMergeResult(data); } + + public async layout(model: TmForgeModel, positions: LayoutElement[]): Promise { + const { data, response } = await this.client.POST('/v1/model/layout', { + body: { model, positions }, + }); + if (!response.ok) { + throw new Error(`Engine layout failed (${response.status}). Nothing was changed.`); + } + return layoutElements(data, positions); + } } /** The `[JSExport]` methods on the WASM `ThreatModelForge.Wasm.Engine` type (all string in/out). */ @@ -934,6 +992,7 @@ interface WasmEngineExports { RuleBundle(): string; Analysis(tmforgeJson: string): string; AnalysisReport(tmforgeJson: string, format: string): string; + Layout(requestJson: string): string; } /** @@ -941,7 +1000,7 @@ interface WasmEngineExports { * engine the `/v1` API runs (both go through the shared `ThreatModelForge.Engine` facade); only the * transport differs. tmforge-json crosses the boundary as a string, binary documents as base64. */ -class WasmEngineClient implements IEngineClient { +export class WasmEngineClient implements IEngineClient { public readonly label = 'engine (wasm)'; private readonly wasm: WasmEngineExports; @@ -1049,6 +1108,10 @@ class WasmEngineClient implements IEngineClient { ) as components['schemas']['MergeResultDto']; return toMergeResult(dto); } + + public async layout(model: TmForgeModel, positions: LayoutElement[]): Promise { + return layoutElements(JSON.parse(this.wasm.Layout(JSON.stringify({ model, positions }))), positions); + } } /** The honest offline fallback client (client-side authoring only). */ diff --git a/src/ThreatModelForge.Wasm/Engine.cs b/src/ThreatModelForge.Wasm/Engine.cs index 7d9267e..9a813b6 100644 --- a/src/ThreatModelForge.Wasm/Engine.cs +++ b/src/ThreatModelForge.Wasm/Engine.cs @@ -161,6 +161,13 @@ public static string Detect(string contentBase64) return format is null ? string.Empty : Serialize(format); } + /// Arranges geometry while preserving boundary membership and actual flow crossings. + /// The LayoutRequestDto JSON: original model, optional proposed positions and metrics. + /// Geometry updates or an explicit refusal, as LayoutResultDto JSON. + [JSExport] + public static string Layout(string requestJson) + => Serialize(EngineService.Layout(JsonSerializer.Deserialize(requestJson, JsonOptions) ?? new LayoutRequestDto())); + /// Reads a document in any registered format into the canonical tmforge-json model. /// The raw document bytes, base64-encoded. /// An explicit format id, or an empty string to content-sniff. diff --git a/test/ThreatModelForge.Api.Tests/ApiEndpointsTest.cs b/test/ThreatModelForge.Api.Tests/ApiEndpointsTest.cs index 53a3c55..78be843 100644 --- a/test/ThreatModelForge.Api.Tests/ApiEndpointsTest.cs +++ b/test/ThreatModelForge.Api.Tests/ApiEndpointsTest.cs @@ -10,6 +10,7 @@ namespace ThreatModelForge.Api.Tests using System.Threading.Tasks; using Microsoft.AspNetCore.Mvc.Testing; using Microsoft.VisualStudio.TestTools.UnitTesting; + using ThreatModelForge.Engine; /// /// Tests the hosted /v1 surface over real HTTP. The rest of this project drives @@ -159,6 +160,54 @@ public async Task Merge_AcceptsBaseOursAndTheirs() Assert.AreEqual(JsonValueKind.Object, body.RootElement.ValueKind); } + /// The HTTP layout route returns exactly the shared facade's author-id geometry. + /// A task. + [TestMethod] + public async Task Layout_MatchesTheSharedEngine() + { + string request = "{\"model\":" + Model + ",\"positions\":[{\"id\":\"a\",\"x\":100,\"y\":150,\"width\":240,\"height\":120}]}"; + JsonSerializerOptions options = new JsonSerializerOptions(JsonSerializerDefaults.Web); + LayoutRequestDto input = JsonSerializer.Deserialize(request, options) ?? new LayoutRequestDto(); + string expected = JsonSerializer.Serialize(EngineService.Layout(input), options); + + using HttpResponseMessage response = await PostJson("/v1/model/layout", request); + + Assert.AreEqual(HttpStatusCode.OK, response.StatusCode); + Assert.AreEqual(expected, await response.Content.ReadAsStringAsync()); + } + + /// The HTTP route validates a Tidy candidate without moving it into new layers. + /// A task. + [TestMethod] + public async Task Layout_PreservesProposedTidyPositions() + { + string request = "{\"model\":" + Model + ",\"positions\":[{\"id\":\"a\",\"x\":345,\"y\":678,\"width\":200,\"height\":120}]}"; + + using HttpResponseMessage response = await PostJson("/v1/model/layout", request); + + Assert.AreEqual(HttpStatusCode.OK, response.StatusCode); + using JsonDocument body = JsonDocument.Parse(await response.Content.ReadAsStringAsync()); + Assert.IsTrue(body.RootElement.GetProperty("success").GetBoolean()); + JsonElement element = body.RootElement.GetProperty("elements")[0]; + Assert.AreEqual("a", element.GetProperty("id").GetString()); + Assert.AreEqual(345, element.GetProperty("x").GetInt32()); + Assert.AreEqual(678, element.GetProperty("y").GetInt32()); + } + + /// A refusal is explicit and carries no partial geometry. + /// A task. + [TestMethod] + public async Task Layout_ReportsInvalidInputWithoutPatches() + { + using HttpResponseMessage response = await PostJson("/v1/model/layout", "{}"); + + Assert.AreEqual(HttpStatusCode.OK, response.StatusCode); + using JsonDocument body = JsonDocument.Parse(await response.Content.ReadAsStringAsync()); + Assert.IsFalse(body.RootElement.GetProperty("success").GetBoolean()); + Assert.AreEqual(0, body.RootElement.GetProperty("elements").GetArrayLength()); + Assert.IsFalse(string.IsNullOrWhiteSpace(body.RootElement.GetProperty("error").GetString())); + } + /// Verifies the .tm7 export is delivered as a downloadable XML document. /// A task. [TestMethod] diff --git a/test/ThreatModelForge.Api.Tests/EngineLayoutTest.cs b/test/ThreatModelForge.Api.Tests/EngineLayoutTest.cs new file mode 100644 index 0000000..d11d608 --- /dev/null +++ b/test/ThreatModelForge.Api.Tests/EngineLayoutTest.cs @@ -0,0 +1,610 @@ +namespace ThreatModelForge.Api.Tests +{ + using System; + using System.Collections.Generic; + using System.IO; + using System.Linq; + using System.Text.Json; + using Microsoft.VisualStudio.TestTools.UnitTesting; + using ThreatModelForge.Analysis; + using ThreatModelForge.Editing; + using ThreatModelForge.Engine; + using ThreatModelForge.Formats; + using ThreatModelForge.Model; + using ThreatModelForge.Model.Abstracts; + + /// Crossing-preserving layout through the host-neutral facade. + [TestClass] + public class EngineLayoutTest + { + /// Layout returns original author ids without rewriting any input or author state. + [TestMethod] + public void LayoutPreservesIdentityFindingsAndAuthorState() + { + TmForgeModelDto model = Model(); + string before = JsonSerializer.Serialize(model); + LayoutResultDto result = EngineService.Layout(new LayoutRequestDto { Model = model }); + + Assert.IsTrue(result.Success, result.Error); + Assert.AreEqual(1, result.Pages); + Assert.AreEqual(2, result.Components); + CollectionAssert.AreEquivalent(model.Elements!.Select(element => element.Id).ToList(), result.Elements.Select(element => element.Id).ToList()); + Assert.AreEqual(before, JsonSerializer.Serialize(model), "the request is immutable on success too"); + + TmForgeModelDto placed = ApplyGeometry(model, result); + Assert.IsTrue(BoundaryCrossingDiff.Compare(Read(model), Read(placed)).IsEmpty); + Assert.IsTrue(ModelDiff.Compare(Read(model), Read(placed)).IsEmpty); + + // The metadata fixture pins an unavailable pack to prove the pin survives. Compare + // real detection with that deliberately missing dependency removed on both sides. + IReadOnlyList beforeFindings = EngineService.Analyze(WithoutPackExpectation(model)); + IReadOnlyList afterFindings = EngineService.Analyze(WithoutPackExpectation(placed)); + Assert.IsTrue(beforeFindings.Count > 2); + Assert.IsFalse(beforeFindings.Concat(afterFindings).Any(finding => finding.Id == "engine-error")); + CollectionAssert.AreEquivalent( + beforeFindings.Select(finding => finding.Id).ToList(), + afterFindings.Select(finding => finding.Id).ToList()); + Assert.AreEqual(JsonSerializer.Serialize(model.Analysis), JsonSerializer.Serialize(placed.Analysis)); + Assert.AreEqual(JsonSerializer.Serialize(model.Threats), JsonSerializer.Serialize(placed.Threats)); + } + + /// Two arrangements and a repeated arrangement all produce the same rectangles. + [TestMethod] + public void LayoutIsDeterministicAndIdempotent() + { + TmForgeModelDto model = Model(); + LayoutResultDto first = EngineService.Layout(new LayoutRequestDto { Model = model }); + LayoutResultDto second = EngineService.Layout(new LayoutRequestDto { Model = model }); + LayoutResultDto repeated = EngineService.Layout(new LayoutRequestDto { Model = ApplyGeometry(model, first) }); + + Assert.IsTrue(first.Success, first.Error); + Assert.IsTrue(repeated.Success, repeated.Error); + Assert.AreEqual(JsonSerializer.Serialize(first), JsonSerializer.Serialize(second)); + Assert.AreEqual(JsonSerializer.Serialize(first), JsonSerializer.Serialize(repeated)); + } + + /// Nested membership is retained and members clear the boundary title and edges. + [TestMethod] + public void LayoutHonorsBoundaryHeaderClearance() + { + TmForgeModelDto model = Model(); + LayoutRequestDto request = new LayoutRequestDto + { + Model = model, + Options = new LayoutOptions { BoundaryHeaderHeight = 80 }, + }; + + LayoutResultDto result = EngineService.Layout(request); + + Assert.IsTrue(result.Success, result.Error); + LayoutElementDto member = result.Elements.Single(element => element.Id == "source"); + LayoutElementDto boundary = result.Elements.Single(element => element.Id == "boundary"); + Assert.AreEqual(100, member.Width); + Assert.AreEqual(60, member.Height); + Assert.IsTrue(member.Y >= boundary.Y + 80 + 24); + Assert.IsTrue(member.X > boundary.X && member.X + member.Width < boundary.X + boundary.Width); + Assert.IsTrue(member.Y + member.Height < boundary.Y + boundary.Height); + Assert.IsTrue(BoundaryCrossingDiff.Compare(Read(model), Read(ApplyGeometry(model, result))).IsEmpty); + } + + /// Overlapping claims cannot be discarded by automatic placement. + [TestMethod] + public void LayoutRefusesOverlappingMembershipWithoutMutatingInput() + { + TmForgeModelDto model = Model(overlapping: true); + string before = JsonSerializer.Serialize(model); + LayoutResultDto result = EngineService.Layout(new LayoutRequestDto + { + Model = model, + }); + + Assert.IsFalse(result.Success); + Assert.IsEmpty(result.Elements); + StringAssert.Contains(result.Error, "boundary"); + Assert.AreEqual(before, JsonSerializer.Serialize(model)); + } + + /// An unsafe page refuses the whole multi-page request without partial patches. + [TestMethod] + public void LayoutRefusesMultiplePagesAtomically() + { + TmForgeModelDto model = MultiPage(overlapping: true); + LayoutResultDto result = EngineService.Layout(new LayoutRequestDto { Model = model }); + + Assert.IsFalse(result.Success); + Assert.IsEmpty(result.Elements); + Assert.AreEqual(0, result.Pages); + } + + /// All page identities and crossing sets survive a multi-page arrangement. + [TestMethod] + public void LayoutPreservesEveryPage() + { + TmForgeModelDto model = MultiPage(overlapping: false); + LayoutResultDto result = EngineService.Layout(new LayoutRequestDto { Model = model }); + + Assert.IsTrue(result.Success, result.Error); + Assert.AreEqual(2, result.Pages); + Assert.AreEqual(6, result.Elements.Count); + Assert.IsTrue(BoundaryCrossingDiff.Compare(Read(model), Read(ApplyGeometry(model, result))).IsEmpty); + } + + /// Zero-hop flows and directed cycles terminate without changing their topology. + [TestMethod] + public void LayoutHandlesCyclesAndSelfLoops() + { + TmForgeModelDto original = Model(); + TmForgeModelDto model = new TmForgeModelDto + { + Elements = original.Elements, + Flows = new[] + { + original.Flows![0], + new TmForgeFlowDto { Id = "return", Source = "target", Target = "source" }, + new TmForgeFlowDto { Id = "self", Source = "source", Target = "source" }, + }, + }; + + LayoutResultDto result = EngineService.Layout(new LayoutRequestDto { Model = model }); + + Assert.IsTrue(result.Success, result.Error); + Assert.IsTrue(BoundaryCrossingDiff.Compare(Read(model), Read(ApplyGeometry(model, result))).IsEmpty); + } + + /// Invalid shapes must not be normalized into apparently safe input by the format reader. + /// The kind to validate. + /// The authored width. + /// The authored height. + [TestMethod] + [DataRow("mystery", 100, 60)] + [DataRow("process", 0, 60)] + [DataRow("process", -1, 60)] + [DataRow("process", 10, 60)] + [DataRow("process", 100001, 60)] + public void LayoutRejectsInvalidShapes(string kind, int width, int height) + { + TmForgeModelDto model = new TmForgeModelDto + { + Elements = new[] { new TmForgeElementDto { Id = "a", Kind = kind, Width = width, Height = height } }, + }; + + LayoutResultDto result = EngineService.Layout(new LayoutRequestDto { Model = model }); + + Assert.IsFalse(result.Success); + Assert.IsEmpty(result.Elements); + } + + /// Duplicate IDs, including differently-spelled copies of one GUID, are refused. + /// The first identity. + /// The colliding identity. + [TestMethod] + [DataRow("a", "a")] + [DataRow("00000000-0000-0000-0000-000000000abc", "00000000000000000000000000000ABC")] + [DataRow("", "b")] + public void LayoutRejectsIdentityCollisions(string first, string second) + { + TmForgeModelDto model = new TmForgeModelDto + { + Elements = new[] + { + new TmForgeElementDto { Id = first, Kind = "process" }, + new TmForgeElementDto { Id = second, Kind = "process" }, + }, + }; + + Assert.IsFalse(EngineService.Layout(new LayoutRequestDto { Model = model }).Success); + } + + /// Single-page dangling flows identify the missing endpoint rather than implying a page mismatch. + /// The source endpoint id. + /// The target endpoint id. + /// The endpoint role that must be named in the error. + [TestMethod] + [DataRow("missing", "target", "source")] + [DataRow("source", "missing", "target")] + public void LayoutRejectsDanglingFlows(string source, string target, string endpoint) + { + TmForgeModelDto model = new TmForgeModelDto + { + Elements = Model().Elements, + Flows = new[] { new TmForgeFlowDto { Id = "audit-request", Name = "Read audit", Source = source, Target = target } }, + }; + + LayoutResultDto result = EngineService.Layout(new LayoutRequestDto { Model = model }); + + Assert.IsFalse(result.Success); + Assert.IsEmpty(result.Elements); + StringAssert.Contains(result.Error, "audit-request"); + StringAssert.Contains(result.Error, "Read audit"); + StringAssert.Contains(result.Error, endpoint); + StringAssert.Contains(result.Error, "missing"); + } + + /// An imported floating connector names its unattached endpoint and leaves the model untouched. + [TestMethod] + public void LayoutIdentifiesUnattachedTarget() + { + TmForgeModelDto model = new TmForgeModelDto + { + Elements = Model().Elements, + Flows = new[] { new TmForgeFlowDto { Id = "watch", Name = "TLS: Watch", Source = "source", Target = Guid.Empty.ToString() } }, + }; + string before = JsonSerializer.Serialize(model); + + LayoutResultDto result = EngineService.Layout(new LayoutRequestDto { Model = model }); + + Assert.IsFalse(result.Success); + Assert.IsEmpty(result.Elements); + StringAssert.Contains(result.Error, "TLS: Watch"); + StringAssert.Contains(result.Error, "unattached target endpoint"); + StringAssert.Contains(result.Error, Guid.Empty.ToString()); + Assert.AreEqual(before, JsonSerializer.Serialize(model)); + } + + /// Duplicate flow ids are reported as identity collisions, not endpoint or page errors. + [TestMethod] + public void LayoutIdentifiesDuplicateFlowIds() + { + TmForgeModelDto model = new TmForgeModelDto + { + Elements = Model().Elements, + Flows = new[] + { + new TmForgeFlowDto { Id = "audit-request", Name = "Read audit", Source = "source", Target = "target" }, + new TmForgeFlowDto { Id = "audit-request", Name = "Read audit again", Source = "source", Target = "target" }, + }, + }; + + LayoutResultDto result = EngineService.Layout(new LayoutRequestDto { Model = model }); + + Assert.IsFalse(result.Success); + Assert.IsEmpty(result.Elements); + StringAssert.Contains(result.Error, "audit-request"); + StringAssert.Contains(result.Error, "duplicate"); + } + + /// An oversized flow label must not be presented as an endpoint or page error. + [TestMethod] + public void LayoutIdentifiesOversizedFlowLabels() + { + TmForgeModelDto model = new TmForgeModelDto + { + Elements = Model().Elements, + Flows = new[] { new TmForgeFlowDto { Id = "verbose-flow", Name = new string('x', 4097), Source = "source", Target = "target" } }, + }; + + LayoutResultDto result = EngineService.Layout(new LayoutRequestDto { Model = model }); + + Assert.IsFalse(result.Success); + Assert.IsEmpty(result.Elements); + StringAssert.Contains(result.Error, "verbose-flow"); + StringAssert.Contains(result.Error, "4096"); + } + + /// Malformed endpoint text is bounded when it is reported to the caller. + [TestMethod] + public void LayoutIdentifiesOversizedEndpointIds() + { + TmForgeModelDto model = new TmForgeModelDto + { + Elements = Model().Elements, + Flows = new[] { new TmForgeFlowDto { Id = "invalid-endpoint", Source = "source", Target = new string('x', 10000) } }, + }; + + LayoutResultDto result = EngineService.Layout(new LayoutRequestDto { Model = model }); + + Assert.IsFalse(result.Success); + Assert.IsEmpty(result.Elements); + StringAssert.Contains(result.Error, "invalid-endpoint"); + StringAssert.Contains(result.Error, "target endpoint id longer than 256"); + Assert.IsTrue(result.Error!.Length < 256); + } + + /// Oversized graphs fail before arrangement and empty models are valid no-ops. + [TestMethod] + public void LayoutBoundsGraphWork() + { + TmForgeModelDto model = new TmForgeModelDto + { + Elements = Enumerable.Range(0, LayoutOperations.MaximumElements + 1) + .Select(index => new TmForgeElementDto { Id = "p" + index, Kind = "process" }).ToArray(), + }; + + LayoutResultDto tooLarge = EngineService.Layout(new LayoutRequestDto { Model = model }); + Assert.IsFalse(tooLarge.Success); + StringAssert.Contains(tooLarge.Error, "limited"); + + LayoutResultDto empty = EngineService.Layout(new LayoutRequestDto { Model = new TmForgeModelDto() }); + Assert.IsTrue(empty.Success, empty.Error); + Assert.IsEmpty(empty.Elements); + Assert.IsFalse(EngineService.Layout(new LayoutRequestDto()).Success); + } + + /// Boundary-ended flows cannot validate with stale endpoints and then reroute on read. + [TestMethod] + public void LayoutRefusesBoundaryEndpoints() + { + TmForgeModelDto model = new TmForgeModelDto + { + Elements = Model().Elements, + Flows = new[] { new TmForgeFlowDto { Id = "f", Source = "boundary", Target = "source" } }, + }; + + LayoutResultDto result = EngineService.Layout(new LayoutRequestDto { Model = model }); + + Assert.IsFalse(result.Success); + Assert.IsEmpty(result.Elements); + StringAssert.Contains(result.Error, "not trust boundaries"); + } + + /// Output must satisfy the same bounds as input, so every success remains arrangeable. + [TestMethod] + public void LayoutRefusesOutOfRangeCandidate() + { + TmForgeModelDto model = new TmForgeModelDto + { + Elements = new[] + { + new TmForgeElementDto { Id = "a", Kind = "process", Width = 20, Height = 20 }, + new TmForgeElementDto { Id = "b", Kind = "process", Width = 20, Height = 20 }, + }, + }; + LayoutResultDto result = EngineService.Layout(new LayoutRequestDto + { + Model = model, Options = new LayoutOptions { OriginY = 1000000 }, + }); + + Assert.IsFalse(result.Success); + Assert.IsEmpty(result.Elements); + } + + /// Page identity must never replace a component identity in the wire-id map. + [TestMethod] + public void LayoutRefusesCrossNamespaceGuidCollision() + { + TmForgeModelDto model = new TmForgeModelDto + { + Diagrams = new[] + { + new TmForgeDiagramDto + { + Id = "page-one", + Elements = new[] { new TmForgeElementDto { Id = "00000000-0000-0000-0000-000000000001", Kind = "process" } }, + }, + new TmForgeDiagramDto { Id = "00000000000000000000000000000001" }, + }, + }; + + LayoutResultDto result = EngineService.Layout(new LayoutRequestDto { Model = model }); + + Assert.IsFalse(result.Success); + StringAssert.Contains(result.Error, "same internal GUID"); + } + + /// A page and element may share an author alias when their internal namespaces differ. + [TestMethod] + public void LayoutAcceptsSeparatePageAndElementAliasNamespaces() + { + TmForgeModelDto model = new TmForgeModelDto + { + Diagrams = new[] + { + new TmForgeDiagramDto { Id = "source", Elements = Model().Elements, Flows = Model().Flows }, + }, + }; + + LayoutResultDto result = EngineService.Layout(new LayoutRequestDto { Model = model }); + + Assert.IsTrue(result.Success, result.Error); + CollectionAssert.Contains(result.Elements.Select(element => element.Id).ToArray(), "source"); + } + + /// A large property bag has no effect and is not hydrated into the geometry candidate. + [TestMethod] + public void LayoutIgnoresNonGeometryState() + { + TmForgeModelDto model = new TmForgeModelDto + { + Elements = new[] + { + new TmForgeElementDto + { + Id = "p", Kind = "process", Name = "Process", + Properties = Enumerable.Range(0, 20000).ToDictionary(index => "k" + index, _ => "value"), + }, + }, + Threats = new[] { new ThreatStateDto { Id = "ignored-malformed-triage" } }, + }; + + LayoutResultDto result = EngineService.Layout(new LayoutRequestDto { Model = model }); + + Assert.IsTrue(result.Success, result.Error); + Assert.AreEqual("p", result.Elements.Single().Id); + } + + /// Page scoping validates one candidate without touching an unsafe sibling. + [TestMethod] + public void LayoutScopesToOnePage() + { + TmForgeModelDto model = MultiPage(overlapping: true); + LayoutResultDto first = EngineService.Layout(new LayoutRequestDto { Model = model, Page = "1" }); + LayoutResultDto named = EngineService.Layout(new LayoutRequestDto { Model = model, Page = "A" }); + + Assert.IsTrue(first.Success, first.Error); + Assert.AreEqual(1, first.Pages); + Assert.AreEqual(JsonSerializer.Serialize(first), JsonSerializer.Serialize(named)); + Assert.IsFalse(EngineService.Layout(new LayoutRequestDto { Model = model, Page = "missing" }).Success); + } + + /// Large dense label sets trip the work bound even below the element-count limits. + [TestMethod] + public void LayoutBoundsDenseGraphWork() + { + TmForgeModelDto model = new TmForgeModelDto + { + Elements = Model().Elements, + Flows = Enumerable.Range(0, 900).Select(index => new TmForgeFlowDto + { + Id = "f" + index, Name = "Request", Source = "source", Target = "target", + }).ToArray(), + }; + + LayoutResultDto result = EngineService.Layout(new LayoutRequestDto { Model = model }); + + Assert.IsFalse(result.Success); + StringAssert.Contains(result.Error, "work budget"); + } + + /// In-place cleanup keeps its proposed coordinates rather than being layered again. + [TestMethod] + public void LayoutValidatesProposedPositionsWithoutRearranging() + { + TmForgeModelDto model = Model(); + string before = JsonSerializer.Serialize(model); + LayoutElementDto[] positions = model.Elements!.Select(element => new LayoutElementDto + { + Id = element.Id, X = element.X + 300, Y = element.Y - 50, + Width = element.Width!.Value, Height = element.Height!.Value, + }).ToArray(); + + LayoutResultDto result = EngineService.Layout(new LayoutRequestDto { Model = model, Positions = positions }); + + Assert.IsTrue(result.Success, result.Error); + Assert.AreEqual(JsonSerializer.Serialize(positions), JsonSerializer.Serialize(result.Elements)); + Assert.AreEqual(before, JsonSerializer.Serialize(model)); + Assert.IsTrue(BoundaryCrossingDiff.Compare(Read(model), Read(ApplyGeometry(model, result))).IsEmpty); + } + + /// A supplied cleanup candidate cannot bypass the crossing-preservation guard. + [TestMethod] + public void LayoutRejectsUnsafeProposedPositions() + { + TmForgeModelDto model = Model(); + LayoutElementDto[] positions = model.Elements!.Select(element => new LayoutElementDto + { + Id = element.Id, X = element.Id == "source" ? 1000 : element.X, Y = element.Y, + Width = element.Width!.Value, Height = element.Height!.Value, + }).ToArray(); + + LayoutResultDto result = EngineService.Layout(new LayoutRequestDto { Model = model, Positions = positions }); + + Assert.IsFalse(result.Success); + Assert.IsEmpty(result.Elements); + StringAssert.Contains(result.Error, "boundary membership"); + } + + /// Proposed geometry must be complete, unique and bounded. + /// The invalid candidate shape. + [TestMethod] + [DataRow("partial")] + [DataRow("duplicate")] + [DataRow("unknown")] + [DataRow("oversized")] + public void LayoutRejectsInvalidProposedPositions(string scenario) + { + TmForgeModelDto model = Model(); + List positions = model.Elements!.Select(element => new LayoutElementDto + { + Id = element.Id, X = element.X, Y = element.Y, Width = element.Width!.Value, Height = element.Height!.Value, + }).ToList(); + if (scenario == "partial") + { + positions.RemoveAt(0); + } + else if (scenario == "duplicate") + { + positions[0] = positions[1]; + } + else if (scenario == "unknown" || scenario == "oversized") + { + positions[0] = new LayoutElementDto + { + Id = scenario == "unknown" ? "absent" : positions[0].Id, + X = 1000001, Width = 100, Height = 100, + }; + } + + LayoutResultDto result = EngineService.Layout(new LayoutRequestDto + { + Model = model, Positions = positions, + }); + + Assert.IsFalse(result.Success, scenario); + Assert.IsEmpty(result.Elements); + } + + private static TmForgeModelDto WithoutPackExpectation(TmForgeModelDto model) => new TmForgeModelDto + { + Schema = model.Schema, Version = model.Version, Elements = model.Elements, Flows = model.Flows, + Diagrams = model.Diagrams, Threats = model.Threats, + Analysis = new TmForgeAnalysisDto { DisabledRuleIds = model.Analysis?.DisabledRuleIds }, + }; + + private static TmForgeModelDto Model(bool overlapping = false, string prefix = "") + { + List elements = new List + { + new TmForgeElementDto { Id = prefix + "boundary", Kind = "boundary", Name = "Service", X = 100, Y = 100, Width = 400, Height = 400 }, + new TmForgeElementDto { Id = prefix + "source", Kind = "process", Name = "API", X = 340, Y = 250, Width = 100, Height = 60 }, + new TmForgeElementDto { Id = prefix + "target", Kind = "datastore", Name = "Audit", X = 760, Y = 250, Width = 100, Height = 60 }, + }; + if (overlapping) + { + elements.Add(new TmForgeElementDto { Id = prefix + "overlap", Kind = "boundary", Name = "Other claim", X = 300, Y = 100, Width = 400, Height = 400 }); + } + + return new TmForgeModelDto + { + Schema = "tmforge-json", Version = "0.1", Elements = elements, + Flows = new[] { new TmForgeFlowDto { Id = prefix + "flow", Name = "Request", Source = prefix + "source", Target = prefix + "target" } }, + Analysis = new TmForgeAnalysisDto + { + DisabledRuleIds = new[] { "TM1003" }, + ExpectedPacks = new[] { new ExpectedRulePackDto { Id = "policy", Fingerprint = "sha256:pinned" } }, + }, + Threats = new[] { new ThreatStateDto { Id = "manual:review", Manual = true, Title = "Reviewed", State = "Accepted", Justification = "Tracked decision" } }, + }; + } + + private static TmForgeModelDto MultiPage(bool overlapping) + { + TmForgeModelDto first = Model(); + TmForgeModelDto second = Model(overlapping, "second-"); + return new TmForgeModelDto + { + Elements = first.Elements, Flows = first.Flows, + Diagrams = new[] + { + new TmForgeDiagramDto { Id = "page-a", Name = "A", Elements = first.Elements, Flows = first.Flows }, + new TmForgeDiagramDto { Id = "page-b", Name = "B", Elements = second.Elements, Flows = second.Flows }, + }, + }; + } + + private static TmForgeModelDto ApplyGeometry(TmForgeModelDto model, LayoutResultDto result) + { + Dictionary patches = result.Elements.ToDictionary(element => element.Id); + IReadOnlyList? Update(IReadOnlyList? elements) => elements?.Select(element => new TmForgeElementDto + { + Id = element.Id, Kind = element.Kind, Name = element.Name, Properties = element.Properties, + X = patches[element.Id].X, Y = patches[element.Id].Y, + Width = patches[element.Id].Width, Height = patches[element.Id].Height, + }).ToArray(); + return new TmForgeModelDto + { + Schema = model.Schema, Version = model.Version, Analysis = model.Analysis, Threats = model.Threats, + Elements = Update(model.Elements), Flows = model.Flows, + Diagrams = model.Diagrams?.Select(page => new TmForgeDiagramDto + { + Id = page.Id, Name = page.Name, Elements = Update(page.Elements), Flows = page.Flows, + }).ToArray(), + }; + } + + private static ThreatModel Read(TmForgeModelDto dto) + { + using MemoryStream stream = new MemoryStream(JsonSerializer.SerializeToUtf8Bytes(dto)); + return new TmForgeJsonFormat().Read(stream); + } + } +} diff --git a/test/ThreatModelForge.Cli.Tests/BoundaryAndLayoutTest.cs b/test/ThreatModelForge.Cli.Tests/BoundaryAndLayoutTest.cs index 008c610..87a7c23 100644 --- a/test/ThreatModelForge.Cli.Tests/BoundaryAndLayoutTest.cs +++ b/test/ThreatModelForge.Cli.Tests/BoundaryAndLayoutTest.cs @@ -5,7 +5,9 @@ namespace ThreatModelForge.Cli.Tests using System.Linq; using System.Text.Json; using Microsoft.VisualStudio.TestTools.UnitTesting; + using ThreatModelForge.Analysis; using ThreatModelForge.Editing; + using ThreatModelForge.Engine; using ThreatModelForge.Model; using ThreatModelForge.Model.Abstracts; @@ -173,6 +175,276 @@ public void LayoutCheckPassesForAnAuthoredModel() Assert.AreEqual(0, Capture(() => LayoutCommand.Run(new[] { path, "--check" })).Exit); } + /// A layout must not discard one of an element's overlapping trust claims. + [TestMethod] + public void LayoutRefusesOverlappingMembershipWithoutWriting() + { + ThreatModel model = LayoutFixture(overlapping: true); + string path = this.SaveFixture(model); + string before = File.ReadAllText(path); + Assert.AreEqual(2, BoundaryCrossingDiff.Capture(model).Single().Boundaries.Count); + + Assert.AreEqual(1, Capture(() => LayoutCommand.Run(new[] { path, "--json" })).Exit); + + Assert.AreEqual(before, File.ReadAllText(path), "a refused layout must not write anything"); + } + + /// Connector coordinates, rather than node centers, determine the actual crossings. + [TestMethod] + public void LayoutRefusesConnectorCrossingChangesWithoutWriting() + { + ThreatModel model = LayoutFixture(overlapping: false); + Connector flow = model.DrawingSurfaceList[0].Lines.Values.OfType().Single(); + flow.SourceX = 50; + Assert.AreEqual(0, BoundaryCrossingDiff.Capture(model).Single().Boundaries.Count); + string path = this.SaveFixture(model); + string before = File.ReadAllText(path); + + Assert.AreEqual(1, Capture(() => LayoutCommand.Run(new[] { path })).Exit); + + Assert.AreEqual(before, File.ReadAllText(path)); + } + + /// Nested boundary memberships and crossings survive successful arrangement and save. + [TestMethod] + public void LayoutPreservesNestedBoundaryCrossings() + { + ThreatModel model = LayoutFixture(overlapping: false); + DrawingSurfaceModel surface = model.DrawingSurfaceList[0]; + AddFixtureBoundary(surface, 100, 100, 600, 600, "Outer"); + string path = this.SaveFixture(model); + Assert.AreEqual(2, BoundaryCrossingDiff.Capture(model).Single().Boundaries.Count); + + Assert.AreEqual(0, Capture(() => LayoutCommand.Run(new[] { path })).Exit); + + (ThreatModel after, _) = CliModelLoader.Load(path); + Assert.IsTrue(BoundaryCrossingDiff.Compare(model, after).IsEmpty); + Assert.IsTrue(ModelDiff.Compare(model, after).IsEmpty, "layout must not change identities, topology or properties"); + } + + /// An unsafe later page must prevent an earlier page from being saved too. + [TestMethod] + public void LayoutRefusesAllPagesAtomically() + { + ThreatModel model = LayoutFixture(overlapping: false); + model.DrawingSurfaceList.Add(LayoutFixture(overlapping: true).DrawingSurfaceList[0]); + string path = this.SaveFixture(model); + string before = File.ReadAllText(path); + + Assert.AreEqual(1, Capture(() => LayoutCommand.Run(new[] { path })).Exit); + + Assert.AreEqual(before, File.ReadAllText(path)); + } + + /// Label-only cleanup remains usable when full layout would change trust claims. + [TestMethod] + public void LayoutLabelsPreservesOverlappingBoundaryCrossings() + { + ThreatModel model = LayoutFixture(overlapping: true); + string path = this.SaveFixture(model); + + Assert.AreEqual(0, Capture(() => LayoutCommand.Run(new[] { path, "--labels" })).Exit); + + (ThreatModel after, _) = CliModelLoader.Load(path); + Assert.IsTrue(BoundaryCrossingDiff.Compare(model, after).IsEmpty); + } + + /// Line trust boundaries use intersection semantics and must also be preserved. + [TestMethod] + public void LayoutRefusesChangedLineBoundaryIntersections() + { + ThreatModel model = LayoutFixture(overlapping: false); + DrawingSurfaceModel surface = model.DrawingSurfaceList[0]; + LineBoundary boundary = new LineBoundary + { + Guid = Guid.NewGuid(), SourceX = 600, SourceY = 100, TargetX = 600, TargetY = 900, + }; + surface.Lines.Add(boundary.Guid, boundary); + Assert.AreEqual(2, BoundaryCrossingDiff.Capture(model).Single().Boundaries.Count); + string path = this.SaveFixture(model); + string before = File.ReadAllText(path); + + Assert.AreEqual(1, Capture(() => LayoutCommand.Run(new[] { path })).Exit); + + Assert.AreEqual(before, File.ReadAllText(path)); + } + + /// A refused multi-page operation leaves the original in-memory geometry untouched. + [TestMethod] + public void SharedLayoutRefusalIsAtomicInMemory() + { + ThreatModel model = LayoutFixture(overlapping: false); + model.DrawingSurfaceList.Add(LayoutFixture(overlapping: true).DrawingSurfaceList[0]); + string before = Geometry(model); + + bool success = LayoutOperations.TryApply(model.DrawingSurfaceList, null, out int moved, out string? error); + + Assert.IsFalse(success); + Assert.AreEqual(0, moved); + StringAssert.Contains(error, "No pages were changed"); + Assert.AreEqual(before, Geometry(model)); + } + + /// Scoping layout to a safe page does not arrange an unsafe sibling. + [TestMethod] + public void LayoutCanScopeToOnePage() + { + ThreatModel model = LayoutFixture(overlapping: false); + model.DrawingSurfaceList.Add(LayoutFixture(overlapping: true).DrawingSurfaceList[0]); + string path = this.SaveFixture(model); + + Assert.AreEqual(0, Capture(() => LayoutCommand.Run(new[] { path, "--page", "1" })).Exit); + + (ThreatModel after, _) = CliModelLoader.Load(path); + Assert.IsTrue(BoundaryCrossingDiff.Compare(model, after).IsEmpty); + Assert.AreEqual( + model.DrawingSurfaceList[1].Borders.Values.OfType().First().Left, + after.DrawingSurfaceList[1].Borders.Values.OfType().First().Left); + } + + /// Boundary placement refuses a clipped shape rather than claiming it is contained. + [TestMethod] + public void AddBoundaryRefusesAnOversizedMemberWithoutWriting() + { + string path = this.NewModel(); + Assert.AreEqual(0, Capture(() => AddCommand.Run(new[] { "boundary", path, "--alias", "TB" })).Exit); + string before = File.ReadAllText(path); + + string[] arguments = new[] + { + "process", path, "--name", "Oversized", "--boundary", "TB", "--width", "400", "--height", "200", + }; + Assert.AreEqual(1, Capture(() => AddCommand.Run(arguments)).Exit); + + Assert.AreEqual(before, File.ReadAllText(path)); + } + + /// Canonical JSON layout patches geometry instead of rewriting identities or author state. + [TestMethod] + public void LayoutPreservesCanonicalJsonIdentityAndMetadata() + { + const string Json = """ + {"schema":"tmforge-json","version":"0.1", + "elements":[{"id":"source","kind":"process","name":"A","x":600,"y":600,"width":100,"height":60}, + {"id":"target","kind":"process","name":"B","x":100,"y":100,"width":100,"height":60}], + "flows":[{"id":"f","source":"source","target":"target","name":"Request","labelOffset":{"x":20,"y":30}}], + "analysis":{"disabledRuleIds":["TM1003"],"expectedPacks":[{"id":"policy","fingerprint":"sha256:pinned"}]}, + "threats":[{"id":"manual:review","manual":true,"state":"Accepted","title":"Decision","justification":"Reviewed"}], + "extension":{"ownedBy":"author"}} + """; + string path = Path.Join(this.WorkingDirectory, "layout.tmforge.json"); + File.WriteAllText(path, Json); + + Assert.AreEqual(0, Capture(() => LayoutCommand.Run(new[] { path })).Exit); + + using JsonDocument before = JsonDocument.Parse(Json); + using JsonDocument after = JsonDocument.Parse(File.ReadAllText(path)); + Assert.AreEqual("source", after.RootElement.GetProperty("elements")[0].GetProperty("id").GetString()); + Assert.AreNotEqual(600, after.RootElement.GetProperty("elements")[0].GetProperty("x").GetInt32()); + foreach (string field in new[] { "flows", "analysis", "threats", "extension" }) + { + Assert.IsTrue(JsonElement.DeepEquals(before.RootElement.GetProperty(field), after.RootElement.GetProperty(field)), field); + } + } + + /// Duplicate wire ids must be refused before the reader silently rekeys the duplicate. + [TestMethod] + public void LayoutRefusesDuplicateCanonicalJsonIdsWithoutWriting() + { + const string Json = """ + {"schema":"tmforge-json","version":"0.1","elements":[ + {"id":"p","kind":"process","x":100,"y":100,"width":100,"height":60}, + {"id":"p","kind":"process","x":600,"y":100,"width":100,"height":60}],"flows":[]} + """; + string path = Path.Join(this.WorkingDirectory, "duplicate.tmforge.json"); + File.WriteAllText(path, Json); + + Assert.AreEqual(1, Capture(() => LayoutCommand.Run(new[] { path })).Exit); + + Assert.AreEqual(Json, File.ReadAllText(path)); + } + + /// JSON cannot store engine label handles, so labels-only reports a non-persisted no-op. + [TestMethod] + public void LayoutJsonLabelsOnlyReportsNoPersistedChange() + { + const string Json = """ + {"schema":"tmforge-json","version":"0.1","elements":[ + {"id":"p","kind":"process","x":100,"y":100,"width":100,"height":60}],"flows":[], + "extension":"retain"} + """; + string path = Path.Join(this.WorkingDirectory, "labels.tmforge.json"); + File.WriteAllText(path, Json); + + (int exit, string output) = Capture(() => LayoutCommand.Run(new[] { path, "--labels", "--json" })); + + Assert.AreEqual(0, exit); + Assert.AreEqual(Json, File.ReadAllText(path)); + using JsonDocument report = JsonDocument.Parse(output); + Assert.IsFalse(report.RootElement.GetProperty("data").GetProperty("labelsPersisted").GetBoolean()); + Assert.AreEqual(0, report.RootElement.GetProperty("data").GetProperty("labelsMoved").GetInt32()); + } + + /// Invalid spacing must be rejected, not ignored or allowed to overlap components. + /// The invalid spacing argument. + [TestMethod] + [DataRow("0")] + [DataRow("-1")] + [DataRow("2147483647")] + [DataRow("not-a-number")] + public void LayoutRejectsInvalidSpacingWithoutWriting(string spacing) + { + string path = this.SaveFixture(LayoutFixture(overlapping: false)); + string before = File.ReadAllText(path); + + Assert.AreEqual(1, Capture(() => LayoutCommand.Run(new[] { path, "--node-spacing", spacing })).Exit); + + Assert.AreEqual(before, File.ReadAllText(path)); + } + + private static string Geometry(ThreatModel model) + { + return JsonSerializer.Serialize(model.DrawingSurfaceList.Select(surface => new + { + shapes = surface.Borders.Values.OfType().Select(element => new + { + element.Guid, element.Left, element.Top, element.Width, element.Height, + }), + lines = surface.Lines.Values.OfType().Select(line => new + { + line.Guid, line.SourceX, line.SourceY, line.TargetX, line.TargetY, line.HandleX, line.HandleY, + }), + })); + } + + private static ThreatModel LayoutFixture(bool overlapping) + { + ThreatModel model = new ThreatModel(); + DrawingSurfaceModel surface = new DrawingSurfaceModel { Header = "Layout", Guid = Guid.NewGuid() }; + model.DrawingSurfaceList.Add(surface); + DiagramEditor editor = new DiagramEditor(model); + Guid source = editor.AddElement(surface, StencilKind.Process, 340, 250); + Guid target = editor.AddElement(surface, StencilKind.Process, 760, 250); + editor.SetElementName(surface, source, "Source"); + editor.SetElementName(surface, target, "Target"); + Guid flow = editor.AddConnector(surface, source, target); + editor.SetElementName(surface, flow, "Request"); + AddFixtureBoundary(surface, 100, 100, 400, 400, "First"); + if (overlapping) + { + AddFixtureBoundary(surface, 300, 100, 400, 400, "Second"); + } + + return model; + } + + private static void AddFixtureBoundary(DrawingSurfaceModel surface, int x, int y, int width, int height, string name) + { + BorderBoundary boundary = new BorderBoundary { Guid = Guid.NewGuid(), Left = x, Top = y, Width = width, Height = height }; + DiagramElementHelper.SetName(boundary, name); + surface.Borders.Add(boundary.Guid, boundary); + } + private static string AddElement(string kind, string path, string name) { (int exit, string stdout) = Capture(() => AddCommand.Run(new[] { kind, path, "--name", name, "--json" })); @@ -227,5 +499,13 @@ private string NewModel() Capture(() => NewCommand.Run(new[] { path, "--name", "Test" })); return path; } + + private string SaveFixture(ThreatModel model) + { + string path = Path.Join(this.WorkingDirectory, "layout.tm7"); + using FileStream stream = File.Create(path); + model.Save(stream); + return path; + } } }