Conversation
…tions) Agent-Logs-Url: https://github.com/Kvikku/Windows-Security-Manager/sessions/3c065562-60ba-4b1c-831a-3c0615677e3b Co-authored-by: Kvikku <85877485+Kvikku@users.noreply.github.com>
… Audit Log pages Agent-Logs-Url: https://github.com/Kvikku/Windows-Security-Manager/sessions/3c065562-60ba-4b1c-831a-3c0615677e3b Co-authored-by: Kvikku <85877485+Kvikku@users.noreply.github.com>
…ocumentation Agent-Logs-Url: https://github.com/Kvikku/Windows-Security-Manager/sessions/3c065562-60ba-4b1c-831a-3c0615677e3b Co-authored-by: Kvikku <85877485+Kvikku@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Kvikku/Windows-Security-Manager/sessions/3c065562-60ba-4b1c-831a-3c0615677e3b Co-authored-by: Kvikku <85877485+Kvikku@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a WinUI 3 desktop GUI to complement the existing CLI, while extracting the shared domain logic (models/definitions/services) into a new Core library so both front-ends use the same implementation.
Changes:
- Extract shared logic into
WindowsSecurityManager.Coreand update CLI/tests to reference it. - Add new unpackaged WinUI 3 GUI project with pages for dashboard, settings, reports, backup/restore, and audit log viewing.
- Update release pipeline to publish both CLI executable and GUI zip asset.
Reviewed changes
Copilot reviewed 26 out of 50 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| WindowsSecurityManager.slnx | Adds Core + GUI projects to the solution. |
| tests/WindowsSecurityManager.Tests/WindowsSecurityManager.Tests.csproj | References Core (in addition to CLI) for shared types/services. |
| src/WindowsSecurityManager/WindowsSecurityManager.csproj | References Core from the CLI project. |
| src/WindowsSecurityManager.Gui/WindowsSecurityManager.Gui.csproj | New WinUI 3 GUI project configuration + packages + Core reference. |
| src/WindowsSecurityManager.Gui/App.xaml | GUI resources initialization. |
| src/WindowsSecurityManager.Gui/App.xaml.cs | GUI bootstrap wiring (providers → manager → audit logger). |
| src/WindowsSecurityManager.Gui/MainWindow.xaml | NavigationView shell for GUI sections. |
| src/WindowsSecurityManager.Gui/MainWindow.xaml.cs | Navigation handling between GUI pages. |
| src/WindowsSecurityManager.Gui/Views/DashboardPage.xaml | Dashboard UI layout for compliance summary + per-category breakdown. |
| src/WindowsSecurityManager.Gui/Views/DashboardPage.xaml.cs | Dashboard page code-behind to refresh and populate UI. |
| src/WindowsSecurityManager.Gui/Views/SettingsPage.xaml | Settings UI with search/filter/profile actions and per-row enable/disable. |
| src/WindowsSecurityManager.Gui/Views/SettingsPage.xaml.cs | Settings interactions wired to SettingsViewModel + status InfoBar. |
| src/WindowsSecurityManager.Gui/Views/ReportPage.xaml | Report UI for generating and exporting reports. |
| src/WindowsSecurityManager.Gui/Views/ReportPage.xaml.cs | Report generation/export orchestration via ReportViewModel. |
| src/WindowsSecurityManager.Gui/Views/BackupPage.xaml | Backup/restore UI. |
| src/WindowsSecurityManager.Gui/Views/BackupPage.xaml.cs | Backup/restore interactions via BackupViewModel. |
| src/WindowsSecurityManager.Gui/Views/AuditLogPage.xaml | Audit log viewer UI. |
| src/WindowsSecurityManager.Gui/Views/AuditLogPage.xaml.cs | Audit log loading + refresh behavior. |
| src/WindowsSecurityManager.Gui/ViewModels/DashboardViewModel.cs | Dashboard compliance aggregation logic over Core reports. |
| src/WindowsSecurityManager.Gui/ViewModels/SettingsViewModel.cs | Settings list/search/filter/profile + enable/disable actions via Core manager. |
| src/WindowsSecurityManager.Gui/ViewModels/ReportViewModel.cs | Report generation + export-to-file logic via Core ReportExporter. |
| src/WindowsSecurityManager.Gui/ViewModels/BackupViewModel.cs | Backup creation + restore logic via Core BackupService. |
| src/WindowsSecurityManager.Gui/ViewModels/AuditLogViewModel.cs | Audit log retrieval and ordering for display. |
| src/WindowsSecurityManager.Core/WindowsSecurityManager.Core.csproj | New Core library project definition. |
| src/WindowsSecurityManager.Core/Models/SettingValueType.cs | Core model: registry value type enum. |
| src/WindowsSecurityManager.Core/Models/SecurityCategory.cs | Core model: setting category enum. |
| src/WindowsSecurityManager.Core/Models/ExportFormat.cs | Core model: report export formats. |
| src/WindowsSecurityManager.Core/Models/SecuritySetting.cs | Core model: security setting definition. |
| src/WindowsSecurityManager.Core/Models/SettingStatus.cs | Core model: computed setting status. |
| src/WindowsSecurityManager.Core/Models/SecurityReport.cs | Core model: compliance report metrics over statuses. |
| src/WindowsSecurityManager.Core/Models/DryRunChange.cs | Core model: dry-run change representation. |
| src/WindowsSecurityManager.Core/Models/SecurityProfile.cs | Core model: profile preset definition. |
| src/WindowsSecurityManager.Core/Models/BackupData.cs | Core model: backup file representation (entries). |
| src/WindowsSecurityManager.Core/Models/AuditLogEntry.cs | Core model: audit log entry representation. |
| src/WindowsSecurityManager.Core/Services/IRegistryService.cs | Core abstraction for registry operations (testability). |
| src/WindowsSecurityManager.Core/Services/ISecuritySettingProvider.cs | Core abstraction for providers of setting definitions. |
| src/WindowsSecurityManager.Core/Services/RegistryService.cs | Core implementation of IRegistryService (Windows registry). |
| src/WindowsSecurityManager.Core/Services/SecuritySettingsManager.cs | Core manager for list/search/enable/disable/report/dry-run/status. |
| src/WindowsSecurityManager.Core/Services/ReportExporter.cs | Core exporter for JSON/CSV/HTML report output. |
| src/WindowsSecurityManager.Core/Services/BackupService.cs | Core backup/restore logic for settings. |
| src/WindowsSecurityManager.Core/Services/AuditLogger.cs | Core audit log file writer/reader. |
| src/WindowsSecurityManager.Core/Definitions/SecurityProfiles.cs | Core curated profiles (CIS L1 / max / dev). |
| src/WindowsSecurityManager.Core/Definitions/DefenderSettings.cs | Core Defender settings provider. |
| src/WindowsSecurityManager.Core/Definitions/AsrSettings.cs | Core ASR rules provider. |
| src/WindowsSecurityManager.Core/Definitions/FirewallSettings.cs | Core Firewall settings provider. |
| src/WindowsSecurityManager.Core/Definitions/CisBenchmarkSettings.cs | Core CIS benchmark settings provider. |
| src/WindowsSecurityManager.Core/Definitions/AccountPolicySettings.cs | Core account policy settings provider. |
| src/WindowsSecurityManager.Core/Definitions/NetworkSecuritySettings.cs | Core network security settings provider. |
| README.md | Updates docs to describe GUI app, build/run, and release assets. |
| .github/workflows/release.yml | Publishes CLI + GUI artifacts and attaches them to GitHub Releases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private void Enable_Click(object sender, RoutedEventArgs e) | ||
| { | ||
| if (sender is Button btn && btn.Tag is string id) | ||
| { | ||
| _viewModel.EnableSetting(id); | ||
| UpdateList(); | ||
| ShowStatus($"Enabled: {id}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Enable_Click always shows a success status message ("Enabled: {id}") even if EnableSetting returns false (e.g., unknown setting ID or failure). Consider having EnableSetting return a bool / expose an error message and only show success when the operation actually succeeded (and show an error severity otherwise).
| private void Disable_Click(object sender, RoutedEventArgs e) | ||
| { | ||
| if (sender is Button btn && btn.Tag is string id) | ||
| { | ||
| _viewModel.DisableSetting(id); | ||
| UpdateList(); | ||
| ShowStatus($"Disabled: {id}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Disable_Click always shows a success status message ("Disabled: {id}") even if DisableSetting returns false. Consider using the ViewModel’s StatusMessage (or a bool return) to decide whether to show success vs error, and avoid reporting a change that didn’t occur.
| [RelayCommand] | ||
| public async Task ExportReportAsync() | ||
| { | ||
| if (Report == null) | ||
| { | ||
| StatusMessage = "Generate a report first"; | ||
| return; | ||
| } | ||
|
|
||
| var exporter = new ReportExporter(); | ||
| var extension = SelectedFormat switch | ||
| { | ||
| ExportFormat.Json => "json", | ||
| ExportFormat.Csv => "csv", | ||
| ExportFormat.Html => "html", | ||
| _ => "txt" | ||
| }; | ||
|
|
||
| var fileName = $"wsm-report-{DateTime.Now:yyyyMMdd-HHmmss}.{extension}"; | ||
| var desktopPath = Environment.GetFolderPath(Environment.SpecialFolder.Desktop); | ||
| var filePath = Path.Combine(desktopPath, fileName); | ||
|
|
||
| await Task.Run(() => exporter.ExportToFile(Report, SelectedFormat, filePath)); | ||
| StatusMessage = $"Report exported to: {filePath}"; | ||
| } |
There was a problem hiding this comment.
ExportReportAsync writes directly to the Desktop path without any error handling. If the Desktop folder is unavailable or the write fails (permissions/IO), this can throw on the UI thread and crash the app; wrap the export in try/catch and surface a helpful StatusMessage instead.
| [RelayCommand] | ||
| public async Task CreateBackupAsync() | ||
| { | ||
| var service = new BackupService(_manager, _registryService); | ||
| var backup = service.CreateBackup(SelectedCategory); | ||
|
|
||
| var fileName = $"wsm-backup-{DateTime.Now:yyyyMMdd-HHmmss}.json"; | ||
| var desktopPath = Environment.GetFolderPath(Environment.SpecialFolder.Desktop); | ||
| var filePath = Path.Combine(desktopPath, fileName); | ||
|
|
||
| await Task.Run(() => service.SaveToFile(backup, filePath)); | ||
| StatusMessage = $"Backup saved: {filePath} ({backup.Entries.Count} settings)"; | ||
| } |
There was a problem hiding this comment.
CreateBackupAsync performs file IO (Desktop path + SaveToFile) without exception handling. A failure (permissions, invalid path, disk full) will bubble up and can terminate the UI; catch expected IO exceptions and report them via StatusMessage.
| catch (Exception ex) when (ex is UnauthorizedAccessException or IOException) | ||
| { | ||
| // Audit logging will be unavailable | ||
| } |
There was a problem hiding this comment.
The exception variable in this catch filter is unused, which produces an 'declared but never used' warning. Consider removing the variable (or catching UnauthorizedAccessException/IOException separately) to keep the build warning-free.
| catch (Exception ex) when (ex is UnauthorizedAccessException or IOException) | |
| { | |
| // Audit logging will be unavailable | |
| } | |
| catch (UnauthorizedAccessException) | |
| { | |
| // Audit logging will be unavailable | |
| } | |
| catch (IOException) | |
| { | |
| // Audit logging will be unavailable | |
| } |
| [RelayCommand] | ||
| public void Refresh() | ||
| { | ||
| var report = _manager.GenerateReport(); | ||
| OverallCompliance = report.CompliancePercentage; | ||
| TotalSettings = report.TotalSettings; | ||
| EnabledCount = report.EnabledCount; | ||
| DisabledCount = report.DisabledCount - report.NotConfiguredCount; | ||
| NotConfiguredCount = report.NotConfiguredCount; | ||
|
|
||
| var categoryItems = new List<CategoryComplianceItem>(); | ||
| foreach (var category in Enum.GetValues<SecurityCategory>()) | ||
| { | ||
| var catReport = _manager.GenerateReport(category); | ||
| if (catReport.TotalSettings > 0) | ||
| { | ||
| categoryItems.Add(new CategoryComplianceItem | ||
| { | ||
| Category = category, | ||
| DisplayName = FormatCategoryName(category), | ||
| CompliancePercentage = catReport.CompliancePercentage, | ||
| EnabledCount = catReport.EnabledCount, | ||
| TotalCount = catReport.TotalSettings | ||
| }); | ||
| } | ||
| } | ||
| Categories = categoryItems; | ||
| } |
There was a problem hiding this comment.
DashboardViewModel.Refresh generates an overall report (reads registry for all settings) and then generates a separate report per category, which re-reads the registry for those settings again. This duplicates registry IO and can noticeably slow refresh as the number of settings grows; consider generating statuses once and computing overall + per-category aggregates from the same in-memory list.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 50 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| partial void OnSelectedCategoryChanged(SecurityCategory? value) => RefreshSettings(); | ||
| partial void OnSearchTextChanged(string value) => RefreshSettings(); | ||
|
|
There was a problem hiding this comment.
RefreshSettings() is invoked automatically on every SearchText change and every category change. Since RefreshSettings() performs registry reads for each setting (via GetSettingStatus), this will run on every keystroke in the search box and can easily freeze the UI. Consider debouncing search input and/or making refresh asynchronous (background thread) with cancellation to avoid repeated registry I/O on the UI thread.
| Settings = allSettings.Select(s => | ||
| { | ||
| var status = _manager.GetSettingStatus(s); | ||
| return new SettingItemViewModel | ||
| { | ||
| Id = s.Id, | ||
| Name = s.Name, | ||
| Description = s.Description, | ||
| Category = s.Category, | ||
| IsEnabled = status.IsEnabled, | ||
| IsConfigured = status.IsConfigured, | ||
| CurrentValue = status.CurrentValue?.ToString() ?? "Not configured", | ||
| ExpectedValue = s.EnabledValue.ToString() ?? "", | ||
| MatchesRecommended = status.MatchesRecommended | ||
| }; | ||
| }).ToList(); |
There was a problem hiding this comment.
RefreshSettings() calls _manager.GetSettingStatus(s) for each setting, which reads the registry synchronously. In the GUI this runs on the UI thread and will block rendering/interaction (especially when triggered by search/filter changes). Consider moving the status computation off the UI thread (async/await), caching statuses, or only recomputing statuses when explicitly refreshing rather than for every filter operation.
| public async Task CreateBackupAsync() | ||
| { | ||
| var service = new BackupService(_manager, _registryService); | ||
| var backup = service.CreateBackup(SelectedCategory); | ||
|
|
||
| var fileName = $"wsm-backup-{DateTime.Now:yyyyMMdd-HHmmss}.json"; | ||
| var desktopPath = Environment.GetFolderPath(Environment.SpecialFolder.Desktop); | ||
| var filePath = Path.Combine(desktopPath, fileName); | ||
|
|
||
| await Task.Run(() => service.SaveToFile(backup, filePath)); | ||
| StatusMessage = $"Backup saved: {filePath} ({backup.Entries.Count} settings)"; | ||
| } |
There was a problem hiding this comment.
CreateBackupAsync() performs the expensive registry read (service.CreateBackup(...)) synchronously before the Task.Run, so the UI thread can still freeze while collecting values. Also, any IO exceptions from SaveToFile (e.g., Desktop path unavailable/permissions) will currently propagate and can crash the app. Consider running the full backup operation off the UI thread and handling exceptions to set a user-friendly StatusMessage.
| [RelayCommand] | ||
| public void GenerateReport() | ||
| { | ||
| Report = _manager.GenerateReport(SelectedCategory); | ||
| StatusMessage = $"Report generated: {Report.TotalSettings} settings, {Report.CompliancePercentage:F1}% compliance"; | ||
| } |
There was a problem hiding this comment.
GenerateReport() runs _manager.GenerateReport(...) synchronously, which reads the registry for every setting. In the GUI this is likely executed on the UI thread (e.g., button click) and can cause noticeable UI hangs. Consider making report generation async (background thread with cancellation) and updating Report/StatusMessage when complete.
| public async Task ExportReportAsync() | ||
| { | ||
| if (Report == null) | ||
| { | ||
| StatusMessage = "Generate a report first"; | ||
| return; | ||
| } | ||
|
|
||
| var exporter = new ReportExporter(); | ||
| var extension = SelectedFormat switch | ||
| { | ||
| ExportFormat.Json => "json", | ||
| ExportFormat.Csv => "csv", | ||
| ExportFormat.Html => "html", | ||
| _ => "txt" | ||
| }; | ||
|
|
||
| var fileName = $"wsm-report-{DateTime.Now:yyyyMMdd-HHmmss}.{extension}"; | ||
| var desktopPath = Environment.GetFolderPath(Environment.SpecialFolder.Desktop); | ||
| var filePath = Path.Combine(desktopPath, fileName); | ||
|
|
||
| await Task.Run(() => exporter.ExportToFile(Report, SelectedFormat, filePath)); | ||
| StatusMessage = $"Report exported to: {filePath}"; | ||
| } |
There was a problem hiding this comment.
ExportReportAsync() writes the export to the user's Desktop but doesn't handle common failures (missing/unavailable Desktop folder, access denied, invalid path, etc.). Because the call originates from an async void event handler, an exception here can crash the app. Wrap the export in a try/catch and surface a clear StatusMessage (and optionally set InfoBar severity) on failure.
| <PropertyGroup> | ||
| <TargetFramework>net8.0-windows</TargetFramework> | ||
| <ImplicitUsings>enable</ImplicitUsings> | ||
| <Nullable>enable</Nullable> |
There was a problem hiding this comment.
The PR description notes that on non-Windows hosts you can build Core + CLI individually, but both the CLI and Core projects target net8.0-windows. Building Windows-targeted TFMs on non-Windows typically requires explicitly setting EnableWindowsTargeting=true, otherwise dotnet build fails. Either update the documentation/PR description to reflect this, or adjust the project settings/TFMs if cross-platform builds are intended.
|
@copilot apply changes based on the comments in this thread |
…dup registry IO Agent-Logs-Url: https://github.com/Kvikku/Windows-Security-Manager/sessions/b26b2cce-e8b0-4606-bb89-ebd20336c5a9 Co-authored-by: Kvikku <85877485+Kvikku@users.noreply.github.com>
…cription pattern Agent-Logs-Url: https://github.com/Kvikku/Windows-Security-Manager/sessions/b26b2cce-e8b0-4606-bb89-ebd20336c5a9 Co-authored-by: Kvikku <85877485+Kvikku@users.noreply.github.com>
All review feedback addressed in commits
|
# Conflicts: # src/WindowsSecurityManager/WindowsSecurityManager.csproj
Adds a WinUI 3 desktop GUI alongside the existing CLI, sharing a newly extracted Core library.
Core library extraction
src/WindowsSecurityManager.Core/class libraryRootNamespace=WindowsSecurityManagerpreserves all namespaces — zero breaking changesProjectReferenceWinUI 3 GUI (
src/WindowsSecurityManager.Gui/)Unpackaged WinUI 3 app (
WindowsPackageType=None) using Windows App SDK 1.8 and CommunityToolkit.Mvvm 8.4.2.NavigationViewshell with 5 sectionsInfoBarstatusSame bootstrap as CLI: 6 providers →
SecuritySettingsManager→AuditLogger.CI/CD
release.ymlnow publishes both CLI (single.exe) and GUI (.zip) as GitHub Release assetsci.ymlunchanged —dotnet buildonwindows-latestalready builds the full solutionSolution structure