From 48bab3e4cb07f5d969c655501901126c6457686b 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:07:45 +0000 Subject: [PATCH] refactor: route the Users cmdlets through UserService (#126) (#198) Get-PveRole, Get-PveUser, New-PveRole and Set-PvePermission each built their own PveHttpClient and hand-rolled requests, while UserService's GetRoles/GetUsers/CreateRole/SetPermission carried the same requests with zero callers. Wire the cmdlets to the service and change SetPermission's propagate/delete parameters from bool with defaults to nullable bool so the omitted-when-unset shipped behaviour of Set-PvePermission survives the move. Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com> --- src/PSProxmoxVE.Core/Services/UserService.cs | 14 ++-- .../Cmdlets/Users/GetPveRoleCmdlet.cs | 12 ++-- .../Cmdlets/Users/GetPveUserCmdlet.cs | 12 ++-- .../Cmdlets/Users/NewPveRoleCmdlet.cs | 13 +--- .../Cmdlets/Users/SetPvePermissionCmdlet.cs | 31 ++++---- .../Services/UserServiceTests.cs | 71 ++++++++++++++++++- 6 files changed, 102 insertions(+), 51 deletions(-) diff --git a/src/PSProxmoxVE.Core/Services/UserService.cs b/src/PSProxmoxVE.Core/Services/UserService.cs index e0295ba..f223491 100644 --- a/src/PSProxmoxVE.Core/Services/UserService.cs +++ b/src/PSProxmoxVE.Core/Services/UserService.cs @@ -531,8 +531,8 @@ namespace PSProxmoxVE.Core.Services /// Comma-separated user IDs. /// Comma-separated group names. /// Comma-separated API token IDs (user@realm!tokenid). - /// Whether to propagate the permission to sub-paths. - /// If true, removes the specified ACL entries. + /// Whether to propagate the permission to sub-paths; omitted when unset. + /// If true, removes the specified ACL entries; omitted when unset. public void SetPermission( PveSession session, string path, @@ -540,8 +540,8 @@ namespace PSProxmoxVE.Core.Services string? users = null, string? groups = null, string? tokens = null, - bool propagate = true, - bool delete = false) + bool? propagate = null, + bool? delete = null) { if (session == null) throw new ArgumentNullException(nameof(session)); if (string.IsNullOrWhiteSpace(path)) throw new ArgumentNullException(nameof(path)); @@ -550,13 +550,13 @@ namespace PSProxmoxVE.Core.Services var formData = new Dictionary { ["path"] = path, - ["roles"] = roles, - ["propagate"] = propagate ? "1" : "0", - ["delete"] = delete ? "1" : "0" + ["roles"] = roles }; if (!string.IsNullOrEmpty(users)) formData["users"] = users!; if (!string.IsNullOrEmpty(groups)) formData["groups"] = groups!; if (!string.IsNullOrEmpty(tokens)) formData["tokens"] = tokens!; + if (propagate.HasValue) formData["propagate"] = propagate.Value ? "1" : "0"; + if (delete.HasValue) formData["delete"] = delete.Value ? "1" : "0"; Invoke(session, client => { diff --git a/src/PSProxmoxVE/Cmdlets/Users/GetPveRoleCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Users/GetPveRoleCmdlet.cs index 0e2e956..1a0fb65 100644 --- a/src/PSProxmoxVE/Cmdlets/Users/GetPveRoleCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Users/GetPveRoleCmdlet.cs @@ -1,7 +1,6 @@ using System.Management.Automation; -using Newtonsoft.Json.Linq; -using PSProxmoxVE.Core.Client; using PSProxmoxVE.Core.Models.Users; +using PSProxmoxVE.Core.Services; namespace PSProxmoxVE.Cmdlets.Users { @@ -23,16 +22,13 @@ namespace PSProxmoxVE.Cmdlets.Users protected override void ProcessRecord() { var session = GetSession(); - using var client = new PveHttpClient(session); WriteVerbose("Getting roles..."); - var json = client.GetAsync("access/roles").GetAwaiter().GetResult(); - var root = JObject.Parse(json); - var data = root["data"] as JArray ?? new JArray(); + var service = new UserService(); + var roles = service.GetRoles(session); - foreach (var item in data) + foreach (var role in roles) { - var role = item.ToObject()!; if (!string.IsNullOrEmpty(RoleId) && !string.Equals(role.RoleId, RoleId, System.StringComparison.OrdinalIgnoreCase)) continue; diff --git a/src/PSProxmoxVE/Cmdlets/Users/GetPveUserCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Users/GetPveUserCmdlet.cs index 3998fd3..0fe34ba 100644 --- a/src/PSProxmoxVE/Cmdlets/Users/GetPveUserCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Users/GetPveUserCmdlet.cs @@ -1,7 +1,6 @@ using System.Management.Automation; -using Newtonsoft.Json.Linq; -using PSProxmoxVE.Core.Client; using PSProxmoxVE.Core.Models.Users; +using PSProxmoxVE.Core.Services; namespace PSProxmoxVE.Cmdlets.Users { @@ -31,16 +30,13 @@ namespace PSProxmoxVE.Cmdlets.Users protected override void ProcessRecord() { var session = GetSession(); - using var client = new PveHttpClient(session); WriteVerbose("Getting users..."); - var json = client.GetAsync("access/users").GetAwaiter().GetResult(); - var root = JObject.Parse(json); - var data = root["data"] as JArray ?? new JArray(); + var service = new UserService(); + var users = service.GetUsers(session); - foreach (var item in data) + foreach (var user in users) { - var user = item.ToObject()!; if (MatchesFilters(user)) WriteObject(user); } diff --git a/src/PSProxmoxVE/Cmdlets/Users/NewPveRoleCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Users/NewPveRoleCmdlet.cs index cd3ea1f..81bad71 100644 --- a/src/PSProxmoxVE/Cmdlets/Users/NewPveRoleCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Users/NewPveRoleCmdlet.cs @@ -1,7 +1,6 @@ -using System.Collections.Generic; using System.Management.Automation; -using PSProxmoxVE.Core.Client; using PSProxmoxVE.Core.Models.Users; +using PSProxmoxVE.Core.Services; namespace PSProxmoxVE.Cmdlets.Users { @@ -32,16 +31,10 @@ namespace PSProxmoxVE.Cmdlets.Users return; var session = GetSession(); - using var client = new PveHttpClient(session); WriteVerbose($"Creating role '{RoleId}'..."); - var data = new Dictionary - { - ["roleid"] = RoleId - }; - if (!string.IsNullOrEmpty(Privileges)) data["privs"] = Privileges!; - - client.PostAsync("access/roles", data).GetAwaiter().GetResult(); + var service = new UserService(); + service.CreateRole(session, RoleId, Privileges); WriteObject(new PveRole { RoleId = RoleId, Privileges = Privileges }); } diff --git a/src/PSProxmoxVE/Cmdlets/Users/SetPvePermissionCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Users/SetPvePermissionCmdlet.cs index 0d54564..df21ffe 100644 --- a/src/PSProxmoxVE/Cmdlets/Users/SetPvePermissionCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Users/SetPvePermissionCmdlet.cs @@ -1,6 +1,5 @@ -using System.Collections.Generic; using System.Management.Automation; -using PSProxmoxVE.Core.Client; +using PSProxmoxVE.Core.Services; namespace PSProxmoxVE.Cmdlets.Users { @@ -48,26 +47,26 @@ namespace PSProxmoxVE.Cmdlets.Users return; var session = GetSession(); - using var client = new PveHttpClient(session); WriteVerbose($"Setting permission for '{UgId}' at '{Path}'..."); - var data = new Dictionary - { - ["path"] = Path, - ["roles"] = Role - }; - + string? users = null, groups = null, tokens = null; if (string.Equals(Type, "group", System.StringComparison.OrdinalIgnoreCase)) - data["groups"] = UgId; + groups = UgId; else if (string.Equals(Type, "token", System.StringComparison.OrdinalIgnoreCase) || UgId.Contains("!")) - data["tokens"] = UgId; + tokens = UgId; else - data["users"] = UgId; + users = UgId; - if (Propagate.IsPresent) data["propagate"] = "1"; - if (Delete.IsPresent) data["delete"] = "1"; - - client.PutAsync("access/acl", data).GetAwaiter().GetResult(); + var service = new UserService(); + service.SetPermission( + session, + Path, + Role, + users: users, + groups: groups, + tokens: tokens, + propagate: Propagate.IsPresent ? true : (bool?)null, + delete: Delete.IsPresent ? true : (bool?)null); } } } diff --git a/tests/PSProxmoxVE.Core.Tests/Services/UserServiceTests.cs b/tests/PSProxmoxVE.Core.Tests/Services/UserServiceTests.cs index 9468806..b6d2ab6 100644 --- a/tests/PSProxmoxVE.Core.Tests/Services/UserServiceTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Services/UserServiceTests.cs @@ -358,6 +358,7 @@ namespace PSProxmoxVE.Core.Tests.Services Assert.Equal(1, result[0].Special); Assert.Equal("CustomOps", result[2].RoleId); Assert.Equal(0, result[2].Special); + Assert.Equal("VM.PowerMgmt,VM.Console", result[2].Privileges); } [Fact] @@ -873,7 +874,10 @@ namespace PSProxmoxVE.Core.Tests.Services d["roles"] == "PVEVMAdmin" && d["users"] == "deploy@pve" && d["propagate"] == "1" && - d["delete"] == "0")), + !d.ContainsKey("delete") && + !d.ContainsKey("groups") && + !d.ContainsKey("tokens") && + d.Count == 4)), Times.Once); } @@ -894,7 +898,70 @@ namespace PSProxmoxVE.Core.Tests.Services d["roles"] == "Administrator" && d["groups"] == "admins" && d["propagate"] == "0" && - d["delete"] == "1")), + d["delete"] == "1" && + !d.ContainsKey("users") && + !d.ContainsKey("tokens") && + d.Count == 5)), + Times.Once); + } + + [Fact] + public void SetPermission_WithToken_SendsTokensKey() + { + // Arrange + _mockClient.Setup(c => c.PutAsync("access/acl", It.IsAny>())) + .ReturnsAsync(@"{""data"":null}"); + + // Act + _service.SetPermission(_session, "/vms/100", "PVEVMAdmin", tokens: "deploy@pve!ci"); + + // Assert + _mockClient.Verify(c => c.PutAsync("access/acl", + It.Is>(d => + d["tokens"] == "deploy@pve!ci" && + !d.ContainsKey("users") && + !d.ContainsKey("groups") && + !d.ContainsKey("propagate") && + !d.ContainsKey("delete") && + d.Count == 3)), + Times.Once); + } + + [Fact] + public void SetPermission_DeleteWithoutPropagate_SendsDeleteOmitsPropagate() + { + // Arrange + _mockClient.Setup(c => c.PutAsync("access/acl", It.IsAny>())) + .ReturnsAsync(@"{""data"":null}"); + + // Act + _service.SetPermission(_session, "/vms/100", "PVEVMAdmin", users: "deploy@pve", delete: true); + + // Assert + _mockClient.Verify(c => c.PutAsync("access/acl", + It.Is>(d => + d["delete"] == "1" && + !d.ContainsKey("propagate") && + d.Count == 4)), + Times.Once); + } + + [Fact] + public void SetPermission_NoPropagateOrDelete_OmitsBothFlags() + { + // Arrange + _mockClient.Setup(c => c.PutAsync("access/acl", It.IsAny>())) + .ReturnsAsync(@"{""data"":null}"); + + // Act + _service.SetPermission(_session, "/vms/100", "PVEVMAdmin", users: "deploy@pve"); + + // Assert + _mockClient.Verify(c => c.PutAsync("access/acl", + It.Is>(d => + !d.ContainsKey("propagate") && + !d.ContainsKey("delete") && + d.Count == 3)), Times.Once); }