Skip to content

Commit aa4b6ab

Browse files
mjcheethammjcheetham
authored
Merge pull request #739 from mjcheetham/cred-noop
Avoid writing credentials when account & password have not changed
2 parents d6834d6 + 3490948 commit aa4b6ab

File tree

6 files changed

+97
-76
lines changed

6 files changed

+97
-76
lines changed

‎src/shared/Core/Interop/InteropUtils.cs

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using System;
2+
using System.Linq;
23
using System.Runtime.InteropServices;
34

45
namespace GitCredentialManager.Interop
@@ -11,5 +12,21 @@ public static byte[] ToByteArray(IntPtr ptr, long count)
1112
Marshal.Copy(ptr, destination, 0, destination.Length);
1213
return destination;
1314
}
15+
16+
public static bool AreEqual(byte[] bytes, IntPtr ptr, uint length)
17+
{
18+
if (bytes.Length == 0 && (ptr == IntPtr.Zero || length == 0))
19+
{
20+
return true;
21+
}
22+
23+
if (bytes.Length != length)
24+
{
25+
return false;
26+
}
27+
28+
byte[] ptrBytes = ToByteArray(ptr, length);
29+
return bytes.SequenceEqual(ptrBytes);
30+
}
1431
}
1532
}

‎src/shared/Core/Interop/Linux/SecretServiceCollection.cs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,16 @@ public unsafe void AddOrUpdate(string service, string account, string secret)
114114
SecretValue* secretValue = null;
115115
GError *error = null;
116116

117+
// If there is an existing credential that matches the same account and password
118+
// then don't bother writing out anything because they're the same!
119+
ICredential existingCred = Get(service, account);
120+
if (existingCred != null &&
121+
StringComparer.Ordinal.Equals(existingCred.Account, account) &&
122+
StringComparer.Ordinal.Equals(existingCred.Password, secret))
123+
{
124+
return;
125+
}
126+
117127
try
118128
{
119129
SecretService* secService = GetSecretService();

‎src/shared/Core/Interop/MacOS/MacOSKeychain.cs

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,6 @@ public void AddOrUpdate(string service, string account, string secret)
103103

104104
string serviceName = CreateServiceName(service);
105105

106-
107106
uint serviceNameLength = (uint) serviceName.Length;
108107
uint accountLength = (uint) (account?.Length ?? 0);
109108

@@ -112,12 +111,12 @@ public void AddOrUpdate(string service, string account, string secret)
112111
// Check if an entry already exists in the keychain
113112
int findResult = SecKeychainFindGenericPassword(
114113
IntPtr.Zero, serviceNameLength, serviceName, accountLength, account,
115-
out uint _, out passwordData, out itemRef);
114+
out uint passwordDataLength, out passwordData, out itemRef);
116115

117116
switch (findResult)
118117
{
119-
// Update existing entry
120-
case OK:
118+
// Update existing entry only if the password/secret is different
119+
case OK when !InteropUtils.AreEqual(secretBytes, passwordData, passwordDataLength):
121120
ThrowIfError(
122121
SecKeychainItemModifyAttributesAndData(itemRef, IntPtr.Zero, (uint) secretBytes.Length, secretBytes),
123122
"Could not update existing item"

‎src/shared/Core/Interop/Windows/Native/Advapi32.cs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
using System;
22
using System.Runtime.InteropServices;
3+
using System.Text;
34
using FILETIME = System.Runtime.InteropServices.ComTypes.FILETIME;
45

56
namespace GitCredentialManager.Interop.Windows.Native
@@ -93,5 +94,16 @@ public struct Win32Credential
9394
public string TargetAlias;
9495
[MarshalAs(UnmanagedType.LPWStr)]
9596
public string UserName;
97+
98+
public string GetCredentialBlobAsString()
99+
{
100+
if (CredentialBlobSize != 0 && CredentialBlob != IntPtr.Zero)
101+
{
102+
byte[] passwordBytes = InteropUtils.ToByteArray(CredentialBlob, CredentialBlobSize);
103+
return Encoding.Unicode.GetString(passwordBytes);
104+
}
105+
106+
return null;
107+
}
96108
}
97109
}

‎src/shared/Core/Interop/Windows/WindowsCredentialManager.cs

Lines changed: 10 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,6 @@ public class WindowsCredentialManager : ICredentialStore
1313

1414
private readonly string _namespace;
1515

16-
#region Constructors
17-
1816
/// <summary>
1917
/// Open the Windows Credential Manager vault for the current user.
2018
/// </summary>
@@ -26,10 +24,6 @@ public WindowsCredentialManager(string @namespace = null)
2624
_namespace = @namespace;
2725
}
2826

29-
#endregion
30-
31-
#region ICredentialStore
32-
3327
public ICredential Get(string service, string account)
3428
{
3529
return Enumerate(service, account).FirstOrDefault();
@@ -66,6 +60,15 @@ public void AddOrUpdate(string service, string account, string secret)
6660
// Create new entry with the account in the target name
6761
targetName = CreateTargetName(service, account);
6862
}
63+
else
64+
{
65+
// No need to write out credential if the account and secret/password are the same
66+
string existingSecret = existingCred.GetCredentialBlobAsString();
67+
if (StringComparer.Ordinal.Equals(existingSecret, secret))
68+
{
69+
return;
70+
}
71+
}
6972
}
7073

7174
byte[] secretBytes = Encoding.Unicode.GetBytes(secret);
@@ -129,8 +132,6 @@ public bool Remove(string service, string account)
129132
return false;
130133
}
131134

132-
#endregion
133-
134135
/// <summary>
135136
/// Check if we can persist credentials to for the current process and logon session.
136137
/// </summary>
@@ -205,14 +206,7 @@ private IEnumerable<WindowsCredential> Enumerate(string service, string account)
205206

206207
private WindowsCredential CreateCredentialFromStructure(Win32Credential credential)
207208
{
208-
string password = null;
209-
if (credential.CredentialBlobSize != 0 && credential.CredentialBlob != IntPtr.Zero)
210-
{
211-
byte[] passwordBytes = InteropUtils.ToByteArray(
212-
credential.CredentialBlob,
213-
credential.CredentialBlobSize);
214-
password = Encoding.Unicode.GetString(passwordBytes);
215-
}
209+
string password = credential.GetCredentialBlobAsString();
216210

217211
// Recover the target name we gave from the internal (raw) target name
218212
string targetName = credential.TargetName.TrimUntilIndexOf(TargetNameLegacyGenericPrefix);

‎src/shared/Core/PlaintextCredentialStore.cs

Lines changed: 45 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
using System;
22
using System.Collections.Generic;
33
using System.IO;
4+
using System.Linq;
45
using System.Text;
56

67
namespace GitCredentialManager
@@ -22,44 +23,26 @@ public PlaintextCredentialStore(IFileSystem fileSystem, string storeRoot, string
2223
protected string Namespace { get; }
2324
protected virtual string CredentialFileExtension => ".credential";
2425

25-
#region ICredentialStore
26-
2726
public ICredential Get(string service, string account)
2827
{
29-
string serviceSlug = CreateServiceSlug(service);
30-
string searchPath = Path.Combine(StoreRoot, serviceSlug);
31-
bool anyAccount = string.IsNullOrWhiteSpace(account);
32-
33-
if (!FileSystem.DirectoryExists(searchPath))
34-
{
35-
return null;
36-
}
37-
38-
IEnumerable<string> allFiles = FileSystem.EnumerateFiles(searchPath, $"*{CredentialFileExtension}");
39-
40-
foreach (string fullPath in allFiles)
41-
{
42-
string accountFile = Path.GetFileNameWithoutExtension(fullPath);
43-
if (anyAccount || StringComparer.OrdinalIgnoreCase.Equals(account, accountFile))
44-
{
45-
// Validate the credential metadata also matches our search
46-
if (TryDeserializeCredential(fullPath, out FileCredential credential) &&
47-
StringComparer.OrdinalIgnoreCase.Equals(service, credential.Service) &&
48-
(anyAccount || StringComparer.OrdinalIgnoreCase.Equals(account, credential.Account)))
49-
{
50-
return credential;
51-
}
52-
}
53-
}
54-
55-
return null;
28+
return Enumerate(service, account).FirstOrDefault();
5629
}
5730

5831
public void AddOrUpdate(string service, string account, string secret)
5932
{
6033
// Ensure the store root exists and permissions are set
6134
EnsureStoreRoot();
6235

36+
FileCredential existingCredential = Enumerate(service, account).FirstOrDefault();
37+
38+
// No need to update existing credential if nothing has changed
39+
if (existingCredential != null &&
40+
StringComparer.Ordinal.Equals(account, existingCredential.Account) &&
41+
StringComparer.Ordinal.Equals(secret, existingCredential.Password))
42+
{
43+
return;
44+
}
45+
6346
string serviceSlug = CreateServiceSlug(service);
6447
string servicePath = Path.Combine(StoreRoot, serviceSlug);
6548

@@ -75,39 +58,16 @@ public void AddOrUpdate(string service, string account, string secret)
7558

7659
public bool Remove(string service, string account)
7760
{
78-
string serviceSlug = CreateServiceSlug(service);
79-
string searchPath = Path.Combine(StoreRoot, serviceSlug);
80-
bool anyAccount = string.IsNullOrWhiteSpace(account);
81-
82-
if (!FileSystem.DirectoryExists(searchPath))
83-
{
84-
return false;
85-
}
86-
87-
IEnumerable<string> allFiles = FileSystem.EnumerateFiles(searchPath, $"*{CredentialFileExtension}");
88-
89-
foreach (string fullPath in allFiles)
61+
foreach (FileCredential credential in Enumerate(service, account))
9062
{
91-
string accountFile = Path.GetFileNameWithoutExtension(fullPath);
92-
if (anyAccount || StringComparer.OrdinalIgnoreCase.Equals(account, accountFile))
93-
{
94-
// Validate the credential metadata also matches our search
95-
if (TryDeserializeCredential(fullPath, out FileCredential credential) &&
96-
StringComparer.OrdinalIgnoreCase.Equals(service, credential.Service) &&
97-
(anyAccount || StringComparer.OrdinalIgnoreCase.Equals(account, credential.Account)))
98-
{
99-
// Delete the credential file
100-
FileSystem.DeleteFile(fullPath);
101-
return true;
102-
}
103-
}
63+
// Only delete the first match
64+
FileSystem.DeleteFile(credential.FullPath);
65+
return true;
10466
}
10567

10668
return false;
10769
}
10870

109-
#endregion
110-
11171
protected virtual bool TryDeserializeCredential(string path, out FileCredential credential)
11272
{
11373
string text;
@@ -162,6 +122,35 @@ protected virtual void SerializeCredential(FileCredential credential)
162122
}
163123
}
164124

125+
private IEnumerable<FileCredential> Enumerate(string service, string account)
126+
{
127+
string serviceSlug = CreateServiceSlug(service);
128+
string searchPath = Path.Combine(StoreRoot, serviceSlug);
129+
bool anyAccount = string.IsNullOrWhiteSpace(account);
130+
131+
if (!FileSystem.DirectoryExists(searchPath))
132+
{
133+
yield break;
134+
}
135+
136+
IEnumerable<string> allFiles = FileSystem.EnumerateFiles(searchPath, $"*{CredentialFileExtension}");
137+
138+
foreach (string fullPath in allFiles)
139+
{
140+
string accountFile = Path.GetFileNameWithoutExtension(fullPath);
141+
if (anyAccount || StringComparer.OrdinalIgnoreCase.Equals(account, accountFile))
142+
{
143+
// Validate the credential metadata also matches our search
144+
if (TryDeserializeCredential(fullPath, out FileCredential credential) &&
145+
StringComparer.OrdinalIgnoreCase.Equals(service, credential.Service) &&
146+
(anyAccount || StringComparer.OrdinalIgnoreCase.Equals(account, credential.Account)))
147+
{
148+
yield return credential;
149+
}
150+
}
151+
}
152+
}
153+
165154
/// <summary>
166155
/// Ensure the store root directory exists. If it does not, create a new directory with
167156
/// permissions that only permit the owner to read/write/execute. Permissions on an existing

0 commit comments

Comments
 (0)