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

Add SecureString password support #9

Closed
wants to merge 2 commits into from

Conversation

michaelwanke
Copy link

Would you consider a modification that would allow a SecureString to be passed to the Impersonation call? If so, is there anything I can do to improve the changes I have included? Thank you for your consideration.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Add SecureString password support for Impersonation call
@apmorton
Copy link

+1 on this - SecureString is a must.

The only thing I would change is leaving _handle and _context readonly, and pass them through to CompleteImpersonation as reference parameters

Realistically this change has no effect on the behavior of the code, but it enforces the original intent that those fields be readonly for the lifetime of the object

@mattjohnsonpint
Copy link
Owner

Looks good to me. Thanks, and sorry I didn't get to this sooner!

@apmorton - That wouldn't help, as the value would still have to be modified. I'm fine removing readonly.

@mattjohnsonpint
Copy link
Owner

merged 78d9a34. I'll bump the version (to 1.1.0) separately before release (soon).

@apmorton
Copy link

@mj1856 this is an example of what I am proposing

    class Class1
    {
        private readonly string _test;

        public Class1()
        {
            test(ref _test);
        }

        private void test(ref string t)
        {
            t = "hello world";
        }
    }

that code is perfectly valid, compiles, works, and preserves the readonly-ness of _test

@mattjohnsonpint
Copy link
Owner

That feels wrong to me... Not sure how it could truly be considered "read-only" then. Let me ponder on this for a bit.

I asked here: http://stackoverflow.com/questions/34552287/why-can-readonly-fields-be-modified-through-ref-parameters

@apmorton
Copy link

It is of course important to note that this only works if you pass it by ref FROM the constructor.

Perhaps out is better suited for this than ref, but the concept is the same.

@michaelwanke
Copy link
Author

Thank you for the discussion and bringing these changes in!

@mattjohnsonpint
Copy link
Owner

No problem. I did some minor refactoring per @apmorton 's feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants