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-12047] Remove usage of ActiveUserState from cipher.service #12814

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open
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 @@ -904,10 +904,13 @@ describe("NotificationBackground", () => {
expect(tabSendMessageSpy).toHaveBeenCalledWith(sender.tab, {
command: "editedCipher",
});
expect(setAddEditCipherInfoSpy).toHaveBeenCalledWith({
cipher: cipherView,
collectionIds: cipherView.collectionIds,
});
expect(setAddEditCipherInfoSpy).toHaveBeenCalledWith(
{
cipher: cipherView,
collectionIds: cipherView.collectionIds,
},
"testId",
);
expect(openAddEditVaultItemPopoutSpy).toHaveBeenCalledWith(sender.tab, {
cipherId: cipherView.id,
});
Expand Down Expand Up @@ -945,7 +948,7 @@ describe("NotificationBackground", () => {
queueMessage,
message.folder,
);
expect(editItemSpy).toHaveBeenCalledWith(cipherView, sender.tab);
expect(editItemSpy).toHaveBeenCalledWith(cipherView, "testId", sender.tab);
expect(tabSendMessageSpy).toHaveBeenCalledWith(sender.tab, {
command: "closeNotificationBar",
});
Expand Down
54 changes: 32 additions & 22 deletions apps/browser/src/autofill/background/notification.background.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
// FIXME: Update this file to be type safe and remove this and next line
// @ts-strict-ignore
import { firstValueFrom, map } from "rxjs";
import { firstValueFrom } from "rxjs";

import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction";
import { PolicyType } from "@bitwarden/common/admin-console/enums";
import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status";
import { getUserId } from "@bitwarden/common/auth/services/account.service";
import {
ExtensionCommand,
ExtensionCommandType,
Expand All @@ -21,6 +22,7 @@
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
import { Utils } from "@bitwarden/common/platform/misc/utils";
import { ThemeStateService } from "@bitwarden/common/platform/theming/theme-state.service";
import { UserId } from "@bitwarden/common/types/guid";
import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service";
import { FolderService } from "@bitwarden/common/vault/abstractions/folder/folder.service.abstraction";
import { CipherType } from "@bitwarden/common/vault/enums";
Expand Down Expand Up @@ -82,8 +84,6 @@
getWebVaultUrlForNotification: () => this.getWebVaultUrl(),
};

private activeUserId$ = this.accountService.activeAccount$.pipe(map((a) => a?.id));

constructor(
private autofillService: AutofillService,
private cipherService: CipherService,
Expand Down Expand Up @@ -256,7 +256,8 @@
return;
}

const ciphers = await this.cipherService.getAllDecryptedForUrl(loginInfo.url);
const activeUserId = await firstValueFrom(getUserId(this.accountService.activeAccount$));
const ciphers = await this.cipherService.getAllDecryptedForUrl(loginInfo.url, activeUserId);
const usernameMatches = ciphers.filter(
(c) => c.login.username != null && c.login.username.toLowerCase() === normalizedUsername,
);
Expand Down Expand Up @@ -334,7 +335,8 @@
}

let id: string = null;
const ciphers = await this.cipherService.getAllDecryptedForUrl(changeData.url);
const activeUserId = await firstValueFrom(getUserId(this.accountService.activeAccount$));
const ciphers = await this.cipherService.getAllDecryptedForUrl(changeData.url, activeUserId);
if (changeData.currentPassword != null) {
const passwordMatches = ciphers.filter(
(c) => c.login.password === changeData.currentPassword,
Expand Down Expand Up @@ -487,15 +489,20 @@

this.notificationQueue.splice(i, 1);

const activeUserId = await firstValueFrom(getUserId(this.accountService.activeAccount$));

if (queueMessage.type === NotificationQueueMessageType.ChangePassword) {
const cipherView = await this.getDecryptedCipherById(queueMessage.cipherId);
const cipherView = await this.getDecryptedCipherById(queueMessage.cipherId, activeUserId);
await this.updatePassword(cipherView, queueMessage.newPassword, edit, tab);
return;
}

// If the vault was locked, check if a cipher needs updating instead of creating a new one
if (queueMessage.wasVaultLocked) {
const allCiphers = await this.cipherService.getAllDecryptedForUrl(queueMessage.uri);
const allCiphers = await this.cipherService.getAllDecryptedForUrl(
queueMessage.uri,
activeUserId,
);
const existingCipher = allCiphers.find(
(c) =>
c.login.username != null && c.login.username.toLowerCase() === queueMessage.username,
Expand All @@ -511,13 +518,11 @@
const newCipher = this.convertAddLoginQueueMessageToCipherView(queueMessage, folderId);

if (edit) {
await this.editItem(newCipher, tab);
await this.editItem(newCipher, activeUserId, tab);
await BrowserApi.tabSendMessage(tab, { command: "closeNotificationBar" });
return;
}

const activeUserId = await firstValueFrom(this.activeUserId$);

const cipher = await this.cipherService.encrypt(newCipher, activeUserId);
try {
await this.cipherService.createWithServer(cipher);
Expand Down Expand Up @@ -549,14 +554,15 @@
) {
cipherView.login.password = newPassword;

const activeUserId = await firstValueFrom(getUserId(this.accountService.activeAccount$));

if (edit) {
await this.editItem(cipherView, tab);
await this.editItem(cipherView, activeUserId, tab);
await BrowserApi.tabSendMessage(tab, { command: "closeNotificationBar" });
await BrowserApi.tabSendMessage(tab, { command: "editedCipher" });
return;
}

const activeUserId = await firstValueFrom(this.activeUserId$);
const cipher = await this.cipherService.encrypt(cipherView, activeUserId);
try {
// We've only updated the password, no need to broadcast editedCipher message
Expand All @@ -574,13 +580,17 @@
* and opens the add/edit vault item popout.
*
* @param cipherView - The cipher to edit
* @param userId - The active account user ID
* @param senderTab - The tab that the message was sent from
*/
private async editItem(cipherView: CipherView, senderTab: chrome.tabs.Tab) {
await this.cipherService.setAddEditCipherInfo({
cipher: cipherView,
collectionIds: cipherView.collectionIds,
});
private async editItem(cipherView: CipherView, userId: UserId, senderTab: chrome.tabs.Tab) {
await this.cipherService.setAddEditCipherInfo(
{
cipher: cipherView,
collectionIds: cipherView.collectionIds,
},
userId,
);

await this.openAddEditVaultItemPopout(senderTab, { cipherId: cipherView.id });
}
Expand All @@ -589,15 +599,15 @@
if (Utils.isNullOrWhitespace(folderId) || folderId === "null") {
return false;
}
const activeUserId = await firstValueFrom(this.activeUserId$);
const activeUserId = await firstValueFrom(getUserId(this.accountService.activeAccount$));

Check warning on line 602 in apps/browser/src/autofill/background/notification.background.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/autofill/background/notification.background.ts#L602

Added line #L602 was not covered by tests
const folders = await firstValueFrom(this.folderService.folderViews$(activeUserId));
return folders.some((x) => x.id === folderId);
}

private async getDecryptedCipherById(cipherId: string) {
const cipher = await this.cipherService.get(cipherId);
private async getDecryptedCipherById(cipherId: string, userId: UserId) {
const cipher = await this.cipherService.get(cipherId, userId);

Check warning on line 608 in apps/browser/src/autofill/background/notification.background.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/autofill/background/notification.background.ts#L608

Added line #L608 was not covered by tests
if (cipher != null && cipher.type === CipherType.Login) {
const activeUserId = await firstValueFrom(this.activeUserId$);
const activeUserId = await firstValueFrom(getUserId(this.accountService.activeAccount$));

Check warning on line 610 in apps/browser/src/autofill/background/notification.background.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/autofill/background/notification.background.ts#L610

Added line #L610 was not covered by tests

return await cipher.decrypt(
await this.cipherService.getKeyForCipherKeyDecryption(cipher, activeUserId),
Expand Down Expand Up @@ -637,7 +647,7 @@
* Returns the first value found from the folder service's folderViews$ observable.
*/
private async getFolderData() {
const activeUserId = await firstValueFrom(this.activeUserId$);
const activeUserId = await firstValueFrom(getUserId(this.accountService.activeAccount$));
return await firstValueFrom(this.folderService.folderViews$(activeUserId));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ describe("OverlayBackground", () => {
inlineMenuFieldQualificationService,
themeStateService,
totpService,
accountService,
generatedPasswordCallbackMock,
addPasswordCallbackMock,
);
Expand Down Expand Up @@ -849,7 +850,7 @@ describe("OverlayBackground", () => {
await flushPromises();

expect(BrowserApi.getTabFromCurrentWindowId).toHaveBeenCalled();
expect(cipherService.getAllDecryptedForUrl).toHaveBeenCalledWith(url, [
expect(cipherService.getAllDecryptedForUrl).toHaveBeenCalledWith(url, mockUserId, [
CipherType.Card,
CipherType.Identity,
]);
Expand All @@ -872,7 +873,7 @@ describe("OverlayBackground", () => {
await flushPromises();

expect(BrowserApi.getTabFromCurrentWindowId).toHaveBeenCalled();
expect(cipherService.getAllDecryptedForUrl).toHaveBeenCalledWith(url);
expect(cipherService.getAllDecryptedForUrl).toHaveBeenCalledWith(url, mockUserId);
expect(cipherService.sortCiphersByLastUsedThenName).toHaveBeenCalled();
expect(overlayBackground["inlineMenuCiphers"]).toStrictEqual(
new Map([
Expand All @@ -891,7 +892,7 @@ describe("OverlayBackground", () => {
await flushPromises();

expect(BrowserApi.getTabFromCurrentWindowId).toHaveBeenCalled();
expect(cipherService.getAllDecryptedForUrl).toHaveBeenCalledWith(url, [
expect(cipherService.getAllDecryptedForUrl).toHaveBeenCalledWith(url, mockUserId, [
CipherType.Card,
CipherType.Identity,
]);
Expand Down
25 changes: 17 additions & 8 deletions apps/browser/src/autofill/background/overlay.background.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,10 @@ import {
} from "rxjs";
import { parse } from "tldts";

import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status";
import { getUserId } from "@bitwarden/common/auth/services/account.service";
import {
AutofillOverlayVisibility,
SHOW_AUTOFILL_BUTTON,
Expand Down Expand Up @@ -225,6 +227,7 @@ export class OverlayBackground implements OverlayBackgroundInterface {
private inlineMenuFieldQualificationService: InlineMenuFieldQualificationService,
private themeStateService: ThemeStateService,
private totpService: TotpService,
private accountService: AccountService,
private generatePasswordCallback: () => Promise<string>,
private addPasswordCallback: (password: string) => Promise<void>,
) {
Expand Down Expand Up @@ -409,9 +412,10 @@ export class OverlayBackground implements OverlayBackgroundInterface {
return this.getAllCipherTypeViews(currentTab);
}

const cipherViews = (await this.cipherService.getAllDecryptedForUrl(currentTab.url || "")).sort(
(a, b) => this.cipherService.sortCiphersByLastUsedThenName(a, b),
);
const activeUserId = await firstValueFrom(getUserId(this.accountService.activeAccount$));
const cipherViews = (
await this.cipherService.getAllDecryptedForUrl(currentTab.url || "", activeUserId)
).sort((a, b) => this.cipherService.sortCiphersByLastUsedThenName(a, b));

return this.cardAndIdentityCiphers
? cipherViews.concat(...this.cardAndIdentityCiphers)
Expand All @@ -429,8 +433,9 @@ export class OverlayBackground implements OverlayBackgroundInterface {
}

this.cardAndIdentityCiphers.clear();
const activeUserId = await firstValueFrom(getUserId(this.accountService.activeAccount$));
const cipherViews = (
await this.cipherService.getAllDecryptedForUrl(currentTab.url || "", [
await this.cipherService.getAllDecryptedForUrl(currentTab.url || "", activeUserId, [
CipherType.Card,
CipherType.Identity,
])
Expand Down Expand Up @@ -2399,10 +2404,14 @@ export class OverlayBackground implements OverlayBackgroundInterface {

try {
this.closeInlineMenu(sender);
await this.cipherService.setAddEditCipherInfo({
cipher: cipherView,
collectionIds: cipherView.collectionIds,
});
const activeUserId = await firstValueFrom(getUserId(this.accountService.activeAccount$));
await this.cipherService.setAddEditCipherInfo(
{
cipher: cipherView,
collectionIds: cipherView.collectionIds,
},
activeUserId,
);

await this.openAddEditVaultItemPopout(sender.tab, {
cipherId: cipherView.id,
Expand Down
11 changes: 10 additions & 1 deletion apps/browser/src/autofill/background/web-request.background.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
// FIXME: Update this file to be type safe and remove this and next line
// @ts-strict-ignore
import { firstValueFrom } from "rxjs";

Check warning on line 3 in apps/browser/src/autofill/background/web-request.background.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/autofill/background/web-request.background.ts#L3

Added line #L3 was not covered by tests

import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status";
import { getUserId } from "@bitwarden/common/auth/services/account.service";

Check warning on line 8 in apps/browser/src/autofill/background/web-request.background.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/autofill/background/web-request.background.ts#L8

Added line #L8 was not covered by tests
import { UriMatchStrategy } from "@bitwarden/common/models/domain/domain-service";
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service";
Expand All @@ -14,6 +18,7 @@
platformUtilsService: PlatformUtilsService,
private cipherService: CipherService,
private authService: AuthService,
private accountService: AccountService,

Check warning on line 21 in apps/browser/src/autofill/background/web-request.background.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/autofill/background/web-request.background.ts#L21

Added line #L21 was not covered by tests
private readonly webRequest: typeof chrome.webRequest,
) {
this.isFirefox = platformUtilsService.isFirefox();
Expand Down Expand Up @@ -55,14 +60,18 @@

// eslint-disable-next-line
private async resolveAuthCredentials(domain: string, success: Function, error: Function) {
if ((await this.authService.getAuthStatus()) < AuthenticationStatus.Unlocked) {
const activeUserId = await firstValueFrom(getUserId(this.accountService.activeAccount$));

Check warning on line 63 in apps/browser/src/autofill/background/web-request.background.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/autofill/background/web-request.background.ts#L63

Added line #L63 was not covered by tests

const authStatus = await firstValueFrom(this.authService.authStatusFor$(activeUserId));

Check warning on line 65 in apps/browser/src/autofill/background/web-request.background.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/autofill/background/web-request.background.ts#L65

Added line #L65 was not covered by tests
if (authStatus < AuthenticationStatus.Unlocked) {
error();
return;
}

try {
const ciphers = await this.cipherService.getAllDecryptedForUrl(
domain,
activeUserId,
null,
UriMatchStrategy.Host,
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import { mock, MockProxy } from "jest-mock-extended";

import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status";
import { mockAccountServiceWith } from "@bitwarden/common/spec";
import { UserId } from "@bitwarden/common/types/guid";
import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service";
import { CipherType } from "@bitwarden/common/vault/enums";
import { CipherRepromptType } from "@bitwarden/common/vault/enums/cipher-reprompt-type";
Expand All @@ -14,6 +16,9 @@ describe("CipherContextMenuHandler", () => {
let authService: MockProxy<AuthService>;
let cipherService: MockProxy<CipherService>;

const mockUserId = "UserId" as UserId;
const accountService = mockAccountServiceWith(mockUserId);

let sut: CipherContextMenuHandler;

beforeEach(() => {
Expand All @@ -24,7 +29,12 @@ describe("CipherContextMenuHandler", () => {

jest.spyOn(MainContextMenuHandler, "removeAll").mockResolvedValue();

sut = new CipherContextMenuHandler(mainContextMenuHandler, authService, cipherService);
sut = new CipherContextMenuHandler(
mainContextMenuHandler,
authService,
cipherService,
accountService,
);
});

afterEach(() => jest.resetAllMocks());
Expand Down Expand Up @@ -119,10 +129,11 @@ describe("CipherContextMenuHandler", () => {

expect(cipherService.getAllDecryptedForUrl).toHaveBeenCalledTimes(1);

expect(cipherService.getAllDecryptedForUrl).toHaveBeenCalledWith("https://test.com", [
CipherType.Card,
CipherType.Identity,
]);
expect(cipherService.getAllDecryptedForUrl).toHaveBeenCalledWith(
"https://test.com",
mockUserId,
[CipherType.Card, CipherType.Identity],
);

expect(mainContextMenuHandler.loadOptions).toHaveBeenCalledTimes(3);

Expand Down
Loading
Loading