[PM-26810] Add OTP support to VerifyPasswordScreen #6034
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
New Issues (1)Checkmarx found the following issues in this Pull Request
Fixed Issues (1)Great job! The following issues were fixed in this Pull Request
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...m/x8bit/bitwarden/ui/vault/feature/exportitems/verifypassword/VerifyPasswordViewModelTest.kt
Dismissed
Show dismissed
Hide dismissed
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6034 +/- ##
==========================================
+ Coverage 84.72% 84.81% +0.09%
==========================================
Files 722 720 -2
Lines 54717 54776 +59
Branches 7583 7606 +23
==========================================
+ Hits 46358 46458 +100
+ Misses 5706 5657 -49
- Partials 2653 2661 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
SaintPatrck
requested review from
a team,
brian-livefront and
david-livefront
as code owners
...bit/bitwarden/ui/vault/feature/exportitems/verifypassword/handlers/VerifyPasswordHandlers.kt
Outdated
Show resolved
Hide resolved
...tlin/com/x8bit/bitwarden/ui/vault/feature/exportitems/verifypassword/VerifyPasswordScreen.kt
Show resolved
Hide resolved
...tlin/com/x8bit/bitwarden/ui/vault/feature/exportitems/verifypassword/VerifyPasswordScreen.kt
Show resolved
Hide resolved
...tlin/com/x8bit/bitwarden/ui/vault/feature/exportitems/verifypassword/VerifyPasswordScreen.kt
Show resolved
Hide resolved
...n/com/x8bit/bitwarden/ui/vault/feature/exportitems/verifypassword/VerifyPasswordViewModel.kt
Outdated
Show resolved
Hide resolved
| } | ||
| val message = when (val result = action.result) { | ||
| is RequestOtpResult.Error -> { | ||
| result.message?.asText() ?: BitwardenString.generic_error_message.asText() |
Collaborator
There was a problem hiding this comment.
Usually we display a dialog for errors and snackbars for success, right?
Contributor
Author
There was a problem hiding this comment.
True. Nice catch. Thanks!
...n/com/x8bit/bitwarden/ui/vault/feature/exportitems/verifypassword/VerifyPasswordViewModel.kt
Outdated
Show resolved
Hide resolved
SaintPatrck
force-pushed
the
PM-26810/cxf-prompt-for-otp-verification
branch
3 times, most recently
from
02ee06c to
8716fa1
Compare
| Spacer( | ||
| Modifier | ||
| .height(12.dp) | ||
| .navigationBarsPadding(), |
Collaborator
There was a problem hiding this comment.
Does this work? Will the height parameter override the padding?
Contributor
Author
There was a problem hiding this comment.
Unless I misunderstand, chaining modifiers is cumulative. Not to mention, these modifiers do different things. height literally sets the height of the Spacer while navigationBarsPadding() adds additional padding below the spacer, if needed. I'm not seeing any visual difference when this is used versus two separate spacers.
Collaborator
There was a problem hiding this comment.
Setting the height, will not allow the padding to exceed the value set. so you end up with a total of 16pd of space, instead of 16 + padding.
You can see the difference here:
Contributor
Author
There was a problem hiding this comment.
It's way more obvious on that screen than the VaultScreens. 🤦
Thanks for explaining. I'll keep that in mind. 👍
SaintPatrck
force-pushed
the
PM-26810/cxf-prompt-for-otp-verification
branch
from
8716fa1 to
e53e995
Compare
This commit introduces support for One-Time Passcode (OTP) verification on the "Verify Password" screen, in addition to the existing master password verification. This is primarily for TDE users who do not have a master password set on their account.
The behavioral changes are as follows:
* If a user does not have a master password, they will be prompted to enter a 6-digit OTP code that is automatically sent to their email address.
* The UI displays a "Resend code" button for the OTP flow.
* The primary action button label has been changed from "Unlock" to "Continue" to accommodate both verification flows.
Specific changes include:
* **ViewModel:**
* `VerifyPasswordViewModel` now checks if the active account has a master password.
* If no master password exists, it automatically requests an OTP and updates the UI to reflect the OTP verification flow.
* New actions (`ResendCodeClick`, `SendOtpCodeResultReceive`, `VerifyOtpResultReceive`) and events (`ShowSnackbar`) have been added to handle the OTP logic.
* The `UnlockClick` action has been renamed to `ContinueClick`.
* The state has been updated to include a `title`, `subtext`, and `showResendCodeButton` to dynamically adjust the screen content.
* **UI (Screen & Composables):**
* `VerifyPasswordScreen.kt` and its content composable have been updated to conditionally display either the master password field or the OTP code field based on the view model's state.
* A "Resend code" button is now shown for the OTP flow.
* The main button is now labeled "Continue" and triggers the `ContinueClick` action.
* A snackbar is now used to show feedback, for instance when an OTP code has been resent.
* **Strings:**
* New string resources have been added for the OTP verification flow, such as "Verify your account email address".
* **Tests:**
* `VerifyPasswordViewModelTest` and `VerifyPasswordScreenTest` have been extensively updated to cover the new OTP verification logic, state changes, and UI interactions. Existing tests were refactored to align with the new logic.
SaintPatrck
force-pushed
the
PM-26810/cxf-prompt-for-otp-verification
branch
from
e53e995 to
8d3011a
Compare
david-livefront
approved these changes
Oct 16, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
PM-26810
📔 Objective
This commit introduces support for One-Time Passcode (OTP) verification on the "Verify Password" screen, in addition to the existing master password verification. This is primarily for TDE users who do not have a master password set on their account.
The behavioral changes are as follows:
Specific changes include:
VerifyPasswordViewModelnow checks if the active account has a master password.ResendCodeClick,SendOtpCodeResultReceive,VerifyOtpResultReceive) and events (ShowSnackbar) have been added to handle the OTP logic.UnlockClickaction has been renamed toContinueClick.title,subtext, andshowResendCodeButtonto dynamically adjust the screen content.VerifyPasswordScreen.ktand its content composable have been updated to conditionally display either the master password field or the OTP code field based on the view model's state.ContinueClickaction.VerifyPasswordViewModelTestandVerifyPasswordScreenTesthave been extensively updated to cover the new OTP verification logic, state changes, and UI interactions. Existing tests were refactored to align with the new logic.📸 Screenshots
Master Password verification
OTP Verification
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes