diff --git a/.changeset/shy-moles-boil.md b/.changeset/shy-moles-boil.md new file mode 100644 index 0000000000000..6b1ae9f833ade --- /dev/null +++ b/.changeset/shy-moles-boil.md @@ -0,0 +1,5 @@ +--- +"@rocket.chat/meteor": patch +--- + +Fixes an issue where removing the permissions of an app would make it impossible to enable it after update diff --git a/apps/meteor/ee/server/apps/storage/AppRealStorage.ts b/apps/meteor/ee/server/apps/storage/AppRealStorage.ts index a3c2cc64c98b0..da319261bd0c0 100644 --- a/apps/meteor/ee/server/apps/storage/AppRealStorage.ts +++ b/apps/meteor/ee/server/apps/storage/AppRealStorage.ts @@ -1,6 +1,7 @@ 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'; +import type { UpdateFilter } from 'mongodb'; export class AppRealStorage extends AppMetadataStorage { constructor(private db: typeof Apps) { @@ -46,8 +47,18 @@ export class AppRealStorage extends AppMetadataStorage { } public async update(item: IAppStorageItem): Promise { - await this.db.updateOne({ id: item.id, _id: item._id }, { $set: item }); - return this.retrieveOne(item.id); + const updateQuery: UpdateFilter = { + $set: item, + }; + + // 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. + if (!item.permissionsGranted) { + updateQuery.$unset = { permissionsGranted: 1 }; + } + + return this.db.findOneAndUpdate({ id: item.id, _id: item._id }, updateQuery, { returnDocument: 'after' }); } public async remove(id: string): Promise<{ success: boolean }> { diff --git a/apps/meteor/ee/tests/unit/server/apps/AppRealStorage.spec.ts b/apps/meteor/ee/tests/unit/server/apps/AppRealStorage.spec.ts new file mode 100644 index 0000000000000..ce2dd7849234e --- /dev/null +++ b/apps/meteor/ee/tests/unit/server/apps/AppRealStorage.spec.ts @@ -0,0 +1,172 @@ +import { AppStatus } from '@rocket.chat/apps-engine/definition/AppStatus'; +import { AppInterface } from '@rocket.chat/apps-engine/definition/metadata'; +import { AppInstallationSource, type IAppStorageItem } from '@rocket.chat/apps-engine/server/storage'; +import { expect } from 'chai'; +import sinon from 'sinon'; + +import { AppRealStorage } from '../../../../server/apps/storage/AppRealStorage'; + +describe('AppRealStorage', () => { + let storage: AppRealStorage; + let mockDb: any; + let mockAppStorageItem: IAppStorageItem; + + beforeEach(() => { + mockDb = { + findOne: sinon.stub(), + find: sinon.stub(), + insertOne: sinon.stub(), + findOneAndUpdate: sinon.stub(), + deleteOne: sinon.stub(), + }; + + storage = new AppRealStorage(mockDb); + + mockAppStorageItem = { + id: 'test-app', + status: AppStatus.INITIALIZED, + installationSource: AppInstallationSource.PRIVATE, + languageContent: {}, + settings: {}, + implemented: {}, + permissionsGranted: [], + info: { + nameSlug: 'test-app', + name: 'Test App', + description: 'Test Description', + version: '1.0.0', + id: 'test-app', + requiredApiVersion: '1.0.0', + author: { + name: 'Test Author', + homepage: 'https://test-author.com', + support: 'https://test-author.com/support', + }, + classFile: 'test-app.js', + implements: [AppInterface.IPostExternalComponentClosed], + iconFile: 'test-icon.png', + }, + }; + }); + + afterEach(() => { + sinon.restore(); + }); + + describe('create', () => { + it('should create a new app storage item', async () => { + mockDb.findOne.resolves(null); + mockDb.insertOne.resolves({ insertedId: 'new-id' }); + + const result = await storage.create(mockAppStorageItem); + + expect(result).to.deep.include(mockAppStorageItem); + expect(result._id).to.equal('new-id'); + expect(result.createdAt).to.be.instanceOf(Date); + expect(result.updatedAt).to.be.instanceOf(Date); + }); + + it('should throw error if app already exists', async () => { + mockDb.findOne.resolves({ id: 'test-app' }); + + await expect(storage.create(mockAppStorageItem)).to.be.rejectedWith('App already exists.'); + }); + }); + + describe('retrieveOne', () => { + it('should retrieve an app by id', async () => { + mockDb.findOne.resolves(mockAppStorageItem); + + const result = await storage.retrieveOne('test-app'); + + expect(result).to.deep.equal(mockAppStorageItem); + expect(mockDb.findOne.calledWith({ $or: [{ _id: 'test-app' }, { id: 'test-app' }] })).to.be.true; + }); + }); + + describe('retrieveAll', () => { + it('should retrieve all apps', async () => { + const mockApps = [ + { id: 'app1', _id: 'id1' }, + { id: 'app2', _id: 'id2' }, + ]; + + mockDb.find.returns({ + toArray: sinon.stub().resolves(mockApps), + }); + + const result = await storage.retrieveAll(); + + expect(result.size).to.equal(2); + expect(result.get('app1')).to.deep.equal(mockApps[0]); + expect(result.get('app2')).to.deep.equal(mockApps[1]); + }); + }); + + describe('retrieveAllPrivate', () => { + it('should retrieve all private apps', async () => { + const mockApps = [ + { id: 'private-app1', _id: 'id1', installationSource: 'private' }, + { id: 'private-app2', _id: 'id2', installationSource: 'private' }, + ]; + + mockDb.find.returns({ + toArray: sinon.stub().resolves(mockApps), + }); + + const result = await storage.retrieveAllPrivate(); + + expect(result.size).to.equal(2); + expect(result.get('private-app1')).to.deep.equal(mockApps[0]); + expect(result.get('private-app2')).to.deep.equal(mockApps[1]); + expect(mockDb.find.calledWith({ installationSource: 'private' })).to.be.true; + }); + }); + + 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 }); + + const result = await storage.remove('test-app'); + + expect(result).to.deep.equal({ success: true }); + expect(mockDb.deleteOne.calledWith({ id: 'test-app' })).to.be.true; + }); + }); +}); diff --git a/packages/apps-engine/src/server/AppManager.ts b/packages/apps-engine/src/server/AppManager.ts index cb398d506085a..69d6cfa18288a 100644 --- a/packages/apps-engine/src/server/AppManager.ts +++ b/packages/apps-engine/src/server/AppManager.ts @@ -721,9 +721,14 @@ export class AppManager { implemented: result.implemented.getValues(), ...(old.marketplaceInfo && { marketplaceInfo: old.marketplaceInfo }), ...(old.sourcePath && { sourcePath: old.sourcePath }), - ...(permissionsGranted && { permissionsGranted }), }; + if (!permissionsGranted) { + delete descriptor.permissionsGranted; + } else { + descriptor.permissionsGranted = permissionsGranted; + } + try { descriptor.sourcePath = await this.appSourceStorage.update(descriptor, appPackage); } catch (error) {