From d70ebd6f21d8e601ebaa6111842e76d47d248903 Mon Sep 17 00:00:00 2001
From: juanvallejo <jvallejo@redhat.com>
Date: Thu, 26 Oct 2017 16:55:46 -0400
Subject: [PATCH] return error on long-form or invalid sa name

bug: 1425398
Returns an error when the long-form name of a ServiceAccount is used
with the --serviceaccount (-z) flag in `oc policy ...' commands, or
if the name given is invalid.
---
 pkg/oc/admin/policy/modify_roles.go | 15 +++++++++++++++
 test/cmd/policy.sh                  |  5 +++++
 2 files changed, 20 insertions(+)

diff --git a/pkg/oc/admin/policy/modify_roles.go b/pkg/oc/admin/policy/modify_roles.go
index 61d487645e81..6f8b8bb05ee8 100644
--- a/pkg/oc/admin/policy/modify_roles.go
+++ b/pkg/oc/admin/policy/modify_roles.go
@@ -4,10 +4,12 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"strings"
 
 	"github.com/spf13/cobra"
 
 	kapierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/validation"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	kapi "k8s.io/kubernetes/pkg/api"
@@ -320,6 +322,19 @@ func (o *RoleModificationOptions) CompleteUserWithSA(f *clientcmd.Factory, cmd *
 		return errors.New("you must specify at least one user or service account")
 	}
 
+	// return an error if a fully-qualified service-account name is used
+	for _, sa := range saNames {
+		if strings.HasPrefix(sa, "system:serviceaccount") {
+			return errors.New("--serviceaccount (-z) should only be used with short-form serviceaccount names (e.g. `default`)")
+		}
+
+		if errCauses := validation.ValidateServiceAccountName(sa, false); len(errCauses) > 0 {
+			message := fmt.Sprintf("%q is not a valid serviceaccount name:\n  ", sa)
+			message += strings.Join(errCauses, "\n  ")
+			return errors.New(message)
+		}
+	}
+
 	authorizationClient, err := f.OpenshiftInternalAuthorizationClient()
 	if err != nil {
 		return err
diff --git a/test/cmd/policy.sh b/test/cmd/policy.sh
index 67313516ef80..afeeacbe4158 100755
--- a/test/cmd/policy.sh
+++ b/test/cmd/policy.sh
@@ -40,6 +40,11 @@ os::cmd::expect_success "oc login -u system:admin -n '${project}'"
 os::cmd::expect_success 'oc delete project policy-login'
 os::cmd::expect_failure_and_text 'oc create policybinding default -n myproject' 'error: the server does not support legacy policy resources'
 
+# validate --serviceaccount values
+os::cmd::expect_success_and_text 'oc policy add-role-to-user admin -z default' 'role "admin" added\: "default"'
+os::cmd::expect_failure_and_text 'oc policy add-role-to-user admin -z system:serviceaccount:test:default' 'should only be used with short\-form serviceaccount names'
+os::cmd::expect_failure_and_text 'oc policy add-role-to-user admin -z :invalid' '"\:invalid" is not a valid serviceaccount name'
+
 # This test validates user level policy
 os::cmd::expect_failure_and_text 'oc policy add-role-to-user' 'you must specify a role'
 os::cmd::expect_failure_and_text 'oc policy add-role-to-user -z NamespaceWithoutRole' 'you must specify a role'