From ae35cb4e65af6e068cd0504272f0828cba763d1b Mon Sep 17 00:00:00 2001 From: Ike <137194738+ike-kottlowski@users.noreply.github.com> Date: Wed, 21 May 2025 08:24:17 -0400 Subject: [PATCH] [PM-20540] Deep-link refactor to fix SSO deep links (#14587) * PM-20540 - TwoFactorAuthComponent - Refactor determineDefaultSuccessRoute to rely on user's auth status as the loginStrategyService's state is cleared after successful AuthN * PM-20540 - DeepLinkGuard - Refactor to exempt login-initiated so that TDE + unlock with MP + deep link works. * doc: Add documentation and change folder structure. * test: add test for new excluded route. --------- Co-authored-by: Jared Snider --- .../organization-routing.module.ts | 2 +- .../src/app/auth/guards/deep-link.guard.ts | 58 ----------- .../{ => deep-link}/deep-link.guard.spec.ts | 14 ++- .../auth/guards/deep-link/deep-link.guard.ts | 99 +++++++++++++++++++ .../src/app/auth/guards/deep-link/readme.md | 23 +++++ apps/web/src/app/oss-routing.module.ts | 2 +- .../organizations-routing.module.ts | 2 +- .../bit-web/src/app/app-routing.module.ts | 2 +- .../two-factor-auth.component.spec.ts | 9 +- .../two-factor-auth.component.ts | 8 +- libs/common/src/platform/misc/utils.ts | 2 +- 11 files changed, 153 insertions(+), 68 deletions(-) delete mode 100644 apps/web/src/app/auth/guards/deep-link.guard.ts rename apps/web/src/app/auth/guards/{ => deep-link}/deep-link.guard.spec.ts (92%) create mode 100644 apps/web/src/app/auth/guards/deep-link/deep-link.guard.ts create mode 100644 apps/web/src/app/auth/guards/deep-link/readme.md diff --git a/apps/web/src/app/admin-console/organizations/organization-routing.module.ts b/apps/web/src/app/admin-console/organizations/organization-routing.module.ts index e5c68b7354..4d8971f74f 100644 --- a/apps/web/src/app/admin-console/organizations/organization-routing.module.ts +++ b/apps/web/src/app/admin-console/organizations/organization-routing.module.ts @@ -14,7 +14,7 @@ import { } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; -import { deepLinkGuard } from "../../auth/guards/deep-link.guard"; +import { deepLinkGuard } from "../../auth/guards/deep-link/deep-link.guard"; import { VaultModule } from "./collections/vault.module"; import { isEnterpriseOrgGuard } from "./guards/is-enterprise-org.guard"; diff --git a/apps/web/src/app/auth/guards/deep-link.guard.ts b/apps/web/src/app/auth/guards/deep-link.guard.ts deleted file mode 100644 index 387e7b17e8..0000000000 --- a/apps/web/src/app/auth/guards/deep-link.guard.ts +++ /dev/null @@ -1,58 +0,0 @@ -// FIXME: Update this file to be type safe and remove this and next line -// @ts-strict-ignore -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"); - } -} diff --git a/apps/web/src/app/auth/guards/deep-link.guard.spec.ts b/apps/web/src/app/auth/guards/deep-link/deep-link.guard.spec.ts similarity index 92% rename from apps/web/src/app/auth/guards/deep-link.guard.spec.ts rename to apps/web/src/app/auth/guards/deep-link/deep-link.guard.spec.ts index 82ed004cf5..dba4dbd835 100644 --- a/apps/web/src/app/auth/guards/deep-link.guard.spec.ts +++ b/apps/web/src/app/auth/guards/deep-link/deep-link.guard.spec.ts @@ -7,7 +7,7 @@ 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 { RouterService } from "../../../core/router.service"; import { deepLinkGuard } from "./deep-link.guard"; @@ -99,6 +99,18 @@ describe("Deep Link Guard", () => { expect(routerService.persistLoginRedirectUrl).not.toHaveBeenCalled(); }); + it('should not persist routerService.previousUrl when routerService.previousUrl contains "login-initiated"', async () => { + // Arrange + authService.getAuthStatus.mockResolvedValue(AuthenticationStatus.Locked); + routerService.getPreviousUrl.mockReturnValue("/login-initiated"); + + // 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 diff --git a/apps/web/src/app/auth/guards/deep-link/deep-link.guard.ts b/apps/web/src/app/auth/guards/deep-link/deep-link.guard.ts new file mode 100644 index 0000000000..003f058096 --- /dev/null +++ b/apps/web/src/app/auth/guards/deep-link/deep-link.guard.ts @@ -0,0 +1,99 @@ +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: string | undefined = + await routerService.getAndClearLoginRedirectUrl(); + if (persistedPreLoginUrl === undefined) { + // Url us undefined, so there is nothing to navigate to. + return true; + } + // Check if the url is empty or null + if (!Utils.isNullOrEmpty(persistedPreLoginUrl)) { + // const urlTree: string | UrlTree = 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) && transientPreviousUrl !== undefined) { + await routerService.persistLoginRedirectUrl(transientPreviousUrl); + } + return true; + }; + + /** + * Check if the URL is valid for deep linking. A valid url is described as not including + * "lock" or "login-initiated". Valid urls are only urls that are not part of login or + * decryption flows. + * We ignore the "lock" url because standard SSO flows will send users to the lock component. + * We ignore "login-initiated" because TDE users decrypting with master passwords are + * sent to the lock component. + * @param url The URL to check. + * @returns True if the URL is valid, false otherwise. + */ + function isValidUrl(url: string | null | undefined): boolean { + if (url === undefined || url === null) { + return false; + } + + if (Utils.isNullOrEmpty(url)) { + return false; + } + const lowerCaseUrl: string = url.toLocaleLowerCase(); + + /** + * "Login-initiated" ignored because it is used for TDE users decrypting from a new device. A TDE user + * can opt to decrypt using their password. Decrypting with a password will send the user to the lock component, + * which is protected by the deep link guard. We don't persist the `login-initiated` url because it is not a + * valid deep-link. We don't want users to be sent to the login-initiated url when they are unlocked. + * If we did navigate to the login-initiated url, the user would get caught by the TDE Guard and be sent + * to the vault and not the intended deep link. + * + * "Lock" is ignored because users cannot deep-link to the lock component if they are already unlocked. + * Users logging in with SSO will be sent to the lock component after they are authenticated with their IdP. + * SSO users would be navigated to the "lock" component loop if we persisted the "lock" url. + */ + + if (lowerCaseUrl.includes("/login-initiated") || lowerCaseUrl.includes("/lock")) { + return false; + } + + return true; + } +} diff --git a/apps/web/src/app/auth/guards/deep-link/readme.md b/apps/web/src/app/auth/guards/deep-link/readme.md new file mode 100644 index 0000000000..82aebf9547 --- /dev/null +++ b/apps/web/src/app/auth/guards/deep-link/readme.md @@ -0,0 +1,23 @@ +# Deep-link Guard + +The `deep-link.guard.ts` supports users who are trying to access a protected route from an unauthenticated or locked state. + +This guard will persist the protected URL to session state when a user is either unauthenticated or in an encrypted/locked state. This allows users to have multiple tabs of the application running simultaneously without interfering with 'previousUrl` functionality. + +Writing to session state allows users who are authenticating through SSO to be routed to their identity provider and back without losing the protected route they were trying to access in the first place. + +The deep link guard will not persist Urls that are in the middle of authentication or decryption. SSO users will sometimes have to decrypt their vault after a successful authentication. This is why we do not persist the `/lock` route. + +## General operation + +The `deep-link.guard.ts` will always return true. The `deep-link.guard.ts` will only persist a URL if the user is in an unauthenticated or locked state. The URL cannot contain `/lock` or `/login-initiated`. The persisted URL is cleared from state when it is read. + +## Routes to protect + +The deep link guards should be used on routes where a user will be navigated to a protected route but may not be authenticated, decrypted, or have an account. + +A use cases is the `emergency-access` route which is a link that is sent to the user's email address, and in order for them to accept the request, they must first authenticate and decrypt. + +## TDE Users decrypting/unlocking with password + +For TDE users opting to decrypt with a password they will be routed from the `login-initiated` to the `lock` route. We ignore the `login-initiated` route for this reason allowing TDE users who decrypt/unlock with a password to still be navigated to the initial request. diff --git a/apps/web/src/app/oss-routing.module.ts b/apps/web/src/app/oss-routing.module.ts index 37da9a35f6..0d6ffb88ad 100644 --- a/apps/web/src/app/oss-routing.module.ts +++ b/apps/web/src/app/oss-routing.module.ts @@ -48,7 +48,7 @@ import { VerifyRecoverDeleteOrgComponent } from "./admin-console/organizations/m import { AcceptFamilySponsorshipComponent } from "./admin-console/organizations/sponsorships/accept-family-sponsorship.component"; import { FamiliesForEnterpriseSetupComponent } from "./admin-console/organizations/sponsorships/families-for-enterprise-setup.component"; import { CreateOrganizationComponent } from "./admin-console/settings/create-organization.component"; -import { deepLinkGuard } from "./auth/guards/deep-link.guard"; +import { deepLinkGuard } from "./auth/guards/deep-link/deep-link.guard"; import { LoginViaWebAuthnComponent } from "./auth/login/login-via-webauthn/login-via-webauthn.component"; import { AcceptOrganizationComponent } from "./auth/organization-invite/accept-organization.component"; import { RecoverDeleteComponent } from "./auth/recover-delete.component"; diff --git a/bitwarden_license/bit-web/src/app/admin-console/organizations/organizations-routing.module.ts b/bitwarden_license/bit-web/src/app/admin-console/organizations/organizations-routing.module.ts index a995b0cb1f..f63140a8b2 100644 --- a/bitwarden_license/bit-web/src/app/admin-console/organizations/organizations-routing.module.ts +++ b/bitwarden_license/bit-web/src/app/admin-console/organizations/organizations-routing.module.ts @@ -6,7 +6,7 @@ import { canAccessSettingsTab } from "@bitwarden/common/admin-console/abstractio import { isEnterpriseOrgGuard } from "@bitwarden/web-vault/app/admin-console/organizations/guards/is-enterprise-org.guard"; import { organizationPermissionsGuard } from "@bitwarden/web-vault/app/admin-console/organizations/guards/org-permissions.guard"; import { OrganizationLayoutComponent } from "@bitwarden/web-vault/app/admin-console/organizations/layouts/organization-layout.component"; -import { deepLinkGuard } from "@bitwarden/web-vault/app/auth/guards/deep-link.guard"; +import { deepLinkGuard } from "@bitwarden/web-vault/app/auth/guards/deep-link/deep-link.guard"; import { SsoComponent } from "../../auth/sso/sso.component"; diff --git a/bitwarden_license/bit-web/src/app/app-routing.module.ts b/bitwarden_license/bit-web/src/app/app-routing.module.ts index 6aed12511c..3f2803695e 100644 --- a/bitwarden_license/bit-web/src/app/app-routing.module.ts +++ b/bitwarden_license/bit-web/src/app/app-routing.module.ts @@ -3,7 +3,7 @@ import { RouterModule, Routes } from "@angular/router"; import { unauthGuardFn } from "@bitwarden/angular/auth/guards"; import { AnonLayoutWrapperComponent } from "@bitwarden/auth/angular"; -import { deepLinkGuard } from "@bitwarden/web-vault/app/auth/guards/deep-link.guard"; +import { deepLinkGuard } from "@bitwarden/web-vault/app/auth/guards/deep-link/deep-link.guard"; import { RouteDataProperties } from "@bitwarden/web-vault/app/core"; import { ProvidersModule } from "./admin-console/providers/providers.module"; diff --git a/libs/auth/src/angular/two-factor-auth/two-factor-auth.component.spec.ts b/libs/auth/src/angular/two-factor-auth/two-factor-auth.component.spec.ts index de3fa7a332..e52f08941d 100644 --- a/libs/auth/src/angular/two-factor-auth/two-factor-auth.component.spec.ts +++ b/libs/auth/src/angular/two-factor-auth/two-factor-auth.component.spec.ts @@ -16,8 +16,10 @@ import { } from "@bitwarden/auth/common"; import { ApiService } from "@bitwarden/common/abstractions/api.service"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { SsoLoginServiceAbstraction } from "@bitwarden/common/auth/abstractions/sso-login.service.abstraction"; import { TwoFactorService } from "@bitwarden/common/auth/abstractions/two-factor.service"; +import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; import { AuthenticationType } from "@bitwarden/common/auth/enums/authentication-type"; import { AuthResult } from "@bitwarden/common/auth/models/domain/auth-result"; import { ForceSetPasswordReason } from "@bitwarden/common/auth/models/domain/force-set-password-reason"; @@ -72,6 +74,7 @@ describe("TwoFactorAuthComponent", () => { let mockEnvService: MockProxy; let mockLoginSuccessHandlerService: MockProxy; let mockTwoFactorAuthCompCacheService: MockProxy; + let mockAuthService: MockProxy; let mockUserDecryptionOpts: { noMasterPassword: UserDecryptionOptions; @@ -106,6 +109,7 @@ describe("TwoFactorAuthComponent", () => { mockDialogService = mock(); mockToastService = mock(); mockTwoFactorAuthCompService = mock(); + mockAuthService = mock(); mockEnvService = mock(); mockLoginSuccessHandlerService = mock(); @@ -204,6 +208,7 @@ describe("TwoFactorAuthComponent", () => { provide: TwoFactorAuthComponentCacheService, useValue: mockTwoFactorAuthCompCacheService, }, + { provide: AuthService, useValue: mockAuthService }, ], }); @@ -295,6 +300,7 @@ describe("TwoFactorAuthComponent", () => { it("navigates to the component's defined success route (vault is default) when the login is successful", async () => { mockLoginStrategyService.logInTwoFactor.mockResolvedValue(new AuthResult()); + mockAuthService.activeAccountStatus$ = new BehaviorSubject(AuthenticationStatus.Unlocked); // Act await component.submit("testToken"); @@ -316,13 +322,14 @@ describe("TwoFactorAuthComponent", () => { async (authType, expectedRoute) => { mockLoginStrategyService.logInTwoFactor.mockResolvedValue(new AuthResult()); currentAuthTypeSubject.next(authType); + mockAuthService.activeAccountStatus$ = new BehaviorSubject(AuthenticationStatus.Locked); // Act await component.submit("testToken"); // Assert expect(mockRouter.navigate).toHaveBeenCalledTimes(1); - expect(mockRouter.navigate).toHaveBeenCalledWith(["lock"], { + expect(mockRouter.navigate).toHaveBeenCalledWith([expectedRoute], { queryParams: { identifier: component.orgSsoIdentifier, }, diff --git a/libs/auth/src/angular/two-factor-auth/two-factor-auth.component.ts b/libs/auth/src/angular/two-factor-auth/two-factor-auth.component.ts index 91901fa354..b14a368e06 100644 --- a/libs/auth/src/angular/two-factor-auth/two-factor-auth.component.ts +++ b/libs/auth/src/angular/two-factor-auth/two-factor-auth.component.ts @@ -24,9 +24,10 @@ import { LoginSuccessHandlerService, } from "@bitwarden/auth/common"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { SsoLoginServiceAbstraction } from "@bitwarden/common/auth/abstractions/sso-login.service.abstraction"; import { TwoFactorService } from "@bitwarden/common/auth/abstractions/two-factor.service"; -import { AuthenticationType } from "@bitwarden/common/auth/enums/authentication-type"; +import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; import { TwoFactorProviderType } from "@bitwarden/common/auth/enums/two-factor-provider-type"; import { AuthResult } from "@bitwarden/common/auth/models/domain/auth-result"; import { ForceSetPasswordReason } from "@bitwarden/common/auth/models/domain/force-set-password-reason"; @@ -167,6 +168,7 @@ export class TwoFactorAuthComponent implements OnInit, OnDestroy { private environmentService: EnvironmentService, private loginSuccessHandlerService: LoginSuccessHandlerService, private twoFactorAuthComponentCacheService: TwoFactorAuthComponentCacheService, + private authService: AuthService, ) {} async ngOnInit() { @@ -507,8 +509,8 @@ export class TwoFactorAuthComponent implements OnInit, OnDestroy { } private async determineDefaultSuccessRoute(): Promise { - const authType = await firstValueFrom(this.loginStrategyService.currentAuthType$); - if (authType == AuthenticationType.Sso || authType == AuthenticationType.UserApiKey) { + const activeAccountStatus = await firstValueFrom(this.authService.activeAccountStatus$); + if (activeAccountStatus === AuthenticationStatus.Locked) { return "lock"; } diff --git a/libs/common/src/platform/misc/utils.ts b/libs/common/src/platform/misc/utils.ts index 203a04851c..f9c5ab4a84 100644 --- a/libs/common/src/platform/misc/utils.ts +++ b/libs/common/src/platform/misc/utils.ts @@ -391,7 +391,7 @@ export class Utils { return str == null || typeof str !== "string" || str.trim() === ""; } - static isNullOrEmpty(str: string): boolean { + static isNullOrEmpty(str: string | null): boolean { return str == null || typeof str !== "string" || str == ""; }