From 7c2289e0decadb96f0d5e977feccecfa69d410bc Mon Sep 17 00:00:00 2001 From: Eric Chiang Date: Thu, 27 Oct 2016 10:20:30 -0700 Subject: [PATCH] *: rename internally used "state" form value to "req" "state" means something specific to OAuth2 and SAML so we don't want to confuse developers who are working on this. Also don't use "session" which could easily be confused with HTTP cookies. --- server/handlers.go | 15 ++++++++------- server/templates.go | 30 +++++++++++++++--------------- server/templates_default.go | 8 ++++---- web/templates/approval.html | 4 ++-- web/templates/login.html | 2 +- web/templates/password.html | 2 +- 6 files changed, 31 insertions(+), 30 deletions(-) diff --git a/server/handlers.go b/server/handlers.go index 6017b126..055c0b83 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -148,11 +148,9 @@ func (s *Server) handleAuthorization(w http.ResponseWriter, r *http.Request) { s.renderError(w, http.StatusInternalServerError, errServerError, "") return } - state := authReq.ID - if len(s.connectors) == 1 { for id := range s.connectors { - http.Redirect(w, r, s.absPath("/auth", id)+"?state="+state, http.StatusFound) + http.Redirect(w, r, s.absPath("/auth", id)+"?req="+authReq.ID, http.StatusFound) return } } @@ -168,7 +166,7 @@ func (s *Server) handleAuthorization(w http.ResponseWriter, r *http.Request) { i++ } - s.templates.login(w, connectorInfos, state) + s.templates.login(w, connectorInfos, authReq.ID) } func (s *Server) handleConnectorLogin(w http.ResponseWriter, r *http.Request) { @@ -179,7 +177,7 @@ func (s *Server) handleConnectorLogin(w http.ResponseWriter, r *http.Request) { return } - authReqID := r.FormValue("state") + authReqID := r.FormValue("req") // TODO(ericchiang): cache user identity. @@ -198,6 +196,9 @@ func (s *Server) handleConnectorLogin(w http.ResponseWriter, r *http.Request) { switch conn := conn.Connector.(type) { case connector.CallbackConnector: + // Use the auth request ID as the "state" token. + // + // TODO(ericchiang): Is this appropriate or should we also be using a nonce? callbackURL, err := conn.LoginURL(s.absURL("/callback"), authReqID) if err != nil { log.Printf("Connector %q returned error when creating callback: %v", connID, err) @@ -342,11 +343,11 @@ func (s *Server) finalizeLogin(identity connector.Identity, authReq storage.Auth if err := s.storage.UpdateAuthRequest(authReq.ID, updater); err != nil { return "", fmt.Errorf("failed to update auth request: %v", err) } - return path.Join(s.issuerURL.Path, "/approval") + "?state=" + authReq.ID, nil + return path.Join(s.issuerURL.Path, "/approval") + "?req=" + authReq.ID, nil } func (s *Server) handleApproval(w http.ResponseWriter, r *http.Request) { - authReq, err := s.storage.GetAuthRequest(r.FormValue("state")) + authReq, err := s.storage.GetAuthRequest(r.FormValue("req")) if err != nil { log.Printf("Failed to get auth request: %v", err) s.renderError(w, http.StatusInternalServerError, errServerError, "") diff --git a/server/templates.go b/server/templates.go index 117d12c5..e8285fe3 100644 --- a/server/templates.go +++ b/server/templates.go @@ -138,29 +138,29 @@ func (n byName) Len() int { return len(n) } func (n byName) Less(i, j int) bool { return n[i].Name < n[j].Name } func (n byName) Swap(i, j int) { n[i], n[j] = n[j], n[i] } -func (t *templates) login(w http.ResponseWriter, connectors []connectorInfo, state string) { +func (t *templates) login(w http.ResponseWriter, connectors []connectorInfo, authReqID string) { sort.Sort(byName(connectors)) data := struct { TemplateConfig Connectors []connectorInfo - State string - }{t.globalData, connectors, state} + AuthReqID string + }{t.globalData, connectors, authReqID} renderTemplate(w, t.loginTmpl, data) } -func (t *templates) password(w http.ResponseWriter, state, callback, lastUsername string, lastWasInvalid bool) { +func (t *templates) password(w http.ResponseWriter, authReqID, callback, lastUsername string, lastWasInvalid bool) { data := struct { TemplateConfig - State string - PostURL string - Username string - Invalid bool - }{t.globalData, state, callback, lastUsername, lastWasInvalid} + AuthReqID string + PostURL string + Username string + Invalid bool + }{t.globalData, authReqID, callback, lastUsername, lastWasInvalid} renderTemplate(w, t.passwordTmpl, data) } -func (t *templates) approval(w http.ResponseWriter, state, username, clientName string, scopes []string) { +func (t *templates) approval(w http.ResponseWriter, authReqID, username, clientName string, scopes []string) { accesses := []string{} for _, scope := range scopes { access, ok := scopeDescriptions[scope] @@ -171,11 +171,11 @@ func (t *templates) approval(w http.ResponseWriter, state, username, clientName sort.Strings(accesses) data := struct { TemplateConfig - User string - Client string - State string - Scopes []string - }{t.globalData, username, clientName, state, accesses} + User string + Client string + AuthReqID string + Scopes []string + }{t.globalData, username, clientName, authReqID, accesses} renderTemplate(w, t.approvalTmpl, data) } diff --git a/server/templates_default.go b/server/templates_default.go index 3a3d031a..651c7411 100644 --- a/server/templates_default.go +++ b/server/templates_default.go @@ -25,7 +25,7 @@ var defaultTemplates = map[string]string{
- +
- +
- + {{ if .Invalid }}
diff --git a/web/templates/approval.html b/web/templates/approval.html index c73a522e..076049c0 100644 --- a/web/templates/approval.html +++ b/web/templates/approval.html @@ -19,7 +19,7 @@
- +
- +
- + {{ if .Invalid }}