fix(httpapi): a broken database must not look like a failed login
authenticate() cannot tell "no such session" from "the session lookup failed" on its own — both arrive as an error. Answering 401 for both meant a disk or file-permission problem presented as every session ending at once: the frontend clears its state and shows the sign-in screen to everybody, and the logs record a wave of failed logins rather than the fault. Now only a missing or unknown credential is 401. Anything else is a 500 and is logged as what it is. Found while writing the smoke tests, where a deleted database file produced SQLITE_CANTOPEN on the connections that had not been opened yet — so most requests succeeded and a few reported the user as signed out, which reads exactly like a flaky auth bug. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
committed by
Kristian Wind
co-authored by
Claude Opus 5
parent
40a679edb6
commit
06ab10a276
@@ -686,3 +686,78 @@ func mustHash(t *testing.T, password string) string {
|
||||
}
|
||||
return hash
|
||||
}
|
||||
|
||||
// A database that cannot be read must not look like a failed login.
|
||||
//
|
||||
// authenticate() cannot tell the two apart on its own: both come back as an error
|
||||
// from the session lookup. Answering 401 for both means a disk problem presents as
|
||||
// every session ending at once — the frontend clears its state and shows the
|
||||
// sign-in screen to everybody — and the logs record a wave of failed logins rather
|
||||
// than the fault. Found by the end-to-end suite, where a deleted database file
|
||||
// turned into exactly that.
|
||||
func TestDatabaseFailureIsNotALogout(t *testing.T) {
|
||||
ts := newTestServer(t)
|
||||
ts.bootstrap(t)
|
||||
|
||||
// Still signed in, and the session cookie is in the jar.
|
||||
resp, _ := ts.do(t, "GET", "/api/v1/auth/me", nil)
|
||||
if resp.StatusCode != http.StatusOK {
|
||||
t.Fatalf("me: status %d", resp.StatusCode)
|
||||
}
|
||||
|
||||
// Pull the database out from under the server. Every query now fails with
|
||||
// something that is not "no such session".
|
||||
if err := ts.db.Close(); err != nil {
|
||||
t.Fatalf("close db: %v", err)
|
||||
}
|
||||
|
||||
resp, body := ts.do(t, "GET", "/api/v1/auth/me", nil)
|
||||
if resp.StatusCode == http.StatusUnauthorized {
|
||||
t.Fatal("a broken database signed the user out instead of reporting a fault")
|
||||
}
|
||||
if resp.StatusCode != http.StatusInternalServerError {
|
||||
t.Errorf("status %d, want 500", resp.StatusCode)
|
||||
}
|
||||
if body["code"] != CodeInternal {
|
||||
t.Errorf("code = %v, want %q", body["code"], CodeInternal)
|
||||
}
|
||||
}
|
||||
|
||||
// The other half of the same contract: no cookie at all is still a plain 401.
|
||||
func TestMissingCredentialsAreStillUnauthorized(t *testing.T) {
|
||||
ts := newTestServer(t)
|
||||
ts.bootstrap(t)
|
||||
|
||||
anonymous := &testServer{Server: ts.Server, db: ts.db, client: newJarClient(t)}
|
||||
resp, body := anonymous.do(t, "GET", "/api/v1/auth/me", nil)
|
||||
if resp.StatusCode != http.StatusUnauthorized {
|
||||
t.Errorf("status %d, want 401", resp.StatusCode)
|
||||
}
|
||||
if body["code"] != CodeUnauthorized {
|
||||
t.Errorf("code = %v, want %q", body["code"], CodeUnauthorized)
|
||||
}
|
||||
}
|
||||
|
||||
// And a cookie that is not a session is a 401 too, not a 500 — an unknown token is
|
||||
// a wrong credential, which is the client's problem and not the server's.
|
||||
func TestUnknownSessionTokenIsUnauthorized(t *testing.T) {
|
||||
ts := newTestServer(t)
|
||||
ts.bootstrap(t)
|
||||
|
||||
req, err := http.NewRequest("GET", ts.URL+"/api/v1/auth/me", nil)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
req.AddCookie(&http.Cookie{Name: "verdande_session", Value: "ikke-en-rigtig-token"})
|
||||
req.Header.Set("Sec-Fetch-Site", "same-origin")
|
||||
|
||||
resp, err := (&http.Client{}).Do(req)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
defer resp.Body.Close()
|
||||
|
||||
if resp.StatusCode != http.StatusUnauthorized {
|
||||
t.Errorf("status %d, want 401", resp.StatusCode)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -78,6 +78,14 @@ func (s *Server) requireAuth(next http.Handler) http.Handler {
|
||||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
user, session, err := s.authenticate(r)
|
||||
if err != nil {
|
||||
// A database that could not be read is not a person who is not signed
|
||||
// in. Answering 401 for both means an infrastructure problem shows up
|
||||
// as everybody being logged out at once — the frontend clears its state
|
||||
// and shows the sign-in screen — and nothing in the logs says why.
|
||||
if !errors.Is(err, errNoCredentials) && !errors.Is(err, store.ErrNotFound) {
|
||||
s.internal(w, "authenticate", err)
|
||||
return
|
||||
}
|
||||
writeError(w, http.StatusUnauthorized, CodeUnauthorized, "not signed in")
|
||||
return
|
||||
}
|
||||
@@ -123,6 +131,11 @@ func (s *Server) requireAdmin(next http.Handler) http.Handler {
|
||||
// Both are accepted everywhere. An API token belongs to a person and carries their
|
||||
// permissions exactly — there is no separate service identity to reason about, and
|
||||
// a script therefore cannot reach anything its owner could not.
|
||||
// errNoCredentials means the request carried nothing to authenticate with, as
|
||||
// opposed to carrying something that turned out to be wrong — or to the lookup
|
||||
// itself failing, which callers must not treat as a failed login.
|
||||
var errNoCredentials = errors.New("no credentials")
|
||||
|
||||
func (s *Server) authenticate(r *http.Request) (*store.User, *store.Session, error) {
|
||||
if token := bearerToken(r); token != "" && auth.IsAPIToken(token) {
|
||||
user, err := s.db.UserByAPIToken(r.Context(), token)
|
||||
@@ -134,7 +147,7 @@ func (s *Server) authenticate(r *http.Request) (*store.User, *store.Session, err
|
||||
|
||||
cookie, err := r.Cookie(s.cookieName())
|
||||
if err != nil || cookie.Value == "" {
|
||||
return nil, nil, errors.New("no credentials")
|
||||
return nil, nil, errNoCredentials
|
||||
}
|
||||
session, user, err := s.db.SessionByToken(r.Context(), cookie.Value)
|
||||
if err != nil {
|
||||
|
||||
Reference in New Issue
Block a user