From 3481c7e85f30bf40369eb11972ae09f0719a6b10 Mon Sep 17 00:00:00 2001 From: Nick Lincoln Date: Sun, 24 Jun 2018 22:32:24 +0100 Subject: [PATCH] prevent passing cache contents by reference (#4184) Signed-off-by: Nick Lincoln --- packages/composer-runtime/lib/registry.js | 35 +++++---- packages/composer-runtime/test/registry.js | 90 +++++++++++++--------- 2 files changed, 71 insertions(+), 54 deletions(-) diff --git a/packages/composer-runtime/lib/registry.js b/packages/composer-runtime/lib/registry.js index 1d44fb352f..91877078f4 100644 --- a/packages/composer-runtime/lib/registry.js +++ b/packages/composer-runtime/lib/registry.js @@ -48,7 +48,7 @@ class Registry extends EventEmitter { this.id = id; this.name = name; this.system = system; - this.resourceMap = new Map(); + this.objectMap = new Map(); } /** @@ -80,7 +80,7 @@ class Registry extends EventEmitter { const resource = this.serializer.fromJSON(object); await this.accessController.check(resource, 'READ'); resources.push(resource); - this.resourceMap.set(resource.getIdentifier(), resource); + this.objectMap.set(resource.getIdentifier(), object); } catch (error) { // Ignore the error; we don't have access. } @@ -95,15 +95,16 @@ class Registry extends EventEmitter { * object when complete, or rejected with an error. */ async get(id) { - if (this.resourceMap.has(id)) { - return this.resourceMap.get(id); + if (this.objectMap.has(id)) { + let object = this.objectMap.get(id); + return this.serializer.fromJSON(object); } else { let object = await this.dataCollection.get(id); object = Registry.removeInternalProperties(object); try { const resource = this.serializer.fromJSON(object); await this.accessController.check(resource, 'READ'); - this.resourceMap.set(id, resource); + this.objectMap.set(id, object); return resource; } catch (error) { throw new Error(`Object with ID '${id}' in collection with ID '${this.type}:${this.id}' does not exist`); @@ -123,7 +124,7 @@ class Registry extends EventEmitter { return false; } - if (this.resourceMap.has(id)) { + if (this.objectMap.has(id)) { return true; } else { let object = await this.dataCollection.get(id); @@ -131,7 +132,7 @@ class Registry extends EventEmitter { try { const resource = this.serializer.fromJSON(object); await this.accessController.check(resource, 'READ'); - this.resourceMap.set(id, resource); + this.objectMap.set(id, object); return true; } catch (error) { return false; @@ -255,8 +256,9 @@ class Registry extends EventEmitter { object = this.addInternalProperties(object); let oldResource; - if (this.resourceMap.has(id)) { - oldResource = this.resourceMap.get(id); + if (this.objectMap.has(id)) { + let oldObject = this.objectMap.get(id); + oldResource = this.serializer.fromJSON(oldObject); } else { let oldObject = await this.dataCollection.get(id); oldResource = this.serializer.fromJSON(oldObject); @@ -266,9 +268,9 @@ class Registry extends EventEmitter { await this.accessController.check(oldResource, 'UPDATE'); await this.dataCollection.update(id, object); - // If is within resourceMap cache, update may have changed the ability to READ item, so remove it - if (this.resourceMap.has(id)) { - this.resourceMap.delete(id); + // If is within objectMap cache, update may have changed the ability to READ item, so remove it + if (this.objectMap.has(id)) { + this.objectMap.delete(id); } this.emit('resourceupdated', { @@ -307,8 +309,9 @@ class Registry extends EventEmitter { // the resource using its ID from the registry. We need to // do this to figure out the type of the resource for // access control. - if (this.resourceMap.has(resource)) { - resource = this.resourceMap.get(resource); + if (this.objectMap.has(resource)) { + let object = this.objectMap.get(resource); + resource = this.serializer.fromJSON(object); } else { let object = await this.dataCollection.get(resource); object = Registry.removeInternalProperties(object); @@ -320,8 +323,8 @@ class Registry extends EventEmitter { await this.dataCollection.remove(id); // Remove from cache if present - if (this.resourceMap.has(id)) { - this.resourceMap.delete(id); + if (this.objectMap.has(id)) { + this.objectMap.delete(id); } this.emit('resourceremoved', { diff --git a/packages/composer-runtime/test/registry.js b/packages/composer-runtime/test/registry.js index 9eee2e46e3..21a5c76c1b 100644 --- a/packages/composer-runtime/test/registry.js +++ b/packages/composer-runtime/test/registry.js @@ -107,7 +107,7 @@ describe('Registry', () => { }).returns(mockResource2); }); - it('should get and parse all of the resources from the registry if not in the cache', () => { + it('should get and parse all of the resources from the registry if not in the object cache', () => { return registry.getAll() .then((resources) => { sinon.assert.calledTwice(mockAccessController.check); @@ -118,12 +118,14 @@ describe('Registry', () => { }); }); - it('should add resources to the cache during the retrieval', async () => { + it('should add objects to the object cache during the retrieval', async () => { + let map = registry.objectMap; + map.size.should.be.equal(0); await registry.getAll(); - let map = registry.resourceMap; - let keys = map.keys(); - for (let key of keys){ - map.get(key).should.be.an.instanceOf(Resource); + map.size.should.be.equal(2); + + for (let key of map.keys()){ + map.get(key).should.be.an.instanceOf(Object); } }); @@ -167,6 +169,7 @@ describe('Registry', () => { describe('#get', () => { let mockResource; + let mockResource2; beforeEach(() => { mockDataCollection.get.withArgs('doge1').resolves({ @@ -174,10 +177,16 @@ describe('Registry', () => { assetId: 'doge1' }); mockResource = sinon.createStubInstance(Resource); + mockResource2 = sinon.createStubInstance(Resource); + mockResource2.getIdentifier.returns('cached'); mockSerializer.fromJSON.withArgs({ $class: 'org.doge.Doge', assetId: 'doge1' }).returns(mockResource); + mockSerializer.fromJSON.withArgs({ + $class: 'org.doge.Doge', + assetId: 'doge2' + }).returns(mockResource2); }); it('should get the specific resource from the registry if not in the cache', () => { @@ -190,21 +199,23 @@ describe('Registry', () => { }); }); - it('should add to the cache after registry retrieval', async () => { + it('should add to the object cache after registry retrieval', async () => { await registry.get('doge1'); - let resource = registry.resourceMap.get('doge1'); - resource.should.be.an.instanceOf(Resource); + let obj = registry.objectMap.get('doge1'); + obj.should.be.an.instanceOf(Object); }); it('should get the specific resource from the cache if present', async () => { - let mockResource = sinon.createStubInstance(Resource); - mockResource.theValue = 'the value 1'; - mockResource.getIdentifier.returns('doge1'); - registry.resourceMap.set('doge1', mockResource); + registry.objectMap.set('doge2', { + $class: 'org.doge.Doge', + assetId: 'doge2' + }); - let resource = await registry.get('doge1'); + let resource = await registry.get('doge2'); resource.should.be.an.instanceOf(Resource); - resource.getIdentifier().should.equal('doge1'); + resource.getIdentifier().should.equal('cached'); + + // Should not go the datacollection route sinon.assert.notCalled(mockDataCollection.get); sinon.assert.notCalled(mockAccessController.check); }); @@ -253,7 +264,11 @@ describe('Registry', () => { return registry.exists('doge1') .should.eventually.be.true .then((exists) => { - registry.resourceMap.get('doge1').should.be.an.instanceOf(Resource); + registry.objectMap.size.should.be.equal(1); + registry.objectMap.get('doge1').should.deep.equal({ + $class: 'org.doge.Doge', + assetId: 'doge1' + }); }); }); @@ -261,7 +276,7 @@ describe('Registry', () => { let mockResource = sinon.createStubInstance(Resource); mockResource.theValue = 'the value 1'; mockResource.getIdentifier.returns('doge1'); - registry.resourceMap.set('doge1', mockResource); + registry.objectMap.set('doge1', mockResource); return registry.exists('doge1') .should.eventually.be.true @@ -590,6 +605,7 @@ describe('Registry', () => { let mockResource, mockOldResource; let mockClassDeclaration, mockOldClassDeclaration; + let mockObject, mockOldObject; beforeEach(() => { // New resources. @@ -600,11 +616,13 @@ describe('Registry', () => { mockClassDeclaration.getSystemType.returns('Asset'); mockResource.getIdentifier.returns('doge1'); mockResource.theValue = 'newValue'; - mockSerializer.toJSON.withArgs(mockResource).onFirstCall().returns({ + mockObject = { $class: 'org.doge.Doge', assetId: 'doge1', newValue: 'newValue' - }); + }; + mockSerializer.toJSON.withArgs(mockResource).onFirstCall().returns(mockObject); + mockSerializer.fromJSON.withArgs(mockObject).returns(mockResource); // Old resources. mockOldResource = sinon.createStubInstance(Resource); mockOldClassDeclaration = sinon.createStubInstance(ClassDeclaration); @@ -613,16 +631,13 @@ describe('Registry', () => { mockOldClassDeclaration.getSystemType.returns('Asset'); mockOldResource.getIdentifier.returns('doge1'); mockOldResource.theValue = 'oldValue'; - mockDataCollection.get.withArgs('doge1').resolves({ - $class: 'org.doge.Doge', - assetId: 'doge1', - newValue: 'oldValue' - }); - mockSerializer.fromJSON.withArgs({ + mockOldObject = { $class: 'org.doge.Doge', assetId: 'doge1', newValue: 'oldValue' - }).returns(mockOldResource); + }; + mockDataCollection.get.withArgs('doge1').resolves(mockOldObject); + mockSerializer.fromJSON.withArgs(mockOldObject).returns(mockOldResource); }); it('should update the resource in the registry via datacollection retrieval if not in the cache', () => { @@ -652,7 +667,7 @@ describe('Registry', () => { it('should update the resource in the registry via cache retrieval if present', () => { mockDataCollection.update.resolves(); let mockEventHandler = sinon.stub(); - registry.resourceMap.set('doge1', mockResource); + registry.objectMap.set('doge1', mockObject); registry.on('resourceupdated', mockEventHandler); return registry.update(mockResource) .then(() => { @@ -673,10 +688,10 @@ describe('Registry', () => { it('should remove the item from teh cache if updated via cache retrieval', async () => { mockDataCollection.update.resolves(); let mockEventHandler = sinon.stub(); - registry.resourceMap.set('doge1', mockResource); + registry.objectMap.set('doge1', mockResource); registry.on('resourceupdated', mockEventHandler); await registry.update(mockResource); - registry.resourceMap.has('doge1').should.be.false; + registry.objectMap.has('doge1').should.be.false; }); it('should throw an error updating a resource to the wrong registry', () => { @@ -776,21 +791,20 @@ describe('Registry', () => { describe('#remove', () => { let mockResource; + let mockObject; beforeEach(() => { // Old resources. // Deleting a resource by ID currently requires that we read it from the registry. - mockDataCollection.get.withArgs('doge1').resolves({ + mockObject = { $class: 'org.doge.Doge', assetId: 'doge1' - }); + }; + mockDataCollection.get.withArgs('doge1').resolves(mockObject); mockResource = sinon.createStubInstance(Resource); mockResource.theValue = 'the value 1'; mockResource.getIdentifier.returns('doge1'); - mockSerializer.fromJSON.withArgs({ - $class: 'org.doge.Doge', - assetId: 'doge1' - }).returns(mockResource); + mockSerializer.fromJSON.withArgs(mockObject).returns(mockResource); // End of resources. }); @@ -828,7 +842,7 @@ describe('Registry', () => { it('should remove the resource by ID from the registry via cache retrieval if present', () => { mockDataCollection.remove.resolves(); - registry.resourceMap.set('doge1', mockResource); + registry.objectMap.set('doge1', mockObject); let mockEventHandler = sinon.stub(); registry.on('resourceremoved', mockEventHandler); return registry.remove('doge1') @@ -845,11 +859,11 @@ describe('Registry', () => { it('should remove the resource from the cache if present', async () => { mockDataCollection.remove.resolves(); - registry.resourceMap.set('doge1', mockResource); + registry.objectMap.set('doge1', mockObject); let mockEventHandler = sinon.stub(); registry.on('resourceremoved', mockEventHandler); await registry.remove('doge1'); - registry.resourceMap.has('doge1').should.be.false; + registry.objectMap.has('doge1').should.be.false; }); it('should throw if the access controller throws an exception', () => {