Handle unexpected server errors more thoroughly in RequestToken

pkg/cmd/util/tokencmd/request_token.go

@@ -1,26 +1,24 @@
 package tokencmd
 
 import (
+	"bytes"
+	"encoding/json"
 	"errors"
+	"fmt"
 	"io"
 	"net/http"
-	"regexp"
+	"net/url"
+
+	"github.com/RangelReale/osincli"
+	"github.com/golang/glog"
 
 	kclient "github.com/GoogleCloudPlatform/kubernetes/pkg/client"
-	"github.com/openshift/origin/pkg/client"
-	"github.com/openshift/origin/pkg/oauth/server/osinserver"
 
+	"github.com/openshift/origin/pkg/client"
 	server "github.com/openshift/origin/pkg/cmd/server/origin"
+	"github.com/openshift/origin/pkg/oauth/server/osinserver"
 )
 
-const accessTokenRedirectPattern = `#access_token=([\w]+)&`
-
-var accessTokenRedirectRegex = regexp.MustCompile(accessTokenRedirectPattern)
-
-type tokenGetterInfo struct {
-	accessToken string
-}
-
 // RequestToken uses the cmd arguments to locate an openshift oauth server and attempts to authenticate
 // it returns the access token if it gets one.  An error if it does not
 func RequestToken(clientCfg *kclient.Config, reader io.Reader, defaultUsername string, defaultPassword string) (string, error) {
@@ -45,20 +43,64 @@ func RequestToken(clientCfg *kclient.Config, reader io.Reader, defaultUsername s
 
 	osClient.Client = &challengingClient{httpClient, reader, defaultUsername, defaultPassword}
 
-	result := osClient.Get().AbsPath(server.OpenShiftOAuthAPIPrefix, osinserver.AuthorizePath).Param("response_type", "token").Param("client_id", "openshift-challenging-client").Do()
+	result := osClient.Get().AbsPath(server.OpenShiftOAuthAPIPrefix, osinserver.AuthorizePath).
+		Param("response_type", "token").
+		Param("client_id", "openshift-challenging-client").
+		Do()
+	if err := result.Error(); err != nil && !isRedirectError(err) {
+		return "", err
+	}
 
 	if len(tokenGetter.accessToken) == 0 {
-		return "", result.Error()
+		r, _ := result.Raw()
+		if description, ok := rawOAuthJSONErrorDescription(r); ok {
+			return "", fmt.Errorf("cannot retrieve a token: %s", description)
+		}
+		glog.V(4).Infof("A request token could not be created, server returned: %s", string(r))
+		return "", fmt.Errorf("the server did not return a token (possible server error)")
 	}
 
 	return tokenGetter.accessToken, nil
 }
 
+func rawOAuthJSONErrorDescription(data []byte) (string, bool) {
+	output := osincli.ResponseData{}
+	decoder := json.NewDecoder(bytes.NewBuffer(data))
+	if err := decoder.Decode(&output); err != nil {
+		return "", false
+	}
+	if _, ok := output["error"]; !ok {
+		return "", false
+	}
+	desc, ok := output["error_description"]
+	if !ok {
+		return "", false
+	}
+	s, ok := desc.(string)
+	if !ok || len(s) == 0 {
+		return "", false
+	}
+	return s, true
+}
+
+const accessTokenKey = "access_token"
+
+var errRedirectComplete = errors.New("found access token")
+
+type tokenGetterInfo struct {
+	accessToken string
+}
+
 // checkRedirect watches the redirects to see if any contain the access_token anchor.  It then stores the value of the access token for later retrieval
 func (tokenGetter *tokenGetterInfo) checkRedirect(req *http.Request, via []*http.Request) error {
-	// if we're redirected with an access token in the anchor, use it to set our transport to a proper bearer auth
-	if matches := accessTokenRedirectRegex.FindAllStringSubmatch(req.URL.String(), 1); matches != nil {
-		tokenGetter.accessToken = matches[0][1]
+	fragment := req.URL.Fragment
+	if values, err := url.ParseQuery(fragment); err == nil {
+		if v, ok := values[accessTokenKey]; ok {
+			if len(v) > 0 {
+				tokenGetter.accessToken = v[0]
+			}
+			return errRedirectComplete
+		}
 	}
 
 	if len(via) >= 10 {
@@ -67,3 +109,14 @@ func (tokenGetter *tokenGetterInfo) checkRedirect(req *http.Request, via []*http
 
 	return nil
 }
+
+func isRedirectError(err error) bool {
+	if err == errRedirectComplete {
+		return true
+	}
+	switch t := err.(type) {
+	case *url.Error:
+		return t.Err == errRedirectComplete
+	}
+	return false
+}

pkg/oauth/server/osinserver/defaults.go

@@ -24,6 +24,7 @@ func NewDefaultServerConfig() *osin.ServerConfig {
 	config.AllowClientSecretInParams = true
 	config.AllowGetAccessRequest = true
 	config.RedirectUriSeparator = ","
+	config.ErrorStatusCode = http.StatusBadRequest
 
 	return config
 }

test/integration/authorization_test.go

@@ -27,42 +27,43 @@ func TestRestrictedAccessForProjectAdmins(t *testing.T) {
 
 	clusterAdminClient, err := testutil.GetClusterAdminClient(clusterAdminKubeConfig)
 	if err != nil {
-		t.Errorf("unexpected error: %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
 
 	clusterAdminClientConfig, err := testutil.GetClusterAdminClientConfig(clusterAdminKubeConfig)
 	if err != nil {
-		t.Errorf("unexpected error: %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
 
 	haroldClient, err := testutil.CreateNewProject(clusterAdminClient, *clusterAdminClientConfig, "hammer-project", "harold")
 	if err != nil {
-		t.Errorf("unexpected error: %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
+
 	markClient, err := testutil.CreateNewProject(clusterAdminClient, *clusterAdminClientConfig, "mallet-project", "mark")
 	if err != nil {
-		t.Errorf("unexpected error: %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
 
 	_, err = haroldClient.Deployments("hammer-project").List(labels.Everything(), fields.Everything())
 	if err != nil {
-		t.Errorf("unexpected error: %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
 
 	_, err = markClient.Deployments("hammer-project").List(labels.Everything(), fields.Everything())
 	if (err == nil) || !kapierror.IsForbidden(err) {
-		t.Errorf("expected forbidden error, but didn't get one")
+		t.Fatalf("unexpected error: %v", err)
 	}
 
 	// projects are a special case where a get of a project actually sets a namespace.  Make sure that
 	// the namespace is properly special cased and set for authorization rules
 	_, err = haroldClient.Projects().Get("hammer-project")
 	if err != nil {
-		t.Errorf("unexpected error: %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
 	_, err = markClient.Projects().Get("hammer-project")
 	if (err == nil) || !kapierror.IsForbidden(err) {
-		t.Errorf("expected forbidden error, but didn't get one")
+		t.Fatalf("unexpected error: %v", err)
 	}
 
 	// TODO restore this once we have detection for whether the cache is up to date.
@@ -94,7 +95,7 @@ func TestOnlyResolveRolesForBindingsThatMatter(t *testing.T) {
 
 	clusterAdminClient, err := testutil.GetClusterAdminClient(clusterAdminKubeConfig)
 	if err != nil {
-		t.Errorf("unexpected error: %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
 
 	addValerie := &policy.AddUserOptions{
@@ -105,11 +106,11 @@ func TestOnlyResolveRolesForBindingsThatMatter(t *testing.T) {
 		Users:            []string{"anypassword:valerie"},
 	}
 	if err := addValerie.Run(); err != nil {
-		t.Errorf("unexpected error: %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
 
 	if err = clusterAdminClient.Roles(bootstrappolicy.DefaultMasterAuthorizationNamespace).Delete(bootstrappolicy.ViewRoleName); err != nil {
-		t.Errorf("unexpected error: %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
 
 	addEdgar := &policy.AddUserOptions{
@@ -120,12 +121,12 @@ func TestOnlyResolveRolesForBindingsThatMatter(t *testing.T) {
 		Users:            []string{"anypassword:edgar"},
 	}
 	if err := addEdgar.Run(); err != nil {
-		t.Errorf("unexpected error: %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
 
 	// try to add Valerie to a non-existent role
 	if err := addValerie.Run(); err == nil || !kapierror.IsNotFound(err) {
-		t.Errorf("unexpected error %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
 
 }
@@ -169,22 +170,22 @@ func TestResourceAccessReview(t *testing.T) {
 
 	clusterAdminClient, err := testutil.GetClusterAdminClient(clusterAdminKubeConfig)
 	if err != nil {
-		t.Errorf("unexpected error: %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
 
 	clusterAdminClientConfig, err := testutil.GetClusterAdminClientConfig(clusterAdminKubeConfig)
 	if err != nil {
-		t.Errorf("unexpected error: %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
 
 	haroldClient, err := testutil.CreateNewProject(clusterAdminClient, *clusterAdminClientConfig, "hammer-project", "harold")
 	if err != nil {
-		t.Errorf("unexpected error: %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
-	markClient, err := testutil.CreateNewProject(clusterAdminClient, *clusterAdminClientConfig, "mallet-project", "mark")
 
+	markClient, err := testutil.CreateNewProject(clusterAdminClient, *clusterAdminClientConfig, "mallet-project", "mark")
 	if err != nil {
-		t.Errorf("unexpected error: %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
 
 	addValerie := &policy.AddUserOptions{
@@ -195,7 +196,7 @@ func TestResourceAccessReview(t *testing.T) {
 		Users:            []string{"anypassword:valerie"},
 	}
 	if err := addValerie.Run(); err != nil {
-		t.Errorf("unexpected error: %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
 
 	addEdgar := &policy.AddUserOptions{
@@ -206,7 +207,7 @@ func TestResourceAccessReview(t *testing.T) {
 		Users:            []string{"anypassword:edgar"},
 	}
 	if err := addEdgar.Run(); err != nil {
-		t.Errorf("unexpected error: %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
 
 	requestWhoCanViewDeployments := &authorizationapi.ResourceAccessReview{Verb: "get", Resource: "deployments"}
@@ -297,22 +298,22 @@ func TestSubjectAccessReview(t *testing.T) {
 
 	clusterAdminClient, err := testutil.GetClusterAdminClient(clusterAdminKubeConfig)
 	if err != nil {
-		t.Errorf("unexpected error: %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
 
 	clusterAdminClientConfig, err := testutil.GetClusterAdminClientConfig(clusterAdminKubeConfig)
 	if err != nil {
-		t.Errorf("unexpected error: %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
 
 	haroldClient, err := testutil.CreateNewProject(clusterAdminClient, *clusterAdminClientConfig, "hammer-project", "harold")
 	if err != nil {
-		t.Errorf("unexpected error: %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
-	markClient, err := testutil.CreateNewProject(clusterAdminClient, *clusterAdminClientConfig, "mallet-project", "mark")
 
+	markClient, err := testutil.CreateNewProject(clusterAdminClient, *clusterAdminClientConfig, "mallet-project", "mark")
 	if err != nil {
-		t.Errorf("unexpected error: %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
 
 	addValerie := &policy.AddUserOptions{
@@ -334,7 +335,7 @@ func TestSubjectAccessReview(t *testing.T) {
 		Users:            []string{"anypassword:edgar"},
 	}
 	if err := addEdgar.Run(); err != nil {
-		t.Errorf("unexpected error: %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
 
 	askCanValerieGetProject := &authorizationapi.SubjectAccessReview{User: "anypassword:valerie", Verb: "get", Resource: "projects"}