From 5c062df3a9c38eb88c8f6df7016e4107d6f9eb26 Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Thu, 26 May 2022 01:22:24 +0000 Subject: [PATCH 01/14] delete API token in logout api --- cli/logout.go | 12 +++++++++++- coderd/database/querier.go | 1 + coderd/database/queries.sql.go | 13 +++++++++++++ coderd/database/queries/apikeys.sql | 7 +++++++ coderd/users.go | 20 +++++++++++++++++++- 5 files changed, 51 insertions(+), 2 deletions(-) diff --git a/cli/logout.go b/cli/logout.go index 15d57b37c0f5b..317dfca51ee76 100644 --- a/cli/logout.go +++ b/cli/logout.go @@ -15,11 +15,16 @@ func logout() *cobra.Command { Use: "logout", Short: "Remove the local authenticated session", RunE: func(cmd *cobra.Command, args []string) error { + client, err := createClient(cmd) + if err != nil { + return err + } + var isLoggedOut bool 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", @@ -54,6 +59,11 @@ func logout() *cobra.Command { return 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") diff --git a/coderd/database/querier.go b/coderd/database/querier.go index db061cfcb36a1..7d4c361cea05b 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -18,6 +18,7 @@ type querier interface { // multiple provisioners from acquiring the same jobs. See: // https://www.postgresql.org/docs/9.5/sql-select.html#SQL-FOR-UPDATE-SHARE AcquireProvisionerJob(ctx context.Context, arg AcquireProvisionerJobParams) (ProvisionerJob, error) + DeleteAPIKeyByID(ctx context.Context, id string) error DeleteGitSSHKey(ctx context.Context, userID uuid.UUID) error DeleteParameterValueByID(ctx context.Context, id uuid.UUID) error GetAPIKeyByID(ctx context.Context, id string) (APIKey, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 57eeb67ac5814..c4d11c772d702 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -15,6 +15,19 @@ import ( "github.com/tabbed/pqtype" ) +const deleteAPIKeyByID = `-- name: DeleteAPIKeyByID :exec +DELETE +FROM + api_keys +WHERE + id = $1 +` + +func (q *sqlQuerier) DeleteAPIKeyByID(ctx context.Context, id string) error { + _, err := q.db.ExecContext(ctx, deleteAPIKeyByID, id) + return err +} + const getAPIKeyByID = `-- name: GetAPIKeyByID :one SELECT id, hashed_secret, user_id, last_used, expires_at, created_at, updated_at, login_type, oauth_access_token, oauth_refresh_token, oauth_id_token, oauth_expiry diff --git a/coderd/database/queries/apikeys.sql b/coderd/database/queries/apikeys.sql index 1af2016f491bf..38ac145ce465e 100644 --- a/coderd/database/queries/apikeys.sql +++ b/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; diff --git a/coderd/users.go b/coderd/users.go index 0e0ce9bda22b8..78ebf246779fd 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -670,7 +670,13 @@ func (api *api) postAPIKey(rw http.ResponseWriter, r *http.Request) { } // Clear the user's session cookie -func (*api) postLogout(rw http.ResponseWriter, _ *http.Request) { +func (api *api) postLogout(rw http.ResponseWriter, r *http.Request) { + // Delete the session token from database + ok := api.deleteAPIKey(rw, r) + if !ok { + return + } + // Get a blank token cookie cookie := &http.Cookie{ // MaxAge < 0 means to delete the cookie now @@ -743,6 +749,18 @@ func (api *api) createAPIKey(rw http.ResponseWriter, r *http.Request, params dat return sessionToken, true } +func (api *api) deleteAPIKey(rw http.ResponseWriter, r *http.Request) bool { + apiKey := httpmw.APIKey(r) + err := api.Database.DeleteAPIKeyByID(r.Context(), apiKey.ID) + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ + Message: fmt.Sprintf("delete api key: %s", err.Error()), + }) + return false + } + return true +} + func (api *api) createUser(ctx context.Context, req codersdk.CreateUserRequest) (database.User, uuid.UUID, error) { var user database.User return user, req.OrganizationID, api.Database.InTx(func(db database.Store) error { From 77880f17c86605cdb6670d7f3720f76f0516b47c Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Thu, 26 May 2022 01:34:07 +0000 Subject: [PATCH 02/14] add deleteapikeybyid to databasefake --- coderd/database/databasefake/databasefake.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 0e16107a43858..4056c290400c0 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/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() From 094498ae8daa544859914eef34721f4bb4ec83b4 Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Thu, 26 May 2022 15:13:40 +0000 Subject: [PATCH 03/14] set blank cookie on logout always --- coderd/users.go | 47 ++++++++++++++++++++++++----------------------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/coderd/users.go b/coderd/users.go index 22db23d85df39..be4b7d2c20fdf 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -85,7 +85,7 @@ func (api *API) postFirstUser(rw http.ResponseWriter, r *http.Request) { // TODO: @emyrk this currently happens outside the database tx used to create // the user. Maybe I add this ability to grant roles in the createUser api // and add some rbac bypass when calling api functions this way?? - // Add the admin role to this first user + // Add the admin role to this first user. _, err = api.Database.UpdateUserRoles(r.Context(), database.UpdateUserRolesParams{ GrantedRoles: []string{rbac.RoleAdmin(), rbac.RoleMember()}, ID: user.ID, @@ -109,7 +109,7 @@ func (api *API) users(rw http.ResponseWriter, r *http.Request) { statusFilter = r.URL.Query().Get("status") ) - // Reading all users across the site + // Reading all users across the site. if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceUser) { return } @@ -162,7 +162,7 @@ func (api *API) users(rw http.ResponseWriter, r *http.Request) { // Creates a new user. func (api *API) postUser(rw http.ResponseWriter, r *http.Request) { - // Create the user on the site + // Create the user on the site. if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceUser) { return } @@ -408,11 +408,11 @@ func (api *API) userRoles(rw http.ResponseWriter, r *http.Request) { return } - // Only include ones we can read from RBAC + // Only include ones we can read from RBAC. memberships = AuthorizeFilter(api, r, rbac.ActionRead, memberships) for _, mem := range memberships { - // If we can read the org member, include the roles + // If we can read the org member, include the roles. if err == nil { resp.OrganizationRoles[mem.OrganizationID] = mem.Roles } @@ -422,7 +422,7 @@ func (api *API) userRoles(rw http.ResponseWriter, r *http.Request) { } func (api *API) putUserRoles(rw http.ResponseWriter, r *http.Request) { - // User is the user to modify + // User is the user to modify. user := httpmw.UserParam(r) roles := httpmw.UserRoles(r) @@ -470,7 +470,7 @@ func (api *API) putUserRoles(rw http.ResponseWriter, r *http.Request) { // updateSiteUserRoles will ensure only site wide roles are passed in as arguments. // If an organization role is included, an error is returned. func (api *API) updateSiteUserRoles(ctx context.Context, args database.UpdateUserRolesParams) (database.User, error) { - // Enforce only site wide roles + // Enforce only site wide roles. for _, r := range args.GrantedRoles { if _, ok := rbac.IsOrgRole(r); ok { return database.User{}, xerrors.Errorf("must only update site wide roles") @@ -504,7 +504,7 @@ func (api *API) organizationsByUser(rw http.ResponseWriter, r *http.Request) { return } - // Only return orgs the user can read + // Only return orgs the user can read. organizations = AuthorizeFilter(api, r, rbac.ActionRead, organizations) publicOrganizations := make([]codersdk.Organization, 0, len(organizations)) @@ -584,7 +584,7 @@ func (api *API) postOrganizationsByUser(rw http.ResponseWriter, r *http.Request) CreatedAt: database.Now(), UpdatedAt: database.Now(), Roles: []string{ - // Also assign member role incase they get demoted from admin + // Also assign member role incase they get demoted from admin. rbac.RoleOrgMember(organization.ID), rbac.RoleOrgAdmin(organization.ID), }, @@ -650,7 +650,7 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { }) } -// Creates a new session key, used for logging in via the CLI +// Creates a new session key, used for logging in via the CLI. func (api *API) postAPIKey(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) @@ -669,9 +669,19 @@ func (api *API) postAPIKey(rw http.ResponseWriter, r *http.Request) { httpapi.Write(rw, http.StatusCreated, codersdk.GenerateAPIKeyResponse{Key: sessionToken}) } -// Clear the user's session cookie +// Clear the user's session cookie. func (api *API) postLogout(rw http.ResponseWriter, r *http.Request) { - // Delete the session token from database + // Get a blank token cookie. + cookie := &http.Cookie{ + // MaxAge < 0 means to delete the cookie now. + MaxAge: -1, + Name: httpmw.SessionTokenKey, + Path: "/", + } + + http.SetCookie(rw, cookie) + + // Delete the session token from database. apiKey := httpmw.APIKey(r) err := api.Database.DeleteAPIKeyByID(r.Context(), apiKey.ID) if err != nil { @@ -681,15 +691,6 @@ func (api *API) postLogout(rw http.ResponseWriter, r *http.Request) { return } - // Get a blank token cookie - cookie := &http.Cookie{ - // MaxAge < 0 means to delete the cookie now - MaxAge: -1, - Name: httpmw.SessionTokenKey, - Path: "/", - } - - http.SetCookie(rw, cookie) httpapi.Write(rw, http.StatusOK, httpapi.Response{ Message: "Logged out!", }) @@ -771,7 +772,7 @@ func (api *API) createUser(ctx context.Context, req codersdk.CreateUserRequest) req.OrganizationID = organization.ID orgRoles = append(orgRoles, rbac.RoleOrgAdmin(req.OrganizationID)) } - // Always also be a member + // Always also be a member. orgRoles = append(orgRoles, rbac.RoleOrgMember(req.OrganizationID)) params := database.InsertUserParams{ @@ -817,7 +818,7 @@ func (api *API) createUser(ctx context.Context, req codersdk.CreateUserRequest) UserID: user.ID, CreatedAt: database.Now(), UpdatedAt: database.Now(), - // By default give them membership to the organization + // By default give them membership to the organization. Roles: orgRoles, }) if err != nil { From eb12beeef529754bf969f689969d223ad1dfa6f5 Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Thu, 26 May 2022 20:38:32 +0000 Subject: [PATCH 04/14] refactor logout flow, add unit tests --- cli/logout.go | 50 ++++++++++++++++---------------- cli/logout_test.go | 58 ++++++++++++++++++++++++++----------- coderd/coderd.go | 2 +- coderd/coderd_test.go | 17 ++++++++++- coderd/users_test.go | 67 ++++++++++++++++++++++++++++++++++++------- 5 files changed, 140 insertions(+), 54 deletions(-) diff --git a/cli/logout.go b/cli/logout.go index 317dfca51ee76..b0e2fa2b3b580 100644 --- 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 '.\n") return nil }, } diff --git a/cli/logout_test.go b/cli/logout_test.go index bae15ef204ca4..d7d0f7a8e046f 100644 --- 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 '.") <-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 '.") <-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 '.") }() - 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 '.") <-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 '.") + }() + + <-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 '.") <-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 index 3a4de3436e1ad..23989acbf26c9 100644 --- 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 index 61236c14bd048..0090c9613be57 100644 --- 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 index b1c3bddc2e89c..ed42eabf202b9 100644 --- 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") }) } From 8bb5713dc8f98f65738da665205a9208205edf1f Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Thu, 26 May 2022 20:41:36 +0000 Subject: [PATCH 05/14] update logout messsage --- cli/logout.go | 2 +- cli/logout_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cli/logout.go b/cli/logout.go index b0e2fa2b3b580..90b7f8f3d653c 100644 --- a/cli/logout.go +++ b/cli/logout.go @@ -67,7 +67,7 @@ func logout() *cobra.Command { 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 '.\n") + _, _ = fmt.Fprintf(cmd.OutOrStdout(), caret+"You are no longer logged in. You can log in using 'coder login '.\n") return nil }, } diff --git a/cli/logout_test.go b/cli/logout_test.go index d7d0f7a8e046f..462bce817c7d0 100644 --- a/cli/logout_test.go +++ b/cli/logout_test.go @@ -41,7 +41,7 @@ func TestLogout(t *testing.T) { pty.ExpectMatch("Are you sure you want to logout?") pty.WriteLine("yes") - pty.ExpectMatch("You are no longer logged in. Try logging in using 'coder login '.") + pty.ExpectMatch("You are no longer logged in. You can log in using 'coder login '.") <-logoutChan }) t.Run("SkipPrompt", func(t *testing.T) { @@ -67,7 +67,7 @@ func TestLogout(t *testing.T) { assert.NoFileExists(t, string(config.Session())) }() - pty.ExpectMatch("You are no longer logged in. Try logging in using 'coder login '.") + pty.ExpectMatch("You are no longer logged in. You can log in using 'coder login '.") <-logoutChan }) t.Run("NoURLFile", func(t *testing.T) { From fd56d39b2855282e8579fb5ba1d32be31185e479 Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Thu, 26 May 2022 21:00:10 +0000 Subject: [PATCH 06/14] use read-only file mode for windows --- cli/logout_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/cli/logout_test.go b/cli/logout_test.go index 462bce817c7d0..f571fb2068317 100644 --- a/cli/logout_test.go +++ b/cli/logout_test.go @@ -3,6 +3,7 @@ package cli_test import ( "os" "regexp" + "runtime" "testing" "github.com/stretchr/testify/assert" @@ -135,7 +136,12 @@ func TestLogout(t *testing.T) { require.FileExists(t, string(config.Session())) // Changing the permissions to throw error during deletion. - err := os.Chmod(string(config), 0500) + var err error + if runtime.GOOS == "windows" { + err = os.Chmod(string(config), 0400) + } else { + err = os.Chmod(string(config), 0500) + } require.NoError(t, err) logoutChan := make(chan struct{}) From 492db6a94a82c96074ed8e06fd7d7c993909c993 Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Thu, 26 May 2022 21:49:47 +0000 Subject: [PATCH 07/14] fix file mode on windows for cleanup --- cli/logout_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cli/logout_test.go b/cli/logout_test.go index f571fb2068317..0faf5a5239f79 100644 --- a/cli/logout_test.go +++ b/cli/logout_test.go @@ -162,7 +162,11 @@ func TestLogout(t *testing.T) { <-logoutChan // Setting the permissions back for cleanup. - err = os.Chmod(string(config), 0700) + if runtime.GOOS == "windows" { + err = os.Chmod(string(config), 0600) + } else { + err = os.Chmod(string(config), 0700) + } require.NoError(t, err) }) } From 2ad0fbc89168796a6ef2e518bfb7d245f1b0d03c Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Fri, 27 May 2022 14:28:50 +0000 Subject: [PATCH 08/14] change file permissions on windows --- cli/logout_test.go | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/cli/logout_test.go b/cli/logout_test.go index 0faf5a5239f79..2d9d61340a92d 100644 --- a/cli/logout_test.go +++ b/cli/logout_test.go @@ -138,11 +138,14 @@ func TestLogout(t *testing.T) { // Changing the permissions to throw error during deletion. var err error if runtime.GOOS == "windows" { - err = os.Chmod(string(config), 0400) + err = os.Chmod(string(config.URL()), 0400) + require.NoError(t, err) + err = os.Chmod(string(config.Session()), 0400) + require.NoError(t, err) } else { err = os.Chmod(string(config), 0500) + require.NoError(t, err) } - require.NoError(t, err) logoutChan := make(chan struct{}) logout, _ := clitest.New(t, "logout", "--global-config", string(config)) @@ -161,13 +164,18 @@ func TestLogout(t *testing.T) { pty.WriteLine("yes") <-logoutChan - // Setting the permissions back for cleanup. - if runtime.GOOS == "windows" { - err = os.Chmod(string(config), 0600) - } else { - err = os.Chmod(string(config), 0700) - } - require.NoError(t, err) + t.Cleanup(func() { + // Setting the permissions back for cleanup. + if runtime.GOOS == "windows" { + err = os.Chmod(string(config.URL()), 0600) + require.NoError(t, err) + err = os.Chmod(string(config.Session()), 0600) + require.NoError(t, err) + } else { + err = os.Chmod(string(config), 0700) + require.NoError(t, err) + } + }) }) } From 9a3731d2eb66e01d0f9b46a7b5acda796da8e835 Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Fri, 27 May 2022 15:36:36 +0000 Subject: [PATCH 09/14] assert error is not nil --- cli/logout_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cli/logout_test.go b/cli/logout_test.go index 2d9d61340a92d..83bb3accf4796 100644 --- a/cli/logout_test.go +++ b/cli/logout_test.go @@ -156,6 +156,7 @@ func TestLogout(t *testing.T) { go func() { defer close(logoutChan) err := logout.Execute() + assert.NotNil(t, err) 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()) }() From c38a0f50be23b368c3b70f203d53947015fe7d3a Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Fri, 27 May 2022 15:43:33 +0000 Subject: [PATCH 10/14] refactor cli --- cli/logout_test.go | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/cli/logout_test.go b/cli/logout_test.go index 83bb3accf4796..e1edcd1bce8da 100644 --- a/cli/logout_test.go +++ b/cli/logout_test.go @@ -146,6 +146,18 @@ func TestLogout(t *testing.T) { err = os.Chmod(string(config), 0500) require.NoError(t, err) } + t.Cleanup(func() { + // Setting the permissions back for cleanup. + if runtime.GOOS == "windows" { + err = os.Chmod(string(config.URL()), 0600) + require.NoError(t, err) + err = os.Chmod(string(config.Session()), 0600) + require.NoError(t, err) + } else { + err = os.Chmod(string(config), 0700) + require.NoError(t, err) + } + }) logoutChan := make(chan struct{}) logout, _ := clitest.New(t, "logout", "--global-config", string(config)) @@ -164,19 +176,6 @@ func TestLogout(t *testing.T) { pty.ExpectMatch("Are you sure you want to logout?") pty.WriteLine("yes") <-logoutChan - - t.Cleanup(func() { - // Setting the permissions back for cleanup. - if runtime.GOOS == "windows" { - err = os.Chmod(string(config.URL()), 0600) - require.NoError(t, err) - err = os.Chmod(string(config.Session()), 0600) - require.NoError(t, err) - } else { - err = os.Chmod(string(config), 0700) - require.NoError(t, err) - } - }) }) } From a19e0929379d3e4c48a9a30bc101b91455c2e183 Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Fri, 27 May 2022 16:03:41 +0000 Subject: [PATCH 11/14] try different file mode on windows --- 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 e1edcd1bce8da..c7c140de1a26c 100644 --- a/cli/logout_test.go +++ b/cli/logout_test.go @@ -138,9 +138,9 @@ func TestLogout(t *testing.T) { // Changing the permissions to throw error during deletion. var err error if runtime.GOOS == "windows" { - err = os.Chmod(string(config.URL()), 0400) + err = os.Chmod(string(config.URL()), 0200) require.NoError(t, err) - err = os.Chmod(string(config.Session()), 0400) + err = os.Chmod(string(config.Session()), 0200) require.NoError(t, err) } else { err = os.Chmod(string(config), 0500) From c82c661a192c74f2316c7029a4ada431944438b0 Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Fri, 27 May 2022 16:20:59 +0000 Subject: [PATCH 12/14] try different file mode on windows --- 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 c7c140de1a26c..c3bb77212c157 100644 --- a/cli/logout_test.go +++ b/cli/logout_test.go @@ -138,9 +138,9 @@ func TestLogout(t *testing.T) { // Changing the permissions to throw error during deletion. var err error if runtime.GOOS == "windows" { - err = os.Chmod(string(config.URL()), 0200) + err = os.Chmod(string(config.URL()), 0000) require.NoError(t, err) - err = os.Chmod(string(config.Session()), 0200) + err = os.Chmod(string(config.Session()), 0000) require.NoError(t, err) } else { err = os.Chmod(string(config), 0500) From 84ec351f16ec7e0248b56cbebd2a6e6c0f2b4a54 Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Fri, 27 May 2022 16:45:26 +0000 Subject: [PATCH 13/14] try keeping the files open on Windows --- cli/logout_test.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/cli/logout_test.go b/cli/logout_test.go index c3bb77212c157..0d9833a2b5f9d 100644 --- a/cli/logout_test.go +++ b/cli/logout_test.go @@ -136,11 +136,15 @@ func TestLogout(t *testing.T) { require.FileExists(t, string(config.Session())) // Changing the permissions to throw error during deletion. - var err error + var ( + err error + urlFile *os.File + sessionFile *os.File + ) if runtime.GOOS == "windows" { - err = os.Chmod(string(config.URL()), 0000) + urlFile, err = os.Open(string(config.URL())) require.NoError(t, err) - err = os.Chmod(string(config.Session()), 0000) + sessionFile, err = os.Open(string(config.Session())) require.NoError(t, err) } else { err = os.Chmod(string(config), 0500) @@ -149,9 +153,9 @@ func TestLogout(t *testing.T) { t.Cleanup(func() { // Setting the permissions back for cleanup. if runtime.GOOS == "windows" { - err = os.Chmod(string(config.URL()), 0600) + err = urlFile.Close() require.NoError(t, err) - err = os.Chmod(string(config.Session()), 0600) + err = sessionFile.Close() require.NoError(t, err) } else { err = os.Chmod(string(config), 0700) From 6707efabf918084c2452ffd242fb95ff48906b79 Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Fri, 27 May 2022 17:08:08 +0000 Subject: [PATCH 14/14] fix the error message on Windows --- cli/logout_test.go | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/cli/logout_test.go b/cli/logout_test.go index 0d9833a2b5f9d..753dcba234bfe 100644 --- a/cli/logout_test.go +++ b/cli/logout_test.go @@ -1,6 +1,7 @@ package cli_test import ( + "fmt" "os" "regexp" "runtime" @@ -125,7 +126,7 @@ func TestLogout(t *testing.T) { <-logoutChan }) - t.Run("ConfigDirectoryPermissionDenied", func(t *testing.T) { + t.Run("CannotDeleteFiles", func(t *testing.T) { t.Parallel() pty := ptytest.New(t) @@ -135,29 +136,31 @@ func TestLogout(t *testing.T) { require.FileExists(t, string(config.URL())) require.FileExists(t, string(config.Session())) - // Changing the permissions to throw error during deletion. 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() { - // Setting the permissions back for cleanup. 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) } @@ -173,7 +176,13 @@ func TestLogout(t *testing.T) { defer close(logoutChan) err := logout.Execute() assert.NotNil(t, err) - errRegex := regexp.MustCompile("Failed to log out.\n\tremove URL file: .+: permission denied\n\tremove session file: .+: permission denied") + 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()) }()