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 18 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
43 changes: 27 additions & 16 deletions apps/browser/src/autofill/background/notification.background.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,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 @@ -262,7 +263,8 @@
return;
}

const ciphers = await this.cipherService.getAllDecryptedForUrl(loginInfo.url);
const activeUserId = await firstValueFrom(this.activeUserId$);
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 @@ -340,7 +342,8 @@
}

let id: string = null;
const ciphers = await this.cipherService.getAllDecryptedForUrl(changeData.url);
const activeUserId = await firstValueFrom(this.activeUserId$);
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 @@ -542,15 +545,20 @@

this.notificationQueue.splice(i, 1);

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

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 @@ -566,13 +574,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 @@ -604,14 +610,15 @@
) {
cipherView.login.password = newPassword;

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

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 @@ -629,13 +636,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 @@ -649,8 +660,8 @@
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 664 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#L664

Added line #L664 was not covered by tests
if (cipher != null && cipher.type === CipherType.Login) {
const activeUserId = await firstValueFrom(this.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
28 changes: 21 additions & 7 deletions apps/browser/src/autofill/background/overlay.background.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ 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 {
Expand Down Expand Up @@ -225,6 +226,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 +411,12 @@ 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(
this.accountService.activeAccount$.pipe(map((a) => a?.id)),
);
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 +434,11 @@ export class OverlayBackground implements OverlayBackgroundInterface {
}

this.cardAndIdentityCiphers.clear();
const activeUserId = await firstValueFrom(
this.accountService.activeAccount$.pipe(map((a) => a?.id)),
);
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 +2407,16 @@ export class OverlayBackground implements OverlayBackgroundInterface {

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

await this.openAddEditVaultItemPopout(sender.tab, {
cipherId: cipherView.id,
Expand Down
12 changes: 11 additions & 1 deletion apps/browser/src/autofill/background/web-request.background.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
// FIXME: Update this file to be type safe and remove this and next line
// @ts-strict-ignore
import { firstValueFrom, map } 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 { UriMatchStrategy } from "@bitwarden/common/models/domain/domain-service";
Expand All @@ -14,6 +17,7 @@
platformUtilsService: PlatformUtilsService,
private cipherService: CipherService,
private authService: AuthService,
private accountService: AccountService,

Check warning on line 20 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#L20

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

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

Check warning on line 62 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#L62

Added line #L62 was not covered by tests
this.accountService.activeAccount$.pipe(map((a) => a?.id)),
);

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

Check warning on line 66 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#L66

Added line #L66 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
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
import { firstValueFrom, map } from "rxjs";

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 { Utils } from "@bitwarden/common/platform/misc/utils";
Expand All @@ -14,6 +17,7 @@
private mainContextMenuHandler: MainContextMenuHandler,
private authService: AuthService,
private cipherService: CipherService,
private accountService: AccountService,
) {}

async update(url: string) {
Expand All @@ -35,7 +39,16 @@
return;
}

const ciphers = await this.cipherService.getAllDecryptedForUrl(url, [
const activeUserId = await firstValueFrom(
this.accountService.activeAccount$.pipe(map((a) => a?.id)),
);

if (!activeUserId) {
// Show error be thrown here or is it okay to just return?
return;

Check warning on line 48 in apps/browser/src/autofill/browser/cipher-context-menu-handler.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/autofill/browser/cipher-context-menu-handler.ts#L48

Added line #L48 was not covered by tests
}

const ciphers = await this.cipherService.getAllDecryptedForUrl(url, activeUserId, [
CipherType.Card,
CipherType.Identity,
]);
Expand Down
Loading
Loading