From e30c5fad0fc1ed2ed772566d4c8120d825e4f401 Mon Sep 17 00:00:00 2001 From: Stanley Goldman Date: Thu, 12 Apr 2018 12:00:36 -0400 Subject: [PATCH 1/8] Validating the authentication result in order to ensure that we have the username --- src/GitHub.Api/Authentication/Credential.cs | 7 ++++++- src/GitHub.Api/Authentication/ICredentialManager.cs | 2 +- src/GitHub.Api/Authentication/IKeychain.cs | 2 +- src/GitHub.Api/Authentication/Keychain.cs | 4 ++-- src/GitHub.Api/Authentication/KeychainAdapter.cs | 4 ++-- src/GitHub.Api/Authentication/LoginManager.cs | 12 +++++++++++- 6 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/GitHub.Api/Authentication/Credential.cs b/src/GitHub.Api/Authentication/Credential.cs index f7c74d3a5..39aec2728 100644 --- a/src/GitHub.Api/Authentication/Credential.cs +++ b/src/GitHub.Api/Authentication/Credential.cs @@ -16,9 +16,14 @@ public Credential(UriString host, string username, string token) this.Token = token; } - public void UpdateToken(string token) + public void UpdateToken(string token, string username = null) { this.Token = token; + + if (username != null) + { + this.Username = username; + } } public UriString Host { get; private set; } diff --git a/src/GitHub.Api/Authentication/ICredentialManager.cs b/src/GitHub.Api/Authentication/ICredentialManager.cs index 28742a6c7..c087e9f07 100644 --- a/src/GitHub.Api/Authentication/ICredentialManager.cs +++ b/src/GitHub.Api/Authentication/ICredentialManager.cs @@ -8,7 +8,7 @@ public interface ICredential : IDisposable UriString Host { get; } string Username { get; } string Token { get; } - void UpdateToken(string token); + void UpdateToken(string token, string username = null); } public interface ICredentialManager diff --git a/src/GitHub.Api/Authentication/IKeychain.cs b/src/GitHub.Api/Authentication/IKeychain.cs index 4aa1bcb97..20d409dcd 100644 --- a/src/GitHub.Api/Authentication/IKeychain.cs +++ b/src/GitHub.Api/Authentication/IKeychain.cs @@ -16,7 +16,7 @@ public interface IKeychain Connection[] Connections { get; } IList Hosts { get; } bool HasKeys { get; } - void SetToken(UriString host, string token); + void SetToken(UriString host, string token, string username = null); event Action ConnectionsChanged; } diff --git a/src/GitHub.Api/Authentication/Keychain.cs b/src/GitHub.Api/Authentication/Keychain.cs index 2b3c933e2..f798efb09 100644 --- a/src/GitHub.Api/Authentication/Keychain.cs +++ b/src/GitHub.Api/Authentication/Keychain.cs @@ -164,11 +164,11 @@ public void SetCredentials(ICredential credential) keychainAdapter.Set(credential); } - public void SetToken(UriString host, string token) + public void SetToken(UriString host, string token, string username = null) { //logger.Trace("SetToken Host:{0}", host); var keychainAdapter = GetKeychainAdapter(host); - keychainAdapter.UpdateToken(token); + keychainAdapter.UpdateToken(token, username); } public void UpdateToken(UriString host, string token) diff --git a/src/GitHub.Api/Authentication/KeychainAdapter.cs b/src/GitHub.Api/Authentication/KeychainAdapter.cs index 3fdde60b3..bdfdc72ac 100644 --- a/src/GitHub.Api/Authentication/KeychainAdapter.cs +++ b/src/GitHub.Api/Authentication/KeychainAdapter.cs @@ -9,9 +9,9 @@ public void Set(ICredential credential) Credential = credential; } - public void UpdateToken(string token) + public void UpdateToken(string token, string username = null) { - Credential.UpdateToken(token); + Credential.UpdateToken(token, username); } public void Clear() diff --git a/src/GitHub.Api/Authentication/LoginManager.cs b/src/GitHub.Api/Authentication/LoginManager.cs index ce0a85ad2..c9c98ded5 100644 --- a/src/GitHub.Api/Authentication/LoginManager.cs +++ b/src/GitHub.Api/Authentication/LoginManager.cs @@ -119,7 +119,17 @@ public async Task ContinueLogin(LoginResultData loginResultData throw new InvalidOperationException("Returned token is null or empty"); } - keychain.SetToken(host, loginResultData.Token); + var octorunTask = new OctorunTask(taskManager.Token, nodeJsExecutablePath.Value, octorunScript.Value, "validate", + user: username, userToken: loginResultData.Token) + .Configure(processManager); + + var validateResult = await octorunTask.StartAsAsync(); + if (!validateResult.IsSuccess) + { + throw new InvalidOperationException("Authentication validation failed"); + } + + keychain.SetToken(host, loginResultData.Token, validateResult.Output[1]); await keychain.Save(host); return loginResultData; From ea31f9d25aa6e1bca5f7c2ade034932b0b1bdffc Mon Sep 17 00:00:00 2001 From: Stanley Goldman Date: Thu, 12 Apr 2018 12:02:32 -0400 Subject: [PATCH 2/8] Removing the UpdateToken method because it is never used --- src/GitHub.Api/Authentication/IKeychain.cs | 1 - src/GitHub.Api/Authentication/Keychain.cs | 8 -------- 2 files changed, 9 deletions(-) diff --git a/src/GitHub.Api/Authentication/IKeychain.cs b/src/GitHub.Api/Authentication/IKeychain.cs index 20d409dcd..553cd0534 100644 --- a/src/GitHub.Api/Authentication/IKeychain.cs +++ b/src/GitHub.Api/Authentication/IKeychain.cs @@ -10,7 +10,6 @@ public interface IKeychain Task Load(UriString host); Task Clear(UriString host, bool deleteFromCredentialManager); Task Save(UriString host); - void UpdateToken(UriString host, string token); void SetCredentials(ICredential credential); void Initialize(); Connection[] Connections { get; } diff --git a/src/GitHub.Api/Authentication/Keychain.cs b/src/GitHub.Api/Authentication/Keychain.cs index f798efb09..614d6fb57 100644 --- a/src/GitHub.Api/Authentication/Keychain.cs +++ b/src/GitHub.Api/Authentication/Keychain.cs @@ -171,14 +171,6 @@ public void SetToken(UriString host, string token, string username = null) keychainAdapter.UpdateToken(token, username); } - public void UpdateToken(UriString host, string token) - { - //logger.Trace("UpdateToken Host:{0}", host); - var keychainAdapter = GetKeychainAdapter(host); - var keychainItem = keychainAdapter.Credential; - keychainItem.UpdateToken(token); - } - private void LoadConnectionsFromDisk() { //logger.Trace("ReadCacheFromDisk Path:{0}", cachePath.ToString()); From 4cd72e0cc415271086ee611134a846ca76ae4a36 Mon Sep 17 00:00:00 2001 From: Stanley Goldman Date: Fri, 20 Apr 2018 09:48:31 -0400 Subject: [PATCH 3/8] Making username required in SetToken --- src/GitHub.Api/Authentication/IKeychain.cs | 2 +- src/GitHub.Api/Authentication/Keychain.cs | 29 ++++++++++++------- src/GitHub.Api/Authentication/LoginManager.cs | 2 +- .../UnitTests/Authentication/KeychainTests.cs | 2 +- 4 files changed, 22 insertions(+), 13 deletions(-) diff --git a/src/GitHub.Api/Authentication/IKeychain.cs b/src/GitHub.Api/Authentication/IKeychain.cs index 553cd0534..a05a748dc 100644 --- a/src/GitHub.Api/Authentication/IKeychain.cs +++ b/src/GitHub.Api/Authentication/IKeychain.cs @@ -15,7 +15,7 @@ public interface IKeychain Connection[] Connections { get; } IList Hosts { get; } bool HasKeys { get; } - void SetToken(UriString host, string token, string username = null); + void SetToken(UriString host, string token, string username); event Action ConnectionsChanged; } diff --git a/src/GitHub.Api/Authentication/Keychain.cs b/src/GitHub.Api/Authentication/Keychain.cs index 614d6fb57..77289a5ba 100644 --- a/src/GitHub.Api/Authentication/Keychain.cs +++ b/src/GitHub.Api/Authentication/Keychain.cs @@ -95,11 +95,15 @@ public Keychain(IEnvironment environment, ICredentialManager credentialManager) public IKeychainAdapter Connect(UriString host) { + Guard.ArgumentNotNull(host, nameof(host)); + return FindOrCreateAdapter(host); } public async Task Load(UriString host) { + Guard.ArgumentNotNull(host, nameof(host)); + var keychainAdapter = FindOrCreateAdapter(host); var connection = GetConnection(host); @@ -145,6 +149,9 @@ public void Initialize() public async Task Clear(UriString host, bool deleteFromCredentialManager) { //logger.Trace("Clear Host:{0}", host); + + Guard.ArgumentNotNull(host, nameof(host)); + //clear octokit credentials await RemoveCredential(host, deleteFromCredentialManager); RemoveConnection(host); @@ -153,6 +160,9 @@ public async Task Clear(UriString host, bool deleteFromCredentialManager) public async Task Save(UriString host) { //logger.Trace("Save: {0}", host); + + Guard.ArgumentNotNull(host, nameof(host)); + var keychainAdapter = await AddCredential(host); AddConnection(new Connection(host, keychainAdapter.Credential.Username)); } @@ -160,13 +170,21 @@ public async Task Save(UriString host) public void SetCredentials(ICredential credential) { //logger.Trace("SetCredentials Host:{0}", credential.Host); + + Guard.ArgumentNotNull(credential, nameof(credential)); + var keychainAdapter = GetKeychainAdapter(credential.Host); keychainAdapter.Set(credential); } - public void SetToken(UriString host, string token, string username = null) + public void SetToken(UriString host, string token, string username) { //logger.Trace("SetToken Host:{0}", host); + + Guard.ArgumentNotNull(host, nameof(host)); + Guard.ArgumentNotNull(token, nameof(token)); + Guard.ArgumentNotNull(username, nameof(username)); + var keychainAdapter = GetKeychainAdapter(host); keychainAdapter.UpdateToken(token, username); } @@ -282,15 +300,6 @@ private void RemoveConnection(UriString host) } } - private void RemoveAllConnections() - { - if (connections.Count > 0) - { - connections.Clear(); - SaveConnectionsToDisk(); - } - } - private void UpdateConnections(Connection[] conns) { var updated = false; diff --git a/src/GitHub.Api/Authentication/LoginManager.cs b/src/GitHub.Api/Authentication/LoginManager.cs index 23a355b1f..06f1b1d6a 100644 --- a/src/GitHub.Api/Authentication/LoginManager.cs +++ b/src/GitHub.Api/Authentication/LoginManager.cs @@ -84,7 +84,7 @@ public async Task Login( throw new InvalidOperationException("Returned token is null or empty"); } - keychain.SetToken(host, loginResultData.Token); + keychain.SetToken(host, loginResultData.Token, null); if (loginResultData.Code == LoginResultCodes.Success) { diff --git a/src/tests/UnitTests/Authentication/KeychainTests.cs b/src/tests/UnitTests/Authentication/KeychainTests.cs index ba5244abd..571c83ac1 100644 --- a/src/tests/UnitTests/Authentication/KeychainTests.cs +++ b/src/tests/UnitTests/Authentication/KeychainTests.cs @@ -292,7 +292,7 @@ public void ShouldConnectSetCredentialsTokenAndSave() keychainAdapter.Credential.Username.Should().Be(username); keychainAdapter.Credential.Token.Should().Be(password); - keychain.SetToken(hostUri, token); + keychain.SetToken(hostUri, token, null); keychainAdapter.Credential.Should().NotBeNull(); keychainAdapter.Credential.Host.Should().Be(hostUri); From 2611f08e7c012fac4cca6d364111e06a6884e1dd Mon Sep 17 00:00:00 2001 From: Stanley Goldman Date: Fri, 20 Apr 2018 09:56:26 -0400 Subject: [PATCH 4/8] Centralize function to retrieve username --- src/GitHub.Api/Authentication/LoginManager.cs | 32 ++++++++++++------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/src/GitHub.Api/Authentication/LoginManager.cs b/src/GitHub.Api/Authentication/LoginManager.cs index 06f1b1d6a..61307713f 100644 --- a/src/GitHub.Api/Authentication/LoginManager.cs +++ b/src/GitHub.Api/Authentication/LoginManager.cs @@ -123,17 +123,8 @@ public async Task ContinueLogin(LoginResultData loginResultData throw new InvalidOperationException("Returned token is null or empty"); } - var octorunTask = new OctorunTask(taskManager.Token, nodeJsExecutablePath.Value, octorunScript.Value, "validate", - user: username, userToken: loginResultData.Token) - .Configure(processManager); - - var validateResult = await octorunTask.StartAsAsync(); - if (!validateResult.IsSuccess) - { - throw new InvalidOperationException("Authentication validation failed"); - } - - keychain.SetToken(host, loginResultData.Token, validateResult.Output[1]); + username = await RetrieveUsername(loginResultData, username); + keychain.SetToken(host, loginResultData.Token, username); await keychain.Save(host); return loginResultData; @@ -209,6 +200,25 @@ private async Task TryLogin( return new LoginResultData(LoginResultCodes.Failed, ret.GetApiErrorMessage() ?? "Failed.", host); } + + private async Task RetrieveUsername(LoginResultData loginResultData, string username) + { + if (!username.Contains("@")) + { + return username; + } + + var octorunTask = new OctorunTask(taskManager.Token, nodeJsExecutablePath.Value, octorunScript.Value, "validate", + user: username, userToken: loginResultData.Token).Configure(processManager); + + var validateResult = await octorunTask.StartAsAsync(); + if (!validateResult.IsSuccess) + { + throw new InvalidOperationException("Authentication validation failed"); + } + + return validateResult.Output[1]; + } } class LoginResultData From e101c3836573cc7f5153f1027129c7c1beaad186 Mon Sep 17 00:00:00 2001 From: Stanley Goldman Date: Fri, 20 Apr 2018 09:56:39 -0400 Subject: [PATCH 5/8] Getting username for regular logins as well --- src/GitHub.Api/Authentication/LoginManager.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/GitHub.Api/Authentication/LoginManager.cs b/src/GitHub.Api/Authentication/LoginManager.cs index 61307713f..fe893e2d5 100644 --- a/src/GitHub.Api/Authentication/LoginManager.cs +++ b/src/GitHub.Api/Authentication/LoginManager.cs @@ -84,7 +84,8 @@ public async Task Login( throw new InvalidOperationException("Returned token is null or empty"); } - keychain.SetToken(host, loginResultData.Token, null); + username = await RetrieveUsername(loginResultData, username); + keychain.SetToken(host, loginResultData.Token, username); if (loginResultData.Code == LoginResultCodes.Success) { From a832a97f7c76b50373d85873372b6426dd5b4e53 Mon Sep 17 00:00:00 2001 From: Stanley Goldman Date: Fri, 20 Apr 2018 09:57:28 -0400 Subject: [PATCH 6/8] Removing unused fields --- src/GitHub.Api/Application/ApiClient.cs | 2 +- src/GitHub.Api/Authentication/LoginManager.cs | 16 ++-------------- 2 files changed, 3 insertions(+), 15 deletions(-) diff --git a/src/GitHub.Api/Application/ApiClient.cs b/src/GitHub.Api/Application/ApiClient.cs index 2ea877a3f..067c05c76 100644 --- a/src/GitHub.Api/Application/ApiClient.cs +++ b/src/GitHub.Api/Application/ApiClient.cs @@ -33,7 +33,7 @@ public ApiClient(UriString hostUrl, IKeychain keychain, IProcessManager processM this.taskManager = taskManager; this.nodeJsExecutablePath = nodeJsExecutablePath; this.octorunScriptPath = octorunScriptPath; - loginManager = new LoginManager(keychain, ApplicationInfo.ClientId, ApplicationInfo.ClientSecret, + loginManager = new LoginManager(keychain, processManager: processManager, taskManager: taskManager, nodeJsExecutablePath: nodeJsExecutablePath, diff --git a/src/GitHub.Api/Authentication/LoginManager.cs b/src/GitHub.Api/Authentication/LoginManager.cs index fe893e2d5..0dfa49a9e 100644 --- a/src/GitHub.Api/Authentication/LoginManager.cs +++ b/src/GitHub.Api/Authentication/LoginManager.cs @@ -1,6 +1,4 @@ using System; -using System.Linq; -using System.Net; using System.Threading.Tasks; using GitHub.Logging; @@ -23,8 +21,6 @@ class LoginManager : ILoginManager private readonly ILogging logger = LogHelper.GetLogger(); private readonly IKeychain keychain; - private readonly string clientId; - private readonly string clientSecret; private readonly IProcessManager processManager; private readonly ITaskManager taskManager; private readonly NPath? nodeJsExecutablePath; @@ -34,25 +30,17 @@ class LoginManager : ILoginManager /// Initializes a new instance of the class. /// /// - /// The application's client API ID. - /// The application's client API secret. /// /// /// /// public LoginManager( - IKeychain keychain, - string clientId, - string clientSecret, - IProcessManager processManager = null, ITaskManager taskManager = null, NPath? nodeJsExecutablePath = null, NPath? octorunScript = null) + IKeychain keychain, IProcessManager processManager = null, ITaskManager taskManager = null, + NPath? nodeJsExecutablePath = null, NPath? octorunScript = null) { Guard.ArgumentNotNull(keychain, nameof(keychain)); - Guard.ArgumentNotNullOrWhiteSpace(clientId, nameof(clientId)); - Guard.ArgumentNotNullOrWhiteSpace(clientSecret, nameof(clientSecret)); this.keychain = keychain; - this.clientId = clientId; - this.clientSecret = clientSecret; this.processManager = processManager; this.taskManager = taskManager; this.nodeJsExecutablePath = nodeJsExecutablePath; From fe7f356e33266d7eef0008d3669d7c5976117334 Mon Sep 17 00:00:00 2001 From: Stanley Goldman Date: Fri, 20 Apr 2018 10:01:47 -0400 Subject: [PATCH 7/8] Making username required --- src/GitHub.Api/Authentication/Credential.cs | 8 ++------ src/GitHub.Api/Authentication/ICredentialManager.cs | 2 +- src/GitHub.Api/Authentication/KeychainAdapter.cs | 2 +- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/GitHub.Api/Authentication/Credential.cs b/src/GitHub.Api/Authentication/Credential.cs index 39aec2728..2e31f9838 100644 --- a/src/GitHub.Api/Authentication/Credential.cs +++ b/src/GitHub.Api/Authentication/Credential.cs @@ -16,14 +16,10 @@ public Credential(UriString host, string username, string token) this.Token = token; } - public void UpdateToken(string token, string username = null) + public void UpdateToken(string token, string username) { this.Token = token; - - if (username != null) - { - this.Username = username; - } + this.Username = username; } public UriString Host { get; private set; } diff --git a/src/GitHub.Api/Authentication/ICredentialManager.cs b/src/GitHub.Api/Authentication/ICredentialManager.cs index c087e9f07..b601d3633 100644 --- a/src/GitHub.Api/Authentication/ICredentialManager.cs +++ b/src/GitHub.Api/Authentication/ICredentialManager.cs @@ -8,7 +8,7 @@ public interface ICredential : IDisposable UriString Host { get; } string Username { get; } string Token { get; } - void UpdateToken(string token, string username = null); + void UpdateToken(string token, string username); } public interface ICredentialManager diff --git a/src/GitHub.Api/Authentication/KeychainAdapter.cs b/src/GitHub.Api/Authentication/KeychainAdapter.cs index bdfdc72ac..abbe9895e 100644 --- a/src/GitHub.Api/Authentication/KeychainAdapter.cs +++ b/src/GitHub.Api/Authentication/KeychainAdapter.cs @@ -9,7 +9,7 @@ public void Set(ICredential credential) Credential = credential; } - public void UpdateToken(string token, string username = null) + public void UpdateToken(string token, string username) { Credential.UpdateToken(token, username); } From b3db4cd5c04b4382ad79537adf79082143379e45 Mon Sep 17 00:00:00 2001 From: Stanley Goldman Date: Fri, 20 Apr 2018 12:25:39 -0400 Subject: [PATCH 8/8] Fixing unit test --- src/tests/UnitTests/Authentication/KeychainTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tests/UnitTests/Authentication/KeychainTests.cs b/src/tests/UnitTests/Authentication/KeychainTests.cs index 571c83ac1..23786626b 100644 --- a/src/tests/UnitTests/Authentication/KeychainTests.cs +++ b/src/tests/UnitTests/Authentication/KeychainTests.cs @@ -292,7 +292,7 @@ public void ShouldConnectSetCredentialsTokenAndSave() keychainAdapter.Credential.Username.Should().Be(username); keychainAdapter.Credential.Token.Should().Be(password); - keychain.SetToken(hostUri, token, null); + keychain.SetToken(hostUri, token, username); keychainAdapter.Credential.Should().NotBeNull(); keychainAdapter.Credential.Host.Should().Be(hostUri);