Remove use of policy API from CLI

pkg/authorization/apis/authorization/helpers.go

@@ -2,7 +2,6 @@ package authorization
 
 import (
 	"fmt"
-	"sort"
 	"strings"
 	"unicode"
 
@@ -105,25 +104,6 @@ func (r PolicyRule) CompactString() string {
 	return fmt.Sprintf(formatString, formatArgs...)
 }
 
-func getRoleBindingValues(roleBindingMap map[string]*RoleBinding) []*RoleBinding {
-	ret := []*RoleBinding{}
-	for _, currBinding := range roleBindingMap {
-		ret = append(ret, currBinding)
-	}
-
-	return ret
-}
-func SortRoleBindings(roleBindingMap map[string]*RoleBinding, reverse bool) []*RoleBinding {
-	roleBindings := getRoleBindingValues(roleBindingMap)
-	if reverse {
-		sort.Sort(sort.Reverse(RoleBindingSorter(roleBindings)))
-	} else {
-		sort.Sort(RoleBindingSorter(roleBindings))
-	}
-
-	return roleBindings
-}
-
 type PolicyBindingSorter []PolicyBinding
 
 func (s PolicyBindingSorter) Len() int {
@@ -136,7 +116,7 @@ func (s PolicyBindingSorter) Swap(i, j int) {
 	s[i], s[j] = s[j], s[i]
 }
 
-type RoleBindingSorter []*RoleBinding
+type RoleBindingSorter []RoleBinding
 
 func (s RoleBindingSorter) Len() int {
 	return len(s)

pkg/cmd/admin/policy/policy.go

@@ -132,34 +132,33 @@ func NewLocalRoleBindingAccessor(bindingNamespace string, client client.Interfac
 }
 
 func (a LocalRoleBindingAccessor) GetExistingRoleBindingsForRole(roleNamespace, role string) ([]*authorizationapi.RoleBinding, error) {
-	existingBindings, err := a.Client.PolicyBindings(a.BindingNamespace).Get(authorizationapi.GetPolicyBindingName(roleNamespace), metav1.GetOptions{})
+	existingBindings, err := a.Client.RoleBindings(a.BindingNamespace).List(metav1.ListOptions{})
 	if err != nil && !kapierrors.IsNotFound(err) {
 		return nil, err
 	}
 
 	ret := make([]*authorizationapi.RoleBinding, 0)
 	// see if we can find an existing binding that points to the role in question.
-	for _, currBinding := range existingBindings.RoleBindings {
-		if currBinding.RoleRef.Name == role {
-			t := currBinding
-			ret = append(ret, t)
+	for i := range existingBindings.Items {
+		// shallow copy outside of the loop so that we can take its address
+		currBinding := existingBindings.Items[i]
+		if currBinding.RoleRef.Name == role && currBinding.RoleRef.Namespace == roleNamespace {
+			ret = append(ret, &currBinding)
 		}
 	}
 
 	return ret, nil
 }
 
 func (a LocalRoleBindingAccessor) GetExistingRoleBindingNames() (*sets.String, error) {
-	policyBindings, err := a.Client.PolicyBindings(a.BindingNamespace).List(metav1.ListOptions{})
+	roleBindings, err := a.Client.RoleBindings(a.BindingNamespace).List(metav1.ListOptions{})
 	if err != nil {
 		return nil, err
 	}
 
 	ret := &sets.String{}
-	for _, existingBindings := range policyBindings.Items {
-		for _, currBinding := range existingBindings.RoleBindings {
-			ret.Insert(currBinding.Name)
-		}
+	for _, currBinding := range roleBindings.Items {
+		ret.Insert(currBinding.Name)
 	}
 
 	return ret, nil
@@ -187,36 +186,38 @@ func NewClusterRoleBindingAccessor(client client.Interface) ClusterRoleBindingAc
 }
 
 func (a ClusterRoleBindingAccessor) GetExistingRoleBindingsForRole(roleNamespace, role string) ([]*authorizationapi.RoleBinding, error) {
-	uncast, err := a.Client.ClusterPolicyBindings().Get(authorizationapi.GetPolicyBindingName(roleNamespace), metav1.GetOptions{})
-	if err != nil && !kapierrors.IsNotFound(err) {
+	// cluster role bindings can only reference cluster roles
+	if roleNamespace != "" {
+		return nil, nil
+	}
+
+	existingBindings, err := a.Client.ClusterRoleBindings().List(metav1.ListOptions{})
+	if err != nil {
 		return nil, err
 	}
-	existingBindings := authorizationapi.ToPolicyBinding(uncast)
 
 	ret := make([]*authorizationapi.RoleBinding, 0)
 	// see if we can find an existing binding that points to the role in question.
-	for _, currBinding := range existingBindings.RoleBindings {
-		if currBinding.RoleRef.Name == role {
-			t := currBinding
-			ret = append(ret, t)
+	for i := range existingBindings.Items {
+		// shallow copy outside of the loop so that we can take its address
+		currBinding := existingBindings.Items[i]
+		if currBinding.RoleRef.Name == role && currBinding.RoleRef.Namespace == "" {
+			ret = append(ret, authorizationapi.ToRoleBinding(&currBinding))
 		}
 	}
 
 	return ret, nil
 }
 
 func (a ClusterRoleBindingAccessor) GetExistingRoleBindingNames() (*sets.String, error) {
-	uncast, err := a.Client.ClusterPolicyBindings().List(metav1.ListOptions{})
+	existingBindings, err := a.Client.ClusterRoleBindings().List(metav1.ListOptions{})
 	if err != nil {
 		return nil, err
 	}
-	policyBindings := authorizationapi.ToPolicyBindingList(uncast)
 
 	ret := &sets.String{}
-	for _, existingBindings := range policyBindings.Items {
-		for _, currBinding := range existingBindings.RoleBindings {
-			ret.Insert(currBinding.Name)
-		}
+	for _, currBinding := range existingBindings.Items {
+		ret.Insert(currBinding.Name)
 	}
 
 	return ret, nil

pkg/cmd/admin/policy/remove_from_project.go

@@ -96,11 +96,12 @@ func (o *RemoveFromProjectOptions) Complete(f *clientcmd.Factory, args []string,
 }
 
 func (o *RemoveFromProjectOptions) Run() error {
-	bindingList, err := o.Client.PolicyBindings(o.BindingNamespace).List(metav1.ListOptions{})
+	roleBindings, err := o.Client.RoleBindings(o.BindingNamespace).List(metav1.ListOptions{})
 	if err != nil {
 		return err
 	}
-	sort.Sort(authorizationapi.PolicyBindingSorter(bindingList.Items))
+	// maintain David's hack from #1973 (see #1975, #1976 and https://bugzilla.redhat.com/show_bug.cgi?id=1215969)
+	sort.Sort(sort.Reverse(authorizationapi.RoleBindingSorter(roleBindings.Items)))
 
 	usersRemoved := sets.String{}
 	groupsRemoved := sets.String{}
@@ -109,47 +110,45 @@ func (o *RemoveFromProjectOptions) Run() error {
 
 	subjectsToRemove := authorizationapi.BuildSubjects(o.Users, o.Groups, uservalidation.ValidateUserName, uservalidation.ValidateGroupName)
 
-	for _, currPolicyBinding := range bindingList.Items {
-		for _, currBinding := range authorizationapi.SortRoleBindings(currPolicyBinding.RoleBindings, true) {
-			originalSubjects := make([]kapi.ObjectReference, len(currBinding.Subjects))
-			copy(originalSubjects, currBinding.Subjects)
-			oldUsers, oldGroups, oldSAs, oldOthers := authorizationapi.SubjectsStrings(currBinding.Namespace, originalSubjects)
-			oldUsersSet, oldGroupsSet, oldSAsSet, oldOtherSet := sets.NewString(oldUsers...), sets.NewString(oldGroups...), sets.NewString(oldSAs...), sets.NewString(oldOthers...)
+	for _, currBinding := range roleBindings.Items {
+		originalSubjects := make([]kapi.ObjectReference, len(currBinding.Subjects))
+		copy(originalSubjects, currBinding.Subjects)
+		oldUsers, oldGroups, oldSAs, oldOthers := authorizationapi.SubjectsStrings(currBinding.Namespace, originalSubjects)
+		oldUsersSet, oldGroupsSet, oldSAsSet, oldOtherSet := sets.NewString(oldUsers...), sets.NewString(oldGroups...), sets.NewString(oldSAs...), sets.NewString(oldOthers...)
 
-			currBinding.Subjects = removeSubjects(currBinding.Subjects, subjectsToRemove)
-			newUsers, newGroups, newSAs, newOthers := authorizationapi.SubjectsStrings(currBinding.Namespace, currBinding.Subjects)
-			newUsersSet, newGroupsSet, newSAsSet, newOtherSet := sets.NewString(newUsers...), sets.NewString(newGroups...), sets.NewString(newSAs...), sets.NewString(newOthers...)
+		currBinding.Subjects = removeSubjects(currBinding.Subjects, subjectsToRemove)
+		newUsers, newGroups, newSAs, newOthers := authorizationapi.SubjectsStrings(currBinding.Namespace, currBinding.Subjects)
+		newUsersSet, newGroupsSet, newSAsSet, newOtherSet := sets.NewString(newUsers...), sets.NewString(newGroups...), sets.NewString(newSAs...), sets.NewString(newOthers...)
 
-			if len(currBinding.Subjects) == len(originalSubjects) {
-				continue
-			}
+		if len(currBinding.Subjects) == len(originalSubjects) {
+			continue
+		}
 
-			_, err = o.Client.RoleBindings(o.BindingNamespace).Update(currBinding)
-			if err != nil {
-				return err
-			}
+		_, err = o.Client.RoleBindings(o.BindingNamespace).Update(&currBinding)
+		if err != nil {
+			return err
+		}
 
-			roleDisplayName := fmt.Sprintf("%s/%s", currBinding.RoleRef.Namespace, currBinding.RoleRef.Name)
-			if len(currBinding.RoleRef.Namespace) == 0 {
-				roleDisplayName = currBinding.RoleRef.Name
-			}
+		roleDisplayName := fmt.Sprintf("%s/%s", currBinding.RoleRef.Namespace, currBinding.RoleRef.Name)
+		if len(currBinding.RoleRef.Namespace) == 0 {
+			roleDisplayName = currBinding.RoleRef.Name
+		}
 
-			if diff := oldUsersSet.Difference(newUsersSet); len(diff) != 0 {
-				fmt.Fprintf(o.Out, "Removing %s from users %v in project %s.\n", roleDisplayName, diff.List(), o.BindingNamespace)
-				usersRemoved.Insert(diff.List()...)
-			}
-			if diff := oldGroupsSet.Difference(newGroupsSet); len(diff) != 0 {
-				fmt.Fprintf(o.Out, "Removing %s from groups %v in project %s.\n", roleDisplayName, diff.List(), o.BindingNamespace)
-				groupsRemoved.Insert(diff.List()...)
-			}
-			if diff := oldSAsSet.Difference(newSAsSet); len(diff) != 0 {
-				fmt.Fprintf(o.Out, "Removing %s from serviceaccounts %v in project %s.\n", roleDisplayName, diff.List(), o.BindingNamespace)
-				sasRemoved.Insert(diff.List()...)
-			}
-			if diff := oldOtherSet.Difference(newOtherSet); len(diff) != 0 {
-				fmt.Fprintf(o.Out, "Removing %s from subjects %v in project %s.\n", roleDisplayName, diff.List(), o.BindingNamespace)
-				othersRemoved.Insert(diff.List()...)
-			}
+		if diff := oldUsersSet.Difference(newUsersSet); len(diff) != 0 {
+			fmt.Fprintf(o.Out, "Removing %s from users %v in project %s.\n", roleDisplayName, diff.List(), o.BindingNamespace)
+			usersRemoved.Insert(diff.List()...)
+		}
+		if diff := oldGroupsSet.Difference(newGroupsSet); len(diff) != 0 {
+			fmt.Fprintf(o.Out, "Removing %s from groups %v in project %s.\n", roleDisplayName, diff.List(), o.BindingNamespace)
+			groupsRemoved.Insert(diff.List()...)
+		}
+		if diff := oldSAsSet.Difference(newSAsSet); len(diff) != 0 {
+			fmt.Fprintf(o.Out, "Removing %s from serviceaccounts %v in project %s.\n", roleDisplayName, diff.List(), o.BindingNamespace)
+			sasRemoved.Insert(diff.List()...)
+		}
+		if diff := oldOtherSet.Difference(newOtherSet); len(diff) != 0 {
+			fmt.Fprintf(o.Out, "Removing %s from subjects %v in project %s.\n", roleDisplayName, diff.List(), o.BindingNamespace)
+			othersRemoved.Insert(diff.List()...)
 		}
 	}
 

test/cmd/policy-storage-admin.sh

@@ -31,8 +31,8 @@ os::cmd::expect_success_and_text 'oc policy can-i create pv' 'yes'
 os::cmd::expect_success_and_text 'oc policy can-i create storageclass' 'yes'
 
 # Test failure to change policy on users for storage-admin
-os::cmd::expect_failure_and_text 'oc policy add-role-to-user admin storage-adm' 'cannot get policybindings'
-os::cmd::expect_failure_and_text 'oc policy remove-user screeley' 'cannot list policybindings'
+os::cmd::expect_failure_and_text 'oc policy add-role-to-user admin storage-adm' 'cannot list rolebindings'
+os::cmd::expect_failure_and_text 'oc policy remove-user screeley' 'cannot list rolebindings'
 os::cmd::expect_success 'oc logout'
 
 # Test that scoped storage-admin now an admin in project foo