Solution for cyclic ACL rules (hyperledger-archives#4157)

* possible solution; g Signed-off-by: Matthew B White <[email protected]> * update error messages Signed-off-by: Matthew B White <[email protected]> * update tests Signed-off-by: Matthew B White <[email protected]>
rossPy · Jun 25, 2018 · 4473d27 · 4473d27
1 parent 3481c7e
commit 4473d27
Show file tree

Hide file tree

Showing 3 changed files with 45 additions and 2 deletions.
diff --git a/packages/composer-runtime/lib/accesscontroller.js b/packages/composer-runtime/lib/accesscontroller.js
@@ -39,6 +39,7 @@ class AccessController {
         this.context = context;
         this.participant = null;
         this.transaction = null;
+        this.aclRuleStack=[];
         LOG.exit(method);
     }
 
@@ -120,7 +121,8 @@ class AccessController {
 
         // Iterate over the ACL rules in order, but stop at the first rule
         // that permits the action.
-        return filteredAclRules.reduce((promise, aclRule) => {
+        return filteredAclRules.reduce((promise, aclRule,currentIndex) => {
+            LOG.debug(`Processing rule ${aclRule.getName()}, index ${currentIndex} `);
             return promise.then((result) => {
                 if (result) {
                     return result;
@@ -211,11 +213,32 @@ class AccessController {
     checkRule(resource, access, participant, transaction, aclRule) {
         const method = 'checkRule';
         LOG.entry(method, resource, access, participant, transaction, aclRule);
+        let pid='',tx='';
+        if (participant){
+            pid = participant.getFullyQualifiedIdentifier();
+        }
+        if (transaction){
+            tx = transaction.getFullyQualifiedIdentifier();
+        }
+        let checkId = `${aclRule.getName()}/${access}/${pid}/${tx}/`;
+        if (this.aclRuleStack.includes(checkId)){
+            this.aclRuleStack=[];
+
+            // This must be an explicit deny rule, so throw.
+            let e = new Error('Cyclic ACL Rule detected, rule condition is invoking the same rule');
+            LOG.error(method, e);
+            this.aclRuleStack=[];
+            return Promise.reject(e);
+        }
+        this.aclRuleStack.push(checkId);
 
         // Is the predicate met?
         return this.matchPredicate(resource, participant, transaction, aclRule)
             .then((result) => {
 
+                // pop...
+                this.aclRuleStack.pop();
+
                 // No, predicate not met.
                 if (!result) {
                     LOG.debug(method, 'Predicate does not match');
@@ -232,6 +255,7 @@ class AccessController {
                 // This must be an explicit deny rule, so throw.
                 let e = new AccessException(resource, access, participant, transaction);
                 LOG.error(method, e);
+                this.aclRuleStack=[];
                 throw e;
 
             });

diff --git a/packages/composer-runtime/lib/registry.js b/packages/composer-runtime/lib/registry.js
@@ -107,7 +107,8 @@ class Registry extends EventEmitter {
                 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`);
+                let e = new Error(`Object with ID '${id}' in collection with ID '${this.type}:${this.id}' does not exist; [cause=${error.message}]`);
+                throw e;
             }
         }
     }

diff --git a/packages/composer-runtime/test/accesscontroller.js b/packages/composer-runtime/test/accesscontroller.js
@@ -380,6 +380,24 @@ describe('AccessController', () => {
                 .should.be.rejectedWith(AccessException, /does not have/);
         });
 
+        it('should reject with cyclic error if rule seen before', () => {
+            setAclFile('rule R1 {description: "Test R1" participant: "org.acme.test.TestParticipant#P5678" operation: READ resource: "org.acme.test.TestAsset#A1234" condition: (true) action: ALLOW}');
+
+             // pretend that we've been here before....
+            controller.aclRuleStack.push('R1/READ/org.acme.test.TestParticipant#P5678/org.acme.test.TestTransaction#T9012/');
+            return controller.checkRule(asset, 'READ', participant, transaction, aclManager.getAclRules()[0]).
+            should.be.rejectedWith(/Cyclic ACL Rule detected/);
+        });
+
+        it('should reject with cyclic error if rule seen before - minimal data', () => {
+            setAclFile('rule R1 {description: "Test R1" participant: "org.acme.test.TestParticipant#P5678" operation: READ resource: "org.acme.test.TestAsset#A1234" condition: (true) action: ALLOW}');
+
+             // pretend that we've been here before....
+            controller.aclRuleStack.push('R1/READ///');
+            return controller.checkRule(asset, 'READ', null, null, aclManager.getAclRules()[0]).
+            should.be.rejectedWith(/Cyclic ACL Rule detected/);
+        });
+
     });
 
     describe('#matchNoun', () => {