fix(apps): prevent unwanted changes to app status (#36990)

.changeset/gorgeous-cougars-visit.md

@@ -0,0 +1,6 @@
+---
+'@rocket.chat/apps-engine': patch
+'@rocket.chat/meteor': patch
+---
+
+Change app update strategies to prevent unwanted data changes in database

apps/meteor/ee/server/apps/storage/AppRealStorage.ts

@@ -1,3 +1,7 @@
+import type { AppStatus } from '@rocket.chat/apps-engine/definition/AppStatus';
+import type { IAppInfo } from '@rocket.chat/apps-engine/definition/metadata';
+import type { ISetting } from '@rocket.chat/apps-engine/definition/settings';
+import type { IMarketplaceInfo } from '@rocket.chat/apps-engine/server/marketplace';
 import type { IAppStorageItem } from '@rocket.chat/apps-engine/server/storage';
 import { AppMetadataStorage } from '@rocket.chat/apps-engine/server/storage';
 import type { Apps } from '@rocket.chat/models';
@@ -48,20 +52,49 @@ export class AppRealStorage extends AppMetadataStorage {
 		return items;
 	}
 
-	public async update({ permissionsGranted, ...item }: IAppStorageItem): Promise<IAppStorageItem> {
-		const updateQuery: UpdateFilter<IAppStorageItem> = {
-			$set: { ...item, ...(permissionsGranted && { permissionsGranted }) },
-			// Note: This is really important, since we currently store the permissionsGranted as null if none are present
-			//       in the App's manifest. So, if there was a permissionGranted and it was removed, we must see the app as having
-			//       no permissionsGranted at all (which means default permissions). So we must actively unset the field.
-			...(!permissionsGranted && { $unset: { permissionsGranted: 1 } }),
-		};
-
-		return this.db.findOneAndUpdate({ id: item.id, _id: item._id }, updateQuery, { returnDocument: 'after' });
-	}
-
 	public async remove(id: string): Promise<{ success: boolean }> {
 		await this.db.deleteOne({ id });
 		return { success: true };
 	}
+
+	public async updatePartialAndReturnDocument(
+		{ _id, ...item }: IAppStorageItem,
+		{ unsetPermissionsGranted = false } = {},
+	): Promise<IAppStorageItem> {
+		if (!_id) {
+			throw new Error('Property _id is required to update an app storage item');
+		}
+
+		const updateQuery: UpdateFilter<IAppStorageItem> = {
+			$set: item,
+		};
+
+		if (unsetPermissionsGranted) {
+			delete item.permissionsGranted;
+			updateQuery.$unset = { permissionsGranted: 1 };
+		}
+
+		return this.db.findOneAndUpdate({ _id }, updateQuery, { returnDocument: 'after' });
+	}
+
+	public async updateStatus(_id: string, status: AppStatus): Promise<boolean> {
+		const result = await this.db.updateOne({ _id }, { $set: { status } });
+		return result.modifiedCount > 0;
+	}
+
+	public async updateSetting(_id: string, setting: ISetting): Promise<boolean> {
+		const result = await this.db.updateOne({ _id }, { $set: { [`settings.${setting.id}`]: setting } });
+
+		return result.modifiedCount > 0;
+	}
+
+	public async updateAppInfo(_id: string, info: IAppInfo): Promise<boolean> {
+		const result = await this.db.updateOne({ _id }, { $set: { info } });
+		return result.modifiedCount > 0;
+	}
+
+	public async updateMarketplaceInfo(_id: string, marketplaceInfo: IMarketplaceInfo[]): Promise<boolean> {
+		const result = await this.db.updateOne({ _id }, { $set: { marketplaceInfo } });
+		return result.modifiedCount > 0;
+	}
 }

apps/meteor/ee/tests/unit/server/apps/AppRealStorage.spec.ts

@@ -1,5 +1,6 @@
 import { AppStatus } from '@rocket.chat/apps-engine/definition/AppStatus';
 import { AppInterface } from '@rocket.chat/apps-engine/definition/metadata';
+import { SettingType } from '@rocket.chat/apps-engine/definition/settings';
 import { AppInstallationSource, type IAppStorageItem } from '@rocket.chat/apps-engine/server/storage';
 import { expect } from 'chai';
 import sinon from 'sinon';
@@ -8,9 +9,17 @@ import { AppRealStorage } from '../../../../server/apps/storage/AppRealStorage';
 
 describe('AppRealStorage', () => {
 	let storage: AppRealStorage;
-	let mockDb: any;
 	let mockAppStorageItem: IAppStorageItem;
 
+	let mockDb: {
+		findOne: sinon.SinonStub;
+		find: sinon.SinonStub;
+		insertOne: sinon.SinonStub;
+		findOneAndUpdate: sinon.SinonStub;
+		deleteOne: sinon.SinonStub;
+		updateOne: sinon.SinonStub;
+	};
+
 	beforeEach(() => {
 		mockDb = {
 			findOne: sinon.stub(),
@@ -18,9 +27,10 @@ describe('AppRealStorage', () => {
 			insertOne: sinon.stub(),
 			findOneAndUpdate: sinon.stub(),
 			deleteOne: sinon.stub(),
+			updateOne: sinon.stub(),
 		};
 
-		storage = new AppRealStorage(mockDb);
+		storage = new AppRealStorage(mockDb as any);
 
 		mockAppStorageItem = {
 			id: 'test-app',
@@ -123,42 +133,6 @@ describe('AppRealStorage', () => {
 		});
 	});
 
-	describe('update', () => {
-		it('should update an app', async () => {
-			mockDb.findOneAndUpdate.resolves(mockAppStorageItem);
-
-			const result = await storage.update(mockAppStorageItem);
-
-			expect(result).to.deep.equal(mockAppStorageItem);
-			expect(
-				mockDb.findOneAndUpdate.calledWith(
-					{ id: mockAppStorageItem.id, _id: mockAppStorageItem._id },
-					{ $set: mockAppStorageItem },
-					{ returnDocument: 'after' },
-				),
-			).to.be.true;
-		});
-
-		it('should unset permissionsGranted if not present', async () => {
-			delete mockAppStorageItem.permissionsGranted;
-
-			mockDb.findOneAndUpdate.resolves(mockAppStorageItem);
-
-			await storage.update(mockAppStorageItem);
-
-			expect(
-				mockDb.findOneAndUpdate.calledWith(
-					{ id: mockAppStorageItem.id, _id: mockAppStorageItem._id },
-					{
-						$set: mockAppStorageItem,
-						$unset: { permissionsGranted: 1 },
-					},
-					{ returnDocument: 'after' },
-				),
-			).to.be.true;
-		});
-	});
-
 	describe('remove', () => {
 		it('should remove an app', async () => {
 			mockDb.deleteOne.resolves({ deletedCount: 1 });
@@ -169,4 +143,263 @@ describe('AppRealStorage', () => {
 			expect(mockDb.deleteOne.calledWith({ id: 'test-app' })).to.be.true;
 		});
 	});
+
+	describe('updatePartialAndReturnDocument', () => {
+		it('should throw error when _id is missing', async () => {
+			const itemWithoutId = { ...mockAppStorageItem };
+
+			await expect(storage.updatePartialAndReturnDocument(itemWithoutId as any)).to.be.rejectedWith(
+				'Property _id is required to update an app storage item',
+			);
+		});
+
+		it('should throw error when _id is null', async () => {
+			const itemWithNullId = { ...mockAppStorageItem, _id: null };
+
+			await expect(storage.updatePartialAndReturnDocument(itemWithNullId as any)).to.be.rejectedWith(
+				'Property _id is required to update an app storage item',
+			);
+		});
+
+		it('should throw error when _id is undefined', async () => {
+			const itemWithUndefinedId = { ...mockAppStorageItem, _id: undefined };
+
+			await expect(storage.updatePartialAndReturnDocument(itemWithUndefinedId as any)).to.be.rejectedWith(
+				'Property _id is required to update an app storage item',
+			);
+		});
+
+		it('should throw error when _id is empty string', async () => {
+			const itemWithEmptyId = { ...mockAppStorageItem, _id: '' };
+
+			await expect(storage.updatePartialAndReturnDocument(itemWithEmptyId as any)).to.be.rejectedWith(
+				'Property _id is required to update an app storage item',
+			);
+		});
+
+		it('should unset permissionsGranted when unsetPermissionsGranted is true', async () => {
+			const updatedItem = { ...mockAppStorageItem, _id: 'test-id' };
+			const expectedItem = { ...mockAppStorageItem };
+			delete expectedItem.permissionsGranted;
+			mockDb.findOneAndUpdate.resolves(expectedItem);
+
+			const result = await storage.updatePartialAndReturnDocument(updatedItem, { unsetPermissionsGranted: true });
+
+			expect(result).to.deep.equal(expectedItem);
+			expect(
+				mockDb.findOneAndUpdate.calledWith(
+					{ _id: 'test-id' },
+					{
+						$set: expectedItem,
+						$unset: { permissionsGranted: 1 },
+					},
+					{ returnDocument: 'after' },
+				),
+			).to.be.true;
+		});
+
+		it('should not unset permissionsGranted when unsetPermissionsGranted is false', async () => {
+			const updatedItem = { ...mockAppStorageItem, _id: 'test-id' };
+			mockDb.findOneAndUpdate.resolves(updatedItem);
+
+			await storage.updatePartialAndReturnDocument(updatedItem, { unsetPermissionsGranted: false });
+
+			expect(mockDb.findOneAndUpdate.calledWith({ _id: 'test-id' }, { $set: mockAppStorageItem }, { returnDocument: 'after' })).to.be.true;
+		});
+
+		it('should not unset permissionsGranted when no options are passed', async () => {
+			const updatedItem = { ...mockAppStorageItem, _id: 'test-id' };
+			mockDb.findOneAndUpdate.resolves(updatedItem);
+
+			await storage.updatePartialAndReturnDocument(updatedItem);
+
+			expect(mockDb.findOneAndUpdate.calledWith({ _id: 'test-id' }, { $set: mockAppStorageItem }, { returnDocument: 'after' })).to.be.true;
+		});
+	});
+
+	describe('updateStatus', () => {
+		it('should update app status and return true when modified', async () => {
+			mockDb.updateOne.resolves({ modifiedCount: 1 });
+
+			const result = await storage.updateStatus('test-id', AppStatus.AUTO_ENABLED);
+
+			expect(result).to.be.true;
+			expect(mockDb.updateOne.calledWith({ _id: 'test-id' }, { $set: { status: AppStatus.AUTO_ENABLED } })).to.be.true;
+		});
+
+		it('should return false when no document was modified', async () => {
+			mockDb.updateOne.resolves({ modifiedCount: 0 });
+
+			const result = await storage.updateStatus('test-id', AppStatus.DISABLED);
+
+			expect(result).to.be.false;
+			expect(mockDb.updateOne.calledWith({ _id: 'test-id' }, { $set: { status: AppStatus.DISABLED } })).to.be.true;
+		});
+	});
+
+	describe('updateSetting', () => {
+		it('should update app setting and return true when modified', async () => {
+			const mockSetting = {
+				id: 'test-setting',
+				type: SettingType.STRING,
+				packageValue: 'default-value',
+				value: 'updated-value',
+				required: false,
+				public: true,
+				i18nLabel: 'Test Setting',
+			};
+			mockDb.updateOne.resolves({ modifiedCount: 1 });
+
+			const result = await storage.updateSetting('test-id', mockSetting);
+
+			expect(result).to.be.true;
+			expect(mockDb.updateOne.calledWith({ _id: 'test-id' }, { $set: { 'settings.test-setting': mockSetting } })).to.be.true;
+		});
+
+		it('should return false when no document was modified', async () => {
+			const mockSetting = {
+				id: 'test-setting',
+				type: SettingType.STRING,
+				packageValue: 'default-value',
+				value: 'updated-value',
+				required: false,
+				public: true,
+				i18nLabel: 'Test Setting',
+			};
+			mockDb.updateOne.resolves({ modifiedCount: 0 });
+
+			const result = await storage.updateSetting('test-id', mockSetting);
+
+			expect(result).to.be.false;
+			expect(mockDb.updateOne.calledWith({ _id: 'test-id' }, { $set: { 'settings.test-setting': mockSetting } })).to.be.true;
+		});
+	});
+
+	describe('updateAppInfo', () => {
+		it('should update app info and return true when modified', async () => {
+			const mockAppInfo = {
+				id: 'test-app',
+				name: 'Updated Test App',
+				nameSlug: 'updated-test-app',
+				description: 'Updated Description',
+				version: '2.0.0',
+				requiredApiVersion: '2.0.0',
+				author: {
+					name: 'Updated Author',
+					homepage: 'https://updated-author.com',
+					support: 'https://updated-author.com/support',
+				},
+				classFile: 'updated-app.js',
+				implements: [],
+				iconFile: 'updated-icon.png',
+			};
+			mockDb.updateOne.resolves({ modifiedCount: 1 });
+
+			const result = await storage.updateAppInfo('test-id', mockAppInfo);
+
+			expect(result).to.be.true;
+			expect(mockDb.updateOne.calledWith({ _id: 'test-id' }, { $set: { info: mockAppInfo } })).to.be.true;
+		});
+
+		it('should return false when no document was modified', async () => {
+			const mockAppInfo = {
+				id: 'test-app',
+				name: 'Updated Test App',
+				nameSlug: 'updated-test-app',
+				description: 'Updated Description',
+				version: '2.0.0',
+				requiredApiVersion: '2.0.0',
+				author: {
+					name: 'Updated Author',
+					homepage: 'https://updated-author.com',
+					support: 'https://updated-author.com/support',
+				},
+				classFile: 'updated-app.js',
+				implements: [],
+				iconFile: 'updated-icon.png',
+			};
+			mockDb.updateOne.resolves({ modifiedCount: 0 });
+
+			const result = await storage.updateAppInfo('test-id', mockAppInfo);
+
+			expect(result).to.be.false;
+			expect(mockDb.updateOne.calledWith({ _id: 'test-id' }, { $set: { info: mockAppInfo } })).to.be.true;
+		});
+	});
+
+	describe('updateMarketplaceInfo', () => {
+		it('should update marketplace info and return true when modified', async () => {
+			const mockMarketplaceInfo = [
+				{
+					id: 'test-app',
+					name: 'Test App',
+					nameSlug: 'test-app',
+					version: '2.0.0',
+					description: 'Test Description',
+					requiredApiVersion: '2.0.0',
+					author: {
+						name: 'Test Author',
+						homepage: 'https://test-author.com',
+						support: 'https://test-author.com/support',
+					},
+					classFile: 'test-app.js',
+					implements: [],
+					iconFile: 'test-icon.png',
+					categories: ['productivity'],
+					status: 'published',
+					isVisible: true,
+					isPurchased: false,
+					isSubscribed: false,
+					isBundled: false,
+					createdDate: '2023-01-01T00:00:00.000Z',
+					modifiedDate: '2023-01-01T00:00:00.000Z',
+					price: 0,
+					purchaseType: 'free' as any,
+				},
+			];
+			mockDb.updateOne.resolves({ modifiedCount: 1 });
+
+			const result = await storage.updateMarketplaceInfo('test-id', mockMarketplaceInfo);
+
+			expect(result).to.be.true;
+			expect(mockDb.updateOne.calledWith({ _id: 'test-id' }, { $set: { marketplaceInfo: mockMarketplaceInfo } })).to.be.true;
+		});
+
+		it('should return false when no document was modified', async () => {
+			const mockMarketplaceInfo = [
+				{
+					id: 'test-app',
+					name: 'Test App',
+					nameSlug: 'test-app',
+					version: '2.0.0',
+					description: 'Test Description',
+					requiredApiVersion: '2.0.0',
+					author: {
+						name: 'Test Author',
+						homepage: 'https://test-author.com',
+						support: 'https://test-author.com/support',
+					},
+					classFile: 'test-app.js',
+					implements: [],
+					iconFile: 'test-icon.png',
+					categories: ['productivity'],
+					status: 'published',
+					isVisible: true,
+					isPurchased: false,
+					isSubscribed: false,
+					isBundled: false,
+					createdDate: '2023-01-01T00:00:00.000Z',
+					modifiedDate: '2023-01-01T00:00:00.000Z',
+					price: 0,
+					purchaseType: 'free' as any,
+				},
+			];
+			mockDb.updateOne.resolves({ modifiedCount: 0 });
+
+			const result = await storage.updateMarketplaceInfo('test-id', mockMarketplaceInfo);
+
+			expect(result).to.be.false;
+			expect(mockDb.updateOne.calledWith({ _id: 'test-id' }, { $set: { marketplaceInfo: mockMarketplaceInfo } })).to.be.true;
+		});
+	});
 });

apps/meteor/server/startup/migrations/v294.ts

@@ -30,7 +30,7 @@ addMigration({
 				installationSource: 'marketplaceInfo' in app ? 'marketplace' : 'private',
 			} as IAppStorageItem;
 
-			await appsStorage.update({
+			await appsStorage.updatePartialAndReturnDocument({
 				...updatedApp,
 				signature: await sigMan.signApp(updatedApp),
 			});

apps/meteor/server/startup/migrations/v307.ts

@@ -31,7 +31,7 @@ addMigration({
 				migrated: true,
 			} as IAppStorageItem;
 
-			await appsStorage.update({
+			await appsStorage.updatePartialAndReturnDocument({
 				...updatedApp,
 				signature: await sigMan.signApp(updatedApp),
 			});

packages/apps-engine/src/server/AppManager.ts

@@ -462,7 +462,7 @@ export class AppManager {
 			storageItem.status = await rl.getStatus();
 			// This is async, but we don't care since it only updates in the database
 			// and it should not mutate any properties we care about
-			await this.appMetadataStorage.update(storageItem).catch();
+			await this.appMetadataStorage.updateStatus(storageItem._id, storageItem.status).catch(() => {});
 		}
 
 		return isSetup;
@@ -495,7 +495,7 @@ export class AppManager {
 		storageItem.status = await app.getStatus();
 		// This is async, but we don't care since it only updates in the database
 		// and it should not mutate any properties we care about
-		await this.appMetadataStorage.update(storageItem).catch();
+		await this.appMetadataStorage.updateStatus(storageItem._id, storageItem.status).catch(() => {});
 
 		return true;
 	}
@@ -514,13 +514,13 @@ export class AppManager {
 		const storageItem = await this.appMetadataStorage.retrieveOne(id);
 
 		app.getStorageItem().marketplaceInfo = storageItem.marketplaceInfo;
-		await app.validateLicense().catch();
+		await app.validateLicense().catch(() => {});
 
 		storageItem.migrated = true;
 		storageItem.signature = await this.getSignatureManager().signApp(storageItem);
-		// This is async, but we don't care since it only updates in the database
-		// and it should not mutate any properties we care about
-		const stored = await this.appMetadataStorage.update(storageItem).catch();
+
+		const { marketplaceInfo, signature, migrated, _id } = storageItem;
+		const stored = await this.appMetadataStorage.updatePartialAndReturnDocument({ marketplaceInfo, signature, migrated, _id });
 
 		await this.updateLocal(stored, app);
 		await this.bridges
@@ -748,7 +748,9 @@ export class AppManager {
 		}
 
 		descriptor.signature = await this.signatureManager.signApp(descriptor);
-		const stored = await this.appMetadataStorage.update(descriptor);
+		const stored = await this.appMetadataStorage.updatePartialAndReturnDocument(descriptor, {
+			unsetPermissionsGranted: typeof permissionsGranted === 'undefined',
+		});
 
 		// Errors here don't really prevent the process from dying, so we don't really need to do anything on the catch
 		await this.getRuntime()
@@ -903,7 +905,7 @@ export class AppManager {
 
 				appStorageItem.marketplaceInfo[0].subscriptionInfo = appInfo.subscriptionInfo;
 
-				return this.appMetadataStorage.update(appStorageItem);
+				return this.appMetadataStorage.updateMarketplaceInfo(appStorageItem._id, appStorageItem.marketplaceInfo);
 			}),
 		).catch();
 
@@ -939,7 +941,7 @@ export class AppManager {
 						const storageItem = app.getStorageItem();
 						storageItem.status = status;
 
-						return this.appMetadataStorage.update(storageItem).catch(console.error) as Promise<void>;
+						return this.appMetadataStorage.updateStatus(storageItem._id, storageItem.status).catch(console.error) as Promise<void>;
 					}),
 			),
 		);
@@ -1070,7 +1072,7 @@ export class AppManager {
 			// This is async, but we don't care since it only updates in the database
 			// and it should not mutate any properties we care about
 			storageItem.status = await app.getStatus();
-			await this.appMetadataStorage.update(storageItem).catch();
+			await this.appMetadataStorage.updateStatus(storageItem._id, storageItem.status).catch(() => {});
 		}
 
 		return result;
@@ -1174,7 +1176,7 @@ export class AppManager {
 			storageItem.status = status;
 			// This is async, but we don't care since it only updates in the database
 			// and it should not mutate any properties we care about
-			await this.appMetadataStorage.update(storageItem).catch();
+			await this.appMetadataStorage.updateStatus(storageItem._id, storageItem.status).catch(() => {});
 		}
 
 		await app.setStatus(status, silenceStatus);

packages/apps-engine/src/server/managers/AppSettingsManager.ts

@@ -34,7 +34,9 @@ export class AppSettingsManager {
 			throw new Error('No App found by the provided id.');
 		}
 
-		const oldSetting = rl.getStorageItem().settings[setting.id];
+		const storageItem = rl.getStorageItem();
+
+		const oldSetting = storageItem.settings[setting.id];
 		if (!oldSetting) {
 			throw new Error('No setting found for the App by the provided id.');
 		}
@@ -43,11 +45,9 @@ export class AppSettingsManager {
 			(await rl.call(AppMethod.ON_PRE_SETTING_UPDATE, { oldSetting, newSetting: setting } as ISettingUpdateContext)) || setting;
 
 		decoratedSetting.updatedAt = new Date();
-		rl.getStorageItem().settings[decoratedSetting.id] = decoratedSetting;
+		storageItem.settings[decoratedSetting.id] = decoratedSetting;
 
-		const item = await this.manager.getStorage().update(rl.getStorageItem());
-
-		rl.setStorageItem(item);
+		await this.manager.getStorage().updateSetting(storageItem._id, decoratedSetting);
 
 		this.manager.getBridges().getAppDetailChangesBridge().doOnAppSettingsChange(appId, decoratedSetting);
 

packages/apps-engine/src/server/storage/AppMetadataStorage.ts

@@ -1,4 +1,8 @@
 import type { IAppStorageItem } from './IAppStorageItem';
+import type { AppStatus } from '../../definition/AppStatus';
+import type { IAppInfo } from '../../definition/metadata/IAppInfo';
+import type { ISetting } from '../../definition/settings';
+import type { IMarketplaceInfo } from '../marketplace';
 
 export abstract class AppMetadataStorage {
 	constructor(private readonly engine: string) {}
@@ -15,7 +19,18 @@ export abstract class AppMetadataStorage {
 
 	public abstract retrieveAllPrivate(): Promise<Map<string, IAppStorageItem>>;
 
-	public abstract update(item: IAppStorageItem): Promise<IAppStorageItem>;
-
 	public abstract remove(id: string): Promise<{ success: boolean }>;
+
+	public abstract updatePartialAndReturnDocument(
+		item: Partial<IAppStorageItem>,
+		options?: { unsetPermissionsGranted?: boolean },
+	): Promise<IAppStorageItem>;
+
+	public abstract updateStatus(_id: string, status: AppStatus): Promise<boolean>;
+
+	public abstract updateSetting(_id: string, setting: ISetting): Promise<boolean>;
+
+	public abstract updateAppInfo(_id: string, info: IAppInfo): Promise<boolean>;
+
+	public abstract updateMarketplaceInfo(_id: string, marketplaceInfo: IMarketplaceInfo[]): Promise<boolean>;
 }

packages/apps-engine/tests/server/managers/AppSettingsManager.spec.ts

@@ -1,6 +1,7 @@
-import { AsyncTest, Expect, SetupFixture, SpyOn, Test } from 'alsatian';
+import { Any, AsyncTest, Expect, SetupFixture, SpyOn, Test } from 'alsatian';
 
 import { AppMethod } from '../../../src/definition/metadata';
+import type { ISetting } from '../../../src/definition/settings';
 import type { AppManager } from '../../../src/server/AppManager';
 import type { ProxiedApp } from '../../../src/server/ProxiedApp';
 import type { AppBridges } from '../../../src/server/bridges';
@@ -33,6 +34,7 @@ export class AppSettingsManagerTestFixture {
 	@SetupFixture
 	public setupFixture() {
 		this.mockStorageItem = {
+			_id: 'test_underscore_id',
 			settings: {},
 		} as IAppStorageItem;
 
@@ -55,8 +57,8 @@ export class AppSettingsManagerTestFixture {
 		this.mockBridges = new TestsAppBridges();
 
 		this.mockStorage = {
-			update(item: IAppStorageItem): Promise<IAppStorageItem> {
-				return Promise.resolve(item);
+			updateSetting(appId: string, setting: ISetting): Promise<boolean> {
+				return Promise.resolve(true);
 			},
 		} as AppMetadataStorage;
 
@@ -125,9 +127,8 @@ export class AppSettingsManagerTestFixture {
 	public async updatingSettingViaAppSettingsManager() {
 		const asm = new AppSettingsManager(this.mockManager);
 
-		SpyOn(this.mockStorage, 'update');
+		SpyOn(this.mockStorage, 'updateSetting');
 		SpyOn(this.mockApp, 'call');
-		SpyOn(this.mockApp, 'setStorageItem');
 		SpyOn(this.mockBridges.getAppDetailChangesBridge(), 'doOnAppSettingsChange');
 
 		await Expect(() => asm.updateAppSetting('fake', TestData.getSetting())).toThrowErrorAsync(Error, 'No App found by the provided id.');
@@ -139,8 +140,8 @@ export class AppSettingsManagerTestFixture {
 		const set = TestData.getSetting('testing');
 		await Expect(() => asm.updateAppSetting('testing', set)).not.toThrowAsync();
 
-		Expect(this.mockStorage.update).toHaveBeenCalledWith(this.mockStorageItem).exactly(1);
-		Expect(this.mockApp.setStorageItem).toHaveBeenCalledWith(this.mockStorageItem).exactly(1);
+		Expect(this.mockStorage.updateSetting).toHaveBeenCalledWith('test_underscore_id', Any(Object).thatMatches(set)).exactly(1);
+
 		Expect(this.mockBridges.getAppDetailChangesBridge().doOnAppSettingsChange).toHaveBeenCalledWith('testing', set).exactly(1);
 
 		Expect(this.mockApp.call).toHaveBeenCalledWith(AppMethod.ONSETTINGUPDATED, set).exactly(1);