Sign in before the first call, not after a 401 that never comes

Logging in failed on both the Web App and the Phone App with PocketBase's {"data":{},"message":"The requested resource wasn't found.","status":404}. The login itself succeeded; the profile fetch right after it — GET /api/me — was what 404ed, and the web app renders the relayed body on the login form, so it read as a rejected sign-in. PocketBase answers a record read it will not allow with 404 rather than 401: it hides the record instead of refusing the credentials. The pb client only re-authenticated on a 401, so with no cached token the first request went out carrying no Authorization header at all, came back 404, and the retry never fired. Nothing ever tried again — the server kept 404ing long after PocketBase was healthy. The cache is empty in exactly the two cases that matter: a startup where the up-front Authenticate failed because PocketBase wasn't up yet, which main.go treats as non-fatal on purpose so a superadmin can still log in and fix the connection; and a Reconfigure from the panel, which clears the token so the new credentials get used. So acquire the token before the first attempt rather than hoping for a 401 to prompt it, across all four superuser paths. Bad credentials now surface as the authentication failure they are instead of masquerading as a missing record. With no service account configured there is nothing to acquire and the call proceeds as before, since the endpoints that need superuser access already answer 503 on their own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-08-29 10:33:33 +02:00
parent 12ec10a797
commit fa569c4030
2 changed files with 228 additions and 2 deletions
@@ -134,9 +134,34 @@ func (c *Client) currentToken() string {
 	return c.token
 }

-// do executes a request, attaching the auth token. On a 401 it re-authenticates
-// once and retries.
+// ensureToken acquires a superuser token when none is cached yet, so that a
+// superuser call never goes out unauthenticated.
+//
+// It matters because PocketBase answers a record read it will not allow with
+// 404, not 401 — it hides the record rather than refusing the credentials. So a
+// request sent with no Authorization header comes back "The requested resource
+// wasn't found", the 401 retries below never fire, and the caller is told the
+// record is missing when the truth is that this client never signed in. The
+// cache is empty in exactly the cases that matter: a startup where Authenticate
+// failed because PocketBase wasn't up yet (deliberately non-fatal), and a
+// Reconfigure, which clears the token so the new credentials are used.
+//
+// With no service account configured there is nothing to acquire, so the call
+// proceeds as before — the endpoints that need superuser access answer 503 on
+// their own.
+func (c *Client) ensureToken(ctx context.Context) error {
+	if c.currentToken() != "" || !c.Configured() {
+		return nil
+	}
+	return c.Authenticate(ctx)
+}
+
+// do executes a request, attaching the auth token — signing in first if there
+// isn't one yet. On a 401 it re-authenticates once and retries.
 func (c *Client) do(ctx context.Context, method, path string, payload any) ([]byte, error) {
+	if err := c.ensureToken(ctx); err != nil {
+		return nil, err
+	}
 	raw, status, err := c.attempt(ctx, method, path, payload)
 	if err != nil {
 		return nil, err
@@ -191,6 +216,9 @@ func (c *Client) attempt(ctx context.Context, method, path string, payload any)
 // PocketBase's own validation errors to the client verbatim (user/organization
 // management) use this; handlers that want Go errors use the typed CRUD helpers.
 func (c *Client) Raw(ctx context.Context, method, path string, payload any) ([]byte, int, error) {
+	if err := c.ensureToken(ctx); err != nil {
+		return nil, 0, err
+	}
 	raw, status, err := c.attempt(ctx, method, path, payload)
 	if err != nil {
 		return nil, 0, err
@@ -396,6 +424,9 @@ func (c *Client) Delete(ctx context.Context, collection, id string) error {
 // superuser (this project's collections have no public access rules).
 func (c *Client) GetFile(ctx context.Context, collection, recordID, filename string) ([]byte, string, error) {
 	path := "/api/files/" + collection + "/" + recordID + "/" + filename
+	if err := c.ensureToken(ctx); err != nil {
+		return nil, "", err
+	}
 	raw, ctype, status, err := c.attemptGetFile(ctx, path)
 	if err != nil {
 		return nil, "", err
@@ -448,6 +479,9 @@ func (c *Client) RequestVerification(ctx context.Context, collection, email stri
 // fields (e.g. the users.avatar upload), which PocketBase doesn't accept as
 // plain JSON. Pass fileField == "" to send only the plain fields.
 func (c *Client) UpdateMultipart(ctx context.Context, collection, id string, fields map[string]string, fileField, filename string, fileContent []byte) error {
+	if err := c.ensureToken(ctx); err != nil {
+		return err
+	}
 	status, err := c.attemptMultipart(ctx, collection, id, fields, fileField, filename, fileContent)
 	if status == http.StatusUnauthorized {
 		if aerr := c.Authenticate(ctx); aerr != nil {
@@ -0,0 +1,192 @@
+package pb
+
+import (
+	"context"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"sync"
+	"testing"
+)
+
+// PocketBase hides a record it will not let you read behind a 404 rather than a
+// 401. A superuser call that goes out with no token therefore comes back
+// "not found", and the client's 401 retry never fires — so these tests pin down
+// that the client signs in *before* its first request whenever it holds no
+// token: after a startup where the up-front Authenticate failed, and after a
+// Reconfigure onto new credentials.
+
+const (
+	fakeSvcToken = "svc-token"
+	fakeRecordID = "rec1"
+)
+
+// fakePB stands in for PocketBase: it serves one record, but only to a caller
+// carrying the superuser token, and records how it was signed into.
+type fakePB struct {
+	mu sync.Mutex
+
+	// Credentials that will be accepted. Everything else gets a 400, as
+	// PocketBase gives for a bad identity/password.
+	email    string
+	password string
+
+	authCalls     int
+	lastIdentity  string
+	guestAttempts int // record reads that arrived without the superuser token
+}
+
+func (f *fakePB) handler() http.Handler {
+	mux := http.NewServeMux()
+
+	mux.HandleFunc("POST /api/collections/_superusers/auth-with-password", func(w http.ResponseWriter, r *http.Request) {
+		var body struct{ Identity, Password string }
+		_ = json.NewDecoder(r.Body).Decode(&body)
+		f.mu.Lock()
+		f.authCalls++
+		f.lastIdentity = body.Identity
+		ok := body.Identity == f.email && body.Password == f.password
+		f.mu.Unlock()
+		if !ok {
+			writeTestJSON(w, 400, map[string]any{"message": "Failed to authenticate."})
+			return
+		}
+		writeTestJSON(w, 200, map[string]any{"token": fakeSvcToken})
+	})
+
+	mux.HandleFunc("GET /api/collections/users/records/{id}", func(w http.ResponseWriter, r *http.Request) {
+		if r.Header.Get("Authorization") != fakeSvcToken {
+			f.mu.Lock()
+			f.guestAttempts++
+			f.mu.Unlock()
+			writeTestJSON(w, 404, map[string]any{
+				"data": map[string]any{}, "message": "The requested resource wasn't found.", "status": 404,
+			})
+			return
+		}
+		writeTestJSON(w, 200, map[string]any{"id": r.PathValue("id"), "email": "driver@test.local"})
+	})
+
+	return mux
+}
+
+func writeTestJSON(w http.ResponseWriter, status int, body any) {
+	w.Header().Set("Content-Type", "application/json")
+	w.WriteHeader(status)
+	_ = json.NewEncoder(w).Encode(body)
+}
+
+func newFakePB(t *testing.T, f *fakePB) string {
+	t.Helper()
+	srv := httptest.NewServer(f.handler())
+	t.Cleanup(srv.Close)
+	return srv.URL
+}
+
+func (f *fakePB) counts() (auth, guest int) {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	return f.authCalls, f.guestAttempts
+}
+
+// A client that never managed to authenticate at startup must sign in on its
+// first call rather than sending it as a guest and reporting PocketBase's 404.
+func TestGetOneAuthenticatesWhenNoTokenCached(t *testing.T) {
+	f := &fakePB{email: "admin@test.local", password: "pw"}
+	c := New(newFakePB(t, f), f.email, f.password)
+
+	var rec struct{ ID, Email string }
+	if err := c.GetOne(context.Background(), "users", fakeRecordID, &rec); err != nil {
+		t.Fatalf("GetOne: %v", err)
+	}
+	if rec.ID != fakeRecordID {
+		t.Fatalf("record id = %q, want %q", rec.ID, fakeRecordID)
+	}
+
+	auth, guest := f.counts()
+	if guest != 0 {
+		t.Errorf("%d record read(s) went out unauthenticated, want 0", guest)
+	}
+	if auth != 1 {
+		t.Errorf("authenticated %d time(s), want 1", auth)
+	}
+}
+
+// The token is cached: a second call reuses it instead of signing in again.
+func TestGetOneReusesCachedToken(t *testing.T) {
+	f := &fakePB{email: "admin@test.local", password: "pw"}
+	c := New(newFakePB(t, f), f.email, f.password)
+
+	for i := 0; i < 3; i++ {
+		if err := c.GetOne(context.Background(), "users", fakeRecordID, new(struct{})); err != nil {
+			t.Fatalf("GetOne #%d: %v", i, err)
+		}
+	}
+	if auth, _ := f.counts(); auth != 1 {
+		t.Errorf("authenticated %d time(s), want 1", auth)
+	}
+}
+
+// Retargeting from the panel clears the cached token, so the next call has to
+// sign in again — under the new credentials.
+func TestReconfigureReauthenticates(t *testing.T) {
+	f := &fakePB{email: "admin@test.local", password: "pw"}
+	url := newFakePB(t, f)
+	c := New(url, f.email, f.password)
+
+	if err := c.GetOne(context.Background(), "users", fakeRecordID, new(struct{})); err != nil {
+		t.Fatalf("GetOne before reconfigure: %v", err)
+	}
+
+	f.mu.Lock()
+	f.email, f.password = "new@test.local", "pw2"
+	f.mu.Unlock()
+	c.Reconfigure(url, "new@test.local", "pw2")
+
+	if err := c.GetOne(context.Background(), "users", fakeRecordID, new(struct{})); err != nil {
+		t.Fatalf("GetOne after reconfigure: %v", err)
+	}
+
+	auth, guest := f.counts()
+	if guest != 0 {
+		t.Errorf("%d record read(s) went out unauthenticated, want 0", guest)
+	}
+	if auth != 2 {
+		t.Errorf("authenticated %d time(s), want 2", auth)
+	}
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	if f.lastIdentity != "new@test.local" {
+		t.Errorf("signed in as %q, want the reconfigured account", f.lastIdentity)
+	}
+}
+
+// Bad credentials must surface as the authentication failure they are, not as
+// PocketBase's 404 for a record the client was never allowed to see.
+func TestGetOneReportsAuthFailureNotNotFound(t *testing.T) {
+	f := &fakePB{email: "admin@test.local", password: "pw"}
+	c := New(newFakePB(t, f), "admin@test.local", "wrong")
+
+	err := c.GetOne(context.Background(), "users", fakeRecordID, new(struct{}))
+	if err == nil {
+		t.Fatal("GetOne succeeded with bad credentials")
+	}
+	if apiErr, ok := err.(*APIError); ok && apiErr.Status == http.StatusNotFound {
+		t.Fatalf("got PocketBase's 404 instead of an auth failure: %v", err)
+	}
+}
+
+// With no service account configured there is nothing to sign in with, so the
+// call goes out as before — the endpoints that need superuser access answer 503
+// on their own rather than relying on this layer.
+func TestNoServiceAccountSkipsAuthentication(t *testing.T) {
+	f := &fakePB{email: "admin@test.local", password: "pw"}
+	c := New(newFakePB(t, f), "", "")
+
+	if err := c.GetOne(context.Background(), "users", fakeRecordID, new(struct{})); err == nil {
+		t.Fatal("GetOne succeeded without a service account")
+	}
+	if auth, _ := f.counts(); auth != 0 {
+		t.Errorf("attempted %d sign-in(s) with no credentials, want 0", auth)
+	}
+}