feat: delete API token in /logout API (#1770)

* delete API token in logout api

* add deleteapikeybyid to databasefake

* set blank cookie on logout always

* refactor logout flow, add unit tests

* update logout messsage

* use read-only file mode for windows

* fix file mode on windows for cleanup

* change file permissions on windows

* assert error is not nil

* refactor cli

* try different file mode on windows

* try different file mode on windows

* try keeping the files open on Windows

* fix the error message on Windows

cli/logout.go

@@ -3,6 +3,7 @@ package cli
 import (
 	"fmt"
 	"os"
+	"strings"
 
 	"github.com/spf13/cobra"
 	"golang.org/x/xerrors"
@@ -15,11 +16,16 @@ func logout() *cobra.Command {
 		Use:   "logout",
 		Short: "Remove the local authenticated session",
 		RunE: func(cmd *cobra.Command, args []string) error {
-			var isLoggedOut bool
+			client, err := createClient(cmd)
+			if err != nil {
+				return err
+			}
+
+			var errors []error
 
 			config := createConfig(cmd)
 
-			_, err := cliui.Prompt(cmd, cliui.PromptOptions{
+			_, err = cliui.Prompt(cmd, cliui.PromptOptions{
 				Text:      "Are you sure you want to logout?",
 				IsConfirm: true,
 				Default:   "yes",
@@ -28,38 +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))
 			}
 
-			// 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. You can log in using 'coder login <url>'.\n")
 			return nil
 		},
 	}

cli/logout_test.go

@@ -1,7 +1,10 @@
 package cli_test
 
 import (
+	"fmt"
 	"os"
+	"regexp"
+	"runtime"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -21,7 +24,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 +43,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. You can log in using 'coder login <url>'.")
 		<-logoutChan
 	})
 	t.Run("SkipPrompt", func(t *testing.T) {
@@ -49,7 +52,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 +69,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. You can log in using 'coder login <url>'.")
 		<-logoutChan
 	})
 	t.Run("NoURLFile", func(t *testing.T) {
@@ -75,7 +78,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 +94,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 +105,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,14 +121,73 @@ 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("CannotDeleteFiles", 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()))
+
+		var (
+			err         error
+			urlFile     *os.File
+			sessionFile *os.File
+		)
+		if runtime.GOOS == "windows" {
+			// Opening the files so Windows does not allow deleting them.
+			urlFile, err = os.Open(string(config.URL()))
+			require.NoError(t, err)
+			sessionFile, err = os.Open(string(config.Session()))
+			require.NoError(t, err)
+		} else {
+			// Changing the permissions to throw error during deletion.
+			err = os.Chmod(string(config), 0500)
+			require.NoError(t, err)
+		}
+		t.Cleanup(func() {
+			if runtime.GOOS == "windows" {
+				// Closing the opened files for cleanup.
+				err = urlFile.Close()
+				require.NoError(t, err)
+				err = sessionFile.Close()
+				require.NoError(t, err)
+			} else {
+				// Setting the permissions back for cleanup.
+				err = os.Chmod(string(config), 0700)
+				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()
+			assert.NotNil(t, err)
+			var errorMessage string
+			if runtime.GOOS == "windows" {
+				errorMessage = "The process cannot access the file because it is being used by another process."
+			} else {
+				errorMessage = "permission denied"
+			}
+			errRegex := regexp.MustCompile(fmt.Sprintf("Failed to log out.\n\tremove URL file: .+: %s\n\tremove session file: .+: %s", errorMessage, errorMessage))
+			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
 	})
 }

coderd/coderd.go

@@ -219,7 +219,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) {
@@ -234,6 +233,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)

coderd/coderd_test.go

@@ -104,6 +104,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
@@ -117,7 +121,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},
 		"POST:/api/v2/csp/reports":      {NoAuthorize: true},
 
@@ -310,8 +313,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, "/")]

coderd/database/databasefake/databasefake.go

@@ -126,6 +126,21 @@ func (q *fakeQuerier) GetAPIKeyByID(_ context.Context, id string) (database.APIK
 	return database.APIKey{}, sql.ErrNoRows
 }
 
+func (q *fakeQuerier) DeleteAPIKeyByID(_ context.Context, id string) error {
+	q.mutex.Lock()
+	defer q.mutex.Unlock()
+
+	for index, apiKey := range q.apiKeys {
+		if apiKey.ID != id {
+			continue
+		}
+		q.apiKeys[index] = q.apiKeys[len(q.apiKeys)-1]
+		q.apiKeys = q.apiKeys[:len(q.apiKeys)-1]
+		return nil
+	}
+	return sql.ErrNoRows
+}
+
 func (q *fakeQuerier) GetFileByHash(_ context.Context, hash string) (database.File, error) {
 	q.mutex.RLock()
 	defer q.mutex.RUnlock()

coderd/database/queries/apikeys.sql

@@ -38,3 +38,10 @@ SET
 	oauth_expiry = $6
 WHERE
 	id = $1;
+
+-- name: DeleteAPIKeyByID :exec
+DELETE
+FROM
+	api_keys
+WHERE
+	id = $1;