fix(plan): P.attr.workspaces[R.id].role == "OWNER" must be simplified (…

…#2059) Fixes #2058 --------- Signed-off-by: Dennis Buduev <dbuduev@users.noreply.github.com> Co-authored-by: Dennis Buduev <dbuduev@users.noreply.github.com>
cerbos · Mar 20, 2024 · 117f02f · 117f02f
1 parent 739f4cf
commit 117f02f
Show file tree

Hide file tree

Showing 5 changed files with 79 additions and 33 deletions.
diff --git a/internal/engine/planner/internal/utils.go b/internal/engine/planner/internal/utils.go
@@ -60,3 +60,14 @@ func MkCallExpr(op string, args ...*exprpb.Expr) *exprpb.Expr {
 	}
 	return e
 }
+
+func MkSelectExpr(operand *exprpb.Expr, field string) *exprpb.Expr {
+	return &exprpb.Expr{
+		ExprKind: &exprpb.Expr_SelectExpr{
+			SelectExpr: &exprpb.Expr_Select{
+				Operand: operand,
+				Field:   field,
+			},
+		},
+	}
+}
diff --git a/internal/engine/planner/matchers/struct_matcher.go b/internal/engine/planner/matchers/struct_matcher.go
@@ -16,7 +16,7 @@ type exprMatcherFunc func(e *exprpb.Expr) (bool, []*exprpb.Expr)
 
 type exprMatcher struct {
 	f  exprMatcherFunc
-	ns []*exprMatcher
+	ns []*exprMatcher // argument matchers
 }
 
 type ExpressionProcessor interface {
@@ -68,7 +68,12 @@ func getConstExprMatcher(s *structMatcher) *exprMatcher {
 func getStructIndexerExprMatcher(s *structMatcher) *exprMatcher {
 	return &exprMatcher{
 		f: func(e *exprpb.Expr) (bool, []*exprpb.Expr) {
-			if indexExpr := e.GetCallExpr(); indexExpr != nil && indexExpr.Function == operators.Index {
+			ex := e
+			if selExpr := ex.GetSelectExpr(); selExpr != nil {
+				s.field = selExpr.Field
+				ex = selExpr.Operand
+			}
+			if indexExpr := ex.GetCallExpr(); indexExpr != nil && indexExpr.Function == operators.Index {
 				return true, indexExpr.Args
 			}
 			return false, nil
@@ -104,47 +109,49 @@ type structMatcher struct {
 	constExpr   *exprpb.Constant
 	rootMatch   *exprMatcher
 	function    string
+	field       string // optional field. E.g. P.attr[R.id].role == "OWNER"
 }
 
 func (s *structMatcher) Process(e *exprpb.Expr) (bool, *exprpb.Expr, error) {
 	r, err := s.rootMatch.run(e)
-	if err != nil {
+	if err != nil || !r {
 		return false, nil, err
 	}
-	if r {
-		var opts []*exprpb.Expr
-		type entry struct {
-			key   *exprpb.Constant
-			value *exprpb.Expr
-		}
-		entries := make([]entry, 0, len(s.structExpr.Entries))
-		for _, item := range s.structExpr.Entries {
-			if key := item.GetMapKey().GetConstExpr(); key != nil {
-				entries = append(entries, entry{key: key, value: item.GetValue()})
-			}
-		}
-		sort.Slice(entries, func(i, j int) bool {
-			return entries[i].key.String() < entries[j].key.String()
-		})
-		for _, item := range entries {
-			opts = append(opts, mkOption(s.function, item.key, item.value, s.indexerExpr, s.constExpr))
-		}
-		n := len(opts)
-		if n == 0 {
-			return false, e, nil
-		}
-		var output *exprpb.Expr
 
-		if n == 1 {
-			output = opts[0]
-		} else {
-			output = mkLogicalOr(opts)
+	type entry struct {
+		key   *exprpb.Constant
+		value *exprpb.Expr
+	}
+	entries := make([]entry, 0, len(s.structExpr.Entries))
+	for _, item := range s.structExpr.Entries {
+		if key := item.GetMapKey().GetConstExpr(); key != nil {
+			entries = append(entries, entry{key: key, value: item.GetValue()})
+		}
+	}
+	sort.Slice(entries, func(i, j int) bool {
+		return entries[i].key.String() < entries[j].key.String()
+	})
+	opts := make([]*exprpb.Expr, 0, len(entries))
+	for _, item := range entries {
+		v := item.value
+		if s.field != "" {
+			v = internal.MkSelectExpr(item.value, s.field)
 		}
-		internal.UpdateIDs(output)
-		return true, output, nil
+		opts = append(opts, mkOption(s.function, item.key, v, s.indexerExpr, s.constExpr))
 	}
+	n := len(opts)
+	if n == 0 {
+		return false, e, nil
+	}
+	var output *exprpb.Expr
 
-	return false, nil, nil
+	if n == 1 {
+		output = opts[0]
+	} else {
+		output = mkLogicalOr(opts)
+	}
+	internal.UpdateIDs(output)
+	return true, output, nil
 }
 
 var supportedOps = map[string]struct{}{

diff --git a/internal/engine/planner/matchers/struct_matcher_test.go b/internal/engine/planner/matchers/struct_matcher_test.go
@@ -27,6 +27,10 @@ func TestStructMatcher(t *testing.T) {
 		{
 			expr: "P.attr.anyMap[R.attr.Id] == R.attr.value",
 		},
+		{
+			expr: `{"a1": {"role": "OWNER"}}[R.id].role == "OWNER"`,
+			res:  true,
+		},
 		{
 			expr: "P.attr.anyMap[R.attr.Id][R.attr.value]",
 		},

diff --git a/internal/test/testdata/query_planner/policies/resource_policy_report_with_map.yaml b/internal/test/testdata/query_planner/policies/resource_policy_report_with_map.yaml
@@ -62,6 +62,14 @@ resourcePolicy:
       condition:
         match:
           expr: P.attr.workspaceMap[R.attr.workspaceId] == "MANAGER"
+    - actions:
+        - edit_easily
+      effect: EFFECT_ALLOW
+      roles:
+        - USER
+      condition:
+        match:
+          expr: P.attr.workspaceMapRoles[R.attr.workspaceId].role == "MANAGER"
     - actions:
         - delete
       effect: EFFECT_ALLOW

diff --git a/internal/test/testdata/query_planner/suite/report_with_map.yaml b/internal/test/testdata/query_planner/suite/report_with_map.yaml
@@ -15,6 +15,10 @@ principal: {
       "A": "MANAGER",
       "B": "MEMBER",
     },
+    "workspaceMapRoles": {
+      "A": { "role": "MANAGER" },
+      "B": { "role": "MEMBER"},
+    },
     "workspaces": [
       {
         "name": "workspaceA",
@@ -114,6 +118,18 @@ tests:
           operands:
             - variable: request.resource.attr.workspaceId
             - value: "A"
+  - action: edit_easily
+    resource:
+      kind: report_with_map
+      policyVersion: default
+    want:
+      kind: KIND_CONDITIONAL
+      condition:
+        expression:
+          operator: eq
+          operands:
+            - variable: request.resource.attr.workspaceId
+            - value: "A"
   - action: delete
     resource:
       kind: report_with_map