From 5a4a54f4af328a0b49dacf5ff5a4521f92cce4a4 Mon Sep 17 00:00:00 2001 From: Dinis Vieira Date: Tue, 16 Apr 2024 21:52:52 +0100 Subject: [PATCH] [PM-7385] Fix for allowing switching accounts while creating a passkey of Android (#3155) * PM-7385 Fixed for allowing switching accounts while creating a passkey on Android. This fixes also include scenarios where we need to unlock the vault after switching Also fixed the issue where tapping on cipher won't do anything after switching. * PM-7385 ensure the Options.Fido2CredentialAction and FromFido2Framework are reset when the Credential flow is started to avoid erratic behaviors when switching accounts, app is in background or other edge case scenarios. These properties where replaced by calls to _fido2MakeCredentialConfirmationUserInterface.IsConfirmingNewCredential instead. * Minor changes and added comments * [PM-7385] Implemented several changes suggested in PR for better/cleaner code. * PM-7385 Added several minor code improvemments. --- .../Fido2MakeCredentialUserInterface.cs | 40 +++++++++++++++---- .../Android/Services/DeviceActionService.cs | 6 ++- ...MakeCredentialConfirmationUserInterface.cs | 16 ++++++++ src/Core/App.xaml.cs | 26 +++++++++--- src/Core/Constants.cs | 2 + .../Vault/AutofillCiphersPageViewModel.cs | 23 ++++------- .../Pages/Vault/CipherAddEditPage.xaml.cs | 3 +- .../Pages/Vault/CipherAddEditPageViewModel.cs | 2 +- src/Core/Pages/Vault/CiphersPageViewModel.cs | 7 +--- .../AccountManagement/AccountsManager.cs | 9 ++++- src/Core/Utilities/AppHelpers.cs | 16 ++++++-- 11 files changed, 110 insertions(+), 40 deletions(-) diff --git a/src/App/Platforms/Android/Autofill/Fido2MakeCredentialUserInterface.cs b/src/App/Platforms/Android/Autofill/Fido2MakeCredentialUserInterface.cs index 94fc70b24..f68ecaf7b 100644 --- a/src/App/Platforms/Android/Autofill/Fido2MakeCredentialUserInterface.cs +++ b/src/App/Platforms/Android/Autofill/Fido2MakeCredentialUserInterface.cs @@ -15,8 +15,10 @@ namespace Bit.App.Platforms.Android.Autofill private readonly IUserVerificationMediatorService _userVerificationMediatorService; private readonly IDeviceActionService _deviceActionService; private readonly IPlatformUtilsService _platformUtilsService; - + private LazyResolve _messagingService = new LazyResolve(); + private TaskCompletionSource<(string cipherId, bool? userVerified)> _confirmCredentialTcs; + private TaskCompletionSource _unlockVaultTcs; private Fido2UserVerificationOptions? _currentDefaultUserVerificationOptions; private Func _checkHasVaultBeenUnlockedInThisTransaction; @@ -37,6 +39,9 @@ namespace Bit.App.Platforms.Android.Autofill public bool HasVaultBeenUnlockedInThisTransaction => _checkHasVaultBeenUnlockedInThisTransaction?.Invoke() == true; + public bool IsConfirmingNewCredential => _confirmCredentialTcs?.Task != null && !_confirmCredentialTcs.Task.IsCompleted; + public bool IsWaitingUnlockVault => _unlockVaultTcs?.Task != null && !_unlockVaultTcs.Task.IsCompleted; + public async Task<(string CipherId, bool UserVerified)> ConfirmNewCredentialAsync(Fido2ConfirmNewCredentialParams confirmNewCredentialParams) { _confirmCredentialTcs?.TrySetCanceled(); @@ -44,9 +49,8 @@ namespace Bit.App.Platforms.Android.Autofill _confirmCredentialTcs = new TaskCompletionSource<(string cipherId, bool? userVerified)>(); _currentDefaultUserVerificationOptions = new Fido2UserVerificationOptions(false, confirmNewCredentialParams.UserVerificationPreference, HasVaultBeenUnlockedInThisTransaction, confirmNewCredentialParams.RpId); - - var messagingService = ServiceContainer.Resolve("messagingService"); - messagingService?.Send("fidoNavigateToAutofillCipher", confirmNewCredentialParams); + + _messagingService.Value.Send(Bit.Core.Constants.CredentialNavigateToAutofillCipherMessageCommand, confirmNewCredentialParams); var (cipherId, isUserVerified) = await _confirmCredentialTcs.Task; @@ -99,11 +103,32 @@ namespace Bit.App.Platforms.Android.Autofill public async Task EnsureUnlockedVaultAsync() { - if (!await _stateService.IsAuthenticatedAsync() || await _vaultTimeoutService.IsLockedAsync()) + if (!await _stateService.IsAuthenticatedAsync() + || + await _vaultTimeoutService.IsLoggedOutByTimeoutAsync() + || + await _vaultTimeoutService.ShouldLogOutByTimeoutAsync()) { - // this should never happen but just in case. - throw new InvalidOperationException("Not authed or vault locked"); + await NavigateAndWaitForUnlockAsync(Bit.Core.Enums.NavigationTarget.HomeLogin); + return; } + + if (!await _vaultTimeoutService.IsLockedAsync()) + { + return; + } + + await NavigateAndWaitForUnlockAsync(Bit.Core.Enums.NavigationTarget.Lock); + } + + private async Task NavigateAndWaitForUnlockAsync(Bit.Core.Enums.NavigationTarget navTarget) + { + _unlockVaultTcs?.TrySetCanceled(); + _unlockVaultTcs = new TaskCompletionSource(); + + _messagingService.Value.Send(Bit.Core.Constants.NavigateToMessageCommand, navTarget); + + await _unlockVaultTcs.Task; } public Task InformExcludedCredentialAsync(string[] existingCipherIds) @@ -118,6 +143,7 @@ namespace Bit.App.Platforms.Android.Autofill } public void Confirm(string cipherId, bool? userVerified) => _confirmCredentialTcs?.TrySetResult((cipherId, userVerified)); + public void ConfirmVaultUnlocked() => _unlockVaultTcs?.TrySetResult(true); public async Task ConfirmAsync(string cipherId, bool alreadyHasFido2Credential, bool? userVerified) { diff --git a/src/App/Platforms/Android/Services/DeviceActionService.cs b/src/App/Platforms/Android/Services/DeviceActionService.cs index e75843806..82174220f 100644 --- a/src/App/Platforms/Android/Services/DeviceActionService.cs +++ b/src/App/Platforms/Android/Services/DeviceActionService.cs @@ -583,7 +583,11 @@ namespace Bit.Droid.Services await ExecuteFido2CreateCredentialAsync(); } - appOptions.Fido2CredentialAction = null; //Clear CredentialAction Value + // Clear CredentialAction and FromFido2Framework values to avoid erratic behaviors in subsequent navigation/flows + // For Fido2CredentialGet these are no longer needed as a new Activity will be initiated. + // For Fido2CredentialCreate the app will rely on IFido2MakeCredentialConfirmationUserInterface.IsConfirmingNewCredential + appOptions.Fido2CredentialAction = null; + appOptions.FromFido2Framework = false; } private async Task ExecuteFido2GetCredentialAsync(AppOptions appOptions) diff --git a/src/Core/Abstractions/IFido2MakeCredentialConfirmationUserInterface.cs b/src/Core/Abstractions/IFido2MakeCredentialConfirmationUserInterface.cs index 81073c7c5..e2ec22614 100644 --- a/src/Core/Abstractions/IFido2MakeCredentialConfirmationUserInterface.cs +++ b/src/Core/Abstractions/IFido2MakeCredentialConfirmationUserInterface.cs @@ -43,6 +43,22 @@ namespace Bit.Core.Abstractions /// void OnConfirmationException(Exception ex); + + /// + /// True if we are already confirming a new credential. + /// + bool IsConfirmingNewCredential { get; } + + /// + /// Call this after the vault was unlocked so that Fido2 credential creation can proceed. + /// + void ConfirmVaultUnlocked(); + + /// + /// True if we are waiting for the vault to be unlocked. + /// + bool IsWaitingUnlockVault { get; } + Fido2UserVerificationOptions? GetCurrentUserVerificationOptions(); void SetCheckHasVaultBeenUnlockedInThisTransaction(Func checkHasVaultBeenUnlockedInThisTransaction); diff --git a/src/Core/App.xaml.cs b/src/Core/App.xaml.cs index df8ba9306..fa9b448f9 100644 --- a/src/Core/App.xaml.cs +++ b/src/Core/App.xaml.cs @@ -38,6 +38,7 @@ namespace Bit.App private readonly IPushNotificationService _pushNotificationService; private readonly IConfigService _configService; private readonly ILogger _logger; + private LazyResolve _userVerificationMediatorService = new LazyResolve(); private static bool _isResumed; // these variables are static because the app is launching new activities on notification click, creating new instances of App. @@ -280,7 +281,7 @@ namespace Bit.App } } } - else if (message.Command == "fidoNavigateToAutofillCipher" && message.Data is Fido2ConfirmNewCredentialParams createParams) + else if (message.Command == Constants.CredentialNavigateToAutofillCipherMessageCommand && message.Data is Fido2ConfirmNewCredentialParams createParams) { ArgumentNullException.ThrowIfNull(MainPage); ArgumentNullException.ThrowIfNull(Options); @@ -331,18 +332,24 @@ namespace Bit.App || message.Command == "unlocked" || message.Command == AccountsManagerMessageCommands.ACCOUNT_SWITCH_COMPLETED) { - if (message.Command == AccountsManagerMessageCommands.ACCOUNT_SWITCH_COMPLETED) + if (message.Command == AccountsManagerMessageCommands.ACCOUNT_SWITCH_COMPLETED && _userVerificationMediatorService.Value.IsConfirmingNewCredential) { - var userVerificationMediatorService = ServiceContainer.Resolve(); - userVerificationMediatorService?.OnConfirmationException(new AccountSwitchedException()); + _userVerificationMediatorService.Value.OnConfirmationException(new AccountSwitchedException()); } - + lock (_processingLoginRequestLock) { // lock doesn't allow for async execution CheckPasswordlessLoginRequestsAsync().Wait(); } } + else if (message.Command == Constants.NavigateToMessageCommand && message.Data is NavigationTarget navigationTarget) + { + await MainThread.InvokeOnMainThreadAsync(() => + { + Navigate(navigationTarget, null); + }); + } } catch (Exception ex) { @@ -713,6 +720,15 @@ namespace Bit.App // If we are in background we add the Navigation Actions to a queue to execute when the app resumes. // Links: https://github.com/dotnet/maui/issues/11501 and https://bitwarden.atlassian.net/wiki/spaces/NMME/pages/664862722/MainPage+Assignments+not+working+on+Android+on+Background+or+App+resume #if ANDROID + if (_userVerificationMediatorService != null && _userVerificationMediatorService.Value.IsConfirmingNewCredential) + { + // if it's creating passkey + // and we have an active pending TaskCompletionSource + // then we let the Fido2 Authenticator flow manage the navigation to avoid issues + // like duplicated navigation to lock page. + return; + } + if (!_isResumed) { _onResumeActions.Enqueue(() => NavigateImpl(navTarget, navParams)); diff --git a/src/Core/Constants.cs b/src/Core/Constants.cs index e11716326..a56127385 100644 --- a/src/Core/Constants.cs +++ b/src/Core/Constants.cs @@ -48,6 +48,8 @@ namespace Bit.Core public const string DisplayEuEnvironmentFlag = "display-eu-environment"; public const string RegionEnvironment = "regionEnvironment"; public const string DuoCallback = "bitwarden://duo-callback"; + public const string NavigateToMessageCommand = "navigateTo"; + public const string CredentialNavigateToAutofillCipherMessageCommand = "credentialNavigateToAutofillCipher"; /// /// This key is used to store the value of "ShouldConnectToWatch" of the last user that had logged in diff --git a/src/Core/Pages/Vault/AutofillCiphersPageViewModel.cs b/src/Core/Pages/Vault/AutofillCiphersPageViewModel.cs index 47df6d26e..38d2bfa8a 100644 --- a/src/Core/Pages/Vault/AutofillCiphersPageViewModel.cs +++ b/src/Core/Pages/Vault/AutofillCiphersPageViewModel.cs @@ -7,16 +7,13 @@ using Bit.Core.Exceptions; using Bit.Core.Models.View; using Bit.Core.Resources.Localization; using Bit.Core.Utilities; -using Bit.Core.Utilities.Fido2; namespace Bit.App.Pages { public class AutofillCiphersPageViewModel : CipherSelectionPageViewModel { private CipherType? _fillType; - private bool _isAndroidFido2CredentialCreation; private AppOptions _appOptions; - private readonly LazyResolve _fido2MakeCredentialConfirmationUserInterface = new LazyResolve(); public string Uri { get; set; } @@ -25,7 +22,6 @@ namespace Bit.App.Pages { Uri = appOptions?.Uri; _fillType = appOptions.FillType; - _isAndroidFido2CredentialCreation = appOptions.FromFido2Framework; _appOptions = appOptions; string name = null; @@ -44,7 +40,7 @@ namespace Bit.App.Pages Name = name; PageTitle = string.Format(AppResources.ItemsForUri, Name ?? "--"); NoDataText = string.Format(AppResources.NoItemsForUri, Name ?? "--"); - AddNewItemText = appOptions.FromFido2Framework ? AppResources.SavePasskeyAsNewLogin : AppResources.AddAnItem; + AddNewItemText = _fido2MakeCredentialConfirmationUserInterface.Value.IsConfirmingNewCredential ? AppResources.SavePasskeyAsNewLogin : AppResources.AddAnItem; } protected override async Task> LoadGroupedItemsAsync() @@ -54,7 +50,7 @@ namespace Bit.App.Pages var matching = ciphers.Item1?.Select(c => new CipherItemViewModel(c, WebsiteIconsEnabled) { - UsePasskeyIconAsPlaceholderFallback = _isAndroidFido2CredentialCreation + UsePasskeyIconAsPlaceholderFallback = _fido2MakeCredentialConfirmationUserInterface.Value.IsConfirmingNewCredential }).ToList(); var hasMatching = matching?.Any() ?? false; @@ -91,12 +87,9 @@ namespace Bit.App.Pages return; } - if (_appOptions.FromFido2Framework) + if (_fido2MakeCredentialConfirmationUserInterface.Value.IsConfirmingNewCredential) { - if (_appOptions.Fido2CredentialAction == CredentialProviderConstants.Fido2CredentialCreate) - { - await _fido2MakeCredentialConfirmationUserInterface.Value.ConfirmAsync(cipher.Id, cipher.Login.HasFido2Credentials, null); - } + await _fido2MakeCredentialConfirmationUserInterface.Value.ConfirmAsync(cipher.Id, cipher.Login.HasFido2Credentials, null); return; } @@ -155,7 +148,7 @@ namespace Bit.App.Pages protected override async Task AddFabCipherAsync() { //Scenario for creating a new Fido2 credential on Android but showing the Cipher Page - if (_isAndroidFido2CredentialCreation) + if (_fido2MakeCredentialConfirmationUserInterface.Value.IsConfirmingNewCredential) { var pageForOther = new CipherAddEditPage(null, CipherType.Login, appOptions: _appOptions); await Page.Navigation.PushModalAsync(new NavigationPage(pageForOther)); @@ -170,7 +163,7 @@ namespace Bit.App.Pages protected override async Task AddCipherAsync() { //Scenario for creating a new Fido2 credential on Android - if (_isAndroidFido2CredentialCreation) + if (_fido2MakeCredentialConfirmationUserInterface.Value.IsConfirmingNewCredential) { _fido2MakeCredentialConfirmationUserInterface.Value.Confirm(null, null); return; @@ -190,9 +183,7 @@ namespace Bit.App.Pages public void Cancel() { - if (_appOptions?.FromFido2Framework == true - && - _appOptions?.Fido2CredentialAction == CredentialProviderConstants.Fido2CredentialCreate) + if (_fido2MakeCredentialConfirmationUserInterface.Value.IsConfirmingNewCredential) { _fido2MakeCredentialConfirmationUserInterface.Value.Cancel(); } diff --git a/src/Core/Pages/Vault/CipherAddEditPage.xaml.cs b/src/Core/Pages/Vault/CipherAddEditPage.xaml.cs index 45ad912b2..4a9744d9b 100644 --- a/src/Core/Pages/Vault/CipherAddEditPage.xaml.cs +++ b/src/Core/Pages/Vault/CipherAddEditPage.xaml.cs @@ -19,6 +19,7 @@ namespace Bit.App.Pages private readonly IAutofillHandler _autofillHandler; private readonly IVaultTimeoutService _vaultTimeoutService; private readonly IUserVerificationService _userVerificationService; + private readonly LazyResolve _fido2MakeCredentialConfirmationUserInterface = new LazyResolve(); private CipherAddEditPageViewModel _vm; private bool _fromAutofill; @@ -45,7 +46,7 @@ namespace Bit.App.Pages _appOptions = appOptions; _fromAutofill = fromAutofill; FromAutofillFramework = _appOptions?.FromAutofillFramework ?? false; - FromAndroidFido2Framework = _appOptions?.FromFido2Framework ?? false; + FromAndroidFido2Framework = _fido2MakeCredentialConfirmationUserInterface.Value.IsConfirmingNewCredential; InitializeComponent(); _vm = BindingContext as CipherAddEditPageViewModel; _vm.Page = this; diff --git a/src/Core/Pages/Vault/CipherAddEditPageViewModel.cs b/src/Core/Pages/Vault/CipherAddEditPageViewModel.cs index 5e5376d5f..0797e6218 100644 --- a/src/Core/Pages/Vault/CipherAddEditPageViewModel.cs +++ b/src/Core/Pages/Vault/CipherAddEditPageViewModel.cs @@ -332,7 +332,7 @@ namespace Bit.App.Pages public async Task LoadAsync(AppOptions appOptions = null) { _fromOtp = appOptions?.OtpData != null; - IsFromFido2Framework = appOptions?.FromFido2Framework ?? false; + IsFromFido2Framework = _fido2MakeCredentialConfirmationUserInterface.IsConfirmingNewCredential; var myEmail = await _stateService.GetEmailAsync(); OwnershipOptions.Add(new KeyValuePair(myEmail, null)); diff --git a/src/Core/Pages/Vault/CiphersPageViewModel.cs b/src/Core/Pages/Vault/CiphersPageViewModel.cs index 7fcbb532a..cfa13324b 100644 --- a/src/Core/Pages/Vault/CiphersPageViewModel.cs +++ b/src/Core/Pages/Vault/CiphersPageViewModel.cs @@ -175,12 +175,9 @@ namespace Bit.App.Pages public async Task SelectCipherAsync(CipherView cipher) { - if (_appOptions.FromFido2Framework) + if (_fido2MakeCredentialConfirmationUserInterface.Value.IsConfirmingNewCredential) { - if (_appOptions.Fido2CredentialAction == CredentialProviderConstants.Fido2CredentialCreate) - { - await _fido2MakeCredentialConfirmationUserInterface.Value.ConfirmAsync(cipher.Id, cipher.Login.HasFido2Credentials, null); - } + await _fido2MakeCredentialConfirmationUserInterface.Value.ConfirmAsync(cipher.Id, cipher.Login.HasFido2Credentials, null); return; } diff --git a/src/Core/Utilities/AccountManagement/AccountsManager.cs b/src/Core/Utilities/AccountManagement/AccountsManager.cs index c557a67d0..5f1a9962e 100644 --- a/src/Core/Utilities/AccountManagement/AccountsManager.cs +++ b/src/Core/Utilities/AccountManagement/AccountsManager.cs @@ -20,6 +20,7 @@ namespace Bit.App.Utilities.AccountManagement private readonly IMessagingService _messagingService; private readonly IWatchDeviceService _watchDeviceService; private readonly IConditionedAwaiterManager _conditionedAwaiterManager; + private LazyResolve _userVerificationMediatorService = new LazyResolve(); Func _getOptionsFunc; private IAccountsManagerHost _accountsManagerHost; @@ -82,7 +83,7 @@ namespace Bit.App.Utilities.AccountManagement if (authed) { if (await _vaultTimeoutService.IsLoggedOutByTimeoutAsync() || - await _vaultTimeoutService.ShouldLogOutByTimeoutAsync()) + await _vaultTimeoutService.ShouldLogOutByTimeoutAsync()) { // TODO implement orgIdentifier flow to SSO Login page, same as email flow below // var orgIdentifier = await _stateService.GetOrgIdentifierAsync(); @@ -100,6 +101,12 @@ namespace Bit.App.Utilities.AccountManagement { _accountsManagerHost.Navigate(NavigationTarget.AddEditCipher); } + else if (_userVerificationMediatorService.Value.IsConfirmingNewCredential) + { + // If we are already confirming a credential we don't need to navigate again. + // This could happen when switching accounts for example. + return; + } else if (Options.FromFido2Framework) { var deviceActionService = Bit.Core.Utilities.ServiceContainer.Resolve(); diff --git a/src/Core/Utilities/AppHelpers.cs b/src/Core/Utilities/AppHelpers.cs index a4a8c2da2..fe3f8f974 100644 --- a/src/Core/Utilities/AppHelpers.cs +++ b/src/Core/Utilities/AppHelpers.cs @@ -431,21 +431,31 @@ namespace Bit.App.Utilities { // this is called after login in or unlocking so we can assume the vault has been unlocked in this transaction here. appOptions.HasUnlockedInThisTransaction = true; - - ServiceContainer.Resolve() - .SetCheckHasVaultBeenUnlockedInThisTransaction(() => appOptions?.HasUnlockedInThisTransaction == true); + + var userVerificationMediatorService = ServiceContainer.Resolve(); + userVerificationMediatorService.SetCheckHasVaultBeenUnlockedInThisTransaction(() => appOptions?.HasUnlockedInThisTransaction == true); if (appOptions.FromAutofillFramework && appOptions.SaveType.HasValue) { App.MainPage = new NavigationPage(new CipherAddEditPage(appOptions: appOptions)); return true; } + + // If we are waiting for an unlock vault we don't want to trigger 'ExecuteFido2CredentialActionAsync' again, + // as it's already running. We just need to 'ConfirmUnlockVault' on the 'userVerificationMediatorService'. + if (userVerificationMediatorService.IsWaitingUnlockVault) + { + userVerificationMediatorService.ConfirmVaultUnlocked(); + return true; + } + if (appOptions.FromFido2Framework && !string.IsNullOrWhiteSpace(appOptions.Fido2CredentialAction)) { var deviceActionService = Bit.Core.Utilities.ServiceContainer.Resolve(); deviceActionService.ExecuteFido2CredentialActionAsync(appOptions).FireAndForget(); return true; } + if (appOptions.Uri != null || appOptions.OtpData != null)