diff --git a/internal/auth/agent_connect.go b/internal/auth/agent_connect.go index fafca6a99..930f47131 100644 --- a/internal/auth/agent_connect.go +++ b/internal/auth/agent_connect.go @@ -111,6 +111,18 @@ type AgentConnectResult struct { ClientID string } +// AgentConnectIntake is what a person needs to approve a connection: the +// link to open, the code to check against the page, and when the code +// dies. Both are the copies the terminal shows, stripped of control +// sequences: whoever reads them shows them to a person, and a JSON encoder +// passes UTF-8-encoded C1 controls through raw. The browser is still sent +// to the validated URL itself. +type AgentConnectIntake struct { + VerificationURI string + UserCode string + ExpiresAt time.Time +} + // AgentConnectOptions configures the connection ceremony. type AgentConnectOptions struct { // DeviceName is where this connector runs, self-asserted and shown to @@ -146,6 +158,16 @@ type AgentConnectOptions struct { // the poll runs, exactly as it does for a device login. Progress io.Writer + // OnIntake, when set, is told what the operator needs to approve the + // connection as soon as the intake has answered, before the browser is + // opened and the wait begins: a caller reading the ceremony as data + // learns the link and the code without parsing the operator's half. + // It is never given the device code, which is the poll's bearer and + // stays inside this flow. A non-nil error ends the ceremony before + // the browser is opened or anything waits: the link has not reached + // whoever was to show it, and nothing is stored. + OnIntake func(AgentConnectIntake) error + // BeforeStore, when set, runs after the poll has handed over the // credential and the mint has proved it, and before anything is // written. A non-nil error aborts the connection and stores nothing. @@ -256,6 +278,24 @@ func (m *Manager) ConnectAgent(ctx context.Context, opts AgentConnectOptions) (* return nil, err } + // One deadline for the whole ceremony, fixed when the intake answers: + // what OnIntake publishes, what the operator is told and where the + // poll stops are the same instant, however long the caller takes. + intake.expiresAt = opts.now().Add(intake.lifetime) + if opts.OnIntake != nil { + if err := opts.OnIntake(AgentConnectIntake{ + VerificationURI: intake.shownURI, + UserCode: intake.userCode, + ExpiresAt: intake.expiresAt, + }); err != nil { + return nil, err + } + if !opts.now().Before(intake.expiresAt) { + // Nothing left to announce, open or poll. + return nil, agentConnectExpired() + } + } + wait := announceAgentConnection(opts.presentation(), intake, opts.now()) conn, err := m.awaitAgentConnection(ctx, &opts, client, intake) wait.Stop() @@ -380,6 +420,10 @@ type agentIntake struct { tokenURI string lifetime time.Duration interval time.Duration + + // expiresAt is when the code dies, fixed once by ConnectAgent as the + // intake answers; everything after it counts down to this instant. + expiresAt time.Time } // openAgentConnection makes the anonymous intake request and validates @@ -463,7 +507,7 @@ func announceAgentConnection(view *LoginOptions, intake *agentIntake, now time.T view.log("\nConnect this computer to a Basecamp agent\n") view.log(" 1. Open this link on any device") view.log(" " + intake.shownURI) - view.log(" 2. Check that the code shown there matches (expires in " + expiresIn(intake.lifetime) + ")") + view.log(" 2. Check that the code shown there matches (expires in " + expiresIn(intake.expiresAt.Sub(now)) + ")") view.log(" " + intake.userCode) view.log(" 3. Pick the agent this computer acts as, and approve it") view.log("") @@ -472,10 +516,10 @@ func announceAgentConnection(view *LoginOptions, intake *agentIntake, now time.T view.log("") view.announceBrowser(intake.verificationURI) - if wait := startApprovalWait(view.Progress, now.Add(intake.lifetime)); wait != nil { + if wait := startApprovalWait(view.Progress, intake.expiresAt); wait != nil { return wait } - view.log("Waiting for approval… (the code expires in " + expiresIn(intake.lifetime) + ")") + view.log("Waiting for approval… (the code expires in " + expiresIn(intake.expiresAt.Sub(now)) + ")") return nil } @@ -487,7 +531,7 @@ func announceAgentConnection(view *LoginOptions, intake *agentIntake, now time.T // the backstop for a server that answers authorization_pending forever — // the code it issued is dead by then whatever it says. func (m *Manager) awaitAgentConnection(ctx context.Context, opts *AgentConnectOptions, client *http.Client, intake *agentIntake) (*AgentConnection, error) { - deadline := opts.now().Add(intake.lifetime) + deadline := intake.expiresAt interval := intake.interval for { // Never wait past the code's own life: an interval longer than diff --git a/internal/auth/agent_connect_test.go b/internal/auth/agent_connect_test.go index e4da47f27..6f324b6ee 100644 --- a/internal/auth/agent_connect_test.go +++ b/internal/auth/agent_connect_test.go @@ -591,6 +591,37 @@ func TestConnectAgentStopsWhenTheCodeLifetimeRunsOut(t *testing.T) { assertNoAgentCredential(t, m) } +// TestConnectAgentKeepsTheDeadlineItPublished: the code dies when the +// intake said it would, however long the caller took over OnIntake. A +// callback that outlived the code leaves nothing to announce, open or +// poll: the deadline it was given is the one the ceremony keeps. +func TestConnectAgentKeepsTheDeadlineItPublished(t *testing.T) { + as := startConnectAS(t) + m := connectManager(t, as) + clock := newTestClock() + + opts := connectOptions(&collectLogger{}, clock) + opts.NoBrowser = false + opts.Local = true + opts.BrowserLauncher = func(string) error { + t.Fatal("opened the browser on a code that had already expired") + return nil + } + var published time.Time + opts.OnIntake = func(intake AgentConnectIntake) error { + published = intake.ExpiresAt + // The caller takes the code's whole life and a second more. + return clock.sleep(context.Background(), intake.ExpiresAt.Sub(clock.Now())+time.Second) + } + + _, err := m.ConnectAgent(context.Background(), opts) + require.Error(t, err) + assert.Contains(t, err.Error(), "expired") + assert.False(t, published.IsZero()) + assert.Empty(t, as.calls(&as.pollForms), "nothing polls a code past the deadline it was published with") + assertNoAgentCredential(t, m) +} + // TestConnectAgentReportsAnAgentThatIsNoLongerActive: an account canceled // or frozen between approval and the poll answers invalid_grant, and the // remedy is a new connection, not a retry. diff --git a/internal/commands/auth_agent.go b/internal/commands/auth_agent.go index a70239f35..5924f0793 100644 --- a/internal/commands/auth_agent.go +++ b/internal/commands/auth_agent.go @@ -1,8 +1,11 @@ package commands import ( + "encoding/json" "fmt" + "io" "os" + "time" "github.com/spf13/cobra" @@ -65,6 +68,21 @@ Over SSH, in CI, or on a host with no display the browser is skipped and the link is yours to open on any device; --local forces a launch anyway, --no-browser skips it. Ctrl-C cancels the wait and stores nothing. +With --json, the ceremony is data for whatever runs it, and stdout carries +two JSON values. The first is one line, written as soon as Basecamp +answers the request: + + {"type":"verification","verification_uri":"…","user_code":"WDJB-MJHT","expires_at":"…","expires_in":600} + +The second is the result: the envelope every command prints under --json, +or its data alone under --agent — the profile, its account, the access +approved and the agent's client id — or the error, if +the operator declined, the code expired, or nothing could be stored. The words meant +for a person go to stderr, and the browser opens as it would without +--json. Neither line ever carries the client secret or the device code. +The link and the code are for the person approving at this computer: +show them there, never anywhere else. + Disconnecting is done in Basecamp: disconnecting the agent there kills the client secret this holds. Drop the local copy with ` + "`basecamp auth logout -P `" + `.`, Annotations: map[string]string{AnnotationProfileMayCreate: "true"}, @@ -75,7 +93,7 @@ client secret this holds. Drop the local copy with ` + "`basecamp auth logout -P return fmt.Errorf("app not initialized") } - if err := refuseMachineOutputLogin(app, "the agent connection"); err != nil { + if err := refuseAgentConnectOutputMode(app); err != nil { return err } if err := refuseNonInteractiveAgentConnect(); err != nil { @@ -90,6 +108,7 @@ client secret this holds. Drop the local copy with ` + "`basecamp auth logout -P return connectAgentProfile(cmd, app, agentConnectFlags{ deviceName: deviceName, softwareName: softwareName, scope: scope, noBrowser: noBrowser, local: local, + machine: app.Flags.JSON || app.Flags.Agent, }) }, } @@ -114,6 +133,58 @@ type agentConnectFlags struct { // quiet leaves out what was stored, for the guided setup, which says // in one line of its own which agent this computer is now connected as. quiet bool + + // machine reads the ceremony out as data: the verification line and + // the result envelope on stdout, the operator's words on stderr. + machine bool +} + +// refuseAgentConnectOutputMode lets the ceremony run under --json (and +// --agent, its data-only form), and refuses the output modes that cannot +// carry it. A --jq filter runs over one envelope, and this writes two +// lines; --quiet, --ids-only and --count would each throw the verification +// line away, and with it the only thing the operator needs. +// +// --agent is let through as --json is: it prints the result's data alone, +// after the same verification line. +func refuseAgentConnectOutputMode(app *appctx.App) error { + if app.Flags.JQFilter != "" { + return output.ErrJQNotSupported("the agent connection") + } + if app.Flags.Quiet || app.Flags.IDsOnly || app.Flags.Count { + return output.ErrUsageHint("The agent connection cannot run under --quiet, --ids-only or --count", + "Run it plainly for a person at a terminal, or with --json to read the link, the code and the result as JSON lines.") + } + return nil +} + +// agentConnectVerification is the ceremony's first JSON line: what the +// operator needs to approve the connection, and nothing else. +type agentConnectVerification struct { + Type string `json:"type"` + VerificationURI string `json:"verification_uri"` + UserCode string `json:"user_code"` + ExpiresAt string `json:"expires_at"` + ExpiresIn int `json:"expires_in"` +} + +// writeAgentConnectVerification writes the verification line, one JSON +// object terminated by a newline, so a reader can act on it while the +// ceremony is still waiting. A write that fails ends the ceremony: under +// --json this line is how the link and the code reach whoever shows them +// to the person, so nothing should wait on an approval they never saw. +func writeAgentConnectVerification(w io.Writer, intake auth.AgentConnectIntake) error { + line := agentConnectVerification{ + Type: "verification", + VerificationURI: intake.VerificationURI, + UserCode: intake.UserCode, + ExpiresAt: intake.ExpiresAt.UTC().Format(time.RFC3339), + ExpiresIn: max(0, int(time.Until(intake.ExpiresAt).Round(time.Second).Seconds())), + } + if err := json.NewEncoder(w).Encode(line); err != nil { + return fmt.Errorf("could not write the verification line, so the connection was stopped: %w", err) + } + return nil } // connectAgentProfile runs the agent-connection handshake for the active @@ -126,7 +197,15 @@ func connectAgentProfile(cmd *cobra.Command, app *appctx.App, f agentConnectFlag } w := cmd.OutOrStdout() - r := output.NewRendererWithTheme(w, false, tui.ResolveTheme(tui.DetectDark())) + // Under --json stdout is the data, so the operator's half of the + // ceremony — the link, the code, the wait line — goes to stderr. + human := w + var onIntake func(auth.AgentConnectIntake) error + if f.machine { + human = cmd.ErrOrStderr() + onIntake = func(intake auth.AgentConnectIntake) error { return writeAgentConnectVerification(w, intake) } + } + r := output.NewRendererWithTheme(human, false, tui.ResolveTheme(tui.DetectDark())) var isDefault bool ctx, stop := loginContext(cmd) @@ -136,20 +215,38 @@ func connectAgentProfile(cmd *cobra.Command, app *appctx.App, f agentConnectFlag Scope: f.scope, NoBrowser: f.noBrowser, Local: f.local, - Logger: func(msg string) { fmt.Fprintln(w, msg) }, - Progress: w, + Logger: func(msg string) { fmt.Fprintln(human, msg) }, + Progress: human, + OnIntake: onIntake, BeforeStore: func(conn *auth.AgentConnection) error { registered, commitErr := target.commit(app, conn) isDefault = registered return commitErr }, }) - err = loginOutcome(ctx, err, w, r) + err = loginOutcome(ctx, err, human, r) stop() if err != nil || f.quiet { return err } + if f.machine { + data := map[string]any{ + "profile": target.name, + "account_id": result.AccountID, + "base_url": app.Config.BaseURL, + "source": "agent_connection", + "oauth_type": result.OAuthType, + "scope": result.Scope, + "client_id": result.ClientID, + "profile_created": target.existing == nil, + } + if isDefault { + data["default"] = true + } + return app.OK(data, output.WithSummary(fmt.Sprintf("Connected profile %q to a Basecamp agent", target.name))) + } + fmt.Fprintln(w) fmt.Fprintln(w, r.Success.Render(fmt.Sprintf("Connected profile %q to a Basecamp agent", target.name))) fmt.Fprintln(w, r.Muted.Render(fmt.Sprintf("Profile: %s · Account: %s · Access: %s · Token: minted on demand, no refresh token", diff --git a/internal/commands/auth_agent_connect_test.go b/internal/commands/auth_agent_connect_test.go index 7c46da3de..5dda615a7 100644 --- a/internal/commands/auth_agent_connect_test.go +++ b/internal/commands/auth_agent_connect_test.go @@ -3,10 +3,14 @@ package commands import ( "bytes" "context" + "encoding/json" + "errors" "fmt" + "io" "net/http" "net/http/httptest" "net/url" + "strings" "sync" "testing" @@ -31,12 +35,21 @@ type connectAS struct { srv *httptest.Server mu sync.Mutex + intakes int pollForms []url.Values tokenForms []url.Values // connection renders the successful poll; the default approves a // full-scope agent in account 999. connection func() string + + // pollStatus, when non-zero, is the poll's status in place of 200: a + // refusal, rendered from connection. + pollStatus int + + // verificationPath, when set, is the path and query of the link the + // intake hands out, in place of the plain approval page. + verificationPath string } func startConnectAS(t *testing.T) *connectAS { @@ -48,9 +61,16 @@ func startConnectAS(t *testing.T) *connectAS { mux := http.NewServeMux() mux.HandleFunc("/oauth/agent_connections", func(w http.ResponseWriter, _ *http.Request) { + as.mu.Lock() + as.intakes++ + as.mu.Unlock() + path := "/connect?user_code=WDJB-MJHT" + if as.verificationPath != "" { + path = as.verificationPath + } w.Header().Set("Content-Type", "application/json") fmt.Fprintf(w, `{"device_code":"dev-code-1","user_code":"WDJB-MJHT","verification_uri_complete":%q,"token_uri":%q,"expires_in":600,"interval":1}`, - as.srv.URL+"/connect?user_code=WDJB-MJHT", as.srv.URL+"/oauth/agent_connection_tokens") + as.srv.URL+path, as.srv.URL+"/oauth/agent_connection_tokens") }) mux.HandleFunc("/oauth/agent_connection_tokens", func(w http.ResponseWriter, r *http.Request) { require.NoError(t, r.ParseForm()) @@ -58,6 +78,9 @@ func startConnectAS(t *testing.T) *connectAS { as.pollForms = append(as.pollForms, r.PostForm) as.mu.Unlock() w.Header().Set("Content-Type", "application/json") + if as.pollStatus != 0 { + w.WriteHeader(as.pollStatus) + } fmt.Fprint(w, as.connection()) }) mux.HandleFunc("/oauth/tokens", func(w http.ResponseWriter, r *http.Request) { @@ -199,17 +222,207 @@ func TestAuthAgentConnectNeedsAProfile(t *testing.T) { assert.Empty(t, as.mints()) } -// TestAuthAgentConnectRefusesMachineOutput: the link, the code and the wait -// line go to stdout, which an envelope also owns. -func TestAuthAgentConnectRefusesMachineOutput(t *testing.T) { +// runAgentConnectJSON runs the ceremony under --json with stdout and stderr +// kept apart, as a program driving it would read them. stdout is a +// lockedBuffer so the mock server's handlers can read it mid-ceremony. +func runAgentConnectJSON(t *testing.T, app *appctx.App, stdout io.Writer, args ...string) (string, error) { + t.Helper() + app.Flags.JSON = true + app.Output = output.New(output.Options{Format: output.FormatJSON, Writer: stdout}) + cmd := NewAuthCmd() + cmd.SetArgs(append([]string{"agent", "connect", "--no-browser"}, args...)) + cmd.SetContext(appctx.WithApp(context.Background(), app)) + var stderr bytes.Buffer + cmd.SetOut(stdout) + cmd.SetErr(&stderr) + cmd.SilenceErrors = true + cmd.SilenceUsage = true + err := cmd.Execute() + return stderr.String(), err +} + +// jsonValues reads stdout as a stream of JSON values, the way a program +// driving the ceremony would: the verification line, then the envelope, +// however that is laid out. +func jsonValues(t *testing.T, stdout string) []map[string]any { + t.Helper() + // Decode to io.EOF rather than while More(): More is for the elements + // of an array or object, and at the top level it stops quietly at a + // stray closing delimiter that a reader of the stream would choke on. + dec := json.NewDecoder(strings.NewReader(stdout)) + var values []map[string]any + for { + var v map[string]any + err := dec.Decode(&v) + if errors.Is(err, io.EOF) { + return values + } + require.NoError(t, err, "stdout is JSON values and nothing else: %q", stdout) + values = append(values, v) + } +} + +// TestAuthAgentConnectJSONWritesTheVerificationThenTheResult: under --json +// the ceremony is two lines of data on stdout. The first carries what the +// operator needs and is written while the ceremony is still waiting on +// them; the last is the result. The operator's words go to stderr. Neither +// stream carries the client secret or the device code. +func TestAuthAgentConnectJSONWritesTheVerificationThenTheResult(t *testing.T) { as := startConnectAS(t) app := connectApp(t, as, &config.Config{ActiveProfile: "agent"}) - app.Flags.JSON = true + stdout := &lockedBuffer{} + + // The poll handler runs on the server's goroutine, so what it saw is + // handed over under a lock. + approve := as.connection + var ( + atPollMu sync.Mutex + atPoll string + ) + as.connection = func() string { + atPollMu.Lock() + atPoll = stdout.String() + atPollMu.Unlock() + return approve() + } - _, err := runAgentConnect(t, app) + stderr, err := runAgentConnectJSON(t, app, stdout, "--device-name", "build-box") + require.NoError(t, err, stderr) + + atPollMu.Lock() + defer atPollMu.Unlock() + require.NotEmpty(t, atPoll, "the verification line is written before the operator approves, not after") + require.True(t, strings.HasSuffix(atPoll, "}\n") && strings.Count(atPoll, "\n") == 1, + "the verification is one line, newline-terminated, so a reader can act on it at once: %q", atPoll) + verification := jsonValues(t, atPoll) + require.Len(t, verification, 1) + assert.Equal(t, "verification", verification[0]["type"]) + assert.Equal(t, as.srv.URL+"/connect?user_code=WDJB-MJHT", verification[0]["verification_uri"]) + assert.Equal(t, "WDJB-MJHT", verification[0]["user_code"]) + assert.NotEmpty(t, verification[0]["expires_at"]) + expiresIn, ok := verification[0]["expires_in"].(float64) + require.True(t, ok) + assert.InDelta(t, 600, expiresIn, 5) + + lines := jsonValues(t, stdout.String()) + require.Len(t, lines, 2, "the verification line, then the result") + data, ok := lines[1]["data"].(map[string]any) + require.True(t, ok, stdout.String()) + assert.Equal(t, "agent", data["profile"]) + assert.Equal(t, "999", data["account_id"]) + assert.Equal(t, "agent", data["oauth_type"]) + assert.Equal(t, "agent_connection", data["source"]) + assert.Equal(t, "full", data["scope"]) + assert.Equal(t, "agent-client", data["client_id"]) + assert.Equal(t, true, data["profile_created"]) + assert.Equal(t, true, data["default"]) + assert.Equal(t, `Connected profile "agent" to a Basecamp agent`, lines[1]["summary"]) + + assert.Contains(t, stderr, "WDJB-MJHT", "a person at the terminal still sees the code") + assert.NotContains(t, stderr, "Connected profile", "the result is data, not a second copy in prose") + for name, stream := range map[string]string{"stdout": stdout.String(), "stderr": stderr} { + assert.NotContains(t, stream, fakeConnectSecret, "%s carries the client secret", name) + assert.NotContains(t, stream, "dev-code-1", "%s carries the device code, the poll's bearer", name) + } + + creds, err := app.Auth.GetStore().Load("profile:agent") + require.NoError(t, err) + assert.Equal(t, fakeConnectSecret, creds.ClientSecret, "the credential is stored exactly as without --json") +} + +// TestAuthAgentConnectJSONVerificationCarriesNoControls: the link is what +// the reader shows the person, and the JSON encoder escapes C0 controls but +// passes UTF-8-encoded C1 controls through raw. A link the server laced +// with one (url.Parse accepts it) reaches the line stripped, as the +// terminal copy is, never as the bytes a terminal would act on. +func TestAuthAgentConnectJSONVerificationCarriesNoControls(t *testing.T) { + as := startConnectAS(t) + as.verificationPath = "/connect\u009b31m?user_code=WDJB-MJHT" + app := connectApp(t, as, &config.Config{ActiveProfile: "agent"}) + stdout := &lockedBuffer{} + + stderr, err := runAgentConnectJSON(t, app, stdout, "--device-name", "build-box") + require.NoError(t, err, stderr) + + assert.NotContains(t, stdout.String(), "\u009b", "a C1 control must not reach stdout raw") + lines := jsonValues(t, stdout.String()) + require.Len(t, lines, 2) + assert.Equal(t, as.srv.URL+"/connect31m?user_code=WDJB-MJHT", lines[0]["verification_uri"], + "the link is the copy the terminal shows") +} + +// failingWriter is a stdout that refuses every write: a full disk behind a +// redirect, or an embedding whose sink has failed. +type failingWriter struct{} + +func (failingWriter) Write([]byte) (int, error) { return 0, errors.New("no space left on device") } + +// TestAuthAgentConnectJSONStopsWhenTheVerificationCannotBeWritten: under +// --json the verification line is how the link and the code reach whoever +// shows them to the person, so a stdout that refuses it ends the ceremony +// there: no wait, no poll, nothing stored. +func TestAuthAgentConnectJSONStopsWhenTheVerificationCannotBeWritten(t *testing.T) { + as := startConnectAS(t) + app := connectApp(t, as, &config.Config{ActiveProfile: "agent"}) + + _, err := runAgentConnectJSON(t, app, failingWriter{}, "--device-name", "build-box") require.Error(t, err) - assert.Contains(t, err.Error(), "machine output mode") + assert.Contains(t, err.Error(), "no space left on device") + + as.mu.Lock() + polls := len(as.pollForms) + as.mu.Unlock() + assert.Zero(t, polls, "nothing waits on an approval whose link never reached anyone") assert.Empty(t, as.mints()) + assertNothingStored(t, app, "agent") +} + +// TestAuthAgentConnectJSONOnADeclineWritesOnlyTheVerification: a declined +// connection ends with an error, which the CLI renders as the last line, +// and nothing is stored. The verification line is already out by then. +func TestAuthAgentConnectJSONOnADeclineWritesOnlyTheVerification(t *testing.T) { + as := startConnectAS(t) + as.pollStatus = 400 + as.connection = func() string { return `{"error":"access_denied"}` } + app := connectApp(t, as, &config.Config{ActiveProfile: "agent"}) + stdout := &lockedBuffer{} + + _, err := runAgentConnectJSON(t, app, stdout, "--device-name", "build-box") + require.Error(t, err) + + lines := jsonValues(t, stdout.String()) + require.Len(t, lines, 1, "no result envelope from the command itself; the error is the CLI's to render") + assert.Equal(t, "verification", lines[0]["type"]) + assert.Empty(t, as.mints(), "a declined connection mints nothing") + assertNothingStored(t, app, "agent") +} + +// TestAuthAgentConnectRefusesOutputModesThatCannotCarryIt: --jq filters one +// envelope and this writes two lines; --quiet, --ids-only and --count would +// throw the verification line away. Each is refused before the server is +// asked for anything. +func TestAuthAgentConnectRefusesOutputModesThatCannotCarryIt(t *testing.T) { + for name, set := range map[string]func(*appctx.App){ + "jq": func(a *appctx.App) { a.Flags.JQFilter = ".data" }, + "quiet": func(a *appctx.App) { a.Flags.Quiet = true }, + "ids-only": func(a *appctx.App) { a.Flags.IDsOnly = true }, + "count": func(a *appctx.App) { a.Flags.Count = true }, + } { + t.Run(name, func(t *testing.T) { + as := startConnectAS(t) + app := connectApp(t, as, &config.Config{ActiveProfile: "agent"}) + set(app) + + _, err := runAgentConnect(t, app) + require.Error(t, err) + assert.Equal(t, output.CodeUsage, output.AsError(err).Code) + as.mu.Lock() + intakes := as.intakes + as.mu.Unlock() + assert.Zero(t, intakes, "refused before the intake is asked for a code") + assert.Empty(t, as.mints()) + }) + } } // TestAuthAgentConnectRefusesNonInteractive: approving a connection needs a diff --git a/skill-evals/cases/basecamp-connect/first-time-setup.yml b/skill-evals/cases/basecamp-connect/first-time-setup.yml index 5fbe18f4e..b24be287a 100644 --- a/skill-evals/cases/basecamp-connect/first-time-setup.yml +++ b/skill-evals/cases/basecamp-connect/first-time-setup.yml @@ -5,8 +5,8 @@ context: >- Everything you need is here, so don't stop to ask: the agent's profile should be called helper, and the agent is called Helper: if the credential turns out to be Helper, I confirm that is the agent. My own profile is jorge. - Only I should be able to drive the agent. When a connection link and code - are printed I open the link and approve the connection. + Only I should be able to drive the agent. When you show me a connection link + and code I open the link and approve the connection. tags: [connect] mocks: - match: 'auth status.*jorge' @@ -19,8 +19,13 @@ mocks: output: '{"ok":false,"code":"api_error","error":"unknown profile \"helper\" (available: jorge)"}' - match: 'profile list' output: '{"ok":true,"data":[{"name":"jorge","account_id":"999","authenticated":true,"base_url":"https://3.basecampapi.com","default":true}],"summary":"1 profile"}' + # Under --json, stdout is two JSON values: the verification line, then the + # result envelope. The person's prose goes to stderr, which the skill does + # not read. - match: 'auth agent connect' - output: "Open this link and enter the code ABCD-EFGH to approve the connection.\nWaiting for approval...\n\nConnected profile \"helper\" to a Basecamp agent\nProfile: helper · Account: 999 · Access: full · Token: minted on demand, no refresh token\nCreated profile \"helper\" for account 999\nCheck it any time: basecamp auth status -P helper" + output: |- + {"type":"verification","verification_uri":"https://3.basecamp.com/connect?user_code=ABCD-EFGH","user_code":"ABCD-EFGH","expires_at":"2026-10-07T12:10:00Z","expires_in":600} + {"ok":true,"data":{"profile":"helper","account_id":"999","base_url":"https://3.basecampapi.com","source":"agent_connection","oauth_type":"agent","scope":"full","client_id":"agent-client","profile_created":true},"summary":"Connected profile \"helper\" to a Basecamp agent"} - match: '^me\b' output: '{"ok":true,"data":{"identity":{"id":7001,"first_name":"Helper","last_name":"","email_address":""},"accounts":[{"id":999,"name":"Acme","current":true}],"person":{"id":4001,"name":"Helper"}}}' - match: 'projects list' @@ -34,6 +39,9 @@ expect_sequence: - match: '^me .*(-P|--profile)[ =]''?helper\b' - match: 'connect setup' accept: + # The connection is read as data: the skill relays the verification line's + # fields rather than scraping the terminal copy. + - 'auth agent connect\b.*\s(--json|-j)(\s|$)' - 'connect setup .*(-P|--profile)[ =]''?helper\b' - 'connect setup .*--operator-profile[ =]''?jorge\b' - 'connect setup .*--serve[ =](''222''|"222"|222)(\s|$)' @@ -88,8 +96,10 @@ reject: - 'connect setup .*--unserve\b' - '--with-token' - '--with-client-credentials' - # Every machine-output mode the interactive connection refuses. - - 'auth agent connect.*(--json|--agent|--quiet|--ids-only|--count|--jq|-j\b|-q\b)' + # The output modes the agent connection refuses: none can carry its + # verification line. --json is required above; --agent is not listed, + # because the CLI takes it, and these rules reject only what it refuses. + - 'auth agent connect.*(--quiet|--ids-only|--count|--jq|-q\b)' - 'connect setup .*--operator[ =]' - 'connect setup .*--(trust[ =]''?(allowlist|project)|allow[ =])' - '--expect-identity' diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index 620b62537..3eae11f0f 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -149,10 +149,22 @@ anything you post. first (`basecamp projects list -P '' --json`), and pass the id. The project called `Launch $(date)` is served as `--serve 222`, never by name. -**Interactive logins.** `basecamp auth agent connect`, `basecamp auth login` and -`basecamp profile create` print a link and wait for a person. Run them without -`--json` or `--quiet`, in the background, and read their output as it arrives, -so you can show the link and code while they wait. +**Interactive logins.** `basecamp auth agent connect` waits for a person to +approve it. Run it with `--json`, in the background, and read its stdout as it +arrives. The first line comes as soon as Basecamp answers, while it waits: + + {"type":"verification","verification_uri":"…","user_code":"WDJB-MJHT","expires_at":"…","expires_in":600} + +Relay `verification_uri`, `user_code` and when it expires to the person right +away. They are already stripped of control characters, so pass them on as they +are. Don't scrape the terminal text. It goes to stderr and is the CLI's copy +for a person at that terminal. The second value is the result envelope: `ok`, +with `data.profile`, `data.account_id` and `data.scope`, or the error and its +hint. + +`basecamp auth login` and `basecamp profile create` refuse `--json`. Run them +plainly, in the background, and read their output as it arrives, so you can +show the link and code while they wait. ## First-time setup @@ -163,9 +175,10 @@ Ask only what you can't find out. '' --json`. - `unknown profile`: it doesn't exist. Connect the agent with - `basecamp auth agent connect -P ''` (add `--no-browser` if the person - is on another device). Show the link and code. They open the link, check the - code, choose the agent (or create one) and approve. + `basecamp auth agent connect -P '' --json` (add `--no-browser` if + the person is on another device). Show them the link and code from its + verification line. They open the link, check the code, choose the agent (or + create one) and approve. - `oauth_type` `agent`: already connected. Don't connect again: that rotates the agent's secret and disconnects any other computer using it. - Anything else: a person's login is stored. Ask; never connect an agent over