From fbe58a81e487557c369c0f3044b968528dd355c5 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 19 Jan 2026 16:54:35 -0500 Subject: [PATCH] refactor(securestoremanager): add platform service abstractions and constants Implement deferred code review findings: - Add IDialogService/IClipboardService interfaces for testable platform operations - Create AvaloniaDialogService and AvaloniaClipboardService implementations - Extract dialog strings and file extensions to centralized Constants classes - Refactor ViewModels to use DI instead of event delegates - Update tests to use mock services --- CodeReviews/SecureStoreManager/CodeReview.md | 853 ++++++++++++++++++ .../Controllers/PipelineController.cs | 2 +- .../Models/Infrastructure/DataUpdate.cs | 4 +- .../Services/ScheduleChecker.cs | 18 +- .../Scripts/002_CreateDataUpdateTable.sql | 2 +- .../App.axaml.cs | 49 +- .../Application/SecretUseCases.cs | 53 ++ .../Application/StoreUseCases.cs | 103 +++ .../Constants/DialogStrings.cs | 50 + .../Constants/FileExtensions.cs | 21 + .../JdeScoping.SecureStoreManager.csproj | 3 + .../Models/SecretEntry.cs | 17 - .../Services/AvaloniaClipboardService.cs | 29 + .../Services/AvaloniaDialogService.cs | 135 +++ .../Services/IClipboardService.cs | 14 + .../Services/IDialogService.cs | 58 ++ .../Services/SecureStoreManager.cs | 57 +- .../ViewModels/AsyncRelayCommand.cs | 72 ++ .../ViewModels/DialogViewModels.cs | 113 ++- .../ViewModels/MainWindowViewModel.cs | 156 ++-- .../ViewModels/SecretItemViewModel.cs | 19 +- .../Views/MainWindow.axaml | 4 +- .../Views/MainWindow.axaml.cs | 112 +-- .../Views/NewStoreDialog.axaml | 6 +- .../Views/NewStoreDialog.axaml.cs | 8 +- .../Views/OpenStoreDialog.axaml | 6 +- .../Views/OpenStoreDialog.axaml.cs | 8 +- .../Views/SecretEditDialog.axaml | 6 +- .../Views/SecretEditDialog.axaml.cs | 4 +- .../ScheduleCheckerTests.cs | 6 +- .../ViewModels/MainWindowViewModelTests.cs | 8 +- .../ViewModels/SecretItemViewModelTests.cs | 41 +- .../Views/MainWindowTests.cs | 51 +- 33 files changed, 1790 insertions(+), 298 deletions(-) create mode 100644 CodeReviews/SecureStoreManager/CodeReview.md create mode 100644 NEW/src/Utils/JdeScoping.SecureStoreManager/Application/SecretUseCases.cs create mode 100644 NEW/src/Utils/JdeScoping.SecureStoreManager/Application/StoreUseCases.cs create mode 100644 NEW/src/Utils/JdeScoping.SecureStoreManager/Constants/DialogStrings.cs create mode 100644 NEW/src/Utils/JdeScoping.SecureStoreManager/Constants/FileExtensions.cs delete mode 100644 NEW/src/Utils/JdeScoping.SecureStoreManager/Models/SecretEntry.cs create mode 100644 NEW/src/Utils/JdeScoping.SecureStoreManager/Services/AvaloniaClipboardService.cs create mode 100644 NEW/src/Utils/JdeScoping.SecureStoreManager/Services/AvaloniaDialogService.cs create mode 100644 NEW/src/Utils/JdeScoping.SecureStoreManager/Services/IClipboardService.cs create mode 100644 NEW/src/Utils/JdeScoping.SecureStoreManager/Services/IDialogService.cs create mode 100644 NEW/src/Utils/JdeScoping.SecureStoreManager/ViewModels/AsyncRelayCommand.cs diff --git a/CodeReviews/SecureStoreManager/CodeReview.md b/CodeReviews/SecureStoreManager/CodeReview.md new file mode 100644 index 0000000..ae28078 --- /dev/null +++ b/CodeReviews/SecureStoreManager/CodeReview.md @@ -0,0 +1,853 @@ +# SecureStoreManager Code Review + +**Review Date:** January 19, 2026 +**Reviewer:** Claude Code +**Project Path:** `NEW/src/Utils/JdeScoping.SecureStoreManager/` +**Focus:** Best Practices, CLEAN Architecture, Code Quality + +--- + +## Executive Summary + +The SecureStoreManager is an Avalonia desktop application that provides a GUI for managing encrypted secrets using the NeoSmart.SecureStore library. The codebase demonstrates solid MVVM fundamentals with readable, well-organized code. However, several architectural concerns limit testability, maintainability, and robustness. + +### Overall Assessment: **B-** → **B+** (Post-Resolution) + +| Category | Grade | Post-Fix | Summary | +|----------|-------|----------|---------| +| Architecture | C+ | B+ | ✅ Added use-case layer (StoreUseCases, SecretUseCases) | +| MVVM Implementation | B | B+ | ✅ Fixed async patterns, CanExecute refresh | +| Service Layer | B- | A- | ✅ Reserved key protection, logging added | +| Code Quality | B | B+ | ✅ AsyncRelayCommand, proper exception handling | +| Testability | C | B | ✅ DI container, constructor injection | +| Error Handling | C+ | B+ | ✅ Exception surfacing, structured logging | + +### Resolution Status: **ALL FINDINGS ADDRESSED** ✅ + +--- + +## 1. Architecture Analysis + +### 1.1 Project Structure + +``` +JdeScoping.SecureStoreManager/ +├── Program.cs # Entry point +├── App.axaml(.cs) # Application bootstrapping +├── Models/ +│ └── SecretEntry.cs # Domain model (unused) +├── Services/ +│ ├── ISecureStoreManager.cs +│ └── SecureStoreManager.cs +├── ViewModels/ +│ ├── ViewModelBase.cs +│ ├── RelayCommand.cs +│ ├── MainWindowViewModel.cs +│ ├── DialogViewModels.cs +│ └── SecretItemViewModel.cs +├── Views/ +│ ├── MainWindow.axaml(.cs) +│ ├── NewStoreDialog.axaml(.cs) +│ ├── OpenStoreDialog.axaml(.cs) +│ └── SecretEditDialog.axaml(.cs) +└── Converters/ + └── BooleanConverters.cs +``` + +### 1.2 Current Layering Issues + +**Issue: Flat, UI-Centric Architecture** + +The current architecture lacks separation between application logic and presentation: + +``` +┌─────────────────────────────────────────┐ +│ Views (Avalonia) │ +├─────────────────────────────────────────┤ +│ ViewModels │ ← Contains business logic +├─────────────────────────────────────────┤ +│ Services (SecureStore) │ +└─────────────────────────────────────────┘ +``` + +**Recommendation: Introduce Application Layer** + +``` +┌─────────────────────────────────────────┐ +│ Views (Avalonia) │ +├─────────────────────────────────────────┤ +│ ViewModels │ ← Presentation logic only +├─────────────────────────────────────────┤ +│ Application Services (Use Cases) │ ← Business logic +├─────────────────────────────────────────┤ +│ Infrastructure Services │ ← SecureStore wrapper +└─────────────────────────────────────────┘ +``` + +### 1.3 Unused Domain Model + +**Location:** `Models/SecretEntry.cs` + +```csharp +public class SecretEntry +{ + public string Key { get; set; } = string.Empty; + public string Value { get; set; } = string.Empty; +} +``` + +**Issue:** This model is defined but never used. ViewModels construct `SecretItemViewModel` directly from service data. + +**Recommendation:** Either: +- Remove `SecretEntry.cs` to reduce dead code +- Use it as the domain model returned by `ISecureStoreManager` for proper layering + +--- + +## 2. MVVM Implementation + +### 2.1 Strengths + +- **ViewModelBase:** Clean implementation of `INotifyPropertyChanged` with `SetProperty` helper +- **RelayCommand:** Standard, readable command implementation +- **Data Binding:** Proper use of XAML bindings for commands and properties +- **Separation:** ViewModels don't directly reference Avalonia types + +### 2.2 MVVM Purity Trade-offs + +**Location:** `Views/MainWindow.axaml.cs` (Lines 22-48) + +The code-behind subscribes to ViewModel events for platform interactions: + +```csharp +private void MainWindow_Loaded(object? sender, RoutedEventArgs e) +{ + ViewModel.OnRequestNewStoreDialog += ShowNewStoreDialog; + ViewModel.OnRequestOpenStoreDialog += ShowOpenStoreDialog; + ViewModel.OnShowError += ShowErrorAsync; + ViewModel.OnShowSaveFileDialog += ShowSaveFileDialogAsync; + // ... more subscriptions +} +``` + +**Assessment:** This is a pragmatic approach in Avalonia, but it: +- Couples ViewModel to specific event signatures +- Makes the View responsible for orchestrating dialogs +- Prevents easy testing of dialog workflows + +**Recommendation:** Abstract platform services behind interfaces: + +```csharp +// Services/IDialogService.cs +public interface IDialogService +{ + Task ShowSaveFileDialogAsync(string title, string filter, string extension); + Task ShowOpenFileDialogAsync(string title, string filter); + Task ShowErrorAsync(string message, string title); + Task ShowInfoAsync(string message, string title); + Task ShowConfirmationAsync(string message, string title); +} + +// Services/IClipboardService.cs +public interface IClipboardService +{ + Task SetTextAsync(string text); +} +``` + +--- + +## 3. Service Layer Analysis + +### 3.1 Interface Design + +**Location:** `Services/ISecureStoreManager.cs` + +```csharp +public interface ISecureStoreManager +{ + bool IsStoreOpen { get; } + bool HasUnsavedChanges { get; } + string? CurrentStorePath { get; } + + void CreateStore(string storePath, string keyFilePath); + void CreateStoreWithPassword(string storePath, string password); + void OpenStore(string storePath, string keyFilePath); + void OpenStoreWithPassword(string storePath, string password); + void CloseStore(); + void Save(); + + IReadOnlyList GetKeys(); + string GetSecret(string key); + void SetSecret(string key, string value); + void RemoveSecret(string key); + + void GenerateKeyFile(string keyFilePath); + void ExportKey(string keyFilePath); +} +``` + +**Strengths:** +- Clear separation of store lifecycle vs. secret operations +- Returns `IReadOnlyList` for keys (immutable) +- Synchronous operations appropriate for local file I/O + +### 3.2 Critical Issue: Reserved Key Vulnerability + +**Location:** `Services/SecureStoreManager.cs` (Lines 159-172) + +```csharp +private const string KeysMetadataKey = "__keys__"; + +private void SaveKeysMetadata() +{ + var keysJson = JsonSerializer.Serialize(_keys.ToList()); + _store!.Set(KeysMetadataKey, keysJson); +} +``` + +**Vulnerability:** There is no protection against a user calling: + +```csharp +storeManager.SetSecret("__keys__", "malicious data"); +``` + +This would corrupt the key tracking metadata and potentially break the store. + +**Recommendation:** Add reserved key validation: + +```csharp +private static readonly HashSet ReservedKeys = new(StringComparer.OrdinalIgnoreCase) +{ + "__keys__" +}; + +public void SetSecret(string key, string value) +{ + ThrowIfStoreNotOpen(); + + if (ReservedKeys.Contains(key)) + throw new ArgumentException($"The key '{key}' is reserved for internal use.", nameof(key)); + + if (string.IsNullOrWhiteSpace(key)) + throw new ArgumentException("Key cannot be null or whitespace.", nameof(key)); + + _store!.Set(key, value); + _keys.Add(key); + HasUnsavedChanges = true; +} +``` + +### 3.3 Documentation Accuracy + +**Location:** `Services/SecureStoreManager.cs` (Line 8) + +```csharp +/// +/// Manages secure storage of secrets using SecureStore library. +/// Provides a WPF-friendly wrapper around the SecureStore functionality. +/// +``` + +**Issue:** The comment references WPF but this is an Avalonia application. + +**Fix:** +```csharp +/// +/// Manages secure storage of secrets using SecureStore library. +/// Provides an Avalonia-friendly wrapper around the SecureStore functionality. +/// +``` + +--- + +## 4. ViewModel Analysis + +### 4.1 MainWindowViewModel - God Object Concern + +**Location:** `ViewModels/MainWindowViewModel.cs` + +The ViewModel has approximately 400 lines with: +- 16 commands (File, Secret, Tools operations) +- State management (store, secrets, selection) +- Dialog coordination (7 event delegates) +- Error handling +- Status message management + +**Recommendation:** Extract use-case classes: + +```csharp +// Application/UseCases/OpenStoreUseCase.cs +public class OpenStoreUseCase +{ + private readonly ISecureStoreManager _storeManager; + private readonly IDialogService _dialogService; + + public async Task ExecuteAsync(string storePath, string? keyFilePath, string? password) + { + try + { + if (!string.IsNullOrEmpty(keyFilePath)) + _storeManager.OpenStore(storePath, keyFilePath); + else if (!string.IsNullOrEmpty(password)) + _storeManager.OpenStoreWithPassword(storePath, password); + else + throw new ArgumentException("Either key file or password required."); + + return true; + } + catch (Exception ex) + { + await _dialogService.ShowErrorAsync($"Failed to open store:\n{ex.Message}", "Error"); + return false; + } + } +} +``` + +### 4.2 Async Void Anti-Pattern + +**Locations:** Multiple methods in `MainWindowViewModel.cs` + +```csharp +// Line 219 +private async void ExecuteNewStore() +{ + if (!await PromptForUnsavedChangesAsync()) + return; + OnRequestNewStoreDialog?.Invoke(); +} + +// Line 237 +private async void ExecuteSave() +{ + await ExecuteSaveAsync(); +} + +// Line 293 +private async void ExecuteDeleteSecret() +{ + // ... async operations +} +``` + +**Issue:** `async void` methods: +- Cannot be awaited +- Exceptions propagate to synchronization context (unobservable) +- Cannot be unit tested with await patterns + +**Recommendation:** Implement AsyncRelayCommand: + +```csharp +public class AsyncRelayCommand : ICommand +{ + private readonly Func _execute; + private readonly Func? _canExecute; + private bool _isExecuting; + + public event EventHandler? CanExecuteChanged; + + public AsyncRelayCommand(Func execute, Func? canExecute = null) + { + _execute = execute ?? throw new ArgumentNullException(nameof(execute)); + _canExecute = canExecute; + } + + public bool CanExecute(object? parameter) + { + return !_isExecuting && (_canExecute?.Invoke() ?? true); + } + + public async void Execute(object? parameter) + { + if (!CanExecute(parameter)) + return; + + _isExecuting = true; + RaiseCanExecuteChanged(); + + try + { + await _execute(); + } + finally + { + _isExecuting = false; + RaiseCanExecuteChanged(); + } + } + + public void RaiseCanExecuteChanged() + { + CanExecuteChanged?.Invoke(this, EventArgs.Empty); + } +} +``` + +### 4.3 CanExecute Not Refreshed on Selection Change + +**Location:** `ViewModels/MainWindowViewModel.cs` (Lines 50-54, 320, 379-392) + +```csharp +public SecretItemViewModel? SelectedSecret +{ + get => _selectedSecret; + set => SetProperty(ref _selectedSecret, value); +} + +private bool CanEditOrDeleteSecret() => _storeManager.IsStoreOpen && SelectedSecret != null; +``` + +**Issue:** When `SelectedSecret` changes, the Edit and Delete command states don't update because `RaiseCanExecuteChanged()` is only called in `NotifyStoreChanged()`. + +**Fix:** Add property change notification: + +```csharp +public SecretItemViewModel? SelectedSecret +{ + get => _selectedSecret; + set + { + if (SetProperty(ref _selectedSecret, value)) + { + (EditSecretCommand as RelayCommand)?.RaiseCanExecuteChanged(); + (DeleteSecretCommand as RelayCommand)?.RaiseCanExecuteChanged(); + } + } +} +``` + +### 4.4 SecretItemViewModel - Silent Exception Swallowing + +**Location:** `ViewModels/SecretItemViewModel.cs` (Lines 73-86) + +```csharp +private async void CopyToClipboard() +{ + try + { + if (OnCopyToClipboard != null) + { + await OnCopyToClipboard(_actualValue); + } + } + catch + { + // Clipboard operations can fail in some scenarios + } +} +``` + +**Issue:** All exceptions are silently swallowed with no user feedback or logging. + +**Recommendation:** + +```csharp +private async void CopyToClipboard() +{ + try + { + if (OnCopyToClipboard != null) + { + await OnCopyToClipboard(_actualValue); + } + } + catch (Exception ex) + { + OnCopyFailed?.Invoke($"Failed to copy to clipboard: {ex.Message}"); + } +} + +public event Action? OnCopyFailed; +``` + +--- + +## 5. Dependency Injection Issues + +### 5.1 Hard-Coded Service Creation + +**Location:** `ViewModels/MainWindowViewModel.cs` (Lines 16-17) + +```csharp +public MainWindowViewModel() : this(new Services.SecureStoreManager()) +{ +} +``` + +**Issue:** The parameterless constructor creates a concrete `SecureStoreManager`, violating: +- Dependency Inversion Principle +- Single Responsibility Principle (ViewModel creates its dependencies) + +**Impact on Testing:** Unit tests must use the constructor with the interface parameter, but the default XAML instantiation uses the parameterless constructor. + +**Recommendation:** Use a service locator or DI container in `App.axaml.cs`: + +```csharp +public partial class App : Application +{ + public static IServiceProvider Services { get; private set; } = null!; + + public override void OnFrameworkInitializationCompleted() + { + var services = new ServiceCollection(); + ConfigureServices(services); + Services = services.BuildServiceProvider(); + + if (ApplicationLifetime is IClassicDesktopStyleApplicationLifetime desktop) + { + desktop.MainWindow = new MainWindow + { + DataContext = Services.GetRequiredService() + }; + } + + base.OnFrameworkInitializationCompleted(); + } + + private void ConfigureServices(IServiceCollection services) + { + services.AddSingleton(); + services.AddTransient(); + } +} +``` + +### 5.2 Dialog ViewModel DataContext in XAML + +**Location:** `Views/NewStoreDialog.axaml` (Lines 9-11) + +```xml + + + +``` + +**Issue:** ViewModel is instantiated in XAML, making dependency injection difficult. + +**Recommendation:** Set DataContext in code-behind or use ViewModelLocator pattern. + +--- + +## 6. Dialog Validation Redundancy + +### 6.1 Dual Validation + +**Locations:** +- `ViewModels/DialogViewModels.cs` - ViewModel validation +- `Views/NewStoreDialog.axaml.cs` - Code-behind validation + +ViewModel: +```csharp +public bool IsValid +{ + get + { + if (string.IsNullOrWhiteSpace(StorePath)) + return false; + // ... + } +} +``` + +Code-behind: +```csharp +private async void CreateButton_Click(object? sender, RoutedEventArgs e) +{ + if (!ViewModel.IsValid) + { + var box = MessageBoxManager.GetMessageBoxStandard( + "Validation Error", + ViewModel.ValidationError ?? "Please fill in all required fields.", + // ... + ); + await box.ShowWindowDialogAsync(this); + return; + } + Close(true); +} +``` + +**Assessment:** The current approach is functional but: +- Validation logic is duplicated conceptually +- UI feedback (message box) is in code-behind rather than bound to ViewModel + +**Recommendation:** Bind Create button's `IsEnabled` to `IsValid`: + +```xml +