Skip to content

[EC-981] OTP handling - Set to selected cipher#2404

Merged
fedemkr merged 8 commits intofeature/EC-979-iOS-third-party-2fafrom
feature/ios-third-party-2fa/EC-981-add-to-existing-cipher
Mar 7, 2023
Merged

[EC-981] OTP handling - Set to selected cipher#2404
fedemkr merged 8 commits intofeature/EC-979-iOS-third-party-2fafrom
feature/ios-third-party-2fa/EC-981-add-to-existing-cipher

Conversation

@fedemkr
Copy link
Member

@fedemkr fedemkr commented Mar 6, 2023

Type of change

  • Bug fix
  • New feature development
  • Tech debt (refactoring, code cleanup, dependency upgrades, etc)
  • Build/deploy pipeline (DevOps)
  • Other

Objective

Add the possibility to choose a cipher to set the scanned OTP handled by the Bitwarden app.

Code changes

AutofillCiphersPage -> CipherSelectionPage

Given that selecting a cipher on Autofill and selecting it on OTP handling is pretty much the same, the AutofillCiphersPage was taken and renamed to CipherSelectionPage to have a common UI to share between these flows. The context changes depending on the AppOptions passed to the page creating the appropriate ViewModel for each flow.

A diagram can illustrate this better:

classDiagram
    class CipherSelectionPageViewModel
    <<Abstract>> CipherSelectionPageViewModel
    CipherSelectionPageViewModel <|-- AutofillCiphersPageViewModel
    CipherSelectionPageViewModel <|-- OTPCipherSelectionPageViewModel
    CipherSelectionPage --> CipherSelectionPageViewModel
Loading

Other changes

  • AccountsManager: Added method to start the navigation flow (as in changing an account) that allows the caller to perform any action to the current AppOptions
  • DeepLinkContext: Added method to handle new incoming Uris
  • App.xaml.cs: Added otp handling logic and updated some hardcoded message commands values with consts
  • MainActivity: Updated hardcoded message commands with consts
  • AppOptions: Added OtpData to get the data from the otp uri
  • CipherSelectionPage.xaml: Added close toolbar item and updated obsolete properties. Also, changed selection to new approach and added callout for the OTP cipher selection.
  • CipherSelectionPage.xaml.cs: Added logic to choose between the Autofill and OTP viewmodels depending on the AppOptions. Also, added logic for Close. Something to highlight, is that the Account Switching toolbar item was left with the default .. icon on iOS because there is an issue and it's not being updated correctly (it just shows a white backgrounded circle). So, for now I've just left it with the default one which IMO is better than the white circle.
  • CipherSelectionPageViewModel.cs: Moved common things to this abstract class and added template methods to be implemented by the children
  • AutofillCiphersPageViewModel: Add inheritance from CipherSelectionPageVIewModel and removed things that are now in the abstract class. Additionally, the select flow was changed to accept an IGroupingsPageListItem to be more generic and flexible.
  • OTPCipherSelectionPageVIewModel: Implemented for OTP handling given the base CipherSelectionPageVIewModel class. The suggested ciphers to show on loading are the ones which include the Issuer of the OTP if exists or the AccountName otherwise.
  • CipherAddEditPageViewModel: Added automatic set of OTP key when coming from OTP flow, the tooltip and the appropriate navigation when saving.
  • CiphersPage.xaml.cs: Moved some initialization to the VM using the Prepare(...) method to have less logic on the view.
  • CiphersPageViewModel: Updated CipherOptionsCommand to use the new async way. Added logic to show all ciphers if there's no search term and it comes from the OTP flow. Also, on selection it goes to the edit page of the cipher when coming from the OTP flow.
  • AppHelpers: Added navigation logic for OTP flow
  • NavigationTarget: Added new target for OTP flow
  • OTPData: Added this struct which is the responsible to get all the attributes/parameters from an OTP Uri to be reused throughout the app.
  • TOTPService: Updated it to use the new OTPData struct.
  • UriExtensions: Added method to unescape a URI string like a OTP URI (it needs to be done multiple times to correctly unescape everything)

Screenshots

Cipher Selection

Cipher selection OTP

Cipher Search

Cipher search OTP

OTP set on Cipher edition

Cipher edition OTP set

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

@fedemkr fedemkr added the ios label Mar 6, 2023
@fedemkr fedemkr changed the base branch from master to feature/EC-979-iOS-third-party-2fa March 6, 2023 17:33
@fedemkr fedemkr requested review from a team and removed request for a team March 6, 2023 17:41
@fedemkr fedemkr added hold do not merge yet ios and removed ios labels Mar 6, 2023
@fedemkr fedemkr requested a review from a team March 6, 2023 20:29
@fedemkr fedemkr removed the hold do not merge yet label Mar 6, 2023
// TODO: There's currently an issue on iOS where the toolbar item is not getting updated
// as the others somehow. Removing this so at least we get the circle with ".." instead
// of a white circle
if (Device.RuntimePlatform != Device.iOS)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible the change of Priority of the avatar from -1 to -2 broke the original colorization trick you did for iOS? (I don't see any hardcoded positions but I may be missing something)

Copy link
Member Author

@fedemkr fedemkr Mar 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great suggestion and I think I didn't try that but I've created another task to track this issue so that I make progress with the whole feature. On this PR I won't be fixing this given that's something minor and can be fixed later on.

@fedemkr fedemkr requested a review from mpbw2 March 7, 2023 15:29
@fedemkr fedemkr merged commit 04d2002 into feature/EC-979-iOS-third-party-2fa Mar 7, 2023
@fedemkr fedemkr deleted the feature/ios-third-party-2fa/EC-981-add-to-existing-cipher branch March 7, 2023 15:41
fedemkr added a commit that referenced this pull request Mar 9, 2023
* [EC-980] Added iOS otpauth handler (#2370)

* EC-980 added Bitwarden as otpauth scheme handler

* EC-980 Fix format

* [EC-981] OTP handling - Set to selected cipher (#2404)

* EC-981 Started adding OTP to existing cipher. Reused AutofillCiphersPage for the cipher selection and refactored it so that we have more code reuse

* EC-981 Fix navigation on otp handling

* EC-981 Fix formatting

* EC-981 Added otp cipher selection callout and add close toolbar item when needed

* PM-1131 implemented cipher creation from otp handling flow with otp key filled (#2407)

* PM-1133 Updated empty states for search and cipher selection on otp flow (#2408)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants