-
Notifications
You must be signed in to change notification settings - Fork 933
Refactor user secrets management to use DI-based factory pattern with thread-safe synchronization #12482
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor user secrets management to use DI-based factory pattern with thread-safe synchronization #12482
Changes from 7 commits
bdfddf4
9222e94
02de777
b852826
ed54150
2c9ad74
482e759
8ab3628
c83d9cf
6308f05
f557876
65060b4
998374d
17002e3
61f9af9
4f29f84
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,39 +49,50 @@ public SecretsStore(string userSecretsId) | |
|
|
||
| public void Save() | ||
| { | ||
| EnsureUserSecretsDirectory(); | ||
|
|
||
| var contents = new JsonObject(); | ||
| if (_secrets is not null) | ||
| var semaphore = UserSecretsFileLock.GetSemaphore(_secretsFilePath); | ||
| semaphore.Wait(); | ||
| try | ||
| { | ||
| foreach (var secret in _secrets.AsEnumerable()) | ||
| // Reload from disk to merge with any concurrent changes | ||
| var currentSecrets = Load(_secretsFilePath); | ||
|
|
||
| // Merge our changes with what's on disk | ||
| foreach (var kvp in _secrets) | ||
| { | ||
| currentSecrets[kvp.Key] = kvp.Value; | ||
| } | ||
|
||
|
|
||
| EnsureUserSecretsDirectory(); | ||
|
|
||
| var contents = new JsonObject(); | ||
| foreach (var secret in currentSecrets) | ||
| { | ||
| contents[secret.Key] = secret.Value; | ||
| } | ||
| } | ||
|
|
||
| // Create a temp file with the correct Unix file mode before moving it to the expected _filePath. | ||
| if (!OperatingSystem.IsWindows()) | ||
| { | ||
| var tempFilename = Path.GetTempFileName(); | ||
| File.Move(tempFilename, _secretsFilePath, overwrite: true); | ||
| } | ||
| // Create a temp file with the correct Unix file mode before moving it to the expected _filePath. | ||
| if (!OperatingSystem.IsWindows()) | ||
| { | ||
| var tempFilename = Path.GetTempFileName(); | ||
| File.Move(tempFilename, _secretsFilePath, overwrite: true); | ||
| } | ||
|
|
||
| var json = contents.ToJsonString(new() | ||
| { | ||
| WriteIndented = true | ||
| }); | ||
| var json = contents.ToJsonString(new() | ||
| { | ||
| WriteIndented = true | ||
| }); | ||
|
|
||
| File.WriteAllText(_secretsFilePath, json, Encoding.UTF8); | ||
| File.WriteAllText(_secretsFilePath, json, Encoding.UTF8); | ||
| } | ||
| finally | ||
| { | ||
| semaphore.Release(); | ||
| } | ||
| } | ||
|
|
||
| private void EnsureUserSecretsDirectory() | ||
| { | ||
| var directoryName = Path.GetDirectoryName(_secretsFilePath); | ||
| if (!string.IsNullOrEmpty(directoryName) && !Directory.Exists(directoryName)) | ||
| { | ||
| Directory.CreateDirectory(directoryName); | ||
| } | ||
| EnsureUserSecretsDirectory(_secretsFilePath); | ||
| } | ||
|
|
||
| private static Dictionary<string, string?> Load(string secretsFilePath) | ||
|
|
@@ -130,14 +141,55 @@ public static bool TrySetUserSecret(Assembly? assembly, string name, string valu | |
| // Save the value to the secret store | ||
| try | ||
| { | ||
| var secretsStore = new SecretsStore(userSecretsId); | ||
| secretsStore.Set(name, value); | ||
| secretsStore.Save(); | ||
| return true; | ||
| var secretsFilePath = PathHelper.GetSecretsPathFromSecretsId(userSecretsId); | ||
| var semaphore = UserSecretsFileLock.GetSemaphore(secretsFilePath); | ||
| semaphore.Wait(); | ||
| try | ||
| { | ||
| // Load, set, and save in one atomic operation to ensure thread safety | ||
| EnsureUserSecretsDirectory(secretsFilePath); | ||
|
|
||
| var secrets = Load(secretsFilePath); | ||
| secrets[name] = value; | ||
|
||
|
|
||
| var contents = new JsonObject(); | ||
| foreach (var secret in secrets) | ||
| { | ||
| contents[secret.Key] = secret.Value; | ||
| } | ||
|
|
||
| // Create a temp file with the correct Unix file mode before moving it to the expected _filePath. | ||
| if (!OperatingSystem.IsWindows()) | ||
| { | ||
| var tempFilename = Path.GetTempFileName(); | ||
| File.Move(tempFilename, secretsFilePath, overwrite: true); | ||
| } | ||
|
|
||
| var json = contents.ToJsonString(new() | ||
| { | ||
| WriteIndented = true | ||
| }); | ||
|
|
||
| File.WriteAllText(secretsFilePath, json, Encoding.UTF8); | ||
| return true; | ||
| } | ||
| finally | ||
| { | ||
| semaphore.Release(); | ||
| } | ||
| } | ||
| catch (Exception) { } // Ignore user secret store errors | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| private static void EnsureUserSecretsDirectory(string secretsFilePath) | ||
| { | ||
| var directoryName = Path.GetDirectoryName(secretsFilePath); | ||
| if (!string.IsNullOrEmpty(directoryName) && !Directory.Exists(directoryName)) | ||
| { | ||
| Directory.CreateDirectory(directoryName); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| namespace Microsoft.Extensions.SecretManager.Tools.Internal; | ||
|
|
||
| /// <summary> | ||
| /// Provides synchronized access to user secrets files within the same process. | ||
| /// Both SecretsStore and UserSecretsDeploymentStateManager use this to ensure writes are serialized. | ||
| /// </summary> | ||
| internal static class UserSecretsFileLock | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is in a shared source file. I assume multiple projects will reference it. Each project's use of the shared file will have a separate static dictionary, and
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea in theory but its only shared by tests and the apphost at the moment. |
||
| { | ||
| // Static semaphore dictionary to synchronize access per file path | ||
| private static readonly Dictionary<string, SemaphoreSlim> s_semaphores = new(); | ||
|
||
| private static readonly object s_semaphoresLock = new(); | ||
|
|
||
| /// <summary> | ||
| /// Gets a semaphore for the specified user secrets file path. | ||
| /// </summary> | ||
| /// <param name="filePath">The full path to the user secrets file.</param> | ||
| /// <returns>A SemaphoreSlim that can be used for synchronization.</returns> | ||
| public static SemaphoreSlim GetSemaphore(string filePath) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(filePath); | ||
|
|
||
| // Normalize the path to ensure consistent locking across different path representations | ||
| var normalizedPath = Path.GetFullPath(filePath); | ||
|
|
||
| lock (s_semaphoresLock) | ||
| { | ||
| if (!s_semaphores.TryGetValue(normalizedPath, out var semaphore)) | ||
| { | ||
| semaphore = new SemaphoreSlim(1, 1); | ||
| s_semaphores[normalizedPath] = semaphore; | ||
| } | ||
| return semaphore; | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.