From 847849931fe7274df8cb060dfc8a080a7faabfb7 Mon Sep 17 00:00:00 2001 From: Bobby Rullo Date: Tue, 17 May 2016 13:49:14 -0700 Subject: [PATCH] Revert "Fix response_type missing param" This reverts commit 821b242c83bd1770af0886899be163bdc77749cd. --- server/http.go | 118 ++++++++++++++++---------------------------- server/http_test.go | 23 --------- 2 files changed, 43 insertions(+), 98 deletions(-) diff --git a/server/http.go b/server/http.go index 8beb1784..a9d2bc46 100644 --- a/server/http.go +++ b/server/http.go @@ -255,7 +255,7 @@ func renderLoginPage(w http.ResponseWriter, r *http.Request, srv OIDCServer, idp v := r.URL.Query() v.Set("connector_id", idpc.ID()) - v.Set("response_type", q.Get("response_type")) + v.Set("response_type", "code") link.URL = httpPathAuth + "?" + v.Encode() td.Links = append(td.Links, link) } @@ -273,92 +273,77 @@ func handleAuthFunc(srv OIDCServer, idpcs []connector.Connector, tpl *template.T } q := r.URL.Query() - - // Retrieve client id - clientid := q.Get("client_id") - - // Retrieve state - state := q.Get("state") - - // Retrieve response_type - responseType := q.Get("response_type") - - // Retrieve scopes - qscope := strings.Fields(q.Get("scope")) - - // Check client ID param - if clientid == "" { - log.Errorf("Invalid auth request: no client_id received") - writeAuthError(w, oauth2.NewError(oauth2.ErrorInvalidRequest), state) + register := q.Get("register") == "1" && registrationEnabled + e := q.Get("error") + if e != "" { + sessionKey := q.Get("state") + if err := srv.KillSession(sessionKey); err != nil { + log.Errorf("Failed killing sessionKey %q: %v", sessionKey, err) + } + renderLoginPage(w, r, srv, idpcs, register, tpl) return } - // Check redirect_uri param, but if it's empty we don't return any error here - qru := q.Get("redirect_uri") - var rURL *url.URL - if qru != "" { - ru, err := url.Parse(qru) - if err != nil { - log.Errorf("Invalid auth request: %v", err) - writeAuthError(w, oauth2.NewError(oauth2.ErrorInvalidRequest), state) - return - } - rURL = ru + connectorID := q.Get("connector_id") + idpc, ok := idx[connectorID] + if !ok { + renderLoginPage(w, r, srv, idpcs, register, tpl) + return } - cm, err := srv.ClientMetadata(clientid) + acr, err := oauth2.ParseAuthCodeRequest(q) if err != nil { - log.Errorf("Failed fetching client %q from repo: %v", clientid, err) - writeAuthError(w, oauth2.NewError(oauth2.ErrorServerError), state) + log.Errorf("Invalid auth request: %v", err) + writeAuthError(w, err, acr.State) + return + } + + cm, err := srv.ClientMetadata(acr.ClientID) + if err != nil { + log.Errorf("Failed fetching client %q from repo: %v", acr.ClientID, err) + writeAuthError(w, oauth2.NewError(oauth2.ErrorServerError), acr.State) return } if cm == nil { - log.Errorf("Client %q not found", clientid) - writeAuthError(w, oauth2.NewError(oauth2.ErrorInvalidRequest), state) + log.Errorf("Client %q not found", acr.ClientID) + writeAuthError(w, oauth2.NewError(oauth2.ErrorInvalidRequest), acr.State) return } if len(cm.RedirectURIs) == 0 { - log.Errorf("Client %q has no redirect URLs", clientid) - writeAuthError(w, oauth2.NewError(oauth2.ErrorServerError), state) + log.Errorf("Client %q has no redirect URLs", acr.ClientID) + writeAuthError(w, oauth2.NewError(oauth2.ErrorServerError), acr.State) return } - redirectURL, err := client.ValidRedirectURL(rURL, cm.RedirectURIs) + redirectURL, err := client.ValidRedirectURL(acr.RedirectURL, cm.RedirectURIs) if err != nil { switch err { case (client.ErrorCantChooseRedirectURL): - log.Errorf("Request must provide redirect URL as client %q has registered many", clientid) - writeAuthError(w, oauth2.NewError(oauth2.ErrorInvalidRequest), state) + log.Errorf("Request must provide redirect URL as client %q has registered many", acr.ClientID) + writeAuthError(w, oauth2.NewError(oauth2.ErrorInvalidRequest), acr.State) return case (client.ErrorInvalidRedirectURL): - log.Errorf("Request provided unregistered redirect URL: %s", rURL) - writeAuthError(w, oauth2.NewError(oauth2.ErrorInvalidRequest), state) + log.Errorf("Request provided unregistered redirect URL: %s", acr.RedirectURL) + writeAuthError(w, oauth2.NewError(oauth2.ErrorInvalidRequest), acr.State) return case (client.ErrorNoValidRedirectURLs): - log.Errorf("There are no registered URLs for the requested client: %s", rURL) - writeAuthError(w, oauth2.NewError(oauth2.ErrorInvalidRequest), state) - return - default: - log.Errorf("Unexpected error checking redirect URL for client %q: %v", clientid, err) - writeAuthError(w, oauth2.NewError(oauth2.ErrorServerError), state) + log.Errorf("There are no registered URLs for the requested client: %s", acr.RedirectURL) + writeAuthError(w, oauth2.NewError(oauth2.ErrorInvalidRequest), acr.State) return } } - // Response type check - switch responseType { - case "code": // Add more cases as we support more response types - default: - log.Errorf("Invalid auth request: unsupported response_type") - redirectAuthError(w, oauth2.NewError(oauth2.ErrorUnsupportedResponseType), state, redirectURL) + if acr.ResponseType != oauth2.ResponseTypeCode { + log.Errorf("unexpected ResponseType: %v: ", acr.ResponseType) + redirectAuthError(w, oauth2.NewError(oauth2.ErrorUnsupportedResponseType), acr.State, redirectURL) return } // Check scopes. var scopes []string foundOpenIDScope := false - for _, scope := range qscope { + for _, scope := range acr.Scope { switch scope { case "openid": foundOpenIDScope = true @@ -379,33 +364,16 @@ func handleAuthFunc(srv OIDCServer, idpcs []connector.Connector, tpl *template.T if !foundOpenIDScope { log.Errorf("Invalid auth request: missing 'openid' in 'scope'") - writeAuthError(w, oauth2.NewError(oauth2.ErrorInvalidRequest), state) - return - } - - register := q.Get("register") == "1" && registrationEnabled - e := q.Get("error") - if e != "" { - if err := srv.KillSession(state); err != nil { - log.Errorf("Failed killing sessionKey %q: %v", state, err) - } - renderLoginPage(w, r, srv, idpcs, register, tpl) - return - } - - connectorID := q.Get("connector_id") - idpc, ok := idx[connectorID] - if !ok { - renderLoginPage(w, r, srv, idpcs, register, tpl) + writeAuthError(w, oauth2.NewError(oauth2.ErrorInvalidRequest), acr.State) return } nonce := q.Get("nonce") - key, err := srv.NewSession(connectorID, clientid, state, redirectURL, nonce, register, qscope) + key, err := srv.NewSession(connectorID, acr.ClientID, acr.State, redirectURL, nonce, register, acr.Scope) if err != nil { log.Errorf("Error creating new session: %v: ", err) - redirectAuthError(w, err, state, redirectURL) + redirectAuthError(w, err, acr.State, redirectURL) return } @@ -431,7 +399,7 @@ func handleAuthFunc(srv OIDCServer, idpcs []connector.Connector, tpl *template.T lu, err := idpc.LoginURL(key, p) if err != nil { log.Errorf("Connector.LoginURL failed: %v", err) - redirectAuthError(w, err, state, redirectURL) + redirectAuthError(w, err, acr.State, redirectURL) return } diff --git a/server/http_test.go b/server/http_test.go index 0399d95f..22f52435 100644 --- a/server/http_test.go +++ b/server/http_test.go @@ -175,29 +175,6 @@ func TestHandleAuthFuncResponsesSingleRedirectURL(t *testing.T) { }, wantCode: http.StatusBadRequest, }, - - // empty response_type - { - query: url.Values{ - "redirect_uri": []string{"http://client.example.com/callback"}, - "client_id": []string{"XXX"}, - "connector_id": []string{"fake"}, - "scope": []string{"openid"}, - }, - wantCode: http.StatusFound, - wantLocation: "http://client.example.com/callback?error=unsupported_response_type&state=", - }, - - // empty client_id - { - query: url.Values{ - "response_type": []string{"code"}, - "redirect_uri": []string{"http://unrecognized.example.com/callback"}, - "connector_id": []string{"fake"}, - "scope": []string{"openid"}, - }, - wantCode: http.StatusBadRequest, - }, } for i, tt := range tests {