Clean up the login api a bit and fix some issues

shana · shana · commit 43828e6ff8d9 · 2018-07-30T16:45:39.000+02:00
- Make sure we save the token before verifying the token,
	and save the new username after verifying it
- Minimize how many times we query the cred manager
diff --git a/src/GitHub.Api/Application/ApiClient.cs b/src/GitHub.Api/Application/ApiClient.cs
@@ -225,7 +225,7 @@ private GitHubUser GetCurrentUser()
 
         private IKeychainAdapter GetValidatedKeychainAdapter(Connection keychainConnection)
         {
-            var keychainAdapter = keychain.Load(keychainConnection.Host);
+            var keychainAdapter = keychain.LoadFromSystem(keychainConnection.Host);
             if (keychainAdapter == null)
                 throw new KeychainEmptyException();
 
diff --git a/src/GitHub.Api/Authentication/Credential.cs b/src/GitHub.Api/Authentication/Credential.cs
@@ -16,7 +16,7 @@ public Credential(UriString host, string username, string token)
             this.Token = token;
         }
 
-        public void UpdateToken(string token, string username)
+        public void Update(string token, string username)
         {
             this.Token = token;
             this.Username = username;
diff --git 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);
+        void Update(string token, string username);
     }
 
     public interface ICredentialManager
diff --git a/src/GitHub.Api/Authentication/IKeychain.cs b/src/GitHub.Api/Authentication/IKeychain.cs
@@ -6,15 +6,13 @@ namespace GitHub.Unity
     public interface IKeychain
     {
         IKeychainAdapter Connect(UriString host);
-        IKeychainAdapter Load(UriString host, bool dontClear = false);
+        IKeychainAdapter LoadFromSystem(UriString host);
         void Clear(UriString host, bool deleteFromCredentialManager);
-        void Save(UriString host);
-        void SetCredentials(ICredential credential);
+        void SaveToSystem(UriString host);
         void Initialize();
         Connection[] Connections { get; }
         IList<UriString> Hosts { get; }
         bool HasKeys { get; }
-        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
@@ -95,40 +95,35 @@ public Keychain(IEnvironment environment, ICredentialManager credentialManager)
         public IKeychainAdapter Connect(UriString host)
         {
             Guard.ArgumentNotNull(host, nameof(host));
-
             return FindOrCreateAdapter(host);
         }
 
-        public IKeychainAdapter Load(UriString host, bool dontClear = false)
+        public IKeychainAdapter LoadFromSystem(UriString host)
         {
             Guard.ArgumentNotNull(host, nameof(host));
 
             var keychainAdapter = Connect(host) as KeychainAdapter;
-            var keychainItem = credentialManager.Load(host);
-            if (keychainItem == null)
+            var credential = credentialManager.Load(host);
+            if (credential == null)
             {
-                if (!dontClear)
-                {
-                    logger.Warning("Cannot load host from Credential Manager; removing from cache");
-                    Clear(host, false);
-                }
+                logger.Warning("Cannot load host from Credential Manager; removing from cache");
+                Clear(host, false);
                 keychainAdapter = null;
             }
             else
             {
+                keychainAdapter.Set(credential);
                 var connection = GetConnection(host);
                 if (connection.Username == null)
                 {
-                    connection.Username = keychainItem.Username;
+                    connection.Username = credential.Username;
                     SaveConnectionsToDisk();
                 }
 
-                if (keychainItem.Username != connection.Username)
+                if (credential.Username != connection.Username)
                 {
-                    logger.Warning("Keychain Username:\"{0}\" does not match cached Username:\"{1}\"; Hopefully it works", keychainItem.Username, connection.Username);
+                    logger.Warning("Keychain Username:\"{0}\" does not match cached Username:\"{1}\"; Hopefully it works", credential.Username, connection.Username);
                 }
-
-                keychainAdapter.Set(keychainItem);
             }
             return keychainAdapter;
         }
@@ -159,32 +154,14 @@ public void Clear(UriString host, bool deleteFromCredentialManager)
             RemoveCredential(host, deleteFromCredentialManager);
         }
 
-        public void Save(UriString host)
+        public void SaveToSystem(UriString host)
         {
             Guard.ArgumentNotNull(host, nameof(host));
 
             var keychainAdapter = AddCredential(host);
             AddConnection(new Connection(host, keychainAdapter.Credential.Username));
         }
 
-        public void SetCredentials(ICredential credential)
-        {
-            Guard.ArgumentNotNull(credential, nameof(credential));
-
-            var keychainAdapter = GetKeychainAdapter(credential.Host);
-            keychainAdapter.Set(credential);
-        }
-
-        public void SetToken(UriString host, string token, string username)
-        {
-            Guard.ArgumentNotNull(host, nameof(host));
-            Guard.ArgumentNotNull(token, nameof(token));
-            Guard.ArgumentNotNull(username, nameof(username));
-
-            var keychainAdapter = GetKeychainAdapter(host);
-            keychainAdapter.UpdateToken(token, username);
-        }
-
         private void LoadConnectionsFromDisk()
         {
             if (cachePath.FileExists())
diff --git 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, string username)
+        public void Update(string token, string username)
         {
-            Credential.UpdateToken(token, username);
+            Credential.Update(token, username);
         }
 
         public void Clear()
@@ -23,5 +23,8 @@ public void Clear()
     public interface IKeychainAdapter
     {
         ICredential Credential { get; }
+        void Set(ICredential credential);
+        void Update(string token, string username);
+        void Clear();
     }
 }
diff --git a/src/GitHub.Api/Authentication/LoginManager.cs b/src/GitHub.Api/Authentication/LoginManager.cs
@@ -56,8 +56,8 @@ public LoginResultData Login(
 
             // Start by saving the username and password, these will be used by the `IGitHubClient`
             // until an authorization token has been created and acquired:
-            keychain.Connect(host);
-            keychain.SetCredentials(new Credential(host, username, password));
+            var keychainAdapter = keychain.Connect(host);
+            keychainAdapter.Set(new Credential(host, username, password));
 
             try
             {
@@ -69,16 +69,13 @@ public LoginResultData Login(
                         throw new InvalidOperationException("Returned token is null or empty");
                     }
 
-                    if (loginResultData.Code == LoginResultCodes.Success)
-                    {
-                        username = RetrieveUsername(loginResultData, username);
-                    }
-
-                    keychain.SetToken(host, loginResultData.Token, username);
+                    keychainAdapter.Update(loginResultData.Token, username);
 
                     if (loginResultData.Code == LoginResultCodes.Success)
                     {
-                        keychain.Save(host);
+                        username = RetrieveUsername(loginResultData, username);
+                        keychainAdapter.Update(loginResultData.Token, username);
+                        keychain.SaveToSystem(host);
                     }
 
                     return loginResultData;
@@ -99,6 +96,9 @@ public LoginResultData ContinueLogin(LoginResultData loginResultData, string two
         {
             var host = loginResultData.Host;
             var keychainAdapter = keychain.Connect(host);
+            if (keychainAdapter.Credential == null) {
+                return new LoginResultData(LoginResultCodes.Failed, Localization.LoginFailed, host);
+            }
             var username = keychainAdapter.Credential.Username;
             var password = keychainAdapter.Credential.Token;
             try
@@ -112,9 +112,10 @@ public LoginResultData ContinueLogin(LoginResultData loginResultData, string two
                         throw new InvalidOperationException("Returned token is null or empty");
                     }
 
+                    keychainAdapter.Update(loginResultData.Token, username);
                     username = RetrieveUsername(loginResultData, username);
-                    keychain.SetToken(host, loginResultData.Token, username);
-                    keychain.Save(host);
+                    keychainAdapter.Update(loginResultData.Token, username);
+                    keychain.SaveToSystem(host);
 
                     return loginResultData;
                 }
diff --git a/src/GitHub.Api/Tasks/OctorunTask.cs b/src/GitHub.Api/Tasks/OctorunTask.cs
@@ -77,9 +77,18 @@ public OctorunTask(CancellationToken token, IKeychain keychain, IEnvironment env
                 : new UriString(cloneUrl.ToRepositoryUri()
                     .GetComponents(UriComponents.SchemeAndServer, UriFormat.SafeUnescaped));
 
-            var adapter = keychain.Load(host, true);
-            if (adapter != null)
+            var adapter = keychain.Connect(host);
+            if (adapter.Credential?.Token != null)
+            {
                 userToken = adapter.Credential.Token;
+            }
+            else
+            {
+                // use a cached adapter if there is one filled out
+                adapter = keychain.LoadFromSystem(host);
+                if (adapter != null)
+                    userToken = adapter.Credential.Token;
+            }
         }
 
         public override void Configure(ProcessStartInfo psi)
diff --git a/src/tests/UnitTests/Authentication/KeychainTests.cs b/src/tests/UnitTests/Authentication/KeychainTests.cs
@@ -176,7 +176,7 @@ public void ShouldLoadFromConnectionManager()
             fileSystem.DidNotReceive().WriteAllLines(Args.String, Arg.Any<string[]>());
 
             var uriString = keychain.Hosts.FirstOrDefault();
-            var keychainAdapter = keychain.Load(uriString);
+            var keychainAdapter = keychain.LoadFromSystem(uriString);
             keychainAdapter.Credential.Username.Should().Be(username);
             keychainAdapter.Credential.Token.Should().Be(token);
             keychainAdapter.Credential.Host.Should().Be(hostUri);
@@ -222,7 +222,7 @@ public void ShouldDeleteFromCacheWhenLoadReturnsNullFromConnectionManager()
             fileSystem.ClearReceivedCalls();
 
             var uriString = keychain.Hosts.FirstOrDefault();
-            var keychainAdapter = keychain.Load(uriString);
+            var keychainAdapter = keychain.LoadFromSystem(uriString);
             keychainAdapter.Should().BeNull();
 
             fileSystem.DidNotReceive().FileExists(Args.String);
@@ -281,21 +281,21 @@ public void ShouldConnectSetCredentialsTokenAndSave()
 
             keychainAdapter.Credential.Should().BeNull();
 
-            keychain.SetCredentials(new Credential(hostUri, username, password));
+            keychainAdapter.Set(new Credential(hostUri, username, password));
 
             keychainAdapter.Credential.Should().NotBeNull();
             keychainAdapter.Credential.Host.Should().Be(hostUri);
             keychainAdapter.Credential.Username.Should().Be(username);
             keychainAdapter.Credential.Token.Should().Be(password);
 
-            keychain.SetToken(hostUri, token, username);
+            keychainAdapter.Update(token, username);
 
             keychainAdapter.Credential.Should().NotBeNull();
             keychainAdapter.Credential.Host.Should().Be(hostUri);
             keychainAdapter.Credential.Username.Should().Be(username);
             keychainAdapter.Credential.Token.Should().Be(token);
 
-            keychain.Save(hostUri);
+            keychain.SaveToSystem(hostUri);
 
             fileSystem.DidNotReceive().FileExists(Args.String);
             fileSystem.DidNotReceive().FileDelete(Args.String);
@@ -352,7 +352,7 @@ public void ShouldConnectSetCredentialsAndClear()
 
             keychainAdapter.Credential.Should().BeNull();
 
-            keychain.SetCredentials(new Credential(hostUri, username, password));
+            keychainAdapter.Set(new Credential(hostUri, username, password));
 
             keychainAdapter.Credential.Should().NotBeNull();
             keychainAdapter.Credential.Host.Should().Be(hostUri);

Original file line number	Diff line number	Diff line change
`@@ -225,7 +225,7 @@ private GitHubUser GetCurrentUser()`
`225`	`225`
`226`	`226`	`private IKeychainAdapter GetValidatedKeychainAdapter(Connection keychainConnection)`
`227`	`227`	`{`
`228`		`- var keychainAdapter = keychain.Load(keychainConnection.Host);`
	`228`	`+ var keychainAdapter = keychain.LoadFromSystem(keychainConnection.Host);`
`229`	`229`	`if (keychainAdapter == null)`
`230`	`230`	`throw new KeychainEmptyException();`
`231`	`231`
Original file line number	Diff line number	Diff line change
`@@ -16,7 +16,7 @@ public Credential(UriString host, string username, string token)`
`16`	`16`	`this.Token = token;`
`17`	`17`	`}`
`18`	`18`
`19`		`- public void UpdateToken(string token, string username)`
	`19`	`+ public void Update(string token, string username)`
`20`	`20`	`{`
`21`	`21`	`this.Token = token;`
`22`	`22`	`this.Username = username;`
Original file line number	Diff line number	Diff line change
`@@ -8,7 +8,7 @@ public interface ICredential : IDisposable`
`8`	`8`	`UriString Host { get; }`
`9`	`9`	`string Username { get; }`
`10`	`10`	`string Token { get; }`
`11`		`- void UpdateToken(string token, string username);`
	`11`	`+ void Update(string token, string username);`
`12`	`12`	`}`
`13`	`13`
`14`	`14`	`public interface ICredentialManager`
Original file line number	Diff line number	Diff line change
`@@ -95,40 +95,35 @@ public Keychain(IEnvironment environment, ICredentialManager credentialManager)`
`95`	`95`	`public IKeychainAdapter Connect(UriString host)`
`96`	`96`	`{`
`97`	`97`	`Guard.ArgumentNotNull(host, nameof(host));`
`98`		`-`
`99`	`98`	`return FindOrCreateAdapter(host);`
`100`	`99`	`}`
`101`	`100`
`102`		`- public IKeychainAdapter Load(UriString host, bool dontClear = false)`
	`101`	`+ public IKeychainAdapter LoadFromSystem(UriString host)`
`103`	`102`	`{`
`104`	`103`	`Guard.ArgumentNotNull(host, nameof(host));`
`105`	`104`
`106`	`105`	`var keychainAdapter = Connect(host) as KeychainAdapter;`
`107`		`- var keychainItem = credentialManager.Load(host);`
`108`		`- if (keychainItem == null)`
	`106`	`+ var credential = credentialManager.Load(host);`
	`107`	`+ if (credential == null)`
`109`	`108`	`{`
`110`		`- if (!dontClear)`
`111`		`- {`
`112`		`- logger.Warning("Cannot load host from Credential Manager; removing from cache");`
`113`		`- Clear(host, false);`
`114`		`- }`
	`109`	`+ logger.Warning("Cannot load host from Credential Manager; removing from cache");`
	`110`	`+ Clear(host, false);`
`115`	`111`	`keychainAdapter = null;`
`116`	`112`	`}`
`117`	`113`	`else`
`118`	`114`	`{`
	`115`	`+ keychainAdapter.Set(credential);`
`119`	`116`	`var connection = GetConnection(host);`
`120`	`117`	`if (connection.Username == null)`
`121`	`118`	`{`
`122`		`- connection.Username = keychainItem.Username;`
	`119`	`+ connection.Username = credential.Username;`
`123`	`120`	`SaveConnectionsToDisk();`
`124`	`121`	`}`
`125`	`122`
`126`		`- if (keychainItem.Username != connection.Username)`
	`123`	`+ if (credential.Username != connection.Username)`
`127`	`124`	`{`
`128`		`- logger.Warning("Keychain Username:\"{0}\" does not match cached Username:\"{1}\"; Hopefully it works", keychainItem.Username, connection.Username);`
	`125`	`+ logger.Warning("Keychain Username:\"{0}\" does not match cached Username:\"{1}\"; Hopefully it works", credential.Username, connection.Username);`
`129`	`126`	`}`
`130`		`-`
`131`		`- keychainAdapter.Set(keychainItem);`
`132`	`127`	`}`
`133`	`128`	`return keychainAdapter;`
`134`	`129`	`}`
`@@ -159,32 +154,14 @@ public void Clear(UriString host, bool deleteFromCredentialManager)`
`159`	`154`	`RemoveCredential(host, deleteFromCredentialManager);`
`160`	`155`	`}`
`161`	`156`
`162`		`- public void Save(UriString host)`
	`157`	`+ public void SaveToSystem(UriString host)`
`163`	`158`	`{`
`164`	`159`	`Guard.ArgumentNotNull(host, nameof(host));`
`165`	`160`
`166`	`161`	`var keychainAdapter = AddCredential(host);`
`167`	`162`	`AddConnection(new Connection(host, keychainAdapter.Credential.Username));`
`168`	`163`	`}`
`169`	`164`
`170`		`- public void SetCredentials(ICredential credential)`
`171`		`- {`
`172`		`- Guard.ArgumentNotNull(credential, nameof(credential));`
`173`		`-`
`174`		`- var keychainAdapter = GetKeychainAdapter(credential.Host);`
`175`		`- keychainAdapter.Set(credential);`
`176`		`- }`
`177`		`-`
`178`		`- public void SetToken(UriString host, string token, string username)`
`179`		`- {`
`180`		`- Guard.ArgumentNotNull(host, nameof(host));`
`181`		`- Guard.ArgumentNotNull(token, nameof(token));`
`182`		`- Guard.ArgumentNotNull(username, nameof(username));`
`183`		`-`
`184`		`- var keychainAdapter = GetKeychainAdapter(host);`
`185`		`- keychainAdapter.UpdateToken(token, username);`
`186`		`- }`
`187`		`-`
`188`	`165`	`private void LoadConnectionsFromDisk()`
`189`	`166`	`{`
`190`	`167`	`if (cachePath.FileExists())`
Original file line number	Diff line number	Diff line change
`@@ -9,9 +9,9 @@ public void Set(ICredential credential)`
`9`	`9`	`Credential = credential;`
`10`	`10`	`}`
`11`	`11`
`12`		`- public void UpdateToken(string token, string username)`
	`12`	`+ public void Update(string token, string username)`
`13`	`13`	`{`
`14`		`- Credential.UpdateToken(token, username);`
	`14`	`+ Credential.Update(token, username);`
`15`	`15`	`}`
`16`	`16`
`17`	`17`	`public void Clear()`
`@@ -23,5 +23,8 @@ public void Clear()`
`23`	`23`	`public interface IKeychainAdapter`
`24`	`24`	`{`
`25`	`25`	`ICredential Credential { get; }`
	`26`	`+ void Set(ICredential credential);`
	`27`	`+ void Update(string token, string username);`
	`28`	`+ void Clear();`
`26`	`29`	`}`
`27`	`30`	`}`