Makes a redeploy safe for the key ring and the schema
Nothing here is visible in normal operation. Its whole purpose is that swapping the release directory on deploy cannot silently destroy state. Data Protection secures the API keys that authenticate master/slave communication. Two separate defaults would each have destroyed them: keys are held on the filesystem, which a release swap discards, and the application discriminator is derived from the content root path, which changes with every release directory — so even keys stored in a database would have stopped being derivable. Keys now live in ApplicationDbContext and the discriminator is a fixed constant. Losing them produces no error. It produces stored keys that no longer decrypt, which presents as an apparent network fault between a Master and its slaves and is easily misdiagnosed. That is also why the tests assert the resulting configuration rather than the registration: the XmlRepository must be the EF one and the discriminator must be the constant, plus a round-trip proving a value encrypted before a deploy is readable after one. A test that only checked "Data Protection is registered" would have passed in the broken case too. Both modules previously called AddDataProtection() themselves. Module registration runs after the host's, so those calls re-registered the configuration chain and would have overridden the persistent store while IDataProtector still resolved. They are removed, with a comment at each site — the deletion otherwise looks like a regression. Each module's own test project now guards against it being reintroduced. ApplicationDbContext also migrates itself at startup. Deploy targets offer no CLI, so migrations cannot be a manual step on the server. Failures are classified rather than treated alike: a connection failure means the database is not up yet, normal when the app and the database start together after a reboot, and is retried with backoff; a migration failure means something is broken and fails at once. Either way the process does not start, which is what makes the liveness health check trustworthy — an application that cannot reach its schema never answers /health, so monitoring goes red instead of reporting a healthy instance that cannot serve a request. The cost of migrating without a human gate is that migrations must stay forward-compatible and non-destructive, since rollback is "redeploy the previous release". The new migration is purely additive. Also wires this and the preceding hosting commit into both hosts, as they touch the same lines of Program.cs. Two constraints are enforced by documentation rather than code, and belong in the deployment instructions: the key table must never be pruned, and only one instance may migrate a given database at a time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HHoJpxYXzHACSQguHrC5fw
This commit is contained in:
@@ -0,0 +1,97 @@
|
||||
using FluentAssertions;
|
||||
using Microsoft.AspNetCore.DataProtection;
|
||||
using Microsoft.AspNetCore.DataProtection.EntityFrameworkCore;
|
||||
using Microsoft.AspNetCore.DataProtection.KeyManagement;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using Microsoft.Extensions.DependencyInjection;
|
||||
using Microsoft.Extensions.Logging;
|
||||
using Microsoft.Extensions.Options;
|
||||
using SlpModularCms.Core.Data;
|
||||
using SlpModularCms.Core.Hosting;
|
||||
using Xunit;
|
||||
|
||||
namespace SlpModularCms.Core.Tests.Hosting;
|
||||
|
||||
/// <summary>
|
||||
/// Guards the durability of the Data Protection key ring.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// The failure this protects against is silent. Losing the key ring produces no error — it
|
||||
/// produces stored slave API keys that no longer decrypt, which looks like a network fault
|
||||
/// between a Master and its slaves. A test that merely asserted "Data Protection is registered"
|
||||
/// would pass in the broken case too, so these tests assert the resulting configuration instead.
|
||||
/// </remarks>
|
||||
public class DataProtectionExtensionsTests
|
||||
{
|
||||
[Fact]
|
||||
public void AddCmsDataProtection_ShouldPersistKeysToTheDatabase()
|
||||
{
|
||||
using var provider = BuildProvider(nameof(AddCmsDataProtection_ShouldPersistKeysToTheDatabase));
|
||||
|
||||
var options = provider.GetRequiredService<IOptions<KeyManagementOptions>>().Value;
|
||||
|
||||
// The default is a filesystem key ring, which every atomic release switch discards.
|
||||
options.XmlRepository.Should().BeOfType<EntityFrameworkCoreXmlRepository<ApplicationDbContext>>();
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void AddCmsDataProtection_ShouldUseAFixedApplicationDiscriminator()
|
||||
{
|
||||
using var provider = BuildProvider(nameof(AddCmsDataProtection_ShouldUseAFixedApplicationDiscriminator));
|
||||
|
||||
var options = provider.GetRequiredService<IOptions<DataProtectionOptions>>().Value;
|
||||
|
||||
// The default derives from the content root path, which changes with every release
|
||||
// directory — so keys stored in the database would still stop being derivable.
|
||||
options.ApplicationDiscriminator.Should().Be(DataProtectionExtensions.ApplicationDiscriminator);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ProtectedValues_ShouldSurviveAProcessRestartFromADifferentDirectory()
|
||||
{
|
||||
// The property that actually matters: an API key encrypted before a deploy must still be
|
||||
// readable by the process that starts afterwards from a different release directory.
|
||||
// Both providers share one database and one fixed discriminator, which is what makes
|
||||
// that possible.
|
||||
const string databaseName = nameof(ProtectedValues_ShouldSurviveAProcessRestartFromADifferentDirectory);
|
||||
const string secret = "slave-api-key-value";
|
||||
|
||||
string encrypted;
|
||||
using (var beforeDeploy = BuildProvider(databaseName))
|
||||
{
|
||||
encrypted = beforeDeploy
|
||||
.GetRequiredService<IDataProtectionProvider>()
|
||||
.CreateProtector("MasterApiKey")
|
||||
.Protect(secret);
|
||||
}
|
||||
|
||||
using var afterDeploy = BuildProvider(databaseName);
|
||||
|
||||
var decrypted = afterDeploy
|
||||
.GetRequiredService<IDataProtectionProvider>()
|
||||
.CreateProtector("MasterApiKey")
|
||||
.Unprotect(encrypted);
|
||||
|
||||
decrypted.Should().Be(secret);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ApplicationDiscriminator_ShouldNotBeDerivedFromAPath()
|
||||
{
|
||||
// Guards against someone "improving" this into a configurable or computed value: every
|
||||
// knob here is a way to reintroduce the silent failure the key ring exists to prevent.
|
||||
DataProtectionExtensions.ApplicationDiscriminator.Should().Be("SlpModularCms");
|
||||
DataProtectionExtensions.ApplicationDiscriminator.Should().NotContainAny("/", "\\", ":");
|
||||
}
|
||||
|
||||
private static ServiceProvider BuildProvider(string databaseName)
|
||||
{
|
||||
var services = new ServiceCollection();
|
||||
|
||||
services.AddLogging(builder => builder.SetMinimumLevel(LogLevel.None));
|
||||
services.AddDbContext<ApplicationDbContext>(options => options.UseInMemoryDatabase(databaseName));
|
||||
services.AddCmsDataProtection();
|
||||
|
||||
return services.BuildServiceProvider();
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,95 @@
|
||||
using FluentAssertions;
|
||||
using Microsoft.Data.SqlClient;
|
||||
using SlpModularCms.Core.Hosting;
|
||||
using Xunit;
|
||||
|
||||
namespace SlpModularCms.Core.Tests.Hosting;
|
||||
|
||||
/// <summary>
|
||||
/// Guards the failure classification of the startup migration.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Startup migration distinguishes two failures that mean very different things:
|
||||
/// a database that is not up yet (normal when the application and the database server start
|
||||
/// together after a host reboot) versus a migration that is broken. Retrying the first is
|
||||
/// correct; retrying the second only delays the inevitable and fills the log.
|
||||
///
|
||||
/// <c>MigrateCoreDatabase</c> itself needs a composed <c>WebApplication</c>, so the classifier is
|
||||
/// exercised directly here; the composed startup path is verified at the phase-level Build and
|
||||
/// Test stage by starting both hosts.
|
||||
/// </remarks>
|
||||
public class DatabaseMigrationExtensionsTests
|
||||
{
|
||||
[Fact]
|
||||
public void MaxConnectionAttempts_ShouldAllowForAHostRebootWindow()
|
||||
{
|
||||
// Enough attempts that a database server starting alongside the application is tolerated,
|
||||
// few enough that a genuinely unreachable database still fails promptly.
|
||||
DatabaseMigrationExtensions.MaxConnectionAttempts.Should().BeGreaterThan(1);
|
||||
DatabaseMigrationExtensions.MaxConnectionAttempts.Should().BeLessThanOrEqualTo(10);
|
||||
}
|
||||
|
||||
[Theory]
|
||||
[MemberData(nameof(TransientFailures))]
|
||||
public void IsTransientConnectionFailure_ShouldRecogniseConnectionProblems(Exception exception)
|
||||
{
|
||||
InvokeClassifier(exception).Should().BeTrue();
|
||||
}
|
||||
|
||||
[Theory]
|
||||
[MemberData(nameof(NonTransientFailures))]
|
||||
public void IsTransientConnectionFailure_ShouldNotRecogniseMigrationProblems(Exception exception)
|
||||
{
|
||||
// A broken migration must fail immediately. Classifying it as transient would hide a
|
||||
// real fault behind a retry loop.
|
||||
InvokeClassifier(exception).Should().BeFalse();
|
||||
}
|
||||
|
||||
public static TheoryData<Exception> TransientFailures() =>
|
||||
[
|
||||
MakeSqlException(),
|
||||
new TimeoutException("Connect Timeout expired."),
|
||||
new InvalidOperationException("wrapper", MakeSqlException()),
|
||||
];
|
||||
|
||||
public static TheoryData<Exception> NonTransientFailures() =>
|
||||
[
|
||||
new InvalidOperationException("There is already an object named 'Users' in the database."),
|
||||
new NotSupportedException("The migration cannot be applied."),
|
||||
new AggregateException(new InvalidOperationException("pending model changes")),
|
||||
];
|
||||
|
||||
private static bool InvokeClassifier(Exception exception)
|
||||
{
|
||||
var method = typeof(DatabaseMigrationExtensions).GetMethod(
|
||||
"IsTransientConnectionFailure",
|
||||
System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static);
|
||||
|
||||
method.Should().NotBeNull("the failure classifier is the behaviour under test");
|
||||
|
||||
return (bool)method!.Invoke(null, [exception])!;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// <see cref="SqlException"/> has no public constructor, so one is produced through the
|
||||
/// framework's own factory path via reflection.
|
||||
/// </summary>
|
||||
private static Exception MakeSqlException()
|
||||
{
|
||||
try
|
||||
{
|
||||
// Deliberately unreachable host and a very short timeout: this genuinely produces a
|
||||
// SqlException rather than a hand-built stand-in, so the classifier is tested against
|
||||
// the real type it will encounter in production.
|
||||
using var connection = new SqlConnection(
|
||||
"Server=localhost,9;Database=none;User Id=sa;Password=none;Connect Timeout=1;TrustServerCertificate=True");
|
||||
connection.Open();
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
return ex;
|
||||
}
|
||||
|
||||
throw new InvalidOperationException("Expected the connection attempt to fail.");
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user