Skip to content

make TestUserCredentialsPropertiesOnWindows stable on Windows Server Core #47031

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
danmoseley merged 3 commits into from
Feb 4, 2021
Merged

make TestUserCredentialsPropertiesOnWindows stable on Windows Server Core #47031

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
92b7452
kill the process first, then remove the access and then remove the user
adamsitnik Jan 15, 2021
5727c5e
is UnauthorizedAccessException thrown from finally swallowing a diffe…
adamsitnik Jan 15, 2021
a2ca848
I was right, the first call to SetAccessControl was throwing
adamsitnik Jan 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,8 +472,11 @@ public void TestUserCredentialsPropertiesOnWindows()
{
p = CreateProcessLong();

// ensure the new user can access the .exe (otherwise you get Access is denied exception)
SetAccessControl(username, p.StartInfo.FileName, add: true);
if (PlatformDetection.IsNotWindowsServerCore) // for this particular Windows version it fails with Attempted to perform an unauthorized operation (#46619)
Copy link
Member

@stephentoub stephentoub Jan 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is still valid without this? If the test succeeds without this on Windows Server Core, why is it needed on other Windows?

Copy link
Member Author

@adamsitnik adamsitnik Jan 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added this recently when I was working on re-enabling this test and it was mandatory on my machine to make it work (#46083)

I have no idea why it does not work on machines from this certain pool. Is it a thing related to Windows Server? Some custom helix machine setup?

The test is restricted to admin accounts only:

runtime/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs

Line 454 in f6e73ad

[ConditionalFact(nameof(IsAdmin_IsNotNano_RemoteExecutorIsSupported))] // Nano has no "netapi32.dll", Admin rights are required

runtime/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs

Lines 28 to 29 in f6e73ad

private static bool IsAdmin_IsNotNano_RemoteExecutorIsSupported
=> PlatformDetection.IsWindowsAndElevated && PlatformDetection.IsNotWindowsNanoServer && RemoteExecutor.IsSupported;

Which makes it even more confusing (I would assume that Admin accounts which can create other user accounts can also add permissions to files?)

{
// ensure the new user can access the .exe (otherwise you get Access is denied exception)
SetAccessControl(username, p.StartInfo.FileName, add: true);
}

p.StartInfo.LoadUserProfile = true;
p.StartInfo.UserName = username;
Expand All @@ -500,10 +503,6 @@ public void TestUserCredentialsPropertiesOnWindows()
}
finally
{
SetAccessControl(username, p.StartInfo.FileName, add: false); // remove the access

Assert.Equal(Interop.ExitCodes.NERR_Success, Interop.NetUserDel(null, username));

if (handle != null)
handle.Dispose();

Expand All @@ -513,6 +512,13 @@ public void TestUserCredentialsPropertiesOnWindows()

Assert.True(p.WaitForExit(WaitInMS));
}

if (PlatformDetection.IsNotWindowsServerCore)
{
SetAccessControl(username, p.StartInfo.FileName, add: false); // remove the access
}

Assert.Equal(Interop.ExitCodes.NERR_Success, Interop.NetUserDel(null, username));
}
}

Expand Down