Merge pull request #33 from github-for-unity/fixes/secure-keychain-adapter

Fix for Keychain to validate credentials from CredentialManager

Author: shana

src/GitHub.Api/Authentication/Keychain.cs

@@ -52,19 +52,26 @@ public IKeychainAdapter Connect(UriString host)
 
         public async Task<IKeychainAdapter> Load(UriString host)
         {
-            var keychainAdapter = FindOrCreateAdapter(host);
+            KeychainAdapter keychainAdapter = FindOrCreateAdapter(host);
+            var cachedConnection = connectionCache[host];
+
+            logger.Trace($@"Loading KeychainAdapter Host:""{host}"" Cached Username:""{cachedConnection.Username}""");
 
-            logger.Trace("Load KeychainAdapter Host:\"{0}\"", host);
             var keychainItem = await credentialManager.Load(host);
 
             if (keychainItem == null)
             {
-                logger.Warning("Cannot load host from credential manager; removing from cache");
+                logger.Warning("Cannot load host from Credential Manager; removing from cache");
+                await Clear(host, false);
+            }
+            else if (keychainItem.Username != cachedConnection.Username)
+            {
+                logger.Warning("Item loaded from credential manager does not match connection cache ; removing from cache");
                 await Clear(host, false);
             }
             else
             {
-                logger.Trace("Loading KeychainItem:{0}", keychainItem.ToString());
+                logger.Trace($@"Loaded from Credential Manager Host:""{keychainItem.Host}"" Username:""{keychainItem.Username}""");
                 keychainAdapter.Set(keychainItem);
             }
 
@@ -113,7 +120,10 @@ private void ReadCacheFromDisk()
             if (connections != null)
             {
                 connectionCache =
-                    connections.Select(item => new Connection { Host = new UriString(item.Host), Username = item.Username })
+                    connections.Select(item => {
+                                    logger.Trace("ReadCacheFromDisk Item Host:{0} Username:{1}", item.Host, item.Username);
+                                    return new Connection { Host = new UriString(item.Host), Username = item.Username };
+                               })
                                .ToDictionary(connection => connection.Host);
             }
             else

src/tests/UnitTests/Authentication/KeychainTests.cs

@@ -18,7 +18,7 @@ public class KeychainTests
         private static readonly SubstituteFactory SubstituteFactory = new SubstituteFactory();
 
         [Test]
-        public void Should_Initialize_When_Cache_Does_Not_Exist()
+        public void ShouldInitializeWhenCacheDoesNotExist()
         {
             const string connectionsCachePath = @"c:\UserCachePath\";
 
@@ -52,7 +52,7 @@ public void Should_Initialize_When_Cache_Does_Not_Exist()
         }
 
         [Test]
-        public void Should_Initialize_When_Cache_Invalid()
+        public void ShouldInitializeWhenCacheInvalid()
         {
             const string connectionsCachePath = @"c:\UserCachePath\";
             const string connectionsCacheFile = @"c:\UserCachePath\connections.json";
@@ -92,7 +92,7 @@ public void Should_Initialize_When_Cache_Invalid()
         }
 
         [Test]
-        public void Should_Initialize_When_Cache_Exists()
+        public void ShouldInitializeWhenCacheExists()
         {
             const string connectionsCachePath = @"c:\UserCachePath\";
             const string connectionsCacheFile = @"c:\UserCachePath\connections.json";
@@ -134,7 +134,7 @@ public void Should_Initialize_When_Cache_Exists()
         }
 
         [Test]
-        public void Should_Load_From_ConnectionManager()
+        public void ShouldLoadFromConnectionManager()
         {
             const string connectionsCachePath = @"c:\UserCachePath\";
             const string connectionsCacheFile = @"c:\UserCachePath\connections.json";
@@ -194,7 +194,7 @@ public void Should_Load_From_ConnectionManager()
         }
 
         [Test]
-        public void Should_Delete_From_Cache_When_Load_Returns_Null_From_ConnectionManager()
+        public void ShouldDeleteFromCacheWhenLoadReturnsNullFromConnectionManager()
         {
             const string connectionsCachePath = @"c:\UserCachePath\";
             const string connectionsCacheFile = @"c:\UserCachePath\connections.json";
@@ -248,7 +248,82 @@ public void Should_Delete_From_Cache_When_Load_Returns_Null_From_ConnectionManag
         }
 
         [Test]
-        public void Should_Connect_Set_Credentials_Token_And_Save()
+        public void ShouldDeleteFromCacheWhenLoadReturnsNullFromConnectionManagerDueToUserMismatch()
+        {
+            const string connectionsCachePath = @"c:\UserCachePath\";
+            const string connectionsCacheFile = @"c:\UserCachePath\connections.json";
+
+            const string cachedUsername = "SomeCachedUser";
+            const string credentialedUsername = "SomeCredentialedUser";
+
+            const string token = "SomeToken";
+
+            var hostUri = new UriString("https://github.com/");
+
+            var fileSystem = SubstituteFactory.CreateFileSystem(new CreateFileSystemOptions
+            {
+                FilesThatExist = new List<string> { connectionsCacheFile },
+                FileContents = new Dictionary<string, IList<string>> {
+                    {connectionsCacheFile, new List<string> {$@"[{{""Host"":""https://github.com/"",""Username"":""{cachedUsername}""}}]"
+                    }}
+                }
+            });
+
+            NPath.FileSystem = fileSystem;
+
+            var environment = SubstituteFactory.CreateEnvironment();
+            environment.UserCachePath.Returns(info => connectionsCachePath.ToNPath());
+            environment.FileSystem.Returns(fileSystem);
+
+            var credentialManager = Substitute.For<ICredentialManager>();
+            credentialManager.Load(hostUri).Returns(info =>
+            {
+                var credential = Substitute.For<ICredential>();
+                credential.Username.Returns(credentialedUsername);
+                credential.Token.Returns(token);
+                credential.Host.Returns(hostUri);
+                return TaskEx.FromResult(credential);
+            });
+
+            var keychain = new Keychain(environment, credentialManager);
+            keychain.Initialize();
+
+            fileSystem.Received(1).FileExists(connectionsCacheFile);
+            fileSystem.DidNotReceive().FileDelete(Args.String);
+            fileSystem.Received(1).ReadAllText(connectionsCacheFile);
+            fileSystem.DidNotReceive().ReadAllLines(Args.String);
+            fileSystem.DidNotReceive().WriteAllText(Args.String, Args.String);
+            fileSystem.DidNotReceive().WriteAllLines(Args.String, Arg.Any<string[]>());
+
+            credentialManager.DidNotReceive().Load(Args.UriString);
+            credentialManager.DidNotReceive().HasCredentials();
+            credentialManager.DidNotReceive().Delete(Args.UriString);
+            credentialManager.DidNotReceive().Save(Arg.Any<ICredential>());
+
+            fileSystem.ClearReceivedCalls();
+
+            var uriString = keychain.Connections.FirstOrDefault();
+            var keychainAdapter = keychain.Load(uriString).Result;
+            keychainAdapter.Credential.Should().BeNull();
+
+            keychainAdapter.OctokitCredentials.AuthenticationType.Should().Be(AuthenticationType.Anonymous);
+            keychainAdapter.OctokitCredentials.Login.Should().BeNull();
+            keychainAdapter.OctokitCredentials.Password.Should().BeNull();
+
+            fileSystem.DidNotReceive().FileExists(Args.String);
+            fileSystem.DidNotReceive().ReadAllText(Args.String);
+            fileSystem.DidNotReceive().FileDelete(Args.String);
+            fileSystem.Received(1).WriteAllText(connectionsCacheFile, "[]");
+            fileSystem.DidNotReceive().WriteAllLines(Args.String, Arg.Any<string[]>());
+
+            credentialManager.Received(1).Load(hostUri);
+            credentialManager.DidNotReceive().HasCredentials();
+            credentialManager.DidNotReceive().Delete(Args.UriString);
+            credentialManager.DidNotReceive().Save(Arg.Any<ICredential>());
+        }
+
+        [Test]
+        public void ShouldConnectSetCredentialsTokenAndSave()
         {
             const string connectionsCachePath = @"c:\UserCachePath\";
             const string connectionsCacheFile = @"c:\UserCachePath\connections.json";
@@ -334,7 +409,7 @@ public void Should_Connect_Set_Credentials_Token_And_Save()
         }
 
         [Test]
-        public void Should_Connect_Set_Credentials_And_Clear()
+        public void ShouldConnectSetCredentialsAndClear()
         {
             const string connectionsCachePath = @"c:\UserCachePath\";
             const string connectionsCacheFile = @"c:\UserCachePath\connections.json";

src/tests/UnitTests/SetUpFixture.cs

@@ -10,7 +10,7 @@ public class SetUpFixture
         [SetUp]
         public void SetUp()
         {
-            //Logging.TracingEnabled = true;
+            Logging.TracingEnabled = true;
 
             Logging.LogAdapter = new MultipleLogAdapter(new FileLogAdapter($"..\\{DateTime.UtcNow.ToString("yyyyMMddHHmmss")}-unit-tests.log"));
         }