Fix conditional GET across the API

src/ServiceControl.AcceptanceTests/WebApi/When_a_request_is_repeated_with_its_etag.cs

@@ -0,0 +1,95 @@
+namespace ServiceControl.AcceptanceTests.WebApi
+{
+    using System.Net;
+    using System.Net.Http;
+    using System.Threading.Tasks;
+    using AcceptanceTesting;
+    using NServiceBus.AcceptanceTesting;
+    using NUnit.Framework;
+    using Recoverability.MessageRedirects;
+
+    class When_a_request_is_repeated_with_its_etag : AcceptanceTest
+    {
+        [TestCase("/api/customchecks", "GET", false)]
+        [TestCase("/api/redirects", "GET", true)]
+        [TestCase("/api/redirect", "HEAD", true)]
+        public async Task Should_answer_not_modified(string url, string method, bool seedARedirect)
+        {
+            Answer issued = null;
+            Answer repeated = null;
+
+            await Define<Context>()
+                .Done(async ctx =>
+                {
+                    if (seedARedirect)
+                    {
+                        await this.Post("/api/redirects", new RedirectRequest
+                        {
+                            fromphysicaladdress = "endpointA@machine1",
+                            tophysicaladdress = "endpointB@machine2"
+                        }, status => status is not HttpStatusCode.Created);
+                    }
+
+                    // Internal custom checks re-report on a timer, so the validator can move between
+                    // the two requests.
+                    for (var attempt = 0; attempt < 5; attempt++)
+                    {
+                        issued = await Ask(method, url, ifNoneMatch: null);
+
+                        if (issued.Etag == null)
+                        {
+                            continue;
+                        }
+
+                        repeated = await Ask(method, url, issued.Etag);
+
+                        if (repeated.Status == HttpStatusCode.NotModified)
+                        {
+                            break;
+                        }
+                    }
+
+                    return true;
+                })
+                .Run();
+
+            Assert.That(issued.Etag, Is.Not.Null, $"{method} {url} issued no ETag, so there is nothing for a client to revalidate against");
+            Assert.That(repeated.Status, Is.EqualTo(HttpStatusCode.NotModified), $"{method} {url} sent the full payload again for a client that already held {issued.Etag}");
+
+            // ServicePulse drives pagination from Total-Count, and a revalidating client takes it from
+            // the 304 rather than from the body it already holds.
+            Assert.That(repeated.TotalCount, Is.Not.Null.And.EqualTo(issued.TotalCount), $"{method} {url} did not carry its Total-Count through to the 304");
+        }
+
+        async Task<Answer> Ask(string method, string url, string ifNoneMatch)
+        {
+            using var response = await Send(method, url, ifNoneMatch);
+
+            return new Answer(
+                response.StatusCode,
+                Header(response, "ETag"),
+                Header(response, "Total-Count"));
+        }
+
+        static string Header(HttpResponseMessage response, string name) =>
+            response.Headers.TryGetValues(name, out var values) ? string.Join(string.Empty, values) : null;
+
+        Task<HttpResponseMessage> Send(string method, string url, string ifNoneMatch)
+        {
+            var request = new HttpRequestMessage(new HttpMethod(method), url);
+
+            if (ifNoneMatch != null)
+            {
+                // Unvalidated, so a malformed validator reaches the server and shows up as a 200
+                // rather than throwing here.
+                request.Headers.TryAddWithoutValidation("If-None-Match", ifNoneMatch);
+            }
+
+            return HttpClient.SendAsync(request);
+        }
+
+        record Answer(HttpStatusCode Status, string Etag, string TotalCount);
+
+        class Context : ScenarioContext;
+    }
+}

src/ServiceControl.Audit.AcceptanceTests/WebApi/When_a_message_body_is_requested_twice.cs

@@ -0,0 +1,92 @@
+namespace ServiceControl.Audit.AcceptanceTests.WebApi
+{
+    using System.Net;
+    using System.Net.Http;
+    using System.Threading.Tasks;
+    using AcceptanceTesting;
+    using AcceptanceTesting.EndpointTemplates;
+    using Audit.Auditing.MessagesView;
+    using NServiceBus;
+    using NServiceBus.AcceptanceTesting;
+    using NServiceBus.Settings;
+    using NUnit.Framework;
+
+    class When_a_message_body_is_requested_twice : AcceptanceTest
+    {
+        [Test]
+        public async Task Should_answer_not_modified()
+        {
+            string issued = null;
+            HttpStatusCode? repeated = null;
+
+            await Define<MyContext>()
+                .WithEndpoint<Receiver>(b => b.When(bus => bus.SendLocal(new MyMessage { Payload = "PAYLOAD" })))
+                .Done(async c =>
+                {
+                    if (c.MessageId == null)
+                    {
+                        return false;
+                    }
+
+                    MessagesView audited = await this.TryGetSingle<MessagesView>("/api/messages?include_system_messages=false&sort=id", m => m.MessageId == c.MessageId);
+
+                    if (audited == null)
+                    {
+                        return false;
+                    }
+
+                    var url = $"/api{audited.BodyUrl}";
+
+                    using var first = await this.GetRaw(url);
+
+                    if (!first.Headers.TryGetValues("ETag", out var values))
+                    {
+                        return false;
+                    }
+
+                    issued = string.Join(string.Empty, values);
+
+                    var request = new HttpRequestMessage(HttpMethod.Get, url);
+                    // Unvalidated, so a malformed validator reaches the server and shows up as a 200.
+                    request.Headers.TryAddWithoutValidation("If-None-Match", issued);
+
+                    using var second = await HttpClient.SendAsync(request);
+                    repeated = second.StatusCode;
+
+                    return true;
+                })
+                .Run();
+
+            Assert.That(issued, Is.Not.Null, "the body response carried no validator, so a client can never revalidate it");
+            Assert.That(repeated, Is.EqualTo(HttpStatusCode.NotModified), $"the body was sent again to a client that already held {issued}");
+        }
+
+        public class Receiver : EndpointConfigurationBuilder
+        {
+            public Receiver() => EndpointSetup<DefaultServerWithAudit>();
+
+            [Handler]
+            public class MyMessageHandler(MyContext testContext, IReadOnlySettings settings) : IHandleMessages<MyMessage>
+            {
+                public Task Handle(MyMessage message, IMessageHandlerContext context)
+                {
+                    testContext.EndpointNameOfReceivingEndpoint = settings.EndpointName();
+                    testContext.MessageId = context.MessageId;
+                    return Task.CompletedTask;
+                }
+            }
+        }
+
+        public class MyMessage : ICommand
+        {
+            public string Payload { get; set; }
+        }
+
+        public class MyContext : ScenarioContext
+        {
+            public string MessageId { get; set; }
+
+            public string EndpointNameOfReceivingEndpoint { get; set; }
+        }
+    }
+}

src/ServiceControl.Audit.Persistence.InMemory/InMemoryAttachmentsBodyStorage.cs

@@ -1,6 +1,5 @@
 namespace ServiceControl.Audit.Persistence.InMemory
 {
-    using System;
     using System.Collections.Generic;
     using System.IO;
     using System.Linq;
@@ -61,7 +60,8 @@ public async Task<StreamResult> TryFetch(string bodyId, CancellationToken cancel
                     Stream = new MemoryStream(messageBody.Content),
                     ContentType = messageBody.ContentType,
                     BodySize = messageBody.BodySize,
-                    Etag = Guid.NewGuid().ToString()
+                    // Bodies are immutable per message, so the id is a stable validator
+                    Etag = bodyId
                 });
         }
 

src/ServiceControl.Audit.Persistence.InMemory/InMemoryAuditDataStore.cs

@@ -168,7 +168,7 @@ Task<MessageBodyView> GetMessageBodyFromMetadata(string messageId)
                 return Task.FromResult(MessageBodyView.NotFound());
             }
 
-            return Task.FromResult(MessageBodyView.FromString(body, contentType, bodySize, string.Empty));
+            return Task.FromResult(MessageBodyView.FromString(body, contentType, bodySize, messageId));
         }
 
         public Task<QueryResult<IList<AuditCount>>> QueryAuditCounts(string endpointName, CancellationToken cancellationToken)

src/ServiceControl.Audit.Persistence.Tests/AuditTests.cs

@@ -115,6 +115,28 @@ public async Task Can_roundtrip_message_body()
             }
         }
 
+        [Test]
+        public async Task Message_body_validator_is_stable_across_reads()
+        {
+            var unitOfWork = await StartAuditUnitOfWork(1);
+
+            var body = new byte[100];
+            Random.Shared.NextBytes(body);
+            var processedMessage = MakeMessage();
+
+            await unitOfWork.RecordProcessedMessage(processedMessage, body);
+
+            await unitOfWork.DisposeAsync();
+
+            var bodyId = GetBodyId(processedMessage);
+
+            var first = await DataStore.GetMessageBody(bodyId, TestContext.CurrentContext.CancellationToken);
+            var second = await DataStore.GetMessageBody(bodyId, TestContext.CurrentContext.CancellationToken);
+
+            Assert.That(first.ETag, Is.Not.Null.And.Not.Empty, "a body with no validator cannot be revalidated, so conditional GET is dead on it");
+            Assert.That(second.ETag, Is.EqualTo(first.ETag), "the body did not change, so neither may its validator");
+        }
+
         [Test]
         public async Task Does_respect_max_message_body()
         {

src/ServiceControl.Audit.UnitTests/Infrastructure/WebApi/ConditionalGetTests.cs

@@ -0,0 +1,97 @@
+namespace ServiceControl.Audit.UnitTests.Infrastructure.WebApi
+{
+    using System.Net;
+    using Audit.Infrastructure.WebApi;
+    using Microsoft.AspNetCore.Http;
+    using Microsoft.AspNetCore.Mvc;
+    using Microsoft.AspNetCore.Mvc.Abstractions;
+    using Microsoft.AspNetCore.Mvc.Filters;
+    using Microsoft.AspNetCore.Routing;
+    using NUnit.Framework;
+
+    [TestFixture]
+    public class ConditionalGetTests
+    {
+        [Test]
+        public void Repeating_a_request_with_the_etag_just_issued_is_not_modified()
+        {
+            var httpContext = new DefaultHttpContext();
+
+            httpContext.Response.WithEtag("4611686018427387904");
+            httpContext.Request.Headers.IfNoneMatch = httpContext.Response.Headers.ETag;
+
+            var context = ResultExecuting(httpContext);
+
+            new NotModifiedStatusHttpHandler().OnResultExecuting(context);
+
+            Assert.That(context.Result, Is.InstanceOf<StatusCodeResult>(),
+                "the client already holds this version, so the response must be a 304 rather than the full payload");
+            Assert.That(((StatusCodeResult)context.Result).StatusCode, Is.EqualTo((int)HttpStatusCode.NotModified));
+        }
+
+        [Test]
+        public void A_different_etag_still_returns_the_payload()
+        {
+            var httpContext = new DefaultHttpContext();
+
+            httpContext.Response.WithEtag("4611686018427387904");
+            httpContext.Request.Headers.IfNoneMatch = "\"something-else\"";
+
+            var context = ResultExecuting(httpContext);
+
+            new NotModifiedStatusHttpHandler().OnResultExecuting(context);
+
+            Assert.That(context.Result, Is.InstanceOf<OkObjectResult>());
+        }
+
+        [Test]
+        public void The_emitted_etag_is_a_well_formed_entity_tag()
+        {
+            var httpContext = new DefaultHttpContext();
+
+            httpContext.Response.WithEtag("4611686018427387904");
+
+            Assert.That(httpContext.Response.GetTypedHeaders().ETag, Is.Not.Null,
+                "an ETag that cannot be parsed as an entity-tag disables conditional GET without any error");
+        }
+
+        [Test]
+        public void A_deterministic_etag_is_a_well_formed_entity_tag()
+        {
+            var httpContext = new DefaultHttpContext();
+
+            httpContext.Response.WithDeterministicEtag("any-non-empty-payload-signature");
+
+            Assert.That(httpContext.Response.GetTypedHeaders().ETag, Is.Not.Null);
+        }
+
+        [Test]
+        public void The_emitted_etag_quotes_the_value_without_altering_it()
+        {
+            var httpContext = new DefaultHttpContext();
+
+            httpContext.Response.WithEtag("4611686018427387904");
+
+            Assert.That(httpContext.Response.Headers.ETag.ToString(), Is.EqualTo("\"4611686018427387904\""));
+        }
+
+        [TestCase(null)]
+        [TestCase("")]
+        public void A_call_site_with_nothing_to_validate_emits_no_etag_header(string value)
+        {
+            var httpContext = new DefaultHttpContext();
+
+            httpContext.Response.WithEtag(value);
+
+            Assert.That(httpContext.Response.Headers.ContainsKey("ETag"), Is.False,
+                "an empty entity-tag is well formed, so it would match itself and answer 304 for unrelated payloads");
+        }
+
+        static ResultExecutingContext ResultExecuting(HttpContext httpContext) =>
+            new(
+                new ActionContext(httpContext, new RouteData(), new ActionDescriptor()),
+                [],
+                new OkObjectResult(new object()),
+                controller: null);
+    }
+}

src/ServiceControl.Audit/Auditing/MessagesView/GetMessagesController.cs

@@ -68,7 +68,7 @@ public async Task<IActionResult> Get(string id, CancellationToken cancellationTo
                 throw new Exception($"Metadata for message '{id}' indicated that a body was present but no content could be found in storage");
             }
 
-            Response.Headers.ETag = result.ETag;
+            Response.WithEtag(result.ETag);
             var contentType = result.ContentType ?? "text/*";
             return result.StringContent != null ? Content(result.StringContent, contentType) : File(result.StreamContent, contentType);
         }

src/ServiceControl.Audit/Infrastructure/WebApi/HttpResponseExtensions.cs

@@ -14,7 +14,19 @@ static class HttpResponseExtensions
     {
         public static void WithTotalCount(this HttpResponse response, long totalCount) => response.WithHeader("Total-Count", totalCount.ToString(CultureInfo.InvariantCulture));
 
-        public static void WithEtag(this HttpResponse response, StringValues value) => response.Headers.ETag = value;
+        public static void WithEtag(this HttpResponse response, StringValues value)
+        {
+            var validator = value.ToString();
+
+            if (string.IsNullOrEmpty(validator))
+            {
+                return;
+            }
+
+            // RFC 9110 requires an entity-tag to be a quoted string. Unquoted, EntityTagHeaderValue
+            // cannot parse it and NotModifiedStatusHttpHandler never matches a client's If-None-Match.
+            response.Headers.ETag = $"\"{validator}\"";
+        }
 
         public static void WithQueryStatsInfo(this HttpResponse response, QueryStatsInfo queryStatsInfo)
         {

src/ServiceControl.MultiInstance.AcceptanceTests/Auditing/When_requesting_a_message_body.cs

@@ -72,7 +72,7 @@ public async Task Should_be_forwarded_to_audit_instance(CancellationToken cancel
             using (Assert.EnterMultipleScope())
             {
                 Assert.That(body, Is.EqualTo(context.MessageBody), "Body bytes mismatch");
-                Assert.That(response.Headers.GetValues("ETag").SingleOrDefault(), Is.Not.Null, "Etag not set");
+                Assert.That(response.Headers.ETag, Is.Not.Null, "Etag not set, or not a well formed entity-tag");
             }
         }