[REVIEW please] CreateGroups additions

[bevanweiss]

Just a placeholder for the current state of development on the ability to add Groups. wixtoolset/issues#8577

Not sure if it works yet, splitting out the tables cost me a little more time than I anticipated, and I'm unsure that the decompile is right.

I'll build out a few more of the build test cases, to validate the expected preconditions etc (i.e. marking CreateGroup when it's not under a component, and similar).

Please raise any style / formatting issues, that way I've got the most time to address them :)

I've just noticed I've missed a couple of Wix4-.Wix6 references (around CustomAction names). I'll tidy these up tomorrow.

Closes wixtoolset/issues#8577

[bevanweiss]

@barnson Are there any additional integration tests that you think would be good to have beyond what I've already got in this?
There's a small set that I haven't yet put in that would be around the domain functionality (similar to the new user one I've added on the other PR #547), I think that would be:

1. Creating/removing a domain group
2. Adding/removing a domain user from a domain group
3. Adding/removing a domain group from a domain group
4. Adding/removing a domain user from a local group
5. Adding/removing a domain group from a local group

I know there's a heap of code in this PR, however there's not really many easily separable portions that would be meaningful on their own. However, let me know if there's any way you can see that I could split it into more easily reviewable/pull-able portions.

[barnson]

I'm not a domain expert but the tests look reasonable to me.

[robmen]

One new test failed:

[xUnit.net 00:04:46.85]     WixToolsetTest.MsiE2E.UtilExtensionGroupTests.FailsIfRestrictedDomain [FAIL]
  Failed WixToolsetTest.MsiE2E.UtilExtensionGroupTests.FailsIfRestrictedDomain [16 s]
  Error Message:
   Assert.True() Failure
Expected: True
Actual:   False
  Stack Trace:
     at WixToolsetTest.MsiE2E.UtilExtensionGroupTests.FailsIfRestrictedDomain() in D:\a\wix\wix\src\test\msi\WixToolsetTest.MsiE2E\UtilExtensionGroupTests.cs:line 159

This is the test:

// Verify that a group cannot be created on a domain on which you don't have create user permission.
[RuntimeFact]
public void FailsIfRestrictedDomain()
{
    var productRestrictedDomain = this.CreatePackageInstaller("ProductRestrictedDomain");

    string logFile = productRestrictedDomain.InstallProduct(MSIExec.MSIExecReturnCode.ERROR_INSTALL_FAILURE, "TESTDOMAIN=DOESNOTEXIST");

    // Verify expected error message in the log file
    Assert.True(LogVerifier.MessageInLogFile(logFile, "CreateGroup:  Error 0x8007054b: failed to find Domain DOESNOTEXIST."));
}

[bevanweiss]

Thanks, it looks like it's the log error message that couldn't be found.
I'll have a look into it.

Any chance there's a way to retrieve the log file from the Github Action?

[robmen]

Any chance there's a way to retrieve the log file from the Github Action?

Not today. E2E Burn logs are captured, but MSI E2E logs are not. 😢

[bevanweiss]

Any chance there's a way to retrieve the log file from the Github Action?

Not today. E2E Burn logs are captured, but MSI E2E logs are not. 😢

No worries.
I did see what looked like that (Burn but not MSI) from the action.

And in your recent PR to update the runner actions it looked like the master repo was exempted from exporting those artefacts to prevent secret leakage also.

I just wanted to avoid a ‘works for me..’ situation.

[robmen]

The master branch handling is for official builds and is (will be) done differently in WiX v6. You can ignore all references to master in the conditions.

[bevanweiss]

Drats. This looks like an issue with the revised Domain->Server method.
I think the intent of this method has changed, my understanding of the original version was that given a Domain, it would identify an applicable Domain Server (Domain Controller, DC) that should be used to direct the domain requests to.
If an invalid Domain was provided to the original method, it would return an error, indicating this domain was not known, and no server could be found.
The revised version of this method appears to just return the domain name as the server, and then because the HRESULT error from the NetAPI call (to get the domain server) is overwritten with the StringCopy result, the method itself returns all happy indications.

I think this GetDomainFromServerName doesn't make sense as-is... since we are explicitely given the domain. It's the domain server that we need to identify (i.e. DC01.DOMAIN / PDC.DOMAIN etc).
And a side effect of this was the validation of the domain (i.e. if the domain doesn't exist, there can be no valid domain server for it, and hence the result value from the method should indicate this).
It was this PR that changed the method: #548

Unsure how I skipped this test on re-basing after that PR landed though...

[bevanweiss]

One new test failed:

Fixed. I went back to the earlier GetDomainServerName() method, since this provides a usable HRESULT if the domain doesn't actually exist.