Skip to content
This repository was archived by the owner on Dec 5, 2024. It is now read-only.

Commit e57463b

Browse files
shanashana
committed
Dedupe code in ApiClient. Also, cache user data and avoid using the keychain username
The system keychain may have a valid token with an invalid username because every git client fills out that information slightly different and the username is not relevant for accessing the API (only the token is needed). If we encounter a mismatch, don't throw immediately (the token might be perfectly valid), but always doublecheck the github user to make sure the username of the connection matches the user that the token is associated with. Cache the GitHubUser object in the Connection list (non serialized) so that if we fill out once and all the information matches, we can reuse it and avoid pinging the API every time. If another git client tramples all over our keychain, it's highly likely that they'll also not fill out the username information anyway, and we always grab the user from GitHub if that happens so we should be relatively safe trusting the cached GitHubUser object (it'll get thrown away whenever Unity reloads so it's also likely that we'll keep refreshing it anyways) In the process of this, cleaned up some duplicated and uneeded code that could lead to issues later on.
1 parent 60da5ec commit e57463b

File tree

8 files changed

+51
-65
lines changed

8 files changed

+51
-65
lines changed

‎src/GitHub.Api/Application/ApiClient.cs‎

Lines changed: 41 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -10,17 +10,6 @@ namespace GitHub.Unity
1010
{
1111
class ApiClient : IApiClient
1212
{
13-
public static IApiClient Create(UriString repositoryUrl, IKeychain keychain, IProcessManager processManager, ITaskManager taskManager, NPath nodeJsExecutablePath, NPath octorunScriptPath)
14-
{
15-
logger.Trace("Creating ApiClient: {0}", repositoryUrl);
16-
17-
var credentialStore = keychain.Connect(repositoryUrl);
18-
var hostAddress = HostAddress.Create(repositoryUrl);
19-
20-
return new ApiClient(repositoryUrl, keychain,
21-
processManager, taskManager, nodeJsExecutablePath, octorunScriptPath);
22-
}
23-
2413
private static readonly ILogging logger = LogHelper.GetLogger<ApiClient>();
2514
public HostAddress HostAddress { get; }
2615
public UriString OriginalUrl { get; }
@@ -81,34 +70,20 @@ public async Task GetOrganizations(Action<Organization[]> onSuccess, Action<Exce
8170
await GetOrganizationInternal(onSuccess, onError);
8271
}
8372

84-
public async Task ValidateCurrentUser(Action onSuccess, Action<Exception> onError = null)
73+
public async Task GetCurrentUser(Action<GitHubUser> onSuccess, Action<Exception> onError = null)
8574
{
8675
Guard.ArgumentNotNull(onSuccess, nameof(onSuccess));
8776
try
8877
{
89-
var keychainConnection = keychain.Connections.First();
90-
var keychainAdapter = await GetValidatedKeychainAdapter(keychainConnection);
91-
await GetValidatedGitHubUser(keychainConnection, keychainAdapter);
92-
onSuccess();
78+
var user = await GetCurrentUser();
79+
onSuccess(user);
9380
}
9481
catch (Exception e)
9582
{
9683
onError?.Invoke(e);
9784
}
9885
}
9986

100-
public async Task GetCurrentUser(Action<GitHubUser> callback)
101-
{
102-
Guard.ArgumentNotNull(callback, "callback");
103-
104-
//TODO: ONE_USER_LOGIN This assumes only ever one user can login
105-
var keychainConnection = keychain.Connections.First();
106-
var keychainAdapter = await GetValidatedKeychainAdapter(keychainConnection);
107-
var user = await GetValidatedGitHubUser(keychainConnection, keychainAdapter);
108-
109-
callback(user);
110-
}
111-
11287
public async Task Login(string username, string password, Action<LoginResult> need2faCode, Action<bool, string> result)
11388
{
11489
Guard.ArgumentNotNull(need2faCode, "need2faCode");
@@ -205,16 +180,33 @@ public async Task<bool> ContinueLoginAsync(LoginResult loginResult, Func<LoginRe
205180
return result.Code == LoginResultCodes.Success;
206181
}
207182

183+
private async Task<GitHubUser> GetCurrentUser()
184+
{
185+
//TODO: ONE_USER_LOGIN This assumes we only support one login
186+
var keychainConnection = keychain.Connections.FirstOrDefault();
187+
if (keychainConnection == null)
188+
throw new KeychainEmptyException();
189+
190+
var keychainAdapter = await GetValidatedKeychainAdapter(keychainConnection);
191+
192+
// we can't trust that the system keychain has the username filled out correctly.
193+
// if it doesn't, we need to grab the username from the server and check it
194+
// unfortunately this means that things will be slower when the keychain doesn't have all the info
195+
if (keychainConnection.User == null || keychainAdapter.Credential.Username != keychainConnection.Username)
196+
{
197+
keychainConnection.User = await GetValidatedGitHubUser(keychainConnection, keychainAdapter);
198+
}
199+
return keychainConnection.User;
200+
}
201+
208202
private async Task<GitHubRepository> CreateRepositoryInternal(string repositoryName, string organization, string description, bool isPrivate)
209203
{
210204
try
211205
{
212206
logger.Trace("Creating repository");
213207

214-
//TODO: ONE_USER_LOGIN This assumes only ever one user can login
215-
var keychainConnection = keychain.Connections.First();
216-
var keychainAdapter = await GetValidatedKeychainAdapter(keychainConnection);
217-
await GetValidatedGitHubUser(keychainConnection, keychainAdapter);
208+
var user = await GetCurrentUser();
209+
var keychainAdapter = keychain.Connect(OriginalUrl);
218210

219211
var command = new StringBuilder("publish -r \"");
220212
command.Append(repositoryName);
@@ -240,7 +232,7 @@ private async Task<GitHubRepository> CreateRepositoryInternal(string repositoryN
240232
}
241233

242234
var octorunTask = new OctorunTask(taskManager.Token, nodeJsExecutablePath, octorunScriptPath, command.ToString(),
243-
user: keychainAdapter.Credential.Username, userToken: keychainAdapter.Credential.Token)
235+
user: user.Login, userToken: keychainAdapter.Credential.Token)
244236
.Configure(processManager);
245237

246238
var ret = await octorunTask.StartAwait();
@@ -273,13 +265,11 @@ private async Task GetOrganizationInternal(Action<Organization[]> onSuccess, Act
273265
{
274266
logger.Trace("Getting Organizations");
275267

276-
//TODO: ONE_USER_LOGIN This assumes only ever one user can login
277-
var keychainConnection = keychain.Connections.First();
278-
var keychainAdapter = await GetValidatedKeychainAdapter(keychainConnection);
279-
await GetValidatedGitHubUser(keychainConnection, keychainAdapter);
268+
var user = await GetCurrentUser();
269+
var keychainAdapter = keychain.Connect(OriginalUrl);
280270

281271
var octorunTask = new OctorunTask(taskManager.Token, nodeJsExecutablePath, octorunScriptPath, "organizations",
282-
user: keychainAdapter.Credential.Username, userToken: keychainAdapter.Credential.Token)
272+
user: user.Login, userToken: keychainAdapter.Credential.Token)
283273
.Configure(processManager);
284274

285275
var ret = await octorunTask.StartAsAsync();
@@ -315,35 +305,30 @@ private async Task GetOrganizationInternal(Action<Organization[]> onSuccess, Act
315305

316306
private async Task<IKeychainAdapter> GetValidatedKeychainAdapter(Connection keychainConnection)
317307
{
318-
if (keychain.HasKeys)
319-
{
320-
var keychainAdapter = await keychain.Load(keychainConnection.Host);
321-
322-
if (string.IsNullOrEmpty(keychainAdapter.Credential?.Username))
323-
{
324-
logger.Warning("LoadKeychainInternal: Username is empty");
325-
throw new TokenUsernameMismatchException(keychainConnection.Username);
326-
}
308+
var keychainAdapter = await keychain.Load(keychainConnection.Host);
309+
if (keychainAdapter == null)
310+
throw new KeychainEmptyException();
327311

328-
if (keychainAdapter.Credential.Username != keychainConnection.Username)
329-
{
330-
logger.Warning("LoadKeychainInternal: Token username does not match");
331-
throw new TokenUsernameMismatchException(keychainConnection.Username, keychainAdapter.Credential.Username);
332-
}
312+
if (string.IsNullOrEmpty(keychainAdapter.Credential?.Username))
313+
{
314+
logger.Warning("LoadKeychainInternal: Username is empty");
315+
throw new TokenUsernameMismatchException(keychainConnection.Username);
316+
}
333317

334-
return keychainAdapter;
318+
if (keychainAdapter.Credential.Username != keychainConnection.Username)
319+
{
320+
logger.Warning("LoadKeychainInternal: Token username does not match");
335321
}
336322

337-
logger.Warning("LoadKeychainInternal: No keys to load");
338-
throw new KeychainEmptyException();
323+
return keychainAdapter;
339324
}
340325

341326
private async Task<GitHubUser> GetValidatedGitHubUser(Connection keychainConnection, IKeychainAdapter keychainAdapter)
342327
{
343328
try
344329
{
345330
var octorunTask = new OctorunTask(taskManager.Token, nodeJsExecutablePath, octorunScriptPath, "validate",
346-
user: keychainAdapter.Credential.Username, userToken: keychainAdapter.Credential.Token)
331+
user: keychainConnection.Username, userToken: keychainAdapter.Credential.Token)
347332
.Configure(processManager);
348333

349334
var ret = await octorunTask.StartAsAsync();

‎src/GitHub.Api/Application/IApiClient.cs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ Task CreateRepository(string name, string description, bool isPrivate,
1414
Task ContinueLogin(LoginResult loginResult, string code);
1515
Task<bool> LoginAsync(string username, string password, Func<LoginResult, string> need2faCode);
1616
Task Logout(UriString host);
17-
Task GetCurrentUser(Action<GitHubUser> callback);
18-
Task ValidateCurrentUser(Action onSuccess, Action<Exception> onError = null);
17+
Task GetCurrentUser(Action<GitHubUser> onSuccess, Action<Exception> onError = null);
1918
}
2019
}

‎src/GitHub.Api/Authentication/Keychain.cs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ public class Connection
1212
{
1313
public string Host { get; set; }
1414
public string Username { get; set; }
15+
[NonSerialized] internal GitHubUser User;
1516

1617
// for json serialization
1718
public Connection()
@@ -99,7 +100,7 @@ public IKeychainAdapter Connect(UriString host)
99100

100101
public async Task<IKeychainAdapter> Load(UriString host)
101102
{
102-
KeychainAdapter keychainAdapter = FindOrCreateAdapter(host);
103+
var keychainAdapter = FindOrCreateAdapter(host);
103104
var connection = GetConnection(host);
104105

105106
//logger.Trace($@"Loading KeychainAdapter Host:""{host}"" Cached Username:""{cachedConnection.Username}""");
@@ -109,6 +110,7 @@ public async Task<IKeychainAdapter> Load(UriString host)
109110
{
110111
logger.Warning("Cannot load host from Credential Manager; removing from cache");
111112
await Clear(host, false);
113+
keychainAdapter = null;
112114
}
113115
else
114116
{

‎src/UnityExtension/Assets/Editor/GitHub.Unity/Services/AuthenticationService.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ class AuthenticationService
1010

1111
public AuthenticationService(UriString host, IKeychain keychain, NPath nodeJsExecutablePath, NPath octorunExecutablePath)
1212
{
13-
client = ApiClient.Create(host, keychain, EntryPoint.ApplicationManager.ProcessManager, EntryPoint.ApplicationManager.TaskManager, nodeJsExecutablePath, octorunExecutablePath);
13+
client = new ApiClient(host, keychain, EntryPoint.ApplicationManager.ProcessManager, EntryPoint.ApplicationManager.TaskManager, nodeJsExecutablePath, octorunExecutablePath);
1414
}
1515

1616
public void Login(string username, string password, Action<string> twofaRequired, Action<bool, string> authResult)

‎src/UnityExtension/Assets/Editor/GitHub.Unity/UI/PopupWindow.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ private void Open(PopupViewType popupViewType, Action<bool> onClose)
120120
{
121121
//Logger.Trace("Validating to open view");
122122

123-
Client.ValidateCurrentUser(() => {
123+
Client.GetCurrentUser(user => {
124124

125125
//Logger.Trace("User validated opening view");
126126

@@ -192,7 +192,7 @@ public IApiClient Client
192192
host = UriString.ToUriString(HostAddress.GitHubDotComHostAddress.WebUri);
193193
}
194194

195-
client = ApiClient.Create(host, Platform.Keychain, Manager.ProcessManager, TaskManager, Environment.NodeJsExecutablePath, Environment.OctorunScriptPath);
195+
client = new ApiClient(host, Platform.Keychain, Manager.ProcessManager, TaskManager, Environment.NodeJsExecutablePath, Environment.OctorunScriptPath);
196196
}
197197

198198
return client;

‎src/UnityExtension/Assets/Editor/GitHub.Unity/UI/PublishView.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ public IApiClient Client
5252
host = UriString.ToUriString(HostAddress.GitHubDotComHostAddress.WebUri);
5353
}
5454

55-
client = ApiClient.Create(host, Platform.Keychain, Manager.ProcessManager, TaskManager, Environment.NodeJsExecutablePath, Environment.OctorunScriptPath);
55+
client = new ApiClient(host, Platform.Keychain, Manager.ProcessManager, TaskManager, Environment.NodeJsExecutablePath, Environment.OctorunScriptPath);
5656
}
5757

5858
return client;

‎src/UnityExtension/Assets/Editor/GitHub.Unity/UI/Window.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -497,7 +497,7 @@ private void SignOut(object obj)
497497
host = UriString.ToUriString(HostAddress.GitHubDotComHostAddress.WebUri);
498498
}
499499

500-
var apiClient = ApiClient.Create(host, Platform.Keychain, null, null, NPath.Default, NPath.Default);
500+
var apiClient = new ApiClient(host, Platform.Keychain, null, null, NPath.Default, NPath.Default);
501501
apiClient.Logout(host);
502502
}
503503

‎src/tests/UnitTests/Authentication/KeychainTests.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,7 @@ public void ShouldDeleteFromCacheWhenLoadReturnsNullFromConnectionManager()
223223

224224
var uriString = keychain.Hosts.FirstOrDefault();
225225
var keychainAdapter = keychain.Load(uriString).Result;
226-
keychainAdapter.Credential.Should().BeNull();
226+
keychainAdapter.Should().BeNull();
227227

228228
fileSystem.DidNotReceive().FileExists(Args.String);
229229
fileSystem.DidNotReceive().ReadAllText(Args.String);

0 commit comments

Comments
 (0)