Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโ€™ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PM-8221] Route user to email OTP entry [clients] #12811

Open
wants to merge 41 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
2877a71
Add new IdentityDeviceVerificationResponse
alec-livefront Dec 27, 2024
b741e45
Respond with IdentityDeviceVerificationResponse
alec-livefront Dec 27, 2024
89cba4f
Add AuthResult and sessionTimeout
alec-livefront Dec 27, 2024
d3d5ee6
Fix imports and integrate IdentityDeviceVerificationResponse
alec-livefront Dec 28, 2024
1d98fa8
Add new device verification component
alec-livefront Dec 28, 2024
0504513
Set up DeviceVerificationRequest
alec-livefront Dec 30, 2024
b377236
Update translations
alec-livefront Jan 2, 2025
1eb8c5a
Handle session timeout and fix resend code
alec-livefront Jan 3, 2025
3db8357
Keep old translation
alec-livefront Jan 7, 2025
4188e4a
Revert "Keep old translation"
alec-livefront Jan 7, 2025
c71bf55
Update AuthResult
alec-livefront Jan 7, 2025
6b637c7
Merge branch 'main' into auth/pm-8221/route-user-to-email-otp-entry-cโ€ฆ
alec-livefront Jan 7, 2025
03fbd03
Update check for new device verification
alec-livefront Jan 8, 2025
818e0fd
PM-8221 - LoginStrategy - (1) Move new device verification to bottomโ€ฆ
JaredSnider-Bitwarden Jan 8, 2025
5d6fa5c
PM-8221 - (1) Update logInNewDeviceVerification to just accept strinโ€ฆ
JaredSnider-Bitwarden Jan 8, 2025
a5b295a
PM-8221 - Tweak response and request models.
JaredSnider-Bitwarden Jan 8, 2025
6e9337a
PM-8221 - Add todo
JaredSnider-Bitwarden Jan 8, 2025
bb49296
Cleanup and rework logInNewDeviceVerification
alec-livefront Jan 9, 2025
c2d4ee2
Merge branch 'main' into auth/pm-8221/route-user-to-email-otp-entry-cโ€ฆ
alec-livefront Jan 9, 2025
fc7754b
Fix import and add logging
alec-livefront Jan 9, 2025
5baac33
Add newDeviceVerificationGuard
alec-livefront Jan 9, 2025
3fda156
Cleanup and fix strict type errors
alec-livefront Jan 10, 2025
0c5a559
Cleanup debug
alec-livefront Jan 10, 2025
426a3dd
Merge branch 'main' into auth/pm-8221/route-user-to-email-otp-entry-cโ€ฆ
alec-livefront Jan 10, 2025
ef37b85
Fix issue logging in and additional strict typing error
alec-livefront Jan 10, 2025
2eb175c
Merge branch 'main' into auth/pm-8221/route-user-to-email-otp-entry-cโ€ฆ
alec-livefront Jan 10, 2025
d4b6682
clean up code check
alec-livefront Jan 10, 2025
b103f99
Fixing tests
alec-livefront Jan 10, 2025
d90b2bd
Merge branch 'main' into auth/pm-8221/route-user-to-email-otp-entry-cโ€ฆ
alec-livefront Jan 10, 2025
76588e0
Fix strict typing errors
alec-livefront Jan 11, 2025
25d3c6e
Merge branch 'auth/pm-8221/route-user-to-email-otp-entry-clients' of โ€ฆ
alec-livefront Jan 11, 2025
c9a00c6
match resend margin from Figma
alec-livefront Jan 13, 2025
d1b0175
Use ExtensionAnonLayoutWrapperComponent
alec-livefront Jan 14, 2025
ce10ca3
Merge branch 'main' into auth/pm-8221/route-user-to-email-otp-entry-cโ€ฆ
alec-livefront Jan 14, 2025
9a8c143
Add toast message on session timeout
alec-livefront Jan 14, 2025
ab91188
Add session timeout toast
alec-livefront Jan 14, 2025
491f29a
Revert "Add toast message on session timeout"
alec-livefront Jan 14, 2025
6bd2558
Do full sync and clear email values
alec-livefront Jan 14, 2025
96a4f03
Update code error handling
alec-livefront Jan 14, 2025
100103d
Add test for logInNewDeviceVerification
alec-livefront Jan 14, 2025
052d9d8
Merge branch 'main' into auth/pm-8221/route-user-to-email-otp-entry-cโ€ฆ
alec-livefront Jan 14, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions apps/browser/src/_locales/en/messages.json
Original file line number Diff line number Diff line change
Expand Up @@ -647,6 +647,12 @@
"verifyIdentity": {
"message": "Verify identity"
},
"weDontRecognizeThisDevice": {
"message": "We don't recognize this device. Enter the code sent to your email to verify your identity."
},
"continueLoggingIn": {
"message": "Continue logging in"
},
"yourVaultIsLocked": {
"message": "Your vault is locked. Verify your identity to continue."
},
Expand Down
19 changes: 19 additions & 0 deletions apps/browser/src/popup/app-routing.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { unauthUiRefreshSwap } from "@bitwarden/angular/auth/functions/unauth-ui
import {
authGuard,
lockGuard,
newDeviceVerificationGuard,
redirectGuard,
tdeDecryptionRequiredGuard,
unauthGuardFn,
Expand Down Expand Up @@ -41,6 +42,8 @@ import {
DevicesIcon,
SsoComponent,
TwoFactorTimeoutIcon,
NewDeviceVerificationComponent,
DeviceVerificationIcon,
} from "@bitwarden/auth/angular";
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
import { LockComponent } from "@bitwarden/key-management/angular";
Expand Down Expand Up @@ -236,6 +239,22 @@ const routes: Routes = [
],
},
),
{
path: "device-verification",
component: AnonLayoutWrapperComponent,
Copy link
Contributor

Choose a reason for hiding this comment

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

โš ๏ธ : this should use the ExtensionAnonLayoutWrapperComponent with ExtensionAnonLayoutWrapperData. We should have a back button for the new page per Figma:
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

canActivate: [newDeviceVerificationGuard()],
children: [{ path: "", component: NewDeviceVerificationComponent }],
data: {
pageIcon: DeviceVerificationIcon,
pageTitle: {
key: "verifyIdentity",
},
pageSubtitle: {
key: "weDontRecognizeThisDevice",
},
elevation: 1,
} satisfies RouteDataProperties & AnonLayoutWrapperData,
},
{
path: "set-password",
component: SetPasswordComponent,
Expand Down
18 changes: 18 additions & 0 deletions apps/desktop/src/app/app-routing.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { unauthUiRefreshSwap } from "@bitwarden/angular/auth/functions/unauth-ui
import {
authGuard,
lockGuard,
newDeviceVerificationGuard,
redirectGuard,
tdeDecryptionRequiredGuard,
unauthGuardFn,
Expand Down Expand Up @@ -38,6 +39,8 @@ import {
DevicesIcon,
SsoComponent,
TwoFactorTimeoutIcon,
NewDeviceVerificationComponent,
DeviceVerificationIcon,
} from "@bitwarden/auth/angular";
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
import { LockComponent } from "@bitwarden/key-management/angular";
Expand Down Expand Up @@ -113,6 +116,21 @@ const routes: Routes = [
},
} satisfies RouteDataProperties & AnonLayoutWrapperData,
},
{
path: "device-verification",
component: AnonLayoutWrapperComponent,
canActivate: [newDeviceVerificationGuard()],
children: [{ path: "", component: NewDeviceVerificationComponent }],
data: {
pageIcon: DeviceVerificationIcon,
pageTitle: {
key: "verifyIdentity",
},
pageSubtitle: {
key: "weDontRecognizeThisDevice",
},
} satisfies RouteDataProperties & AnonLayoutWrapperData,
},
{ path: "register", component: RegisterComponent },
{
path: "new-device-notice",
Expand Down
9 changes: 9 additions & 0 deletions apps/desktop/src/locales/en/messages.json
Original file line number Diff line number Diff line change
Expand Up @@ -885,6 +885,15 @@
"message": "Verify with Duo Security for your organization using the Duo Mobile app, SMS, phone call, or U2F security key.",
"description": "'Duo Security' and 'Duo Mobile' are product names and should not be translated."
},
"verifyIdentity": {
"message": "Verify your Identity"
},
"weDontRecognizeThisDevice": {
"message": "We don't recognize this device. Enter the code sent to your email to verify your identity."
},
"continueLoggingIn": {
"message": "Continue logging in"
},
"webAuthnTitle": {
"message": "FIDO2 WebAuthn"
},
Expand Down
22 changes: 22 additions & 0 deletions apps/web/src/app/oss-routing.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
redirectGuard,
tdeDecryptionRequiredGuard,
unauthGuardFn,
newDeviceVerificationGuard,
} from "@bitwarden/angular/auth/guards";
import { canAccessFeature } from "@bitwarden/angular/platform/guard/feature-flag.guard";
import { generatorSwap } from "@bitwarden/angular/tools/generator/generator-swap";
Expand Down Expand Up @@ -37,6 +38,8 @@ import {
SsoComponent,
VaultIcon,
LoginDecryptionOptionsComponent,
NewDeviceVerificationComponent,
DeviceVerificationIcon,
} from "@bitwarden/auth/angular";
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
import { LockComponent } from "@bitwarden/key-management/angular";
Expand Down Expand Up @@ -586,6 +589,25 @@ const routes: Routes = [
titleId: "recoverAccountTwoStep",
} satisfies RouteDataProperties & AnonLayoutWrapperData,
},
{
path: "device-verification",
canActivate: [newDeviceVerificationGuard()],
children: [
{
path: "",
component: NewDeviceVerificationComponent,
},
],
data: {
pageIcon: DeviceVerificationIcon,
pageTitle: {
key: "verifyIdentity",
},
pageSubtitle: {
key: "weDontRecognizeThisDevice",
},
} satisfies RouteDataProperties & AnonLayoutWrapperData,
},
{
path: "accept-emergency",
canActivate: [deepLinkGuard()],
Expand Down
6 changes: 6 additions & 0 deletions apps/web/src/locales/en/messages.json
Original file line number Diff line number Diff line change
Expand Up @@ -1128,6 +1128,12 @@
"verifyIdentity": {
"message": "Verify your Identity"
},
"weDontRecognizeThisDevice": {
"message": "We don't recognize this device. Enter the code sent to your email to verify your identity."
},
"continueLoggingIn": {
"message": "Continue logging in"
},
"whatIsADevice": {
"message": "What is a device?"
},
Expand Down
1 change: 1 addition & 0 deletions libs/angular/src/auth/guards/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@
export * from "./redirect.guard";
export * from "./tde-decryption-required.guard";
export * from "./unauth.guard";
export * from "./new-device-verification.guard";

Check warning on line 6 in libs/angular/src/auth/guards/index.ts

View check run for this annotation

Codecov / codecov/patch

libs/angular/src/auth/guards/index.ts#L6

Added line #L6 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice test suite!

Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import { TestBed } from "@angular/core/testing";
import { Router } from "@angular/router";
import { RouterTestingModule } from "@angular/router/testing";
import { MockProxy, mock } from "jest-mock-extended";
import { BehaviorSubject } from "rxjs";

import { EmptyComponent } from "@bitwarden/angular/platform/guard/feature-flag.guard.spec";
import { LoginStrategyServiceAbstraction } from "@bitwarden/auth/common";
import { AuthenticationType } from "@bitwarden/common/auth/enums/authentication-type";
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";

import { newDeviceVerificationGuard } from "./new-device-verification.guard";

describe("NewDeviceVerificationGuard", () => {
const setup = (authType: AuthenticationType | null) => {
const loginStrategyService: MockProxy<LoginStrategyServiceAbstraction> =
mock<LoginStrategyServiceAbstraction>();
const currentAuthTypeSubject = new BehaviorSubject<AuthenticationType | null>(authType);
loginStrategyService.currentAuthType$ = currentAuthTypeSubject;

const logService: MockProxy<LogService> = mock<LogService>();

const testBed = TestBed.configureTestingModule({
imports: [
RouterTestingModule.withRoutes([
{ path: "", component: EmptyComponent },
{
path: "device-verification",
component: EmptyComponent,
canActivate: [newDeviceVerificationGuard()],
},
{ path: "login", component: EmptyComponent },
]),
],
providers: [
{ provide: LoginStrategyServiceAbstraction, useValue: loginStrategyService },
{ provide: LogService, useValue: logService },
],
});

return {
router: testBed.inject(Router),
logService,
};
};

it("creates the guard", () => {
const { router } = setup(AuthenticationType.Password);
expect(router).toBeTruthy();
});

it("allows access with an active login session", async () => {
const { router } = setup(AuthenticationType.Password);

await router.navigate(["device-verification"]);
expect(router.url).toBe("/device-verification");
});

it("redirects to login with no active session", async () => {
const { router, logService } = setup(null);

await router.navigate(["device-verification"]);
expect(router.url).toBe("/login");
expect(logService.error).toHaveBeenCalledWith("No active login session found.");
});
});
28 changes: 28 additions & 0 deletions libs/angular/src/auth/guards/new-device-verification.guard.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { inject } from "@angular/core";
import { CanActivateFn, Router } from "@angular/router";
import { firstValueFrom } from "rxjs";

import { LoginStrategyServiceAbstraction } from "@bitwarden/auth/common";
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";

/**
* Guard that ensures there is an active login session before allowing access
* to the new device verification route.
* If not, redirects to login.
*/
export function newDeviceVerificationGuard(): CanActivateFn {
return async () => {
const loginStrategyService = inject(LoginStrategyServiceAbstraction);
const logService = inject(LogService);
const router = inject(Router);

// Check if we have a valid login session
const authType = await firstValueFrom(loginStrategyService.currentAuthType$);
if (authType === null) {
logService.error("No active login session found.");
return router.createUrlTree(["/login"]);
}

return true;
};
}
33 changes: 33 additions & 0 deletions libs/angular/src/services/jslib-services.module.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

โ“ : is this still necessary after we updated the NewDeviceVerificationComponent to use the LoginStrategyService instead of the PasswordLoginStrategy directly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do, since we're subscribing to passwordLoginStrategy.sessionTimeout$ to detect a timeout (in new-device-verification.component.ts).

Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ import {
DefaultAuthRequestApiService,
DefaultLoginSuccessHandlerService,
LoginSuccessHandlerService,
PasswordLoginStrategy,
PasswordLoginStrategyData,
} from "@bitwarden/auth/common";
import { ApiService as ApiServiceAbstraction } from "@bitwarden/common/abstractions/api.service";
import { AuditService as AuditServiceAbstraction } from "@bitwarden/common/abstractions/audit.service";
Expand Down Expand Up @@ -1434,6 +1436,37 @@ const safeProviders: SafeProvider[] = [
useClass: DefaultLoginSuccessHandlerService,
deps: [SyncService, UserAsymmetricKeysRegenerationService],
}),
safeProvider({
provide: PasswordLoginStrategy,
useClass: PasswordLoginStrategy,
deps: [
PasswordLoginStrategyData,
PasswordStrengthServiceAbstraction,
PolicyServiceAbstraction,
LoginStrategyServiceAbstraction,
AccountServiceAbstraction,
InternalMasterPasswordServiceAbstraction,
KeyService,
EncryptService,
ApiServiceAbstraction,
TokenServiceAbstraction,
AppIdServiceAbstraction,
PlatformUtilsServiceAbstraction,
MessagingServiceAbstraction,
LogService,
StateServiceAbstraction,
TwoFactorServiceAbstraction,
InternalUserDecryptionOptionsServiceAbstraction,
BillingAccountProfileStateService,
VaultTimeoutSettingsServiceAbstraction,
KdfConfigService,
],
}),
safeProvider({
provide: PasswordLoginStrategyData,
useClass: PasswordLoginStrategyData,
deps: [],
}),
];

@NgModule({
Expand Down
18 changes: 18 additions & 0 deletions libs/auth/src/angular/icons/device-verification.icon.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { svgIcon } from "@bitwarden/components";

Check warning on line 1 in libs/auth/src/angular/icons/device-verification.icon.ts

View check run for this annotation

Codecov / codecov/patch

libs/auth/src/angular/icons/device-verification.icon.ts#L1

Added line #L1 was not covered by tests

export const DeviceVerificationIcon = svgIcon`

Check warning on line 3 in libs/auth/src/angular/icons/device-verification.icon.ts

View check run for this annotation

Codecov / codecov/patch

libs/auth/src/angular/icons/device-verification.icon.ts#L3

Added line #L3 was not covered by tests
<svg viewBox="0 0 98 95" fill="none" xmlns="http://www.w3.org/2000/svg">
<path class="tw-stroke-art-primary" d="M12.1759 27.7453L2.54349 34.9329C1.57215 35.6577 1 36.7986 1 38.0105V89.6281C1 91.7489 2.71922 93.4681 4.84 93.4681H93.16C95.2808 93.4681 97 91.7489 97 89.6281V38.0276C97 36.806 96.4188 35.6574 95.4347 34.9338L85.6576 27.7453M61.8791 10.2622L50.9367 2.2168C49.5753 1.21588 47.7197 1.22245 46.3655 2.23297L35.6054 10.2622" stroke-width="1.92"/>
<path class="tw-stroke-art-primary" d="M85.7661 45.4682V12.1542C85.7661 11.0938 84.9064 10.2342 83.8461 10.2342H14.1541C13.0937 10.2342 12.2341 11.0938 12.2341 12.1542V45.4682" stroke-width="1.92" stroke-linecap="round"/>
<path class="tw-stroke-art-primary" d="M95.7335 92.1003L62.3151 61.2912C61.2514 60.3106 59.8576 59.7661 58.4109 59.7661H38.043C36.5571 59.7661 35.1286 60.3404 34.0562 61.3689L2.01148 92.1003" stroke-width="1.92"/>
<line class="tw-stroke-art-primary" x1="96.157" y1="39.125" x2="61.0395" y2="60.0979" stroke-width="1.92" stroke-linecap="round"/>
<path class="tw-stroke-art-primary" d="M1.84229 39.1248L36.673 59.7488" stroke-width="1.92" stroke-linecap="round"/>
<rect class="tw-stroke-art-accent" x="23.0046" y="25.5344" width="51.925" height="17.4487" rx="8.72434" stroke-width="0.96"/>
<circle class="tw-fill-art-accent" cx="30.2299" cy="34.2588" r="2.24846"/>
<circle class="tw-fill-art-accent" cx="45.2196" cy="34.2587" r="2.24846"/>
<circle class="tw-fill-art-accent" cx="60.2094" cy="34.2587" r="2.24846"/>
<circle class="tw-fill-art-accent" cx="37.7248" cy="34.2587" r="2.24846"/>
<circle class="tw-fill-art-accent" cx="52.7145" cy="34.2587" r="2.24846"/>
<circle class="tw-fill-art-accent" cx="67.704" cy="34.2587" r="2.24846"/>
</svg>
`;
1 change: 1 addition & 0 deletions libs/auth/src/angular/icons/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,4 @@
export * from "./registration-expired-link.icon";
export * from "./sso-key.icon";
export * from "./two-factor-timeout.icon";
export * from "./device-verification.icon";

Check warning on line 15 in libs/auth/src/angular/icons/index.ts

View check run for this annotation

Codecov / codecov/patch

libs/auth/src/angular/icons/index.ts#L15

Added line #L15 was not covered by tests
3 changes: 3 additions & 0 deletions libs/auth/src/angular/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,3 +71,6 @@
// login approval
export * from "./login-approval/login-approval.component";
export * from "./login-approval/default-login-approval-component.service";

// device verification
export * from "./new-device-verification/new-device-verification.component";

Check warning on line 76 in libs/auth/src/angular/index.ts

View check run for this annotation

Codecov / codecov/patch

libs/auth/src/angular/index.ts#L76

Added line #L76 was not covered by tests
6 changes: 6 additions & 0 deletions libs/auth/src/angular/login/login.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,12 @@
return;
}

// Redirect to device verification if this is an unknown device
if (authResult.requiresDeviceVerification) {
await this.router.navigate(["device-verification"]);
return;

Check warning on line 289 in libs/auth/src/angular/login/login.component.ts

View check run for this annotation

Codecov / codecov/patch

libs/auth/src/angular/login/login.component.ts#L288-L289

Added lines #L288 - L289 were not covered by tests
}

// If none of the above cases are true, proceed with login...
await this.evaluatePassword();

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<form [formGroup]="formGroup" [bitSubmit]="submit">
<bit-form-field>
<bit-label>{{ "verificationCode" | i18n }}</bit-label>
<input
bitInput
type="text"
id="verificationCode"
name="verificationCode"
formControlName="code"
appInputVerbatim
/>
</bit-form-field>

<button
bitLink
type="button"
linkType="primary"
(click)="resendOTP()"
[disabled]="disableRequestOTP"
>
{{ "resendCode" | i18n }}
</button>

<div class="tw-flex tw-mt-4">
<button
bitButton
buttonType="primary"
type="submit"
[block]="true"
[disabled]="formGroup.invalid"
>
{{ "continueLoggingIn" | i18n }}
</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

๐Ÿ’ก , 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

</div>
</form>
Loading
Loading