From 3304a682c61629b9891de61d9659ff3415082df5 Mon Sep 17 00:00:00 2001 From: Julian LaNeve Date: Tue, 21 Jul 2026 14:40:16 -0400 Subject: [PATCH 1/2] Clear the refresh token on logout, not just the access token astro logout cleared the access token and email but left the refresh token in ~/.astro/config.yaml. That token is still a live credential: it can be exchanged with Auth0 for a fresh access token, so a user who believed they were logged out still had a working session on disk. Logout now clears the refresh token alongside the access token and email. It also now returns an error instead of returning silently when a field fails to clear, and skips the "Successfully logged out" message on that path, so a partial logout is visible instead of looking clean. The one caller (cmd/auth.go's logout) now returns that error up to cobra, which was a small enough change to make rather than keeping the fix message-only. Co-Authored-By: Claude Fable 5 --- cloud/auth/auth.go | 18 ++++++++++++++---- cloud/auth/auth_test.go | 30 ++++++++++++++++++++++++------ cmd/auth.go | 5 ++--- cmd/auth_test.go | 3 ++- 4 files changed, 42 insertions(+), 14 deletions(-) diff --git a/cloud/auth/auth.go b/cloud/auth/auth.go index b1237d6b8..9a4cfe1af 100644 --- a/cloud/auth/auth.go +++ b/cloud/auth/auth.go @@ -424,26 +424,36 @@ func Login(domain, token string, astroV1Client astrov1.APIClient, out io.Writer, } // Logout logs a user out of the docker registry. Will need to logout of Astro next. -func Logout(domain string, out io.Writer) { +// It clears every credential held in the context (access token, refresh token, +// and email) so no live token is left behind in the config file. +func Logout(domain string, out io.Writer) error { c, _ := context.GetContext(domain) err = c.SetContextKey("token", "") if err != nil { - return + fmt.Fprintln(out, "Failed to clear access token: ", err.Error()) + return err + } + err = c.SetContextKey("refreshtoken", "") + if err != nil { + fmt.Fprintln(out, "Failed to clear refresh token: ", err.Error()) + return err } err = c.SetContextKey("user_email", "") if err != nil { - return + fmt.Fprintln(out, "Failed to clear user email: ", err.Error()) + return err } // remove the current context err = config.ResetCurrentContext() if err != nil { fmt.Fprintln(out, "Failed to reset current context: ", err.Error()) - return + return err } fmt.Fprintln(out, "Successfully logged out of Astronomer") + return nil } func FetchDomainAuthConfig(domain string) (Config, error) { diff --git a/cloud/auth/auth_test.go b/cloud/auth/auth_test.go index 29524ad43..e3e5a7d57 100644 --- a/cloud/auth/auth_test.go +++ b/cloud/auth/auth_test.go @@ -921,12 +921,13 @@ func TestLogout(t *testing.T) { testUtil.InitTestConfig(testUtil.LocalPlatform) t.Run("success", func(t *testing.T) { buf := new(bytes.Buffer) - Logout("astronomer.io", buf) + err := Logout("astronomer.io", buf) + assert.NoError(t, err) assert.Equal(t, "Successfully logged out of Astronomer\n", buf.String()) }) t.Run("success_with_email", func(t *testing.T) { - assertions := func(expUserEmail string, expToken string) { + assertions := func(expUserEmail, expToken, expRefreshToken string) { contexts, err := config.GetContexts() assert.NoError(t, err) context := contexts.Contexts["localhost"] @@ -934,6 +935,7 @@ func TestLogout(t *testing.T) { assert.NoError(t, err) assert.Equal(t, expUserEmail, context.UserEmail) assert.Equal(t, expToken, context.Token) + assert.Equal(t, expRefreshToken, context.RefreshToken) } testUtil.InitTestConfig(testUtil.LocalPlatform) c, err := config.GetCurrentContext() @@ -942,16 +944,32 @@ func TestLogout(t *testing.T) { assert.NoError(t, err) err = c.SetContextKey("token", "Bearer some-token") assert.NoError(t, err) + err = c.SetContextKey("refreshtoken", "some-refresh-token") + assert.NoError(t, err) // test before - assertions("test.user@astronomer.io", "Bearer some-token") + assertions("test.user@astronomer.io", "Bearer some-token", "some-refresh-token") // log out c, err = config.GetCurrentContext() assert.NoError(t, err) - Logout(c.Domain, os.Stdout) + buf := new(bytes.Buffer) + err = Logout(c.Domain, buf) + assert.NoError(t, err) + + // test after logout: token, refresh token, and email are all cleared + assertions("", "", "") + assert.Equal(t, "Successfully logged out of Astronomer\n", buf.String()) + }) - // test after logout - assertions("", "") + t.Run("partial_failure_does_not_report_success", func(t *testing.T) { + testUtil.InitTestConfig(testUtil.LocalPlatform) + buf := new(bytes.Buffer) + // an empty domain makes every SetContextKey call fail (no domain configured), + // so Logout should bail out with an error instead of printing success. + err := Logout("", buf) + assert.Error(t, err) + assert.NotContains(t, buf.String(), "Successfully logged out of Astronomer") + assert.Contains(t, buf.String(), "Failed to clear access token") }) } diff --git a/cmd/auth.go b/cmd/auth.go index 930db978e..c8b77996b 100644 --- a/cmd/auth.go +++ b/cmd/auth.go @@ -83,10 +83,9 @@ func logout(cmd *cobra.Command, args []string, out io.Writer) error { cmd.SilenceUsage = true if context.IsCloudDomain(domain) { - cloudLogout(domain, out) - } else { - softwareLogout(domain) + return cloudLogout(domain, out) } + softwareLogout(domain) return nil } diff --git a/cmd/auth_test.go b/cmd/auth_test.go index 0d2698220..78a159715 100644 --- a/cmd/auth_test.go +++ b/cmd/auth_test.go @@ -64,8 +64,9 @@ func (s *CmdSuite) TestLogout() { localDomain := "localhost" softwareDomain := "astronomer_dev.com" - cloudLogout = func(domain string, out io.Writer) { + cloudLogout = func(domain string, out io.Writer) error { s.Equal(localDomain, domain) + return nil } softwareLogout = func(domain string) { s.Equal(softwareDomain, domain) From 4747626043e3317465cd9266d81b59ae351bb1ab Mon Sep 17 00:00:00 2001 From: Greg Neiheisel Date: Thu, 8 Oct 2026 21:39:30 -0400 Subject: [PATCH 2/2] Clear logout credentials in one write, and only for a domain logged into Review follow-ups on the logout fix: - Clear the token, refresh token, email and expiry in one config write (config.Context.ClearCredentials), so a save that fails leaves the context as it was rather than with the refresh token still in place, and the token's expiry is no longer left behind. - Fail on a domain with no login instead of writing a stub context for it and reporting success while the real credentials stay on disk. - Reset the current context only when the logged-out domain is the current one, so logging out of another domain keeps the selection. - Return wrapped errors instead of printing a line and returning the bare error, which cobra then printed a second time without context. - Use a local err instead of the package-level one, and correct the doc comment, which described a registry logout this does not do. Tests now cover each of these, including that logout returns cloudLogout's error; the old partial-failure case used an empty domain and never reached a second write. Co-Authored-By: Claude --- cloud/auth/auth.go | 36 ++++++---------- cloud/auth/auth_test.go | 95 ++++++++++++++++++++++++----------------- cmd/auth_test.go | 13 +++++- config/context.go | 26 ++++++++++- 4 files changed, 107 insertions(+), 63 deletions(-) diff --git a/cloud/auth/auth.go b/cloud/auth/auth.go index 8a0a3f4d9..870ecff83 100644 --- a/cloud/auth/auth.go +++ b/cloud/auth/auth.go @@ -423,33 +423,25 @@ func Login(domain, token string, astroV1Client astrov1.APIClient, out io.Writer, return nil } -// Logout logs a user out of the docker registry. Will need to logout of Astro next. -// It clears every credential held in the context (access token, refresh token, -// and email) so no live token is left behind in the config file. +// Logout logs a user out of an Astro domain. It clears every credential held in +// the domain's context (access token, refresh token, email and expiry) so no +// live token is left behind in the config file, and unsets the current context +// when that domain was the current one. func Logout(domain string, out io.Writer) error { - c, _ := context.GetContext(domain) - - err = c.SetContextKey("token", "") - if err != nil { - fmt.Fprintln(out, "Failed to clear access token: ", err.Error()) - return err - } - err = c.SetContextKey("refreshtoken", "") + c, err := context.GetContext(domain) if err != nil { - fmt.Fprintln(out, "Failed to clear refresh token: ", err.Error()) - return err + return fmt.Errorf("failed to find a login for %s: %w", domain, err) } - err = c.SetContextKey("user_email", "") - if err != nil { - fmt.Fprintln(out, "Failed to clear user email: ", err.Error()) - return err + + if err := c.ClearCredentials(); err != nil { + return fmt.Errorf("failed to clear the credentials for %s: %w", domain, err) } - // remove the current context - err = config.ResetCurrentContext() - if err != nil { - fmt.Fprintln(out, "Failed to reset current context: ", err.Error()) - return err + // Logging out of another domain leaves the current one selected + if current, err := config.GetCurrentDomain(); err == nil && current == domain { + if err := config.ResetCurrentContext(); err != nil { + return fmt.Errorf("failed to reset the current context: %w", err) + } } fmt.Fprintln(out, "Successfully logged out of Astronomer") diff --git a/cloud/auth/auth_test.go b/cloud/auth/auth_test.go index 7dbda8fc1..b58600147 100644 --- a/cloud/auth/auth_test.go +++ b/cloud/auth/auth_test.go @@ -948,58 +948,75 @@ func TestLogin(t *testing.T) { } func TestLogout(t *testing.T) { - testUtil.InitTestConfig(testUtil.LocalPlatform) - t.Run("success", func(t *testing.T) { - buf := new(bytes.Buffer) - err := Logout("astronomer.io", buf) + // loggedIn gives the context every credential a login writes + loggedIn := func(t *testing.T, c config.Context) { + t.Helper() + assert.NoError(t, c.SetContextKey("user_email", "test.user@astronomer.io")) + assert.NoError(t, c.SetContextKey("token", "Bearer some-token")) + assert.NoError(t, c.SetContextKey("refreshtoken", "some-refresh-token")) + assert.NoError(t, c.SetExpiresIn(3600)) + } + stored := func(t *testing.T, domain string) config.Context { + t.Helper() + c, err := (&config.Context{Domain: domain}).GetContext() assert.NoError(t, err) - assert.Equal(t, "Successfully logged out of Astronomer\n", buf.String()) - }) - - t.Run("success_with_email", func(t *testing.T) { - assertions := func(expUserEmail, expToken, expRefreshToken string) { - contexts, err := config.GetContexts() - assert.NoError(t, err) - context := contexts.Contexts["localhost"] + return c + } - assert.NoError(t, err) - assert.Equal(t, expUserEmail, context.UserEmail) - assert.Equal(t, expToken, context.Token) - assert.Equal(t, expRefreshToken, context.RefreshToken) - } + t.Run("clears every credential and the current context", func(t *testing.T) { testUtil.InitTestConfig(testUtil.LocalPlatform) c, err := config.GetCurrentContext() assert.NoError(t, err) - err = c.SetContextKey("user_email", "test.user@astronomer.io") - assert.NoError(t, err) - err = c.SetContextKey("token", "Bearer some-token") - assert.NoError(t, err) - err = c.SetContextKey("refreshtoken", "some-refresh-token") - assert.NoError(t, err) - // test before - assertions("test.user@astronomer.io", "Bearer some-token", "some-refresh-token") + loggedIn(t, c) - // log out - c, err = config.GetCurrentContext() - assert.NoError(t, err) buf := new(bytes.Buffer) - err = Logout(c.Domain, buf) - assert.NoError(t, err) + assert.NoError(t, Logout(c.Domain, buf)) - // test after logout: token, refresh token, and email are all cleared - assertions("", "", "") + after := stored(t, c.Domain) + assert.Empty(t, after.Token) + assert.Empty(t, after.RefreshToken) + assert.Empty(t, after.UserEmail) + expiresIn, err := after.GetExpiresIn() + assert.NoError(t, err) + assert.True(t, expiresIn.IsZero(), "expiry left behind: %v", expiresIn) + _, err = config.GetCurrentDomain() + assert.ErrorIs(t, err, config.ErrGetHomeString) assert.Equal(t, "Successfully logged out of Astronomer\n", buf.String()) }) - t.Run("partial_failure_does_not_report_success", func(t *testing.T) { + t.Run("logging out of another domain keeps the current one", func(t *testing.T) { + testUtil.InitTestConfig(testUtil.LocalPlatform) + current, err := config.GetCurrentContext() + assert.NoError(t, err) + loggedIn(t, current) + other := config.Context{Domain: "astronomer-dev.io", Token: "Bearer other-token", RefreshToken: "other-refresh-token"} + assert.NoError(t, other.SetContext()) + + assert.NoError(t, Logout(other.Domain, new(bytes.Buffer))) + + assert.Empty(t, stored(t, other.Domain).RefreshToken) + domain, err := config.GetCurrentDomain() + assert.NoError(t, err) + assert.Equal(t, current.Domain, domain) + assert.Equal(t, "some-refresh-token", stored(t, current.Domain).RefreshToken) + }) + + t.Run("an unknown domain fails without reporting success", func(t *testing.T) { testUtil.InitTestConfig(testUtil.LocalPlatform) + current, err := config.GetCurrentContext() + assert.NoError(t, err) + loggedIn(t, current) + buf := new(bytes.Buffer) - // an empty domain makes every SetContextKey call fail (no domain configured), - // so Logout should bail out with an error instead of printing success. - err := Logout("", buf) - assert.Error(t, err) - assert.NotContains(t, buf.String(), "Successfully logged out of Astronomer") - assert.Contains(t, buf.String(), "Failed to clear access token") + err = Logout("never-logged-in.io", buf) + assert.ErrorContains(t, err, "never-logged-in.io") + assert.Empty(t, buf.String()) + // no stub context is written, and the real login is untouched + assert.False(t, (&config.Context{Domain: "never-logged-in.io"}).ContextExists()) + assert.Equal(t, "some-refresh-token", stored(t, current.Domain).RefreshToken) + domain, err := config.GetCurrentDomain() + assert.NoError(t, err) + assert.Equal(t, current.Domain, domain) }) } diff --git a/cmd/auth_test.go b/cmd/auth_test.go index d6cf580fd..b907719fd 100644 --- a/cmd/auth_test.go +++ b/cmd/auth_test.go @@ -2,6 +2,7 @@ package cmd import ( "bytes" + "errors" "io" "os" @@ -64,9 +65,13 @@ func (s *CmdSuite) TestLogout() { localDomain := "localhost" apcDomain := "astronomer_dev.com" + origCloudLogout, origAPCLogout := cloudLogout, apcLogout + defer func() { cloudLogout, apcLogout = origCloudLogout, origAPCLogout }() + + var cloudLogoutErr error cloudLogout = func(domain string, out io.Writer) error { s.Equal(localDomain, domain) - return nil + return cloudLogoutErr } apcLogout = func(domain string) { s.Equal(apcDomain, domain) @@ -76,6 +81,12 @@ func (s *CmdSuite) TestLogout() { err := logout(&cobra.Command{}, []string{localDomain}, os.Stdout) s.NoError(err) + // a cloud logout that fails fails the command, so it exits non-zero + cloudLogoutErr = errors.New("failed to clear the credentials") + err = logout(&cobra.Command{}, []string{localDomain}, os.Stdout) + s.ErrorIs(err, cloudLogoutErr) + cloudLogoutErr = nil + // software logout success err = logout(&cobra.Command{}, []string{apcDomain}, os.Stdout) s.NoError(err) diff --git a/config/context.go b/config/context.go index 258c91d05..cce5b699f 100644 --- a/config/context.go +++ b/config/context.go @@ -159,16 +159,40 @@ func (c *Context) SetContextKey(key, value string) error { // a partial struct with every unset field zeroed out. // See https://github.com/spf13/viper/issues/1106. func setContextField(cKey, field string, value interface{}) error { + return updateContextMap(cKey, func(ctxMap map[string]interface{}) { ctxMap[field] = value }) +} + +// updateContextMap applies update to the context's map and persists the config +// in one write, for the reason setContextField gives. +func updateContextMap(cKey string, update func(map[string]interface{})) error { parentPath := fmt.Sprintf("%s.%s", contextsKey, cKey) ctxMap := viperHome.GetStringMap(parentPath) if ctxMap == nil { ctxMap = map[string]interface{}{} } - ctxMap[field] = value + update(ctxMap) viperHome.Set(parentPath, ctxMap) return saveConfig(viperHome, HomeConfigFile) } +// ClearCredentials empties every credential the context holds: the access +// token, the refresh token, the user's email and the token's expiry. It is one +// write, so a logout that fails to save leaves the context as it was rather +// than with the long-lived refresh token still in place. +func (c *Context) ClearCredentials() error { + cKey, err := c.GetContextKey() + if err != nil { + return err + } + return updateContextMap(cKey, func(ctxMap map[string]interface{}) { + ctxMap["token"] = "" + ctxMap["refreshtoken"] = "" + ctxMap["user_email"] = "" + // viper lowercases keys, so SetExpiresIn's "ExpiresIn" is stored as this + delete(ctxMap, "expiresin") + }) +} + // set organization id and short name in context config func (c *Context) SetOrganizationContext(orgID, orgProduct string) error { err := c.SetContextKey("organization", orgID) // c.Organization