mirror of
https://github.com/bitwarden/browser
synced 2026-02-01 09:13:54 +00:00
add defensive checks for forceSetPasswordReason
This commit is contained in:
@@ -23,9 +23,11 @@ import { PolicyService } from "@bitwarden/common/admin-console/abstractions/poli
|
||||
import { PolicyType } from "@bitwarden/common/admin-console/enums";
|
||||
import { Organization } from "@bitwarden/common/admin-console/models/domain/organization";
|
||||
import { Account, AccountService } from "@bitwarden/common/auth/abstractions/account.service";
|
||||
import { ForceSetPasswordReason } from "@bitwarden/common/auth/models/domain/force-set-password-reason";
|
||||
import { getUserId } from "@bitwarden/common/auth/services/account.service";
|
||||
import { BillingAccountProfileStateService } from "@bitwarden/common/billing/abstractions/account/billing-account-profile-state.service";
|
||||
import { EventType } from "@bitwarden/common/enums";
|
||||
import { MasterPasswordServiceAbstraction } from "@bitwarden/common/key-management/master-password/abstractions/master-password.service.abstraction";
|
||||
import { BroadcasterService } from "@bitwarden/common/platform/abstractions/broadcaster.service";
|
||||
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
|
||||
import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service";
|
||||
@@ -224,6 +226,7 @@ export class VaultV2Component<C extends CipherViewLike>
|
||||
private cipherArchiveService: CipherArchiveService,
|
||||
private policyService: PolicyService,
|
||||
private archiveCipherUtilitiesService: ArchiveCipherUtilitiesService,
|
||||
private masterPasswordService: MasterPasswordServiceAbstraction,
|
||||
) {}
|
||||
|
||||
async ngOnInit() {
|
||||
@@ -333,13 +336,12 @@ export class VaultV2Component<C extends CipherViewLike>
|
||||
this.searchBarService.setEnabled(true);
|
||||
this.searchBarService.setPlaceholderText(this.i18nService.t("searchVault"));
|
||||
|
||||
// If there are pending auth requests for this user, a LoginApprovalDialogComponent will open
|
||||
this.messagingService.send("openLoginApproval");
|
||||
|
||||
this.activeUserId = await firstValueFrom(
|
||||
this.accountService.activeAccount$.pipe(getUserId),
|
||||
).catch((): any => null);
|
||||
|
||||
await this.sendOpenLoginApprovalMessage(this.activeUserId);
|
||||
|
||||
if (this.activeUserId) {
|
||||
this.cipherService
|
||||
.failedToDecryptCiphers$(this.activeUserId)
|
||||
@@ -1005,4 +1007,27 @@ export class VaultV2Component<C extends CipherViewLike>
|
||||
}
|
||||
return repromptResult;
|
||||
}
|
||||
|
||||
/**
|
||||
* Sends a message that will retrieve any pending auth requests from the server. If there are
|
||||
* pending auth requests for this user, a LoginApprovalDialogComponent will open. If there
|
||||
* are no pending auth requests, nothing happens (see AppComponent: "openLoginApproval").
|
||||
*/
|
||||
private async sendOpenLoginApprovalMessage(activeUserId: UserId) {
|
||||
// This is a defensive check against a race condition where a user may have successfully logged
|
||||
// in with no forceSetPasswordReason, but while the vault component is loading, a sync sets
|
||||
// forceSetPasswordReason.TdeUserWithoutPasswordHasPasswordResetPermission (see DefaultSyncService).
|
||||
// This could potentially happen if an Admin upgrades the user's permissions as the user is logging
|
||||
// in. In this rare case we do not want to send an "openLoginApproval" message.
|
||||
//
|
||||
// This also keeps parity with other usages of the "openLoginApproval" message. That is: don't send
|
||||
// an "openLoginApproval" message if the user is required to set/change their password.
|
||||
const forceSetPasswordReason = await firstValueFrom(
|
||||
this.masterPasswordService.forceSetPasswordReason$(activeUserId),
|
||||
);
|
||||
if (forceSetPasswordReason === ForceSetPasswordReason.None) {
|
||||
// If there are pending auth requests for this user, a LoginApprovalDialogComponent will open
|
||||
this.messagingService.send("openLoginApproval");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -267,6 +267,29 @@ describe("DefaultAuthRequestAnsweringService", () => {
|
||||
// Assert
|
||||
expect(messagingService.send).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("should NOT process pending auth requests when switching to an Unlocked user who is required to set/change their master password", async () => {
|
||||
// Arrange
|
||||
masterPasswordService.forceSetPasswordReason$.mockReturnValue(
|
||||
of(ForceSetPasswordReason.WeakMasterPassword),
|
||||
);
|
||||
authStatusForSubjects.set(otherUserId, new BehaviorSubject(AuthenticationStatus.Unlocked));
|
||||
pendingRequestMarkers = [{ userId: otherUserId, receivedAtMs: Date.now() }];
|
||||
pendingAuthRequestsState.getAll$.mockReturnValue(of(pendingRequestMarkers));
|
||||
|
||||
// Act
|
||||
sut.setupUnlockListenersForProcessingAuthRequests(destroy$);
|
||||
activeAccount$.next({
|
||||
id: otherUserId,
|
||||
email: "other@example.com",
|
||||
emailVerified: true,
|
||||
name: "Other",
|
||||
});
|
||||
|
||||
// Assert
|
||||
await new Promise((resolve) => setTimeout(resolve, 0));
|
||||
expect(messagingService.send).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe("authentication status transitions", () => {
|
||||
@@ -380,6 +403,24 @@ describe("DefaultAuthRequestAnsweringService", () => {
|
||||
expect(messagingService.send).toHaveBeenCalledTimes(2);
|
||||
expect(messagingService.send).toHaveBeenCalledWith("openLoginApproval");
|
||||
});
|
||||
|
||||
it("should NOT process pending auth requests when active account transitions to Unlocked but is required to set/change their master password", async () => {
|
||||
// Arrange
|
||||
masterPasswordService.forceSetPasswordReason$.mockReturnValue(
|
||||
of(ForceSetPasswordReason.WeakMasterPassword),
|
||||
);
|
||||
activeAccountStatus$.next(AuthenticationStatus.Locked);
|
||||
pendingRequestMarkers = [{ userId, receivedAtMs: Date.now() }];
|
||||
pendingAuthRequestsState.getAll$.mockReturnValue(of(pendingRequestMarkers));
|
||||
|
||||
// Act
|
||||
sut.setupUnlockListenersForProcessingAuthRequests(destroy$);
|
||||
activeAccountStatus$.next(AuthenticationStatus.Unlocked);
|
||||
|
||||
// Assert
|
||||
await new Promise((resolve) => setTimeout(resolve, 0));
|
||||
expect(messagingService.send).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe("subscription cleanup", () => {
|
||||
|
||||
@@ -103,6 +103,16 @@ export class DefaultAuthRequestAnsweringService implements AuthRequestAnsweringS
|
||||
* approval dialog.
|
||||
*/
|
||||
private async processPendingAuthRequests(): Promise<void> {
|
||||
const activeUserId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId));
|
||||
|
||||
// Only continue if the active user is not required to set/change their master password
|
||||
const forceSetPasswordReason = await firstValueFrom(
|
||||
this.masterPasswordService.forceSetPasswordReason$(activeUserId),
|
||||
);
|
||||
if (forceSetPasswordReason !== ForceSetPasswordReason.None) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Prune any stale pending requests (older than 15 minutes)
|
||||
// This comes from GlobalSettings.cs
|
||||
// public TimeSpan UserRequestExpiration { get; set; } = TimeSpan.FromMinutes(15);
|
||||
@@ -114,7 +124,6 @@ export class DefaultAuthRequestAnsweringService implements AuthRequestAnsweringS
|
||||
(await firstValueFrom(this.pendingAuthRequestsState.getAll$())) ?? [];
|
||||
|
||||
if (pendingAuthRequestsInState.length > 0) {
|
||||
const activeUserId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId));
|
||||
const pendingAuthRequestsForActiveUser = pendingAuthRequestsInState.some(
|
||||
(e) => e.userId === activeUserId,
|
||||
);
|
||||
|
||||
Reference in New Issue
Block a user