diff --git a/cmd/agentbbs/gitkey_test.go b/cmd/agentbbs/gitkey_test.go new file mode 100644 index 0000000..f8781a8 --- /dev/null +++ b/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) + } +} diff --git a/cmd/agentbbs/main.go b/cmd/agentbbs/main.go index d76b3a9..675fbcf 100644 --- a/cmd/agentbbs/main.go +++ b/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 { diff --git a/internal/forgejo/forgejo.go b/internal/forgejo/forgejo.go index b551cac..1860e92 100644 --- a/internal/forgejo/forgejo.go +++ b/internal/forgejo/forgejo.go @@ -207,8 +207,15 @@ 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)) @@ -216,6 +223,14 @@ func (c Config) EnsureKey(username, title, pubKey string) (added bool, err error 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 { diff --git a/internal/forgejo/key422_test.go b/internal/forgejo/key422_test.go new file mode 100644 index 0000000..be9453d --- /dev/null +++ b/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) + } +}