From 0f1d85523b2c4be31a9573bcec34c08fe84005a4 Mon Sep 17 00:00:00 2001 From: Hacks4Snacks Date: Wed, 2 Sep 2026 17:12:08 -0500 Subject: [PATCH] improve layout handling so agent based tm7 creation is aware of boundaries --- build/Dockerfile | 9 +- build/Dockerfile.api | 16 +- docs/cli-reference.md | 41 +- docs/deployment.md | 21 +- .../Tm7ExportPreparer.cs | 12 + src/ThreatModelForge.Cli/CommandCatalog.cs | 2 +- src/ThreatModelForge.Cli/LayoutCommand.cs | 119 +++++- .../Model/Abstracts/LineElement.cs | 16 + src/ThreatModelForge.Editing/DiagramEditor.cs | 11 +- src/ThreatModelForge.Editing/DiagramLabels.cs | 367 ++++++++++++++++++ src/ThreatModelForge.Editing/DiagramLayout.cs | 357 +++++++++++++++-- src/ThreatModelForge.Editing/LabelOverlap.cs | 38 ++ src/ThreatModelForge.Editing/LayoutOptions.cs | 46 ++- .../Authoring/ManifestSupport.cs | 10 + .../Tm7ExportPreparerTest.cs | 111 ++++++ .../BoundaryAndLayoutTest.cs | 74 ++++ .../DiagramLabelsTest.cs | 237 +++++++++++ .../DiagramLayoutTest.cs | 89 ++++- 18 files changed, 1509 insertions(+), 67 deletions(-) create mode 100644 src/ThreatModelForge.Editing/DiagramLabels.cs create mode 100644 src/ThreatModelForge.Editing/LabelOverlap.cs create mode 100644 test/ThreatModelForge.Editing.Tests/DiagramLabelsTest.cs diff --git a/build/Dockerfile b/build/Dockerfile index f93836d..65e2790 100644 --- a/build/Dockerfile +++ b/build/Dockerfile @@ -4,18 +4,25 @@ # Build from the repo root: # docker build -f build/Dockerfile -t tmforge-cli . # docker buildx build -f build/Dockerfile --platform linux/amd64,linux/arm64 -t tmforge-cli . +# Behind a package mirror, point the restore at it: +# docker build -f build/Dockerfile --build-arg NUGET_FEED= -t tmforge-cli . # Build FROM mcr.microsoft.com/dotnet/sdk:10.0 AS build WORKDIR /src +# The NuGet v3 index to restore from. Defaults to nuget.org; override it on a network that reaches +# only an internal mirror, where the checked-in NuGet.config source is unreachable. +ARG NUGET_FEED=https://api.nuget.org/v3/index.json + # Copy repo-level MSBuild configuration first for better layer caching. COPY global.json NuGet.config Directory.Build.props Directory.Packages.props stylecop.json .editorconfig ./ COPY src/ ./src/ # Publish the CLI as a framework-dependent, portable (AnyCPU) app. It runs on whatever # architecture the base image is, so no per-RID build is needed here. -RUN dotnet publish src/ThreatModelForge.Cli/ThreatModelForge.Cli.csproj -c Release -o /app/tmforge +RUN dotnet publish src/ThreatModelForge.Cli/ThreatModelForge.Cli.csproj -c Release -o /app/tmforge \ + -p:RestoreSources="${NUGET_FEED}" # Runtime stage # Standard runtime image: it has a shell so the PATH wrapper script below works. diff --git a/build/Dockerfile.api b/build/Dockerfile.api index bf38088..c964155 100644 --- a/build/Dockerfile.api +++ b/build/Dockerfile.api @@ -6,6 +6,9 @@ # Build from the repo root: # docker build -f build/Dockerfile.api -t tmforge . # docker buildx build -f build/Dockerfile.api --platform linux/amd64,linux/arm64 -t tmforge . +# Behind package mirrors, point the restores at them: +# docker build -f build/Dockerfile.api --build-arg NUGET_FEED= \ +# --build-arg NPM_REGISTRY= -t tmforge . # Run it: # docker run --rm -p 8080:8080 tmforge # then open http://localhost:8080/ @@ -14,9 +17,14 @@ # SPA in a dedicated node stage instead and skip that step during publish (/p:BuildStudio=false). FROM mcr.microsoft.com/azurelinux/base/nodejs:24 AS spa WORKDIR /studio + +# The npm registry to install from. Defaults to the public one; override it on a network that +# reaches only an internal mirror. +ARG NPM_REGISTRY=https://registry.npmjs.org/ + # Install deps first (cached until the lockfile changes). COPY src/ThreatModelForge.Studio/package.json src/ThreatModelForge.Studio/package-lock.json ./ -RUN npm ci +RUN npm config set registry "${NPM_REGISTRY}" && npm ci COPY src/ThreatModelForge.Studio/ ./ RUN npm run build @@ -24,12 +32,16 @@ RUN npm run build FROM mcr.microsoft.com/dotnet/sdk:10.0 AS build WORKDIR /src +# The NuGet v3 index to restore from. See NPM_REGISTRY above. +ARG NUGET_FEED=https://api.nuget.org/v3/index.json + COPY global.json NuGet.config Directory.Build.props Directory.Packages.props stylecop.json .editorconfig ./ COPY src/ ./src/ # Drop the pre-built SPA into the API's wwwroot, then publish without re-running the npm step. COPY --from=spa /studio/dist/ ./src/ThreatModelForge.Api/wwwroot/ -RUN dotnet publish src/ThreatModelForge.Api/ThreatModelForge.Api.csproj -c Release -o /app /p:BuildStudio=false +RUN dotnet publish src/ThreatModelForge.Api/ThreatModelForge.Api.csproj -c Release -o /app \ + -p:BuildStudio=false -p:RestoreSources="${NUGET_FEED}" # runtime FROM mcr.microsoft.com/dotnet/aspnet:10.0 AS runtime diff --git a/docs/cli-reference.md b/docs/cli-reference.md index bc0e7a9..4534f59 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -44,7 +44,7 @@ tmforge [options] | [`rename`](#rename) | Author | Rename an element. | | [`set`](#set) | Author | Set an element/flow's name or properties. | | [`page`](#page) | Author | List, add, rename, reorder, or remove pages (diagrams). | -| [`layout`](#layout) | Author | Auto-lay-out the diagram (layered; no hand-placed coordinates). | +| [`layout`](#layout) | Author | Auto-lay-out the diagram (layered, boundary aware; places flow labels). | | [`rules`](#rules) | Analyze | Compile an MTMT `.tb7` template into a versioned rule pack. | | [`analyze`](#analyze) | Analyze | Evaluate the analysis rules against a model. | | [`analysis`](#analysis) | Analyze | Validate a stored analysis document (and check whether it is stale), or compare two of them. | @@ -511,19 +511,38 @@ tmforge page reorder payments.tm7 --page "Payments service" --to 1 ### `layout` Apply a deterministic **layered auto-layout** so you never hand-place coordinates: components are -arranged left-to-right by their data flows and connectors are re-routed. Trust boundaries are left in -place, so run this to tidy a graph (it arranges the data-flow graph rather than preserving boundary -placement). +arranged left-to-right by their data flows, connectors are re-routed, and flow labels are placed +clear of the shapes and of one another. + +Layout is **trust-boundary aware**. Every component keeps the boundary it was inside, each boundary +is resized around exactly the members it holds, and the boundaries are then arranged by the flows +between them. That matters because boundary membership is what the analysis is derived from: an +arrangement that moved a component out of its boundary would change what the model means, not just +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. ```text -tmforge layout [--page ] [--node-spacing ] [--layer-spacing ] [--json] +tmforge layout [--page ] [--node-spacing ] [--layer-spacing ] [--labels] [--check] [--json] ``` +| Option | Meaning | +| --- | --- | +| `--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. | + ```bash tmforge layout payments.tm7 tmforge layout payments.tm7 --node-spacing 60 --layer-spacing 120 +tmforge layout payments.tm7 --labels # keep the authored geometry, fix the labels +tmforge layout payments.tm7 --check --json # gate on legibility ``` +A flow's name is drawn as a **single unwrapped line** centred on its connector, so a long name needs +far more room than the flow it names — at the tool's default font a fifty-character name is wider +than a typical trust boundary. `tmforge apply` places labels as well as it can, but placement cannot +shorten text: when `--check` still reports overlaps, the remedy is shorter flow names (with the +sentence moved into a property such as `Description`) or fewer objects per page. + --- ## Analysis, reporting & conversion @@ -867,6 +886,12 @@ A `.tm7` target embeds the Threat Model Forge knowledge base by default so the f `--knowledge-base ` to embed a specific one instead. A knowledge base already present in the source model (for example, a file authored in the tool) is preserved. +A `.tm7` target also gets its flow labels placed, because the tool draws each flow's name on its +connector and most source formats carry no label position at all — `tmforge-json`, the canonical wire +model Studio and the API exchange, records only a flow's endpoints and name. Without this, every label +converted from one of them would land on its connector's midpoint and flows sharing a pair of +endpoints would print their names on top of each other. A label the source did position is preserved. + ```bash tmforge convert payments.tm7 --to drawio --out payments.drawio tmforge convert payments.drawio --to tm7 --out payments.tm7 @@ -1003,6 +1028,12 @@ tmforge apply model.json --out model.tm7 tmforge apply model.json --dry-run ``` +Shapes go exactly where the manifest asks, but a flow's label has no coordinates to declare: the tool +draws the name on the connector itself, so two flows between one pair of elements would print their +names on the same spot. Every write to `.tm7` therefore places the labels for you, adjusting only the +connectors' curve handles — nothing the analysis reads, and never a label somebody has already moved. +Run [`tmforge layout --check`](#layout) on the result to confirm none is still covered. + A manifest is a model's *source*, not a model. The read-only verbs (`open`, `list`, `show`, `analyze`, …) take a model file, so pointing one at a manifest reports that and names the `apply` command to run first. The same recognition lets **Studio** open a manifest directly — see diff --git a/docs/deployment.md b/docs/deployment.md index 3b61e53..a130008 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -29,6 +29,23 @@ docker build -f build/Dockerfile.api -t tmforge . docker run --rm -p 8080:8080 tmforge # -> http://localhost:8080/ ``` +### Building behind a package mirror + +Both image builds restore from the public package feeds by default. On a network that reaches only +an internal mirror, point them at it — the build needs no other change: + +```bash +docker build -f build/Dockerfile.api \ + --build-arg NUGET_FEED=https:///nuget/v3/index.json \ + --build-arg NPM_REGISTRY=https:///npm/ \ + -t tmforge . +``` + +| Build argument | Default | Used by | +| --- | --- | --- | +| `NUGET_FEED` | `https://api.nuget.org/v3/index.json` | Both images; overrides the source in `NuGet.config`. | +| `NPM_REGISTRY` | `https://registry.npmjs.org/` | `Dockerfile.api` only, for the Studio SPA. | + - Studio: `http://localhost:8080/` - API: `http://localhost:8080/v1/...` - OpenAPI: `http://localhost:8080/openapi/v1.json` @@ -197,7 +214,9 @@ docker build -f build/Dockerfile -t tmforge-cli . docker run --rm -v "$PWD:/work" tmforge-cli tmforge analyze model.tm7 ``` -The image mounts your files at `/work`, so paths in your commands are relative to it. +The image mounts your files at `/work`, so paths in your commands are relative to it. Behind a +package mirror, pass `--build-arg NUGET_FEED=` — see +[Building behind a package mirror](#building-behind-a-package-mirror). ## CI/CD diff --git a/src/ThreatModelForge.Analysis/Tm7ExportPreparer.cs b/src/ThreatModelForge.Analysis/Tm7ExportPreparer.cs index 3800d31..991cdd3 100644 --- a/src/ThreatModelForge.Analysis/Tm7ExportPreparer.cs +++ b/src/ThreatModelForge.Analysis/Tm7ExportPreparer.cs @@ -77,6 +77,18 @@ public static void Prepare(ThreatModel model, RuleSet ruleSet) throw new ArgumentNullException(nameof(ruleSet)); } + // Give any flow label that has no recorded position one, before the shift below, so the + // normalization accounts for where the labels actually ended up. The tool prints a flow's + // name on its connector, and a model arriving from a format that carries no connector + // geometry — the canonical tmforge-json the API and Studio speak — has no positions at + // all, so every label would otherwise land on its connector's midpoint and flows sharing a + // pair of endpoints would print their names on top of each other. A label an author placed + // is left exactly where it is. + foreach (DrawingSurfaceModel surface in model.DrawingSurfaceList) + { + DiagramLabels.DeconflictUnplaced(surface); + } + // Shift each surface so no element sits below the tool's minimum drawing coordinate. This is // independent of the knowledge base, so it runs before the foreign-knowledge-base short // circuit below. diff --git a/src/ThreatModelForge.Cli/CommandCatalog.cs b/src/ThreatModelForge.Cli/CommandCatalog.cs index 7c3b1d6..4627bfd 100644 --- a/src/ThreatModelForge.Cli/CommandCatalog.cs +++ b/src/ThreatModelForge.Cli/CommandCatalog.cs @@ -29,7 +29,7 @@ internal static class CommandCatalog new CommandInfo("rename", "Rename an element.", "id, name", RenameCommand.Run), new CommandInfo("set", "Set an element/flow's name or properties (protocol, port, auth, ...).", "id, name, properties{}", SetCommand.Run), new CommandInfo("page", "List, add, rename, reorder, or remove pages (diagrams).", "ls: count, items[]; add: index, name, id; rename: id, name; rm: id, name, remaining; reorder: id, name, index", PageCommand.Run), - new CommandInfo("layout", "Auto-lay-out the diagram (layered; no hand-placed coordinates).", "pages, components", LayoutCommand.Run), + new CommandInfo("layout", "Auto-lay-out the diagram (layered, boundary aware; --labels places only the flow labels, --check reports obstructed ones).", "pages, components, labelsMoved, labelOverlaps; with --check also overlaps[]{flow,obstructedBy,kind,area}", LayoutCommand.Run), new CommandInfo("rules", "Compile MTMT templates into versioned analysis rule packs.", "operation,input,output,strict,status,packId,packName,sourceCount,emittedCount,skippedCount,warningCount,categoryDistribution{},diagnostics[]", RulesCommand.Run), new CommandInfo("analyze", "Analyze a threat model against its analysis rules.", "a SARIF-style model report (runs[].results[]); see docs/cli-reference.md", AnalyzeCommand.Run), new CommandInfo("analysis", "Validate a stored tmforge-analysis document (optionally against its model).", "operation, path, status, stale, schemaVersion, findingCount, problems[]", AnalysisCommand.Run), diff --git a/src/ThreatModelForge.Cli/LayoutCommand.cs b/src/ThreatModelForge.Cli/LayoutCommand.cs index 2e59af3..a8802fd 100644 --- a/src/ThreatModelForge.Cli/LayoutCommand.cs +++ b/src/ThreatModelForge.Cli/LayoutCommand.cs @@ -14,8 +14,10 @@ namespace ThreatModelForge.Cli /// /// Implements tmforge layout: applies a deterministic, dependency-free layered auto-layout /// to a model's pages, so an author never has to hand-place coordinates. Components are arranged - /// left-to-right by their data flows and connectors are re-routed; trust boundaries are left where - /// they are (so this arranges the data-flow graph rather than preserving boundary placement). + /// left-to-right by their data flows inside the trust boundary they already belong to, boundaries + /// are resized around their members, and flow labels are placed clear of the shapes and of each + /// other. --labels restricts it to the labels, which is what a model with hand-placed or + /// manifest-declared geometry wants. /// internal static class LayoutCommand { @@ -32,13 +34,15 @@ public static int Run(string[] args) return 1; } - CliArgs parsed = CliArgs.Parse(args, new[] { "node-spacing", "layer-spacing", "page" }); + CliArgs parsed = CliArgs.Parse(args, new[] { "node-spacing", "layer-spacing", "page" }, new[] { "labels", "check" }); if (parsed.Help) { PrintUsage(); return 0; } + bool labelsOnly = parsed.HasFlag("labels"); + bool check = parsed.HasFlag("check"); if (parsed.UnknownFlags.Count > 0) { Console.Error.WriteLine("Unknown option: " + parsed.UnknownFlags[0]); @@ -60,7 +64,7 @@ public static int Run(string[] args) } (ThreatModel model, IThreatModelFormat? format) = CliModelLoader.Load(input!); - if (format == null || !format.Capabilities.CanWrite) + if (format == null || (!check && !format.Capabilities.CanWrite)) { Console.Error.WriteLine("The model's format does not support writing."); return 1; @@ -94,26 +98,120 @@ public static int Run(string[] args) } int components = 0; + int labelled = 0; + List overlaps = new List(); foreach (DrawingSurfaceModel diagram in targets) { components += diagram.Borders.Values.OfType().Count(element => !(element is BorderBoundary)); - DiagramLayout.Apply(diagram, options); + if (check) + { + overlaps.AddRange(DiagramLabels.Inspect(diagram, options)); + continue; + } + + if (labelsOnly) + { + labelled += DiagramLabels.Deconflict(diagram, options); + } + else + { + labelled += DiagramLayout.Apply(diagram, options); + } + + overlaps.AddRange(DiagramLabels.Inspect(diagram, options)); + } + + if (check) + { + return Report(parsed, targets.Count, components, overlaps); } AuthoringSupport.Save(model, input!, format); if (parsed.Json) { - CliJson.WriteEnvelope("layout", new { pages = targets.Count, components }); + CliJson.WriteEnvelope("layout", new + { + pages = targets.Count, + components, + labelsMoved = labelled, + labelOverlaps = overlaps.Count, + }); } else { - Console.Error.WriteLine("Laid out " + components + " component(s) across " + targets.Count + " page(s) in " + input + "."); + Console.Error.WriteLine((labelsOnly + ? "Placed " + labelled + " flow label(s)" + : "Laid out " + components + " component(s)") + + " across " + targets.Count + " page(s) in " + input + "."); + WarnAboutOverlaps(overlaps); } 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. + /// + /// The parsed arguments. + /// The number of pages inspected. + /// The number of components inspected. + /// The collisions found. + /// Zero when the diagram is legible; one when it is not. + private static int Report(CliArgs parsed, int pages, int components, List overlaps) + { + if (parsed.Json) + { + CliJson.WriteEnvelope("layout", new + { + pages, + components, + labelOverlaps = overlaps.Count, + overlaps = overlaps.Select(overlap => new + { + flow = overlap.Flow, + obstructedBy = overlap.ObstructedBy, + kind = overlap.Kind, + area = overlap.Area, + }).ToList(), + }); + } + else if (overlaps.Count == 0) + { + Console.Error.WriteLine("No obstructed flow labels across " + pages + " page(s)."); + } + else + { + WarnAboutOverlaps(overlaps); + foreach (LabelOverlap overlap in overlaps) + { + Console.Error.WriteLine(" '" + overlap.Flow + "' is covered by the " + overlap.Kind + " '" + overlap.ObstructedBy + "'."); + } + } + + return overlaps.Count == 0 ? 0 : 1; + } + + /// + /// Warns that labels remain obstructed. Layout can move a label but not shorten it: a flow + /// name wider than the space between the elements it joins cannot be placed clear of them, so + /// the remedy is a shorter name (with the sentence moved into a property) or fewer objects per + /// page, and the caller is the only one who can choose. + /// + /// The collisions found. + private static void WarnAboutOverlaps(List overlaps) + { + if (overlaps.Count == 0) + { + return; + } + + Console.Error.WriteLine( + overlaps.Count + " flow label(s) are still covered. Flow names are drawn unwrapped, so a long " + + "name needs more room than the flow it names; shorten the names or split the page."); + } + private static bool TryGetInt(CliArgs parsed, string name, out int value) { string? raw = parsed.Get(name); @@ -128,11 +226,14 @@ private static bool TryGetInt(CliArgs parsed, string name, out int value) private static void PrintUsage() { - Console.Error.WriteLine("Auto-lay-out a model's pages (layered left-to-right; boundaries are left in place)."); + Console.Error.WriteLine("Auto-lay-out a model's pages (layered, trust-boundary aware, with flow labels placed)."); Console.Error.WriteLine("Usage:"); - Console.Error.WriteLine(" tmforge layout [--page ] [--node-spacing ] [--layer-spacing ] [--json] "); + Console.Error.WriteLine(" tmforge layout [--page ] [--node-spacing ] [--layer-spacing ] [--labels] [--check] [--json] "); 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(" --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.Core/Model/Abstracts/LineElement.cs b/src/ThreatModelForge.Core/Model/Abstracts/LineElement.cs index 9abe25c..83ebc6c 100644 --- a/src/ThreatModelForge.Core/Model/Abstracts/LineElement.cs +++ b/src/ThreatModelForge.Core/Model/Abstracts/LineElement.cs @@ -38,6 +38,22 @@ public int HandleY set => this.handleY = value; } + /// + /// Gets a value indicating whether the curve handle sits at the midpoint of the endpoints, + /// which is where it lies when nobody has moved it: either because none was ever recorded, or + /// because the midpoint was written eagerly when the line was created. + /// + /// + /// A caller that positions labels needs this to tell a deliberate placement from a default + /// one. The handle is both the curve's control point and the point the label is drawn on, so a + /// handle at the midpoint means a straight line carrying no placement intent, and a handle + /// anywhere else is somebody's decision. It is deliberately not a + /// — it describes the stored state rather than adding to it. + /// + public bool HandleIsAtMidpoint => (this.handleX == 0 && this.handleY == 0) + || (this.HandleX == (this.SourceX + this.TargetX) / 2 + && this.HandleY == (this.SourceY + this.TargetY) / 2); + /// /// Gets or sets the source connection port. It serializes as a StencilConnectionPort /// name (None when unset) because the Microsoft Threat Modeling Tool types this member diff --git a/src/ThreatModelForge.Editing/DiagramEditor.cs b/src/ThreatModelForge.Editing/DiagramEditor.cs index 5731c90..544b2d4 100644 --- a/src/ThreatModelForge.Editing/DiagramEditor.cs +++ b/src/ThreatModelForge.Editing/DiagramEditor.cs @@ -397,16 +397,7 @@ private static void MoveAttachedConnectors(DrawingSurfaceModel diagram, Guid nod } } - private static bool IsHandleAtMidpoint(LineElement line) - { - if (line.HandleX == 0 && line.HandleY == 0) - { - return true; - } - - return line.HandleX == (line.SourceX + line.TargetX) / 2 - && line.HandleY == (line.SourceY + line.TargetY) / 2; - } + private static bool IsHandleAtMidpoint(LineElement line) => line.HandleIsAtMidpoint; private static DrawingElement CreateStencil(StencilKind kind) { diff --git a/src/ThreatModelForge.Editing/DiagramLabels.cs b/src/ThreatModelForge.Editing/DiagramLabels.cs new file mode 100644 index 0000000..0b40205 --- /dev/null +++ b/src/ThreatModelForge.Editing/DiagramLabels.cs @@ -0,0 +1,367 @@ +namespace ThreatModelForge.Editing +{ + using System; + using System.Collections.Generic; + using System.Linq; + using ThreatModelForge.Model; + using ThreatModelForge.Model.Abstracts; + + /// + /// Deterministic placement of data-flow labels. A connector's name is drawn as a single unwrapped + /// line of text at the midpoint of its curve, so a long name occupies far more of the canvas than + /// the line it belongs to: at the Microsoft Threat Modeling Tool's default font a fifty-character + /// flow name is wider than a trust boundary. Nothing in the model records that, so a diagram whose + /// shapes are perfectly placed still renders as overlapping text, and two flows between the same + /// pair of elements get byte-identical label positions and print on top of each other. + /// + /// + /// This moves the curve handle only. Trust-boundary crossing — and therefore the whole analysis — + /// is derived from a connector's source and target endpoints, which are left untouched, so + /// relabelling a diagram cannot change what it means. The handle is chosen by sliding the label + /// along its own connector first, which leaves the drawn line straight because a control point on + /// the segment reparameterizes the curve without bending it, and only then by offsetting it into a + /// parallel lane, which bows the line away from whatever it was covering. + /// + public static class DiagramLabels + { + /// Fractions along the connector the label is allowed to slide to, nearest the middle first. + private static readonly double[] Positions = { 0.50, 0.40, 0.60, 0.32, 0.68, 0.26, 0.74 }; + + /// Lane offsets perpendicular to the connector, in lane widths, nearest first. + private static readonly int[] Lanes = { 0, -1, 1, -2, 2, -3, 3 }; + + /// + /// Chooses a legible position for every named data-flow label on a diagram, in place. The + /// result is a pure function of the diagram, so repeated runs are byte-identical. + /// + /// The diagram to relabel. + /// Label metrics, or for the defaults. + /// The number of connectors whose handle moved. + public static int Deconflict(DrawingSurfaceModel diagram, LayoutOptions? options = null) + { + return Place(diagram, options, onlyUnplaced: false); + } + + /// + /// Chooses a position for each named data-flow label that has none, leaving every label an + /// author already positioned exactly where it is and treating those as obstacles to avoid. + /// + /// + /// This is what a format export wants. A model arriving from the canonical tmforge-json — which + /// carries no connector geometry at all — has no label positions to preserve, so every label + /// needs one or they all pile onto their connectors' midpoints. A model loaded from a + /// hand-arranged .tm7 has them, and rewriting those would discard a person's work on + /// the way through a tool that was only asked to save the file. A label still sitting at its + /// connector's midpoint counts as unplaced: that is where every connector starts. + /// + /// The diagram to relabel. + /// Label metrics, or for the defaults. + /// The number of labels that were given a position. + public static int DeconflictUnplaced(DrawingSurfaceModel diagram, LayoutOptions? options = null) + { + return Place(diagram, options, onlyUnplaced: true); + } + + /// + /// Reports every data-flow label that is covered by a shape or by another label, worst first. + /// Reads the diagram without modifying it, so a caller can gate on a diagram being legible + /// before it is published. + /// + /// The diagram to inspect. + /// Label metrics, or for the defaults. + /// The overlapping pairs; empty when every label is clear. + public static IReadOnlyList Inspect(DrawingSurfaceModel diagram, LayoutOptions? options = null) + { + if (diagram == null) + { + throw new ArgumentNullException(nameof(diagram)); + } + + LayoutOptions effective = options ?? new LayoutOptions(); + List connectors = NamedConnectors(diagram).ToList(); + List labels = connectors.Select(connector => LabelBox(connector, effective)).ToList(); + List overlaps = new List(); + + for (int index = 0; index < connectors.Count; index++) + { + foreach (DrawingElement shape in Shapes(diagram)) + { + int area = labels[index].Overlap(Rect.Of(shape)); + if (area > 0) + { + overlaps.Add(new LabelOverlap( + DiagramElementHelper.GetName(connectors[index]), + DiagramElementHelper.GetName(shape), + "element", + area)); + } + } + + for (int other = index + 1; other < connectors.Count; other++) + { + int area = labels[index].Overlap(labels[other]); + if (area > 0) + { + overlaps.Add(new LabelOverlap( + DiagramElementHelper.GetName(connectors[index]), + DiagramElementHelper.GetName(connectors[other]), + "flow", + area)); + } + } + } + + return overlaps + .OrderByDescending(overlap => overlap.Area) + .ThenBy(overlap => overlap.Flow, StringComparer.Ordinal) + .ThenBy(overlap => overlap.ObstructedBy, StringComparer.Ordinal) + .ToList(); + } + + /// + /// Gets the point a connector's label is drawn at: the midpoint of the quadratic curve through + /// its handle, which is what the renderer and the Microsoft Threat Modeling Tool both center + /// the text on. + /// + /// The connector. + /// The label's center point. + public static (int X, int Y) LabelCenter(LineElement line) + { + if (line == null) + { + throw new ArgumentNullException(nameof(line)); + } + + return ( + (line.SourceX + (2 * line.HandleX) + line.TargetX) / 4, + (line.SourceY + (2 * line.HandleY) + line.TargetY) / 4); + } + + private static int Place(DrawingSurfaceModel diagram, LayoutOptions? options, bool onlyUnplaced) + { + if (diagram == null) + { + throw new ArgumentNullException(nameof(diagram)); + } + + LayoutOptions effective = options ?? new LayoutOptions(); + List obstacles = ShapeRects(diagram); + List placed = new List(); + int moved = 0; + + List connectors = NamedConnectors(diagram).ToList(); + if (onlyUnplaced) + { + // A label somebody moved keeps its place, but still occupies it: a label being placed + // now has to avoid it, or honouring that choice would just move the collision. A handle + // at the endpoint midpoint is the default every connector is created with, so it counts + // as unplaced rather than as a decision to leave the label there. + foreach (Connector positioned in connectors.Where(connector => !connector.HandleIsAtMidpoint)) + { + placed.Add(LabelBox(positioned, effective)); + } + + connectors = connectors.Where(connector => connector.HandleIsAtMidpoint).ToList(); + } + + foreach (Connector connector in connectors) + { + Rect label = LabelBox(connector, effective); + Rect chosen = label; + long best = long.MaxValue; + + foreach ((int X, int Y) candidate in Candidates(connector, effective)) + { + Rect option = label.CenteredOn(candidate.X, candidate.Y); + long cost = Cost(option, obstacles, placed); + if (cost < best) + { + best = cost; + chosen = option; + if (cost == 0) + { + break; + } + } + } + + placed.Add(chosen); + if (SetLabelCenter(connector, chosen.CenterX, chosen.CenterY)) + { + moved++; + } + } + + return moved; + } + + /// Moves a connector's handle so its label is drawn centered on a point. + /// The connector. + /// The x coordinate the label should center on. + /// The y coordinate the label should center on. + /// when the handle moved. + private static bool SetLabelCenter(LineElement line, int x, int y) + { + int handleX = ((4 * x) - line.SourceX - line.TargetX) / 2; + int handleY = ((4 * y) - line.SourceY - line.TargetY) / 2; + + // A handle stored as zero means "unset" and reads back as the endpoint midpoint, which + // would silently discard the placement. One unit off is invisible and unambiguous. + handleX = handleX == 0 ? 1 : handleX; + handleY = handleY == 0 ? 1 : handleY; + + if (line.HandleX == handleX && line.HandleY == handleY) + { + return false; + } + + line.HandleX = handleX; + line.HandleY = handleY; + return true; + } + + /// + /// Enumerates the positions a label may take, in order of increasing disturbance: the natural + /// midpoint first, then slid along the connector, then pushed into parallel lanes. + /// + /// The connector being labelled. + /// The label metrics. + /// The candidate label centers. + private static IEnumerable<(int X, int Y)> Candidates(Connector connector, LayoutOptions options) + { + double deltaX = connector.TargetX - connector.SourceX; + double deltaY = connector.TargetY - connector.SourceY; + double length = Math.Sqrt((deltaX * deltaX) + (deltaY * deltaY)); + + // A connector between two coincident points has no direction to slide or offset along; + // lanes are stacked straight up so at least the labels do not print on one another. + (double NormalX, double NormalY) normal = length <= 0.0 + ? (0.0, -1.0) + : (-deltaY / length, deltaX / length); + + int lanes = Math.Max(0, options.LabelLanes); + foreach (int lane in Lanes) + { + if (Math.Abs(lane) > lanes) + { + continue; + } + + double offset = lane * options.LabelLaneSpacing; + foreach (double position in Positions) + { + double x = connector.SourceX + (deltaX * position) + (normal.NormalX * offset); + double y = connector.SourceY + (deltaY * position) + (normal.NormalY * offset); + yield return ((int)Math.Round(x), (int)Math.Round(y)); + } + } + } + + /// Scores a candidate by how much of it is covered; zero means clear. + /// The label rectangle being scored. + /// The shapes on the diagram. + /// The labels already positioned. + /// The total covered area. + private static long Cost(Rect candidate, List obstacles, List placed) + { + long cost = 0; + foreach (Rect obstacle in obstacles) + { + cost += candidate.Overlap(obstacle); + } + + foreach (Rect label in placed) + { + cost += candidate.Overlap(label); + } + + return cost; + } + + /// Builds the rectangle a connector's label currently occupies. + /// The connector. + /// The label metrics. + /// The label rectangle. + private static Rect LabelBox(Connector connector, LayoutOptions options) + { + int width = Math.Max( + options.LabelCharacterWidth, + DiagramElementHelper.GetName(connector).Length * options.LabelCharacterWidth); + (int x, int y) = LabelCenter(connector); + return new Rect(x - (width / 2), y - (options.LabelHeight / 2), width, options.LabelHeight); + } + + /// + /// Gets the named connectors of a diagram in identifier order, so placement does not depend on + /// dictionary enumeration order. An unnamed connector draws no text and is skipped. + /// + /// The diagram. + /// The connectors to label. + private static IEnumerable NamedConnectors(DrawingSurfaceModel diagram) + { + return diagram.Lines.Values + .OfType() + .Where(connector => !string.IsNullOrWhiteSpace(DiagramElementHelper.GetName(connector))) + .OrderBy(connector => connector.Guid); + } + + /// + /// Gets the shapes a label must avoid. A trust boundary is excluded: it is a region that a + /// label legitimately sits inside, not something a label can be covered by. + /// + /// The diagram. + /// The shapes. + private static IEnumerable Shapes(DrawingSurfaceModel diagram) + { + return diagram.Borders.Values + .OfType() + .Where(element => !(element is BorderBoundary)) + .OrderBy(element => element.Guid); + } + + /// Gets the shape rectangles a label must avoid. + /// The diagram. + /// The rectangles. + private static List ShapeRects(DrawingSurfaceModel diagram) + { + return Shapes(diagram).Select(Rect.Of).ToList(); + } + + /// An axis-aligned rectangle in drawing coordinates. + private readonly struct Rect + { + public Rect(int x, int y, int width, int height) + { + this.X = x; + this.Y = y; + this.Width = width; + this.Height = height; + } + + public int X { get; } + + public int Y { get; } + + public int Width { get; } + + public int Height { get; } + + public int CenterX => this.X + (this.Width / 2); + + public int CenterY => this.Y + (this.Height / 2); + + public static Rect Of(DrawingElement element) + => new Rect(element.Left, element.Top, element.Width, element.Height); + + public Rect CenteredOn(int x, int y) + => new Rect(x - (this.Width / 2), y - (this.Height / 2), this.Width, this.Height); + + public int Overlap(Rect other) + { + int width = Math.Min(this.X + this.Width, other.X + other.Width) - Math.Max(this.X, other.X); + int height = Math.Min(this.Y + this.Height, other.Y + other.Height) - Math.Max(this.Y, other.Y); + return width > 0 && height > 0 ? width * height : 0; + } + } + } +} diff --git a/src/ThreatModelForge.Editing/DiagramLayout.cs b/src/ThreatModelForge.Editing/DiagramLayout.cs index 870170a..b2aa479 100644 --- a/src/ThreatModelForge.Editing/DiagramLayout.cs +++ b/src/ThreatModelForge.Editing/DiagramLayout.cs @@ -10,19 +10,38 @@ namespace ThreatModelForge.Editing /// Deterministic, dependency-free automatic layout for a diagram. Given a logical graph whose /// components may lack coordinates — for example, an agent- or CLI-authored model — it assigns /// readable, non-overlapping positions using a layered left-to-right placement derived from the - /// data-flow connectors, then re-routes connector endpoints to the element edges. The result is - /// a pure function of the input, so repeated runs are byte-identical, and every component ends - /// up at a distinct coordinate so geometry-dependent analysis (for example, boundary crossing) - /// has meaningful input. Trust boundaries are left where they are. + /// data-flow connectors, then re-routes connector endpoints to the element edges and places the + /// flow labels so they do not print over one another. The result is a pure function of the input, + /// so repeated runs are byte-identical. /// + /// + /// Layout is trust-boundary aware. Every component keeps the boundary it was inside, because a + /// boundary is not decoration: which boundaries a flow crosses is what the analysis is derived + /// from, so an arrangement that moved a component out of its boundary would silently change the + /// model's meaning rather than just its drawing. Members are arranged within their boundary, the + /// boundary is then sized to hold exactly them, and the boundaries themselves are arranged by the + /// flows between them. Columns wrap onto a new row rather than running off the canvas, because the + /// Microsoft Threat Modeling Tool's drawing surface is bounded and taller than it is wide. + /// + /// 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. + /// + /// public static class DiagramLayout { + /// The size given to a trust boundary that holds nothing. + private const int EmptyBoundarySize = 120; + /// /// Applies the layered auto-layout to the supplied diagram in place. /// /// The diagram to lay out. /// Spacing options, or for the defaults. - public static void Apply(DrawingSurfaceModel diagram, LayoutOptions? options = null) + /// The number of flow labels that were moved clear of an obstruction. + public static int Apply(DrawingSurfaceModel diagram, LayoutOptions? options = null) { if (diagram == null) { @@ -34,15 +53,18 @@ public static void Apply(DrawingSurfaceModel diagram, LayoutOptions? options = n List components = CollectComponents(diagram); if (components.Count == 0) { - return; + return 0; } HashSet componentGuids = new HashSet(components.Select(element => element.Guid)); List<(Guid Source, Guid Target)> edges = CollectEdges(diagram, componentGuids); - List> layers = AssignLayers(components, edges); - PlaceColumns(layers, effectiveOptions); + Group root = BuildGroups(diagram, components); + Measure(root, edges, effectiveOptions, effectiveOptions.MaxWidth); + Place(root, effectiveOptions.OriginX, effectiveOptions.OriginY, effectiveOptions); + RerouteConnectors(diagram, componentGuids); + return DiagramLabels.Deconflict(diagram, effectiveOptions); } private static List CollectComponents(DrawingSurfaceModel diagram) @@ -67,18 +89,191 @@ private static List CollectComponents(DrawingSurfaceModel diagra .ToList(); } - private static List> AssignLayers( - List components, - List<(Guid Source, Guid Target)> edges) + /// + /// Builds the containment tree the layout preserves: each component joins the innermost trust + /// boundary that currently holds it, and each boundary joins the innermost boundary that + /// currently holds it. Membership is read from the existing geometry because that is exactly + /// what the analyzer reads, so the arrangement cannot disagree with the analysis. + /// + /// The diagram. + /// The components to place. + /// The root group, whose members and children are the unbounded objects. + private static Group BuildGroups(DrawingSurfaceModel diagram, List components) + { + List boundaries = diagram.Borders.Values + .OfType() + .OrderBy(boundary => boundary.Guid) + .ToList(); + + Group root = new Group(null); + Dictionary groups = boundaries.ToDictionary( + boundary => boundary.Guid, + boundary => new Group(boundary)); + + foreach (BorderBoundary boundary in boundaries) + { + // Strictly larger, so two boundaries drawn on one rectangle cannot each claim the other + // as its parent and produce a cycle in the tree. + BorderBoundary? parent = boundaries + .Where(other => other.Guid != boundary.Guid + && Area(other) > Area(boundary) + && Contains(other, boundary.Left, boundary.Top) + && Contains(other, boundary.Left + boundary.Width, boundary.Top + boundary.Height)) + .OrderBy(Area) + .ThenBy(other => other.Guid) + .FirstOrDefault(); + + (parent == null ? root : groups[parent.Guid]).Children.Add(groups[boundary.Guid]); + } + + foreach (DrawingElement component in components) + { + BorderBoundary? host = boundaries + .Where(boundary => Contains( + boundary, + component.Left + (component.Width / 2), + component.Top + (component.Height / 2))) + .OrderBy(Area) + .ThenBy(boundary => boundary.Guid) + .FirstOrDefault(); + + (host == null ? root : groups[host.Guid]).Members.Add(component); + } + + return root; + } + + /// + /// Arranges a group's contents and records the size it needs, innermost group first. + /// + /// The group to measure. + /// Every data flow in the diagram, as component pairs. + /// The spacing options. + /// The width the group's contents may occupy before wrapping. + private static void Measure(Group group, List<(Guid Source, Guid Target)> edges, LayoutOptions options, int available) + { + int padding = group.Boundary == null ? 0 : options.BoundaryPadding; + int inner = Math.Max(EmptyBoundarySize, available - (2 * padding)); + + foreach (Group child in group.Children.OrderBy(child => child.Key)) + { + Measure(child, edges, options, inner); + } + + group.Cells.AddRange(group.Members.OrderBy(member => member.Guid).Select(Cell.For)); + group.Cells.AddRange(group.Children.OrderBy(child => child.Key).Select(Cell.For)); + foreach (Cell cell in group.Cells) + { + group.Reach.UnionWith(cell.Reach); + } + + if (group.Cells.Count == 0) + { + group.Width = group.Boundary == null ? 0 : EmptyBoundarySize; + group.Height = group.Boundary == null ? 0 : EmptyBoundarySize; + return; + } + + (int contentWidth, int contentHeight) = PlaceCells(group, edges, options, inner); + group.Width = contentWidth + (2 * padding); + group.Height = contentHeight + (2 * padding) + (group.Boundary == null ? 0 : options.BoundaryHeaderHeight); + } + + /// + /// Assigns each cell of a group an offset relative to the group's content box, by layering the + /// cells left to right along the flows between them and wrapping onto a new row when a column + /// run would exceed the available width. + /// + /// The group whose cells are being placed. + /// Every data flow in the diagram, as component pairs. + /// The spacing options. + /// The width the content may occupy before wrapping. + /// The size of the arranged content. + private static (int Width, int Height) PlaceCells( + Group group, + List<(Guid Source, Guid Target)> edges, + LayoutOptions options, + int available) + { + List> layers = AssignLayers(group.Cells, CellEdges(group, edges)); + + int contentWidth = 0; + int contentHeight = 0; + int columnX = 0; + int rowTop = 0; + int rowBottom = 0; + + foreach (List column in layers) + { + int columnWidth = column.Max(cell => cell.Width); + if (columnX > 0 && columnX + columnWidth > available) + { + columnX = 0; + rowTop = rowBottom + options.NodeSpacing; + } + + int y = rowTop; + foreach (Cell cell in column) + { + cell.OffsetX = columnX + ((columnWidth - cell.Width) / 2); + cell.OffsetY = y; + y += cell.Height + options.NodeSpacing; + contentWidth = Math.Max(contentWidth, cell.OffsetX + cell.Width); + contentHeight = Math.Max(contentHeight, cell.OffsetY + cell.Height); + } + + rowBottom = Math.Max(rowBottom, y - options.NodeSpacing); + columnX += columnWidth + options.LayerSpacing; + } + + return (contentWidth, contentHeight); + } + + /// + /// Projects the diagram's flows onto a group's own cells: a flow between two components in + /// different cells of this group orders those cells, and one that stays inside a single cell + /// is that cell's own business. + /// + /// The group. + /// Every data flow in the diagram, as component pairs. + /// The distinct ordered cell pairs. + private static List<(int Source, int Target)> CellEdges(Group group, List<(Guid Source, Guid Target)> edges) + { + Dictionary owner = new Dictionary(); + for (int index = 0; index < group.Cells.Count; index++) + { + foreach (Guid component in group.Cells[index].Reach) + { + owner[component] = index; + } + } + + HashSet<(int Source, int Target)> seen = new HashSet<(int Source, int Target)>(); + List<(int Source, int Target)> projected = new List<(int Source, int Target)>(); + foreach ((Guid source, Guid target) in edges) + { + if (owner.TryGetValue(source, out int from) && + owner.TryGetValue(target, out int to) && + from != to && + seen.Add((from, to))) + { + projected.Add((from, to)); + } + } + + return projected; + } + + private static List> AssignLayers(List cells, List<(int Source, int Target)> edges) { // Longest-path layering by bounded relaxation: layer[target] = max(layer[source] + 1). // The pass count is capped at the node count so a cyclic graph terminates deterministically // instead of relaxing forever. - Dictionary layerOf = components.ToDictionary(element => element.Guid, _ => 0); - for (int pass = 0; pass < components.Count; pass++) + int[] layerOf = new int[cells.Count]; + for (int pass = 0; pass < cells.Count; pass++) { bool changed = false; - foreach ((Guid source, Guid target) in edges) + foreach ((int source, int target) in edges) { int candidate = layerOf[source] + 1; if (candidate > layerOf[target]) @@ -94,28 +289,47 @@ private static List> AssignLayers( } } - return components - .GroupBy(element => layerOf[element.Guid]) - .OrderBy(group => group.Key) - .Select(group => group.OrderBy(element => element.Guid).ToList()) + return cells + .Select((cell, index) => (Cell: cell, Layer: layerOf[index])) + .GroupBy(entry => entry.Layer) + .OrderBy(layer => layer.Key) + .Select(layer => layer.Select(entry => entry.Cell).OrderBy(cell => cell.Key).ToList()) .ToList(); } - private static void PlaceColumns(List> layers, LayoutOptions options) + /// + /// Writes a measured group's arrangement to the diagram, outermost group first. + /// + /// The measured group. + /// The left edge the group occupies. + /// The top edge the group occupies. + /// The spacing options. + private static void Place(Group group, int x, int y, LayoutOptions options) { - int columnX = options.OriginX; - foreach (List column in layers) + if (group.Boundary != null) { - int columnWidth = column.Max(element => element.Width); - int y = options.OriginY; - foreach (DrawingElement element in column) + group.Boundary.Left = x; + group.Boundary.Top = y; + group.Boundary.Width = group.Width; + group.Boundary.Height = group.Height; + } + + int padding = group.Boundary == null ? 0 : options.BoundaryPadding; + int header = group.Boundary == null ? 0 : options.BoundaryHeaderHeight; + int innerX = x + padding; + int innerY = y + padding + header; + + foreach (Cell cell in group.Cells) + { + if (cell.Element != null) { - element.Left = columnX + ((columnWidth - element.Width) / 2); - element.Top = y; - y += element.Height + options.NodeSpacing; + cell.Element.Left = innerX + cell.OffsetX; + cell.Element.Top = innerY + cell.OffsetY; + } + else + { + Place(cell.Child!, innerX + cell.OffsetX, innerY + cell.OffsetY, options); } - - columnX += columnWidth + options.LayerSpacing; } } @@ -141,5 +355,90 @@ private static void RerouteConnectors(DrawingSurfaceModel diagram, HashSet 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) + => x >= outer.Left && x <= outer.Left + outer.Width + && y >= outer.Top && y <= outer.Top + outer.Height; + + /// A trust boundary and everything drawn inside it, arranged as a unit. + private sealed class Group + { + public Group(BorderBoundary? boundary) + { + this.Boundary = boundary; + } + + /// Gets the boundary this group draws, or for the page itself. + public BorderBoundary? Boundary { get; } + + /// Gets the components directly inside this boundary. + public List Members { get; } = new List(); + + /// Gets the boundaries nested directly inside this one. + public List Children { get; } = new List(); + + /// Gets the arranged contents, in placement order. + public List Cells { get; } = new List(); + + /// Gets every component in this group's subtree. + public HashSet Reach { get; } = new HashSet(); + + /// Gets the identifier this group sorts by. + public Guid Key => this.Boundary?.Guid ?? Guid.Empty; + + /// Gets or sets the measured width. + public int Width { get; set; } + + /// Gets or sets the measured height. + public int Height { get; set; } + } + + /// One arranged item of a group: either a component or a nested boundary. + private sealed class Cell + { + private Cell(DrawingElement? element, Group? child) + { + this.Element = element; + this.Child = child; + } + + /// Gets the component this cell holds, or when it holds a boundary. + public DrawingElement? Element { get; } + + /// Gets the nested boundary this cell holds, or when it holds a component. + public Group? Child { get; } + + /// Gets or sets the left offset within the owning group's content box. + public int OffsetX { get; set; } + + /// Gets or sets the top offset within the owning group's content box. + public int OffsetY { get; set; } + + /// Gets the width this cell occupies. + public int Width => this.Element?.Width ?? this.Child!.Width; + + /// Gets the height this cell occupies. + public int Height => this.Element?.Height ?? this.Child!.Height; + + /// Gets the identifier this cell sorts by. + public Guid Key => this.Element?.Guid ?? this.Child!.Key; + + /// Gets every component this cell contains. + public IEnumerable Reach => this.Element != null + ? new[] { this.Element.Guid } + : this.Child!.Reach; + + /// Creates a cell for a component. + /// The component. + /// The cell. + public static Cell For(DrawingElement element) => new Cell(element, null); + + /// Creates a cell for a nested boundary. + /// The nested group. + /// The cell. + public static Cell For(Group child) => new Cell(null, child); + } } } diff --git a/src/ThreatModelForge.Editing/LabelOverlap.cs b/src/ThreatModelForge.Editing/LabelOverlap.cs new file mode 100644 index 0000000..3e701c3 --- /dev/null +++ b/src/ThreatModelForge.Editing/LabelOverlap.cs @@ -0,0 +1,38 @@ +namespace ThreatModelForge.Editing +{ + using System; + + /// + /// One pair of drawing objects whose rendered text occupies the same space, reported by + /// . A diagram that reports none is legible in the Microsoft + /// Threat Modeling Tool; one that reports many is the smear of stacked text a large generated + /// model produces by default. + /// + public sealed class LabelOverlap + { + /// Initializes a new instance of the class. + /// The name of the flow whose label is obstructed. + /// The name of the object it collides with. + /// What the obstruction is: flow or element. + /// The area of the intersection, in square drawing units. + public LabelOverlap(string flow, string obstructedBy, string kind, int area) + { + this.Flow = flow ?? throw new ArgumentNullException(nameof(flow)); + this.ObstructedBy = obstructedBy ?? throw new ArgumentNullException(nameof(obstructedBy)); + this.Kind = kind ?? throw new ArgumentNullException(nameof(kind)); + this.Area = area; + } + + /// Gets the name of the flow whose label is obstructed. + public string Flow { get; } + + /// Gets the name of the object obstructing it. + public string ObstructedBy { get; } + + /// Gets what the obstruction is: flow for another label, element for a shape. + public string Kind { get; } + + /// Gets the area of the intersection, in square drawing units. + public int Area { get; } + } +} diff --git a/src/ThreatModelForge.Editing/LayoutOptions.cs b/src/ThreatModelForge.Editing/LayoutOptions.cs index 997c9dd..b40facc 100644 --- a/src/ThreatModelForge.Editing/LayoutOptions.cs +++ b/src/ThreatModelForge.Editing/LayoutOptions.cs @@ -19,11 +19,53 @@ public sealed class LayoutOptions /// /// Gets or sets the horizontal gap between adjacent layers (columns). /// - public int LayerSpacing { get; set; } = 80; + public int LayerSpacing { get; set; } = 160; /// /// Gets or sets the vertical gap between adjacent nodes within a layer. /// - public int NodeSpacing { get; set; } = 40; + public int NodeSpacing { get; set; } = 48; + + /// + /// 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. + /// + public int MaxWidth { get; set; } = 1760; + + /// + /// Gets or sets the padding between a trust boundary's edge and the members inside it. + /// + public int BoundaryPadding { get; set; } = 24; + + /// + /// 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. + /// + public int BoundaryHeaderHeight { get; set; } = 24; + + /// + /// 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. + /// + public int LabelCharacterWidth { get; set; } = 7; + + /// + /// Gets or sets the height of a data-flow label. + /// + public int LabelHeight { get; set; } = 18; + + /// + /// Gets or sets the perpendicular distance between adjacent label lanes. + /// + public int LabelLaneSpacing { get; set; } = 26; + + /// + /// 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. + /// + public int LabelLanes { get; set; } = 3; } } diff --git a/src/ThreatModelForge.Engine/Authoring/ManifestSupport.cs b/src/ThreatModelForge.Engine/Authoring/ManifestSupport.cs index 2e4baef..9a14d75 100644 --- a/src/ThreatModelForge.Engine/Authoring/ManifestSupport.cs +++ b/src/ThreatModelForge.Engine/Authoring/ManifestSupport.cs @@ -455,6 +455,16 @@ public static bool Build(Manifest manifest, bool force, out ThreatModel model, o return false; } + // Shapes go exactly where the manifest asks, but a flow's label has no coordinates to + // declare: the tool draws it on the connector, so two flows between one pair of elements + // print their names on the same spot and a long name runs across whatever it passes over. + // Placing the labels is therefore the builder's job, not the author's. It moves only the + // curve handles, which nothing in the analysis reads. + foreach (PageContext page in pageOrder) + { + DiagramLabels.Deconflict(page.Surface); + } + summary = new ManifestSummary(boundaries.Count, elements.Count, flows.Count); return true; } diff --git a/test/ThreatModelForge.Analysis.Tests/Tm7ExportPreparerTest.cs b/test/ThreatModelForge.Analysis.Tests/Tm7ExportPreparerTest.cs index 12a7a05..eed5ad8 100644 --- a/test/ThreatModelForge.Analysis.Tests/Tm7ExportPreparerTest.cs +++ b/test/ThreatModelForge.Analysis.Tests/Tm7ExportPreparerTest.cs @@ -1,10 +1,12 @@ namespace ThreatModelForge.Analysis.Tests { using System; + using System.Collections.Generic; using System.Linq; using Microsoft.VisualStudio.TestTools.UnitTesting; using ThreatModelForge.KnowledgeBase; using ThreatModelForge.Model; + using ThreatModelForge.Model.Abstracts; /// /// Unit tests for . @@ -21,6 +23,60 @@ public void PrepareThrowsForNullModel() Assert.Throws(() => Tm7ExportPreparer.Prepare(null!)); } + /// + /// Verifies that flow labels with no recorded position are placed apart. Every write path to + /// the tool's format runs through here, so this is what stops a model that arrived over the + /// canonical tmforge-json — which carries no connector geometry — from drawing two flows' + /// names on the same point. + /// + [TestMethod] + public void PreparePlacesUnpositionedFlowLabels() + { + ThreatModel model = ModelWithStackedFlowLabels(); + DrawingSurfaceModel surface = model.DrawingSurfaceList[0]; + Assert.AreEqual(1, LabelCenters(surface).Distinct().Count(), "the defect under test is that both labels start on one point"); + + Tm7ExportPreparer.Prepare(model); + + Assert.AreEqual(2, LabelCenters(surface).Distinct().Count(), "each flow label needs its own position"); + } + + /// + /// Verifies that a label somebody positioned is carried through untouched. Saving a file is not + /// permission to re-arrange the diagram inside it. + /// + [TestMethod] + public void PreparePreservesPositionedFlowLabels() + { + ThreatModel model = ModelWithStackedFlowLabels(); + DrawingSurfaceModel surface = model.DrawingSurfaceList[0]; + Connector chosen = surface.Lines.Values.OfType().OrderBy(flow => flow.Guid).First(); + chosen.HandleX = 400; + chosen.HandleY = 260; + + Tm7ExportPreparer.Prepare(model); + + Assert.AreEqual(400, chosen.HandleX); + Assert.AreEqual(260, chosen.HandleY); + } + + /// + /// Verifies that re-preparing an already-prepared model leaves its label positions alone, so an + /// iterative authoring loop does not shuffle the diagram on every save. + /// + [TestMethod] + public void PrepareLeavesAlreadyPlacedLabelsAlone() + { + ThreatModel model = ModelWithStackedFlowLabels(); + DrawingSurfaceModel surface = model.DrawingSurfaceList[0]; + + Tm7ExportPreparer.Prepare(model); + List<(int X, int Y)> first = LabelCenters(surface).ToList(); + Tm7ExportPreparer.Prepare(model); + + CollectionAssert.AreEqual(first, LabelCenters(surface).ToList()); + } + /// /// Verifies that a model without a knowledge base gains the default one and has its /// schema-backed properties typed. @@ -429,11 +485,66 @@ private static ThreatModel ModelWithFlow(string key, string value) return model; } + /// + /// Builds a surface holding two named flows between one pair of elements. The canonical + /// tmforge-json the API and Studio exchange carries no connector geometry, so a model arriving + /// from it looks exactly like this: endpoints derived from the shapes and no label position, + /// which draws both names on the same point. + /// + /// The model. + private static ThreatModel ModelWithStackedFlowLabels() + { + DrawingSurfaceModel surface = new DrawingSurfaceModel { Guid = Guid.NewGuid() }; + Guid left = Guid.NewGuid(); + Guid right = Guid.NewGuid(); + surface.Borders[left] = new StencilEllipse { Guid = left, GenericTypeId = "GE.P", Left = 100, Top = 100, Width = 100, Height = 60 }; + surface.Borders[right] = new StencilEllipse { Guid = right, GenericTypeId = "GE.P", Left = 600, Top = 100, Width = 100, Height = 60 }; + + foreach ((string Name, Guid Source, Guid Target) flow in new[] + { + ("Credential material handed to the signer", left, right), + ("Signed material returned to the caller", right, left), + }) + { + Connector connector = new Connector + { + Guid = Guid.NewGuid(), + GenericTypeId = "GE.DF", + SourceGuid = flow.Source, + TargetGuid = flow.Target, + SourceX = 200, + SourceY = 130, + TargetX = 600, + TargetY = 130, + }; + connector.Properties.Add(new StringDisplayAttribute { DisplayName = "Name", Name = "Name", Value = flow.Name }); + surface.Lines[connector.Guid] = connector; + } + + ThreatModel model = new ThreatModel(); + model.DrawingSurfaceList.Add(surface); + return model; + } + private static Connector Flow(ThreatModel model) { return model.DrawingSurfaceList[0].Lines.Values.OfType().Single(); } + /// Gets the point each flow's label is drawn on, in a stable order. + /// The surface to read. + /// The label centers. + private static IEnumerable<(int X, int Y)> LabelCenters(DrawingSurfaceModel surface) + { + return surface.Lines.Values + .OfType() + .OrderBy(flow => flow.Guid) + .Select(flow => ( + (flow.SourceX + (2 * flow.HandleX) + flow.TargetX) / 4, + (flow.SourceY + (2 * flow.HandleY) + flow.TargetY) / 4)) + .ToList(); + } + private sealed class PriorityRule : Rule { /// Initializes a new instance of the class. diff --git a/test/ThreatModelForge.Cli.Tests/BoundaryAndLayoutTest.cs b/test/ThreatModelForge.Cli.Tests/BoundaryAndLayoutTest.cs index c449e05..008c610 100644 --- a/test/ThreatModelForge.Cli.Tests/BoundaryAndLayoutTest.cs +++ b/test/ThreatModelForge.Cli.Tests/BoundaryAndLayoutTest.cs @@ -99,6 +99,80 @@ public void LayoutArrangesComponents() Assert.AreNotEqual(first.Left, second.Left, "connected components should land in different layers (columns)"); } + /// + /// tmforge layout --labels places the flow labels without rearranging hand-placed + /// shapes, which is what a model whose geometry comes from a manifest needs. The whole surface + /// may still be translated to stay inside the tool's coordinate range, so what must hold is + /// that the shapes keep their positions relative to one another. + /// + [TestMethod] + public void LayoutLabelsLeavesShapesInPlace() + { + string path = this.NewModel(); + string a = AddElement("process", path, "A"); + string b = AddElement("process", path, "B"); + Capture(() => ConnectCommand.Run(new[] { path, "--source", a, "--target", b, "--name", "Credential material handed to the signer" })); + Capture(() => ConnectCommand.Run(new[] { path, "--source", b, "--target", a, "--name", "Signed material returned to the caller" })); + (DrawingElement beforeFirst, DrawingElement beforeSecond) = LoadPair(path, "A", "B"); + (int offsetX, int offsetY) = (beforeSecond.Left - beforeFirst.Left, beforeSecond.Top - beforeFirst.Top); + + (int exit, string stdout) = Capture(() => LayoutCommand.Run(new[] { path, "--labels", "--json" })); + + Assert.AreEqual(0, exit); + using JsonDocument document = JsonDocument.Parse(stdout); + Assert.AreEqual(0, document.RootElement.GetProperty("data").GetProperty("labelOverlaps").GetInt32()); + + (DrawingElement afterFirst, DrawingElement afterSecond) = LoadPair(path, "A", "B"); + Assert.AreEqual(offsetX, afterSecond.Left - afterFirst.Left, "shapes must keep their relative arrangement"); + Assert.AreEqual(offsetY, afterSecond.Top - afterFirst.Top, "shapes must keep their relative arrangement"); + } + + /// + /// tmforge layout --check fails and writes nothing when a label cannot be placed clear. + /// Placement can move a name but not shorten it, so a name with nowhere to go stays covered, + /// and a publishing gate has to be able to see that. + /// + [TestMethod] + public void LayoutCheckReportsObstructedLabelsWithoutWriting() + { + // A long name between two shapes, with a third covering every direction it could move to. + const string Boxed = + "{\"name\":\"Boxed\",\"elements\":[" + + "{\"alias\":\"a\",\"kind\":\"process\",\"name\":\"A\",\"x\":100,\"y\":200,\"width\":100,\"height\":60}," + + "{\"alias\":\"b\",\"kind\":\"process\",\"name\":\"B\",\"x\":700,\"y\":200,\"width\":100,\"height\":60}," + + "{\"alias\":\"c\",\"kind\":\"store\",\"name\":\"Covering store\",\"x\":210,\"y\":100,\"width\":480,\"height\":300}]," + + "\"flows\":[{\"from\":\"a\",\"to\":\"b\",\"name\":\"A long flow name with nowhere to go\"}]}"; + string manifest = Path.Join(this.WorkingDirectory, "boxed.tm.json"); + string path = Path.Join(this.WorkingDirectory, "boxed.tm7"); + File.WriteAllText(manifest, Boxed); + Assert.AreEqual(0, Capture(() => ApplyCommand.Run(new[] { manifest, "--out", path })).Exit); + string before = File.ReadAllText(path); + + (int exit, string stdout) = Capture(() => LayoutCommand.Run(new[] { path, "--check", "--json" })); + + Assert.AreEqual(1, exit, "an obstructed diagram must fail the check"); + using JsonDocument document = JsonDocument.Parse(stdout); + Assert.IsTrue(document.RootElement.GetProperty("data").GetProperty("labelOverlaps").GetInt32() > 0); + Assert.AreEqual(before, File.ReadAllText(path), "--check must not write the model"); + } + + /// + /// tmforge layout --check passes on a model the authoring verbs produced. Every write + /// path to the tool's format places the labels, so an authored model is legible on arrival + /// rather than only after someone remembers to run a layout pass. + /// + [TestMethod] + public void LayoutCheckPassesForAnAuthoredModel() + { + string path = this.NewModel(); + string a = AddElement("process", path, "A"); + string b = AddElement("process", path, "B"); + Capture(() => ConnectCommand.Run(new[] { path, "--source", a, "--target", b, "--name", "Credential material handed to the signer" })); + Capture(() => ConnectCommand.Run(new[] { path, "--source", b, "--target", a, "--name", "Signed material returned to the caller" })); + + Assert.AreEqual(0, Capture(() => LayoutCommand.Run(new[] { path, "--check" })).Exit); + } + private static string AddElement(string kind, string path, string name) { (int exit, string stdout) = Capture(() => AddCommand.Run(new[] { kind, path, "--name", name, "--json" })); diff --git a/test/ThreatModelForge.Editing.Tests/DiagramLabelsTest.cs b/test/ThreatModelForge.Editing.Tests/DiagramLabelsTest.cs new file mode 100644 index 0000000..46b6e8a --- /dev/null +++ b/test/ThreatModelForge.Editing.Tests/DiagramLabelsTest.cs @@ -0,0 +1,237 @@ +namespace ThreatModelForge.Editing.Tests +{ + using System; + using System.Collections.Generic; + using System.Linq; + using Microsoft.VisualStudio.TestTools.UnitTesting; + using ThreatModelForge.Editing; + using ThreatModelForge.Model; + using ThreatModelForge.Model.Abstracts; + + /// + /// Unit tests for . + /// + [TestClass] + public class DiagramLabelsTest + { + private static readonly Guid NodeA = new Guid("00000000-0000-0000-0000-0000000000a1"); + private static readonly Guid NodeB = new Guid("00000000-0000-0000-0000-0000000000b2"); + + /// + /// Verifies that two flows between the same pair of elements do not print their names on the + /// same spot, which is what the geometric midpoint gives them by default. + /// + [TestMethod] + public void DeconflictSeparatesTwoFlowsBetweenOnePair() + { + DrawingSurfaceModel diagram = BuildPair(); + Connector first = AddConnector(diagram, "11111111-0000-0000-0000-000000000001", NodeA, NodeB, "Request submitted for approval"); + Connector second = AddConnector(diagram, "22222222-0000-0000-0000-000000000002", NodeB, NodeA, "Approval result returned"); + Assert.AreEqual(DiagramLabels.LabelCenter(first), DiagramLabels.LabelCenter(second), "the defect under test is that both start in one place"); + + DiagramLabels.Deconflict(diagram); + + Assert.AreNotEqual(DiagramLabels.LabelCenter(first), DiagramLabels.LabelCenter(second)); + Assert.AreEqual(0, DiagramLabels.Inspect(diagram).Count(overlap => overlap.Kind == "flow")); + } + + /// + /// Verifies that a label is moved off an element it would otherwise be printed across. + /// + [TestMethod] + public void DeconflictMovesALabelOffAnElementItCovers() + { + DrawingSurfaceModel diagram = BuildPair(); + AddComponent(diagram, new Guid("00000000-0000-0000-0000-0000000000c3"), 260, 84); + Connector connector = AddConnector(diagram, "11111111-0000-0000-0000-000000000001", NodeA, NodeB, "Credential material handed to the signer"); + + Assert.IsTrue(DiagramLabels.Inspect(diagram).Count > 0, "the label must start out covered for this to test anything"); + + DiagramLabels.Deconflict(diagram); + + Assert.AreEqual(0, DiagramLabels.Inspect(diagram).Count); + Assert.IsNotNull(connector); + } + + /// + /// Verifies that placement is a pure function of the diagram, so repeated runs are identical. + /// + [TestMethod] + public void DeconflictIsDeterministic() + { + DrawingSurfaceModel first = BuildCrowdedDiagram(); + DrawingSurfaceModel second = BuildCrowdedDiagram(); + + DiagramLabels.Deconflict(first); + DiagramLabels.Deconflict(second); + + List<(int X, int Y)> left = Centers(first); + List<(int X, int Y)> right = Centers(second); + CollectionAssert.AreEqual(left, right); + } + + /// + /// Verifies that a label with nothing in its way is left exactly where the tool would draw it, + /// so a legible diagram is not perturbed. + /// + [TestMethod] + public void DeconflictLeavesAClearLabelInPlace() + { + DrawingSurfaceModel diagram = BuildPair(); + Connector connector = AddConnector(diagram, "11111111-0000-0000-0000-000000000001", NodeA, NodeB, "Ping"); + (int X, int Y) before = DiagramLabels.LabelCenter(connector); + + DiagramLabels.Deconflict(diagram); + + Assert.AreEqual(before, DiagramLabels.LabelCenter(connector)); + } + + /// + /// Verifies that source and target endpoints are untouched, because trust-boundary crossing — + /// and therefore the analysis — is derived from them. + /// + [TestMethod] + public void DeconflictLeavesConnectorEndpointsUntouched() + { + DrawingSurfaceModel diagram = BuildCrowdedDiagram(); + List<(int, int, int, int)> before = diagram.Lines.Values.OfType() + .OrderBy(connector => connector.Guid) + .Select(connector => (connector.SourceX, connector.SourceY, connector.TargetX, connector.TargetY)) + .ToList(); + + DiagramLabels.Deconflict(diagram); + + List<(int, int, int, int)> after = diagram.Lines.Values.OfType() + .OrderBy(connector => connector.Guid) + .Select(connector => (connector.SourceX, connector.SourceY, connector.TargetX, connector.TargetY)) + .ToList(); + CollectionAssert.AreEqual(before, after); + } + + /// + /// Verifies that an unnamed connector draws no text and so is not placed. + /// + [TestMethod] + public void DeconflictIgnoresUnnamedConnectors() + { + DrawingSurfaceModel diagram = BuildPair(); + AddConnector(diagram, "11111111-0000-0000-0000-000000000001", NodeA, NodeB, null); + AddConnector(diagram, "22222222-0000-0000-0000-000000000002", NodeB, NodeA, null); + + Assert.AreEqual(0, DiagramLabels.Deconflict(diagram)); + Assert.AreEqual(0, DiagramLabels.Inspect(diagram).Count); + } + + /// + /// Verifies that a label covered by an element is reported, with the element named. + /// + [TestMethod] + public void InspectReportsALabelCoveredByAnElement() + { + DrawingSurfaceModel diagram = BuildPair(); + DrawingElement blocker = AddComponent(diagram, new Guid("00000000-0000-0000-0000-0000000000c3"), 260, 84); + DiagramElementHelper.SetName(blocker, "Broker"); + AddConnector(diagram, "11111111-0000-0000-0000-000000000001", NodeA, NodeB, "Credential material handed to the signer"); + + IReadOnlyList overlaps = DiagramLabels.Inspect(diagram); + + Assert.AreEqual(1, overlaps.Count); + Assert.AreEqual("element", overlaps[0].Kind); + Assert.AreEqual("Broker", overlaps[0].ObstructedBy); + Assert.IsTrue(overlaps[0].Area > 0); + } + + /// + /// Verifies that a label sitting inside a trust boundary is not reported: a boundary is a + /// region a label legitimately sits in, not something that covers it. + /// + [TestMethod] + public void InspectIgnoresTrustBoundaries() + { + DrawingSurfaceModel diagram = BuildPair(); + Guid guid = Guid.NewGuid(); + diagram.Borders[guid] = new BorderBoundary { Guid = guid, Left = 0, Top = 0, Width = 600, Height = 400 }; + AddConnector(diagram, "11111111-0000-0000-0000-000000000001", NodeA, NodeB, "Credential material handed to the signer"); + + Assert.AreEqual(0, DiagramLabels.Inspect(diagram).Count); + } + + /// + /// Verifies that a null diagram throws rather than being silently ignored. + /// + [TestMethod] + public void ThrowsOnNullDiagram() + { + Assert.Throws(() => DiagramLabels.Deconflict(null!)); + Assert.Throws(() => DiagramLabels.Inspect(null!)); + } + + private static List<(int X, int Y)> Centers(DrawingSurfaceModel diagram) + { + return diagram.Lines.Values + .OfType() + .OrderBy(connector => connector.Guid) + .Select(DiagramLabels.LabelCenter) + .ToList(); + } + + private static DrawingElement AddComponent(DrawingSurfaceModel diagram, Guid guid, int left, int top) + { + StencilEllipse element = new StencilEllipse + { + Guid = guid, + TypeId = "GE.P", + GenericTypeId = "GE.P", + Left = left, + Top = top, + Width = 100, + Height = 60, + }; + diagram.Borders[guid] = element; + return element; + } + + private static Connector AddConnector(DrawingSurfaceModel diagram, string guid, Guid source, Guid target, string? name) + { + DrawingElement from = (DrawingElement)diagram.Borders[source]; + DrawingElement to = (DrawingElement)diagram.Borders[target]; + Connector connector = new Connector + { + Guid = new Guid(guid), + TypeId = "GE.DF", + SourceGuid = source, + TargetGuid = target, + SourceX = from.Left + from.Width, + SourceY = from.Top + (from.Height / 2), + TargetX = to.Left, + TargetY = to.Top + (to.Height / 2), + }; + + if (name != null) + { + DiagramElementHelper.SetName(connector, name); + } + + diagram.Lines[connector.Guid] = connector; + return connector; + } + + private static DrawingSurfaceModel BuildPair() + { + DrawingSurfaceModel diagram = new DrawingSurfaceModel { Guid = Guid.NewGuid(), Header = "Main" }; + AddComponent(diagram, NodeA, 40, 84); + AddComponent(diagram, NodeB, 480, 84); + return diagram; + } + + private static DrawingSurfaceModel BuildCrowdedDiagram() + { + DrawingSurfaceModel diagram = BuildPair(); + AddComponent(diagram, new Guid("00000000-0000-0000-0000-0000000000c3"), 260, 84); + AddConnector(diagram, "11111111-0000-0000-0000-000000000001", NodeA, NodeB, "Rotated encryption key delivered by the manager"); + AddConnector(diagram, "22222222-0000-0000-0000-000000000002", NodeB, NodeA, "Rotation policy consumed by the manager"); + AddConnector(diagram, "33333333-0000-0000-0000-000000000003", NodeA, NodeB, "Snapshot manifest published to the sync job"); + return diagram; + } + } +} diff --git a/test/ThreatModelForge.Editing.Tests/DiagramLayoutTest.cs b/test/ThreatModelForge.Editing.Tests/DiagramLayoutTest.cs index 8b378c7..d8dddaa 100644 --- a/test/ThreatModelForge.Editing.Tests/DiagramLayoutTest.cs +++ b/test/ThreatModelForge.Editing.Tests/DiagramLayoutTest.cs @@ -2,6 +2,7 @@ namespace ThreatModelForge.Editing.Tests { using System; using System.Collections.Generic; + using System.Globalization; using System.Linq; using Microsoft.VisualStudio.TestTools.UnitTesting; using ThreatModelForge.Editing; @@ -95,20 +96,86 @@ public void ApplyReroutesConnectorEndpointsToElementEdges() } /// - /// Verifies that trust boundaries are not moved by the component layout. + /// Verifies that a component stays inside the trust boundary it started in. Boundary + /// membership is what the analysis is derived from, so an arrangement that moved a component + /// out of its boundary would change the model's meaning rather than just its drawing. /// [TestMethod] - public void ApplyLeavesTrustBoundariesUntouched() + public void ApplyKeepsComponentsInsideTheirTrustBoundary() { DrawingSurfaceModel diagram = BuildChain(NodeA, NodeB); - Guid boundaryGuid = new Guid("00000000-0000-0000-0000-0000000000e5"); - BorderBoundary boundary = new BorderBoundary { Guid = boundaryGuid, Left = 500, Top = 600, Width = 300, Height = 200 }; - diagram.Borders[boundaryGuid] = boundary; + BorderBoundary boundary = AddBoundary(diagram, 500, 600, 300, 200); + DrawingElement member = Component(diagram, NodeA); + member.Left = 520; + member.Top = 620; DiagramLayout.Apply(diagram); - Assert.AreEqual(500, boundary.Left); - Assert.AreEqual(600, boundary.Top); + member = Component(diagram, NodeA); + Assert.IsTrue( + member.Left >= boundary.Left && member.Top >= boundary.Top + && member.Left + member.Width <= boundary.Left + boundary.Width + && member.Top + member.Height <= boundary.Top + boundary.Height, + "the member must still be inside its boundary"); + } + + /// + /// Verifies that a trust boundary is resized around its members instead of keeping a size that + /// no longer has anything to do with what it holds. + /// + [TestMethod] + public void ApplyResizesTrustBoundaryAroundItsMembers() + { + DrawingSurfaceModel diagram = BuildChain(NodeA, NodeB); + BorderBoundary boundary = AddBoundary(diagram, 500, 600, 900, 800); + Component(diagram, NodeA).Left = 520; + Component(diagram, NodeA).Top = 620; + + DiagramLayout.Apply(diagram); + + DrawingElement member = Component(diagram, NodeA); + Assert.IsTrue(boundary.Width < 900, "an oversized boundary should shrink to its one member"); + Assert.IsTrue(boundary.Width >= member.Width, "the boundary must still hold its member"); + Assert.IsTrue(boundary.Height >= member.Height, "the boundary must still hold its member"); + } + + /// + /// Verifies that two trust boundaries are placed apart, so a member cannot be read as + /// belonging to the wrong one. + /// + [TestMethod] + public void ApplySeparatesOverlappingTrustBoundaries() + { + DrawingSurfaceModel diagram = BuildChain(NodeA, NodeB); + BorderBoundary first = AddBoundary(diagram, 100, 100, 400, 400); + BorderBoundary second = AddBoundary(diagram, 150, 150, 400, 400); + Component(diagram, NodeA).Left = 120; + Component(diagram, NodeA).Top = 120; + Component(diagram, NodeB).Left = 400; + Component(diagram, NodeB).Top = 400; + + DiagramLayout.Apply(diagram); + + Assert.IsFalse(Overlaps(first, second), "boundaries must not overlap after layout"); + } + + /// + /// Verifies that a wide graph wraps onto a new row rather than running off the right-hand edge + /// of the tool's bounded drawing surface. + /// + [TestMethod] + public void ApplyWrapsColumnsWithinTheCanvasWidth() + { + Guid[] nodes = Enumerable.Range(1, 20) + .Select(index => new Guid("00000000-0000-0000-0000-0000000000" + index.ToString("x2", CultureInfo.InvariantCulture))) + .ToArray(); + DrawingSurfaceModel diagram = BuildChain(nodes); + LayoutOptions options = new LayoutOptions(); + + DiagramLayout.Apply(diagram, options); + + int right = diagram.Borders.Values.OfType().Max(element => element.Left + element.Width); + Assert.IsTrue(right <= options.OriginX + options.MaxWidth, "layout must stay within the canvas width, was " + right); } /// @@ -165,6 +232,14 @@ private static void AddComponent(DrawingSurfaceModel diagram, Guid guid) diagram.Borders[guid] = new StencilEllipse { Guid = guid, TypeId = "GE.P", GenericTypeId = "GE.P", Width = 100, Height = 60 }; } + private static BorderBoundary AddBoundary(DrawingSurfaceModel diagram, int left, int top, int width, int height) + { + Guid guid = Guid.NewGuid(); + BorderBoundary boundary = new BorderBoundary { Guid = guid, Left = left, Top = top, Width = width, Height = height }; + diagram.Borders[guid] = boundary; + return boundary; + } + private static void AddConnector(DrawingSurfaceModel diagram, Guid source, Guid target) { Guid guid = Guid.NewGuid();