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

Jump to bottom
Merged
danmoseley merged 3 commits into from
Feb 4, 2021
Merged

make TestUserCredentialsPropertiesOnWindows stable on Windows Server Core #47031

danmoseley merged 3 commits into from
Feb 4, 2021

Conversation

@adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Jan 15, 2021 •
edited
Loading

Fixes #46619

@adamsitnik
kill the process first, then remove the access and then remove the user
92b7452 
this is to try to make TestUserCredentialsPropertiesOnWindows stable on Windows Server Core
@adamsitnik adamsitnik added area-System.Diagnostics.Process test-bug Problem in test source code (most likely) labels Jan 15, 2021
@adamsitnik adamsitnik added this to the 6.0.0 milestone Jan 15, 2021
@ghost
Copy link

ghost commented Jan 15, 2021

Tagging subscribers to this area: @eiriktsarpalis
See info in area-owners.md if you want to be subscribed.

Issue Details
Author: adamsitnik
Assignees: -
Labels:

area-System.Diagnostics.Process, test bug
Milestone: 6.0.0

@adamsitnik
Copy link
Member Author

adamsitnik commented Jan 15, 2021

/azp run runtime-libraries-coreclr outerloop-windows

@azure-pipelines
Copy link

azure-pipelines bot commented Jan 15, 2021

Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik
Copy link
Member Author

adamsitnik commented Jan 15, 2021

/azp run runtime-libraries-coreclr outerloop-windows

@azure-pipelines
Copy link

azure-pipelines bot commented Jan 15, 2021

Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik
I was right, the first call to SetAccessControl was throwing
a2ca848 
What if we don't give the user rigts to execute it at all?
@adamsitnik
Copy link
Member Author

adamsitnik commented Jan 15, 2021

/azp run runtime-libraries-coreclr outerloop-windows

@azure-pipelines
Copy link

azure-pipelines bot commented Jan 15, 2021

Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik adamsitnik marked this pull request as ready for review January 15, 2021 12:23
@adamsitnik
Copy link
Member Author

adamsitnik commented Jan 15, 2021

@eiriktsarpalis it's now properly passing

stephentoub
stephentoub reviewed Jan 15, 2021
View reviewed changes
src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs

// 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?)

@adamsitnik adamsitnik mentioned this pull request Jan 18, 2021
Closed
danmoseley
danmoseley approved these changes Feb 4, 2021
View reviewed changes
Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

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

I would like to know the answer too, but merging this gets us into a better state.

@danmoseley
Copy link
Member

danmoseley commented Feb 4, 2021

Hit this again -- so I'm merging!

@danmoseley
Copy link
Member

danmoseley commented Feb 4, 2021

unrelated failures.

@danmoseley danmoseley merged commit 8241168 into dotnet:master Feb 4, 2021
@adamsitnik adamsitnik deleted the test46619 branch February 5, 2021 08:49
@ghost ghost locked as resolved and limited conversation to collaborators Mar 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoub stephentoub stephentoub left review comments

@danmoseley danmoseley danmoseley approved these changes

@eiriktsarpalis eiriktsarpalis Awaiting requested review from eiriktsarpalis

@jozkee jozkee Awaiting requested review from jozkee

@carlossanlop carlossanlop Awaiting requested review from carlossanlop

Assignees

No one assigned

Labels

area-System.Diagnostics.Process test-bug Problem in test source code (most likely)

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

Test failure: System.Diagnostics.Tests.ProcessStartInfoTests.TestUserCredentialsPropertiesOnWindows

3 participants

@adamsitnik @danmoseley @stephentoub