cleanup of oidc login code

auth/oidc.go

@@ -90,44 +90,31 @@ func (b *OIDCBackend) CallbackHandler() http.Handler {
 }
 
 func (b *OIDCBackend) HandleLogin(w http.ResponseWriter, r *http.Request) {
-	var s *Session
-	var sid string
-
-	sc, err := r.Cookie(SessionCookieName)
-	if sc != nil && err == nil {
-		sid = sc.Value
-		s = auth.sessions.get(sc.Value)
-		if s != nil && s.Expired() {
-			auth.sessions.remove(sid)
-			s = nil
-		}
-	}
-
-	if s != nil && s.oidc != nil {
+	sid, s := getSessionFromCookie(r)
+	if s != nil {
+		msg := ""
 		if s.Username != "" {
-			http.Error(w, "you are still logged in, please logout first!", http.StatusBadRequest)
-			return
+			msg = "You are still logged in, please logout first."
+		} else if s.oidc == nil {
+			msg = "This is not an OIDC session."
+		} else {
+			msg = "OIDC login already in progress, retry later."
 		}
-		http.Error(w, "OIDC login already in progress, retry later", http.StatusConflict)
+		http.Error(w, msg, http.StatusConflict)
 		return
 	}
 
-	if s == nil {
-		s = &Session{}
-		if s.oidc, err = NewOIDCSession(); err != nil {
-			http.Error(w, err.Error(), http.StatusInternalServerError)
-			return
-		}
-		if sid, err = auth.sessions.insert(s); err != nil {
-			http.Error(w, err.Error(), http.StatusInternalServerError)
-			return
-		}
-
-		// TODO: make cookie settings more secure!
-		sc = &http.Cookie{Name: SessionCookieName, Value: sid, Expires: s.Expires}
-		http.SetCookie(w, sc)
+	os, err := NewOIDCSession()
+	if err != nil {
+		http.Error(w, "Failed to generate new OIDC session: "+err.Error(), http.StatusInternalServerError)
+		return
 	}
-
+	s = &Session{oidc: os}
+	if sid, err = auth.sessions.insert(s); err != nil {
+		http.Error(w, "Failed to generate new session: "+err.Error(), http.StatusInternalServerError)
+		return
+	}
+	setSessionCookie(w, sid, s.Expires)
 	http.Redirect(w, r, b.oauth2Config.AuthCodeURL(s.oidc.State, oidc.Nonce(s.oidc.Nonce)), http.StatusFound)
 }
 
@@ -135,38 +122,24 @@ type oidcCallbackHandler struct {
 	backend *OIDCBackend
 }
 
-func invalidateSessionCookie(w http.ResponseWriter) {
-	// TODO: this needs to improve
-	http.SetCookie(w, &http.Cookie{Name: SessionCookieName, Value: "invalid", MaxAge: -1})
-}
-
 func (h *oidcCallbackHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	sc, err := r.Cookie(SessionCookieName)
-	if sc == nil || err != nil {
-		http.Error(w, "no session cookie found, who are you?", http.StatusUnauthorized)
-		return
-	}
-	sid := sc.Value
-
-	s := auth.sessions.get(sid)
-	if s == nil || s.Expired() {
+	sid, s := getSessionFromCookie(r)
+	if s == nil {
 		invalidateSessionCookie(w)
-		http.Error(w, "invalid session cookie or session already expired", http.StatusUnauthorized)
+		http.Error(w, "Request does not contain a valid session cookie or session is already expired.", http.StatusUnauthorized)
 		return
 	}
+
 	if s.oidc == nil {
-		http.Error(w, "this not an OIDC session", http.StatusUnauthorized)
+		http.Error(w, "This is not an OIDC session.", http.StatusConflict)
 		return
 	}
 	if s.Username != "" {
-		http.Error(w, "this session is already logged in", http.StatusConflict)
+		http.Error(w, "This session is already logged in.", http.StatusConflict)
 		return
 	}
-
 	if r.URL.Query().Get("state") != s.oidc.State {
-		invalidateSessionCookie(w)
-		auth.sessions.remove(sid)
-		http.Error(w, "OIDC: state did not match", http.StatusBadRequest)
+		http.Error(w, "OIDC verification failed: state did not match", http.StatusBadRequest)
 		return
 	}
 
@@ -174,29 +147,29 @@ func (h *oidcCallbackHandler) ServeHTTP(w http.ResponseWriter, r *http.Request)
 	if err != nil {
 		invalidateSessionCookie(w)
 		auth.sessions.remove(sid)
-		http.Error(w, "OIDC: failed to exchange token: "+err.Error(), http.StatusBadRequest)
+		http.Error(w, "OAuth2 token exchange failed: "+err.Error(), http.StatusBadRequest)
 		return
 	}
-
 	rawIDToken, ok := oauth2Token.Extra("id_token").(string)
 	if !ok {
 		invalidateSessionCookie(w)
 		auth.sessions.remove(sid)
-		http.Error(w, "OIDC: no id_token field in oauth2 token.", http.StatusInternalServerError)
+		http.Error(w, "OIDC verification failed: no id_token field in oauth2 token.", http.StatusInternalServerError)
 		return
 	}
+
 	// Verify the ID Token signature and nonce.
 	idToken, err := h.backend.verifier.Verify(r.Context(), rawIDToken)
 	if err != nil {
 		invalidateSessionCookie(w)
 		auth.sessions.remove(sid)
-		http.Error(w, "OIDC: failed to verify ID Token: "+err.Error(), http.StatusInternalServerError)
+		http.Error(w, "OAuth2 ID token verification failed: "+err.Error(), http.StatusInternalServerError)
 		return
 	}
 	if idToken.Nonce != s.oidc.Nonce {
 		invalidateSessionCookie(w)
 		auth.sessions.remove(sid)
-		http.Error(w, "OIDC: invalid ID token nonce", http.StatusInternalServerError)
+		http.Error(w, "OAuth2 ID token  verification failed: invalid nonce", http.StatusInternalServerError)
 		return
 	}
 
@@ -204,7 +177,7 @@ func (h *oidcCallbackHandler) ServeHTTP(w http.ResponseWriter, r *http.Request)
 	if err != nil {
 		invalidateSessionCookie(w)
 		auth.sessions.remove(sid)
-		http.Error(w, "OIDC: failed to get userinfo: "+err.Error(), http.StatusInternalServerError)
+		http.Error(w, "Fetching OIDC UserInfo failed: "+err.Error(), http.StatusInternalServerError)
 		return
 	}
 
@@ -219,7 +192,7 @@ func (h *oidcCallbackHandler) ServeHTTP(w http.ResponseWriter, r *http.Request)
 	if err = auth.sessions.update(sid, newS); err != nil {
 		invalidateSessionCookie(w)
 		auth.sessions.remove(sid)
-		http.Error(w, "OIDC: failed to update session: "+err.Error(), http.StatusInternalServerError)
+		http.Error(w, "Updating session failed: "+err.Error(), http.StatusInternalServerError)
 		return
 	}
 

auth/sessions.go

@@ -26,6 +26,7 @@ package auth
 
 import (
 	"errors"
+	"net/http"
 	"sync"
 	"time"
 )
@@ -46,6 +47,37 @@ func (s *Session) Expired() bool {
 	return s.Expires.Before(time.Now())
 }
 
+func getSessionFromCookie(r *http.Request) (string, *Session) {
+	sc, err := r.Cookie(SessionCookieName)
+	if sc == nil && err != nil {
+		return "", nil
+	}
+
+	sid := sc.Value
+	s := auth.sessions.get(sid)
+	if s == nil {
+		return "", nil
+	}
+	if s.Expired() {
+		auth.sessions.remove(sid)
+		return "", nil
+	}
+
+	return sid, s
+}
+
+func setSessionCookie(w http.ResponseWriter, sid string, expires time.Time) {
+	// TODO: make cookie settings more secure!
+	sc := &http.Cookie{Name: SessionCookieName, Value: sid, Expires: expires}
+	http.SetCookie(w, sc)
+}
+
+func invalidateSessionCookie(w http.ResponseWriter) {
+	// TODO: this needs to improve
+	sc := &http.Cookie{Name: SessionCookieName, Value: "invalid", MaxAge: -1}
+	http.SetCookie(w, sc)
+}
+
 type SessionManager struct {
 	mutex    sync.RWMutex
 	maxAge   time.Duration