From 6957ec7c6071ccfc545acdbf64f9816a1a558784 Mon Sep 17 00:00:00 2001 From: Sluijsens Date: Tue, 28 Jul 2026 12:31:00 +0200 Subject: [PATCH] Tells apart a slave that is down from one that does not know us MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The integrity check mapped four outcomes onto a single null: no response, a 404, a rejected key, and a genuine answer. Only "rejected key" is recoverable, and it was being reported as "unreachable" and never repaired — so an instance registered against the wrong URL stayed broken until someone edited the database by hand. That is exactly what happened locally. GetRegisteredMasterUrlAsync now returns an outcome alongside the URL. Unauthorized triggers registration; 404 is reported as "this host does not serve the master/slave protocol", which names the actual mistake instead of hiding it behind a generic contact failure; unreachable and server errors behave as before. Registering on a rejected key cannot hijack a slave that belongs to another master: the slave accepts a registration only when it has none, and refuses any key that does not match an existing one. So it succeeds exactly in the case worth recovering and fails harmlessly otherwise. That guarantee lives on the slave, so the test asserting the refusal now says out loud that the master depends on it. Found while diagnosing a status push that failed against a frontend URL. Small and contained, so fixed here rather than filed as tech debt. 372 tests pass, up from 366. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HHoJpxYXzHACSQguHrC5fw --- .../MasterAvailabilityServiceTests.cs | 10 +++ .../Services/CmsInstanceServiceTests.cs | 89 +++++++++++++++++-- .../Services/SlaveApiClientTests.cs | 42 +++++++-- .../Services/CmsInstanceService.cs | 48 ++++++++-- .../Services/ISlaveApiClient.cs | 10 ++- .../Services/SlaveApiClient.cs | 27 +++++- .../Services/SlaveContactResult.cs | 45 ++++++++++ 7 files changed, 251 insertions(+), 20 deletions(-) create mode 100644 src/SlpModularCms.Modules.Master/Services/SlaveContactResult.cs diff --git a/src/SlpModularCms.Modules.Availability.Tests/Services/MasterAvailabilityServiceTests.cs b/src/SlpModularCms.Modules.Availability.Tests/Services/MasterAvailabilityServiceTests.cs index 6530e62..789af62 100644 --- a/src/SlpModularCms.Modules.Availability.Tests/Services/MasterAvailabilityServiceTests.cs +++ b/src/SlpModularCms.Modules.Availability.Tests/Services/MasterAvailabilityServiceTests.cs @@ -68,6 +68,16 @@ public class MasterAvailabilityServiceTests : IDisposable await _repo.Received(1).SaveChangesAsync(); } + /// + /// A slave that already belongs to a master refuses any other key. + /// + /// + /// Do not relax this. The master's integrity check registers automatically when a slave + /// rejects its key (CmsInstanceService.VerifyIntegrityAsync), which is safe only because + /// this refusal holds: registration then succeeds exactly for a slave that has no registration + /// yet, and fails for one that belongs to someone else. Weaken it and that automatic + /// registration becomes a way for one master to take over another master's slave. + /// [Fact] public async Task RegisterAsync_ReturnsFalse_WhenKeyMismatch() { diff --git a/src/SlpModularCms.Modules.Master.Tests/Services/CmsInstanceServiceTests.cs b/src/SlpModularCms.Modules.Master.Tests/Services/CmsInstanceServiceTests.cs index 77392c4..9108c66 100644 --- a/src/SlpModularCms.Modules.Master.Tests/Services/CmsInstanceServiceTests.cs +++ b/src/SlpModularCms.Modules.Master.Tests/Services/CmsInstanceServiceTests.cs @@ -1,4 +1,4 @@ -using FluentAssertions; +using FluentAssertions; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; @@ -258,7 +258,7 @@ public class CmsInstanceServiceTests var instance = ActiveInstance(); _repo.GetActiveAsync().Returns([instance]); _protector.Unprotect("encrypted-key").Returns("plain"); - _slaveClient.GetRegisteredMasterUrlAsync(Arg.Any(), Arg.Any()).Returns((string?)null); + _slaveClient.GetRegisteredMasterUrlAsync(Arg.Any(), Arg.Any()).Returns(RegisteredMasterUrlResult.Unreachable); _httpContextAccessor.HttpContext.Returns((HttpContext?)null); await CreateSut().VerifyIntegrityAsync(); @@ -272,7 +272,7 @@ public class CmsInstanceServiceTests var instance = ActiveInstance(); _repo.GetActiveAsync().Returns([instance]); _protector.Unprotect("encrypted-key").Returns("plain"); - _slaveClient.GetRegisteredMasterUrlAsync(Arg.Any(), Arg.Any()).Returns("https://old-master.test"); + _slaveClient.GetRegisteredMasterUrlAsync(Arg.Any(), Arg.Any()).Returns(RegisteredMasterUrlResult.Ok("https://old-master.test")); _slaveClient.RegisterMasterAsync(Arg.Any(), Arg.Any(), Arg.Any()).Returns(true); _httpContextAccessor.HttpContext.Returns((HttpContext?)null); @@ -288,7 +288,7 @@ public class CmsInstanceServiceTests instance.LastIntegrityCheckFailedAt = DateTimeOffset.UtcNow.AddHours(-1); _repo.GetActiveAsync().Returns([instance]); _protector.Unprotect("encrypted-key").Returns("plain"); - _slaveClient.GetRegisteredMasterUrlAsync(Arg.Any(), Arg.Any()).Returns("https://master.test"); + _slaveClient.GetRegisteredMasterUrlAsync(Arg.Any(), Arg.Any()).Returns(RegisteredMasterUrlResult.Ok("https://master.test")); _httpContextAccessor.HttpContext.Returns((HttpContext?)null); await CreateSut().VerifyIntegrityAsync(); @@ -304,7 +304,7 @@ public class CmsInstanceServiceTests instance.DisableMessage = "Onderhoud"; _repo.GetActiveAsync().Returns([instance]); _protector.Unprotect("encrypted-key").Returns("plain"); - _slaveClient.GetRegisteredMasterUrlAsync(Arg.Any(), Arg.Any()).Returns("https://master.test"); + _slaveClient.GetRegisteredMasterUrlAsync(Arg.Any(), Arg.Any()).Returns(RegisteredMasterUrlResult.Ok("https://master.test")); _slaveClient.PushStatusAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()).Returns(true); _httpContextAccessor.HttpContext.Returns((HttpContext?)null); @@ -314,6 +314,85 @@ public class CmsInstanceServiceTests _repo.Received().Update(Arg.Is(i => i.LastStatusPushedAt.HasValue)); } + /// + /// The state this branch exists for: the slave answers but does not recognise us, which + /// happens when the original registration call went to the wrong URL. It used to be reported + /// as "unreachable" and never repaired, so the instance stayed broken until someone edited the + /// database by hand. + /// + [Fact] + public async Task VerifyIntegrityAsync_Registers_WhenSlaveDoesNotRecogniseThisMaster() + { + var instance = ActiveInstance(); + _repo.GetActiveAsync().Returns([instance]); + _protector.Unprotect("encrypted-key").Returns("plain"); + _slaveClient.GetRegisteredMasterUrlAsync(Arg.Any(), Arg.Any()) + .Returns(RegisteredMasterUrlResult.Unauthorized); + _slaveClient.RegisterMasterAsync(Arg.Any(), Arg.Any(), Arg.Any()).Returns(true); + _httpContextAccessor.HttpContext.Returns((HttpContext?)null); + + await CreateSut().VerifyIntegrityAsync(); + + await _slaveClient.Received(1).RegisterMasterAsync("https://slave.test", "plain", "https://master.test"); + _repo.Received().Update(Arg.Is(i => i.LastIntegrityCheckFailedAt == null && i.LastContactedAt.HasValue)); + } + + /// + /// A slave that belongs to a different master refuses the registration, and the master must + /// accept that rather than keep hammering. The refusal itself is the slave's job — see + /// MasterAvailabilityServiceTests — this asserts the master honours it. + /// + [Fact] + public async Task VerifyIntegrityAsync_StaysFailed_WhenRegistrationIsRefused() + { + var instance = ActiveInstance(); + _repo.GetActiveAsync().Returns([instance]); + _protector.Unprotect("encrypted-key").Returns("plain"); + _slaveClient.GetRegisteredMasterUrlAsync(Arg.Any(), Arg.Any()) + .Returns(RegisteredMasterUrlResult.Unauthorized); + _slaveClient.RegisterMasterAsync(Arg.Any(), Arg.Any(), Arg.Any()).Returns(false); + _httpContextAccessor.HttpContext.Returns((HttpContext?)null); + + await CreateSut().VerifyIntegrityAsync(); + + _repo.Received().Update(Arg.Is(i => i.LastIntegrityCheckFailedAt.HasValue)); + } + + /// + /// A URL pointing at a frontend rather than an instance's API. Registering again would 404 + /// just the same, so it must not be attempted — the URL is wrong and only a human can fix it. + /// + [Fact] + public async Task VerifyIntegrityAsync_DoesNotRegister_WhenHostDoesNotSpeakTheProtocol() + { + var instance = ActiveInstance(); + _repo.GetActiveAsync().Returns([instance]); + _protector.Unprotect("encrypted-key").Returns("plain"); + _slaveClient.GetRegisteredMasterUrlAsync(Arg.Any(), Arg.Any()) + .Returns(RegisteredMasterUrlResult.NotAProtocolEndpoint); + _httpContextAccessor.HttpContext.Returns((HttpContext?)null); + + await CreateSut().VerifyIntegrityAsync(); + + await _slaveClient.DidNotReceive().RegisterMasterAsync(Arg.Any(), Arg.Any(), Arg.Any()); + _repo.Received().Update(Arg.Is(i => i.LastIntegrityCheckFailedAt.HasValue)); + } + + [Fact] + public async Task VerifyIntegrityAsync_DoesNotRegister_WhenSlaveIsUnreachable() + { + var instance = ActiveInstance(); + _repo.GetActiveAsync().Returns([instance]); + _protector.Unprotect("encrypted-key").Returns("plain"); + _slaveClient.GetRegisteredMasterUrlAsync(Arg.Any(), Arg.Any()) + .Returns(RegisteredMasterUrlResult.Unreachable); + _httpContextAccessor.HttpContext.Returns((HttpContext?)null); + + await CreateSut().VerifyIntegrityAsync(); + + await _slaveClient.DidNotReceive().RegisterMasterAsync(Arg.Any(), Arg.Any(), Arg.Any()); + } + private static CmsInstance ActiveInstance() => new() { Id = Guid.NewGuid(), diff --git a/src/SlpModularCms.Modules.Master.Tests/Services/SlaveApiClientTests.cs b/src/SlpModularCms.Modules.Master.Tests/Services/SlaveApiClientTests.cs index 928abc7..4f2020d 100644 --- a/src/SlpModularCms.Modules.Master.Tests/Services/SlaveApiClientTests.cs +++ b/src/SlpModularCms.Modules.Master.Tests/Services/SlaveApiClientTests.cs @@ -1,4 +1,4 @@ -using FluentAssertions; +using FluentAssertions; using SlpModularCms.Modules.Master.Services; using System.Net; using System.Text; @@ -71,7 +71,7 @@ public class SlaveApiClientTests } [Fact] - public async Task GetRegisteredMasterUrlAsync_ReturnsMasterUrl_WhenResponseIsSuccess() + public async Task GetRegisteredMasterUrlAsync_ReturnsOkWithMasterUrl_WhenResponseIsSuccess() { var json = JsonSerializer.Serialize(new { MasterUrl = MasterUrl }); var handler = new FakeHttpMessageHandler(HttpStatusCode.OK, json); @@ -79,18 +79,50 @@ public class SlaveApiClientTests var result = await sut.GetRegisteredMasterUrlAsync(SlaveUrl, ApiKey); - result.Should().Be(MasterUrl); + result.Outcome.Should().Be(SlaveContactOutcome.Ok); + result.MasterUrl.Should().Be(MasterUrl); } + /// + /// The slave answered and refused the key. Distinct from unreachable, because it is the one + /// failure the integrity check can repair on its own. + /// [Fact] - public async Task GetRegisteredMasterUrlAsync_ReturnsNull_WhenResponseIsFailure() + public async Task GetRegisteredMasterUrlAsync_ReturnsUnauthorized_WhenSlaveRejectsTheKey() + { + var handler = new FakeHttpMessageHandler(HttpStatusCode.Unauthorized); + var sut = CreateSut(handler); + + var result = await sut.GetRegisteredMasterUrlAsync(SlaveUrl, ApiKey); + + result.Outcome.Should().Be(SlaveContactOutcome.Unauthorized); + } + + /// + /// Something is listening but does not serve /api/v1/master/*. Almost always a URL pointing at + /// a frontend instead of an instance's API — worth its own outcome, because reporting it as a + /// generic contact failure hides the actual mistake. + /// + [Fact] + public async Task GetRegisteredMasterUrlAsync_ReturnsNotAProtocolEndpoint_OnNotFound() { var handler = new FakeHttpMessageHandler(HttpStatusCode.NotFound); var sut = CreateSut(handler); var result = await sut.GetRegisteredMasterUrlAsync(SlaveUrl, ApiKey); - result.Should().BeNull(); + result.Outcome.Should().Be(SlaveContactOutcome.NotAProtocolEndpoint); + } + + [Fact] + public async Task GetRegisteredMasterUrlAsync_ReturnsUnreachable_OnServerError() + { + var handler = new FakeHttpMessageHandler(HttpStatusCode.InternalServerError); + var sut = CreateSut(handler); + + var result = await sut.GetRegisteredMasterUrlAsync(SlaveUrl, ApiKey); + + result.Outcome.Should().Be(SlaveContactOutcome.Unreachable); } [Fact] diff --git a/src/SlpModularCms.Modules.Master/Services/CmsInstanceService.cs b/src/SlpModularCms.Modules.Master/Services/CmsInstanceService.cs index c273db4..c3c8ae2 100644 --- a/src/SlpModularCms.Modules.Master/Services/CmsInstanceService.cs +++ b/src/SlpModularCms.Modules.Master/Services/CmsInstanceService.cs @@ -157,20 +157,56 @@ public class CmsInstanceService(MasterServiceDependencies deps) : ICmsInstanceSe try { var plainKey = deps.ApiKeyProtector.Unprotect(instance.ApiKey); - var registeredUrl = await deps.SlaveClient.GetRegisteredMasterUrlAsync(instance.Url, plainKey); + var contact = await deps.SlaveClient.GetRegisteredMasterUrlAsync(instance.Url, plainKey); - if (registeredUrl is null) + if (contact.Outcome is SlaveContactOutcome.Unreachable or SlaveContactOutcome.NotAProtocolEndpoint) { - deps.Logger.LogWarning("Slave {SlaveUrl} unreachable during integrity check.", instance.Url); + if (contact.Outcome == SlaveContactOutcome.NotAProtocolEndpoint) + { + // Distinct from "down": something is listening but does not serve + // /api/v1/master/*. Almost always a URL pointing at a frontend or an + // unrelated site rather than a CMS instance's API — which is invisible if + // this is reported as a generic contact failure. + deps.Logger.LogWarning( + "Slave {SlaveUrl} responded but does not serve the master/slave protocol (404). Is this the instance's API URL?", + instance.Url); + } + else + { + deps.Logger.LogWarning("Slave {SlaveUrl} unreachable during integrity check.", instance.Url); + } + instance.LastIntegrityCheckFailedAt = DateTimeOffset.UtcNow; deps.Repository.Update(instance); await deps.Repository.SaveChangesAsync(); continue; } - if (!string.Equals(registeredUrl, masterUrl, StringComparison.OrdinalIgnoreCase)) + // Two states need the same repair — the slave does not recognise us, or it + // recognises us under a stale master URL. + // + // Registering on a rejected key cannot hijack a slave that belongs to someone + // else: the slave accepts a registration only when it has none yet, and refuses + // any key that does not match an existing one (MasterAvailabilityService. + // RegisterAsync). So this succeeds exactly in the case worth recovering — a slave + // that was never registered, typically because the original registration call went + // to the wrong URL — and fails harmlessly otherwise. That guarantee lives on the + // slave, and MasterAvailabilityServiceTests locks it down. + var needsRegistration = contact.Outcome == SlaveContactOutcome.Unauthorized + || !string.Equals(contact.MasterUrl, masterUrl, StringComparison.OrdinalIgnoreCase); + + if (needsRegistration) { - deps.Logger.LogWarning("Slave {SlaveUrl} has wrong master URL '{RegisteredUrl}'; re-registering.", instance.Url, registeredUrl); + if (contact.Outcome == SlaveContactOutcome.Unauthorized) + { + deps.Logger.LogWarning( + "Slave {SlaveUrl} does not recognise this master; attempting registration.", instance.Url); + } + else + { + deps.Logger.LogWarning("Slave {SlaveUrl} has wrong master URL '{RegisteredUrl}'; re-registering.", instance.Url, contact.MasterUrl); + } + var reRegistered = await deps.SlaveClient.RegisterMasterAsync(instance.Url, plainKey, masterUrl); if (reRegistered) { @@ -179,6 +215,8 @@ public class CmsInstanceService(MasterServiceDependencies deps) : ICmsInstanceSe } else { + // A slave registered to a different master lands here, and stays here. + deps.Logger.LogWarning("Registration with slave {SlaveUrl} was refused.", instance.Url); instance.LastIntegrityCheckFailedAt = DateTimeOffset.UtcNow; } } diff --git a/src/SlpModularCms.Modules.Master/Services/ISlaveApiClient.cs b/src/SlpModularCms.Modules.Master/Services/ISlaveApiClient.cs index 0438711..daafdc1 100644 --- a/src/SlpModularCms.Modules.Master/Services/ISlaveApiClient.cs +++ b/src/SlpModularCms.Modules.Master/Services/ISlaveApiClient.cs @@ -4,5 +4,13 @@ public interface ISlaveApiClient { Task RegisterMasterAsync(string slaveUrl, string plainApiKey, string masterUrl); Task PushStatusAsync(string slaveUrl, string plainApiKey, bool isAvailable, string? disableMessage); - Task GetRegisteredMasterUrlAsync(string slaveUrl, string plainApiKey); + /// + /// Asks the slave which master it is registered to. + /// + /// + /// A result carrying why the call ended as it did, not just the URL. The integrity + /// check needs the distinction: an unreachable slave can only be retried, whereas one that + /// rejects our key may simply have no registration yet and can be recovered. + /// + Task GetRegisteredMasterUrlAsync(string slaveUrl, string plainApiKey); } diff --git a/src/SlpModularCms.Modules.Master/Services/SlaveApiClient.cs b/src/SlpModularCms.Modules.Master/Services/SlaveApiClient.cs index 9bd1d0f..1b300c0 100644 --- a/src/SlpModularCms.Modules.Master/Services/SlaveApiClient.cs +++ b/src/SlpModularCms.Modules.Master/Services/SlaveApiClient.cs @@ -1,3 +1,4 @@ +using System.Net; using System.Net.Http.Json; using System.Text.Json; @@ -41,7 +42,7 @@ public class SlaveApiClient(HttpClient httpClient) : ISlaveApiClient } } - public async Task GetRegisteredMasterUrlAsync(string slaveUrl, string plainApiKey) + public async Task GetRegisteredMasterUrlAsync(string slaveUrl, string plainApiKey) { try { @@ -49,15 +50,33 @@ public class SlaveApiClient(HttpClient httpClient) : ISlaveApiClient request.Headers.Add("X-Master-Api-Key", plainApiKey); var response = await httpClient.SendAsync(request); + + if (response.StatusCode == HttpStatusCode.Unauthorized) + { + // The slave answered and refused the key. Recoverable when it simply has no + // registration yet, so the caller gets to decide rather than seeing "unreachable". + return RegisteredMasterUrlResult.Unauthorized; + } + + if (response.StatusCode == HttpStatusCode.NotFound) + { + // Something is listening, but it does not serve /api/v1/master/*. Nearly always a + // URL pointing at a frontend or an unrelated site instead of a CMS instance's API. + return RegisteredMasterUrlResult.NotAProtocolEndpoint; + } + if (!response.IsSuccessStatusCode) - return null; + { + return RegisteredMasterUrlResult.Unreachable; + } var result = await response.Content.ReadFromJsonAsync(JsonOptions); - return result?.MasterUrl; + return RegisteredMasterUrlResult.Ok(result?.MasterUrl); } catch { - return null; + // No HTTP response at all: host down, DNS, TLS or timeout. + return RegisteredMasterUrlResult.Unreachable; } } diff --git a/src/SlpModularCms.Modules.Master/Services/SlaveContactResult.cs b/src/SlpModularCms.Modules.Master/Services/SlaveContactResult.cs new file mode 100644 index 0000000..e82657b --- /dev/null +++ b/src/SlpModularCms.Modules.Master/Services/SlaveContactResult.cs @@ -0,0 +1,45 @@ +namespace SlpModularCms.Modules.Master.Services; + +/// +/// How a call to a slave ended. +/// +/// +/// These four cases used to collapse into a single null, which meant the integrity check +/// could not tell "the slave is down" from "the slave does not recognise this master" — and only +/// the second is recoverable. It also meant a host that does not speak this protocol at all (a +/// frontend dev server, say) reported exactly the same as an unreachable one. +/// +public enum SlaveContactOutcome +{ + /// No HTTP response at all: host down, wrong host, DNS or TLS failure, timeout. + Unreachable, + + /// + /// The host answered, but not with this protocol — a 404 on /api/v1/master/*. Almost + /// always a URL pointing at something other than a CMS instance's API. + /// + NotAProtocolEndpoint, + + /// + /// The slave answered and rejected our API key. Either it has no registration yet, or it + /// belongs to a different master. + /// + Unauthorized, + + /// The slave answered and accepted our key. + Ok +} + +/// Result of asking a slave which master it is registered to. +/// How the call ended. +/// The registered master URL; only meaningful when . +public readonly record struct RegisteredMasterUrlResult(SlaveContactOutcome Outcome, string? MasterUrl) +{ + public static RegisteredMasterUrlResult Unreachable { get; } = new(SlaveContactOutcome.Unreachable, null); + + public static RegisteredMasterUrlResult NotAProtocolEndpoint { get; } = new(SlaveContactOutcome.NotAProtocolEndpoint, null); + + public static RegisteredMasterUrlResult Unauthorized { get; } = new(SlaveContactOutcome.Unauthorized, null); + + public static RegisteredMasterUrlResult Ok(string? masterUrl) => new(SlaveContactOutcome.Ok, masterUrl); +}