From e57463bdaafcf6e28d4671009d29ff97a8a505b2 Mon Sep 17 00:00:00 2001 From: Andreia Gaita Date: Tue, 3 Apr 2018 16:07:54 +0200 Subject: [PATCH] Dedupe code in ApiClient. Also, cache user data and avoid using the keychain username The system keychain may have a valid token with an invalid username because every git client fills out that information slightly different and the username is not relevant for accessing the API (only the token is needed). If we encounter a mismatch, don't throw immediately (the token might be perfectly valid), but always doublecheck the github user to make sure the username of the connection matches the user that the token is associated with. Cache the GitHubUser object in the Connection list (non serialized) so that if we fill out once and all the information matches, we can reuse it and avoid pinging the API every time. If another git client tramples all over our keychain, it's highly likely that they'll also not fill out the username information anyway, and we always grab the user from GitHub if that happens so we should be relatively safe trusting the cached GitHubUser object (it'll get thrown away whenever Unity reloads so it's also likely that we'll keep refreshing it anyways) In the process of this, cleaned up some duplicated and uneeded code that could lead to issues later on. --- src/GitHub.Api/Application/ApiClient.cs | 97 ++++++++----------- src/GitHub.Api/Application/IApiClient.cs | 3 +- src/GitHub.Api/Authentication/Keychain.cs | 4 +- .../Services/AuthenticationService.cs | 2 +- .../Editor/GitHub.Unity/UI/PopupWindow.cs | 4 +- .../Editor/GitHub.Unity/UI/PublishView.cs | 2 +- .../Assets/Editor/GitHub.Unity/UI/Window.cs | 2 +- .../UnitTests/Authentication/KeychainTests.cs | 2 +- 8 files changed, 51 insertions(+), 65 deletions(-) diff --git a/src/GitHub.Api/Application/ApiClient.cs b/src/GitHub.Api/Application/ApiClient.cs index ab1f71aca..8eac0bd58 100644 --- a/src/GitHub.Api/Application/ApiClient.cs +++ b/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(); public HostAddress HostAddress { get; } public UriString OriginalUrl { get; } @@ -81,15 +70,13 @@ public async Task GetOrganizations(Action onSuccess, Action onError = null) + public async Task GetCurrentUser(Action onSuccess, Action 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) { @@ -97,18 +84,6 @@ public async Task ValidateCurrentUser(Action onSuccess, Action onErro } } - public async Task GetCurrentUser(Action 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 need2faCode, Action result) { Guard.ArgumentNotNull(need2faCode, "need2faCode"); @@ -205,16 +180,33 @@ public async Task ContinueLoginAsync(LoginResult loginResult, Func 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 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 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 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,27 +305,22 @@ private async Task GetOrganizationInternal(Action onSuccess, Act private async Task 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 GetValidatedGitHubUser(Connection keychainConnection, IKeychainAdapter keychainAdapter) @@ -343,7 +328,7 @@ private async Task GetValidatedGitHubUser(Connection keychainConnect 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(); diff --git a/src/GitHub.Api/Application/IApiClient.cs b/src/GitHub.Api/Application/IApiClient.cs index 438e9bbf5..0ab28ceaf 100644 --- a/src/GitHub.Api/Application/IApiClient.cs +++ b/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 LoginAsync(string username, string password, Func need2faCode); Task Logout(UriString host); - Task GetCurrentUser(Action callback); - Task ValidateCurrentUser(Action onSuccess, Action onError = null); + Task GetCurrentUser(Action onSuccess, Action onError = null); } } diff --git a/src/GitHub.Api/Authentication/Keychain.cs b/src/GitHub.Api/Authentication/Keychain.cs index 914749fca..2b3c933e2 100644 --- a/src/GitHub.Api/Authentication/Keychain.cs +++ b/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 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 Load(UriString host) { logger.Warning("Cannot load host from Credential Manager; removing from cache"); await Clear(host, false); + keychainAdapter = null; } else { diff --git a/src/UnityExtension/Assets/Editor/GitHub.Unity/Services/AuthenticationService.cs b/src/UnityExtension/Assets/Editor/GitHub.Unity/Services/AuthenticationService.cs index 73b38da86..cf6f12222 100644 --- a/src/UnityExtension/Assets/Editor/GitHub.Unity/Services/AuthenticationService.cs +++ b/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 twofaRequired, Action authResult) diff --git a/src/UnityExtension/Assets/Editor/GitHub.Unity/UI/PopupWindow.cs b/src/UnityExtension/Assets/Editor/GitHub.Unity/UI/PopupWindow.cs index bfa643fb6..2f421b7a6 100644 --- a/src/UnityExtension/Assets/Editor/GitHub.Unity/UI/PopupWindow.cs +++ b/src/UnityExtension/Assets/Editor/GitHub.Unity/UI/PopupWindow.cs @@ -120,7 +120,7 @@ private void Open(PopupViewType popupViewType, Action 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; diff --git a/src/UnityExtension/Assets/Editor/GitHub.Unity/UI/PublishView.cs b/src/UnityExtension/Assets/Editor/GitHub.Unity/UI/PublishView.cs index 5f47fe2f6..e9ab3ceb9 100644 --- a/src/UnityExtension/Assets/Editor/GitHub.Unity/UI/PublishView.cs +++ b/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; diff --git a/src/UnityExtension/Assets/Editor/GitHub.Unity/UI/Window.cs b/src/UnityExtension/Assets/Editor/GitHub.Unity/UI/Window.cs index 6551d900a..a3f72eb92 100644 --- a/src/UnityExtension/Assets/Editor/GitHub.Unity/UI/Window.cs +++ b/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); } diff --git a/src/tests/UnitTests/Authentication/KeychainTests.cs b/src/tests/UnitTests/Authentication/KeychainTests.cs index f86623ace..ba5244abd 100644 --- a/src/tests/UnitTests/Authentication/KeychainTests.cs +++ b/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);