Skip to content
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.

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

Consolidate SafeTokenHandle and SafeAccessTokenHandle #16030

Closed
venkat-raman251 opened this issue Jan 6, 2016 · 7 comments
Closed

Consolidate SafeTokenHandle and SafeAccessTokenHandle #16030

venkat-raman251 opened this issue Jan 6, 2016 · 7 comments
Assignees
Labels
area-Microsoft.Win32 enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@venkat-raman251
Copy link
Contributor

SafeTokenHandle and SafeAccessTokenHandle in corefx / src / System.Security.Principal.Windows / src / Microsoft / Win32 / SafeHandles / seem to be the same. They must be consolidated into one or needs investigation as to why they both are required.

@venkat-raman251
Copy link
Contributor Author

@weshaggard : Who can look into this? Or is this just a placeholder for us to visit later.

@weshaggard
Copy link
Member

It is a clean-up work-item that we will visit later. This is a perfect "up-for-grabs" item that someone in the community might be willing to pickup.

@venkat-raman251
Copy link
Contributor Author

@weshaggard Would this item include cleaning up the Common folder? Which is currentlly inconsistent as some use SafeTokenHandle and some use SafeAccessTokenHandle

@Clockwork-Muse
Copy link
Contributor

From a cursory look: the static SafeTokenHandle.InvalidHandle can be set to a valid handle. That might result in surprises later.

@Clockwork-Muse
Copy link
Contributor

SafeAccessTokenHandle has a ref which is annotated with SecurityCritical, even though the src one is not.
SafeTokenHandle in the System.Security.Principal.Windows package appears to be unused.
There are additional copies in System.Security.AccessControl and System.Diagnostics.Process, one of which is identical and one which is a copy of SafeAccessTokenHandle.

System.Security.AccessControl has a reference to Principal.Windows, so presumably could use that one if it was made public (but the visibility change would likely be bad).

So should all three be consolidated into one? (presumably into Common, then)

@AlexBHarley
Copy link
Contributor

@Clockwork-Muse started working on this in PR dotnet/corefx#6101 - however as of your latest comment, it seems like there is more to be done

stephentoub referenced this issue in dotnet/corefx Feb 21, 2016
Fix for issue #5209 - Consolidated SafeAccessTokenHandle and SafeTokenHandle
@stephentoub
Copy link
Member

Fixed by dotnet/corefx#6282

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 1.0.0-rtm milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Jan 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Microsoft.Win32 enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

6 participants