diff --git a/pkg/cmd/util/tokencmd/request_token.go b/pkg/cmd/util/tokencmd/request_token.go index d56119a66683..ecd158eabf88 100644 --- a/pkg/cmd/util/tokencmd/request_token.go +++ b/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 +} diff --git a/pkg/oauth/server/osinserver/defaults.go b/pkg/oauth/server/osinserver/defaults.go index eba5f910a7af..c9f4f072bce6 100644 --- a/pkg/oauth/server/osinserver/defaults.go +++ b/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 } diff --git a/test/integration/authorization_test.go b/test/integration/authorization_test.go index b13db1300fac..88c2d968ff46 100644 --- a/test/integration/authorization_test.go +++ b/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"}