[PM-8221] Route user to email OTP entry [clients] #12811
+631
−56
Conversation
This file contains 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
This reverts commit 3db8357.
…of processing (2) processDeviceVerificationResponse should cache data
… (2) Update form to use bitSubmit (3) Add todo for authenticating guard
|
Fixed Issues (170)Great job! The following issues were fixed in this Pull Request
|
…ttps://github.com/bitwarden/clients into auth/pm-8221/route-user-to-email-otp-entry-clients
JaredSnider-Bitwarden
requested changes
Jan 13, 2025
| @@ -236,6 +239,22 @@ const routes: Routes = [ | |||
| ], | |||
| }, | |||
| ), | |||
| { | |||
| path: "device-verification", | |||
| component: AnonLayoutWrapperComponent, | |||
There was a problem hiding this comment.
ExtensionAnonLayoutWrapperComponent with ExtensionAnonLayoutWrapperData. We should have a back button for the new page per Figma:
| [disabled]="formGroup.invalid" | ||
| > | ||
| {{ "continueLoggingIn" | i18n }} | ||
| </button> |
There was a problem hiding this comment.
💡 , since we have a back button or x on the extension, IOS, and Android, I wonder if we should have an explicit cancel button here as well for desktop and web (using the new device verification service to inform the component to show the button or not). Can you run that by product / design? Note: the bitwarden logo does take the user back to login so that might be deemed sufficient, but we do have a cancel on 2FA which is an comparable screen to this one.
There was a problem hiding this comment.
There was a problem hiding this comment.
❓ : is this still necessary after we updated the NewDeviceVerificationComponent to use the LoginStrategyService instead of the PasswordLoginStrategy directly?
There was a problem hiding this comment.
Nice test suite!
There was a problem hiding this comment.
Thank you for tackling the ts strict updates. Since this is the orchestrator of all login methods, please request a smoke test of all the touched strategies in the QA notes for this task.
There was a problem hiding this comment.
❓ can we add tests for this please?
There was a problem hiding this comment.
❓ can we add tests for this please?
| * @param {IdentityDeviceVerificationResponse} response - The response from the server indicating that device verification is required. | ||
| * @returns {Promise<AuthResult>} - A promise that resolves to an AuthResult object | ||
| */ | ||
| protected async processDeviceVerificationResponse( |
There was a problem hiding this comment.
❓ can we add tests for this please?
| .pipe(takeUntil(this.destroy$)) | ||
| .subscribe((timedOut) => { | ||
| if (timedOut) { | ||
| this.logService.error("Session timed out."); |
There was a problem hiding this comment.
💡 , we probably should show a toast to the user in this scenario?
| await this.router.navigate(["/update-temp-password"]); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
❓ : We should for sure add a call to await this.syncService.fullSync(true); here.
However, can you investigate to see if we also need a call like this.loginEmailService.clearValues(); like 2FA has?
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
https://bitwarden.atlassian.net/browse/PM-8221?atlOrigin=eyJpIjoiOTdiY2RkOWIwYmFhNDUyMGI1MDA3MDM0M2ZkMGI1OTkiLCJwIjoiaiJ9
📔 Objective
Add an intermediate route (
/device-verification) that requires a user to enter an OTP to proceed to the vault when they are attempting to log in on an unknown device.📸 Screenshots
Web
GMT20250110-202812_Clip_Alec.Rippberger.s.Clip.01_10_2025.mp4
Desktop
GMT20250110-200921_Clip_Alec.Rippberger.s.Clip.01_10_2025.mp4
Browser Extension
GMT20250110-192642_Clip_Alec.Rippberger.s.Clip.01_10_2025.mp4
⏰ 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