diff --git a/.surface b/.surface index 65a006059..c3d974102 100644 --- a/.surface +++ b/.surface @@ -668,6 +668,8 @@ CMD basecamp config show CMD basecamp config trust CMD basecamp config unset CMD basecamp config untrust +CMD basecamp connect +CMD basecamp connect setup CMD basecamp docs CMD basecamp docs archive CMD basecamp docs doc @@ -5341,6 +5343,62 @@ FLAG basecamp config untrust --stats type=bool FLAG basecamp config untrust --styled type=bool FLAG basecamp config untrust --todolist type=string FLAG basecamp config untrust --verbose type=count +FLAG basecamp connect --account type=string +FLAG basecamp connect --agent type=bool +FLAG basecamp connect --cache-dir type=string +FLAG basecamp connect --count type=bool +FLAG basecamp connect --help type=bool +FLAG basecamp connect --hints type=bool +FLAG basecamp connect --ids-only type=bool +FLAG basecamp connect --in type=string +FLAG basecamp connect --jq type=string +FLAG basecamp connect --json type=bool +FLAG basecamp connect --markdown type=bool +FLAG basecamp connect --md type=bool +FLAG basecamp connect --no-hints type=bool +FLAG basecamp connect --no-stats type=bool +FLAG basecamp connect --profile type=string +FLAG basecamp connect --project type=string +FLAG basecamp connect --quiet type=bool +FLAG basecamp connect --stats type=bool +FLAG basecamp connect --styled type=bool +FLAG basecamp connect --todolist type=string +FLAG basecamp connect --verbose type=count +FLAG basecamp connect setup --account type=string +FLAG basecamp connect setup --agent type=bool +FLAG basecamp connect setup --allow type=stringArray +FLAG basecamp connect setup --cache-dir type=string +FLAG basecamp connect setup --class type=stringArray +FLAG basecamp connect setup --concurrency type=int +FLAG basecamp connect setup --count type=bool +FLAG basecamp connect setup --deadline type=duration +FLAG basecamp connect setup --driver type=string +FLAG basecamp connect setup --expect-identity type=string +FLAG basecamp connect setup --help type=bool +FLAG basecamp connect setup --hints type=bool +FLAG basecamp connect setup --ids-only type=bool +FLAG basecamp connect setup --in type=string +FLAG basecamp connect setup --jq type=string +FLAG basecamp connect setup --json type=bool +FLAG basecamp connect setup --markdown type=bool +FLAG basecamp connect setup --md type=bool +FLAG basecamp connect setup --no-hints type=bool +FLAG basecamp connect setup --no-stats type=bool +FLAG basecamp connect setup --no-watch-completions type=stringArray +FLAG basecamp connect setup --operator type=string +FLAG basecamp connect setup --operator-profile type=string +FLAG basecamp connect setup --profile type=string +FLAG basecamp connect setup --project type=string +FLAG basecamp connect setup --quiet type=bool +FLAG basecamp connect setup --remove-route type=stringArray +FLAG basecamp connect setup --route type=stringArray +FLAG basecamp connect setup --stats type=bool +FLAG basecamp connect setup --styled type=bool +FLAG basecamp connect setup --todolist type=string +FLAG basecamp connect setup --trust type=string +FLAG basecamp connect setup --verbose type=count +FLAG basecamp connect setup --watch-completions type=stringArray +FLAG basecamp connect setup --worktrees type=bool FLAG basecamp docs --account type=string FLAG basecamp docs --agent type=bool FLAG basecamp docs --cache-dir type=string @@ -18517,6 +18575,8 @@ SUB basecamp config show SUB basecamp config trust SUB basecamp config unset SUB basecamp config untrust +SUB basecamp connect +SUB basecamp connect setup SUB basecamp docs SUB basecamp docs archive SUB basecamp docs doc diff --git a/e2e/smoke/smoke_lifecycle.bats b/e2e/smoke/smoke_lifecycle.bats index 7e01734e9..4fee36ec9 100644 --- a/e2e/smoke/smoke_lifecycle.bats +++ b/e2e/smoke/smoke_lifecycle.bats @@ -16,6 +16,10 @@ load smoke_helper mark_out_of_scope "Waits on a person approving the connection in a browser" } +@test "connect setup is out of scope" { + mark_out_of_scope "Needs a connected agent profile and writes local connector policy — covered by Go tests in internal/commands and internal/connector/setup" +} + @test "auth refresh is out of scope" { mark_out_of_scope "Requires OAuth credentials" } diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 0a499a37e..dbdc44d05 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -143,11 +143,16 @@ func NewManager(cfg *config.Config, httpClient *http.Client) *Manager { } } +// ProfileCredentialKey is the credential key a named profile's credential +// is stored under, for a caller that must name one profile's credential +// without a manager bound to it. +func ProfileCredentialKey(profile string) string { return "profile:" + profile } + // credentialKey returns the storage key for credentials. // Profile mode: "profile:", No-profile mode: origin URL. func (m *Manager) credentialKey() string { if m.cfg.ActiveProfile != "" { - return "profile:" + m.cfg.ActiveProfile + return ProfileCredentialKey(m.cfg.ActiveProfile) } return config.NormalizeBaseURL(m.cfg.BaseURL) } diff --git a/internal/auth/lock.go b/internal/auth/lock.go index 9626de16a..4eed9dc5d 100644 --- a/internal/auth/lock.go +++ b/internal/auth/lock.go @@ -7,6 +7,7 @@ import ( "fmt" "os" "path/filepath" + "sync/atomic" "time" "github.com/gofrs/flock" @@ -128,6 +129,73 @@ func (s *Store) withKeyLock(ctx context.Context, key string, fn func() error) er return fn() } +// WithCredential runs fn with one key's credential while holding that key's +// cross-process lock, so no other process's login, refresh, import or +// logout for the key can land while fn runs. A caller that must act on a +// credential being what it just read — writing a file that names the +// identity it authenticates as, say — does its read, its decision and its +// write inside fn. +// +// A host that cannot lock at all is an error here, not a warning: the +// callers that reach for this are the ones whose guarantee is the lock. +// +// fn must not take the same key's lock again — no AccessToken, no +// StoredAccessToken, no SetUserIdentity, no nested WithCredential. Locks +// are per open file description, so a second acquire in this process waits +// on itself until the wait bound expires. fn must not make a network +// request either: every other process's work on the key queues behind it. +// +// A key with nothing stored is reported as ErrNoCredential, never as a nil +// credential. +func (s *Store) WithCredential(ctx context.Context, key string, fn func(HeldCredential) error) error { + release, err := s.lockFile(lockRequest{name: keyLockName(key), what: "credential", done: ctx.Done(), cause: ctx.Err, require: true}) + if err != nil { + return err + } + defer release() + creds, err := s.load(key, lockRequest{done: ctx.Done(), cause: ctx.Err}) + if err != nil { + return err + } + held := &heldCredential{creds: creds, key: key} + defer held.released.Store(true) + return fn(held) +} + +// HeldCredential is a credential read while its key's lock is held, and the +// proof a write can ask for: a function that must not run without the lock +// takes one. Only WithCredential makes one, it stops counting when the lock +// is released, and Held is the only way to read it — the interface carries +// nothing a wrapper could answer for. +type HeldCredential interface { + // heldUnderLock cannot be implemented outside this package. It can be + // promoted by embedding the interface, which is why nothing reads a + // proof through methods: Held is the only reader. + heldUnderLock() +} + +type heldCredential struct { + creds *Credentials + key string + released atomic.Bool +} + +func (h *heldCredential) valid() bool { return !h.released.Load() } + +func (h *heldCredential) heldUnderLock() {} + +// Held reads a lock proof: the credential and key it was made with, and +// whether the lock is still held. Anything this package did not make — a +// nil, a struct embedding the interface — is reported as not held, whatever +// its own methods say. +func Held(h HeldCredential) (creds *Credentials, key string, held bool) { + own, ok := h.(*heldCredential) + if !ok || own == nil || !own.valid() || own.creds == nil || own.key == "" { + return nil, "", false + } + return own.creds, own.key, true +} + // withStoreLock runs fn while holding the whole-store lock. fn must be one // store operation and must not make a network request: every process's // Save and Delete queue behind it. @@ -237,6 +305,11 @@ type lockRequest struct { wait time.Duration done <-chan struct{} cause func() error + // require refuses the unlocked fall-through: a caller whose guarantee + // IS the lock (it decides on a credential and writes a file naming what + // that credential authenticates as) gets an error where an ordinary + // caller would get a warning and an unsynchronized run. + require bool } // lockFile takes the lock named name — exclusive, or shared when the @@ -271,7 +344,15 @@ func (s *Store) lockFile(req lockRequest) (func(), error) { return s.unlocked(req, err.Error()) } - fl := flock.New(filepath.Join(s.lockDir(), req.name)) + lockPath := filepath.Join(s.lockDir(), req.name) + if req.require { + // A required lock is only a lock if every process takes the same + // inode: refuse a lock directory or file this user does not own. + if err := requirePrivateLockPath(s.lockDir(), lockPath); err != nil { + return s.unlocked(req, err.Error()) + } + } + fl := flock.New(lockPath) try := fl.TryLock if req.shared { try = fl.TryRLock @@ -352,6 +433,16 @@ func (s *Store) unlocked(req lockRequest, reason string) (func(), error) { if err := lockCause(req.cause); err != nil { return noop, err } + if req.require { + // The caller asked for the lock itself, not for best effort: there + // is nothing to fall through to. + return noop, &output.Error{ + Code: output.CodeLockUnavailable, + Message: fmt.Sprintf("This host cannot lock the %s: %s", req.what, richtext.SanitizeSingleLine(reason)), + Hint: "Nothing was changed. Other commands still work; connector setup is what needs the lock, so point XDG_CONFIG_HOME at a filesystem that supports locking " + + "(some network and FUSE mounts do not).", + } + } s.warnUnlockable(req.what, reason) return noop, nil } diff --git a/internal/auth/lock_test.go b/internal/auth/lock_test.go index 89bb94139..4c66f26f0 100644 --- a/internal/auth/lock_test.go +++ b/internal/auth/lock_test.go @@ -1097,3 +1097,138 @@ func TestStoreSaveKeepsOtherKeys(t *testing.T) { require.NoError(t, json.Unmarshal(raw, &all)) assert.Len(t, all, 8) } + +// WithCredential holds the key's lock while fn runs: a caller that decides +// on a credential and writes inside fn cannot have it replaced underneath. +func TestWithCredentialHoldsTheKeyLock(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + dir := t.TempDir() + store := NewStore(dir) + const key = "profile:agent" + require.NoError(t, store.Save(key, &Credentials{AccessToken: "first", OAuthType: "agent"})) + + inside := make(chan struct{}) + replaced := make(chan error, 1) + err := store.WithCredential(context.Background(), key, func(held HeldCredential) error { + creds, heldKey, ok := Held(held) + require.True(t, ok) + assert.Equal(t, "first", creds.AccessToken) + assert.Equal(t, key, heldKey) + go func() { + close(inside) + replaced <- NewStore(dir).withKeyLock(context.Background(), key, func() error { + return NewStore(dir).Save(key, &Credentials{AccessToken: "second", OAuthType: "agent"}) + }) + }() + <-inside + time.Sleep(200 * time.Millisecond) // let the other writer reach the lock + // The other writer is waiting on the key lock, so what was read is + // still what is stored. + again, err := store.Load(key) + require.NoError(t, err) + assert.Equal(t, "first", again.AccessToken) + return nil + }) + require.NoError(t, err) + require.NoError(t, <-replaced) + + after, err := store.Load(key) + require.NoError(t, err) + assert.Equal(t, "second", after.AccessToken, "the waiting writer lands once the section ends") +} + +// A host that cannot lock at all is an error for WithCredential, never a +// warning and an unsynchronized run: its callers' guarantee is the lock. +func TestWithCredentialRefusesWhenTheHostCannotLock(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + dir := t.TempDir() + store := NewStore(dir) + const key = "profile:agent" + require.NoError(t, store.Save(key, &Credentials{AccessToken: "first", OAuthType: "agent"})) + // No lock file can be created where nothing may be written. + locks := store.lockDir() + require.NoError(t, os.MkdirAll(locks, 0o700)) + require.NoError(t, os.Chmod(locks, 0o500)) + t.Cleanup(func() { _ = os.Chmod(locks, 0o700) }) + + ran := false + err := store.WithCredential(context.Background(), key, func(HeldCredential) error { + ran = true + return nil + }) + require.Error(t, err) + assert.False(t, ran, "the section never runs unlocked") + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, output.CodeLockUnavailable, apiErr.Code) +} + +// The proof stops being valid when the lock is released, so a callback that +// keeps it cannot write with it afterwards. +func TestHeldCredentialIsInvalidAfterTheLockIsReleased(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + store := NewStore(t.TempDir()) + const key = "profile:agent" + require.NoError(t, store.Save(key, &Credentials{AccessToken: "first", OAuthType: "agent"})) + + var kept HeldCredential + require.NoError(t, store.WithCredential(context.Background(), key, func(held HeldCredential) error { + kept = held + creds, heldKey, ok := Held(held) + require.True(t, ok) + require.NotNil(t, creds) + assert.Equal(t, key, heldKey) + return nil + })) + + creds, heldKey, ok := Held(kept) + assert.False(t, ok, "the lock is gone") + assert.Nil(t, creds) + assert.Empty(t, heldKey) +} + +// A required lock is only a lock if every process takes the same inode, so +// a lock directory or lock file this user does not own is refused rather +// than used. +func TestWithCredentialRefusesAnUntrustworthyLockPath(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + const key = "profile:agent" + + for name, plant := range map[string]func(t *testing.T, dir string){ + "the lock directory is a symlink": func(t *testing.T, dir string) { + locks := filepath.Join(dir, "locks") + require.NoError(t, os.RemoveAll(locks)) + require.NoError(t, os.Symlink(t.TempDir(), locks)) + }, + "the lock directory is writable by others": func(t *testing.T, dir string) { + locks := filepath.Join(dir, "locks") + require.NoError(t, os.MkdirAll(locks, 0o700)) + require.NoError(t, os.Chmod(locks, 0o777)) + }, + "the lock file is a symlink": func(t *testing.T, dir string) { + locks := filepath.Join(dir, "locks") + require.NoError(t, os.MkdirAll(locks, 0o700)) + planted := filepath.Join(locks, keyLockName(key)) + require.NoError(t, os.RemoveAll(planted)) + require.NoError(t, os.Symlink(filepath.Join(t.TempDir(), "planted"), planted)) + }, + } { + t.Run(name, func(t *testing.T) { + dir := t.TempDir() + store := NewStore(dir) + require.NoError(t, store.Save(key, &Credentials{AccessToken: "first", OAuthType: "agent"})) + plant(t, dir) + + ran := false + err := store.WithCredential(context.Background(), key, func(HeldCredential) error { + ran = true + return nil + }) + require.Error(t, err) + assert.False(t, ran, "the section never runs on a lock it cannot trust") + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, output.CodeLockUnavailable, apiErr.Code) + }) + } +} diff --git a/internal/auth/lockpriv_other.go b/internal/auth/lockpriv_other.go new file mode 100644 index 000000000..25ef77542 --- /dev/null +++ b/internal/auth/lockpriv_other.go @@ -0,0 +1,8 @@ +//go:build !unix + +package auth + +// Without Unix ownership and modes there is nothing to check here: a +// required lock rests on the platform's own access control for the user's +// configuration directory. +func requirePrivateLockPath(string, string) error { return nil } diff --git a/internal/auth/lockpriv_unix.go b/internal/auth/lockpriv_unix.go new file mode 100644 index 000000000..b5b5bdb0a --- /dev/null +++ b/internal/auth/lockpriv_unix.go @@ -0,0 +1,55 @@ +//go:build unix + +package auth + +import ( + "errors" + "fmt" + "os" + "syscall" +) + +// requirePrivateLockPath refuses a lock directory or lock file that is not +// this user's own: a symlink (which could have been planted while the +// config directory was permissive and now point somewhere else), a file +// another user owns, or one another user can write. A required lock is only +// a lock if every process that takes it takes the same inode, and that is +// what these checks are for. +func requirePrivateLockPath(dir, file string) error { + if err := requirePrivate(dir, true); err != nil { + return err + } + switch err := requirePrivate(file, false); { + case errors.Is(err, os.ErrNotExist): + // The lock file is created, owner-only, by the acquisition itself. + return nil + default: + return err + } +} + +func requirePrivate(path string, wantDir bool) error { + info, err := os.Lstat(path) + if err != nil { + return err + } + switch { + case info.Mode()&os.ModeSymlink != 0: + return fmt.Errorf("%s is a symlink", path) + case wantDir && !info.IsDir(): + return fmt.Errorf("%s is not a directory", path) + case !wantDir && !info.Mode().IsRegular(): + return fmt.Errorf("%s is not a regular file", path) + } + st, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return fmt.Errorf("the owner of %s cannot be read", path) + } + if int(st.Uid) != os.Geteuid() { + return fmt.Errorf("%s belongs to uid %d", path, st.Uid) + } + if perm := info.Mode().Perm(); perm&0o022 != 0 { + return fmt.Errorf("%s is writable by other users (mode %04o)", path, perm) + } + return nil +} diff --git a/internal/cli/root.go b/internal/cli/root.go index 3cfd19869..a68c83b4a 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -375,6 +375,7 @@ func Execute() { cmd.AddCommand(commands.NewBonfireCmd()) cmd.AddCommand(commands.NewAgentHookCmd()) cmd.AddCommand(commands.NewMCPCmd()) + cmd.AddCommand(commands.NewConnectCmd()) // Tier-2 stdin guard: reject a stray literal "-" when stdin is piped, // everywhere a command doesn't explicitly accept it — except cobra's diff --git a/internal/commands/commands.go b/internal/commands/commands.go index 5fda07a5a..c76a9a23f 100644 --- a/internal/commands/commands.go +++ b/internal/commands/commands.go @@ -146,6 +146,7 @@ func CommandCategories() []CommandCategory { {Name: "bonfire", Category: "additional", Description: "Multi-chat orchestration", Actions: []string{"split", "layout"}, Experimental: true, DevOnly: true}, {Name: "api", Category: "additional", Description: "Raw API access"}, {Name: "mcp", Category: "additional", Description: "Serve Basecamp to MCP clients over stdio"}, + {Name: "connect", Category: "additional", Description: "Run a local agent connector for a Basecamp agent", Actions: []string{"setup"}}, {Name: "help", Category: "additional", Description: "Show help"}, {Name: "version", Category: "additional", Description: "Show version"}, }, diff --git a/internal/commands/commands_test.go b/internal/commands/commands_test.go index 1bea37acf..72e6070ab 100644 --- a/internal/commands/commands_test.go +++ b/internal/commands/commands_test.go @@ -122,6 +122,7 @@ func buildRootWithAllCommands() *cobra.Command { root.AddCommand(commands.NewProfileCmd()) root.AddCommand(commands.NewBonfireCmd()) root.AddCommand(commands.NewMCPCmd()) + root.AddCommand(commands.NewConnectCmd()) commands.InstallDashGuard(root) root.InitDefaultHelpCmd() return root diff --git a/internal/commands/connect.go b/internal/commands/connect.go new file mode 100644 index 000000000..bf33857ad --- /dev/null +++ b/internal/commands/connect.go @@ -0,0 +1,860 @@ +package commands + +import ( + "context" + "errors" + "fmt" + "net/http" + "os" + "strconv" + "strings" + "time" + + "github.com/spf13/cobra" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + + "github.com/basecamp/basecamp-cli/internal/appctx" + "github.com/basecamp/basecamp-cli/internal/auth" + "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/connector/admission" + "github.com/basecamp/basecamp-cli/internal/connector/setup" + "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/richtext" + "github.com/basecamp/basecamp-cli/internal/version" +) + +// NewConnectCmd is the local agent connector's command group. +func NewConnectCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "connect", + Short: "Set up a local agent connector for a Basecamp agent", + Long: `Run a local agent connector: it listens to the account event feed as a +Basecamp agent, admits what a trusted person asks of that agent, and hands +the work to a local coding agent that replies in Basecamp as the agent. + +Connect the agent to a profile first (basecamp auth agent connect -P ), +then run setup on that profile: it records who may drive the agent, maps +projects to the directories their work runs in, and checks the connector is +ready.`, + } + cmd.AddCommand(newConnectSetupCmd()) + return cmd +} + +// connectSetupFlags are setup's flags, as typed. +type connectSetupFlags struct { + expectIdentity string + + operator string + operatorProfile string + + trust string + allow []string + + routes []string + classes []string + watch []string + unwatch []string + unroute []string + driver string + parallel int + deadline time.Duration + worktrees bool +} + +func newConnectSetupCmd() *cobra.Command { + var f connectSetupFlags + + cmd := &cobra.Command{ + Use: "setup", + Short: "Choose who may drive a connected agent, route projects, and check readiness", + Long: `Set up the connector for the agent a profile holds: record the trusted +operator, write connect.json, and check what can be checked before the +connector runs. + +Credential. Setup does not obtain one; connect the profile first, then run +setup against it: + basecamp auth agent connect -P agent # an Agent person + basecamp auth login -P bot --expect-identity # a bot user (v1) +On the bot-user path pass --expect-identity to setup as well, so it can prove +the login is the bot and not you; later runs remember it. + +Operator. The person whose instructions the agent follows, keyed on Person +id. Name them by their own profile (--operator-profile, which proves who +they are), or by id (--operator), which the agent must be able to read — +Basecamp refuses that read to an Agent identity today, so on the agent +connection path use --operator-profile. With neither, setup keeps the operator +connect.json already has; on a first setup one of the two is required. + +Trust. operator (default): the operator alone. allowlist: the operator and +the people passed with --allow. project: the operator and any non-client +member of the event's project. Assignments are the operator's alone in every +mode. + +Routes. connect.json is the only authority for which directory a project's +work runs in: --route =. A project with no route gets a +holding reply and no work. --watch-completions makes the agent +hear every trusted completion in that project without being assigned. + +connect.json is written owner-only and refused when anyone else could have +changed it or a directory above it. Where this CLI cannot verify that +(Windows), or where the filesystem holding the configuration directory +cannot lock, setup refuses rather than write a trust file it cannot vouch +for. + +Every check runs before connect.json is written, and it is written only +when all of them pass. Exit status: usage for refused input, auth when the +profile's credential is missing, unreadable, cannot be proven, or is not the +agent it should be, not_ready when a readiness check failed. Setup never +stores, replaces or removes a credential; a token due for renewal is renewed +as by any command. + +Run setup again to change any of it; what you do not pass is kept. + +Examples: + basecamp auth agent connect -P agent + basecamp connect setup -P agent --operator-profile me --route 12345=~/Work/app + basecamp connect setup -P agent --operator-profile me --trust allowlist --allow 111 --allow 222 + basecamp connect setup -P bot --operator-profile me --expect-identity 4242 --route 12345=~/Work/app + basecamp connect setup -P agent --class 12345=internal --deadline 90m --worktrees`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + app := appctx.FromContext(cmd.Context()) + if app == nil { + return fmt.Errorf("app not initialized") + } + return runConnectSetup(cmd, app, &f) + }, + } + + fl := cmd.Flags() + fl.StringVar(&f.expectIdentity, "expect-identity", "", "Bot-user path: the identity id the profile's login must authenticate as") + fl.StringVar(&f.operator, "operator", "", "Person id of the operator the agent follows") + fl.StringVar(&f.operatorProfile, "operator-profile", "", "Profile whose identity is the operator") + fl.StringVar(&f.trust, "trust", "", "Who may drive the agent: operator, allowlist or project") + fl.StringArrayVar(&f.allow, "allow", nil, "Person id to trust besides the operator (repeatable; implies --trust allowlist)") + fl.StringArrayVar(&f.routes, "route", nil, "Route a project to a directory: = (repeatable)") + fl.StringArrayVar(&f.unroute, "remove-route", nil, "Remove a project's route (repeatable)") + fl.StringArrayVar(&f.classes, "class", nil, "Classify a routed project: =, or = to clear it (repeatable)") + fl.StringArrayVar(&f.watch, "watch-completions", nil, "Admit every trusted completion in a routed project (repeatable)") + fl.StringArrayVar(&f.unwatch, "no-watch-completions", nil, "Stop watching a project's completions (repeatable)") + fl.StringVar(&f.driver, "driver", "", "How workers are run: spawn or acp (default spawn)") + fl.IntVar(&f.parallel, "concurrency", 0, fmt.Sprintf("Workers at once (default %d)", setup.DefaultConcurrency)) + fl.DurationVar(&f.deadline, "deadline", 0, fmt.Sprintf("Deadline per task (default %s)", setup.DefaultDeadline)) + fl.BoolVar(&f.worktrees, "worktrees", false, "Give each task its own git worktree") + cmd.MarkFlagsMutuallyExclusive("operator", "operator-profile") + + return cmd +} + +func runConnectSetup(cmd *cobra.Command, app *appctx.App, f *connectSetupFlags) error { + ctx := cmd.Context() + + name := app.Config.ActiveProfile + if name == "" { + return output.ErrUsageHint("Setup needs the agent's profile", "Pass -P/--profile , a profile connected with `basecamp auth agent connect`.") + } + if !isValidProfileName(name) { + return output.ErrUsage(fmt.Sprintf("Invalid profile name %q: use only letters, numbers, hyphens, and underscores", name)) + } + if os.Getenv("BASECAMP_TOKEN") != "" { + return errEnvTokenShadows("connect setup cannot check the agent while BASECAMP_TOKEN is set") + } + + changes, err := f.changes(cmd) + if err != nil { + return err + } + worktreesChanged(cmd, f, &changes) + expect, err := parseExpectIdentity(f.expectIdentity) + if err != nil { + return err + } + operatorID, err := parsePositiveID("--operator", f.operator) + if err != nil { + return err + } + if f.operatorProfile == name { + return output.ErrUsage("--operator-profile names the agent's own profile; the operator is a different person") + } + + path, err := setup.Path(config.GlobalConfigDir(), name) + if err != nil { + return output.ErrUsage(err.Error()) + } + // One setup per profile at a time: load, change and save are one step. + unlock, err := setup.Lock(path) + if err != nil { + return classifyLockError(name, err) + } + defer unlock() + + existing, err := setup.Load(path) + exists := err == nil + switch { + case errors.Is(err, os.ErrNotExist): + existing = setup.New(name) + case err != nil: + return output.ErrUsageHint("connect.json cannot be used: "+err.Error(), + "Nothing was changed. Fix or remove "+richtext.SanitizeSingleLine(path)+", then run setup again.") + } + if existing.Profile != name { + return output.ErrUsage(fmt.Sprintf("%s names profile %q, not %q", path, existing.Profile, name)) + } + + // Everything refusable without the network is refused first. + next, err := setup.Apply(existing, changes) + if err != nil { + return output.ErrUsage(err.Error()) + } + if operatorID == 0 && f.operatorProfile == "" && existing.Trust.OperatorID == 0 { + return output.ErrUsageHint("Setup needs to know who the operator is", + "Pass --operator-profile (or --operator ). The operator is the person the agent takes instructions from, and is never guessed.") + } + var operatorMgr *operatorProfile + if f.operatorProfile != "" { + if operatorMgr, err = operatorProfileManager(ctx, app, f.operatorProfile); err != nil { + return err + } + } + accountID, err := connectAccount(app, name) + if err != nil { + return err + } + if exists && !accountIDsEqual(existing.AccountID, accountID) { + return output.ErrUsageHint( + fmt.Sprintf("connect.json was set up in account %s, and profile %q is bound to account %s", existing.AccountID, name, accountID), + "Nothing was changed. Remove "+richtext.SanitizeSingleLine(path)+" to set this profile up afresh.") + } + kind, err := connectCredentialKind(ctx, app) + if err != nil { + return err + } + // The credential's own disagreement with connect.json comes first: it is + // the wrong credential whatever the flags say. + if exists && kind != "" && existing.Agent.Kind != kind { + return output.ErrAuth(fmt.Sprintf("connect.json was set up for a %s credential, and profile %q now holds a %s one; remove %s to set this profile up afresh", + existing.Agent.Kind, name, kind, richtext.SanitizeSingleLine(path))) + } + if err := refuseCredentialConflicts(name, path, kind, expect, exists, existing); err != nil { + return err + } + if kind == setup.KindBotUser { + switch { + case expect == 0: + expect = existing.Agent.IdentityID + case exists && existing.Agent.IdentityID != 0 && expect != existing.Agent.IdentityID: + // Rebinding the file to another bot is a deliberate act, not a + // flag: connect.json's trust was recorded for the agent it names. + return output.ErrAuth(fmt.Sprintf( + "connect.json is set up for identity %d, and --expect-identity names %d; remove %s to set this profile up for another bot", + existing.Agent.IdentityID, expect, richtext.SanitizeSingleLine(path))) + } + } + + var checks []setup.Check + + // Token: one snapshot of the credential and the token it yields, which + // every read below spends. A credential another process stores under + // the profile while setup runs is not what setup checked, so it is + // compared again before anything is written. + token, err := app.Auth.AccessToken(ctx) + if err != nil { + return output.ErrAuth(fmt.Sprintf("Profile %q holds a credential that does not produce a token: %s", name, setup.ErrorText(err))) + } + creds, err := app.Auth.GetStore().LoadContext(ctx, app.Auth.CredentialKey()) + if err != nil { + return output.ErrAuth("The stored credential could not be read: " + setup.ErrorText(err)) + } + if creds.AccessToken != token { + return errCredentialChanged(name) + } + provider := &basecamp.StaticTokenProvider{Token: token} + checks = append(checks, setup.Check{Name: "Token", Status: setup.StatusPass, Message: "The profile's credential yields a token"}) + + // Identity. + client := connectSDKClient(app, provider) + reader := setup.SDKReader{Client: client.ForAccount(accountID)} + me, err := reader.Me(ctx) + if err != nil { + return output.ErrAuth(fmt.Sprintf("Could not read who profile %q is in account %s: %s", name, accountID, setup.ErrorText(err))) + } + identityCheck, err := checkConnectIdentity(ctx, app, client, kind, creds.OAuthType, me, expect) + if err != nil { + return err + } + checks = append(checks, identityCheck) + if exists && existing.Agent.PersonID != me.ID { + return output.ErrAuth(fmt.Sprintf("connect.json was set up for agent person %d, and profile %q now authenticates as person %d; if this is a different agent on purpose, remove %s and run setup again", + existing.Agent.PersonID, name, me.ID, richtext.SanitizeSingleLine(path))) + } + checks = append(checks, setup.ScopeCheck(creds.OAuthType, creds.Scope)) + + // Trust, verified before anything is written: connect.json is the trust + // anchor, and nobody in it is recorded unverified. People are read + // through the operator's own credential when there is one, since + // Basecamp refuses person reads to an Agent identity today. + trust := setup.Trust{ + Allowlist: next.Trust.AllowlistIDs, + OperatorProfile: f.operatorProfile, + } + if exists { + trust.Recorded = existing.Trust + } + people := setup.Reader(reader) + if operatorMgr != nil { + op, opReader, err := resolveOperatorProfile(ctx, operatorMgr, f.operatorProfile, accountID) + if err != nil { + return err + } + trust.Operator = op + people = opReader + } else { + if operatorID == 0 { + operatorID = existing.Trust.OperatorID + } + trust.Operator = setup.Person{ID: operatorID} + } + trustChecks := setup.VerifyTrust(ctx, people, trust, me.ID) + for _, c := range trustChecks { + if c.Status == setup.StatusFail { + return output.ErrUsageHint("Trust was refused, and nothing was written: "+c.Name+": "+c.Message, c.Hint) + } + } + checks = append(checks, trustChecks...) + + next.AccountID = accountID + next.Agent = setup.Agent{PersonID: me.ID, Kind: kind} + if kind == setup.KindBotUser { + next.Agent.IdentityID = expect + } + next.Trust.OperatorID = trust.Operator.ID + if err := next.Validate(); err != nil { + return output.ErrUsage("connect.json was not written: " + err.Error()) + } + + // Every check runs before connect.json is written, and it is written + // only when all of them pass: a setup that is not ready changes nothing. + report := &setup.Report{ + Path: path, + Profile: name, + AccountID: accountID, + AgentPersonID: me.ID, + AgentKind: kind, + OperatorID: next.Trust.OperatorID, + TrustMode: string(next.Trust.Mode), + Routes: len(next.Projects), + } + report.Add(checks...) + report.Add(setup.TicketCheck(ctx, reader, kind)) + report.Add(setup.RouteChecks(ctx, reader, next)...) + // A command the person stopped did not find the connector unready: it + // found nothing, and says so as an interruption. + if err := ctx.Err(); err != nil { + return err + } + + w := cmd.OutOrStdout() + styled := app.Output.EffectiveFormat() == output.FormatStyled + title := "Connector setup for profile " + strconv.Quote(name) + if len(report.Failed()) > 0 { + if styled { + renderChecksStyled(w, title, summarizeChecks(asDoctorChecks(report.Checks()))) + } + return errConnectorNotReady(report) + } + + // The write and the last look at the credential are one step under the + // credential key's own lock, which every login, refresh and import + // takes: no other command can replace the profile's credential between + // the check and the write. What is written names the identity that + // credential authenticates as, and connect.json's VerifyAgent is what + // the connector re-checks at start-up, so a credential replaced later + // stops it rather than making it act as the wrong agent. + saveErr := app.Auth.GetStore().WithCredential(ctx, app.Auth.CredentialKey(), func(held auth.HeldCredential) error { + stored, _, ok := auth.Held(held) + if !ok || !sameCredential(creds, stored) { + return errCredentialChanged(name) + } + if err := next.VerifyAgent(kind, me.ID, expect); err != nil { + return output.ErrAuth(err.Error()) + } + return setup.Save(held, path, next) + }) + if saveErr != nil { + return classifyWriteError(name, saveErr) + } + report.Written = true + + summary := summarizeChecks(asDoctorChecks(report.Checks())) + if !report.Ready() { + return errConnectorNotReady(report) + } + if styled { + renderChecksStyled(w, title, summary) + fmt.Fprintf(w, " connect.json written: %s\n\n", richtext.SanitizeSingleLine(path)) + return nil + } + return app.OK(report, output.WithSummary("connect.json written; "+summary.Summary())) +} + +// classifyWriteError puts the last step's failures in the command's exit +// contract: a credential that is gone or unreadable is auth, another +// process holding the profile's credential or setup lock is busy, a +// connect.json nobody else may change is usage. +func classifyWriteError(name string, err error) error { + var apiErr *output.Error + profile := shellQuote(name) + switch { + case errors.Is(err, context.Canceled), errors.Is(err, context.DeadlineExceeded): + // A person who stopped the command, or a deadline: the run ended, + // it was not misused, and nothing was written. + return err + case errors.Is(err, setup.ErrSetupRunning): + return errBusy(name, err) + case errors.As(err, &apiErr) && apiErr.Code == output.CodeRateLimit: + // The credential store reports contention on its own key as a + // retryable rate limit; here that is the profile being busy. + return errBusy(name, err) + case errors.As(err, &apiErr): + // Already in the contract: the credential store reports contention + // as a retryable rate_limit, and setup's own refusals are typed. + return err + case errors.Is(err, auth.ErrNoCredential): + return &output.Error{Code: output.CodeAuth, + Message: fmt.Sprintf("Profile %q's credential was removed while setup was checking it, so nothing was written", name), + Hint: "Connect the agent again: basecamp auth agent connect -P " + profile} + case errors.Is(err, auth.ErrInvalidCredentials): + return &output.Error{Code: output.CodeAuth, + Message: fmt.Sprintf("Profile %q's stored credential could not be read, so nothing was written", name), + Hint: "Connect the agent again: basecamp auth agent connect -P " + profile} + case errors.Is(err, setup.ErrNotPrivate): + return output.ErrUsageHint("connect.json was not written: "+err.Error(), "Setup writes connect.json only where nobody else can change it.") + } + return output.ErrUsage("connect.json was not written: " + err.Error()) +} + +// classifyLockError puts a refused lock in the command's exit contract: +// another setup on the profile is busy, a host that cannot lock at all is +// lock_unavailable, and a connect.json nobody else may change stays usage. +func classifyLockError(name string, err error) error { + switch { + case errors.Is(err, setup.ErrSetupRunning): + return errBusy(name, err) + case errors.Is(err, setup.ErrLockUnavailable): + return &output.Error{Code: output.CodeLockUnavailable, + Message: fmt.Sprintf("This host cannot lock profile %q's connector policy: %s", name, setup.ErrorText(err)), + Hint: "Nothing was changed. Other commands still work; connector setup is what needs the lock, so point XDG_CONFIG_HOME at a filesystem that supports locking " + + "(some network and FUSE mounts do not)."} + case errors.Is(err, setup.ErrNotPrivate): + return output.ErrUsageHint(err.Error(), "Setup locks and writes connect.json only where nobody else can change it.") + } + return output.ErrUsage(err.Error()) +} + +func errBusy(name string, err error) error { + return &output.Error{Code: output.CodeBusy, Retryable: true, + Message: fmt.Sprintf("Another command is working on profile %q right now, so nothing was changed: %s", name, setup.ErrorText(err)), + Hint: "Nothing is wrong with the profile. Run setup again when it has finished."} +} + +// sameCredential reports whether two loads of a profile's credential are the +// same credential with the same token: what setup checked is what is stored. +func sameCredential(a, b *auth.Credentials) bool { + return a != nil && b != nil && + a.OAuthType == b.OAuthType && a.ClientID == b.ClientID && a.Scope == b.Scope && + a.AccessToken == b.AccessToken && a.RefreshToken == b.RefreshToken +} + +func errCredentialChanged(name string) error { + return &output.Error{Code: output.CodeAuth, + Message: fmt.Sprintf("Profile %q's credential changed while setup was checking it, so nothing was written", name), + Hint: "Another command stored or renewed a credential under the profile. Run setup again to check the one it holds now."} +} + +// codeNotReady is the error code for a setup whose checks failed: the +// connector would not run, and connect.json was not written. +const codeNotReady = "not_ready" + +// errConnectorNotReady reports a report that is not ready as the command's +// error, so a script or an agent reading the exit status sees what a person +// sees: every check that failed, by name. +func errConnectorNotReady(report *setup.Report) error { + failures := report.Failed() + failed := make([]string, 0, len(failures)) + hint := "" + for _, c := range failures { + failed = append(failed, c.Name+": "+c.Message) + if hint == "" { + hint = c.Hint + } + } + if hint == "" { + hint = "Fix what failed and run setup again." + } + return &output.Error{ + Code: codeNotReady, + Message: fmt.Sprintf("The connector is not ready, so %s was not written. %s", richtext.SanitizeSingleLine(report.Path), strings.Join(failed, "; ")), + Hint: hint, + } +} + +// canonicalAccount is the account id as a number spells it, the spelling +// the connector's instance lock uses. +func canonicalAccount(raw string) (string, error) { + if err := requireNumericAccount(raw); err != nil { + return "", err + } + n, err := strconv.ParseUint(raw, 10, 64) + if err != nil || n == 0 { + return "", output.ErrUsage(fmt.Sprintf("Invalid account ID %q", raw)) + } + return strconv.FormatUint(n, 10), nil +} + +// changes turns setup's flags into the changes connect.json takes. +func (f *connectSetupFlags) changes(cmd *cobra.Command) (setup.Changes, error) { + var ch setup.Changes + if f.trust != "" { + ch.Trust = admission.TrustMode(f.trust) + switch ch.Trust { + case admission.TrustOperator, admission.TrustAllowlist, admission.TrustProject: + default: + return ch, output.ErrUsage(fmt.Sprintf("Invalid --trust %q: use operator, allowlist or project", f.trust)) + } + } + for _, raw := range f.allow { + for part := range strings.SplitSeq(raw, ",") { + id, err := parsePositiveID("--allow", strings.TrimSpace(part)) + if err != nil || id == 0 { + return ch, output.ErrUsage(fmt.Sprintf("Invalid --allow %q: expected a Person id", raw)) + } + ch.Allow = append(ch.Allow, id) + } + } + + var err error + if ch.Routes, err = parseProjectPairs("--route", f.routes); err != nil { + return ch, err + } + if ch.Classes, err = parseProjectPairsAllowEmpty("--class", f.classes); err != nil { + return ch, err + } + for _, list := range []struct { + flag string + raw []string + on bool + }{{"--watch-completions", f.watch, true}, {"--no-watch-completions", f.unwatch, false}} { + for _, raw := range list.raw { + id, err := parsePositiveID(list.flag, raw) + if err != nil || id == 0 { + return ch, output.ErrUsage(fmt.Sprintf("Invalid %s %q: expected a project id", list.flag, raw)) + } + if ch.WatchCompletions == nil { + ch.WatchCompletions = map[int64]bool{} + } + if prev, seen := ch.WatchCompletions[id]; seen && prev != list.on { + return ch, output.ErrUsage(fmt.Sprintf("Project %d is given both --watch-completions and --no-watch-completions", id)) + } + ch.WatchCompletions[id] = list.on + } + } + for _, raw := range f.unroute { + id, err := parsePositiveID("--remove-route", raw) + if err != nil || id == 0 { + return ch, output.ErrUsage(fmt.Sprintf("Invalid --remove-route %q: expected a project id", raw)) + } + ch.Remove = append(ch.Remove, id) + } + + switch f.driver { + case "", setup.DriverSpawn, setup.DriverACP: + ch.Driver = f.driver + default: + return ch, output.ErrUsage(fmt.Sprintf("Invalid --driver %q: use spawn or acp", f.driver)) + } + // A typed zero is out of range, not a request for the default: the flags + // are read as typed, not as their zero values. + if cmd.Flags().Changed("concurrency") { + if f.parallel < 1 || f.parallel > setup.MaxConcurrency { + return ch, output.ErrUsage(fmt.Sprintf("Invalid --concurrency %d: use 1 to %d", f.parallel, setup.MaxConcurrency)) + } + ch.Concurrency = f.parallel + } + if cmd.Flags().Changed("deadline") { + if f.deadline < setup.MinDeadline || f.deadline > setup.MaxDeadline { + return ch, output.ErrUsage(fmt.Sprintf("Invalid --deadline %s: use %s to %s", f.deadline, setup.MinDeadline, setup.MaxDeadline)) + } + ch.Deadline = f.deadline + } + return ch, nil +} + +// worktreesChanged wires --worktrees, which is only a change when typed. +func worktreesChanged(cmd *cobra.Command, f *connectSetupFlags, ch *setup.Changes) { + if cmd.Flags().Changed("worktrees") { + v := f.worktrees + ch.Worktrees = &v + } +} + +// parseProjectPairs parses repeatable = flags. +func parseProjectPairs(flag string, raw []string) (map[int64]string, error) { + return parseProjectPairsWith(flag, raw, false) +} + +// parseProjectPairsAllowEmpty is parseProjectPairs where = (an +// empty value) is meaningful: it clears the setting. +func parseProjectPairsAllowEmpty(flag string, raw []string) (map[int64]string, error) { + return parseProjectPairsWith(flag, raw, true) +} + +func parseProjectPairsWith(flag string, raw []string, allowEmpty bool) (map[int64]string, error) { + if len(raw) == 0 { + return nil, nil + } + out := make(map[int64]string, len(raw)) + for _, pair := range raw { + idText, value, ok := strings.Cut(pair, "=") + id, err := parsePositiveID(flag, strings.TrimSpace(idText)) + if !ok || err != nil || id == 0 || (value == "" && !allowEmpty) { + return nil, output.ErrUsage(fmt.Sprintf("Invalid %s %q: expected =", flag, pair)) + } + if _, dup := out[id]; dup { + return nil, output.ErrUsage(fmt.Sprintf("%s names project %d twice", flag, id)) + } + out[id] = value + } + return out, nil +} + +// parsePositiveID parses a numeric id flag: 0 when absent. +func parsePositiveID(flag, raw string) (int64, error) { + if raw == "" { + return 0, nil + } + id, err := strconv.ParseInt(raw, 10, 64) + if err != nil || id <= 0 { + return 0, output.ErrUsage(fmt.Sprintf("Invalid %s %q: expected a numeric id", flag, raw)) + } + return id, nil +} + +// refuseCredentialConflicts refuses a profile setup cannot use as it is: +// one with no credential (setup does not obtain one), --expect-identity +// against an Agent's credential or connect.json, and a person's login with +// nothing pinning it to the bot. +func refuseCredentialConflicts(name, path, held string, expect int64, exists bool, existing setup.File) error { + profile := shellQuote(name) + switch { + // What connect.json already says comes first: a remediation that the + // existing file would refuse anyway is no remediation. + case expect != 0 && exists && existing.Agent.Kind == setup.KindAgent: + return output.ErrUsageHint("--expect-identity is for the bot-user path, and connect.json was set up for an Agent", + fmt.Sprintf("Connect the agent again (basecamp auth agent connect -P %s) and drop --expect-identity, or remove %s to set this profile up as a bot user.", + profile, richtext.SanitizeSingleLine(path))) + case held == "" && (expect != 0 || (exists && existing.Agent.Kind == setup.KindBotUser)): + identity := expect + if identity == 0 { + identity = existing.Agent.IdentityID + } + return &output.Error{Code: output.CodeAuth, + Message: fmt.Sprintf("Profile %q holds no credential", name), + Hint: fmt.Sprintf("Log the bot in first: basecamp auth login -P %s --expect-identity %d, then run setup again.", profile, identity)} + case held == "": + return &output.Error{Code: output.CodeAuth, + Message: fmt.Sprintf("Profile %q holds no credential", name), + Hint: fmt.Sprintf("Connect the agent first: basecamp auth agent connect -P %s, then run setup again.", profile)} + case expect != 0 && held == setup.KindAgent: + return output.ErrUsageHint("--expect-identity is for the bot-user path, and profile "+strconv.Quote(name)+" holds an Agent's credential, which has no identity", + fmt.Sprintf("Drop --expect-identity, or log a bot in under another profile: basecamp auth login -P --expect-identity %d.", expect)) + case held == setup.KindBotUser && expect == 0 && existing.Agent.IdentityID == 0: + return output.ErrUsageHint(fmt.Sprintf("Profile %q holds a person's login, not an Agent's credential", name), + "On the bot-user path pass --expect-identity , so setup can prove this login is the bot and not you.") + } + return nil +} + +// connectAccount is the account setup works in: the one the agent's +// profile is bound to, which is where its credential was connected. It is +// never a config-wide default, and an --account or BASECAMP_ACCOUNT_ID that +// names another account is refused rather than preferred. +func connectAccount(app *appctx.App, name string) (string, error) { + p := app.Config.Profiles[name] + if p == nil { + return "", output.ErrUsageHint(fmt.Sprintf("Profile %q does not exist", name), + "Connect the agent first: basecamp auth agent connect -P "+shellQuote(name)) + } + bound, err := canonicalAccount(p.AccountID) + if err != nil { + return "", output.ErrUsageHint(fmt.Sprintf("Profile %q is not bound to an account", name), + "Bind it: basecamp profile set "+shellQuote(name)+" account_id ") + } + if accountGivenExplicitly(app) && !accountIDsEqual(app.Config.AccountID, bound) { + return "", output.ErrUsageHint( + fmt.Sprintf("Profile %q is bound to account %s, and this command named account %s", name, bound, app.Config.AccountID), + "Setup works in the profile's own account. Drop --account (or BASECAMP_ACCOUNT_ID).") + } + return bound, nil +} + +// connectCredentialKind is the kind of credential the active profile holds, +// "" for none. A store that cannot be read is an error, not "none". +func connectCredentialKind(ctx context.Context, app *appctx.App) (string, error) { + return credentialKindOf(ctx, app.Auth) +} + +func credentialKindOf(ctx context.Context, mgr *auth.Manager) (string, error) { + creds, err := mgr.GetStore().LoadContext(ctx, mgr.CredentialKey()) + switch { + case errors.Is(err, auth.ErrNoCredential): + return "", nil + case err != nil: + return "", output.ErrAuth("The stored credential could not be read: " + setup.ErrorText(err)) + case creds.OAuthType == "agent": + return setup.KindAgent, nil + default: + return setup.KindBotUser, nil + } +} + +// checkConnectIdentity proves the profile is the agent it is meant to be. +// An Agent credential must read back as an Agent person. A bot user's login +// must be the identity --expect-identity pinned, and must not be an Agent. +func checkConnectIdentity(ctx context.Context, app *appctx.App, client *basecamp.Client, kind, oauthType string, me setup.Person, expect int64) (setup.Check, error) { + c := setup.Check{Name: "Identity", Status: setup.StatusPass} + if me.ID <= 0 { + return c, output.ErrAuth(fmt.Sprintf("The profile's credential read back no person id (%d); it cannot be the agent", me.ID)) + } + switch kind { + case setup.KindAgent: + if me.PersonableType != setup.PersonableAgent { + return c, output.ErrAuth(fmt.Sprintf("The agent credential authenticates as person %d, whose type is %q, not an Agent", me.ID, me.PersonableType)) + } + c.Message = fmt.Sprintf("Agent person %d", me.ID) + default: + if me.PersonableType == setup.PersonableAgent { + return c, output.ErrAuth(fmt.Sprintf("Person %d is an Agent, but the profile holds a person's login", me.ID)) + } + endpoint, err := app.Auth.AuthorizationEndpointFor(oauthType) + if err != nil { + return c, output.ErrAuth("Could not locate the login's identity endpoint: " + setup.ErrorText(err)) + } + info, err := client.Authorization().GetInfo(ctx, &basecamp.GetInfoOptions{Endpoint: endpoint, FilterProduct: "bc3"}) + if err != nil { + return c, output.ErrAuth(fmt.Sprintf("Could not read the login's identity: %s", setup.ErrorText(err))) + } + if info.Identity.ID != expect { + return c, output.ErrAuth(fmt.Sprintf("The profile's login is identity %d, not the %d --expect-identity names; nothing was written", info.Identity.ID, expect)) + } + c.Message = fmt.Sprintf("Bot user person %d (identity %d)", me.ID, expect) + } + return c, nil +} + +// profileConfig is the configuration a named profile runs under, in the +// CLI's own precedence: environment over profile over file over defaults. +// It is root's chain for the active profile (ApplyProfile, then LoadFromEnv +// re-applied over it), minus this invocation's flags, which name the agent's +// run and not the operator's profile. +func profileConfig(profile string) (*config.Config, error) { + cfg, err := config.Load(config.FlagOverrides{}) + if err != nil { + return nil, err + } + if _, ok := cfg.Profiles[profile]; !ok { + return nil, output.ErrUsage(fmt.Sprintf("Operator profile %q does not exist", profile)) + } + if err := cfg.ApplyProfile(profile); err != nil { + return nil, err + } + if err := config.LoadFromEnv(cfg); err != nil { + return nil, err + } + return cfg, nil +} + +// operatorProfileManager checks, without the network, that the operator's +// profile can name the operator: it exists, is on the agent's Basecamp, and +// holds a person's credential. +func operatorProfileManager(ctx context.Context, app *appctx.App, profile string) (*operatorProfile, error) { + cfg, err := profileConfig(profile) + if err != nil { + return nil, err + } + if config.NormalizeBaseURL(cfg.BaseURL) != config.NormalizeBaseURL(app.Config.BaseURL) { + return nil, output.ErrUsage(fmt.Sprintf("Operator profile %q is on %s, and the agent is on %s", profile, cfg.BaseURL, app.Config.BaseURL)) + } + mgr := auth.NewManager(cfg, nil) + if store := app.Auth.GetStore(); store != nil { + mgr.SetStore(store) + } + switch kind, err := credentialKindOf(ctx, mgr); { + case err != nil: + return nil, err + case kind == "": + return nil, output.ErrUsageHint(fmt.Sprintf("Operator profile %q holds no credential", profile), "Log in: basecamp auth login -P "+shellQuote(profile)) + case kind == setup.KindAgent: + return nil, output.ErrUsage(fmt.Sprintf("Operator profile %q holds an Agent's credential; an operator is a person", profile)) + } + return &operatorProfile{mgr: mgr, baseURL: cfg.BaseURL}, nil +} + +// operatorProfile is the operator's own credential, checked and ready to read. +type operatorProfile struct { + mgr *auth.Manager + baseURL string +} + +// resolveOperatorProfile reads the operator's person in the agent's account +// through the operator's own credential, which proves who they are in a way +// a typed id cannot. +func resolveOperatorProfile(ctx context.Context, op *operatorProfile, profile, accountID string) (setup.Person, setup.Reader, error) { + client := connectSDKClientFor(op.baseURL, &managerTokens{mgr: op.mgr}) + reader := setup.SDKReader{Client: client.ForAccount(accountID)} + me, err := reader.Me(ctx) + if err != nil { + return setup.Person{}, nil, output.ErrAuth(fmt.Sprintf("Could not read who operator profile %q is in account %s: %s", profile, accountID, setup.ErrorText(err))) + } + if me.ID <= 0 { + return setup.Person{}, nil, output.ErrAuth(fmt.Sprintf("Operator profile %q reported no person id", profile)) + } + return me, reader, nil +} + +func asDoctorChecks(in []setup.Check) []Check { + out := make([]Check, len(in)) + for i, c := range in { + out[i] = Check(c) + } + return out +} + +// managerTokens is a TokenProvider over an auth manager. +type managerTokens struct{ mgr *auth.Manager } + +func (t *managerTokens) AccessToken(ctx context.Context) (string, error) { + return t.mgr.AccessToken(ctx) +} + +// connectSDKClient is the client setup reads through: no request hooks, no +// debug logger and no cache. The hooks and logger print request URLs, and +// the feed's URLs carry positions and tickets; -v on setup must not be a way +// to print either. +func connectSDKClient(app *appctx.App, tokens basecamp.TokenProvider) *basecamp.Client { + return connectSDKClientFor(app.Config.BaseURL, tokens) +} + +func connectSDKClientFor(baseURL string, tokens basecamp.TokenProvider) *basecamp.Client { + return basecamp.NewClient(&basecamp.Config{BaseURL: baseURL}, tokens, connectSDKOptions()...) +} + +// connectSDKOptions are the only options setup's clients get: a transport +// and a user agent, never hooks or a logger. +func connectSDKOptions() []basecamp.ClientOption { + return []basecamp.ClientOption{ + basecamp.WithTransport(http.DefaultTransport), + basecamp.WithUserAgent(version.UserAgent() + " " + basecamp.DefaultUserAgent), + } +} diff --git a/internal/commands/connect_setup_test.go b/internal/commands/connect_setup_test.go new file mode 100644 index 000000000..1fd306324 --- /dev/null +++ b/internal/commands/connect_setup_test.go @@ -0,0 +1,1236 @@ +//go:build unix + +package commands + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/appctx" + "github.com/basecamp/basecamp-cli/internal/auth" + "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/connector/admission" + "github.com/basecamp/basecamp-cli/internal/connector/setup" + "github.com/basecamp/basecamp-cli/internal/output" +) + +const ( + setupAgentPerson int64 = 52007412 + setupOperatorPerson int64 = 26909558 + setupBotPerson int64 = 51177542 + setupBotIdentity int64 = 4242 + setupProject int64 = 48699913 + setupClientPerson int64 = 1003 + + // Tokens the mock server tells apart. None is shaped like a real one. + setupAgentToken = "minted" + setupOperatorToken = "operator-token" + setupBotToken = "bot-token" + setupTicket = "not-a-real-ticket" +) + +// connectSetupServer is Basecamp for connect setup: the connection ceremony, and +// the account reads setup makes, answered per bearer. +type connectSetupServer struct { + srv *httptest.Server + + mu sync.Mutex + intakes int + paths []string + + // agentID is who the agent's token reads back as. + agentID int64 + // refuseAgentReads answers the project reads with 403 for an Agent token, + // as bc3 does today. + refuseAgentReads bool + // refusePeople answers the people read with 403. + refusePeople bool + // agentAsUser answers the agent's identity read as a person, not an Agent. + agentAsUser bool + // grantScope is the scope the connection hands over; "" means full. + grantScope string + // duringMint runs when the stream ticket is minted, between setup's + // identity read and its write, as another process's change would land. + duringMint func() + // mintFailure, when set, answers the stream ticket mint. + mintFailure func(w http.ResponseWriter) +} + +func startConnectSetupServer(t *testing.T) *connectSetupServer { + t.Helper() + s := &connectSetupServer{agentID: setupAgentPerson} + mux := http.NewServeMux() + bearer := func(r *http.Request) string { return strings.TrimPrefix(r.Header.Get("Authorization"), "Bearer ") } + writeJSON := func(w http.ResponseWriter, v any) { + w.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(w).Encode(v)) + } + + mux.HandleFunc("/oauth/agent_connections", func(w http.ResponseWriter, _ *http.Request) { + s.mu.Lock() + s.intakes++ + s.mu.Unlock() + writeJSON(w, map[string]any{ + "device_code": "dev-code-1", "user_code": "WDJB-MJHT", + "verification_uri_complete": s.srv.URL + "/connect?user_code=WDJB-MJHT", + "token_uri": s.srv.URL + "/oauth/agent_connection_tokens", + "expires_in": 600, "interval": 1, + }) + }) + mux.HandleFunc("/oauth/agent_connection_tokens", func(w http.ResponseWriter, _ *http.Request) { + scope := s.grantScope + if scope == "" { + scope = "full" + } + writeJSON(w, map[string]any{"client_id": "agent-client", "client_secret": fakeConnectSecret, "account_id": "999", "scope": scope}) + }) + mux.HandleFunc("/oauth/tokens", func(w http.ResponseWriter, _ *http.Request) { + scope := s.grantScope + if scope == "" { + scope = "full" + } + writeJSON(w, map[string]any{"access_token": setupAgentToken, "token_type": "bearer", "expires_in": 3600, "resource": "urn:bc:agent:42", "scope": scope}) + }) + mux.HandleFunc("/authorization.json", func(w http.ResponseWriter, r *http.Request) { + if bearer(r) != setupBotToken { + w.WriteHeader(http.StatusUnauthorized) + return + } + writeJSON(w, map[string]any{ + "identity": map[string]any{"id": setupBotIdentity, "first_name": "Bot", "last_name": "User"}, + "accounts": []map[string]any{{"id": 999, "name": "Acme", "href": s.srv.URL + "/999", "product": "bc3"}}, + }) + }) + mux.HandleFunc("/999/my/profile.json", func(w http.ResponseWriter, r *http.Request) { + switch bearer(r) { + case setupAgentToken: + personable := "Agent" + if s.agentAsUser { + personable = "User" + } + writeJSON(w, map[string]any{"id": s.agentID, "name": "Marie Chef", "personable_type": personable}) + case setupOperatorToken: + writeJSON(w, map[string]any{"id": setupOperatorPerson, "name": "Operator", "personable_type": "User"}) + case setupBotToken: + writeJSON(w, map[string]any{"id": setupBotPerson, "name": "Bot", "personable_type": "User"}) + default: + w.WriteHeader(http.StatusUnauthorized) + } + }) + mux.HandleFunc(fmt.Sprintf("/999/people/%d", setupOperatorPerson), func(w http.ResponseWriter, _ *http.Request) { + if s.refusePeople { + w.WriteHeader(http.StatusForbidden) + return + } + writeJSON(w, map[string]any{"id": setupOperatorPerson, "name": "Operator", "personable_type": "User"}) + }) + mux.HandleFunc(fmt.Sprintf("/999/people/%d", setupOperatorPerson+1), func(w http.ResponseWriter, r *http.Request) { + if bearer(r) != setupOperatorToken { + w.WriteHeader(http.StatusForbidden) // as bc3 refuses an Agent today + return + } + writeJSON(w, map[string]any{"id": setupOperatorPerson + 1, "name": "Colleague", "personable_type": "User"}) + }) + mux.HandleFunc(fmt.Sprintf("/999/people/%d", setupAgentPerson), func(w http.ResponseWriter, _ *http.Request) { + writeJSON(w, map[string]any{"id": setupAgentPerson, "name": "Marie Chef", "personable_type": "Agent"}) + }) + mux.HandleFunc(fmt.Sprintf("/999/people/%d", setupClientPerson), func(w http.ResponseWriter, _ *http.Request) { + writeJSON(w, map[string]any{"id": setupClientPerson, "name": "Client", "personable_type": "User", "client": true}) + }) + mux.HandleFunc("/999/events/stream_ticket.json", func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + w.WriteHeader(http.StatusMethodNotAllowed) + return + } + if s.duringMint != nil { + s.duringMint() + } + if s.mintFailure != nil { + s.mintFailure(w) + return + } + writeJSON(w, map[string]any{"ticket": setupTicket, "expires_in": 120, "url": "wss://example.test/cable?ticket=" + setupTicket}) + }) + projectRead := func(w http.ResponseWriter, r *http.Request, body any) { + if s.refuseAgentReads && bearer(r) == setupAgentToken { + w.WriteHeader(http.StatusForbidden) + return + } + writeJSON(w, body) + } + mux.HandleFunc(fmt.Sprintf("/999/projects/%d", setupProject), func(w http.ResponseWriter, r *http.Request) { + projectRead(w, r, map[string]any{"id": setupProject, "name": "Connector"}) + }) + mux.HandleFunc(fmt.Sprintf("/999/projects/%d/people.json", setupProject), func(w http.ResponseWriter, r *http.Request) { + projectRead(w, r, []any{}) + }) + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + s.mu.Lock() + s.paths = append(s.paths, r.Method+" "+r.URL.Path) + s.mu.Unlock() + http.NotFound(w, r) + }) + + s.srv = httptest.NewServer(mux) + t.Cleanup(s.srv.Close) + return s +} + +func (s *connectSetupServer) intakeCount() int { + s.mu.Lock() + defer s.mu.Unlock() + return s.intakes +} + +// bareSetupApp is an App for connect setup with nothing connected yet: a +// file credential store and the global config under a temp XDG_CONFIG_HOME, +// and the issuer pinned to the mock server. +func bareSetupApp(t *testing.T, s *connectSetupServer, profile string) *appctx.App { + t.Helper() + t.Setenv("BASECAMP_NO_KEYRING", "1") + t.Setenv("BASECAMP_TOKEN", "") + t.Setenv("BASECAMP_NONINTERACTIVE", "") + t.Setenv("BASECAMP_OAUTH_ISSUER", s.srv.URL) + home := t.TempDir() + t.Setenv("USERPROFILE", home) + t.Setenv("XDG_CONFIG_HOME", home) + return newConnectSetupApp(t, s, profile) +} + +// connectSetupApp is bareSetupApp with the profile already holding the +// agent's credential when it is "agent": connected the way an operator +// connects it, with `basecamp auth agent connect`. +func connectSetupApp(t *testing.T, s *connectSetupServer, profile string) *appctx.App { + t.Helper() + app := bareSetupApp(t, s, profile) + if profile != "agent" { + return app + } + out, err := runAgentConnect(t, app) + require.NoError(t, err, out) + return newConnectSetupApp(t, s, profile) +} + +// newConnectSetupApp builds an App over the environment already set, as a second +// invocation of the CLI would: from the global config file on disk. +func newConnectSetupApp(t *testing.T, s *connectSetupServer, profile string) *appctx.App { + t.Helper() + cfg, err := config.Load(config.FlagOverrides{}) + require.NoError(t, err) + cfg.BaseURL = s.srv.URL + if _, ok := cfg.Profiles[profile]; ok { + require.NoError(t, cfg.ApplyProfile(profile)) + } else { + cfg.ActiveProfile = profile + } + authMgr := auth.NewManager(cfg, s.srv.Client()) + authMgr.SetStore(auth.NewStore(config.GlobalConfigDir())) + return &appctx.App{ + Config: cfg, + Auth: authMgr, + Output: output.New(output.Options{Format: output.FormatStyled, Writer: &bytes.Buffer{}}), + } +} + +func runConnectSetupCmd(t *testing.T, app *appctx.App, args ...string) (string, error) { + t.Helper() + return runConnectSetupCmdIn(context.Background(), t, app, args...) +} + +// runConnectSetupCmdIn runs the command under ctx, which the command tree +// carries itself (cobra's SetContext), so there is no context to pass on. +// +//nolint:contextcheck // the context is handed to the command, not to a call +func runConnectSetupCmdIn(ctx context.Context, t *testing.T, app *appctx.App, args ...string) (string, error) { + t.Helper() + cmd := NewConnectCmd() + cmd.SetArgs(append([]string{"setup"}, args...)) + cmd.SetContext(appctx.WithApp(ctx, app)) + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&out) + cmd.SilenceErrors = true + cmd.SilenceUsage = true + err := cmd.Execute() + return out.String(), err +} + +// storeConnectProfile registers a profile in the global config and stores a +// person's token under it, as a completed login would have. +func storeConnectProfile(t *testing.T, s *connectSetupServer, name, token string) { + t.Helper() + storeConnectProfileScoped(t, s, name, token, "full") +} + +func storeConnectProfileScoped(t *testing.T, s *connectSetupServer, name, token, scope string) { + t.Helper() + _, err := registerProfile(name, &config.ProfileConfig{BaseURL: s.srv.URL, AccountID: "999", Scope: scope}) + require.NoError(t, err) + cfg := config.Default() + cfg.BaseURL = s.srv.URL + cfg.ActiveProfile = name + mgr := auth.NewManager(cfg, s.srv.Client()) + mgr.SetStore(auth.NewStore(config.GlobalConfigDir())) + require.NoError(t, mgr.ImportToken(context.Background(), token, scope, "", "", time.Now().Add(24*time.Hour))) +} + +// routeArg routes the test project to a fresh directory. +func routeArg(t *testing.T) string { + t.Helper() + return fmt.Sprintf("--route=%d=%s", setupProject, t.TempDir()) +} + +// firstSetup runs a successful first setup of the agent profile. +func firstSetup(t *testing.T, s *connectSetupServer) { + t.Helper() + out, err := runConnectSetupCmd(t, connectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupOperatorPerson), routeArg(t)) + require.NoError(t, err, out) +} + +func assertNotWritten(t *testing.T, profile string) { + t.Helper() + _, statErr := os.Stat(connectSetupPath(t, profile)) + assert.True(t, os.IsNotExist(statErr), "nothing was written") +} + +func connectSetupPath(t *testing.T, profile string) string { + t.Helper() + path, err := setup.Path(config.GlobalConfigDir(), profile) + require.NoError(t, err) + return path +} + +// On a profile connected to the agent, setup leaves a connect.json with a +// routed project that admission reads, and passing token, identity and mint +// checks. +func TestConnectSetupOnAConnectedProfile(t *testing.T) { + s := startConnectSetupServer(t) + app := connectSetupApp(t, s, "agent") + repo := t.TempDir() + + out, err := runConnectSetupCmd(t, app, + "--operator", fmt.Sprint(setupOperatorPerson), + "--route", fmt.Sprintf("%d=%s", setupProject, repo), + "--watch-completions", fmt.Sprint(setupProject), + "--class", fmt.Sprintf("%d=internal", setupProject)) + require.NoError(t, err, out) + + assert.NotContains(t, out, fakeConnectSecret, "the client secret is never echoed") + assert.NotContains(t, out, setupTicket, "the stream ticket is never echoed") + assert.NotContains(t, out, "not ready") + for _, check := range []string{"Token", "Identity", "Operator", "Stream ticket", fmt.Sprintf("Project %d", setupProject)} { + assert.Contains(t, out, check) + } + + data, err := os.ReadFile(connectSetupPath(t, "agent")) + require.NoError(t, err) + p, err := admission.ParsePolicy(data) + require.NoError(t, err) + p.AgentID = setupAgentPerson + require.NoError(t, p.Validate()) + assert.Equal(t, setupOperatorPerson, p.Trust.OperatorID) + realRepo, err := filepath.EvalSymlinks(repo) + require.NoError(t, err) + assert.Equal(t, admission.Route{Path: realRepo, Class: "internal", WatchCompletions: true}, p.Projects[setupProject]) + + f, err := setup.Load(connectSetupPath(t, "agent")) + require.NoError(t, err) + assert.Equal(t, setup.Agent{PersonID: setupAgentPerson, Kind: setup.KindAgent}, f.Agent) + assert.Equal(t, "999", f.AccountID) + assert.NotContains(t, string(data), fakeConnectSecret, "connect.json holds no credential") + + // A second run uses the connected profile as it is, and keeps what it + // is not told to change. + app = newConnectSetupApp(t, s, "agent") + out, err = runConnectSetupCmd(t, app, "--concurrency", "4") + require.NoError(t, err, out) + again, err := setup.Load(connectSetupPath(t, "agent")) + require.NoError(t, err) + assert.Equal(t, 4, again.Concurrency) + assert.Equal(t, f.Projects, again.Projects) + assert.Equal(t, f.Trust, again.Trust) +} + +// bc3 refuses admission's reads to an Agent identity today. Setup says so +// in words, and fails: a connector that would block every event is not +// ready, and the exit status says so to a script as the output does to a +// person. +func TestConnectSetupNamesTheAgentReadRefusal(t *testing.T) { + s := startConnectSetupServer(t) + s.refuseAgentReads = true + app := connectSetupApp(t, s, "agent") + + out, err := runConnectSetupCmd(t, app, "--operator", fmt.Sprint(setupOperatorPerson), routeArg(t)) + require.Error(t, err, out) + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, "not_ready", apiErr.Code) + assert.Contains(t, apiErr.Message, "Agent identity") + assert.Contains(t, apiErr.Hint, "--expect-identity") + assert.Contains(t, out, "Agent identity", "the checks are still shown") + assertNotWritten(t, "agent") +} + +func TestConnectSetupWithNoRouteIsNotReady(t *testing.T) { + s := startConnectSetupServer(t) + out, err := runConnectSetupCmd(t, connectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupOperatorPerson)) + require.Error(t, err, out) + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, "not_ready", apiErr.Code) +} + +// Bad input is refused before anything is read or written. +func TestConnectSetupRefusesBadInput(t *testing.T) { + op := fmt.Sprint(setupOperatorPerson) + wantMessage := map[string]string{ + "operator profile unlogged": "holds no credential", + "missing operator profile": "does not exist", + "no operator": "who the operator is", + "expect-identity on agent": "holds an Agent's credential", + } + for name, args := range map[string][]string{ + "missing route directory": {"--operator", op, "--route", fmt.Sprintf("%d=/does/not/exist", setupProject)}, + "class without a route": {"--operator", op, "--class", fmt.Sprintf("%d=internal", setupProject)}, + "bad trust mode": {"--operator", op, "--trust", "domain"}, + "allow outside allowlist": {"--operator", op, "--trust", "project", "--allow", "7"}, + "bad driver": {"--operator", op, "--driver", "fork"}, + "bad concurrency": {"--operator", op, "--concurrency", "100"}, + "zero concurrency": {"--operator", op, "--concurrency", "0"}, + "bad deadline": {"--operator", op, "--deadline", "5s"}, + "zero deadline": {"--operator", op, "--deadline", "0"}, + "malformed route": {"--operator", op, "--route", "not-a-pair"}, + "no operator": {"--route", fmt.Sprintf("%d=%s", setupProject, os.TempDir())}, + "missing operator profile": {"--operator-profile", "nobody"}, + "operator profile unlogged": {"--operator-profile", "unlogged"}, + "expect-identity on agent": {"--operator", op, "--expect-identity", "4242"}, + } { + t.Run(name, func(t *testing.T) { + s := startConnectSetupServer(t) + connectSetupApp(t, s, "agent") + _, err := registerProfile("unlogged", &config.ProfileConfig{BaseURL: s.srv.URL, AccountID: "999"}) + require.NoError(t, err) + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), args...) + require.Error(t, err, out) + assertNotWritten(t, "agent") + if want, ok := wantMessage[name]; ok { + assert.Contains(t, err.Error(), want) + } + }) + } +} + +// Setup does not obtain a credential: a profile without one is refused +// with the command that connects it, and nothing is requested or written. +func TestConnectSetupRefusesAProfileWithNoCredential(t *testing.T) { + s := startConnectSetupServer(t) + bareSetupApp(t, s, "agent") + _, err := registerProfile("agent", &config.ProfileConfig{BaseURL: s.srv.URL, AccountID: "999"}) + require.NoError(t, err) + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupOperatorPerson), routeArg(t)) + require.Error(t, err, out) + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, output.CodeAuth, apiErr.Code) + assert.Contains(t, apiErr.Hint, "basecamp auth agent connect -P agent") + assert.Zero(t, s.intakeCount(), "setup never starts a connection") + assertNotWritten(t, "agent") + + out, err = runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupOperatorPerson), "--expect-identity", "4242", routeArg(t)) + require.Error(t, err, out) + require.ErrorAs(t, err, &apiErr) + assert.Contains(t, apiErr.Hint, "basecamp auth login -P agent --expect-identity 4242") +} + +// Setup works in the account the agent's profile is bound to: never a +// config-wide default, and an explicit --account naming another account is +// refused rather than preferred. +func TestConnectSetupWorksInTheProfilesOwnAccount(t *testing.T) { + s := startConnectSetupServer(t) + connectSetupApp(t, s, "agent") + + app := newConnectSetupApp(t, s, "agent") + app.Config.AccountID = "1000" // a config-wide default, not the profile's + app.Config.Sources["account_id"] = string(config.SourceGlobal) + out, err := runConnectSetupCmd(t, app, "--operator", fmt.Sprint(setupOperatorPerson), routeArg(t)) + require.NoError(t, err, out) + f, err := setup.Load(connectSetupPath(t, "agent")) + require.NoError(t, err) + assert.Equal(t, "999", f.AccountID) + + app = newConnectSetupApp(t, s, "agent") + app.Config.AccountID = "1000" + app.Config.Sources["account_id"] = string(config.SourceFlag) + out, err = runConnectSetupCmd(t, app, "--concurrency", "3") + require.Error(t, err, out) + assert.Contains(t, err.Error(), "bound to account 999") +} + +// A credential that reads back no person id is not the agent. +func TestConnectSetupRefusesAZeroPersonID(t *testing.T) { + s := startConnectSetupServer(t) + connectSetupApp(t, s, "agent") + s.agentID = 0 + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupOperatorPerson), routeArg(t)) + require.Error(t, err, out) + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, output.CodeAuth, apiErr.Code) + assert.Contains(t, apiErr.Message, "no person id") + assertNotWritten(t, "agent") +} + +// Every setting setup writes can be reversed by setup: a class is cleared +// with =. +func TestConnectSetupClearsAClass(t *testing.T) { + s := startConnectSetupServer(t) + out, err := runConnectSetupCmd(t, connectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupOperatorPerson), routeArg(t), + "--class", fmt.Sprintf("%d=internal", setupProject)) + require.NoError(t, err, out) + + out, err = runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--class", fmt.Sprintf("%d=", setupProject)) + require.NoError(t, err, out) + f, err := setup.Load(connectSetupPath(t, "agent")) + require.NoError(t, err) + assert.Empty(t, f.Projects[setupProject].Class) +} + +// Setup never changes the credential: a readiness failure, a wrong agent +// or a scope that is too narrow leaves it as it was, and each reports its +// own code. +func TestConnectSetupNeverChangesTheCredential(t *testing.T) { + credential := func(t *testing.T) string { + t.Helper() + data, err := os.ReadFile(filepath.Join(config.GlobalConfigDir(), "credentials.json")) + require.NoError(t, err) + return string(data) + } + for name, tc := range map[string]struct { + prepare func(s *connectSetupServer) + code string + }{ + "a readiness check fails": {prepare: func(s *connectSetupServer) { s.refuseAgentReads = true }, code: codeNotReady}, + "not an Agent": {prepare: func(s *connectSetupServer) { s.agentAsUser = true }, code: output.CodeAuth}, + "granted read only": {prepare: func(s *connectSetupServer) { s.grantScope = "read" }, code: codeNotReady}, + } { + t.Run(name, func(t *testing.T) { + s := startConnectSetupServer(t) + tc.prepare(s) + connectSetupApp(t, s, "agent") + before := credential(t) + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupOperatorPerson), routeArg(t)) + require.Error(t, err, out) + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, tc.code, apiErr.Code) + assert.Equal(t, before, credential(t)) + assertNotWritten(t, "agent") + }) + } +} + +func TestConnectSetupRefusesAConnectJSONOthersCanWrite(t *testing.T) { + s := startConnectSetupServer(t) + firstSetup(t, s) + + path := connectSetupPath(t, "agent") + require.NoError(t, os.Chmod(path, 0o666)) + before, err := os.ReadFile(path) + require.NoError(t, err) + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--concurrency", "3") + require.Error(t, err, out) + assert.Contains(t, err.Error(), "not private") + after, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, before, after) +} + +// A profile re-pointed at another agent does not inherit the first agent's +// trust and routes. +func TestConnectSetupRefusesADifferentAgentUnderTheSameProfile(t *testing.T) { + s := startConnectSetupServer(t) + firstSetup(t, s) + before, err := os.ReadFile(connectSetupPath(t, "agent")) + require.NoError(t, err) + + s.agentID = 777 + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent")) + require.Error(t, err, out) + assert.Contains(t, err.Error(), "person 777") + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, output.CodeAuth, apiErr.Code, "a different agent is a wrong credential") + after, err := os.ReadFile(connectSetupPath(t, "agent")) + require.NoError(t, err) + assert.Equal(t, before, after) +} + +func TestConnectSetupRefusesAnotherAccountUnderTheSameProfile(t *testing.T) { + s := startConnectSetupServer(t) + firstSetup(t, s) + before, err := os.ReadFile(connectSetupPath(t, "agent")) + require.NoError(t, err) + + configData, configPath, err := loadGlobalConfigFile() + require.NoError(t, err) + globalProfileEntry(configData, "agent")["account_id"] = "1000" // the profile rebound elsewhere + require.NoError(t, atomicWriteJSON(configPath, configData)) + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent")) + require.Error(t, err, out) + assert.Contains(t, err.Error(), "set up in account 999") + after, err := os.ReadFile(connectSetupPath(t, "agent")) + require.NoError(t, err) + assert.Equal(t, before, after) +} + +func TestConnectSetupRefusesTheAgentAsItsOwnOperator(t *testing.T) { + s := startConnectSetupServer(t) + out, err := runConnectSetupCmd(t, connectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupAgentPerson), routeArg(t)) + require.Error(t, err, out) + assert.Contains(t, err.Error(), "never authorizes", "refused by setup, before the file layer's own refusal") + assertNotWritten(t, "agent") +} + +// The trust anchor is verified before it is recorded: an operator id that +// is a client, or that the agent cannot read, is refused and nothing is +// written. +func TestConnectSetupVerifiesTheOperatorBeforeWriting(t *testing.T) { + s := startConnectSetupServer(t) + out, err := runConnectSetupCmd(t, connectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupClientPerson), routeArg(t)) + require.Error(t, err, out) + assert.Contains(t, err.Error(), "client") + assertNotWritten(t, "agent") + + s.refusePeople = true + out, err = runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupOperatorPerson), routeArg(t)) + require.Error(t, err, out) + assert.Contains(t, err.Error(), "cannot be verified") + assertNotWritten(t, "agent") +} + +// The operator named by their own profile is who that credential proves +// they are, read in the agent's account. +func TestConnectSetupResolvesTheOperatorFromTheirProfile(t *testing.T) { + s := startConnectSetupServer(t) + s.refusePeople = true // the agent need not read the operator: their own credential did + connectSetupApp(t, s, "agent") + storeConnectProfile(t, s, "me", setupOperatorToken) + app := newConnectSetupApp(t, s, "agent") + + out, err := runConnectSetupCmd(t, app, "--operator-profile", "me", routeArg(t)) + require.NoError(t, err, out) + f, err := setup.Load(connectSetupPath(t, "agent")) + require.NoError(t, err) + assert.Equal(t, setupOperatorPerson, f.Trust.OperatorID) + assert.Contains(t, out, `profile "me"`) +} + +// A person's login under the agent profile could be the operator's own, so +// the bot-user path is pinned by identity. +func TestConnectSetupOnTheBotUserPath(t *testing.T) { + s := startConnectSetupServer(t) + connectSetupApp(t, s, "bot") + storeConnectProfile(t, s, "bot", setupBotToken) + + _, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "bot"), "--operator", fmt.Sprint(setupOperatorPerson), routeArg(t)) + require.Error(t, err, "a person's login without --expect-identity could be the operator's own") + assert.Contains(t, err.Error(), "a person's login, not an Agent's credential") + assertNotWritten(t, "bot") + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "bot"), "--operator", fmt.Sprint(setupOperatorPerson), "--expect-identity", "1", routeArg(t)) + require.Error(t, err, out) + assert.Contains(t, err.Error(), "not the 1 --expect-identity names") + assertNotWritten(t, "bot") + + out, err = runConnectSetupCmd(t, newConnectSetupApp(t, s, "bot"), + "--operator", fmt.Sprint(setupOperatorPerson), + "--expect-identity", fmt.Sprint(setupBotIdentity), + routeArg(t)) + require.NoError(t, err, out) + assert.NotContains(t, out, setupTicket) + f, err := setup.Load(connectSetupPath(t, "bot")) + require.NoError(t, err) + assert.Equal(t, setup.Agent{PersonID: setupBotPerson, Kind: setup.KindBotUser, IdentityID: setupBotIdentity}, f.Agent) + + // The pinned identity is remembered: a rerun need not restate it. + out, err = runConnectSetupCmd(t, newConnectSetupApp(t, s, "bot"), "--worktrees") + require.NoError(t, err, out) +} + +// A read-only credential cannot reply or acknowledge, so it is not ready. +func TestConnectSetupReadOnlyCredentialIsNotReady(t *testing.T) { + s := startConnectSetupServer(t) + connectSetupApp(t, s, "bot") + storeConnectProfileScoped(t, s, "bot", setupBotToken, "read") + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "bot"), + "--operator", fmt.Sprint(setupOperatorPerson), "--expect-identity", fmt.Sprint(setupBotIdentity), routeArg(t)) + require.Error(t, err, out) + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, "not_ready", apiErr.Code) + assert.Contains(t, apiErr.Message, `granted "read"`) +} + +func TestConnectSetupRefusesExpectIdentityForAnAgent(t *testing.T) { + s := startConnectSetupServer(t) + app := connectSetupApp(t, s, "agent") + + // The credential alone says it: an Agent has no identity to pin. + out, err := runConnectSetupCmd(t, app, "--operator", fmt.Sprint(setupOperatorPerson), "--expect-identity", "4242", routeArg(t)) + require.Error(t, err, out) + assert.Contains(t, err.Error(), "holds an Agent's credential") + assert.Contains(t, err.Error(), "Drop --expect-identity") + assertNotWritten(t, "agent") + + // And once connect.json says it, the file is what the remediation + // talks about, since a bot login would not satisfy it either. + out, err = runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupOperatorPerson), routeArg(t)) + require.NoError(t, err, out) + before, err := os.ReadFile(connectSetupPath(t, "agent")) + require.NoError(t, err) + + out, err = runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--expect-identity", "4242") + require.Error(t, err, out) + assert.Contains(t, err.Error(), "connect.json was set up for an Agent") + assert.Contains(t, err.Error(), "basecamp auth agent connect -P agent") + after, err := os.ReadFile(connectSetupPath(t, "agent")) + require.NoError(t, err) + assert.Equal(t, before, after) +} + +// Every credential conflict names a command that fixes it. +func TestConnectSetupConflictsNameACommandToRun(t *testing.T) { + for name, tc := range map[string]struct { + prepare func(t *testing.T, s *connectSetupServer) + args []string + want string + }{ + "no credential": { + prepare: func(t *testing.T, s *connectSetupServer) { + _, err := registerProfile("agent", &config.ProfileConfig{BaseURL: s.srv.URL, AccountID: "999"}) + require.NoError(t, err) + }, + want: "basecamp auth agent connect -P agent", + }, + "no credential, bot path": { + prepare: func(t *testing.T, s *connectSetupServer) { + _, err := registerProfile("agent", &config.ProfileConfig{BaseURL: s.srv.URL, AccountID: "999"}) + require.NoError(t, err) + }, + args: []string{"--expect-identity", "4242"}, + want: "basecamp auth login -P agent --expect-identity 4242", + }, + "a person's login with nothing pinning it": { + prepare: func(t *testing.T, s *connectSetupServer) { storeConnectProfile(t, s, "agent", setupBotToken) }, + want: "--expect-identity", + }, + } { + t.Run(name, func(t *testing.T) { + s := startConnectSetupServer(t) + bareSetupApp(t, s, "agent") + tc.prepare(t, s) + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), append(tc.args, "--operator", fmt.Sprint(setupOperatorPerson), routeArg(t))...) + require.Error(t, err, out) + assert.Contains(t, err.Error(), tc.want) + assertNotWritten(t, "agent") + }) + } +} + +// The operator profile is read under the CLI's own precedence: the +// environment wins over what the profile's entry stores, as it does for the +// agent's own profile. +func TestConnectSetupOperatorProfileFollowsEnvironmentPrecedence(t *testing.T) { + for name, tc := range map[string]struct { + stored string // the operator profile entry's base_url + env string // BASECAMP_BASE_URL + refused bool + }{ + "stored matches, no environment": {stored: "server"}, + "environment supplies the match": {stored: "https://elsewhere.example", env: "server"}, + "environment moves both to the same": {stored: "server", env: "server"}, + "stored differs, no environment": {stored: "https://elsewhere.example", refused: true}, + } { + t.Run(name, func(t *testing.T) { + s := startConnectSetupServer(t) + connectSetupApp(t, s, "agent") + stored := tc.stored + if stored == "server" { + stored = s.srv.URL + } + _, err := registerProfile("me", &config.ProfileConfig{BaseURL: stored, AccountID: "999", Scope: "full"}) + require.NoError(t, err) + cfg := config.Default() + cfg.BaseURL = s.srv.URL + cfg.ActiveProfile = "me" + mgr := auth.NewManager(cfg, s.srv.Client()) + mgr.SetStore(auth.NewStore(config.GlobalConfigDir())) + require.NoError(t, mgr.ImportToken(context.Background(), setupOperatorToken, "full", "", "", time.Now().Add(24*time.Hour))) + if tc.env != "" { + t.Setenv("BASECAMP_BASE_URL", s.srv.URL) + } + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--operator-profile", "me", routeArg(t)) + if tc.refused { + require.Error(t, err, out) + assert.Contains(t, err.Error(), "is on") + return + } + require.NoError(t, err, out) + f, err := setup.Load(connectSetupPath(t, "agent")) + require.NoError(t, err) + assert.Equal(t, setupOperatorPerson, f.Trust.OperatorID) + }) + } +} + +// A profile another setup is working on is busy, not broken. +func TestConnectSetupReportsAnotherSetupAsBusy(t *testing.T) { + s := startConnectSetupServer(t) + connectSetupApp(t, s, "agent") + unlock, err := setup.Lock(connectSetupPath(t, "agent")) + require.NoError(t, err) + t.Cleanup(unlock) + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupOperatorPerson), routeArg(t)) + require.Error(t, err, out) + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, output.CodeBusy, apiErr.Code) + assert.Contains(t, apiErr.Hint, "Run setup again") +} + +// A credential removed while the checks run is an auth failure, not a +// usage error, and nothing is written. +func TestConnectSetupReportsACredentialRemovedMidRunAsAuth(t *testing.T) { + s := startConnectSetupServer(t) + connectSetupApp(t, s, "agent") + s.duringMint = func() { + cfg := config.Default() + cfg.ActiveProfile = "agent" + require.NoError(t, auth.NewStore(config.GlobalConfigDir()).Delete(auth.NewManager(cfg, nil).CredentialKey())) + } + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupOperatorPerson), routeArg(t)) + require.Error(t, err, out) + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, output.CodeAuth, apiErr.Code) + assert.Contains(t, apiErr.Hint, "basecamp auth agent connect -P agent") + assertNotWritten(t, "agent") +} + +// Machine output carries the result, and neither the client secret nor the +// stream ticket. +func TestConnectSetupJSONOutput(t *testing.T) { + s := startConnectSetupServer(t) + firstSetup(t, s) + + app := newConnectSetupApp(t, s, "agent") + var buf bytes.Buffer + app.Output = output.New(output.Options{Format: output.FormatJSON, Writer: &buf}) + out, err := runConnectSetupCmd(t, app, "--concurrency", "3") + require.NoError(t, err, out) + + var envelope struct { + OK bool `json:"ok"` + Data struct { + Ready bool `json:"ready"` + Written bool `json:"written"` + AgentPersonID int64 `json:"agent_person_id"` + OperatorID int64 `json:"operator_id"` + } `json:"data"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope), buf.String()) + assert.True(t, envelope.OK) + assert.True(t, envelope.Data.Ready) + assert.True(t, envelope.Data.Written) + assert.Equal(t, setupAgentPerson, envelope.Data.AgentPersonID) + assert.Equal(t, setupOperatorPerson, envelope.Data.OperatorID) + assert.NotContains(t, buf.String(), setupTicket) + assert.NotContains(t, buf.String(), fakeConnectSecret) +} + +// A credential store that cannot be read is not "no credential": setup must +// not run a new connection over a credential it only failed to load. +func TestConnectSetupRefusesAnUnreadableCredentialStore(t *testing.T) { + s := startConnectSetupServer(t) + app := connectSetupApp(t, s, "agent") + require.NoError(t, os.MkdirAll(config.GlobalConfigDir(), 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(config.GlobalConfigDir(), "credentials.json"), []byte("{not json"), 0o600)) + + out, err := runConnectSetupCmd(t, app, "--operator", fmt.Sprint(setupOperatorPerson), routeArg(t)) + require.Error(t, err, out) + assertNotWritten(t, "agent") +} + +// An operator proven by their own profile stays proven: a later run that +// changes only a route keeps it, even though the agent cannot read people. +func TestConnectSetupKeepsARecordedOperatorTheAgentCannotRead(t *testing.T) { + s := startConnectSetupServer(t) + s.refusePeople = true + connectSetupApp(t, s, "agent") + storeConnectProfile(t, s, "me", setupOperatorToken) + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--operator-profile", "me", routeArg(t)) + require.NoError(t, err, out) + + out, err = runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), routeArg(t)) + require.NoError(t, err, out) + assert.Contains(t, out, "could not be re-read") + f, err := setup.Load(connectSetupPath(t, "agent")) + require.NoError(t, err) + assert.Equal(t, setupOperatorPerson, f.Trust.OperatorID) + + // A different id is a new trust anchor, verified as one: here it reads + // back as a client and is refused. + s.refusePeople = false + out, err = runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupClientPerson)) + require.Error(t, err, out) + assert.Contains(t, err.Error(), "client") +} + +// Everyone connect.json trusts is verified before it is written, the +// allowlist included: an id nobody can read, or one that is a client, is +// refused and nothing is written. +func TestConnectSetupVerifiesTheAllowlistBeforeWriting(t *testing.T) { + for name, id := range map[string]int64{"unknown person": 424242, "client": setupClientPerson, "the agent": setupAgentPerson} { + t.Run(name, func(t *testing.T) { + s := startConnectSetupServer(t) + connectSetupApp(t, s, "agent") + storeConnectProfile(t, s, "me", setupOperatorToken) + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--operator-profile", "me", "--allow", fmt.Sprint(id), routeArg(t)) + require.Error(t, err, out) + assert.Contains(t, err.Error(), fmt.Sprintf("Allowlist %d", id)) + assertNotWritten(t, "agent") + }) + } + + s := startConnectSetupServer(t) + connectSetupApp(t, s, "agent") + storeConnectProfile(t, s, "me", setupOperatorToken) + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--operator-profile", "me", "--allow", fmt.Sprint(setupOperatorPerson+1), routeArg(t)) + require.NoError(t, err, out) + f, err := setup.Load(connectSetupPath(t, "agent")) + require.NoError(t, err) + assert.Equal(t, []int64{setupOperatorPerson + 1}, f.Trust.AllowlistIDs) +} + +// The scope that decides readiness is the one the credential was granted, +// not the one the profile's configuration names. +func TestConnectSetupReadsTheGrantedScopeNotTheProfiles(t *testing.T) { + s := startConnectSetupServer(t) + connectSetupApp(t, s, "bot") + storeConnectProfile(t, s, "bot", setupBotToken) // configured full + cfg := config.Default() + cfg.BaseURL = s.srv.URL + cfg.ActiveProfile = "bot" + mgr := auth.NewManager(cfg, s.srv.Client()) + mgr.SetStore(auth.NewStore(config.GlobalConfigDir())) + require.NoError(t, mgr.ImportToken(context.Background(), setupBotToken, "read", "", "", time.Now().Add(24*time.Hour))) // granted read + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "bot"), + "--operator", fmt.Sprint(setupOperatorPerson), "--expect-identity", fmt.Sprint(setupBotIdentity), routeArg(t)) + require.Error(t, err, out) + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, "not_ready", apiErr.Code) + assert.Contains(t, apiErr.Message, `"read"`) +} + +// The stream ticket is a bearer credential. When the mint fails with a body +// that carries one, as a malformed error response can, that body reaches no +// output: not the checks, not the error, not connect.json. +func TestConnectSetupNeverPrintsATicketFromAFailedMint(t *testing.T) { + const canary = "CANARY-bearer-ticket-7f3a" + s := startConnectSetupServer(t) + s.mintFailure = func(w http.ResponseWriter) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusUnprocessableEntity) + fmt.Fprintf(w, `{"error":%q,"message":%q,"ticket":%q,"url":"wss://example.test/cable?ticket=%s"}`, canary, "ticket "+canary, canary, canary) + } + connectSetupApp(t, s, "agent") + storeConnectProfile(t, s, "me", setupOperatorToken) + + app := newConnectSetupApp(t, s, "agent") + var envelope bytes.Buffer + app.Output = output.New(output.Options{Format: output.FormatStyled, Writer: &envelope}) + out, err := runConnectSetupCmd(t, app, "--operator-profile", "me", routeArg(t)) + require.Error(t, err, out) + + assert.NotContains(t, out, canary, "command output") + assert.NotContains(t, err.Error(), canary, "the error") + assert.NotContains(t, envelope.String(), canary, "the app's output") + assertNotWritten(t, "agent") + assert.Contains(t, err.Error(), "HTTP 422", "the failure is still reported, by status") +} + +// A command setup suggests is meant to be pasted, so a config-derived +// profile name in it is shell-quoted. +func TestConnectSetupQuotesProfileNamesInSuggestedCommands(t *testing.T) { + s := startConnectSetupServer(t) + connectSetupApp(t, s, "agent") + _, err := registerProfile("my profile;rm", &config.ProfileConfig{BaseURL: s.srv.URL, AccountID: "999"}) + require.NoError(t, err) + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--operator-profile", "my profile;rm") + require.Error(t, err, out) + assert.Contains(t, err.Error(), `basecamp auth login -P 'my profile;rm'`) +} + +// connect.json's path comes from the environment, and reaches one-line +// terminal sinks: control characters in it must not break those lines. +func TestConnectSetupSanitizesThePathItPrints(t *testing.T) { + s := startConnectSetupServer(t) + bareSetupApp(t, s, "bot") + home := filepath.Join(t.TempDir(), "evil\nFAKE ✓ line") + require.NoError(t, os.Mkdir(home, 0o700)) + t.Setenv("XDG_CONFIG_HOME", home) + t.Setenv("USERPROFILE", home) + storeConnectProfile(t, s, "bot", setupBotToken) + + args := []string{"--operator", fmt.Sprint(setupOperatorPerson), "--expect-identity", fmt.Sprint(setupBotIdentity)} + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "bot"), args...) // no route: not ready, and the error names the path + require.Error(t, err, out) + assert.NotContains(t, err.Error(), "evil\nFAKE") + + out, err = runConnectSetupCmd(t, newConnectSetupApp(t, s, "bot"), append(args, routeArg(t))...) + require.NoError(t, err, out) + assert.Contains(t, out, "connect.json written") + assert.NotContains(t, out, "evil\nFAKE") +} + +// A profile whose credential changed kind since connect.json was written is +// a wrong credential, refused as auth with connect.json untouched. +func TestConnectSetupRefusesACredentialOfAnotherKind(t *testing.T) { + s := startConnectSetupServer(t) + bareSetupApp(t, s, "agent") + storeConnectProfile(t, s, "agent", setupBotToken) + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), + "--operator", fmt.Sprint(setupOperatorPerson), "--expect-identity", fmt.Sprint(setupBotIdentity), routeArg(t)) + require.NoError(t, err, out) + before, err := os.ReadFile(connectSetupPath(t, "agent")) + require.NoError(t, err) + + // The profile is now connected to an Agent instead. + app := newConnectSetupApp(t, s, "agent") + out, err = runAgentConnect(t, app) + require.NoError(t, err, out) + + out, err = runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent")) + require.Error(t, err, out) + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, output.CodeAuth, apiErr.Code) + after, err := os.ReadFile(connectSetupPath(t, "agent")) + require.NoError(t, err) + assert.Equal(t, before, after) +} + +// A route kept from connect.json whose directory has gone makes a rerun not +// ready, and connect.json is left as it was. +func TestConnectSetupRechecksRetainedRoutes(t *testing.T) { + s := startConnectSetupServer(t) + repo := filepath.Join(t.TempDir(), "repo") + require.NoError(t, os.Mkdir(repo, 0o700)) + out, err := runConnectSetupCmd(t, connectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupOperatorPerson), fmt.Sprintf("--route=%d=%s", setupProject, repo)) + require.NoError(t, err, out) + before, err := os.ReadFile(connectSetupPath(t, "agent")) + require.NoError(t, err) + + require.NoError(t, os.Remove(repo)) + out, err = runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--concurrency", "3") + require.Error(t, err, out) + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, codeNotReady, apiErr.Code) + assert.Contains(t, apiErr.Message, "no longer usable") + after, err := os.ReadFile(connectSetupPath(t, "agent")) + require.NoError(t, err) + assert.Equal(t, before, after) +} + +// Setup checks one credential: when another process stores a different one +// under the profile while the checks run, nothing is written for either. +func TestConnectSetupRefusesACredentialReplacedDuringTheChecks(t *testing.T) { + s := startConnectSetupServer(t) + bareSetupApp(t, s, "bot") + storeConnectProfile(t, s, "bot", setupBotToken) + s.duringMint = func() { + cfg := config.Default() + cfg.BaseURL = s.srv.URL + cfg.ActiveProfile = "bot" + mgr := auth.NewManager(cfg, s.srv.Client()) + mgr.SetStore(auth.NewStore(config.GlobalConfigDir())) + require.NoError(t, mgr.ImportToken(context.Background(), setupOperatorToken, "full", "", "", time.Now().Add(24*time.Hour))) + } + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "bot"), + "--operator", fmt.Sprint(setupOperatorPerson), "--expect-identity", fmt.Sprint(setupBotIdentity), routeArg(t)) + require.Error(t, err, out) + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, output.CodeAuth, apiErr.Code) + assert.Contains(t, apiErr.Message, "changed while setup was checking it") + assertNotWritten(t, "bot") +} + +// What setup writes names the identity the profile's credential +// authenticates as, and connect.json's own check refuses any other. That is +// what makes a credential replaced after setup harmless: the connector +// stops instead of acting as the wrong agent. +func TestConnectSetupWritesAPolicyBoundToTheCredential(t *testing.T) { + s := startConnectSetupServer(t) + out, err := runConnectSetupCmd(t, connectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupOperatorPerson), routeArg(t)) + require.NoError(t, err, out) + + f, err := setup.Load(connectSetupPath(t, "agent")) + require.NoError(t, err) + require.NoError(t, f.VerifyAgent(setup.KindAgent, setupAgentPerson, 0)) + + // The profile is pointed at someone else afterwards. + assert.Error(t, f.VerifyAgent(setup.KindBotUser, setupBotPerson, setupBotIdentity)) + assert.Error(t, f.VerifyAgent(setup.KindAgent, 777, 0)) + + // And setup itself refuses the next run for the same reason. + storeConnectProfile(t, s, "agent", setupBotToken) + out, err = runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--expect-identity", fmt.Sprint(setupBotIdentity)) + require.Error(t, err, out) + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, output.CodeAuth, apiErr.Code) +} + +// A host that cannot lock the credential at all is refused: setup never +// runs its last check and its write unsynchronized, and changes nothing. +func TestConnectSetupRefusesWhenTheHostCannotLock(t *testing.T) { + s := startConnectSetupServer(t) + connectSetupApp(t, s, "agent") + locks := filepath.Join(config.GlobalConfigDir(), "locks") + require.NoError(t, os.MkdirAll(locks, 0o700)) + entries, err := os.ReadDir(locks) + require.NoError(t, err) + for _, e := range entries { + require.NoError(t, os.Remove(filepath.Join(locks, e.Name()))) + } + require.NoError(t, os.Chmod(locks, 0o500)) + t.Cleanup(func() { _ = os.Chmod(locks, 0o700) }) + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupOperatorPerson), routeArg(t)) + require.Error(t, err, out) + assert.Contains(t, err.Error(), "cannot lock") + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, output.CodeLockUnavailable, apiErr.Code) + assertNotWritten(t, "agent") +} + +// A rerun does not rebind connect.json to another bot because a flag says +// so: that is a deliberate act, and setup says how to make it. +func TestConnectSetupRefusesToRebindToAnotherIdentity(t *testing.T) { + s := startConnectSetupServer(t) + bareSetupApp(t, s, "bot") + storeConnectProfile(t, s, "bot", setupBotToken) + args := []string{"--operator", fmt.Sprint(setupOperatorPerson), "--expect-identity", fmt.Sprint(setupBotIdentity)} + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "bot"), append(args, routeArg(t))...) + require.NoError(t, err, out) + before, err := os.ReadFile(connectSetupPath(t, "bot")) + require.NoError(t, err) + + out, err = runConnectSetupCmd(t, newConnectSetupApp(t, s, "bot"), "--expect-identity", "999999") + require.Error(t, err, out) + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, output.CodeAuth, apiErr.Code) + assert.Contains(t, apiErr.Message, "remove") + after, err := os.ReadFile(connectSetupPath(t, "bot")) + require.NoError(t, err) + assert.Equal(t, before, after) +} + +// Contention is retryable, and named as busy wherever it comes from. +func TestConnectSetupBusyIsRetryable(t *testing.T) { + s := startConnectSetupServer(t) + connectSetupApp(t, s, "agent") + unlock, err := setup.Lock(connectSetupPath(t, "agent")) + require.NoError(t, err) + t.Cleanup(unlock) + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupOperatorPerson), routeArg(t)) + require.Error(t, err, out) + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, output.CodeBusy, apiErr.Code) + assert.True(t, apiErr.Retryable, "machine consumers read retryable, not the code") +} + +// A host that cannot take the setup lock is lock_unavailable, not usage. +func TestConnectSetupClassifiesAnUnlockableHost(t *testing.T) { + s := startConnectSetupServer(t) + out, err := runConnectSetupCmd(t, connectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupOperatorPerson), routeArg(t)) + require.NoError(t, err, out) + // A lock file this user cannot open at all: setup must not fall back to + // running without it. + lockPath := filepath.Join(filepath.Dir(connectSetupPath(t, "agent")), ".connect.lock") + require.NoError(t, os.WriteFile(lockPath, nil, 0o600)) + require.NoError(t, os.Chmod(lockPath, 0o000)) + t.Cleanup(func() { _ = os.Chmod(lockPath, 0o600) }) + + out, err = runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--concurrency", "3") + require.Error(t, err, out) + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, output.CodeLockUnavailable, apiErr.Code) +} + +// A person who stops setup gets the interruption, not a verdict on the +// connector's readiness. +func TestConnectSetupPropagatesCancellation(t *testing.T) { + s := startConnectSetupServer(t) + connectSetupApp(t, s, "agent") + ctx, cancel := context.WithCancel(context.Background()) + s.duringMint = cancel + + out, err := runConnectSetupCmdIn(ctx, t, newConnectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupOperatorPerson), routeArg(t)) + require.Error(t, err, out) + assert.ErrorIs(t, err, context.Canceled) + assertNotWritten(t, "agent") +} + +// Stopping setup while it waits for the credential lock is an +// interruption, not a usage error. +func TestConnectSetupClassifiesCancellationUnderTheLock(t *testing.T) { + for name, err := range map[string]error{ + "canceled": context.Canceled, + "deadline": context.DeadlineExceeded, + } { + t.Run(name, func(t *testing.T) { + got := classifyWriteError("agent", fmt.Errorf("under the lock: %w", err)) + assert.ErrorIs(t, got, err) + var apiErr *output.Error + if errors.As(got, &apiErr) { + assert.NotEqual(t, output.CodeUsage, apiErr.Code) + } + }) + } +} diff --git a/internal/commands/doctor.go b/internal/commands/doctor.go index 690e2f1ff..1c758514b 100644 --- a/internal/commands/doctor.go +++ b/internal/commands/doctor.go @@ -1192,6 +1192,12 @@ func pluralize(n int, singular, plural string) string { // renderDoctorStyled outputs a human-friendly styled format for TTY. func renderDoctorStyled(w io.Writer, result *DoctorResult) { + renderChecksStyled(w, "Basecamp CLI Doctor", result) +} + +// renderChecksStyled renders a titled list of checks and their summary, for +// every command that reports checks the way doctor does. +func renderChecksStyled(w io.Writer, title string, result *DoctorResult) { r := output.NewRenderer(w, false) // Status icon styles @@ -1218,7 +1224,7 @@ func renderDoctorStyled(w io.Writer, result *DoctorResult) { } fmt.Fprintln(w) - fmt.Fprintln(w, r.Summary.Render("Basecamp CLI Doctor")) + fmt.Fprintln(w, r.Summary.Render(title)) fmt.Fprintln(w) for _, check := range result.Checks { diff --git a/internal/connector/setup/apply.go b/internal/connector/setup/apply.go new file mode 100644 index 000000000..105db6eba --- /dev/null +++ b/internal/connector/setup/apply.go @@ -0,0 +1,176 @@ +package setup + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "slices" + "strings" + "time" + + "github.com/basecamp/basecamp-cli/internal/connector/admission" +) + +// Changes is what one setup run asks to change. A zero field leaves the +// file's value alone, so setup can be run again to add a route without +// restating everything else. +type Changes struct { + // Trust is the trust mode, "" to keep the file's. + Trust admission.TrustMode + // Allow replaces the allowlist when non-empty; it implies allowlist mode. + Allow []int64 + + // Routes maps a project (bucket) id to the directory its work runs in, + // as the operator typed it; ResolveDir makes it the approved entry. + Routes map[int64]string + // Classes sets a routed project's class; an empty class clears it. + Classes map[int64]string + // WatchCompletions turns watch_completions on (true) or off (false) for + // a routed project. + WatchCompletions map[int64]bool + // Remove drops projects' routes. + Remove []int64 + + Driver string + Concurrency int + Deadline time.Duration + // Worktrees is nil to keep the file's value. + Worktrees *bool +} + +// Apply returns f with ch applied. f is not modified. Everything that can +// be refused without the network is refused here; the result still has to +// pass Validate, which Save runs. +func Apply(f File, ch Changes) (File, error) { + out := f + out.Projects = make(map[int64]admission.Route, len(f.Projects)) + for id, r := range f.Projects { + out.Projects[id] = r + } + out.Trust.AllowlistIDs = slices.Clone(f.Trust.AllowlistIDs) + + if err := applyTrust(&out.Trust, ch); err != nil { + return File{}, err + } + + for _, id := range ch.Remove { + if _, routed := ch.Routes[id]; routed { + return File{}, fmt.Errorf("project %d is both routed and removed in one run", id) + } + delete(out.Projects, id) // removing a route that is not there is already done + } + for id, raw := range ch.Routes { + if id <= 0 { + return File{}, fmt.Errorf("route: %d is not a project id", id) + } + dir, err := ResolveDir(raw) + if err != nil { + return File{}, fmt.Errorf("route for project %d: %w", id, err) + } + r := out.Projects[id] + r.Path = dir + out.Projects[id] = r + } + for id, class := range ch.Classes { + r, ok := out.Projects[id] + if !ok { + return File{}, fmt.Errorf("class for project %d: the project has no route; add one with --route %d=", id, id) + } + if class != "" && !ValidClass(class) { + return File{}, fmt.Errorf("class %q for project %d: use lowercase letters, digits, - or _, at most 40", class, id) + } + r.Class = class + out.Projects[id] = r + } + for id, on := range ch.WatchCompletions { + r, ok := out.Projects[id] + if !ok { + return File{}, fmt.Errorf("watch_completions for project %d: the project has no route; add one with --route %d=", id, id) + } + r.WatchCompletions = on + out.Projects[id] = r + } + + if ch.Driver != "" { + out.Driver = ch.Driver + } + if ch.Concurrency != 0 { + out.Concurrency = ch.Concurrency + } + if ch.Deadline != 0 { + out.Deadline = Duration(ch.Deadline) + } + if ch.Worktrees != nil { + out.Worktrees = *ch.Worktrees + } + return out, nil +} + +func applyTrust(t *admission.Trust, ch Changes) error { + mode := ch.Trust + if len(ch.Allow) > 0 { + if mode != "" && mode != admission.TrustAllowlist { + return fmt.Errorf("--allow names people to trust, which is allowlist mode, not %q", mode) + } + mode = admission.TrustAllowlist + } + if mode == "" { + return nil + } + switch mode { + case admission.TrustOperator, admission.TrustProject: + // Leaving allowlist mode drops the list: admission refuses a list + // outside its mode, and a list kept "for later" is trust nobody + // can see. + t.AllowlistIDs = nil + case admission.TrustAllowlist: + if len(ch.Allow) > 0 { + ids := slices.Clone(ch.Allow) + slices.Sort(ids) + t.AllowlistIDs = slices.Compact(ids) + } + if len(t.AllowlistIDs) == 0 { + return errors.New("allowlist mode needs at least one --allow ") + } + default: + return fmt.Errorf("unknown trust mode %q: use operator, allowlist or project", mode) + } + t.Mode = mode + return nil +} + +// ResolveDir turns a route directory as typed into the entry connect.json +// approves: ~ expanded, absolute, symlinks resolved, and required to be an +// existing directory. Resolving symlinks means the approved entry names the +// directory work will really run in, so a link retargeted later does not +// move the route with it. +func ResolveDir(raw string) (string, error) { + if strings.TrimSpace(raw) == "" { + return "", errors.New("the directory is empty") + } + path := raw + if path == "~" || strings.HasPrefix(path, "~/") { + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("expand ~: %w", err) + } + path = filepath.Join(home, strings.TrimPrefix(path, "~")) + } + abs, err := filepath.Abs(path) + if err != nil { + return "", err + } + resolved, err := filepath.EvalSymlinks(abs) + if err != nil { + return "", fmt.Errorf("%s: %w", abs, err) + } + info, err := os.Stat(resolved) + if err != nil { + return "", err + } + if !info.IsDir() { + return "", fmt.Errorf("%s is not a directory", resolved) + } + return filepath.Clean(resolved), nil +} diff --git a/internal/connector/setup/apply_test.go b/internal/connector/setup/apply_test.go new file mode 100644 index 000000000..e60c2097d --- /dev/null +++ b/internal/connector/setup/apply_test.go @@ -0,0 +1,155 @@ +//go:build unix + +package setup + +import ( + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/connector/admission" +) + +func TestApplyKeepsWhatIsNotPassed(t *testing.T) { + f := validFile(t) + f.Trust = admission.Trust{Mode: admission.TrustAllowlist, OperatorID: operatorID, AllowlistIDs: []int64{7}} + f.Worktrees = true + + out, err := Apply(f, Changes{}) + require.NoError(t, err) + assert.Equal(t, f, out) +} + +func TestApplyDoesNotModifyItsInput(t *testing.T) { + f := validFile(t) + before := len(f.Projects) + _, err := Apply(f, Changes{Remove: []int64{projectID}, Allow: []int64{9}}) + require.NoError(t, err) + assert.Len(t, f.Projects, before) + assert.Empty(t, f.Trust.AllowlistIDs) +} + +func TestApplyTrust(t *testing.T) { + base := validFile(t) + + t.Run("allow implies allowlist, sorted and deduplicated", func(t *testing.T) { + out, err := Apply(base, Changes{Allow: []int64{9, 3, 9}}) + require.NoError(t, err) + assert.Equal(t, admission.TrustAllowlist, out.Trust.Mode) + assert.Equal(t, []int64{3, 9}, out.Trust.AllowlistIDs) + }) + t.Run("allow contradicting the mode is refused", func(t *testing.T) { + _, err := Apply(base, Changes{Trust: admission.TrustProject, Allow: []int64{9}}) + assert.Error(t, err) + }) + t.Run("allowlist with nobody on it is refused", func(t *testing.T) { + _, err := Apply(base, Changes{Trust: admission.TrustAllowlist}) + assert.Error(t, err) + }) + t.Run("leaving allowlist drops the list", func(t *testing.T) { + f := base + f.Trust = admission.Trust{Mode: admission.TrustAllowlist, OperatorID: operatorID, AllowlistIDs: []int64{7}} + out, err := Apply(f, Changes{Trust: admission.TrustProject}) + require.NoError(t, err) + assert.Equal(t, admission.TrustProject, out.Trust.Mode) + assert.Empty(t, out.Trust.AllowlistIDs) + _, err = out.Policy(agentID) + assert.NoError(t, err, "admission accepts the result") + }) + t.Run("the operator is untouched", func(t *testing.T) { + out, err := Apply(base, Changes{Trust: admission.TrustProject}) + require.NoError(t, err) + assert.Equal(t, operatorID, out.Trust.OperatorID) + }) +} + +func TestApplyRoutes(t *testing.T) { + base := validFile(t) + + t.Run("a route resolves to the real directory", func(t *testing.T) { + target := t.TempDir() + link := filepath.Join(t.TempDir(), "app") + require.NoError(t, os.Symlink(target, link)) + + out, err := Apply(base, Changes{Routes: map[int64]string{777: link}}) + require.NoError(t, err) + want, err := filepath.EvalSymlinks(target) + require.NoError(t, err) + assert.Equal(t, want, out.Projects[777].Path, "the approved entry names where work really runs") + }) + t.Run("rerouting keeps class and watch_completions", func(t *testing.T) { + dir := t.TempDir() + out, err := Apply(base, Changes{Routes: map[int64]string{projectID: dir}}) + require.NoError(t, err) + assert.Equal(t, "internal", out.Projects[projectID].Class) + assert.True(t, out.Projects[projectID].WatchCompletions) + }) + t.Run("a directory that does not exist is refused", func(t *testing.T) { + _, err := Apply(base, Changes{Routes: map[int64]string{777: filepath.Join(t.TempDir(), "missing")}}) + assert.Error(t, err) + }) + t.Run("a file is not a directory", func(t *testing.T) { + file := filepath.Join(t.TempDir(), "f") + require.NoError(t, os.WriteFile(file, nil, 0o600)) + _, err := Apply(base, Changes{Routes: map[int64]string{777: file}}) + assert.Error(t, err) + }) + t.Run("class and watch_completions need a route", func(t *testing.T) { + _, err := Apply(base, Changes{Classes: map[int64]string{777: "internal"}}) + assert.Error(t, err) + _, err = Apply(base, Changes{WatchCompletions: map[int64]bool{777: true}}) + assert.Error(t, err) + }) + t.Run("class and watch_completions apply with a new route", func(t *testing.T) { + out, err := Apply(base, Changes{ + Routes: map[int64]string{777: t.TempDir()}, + Classes: map[int64]string{777: "client-work"}, + WatchCompletions: map[int64]bool{777: true, projectID: false}, + }) + require.NoError(t, err) + assert.Equal(t, "client-work", out.Projects[777].Class) + assert.True(t, out.Projects[777].WatchCompletions) + assert.False(t, out.Projects[projectID].WatchCompletions) + }) + t.Run("an invalid class is refused", func(t *testing.T) { + _, err := Apply(base, Changes{Classes: map[int64]string{projectID: "Has Spaces"}}) + assert.Error(t, err) + }) + t.Run("remove", func(t *testing.T) { + out, err := Apply(base, Changes{Remove: []int64{projectID}}) + require.NoError(t, err) + assert.NotContains(t, out.Projects, projectID) + + again, err := Apply(out, Changes{Remove: []int64{projectID}}) + require.NoError(t, err, "removing a route that is already gone is idempotent") + assert.Equal(t, out.Projects, again.Projects) + _, err = Apply(base, Changes{Remove: []int64{projectID}, Routes: map[int64]string{projectID: t.TempDir()}}) + assert.Error(t, err, "routing and removing one project in one run") + }) +} + +func TestApplyDispatchSettings(t *testing.T) { + on := true + out, err := Apply(validFile(t), Changes{Driver: DriverACP, Concurrency: 4, Deadline: 90 * time.Minute, Worktrees: &on}) + require.NoError(t, err) + assert.Equal(t, DriverACP, out.Driver) + assert.Equal(t, 4, out.Concurrency) + assert.Equal(t, Duration(90*time.Minute), out.Deadline) + assert.True(t, out.Worktrees) +} + +func TestResolveDirExpandsHome(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + require.NoError(t, os.Mkdir(filepath.Join(home, "work"), 0o700)) + + got, err := ResolveDir("~/work") + require.NoError(t, err) + want, err := filepath.EvalSymlinks(filepath.Join(home, "work")) + require.NoError(t, err) + assert.Equal(t, want, got) +} diff --git a/internal/connector/setup/checks.go b/internal/connector/setup/checks.go new file mode 100644 index 000000000..834cbb1c8 --- /dev/null +++ b/internal/connector/setup/checks.go @@ -0,0 +1,385 @@ +package setup + +import ( + "context" + "errors" + "fmt" + "net/http" + "slices" + "time" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + + "github.com/basecamp/basecamp-cli/internal/connector/admission" + "github.com/basecamp/basecamp-cli/internal/richtext" +) + +// PersonableAgent is the personable type Basecamp gives an Agent person. +const PersonableAgent = "Agent" + +// Check statuses, the doctor command's vocabulary. +const ( + StatusPass = "pass" + StatusFail = "fail" + StatusWarn = "warn" + StatusSkip = "skip" +) + +// Check is one setup check's result. +type Check struct { + Name string `json:"name"` + Status string `json:"status"` + Message string `json:"message"` + Hint string `json:"hint,omitempty"` +} + +// Person is the part of a Basecamp person the checks look at. +type Person struct { + ID int64 + Name string + PersonableType string + Client bool +} + +// Reader is what the checks read, as the agent, in the agent's account. +type Reader interface { + // Me is the person the credential authenticates as. + Me(ctx context.Context) (Person, error) + // Person reads someone else in the account. + Person(ctx context.Context, id int64) (Person, error) + // MintStreamTicket mints a stream ticket and throws it away. The ticket + // is a bearer credential: an implementation must neither return, log nor + // keep it. + MintStreamTicket(ctx context.Context) error + // Project reads a project. + Project(ctx context.Context, id int64) error + // ProjectPeople reads a project's people. + ProjectPeople(ctx context.Context, id int64) error +} + +// ErrMalformedTicket reports a mint that answered without a ticket or URL. +var ErrMalformedTicket = errors.New("the stream ticket response carries no ticket, no URL, or no lifetime within bounds") + +// SDKReader is Reader over an SDK account client. Build the client without +// request hooks or a debug logger: they log request URLs, and a feed URL can +// carry a position or a ticket. +type SDKReader struct { + Client *basecamp.AccountClient +} + +// MaxTicketLifetime bounds a ticket's reported lifetime. The server issues +// about two minutes; a lifetime past an hour is not one the contract allows. +const MaxTicketLifetime = time.Hour + +// Me implements Reader. +func (r SDKReader) Me(ctx context.Context) (Person, error) { + p, err := r.Client.People().Me(ctx) + if err != nil { + return Person{}, err + } + return fromSDK(p), nil +} + +// Person implements Reader. +func (r SDKReader) Person(ctx context.Context, id int64) (Person, error) { + p, err := r.Client.People().Get(ctx, id) + if err != nil { + return Person{}, err + } + return fromSDK(p), nil +} + +// MintStreamTicket implements Reader through the SDK's stream-ticket +// operation, and accepts exactly what the connector runtime's minter +// accepts (eventfeed's liveMinter refuses a ticket with no credential, no +// URL or no positive lifetime), with the lifetime also bounded above. The +// ticket is dropped on return. +func (r SDKReader) MintStreamTicket(ctx context.Context) error { + ticket, err := r.Client.EventFeed().CreateStreamTicket(ctx) + if err != nil { + return err + } + if !UsableTicket(ticket) { + return ErrMalformedTicket + } + return nil +} + +// UsableTicket reports whether a minted ticket is one the connector could +// dial: a credential, a URL, and a lifetime above zero and at most +// MaxTicketLifetime. +func UsableTicket(t *basecamp.StreamTicket) bool { + // Compared in whole seconds: converting to a Duration first would + // overflow for a huge value and could wrap back under the bound. + return t != nil && t.Ticket != "" && t.URL != "" && + t.ExpiresIn > 0 && int64(t.ExpiresIn) <= int64(MaxTicketLifetime/time.Second) +} + +// Project implements Reader. +func (r SDKReader) Project(ctx context.Context, id int64) error { + _, err := r.Client.Projects().Get(ctx, id) + return err +} + +// ProjectPeople implements Reader. +func (r SDKReader) ProjectPeople(ctx context.Context, id int64) error { + _, err := r.Client.People().ListProjectPeople(ctx, id, nil) + return err +} + +func fromSDK(p *basecamp.Person) Person { + if p == nil { + return Person{} + } + return Person{ID: p.ID, Name: p.Name, PersonableType: p.PersonableType, Client: p.Client} +} + +// httpStatus is the status a failed read answered with, 0 when it did not +// reach an answer. +func httpStatus(err error) int { + var apiErr *basecamp.Error + if errors.As(err, &apiErr) { + return apiErr.HTTPStatus + } + return 0 +} + +func refused(err error) bool { + switch httpStatus(err) { + case http.StatusForbidden, http.StatusNotFound: + return true + } + return false +} + +// agentReadsRefused is what setup says when Basecamp refuses a read admission +// makes to an Agent identity. The connector would take events off the feed +// and block every one of them on a read it can never make, which from the +// outside looks like an agent that ignores its mentions. +const agentReadsRefused = "Basecamp refuses this read to an Agent identity today, and admission makes it for every event: " + + "the connector would see mentions and block each one on a read it cannot make" + +const agentReadsRefusedHint = "Until Basecamp allows Agent identities these reads, run the connector as a bot user: " + + "basecamp connect setup -P --expect-identity " + +// ScopeCheck classifies the scope a credential was granted, as stored from +// the server's answer. Only "full" lets the agent reply and acknowledge. +// Launchpad grants carry no scope and are full access. Anything else — +// read, empty on a credential that should carry one, or a value this CLI +// does not know — is insufficient, never assumed full. +func ScopeCheck(oauthType, granted string) Check { + c := Check{Name: "Scope"} + switch { + case granted == "full", granted == "" && oauthType == "launchpad": + c.Status, c.Message = StatusPass, "Full access: the agent can reply and acknowledge" + case granted == "": + c.Status, c.Message = StatusFail, "The credential records no granted scope, so it is not known to allow replies" + c.Hint = "Reconnect with full access." + default: + c.Status = StatusFail + c.Message = fmt.Sprintf("The credential was granted %q, not full access: the agent could not reply or acknowledge", richtext.SanitizeSingleLine(granted)) + c.Hint = "Reconnect with full access." + } + return c +} + +// TicketCheck mints a stream ticket as the agent. +func TicketCheck(ctx context.Context, r Reader, kind string) Check { + c := Check{Name: "Stream ticket"} + err := r.MintStreamTicket(ctx) + switch { + case err == nil: + c.Status, c.Message = StatusPass, "The agent can mint a ticket for the account event feed" + case refused(err) || httpStatus(err) == http.StatusUnauthorized: + c.Status = StatusFail + c.Message = fmt.Sprintf("Basecamp refused the ticket mint (HTTP %d): the connector could not open the account feed", httpStatus(err)) + if kind == KindAgent { + c.Hint = "The account event feed has to be enabled for this agent's account and principal." + } else { + c.Hint = "The account event feed has to be enabled for this account." + } + default: + c.Status, c.Message = StatusFail, "The ticket mint failed: "+ErrorText(err) + } + return c +} + +// Trust is who connect.json says may drive the agent, as setup is about to +// write it, and how each person in it came to be named. +type Trust struct { + // Operator is the operator. When OperatorProfile is set, it is the person + // that profile's own credential read back, which proves who they are. + Operator Person + OperatorProfile string + // Allowlist is every allowlisted Person id the file will hold. + Allowlist []int64 + // Recorded is the trust connect.json already holds for this same agent + // and account. It is taken as the owner's own record — the file is + // private to them, and setup verified it if setup wrote it — so a person + // in it who cannot be re-read now is a warning, not a refusal. A read + // that answers still decides. The zero value records nobody. + Recorded admission.Trust +} + +// VerifyTrust is the one check of the trust anchor every setup goes +// through before connect.json is written. A failed result means nothing may +// be written. The properties, each checked here: +// +// 1. The operator and every allowlisted id are Person ids (positive). +// 2. None of them is the agent itself: the agent's own id never authorizes. +// 3. Each was verified as a person in the agent's account: the operator +// by their own profile's credential, or else by reading them; every +// allowlisted id by reading it. A read that is refused or fails refuses +// the id, unless connect.json already records that same person in that +// same role, which was verified when it was written. +// 4. None of them reads back as an Agent person: agents never authorize. +// 5. None of them reads back as a client of the account. +// 6. A read answers for the id that was asked for. +// +// people reads persons in the account; pass a reader built on the operator's +// own credential when there is one, since Basecamp refuses person reads to +// an Agent identity today. +func VerifyTrust(ctx context.Context, people Reader, t Trust, agentID int64) []Check { + ids := slices.Clone(t.Allowlist) + slices.Sort(ids) + ids = slices.Compact(ids) + checks := make([]Check, 0, 1+len(ids)) + checks = append(checks, verifyPerson(ctx, people, "Operator", t.Operator, agentID, t.OperatorProfile, t.Recorded.OperatorID == t.Operator.ID && t.Operator.ID > 0)) + for _, id := range ids { + name := fmt.Sprintf("Allowlist %d", id) + checks = append(checks, verifyPerson(ctx, people, name, Person{ID: id}, agentID, "", slices.Contains(t.Recorded.AllowlistIDs, id))) + } + return checks +} + +func verifyPerson(ctx context.Context, r Reader, name string, p Person, agentID int64, fromProfile string, recorded bool) Check { + c := Check{Name: name} + switch { + case p.ID <= 0: + c.Status, c.Message = StatusFail, fmt.Sprintf("%d is not a Person id", p.ID) + return c + case p.ID == agentID: + c.Status, c.Message = StatusFail, fmt.Sprintf("Person %d is the agent itself; the agent's own id never authorizes", p.ID) + return c + } + source := "" + if fromProfile != "" { + source = fmt.Sprintf(", the identity of profile %q", fromProfile) + } else { + read, err := r.Person(ctx, p.ID) + switch { + case err != nil && recorded: + c.Status = StatusWarn + c.Message = fmt.Sprintf("Person %d, already recorded in connect.json, could not be re-read: %s", p.ID, ErrorText(err)) + c.Hint = "It was verified when it was recorded. To verify the operator again, pass --operator-profile ." + return c + case err != nil: + c.Status = StatusFail + c.Message = fmt.Sprintf("Person %d could not be read (%s), so it cannot be verified", p.ID, ErrorText(err)) + c.Hint = "Pass --operator-profile : people are read through the operator's own credential, which Basecamp does not refuse." + return c + case read.ID != p.ID: + c.Status, c.Message = StatusFail, fmt.Sprintf("Reading person %d answered with person %d", p.ID, read.ID) + return c + } + p = read + } + switch { + case p.PersonableType == PersonableAgent: + c.Status, c.Message = StatusFail, fmt.Sprintf("Person %d is an Agent; agents never authorize", p.ID) + case p.Client: + c.Status, c.Message = StatusFail, fmt.Sprintf("Person %d is a client of the account; only members are trusted", p.ID) + default: + c.Status = StatusPass + c.Message = fmt.Sprintf("Person %d, %s%s", p.ID, richtext.SanitizeSingleLine(p.Name), source) + } + return c +} + +// RouteChecks reads each routed project the way admission will, as the +// agent: the project, and its people (project trust mode's membership read). +func RouteChecks(ctx context.Context, r Reader, f File) []Check { + if len(f.Projects) == 0 { + return []Check{{ + Name: "Routes", + Status: StatusFail, + Message: "No project is routed: every mention would get a holding reply and no work", + Hint: "Add one: basecamp connect setup -P " + f.Profile + " --route =", + }} + } + ids := make([]int64, 0, len(f.Projects)) + for id := range f.Projects { + ids = append(ids, id) + } + slices.Sort(ids) + + checks := make([]Check, 0, len(ids)) + for _, id := range ids { + checks = append(checks, routeCheck(ctx, r, f.Agent.Kind, id, f.Projects[id].Path)) + } + return checks +} + +func routeCheck(ctx context.Context, r Reader, kind string, id int64, path string) Check { + c := Check{Name: fmt.Sprintf("Project %d", id)} + // The directory is checked as a new route's is, whether the route was + // passed now or kept from connect.json: it must still resolve to itself, + // an existing directory. + if resolved, err := ResolveDir(path); err != nil || resolved != path { + c.Status = StatusFail + c.Message = "The route's directory is no longer usable: " + richtext.SanitizeSingleLine(path) + c.Hint = fmt.Sprintf("Route the project again: --route %d=, or remove it: --remove-route %d.", id, id) + return c + } + for _, read := range []struct { + what string + run func(context.Context, int64) error + }{ + {"the project", r.Project}, + {"the project's people", r.ProjectPeople}, + } { + err := read.run(ctx, id) + if err == nil { + continue + } + c.Status = StatusFail + switch { + case refused(err) && kind == KindAgent: + c.Message = fmt.Sprintf("Reading %s was refused (HTTP %d). %s", read.what, httpStatus(err), agentReadsRefused) + c.Hint = "If the agent is not on this project, add it there. Otherwise: " + agentReadsRefusedHint + case refused(err): + c.Message = fmt.Sprintf("Reading %s was refused (HTTP %d): the agent cannot see project %d", read.what, httpStatus(err), id) + c.Hint = "Add the agent to the project in Basecamp, then run setup again." + default: + c.Message = fmt.Sprintf("Reading %s failed: %s", read.what, ErrorText(err)) + } + return c + } + c.Status = StatusPass + c.Message = "Readable by the agent, routed to " + richtext.SanitizeSingleLine(path) + return c +} + +// ErrorText is the one way setup turns a read's error into text, for every +// check and every refusal. An HTTP answer is reduced to its status: the SDK +// builds an error's message from the response body, which the server +// controls, and a ticket mint's body can carry the ticket and the feed URL. +// Only an error that never reached a server keeps its text, reduced to one +// line. +func ErrorText(err error) string { + if err == nil { + return "" + } + var apiErr *basecamp.Error + errors.As(err, &apiErr) + if apiErr != nil { + if apiErr.HTTPStatus != 0 { + return fmt.Sprintf("HTTP %d", apiErr.HTTPStatus) + } + if apiErr.Code != "" { + return "request failed (" + richtext.SanitizeSingleLine(apiErr.Code) + ")" + } + return "request failed" + } + return richtext.SanitizeSingleLine(err.Error()) +} diff --git a/internal/connector/setup/checks_test.go b/internal/connector/setup/checks_test.go new file mode 100644 index 000000000..2bf8e0453 --- /dev/null +++ b/internal/connector/setup/checks_test.go @@ -0,0 +1,342 @@ +//go:build unix + +package setup + +import ( + "context" + "encoding/json" + "fmt" + "math" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "sync" + "testing" + "time" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/connector/admission" +) + +// fakeReader answers each read from a table; an absent entry succeeds. +type fakeReader struct { + people map[int64]Person + personErr error + mintErr error + projectErr map[int64]error + projectPplErr map[int64]error +} + +func (f *fakeReader) Me(context.Context) (Person, error) { return f.people[agentID], nil } +func (f *fakeReader) Person(_ context.Context, id int64) (Person, error) { + if f.personErr != nil { + return Person{}, f.personErr + } + return f.people[id], nil +} +func (f *fakeReader) MintStreamTicket(context.Context) error { return f.mintErr } +func (f *fakeReader) Project(_ context.Context, id int64) error { + return f.projectErr[id] +} +func (f *fakeReader) ProjectPeople(_ context.Context, id int64) error { + return f.projectPplErr[id] +} + +func status(code int) error { + return &basecamp.Error{Code: "api_error", Message: http.StatusText(code), HTTPStatus: code} +} + +// The known bc3 limitation: an Agent identity is refused the reads admission +// makes. Setup has to say so, rather than write a file the connector would +// start on and then block every event against. +func TestRouteChecksNameTheAgentReadRefusal(t *testing.T) { + f := validFile(t) + r := &fakeReader{projectErr: map[int64]error{projectID: status(http.StatusForbidden)}} + + checks := RouteChecks(context.Background(), r, f) + require.Len(t, checks, 2) + byName := map[string]Check{} + for _, c := range checks { + byName[c.Name] = c + } + + refused := byName[fmt.Sprintf("Project %d", projectID)] + assert.Equal(t, StatusFail, refused.Status) + assert.Contains(t, refused.Message, "Agent identity") + assert.Contains(t, refused.Hint, "--expect-identity", "the way to run today is named") + + assert.Equal(t, StatusPass, byName[fmt.Sprintf("Project %d", otherProj)].Status) +} + +func TestRouteChecksReadProjectPeopleToo(t *testing.T) { + f := validFile(t) + r := &fakeReader{projectPplErr: map[int64]error{otherProj: status(http.StatusForbidden)}} + + var failed []Check + for _, c := range RouteChecks(context.Background(), r, f) { + if c.Status == StatusFail { + failed = append(failed, c) + } + } + require.Len(t, failed, 1) + assert.Equal(t, fmt.Sprintf("Project %d", otherProj), failed[0].Name) + assert.Contains(t, failed[0].Message, "people") +} + +func TestRouteChecksForABotUserSayToAddTheAgent(t *testing.T) { + f := validFile(t) + f.Agent = Agent{PersonID: agentID, Kind: KindBotUser, IdentityID: 99} + r := &fakeReader{projectErr: map[int64]error{projectID: status(http.StatusNotFound)}} + + for _, c := range RouteChecks(context.Background(), r, f) { + if c.Status != StatusFail { + continue + } + assert.NotContains(t, c.Message, "Agent identity", "a bot user is not an Agent") + assert.Contains(t, c.Hint, "Add the agent to the project") + return + } + t.Fatal("the refused project did not fail") +} + +func TestRouteChecksWarnWithNoRoutes(t *testing.T) { + f := validFile(t) + f.Projects = map[int64]admission.Route{} + checks := RouteChecks(context.Background(), &fakeReader{}, f) + require.Len(t, checks, 1) + assert.Equal(t, StatusFail, checks[0].Status, "a connector with no route does no work, so it is not ready") +} + +func TestTicketCheck(t *testing.T) { + assert.Equal(t, StatusPass, TicketCheck(context.Background(), &fakeReader{}, KindAgent).Status) + + c := TicketCheck(context.Background(), &fakeReader{mintErr: status(http.StatusForbidden)}, KindAgent) + assert.Equal(t, StatusFail, c.Status) + assert.Contains(t, c.Message, "403") + + c = TicketCheck(context.Background(), &fakeReader{mintErr: ErrMalformedTicket}, KindAgent) + assert.Equal(t, StatusFail, c.Status) +} + +func TestVerifyTrust(t *testing.T) { + ctx := context.Background() + people := map[int64]Person{ + operatorID: {ID: operatorID, Name: "Operator"}, + 7: {ID: 7, PersonableType: PersonableAgent}, + 8: {ID: 8, Client: true}, + 9: {ID: 9, Name: "Member"}, + } + r := &fakeReader{people: people} + forbidden := &fakeReader{personErr: status(http.StatusForbidden)} + statuses := func(checks []Check) []string { + out := make([]string, 0, len(checks)) + for _, c := range checks { + out = append(out, c.Name+"="+c.Status) + } + return out + } + verify := func(r Reader, trust Trust) []string { return statuses(VerifyTrust(ctx, r, trust, agentID)) } + + assert.Equal(t, []string{"Operator=pass"}, verify(r, Trust{Operator: Person{ID: operatorID}})) + assert.Equal(t, []string{"Operator=pass"}, verify(forbidden, Trust{Operator: people[operatorID], OperatorProfile: "me"}), + "an operator proved by their own profile needs no read") + + t.Run("property 1: a Person id", func(t *testing.T) { + assert.Equal(t, []string{"Operator=fail"}, verify(r, Trust{})) + }) + t.Run("property 2: never the agent", func(t *testing.T) { + assert.Equal(t, []string{"Operator=fail"}, verify(r, Trust{Operator: Person{ID: agentID}})) + assert.Equal(t, []string{"Operator=fail"}, verify(r, Trust{Operator: Person{ID: agentID}, OperatorProfile: "me"})) + assert.Equal(t, []string{"Operator=pass", fmt.Sprintf("Allowlist %d=fail", agentID)}, + verify(r, Trust{Operator: Person{ID: operatorID}, Allowlist: []int64{agentID}})) + }) + t.Run("property 3: verified, unless already recorded", func(t *testing.T) { + assert.Equal(t, []string{"Operator=fail"}, verify(forbidden, Trust{Operator: Person{ID: operatorID}})) + assert.Equal(t, []string{"Operator=warn"}, verify(forbidden, Trust{Operator: Person{ID: operatorID}, Recorded: admission.Trust{OperatorID: operatorID}})) + assert.Equal(t, []string{"Operator=fail"}, verify(forbidden, Trust{Operator: Person{ID: 9}, Recorded: admission.Trust{OperatorID: operatorID}}), + "a different operator is a new trust anchor") + + trust := Trust{Operator: people[operatorID], OperatorProfile: "me", Allowlist: []int64{9, 10}} + assert.Equal(t, []string{"Operator=pass", "Allowlist 9=fail", "Allowlist 10=fail"}, verify(forbidden, trust), "allowlisted ids are verified too") + trust.Recorded = admission.Trust{AllowlistIDs: []int64{9}} + assert.Equal(t, []string{"Operator=pass", "Allowlist 9=warn", "Allowlist 10=fail"}, verify(forbidden, trust)) + }) + t.Run("property 4: not an Agent", func(t *testing.T) { + assert.Equal(t, []string{"Operator=fail"}, verify(r, Trust{Operator: Person{ID: 7}})) + assert.Equal(t, []string{"Operator=fail"}, verify(r, Trust{Operator: people[7], OperatorProfile: "me"})) + assert.Equal(t, []string{"Operator=fail"}, verify(r, Trust{Operator: Person{ID: 7}, Recorded: admission.Trust{OperatorID: 7}}), + "recorded is no excuse when the read answers") + assert.Equal(t, []string{"Operator=pass", "Allowlist 7=fail"}, verify(r, Trust{Operator: Person{ID: operatorID}, Allowlist: []int64{7}})) + }) + t.Run("property 5: not a client", func(t *testing.T) { + assert.Equal(t, []string{"Operator=fail"}, verify(r, Trust{Operator: Person{ID: 8}})) + assert.Equal(t, []string{"Operator=pass", "Allowlist 8=fail"}, verify(r, Trust{Operator: Person{ID: operatorID}, Allowlist: []int64{8}})) + }) + t.Run("property 6: the read answers for the id asked", func(t *testing.T) { + liar := &fakeReader{people: map[int64]Person{operatorID: {ID: 9, Name: "Someone else"}}} + assert.Equal(t, []string{"Operator=fail"}, verify(liar, Trust{Operator: Person{ID: operatorID}})) + }) +} + +func TestReportIsReadyOnlyWhenEveryCheckPassed(t *testing.T) { + r := &Report{Written: true} + assert.False(t, r.Ready(), "no check ran") + + r.Add(Check{Name: "a", Status: StatusPass}, Check{Name: "b", Status: StatusWarn}) + assert.True(t, r.Ready()) + + r.Add(Check{Name: "c", Status: ""}) + assert.False(t, r.Ready(), "a check with no status has not passed") + + r = &Report{Written: true} + r.Add(Check{Name: "a", Status: StatusFail}) + assert.False(t, r.Ready()) + + r = &Report{} + r.Add(Check{Name: "a", Status: StatusPass}) + assert.False(t, r.Ready(), "nothing written is not ready") + + r = &Report{Written: true} + r.Add(Check{Name: "a", Status: StatusFail}) + data, err := json.Marshal(r) + require.NoError(t, err) + assert.Contains(t, string(data), `"ready":false`) + assert.Contains(t, string(data), `"written":true`) +} + +// The ticket is a bearer credential. The mint check posts to the account +// feed's ticket endpoint and keeps nothing: not in its result, not in an +// error. It accepts exactly the tickets the connector could dial. +func TestSDKReaderMintsAndDiscardsTheTicket(t *testing.T) { + const ticket = "not-a-real-ticket" + var mu sync.Mutex + var seen []string + answer := "" + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + seen = append(seen, r.Method+" "+r.URL.Path) + mu.Unlock() + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, answer) + })) + t.Cleanup(srv.Close) + client := basecamp.NewClient(&basecamp.Config{BaseURL: srv.URL}, &basecamp.StaticTokenProvider{Token: "t"}).ForAccount("999") + mint := func(body string) error { + answer = body + return SDKReader{Client: client}.MintStreamTicket(context.Background()) + } + url := "wss://example.test/cable?ticket=" + ticket + + require.NoError(t, mint(fmt.Sprintf(`{"ticket":%q,"expires_in":120,"url":%q}`, ticket, url))) + assert.Equal(t, "POST /999/events/stream_ticket.json", seen[0]) + + for name, body := range map[string]string{ + "no ticket": fmt.Sprintf(`{"expires_in":120,"url":%q}`, url), + "no url": fmt.Sprintf(`{"ticket":%q,"expires_in":120}`, ticket), + "no lifetime": fmt.Sprintf(`{"ticket":%q,"url":%q}`, ticket, url), + "zero lifetime": fmt.Sprintf(`{"ticket":%q,"expires_in":0,"url":%q}`, ticket, url), + "negative lifetime": fmt.Sprintf(`{"ticket":%q,"expires_in":-5,"url":%q}`, ticket, url), + "absurd lifetime": fmt.Sprintf(`{"ticket":%q,"expires_in":86400,"url":%q}`, ticket, url), + "unparseable lifetime": fmt.Sprintf(`{"ticket":%q,"expires_in":"soon","url":%q}`, ticket, url), + "fractional lifetime": fmt.Sprintf(`{"ticket":%q,"expires_in":1.5,"url":%q}`, ticket, url), + "lifetime past int range": fmt.Sprintf(`{"ticket":%q,"expires_in":99999999999999999999,"url":%q}`, ticket, url), + } { + t.Run(name, func(t *testing.T) { + err := mint(body) + require.Error(t, err) + assert.NotContains(t, err.Error(), ticket) + assert.NotContains(t, ErrorText(err), ticket) + assert.Equal(t, StatusFail, TicketCheck(context.Background(), SDKReader{Client: client}, KindAgent).Status) + }) + } +} + +// A route directory's name reaches a one-line terminal sink; control +// characters in it must not restyle or break that line. +func TestRouteChecksSanitizeThePath(t *testing.T) { + f := validFile(t) + f.Projects = map[int64]admission.Route{projectID: {Path: "/work/evil\x1b[31m\nFAKE ✓ line"}} + checks := RouteChecks(context.Background(), &fakeReader{}, f) + require.Len(t, checks, 1) + assert.NotContains(t, checks[0].Message, "\x1b") + assert.NotContains(t, checks[0].Message, "\n") +} + +// ErrorText is the one formatter for read errors: an HTTP answer is its +// status and nothing the server wrote. +func TestErrorTextKeepsNothingTheServerWrote(t *testing.T) { + const canary = "CANARY-bearer" + httpErr := &basecamp.Error{Code: "validation", Message: "ticket " + canary, Hint: canary, HTTPStatus: 422} + assert.Equal(t, "HTTP 422", ErrorText(httpErr)) + assert.Equal(t, "HTTP 422", ErrorText(fmt.Errorf("wrapped: %w", httpErr))) + assert.NotContains(t, ErrorText(&basecamp.Error{Code: "network", Message: canary}), canary) + assert.Equal(t, ErrMalformedTicket.Error(), ErrorText(ErrMalformedTicket)) +} + +func TestScopeCheck(t *testing.T) { + for _, tc := range []struct { + oauthType, granted, want string + }{ + {"agent", "full", StatusPass}, + {"bc5", "full", StatusPass}, + {"launchpad", "", StatusPass}, + {"agent", "read", StatusFail}, + {"bc5", "read", StatusFail}, + {"agent", "", StatusFail}, + {"bc5", "", StatusFail}, + {"launchpad", "read", StatusFail}, + {"agent", "Full", StatusFail}, + {"agent", "full read", StatusFail}, + {"agent", "write", StatusFail}, + {"agent", "admin", StatusFail}, + {"agent", " full", StatusFail}, + {"", "", StatusFail}, + {"unknown", "", StatusFail}, + } { + t.Run(tc.oauthType+"/"+tc.granted, func(t *testing.T) { + assert.Equal(t, tc.want, ScopeCheck(tc.oauthType, tc.granted).Status, "unknown scopes are never full") + }) + } +} + +// A route kept from connect.json is checked as a new one is: a directory +// that has since gone is not ready. +func TestRouteChecksCheckEveryRoutesDirectory(t *testing.T) { + f := validFile(t) + gone := f.Projects[otherProj].Path + require.NoError(t, os.Remove(gone)) + file := filepath.Join(t.TempDir(), "file") + require.NoError(t, os.WriteFile(file, nil, 0o600)) + f.Projects[777] = admission.Route{Path: file} + + byName := map[string]Check{} + for _, c := range RouteChecks(context.Background(), &fakeReader{}, f) { + byName[c.Name] = c + } + assert.Equal(t, StatusPass, byName[fmt.Sprintf("Project %d", projectID)].Status) + assert.Equal(t, StatusFail, byName[fmt.Sprintf("Project %d", otherProj)].Status, "a directory that is gone") + assert.Equal(t, StatusFail, byName["Project 777"].Status, "a file where the directory was") +} + +// The lifetime bound is compared in whole seconds, so no value wraps past +// it: the largest int, and the first value whose conversion to a Duration +// overflows, are refused like any other absurd lifetime. +func TestUsableTicketBoundsTheLifetimeWithoutOverflow(t *testing.T) { + ticket := func(seconds int) *basecamp.StreamTicket { + return &basecamp.StreamTicket{Ticket: "t", URL: "wss://example.test", ExpiresIn: seconds} + } + maxSeconds := int(MaxTicketLifetime / time.Second) + assert.True(t, UsableTicket(ticket(120))) + assert.True(t, UsableTicket(ticket(maxSeconds))) + assert.False(t, UsableTicket(ticket(maxSeconds+1))) + assert.False(t, UsableTicket(ticket(0))) + assert.False(t, UsableTicket(ticket(-1))) + assert.False(t, UsableTicket(ticket(math.MaxInt64/int(time.Second))), "MaxInt64/1e9: the last value a Duration holds") + assert.False(t, UsableTicket(ticket(math.MaxInt64/int(time.Second)+1)), "just past it, where the conversion overflows") + assert.False(t, UsableTicket(ticket(math.MaxInt64)), "wraps to -1s when converted first") +} diff --git a/internal/connector/setup/file.go b/internal/connector/setup/file.go new file mode 100644 index 000000000..efe93b805 --- /dev/null +++ b/internal/connector/setup/file.go @@ -0,0 +1,408 @@ +// Package setup is `basecamp connect setup`'s half of the connector (plan +// step 16): connect.json — its schema, where it lives, how it is written +// and how it is refused — and the checks that can run before the connector +// does. +// +// # connect.json +// +// connect.json is the only authority for which directory a project maps to +// and for who may drive the agent. It is local policy: nothing read from +// Basecamp adds a route or widens trust. Its admission part (trust and +// projects) is exactly what admission.ParsePolicy reads; the rest belongs to +// dispatch. +// +// Because the file is the trust anchor, a copy another user could have +// written is not one to act on. Save writes it owner-only (0600, in 0700 +// directories) and atomically; Load refuses a file or directory another +// user owns or can write, a symlink, and anything that is not a regular +// file. A policy the connector would misread is refused at both ends. +// +// # What is not here +// +// The agent's credential. Setup runs on a profile that already holds it +// (connected with `basecamp auth agent connect`, or a bot user logged in +// with `basecamp auth login --expect-identity`), and never stores, replaces +// or removes one. connect.json records only the identity that credential +// proved, so a later run can refuse a profile that has since been pointed at +// someone else. +package setup + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "io" + "path/filepath" + "regexp" + "strconv" + "time" + + "github.com/basecamp/basecamp-cli/internal/auth" + "github.com/basecamp/basecamp-cli/internal/connector/admission" +) + +// Version is the connect.json schema version this package writes and reads. +const Version = 1 + +// FileName is connect.json's name inside its profile directory. +const FileName = "connect.json" + +// Drivers. +const ( + DriverSpawn = "spawn" + DriverACP = "acp" +) + +// Defaults, from the connector spec. +const ( + DefaultDriver = DriverSpawn + DefaultConcurrency = 2 + DefaultDeadline = 45 * time.Minute + + MaxConcurrency = 32 + MinDeadline = time.Minute + MaxDeadline = 24 * time.Hour +) + +// Credential kinds recorded for the agent. +const ( + // KindAgent is an Agent person holding a client_credentials profile, the + // connection ceremony's result. + KindAgent = "agent" + // KindBotUser is a person login (a device-flow bot user) pinned by + // --expect-identity: version 1's path. + KindBotUser = "bot_user" +) + +// File is connect.json. +type File struct { + Version int `json:"version"` + + // Profile is the CLI profile holding the agent's credential. + Profile string `json:"profile"` + // AccountID is the account the agent belongs to. + AccountID string `json:"account_id"` + // Agent is who the profile's credential proved to be when setup ran. + Agent Agent `json:"agent"` + + // Trust and Projects are admission's, in admission's own types, so the + // two cannot drift. + Trust admission.Trust `json:"trust"` + Projects map[int64]admission.Route `json:"projects"` + + Driver string `json:"driver"` + Concurrency int `json:"concurrency"` + Deadline Duration `json:"deadline"` + Worktrees bool `json:"worktrees"` +} + +// Agent is the identity the agent profile authenticated as at setup. +type Agent struct { + // PersonID is the agent's Person id in AccountID. Admission takes the + // agent's id from the profile's verified identity at run time; this is + // what that identity is checked against. + PersonID int64 `json:"person_id"` + // Kind is KindAgent or KindBotUser. + Kind string `json:"kind"` + // IdentityID is the bot user's account-independent identity, the value + // --expect-identity pins. Zero for an Agent person, which has none. + IdentityID int64 `json:"identity_id,omitempty"` +} + +// Duration is a time.Duration written as Go's duration string ("45m0s"). +type Duration time.Duration + +// MarshalJSON implements json.Marshaler. +func (d Duration) MarshalJSON() ([]byte, error) { + return json.Marshal(time.Duration(d).String()) +} + +// UnmarshalJSON implements json.Unmarshaler. +func (d *Duration) UnmarshalJSON(b []byte) error { + var s string + if err := json.Unmarshal(b, &s); err != nil { + return fmt.Errorf("deadline must be a duration string such as \"45m\": %w", err) + } + v, err := time.ParseDuration(s) + if err != nil { + return fmt.Errorf("deadline: %w", err) + } + *d = Duration(v) + return nil +} + +// New is an empty connect.json for a profile, with the spec's defaults. +func New(profile string) File { + return File{ + Version: Version, + Profile: profile, + Trust: admission.Trust{Mode: admission.TrustOperator}, + Projects: map[int64]admission.Route{}, + Driver: DefaultDriver, + Concurrency: DefaultConcurrency, + Deadline: Duration(DefaultDeadline), + } +} + +// Path is where a profile's connect.json lives: under the CLI's config +// directory, one directory per profile, since a connector runs as one +// profile's agent. +func Path(configDir, profile string) (string, error) { + if !validProfile.MatchString(profile) { + return "", fmt.Errorf("connect.json needs a profile name of letters, numbers, hyphens and underscores, got %q", profile) + } + return filepath.Join(configDir, "connect", profile, FileName), nil +} + +var ( + validProfile = regexp.MustCompile(`^[A-Za-z0-9_-]+$`) + validClass = regexp.MustCompile(`^[a-z0-9][a-z0-9_-]{0,39}$`) +) + +// ValidClass reports whether a class is one connect.json can carry: it is +// written onto every stdout pointer line, so it stays a short plain token. +func ValidClass(class string) bool { return validClass.MatchString(class) } + +// Policy is the admission policy this file yields for the agent, obtained +// the way the connector obtains it: by serializing the file and parsing it +// with admission.ParsePolicy, then validating. A file whose policy does not +// validate is one admission would refuse, so setup refuses to write it. +func (f File) Policy(agentID int64) (admission.Policy, error) { + data, err := json.Marshal(f) + if err != nil { + return admission.Policy{}, err + } + p, err := admission.ParsePolicy(data) + if err != nil { + return admission.Policy{}, err + } + p.AgentID = agentID + if err := p.Validate(); err != nil { + return admission.Policy{}, err + } + return p, nil +} + +// Validate refuses a file the connector could misread. It covers everything +// Policy does and the dispatch settings besides. +func (f File) Validate() error { + switch { + case f.Version != Version: + return fmt.Errorf("connect.json version %d is not %d", f.Version, Version) + case !validProfile.MatchString(f.Profile): + return fmt.Errorf("connect.json names no valid profile (%q)", f.Profile) + case !isAccountID(f.AccountID): + return fmt.Errorf("connect.json account_id %q is not a numeric account id", f.AccountID) + case f.Agent.PersonID <= 0: + return errors.New("connect.json records no agent person id") + case f.Trust.Mode == "": + return errors.New("connect.json names no trust mode") + } + switch f.Agent.Kind { + case KindAgent: + if f.Agent.IdentityID != 0 { + return errors.New("connect.json records an identity id for an Agent person, which has none") + } + case KindBotUser: + if f.Agent.IdentityID <= 0 { + return errors.New("connect.json records a bot user without the identity id that pins it") + } + default: + return fmt.Errorf("connect.json agent kind %q is not %q or %q", f.Agent.Kind, KindAgent, KindBotUser) + } + if _, err := f.Policy(f.Agent.PersonID); err != nil { + return err + } + for bucket, route := range f.Projects { + if !filepath.IsAbs(route.Path) || filepath.Clean(route.Path) != route.Path { + return fmt.Errorf("route for project %d: path %q is not a clean absolute path", bucket, route.Path) + } + if route.Class != "" && !ValidClass(route.Class) { + return fmt.Errorf("route for project %d: class %q must be lowercase letters, digits, - or _, at most 40", bucket, route.Class) + } + } + switch f.Driver { + case DriverSpawn, DriverACP: + default: + return fmt.Errorf("connect.json driver %q is not %q or %q", f.Driver, DriverSpawn, DriverACP) + } + if f.Concurrency < 1 || f.Concurrency > MaxConcurrency { + return fmt.Errorf("connect.json concurrency %d is outside 1..%d", f.Concurrency, MaxConcurrency) + } + if d := time.Duration(f.Deadline); d < MinDeadline || d > MaxDeadline { + return fmt.Errorf("connect.json deadline %s is outside %s..%s", d, MinDeadline, MaxDeadline) + } + return nil +} + +// Parse decodes connect.json strictly. It refuses what encoding/json would +// quietly accept: an unknown key (a misspelled "watch_completion" ignored is +// a project the operator believes is driven and is not), a key given twice +// (only the last would count, so the file would not say what it reads as), +// and anything after the object. +func Parse(data []byte) (File, error) { + if err := refuseDuplicateKeys(data); err != nil { + return File{}, fmt.Errorf("parse connect.json: %w", err) + } + dec := json.NewDecoder(bytes.NewReader(data)) + dec.DisallowUnknownFields() + var f File + if err := dec.Decode(&f); err != nil { + return File{}, fmt.Errorf("parse connect.json: %w", err) + } + if _, err := dec.Token(); !errors.Is(err, io.EOF) { + return File{}, errors.New("parse connect.json: trailing data after the object") + } + if f.Projects == nil { + f.Projects = map[int64]admission.Route{} + } + if err := f.Validate(); err != nil { + return File{}, err + } + return f, nil +} + +// canonicalKey reports whether a key is in its one canonical spelling: +// lowercase letters, digits and underscores for names, plain decimal for +// project ids. +func canonicalKey(key string) bool { + if n, err := strconv.ParseInt(key, 10, 64); err == nil { + return strconv.FormatInt(n, 10) == key + } + if key == "" { + return false + } + for _, r := range key { + if (r < 'a' || r > 'z') && (r < '0' || r > '9') && r != '_' { + return false + } + } + return true +} + +// refuseDuplicateKeys walks the JSON document and refuses any object that +// names a key twice. encoding/json matches field names case-insensitively +// and parses project ids as numbers, so "Trust" is "trust" and "048699913" +// is 48699913 to it; every key must therefore be in its one canonical +// spelling, which makes an exact comparison a complete one. +func refuseDuplicateKeys(data []byte) error { + dec := json.NewDecoder(bytes.NewReader(data)) + dec.UseNumber() + var walk func() error + walk = func() error { + tok, err := dec.Token() + if err != nil { + return err + } + delim, ok := tok.(json.Delim) + if !ok { + return nil + } + switch delim { + case '{': + seen := map[string]bool{} + for dec.More() { + keyTok, err := dec.Token() + if err != nil { + return err + } + key, _ := keyTok.(string) + if !canonicalKey(key) { + return fmt.Errorf("key %q is not spelled canonically: names are lowercase, project ids plain decimal", key) + } + if seen[key] { + return fmt.Errorf("key %q appears twice in one object", key) + } + seen[key] = true + if err := walk(); err != nil { + return err + } + } + case '[': + for dec.More() { + if err := walk(); err != nil { + return err + } + } + } + _, err = dec.Token() // the closing delimiter + return err + } + return walk() +} + +// VerifyAgent refuses a connect.json that does not describe the identity a +// profile's credential authenticates as. It is the check that makes a +// credential replaced behind setup's back harmless: whoever acts on +// connect.json — setup before it writes, the connector before it runs — +// proves the file and the credential name one agent, and refuses otherwise. +func (f File) VerifyAgent(kind string, personID, identityID int64) error { + switch { + case personID <= 0: + return fmt.Errorf("connect.json names agent person %d, and the credential authenticates as no person", f.Agent.PersonID) + case f.Agent.PersonID != personID: + return fmt.Errorf("connect.json names agent person %d, and the credential authenticates as person %d", f.Agent.PersonID, personID) + case f.Agent.Kind != kind: + return fmt.Errorf("connect.json names a %s credential, and the profile holds a %s one", f.Agent.Kind, kind) + case f.Agent.Kind == KindBotUser && f.Agent.IdentityID != identityID: + return fmt.Errorf("connect.json names identity %d, and the login is identity %d", f.Agent.IdentityID, identityID) + } + return nil +} + +// Load reads and validates the connect.json at path. A file that does not +// exist is reported as os.ErrNotExist (use errors.Is). A file this user does +// not solely control is refused before a byte of it is believed. +func Load(path string) (File, error) { + data, err := readPrivate(path) + if err != nil { + return File{}, err + } + return Parse(data) +} + +// Save validates f and writes it to path atomically, owner-only, while the +// caller holds the lock on the credential the file is about to name. The +// proof is auth.HeldCredential, and auth.Held is what reads it: a value +// this package could fabricate — a struct embedding the interface — is not +// one auth made, and is refused. +// +// The proof must name this file's own profile: a lock held on another +// profile's credential is not a lock on this one. The proof says the lock +// was held when Save checked it. What happens after +// that is the write's own: one atomic rename, so a connect.json exists +// whole or not at all. +// +// held is only proof; what is written comes from f, which the caller has +// already checked against the credential (see File.VerifyAgent). +func Save(held auth.HeldCredential, path string, f File) error { + _, key, ok := auth.Held(held) + if !ok { + return errors.New("connect.json is written only while the profile's credential is locked") + } + if want := auth.ProfileCredentialKey(f.Profile); key != want { + // A lock on somebody else's credential is not a lock on this one. + return fmt.Errorf("connect.json for profile %q is written only under that profile's credential lock, and %q is held", f.Profile, key) + } + return save(path, f) +} + +// save is Save without the proof, for this package's own tests. +func save(path string, f File) error { + if err := f.Validate(); err != nil { + return err + } + data, err := json.MarshalIndent(f, "", " ") + if err != nil { + return err + } + return writePrivate(path, append(data, '\n')) +} + +// isAccountID accepts an account id only in its canonical spelling, the one +// the connector's instance lock is keyed on. +func isAccountID(s string) bool { + n, err := strconv.ParseUint(s, 10, 64) + return err == nil && n > 0 && strconv.FormatUint(n, 10) == s +} diff --git a/internal/connector/setup/file_test.go b/internal/connector/setup/file_test.go new file mode 100644 index 000000000..02985305f --- /dev/null +++ b/internal/connector/setup/file_test.go @@ -0,0 +1,266 @@ +//go:build unix + +package setup + +import ( + "context" + "encoding/json" + "errors" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/auth" + "github.com/basecamp/basecamp-cli/internal/connector/admission" +) + +const ( + agentID int64 = 52007412 + operatorID int64 = 26909558 + projectID int64 = 48699913 + otherProj int64 = 555 +) + +// validFile is a complete connect.json for the agent profile, rooted in dir. +func validFile(t *testing.T) File { + t.Helper() + f := New("agent") + f.AccountID = "2914079" + f.Agent = Agent{PersonID: agentID, Kind: KindAgent} + f.Trust.OperatorID = operatorID + f.Projects[projectID] = admission.Route{Path: t.TempDir(), Class: "internal", WatchCompletions: true} + f.Projects[otherProj] = admission.Route{Path: t.TempDir()} + return f +} + +func configDir(t *testing.T) string { + t.Helper() + root := t.TempDir() + // Where ownership cannot be read, connect.json must live under the + // user's profile directory; make the test's root that directory. + t.Setenv("USERPROFILE", root) + dir := filepath.Join(root, "basecamp") + require.NoError(t, os.MkdirAll(dir, 0o700)) + return dir +} + +// TestAdmissionReadsWhatSetupWrites is the seam: the file setup saves is the +// policy admission parses, watch_completions included, with nothing lost on +// the way through either decoder. +func TestAdmissionReadsWhatSetupWrites(t *testing.T) { + path, err := Path(configDir(t), "agent") + require.NoError(t, err) + f := validFile(t) + require.NoError(t, save(path, f)) + + data, err := os.ReadFile(path) + require.NoError(t, err) + p, err := admission.ParsePolicy(data) + require.NoError(t, err) + p.AgentID = agentID + require.NoError(t, p.Validate()) + + assert.Equal(t, admission.TrustOperator, p.Trust.Mode) + assert.Equal(t, operatorID, p.Trust.OperatorID) + assert.Equal(t, f.Projects, p.Projects) + assert.True(t, p.Projects[projectID].WatchCompletions) + + loaded, err := Load(path) + require.NoError(t, err) + assert.Equal(t, f, loaded) +} + +func TestSaveWritesTheSpecDefaults(t *testing.T) { + path, err := Path(configDir(t), "agent") + require.NoError(t, err) + require.NoError(t, save(path, validFile(t))) + + var raw map[string]any + data, err := os.ReadFile(path) + require.NoError(t, err) + require.NoError(t, json.Unmarshal(data, &raw)) + assert.Equal(t, "spawn", raw["driver"]) + assert.EqualValues(t, 2, raw["concurrency"]) + assert.Equal(t, "45m0s", raw["deadline"]) + assert.Equal(t, false, raw["worktrees"]) +} + +func TestLoadReportsAMissingFileAsNotExist(t *testing.T) { + path, err := Path(configDir(t), "agent") + require.NoError(t, err) + _, err = Load(path) + assert.True(t, errors.Is(err, os.ErrNotExist), "got %v", err) +} + +// A misspelled key silently ignored is a setting the operator believes is on +// and is not; for trust and routes that is not a harmless typo. +func TestParseRefusesUnknownKeys(t *testing.T) { + data, err := json.Marshal(validFile(t)) + require.NoError(t, err) + var raw map[string]any + require.NoError(t, json.Unmarshal(data, &raw)) + projects := raw["projects"].(map[string]any) + route := projects["48699913"].(map[string]any) + delete(route, "watch_completions") + route["watch_completion"] = true + data, err = json.Marshal(raw) + require.NoError(t, err) + + _, err = Parse(data) + require.Error(t, err) + assert.Contains(t, err.Error(), "watch_completion") +} + +func TestValidateFailsClosed(t *testing.T) { + for name, mutate := range map[string]func(*File){ + "wrong version": func(f *File) { f.Version = 2 }, + "no profile": func(f *File) { f.Profile = "" }, + "profile with a slash": func(f *File) { f.Profile = "../agent" }, + "non-numeric account": func(f *File) { f.AccountID = "acme" }, + "account not canonical": func(f *File) { f.AccountID = "02914079" }, + "empty trust mode": func(f *File) { f.Trust.Mode = "" }, + "no agent": func(f *File) { f.Agent.PersonID = 0 }, + "unknown agent kind": func(f *File) { f.Agent.Kind = "robot" }, + "agent with an identity": func(f *File) { f.Agent.IdentityID = 7 }, + "bot user without identity": func(f *File) { f.Agent.Kind = KindBotUser }, + "no operator": func(f *File) { f.Trust.OperatorID = 0 }, + "operator is the agent": func(f *File) { f.Trust.OperatorID = agentID }, + "unknown trust mode": func(f *File) { f.Trust.Mode = "domain" }, + "agent in the allowlist": func(f *File) { + f.Trust = admission.Trust{Mode: admission.TrustAllowlist, OperatorID: operatorID, AllowlistIDs: []int64{agentID}} + }, + "relative route": func(f *File) { f.Projects[projectID] = admission.Route{Path: "work/app"} }, + "unclean route": func(f *File) { f.Projects[projectID] = admission.Route{Path: "/work/../etc"} }, + "route without a path": func(f *File) { f.Projects[projectID] = admission.Route{} }, + "class with spaces": func(f *File) { f.Projects[projectID] = admission.Route{Path: "/work", Class: "a b"} }, + "unknown driver": func(f *File) { f.Driver = "fork" }, + "no concurrency": func(f *File) { f.Concurrency = 0 }, + "too much concurrency": func(f *File) { f.Concurrency = MaxConcurrency + 1 }, + "deadline too short": func(f *File) { f.Deadline = Duration(time.Second) }, + "deadline too long": func(f *File) { f.Deadline = Duration(48 * time.Hour) }, + "route for a non-project": func(f *File) { + f.Projects[-1] = admission.Route{Path: "/work"} + }, + } { + t.Run(name, func(t *testing.T) { + f := validFile(t) + mutate(&f) + assert.Error(t, f.Validate()) + + path, err := Path(configDir(t), "agent") + require.NoError(t, err) + assert.Error(t, save(path, f), "Save must refuse what Validate refuses") + _, statErr := os.Stat(path) + assert.True(t, os.IsNotExist(statErr), "nothing is written") + }) + } + require.NoError(t, validFile(t).Validate()) +} + +func TestPathRefusesAProfileThatIsNotAName(t *testing.T) { + for _, name := range []string{"", "..", "a/b", "a b", `a\b`} { + _, err := Path("/cfg", name) + assert.Error(t, err, name) + } + path, err := Path("/cfg", "agent_1") + require.NoError(t, err) + assert.Equal(t, filepath.Join("/cfg", "connect", "agent_1", "connect.json"), path) +} + +func TestParseRefusesDuplicateKeysAndTrailingData(t *testing.T) { + data, err := json.Marshal(validFile(t)) + require.NoError(t, err) + _, err = Parse(data) + require.NoError(t, err) + + for name, doc := range map[string]string{ + "trailing ]": string(data) + "]", + "trailing }": string(data) + "}", + "trailing object": string(data) + "{}", + "duplicate top-level": `{"driver":"acp",` + string(data[1:]), + "duplicate operator_id": strings.Replace(string(data), `"operator_id":`, `"operator_id":1,"operator_id":`, 1), + "duplicate nested route": strings.Replace(string(data), `"48699913":{`, `"48699913":{"path":"/elsewhere",`, 1), + "case variant key": strings.Replace(string(data), `"trust":`, `"Trust":`, 1), + "long s variant key": strings.Replace(string(data), `"worktrees"`, `"worktreeſ"`, 1), + "padded project id": strings.Replace(string(data), `"48699913":{`, `"048699913":{`, 1), + "signed project id": strings.Replace(string(data), `"48699913":{`, `"+48699913":{`, 1), + } { + t.Run(name, func(t *testing.T) { + require.NotEqual(t, string(data), doc, "the fixture changed the document") + _, err := Parse([]byte(doc)) + assert.Error(t, err) + }) + } +} + +// VerifyAgent is what makes a credential swapped behind setup's back +// harmless: whoever acts on connect.json proves the file and the credential +// name one agent. +func TestVerifyAgent(t *testing.T) { + f := validFile(t) + require.NoError(t, f.VerifyAgent(KindAgent, agentID, 0)) + + assert.Error(t, f.VerifyAgent(KindAgent, agentID+1, 0), "another person") + assert.Error(t, f.VerifyAgent(KindAgent, 0, 0), "no person") + assert.Error(t, f.VerifyAgent(KindBotUser, agentID, 4242), "another kind") + + bot := validFile(t) + bot.Agent = Agent{PersonID: agentID, Kind: KindBotUser, IdentityID: 4242} + require.NoError(t, bot.VerifyAgent(KindBotUser, agentID, 4242)) + assert.Error(t, bot.VerifyAgent(KindBotUser, agentID, 99), "another identity") + assert.Error(t, bot.VerifyAgent(KindAgent, agentID, 4242), "another kind") +} + +// connect.json is written only under the credential's lock. The proof is +// auth's to make: no other package can implement it, and Save refuses +// anything that is not a live one. +func TestSaveRefusesWithoutAHeldCredential(t *testing.T) { + path, err := Path(configDir(t), "agent") + require.NoError(t, err) + require.Error(t, Save(nil, path, validFile(t))) + _, statErr := os.Stat(path) + assert.True(t, os.IsNotExist(statErr), "nothing is written") +} + +// A proof this package could make for itself is not one auth made: a struct +// that embeds the interface promotes its unexported method and can answer +// anything, and Save refuses it. +type forgedHold struct{ auth.HeldCredential } + +// It answers everything a reader might ask, and is still not a proof auth +// made. +func (forgedHold) Valid() bool { return true } +func (forgedHold) Key() string { return "profile:agent" } +func (forgedHold) Credentials() *auth.Credentials { return &auth.Credentials{AccessToken: "forged"} } + +func TestSaveRefusesAForgedHold(t *testing.T) { + path, err := Path(configDir(t), "agent") + require.NoError(t, err) + require.Error(t, Save(forgedHold{}, path, validFile(t))) + _, statErr := os.Stat(path) + assert.True(t, os.IsNotExist(statErr), "nothing is written") +} + +// A lock on another profile's credential is not a lock on this one. +func TestSaveRefusesAHoldOnAnotherProfile(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + store := auth.NewStore(t.TempDir()) + const otherKey = "profile:other" + require.NoError(t, store.Save(otherKey, &auth.Credentials{AccessToken: "t", OAuthType: "agent"})) + + path, err := Path(configDir(t), "agent") + require.NoError(t, err) + f := validFile(t) // profile "agent" + err = store.WithCredential(context.Background(), otherKey, func(held auth.HeldCredential) error { + return Save(held, path, f) + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "profile \"agent\"") + _, statErr := os.Stat(path) + assert.True(t, os.IsNotExist(statErr), "nothing is written") +} diff --git a/internal/connector/setup/private.go b/internal/connector/setup/private.go new file mode 100644 index 000000000..05c722eee --- /dev/null +++ b/internal/connector/setup/private.go @@ -0,0 +1,179 @@ +package setup + +import ( + "errors" + "fmt" + "io" + "os" + "path/filepath" + + "github.com/gofrs/flock" +) + +// maxFileBytes bounds a connect.json read. A real one is a few kilobytes. +const maxFileBytes = 1 << 20 + +// ErrNotPrivate reports a connect.json, or a directory holding it, that +// someone other than this user could have written. +var ErrNotPrivate = errors.New("connect.json is not private to this user") + +// privateDirs are the directories setup owns on the way to connect.json: the +// CLI's config directory, connect/ and connect//. Each must be this +// user's alone. Everything above them is checked by checkAncestors. +func privateDirs(path string) []string { + profileDir := filepath.Dir(path) + connectDir := filepath.Dir(profileDir) + return []string{filepath.Dir(connectDir), connectDir, profileDir} +} + +// ensurePrivateDirs creates what is missing of privateDirs, owner-only, and +// refuses when any of them, or anything above them, could be changed by +// someone else. +func ensurePrivateDirs(path string) error { + dirs := privateDirs(path) + if err := checkAncestors(filepath.Dir(dirs[0])); err != nil { + return err + } + for _, dir := range dirs { + switch _, err := os.Lstat(dir); { + case errors.Is(err, os.ErrNotExist): + if err := os.Mkdir(dir, 0o700); err != nil && !errors.Is(err, os.ErrExist) { + return fmt.Errorf("create %s: %w", dir, err) + } + case err != nil: + return fmt.Errorf("inspect %s: %w", dir, err) + } + if err := checkPrivateDir(dir); err != nil { + return err + } + } + return nil +} + +// ErrSetupRunning reports another setup already running for this profile. +var ErrSetupRunning = errors.New("another connect setup is running for this profile") + +// ErrLockUnavailable reports a host that cannot take the setup lock at all: +// a filesystem without flock, a lock file this user does not own. Setup's +// guarantee is the lock, so it refuses rather than run without one. +var ErrLockUnavailable = errors.New("this host cannot lock the connector's policy") + +// Lock takes the per-profile setup lock beside connect.json, so two setups +// cannot interleave a load, a change and a save. It refuses rather than +// waits: a second setup on one profile is a mistake to report, not a queue. +func Lock(path string) (unlock func(), err error) { + if err := ensurePrivateDirs(path); err != nil { + return nil, err + } + lockPath := filepath.Join(filepath.Dir(path), ".connect.lock") + // The lock is only a lock if it is this user's own file: a symlink or a + // foreign file left in the directory could point two setups at + // different inodes, and they would not exclude each other. + if err := checkPrivateLockFile(lockPath); err != nil { + if errors.Is(err, ErrNotPrivate) { + return nil, err + } + // The lock file is there and cannot even be inspected: this host + // cannot lock, which is not the same as bad input. + return nil, fmt.Errorf("%w: %s: %w", ErrLockUnavailable, lockPath, err) + } + lock := flock.New(lockPath, flock.SetPermissions(0o600)) + held, err := lock.TryLock() + if err != nil { + return nil, fmt.Errorf("%w: %s: %w", ErrLockUnavailable, lockPath, err) + } + if !held { + return nil, fmt.Errorf("%w: %s", ErrSetupRunning, filepath.Dir(path)) + } + return func() { _ = lock.Unlock() }, nil +} + +// checkPrivateLockFile refuses a lock file this user does not solely own. A +// file that does not exist yet is fine: flock creates it in a directory +// ensurePrivateDirs has already vetted. +func checkPrivateLockFile(path string) error { + f, err := openNoFollow(path) + switch { + case errors.Is(err, os.ErrNotExist): + return nil + case err != nil: + return err + } + defer f.Close() + return checkPrivateFile(f, path) +} + +func readPrivate(path string) ([]byte, error) { + dirs := privateDirs(path) + if err := checkAncestors(filepath.Dir(dirs[0])); err != nil { + return nil, err + } + for _, dir := range dirs { + if err := checkPrivateDir(dir); err != nil { + return nil, err + } + } + f, err := openNoFollow(path) + if err != nil { + return nil, err + } + defer f.Close() + if err := checkPrivateFile(f, path); err != nil { + return nil, err + } + data, err := io.ReadAll(io.LimitReader(f, maxFileBytes+1)) + if err != nil { + return nil, fmt.Errorf("read %s: %w", path, err) + } + if len(data) > maxFileBytes { + return nil, fmt.Errorf("%s is larger than %d bytes", path, maxFileBytes) + } + return data, nil +} + +func writePrivate(path string, data []byte) error { + if err := ensurePrivateDirs(path); err != nil { + return err + } + // An existing connect.json that is not ours is refused rather than + // replaced: the rename would succeed, and whoever planted it would learn + // nothing, but the operator would never hear that it had been there. + switch f, err := openNoFollow(path); { + case err == nil: + checkErr := checkPrivateFile(f, path) + f.Close() + if checkErr != nil { + return checkErr + } + case !errors.Is(err, os.ErrNotExist): + return err + } + + dir := filepath.Dir(path) + tmp, err := os.CreateTemp(dir, ".connect-*.json") + if err != nil { + return fmt.Errorf("write %s: %w", path, err) + } + tmpName := tmp.Name() + defer os.Remove(tmpName) // a no-op once renamed + if err := tmp.Chmod(0o600); err != nil { + tmp.Close() + return fmt.Errorf("write %s: %w", path, err) + } + if _, err := tmp.Write(data); err != nil { + tmp.Close() + return fmt.Errorf("write %s: %w", path, err) + } + if err := tmp.Sync(); err != nil { + tmp.Close() + return fmt.Errorf("write %s: %w", path, err) + } + if err := tmp.Close(); err != nil { + return fmt.Errorf("write %s: %w", path, err) + } + if err := os.Rename(tmpName, path); err != nil { + return fmt.Errorf("write %s: %w", path, err) + } + syncDir(dir) + return nil +} diff --git a/internal/connector/setup/private_other.go b/internal/connector/setup/private_other.go new file mode 100644 index 000000000..75001145a --- /dev/null +++ b/internal/connector/setup/private_other.go @@ -0,0 +1,24 @@ +//go:build !unix + +package setup + +import ( + "fmt" + "os" + "runtime" +) + +// On platforms without Unix ownership and modes, this package cannot verify +// that nobody else can change connect.json: that would take reading owners +// and ACLs, which it does not do. connect.json is the trust anchor, so setup +// fails closed there rather than write or read one it cannot vouch for. + +func errUnsupported() error { + return fmt.Errorf("%w: on %s this CLI cannot verify who can change connect.json, so the connector's trust file is not supported here", ErrNotPrivate, runtime.GOOS) +} + +func checkAncestors(string) error { return errUnsupported() } +func checkPrivateDir(string) error { return errUnsupported() } +func checkPrivateFile(*os.File, string) error { return errUnsupported() } +func openNoFollow(string) (*os.File, error) { return nil, errUnsupported() } +func syncDir(string) {} diff --git a/internal/connector/setup/private_unix.go b/internal/connector/setup/private_unix.go new file mode 100644 index 000000000..d44ef5cd3 --- /dev/null +++ b/internal/connector/setup/private_unix.go @@ -0,0 +1,139 @@ +//go:build unix + +package setup + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "syscall" +) + +// checkPrivateDir refuses a directory that is a symlink, belongs to another +// user, or is writable by group or others: anyone who can write the +// directory can replace connect.json in it. +func checkPrivateDir(dir string) error { + info, err := os.Lstat(dir) + if err != nil { + return fmt.Errorf("inspect %s: %w", dir, err) + } + if info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("%w: %s is a symlink", ErrNotPrivate, dir) + } + if !info.IsDir() { + return fmt.Errorf("%w: %s is not a directory", ErrNotPrivate, dir) + } + return checkOwnerAndMode(info, dir) +} + +// checkPrivateFile refuses an open connect.json that is not a regular file, +// belongs to another user, or is writable by group or others. It inspects +// the descriptor, not the name, so what is checked is what is read. +func checkPrivateFile(f *os.File, path string) error { + info, err := f.Stat() + if err != nil { + return fmt.Errorf("inspect %s: %w", path, err) + } + if !info.Mode().IsRegular() { + return fmt.Errorf("%w: %s is not a regular file", ErrNotPrivate, path) + } + return checkOwnerAndMode(info, path) +} + +func checkOwnerAndMode(info os.FileInfo, path string) error { + st, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return fmt.Errorf("%w: cannot read the owner of %s", ErrNotPrivate, path) + } + if int(st.Uid) != os.Geteuid() { + return fmt.Errorf("%w: %s belongs to uid %d", ErrNotPrivate, path, st.Uid) + } + if perm := info.Mode().Perm(); perm&0o022 != 0 { + return fmt.Errorf("%w: %s is writable by other users (mode %04o); chmod go-w it, or remove it and run setup again", ErrNotPrivate, path, perm) + } + return nil +} + +// checkAncestors refuses a path to the config directory that someone other +// than this user or root could redirect: every directory on it, as written +// and as its symlinks resolve, must belong to root or this user and must not +// be writable by group or others unless it is sticky (as /tmp is, where +// nobody may rename another's entry). A symlink on the way must belong to +// root or this user too. With every ancestor settled, nothing between the +// checks and the open can be swapped by anyone else. +func checkAncestors(dir string) error { + abs, err := filepath.Abs(dir) + if err != nil { + return err + } + if err := checkChain(abs); err != nil { + return err + } + resolved, err := filepath.EvalSymlinks(abs) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("%w: %s does not exist", ErrNotPrivate, abs) + } + return fmt.Errorf("resolve %s: %w", abs, err) + } + if resolved == abs { + return nil + } + return checkChain(resolved) +} + +// checkChain checks each prefix of an absolute path, root first. +func checkChain(abs string) error { + var prefixes []string + for p := abs; ; p = filepath.Dir(p) { + prefixes = append(prefixes, p) + if filepath.Dir(p) == p { + break + } + } + for i := len(prefixes) - 1; i >= 0; i-- { + p := prefixes[i] + info, err := os.Lstat(p) + if err != nil { + return fmt.Errorf("inspect %s: %w", p, err) + } + st, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return fmt.Errorf("%w: cannot read the owner of %s", ErrNotPrivate, p) + } + if st.Uid != 0 && int(st.Uid) != os.Geteuid() { + return fmt.Errorf("%w: %s belongs to uid %d, neither root nor you (in a user-namespace sandbox, run setup outside it)", ErrNotPrivate, p, st.Uid) + } + if info.Mode()&os.ModeSymlink != 0 { + continue + } + if info.Mode().Perm()&0o022 != 0 && info.Mode()&os.ModeSticky == 0 { + return fmt.Errorf("%w: %s is writable by other users (mode %04o); anyone who can write it could replace connect.json below it. Run chmod go-w %s", ErrNotPrivate, p, info.Mode().Perm(), p) + } + } + return nil +} + +// openNoFollow opens path for reading, refusing a symlink at the final +// component and never blocking on a FIFO planted there. +func openNoFollow(path string) (*os.File, error) { + f, err := os.OpenFile(path, os.O_RDONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK|syscall.O_CLOEXEC, 0) + if err != nil { + if errors.Is(err, syscall.ELOOP) { + return nil, fmt.Errorf("%w: %s is a symlink", ErrNotPrivate, path) + } + if errors.Is(err, os.ErrNotExist) { + return nil, err + } + return nil, fmt.Errorf("open %s: %w", path, err) + } + return f, nil +} + +func syncDir(dir string) { + if d, err := os.Open(dir); err == nil { + _ = d.Sync() + d.Close() + } +} diff --git a/internal/connector/setup/private_unix_test.go b/internal/connector/setup/private_unix_test.go new file mode 100644 index 000000000..5b8fdb560 --- /dev/null +++ b/internal/connector/setup/private_unix_test.go @@ -0,0 +1,189 @@ +//go:build unix + +package setup + +import ( + "errors" + "os" + "path/filepath" + "syscall" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func savedPath(t *testing.T) string { + t.Helper() + path, err := Path(configDir(t), "agent") + require.NoError(t, err) + require.NoError(t, save(path, validFile(t))) + return path +} + +func TestSaveIsOwnerOnly(t *testing.T) { + cfg := configDir(t) + path, err := Path(cfg, "agent") + require.NoError(t, err) + file := validFile(t) + require.NoError(t, os.Remove(cfg)) // Save creates the config directory too + + old := syscall.Umask(0) + t.Cleanup(func() { syscall.Umask(old) }) + require.NoError(t, save(path, file)) + + for _, p := range []string{cfg, filepath.Dir(filepath.Dir(path)), filepath.Dir(path)} { + info, err := os.Stat(p) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o700), info.Mode().Perm(), p) + } + info, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm()) + + entries, err := os.ReadDir(filepath.Dir(path)) + require.NoError(t, err) + assert.Len(t, entries, 1, "no temporary file is left behind") +} + +// connect.json is the trust anchor: whoever can write it chooses whom the +// agent obeys and where its work runs. A copy someone else could have +// written is refused on read, and never silently replaced on write. +func TestLoadRefusesAFileOthersCanWrite(t *testing.T) { + for _, mode := range []os.FileMode{0o620, 0o602, 0o666} { + path := savedPath(t) + require.NoError(t, os.Chmod(path, mode)) + + _, err := Load(path) + require.Error(t, err, "mode %04o", mode) + assert.True(t, errors.Is(err, ErrNotPrivate), "mode %04o: %v", mode, err) + + err = save(path, validFile(t)) + assert.True(t, errors.Is(err, ErrNotPrivate), "Save over mode %04o: %v", mode, err) + } +} + +func TestLoadAcceptsAFileOthersCanOnlyRead(t *testing.T) { + path := savedPath(t) + require.NoError(t, os.Chmod(path, 0o644)) + _, err := Load(path) + assert.NoError(t, err) +} + +func TestLoadRefusesADirectoryOthersCanWrite(t *testing.T) { + for _, level := range []int{1, 2, 3} { + path := savedPath(t) + dir := path + for range level { + dir = filepath.Dir(dir) + } + require.NoError(t, os.Chmod(dir, 0o770)) + + _, err := Load(path) + assert.True(t, errors.Is(err, ErrNotPrivate), "%s: %v", dir, err) + assert.True(t, errors.Is(save(path, validFile(t)), ErrNotPrivate), dir) + } +} + +func TestLoadRefusesASymlink(t *testing.T) { + path := savedPath(t) + elsewhere := filepath.Join(t.TempDir(), "connect.json") + data, err := os.ReadFile(path) + require.NoError(t, err) + require.NoError(t, os.WriteFile(elsewhere, data, 0o600)) + require.NoError(t, os.Remove(path)) + require.NoError(t, os.Symlink(elsewhere, path)) + + _, err = Load(path) + assert.True(t, errors.Is(err, ErrNotPrivate), "%v", err) + assert.True(t, errors.Is(save(path, validFile(t)), ErrNotPrivate)) +} + +func TestLoadRefusesASymlinkedProfileDirectory(t *testing.T) { + path := savedPath(t) + profileDir := filepath.Dir(path) + moved := filepath.Join(t.TempDir(), "agent") + require.NoError(t, os.Rename(profileDir, moved)) + require.NoError(t, os.Symlink(moved, profileDir)) + + _, err := Load(path) + assert.True(t, errors.Is(err, ErrNotPrivate), "%v", err) +} + +func TestLoadRefusesANonRegularFile(t *testing.T) { + path := savedPath(t) + require.NoError(t, os.Remove(path)) + require.NoError(t, syscall.Mkfifo(path, 0o600)) + + _, err := Load(path) + assert.True(t, errors.Is(err, ErrNotPrivate), "a FIFO is refused without blocking: %v", err) +} + +// The trust anchor is only as private as the path to it: an ancestor +// another user can write lets them rename the config directory away and put +// their own in its place, whatever the modes inside say. +func TestLoadRefusesAnAncestorOthersCanWrite(t *testing.T) { + shared := t.TempDir() + cfg := filepath.Join(shared, "home", "basecamp") + require.NoError(t, os.MkdirAll(cfg, 0o700)) + path, err := Path(cfg, "agent") + require.NoError(t, err) + require.NoError(t, save(path, validFile(t))) + + require.NoError(t, os.Chmod(filepath.Join(shared, "home"), 0o777)) + _, err = Load(path) + assert.True(t, errors.Is(err, ErrNotPrivate), "%v", err) + assert.True(t, errors.Is(save(path, validFile(t)), ErrNotPrivate)) + + // A sticky shared directory, as /tmp is, lets nobody rename another's + // entry, so it is not a way in. + require.NoError(t, os.Chmod(filepath.Join(shared, "home"), 0o777|os.ModeSticky)) + _, err = Load(path) + assert.NoError(t, err) +} + +func TestLoadRefusesASymlinkedConfigDirectory(t *testing.T) { + path := savedPath(t) + cfg := filepath.Dir(filepath.Dir(filepath.Dir(path))) + moved := filepath.Join(t.TempDir(), "basecamp") + require.NoError(t, os.Rename(cfg, moved)) + require.NoError(t, os.Symlink(moved, cfg)) + + _, err := Load(path) + assert.True(t, errors.Is(err, ErrNotPrivate), "%v", err) +} + +// An ancestor symlink this user owns (a home directory reached through a +// link, /var on macOS) is followed, and where it leads is checked too. +func TestLoadFollowsAnOwnAncestorSymlinkAndChecksItsTarget(t *testing.T) { + target := t.TempDir() + cfg := filepath.Join(target, "basecamp") + require.NoError(t, os.MkdirAll(cfg, 0o700)) + link := filepath.Join(t.TempDir(), "home") + require.NoError(t, os.Symlink(target, link)) + + path, err := Path(filepath.Join(link, "basecamp"), "agent") + require.NoError(t, err) + require.NoError(t, save(path, validFile(t))) + _, err = Load(path) + require.NoError(t, err) + + require.NoError(t, os.Chmod(target, 0o777)) + _, err = Load(path) + assert.True(t, errors.Is(err, ErrNotPrivate), "the target's modes count: %v", err) +} + +func TestLockRefusesASecondSetup(t *testing.T) { + path, err := Path(configDir(t), "agent") + require.NoError(t, err) + unlock, err := Lock(path) + require.NoError(t, err) + + _, err = Lock(path) + assert.Error(t, err) + + unlock() + unlockAgain, err := Lock(path) + require.NoError(t, err) + unlockAgain() +} diff --git a/internal/connector/setup/report.go b/internal/connector/setup/report.go new file mode 100644 index 000000000..cdf2aa27b --- /dev/null +++ b/internal/connector/setup/report.go @@ -0,0 +1,65 @@ +package setup + +import ( + "encoding/json" + "slices" +) + +// Report is the outcome of one setup run: what was written and every check. +// Whether the connector is ready is never stored: it is derived from the +// checks every time it is asked for, so no code path can report ready while +// a check has failed or before any check ran. +type Report struct { + Path string + Profile string + AccountID string + AgentPersonID int64 + AgentKind string + OperatorID int64 + TrustMode string + Routes int + Written bool + + checks []Check +} + +// Add records checks, in order. +func (r *Report) Add(checks ...Check) { r.checks = append(r.checks, checks...) } + +// Checks returns the checks recorded so far. +func (r *Report) Checks() []Check { return slices.Clone(r.checks) } + +// Failed returns the checks that failed. +func (r *Report) Failed() []Check { + var failed []Check + for _, c := range r.checks { + if c.Status != StatusPass && c.Status != StatusWarn && c.Status != StatusSkip { + failed = append(failed, c) + } + } + return failed +} + +// Ready reports whether the connector could run as set up: connect.json was +// written, at least one check ran, and none failed. A check with a status +// outside the known ones counts as failed. +func (r *Report) Ready() bool { + return r.Written && len(r.checks) > 0 && len(r.Failed()) == 0 +} + +// MarshalJSON writes the report with ready derived, never stored. +func (r *Report) MarshalJSON() ([]byte, error) { + return json.Marshal(struct { + Path string `json:"path"` + Profile string `json:"profile"` + AccountID string `json:"account_id"` + AgentPersonID int64 `json:"agent_person_id"` + AgentKind string `json:"agent_kind"` + OperatorID int64 `json:"operator_id"` + TrustMode string `json:"trust_mode"` + Routes int `json:"routes"` + Written bool `json:"written"` + Ready bool `json:"ready"` + Checks []Check `json:"checks"` + }{r.Path, r.Profile, r.AccountID, r.AgentPersonID, r.AgentKind, r.OperatorID, r.TrustMode, r.Routes, r.Written, r.Ready(), r.Checks()}) +} diff --git a/internal/output/codes.go b/internal/output/codes.go index a0b620e3d..2882f303c 100644 --- a/internal/output/codes.go +++ b/internal/output/codes.go @@ -53,6 +53,17 @@ const ( ExitTerminated = 143 ) +// CodeBusy marks work another process is doing right now — a profile's +// credential or connector policy locked by another command — and +// CodeLockUnavailable a host that cannot lock at all, so a command whose +// guarantee is the lock refuses to run. Both are the caller's cue to try +// again (the first shortly, the second after fixing the host), which is +// what the rate-limit status says in this table. +const ( + CodeBusy = "busy" + CodeLockUnavailable = "lock_unavailable" +) + // ExitCodeFor returns the exit code for a given error code. func ExitCodeFor(code string) int { switch code { @@ -64,6 +75,8 @@ func ExitCodeFor(code string) int { return ExitInterrupted case CodeTerminated: return ExitTerminated + case CodeBusy, CodeLockUnavailable: + return ExitRateLimit } return clioutput.ExitCodeFor(code) } diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index 8ad977a0c..d53358857 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -1453,6 +1453,7 @@ BASECAMP_NONINTERACTIVE=1 basecamp auth login --device-code # The only OAuth lo basecamp auth login --with-token -P bot --account # Import a personal access token from stdin (pipe it in) basecamp auth login --with-client-credentials --client-id -P agent --account # Authenticate as a Basecamp agent: client secret on stdin, self-token minted on demand (no refresh token) basecamp auth agent connect -P agent # Connect this computer to a Basecamp agent: approve it in a browser and its OAuth client is stored — nothing to paste +basecamp connect setup -P agent --operator-profile --route = # Set up a local agent connector on a connected profile (run `auth agent connect` first): verifies trust, checks token, identity, scope, ticket mint and project reads, then writes connect.json ``` **Before running ANY of the logins above, check `oauth_type`.** `basecamp auth