From d20becc0ef9ae20cfa495b64c743d4358c0d5b35 Mon Sep 17 00:00:00 2001 From: Rob Mensching Date: Tue, 24 Dec 2024 18:11:55 -0800 Subject: [PATCH] Group creation and update PR nits and feedback fixes --- src/ext/Util/ca/scaexec.cpp | 33 +++++++++---------- src/ext/Util/ca/scasched.cpp | 2 +- src/ext/Util/wixext/UtilCompiler.cs | 6 ++-- .../burn/WixTestTools/RuntimeFactAttribute.cs | 9 ++--- .../burn/WixTestTools/UserGroupVerifier.cs | 2 +- 5 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/ext/Util/ca/scaexec.cpp b/src/ext/Util/ca/scaexec.cpp index fea60b010..a2ecdaab8 100644 --- a/src/ext/Util/ca/scaexec.cpp +++ b/src/ext/Util/ca/scaexec.cpp @@ -720,8 +720,7 @@ static HRESULT RemoveGroupInternal( NET_API_STATUS er = ::NetLocalGroupDel(pwzServerName, wzName); hr = HRESULT_FROM_WIN32(er); - if (HRESULT_FROM_WIN32(ERROR_NO_SUCH_ALIAS) == hr - || HRESULT_FROM_WIN32(NERR_GroupNotFound) == hr) // we wanted to delete it.. and the group doesn't exist.. solved. + if (HRESULT_FROM_WIN32(ERROR_NO_SUCH_ALIAS) == hr || HRESULT_FROM_WIN32(NERR_GroupNotFound) == hr) // we wanted to delete it.. and the group doesn't exist.. solved. { hr = S_OK; } @@ -1303,16 +1302,16 @@ extern "C" UINT __stdcall CreateGroup( er = ::NetLocalGroupAdd(pwzServerName, 1, reinterpret_cast(&groupInfo1), &dw); hr = HRESULT_FROM_WIN32(er); - if (HRESULT_FROM_WIN32(ERROR_ALIAS_EXISTS) == hr - || HRESULT_FROM_WIN32(NERR_GroupExists) == hr) + if (HRESULT_FROM_WIN32(ERROR_ALIAS_EXISTS) == hr || HRESULT_FROM_WIN32(NERR_GroupExists) == hr) { if (SCAG_FAIL_IF_EXISTS & iAttributes) { MessageExitOnFailure(hr, msierrGRPFailedGroupCreateExists, "Group (%ls\\%ls) was not supposed to exist, but does", pwzDomain, pwzName); } - hr = S_OK; // Make sure that we don't report this situation as an error + // Make sure that we don't report this situation as an error // if we fall through the tests that follow. + hr = S_OK; if (SCAG_UPDATE_IF_EXISTS & iAttributes) { @@ -1505,6 +1504,7 @@ extern "C" UINT __stdcall CreateGroupRollback( { pwzComment = pwzOriginalComment; } + hr = WcaReadIntegerFromCaData(&pwz, &iOriginalAttributes); if (FAILED(hr)) { @@ -1618,7 +1618,7 @@ extern "C" UINT __stdcall RemoveGroup( return WcaFinalize(er); } -HRESULT AlterGroupMembership(bool remove, bool isRollback) +HRESULT AlterGroupMembership(BOOL fRemove, BOOL fIsRollback) { HRESULT hr = S_OK; NET_API_STATUS er = ERROR_SUCCESS; @@ -1663,7 +1663,7 @@ HRESULT AlterGroupMembership(bool remove, bool isRollback) hr = WcaReadStringFromCaData(&pwz, &pwzScriptKey); ExitOnFailure(hr, "failed to read scriptkey from custom action data"); - if (isRollback) + if (fIsRollback) { // if the script file doesn't exist, then we'll abandon this rollback hr = WcaCaScriptOpen(WCA_ACTION_INSTALL, WCA_CASCRIPT_ROLLBACK, FALSE, pwzScriptKey, &hRollbackScript); @@ -1693,7 +1693,7 @@ HRESULT AlterGroupMembership(bool remove, bool isRollback) } memberInfo3.lgrmi3_domainandname = pwzChildFullName; - if (remove) + if (fRemove) { er = ::NetLocalGroupDelMembers(pwzServerName, pwzParentName, 3, (LPBYTE)&memberInfo3, 1); } @@ -1705,18 +1705,16 @@ HRESULT AlterGroupMembership(bool remove, bool isRollback) // if there was no error, the action succeeded, and we should flag that it's something which might need // to be rolled back - if (S_OK == hr && !isRollback) + if (S_OK == hr && !fIsRollback) { // we create a script file, the rollback matching this scriptkey will occur if the file exists hr = WcaCaScriptCreate(WCA_ACTION_INSTALL, WCA_CASCRIPT_ROLLBACK, FALSE, pwzScriptKey, FALSE, &hRollbackScript); WcaCaScriptClose(hRollbackScript, WCA_CASCRIPT_CLOSE_PRESERVE); } - if (remove) + if (fRemove) { - if (HRESULT_FROM_WIN32(NERR_GroupNotFound) == hr - || HRESULT_FROM_WIN32(ERROR_NO_SUCH_MEMBER) == hr - || HRESULT_FROM_WIN32(ERROR_MEMBER_NOT_IN_ALIAS) == hr) + if (HRESULT_FROM_WIN32(NERR_GroupNotFound) == hr || HRESULT_FROM_WIN32(ERROR_NO_SUCH_MEMBER) == hr || HRESULT_FROM_WIN32(ERROR_MEMBER_NOT_IN_ALIAS) == hr) { hr = S_OK; } @@ -1771,7 +1769,7 @@ extern "C" UINT __stdcall AddGroupMembership( ExitOnFailure(hr, "failed to initialize COM"); fInitializedCom = TRUE; - hr = AlterGroupMembership(false, false); + hr = AlterGroupMembership(FALSE, FALSE); LExit: if (fInitializedCom) @@ -1807,7 +1805,7 @@ extern "C" UINT __stdcall AddGroupMembershipRollback( ExitOnFailure(hr, "failed to initialize COM"); fInitializedCom = TRUE; - hr = AlterGroupMembership(true, true); + hr = AlterGroupMembership(TRUE, TRUE); LExit: if (fInitializedCom) @@ -1842,13 +1840,14 @@ extern "C" UINT __stdcall RemoveGroupMembership( ExitOnFailure(hr, "failed to initialize COM"); fInitializedCom = TRUE; - hr = AlterGroupMembership(true, false); + hr = AlterGroupMembership(TRUE, FALSE); LExit: if (fInitializedCom) { ::CoUninitialize(); } + return WcaFinalize(FAILED(hr) ? ERROR_INSTALL_FAILED : ERROR_SUCCESS); } @@ -1878,7 +1877,7 @@ extern "C" UINT __stdcall RemoveGroupMembershipRollback( ExitOnFailure(hr, "failed to initialize COM"); fInitializedCom = TRUE; - hr = AlterGroupMembership(false, true); + hr = AlterGroupMembership(FALSE, TRUE); LExit: if (fInitializedCom) diff --git a/src/ext/Util/ca/scasched.cpp b/src/ext/Util/ca/scasched.cpp index d7378b13b..cef7fecba 100644 --- a/src/ext/Util/ca/scasched.cpp +++ b/src/ext/Util/ca/scasched.cpp @@ -134,7 +134,7 @@ extern "C" UINT __stdcall ConfigureGroups( __in MSIHANDLE hInstall ) { - AssertSz(0, "Debug ConfigureGroups"); + //AssertSz(0, "Debug ConfigureGroups"); HRESULT hr = S_OK; UINT er = ERROR_SUCCESS; diff --git a/src/ext/Util/wixext/UtilCompiler.cs b/src/ext/Util/wixext/UtilCompiler.cs index 4b1e43b52..f59ffcd61 100644 --- a/src/ext/Util/wixext/UtilCompiler.cs +++ b/src/ext/Util/wixext/UtilCompiler.cs @@ -1485,7 +1485,7 @@ private void ParseGroupElement(Intermediate intermediate, IntermediateSection se switch (child.Name.LocalName) { case "GroupRef": - this.ParseGroupRefElement(intermediate, section, child, id.Id, groupType:true); + this.ParseGroupRefElement(intermediate, section, child, id.Id, groupType: true); break; default: this.ParseHelper.UnexpectedElement(element, child); @@ -1521,7 +1521,7 @@ private void ParseGroupElement(Intermediate intermediate, IntermediateSection se /// Element to parse. /// Required child id to be joined to the group. /// whether the child is a group (true) or a user (false) - private void ParseGroupRefElement(Intermediate intermediate, IntermediateSection section, XElement element, string childId, bool groupType=false) + private void ParseGroupRefElement(Intermediate intermediate, IntermediateSection section, XElement element, string childId, bool groupType) { var sourceLineNumbers = this.ParseHelper.GetSourceLineNumbers(element); string groupId = null; @@ -3588,7 +3588,7 @@ private void ParseUserElement(Intermediate intermediate, IntermediateSection sec this.Messaging.Write(UtilErrors.IllegalElementWithoutComponent(childSourceLineNumbers, child.Name.LocalName)); } - this.ParseGroupRefElement(intermediate, section, child, id.Id, groupType:false); + this.ParseGroupRefElement(intermediate, section, child, id.Id, groupType: false); break; default: this.ParseHelper.UnexpectedElement(element, child); diff --git a/src/test/burn/WixTestTools/RuntimeFactAttribute.cs b/src/test/burn/WixTestTools/RuntimeFactAttribute.cs index 02cec9e44..e21617e70 100644 --- a/src/test/burn/WixTestTools/RuntimeFactAttribute.cs +++ b/src/test/burn/WixTestTools/RuntimeFactAttribute.cs @@ -9,6 +9,8 @@ namespace WixTestTools public class RuntimeFactAttribute : SkippableFactAttribute { + private bool domainRequired; + const string RequiredEnvironmentVariableName = "RuntimeTestsEnabled"; const string RequiredDomainEnvironmentVariableName = "RuntimeDomainTestsEnabled"; @@ -38,17 +40,16 @@ static RuntimeFactAttribute() RuntimeDomainTestsEnabled = Boolean.TryParse(domainTestsEnabledString, out var domainTestsEnabled) && domainTestsEnabled; } - private bool _domainRequired; public bool DomainRequired { get { - return _domainRequired; + return this.domainRequired; } set { - _domainRequired = value; - if (_domainRequired && String.IsNullOrEmpty(this.Skip) && (!RunningInDomain || !RuntimeDomainTestsEnabled)) + this.domainRequired = value; + if (this.domainRequired && String.IsNullOrEmpty(this.Skip) && (!RunningInDomain || !RuntimeDomainTestsEnabled)) { this.Skip = $"These tests require the test host to be running as a domain member ({(RunningInDomain ? "passed" : "failed")}). These tests affect both MACHINE AND DOMAIN state. To accept the consequences, set the {RequiredDomainEnvironmentVariableName} environment variable to true ({(RuntimeDomainTestsEnabled ? "passed" : "failed")})."; } diff --git a/src/test/burn/WixTestTools/UserGroupVerifier.cs b/src/test/burn/WixTestTools/UserGroupVerifier.cs index 52a1a6bfb..8c2da46ef 100644 --- a/src/test/burn/WixTestTools/UserGroupVerifier.cs +++ b/src/test/burn/WixTestTools/UserGroupVerifier.cs @@ -29,7 +29,7 @@ public static void CreateLocalGroup(string groupName) } /// - /// Deletes a local gorup from the machine + /// Deletes a local group from the machine /// /// group name to delete /// Has to be run as an Admin