From abff3c838dfe048839d4c64b20e34007e3dfc37f Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Mon, 23 May 2022 21:25:44 +0000 Subject: [PATCH 1/5] Improve CLI logout flow --- cli/logout.go | 31 +++++++++++++++++++++--- cli/logout_test.go | 60 ++++++++++++++++++++++++++++++++++++---------- cli/root.go | 5 ++-- 3 files changed, 79 insertions(+), 17 deletions(-) diff --git a/cli/logout.go b/cli/logout.go index 0271454e9c374..f46d97688fc06 100644 --- a/cli/logout.go +++ b/cli/logout.go @@ -11,12 +11,37 @@ import ( func logout() *cobra.Command { return &cobra.Command{ Use: "logout", - Short: "Remove local autheticated session", + Short: "Remove the local authenticated session", RunE: func(cmd *cobra.Command, args []string) error { config := createConfig(cmd) - err := os.RemoveAll(string(config)) + + loginHelper := "You are not logged in. Try logging in using 'coder login '." + + err := config.URL().Delete() + + if err != nil { + // If the URL configuration file is absent, the user is logged out + if os.IsNotExist(err) { + return xerrors.New(loginHelper) + } + return xerrors.Errorf("remove URL file: %w", err) + } + + err = config.Session().Delete() + if err != nil { - return xerrors.Errorf("remove files at %s: %w", config, err) + // If the session configuration file is absent, the user is logged out + if os.IsNotExist(err) { + return xerrors.New(loginHelper) + } + return xerrors.Errorf("remove session file: %w", err) + } + + err = config.Organization().Delete() + + // If the organization configuration file is absent, we should still log out + if err != nil && !os.IsNotExist(err) { + return xerrors.Errorf("remove organization file: %w", err) } _, _ = fmt.Fprintf(cmd.OutOrStdout(), caret+"Successfully logged out.\n") diff --git a/cli/logout_test.go b/cli/logout_test.go index 2e36d114aa047..d0bcabdd6ac0e 100644 --- a/cli/logout_test.go +++ b/cli/logout_test.go @@ -1,19 +1,63 @@ package cli_test import ( + "os" "testing" - "github.com/stretchr/testify/require" + "github.com/stretchr/testify/assert" "github.com/coder/coder/cli/clitest" + "github.com/coder/coder/cli/config" "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/pty/ptytest" ) func TestLogout(t *testing.T) { t.Parallel() + t.Run("Logout", func(t *testing.T) { + t.Parallel() + + config := login(t) + + // ensure session files exist + assert.FileExists(t, string(config.URL())) + assert.FileExists(t, string(config.Session())) + + logout, _ := clitest.New(t, "logout", "--global-config", string(config)) + err := logout.Execute() + assert.NoError(t, err) + assert.NoFileExists(t, string(config.URL())) + assert.NoFileExists(t, string(config.Session())) + }) + t.Run("NoURLFile", func(t *testing.T) { + t.Parallel() + + logout, _ := clitest.New(t, "logout") + + err := logout.Execute() + assert.EqualError(t, err, "You are not logged in. Try logging in using 'coder login '.") + }) + t.Run("NoSessionFile", func(t *testing.T) { + t.Parallel() + + config := login(t) + + // ensure session files exist + assert.FileExists(t, string(config.URL())) + assert.FileExists(t, string(config.Session())) + + os.RemoveAll(string(config.Session())) + + logout, _ := clitest.New(t, "logout", "--global-config", string(config)) + + err := logout.Execute() + assert.EqualError(t, err, "You are not logged in. Try logging in using 'coder login '.") + }) +} + +func login(t *testing.T) config.Root { + t.Helper() - // login client := coderdtest.New(t, nil) coderdtest.CreateFirstUser(t, client) @@ -25,7 +69,7 @@ func TestLogout(t *testing.T) { go func() { defer close(doneChan) err := root.Execute() - require.NoError(t, err) + assert.NoError(t, err) }() pty.ExpectMatch("Paste your token here:") @@ -33,13 +77,5 @@ func TestLogout(t *testing.T) { pty.ExpectMatch("Welcome to Coder") <-doneChan - // ensure session files exist - require.FileExists(t, string(config.URL())) - require.FileExists(t, string(config.Session())) - - logout, _ := clitest.New(t, "logout", "--global-config", string(config)) - err := logout.Execute() - require.NoError(t, err) - require.NoFileExists(t, string(config.URL())) - require.NoFileExists(t, string(config.Session())) + return config } diff --git a/cli/root.go b/cli/root.go index 7398986608b79..b8919369d97c4 100644 --- a/cli/root.go +++ b/cli/root.go @@ -112,12 +112,13 @@ func Root() *cobra.Command { func createClient(cmd *cobra.Command) (*codersdk.Client, error) { root := createConfig(cmd) rawURL, err := cmd.Flags().GetString(varURL) + loginHelper := "You are not logged in. Try logging in using 'coder login '." if err != nil || rawURL == "" { rawURL, err = root.URL().Read() if err != nil { // If the configuration files are absent, the user is logged out if os.IsNotExist(err) { - return nil, xerrors.New("You are not logged in. Try logging in using 'coder login '.") + return nil, xerrors.New(loginHelper) } return nil, err } @@ -132,7 +133,7 @@ func createClient(cmd *cobra.Command) (*codersdk.Client, error) { if err != nil { // If the configuration files are absent, the user is logged out if os.IsNotExist(err) { - return nil, xerrors.New("You are not logged in. Try logging in using 'coder login '.") + return nil, xerrors.New(loginHelper) } return nil, err } From a7775b02ac6f21012cf22a19d4458224dd51535d Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Mon, 23 May 2022 21:35:40 +0000 Subject: [PATCH 2/5] Fix lint error --- cli/logout_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/logout_test.go b/cli/logout_test.go index d0bcabdd6ac0e..2e7b7ec5a8431 100644 --- a/cli/logout_test.go +++ b/cli/logout_test.go @@ -62,7 +62,7 @@ func login(t *testing.T) config.Root { coderdtest.CreateFirstUser(t, client) doneChan := make(chan struct{}) - root, config := clitest.New(t, "login", "--force-tty", client.URL.String(), "--no-open") + root, cfg := clitest.New(t, "login", "--force-tty", client.URL.String(), "--no-open") pty := ptytest.New(t) root.SetIn(pty.Input()) root.SetOut(pty.Output()) @@ -77,5 +77,5 @@ func login(t *testing.T) config.Root { pty.ExpectMatch("Welcome to Coder") <-doneChan - return config + return cfg } From 0584479cd3c941bdaffba74a297c5bc0db0b834e Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Mon, 23 May 2022 22:42:16 +0000 Subject: [PATCH 3/5] Make notLoggedInMessage a const --- cli/logout.go | 9 ++------- cli/root.go | 20 ++++++++++---------- 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/cli/logout.go b/cli/logout.go index f46d97688fc06..cfc7ce3b29c0b 100644 --- a/cli/logout.go +++ b/cli/logout.go @@ -15,30 +15,25 @@ func logout() *cobra.Command { RunE: func(cmd *cobra.Command, args []string) error { config := createConfig(cmd) - loginHelper := "You are not logged in. Try logging in using 'coder login '." - err := config.URL().Delete() - if err != nil { // If the URL configuration file is absent, the user is logged out if os.IsNotExist(err) { - return xerrors.New(loginHelper) + return xerrors.New(notLoggedInMessage) } return xerrors.Errorf("remove URL file: %w", err) } err = config.Session().Delete() - if err != nil { // If the session configuration file is absent, the user is logged out if os.IsNotExist(err) { - return xerrors.New(loginHelper) + return xerrors.New(notLoggedInMessage) } return xerrors.Errorf("remove session file: %w", err) } err = config.Organization().Delete() - // If the organization configuration file is absent, we should still log out if err != nil && !os.IsNotExist(err) { return xerrors.Errorf("remove organization file: %w", err) diff --git a/cli/root.go b/cli/root.go index b8919369d97c4..1314c71ea280d 100644 --- a/cli/root.go +++ b/cli/root.go @@ -31,13 +31,14 @@ var ( ) const ( - varURL = "url" - varToken = "token" - varAgentToken = "agent-token" - varAgentURL = "agent-url" - varGlobalConfig = "global-config" - varNoOpen = "no-open" - varForceTty = "force-tty" + varURL = "url" + varToken = "token" + varAgentToken = "agent-token" + varAgentURL = "agent-url" + varGlobalConfig = "global-config" + varNoOpen = "no-open" + varForceTty = "force-tty" + notLoggedInMessage = "You are not logged in. Try logging in using 'coder login '." ) func init() { @@ -112,13 +113,12 @@ func Root() *cobra.Command { func createClient(cmd *cobra.Command) (*codersdk.Client, error) { root := createConfig(cmd) rawURL, err := cmd.Flags().GetString(varURL) - loginHelper := "You are not logged in. Try logging in using 'coder login '." if err != nil || rawURL == "" { rawURL, err = root.URL().Read() if err != nil { // If the configuration files are absent, the user is logged out if os.IsNotExist(err) { - return nil, xerrors.New(loginHelper) + return nil, xerrors.New(notLoggedInMessage) } return nil, err } @@ -133,7 +133,7 @@ func createClient(cmd *cobra.Command) (*codersdk.Client, error) { if err != nil { // If the configuration files are absent, the user is logged out if os.IsNotExist(err) { - return nil, xerrors.New(loginHelper) + return nil, xerrors.New(notLoggedInMessage) } return nil, err } From 33adc09e6abec3a9dc733be0db5ad0d567d5e5d8 Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Mon, 23 May 2022 23:46:37 +0000 Subject: [PATCH 4/5] successful logout with a msg when cfg files are absent --- cli/logout.go | 27 +++++++++++------ cli/logout_test.go | 72 ++++++++++++++++++++++++++++++++++++-------- cli/userlist_test.go | 1 + 3 files changed, 78 insertions(+), 22 deletions(-) diff --git a/cli/logout.go b/cli/logout.go index cfc7ce3b29c0b..6f6d6d4e9b468 100644 --- a/cli/logout.go +++ b/cli/logout.go @@ -13,32 +13,41 @@ func logout() *cobra.Command { Use: "logout", Short: "Remove the local authenticated session", RunE: func(cmd *cobra.Command, args []string) error { + var isLoggedOut bool + config := createConfig(cmd) err := config.URL().Delete() if err != nil { - // If the URL configuration file is absent, the user is logged out - if os.IsNotExist(err) { - return xerrors.New(notLoggedInMessage) + // Only throw error if the URL configuration file is present, + // otherwise the user is already logged out, and we proceed + if !os.IsNotExist(err) { + return xerrors.Errorf("remove URL file: %w", err) } - return xerrors.Errorf("remove URL file: %w", err) + isLoggedOut = true } err = config.Session().Delete() if err != nil { - // If the session configuration file is absent, the user is logged out - if os.IsNotExist(err) { - return xerrors.New(notLoggedInMessage) + // Only throw error if the session configuration file is present, + // otherwise the user is already logged out, and we proceed + if !os.IsNotExist(err) { + return xerrors.Errorf("remove session file: %w", err) } - return xerrors.Errorf("remove session file: %w", err) + isLoggedOut = true } err = config.Organization().Delete() - // If the organization configuration file is absent, we should still log out + // If the organization configuration file is absent, we still proceed if err != nil && !os.IsNotExist(err) { return xerrors.Errorf("remove organization file: %w", err) } + // If the user was already logged out, we show them a message + if isLoggedOut { + _, _ = fmt.Fprintf(cmd.OutOrStdout(), notLoggedInMessage+"\n") + } + _, _ = fmt.Fprintf(cmd.OutOrStdout(), caret+"Successfully logged out.\n") return nil }, diff --git a/cli/logout_test.go b/cli/logout_test.go index 2e7b7ec5a8431..8600aa44a479f 100644 --- a/cli/logout_test.go +++ b/cli/logout_test.go @@ -17,30 +17,64 @@ func TestLogout(t *testing.T) { t.Run("Logout", func(t *testing.T) { t.Parallel() - config := login(t) + pty := ptytest.New(t) + config := login(t, pty) // ensure session files exist assert.FileExists(t, string(config.URL())) assert.FileExists(t, string(config.Session())) + logoutChan := make(chan struct{}) logout, _ := clitest.New(t, "logout", "--global-config", string(config)) - err := logout.Execute() - assert.NoError(t, err) - assert.NoFileExists(t, string(config.URL())) - assert.NoFileExists(t, string(config.Session())) + logout.SetIn(pty.Input()) + logout.SetOut(pty.Output()) + + go func() { + defer close(logoutChan) + err := logout.Execute() + assert.NoError(t, err) + assert.NoFileExists(t, string(config.URL())) + assert.NoFileExists(t, string(config.Session())) + }() + + pty.ExpectMatch("Successfully logged out") + <-logoutChan }) t.Run("NoURLFile", func(t *testing.T) { t.Parallel() - logout, _ := clitest.New(t, "logout") + pty := ptytest.New(t) + config := login(t, pty) + + // ensure session files exist + assert.FileExists(t, string(config.URL())) + assert.FileExists(t, string(config.Session())) + + os.RemoveAll(string(config.URL())) - err := logout.Execute() - assert.EqualError(t, err, "You are not logged in. Try logging in using 'coder login '.") + logoutChan := make(chan struct{}) + logout, _ := clitest.New(t, "logout", "--global-config", string(config)) + + logout.SetIn(pty.Input()) + logout.SetOut(pty.Output()) + + go func() { + defer close(logoutChan) + err := logout.Execute() + assert.NoError(t, err) + assert.NoFileExists(t, string(config.URL())) + assert.NoFileExists(t, string(config.Session())) + }() + + pty.ExpectMatch("You are not logged in. Try logging in using 'coder login '.") + pty.ExpectMatch("Successfully logged out") + <-logoutChan }) t.Run("NoSessionFile", func(t *testing.T) { t.Parallel() - config := login(t) + pty := ptytest.New(t) + config := login(t, pty) // ensure session files exist assert.FileExists(t, string(config.URL())) @@ -48,14 +82,27 @@ func TestLogout(t *testing.T) { os.RemoveAll(string(config.Session())) + logoutChan := make(chan struct{}) logout, _ := clitest.New(t, "logout", "--global-config", string(config)) - err := logout.Execute() - assert.EqualError(t, err, "You are not logged in. Try logging in using 'coder login '.") + logout.SetIn(pty.Input()) + logout.SetOut(pty.Output()) + + go func() { + defer close(logoutChan) + err := logout.Execute() + assert.NoError(t, err) + assert.NoFileExists(t, string(config.URL())) + assert.NoFileExists(t, string(config.Session())) + }() + + pty.ExpectMatch("You are not logged in. Try logging in using 'coder login '.") + pty.ExpectMatch("Successfully logged out") + <-logoutChan }) } -func login(t *testing.T) config.Root { +func login(t *testing.T, pty *ptytest.PTY) config.Root { t.Helper() client := coderdtest.New(t, nil) @@ -63,7 +110,6 @@ func login(t *testing.T) config.Root { doneChan := make(chan struct{}) root, cfg := clitest.New(t, "login", "--force-tty", client.URL.String(), "--no-open") - pty := ptytest.New(t) root.SetIn(pty.Input()) root.SetOut(pty.Output()) go func() { diff --git a/cli/userlist_test.go b/cli/userlist_test.go index 8529f6980fb9c..7c3d596e220b0 100644 --- a/cli/userlist_test.go +++ b/cli/userlist_test.go @@ -12,6 +12,7 @@ import ( ) func TestUserList(t *testing.T) { + t.Parallel() t.Run("List", func(t *testing.T) { t.Parallel() client := coderdtest.New(t, nil) From 06142b056e8b004f3f74d100cb248274ea4709e8 Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Tue, 24 May 2022 15:54:11 +0000 Subject: [PATCH 5/5] use require, os.remove, show only one message, add prompt --- cli/logout.go | 24 +++++++++++++++----- cli/logout_test.go | 55 ++++++++++++++++++++++++++++++++++++---------- 2 files changed, 63 insertions(+), 16 deletions(-) diff --git a/cli/logout.go b/cli/logout.go index 6f6d6d4e9b468..15d57b37c0f5b 100644 --- a/cli/logout.go +++ b/cli/logout.go @@ -6,10 +6,12 @@ import ( "github.com/spf13/cobra" "golang.org/x/xerrors" + + "github.com/coder/coder/cli/cliui" ) func logout() *cobra.Command { - return &cobra.Command{ + cmd := &cobra.Command{ Use: "logout", Short: "Remove the local authenticated session", RunE: func(cmd *cobra.Command, args []string) error { @@ -17,7 +19,16 @@ func logout() *cobra.Command { config := createConfig(cmd) - err := config.URL().Delete() + _, err := cliui.Prompt(cmd, cliui.PromptOptions{ + Text: "Are you sure you want to logout?", + IsConfirm: true, + Default: "yes", + }) + if err != nil { + return err + } + + err = config.URL().Delete() if err != nil { // Only throw error if the URL configuration file is present, // otherwise the user is already logged out, and we proceed @@ -43,13 +54,16 @@ func logout() *cobra.Command { return xerrors.Errorf("remove organization file: %w", err) } - // If the user was already logged out, we show them a message + // If the user was already logged out, we show them a different message if isLoggedOut { _, _ = fmt.Fprintf(cmd.OutOrStdout(), notLoggedInMessage+"\n") + } else { + _, _ = fmt.Fprintf(cmd.OutOrStdout(), caret+"Successfully logged out.\n") } - - _, _ = fmt.Fprintf(cmd.OutOrStdout(), caret+"Successfully logged out.\n") return nil }, } + + cliui.AllowSkipPrompt(cmd) + return cmd } diff --git a/cli/logout_test.go b/cli/logout_test.go index 8600aa44a479f..bae15ef204ca4 100644 --- a/cli/logout_test.go +++ b/cli/logout_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/coder/coder/cli/clitest" "github.com/coder/coder/cli/config" @@ -21,8 +22,8 @@ func TestLogout(t *testing.T) { config := login(t, pty) // ensure session files exist - assert.FileExists(t, string(config.URL())) - assert.FileExists(t, string(config.Session())) + require.FileExists(t, string(config.URL())) + require.FileExists(t, string(config.Session())) logoutChan := make(chan struct{}) logout, _ := clitest.New(t, "logout", "--global-config", string(config)) @@ -37,6 +38,34 @@ func TestLogout(t *testing.T) { assert.NoFileExists(t, string(config.Session())) }() + pty.ExpectMatch("Are you sure you want to logout?") + pty.WriteLine("yes") + pty.ExpectMatch("Successfully logged out") + <-logoutChan + }) + t.Run("SkipPrompt", func(t *testing.T) { + t.Parallel() + + pty := ptytest.New(t) + config := login(t, pty) + + // ensure session files exist + require.FileExists(t, string(config.URL())) + require.FileExists(t, string(config.Session())) + + logoutChan := make(chan struct{}) + logout, _ := clitest.New(t, "logout", "--global-config", string(config), "-y") + logout.SetIn(pty.Input()) + logout.SetOut(pty.Output()) + + go func() { + defer close(logoutChan) + err := logout.Execute() + assert.NoError(t, err) + assert.NoFileExists(t, string(config.URL())) + assert.NoFileExists(t, string(config.Session())) + }() + pty.ExpectMatch("Successfully logged out") <-logoutChan }) @@ -47,10 +76,11 @@ func TestLogout(t *testing.T) { config := login(t, pty) // ensure session files exist - assert.FileExists(t, string(config.URL())) - assert.FileExists(t, string(config.Session())) + require.FileExists(t, string(config.URL())) + require.FileExists(t, string(config.Session())) - os.RemoveAll(string(config.URL())) + err := os.Remove(string(config.URL())) + require.NoError(t, err) logoutChan := make(chan struct{}) logout, _ := clitest.New(t, "logout", "--global-config", string(config)) @@ -66,8 +96,9 @@ func TestLogout(t *testing.T) { assert.NoFileExists(t, string(config.Session())) }() + pty.ExpectMatch("Are you sure you want to logout?") + pty.WriteLine("yes") pty.ExpectMatch("You are not logged in. Try logging in using 'coder login '.") - pty.ExpectMatch("Successfully logged out") <-logoutChan }) t.Run("NoSessionFile", func(t *testing.T) { @@ -77,10 +108,11 @@ func TestLogout(t *testing.T) { config := login(t, pty) // ensure session files exist - assert.FileExists(t, string(config.URL())) - assert.FileExists(t, string(config.Session())) + require.FileExists(t, string(config.URL())) + require.FileExists(t, string(config.Session())) - os.RemoveAll(string(config.Session())) + err := os.Remove(string(config.Session())) + require.NoError(t, err) logoutChan := make(chan struct{}) logout, _ := clitest.New(t, "logout", "--global-config", string(config)) @@ -90,14 +122,15 @@ func TestLogout(t *testing.T) { go func() { defer close(logoutChan) - err := logout.Execute() + err = logout.Execute() assert.NoError(t, err) assert.NoFileExists(t, string(config.URL())) assert.NoFileExists(t, string(config.Session())) }() + pty.ExpectMatch("Are you sure you want to logout?") + pty.WriteLine("yes") pty.ExpectMatch("You are not logged in. Try logging in using 'coder login '.") - pty.ExpectMatch("Successfully logged out") <-logoutChan }) }