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);