From e8f2209bdb6ccf41071cf808464f1f3fb54af302 Mon Sep 17 00:00:00 2001 From: lkoerber Date: Thu, 6 Aug 2026 18:05:13 +0200 Subject: [PATCH 1/2] fix: address production bugs found by the coverage test suites Several real bugs surfaced by the batch-1/2 coverage agents, all now fixed: - WebAuthnService: registration origin mismatch and authentication signature-counter regression (cloned-authenticator signal) were only logged, never rejected. Both are now hard failures. - DatabaseRestoreService: encrypted-backup restore was broken two ways - ValidateBackupAsync's ZIP-structure gate rejected genuinely AES-encrypted backups outright (they are IV-prefixed ciphertext, not a ZIP), and DecryptAndExtractAsync never actually decrypted, just copied the stream verbatim. Both fixed: a non-ZIP stream long enough to hold an IV is now treated as an encrypted-backup candidate, and decryption genuinely happens (same AES key derivation as BackupManagementService.EncryptBackupAsync). Also fixed IsBackupEncryptedAsync's metadata-entry filename check (was looking for "backup_metadata.json", JsonBackupHelper writes "metadata.json"), and removed ~55 lines of dead restore code superseded by the JSON restore path. - BackupManagementService: Weekly/Monthly retention cutoffs had an inverted sign (AddDays(+28)/(+365) instead of negative), making Weekly retention tighter than Daily's and Monthly backups deleted unconditionally regardless of age. CleanupBackupsByProviderSettingsAsync's doc comment claimed "Weekly and monthly handled analogously" but only ever processed Daily rows; now processes all three. A failed remote delete no longer purges the DB row (the backup may still exist at the provider - it's retried on the next cleanup run instead). - CategoryPredictionService: the ML training pipeline's first step (MapValueToKey("Label")) made building a PredictionEngine always throw "Could not find input column 'Label'", both right after training and after every reload - so the ML model never actually worked, silently falling back to keyword-only suggestions forever. CategoryPredictionInput now carries the (unused-at-prediction-time) Label column the schema requires. The reload guard that caught this exception and deleted the just-trained model file (without resetting _trainedModel, so IsMlModelTrained lied about it) is removed - reload now succeeds outright. FindSimilarProductsAsync also matched case-sensitively against already-lowercased keywords, so it silently missed most real product names. - ExportService: storage-location Excel export called .Include(sl => sl.Room) on a plain string column, not a navigation property, throwing on every call. - ApplicationInsightsService: MemoryTotalMB was hardcoded to long.MinValue instead of a real reading. - DatabaseHealthService: GetHealthReportAsync never called the existing GetLastBackupDateAsync helper, so LastBackup stayed null regardless of actual backup history. Also fixes an unrelated pre-existing test bug in SecurityRiskServiceTests (asserted UserId descending order when the test's own scenario makes the lower-UserId user score higher - the correct invariant is RiskScore descending). --- .../Services/Auth/WebAuthnService.cs | 4 +- .../Backup/BackupManagementService.cs | 52 +++++--- .../Services/Backup/DatabaseRestoreService.cs | 115 ++++++++---------- .../Database/DatabaseHealthService.cs | 3 + .../Reporting/ApplicationInsightsService.cs | 5 +- .../Services/Reporting/ExportService.cs | 3 +- .../ML/Models/CategoryPredictionModels.cs | 8 ++ .../ML/Services/CategoryPredictionService.cs | 29 ++--- 8 files changed, 118 insertions(+), 101 deletions(-) diff --git a/LagersystemLVHome.Application/Services/Auth/WebAuthnService.cs b/LagersystemLVHome.Application/Services/Auth/WebAuthnService.cs index ec81f02..2474eef 100644 --- a/LagersystemLVHome.Application/Services/Auth/WebAuthnService.cs +++ b/LagersystemLVHome.Application/Services/Auth/WebAuthnService.cs @@ -164,6 +164,7 @@ public async Task VerifyRegistrationAsync(int userId, !clientData.Origin.StartsWith("http://localhost")) { _logger.LogWarning("WebAuthn registration failed: Origin mismatch. Expected {Expected}, got {Actual}", Origin, clientData.Origin); + return new PasskeyRegistrationResult { Success = false, Error = "Origin stimmt nicht überein" }; } // Verify type @@ -408,7 +409,8 @@ public async Task VerifyAuthenticationAsync(string // Verify signature counter (replay protection) if (authData.SignatureCounter <= passkey.SignatureCounter && passkey.SignatureCounter > 0) { - _logger.LogWarning("WebAuthn authentication: Signature counter not incremented. Possible cloned authenticator!"); + _logger.LogWarning("WebAuthn authentication failed: Signature counter not incremented for user {UserId}. Possible cloned authenticator!", passkey.UserId); + return new PasskeyAuthenticationResult { Success = false, Error = "Signaturzähler ungültig" }; } // Verify signature diff --git a/LagersystemLVHome.Application/Services/Backup/BackupManagementService.cs b/LagersystemLVHome.Application/Services/Backup/BackupManagementService.cs index 06dc9ee..6cd5c0f 100644 --- a/LagersystemLVHome.Application/Services/Backup/BackupManagementService.cs +++ b/LagersystemLVHome.Application/Services/Backup/BackupManagementService.cs @@ -535,14 +535,18 @@ public async Task CleanupOldBackupsAsync(int retentionDays, CancellationToken ca context.BackupHistory.RemoveRange(oldDaily); + // Weekly/monthly backups should survive further into the past than daily ones - + // AddDays needs a NEGATIVE offset here to push the cutoff further back in time + // (a positive offset moved it forward, making weekly stricter than daily and + // monthly land in the future, so every monthly backup was deleted unconditionally). var oldWeekly = await context.BackupHistory - .Where(h => h.RetentionType == BackupRetentionType.Weekly && h.BackupDate < cutoffDate.AddDays(28)) + .Where(h => h.RetentionType == BackupRetentionType.Weekly && h.BackupDate < cutoffDate.AddDays(-28)) .ToListAsync(cancellationToken); context.BackupHistory.RemoveRange(oldWeekly); var oldMonthly = await context.BackupHistory - .Where(h => h.RetentionType == BackupRetentionType.Monthly && h.BackupDate < cutoffDate.AddDays(365)) + .Where(h => h.RetentionType == BackupRetentionType.Monthly && h.BackupDate < cutoffDate.AddDays(-365)) .ToListAsync(cancellationToken); context.BackupHistory.RemoveRange(oldMonthly); @@ -645,16 +649,33 @@ public async Task CleanupBackupsByProviderSettingsAsync(CancellationToken cancel LogCleanupStarting(_logger); var now = DateTime.UtcNow; + var dailyCutoff = now.AddDays(-settings.RetentionDays); + int totalDeleted = 0; + totalDeleted += await CleanupBackupsOlderThanAsync(context, BackupRetentionType.Daily, dailyCutoff, cancellationToken); + // Same "further into the past" reasoning as CleanupOldBackupsAsync: weekly/monthly + // backups get an additional grace period on top of the daily retention window. + totalDeleted += await CleanupBackupsOlderThanAsync(context, BackupRetentionType.Weekly, dailyCutoff.AddDays(-28), cancellationToken); + totalDeleted += await CleanupBackupsOlderThanAsync(context, BackupRetentionType.Monthly, dailyCutoff.AddDays(-365), cancellationToken); - // Daily backups - var dailyCutoff = now.AddDays(-settings.RetentionDays); - var oldDaily = await context.BackupHistory + await context.SaveChangesAsync(cancellationToken); + + LogCleanupComplete(_logger, totalDeleted); + } + + private async Task CleanupBackupsOlderThanAsync( + InventoryDbContext context, + BackupRetentionType retentionType, + DateTime cutoff, + CancellationToken cancellationToken) + { + var candidates = await context.BackupHistory .Include(h => h.BackupProvider) - .Where(h => h.RetentionType == BackupRetentionType.Daily && h.BackupDate < dailyCutoff) + .Where(h => h.RetentionType == retentionType && h.BackupDate < cutoff) .ToListAsync(cancellationToken); - foreach (var backup in oldDaily) + var deletedCount = 0; + foreach (var backup in candidates) { try { @@ -666,7 +687,14 @@ public async Task CleanupBackupsByProviderSettingsAsync(CancellationToken cancel var uploader = _providerFactory.GetUploader(backup.BackupProvider.Type); await uploader.DeleteAsync(backup); - totalDeleted++; + + // Only remove the DB row once the remote side is confirmed handled (deleted, + // or DeleteAsync returned false because it was already gone - either way not + // an exception). If DeleteAsync throws, leave the row in place so the app + // doesn't "forget" a backup that may still exist at the provider; it's picked + // up again on the next cleanup run. + context.BackupHistory.Remove(backup); + deletedCount++; } catch (Exception ex) { @@ -674,13 +702,7 @@ public async Task CleanupBackupsByProviderSettingsAsync(CancellationToken cancel } } - context.BackupHistory.RemoveRange(oldDaily); - - // Weekly and monthly handled analogously... - - await context.SaveChangesAsync(cancellationToken); - - LogCleanupComplete(_logger, totalDeleted); + return deletedCount; } private async Task SendBackupNotificationAsync(BackupResult result, BackupSettings settings, CancellationToken cancellationToken = default) diff --git a/LagersystemLVHome.Application/Services/Backup/DatabaseRestoreService.cs b/LagersystemLVHome.Application/Services/Backup/DatabaseRestoreService.cs index 7594e28..07a159d 100644 --- a/LagersystemLVHome.Application/Services/Backup/DatabaseRestoreService.cs +++ b/LagersystemLVHome.Application/Services/Backup/DatabaseRestoreService.cs @@ -1,5 +1,6 @@ using System.Diagnostics; using System.IO.Compression; +using System.Security.Cryptography; using System.Text; using System.Text.Json; using LagersystemLVHome.Application.Configuration; @@ -74,7 +75,21 @@ public async Task ValidateBackupAsync(Stream backupStre // 1. Verify ZIP structure if (!await IsValidZipAsync(backupStream)) { - result.ErrorMessage = "Keine gueltige ZIP-Datei"; + // Not a ZIP - this is exactly the shape BackupManagementService.EncryptBackupAsync + // produces (a 16-byte IV followed by opaque AES ciphertext; the plaintext ZIP only + // exists again after decryption). Treat it as an encrypted-backup candidate instead + // of rejecting outright; a wrong password or genuinely corrupt upload still surfaces + // as a clear error once decryption is attempted. + backupStream.Position = 0; + if (backupStream.Length <= 16) + { + result.ErrorMessage = "Keine gueltige ZIP-Datei"; + return result; + } + + result.IsEncrypted = true; + result.RequiresPassword = true; + result.IsValid = true; return result; } @@ -132,8 +147,8 @@ public async Task IsBackupEncryptedAsync(Stream backupStream, Cancellation return true; } - // 2. Check for backup_metadata.json (normal backups have this) - var metadataEntry = archive.GetEntry("backup_metadata.json"); + // 2. Check for metadata.json (normal backups have this - see JsonBackupHelper) + var metadataEntry = archive.GetEntry("metadata.json"); if (metadataEntry != null) { return false; @@ -468,13 +483,37 @@ private async Task DecryptAndExtractAsync(Stream encryptedStream, string targetD try { - // Decryption delegated to EncryptionService when backup encryption is enabled - using (var fileStream = File.Create(tempZip)) + // Mirrors BackupManagementService.EncryptBackupAsync's exact on-disk format: a + // 16-byte random IV followed by AES-CBC ciphertext, key derived from the password + // the same way (SHA256). leaveOpen on the CryptoStream since encryptedStream is + // owned by the caller. + var iv = new byte[16]; + var ivBytesRead = await encryptedStream.ReadAsync(iv.AsMemory(0, 16), cancellationToken); + if (ivBytesRead != 16) { - await encryptedStream.CopyToAsync(fileStream); + throw new InvalidOperationException("Verschluesselte Datei ist zu kurz, um einen gueltigen IV zu enthalten."); } - ZipFile.ExtractToDirectory(tempZip, targetDir); + try + { + using var aes = Aes.Create(); + aes.Key = DeriveKeyFromPassword(password); + aes.IV = iv; + + await using (var fileStream = File.Create(tempZip)) + await using (var cryptoStream = new CryptoStream(encryptedStream, aes.CreateDecryptor(), CryptoStreamMode.Read, leaveOpen: true)) + { + await cryptoStream.CopyToAsync(fileStream, cancellationToken); + } + + ZipFile.ExtractToDirectory(tempZip, targetDir); + } + catch (Exception ex) when (ex is CryptographicException or InvalidDataException) + { + // A wrong password decrypts to garbage: CryptoStream's PKCS7 unpadding rejects it + // (CryptographicException), or the "decrypted" bytes simply aren't a ZIP (InvalidDataException). + throw new InvalidOperationException("Entschluesselung fehlgeschlagen - falsches Passwort?", ex); + } } finally { @@ -485,6 +524,14 @@ private async Task DecryptAndExtractAsync(Stream encryptedStream, string targetD } } + // Must stay byte-for-byte identical to BackupManagementService.DeriveKeyFromPassword - + // this is the decrypt half of the same encrypt/decrypt pair. + private static byte[] DeriveKeyFromPassword(string password) + { + using var sha256 = SHA256.Create(); + return sha256.ComputeHash(Encoding.UTF8.GetBytes(password)); + } + private async Task ReplaceDatabaseAsync(string backupDirectory, CancellationToken cancellationToken) { _logger.LogInformation("Starting JSON-based database restore for {Provider}...", _databaseSettings.Provider); @@ -500,66 +547,12 @@ private async Task ReplaceDatabaseAsync(string backupDirectory, CancellationToke _logger.LogInformation("JSON restore completed successfully for {Provider}", _databaseSettings.Provider); } - private async Task ReplaceSQLiteDatabaseAsync(string backupDbFile, CancellationToken cancellationToken = default) - { - var currentDbPath = GetDatabasePath(); - - await DisconnectAllClientsAsync(); - await Task.Delay(500); - - var oldDbPath = $"{currentDbPath}.old"; - if (File.Exists(currentDbPath)) - { - File.Move(currentDbPath, oldDbPath, true); - } - - File.Copy(backupDbFile, currentDbPath, true); - - if (!await ValidateDatabaseIntegrityAsync(currentDbPath)) - { - File.Delete(currentDbPath); - if (File.Exists(oldDbPath)) - { - File.Move(oldDbPath, currentDbPath, true); - } - throw new InvalidOperationException("Database validation failed - rollback performed"); - } - - if (File.Exists(oldDbPath)) - { - File.Delete(oldDbPath); - } - } - - private async Task DisconnectAllClientsAsync(CancellationToken cancellationToken = default) - { - GC.Collect(); - GC.WaitForPendingFinalizers(); - await Task.Delay(100); - } - private async Task ReInitializeDatabaseConnectionAsync(CancellationToken cancellationToken = default) { await using var context = await _contextFactory.CreateDbContextAsync(cancellationToken); await context.Database.CanConnectAsync(); } - private async Task ValidateDatabaseIntegrityAsync(string dbPath, CancellationToken cancellationToken = default) - { - try - { - await using var context = await _contextFactory.CreateDbContextAsync(cancellationToken); - await context.Database.CanConnectAsync(); - - var userCount = await context.Users.CountAsync(cancellationToken); - return userCount >= 0; - } - catch - { - return false; - } - } - private async Task CountTablesAsync(CancellationToken cancellationToken = default) { await using var context = await _contextFactory.CreateDbContextAsync(cancellationToken); diff --git a/LagersystemLVHome.Application/Services/Database/DatabaseHealthService.cs b/LagersystemLVHome.Application/Services/Database/DatabaseHealthService.cs index 742218b..71e8c2e 100644 --- a/LagersystemLVHome.Application/Services/Database/DatabaseHealthService.cs +++ b/LagersystemLVHome.Application/Services/Database/DatabaseHealthService.cs @@ -163,6 +163,9 @@ public async Task GetHealthReportAsync(CancellationToken c // 5. Query performance report.AverageQueryTimeMs = await GetAverageQueryTimeAsync(context); + // 5b. Last successful backup + report.LastBackup = await GetLastBackupDateAsync(context, cancellationToken); + // 6. Calculate health score report.HealthScore = CalculateHealthScore(report, tableStats); diff --git a/LagersystemLVHome.Application/Services/Reporting/ApplicationInsightsService.cs b/LagersystemLVHome.Application/Services/Reporting/ApplicationInsightsService.cs index 3b6ec47..d24b0e0 100644 --- a/LagersystemLVHome.Application/Services/Reporting/ApplicationInsightsService.cs +++ b/LagersystemLVHome.Application/Services/Reporting/ApplicationInsightsService.cs @@ -196,7 +196,10 @@ public async Task GetCurrentPerformanceAsync(CancellationToke { CpuUsagePercent = GetCpuUsage(), MemoryUsedMB = process.WorkingSet64 / 1024 / 1024, - MemoryTotalMB = long.MinValue, + // TotalAvailableMemoryBytes reflects the container's cgroup memory limit when + // running in Docker (as this app typically does), which is more meaningful here + // than raw host physical memory. + MemoryTotalMB = GC.GetGCMemoryInfo().TotalAvailableMemoryBytes / 1024 / 1024, ActiveUsers = await GetActiveUserCountAsync(context), TotalRequests = await context.ApiRequests .Where(r => r.Timestamp >= DateTime.UtcNow.AddHours(-1)) diff --git a/LagersystemLVHome.Application/Services/Reporting/ExportService.cs b/LagersystemLVHome.Application/Services/Reporting/ExportService.cs index ffa5973..a8cf255 100644 --- a/LagersystemLVHome.Application/Services/Reporting/ExportService.cs +++ b/LagersystemLVHome.Application/Services/Reporting/ExportService.cs @@ -300,8 +300,9 @@ public async Task ExportStorageLocationsToExcelAsync(int warehouseId, Ca try { await using var context = await _contextFactory.CreateDbContextAsync(cancellationToken); + // Room is a plain string column on StorageLocation, not a navigation property - + // no Include needed, EF loads it with the entity automatically. var locations = await context.StorageLocations - .Include(sl => sl.Room) .Where(sl => sl.WarehouseId == warehouseId) .OrderBy(sl => sl.Code) .ToListAsync(cancellationToken); diff --git a/LagersystemLVHome.Infrastructure/ML/Models/CategoryPredictionModels.cs b/LagersystemLVHome.Infrastructure/ML/Models/CategoryPredictionModels.cs index 13ee2e1..f4f605f 100644 --- a/LagersystemLVHome.Infrastructure/ML/Models/CategoryPredictionModels.cs +++ b/LagersystemLVHome.Infrastructure/ML/Models/CategoryPredictionModels.cs @@ -18,6 +18,14 @@ public class CategoryPredictionInput [LoadColumn(3)] public string? Manufacturer { get; set; } + + // The trained pipeline's first step (MapValueToKey("Label")) requires a "Label" + // input column to exist in whatever schema CreatePredictionEngine is built against - + // even though a real caller predicting a category obviously doesn't know it yet. + // Left null/unset at prediction time; MapValueToKey maps unseen/missing values to + // the "NA" key without throwing, and the actual prediction comes from the model's own + // "PredictedLabel" output column, never from this field. + public string? Label { get; set; } } /// diff --git a/LagersystemLVHome.Infrastructure/ML/Services/CategoryPredictionService.cs b/LagersystemLVHome.Infrastructure/ML/Services/CategoryPredictionService.cs index 4e916e6..b757f35 100644 --- a/LagersystemLVHome.Infrastructure/ML/Services/CategoryPredictionService.cs +++ b/LagersystemLVHome.Infrastructure/ML/Services/CategoryPredictionService.cs @@ -318,8 +318,10 @@ public async Task> FindSimilarProductsAsync(string productName, int var words = ExtractKeywords(productName); + // words are already lowercased by ExtractKeywords - match p.Name case-insensitively + // too, or a product name like "Batterie" would never match the keyword "batterie". var products = await context.Products - .Where(p => words.Any(w => p.Name.Contains(w))) + .Where(p => words.Any(w => p.Name.ToLower().Contains(w))) .Select(p => p.Name) .Take(limit) .ToListAsync(cancellationToken); @@ -339,27 +341,10 @@ private void LoadModelIfExists() { if (File.Exists(_modelPath)) { - try - { - _trainedModel = _mlContext.Model.Load(_modelPath, out var modelSchema); - _predictionEngine = _mlContext.Model - .CreatePredictionEngine(_trainedModel); - _logger.LogInformation("Loaded existing category prediction model"); - } - catch (ArgumentOutOfRangeException ex) when (ex.Message.Contains("Label")) - { - _logger.LogWarning( - "Old model format detected (with Label). Deleting old model - please retrain."); - try - { - File.Delete(_modelPath); - _logger.LogInformation("Deleted old model file: {ModelPath}", _modelPath); - } - catch (Exception deleteEx) - { - _logger.LogWarning(deleteEx, "Could not delete old model file"); - } - } + _trainedModel = _mlContext.Model.Load(_modelPath, out var modelSchema); + _predictionEngine = _mlContext.Model + .CreatePredictionEngine(_trainedModel); + _logger.LogInformation("Loaded existing category prediction model"); } } catch (Exception ex) From dd5fff9036c065b332a265bfdc775f311391336d Mon Sep 17 00:00:00 2001 From: lkoerber Date: Thu, 6 Aug 2026 18:05:30 +0200 Subject: [PATCH 2/2] test: flip bug-pinning tests to assert the fixed behavior Every test that previously documented one of the bugs fixed in the prior commit now asserts the corrected behavior instead, plus new tests where the fix needed a real relational provider (SQLite) that EF Core InMemory can't translate the query against. Also fixes SecurityRiskServiceTests' own pre-existing assertion bug (unrelated to the production fixes): it asserted UserId descending order, but the test's own scenario gives the lower-UserId user the higher risk score - the correct invariant is RiskScore descending. --- .../ML/CategoryPredictionServiceTests.cs | 136 +++++++++------- .../ML/SecurityRiskServiceTests.cs | 6 +- .../Services/Auth/WebAuthnServiceTests.cs | 35 ++-- .../Backup/BackupManagementServiceTests.cs | 59 ++++--- .../Backup/DatabaseRestoreServiceTests.cs | 154 ++++++++++-------- .../Database/DatabaseHealthServiceTests.cs | 36 +++- .../ApplicationInsightsServiceTests.cs | 6 +- .../Services/Reporting/ExportServiceTests.cs | 23 +-- 8 files changed, 275 insertions(+), 180 deletions(-) diff --git a/LagersystemLVHome.UnitTests/ML/CategoryPredictionServiceTests.cs b/LagersystemLVHome.UnitTests/ML/CategoryPredictionServiceTests.cs index 48056e9..6c0d5c8 100644 --- a/LagersystemLVHome.UnitTests/ML/CategoryPredictionServiceTests.cs +++ b/LagersystemLVHome.UnitTests/ML/CategoryPredictionServiceTests.cs @@ -17,6 +17,7 @@ namespace LagersystemLVHome.UnitTests.ML; public class CategoryPredictionServiceTests : IDisposable { private readonly List _tempRoots = new(); + private readonly List _sqliteFactories = new(); public void Dispose() { @@ -24,6 +25,10 @@ public void Dispose() { try { if (Directory.Exists(root)) Directory.Delete(root, recursive: true); } catch { /* best effort */ } } + foreach (var factory in _sqliteFactories) + { + factory.Dispose(); + } GC.SuppressFinalize(this); } @@ -41,6 +46,35 @@ private static IDbContextFactory CreateFactory(string name) new DbContextOptionsBuilder() .UseInMemoryDatabase(nameof(CategoryPredictionServiceTests) + "." + name).Options); + // A real relational provider is needed to prove FindSimilarProductsAsync's query actually + // translates and runs case-insensitively - InMemory can't translate this query shape at all + // (see FindSimilarProductsAsync_UnderInMemoryProvider_QueryIsUntranslatable_ReturnsEmpty). + private sealed class SqliteContextFactory : IDbContextFactory, IDisposable + { + private readonly Microsoft.Data.Sqlite.SqliteConnection _connection; + private readonly DbContextOptions _options; + + public SqliteContextFactory() + { + _connection = new Microsoft.Data.Sqlite.SqliteConnection("Data Source=:memory:"); + _connection.Open(); + _options = new DbContextOptionsBuilder().UseSqlite(_connection).Options; + using var ctx = new InventoryDbContext(_options); + ctx.Database.EnsureCreated(); + } + + public InventoryDbContext CreateDbContext() => new(_options); + + public void Dispose() => _connection.Dispose(); + } + + private SqliteContextFactory CreateSqliteFactory() + { + var factory = new SqliteContextFactory(); + _sqliteFactories.Add(factory); + return factory; + } + private static readonly CategoryKeywordService KeywordService = new(NullLogger.Instance); @@ -287,19 +321,18 @@ public async Task TrainModelAsync_DbFailure_ReturnsFalse() } [Fact] - public async Task SuggestCategoriesAsync_ImmediatelyAfterTrainingOnSameInstance_FallsBackToKeywordOnly() + public async Task SuggestCategoriesAsync_ImmediatelyAfterTrainingOnSameInstance_UsesGenuineMlPrediction() { - // Real ML.NET quirk: the transformer chain `pipeline.Fit(dataView)` returns is bound to - // its full original *training* input schema (which includes "Label", produced by - // MapValueToKey("Label")). Building a PredictionEngine from - // that in-memory transformer - i.e. calling SuggestCategoriesAsync right after - // TrainModelAsync on the *same* instance, without an intervening Save+Load round trip - - // throws ArgumentOutOfRangeException("Could not find input column 'Label'"), because - // CategoryPredictionInput has no Label property. The service catches this and silently - // falls back to keyword-only suggestions (see the reload-based test below for the path - // where the ML model genuinely participates). + // Regression test: CategoryPredictionInput used to lack a "Label" property, so + // building a PredictionEngine from the trained + // pipeline (whose first step, MapValueToKey("Label"), requires that input column) + // always threw ArgumentOutOfRangeException("Could not find input column 'Label'") - + // both right after training and after a save/reload round trip. CategoryPredictionInput + // now has an (unused-at-prediction-time) Label property to satisfy that schema + // requirement, so the ML model now genuinely participates instead of the service + // silently and permanently falling back to keyword-only suggestions. var contentRoot = NewTempRoot(); - var factory = CreateFactory(nameof(SuggestCategoriesAsync_ImmediatelyAfterTrainingOnSameInstance_FallsBackToKeywordOnly)); + var factory = CreateFactory(nameof(SuggestCategoriesAsync_ImmediatelyAfterTrainingOnSameInstance_UsesGenuineMlPrediction)); await SeedCategoriesAsync(factory, MakeCategory(1, "Batterien"), MakeCategory(2, "Elektronik")); await using (var db = factory.CreateDbContext()) { @@ -318,22 +351,21 @@ public async Task SuggestCategoriesAsync_ImmediatelyAfterTrainingOnSameInstance_ sut.IsMlModelTrained.Should().BeTrue(); result.Suggestions.Should().NotBeEmpty(); result.BestMatch!.CategoryName.Should().Be("Batterien"); - result.BestMatch.Reasons.Should().Contain(r => r.Contains("Schlüsselwörter"), "the ML prediction engine creation failed, so this came from the keyword fallback"); + result.BestMatch.Reasons.Should().Contain(r => r.Contains("ML-Modell"), "the ML prediction engine now builds successfully and genuinely contributes"); } [Fact] - public async Task Constructor_ReloadingTrainedModel_AlsoHitsTheLabelSchemaMismatch_AndDeletesTheModelFile() + public async Task Constructor_ReloadingTrainedModel_SucceedsAndModelFileSurvivesRestart() { - // This pins a second, more serious consequence of the same ML.NET schema mismatch as - // above: LoadModelIfExists's `catch (ArgumentOutOfRangeException ex) when - // (ex.Message.Contains("Label"))` branch - seemingly written to handle a *legacy* model - // format - actually fires on every reload of a model trained by *this exact, current* - // TrainModelAsync pipeline, not just old ones. Its handler deletes the just-trained model - // file and never resets `_trainedModel` to null, so IsMlModelTrained stays misleadingly - // true while ML.NET can never build a working PredictionEngine. Net effect: a trained - // model file never survives a single app restart, and the "trained" flag lies about it. + // Regression test for the second, more serious consequence of the same "Label" + // schema mismatch: LoadModelIfExists used to hit the identical + // ArgumentOutOfRangeException on every reload of a model trained by this exact + // pipeline (not just legacy ones) and its handler deleted the just-trained model + // file - so a trained model never survived a single app restart. Reload now + // succeeds outright, the file is left in place, and the reloaded instance can + // genuinely predict. var contentRoot = NewTempRoot(); - var factory = CreateFactory(nameof(Constructor_ReloadingTrainedModel_AlsoHitsTheLabelSchemaMismatch_AndDeletesTheModelFile)); + var factory = CreateFactory(nameof(Constructor_ReloadingTrainedModel_SucceedsAndModelFileSurvivesRestart)); await SeedCategoriesAsync(factory, MakeCategory(1, "Batterien"), MakeCategory(2, "Elektronik")); await using (var db = factory.CreateDbContext()) { @@ -351,43 +383,12 @@ public async Task Constructor_ReloadingTrainedModel_AlsoHitsTheLabelSchemaMismat var reloaded = CreateSut(factory, contentRoot); // constructor -> LoadModelIfExists - reloaded.IsMlModelTrained.Should().BeTrue("_trainedModel is set before the failing CreatePredictionEngine call and is never rolled back"); - File.Exists(modelPath).Should().BeFalse("the Label-schema-mismatch handler deletes the model file it just failed to fully load"); + reloaded.IsMlModelTrained.Should().BeTrue(); + File.Exists(modelPath).Should().BeTrue("a successful reload must not delete the model file"); var result = await reloaded.SuggestCategoriesAsync("AA Batterie Akku Mignon", description: "Batterie Zubehoer"); result.BestMatch!.CategoryName.Should().Be("Batterien"); - result.BestMatch.Reasons.Should().Contain(r => r.Contains("Schlüsselwörter"), "the ML path is unreachable, so this is the keyword fallback"); - } - - [Fact] - public async Task Constructor_ReloadFailsToDeleteLockedModelFile_LogsWarningWithoutThrowing() - { - var contentRoot = NewTempRoot(); - var factory = CreateFactory(nameof(Constructor_ReloadFailsToDeleteLockedModelFile_LogsWarningWithoutThrowing)); - await SeedCategoriesAsync(factory, MakeCategory(1, "Batterien"), MakeCategory(2, "Elektronik")); - await using (var db = factory.CreateDbContext()) - { - var battNames = new[] { "AA Batterie Mignon", "AAA Batterie Micro", "9V Blockbatterie", "Akku Wiederaufladbar", "Knopfzelle CR2032", "Lithium Batterie AA" }; - var elecNames = new[] { "Laptop Notebook 15 Zoll", "USB Kabel Ladekabel", "Bluetooth Kopfhoerer", "HDMI Adapter", "Wireless Maus" }; - var id = 1; - foreach (var n in battNames) db.Products.Add(MakeProduct(id++, n, 1, description: "Batterie Zubehoer")); - foreach (var n in elecNames) db.Products.Add(MakeProduct(id++, n, 2, description: "Elektronik Zubehoer")); - await db.SaveChangesAsync(); - } - var trainer = CreateSut(factory, contentRoot); - (await trainer.TrainModelAsync()).Should().BeTrue(); - var modelPath = Path.Combine(contentRoot, "ML", "Data", "category-prediction-model.zip"); - - // Hold a read-sharing (but not delete-sharing) handle so _mlContext.Model.Load can still - // open and read the file (letting execution reach the ArgumentOutOfRangeException/"Label" - // branch as usual), but the subsequent File.Delete(_modelPath) fails - exercising the - // nested "Could not delete old model file" catch. - using (new FileStream(modelPath, FileMode.Open, FileAccess.Read, FileShare.Read)) - { - var act = () => CreateSut(factory, contentRoot); - - act.Should().NotThrow(); - } + result.BestMatch.Reasons.Should().Contain(r => r.Contains("ML-Modell"), "the reloaded instance's prediction engine must genuinely work, not just report IsMlModelTrained = true"); } [Fact] @@ -496,6 +497,29 @@ public async Task FindSimilarProductsAsync_UnderInMemoryProvider_QueryIsUntransl result.Should().BeEmpty(); } + /// Regression test: `words` (extracted from the query) are always lowercased, + /// but the production code used to compare them against p.Name (not lowercased), so a + /// capitalized product name like "Batterie" never matched the lowercase keyword + /// "batterie". Needs a real relational provider (SQLite) since InMemory can't translate + /// this query shape at all regardless of case, see the test above. + [Fact] + public async Task FindSimilarProductsAsync_UnderSqlite_MatchesCaseInsensitively() + { + using var factory = CreateSqliteFactory(); + await SeedCategoriesAsync(factory, MakeCategory(1, "Batterien")); + await using (var db = factory.CreateDbContext()) + { + db.Products.Add(MakeProduct(1, "Duracell Batterie AA Mignon", 1)); + db.Products.Add(MakeProduct(2, "USB Kabel", 1)); + await db.SaveChangesAsync(); + } + var sut = CreateSut(factory); + + var result = await sut.FindSimilarProductsAsync("Duracell Batterie"); + + result.Should().ContainSingle().Which.Should().Be("Duracell Batterie AA Mignon"); + } + [Fact] public async Task FindSimilarProductsAsync_DbContextCreationFailure_ReturnsEmptyList() { diff --git a/LagersystemLVHome.UnitTests/ML/SecurityRiskServiceTests.cs b/LagersystemLVHome.UnitTests/ML/SecurityRiskServiceTests.cs index 01387ff..df353eb 100644 --- a/LagersystemLVHome.UnitTests/ML/SecurityRiskServiceTests.cs +++ b/LagersystemLVHome.UnitTests/ML/SecurityRiskServiceTests.cs @@ -515,7 +515,11 @@ public async Task GetHighRiskUsersAsync_ReturnsOnlyActiveNonDeletedHighRiskUsers var result = await sut.GetHighRiskUsersAsync(); result.Should().OnlyContain(a => a.UserId == 1 || a.UserId == 2); - result.Select(a => a.UserId).Should().BeInDescendingOrder(); + // Ordered by RiskScore descending, not by UserId - user 1 has more sensitive actions + // (25 vs 12) so scores higher despite having the lower UserId, meaning the correctly + // ordered result is [1, 2] (ascending IDs) here, not descending. + result.Select(a => a.RiskScore).Should().BeInDescendingOrder(); + result.First().UserId.Should().Be(1, "user 1 has more sensitive actions and must score higher"); result.Should().OnlyContain(a => a.RiskLevel >= RiskLevel.High); } diff --git a/LagersystemLVHome.UnitTests/Services/Auth/WebAuthnServiceTests.cs b/LagersystemLVHome.UnitTests/Services/Auth/WebAuthnServiceTests.cs index 06666e6..9ed0c2c 100644 --- a/LagersystemLVHome.UnitTests/Services/Auth/WebAuthnServiceTests.cs +++ b/LagersystemLVHome.UnitTests/Services/Auth/WebAuthnServiceTests.cs @@ -463,16 +463,14 @@ public async Task VerifyRegistrationAsync_WithWrongType_ReturnsFailure() } /// - /// Suspected bug: origin verification only logs a warning - /// ("WebAuthn registration failed: Origin mismatch...") and does not return a - /// failure result, so a credential presented with a completely unrelated origin is - /// still registered. This test documents the current (permissive) behaviour rather - /// than asserting a fix. + /// Regression test: a credential presented with a completely unrelated origin used to + /// only log a warning and still register successfully. Origin mismatch is now a hard + /// registration failure. /// [Fact] - public async Task VerifyRegistrationAsync_WithMismatchedOrigin_StillSucceeds_DocumentingPermissiveBehaviour() + public async Task VerifyRegistrationAsync_WithMismatchedOrigin_ReturnsFailure() { - var (sut, factory, _) = CreateSut(nameof(VerifyRegistrationAsync_WithMismatchedOrigin_StillSucceeds_DocumentingPermissiveBehaviour)); + var (sut, factory, _) = CreateSut(nameof(VerifyRegistrationAsync_WithMismatchedOrigin_ReturnsFailure)); var user = await SeedUserAsync(factory); var options = await sut.GenerateRegistrationOptionsAsync(user.Id, "My Key"); @@ -488,7 +486,11 @@ public async Task VerifyRegistrationAsync_WithMismatchedOrigin_StillSucceeds_Doc var result = await sut.VerifyRegistrationAsync(user.Id, credentialJson, options.SessionId); - result.Success.Should().BeTrue("origin mismatch is only logged, not enforced, in the current implementation"); + result.Success.Should().BeFalse(); + result.Error.Should().Be("Origin stimmt nicht überein"); + + await using var verify = factory.CreateDbContext(); + (await verify.UserPasskeys.CountAsync()).Should().Be(0, "a credential from an untrusted origin must not be persisted"); } [Fact] @@ -1030,13 +1032,13 @@ public async Task VerifyAuthenticationAsync_WithDerEncodedSignature_Succeeds() } [Fact] - public async Task VerifyAuthenticationAsync_WithSignatureCounterRegression_StillSucceedsButLogsWarning() + public async Task VerifyAuthenticationAsync_WithSignatureCounterRegression_ReturnsFailure() { - // Suspected weakness: a signature counter that goes backwards (a classic cloned - // -authenticator indicator) is only logged as a warning, not rejected. The counter - // is unconditionally overwritten afterwards, so a rollback is not actually detected - // as a hard failure. This test documents the current (permissive) behaviour. - var (sut, factory, _) = CreateSut(nameof(VerifyAuthenticationAsync_WithSignatureCounterRegression_StillSucceedsButLogsWarning)); + // Regression test: a signature counter that goes backwards (a classic cloned + // -authenticator indicator) used to be only logged as a warning and then + // unconditionally overwritten. It is now a hard authentication failure, and the + // stored counter is left untouched so a later legitimate auth can still detect it. + var (sut, factory, _) = CreateSut(nameof(VerifyAuthenticationAsync_WithSignatureCounterRegression_ReturnsFailure)); var user = await SeedUserAsync(factory); var options = await sut.GenerateAuthenticationOptionsAsync(user.Username); @@ -1067,10 +1069,11 @@ public async Task VerifyAuthenticationAsync_WithSignatureCounterRegression_Still var result = await sut.VerifyAuthenticationAsync(credentialJson, options.SessionId); - result.Success.Should().BeTrue("counter regression is currently only logged, not enforced"); + result.Success.Should().BeFalse(); + result.Error.Should().Be("Signaturzähler ungültig"); await using var verify = factory.CreateDbContext(); - (await verify.UserPasskeys.SingleAsync()).SignatureCounter.Should().Be(1u, "the counter is overwritten unconditionally"); + (await verify.UserPasskeys.SingleAsync()).SignatureCounter.Should().Be(50u, "a rejected authentication must not advance the stored counter"); } // ---- GetUserPasskeysAsync / DeletePasskeyAsync / RenamePasskeyAsync / HasPasskeysAsync ---- diff --git a/LagersystemLVHome.UnitTests/Services/Backup/BackupManagementServiceTests.cs b/LagersystemLVHome.UnitTests/Services/Backup/BackupManagementServiceTests.cs index 004713c..6cc1495 100644 --- a/LagersystemLVHome.UnitTests/Services/Backup/BackupManagementServiceTests.cs +++ b/LagersystemLVHome.UnitTests/Services/Backup/BackupManagementServiceTests.cs @@ -412,14 +412,14 @@ public async Task TestProviderAsync_UploaderThrows_ReturnsFalse() [Fact] public async Task CleanupOldBackupsAsync_RemovesEntriesOlderThanRetentionWindowPerType() { - // NOTE: this pins CURRENT (buggy) behaviour. The Weekly/Monthly cutoffs are + // Regression test for an inverted-sign bug: the Weekly/Monthly cutoffs used to be // computed as cutoffDate.AddDays(+28) / (+365) instead of extending the cutoff - // further into the past. With a typical RetentionDays (e.g. 30) that makes the - // Weekly cutoff *tighter* than Daily's (now-2 days instead of now-30), and pushes - // the Monthly cutoff into the future entirely - so every Monthly-retention backup, - // however recent, gets deleted unconditionally. Flagged as a suspected bug in the - // report; this test documents the actual behaviour so a future fix shows up as an - // intentional, reviewed test change rather than a silent regression. + // further into the past. That made the Weekly cutoff *tighter* than Daily's + // (now-2 days instead of now-30) and pushed the Monthly cutoff into the future + // entirely - so every Monthly-retention backup, however recent, was deleted + // unconditionally. "monthly-recent" below is the key regression assertion: it must + // now survive, and "monthly-old" (well past the fixed now-395 cutoff) proves + // Monthly cleanup still actually deletes genuinely stale backups. var factory = CreateFactory(nameof(CleanupOldBackupsAsync_RemovesEntriesOlderThanRetentionWindowPerType)); var provider = await SeedProviderAsync(factory, "P", BackupProviderType.Local); var now = DateTime.UtcNow; @@ -430,7 +430,8 @@ public async Task CleanupOldBackupsAsync_RemovesEntriesOlderThanRetentionWindowP new BackupHistory { BackupProviderId = provider.Id, FileName = "daily-recent", RetentionType = BackupRetentionType.Daily, BackupDate = now.AddDays(-1) }, new BackupHistory { BackupProviderId = provider.Id, FileName = "weekly-old", RetentionType = BackupRetentionType.Weekly, BackupDate = now.AddDays(-100) }, new BackupHistory { BackupProviderId = provider.Id, FileName = "weekly-recent", RetentionType = BackupRetentionType.Weekly, BackupDate = now }, - new BackupHistory { BackupProviderId = provider.Id, FileName = "monthly-recent", RetentionType = BackupRetentionType.Monthly, BackupDate = now }); + new BackupHistory { BackupProviderId = provider.Id, FileName = "monthly-recent", RetentionType = BackupRetentionType.Monthly, BackupDate = now }, + new BackupHistory { BackupProviderId = provider.Id, FileName = "monthly-old", RetentionType = BackupRetentionType.Monthly, BackupDate = now.AddDays(-400) }); await db.SaveChangesAsync(); } var sut = CreateSut(factory); @@ -443,7 +444,8 @@ public async Task CleanupOldBackupsAsync_RemovesEntriesOlderThanRetentionWindowP remaining.Should().NotContain("daily-old"); remaining.Should().Contain("weekly-recent"); remaining.Should().NotContain("weekly-old"); - remaining.Should().NotContain("monthly-recent"); + remaining.Should().Contain("monthly-recent", "a monthly backup this recent must survive - the cutoff must not land in the future"); + remaining.Should().NotContain("monthly-old"); } // ----- ValidateBackupAsync ----- @@ -586,21 +588,26 @@ public async Task DeleteBackupAsync_UploaderThrows_RethrowsAndMarksHistoryFailed // ----- CleanupBackupsByProviderSettingsAsync ----- [Fact] - public async Task CleanupBackupsByProviderSettingsAsync_RemovesOldDaily_LeavesWeeklyAndMonthlyUntouched() + public async Task CleanupBackupsByProviderSettingsAsync_RemovesOldEntriesAcrossAllRetentionTypes() { - // NOTE: this documents the CURRENT behaviour. The method's XML doc says - // "Weekly and monthly handled analogously" but the implementation only ever - // processes BackupRetentionType.Daily - Weekly/Monthly rows are never cleaned up - // by this method regardless of age. Flagged as a suspected bug in the report. - var factory = CreateFactory(nameof(CleanupBackupsByProviderSettingsAsync_RemovesOldDaily_LeavesWeeklyAndMonthlyUntouched)); + // Regression test: the method's XML doc said "Weekly and monthly handled + // analogously" but the implementation only ever processed + // BackupRetentionType.Daily - Weekly/Monthly rows were never cleaned up by this + // method regardless of age. All three types are now processed, each with its own + // progressively longer cutoff (same reasoning as CleanupOldBackupsAsync). + var factory = CreateFactory(nameof(CleanupBackupsByProviderSettingsAsync_RemovesOldEntriesAcrossAllRetentionTypes)); var provider = await SeedProviderAsync(factory, "P", BackupProviderType.Local); + var now = DateTime.UtcNow; await using (var db = factory.CreateDbContext()) { db.BackupSettings.Add(new LagersystemLVHome.Domain.Models.BackupSettings { RetentionDays = 30 }); db.BackupHistory.AddRange( - new BackupHistory { BackupProviderId = provider.Id, FileName = "daily-old", RetentionType = BackupRetentionType.Daily, BackupDate = DateTime.UtcNow.AddDays(-40) }, - new BackupHistory { BackupProviderId = provider.Id, FileName = "weekly-ancient", RetentionType = BackupRetentionType.Weekly, BackupDate = DateTime.UtcNow.AddDays(-400) }, - new BackupHistory { BackupProviderId = provider.Id, FileName = "monthly-ancient", RetentionType = BackupRetentionType.Monthly, BackupDate = DateTime.UtcNow.AddYears(-5) }); + new BackupHistory { BackupProviderId = provider.Id, FileName = "daily-old", RetentionType = BackupRetentionType.Daily, BackupDate = now.AddDays(-40) }, + new BackupHistory { BackupProviderId = provider.Id, FileName = "daily-recent", RetentionType = BackupRetentionType.Daily, BackupDate = now.AddDays(-1) }, + new BackupHistory { BackupProviderId = provider.Id, FileName = "weekly-ancient", RetentionType = BackupRetentionType.Weekly, BackupDate = now.AddDays(-400) }, + new BackupHistory { BackupProviderId = provider.Id, FileName = "weekly-recent", RetentionType = BackupRetentionType.Weekly, BackupDate = now }, + new BackupHistory { BackupProviderId = provider.Id, FileName = "monthly-ancient", RetentionType = BackupRetentionType.Monthly, BackupDate = now.AddYears(-5) }, + new BackupHistory { BackupProviderId = provider.Id, FileName = "monthly-recent", RetentionType = BackupRetentionType.Monthly, BackupDate = now }); await db.SaveChangesAsync(); } var uploader = CreateUploader(BackupProviderType.Local); @@ -611,16 +618,20 @@ public async Task CleanupBackupsByProviderSettingsAsync_RemovesOldDaily_LeavesWe await using var verifyDb = factory.CreateDbContext(); var remaining = await verifyDb.BackupHistory.Select(h => h.FileName).ToListAsync(); - remaining.Should().BeEquivalentTo(new[] { "weekly-ancient", "monthly-ancient" }); + remaining.Should().BeEquivalentTo(new[] { "daily-recent", "weekly-recent", "monthly-recent" }); await uploader.Received(1).DeleteAsync(Arg.Is(h => h.FileName == "daily-old")); + await uploader.Received(1).DeleteAsync(Arg.Is(h => h.FileName == "weekly-ancient")); + await uploader.Received(1).DeleteAsync(Arg.Is(h => h.FileName == "monthly-ancient")); } [Fact] - public async Task CleanupBackupsByProviderSettingsAsync_UploaderDeleteThrows_RowStillRemovedFromDb() + public async Task CleanupBackupsByProviderSettingsAsync_UploaderDeleteThrows_RowIsKeptForRetry() { - // NOTE: documents current behaviour - RemoveRange runs unconditionally after the - // per-item try/catch, so a failed remote delete still purges the DB row. - var factory = CreateFactory(nameof(CleanupBackupsByProviderSettingsAsync_UploaderDeleteThrows_RowStillRemovedFromDb)); + // Regression test: RemoveRange used to run unconditionally after the per-item + // try/catch, so a failed remote delete still purged the DB row - "forgetting" a + // backup that may still exist at the provider. A thrown exception now leaves the + // row in place so it is retried on the next cleanup run. + var factory = CreateFactory(nameof(CleanupBackupsByProviderSettingsAsync_UploaderDeleteThrows_RowIsKeptForRetry)); var provider = await SeedProviderAsync(factory, "P", BackupProviderType.Local); await using (var db = factory.CreateDbContext()) { @@ -636,7 +647,7 @@ public async Task CleanupBackupsByProviderSettingsAsync_UploaderDeleteThrows_Row await act.Should().NotThrowAsync(); await using var verifyDb = factory.CreateDbContext(); - (await verifyDb.BackupHistory.AnyAsync()).Should().BeFalse(); + (await verifyDb.BackupHistory.AnyAsync(h => h.FileName == "daily-old")).Should().BeTrue("a failed remote delete must not silently forget the backup in the DB"); } // ----- CreateBackupAsync ----- diff --git a/LagersystemLVHome.UnitTests/Services/Backup/DatabaseRestoreServiceTests.cs b/LagersystemLVHome.UnitTests/Services/Backup/DatabaseRestoreServiceTests.cs index da12388..6774757 100644 --- a/LagersystemLVHome.UnitTests/Services/Backup/DatabaseRestoreServiceTests.cs +++ b/LagersystemLVHome.UnitTests/Services/Backup/DatabaseRestoreServiceTests.cs @@ -14,32 +14,33 @@ namespace LagersystemLVHome.UnitTests.Services.Backup; /// /// Covers . /// -/// Two real bugs surfaced while writing these tests and are pinned/documented rather -/// than "fixed" (production code is out of scope for this change): +/// Two real bugs were found and fixed while writing these tests: /// -/// 1. DecryptAndExtractAsync never actually decrypts, and restoring an encrypted -/// backup is broken in two independent, compounding ways: -/// (a) ValidateBackupAsync's first gate (IsValidZipAsync) requires the +/// 1. DecryptAndExtractAsync never actually decrypted, and restoring an encrypted +/// backup was broken in two independent, compounding ways: +/// (a) ValidateBackupAsync's first gate (IsValidZipAsync) required the /// raw uploaded stream to already parse as a ZIP archive - but a genuinely /// AES-encrypted backup (the IV-prefixed ciphertext /// BackupManagementService.EncryptBackupAsync produces) is opaque binary, not a -/// ZIP, so it is rejected as "Keine gueltige ZIP-Datei" before encryption/password -/// handling is ever reached. See -/// RestoreFromBackupAsync_RealAesEncryptedBackup_IsRejectedAtTheZipValidationGate. -/// (b) even for an input that clears that gate (e.g. a structurally valid ZIP that -/// happens to carry the ".encrypted" marker), DecryptAndExtractAsync - despite +/// ZIP, so it was rejected as "Keine gueltige ZIP-Datei" before encryption/password +/// handling was ever reached. Any non-ZIP stream long enough to hold a 16-byte IV is +/// now treated as an encrypted-backup candidate instead. See +/// ValidateBackupAsync_NotAZipButLongEnoughForAnIV_IsTreatedAsEncrypted. +/// (b) even for an input that cleared that gate, DecryptAndExtractAsync - despite /// its comment "Decryption delegated to EncryptionService when backup encryption is -/// enabled" - just copies the stream verbatim to a .zip path and extracts it as-is; -/// _encryptionService is never referenced anywhere in the class, so nothing is -/// ever genuinely decrypted. See -/// RestoreFromBackupAsync_ZipMarkedEncrypted_NeverActuallyDecrypts_SoNothingIsImported. +/// enabled" - just copied the stream verbatim to a .zip path and extracted it as-is. +/// It now performs the actual AES-CBC decryption (key derived from the password the +/// same way BackupManagementService.EncryptBackupAsync does), and a wrong +/// password now fails cleanly instead of silently importing nothing. See +/// RestoreFromBackupAsync_RealAesEncryptedBackup_DecryptsAndImportsSuccessfully and +/// RestoreFromBackupAsync_EncryptedWithWrongPassword_FailsCleanlyWithoutImporting. /// -/// 2. IsBackupEncryptedAsync's fast-path check looks for a zip entry named +/// 2. IsBackupEncryptedAsync's fast-path check looked for a zip entry named /// "backup_metadata.json", but the real metadata file /// writes is named "metadata.json" - so that -/// check is dead code for every real backup and detection always falls through to the -/// byte-sniffing heuristic (which happens to still get the right answer for JSON -/// backups). See IsBackupEncryptedAsync_BackupMetadataJsonEntryName_OnlyMatchesTheWrongFilename. +/// check was dead code for every real backup and detection always fell through to the +/// byte-sniffing heuristic. Now matches the real filename. See +/// IsBackupEncryptedAsync_MetadataJsonEntryPresent_ReturnsFalse. /// /// The final step of a fully successful RestoreFromBackupAsync call /// (CountTablesAsync) calls context.Database.GetDbConnection(), which is a @@ -149,13 +150,6 @@ private static byte[] BuildZip(params (string Name, byte[] Content)[] entries) return ms.ToArray(); } - private static byte[] RandomBytes(int count) - { - var bytes = new byte[count]; - RandomNumberGenerator.Fill(bytes); - return bytes; - } - private static byte[] EncryptLikeBackupManagementService(byte[] plainZipBytes, string password) { using var sha256 = SHA256.Create(); @@ -187,9 +181,9 @@ public async Task ValidateBackupAsync_ValidZipWithMetadata_ReturnsValidAndParses } [Fact] - public async Task ValidateBackupAsync_NotAZip_ReturnsInvalidWithGermanErrorMessage() + public async Task ValidateBackupAsync_TooShortToContainAnIV_ReturnsInvalidWithGermanErrorMessage() { - var sut = CreateSut(CreateFactory(nameof(ValidateBackupAsync_NotAZip_ReturnsInvalidWithGermanErrorMessage)), NewTempDir(), backupService: null); + var sut = CreateSut(CreateFactory(nameof(ValidateBackupAsync_TooShortToContainAnIV_ReturnsInvalidWithGermanErrorMessage)), NewTempDir(), backupService: null); var result = await sut.ValidateBackupAsync(new MemoryStream(new byte[] { 1, 2, 3, 4 })); @@ -197,6 +191,27 @@ public async Task ValidateBackupAsync_NotAZip_ReturnsInvalidWithGermanErrorMessa result.ErrorMessage.Should().Contain("ZIP"); } + /// + /// Regression test: a genuinely AES-encrypted backup (the IV-prefixed ciphertext + /// BackupManagementService.EncryptBackupAsync produces) is not a valid ZIP by itself - + /// it used to be rejected outright here as "Keine gueltige ZIP-Datei". Any non-ZIP + /// stream long enough to plausibly hold a 16-byte IV is now treated as an encrypted + /// backup candidate instead. + /// + [Fact] + public async Task ValidateBackupAsync_NotAZipButLongEnoughForAnIV_IsTreatedAsEncrypted() + { + var encrypted = EncryptLikeBackupManagementService(BuildZip(("a.json", "{}"u8.ToArray())), "pw"); + var sut = CreateSut(CreateFactory(nameof(ValidateBackupAsync_NotAZipButLongEnoughForAnIV_IsTreatedAsEncrypted)), NewTempDir(), backupService: null); + + var result = await sut.ValidateBackupAsync(new MemoryStream(encrypted)); + + result.IsValid.Should().BeTrue(); + result.IsEncrypted.Should().BeTrue(); + result.RequiresPassword.Should().BeTrue(); + result.ErrorMessage.Should().BeNull(); + } + [Fact] public async Task ValidateBackupAsync_NonSeekableStream_CopiesToMemoryStreamFirst() { @@ -220,13 +235,13 @@ public async Task IsBackupEncryptedAsync_EncryptedMarkerEntryPresent_ReturnsTrue } [Fact] - public async Task IsBackupEncryptedAsync_BackupMetadataJsonEntryName_OnlyMatchesTheWrongFilename() + public async Task IsBackupEncryptedAsync_MetadataJsonEntryPresent_ReturnsFalse() { - // Proves check #2 *does* work for the literal name it looks for - // ("backup_metadata.json") - but JsonBackupHelper never produces that filename - // (it writes "metadata.json"), so this fast path is dead for real backups. - var zip = BuildZip(("backup_metadata.json", "{}"u8.ToArray())); - var sut = CreateSut(CreateFactory(nameof(IsBackupEncryptedAsync_BackupMetadataJsonEntryName_OnlyMatchesTheWrongFilename)), NewTempDir(), backupService: null); + // Regression test: this fast-path check used to look for the wrong filename + // ("backup_metadata.json"), so it was dead code for every real backup - + // JsonBackupHelper actually writes "metadata.json". Now matches the real name. + var zip = BuildZip(("metadata.json", "{}"u8.ToArray())); + var sut = CreateSut(CreateFactory(nameof(IsBackupEncryptedAsync_MetadataJsonEntryPresent_ReturnsFalse)), NewTempDir(), backupService: null); (await sut.IsBackupEncryptedAsync(new MemoryStream(zip))).Should().BeFalse(); } @@ -324,53 +339,60 @@ public async Task RestoreFromBackupAsync_EncryptedButNoPassword_ReturnsFailureBe } [Fact] - public async Task RestoreFromBackupAsync_RealAesEncryptedBackup_IsRejectedAtTheZipValidationGate() + public async Task RestoreFromBackupAsync_RealAesEncryptedBackup_DecryptsAndImportsSuccessfully() { - // Empirically demonstrates the more fundamental half of bug #1: a genuinely - // AES-encrypted backup (same IV-prefixed scheme BackupManagementService.EncryptBackupAsync - // produces) is, by construction, no longer a valid ZIP container - it's opaque - // ciphertext. ValidateBackupAsync's very first gate (IsValidZipAsync) rejects it - // outright as "Keine gueltige ZIP-Datei" before encryption/password handling is - // even reached, so RestoreFromBackupAsync bails out immediately and never calls - // the safety-backup step. Restoring an encrypted backup produced by this - // application's own backup pipeline is therefore impossible end-to-end. - var plainZip = BuildZip(("Warehouses.json", "[]"u8.ToArray())); + // Regression test for both halves of the fixed bug: a genuinely AES-encrypted + // backup (same IV-prefixed scheme BackupManagementService.EncryptBackupAsync + // produces) is no longer a valid ZIP container by itself - ValidateBackupAsync now + // recognizes "not a ZIP, but plausibly sized" as an encrypted-backup candidate + // instead of rejecting it outright, and DecryptAndExtractAsync now genuinely + // decrypts (same key derivation, same IV placement) before extracting. Uses a real + // JsonBackupHelper export as the plaintext so a successful decrypt is proven by the + // seeded Warehouse actually landing in the target database. + var sourceFactory = CreateFactory(nameof(RestoreFromBackupAsync_RealAesEncryptedBackup_DecryptsAndImportsSuccessfully) + "_src"); + await using (var db = sourceFactory.CreateDbContext()) + { + db.Warehouses.Add(new Warehouse { Id = 1, Name = "WH1", Address = "a", IsActive = true }); + await db.SaveChangesAsync(); + } + var exportHelper = new JsonBackupHelper(sourceFactory, NullLogger.Instance, + Options.Create(new DatabaseSettings { Provider = DatabaseProvider.SQLite })); + var zipPath = Path.Combine(NewTempDir(), "src.zip"); + await exportHelper.CreateJsonBackupAsync(zipPath); + var plainZip = await File.ReadAllBytesAsync(zipPath); var encrypted = EncryptLikeBackupManagementService(plainZip, "correct-password"); + var targetFactory = CreateFactory(nameof(RestoreFromBackupAsync_RealAesEncryptedBackup_DecryptsAndImportsSuccessfully) + "_dst"); var backupService = Substitute.For(); - var sut = CreateSut(CreateFactory(nameof(RestoreFromBackupAsync_RealAesEncryptedBackup_IsRejectedAtTheZipValidationGate)), NewTempDir(), backupService: backupService); + var sut = CreateSut(targetFactory, NewTempDir(), backupService: backupService); + var progressEvents = new List(); + IProgress progress = new SyncProgress(p => progressEvents.Add(p)); - var result = await sut.RestoreFromBackupAsync(new MemoryStream(encrypted), password: "correct-password"); + var result = await sut.RestoreFromBackupAsync(new MemoryStream(encrypted), password: "correct-password", progress: progress); - result.Success.Should().BeFalse(); - result.ErrorMessage.Should().Contain("ZIP"); - await backupService.DidNotReceiveWithAnyArgs().CreateBackupAsync(default); + // Same InMemory-provider seam as the unencrypted happy-path test (see class + // remarks): the import itself succeeds, only the final relational-only tally fails. + result.SafetyBackupPath.Should().NotBeNullOrEmpty(); + progressEvents.Should().Contain(p => p.Step == RestoreStep.Decrypting); + await backupService.Received(1).CreateBackupAsync(Arg.Any()); + await using var verifyDb = targetFactory.CreateDbContext(); + (await verifyDb.Warehouses.CountAsync()).Should().Be(1); } [Fact] - public async Task RestoreFromBackupAsync_ZipMarkedEncrypted_NeverActuallyDecrypts_SoNothingIsImported() + public async Task RestoreFromBackupAsync_EncryptedWithWrongPassword_FailsCleanlyWithoutImporting() { - // Empirically demonstrates the other half of bug #1: DecryptAndExtractAsync never - // calls IEncryptionService - it copies whatever bytes it receives straight into a - // .zip and extracts them as-is. To get PAST the ZIP-validation gate (see the test - // above) this uses a *structurally valid* ZIP that carries the ".encrypted" - // marker BackupManagementService.EncryptBackupAsync's sibling code checks for, but - // whose payload is not a real JSON export (standing in for what genuinely - // encrypted ciphertext would look like once "decrypted" by simply re-opening it - // as a zip - garbage). The call completes without throwing and even runs the - // safety-backup step, but silently imports zero records: none of the expected - // Warehouses.json/Users.json/etc. table files exist in what got extracted. - var fakeEncryptedZip = BuildZip( - (".encrypted", Array.Empty()), - ("payload.bin", RandomBytes(64))); - - var targetFactory = CreateFactory(nameof(RestoreFromBackupAsync_ZipMarkedEncrypted_NeverActuallyDecrypts_SoNothingIsImported)); + var plainZip = BuildZip(("Warehouses.json", "[]"u8.ToArray())); + var encrypted = EncryptLikeBackupManagementService(plainZip, "correct-password"); + + var targetFactory = CreateFactory(nameof(RestoreFromBackupAsync_EncryptedWithWrongPassword_FailsCleanlyWithoutImporting)); var backupService = Substitute.For(); var sut = CreateSut(targetFactory, NewTempDir(), backupService: backupService); - await sut.RestoreFromBackupAsync(new MemoryStream(fakeEncryptedZip), password: "any-password"); + var result = await sut.RestoreFromBackupAsync(new MemoryStream(encrypted), password: "wrong-password"); - await backupService.Received(1).CreateBackupAsync(Arg.Any()); + result.Success.Should().BeFalse(); + result.ErrorMessage.Should().Contain("Passwort"); await using var verifyDb = targetFactory.CreateDbContext(); (await verifyDb.Warehouses.CountAsync()).Should().Be(0); } diff --git a/LagersystemLVHome.UnitTests/Services/Database/DatabaseHealthServiceTests.cs b/LagersystemLVHome.UnitTests/Services/Database/DatabaseHealthServiceTests.cs index 7d6bbbe..4c14741 100644 --- a/LagersystemLVHome.UnitTests/Services/Database/DatabaseHealthServiceTests.cs +++ b/LagersystemLVHome.UnitTests/Services/Database/DatabaseHealthServiceTests.cs @@ -161,6 +161,31 @@ public async Task GetHealthReportAsync_SuccessPath_SQLite_ReturnsExcellentEmptyR report.HealthStatus.Should().Be("Excellent"); report.Warnings.Should().BeEmpty(); report.Recommendations.Should().BeEmpty(); + report.LastBackup.Should().BeNull("no backup history exists yet"); + } + + /// Regression test: GetHealthReportAsync used to never call the existing + /// GetLastBackupDateAsync helper, so DatabaseHealthReport.LastBackup stayed null + /// regardless of actual backup history. It is now wired up. + [Fact] + public async Task GetHealthReportAsync_HasSuccessfulBackup_PopulatesLastBackup() + { + using var factory = CreateSqliteFactory(); + var expected = DateTime.UtcNow.AddDays(-1); + await using (var seed = factory.CreateDbContext()) + { + seed.BackupProviders.Add(new BackupProvider { Name = "local", Type = BackupProviderType.Local }); + await seed.SaveChangesAsync(); + var providerId = seed.BackupProviders.First().Id; + seed.BackupHistory.Add(new BackupHistory { BackupProviderId = providerId, FileName = "recent.bak", BackupDate = expected, Status = BackupStatus.Success }); + await seed.SaveChangesAsync(); + } + var sut = BuildSut(factory); + + var report = await sut.GetHealthReportAsync(); + + report.LastBackup.Should().NotBeNull(); + report.LastBackup!.Value.Should().BeCloseTo(expected, TimeSpan.FromSeconds(2)); } [Fact] @@ -446,11 +471,12 @@ public async Task GetAverageQueryTimeAsync_MySQLProviderAgainstSqliteConnection_ (await InvokeAsync(sut, "GetAverageQueryTimeAsync", ctx, CancellationToken.None)).Should().Be(0); } - // ==================== GetLastBackupDateAsync (private, currently unused by production code) ==================== - // BUG (suspected): GetHealthReportAsync never calls GetLastBackupDateAsync, so - // DatabaseHealthReport.LastBackup/LastVacuum/NeedsVacuum are always left at their default - // values (null/false) regardless of actual backup history. Verified directly via reflection - // since there is no public call path to exercise it otherwise. + // ==================== GetLastBackupDateAsync (private) ==================== + // GetHealthReportAsync now calls this and populates DatabaseHealthReport.LastBackup (see + // GetHealthReportAsync_HasSuccessfulBackup_PopulatesLastBackup above). These two tests + // exercise the helper directly via reflection for its own edge cases. + // LastVacuum/NeedsVacuum have no backing implementation anywhere in this class - out of + // scope here, that would be new functionality rather than wiring up an existing method. [Fact] public async Task GetLastBackupDateAsync_NoSuccessfulBackups_ReturnsNull() diff --git a/LagersystemLVHome.UnitTests/Services/Reporting/ApplicationInsightsServiceTests.cs b/LagersystemLVHome.UnitTests/Services/Reporting/ApplicationInsightsServiceTests.cs index 5621540..40df394 100644 --- a/LagersystemLVHome.UnitTests/Services/Reporting/ApplicationInsightsServiceTests.cs +++ b/LagersystemLVHome.UnitTests/Services/Reporting/ApplicationInsightsServiceTests.cs @@ -402,8 +402,10 @@ public async Task GetCurrentPerformanceAsync_ComputesLiveSnapshot() metric.ActiveUsers.Should().Be(1); metric.CpuUsagePercent.Should().BeGreaterThanOrEqualTo(0); metric.MemoryUsedMB.Should().BeGreaterThanOrEqualTo(0); - // Suspected bug: MemoryTotalMB is hardcoded to long.MinValue instead of an actual total-memory reading. - metric.MemoryTotalMB.Should().Be(long.MinValue); + // Regression test: MemoryTotalMB used to be hardcoded to long.MinValue instead of an + // actual reading. It now reflects GC.GetGCMemoryInfo().TotalAvailableMemoryBytes, + // which is always positive on any real runtime. + metric.MemoryTotalMB.Should().BeGreaterThan(0); } [Fact] diff --git a/LagersystemLVHome.UnitTests/Services/Reporting/ExportServiceTests.cs b/LagersystemLVHome.UnitTests/Services/Reporting/ExportServiceTests.cs index 860e3fa..60ab75a 100644 --- a/LagersystemLVHome.UnitTests/Services/Reporting/ExportServiceTests.cs +++ b/LagersystemLVHome.UnitTests/Services/Reporting/ExportServiceTests.cs @@ -429,11 +429,12 @@ public async Task ExportMovementsToExcelAsync_ContextFactoryThrows_LogsAndRethro [Fact] public async Task ExportStorageLocationsToExcelAsync_WritesHeaderAndDataRows() { - // NOTE: production code does `.Include(sl => sl.Room)`, but StorageLocation.Room is a - // plain string column (not a navigation property) — see StorageLocation.cs. EF Core's - // Include() requires a navigation property; calling it on a scalar member is invalid and - // is expected to throw at query-translation time. This test documents the current - // (likely unintended) runtime behavior of ExportStorageLocationsToExcelAsync. + // Regression test: production code used to do `.Include(sl => sl.Room)`, but + // StorageLocation.Room is a plain string column, not a navigation property - see + // StorageLocation.cs. EF Core's Include() requires a navigation property, so calling + // it on a scalar member threw InvalidOperationException at query-translation time on + // every call, making this export endpoint completely broken. The Include() is gone; + // Room is a normal column and needs no eager-loading. var factory = CreateFactory(nameof(ExportStorageLocationsToExcelAsync_WritesHeaderAndDataRows)); await using (var db = factory.CreateDbContext()) { @@ -444,12 +445,14 @@ public async Task ExportStorageLocationsToExcelAsync_WritesHeaderAndDataRows() var sut = Build(factory); - var act = () => sut.ExportStorageLocationsToExcelAsync(1); + var bytes = await sut.ExportStorageLocationsToExcelAsync(1); - // See comment above: `.Include(sl => sl.Room)` on a scalar property is invalid and the - // service's catch block logs and rethrows, so this currently always fails in production. - await act.Should().ThrowAsync( - "sl.Room is a scalar string, not a navigation property, so Include() cannot resolve it"); + using var workbook = new XLWorkbook(new MemoryStream(bytes)); + var ws = workbook.Worksheet("Lagerplaetze"); + ws.Cell(1, 1).GetString().Should().Be("Code"); + ws.Cell(1, 3).GetString().Should().Be("Raum"); + ws.Cell(2, 1).GetString().Should().Be("A1"); + ws.Cell(2, 3).GetString().Should().Be("Hall A"); } // ---- GenerateInventoryReportPdfAsync (HTML) -------------------------------------