lib/api: Extract session store (#9425)

This is an extract from PR #9175, which can be reviewed in isolation to
reduce the volume of changes to review all at once in #9175. There are
about to be several services and API handlers that read and set cookies
and session state, so this abstraction will prove helpful.

In particular a motivating cause for this is that with the current
architecture in PR #9175, in `api.go` the [`webauthnService` needs to
access the
session](https://github.com/syncthing/syncthing/pull/9175/files#diff-e2e14f22d818b8e635572ef0ee7718dee875c365e07225d760a6faae8be7772dR309-R310)
for authentication purposes but needs to be instantiated before the
`configMuxBuilder` for config purposes, because the WebAuthn additions
to config management need to perform WebAuthn registration ceremonies,
but currently the session management is embedded in the
`basicAuthAndSessionMiddleware` which is [instantiated much
later](https://github.com/syncthing/syncthing/pull/9175/files#diff-e2e14f22d818b8e635572ef0ee7718dee875c365e07225d760a6faae8be7772dL371-R380)
and only if authentication is enabled in `guiCfg`. This refactorization
extracts the session management out from `basicAuthAndSessionMiddleware`
so that `basicAuthAndSessionMiddleware` and `webauthnService` can both
use the same shared session management service to perform session
management logic.

### Testing

This is a refactorization intended to not change any externally
observable behaviour, so existing tests (e.g., `api_auth_test.go`)
should cover this where appropriate. I have manually verified that:

- Appending `+ "foo"` to the cookie name in `createSession` causes
`TestHtmlFormLogin/invalid_URL_returns_403_before_auth_and_404_after_auth`
and `TestHtmlFormLogin/UTF-8_auth_works` to fail
- Inverting the return value of `hasValidSession` cases a whole bunch of
tests in `TestHTTPLogin` and `TestHtmlFormLogin` to fail
- (Fixed) Changing the cookie to `MaxAge: 1000` in `destroySession` does
NOT cause any tests to fail!
- Added tests `TestHtmlFormLogin/Logout_removes_the_session_cookie`,
`TestHTTPLogin/*/Logout_removes_the_session_cookie`,
`TestHtmlFormLogin/Session_cookie_is_invalid_after_logout` and
`TestHTTPLogin/200_path#01/Session_cookie_is_invalid_after_logout` to
cover this.
- Manually verified that these tests pass both before and after the
changes in this PR, and that changing the cookie to `MaxAge: 1000` or
not calling `m.tokens.Delete(cookie.Value)` in `destroySession` makes
the respective pair of tests fail.

lib/api/api.go

@@ -368,8 +368,8 @@ func (s *service) Serve(ctx context.Context) error {
 
 	// Wrap everything in basic auth, if user/password is set.
 	if guiCfg.IsAuthEnabled() {
-		sessionCookieName := "sessionid-" + s.id.Short().String()
-		authMW := newBasicAuthAndSessionMiddleware(sessionCookieName, s.id.Short().String(), guiCfg, s.cfg.LDAP(), handler, s.evLogger, s.miscDB)
+		tokenCookieManager := newTokenCookieManager(s.id.Short().String(), guiCfg, s.evLogger, s.miscDB)
+		authMW := newBasicAuthAndSessionMiddleware(tokenCookieManager, guiCfg, s.cfg.LDAP(), handler, s.evLogger)
 		handler = authMW
 
 		restMux.Handler(http.MethodPost, "/rest/noauth/auth/password", http.HandlerFunc(authMW.passwordAuthHandler))

lib/api/api_auth.go

@@ -17,7 +17,6 @@ import (
 
 	ldap "github.com/go-ldap/ldap/v3"
 	"github.com/syncthing/syncthing/lib/config"
-	"github.com/syncthing/syncthing/lib/db"
 	"github.com/syncthing/syncthing/lib/events"
 	"github.com/syncthing/syncthing/lib/rand"
 )
@@ -80,24 +79,20 @@ func isNoAuthPath(path string) bool {
 }
 
 type basicAuthAndSessionMiddleware struct {
-	cookieName string
-	shortID    string
+	tokenCookieManager *tokenCookieManager
 	guiCfg             config.GUIConfiguration
 	ldapCfg            config.LDAPConfiguration
 	next               http.Handler
 	evLogger           events.Logger
-	tokens     *tokenManager
 }
 
-func newBasicAuthAndSessionMiddleware(cookieName, shortID string, guiCfg config.GUIConfiguration, ldapCfg config.LDAPConfiguration, next http.Handler, evLogger events.Logger, miscDB *db.NamespacedKV) *basicAuthAndSessionMiddleware {
+func newBasicAuthAndSessionMiddleware(tokenCookieManager *tokenCookieManager, guiCfg config.GUIConfiguration, ldapCfg config.LDAPConfiguration, next http.Handler, evLogger events.Logger) *basicAuthAndSessionMiddleware {
 	return &basicAuthAndSessionMiddleware{
-		cookieName: cookieName,
-		shortID:    shortID,
+		tokenCookieManager: tokenCookieManager,
 		guiCfg:             guiCfg,
 		ldapCfg:            ldapCfg,
 		next:               next,
 		evLogger:           evLogger,
-		tokens:     newTokenManager("sessions", miscDB, maxSessionLifetime, maxActiveSessions),
 	}
 }
 
@@ -107,22 +102,14 @@ func (m *basicAuthAndSessionMiddleware) ServeHTTP(w http.ResponseWriter, r *http
 		return
 	}
 
-	for _, cookie := range r.Cookies() {
-		// We iterate here since there may, historically, be multiple
-		// cookies with the same name but different path. Any "old" ones
-		// won't match an existing session and will be ignored, then
-		// later removed on logout or when timing out.
-		if cookie.Name == m.cookieName {
-			if m.tokens.Check(cookie.Value) {
+	if m.tokenCookieManager.hasValidSession(r) {
 		m.next.ServeHTTP(w, r)
 		return
 	}
-		}
-	}
 
 	// Fall back to Basic auth if provided
 	if username, ok := attemptBasicAuth(r, m.guiCfg, m.ldapCfg, m.evLogger); ok {
-		m.createSession(username, false, w, r)
+		m.tokenCookieManager.createSession(username, false, w, r)
 		m.next.ServeHTTP(w, r)
 		return
 	}
@@ -136,7 +123,7 @@ func (m *basicAuthAndSessionMiddleware) ServeHTTP(w http.ResponseWriter, r *http
 	// Some browsers don't send the Authorization request header unless prompted by a 401 response.
 	// This enables https://user:pass@localhost style URLs to keep working.
 	if m.guiCfg.SendBasicAuthPrompt {
-		unauthorized(w, m.shortID)
+		unauthorized(w, m.tokenCookieManager.shortID)
 		return
 	}
 
@@ -156,7 +143,7 @@ func (m *basicAuthAndSessionMiddleware) passwordAuthHandler(w http.ResponseWrite
 	}
 
 	if auth(req.Username, req.Password, m.guiCfg, m.ldapCfg) {
-		m.createSession(req.Username, req.StayLoggedIn, w, r)
+		m.tokenCookieManager.createSession(req.Username, req.StayLoggedIn, w, r)
 		w.WriteHeader(http.StatusNoContent)
 		return
 	}
@@ -189,55 +176,8 @@ func attemptBasicAuth(r *http.Request, guiCfg config.GUIConfiguration, ldapCfg c
 	return "", false
 }
 
-func (m *basicAuthAndSessionMiddleware) createSession(username string, persistent bool, w http.ResponseWriter, r *http.Request) {
-	sessionid := m.tokens.New()
-
-	// Best effort detection of whether the connection is HTTPS --
-	// either directly to us, or as used by the client towards a reverse
-	// proxy who sends us headers.
-	connectionIsHTTPS := r.TLS != nil ||
-		strings.ToLower(r.Header.Get("x-forwarded-proto")) == "https" ||
-		strings.Contains(strings.ToLower(r.Header.Get("forwarded")), "proto=https")
-	// If the connection is HTTPS, or *should* be HTTPS, set the Secure
-	// bit in cookies.
-	useSecureCookie := connectionIsHTTPS || m.guiCfg.UseTLS()
-
-	maxAge := 0
-	if persistent {
-		maxAge = int(maxSessionLifetime.Seconds())
-	}
-	http.SetCookie(w, &http.Cookie{
-		Name:  m.cookieName,
-		Value: sessionid,
-		// In HTTP spec Max-Age <= 0 means delete immediately,
-		// but in http.Cookie MaxAge = 0 means unspecified (session) and MaxAge < 0 means delete immediately
-		MaxAge: maxAge,
-		Secure: useSecureCookie,
-		Path:   "/",
-	})
-
-	emitLoginAttempt(true, username, r.RemoteAddr, m.evLogger)
-}
-
 func (m *basicAuthAndSessionMiddleware) handleLogout(w http.ResponseWriter, r *http.Request) {
-	for _, cookie := range r.Cookies() {
-		// We iterate here since there may, historically, be multiple
-		// cookies with the same name but different path. We drop them
-		// all.
-		if cookie.Name == m.cookieName {
-			m.tokens.Delete(cookie.Value)
-
-			// Delete the cookie
-			http.SetCookie(w, &http.Cookie{
-				Name:   m.cookieName,
-				Value:  "",
-				MaxAge: -1,
-				Secure: cookie.Secure,
-				Path:   cookie.Path,
-			})
-		}
-	}
-
+	m.tokenCookieManager.destroySession(w, r)
 	w.WriteHeader(http.StatusNoContent)
 }
 

lib/api/api_test.go

@@ -498,6 +498,15 @@ func hasSessionCookie(cookies []*http.Cookie) bool {
 	return false
 }
 
+func hasDeleteSessionCookie(cookies []*http.Cookie) bool {
+	for _, cookie := range cookies {
+		if cookie.MaxAge < 0 && strings.HasPrefix(cookie.Name, "sessionid") {
+			return true
+		}
+	}
+	return false
+}
+
 func httpGet(url string, basicAuthUsername string, basicAuthPassword string, xapikeyHeader string, authorizationBearer string, cookies []*http.Cookie, t *testing.T) *http.Response {
 	req, err := http.NewRequest("GET", url, nil)
 	for _, cookie := range cookies {
@@ -527,7 +536,7 @@ func httpGet(url string, basicAuthUsername string, basicAuthPassword string, xap
 	return resp
 }
 
-func httpPost(url string, body map[string]string, t *testing.T) *http.Response {
+func httpPost(url string, body map[string]string, cookies []*http.Cookie, t *testing.T) *http.Response {
 	bodyBytes, err := json.Marshal(body)
 	if err != nil {
 		t.Fatal(err)
@@ -538,6 +547,10 @@ func httpPost(url string, body map[string]string, t *testing.T) *http.Response {
 		t.Fatal(err)
 	}
 
+	for _, cookie := range cookies {
+		req.AddCookie(cookie)
+	}
+
 	resp, err := http.DefaultClient.Do(req)
 	if err != nil {
 		t.Fatal(err)
@@ -622,6 +635,43 @@ func TestHTTPLogin(t *testing.T) {
 				}
 			})
 
+			t.Run("Logout removes the session cookie", func(t *testing.T) {
+				t.Parallel()
+				resp := httpGetBasicAuth(url, "üser", "räksmörgås") // string literals in Go source code are in UTF-8
+				if resp.StatusCode != expectedOkStatus {
+					t.Errorf("Unexpected non-%d return code %d for authed request (UTF-8)", expectedOkStatus, resp.StatusCode)
+				}
+				if !hasSessionCookie(resp.Cookies()) {
+					t.Errorf("Expected session cookie for authed request (UTF-8)")
+				}
+				logoutResp := httpPost(baseURL+"/rest/noauth/auth/logout", nil, resp.Cookies(), t)
+				if !hasDeleteSessionCookie(logoutResp.Cookies()) {
+					t.Errorf("Expected session cookie to be deleted for logout request")
+				}
+			})
+
+			t.Run("Session cookie is invalid after logout", func(t *testing.T) {
+				t.Parallel()
+				loginResp := httpGetBasicAuth(url, "üser", "räksmörgås") // string literals in Go source code are in UTF-8
+				if loginResp.StatusCode != expectedOkStatus {
+					t.Errorf("Unexpected non-%d return code %d for authed request (UTF-8)", expectedOkStatus, loginResp.StatusCode)
+				}
+				if !hasSessionCookie(loginResp.Cookies()) {
+					t.Errorf("Expected session cookie for authed request (UTF-8)")
+				}
+
+				resp := httpGet(url, "", "", "", "", loginResp.Cookies(), t)
+				if resp.StatusCode != expectedOkStatus {
+					t.Errorf("Unexpected non-%d return code %d for cookie-authed request (UTF-8)", expectedOkStatus, resp.StatusCode)
+				}
+
+				httpPost(baseURL+"/rest/noauth/auth/logout", nil, loginResp.Cookies(), t)
+				resp = httpGet(url, "", "", "", "", loginResp.Cookies(), t)
+				if resp.StatusCode != expectedFailStatus {
+					t.Errorf("Expected session to be invalid (status %d) after logout, got status: %d", expectedFailStatus, resp.StatusCode)
+				}
+			})
+
 			t.Run("ISO-8859-1 auth works", func(t *testing.T) {
 				t.Parallel()
 				resp := httpGetBasicAuth(url, "\xfcser", "r\xe4ksm\xf6rg\xe5s") // escaped ISO-8859-1
@@ -708,7 +758,7 @@ func TestHtmlFormLogin(t *testing.T) {
 	resourceUrl404 := baseURL + "/any-path/that/does/nooooooot/match-any/noauth-pattern"
 
 	performLogin := func(username string, password string) *http.Response {
-		return httpPost(loginUrl, map[string]string{"username": username, "password": password}, t)
+		return httpPost(loginUrl, map[string]string{"username": username, "password": password}, nil, t)
 	}
 
 	performResourceRequest := func(url string, cookies []*http.Cookie) *http.Response {
@@ -773,9 +823,40 @@ func TestHtmlFormLogin(t *testing.T) {
 		}
 	})
 
+	t.Run("Logout removes the session cookie", func(t *testing.T) {
+		t.Parallel()
+		// JSON is always UTF-8, so ISO-8859-1 case is not applicable
+		resp := performLogin("üser", "räksmörgås") // string literals in Go source code are in UTF-8
+		if resp.StatusCode != http.StatusNoContent {
+			t.Errorf("Unexpected non-204 return code %d for authed request (UTF-8)", resp.StatusCode)
+		}
+		logoutResp := httpPost(baseURL+"/rest/noauth/auth/logout", nil, resp.Cookies(), t)
+		if !hasDeleteSessionCookie(logoutResp.Cookies()) {
+			t.Errorf("Expected session cookie to be deleted for logout request")
+		}
+	})
+
+	t.Run("Session cookie is invalid after logout", func(t *testing.T) {
+		t.Parallel()
+		// JSON is always UTF-8, so ISO-8859-1 case is not applicable
+		loginResp := performLogin("üser", "räksmörgås") // string literals in Go source code are in UTF-8
+		if loginResp.StatusCode != http.StatusNoContent {
+			t.Errorf("Unexpected non-204 return code %d for authed request (UTF-8)", loginResp.StatusCode)
+		}
+		resp := performResourceRequest(resourceUrl, loginResp.Cookies())
+		if resp.StatusCode != http.StatusOK {
+			t.Errorf("Unexpected non-200 return code %d for authed request (UTF-8)", resp.StatusCode)
+		}
+		httpPost(baseURL+"/rest/noauth/auth/logout", nil, loginResp.Cookies(), t)
+		resp = performResourceRequest(resourceUrl, loginResp.Cookies())
+		if resp.StatusCode != http.StatusForbidden {
+			t.Errorf("Expected session to be invalid (status 403) after logout, got status: %d", resp.StatusCode)
+		}
+	})
+
 	t.Run("form login is not applicable to other URLs", func(t *testing.T) {
 		t.Parallel()
-		resp := httpPost(baseURL+"/meta.js", map[string]string{"username": "üser", "password": "räksmörgås"}, t)
+		resp := httpPost(baseURL+"/meta.js", map[string]string{"username": "üser", "password": "räksmörgås"}, nil, t)
 		if resp.StatusCode != http.StatusForbidden {
 			t.Errorf("Unexpected non-403 return code %d for incorrect form login URL", resp.StatusCode)
 		}

lib/api/tokenmanager.go

@@ -7,10 +7,14 @@
 package api
 
 import (
+	"net/http"
 	"slices"
+	"strings"
 	"time"
 
+	"github.com/syncthing/syncthing/lib/config"
 	"github.com/syncthing/syncthing/lib/db"
+	"github.com/syncthing/syncthing/lib/events"
 	"github.com/syncthing/syncthing/lib/rand"
 	"github.com/syncthing/syncthing/lib/sync"
 )
@@ -135,3 +139,86 @@ func (m *tokenManager) scheduledSave() {
 	bs, _ := m.tokens.Marshal()      // can't fail
 	_ = m.miscDB.PutBytes(m.key, bs) // can fail, but what are we going to do?
 }
+
+type tokenCookieManager struct {
+	cookieName string
+	shortID    string
+	guiCfg     config.GUIConfiguration
+	evLogger   events.Logger
+	tokens     *tokenManager
+}
+
+func newTokenCookieManager(shortID string, guiCfg config.GUIConfiguration, evLogger events.Logger, miscDB *db.NamespacedKV) *tokenCookieManager {
+	return &tokenCookieManager{
+		cookieName: "sessionid-" + shortID,
+		shortID:    shortID,
+		guiCfg:     guiCfg,
+		evLogger:   evLogger,
+		tokens:     newTokenManager("sessions", miscDB, maxSessionLifetime, maxActiveSessions),
+	}
+}
+
+func (m *tokenCookieManager) createSession(username string, persistent bool, w http.ResponseWriter, r *http.Request) {
+	sessionid := m.tokens.New()
+
+	// Best effort detection of whether the connection is HTTPS --
+	// either directly to us, or as used by the client towards a reverse
+	// proxy who sends us headers.
+	connectionIsHTTPS := r.TLS != nil ||
+		strings.ToLower(r.Header.Get("x-forwarded-proto")) == "https" ||
+		strings.Contains(strings.ToLower(r.Header.Get("forwarded")), "proto=https")
+	// If the connection is HTTPS, or *should* be HTTPS, set the Secure
+	// bit in cookies.
+	useSecureCookie := connectionIsHTTPS || m.guiCfg.UseTLS()
+
+	maxAge := 0
+	if persistent {
+		maxAge = int(maxSessionLifetime.Seconds())
+	}
+	http.SetCookie(w, &http.Cookie{
+		Name:  m.cookieName,
+		Value: sessionid,
+		// In HTTP spec Max-Age <= 0 means delete immediately,
+		// but in http.Cookie MaxAge = 0 means unspecified (session) and MaxAge < 0 means delete immediately
+		MaxAge: maxAge,
+		Secure: useSecureCookie,
+		Path:   "/",
+	})
+
+	emitLoginAttempt(true, username, r.RemoteAddr, m.evLogger)
+}
+
+func (m *tokenCookieManager) hasValidSession(r *http.Request) bool {
+	for _, cookie := range r.Cookies() {
+		// We iterate here since there may, historically, be multiple
+		// cookies with the same name but different path. Any "old" ones
+		// won't match an existing session and will be ignored, then
+		// later removed on logout or when timing out.
+		if cookie.Name == m.cookieName {
+			if m.tokens.Check(cookie.Value) {
+				return true
+			}
+		}
+	}
+	return false
+}
+
+func (m *tokenCookieManager) destroySession(w http.ResponseWriter, r *http.Request) {
+	for _, cookie := range r.Cookies() {
+		// We iterate here since there may, historically, be multiple
+		// cookies with the same name but different path. We drop them
+		// all.
+		if cookie.Name == m.cookieName {
+			m.tokens.Delete(cookie.Value)
+
+			// Create a cookie deletion command
+			http.SetCookie(w, &http.Cookie{
+				Name:   m.cookieName,
+				Value:  "",
+				MaxAge: -1,
+				Secure: cookie.Secure,
+				Path:   cookie.Path,
+			})
+		}
+	}
+}