From d3b5c424c4894463a1bb94b1b6c85701af17cdf6 Mon Sep 17 00:00:00 2001 From: deads2k Date: Fri, 27 Feb 2015 09:08:52 -0500 Subject: [PATCH 1/3] add subject access review integration tests --- pkg/client/client.go | 4 + pkg/client/subjectaccessreview.go | 19 ++ test/integration/authorization_test.go | 235 +++++++++++++++++++++---- 3 files changed, 222 insertions(+), 36 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index 6801b73e7da7..a5015aa4a0b3 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -132,6 +132,10 @@ func (c *Client) SubjectAccessReviews(namespace string) SubjectAccessReviewInter return newSubjectAccessReviews(c, namespace) } +func (c *Client) RootSubjectAccessReviews() SubjectAccessReviewInterface { + return newRootSubjectAccessReviews(c) +} + // Client is an OpenShift client object type Client struct { *kclient.RESTClient diff --git a/pkg/client/subjectaccessreview.go b/pkg/client/subjectaccessreview.go index 98a27acbadc4..498f16a1890c 100644 --- a/pkg/client/subjectaccessreview.go +++ b/pkg/client/subjectaccessreview.go @@ -34,3 +34,22 @@ func (c *subjectAccessReviews) Create(policy *authorizationapi.SubjectAccessRevi err = c.r.Post().Namespace(c.ns).Resource("subjectAccessReviews").Body(policy).Do().Into(result) return } + +// rootSubjectAccessReviews implements RootSubjectAccessReviews interface +type rootSubjectAccessReviews struct { + r *Client +} + +// newRootSubjectAccessReviews returns a rootSubjectAccessReviews +func newRootSubjectAccessReviews(c *Client) *rootSubjectAccessReviews { + return &rootSubjectAccessReviews{ + r: c, + } +} + +// Create creates new policy. Returns the server's representation of the policy and error if one occurs. +func (c *rootSubjectAccessReviews) Create(policy *authorizationapi.SubjectAccessReview) (result *authorizationapi.SubjectAccessReviewResponse, err error) { + result = &authorizationapi.SubjectAccessReviewResponse{} + err = c.r.Post().Resource("subjectAccessReviews").Body(policy).Do().Into(result) + return +} diff --git a/test/integration/authorization_test.go b/test/integration/authorization_test.go index 73be9dcbbfb5..97595fd18bf7 100644 --- a/test/integration/authorization_test.go +++ b/test/integration/authorization_test.go @@ -11,6 +11,7 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/util" authorizationapi "github.com/openshift/origin/pkg/authorization/api" + "github.com/openshift/origin/pkg/client" policy "github.com/openshift/origin/pkg/cmd/experimental/policy" ) @@ -82,6 +83,42 @@ func TestRestrictedAccessForProjectAdmins(t *testing.T) { var globalClusterAdminUsers = util.NewStringSet("system:kube-client", "system:openshift-client", "system:openshift-deployer") var globalClusterAdminGroups = util.NewStringSet("system:cluster-admins") +type resourceAccessReviewTest struct { + clientInterface client.ResourceAccessReviewInterface + review *authorizationapi.ResourceAccessReview + + // TODO use resource access review response once internal types use util.StringSet + users util.StringSet + groups util.StringSet + namespace string + err string +} + +func (test resourceAccessReviewTest) run(t *testing.T) { + actualResponse, err := test.clientInterface.Create(test.review) + if len(test.err) > 0 { + if err == nil { + t.Errorf("Expected error: %v", test.err) + } else if !strings.Contains(err.Error(), test.err) { + t.Errorf("expected %v, got %v", test.err, err) + } + } else { + if err != nil { + t.Errorf("unexpected error: %v", err) + } + } + + expectedResponse := authorizationapi.ResourceAccessReviewResponse{ + Users: test.users.List(), + Groups: test.groups.List(), + Namespace: test.namespace, + } + + if reflect.DeepEqual(actualResponse, expectedResponse) { + t.Errorf("%#v: expected %v, got %v", test.review, expectedResponse, actualResponse) + } +} + func TestResourceAccessReview(t *testing.T) { startConfig, err := StartTestMaster() if err != nil { @@ -126,55 +163,181 @@ func TestResourceAccessReview(t *testing.T) { requestWhoCanViewDeployments := &authorizationapi.ResourceAccessReview{Verb: "get", Resource: "deployments"} - whoCanViewDeploymentInHammer, err := haroldClient.ResourceAccessReviews("hammer-project").Create(requestWhoCanViewDeployments) - if err != nil { - t.Errorf("unexpected error: %v", err) + { + test := resourceAccessReviewTest{ + clientInterface: haroldClient.ResourceAccessReviews("hammer-project"), + review: requestWhoCanViewDeployments, + users: util.NewStringSet("anypassword:harold", "anypassword:valerie"), + groups: globalClusterAdminGroups, + namespace: "hammer-project", + } + test.users.Insert(globalClusterAdminUsers.List()...) + test.run(t) } - expectedHammerUsers := util.NewStringSet("anypassword:harold", "anypassword:valerie") - expectedHammerUsers.Insert(globalClusterAdminUsers.List()...) - expectedHammerGroups := globalClusterAdminGroups - actualHammerUsers := util.NewStringSet(whoCanViewDeploymentInHammer.Users...) - actualHammerGroups := util.NewStringSet(whoCanViewDeploymentInHammer.Groups...) - if !reflect.DeepEqual(expectedHammerGroups.List(), actualHammerGroups.List()) { - t.Errorf("expected %v, got %v", expectedHammerGroups.List(), actualHammerGroups.List()) + { + test := resourceAccessReviewTest{ + clientInterface: markClient.ResourceAccessReviews("mallet-project"), + review: requestWhoCanViewDeployments, + users: util.NewStringSet("anypassword:mark", "anypassword:edgar"), + groups: globalClusterAdminGroups, + namespace: "mallet-project", + } + test.users.Insert(globalClusterAdminUsers.List()...) + test.run(t) } - if !reflect.DeepEqual(expectedHammerUsers.List(), actualHammerUsers.List()) { - t.Errorf("expected %v, got %v", expectedHammerUsers.List(), actualHammerUsers.List()) + + // mark should not be able to make global access review requests + { + test := resourceAccessReviewTest{ + clientInterface: markClient.RootResourceAccessReviews(), + review: requestWhoCanViewDeployments, + err: "Forbidden", + } + test.run(t) } - whoCanViewDeploymentInMallet, err := markClient.ResourceAccessReviews("mallet-project").Create(requestWhoCanViewDeployments) - if err != nil { - t.Errorf("unexpected error: %v", err) + // a cluster-admin should be able to make global access review requests + { + test := resourceAccessReviewTest{ + clientInterface: openshiftClient.RootResourceAccessReviews(), + review: requestWhoCanViewDeployments, + users: globalClusterAdminUsers, + groups: globalClusterAdminGroups, + } + test.run(t) } - expectedMalletUsers := util.NewStringSet("anypassword:mark", "anypassword:edgar") - expectedMalletUsers.Insert(globalClusterAdminUsers.List()...) - expectedMalletGroups := globalClusterAdminGroups - actualMalletUsers := util.NewStringSet(whoCanViewDeploymentInMallet.Users...) - actualMalletGroups := util.NewStringSet(whoCanViewDeploymentInMallet.Groups...) - if !reflect.DeepEqual(expectedMalletGroups.List(), actualMalletGroups.List()) { - t.Errorf("expected %v, got %v", expectedMalletGroups.List(), actualMalletGroups.List()) +} + +type subjectAccessReviewTest struct { + clientInterface client.SubjectAccessReviewInterface + review *authorizationapi.SubjectAccessReview + + response authorizationapi.SubjectAccessReviewResponse + err string +} + +func (test subjectAccessReviewTest) run(t *testing.T) { + actualResponse, err := test.clientInterface.Create(test.review) + if len(test.err) > 0 { + if err == nil { + t.Errorf("Expected error: %v", test.err) + } else if !strings.Contains(err.Error(), test.err) { + t.Errorf("expected %v, got %v", test.err, err) + } + } else { + if err != nil { + t.Errorf("unexpected error: %v", err) + } } - if !reflect.DeepEqual(expectedMalletUsers.List(), actualMalletUsers.List()) { - t.Errorf("expected %v, got %v", expectedMalletUsers.List(), actualMalletUsers.List()) + + if reflect.DeepEqual(actualResponse, test.response) { + t.Errorf("%#v: expected %v, got %v", test.review, test.response, actualResponse) } +} - // mark should not be able to make global access review requests - _, err = markClient.RootResourceAccessReviews().Create(requestWhoCanViewDeployments) - if (err == nil) || (!strings.Contains(err.Error(), "Forbidden")) { - t.Errorf("expected forbidden error, but didn't get one") +// TODO add test for "this user" once the subject access review supports it +func TestSubjectAccessReview(t *testing.T) { + startConfig, err := StartTestMaster() + if err != nil { + t.Fatalf("unexpected error: %v", err) } - // a cluster-admin should be able to make global access review requests - whoCanViewDeploymentInAnyNamespace, err := openshiftClient.RootResourceAccessReviews().Create(requestWhoCanViewDeployments) + openshiftClient, openshiftClientConfig, err := startConfig.GetOpenshiftClient() if err != nil { t.Errorf("unexpected error: %v", err) } - actualAnyUsers := util.NewStringSet(whoCanViewDeploymentInAnyNamespace.Users...) - actualAnyGroups := util.NewStringSet(whoCanViewDeploymentInAnyNamespace.Groups...) - if !reflect.DeepEqual(globalClusterAdminGroups.List(), actualAnyGroups.List()) { - t.Errorf("expected %v, got %v", globalClusterAdminGroups.List(), actualAnyGroups.List()) + + haroldClient, err := CreateNewProject(openshiftClient, *openshiftClientConfig, "hammer-project", "harold") + if err != nil { + t.Errorf("unexpected error: %v", err) + } + markClient, err := CreateNewProject(openshiftClient, *openshiftClientConfig, "mallet-project", "mark") + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + addValerie := &policy.AddUserOptions{ + RoleNamespace: "master", + RoleName: "view", + BindingNamespace: "hammer-project", + Client: haroldClient, + Users: []string{"anypassword:valerie"}, + } + if err := addValerie.Run(); err != nil { + t.Errorf("unexpected error: %v", err) + } + + addEdgar := &policy.AddUserOptions{ + RoleNamespace: "master", + RoleName: "edit", + BindingNamespace: "mallet-project", + Client: markClient, + Users: []string{"anypassword:edgar"}, } - if !reflect.DeepEqual(globalClusterAdminUsers.List(), actualAnyUsers.List()) { - t.Errorf("expected %v, got %v", globalClusterAdminUsers.List(), actualAnyUsers.List()) + if err := addEdgar.Run(); err != nil { + t.Errorf("unexpected error: %v", err) } + + askCanValerieGetProject := &authorizationapi.SubjectAccessReview{User: "anypassword:valerie", Verb: "get", Resource: "projects"} + subjectAccessReviewTest{ + clientInterface: haroldClient.SubjectAccessReviews("hammer-project"), + review: askCanValerieGetProject, + response: authorizationapi.SubjectAccessReviewResponse{ + Allowed: true, + Reason: "allowed by rule in hammer-project", + Namespace: "hammer-project", + }, + }.run(t) + subjectAccessReviewTest{ + clientInterface: markClient.SubjectAccessReviews("mallet-project"), + review: askCanValerieGetProject, + response: authorizationapi.SubjectAccessReviewResponse{ + Allowed: false, + Reason: "denied by default", + Namespace: "mallet-project", + }, + }.run(t) + + askCanEdgarDeletePods := &authorizationapi.SubjectAccessReview{User: "anypassword:edgar", Verb: "delete", Resource: "pods"} + subjectAccessReviewTest{ + clientInterface: markClient.SubjectAccessReviews("mallet-project"), + review: askCanEdgarDeletePods, + response: authorizationapi.SubjectAccessReviewResponse{ + Allowed: true, + Reason: "allowed by rule in mallet-project", + Namespace: "mallet-project", + }, + }.run(t) + subjectAccessReviewTest{ + clientInterface: haroldClient.SubjectAccessReviews("mallet-project"), + review: askCanEdgarDeletePods, + err: "Forbidden", + }.run(t) + + askCanHaroldUpdateProject := &authorizationapi.SubjectAccessReview{User: "anypassword:harold", Verb: "update", Resource: "projects"} + subjectAccessReviewTest{ + clientInterface: haroldClient.SubjectAccessReviews("hammer-project"), + review: askCanHaroldUpdateProject, + response: authorizationapi.SubjectAccessReviewResponse{ + Allowed: true, + Reason: "allowed by rule in hammer-project", + Namespace: "hammer-project", + }, + }.run(t) + + askCanClusterAdminsCreateProject := &authorizationapi.SubjectAccessReview{Groups: []string{"system:cluster-admins"}, Verb: "create", Resource: "projects"} + subjectAccessReviewTest{ + clientInterface: openshiftClient.RootSubjectAccessReviews(), + review: askCanClusterAdminsCreateProject, + response: authorizationapi.SubjectAccessReviewResponse{ + Allowed: true, + Reason: "", + Namespace: "", + }, + }.run(t) + subjectAccessReviewTest{ + clientInterface: haroldClient.RootSubjectAccessReviews(), + review: askCanClusterAdminsCreateProject, + err: "Forbidden", + }.run(t) } From f35dc17e6fa88643560038855e4c82219bf86510 Mon Sep 17 00:00:00 2001 From: deads2k Date: Fri, 27 Feb 2015 12:44:01 -0500 Subject: [PATCH 2/3] switch access review users and groups to stringsets --- pkg/authorization/api/types.go | 6 +-- pkg/authorization/api/v1beta1/conversion.go | 38 ++++++++++++++++ pkg/authorization/api/v1beta1/types.go | 6 +-- pkg/authorization/authorizer/authorizer.go | 4 +- pkg/authorization/authorizer/interfaces.go | 3 +- pkg/authorization/authorizer/subjects_test.go | 13 +++--- .../resourceaccessreview/rest_test.go | 10 ++--- .../registry/subjectaccessreview/rest.go | 2 +- .../registry/subjectaccessreview/rest_test.go | 10 ++--- pkg/project/auth/reviewer.go | 4 +- test/integration/authorization_test.go | 44 +++++++++---------- 11 files changed, 89 insertions(+), 51 deletions(-) diff --git a/pkg/authorization/api/types.go b/pkg/authorization/api/types.go index c6a96caa5c3b..bdde02a761e0 100644 --- a/pkg/authorization/api/types.go +++ b/pkg/authorization/api/types.go @@ -145,9 +145,9 @@ type ResourceAccessReviewResponse struct { // Namespace is the namespace used for the access review Namespace string // Users is the list of users who can perform the action - Users []string + Users kutil.StringSet // Groups is the list of groups who can perform the action - Groups []string + Groups kutil.StringSet } // ResourceAccessReview is a means to request a list of which users and groups are authorized to perform the @@ -188,7 +188,7 @@ type SubjectAccessReview struct { // User is optional. If both User and Groups are empty, the current authenticated user is used. User string // Groups is optional. Groups is the list of groups to which the User belongs. - Groups []string + Groups kutil.StringSet // Content is the actual content of the request for create and update Content kruntime.EmbeddedObject // ResourceName is the name of the resource being requested for a "get" or deleted for a "delete" diff --git a/pkg/authorization/api/v1beta1/conversion.go b/pkg/authorization/api/v1beta1/conversion.go index 6a39c4cf4743..1d50fdc650f0 100644 --- a/pkg/authorization/api/v1beta1/conversion.go +++ b/pkg/authorization/api/v1beta1/conversion.go @@ -12,6 +12,44 @@ import ( func init() { err := api.Scheme.AddConversionFuncs( + func(in *SubjectAccessReview, out *newer.SubjectAccessReview, s conversion.Scope) error { + if err := s.DefaultConvert(in, out, conversion.IgnoreMissingFields); err != nil { + return err + } + + out.Groups = util.NewStringSet(in.GroupsSlice...) + + return nil + }, + func(in *newer.SubjectAccessReview, out *SubjectAccessReview, s conversion.Scope) error { + if err := s.DefaultConvert(in, out, conversion.IgnoreMissingFields); err != nil { + return err + } + + out.GroupsSlice = in.Groups.List() + + return nil + }, + func(in *ResourceAccessReviewResponse, out *newer.ResourceAccessReviewResponse, s conversion.Scope) error { + if err := s.DefaultConvert(in, out, conversion.IgnoreMissingFields); err != nil { + return err + } + + out.Users = util.NewStringSet(in.UsersSlice...) + out.Groups = util.NewStringSet(in.GroupsSlice...) + + return nil + }, + func(in *newer.ResourceAccessReviewResponse, out *ResourceAccessReviewResponse, s conversion.Scope) error { + if err := s.DefaultConvert(in, out, conversion.IgnoreMissingFields); err != nil { + return err + } + + out.UsersSlice = in.Users.List() + out.GroupsSlice = in.Groups.List() + + return nil + }, func(in *PolicyRule, out *newer.PolicyRule, s conversion.Scope) error { if err := s.Convert(&in.AttributeRestrictions, &out.AttributeRestrictions, 0); err != nil { return err diff --git a/pkg/authorization/api/v1beta1/types.go b/pkg/authorization/api/v1beta1/types.go index 98ad9749d271..366876e48d36 100644 --- a/pkg/authorization/api/v1beta1/types.go +++ b/pkg/authorization/api/v1beta1/types.go @@ -95,9 +95,9 @@ type ResourceAccessReviewResponse struct { // Namespace is the namespace used for the access review Namespace string `json:"namespace,omitempty"` // Users is the list of users who can perform the action - Users []string `json:"users"` + UsersSlice []string `json:"users"` // Groups is the list of groups who can perform the action - Groups []string `json:"groups"` + GroupsSlice []string `json:"groups"` } // ResourceAccessReview is a means to request a list of which users and groups are authorized to perform the @@ -148,7 +148,7 @@ type SubjectAccessReview struct { // User is optional. If both User and Groups are empty, the current authenticated user is used. User string `json:"user"` // Groups is optional. Groups is the list of groups to which the User belongs. - Groups []string `json:"groups"` + GroupsSlice []string `json:"groups"` // Content is the actual content of the request for create and update Content kruntime.RawExtension `json:"content,omitempty"` // ResourceName is the name of the resource being requested for a "get" or deleted for a "delete" diff --git a/pkg/authorization/authorizer/authorizer.go b/pkg/authorization/authorizer/authorizer.go index a713ad167805..2a3a11947dae 100644 --- a/pkg/authorization/authorizer/authorizer.go +++ b/pkg/authorization/authorizer/authorizer.go @@ -55,7 +55,7 @@ func (a *openshiftAuthorizer) Authorize(ctx kapi.Context, passedAttributes Autho return false, "denied by default", nil } -func (a *openshiftAuthorizer) GetAllowedSubjects(ctx kapi.Context, attributes AuthorizationAttributes) ([]string, []string, error) { +func (a *openshiftAuthorizer) GetAllowedSubjects(ctx kapi.Context, attributes AuthorizationAttributes) (util.StringSet, util.StringSet, error) { masterContext := kapi.WithNamespace(ctx, a.masterAuthorizationNamespace) globalUsers, globalGroups, err := a.getAllowedSubjectsFromNamespaceBindings(masterContext, attributes) if err != nil { @@ -74,7 +74,7 @@ func (a *openshiftAuthorizer) GetAllowedSubjects(ctx kapi.Context, attributes Au groups.Insert(globalGroups.List()...) groups.Insert(localGroups.List()...) - return users.List(), groups.List(), nil + return users, groups, nil } func (a *openshiftAuthorizer) getAllowedSubjectsFromNamespaceBindings(ctx kapi.Context, passedAttributes AuthorizationAttributes) (util.StringSet, util.StringSet, error) { diff --git a/pkg/authorization/authorizer/interfaces.go b/pkg/authorization/authorizer/interfaces.go index 3e70509c6706..f907d635d6fb 100644 --- a/pkg/authorization/authorizer/interfaces.go +++ b/pkg/authorization/authorizer/interfaces.go @@ -4,11 +4,12 @@ import ( "net/http" kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/util" ) type Authorizer interface { Authorize(ctx kapi.Context, a AuthorizationAttributes) (allowed bool, reason string, err error) - GetAllowedSubjects(ctx kapi.Context, attributes AuthorizationAttributes) ([]string, []string, error) + GetAllowedSubjects(ctx kapi.Context, attributes AuthorizationAttributes) (util.StringSet, util.StringSet, error) } type AuthorizationAttributeBuilder interface { diff --git a/pkg/authorization/authorizer/subjects_test.go b/pkg/authorization/authorizer/subjects_test.go index 365dbc3508a8..792c2e1bf052 100644 --- a/pkg/authorization/authorizer/subjects_test.go +++ b/pkg/authorization/authorizer/subjects_test.go @@ -4,6 +4,7 @@ import ( "testing" kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/util" authorizationapi "github.com/openshift/origin/pkg/authorization/api" testpolicyregistry "github.com/openshift/origin/pkg/authorization/registry/test" @@ -19,8 +20,8 @@ type subjectsTest struct { context kapi.Context attributes *DefaultAuthorizationAttributes - expectedUsers []string - expectedGroups []string + expectedUsers util.StringSet + expectedGroups util.StringSet expectedError string } @@ -31,8 +32,8 @@ func TestSubjects(t *testing.T) { Verb: "get", Resource: "pods", }, - expectedUsers: []string{"Anna", "ClusterAdmin", "Ellen", "Valerie", "system:kube-client", "system:openshift-client", "system:openshift-deployer"}, - expectedGroups: []string{"RootUsers", "system:cluster-admins"}, + expectedUsers: util.NewStringSet("Anna", "ClusterAdmin", "Ellen", "Valerie", "system:kube-client", "system:openshift-client", "system:openshift-deployer"), + expectedGroups: util.NewStringSet("RootUsers", "system:cluster-admins"), } test.policies = newDefaultGlobalPolicies() test.policies = append(test.policies, newAdzePolicies()...) @@ -49,7 +50,7 @@ func (test *subjectsTest) test(t *testing.T) { actualUsers, actualGroups, actualError := authorizer.GetAllowedSubjects(test.context, *test.attributes) - matchStringSlice(test.expectedUsers, actualUsers, "users", t) - matchStringSlice(test.expectedGroups, actualGroups, "groups", t) + matchStringSlice(test.expectedUsers.List(), actualUsers.List(), "users", t) + matchStringSlice(test.expectedGroups.List(), actualGroups.List(), "groups", t) matchError(test.expectedError, actualError, "error", t) } diff --git a/pkg/authorization/registry/resourceaccessreview/rest_test.go b/pkg/authorization/registry/resourceaccessreview/rest_test.go index 52a39e537103..45bb4e34822f 100644 --- a/pkg/authorization/registry/resourceaccessreview/rest_test.go +++ b/pkg/authorization/registry/resourceaccessreview/rest_test.go @@ -28,7 +28,7 @@ type testAuthorizer struct { func (a *testAuthorizer) Authorize(ctx kapi.Context, attributes authorizer.AuthorizationAttributes) (allowed bool, reason string, err error) { return false, "", errors.New("unsupported") } -func (a *testAuthorizer) GetAllowedSubjects(ctx kapi.Context, passedAttributes authorizer.AuthorizationAttributes) ([]string, []string, error) { +func (a *testAuthorizer) GetAllowedSubjects(ctx kapi.Context, passedAttributes authorizer.AuthorizationAttributes) (util.StringSet, util.StringSet, error) { attributes, ok := passedAttributes.(*authorizer.DefaultAuthorizationAttributes) if !ok { return nil, nil, errors.New("unexpected type for test") @@ -36,9 +36,9 @@ func (a *testAuthorizer) GetAllowedSubjects(ctx kapi.Context, passedAttributes a a.actualAttributes = attributes if len(a.err) == 0 { - return a.users.List(), a.groups.List(), nil + return a.users, a.groups, nil } - return a.users.List(), a.groups.List(), errors.New(a.err) + return a.users, a.groups, errors.New(a.err) } func TestEmptyReturn(t *testing.T) { @@ -94,8 +94,8 @@ func (r *resourceAccessTest) runTest(t *testing.T) { expectedResponse := &authorizationapi.ResourceAccessReviewResponse{ Namespace: namespace, - Users: r.authorizer.users.List(), - Groups: r.authorizer.groups.List(), + Users: r.authorizer.users, + Groups: r.authorizer.groups, } expectedAttributes := &authorizer.DefaultAuthorizationAttributes{ diff --git a/pkg/authorization/registry/subjectaccessreview/rest.go b/pkg/authorization/registry/subjectaccessreview/rest.go index 5b2590fa4c33..54e4581456f4 100644 --- a/pkg/authorization/registry/subjectaccessreview/rest.go +++ b/pkg/authorization/registry/subjectaccessreview/rest.go @@ -37,7 +37,7 @@ func (r *REST) Create(ctx kapi.Context, obj runtime.Object) (runtime.Object, err namespace := kapi.NamespaceValue(ctx) user := &user.DefaultInfo{ Name: subjectAccessReview.User, - Groups: subjectAccessReview.Groups, + Groups: subjectAccessReview.Groups.List(), } requestContext := kapi.WithUser(kapi.WithNamespace(kapi.NewDefaultContext(), namespace), user) diff --git a/pkg/authorization/registry/subjectaccessreview/rest_test.go b/pkg/authorization/registry/subjectaccessreview/rest_test.go index 9c70c6e06e25..cae1866d3f50 100644 --- a/pkg/authorization/registry/subjectaccessreview/rest_test.go +++ b/pkg/authorization/registry/subjectaccessreview/rest_test.go @@ -38,8 +38,8 @@ func (a *testAuthorizer) Authorize(ctx kapi.Context, passedAttributes authorizer } return a.allowed, a.reason, errors.New(a.err) } -func (a *testAuthorizer) GetAllowedSubjects(ctx kapi.Context, passedAttributes authorizer.AuthorizationAttributes) ([]string, []string, error) { - return nil, nil, nil +func (a *testAuthorizer) GetAllowedSubjects(ctx kapi.Context, passedAttributes authorizer.AuthorizationAttributes) (util.StringSet, util.StringSet, error) { + return util.StringSet{}, util.StringSet{}, nil } func TestEmptyReturn(t *testing.T) { @@ -50,7 +50,7 @@ func TestEmptyReturn(t *testing.T) { }, reviewRequest: &authorizationapi.SubjectAccessReview{ User: "foo", - Groups: []string{}, + Groups: util.NewStringSet(), Verb: "get", Resource: "pods", }, @@ -66,7 +66,7 @@ func TestNoErrors(t *testing.T) { reason: "because good things", }, reviewRequest: &authorizationapi.SubjectAccessReview{ - Groups: []string{"master"}, + Groups: util.NewStringSet("master"), Verb: "delete", Resource: "deploymentConfigs", }, @@ -82,7 +82,7 @@ func TestErrors(t *testing.T) { }, reviewRequest: &authorizationapi.SubjectAccessReview{ User: "foo", - Groups: []string{"first", "second"}, + Groups: util.NewStringSet("first", "second"), Verb: "get", Resource: "pods", }, diff --git a/pkg/project/auth/reviewer.go b/pkg/project/auth/reviewer.go index b4d8d2d144da..e78921b50599 100644 --- a/pkg/project/auth/reviewer.go +++ b/pkg/project/auth/reviewer.go @@ -17,12 +17,12 @@ type review struct { // Users returns the users that can access a resource func (r *review) Users() []string { - return r.response.Users + return r.response.Users.List() } // Groups returns the groups that can access a resource func (r *review) Groups() []string { - return r.response.Groups + return r.response.Groups.List() } // Reviewer performs access reviews for a project by name diff --git a/test/integration/authorization_test.go b/test/integration/authorization_test.go index 97595fd18bf7..19b6864f2c2b 100644 --- a/test/integration/authorization_test.go +++ b/test/integration/authorization_test.go @@ -88,10 +88,8 @@ type resourceAccessReviewTest struct { review *authorizationapi.ResourceAccessReview // TODO use resource access review response once internal types use util.StringSet - users util.StringSet - groups util.StringSet - namespace string - err string + response authorizationapi.ResourceAccessReviewResponse + err string } func (test resourceAccessReviewTest) run(t *testing.T) { @@ -108,14 +106,8 @@ func (test resourceAccessReviewTest) run(t *testing.T) { } } - expectedResponse := authorizationapi.ResourceAccessReviewResponse{ - Users: test.users.List(), - Groups: test.groups.List(), - Namespace: test.namespace, - } - - if reflect.DeepEqual(actualResponse, expectedResponse) { - t.Errorf("%#v: expected %v, got %v", test.review, expectedResponse, actualResponse) + if reflect.DeepEqual(actualResponse, test.response) { + t.Errorf("%#v: expected %v, got %v", test.review, test.response, actualResponse) } } @@ -167,22 +159,26 @@ func TestResourceAccessReview(t *testing.T) { test := resourceAccessReviewTest{ clientInterface: haroldClient.ResourceAccessReviews("hammer-project"), review: requestWhoCanViewDeployments, - users: util.NewStringSet("anypassword:harold", "anypassword:valerie"), - groups: globalClusterAdminGroups, - namespace: "hammer-project", + response: authorizationapi.ResourceAccessReviewResponse{ + Users: util.NewStringSet("anypassword:harold", "anypassword:valerie"), + Groups: globalClusterAdminGroups, + Namespace: "hammer-project", + }, } - test.users.Insert(globalClusterAdminUsers.List()...) + test.response.Users.Insert(globalClusterAdminUsers.List()...) test.run(t) } { test := resourceAccessReviewTest{ clientInterface: markClient.ResourceAccessReviews("mallet-project"), review: requestWhoCanViewDeployments, - users: util.NewStringSet("anypassword:mark", "anypassword:edgar"), - groups: globalClusterAdminGroups, - namespace: "mallet-project", + response: authorizationapi.ResourceAccessReviewResponse{ + Users: util.NewStringSet("anypassword:mark", "anypassword:edgar"), + Groups: globalClusterAdminGroups, + Namespace: "mallet-project", + }, } - test.users.Insert(globalClusterAdminUsers.List()...) + test.response.Users.Insert(globalClusterAdminUsers.List()...) test.run(t) } @@ -201,8 +197,10 @@ func TestResourceAccessReview(t *testing.T) { test := resourceAccessReviewTest{ clientInterface: openshiftClient.RootResourceAccessReviews(), review: requestWhoCanViewDeployments, - users: globalClusterAdminUsers, - groups: globalClusterAdminGroups, + response: authorizationapi.ResourceAccessReviewResponse{ + Users: globalClusterAdminUsers, + Groups: globalClusterAdminGroups, + }, } test.run(t) } @@ -325,7 +323,7 @@ func TestSubjectAccessReview(t *testing.T) { }, }.run(t) - askCanClusterAdminsCreateProject := &authorizationapi.SubjectAccessReview{Groups: []string{"system:cluster-admins"}, Verb: "create", Resource: "projects"} + askCanClusterAdminsCreateProject := &authorizationapi.SubjectAccessReview{Groups: util.NewStringSet("system:cluster-admins"), Verb: "create", Resource: "projects"} subjectAccessReviewTest{ clientInterface: openshiftClient.RootSubjectAccessReviews(), review: askCanClusterAdminsCreateProject, From 5b57b99fdac67affa3a7dba9bf8bc770a8653f79 Mon Sep 17 00:00:00 2001 From: deads2k Date: Fri, 27 Feb 2015 12:16:24 -0500 Subject: [PATCH 3/3] allow current-user subjectaccessreview --- .../registry/subjectaccessreview/rest.go | 24 +++++++++++++++---- test/integration/authorization_test.go | 24 +++++++++++++++++-- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/pkg/authorization/registry/subjectaccessreview/rest.go b/pkg/authorization/registry/subjectaccessreview/rest.go index 54e4581456f4..73b0f333a1d5 100644 --- a/pkg/authorization/registry/subjectaccessreview/rest.go +++ b/pkg/authorization/registry/subjectaccessreview/rest.go @@ -1,6 +1,7 @@ package subjectaccessreview import ( + "errors" "fmt" kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api" @@ -34,12 +35,25 @@ func (r *REST) Create(ctx kapi.Context, obj runtime.Object) (runtime.Object, err return nil, fmt.Errorf("not a subjectAccessReview: %#v", obj) } - namespace := kapi.NamespaceValue(ctx) - user := &user.DefaultInfo{ - Name: subjectAccessReview.User, - Groups: subjectAccessReview.Groups.List(), + var userToCheck user.Info + if (len(subjectAccessReview.User) == 0) && (len(subjectAccessReview.Groups) == 0) { + // if no user or group was specified, use the info from the context + ctxUser, exists := kapi.UserFrom(ctx) + if !exists { + return nil, errors.New("user missing from context") + } + userToCheck = ctxUser + + } else { + userToCheck = &user.DefaultInfo{ + Name: subjectAccessReview.User, + Groups: subjectAccessReview.Groups.List(), + } + } - requestContext := kapi.WithUser(kapi.WithNamespace(kapi.NewDefaultContext(), namespace), user) + + namespace := kapi.NamespaceValue(ctx) + requestContext := kapi.WithUser(ctx, userToCheck) attributes := &authorizer.DefaultAuthorizationAttributes{ Verb: subjectAccessReview.Verb, diff --git a/test/integration/authorization_test.go b/test/integration/authorization_test.go index 19b6864f2c2b..8a5936c0cfcb 100644 --- a/test/integration/authorization_test.go +++ b/test/integration/authorization_test.go @@ -87,7 +87,6 @@ type resourceAccessReviewTest struct { clientInterface client.ResourceAccessReviewInterface review *authorizationapi.ResourceAccessReview - // TODO use resource access review response once internal types use util.StringSet response authorizationapi.ResourceAccessReviewResponse err string } @@ -233,7 +232,6 @@ func (test subjectAccessReviewTest) run(t *testing.T) { } } -// TODO add test for "this user" once the subject access review supports it func TestSubjectAccessReview(t *testing.T) { startConfig, err := StartTestMaster() if err != nil { @@ -338,4 +336,26 @@ func TestSubjectAccessReview(t *testing.T) { review: askCanClusterAdminsCreateProject, err: "Forbidden", }.run(t) + + askCanICreatePods := &authorizationapi.SubjectAccessReview{Verb: "create", Resource: "projects"} + subjectAccessReviewTest{ + clientInterface: haroldClient.SubjectAccessReviews("hammer-project"), + review: askCanICreatePods, + response: authorizationapi.SubjectAccessReviewResponse{ + Allowed: true, + Reason: "allowed by rule in hammer-project", + Namespace: "hammer-project", + }, + }.run(t) + askCanICreatePolicyBindings := &authorizationapi.SubjectAccessReview{Verb: "create", Resource: "policybindings"} + subjectAccessReviewTest{ + clientInterface: haroldClient.SubjectAccessReviews("hammer-project"), + review: askCanICreatePolicyBindings, + response: authorizationapi.SubjectAccessReviewResponse{ + Allowed: false, + Reason: "denied by default", + Namespace: "hammer-project", + }, + }.run(t) + }