oc login: Show tokenURL message if only IDP is basic and user has not…

… provided username
openshift · Oct 14, 2020 · 4f3f784 · 4f3f784
1 parent 2a8dbc2
commit 4f3f784
Show file tree

Hide file tree

Showing 5 changed files with 67 additions and 64 deletions.
diff --git a/pkg/cli/login/login.go b/pkg/cli/login/login.go
@@ -18,6 +18,7 @@ import (
 
 	"github.com/openshift/oc/pkg/helpers/flagtypes"
 	kubeconfiglib "github.com/openshift/oc/pkg/helpers/kubeconfig"
+	"github.com/openshift/oc/pkg/helpers/tokencmd"
 )
 
 var (
@@ -59,10 +60,11 @@ func NewCmdLogin(f kcmdutil.Factory, streams genericclioptions.IOStreams) *cobra
 			kcmdutil.CheckErr(o.Validate(cmd, kcmdutil.GetFlagString(cmd, "server"), args))
 
 			if err := o.Run(); kapierrors.IsUnauthorized(err) {
-				fmt.Fprintln(streams.Out, "Login failed (401 Unauthorized)")
-				fmt.Fprintln(streams.Out, "Verify you have provided correct credentials.")
-
 				if err, isStatusErr := err.(*kapierrors.StatusError); isStatusErr {
+					if err.Status().Message != tokencmd.BasicAuthNoUsernameMessage {
+						fmt.Fprintln(streams.Out, "Login failed (401 Unauthorized)")
+						fmt.Fprintln(streams.Out, "Verify you have provided correct credentials.")
+					}
 					if details := err.Status().Details; details != nil {
 						for _, cause := range details.Causes {
 							fmt.Fprintln(streams.Out, cause.Message)

diff --git a/pkg/helpers/tokencmd/basicauth.go b/pkg/helpers/tokencmd/basicauth.go
@@ -14,6 +14,18 @@ import (
 	"github.com/openshift/oc/pkg/helpers/term"
 )
 
+// BasicAuthNoUsernameError enables NewBasicAuthNoUsernameError implementation
+type BasicAuthNoUsernameError struct{}
+
+func (e *BasicAuthNoUsernameError) Error() string {
+	return "did not receive username - print token url instead of basic prompt"
+}
+
+// NewBasicAuthNoUsernameError returns an error for a basic challenge without a username
+func NewBasicAuthNoUsernameError() error {
+	return &BasicAuthNoUsernameError{}
+}
+
 type BasicChallengeHandler struct {
 	// Host is the server being authenticated to. Used only for displaying messages when prompting for username/password
 	Host string
@@ -54,7 +66,15 @@ func (c *BasicChallengeHandler) HandleChallenge(requestURL string, headers http.
 	missingUsername := len(username) == 0
 	missingPassword := len(password) == 0
 
-	if (missingUsername || missingPassword) && c.Reader != nil {
+	if missingUsername {
+		return nil, false, NewBasicAuthNoUsernameError()
+	}
+	// Basic auth does not support usernames containing colons
+	// http://tools.ietf.org/html/rfc2617#section-2
+	if strings.Contains(username, ":") {
+		return nil, false, fmt.Errorf("username %s is invalid for basic auth", username)
+	}
+	if missingPassword && c.Reader != nil {
 		w := c.Writer
 		if w == nil {
 			w = os.Stdout
@@ -65,33 +85,19 @@ func (c *BasicChallengeHandler) HandleChallenge(requestURL string, headers http.
 		} else {
 			fmt.Fprintf(w, "Authentication required for %s\n", c.Host)
 		}
-		if missingUsername {
-			username = term.PromptForString(c.Reader, w, "Username: ")
-		} else {
-			fmt.Fprintf(w, "Username: %s\n", username)
-		}
+		fmt.Fprintf(w, "Username: %s\n", username)
 		if missingPassword {
 			password = term.PromptForPasswordString(c.Reader, w, "Password: ")
 		}
 		// remember so we don't re-prompt
 		c.prompted = true
 	}
 
-	if len(username) > 0 || len(password) > 0 {
-		// Basic auth does not support usernames containing colons
-		// http://tools.ietf.org/html/rfc2617#section-2
-		if strings.Contains(username, ":") {
-			return nil, false, fmt.Errorf("username %s is invalid for basic auth", username)
-		}
-		responseHeaders := http.Header{}
-		responseHeaders.Set("Authorization", getBasicHeader(username, password))
-		// remember so we don't re-handle non-interactively
-		c.handled = true
-		return responseHeaders, true, nil
-	}
-
-	klog.V(2).Info("no username or password available")
-	return nil, false, nil
+	responseHeaders := http.Header{}
+	responseHeaders.Set("Authorization", getBasicHeader(username, password))
+	// remember so we don't re-handle non-interactively
+	c.handled = true
+	return responseHeaders, true, nil
 }
 func (c *BasicChallengeHandler) CompleteChallenge(requestURL string, headers http.Header) error {
 	return nil
@@ -112,6 +118,8 @@ var basicRegexes = []*regexp.Regexp{
 	regexp.MustCompile(`(?i)^\s*basic(?:\s+|$)`),
 }
 
+// basicRealm returns true if a header indicates a basic auth challenge,
+// and the realm if one exists.
 func basicRealm(headers http.Header) (bool, string) {
 	for _, challengeHeader := range headers[http.CanonicalHeaderKey("WWW-Authenticate")] {
 		for _, r := range basicRegexes {

diff --git a/pkg/helpers/tokencmd/basicauth_test.go b/pkg/helpers/tokencmd/basicauth_test.go
@@ -44,7 +44,7 @@ func TestHandleChallenge(t *testing.T) {
 					ExpectedCanHandle: true,
 					ExpectedHeaders:   nil,
 					ExpectedHandled:   false,
-					ExpectedErr:       nil,
+					ExpectedErr:       NewBasicAuthNoUsernameError(),
 					ExpectedPrompt:    "",
 				},
 			},
@@ -98,34 +98,6 @@ Password: `,
 			},
 		},
 
-		"interactive challenge": {
-			Handler: &BasicChallengeHandler{
-				Host:     "myhost",
-				Reader:   bytes.NewBufferString("myuser\nmypassword\n"),
-				Username: "",
-				Password: "",
-			},
-			Challenges: []Challenge{
-				{
-					Headers:           basicChallenge,
-					ExpectedCanHandle: true,
-					ExpectedHeaders:   http.Header{AUTHORIZATION: []string{getBasicHeader("myuser", "mypassword")}},
-					ExpectedHandled:   true,
-					ExpectedErr:       nil,
-					ExpectedPrompt: `Authentication required for myhost (myrealm)
-Username: Password: `,
-				},
-				{
-					Headers:           basicChallenge,
-					ExpectedCanHandle: true,
-					ExpectedHeaders:   nil,
-					ExpectedHandled:   false,
-					ExpectedErr:       nil,
-					ExpectedPrompt:    ``,
-				},
-			},
-		},
-
 		"non-interactive challenge with reader defaults": {
 			Handler: &BasicChallengeHandler{
 				Host:     "myhost",
@@ -185,9 +157,7 @@ Username: Password: `,
 					ExpectedHeaders:   nil,
 					ExpectedHandled:   false,
 					ExpectedErr:       errors.New("username system:admin is invalid for basic auth"),
-					ExpectedPrompt: `Authentication required for myhost (myrealm)
-Username: system:admin
-Password: `,
+					ExpectedPrompt:    "",
 				},
 			},
 		},

diff --git a/pkg/helpers/tokencmd/request_token.go b/pkg/helpers/tokencmd/request_token.go
@@ -43,6 +43,9 @@ const (
 
 	// token fakes the missing osin.TOKEN const
 	token osincli.AuthorizeRequestType = "token"
+
+	// BasicAuthNoUsernameMessage will differentiate unauthorized errors from basic login with no username
+	BasicAuthNoUsernameMessage = "BasicChallengeNoUsername"
 )
 
 // ChallengeHandler handles responses to WWW-Authenticate challenges.
@@ -225,13 +228,23 @@ func (o *RequestTokenOptions) RequestToken() (string, error) {
 		defer resp.Body.Close()
 
 		if resp.StatusCode == http.StatusUnauthorized {
-			if resp.Header.Get("WWW-Authenticate") != "" {
+			if len(resp.Header.Get("WWW-Authenticate")) > 0 {
 				if !o.Handler.CanHandle(resp.Header) {
 					return "", apierrs.NewUnauthorized("unhandled challenge")
 				}
 				// Handle the challenge
 				newRequestHeaders, shouldRetry, err := o.Handler.HandleChallenge(requestURL, resp.Header)
 				if err != nil {
+					if _, ok := err.(*BasicAuthNoUsernameError); ok {
+						tokenPromptErr := apierrs.NewUnauthorized(BasicAuthNoUsernameMessage)
+						klog.V(4).Infof("%v", err)
+						tokenPromptErr.ErrStatus.Details = &metav1.StatusDetails{
+							Causes: []metav1.StatusCause{
+								{Message: fmt.Sprintf("You must obtain an API token by visiting %s/request", o.OsinConfig.TokenUrl)},
+							},
+						}
+						return "", tokenPromptErr
+					}
 					return "", err
 				}
 				if !shouldRetry {

diff --git a/pkg/helpers/tokencmd/request_token_test.go b/pkg/helpers/tokencmd/request_token_test.go
@@ -149,6 +149,7 @@ func TestRequestToken(t *testing.T) {
 
 	basicChallenge1 := resp{401, "", []string{"Basic realm=foo"}}
 	basicRequest1 := req{"Basic bXl1c2VyOm15cGFzc3dvcmQ=", "", ""} // base64("myuser:mypassword")
+	basicRequestOnlyUsername := req{"Basic bXl1c2VyOg==", "", ""}  // base64("myuser:")
 	basicChallenge2 := resp{401, "", []string{"Basic realm=seriously...foo"}}
 
 	negotiateChallenge1 := resp{401, "", []string{"Negotiate"}}
@@ -213,26 +214,35 @@ func TestRequestToken(t *testing.T) {
 			},
 			ExpectedError: "unhandled challenge",
 		},
-		"failing basic handler, basic challenge, failure": {
+		"no username, basic challenge, failure": {
 			Handler: &BasicChallengeHandler{},
 			Requests: []requestResponse{
 				{initialHead, initialHeadResp},
 				{initialRequest, basicChallenge1},
 			},
+			ExpectedError: BasicAuthNoUsernameMessage,
+		},
+		"failing basic handler, basic challenge, failure": {
+			Handler: &BasicChallengeHandler{Username: "myuser"},
+			Requests: []requestResponse{
+				{initialHead, initialHeadResp},
+				{initialRequest, basicChallenge1},
+				{basicRequestOnlyUsername, basicChallenge1},
+			},
 			ExpectedError: "challenger chose not to retry the request",
 		},
 
 		// Prompting basic handler
 		"prompting basic handler, no challenge, success": {
-			Handler: &BasicChallengeHandler{Reader: bytes.NewBufferString("myuser\nmypassword\n")},
+			Handler: &BasicChallengeHandler{Username: "myuser", Reader: bytes.NewBufferString("mypassword\n")},
 			Requests: []requestResponse{
 				{initialHead, initialHeadResp},
 				{initialRequest, success},
 			},
 			ExpectedToken: successfulToken,
 		},
 		"prompting basic handler, basic challenge, success": {
-			Handler: &BasicChallengeHandler{Reader: bytes.NewBufferString("myuser\nmypassword\n")},
+			Handler: &BasicChallengeHandler{Username: "myuser", Reader: bytes.NewBufferString("mypassword\n")},
 			Requests: []requestResponse{
 				{initialHead, initialHeadResp},
 				{initialRequest, basicChallenge1},
@@ -241,7 +251,7 @@ func TestRequestToken(t *testing.T) {
 			ExpectedToken: successfulToken,
 		},
 		"prompting basic handler, basic+negotiate challenge, success": {
-			Handler: &BasicChallengeHandler{Reader: bytes.NewBufferString("myuser\nmypassword\n")},
+			Handler: &BasicChallengeHandler{Username: "myuser", Reader: bytes.NewBufferString("mypassword\n")},
 			Requests: []requestResponse{
 				{initialHead, initialHeadResp},
 				{initialRequest, doubleChallenge},
@@ -250,11 +260,11 @@ func TestRequestToken(t *testing.T) {
 			ExpectedToken: successfulToken,
 		},
 		"prompting basic handler, basic challenge, failure": {
-			Handler: &BasicChallengeHandler{Reader: bytes.NewBufferString("myuser\nmypassword\n")},
+			Handler: &BasicChallengeHandler{Username: "myuser"},
 			Requests: []requestResponse{
 				{initialHead, initialHeadResp},
 				{initialRequest, basicChallenge1},
-				{basicRequest1, basicChallenge2},
+				{basicRequestOnlyUsername, basicChallenge2},
 			},
 			ExpectedError: "challenger chose not to retry the request",
 		},
@@ -381,7 +391,7 @@ func TestRequestToken(t *testing.T) {
 		"failing negotiate+prompting basic handler, negotiate+basic challenge, success": {
 			Handler: NewMultiHandler(
 				&NegotiateChallengeHandler{negotiator: &failingNegotiator{}},
-				&BasicChallengeHandler{Reader: bytes.NewBufferString("myuser\nmypassword\n")},
+				&BasicChallengeHandler{Username: "myuser", Reader: bytes.NewBufferString("mypassword\n")},
 			),
 			Requests: []requestResponse{
 				{initialHead, initialHeadResp},