ApiClient and keychain adapter cleanup and fixes

src/GitHub.Api/Application/ApiClient.cs

@@ -10,17 +10,6 @@ namespace GitHub.Unity
 {
     class ApiClient : IApiClient
     {
-        public static IApiClient Create(UriString repositoryUrl, IKeychain keychain, IProcessManager processManager, ITaskManager taskManager, NPath nodeJsExecutablePath, NPath octorunScriptPath)
-        {
-            logger.Trace("Creating ApiClient: {0}", repositoryUrl);
-
-            var credentialStore = keychain.Connect(repositoryUrl);
-            var hostAddress = HostAddress.Create(repositoryUrl);
-
-            return new ApiClient(repositoryUrl, keychain,
-                processManager, taskManager, nodeJsExecutablePath, octorunScriptPath);
-        }
-
         private static readonly ILogging logger = LogHelper.GetLogger<ApiClient>();
         public HostAddress HostAddress { get; }
         public UriString OriginalUrl { get; }
@@ -81,34 +70,20 @@ public async Task GetOrganizations(Action<Organization[]> onSuccess, Action<Exce
             await GetOrganizationInternal(onSuccess, onError);
         }
 
-        public async Task ValidateCurrentUser(Action onSuccess, Action<Exception> onError = null)
+        public async Task GetCurrentUser(Action<GitHubUser> onSuccess, Action<Exception> onError = null)
         {
             Guard.ArgumentNotNull(onSuccess, nameof(onSuccess));
             try
             {
-                var keychainConnection = keychain.Connections.First();
-                var keychainAdapter = await GetValidatedKeychainAdapter(keychainConnection);
-                await GetValidatedGitHubUser(keychainConnection, keychainAdapter);
-                onSuccess();
+                var user = await GetCurrentUser();
+                onSuccess(user);
             }
             catch (Exception e)
             {
                 onError?.Invoke(e);
             }
         }
 
-        public async Task GetCurrentUser(Action<GitHubUser> callback)
-        {
-            Guard.ArgumentNotNull(callback, "callback");
-
-            //TODO: ONE_USER_LOGIN This assumes only ever one user can login
-            var keychainConnection = keychain.Connections.First();
-            var keychainAdapter = await GetValidatedKeychainAdapter(keychainConnection);
-            var user = await GetValidatedGitHubUser(keychainConnection, keychainAdapter);
-
-            callback(user);
-        }
-
         public async Task Login(string username, string password, Action<LoginResult> need2faCode, Action<bool, string> result)
         {
             Guard.ArgumentNotNull(need2faCode, "need2faCode");
@@ -205,16 +180,33 @@ public async Task<bool> ContinueLoginAsync(LoginResult loginResult, Func<LoginRe
             return result.Code == LoginResultCodes.Success;
         }
 
+        private async Task<GitHubUser> GetCurrentUser()
+        {
+            //TODO: ONE_USER_LOGIN This assumes we only support one login
+            var keychainConnection = keychain.Connections.FirstOrDefault();
+            if (keychainConnection == null)
+                throw new KeychainEmptyException();
+
+            var keychainAdapter = await GetValidatedKeychainAdapter(keychainConnection);
+
+            // we can't trust that the system keychain has the username filled out correctly.
+            // if it doesn't, we need to grab the username from the server and check it
+            // unfortunately this means that things will be slower when the keychain doesn't have all the info
+            if (keychainConnection.User == null || keychainAdapter.Credential.Username != keychainConnection.Username)
+            {
+                keychainConnection.User = await GetValidatedGitHubUser(keychainConnection, keychainAdapter);
+            }
+            return keychainConnection.User;
+        }
+
         private async Task<GitHubRepository> CreateRepositoryInternal(string repositoryName, string organization, string description, bool isPrivate)
         {
             try
             {
                 logger.Trace("Creating repository");
 
-                //TODO: ONE_USER_LOGIN This assumes only ever one user can login
-                var keychainConnection = keychain.Connections.First();
-                var keychainAdapter = await GetValidatedKeychainAdapter(keychainConnection);
-                await GetValidatedGitHubUser(keychainConnection, keychainAdapter);
+                var user = await GetCurrentUser();
+                var keychainAdapter = keychain.Connect(OriginalUrl);
 
                 var command = new StringBuilder("publish -r \"");
                 command.Append(repositoryName);
@@ -240,7 +232,7 @@ private async Task<GitHubRepository> CreateRepositoryInternal(string repositoryN
                 }
 
                 var octorunTask = new OctorunTask(taskManager.Token, nodeJsExecutablePath, octorunScriptPath, command.ToString(),
-                    user: keychainAdapter.Credential.Username, userToken: keychainAdapter.Credential.Token)
+                    user: user.Login, userToken: keychainAdapter.Credential.Token)
                     .Configure(processManager);
 
                 var ret = await octorunTask.StartAwait();
@@ -273,13 +265,11 @@ private async Task GetOrganizationInternal(Action<Organization[]> onSuccess, Act
             {
                 logger.Trace("Getting Organizations");
 
-                //TODO: ONE_USER_LOGIN This assumes only ever one user can login
-                var keychainConnection = keychain.Connections.First();
-                var keychainAdapter = await GetValidatedKeychainAdapter(keychainConnection);
-                await GetValidatedGitHubUser(keychainConnection, keychainAdapter);
+                var user = await GetCurrentUser();
+                var keychainAdapter = keychain.Connect(OriginalUrl);
 
                 var octorunTask = new OctorunTask(taskManager.Token, nodeJsExecutablePath, octorunScriptPath, "organizations",
-                        user: keychainAdapter.Credential.Username, userToken: keychainAdapter.Credential.Token)
+                        user: user.Login, userToken: keychainAdapter.Credential.Token)
                     .Configure(processManager);
 
                 var ret = await octorunTask.StartAsAsync();
@@ -315,35 +305,30 @@ private async Task GetOrganizationInternal(Action<Organization[]> onSuccess, Act
 
         private async Task<IKeychainAdapter> GetValidatedKeychainAdapter(Connection keychainConnection)
         {
-            if (keychain.HasKeys)
-            {
-                var keychainAdapter = await keychain.Load(keychainConnection.Host);
-
-                if (string.IsNullOrEmpty(keychainAdapter.Credential?.Username))
-                {
-                    logger.Warning("LoadKeychainInternal: Username is empty");
-                    throw new TokenUsernameMismatchException(keychainConnection.Username);
-                }
+            var keychainAdapter = await keychain.Load(keychainConnection.Host);
+            if (keychainAdapter == null)
+                throw new KeychainEmptyException();
 
-                if (keychainAdapter.Credential.Username != keychainConnection.Username)
-                {
-                    logger.Warning("LoadKeychainInternal: Token username does not match");
-                    throw new TokenUsernameMismatchException(keychainConnection.Username, keychainAdapter.Credential.Username);
-                }
+            if (string.IsNullOrEmpty(keychainAdapter.Credential?.Username))
+            {
+                logger.Warning("LoadKeychainInternal: Username is empty");
+                throw new TokenUsernameMismatchException(keychainConnection.Username);
+            }
 
-                return keychainAdapter;
+            if (keychainAdapter.Credential.Username != keychainConnection.Username)
+            {
+                logger.Warning("LoadKeychainInternal: Token username does not match");
             }
 
-            logger.Warning("LoadKeychainInternal: No keys to load");
-            throw new KeychainEmptyException();
+            return keychainAdapter;
         }
 
         private async Task<GitHubUser> GetValidatedGitHubUser(Connection keychainConnection, IKeychainAdapter keychainAdapter)
         {
             try
             {
                 var octorunTask = new OctorunTask(taskManager.Token, nodeJsExecutablePath, octorunScriptPath, "validate",
-                        user: keychainAdapter.Credential.Username, userToken: keychainAdapter.Credential.Token)
+                        user: keychainConnection.Username, userToken: keychainAdapter.Credential.Token)
                     .Configure(processManager);
 
                 var ret = await octorunTask.StartAsAsync();

src/GitHub.Api/Application/IApiClient.cs

@@ -14,7 +14,6 @@ Task CreateRepository(string name, string description, bool isPrivate,
         Task ContinueLogin(LoginResult loginResult, string code);
         Task<bool> LoginAsync(string username, string password, Func<LoginResult, string> need2faCode);
         Task Logout(UriString host);
-        Task GetCurrentUser(Action<GitHubUser> callback);
-        Task ValidateCurrentUser(Action onSuccess, Action<Exception> onError = null);
+        Task GetCurrentUser(Action<GitHubUser> onSuccess, Action<Exception> onError = null);
     }
 }

src/GitHub.Api/Authentication/Keychain.cs

@@ -12,6 +12,7 @@ public class Connection
     {
         public string Host { get; set; }
         public string Username { get; set; }
+        [NonSerialized] internal GitHubUser User;
 
         // for json serialization
         public Connection()
@@ -99,7 +100,7 @@ public IKeychainAdapter Connect(UriString host)
 
         public async Task<IKeychainAdapter> Load(UriString host)
         {
-            KeychainAdapter keychainAdapter = FindOrCreateAdapter(host);
+            var keychainAdapter = FindOrCreateAdapter(host);
             var connection = GetConnection(host);
 
             //logger.Trace($@"Loading KeychainAdapter Host:""{host}"" Cached Username:""{cachedConnection.Username}""");
@@ -109,6 +110,7 @@ public async Task<IKeychainAdapter> Load(UriString host)
             {
                 logger.Warning("Cannot load host from Credential Manager; removing from cache");
                 await Clear(host, false);
+                keychainAdapter = null;
             }
             else
             {

src/UnityExtension/Assets/Editor/GitHub.Unity/Services/AuthenticationService.cs

@@ -10,7 +10,7 @@ class AuthenticationService
 
         public AuthenticationService(UriString host, IKeychain keychain, NPath nodeJsExecutablePath, NPath octorunExecutablePath)
         {
-            client = ApiClient.Create(host, keychain, EntryPoint.ApplicationManager.ProcessManager, EntryPoint.ApplicationManager.TaskManager, nodeJsExecutablePath, octorunExecutablePath);
+            client = new ApiClient(host, keychain, EntryPoint.ApplicationManager.ProcessManager, EntryPoint.ApplicationManager.TaskManager, nodeJsExecutablePath, octorunExecutablePath);
         }
 
         public void Login(string username, string password, Action<string> twofaRequired, Action<bool, string> authResult)

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/PopupWindow.cs

@@ -120,7 +120,7 @@ private void Open(PopupViewType popupViewType, Action<bool> onClose)
             {
                 //Logger.Trace("Validating to open view");
 
-                Client.ValidateCurrentUser(() => {
+                Client.GetCurrentUser(user => {
 
                     //Logger.Trace("User validated opening view");
 
@@ -192,7 +192,7 @@ public IApiClient Client
                         host = UriString.ToUriString(HostAddress.GitHubDotComHostAddress.WebUri);
                     }
 
-                    client = ApiClient.Create(host, Platform.Keychain, Manager.ProcessManager, TaskManager, Environment.NodeJsExecutablePath, Environment.OctorunScriptPath);
+                    client = new ApiClient(host, Platform.Keychain, Manager.ProcessManager, TaskManager, Environment.NodeJsExecutablePath, Environment.OctorunScriptPath);
                 }
 
                 return client;

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/PublishView.cs

@@ -52,7 +52,7 @@ public IApiClient Client
                         host = UriString.ToUriString(HostAddress.GitHubDotComHostAddress.WebUri);
                     }
 
-                    client = ApiClient.Create(host, Platform.Keychain, Manager.ProcessManager, TaskManager, Environment.NodeJsExecutablePath, Environment.OctorunScriptPath);
+                    client = new ApiClient(host, Platform.Keychain, Manager.ProcessManager, TaskManager, Environment.NodeJsExecutablePath, Environment.OctorunScriptPath);
                 }
 
                 return client;

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/Window.cs

@@ -497,7 +497,7 @@ private void SignOut(object obj)
                 host = UriString.ToUriString(HostAddress.GitHubDotComHostAddress.WebUri);
             }
 
-            var apiClient = ApiClient.Create(host, Platform.Keychain, null, null, NPath.Default, NPath.Default);
+            var apiClient = new ApiClient(host, Platform.Keychain, null, null, NPath.Default, NPath.Default);
             apiClient.Logout(host);
         }
 

src/tests/UnitTests/Authentication/KeychainTests.cs

@@ -223,7 +223,7 @@ public void ShouldDeleteFromCacheWhenLoadReturnsNullFromConnectionManager()
 
             var uriString = keychain.Hosts.FirstOrDefault();
             var keychainAdapter = keychain.Load(uriString).Result;
-            keychainAdapter.Credential.Should().BeNull();
+            keychainAdapter.Should().BeNull();
 
             fileSystem.DidNotReceive().FileExists(Args.String);
             fileSystem.DidNotReceive().ReadAllText(Args.String);