Merge pull request #11425 from liggitt/tolerate-evaluation-errors

Merged by openshift-bot

pkg/project/auth/cache.go

@@ -1,6 +1,7 @@
 package auth
 
 import (
+	"errors"
 	"fmt"
 	"strings"
 	"sync"
@@ -413,6 +414,10 @@ func (ac *AuthorizationCache) syncRequest(request *reviewRequest, userSubjectRec
 	addSubjectsToNamespace(groupSubjectRecordStore, review.Groups(), namespace)
 	cacheReviewRecord(request, lastKnownValue, review, reviewRecordStore)
 	ac.notifyWatchers(namespace, lastKnownValue, sets.NewString(review.Users()...), sets.NewString(review.Groups()...))
+
+	if errMsg := review.EvaluationError(); len(errMsg) > 0 {
+		return errors.New(errMsg)
+	}
 	return nil
 }
 

pkg/project/auth/cache_test.go

@@ -88,6 +88,7 @@ func (this *MockPolicyClient) LastSyncResourceVersion() string {
 type mockReview struct {
 	users  []string
 	groups []string
+	err    string
 }
 
 // Users returns the users that can access a resource
@@ -100,6 +101,10 @@ func (r *mockReview) Groups() []string {
 	return r.groups
 }
 
+func (r *mockReview) EvaluationError() string {
+	return r.err
+}
+
 // common test users
 var (
 	alice = &user.DefaultInfo{

pkg/project/auth/reviewer.go

@@ -12,11 +12,13 @@ import (
 type Review interface {
 	Users() []string
 	Groups() []string
+	EvaluationError() string
 }
 
 type defaultReview struct {
-	users  []string
-	groups []string
+	users           []string
+	groups          []string
+	evaluationError string
 }
 
 func (r *defaultReview) Users() []string {
@@ -28,6 +30,10 @@ func (r *defaultReview) Groups() []string {
 	return r.groups
 }
 
+func (r *defaultReview) EvaluationError() string {
+	return r.evaluationError
+}
+
 type review struct {
 	response *authorizationapi.ResourceAccessReviewResponse
 }
@@ -42,6 +48,10 @@ func (r *review) Groups() []string {
 	return r.response.Groups.List()
 }
 
+func (r *review) EvaluationError() string {
+	return r.response.EvaluationError
+}
+
 // Reviewer performs access reviews for a project by name
 type Reviewer interface {
 	Review(name string) (Review, error)
@@ -98,13 +108,12 @@ func (r *authorizerReviewer) Review(namespaceName string) (Review, error) {
 
 	ctx := kapi.WithNamespace(kapi.NewContext(), namespaceName)
 	users, groups, err := r.policyChecker.GetAllowedSubjects(ctx, attributes)
-	if err != nil {
-		return nil, err
-	}
-
 	review := &defaultReview{
 		users:  users.List(),
 		groups: groups.List(),
 	}
+	if err != nil {
+		review.evaluationError = err.Error()
+	}
 	return review, nil
 }

test/integration/project_test.go

@@ -7,8 +7,10 @@ import (
 
 	kapi "k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/util/sets"
+	"k8s.io/kubernetes/pkg/util/wait"
 	"k8s.io/kubernetes/pkg/watch"
 
+	authorizationapi "github.com/openshift/origin/pkg/authorization/api"
 	"github.com/openshift/origin/pkg/authorization/authorizer/scope"
 	buildapi "github.com/openshift/origin/pkg/build/api"
 	policy "github.com/openshift/origin/pkg/cmd/admin/policy"
@@ -427,6 +429,110 @@ func TestScopedProjectAccess(t *testing.T) {
 	waitForOnlyDelete("one", oneTwoWatch, t)
 }
 
+func TestInvalidRoleRefs(t *testing.T) {
+	testutil.RequireEtcd(t)
+	defer testutil.DumpEtcdOnFailure(t)
+	_, clusterAdminKubeConfig, err := testserver.StartTestMaster()
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	clusterAdminClient, err := testutil.GetClusterAdminClient(clusterAdminKubeConfig)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	clusterAdminClientConfig, err := testutil.GetClusterAdminClientConfig(clusterAdminKubeConfig)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	bobClient, _, _, err := testutil.GetClientForUser(*clusterAdminClientConfig, "bob")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	aliceClient, _, _, err := testutil.GetClientForUser(*clusterAdminClientConfig, "alice")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	if _, err := testserver.CreateNewProject(clusterAdminClient, *clusterAdminClientConfig, "foo", "bob"); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if _, err := testserver.CreateNewProject(clusterAdminClient, *clusterAdminClientConfig, "bar", "alice"); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	roleName := "missing-role"
+	if _, err := clusterAdminClient.ClusterRoles().Create(&authorizationapi.ClusterRole{ObjectMeta: kapi.ObjectMeta{Name: roleName}}); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	modifyRole := &policy.RoleModificationOptions{RoleName: roleName, Users: []string{"someuser"}}
+	// mess up rolebindings in "foo"
+	modifyRole.RoleBindingAccessor = policy.NewLocalRoleBindingAccessor("foo", clusterAdminClient)
+	if err := modifyRole.AddRole(); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	// mess up rolebindings in "bar"
+	modifyRole.RoleBindingAccessor = policy.NewLocalRoleBindingAccessor("bar", clusterAdminClient)
+	if err := modifyRole.AddRole(); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	// mess up clusterrolebindings
+	modifyRole.RoleBindingAccessor = policy.NewClusterRoleBindingAccessor(clusterAdminClient)
+	if err := modifyRole.AddRole(); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	// Orphan the rolebindings by deleting the role
+	if err := clusterAdminClient.ClusterRoles().Delete(roleName); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	// wait for evaluation errors to show up in both namespaces and at cluster scope
+	if err := wait.PollImmediate(100*time.Millisecond, 10*time.Second, func() (bool, error) {
+		review := &authorizationapi.ResourceAccessReview{Action: authorizationapi.Action{Verb: "get", Resource: "pods"}}
+		review.Action.Namespace = "foo"
+		if resp, err := clusterAdminClient.ResourceAccessReviews().Create(review); err != nil || resp.EvaluationError == "" {
+			return false, err
+		}
+		review.Action.Namespace = "bar"
+		if resp, err := clusterAdminClient.ResourceAccessReviews().Create(review); err != nil || resp.EvaluationError == "" {
+			return false, err
+		}
+		review.Action.Namespace = ""
+		if resp, err := clusterAdminClient.ResourceAccessReviews().Create(review); err != nil || resp.EvaluationError == "" {
+			return false, err
+		}
+		return true, nil
+	}); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	// Make sure bob still sees his project (and only his project)
+	if projects, err := bobClient.Projects().List(kapi.ListOptions{}); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	} else if hasErr := hasExactlyTheseProjects(projects, sets.NewString("foo")); hasErr != nil {
+		t.Error(hasErr)
+	}
+	// Make sure alice still sees her project (and only her project)
+	if projects, err := aliceClient.Projects().List(kapi.ListOptions{}); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	} else if hasErr := hasExactlyTheseProjects(projects, sets.NewString("bar")); hasErr != nil {
+		t.Error(hasErr)
+	}
+	// Make sure cluster admin still sees all projects
+	if projects, err := clusterAdminClient.Projects().List(kapi.ListOptions{}); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	} else {
+		projectNames := sets.NewString()
+		for _, project := range projects.Items {
+			projectNames.Insert(project.Name)
+		}
+		if !projectNames.HasAll("foo", "bar", "openshift-infra", "openshift", "default") {
+			t.Errorf("Expected projects foo and bar, got %v", projectNames.List())
+		}
+	}
+}
+
 func hasExactlyTheseProjects(list *projectapi.ProjectList, projects sets.String) error {
 	if len(list.Items) != len(projects) {
 		return fmt.Errorf("expected %v, got %v", projects, list.Items)