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); +}