From 62a9a38c73ef88c4ba52bdf85e7dc4ff68f8adf1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kov=C3=A1cs=20B=C3=A1lint=20Hunor?= Date: Mon, 2 Nov 2026 16:53:21 +0200 Subject: [PATCH] Fix: Avoid blocking the UI thread on keyring unlock ISecretStore is now asynchronous end to end so a read or write that triggers the Secret Service unlock prompt no longer holds the calling thread. Git credential helpers and the FTP credential lookup await the store. Co-Authored-By: Claude Sonnet 5.5 --- src/Files.App.Storage/Ftp/FtpClientFactory.cs | 6 +- src/Files.App.Storage/Ftp/FtpHelpers.cs | 4 +- src/Files.App.Storage/Ftp/FtpManager.cs | 63 +++++---- src/Files.App.Storage/Ftp/FtpStorable.cs | 4 +- src/Files.App.Storage/Ftp/FtpStorageFile.cs | 2 +- src/Files.App.Storage/Ftp/FtpStorageFolder.cs | 10 +- .../Ftp/FtpStorageService.cs | 2 +- .../Data/Contracts/IVersionControlService.cs | 4 +- src/Files.App/Helpers/CredentialsHelpers.cs | 12 +- .../Helpers/UI/UIFilesystemHelpers.cs | 2 +- src/Files.App/Services/Git/LibGit2Service.cs | 4 +- src/Files.App/Utils/Git/GitHelpers.cs | 16 +-- .../Storage/StorageItems/FtpStorageFile.cs | 22 ++-- .../Storage/StorageItems/FtpStorageFolder.cs | 20 +-- .../ViewModels/Settings/DevToolsViewModel.cs | 14 +- .../Secrets/ISecretStore.cs | 9 +- .../Secrets/SecretServiceStore.cs | 66 ++++++---- .../Storables/FtpStorableRouteTests.cs | 45 ++++--- .../SecretServiceStoreTests.cs | 43 +++--- .../SecretServiceUnlockTests.cs | 124 +++++++++++++++--- 20 files changed, 305 insertions(+), 167 deletions(-) diff --git a/src/Files.App.Storage/Ftp/FtpClientFactory.cs b/src/Files.App.Storage/Ftp/FtpClientFactory.cs index da0337749135..d3acc8f7c327 100644 --- a/src/Files.App.Storage/Ftp/FtpClientFactory.cs +++ b/src/Files.App.Storage/Ftp/FtpClientFactory.cs @@ -3,6 +3,7 @@ using FluentFTP; using System.Net; +using System.Threading.Tasks; namespace Files.App.Storage { @@ -13,14 +14,15 @@ public static class FtpClientFactory { /// The parsed location. /// A credential the user entered for this item; otherwise the one cached for the exact scheme+host+port. - public static AsyncFtpClient Create(FtpUrl url, NetworkCredential? explicitCredential = null) + public static async Task CreateAsync(FtpUrl url, NetworkCredential? explicitCredential = null) { // Credentials embedded in the URL are kept (session only) but never override a saved one for this scope. FtpManager.RememberUrlCredential(url); // Only the exact scheme+host+port key is consulted; there is no host-only fallback. + // The saved credential may need the keyring unlocked, so the lookup is awaited rather than blocked on. var credentials = explicitCredential - ?? (FtpManager.Credentials.TryGetValue(url.GetCredentialKey(), out var known) ? known : FtpManager.Anonymous); + ?? (await FtpManager.Credentials.TryGetSavedAsync(url.GetCredentialKey()).ConfigureAwait(false) is { } known ? known : FtpManager.Anonymous); var isAnonymous = ReferenceEquals(credentials, FtpManager.Anonymous); var client = new AsyncFtpClient(url.Host, credentials, url.Port); diff --git a/src/Files.App.Storage/Ftp/FtpHelpers.cs b/src/Files.App.Storage/Ftp/FtpHelpers.cs index ebbcd06f444f..d4924dd680af 100644 --- a/src/Files.App.Storage/Ftp/FtpHelpers.cs +++ b/src/Files.App.Storage/Ftp/FtpHelpers.cs @@ -29,7 +29,7 @@ public static bool IsSameFtpPath(string firstPath, string secondPath) string.Equals(GetFtpPath(firstPath), GetFtpPath(secondPath), StringComparison.Ordinal); } - public static AsyncFtpClient GetFtpClient(string ftpPath) - => FtpClientFactory.Create(FtpUrl.Parse(ftpPath)); + public static Task GetFtpClientAsync(string ftpPath) + => FtpClientFactory.CreateAsync(FtpUrl.Parse(ftpPath)); } } diff --git a/src/Files.App.Storage/Ftp/FtpManager.cs b/src/Files.App.Storage/Ftp/FtpManager.cs index f3ea724bcd29..e080bb4e719e 100644 --- a/src/Files.App.Storage/Ftp/FtpManager.cs +++ b/src/Files.App.Storage/Ftp/FtpManager.cs @@ -7,13 +7,14 @@ using System.Collections.Generic; using System.Linq; using System.Net; +using System.Threading.Tasks; namespace Files.App.Storage { public static class FtpManager { /// - /// Credentials by host. Setting an entry also saves the password in the secret store, when one is configured. + /// Credentials by host. also saves the password in the secret store, when one is configured. /// public static readonly FtpCredentialCache Credentials = new(); @@ -72,10 +73,7 @@ public sealed class FtpCredentialCache : IReadOnlyDictionary TryGetValue(host, out var credential) ? credential : throw new KeyNotFoundException(); - set => Set(host, value, persist: true); - } + => TryGetValue(host, out var credential) ? credential : throw new KeyNotFoundException(); /// /// Keeps credentials taken from a URL for this session only. They are never persisted and never override a saved @@ -90,7 +88,10 @@ public void SetFromUrl(string credentialKey, NetworkCredential credential) public NetworkCredential Get(string host, NetworkCredential defaultValue) => TryGetValue(host, out var credential) ? credential : defaultValue; - public void Remove(string host) + /// + /// Removes the credential for from the session and the secret store. + /// + public async Task RemoveAsync(string host) { lock (_lock) { @@ -103,9 +104,9 @@ public void Remove(string host) try { var resource = GetResource(host); - if (store.Get(resource, LastUserAccount) is { } user) - store.Delete(resource, user); - store.Delete(resource, LastUserAccount); + if (await store.GetAsync(resource, LastUserAccount).ConfigureAwait(false) is { } user) + await store.DeleteAsync(resource, user).ConfigureAwait(false); + await store.DeleteAsync(resource, LastUserAccount).ConfigureAwait(false); } catch (Exception) { @@ -114,14 +115,33 @@ public void Remove(string host) } } + /// + /// Looks up the credential held in memory for this session; the secret store is not consulted, see . + /// public bool TryGetValue(string host, out NetworkCredential value) { lock (_lock) { - if (_session.TryGetValue(host, out value!)) + if (_session.TryGetValue(host, out value!) || _fromUrl.TryGetValue(host, out value!)) return true; } + value = null!; + return false; + } + + /// + /// Finds the credential for : this session's, then the saved one, then one taken from a URL. + /// Reading the secret store may wait for the user to unlock the keyring, so this never blocks. + /// + public async Task TryGetSavedAsync(string host) + { + lock (_lock) + { + if (_session.TryGetValue(host, out var session)) + return session; + } + // Legacy bare-host keys have no scheme/port scope and are never persisted or read from the store var scoped = host.Contains("://", StringComparison.Ordinal); @@ -130,11 +150,11 @@ public bool TryGetValue(string host, out NetworkCredential value) try { var resource = GetResource(host); - if (store.Get(resource, LastUserAccount) is { Length: > 0 } user && store.Get(resource, user) is { } password) + if (await store.GetAsync(resource, LastUserAccount).ConfigureAwait(false) is { Length: > 0 } user + && await store.GetAsync(resource, user).ConfigureAwait(false) is { } password) { // Not cached: the password stays in memory only as long as the caller needs it - value = new NetworkCredential(user, password); - return true; + return new NetworkCredential(user, password); } } catch (Exception) @@ -145,29 +165,28 @@ public bool TryGetValue(string host, out NetworkCredential value) lock (_lock) { - if (_fromUrl.TryGetValue(host, out value!)) - return true; + return _fromUrl.TryGetValue(host, out var fromUrl) ? fromUrl : null; } - - value = null!; - return false; } - private void Set(string host, NetworkCredential credential, bool persist) + /// + /// Keeps for this session and saves it in the secret store when one is configured. + /// + public async Task SetAsync(string host, NetworkCredential credential) { lock (_lock) { _session[host] = credential; } - if (!persist || !host.Contains("://", StringComparison.Ordinal) || FtpManager.SecretStore is not { } store || string.IsNullOrEmpty(credential.UserName)) + if (!host.Contains("://", StringComparison.Ordinal) || FtpManager.SecretStore is not { } store || string.IsNullOrEmpty(credential.UserName)) return; try { var resource = GetResource(host); - store.Save(resource, credential.UserName, credential.Password); - store.Save(resource, LastUserAccount, credential.UserName); + await store.SaveAsync(resource, credential.UserName, credential.Password).ConfigureAwait(false); + await store.SaveAsync(resource, LastUserAccount, credential.UserName).ConfigureAwait(false); } catch (Exception) { diff --git a/src/Files.App.Storage/Ftp/FtpStorable.cs b/src/Files.App.Storage/Ftp/FtpStorable.cs index 92764e0c8d5b..7c98c7bdcf67 100644 --- a/src/Files.App.Storage/Ftp/FtpStorable.cs +++ b/src/Files.App.Storage/Ftp/FtpStorable.cs @@ -31,9 +31,9 @@ protected internal FtpStorable(string path, string name, IFolder? parent) return Task.FromResult(Parent); } - protected AsyncFtpClient GetFtpClient() + protected Task GetFtpClientAsync() { - return FtpHelpers.GetFtpClient(Id); + return FtpHelpers.GetFtpClientAsync(Id); } } } diff --git a/src/Files.App.Storage/Ftp/FtpStorageFile.cs b/src/Files.App.Storage/Ftp/FtpStorageFile.cs index c098400498a0..5ad8c70245b8 100644 --- a/src/Files.App.Storage/Ftp/FtpStorageFile.cs +++ b/src/Files.App.Storage/Ftp/FtpStorageFile.cs @@ -15,7 +15,7 @@ public FtpStorageFile(string path, string name, IFolder? parent) /// public async Task OpenStreamAsync(FileAccess access, CancellationToken cancellationToken = default) { - var ftpClient = GetFtpClient(); + var ftpClient = await GetFtpClientAsync(); try { await ftpClient.EnsureConnectedAsync(cancellationToken); diff --git a/src/Files.App.Storage/Ftp/FtpStorageFolder.cs b/src/Files.App.Storage/Ftp/FtpStorageFolder.cs index 8b38709e0f88..8118f54462df 100644 --- a/src/Files.App.Storage/Ftp/FtpStorageFolder.cs +++ b/src/Files.App.Storage/Ftp/FtpStorageFolder.cs @@ -17,7 +17,7 @@ public FtpStorageFolder(string path, string name, IFolder? parent) /// public async Task GetFirstByNameAsync(string folderName, CancellationToken cancellationToken = default) { - using var ftpClient = GetFtpClient(); + using var ftpClient = await GetFtpClientAsync(); await ftpClient.EnsureConnectedAsync(cancellationToken); var itemId = $"{Id.TrimEnd('/')}/{folderName}"; @@ -37,7 +37,7 @@ public async Task GetFirstByNameAsync(string folderName, Cancell /// public async IAsyncEnumerable GetItemsAsync(StorableType kind = StorableType.All, [EnumeratorCancellation] CancellationToken cancellationToken = default) { - using var ftpClient = GetFtpClient(); + using var ftpClient = await GetFtpClientAsync(); await ftpClient.EnsureConnectedAsync(cancellationToken); foreach (var item in await ftpClient.GetListing(FtpHelpers.GetFtpPath(Id), cancellationToken)) @@ -59,7 +59,7 @@ public Task GetFolderWatcherAsync(CancellationToken cancellation /// public async Task DeleteAsync(IStorableChild item, CancellationToken cancellationToken = default) { - using var ftpClient = GetFtpClient(); + using var ftpClient = await GetFtpClientAsync(); await ftpClient.EnsureConnectedAsync(cancellationToken); if (item is IFile locatableFile) @@ -108,7 +108,7 @@ public async Task MoveFromAsync(IStorableChild itemToMove, IModi /// public async Task CreateFileAsync(string desiredName, bool overwrite = default, CancellationToken cancellationToken = default) { - using var ftpClient = GetFtpClient(); + using var ftpClient = await GetFtpClientAsync(); await ftpClient.EnsureConnectedAsync(cancellationToken); var newId = $"{Id.TrimEnd('/')}/{desiredName}"; @@ -139,7 +139,7 @@ public async Task CreateFileAsync(string desiredName, bool overwrite /// public async Task CreateFolderAsync(string desiredName, bool overwrite = default, CancellationToken cancellationToken = default) { - using var ftpClient = GetFtpClient(); + using var ftpClient = await GetFtpClientAsync(); await ftpClient.EnsureConnectedAsync(cancellationToken); var newId = $"{Id.TrimEnd('/')}/{desiredName}"; diff --git a/src/Files.App.Storage/Ftp/FtpStorageService.cs b/src/Files.App.Storage/Ftp/FtpStorageService.cs index 1ec63a8b1a74..6222f0d07cfb 100644 --- a/src/Files.App.Storage/Ftp/FtpStorageService.cs +++ b/src/Files.App.Storage/Ftp/FtpStorageService.cs @@ -46,7 +46,7 @@ public async Task GetFileAsync(string id, CancellationToken cancellationT // The same parsed URL provides the credential scope, connection target and path. private static async Task GetObjectInfoAsync(FtpUrl url, CancellationToken cancellationToken) { - using var ftpClient = FtpClientFactory.Create(url); + using var ftpClient = await FtpClientFactory.CreateAsync(url); try { await ftpClient.EnsureConnectedAsync(cancellationToken); diff --git a/src/Files.App/Data/Contracts/IVersionControlService.cs b/src/Files.App/Data/Contracts/IVersionControlService.cs index a7c5b3388053..e7be5afceda3 100644 --- a/src/Files.App/Data/Contracts/IVersionControlService.cs +++ b/src/Files.App/Data/Contracts/IVersionControlService.cs @@ -166,13 +166,13 @@ public interface IVersionControlService /// /// Removes any stored credentials associated with the version control provider. /// - void RemoveSavedCredentials(); + Task RemoveSavedCredentialsAsync(); /// /// Gets any stored credentials associated with the version control provider. /// /// The stored credential (typically a token), or an empty string if none exists. - string GetSavedCredentials(); + Task GetSavedCredentialsAsync(); /// /// Initialises a new repository at the specified location. diff --git a/src/Files.App/Helpers/CredentialsHelpers.cs b/src/Files.App/Helpers/CredentialsHelpers.cs index ba48ea348d84..0d5204126587 100644 --- a/src/Files.App/Helpers/CredentialsHelpers.cs +++ b/src/Files.App/Helpers/CredentialsHelpers.cs @@ -8,11 +8,11 @@ internal sealed class CredentialsHelpers // Linux: the Secret Service (libsecret backend) through ISecretStore; PasswordVault does not exist there private static ISecretStore? LinuxStore => OperatingSystem.IsLinux() ? Ioc.Default.GetService() : null; - public static void SavePassword(string resourceName, string username, string password) + public static async Task SavePasswordAsync(string resourceName, string username, string password) { if (LinuxStore is { } store) { - store.Save(resourceName, username, password); + await store.SaveAsync(resourceName, username, password); return; } @@ -23,11 +23,11 @@ public static void SavePassword(string resourceName, string username, string pas } // Remove saved credentials from the vault - public static void DeleteSavedPassword(string resourceName, string username) + public static async Task DeleteSavedPasswordAsync(string resourceName, string username) { if (LinuxStore is { } store) { - store.Delete(resourceName, username); + await store.DeleteAsync(resourceName, username); return; } @@ -37,10 +37,10 @@ public static void DeleteSavedPassword(string resourceName, string username) vault.Remove(credential); } - public static string GetPassword(string resourceName, string username) + public static async Task GetPasswordAsync(string resourceName, string username) { if (LinuxStore is { } store) - return store.Get(resourceName, username) ?? string.Empty; + return await store.GetAsync(resourceName, username) ?? string.Empty; try { diff --git a/src/Files.App/Helpers/UI/UIFilesystemHelpers.cs b/src/Files.App/Helpers/UI/UIFilesystemHelpers.cs index d80dba1c1707..7bfd19b36ebb 100644 --- a/src/Files.App/Helpers/UI/UIFilesystemHelpers.cs +++ b/src/Files.App/Helpers/UI/UIFilesystemHelpers.cs @@ -364,7 +364,7 @@ public async static Task RequestPassword(IPasswordProtectedIt { // Scoped by scheme+host+port so the password is never offered to another service on the same host var host = FtpUrl.Parse(path).GetCredentialKey(); - FtpManager.Credentials[host] = new NetworkCredential(credentials.UserName, credentials.SecurePassword); + await FtpManager.Credentials.SetAsync(host, new NetworkCredential(credentials.UserName, credentials.SecurePassword)); } return credentials; diff --git a/src/Files.App/Services/Git/LibGit2Service.cs b/src/Files.App/Services/Git/LibGit2Service.cs index 5a5c7a5937ab..bda6faf5f2d9 100644 --- a/src/Files.App/Services/Git/LibGit2Service.cs +++ b/src/Files.App/Services/Git/LibGit2Service.cs @@ -409,11 +409,13 @@ public async Task FetchOriginAsync(string? repositoryPath, bool reportProgress, await MainWindow.Instance.DispatcherQueue.EnqueueOrInvokeAsync( () => IsExecutingGitAction = Volatile.Read(ref _activeFetchCount) > 0); + // Awaited here: reading the token may wait for the user to unlock the keyring + var token = await CredentialsHelpers.GetPasswordAsync(GIT_RESOURCE_NAME, GIT_RESOURCE_USERNAME); + await Task.Run(() => { using var repository = new Repository(repositoryPath); var signature = repository.Config.BuildSignature(DateTimeOffset.Now); - var token = CredentialsHelpers.GetPassword(GIT_RESOURCE_NAME, GIT_RESOURCE_USERNAME); var remotes = repository.Network.Remotes.ToArray(); var hasFetchFailure = false; diff --git a/src/Files.App/Utils/Git/GitHelpers.cs b/src/Files.App/Utils/Git/GitHelpers.cs index 71984e7d50fa..e32a4a68a8ab 100644 --- a/src/Files.App/Utils/Git/GitHelpers.cs +++ b/src/Files.App/Utils/Git/GitHelpers.cs @@ -208,7 +208,7 @@ public static async Task SyncOriginAsync(string? repositoryPath, string? branchN }, }; - var token = CredentialsHelpers.GetPassword(GIT_RESOURCE_NAME, GIT_RESOURCE_USERNAME); + var token = await CredentialsHelpers.GetPasswordAsync(GIT_RESOURCE_NAME, GIT_RESOURCE_USERNAME); if (!string.IsNullOrWhiteSpace(token)) { fetchOptions.CredentialsProvider = (url, user, cred) @@ -276,11 +276,11 @@ private static async Task PushToOriginCoreAsync( if (signature is null) return GitOperationResult.GenericError; - var token = CredentialsHelpers.GetPassword(GIT_RESOURCE_NAME, GIT_RESOURCE_USERNAME); + var token = await CredentialsHelpers.GetPasswordAsync(GIT_RESOURCE_NAME, GIT_RESOURCE_USERNAME); if (string.IsNullOrWhiteSpace(token)) { await RequireGitAuthenticationAsync(); - token = CredentialsHelpers.GetPassword(GIT_RESOURCE_NAME, GIT_RESOURCE_USERNAME); + token = await CredentialsHelpers.GetPasswordAsync(GIT_RESOURCE_NAME, GIT_RESOURCE_USERNAME); } var progressRange = endPercentage - startPercentage; @@ -483,7 +483,7 @@ public static async Task RequireGitAuthenticationAsync() pending = false; - CredentialsHelpers.SavePassword( + await CredentialsHelpers.SavePasswordAsync( GIT_RESOURCE_NAME, GIT_RESOURCE_USERNAME, token); @@ -583,15 +583,15 @@ public static GitItemModel GetGitInformationForItem(Repository repository, strin } // Remove saved credentails - public static void RemoveSavedCredentials() + public static Task RemoveSavedCredentialsAsync() { - CredentialsHelpers.DeleteSavedPassword(GIT_RESOURCE_NAME, GIT_RESOURCE_USERNAME); + return CredentialsHelpers.DeleteSavedPasswordAsync(GIT_RESOURCE_NAME, GIT_RESOURCE_USERNAME); } // Get saved credentails - public static string GetSavedCredentials() + public static Task GetSavedCredentialsAsync() { - return CredentialsHelpers.GetPassword(GIT_RESOURCE_NAME, GIT_RESOURCE_USERNAME); + return CredentialsHelpers.GetPasswordAsync(GIT_RESOURCE_NAME, GIT_RESOURCE_USERNAME); } public static async Task InitializeRepositoryAsync(string? path) diff --git a/src/Files.App/Utils/Storage/StorageItems/FtpStorageFile.cs b/src/Files.App/Utils/Storage/StorageItems/FtpStorageFile.cs index 33f97d11e326..26472a305b3c 100644 --- a/src/Files.App/Utils/Storage/StorageItems/FtpStorageFile.cs +++ b/src/Files.App/Utils/Storage/StorageItems/FtpStorageFile.cs @@ -82,7 +82,7 @@ public override IAsyncOperation GetBasicPropertiesAsync() { return AsyncInfo.Run((cancellationToken) => SafetyExtensions.Wrap(async () => { - using var ftpClient = GetFtpClient(); + using var ftpClient = await GetFtpClientAsync(); if (!await ftpClient.EnsureConnectedAsync()) { return new BaseBasicProperties(); @@ -97,7 +97,7 @@ public override IAsyncOperation GetBasicPropertiesAsync() { return AsyncInfo.Run((cancellationToken) => SafetyExtensions.Wrap(async () => { - var ftpClient = GetFtpClient(); + var ftpClient = await GetFtpClientAsync(); if (!await ftpClient.EnsureConnectedAsync()) { return null; @@ -123,7 +123,7 @@ public override IAsyncOperation GetBasicPropertiesAsync() { return AsyncInfo.Run((cancellationToken) => SafetyExtensions.Wrap(async () => { - var ftpClient = GetFtpClient(); + var ftpClient = await GetFtpClientAsync(); if (!await ftpClient.EnsureConnectedAsync()) { return null; @@ -139,7 +139,7 @@ public override IAsyncOperation GetBasicPropertiesAsync() { return AsyncInfo.Run((cancellationToken) => SafetyExtensions.Wrap(async () => { - var ftpClient = GetFtpClient(); + var ftpClient = await GetFtpClientAsync(); if (!await ftpClient.EnsureConnectedAsync()) { return null; @@ -161,7 +161,7 @@ public override IAsyncOperation GetBasicPropertiesAsync() { return AsyncInfo.Run((cancellationToken) => SafetyExtensions.Wrap(async () => { - using var ftpClient = GetFtpClient(); + using var ftpClient = await GetFtpClientAsync(); if (!await ftpClient.EnsureConnectedAsync()) { return null; @@ -194,7 +194,7 @@ public override IAsyncAction MoveAsync(IStorageFolder destinationFolder, string { return AsyncInfo.Run((cancellationToken) => SafetyExtensions.WrapAsync(async () => { - using var ftpClient = GetFtpClient(); + using var ftpClient = await GetFtpClientAsync(); if (!await ftpClient.EnsureConnectedAsync()) throw new IOException($"Failed to connect to FTP server."); @@ -224,7 +224,7 @@ public override IAsyncAction RenameAsync(string desiredName, NameCollisionOption { return AsyncInfo.Run((cancellationToken) => SafetyExtensions.WrapAsync(async () => { - using var ftpClient = GetFtpClient(); + using var ftpClient = await GetFtpClientAsync(); if (!await ftpClient.EnsureConnectedAsync()) { return; @@ -244,7 +244,7 @@ public override IAsyncAction DeleteAsync() { return AsyncInfo.Run((cancellationToken) => SafetyExtensions.WrapAsync(async () => { - using var ftpClient = GetFtpClient(); + using var ftpClient = await GetFtpClientAsync(); if (await ftpClient.EnsureConnectedAsync()) { await ftpClient.DeleteFile(FtpPath, cancellationToken); @@ -260,10 +260,10 @@ public override IAsyncAction DeleteAsync() public override IAsyncOperation GetThumbnailAsync(ThumbnailMode mode, uint requestedSize, ThumbnailOptions options) => Task.FromResult(null).AsAsyncOperation(); - private AsyncFtpClient GetFtpClient() + private Task GetFtpClientAsync() { // Same URL parsing, credential scope and TLS policy as the OwlCore storables - return FtpClientFactory.Create( + return FtpClientFactory.CreateAsync( FtpUrl.Parse(Path), Credentials is null ? null : new NetworkCredential(Credentials.UserName, Credentials.SecurePassword)); } @@ -272,7 +272,7 @@ private async void FtpDataStreamingHandlerAsync(StreamedFileDataRequest request) { try { - using var ftpClient = GetFtpClient(); + using var ftpClient = await GetFtpClientAsync(); if (!await ftpClient.EnsureConnectedAsync()) { request.FailAndClose(StreamedFileFailureMode.CurrentlyUnavailable); diff --git a/src/Files.App/Utils/Storage/StorageItems/FtpStorageFolder.cs b/src/Files.App/Utils/Storage/StorageItems/FtpStorageFolder.cs index 324ecd74d589..44d5a6d78b5d 100644 --- a/src/Files.App/Utils/Storage/StorageItems/FtpStorageFolder.cs +++ b/src/Files.App/Utils/Storage/StorageItems/FtpStorageFolder.cs @@ -70,7 +70,7 @@ public override IAsyncOperation GetBasicPropertiesAsync() { return AsyncInfo.Run((cancellationToken) => SafetyExtensions.Wrap(async () => { - using var ftpClient = GetFtpClient(); + using var ftpClient = await GetFtpClientAsync(); if (!await ftpClient.EnsureConnectedAsync()) { return new BaseBasicProperties(); @@ -85,7 +85,7 @@ public override IAsyncOperation GetBasicPropertiesAsync() { return AsyncInfo.Run((cancellationToken) => SafetyExtensions.Wrap(async () => { - using var ftpClient = GetFtpClient(); + using var ftpClient = await GetFtpClientAsync(); if (!await ftpClient.EnsureConnectedAsync()) { return null; @@ -128,7 +128,7 @@ public override IAsyncOperation GetBasicPropertiesAsync() { return AsyncInfo.Run((cancellationToken) => SafetyExtensions.Wrap?>(async () => { - using var ftpClient = GetFtpClient(); + using var ftpClient = await GetFtpClientAsync(); if (!await ftpClient.EnsureConnectedAsync()) { return null; @@ -204,7 +204,7 @@ public override IAsyncOperation GetBasicPropertiesAsync() { return AsyncInfo.Run((cancellationToken) => SafetyExtensions.Wrap(async () => { - using var ftpClient = GetFtpClient(); + using var ftpClient = await GetFtpClientAsync(); if (!await ftpClient.EnsureConnectedAsync()) { return null; @@ -255,7 +255,7 @@ public override IAsyncOperation GetBasicPropertiesAsync() { return AsyncInfo.Run((cancellationToken) => SafetyExtensions.Wrap(async () => { - using var ftpClient = GetFtpClient(); + using var ftpClient = await GetFtpClientAsync(); if (!await ftpClient.EnsureConnectedAsync()) { throw new IOException($"Failed to connect to FTP server."); @@ -288,7 +288,7 @@ public override IAsyncOperation GetBasicPropertiesAsync() { return AsyncInfo.Run((cancellationToken) => SafetyExtensions.Wrap(async () => { - using var ftpClient = GetFtpClient(); + using var ftpClient = await GetFtpClientAsync(); if (!await ftpClient.EnsureConnectedAsync()) throw new IOException($"Failed to connect to FTP server."); @@ -318,7 +318,7 @@ public override IAsyncAction RenameAsync(string desiredName, NameCollisionOption { return AsyncInfo.Run((cancellationToken) => SafetyExtensions.WrapAsync(async () => { - using var ftpClient = GetFtpClient(); + using var ftpClient = await GetFtpClientAsync(); if (!await ftpClient.EnsureConnectedAsync()) { return; @@ -338,7 +338,7 @@ public override IAsyncAction DeleteAsync() { return AsyncInfo.Run((cancellationToken) => SafetyExtensions.WrapAsync(async () => { - using var ftpClient = GetFtpClient(); + using var ftpClient = await GetFtpClientAsync(); if (await ftpClient.EnsureConnectedAsync()) { await ftpClient.DeleteDirectory(FtpPath, cancellationToken); @@ -369,10 +369,10 @@ public override IAsyncAction DeleteAsync() public override IAsyncOperation GetThumbnailAsync(ThumbnailMode mode, uint requestedSize, ThumbnailOptions options) => Task.FromResult(null).AsAsyncOperation(); - private AsyncFtpClient GetFtpClient() + private Task GetFtpClientAsync() { // Same URL parsing, credential scope and TLS policy as the OwlCore storables - return FtpClientFactory.Create( + return FtpClientFactory.CreateAsync( FtpUrl.Parse(Path), Credentials is null ? null : new NetworkCredential(Credentials.UserName, Credentials.SecurePassword)); } diff --git a/src/Files.App/ViewModels/Settings/DevToolsViewModel.cs b/src/Files.App/ViewModels/Settings/DevToolsViewModel.cs index 027edf019189..773058d92e8e 100644 --- a/src/Files.App/ViewModels/Settings/DevToolsViewModel.cs +++ b/src/Files.App/ViewModels/Settings/DevToolsViewModel.cs @@ -99,9 +99,10 @@ public DevToolsViewModel() IsIDENameValid = true; // LINUX-TODO(devtools): Enable GitHub sign-in when Linux credential storage is available. - IsLogoutEnabled = IsWindows && GitHelpers.GetSavedCredentials() != string.Empty; + if (IsWindows) + _ = UpdateIsLogoutEnabledAsync(); - RemoveCredentialsCommand = new RelayCommand(DoRemoveCredentials); + RemoveCredentialsCommand = new RelayCommand(DoRemoveCredentialsAsync); ConnectToGitHubCommand = new RelayCommand(DoConnectToGitHubAsync); CancelIDEChangesCommand = new RelayCommand(DoCancelIDEChanges); SaveIDEChangesCommand = new RelayCommand(DoSaveIDEChanges); @@ -123,9 +124,14 @@ public string SelectedOpenInIDEOption } } - public void DoRemoveCredentials() + private async Task UpdateIsLogoutEnabledAsync() { - GitHelpers.RemoveSavedCredentials(); + IsLogoutEnabled = await GitHelpers.GetSavedCredentialsAsync() != string.Empty; + } + + public async void DoRemoveCredentialsAsync() + { + await GitHelpers.RemoveSavedCredentialsAsync(); IsLogoutEnabled = false; } diff --git a/src/Files.Platform.Abstractions/Secrets/ISecretStore.cs b/src/Files.Platform.Abstractions/Secrets/ISecretStore.cs index a7e8be857555..8e8a97b7dc35 100644 --- a/src/Files.Platform.Abstractions/Secrets/ISecretStore.cs +++ b/src/Files.Platform.Abstractions/Secrets/ISecretStore.cs @@ -1,10 +1,13 @@ // Copyright (c) Files Community // Licensed under the MIT License. +using System.Threading.Tasks; + namespace Files.Platform.Abstractions.Secrets { /// /// Stores small secrets (tokens, passwords) for the app, replacing the Windows credential vault. + /// Operations are asynchronous because the keyring may need the user to unlock it first; never block a thread on them. /// public interface ISecretStore { @@ -16,16 +19,16 @@ public interface ISecretStore /// /// Saves (or replaces) the secret of and . /// - void Save(string resource, string account, string secret); + Task SaveAsync(string resource, string account, string secret); /// /// Returns the secret, or when there is none. /// - string? Get(string resource, string account); + Task GetAsync(string resource, string account); /// /// Removes the secret. Returns whether one was removed. /// - bool Delete(string resource, string account); + Task DeleteAsync(string resource, string account); } } diff --git a/src/Files.Platform.Linux/Secrets/SecretServiceStore.cs b/src/Files.Platform.Linux/Secrets/SecretServiceStore.cs index dd647dcbe701..b4592b04b729 100644 --- a/src/Files.Platform.Linux/Secrets/SecretServiceStore.cs +++ b/src/Files.Platform.Linux/Secrets/SecretServiceStore.cs @@ -41,7 +41,7 @@ public sealed class SecretServiceStore : ISecretStore private long promptBlockedUntilTicks; private readonly ConcurrentDictionary<(string, string), string> memory = new(); private readonly ConcurrentDictionary<(string, string), byte> tombstones = new(); - private readonly ConcurrentDictionary<(string, string), object> locks = new(); + private readonly ConcurrentDictionary<(string, string), SemaphoreSlim> locks = new(); /// /// Creates the store. null means the user's session bus. @@ -66,23 +66,31 @@ internal SecretServiceStore(string? busAddress, TimeSpan unlockTimeout, TimeSpan public bool IsPersistent { get; private set; } /// - public void Save(string resource, string account, string secret) + public async Task SaveAsync(string resource, string account, string secret) { var key = (resource, account); - lock (LockFor(key)) + var keyLock = LockFor(key); + await keyLock.WaitAsync().ConfigureAwait(false); + try { memory[key] = secret; tombstones.TryRemove(key, out _); // Get prefers the in-memory value, so a failed update never lets the stale persisted secret win in this process. - IsPersistent = Run(c => SaveAsync(c, resource, account, secret)); + IsPersistent = (await RunAsync(async c => await SaveAsync(c, resource, account, secret).ConfigureAwait(false) ? "1" : null).ConfigureAwait(false)).Result is not null; + } + finally + { + keyLock.Release(); } } /// - public string? Get(string resource, string account) + public async Task GetAsync(string resource, string account) { var key = (resource, account); - lock (LockFor(key)) + var keyLock = LockFor(key); + await keyLock.WaitAsync().ConfigureAwait(false); + try { if (memory.TryGetValue(key, out var value)) return value; @@ -91,25 +99,30 @@ public void Save(string resource, string account, string secret) { // A delete that was not confirmed is retried; the persisted copy must not come back meanwhile. The per-key lock // guarantees no newer Save can be running, so the retry never removes a replacement. - if (RunDelete(resource, account, out _) is "deleted" or "absent") + if (await RunDeleteAsync(resource, account).ConfigureAwait(false) is ("deleted" or "absent", _)) tombstones.TryRemove(key, out _); return null; } - var stored = Run(c => GetAsync(c, resource, account), out var found); - return found ? stored : null; + return (await RunAsync(c => GetAsync(c, resource, account)).ConfigureAwait(false)).Result; + } + finally + { + keyLock.Release(); } } /// /// when nothing was deleted or when the persisted copy could not be removed (it is then hidden for this process). - public bool Delete(string resource, string account) + public async Task DeleteAsync(string resource, string account) { var key = (resource, account); - lock (LockFor(key)) + var keyLock = LockFor(key); + await keyLock.WaitAsync().ConfigureAwait(false); + try { var removedMemory = memory.TryRemove(key, out _); - var outcome = RunDelete(resource, account, out _); + var (outcome, _) = await RunDeleteAsync(resource, account).ConfigureAwait(false); if (outcome is "deleted" or "absent") { // Only a live service that confirmed the item is gone (or never existed) clears the marker @@ -120,33 +133,34 @@ public bool Delete(string resource, string account) tombstones[key] = 0; return outcome is "unavailable" && removedMemory; } + finally + { + keyLock.Release(); + } } - private object LockFor((string, string) key) => locks.GetOrAdd(key, static _ => new object()); - - private string RunDelete(string resource, string account, out bool reachable) - => Run(c => DeleteAsync(c, resource, account), out reachable) ?? (reachable ? "failed" : "unavailable"); + private SemaphoreSlim LockFor((string, string) key) => locks.GetOrAdd(key, static _ => new SemaphoreSlim(1, 1)); - private bool Run(Func> action) - => Run(async c => await action(c).ConfigureAwait(false) ? "1" : null, out _) is not null; + private async Task<(string Outcome, bool Reachable)> RunDeleteAsync(string resource, string account) + { + var (result, reachable) = await RunAsync(c => DeleteAsync(c, resource, account)).ConfigureAwait(false); + return (result ?? (reachable ? "failed" : "unavailable"), reachable); + } // The budget must outlast the worst sequence so the prompt Dismiss runs before the connection is disposed. Save: OpenSession + CreateItem, // then Unlock + AddMatch + Prompt, then OpenSession + CreateItem again (7 calls); Get/Delete are shorter, and a dismissed or timed-out prompt // ends with Dismiss instead of the retry. Waiting for the unlock gate and for the user's answer each take up to unlockTimeout. - private T? Run(Func> action, out bool reachable) where T : class + private async Task<(T? Result, bool Reachable)> RunAsync(Func> action) where T : class { - var wasReachable = false; + var reachable = false; try { - return RunAsync(action, () => wasReachable = true, CallTimeout * 8 + unlockTimeout * 2).GetAwaiter().GetResult(); + var result = await RunAsync(action, () => reachable = true, CallTimeout * 8 + unlockTimeout * 2).ConfigureAwait(false); + return (result, reachable); } catch (Exception ex) when (ex is TimeoutException or DBusExceptionBase or ObjectDisposedException or InvalidOperationException or System.IO.IOException) { - return null; - } - finally - { - reachable = wasReachable; + return (null, reachable); } } diff --git a/tests/Files.Platform.Tests/Storables/FtpStorableRouteTests.cs b/tests/Files.Platform.Tests/Storables/FtpStorableRouteTests.cs index 025c95c3fe48..581a622a709c 100644 --- a/tests/Files.Platform.Tests/Storables/FtpStorableRouteTests.cs +++ b/tests/Files.Platform.Tests/Storables/FtpStorableRouteTests.cs @@ -158,7 +158,7 @@ public async Task Resolver_DispatchesFtpBeforeLocal() } [TestMethod] - public void Credentials_PersistThroughSecretStore_ScopedBySchemeHostAndPort() + public async Task Credentials_PersistThroughSecretStore_ScopedBySchemeHostAndPort() { var store = new MemorySecretStore(); var previous = FtpManager.SecretStore; @@ -167,19 +167,20 @@ public void Credentials_PersistThroughSecretStore_ScopedBySchemeHostAndPort() { var key = FtpUrl.Parse("ftps://Example.org/x").GetCredentialKey(); var cache = new FtpCredentialCache(); - cache[key] = new NetworkCredential("bob", "pw"); + await cache.SetAsync(key, new NetworkCredential("bob", "pw")); Assert.AreEqual("ftps://example.org:990", key); - Assert.AreEqual("pw", store.Get("Files FTP ftps://example.org:990", "bob")); + Assert.AreEqual("pw", await store.GetAsync("Files FTP ftps://example.org:990", "bob")); var fresh = new FtpCredentialCache(); - Assert.IsTrue(fresh.TryGetValue(key, out var credential)); + var credential = await fresh.TryGetSavedAsync(key); + Assert.IsNotNull(credential); Assert.AreEqual("pw", credential.Password); // Other scheme or port never sees it - Assert.IsFalse(fresh.TryGetValue(FtpUrl.Parse("ftp://example.org").GetCredentialKey(), out _)); - Assert.IsFalse(fresh.TryGetValue(FtpUrl.Parse("ftps://example.org:2121").GetCredentialKey(), out _)); - Assert.IsFalse(fresh.TryGetValue("example.org", out _)); + Assert.IsNull(await fresh.TryGetSavedAsync(FtpUrl.Parse("ftp://example.org").GetCredentialKey())); + Assert.IsNull(await fresh.TryGetSavedAsync(FtpUrl.Parse("ftps://example.org:2121").GetCredentialKey())); + Assert.IsNull(await fresh.TryGetSavedAsync("example.org")); } finally { @@ -188,7 +189,7 @@ public void Credentials_PersistThroughSecretStore_ScopedBySchemeHostAndPort() } [TestMethod] - public void UrlCredentials_AreNeverPersisted_AndNeverOverrideSaved() + public async Task UrlCredentials_AreNeverPersisted_AndNeverOverrideSaved() { var store = new MemorySecretStore(); var previous = FtpManager.SecretStore; @@ -204,11 +205,11 @@ public void UrlCredentials_AreNeverPersisted_AndNeverOverrideSaved() Assert.IsFalse(new FtpCredentialCache().TryGetValue(key, out _)); Assert.IsFalse(cache.TryGetValue(FtpUrl.Parse("ftp://other").GetCredentialKey(), out _)); - cache[key] = new NetworkCredential("bob", "saved"); + await cache.SetAsync(key, new NetworkCredential("bob", "saved")); cache.SetFromUrl(key, new NetworkCredential("eve", "typed")); Assert.IsTrue(cache.TryGetValue(key, out var chosen)); Assert.AreEqual("bob", chosen.UserName); - Assert.IsNull(store.Get("Files FTP ftp://h:21", "eve")); + Assert.IsNull(await store.GetAsync("Files FTP ftp://h:21", "eve")); } finally { @@ -217,7 +218,7 @@ public void UrlCredentials_AreNeverPersisted_AndNeverOverrideSaved() } [TestMethod] - public void LegacyHostKeys_AreSessionOnly() + public async Task LegacyHostKeys_AreSessionOnly() { var store = new MemorySecretStore(); var previous = FtpManager.SecretStore; @@ -225,9 +226,9 @@ public void LegacyHostKeys_AreSessionOnly() try { var cache = new FtpCredentialCache(); - cache["host"] = new NetworkCredential("bob", "pw"); + await cache.SetAsync("host", new NetworkCredential("bob", "pw")); - Assert.IsNull(store.Get("Files FTP host", "bob")); + Assert.IsNull(await store.GetAsync("Files FTP host", "bob")); Assert.IsTrue(cache.TryGetValue("host", out _)); } finally @@ -306,14 +307,14 @@ public async Task Route_RemembersUrlCredentialBeforeStrippingIt() } [TestMethod] - public void Factory_UsesUrlCredential_ButSavedCredentialWins() + public async Task Factory_UsesUrlCredential_ButSavedCredentialWins() { var plain = FtpUrl.Parse("ftpes://alice:secret@factory1.example/p"); - using (var client = FtpClientFactory.Create(plain)) + using (var client = await FtpClientFactory.CreateAsync(plain)) Assert.AreEqual("alice", client.Credentials.UserName); - FtpManager.Credentials[FtpUrl.Parse("ftpes://factory2.example").GetCredentialKey()] = new NetworkCredential("bob", "saved"); - using (var client = FtpClientFactory.Create(FtpUrl.Parse("ftpes://eve:typed@factory2.example/p"))) + await FtpManager.Credentials.SetAsync(FtpUrl.Parse("ftpes://factory2.example").GetCredentialKey(), new NetworkCredential("bob", "saved")); + using (var client = await FtpClientFactory.CreateAsync(FtpUrl.Parse("ftpes://eve:typed@factory2.example/p"))) Assert.AreEqual("bob", client.Credentials.UserName); } @@ -412,9 +413,13 @@ private sealed class MemorySecretStore : ISecretStore private readonly System.Collections.Generic.Dictionary<(string, string), string> _items = new(); public bool IsPersistent => false; - public void Save(string resource, string account, string secret) => _items[(resource, account)] = secret; - public string? Get(string resource, string account) => _items.TryGetValue((resource, account), out var v) ? v : null; - public bool Delete(string resource, string account) => _items.Remove((resource, account)); + public Task SaveAsync(string resource, string account, string secret) + { + _items[(resource, account)] = secret; + return Task.CompletedTask; + } + public Task GetAsync(string resource, string account) => Task.FromResult(_items.TryGetValue((resource, account), out var v) ? v : null); + public Task DeleteAsync(string resource, string account) => Task.FromResult(_items.Remove((resource, account))); } } } diff --git a/tests/Files.Platform.Tests/SystemIntegration/SecretServiceStoreTests.cs b/tests/Files.Platform.Tests/SystemIntegration/SecretServiceStoreTests.cs index 6fd281789925..1aa10af043e0 100644 --- a/tests/Files.Platform.Tests/SystemIntegration/SecretServiceStoreTests.cs +++ b/tests/Files.Platform.Tests/SystemIntegration/SecretServiceStoreTests.cs @@ -8,6 +8,7 @@ using System.IO; using System.Runtime.Versioning; using System.Threading; +using System.Threading.Tasks; namespace Files.Platform.Tests.SystemIntegration { @@ -16,37 +17,37 @@ namespace Files.Platform.Tests.SystemIntegration public sealed class SecretServiceStoreTests { [TestMethod] - public void WithoutService_SecretsLiveInMemory() + public async Task WithoutService_SecretsLiveInMemory() { using var bus = PrivateBus.Start(); var store = new SecretServiceStore(bus.Address); - store.Save("resource", "account", "s3cret"); + await store.SaveAsync("resource", "account", "s3cret"); Assert.IsFalse(store.IsPersistent); - Assert.AreEqual("s3cret", store.Get("resource", "account")); - Assert.IsNull(store.Get("resource", "other")); - Assert.IsTrue(store.Delete("resource", "account")); - Assert.IsNull(store.Get("resource", "account")); + Assert.AreEqual("s3cret", await store.GetAsync("resource", "account")); + Assert.IsNull(await store.GetAsync("resource", "other")); + Assert.IsTrue(await store.DeleteAsync("resource", "account")); + Assert.IsNull(await store.GetAsync("resource", "account")); } [TestMethod] - public void UnconfirmedDelete_NeverResurrectsAndDoesNotEatReplacement() + public async Task UnconfirmedDelete_NeverResurrectsAndDoesNotEatReplacement() { using var bus = PrivateBus.Start(); var store = new SecretServiceStore(bus.Address); - store.Save("r", "a", "old"); - store.Delete("r", "a"); - Assert.IsNull(store.Get("r", "a")); + await store.SaveAsync("r", "a", "old"); + await store.DeleteAsync("r", "a"); + Assert.IsNull(await store.GetAsync("r", "a")); - store.Save("r", "a", "new"); - Assert.AreEqual("new", store.Get("r", "a")); - Assert.AreEqual("new", store.Get("r", "a")); // a stale tombstone retry must not remove the replacement + await store.SaveAsync("r", "a", "new"); + Assert.AreEqual("new", await store.GetAsync("r", "a")); + Assert.AreEqual("new", await store.GetAsync("r", "a")); // a stale tombstone retry must not remove the replacement } [TestMethod] - public void WithKeyring_RoundTripsThroughSecretService() + public async Task WithKeyring_RoundTripsThroughSecretService() { using var bus = PrivateBus.Start(); var home = Path.Combine(Path.GetTempPath(), "fkr-" + Guid.NewGuid().ToString("N")[..8]); @@ -90,10 +91,10 @@ public void WithKeyring_RoundTripsThroughSecretService() var deadline = DateTime.UtcNow.AddSeconds(8); while (DateTime.UtcNow < deadline) { - store.Save("Files:https://example.invalid", "token", "ghp_secret"); + await store.SaveAsync("Files:https://example.invalid", "token", "ghp_secret"); if (store.IsPersistent) break; - Thread.Sleep(300); + await Task.Delay(300); } if (!store.IsPersistent) @@ -101,13 +102,13 @@ public void WithKeyring_RoundTripsThroughSecretService() // A second store has no in-memory copy, so this proves the value went through the service var other = new SecretServiceStore(bus.Address); - Assert.AreEqual("ghp_secret", other.Get("Files:https://example.invalid", "token")); + Assert.AreEqual("ghp_secret", await other.GetAsync("Files:https://example.invalid", "token")); - store.Save("Files:https://example.invalid", "token", "replaced"); - Assert.AreEqual("replaced", new SecretServiceStore(bus.Address).Get("Files:https://example.invalid", "token")); + await store.SaveAsync("Files:https://example.invalid", "token", "replaced"); + Assert.AreEqual("replaced", await new SecretServiceStore(bus.Address).GetAsync("Files:https://example.invalid", "token")); - Assert.IsTrue(store.Delete("Files:https://example.invalid", "token")); - Assert.IsNull(new SecretServiceStore(bus.Address).Get("Files:https://example.invalid", "token")); + Assert.IsTrue(await store.DeleteAsync("Files:https://example.invalid", "token")); + Assert.IsNull(await new SecretServiceStore(bus.Address).GetAsync("Files:https://example.invalid", "token")); } finally { diff --git a/tests/Files.Platform.Tests/SystemIntegration/SecretServiceUnlockTests.cs b/tests/Files.Platform.Tests/SystemIntegration/SecretServiceUnlockTests.cs index 084b784bfd32..37c6d488dfa8 100644 --- a/tests/Files.Platform.Tests/SystemIntegration/SecretServiceUnlockTests.cs +++ b/tests/Files.Platform.Tests/SystemIntegration/SecretServiceUnlockTests.cs @@ -43,6 +43,9 @@ private sealed class FakeSecretService : IPathMethodHandler, IDisposable public int UnlockCalls { get; private set; } + // When set, the prompt answers only after the test completes it: true accepts, false dismisses + public TaskCompletionSource? PromptAnswer { get; set; } + public int PromptCalls { get; private set; } public int DismissCalls { get; private set; } @@ -168,6 +171,18 @@ private void CompletePrompt() var immediate = Mode == UnlockMode.PromptImmediate; var dismissed = Mode is not (UnlockMode.PromptAccept or UnlockMode.PromptImmediate); + if (PromptAnswer is { } answer) + { + _ = Task.Run(async () => + { + var accepted = await answer.Task; + if (accepted) + Locked = false; + SendCompleted(!accepted); + }); + return; + } + if (!dismissed) Locked = false; @@ -241,10 +256,81 @@ private void CreateItem(MethodContext context) public void Dispose() => connection.Dispose(); } - // The store blocks its caller, so it is driven off the test thread to mirror the app's use from a worker private static SecretServiceStore NewStore(PrivateBus bus, int unlockTimeoutMs = 5000) => new(bus.Address, TimeSpan.FromMilliseconds(unlockTimeoutMs), TimeSpan.FromMinutes(5)); + [TestMethod] + public async Task Locked_SlowUnlock_DoesNotBlockTheCaller() + { + using var bus = PrivateBus.Start(); + using var service = await FakeSecretService.StartAsync(bus.Address); + service.Seed("r", "a", "stored"); + service.Locked = true; + service.Mode = UnlockMode.PromptAccept; + service.PromptAnswer = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + var read = NewStore(bus).GetAsync("r", "a"); + + // The call returned while the prompt is still unanswered, so the calling thread was not held + await WaitUntilAsync(() => service.PromptCalls == 1); + Assert.IsFalse(read.IsCompleted); + + service.PromptAnswer.SetResult(true); + Assert.AreEqual("stored", await read); + } + + [TestMethod] + public async Task Locked_SlowSaveUnlock_DoesNotBlockTheCaller() + { + using var bus = PrivateBus.Start(); + using var service = await FakeSecretService.StartAsync(bus.Address); + service.Locked = true; + service.Mode = UnlockMode.PromptAccept; + service.PromptAnswer = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + var store = NewStore(bus); + var save = store.SaveAsync("r", "a", "s3cret"); + + await WaitUntilAsync(() => service.PromptCalls == 1); + Assert.IsFalse(save.IsCompleted); + + service.PromptAnswer.SetResult(true); + await save; + Assert.IsTrue(store.IsPersistent); + Assert.AreEqual("s3cret", await NewStore(bus).GetAsync("r", "a")); + } + + [TestMethod] + public async Task Locked_SlowUnlockThenDismissed_FallsBackToMemory() + { + using var bus = PrivateBus.Start(); + using var service = await FakeSecretService.StartAsync(bus.Address); + service.Locked = true; + service.Mode = UnlockMode.PromptAccept; + service.PromptAnswer = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + var store = NewStore(bus); + var save = store.SaveAsync("r", "a", "s3cret"); + + await WaitUntilAsync(() => service.PromptCalls == 1); + Assert.IsFalse(save.IsCompleted); + + service.PromptAnswer.SetResult(false); + await save; + Assert.IsFalse(store.IsPersistent); + Assert.AreEqual("s3cret", await store.GetAsync("r", "a")); + } + + private static async Task WaitUntilAsync(Func condition) + { + var deadline = DateTime.UtcNow.AddSeconds(5); + while (!condition()) + { + Assert.IsTrue(DateTime.UtcNow < deadline, "The fake service never received the prompt call."); + await Task.Delay(10); + } + } + [TestMethod] public async Task Unlocked_SavesWithoutAnyPrompt() { @@ -252,12 +338,12 @@ public async Task Unlocked_SavesWithoutAnyPrompt() using var service = await FakeSecretService.StartAsync(bus.Address); var store = NewStore(bus); - await Task.Run(() => store.Save("r", "a", "s3cret")); + await store.SaveAsync("r", "a", "s3cret"); Assert.IsTrue(store.IsPersistent); Assert.AreEqual(0, service.UnlockCalls); Assert.AreEqual(0, service.PromptCalls); - Assert.AreEqual("s3cret", await Task.Run(() => NewStore(bus).Get("r", "a"))); + Assert.AreEqual("s3cret", await NewStore(bus).GetAsync("r", "a")); } [TestMethod] @@ -269,12 +355,12 @@ public async Task Locked_UnlockWithoutPrompt_RetriesAndPersists() service.Mode = UnlockMode.Silent; var store = NewStore(bus); - await Task.Run(() => store.Save("r", "a", "s3cret")); + await store.SaveAsync("r", "a", "s3cret"); Assert.IsTrue(store.IsPersistent); Assert.AreEqual(1, service.UnlockCalls); Assert.AreEqual(0, service.PromptCalls); - Assert.AreEqual("s3cret", await Task.Run(() => NewStore(bus).Get("r", "a"))); + Assert.AreEqual("s3cret", await NewStore(bus).GetAsync("r", "a")); } [TestMethod] @@ -286,11 +372,11 @@ public async Task Locked_PromptCompleted_RetriesAndPersists() service.Mode = UnlockMode.PromptAccept; var store = NewStore(bus); - await Task.Run(() => store.Save("r", "a", "s3cret")); + await store.SaveAsync("r", "a", "s3cret"); Assert.IsTrue(store.IsPersistent); Assert.AreEqual(1, service.PromptCalls); - Assert.AreEqual("s3cret", await Task.Run(() => NewStore(bus).Get("r", "a"))); + Assert.AreEqual("s3cret", await NewStore(bus).GetAsync("r", "a")); } [TestMethod] @@ -302,13 +388,13 @@ public async Task Locked_PromptDismissed_FallsBackToMemoryAndDoesNotNag() service.Mode = UnlockMode.PromptDismiss; var store = NewStore(bus); - await Task.Run(() => store.Save("r", "a", "s3cret")); + await store.SaveAsync("r", "a", "s3cret"); Assert.IsFalse(store.IsPersistent); - Assert.AreEqual("s3cret", await Task.Run(() => store.Get("r", "a"))); + Assert.AreEqual("s3cret", await store.GetAsync("r", "a")); Assert.AreEqual(1, service.PromptCalls); - await Task.Run(() => store.Save("r", "b", "other")); + await store.SaveAsync("r", "b", "other"); Assert.IsFalse(store.IsPersistent); Assert.AreEqual(1, service.PromptCalls, "A refused prompt must not be repeated during the cooldown."); } @@ -322,10 +408,10 @@ public async Task Locked_UnlockFails_FallsBackToMemory() service.Mode = UnlockMode.Error; var store = NewStore(bus); - await Task.Run(() => store.Save("r", "a", "s3cret")); + await store.SaveAsync("r", "a", "s3cret"); Assert.IsFalse(store.IsPersistent); - Assert.AreEqual("s3cret", await Task.Run(() => store.Get("r", "a"))); + Assert.AreEqual("s3cret", await store.GetAsync("r", "a")); Assert.AreEqual(0, service.PromptCalls); } @@ -338,10 +424,10 @@ public async Task Locked_PromptNeverAnswered_TimesOutDismissesAndFallsBack() service.Mode = UnlockMode.PromptNever; var store = NewStore(bus, unlockTimeoutMs: 400); - await Task.Run(() => store.Save("r", "a", "s3cret")); + await store.SaveAsync("r", "a", "s3cret"); Assert.IsFalse(store.IsPersistent); - Assert.AreEqual("s3cret", await Task.Run(() => store.Get("r", "a"))); + Assert.AreEqual("s3cret", await store.GetAsync("r", "a")); Assert.AreEqual(1, service.PromptCalls); Assert.AreEqual(1, service.DismissCalls); } @@ -355,7 +441,7 @@ public async Task LockedItem_GetPromptsThenReadsSecret() service.Locked = true; service.Mode = UnlockMode.PromptAccept; - Assert.AreEqual("stored", await Task.Run(() => NewStore(bus).Get("r", "a"))); + Assert.AreEqual("stored", await NewStore(bus).GetAsync("r", "a")); Assert.AreEqual(1, service.PromptCalls); } @@ -368,11 +454,11 @@ public async Task Locked_PromptCompletesImmediately_StillUnlocks() service.Mode = UnlockMode.PromptImmediate; var store = NewStore(bus); - await Task.Run(() => store.Save("r", "a", "s3cret")); + await store.SaveAsync("r", "a", "s3cret"); Assert.IsTrue(store.IsPersistent); Assert.AreEqual(1, service.PromptCalls); - Assert.AreEqual("s3cret", await Task.Run(() => NewStore(bus).Get("r", "a"))); + Assert.AreEqual("s3cret", await NewStore(bus).GetAsync("r", "a")); } [TestMethod] @@ -384,9 +470,9 @@ public async Task LockedItem_DeletePromptsThenRemoves() service.Locked = true; service.Mode = UnlockMode.PromptAccept; - Assert.IsTrue(await Task.Run(() => NewStore(bus).Delete("r", "a"))); + Assert.IsTrue(await NewStore(bus).DeleteAsync("r", "a")); Assert.AreEqual(1, service.PromptCalls); - Assert.IsNull(await Task.Run(() => NewStore(bus).Get("r", "a"))); + Assert.IsNull(await NewStore(bus).GetAsync("r", "a")); } } }