From 1bc6d85153006a2090142e04157f97c27295cef9 Mon Sep 17 00:00:00 2001 From: Santiago Medina Rolong Date: Wed, 15 Jul 2026 17:16:42 -0700 Subject: [PATCH] fix(auth): stop false OAuth2 no-credentials warning without --app The check used GetApp("") which never resolves the default app, so it always warned when any other app had credentials. Resolve default_app correctly and name that app in the warning. --- CHANGELOG.md | 6 ++++ cli/auth.go | 67 ++++++++++++++++++++++++------------- cli/auth_test.go | 86 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 136 insertions(+), 23 deletions(-) create mode 100644 cli/auth_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index ca07d0a..1bd6fd2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ All user-visible bugs and enhancements should be recorded here. +## Unreleased + +### Fixed + +- [2026-07-15] `xurl auth oauth2` no longer always warns that the "default" app has no client credentials when `--app` is omitted. The check used `GetApp("")` (empty-key map lookup, always nil) instead of the real default app, so it false-alarmed even when the active default (e.g. `app-2`) had credentials. The warning now resolves `default_app` and names that app correctly. + ## v1.2.2 - 2026-06-29 ### Fixed diff --git a/cli/auth.go b/cli/auth.go index c4bb8c4..9d996c4 100644 --- a/cli/auth.go +++ b/cli/auth.go @@ -114,32 +114,19 @@ on any device, and paste the resulting redirect URL (or code) back in.`, username = args[0] } - // Warn when --app is not specified and the default app has no - // client credentials but another registered app does. Tokens + // Warn when --app is not specified and the active/default app has + // no client credentials but another registered app does. Tokens // saved to a credential-less app cannot be refreshed, causing // cryptic 401 errors on all subsequent API calls. - if a.AppName() == "" { - defaultApp := a.TokenStore.GetApp("") - hasCredentials := defaultApp != nil && defaultApp.ClientID != "" - if !hasCredentials { - var credentialed []string - for _, name := range a.TokenStore.ListApps() { - app := a.TokenStore.GetApp(name) - if app != nil && app.ClientID != "" { - credentialed = append(credentialed, name) - } - } - if len(credentialed) > 0 { - fmt.Fprintf(os.Stderr, "\033[33m⚠️ No --app specified. The OAuth2 token will be saved to the \"default\" app,\n") - fmt.Fprintf(os.Stderr, " which has no client credentials stored. API calls will fail with 401 errors.\n\n") - fmt.Fprintf(os.Stderr, " App(s) with credentials available:\n") - for _, name := range credentialed { - app := a.TokenStore.GetApp(name) - fmt.Fprintf(os.Stderr, " --app %s [client_id: %s…]\n", name, truncate(app.ClientID, 8)) - } - fmt.Fprintf(os.Stderr, "\n Run instead: xurl auth oauth2 --app %s\n\n", credentialed[0]) - } + if warn, targetName, credentialed := oauth2NoAppCredentialWarning(a.TokenStore, a.AppName()); warn { + fmt.Fprintf(os.Stderr, "\033[33m⚠️ No --app specified. The OAuth2 token will be saved to the %q app,\n", targetName) + fmt.Fprintf(os.Stderr, " which has no client credentials stored. API calls will fail with 401 errors.\n\n") + fmt.Fprintf(os.Stderr, " App(s) with credentials available:\n") + for _, name := range credentialed { + app := a.TokenStore.GetApp(name) + fmt.Fprintf(os.Stderr, " --app %s [client_id: %s…]\n", name, truncate(app.ClientID, 8)) } + fmt.Fprintf(os.Stderr, "\n Run instead: xurl auth oauth2 --app %s\n\n", credentialed[0]) } var err error @@ -681,6 +668,40 @@ Examples: // ─── helpers ──────────────────────────────────────────────────────── +// oauth2NoAppCredentialWarning reports whether to warn that an OAuth2 login +// without --app will target an app that has no stored client credentials, +// while at least one other registered app does. +// +// GetApp maps by exact name and does not resolve the empty override to the +// default app, so callers must use GetDefaultApp (not GetApp("")). +func oauth2NoAppCredentialWarning(ts *store.TokenStore, appOverride string) (warn bool, targetName string, credentialed []string) { + if appOverride != "" { + return false, "", nil + } + + targetName = ts.GetDefaultApp() + if targetName == "" { + // ResolveApp would auto-create this name when saving the token. + targetName = "default" + } + + target := ts.GetApp(targetName) + if target != nil && target.ClientID != "" { + return false, targetName, nil + } + + for _, name := range ts.ListApps() { + app := ts.GetApp(name) + if app != nil && app.ClientID != "" { + credentialed = append(credentialed, name) + } + } + if len(credentialed) == 0 { + return false, targetName, nil + } + return true, targetName, credentialed +} + func truncate(s string, maxLen int) string { if len(s) <= maxLen { return s diff --git a/cli/auth_test.go b/cli/auth_test.go new file mode 100644 index 0000000..bad913f --- /dev/null +++ b/cli/auth_test.go @@ -0,0 +1,86 @@ +package cli + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/xdevplatform/xurl/store" +) + +func TestOAuth2NoAppCredentialWarning(t *testing.T) { + t.Run("no warning when --app is set", func(t *testing.T) { + ts := &store.TokenStore{ + Apps: map[string]*store.App{ + "default": {ClientID: "", OAuth2Tokens: map[string]store.Token{}}, + "my-app": {ClientID: "cid", OAuth2Tokens: map[string]store.Token{}}, + }, + DefaultApp: "default", + } + warn, _, _ := oauth2NoAppCredentialWarning(ts, "my-app") + assert.False(t, warn) + }) + + t.Run("no warning when default app has credentials", func(t *testing.T) { + // Regression: GetApp("") always returned nil, so this case spuriously warned + // even when the real default (here app-2) had client credentials. + ts := &store.TokenStore{ + Apps: map[string]*store.App{ + "app-2": {ClientID: "WEpLT2ZF", OAuth2Tokens: map[string]store.Token{}}, + "default": {ClientID: "VUttdG9P", OAuth2Tokens: map[string]store.Token{}}, + }, + DefaultApp: "app-2", + } + warn, targetName, credentialed := oauth2NoAppCredentialWarning(ts, "") + assert.False(t, warn) + assert.Equal(t, "app-2", targetName) + assert.Nil(t, credentialed) + }) + + t.Run("warns when default app lacks credentials but others have them", func(t *testing.T) { + ts := &store.TokenStore{ + Apps: map[string]*store.App{ + "default": {ClientID: "", OAuth2Tokens: map[string]store.Token{}}, + "my-app": {ClientID: "abc12345xyz", OAuth2Tokens: map[string]store.Token{}}, + }, + DefaultApp: "default", + } + warn, targetName, credentialed := oauth2NoAppCredentialWarning(ts, "") + require.True(t, warn) + assert.Equal(t, "default", targetName) + assert.Equal(t, []string{"my-app"}, credentialed) + }) + + t.Run("uses real default app name in warning target", func(t *testing.T) { + ts := &store.TokenStore{ + Apps: map[string]*store.App{ + "empty-app": {ClientID: "", OAuth2Tokens: map[string]store.Token{}}, + "prod": {ClientID: "prod-id-1", OAuth2Tokens: map[string]store.Token{}}, + }, + DefaultApp: "empty-app", + } + warn, targetName, credentialed := oauth2NoAppCredentialWarning(ts, "") + require.True(t, warn) + assert.Equal(t, "empty-app", targetName) + assert.Equal(t, []string{"prod"}, credentialed) + }) + + t.Run("no warning when no app has credentials", func(t *testing.T) { + ts := &store.TokenStore{ + Apps: map[string]*store.App{ + "default": {ClientID: "", OAuth2Tokens: map[string]store.Token{}}, + }, + DefaultApp: "default", + } + warn, _, _ := oauth2NoAppCredentialWarning(ts, "") + assert.False(t, warn) + }) + + t.Run("no warning when store is empty", func(t *testing.T) { + ts := &store.TokenStore{Apps: map[string]*store.App{}} + warn, targetName, _ := oauth2NoAppCredentialWarning(ts, "") + assert.False(t, warn) + assert.Equal(t, "default", targetName) + }) +}