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-3169] Login decryption options in extension popup #5909

Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
5493a58
[PM-3169] refactor: lock guard and add new redirect guard
coroiu Jul 27, 2023
f7a8dc5
[PM-3169] feat: implement fully rewritten routing
coroiu Jul 27, 2023
cf13946
[PM-3169] feat: close SSO window
coroiu Jul 27, 2023
63a9887
[PM-3169] feat: store sso org identifier in state
coroiu Jul 28, 2023
0254c38
Merge branch 'feature/trusted-device-encryption' into PM-3169-login-d…
coroiu Jul 28, 2023
10f5ead
[PM-3169] fix: tests
coroiu Jul 28, 2023
e502ce2
[PM-3169] feat: get rid of unconventional patch method
coroiu Jul 28, 2023
eab5a4d
Merge branch 'feature/trusted-device-encryption' into PM-3169-login-d…
JaredSnider-Bitwarden Aug 1, 2023
e8684fc
Merge branch 'feature/trusted-device-encryption' into PM-3169-login-d…
JaredSnider-Bitwarden Aug 2, 2023
b6f791c
PM-3169 - SSO & 2FA Comps - Update naming of new callback to match ex…
JaredSnider-Bitwarden Aug 2, 2023
f86f44d
Merge branch 'feature/trusted-device-encryption' into PM-3169-login-d…
JaredSnider-Bitwarden Aug 2, 2023
268c8fa
PM-3169 - Update LockGuard to have a special exception for allowing t…
JaredSnider-Bitwarden Aug 2, 2023
32f0201
PM-3169 - Per discussion w/ Jake and Justin, rename login-initiated g…
JaredSnider-Bitwarden Aug 2, 2023
62a0b5e
PM-3169 - Add some additional context to new redirect guard scenario
JaredSnider-Bitwarden Aug 2, 2023
22e1980
PM-3169 - Per PR feedback, replace all callback types with Promise<vo…
JaredSnider-Bitwarden Aug 4, 2023
0c1461e
PM-3169 - StateSvc - Per PR feedback, update setUserSsoOrganizationId…
JaredSnider-Bitwarden Aug 4, 2023
8758ddb
PM-3169 - Replace onSuccessfulLogin type to compile
JaredSnider-Bitwarden Aug 4, 2023
f42f04c
PM-3169 - Add clarification comment for why we are not using a query …
JaredSnider-Bitwarden Aug 4, 2023
237b3f3
PM-3169 - Per discussion with Justin, only use memory for SsoOrgId as…
JaredSnider-Bitwarden Aug 4, 2023
7934371
PM-3169 - Add missing ssoIdentifierRequired translation to desktop an…
JaredSnider-Bitwarden Aug 4, 2023
deb75ea
PM-3169 - After discussing with Justin again, we realized that memory…
JaredSnider-Bitwarden Aug 4, 2023
e754ba6
PM-3169 - Per PR feedback, remove hasEverHadUserKey logic as we can j…
JaredSnider-Bitwarden Aug 4, 2023
82f7b98
PM-3169 - Per design discussion with Danielle, move account created t…
JaredSnider-Bitwarden Aug 4, 2023
7cf92a2
Merge remote-tracking branch 'origin/feature/trusted-device-encryptio…
JaredSnider-Bitwarden Aug 4, 2023
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
3 changes: 3 additions & 0 deletions apps/browser/src/_locales/en/messages.json
Original file line number Diff line number Diff line change
Expand Up @@ -2251,6 +2251,9 @@
"approveWithMasterPassword": {
"message": "Approve with master password"
},
"ssoIdentifierRequired": {
"message": "Organization SSO identifier is required."
},
"eu": {
"message": "EU",
"description": "European Union"
Expand Down
1 change: 0 additions & 1 deletion apps/browser/src/auth/popup/services/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
export { LockGuardService } from "./lock-guard.service";
export { UnauthGuardService } from "./unauth-guard.service";
8 changes: 0 additions & 8 deletions apps/browser/src/auth/popup/services/lock-guard.service.ts

This file was deleted.

13 changes: 8 additions & 5 deletions apps/browser/src/auth/popup/sso.component.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { Component } from "@angular/core";
import { Component, Inject } from "@angular/core";
import { ActivatedRoute, Router } from "@angular/router";

import { SsoComponent as BaseSsoComponent } from "@bitwarden/angular/auth/components/sso.component";
import { WINDOW } from "@bitwarden/angular/services/injection-tokens";
import { ApiService } from "@bitwarden/common/abstractions/api.service";
import { VaultTimeoutService } from "@bitwarden/common/abstractions/vaultTimeout/vaultTimeout.service";
import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status";
import { ConfigServiceAbstraction } from "@bitwarden/common/platform/abstractions/config/config.service.abstraction";
Expand Down Expand Up @@ -37,7 +37,7 @@ export class SsoComponent extends BaseSsoComponent {
environmentService: EnvironmentService,
logService: LogService,
configService: ConfigServiceAbstraction,
private vaultTimeoutService: VaultTimeoutService
@Inject(WINDOW) private win: Window
) {
super(
authService,
Expand Down Expand Up @@ -67,8 +67,11 @@ export class SsoComponent extends BaseSsoComponent {
BrowserApi.reloadOpenWindows();
}

const thisWindow = window.open("", "_self");
thisWindow.close();
this.win.close();
};

super.onSuccessfulLoginTdeNavigate = async () => {
this.win.close();
};
}
}
4 changes: 4 additions & 0 deletions apps/browser/src/auth/popup/two-factor.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ export class TwoFactorComponent extends BaseTwoFactorComponent {
this.loginService.clearValues();
return syncService.fullSync(true);
};

super.onSuccessfulLoginTdeNavigate = async () => {
this.win.close();
};
super.successRoute = "/tabs/vault";
// FIXME: Chromium 110 has broken WebAuthn support in extensions via an iframe
this.webAuthnNewTab = true;
Expand Down
14 changes: 10 additions & 4 deletions apps/browser/src/popup/app-routing.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@ import { Injectable, NgModule } from "@angular/core";
import { ActivatedRouteSnapshot, RouteReuseStrategy, RouterModule, Routes } from "@angular/router";

import { AuthGuard } from "@bitwarden/angular/auth/guards/auth.guard";
import { LockGuard } from "@bitwarden/angular/auth/guards/lock.guard";
import { lockGuard } from "@bitwarden/angular/auth/guards/lock.guard";
import { tdeDecryptionRequiredGuard } from "@bitwarden/angular/auth/guards/tde-decryption-required.guard";
import { UnauthGuard } from "@bitwarden/angular/auth/guards/unauth.guard";
import { canAccessFeature } from "@bitwarden/angular/guard/feature-flag.guard";
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";

import { redirectGuard } from "../../../../libs/angular/src/auth/guards/redirect.guard";
import { EnvironmentComponent } from "../auth/popup/environment.component";
import { HintComponent } from "../auth/popup/hint.component";
import { HomeComponent } from "../auth/popup/home.component";
Expand Down Expand Up @@ -52,8 +54,9 @@ import { TabsComponent } from "./tabs.component";
const routes: Routes = [
{
path: "",
redirectTo: "home",
pathMatch: "full",
children: [], // Children lets us have an empty component.
canActivate: [redirectGuard({ loggedIn: "/tabs/vault", loggedOut: "/home", locked: "/lock" })],
},
{
path: "vault",
Expand Down Expand Up @@ -87,7 +90,7 @@ const routes: Routes = [
{
path: "lock",
component: LockComponent,
canActivate: [LockGuard],
canActivate: [lockGuard()],
data: { state: "lock" },
},
{
Expand All @@ -105,7 +108,10 @@ const routes: Routes = [
{
path: "login-initiated",
component: LoginDecryptionOptionsComponent,
canActivate: [LockGuard, canAccessFeature(FeatureFlag.TrustedDeviceEncryption)],
canActivate: [
tdeDecryptionRequiredGuard(),
canAccessFeature(FeatureFlag.TrustedDeviceEncryption),
],
},
{
path: "sso",
Expand Down
4 changes: 1 addition & 3 deletions apps/browser/src/popup/services/services.module.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { APP_INITIALIZER, LOCALE_ID, NgModule } from "@angular/core";

import { LockGuard as BaseLockGuardService } from "@bitwarden/angular/auth/guards/lock.guard";
import { UnauthGuard as BaseUnauthGuardService } from "@bitwarden/angular/auth/guards/unauth.guard";
import { DialogServiceAbstraction } from "@bitwarden/angular/services/dialog";
import { MEMORY_STORAGE, SECURE_STORAGE } from "@bitwarden/angular/services/injection-tokens";
Expand Down Expand Up @@ -84,7 +83,7 @@ import { VaultExportServiceAbstraction } from "@bitwarden/exporter/vault-export"

import { BrowserOrganizationService } from "../../admin-console/services/browser-organization.service";
import { BrowserPolicyService } from "../../admin-console/services/browser-policy.service";
import { LockGuardService, UnauthGuardService } from "../../auth/popup/services";
import { UnauthGuardService } from "../../auth/popup/services";
import { AutofillService } from "../../autofill/services/abstractions/autofill.service";
import MainBackground from "../../background/main.background";
import { Account } from "../../models/account";
Expand Down Expand Up @@ -144,7 +143,6 @@ function getBgService<T>(service: keyof MainBackground) {
deps: [InitService],
multi: true,
},
{ provide: BaseLockGuardService, useClass: LockGuardService },
{ provide: BaseUnauthGuardService, useClass: UnauthGuardService },
{ provide: PopupUtilsService, useFactory: () => new PopupUtilsService(isPrivateMode) },
{
Expand Down
18 changes: 14 additions & 4 deletions apps/desktop/src/app/app-routing.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ import { NgModule } from "@angular/core";
import { RouterModule, Routes } from "@angular/router";

import { AuthGuard } from "@bitwarden/angular/auth/guards/auth.guard";
import { LockGuard } from "@bitwarden/angular/auth/guards/lock.guard";
import { lockGuard } from "@bitwarden/angular/auth/guards/lock.guard";
import { redirectGuard } from "@bitwarden/angular/auth/guards/redirect.guard";
import { tdeDecryptionRequiredGuard } from "@bitwarden/angular/auth/guards/tde-decryption-required.guard";
import { canAccessFeature } from "@bitwarden/angular/guard/feature-flag.guard";
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";

Expand All @@ -24,11 +26,16 @@ import { VaultComponent } from "../vault/app/vault/vault.component";
import { SendComponent } from "./tools/send/send.component";

const routes: Routes = [
{ path: "", redirectTo: "/vault", pathMatch: "full" },
{
path: "",
pathMatch: "full",
children: [], // Children lets us have an empty component.
canActivate: [redirectGuard({ loggedIn: "/vault", loggedOut: "/login", locked: "/lock" })],
},
{
path: "lock",
component: LockComponent,
canActivate: [LockGuard],
canActivate: [lockGuard()],
},
{
path: "login",
Expand All @@ -47,7 +54,10 @@ const routes: Routes = [
{
path: "login-initiated",
component: LoginDecryptionOptionsComponent,
canActivate: [LockGuard, canAccessFeature(FeatureFlag.TrustedDeviceEncryption)],
canActivate: [
tdeDecryptionRequiredGuard(),
canAccessFeature(FeatureFlag.TrustedDeviceEncryption),
],
},
{ path: "register", component: RegisterComponent },
{
Expand Down
3 changes: 3 additions & 0 deletions apps/desktop/src/locales/en/messages.json
Original file line number Diff line number Diff line change
Expand Up @@ -2273,6 +2273,9 @@
"region": {
"message": "Region"
},
"ssoIdentifierRequired": {
"message": "Organization SSO identifier is required."
},
"eu": {
"message": "EU",
"description": "European Union"
Expand Down
24 changes: 0 additions & 24 deletions apps/web/src/app/guards/home.guard.ts

This file was deleted.

14 changes: 9 additions & 5 deletions apps/web/src/app/oss-routing.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ import { NgModule } from "@angular/core";
import { Route, RouterModule, Routes } from "@angular/router";

import { AuthGuard } from "@bitwarden/angular/auth/guards/auth.guard";
import { LockGuard } from "@bitwarden/angular/auth/guards/lock.guard";
import { lockGuard } from "@bitwarden/angular/auth/guards/lock.guard";
import { redirectGuard } from "@bitwarden/angular/auth/guards/redirect.guard";
import { tdeDecryptionRequiredGuard } from "@bitwarden/angular/auth/guards/tde-decryption-required.guard";
import { UnauthGuard } from "@bitwarden/angular/auth/guards/unauth.guard";
import { canAccessFeature } from "@bitwarden/angular/guard/feature-flag.guard";
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
Expand Down Expand Up @@ -34,7 +36,6 @@ import { UpdatePasswordComponent } from "./auth/update-password.component";
import { UpdateTempPasswordComponent } from "./auth/update-temp-password.component";
import { VerifyEmailTokenComponent } from "./auth/verify-email-token.component";
import { VerifyRecoverDeleteComponent } from "./auth/verify-recover-delete.component";
import { HomeGuard } from "./guards/home.guard";
import { FrontendLayoutComponent } from "./layouts/frontend-layout.component";
import { UserLayoutComponent } from "./layouts/user-layout.component";
import { ReportsModule } from "./reports";
Expand All @@ -59,7 +60,7 @@ const routes: Routes = [
path: "",
pathMatch: "full",
children: [], // Children lets us have an empty component.
canActivate: [HomeGuard], // Redirects either to vault, login or lock page.
canActivate: [redirectGuard({ loggedIn: "/vault", loggedOut: "/login", locked: "/lock" })],
},
{ path: "login", component: LoginComponent, canActivate: [UnauthGuard] },
{
Expand All @@ -76,7 +77,10 @@ const routes: Routes = [
{
path: "login-initiated",
component: LoginDecryptionOptionsComponent,
canActivate: [LockGuard, canAccessFeature(FeatureFlag.TrustedDeviceEncryption)],
canActivate: [
tdeDecryptionRequiredGuard(),
canAccessFeature(FeatureFlag.TrustedDeviceEncryption),
],
},
{
path: "register",
Expand Down Expand Up @@ -109,7 +113,7 @@ const routes: Routes = [
{
path: "lock",
component: LockComponent,
canActivate: [LockGuard],
canActivate: [lockGuard()],
},
{ path: "verify-email", component: VerifyEmailTokenComponent },
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@ import { FormBuilder, FormControl } from "@angular/forms";
import { ActivatedRoute, Router } from "@angular/router";
import {
firstValueFrom,
map,
switchMap,
Subject,
catchError,
from,
of,
finalize,
takeUntil,
defer,
throwError,
} from "rxjs";

import { ApiService } from "@bitwarden/common/abstractions/api.service";
Expand Down Expand Up @@ -106,11 +107,9 @@ export class BaseLoginDecryptionOptionsComponent implements OnInit, OnDestroy {
// We are dealing with a new account if:
// - User does not have admin approval (i.e. has not enrolled into admin reset)
// - AND does not have a master password
this.platformUtilsService.showToast(
"success",
null,
this.i18nService.t("accountSuccessfullyCreated")
);

// TODO: discuss how this doesn't make any sense to show here
JaredSnider-Bitwarden marked this conversation as resolved.
Show resolved Hide resolved

this.loadNewUserData();
} else {
this.loadUntrustedDeviceData(accountDecryptionOptions);
Expand Down Expand Up @@ -140,14 +139,19 @@ export class BaseLoginDecryptionOptionsComponent implements OnInit, OnDestroy {
}

async loadNewUserData() {
const autoEnrollStatus$ = this.activatedRoute.queryParamMap.pipe(
map((params) => params.get("identifier")),
switchMap((identifier) => {
if (identifier == null) {
return of(null);
const autoEnrollStatus$ = defer(() =>
this.stateService.getUserSsoOrganizationIdentifier()
).pipe(
switchMap((organizationIdentifier) => {
if (organizationIdentifier == undefined) {
return throwError(() => new Error(this.i18nService.t("ssoIdentifierRequired")));
}

return from(this.organizationApiService.getAutoEnrollStatus(identifier));
return from(this.organizationApiService.getAutoEnrollStatus(organizationIdentifier));
}),
catchError((err: unknown) => {
this.validationService.showError(err);
return of(undefined);
})
JaredSnider-Bitwarden marked this conversation as resolved.
Show resolved Hide resolved
);

Expand Down Expand Up @@ -225,7 +229,7 @@ export class BaseLoginDecryptionOptionsComponent implements OnInit, OnDestroy {

async approveWithMasterPassword() {
await this.deviceTrustCryptoService.setShouldTrustDevice(this.rememberDevice.value);
this.router.navigate(["/lock"]);
this.router.navigate(["/lock"], { queryParams: { from: "login-initiated" } });
}

async createUser() {
Expand All @@ -240,6 +244,12 @@ export class BaseLoginDecryptionOptionsComponent implements OnInit, OnDestroy {
const keysRequest = new KeysRequest(publicKey, privateKey.encryptedString);
await this.apiService.postAccountKeys(keysRequest);

this.platformUtilsService.showToast(
"success",
null,
this.i18nService.t("accountSuccessfullyCreated")
);

await this.passwordResetEnrollmentService.enroll(this.data.organizationId);

if (this.rememberDeviceForm.value.rememberDevice) {
Expand Down
27 changes: 21 additions & 6 deletions libs/angular/src/auth/components/sso.component.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Component } from "@angular/core";

Check notice on line 1 in libs/angular/src/auth/components/sso.component.spec.ts

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (feature/trusted-device-encryption)

ℹ Getting worse: Code Duplication

introduced similar code in: "calls onSuccessfulLoginTdeNavigate instead of router.navigate when the callback is defined". Avoid duplicated, aka copy-pasted, code inside the module. More duplication lowers the code health.

Check notice on line 1 in libs/angular/src/auth/components/sso.component.spec.ts

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (feature/trusted-device-encryption)

✅ Getting better: Code Duplication

reduced similar code in: "calls authService.logIn and navigates to the component's defined 2FA route when the auth result requires 2FA and onSuccessfulLoginTwoFactorNavigate is not defined","navigates to the component's defined trusted device encryption route when login is successful". Avoid duplicated, aka copy-pasted, code inside the module. More duplication lowers the code health.
import { ComponentFixture, TestBed } from "@angular/core/testing";
import { ActivatedRoute, Router } from "@angular/router";
import { MockProxy, mock } from "jest-mock-extended";
Expand Down Expand Up @@ -73,6 +73,7 @@
let mockOnSuccessfulLoginTwoFactorNavigate: jest.Mock;
let mockOnSuccessfulLoginChangePasswordNavigate: jest.Mock;
let mockOnSuccessfulLoginForceResetNavigate: jest.Mock;
let mockOnSuccessfulLoginTdeNavigate: jest.Mock;

let mockAcctDecryptionOpts: {
noMasterPassword: AccountDecryptionOptions;
Expand Down Expand Up @@ -118,6 +119,7 @@
mockOnSuccessfulLoginTwoFactorNavigate = jest.fn();
mockOnSuccessfulLoginChangePasswordNavigate = jest.fn();
mockOnSuccessfulLoginForceResetNavigate = jest.fn();
mockOnSuccessfulLoginTdeNavigate = jest.fn();

Check warning on line 122 in libs/angular/src/auth/components/sso.component.spec.ts

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (feature/trusted-device-encryption)

❌ Getting worse: Large Method

beforeEach increases from 87 to 88 lines of code, threshold = 70. Large functions with many lines of code are generally harder to understand and lower the code health. Avoid adding more lines to this function.

mockAcctDecryptionOpts = {
noMasterPassword: new AccountDecryptionOptions({
Expand Down Expand Up @@ -381,16 +383,29 @@
mockAuthService.logIn.mockResolvedValue(authResult);
});

it("navigates to the component's defined trusted device encryption route when login is successful", async () => {
it("navigates to the component's defined trusted device encryption route when login is successful and no callback is defined", async () => {
await _component.logIn(code, codeVerifier, orgIdFromState);

expect(mockAuthService.logIn).toHaveBeenCalledTimes(1);
expect(mockRouter.navigate).toHaveBeenCalledTimes(1);
expect(mockRouter.navigate).toHaveBeenCalledWith([_component.trustedDeviceEncRoute], {
queryParams: {
identifier: orgIdFromState,
},
});
expect(mockRouter.navigate).toHaveBeenCalledWith(
[_component.trustedDeviceEncRoute],
undefined
);
expect(mockLogService.error).not.toHaveBeenCalled();
});

it("calls onSuccessfulLoginTdeNavigate instead of router.navigate when the callback is defined", async () => {
mockOnSuccessfulLoginTdeNavigate = jest.fn().mockResolvedValue(null);
component.onSuccessfulLoginTdeNavigate = mockOnSuccessfulLoginTdeNavigate;

await _component.logIn(code, codeVerifier, orgIdFromState);

expect(mockAuthService.logIn).toHaveBeenCalledTimes(1);

expect(mockOnSuccessfulLoginTdeNavigate).toHaveBeenCalledTimes(1);

expect(mockRouter.navigate).not.toHaveBeenCalled();
expect(mockLogService.error).not.toHaveBeenCalled();
});
});
Expand Down
Loading