Fix bug where duplicate projects getting returned from OPA in 2.1 cou…

…ld result in * (chef#1963) This is an edge case where the duplicates would be the same length as the total projects in the system. For example, if you have a single custom project named project1 one and two policies that allow user bob on project1 for a specific resource and action, then V2ProjectsAuthorized would return ["project1", "project1"]. Since the total projects in the system would be ["(unassigned)", "project1"], ProjectsAuthorized would incorrectly equate the result of V2ProjectsAuthorized to * due to the fact that their lengths matched. This edge case would happen anytime NumberOfProjectsAllowingAProjectForASpecificResourceAndAction = NumberOfCustomProjects + 1 (for the unassigned project). V2ProjectsAuthorized has been updated to not return duplicates which was never the intended behavior. The duplicates were introduced in this change: chef@c21ddbe Due to the introduction of using a partial document: chef@c21ddbe#diff-2d317bb33ff7babca7e1290b0fb1dcf2R241 Which unfortunately contain duplicates: open-policy-agent/opa#429 Signed-off-by: Tyler Cloke <[email protected]>
cinc-project · Oct 24, 2019 · 5a21a7d · 5a21a7d
1 parent 7172d1f
commit 5a21a7d
Show file tree

Hide file tree

Showing 3 changed files with 143 additions and 4 deletions.
diff --git a/components/authz-service/engine/conformance/conformance_v2_test.go b/components/authz-service/engine/conformance/conformance_v2_test.go
@@ -99,7 +99,6 @@ func TestV2p1ProjectsAuthorized(t *testing.T) {
 				require.NoError(t, err)
 				assert.Equal(t, []string{}, actual)
 			})
-
 			t.Run("policy with one of the requested projects returns matched project", func(t *testing.T) {
 				pol := map[string]interface{}{
 					"members": engine.Subject(sub),
@@ -140,6 +139,72 @@ func TestV2p1ProjectsAuthorized(t *testing.T) {
 				assert.ElementsMatch(t, []string{proj1, proj2}, actual)
 			})
 
+			t.Run("policy with an allowed project that overlaps will return that project", func(t *testing.T) {
+				pol1 := map[string]interface{}{
+					"members": engine.Subject(sub),
+					"statements": map[string]interface{}{
+						"statement-id-0": map[string]interface{}{
+							"role":      "handyman",
+							"resources": []string{res},
+							"effect":    "allow",
+							"projects":  []string{proj1},
+						},
+					},
+				}
+				pol2 := map[string]interface{}{
+					"members": engine.Subject(sub),
+					"statements": map[string]interface{}{
+						"statement-id-0": map[string]interface{}{
+							"actions":   []string{act},
+							"resources": []string{res},
+							"effect":    "allow",
+							"projects":  []string{proj1},
+						},
+					},
+				}
+				role := map[string]interface{}{
+					"id":      "handyman",
+					"actions": []string{act},
+				}
+				setPoliciesV2p1(t, e, pol1, pol2, role)
+				actual, err := e.V2ProjectsAuthorized(args([]string{proj1, proj2}))
+				require.NoError(t, err)
+				assert.ElementsMatch(t, []string{proj1}, actual)
+			})
+
+			t.Run("policies with some of the requested projects that contain duplicates returns matched projects without duplicates", func(t *testing.T) {
+				pol1 := map[string]interface{}{
+					"members": engine.Subject(sub),
+					"statements": map[string]interface{}{
+						"statement-id-0": map[string]interface{}{
+							"role":      "handyman",
+							"resources": []string{res},
+							"effect":    "allow",
+							"projects":  []string{proj1, "other-project", proj2},
+						},
+					},
+				}
+				pol2 := map[string]interface{}{
+					"members": engine.Subject(sub),
+					"statements": map[string]interface{}{
+						"statement-id-0": map[string]interface{}{
+							"actions":   []string{act},
+							"resources": []string{res},
+							"effect":    "allow",
+							"projects":  []string{"other-project", proj2},
+						},
+					},
+				}
+				role := map[string]interface{}{
+					"id":      "handyman",
+					"actions": []string{act},
+				}
+				setPoliciesV2p1(t, e, pol1, pol2, role)
+				actual, err := e.V2ProjectsAuthorized(args([]string{proj1, proj2}))
+				require.NoError(t, err)
+				assert.ElementsMatch(t, []string{proj1, proj2}, actual)
+			})
+
 			t.Run("policy with no matching projects returns empty list", func(t *testing.T) {
 				pol := map[string]interface{}{
 					"members": engine.Subject(sub),

diff --git a/components/authz-service/engine/opa/opa.go b/components/authz-service/engine/opa/opa.go
@@ -5,6 +5,7 @@ package opa
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"os"
 	"strings"
@@ -286,7 +287,12 @@ func dumpData(ctx context.Context, store storage.Store, l logger.Logger) error {
 		return err
 	}
 
-	l.Infof("data: %#v", data)
+	jsonData, err := json.Marshal(data)
+	if err != nil {
+		return err
+	}
+
+	l.Infof("data: %s", jsonData)
 	return store.Commit(ctx, txn)
 }
 
@@ -549,13 +555,18 @@ func (s *State) stringArrayFromResults(exps []*rego.ExpressionValue) ([]string,
 }
 
 func (s *State) projectsFromPreparedEvalQuery(rs rego.ResultSet) ([]string, error) {
-	result := make([]string, len(rs))
+	projectsFound := make(map[string]bool, len(rs))
+	result := make([]string, 0, len(rs))
 	for i := range rs {
 		var ok bool
-		result[i], ok = rs[i].Bindings["project"].(string)
+		proj, ok := rs[i].Bindings["project"].(string)
 		if !ok {
 			return nil, &UnexpectedResultExpressionError{exps: rs[i].Expressions}
 		}
+		if !projectsFound[proj] {
+			result = append(result, proj)
+			projectsFound[proj] = true
+		}
 	}
 	return result, nil
 }

diff --git a/components/authz-service/server/v2/system_integration_test.go b/components/authz-service/server/v2/system_integration_test.go
@@ -11,6 +11,7 @@ import (
 	"google.golang.org/grpc/codes"
 
 	api_v2 "github.com/chef/automate/api/interservice/authz/v2"
+	constants_v2 "github.com/chef/automate/components/authz-service/constants/v2"
 	"github.com/chef/automate/components/authz-service/testhelpers"
 	"github.com/chef/automate/lib/grpc/grpctest"
 )
@@ -59,6 +60,68 @@ func TestIntegrationSystemPolicies(t *testing.T) {
 	}
 }
 
+func TestIntegrationValidateProjectAssignmentWithOverlappingProjects(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	ts := setupWithOPAV2p1(t)
+	defer ts.Shutdown(t, ctx)
+	cl := ts.Authz
+
+	user := "user:local:dave"
+	authorizedProjectId := "authorized-project"
+
+	_, err := ts.Projects.CreateProject(ctx, &api_v2.CreateProjectReq{
+		Id:   authorizedProjectId,
+		Name: "Project Authorized",
+	})
+	require.NoError(t, err)
+
+	statement1 := api_v2.Statement{
+		Effect:    api_v2.Statement_ALLOW,
+		Resources: []string{"*"},
+		Actions:   []string{"*"},
+		Projects:  []string{authorizedProjectId},
+	}
+	req1 := api_v2.CreatePolicyReq{
+		Id:         "policy-1",
+		Name:       "my favorite policy",
+		Members:    []string{user},
+		Statements: []*api_v2.Statement{&statement1},
+	}
+	_, err = ts.Policy.CreatePolicy(ctx, &req1)
+	require.NoError(t, err)
+
+	statement2 := api_v2.Statement{
+		Effect:    api_v2.Statement_ALLOW,
+		Resources: []string{"*"},
+		// Project Owner contains iam:teams:list
+		Role:     constants_v2.ProjectOwnerRoleID,
+		Projects: []string{authorizedProjectId},
+	}
+	req2 := api_v2.CreatePolicyReq{
+		Id:         "policy-2",
+		Name:       "my second favorite policy",
+		Members:    []string{user},
+		Statements: []*api_v2.Statement{&statement2},
+	}
+	_, err = ts.Policy.CreatePolicy(ctx, &req2)
+	require.NoError(t, err)
+
+	// force sync refresh to load new policies
+	err = ts.PolicyRefresher.Refresh(ctx)
+	require.NoError(t, err)
+
+	t.Run("when there are policies granting overlapping permissions on the same project, only that project is returned", func(t *testing.T) {
+		resp, err := cl.ProjectsAuthorized(ctx, &api_v2.ProjectsAuthorizedReq{
+			Subjects: []string{user},
+			Resource: "iam:teams",
+			Action:   "iam:teams:list",
+		})
+		assert.NoError(t, err)
+		assert.ElementsMatch(t, []string{authorizedProjectId}, resp.Projects)
+	})
+}
+
 // bug: this test passes contingent on the above test being run.
 // issue seems to be related to context metadata
 func TestIntegrationValidateProjectAssignment(t *testing.T) {