From 1823efa0e539e276325131a51ee59dfd9c11964b Mon Sep 17 00:00:00 2001 From: Federico Maccaroni Date: Fri, 7 Apr 2023 18:24:54 +0100 Subject: [PATCH] [PM-1576] Fix Race condition AccountsManager registration (#2434) * PM-1576 Moved registration of AccountsManager to avoid race conditions with the app start. To do so, added ConditionedAwaiterManager so that it handles a task to be awaited or completed depending on the callers. * PM-1576 Fix format * PM-1576 Fix throw to preserve StackTrace --- src/Android/MainApplication.cs | 3 +- .../AccountManagement/AccountsManager.cs | 15 ++++- .../IConditionedAwaiterManager.cs | 17 +++++ .../Services/ConditionedAwaiterManager.cs | 42 +++++++++++++ src/Core/Services/EnvironmentService.cs | 62 ++++++++++++------- src/Core/Utilities/ServiceContainer.cs | 4 +- src/iOS.Core/Utilities/iOSCoreHelpers.cs | 26 ++++---- 7 files changed, 132 insertions(+), 37 deletions(-) create mode 100644 src/Core/Abstractions/IConditionedAwaiterManager.cs create mode 100644 src/Core/Services/ConditionedAwaiterManager.cs diff --git a/src/Android/MainApplication.cs b/src/Android/MainApplication.cs index f0f094bc8..72dc30f3c 100644 --- a/src/Android/MainApplication.cs +++ b/src/Android/MainApplication.cs @@ -81,7 +81,8 @@ namespace Bit.Droid ServiceContainer.Resolve("authService"), ServiceContainer.Resolve("logger"), ServiceContainer.Resolve("messagingService"), - ServiceContainer.Resolve()); + ServiceContainer.Resolve(), + ServiceContainer.Resolve()); ServiceContainer.Register("accountsManager", accountsManager); } #if !FDROID diff --git a/src/App/Utilities/AccountManagement/AccountsManager.cs b/src/App/Utilities/AccountManagement/AccountsManager.cs index b08714b19..919070359 100644 --- a/src/App/Utilities/AccountManagement/AccountsManager.cs +++ b/src/App/Utilities/AccountManagement/AccountsManager.cs @@ -22,6 +22,7 @@ namespace Bit.App.Utilities.AccountManagement private readonly ILogger _logger; private readonly IMessagingService _messagingService; private readonly IWatchDeviceService _watchDeviceService; + private readonly IConditionedAwaiterManager _conditionedAwaiterManager; Func _getOptionsFunc; private IAccountsManagerHost _accountsManagerHost; @@ -34,7 +35,8 @@ namespace Bit.App.Utilities.AccountManagement IAuthService authService, ILogger logger, IMessagingService messagingService, - IWatchDeviceService watchDeviceService) + IWatchDeviceService watchDeviceService, + IConditionedAwaiterManager conditionedAwaiterManager) { _broadcasterService = broadcasterService; _vaultTimeoutService = vaultTimeoutService; @@ -45,6 +47,7 @@ namespace Bit.App.Utilities.AccountManagement _logger = logger; _messagingService = messagingService; _watchDeviceService = watchDeviceService; + _conditionedAwaiterManager = conditionedAwaiterManager; } private AppOptions Options => _getOptionsFunc?.Invoke() ?? new AppOptions { IosExtension = true }; @@ -59,6 +62,8 @@ namespace Bit.App.Utilities.AccountManagement public async Task StartDefaultNavigationFlowAsync(Action appOptionsAction) { + await _conditionedAwaiterManager.GetAwaiterForPrecondition(AwaiterPrecondition.EnvironmentUrlsInited); + appOptionsAction(Options); await NavigateOnAccountChangeAsync(); @@ -66,6 +71,8 @@ namespace Bit.App.Utilities.AccountManagement public async Task NavigateOnAccountChangeAsync(bool? isAuthed = null) { + await _conditionedAwaiterManager.GetAwaiterForPrecondition(AwaiterPrecondition.EnvironmentUrlsInited); + // TODO: this could be improved by doing chain of responsability pattern // but for now it may be an overkill, if logic gets more complex consider refactoring it @@ -132,6 +139,8 @@ namespace Bit.App.Utilities.AccountManagement { try { + await _conditionedAwaiterManager.GetAwaiterForPrecondition(AwaiterPrecondition.EnvironmentUrlsInited); + switch (message.Command) { case AccountsManagerMessageCommands.LOCKED: @@ -206,6 +215,8 @@ namespace Bit.App.Utilities.AccountManagement public async Task LogOutAsync(string userId, bool userInitiated, bool expired) { + await _conditionedAwaiterManager.GetAwaiterForPrecondition(AwaiterPrecondition.EnvironmentUrlsInited); + await AppHelpers.LogOutAsync(userId, userInitiated); await NavigateOnAccountChangeAsync(); _authService.LogOut(() => @@ -244,6 +255,8 @@ namespace Bit.App.Utilities.AccountManagement AppResources.AccountAlreadyAdded, AppResources.Yes, AppResources.Cancel); if (switchToAccount) { + await _conditionedAwaiterManager.GetAwaiterForPrecondition(AwaiterPrecondition.EnvironmentUrlsInited); + await _stateService.SetActiveUserAsync(userId); _messagingService.Send("switchedAccount"); } diff --git a/src/Core/Abstractions/IConditionedAwaiterManager.cs b/src/Core/Abstractions/IConditionedAwaiterManager.cs new file mode 100644 index 000000000..ea7dd951b --- /dev/null +++ b/src/Core/Abstractions/IConditionedAwaiterManager.cs @@ -0,0 +1,17 @@ +using System; +using System.Threading.Tasks; + +namespace Bit.Core.Abstractions +{ + public enum AwaiterPrecondition + { + EnvironmentUrlsInited + } + + public interface IConditionedAwaiterManager + { + Task GetAwaiterForPrecondition(AwaiterPrecondition awaiterPrecondition); + void SetAsCompleted(AwaiterPrecondition awaiterPrecondition); + void SetException(AwaiterPrecondition awaiterPrecondition, Exception ex); + } +} diff --git a/src/Core/Services/ConditionedAwaiterManager.cs b/src/Core/Services/ConditionedAwaiterManager.cs new file mode 100644 index 000000000..f318996f5 --- /dev/null +++ b/src/Core/Services/ConditionedAwaiterManager.cs @@ -0,0 +1,42 @@ +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Threading.Tasks; +using Bit.Core.Abstractions; + +namespace Bit.Core.Services +{ + public class ConditionedAwaiterManager : IConditionedAwaiterManager + { + private readonly ConcurrentDictionary> _preconditionsTasks = new ConcurrentDictionary> + { + [AwaiterPrecondition.EnvironmentUrlsInited] = new TaskCompletionSource() + }; + + public Task GetAwaiterForPrecondition(AwaiterPrecondition awaiterPrecondition) + { + if (_preconditionsTasks.TryGetValue(awaiterPrecondition, out var tcs)) + { + return tcs.Task; + } + + return Task.CompletedTask; + } + + public void SetAsCompleted(AwaiterPrecondition awaiterPrecondition) + { + if (_preconditionsTasks.TryGetValue(awaiterPrecondition, out var tcs)) + { + tcs.TrySetResult(true); + } + } + + public void SetException(AwaiterPrecondition awaiterPrecondition, Exception ex) + { + if (_preconditionsTasks.TryGetValue(awaiterPrecondition, out var tcs)) + { + tcs.TrySetException(ex); + } + } + } +} diff --git a/src/Core/Services/EnvironmentService.cs b/src/Core/Services/EnvironmentService.cs index a2479c87f..a7a9dd197 100644 --- a/src/Core/Services/EnvironmentService.cs +++ b/src/Core/Services/EnvironmentService.cs @@ -3,6 +3,7 @@ using System.Threading.Tasks; using Bit.Core.Abstractions; using Bit.Core.Models.Data; using Bit.Core.Models.Domain; +using Bit.Core.Utilities; namespace Bit.Core.Services { @@ -13,13 +14,16 @@ namespace Bit.Core.Services private readonly IApiService _apiService; private readonly IStateService _stateService; + private readonly IConditionedAwaiterManager _conditionedAwaiterManager; public EnvironmentService( IApiService apiService, - IStateService stateService) + IStateService stateService, + IConditionedAwaiterManager conditionedAwaiterManager) { _apiService = apiService; _stateService = stateService; + _conditionedAwaiterManager = conditionedAwaiterManager; } public string BaseUrl { get; set; } @@ -52,30 +56,44 @@ namespace Bit.Core.Services public async Task SetUrlsFromStorageAsync() { - var urls = await _stateService.GetEnvironmentUrlsAsync(); - if (urls == null) + try { - urls = await _stateService.GetPreAuthEnvironmentUrlsAsync(); - } - if (urls == null) - { - urls = new EnvironmentUrlData(); - } - var envUrls = new EnvironmentUrls(); - if (!string.IsNullOrWhiteSpace(urls.Base)) - { - BaseUrl = envUrls.Base = urls.Base; + var urls = await _stateService.GetEnvironmentUrlsAsync(); + if (urls == null) + { + urls = await _stateService.GetPreAuthEnvironmentUrlsAsync(); + } + if (urls == null) + { + urls = new EnvironmentUrlData(); + } + var envUrls = new EnvironmentUrls(); + if (!string.IsNullOrWhiteSpace(urls.Base)) + { + BaseUrl = envUrls.Base = urls.Base; + _apiService.SetUrls(envUrls); + + _conditionedAwaiterManager.SetAsCompleted(AwaiterPrecondition.EnvironmentUrlsInited); + return; + } + + BaseUrl = urls.Base; + WebVaultUrl = urls.WebVault; + ApiUrl = envUrls.Api = urls.Api; + IdentityUrl = envUrls.Identity = urls.Identity; + IconsUrl = urls.Icons; + NotificationsUrl = urls.Notifications; + EventsUrl = envUrls.Events = urls.Events; _apiService.SetUrls(envUrls); - return; + + _conditionedAwaiterManager.SetAsCompleted(AwaiterPrecondition.EnvironmentUrlsInited); } - BaseUrl = urls.Base; - WebVaultUrl = urls.WebVault; - ApiUrl = envUrls.Api = urls.Api; - IdentityUrl = envUrls.Identity = urls.Identity; - IconsUrl = urls.Icons; - NotificationsUrl = urls.Notifications; - EventsUrl = envUrls.Events = urls.Events; - _apiService.SetUrls(envUrls); + catch (System.Exception ex) + { + _conditionedAwaiterManager.SetException(AwaiterPrecondition.EnvironmentUrlsInited, ex); + throw; + } + } public async Task SetUrlsAsync(EnvironmentUrlData urls) diff --git a/src/Core/Utilities/ServiceContainer.cs b/src/Core/Utilities/ServiceContainer.cs index eb09bf58f..ed60f9c99 100644 --- a/src/Core/Utilities/ServiceContainer.cs +++ b/src/Core/Utilities/ServiceContainer.cs @@ -33,6 +33,7 @@ namespace Bit.Core.Utilities SearchService searchService = null; + var conditionedRunner = new ConditionedAwaiterManager(); var tokenService = new TokenService(stateService); var apiService = new ApiService(tokenService, platformUtilsService, (extras) => { @@ -82,12 +83,13 @@ namespace Bit.Core.Utilities keyConnectorService, passwordGenerationService); var exportService = new ExportService(folderService, cipherService, cryptoService); var auditService = new AuditService(cryptoFunctionService, apiService); - var environmentService = new EnvironmentService(apiService, stateService); + var environmentService = new EnvironmentService(apiService, stateService, conditionedRunner); var eventService = new EventService(apiService, stateService, organizationService, cipherService); var userVerificationService = new UserVerificationService(apiService, platformUtilsService, i18nService, cryptoService); var usernameGenerationService = new UsernameGenerationService(cryptoService, apiService, stateService); + Register(conditionedRunner); Register("tokenService", tokenService); Register("apiService", apiService); Register("appIdService", appIdService); diff --git a/src/iOS.Core/Utilities/iOSCoreHelpers.cs b/src/iOS.Core/Utilities/iOSCoreHelpers.cs index e56560b9a..d37b6a4cd 100644 --- a/src/iOS.Core/Utilities/iOSCoreHelpers.cs +++ b/src/iOS.Core/Utilities/iOSCoreHelpers.cs @@ -156,6 +156,20 @@ namespace Bit.iOS.Core.Utilities ServiceContainer.Resolve("authService").Init(); (ServiceContainer. Resolve("platformUtilsService") as MobilePlatformUtilsService).Init(); + + var accountsManager = new AccountsManager( + ServiceContainer.Resolve("broadcasterService"), + ServiceContainer.Resolve("vaultTimeoutService"), + ServiceContainer.Resolve("secureStorageService"), + ServiceContainer.Resolve("stateService"), + ServiceContainer.Resolve("platformUtilsService"), + ServiceContainer.Resolve("authService"), + ServiceContainer.Resolve("logger"), + ServiceContainer.Resolve("messagingService"), + ServiceContainer.Resolve(), + ServiceContainer.Resolve()); + ServiceContainer.Register("accountsManager", accountsManager); + // Note: This is not awaited var bootstrapTask = BootstrapAsync(postBootstrapFunc); } @@ -235,18 +249,6 @@ namespace Bit.iOS.Core.Utilities ServiceContainer.Resolve("cryptoService")); ServiceContainer.Register("verificationActionsFlowHelper", verificationActionsFlowHelper); - var accountsManager = new AccountsManager( - ServiceContainer.Resolve("broadcasterService"), - ServiceContainer.Resolve("vaultTimeoutService"), - ServiceContainer.Resolve("secureStorageService"), - ServiceContainer.Resolve("stateService"), - ServiceContainer.Resolve("platformUtilsService"), - ServiceContainer.Resolve("authService"), - ServiceContainer.Resolve("logger"), - ServiceContainer.Resolve("messagingService"), - ServiceContainer.Resolve()); - ServiceContainer.Register("accountsManager", accountsManager); - if (postBootstrapFunc != null) { await postBootstrapFunc.Invoke();