Only erase credentials that match user/pass when present

Only erase stored credentials when the user name and password values
match those passed on standard input, if any. When no user name and/or
password are given on standard input we skip checking the stored value
and just erase the credential.

This is an important but possibly subtle behaviour to help prevent
erroneous erasure of credentials.

Calls to `git-credential fill` should immediately be followed by
`git-credential approve` or `reject`. If there are concurrent processes
that have called `fill` but not yet `approve` or `reject`, there's a
gap when another (faster) process could have completed the
`fill+approve` combination and have stored a different credential value
than what the first process got from `fill`.

If the first process fails and calls `reject` the valid credential as
`approve`-ed by the second process would be deleted. This change helps
prevent such a scenario (although there is still a small gap inside the
GCM erase command itself between reading the OS's credential store and
deleting from it - we could look at some system-wide lock around the
credential store if this is deemed a problem later).

Author: mjcheetham

common/src/Microsoft.Git.CredentialManager/Commands/EraseCommand.cs

@@ -1,6 +1,8 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license.
+using System;
 using System.Threading.Tasks;
+using Microsoft.Git.CredentialManager.SecureStorage;
 
 namespace Microsoft.Git.CredentialManager.Commands
 {
@@ -16,13 +18,46 @@ public EraseCommand(IHostProviderRegistry hostProviderRegistry)
 
         protected override Task ExecuteInternalAsync(ICommandContext context, InputArguments input, IHostProvider provider, string credentialKey)
         {
-            context.Trace.WriteLine("Erasing credential...");
+            // Try to locate an existing credential with the computed key
+            context.Trace.WriteLine("Looking for existing credential in store...");
+            ICredential credential = context.CredentialStore.Get(credentialKey);
+            if (credential == null)
+            {
+                context.Trace.WriteLine("No stored credential was found.");
+                return Task.CompletedTask;
+            }
+            else
+            {
+                context.Trace.WriteLine("Existing credential found.");
+            }
+
+            // If we've been given a specific username and/or password we should only proceed
+            // to erase the stored credential if they match exactly
+            if (!string.IsNullOrWhiteSpace(input.UserName) && !StringComparer.Ordinal.Equals(input.UserName, credential.UserName))
+            {
+                context.Trace.WriteLine("Stored username does not match specified username - not erasing credential.");
+                context.Trace.WriteLine($"\tInput  username={input.UserName}");
+                context.Trace.WriteLine($"\tStored username={credential.UserName}");
+                return Task.CompletedTask;
+            }
+
+            if (!string.IsNullOrWhiteSpace(input.Password) && !StringComparer.Ordinal.Equals(input.Password, credential.Password))
+            {
+                context.Trace.WriteLine("Stored password does not match specified password - not erasing credential.");
+                context.Trace.WriteLineSecrets("\tInput  password={0}", new object[] {input.Password});
+                context.Trace.WriteLineSecrets("\tStored password={0}", new object[] {credential.Password});
+                return Task.CompletedTask;
+            }
 
-            // Delete the credential in the store.
+            context.Trace.WriteLine("Erasing stored credential...");
             if (context.CredentialStore.Remove(credentialKey))
             {
                 context.Trace.WriteLine("Credential was successfully erased.");
             }
+            else
+            {
+                context.Trace.WriteLine("Credential erase failed.");
+            }
 
             return Task.CompletedTask;
         }

common/tests/Microsoft.Git.CredentialManager.Tests/Commands/EraseCommandTests.cs

@@ -30,7 +30,7 @@ public void EraseCommand_CanExecuteAsync(string argString, bool expected)
         }
 
         [Fact]
-        public async Task EraseCommand_ExecuteAsync_CredentialExists_ErasesCredential()
+        public async Task EraseCommand_ExecuteAsync_NoInputUserPass_CredentialExists_ErasesCredential()
         {
             const string testCredentialKey = "test-cred-key";
 
@@ -57,6 +57,90 @@ public async Task EraseCommand_ExecuteAsync_CredentialExists_ErasesCredential()
             Assert.True(context.CredentialStore.ContainsKey("git:credential2"));
         }
 
+        [Fact]
+        public async Task EraseCommand_ExecuteAsync_InputUserPass_CredentialExists_UserNotMatch_DoesNothing()
+        {
+            const string testUserName = "john.doe";
+            const string testPassword = "letmein123";
+            const string testCredentialKey = "test-cred-key";
+            string stdIn = $"username={testUserName}\npassword={testPassword}\n\n";
+
+            var provider = new TestHostProvider {CredentialKey = testCredentialKey};
+            var providerRegistry = new TestHostProviderRegistry {Provider = provider};
+            var context = new TestCommandContext
+            {
+                StdIn = stdIn,
+                CredentialStore =
+                {
+                    [$"git:{testCredentialKey}"] = new GitCredential("different-username", testPassword),
+                }
+            };
+
+            string[] cmdArgs = {"erase"};
+            var command = new EraseCommand(providerRegistry);
+
+            await command.ExecuteAsync(context, cmdArgs);
+
+            Assert.Equal(1, context.CredentialStore.Count);
+            Assert.True(context.CredentialStore.ContainsKey($"git:{testCredentialKey}"));
+        }
+
+        [Fact]
+        public async Task EraseCommand_ExecuteAsync_InputUserPass_CredentialExists_PassNotMatch_DoesNothing()
+        {
+            const string testUserName = "john.doe";
+            const string testPassword = "letmein123";
+            const string testCredentialKey = "test-cred-key";
+            string stdIn = $"username={testUserName}\npassword={testPassword}\n\n";
+
+            var provider = new TestHostProvider {CredentialKey = testCredentialKey};
+            var providerRegistry = new TestHostProviderRegistry {Provider = provider};
+            var context = new TestCommandContext
+            {
+                StdIn = stdIn,
+                CredentialStore =
+                {
+                    [$"git:{testCredentialKey}"] = new GitCredential(testUserName, "different-password"),
+                }
+            };
+
+            string[] cmdArgs = {"erase"};
+            var command = new EraseCommand(providerRegistry);
+
+            await command.ExecuteAsync(context, cmdArgs);
+
+            Assert.Equal(1, context.CredentialStore.Count);
+            Assert.True(context.CredentialStore.ContainsKey($"git:{testCredentialKey}"));
+        }
+
+        [Fact]
+        public async Task EraseCommand_ExecuteAsync_InputUserPass_CredentialExists_UserPassMatch_ErasesCredential()
+        {
+            const string testUserName = "john.doe";
+            const string testPassword = "letmein123";
+            const string testCredentialKey = "test-cred-key";
+            string stdIn = $"username={testUserName}\npassword={testPassword}\n\n";
+
+            var provider = new TestHostProvider {CredentialKey = testCredentialKey};
+            var providerRegistry = new TestHostProviderRegistry {Provider = provider};
+            var context = new TestCommandContext
+            {
+                StdIn = stdIn,
+                CredentialStore =
+                {
+                    [$"git:{testCredentialKey}"] = new GitCredential(testUserName, testPassword),
+                }
+            };
+
+            string[] cmdArgs = {"erase"};
+            var command = new EraseCommand(providerRegistry);
+
+            await command.ExecuteAsync(context, cmdArgs);
+
+            Assert.Equal(0, context.CredentialStore.Count);
+            Assert.False(context.CredentialStore.ContainsKey($"git:{testCredentialKey}"));
+        }
+
         [Fact]
         public async Task EraseCommand_ExecuteAsync_NoCredential_DoesNothing()
         {