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

Commit 43828e6

Browse files
shanashana
committed
Clean up the login api a bit and fix some issues
- 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
1 parent c3ae9a0 commit 43828e6

File tree

9 files changed

+49
-61
lines changed

9 files changed

+49
-61
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -225,7 +225,7 @@ private GitHubUser GetCurrentUser()
225225

226226
private IKeychainAdapter GetValidatedKeychainAdapter(Connection keychainConnection)
227227
{
228-
var keychainAdapter = keychain.Load(keychainConnection.Host);
228+
var keychainAdapter = keychain.LoadFromSystem(keychainConnection.Host);
229229
if (keychainAdapter == null)
230230
throw new KeychainEmptyException();
231231

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ public Credential(UriString host, string username, string token)
1616
this.Token = token;
1717
}
1818

19-
public void UpdateToken(string token, string username)
19+
public void Update(string token, string username)
2020
{
2121
this.Token = token;
2222
this.Username = username;

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ public interface ICredential : IDisposable
88
UriString Host { get; }
99
string Username { get; }
1010
string Token { get; }
11-
void UpdateToken(string token, string username);
11+
void Update(string token, string username);
1212
}
1313

1414
public interface ICredentialManager

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,13 @@ namespace GitHub.Unity
66
public interface IKeychain
77
{
88
IKeychainAdapter Connect(UriString host);
9-
IKeychainAdapter Load(UriString host, bool dontClear = false);
9+
IKeychainAdapter LoadFromSystem(UriString host);
1010
void Clear(UriString host, bool deleteFromCredentialManager);
11-
void Save(UriString host);
12-
void SetCredentials(ICredential credential);
11+
void SaveToSystem(UriString host);
1312
void Initialize();
1413
Connection[] Connections { get; }
1514
IList<UriString> Hosts { get; }
1615
bool HasKeys { get; }
17-
void SetToken(UriString host, string token, string username);
1816

1917
event Action ConnectionsChanged;
2018
}

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

Lines changed: 10 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -95,40 +95,35 @@ public Keychain(IEnvironment environment, ICredentialManager credentialManager)
9595
public IKeychainAdapter Connect(UriString host)
9696
{
9797
Guard.ArgumentNotNull(host, nameof(host));
98-
9998
return FindOrCreateAdapter(host);
10099
}
101100

102-
public IKeychainAdapter Load(UriString host, bool dontClear = false)
101+
public IKeychainAdapter LoadFromSystem(UriString host)
103102
{
104103
Guard.ArgumentNotNull(host, nameof(host));
105104

106105
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)
109108
{
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);
115111
keychainAdapter = null;
116112
}
117113
else
118114
{
115+
keychainAdapter.Set(credential);
119116
var connection = GetConnection(host);
120117
if (connection.Username == null)
121118
{
122-
connection.Username = keychainItem.Username;
119+
connection.Username = credential.Username;
123120
SaveConnectionsToDisk();
124121
}
125122

126-
if (keychainItem.Username != connection.Username)
123+
if (credential.Username != connection.Username)
127124
{
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);
129126
}
130-
131-
keychainAdapter.Set(keychainItem);
132127
}
133128
return keychainAdapter;
134129
}
@@ -159,32 +154,14 @@ public void Clear(UriString host, bool deleteFromCredentialManager)
159154
RemoveCredential(host, deleteFromCredentialManager);
160155
}
161156

162-
public void Save(UriString host)
157+
public void SaveToSystem(UriString host)
163158
{
164159
Guard.ArgumentNotNull(host, nameof(host));
165160

166161
var keychainAdapter = AddCredential(host);
167162
AddConnection(new Connection(host, keychainAdapter.Credential.Username));
168163
}
169164

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-
188165
private void LoadConnectionsFromDisk()
189166
{
190167
if (cachePath.FileExists())

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,9 @@ public void Set(ICredential credential)
99
Credential = credential;
1010
}
1111

12-
public void UpdateToken(string token, string username)
12+
public void Update(string token, string username)
1313
{
14-
Credential.UpdateToken(token, username);
14+
Credential.Update(token, username);
1515
}
1616

1717
public void Clear()
@@ -23,5 +23,8 @@ public void Clear()
2323
public interface IKeychainAdapter
2424
{
2525
ICredential Credential { get; }
26+
void Set(ICredential credential);
27+
void Update(string token, string username);
28+
void Clear();
2629
}
2730
}

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

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,8 @@ public LoginResultData Login(
5656

5757
// Start by saving the username and password, these will be used by the `IGitHubClient`
5858
// until an authorization token has been created and acquired:
59-
keychain.Connect(host);
60-
keychain.SetCredentials(new Credential(host, username, password));
59+
var keychainAdapter = keychain.Connect(host);
60+
keychainAdapter.Set(new Credential(host, username, password));
6161

6262
try
6363
{
@@ -69,16 +69,13 @@ public LoginResultData Login(
6969
throw new InvalidOperationException("Returned token is null or empty");
7070
}
7171

72-
if (loginResultData.Code == LoginResultCodes.Success)
73-
{
74-
username = RetrieveUsername(loginResultData, username);
75-
}
76-
77-
keychain.SetToken(host, loginResultData.Token, username);
72+
keychainAdapter.Update(loginResultData.Token, username);
7873

7974
if (loginResultData.Code == LoginResultCodes.Success)
8075
{
81-
keychain.Save(host);
76+
username = RetrieveUsername(loginResultData, username);
77+
keychainAdapter.Update(loginResultData.Token, username);
78+
keychain.SaveToSystem(host);
8279
}
8380

8481
return loginResultData;
@@ -99,6 +96,9 @@ public LoginResultData ContinueLogin(LoginResultData loginResultData, string two
9996
{
10097
var host = loginResultData.Host;
10198
var keychainAdapter = keychain.Connect(host);
99+
if (keychainAdapter.Credential == null) {
100+
return new LoginResultData(LoginResultCodes.Failed, Localization.LoginFailed, host);
101+
}
102102
var username = keychainAdapter.Credential.Username;
103103
var password = keychainAdapter.Credential.Token;
104104
try
@@ -112,9 +112,10 @@ public LoginResultData ContinueLogin(LoginResultData loginResultData, string two
112112
throw new InvalidOperationException("Returned token is null or empty");
113113
}
114114

115+
keychainAdapter.Update(loginResultData.Token, username);
115116
username = RetrieveUsername(loginResultData, username);
116-
keychain.SetToken(host, loginResultData.Token, username);
117-
keychain.Save(host);
117+
keychainAdapter.Update(loginResultData.Token, username);
118+
keychain.SaveToSystem(host);
118119

119120
return loginResultData;
120121
}

‎src/GitHub.Api/Tasks/OctorunTask.cs

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,9 +77,18 @@ public OctorunTask(CancellationToken token, IKeychain keychain, IEnvironment env
7777
: new UriString(cloneUrl.ToRepositoryUri()
7878
.GetComponents(UriComponents.SchemeAndServer, UriFormat.SafeUnescaped));
7979

80-
var adapter = keychain.Load(host, true);
81-
if (adapter != null)
80+
var adapter = keychain.Connect(host);
81+
if (adapter.Credential?.Token != null)
82+
{
8283
userToken = adapter.Credential.Token;
84+
}
85+
else
86+
{
87+
// use a cached adapter if there is one filled out
88+
adapter = keychain.LoadFromSystem(host);
89+
if (adapter != null)
90+
userToken = adapter.Credential.Token;
91+
}
8392
}
8493

8594
public override void Configure(ProcessStartInfo psi)

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ public void ShouldLoadFromConnectionManager()
176176
fileSystem.DidNotReceive().WriteAllLines(Args.String, Arg.Any<string[]>());
177177

178178
var uriString = keychain.Hosts.FirstOrDefault();
179-
var keychainAdapter = keychain.Load(uriString);
179+
var keychainAdapter = keychain.LoadFromSystem(uriString);
180180
keychainAdapter.Credential.Username.Should().Be(username);
181181
keychainAdapter.Credential.Token.Should().Be(token);
182182
keychainAdapter.Credential.Host.Should().Be(hostUri);
@@ -222,7 +222,7 @@ public void ShouldDeleteFromCacheWhenLoadReturnsNullFromConnectionManager()
222222
fileSystem.ClearReceivedCalls();
223223

224224
var uriString = keychain.Hosts.FirstOrDefault();
225-
var keychainAdapter = keychain.Load(uriString);
225+
var keychainAdapter = keychain.LoadFromSystem(uriString);
226226
keychainAdapter.Should().BeNull();
227227

228228
fileSystem.DidNotReceive().FileExists(Args.String);
@@ -281,21 +281,21 @@ public void ShouldConnectSetCredentialsTokenAndSave()
281281

282282
keychainAdapter.Credential.Should().BeNull();
283283

284-
keychain.SetCredentials(new Credential(hostUri, username, password));
284+
keychainAdapter.Set(new Credential(hostUri, username, password));
285285

286286
keychainAdapter.Credential.Should().NotBeNull();
287287
keychainAdapter.Credential.Host.Should().Be(hostUri);
288288
keychainAdapter.Credential.Username.Should().Be(username);
289289
keychainAdapter.Credential.Token.Should().Be(password);
290290

291-
keychain.SetToken(hostUri, token, username);
291+
keychainAdapter.Update(token, username);
292292

293293
keychainAdapter.Credential.Should().NotBeNull();
294294
keychainAdapter.Credential.Host.Should().Be(hostUri);
295295
keychainAdapter.Credential.Username.Should().Be(username);
296296
keychainAdapter.Credential.Token.Should().Be(token);
297297

298-
keychain.Save(hostUri);
298+
keychain.SaveToSystem(hostUri);
299299

300300
fileSystem.DidNotReceive().FileExists(Args.String);
301301
fileSystem.DidNotReceive().FileDelete(Args.String);
@@ -352,7 +352,7 @@ public void ShouldConnectSetCredentialsAndClear()
352352

353353
keychainAdapter.Credential.Should().BeNull();
354354

355-
keychain.SetCredentials(new Credential(hostUri, username, password));
355+
keychainAdapter.Set(new Credential(hostUri, username, password));
356356

357357
keychainAdapter.Credential.Should().NotBeNull();
358358
keychainAdapter.Credential.Host.Should().Be(hostUri);

0 commit comments

Comments
 (0)