fix: Only produce outputs for activated rules (#1631)

* fix: Only produce outputs for activated rules Rule output expressions should only be evaluated and appended to the API response if the rule is activated during the processing of the request. Fixes #1630 Signed-off-by: Charith Ellawala <charith@cerbos.dev> * Move once to correct place Signed-off-by: Charith Ellawala <charith@cerbos.dev> * Remove sync.Once and update tests Signed-off-by: Charith Ellawala <charith@cerbos.dev> --------- Signed-off-by: Charith Ellawala <charith@cerbos.dev>
cerbos · Jun 8, 2023 · 5925445 · 5925445
1 parent a6fba65
commit 5925445
Show file tree

Hide file tree

Showing 10 changed files with 67 additions and 58 deletions.
diff --git a/internal/engine/evaluator.go b/internal/engine/evaluator.go
@@ -156,23 +156,12 @@ func (rpe *resourcePolicyEvaluator) Evaluate(ctx context.Context, tctx tracer.Co
 		for _, rule := range p.Rules {
 			rctx := sctx.StartRule(rule.Name)
 
-			if rule.Output != nil {
-				octx := rctx.StartOutput(rule.Name)
-
-				output := &enginev1.OutputEntry{
-					Src: namer.RuleFQN(rpe.policy.Meta, p.Scope, rule.Name),
-					Val: rpe.evalParams.evaluateProtobufValueCELExpr(rule.Output.Checked, variables, input),
-				}
-				result.Outputs = append(result.Outputs, output)
-
-				octx.ComputedOutput(output)
-			}
-
 			if !internal.SetIntersects(rule.Roles, effectiveRoles) && !internal.SetIntersects(rule.DerivedRoles, effectiveDerivedRoles) {
 				rctx.Skipped(nil, "No matching roles or derived roles")
 				continue
 			}
 
+			ruleActivated := false
 			for actionGlob := range rule.Actions {
 				matchedActions := util.FilterGlob(actionGlob, actionsToResolve)
 				for _, action := range matchedActions {
@@ -190,8 +179,22 @@ func (rpe *resourcePolicyEvaluator) Evaluate(ctx context.Context, tctx tracer.Co
 
 					result.setEffect(action, EffectInfo{Effect: rule.Effect, Policy: policyKey, Scope: p.Scope})
 					actx.AppliedEffect(rule.Effect, "")
+					ruleActivated = true
 				}
 			}
+
+			// evaluate output expression if the rule was activated
+			if ruleActivated && rule.Output != nil {
+				octx := rctx.StartOutput(rule.Name)
+
+				output := &enginev1.OutputEntry{
+					Src: namer.RuleFQN(rpe.policy.Meta, p.Scope, rule.Name),
+					Val: rpe.evalParams.evaluateProtobufValueCELExpr(rule.Output.Checked, variables, input),
+				}
+				result.Outputs = append(result.Outputs, output)
+
+				octx.ComputedOutput(output)
+			}
 		}
 	}
 
@@ -238,6 +241,7 @@ func (ppe *principalPolicyEvaluator) Evaluate(ctx context.Context, tctx tracer.C
 
 			for _, rule := range resourceRules.ActionRules {
 				matchedActions := util.FilterGlob(rule.Action, actionsToResolve)
+				ruleActivated := false
 				for _, action := range matchedActions {
 					actx := rctx.StartAction(action)
 					ok, err := ppe.evalParams.satisfiesCondition(actx.StartCondition(), rule.Condition, variables, input)
@@ -250,11 +254,14 @@ func (ppe *principalPolicyEvaluator) Evaluate(ctx context.Context, tctx tracer.C
 						actx.Skipped(nil, "Condition not satisfied")
 						continue
 					}
+
 					result.setEffect(action, EffectInfo{Effect: rule.Effect, Policy: policyKey, Scope: p.Scope})
 					actx.AppliedEffect(rule.Effect, "")
+					ruleActivated = true
 				}
 
-				if rule.Output != nil {
+				// evaluate output expression if the rule was activated
+				if ruleActivated && rule.Output != nil {
 					octx := rctx.StartOutput(rule.Name)
 
 					output := &enginev1.OutputEntry{

diff --git a/internal/server/tests.go b/internal/server/tests.go
@@ -390,7 +390,7 @@ func (tr *TestRunner) checkCORS(c *http.Client, hostAddr string) func(*testing.T
 	}
 }
 
-func compareProto(t *testing.T, want, have interface{}) {
+func compareProto(t *testing.T, want, have proto.Message) {
 	t.Helper()
 
 	require.Empty(t, cmp.Diff(want, have,

diff --git a/internal/storage/bundle/remote_source_test.go b/internal/storage/bundle/remote_source_test.go
@@ -19,7 +19,7 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-const bundleID = "h1:EVPzJ8jAvo0+qEaXJPe8roBCAdfGAdowddMAPMkxZY8="
+const bundleID = "h1:441rLQQdIotHlkzP1SOoFBQHdLA4VNfxAgr8zWv7ip4="
 
 func TestRemoteSource(t *testing.T) {
 	bundlePath := filepath.Join(test.PathToDir(t, "bundle"), "bundle.crbp")

diff --git a/internal/test/testdata/bundle/bundle.crbp b/internal/test/testdata/bundle/bundle.crbp
diff --git a/internal/test/testdata/bundle/key.txt b/internal/test/testdata/bundle/key.txt
@@ -0,0 +1,3 @@
+# created: 2023-06-08T09:55:24+01:00
+# public key: age1rlszmt4d7lu6wjkj00qdepya8anq8zcujyw8dqskcd3spjg2cs2sgerh0q
+AGE-SECRET-KEY-14SPXE0NZUG25FE5RW5PD7FKGPKP5CZAG53TR6HHAAN7NPMYP0YES4RQFVS
diff --git a/internal/test/testdata/bundle/manifest.json b/internal/test/testdata/bundle/manifest.json
@@ -1,32 +1,32 @@
 {
-  "apiVersion": "api.cerbos.cloud/v1",
-  "policyIndex": {
-    "cerbos.principal.daisy_duck.vdefault": "policies/1E0CD2615BE3760B",
-    "cerbos.principal.donald_duck.v20210210": "policies/1CC91E277263674D",
-    "cerbos.principal.donald_duck.vdefault": "policies/2F26F3660A97BECD",
-    "cerbos.principal.donald_duck.vdefault/acme": "policies/4C6062CFBE8627DB",
-    "cerbos.principal.donald_duck.vdefault/acme.hr": "policies/60C13DA6FD1DF10C",
-    "cerbos.principal.terry_tibbs.vdefault": "policies/BCD8E5E5F425900C",
-    "cerbos.resource.account.vdefault": "policies/EE58DE7B0F7DA6B9",
-    "cerbos.resource.album_object.vdefault": "policies/684ED25FF04663F2",
-    "cerbos.resource.equipment_request.vdefault": "policies/51F53279EC2F9999",
-    "cerbos.resource.equipment_request.vdefault/acme": "policies/4E007D10C67C7E53",
-    "cerbos.resource.leave_request.v20210210": "policies/EAE733C912F22DA",
-    "cerbos.resource.leave_request.vdefault": "policies/EF8C9BEB57967AA0",
-    "cerbos.resource.leave_request.vdefault/acme": "policies/827F815FFA0C2AC5",
-    "cerbos.resource.leave_request.vdefault/acme.hr": "policies/FBAEBB70E5F8BBE6",
-    "cerbos.resource.leave_request.vdefault/acme.hr.uk": "policies/BC5AE9FB81ACF7B2",
-    "cerbos.resource.leave_request.vstaging": "policies/AD56144EE420A347",
-    "cerbos.resource.purchase_order.vdefault": "policies/B55EE74E34DF4F2B"
+  "apiVersion":  "api.cerbos.cloud/v1",
+  "policyIndex":  {
+    "cerbos.principal.daisy_duck.vdefault":  "policies/1E0CD2615BE3760B",
+    "cerbos.principal.donald_duck.v20210210":  "policies/1CC91E277263674D",
+    "cerbos.principal.donald_duck.vdefault":  "policies/2F26F3660A97BECD",
+    "cerbos.principal.donald_duck.vdefault/acme":  "policies/4C6062CFBE8627DB",
+    "cerbos.principal.donald_duck.vdefault/acme.hr":  "policies/60C13DA6FD1DF10C",
+    "cerbos.principal.terry_tibbs.vdefault":  "policies/BCD8E5E5F425900C",
+    "cerbos.resource.account.vdefault":  "policies/EE58DE7B0F7DA6B9",
+    "cerbos.resource.album_object.vdefault":  "policies/684ED25FF04663F2",
+    "cerbos.resource.equipment_request.vdefault":  "policies/51F53279EC2F9999",
+    "cerbos.resource.equipment_request.vdefault/acme":  "policies/4E007D10C67C7E53",
+    "cerbos.resource.leave_request.v20210210":  "policies/EAE733C912F22DA",
+    "cerbos.resource.leave_request.vdefault":  "policies/EF8C9BEB57967AA0",
+    "cerbos.resource.leave_request.vdefault/acme":  "policies/827F815FFA0C2AC5",
+    "cerbos.resource.leave_request.vdefault/acme.hr":  "policies/FBAEBB70E5F8BBE6",
+    "cerbos.resource.leave_request.vdefault/acme.hr.uk":  "policies/BC5AE9FB81ACF7B2",
+    "cerbos.resource.leave_request.vstaging":  "policies/AD56144EE420A347",
+    "cerbos.resource.purchase_order.vdefault":  "policies/B55EE74E34DF4F2B"
   },
-  "schemas": [
+  "schemas":  [
     "principal.json",
     "resources/leave_request.json",
     "resources/purchase_order.json",
     "resources/salary_record.json"
   ],
-  "meta": {
-    "identifier": "h1:EVPzJ8jAvo0+qEaXJPe8roBCAdfGAdowddMAPMkxZY8=",
-    "source": "/Users/samuellock/cerbos/cerbos/internal/test/testdata/store"
+  "meta":  {
+    "identifier":  "h1:441rLQQdIotHlkzP1SOoFBQHdLA4VNfxAgr8zWv7ip4=",
+    "source":  "/home/cell/work/cerbos/internal/test/testdata/store"
   }
 }
diff --git a/internal/test/testdata/bundle/secret_key.txt b/internal/test/testdata/bundle/secret_key.txt
@@ -1 +1 @@
-CERBOS-1SomeFakeWorkspaceIDSamlMadeUpSoItCouldBeCutOut-PVCDJFES5KDVJPL2Y8U6NEW7W6NHL5Z8HG7KSUGJRTCWSN4RJW4Q8HVPZE
+CERBOS-1HDHY70IHFLXD-4SPXE0NZUG25FE5RW5PD7FKGPKP5CZAG53TR6HHAAN7NPMYP0YES4RQFVS
diff --git a/internal/test/testdata/server/checks/check_resources/cr_case_06.yaml b/internal/test/testdata/server/checks/check_resources/cr_case_06.yaml
@@ -94,10 +94,6 @@ checkResources:
                 "nested_formatted_string": "id:john"
               },
             }
-          },
-          {
-            "src": "resource.equipment_request.vdefault#rule-002",
-            "val": "approval_status:john:DRAFT"
           }
         ]
       },
@@ -127,10 +123,6 @@ checkResources:
                 "nested_formatted_string": "id:john"
               },
             }
-          },
-          {
-            "src": "resource.equipment_request.vdefault#rule-002",
-            "val": "approval_status:john:DRAFT"
           }
         ]
       }

diff --git a/internal/test/testdata/server/checks/check_resources/cr_case_07.yaml b/internal/test/testdata/server/checks/check_resources/cr_case_07.yaml
@@ -22,7 +22,8 @@ checkResources:
         "actions": [
           "view:public",
           "approve",
-          "create"
+          "create",
+          "delete"
         ],
         "resource": {
           "kind": "equipment_request",
@@ -50,11 +51,12 @@ checkResources:
         "actions": {
           "view:public": "EFFECT_ALLOW",
           "approve": "EFFECT_DENY",
-          "create": "EFFECT_ALLOW"
+          "create": "EFFECT_ALLOW",
+          "delete": "EFFECT_DENY",
         },
         "outputs": [
           {
-            "src": "principal.terry_tibbs.vdefault#equipment_request_rule-001",
+            "src": "principal.terry_tibbs.vdefault#create-rule",
             "val": ["foo", ["bar", true]]
           },
           {
@@ -72,11 +74,7 @@ checkResources:
                 "nested_formatted_string": "id:terry_tibbs"
               },
             }
-          },
-          {
-            "src": "resource.equipment_request.vdefault#rule-002",
-            "val": "approval_status:terry_tibbs:DRAFT"
-          },
+          }
         ]
       }
     ]

diff --git a/internal/test/testdata/store/principal_policies/policy_04.yaml b/internal/test/testdata/store/principal_policies/policy_04.yaml
@@ -6,8 +6,17 @@ principalPolicy:
   rules:
     - resource: equipment_request
       actions:
-        - action: "create"
+        - name: reject-rule
+          action: "reject"
+          effect: EFFECT_ALLOW
+          output:
+            expr: |-
+              ["foo"]
+
+        - name: create-rule
+          action: "create"
           effect: EFFECT_ALLOW
           output:
             expr: |-
               ["foo", ["bar", true]]
+