Add asynchronous overload of WindowsIdentity.RunImpersonated #1152

MarcoRossignoli · 2019-12-25T10:30:47Z

closes https://github.com/dotnet/corefx/issues/24977

On issue there is #nullable enable on review comment, but I don't know what it mean, this namespaces seem not annotated yet.

At the moment there is not "real" test on "another" user token, I've added some plumbing to test with new win user, took idea from https://github.com/dotnet/runtime/blob/4f9ae42d861fcb4be2fcd5d3d55d5f227d30e723/src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.Windows.cs maybe we could at the end merge helper inside Common.
Let me know if it's too much and I'll remove all and will add only simple "same access token current user" test.
Folded two api test in one to reuse assertions and also because the test are pretty the same.

cc: @stephentoub @bartonjs @GrabYourPitchforks

Merry Christmas to all team!

MarcoRossignoli · 2019-12-25T11:13:07Z

@ViktorHofer @safern is this expected?Am I missing something?Maybe worflow is changed a bit
I want to run netfx test on my local
Ran

D:\git\runtime (asyncrunimpersonated -> origin)
λ libraries.cmd -framework netfx

And after

D:\git\runtime\src\libraries\System.Security.Principal.Windows\tests (asyncrunimpersonated -> origin)
λ msbuild /t:rebuildandtest /p:TargetGroup=netfx /v:m
Microsoft (R) Build Engine version 16.5.0-preview-19562-03+d72e25031 for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.

  CoreFx.Private.TestUtilities -> D:\git\runtime\artifacts\bin\CoreFx.Private.TestUtilities\netfx-Debug\CoreFx.Private.TestUtilities.dll
D:\git\runtime\eng\testing\xunit\xunit.console.targets(67,86): error MSB4184: The expression "[System.IO.Path]::GetDirectoryName('')" cannot be evaluated. The path is not of a legal form. [D:\git\runtime\src\
libraries\System.Security.Principal.Windows\tests\System.Security.Principal.Windows.Tests.csproj]

MarcoRossignoli · 2019-12-25T15:51:54Z

/azp list

azure-pipelines · 2019-12-25T15:51:59Z

Commenter does not have sufficient privileges for PR 1152 in repo dotnet/runtime

...aries/System.Security.Principal.Windows/tests/WindowsIdentityImpersonatedTests.netcoreapp.cs

src/libraries/System.Security.Principal.Windows/ref/System.Security.Principal.Windows.cs

safern · 2020-01-06T17:58:16Z

@ViktorHofer @safern is this expected?Am I missing something?Maybe worflow is changed a bit
I want to run netfx test on my local

I believe this is a bug introduced when moving the corefx testing targets from arcade into the consolidated repo.

I believe we should condition this line to only execute when TargetsNetcoreApp == true.

...aries/System.Security.Principal.Windows/tests/WindowsIdentityImpersonatedTests.netcoreapp.cs

...libraries/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs

safern · 2020-01-06T18:34:31Z

I believe we should condition this line to only execute when TargetsNetcoreApp == true.

Actually it seems like the XunitConsoleNetCore21AppPath is wrong and it is not defined, that's why you're getting an empty path. Those properties seem to be defined by the xunit nuget package, and it seems like the property name is: XunitConsoleNetCore2AppPath

<PropertyGroup>
    <!-- Lowest supported version -->
    <XunitConsolePath>$(MSBuildThisFileDirectory)..\tools\net452\xunit.console.exe</XunitConsolePath>
    <XunitConsolePathX86>$(MSBuildThisFileDirectory)..\tools\net452\xunit.console.x86.exe</XunitConsolePathX86>

    <!-- Version specific -->
    <XunitConsole452Path>$(MSBuildThisFileDirectory)..\tools\net452\xunit.console.exe</XunitConsole452Path>
    <XunitConsole452PathX86>$(MSBuildThisFileDirectory)..\tools\net452\xunit.console.x86.exe</XunitConsole452PathX86>
    <XunitConsole46Path>$(MSBuildThisFileDirectory)..\tools\net46\xunit.console.exe</XunitConsole46Path>
    <XunitConsole46PathX86>$(MSBuildThisFileDirectory)..\tools\net46\xunit.console.x86.exe</XunitConsole46PathX86>
    <XunitConsole461Path>$(MSBuildThisFileDirectory)..\tools\net461\xunit.console.exe</XunitConsole461Path>
    <XunitConsole461PathX86>$(MSBuildThisFileDirectory)..\tools\net461\xunit.console.x86.exe</XunitConsole461PathX86>
    <XunitConsole462Path>$(MSBuildThisFileDirectory)..\tools\net462\xunit.console.exe</XunitConsole462Path>
    <XunitConsole462PathX86>$(MSBuildThisFileDirectory)..\tools\net462\xunit.console.x86.exe</XunitConsole462PathX86>
    <XunitConsole47Path>$(MSBuildThisFileDirectory)..\tools\net47\xunit.console.exe</XunitConsole47Path>
    <XunitConsole47PathX86>$(MSBuildThisFileDirectory)..\tools\net47\xunit.console.x86.exe</XunitConsole47PathX86>
    <XunitConsole471Path>$(MSBuildThisFileDirectory)..\tools\net471\xunit.console.exe</XunitConsole471Path>
    <XunitConsole471PathX86>$(MSBuildThisFileDirectory)..\tools\net471\xunit.console.x86.exe</XunitConsole471PathX86>
    <XunitConsole472Path>$(MSBuildThisFileDirectory)..\tools\net472\xunit.console.exe</XunitConsole472Path>
    <XunitConsole472PathX86>$(MSBuildThisFileDirectory)..\tools\net472\xunit.console.x86.exe</XunitConsole472PathX86>

    <!-- Lowest supported version -->
    <XunitConsoleNetCoreAppPath>$(MSBuildThisFileDirectory)..\tools\netcoreapp1.0\xunit.console.dll</XunitConsoleNetCoreAppPath>

    <!-- Version specific -->
    <XunitConsoleNetCore1AppPath>$(MSBuildThisFileDirectory)..\tools\netcoreapp1.0\xunit.console.dll</XunitConsoleNetCore1AppPath>
    <XunitConsoleNetCore2AppPath>$(MSBuildThisFileDirectory)..\tools\netcoreapp2.0\xunit.console.dll</XunitConsoleNetCore2AppPath>
  </PropertyGroup>

MarcoRossignoli · 2020-01-06T19:23:13Z

/azp list

azure-pipelines · 2020-01-06T19:23:18Z

Commenter does not have sufficient privileges for PR 1152 in repo dotnet/runtime

...libraries/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs

...aries/System.Security.Principal.Windows/tests/WindowsIdentityImpersonatedTests.netcoreapp.cs

ViktorHofer · 2020-01-14T17:25:27Z

Actually it seems like the XunitConsoleNetCore21AppPath is wrong and it is not defined, that's why you're getting an empty path. Those properties seem to be defined by the xunit nuget package, and it seems like the property name is: XunitConsoleNetCore2AppPath

@safern this is a custom property defined in our own xunit console runner: https://github.com/dotnet/arcade/blob/master/src/Microsoft.DotNet.XUnitConsoleRunner/src/build/Microsoft.DotNet.XUnitConsoleRunner.props#L4.

I assume this isn't working because netcoreapp wasn't restored correctly or the design time imports failed for some reason. To guard against that we should add '$(XunitConsoleNetCore21AppPath)' != '' in eng\testing\xunit\xunit.console.targets:L67.

ViktorHofer · 2020-01-14T17:29:35Z

Submitted #1715 to guard against such case.

update comment

Fix comment

fix comment

...libraries/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs

stephentoub

Thanks!

stephentoub · 2020-01-22T22:06:28Z

...libraries/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs

            RunImpersonatedInternal(safeAccessTokenHandle, () => result = func());
            return result;
        }

+        /// <summary>
+        /// Runs the specified asynchronous action as the impersonated Windows identity


Suggested change

/// Runs the specified asynchronous action as the impersonated Windows identity

/// Runs the specified asynchronous action as the impersonated Windows identity.

stephentoub · 2020-01-22T22:07:03Z

...libraries/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs

+            => RunImpersonated(safeAccessTokenHandle, func);
+
+        /// <summary>
+        /// Runs the specified asynchronous action as the impersonated Windows identity


Suggested change

/// Runs the specified asynchronous action as the impersonated Windows identity

/// Runs the specified asynchronous action as the impersonated Windows identity.

add RunImpersonateAsync overloads

181b8d9

Dotnet-GitSync-Bot added the area-System.Security label Dec 25, 2019

skip on full framework

fa80ea5

MarcoRossignoli changed the title ~~Add asynchronous overload of WindowsIdentity.RunImpersonated~~ [WIP]Add asynchronous overload of WindowsIdentity.RunImpersonated Dec 25, 2019

run test netcoreapp only

7e9cf08

MarcoRossignoli changed the title ~~[WIP]Add asynchronous overload of WindowsIdentity.RunImpersonated~~ Add asynchronous overload of WindowsIdentity.RunImpersonated Dec 25, 2019

skip tests on nanoserver

584e2f4

danmoseley reviewed Dec 26, 2019

View reviewed changes

...aries/System.Security.Principal.Windows/tests/WindowsIdentityImpersonatedTests.netcoreapp.cs Outdated Show resolved Hide resolved

danmoseley reviewed Dec 26, 2019

View reviewed changes

...aries/System.Security.Principal.Windows/tests/WindowsIdentityImpersonatedTests.netcoreapp.cs Show resolved Hide resolved

bartonjs reviewed Dec 27, 2019

View reviewed changes

...aries/System.Security.Principal.Windows/tests/WindowsIdentityImpersonatedTests.netcoreapp.cs Outdated Show resolved Hide resolved

address PR feedback

4a4d3ae

MarcoRossignoli requested review from danmoseley and bartonjs December 27, 2019 14:11

danmoseley reviewed Dec 27, 2019

View reviewed changes

src/libraries/System.Security.Principal.Windows/ref/System.Security.Principal.Windows.cs Outdated Show resolved Hide resolved

address PR feedback

7714e53

MarcoRossignoli requested a review from danmoseley January 3, 2020 17:35

stephentoub reviewed Jan 6, 2020

View reviewed changes

...aries/System.Security.Principal.Windows/tests/WindowsIdentityImpersonatedTests.netcoreapp.cs Outdated Show resolved Hide resolved

bartonjs reviewed Jan 6, 2020

View reviewed changes

...libraries/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs Outdated Show resolved Hide resolved

address PR feedback

847e028

stephentoub reviewed Jan 14, 2020

View reviewed changes

...libraries/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs Show resolved Hide resolved

stephentoub reviewed Jan 14, 2020

View reviewed changes

...aries/System.Security.Principal.Windows/tests/WindowsIdentityImpersonatedTests.netcoreapp.cs Outdated Show resolved Hide resolved

address PR feedback

636a7a2

MarcoRossignoli requested a review from stephentoub January 15, 2020 14:58

MarcoRossignoli added 5 commits January 17, 2020 23:46

Update WindowsIdentity.cs

fa80b6f

update comment

Update WindowsIdentity.cs

83c42c2

update comment

Update WindowsIdentity.cs

e2fa81a

Fix comment

Update WindowsIdentity.cs

9075c79

fix comment

Fix comment

e24d27e

MarcoRossignoli commented Jan 17, 2020

View reviewed changes

...libraries/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs Show resolved Hide resolved

carlossanlop reviewed Jan 21, 2020

View reviewed changes

...libraries/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs Show resolved Hide resolved

carlossanlop added the new-api-needs-documentation label Jan 21, 2020

stephentoub approved these changes Jan 22, 2020

View reviewed changes

stephentoub merged commit beff8ab into dotnet:master Jan 22, 2020

MarcoRossignoli deleted the asyncrunimpersonated branch January 23, 2020 07:08

ghost locked as resolved and limited conversation to collaborators Dec 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add asynchronous overload of WindowsIdentity.RunImpersonated #1152

Add asynchronous overload of WindowsIdentity.RunImpersonated #1152

MarcoRossignoli commented Dec 25, 2019 •

edited

Loading

MarcoRossignoli commented Dec 25, 2019

MarcoRossignoli commented Dec 25, 2019

azure-pipelines bot commented Dec 25, 2019

safern commented Jan 6, 2020 •

edited

Loading

safern commented Jan 6, 2020

MarcoRossignoli commented Jan 6, 2020

azure-pipelines bot commented Jan 6, 2020

ViktorHofer commented Jan 14, 2020

ViktorHofer commented Jan 14, 2020

stephentoub left a comment

stephentoub Jan 22, 2020

stephentoub Jan 22, 2020

	/// Runs the specified asynchronous action as the impersonated Windows identity
	/// Runs the specified asynchronous action as the impersonated Windows identity.

Add asynchronous overload of WindowsIdentity.RunImpersonated #1152

Add asynchronous overload of WindowsIdentity.RunImpersonated #1152

Conversation

MarcoRossignoli commented Dec 25, 2019 • edited Loading

MarcoRossignoli commented Dec 25, 2019

MarcoRossignoli commented Dec 25, 2019

azure-pipelines bot commented Dec 25, 2019

safern commented Jan 6, 2020 • edited Loading

safern commented Jan 6, 2020

MarcoRossignoli commented Jan 6, 2020

azure-pipelines bot commented Jan 6, 2020

ViktorHofer commented Jan 14, 2020

ViktorHofer commented Jan 14, 2020

stephentoub left a comment

Choose a reason for hiding this comment

stephentoub Jan 22, 2020

Choose a reason for hiding this comment

stephentoub Jan 22, 2020

Choose a reason for hiding this comment

MarcoRossignoli commented Dec 25, 2019 •

edited

Loading

safern commented Jan 6, 2020 •

edited

Loading