diff --git a/internal/httpapi/auth_test.go b/internal/httpapi/auth_test.go index b4d7ba4..5a1e73f 100644 --- a/internal/httpapi/auth_test.go +++ b/internal/httpapi/auth_test.go @@ -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) + } +} diff --git a/internal/httpapi/middleware.go b/internal/httpapi/middleware.go index 5668246..477cdb4 100644 --- a/internal/httpapi/middleware.go +++ b/internal/httpapi/middleware.go @@ -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 {