• 1

• 75.5k
• 18
• 194
• 467

I don't see a problem with how the SecureString objects are handled - seems pretty solid to me, but I've never used those so I'm possibly missing something that someone else could point out here.

#Sealed Class

I haven't looked at the actual project's source code, but general design guidelines have this to say about sealing classes:

• DO NOT seal classes without having a good reason to do so.

However it also defines a number of "good reasons" for sealing a class, including:

• The class stores security-sensitive secrets in inherited protected members.

In other words, a sealed class seems warranted here.

#InvalidOperationException

Out of all possible existing exception classes in the framework, and instead of creating your own derived exception class, you chose to throw an InvalidOperationException in the implementation of the GitCredentialHandler override.

MSDN describes this exception as follows:

The exception that is thrown when a method call is invalid for the object's current state.

Turns out this was, in my own humble opinion, the best decision to make: throwing any other exception type here would violate POLS - when client code is calling this method and the object's state shouldn't allow it, the programmer has all rights to expect an InvalidOperationException to be thrown.

#XML Documentation

This is an API - having complete and accurate XML documentation is ideal.

 /// <summary>
 /// Callback to acquire a credential object.
 /// </summary>
 /// <param name="cred">The newly created credential object.</param>
 /// <returns>0 for success, &lt; 0 to indicate an error, &gt; 0 to indicate no credential was acquired.</returns>

I think this documentation could be better, but looking at the base class on GitHub I see that you've copied most of it from the base class, which is a good idea.

However the method can throw an InvalidOperationException - you could add something like this:

/// <exception cref="InvalidOperationException">
/// Thrown if this method is called when either <see cref="UserName"/> or <see cref="Password"/> is <c>null</c>.
/// </exception>

#Style

You know me, I think this is too verbose:

IntPtr passwordPtr = IntPtr.Zero;
IntPtr userNamePtr = IntPtr.Zero;

Compared to:

var passwordPtr = IntPtr.Zero;
var userNamePtr = IntPtr.Zero;

Which is just as clear IMO. Not much gain here though. And given that the rest of the code seems to prefer the verbose way, sticking to the existing style is the best decision you could make.

And not only for usage of var:

• I would use a constructor, but it's the style of the project to use an object initializer.
• It's also in the style of the project to place the properties at the bottom like you'll see below.

#UserName vs. Password

A user name can be sensitive information in certain circumstances - and although I don't think this is one, there's a saying that summarizes everything I'd have to say about whether or not having the user name as a SecureString is overkill:

Better safe than sorry.

-- unknown

However, the ironic part is this:

return NativeMethods.git_cred_userpass_plaintext_new(...);

Given that credentials end up being passed as plain text, I think I'd keep the user name as a plain string.