refactor logout flow, add unit tests

coder · AbhineetJain · May 27, 2022 · May 26, 2022 · May 26, 2022 · May 26, 2022
commit eb12beeef529754bf969f689969d223ad1dfa6f5
diff --git a/cli/logout.go b/cli/logout.go
@@ -3,6 +3,7 @@ package cli
 import (
 	"fmt"
 	"os"
+	"strings"
 
 	"github.com/spf13/cobra"
 	"golang.org/x/xerrors"
@@ -20,7 +21,7 @@ func logout() *cobra.Command {
 				return err
 			}
 
-			var isLoggedOut bool
+			var errors []error
 
 			config := createConfig(cmd)
 
@@ -33,43 +34,40 @@ func logout() *cobra.Command {
 				return err
 			}
 
-			err = config.URL().Delete()
+			err = client.Logout(cmd.Context())
 			if err != nil {
-				// 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)
-				}
-				isLoggedOut = true
+				errors = append(errors, xerrors.Errorf("logout api: %w", err))
+			}
+
+			err = config.URL().Delete()
+			// Only throw error if the URL configuration file is present,
+			// otherwise the user is already logged out, and we proceed
+			if err != nil && !os.IsNotExist(err) {
+				errors = append(errors, xerrors.Errorf("remove URL file: %w", err))
 			}
 
 			err = config.Session().Delete()
-			if err != nil {
-				// 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)
-				}
-				isLoggedOut = true
+			// Only throw error if the session configuration file is present,
+			// otherwise the user is already logged out, and we proceed
+			if err != nil && !os.IsNotExist(err) {
+				errors = append(errors, xerrors.Errorf("remove session file: %w", err))
 			}
 
 			err = config.Organization().Delete()
 			// If the organization configuration file is absent, we still proceed
 			if err != nil && !os.IsNotExist(err) {
-				return xerrors.Errorf("remove organization file: %w", err)
+				errors = append(errors, xerrors.Errorf("remove organization file: %w", err))
 			}
 
-			err = client.Logout(cmd.Context())
-			if err != nil {
-				return xerrors.Errorf("logout: %w", err)
-			}
-
-			// 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")
+			if len(errors) > 0 {
+				var errorStringBuilder strings.Builder
+				for _, err := range errors {
+					_, _ = fmt.Fprint(&errorStringBuilder, "\t"+err.Error()+"\n")
+				}
+				errorString := strings.TrimRight(errorStringBuilder.String(), "\n")
+				return xerrors.New("Failed to log out.\n" + errorString)
 			}
+			_, _ = fmt.Fprintf(cmd.OutOrStdout(), caret+"You are no longer logged in. Try logging in using 'coder login <url>'.\n")
 			return nil
 		},
 	}

diff --git a/cli/logout_test.go b/cli/logout_test.go
@@ -2,6 +2,7 @@ package cli_test
 
 import (
 	"os"
+	"regexp"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -21,7 +22,7 @@ func TestLogout(t *testing.T) {
 		pty := ptytest.New(t)
 		config := login(t, pty)
 
-		// ensure session files exist
+		// Ensure session files exist.
 		require.FileExists(t, string(config.URL()))
 		require.FileExists(t, string(config.Session()))
 
@@ -40,7 +41,7 @@ func TestLogout(t *testing.T) {
 
 		pty.ExpectMatch("Are you sure you want to logout?")
 		pty.WriteLine("yes")
-		pty.ExpectMatch("Successfully logged out")
+		pty.ExpectMatch("You are no longer logged in. Try logging in using 'coder login <url>'.")
 		<-logoutChan
 	})
 	t.Run("SkipPrompt", func(t *testing.T) {
@@ -49,7 +50,7 @@ func TestLogout(t *testing.T) {
 		pty := ptytest.New(t)
 		config := login(t, pty)
 
-		// ensure session files exist
+		// Ensure session files exist.
 		require.FileExists(t, string(config.URL()))
 		require.FileExists(t, string(config.Session()))
 
@@ -66,7 +67,7 @@ func TestLogout(t *testing.T) {
 			assert.NoFileExists(t, string(config.Session()))
 		}()
 
-		pty.ExpectMatch("Successfully logged out")
+		pty.ExpectMatch("You are no longer logged in. Try logging in using 'coder login <url>'.")
 		<-logoutChan
 	})
 	t.Run("NoURLFile", func(t *testing.T) {
@@ -75,7 +76,7 @@ func TestLogout(t *testing.T) {
 		pty := ptytest.New(t)
 		config := login(t, pty)
 
-		// ensure session files exist
+		// Ensure session files exist.
 		require.FileExists(t, string(config.URL()))
 		require.FileExists(t, string(config.Session()))
 
@@ -91,14 +92,9 @@ func TestLogout(t *testing.T) {
 		go func() {
 			defer close(logoutChan)
 			err := logout.Execute()
-			assert.NoError(t, err)
-			assert.NoFileExists(t, string(config.URL()))
-			assert.NoFileExists(t, string(config.Session()))
+			assert.EqualError(t, err, "You are not logged in. Try logging in using 'coder login <url>'.")
 		}()
 
-		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 <url>'.")
 		<-logoutChan
 	})
 	t.Run("NoSessionFile", func(t *testing.T) {
@@ -107,7 +103,7 @@ func TestLogout(t *testing.T) {
 		pty := ptytest.New(t)
 		config := login(t, pty)
 
-		// ensure session files exist
+		// Ensure session files exist.
 		require.FileExists(t, string(config.URL()))
 		require.FileExists(t, string(config.Session()))
 
@@ -123,15 +119,45 @@ func TestLogout(t *testing.T) {
 		go func() {
 			defer close(logoutChan)
 			err = logout.Execute()
-			assert.NoError(t, err)
-			assert.NoFileExists(t, string(config.URL()))
-			assert.NoFileExists(t, string(config.Session()))
+			assert.EqualError(t, err, "You are not logged in. Try logging in using 'coder login <url>'.")
+		}()
+
+		<-logoutChan
+	})
+	t.Run("ConfigDirectoryPermissionDenied", 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()))
+
+		// Changing the permissions to throw error during deletion.
+		err := os.Chmod(string(config), 0500)
+		require.NoError(t, err)
+
+		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()
+			errRegex := regexp.MustCompile("Failed to log out.\n\tremove URL file: .+: permission denied\n\tremove session file: .+: permission denied")
+			assert.Regexp(t, errRegex, err.Error())
 		}()
 
 		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 <url>'.")
 		<-logoutChan
+
+		// Setting the permissions back for cleanup.
+		err = os.Chmod(string(config), 0700)
+		require.NoError(t, err)
 	})
 }
 

diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -206,7 +206,6 @@ func New(options *Options) *API {
 			r.Get("/first", api.firstUser)
 			r.Post("/first", api.postFirstUser)
 			r.Post("/login", api.postLogin)
-			r.Post("/logout", api.postLogout)
 			r.Get("/authmethods", api.userAuthMethods)
 			r.Route("/oauth2", func(r chi.Router) {
 				r.Route("/github", func(r chi.Router) {
@@ -221,6 +220,7 @@ func New(options *Options) *API {
 				)
 				r.Post("/", api.postUser)
 				r.Get("/", api.users)
+				r.Post("/logout", api.postLogout)
 				// These routes query information about site wide roles.
 				r.Route("/roles", func(r chi.Router) {
 					r.Get("/", api.assignableSiteRoles)

diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go
@@ -83,6 +83,10 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
 	workspaceRBACObj := rbac.ResourceWorkspace.InOrg(organization.ID).WithID(workspace.ID.String()).WithOwner(workspace.OwnerID.String())
 
 	// skipRoutes allows skipping routes from being checked.
+	skipRoutes := map[string]string{
+		"POST:/api/v2/users/logout": "Logging out deletes the API Key for other routes",
+	}
+
 	type routeCheck struct {
 		NoAuthorize  bool
 		AssertAction rbac.Action
@@ -96,7 +100,6 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
 		"GET:/api/v2/users/first":       {NoAuthorize: true},
 		"POST:/api/v2/users/first":      {NoAuthorize: true},
 		"POST:/api/v2/users/login":      {NoAuthorize: true},
-		"POST:/api/v2/users/logout":     {NoAuthorize: true},
 		"GET:/api/v2/users/authmethods": {NoAuthorize: true},
 
 		// Has it's own auth
@@ -267,8 +270,20 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
 		assertRoute[noTrailSlash] = v
 	}
 
+	for k, v := range skipRoutes {
+		noTrailSlash := strings.TrimRight(k, "/")
+		if _, ok := skipRoutes[noTrailSlash]; ok && noTrailSlash != k {
+			t.Errorf("route %q & %q is declared twice", noTrailSlash, k)
+			t.FailNow()
+		}
+		skipRoutes[noTrailSlash] = v
+	}
+
 	err = chi.Walk(api.Handler, func(method string, route string, handler http.Handler, middlewares ...func(http.Handler) http.Handler) error {
 		name := method + ":" + route
+		if _, ok := skipRoutes[strings.TrimRight(name, "/")]; ok {
+			return nil
+		}
 		t.Run(name, func(t *testing.T) {
 			authorizer.reset()
 			routeAssertions, ok := assertRoute[strings.TrimRight(name, "/")]

diff --git a/coderd/users_test.go b/coderd/users_test.go
@@ -2,15 +2,18 @@ package coderd_test
 
 import (
 	"context"
+	"database/sql"
 	"fmt"
 	"net/http"
 	"sort"
+	"strings"
 	"testing"
 
 	"github.com/google/uuid"
 	"github.com/stretchr/testify/require"
 
 	"github.com/coder/coder/coderd/coderdtest"
+	"github.com/coder/coder/coderd/database/databasefake"
 	"github.com/coder/coder/coderd/httpmw"
 	"github.com/coder/coder/coderd/rbac"
 	"github.com/coder/coder/codersdk"
@@ -103,27 +106,71 @@ func TestPostLogin(t *testing.T) {
 func TestPostLogout(t *testing.T) {
 	t.Parallel()
 
-	t.Run("ClearCookie", func(t *testing.T) {
+	// Checks that the cookie is cleared and the API Key is deleted from the database.
+	t.Run("Logout", func(t *testing.T) {
 		t.Parallel()
 
-		client := coderdtest.New(t, nil)
+		ctx := context.Background()
+		client, api := coderdtest.NewWithAPI(t, nil)
+		_ = coderdtest.CreateFirstUser(t, client)
+		keyID := strings.Split(client.SessionToken, "-")[0]
+
+		apiKey, err := api.Database.GetAPIKeyByID(ctx, keyID)
+		require.NoError(t, err)
+		require.Equal(t, keyID, apiKey.ID, "API key should exist in the database")
+
 		fullURL, err := client.URL.Parse("/api/v2/users/logout")
 		require.NoError(t, err, "Server URL should parse successfully")
 
-		req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, fullURL.String(), nil)
-		require.NoError(t, err, "/logout request construction should succeed")
+		res, err := client.Request(ctx, http.MethodPost, fullURL.String(), nil)
+		require.NoError(t, err, "/logout request should succeed")
+		res.Body.Close()
+		require.Equal(t, http.StatusOK, res.StatusCode)
+
+		cookies := res.Cookies()
+		require.Len(t, cookies, 1, "Exactly one cookie should be returned")
 
-		httpClient := &http.Client{}
+		require.Equal(t, httpmw.SessionTokenKey, cookies[0].Name, "Cookie should be the auth cookie")
+		require.Equal(t, -1, cookies[0].MaxAge, "Cookie should be set to delete")
 
-		response, err := httpClient.Do(req)
+		apiKey, err = api.Database.GetAPIKeyByID(ctx, keyID)
+		require.ErrorIs(t, err, sql.ErrNoRows, "API key should not exist in the database")
+	})
+
+	t.Run("LogoutWithoutKey", func(t *testing.T) {
+		t.Parallel()
+
+		ctx := context.Background()
+		client, api := coderdtest.NewWithAPI(t, nil)
+		_ = coderdtest.CreateFirstUser(t, client)
+		keyID := strings.Split(client.SessionToken, "-")[0]
+
+		apiKey, err := api.Database.GetAPIKeyByID(ctx, keyID)
+		require.NoError(t, err)
+		require.Equal(t, keyID, apiKey.ID, "API key should exist in the database")
+
+		// Setting a fake database without the API Key to be used by the API.
+		// The middleware that extracts the API key is already set to read
+		// from the original database.
+		dbWithoutKey := databasefake.New()
+		api.Database = dbWithoutKey
+
+		fullURL, err := client.URL.Parse("/api/v2/users/logout")
+		require.NoError(t, err, "Server URL should parse successfully")
+
+		res, err := client.Request(ctx, http.MethodPost, fullURL.String(), nil)
 		require.NoError(t, err, "/logout request should succeed")
-		response.Body.Close()
+		res.Body.Close()
+		require.Equal(t, http.StatusInternalServerError, res.StatusCode)
 
-		cookies := response.Cookies()
+		cookies := res.Cookies()
 		require.Len(t, cookies, 1, "Exactly one cookie should be returned")
 
-		require.Equal(t, cookies[0].Name, httpmw.SessionTokenKey, "Cookie should be the auth cookie")
-		require.Equal(t, cookies[0].MaxAge, -1, "Cookie should be set to delete")
+		require.Equal(t, httpmw.SessionTokenKey, cookies[0].Name, "Cookie should be the auth cookie")
+		require.Equal(t, -1, cookies[0].MaxAge, "Cookie should be set to delete")
+
+		apiKey, err = api.Database.GetAPIKeyByID(ctx, keyID)
+		require.ErrorIs(t, err, sql.ErrNoRows, "API key should not exist in the database")
 	})
 }