fix(agentgit): register the member's SSH key on every BBS login, not just at signup

cmd/agentbbs/gitkey_test.go

@@ -0,0 +1,81 @@
+package main
+
+import (
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/profullstack/agentbbs/internal/forgejo"
+	"github.com/profullstack/agentbbs/internal/store"
+)
+
+// Throwaway keys generated for these tests only.
+const testPubKey = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIOeWE4BpdSRsfc8l6w9clDKPTDH9GX/oYSgtxM3ohyhV chovy@bbs"
+
+func TestGitKeyTitleIsPerKey(t *testing.T) {
+	const other = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIHHfPGCIu0pk6TrpwdX9VBrGbQXUU44L5ovCRqFFsWSo chovy@bbs"
+
+	a := gitKeyTitle(testPubKey)
+	if !strings.HasPrefix(a, "agentbbs ") {
+		t.Errorf("title = %q, want an \"agentbbs \" prefix", a)
+	}
+	// Two different keys must not collide on title: Forgejo 422s a duplicate
+	// title, which would silently drop the member's rotated key.
+	if b := gitKeyTitle(other); a == b {
+		t.Errorf("distinct keys share the title %q", a)
+	}
+	// The same key is stable across logins, so we don't pile up entries.
+	if a != gitKeyTitle(testPubKey) {
+		t.Error("gitKeyTitle is not stable for the same key")
+	}
+	// Unparseable input still yields a usable title rather than panicking.
+	if got := gitKeyTitle("not-a-key"); got != "agentbbs" {
+		t.Errorf("gitKeyTitle(garbage) = %q, want \"agentbbs\"", got)
+	}
+}
+
+// TestProvisionGitRegistersKeyForExistingAccount is the regression guard for the
+// bug that lost chovy's key: provisionGit used to return early when the Forgejo
+// account already existed, so EnsureKey ran only at first provisioning and a key
+// deleted on AgentGit never came back on any later BBS login.
+func TestProvisionGitRegistersKeyForExistingAccount(t *testing.T) {
+	var posted map[string]any
+	createAttempted := false
+
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch {
+		// Account already exists — this is the case that used to bail out.
+		case r.Method == http.MethodGet && r.URL.Path == "/api/v1/users/chovy":
+			_, _ = w.Write([]byte(`{"id":1,"login":"chovy"}`))
+		case r.Method == http.MethodGet && r.URL.Path == "/api/v1/users/chovy/keys":
+			_, _ = w.Write([]byte(`[]`))
+		case r.Method == http.MethodPost && r.URL.Path == "/api/v1/admin/users/chovy/keys":
+			_ = json.NewDecoder(r.Body).Decode(&posted)
+			w.WriteHeader(http.StatusCreated)
+			_, _ = w.Write([]byte(`{"id":7}`))
+		case r.Method == http.MethodPost && r.URL.Path == "/api/v1/admin/users":
+			createAttempted = true
+			w.WriteHeader(http.StatusUnprocessableEntity)
+		default:
+			t.Errorf("unexpected %s %s", r.Method, r.URL.Path)
+		}
+	}))
+	defer srv.Close()
+
+	a := &app{forgejo: forgejo.Config{BaseURL: srv.URL, Token: "secret"}}
+	u := &store.User{ID: 1, Name: "chovy", Email: "chovy@example.com", EmailVerified: true}
+
+	a.provisionGit(u, testPubKey)
+
+	if createAttempted {
+		t.Error("must not re-create an account that already exists")
+	}
+	if posted == nil {
+		t.Fatal("key was never POSTed for an existing account — the early return is back")
+	}
+	if posted["key"] != testPubKey {
+		t.Errorf("posted key = %v, want %q", posted["key"], testPubKey)
+	}
+}

cmd/agentbbs/main.go

@@ -1236,23 +1236,28 @@ func (a *app) provisionGit(u *store.User, pubKey string) {
 		log.Error("forgejo provision", "user", u.Name, "err", err)
 		return
 	}
-	if !created {
-		return
+	if created {
+		log.Info("provisioned git account", "user", u.Name, "host", a.forgejo.BaseURL)
 	}
-	log.Info("provisioned git account", "user", u.Name, "host", a.forgejo.BaseURL)
 	// Register the BBS SSH key so the member can push with the same key they sign
-	// in with. No-op when called without a session key (e.g. the web verify flow).
+	// in with. This runs on EVERY call, not only when the account was just made:
+	// EnsureKey is idempotent, so re-running it re-adds a key the member removed
+	// on AgentGit, picks up a rotated BBS key, and backfills members who joined
+	// before the key was captured. Gating it on `created` meant a member's key
+	// could only ever be registered once, and never came back once deleted.
+	// No-op when called without a session key (e.g. the web verify flow).
 	if pubKey != "" {
-		if added, err := a.forgejo.EnsureKey(u.Name, "agentbbs", pubKey); err != nil {
+		if added, err := a.forgejo.EnsureKey(u.Name, gitKeyTitle(pubKey), pubKey); err != nil {
 			log.Error("forgejo ssh key", "user", u.Name, "err", err)
 		} else if added {
 			log.Info("registered git ssh key", "user", u.Name)
 		}
 	}
 	// Email the verified address their web sign-in link + one-time password so
-	// they can log in to the Forgejo UI and create repositories. Best-effort:
-	// the account already exists, so a mail failure must not block anything.
-	if a.mail.Configured() {
+	// they can log in to the Forgejo UI and create repositories. First creation
+	// only — password is empty for an account that already existed. Best-effort:
+	// a mail failure must not block anything.
+	if created && a.mail.Configured() {
 		if err := a.mail.Send(u.Email, "Your git.profullstack.com account is ready",
 			gitWelcomeEmailBody(u.Name, password, a.forgejo.LoginURL())); err != nil {
 			log.Error("git welcome email", "user", u.Name, "err", err)
@@ -1276,6 +1281,22 @@ func gitWelcomeEmailBody(name, password, loginURL string) string {
 		"If you didn't request this, you can ignore this email.\n"
 }
 
+// gitKeyTitle labels the key in Forgejo. The fingerprint is baked into the title
+// so a member who rotates their BBS key gets a second entry rather than colliding
+// with the old one — Forgejo rejects a duplicate title with 422, which would
+// otherwise drop the new key on the floor.
+func gitKeyTitle(pubKey string) string {
+	pk, _, _, _, err := gossh.ParseAuthorizedKey([]byte(pubKey))
+	if err != nil {
+		return "agentbbs"
+	}
+	fp := strings.TrimPrefix(gossh.FingerprintSHA256(pk), "SHA256:")
+	if len(fp) > 12 {
+		fp = fp[:12]
+	}
+	return "agentbbs " + fp
+}
+
 // authorizedKey renders the session's public key as a single authorized_keys
 // line, or "" when the session has no key (guests / keyboard-interactive).
 func authorizedKey(s ssh.Session) string {

internal/forgejo/forgejo.go

@@ -207,15 +207,30 @@ func (c Config) EnsureKey(username, title, pubKey string) (added bool, err error
 	if err != nil {
 		return false, err
 	}
+	// Forgejo answers 422 both for "this key/title is already here" (benign, we
+	// raced or the comment differs) and for "this key content is unusable".
+	// Treating every 422 as benign hid real rejections forever, so only swallow
+	// the ones that say the key or title is already taken.
 	if status == http.StatusUnprocessableEntity {
-		return false, nil // key already exists (raced or comment differs)
+		if alreadyUsed(resp) {
+			return false, nil
+		}
+		return false, fmt.Errorf("forgejo rejected key %q: %s", username, truncate(resp, 200))
 	}
 	if status < 200 || status >= 300 {
 		return false, fmt.Errorf("forgejo add key %q: %d: %s", username, status, truncate(resp, 200))
 	}
 	return true, nil
 }
 
+// alreadyUsed reports whether a 422 body is Forgejo saying the key or its title
+// is already on the account, as opposed to rejecting the key content itself.
+// Forgejo's wording: "Key content has been used as non-deploy key" /
+// "Key title has been used".
+func alreadyUsed(resp string) bool {
+	return strings.Contains(strings.ToLower(resp), "has been used")
+}
+
 // keyMaterial returns the type+base64 of an authorized-key line, dropping the
 // optional comment so the same key compares equal regardless of how it's labeled.
 func keyMaterial(authorizedKey string) string {

internal/forgejo/key422_test.go

@@ -0,0 +1,56 @@
+package forgejo
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+)
+
+// key422Server answers the dedupe GET with an empty list, then returns 422 with
+// the given body for the POST.
+func key422Server(t *testing.T, body string) *httptest.Server {
+	t.Helper()
+	return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.Method == http.MethodGet {
+			_, _ = w.Write([]byte(`[]`))
+			return
+		}
+		w.WriteHeader(http.StatusUnprocessableEntity)
+		_, _ = w.Write([]byte(body))
+	}))
+}
+
+func TestEnsureKeyTreatsAlreadyUsed422AsBenign(t *testing.T) {
+	srv := key422Server(t, `{"message":"Key content has been used as non-deploy key"}`)
+	defer srv.Close()
+
+	c := Config{BaseURL: srv.URL, Token: "secret"}
+	added, err := c.EnsureKey("alice", "agentbbs", aliceKey)
+	if err != nil {
+		t.Fatalf("a duplicate key must not be an error, got %v", err)
+	}
+	if added {
+		t.Error("expected added=false for a key already on the account")
+	}
+}
+
+// A 422 that is Forgejo rejecting the key content must surface, not be silently
+// swallowed as "already exists" — that is how an unregisterable key stayed
+// invisible in the logs forever.
+func TestEnsureKeySurfacesRejecting422(t *testing.T) {
+	srv := key422Server(t, `{"message":"Key content is not a valid SSH key"}`)
+	defer srv.Close()
+
+	c := Config{BaseURL: srv.URL, Token: "secret"}
+	added, err := c.EnsureKey("alice", "agentbbs", aliceKey)
+	if err == nil {
+		t.Fatal("expected an error when Forgejo rejects the key content")
+	}
+	if added {
+		t.Error("expected added=false on rejection")
+	}
+	if !strings.Contains(err.Error(), "not a valid SSH key") {
+		t.Errorf("error should carry Forgejo's reason, got %v", err)
+	}
+}