Skip to content
This repository has been archived by the owner on Mar 8, 2020. It is now read-only.

prevent double ACL checking on transaction submission #4075

Merged
merged 2 commits into from
May 31, 2018
Merged

prevent double ACL checking on transaction submission #4075

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
400faef
prevent double ACL checking on transaction submission
nklincoln May 29, 2018
686406c
Merge branch 'master' into submitTransaction
mbwhite May 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
148 changes: 60 additions & 88 deletions packages/composer-runtime/lib/engine.transactions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,10 @@ class EngineTransactions {
* @return {Promise} A promise that will be resolved when complete, or rejected
* with an error.
*/
submitTransaction(context, args) {
async submitTransaction(context, args) {
const method = 'submitTransaction';
LOG.entry(method, context, args);

const t0 = Date.now();

if (args.length !== 1) {
Expand All @@ -43,106 +44,77 @@ class EngineTransactions {
throw new Error(util.format('Invalid arguments "%j" to function "%s", expecting "%j"', args, 'submitTransaction', [ 'serializedResource']));
}

// Find the default transaction registry.
let registryManager = context.getRegistryManager();
let transaction = null;
let historian = null;
let txRegistry = null;

// Parse the transaction from the JSON string..
LOG.debug(method, 'Parsing transaction from JSON');
let transactionData = JSON.parse(args[0]);

// Now we need to convert the JavaScript object into a transaction resource.
LOG.debug(method, 'Parsing transaction from parsed JSON object');
// First we parse *our* copy, that is not resolved. This is the copy that gets added to the
// This is *our* copy, that is not resolved. It is the copy that gets added to the
// transaction registry, and is the one in the context (for adding log entries).
transaction = context.getSerializer().fromJSON(transactionData);
LOG.debug(method, 'Parsing transaction from parsed JSON object');
const transaction = context.getSerializer().fromJSON(transactionData);
const txClass = transaction.getFullyQualifiedType();

// Store the transaction in the context.
context.setTransaction(transaction);

// This is the count of transaction processor functions executed.
let totalCount = 0;
// Get the transaction and historian registries
let registryManager = context.getRegistryManager();

LOG.debug(method, 'Getting default transaction registry for ' + txClass);
const txRegistry = await registryManager.get('Transaction', txClass);

LOG.debug(method, 'Getting historian registry');
const historian = await registryManager.get('Asset', 'org.hyperledger.composer.system.HistorianRecord');

// Form the historian record
const record = await this.createHistorianRecord(transaction, context);

// check that we can add to both these registries ahead of time
LOG.debug(method, 'Validating ability to create in Transaction and Historian registries');
let canTxAdd = await txRegistry.testAdd(transaction);
Copy link
Contributor

@mbwhite mbwhite May 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wondering if it would more efficient to put an if check after each call

let canTxAdd = await txRegistry.testAdd(transaction);
if (canTxAdd){
   throw canTxAdd;
}
let canHistorianAdd = await historian.testAdd(record);
if (canHistorianAdd){
  throw canHistoriandAdd;
}

let canHistorianAdd = await historian.testAdd(record);

if (canTxAdd || canHistorianAdd){
throw canTxAdd ? canTxAdd : canHistorianAdd;
}

let txClass = transaction.getFullyQualifiedType();
LOG.debug(method, 'Getting default transaction registry for '+txClass);
// Resolve the users copy of the transaction.
LOG.debug(method, 'Parsed transaction, resolving it', transaction);
let resolvedTransaction;

// Get the historian.
LOG.debug(method, 'Getting historian');
return registryManager.get('Asset', 'org.hyperledger.composer.system.HistorianRecord')
.then((result) => {
historian = result;
LOG.debug(method, 'Getting default transaction registry for '+txClass);
return registryManager.get('Transaction', txClass);
})
.then((result) => {
txRegistry = result;
// check that we can add to both these registries ahead of time
return txRegistry.testAdd(transaction);
})
.then((result)=>{
if (result){
throw result;
}
return context.getResolver().resolve(transaction);
})
.then((resolvedTransaction_) => {
// Save the resolved transaction.
resolvedTransaction = resolvedTransaction_;

// Execute any system transaction processor functions.
const api = context.getApi();
return context.getTransactionHandlers().reduce((promise, transactionHandler) => {
return promise.then(() => {

return transactionHandler.execute(api, resolvedTransaction)
.then((count) => {
totalCount += count;
});
});
}, Promise.resolve());

})
.then(() => {
// Execute any user transaction processor functions.
const api = context.getApi();
return context.getCompiledScriptBundle().execute(api, resolvedTransaction)
.then((count) => {
totalCount += count;
});

})
.then(() => {
// Check that a transaction processor function was executed.
if (totalCount === 0) {
const error = new Error(`Could not find any functions to execute for transaction ${resolvedTransaction.getFullyQualifiedIdentifier()}`);
LOG.error(method, error);
LOG.debug('@PERF ' + method, 'Total (ms) duration: ' + (Date.now() - t0).toFixed(2));
throw error;
}

// Store the transaction in the transaction registry.
LOG.debug(method, 'Storing executed transaction in transaction registry');
return txRegistry.add(transaction);
})
.then(()=>{
return this.createHistorianRecord(transaction,context);
})
.then((result) => {
// Store the transaction in the transaction registry.
LOG.debug(method, 'Storing historian record in the registry');
return historian.add(result);
})
.then(() => {
context.clearTransaction();
LOG.exit(method);
LOG.debug('@PERF ' + method, 'Total (ms) duration: ' + (Date.now() - t0).toFixed(2));
});
LOG.debug(method, 'Resolving transaction', transaction);
const resolvedTransaction = await context.getResolver().resolve(transaction);

// Execute any system transaction processor functions.
let count = 0;
let totalCount = 0;
for (let transactionHandler of context.getTransactionHandlers()) {
count = await transactionHandler.execute(context.getApi(), resolvedTransaction);
totalCount += count;
}

// Execute any user transaction processor functions.
count = await context.getCompiledScriptBundle().execute(context.getApi(), resolvedTransaction);
totalCount += count;

// Check that a transaction processor function was executed.
if (totalCount === 0) {
const error = new Error(`Could not find any functions to execute for transaction ${resolvedTransaction.getFullyQualifiedIdentifier()}`);
LOG.error(method, error);
LOG.debug('@PERF ' + method, 'Total (ms) duration: ' + (Date.now() - t0).toFixed(2));
throw error;
}

// Store the transaction in the transaction registry.
LOG.debug(method, 'Storing executed transaction in Transaction registry');
await txRegistry.add(transaction, {noTest: true});

// Store the transaction in the historian registry.
LOG.debug(method, 'Storing Historian record in Historian registry');
await historian.add(record, {noTest: true});

context.clearTransaction();
LOG.exit(method);
LOG.debug('@PERF ' + method, 'Total (ms) duration: ' + (Date.now() - t0).toFixed(2));
return Promise.resolve();
}

/**
Expand Down
11 changes: 8 additions & 3 deletions packages/composer-runtime/lib/registry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,12 +156,17 @@ class Registry extends EventEmitter {
* @param {boolean} [options.convertResourcesToRelationships] Permit resources
* in the place of relationships, defaults to false.
* @param {boolean} [options.forceAdd] Forces adding the object even if it present (default to false)
* @param {boolean} [options.noTest] skips application of the testAdd method (default to false)
*/
async add(resource, options = {}) {
const error = await this.testAdd(resource);
if (error) {
throw error;

if (!options.noTest){
const error = await this.testAdd(resource);
if (error) {
throw error;
}
}

const id = resource.getIdentifier();
options = Object.assign({}, baseDefaultOptions, options);
let object = this.serializer.toJSON(resource, options);
Expand Down
9 changes: 8 additions & 1 deletion packages/composer-runtime/test/engine.transactions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,13 +186,20 @@ describe('EngineTransactions', () => {
});
});

it('should execute the transaction using a user handler - but with an error', () => {
it('should throw an error if ACL rules prevent interaction with the transaction registry', () => {
mockCompiledScriptBundle.execute.resolves(1);
mockRegistry.testAdd.resolves(new Error('uh-oh'));
return engine.invoke(mockContext, 'submitTransaction', [ JSON.stringify(fakeJSON)])
.should.be.rejectedWith(/uh-oh/);
});

it('should throw an error if ACL rules prevent interaction with the historian registry', () => {
mockCompiledScriptBundle.execute.resolves(1);
mockHistorian.testAdd.resolves(new Error('uh-oh'));
return engine.invoke(mockContext, 'submitTransaction', [ JSON.stringify(fakeJSON)])
.should.be.rejectedWith(/uh-oh/);
});

it('should execute the transaction using multiple system handlers and a user handler', () => {
mockTransactionHandler1.execute.resolves(1);
mockTransactionHandler2.execute.resolves(1);
Expand Down
21 changes: 21 additions & 0 deletions packages/composer-runtime/test/registry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,27 @@ describe('Registry', () => {
return registry.add(mockResource).should.be.rejected;
});

it('should skip testAdd if option provided', () => {
mockDataCollection.add.resolves();
let mockEventHandler = sinon.stub();
registry.on('resourceadded', mockEventHandler);
return registry.add(mockResource, {noTest: true})
.then(() => {
sinon.assert.notCalled(mockAccessController.check);
sinon.assert.calledWith(mockDataCollection.add, 'doge1', {
$registryType: 'Asset',
$registryId: 'doges',
$class: 'org.doge.Doge',
assetId: 'doge1'
});
sinon.assert.calledOnce(mockEventHandler);
sinon.assert.calledWith(mockEventHandler, {
registry: registry,
resource: mockResource
});
});
});

});

describe('#updateAll', () => {
Expand Down