1
0
mirror of https://github.com/bitwarden/browser.git synced 2025-01-02 18:17:46 +01:00

PM-1391-Added previous-url to global-state (#5733)

* added previous-url to global-state

* updated storage of previousUrl for SSO/MFA flows

* revert file changes

* added post login routing

* Clear PreviousUrl from storage on new Login

* Components do not call StateService anymore

* removed needed query params

* refactored components to use RouterService

* fixed build error

* fixed mfa component

* updated logic for previous Url

* removed unneeded base implementation

* Added state call for Redirect Guard

* Fixed test cases

* Remove routing service calls

* renamed global field, changed routing to guard

* reverting constructor changes and git lint issue

* fixing constructor ordering

* fixing diffs to be clearer on actual cahnges.

* addressing accepting emergency access case

* refactor and add locked state logic

* refactor name of guard to be more clear

* Added comments and tests

* comments + support lock page deep linking + code ownership

* readability updates

* Combined guards and specs updated routing

* Update oss-routing.module.ts

* fixed stroybook build
This commit is contained in:
Ike 2023-11-22 08:54:12 -08:00 committed by GitHub
parent a6e3d4d244
commit f1691a5ef1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
14 changed files with 321 additions and 90 deletions

View File

@ -16,13 +16,14 @@ import { OrganizationPermissionsGuard } from "../../admin-console/organizations/
import { OrganizationRedirectGuard } from "../../admin-console/organizations/guards/org-redirect.guard";
import { OrganizationLayoutComponent } from "../../admin-console/organizations/layouts/organization-layout.component";
import { GroupsComponent } from "../../admin-console/organizations/manage/groups.component";
import { deepLinkGuard } from "../../auth/guards/deep-link.guard";
import { VaultModule } from "../../vault/org-vault/vault.module";
const routes: Routes = [
{
path: ":organizationId",
component: OrganizationLayoutComponent,
canActivate: [AuthGuard, OrganizationPermissionsGuard],
canActivate: [deepLinkGuard(), AuthGuard, OrganizationPermissionsGuard],
data: {
organizationPermissions: canAccessOrgAdmin,
},

View File

@ -111,14 +111,12 @@ export class AppComponent implements OnDestroy, OnInit {
this.notificationsService.updateConnection(false);
break;
case "loggedOut":
this.routerService.setPreviousUrl(null);
this.notificationsService.updateConnection(false);
break;
case "unlocked":
this.notificationsService.updateConnection(false);
break;
case "authBlocked":
this.routerService.setPreviousUrl(message.url);
this.router.navigate(["/"]);
break;
case "logout":
@ -132,7 +130,6 @@ export class AppComponent implements OnDestroy, OnInit {
this.router.navigate(["lock"]);
break;
case "lockedUrl":
this.routerService.setPreviousUrl(message.url);
break;
case "syncStarted":
break;

View File

@ -36,7 +36,6 @@ export class AcceptEmergencyComponent extends BaseAcceptComponent {
async authedHandler(qParams: Params): Promise<void> {
this.actionPromise = this.emergencyAccessService.accept(qParams.id, qParams.token);
await this.actionPromise;
await this.stateService.setEmergencyAccessInvitation(null);
this.platformUtilService.showToast(
"success",
this.i18nService.t("inviteAccepted"),
@ -52,8 +51,5 @@ export class AcceptEmergencyComponent extends BaseAcceptComponent {
// Fix URL encoding of space issue with Angular
this.name = this.name.replace(/\+/g, " ");
}
// save the invitation to state so sso logins can find it later
await this.stateService.setEmergencyAccessInvitation(qParams);
}
}

View File

@ -0,0 +1,190 @@
import { Component } from "@angular/core";
import { TestBed } from "@angular/core/testing";
import { Router, provideRouter } from "@angular/router";
import { RouterTestingHarness } from "@angular/router/testing";
import { MockProxy, mock } from "jest-mock-extended";
import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status";
import { RouterService } from "../../core/router.service";
import { deepLinkGuard } from "./deep-link.guard";
@Component({
template: "",
})
export class GuardedRouteTestComponent {}
@Component({
template: "",
})
export class LockTestComponent {}
@Component({
template: "",
})
export class RedirectTestComponent {}
/**
* We are assuming the guard is always being called. We are creating routes using the
* RouterTestingHarness.
*
* when persisting a URL to storage we don't care wether or not the user is locked or logged out.
* We only care about where the user is going, and has been.
*
* We are testing the activatedComponent because we are testing that the guard redirects when a user is
* unlocked.
*/
describe("Deep Link Guard", () => {
let authService: MockProxy<AuthService>;
let routerService: MockProxy<RouterService>;
let routerHarness: RouterTestingHarness;
beforeEach(async () => {
authService = mock<AuthService>();
routerService = mock<RouterService>();
TestBed.configureTestingModule({
providers: [
{ provide: AuthService, useValue: authService },
{ provide: RouterService, useValue: routerService },
provideRouter([
{
path: "guarded-route",
component: GuardedRouteTestComponent,
canActivate: [deepLinkGuard()],
},
{
path: "lock-route",
component: LockTestComponent,
canActivate: [deepLinkGuard()],
},
{
path: "redirect-route",
component: RedirectTestComponent,
},
]),
],
});
routerHarness = await RouterTestingHarness.create();
});
// Story: User's vault times out
it('should persist routerService.previousUrl when routerService.previousUrl does not contain "lock"', async () => {
// Arrange
authService.getAuthStatus.mockResolvedValue(AuthenticationStatus.Locked);
routerService.getPreviousUrl.mockReturnValue("/previous-url");
// Act
await routerHarness.navigateByUrl("/lock-route");
// Assert
expect(routerService.persistLoginRedirectUrl).toHaveBeenCalledWith("/previous-url");
});
// Story: User's vault times out and previousUrl contains "lock"
it('should not persist routerService.previousUrl when routerService.previousUrl contains "lock"', async () => {
// Arrange
authService.getAuthStatus.mockResolvedValue(AuthenticationStatus.Locked);
routerService.getPreviousUrl.mockReturnValue("/lock");
// Act
await routerHarness.navigateByUrl("/lock-route");
// Assert
expect(routerService.persistLoginRedirectUrl).not.toHaveBeenCalled();
});
// Story: User's vault times out and previousUrl is undefined
it("should not persist routerService.previousUrl when routerService.previousUrl is undefined", async () => {
// Arrange
authService.getAuthStatus.mockResolvedValue(AuthenticationStatus.Locked);
routerService.getPreviousUrl.mockReturnValue(undefined);
// Act
await routerHarness.navigateByUrl("/lock-route");
// Assert
expect(routerService.persistLoginRedirectUrl).not.toHaveBeenCalled();
});
// Story: User tries to deep link to a guarded route and is logged out
it('should persist currentUrl when currentUrl does not contain "lock"', async () => {
// Arrange
authService.getAuthStatus.mockResolvedValue(AuthenticationStatus.LoggedOut);
// Act
await routerHarness.navigateByUrl("/guarded-route?item=123");
// Assert
expect(routerService.persistLoginRedirectUrl).toHaveBeenCalledWith("/guarded-route?item=123");
});
// Story: User tries to deep link to "lock"
it('should not persist currentUrl if the currentUrl contains "lock"', async () => {
// Arrange
authService.getAuthStatus.mockResolvedValue(AuthenticationStatus.LoggedOut);
// Act
await routerHarness.navigateByUrl("/lock-route");
// Assert
expect(routerService.persistLoginRedirectUrl).not.toHaveBeenCalled();
});
// Story: User tries to deep link to a guarded route from the lock page
it("should persist currentUrl over previousUrl", async () => {
// Arrange
authService.getAuthStatus.mockResolvedValue(AuthenticationStatus.Locked);
routerService.getPreviousUrl.mockReturnValue("/previous-url");
// Act
await routerHarness.navigateByUrl("/guarded-route?item=123");
// Assert
expect(routerService.persistLoginRedirectUrl).toHaveBeenCalledWith("/guarded-route?item=123");
});
// Story: user tries to deep link and is unlocked
it("should not persist any URL if the user is unlocked", async () => {
// Arrange
authService.getAuthStatus.mockResolvedValue(AuthenticationStatus.Unlocked);
// Act
await routerHarness.navigateByUrl("/guarded-route");
// Assert
expect(routerService.persistLoginRedirectUrl).not.toHaveBeenCalled();
});
// Story: User is redirected
it("should redirect user", async () => {
// Arrange
authService.getAuthStatus.mockResolvedValue(AuthenticationStatus.Unlocked);
routerService.getAndClearLoginRedirectUrl.mockResolvedValue("/redirect-route");
// Act
const activatedComponent = await routerHarness.navigateByUrl("/guarded-route");
// Assert
expect(routerService.persistLoginRedirectUrl).not.toHaveBeenCalled();
expect(TestBed.inject(Router).url).toEqual("/redirect-route");
expect(activatedComponent).toBeInstanceOf(RedirectTestComponent);
});
// Story: User is not redirected
it("should not redirect user", async () => {
// Arrange
authService.getAuthStatus.mockResolvedValue(AuthenticationStatus.Unlocked);
routerService.getAndClearLoginRedirectUrl.mockResolvedValue("");
// Act
const activatedComponent = await routerHarness.navigateByUrl("/guarded-route");
// Assert
expect(routerService.persistLoginRedirectUrl).not.toHaveBeenCalled();
expect(TestBed.inject(Router).url).toEqual("/guarded-route");
expect(activatedComponent).toBeInstanceOf(GuardedRouteTestComponent);
});
});

View File

@ -0,0 +1,56 @@
import { inject } from "@angular/core";
import { CanActivateFn, Router } from "@angular/router";
import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status";
import { Utils } from "@bitwarden/common/platform/misc/utils";
import { RouterService } from "../../core/router.service";
/**
* Guard to persist and apply deep links to handle users who are not unlocked.
* @returns returns true. If user is not Unlocked will store URL to state for redirect once
* user is unlocked/Authenticated.
*/
export function deepLinkGuard(): CanActivateFn {
return async (route, routerState) => {
// Inject Services
const authService = inject(AuthService);
const router = inject(Router);
const routerService = inject(RouterService);
// Fetch State
const currentUrl = routerState.url;
const transientPreviousUrl = routerService.getPreviousUrl();
const authStatus = await authService.getAuthStatus();
// Evaluate State
/** before anything else, check if the user is already unlocked. */
if (authStatus === AuthenticationStatus.Unlocked) {
const persistedPreLoginUrl = await routerService.getAndClearLoginRedirectUrl();
if (!Utils.isNullOrEmpty(persistedPreLoginUrl)) {
return router.navigateByUrl(persistedPreLoginUrl);
}
return true;
}
/**
* At this point the user is either `locked` or `loggedOut`, it doesn't matter.
* We opt to persist the currentUrl over the transient previousUrl. This supports
* the case where a user is locked out of their vault and they deep link from
* the "lock" page.
*
* When the user is locked out of their vault the currentUrl contains "lock" so it will
* not be persisted, the previousUrl will be persisted instead.
*/
if (isValidUrl(currentUrl)) {
await routerService.persistLoginRedirectUrl(currentUrl);
} else if (isValidUrl(transientPreviousUrl)) {
await routerService.persistLoginRedirectUrl(transientPreviousUrl);
}
return true;
};
function isValidUrl(url: string | null | undefined): boolean {
return !Utils.isNullOrEmpty(url) && !url?.toLocaleLowerCase().includes("lock");
}
}

View File

@ -19,8 +19,6 @@ import { StateService } from "@bitwarden/common/platform/abstractions/state.serv
import { PasswordStrengthServiceAbstraction } from "@bitwarden/common/tools/password-strength";
import { DialogService } from "@bitwarden/components";
import { RouterService } from "../core";
@Component({
selector: "app-lock",
templateUrl: "lock.component.html",
@ -35,7 +33,6 @@ export class LockComponent extends BaseLockComponent {
vaultTimeoutService: VaultTimeoutService,
vaultTimeoutSettingsService: VaultTimeoutSettingsService,
environmentService: EnvironmentService,
private routerService: RouterService,
stateService: StateService,
apiService: ApiService,
logService: LogService,
@ -72,10 +69,6 @@ export class LockComponent extends BaseLockComponent {
async ngOnInit() {
await super.ngOnInit();
this.onSuccessfulSubmit = async () => {
const previousUrl = this.routerService.getPreviousUrl();
if (previousUrl && previousUrl !== "/" && previousUrl.indexOf("lock") === -1) {
this.successRoute = previousUrl;
}
this.router.navigateByUrl(this.successRoute);
};
}

View File

@ -172,13 +172,8 @@ export class LoginComponent extends BaseLoginComponent implements OnInit {
}
}
const previousUrl = this.routerService.getPreviousUrl();
if (previousUrl) {
this.router.navigateByUrl(previousUrl);
} else {
this.loginService.clearValues();
this.router.navigate([this.successRoute]);
}
this.loginService.clearValues();
this.router.navigate([this.successRoute]);
}
goToHint() {

View File

@ -7,7 +7,6 @@ import { ApiService } from "@bitwarden/common/abstractions/api.service";
import { OrgDomainApiServiceAbstraction } from "@bitwarden/common/admin-console/abstractions/organization-domain/org-domain-api.service.abstraction";
import { OrganizationDomainSsoDetailsResponse } from "@bitwarden/common/admin-console/abstractions/organization-domain/responses/organization-domain-sso-details.response";
import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
import { LoginService } from "@bitwarden/common/auth/abstractions/login.service";
import { HttpStatusCode } from "@bitwarden/common/enums";
import { ErrorResponse } from "@bitwarden/common/models/response/error.response";
import { ConfigServiceAbstraction } from "@bitwarden/common/platform/abstractions/config/config.service.abstraction";
@ -39,7 +38,6 @@ export class SsoComponent extends BaseSsoComponent {
passwordGenerationService: PasswordGenerationServiceAbstraction,
logService: LogService,
private orgDomainApiService: OrgDomainApiServiceAbstraction,
private loginService: LoginService,
private validationService: ValidationService,
configService: ConfigServiceAbstraction
) {
@ -64,21 +62,6 @@ export class SsoComponent extends BaseSsoComponent {
async ngOnInit() {
super.ngOnInit();
// if we have an emergency access invite, redirect to emergency access
const emergencyAccessInvite = await this.stateService.getEmergencyAccessInvitation();
if (emergencyAccessInvite != null) {
this.onSuccessfulLoginNavigate = async () => {
this.router.navigate(["/accept-emergency"], {
queryParams: {
id: emergencyAccessInvite.id,
name: emergencyAccessInvite.name,
email: emergencyAccessInvite.email,
token: emergencyAccessInvite.token,
},
});
};
}
// eslint-disable-next-line rxjs-angular/prefer-takeuntil, rxjs/no-async-subscribe
this.route.queryParams.pipe(first()).subscribe(async (qParams) => {
if (qParams.identifier != null) {

View File

@ -18,8 +18,6 @@ import { LogService } from "@bitwarden/common/platform/abstractions/log.service"
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
import { StateService } from "@bitwarden/common/platform/abstractions/state.service";
import { RouterService } from "../core";
import { TwoFactorOptionsComponent } from "./two-factor-options.component";
@Component({
@ -44,7 +42,6 @@ export class TwoFactorComponent extends BaseTwoFactorComponent {
logService: LogService,
twoFactorService: TwoFactorService,
appIdService: AppIdService,
private routerService: RouterService,
loginService: LoginService,
configService: ConfigServiceAbstraction,
@Inject(WINDOW) protected win: Window
@ -97,29 +94,10 @@ export class TwoFactorComponent extends BaseTwoFactorComponent {
goAfterLogIn = async () => {
this.loginService.clearValues();
const previousUrl = this.routerService.getPreviousUrl();
if (previousUrl) {
this.router.navigateByUrl(previousUrl);
} else {
// if we have an emergency access invite, redirect to emergency access
const emergencyAccessInvite = await this.stateService.getEmergencyAccessInvitation();
if (emergencyAccessInvite != null) {
this.router.navigate(["/accept-emergency"], {
queryParams: {
id: emergencyAccessInvite.id,
name: emergencyAccessInvite.name,
email: emergencyAccessInvite.email,
token: emergencyAccessInvite.token,
},
});
return;
}
this.router.navigate([this.successRoute], {
queryParams: {
identifier: this.orgIdentifier,
},
});
}
this.router.navigate([this.successRoute], {
queryParams: {
identifier: this.orgIdentifier,
},
});
};
}

View File

@ -4,6 +4,8 @@ import { ActivatedRoute, NavigationEnd, Router } from "@angular/router";
import { filter } from "rxjs";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { StateService } from "@bitwarden/common/platform/abstractions/state.service";
import { Utils } from "@bitwarden/common/platform/misc/utils";
@Injectable()
export class RouterService {
@ -14,6 +16,7 @@ export class RouterService {
private router: Router,
private activatedRoute: ActivatedRoute,
private titleService: Title,
private stateService: StateService,
i18nService: I18nService
) {
this.currentUrl = this.router.url;
@ -51,11 +54,33 @@ export class RouterService {
});
}
getPreviousUrl() {
getPreviousUrl(): string | undefined {
return this.previousUrl;
}
setPreviousUrl(url: string) {
setPreviousUrl(url: string): void {
this.previousUrl = url;
}
/**
* Save URL to Global State. This service is used during the login process
* @param url URL being saved to the Global State
*/
async persistLoginRedirectUrl(url: string): Promise<void> {
await this.stateService.setDeepLinkRedirectUrl(url);
}
/**
* Fetch and clear persisted LoginRedirectUrl if present in state
*/
async getAndClearLoginRedirectUrl(): Promise<string> | undefined {
const persistedPreLoginUrl = await this.stateService.getDeepLinkRedirectUrl();
if (!Utils.isNullOrEmpty(persistedPreLoginUrl)) {
await this.persistLoginRedirectUrl(null);
return persistedPreLoginUrl;
}
return;
}
}

View File

@ -18,6 +18,8 @@ import { FamiliesForEnterpriseSetupComponent } from "./admin-console/organizatio
import { CreateOrganizationComponent } from "./admin-console/settings/create-organization.component";
import { SponsoredFamiliesComponent } from "./admin-console/settings/sponsored-families.component";
import { AcceptOrganizationComponent } from "./auth/accept-organization.component";
import { AcceptEmergencyComponent } from "./auth/emergency-access/accept/accept-emergency.component";
import { deepLinkGuard } from "./auth/guards/deep-link.guard";
import { HintComponent } from "./auth/hint.component";
import { LockComponent } from "./auth/lock.component";
import { LoginDecryptionOptionsComponent } from "./auth/login/login-decryption-options/login-decryption-options.component";
@ -113,16 +115,19 @@ const routes: Routes = [
{
path: "lock",
component: LockComponent,
canActivate: [lockGuard()],
canActivate: [deepLinkGuard(), lockGuard()],
},
{ path: "verify-email", component: VerifyEmailTokenComponent },
{
path: "accept-organization",
component: AcceptOrganizationComponent,
canActivate: [deepLinkGuard()],
data: { titleId: "joinOrganization", doNotSaveUrl: false },
},
{
path: "accept-emergency",
component: AcceptEmergencyComponent,
canActivate: [deepLinkGuard()],
data: { titleId: "acceptEmergency", doNotSaveUrl: false },
loadComponent: () =>
import("./auth/emergency-access/accept/accept-emergency.component").then(
@ -132,6 +137,7 @@ const routes: Routes = [
{
path: "accept-families-for-enterprise",
component: AcceptFamilySponsorshipComponent,
canActivate: [deepLinkGuard()],
data: { titleId: "acceptFamilySponsorship", doNotSaveUrl: false },
},
{ path: "recover", pathMatch: "full", redirectTo: "recover-2fa" },
@ -188,7 +194,7 @@ const routes: Routes = [
{
path: "",
component: UserLayoutComponent,
canActivate: [AuthGuard],
canActivate: [deepLinkGuard(), AuthGuard],
children: [
{
path: "vault",

View File

@ -430,8 +430,6 @@ export abstract class StateService<T extends Account = Account> {
setOpenAtLogin: (value: boolean, options?: StorageOptions) => Promise<void>;
getOrganizationInvitation: (options?: StorageOptions) => Promise<any>;
setOrganizationInvitation: (value: any, options?: StorageOptions) => Promise<void>;
getEmergencyAccessInvitation: (options?: StorageOptions) => Promise<any>;
setEmergencyAccessInvitation: (value: any, options?: StorageOptions) => Promise<void>;
/**
* @deprecated Do not call this directly, use OrganizationService
*/
@ -532,4 +530,17 @@ export abstract class StateService<T extends Account = Account> {
value: Record<string, Record<string, boolean>>,
options?: StorageOptions
) => Promise<void>;
/**
* fetches string value of URL user tried to navigate to while unauthenticated.
* @param options Defines the storage options for the URL; Defaults to session Storage.
* @returns route called prior to successful login.
*/
getDeepLinkRedirectUrl: (options?: StorageOptions) => Promise<string>;
/**
* Store URL in session storage by default, but can be configured. Developed to handle
* unauthN interrupted navigation.
* @param url URL of route
* @param options Defines the storage options for the URL; Defaults to session Storage.
*/
setDeepLinkRedirectUrl: (url: string, options?: StorageOptions) => Promise<void>;
}

View File

@ -7,7 +7,6 @@ export class GlobalState {
installedVersion?: string;
locale?: string;
organizationInvitation?: any;
emergencyAccessInvitation?: any;
ssoCodeVerifier?: string;
ssoOrganizationIdentifier?: string;
ssoState?: string;
@ -41,4 +40,5 @@ export class GlobalState {
disableChangedPasswordNotification?: boolean;
disableContextMenuItem?: boolean;
autoFillOverlayVisibility?: number;
deepLinkRedirectUrl?: string;
}

View File

@ -2386,23 +2386,6 @@ export class StateService<
);
}
async getEmergencyAccessInvitation(options?: StorageOptions): Promise<any> {
return (
await this.getGlobals(this.reconcileOptions(options, await this.defaultOnDiskOptions()))
)?.emergencyAccessInvitation;
}
async setEmergencyAccessInvitation(value: any, options?: StorageOptions): Promise<void> {
const globals = await this.getGlobals(
this.reconcileOptions(options, await this.defaultOnDiskOptions())
);
globals.emergencyAccessInvitation = value;
await this.saveGlobals(
globals,
this.reconcileOptions(options, await this.defaultOnDiskOptions())
);
}
/**
* @deprecated Do not call this directly, use OrganizationService
*/
@ -2884,6 +2867,23 @@ export class StateService<
);
}
async getDeepLinkRedirectUrl(options?: StorageOptions): Promise<string> {
return (
await this.getGlobals(this.reconcileOptions(options, await this.defaultOnDiskOptions()))
)?.deepLinkRedirectUrl;
}
async setDeepLinkRedirectUrl(url: string, options?: StorageOptions): Promise<void> {
const globals = await this.getGlobals(
this.reconcileOptions(options, await this.defaultOnDiskOptions())
);
globals.deepLinkRedirectUrl = url;
await this.saveGlobals(
globals,
this.reconcileOptions(options, await this.defaultOnDiskOptions())
);
}
protected async getGlobals(options: StorageOptions): Promise<TGlobalState> {
let globals: TGlobalState;
if (this.useMemory(options.storageLocation)) {