Group creation and update PR nits and feedback fixes

robmen · robmen · commit d20becc0ef9a · 2024-12-24T18:55:13.000-08:00
diff --git 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<LPBYTE>(&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
@@ -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
@@ -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
         /// <param name="element">Element to parse.</param>
         /// <param name="childId">Required child id to be joined to the group.</param>
         /// <param name="groupType">whether the child is a group (true) or a user (false)</param>
-        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
@@ -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
@@ -29,7 +29,7 @@ public static void CreateLocalGroup(string groupName)
         }
 
         /// <summary>
-        /// Deletes a local gorup from the machine
+        /// Deletes a local group from the machine
         /// </summary>
         /// <param name="groupName">group name to delete</param>
         /// <remarks>Has to be run as an Admin</remarks>

Original file line number	Diff line number	Diff line change
`@@ -720,8 +720,7 @@ static HRESULT RemoveGroupInternal(`
`720`	`720`
`721`	`721`	`NET_API_STATUS er = ::NetLocalGroupDel(pwzServerName, wzName);`
`722`	`722`	`hr = HRESULT_FROM_WIN32(er);`
`723`		`- if (HRESULT_FROM_WIN32(ERROR_NO_SUCH_ALIAS) == hr`
`724`		`- \|\| HRESULT_FROM_WIN32(NERR_GroupNotFound) == hr) // we wanted to delete it.. and the group doesn't exist.. solved.`
	`723`	`+ 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.`
`725`	`724`	`{`
`726`	`725`	`hr = S_OK;`
`727`	`726`	`}`
`@@ -1303,16 +1302,16 @@ extern "C" UINT __stdcall CreateGroup(`
`1303`	`1302`	`er = ::NetLocalGroupAdd(pwzServerName, 1, reinterpret_cast<LPBYTE>(&groupInfo1), &dw);`
`1304`	`1303`	`hr = HRESULT_FROM_WIN32(er);`
`1305`	`1304`
`1306`		`- if (HRESULT_FROM_WIN32(ERROR_ALIAS_EXISTS) == hr`
`1307`		`- \|\| HRESULT_FROM_WIN32(NERR_GroupExists) == hr)`
	`1305`	`+ if (HRESULT_FROM_WIN32(ERROR_ALIAS_EXISTS) == hr \|\| HRESULT_FROM_WIN32(NERR_GroupExists) == hr)`
`1308`	`1306`	`{`
`1309`	`1307`	`if (SCAG_FAIL_IF_EXISTS & iAttributes)`
`1310`	`1308`	`{`
`1311`	`1309`	`MessageExitOnFailure(hr, msierrGRPFailedGroupCreateExists, "Group (%ls\\%ls) was not supposed to exist, but does", pwzDomain, pwzName);`
`1312`	`1310`	`}`
`1313`	`1311`
`1314`		`- hr = S_OK; // Make sure that we don't report this situation as an error`
	`1312`	`+ // Make sure that we don't report this situation as an error`
`1315`	`1313`	`// if we fall through the tests that follow.`
	`1314`	`+ hr = S_OK;`
`1316`	`1315`
`1317`	`1316`	`if (SCAG_UPDATE_IF_EXISTS & iAttributes)`
`1318`	`1317`	`{`
`@@ -1505,6 +1504,7 @@ extern "C" UINT __stdcall CreateGroupRollback(`
`1505`	`1504`	`{`
`1506`	`1505`	`pwzComment = pwzOriginalComment;`
`1507`	`1506`	`}`
	`1507`	`+`
`1508`	`1508`	`hr = WcaReadIntegerFromCaData(&pwz, &iOriginalAttributes);`
`1509`	`1509`	`if (FAILED(hr))`
`1510`	`1510`	`{`
`@@ -1618,7 +1618,7 @@ extern "C" UINT __stdcall RemoveGroup(`
`1618`	`1618`	`return WcaFinalize(er);`
`1619`	`1619`	`}`
`1620`	`1620`
`1621`		`-HRESULT AlterGroupMembership(bool remove, bool isRollback)`
	`1621`	`+HRESULT AlterGroupMembership(BOOL fRemove, BOOL fIsRollback)`
`1622`	`1622`	`{`
`1623`	`1623`	`HRESULT hr = S_OK;`
`1624`	`1624`	`NET_API_STATUS er = ERROR_SUCCESS;`
`@@ -1663,7 +1663,7 @@ HRESULT AlterGroupMembership(bool remove, bool isRollback)`
`1663`	`1663`	`hr = WcaReadStringFromCaData(&pwz, &pwzScriptKey);`
`1664`	`1664`	`ExitOnFailure(hr, "failed to read scriptkey from custom action data");`
`1665`	`1665`
`1666`		`- if (isRollback)`
	`1666`	`+ if (fIsRollback)`
`1667`	`1667`	`{`
`1668`	`1668`	`// if the script file doesn't exist, then we'll abandon this rollback`
`1669`	`1669`	`hr = WcaCaScriptOpen(WCA_ACTION_INSTALL, WCA_CASCRIPT_ROLLBACK, FALSE, pwzScriptKey, &hRollbackScript);`
`@@ -1693,7 +1693,7 @@ HRESULT AlterGroupMembership(bool remove, bool isRollback)`
`1693`	`1693`	`}`
`1694`	`1694`	`memberInfo3.lgrmi3_domainandname = pwzChildFullName;`
`1695`	`1695`
`1696`		`- if (remove)`
	`1696`	`+ if (fRemove)`
`1697`	`1697`	`{`
`1698`	`1698`	`er = ::NetLocalGroupDelMembers(pwzServerName, pwzParentName, 3, (LPBYTE)&memberInfo3, 1);`
`1699`	`1699`	`}`
`@@ -1705,18 +1705,16 @@ HRESULT AlterGroupMembership(bool remove, bool isRollback)`
`1705`	`1705`
`1706`	`1706`	`// if there was no error, the action succeeded, and we should flag that it's something which might need`
`1707`	`1707`	`// to be rolled back`
`1708`		`- if (S_OK == hr && !isRollback)`
	`1708`	`+ if (S_OK == hr && !fIsRollback)`
`1709`	`1709`	`{`
`1710`	`1710`	`// we create a script file, the rollback matching this scriptkey will occur if the file exists`
`1711`	`1711`	`hr = WcaCaScriptCreate(WCA_ACTION_INSTALL, WCA_CASCRIPT_ROLLBACK, FALSE, pwzScriptKey, FALSE, &hRollbackScript);`
`1712`	`1712`	`WcaCaScriptClose(hRollbackScript, WCA_CASCRIPT_CLOSE_PRESERVE);`
`1713`	`1713`	`}`
`1714`	`1714`
`1715`		`- if (remove)`
	`1715`	`+ if (fRemove)`
`1716`	`1716`	`{`
`1717`		`- if (HRESULT_FROM_WIN32(NERR_GroupNotFound) == hr`
`1718`		`- \|\| HRESULT_FROM_WIN32(ERROR_NO_SUCH_MEMBER) == hr`
`1719`		`- \|\| HRESULT_FROM_WIN32(ERROR_MEMBER_NOT_IN_ALIAS) == hr)`
	`1717`	`+ if (HRESULT_FROM_WIN32(NERR_GroupNotFound) == hr \|\| HRESULT_FROM_WIN32(ERROR_NO_SUCH_MEMBER) == hr \|\| HRESULT_FROM_WIN32(ERROR_MEMBER_NOT_IN_ALIAS) == hr)`
`1720`	`1718`	`{`
`1721`	`1719`	`hr = S_OK;`
`1722`	`1720`	`}`
`@@ -1771,7 +1769,7 @@ extern "C" UINT __stdcall AddGroupMembership(`
`1771`	`1769`	`ExitOnFailure(hr, "failed to initialize COM");`
`1772`	`1770`	`fInitializedCom = TRUE;`
`1773`	`1771`
`1774`		`- hr = AlterGroupMembership(false, false);`
	`1772`	`+ hr = AlterGroupMembership(FALSE, FALSE);`
`1775`	`1773`
`1776`	`1774`	`LExit:`
`1777`	`1775`	`if (fInitializedCom)`
`@@ -1807,7 +1805,7 @@ extern "C" UINT __stdcall AddGroupMembershipRollback(`
`1807`	`1805`	`ExitOnFailure(hr, "failed to initialize COM");`
`1808`	`1806`	`fInitializedCom = TRUE;`
`1809`	`1807`
`1810`		`- hr = AlterGroupMembership(true, true);`
	`1808`	`+ hr = AlterGroupMembership(TRUE, TRUE);`
`1811`	`1809`
`1812`	`1810`	`LExit:`
`1813`	`1811`	`if (fInitializedCom)`
`@@ -1842,13 +1840,14 @@ extern "C" UINT __stdcall RemoveGroupMembership(`
`1842`	`1840`	`ExitOnFailure(hr, "failed to initialize COM");`
`1843`	`1841`	`fInitializedCom = TRUE;`
`1844`	`1842`
`1845`		`- hr = AlterGroupMembership(true, false);`
	`1843`	`+ hr = AlterGroupMembership(TRUE, FALSE);`
`1846`	`1844`
`1847`	`1845`	`LExit:`
`1848`	`1846`	`if (fInitializedCom)`
`1849`	`1847`	`{`
`1850`	`1848`	`::CoUninitialize();`
`1851`	`1849`	`}`
	`1850`	`+`
`1852`	`1851`	`return WcaFinalize(FAILED(hr) ? ERROR_INSTALL_FAILED : ERROR_SUCCESS);`
`1853`	`1852`	`}`
`1854`	`1853`
`@@ -1878,7 +1877,7 @@ extern "C" UINT __stdcall RemoveGroupMembershipRollback(`
`1878`	`1877`	`ExitOnFailure(hr, "failed to initialize COM");`
`1879`	`1878`	`fInitializedCom = TRUE;`
`1880`	`1879`
`1881`		`- hr = AlterGroupMembership(false, true);`
	`1880`	`+ hr = AlterGroupMembership(FALSE, TRUE);`
`1882`	`1881`
`1883`	`1882`	`LExit:`
`1884`	`1883`	`if (fInitializedCom)`
Original file line number	Diff line number	Diff line change
`@@ -134,7 +134,7 @@ extern "C" UINT __stdcall ConfigureGroups(`
`134`	`134`	`__in MSIHANDLE hInstall`
`135`	`135`	`)`
`136`	`136`	`{`
`137`		`- AssertSz(0, "Debug ConfigureGroups");`
	`137`	`+ //AssertSz(0, "Debug ConfigureGroups");`
`138`	`138`
`139`	`139`	`HRESULT hr = S_OK;`
`140`	`140`	`UINT er = ERROR_SUCCESS;`
Original file line number	Diff line number	Diff line change
`@@ -9,6 +9,8 @@ namespace WixTestTools`
`9`	`9`
`10`	`10`	`public class RuntimeFactAttribute : SkippableFactAttribute`
`11`	`11`	`{`
	`12`	`+ private bool domainRequired;`
	`13`	`+`
`12`	`14`	`const string RequiredEnvironmentVariableName = "RuntimeTestsEnabled";`
`13`	`15`	`const string RequiredDomainEnvironmentVariableName = "RuntimeDomainTestsEnabled";`
`14`	`16`
`@@ -38,17 +40,16 @@ static RuntimeFactAttribute()`
`38`	`40`	`RuntimeDomainTestsEnabled = Boolean.TryParse(domainTestsEnabledString, out var domainTestsEnabled) && domainTestsEnabled;`
`39`	`41`	`}`
`40`	`42`
`41`		`- private bool _domainRequired;`
`42`	`43`	`public bool DomainRequired`
`43`	`44`	`{`
`44`	`45`	`get`
`45`	`46`	`{`
`46`		`- return _domainRequired;`
	`47`	`+ return this.domainRequired;`
`47`	`48`	`}`
`48`	`49`	`set`
`49`	`50`	`{`
`50`		`- _domainRequired = value;`
`51`		`- if (_domainRequired && String.IsNullOrEmpty(this.Skip) && (!RunningInDomain \|\| !RuntimeDomainTestsEnabled))`
	`51`	`+ this.domainRequired = value;`
	`52`	`+ if (this.domainRequired && String.IsNullOrEmpty(this.Skip) && (!RunningInDomain \|\| !RuntimeDomainTestsEnabled))`
`52`	`53`	`{`
`53`	`54`	`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")}).";`
`54`	`55`	`}`
Original file line number	Diff line number	Diff line change
`@@ -29,7 +29,7 @@ public static void CreateLocalGroup(string groupName)`
`29`	`29`	`}`
`30`	`30`
`31`	`31`	`/// <summary>`
`32`		`- /// Deletes a local gorup from the machine`
	`32`	`+ /// Deletes a local group from the machine`
`33`	`33`	`/// </summary>`
`34`	`34`	`/// <param name="groupName">group name to delete</param>`
`35`	`35`	`/// <remarks>Has to be run as an Admin</remarks>`