Skip to content

Commit

Permalink
prevent passing cache contents by reference (hyperledger-archives#4184)
Browse files Browse the repository at this point in the history 
Signed-off-by: Nick Lincoln <[email protected]>
  • Loading branch information
@nklincoln
nklincoln authored and Dave Kelsey committed Jun 24, 2018
1 parent 8c90b3c commit 3481c7e
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 54 deletions.
35 changes: 19 additions & 16 deletions packages/composer-runtime/lib/registry.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

/**
Expand Down Expand Up @@ -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.
}
Expand All @@ -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`);
Expand All @@ -123,15 +124,15 @@ 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);
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 true;
} catch (error) {
return false;
Expand Down Expand Up @@ -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);
Expand All @@ -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', {
Expand Down Expand Up @@ -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);
Expand All @@ -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', {
Expand Down
90 changes: 52 additions & 38 deletions packages/composer-runtime/test/registry.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
}
});

Expand Down Expand Up @@ -167,17 +169,24 @@ describe('Registry', () => {
describe('#get', () => {

let mockResource;
let mockResource2;

beforeEach(() => {
mockDataCollection.get.withArgs('doge1').resolves({
$class: 'org.doge.Doge',
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', () => {
Expand All @@ -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);
});
Expand Down Expand Up @@ -253,15 +264,19 @@ 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'
});
});
});

it('should determine whether a specific resource exists via cache existence', async () => {
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
Expand Down Expand Up @@ -590,6 +605,7 @@ describe('Registry', () => {

let mockResource, mockOldResource;
let mockClassDeclaration, mockOldClassDeclaration;
let mockObject, mockOldObject;

beforeEach(() => {
// New resources.
Expand All @@ -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);
Expand All @@ -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', () => {
Expand Down Expand Up @@ -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(() => {
Expand All @@ -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', () => {
Expand Down Expand Up @@ -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.
});

Expand Down Expand Up @@ -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')
Expand All @@ -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', () => {
Expand Down

0 comments on commit 3481c7e

Please sign in to comment.