From b790d965c9ab27a1fc49b8b42e6fe2f390c2630f Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Fri, 25 Aug 2017 13:29:35 +0200 Subject: [PATCH 1/7] ILoginCache -> IKeychain --- .../{ILoginCache.cs => IKeychain.cs} | 18 +++++----- src/GitHub.Api/LoginManager.cs | 14 ++++---- ...indowsLoginCache.cs => WindowsKeychain.cs} | 12 +++---- src/UnitTests/GitHub.Api/LoginManagerTests.cs | 34 +++++++++---------- 4 files changed, 39 insertions(+), 39 deletions(-) rename src/GitHub.Api/{ILoginCache.cs => IKeychain.cs} (57%) rename src/GitHub.Api/{WindowsLoginCache.cs => WindowsKeychain.cs} (89%) diff --git a/src/GitHub.Api/ILoginCache.cs b/src/GitHub.Api/IKeychain.cs similarity index 57% rename from src/GitHub.Api/ILoginCache.cs rename to src/GitHub.Api/IKeychain.cs index 63da41c5e1..f6170e766b 100644 --- a/src/GitHub.Api/ILoginCache.cs +++ b/src/GitHub.Api/IKeychain.cs @@ -5,33 +5,33 @@ namespace GitHub.Api { /// - /// Stores login details. + /// Represents a keychain used to store credentials. /// - public interface ILoginCache + public interface IKeychain { /// - /// Gets the login details for the specified host address. + /// Loads the credentials for the specified host address. /// /// The host address. /// - /// A task returning a tuple containing the retrieved username and password. + /// A task returning a tuple consisting of the retrieved username and password. /// - Task> GetLogin(HostAddress hostAddress); + Task> Load(HostAddress hostAddress); /// - /// Saves the login details for the specified host address. + /// Saves the credentials for the specified host address. /// /// The username. /// The password. /// The host address. /// A task tracking the operation. - Task SaveLogin(string userName, string password, HostAddress hostAddress); + Task Save(string userName, string password, HostAddress hostAddress); /// - /// Removes the login details for the specified host address. + /// Deletes the login details for the specified host address. /// /// /// A task tracking the operation. - Task EraseLogin(HostAddress hostAddress); + Task Delete(HostAddress hostAddress); } } diff --git a/src/GitHub.Api/LoginManager.cs b/src/GitHub.Api/LoginManager.cs index a647a0e2df..d10e10f3e6 100644 --- a/src/GitHub.Api/LoginManager.cs +++ b/src/GitHub.Api/LoginManager.cs @@ -13,7 +13,7 @@ namespace GitHub.Api public class LoginManager : ILoginManager { readonly string[] scopes = { "user", "repo", "gist", "write:public_key" }; - readonly ILoginCache loginCache; + readonly IKeychain loginCache; readonly ITwoFactorChallengeHandler twoFactorChallengeHandler; readonly string clientId; readonly string clientSecret; @@ -30,7 +30,7 @@ public class LoginManager : ILoginManager /// An note to store with the authorization. /// The machine fingerprint. public LoginManager( - ILoginCache loginCache, + IKeychain loginCache, ITwoFactorChallengeHandler twoFactorChallengeHandler, string clientId, string clientSecret, @@ -64,7 +64,7 @@ public async Task Login( // Start by saving the username and password, these will be used by the `IGitHubClient` // until an authorization token has been created and acquired: - await loginCache.SaveLogin(userName, password, hostAddress).ConfigureAwait(false); + await loginCache.Save(userName, password, hostAddress).ConfigureAwait(false); var newAuth = new NewAuthorization { @@ -99,13 +99,13 @@ public async Task Login( } else { - await loginCache.EraseLogin(hostAddress).ConfigureAwait(false); + await loginCache.Delete(hostAddress).ConfigureAwait(false); throw; } } } while (auth == null); - await loginCache.SaveLogin(userName, auth.Token, hostAddress).ConfigureAwait(false); + await loginCache.Save(userName, auth.Token, hostAddress).ConfigureAwait(false); var retry = 0; @@ -141,7 +141,7 @@ public async Task Logout(HostAddress hostAddress, IGitHubClient client) Guard.ArgumentNotNull(hostAddress, nameof(hostAddress)); Guard.ArgumentNotNull(client, nameof(client)); - await loginCache.EraseLogin(hostAddress); + await loginCache.Delete(hostAddress); } async Task CreateAndDeleteExistingApplicationAuthorization( @@ -219,7 +219,7 @@ async Task HandleTwoFactorAuthorization( catch (Exception e) { await twoFactorChallengeHandler.ChallengeFailed(e); - await loginCache.EraseLogin(hostAddress).ConfigureAwait(false); + await loginCache.Delete(hostAddress).ConfigureAwait(false); throw; } } diff --git a/src/GitHub.Api/WindowsLoginCache.cs b/src/GitHub.Api/WindowsKeychain.cs similarity index 89% rename from src/GitHub.Api/WindowsLoginCache.cs rename to src/GitHub.Api/WindowsKeychain.cs index 118c3c7887..b758ea29ec 100644 --- a/src/GitHub.Api/WindowsLoginCache.cs +++ b/src/GitHub.Api/WindowsKeychain.cs @@ -8,14 +8,14 @@ namespace GitHub.Api { /// - /// A login cache that stores logins in the windows credential cache. + /// A keychain that stores logins in the windows credential store. /// - [Export(typeof(ILoginCache))] + [Export(typeof(IKeychain))] [PartCreationPolicy(CreationPolicy.Shared)] - public class WindowsLoginCache : ILoginCache + public class WindowsKeychain : IKeychain { /// - public Task> GetLogin(HostAddress hostAddress) + public Task> Load(HostAddress hostAddress) { Guard.ArgumentNotNull(hostAddress, nameof(hostAddress)); @@ -33,7 +33,7 @@ public Task> GetLogin(HostAddress hostAddress) } /// - public Task SaveLogin(string userName, string password, HostAddress hostAddress) + public Task Save(string userName, string password, HostAddress hostAddress) { Guard.ArgumentNotEmptyString(userName, nameof(userName)); Guard.ArgumentNotEmptyString(password, nameof(password)); @@ -56,7 +56,7 @@ public Task SaveLogin(string userName, string password, HostAddress hostAddress) } /// - public Task EraseLogin(HostAddress hostAddress) + public Task Delete(HostAddress hostAddress) { Guard.ArgumentNotNull(hostAddress, nameof(hostAddress)); diff --git a/src/UnitTests/GitHub.Api/LoginManagerTests.cs b/src/UnitTests/GitHub.Api/LoginManagerTests.cs index 8d2507442b..b5a5b3fb03 100644 --- a/src/UnitTests/GitHub.Api/LoginManagerTests.cs +++ b/src/UnitTests/GitHub.Api/LoginManagerTests.cs @@ -21,13 +21,13 @@ public async Task LoginTokenIsSavedToCache() client.Authorization.GetOrCreateApplicationAuthentication("id", "secret", Arg.Any()) .Returns(new ApplicationAuthorization("123abc")); - var loginCache = Substitute.For(); + var loginCache = Substitute.For(); var tfa = Substitute.For(); var target = new LoginManager(loginCache, tfa, "id", "secret"); await target.Login(host, client, "foo", "bar"); - await loginCache.Received().SaveLogin("foo", "123abc", host); + await loginCache.Received().Save("foo", "123abc", host); } [Fact] @@ -39,7 +39,7 @@ public async Task LoggedInUserIsReturned() .Returns(new ApplicationAuthorization("123abc")); client.User.Current().Returns(user); - var loginCache = Substitute.For(); + var loginCache = Substitute.For(); var tfa = Substitute.For(); var target = new LoginManager(loginCache, tfa, "id", "secret"); @@ -62,7 +62,7 @@ public async Task DeletesExistingAuthenticationIfNullTokenReturned() new ApplicationAuthorization("123abc")); client.User.Current().Returns(user); - var loginCache = Substitute.For(); + var loginCache = Substitute.For(); var tfa = Substitute.For(); var target = new LoginManager(loginCache, tfa, "id", "secret"); @@ -70,7 +70,7 @@ public async Task DeletesExistingAuthenticationIfNullTokenReturned() await client.Authorization.Received(2).GetOrCreateApplicationAuthentication("id", "secret", Arg.Any()); await client.Authorization.Received(1).Delete(0); - await loginCache.Received().SaveLogin("foo", "123abc", host); + await loginCache.Received().Save("foo", "123abc", host); } [Fact] @@ -84,7 +84,7 @@ public async Task TwoFactorExceptionIsPassedToHandler() client.Authorization.GetOrCreateApplicationAuthentication("id", "secret", Arg.Any(), "123456") .Returns(new ApplicationAuthorization("123abc")); - var loginCache = Substitute.For(); + var loginCache = Substitute.For(); var tfa = Substitute.For(); tfa.HandleTwoFactorException(exception).Returns(new TwoFactorChallengeResult("123456")); @@ -111,7 +111,7 @@ public async Task Failed2FACodeResultsInRetry() client.Authorization.GetOrCreateApplicationAuthentication("id", "secret", Arg.Any(), "123456") .Returns(new ApplicationAuthorization("123abc")); - var loginCache = Substitute.For(); + var loginCache = Substitute.For(); var tfa = Substitute.For(); tfa.HandleTwoFactorException(exception).Returns( new TwoFactorChallengeResult("111111"), @@ -146,7 +146,7 @@ public async Task HandlerNotifiedOfExceptionIn2FAChallengeResponse() client.Authorization.GetOrCreateApplicationAuthentication("id", "secret", Arg.Any(), "111111") .Returns(_ => { throw loginAttemptsException; }); - var loginCache = Substitute.For(); + var loginCache = Substitute.For(); var tfa = Substitute.For(); tfa.HandleTwoFactorException(twoFaException).Returns( new TwoFactorChallengeResult("111111"), @@ -176,7 +176,7 @@ public async Task RequestResendCodeResultsInRetryingLogin() .Returns(new ApplicationAuthorization("456def")); client.User.Current().Returns(user); - var loginCache = Substitute.For(); + var loginCache = Substitute.For(); var tfa = Substitute.For(); tfa.HandleTwoFactorException(exception).Returns( TwoFactorChallengeResult.RequestResendCode, @@ -201,13 +201,13 @@ public async Task UsesUsernameAndPasswordInsteadOfAuthorizationTokenWhenEnterpri }); client.User.Current().Returns(user); - var loginCache = Substitute.For(); + var loginCache = Substitute.For(); var tfa = Substitute.For(); var target = new LoginManager(loginCache, tfa, "id", "secret"); await target.Login(enterprise, client, "foo", "bar"); - await loginCache.Received().SaveLogin("foo", "bar", enterprise); + await loginCache.Received().Save("foo", "bar", enterprise); } [Fact] @@ -219,13 +219,13 @@ public async Task ErasesLoginWhenUnauthorized() client.Authorization.GetOrCreateApplicationAuthentication("id", "secret", Arg.Any()) .Returns(_ => { throw new AuthorizationException(); }); - var loginCache = Substitute.For(); + var loginCache = Substitute.For(); var tfa = Substitute.For(); var target = new LoginManager(loginCache, tfa, "id", "secret"); await Assert.ThrowsAsync(async () => await target.Login(enterprise, client, "foo", "bar")); - await loginCache.Received().EraseLogin(enterprise); + await loginCache.Received().Delete(enterprise); } [Fact] @@ -237,13 +237,13 @@ public async Task ErasesLoginWhenNonOctokitExceptionThrown() client.Authorization.GetOrCreateApplicationAuthentication("id", "secret", Arg.Any()) .Returns(_ => { throw new InvalidOperationException(); }); - var loginCache = Substitute.For(); + var loginCache = Substitute.For(); var tfa = Substitute.For(); var target = new LoginManager(loginCache, tfa, "id", "secret"); await Assert.ThrowsAsync(async () => await target.Login(host, client, "foo", "bar")); - await loginCache.Received().EraseLogin(host); + await loginCache.Received().Delete(host); } [Fact] @@ -259,14 +259,14 @@ public async Task ErasesLoginWhenNonOctokitExceptionThrownIn2FA() .Returns(_ => { throw new InvalidOperationException(); }); client.User.Current().Returns(user); - var loginCache = Substitute.For(); + var loginCache = Substitute.For(); var tfa = Substitute.For(); tfa.HandleTwoFactorException(exception).Returns(new TwoFactorChallengeResult("123456")); var target = new LoginManager(loginCache, tfa, "id", "secret"); await Assert.ThrowsAsync(async () => await target.Login(host, client, "foo", "bar")); - await loginCache.Received().EraseLogin(host); + await loginCache.Received().Delete(host); } } } From 1c811041db07616b07f905cf055f2cd41ba19c47 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Fri, 25 Aug 2017 15:48:59 +0200 Subject: [PATCH 2/7] Use IKeychain for credentials. Previously credentials were read by 4 separate interfaces/classes: - `IKeychain`/`WindowsKeychain` - `ILoginCache`/`LoginCache` - `SimpleCredentialStore` - `CredentialCache` Make everything go via `IKeychain`. --- src/GitHub.Api/GitHub.Api.csproj | 10 +- src/GitHub.Api/IKeychain.cs | 15 +- src/GitHub.Api/KeychainCredentialStore.cs | 32 ++ src/GitHub.Api/LoginManager.cs | 14 +- src/GitHub.Api/SimpleApiClientFactory.cs | 12 +- src/GitHub.Api/SimpleCredentialStore.cs | 83 ----- src/GitHub.Api/WindowsKeychain.cs | 2 +- src/GitHub.App/Caches/CredentialCache.cs | 275 ---------------- ...nCache.cs => ObservableKeychainAdapter.cs} | 37 ++- src/GitHub.App/Caches/SharedCache.cs | 21 +- src/GitHub.App/Factories/ApiClientFactory.cs | 23 +- .../Factories/RepositoryHostFactory.cs | 10 +- src/GitHub.App/GitHub.App.csproj | 4 +- src/GitHub.App/Models/RepositoryHost.cs | 23 +- .../Services/GitHubCredentialProvider.cs | 42 +-- .../Services/GitHubCredentialStore.cs | 44 --- ...Cache.cs => IObservableKeychainAdapter.cs} | 2 +- .../Caches/ISharedCache.cs | 1 - .../GitHub.Exports.Reactive.csproj | 2 +- .../GitHub.Extensions.Reactive.csproj | 2 +- .../GitHub.Extensions.csproj | 2 +- src/GitHub.StartPage/GitHub.StartPage.csproj | 2 +- .../GitHub.TeamFoundation.14.csproj | 2 +- .../GitHub.TeamFoundation.15.csproj | 2 +- .../GitHub.UI.Reactive.csproj | 2 +- src/GitHub.UI/GitHub.UI.csproj | 2 +- src/GitHub.VisualStudio/GitHubPackage.cs | 2 +- .../Services/ConnectionManager.cs | 5 +- .../GitHub.Api/SimpleApiClientFactoryTests.cs | 10 + .../GitHub.App/Caches/CredentialCacheTests.cs | 294 ------------------ .../Services/ConnectionManagerTests.cs | 3 +- src/UnitTests/Helpers/TestLoginCache.cs | 7 +- src/UnitTests/Helpers/TestSharedCache.cs | 2 +- src/UnitTests/UnitTests.csproj | 1 - 34 files changed, 166 insertions(+), 824 deletions(-) create mode 100644 src/GitHub.Api/KeychainCredentialStore.cs delete mode 100644 src/GitHub.Api/SimpleCredentialStore.cs delete mode 100644 src/GitHub.App/Caches/CredentialCache.cs rename src/GitHub.App/Caches/{LoginCache.cs => ObservableKeychainAdapter.cs} (52%) delete mode 100644 src/GitHub.App/Services/GitHubCredentialStore.cs rename src/GitHub.Exports.Reactive/Caches/{ILoginCache.cs => IObservableKeychainAdapter.cs} (86%) delete mode 100644 src/UnitTests/GitHub.App/Caches/CredentialCacheTests.cs diff --git a/src/GitHub.Api/GitHub.Api.csproj b/src/GitHub.Api/GitHub.Api.csproj index dc719d5460..a996c2ddb0 100644 --- a/src/GitHub.Api/GitHub.Api.csproj +++ b/src/GitHub.Api/GitHub.Api.csproj @@ -50,17 +50,17 @@ - + ApiClientConfiguration_User.cs - - + + - - + + Properties\SolutionInfo.cs diff --git a/src/GitHub.Api/IKeychain.cs b/src/GitHub.Api/IKeychain.cs index f6170e766b..88d824c98a 100644 --- a/src/GitHub.Api/IKeychain.cs +++ b/src/GitHub.Api/IKeychain.cs @@ -12,26 +12,27 @@ public interface IKeychain /// /// Loads the credentials for the specified host address. /// - /// The host address. + /// The host address. /// - /// A task returning a tuple consisting of the retrieved username and password. + /// A task returning a tuple consisting of the retrieved username and password or null + /// if the credentials were not found. /// - Task> Load(HostAddress hostAddress); + Task> Load(HostAddress address); /// /// Saves the credentials for the specified host address. /// /// The username. /// The password. - /// The host address. + /// The host address. /// A task tracking the operation. - Task Save(string userName, string password, HostAddress hostAddress); + Task Save(string userName, string password, HostAddress address); /// /// Deletes the login details for the specified host address. /// - /// + /// /// A task tracking the operation. - Task Delete(HostAddress hostAddress); + Task Delete(HostAddress address); } } diff --git a/src/GitHub.Api/KeychainCredentialStore.cs b/src/GitHub.Api/KeychainCredentialStore.cs new file mode 100644 index 0000000000..1a039a52ec --- /dev/null +++ b/src/GitHub.Api/KeychainCredentialStore.cs @@ -0,0 +1,32 @@ +using System; +using System.Threading.Tasks; +using GitHub.Extensions; +using GitHub.Primitives; +using Octokit; + +namespace GitHub.Api +{ + /// + /// An Octokit credential store that reads from an . + /// + public class KeychainCredentialStore : ICredentialStore + { + readonly IKeychain keychain; + readonly HostAddress address; + + public KeychainCredentialStore(IKeychain keychain, HostAddress address) + { + Guard.ArgumentNotNull(keychain, nameof(keychain)); + Guard.ArgumentNotNull(address, nameof(keychain)); + + this.keychain = keychain; + this.address = address; + } + + public async Task GetCredentials() + { + var userPass = await keychain.Load(address).ConfigureAwait(false); + return userPass != null ? new Credentials(userPass.Item1, userPass.Item2) : Credentials.Anonymous; + } + } +} diff --git a/src/GitHub.Api/LoginManager.cs b/src/GitHub.Api/LoginManager.cs index d10e10f3e6..dc20a7a1ec 100644 --- a/src/GitHub.Api/LoginManager.cs +++ b/src/GitHub.Api/LoginManager.cs @@ -13,7 +13,7 @@ namespace GitHub.Api public class LoginManager : ILoginManager { readonly string[] scopes = { "user", "repo", "gist", "write:public_key" }; - readonly IKeychain loginCache; + readonly IKeychain keychain; readonly ITwoFactorChallengeHandler twoFactorChallengeHandler; readonly string clientId; readonly string clientSecret; @@ -42,7 +42,7 @@ public LoginManager( Guard.ArgumentNotEmptyString(clientId, nameof(clientId)); Guard.ArgumentNotEmptyString(clientSecret, nameof(clientSecret)); - this.loginCache = loginCache; + this.keychain = loginCache; this.twoFactorChallengeHandler = twoFactorChallengeHandler; this.clientId = clientId; this.clientSecret = clientSecret; @@ -64,7 +64,7 @@ public async Task Login( // Start by saving the username and password, these will be used by the `IGitHubClient` // until an authorization token has been created and acquired: - await loginCache.Save(userName, password, hostAddress).ConfigureAwait(false); + await keychain.Save(userName, password, hostAddress).ConfigureAwait(false); var newAuth = new NewAuthorization { @@ -99,13 +99,13 @@ public async Task Login( } else { - await loginCache.Delete(hostAddress).ConfigureAwait(false); + await keychain.Delete(hostAddress).ConfigureAwait(false); throw; } } } while (auth == null); - await loginCache.Save(userName, auth.Token, hostAddress).ConfigureAwait(false); + await keychain.Save(userName, auth.Token, hostAddress).ConfigureAwait(false); var retry = 0; @@ -141,7 +141,7 @@ public async Task Logout(HostAddress hostAddress, IGitHubClient client) Guard.ArgumentNotNull(hostAddress, nameof(hostAddress)); Guard.ArgumentNotNull(client, nameof(client)); - await loginCache.Delete(hostAddress); + await keychain.Delete(hostAddress); } async Task CreateAndDeleteExistingApplicationAuthorization( @@ -219,7 +219,7 @@ async Task HandleTwoFactorAuthorization( catch (Exception e) { await twoFactorChallengeHandler.ChallengeFailed(e); - await loginCache.Delete(hostAddress).ConfigureAwait(false); + await keychain.Delete(hostAddress).ConfigureAwait(false); throw; } } diff --git a/src/GitHub.Api/SimpleApiClientFactory.cs b/src/GitHub.Api/SimpleApiClientFactory.cs index 4562e29312..a7795f514b 100644 --- a/src/GitHub.Api/SimpleApiClientFactory.cs +++ b/src/GitHub.Api/SimpleApiClientFactory.cs @@ -1,5 +1,4 @@ using System; -using System.Collections.Generic; using System.ComponentModel.Composition; using GitHub.Models; using GitHub.Primitives; @@ -14,6 +13,7 @@ namespace GitHub.Api [PartCreationPolicy(CreationPolicy.Shared)] public class SimpleApiClientFactory : ISimpleApiClientFactory { + readonly IKeychain keychain; readonly ProductHeaderValue productHeader; readonly Lazy lazyEnterpriseProbe; readonly Lazy lazyWikiProbe; @@ -21,8 +21,13 @@ public class SimpleApiClientFactory : ISimpleApiClientFactory static readonly ConcurrentDictionary cache = new ConcurrentDictionary(); [ImportingConstructor] - public SimpleApiClientFactory(IProgram program, Lazy enterpriseProbe, Lazy wikiProbe) + public SimpleApiClientFactory( + IProgram program, + IKeychain keychain, + Lazy enterpriseProbe, + Lazy wikiProbe) { + this.keychain = keychain; productHeader = program.ProductHeader; lazyEnterpriseProbe = enterpriseProbe; lazyWikiProbe = wikiProbe; @@ -31,8 +36,9 @@ public SimpleApiClientFactory(IProgram program, Lazy enter public Task Create(UriString repositoryUrl) { var hostAddress = HostAddress.Create(repositoryUrl); + var credentialStore = new KeychainCredentialStore(keychain, hostAddress); var result = cache.GetOrAdd(repositoryUrl, new SimpleApiClient(repositoryUrl, - new GitHubClient(productHeader, new SimpleCredentialStore(hostAddress), hostAddress.ApiUri), + new GitHubClient(productHeader, credentialStore, hostAddress.ApiUri), lazyEnterpriseProbe, lazyWikiProbe)); return Task.FromResult(result); } diff --git a/src/GitHub.Api/SimpleCredentialStore.cs b/src/GitHub.Api/SimpleCredentialStore.cs deleted file mode 100644 index a50e080152..0000000000 --- a/src/GitHub.Api/SimpleCredentialStore.cs +++ /dev/null @@ -1,83 +0,0 @@ -using System.Threading.Tasks; -using GitHub.Primitives; -using Octokit; -using System; -using GitHub.Authentication.CredentialManagement; - -namespace GitHub.Api -{ - public class SimpleCredentialStore : ICredentialStore - { - readonly HostAddress hostAddress; - - public SimpleCredentialStore(HostAddress hostAddress) - { - this.hostAddress = hostAddress; - } - - public Task GetCredentials() - { - var keyHost = GetKeyHost(hostAddress.CredentialCacheKeyHost); - using (var credential = new Credential()) - { - credential.Target = keyHost; - credential.Type = CredentialType.Generic; - if (credential.Load()) - return Task.FromResult(new Credentials(credential.Username, credential.Password)); - } - return Task.FromResult(Credentials.Anonymous); - } - - public static Task RemoveCredentials(string key) - { - var keyGit = GetKeyGit(key); - if (!DeleteKey(keyGit)) - return Task.FromResult(false); - - var keyHost = GetKeyHost(key); - if (!DeleteKey(keyHost)) - return Task.FromResult(false); - - return Task.FromResult(true); - } - - static bool DeleteKey(string key) - { - using (var credential = new Credential()) - { - credential.Target = key; - if (!credential.Load()) - return false; - return credential.Delete(); - } - } - - static string GetKeyHost(string key) - { - key = FormatKey(key); - if (key.StartsWith("git:", StringComparison.Ordinal)) - key = key.Substring("git:".Length); - if (!key.EndsWith("/", StringComparison.Ordinal)) - key += '/'; - return key; - } - - static string FormatKey(string key) - { - if (key.StartsWith("login:", StringComparison.Ordinal)) - key = key.Substring("login:".Length); - return key; - } - - static string GetKeyGit(string key) - { - key = FormatKey(key); - // it appears this is how MS expects the host key - if (!key.StartsWith("git:", StringComparison.Ordinal)) - key = "git:" + key; - if (key.EndsWith("/", StringComparison.Ordinal)) - key = key.Substring(0, key.Length - 1); - return key; - } - } -} diff --git a/src/GitHub.Api/WindowsKeychain.cs b/src/GitHub.Api/WindowsKeychain.cs index b758ea29ec..5597831f03 100644 --- a/src/GitHub.Api/WindowsKeychain.cs +++ b/src/GitHub.Api/WindowsKeychain.cs @@ -29,7 +29,7 @@ public Task> Load(HostAddress hostAddress) return Task.FromResult(Tuple.Create(credential.Username, credential.Password)); } - return Task.FromResult(Tuple.Create(null, null)); + return Task.FromResult>(null); } /// diff --git a/src/GitHub.App/Caches/CredentialCache.cs b/src/GitHub.App/Caches/CredentialCache.cs deleted file mode 100644 index 9d51d4fab1..0000000000 --- a/src/GitHub.App/Caches/CredentialCache.cs +++ /dev/null @@ -1,275 +0,0 @@ -using System; -using System.Collections.Generic; -using System.Reactive; -using System.Reactive.Concurrency; -using System.Reactive.Linq; -using System.Reactive.Subjects; -using System.Text; -using Akavache; -using GitHub.Helpers; -using GitHub.Authentication.CredentialManagement; -using System.Security; - -namespace GitHub.Caches -{ - public class CredentialCache : ISecureBlobCache, IObjectBlobCache - { - public IScheduler Scheduler { get; protected set; } - - readonly AsyncSubject shutdown = new AsyncSubject(); - public IObservable Shutdown => shutdown; - - public IObservable Flush() - { - if (disposed) return ExceptionHelper.ObservableThrowObjectDisposedException("CredentialCache"); - return Observable.Return(Unit.Default); - } - - public IObservable Get(string key) - { - if (disposed) return ExceptionHelper.ObservableThrowObjectDisposedException("CredentialCache"); - - var keyHost = GetKeyHost(key); - using (var credential = new Credential()) - { - credential.Target = keyHost; - if (credential.Load()) - return Observable.Return(Encoding.Unicode.GetBytes(credential.Password)); - } - return ExceptionHelper.ObservableThrowKeyNotFoundException(key); - } - - public IObservable> GetAllKeys() - { - throw new NotImplementedException(); - } - - public IObservable GetCreatedAt(string key) - { - throw new NotImplementedException(); - } - - public IObservable Insert(string key, byte[] data, DateTimeOffset? absoluteExpiration = default(DateTimeOffset?)) - { - return ExceptionHelper.ObservableThrowInvalidOperationException(key); - } - - public IObservable Invalidate(string key) - { - if (disposed) return ExceptionHelper.ObservableThrowObjectDisposedException("CredentialCache"); - - var keyGit = GetKeyGit(key); - if (!DeleteKey(keyGit)) - return ExceptionHelper.ObservableThrowKeyNotFoundException(keyGit); - - var keyHost = GetKeyHost(key); - if (!DeleteKey(keyHost)) - return ExceptionHelper.ObservableThrowKeyNotFoundException(keyHost); - - return Observable.Return(Unit.Default); - } - - public IObservable InvalidateAll() - { - throw new NotImplementedException(); - } - - public IObservable Vacuum() - { - throw new NotImplementedException(); - } - - public IObservable InsertObject(string key, T value, DateTimeOffset? absoluteExpiration = default(DateTimeOffset?)) - { - if (disposed) return ExceptionHelper.ObservableThrowObjectDisposedException("CredentialCache"); - - if (value is Tuple) - return InsertTuple(key, value as Tuple); - if (value is Tuple) - return InsertTuple(key, value as Tuple); - - return ExceptionHelper.ObservableThrowInvalidOperationException(key); - } - - static IObservable InsertTuple(string key, Tuple values) - { - var keyGit = GetKeyGit(key); - if (!SaveKey(keyGit, values.Item1, values.Item2)) - return ExceptionHelper.ObservableThrowInvalidOperationException(keyGit); - - var keyHost = GetKeyHost(key); - if (!SaveKey(keyHost, values.Item1, values.Item2)) - return ExceptionHelper.ObservableThrowInvalidOperationException(keyGit); - - return Observable.Return(Unit.Default); - } - - static IObservable InsertTuple(string key, Tuple values) - { - var keyGit = GetKeyGit(key); - if (!SaveKey(keyGit, values.Item1, values.Item2)) - return ExceptionHelper.ObservableThrowInvalidOperationException(keyGit); - - var keyHost = GetKeyHost(key); - if (!SaveKey(keyHost, values.Item1, values.Item2)) - return ExceptionHelper.ObservableThrowInvalidOperationException(keyGit); - - return Observable.Return(Unit.Default); - } - - public IObservable GetObject(string key) - { - if (typeof(T) == typeof(Tuple)) - return (IObservable) GetTuple(key); - if (typeof(T) == typeof(Tuple)) - return (IObservable)GetSecureTuple(key); - return ExceptionHelper.ObservableThrowInvalidOperationException(key); - } - - IObservable> GetTuple(string key) - { - if (disposed) return ExceptionHelper.ObservableThrowObjectDisposedException>("CredentialCache"); - - var keyHost = GetKeyHost(key); - var ret = GetKey(keyHost); - return ret != null - ? Observable.Return(ret) - : ExceptionHelper.ObservableThrowKeyNotFoundException>(keyHost); - } - - IObservable> GetSecureTuple(string key) - { - if (disposed) return ExceptionHelper.ObservableThrowObjectDisposedException>("CredentialCache"); - - var keyHost = GetKeyHost(key); - var ret = GetSecureKey(keyHost); - return ret != null - ? Observable.Return(ret) - : ExceptionHelper.ObservableThrowKeyNotFoundException>(keyHost); - } - - public IObservable> GetAllObjects() - { - throw new NotImplementedException(); - } - - public IObservable GetObjectCreatedAt(string key) - { - throw new NotImplementedException(); - } - - public IObservable InvalidateObject(string key) - { - if (disposed) return ExceptionHelper.ObservableThrowObjectDisposedException("CredentialCache"); - - var keyGit = GetKeyGit(key); - if (!DeleteKey(keyGit)) - return ExceptionHelper.ObservableThrowKeyNotFoundException(keyGit); - - var keyHost = GetKeyHost(key); - if (!DeleteKey(keyHost)) - return ExceptionHelper.ObservableThrowKeyNotFoundException(key); - - return Observable.Return(Unit.Default); - } - - public IObservable InvalidateAllObjects() - { - throw new NotImplementedException(); - } - - static string FormatKey(string key) - { - if (key.StartsWith("login:", StringComparison.Ordinal)) - key = key.Substring("login:".Length); - return key; - } - - static string GetKeyGit(string key) - { - key = FormatKey(key); - // it appears this is how MS expects the host key - if (!key.StartsWith("git:", StringComparison.Ordinal)) - key = "git:" + key; - if (key.EndsWith("/", StringComparison.Ordinal)) - key = key.Substring(0, key.Length - 1); - return key; - } - - static string GetKeyHost(string key) - { - key = FormatKey(key); - if (key.StartsWith("git:", StringComparison.Ordinal)) - key = key.Substring("git:".Length); - if (!key.EndsWith("/", StringComparison.Ordinal)) - key += '/'; - return key; - } - - static bool DeleteKey(string key) - { - using (var credential = new Credential()) - { - credential.Target = key; - return credential.Load() && credential.Delete(); - } - } - - static bool SaveKey(string key, string user, string pwd) - { - using (var credential = new Credential(user, pwd, key)) - { - return credential.Save(); - } - } - - static bool SaveKey(string key, string user, SecureString pwd) - { - using (var credential = new Credential(user, pwd, key)) - { - return credential.Save(); - } - } - - static Tuple GetKey(string key) - { - using (var credential = new Credential()) - { - credential.Target = key; - return credential.Load() - ? new Tuple(credential.Username, credential.Password) - : null; - } - } - - static Tuple GetSecureKey(string key) - { - using (var credential = new Credential()) - { - credential.Target = key; - return credential.Load() - ? new Tuple(credential.Username, credential.SecurePassword) - : null; - } - } - - bool disposed; - void Dispose(bool disposing) - { - if (disposing) - { - if (disposed) return; - - Scheduler = null; - shutdown.OnNext(Unit.Default); - shutdown.OnCompleted(); - disposed = true; - } - } - public void Dispose() - { - Dispose(true); - GC.SuppressFinalize(this); - } - } -} diff --git a/src/GitHub.App/Caches/LoginCache.cs b/src/GitHub.App/Caches/ObservableKeychainAdapter.cs similarity index 52% rename from src/GitHub.App/Caches/LoginCache.cs rename to src/GitHub.App/Caches/ObservableKeychainAdapter.cs index eec74552b3..36695869e0 100644 --- a/src/GitHub.App/Caches/LoginCache.cs +++ b/src/GitHub.App/Caches/ObservableKeychainAdapter.cs @@ -3,26 +3,37 @@ using System.Globalization; using System.Reactive; using System.Reactive.Linq; +using System.Reactive.Threading.Tasks; using Akavache; +using GitHub.Api; using GitHub.Extensions; +using GitHub.Models; using GitHub.Primitives; using NLog; namespace GitHub.Caches { - [Export(typeof(ILoginCache))] + /// + /// An observable wrapper around a . + /// + /// + /// There are some older classes such as that want an observable + /// interface for reading credentials. They should be rewritten to use `Task`s but for the + /// moment this class serves as an adapter for those clients. + /// + [Export(typeof(IObservableKeychainAdapter))] [PartCreationPolicy(CreationPolicy.Shared)] - public sealed class LoginCache : ILoginCache + public sealed class ObservableKeychainAdapter : IObservableKeychainAdapter { static readonly Logger log = LogManager.GetCurrentClassLogger(); - readonly ISharedCache cache; + readonly IKeychain keychain; static readonly LoginInfo empty = new LoginInfo("", ""); [ImportingConstructor] - public LoginCache(ISharedCache cache) + public ObservableKeychainAdapter(IKeychain keychain) { - this.cache = cache; + this.keychain = keychain; } public static LoginInfo EmptyLoginInfo @@ -32,8 +43,9 @@ public static LoginInfo EmptyLoginInfo public IObservable GetLoginAsync(HostAddress hostAddress) { - return cache.Secure.GetLoginAsync(hostAddress.CredentialCacheKeyHost) - .Catch(e => Observable.Return(empty)); + return keychain.Load(hostAddress) + .ToObservable() + .Select(x => new LoginInfo(x.Item1, x.Item2)); } public IObservable SaveLogin(string user, string password, HostAddress hostAddress) @@ -41,22 +53,19 @@ public IObservable SaveLogin(string user, string password, HostAddress hos Guard.ArgumentNotEmptyString(user, nameof(user)); Guard.ArgumentNotEmptyString(password, nameof(password)); - return cache.Secure.SaveLogin(user, password, hostAddress.CredentialCacheKeyHost); + return keychain.Save(user, password, hostAddress).ToObservable(); } public IObservable EraseLogin(HostAddress hostAddress) { log.Info(CultureInfo.CurrentCulture, "Erasing the git credential cache for host '{0}'", hostAddress.CredentialCacheKeyHost); - return cache.Secure.EraseLogin(hostAddress.CredentialCacheKeyHost); - } - public IObservable Flush() - { - log.Info("Flushing the login cache"); - return cache.Secure.Flush(); + return keychain.Delete(hostAddress).ToObservable(); } + public IObservable Flush() => Observable.Return(Unit.Default); + public void Dispose() {} } diff --git a/src/GitHub.App/Caches/SharedCache.cs b/src/GitHub.App/Caches/SharedCache.cs index 51c2fe0a97..3d72120e39 100644 --- a/src/GitHub.App/Caches/SharedCache.cs +++ b/src/GitHub.App/Caches/SharedCache.cs @@ -30,33 +30,18 @@ static SharedCache() } } - public SharedCache() : this(null, null, null) + public SharedCache() : this(null, null) { } - protected SharedCache(IBlobCache userAccountCache, IBlobCache localMachineCache, ISecureBlobCache secureCache) + protected SharedCache(IBlobCache userAccountCache, IBlobCache localMachineCache) { - if (secureCache == null) - { - try - { - BlobCache.Secure = new CredentialCache(); - secureCache = BlobCache.Secure; - } - catch (Exception e) - { - log.Error("Failed to set up secure cache.", e); - secureCache = new InMemoryBlobCache(); - } - } UserAccount = userAccountCache ?? GetBlobCacheWithFallback(() => BlobCache.UserAccount, "UserAccount"); LocalMachine = localMachineCache ?? GetBlobCacheWithFallback(() => BlobCache.LocalMachine, "LocalMachine"); - Secure = secureCache; } public IBlobCache UserAccount { get; private set; } public IBlobCache LocalMachine { get; private set; } - public ISecureBlobCache Secure { get; private set; } public IObservable GetEnterpriseHostUri() { @@ -84,8 +69,6 @@ protected virtual void Dispose(bool disposing) UserAccount.Shutdown.Wait(); LocalMachine.Dispose(); LocalMachine.Shutdown.Wait(); - Secure.Dispose(); - Secure.Shutdown.Wait(); disposed = true; } } diff --git a/src/GitHub.App/Factories/ApiClientFactory.cs b/src/GitHub.App/Factories/ApiClientFactory.cs index 4f904be737..2028f3dba5 100644 --- a/src/GitHub.App/Factories/ApiClientFactory.cs +++ b/src/GitHub.App/Factories/ApiClientFactory.cs @@ -1,15 +1,13 @@ -using System.ComponentModel.Composition; +using System; +using System.ComponentModel.Composition; +using System.Threading.Tasks; using GitHub.Api; -using GitHub.Caches; +using GitHub.Infrastructure; using GitHub.Models; using GitHub.Primitives; -using GitHub.Services; using Octokit; using Octokit.Reactive; using ApiClient = GitHub.Api.ApiClient; -using GitHub.Infrastructure; -using System.Threading.Tasks; -using ILoginCache = GitHub.Caches.ILoginCache; namespace GitHub.Factories { @@ -20,19 +18,18 @@ public class ApiClientFactory : IApiClientFactory readonly ProductHeaderValue productHeader; [ImportingConstructor] - public ApiClientFactory(ILoginCache loginCache, IProgram program, ILoggingConfiguration config) + public ApiClientFactory(IKeychain keychain, IProgram program, ILoggingConfiguration config) { - LoginCache = loginCache; + Keychain = keychain; productHeader = program.ProductHeader; config.Configure(); } public Task CreateGitHubClient(HostAddress hostAddress) { - return Task.FromResult(new GitHubClient( - productHeader, - new GitHubCredentialStore(hostAddress, LoginCache), - hostAddress.ApiUri)); + var credentialStore = new KeychainCredentialStore(Keychain, hostAddress); + var result = new GitHubClient(productHeader, credentialStore, hostAddress.ApiUri); + return Task.FromResult(result); } public async Task Create(HostAddress hostAddress) @@ -42,6 +39,6 @@ public async Task Create(HostAddress hostAddress) new ObservableGitHubClient(await CreateGitHubClient(hostAddress))); } - protected ILoginCache LoginCache { get; private set; } + protected IKeychain Keychain { get; private set; } } } diff --git a/src/GitHub.App/Factories/RepositoryHostFactory.cs b/src/GitHub.App/Factories/RepositoryHostFactory.cs index 80235cda5a..a3c7d13fb1 100644 --- a/src/GitHub.App/Factories/RepositoryHostFactory.cs +++ b/src/GitHub.App/Factories/RepositoryHostFactory.cs @@ -8,7 +8,7 @@ using System.Reactive.Disposables; using System.Threading.Tasks; using GitHub.Api; -using ILoginCache = GitHub.Caches.ILoginCache; +using IObservableKeychainAdapter = GitHub.Caches.IObservableKeychainAdapter; namespace GitHub.Factories { @@ -19,7 +19,7 @@ public class RepositoryHostFactory : IRepositoryHostFactory readonly IApiClientFactory apiClientFactory; readonly IHostCacheFactory hostCacheFactory; readonly ILoginManager loginManager; - readonly ILoginCache loginCache; + readonly IObservableKeychainAdapter keychain; readonly IAvatarProvider avatarProvider; readonly CompositeDisposable hosts = new CompositeDisposable(); readonly IUsageTracker usage; @@ -29,14 +29,14 @@ public RepositoryHostFactory( IApiClientFactory apiClientFactory, IHostCacheFactory hostCacheFactory, ILoginManager loginManager, - ILoginCache loginCache, + IObservableKeychainAdapter keychain, IAvatarProvider avatarProvider, IUsageTracker usage) { this.apiClientFactory = apiClientFactory; this.hostCacheFactory = hostCacheFactory; this.loginManager = loginManager; - this.loginCache = loginCache; + this.keychain = keychain; this.avatarProvider = avatarProvider; this.usage = usage; } @@ -46,7 +46,7 @@ public async Task Create(HostAddress hostAddress) var apiClient = await apiClientFactory.Create(hostAddress); var hostCache = await hostCacheFactory.Create(hostAddress); var modelService = new ModelService(apiClient, hostCache, avatarProvider); - var host = new RepositoryHost(apiClient, modelService, loginManager, loginCache, usage); + var host = new RepositoryHost(apiClient, modelService, loginManager, keychain, usage); hosts.Add(host); return host; } diff --git a/src/GitHub.App/GitHub.App.csproj b/src/GitHub.App/GitHub.App.csproj index 2465b5148f..c23f684ee6 100644 --- a/src/GitHub.App/GitHub.App.csproj +++ b/src/GitHub.App/GitHub.App.csproj @@ -165,7 +165,6 @@ - Properties\SolutionInfo.cs @@ -178,7 +177,7 @@ - + @@ -196,7 +195,6 @@ - diff --git a/src/GitHub.App/Models/RepositoryHost.cs b/src/GitHub.App/Models/RepositoryHost.cs index f5a6a39a08..f1e357c35a 100644 --- a/src/GitHub.App/Models/RepositoryHost.cs +++ b/src/GitHub.App/Models/RepositoryHost.cs @@ -2,24 +2,21 @@ using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Globalization; -using System.Net; +using System.Linq; using System.Reactive; using System.Reactive.Linq; +using System.Reactive.Threading.Tasks; +using System.Threading.Tasks; using GitHub.Api; using GitHub.Authentication; using GitHub.Caches; -using GitHub.Extensions.Reactive; +using GitHub.Extensions; using GitHub.Primitives; using GitHub.Services; using NLog; using Octokit; using ReactiveUI; -using System.Linq; -using System.Reactive.Threading.Tasks; -using System.Collections.Generic; -using GitHub.Extensions; -using ILoginCache = GitHub.Caches.ILoginCache; -using System.Threading.Tasks; +using IObservableKeychainAdapter = GitHub.Caches.IObservableKeychainAdapter; namespace GitHub.Models { @@ -31,7 +28,7 @@ public class RepositoryHost : ReactiveObject, IRepositoryHost readonly ILoginManager loginManager; readonly HostAddress hostAddress; - readonly ILoginCache loginCache; + readonly IObservableKeychainAdapter keychain; readonly IUsageTracker usage; bool isLoggedIn; @@ -40,13 +37,13 @@ public RepositoryHost( IApiClient apiClient, IModelService modelService, ILoginManager loginManager, - ILoginCache loginCache, + IObservableKeychainAdapter keychain, IUsageTracker usage) { ApiClient = apiClient; ModelService = modelService; this.loginManager = loginManager; - this.loginCache = loginCache; + this.keychain = keychain; this.usage = usage; Debug.Assert(apiClient.HostAddress != null, "HostAddress of an api client shouldn't be null"); @@ -130,7 +127,7 @@ public IObservable LogOut() log.Info(CultureInfo.InvariantCulture, "Logged off of host '{0}'", hostAddress.ApiUri); - return loginCache.EraseLogin(Address) + return keychain.EraseLogin(Address) .Catch(e => { log.Warn("ASSERT! Failed to erase login. Going to invalidate cache anyways.", e); @@ -171,7 +168,7 @@ IObservable LoginWithApiUser(UserAndScopes userAndScopes) if (result == AuthenticationResult.VerificationFailure) { - return loginCache.EraseLogin(Address).Select(_ => result); + return keychain.EraseLogin(Address).Select(_ => result); } return Observable.Return(result); }) diff --git a/src/GitHub.App/Services/GitHubCredentialProvider.cs b/src/GitHub.App/Services/GitHubCredentialProvider.cs index 97b0b71fa1..03a1e4bf18 100644 --- a/src/GitHub.App/Services/GitHubCredentialProvider.cs +++ b/src/GitHub.App/Services/GitHubCredentialProvider.cs @@ -1,12 +1,11 @@ using System; using System.ComponentModel.Composition; -using System.Reactive.Linq; -using System.Security; -using Akavache; -using GitHub.Caches; +using GitHub.Api; using GitHub.Extensions; using GitHub.Primitives; using LibGit2Sharp; +using Microsoft.VisualStudio.Shell; +using NLog; namespace GitHub.Services { @@ -14,14 +13,15 @@ namespace GitHub.Services [PartCreationPolicy(CreationPolicy.Shared)] class GitHubCredentialProvider : IGitHubCredentialProvider { - readonly ISecureBlobCache secureCache = null; + static readonly Logger log = LogManager.GetCurrentClassLogger(); + readonly IKeychain keychain; [ImportingConstructor] - public GitHubCredentialProvider(ISharedCache sharedCache) + public GitHubCredentialProvider(IKeychain keychain) { - Guard.ArgumentNotNull(sharedCache, nameof(sharedCache)); + Guard.ArgumentNotNull(keychain, nameof(keychain)); - secureCache = sharedCache.Secure; + this.keychain = keychain; } /// @@ -34,21 +34,21 @@ public Credentials HandleCredentials(string url, string username, SupportedCrede return null; // wondering if we should return DefaultCredentials instead var host = HostAddress.Create(url); - return secureCache.GetObject>("login:" + host.CredentialCacheKeyHost) - .Select(CreateCredentials) - .Catch(Observable.Return(null)) - .Wait(); - } - - static Credentials CreateCredentials(Tuple data) - { - Guard.ArgumentNotNull(data, nameof(data)); - return new SecureUsernamePasswordCredentials + try + { + var credentials = ThreadHelper.JoinableTaskFactory.Run(async () => await keychain.Load(host)); + return new UsernamePasswordCredentials + { + Username = credentials.Item1, + Password = credentials.Item2, + }; + } + catch (Exception e) { - Username = data.Item1, - Password = data.Item2 - }; + log.Error("Error loading credentials in GitHubCredentialProvider.", e); + return null; + } } } } \ No newline at end of file diff --git a/src/GitHub.App/Services/GitHubCredentialStore.cs b/src/GitHub.App/Services/GitHubCredentialStore.cs deleted file mode 100644 index 4bee84389f..0000000000 --- a/src/GitHub.App/Services/GitHubCredentialStore.cs +++ /dev/null @@ -1,44 +0,0 @@ -using System.Globalization; -using System.Reactive.Linq; -using System.Threading.Tasks; -using Akavache; -using GitHub.Caches; -using GitHub.Extensions.Reactive; -using GitHub.Primitives; -using NLog; -using Octokit; - -namespace GitHub.Services -{ - public class GitHubCredentialStore : ICredentialStore - { - static readonly Logger log = LogManager.GetCurrentClassLogger(); - readonly HostAddress hostAddress; - readonly ILoginCache loginCache; - - public GitHubCredentialStore(HostAddress hostAddress, ILoginCache loginCache) - { - this.hostAddress = hostAddress; - this.loginCache = loginCache; - } - - public async Task GetCredentials() - { - return await loginCache.GetLoginAsync(hostAddress) - .CatchNonCritical(Observable.Return(LoginCache.EmptyLoginInfo)) - .Select(CreateFromLoginInfo); - } - - Credentials CreateFromLoginInfo(LoginInfo loginInfo) - { - if (loginInfo == LoginCache.EmptyLoginInfo) - { - log.Debug(CultureInfo.InvariantCulture, "Could not retrieve login info from the secure cache '{0}'", - hostAddress.CredentialCacheKeyHost); - return Credentials.Anonymous; - } - - return new Credentials(loginInfo.UserName, loginInfo.Password); - } - } -} diff --git a/src/GitHub.Exports.Reactive/Caches/ILoginCache.cs b/src/GitHub.Exports.Reactive/Caches/IObservableKeychainAdapter.cs similarity index 86% rename from src/GitHub.Exports.Reactive/Caches/ILoginCache.cs rename to src/GitHub.Exports.Reactive/Caches/IObservableKeychainAdapter.cs index aecaa03a3b..b0630345ef 100644 --- a/src/GitHub.Exports.Reactive/Caches/ILoginCache.cs +++ b/src/GitHub.Exports.Reactive/Caches/IObservableKeychainAdapter.cs @@ -5,7 +5,7 @@ namespace GitHub.Caches { - public interface ILoginCache : IDisposable + public interface IObservableKeychainAdapter : IDisposable { IObservable GetLoginAsync(HostAddress hostAddress); IObservable SaveLogin(string user, string password, HostAddress hostAddress); diff --git a/src/GitHub.Exports.Reactive/Caches/ISharedCache.cs b/src/GitHub.Exports.Reactive/Caches/ISharedCache.cs index 06ed9b386c..1df074ca27 100644 --- a/src/GitHub.Exports.Reactive/Caches/ISharedCache.cs +++ b/src/GitHub.Exports.Reactive/Caches/ISharedCache.cs @@ -12,7 +12,6 @@ public interface ISharedCache : IDisposable { IBlobCache UserAccount { get; } IBlobCache LocalMachine { get; } - ISecureBlobCache Secure { get; } /// /// Retrieves the Enterpise Host Uri from cache if present. diff --git a/src/GitHub.Exports.Reactive/GitHub.Exports.Reactive.csproj b/src/GitHub.Exports.Reactive/GitHub.Exports.Reactive.csproj index a9196683af..44ab2da0a5 100644 --- a/src/GitHub.Exports.Reactive/GitHub.Exports.Reactive.csproj +++ b/src/GitHub.Exports.Reactive/GitHub.Exports.Reactive.csproj @@ -120,7 +120,7 @@ - + diff --git a/src/GitHub.Extensions.Reactive/GitHub.Extensions.Reactive.csproj b/src/GitHub.Extensions.Reactive/GitHub.Extensions.Reactive.csproj index 93d2177a96..fde2e371ae 100644 --- a/src/GitHub.Extensions.Reactive/GitHub.Extensions.Reactive.csproj +++ b/src/GitHub.Extensions.Reactive/GitHub.Extensions.Reactive.csproj @@ -43,7 +43,7 @@ true ..\common\GitHubVS.ruleset - + diff --git a/src/GitHub.Extensions/GitHub.Extensions.csproj b/src/GitHub.Extensions/GitHub.Extensions.csproj index 266f935cb0..1696f0c55b 100644 --- a/src/GitHub.Extensions/GitHub.Extensions.csproj +++ b/src/GitHub.Extensions/GitHub.Extensions.csproj @@ -43,7 +43,7 @@ true ..\common\GitHubVS.ruleset - + ..\..\packages\Microsoft.VisualStudio.Shell.Immutable.10.0.10.0.30319\lib\net40\Microsoft.VisualStudio.Shell.Immutable.10.0.dll diff --git a/src/GitHub.StartPage/GitHub.StartPage.csproj b/src/GitHub.StartPage/GitHub.StartPage.csproj index 27025e3d32..35b412f77c 100644 --- a/src/GitHub.StartPage/GitHub.StartPage.csproj +++ b/src/GitHub.StartPage/GitHub.StartPage.csproj @@ -11,7 +11,7 @@ - + Debug diff --git a/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj b/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj index fd4d585e78..a07a1792df 100644 --- a/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj +++ b/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj @@ -34,7 +34,7 @@ prompt 4 - + ..\..\packages\LibGit2Sharp.0.22.0\lib\net40\LibGit2Sharp.dll diff --git a/src/GitHub.TeamFoundation.15/GitHub.TeamFoundation.15.csproj b/src/GitHub.TeamFoundation.15/GitHub.TeamFoundation.15.csproj index 666ca65c6a..ed4e24a6af 100644 --- a/src/GitHub.TeamFoundation.15/GitHub.TeamFoundation.15.csproj +++ b/src/GitHub.TeamFoundation.15/GitHub.TeamFoundation.15.csproj @@ -34,7 +34,7 @@ prompt 4 - + ..\..\packages\LibGit2Sharp.0.22.0\lib\net40\LibGit2Sharp.dll diff --git a/src/GitHub.UI.Reactive/GitHub.UI.Reactive.csproj b/src/GitHub.UI.Reactive/GitHub.UI.Reactive.csproj index b33bea5dcb..bdb44f975d 100644 --- a/src/GitHub.UI.Reactive/GitHub.UI.Reactive.csproj +++ b/src/GitHub.UI.Reactive/GitHub.UI.Reactive.csproj @@ -44,7 +44,7 @@ true ..\common\GitHubVS.ruleset - + diff --git a/src/GitHub.UI/GitHub.UI.csproj b/src/GitHub.UI/GitHub.UI.csproj index 41d1d43933..f6db923466 100644 --- a/src/GitHub.UI/GitHub.UI.csproj +++ b/src/GitHub.UI/GitHub.UI.csproj @@ -40,7 +40,7 @@ true ..\common\GitHubVS.ruleset - + ..\..\packages\Expression.Blend.Sdk.WPF.1.0.1\lib\net45\Microsoft.Expression.Interactions.dll diff --git a/src/GitHub.VisualStudio/GitHubPackage.cs b/src/GitHub.VisualStudio/GitHubPackage.cs index ff9e53b274..9757861155 100644 --- a/src/GitHub.VisualStudio/GitHubPackage.cs +++ b/src/GitHub.VisualStudio/GitHubPackage.cs @@ -198,7 +198,7 @@ async Task CreateService(IAsyncServiceContainer container, CancellationT else if (serviceType == typeof(ILoginManager)) { var serviceProvider = await GetServiceAsync(typeof(IGitHubServiceProvider)) as IGitHubServiceProvider; - var loginCache = serviceProvider.GetService(); + var loginCache = serviceProvider.GetService(); var twoFaHandler = serviceProvider.GetService(); return new LoginManager( diff --git a/src/GitHub.VisualStudio/Services/ConnectionManager.cs b/src/GitHub.VisualStudio/Services/ConnectionManager.cs index c5618f6a79..08498332bf 100644 --- a/src/GitHub.VisualStudio/Services/ConnectionManager.cs +++ b/src/GitHub.VisualStudio/Services/ConnectionManager.cs @@ -19,6 +19,7 @@ public class ConnectionManager : IConnectionManager { readonly IVSGitServices vsGitServices; readonly IConnectionCache cache; + readonly IKeychain keychain; readonly ILoginManager loginManager; readonly IApiClientFactory apiClientFactory; @@ -28,11 +29,13 @@ public class ConnectionManager : IConnectionManager public ConnectionManager( IVSGitServices vsGitServices, IConnectionCache cache, + IKeychain keychain, ILoginManager loginManager, IApiClientFactory apiClientFactory) { this.vsGitServices = vsGitServices; this.cache = cache; + this.keychain = keychain; this.loginManager = loginManager; this.apiClientFactory = apiClientFactory; @@ -111,7 +114,7 @@ void RefreshConnections(object sender, System.Collections.Specialized.NotifyColl { // RepositoryHosts hasn't been loaded so it can't handle logging out, we have to do it ourselves if (DoLogin == null) - Api.SimpleCredentialStore.RemoveCredentials(c.HostAddress.CredentialCacheKeyHost); + keychain.Delete(c.HostAddress).Forget(); c.Dispose(); } } diff --git a/src/UnitTests/GitHub.Api/SimpleApiClientFactoryTests.cs b/src/UnitTests/GitHub.Api/SimpleApiClientFactoryTests.cs index cdb617a9a5..43d0bfda84 100644 --- a/src/UnitTests/GitHub.Api/SimpleApiClientFactoryTests.cs +++ b/src/UnitTests/GitHub.Api/SimpleApiClientFactoryTests.cs @@ -16,10 +16,12 @@ public async Task CreatesNewInstanceOfSimpleApiClient() { const string url = "https://github.com/github/CreatesNewInstanceOfSimpleApiClient"; var program = new Program(); + var keychain = Substitute.For(); var enterpriseProbe = Substitute.For(); var wikiProbe = Substitute.For(); var factory = new SimpleApiClientFactory( program, + CreateKeychain(), new Lazy(() => enterpriseProbe), new Lazy(() => wikiProbe)); @@ -42,6 +44,7 @@ public async Task RemovesClientFromCache() var wikiProbe = Substitute.For(); var factory = new SimpleApiClientFactory( program, + CreateKeychain(), new Lazy(() => enterpriseProbe), new Lazy(() => wikiProbe)); @@ -51,4 +54,11 @@ public async Task RemovesClientFromCache() Assert.NotSame(client, factory.Create(url)); } } + + static IKeychain CreateKeychain() + { + var result = Substitute.For(); + result.Load(null).ReturnsForAnyArgs(Tuple.Create("user", "pass")); + return result; + } } diff --git a/src/UnitTests/GitHub.App/Caches/CredentialCacheTests.cs b/src/UnitTests/GitHub.App/Caches/CredentialCacheTests.cs deleted file mode 100644 index 1dfbfc0b00..0000000000 --- a/src/UnitTests/GitHub.App/Caches/CredentialCacheTests.cs +++ /dev/null @@ -1,294 +0,0 @@ -using System; -using System.Collections.Generic; -using System.Reactive.Linq; -using System.Text; -using System.Threading.Tasks; -using GitHub.Caches; -using Xunit; - -public class CredentialCacheTests : TestBaseClass -{ - public class TheGetObjectMethod : TestBaseClass - { - [Fact] - public async Task RetrievesValueWithAlternateKeys() - { - const string key = nameof(RetrievesValueWithAlternateKeys); - using (var credentialCache = new CredentialCache()) - { - try - { - var credential = Tuple.Create("somebody", "somebody's secret"); - await credentialCache.InsertObject(key, credential); - - var retrieved = await credentialCache.GetObject>(key); - - Assert.Equal("somebody", retrieved.Item1); - Assert.Equal("somebody's secret", retrieved.Item2); - - var retrieved2 = await credentialCache.GetObject>("git:" + key + "/"); - - Assert.Equal("somebody", retrieved2.Item1); - Assert.Equal("somebody's secret", retrieved2.Item2); - - var retrieved3 = await credentialCache.GetObject>("login:" + key + "/"); - - Assert.Equal("somebody", retrieved3.Item1); - Assert.Equal("somebody's secret", retrieved3.Item2); - } - finally - { - await credentialCache.Invalidate(key); - } - } - } - - [Fact] - public async Task ThrowsObservableInvalidOperationExceptionWhenRetrievingSomethingNotATuple() - { - using (var credentialCache = new CredentialCache()) - { - await Assert.ThrowsAsync( - async () => await credentialCache.GetObject("_")); - } - } - - [Fact] - public async Task ThrowsObjectDisposedExceptionWhenDisposed() - { - using (var credentialCache = new CredentialCache()) - { - credentialCache.Dispose(); - await Assert.ThrowsAsync( - async () => await credentialCache.GetObject>("_")); - } - } - } - - public class TheInsertObjectMethod : TestBaseClass - { - [Fact] - public async Task StoresCredentialForKeyAndGitKey() - { - using (var credentialCache = new CredentialCache()) - { - try - { - var credential = Tuple.Create("somebody", "somebody's secret"); - - await credentialCache.InsertObject(nameof(StoresCredentialForKeyAndGitKey), credential); - - var retrieved = await credentialCache.GetObject>(nameof(StoresCredentialForKeyAndGitKey)); - Assert.Equal("somebody", retrieved.Item1); - Assert.Equal("somebody's secret", retrieved.Item2); - var retrieved2 = await credentialCache.GetObject>("git:" + nameof(StoresCredentialForKeyAndGitKey)); - Assert.Equal("somebody", retrieved2.Item1); - Assert.Equal("somebody's secret", retrieved2.Item2); - } - finally - { - try - { - await credentialCache.Invalidate(nameof(StoresCredentialForKeyAndGitKey)); - } - catch (Exception) - { - } - } - } - } - - [Fact] - public async Task ThrowsObjectDisposedExceptionWhenDisposed() - { - using (var credentialCache = new CredentialCache()) - { - credentialCache.Dispose(); - await Assert.ThrowsAsync( - async () => await credentialCache.InsertObject("_", new object())); - } - } - } - - public class TheInsertMethod : TestBaseClass - { - [Fact] - public async Task ThrowsInvalidOperationException() - { - using (var credentialCache = new CredentialCache()) - { - await Assert.ThrowsAsync( - async () => await credentialCache.Insert("key", new byte[] {})); - } - } - } - - public class TheGetMethod : TestBaseClass - { - [Fact] - public async Task RetrievesPasswordAsUnicodeBytes() - { - using (var credentialCache = new CredentialCache()) - { - try - { - var credential = Tuple.Create("somebody", "somebody's secret"); - await credentialCache.InsertObject(nameof(RetrievesPasswordAsUnicodeBytes), credential); - - var retrieved = await credentialCache.Get(nameof(RetrievesPasswordAsUnicodeBytes)); - - Assert.Equal("somebody's secret", Encoding.Unicode.GetString(retrieved)); - } - finally - { - await credentialCache.Invalidate(nameof(RetrievesPasswordAsUnicodeBytes)); - } - } - } - - [Fact] - public async Task ThrowsObservableKeyNotFoundExceptionWhenKeyNotFound() - { - using (var credentialCache = new CredentialCache()) - { - await Assert.ThrowsAsync(async () => await credentialCache.Get("unknownkey")); - } - } - - [Fact] - public async Task ThrowsObjectDisposedExceptionWhenDisposed() - { - using (var credentialCache = new CredentialCache()) - { - credentialCache.Dispose(); - await Assert.ThrowsAsync( - async () => await credentialCache.Get("_")); - } - } - } - - public class TheInvalidateMethod : TestBaseClass - { - [Fact] - public async Task InvalidatesTheCredential() - { - const string key = "TheInvalidateMethod.InvalidatesTheCredential"; - using (var credentialCache = new CredentialCache()) - { - var credential = Tuple.Create("somebody", "somebody's secret"); - await credentialCache.InsertObject(key, credential); - await credentialCache.Invalidate(key); - await Assert.ThrowsAsync(async () => await credentialCache.Get(key)); - } - } - - [Fact] - public async Task ThrowsKeyNotFoundExceptionWhenKeyNotFound() - { - using (var credentialCache = new CredentialCache()) - { - await Assert.ThrowsAsync( - async () => await credentialCache.Invalidate("git:_")); - await Assert.ThrowsAsync( - async () => await credentialCache.Invalidate("_")); - } - } - - [Fact] - public async Task ThrowsObjectDisposedExceptionWhenDisposed() - { - using (var credentialCache = new CredentialCache()) - { - credentialCache.Dispose(); - await Assert.ThrowsAsync( - async () => await credentialCache.Invalidate("_")); - } - } - } - - public class TheInvalidateObjectMethod : TestBaseClass - { - [Fact] - public async Task InvalidatesTheCredential() - { - const string key = "TheInvalidateObjectMethod.InvalidatesTheCredential"; - using (var credentialCache = new CredentialCache()) - { - var credential = Tuple.Create("somebody", "somebody's secret"); - await credentialCache.InsertObject(key, credential); - await credentialCache.InvalidateObject>(key); - await Assert.ThrowsAsync(async () => await credentialCache.Get(key)); - } - } - - [Fact] - public async Task ThrowsObjectDisposedExceptionWhenDisposed() - { - using (var credentialCache = new CredentialCache()) - { - credentialCache.Dispose(); - await Assert.ThrowsAsync( - async () => await credentialCache.InvalidateObject>("_")); - } - } - - [Fact] - public async Task ThrowsKeyNotFoundExceptionWhenKeyNotFound() - { - using (var credentialCache = new CredentialCache()) - { - await Assert.ThrowsAsync( - async () => await credentialCache.InvalidateObject>("git:_")); - await Assert.ThrowsAsync( - async () => await credentialCache.InvalidateObject>("_")); - } - } - } - - public class TheFlushMethod : TestBaseClass - { - [Fact] - public async Task ThrowsObjectDisposedExceptionWhenDisposed() - { - using (var credentialCache = new CredentialCache()) - { - await credentialCache.Flush(); - - credentialCache.Dispose(); - await Assert.ThrowsAsync(async () => await credentialCache.Flush()); - } - } - } - - public class TheDisposeMethod : TestBaseClass - { - [Fact] - public void SignalsShutdown() - { - bool shutdown = false; - using (var credentialCache = new CredentialCache()) - { - credentialCache.Shutdown.Subscribe(_ => shutdown = true); - } - Assert.True(shutdown); - } - } - - public class MethodsNotImplementedOnPurpose : TestBaseClass - { - [Fact] - public void ThrowNotImplementedException() - { - using (var credentialCache = new CredentialCache()) - { - Assert.Throws(() => credentialCache.GetAllKeys()); - Assert.Throws(() => credentialCache.GetCreatedAt("")); - Assert.Throws(() => credentialCache.InvalidateAll()); - Assert.Throws(() => credentialCache.InvalidateAllObjects()); - Assert.Throws(() => credentialCache.Vacuum()); - Assert.Throws(() => credentialCache.GetAllObjects()); - Assert.Throws(() => credentialCache.GetObjectCreatedAt("")); - } - } - } -} diff --git a/src/UnitTests/GitHub.VisualStudio/Services/ConnectionManagerTests.cs b/src/UnitTests/GitHub.VisualStudio/Services/ConnectionManagerTests.cs index 04dcc4770e..269d61ff60 100644 --- a/src/UnitTests/GitHub.VisualStudio/Services/ConnectionManagerTests.cs +++ b/src/UnitTests/GitHub.VisualStudio/Services/ConnectionManagerTests.cs @@ -28,9 +28,10 @@ public void IsSavedToDiskWhenConnectionAdded() operatingSystem.File.Exists(@"c:\fake\GHfVS\ghfvs.connections").Returns(true); operatingSystem.File.ReadAllText(@"c:\fake\GHfVS\ghfvs.connections", Encoding.UTF8).Returns(""); var cache = Substitute.For(); + var keychain = Substitute.For(); var loginManager = Substitute.For(); var apiClientFactory = Substitute.For(); - var manager = new ConnectionManager(Substitutes.IVSGitServices, cache, loginManager, apiClientFactory); + var manager = new ConnectionManager(Substitutes.IVSGitServices, cache, keychain, loginManager, apiClientFactory); manager.Connections.Add(new Connection(manager, HostAddress.GitHubDotComHostAddress, "coolio")); diff --git a/src/UnitTests/Helpers/TestLoginCache.cs b/src/UnitTests/Helpers/TestLoginCache.cs index 1eeb8bb233..48cccc6161 100644 --- a/src/UnitTests/Helpers/TestLoginCache.cs +++ b/src/UnitTests/Helpers/TestLoginCache.cs @@ -1,14 +1,17 @@ using System; using System.Reactive; using Akavache; +using GitHub.Api; using GitHub.Caches; using GitHub.Primitives; +using NSubstitute; namespace UnitTests.Helpers { - public class TestLoginCache : ILoginCache + public class TestLoginCache : IObservableKeychainAdapter { - readonly LoginCache loginCacheDelegate = new LoginCache(new TestSharedCache()); + static readonly IKeychain keychain = Substitute.For(); + readonly ObservableKeychainAdapter loginCacheDelegate = new ObservableKeychainAdapter(keychain); public void Dispose() { diff --git a/src/UnitTests/Helpers/TestSharedCache.cs b/src/UnitTests/Helpers/TestSharedCache.cs index e50a11e74c..30c9177930 100644 --- a/src/UnitTests/Helpers/TestSharedCache.cs +++ b/src/UnitTests/Helpers/TestSharedCache.cs @@ -5,7 +5,7 @@ namespace UnitTests.Helpers { public class TestSharedCache : SharedCache { - public TestSharedCache() : base(new InMemoryBlobCache(), new InMemoryBlobCache(), new InMemoryBlobCache()) + public TestSharedCache() : base(new InMemoryBlobCache(), new InMemoryBlobCache()) { } } diff --git a/src/UnitTests/UnitTests.csproj b/src/UnitTests/UnitTests.csproj index 953366b087..11d271fdc7 100644 --- a/src/UnitTests/UnitTests.csproj +++ b/src/UnitTests/UnitTests.csproj @@ -221,7 +221,6 @@ - From 21f888ef1bdbcc1f95495731496a046a9a5f8f8e Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Fri, 25 Aug 2017 16:01:36 +0200 Subject: [PATCH 3/7] Updated parameter name. --- src/GitHub.Api/LoginManager.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/GitHub.Api/LoginManager.cs b/src/GitHub.Api/LoginManager.cs index dc20a7a1ec..01cfe196dd 100644 --- a/src/GitHub.Api/LoginManager.cs +++ b/src/GitHub.Api/LoginManager.cs @@ -23,26 +23,26 @@ public class LoginManager : ILoginManager /// /// Initializes a new instance of the class. /// - /// The cache in which to store login details. + /// The keychain in which to store credentials. /// The handler for 2FA challenges. /// The application's client API ID. /// The application's client API secret. /// An note to store with the authorization. /// The machine fingerprint. public LoginManager( - IKeychain loginCache, + IKeychain keychain, ITwoFactorChallengeHandler twoFactorChallengeHandler, string clientId, string clientSecret, string authorizationNote = null, string fingerprint = null) { - Guard.ArgumentNotNull(loginCache, nameof(loginCache)); + Guard.ArgumentNotNull(keychain, nameof(keychain)); Guard.ArgumentNotNull(twoFactorChallengeHandler, nameof(twoFactorChallengeHandler)); Guard.ArgumentNotEmptyString(clientId, nameof(clientId)); Guard.ArgumentNotEmptyString(clientSecret, nameof(clientSecret)); - this.keychain = loginCache; + this.keychain = keychain; this.twoFactorChallengeHandler = twoFactorChallengeHandler; this.clientId = clientId; this.clientSecret = clientSecret; From 7e061d15593e681d087a1950b6b33f711656cb89 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Fri, 25 Aug 2017 16:07:28 +0200 Subject: [PATCH 4/7] Removed unneeded usings. --- src/GitHub.App/Factories/RepositoryHostFactory.cs | 2 -- src/GitHub.App/Models/RepositoryHost.cs | 1 - 2 files changed, 3 deletions(-) diff --git a/src/GitHub.App/Factories/RepositoryHostFactory.cs b/src/GitHub.App/Factories/RepositoryHostFactory.cs index a3c7d13fb1..f8ab39fe0d 100644 --- a/src/GitHub.App/Factories/RepositoryHostFactory.cs +++ b/src/GitHub.App/Factories/RepositoryHostFactory.cs @@ -1,6 +1,5 @@ using System; using System.ComponentModel.Composition; -using GitHub.Authentication; using GitHub.Caches; using GitHub.Models; using GitHub.Primitives; @@ -8,7 +7,6 @@ using System.Reactive.Disposables; using System.Threading.Tasks; using GitHub.Api; -using IObservableKeychainAdapter = GitHub.Caches.IObservableKeychainAdapter; namespace GitHub.Factories { diff --git a/src/GitHub.App/Models/RepositoryHost.cs b/src/GitHub.App/Models/RepositoryHost.cs index f1e357c35a..2eab065fa8 100644 --- a/src/GitHub.App/Models/RepositoryHost.cs +++ b/src/GitHub.App/Models/RepositoryHost.cs @@ -16,7 +16,6 @@ using NLog; using Octokit; using ReactiveUI; -using IObservableKeychainAdapter = GitHub.Caches.IObservableKeychainAdapter; namespace GitHub.Models { From c8b8dd7dfcdf15a6f04d8665a84e31c460c886a4 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Fri, 25 Aug 2017 16:16:34 +0200 Subject: [PATCH 5/7] Updated variable names. --- src/GitHub.VisualStudio/GitHubPackage.cs | 4 +- src/UnitTests/GitHub.Api/LoginManagerTests.cs | 56 +++++++++---------- 2 files changed, 30 insertions(+), 30 deletions(-) diff --git a/src/GitHub.VisualStudio/GitHubPackage.cs b/src/GitHub.VisualStudio/GitHubPackage.cs index 9757861155..2bdfdcdf7c 100644 --- a/src/GitHub.VisualStudio/GitHubPackage.cs +++ b/src/GitHub.VisualStudio/GitHubPackage.cs @@ -198,11 +198,11 @@ async Task CreateService(IAsyncServiceContainer container, CancellationT else if (serviceType == typeof(ILoginManager)) { var serviceProvider = await GetServiceAsync(typeof(IGitHubServiceProvider)) as IGitHubServiceProvider; - var loginCache = serviceProvider.GetService(); + var keychain = serviceProvider.GetService(); var twoFaHandler = serviceProvider.GetService(); return new LoginManager( - loginCache, + keychain, twoFaHandler, ApiClientConfiguration.ClientId, ApiClientConfiguration.ClientSecret, diff --git a/src/UnitTests/GitHub.Api/LoginManagerTests.cs b/src/UnitTests/GitHub.Api/LoginManagerTests.cs index b5a5b3fb03..00f317d3db 100644 --- a/src/UnitTests/GitHub.Api/LoginManagerTests.cs +++ b/src/UnitTests/GitHub.Api/LoginManagerTests.cs @@ -21,13 +21,13 @@ public async Task LoginTokenIsSavedToCache() client.Authorization.GetOrCreateApplicationAuthentication("id", "secret", Arg.Any()) .Returns(new ApplicationAuthorization("123abc")); - var loginCache = Substitute.For(); + var keychain = Substitute.For(); var tfa = Substitute.For(); - var target = new LoginManager(loginCache, tfa, "id", "secret"); + var target = new LoginManager(keychain, tfa, "id", "secret"); await target.Login(host, client, "foo", "bar"); - await loginCache.Received().Save("foo", "123abc", host); + await keychain.Received().Save("foo", "123abc", host); } [Fact] @@ -39,10 +39,10 @@ public async Task LoggedInUserIsReturned() .Returns(new ApplicationAuthorization("123abc")); client.User.Current().Returns(user); - var loginCache = Substitute.For(); + var keychain = Substitute.For(); var tfa = Substitute.For(); - var target = new LoginManager(loginCache, tfa, "id", "secret"); + var target = new LoginManager(keychain, tfa, "id", "secret"); var result = await target.Login(host, client, "foo", "bar"); Assert.Same(user, result); @@ -62,15 +62,15 @@ public async Task DeletesExistingAuthenticationIfNullTokenReturned() new ApplicationAuthorization("123abc")); client.User.Current().Returns(user); - var loginCache = Substitute.For(); + var keychain = Substitute.For(); var tfa = Substitute.For(); - var target = new LoginManager(loginCache, tfa, "id", "secret"); + var target = new LoginManager(keychain, tfa, "id", "secret"); var result = await target.Login(host, client, "foo", "bar"); await client.Authorization.Received(2).GetOrCreateApplicationAuthentication("id", "secret", Arg.Any()); await client.Authorization.Received(1).Delete(0); - await loginCache.Received().Save("foo", "123abc", host); + await keychain.Received().Save("foo", "123abc", host); } [Fact] @@ -84,11 +84,11 @@ public async Task TwoFactorExceptionIsPassedToHandler() client.Authorization.GetOrCreateApplicationAuthentication("id", "secret", Arg.Any(), "123456") .Returns(new ApplicationAuthorization("123abc")); - var loginCache = Substitute.For(); + var keychain = Substitute.For(); var tfa = Substitute.For(); tfa.HandleTwoFactorException(exception).Returns(new TwoFactorChallengeResult("123456")); - var target = new LoginManager(loginCache, tfa, "id", "secret"); + var target = new LoginManager(keychain, tfa, "id", "secret"); await target.Login(host, client, "foo", "bar"); await client.Authorization.Received().GetOrCreateApplicationAuthentication( @@ -111,13 +111,13 @@ public async Task Failed2FACodeResultsInRetry() client.Authorization.GetOrCreateApplicationAuthentication("id", "secret", Arg.Any(), "123456") .Returns(new ApplicationAuthorization("123abc")); - var loginCache = Substitute.For(); + var keychain = Substitute.For(); var tfa = Substitute.For(); tfa.HandleTwoFactorException(exception).Returns( new TwoFactorChallengeResult("111111"), new TwoFactorChallengeResult("123456")); - var target = new LoginManager(loginCache, tfa, "id", "secret"); + var target = new LoginManager(keychain, tfa, "id", "secret"); await target.Login(host, client, "foo", "bar"); await client.Authorization.Received(1).GetOrCreateApplicationAuthentication( @@ -146,13 +146,13 @@ public async Task HandlerNotifiedOfExceptionIn2FAChallengeResponse() client.Authorization.GetOrCreateApplicationAuthentication("id", "secret", Arg.Any(), "111111") .Returns(_ => { throw loginAttemptsException; }); - var loginCache = Substitute.For(); + var keychain = Substitute.For(); var tfa = Substitute.For(); tfa.HandleTwoFactorException(twoFaException).Returns( new TwoFactorChallengeResult("111111"), new TwoFactorChallengeResult("123456")); - var target = new LoginManager(loginCache, tfa, "id", "secret"); + var target = new LoginManager(keychain, tfa, "id", "secret"); Assert.ThrowsAsync(async () => await target.Login(host, client, "foo", "bar")); await client.Authorization.Received(1).GetOrCreateApplicationAuthentication( @@ -176,13 +176,13 @@ public async Task RequestResendCodeResultsInRetryingLogin() .Returns(new ApplicationAuthorization("456def")); client.User.Current().Returns(user); - var loginCache = Substitute.For(); + var keychain = Substitute.For(); var tfa = Substitute.For(); tfa.HandleTwoFactorException(exception).Returns( TwoFactorChallengeResult.RequestResendCode, new TwoFactorChallengeResult("123456")); - var target = new LoginManager(loginCache, tfa, "id", "secret"); + var target = new LoginManager(keychain, tfa, "id", "secret"); await target.Login(host, client, "foo", "bar"); await client.Authorization.Received(2).GetOrCreateApplicationAuthentication("id", "secret", Arg.Any()); @@ -201,13 +201,13 @@ public async Task UsesUsernameAndPasswordInsteadOfAuthorizationTokenWhenEnterpri }); client.User.Current().Returns(user); - var loginCache = Substitute.For(); + var keychain = Substitute.For(); var tfa = Substitute.For(); - var target = new LoginManager(loginCache, tfa, "id", "secret"); + var target = new LoginManager(keychain, tfa, "id", "secret"); await target.Login(enterprise, client, "foo", "bar"); - await loginCache.Received().Save("foo", "bar", enterprise); + await keychain.Received().Save("foo", "bar", enterprise); } [Fact] @@ -219,13 +219,13 @@ public async Task ErasesLoginWhenUnauthorized() client.Authorization.GetOrCreateApplicationAuthentication("id", "secret", Arg.Any()) .Returns(_ => { throw new AuthorizationException(); }); - var loginCache = Substitute.For(); + var keychain = Substitute.For(); var tfa = Substitute.For(); - var target = new LoginManager(loginCache, tfa, "id", "secret"); + var target = new LoginManager(keychain, tfa, "id", "secret"); await Assert.ThrowsAsync(async () => await target.Login(enterprise, client, "foo", "bar")); - await loginCache.Received().Delete(enterprise); + await keychain.Received().Delete(enterprise); } [Fact] @@ -237,13 +237,13 @@ public async Task ErasesLoginWhenNonOctokitExceptionThrown() client.Authorization.GetOrCreateApplicationAuthentication("id", "secret", Arg.Any()) .Returns(_ => { throw new InvalidOperationException(); }); - var loginCache = Substitute.For(); + var keychain = Substitute.For(); var tfa = Substitute.For(); - var target = new LoginManager(loginCache, tfa, "id", "secret"); + var target = new LoginManager(keychain, tfa, "id", "secret"); await Assert.ThrowsAsync(async () => await target.Login(host, client, "foo", "bar")); - await loginCache.Received().Delete(host); + await keychain.Received().Delete(host); } [Fact] @@ -259,14 +259,14 @@ public async Task ErasesLoginWhenNonOctokitExceptionThrownIn2FA() .Returns(_ => { throw new InvalidOperationException(); }); client.User.Current().Returns(user); - var loginCache = Substitute.For(); + var keychain = Substitute.For(); var tfa = Substitute.For(); tfa.HandleTwoFactorException(exception).Returns(new TwoFactorChallengeResult("123456")); - var target = new LoginManager(loginCache, tfa, "id", "secret"); + var target = new LoginManager(keychain, tfa, "id", "secret"); await Assert.ThrowsAsync(async () => await target.Login(host, client, "foo", "bar")); - await loginCache.Received().Delete(host); + await keychain.Received().Delete(host); } } } From 95f2c0ba9a6bdef9b9688a238e4b854e0619a766 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Fri, 25 Aug 2017 16:54:36 +0200 Subject: [PATCH 6/7] Remove `IObservableKeychainAdapter`. Just call `ToObservable` on the task returned by `IKeychain`. --- .../Caches/ObservableKeychainAdapter.cs | 72 ------------------- .../Factories/RepositoryHostFactory.cs | 4 +- src/GitHub.App/GitHub.App.csproj | 1 - src/GitHub.App/Models/RepositoryHost.cs | 8 +-- .../Caches/IObservableKeychainAdapter.cs | 15 ---- .../GitHub.Exports.Reactive.csproj | 1 - .../GitHub.App/Models/RepositoryHostTests.cs | 21 +++--- src/UnitTests/Helpers/TestLoginCache.cs | 41 ----------- src/UnitTests/UnitTests.csproj | 1 - 9 files changed, 16 insertions(+), 148 deletions(-) delete mode 100644 src/GitHub.App/Caches/ObservableKeychainAdapter.cs delete mode 100644 src/GitHub.Exports.Reactive/Caches/IObservableKeychainAdapter.cs delete mode 100644 src/UnitTests/Helpers/TestLoginCache.cs diff --git a/src/GitHub.App/Caches/ObservableKeychainAdapter.cs b/src/GitHub.App/Caches/ObservableKeychainAdapter.cs deleted file mode 100644 index 36695869e0..0000000000 --- a/src/GitHub.App/Caches/ObservableKeychainAdapter.cs +++ /dev/null @@ -1,72 +0,0 @@ -using System; -using System.ComponentModel.Composition; -using System.Globalization; -using System.Reactive; -using System.Reactive.Linq; -using System.Reactive.Threading.Tasks; -using Akavache; -using GitHub.Api; -using GitHub.Extensions; -using GitHub.Models; -using GitHub.Primitives; -using NLog; - -namespace GitHub.Caches -{ - /// - /// An observable wrapper around a . - /// - /// - /// There are some older classes such as that want an observable - /// interface for reading credentials. They should be rewritten to use `Task`s but for the - /// moment this class serves as an adapter for those clients. - /// - [Export(typeof(IObservableKeychainAdapter))] - [PartCreationPolicy(CreationPolicy.Shared)] - public sealed class ObservableKeychainAdapter : IObservableKeychainAdapter - { - static readonly Logger log = LogManager.GetCurrentClassLogger(); - readonly IKeychain keychain; - - static readonly LoginInfo empty = new LoginInfo("", ""); - - [ImportingConstructor] - public ObservableKeychainAdapter(IKeychain keychain) - { - this.keychain = keychain; - } - - public static LoginInfo EmptyLoginInfo - { - get { return empty; } - } - - public IObservable GetLoginAsync(HostAddress hostAddress) - { - return keychain.Load(hostAddress) - .ToObservable() - .Select(x => new LoginInfo(x.Item1, x.Item2)); - } - - public IObservable SaveLogin(string user, string password, HostAddress hostAddress) - { - Guard.ArgumentNotEmptyString(user, nameof(user)); - Guard.ArgumentNotEmptyString(password, nameof(password)); - - return keychain.Save(user, password, hostAddress).ToObservable(); - } - - public IObservable EraseLogin(HostAddress hostAddress) - { - log.Info(CultureInfo.CurrentCulture, "Erasing the git credential cache for host '{0}'", - hostAddress.CredentialCacheKeyHost); - - return keychain.Delete(hostAddress).ToObservable(); - } - - public IObservable Flush() => Observable.Return(Unit.Default); - - public void Dispose() - {} - } -} diff --git a/src/GitHub.App/Factories/RepositoryHostFactory.cs b/src/GitHub.App/Factories/RepositoryHostFactory.cs index f8ab39fe0d..6cb5c662a1 100644 --- a/src/GitHub.App/Factories/RepositoryHostFactory.cs +++ b/src/GitHub.App/Factories/RepositoryHostFactory.cs @@ -17,7 +17,7 @@ public class RepositoryHostFactory : IRepositoryHostFactory readonly IApiClientFactory apiClientFactory; readonly IHostCacheFactory hostCacheFactory; readonly ILoginManager loginManager; - readonly IObservableKeychainAdapter keychain; + readonly IKeychain keychain; readonly IAvatarProvider avatarProvider; readonly CompositeDisposable hosts = new CompositeDisposable(); readonly IUsageTracker usage; @@ -27,7 +27,7 @@ public RepositoryHostFactory( IApiClientFactory apiClientFactory, IHostCacheFactory hostCacheFactory, ILoginManager loginManager, - IObservableKeychainAdapter keychain, + IKeychain keychain, IAvatarProvider avatarProvider, IUsageTracker usage) { diff --git a/src/GitHub.App/GitHub.App.csproj b/src/GitHub.App/GitHub.App.csproj index c23f684ee6..f85b27b648 100644 --- a/src/GitHub.App/GitHub.App.csproj +++ b/src/GitHub.App/GitHub.App.csproj @@ -177,7 +177,6 @@ - diff --git a/src/GitHub.App/Models/RepositoryHost.cs b/src/GitHub.App/Models/RepositoryHost.cs index 2eab065fa8..0509a44cd7 100644 --- a/src/GitHub.App/Models/RepositoryHost.cs +++ b/src/GitHub.App/Models/RepositoryHost.cs @@ -27,7 +27,7 @@ public class RepositoryHost : ReactiveObject, IRepositoryHost readonly ILoginManager loginManager; readonly HostAddress hostAddress; - readonly IObservableKeychainAdapter keychain; + readonly IKeychain keychain; readonly IUsageTracker usage; bool isLoggedIn; @@ -36,7 +36,7 @@ public RepositoryHost( IApiClient apiClient, IModelService modelService, ILoginManager loginManager, - IObservableKeychainAdapter keychain, + IKeychain keychain, IUsageTracker usage) { ApiClient = apiClient; @@ -126,7 +126,7 @@ public IObservable LogOut() log.Info(CultureInfo.InvariantCulture, "Logged off of host '{0}'", hostAddress.ApiUri); - return keychain.EraseLogin(Address) + return keychain.Delete(Address).ToObservable() .Catch(e => { log.Warn("ASSERT! Failed to erase login. Going to invalidate cache anyways.", e); @@ -167,7 +167,7 @@ IObservable LoginWithApiUser(UserAndScopes userAndScopes) if (result == AuthenticationResult.VerificationFailure) { - return keychain.EraseLogin(Address).Select(_ => result); + return keychain.Delete(Address).ToObservable().Select(_ => result); } return Observable.Return(result); }) diff --git a/src/GitHub.Exports.Reactive/Caches/IObservableKeychainAdapter.cs b/src/GitHub.Exports.Reactive/Caches/IObservableKeychainAdapter.cs deleted file mode 100644 index b0630345ef..0000000000 --- a/src/GitHub.Exports.Reactive/Caches/IObservableKeychainAdapter.cs +++ /dev/null @@ -1,15 +0,0 @@ -using System; -using System.Reactive; -using Akavache; -using GitHub.Primitives; - -namespace GitHub.Caches -{ - public interface IObservableKeychainAdapter : IDisposable - { - IObservable GetLoginAsync(HostAddress hostAddress); - IObservable SaveLogin(string user, string password, HostAddress hostAddress); - IObservable EraseLogin(HostAddress hostAddress); - IObservable Flush(); - } -} diff --git a/src/GitHub.Exports.Reactive/GitHub.Exports.Reactive.csproj b/src/GitHub.Exports.Reactive/GitHub.Exports.Reactive.csproj index 44ab2da0a5..ece0ce2ed6 100644 --- a/src/GitHub.Exports.Reactive/GitHub.Exports.Reactive.csproj +++ b/src/GitHub.Exports.Reactive/GitHub.Exports.Reactive.csproj @@ -120,7 +120,6 @@ - diff --git a/src/UnitTests/GitHub.App/Models/RepositoryHostTests.cs b/src/UnitTests/GitHub.App/Models/RepositoryHostTests.cs index 139631c037..36bf294695 100644 --- a/src/UnitTests/GitHub.App/Models/RepositoryHostTests.cs +++ b/src/UnitTests/GitHub.App/Models/RepositoryHostTests.cs @@ -12,7 +12,6 @@ using GitHub.Services; using NSubstitute; using Octokit; -using UnitTests.Helpers; using Xunit; public class RepositoryHostTests @@ -32,9 +31,9 @@ public async Task LogsTheUserInSuccessfullyAndCachesRelevantInfo() var modelService = new ModelService(apiClient, hostCache, Substitute.For()); var loginManager = Substitute.For(); loginManager.Login(HostAddress.GitHubDotComHostAddress, Arg.Any(), "baymax", "aPassword").Returns(CreateUserAndScopes("baymax").User); - var loginCache = new TestLoginCache(); + var keychain = Substitute.For(); var usage = Substitute.For(); - var host = new RepositoryHost(apiClient, modelService, loginManager, loginCache, usage); + var host = new RepositoryHost(apiClient, modelService, loginManager, keychain, usage); var result = await host.LogIn("baymax", "aPassword"); @@ -57,9 +56,9 @@ public async Task IncrementsLoginCount() var modelService = Substitute.For(); var loginManager = Substitute.For(); loginManager.Login(HostAddress.GitHubDotComHostAddress, Arg.Any(), "baymax", "aPassword").Returns(CreateUserAndScopes("baymax").User); - var loginCache = new TestLoginCache(); + var keychain = Substitute.For(); var usage = Substitute.For(); - var host = new RepositoryHost(apiClient, modelService, loginManager, loginCache, usage); + var host = new RepositoryHost(apiClient, modelService, loginManager, keychain, usage); var result = await host.LogIn("baymax", "aPassword"); @@ -76,9 +75,9 @@ public async Task DoesNotLogInWhenRetrievingOauthTokenFails() var loginManager = Substitute.For(); loginManager.Login(HostAddress.GitHubDotComHostAddress, Arg.Any(), "jiminy", "cricket") .Returns(_ => { throw new NotFoundException("", HttpStatusCode.BadGateway); }); - var loginCache = new TestLoginCache(); + var keychain = Substitute.For(); var usage = Substitute.For(); - var host = new RepositoryHost(apiClient, modelService, loginManager, loginCache, usage); + var host = new RepositoryHost(apiClient, modelService, loginManager, keychain, usage); await Assert.ThrowsAsync(async () => await host.LogIn("jiminy", "cricket")); @@ -98,9 +97,9 @@ public async Task LogsTheUserInSuccessfullyAndCachesRelevantInfo() var modelService = new ModelService(apiClient, hostCache, Substitute.For()); var loginManager = Substitute.For(); loginManager.LoginFromCache(HostAddress.GitHubDotComHostAddress, Arg.Any()).Returns(CreateUserAndScopes("baymax").User); - var loginCache = new TestLoginCache(); + var keychain = Substitute.For(); var usage = Substitute.For(); - var host = new RepositoryHost(apiClient, modelService, loginManager, loginCache, usage); + var host = new RepositoryHost(apiClient, modelService, loginManager, keychain, usage); var result = await host.LogInFromCache(); @@ -119,9 +118,9 @@ public async Task IncrementsLoginCount() var modelService = Substitute.For(); var loginManager = Substitute.For(); loginManager.LoginFromCache(HostAddress.GitHubDotComHostAddress, Arg.Any()).Returns(CreateUserAndScopes("baymax").User); - var loginCache = new TestLoginCache(); + var keychain = Substitute.For(); var usage = Substitute.For(); - var host = new RepositoryHost(apiClient, modelService, loginManager, loginCache, usage); + var host = new RepositoryHost(apiClient, modelService, loginManager, keychain, usage); var result = await host.LogInFromCache(); diff --git a/src/UnitTests/Helpers/TestLoginCache.cs b/src/UnitTests/Helpers/TestLoginCache.cs deleted file mode 100644 index 48cccc6161..0000000000 --- a/src/UnitTests/Helpers/TestLoginCache.cs +++ /dev/null @@ -1,41 +0,0 @@ -using System; -using System.Reactive; -using Akavache; -using GitHub.Api; -using GitHub.Caches; -using GitHub.Primitives; -using NSubstitute; - -namespace UnitTests.Helpers -{ - public class TestLoginCache : IObservableKeychainAdapter - { - static readonly IKeychain keychain = Substitute.For(); - readonly ObservableKeychainAdapter loginCacheDelegate = new ObservableKeychainAdapter(keychain); - - public void Dispose() - { - loginCacheDelegate.Dispose(); - } - - public IObservable GetLoginAsync(HostAddress hostAddress) - { - return loginCacheDelegate.GetLoginAsync(hostAddress); - } - - public IObservable SaveLogin(string user, string password, HostAddress hostAddress) - { - return loginCacheDelegate.SaveLogin(user, password, hostAddress); - } - - public IObservable EraseLogin(HostAddress hostAddress) - { - return loginCacheDelegate.EraseLogin(hostAddress); - } - - public IObservable Flush() - { - return loginCacheDelegate.Flush(); - } - } -} diff --git a/src/UnitTests/UnitTests.csproj b/src/UnitTests/UnitTests.csproj index 11d271fdc7..c0556c25f5 100644 --- a/src/UnitTests/UnitTests.csproj +++ b/src/UnitTests/UnitTests.csproj @@ -264,7 +264,6 @@ - True From bcfbd798781b31df16f110dd2c90a68dec8d07ea Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Thu, 7 Sep 2017 15:12:07 +0200 Subject: [PATCH 7/7] Fix broken merge. --- .../GitHub.App/Models/RepositoryHostTests.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/UnitTests/GitHub.App/Models/RepositoryHostTests.cs b/src/UnitTests/GitHub.App/Models/RepositoryHostTests.cs index 36bf294695..93db5bf0a7 100644 --- a/src/UnitTests/GitHub.App/Models/RepositoryHostTests.cs +++ b/src/UnitTests/GitHub.App/Models/RepositoryHostTests.cs @@ -26,11 +26,11 @@ public async Task LogsTheUserInSuccessfullyAndCachesRelevantInfo() apiClient.GetOrCreateApplicationAuthenticationCode( Args.TwoFactorChallengCallback, Args.String, Args.Boolean) .Returns(Observable.Return(new ApplicationAuthorization("S3CR3TS"))); - apiClient.GetUser().Returns(Observable.Return(CreateUserAndScopes("baymax"))); + apiClient.GetUser().Returns(Observable.Return(CreateOctokitUser("baymax"))); var hostCache = new InMemoryBlobCache(); var modelService = new ModelService(apiClient, hostCache, Substitute.For()); var loginManager = Substitute.For(); - loginManager.Login(HostAddress.GitHubDotComHostAddress, Arg.Any(), "baymax", "aPassword").Returns(CreateUserAndScopes("baymax").User); + loginManager.Login(HostAddress.GitHubDotComHostAddress, Arg.Any(), "baymax", "aPassword").Returns(CreateOctokitUser("baymax")); var keychain = Substitute.For(); var usage = Substitute.For(); var host = new RepositoryHost(apiClient, modelService, loginManager, keychain, usage); @@ -51,11 +51,11 @@ public async Task IncrementsLoginCount() apiClient.GetOrCreateApplicationAuthenticationCode( Args.TwoFactorChallengCallback, Args.String, Args.Boolean) .Returns(Observable.Return(new ApplicationAuthorization("S3CR3TS"))); - apiClient.GetUser().Returns(Observable.Return(CreateUserAndScopes("baymax"))); + apiClient.GetUser().Returns(Observable.Return(CreateOctokitUser("baymax"))); var hostCache = new InMemoryBlobCache(); var modelService = Substitute.For(); var loginManager = Substitute.For(); - loginManager.Login(HostAddress.GitHubDotComHostAddress, Arg.Any(), "baymax", "aPassword").Returns(CreateUserAndScopes("baymax").User); + loginManager.Login(HostAddress.GitHubDotComHostAddress, Arg.Any(), "baymax", "aPassword").Returns(CreateOctokitUser("baymax")); var keychain = Substitute.For(); var usage = Substitute.For(); var host = new RepositoryHost(apiClient, modelService, loginManager, keychain, usage); @@ -96,7 +96,7 @@ public async Task LogsTheUserInSuccessfullyAndCachesRelevantInfo() var hostCache = new InMemoryBlobCache(); var modelService = new ModelService(apiClient, hostCache, Substitute.For()); var loginManager = Substitute.For(); - loginManager.LoginFromCache(HostAddress.GitHubDotComHostAddress, Arg.Any()).Returns(CreateUserAndScopes("baymax").User); + loginManager.LoginFromCache(HostAddress.GitHubDotComHostAddress, Arg.Any()).Returns(CreateOctokitUser("baymax")); var keychain = Substitute.For(); var usage = Substitute.For(); var host = new RepositoryHost(apiClient, modelService, loginManager, keychain, usage); @@ -117,7 +117,7 @@ public async Task IncrementsLoginCount() var hostCache = new InMemoryBlobCache(); var modelService = Substitute.For(); var loginManager = Substitute.For(); - loginManager.LoginFromCache(HostAddress.GitHubDotComHostAddress, Arg.Any()).Returns(CreateUserAndScopes("baymax").User); + loginManager.LoginFromCache(HostAddress.GitHubDotComHostAddress, Arg.Any()).Returns(CreateOctokitUser("baymax")); var keychain = Substitute.For(); var usage = Substitute.For(); var host = new RepositoryHost(apiClient, modelService, loginManager, keychain, usage);