From 1f1c4146329d611187e36822539d811e6e296eb4 Mon Sep 17 00:00:00 2001 From: "goodolclint-claude[bot]" <323206664+goodolclint-claude[bot]@users.noreply.github.com> Date: Thu, 3 Sep 2026 00:44:03 +0000 Subject: [PATCH] refactor: one firewall scope validator instead of 18 copies (#204) Collapses the ~24-line Level/Node/VmId/Group validation block duplicated across 18 Firewall cmdlets into a single FirewallScope.TryValidate helper in PSProxmoxVE.Core, beside FirewallService.BuildBasePath which already owns the level-to-path mapping (ADR 0021: request-payload/validation correctness is proven offline, not against a live cluster). Each cmdlet now makes one TryValidate call and, on failure, one ThrowTerminatingError with the same ErrorId (NodeRequired/VmIdRequired/ GroupRequired), ErrorCategory.InvalidArgument, target object (null) and message text it used before, so the Pester assertions under tests/PSProxmoxVE.Tests/Firewall/ keep passing unedited. Part of #154. Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com> --- .../Services/FirewallScope.cs | 49 ++++++++ .../Firewall/GetPveFirewallAliasCmdlet.cs | 23 +--- .../Firewall/GetPveFirewallIpSetCmdlet.cs | 23 +--- .../GetPveFirewallIpSetEntryCmdlet.cs | 24 +--- .../Firewall/GetPveFirewallOptionsCmdlet.cs | 24 +--- .../Firewall/GetPveFirewallRefCmdlet.cs | 24 +--- .../Firewall/GetPveFirewallRuleCmdlet.cs | 34 +----- .../Firewall/NewPveFirewallAliasCmdlet.cs | 24 +--- .../Firewall/NewPveFirewallIpSetCmdlet.cs | 24 +--- .../NewPveFirewallIpSetEntryCmdlet.cs | 24 +--- .../Firewall/NewPveFirewallRuleCmdlet.cs | 34 +----- .../Firewall/RemovePveFirewallAliasCmdlet.cs | 24 +--- .../Firewall/RemovePveFirewallIpSetCmdlet.cs | 24 +--- .../RemovePveFirewallIpSetEntryCmdlet.cs | 24 +--- .../Firewall/RemovePveFirewallRuleCmdlet.cs | 34 +----- .../Firewall/SetPveFirewallAliasCmdlet.cs | 24 +--- .../SetPveFirewallIpSetEntryCmdlet.cs | 24 +--- .../Firewall/SetPveFirewallOptionsCmdlet.cs | 24 +--- .../Firewall/SetPveFirewallRuleCmdlet.cs | 34 +----- .../Services/FirewallServiceTests.cs | 111 ++++++++++++++++++ 20 files changed, 232 insertions(+), 398 deletions(-) create mode 100644 src/PSProxmoxVE.Core/Services/FirewallScope.cs diff --git a/src/PSProxmoxVE.Core/Services/FirewallScope.cs b/src/PSProxmoxVE.Core/Services/FirewallScope.cs new file mode 100644 index 0000000..92fa808 --- /dev/null +++ b/src/PSProxmoxVE.Core/Services/FirewallScope.cs @@ -0,0 +1,49 @@ +using System; + +namespace PSProxmoxVE.Core.Services +{ + /// + /// Validates the Level/Node/VmId/Group combination shared by the firewall cmdlets. + /// + public static class FirewallScope + { + /// + /// Returns false and sets / to the + /// first violated rule (Node, then VmId, then Group) when the identifiers required for + /// are missing; otherwise returns true. + /// + public static bool TryValidate(string level, string? node, int? vmid, string? group, + out string errorId, out string message) + { + var isCluster = string.Equals(level, "Cluster", StringComparison.OrdinalIgnoreCase); + var isGroup = string.Equals(level, "Group", StringComparison.OrdinalIgnoreCase); + var isVmOrContainer = string.Equals(level, "Vm", StringComparison.OrdinalIgnoreCase) || + string.Equals(level, "Container", StringComparison.OrdinalIgnoreCase); + + if (!isCluster && !isGroup && string.IsNullOrEmpty(node)) + { + errorId = "NodeRequired"; + message = "Node is required when Level is not Cluster."; + return false; + } + + if (isVmOrContainer && !vmid.HasValue) + { + errorId = "VmIdRequired"; + message = "VmId is required when Level is Vm or Container."; + return false; + } + + if (isGroup && string.IsNullOrWhiteSpace(group)) + { + errorId = "GroupRequired"; + message = "Group is required when Level is Group."; + return false; + } + + errorId = string.Empty; + message = string.Empty; + return true; + } + } +} diff --git a/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallAliasCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallAliasCmdlet.cs index 8b81f87..5812cfe 100644 --- a/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallAliasCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallAliasCmdlet.cs @@ -27,26 +27,11 @@ namespace PSProxmoxVE.Cmdlets.Firewall protected override void ProcessRecord() { var level = Level; - if (!string.Equals(level, "Cluster", StringComparison.OrdinalIgnoreCase)) + if (!FirewallScope.TryValidate(level, Node, VmId, null, out var scopeErrorId, out var scopeMessage)) { - if (string.IsNullOrEmpty(Node)) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("Node is required when Level is not Cluster."), - "NodeRequired", ErrorCategory.InvalidArgument, null)); - return; - } - } - if (string.Equals(level, "Vm", StringComparison.OrdinalIgnoreCase) || - string.Equals(level, "Container", StringComparison.OrdinalIgnoreCase)) - { - if (!VmId.HasValue) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("VmId is required when Level is Vm or Container."), - "VmIdRequired", ErrorCategory.InvalidArgument, null)); - return; - } + ThrowTerminatingError(new ErrorRecord( + new PSArgumentException(scopeMessage), scopeErrorId, ErrorCategory.InvalidArgument, null)); + return; } var session = GetSession(); diff --git a/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallIpSetCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallIpSetCmdlet.cs index 402bd7e..ad92a53 100644 --- a/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallIpSetCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallIpSetCmdlet.cs @@ -27,26 +27,11 @@ namespace PSProxmoxVE.Cmdlets.Firewall protected override void ProcessRecord() { var level = Level; - if (!string.Equals(level, "Cluster", StringComparison.OrdinalIgnoreCase)) + if (!FirewallScope.TryValidate(level, Node, VmId, null, out var scopeErrorId, out var scopeMessage)) { - if (string.IsNullOrEmpty(Node)) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("Node is required when Level is not Cluster."), - "NodeRequired", ErrorCategory.InvalidArgument, null)); - return; - } - } - if (string.Equals(level, "Vm", StringComparison.OrdinalIgnoreCase) || - string.Equals(level, "Container", StringComparison.OrdinalIgnoreCase)) - { - if (!VmId.HasValue) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("VmId is required when Level is Vm or Container."), - "VmIdRequired", ErrorCategory.InvalidArgument, null)); - return; - } + ThrowTerminatingError(new ErrorRecord( + new PSArgumentException(scopeMessage), scopeErrorId, ErrorCategory.InvalidArgument, null)); + return; } var session = GetSession(); diff --git a/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallIpSetEntryCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallIpSetEntryCmdlet.cs index 5193e24..6b7d860 100644 --- a/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallIpSetEntryCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallIpSetEntryCmdlet.cs @@ -1,4 +1,3 @@ -using System; using System.Management.Automation; using PSProxmoxVE.Core.Models.Firewall; using PSProxmoxVE.Core.Services; @@ -26,26 +25,11 @@ namespace PSProxmoxVE.Cmdlets.Firewall protected override void ProcessRecord() { var level = Level; - if (!string.Equals(level, "Cluster", StringComparison.OrdinalIgnoreCase)) + if (!FirewallScope.TryValidate(level, Node, VmId, null, out var scopeErrorId, out var scopeMessage)) { - if (string.IsNullOrEmpty(Node)) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("Node is required when Level is not Cluster."), - "NodeRequired", ErrorCategory.InvalidArgument, null)); - return; - } - } - if (string.Equals(level, "Vm", StringComparison.OrdinalIgnoreCase) || - string.Equals(level, "Container", StringComparison.OrdinalIgnoreCase)) - { - if (!VmId.HasValue) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("VmId is required when Level is Vm or Container."), - "VmIdRequired", ErrorCategory.InvalidArgument, null)); - return; - } + ThrowTerminatingError(new ErrorRecord( + new PSArgumentException(scopeMessage), scopeErrorId, ErrorCategory.InvalidArgument, null)); + return; } var session = GetSession(); diff --git a/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallOptionsCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallOptionsCmdlet.cs index bb01744..ea7b238 100644 --- a/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallOptionsCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallOptionsCmdlet.cs @@ -1,4 +1,3 @@ -using System; using System.Management.Automation; using PSProxmoxVE.Core.Models.Firewall; using PSProxmoxVE.Core.Services; @@ -23,26 +22,11 @@ namespace PSProxmoxVE.Cmdlets.Firewall protected override void ProcessRecord() { var level = Level; - if (!string.Equals(level, "Cluster", StringComparison.OrdinalIgnoreCase)) + if (!FirewallScope.TryValidate(level, Node, VmId, null, out var scopeErrorId, out var scopeMessage)) { - if (string.IsNullOrEmpty(Node)) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("Node is required when Level is not Cluster."), - "NodeRequired", ErrorCategory.InvalidArgument, null)); - return; - } - } - if (string.Equals(level, "Vm", StringComparison.OrdinalIgnoreCase) || - string.Equals(level, "Container", StringComparison.OrdinalIgnoreCase)) - { - if (!VmId.HasValue) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("VmId is required when Level is Vm or Container."), - "VmIdRequired", ErrorCategory.InvalidArgument, null)); - return; - } + ThrowTerminatingError(new ErrorRecord( + new PSArgumentException(scopeMessage), scopeErrorId, ErrorCategory.InvalidArgument, null)); + return; } var session = GetSession(); diff --git a/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallRefCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallRefCmdlet.cs index e67fc0e..55ab7b0 100644 --- a/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallRefCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallRefCmdlet.cs @@ -1,4 +1,3 @@ -using System; using System.Management.Automation; using PSProxmoxVE.Core.Models.Firewall; using PSProxmoxVE.Core.Services; @@ -26,26 +25,11 @@ namespace PSProxmoxVE.Cmdlets.Firewall protected override void ProcessRecord() { var level = Level; - if (!string.Equals(level, "Cluster", StringComparison.OrdinalIgnoreCase)) + if (!FirewallScope.TryValidate(level, Node, VmId, null, out var scopeErrorId, out var scopeMessage)) { - if (string.IsNullOrEmpty(Node)) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("Node is required when Level is not Cluster."), - "NodeRequired", ErrorCategory.InvalidArgument, null)); - return; - } - } - if (string.Equals(level, "Vm", StringComparison.OrdinalIgnoreCase) || - string.Equals(level, "Container", StringComparison.OrdinalIgnoreCase)) - { - if (!VmId.HasValue) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("VmId is required when Level is Vm or Container."), - "VmIdRequired", ErrorCategory.InvalidArgument, null)); - return; - } + ThrowTerminatingError(new ErrorRecord( + new PSArgumentException(scopeMessage), scopeErrorId, ErrorCategory.InvalidArgument, null)); + return; } var session = GetSession(); diff --git a/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallRuleCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallRuleCmdlet.cs index 4a5c35e..0d822c5 100644 --- a/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallRuleCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Firewall/GetPveFirewallRuleCmdlet.cs @@ -30,37 +30,11 @@ namespace PSProxmoxVE.Cmdlets.Firewall protected override void ProcessRecord() { var level = Level; - if (!string.Equals(level, "Cluster", StringComparison.OrdinalIgnoreCase) && - !string.Equals(level, "Group", StringComparison.OrdinalIgnoreCase)) + if (!FirewallScope.TryValidate(level, Node, VmId, Group, out var scopeErrorId, out var scopeMessage)) { - if (string.IsNullOrEmpty(Node)) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("Node is required when Level is not Cluster."), - "NodeRequired", ErrorCategory.InvalidArgument, null)); - return; - } - } - if (string.Equals(level, "Vm", StringComparison.OrdinalIgnoreCase) || - string.Equals(level, "Container", StringComparison.OrdinalIgnoreCase)) - { - if (!VmId.HasValue) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("VmId is required when Level is Vm or Container."), - "VmIdRequired", ErrorCategory.InvalidArgument, null)); - return; - } - } - if (string.Equals(level, "Group", StringComparison.OrdinalIgnoreCase)) - { - if (string.IsNullOrWhiteSpace(Group)) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("Group is required when Level is Group."), - "GroupRequired", ErrorCategory.InvalidArgument, null)); - return; - } + ThrowTerminatingError(new ErrorRecord( + new PSArgumentException(scopeMessage), scopeErrorId, ErrorCategory.InvalidArgument, null)); + return; } var session = GetSession(); diff --git a/src/PSProxmoxVE/Cmdlets/Firewall/NewPveFirewallAliasCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Firewall/NewPveFirewallAliasCmdlet.cs index e8515e3..220baac 100644 --- a/src/PSProxmoxVE/Cmdlets/Firewall/NewPveFirewallAliasCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Firewall/NewPveFirewallAliasCmdlet.cs @@ -1,4 +1,3 @@ -using System; using System.Management.Automation; using PSProxmoxVE.Core.Models.Firewall; using PSProxmoxVE.Core.Services; @@ -32,26 +31,11 @@ namespace PSProxmoxVE.Cmdlets.Firewall protected override void ProcessRecord() { var level = Level; - if (!string.Equals(level, "Cluster", StringComparison.OrdinalIgnoreCase)) + if (!FirewallScope.TryValidate(level, Node, VmId, null, out var scopeErrorId, out var scopeMessage)) { - if (string.IsNullOrEmpty(Node)) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("Node is required when Level is not Cluster."), - "NodeRequired", ErrorCategory.InvalidArgument, null)); - return; - } - } - if (string.Equals(level, "Vm", StringComparison.OrdinalIgnoreCase) || - string.Equals(level, "Container", StringComparison.OrdinalIgnoreCase)) - { - if (!VmId.HasValue) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("VmId is required when Level is Vm or Container."), - "VmIdRequired", ErrorCategory.InvalidArgument, null)); - return; - } + ThrowTerminatingError(new ErrorRecord( + new PSArgumentException(scopeMessage), scopeErrorId, ErrorCategory.InvalidArgument, null)); + return; } if (!ShouldProcess($"firewall alias '{Name}' ({Level})", "Create")) diff --git a/src/PSProxmoxVE/Cmdlets/Firewall/NewPveFirewallIpSetCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Firewall/NewPveFirewallIpSetCmdlet.cs index 789a3c3..0556ff5 100644 --- a/src/PSProxmoxVE/Cmdlets/Firewall/NewPveFirewallIpSetCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Firewall/NewPveFirewallIpSetCmdlet.cs @@ -1,4 +1,3 @@ -using System; using System.Management.Automation; using PSProxmoxVE.Core.Models.Firewall; using PSProxmoxVE.Core.Services; @@ -29,26 +28,11 @@ namespace PSProxmoxVE.Cmdlets.Firewall protected override void ProcessRecord() { var level = Level; - if (!string.Equals(level, "Cluster", StringComparison.OrdinalIgnoreCase)) + if (!FirewallScope.TryValidate(level, Node, VmId, null, out var scopeErrorId, out var scopeMessage)) { - if (string.IsNullOrEmpty(Node)) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("Node is required when Level is not Cluster."), - "NodeRequired", ErrorCategory.InvalidArgument, null)); - return; - } - } - if (string.Equals(level, "Vm", StringComparison.OrdinalIgnoreCase) || - string.Equals(level, "Container", StringComparison.OrdinalIgnoreCase)) - { - if (!VmId.HasValue) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("VmId is required when Level is Vm or Container."), - "VmIdRequired", ErrorCategory.InvalidArgument, null)); - return; - } + ThrowTerminatingError(new ErrorRecord( + new PSArgumentException(scopeMessage), scopeErrorId, ErrorCategory.InvalidArgument, null)); + return; } if (!ShouldProcess($"firewall IP set '{Name}' ({Level})", "Create")) diff --git a/src/PSProxmoxVE/Cmdlets/Firewall/NewPveFirewallIpSetEntryCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Firewall/NewPveFirewallIpSetEntryCmdlet.cs index eafacf6..0e0f20e 100644 --- a/src/PSProxmoxVE/Cmdlets/Firewall/NewPveFirewallIpSetEntryCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Firewall/NewPveFirewallIpSetEntryCmdlet.cs @@ -1,4 +1,3 @@ -using System; using System.Management.Automation; using PSProxmoxVE.Core.Models.Firewall; using PSProxmoxVE.Core.Services; @@ -35,26 +34,11 @@ namespace PSProxmoxVE.Cmdlets.Firewall protected override void ProcessRecord() { var level = Level; - if (!string.Equals(level, "Cluster", StringComparison.OrdinalIgnoreCase)) + if (!FirewallScope.TryValidate(level, Node, VmId, null, out var scopeErrorId, out var scopeMessage)) { - if (string.IsNullOrEmpty(Node)) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("Node is required when Level is not Cluster."), - "NodeRequired", ErrorCategory.InvalidArgument, null)); - return; - } - } - if (string.Equals(level, "Vm", StringComparison.OrdinalIgnoreCase) || - string.Equals(level, "Container", StringComparison.OrdinalIgnoreCase)) - { - if (!VmId.HasValue) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("VmId is required when Level is Vm or Container."), - "VmIdRequired", ErrorCategory.InvalidArgument, null)); - return; - } + ThrowTerminatingError(new ErrorRecord( + new PSArgumentException(scopeMessage), scopeErrorId, ErrorCategory.InvalidArgument, null)); + return; } if (!ShouldProcess($"IP set entry '{Cidr}' in '{Name}' ({Level})", "Create")) diff --git a/src/PSProxmoxVE/Cmdlets/Firewall/NewPveFirewallRuleCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Firewall/NewPveFirewallRuleCmdlet.cs index c349196..3d321f1 100644 --- a/src/PSProxmoxVE/Cmdlets/Firewall/NewPveFirewallRuleCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Firewall/NewPveFirewallRuleCmdlet.cs @@ -65,37 +65,11 @@ namespace PSProxmoxVE.Cmdlets.Firewall protected override void ProcessRecord() { var level = Level; - if (!string.Equals(level, "Cluster", StringComparison.OrdinalIgnoreCase) && - !string.Equals(level, "Group", StringComparison.OrdinalIgnoreCase)) + if (!FirewallScope.TryValidate(level, Node, VmId, Group, out var scopeErrorId, out var scopeMessage)) { - if (string.IsNullOrEmpty(Node)) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("Node is required when Level is not Cluster."), - "NodeRequired", ErrorCategory.InvalidArgument, null)); - return; - } - } - if (string.Equals(level, "Vm", StringComparison.OrdinalIgnoreCase) || - string.Equals(level, "Container", StringComparison.OrdinalIgnoreCase)) - { - if (!VmId.HasValue) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("VmId is required when Level is Vm or Container."), - "VmIdRequired", ErrorCategory.InvalidArgument, null)); - return; - } - } - if (string.Equals(level, "Group", StringComparison.OrdinalIgnoreCase)) - { - if (string.IsNullOrWhiteSpace(Group)) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("Group is required when Level is Group."), - "GroupRequired", ErrorCategory.InvalidArgument, null)); - return; - } + ThrowTerminatingError(new ErrorRecord( + new PSArgumentException(scopeMessage), scopeErrorId, ErrorCategory.InvalidArgument, null)); + return; } var target = string.Equals(level, "Group", StringComparison.OrdinalIgnoreCase) diff --git a/src/PSProxmoxVE/Cmdlets/Firewall/RemovePveFirewallAliasCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Firewall/RemovePveFirewallAliasCmdlet.cs index 4281744..5591ad2 100644 --- a/src/PSProxmoxVE/Cmdlets/Firewall/RemovePveFirewallAliasCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Firewall/RemovePveFirewallAliasCmdlet.cs @@ -1,4 +1,3 @@ -using System; using System.Management.Automation; using PSProxmoxVE.Core.Models.Firewall; using PSProxmoxVE.Core.Services; @@ -26,26 +25,11 @@ namespace PSProxmoxVE.Cmdlets.Firewall protected override void ProcessRecord() { var level = Level; - if (!string.Equals(level, "Cluster", StringComparison.OrdinalIgnoreCase)) + if (!FirewallScope.TryValidate(level, Node, VmId, null, out var scopeErrorId, out var scopeMessage)) { - if (string.IsNullOrEmpty(Node)) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("Node is required when Level is not Cluster."), - "NodeRequired", ErrorCategory.InvalidArgument, null)); - return; - } - } - if (string.Equals(level, "Vm", StringComparison.OrdinalIgnoreCase) || - string.Equals(level, "Container", StringComparison.OrdinalIgnoreCase)) - { - if (!VmId.HasValue) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("VmId is required when Level is Vm or Container."), - "VmIdRequired", ErrorCategory.InvalidArgument, null)); - return; - } + ThrowTerminatingError(new ErrorRecord( + new PSArgumentException(scopeMessage), scopeErrorId, ErrorCategory.InvalidArgument, null)); + return; } if (!ShouldProcess($"firewall alias '{Name}' ({Level})", "Remove")) diff --git a/src/PSProxmoxVE/Cmdlets/Firewall/RemovePveFirewallIpSetCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Firewall/RemovePveFirewallIpSetCmdlet.cs index 9e55d1c..e2af61e 100644 --- a/src/PSProxmoxVE/Cmdlets/Firewall/RemovePveFirewallIpSetCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Firewall/RemovePveFirewallIpSetCmdlet.cs @@ -1,4 +1,3 @@ -using System; using System.Management.Automation; using PSProxmoxVE.Core.Models.Firewall; using PSProxmoxVE.Core.Services; @@ -26,26 +25,11 @@ namespace PSProxmoxVE.Cmdlets.Firewall protected override void ProcessRecord() { var level = Level; - if (!string.Equals(level, "Cluster", StringComparison.OrdinalIgnoreCase)) + if (!FirewallScope.TryValidate(level, Node, VmId, null, out var scopeErrorId, out var scopeMessage)) { - if (string.IsNullOrEmpty(Node)) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("Node is required when Level is not Cluster."), - "NodeRequired", ErrorCategory.InvalidArgument, null)); - return; - } - } - if (string.Equals(level, "Vm", StringComparison.OrdinalIgnoreCase) || - string.Equals(level, "Container", StringComparison.OrdinalIgnoreCase)) - { - if (!VmId.HasValue) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("VmId is required when Level is Vm or Container."), - "VmIdRequired", ErrorCategory.InvalidArgument, null)); - return; - } + ThrowTerminatingError(new ErrorRecord( + new PSArgumentException(scopeMessage), scopeErrorId, ErrorCategory.InvalidArgument, null)); + return; } if (!ShouldProcess($"firewall IP set '{Name}' ({Level})", "Remove")) diff --git a/src/PSProxmoxVE/Cmdlets/Firewall/RemovePveFirewallIpSetEntryCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Firewall/RemovePveFirewallIpSetEntryCmdlet.cs index 7b1735a..a2e95a9 100644 --- a/src/PSProxmoxVE/Cmdlets/Firewall/RemovePveFirewallIpSetEntryCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Firewall/RemovePveFirewallIpSetEntryCmdlet.cs @@ -1,4 +1,3 @@ -using System; using System.Management.Automation; using PSProxmoxVE.Core.Models.Firewall; using PSProxmoxVE.Core.Services; @@ -29,26 +28,11 @@ namespace PSProxmoxVE.Cmdlets.Firewall protected override void ProcessRecord() { var level = Level; - if (!string.Equals(level, "Cluster", StringComparison.OrdinalIgnoreCase)) + if (!FirewallScope.TryValidate(level, Node, VmId, null, out var scopeErrorId, out var scopeMessage)) { - if (string.IsNullOrEmpty(Node)) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("Node is required when Level is not Cluster."), - "NodeRequired", ErrorCategory.InvalidArgument, null)); - return; - } - } - if (string.Equals(level, "Vm", StringComparison.OrdinalIgnoreCase) || - string.Equals(level, "Container", StringComparison.OrdinalIgnoreCase)) - { - if (!VmId.HasValue) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("VmId is required when Level is Vm or Container."), - "VmIdRequired", ErrorCategory.InvalidArgument, null)); - return; - } + ThrowTerminatingError(new ErrorRecord( + new PSArgumentException(scopeMessage), scopeErrorId, ErrorCategory.InvalidArgument, null)); + return; } if (!ShouldProcess($"IP set entry '{Cidr}' from '{Name}' ({Level})", "Remove")) diff --git a/src/PSProxmoxVE/Cmdlets/Firewall/RemovePveFirewallRuleCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Firewall/RemovePveFirewallRuleCmdlet.cs index 4d4fb99..3c289e4 100644 --- a/src/PSProxmoxVE/Cmdlets/Firewall/RemovePveFirewallRuleCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Firewall/RemovePveFirewallRuleCmdlet.cs @@ -29,37 +29,11 @@ namespace PSProxmoxVE.Cmdlets.Firewall protected override void ProcessRecord() { var level = Level; - if (!string.Equals(level, "Cluster", StringComparison.OrdinalIgnoreCase) && - !string.Equals(level, "Group", StringComparison.OrdinalIgnoreCase)) + if (!FirewallScope.TryValidate(level, Node, VmId, Group, out var scopeErrorId, out var scopeMessage)) { - if (string.IsNullOrEmpty(Node)) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("Node is required when Level is not Cluster."), - "NodeRequired", ErrorCategory.InvalidArgument, null)); - return; - } - } - if (string.Equals(level, "Vm", StringComparison.OrdinalIgnoreCase) || - string.Equals(level, "Container", StringComparison.OrdinalIgnoreCase)) - { - if (!VmId.HasValue) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("VmId is required when Level is Vm or Container."), - "VmIdRequired", ErrorCategory.InvalidArgument, null)); - return; - } - } - if (string.Equals(level, "Group", StringComparison.OrdinalIgnoreCase)) - { - if (string.IsNullOrWhiteSpace(Group)) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("Group is required when Level is Group."), - "GroupRequired", ErrorCategory.InvalidArgument, null)); - return; - } + ThrowTerminatingError(new ErrorRecord( + new PSArgumentException(scopeMessage), scopeErrorId, ErrorCategory.InvalidArgument, null)); + return; } var target = string.Equals(level, "Group", StringComparison.OrdinalIgnoreCase) diff --git a/src/PSProxmoxVE/Cmdlets/Firewall/SetPveFirewallAliasCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Firewall/SetPveFirewallAliasCmdlet.cs index d2766f1..582f79c 100644 --- a/src/PSProxmoxVE/Cmdlets/Firewall/SetPveFirewallAliasCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Firewall/SetPveFirewallAliasCmdlet.cs @@ -1,4 +1,3 @@ -using System; using System.Management.Automation; using PSProxmoxVE.Core.Models.Firewall; using PSProxmoxVE.Core.Services; @@ -32,26 +31,11 @@ namespace PSProxmoxVE.Cmdlets.Firewall protected override void ProcessRecord() { var level = Level; - if (!string.Equals(level, "Cluster", StringComparison.OrdinalIgnoreCase)) + if (!FirewallScope.TryValidate(level, Node, VmId, null, out var scopeErrorId, out var scopeMessage)) { - if (string.IsNullOrEmpty(Node)) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("Node is required when Level is not Cluster."), - "NodeRequired", ErrorCategory.InvalidArgument, null)); - return; - } - } - if (string.Equals(level, "Vm", StringComparison.OrdinalIgnoreCase) || - string.Equals(level, "Container", StringComparison.OrdinalIgnoreCase)) - { - if (!VmId.HasValue) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("VmId is required when Level is Vm or Container."), - "VmIdRequired", ErrorCategory.InvalidArgument, null)); - return; - } + ThrowTerminatingError(new ErrorRecord( + new PSArgumentException(scopeMessage), scopeErrorId, ErrorCategory.InvalidArgument, null)); + return; } if (!ShouldProcess($"firewall alias '{Name}' ({Level})", "Update")) diff --git a/src/PSProxmoxVE/Cmdlets/Firewall/SetPveFirewallIpSetEntryCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Firewall/SetPveFirewallIpSetEntryCmdlet.cs index 49eb4be..504e15b 100644 --- a/src/PSProxmoxVE/Cmdlets/Firewall/SetPveFirewallIpSetEntryCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Firewall/SetPveFirewallIpSetEntryCmdlet.cs @@ -1,4 +1,3 @@ -using System; using System.Management.Automation; using PSProxmoxVE.Core.Models.Firewall; using PSProxmoxVE.Core.Services; @@ -35,26 +34,11 @@ namespace PSProxmoxVE.Cmdlets.Firewall protected override void ProcessRecord() { var level = Level; - if (!string.Equals(level, "Cluster", StringComparison.OrdinalIgnoreCase)) + if (!FirewallScope.TryValidate(level, Node, VmId, null, out var scopeErrorId, out var scopeMessage)) { - if (string.IsNullOrEmpty(Node)) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("Node is required when Level is not Cluster."), - "NodeRequired", ErrorCategory.InvalidArgument, null)); - return; - } - } - if (string.Equals(level, "Vm", StringComparison.OrdinalIgnoreCase) || - string.Equals(level, "Container", StringComparison.OrdinalIgnoreCase)) - { - if (!VmId.HasValue) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("VmId is required when Level is Vm or Container."), - "VmIdRequired", ErrorCategory.InvalidArgument, null)); - return; - } + ThrowTerminatingError(new ErrorRecord( + new PSArgumentException(scopeMessage), scopeErrorId, ErrorCategory.InvalidArgument, null)); + return; } if (!ShouldProcess($"IP set entry '{Cidr}' in '{Name}' ({Level})", "Update")) diff --git a/src/PSProxmoxVE/Cmdlets/Firewall/SetPveFirewallOptionsCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Firewall/SetPveFirewallOptionsCmdlet.cs index 36fa255..947ad9d 100644 --- a/src/PSProxmoxVE/Cmdlets/Firewall/SetPveFirewallOptionsCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Firewall/SetPveFirewallOptionsCmdlet.cs @@ -1,4 +1,3 @@ -using System; using System.Collections.Generic; using System.Management.Automation; using PSProxmoxVE.Core.Models.Firewall; @@ -53,26 +52,11 @@ namespace PSProxmoxVE.Cmdlets.Firewall protected override void ProcessRecord() { var level = Level; - if (!string.Equals(level, "Cluster", StringComparison.OrdinalIgnoreCase)) + if (!FirewallScope.TryValidate(level, Node, VmId, null, out var scopeErrorId, out var scopeMessage)) { - if (string.IsNullOrEmpty(Node)) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("Node is required when Level is not Cluster."), - "NodeRequired", ErrorCategory.InvalidArgument, null)); - return; - } - } - if (string.Equals(level, "Vm", StringComparison.OrdinalIgnoreCase) || - string.Equals(level, "Container", StringComparison.OrdinalIgnoreCase)) - { - if (!VmId.HasValue) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("VmId is required when Level is Vm or Container."), - "VmIdRequired", ErrorCategory.InvalidArgument, null)); - return; - } + ThrowTerminatingError(new ErrorRecord( + new PSArgumentException(scopeMessage), scopeErrorId, ErrorCategory.InvalidArgument, null)); + return; } if (!ShouldProcess($"firewall options ({Level})", "Update")) diff --git a/src/PSProxmoxVE/Cmdlets/Firewall/SetPveFirewallRuleCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Firewall/SetPveFirewallRuleCmdlet.cs index 5eea509..de2e880 100644 --- a/src/PSProxmoxVE/Cmdlets/Firewall/SetPveFirewallRuleCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Firewall/SetPveFirewallRuleCmdlet.cs @@ -68,37 +68,11 @@ namespace PSProxmoxVE.Cmdlets.Firewall protected override void ProcessRecord() { var level = Level; - if (!string.Equals(level, "Cluster", StringComparison.OrdinalIgnoreCase) && - !string.Equals(level, "Group", StringComparison.OrdinalIgnoreCase)) + if (!FirewallScope.TryValidate(level, Node, VmId, Group, out var scopeErrorId, out var scopeMessage)) { - if (string.IsNullOrEmpty(Node)) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("Node is required when Level is not Cluster."), - "NodeRequired", ErrorCategory.InvalidArgument, null)); - return; - } - } - if (string.Equals(level, "Vm", StringComparison.OrdinalIgnoreCase) || - string.Equals(level, "Container", StringComparison.OrdinalIgnoreCase)) - { - if (!VmId.HasValue) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("VmId is required when Level is Vm or Container."), - "VmIdRequired", ErrorCategory.InvalidArgument, null)); - return; - } - } - if (string.Equals(level, "Group", StringComparison.OrdinalIgnoreCase)) - { - if (string.IsNullOrWhiteSpace(Group)) - { - ThrowTerminatingError(new ErrorRecord( - new PSArgumentException("Group is required when Level is Group."), - "GroupRequired", ErrorCategory.InvalidArgument, null)); - return; - } + ThrowTerminatingError(new ErrorRecord( + new PSArgumentException(scopeMessage), scopeErrorId, ErrorCategory.InvalidArgument, null)); + return; } var target = string.Equals(level, "Group", StringComparison.OrdinalIgnoreCase) diff --git a/tests/PSProxmoxVE.Core.Tests/Services/FirewallServiceTests.cs b/tests/PSProxmoxVE.Core.Tests/Services/FirewallServiceTests.cs index 62cfb5a..b0831d4 100644 --- a/tests/PSProxmoxVE.Core.Tests/Services/FirewallServiceTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Services/FirewallServiceTests.cs @@ -326,5 +326,116 @@ namespace PSProxmoxVE.Core.Tests.Services Assert.Throws("group", () => service.RemoveGroupRule(CreateSession(), " ", 0)); } + + // ------------------------------------------------------------------------- + // FirewallScope.TryValidate + // ------------------------------------------------------------------------- + + [Theory] + [InlineData("Cluster", null, null, null)] + [InlineData("cluster", null, null, null)] + [InlineData("Node", "pve1", null, null)] + [InlineData("NODE", "pve1", null, null)] + [InlineData("Vm", "pve1", 100, null)] + [InlineData("vm", "pve1", 100, null)] + [InlineData("Container", "pve1", 101, null)] + [InlineData("Group", null, null, "web-servers")] + [InlineData("GROUP", null, null, "web-servers")] + public void TryValidate_WithRequiredIdentifiers_ReturnsTrue(string level, string? node, int? vmid, string? group) + { + var result = FirewallScope.TryValidate(level, node, vmid, group, out var errorId, out var message); + + Assert.True(result); + Assert.Equal(string.Empty, errorId); + Assert.Equal(string.Empty, message); + } + + [Theory] + [InlineData("Node")] + [InlineData("Vm")] + [InlineData("Container")] + public void TryValidate_MissingNode_ReturnsFalseWithNodeRequired(string level) + { + var result = FirewallScope.TryValidate(level, null, 100, null, out var errorId, out var message); + + Assert.False(result); + Assert.Equal("NodeRequired", errorId); + Assert.Equal("Node is required when Level is not Cluster.", message); + } + + [Fact] + public void TryValidate_WhitespaceNode_IsAcceptedLikeTheOriginalIsNullOrEmptyCheck() + { + var result = FirewallScope.TryValidate("Node", " ", null, null, out var errorId, out var message); + + Assert.True(result); + Assert.Equal(string.Empty, errorId); + Assert.Equal(string.Empty, message); + } + + [Fact] + public void TryValidate_VmMissingNodeAndVmId_ReturnsFalseWithNodeRequired() + { + var result = FirewallScope.TryValidate("Vm", null, null, null, out var errorId, out var message); + + Assert.False(result); + Assert.Equal("NodeRequired", errorId); + Assert.Equal("Node is required when Level is not Cluster.", message); + } + + [Theory] + [InlineData("Vm")] + [InlineData("Container")] + public void TryValidate_MissingVmId_ReturnsFalseWithVmIdRequired(string level) + { + var result = FirewallScope.TryValidate(level, "pve1", null, null, out var errorId, out var message); + + Assert.False(result); + Assert.Equal("VmIdRequired", errorId); + Assert.Equal("VmId is required when Level is Vm or Container.", message); + } + + [Fact] + public void TryValidate_GroupMissingGroup_ReturnsFalseWithGroupRequired() + { + var result = FirewallScope.TryValidate("Group", null, null, null, out var errorId, out var message); + + Assert.False(result); + Assert.Equal("GroupRequired", errorId); + Assert.Equal("Group is required when Level is Group.", message); + } + + [Fact] + public void TryValidate_GroupWhitespaceGroup_ReturnsFalseWithGroupRequired() + { + var result = FirewallScope.TryValidate("Group", null, null, " ", out var errorId, out var message); + + Assert.False(result); + Assert.Equal("GroupRequired", errorId); + Assert.Equal("Group is required when Level is Group.", message); + } + + // ------------------------------------------------------------------------- + // GetRules base path (cross-checked against an accepted FirewallScope.TryValidate scope) + // ------------------------------------------------------------------------- + + [Theory] + [InlineData("Cluster", null, null, "cluster/firewall")] + [InlineData("Node", "pve1", null, "nodes/pve1/firewall")] + [InlineData("Vm", "pve1", 100, "nodes/pve1/qemu/100/firewall")] + [InlineData("Container", "pve1", 101, "nodes/pve1/lxc/101/firewall")] + public void GetRules_AcceptedScope_UsesExpectedBasePath( + string level, string? node, int? vmid, string expectedPath) + { + Assert.True(FirewallScope.TryValidate(level, node, vmid, null, out _, out _)); + + var mockClient = new Mock(); + mockClient.Setup(c => c.GetAsync(It.IsAny())).ReturnsAsync(RulesJson()); + var service = new FirewallService(mockClient.Object); + + service.GetRules(CreateSession(), level, node, vmid); + + mockClient.Verify(c => c.GetAsync($"{expectedPath}/rules"), Times.Once); + } } }