diff --git a/internal/auth/agent.go b/internal/auth/agent.go index 102452bdc..a01b0c301 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 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 { command += " --account " + m.accountToBind() diff --git a/internal/commands/auth.go b/internal/commands/auth.go index a505c1ccf..bcf38e7e0 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -853,13 +853,15 @@ 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 { + // 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 } - globalUnbound = unbound + blocker = globalBindingBlocker(app.Config, name) } target := &profileTarget{name: name, account: account, existing: existing} @@ -870,16 +872,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 } @@ -1165,7 +1166,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 f72f79f71..ef07dfcd7 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 @@ -281,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/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..e48507027 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -866,15 +866,19 @@ 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 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), - "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( - 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 @@ -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..c7de8faec 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,34 @@ 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, "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") } +// 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 +84,79 @@ 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)) +} + +// 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"} + 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 /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") +} + +// 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 4e247bc49..4bc436444 100644 --- a/internal/commands/profile.go +++ b/internal/commands/profile.go @@ -553,18 +553,135 @@ 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() +// 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 { + 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 + // 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 "" + } + closest := origin.Closest() + if hidden := origin.ReplacedLayer(config.SourceGlobal); hidden != nil { + // 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. An entry added there is refined by the ones that do define + // 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(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 + // 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")), + richtext.SanitizeSingleLine(closest.BaseURL), + 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, 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. +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 false, err + return true } - entry := globalProfileEntry(configData, name) - return entry != nil && getStringOrNumber(entry, "account_id") == "", nil + var parsed map[string]any + if json.Unmarshal(data, &parsed) != nil || parsed == nil { + return true + } + profiles, present := parsed["profiles"] + 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 +} + +// 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 "" +} + +// 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.Fields[field] } // bindProfileAccount sets the account on an existing profile entry in the @@ -585,19 +702,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..791faaa0b --- /dev/null +++ b/internal/commands/profile_layers_helpers_test.go @@ -0,0 +1,68 @@ +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) + // 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)) + 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..9dc6f8166 --- /dev/null +++ b/internal/commands/profile_layers_test.go @@ -0,0 +1,197 @@ +//go:build unix + +package commands + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "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()) +} + +// 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")+")") +} + +// 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") +} + +// 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)) + 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":null}`, // and so is null + } { + 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) + 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 5efb2a150..6d582c201 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -52,6 +52,76 @@ 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 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 { + 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) *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 +// fields win. +func (o *ProfileOrigin) Closest() ProfileLayer { + if len(o.Layers) == 0 { + return ProfileLayer{} + } + return o.Layers[len(o.Layers)-1] } // IsExperimental returns true if the named experimental feature is enabled. @@ -333,41 +403,122 @@ 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}) } } + // 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) } } } +// 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, 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. +// +// 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) + 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] + account := getStringOrNumber(entry, "account_id") + 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 { + 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)} + 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 account != "" { + set("account_id", &p.AccountID, account) + } + if v := getStringOrNumber(entry, "project_id"); v != "" { + if !sameID(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 != "" { + 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) + } +} + +// 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 + } + 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 08a2cf278..2a7ab7387 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,167 @@ 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, 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) +} + +// 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") +} + +// 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"] + 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") +} + +// 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"}}}`}, + profileLayer{SourceLocal, `{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","account_id":"0999"}}}`}, + ) + + assert.Equal(t, "1", cfg.Profiles["bot"].ProjectID) + 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. +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"]) +} + +// 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) + + 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") +}