diff --git a/pkg/cmd/admin/policy/modify_roles.go b/pkg/cmd/admin/policy/modify_roles.go index b71f94dde9e6..3b19e8d422b7 100644 --- a/pkg/cmd/admin/policy/modify_roles.go +++ b/pkg/cmd/admin/policy/modify_roles.go @@ -43,6 +43,7 @@ type RoleModificationOptions struct { RoleName string RoleBindingAccessor RoleBindingAccessor + Targets []string Users []string Groups []string Subjects []kapi.ObjectReference @@ -63,7 +64,10 @@ func NewCmdAddRoleToGroup(name, fullName string, f *clientcmd.Factory, out io.Wr if err := options.AddRole(); err != nil { kcmdutil.CheckErr(err) + return } + printSuccessForCommand(options.RoleName, true, "group", options.Targets, true, out) + }, } @@ -89,7 +93,9 @@ func NewCmdAddRoleToUser(name, fullName string, f *clientcmd.Factory, out io.Wri if err := options.AddRole(); err != nil { kcmdutil.CheckErr(err) + return } + printSuccessForCommand(options.RoleName, true, "user", options.Targets, true, out) }, } @@ -114,7 +120,9 @@ func NewCmdRemoveRoleFromGroup(name, fullName string, f *clientcmd.Factory, out if err := options.RemoveRole(); err != nil { kcmdutil.CheckErr(err) + return } + printSuccessForCommand(options.RoleName, false, "group", options.Targets, true, out) }, } @@ -139,7 +147,9 @@ func NewCmdRemoveRoleFromUser(name, fullName string, f *clientcmd.Factory, out i if err := options.RemoveRole(); err != nil { kcmdutil.CheckErr(err) + return } + printSuccessForCommand(options.RoleName, false, "user", options.Targets, true, out) }, } @@ -164,7 +174,9 @@ func NewCmdAddClusterRoleToGroup(name, fullName string, f *clientcmd.Factory, ou if err := options.AddRole(); err != nil { kcmdutil.CheckErr(err) + return } + printSuccessForCommand(options.RoleName, true, "group", options.Targets, false, out) }, } @@ -186,7 +198,9 @@ func NewCmdAddClusterRoleToUser(name, fullName string, f *clientcmd.Factory, out if err := options.AddRole(); err != nil { kcmdutil.CheckErr(err) + return } + printSuccessForCommand(options.RoleName, true, "user", options.Targets, false, out) }, } @@ -208,7 +222,9 @@ func NewCmdRemoveClusterRoleFromGroup(name, fullName string, f *clientcmd.Factor if err := options.RemoveRole(); err != nil { kcmdutil.CheckErr(err) + return } + printSuccessForCommand(options.RoleName, false, "group", options.Targets, false, out) }, } @@ -230,7 +246,9 @@ func NewCmdRemoveClusterRoleFromUser(name, fullName string, f *clientcmd.Factory if err := options.RemoveRole(); err != nil { kcmdutil.CheckErr(err) + return } + printSuccessForCommand(options.RoleName, false, "user", options.Targets, false, out) }, } @@ -247,6 +265,8 @@ func (o *RoleModificationOptions) CompleteUserWithSA(f *clientcmd.Factory, args o.Users = append(o.Users, args[1:]...) } + o.Targets = o.Users + if (len(o.Users) == 0) && (len(saNames) == 0) { return errors.New("you must specify at least one user or service account") } @@ -263,6 +283,7 @@ func (o *RoleModificationOptions) CompleteUserWithSA(f *clientcmd.Factory, args o.RoleBindingAccessor = NewLocalRoleBindingAccessor(roleBindingNamespace, osClient) for _, sa := range saNames { + o.Targets = append(o.Targets, sa) o.Subjects = append(o.Subjects, kapi.ObjectReference{Namespace: roleBindingNamespace, Name: sa, Kind: "ServiceAccount"}) } @@ -277,6 +298,8 @@ func (o *RoleModificationOptions) Complete(f *clientcmd.Factory, args []string, o.RoleName = args[0] *target = append(*target, args[1:]...) + o.Targets = *target + osClient, _, err := f.Clients() if err != nil { return err @@ -396,3 +419,23 @@ existingLoop: return newSubjects } + +// prints affirmative output for role modification commands +func printSuccessForCommand(role string, didAdd bool, targetName string, targets []string, isNamespaced bool, out io.Writer) { + verb := "removed" + clusterScope := "cluster " + allTargets := fmt.Sprintf("%q", targets) + if isNamespaced { + clusterScope = "" + } + if len(targets) > 1 { + targetName = fmt.Sprintf("%ss", targetName) + } else if len(targets) == 1 { + allTargets = fmt.Sprintf("%q", targets[0]) + } + if didAdd { + verb = "added" + } + + fmt.Fprintf(out, "%srole %q %s: %s\n", clusterScope, role, verb, allTargets) +} diff --git a/test/cmd/policy.sh b/test/cmd/policy.sh index 5af48c1be912..a576ed55a3ce 100755 --- a/test/cmd/policy.sh +++ b/test/cmd/policy.sh @@ -30,20 +30,20 @@ os::cmd::expect_failure_and_text 'oc policy add-role-to-user' 'you must specify os::cmd::expect_failure_and_text 'oc policy add-role-to-user -z NamespaceWithoutRole' 'you must specify a role' os::cmd::expect_failure_and_text 'oc policy add-role-to-user view' 'you must specify at least one user or service account' -os::cmd::expect_success 'oc policy add-role-to-group cluster-admin system:unauthenticated' -os::cmd::expect_success 'oc policy add-role-to-user cluster-admin system:no-user' +os::cmd::expect_success_and_text 'oc policy add-role-to-group cluster-admin system:unauthenticated' 'role "cluster-admin" added: "system:unauthenticated"' +os::cmd::expect_success_and_text 'oc policy add-role-to-user cluster-admin system:no-user' 'role "cluster-admin" added: "system:no-user"' os::cmd::expect_success 'oc get rolebinding/cluster-admin --no-headers' os::cmd::expect_success_and_text 'oc get rolebinding/cluster-admin --no-headers' 'system:no-user' -os::cmd::expect_success 'oc policy add-role-to-user cluster-admin -z=one,two --serviceaccount=three,four' +os::cmd::expect_success_and_text 'oc policy add-role-to-user cluster-admin -z=one,two --serviceaccount=three,four' 'role "cluster-admin" added: \["one" "two" "three" "four"\]' os::cmd::expect_success 'oc get rolebinding/cluster-admin --no-headers' os::cmd::expect_success_and_text 'oc get rolebinding/cluster-admin --no-headers' 'one' os::cmd::expect_success_and_text 'oc get rolebinding/cluster-admin --no-headers' 'four' -os::cmd::expect_success 'oc policy remove-role-from-group cluster-admin system:unauthenticated' +os::cmd::expect_success_and_text 'oc policy remove-role-from-group cluster-admin system:unauthenticated' 'role "cluster-admin" removed: "system:unauthenticated"' -os::cmd::expect_success 'oc policy remove-role-from-user cluster-admin system:no-user' -os::cmd::expect_success 'oc policy remove-role-from-user cluster-admin -z=one,two --serviceaccount=three,four' +os::cmd::expect_success_and_text 'oc policy remove-role-from-user cluster-admin system:no-user' 'role "cluster-admin" removed: "system:no-user"' +os::cmd::expect_success_and_text 'oc policy remove-role-from-user cluster-admin -z=one,two --serviceaccount=three,four' 'role "cluster-admin" removed: \["one" "two" "three" "four"\]' os::cmd::expect_success 'oc get rolebinding/cluster-admin --no-headers' os::cmd::expect_success_and_not_text 'oc get rolebinding/cluster-admin --no-headers' 'four' @@ -51,7 +51,7 @@ os::cmd::expect_success 'oc policy remove-group system:unauthenticated' os::cmd::expect_success 'oc policy remove-user system:no-user' -os::cmd::expect_success 'oc policy add-role-to-user admin namespaced-user' +os::cmd::expect_success_and_text 'oc policy add-role-to-user admin namespaced-user' 'role "admin" added: "namespaced-user"' # Ensure the user has create permissions on builds, but that build strategy permissions are granted through the authenticated users group os::cmd::try_until_text 'oadm policy who-can create builds' 'namespaced-user' os::cmd::expect_success_and_not_text 'oadm policy who-can create builds/docker' 'namespaced-user' @@ -62,9 +62,9 @@ os::cmd::expect_success_and_text 'oadm policy who-can create builds/docker' os::cmd::expect_success_and_text 'oadm policy who-can create builds/source' 'system:authenticated' os::cmd::expect_success_and_text 'oadm policy who-can create builds/jenkinspipeline' 'system:authenticated' # if this method for removing access to docker/custom/source/jenkinspipeline builds changes, docs need to be updated as well -os::cmd::expect_success 'oadm policy remove-cluster-role-from-group system:build-strategy-docker system:authenticated' -os::cmd::expect_success 'oadm policy remove-cluster-role-from-group system:build-strategy-source system:authenticated' -os::cmd::expect_success 'oadm policy remove-cluster-role-from-group system:build-strategy-jenkinspipeline system:authenticated' +os::cmd::expect_success_and_text 'oadm policy remove-cluster-role-from-group system:build-strategy-docker system:authenticated' 'cluster role "system:build-strategy-docker" removed: "system:authenticated"' +os::cmd::expect_success_and_text 'oadm policy remove-cluster-role-from-group system:build-strategy-source system:authenticated' 'cluster role "system:build-strategy-source" removed: "system:authenticated"' +os::cmd::expect_success_and_text 'oadm policy remove-cluster-role-from-group system:build-strategy-jenkinspipeline system:authenticated' 'cluster role "system:build-strategy-jenkinspipeline" removed: "system:authenticated"' # ensure build strategy permissions no longer exist os::cmd::try_until_failure 'oadm policy who-can create builds/source | grep system:authenticated' os::cmd::expect_success_and_not_text 'oadm policy who-can create builds/docker' 'system:authenticated' @@ -73,7 +73,7 @@ os::cmd::expect_success_and_not_text 'oadm policy who-can create builds/jenkinsp # ensure system:authenticated users can not create custom builds by default, but can if explicitly granted access os::cmd::expect_success_and_not_text 'oadm policy who-can create builds/custom' 'system:authenticated' -os::cmd::expect_success 'oadm policy add-cluster-role-to-group system:build-strategy-custom system:authenticated' +os::cmd::expect_success_and_text 'oadm policy add-cluster-role-to-group system:build-strategy-custom system:authenticated' 'cluster role "system:build-strategy-custom" added: "system:authenticated"' os::cmd::expect_success_and_text 'oadm policy who-can create builds/custom' 'system:authenticated' os::cmd::expect_success 'oadm policy reconcile-cluster-role-bindings --confirm'