lib/api: Check basic auth (and set session cookie) before noauth exceptions (#9159)

This is motivated by the Android app:
https://github.com/syncthing/syncthing-android/pull/1982#issuecomment-1752042554

The planned fix in response to basic auth behaviour changing in #8757
was to add the `Authorization` header when opening the WebView, but it
turns out the function used only applies the header to the initial page
load, not any subsequent script loads or AJAX calls. The
`basicAuthAndSessionMiddleware` checks for no-auth exceptions before
checking the `Authorization` header, so the header has no effect on the
initial page load since the `/` path is a no-auth exception. Thus the
Android app fails to log in when opening the WebView.

This changes the order of checks in `basicAuthAndSessionMiddleware` so
that the `Authorization` header is always checked if present, and a
session cookie is set if it is valid. Only after that does the
middleware fall back to checking for no-auth exceptions.

`api_test.go` has been expanded with additional checks:
- Check that a session cookie is set whenever correct basic auth is
provided.
- Check that a session cookie is not set when basic auth is incorrect.
- Check that a session cookie is not set when authenticating with an API
token (either via `X-Api-Key` or `Authorization: Bearer`).

And an additional test case:
- Check that requests to `/` always succeed, but receive a session
cookie when correct basic auth is provided.

I have manually verified that
- The new assertions fail if the `createSession` call is removed in
`basicAuthAndSessionMiddleware`.
- The new test cases in e6e4df4d70 fail
before the change in 0e47d37e73 is
applied.
This commit is contained in:
Emil Lundberg
2023-10-10 07:48:55 +02:00
committed by Jakob Borg
parent 6e4574a9f7
commit ea1ea366d2
2 changed files with 39 additions and 10 deletions
+8 -8
View File
@@ -43,13 +43,11 @@ func antiBruteForceSleep() {
}
func unauthorized(w http.ResponseWriter) {
antiBruteForceSleep()
w.Header().Set("WWW-Authenticate", "Basic realm=\"Authorization Required\"")
http.Error(w, "Not Authorized", http.StatusUnauthorized)
}
func forbidden(w http.ResponseWriter) {
antiBruteForceSleep()
http.Error(w, "Forbidden", http.StatusForbidden)
}
@@ -87,12 +85,6 @@ func basicAuthAndSessionMiddleware(cookieName string, guiCfg config.GUIConfigura
return
}
// Exception for static assets and REST calls that don't require authentication.
if isNoAuthPath(r.URL.Path) {
next.ServeHTTP(w, r)
return
}
cookie, err := r.Cookie(cookieName)
if err == nil && cookie != nil {
sessionsMut.Lock()
@@ -111,6 +103,12 @@ func basicAuthAndSessionMiddleware(cookieName string, guiCfg config.GUIConfigura
return
}
// Exception for static assets and REST calls that don't require authentication.
if isNoAuthPath(r.URL.Path) {
next.ServeHTTP(w, r)
return
}
// 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 guiCfg.SendBasicAuthPrompt {
@@ -141,6 +139,7 @@ func passwordAuthHandler(cookieName string, guiCfg config.GUIConfiguration, ldap
}
emitLoginAttempt(false, req.Username, r.RemoteAddr, evLogger)
antiBruteForceSleep()
forbidden(w)
})
}
@@ -164,6 +163,7 @@ func attemptBasicAuth(r *http.Request, guiCfg config.GUIConfiguration, ldapCfg c
}
emitLoginAttempt(false, username, r.RemoteAddr, evLogger)
antiBruteForceSleep()
return "", false
}