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-17776] New Device - SSO Check #13177

Merged
merged 5 commits into from
Feb 7, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@ import { TestBed } from "@angular/core/testing";
import { ActivatedRouteSnapshot, Router, RouterStateSnapshot } from "@angular/router";
import { BehaviorSubject } from "rxjs";

import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction";
import { PolicyType } from "@bitwarden/common/admin-console/enums";
import { Account, AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction";
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
Expand Down Expand Up @@ -36,17 +35,21 @@ describe("NewDeviceVerificationNoticeGuard", () => {
});
const isSelfHost = jest.fn().mockReturnValue(false);
const getProfileTwoFactorEnabled = jest.fn().mockResolvedValue(false);
const policyAppliesToActiveUser$ = jest.fn().mockReturnValue(new BehaviorSubject<boolean>(false));
const noticeState$ = jest.fn().mockReturnValue(new BehaviorSubject(null));
const getProfileCreationDate = jest.fn().mockResolvedValue(eightDaysAgo);
const hasMasterPasswordAndMasterKeyHash = jest.fn().mockResolvedValue(true);
const getUserSSOBound = jest.fn().mockResolvedValue(false);
const getUserSSOBoundAdminOwner = jest.fn().mockResolvedValue(false);

beforeEach(() => {
getFeatureFlag.mockClear();
isSelfHost.mockClear();
getProfileCreationDate.mockClear();
getProfileTwoFactorEnabled.mockClear();
policyAppliesToActiveUser$.mockClear();
createUrlTree.mockClear();
hasMasterPasswordAndMasterKeyHash.mockClear();
getUserSSOBound.mockClear();
getUserSSOBoundAdminOwner.mockClear();

TestBed.configureTestingModule({
providers: [
Expand All @@ -55,10 +58,15 @@ describe("NewDeviceVerificationNoticeGuard", () => {
{ provide: NewDeviceVerificationNoticeService, useValue: { noticeState$ } },
{ provide: AccountService, useValue: { activeAccount$ } },
{ provide: PlatformUtilsService, useValue: { isSelfHost } },
{ provide: PolicyService, useValue: { policyAppliesToActiveUser$ } },
{ provide: UserVerificationService, useValue: { hasMasterPasswordAndMasterKeyHash } },
{
provide: VaultProfileService,
useValue: { getProfileCreationDate, getProfileTwoFactorEnabled },
useValue: {
getProfileCreationDate,
getProfileTwoFactorEnabled,
getUserSSOBound,
getUserSSOBoundAdminOwner,
},
},
],
});
Expand Down Expand Up @@ -90,7 +98,7 @@ describe("NewDeviceVerificationNoticeGuard", () => {
expect(isSelfHost).not.toHaveBeenCalled();
expect(getProfileTwoFactorEnabled).not.toHaveBeenCalled();
expect(getProfileCreationDate).not.toHaveBeenCalled();
expect(policyAppliesToActiveUser$).not.toHaveBeenCalled();
expect(hasMasterPasswordAndMasterKeyHash).not.toHaveBeenCalled();
});
});

Expand Down Expand Up @@ -121,13 +129,6 @@ describe("NewDeviceVerificationNoticeGuard", () => {
expect(await newDeviceGuard()).toBe(true);
});

it("returns `true` SSO is required", async () => {
policyAppliesToActiveUser$.mockReturnValueOnce(new BehaviorSubject(true));

expect(await newDeviceGuard()).toBe(true);
expect(policyAppliesToActiveUser$).toHaveBeenCalledWith(PolicyType.RequireSso);
});

it("returns `true` when the profile was created less than a week ago", async () => {
const sixDaysAgo = new Date();
sixDaysAgo.setDate(sixDaysAgo.getDate() - 6);
Expand All @@ -143,6 +144,57 @@ describe("NewDeviceVerificationNoticeGuard", () => {
expect(await newDeviceGuard()).toBe(true);
});

describe("SSO bound", () => {
beforeEach(() => {
getFeatureFlag.mockImplementation((key) => {
if (key === FeatureFlag.NewDeviceVerificationPermanentDismiss) {
return Promise.resolve(true);
}

return Promise.resolve(false);
});
});

afterAll(() => {
getFeatureFlag.mockReturnValue(false);
});

it('returns "true" when the user is SSO bound and not an admin or owner', async () => {
getUserSSOBound.mockResolvedValueOnce(true);
getUserSSOBoundAdminOwner.mockResolvedValueOnce(false);

expect(await newDeviceGuard()).toBe(true);
});

it('returns "true" when the user is an admin or owner of an SSO bound organization and has not logged in with their master password', async () => {
getUserSSOBound.mockResolvedValueOnce(true);
getUserSSOBoundAdminOwner.mockResolvedValueOnce(true);
hasMasterPasswordAndMasterKeyHash.mockResolvedValueOnce(false);

expect(await newDeviceGuard()).toBe(true);
});

it("shows notice when the user is an admin or owner of an SSO bound organization and logged in with their master password", async () => {
getUserSSOBound.mockResolvedValueOnce(true);
getUserSSOBoundAdminOwner.mockResolvedValueOnce(true);
hasMasterPasswordAndMasterKeyHash.mockResolvedValueOnce(true);

await newDeviceGuard();

expect(createUrlTree).toHaveBeenCalledWith(["/new-device-notice"]);
});

it("shows notice when the user that is not in an SSO bound organization", async () => {
getUserSSOBound.mockResolvedValueOnce(false);
getUserSSOBoundAdminOwner.mockResolvedValueOnce(false);
hasMasterPasswordAndMasterKeyHash.mockResolvedValueOnce(true);

await newDeviceGuard();

expect(createUrlTree).toHaveBeenCalledWith(["/new-device-notice"]);
});
});

describe("temp flag", () => {
beforeEach(() => {
getFeatureFlag.mockImplementation((key) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@ import { inject } from "@angular/core";
import { ActivatedRouteSnapshot, CanActivateFn, Router } from "@angular/router";
import { firstValueFrom, Observable } from "rxjs";

import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction";
import { PolicyType } from "@bitwarden/common/admin-console/enums";
import { Account, AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction";
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
Expand All @@ -20,8 +19,8 @@ export const NewDeviceVerificationNoticeGuard: CanActivateFn = async (
const newDeviceVerificationNoticeService = inject(NewDeviceVerificationNoticeService);
const accountService = inject(AccountService);
const platformUtilsService = inject(PlatformUtilsService);
const policyService = inject(PolicyService);
const vaultProfileService = inject(VaultProfileService);
const userVerificationService = inject(UserVerificationService);

if (route.queryParams["fromNewDeviceVerification"]) {
return true;
Expand All @@ -47,16 +46,21 @@ export const NewDeviceVerificationNoticeGuard: CanActivateFn = async (

try {
const isSelfHosted = platformUtilsService.isSelfHost();
const requiresSSO = await isSSORequired(policyService);
const userIsSSOUser = await ssoAppliesToUser(
userVerificationService,
vaultProfileService,
currentAcct.id,
);
const has2FAEnabled = await hasATwoFactorProviderEnabled(vaultProfileService, currentAcct.id);
const isProfileLessThanWeekOld = await profileIsLessThanWeekOld(
vaultProfileService,
currentAcct.id,
);

// When any of the following are true, the device verification notice is
// not applicable for the user.
if (has2FAEnabled || isSelfHosted || requiresSSO || isProfileLessThanWeekOld) {
// not applicable for the user. When the user has *not* logged in with their
// master password, assume they logged in with SSO.
if (has2FAEnabled || isSelfHosted || userIsSSOUser || isProfileLessThanWeekOld) {
return true;
}
} catch {
Expand Down Expand Up @@ -105,9 +109,39 @@ async function profileIsLessThanWeekOld(
return !isMoreThan7DaysAgo(creationDate);
}

/** Returns true when the user is required to login via SSO */
async function isSSORequired(policyService: PolicyService) {
return firstValueFrom(policyService.policyAppliesToActiveUser$(PolicyType.RequireSso));
/**
* Returns true when either:
* - The user is SSO bound to an organization and is not an Admin or Owner
* - The user is an Admin or Owner of an organization with SSO bound and has not logged in with their master password
*
* NOTE: There are edge cases where this does not satisfy the original requirement of showing the notice to
* users who are subject to the SSO required policy. When Owners and Admins log in with their MP they will see the notice
* when they log in with SSO they will not. This is a concession made because the original logic references policies would not work for TDE users.
* When this guard is run for those users a sync hasn't occurred and thus the policies are not available.
*/
async function ssoAppliesToUser(
userVerificationService: UserVerificationService,
vaultProfileService: VaultProfileService,
userId: string,
) {
const userSSOBound = await vaultProfileService.getUserSSOBound(userId);
const userSSOBoundAdminOwner = await vaultProfileService.getUserSSOBoundAdminOwner(userId);
const userLoggedInWithMP = await userLoggedInWithMasterPassword(userVerificationService, userId);

const nonOwnerAdminSsoUser = userSSOBound && !userSSOBoundAdminOwner;
const ssoAdminOwnerLoggedInWithMP = userSSOBoundAdminOwner && !userLoggedInWithMP;

return nonOwnerAdminSsoUser || ssoAdminOwnerLoggedInWithMP;
}

/**
* Returns true when the user logged in with their master password.
*/
async function userLoggedInWithMasterPassword(
userVerificationService: UserVerificationService,
userId: string,
) {
return userVerificationService.hasMasterPasswordAndMasterKeyHash(userId);
}

/** Returns the true when the date given is older than 7 days */
Expand Down
67 changes: 67 additions & 0 deletions libs/angular/src/vault/services/vault-profile.service.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { TestBed } from "@angular/core/testing";

import { ApiService } from "@bitwarden/common/abstractions/api.service";
import { OrganizationUserType } from "@bitwarden/common/admin-console/enums";

import { VaultProfileService } from "./vault-profile.service";

Expand All @@ -13,6 +14,12 @@ describe("VaultProfileService", () => {
creationDate: hardcodedDateString,
twoFactorEnabled: true,
id: "new-user-id",
organizations: [
{
ssoBound: true,
type: OrganizationUserType.Admin,
},
],
});

beforeEach(() => {
Expand Down Expand Up @@ -91,4 +98,64 @@ describe("VaultProfileService", () => {
expect(getProfile).not.toHaveBeenCalled();
});
});

describe("getUserSSOBound", () => {
it("calls `getProfile` when stored ssoBound property is not stored", async () => {
expect(service["userIsSsoBound"]).toBeNull();

const userIsSsoBound = await service.getUserSSOBound(userId);

expect(userIsSsoBound).toBe(true);
expect(getProfile).toHaveBeenCalled();
});

it("calls `getProfile` when stored profile id does not match", async () => {
service["userIsSsoBound"] = false;
service["userId"] = "old-user-id";

const userIsSsoBound = await service.getUserSSOBound(userId);

expect(userIsSsoBound).toBe(true);
expect(getProfile).toHaveBeenCalled();
});

it("does not call `getProfile` when ssoBound property is already stored", async () => {
service["userIsSsoBound"] = false;

const userIsSsoBound = await service.getUserSSOBound(userId);

expect(userIsSsoBound).toBe(false);
expect(getProfile).not.toHaveBeenCalled();
});
});

describe("getUserSSOBoundAdminOwner", () => {
it("calls `getProfile` when stored userIsSsoBoundAdminOwner property is not stored", async () => {
expect(service["userIsSsoBoundAdminOwner"]).toBeNull();

const userIsSsoBoundAdminOwner = await service.getUserSSOBoundAdminOwner(userId);

expect(userIsSsoBoundAdminOwner).toBe(true);
expect(getProfile).toHaveBeenCalled();
});

it("calls `getProfile` when stored profile id does not match", async () => {
service["userIsSsoBoundAdminOwner"] = false;
service["userId"] = "old-user-id";

const userIsSsoBoundAdminOwner = await service.getUserSSOBoundAdminOwner(userId);

expect(userIsSsoBoundAdminOwner).toBe(true);
expect(getProfile).toHaveBeenCalled();
});

it("does not call `getProfile` when userIsSsoBoundAdminOwner property is already stored", async () => {
service["userIsSsoBoundAdminOwner"] = false;

const userIsSsoBoundAdminOwner = await service.getUserSSOBoundAdminOwner(userId);

expect(userIsSsoBoundAdminOwner).toBe(false);
expect(getProfile).not.toHaveBeenCalled();
});
});
});
38 changes: 38 additions & 0 deletions libs/angular/src/vault/services/vault-profile.service.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Injectable, inject } from "@angular/core";

import { ApiService } from "@bitwarden/common/abstractions/api.service";
import { OrganizationUserType } from "@bitwarden/common/admin-console/enums";
import { ProfileResponse } from "@bitwarden/common/models/response/profile.response";

@Injectable({
Expand All @@ -24,6 +25,12 @@ export class VaultProfileService {
/** True when 2FA is enabled on the profile. */
private profile2FAEnabled: boolean | null = null;

/** True when ssoBound is true for any of the users organizations */
private userIsSsoBound: boolean | null = null;

/** True when the user is an admin or owner of the ssoBound organization */
private userIsSsoBoundAdminOwner: boolean | null = null;

/**
* Returns the creation date of the profile.
* Note: `Date`s are mutable in JS, creating a new
Expand Down Expand Up @@ -52,12 +59,43 @@ export class VaultProfileService {
return profile.twoFactorEnabled;
}

/**
* Returns whether the user logs in with SSO for any organization.
*/
async getUserSSOBound(userId: string): Promise<boolean> {
if (this.userIsSsoBound !== null && userId === this.userId) {
return Promise.resolve(this.userIsSsoBound);
}

await this.fetchAndCacheProfile();

return !!this.userIsSsoBound;
}

/**
* Returns true when the user is an Admin or Owner of an organization with `ssoBound` true.
*/
async getUserSSOBoundAdminOwner(userId: string): Promise<boolean> {
if (this.userIsSsoBoundAdminOwner !== null && userId === this.userId) {
return Promise.resolve(this.userIsSsoBoundAdminOwner);
}

await this.fetchAndCacheProfile();

return !!this.userIsSsoBoundAdminOwner;
}

private async fetchAndCacheProfile(): Promise<ProfileResponse> {
const profile = await this.apiService.getProfile();

this.userId = profile.id;
this.profileCreatedDate = profile.creationDate;
this.profile2FAEnabled = profile.twoFactorEnabled;
const ssoBoundOrg = profile.organizations.find((org) => org.ssoBound);
this.userIsSsoBound = !!ssoBoundOrg;
this.userIsSsoBoundAdminOwner =
ssoBoundOrg?.type === OrganizationUserType.Admin ||
ssoBoundOrg?.type === OrganizationUserType.Owner;

return profile;
}
Expand Down
Loading