diff --git a/doc/wiki/bpmn-workflows.md b/doc/wiki/bpmn-workflows.md index 46b0211fa..eb1283eea 100644 --- a/doc/wiki/bpmn-workflows.md +++ b/doc/wiki/bpmn-workflows.md @@ -129,7 +129,8 @@ whole-definition import and resets them from the document, same as it always has The `document` GET/PUT pair exists for Studio (W21, part of #7909): Studio holds a BPMN process as the library's own JSON payload, not as `.bpmn` XML, and editing it — binding a task (W11), moving a shape (W14) — has to write that JSON back through the same path `Import` uses, or the stored BPMN source drifts out of sync with the definition (see -`BpmnInterchangeDocumentService.SourceVersionCustomPropertyKey`) and `Export` starts refusing with `422`. +`BpmnInterchangeDocumentService.SourceVersionCustomPropertyKey` and `SourceGraphHashCustomPropertyKey`) and `Export` +starts refusing with `422`. The request and response bodies on both routes are the `bpmnDefinitions` document exactly as `Bpmn.Model` serializes it: property names as `Bpmn.Model`'s own `[JsonPropertyName]` attributes declare them, and any enum as its underlying @@ -142,7 +143,8 @@ imported with — recorded on the workflow definition the first time it is impor `document` `PUT`, so an edit to a multi-process document does not have to name the process again on every save. **The document `PUT` edits the BPMN document, not the whole definition.** Only the activity graph the newly bound -document produces and the `Bpmn:*` custom properties (`SourceXml`, `SourceVersion`, `SourceProcessId`) change; the +document produces and the `Bpmn:*` custom properties (`SourceXml`, `SourceVersion`, `SourceProcessId`, +`SourceGraphHash`) change; the definition's name, description, variables, inputs, outputs, outcomes, options, tool version and every other custom property are carried forward exactly as they stood before the `PUT`. This is what lets Studio's binding UX (elsa- studio#1001) save a binding change through this endpoint without silently resetting metadata the author set some @@ -166,19 +168,30 @@ everything the reader retained on import (foreign extension elements, foreign at DI layout). Instead, it returns exactly the document the most recent successful import stored. This has a real consequence for one kind of edit: **an edit made through Elsa's own designer, without going back through BPMN, is not reflected in what `Export` returns** — the graph moves on, but the stored source still describes the document as it -stood before that edit. +stood before that edit. Rather than silently returning that pre-edit document, `Export` and the document `GET` refuse +it as stale — see below. An edit made through the `document` `PUT` endpoint above is different: it re-imports, so the stored source and the graph move together, and **`Export` reflects the edit immediately afterward.** `Export` also refuses outright, with `422 Unprocessable Entity`, rather than silently returning a stale or wrong -document, in two situations: +document, in three situations: - The definition does not currently carry BPMN source — either it was never imported from BPMN, or a later save replaced its custom properties wholesale (BPMN source travels on the same `CustomProperties` dictionary a workflow edit can overwrite). - The definition has changed — by version — since the source was recorded, meaning the stored BPMN text no longer corresponds to the current definition. +- The definition's activity graph has changed since the source was recorded, even though its version has not. An + unpublished draft is saved in place (same row, same version), so a designer save that edits a bound activity's + inputs — as Studio's binding UX (elsa-studio#1001) does — moves the graph without moving the version, which the + version check above cannot see. `Import` also records a SHA-256 hash of the graph + (`BpmnInterchangeDocumentService.SourceGraphHashCustomPropertyKey`, `Bpmn:SourceGraphHash`) at the moment it stores + the source, and `Export`/the document `GET` refuse when the current graph's hash no longer matches it. A definition + imported before this marker existed carries no value for it and falls back to the version-only check, so it is not + refused just for predating the marker. One practical consequence: a document `GET` performed after a designer save + now returns `422` too — Studio has to re-import (or PUT a fresh document) rather than edit a document that no + longer describes the current graph. A missing `definitionId` returns `404 Not Found`. diff --git a/src/modules/Elsa.Bpmn.Interchange/Endpoints/Bpmn/Document/BpmnDocumentETag.cs b/src/modules/Elsa.Bpmn.Interchange/Endpoints/Bpmn/Document/BpmnDocumentETag.cs index 61435c101..b68f2a125 100644 --- a/src/modules/Elsa.Bpmn.Interchange/Endpoints/Bpmn/Document/BpmnDocumentETag.cs +++ b/src/modules/Elsa.Bpmn.Interchange/Endpoints/Bpmn/Document/BpmnDocumentETag.cs @@ -1,7 +1,5 @@ -using System.Buffers.Binary; using System.Globalization; using System.Security.Cryptography; -using System.Text; using Elsa.Bpmn.Interchange.Services; using Elsa.Extensions; using Elsa.Workflows.Management.Entities; @@ -41,29 +39,11 @@ internal static class BpmnDocumentETag var sourceXml = definition.CustomProperties.TryGetValue(BpmnInterchangeDocumentService.SourceXmlCustomPropertyKey, out var xml) ? xml : null; using var hash = IncrementalHash.CreateHash(HashAlgorithmName.SHA256); - Append(hash, definition.Id); - Append(hash, definition.Version.ToString(CultureInfo.InvariantCulture)); - Append(hash, sourceXml); - Append(hash, definition.StringData); + BpmnContentHash.AppendField(hash, definition.Id); + BpmnContentHash.AppendField(hash, definition.Version.ToString(CultureInfo.InvariantCulture)); + BpmnContentHash.AppendField(hash, sourceXml); + BpmnContentHash.AppendField(hash, definition.StringData); return $"\"{Convert.ToHexString(hash.GetHashAndReset())}\""; } - - private static void Append(IncrementalHash hash, string? value) - { - Span header = stackalloc byte[5]; - - if (value is null) - { - header[0] = 0; - hash.AppendData(header[..1]); - return; - } - - var bytes = Encoding.UTF8.GetBytes(value); - header[0] = 1; - BinaryPrimitives.WriteInt32BigEndian(header[1..], bytes.Length); - hash.AppendData(header); - hash.AppendData(bytes); - } } diff --git a/src/modules/Elsa.Bpmn.Interchange/Services/BpmnContentHash.cs b/src/modules/Elsa.Bpmn.Interchange/Services/BpmnContentHash.cs new file mode 100644 index 000000000..9ac0dfd6f --- /dev/null +++ b/src/modules/Elsa.Bpmn.Interchange/Services/BpmnContentHash.cs @@ -0,0 +1,47 @@ +using System.Buffers.Binary; +using System.Security.Cryptography; +using System.Text; + +namespace Elsa.Bpmn.Interchange.Services; + +/// +/// The one place a workflow definition's serialized activity graph +/// () is turned into a content hash, +/// shared by — which records the hash at import time under +/// to tell a designer-edited graph +/// apart from the one BPMN source was imported against — and +/// , so the two never disagree about how +/// the same field hashes. +/// +internal static class BpmnContentHash +{ + /// The hex-encoded SHA-256 of a workflow definition's serialized activity graph. + public static string OfGraph(string? stringData) + { + using var hash = IncrementalHash.CreateHash(HashAlgorithmName.SHA256); + AppendField(hash, stringData); + return Convert.ToHexString(hash.GetHashAndReset()); + } + + /// + /// Appends a length-prefixed UTF-8 encoding of to , marking a + /// null value distinctly from an empty one so no two different inputs feed the hash the same bytes. + /// + public static void AppendField(IncrementalHash hash, string? value) + { + Span header = stackalloc byte[5]; + + if (value is null) + { + header[0] = 0; + hash.AppendData(header[..1]); + return; + } + + var bytes = Encoding.UTF8.GetBytes(value); + header[0] = 1; + BinaryPrimitives.WriteInt32BigEndian(header[1..], bytes.Length); + hash.AppendData(header); + hash.AppendData(bytes); + } +} diff --git a/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs b/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs index 402af7383..9e403278b 100644 --- a/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs +++ b/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs @@ -57,7 +57,9 @@ namespace Elsa.Bpmn.Interchange.Services; /// while the stored BPMN text still describes the pre-edit document. /// records the definition's own version at the moment of import for exactly this: if the current version no longer /// matches, the stored source is stale, and exporting it would silently hand back a document that is not what the -/// caller has, which is worse than refusing. +/// caller has, which is worse than refusing. A version bump alone misses one case, though: an unpublished draft is +/// saved in place — same row, same version — so a designer save of the draft moves the graph without moving the +/// version. closes that gap with a content hash of the graph itself. /// /// /// A whole-definition import and a document edit disagree about what else gets replaced. @@ -66,9 +68,10 @@ namespace Elsa.Bpmn.Interchange.Services; /// inputs, outputs, outcomes, options, tool version, read-only flag and custom properties with what that model carries. /// is different: it edits the BPMN document of an existing definition, so /// it passes that definition to the shared import logic's preserveMetadataFrom parameter, which carries all -/// of the above onto the result unchanged. Either way, only the bound activity graph and the three custom properties +/// of the above onto the result unchanged. Either way, only the bound activity graph and the four custom properties /// this service owns (, , -/// ) come from the import itself. +/// , ) come from the +/// import itself. /// /// /// Capability refusal happens here, at import, not at BpmnGraph.Build. @@ -100,8 +103,9 @@ public sealed class BpmnInterchangeDocumentService( /// compares this against the definition's current version to tell a /// still-current source from a stale one. The version number is what itself /// already exposes for "has this definition changed", so this reuses it rather than inventing a second notion of - /// change (a content hash, a timestamp) that could disagree with the versioning the rest of the system already - /// uses. + /// change for the case a version bump alone catches: a publish, or any other save that assigns a new version. + /// It does not, on its own, catch an unpublished draft saved in place — same row, same version — which is what + /// is for. /// public const string SourceVersionCustomPropertyKey = "Bpmn:SourceVersion"; @@ -118,6 +122,28 @@ public sealed class BpmnInterchangeDocumentService( /// public const string SourceProcessIdCustomPropertyKey = "Bpmn:SourceProcessId"; + /// + /// The workflow definition custom property records a content hash of the definition's + /// serialized activity graph () under, at the moment it stores + /// . + /// + /// + /// An unpublished draft is saved in place — same row, same version — so a designer save of the draft that edits + /// a bound activity's inputs changes without changing + /// 's own Version, which alone + /// cannot tell apart from no change at all. This marker closes that gap: and + /// also treat the stored source as stale when the current graph's hash differs from + /// the one recorded here, in addition to the existing version check. Computed with the same hashing + /// uses for the graph field, via + /// , so the two never disagree about what "the graph changed" means. + /// + /// A definition imported before this marker existed carries no value for this key; + /// falls back to the version-only check for it rather than refusing every definition imported under the older + /// behaviour. + /// + /// + public const string SourceGraphHashCustomPropertyKey = "Bpmn:SourceGraphHash"; + /// /// The host capabilities this deployment's BPMN runtime declares. /// @@ -177,8 +203,9 @@ public sealed class BpmnInterchangeDocumentService( /// When set, the definition this import must otherwise leave untouched: its name, description, variables, /// inputs, outputs, outcomes, options, tool version, read-only flag and custom properties are carried onto the /// imported definition as-is, and only the bound activity graph and the , - /// and custom - /// properties this method owns change. This is what passes so the document PUT + /// , and + /// custom properties this method owns change. This is what + /// passes so the document PUT /// edits the BPMN document without silently resetting the rest of the definition; left null for a /// whole-definition import, where the model built from the document alone is the intended contract. /// @@ -240,6 +267,7 @@ public sealed class BpmnInterchangeDocumentService( persisted.CustomProperties[SourceXmlCustomPropertyKey] = xml; persisted.CustomProperties[SourceVersionCustomPropertyKey] = persisted.Version; persisted.CustomProperties[SourceProcessIdCustomPropertyKey] = rootDefinition.ProcessId; + persisted.CustomProperties[SourceGraphHashCustomPropertyKey] = BpmnContentHash.OfGraph(persisted.StringData); await store.SaveAsync(persisted, cancellationToken); } @@ -307,7 +335,7 @@ public sealed class BpmnInterchangeDocumentService( /// properties other than the ones this service owns — is carried onto the result unchanged; see the shared /// import logic's preserveMetadataFrom parameter, which this passes the existing definition to. Only the activity graph /// and the // - /// custom properties move. + /// / custom properties move. /// /// The edited document, deserialized through the library's own JSON converters. /// The workflow definition to update. @@ -383,6 +411,22 @@ public sealed class BpmnInterchangeDocumentService( + "would silently return a document that is not what this definition currently is."); } + // The version check above catches a publish or any other save that assigns a new version, but an unpublished + // draft is saved in place — same row, same version — so a designer save of the draft (e.g. an edit to a bound + // activity's inputs) moves the graph without moving the version. SourceGraphHashCustomPropertyKey catches that: + // a definition imported before this marker existed carries no value for it, so it falls back to the + // version-only check above rather than refusing every definition imported under the older behaviour. + if (definition.CustomProperties.TryGetValue(SourceGraphHashCustomPropertyKey, out var sourceGraphHash) + && !string.IsNullOrEmpty(sourceGraphHash) + && sourceGraphHash != BpmnContentHash.OfGraph(definition.StringData)) + { + throw new BpmnExportUnavailableException( + $"Workflow definition '{definition.DefinitionId}' has changed since it was imported from BPMN: its activity graph no longer matches " + + "the graph the stored source was imported against, even though its version has not changed (an unpublished draft is saved in place). " + + "The BPMN source stored on it no longer corresponds to this definition, so exporting it would silently return a document that is " + + "not what this definition currently is."); + } + return xml; } diff --git a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Scenarios/Interchange/BpmnExportAvailabilityTests.cs b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Scenarios/Interchange/BpmnExportAvailabilityTests.cs index 91d2eb923..479241459 100644 --- a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Scenarios/Interchange/BpmnExportAvailabilityTests.cs +++ b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Scenarios/Interchange/BpmnExportAvailabilityTests.cs @@ -1,4 +1,5 @@ using Bpmn.Interchange; +using Elsa.Bpmn.Activities; using Elsa.Bpmn.Interchange.Binding; using Elsa.Bpmn.Interchange.Exceptions; using Elsa.Bpmn.Interchange.IntegrationTests.Support; @@ -6,6 +7,7 @@ using Elsa.Bpmn.Interchange.Services; using Elsa.Common.Models; using Elsa.Extensions; using Elsa.Testing.Shared; +using Elsa.Workflows.Activities; using Elsa.Workflows.Management; using Elsa.Workflows.Management.Entities; using Elsa.Workflows.Management.Filters; @@ -51,6 +53,79 @@ public class BpmnExportAvailabilityTests(ITestOutputHelper testOutputHelper) : B Assert.Equal(stored.Version, sourceVersion); } + [Fact(DisplayName = "Exporting a definition whose unpublished draft was saved from the designer with a changed graph is refused as stale, even though its version has not changed")] + public async Task Export_OfADefinitionSavedFromTheDesignerWithAChangedGraph_IsRefusedAsStale() + { + var xml = ReadAsset("camunda-order-process.bpmn"); + var imported = await DocumentService.ImportAsync(xml, definitionId: null, name: null, processId: null, CancellationToken.None); + Assert.True(imported.ImportResult.Succeeded); + var definitionId = imported.ImportResult.WorkflowDefinition.DefinitionId; + + // Captured as plain values, not the live entity: the in-memory store this test runs against hands back the + // same object reference on every find, so holding onto the entity itself would see the designer's edit + // through both "before" and "after" variables. + var storedBefore = await FindLatestAsync(definitionId); + var (versionBeforeSave, stringDataBeforeSave) = (storedBefore.Version, storedBefore.StringData); + + await SaveDraftFromTheDesignerAsync(definitionId); + + var storedAfterSave = await FindLatestAsync(definitionId); + + // Saved in place: same version, only the graph moved. + Assert.Equal(versionBeforeSave, storedAfterSave.Version); + Assert.NotEqual(stringDataBeforeSave, storedAfterSave.StringData); + + var exception = Assert.Throws(() => DocumentService.Export(storedAfterSave)); + + Assert.Contains("has changed since it was imported", exception.Message); + + var documentException = Assert.Throws(() => DocumentService.ReadDocument(storedAfterSave)); + Assert.Contains("has changed since it was imported", documentException.Message); + } + + [Fact(DisplayName = "Exporting a definition whose unpublished draft was saved from the designer with no graph change still succeeds")] + public async Task Export_OfADefinitionSavedFromTheDesignerWithNoGraphChange_StillSucceeds() + { + var xml = ReadAsset("camunda-order-process.bpmn"); + var imported = await DocumentService.ImportAsync(xml, definitionId: null, name: null, processId: null, CancellationToken.None); + Assert.True(imported.ImportResult.Succeeded); + var definitionId = imported.ImportResult.WorkflowDefinition.DefinitionId; + + // Round-trips the draft through the designer's save path unedited, so the graph hash still matches. + var draft = await DefinitionPublisher.GetDraftAsync(definitionId, VersionOptions.Latest); + Assert.NotNull(draft); + await DefinitionPublisher.SaveDraftAsync(draft!); + + var stored = await FindLatestAsync(definitionId); + + var bytes = DocumentService.Export(stored); + + Assert.NotEmpty(bytes); + } + + [Fact(DisplayName = "A definition imported before the graph-hash marker existed still exports after a designer save, on version alone")] + public async Task Export_OfADefinitionWithoutAGraphHashMarker_FallsBackToVersionOnlyAfterADesignerSave() + { + var xml = ReadAsset("camunda-order-process.bpmn"); + var imported = await DocumentService.ImportAsync(xml, definitionId: null, name: null, processId: null, CancellationToken.None); + Assert.True(imported.ImportResult.Succeeded); + var definitionId = imported.ImportResult.WorkflowDefinition.DefinitionId; + + // Simulates a definition imported before this marker existed. + var stored = await FindLatestAsync(definitionId); + stored.CustomProperties.Remove(BpmnInterchangeDocumentService.SourceGraphHashCustomPropertyKey); + await DefinitionStore.SaveAsync(stored); + + await SaveDraftFromTheDesignerAsync(definitionId); + + var storedAfterSave = await FindLatestAsync(definitionId); + Assert.False(storedAfterSave.CustomProperties.ContainsKey(BpmnInterchangeDocumentService.SourceGraphHashCustomPropertyKey)); + + var bytes = DocumentService.Export(storedAfterSave); + + Assert.NotEmpty(bytes); + } + [Fact(DisplayName = "Exporting a definition whose custom properties no longer carry the BPMN source is refused, naming that as the reason")] public async Task Export_OfADefinitionWithoutStoredSource_IsRefused() { @@ -171,6 +246,24 @@ public class BpmnExportAvailabilityTests(ITestOutputHelper testOutputHelper) : B return definition!; } + /// + /// Saves the latest draft of the way the workflow-definition save endpoint + /// Studio's designer calls does — + /// then — with the bound activity's text edited, same + /// as BpmnInterchangeEndpointTests.SaveDraftFromTheDesignerAsync. + /// + private async Task SaveDraftFromTheDesignerAsync(string definitionId) + { + var draft = await DefinitionPublisher.GetDraftAsync(definitionId, VersionOptions.Latest); + Assert.NotNull(draft); + + var root = Assert.IsType(ActivitySerializer.Deserialize(draft!.StringData!)); + Assert.Single(root.Activities.OfType()).Text = new("Notifying the warehouse, edited in the designer"); + draft.StringData = ActivitySerializer.Serialize(root); + + await DefinitionPublisher.SaveDraftAsync(draft); + } + /// /// Mirrors the one line of the real /// this test cares about — draft.CustomProperties = model.CustomProperties ?? new Dictionary<...>() — diff --git a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Scenarios/Interchange/BpmnInterchangeTestBase.cs b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Scenarios/Interchange/BpmnInterchangeTestBase.cs index b188bbbad..86482abc2 100644 --- a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Scenarios/Interchange/BpmnInterchangeTestBase.cs +++ b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Scenarios/Interchange/BpmnInterchangeTestBase.cs @@ -2,6 +2,7 @@ using Elsa.Bpmn.Interchange.IntegrationTests.Support; using Elsa.Bpmn.Interchange.Services; using Elsa.Extensions; using Elsa.Testing.Shared; +using Elsa.Workflows; using Elsa.Workflows.Management; using Microsoft.Extensions.DependencyInjection; using Xunit.Abstractions; @@ -23,12 +24,25 @@ public abstract class BpmnInterchangeTestBase : IAsyncLifetime DocumentService = _services.GetRequiredService(); DefinitionStore = _services.GetRequiredService(); + DefinitionPublisher = _services.GetRequiredService(); + ActivitySerializer = _services.GetRequiredService(); } protected BpmnInterchangeDocumentService DocumentService { get; } protected IWorkflowDefinitionStore DefinitionStore { get; } + /// + /// Resolves the same the workflow-definition save endpoint the + /// designer uses calls, so a test can save a draft the way the designer does: + /// + /// then . + /// + protected IWorkflowDefinitionPublisher DefinitionPublisher { get; } + + /// Resolves activities out of a draft's StringData so a test can edit one before saving it back. + protected IActivitySerializer ActivitySerializer { get; } + public Task InitializeAsync() => _services.PopulateRegistriesAsync(); public Task DisposeAsync() => Task.CompletedTask;