From 66dfbcd5d8b360a9eb9cb1c213aba44eead36de3 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Thu, 17 Sep 2026 12:13:28 +0200 Subject: [PATCH 1/9] Let a local profile entry refine the global one, and name the file that hides a binding A trusted repo or local config that defined a profile with no account_id replaced the global config's entry whole, so an account bound there was never seen. Entries for the same Basecamp now merge field by field; an entry for another Basecamp still replaces the farther one. config.Load records which file each profile and field came from, and connect setup, auth agent connect and the headless logins use it to name the file that hides a binding, refusing to bind where it would not take effect. --- internal/auth/agent.go | 11 +- internal/commands/auth.go | 21 +-- internal/commands/auth_agent.go | 14 +- internal/commands/auth_login_test.go | 55 +++---- internal/commands/connect.go | 32 ++-- internal/commands/connect_unbound_test.go | 48 ++++-- internal/commands/profile.go | 57 ++++--- .../commands/profile_layers_helpers_test.go | 65 ++++++++ internal/commands/profile_layers_test.go | 130 +++++++++++++++ internal/config/config.go | 154 +++++++++++++++--- internal/config/config_test.go | 91 +++++++++++ 11 files changed, 533 insertions(+), 145 deletions(-) create mode 100644 internal/commands/profile_layers_helpers_test.go create mode 100644 internal/commands/profile_layers_test.go diff --git a/internal/auth/agent.go b/internal/auth/agent.go index 102452bdc..cdcd9e316 100644 --- a/internal/auth/agent.go +++ b/internal/auth/agent.go @@ -525,12 +525,11 @@ func (m *Manager) agentLoginCommand(clientID, scope string) string { // // An entry that EXISTS without an account deliberately gets no // --account. The login binds one there only when the global config - // file is the layer that defines the profile, and from here there is - // no way to know which layer did — a repo or system config that - // defines it accountless would make this command fail with "not the - // global config's entry", and a command handed over to be run - // verbatim should not be one that cannot work. Without it the login - // refuses by naming exactly what it needs, layer included, which is + // file is one of the layers the profile is made of — a repo or local + // entry for another Basecamp, or one the global file lacks, would make + // this command fail — and a command handed over to be run verbatim + // should not be one that cannot work. Without it the login refuses by + // naming exactly what it needs, the file to change included, which is // better guidance than a guess. if entry, registered := m.cfg.Profiles[m.cfg.ActiveProfile]; !registered || entry == nil { command += " --account " + m.accountToBind() diff --git a/internal/commands/auth.go b/internal/commands/auth.go index a505c1ccf..5651045e0 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -853,13 +853,9 @@ func resolveHeadlessProfile(app *appctx.App, login headlessLogin) (*profileTarge return nil, err } } - globalUnbound := false + blocker := "" if existing != nil && existing.AccountID == "" { - unbound, err := globalProfileIsUnbound(name) - if err != nil { - return nil, err - } - globalUnbound = unbound + blocker = globalBindingBlocker(app.Config, name) } target := &profileTarget{name: name, account: account, existing: existing} @@ -870,16 +866,15 @@ func resolveHeadlessProfile(app *appctx.App, login headlessLogin) (*profileTarge case existing == nil: target.created = &config.ProfileConfig{BaseURL: app.Config.BaseURL, AccountID: account} target.existing = nil + case blocker != "": + // Binding writes the global config's entry, and for this profile + // that would not take effect. Refused before --account is asked + // for, which would only lead to this refusal. + return nil, output.ErrUsageHint(fmt.Sprintf("Profile %q has no account, and a login cannot bind one", name), + blocker+", then rerun "+login.rerun+".") case existing.AccountID == "" && !accountGivenExplicitly(app): return nil, output.ErrUsageHint(fmt.Sprintf("Profile %q has no account", name), "Pass --account to bind it alongside "+login.credential+".") - case existing.AccountID == "" && !globalUnbound: - // Binding rewrites the global config file. The effective profile - // is accountless, so if the global entry is missing or already - // carries an account, the accountless one came from a system, repo - // or local config and would keep shadowing whatever is written. - return nil, output.ErrUsageHint(fmt.Sprintf("Profile %q has no account and is not the global config's entry", name), - "Add account_id to the config file that defines it, then rerun "+login.rerun+".") case existing.AccountID == "": target.bind = true } diff --git a/internal/commands/auth_agent.go b/internal/commands/auth_agent.go index f72f79f71..a35a5a2bd 100644 --- a/internal/commands/auth_agent.go +++ b/internal/commands/auth_agent.go @@ -225,22 +225,16 @@ func resolveAgentConnectProfile(app *appctx.App) (*agentConnectTarget, error) { // Registering or binding rewrites the global config file. Prove it can // be before an operator approves anything — and, when the entry exists - // without an account, that the global file is the layer that defines - // it, since a system, repo or local config's accountless entry would - // keep shadowing whatever is written. + // without an account, that the account written there takes effect. if target.existing == nil || target.existing.AccountID == "" { if err := globalConfigTakesProfiles(); err != nil { return nil, err } } if target.existing != nil && target.existing.AccountID == "" { - unbound, err := globalProfileIsUnbound(name) - if err != nil { - return nil, err - } - if !unbound { - return nil, output.ErrUsageHint(fmt.Sprintf("Profile %q has no account and is not the global config's entry", name), - "Add account_id to the config file that defines it, then rerun the connection.") + if blocker := globalBindingBlocker(app.Config, name); blocker != "" { + return nil, output.ErrUsageHint(fmt.Sprintf("Profile %q has no account, and connecting cannot bind one", name), + blocker+", then rerun the connection.") } } return target, nil diff --git a/internal/commands/auth_login_test.go b/internal/commands/auth_login_test.go index 247632e7c..52ac77411 100644 --- a/internal/commands/auth_login_test.go +++ b/internal/commands/auth_login_test.go @@ -939,15 +939,11 @@ func TestAuthLoginWithTokenFailsClosedWithoutAnIdentity(t *testing.T) { func TestAuthLoginWithTokenBindsAnUnboundExistingProfile(t *testing.T) { srv := startLoginIdentityServer(t, "bc_at_secret") - cfg := &config.Config{ - ActiveProfile: "bot", - Profiles: map[string]*config.ProfileConfig{"bot": {ProjectID: "42"}}, - } - app, _ := loginTestApp(t, srv, cfg) - app.Config.Sources["profiles"] = "global" + app, _ := loginTestApp(t, srv, &config.Config{}) require.NoError(t, os.MkdirAll(config.GlobalConfigDir(), 0o700)) require.NoError(t, os.WriteFile(filepath.Join(config.GlobalConfigDir(), "config.json"), []byte(`{"profiles":{"bot":{"base_url":"`+srv.srv.URL+`","project_id":"42"}}}`), 0o600)) + reloadApp(t, app, srv.srv.URL, "bot") t.Run("without an explicit account the import is refused", func(t *testing.T) { withAccount(app, "999", "global") @@ -1049,51 +1045,48 @@ func TestAuthLoginWithTokenWithoutReportedExpiryIsNonExpiring(t *testing.T) { assert.Nil(t, envelope.Data["expires_at"]) } -// TestAuthLoginWithTokenBindsOnlyTheGlobalConfigsOwnEntry: config layers -// merge per profile name, so cfg.Sources["profiles"] is aggregate and says -// nothing about where this profile came from. The global file's entry is -// the evidence: bind when it exists and is unbound, refuse otherwise — -// before stdin is read. -func TestAuthLoginWithTokenBindsOnlyTheGlobalConfigsOwnEntry(t *testing.T) { +// TestAuthLoginWithTokenBindsOnlyWhereTheBindingTakesEffect: binding +// writes the global config's entry, which binds the profile only when that +// entry is one of the layers the profile is made of. Where it is not, the +// import is refused, naming the file to change, before stdin is read. +func TestAuthLoginWithTokenBindsOnlyWhereTheBindingTakesEffect(t *testing.T) { cases := map[string]struct { global string - sources string + local string wantErr string }{ "global file has no such profile": { global: `{"profiles":{"other":{"base_url":"https://3.basecampapi.com"}}}`, - sources: "repo", - wantErr: "not the global config's entry", + local: `{"profiles":{"bot":{"base_url":"BASE"}}}`, + wantErr: "Its entry comes from LOCAL, not the global config", }, - "global entry is already bound (shadowed by an accountless layer)": { - global: `{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","account_id":"111"}}}`, - sources: "local", - wantErr: "not the global config's entry", + "global entry replaced by a local entry for another Basecamp": { + global: `{"profiles":{"bot":{"base_url":"https://elsewhere.example","account_id":"111"}}}`, + local: `{"profiles":{"bot":{"base_url":"BASE"}}}`, + wantErr: "Its entry in LOCAL is for BASE, not https://elsewhere.example, so it replaces the global config's entry", }, - "global entry is unbound even though another layer contributed profiles": { - global: `{"profiles":{"bot":{"base_url":"BASE","project_id":"42"}}}`, - sources: "repo", + "global entry refined by a local entry for the same Basecamp": { + global: `{"profiles":{"bot":{"base_url":"BASE","project_id":"42"}}}`, + local: `{"profiles":{"bot":{"base_url":"BASE","todolist_id":"7"}}}`, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { srv := startLoginIdentityServer(t, "bc_at_secret") - cfg := &config.Config{ - ActiveProfile: "bot", - Profiles: map[string]*config.ProfileConfig{"bot": {}}, - Sources: map[string]string{"profiles": tc.sources}, - } - app, _ := loginTestApp(t, srv, cfg) - withAccount(app, "999", "flag") + app, _ := loginTestApp(t, srv, &config.Config{}) require.NoError(t, os.MkdirAll(config.GlobalConfigDir(), 0o700)) require.NoError(t, os.WriteFile(filepath.Join(config.GlobalConfigDir(), "config.json"), []byte(strings.ReplaceAll(tc.global, "BASE", srv.srv.URL)), 0o600)) + local := trustedLocalConfig(t, strings.ReplaceAll(tc.local, "BASE", srv.srv.URL)) + reloadApp(t, app, srv.srv.URL, "bot") + withAccount(app, "999", "flag") in := strings.NewReader("bc_at_secret") out, err := runLogin(t, app, in, "--with-token") if tc.wantErr != "" { require.Error(t, err) - assert.Contains(t, err.Error(), tc.wantErr) + want := strings.NewReplacer("LOCAL", local, "BASE", srv.srv.URL).Replace(tc.wantErr) + assert.Contains(t, err.Error(), want) assert.Equal(t, 12, in.Len(), "refused before stdin is read") assert.Empty(t, srv.seenBearers()) return @@ -1102,6 +1095,8 @@ func TestAuthLoginWithTokenBindsOnlyTheGlobalConfigsOwnEntry(t *testing.T) { bot := readGlobalConfig(t)["profiles"].(map[string]any)["bot"].(map[string]any) assert.Equal(t, "999", bot["account_id"]) assert.Equal(t, "42", bot["project_id"]) + reloadApp(t, app, srv.srv.URL, "bot") + assert.Equal(t, "999", app.Config.Profiles["bot"].AccountID, "the binding takes effect") }) } } diff --git a/internal/commands/connect.go b/internal/commands/connect.go index e293bb6f1..365f30819 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -866,11 +866,15 @@ func connectAccount(app *appctx.App, name string) (string, error) { // An entry that names an account nothing can use is not an unbound // profile: say what is wrong with the account it does name. if p.AccountID != "" { + where := "the config file that defines the profile" + if origin := app.Config.ProfileOrigins[name]; origin != nil && origin.Fields["account_id"] != "" { + where = "the profile's entry in " + richtext.SanitizeSingleLine(origin.Fields["account_id"]) + } return "", output.ErrUsageHint( fmt.Sprintf("Profile %q names account %q, which is not an account ID", name, p.AccountID), - "Correct account_id in the config file that defines the profile.") + "Correct account_id in "+where+".") } - return "", unboundProfileError(name) + return "", unboundProfileError(app.Config, name) } if accountGivenExplicitly(app) && !accountIDsEqual(app.Config.AccountID, bound) { return "", output.ErrUsageHint( @@ -891,25 +895,13 @@ func connectAccount(app *appctx.App, name string) (string, error) { // bot that already has its credential the account goes into its config entry // by hand rather than through another login. // -// Even `auth agent connect` refuses an accountless entry that is not the -// global config's, because the entry it would write stays shadowed by the -// system, repo or local config that defines it. For that profile the only -// remedy is the file the entry comes from. -func unboundProfileError(name string) error { +// Those commands all write the global config's entry, which does not bind +// every profile: globalBindingBlocker, the question they ask themselves, +// says when it would not, and names the file to change instead. +func unboundProfileError(cfg *config.Config, name string) error { message := fmt.Sprintf("Profile %q is not bound to an account", name) - // The same question `auth agent connect` asks itself, so the hint is - // right whenever that command is: it takes an unbound global entry as - // its own to write. Where that reading is wrong — a trusted repo or - // local config defining the same accountless profile, which keeps - // shadowing the global entry after it is written — it is wrong for the - // command too, and telling them apart needs per-profile provenance the - // config layer does not keep. - // - // A global config that cannot be read is no reason to fail here: the - // command named below reports that itself, in full. - if unbound, err := globalProfileIsUnbound(name); err == nil && !unbound { - return output.ErrUsageHint(message, - "The entry is not the global config's, so no command can bind it. Add account_id to the config file that defines it.") + if blocker := globalBindingBlocker(cfg, name); blocker != "" { + return output.ErrUsageHint(message, blocker+".") } global := filepath.Join(config.GlobalConfigDir(), "config.json") return output.ErrUsageHint(message, diff --git a/internal/commands/connect_unbound_test.go b/internal/commands/connect_unbound_test.go index 05ef31bcc..e5fc9b2dd 100644 --- a/internal/commands/connect_unbound_test.go +++ b/internal/commands/connect_unbound_test.go @@ -12,7 +12,6 @@ import ( "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/config" - "github.com/basecamp/basecamp-cli/internal/output" ) // writeGlobalConfig puts a config file where GlobalConfigDir will find it, @@ -26,22 +25,15 @@ func writeGlobalConfig(t *testing.T, contents string) { require.NoError(t, os.WriteFile(filepath.Join(dir, "config.json"), []byte(contents), 0o600)) } -// hintOf is the hint an error carries for the operator to act on. -func hintOf(t *testing.T, err error) string { - t.Helper() - var e *output.Error - require.ErrorAs(t, err, &e) - return e.Hint -} - // No command binds an account on its own. Connecting an Agent writes its // account; a bot user's browser login writes none — `auth login --account` // binds only on the headless paths — so a bot's remedy is its config entry, // and naming `auth login` would send the operator round the same loop. func TestUnboundProfileHintNamesOnlyWhatBinds(t *testing.T) { writeGlobalConfig(t, `{"profiles":{"agent":{"base_url":"https://3.basecampapi.com"}}}`) + trustedLocalConfig(t, `{}`) - hint := hintOf(t, unboundProfileError("agent")) + hint := hintOf(t, unboundProfileError(loadConfig(t), "agent")) assert.Contains(t, hint, "basecamp auth agent connect -P agent") assert.Contains(t, hint, "add account_id to the profile's entry in "+filepath.Join(config.GlobalConfigDir(), "config.json")) @@ -49,19 +41,31 @@ func TestUnboundProfileHintNamesOnlyWhatBinds(t *testing.T) { assert.NotContains(t, hint, "auth login", "a browser login binds no account") } -// Storing the credential again cannot bind a profile whose accountless entry -// comes from a system, repo or local config: both login paths refuse it, -// because the global entry they would write stays shadowed. Sending the -// operator there would be another command that fails on them. +// Storing the credential again cannot bind a profile whose entry comes from +// a system, repo or local config and not the global one: both login paths +// refuse it, because the global entry they would write is not part of the +// profile. Sending the operator there would be another command that fails +// on them, so the hint names the file the entry does come from. func TestUnboundProfileHintNamesTheConfigFileWhenTheEntryIsNotGlobal(t *testing.T) { writeGlobalConfig(t, `{"profiles":{"other":{"account_id":"999"}}}`) + local := trustedLocalConfig(t, `{"profiles":{"agent":{"base_url":"https://3.basecampapi.com"}}}`) - hint := hintOf(t, unboundProfileError("agent")) + hint := hintOf(t, unboundProfileError(loadConfig(t), "agent")) - assert.Contains(t, hint, "account_id to the config file that defines it") + assert.Contains(t, hint, "Its entry comes from "+local+", not the global config") + assert.Contains(t, hint, "Add account_id to the profile's entry there") assert.NotContains(t, hint, "basecamp auth", "a command that would refuse this profile is no remedy") } +// loadConfig is the configuration the CLI would load from where the test +// stands. +func loadConfig(t *testing.T) *config.Config { + t.Helper() + cfg, err := config.Load(config.FlagOverrides{}) + require.NoError(t, err) + return cfg +} + // A profile that names an account nothing can parse is not an unbound one, // and calling it unbound sends the operator to bind an account it already // has. The account it does name is what is wrong. @@ -77,3 +81,15 @@ func TestConnectAccountReportsAnUnusableAccountAsItself(t *testing.T) { assert.NotContains(t, err.Error(), "not bound") assert.Contains(t, hintOf(t, err), "Correct account_id") } + +// The file an unusable account comes from is the one to correct, and a +// global entry's account is not where a local entry's comes from. +func TestConnectAccountNamesTheFileOfAnUnusableAccount(t *testing.T) { + writeGlobalConfig(t, `{"profiles":{"agent":{"base_url":"https://3.basecampapi.com","account_id":"999"}}}`) + local := trustedLocalConfig(t, `{"profiles":{"agent":{"base_url":"https://3.basecampapi.com","account_id":"not-a-number"}}}`) + + _, err := connectAccount(&appctx.App{Config: loadConfig(t)}, "agent") + + require.Error(t, err) + assert.Equal(t, "Correct account_id in the profile's entry in "+local+".", hintOf(t, err)) +} diff --git a/internal/commands/profile.go b/internal/commands/profile.go index 4e247bc49..52c343bba 100644 --- a/internal/commands/profile.go +++ b/internal/commands/profile.go @@ -553,18 +553,40 @@ func globalProfileEntry(configData map[string]any, name string) map[string]any { return entry } -// globalProfileIsUnbound reports whether the global config file defines the -// profile without an account — the one shape bindProfileAccount can act on. -// Config layers merge per profile name, so the effective profile being -// accountless says nothing about which file it came from; the global entry -// itself is the evidence. -func globalProfileIsUnbound(name string) (bool, error) { - configData, _, err := loadGlobalConfigFile() - if err != nil { - return false, err +// globalBindingBlocker says why binding an accountless profile would not +// take effect, and what to change instead; "" when it would. +// +// Every command that binds a profile's account (`auth agent connect`, the +// headless logins) does it one way: bindProfileAccount writes account_id +// into the profile's entry in the global config file. So this is the one +// question they ask before binding, and the one the hints that send an +// operator to them ask first. +// +// The write takes effect exactly when the global file is one of the layers +// the profile is made of (config.mergeProfile has the rule): the profile has +// no account, so no closer layer sets one, and the global file's shows +// through. When the global file is not one of them, either a closer entry +// for another Basecamp replaced its entry whole, or it has no entry, and the +// account has to go into a file that is. +func globalBindingBlocker(cfg *config.Config, name string) string { + origin := cfg.ProfileOrigins[name] + if origin == nil || len(origin.Layers) == 0 { + // Only a config file defines a profile; an entry with no origin was + // made in memory, and the global file is where it belongs. + return "It comes from no config file, so add it, with account_id, to " + + richtext.SanitizeSingleLine(filepath.Join(config.GlobalConfigDir(), "config.json")) + } + if origin.Includes(config.SourceGlobal) { + return "" } - entry := globalProfileEntry(configData, name) - return entry != nil && getStringOrNumber(entry, "account_id") == "", nil + closest := richtext.SanitizeSingleLine(origin.Closest().Path) + if hidden := origin.ReplacedLayer(config.SourceGlobal); hidden != nil { + replacer := origin.Layers[0] + return fmt.Sprintf("Its entry in %s is for %s, not %s, so it replaces the global config's entry in %s and any account bound there. Add account_id to the profile's entry in %s", + richtext.SanitizeSingleLine(replacer.Path), richtext.SanitizeSingleLine(replacer.BaseURL), + richtext.SanitizeSingleLine(hidden.BaseURL), richtext.SanitizeSingleLine(hidden.Path), closest) + } + return fmt.Sprintf("Its entry comes from %s, not the global config, so no command can bind it. Add account_id to the profile's entry there", closest) } // bindProfileAccount sets the account on an existing profile entry in the @@ -585,19 +607,6 @@ func bindProfileAccount(name, account string) error { return atomicWriteJSON(configPath, configData) } -// getStringOrNumber reads a config value that may be stored as a string or -// a JSON number, as config.loadFromFile accepts for IDs. -func getStringOrNumber(m map[string]any, key string) string { - switch v := m[key].(type) { - case string: - return v - case float64: - return strconv.FormatFloat(v, 'f', -1, 64) - default: - return "" - } -} - // unregisterProfile removes a profile entry from the global config file, // clearing default_profile when it named this profile. Credentials are the // caller's to remove. diff --git a/internal/commands/profile_layers_helpers_test.go b/internal/commands/profile_layers_helpers_test.go new file mode 100644 index 000000000..0a5eddb3c --- /dev/null +++ b/internal/commands/profile_layers_helpers_test.go @@ -0,0 +1,65 @@ +package commands + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/appctx" + "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/output" +) + +// elsewhereBaseURL is a Basecamp other than the mock servers'. +const elsewhereBaseURL = "https://elsewhere.example" + +// trustedLocalConfig makes a fresh directory the working directory, with a +// local config holding contents that `basecamp config trust` has trusted. +// HOME (USERPROFILE on Windows) points away from it, so no repo config is found above it. It +// returns the local config's path. +func trustedLocalConfig(t *testing.T, contents string) string { + t.Helper() + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + dir := t.TempDir() + path := filepath.Join(dir, ".basecamp", "config.json") + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o700)) + require.NoError(t, os.WriteFile(path, []byte(contents), 0o600)) + require.NoError(t, config.NewTrustStore(config.GlobalConfigDir()).Trust(path)) + t.Chdir(dir) + return path +} + +// hintOf is the hint an error carries for the operator to act on. +func hintOf(t *testing.T, err error) string { + t.Helper() + var e *output.Error + require.ErrorAs(t, err, &e) + return e.Hint +} + +// writeGlobalConfigHere writes the global config file under the +// XDG_CONFIG_HOME already set. +func writeGlobalConfigHere(t *testing.T, contents string) { + t.Helper() + require.NoError(t, os.MkdirAll(config.GlobalConfigDir(), 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(config.GlobalConfigDir(), "config.json"), []byte(contents), 0o600)) +} + +// reloadApp replaces app's config with what config.Load reads +// now, the way the next invocation of the CLI would see it. +func reloadApp(t *testing.T, app *appctx.App, baseURL, profile string) { + t.Helper() + cfg, err := config.Load(config.FlagOverrides{}) + require.NoError(t, err) + cfg.BaseURL = baseURL + if _, ok := cfg.Profiles[profile]; ok { + require.NoError(t, cfg.ApplyProfile(profile)) + } else { + cfg.ActiveProfile = profile + } + *app.Config = *cfg +} diff --git a/internal/commands/profile_layers_test.go b/internal/commands/profile_layers_test.go new file mode 100644 index 000000000..aa6d3e8e8 --- /dev/null +++ b/internal/commands/profile_layers_test.go @@ -0,0 +1,130 @@ +//go:build unix + +package commands + +import ( + "fmt" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/connector/setup" +) + +// These tests put a profile in two config files, the global one and a +// trusted local one, and run the commands over config.Load as the CLI would: +// the layering is the thing under test, so no config is built by hand. + +// The operator bound the account in the global config and runs setup again. +// A trusted local config that names the same profile on the same Basecamp, +// without an account, refines the global entry rather than replacing it, +// so the binding is seen. +func TestConnectSetupSeesTheGlobalBindingThroughALocalEntry(t *testing.T) { + s := startConnectSetupServer(t) + connectSetupApp(t, s, "agent") + trustedLocalConfig(t, fmt.Sprintf(`{"profiles":{"agent":{"base_url":%q,"project_id":"42"}}}`, s.srv.URL)) + + app := newConnectSetupApp(t, s, "agent") + 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) + assert.Equal(t, "42", app.Config.Profiles["agent"].ProjectID, "the local entry's own fields still win") +} + +// A local entry for another Basecamp replaces the global one whole, and the +// account bound there with it. Setup says which file hides the binding and +// what to change, rather than sending the operator to bind it again. +func TestConnectSetupNamesTheFileThatHidesTheGlobalBinding(t *testing.T) { + s := startConnectSetupServer(t) + connectSetupApp(t, s, "agent") + local := trustedLocalConfig(t, fmt.Sprintf(`{"profiles":{"agent":{"base_url":%q}}}`, elsewhereBaseURL)) + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupOperatorPerson), routeArg(t)) + require.Error(t, err, out) + assert.Contains(t, err.Error(), "not bound to an account") + hint := hintOf(t, err) + assert.Contains(t, hint, local, "the file that hides the binding") + assert.Contains(t, hint, filepath.Join(config.GlobalConfigDir(), "config.json"), "the file whose binding is hidden") + assert.NotContains(t, hint, "basecamp auth", "binding the global entry again changes nothing") + assertNotWritten(t, "agent") +} + +// The operator connects the agent under a profile whose global entry has no +// account yet, while a trusted local config names the same profile on the +// same Basecamp. The account the connection binds in the global entry is +// the one the next command sees. +func TestAuthAgentConnectBindsThroughALocalEntry(t *testing.T) { + as := startConnectAS(t) + app := connectApp(t, as, &config.Config{}) + writeGlobalConfigHere(t, fmt.Sprintf(`{"profiles":{"agent":{"base_url":%q}}}`, as.srv.URL)) + trustedLocalConfig(t, fmt.Sprintf(`{"profiles":{"agent":{"base_url":%q,"project_id":"42"}}}`, as.srv.URL)) + reloadApp(t, app, as.srv.URL, "agent") + + out, err := runAgentConnect(t, app, "--device-name", "build-box") + require.NoError(t, err, out) + + reloadApp(t, app, as.srv.URL, "agent") + assert.Equal(t, "999", app.Config.Profiles["agent"].AccountID, "the binding takes effect") + assert.Equal(t, "42", app.Config.Profiles["agent"].ProjectID) +} + +// A global entry already bound, under a same-Basecamp local entry with no +// account, is a bound profile: connecting the agent in that account again +// is not refused as unbindable. +func TestAuthAgentConnectAcceptsAGlobalBindingUnderALocalEntry(t *testing.T) { + as := startConnectAS(t) + app := connectApp(t, as, &config.Config{}) + writeGlobalConfigHere(t, fmt.Sprintf(`{"profiles":{"agent":{"base_url":%q,"account_id":"999"}}}`, as.srv.URL)) + trustedLocalConfig(t, fmt.Sprintf(`{"profiles":{"agent":{"base_url":%q}}}`, as.srv.URL)) + reloadApp(t, app, as.srv.URL, "agent") + + out, err := runAgentConnect(t, app, "--device-name", "build-box") + require.NoError(t, err, out) + _, err = app.Auth.GetStore().Load("profile:agent") + require.NoError(t, err) +} + +// Where the global entry is replaced whole by a local entry for another +// Basecamp, binding it would not take effect. The connection is refused +// before the operator approves anything, naming the file that hides it. +func TestAuthAgentConnectRefusesToBindWhereTheBindingIsHidden(t *testing.T) { + as := startConnectAS(t) + app := connectApp(t, as, &config.Config{}) + global := fmt.Sprintf(`{"profiles":{"agent":{"base_url":%q}}}`, elsewhereBaseURL) + writeGlobalConfigHere(t, global) + local := trustedLocalConfig(t, fmt.Sprintf(`{"profiles":{"agent":{"base_url":%q}}}`, as.srv.URL)) + reloadApp(t, app, as.srv.URL, "agent") + + out, err := runAgentConnect(t, app, "--device-name", "build-box") + require.Error(t, err, out) + hint := hintOf(t, err) + assert.Contains(t, hint, local, "the file that hides the binding") + assert.Contains(t, hint, filepath.Join(config.GlobalConfigDir(), "config.json")) + assert.Empty(t, as.mints(), "refused before anything is approved") + _, loadErr := app.Auth.GetStore().Load("profile:agent") + assert.Error(t, loadErr, "nothing stored") + data, readErr := os.ReadFile(filepath.Join(config.GlobalConfigDir(), "config.json")) + require.NoError(t, readErr) + assert.JSONEq(t, global, string(data), "the global entry is left as it was") +} + +// A profile only a local config defines has no global entry to bind: the +// refusal names the file it comes from. +func TestAuthAgentConnectNamesTheFileDefiningALocalOnlyProfile(t *testing.T) { + as := startConnectAS(t) + app := connectApp(t, as, &config.Config{}) + writeGlobalConfigHere(t, `{}`) + local := trustedLocalConfig(t, fmt.Sprintf(`{"profiles":{"agent":{"base_url":%q}}}`, as.srv.URL)) + reloadApp(t, app, as.srv.URL, "agent") + + out, err := runAgentConnect(t, app, "--device-name", "build-box") + require.Error(t, err, out) + assert.Contains(t, hintOf(t, err), local) + assert.Empty(t, as.mints()) +} diff --git a/internal/config/config.go b/internal/config/config.go index 5efb2a150..5ec518631 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -52,6 +52,64 @@ type Config struct { // Sources tracks where each value came from (for debugging). Sources map[string]string `json:"-"` + + // ProfileOrigins records, per profile name, the config files its entry + // in Profiles was made from. Set by the file layers only: a profile an + // invocation adds to Profiles in memory has none. + ProfileOrigins map[string]*ProfileOrigin `json:"-"` +} + +// ProfileLayer is one config file's entry for a profile. +type ProfileLayer struct { + Source Source + Path string + BaseURL string +} + +// ProfileOrigin is where a profile's effective entry came from. See +// mergeProfile for how entries from different files combine. +type ProfileOrigin struct { + // Layers are the files whose entries make up the profile, farthest + // first: the first defines it, and each later one refines it. + Layers []ProfileLayer + + // Fields maps each field set on the profile, by its JSON key, to the + // path of the file that set it. + Fields map[string]string + + // Replaced are the files whose entries a closer entry for a different + // Basecamp replaced whole, farthest first. Nothing of them applies. + Replaced []ProfileLayer +} + +// Includes reports whether a file of the given source contributes to the +// profile. +func (o *ProfileOrigin) Includes(source Source) bool { + return o.find(o.Layers, source) != nil +} + +// ReplacedLayer returns the replaced entry from a file of the given source, +// nil when no such entry was replaced. +func (o *ProfileOrigin) ReplacedLayer(source Source) *ProfileLayer { + return o.find(o.Replaced, source) +} + +// Closest is the closest file contributing to the profile: the one whose +// fields win. +func (o *ProfileOrigin) Closest() ProfileLayer { + if len(o.Layers) == 0 { + return ProfileLayer{} + } + return o.Layers[len(o.Layers)-1] +} + +func (o *ProfileOrigin) find(layers []ProfileLayer, source Source) *ProfileLayer { + for i := len(layers) - 1; i >= 0; i-- { + if layers[i].Source == source { + return &layers[i] + } + } + return nil } // IsExperimental returns true if the named experimental feature is enabled. @@ -333,34 +391,9 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { if untrusted { fmt.Fprintf(os.Stderr, "warning: ignoring profiles from %s config at %s\n (authority key from local/repo config; run `basecamp config trust %s` to allow)\n", source, path, ShellQuote(path)) } else { - if cfg.Profiles == nil { - cfg.Profiles = make(map[string]*ProfileConfig) - } for name, profileData := range v { if profileMap, ok := profileData.(map[string]any); ok { - profileCfg := &ProfileConfig{} - if baseURL, ok := profileMap["base_url"].(string); ok && baseURL != "" { - profileCfg.BaseURL = baseURL - } else { - // Skip profiles with empty or missing base_url - continue - } - if accountID := getStringOrNumber(profileMap, "account_id"); accountID != "" { - profileCfg.AccountID = accountID - } - if projectID := getStringOrNumber(profileMap, "project_id"); projectID != "" { - profileCfg.ProjectID = projectID - } - if todolistID := getStringOrNumber(profileMap, "todolist_id"); todolistID != "" { - profileCfg.TodolistID = todolistID - } - if scope, ok := profileMap["scope"].(string); ok { - profileCfg.Scope = scope - } - if clientID, ok := profileMap["client_id"].(string); ok { - profileCfg.ClientID = clientID - } - cfg.Profiles[name] = profileCfg + mergeProfile(cfg, name, profileMap, ProfileLayer{Source: source, Path: path}) } } cfg.Sources["profiles"] = string(source) @@ -368,6 +401,75 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { } } +// mergeProfile layers one config file's entry for a profile over the entry +// the farther files made of it. This is the one place profile entries from +// different files meet, and the rule is: +// +// - An entry with no base_url is skipped, as if the file did not name the +// profile. +// - An entry for the same Basecamp as the one it layers over — the same +// base_url, normalized — refines it field by field. A field the closer +// entry sets wins; a field it leaves unset keeps the farther file's +// value. So a trusted repo or local config that names a profile's +// project does not hide the account the global config binds it to, +// just as an unset top-level key never hides a farther file's. +// - An entry for a different Basecamp replaces the farther one whole. IDs +// and a client from one Basecamp mean nothing on another, so nothing is +// carried across; the replaced files are kept in the origin, since an +// account bound there no longer applies. +// +// Unset means absent: for the IDs also empty (as for the top-level IDs), +// while a present scope or client_id sets the field even when empty. +// +// Every field set records the file that set it, in cfg.ProfileOrigins. +func mergeProfile(cfg *Config, name string, entry map[string]any, layer ProfileLayer) { + baseURL, _ := entry["base_url"].(string) + if baseURL == "" { + return + } + layer.BaseURL = baseURL + if cfg.Profiles == nil { + cfg.Profiles = make(map[string]*ProfileConfig) + } + if cfg.ProfileOrigins == nil { + cfg.ProfileOrigins = make(map[string]*ProfileOrigin) + } + + p, origin := cfg.Profiles[name], cfg.ProfileOrigins[name] + if p == nil || origin == nil || NormalizeBaseURL(p.BaseURL) != NormalizeBaseURL(baseURL) { + replaced := []ProfileLayer(nil) + if p != nil && origin != nil { + replaced = append(append(replaced, origin.Replaced...), origin.Layers...) + } + p = &ProfileConfig{} + origin = &ProfileOrigin{Replaced: replaced, Fields: make(map[string]string)} + cfg.Profiles[name] = p + cfg.ProfileOrigins[name] = origin + } + origin.Layers = append(origin.Layers, layer) + + set := func(field string, dst *string, value string) { + *dst = value + origin.Fields[field] = layer.Path + } + set("base_url", &p.BaseURL, baseURL) + if v := getStringOrNumber(entry, "account_id"); v != "" { + set("account_id", &p.AccountID, v) + } + if v := getStringOrNumber(entry, "project_id"); v != "" { + set("project_id", &p.ProjectID, v) + } + if v := getStringOrNumber(entry, "todolist_id"); v != "" { + set("todolist_id", &p.TodolistID, v) + } + if v, ok := entry["scope"].(string); ok { + set("scope", &p.Scope, v) + } + if v, ok := entry["client_id"].(string); ok { + set("client_id", &p.ClientID, v) + } +} + // LoadFromEnv loads configuration from environment variables. // Exported so root.go can re-apply after profile overlay. // diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 08a2cf278..fa411d059 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -2,6 +2,7 @@ package config import ( "encoding/json" + "fmt" "os" "path/filepath" "testing" @@ -1275,3 +1276,93 @@ func TestNonInteractiveEnv(t *testing.T) { }) } } + +// profileLayer is a config file's contents and the layer it loads as. +type profileLayer struct { + source Source + contents string +} + +// loadProfileLayers loads each file's contents as a trusted layer of the +// given source, in order, farthest first, and returns the config and the +// files' paths. +func loadProfileLayers(t *testing.T, layers ...profileLayer) (*Config, []string) { + t.Helper() + dir := t.TempDir() + trust := NewTrustStore(dir) + cfg := Default() + paths := make([]string, len(layers)) + for i, l := range layers { + paths[i] = filepath.Join(dir, fmt.Sprintf("layer%d", i), "config.json") + require.NoError(t, os.MkdirAll(filepath.Dir(paths[i]), 0o700)) + require.NoError(t, os.WriteFile(paths[i], []byte(l.contents), 0o600)) + require.NoError(t, trust.Trust(paths[i])) + loadFromFile(cfg, paths[i], l.source, trust) + } + return cfg, paths +} + +// A closer entry for the same Basecamp refines the farther one field by +// field: what it sets wins, what it leaves unset is kept. So a local entry +// without an account does not hide the global binding. +func TestProfileEntryForTheSameBasecampRefinesFieldByField(t *testing.T) { + cfg, paths := loadProfileLayers(t, + profileLayer{SourceGlobal, `{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","account_id":999,"project_id":"1","scope":"read","client_id":"c"}}}`}, + profileLayer{SourceLocal, `{"profiles":{"bot":{"base_url":"https://3.basecampapi.com/","account_id":"","project_id":"42","scope":""}}}`}, + ) + + p := cfg.Profiles["bot"] + require.NotNil(t, p) + assert.Equal(t, "https://3.basecampapi.com/", p.BaseURL, "base_urls equal once normalized; the closer spelling wins") + assert.Equal(t, "999", p.AccountID, "an empty account is unset, and hides nothing") + assert.Equal(t, "42", p.ProjectID) + assert.Equal(t, "", p.Scope, "a present scope sets the field, even empty") + assert.Equal(t, "c", p.ClientID) + + origin := cfg.ProfileOrigins["bot"] + require.NotNil(t, origin) + assert.Equal(t, []ProfileLayer{ + {Source: SourceGlobal, Path: paths[0], BaseURL: "https://3.basecampapi.com"}, + {Source: SourceLocal, Path: paths[1], BaseURL: "https://3.basecampapi.com/"}, + }, origin.Layers) + assert.Empty(t, origin.Replaced) + assert.Equal(t, map[string]string{ + "base_url": paths[1], "account_id": paths[0], "project_id": paths[1], "scope": paths[1], "client_id": paths[0], + }, origin.Fields) + assert.True(t, origin.Includes(SourceGlobal)) + assert.Equal(t, paths[1], origin.Closest().Path) +} + +// A closer entry for another Basecamp replaces the farther one whole: no ID +// or client is carried across Basecamps, and the replaced files are kept. +func TestProfileEntryForAnotherBasecampReplacesTheEntryWhole(t *testing.T) { + cfg, paths := loadProfileLayers(t, + profileLayer{SourceSystem, `{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","project_id":"1"}}}`}, + profileLayer{SourceGlobal, `{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","account_id":"999","client_id":"c"}}}`}, + profileLayer{SourceRepo, `{"profiles":{"bot":{"base_url":"http://localhost:3000"}}}`}, + profileLayer{SourceLocal, `{"profiles":{"bot":{"base_url":"http://localhost:3000","project_id":"42"}}}`}, + ) + + assert.Equal(t, &ProfileConfig{BaseURL: "http://localhost:3000", ProjectID: "42"}, cfg.Profiles["bot"]) + origin := cfg.ProfileOrigins["bot"] + assert.False(t, origin.Includes(SourceGlobal)) + assert.Equal(t, []string{paths[2], paths[3]}, []string{origin.Layers[0].Path, origin.Layers[1].Path}) + require.Len(t, origin.Replaced, 2) + hidden := origin.ReplacedLayer(SourceGlobal) + require.NotNil(t, hidden) + assert.Equal(t, paths[1], hidden.Path) + assert.Equal(t, "https://3.basecampapi.com", hidden.BaseURL) + assert.Equal(t, map[string]string{"base_url": paths[3], "project_id": paths[3]}, origin.Fields) +} + +// An entry without a base_url names nothing, and changes nothing. +func TestProfileEntryWithoutABaseURLIsSkipped(t *testing.T) { + cfg, paths := loadProfileLayers(t, + profileLayer{SourceGlobal, `{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","account_id":"999"}}}`}, + profileLayer{SourceLocal, `{"profiles":{"bot":{"account_id":"1"},"orphan":{"project_id":"1"}}}`}, + ) + + assert.Equal(t, "999", cfg.Profiles["bot"].AccountID) + assert.Equal(t, []ProfileLayer{{Source: SourceGlobal, Path: paths[0], BaseURL: "https://3.basecampapi.com"}}, cfg.ProfileOrigins["bot"].Layers) + assert.NotContains(t, cfg.Profiles, "orphan") +} From 0207b23693807a6cf61613b93cde8aedd297c456 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Thu, 17 Sep 2026 12:29:42 +0200 Subject: [PATCH 2/9] Replace a profile entry that names another account, and name the right replacing file An entry naming a different account on the same Basecamp now replaces the farther one whole, so a project or todolist from one account is never carried into another. The hint names the file that replaced the global entry even after a later entry replaced that one, and the test helper resolves the temp dir as the loader does. --- internal/auth/agent.go | 4 +-- internal/commands/connect_unbound_test.go | 16 ++++++++++++ internal/commands/profile.go | 21 ++++++++++++++- .../commands/profile_layers_helpers_test.go | 5 +++- internal/config/config.go | 18 ++++++++----- internal/config/config_test.go | 26 +++++++++++++++++++ 6 files changed, 79 insertions(+), 11 deletions(-) diff --git a/internal/auth/agent.go b/internal/auth/agent.go index cdcd9e316..78181f5f0 100644 --- a/internal/auth/agent.go +++ b/internal/auth/agent.go @@ -526,8 +526,8 @@ func (m *Manager) agentLoginCommand(clientID, scope string) string { // An entry that EXISTS without an account deliberately gets no // --account. The login binds one there only when the global config // file is one of the layers the profile is made of — a repo or local - // entry for another Basecamp, or one the global file lacks, would make - // this command fail — and a command handed over to be run verbatim + // entry for another Basecamp, or a profile only a system, repo or local + // config defines, would make this command fail — and a command handed over to be run verbatim // should not be one that cannot work. Without it the login refuses by // naming exactly what it needs, the file to change included, which is // better guidance than a guess. diff --git a/internal/commands/connect_unbound_test.go b/internal/commands/connect_unbound_test.go index e5fc9b2dd..fdabb1cb0 100644 --- a/internal/commands/connect_unbound_test.go +++ b/internal/commands/connect_unbound_test.go @@ -93,3 +93,19 @@ func TestConnectAccountNamesTheFileOfAnUnusableAccount(t *testing.T) { require.Error(t, err) assert.Equal(t, "Correct account_id in the profile's entry in "+local+".", hintOf(t, err)) } + +// The file that hid the global entry is the first after it on another +// Basecamp, even when a closer entry is back on the global entry's Basecamp +// and replaced that file in turn. +func TestGlobalBindingBlockerNamesTheFileThatReplacedTheGlobalEntry(t *testing.T) { + global := config.ProfileLayer{Source: config.SourceGlobal, Path: "/g/config.json", BaseURL: "https://3.basecampapi.com"} + repo := config.ProfileLayer{Source: config.SourceRepo, Path: "/r/config.json", BaseURL: "http://localhost:3000"} + local := config.ProfileLayer{Source: config.SourceLocal, Path: "/l/config.json", BaseURL: "https://3.basecampapi.com/"} + cfg := &config.Config{ProfileOrigins: map[string]*config.ProfileOrigin{ + "agent": {Replaced: []config.ProfileLayer{global, repo}, Layers: []config.ProfileLayer{local}}, + }} + + assert.Equal(t, + "Its entry in /r/config.json is for http://localhost:3000, not https://3.basecampapi.com, so it replaces the global config's entry in /g/config.json and any account bound there. Add account_id to the profile's entry in /l/config.json", + globalBindingBlocker(cfg, "agent")) +} diff --git a/internal/commands/profile.go b/internal/commands/profile.go index 52c343bba..83a00d9c7 100644 --- a/internal/commands/profile.go +++ b/internal/commands/profile.go @@ -581,7 +581,7 @@ func globalBindingBlocker(cfg *config.Config, name string) string { } closest := richtext.SanitizeSingleLine(origin.Closest().Path) if hidden := origin.ReplacedLayer(config.SourceGlobal); hidden != nil { - replacer := origin.Layers[0] + replacer := firstEntryForAnotherBasecamp(origin, *hidden) return fmt.Sprintf("Its entry in %s is for %s, not %s, so it replaces the global config's entry in %s and any account bound there. Add account_id to the profile's entry in %s", richtext.SanitizeSingleLine(replacer.Path), richtext.SanitizeSingleLine(replacer.BaseURL), richtext.SanitizeSingleLine(hidden.BaseURL), richtext.SanitizeSingleLine(hidden.Path), closest) @@ -589,6 +589,25 @@ func globalBindingBlocker(cfg *config.Config, name string) string { return fmt.Sprintf("Its entry comes from %s, not the global config, so no command can bind it. Add account_id to the profile's entry there", closest) } +// firstEntryForAnotherBasecamp is the entry that replaced hidden for being +// on another Basecamp: the first, after it, whose base_url differs. A later +// entry can be back on hidden's Basecamp, having replaced the one that did. +// An entry that replaced it for naming another account sets one, and a +// profile with no account has none of those left; Layers[0] is the fallback. +func firstEntryForAnotherBasecamp(origin *config.ProfileOrigin, hidden config.ProfileLayer) config.ProfileLayer { + after := false + for _, l := range append(append([]config.ProfileLayer(nil), origin.Replaced...), origin.Layers...) { + if l == hidden { + after = true + continue + } + if after && config.NormalizeBaseURL(l.BaseURL) != config.NormalizeBaseURL(hidden.BaseURL) { + return l + } + } + return origin.Layers[0] +} + // bindProfileAccount sets the account on an existing profile entry in the // global config file, leaving every other field of the entry as it is. func bindProfileAccount(name, account string) error { diff --git a/internal/commands/profile_layers_helpers_test.go b/internal/commands/profile_layers_helpers_test.go index 0a5eddb3c..791faaa0b 100644 --- a/internal/commands/profile_layers_helpers_test.go +++ b/internal/commands/profile_layers_helpers_test.go @@ -24,7 +24,10 @@ func trustedLocalConfig(t *testing.T, contents string) string { home := t.TempDir() t.Setenv("HOME", home) t.Setenv("USERPROFILE", home) - dir := t.TempDir() + // Resolved, as the config loader resolves the working directory, so + // the path is the one hints name where the temp dir is a symlink. + dir, err := filepath.EvalSymlinks(t.TempDir()) + require.NoError(t, err) path := filepath.Join(dir, ".basecamp", "config.json") require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o700)) require.NoError(t, os.WriteFile(path, []byte(contents), 0o600)) diff --git a/internal/config/config.go b/internal/config/config.go index 5ec518631..a89d3cbcc 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -413,10 +413,12 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { // value. So a trusted repo or local config that names a profile's // project does not hide the account the global config binds it to, // just as an unset top-level key never hides a farther file's. -// - An entry for a different Basecamp replaces the farther one whole. IDs -// and a client from one Basecamp mean nothing on another, so nothing is -// carried across; the replaced files are kept in the origin, since an -// account bound there no longer applies. +// - An entry for a different Basecamp, or for a different account on the +// same one (both name an account_id, and they differ), replaces the +// farther one whole. A project, todolist or client from one account +// means nothing in another, so nothing is carried across; the replaced +// files are kept in the origin, since an account bound there no longer +// applies. // // Unset means absent: for the IDs also empty (as for the top-level IDs), // while a present scope or client_id sets the field even when empty. @@ -436,7 +438,9 @@ func mergeProfile(cfg *Config, name string, entry map[string]any, layer ProfileL } p, origin := cfg.Profiles[name], cfg.ProfileOrigins[name] - if p == nil || origin == nil || NormalizeBaseURL(p.BaseURL) != NormalizeBaseURL(baseURL) { + account := getStringOrNumber(entry, "account_id") + otherAccount := p != nil && p.AccountID != "" && account != "" && p.AccountID != account + if p == nil || origin == nil || otherAccount || NormalizeBaseURL(p.BaseURL) != NormalizeBaseURL(baseURL) { replaced := []ProfileLayer(nil) if p != nil && origin != nil { replaced = append(append(replaced, origin.Replaced...), origin.Layers...) @@ -453,8 +457,8 @@ func mergeProfile(cfg *Config, name string, entry map[string]any, layer ProfileL origin.Fields[field] = layer.Path } set("base_url", &p.BaseURL, baseURL) - if v := getStringOrNumber(entry, "account_id"); v != "" { - set("account_id", &p.AccountID, v) + if account != "" { + set("account_id", &p.AccountID, account) } if v := getStringOrNumber(entry, "project_id"); v != "" { set("project_id", &p.ProjectID, v) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index fa411d059..e8e2f6287 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1366,3 +1366,29 @@ func TestProfileEntryWithoutABaseURLIsSkipped(t *testing.T) { assert.Equal(t, []ProfileLayer{{Source: SourceGlobal, Path: paths[0], BaseURL: "https://3.basecampapi.com"}}, cfg.ProfileOrigins["bot"].Layers) assert.NotContains(t, cfg.Profiles, "orphan") } + +// A closer entry that names another account on the same Basecamp is another +// identity: the farther entry's project, todolist and client belong to the +// farther account and are not carried across. +func TestProfileEntryForAnotherAccountReplacesTheEntryWhole(t *testing.T) { + cfg, paths := loadProfileLayers(t, + profileLayer{SourceGlobal, `{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","account_id":"999","project_id":"1","todolist_id":"2","client_id":"c"}}}`}, + profileLayer{SourceRepo, `{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","account_id":555}}}`}, + profileLayer{SourceLocal, `{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","account_id":"555","project_id":"42"}}}`}, + ) + + assert.Equal(t, &ProfileConfig{BaseURL: "https://3.basecampapi.com", AccountID: "555", ProjectID: "42"}, cfg.Profiles["bot"]) + origin := cfg.ProfileOrigins["bot"] + assert.Equal(t, paths[0], origin.ReplacedLayer(SourceGlobal).Path) + assert.Equal(t, []string{paths[1], paths[2]}, []string{origin.Layers[0].Path, origin.Layers[1].Path}, "the same account refines") +} + +// A closer entry that binds an unbound farther one refines it. +func TestProfileEntryBindingAnUnboundOneRefinesIt(t *testing.T) { + cfg, _ := loadProfileLayers(t, + profileLayer{SourceGlobal, `{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","project_id":"1"}}}`}, + profileLayer{SourceLocal, `{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","account_id":"555"}}}`}, + ) + + assert.Equal(t, &ProfileConfig{BaseURL: "https://3.basecampapi.com", AccountID: "555", ProjectID: "1"}, cfg.Profiles["bot"]) +} From cce26933f1cb47849435c5d213ac0140229186eb Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Thu, 17 Sep 2026 12:52:17 +0200 Subject: [PATCH 3/9] Compare accounts as numbers, record what replaced an entry, and name the files Account IDs in the merge are compared as numbers, as the commands compare them. Each replaced entry now records the entry that replaced it, so the hint names that file and says whether it was another Basecamp or another account. A profile no global entry defines is told it can have one, which the closer entries refine, rather than being sent to write an operator's account into a shared repo config. Messages about a profile's account name the file it is bound in. --- internal/commands/auth.go | 2 +- internal/commands/auth_agent.go | 2 +- internal/commands/connect.go | 6 +- internal/commands/connect_unbound_test.go | 35 +++++++---- internal/commands/profile.go | 58 +++++++++++------- internal/commands/profile_layers_test.go | 16 +++++ internal/config/config.go | 72 +++++++++++++++++------ internal/config/config_test.go | 22 ++++++- 8 files changed, 155 insertions(+), 58 deletions(-) diff --git a/internal/commands/auth.go b/internal/commands/auth.go index 5651045e0..c85c0bcbc 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -1160,7 +1160,7 @@ func requireProfileBinding(app *appctx.App, name string, existing *config.Profil return output.ErrUsageHint(fmt.Sprintf("Profile %q is bound to %s, not %s", name, existing.BaseURL, app.Config.BaseURL), "Use a different profile, or drop the BASECAMP_BASE_URL override.") case existing.AccountID != "" && app.Config.AccountID != "" && !accountIDsEqual(app.Config.AccountID, existing.AccountID): - return output.ErrUsageHint(fmt.Sprintf("Profile %q is bound to account %s, not %s", name, existing.AccountID, app.Config.AccountID), + return output.ErrUsageHint(fmt.Sprintf("Profile %q is bound to account %s%s, not %s", name, existing.AccountID, boundIn(app.Config, name), app.Config.AccountID), "Use a different profile, or drop the --account / BASECAMP_ACCOUNT_ID override.") } return nil diff --git a/internal/commands/auth_agent.go b/internal/commands/auth_agent.go index a35a5a2bd..ef07dfcd7 100644 --- a/internal/commands/auth_agent.go +++ b/internal/commands/auth_agent.go @@ -275,7 +275,7 @@ func (t *agentConnectTarget) commit(app *appctx.App, conn *auth.AgentConnection) t.existing.AccountID = conn.AccountID case !accountIDsEqual(t.existing.AccountID, conn.AccountID): return false, output.ErrUsageHint( - fmt.Sprintf("Profile %q is bound to account %s, and the approved agent belongs to account %s", t.name, t.existing.AccountID, conn.AccountID), + fmt.Sprintf("Profile %q is bound to account %s%s, and the approved agent belongs to account %s", t.name, t.existing.AccountID, boundIn(app.Config, t.name), conn.AccountID), "Nothing was stored. Connect the agent under a profile of its own: -P .") } return isDefault, nil diff --git a/internal/commands/connect.go b/internal/commands/connect.go index 365f30819..e48507027 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -867,8 +867,8 @@ func connectAccount(app *appctx.App, name string) (string, error) { // profile: say what is wrong with the account it does name. if p.AccountID != "" { where := "the config file that defines the profile" - if origin := app.Config.ProfileOrigins[name]; origin != nil && origin.Fields["account_id"] != "" { - where = "the profile's entry in " + richtext.SanitizeSingleLine(origin.Fields["account_id"]) + if path := profileFieldFile(app.Config, name, "account_id"); path != "" { + where = "the profile's entry in " + richtext.SanitizeSingleLine(path) } return "", output.ErrUsageHint( fmt.Sprintf("Profile %q names account %q, which is not an account ID", name, p.AccountID), @@ -878,7 +878,7 @@ func connectAccount(app *appctx.App, name string) (string, error) { } 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), + fmt.Sprintf("Profile %q is bound to account %s%s, and this command named account %s", name, bound, boundIn(app.Config, name), app.Config.AccountID), "Setup works in the profile's own account. Drop --account (or BASECAMP_ACCOUNT_ID).") } return bound, nil diff --git a/internal/commands/connect_unbound_test.go b/internal/commands/connect_unbound_test.go index fdabb1cb0..bef202b56 100644 --- a/internal/commands/connect_unbound_test.go +++ b/internal/commands/connect_unbound_test.go @@ -53,7 +53,10 @@ func TestUnboundProfileHintNamesTheConfigFileWhenTheEntryIsNotGlobal(t *testing. hint := hintOf(t, unboundProfileError(loadConfig(t), "agent")) assert.Contains(t, hint, "Its entry comes from "+local+", not the global config") - assert.Contains(t, hint, "Add account_id to the profile's entry there") + assert.Contains(t, hint, "Add an entry for it to "+filepath.Join(config.GlobalConfigDir(), "config.json")+ + ", with the same base_url (https://3.basecampapi.com) and an account_id", + "a global entry for the same Basecamp is refined by this one, so the account in it holds") + assert.Contains(t, hint, "or add account_id to the entry in "+local) assert.NotContains(t, hint, "basecamp auth", "a command that would refuse this profile is no remedy") } @@ -94,18 +97,26 @@ func TestConnectAccountNamesTheFileOfAnUnusableAccount(t *testing.T) { assert.Equal(t, "Correct account_id in the profile's entry in "+local+".", hintOf(t, err)) } -// The file that hid the global entry is the first after it on another -// Basecamp, even when a closer entry is back on the global entry's Basecamp -// and replaced that file in turn. -func TestGlobalBindingBlockerNamesTheFileThatReplacedTheGlobalEntry(t *testing.T) { +// The hint names the entry that actually replaced the global one, and why, +// even when a closer entry has since replaced that one in turn. +func TestGlobalBindingBlockerNamesTheEntryThatReplacedTheGlobalOne(t *testing.T) { global := config.ProfileLayer{Source: config.SourceGlobal, Path: "/g/config.json", BaseURL: "https://3.basecampapi.com"} - repo := config.ProfileLayer{Source: config.SourceRepo, Path: "/r/config.json", BaseURL: "http://localhost:3000"} - local := config.ProfileLayer{Source: config.SourceLocal, Path: "/l/config.json", BaseURL: "https://3.basecampapi.com/"} - cfg := &config.Config{ProfileOrigins: map[string]*config.ProfileOrigin{ - "agent": {Replaced: []config.ProfileLayer{global, repo}, Layers: []config.ProfileLayer{local}}, - }} + repo := config.ProfileLayer{Source: config.SourceRepo, Path: "/r/config.json", BaseURL: "https://3.basecampapi.com/"} + local := config.ProfileLayer{Source: config.SourceLocal, Path: "/l/config.json", BaseURL: "http://localhost:3000"} + origin := func(by config.ProfileLayer) *config.Config { + return &config.Config{ProfileOrigins: map[string]*config.ProfileOrigin{ + "agent": { + Replaced: []config.ReplacedProfileLayer{{ProfileLayer: global, By: by}, {ProfileLayer: repo, By: local}}, + Layers: []config.ProfileLayer{local}, + }, + }} + } + + assert.Equal(t, + "Its entry in /r/config.json names another account, so it replaces the global config's entry in /g/config.json and any account bound there. Add account_id to the profile's entry in /l/config.json", + globalBindingBlocker(origin(repo), "agent"), "the repo entry replaced it, for another account on the same Basecamp") assert.Equal(t, - "Its entry in /r/config.json is for http://localhost:3000, not https://3.basecampapi.com, so it replaces the global config's entry in /g/config.json and any account bound there. Add account_id to the profile's entry in /l/config.json", - globalBindingBlocker(cfg, "agent")) + "Its entry in /l/config.json is for http://localhost:3000, not https://3.basecampapi.com, so it replaces the global config's entry in /g/config.json and any account bound there. Add account_id to the profile's entry in /l/config.json", + globalBindingBlocker(origin(local), "agent"), "a replacement for another Basecamp says which") } diff --git a/internal/commands/profile.go b/internal/commands/profile.go index 83a00d9c7..1bf4c0b63 100644 --- a/internal/commands/profile.go +++ b/internal/commands/profile.go @@ -579,33 +579,47 @@ func globalBindingBlocker(cfg *config.Config, name string) string { if origin.Includes(config.SourceGlobal) { return "" } - closest := richtext.SanitizeSingleLine(origin.Closest().Path) + closest := origin.Closest() if hidden := origin.ReplacedLayer(config.SourceGlobal); hidden != nil { - replacer := firstEntryForAnotherBasecamp(origin, *hidden) - return fmt.Sprintf("Its entry in %s is for %s, not %s, so it replaces the global config's entry in %s and any account bound there. Add account_id to the profile's entry in %s", - richtext.SanitizeSingleLine(replacer.Path), richtext.SanitizeSingleLine(replacer.BaseURL), - richtext.SanitizeSingleLine(hidden.BaseURL), richtext.SanitizeSingleLine(hidden.Path), closest) + // The entry that replaced it did so for being on another Basecamp, + // or for naming another account there. + why := fmt.Sprintf("is for %s, not %s", richtext.SanitizeSingleLine(hidden.By.BaseURL), richtext.SanitizeSingleLine(hidden.BaseURL)) + if config.NormalizeBaseURL(hidden.By.BaseURL) == config.NormalizeBaseURL(hidden.BaseURL) { + why = "names another account" + } + return fmt.Sprintf("Its entry in %s %s, so it replaces the global config's entry in %s and any account bound there. Add account_id to the profile's entry in %s", + richtext.SanitizeSingleLine(hidden.By.Path), why, richtext.SanitizeSingleLine(hidden.Path), richtext.SanitizeSingleLine(closest.Path)) + } + // The global config has no entry for this profile, so there is nothing + // to bind — but an entry added there for the same Basecamp is refined + // by the ones that do define it, and the account in it holds. That is + // the remedy to name first: a repo config is shared, and an operator's + // account does not belong in it (nor can they always write /etc). + return fmt.Sprintf("Its entry comes from %s, not the global config, so no command can bind it. Add an entry for it to %s, with the same base_url (%s) and an account_id — or add account_id to the entry in %s", + richtext.SanitizeSingleLine(closest.Path), + richtext.SanitizeSingleLine(filepath.Join(config.GlobalConfigDir(), "config.json")), + richtext.SanitizeSingleLine(closest.BaseURL), + richtext.SanitizeSingleLine(closest.Path)) +} + +// boundIn names the config file a profile's account came from, as " (bound +// in )" for a message about that account, and is empty for an entry +// this invocation made rather than a file. +func boundIn(cfg *config.Config, name string) string { + if path := profileFieldFile(cfg, name, "account_id"); path != "" { + return " (bound in " + richtext.SanitizeSingleLine(path) + ")" } - return fmt.Sprintf("Its entry comes from %s, not the global config, so no command can bind it. Add account_id to the profile's entry there", closest) + return "" } -// firstEntryForAnotherBasecamp is the entry that replaced hidden for being -// on another Basecamp: the first, after it, whose base_url differs. A later -// entry can be back on hidden's Basecamp, having replaced the one that did. -// An entry that replaced it for naming another account sets one, and a -// profile with no account has none of those left; Layers[0] is the fallback. -func firstEntryForAnotherBasecamp(origin *config.ProfileOrigin, hidden config.ProfileLayer) config.ProfileLayer { - after := false - for _, l := range append(append([]config.ProfileLayer(nil), origin.Replaced...), origin.Layers...) { - if l == hidden { - after = true - continue - } - if after && config.NormalizeBaseURL(l.BaseURL) != config.NormalizeBaseURL(hidden.BaseURL) { - return l - } +// profileFieldFile is the path of the config file that set a field on a +// profile, "" when the profile did not come from files. +func profileFieldFile(cfg *config.Config, name, field string) string { + origin := cfg.ProfileOrigins[name] + if origin == nil { + return "" } - return origin.Layers[0] + return origin.Fields[field] } // bindProfileAccount sets the account on an existing profile entry in the diff --git a/internal/commands/profile_layers_test.go b/internal/commands/profile_layers_test.go index aa6d3e8e8..c102077a1 100644 --- a/internal/commands/profile_layers_test.go +++ b/internal/commands/profile_layers_test.go @@ -128,3 +128,19 @@ func TestAuthAgentConnectNamesTheFileDefiningALocalOnlyProfile(t *testing.T) { assert.Contains(t, hintOf(t, err), local) assert.Empty(t, as.mints()) } + +// A profile bound to another account refuses the connection, naming the +// file the account is bound in — the global config, seen through the local +// entry that refines it. +func TestAuthAgentConnectNamesTheFileAProfileIsBoundIn(t *testing.T) { + as := startConnectAS(t) + app := connectApp(t, as, &config.Config{}) + writeGlobalConfigHere(t, fmt.Sprintf(`{"profiles":{"agent":{"base_url":%q,"account_id":"123"}}}`, as.srv.URL)) + trustedLocalConfig(t, fmt.Sprintf(`{"profiles":{"agent":{"base_url":%q,"project_id":"42"}}}`, as.srv.URL)) + reloadApp(t, app, as.srv.URL, "agent") + + _, err := runAgentConnect(t, app, "--device-name", "build-box") + + require.Error(t, err) + assert.Contains(t, err.Error(), "bound to account 123 (bound in "+filepath.Join(config.GlobalConfigDir(), "config.json")+")") +} diff --git a/internal/config/config.go b/internal/config/config.go index a89d3cbcc..d9d957bdc 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -77,21 +77,42 @@ type ProfileOrigin struct { // path of the file that set it. Fields map[string]string - // Replaced are the files whose entries a closer entry for a different - // Basecamp replaced whole, farthest first. Nothing of them applies. - Replaced []ProfileLayer + // Replaced are the entries a closer one replaced whole, farthest + // first, each with the entry that replaced it. Nothing of them + // applies. + Replaced []ReplacedProfileLayer +} + +// ReplacedProfileLayer is a file's entry for a profile that a closer entry +// replaced whole, and the entry that replaced it. +type ReplacedProfileLayer struct { + ProfileLayer + + // By is the entry that replaced it: for another Basecamp, or for + // another account on the same one. + By ProfileLayer } // Includes reports whether a file of the given source contributes to the // profile. func (o *ProfileOrigin) Includes(source Source) bool { - return o.find(o.Layers, source) != nil + for _, l := range o.Layers { + if l.Source == source { + return true + } + } + return false } // ReplacedLayer returns the replaced entry from a file of the given source, // nil when no such entry was replaced. -func (o *ProfileOrigin) ReplacedLayer(source Source) *ProfileLayer { - return o.find(o.Replaced, source) +func (o *ProfileOrigin) ReplacedLayer(source Source) *ReplacedProfileLayer { + for i := len(o.Replaced) - 1; i >= 0; i-- { + if o.Replaced[i].Source == source { + return &o.Replaced[i] + } + } + return nil } // Closest is the closest file contributing to the profile: the one whose @@ -103,15 +124,6 @@ func (o *ProfileOrigin) Closest() ProfileLayer { return o.Layers[len(o.Layers)-1] } -func (o *ProfileOrigin) find(layers []ProfileLayer, source Source) *ProfileLayer { - for i := len(layers) - 1; i >= 0; i-- { - if layers[i].Source == source { - return &layers[i] - } - } - return nil -} - // IsExperimental returns true if the named experimental feature is enabled. func (c *Config) IsExperimental(name string) bool { if c.Experimental == nil { @@ -396,6 +408,9 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { mergeProfile(cfg, name, profileMap, ProfileLayer{Source: source, Path: path}) } } + // An aggregate: the last file that contributed any profile. + // Which file each profile and field came from is recorded in + // ProfileOrigins, and nothing is decided from this. cfg.Sources["profiles"] = string(source) } } @@ -439,11 +454,14 @@ func mergeProfile(cfg *Config, name string, entry map[string]any, layer ProfileL p, origin := cfg.Profiles[name], cfg.ProfileOrigins[name] account := getStringOrNumber(entry, "account_id") - otherAccount := p != nil && p.AccountID != "" && account != "" && p.AccountID != account + otherAccount := p != nil && p.AccountID != "" && account != "" && !sameAccountID(p.AccountID, account) if p == nil || origin == nil || otherAccount || NormalizeBaseURL(p.BaseURL) != NormalizeBaseURL(baseURL) { - replaced := []ProfileLayer(nil) + replaced := []ReplacedProfileLayer(nil) if p != nil && origin != nil { - replaced = append(append(replaced, origin.Replaced...), origin.Layers...) + replaced = append(replaced, origin.Replaced...) + for _, l := range origin.Layers { + replaced = append(replaced, ReplacedProfileLayer{ProfileLayer: l, By: layer}) + } } p = &ProfileConfig{} origin = &ProfileOrigin{Replaced: replaced, Fields: make(map[string]string)} @@ -474,6 +492,24 @@ func mergeProfile(cfg *Config, name string, entry map[string]any, layer ProfileL } } +// sameAccountID reports whether two account IDs name the same account: equal +// as decimal numbers, so "0999" is "999", however many digits they run to. +// A value that is not all digits is the same only as its exact spelling. +func sameAccountID(a, b string) bool { + if a == b { + return true + } + canonical := func(id string) (string, bool) { + if id == "" || strings.TrimLeft(id, "0123456789") != "" { + return "", false + } + return strings.TrimLeft(id, "0"), true + } + ca, okA := canonical(a) + cb, okB := canonical(b) + return okA && okB && ca == cb +} + // LoadFromEnv loads configuration from environment variables. // Exported so root.go can re-apply after profile overlay. // diff --git a/internal/config/config_test.go b/internal/config/config_test.go index e8e2f6287..d39561234 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1326,6 +1326,7 @@ func TestProfileEntryForTheSameBasecampRefinesFieldByField(t *testing.T) { {Source: SourceLocal, Path: paths[1], BaseURL: "https://3.basecampapi.com/"}, }, origin.Layers) assert.Empty(t, origin.Replaced) + assert.Empty(t, origin.Replaced) assert.Equal(t, map[string]string{ "base_url": paths[1], "account_id": paths[0], "project_id": paths[1], "scope": paths[1], "client_id": paths[0], }, origin.Fields) @@ -1352,6 +1353,7 @@ func TestProfileEntryForAnotherBasecampReplacesTheEntryWhole(t *testing.T) { require.NotNil(t, hidden) assert.Equal(t, paths[1], hidden.Path) assert.Equal(t, "https://3.basecampapi.com", hidden.BaseURL) + assert.Equal(t, paths[2], hidden.By.Path, "the repo entry, for another Basecamp, replaced it") assert.Equal(t, map[string]string{"base_url": paths[3], "project_id": paths[3]}, origin.Fields) } @@ -1379,10 +1381,28 @@ func TestProfileEntryForAnotherAccountReplacesTheEntryWhole(t *testing.T) { assert.Equal(t, &ProfileConfig{BaseURL: "https://3.basecampapi.com", AccountID: "555", ProjectID: "42"}, cfg.Profiles["bot"]) origin := cfg.ProfileOrigins["bot"] - assert.Equal(t, paths[0], origin.ReplacedLayer(SourceGlobal).Path) + hidden := origin.ReplacedLayer(SourceGlobal) + assert.Equal(t, paths[0], hidden.Path) + assert.Equal(t, paths[1], hidden.By.Path, "the repo entry, for another account, replaced it") assert.Equal(t, []string{paths[1], paths[2]}, []string{origin.Layers[0].Path, origin.Layers[1].Path}, "the same account refines") } +// Account IDs are compared as numbers, as the commands compare them: a +// spelling with leading zeros is the same account, and refines. +func TestProfileEntryForTheSameAccountSpelledDifferentlyRefines(t *testing.T) { + cfg, _ := loadProfileLayers(t, + profileLayer{SourceGlobal, `{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","account_id":"999","project_id":"1"}}}`}, + profileLayer{SourceLocal, `{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","account_id":"0999"}}}`}, + ) + + assert.Equal(t, "1", cfg.Profiles["bot"].ProjectID) + assert.True(t, sameAccountID("000123456789012345678901234567890", "123456789012345678901234567890"), "past int64") + assert.True(t, sameAccountID("0", "000")) + assert.False(t, sameAccountID("999", "9990")) + assert.False(t, sameAccountID("x999", "999")) + assert.True(t, sameAccountID("abc", "abc")) +} + // A closer entry that binds an unbound farther one refines it. func TestProfileEntryBindingAnUnboundOneRefinesIt(t *testing.T) { cfg, _ := loadProfileLayers(t, From ef77975d673e1cf87767b532a1ae01e35d490787 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Thu, 17 Sep 2026 13:20:59 +0200 Subject: [PATCH 4/9] Say nothing about files when the global config cannot be read A global config the loader could not parse is skipped, profiles and all, so the layers it recorded are not evidence about which file defines a profile. The blocker now says nothing in that case, and the headless logins prove the global file can take a write before they do, as the connection already did: all three commands report the unusable file itself. The hint for a profile with no global entry names base_url as something to give the entry, since an entry there may exist without one. --- internal/auth/agent.go | 6 ++--- internal/commands/auth.go | 6 +++++ internal/commands/connect_unbound_test.go | 4 ++-- internal/commands/profile.go | 27 ++++++++++++++++++++++- internal/commands/profile_layers_test.go | 25 +++++++++++++++++++++ internal/config/config_test.go | 1 - 6 files changed, 62 insertions(+), 7 deletions(-) diff --git a/internal/auth/agent.go b/internal/auth/agent.go index 78181f5f0..a01b0c301 100644 --- a/internal/auth/agent.go +++ b/internal/auth/agent.go @@ -526,9 +526,9 @@ func (m *Manager) agentLoginCommand(clientID, scope string) string { // An entry that EXISTS without an account deliberately gets no // --account. The login binds one there only when the global config // file is one of the layers the profile is made of — a repo or local - // entry for another Basecamp, or a profile only a system, repo or local - // config defines, would make this command fail — and a command handed over to be run verbatim - // should not be one that cannot work. Without it the login refuses by + // entry for another Basecamp, or a profile only a system, repo or + // local config defines, would make this command fail — and a command + // handed over to be run verbatim should not be one that cannot work. Without it the login refuses by // naming exactly what it needs, the file to change included, which is // better guidance than a guess. if entry, registered := m.cfg.Profiles[m.cfg.ActiveProfile]; !registered || entry == nil { diff --git a/internal/commands/auth.go b/internal/commands/auth.go index c85c0bcbc..bcf38e7e0 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -855,6 +855,12 @@ func resolveHeadlessProfile(app *appctx.App, login headlessLogin) (*profileTarge } blocker := "" if existing != nil && existing.AccountID == "" { + // Binding rewrites the global config file: a file the writers would + // refuse is reported as itself, before anything is said about which + // file defines the profile, and before the secret is consumed. + if err := globalConfigTakesProfiles(); err != nil { + return nil, err + } blocker = globalBindingBlocker(app.Config, name) } diff --git a/internal/commands/connect_unbound_test.go b/internal/commands/connect_unbound_test.go index bef202b56..fcf726b91 100644 --- a/internal/commands/connect_unbound_test.go +++ b/internal/commands/connect_unbound_test.go @@ -53,8 +53,8 @@ func TestUnboundProfileHintNamesTheConfigFileWhenTheEntryIsNotGlobal(t *testing. hint := hintOf(t, unboundProfileError(loadConfig(t), "agent")) assert.Contains(t, hint, "Its entry comes from "+local+", not the global config") - assert.Contains(t, hint, "Add an entry for it to "+filepath.Join(config.GlobalConfigDir(), "config.json")+ - ", with the same base_url (https://3.basecampapi.com) and an account_id", + assert.Contains(t, hint, "Give it an entry in "+filepath.Join(config.GlobalConfigDir(), "config.json")+ + " with base_url https://3.basecampapi.com and an account_id", "a global entry for the same Basecamp is refined by this one, so the account in it holds") assert.Contains(t, hint, "or add account_id to the entry in "+local) assert.NotContains(t, hint, "basecamp auth", "a command that would refuse this profile is no remedy") diff --git a/internal/commands/profile.go b/internal/commands/profile.go index 1bf4c0b63..0187339b8 100644 --- a/internal/commands/profile.go +++ b/internal/commands/profile.go @@ -569,6 +569,14 @@ func globalProfileEntry(configData map[string]any, name string) map[string]any { // for another Basecamp replaced its entry whole, or it has no entry, and the // account has to go into a file that is. func globalBindingBlocker(cfg *config.Config, name string) string { + if globalConfigUnusable() { + // A global config the loader could not read or parse was skipped, + // so the layers say nothing about which file defines the profile — + // the one that does may be the file that was skipped. Say nothing + // about files: the commands that bind report the unusable file + // itself, in full, before they write. + return "" + } origin := cfg.ProfileOrigins[name] if origin == nil || len(origin.Layers) == 0 { // Only a config file defines a profile; an entry with no origin was @@ -595,13 +603,30 @@ func globalBindingBlocker(cfg *config.Config, name string) string { // by the ones that do define it, and the account in it holds. That is // the remedy to name first: a repo config is shared, and an operator's // account does not belong in it (nor can they always write /etc). - return fmt.Sprintf("Its entry comes from %s, not the global config, so no command can bind it. Add an entry for it to %s, with the same base_url (%s) and an account_id — or add account_id to the entry in %s", + return fmt.Sprintf("Its entry comes from %s, not the global config, so no command can bind it. Give it an entry in %s with base_url %s and an account_id — or add account_id to the entry in %s", richtext.SanitizeSingleLine(closest.Path), richtext.SanitizeSingleLine(filepath.Join(config.GlobalConfigDir(), "config.json")), richtext.SanitizeSingleLine(closest.BaseURL), richtext.SanitizeSingleLine(closest.Path)) } +// globalConfigUnusable reports whether the global config file exists but +// cannot be read or parsed. config.Load skips such a file, profiles and +// all, so what it recorded about where a profile comes from is not +// evidence: the entry that defines it may be in the file that was skipped. +func globalConfigUnusable() bool { + path := filepath.Join(config.GlobalConfigDir(), "config.json") + data, err := os.ReadFile(path) //nolint:gosec // G304: the global config path + if os.IsNotExist(err) { + return false + } + if err != nil { + return true + } + var parsed map[string]any + return json.Unmarshal(data, &parsed) != nil || parsed == nil +} + // boundIn names the config file a profile's account came from, as " (bound // in )" for a message about that account, and is empty for an entry // this invocation made rather than a file. diff --git a/internal/commands/profile_layers_test.go b/internal/commands/profile_layers_test.go index c102077a1..6063c66b2 100644 --- a/internal/commands/profile_layers_test.go +++ b/internal/commands/profile_layers_test.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -144,3 +145,27 @@ func TestAuthAgentConnectNamesTheFileAProfileIsBoundIn(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "bound to account 123 (bound in "+filepath.Join(config.GlobalConfigDir(), "config.json")+")") } + +// A global config that cannot be parsed was skipped by the loader, so what +// it recorded about the profile's files proves nothing. Both binding paths +// report the unusable file itself rather than telling the operator their +// entry comes from somewhere else. +func TestUnusableGlobalConfigIsReportedAsItself(t *testing.T) { + as := startConnectAS(t) + app := connectApp(t, as, &config.Config{}) + writeGlobalConfigHere(t, fmt.Sprintf(`{"profiles":{"agent":{"base_url":%q,"account_id":"999"`, as.srv.URL)) + trustedLocalConfig(t, fmt.Sprintf(`{"profiles":{"agent":{"base_url":%q}}}`, as.srv.URL)) + reloadApp(t, app, as.srv.URL, "agent") + + _, err := runAgentConnect(t, app, "--device-name", "build-box") + require.Error(t, err) + assert.Contains(t, err.Error(), "is not valid JSON, refusing to rewrite it") + assert.Empty(t, as.mints()) + + in := strings.NewReader("bc_at_secret") + _, err = runLogin(t, app, in, "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), "is not valid JSON, refusing to rewrite it") + assert.NotContains(t, err.Error(), "not the global config") + assert.Equal(t, 12, in.Len(), "refused before stdin is read") +} diff --git a/internal/config/config_test.go b/internal/config/config_test.go index d39561234..c60ab93cb 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1326,7 +1326,6 @@ func TestProfileEntryForTheSameBasecampRefinesFieldByField(t *testing.T) { {Source: SourceLocal, Path: paths[1], BaseURL: "https://3.basecampapi.com/"}, }, origin.Layers) assert.Empty(t, origin.Replaced) - assert.Empty(t, origin.Replaced) assert.Equal(t, map[string]string{ "base_url": paths[1], "account_id": paths[0], "project_id": paths[1], "scope": paths[1], "client_id": paths[0], }, origin.Fields) From 6ca06c911b24ce99f47ba4a375c8d1d4734619ef Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Thu, 17 Sep 2026 13:26:48 +0200 Subject: [PATCH 5/9] Do not offer a global entry that would be replaced in turn Where entries for the profile already replace one another, an entry added to the global config is replaced among them, so the account in it would never hold. That case now names the entry that replaces everything farther and sends the operator to the effective one. --- internal/commands/connect_unbound_test.go | 20 ++++++++++++++++++++ internal/commands/profile.go | 17 +++++++++++++---- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/internal/commands/connect_unbound_test.go b/internal/commands/connect_unbound_test.go index fcf726b91..86ea04bbf 100644 --- a/internal/commands/connect_unbound_test.go +++ b/internal/commands/connect_unbound_test.go @@ -100,6 +100,7 @@ func TestConnectAccountNamesTheFileOfAnUnusableAccount(t *testing.T) { // The hint names the entry that actually replaced the global one, and why, // even when a closer entry has since replaced that one in turn. func TestGlobalBindingBlockerNamesTheEntryThatReplacedTheGlobalOne(t *testing.T) { + writeGlobalConfig(t, `{}`) global := config.ProfileLayer{Source: config.SourceGlobal, Path: "/g/config.json", BaseURL: "https://3.basecampapi.com"} repo := config.ProfileLayer{Source: config.SourceRepo, Path: "/r/config.json", BaseURL: "https://3.basecampapi.com/"} local := config.ProfileLayer{Source: config.SourceLocal, Path: "/l/config.json", BaseURL: "http://localhost:3000"} @@ -120,3 +121,22 @@ func TestGlobalBindingBlockerNamesTheEntryThatReplacedTheGlobalOne(t *testing.T) "Its entry in /l/config.json is for http://localhost:3000, not https://3.basecampapi.com, so it replaces the global config's entry in /g/config.json and any account bound there. Add account_id to the profile's entry in /l/config.json", globalBindingBlocker(origin(local), "agent"), "a replacement for another Basecamp says which") } + +// Where entries for the profile already replace one another, a global entry +// would be replaced among them, so it is no remedy and is not offered. +func TestGlobalBindingBlockerDoesNotOfferAGlobalEntryThatWouldBeReplaced(t *testing.T) { + writeGlobalConfig(t, `{}`) + repo := config.ProfileLayer{Source: config.SourceRepo, Path: "/r/config.json", BaseURL: "https://3.basecampapi.com"} + local := config.ProfileLayer{Source: config.SourceLocal, Path: "/l/config.json", BaseURL: "http://localhost:3000"} + cfg := &config.Config{ProfileOrigins: map[string]*config.ProfileOrigin{ + "agent": { + Replaced: []config.ReplacedProfileLayer{{ProfileLayer: repo, By: local}}, + Layers: []config.ProfileLayer{local}, + }, + }} + + hint := globalBindingBlocker(cfg, "agent") + + assert.Equal(t, "Its entry comes from /l/config.json, not the global config, and the entry in /l/config.json, for http://localhost:3000, replaces everything farther — an entry added to the global config among them. Add account_id to the profile's entry in /l/config.json", hint) + assert.NotContains(t, hint, config.GlobalConfigDir()+"/config.json", "a global entry would be replaced too") +} diff --git a/internal/commands/profile.go b/internal/commands/profile.go index 0187339b8..27c70c515 100644 --- a/internal/commands/profile.go +++ b/internal/commands/profile.go @@ -599,10 +599,19 @@ func globalBindingBlocker(cfg *config.Config, name string) string { richtext.SanitizeSingleLine(hidden.By.Path), why, richtext.SanitizeSingleLine(hidden.Path), richtext.SanitizeSingleLine(closest.Path)) } // The global config has no entry for this profile, so there is nothing - // to bind — but an entry added there for the same Basecamp is refined - // by the ones that do define it, and the account in it holds. That is - // the remedy to name first: a repo config is shared, and an operator's - // account does not belong in it (nor can they always write /etc). + // to bind. An entry added there is refined by the ones that do define + // it — and the account in it holds — but only while nothing between + // them replaces it. Where something does, the global config is no + // remedy at all: a new entry would be replaced the same way. + if len(origin.Replaced) > 0 { + replacer := origin.Replaced[0].By + return fmt.Sprintf("Its entry comes from %s, not the global config, and the entry in %s, for %s, replaces everything farther — an entry added to the global config among them. Add account_id to the profile's entry in %s", + richtext.SanitizeSingleLine(closest.Path), richtext.SanitizeSingleLine(replacer.Path), + richtext.SanitizeSingleLine(replacer.BaseURL), richtext.SanitizeSingleLine(closest.Path)) + } + // Naming the global config first: a repo config is shared, and an + // operator's account does not belong in it (nor can they always + // write /etc). return fmt.Sprintf("Its entry comes from %s, not the global config, so no command can bind it. Give it an entry in %s with base_url %s and an account_id — or add account_id to the entry in %s", richtext.SanitizeSingleLine(closest.Path), richtext.SanitizeSingleLine(filepath.Join(config.GlobalConfigDir(), "config.json")), From f7196a9be709c4cee25cdf6d097e9dfc45c21ab9 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Thu, 17 Sep 2026 14:35:05 +0200 Subject: [PATCH 6/9] A replaced system entry is no reason to refuse the global remedy The global config is closer than the system one, so an entry added there replaces a system entry rather than being replaced by it. Only a replaced repo or local entry proves a new global entry would go the same way. --- internal/commands/connect_unbound_test.go | 20 +++++++++++++++ internal/commands/profile.go | 30 +++++++++++++++++------ 2 files changed, 43 insertions(+), 7 deletions(-) diff --git a/internal/commands/connect_unbound_test.go b/internal/commands/connect_unbound_test.go index 86ea04bbf..c7de8faec 100644 --- a/internal/commands/connect_unbound_test.go +++ b/internal/commands/connect_unbound_test.go @@ -140,3 +140,23 @@ func TestGlobalBindingBlockerDoesNotOfferAGlobalEntryThatWouldBeReplaced(t *test assert.Equal(t, "Its entry comes from /l/config.json, not the global config, and the entry in /l/config.json, for http://localhost:3000, replaces everything farther — an entry added to the global config among them. Add account_id to the profile's entry in /l/config.json", hint) assert.NotContains(t, hint, config.GlobalConfigDir()+"/config.json", "a global entry would be replaced too") } + +// A replaced system entry is no such obstacle: the global config is closer, +// so an entry added there replaces the system one rather than the other way +// round, and the closer entries refine it. +func TestGlobalBindingBlockerOffersAGlobalEntryOverAReplacedSystemOne(t *testing.T) { + writeGlobalConfig(t, `{}`) + system := config.ProfileLayer{Source: config.SourceSystem, Path: "/etc/basecamp/config.json", BaseURL: "https://3.basecampapi.com"} + repo := config.ProfileLayer{Source: config.SourceRepo, Path: "/r/config.json", BaseURL: "http://localhost:3000"} + cfg := &config.Config{ProfileOrigins: map[string]*config.ProfileOrigin{ + "agent": { + Replaced: []config.ReplacedProfileLayer{{ProfileLayer: system, By: repo}}, + Layers: []config.ProfileLayer{repo}, + }, + }} + + assert.Equal(t, + "Its entry comes from /r/config.json, not the global config, so no command can bind it. Give it an entry in "+ + filepath.Join(config.GlobalConfigDir(), "config.json")+" with base_url http://localhost:3000 and an account_id — or add account_id to the entry in /r/config.json", + globalBindingBlocker(cfg, "agent")) +} diff --git a/internal/commands/profile.go b/internal/commands/profile.go index 27c70c515..7b800a978 100644 --- a/internal/commands/profile.go +++ b/internal/commands/profile.go @@ -600,14 +600,16 @@ func globalBindingBlocker(cfg *config.Config, name string) string { } // The global config has no entry for this profile, so there is nothing // to bind. An entry added there is refined by the ones that do define - // it — and the account in it holds — but only while nothing between - // them replaces it. Where something does, the global config is no - // remedy at all: a new entry would be replaced the same way. - if len(origin.Replaced) > 0 { - replacer := origin.Replaced[0].By + // it — and the account in it holds — but only while nothing closer + // than the global config replaces it. Where something does, the global + // config is no remedy at all: a new entry would be replaced the same + // way. A replaced entry the system config made is no such evidence: it + // is farther than the global config, and a new entry there replaces it + // rather than the other way round. + if replaced := firstReplacedAfterGlobal(origin); replaced != nil { return fmt.Sprintf("Its entry comes from %s, not the global config, and the entry in %s, for %s, replaces everything farther — an entry added to the global config among them. Add account_id to the profile's entry in %s", - richtext.SanitizeSingleLine(closest.Path), richtext.SanitizeSingleLine(replacer.Path), - richtext.SanitizeSingleLine(replacer.BaseURL), richtext.SanitizeSingleLine(closest.Path)) + richtext.SanitizeSingleLine(closest.Path), richtext.SanitizeSingleLine(replaced.By.Path), + richtext.SanitizeSingleLine(replaced.By.BaseURL), richtext.SanitizeSingleLine(closest.Path)) } // Naming the global config first: a repo config is shared, and an // operator's account does not belong in it (nor can they always @@ -619,6 +621,20 @@ func globalBindingBlocker(cfg *config.Config, name string) string { richtext.SanitizeSingleLine(closest.Path)) } +// firstReplacedAfterGlobal is the farthest replaced entry a new global +// entry would be replaced along with: one a repo or local config made, +// which is closer than the global config. A system entry is farther, so a +// replacement of one says nothing about an entry added to the global +// config. Nil when there is none. +func firstReplacedAfterGlobal(origin *config.ProfileOrigin) *config.ReplacedProfileLayer { + for i := range origin.Replaced { + if origin.Replaced[i].Source == config.SourceRepo || origin.Replaced[i].Source == config.SourceLocal { + return &origin.Replaced[i] + } + } + return nil +} + // globalConfigUnusable reports whether the global config file exists but // cannot be read or parsed. config.Load skips such a file, profiles and // all, so what it recorded about where a profile comes from is not From d3d62b7d753511c5c1d2f61a7464ddda7183c9af Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Thu, 17 Sep 2026 14:58:15 +0200 Subject: [PATCH 7/9] A todolist belongs to a project, and setup proves the unusable-config guard An entry that names another project no longer inherits the farther file's todolist, which is a todolist of the project it replaced. And connect setup, the one path that reaches the guard for a global config that cannot be parsed, now has the test for it. --- internal/commands/profile_layers_test.go | 16 ++++++++++++++++ internal/config/config.go | 8 ++++++++ internal/config/config_test.go | 21 +++++++++++++++++++++ 3 files changed, 45 insertions(+) diff --git a/internal/commands/profile_layers_test.go b/internal/commands/profile_layers_test.go index 6063c66b2..52f538566 100644 --- a/internal/commands/profile_layers_test.go +++ b/internal/commands/profile_layers_test.go @@ -169,3 +169,19 @@ func TestUnusableGlobalConfigIsReportedAsItself(t *testing.T) { assert.NotContains(t, err.Error(), "not the global config") assert.Equal(t, 12, in.Len(), "refused before stdin is read") } + +// Setup reads the profile's account, so it asks the same question, and a +// global config it cannot parse is reported as that rather than as a +// profile defined somewhere else. +func TestConnectSetupReportsAnUnusableGlobalConfigAsItself(t *testing.T) { + s := startConnectSetupServer(t) + bareSetupApp(t, s, "agent") + trustedLocalConfig(t, fmt.Sprintf(`{"profiles":{"agent":{"base_url":%q}}}`, s.srv.URL)) + writeGlobalConfigHere(t, fmt.Sprintf(`{"profiles":{"agent":{"base_url":%q,"account_id":"999"`, s.srv.URL)) + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupOperatorPerson), routeArg(t)) + require.Error(t, err, out) + hint := hintOf(t, err) + assert.NotContains(t, hint, "not the global config", "the global config was skipped, so it is no evidence") + assert.Contains(t, hint, "basecamp auth agent connect -P agent", "the generic remedy, which reports the file itself") +} diff --git a/internal/config/config.go b/internal/config/config.go index d9d957bdc..8e4770250 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -438,6 +438,9 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { // Unset means absent: for the IDs also empty (as for the top-level IDs), // while a present scope or client_id sets the field even when empty. // +// One field is dropped rather than kept: a todolist belongs to a project, so +// an entry that names another project and no todolist inherits none. +// // Every field set records the file that set it, in cfg.ProfileOrigins. func mergeProfile(cfg *Config, name string, entry map[string]any, layer ProfileLayer) { baseURL, _ := entry["base_url"].(string) @@ -479,6 +482,11 @@ func mergeProfile(cfg *Config, name string, entry map[string]any, layer ProfileL set("account_id", &p.AccountID, account) } if v := getStringOrNumber(entry, "project_id"); v != "" { + if p.ProjectID != v && p.TodolistID != "" { + // The inherited todolist is in the project being replaced. + p.TodolistID = "" + delete(origin.Fields, "todolist_id") + } set("project_id", &p.ProjectID, v) } if v := getStringOrNumber(entry, "todolist_id"); v != "" { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index c60ab93cb..f2d878480 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1411,3 +1411,24 @@ func TestProfileEntryBindingAnUnboundOneRefinesIt(t *testing.T) { assert.Equal(t, &ProfileConfig{BaseURL: "https://3.basecampapi.com", AccountID: "555", ProjectID: "1"}, cfg.Profiles["bot"]) } + +// A todolist belongs to a project: an entry that names another project +// leaves the farther file's todolist behind, and one that names its own +// todolist keeps it. +func TestProfileEntryForAnotherProjectDropsTheInheritedTodolist(t *testing.T) { + cfg, paths := loadProfileLayers(t, + profileLayer{SourceGlobal, `{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","account_id":"999","project_id":"9","todolist_id":"3"}}}`}, + profileLayer{SourceRepo, `{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","project_id":"42"}}}`}, + ) + + assert.Equal(t, &ProfileConfig{BaseURL: "https://3.basecampapi.com", AccountID: "999", ProjectID: "42"}, cfg.Profiles["bot"]) + assert.NotContains(t, cfg.ProfileOrigins["bot"].Fields, "todolist_id") + assert.Equal(t, paths[0], cfg.ProfileOrigins["bot"].Fields["account_id"], "the account is not a project's") + + cfg, _ = loadProfileLayers(t, + profileLayer{SourceGlobal, `{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","project_id":"9","todolist_id":"3"}}}`}, + profileLayer{SourceRepo, `{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","project_id":"42","todolist_id":"7"}}}`}, + ) + + assert.Equal(t, "7", cfg.Profiles["bot"].TodolistID) +} From 8d3d98e9d85b01dda5af6e924a70fc857bba9c4b Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Thu, 17 Sep 2026 15:04:46 +0200 Subject: [PATCH 8/9] Compare project IDs as numbers too, and count a bad profiles shape as unusable One rule for IDs: equal as decimal numbers, so a project spelled "09" is the project spelled "9" and its todolist is kept. A global config whose profiles value is not an object is skipped by the loader and refused by every writer, so it is reported as itself rather than recommended as the place to add an entry. --- internal/commands/profile.go | 18 ++++++++++++++---- internal/commands/profile_layers_test.go | 11 ++++++++++- internal/config/config.go | 13 +++++++------ internal/config/config_test.go | 22 +++++++++++++++------- 4 files changed, 46 insertions(+), 18 deletions(-) diff --git a/internal/commands/profile.go b/internal/commands/profile.go index 7b800a978..3d0752ee5 100644 --- a/internal/commands/profile.go +++ b/internal/commands/profile.go @@ -636,9 +636,11 @@ func firstReplacedAfterGlobal(origin *config.ProfileOrigin) *config.ReplacedProf } // globalConfigUnusable reports whether the global config file exists but -// cannot be read or parsed. config.Load skips such a file, profiles and -// all, so what it recorded about where a profile comes from is not -// evidence: the entry that defines it may be in the file that was skipped. +// cannot be read, cannot be parsed, or holds a "profiles" value that is not +// an object. The loader skips such a file's profiles, so what it recorded +// about where a profile comes from is not evidence — the entry that defines +// it may be in the file that was skipped — and every writer refuses the +// file, which is what the operator has to hear instead. func globalConfigUnusable() bool { path := filepath.Join(config.GlobalConfigDir(), "config.json") data, err := os.ReadFile(path) //nolint:gosec // G304: the global config path @@ -649,7 +651,15 @@ func globalConfigUnusable() bool { return true } var parsed map[string]any - return json.Unmarshal(data, &parsed) != nil || parsed == nil + if json.Unmarshal(data, &parsed) != nil || parsed == nil { + return true + } + profiles, present := parsed["profiles"] + if !present || profiles == nil { + return false + } + _, isObject := profiles.(map[string]any) + return !isObject } // boundIn names the config file a profile's account came from, as " (bound diff --git a/internal/commands/profile_layers_test.go b/internal/commands/profile_layers_test.go index 52f538566..9f3d6c08e 100644 --- a/internal/commands/profile_layers_test.go +++ b/internal/commands/profile_layers_test.go @@ -177,8 +177,17 @@ func TestConnectSetupReportsAnUnusableGlobalConfigAsItself(t *testing.T) { s := startConnectSetupServer(t) bareSetupApp(t, s, "agent") trustedLocalConfig(t, fmt.Sprintf(`{"profiles":{"agent":{"base_url":%q}}}`, s.srv.URL)) - writeGlobalConfigHere(t, fmt.Sprintf(`{"profiles":{"agent":{"base_url":%q,"account_id":"999"`, s.srv.URL)) + for _, global := range []string{ + fmt.Sprintf(`{"profiles":{"agent":{"base_url":%q,"account_id":"999"`, s.srv.URL), // never closed + `{"profiles":[]}`, // a shape every writer refuses + } { + writeGlobalConfigHere(t, global) + assertSetupReportsTheGlobalConfig(t, s) + } +} +func assertSetupReportsTheGlobalConfig(t *testing.T, s *connectSetupServer) { + t.Helper() out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupOperatorPerson), routeArg(t)) require.Error(t, err, out) hint := hintOf(t, err) diff --git a/internal/config/config.go b/internal/config/config.go index 8e4770250..6d582c201 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -457,7 +457,7 @@ func mergeProfile(cfg *Config, name string, entry map[string]any, layer ProfileL p, origin := cfg.Profiles[name], cfg.ProfileOrigins[name] account := getStringOrNumber(entry, "account_id") - otherAccount := p != nil && p.AccountID != "" && account != "" && !sameAccountID(p.AccountID, account) + otherAccount := p != nil && p.AccountID != "" && account != "" && !sameID(p.AccountID, account) if p == nil || origin == nil || otherAccount || NormalizeBaseURL(p.BaseURL) != NormalizeBaseURL(baseURL) { replaced := []ReplacedProfileLayer(nil) if p != nil && origin != nil { @@ -482,7 +482,7 @@ func mergeProfile(cfg *Config, name string, entry map[string]any, layer ProfileL set("account_id", &p.AccountID, account) } if v := getStringOrNumber(entry, "project_id"); v != "" { - if p.ProjectID != v && p.TodolistID != "" { + if !sameID(p.ProjectID, v) && p.TodolistID != "" { // The inherited todolist is in the project being replaced. p.TodolistID = "" delete(origin.Fields, "todolist_id") @@ -500,10 +500,11 @@ func mergeProfile(cfg *Config, name string, entry map[string]any, layer ProfileL } } -// sameAccountID reports whether two account IDs name the same account: equal -// as decimal numbers, so "0999" is "999", however many digits they run to. -// A value that is not all digits is the same only as its exact spelling. -func sameAccountID(a, b string) bool { +// sameID reports whether two IDs name the same record: equal as decimal +// numbers, so "0999" is "999", however many digits they run to — the +// comparison the commands and the name resolver make. A value that is not +// all digits is the same only as its exact spelling. +func sameID(a, b string) bool { if a == b { return true } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index f2d878480..2a7ab7387 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1386,8 +1386,9 @@ func TestProfileEntryForAnotherAccountReplacesTheEntryWhole(t *testing.T) { assert.Equal(t, []string{paths[1], paths[2]}, []string{origin.Layers[0].Path, origin.Layers[1].Path}, "the same account refines") } -// Account IDs are compared as numbers, as the commands compare them: a -// spelling with leading zeros is the same account, and refines. +// IDs are compared as numbers, as the commands and the name resolver +// compare them: a spelling with leading zeros is the same account, and +// refines. func TestProfileEntryForTheSameAccountSpelledDifferentlyRefines(t *testing.T) { cfg, _ := loadProfileLayers(t, profileLayer{SourceGlobal, `{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","account_id":"999","project_id":"1"}}}`}, @@ -1395,11 +1396,11 @@ func TestProfileEntryForTheSameAccountSpelledDifferentlyRefines(t *testing.T) { ) assert.Equal(t, "1", cfg.Profiles["bot"].ProjectID) - assert.True(t, sameAccountID("000123456789012345678901234567890", "123456789012345678901234567890"), "past int64") - assert.True(t, sameAccountID("0", "000")) - assert.False(t, sameAccountID("999", "9990")) - assert.False(t, sameAccountID("x999", "999")) - assert.True(t, sameAccountID("abc", "abc")) + assert.True(t, sameID("000123456789012345678901234567890", "123456789012345678901234567890"), "past int64") + assert.True(t, sameID("0", "000")) + assert.False(t, sameID("999", "9990")) + assert.False(t, sameID("x999", "999")) + assert.True(t, sameID("abc", "abc")) } // A closer entry that binds an unbound farther one refines it. @@ -1431,4 +1432,11 @@ func TestProfileEntryForAnotherProjectDropsTheInheritedTodolist(t *testing.T) { ) assert.Equal(t, "7", cfg.Profiles["bot"].TodolistID) + + cfg, _ = loadProfileLayers(t, + profileLayer{SourceGlobal, `{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","project_id":"9","todolist_id":"3"}}}`}, + profileLayer{SourceRepo, `{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","project_id":"09"}}}`}, + ) + + assert.Equal(t, "3", cfg.Profiles["bot"].TodolistID, "the same project, spelled differently") } From ca3fd9c26da0b10859811374955d78e099653ecf Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Thu, 17 Sep 2026 15:10:03 +0200 Subject: [PATCH 9/9] A null profiles value is refused by every writer, so it is unusable too globalProfilesMap refuses a "profiles" value of any shape but an object, null included, so the global config is reported as itself rather than recommended as the place to add an entry. --- internal/commands/profile.go | 8 +++++--- internal/commands/profile_layers_test.go | 3 ++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/internal/commands/profile.go b/internal/commands/profile.go index 3d0752ee5..4bc436444 100644 --- a/internal/commands/profile.go +++ b/internal/commands/profile.go @@ -636,8 +636,8 @@ func firstReplacedAfterGlobal(origin *config.ProfileOrigin) *config.ReplacedProf } // globalConfigUnusable reports whether the global config file exists but -// cannot be read, cannot be parsed, or holds a "profiles" value that is not -// an object. The loader skips such a file's profiles, so what it recorded +// cannot be read, cannot be parsed, or holds a "profiles" value that is +// present and not an object (null included). The loader skips such a file's profiles, so what it recorded // about where a profile comes from is not evidence — the entry that defines // it may be in the file that was skipped — and every writer refuses the // file, which is what the operator has to hear instead. @@ -655,9 +655,11 @@ func globalConfigUnusable() bool { return true } profiles, present := parsed["profiles"] - if !present || profiles == nil { + if !present { return false } + // A present value of any other shape, null included, is what + // globalProfilesMap refuses to rewrite. _, isObject := profiles.(map[string]any) return !isObject } diff --git a/internal/commands/profile_layers_test.go b/internal/commands/profile_layers_test.go index 9f3d6c08e..9dc6f8166 100644 --- a/internal/commands/profile_layers_test.go +++ b/internal/commands/profile_layers_test.go @@ -179,7 +179,8 @@ func TestConnectSetupReportsAnUnusableGlobalConfigAsItself(t *testing.T) { trustedLocalConfig(t, fmt.Sprintf(`{"profiles":{"agent":{"base_url":%q}}}`, s.srv.URL)) for _, global := range []string{ fmt.Sprintf(`{"profiles":{"agent":{"base_url":%q,"account_id":"999"`, s.srv.URL), // never closed - `{"profiles":[]}`, // a shape every writer refuses + `{"profiles":[]}`, // a shape every writer refuses + `{"profiles":null}`, // and so is null } { writeGlobalConfigHere(t, global) assertSetupReportsTheGlobalConfig(t, s)