From 553977c16c491274723decc54a5b2fb5bb72256a Mon Sep 17 00:00:00 2001 From: Stanley Goldman Date: Wed, 11 Oct 2017 17:07:14 -0400 Subject: [PATCH 1/2] Throwing an exception when the cached username does not match the one we get from the api --- src/GitHub.Api/Application/ApiClient.cs | 60 +++++++++++++---------- src/GitHub.Api/Application/IApiClient.cs | 1 - src/GitHub.Api/Authentication/Keychain.cs | 2 +- 3 files changed, 35 insertions(+), 28 deletions(-) diff --git a/src/GitHub.Api/Application/ApiClient.cs b/src/GitHub.Api/Application/ApiClient.cs index c4ae1782d..d5bad7799 100644 --- a/src/GitHub.Api/Application/ApiClient.cs +++ b/src/GitHub.Api/Application/ApiClient.cs @@ -195,10 +195,8 @@ public async Task ContinueLoginAsync(LoginResult loginResult, Func> GetOrganizationInternal() { logger.Trace("Getting Organizations"); - if (!await LoadKeychainInternal()) - { - return new List(); - } + await ValidateKeychain(); + await ValidateCurrentUserInternal(); var organizations = await githubClient.Organization.GetAllForCurrent(); @@ -257,16 +253,16 @@ private async Task> GetOrganizationInternal() { try { - logger.Trace("Getting Organizations"); + logger.Trace("Getting Current User"); if (!await LoadKeychainInternal()) { - return null; + throw new ApplicationException("Error Loading Keychain"); } userCache = await githubClient.User.Current(); } - catch(Exception ex) + catch (Exception ex) { logger.Error(ex, "Error Getting Current User"); throw; @@ -275,6 +271,29 @@ private async Task> GetOrganizationInternal() return userCache; } + private async Task ValidateCurrentUserInternal() + { + Octokit.User apiUsername; + string cachedUsername; + + try + { + logger.Trace("Validating User"); + apiUsername = await GetCurrentUserInternal(); + cachedUsername = keychain.Connections.First().Username; + } + catch (Exception ex) + { + logger.Error(ex, "Error Validating User"); + throw; + } + + if (apiUsername.Name != cachedUsername) + { + throw new ApplicationException("Credentialed user does not match API"); + } + } + private async Task LoadKeychainInternal() { logger.Trace("LoadKeychainInternal"); @@ -283,7 +302,7 @@ private async Task LoadKeychainInternal() { if (!keychain.NeedsLoad) { - logger.Trace("LoadKeychainInternal: Has keys does not need load"); + logger.Trace("LoadKeychainInternal: Previously Loaded"); return true; } @@ -301,23 +320,12 @@ private async Task LoadKeychainInternal() return false; } - public async Task ValidateCredentials() + private async Task ValidateKeychain() { - try + if (!await LoadKeychainInternal()) { - var store = keychain.Connect(OriginalUrl); - - if (store.OctokitCredentials != Credentials.Anonymous) - { - var credential = store.Credential; - await githubClient.Authorization.CheckApplicationAuthentication(ApplicationInfo.ClientId, credential.Token); - } - } - catch - { - return false; + throw new ApplicationException("Error Loading Keychain"); } - return true; } } } diff --git a/src/GitHub.Api/Application/IApiClient.cs b/src/GitHub.Api/Application/IApiClient.cs index 1ebbcd414..78b72e280 100644 --- a/src/GitHub.Api/Application/IApiClient.cs +++ b/src/GitHub.Api/Application/IApiClient.cs @@ -14,7 +14,6 @@ interface IApiClient Task Login(string username, string password, Action need2faCode, Action result); Task ContinueLogin(LoginResult loginResult, string code); Task LoginAsync(string username, string password, Func need2faCode); - Task ValidateCredentials(); Task Logout(UriString host); Task GetCurrentUser(Action callback); Task LoadKeychain(Action callback); diff --git a/src/GitHub.Api/Authentication/Keychain.cs b/src/GitHub.Api/Authentication/Keychain.cs index b531735f9..bab311239 100644 --- a/src/GitHub.Api/Authentication/Keychain.cs +++ b/src/GitHub.Api/Authentication/Keychain.cs @@ -68,7 +68,7 @@ public async Task Load(UriString host) { if (keychainItem.Username != cachedConnection.Username) { - logger.Warning("Keychain Username: {0} does not match; Hopefully it works", keychainItem.Username); + logger.Warning("Keychain Username:\"{0}\" does not match cached Username:\"{1}\"; Hopefully it works", keychainItem.Username, cachedConnection.Username); } logger.Trace("Loaded from Credential Manager Host:\"{0}\" Username:\"{1}\"", keychainItem.Host, keychainItem.Username); From 7568a55cc677f9c74aa2977a57a3f4b23a61fb10 Mon Sep 17 00:00:00 2001 From: Stanley Goldman Date: Thu, 12 Oct 2017 10:47:56 -0400 Subject: [PATCH 2/2] Validating the user and throwing custom exceptions --- src/GitHub.Api/Application/ApiClient.cs | 92 +++++++++++-------- src/GitHub.Api/Application/IApiClient.cs | 2 +- .../Editor/GitHub.Unity/UI/PublishView.cs | 18 ++++ 3 files changed, 71 insertions(+), 41 deletions(-) diff --git a/src/GitHub.Api/Application/ApiClient.cs b/src/GitHub.Api/Application/ApiClient.cs index d5bad7799..d039d228f 100644 --- a/src/GitHub.Api/Application/ApiClient.cs +++ b/src/GitHub.Api/Application/ApiClient.cs @@ -20,17 +20,13 @@ public static IApiClient Create(UriString repositoryUrl, IKeychain keychain) new GitHubClient(AppConfiguration.ProductHeader, credentialStore, hostAddress.ApiUri)); } - private static readonly Unity.ILogging logger = Unity.Logging.GetLogger(); + private static readonly ILogging logger = Logging.GetLogger(); public HostAddress HostAddress { get; } public UriString OriginalUrl { get; } private readonly IKeychain keychain; private readonly IGitHubClient githubClient; private readonly ILoginManager loginManager; - private static readonly SemaphoreSlim sem = new SemaphoreSlim(1); - - IList organizationsCache; - Octokit.User userCache; string owner; bool? isEnterprise; @@ -72,11 +68,10 @@ public async Task CreateRepository(NewRepository newRepository, Action> callback) + public async Task GetOrganizations(Action> onSuccess, Action onError = null) { - Guard.ArgumentNotNull(callback, "callback"); - var organizations = await GetOrganizationInternal(); - callback(organizations); + Guard.ArgumentNotNull(onSuccess, nameof(onSuccess)); + await GetOrganizationInternal(onSuccess, onError); } public async Task LoadKeychain(Action callback) @@ -222,7 +217,7 @@ public async Task ContinueLoginAsync(LoginResult loginResult, Func> GetOrganizationInternal() + private async Task GetOrganizationInternal(Action> onSuccess, Action onError = null) { try { @@ -237,16 +232,14 @@ private async Task> GetOrganizationInternal() if (organizations != null) { - organizationsCache = organizations.ToArray(); + onSuccess(organizations.ToArray()); } } catch(Exception ex) { logger.Error(ex, "Error Getting Organizations"); - throw; + onError?.Invoke(ex); } - - return organizationsCache; } private async Task GetCurrentUserInternal() @@ -254,50 +247,31 @@ private async Task> GetOrganizationInternal() try { logger.Trace("Getting Current User"); + await ValidateKeychain(); - if (!await LoadKeychainInternal()) - { - throw new ApplicationException("Error Loading Keychain"); - } - - userCache = await githubClient.User.Current(); + return await githubClient.User.Current(); } catch (Exception ex) { logger.Error(ex, "Error Getting Current User"); throw; } - - return userCache; } private async Task ValidateCurrentUserInternal() { - Octokit.User apiUsername; - string cachedUsername; - - try - { - logger.Trace("Validating User"); - apiUsername = await GetCurrentUserInternal(); - cachedUsername = keychain.Connections.First().Username; - } - catch (Exception ex) - { - logger.Error(ex, "Error Validating User"); - throw; - } + logger.Trace("Validating User"); + var apiUsername = await GetCurrentUserInternal(); + var cachedUsername = keychain.Connections.First().Username; if (apiUsername.Name != cachedUsername) { - throw new ApplicationException("Credentialed user does not match API"); + throw new TokenUsernameMismatchException(); } } private async Task LoadKeychainInternal() { - logger.Trace("LoadKeychainInternal"); - if (keychain.HasKeys) { if (!keychain.NeedsLoad) @@ -312,6 +286,8 @@ private async Task LoadKeychainInternal() var uriString = keychain.Connections.First().Host; var keychainAdapter = await keychain.Load(uriString); + logger.Trace("LoadKeychainInternal: Loaded"); + return keychainAdapter.OctokitCredentials != Credentials.Anonymous; } @@ -324,8 +300,44 @@ private async Task ValidateKeychain() { if (!await LoadKeychainInternal()) { - throw new ApplicationException("Error Loading Keychain"); + throw new KeychainEmptyException(); } } } + + class ApiClientException : Exception + { + public ApiClientException() + { } + + public ApiClientException(string message) : base(message) + { } + + public ApiClientException(string message, Exception innerException) : base(message, innerException) + { } + } + + class TokenUsernameMismatchException : ApiClientException + { + public TokenUsernameMismatchException() + { } + + public TokenUsernameMismatchException(string message) : base(message) + { } + + public TokenUsernameMismatchException(string message, Exception innerException) : base(message, innerException) + { } + } + + class KeychainEmptyException : ApiClientException + { + public KeychainEmptyException() + { } + + public KeychainEmptyException(string message) : base(message) + { } + + public KeychainEmptyException(string message, Exception innerException) : base(message, innerException) + { } + } } diff --git a/src/GitHub.Api/Application/IApiClient.cs b/src/GitHub.Api/Application/IApiClient.cs index 78b72e280..95c5623ce 100644 --- a/src/GitHub.Api/Application/IApiClient.cs +++ b/src/GitHub.Api/Application/IApiClient.cs @@ -10,7 +10,7 @@ interface IApiClient HostAddress HostAddress { get; } UriString OriginalUrl { get; } Task CreateRepository(NewRepository newRepository, Action callback, string organization = null); - Task GetOrganizations(Action> callback); + Task GetOrganizations(Action> onSuccess, Action onError = null); Task Login(string username, string password, Action need2faCode, Action result); Task ContinueLogin(LoginResult loginResult, string code); Task LoginAsync(string username, string password, Func need2faCode); diff --git a/src/UnityExtension/Assets/Editor/GitHub.Unity/UI/PublishView.cs b/src/UnityExtension/Assets/Editor/GitHub.Unity/UI/PublishView.cs index addb84e63..f84a66858 100644 --- a/src/UnityExtension/Assets/Editor/GitHub.Unity/UI/PublishView.cs +++ b/src/UnityExtension/Assets/Editor/GitHub.Unity/UI/PublishView.cs @@ -122,6 +122,24 @@ private void LoadOrganizations() owners = new[] { OwnersDefaultText, username }.Union(organizationLogins).ToArray(); isBusy = false; + }, exception => { + var tokenUsernameMismatchException = exception as TokenUsernameMismatchException; + if (tokenUsernameMismatchException != null) + { + //This is a specific case + + Logger.Error(exception, "Token Username Mismatch"); + return; + } + + var keychainEmptyException = exception as KeychainEmptyException; + if (keychainEmptyException != null) + { + Logger.Error(exception, "Keychain empty"); + return; + } + + Logger.Error(exception, "Unhandled Exception Type"); }); }); }