From 51eee1f8106d1cdefd1885ecb143dc121f339be2 Mon Sep 17 00:00:00 2001 From: giammin Date: Thu, 22 Oct 2026 22:52:14 +0200 Subject: [PATCH] feat!: support key deletion via IDeletableXmlRepository Add IDeletableDataProtectionStorage with Delete(keys). When the storage passed to PersistKeysToStorage implements it, the key repository also implements IDeletableXmlRepository, so XmlKeyManager.DeleteKeys works. Failures are logged and reported as false; the key XML is never logged. The SQL Server and PostgreSQL repositories delete the selected rows by Id, in deletion order, in a single transaction. Note: in ASP.NET Core 10.0 XmlKeyManager exposes DeleteKeys but does not implement IDeletableKeyManager; the README documents the usage. BREAKING CHANGE: IDbDataProtectionStorage now extends IDeletableDataProtectionStorage. Custom implementations of IDbDataProtectionStorage must implement Delete(IReadOnlyCollection). Closes #25 --- AGENTS.md | 10 +- README.md | 58 +++++++- .../PostgreSQLDataProtectionRepository.cs | 38 +++++ .../SQLServerDataProtectionRepository.cs | 39 ++++++ .../IDBDataProtectionStorage.cs | 2 +- .../DataProtectionStorageExtensions.cs | 8 +- .../DeletableStorageWrapper.cs | 96 +++++++++++++ .../IDeletableDataProtectionStorage.cs | 20 +++ .../LoggingExtensions.cs | 14 ++ .../StorageWrapper.cs | 33 +++-- .../DataProtectionStorageExtensionsTests.cs | 83 +++++++++++ .../DeletableStorageWrapperTests.cs | 131 ++++++++++++++++++ ...PostgreSqlDataProtectionRepositoryTests.cs | 80 +++++++++++ .../PostgreSQL/PostgreSqlEndToEndTests.cs | 19 +++ .../SqlServerDataProtectionRepositoryTests.cs | 71 ++++++++++ .../SQLServer/SqlServerEndToEndTests.cs | 19 +++ 16 files changed, 703 insertions(+), 18 deletions(-) create mode 100644 src/AspNetCore.DataProtection.CustomStorage/DeletableStorageWrapper.cs create mode 100644 src/AspNetCore.DataProtection.CustomStorage/IDeletableDataProtectionStorage.cs create mode 100644 tests/AspNetCore.DataProtection.CustomStorage.Tests/DataProtectionStorageExtensionsTests.cs create mode 100644 tests/AspNetCore.DataProtection.CustomStorage.Tests/DeletableStorageWrapperTests.cs diff --git a/AGENTS.md b/AGENTS.md index dc9dc5e..5f81b35 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -38,7 +38,9 @@ dotnet test --no-build -c Release --filter-method "*.StorageWrapperTests.StoreEl ``` ASP.NET Core Data Protection (IXmlRepository) └── StorageWrapper [Core: bridges IXmlRepository <-> IDataProtectionStorage] + (DeletableStorageWrapper also bridges IDeletableXmlRepository <-> IDeletableDataProtectionStorage) └── IDataProtectionStorage [Core: GetAll() / Insert()] + └── IDeletableDataProtectionStorage [Core: adds Delete()] └── IDbDataProtectionStorage [Dapper: adds InitializeDb() / GetAllAsync()] ├── SQLServerDataProtectionRepository [Dapper.SQLServer] └── PostgreSQLDataProtectionRepository [Dapper.PostgreSQL] @@ -46,7 +48,8 @@ ASP.NET Core Data Protection (IXmlRepository) ### Wiring -- **Core:** `PersistKeysToStorage()` registers a singleton `IConfigureOptions` that sets `XmlRepository` to a `StorageWrapper`. It does **not** register `TStorage`; the caller must. +- **Core:** `PersistKeysToStorage()` registers a singleton `IConfigureOptions` that sets `XmlRepository` to a `StorageWrapper`, or to a `DeletableStorageWrapper` when `TStorage` is assignable to `IDeletableDataProtectionStorage` (so `XmlKeyManager.CanDeleteKeys` reflects the storage). It does **not** register `TStorage`; the caller must. +- `DeletableStorageWrapper.DeleteElements` offers the parsed keys (malformed rows skipped), then passes the chosen `GetAll()` instances, ordered by `DeletionOrder`, to a single `Delete()` call. A storage exception is logged (`ErrorDeletingKeysFromStorage`) and returns `false`. Note: in ASP.NET Core 10.0, `XmlKeyManager` exposes `DeleteKeys` but does not implement `IDeletableKeyManager`. - `StorageWrapper` (internal) creates a new DI scope and resolves `TStorage` on every `GetAllElements` / `StoreElement` call, so scoped storage implementations work. It trims `FriendlyName`, normalizes empty or whitespace-only names to `null`, and wraps any `Insert` exception in `KeyInsertException`. `GetAllElements` logs and skips rows with malformed XML (`SkippingMalformedKey`) instead of failing the whole key ring. - Never log or expose key XML (it may contain unencrypted key material); this includes `XmlException` messages, which can quote the input. - **DB providers:** `PersistKeysWithDapperInSqlServer(...)` / `PersistKeysWithDapperInPostgreSQL(...)` configure `IOptions`, register the connection source as a singleton (`SqlConnectionFactory` / `NpgsqlDataSource`), register the repository as scoped `IDbDataProtectionStorage`, then call `PersistKeysToStorage()`. @@ -63,6 +66,7 @@ Both repositories use raw SQL via Dapper. They differ in naming conventions: | Unique index on friendly name | Filtered NONCLUSTERED (`WHERE ... IS NOT NULL`) | `CREATE UNIQUE INDEX IF NOT EXISTS` (NULLs allowed) | - The PostgreSQL provider sets the global `Dapper.DefaultTypeMap.MatchNamesWithUnderscores = true` so snake_case columns map to `DataProtectionKeyEntity`. This affects the whole host process (documented in the README). +- `Delete()` requires `DataProtectionKeyEntity` instances (as returned by `GetAll()`) and deletes by `Id`, row by row in order, in one transaction. - Schema and table names are interpolated directly into SQL strings, not parameterized or validated. - Defaults are in each provider's `Constants.cs`. Keep the table DDL in sync with the "Database schema" section of `README.md`. @@ -70,9 +74,9 @@ Both repositories use raw SQL via Dapper. They differ in naming conventions: Single test project: `tests/AspNetCore.DataProtection.CustomStorage.Tests` (xUnit v3, FluentAssertions, NSubstitute, Bogus). -- `StorageWrapperTests` and `DapperDataProtectionExtensionsTests` are pure unit tests. +- `StorageWrapperTests`, `DeletableStorageWrapperTests` and `DapperDataProtectionExtensionsTests` are pure unit tests. `DataProtectionStorageExtensionsTests` runs the real DI container and `XmlKeyManager` against in-memory storages. - `SQLServer/` and `PostgreSQL/` tests use an `IClassFixture` (`*ContainerFixture`) that starts a Testcontainers database, calls `InitializeDb()` with the default config, and exposes `ResetDatabaseAsync()` (Respawn) to clear data between tests. -- `*EndToEndTests` wire each provider through the public extension methods and a real DI container (`AddDataProtection().PersistKeysWithDapperIn...()`, `UseDapperDataProtection()`), then protect/unprotect, including across two service providers sharing one table. Each test uses its own table, so it does not depend on Respawn. +- `*EndToEndTests` wire each provider through the public extension methods and a real DI container (`AddDataProtection().PersistKeysWithDapperIn...()`, `UseDapperDataProtection()`), then protect/unprotect, including across two service providers sharing one table, and delete keys through `XmlKeyManager.DeleteKeys`. Each test uses its own table, so it does not depend on Respawn. ### CI/CD (`.github/workflows`) diff --git a/README.md b/README.md index 27dd8d3..7113315 100644 --- a/README.md +++ b/README.md @@ -33,6 +33,7 @@ The base package lets you plug any backend into the Data Protection key ring by - [Configuration](#configuration) - [Database schema](#database-schema) - [Custom storage backend](#custom-storage-backend) +- [Deleting keys](#deleting-keys) - [How it works](#how-it-works) - [Contributing](#contributing) - [License](#license) @@ -224,20 +225,71 @@ builder.Services.AddDataProtection() on each read/write, so you must register it yourself (any lifetime works). If an `Insert` throws, it is wrapped in a `KeyInsertException`. +To support [key deletion](#deleting-keys), implement `IDeletableDataProtectionStorage` instead: + +```csharp +public class MyDataProtectionStorage : IDeletableDataProtectionStorage +{ + // GetAll() and Insert() as above + + public void Delete(IReadOnlyCollection keys) + { + // delete every key, in the given order, or none of them (throw on failure) + } +} +``` + +The keys passed to `Delete` are instances previously returned by `GetAll()` on the same storage +instance, so a backend can return a derived record that carries its own identifier (as the Dapper +providers do with `DataProtectionKeyEntity.Id`) and use it to locate the rows to delete. + +## Deleting keys + +Deleting a key makes the data it protected permanently unreadable; Microsoft +[recommends not deleting keys](https://learn.microsoft.com/aspnet/core/security/data-protection/configuration/default-settings#delete-keys) +except in exceptional cases. When the storage supports it (both database providers do, as does any +`IDeletableDataProtectionStorage`), old keys can be deleted through the key manager: + +```csharp +using Microsoft.AspNetCore.DataProtection.KeyManagement; + +var keyManager = app.Services.GetRequiredService(); +if (keyManager is XmlKeyManager { CanDeleteKeys: true } xmlKeyManager) +{ + var yearAgo = DateTimeOffset.UtcNow.AddYears(-1); + if (!xmlKeyManager.DeleteKeys(key => key.ExpirationDate < yearAgo)) + { + // the deletion failed and no key was deleted; see the ErrorDeletingKeysFromStorage log entry + } +} +``` + +> **Note:** in ASP.NET Core 10.0, `XmlKeyManager` exposes `CanDeleteKeys` and `DeleteKeys` but does +> not implement `IDeletableKeyManager`, so a `keyManager is IDeletableKeyManager` check is `false` +> even when deletion is supported. + +The selected keys are deleted with a single `Delete` call. The database providers delete the rows +by `Id`, in deletion order, in one transaction: either every selected key is deleted or none is. +If the storage throws, the error is logged (event `ErrorDeletingKeysFromStorage`) and `DeleteKeys` +returns `false`. Each deleted key is logged by friendly name (event `DeletingKeyFromStorage`); the +key XML is never logged. A stored key whose XML is malformed is not offered for deletion. + ## How it works `PersistKeysToStorage()` registers an internal `IXmlRepository` adapter as the Data Protection key repository. The adapter maps the framework's XML-element calls to the simpler `IDataProtectionStorage` contract (`GetAll()` / `Insert()`), resolving `TStorage` from a scoped -service provider for every operation. +service provider for every operation. When `TStorage` implements `IDeletableDataProtectionStorage`, +the adapter also implements the framework's `IDeletableXmlRepository` and maps deletions to `Delete()`. When the key ring is loaded, a stored key whose XML is malformed is skipped and logged as an error (event `SkippingMalformedKey`, with the friendly name and the parse position; the XML itself is never logged). The remaining keys stay usable, but data protected with the skipped key can no longer be unprotected, and if no valid key remains, Data Protection generates a new one. -The database providers add a Dapper layer (`IDbDataProtectionStorage`) on top of that contract, -implementing the raw SQL for SQL Server and PostgreSQL, plus schema initialization +The database providers add a Dapper layer (`IDbDataProtectionStorage`, which extends +`IDeletableDataProtectionStorage`) on top of that contract, +implementing the raw SQL for SQL Server and PostgreSQL, plus key deletion, schema initialization (`InitializeDb()`) and an async read path (`GetAllAsync()`) that returns the stored `DataProtectionKeyEntity` rows, including `Id` and `InsertDate`, for tooling such as key auditing. diff --git a/src/AspNetCore.DataProtection.CustomStorage.Dapper.PostgreSQL/PostgreSQLDataProtectionRepository.cs b/src/AspNetCore.DataProtection.CustomStorage.Dapper.PostgreSQL/PostgreSQLDataProtectionRepository.cs index 9f6d846..acfa5d9 100644 --- a/src/AspNetCore.DataProtection.CustomStorage.Dapper.PostgreSQL/PostgreSQLDataProtectionRepository.cs +++ b/src/AspNetCore.DataProtection.CustomStorage.Dapper.PostgreSQL/PostgreSQLDataProtectionRepository.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Threading.Tasks; using Dapper; using Microsoft.Extensions.Options; @@ -65,6 +66,43 @@ public void Insert(DataProtectionKey key) key); } + /// + /// + /// The rows are deleted by , in the given order, in a single transaction. + /// A row that no longer exists is not an error. + /// + /// a key is not a returned by + public void Delete(IReadOnlyCollection keys) + { + var ids = GetIds(keys); + if (ids.Count == 0) + { + return; + } + + using var connection = _dataSource.OpenConnection(); + using var transaction = connection.BeginTransaction(); + // Dapper executes the statement once per id, in order. + connection.Execute( + $""" + DELETE FROM {_config.SchemaName}.{_config.TableName} + WHERE id = @{nameof(DataProtectionKeyEntity.Id)} + """, + ids.Select(id => new { Id = id }), + transaction); + transaction.Commit(); + } + + private static List GetIds(IReadOnlyCollection keys) + { + ArgumentNullException.ThrowIfNull(keys); + return keys + .Select(key => key as DataProtectionKeyEntity ?? throw new ArgumentException( + $"Every key must be a {nameof(DataProtectionKeyEntity)} returned by {nameof(GetAll)}.", nameof(keys))) + .Select(entity => entity.Id) + .ToList(); + } + /// public void InitializeDb() { diff --git a/src/AspNetCore.DataProtection.CustomStorage.Dapper.SQLServer/SQLServerDataProtectionRepository.cs b/src/AspNetCore.DataProtection.CustomStorage.Dapper.SQLServer/SQLServerDataProtectionRepository.cs index e0444a3..7b1a2a1 100644 --- a/src/AspNetCore.DataProtection.CustomStorage.Dapper.SQLServer/SQLServerDataProtectionRepository.cs +++ b/src/AspNetCore.DataProtection.CustomStorage.Dapper.SQLServer/SQLServerDataProtectionRepository.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Threading.Tasks; using Dapper; using Microsoft.Extensions.Options; @@ -64,6 +65,44 @@ public void Insert(DataProtectionKey key) """, key); } + /// + /// + /// The rows are deleted by , in the given order, in a single transaction. + /// A row that no longer exists is not an error. + /// + /// a key is not a returned by + public void Delete(IReadOnlyCollection keys) + { + var ids = GetIds(keys); + if (ids.Count == 0) + { + return; + } + + using var connection = _connectionFactory.CreateConnection(); + connection.Open(); + using var transaction = connection.BeginTransaction(); + // Dapper executes the statement once per id, in order. + connection.Execute( + $""" + DELETE FROM [{_config.SchemaName}].[{_config.TableName}] + WHERE [{nameof(DataProtectionKeyEntity.Id)}] = @{nameof(DataProtectionKeyEntity.Id)} + """, + ids.Select(id => new { Id = id }), + transaction); + transaction.Commit(); + } + + private static List GetIds(IReadOnlyCollection keys) + { + ArgumentNullException.ThrowIfNull(keys); + return keys + .Select(key => key as DataProtectionKeyEntity ?? throw new ArgumentException( + $"Every key must be a {nameof(DataProtectionKeyEntity)} returned by {nameof(GetAll)}.", nameof(keys))) + .Select(entity => entity.Id) + .ToList(); + } + /// public void InitializeDb() { diff --git a/src/AspNetCore.DataProtection.CustomStorage.Dapper/IDBDataProtectionStorage.cs b/src/AspNetCore.DataProtection.CustomStorage.Dapper/IDBDataProtectionStorage.cs index bae64c0..a50a5a8 100644 --- a/src/AspNetCore.DataProtection.CustomStorage.Dapper/IDBDataProtectionStorage.cs +++ b/src/AspNetCore.DataProtection.CustomStorage.Dapper/IDBDataProtectionStorage.cs @@ -4,7 +4,7 @@ namespace AspNetCore.DataProtection.CustomStorage.Dapper; /// -public interface IDbDataProtectionStorage:IDataProtectionStorage +public interface IDbDataProtectionStorage:IDeletableDataProtectionStorage { /// /// create the storage/table if needed diff --git a/src/AspNetCore.DataProtection.CustomStorage/DataProtectionStorageExtensions.cs b/src/AspNetCore.DataProtection.CustomStorage/DataProtectionStorageExtensions.cs index 9dccb40..324e61a 100644 --- a/src/AspNetCore.DataProtection.CustomStorage/DataProtectionStorageExtensions.cs +++ b/src/AspNetCore.DataProtection.CustomStorage/DataProtectionStorageExtensions.cs @@ -13,7 +13,9 @@ namespace AspNetCore.DataProtection.CustomStorage; public static class DataProtectionStorageExtensions { /// - /// Configures the data protection system to persist keys to a custom storage + /// Configures the data protection system to persist keys to a custom storage. + /// When implements , + /// keys can also be deleted through . /// /// The instance to modify. /// The value . @@ -25,7 +27,9 @@ public static IDataProtectionBuilder PersistKeysToStorage(this IDataPr var loggerFactory = services.GetService() ?? NullLoggerFactory.Instance; return new ConfigureOptions(options => { - options.XmlRepository = new StorageWrapper(services, loggerFactory); + options.XmlRepository = typeof(TStorage).IsAssignableTo(typeof(IDeletableDataProtectionStorage)) + ? new DeletableStorageWrapper(services, loggerFactory) + : new StorageWrapper(services, loggerFactory); }); }); diff --git a/src/AspNetCore.DataProtection.CustomStorage/DeletableStorageWrapper.cs b/src/AspNetCore.DataProtection.CustomStorage/DeletableStorageWrapper.cs new file mode 100644 index 0000000..53f1a6e --- /dev/null +++ b/src/AspNetCore.DataProtection.CustomStorage/DeletableStorageWrapper.cs @@ -0,0 +1,96 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Xml.Linq; +using Microsoft.AspNetCore.DataProtection.Repositories; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; + +namespace AspNetCore.DataProtection.CustomStorage; + +/// +/// handle retrieving, storing and deleting the keys for the DataProtection with a storage +/// that implements +/// +/// the registered storage type; it must implement +internal class DeletableStorageWrapper : StorageWrapper, IDeletableXmlRepository + where TStorage : IDataProtectionStorage +{ + /// + /// + /// + /// + /// + /// + /// does not implement + public DeletableStorageWrapper(IServiceProvider services, ILoggerFactory loggerFactory) + : base(services, loggerFactory) + { + if (!typeof(TStorage).IsAssignableTo(typeof(IDeletableDataProtectionStorage))) + { + throw new ArgumentException( + $"'{typeof(TStorage).Name}' does not implement '{nameof(IDeletableDataProtectionStorage)}'.", + nameof(TStorage)); + } + } + + /// + /// + /// A stored key whose XML is malformed is logged and left out of the snapshot, so it is never deleted. + /// The chosen keys are deleted with a single call; + /// if it fails, the error is logged and false is returned. + /// + public bool DeleteElements(Action> chooseElements) + { + ArgumentNullException.ThrowIfNull(chooseElements); + using var scope = _services.CreateScope(); + var storage = (IDeletableDataProtectionStorage)scope.ServiceProvider.GetRequiredService(); + + var deletableElements = new List(); + foreach (var key in storage.GetAll()) + { + var element = TryParseKey(key); + if (element is not null) + { + deletableElements.Add(new DeletableElement(key, element)); + } + } + + chooseElements(deletableElements); + + var keysToDelete = deletableElements + .Where(e => e.DeletionOrder.HasValue) + .OrderBy(e => e.DeletionOrder.GetValueOrDefault()) + .Select(e => e.Key) + .ToList(); + if (keysToDelete.Count == 0) + { + return true; + } + + foreach (var key in keysToDelete) + { + _logger.LogDeletingKeyFromStorage(key.FriendlyName, typeof(TStorage).Name); + } + + try + { + storage.Delete(keysToDelete); + return true; + } + catch (Exception e) + { + _logger.LogErrorDeletingKeysFromStorage(e, keysToDelete.Count, typeof(TStorage).Name); + return false; + } + } + + private sealed class DeletableElement(DataProtectionKey key, XElement element) : IDeletableElement + { + public DataProtectionKey Key { get; } = key; + + public XElement Element { get; } = element; + + public int? DeletionOrder { get; set; } + } +} diff --git a/src/AspNetCore.DataProtection.CustomStorage/IDeletableDataProtectionStorage.cs b/src/AspNetCore.DataProtection.CustomStorage/IDeletableDataProtectionStorage.cs new file mode 100644 index 0000000..9edfb6f --- /dev/null +++ b/src/AspNetCore.DataProtection.CustomStorage/IDeletableDataProtectionStorage.cs @@ -0,0 +1,20 @@ +using System.Collections.Generic; + +namespace AspNetCore.DataProtection.CustomStorage; + +/// +/// Storage/repository interface that also supports the deletion of stored keys. +/// When the storage passed to +/// implements this interface, keys can be deleted through +/// . +/// +public interface IDeletableDataProtectionStorage : IDataProtectionStorage +{ + /// + /// delete the given keys, in the given order. + /// The keys are instances previously returned by on the same instance. + /// Either every key is deleted or none is: on failure the implementation must throw and leave the storage unchanged. + /// + /// the keys to delete, in deletion order + void Delete(IReadOnlyCollection keys); +} diff --git a/src/AspNetCore.DataProtection.CustomStorage/LoggingExtensions.cs b/src/AspNetCore.DataProtection.CustomStorage/LoggingExtensions.cs index 3be2235..127dd8c 100644 --- a/src/AspNetCore.DataProtection.CustomStorage/LoggingExtensions.cs +++ b/src/AspNetCore.DataProtection.CustomStorage/LoggingExtensions.cs @@ -35,4 +35,18 @@ internal static partial class LoggingExtensions /// [LoggerMessage(5, LogLevel.Error, "Skipping key '{FriendlyName}': its XML is malformed (line {LineNumber}, position {LinePosition}).", EventName = "SkippingMalformedKey")] public static partial void LogSkippingMalformedKey(this ILogger logger, string? friendlyName, int lineNumber, int linePosition); + + /// + /// Logs an information message for each key about to be deleted from storage. + /// The key XML is never logged because it may contain unencrypted key material. + /// + [LoggerMessage(6, LogLevel.Information, "Deleting key '{FriendlyName}' with '{storage}'.", EventName = "DeletingKeyFromStorage")] + public static partial void LogDeletingKeyFromStorage(this ILogger logger, string? friendlyName, string storage); + + /// + /// Logs an error message when deleting keys from storage fails. + /// The key XML is never logged because it may contain unencrypted key material. + /// + [LoggerMessage(7, LogLevel.Error, "Error deleting {Count} key(s) with '{storage}'.", EventName = "ErrorDeletingKeysFromStorage")] + public static partial void LogErrorDeletingKeysFromStorage(this ILogger logger, Exception exception, int count, string storage); } diff --git a/src/AspNetCore.DataProtection.CustomStorage/StorageWrapper.cs b/src/AspNetCore.DataProtection.CustomStorage/StorageWrapper.cs index e05605f..74cad67 100644 --- a/src/AspNetCore.DataProtection.CustomStorage/StorageWrapper.cs +++ b/src/AspNetCore.DataProtection.CustomStorage/StorageWrapper.cs @@ -15,8 +15,8 @@ namespace AspNetCore.DataProtection.CustomStorage; /// internal class StorageWrapper : IXmlRepository where TStorage : IDataProtectionStorage { - private readonly IServiceProvider _services; - private readonly ILogger _logger; + protected readonly IServiceProvider _services; + protected readonly ILogger _logger; /// /// /// @@ -44,20 +44,35 @@ public IReadOnlyCollection GetAllElements() var elements = ImmutableArray.CreateBuilder(); foreach (var key in storage.GetAll()) { - _logger.ReadingXmlFromKey(key.FriendlyName); - try + var element = TryParseKey(key); + if (element is not null) { - elements.Add(XElement.Parse(key.Xml)); - } - catch (XmlException e) - { - _logger.LogSkippingMalformedKey(key.FriendlyName, e.LineNumber, e.LinePosition); + elements.Add(element); } } return elements.ToImmutable(); } + /// + /// parse the XML of a stored key + /// + /// the stored key + /// the parsed element, or null when the XML is malformed (the key is logged and skipped) + protected XElement? TryParseKey(DataProtectionKey key) + { + _logger.ReadingXmlFromKey(key.FriendlyName); + try + { + return XElement.Parse(key.Xml); + } + catch (XmlException e) + { + _logger.LogSkippingMalformedKey(key.FriendlyName, e.LineNumber, e.LinePosition); + return null; + } + } + /// public void StoreElement(XElement element, string? friendlyName) { diff --git a/tests/AspNetCore.DataProtection.CustomStorage.Tests/DataProtectionStorageExtensionsTests.cs b/tests/AspNetCore.DataProtection.CustomStorage.Tests/DataProtectionStorageExtensionsTests.cs new file mode 100644 index 0000000..c623e44 --- /dev/null +++ b/tests/AspNetCore.DataProtection.CustomStorage.Tests/DataProtectionStorageExtensionsTests.cs @@ -0,0 +1,83 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using FluentAssertions; +using Microsoft.AspNetCore.DataProtection; +using Microsoft.AspNetCore.DataProtection.KeyManagement; +using Microsoft.AspNetCore.DataProtection.Repositories; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using Xunit; + +namespace AspNetCore.DataProtection.CustomStorage.Tests; + +public class DataProtectionStorageExtensionsTests +{ + [Fact] + public void PersistKeysToStorage_StorageNotDeletable_RepositoryDoesNotSupportDeletion() + { + using var provider = BuildProvider(); + + provider.GetRequiredService>().Value.XmlRepository + .Should().NotBeNull().And.NotBeAssignableTo(); + provider.GetRequiredService().As().CanDeleteKeys + .Should().BeFalse(); + } + + [Fact] + public void PersistKeysToStorage_StorageDeletable_RepositorySupportsDeletion() + { + using var provider = BuildProvider(); + + provider.GetRequiredService>().Value.XmlRepository + .Should().BeAssignableTo(); + provider.GetRequiredService().As().CanDeleteKeys + .Should().BeTrue(); + } + + [Fact] + public void DeleteKeys_StorageDeletable_DeletesMatchingKeysFromStorage() + { + using var provider = BuildProvider(); + // XmlKeyManager exposes DeleteKeys but does not implement IDeletableKeyManager. + var keyManager = (XmlKeyManager)provider.GetRequiredService(); + var now = DateTimeOffset.UtcNow; + var kept = keyManager.CreateNewKey(now, now.AddDays(90)); + var deleted = keyManager.CreateNewKey(now, now.AddDays(90)); + + var result = keyManager.DeleteKeys(key => key.KeyId == deleted.KeyId); + + result.Should().BeTrue(); + keyManager.GetAllKeys().Select(k => k.KeyId).Should().Equal(kept.KeyId); + provider.GetRequiredService().Keys.Should().ContainSingle() + .Which.Xml.Should().Contain(kept.KeyId.ToString()); + } + + private static ServiceProvider BuildProvider() where TStorage : class, IDataProtectionStorage + { + var services = new ServiceCollection(); + services.AddSingleton(); + services.AddDataProtection().PersistKeysToStorage(); + return services.BuildServiceProvider(); + } + + private class InMemoryStorage : IDataProtectionStorage + { + public List Keys { get; } = []; + + public IEnumerable GetAll() => Keys.ToList(); + + public void Insert(DataProtectionKey key) => Keys.Add(key); + } + + private sealed class DeletableInMemoryStorage : InMemoryStorage, IDeletableDataProtectionStorage + { + public void Delete(IReadOnlyCollection keys) + { + foreach (var key in keys) + { + Keys.Remove(key); + } + } + } +} diff --git a/tests/AspNetCore.DataProtection.CustomStorage.Tests/DeletableStorageWrapperTests.cs b/tests/AspNetCore.DataProtection.CustomStorage.Tests/DeletableStorageWrapperTests.cs new file mode 100644 index 0000000..f15b0dc --- /dev/null +++ b/tests/AspNetCore.DataProtection.CustomStorage.Tests/DeletableStorageWrapperTests.cs @@ -0,0 +1,131 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Xml.Linq; +using FluentAssertions; +using Microsoft.AspNetCore.DataProtection.Repositories; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using NSubstitute; +using Xunit; + +namespace AspNetCore.DataProtection.CustomStorage.Tests; + +public class DeletableStorageWrapperTests +{ + private const string KeyMaterial = "c2VjcmV0LWtleS1tYXRlcmlhbA=="; + + private readonly IServiceProvider _services; + private readonly IDeletableDataProtectionStorage _storage; + private readonly CapturingLoggerFactory _loggerFactory = new(); + private readonly DeletableStorageWrapper _sut; + + private readonly DataProtectionKey _first = CreateKey("key-1"); + private readonly DataProtectionKey _second = CreateKey("key-2"); + private readonly DataProtectionKey _third = CreateKey("key-3"); + + public DeletableStorageWrapperTests() + { + _services = Substitute.For(); + _storage = Substitute.For(); + + var scope = Substitute.For(); + scope.ServiceProvider.Returns(_services); + var serviceScopeFactory = Substitute.For(); + serviceScopeFactory.CreateScope().Returns(scope); + + _services.GetService(typeof(IServiceScopeFactory)).Returns(serviceScopeFactory); + _services.GetService(typeof(IDeletableDataProtectionStorage)).Returns(_storage); + _storage.GetAll().Returns([_first, _second, _third]); + + _sut = new DeletableStorageWrapper(_services, _loggerFactory); + } + + [Fact] + public void DeleteElements_ChooseElementsIsNull_ThrowsArgumentNullException() + { + _sut.Invoking(x => x.DeleteElements(null!)) + .Should().Throw(); + } + + [Fact] + public void DeleteElements_SnapshotContainsStoredElements() + { + IReadOnlyCollection? snapshot = null; + + _sut.DeleteElements(elements => snapshot = elements); + + snapshot.Should().NotBeNull(); + snapshot!.Select(e => e.Element.ToString(SaveOptions.DisableFormatting)) + .Should().Equal(_first.Xml, _second.Xml, _third.Xml); + snapshot.Should().OnlyContain(e => e.DeletionOrder == null); + } + + [Fact] + public void DeleteElements_RowWithMalformedXml_ExcludedFromSnapshot() + { + var malformed = new DataProtectionKey { FriendlyName = "key-malformed", Xml = "" }; + _storage.GetAll().Returns([malformed, _first]); + IReadOnlyCollection? snapshot = null; + + _sut.DeleteElements(elements => snapshot = elements); + + snapshot.Should().ContainSingle() + .Which.Element.ToString(SaveOptions.DisableFormatting).Should().Be(_first.Xml); + } + + [Fact] + public void DeleteElements_NoElementChosen_ReturnsTrueWithoutDeleting() + { + var result = _sut.DeleteElements(_ => { }); + + result.Should().BeTrue(); + _storage.DidNotReceiveWithAnyArgs().Delete(null!); + } + + [Fact] + public void DeleteElements_ElementsChosen_DeletesStoredKeysInDeletionOrder() + { + var result = _sut.DeleteElements(elements => + { + var list = elements.ToList(); + list[0].DeletionOrder = 2; + list[2].DeletionOrder = 1; + }); + + result.Should().BeTrue(); + _storage.Received(1).Delete(Arg.Is>(keys => + keys.Count == 2 && ReferenceEquals(keys.First(), _third) && ReferenceEquals(keys.Last(), _first))); + } + + [Fact] + public void DeleteElements_ElementsChosen_LogsDeletedKeysWithoutKeyXml() + { + _sut.DeleteElements(elements => elements.First().DeletionOrder = 1); + + _loggerFactory.Entries.Should().ContainSingle(e => e.EventId.Name == "DeletingKeyFromStorage") + .Which.Contains(_first.FriendlyName!).Should().BeTrue(); + _loggerFactory.Entries.Should().NotContain(e => e.Contains(KeyMaterial)); + } + + [Fact] + public void DeleteElements_StorageFails_ReturnsFalseAndLogsErrorWithoutKeyXml() + { + var storageException = new InvalidOperationException("delete failed"); + _storage.WhenForAnyArgs(x => x.Delete(null!)).Throw(storageException); + + var result = _sut.DeleteElements(elements => elements.First().DeletionOrder = 1); + + result.Should().BeFalse(); + _loggerFactory.Entries.Should().ContainSingle(e => e.Level == LogLevel.Error) + .Which.Exception.Should().BeSameAs(storageException); + _loggerFactory.Entries.Should().NotContain(e => e.Contains(KeyMaterial)); + } + + private static DataProtectionKey CreateKey(string friendlyName) => new() + { + FriendlyName = friendlyName, + Xml = new XElement("key", new XAttribute("id", friendlyName), new XElement("masterKey", KeyMaterial)) + .ToString(SaveOptions.DisableFormatting) + }; +} diff --git a/tests/AspNetCore.DataProtection.CustomStorage.Tests/PostgreSQL/PostgreSqlDataProtectionRepositoryTests.cs b/tests/AspNetCore.DataProtection.CustomStorage.Tests/PostgreSQL/PostgreSqlDataProtectionRepositoryTests.cs index ed901d9..fc9c960 100644 --- a/tests/AspNetCore.DataProtection.CustomStorage.Tests/PostgreSQL/PostgreSqlDataProtectionRepositoryTests.cs +++ b/tests/AspNetCore.DataProtection.CustomStorage.Tests/PostgreSQL/PostgreSqlDataProtectionRepositoryTests.cs @@ -127,4 +127,84 @@ public void InitializeDb_CustomSchemaAndTable_PersistsKeysInConfiguredTable() } _sut.GetAll().Should().NotContain(x => x.FriendlyName == key.FriendlyName); } + + [Fact] + public void Delete_NullKeys_ThrowsArgumentNullException() + { + _sut.Invoking(x => x.Delete(null!)) + .Should().Throw(); + } + + [Fact] + public async Task Delete_KeysReturnedByGetAll_DeletesOnlyThoseKeys() + { + await _fixture.ResetDatabaseAsync(); + _sut.Insert(new DataProtectionKey { FriendlyName = "DeleteKept", Xml = "xml1" }); + _sut.Insert(new DataProtectionKey { FriendlyName = "DeleteRemoved", Xml = "xml2" }); + _sut.Insert(new DataProtectionKey { FriendlyName = null, Xml = "xml3" }); + var keysToDelete = _sut.GetAll().Where(x => x.FriendlyName != "DeleteKept").ToList(); + + _sut.Delete(keysToDelete); + + _sut.GetAll().Should().ContainSingle() + .Which.FriendlyName.Should().Be("DeleteKept"); + } + + [Fact] + public async Task Delete_KeyNotReturnedByGetAll_ThrowsArgumentExceptionAndDeletesNothing() + { + await _fixture.ResetDatabaseAsync(); + _sut.Insert(new DataProtectionKey { FriendlyName = "DeleteKept", Xml = "xml1" }); + var stored = _sut.GetAll().Single(); + + _sut.Invoking(x => x.Delete([stored, new DataProtectionKey { FriendlyName = "DeleteKept", Xml = "xml1" }])) + .Should().Throw(); + + _sut.GetAll().Should().ContainSingle(); + } + + [Fact] + public async Task Delete_KeyAlreadyDeleted_Succeeds() + { + await _fixture.ResetDatabaseAsync(); + _sut.Insert(new DataProtectionKey { FriendlyName = "DeleteTwice", Xml = "xml1" }); + var stored = _sut.GetAll().ToList(); + _sut.Delete(stored); + + _sut.Invoking(x => x.Delete(stored)).Should().NotThrow(); + } + + [Fact] + public void Delete_LaterDeletionFails_RollsBackEarlierDeletions() + { + var tableName = $"keys_{Guid.NewGuid().ToString("N")[..12]}"; + var options = Substitute.For>(); + options.Value.Returns(new DapperDataProtectionConfig + { + SchemaName = Constants.DefaultSchema, + TableName = tableName + }); + var sut = new PostgreSQLDataProtectionRepository(_fixture.NpgsqlDataSource, options); + sut.InitializeDb(); + sut.Insert(new DataProtectionKey { FriendlyName = "DeleteFirst", Xml = "xml1" }); + sut.Insert(new DataProtectionKey { FriendlyName = "DeleteFails", Xml = "xml2" }); + using (var connection = _fixture.NpgsqlDataSource.OpenConnection()) + { + connection.Execute( + $""" + CREATE FUNCTION {Constants.DefaultSchema}.fail_delete_{tableName}() RETURNS trigger AS $$ + BEGIN + IF OLD.friendly_name = 'DeleteFails' THEN RAISE EXCEPTION 'delete failed'; END IF; + RETURN OLD; + END $$ LANGUAGE plpgsql; + CREATE TRIGGER tr_{tableName}_fail_delete BEFORE DELETE ON {Constants.DefaultSchema}.{tableName} + FOR EACH ROW EXECUTE FUNCTION {Constants.DefaultSchema}.fail_delete_{tableName}(); + """); + } + var keys = sut.GetAll().OrderBy(x => x.FriendlyName == "DeleteFails").ToList(); + + sut.Invoking(x => x.Delete(keys)).Should().Throw().WithMessage("*delete failed*"); + + sut.GetAll().Select(x => x.FriendlyName).Should().BeEquivalentTo("DeleteFirst", "DeleteFails"); + } } diff --git a/tests/AspNetCore.DataProtection.CustomStorage.Tests/PostgreSQL/PostgreSqlEndToEndTests.cs b/tests/AspNetCore.DataProtection.CustomStorage.Tests/PostgreSQL/PostgreSqlEndToEndTests.cs index 1de6675..583dd9b 100644 --- a/tests/AspNetCore.DataProtection.CustomStorage.Tests/PostgreSQL/PostgreSqlEndToEndTests.cs +++ b/tests/AspNetCore.DataProtection.CustomStorage.Tests/PostgreSQL/PostgreSqlEndToEndTests.cs @@ -1,10 +1,12 @@ using System; using System.Collections.Generic; +using System.Linq; using AspNetCore.DataProtection.CustomStorage.Dapper; using AspNetCore.DataProtection.CustomStorage.Dapper.PostgreSQL; using Dapper; using FluentAssertions; using Microsoft.AspNetCore.DataProtection; +using Microsoft.AspNetCore.DataProtection.KeyManagement; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; @@ -54,6 +56,23 @@ public void Protect_UnprotectWithSecondServiceProviderSharingDatabase_RoundTrips GetStoredFriendlyNames(tableName).Should().ContainSingle(); } + [Fact] + public void DeleteKeys_KeySelected_DeletesItFromTable() + { + var tableName = NewTableName(); + using var services = BuildServiceProvider(tableName); + // XmlKeyManager exposes DeleteKeys but does not implement IDeletableKeyManager. + var keyManager = (XmlKeyManager)services.GetRequiredService(); + var now = DateTimeOffset.UtcNow; + var kept = keyManager.CreateNewKey(now, now.AddDays(90)); + var deleted = keyManager.CreateNewKey(now, now.AddDays(90)); + + keyManager.DeleteKeys(key => key.KeyId == deleted.KeyId).Should().BeTrue(); + + keyManager.GetAllKeys().Select(k => k.KeyId).Should().Equal(kept.KeyId); + GetStoredFriendlyNames(tableName).Should().Equal($"key-{kept.KeyId:D}"); + } + // PostgreSQL folds unquoted identifiers to lower case; keep the name short enough for derived index names. private static string NewTableName() => $"keys_{Guid.NewGuid().ToString("N")[..12]}"; diff --git a/tests/AspNetCore.DataProtection.CustomStorage.Tests/SQLServer/SqlServerDataProtectionRepositoryTests.cs b/tests/AspNetCore.DataProtection.CustomStorage.Tests/SQLServer/SqlServerDataProtectionRepositoryTests.cs index a93d253..7058459 100644 --- a/tests/AspNetCore.DataProtection.CustomStorage.Tests/SQLServer/SqlServerDataProtectionRepositoryTests.cs +++ b/tests/AspNetCore.DataProtection.CustomStorage.Tests/SQLServer/SqlServerDataProtectionRepositoryTests.cs @@ -103,6 +103,77 @@ public async Task GetAllAsync_KeysInserted_ReturnsEntitiesWithIdAndInsertDate() entities.Select(x => x.Id).Should().OnlyHaveUniqueItems(); } + [Fact] + public void Delete_NullKeys_ThrowsArgumentNullException() + { + _sut.Invoking(x => x.Delete(null!)) + .Should().Throw(); + } + + [Fact] + public async Task Delete_KeysReturnedByGetAll_DeletesOnlyThoseKeys() + { + await _fixture.ResetDatabaseAsync(); + _sut.Insert(new DataProtectionKey { FriendlyName = "DeleteKept", Xml = "xml1" }); + _sut.Insert(new DataProtectionKey { FriendlyName = "DeleteRemoved", Xml = "xml2" }); + _sut.Insert(new DataProtectionKey { FriendlyName = null, Xml = "xml3" }); + var keysToDelete = _sut.GetAll().Where(x => x.FriendlyName != "DeleteKept").ToList(); + + _sut.Delete(keysToDelete); + + _sut.GetAll().Should().ContainSingle() + .Which.FriendlyName.Should().Be("DeleteKept"); + } + + [Fact] + public async Task Delete_KeyNotReturnedByGetAll_ThrowsArgumentExceptionAndDeletesNothing() + { + await _fixture.ResetDatabaseAsync(); + _sut.Insert(new DataProtectionKey { FriendlyName = "DeleteKept", Xml = "xml1" }); + var stored = _sut.GetAll().Single(); + + _sut.Invoking(x => x.Delete([stored, new DataProtectionKey { FriendlyName = "DeleteKept", Xml = "xml1" }])) + .Should().Throw(); + + _sut.GetAll().Should().ContainSingle(); + } + + [Fact] + public async Task Delete_KeyAlreadyDeleted_Succeeds() + { + await _fixture.ResetDatabaseAsync(); + _sut.Insert(new DataProtectionKey { FriendlyName = "DeleteTwice", Xml = "xml1" }); + var stored = _sut.GetAll().ToList(); + _sut.Delete(stored); + + _sut.Invoking(x => x.Delete(stored)).Should().NotThrow(); + } + + [Fact] + public void Delete_LaterDeletionFails_RollsBackEarlierDeletions() + { + var tableName = NewTableName(); + var sut = CreateSut(tableName); + sut.InitializeDb(); + sut.Insert(new DataProtectionKey { FriendlyName = "DeleteFirst", Xml = "xml1" }); + sut.Insert(new DataProtectionKey { FriendlyName = "DeleteFails", Xml = "xml2" }); + using (var connection = _fixture.SqlConnectionFactory.CreateConnection()) + { + connection.Execute( + $""" + CREATE TRIGGER [{Constants.DefaultSchema}].[TR_{tableName}_FailDelete] + ON [{Constants.DefaultSchema}].[{tableName}] AFTER DELETE AS + IF EXISTS(SELECT 1 FROM deleted WHERE [FriendlyName] = N'DeleteFails') + THROW 50000, N'delete failed', 1; + """); + } + var keys = sut.GetAll().OrderBy(x => x.FriendlyName == "DeleteFails").ToList(); + + sut.Invoking(x => x.Delete(keys)).Should().Throw().WithMessage("*delete failed*"); + + sut.GetAll().Select(x => x.FriendlyName).Should().BeEquivalentTo("DeleteFirst", "DeleteFails"); + } + [Fact] public void InitializeDb_TableExistsWithoutIndex_CreatesIndex() { diff --git a/tests/AspNetCore.DataProtection.CustomStorage.Tests/SQLServer/SqlServerEndToEndTests.cs b/tests/AspNetCore.DataProtection.CustomStorage.Tests/SQLServer/SqlServerEndToEndTests.cs index ae43ec5..80cbfb2 100644 --- a/tests/AspNetCore.DataProtection.CustomStorage.Tests/SQLServer/SqlServerEndToEndTests.cs +++ b/tests/AspNetCore.DataProtection.CustomStorage.Tests/SQLServer/SqlServerEndToEndTests.cs @@ -1,10 +1,12 @@ using System; using System.Collections.Generic; +using System.Linq; using AspNetCore.DataProtection.CustomStorage.Dapper; using AspNetCore.DataProtection.CustomStorage.Dapper.SQLServer; using Dapper; using FluentAssertions; using Microsoft.AspNetCore.DataProtection; +using Microsoft.AspNetCore.DataProtection.KeyManagement; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; @@ -55,6 +57,23 @@ public void Protect_UnprotectWithSecondServiceProviderSharingDatabase_RoundTrips GetStoredFriendlyNames(tableName).Should().ContainSingle(); } + [Fact] + public void DeleteKeys_KeySelected_DeletesItFromTable() + { + var tableName = NewTableName(); + using var services = BuildServiceProvider(tableName); + // XmlKeyManager exposes DeleteKeys but does not implement IDeletableKeyManager. + var keyManager = (XmlKeyManager)services.GetRequiredService(); + var now = DateTimeOffset.UtcNow; + var kept = keyManager.CreateNewKey(now, now.AddDays(90)); + var deleted = keyManager.CreateNewKey(now, now.AddDays(90)); + + keyManager.DeleteKeys(key => key.KeyId == deleted.KeyId).Should().BeTrue(); + + keyManager.GetAllKeys().Select(k => k.KeyId).Should().Equal(kept.KeyId); + GetStoredFriendlyNames(tableName).Should().Equal($"key-{kept.KeyId:D}"); + } + private static string NewTableName() => $"Keys_{Guid.NewGuid():N}"; private ServiceProvider BuildServiceProvider(string tableName)