Implement automated remediation workflows#6
Conversation
Co-authored-by: robgrame <12012136+robgrame@users.noreply.github.com>
Co-authored-by: robgrame <12012136+robgrame@users.noreply.github.com>
…nd UI Co-authored-by: robgrame <12012136+robgrame@users.noreply.github.com>
Co-authored-by: robgrame <12012136+robgrame@users.noreply.github.com>
Co-authored-by: robgrame <12012136+robgrame@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This pull request implements the Automated Remediation Workflows feature for the Secure Boot Certificate Watcher project. It enables administrators to define and execute automated actions in response to specific device conditions and alerts.
Key Changes:
- Introduces workflow engine with trigger evaluation and action execution
- Adds database models and migrations for workflows and execution history
- Implements REST API endpoints for workflow management
- Creates web UI pages for workflow creation, listing, and details viewing
Reviewed Changes
Copilot reviewed 26 out of 28 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/AUTOMATED_REMEDIATION_WORKFLOWS.md | Comprehensive documentation for the new workflows feature |
| SecureBootWatcher.Shared/Models/RemediationWorkflow.cs | Shared data models for workflows, triggers, and actions |
| SecureBootWatcher.Shared/Models/WorkflowExecution.cs | Execution tracking model with status enum |
| SecureBootDashboard.Api/Data/RemediationWorkflowEntity.cs | Database entity for storing workflow configurations |
| SecureBootDashboard.Api/Data/WorkflowExecutionEntity.cs | Database entity for execution history |
| SecureBootDashboard.Api/Data/SecureBootDbContext.cs | DbContext updates with new DbSets and relationships |
| SecureBootDashboard.Api/Data/Migrations/* | EF Core migration files for database schema |
| SecureBootDashboard.Api/Services/WorkflowEngine.cs | Core workflow evaluation and execution engine |
| SecureBootDashboard.Api/Controllers/RemediationWorkflowsController.cs | REST API endpoints for workflow CRUD operations |
| SecureBootDashboard.Api/Program.cs | Service registration for WorkflowEngine |
| SecureBootDashboard.Web/Pages/Workflows/* | Razor pages for UI (List, Create, Details) |
| SecureBootDashboard.Web/Pages/Shared/_Layout.cshtml | Navigation menu update |
| SecureBootDashboard.Api.Tests/Services/WorkflowEngineTests.cs | Unit tests for workflow engine |
| SecureBootDashboard.Api.Tests/*.csproj | Added EF Core InMemory package for tests |
| README.md | Updated feature list and roadmap |
Files not reviewed (1)
- SecureBootDashboard.Api/Data/Migrations/20251107060811_AddRemediationWorkflows.Designer.cs: Language not supported
| _logger.LogInformation("Created workflow {WorkflowId} with name length {NameLength}", | ||
| workflow.Id, workflow.Name.Length); |
There was a problem hiding this comment.
The log message 'with name length {NameLength}' is unclear and doesn't provide useful information. Consider logging the actual workflow name instead: _logger.LogInformation(\"Created workflow {WorkflowId} '{WorkflowName}'\", workflow.Id, workflow.Name);
| _logger.LogInformation("Created workflow {WorkflowId} with name length {NameLength}", | |
| workflow.Id, workflow.Name.Length); | |
| _logger.LogInformation("Created workflow {WorkflowId} '{WorkflowName}'", | |
| workflow.Id, workflow.Name); |
| _logger.LogInformation("Updated workflow {WorkflowId} with name length {NameLength}", | ||
| workflow.Id, workflow.Name.Length); |
There was a problem hiding this comment.
The log message 'with name length {NameLength}' is unclear and doesn't provide useful information. Consider logging the actual workflow name instead: _logger.LogInformation(\"Updated workflow {WorkflowId} '{WorkflowName}'\", workflow.Id, workflow.Name);
| _logger.LogInformation("Updated workflow {WorkflowId} with name length {NameLength}", | |
| workflow.Id, workflow.Name.Length); | |
| _logger.LogInformation("Updated workflow {WorkflowId} '{WorkflowName}'", | |
| workflow.Id, workflow.Name); |
| foreach (var workflowEntity in workflows) | ||
| { | ||
| var workflow = MapToWorkflow(workflowEntity); | ||
| if (await EvaluateTriggerAsync(workflow.Trigger, device, report, cancellationToken)) | ||
| { | ||
| _logger.LogInformation("Workflow {WorkflowId} ({WorkflowName}) triggered for device {DeviceId}", | ||
| workflow.Id, workflow.Name, deviceId); | ||
|
|
||
| var execution = await ExecuteWorkflowAsync(workflow, device, report, cancellationToken); | ||
| executions.Add(execution); | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
| if (!string.IsNullOrEmpty(trigger.DeploymentState)) | ||
| { | ||
| if (!string.Equals(report.DeploymentState, trigger.DeploymentState, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
| if (!string.IsNullOrEmpty(trigger.DeploymentState)) | |
| { | |
| if (!string.Equals(report.DeploymentState, trigger.DeploymentState, StringComparison.OrdinalIgnoreCase)) | |
| { | |
| return false; | |
| } | |
| if (!string.IsNullOrEmpty(trigger.DeploymentState) && | |
| !string.Equals(report.DeploymentState, trigger.DeploymentState, StringComparison.OrdinalIgnoreCase)) | |
| { | |
| return false; |
| if (!string.IsNullOrEmpty(trigger.ManufacturerMatches)) | ||
| { | ||
| if (!string.Equals(device.Manufacturer, trigger.ManufacturerMatches, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
| if (!string.IsNullOrEmpty(trigger.ManufacturerMatches)) | |
| { | |
| if (!string.Equals(device.Manufacturer, trigger.ManufacturerMatches, StringComparison.OrdinalIgnoreCase)) | |
| { | |
| return false; | |
| } | |
| if (!string.IsNullOrEmpty(trigger.ManufacturerMatches) && | |
| !string.Equals(device.Manufacturer, trigger.ManufacturerMatches, StringComparison.OrdinalIgnoreCase)) | |
| { | |
| return false; |
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Error executing action {ActionType}", action.ActionType); | ||
| result.Success = false; | ||
| result.Message = $"Action failed: {ex.Message}"; | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Error fetching workflows"); | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Error creating workflow"); | ||
| ModelState.AddModelError(string.Empty, "An error occurred while creating the workflow"); | ||
| return Page(); | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Error fetching workflow details for {WorkflowId}", id); | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
| catch (Exception ex) | |
| { | |
| _logger.LogError(ex, "Error fetching workflow details for {WorkflowId}", id); | |
| } | |
| catch (HttpRequestException ex) | |
| { | |
| _logger.LogError(ex, "HTTP error fetching workflow details for {WorkflowId}", id); | |
| } | |
| catch (System.Text.Json.JsonException ex) | |
| { | |
| _logger.LogError(ex, "JSON error deserializing workflow details for {WorkflowId}", id); | |
| } |
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Error deleting workflow {WorkflowId}", id); | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
|
Review and fix conflicts |
| CertificateExpiringWithinDays | ||
| }; | ||
|
|
||
| var actionTypeEnum = Enum.Parse<WorkflowActionType>(ActionType); |
There was a problem hiding this comment.
Enum.Parse<WorkflowActionType>(ActionType) can throw ArgumentException if the ActionType string doesn't match any enum value. This should be validated or use Enum.TryParse to prevent unhandled exceptions. Consider adding validation or catching this specific exception type.
| var actionTypeEnum = Enum.Parse<WorkflowActionType>(ActionType); | |
| if (!Enum.TryParse<WorkflowActionType>(ActionType, out var actionTypeEnum)) | |
| { | |
| ModelState.AddModelError(nameof(ActionType), "Invalid action type specified."); | |
| return Page(); | |
| } |
| public string ActionType { get; set; } = "LogEntry"; | ||
|
|
||
| [BindProperty] | ||
| public string ActionConfiguration { get; set; } = "{\"message\": \"Workflow triggered\"}"; |
There was a problem hiding this comment.
The ActionConfiguration property is not validated as JSON before submission. If users enter invalid JSON, it will cause errors during workflow execution. Consider adding client-side or server-side JSON validation to provide immediate feedback.
| function confirmDelete(id, name) { | ||
| if (confirm(`Are you sure you want to delete workflow "${name}"?`)) { |
There was a problem hiding this comment.
The workflow name is directly embedded into JavaScript string using @workflow.Name which can cause XSS vulnerabilities if the name contains quotes or special characters. Consider using Html.JavaScriptStringEncode or passing the data through a data attribute instead.
| var device = await _dbContext.Devices | ||
| .Include(d => d.Reports.OrderByDescending(r => r.CreatedAtUtc).Take(1)) | ||
| .FirstOrDefaultAsync(d => d.Id == deviceId, cancellationToken); | ||
|
|
||
| if (device == null) | ||
| { | ||
| _logger.LogWarning("Device {DeviceId} not found for workflow evaluation", deviceId); | ||
| return executions; | ||
| } | ||
|
|
||
| var report = device.Reports.FirstOrDefault(); | ||
| if (report == null) | ||
| { | ||
| _logger.LogWarning("No reports found for device {DeviceId}", deviceId); | ||
| return executions; | ||
| } |
There was a problem hiding this comment.
The method loads the device with only the latest report using Include(d => d.Reports.OrderByDescending(...).Take(1)), but then uses the specific reportId parameter elsewhere. This creates an inconsistency - the method should either load the specific report by ID, or not accept a reportId parameter at all if it always uses the latest report.
| var config = JsonSerializer.Deserialize<Dictionary<string, string>>(action.ConfigurationJson); | ||
| if (config == null) return; | ||
|
|
||
| var existingTags = string.IsNullOrEmpty(device.TagsJson) | ||
| ? new Dictionary<string, string>() | ||
| : JsonSerializer.Deserialize<Dictionary<string, string>>(device.TagsJson) ?? new Dictionary<string, string>(); |
There was a problem hiding this comment.
JSON deserialization failures are silently handled with null returns. If the ConfigurationJson or TagsJson contains invalid JSON, the deserialization will throw an exception that isn't caught here, potentially causing action execution to fail. Consider wrapping deserialization in try-catch or validating JSON format beforehand.
| if (trigger.HasExpiredCertificates.HasValue && trigger.HasExpiredCertificates.Value) | ||
| { | ||
| if (certificates?.ExpiredCertificateCount == 0) | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
| if (trigger.HasExpiredCertificates.HasValue && trigger.HasExpiredCertificates.Value) | |
| { | |
| if (certificates?.ExpiredCertificateCount == 0) | |
| { | |
| return false; | |
| } | |
| if (trigger.HasExpiredCertificates.HasValue && trigger.HasExpiredCertificates.Value && certificates?.ExpiredCertificateCount == 0) | |
| { | |
| return false; |
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Error evaluating trigger for workflow"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Error executing action {ActionType}", action.ActionType); | ||
| result.Success = false; | ||
| result.Message = $"Action failed: {ex.Message}"; | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
| catch (Exception ex) | |
| { | |
| _logger.LogError(ex, "Error executing action {ActionType}", action.ActionType); | |
| result.Success = false; | |
| result.Message = $"Action failed: {ex.Message}"; | |
| } | |
| catch (JsonException ex) | |
| { | |
| _logger.LogError(ex, "JSON error executing action {ActionType}", action.ActionType); | |
| result.Success = false; | |
| result.Message = $"JSON error: {ex.Message}"; | |
| } | |
| catch (DbUpdateException ex) | |
| { | |
| _logger.LogError(ex, "Database update error executing action {ActionType}", action.ActionType); | |
| result.Success = false; | |
| result.Message = $"Database error: {ex.Message}"; | |
| } | |
| catch (InvalidOperationException ex) | |
| { | |
| _logger.LogError(ex, "Invalid operation executing action {ActionType}", action.ActionType); | |
| result.Success = false; | |
| result.Message = $"Invalid operation: {ex.Message}"; | |
| } | |
| catch (Exception ex) when (!(ex is OutOfMemoryException) && !(ex is StackOverflowException) && !(ex is ThreadAbortException)) | |
| { | |
| _logger.LogError(ex, "Unexpected error executing action {ActionType}", action.ActionType); | |
| result.Success = false; | |
| result.Message = $"Unexpected error: {ex.Message}"; | |
| } |
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Error creating workflow"); | ||
| ModelState.AddModelError(string.Empty, "An error occurred while creating the workflow"); |
There was a problem hiding this comment.
Generic catch clause.
| catch (Exception ex) | |
| { | |
| _logger.LogError(ex, "Error creating workflow"); | |
| ModelState.AddModelError(string.Empty, "An error occurred while creating the workflow"); | |
| catch (HttpRequestException ex) | |
| { | |
| _logger.LogError(ex, "HTTP request error while creating workflow"); | |
| ModelState.AddModelError(string.Empty, "A network error occurred while creating the workflow"); | |
| return Page(); | |
| } | |
| catch (JsonException ex) | |
| { | |
| _logger.LogError(ex, "JSON serialization error while creating workflow"); | |
| ModelState.AddModelError(string.Empty, "A data processing error occurred while creating the workflow"); | |
| return Page(); | |
| } | |
| catch (ArgumentException ex) | |
| { | |
| _logger.LogError(ex, "Invalid argument while creating workflow"); | |
| ModelState.AddModelError(string.Empty, "Invalid input provided for workflow creation"); | |
| return Page(); | |
| } | |
| // Catch any other unexpected exceptions to prevent unhandled errors from leaking to the user. | |
| catch (Exception ex) | |
| { | |
| _logger.LogError(ex, "Unexpected error creating workflow"); | |
| ModelState.AddModelError(string.Empty, "An unexpected error occurred while creating the workflow"); |
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Error deleting workflow {WorkflowId}", id); | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
| catch (Exception ex) | |
| { | |
| _logger.LogError(ex, "Error deleting workflow {WorkflowId}", id); | |
| } | |
| catch (HttpRequestException ex) | |
| { | |
| _logger.LogError(ex, "HTTP error deleting workflow {WorkflowId}", id); | |
| } | |
| catch (TaskCanceledException ex) | |
| { | |
| _logger.LogError(ex, "Request timed out deleting workflow {WorkflowId}", id); | |
| } | |
| catch (Exception ex) when (!(ex is OutOfMemoryException) && !(ex is StackOverflowException) && !(ex is ThreadAbortException)) | |
| { | |
| _logger.LogError(ex, "Unexpected error deleting workflow {WorkflowId}", id); | |
| } |
| <nav aria-label="breadcrumb"> | ||
| <ol class="breadcrumb"> | ||
| <li class="breadcrumb-item"><a asp-page="./List">Workflows</a></li> | ||
| <li class="breadcrumb-item active" aria-current="page">@(Model.Workflow?.Name ?? "Unknown Workflow")</li> |
There was a problem hiding this comment.
[nitpick] Corrected spelling of 'Unknown Workflow' to maintain consistency with the error message on line 24, which uses 'Workflow not found.' Consider using 'Workflow Not Found' for consistency.
| <li class="breadcrumb-item active" aria-current="page">@(Model.Workflow?.Name ?? "Unknown Workflow")</li> | |
| <li class="breadcrumb-item active" aria-current="page">@(Model.Workflow?.Name ?? "Workflow Not Found")</li> |
| } | ||
| } | ||
| catch (Exception ex) | ||
| { |
There was a problem hiding this comment.
Generic catch clause.
| { | |
| { | |
| // Rethrow critical exceptions | |
| if (ex is OutOfMemoryException || | |
| ex is StackOverflowException || | |
| ex is ThreadAbortException) | |
| { | |
| throw; | |
| } |
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Error creating workflow"); | ||
| ModelState.AddModelError(string.Empty, "An error occurred while creating the workflow"); | ||
| return Page(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
| catch (Exception ex) | |
| { | |
| _logger.LogError(ex, "Error creating workflow"); | |
| ModelState.AddModelError(string.Empty, "An error occurred while creating the workflow"); | |
| return Page(); | |
| } | |
| } | |
| catch (HttpRequestException ex) | |
| { | |
| _logger.LogError(ex, "HTTP error creating workflow"); | |
| ModelState.AddModelError(string.Empty, "A network error occurred while creating the workflow"); | |
| return Page(); | |
| } | |
| catch (JsonException ex) | |
| { | |
| _logger.LogError(ex, "JSON error creating workflow"); | |
| ModelState.AddModelError(string.Empty, "A data error occurred while creating the workflow"); | |
| return Page(); | |
| } | |
| catch (ArgumentException ex) | |
| { | |
| _logger.LogError(ex, "Invalid argument creating workflow"); | |
| ModelState.AddModelError(string.Empty, "Invalid input provided for workflow creation"); | |
| return Page(); | |
| } |
| Executions = executions ?? new List<ExecutionHistory>(); | ||
| } | ||
| } | ||
| catch (Exception ex) |
There was a problem hiding this comment.
Generic catch clause.
| catch (Exception ex) | |
| catch (Exception ex) when (ex is not OperationCanceledException) |
Adds workflow automation for responding to device conditions and alerts per v1.2 roadmap.
Core Components
Data Models
RemediationWorkflow: workflow definition with triggers, actions, priorityWorkflowTrigger: conditions for execution (deployment state, cert expiration, fleet/manufacturer filters, alert text, device inactivity)WorkflowAction: typed actions (LogEntry, UpdateDeviceTags, EmailNotification*, Webhook*)WorkflowExecution: execution history with status and resultsEngine & API
WorkflowEngine: evaluates triggers against device/report data, executes actions sequentially, persists resultsRemediationWorkflowsController: REST endpoints for CRUD + execution historyRemediationWorkflows,WorkflowExecutionstables with proper indexingWeb UI
Example Workflow
Security
Sanitized user-provided values in logs to prevent log forging (cs/log-forging).
Testing
5 new unit tests covering trigger evaluation, action execution, workflow filtering. All 20 tests passing.
*Email and Webhook actions are placeholders for future implementation; engine supports extensibility.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.