Skip to content

Commit 55177c4

Browse files
shane-meltonvincentsalucci
shane-melton
authored and
vincentsalucci
committed
[PM-21041] Fix cipher view security tasks fetching (#14569)
* [PM-21041] Add taskEnabled$ dependency to tasks$ observable * [PM-21041] Rework cipher view component to only check tasks for organization Login type ciphers - Remove dependency on feature flag check (handled by tasks$ observable now) - Add try/catch in case of request failures to avoid breaking component initialization * [PM-21041] Remove now redundant taskEnabled$ chain * [PM-21041] Fix tests
1 parent 2cb810c commit 55177c4

File tree

6 files changed

+146
-72
lines changed

6 files changed

+146
-72
lines changed

‎apps/browser/src/vault/popup/components/at-risk-callout/at-risk-password-callout.component.ts

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { CommonModule } from "@angular/common";
22
import { Component, inject } from "@angular/core";
33
import { RouterModule } from "@angular/router";
4-
import { map, of, switchMap } from "rxjs";
4+
import { map, switchMap } from "rxjs";
55

66
import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
77
import { getUserId } from "@bitwarden/common/auth/services/account.service";
@@ -20,21 +20,7 @@ export class AtRiskPasswordCalloutComponent {
2020
private activeAccount$ = inject(AccountService).activeAccount$.pipe(getUserId);
2121

2222
protected pendingTasks$ = this.activeAccount$.pipe(
23-
switchMap((userId) =>
24-
this.taskService.tasksEnabled$(userId).pipe(
25-
switchMap((enabled) => {
26-
if (!enabled) {
27-
return of([]);
28-
}
29-
return this.taskService
30-
.pendingTasks$(userId)
31-
.pipe(
32-
map((tasks) =>
33-
tasks.filter((t) => t.type === SecurityTaskType.UpdateAtRiskCredential),
34-
),
35-
);
36-
}),
37-
),
38-
),
23+
switchMap((userId) => this.taskService.pendingTasks$(userId)),
24+
map((tasks) => tasks.filter((t) => t.type === SecurityTaskType.UpdateAtRiskCredential)),
3925
);
4026
}

‎apps/web/src/app/auth/settings/emergency-access/view/emergency-view-dialog.component.spec.ts

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,16 @@ import { OrganizationService } from "@bitwarden/common/admin-console/abstraction
88
import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
99
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
1010
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
11+
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
1112
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
1213
import { Utils } from "@bitwarden/common/platform/misc/utils";
1314
import { FakeAccountService, mockAccountServiceWith } from "@bitwarden/common/spec";
14-
import { UserId } from "@bitwarden/common/types/guid";
15+
import { UserId, EmergencyAccessId } from "@bitwarden/common/types/guid";
1516
import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service";
1617
import { FolderService } from "@bitwarden/common/vault/abstractions/folder/folder.service.abstraction";
1718
import { CipherType } from "@bitwarden/common/vault/enums";
1819
import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view";
20+
import { LoginView } from "@bitwarden/common/vault/models/view/login.view";
1921
import { TaskService } from "@bitwarden/common/vault/tasks";
2022
import { DialogService, DialogRef, DIALOG_DATA } from "@bitwarden/components";
2123
import { ChangeLoginPasswordService } from "@bitwarden/vault";
@@ -28,14 +30,15 @@ describe("EmergencyViewDialogComponent", () => {
2830

2931
const open = jest.fn();
3032
const close = jest.fn();
33+
const emergencyAccessId = "emergency-access-id" as EmergencyAccessId;
3134

3235
const mockCipher = {
3336
id: "cipher1",
3437
name: "Cipher",
3538
type: CipherType.Login,
36-
login: { uris: [] },
39+
login: { uris: [] } as Partial<LoginView>,
3740
card: {},
38-
} as CipherView;
41+
} as Partial<CipherView> as CipherView;
3942

4043
const accountService: FakeAccountService = mockAccountServiceWith(Utils.newGuid() as UserId);
4144

@@ -56,6 +59,7 @@ describe("EmergencyViewDialogComponent", () => {
5659
{ provide: DIALOG_DATA, useValue: { cipher: mockCipher } },
5760
{ provide: AccountService, useValue: accountService },
5861
{ provide: TaskService, useValue: mock<TaskService>() },
62+
{ provide: LogService, useValue: mock<LogService>() },
5963
],
6064
})
6165
.overrideComponent(EmergencyViewDialogComponent, {
@@ -94,18 +98,24 @@ describe("EmergencyViewDialogComponent", () => {
9498
});
9599

96100
it("opens dialog", () => {
97-
EmergencyViewDialogComponent.open({ open } as unknown as DialogService, { cipher: mockCipher });
101+
EmergencyViewDialogComponent.open({ open } as unknown as DialogService, {
102+
cipher: mockCipher,
103+
emergencyAccessId,
104+
});
98105

99106
expect(open).toHaveBeenCalled();
100107
});
101108

102109
it("closes the dialog", () => {
103-
EmergencyViewDialogComponent.open({ open } as unknown as DialogService, { cipher: mockCipher });
110+
EmergencyViewDialogComponent.open({ open } as unknown as DialogService, {
111+
cipher: mockCipher,
112+
emergencyAccessId,
113+
});
104114
fixture.detectChanges();
105115

106116
const cancelButton = fixture.debugElement.queryAll(By.css("button")).pop();
107117

108-
cancelButton.nativeElement.click();
118+
cancelButton!.nativeElement.click();
109119

110120
expect(close).toHaveBeenCalled();
111121
});

‎libs/common/src/vault/tasks/services/default-task.service.spec.ts

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,34 @@ describe("Default task service", () => {
108108
});
109109

110110
describe("tasks$", () => {
111+
beforeEach(() => {
112+
mockGetFeatureFlag$.mockReturnValue(new BehaviorSubject(true));
113+
mockGetAllOrgs$.mockReturnValue(
114+
new BehaviorSubject([
115+
{
116+
useRiskInsights: true,
117+
},
118+
] as Organization[]),
119+
);
120+
});
121+
122+
it("should return an empty array if tasks are not enabled", async () => {
123+
mockGetAllOrgs$.mockReturnValue(
124+
new BehaviorSubject([
125+
{
126+
useRiskInsights: false,
127+
},
128+
] as Organization[]),
129+
);
130+
131+
const { tasks$ } = service;
132+
133+
const result = await firstValueFrom(tasks$("user-id" as UserId));
134+
135+
expect(result.length).toBe(0);
136+
expect(mockApiSend).not.toHaveBeenCalled();
137+
});
138+
111139
it("should fetch tasks from the API when the state is null", async () => {
112140
mockApiSend.mockResolvedValue({
113141
data: [
@@ -153,6 +181,34 @@ describe("Default task service", () => {
153181
});
154182

155183
describe("pendingTasks$", () => {
184+
beforeEach(() => {
185+
mockGetFeatureFlag$.mockReturnValue(new BehaviorSubject(true));
186+
mockGetAllOrgs$.mockReturnValue(
187+
new BehaviorSubject([
188+
{
189+
useRiskInsights: true,
190+
},
191+
] as Organization[]),
192+
);
193+
});
194+
195+
it("should return an empty array if tasks are not enabled", async () => {
196+
mockGetAllOrgs$.mockReturnValue(
197+
new BehaviorSubject([
198+
{
199+
useRiskInsights: false,
200+
},
201+
] as Organization[]),
202+
);
203+
204+
const { pendingTasks$ } = service;
205+
206+
const result = await firstValueFrom(pendingTasks$("user-id" as UserId));
207+
208+
expect(result.length).toBe(0);
209+
expect(mockApiSend).not.toHaveBeenCalled();
210+
});
211+
156212
it("should filter tasks to only pending tasks", async () => {
157213
fakeStateProvider.singleUser.mockFor("user-id" as UserId, SECURITY_TASKS, [
158214
{

‎libs/common/src/vault/tasks/services/default-task.service.ts

Lines changed: 30 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,14 @@
1-
import { combineLatest, filter, map, merge, Observable, of, Subscription, switchMap } from "rxjs";
1+
import {
2+
combineLatest,
3+
filter,
4+
map,
5+
merge,
6+
Observable,
7+
of,
8+
Subscription,
9+
switchMap,
10+
distinctUntilChanged,
11+
} from "rxjs";
212

313
import { ApiService } from "@bitwarden/common/abstractions/api.service";
414
import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction";
@@ -45,20 +55,30 @@ export class DefaultTaskService implements TaskService {
4555
.organizations$(userId)
4656
.pipe(map((orgs) => orgs.some((o) => o.useRiskInsights))),
4757
this.configService.getFeatureFlag$(FeatureFlag.SecurityTasks),
48-
]).pipe(map(([atLeastOneOrgEnabled, flagEnabled]) => atLeastOneOrgEnabled && flagEnabled));
58+
]).pipe(
59+
map(([atLeastOneOrgEnabled, flagEnabled]) => atLeastOneOrgEnabled && flagEnabled),
60+
distinctUntilChanged(),
61+
);
4962
});
5063

5164
tasks$ = perUserCache$((userId) => {
52-
return this.taskState(userId).state$.pipe(
53-
switchMap(async (tasks) => {
54-
if (tasks == null) {
55-
await this.fetchTasksFromApi(userId);
56-
return null;
65+
return this.tasksEnabled$(userId).pipe(
66+
switchMap((enabled) => {
67+
if (!enabled) {
68+
return of([]);
5769
}
58-
return tasks;
70+
return this.taskState(userId).state$.pipe(
71+
switchMap(async (tasks) => {
72+
if (tasks == null) {
73+
await this.fetchTasksFromApi(userId);
74+
return null;
75+
}
76+
return tasks;
77+
}),
78+
filterOutNullish(),
79+
map((tasks) => tasks.map((t) => new SecurityTask(t))),
80+
);
5981
}),
60-
filterOutNullish(),
61-
map((tasks) => tasks.map((t) => new SecurityTask(t))),
6282
);
6383
});
6484

‎libs/vault/src/cipher-view/cipher-view.component.html

Lines changed: 13 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3,19 +3,18 @@
33
{{ "cardExpiredMessage" | i18n }}
44
</bit-callout>
55

6-
<ng-container *ngIf="isSecurityTasksEnabled$ | async">
7-
<bit-callout
8-
*ngIf="cipher?.login.uris.length > 0 && hadPendingChangePasswordTask"
9-
type="warning"
10-
[title]="''"
11-
>
12-
<i class="bwi bwi-exclamation-triangle tw-text-warning" aria-hidden="true"></i>
13-
<a bitLink href="#" appStopClick (click)="launchChangePassword()">
14-
{{ "changeAtRiskPassword" | i18n }}
15-
<i class="bwi bwi-external-link tw-ml-1" aria-hidden="true"></i>
16-
</a>
17-
</bit-callout>
18-
</ng-container>
6+
<bit-callout
7+
*ngIf="cipher?.login.uris.length > 0 && hadPendingChangePasswordTask"
8+
type="warning"
9+
[title]="''"
10+
>
11+
<i class="bwi bwi-exclamation-triangle tw-text-warning" aria-hidden="true"></i>
12+
<a bitLink href="#" appStopClick (click)="launchChangePassword()">
13+
{{ "changeAtRiskPassword" | i18n }}
14+
<i class="bwi bwi-external-link tw-ml-1" aria-hidden="true"></i>
15+
</a>
16+
</bit-callout>
17+
1918
<!-- HELPER TEXT -->
2019
<p
2120
class="tw-text-sm tw-text-muted"
@@ -40,9 +39,7 @@
4039
*ngIf="hasLogin"
4140
[cipher]="cipher"
4241
[activeUserId]="activeUserId$ | async"
43-
[hadPendingChangePasswordTask]="
44-
hadPendingChangePasswordTask && (isSecurityTasksEnabled$ | async)
45-
"
42+
[hadPendingChangePasswordTask]="hadPendingChangePasswordTask"
4643
(handleChangePassword)="launchChangePassword()"
4744
></app-login-credentials-view>
4845

‎libs/vault/src/cipher-view/cipher-view.component.ts

Lines changed: 28 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,12 @@ import { Organization } from "@bitwarden/common/admin-console/models/domain/orga
1212
import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
1313
import { getUserId } from "@bitwarden/common/auth/services/account.service";
1414
import { isCardExpired } from "@bitwarden/common/autofill/utils";
15-
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
16-
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
15+
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
1716
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
1817
import { CipherId, CollectionId, EmergencyAccessId, UserId } from "@bitwarden/common/types/guid";
1918
import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service";
2019
import { FolderService } from "@bitwarden/common/vault/abstractions/folder/folder.service.abstraction";
20+
import { CipherType } from "@bitwarden/common/vault/enums";
2121
import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view";
2222
import { FolderView } from "@bitwarden/common/vault/models/view/folder.view";
2323
import { SecurityTaskType, TaskService } from "@bitwarden/common/vault/tasks";
@@ -80,7 +80,6 @@ export class CipherViewComponent implements OnChanges, OnDestroy {
8080
private destroyed$: Subject<void> = new Subject();
8181
cardIsExpired: boolean = false;
8282
hadPendingChangePasswordTask: boolean = false;
83-
isSecurityTasksEnabled$ = this.configService.getFeatureFlag$(FeatureFlag.SecurityTasks);
8483

8584
constructor(
8685
private organizationService: OrganizationService,
@@ -90,8 +89,8 @@ export class CipherViewComponent implements OnChanges, OnDestroy {
9089
private defaultTaskService: TaskService,
9190
private platformUtilsService: PlatformUtilsService,
9291
private changeLoginPasswordService: ChangeLoginPasswordService,
93-
private configService: ConfigService,
9492
private cipherService: CipherService,
93+
private logService: LogService,
9594
) {}
9695

9796
async ngOnChanges() {
@@ -157,20 +156,15 @@ export class CipherViewComponent implements OnChanges, OnDestroy {
157156

158157
const userId = await firstValueFrom(this.activeUserId$);
159158

160-
// Show Tasks for Manage and Edit permissions
161-
// Using cipherService to see if user has access to cipher in a non-AC context to address with Edit Except Password permissions
162-
const allCiphers = await firstValueFrom(this.cipherService.ciphers$(userId));
163-
const cipherServiceCipher = allCiphers[this.cipher?.id as CipherId];
164-
165-
if (cipherServiceCipher?.edit && cipherServiceCipher?.viewPassword) {
166-
await this.checkPendingChangePasswordTasks(userId);
167-
}
168-
169-
if (this.cipher.organizationId && userId) {
159+
if (this.cipher.organizationId) {
170160
this.organization$ = this.organizationService
171161
.organizations$(userId)
172162
.pipe(getOrganizationById(this.cipher.organizationId))
173163
.pipe(takeUntil(this.destroyed$));
164+
165+
if (this.cipher.type === CipherType.Login) {
166+
await this.checkPendingChangePasswordTasks(userId);
167+
}
174168
}
175169

176170
if (this.cipher.folderId) {
@@ -181,17 +175,28 @@ export class CipherViewComponent implements OnChanges, OnDestroy {
181175
}
182176

183177
async checkPendingChangePasswordTasks(userId: UserId): Promise<void> {
184-
if (!(await firstValueFrom(this.isSecurityTasksEnabled$))) {
185-
return;
186-
}
178+
try {
179+
// Show Tasks for Manage and Edit permissions
180+
// Using cipherService to see if user has access to cipher in a non-AC context to address with Edit Except Password permissions
181+
const allCiphers = await firstValueFrom(this.cipherService.ciphers$(userId));
182+
const cipherServiceCipher = allCiphers[this.cipher?.id as CipherId];
183+
184+
if (!cipherServiceCipher?.edit || !cipherServiceCipher?.viewPassword) {
185+
this.hadPendingChangePasswordTask = false;
186+
return;
187+
}
187188

188-
const tasks = await firstValueFrom(this.defaultTaskService.pendingTasks$(userId));
189+
const tasks = await firstValueFrom(this.defaultTaskService.pendingTasks$(userId));
189190

190-
this.hadPendingChangePasswordTask = tasks?.some((task) => {
191-
return (
192-
task.cipherId === this.cipher?.id && task.type === SecurityTaskType.UpdateAtRiskCredential
193-
);
194-
});
191+
this.hadPendingChangePasswordTask = tasks?.some((task) => {
192+
return (
193+
task.cipherId === this.cipher?.id && task.type === SecurityTaskType.UpdateAtRiskCredential
194+
);
195+
});
196+
} catch (error) {
197+
this.hadPendingChangePasswordTask = false;
198+
this.logService.error("Failed to retrieve change password tasks for cipher", error);
199+
}
195200
}
196201

197202
launchChangePassword = async () => {

0 commit comments

Comments
 (0)