mirror of
https://github.com/bitwarden/browser
synced 2025-12-13 23:03:32 +00:00
Fix stale data issue in new login popout (#17307)
* Fix stale data issue in new login popout * Update the comments * Address critical claude code bot suggestions * Clean out all stale data from pop up * Fix cached cipher issue * Fix caching issue between tab and overlay flow * Address claude comments
This commit is contained in:
@@ -381,4 +381,88 @@ describe("AddEditV2Component", () => {
|
|||||||
expect(navigate).toHaveBeenCalledWith(["/tabs/vault"]);
|
expect(navigate).toHaveBeenCalledWith(["/tabs/vault"]);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe("reloadAddEditCipherData", () => {
|
||||||
|
beforeEach(fakeAsync(() => {
|
||||||
|
addEditCipherInfo$.next({
|
||||||
|
cipher: {
|
||||||
|
name: "InitialName",
|
||||||
|
type: CipherType.Login,
|
||||||
|
login: {
|
||||||
|
password: "initialPassword",
|
||||||
|
username: "initialUsername",
|
||||||
|
uris: [{ uri: "https://initial.com" }],
|
||||||
|
},
|
||||||
|
},
|
||||||
|
} as AddEditCipherInfo);
|
||||||
|
queryParams$.next({});
|
||||||
|
tick();
|
||||||
|
|
||||||
|
cipherServiceMock.setAddEditCipherInfo.mockClear();
|
||||||
|
}));
|
||||||
|
|
||||||
|
it("replaces all initialValues with new data, clearing stale fields", fakeAsync(() => {
|
||||||
|
const newCipherInfo = {
|
||||||
|
cipher: {
|
||||||
|
name: "UpdatedName",
|
||||||
|
type: CipherType.Login,
|
||||||
|
login: {
|
||||||
|
password: "updatedPassword",
|
||||||
|
uris: [{ uri: "https://updated.com" }],
|
||||||
|
},
|
||||||
|
},
|
||||||
|
} as AddEditCipherInfo;
|
||||||
|
|
||||||
|
addEditCipherInfo$.next(newCipherInfo);
|
||||||
|
|
||||||
|
const messageListener = component["messageListener"];
|
||||||
|
messageListener({ command: "reloadAddEditCipherData" });
|
||||||
|
tick();
|
||||||
|
|
||||||
|
expect(component.config.initialValues).toEqual({
|
||||||
|
name: "UpdatedName",
|
||||||
|
password: "updatedPassword",
|
||||||
|
loginUri: "https://updated.com",
|
||||||
|
} as OptionalInitialValues);
|
||||||
|
|
||||||
|
expect(cipherServiceMock.setAddEditCipherInfo).toHaveBeenCalledWith(null, "UserId");
|
||||||
|
}));
|
||||||
|
|
||||||
|
it("does not reload data if config is not set", fakeAsync(() => {
|
||||||
|
component.config = null;
|
||||||
|
|
||||||
|
const messageListener = component["messageListener"];
|
||||||
|
messageListener({ command: "reloadAddEditCipherData" });
|
||||||
|
tick();
|
||||||
|
|
||||||
|
expect(cipherServiceMock.setAddEditCipherInfo).not.toHaveBeenCalled();
|
||||||
|
}));
|
||||||
|
|
||||||
|
it("does not reload data if latestCipherInfo is null", fakeAsync(() => {
|
||||||
|
addEditCipherInfo$.next(null);
|
||||||
|
|
||||||
|
const messageListener = component["messageListener"];
|
||||||
|
messageListener({ command: "reloadAddEditCipherData" });
|
||||||
|
tick();
|
||||||
|
|
||||||
|
expect(component.config.initialValues).toEqual({
|
||||||
|
name: "InitialName",
|
||||||
|
password: "initialPassword",
|
||||||
|
username: "initialUsername",
|
||||||
|
loginUri: "https://initial.com",
|
||||||
|
} as OptionalInitialValues);
|
||||||
|
|
||||||
|
expect(cipherServiceMock.setAddEditCipherInfo).not.toHaveBeenCalled();
|
||||||
|
}));
|
||||||
|
|
||||||
|
it("ignores messages with different commands", fakeAsync(() => {
|
||||||
|
const initialValues = component.config.initialValues;
|
||||||
|
|
||||||
|
const messageListener = component["messageListener"];
|
||||||
|
messageListener({ command: "someOtherCommand" });
|
||||||
|
tick();
|
||||||
|
|
||||||
|
expect(component.config.initialValues).toBe(initialValues);
|
||||||
|
}));
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -1,7 +1,7 @@
|
|||||||
// FIXME: Update this file to be type safe and remove this and next line
|
// FIXME: Update this file to be type safe and remove this and next line
|
||||||
// @ts-strict-ignore
|
// @ts-strict-ignore
|
||||||
import { CommonModule } from "@angular/common";
|
import { CommonModule } from "@angular/common";
|
||||||
import { Component, OnInit } from "@angular/core";
|
import { Component, OnInit, OnDestroy } from "@angular/core";
|
||||||
import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
|
import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
|
||||||
import { FormsModule } from "@angular/forms";
|
import { FormsModule } from "@angular/forms";
|
||||||
import { ActivatedRoute, Params, Router } from "@angular/router";
|
import { ActivatedRoute, Params, Router } from "@angular/router";
|
||||||
@@ -158,7 +158,7 @@ export type AddEditQueryParams = Partial<Record<keyof QueryParams, string>>;
|
|||||||
IconButtonModule,
|
IconButtonModule,
|
||||||
],
|
],
|
||||||
})
|
})
|
||||||
export class AddEditV2Component implements OnInit {
|
export class AddEditV2Component implements OnInit, OnDestroy {
|
||||||
headerText: string;
|
headerText: string;
|
||||||
config: CipherFormConfig;
|
config: CipherFormConfig;
|
||||||
canDeleteCipher$: Observable<boolean>;
|
canDeleteCipher$: Observable<boolean>;
|
||||||
@@ -200,12 +200,58 @@ export class AddEditV2Component implements OnInit {
|
|||||||
this.subscribeToParams();
|
this.subscribeToParams();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private messageListener: (message: any) => void;
|
||||||
|
|
||||||
async ngOnInit() {
|
async ngOnInit() {
|
||||||
this.fido2PopoutSessionData = await firstValueFrom(this.fido2PopoutSessionData$);
|
this.fido2PopoutSessionData = await firstValueFrom(this.fido2PopoutSessionData$);
|
||||||
|
|
||||||
if (BrowserPopupUtils.inPopout(window)) {
|
if (BrowserPopupUtils.inPopout(window)) {
|
||||||
this.popupCloseWarningService.enable();
|
this.popupCloseWarningService.enable();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Listen for messages to reload cipher data when the pop up is already open
|
||||||
|
this.messageListener = async (message: any) => {
|
||||||
|
if (message?.command === "reloadAddEditCipherData") {
|
||||||
|
try {
|
||||||
|
await this.reloadCipherData();
|
||||||
|
} catch (error) {
|
||||||
|
this.logService.error("Failed to reload cipher data", error);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
};
|
||||||
|
BrowserApi.addListener(chrome.runtime.onMessage, this.messageListener);
|
||||||
|
}
|
||||||
|
|
||||||
|
ngOnDestroy() {
|
||||||
|
if (this.messageListener) {
|
||||||
|
BrowserApi.removeListener(chrome.runtime.onMessage, this.messageListener);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Reloads the cipher data when the popup is already open and new form data is submitted.
|
||||||
|
* This completely replaces the initialValues to clear any stale data from the previous submission.
|
||||||
|
*/
|
||||||
|
private async reloadCipherData() {
|
||||||
|
if (!this.config) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
const activeUserId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId));
|
||||||
|
|
||||||
|
const latestCipherInfo = await firstValueFrom(
|
||||||
|
this.cipherService.addEditCipherInfo$(activeUserId),
|
||||||
|
);
|
||||||
|
|
||||||
|
if (latestCipherInfo != null) {
|
||||||
|
this.config = {
|
||||||
|
...this.config,
|
||||||
|
initialValues: mapAddEditCipherInfoToInitialValues(latestCipherInfo),
|
||||||
|
};
|
||||||
|
|
||||||
|
// Be sure to clear the "cached" cipher info, so it doesn't get used again
|
||||||
|
await this.cipherService.setAddEditCipherInfo(null, activeUserId);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|||||||
@@ -2,6 +2,7 @@ import { mock } from "jest-mock-extended";
|
|||||||
|
|
||||||
import { CipherType } from "@bitwarden/common/vault/enums";
|
import { CipherType } from "@bitwarden/common/vault/enums";
|
||||||
|
|
||||||
|
import { BrowserApi } from "../../../platform/browser/browser-api";
|
||||||
import BrowserPopupUtils from "../../../platform/browser/browser-popup-utils";
|
import BrowserPopupUtils from "../../../platform/browser/browser-popup-utils";
|
||||||
|
|
||||||
import {
|
import {
|
||||||
@@ -23,6 +24,19 @@ describe("VaultPopoutWindow", () => {
|
|||||||
.spyOn(BrowserPopupUtils, "closeSingleActionPopout")
|
.spyOn(BrowserPopupUtils, "closeSingleActionPopout")
|
||||||
.mockImplementation();
|
.mockImplementation();
|
||||||
|
|
||||||
|
beforeEach(() => {
|
||||||
|
jest.spyOn(BrowserApi, "tabsQuery").mockResolvedValue([]);
|
||||||
|
jest.spyOn(BrowserApi, "updateWindowProperties").mockResolvedValue();
|
||||||
|
global.chrome = {
|
||||||
|
...global.chrome,
|
||||||
|
runtime: {
|
||||||
|
...global.chrome?.runtime,
|
||||||
|
sendMessage: jest.fn().mockResolvedValue(undefined),
|
||||||
|
getURL: jest.fn((path) => `chrome-extension://extension-id/${path}`),
|
||||||
|
},
|
||||||
|
};
|
||||||
|
});
|
||||||
|
|
||||||
afterEach(() => {
|
afterEach(() => {
|
||||||
jest.clearAllMocks();
|
jest.clearAllMocks();
|
||||||
});
|
});
|
||||||
@@ -123,6 +137,32 @@ describe("VaultPopoutWindow", () => {
|
|||||||
},
|
},
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("sends a message to refresh data when the popup is already open", async () => {
|
||||||
|
const existingPopupTab = {
|
||||||
|
id: 123,
|
||||||
|
windowId: 456,
|
||||||
|
url: `chrome-extension://extension-id/popup/index.html#/edit-cipher?singleActionPopout=${VaultPopoutType.addEditVaultItem}_${CipherType.Login}`,
|
||||||
|
} as chrome.tabs.Tab;
|
||||||
|
|
||||||
|
jest.spyOn(BrowserApi, "tabsQuery").mockResolvedValue([existingPopupTab]);
|
||||||
|
const sendMessageSpy = jest.spyOn(chrome.runtime, "sendMessage");
|
||||||
|
const updateWindowSpy = jest.spyOn(BrowserApi, "updateWindowProperties");
|
||||||
|
|
||||||
|
await openAddEditVaultItemPopout(
|
||||||
|
mock<chrome.tabs.Tab>({ windowId: 1, url: "https://jest-testing-website.com" }),
|
||||||
|
{
|
||||||
|
cipherType: CipherType.Login,
|
||||||
|
},
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(openPopoutSpy).not.toHaveBeenCalled();
|
||||||
|
expect(sendMessageSpy).toHaveBeenCalledWith({
|
||||||
|
command: "reloadAddEditCipherData",
|
||||||
|
data: { cipherId: undefined, cipherType: CipherType.Login },
|
||||||
|
});
|
||||||
|
expect(updateWindowSpy).toHaveBeenCalledWith(456, { focused: true });
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("closeAddEditVaultItemPopout", () => {
|
describe("closeAddEditVaultItemPopout", () => {
|
||||||
|
|||||||
@@ -115,10 +115,26 @@ async function openAddEditVaultItemPopout(
|
|||||||
addEditCipherUrl += formatQueryString("uri", url);
|
addEditCipherUrl += formatQueryString("uri", url);
|
||||||
}
|
}
|
||||||
|
|
||||||
await BrowserPopupUtils.openPopout(addEditCipherUrl, {
|
const extensionUrl = chrome.runtime.getURL("popup/index.html");
|
||||||
singleActionKey,
|
const existingPopupTabs = await BrowserApi.tabsQuery({ url: `${extensionUrl}*` });
|
||||||
senderWindowId: windowId,
|
const existingPopup = existingPopupTabs.find((tab) =>
|
||||||
});
|
tab.url?.includes(`singleActionPopout=${singleActionKey}`),
|
||||||
|
);
|
||||||
|
// Check if the an existing popup is already open
|
||||||
|
try {
|
||||||
|
await chrome.runtime.sendMessage({
|
||||||
|
command: "reloadAddEditCipherData",
|
||||||
|
data: { cipherId, cipherType },
|
||||||
|
});
|
||||||
|
await BrowserApi.updateWindowProperties(existingPopup.windowId, {
|
||||||
|
focused: true,
|
||||||
|
});
|
||||||
|
} catch {
|
||||||
|
await BrowserPopupUtils.openPopout(addEditCipherUrl, {
|
||||||
|
singleActionKey,
|
||||||
|
senderWindowId: windowId,
|
||||||
|
});
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|||||||
@@ -42,9 +42,18 @@ describe("CipherFormComponent", () => {
|
|||||||
{ provide: CipherFormService, useValue: mockAddEditFormService },
|
{ provide: CipherFormService, useValue: mockAddEditFormService },
|
||||||
{
|
{
|
||||||
provide: CipherFormCacheService,
|
provide: CipherFormCacheService,
|
||||||
useValue: { init: jest.fn(), getCachedCipherView: jest.fn() },
|
useValue: { init: jest.fn(), getCachedCipherView: jest.fn(), clearCache: jest.fn() },
|
||||||
|
},
|
||||||
|
{
|
||||||
|
provide: ViewCacheService,
|
||||||
|
useValue: {
|
||||||
|
signal: jest.fn(() => {
|
||||||
|
const signalFn = (): any => null;
|
||||||
|
signalFn.set = jest.fn();
|
||||||
|
return signalFn;
|
||||||
|
}),
|
||||||
|
},
|
||||||
},
|
},
|
||||||
{ provide: ViewCacheService, useValue: { signal: jest.fn(() => (): any => null) } },
|
|
||||||
{ provide: ConfigService, useValue: mock<ConfigService>() },
|
{ provide: ConfigService, useValue: mock<ConfigService>() },
|
||||||
{ provide: AccountService, useValue: mockAccountService },
|
{ provide: AccountService, useValue: mockAccountService },
|
||||||
{ provide: CipherArchiveService, useValue: mockCipherArchiveService },
|
{ provide: CipherArchiveService, useValue: mockCipherArchiveService },
|
||||||
|
|||||||
@@ -304,13 +304,30 @@ export class CipherFormComponent implements AfterViewInit, OnInit, OnChanges, Ci
|
|||||||
* Updates `updatedCipherView` based on the value from the cache.
|
* Updates `updatedCipherView` based on the value from the cache.
|
||||||
*/
|
*/
|
||||||
setInitialCipherFromCache() {
|
setInitialCipherFromCache() {
|
||||||
|
// If we are coming from the overlay/popup flow clear the cache to avoid old cached data
|
||||||
|
const hasOverlayData =
|
||||||
|
this.config.initialValues &&
|
||||||
|
(this.config.initialValues.username !== undefined ||
|
||||||
|
this.config.initialValues.password !== undefined);
|
||||||
|
|
||||||
|
if (hasOverlayData) {
|
||||||
|
this.cipherFormCacheService.clearCache();
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
const cachedCipher = this.cipherFormCacheService.getCachedCipherView();
|
const cachedCipher = this.cipherFormCacheService.getCachedCipherView();
|
||||||
if (cachedCipher === null) {
|
if (cachedCipher === null) {
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Use the cached cipher when it matches the cipher being edited
|
const isEditingExistingCipher =
|
||||||
if (this.updatedCipherView.id === cachedCipher.id) {
|
this.updatedCipherView.id && this.updatedCipherView.id === cachedCipher.id;
|
||||||
|
const isCreatingNewCipher =
|
||||||
|
!this.updatedCipherView.id &&
|
||||||
|
!cachedCipher.id &&
|
||||||
|
this.updatedCipherView.type === cachedCipher.type;
|
||||||
|
|
||||||
|
if (isEditingExistingCipher || isCreatingNewCipher) {
|
||||||
this.updatedCipherView = cachedCipher;
|
this.updatedCipherView = cachedCipher;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -382,6 +399,9 @@ export class CipherFormComponent implements AfterViewInit, OnInit, OnChanges, Ci
|
|||||||
this.config,
|
this.config,
|
||||||
);
|
);
|
||||||
|
|
||||||
|
// Clear the cache after successful save
|
||||||
|
this.cipherFormCacheService.clearCache();
|
||||||
|
|
||||||
this.toastService.showToast({
|
this.toastService.showToast({
|
||||||
variant: "success",
|
variant: "success",
|
||||||
title: null,
|
title: null,
|
||||||
|
|||||||
@@ -22,7 +22,6 @@ export class CipherFormCacheService {
|
|||||||
key: CIPHER_FORM_CACHE_KEY,
|
key: CIPHER_FORM_CACHE_KEY,
|
||||||
initialValue: null,
|
initialValue: null,
|
||||||
deserializer: CipherView.fromJSON,
|
deserializer: CipherView.fromJSON,
|
||||||
clearOnTabChange: true,
|
|
||||||
});
|
});
|
||||||
|
|
||||||
constructor() {
|
constructor() {
|
||||||
@@ -45,4 +44,11 @@ export class CipherFormCacheService {
|
|||||||
getCachedCipherView(): CipherView | null {
|
getCachedCipherView(): CipherView | null {
|
||||||
return this.cipherCache();
|
return this.cipherCache();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Clear the cached CipherView.
|
||||||
|
*/
|
||||||
|
clearCache(): void {
|
||||||
|
this.cipherCache.set(null);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user