Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions pkg/authorization/api/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
Expand Down
38 changes: 38 additions & 0 deletions pkg/authorization/api/v1beta1/conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions pkg/authorization/api/v1beta1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the rename?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the names match, then conversions get crazy as automatic attempts are made to map unlike types in defaultconversions. Different names allows stock usage of default conversion and makes any lapses in proper overlapping leap out during unit tests.

// 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
Expand Down Expand Up @@ -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"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please, let us have a variable named "HomeSlice" at some point

// 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"
Expand Down
4 changes: 2 additions & 2 deletions pkg/authorization/authorizer/authorizer.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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) {
Expand Down
3 changes: 2 additions & 1 deletion pkg/authorization/authorizer/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
13 changes: 7 additions & 6 deletions pkg/authorization/authorizer/subjects_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -19,8 +20,8 @@ type subjectsTest struct {
context kapi.Context
attributes *DefaultAuthorizationAttributes

expectedUsers []string
expectedGroups []string
expectedUsers util.StringSet
expectedGroups util.StringSet
expectedError string
}

Expand All @@ -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()...)
Expand All @@ -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)
}
10 changes: 5 additions & 5 deletions pkg/authorization/registry/resourceaccessreview/rest_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,17 @@ 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")
}

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) {
Expand Down Expand Up @@ -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{
Expand Down
24 changes: 19 additions & 5 deletions pkg/authorization/registry/subjectaccessreview/rest.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package subjectaccessreview

import (
"errors"
"fmt"

kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api"
Expand Down Expand Up @@ -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,
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,
Expand Down
10 changes: 5 additions & 5 deletions pkg/authorization/registry/subjectaccessreview/rest_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -50,7 +50,7 @@ func TestEmptyReturn(t *testing.T) {
},
reviewRequest: &authorizationapi.SubjectAccessReview{
User: "foo",
Groups: []string{},
Groups: util.NewStringSet(),
Verb: "get",
Resource: "pods",
},
Expand All @@ -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",
},
Expand All @@ -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",
},
Expand Down
4 changes: 4 additions & 0 deletions pkg/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
19 changes: 19 additions & 0 deletions pkg/client/subjectaccessreview.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smarterclayton I thought a single resource type couldn't be both namespace-scoped and root-scoped... won't this come across in the resthander as a root-scoped object?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who knows, David should go test it.

----- Original Message -----

+// 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)

@smarterclayton I thought a single resource type couldn't be both
namespace-scoped and root-scoped... won't this come across in the resthander
as a root-scoped object?


Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1186/files#r25808075

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I should've phrased the question differently. I saw the mutterings upstream about the meaning of an empty namespace changing or getting more limited. I didn't know if using a non-namespaced call would be able to continue to mean query across all namespaces.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no reason for it not to continue to mean that at this time.

return
}
4 changes: 2 additions & 2 deletions pkg/project/auth/reviewer.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading