Allow login with email (#690)

* Validating the authentication result in order to ensure that we have the username

* Removing the UpdateToken method because it is never used

* Making username required in SetToken

* Centralize function to retrieve username

* Getting username for regular logins as well

* Removing unused fields

* Making username required

* Fixing unit test

Author: StanleyGoldman

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,

src/GitHub.Api/Authentication/Credential.cs

@@ -16,9 +16,10 @@ public Credential(UriString host, string username, string token)
             this.Token = token;
         }
 
-        public void UpdateToken(string token)
+        public void UpdateToken(string token, string username)
         {
             this.Token = token;
+            this.Username = username;
         }
 
         public UriString Host { get; private set; }

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);
     }
 
     public interface ICredentialManager

src/GitHub.Api/Authentication/IKeychain.cs

@@ -10,13 +10,12 @@ public interface IKeychain
         Task<IKeychainAdapter> 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; }
         IList<UriString> Hosts { get; }
         bool HasKeys { get; }
-        void SetToken(UriString host, string token);
+        void SetToken(UriString host, string token, string username);
 
         event Action ConnectionsChanged;
     }

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<IKeychainAdapter> 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,30 +160,33 @@ 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));
         }
 
         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)
+        public void SetToken(UriString host, string token, string username)
         {
             //logger.Trace("SetToken Host:{0}", host);
-            var keychainAdapter = GetKeychainAdapter(host);
-            keychainAdapter.UpdateToken(token);
-        }
 
-        public void UpdateToken(UriString host, string token)
-        {
-            //logger.Trace("UpdateToken Host:{0}", host);
+            Guard.ArgumentNotNull(host, nameof(host));
+            Guard.ArgumentNotNull(token, nameof(token));
+            Guard.ArgumentNotNull(username, nameof(username));
+
             var keychainAdapter = GetKeychainAdapter(host);
-            var keychainItem = keychainAdapter.Credential;
-            keychainItem.UpdateToken(token);
+            keychainAdapter.UpdateToken(token, username);
         }
 
         private void LoadConnectionsFromDisk()
@@ -290,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;

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)
         {
-            Credential.UpdateToken(token);
+            Credential.UpdateToken(token, username);
         }
 
         public void Clear()

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<LoginManager>();
 
         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 <see cref="LoginManager"/> class.
         /// </summary>
         /// <param name="keychain"></param>
-        /// <param name="clientId">The application's client API ID.</param>
-        /// <param name="clientSecret">The application's client API secret.</param>
         /// <param name="processManager"></param>
         /// <param name="taskManager"></param>
         /// <param name="nodeJsExecutablePath"></param>
         /// <param name="octorunScript"></param>
         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;
@@ -84,7 +72,8 @@ public async Task<LoginResultData> Login(
                         throw new InvalidOperationException("Returned token is null or empty");
                     }
 
-                    keychain.SetToken(host, loginResultData.Token);
+                    username = await RetrieveUsername(loginResultData, username);
+                    keychain.SetToken(host, loginResultData.Token, username);
 
                     if (loginResultData.Code == LoginResultCodes.Success)
                     {
@@ -123,7 +112,8 @@ public async Task<LoginResultData> ContinueLogin(LoginResultData loginResultData
                         throw new InvalidOperationException("Returned token is null or empty");
                     }
 
-                    keychain.SetToken(host, loginResultData.Token);
+                    username = await RetrieveUsername(loginResultData, username);
+                    keychain.SetToken(host, loginResultData.Token, username);
                     await keychain.Save(host);
 
                     return loginResultData;
@@ -199,6 +189,25 @@ private async Task<LoginResultData> TryLogin(
 
             return new LoginResultData(LoginResultCodes.Failed, ret.GetApiErrorMessage() ?? "Failed.", host);
         }
+
+        private async Task<string> 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

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, username);
 
             keychainAdapter.Credential.Should().NotBeNull();
             keychainAdapter.Credential.Host.Should().Be(hostUri);