Skip to content

Commit eb12bee

Browse files
AbhineetJainAbhineetJain
committed
refactor logout flow, add unit tests
1 parent 094498a commit eb12bee

File tree

5 files changed

+140
-54
lines changed

5 files changed

+140
-54
lines changed

‎cli/logout.go

Lines changed: 24 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package cli
33
import (
44
"fmt"
55
"os"
6+
"strings"
67

78
"github.com/spf13/cobra"
89
"golang.org/x/xerrors"
@@ -20,7 +21,7 @@ func logout() *cobra.Command {
2021
return err
2122
}
2223

23-
var isLoggedOut bool
24+
var errors []error
2425

2526
config := createConfig(cmd)
2627

@@ -33,43 +34,40 @@ func logout() *cobra.Command {
3334
return err
3435
}
3536

36-
err = config.URL().Delete()
37+
err = client.Logout(cmd.Context())
3738
if err != nil {
38-
// Only throw error if the URL configuration file is present,
39-
// otherwise the user is already logged out, and we proceed
40-
if !os.IsNotExist(err) {
41-
return xerrors.Errorf("remove URL file: %w", err)
42-
}
43-
isLoggedOut = true
39+
errors = append(errors, xerrors.Errorf("logout api: %w", err))
40+
}
41+
42+
err = config.URL().Delete()
43+
// Only throw error if the URL configuration file is present,
44+
// otherwise the user is already logged out, and we proceed
45+
if err != nil && !os.IsNotExist(err) {
46+
errors = append(errors, xerrors.Errorf("remove URL file: %w", err))
4447
}
4548

4649
err = config.Session().Delete()
47-
if err != nil {
48-
// Only throw error if the session configuration file is present,
49-
// otherwise the user is already logged out, and we proceed
50-
if !os.IsNotExist(err) {
51-
return xerrors.Errorf("remove session file: %w", err)
52-
}
53-
isLoggedOut = true
50+
// Only throw error if the session configuration file is present,
51+
// otherwise the user is already logged out, and we proceed
52+
if err != nil && !os.IsNotExist(err) {
53+
errors = append(errors, xerrors.Errorf("remove session file: %w", err))
5454
}
5555

5656
err = config.Organization().Delete()
5757
// If the organization configuration file is absent, we still proceed
5858
if err != nil && !os.IsNotExist(err) {
59-
return xerrors.Errorf("remove organization file: %w", err)
59+
errors = append(errors, xerrors.Errorf("remove organization file: %w", err))
6060
}
6161

62-
err = client.Logout(cmd.Context())
63-
if err != nil {
64-
return xerrors.Errorf("logout: %w", err)
65-
}
66-
67-
// If the user was already logged out, we show them a different message
68-
if isLoggedOut {
69-
_, _ = fmt.Fprintf(cmd.OutOrStdout(), notLoggedInMessage+"\n")
70-
} else {
71-
_, _ = fmt.Fprintf(cmd.OutOrStdout(), caret+"Successfully logged out.\n")
62+
if len(errors) > 0 {
63+
var errorStringBuilder strings.Builder
64+
for _, err := range errors {
65+
_, _ = fmt.Fprint(&errorStringBuilder, "\t"+err.Error()+"\n")
66+
}
67+
errorString := strings.TrimRight(errorStringBuilder.String(), "\n")
68+
return xerrors.New("Failed to log out.\n" + errorString)
7269
}
70+
_, _ = fmt.Fprintf(cmd.OutOrStdout(), caret+"You are no longer logged in. Try logging in using 'coder login <url>'.\n")
7371
return nil
7472
},
7573
}

‎cli/logout_test.go

Lines changed: 42 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package cli_test
22

33
import (
44
"os"
5+
"regexp"
56
"testing"
67

78
"github.com/stretchr/testify/assert"
@@ -21,7 +22,7 @@ func TestLogout(t *testing.T) {
2122
pty := ptytest.New(t)
2223
config := login(t, pty)
2324

24-
// ensure session files exist
25+
// Ensure session files exist.
2526
require.FileExists(t, string(config.URL()))
2627
require.FileExists(t, string(config.Session()))
2728

@@ -40,7 +41,7 @@ func TestLogout(t *testing.T) {
4041

4142
pty.ExpectMatch("Are you sure you want to logout?")
4243
pty.WriteLine("yes")
43-
pty.ExpectMatch("Successfully logged out")
44+
pty.ExpectMatch("You are no longer logged in. Try logging in using 'coder login <url>'.")
4445
<-logoutChan
4546
})
4647
t.Run("SkipPrompt", func(t *testing.T) {
@@ -49,7 +50,7 @@ func TestLogout(t *testing.T) {
4950
pty := ptytest.New(t)
5051
config := login(t, pty)
5152

52-
// ensure session files exist
53+
// Ensure session files exist.
5354
require.FileExists(t, string(config.URL()))
5455
require.FileExists(t, string(config.Session()))
5556

@@ -66,7 +67,7 @@ func TestLogout(t *testing.T) {
6667
assert.NoFileExists(t, string(config.Session()))
6768
}()
6869

69-
pty.ExpectMatch("Successfully logged out")
70+
pty.ExpectMatch("You are no longer logged in. Try logging in using 'coder login <url>'.")
7071
<-logoutChan
7172
})
7273
t.Run("NoURLFile", func(t *testing.T) {
@@ -75,7 +76,7 @@ func TestLogout(t *testing.T) {
7576
pty := ptytest.New(t)
7677
config := login(t, pty)
7778

78-
// ensure session files exist
79+
// Ensure session files exist.
7980
require.FileExists(t, string(config.URL()))
8081
require.FileExists(t, string(config.Session()))
8182

@@ -91,14 +92,9 @@ func TestLogout(t *testing.T) {
9192
go func() {
9293
defer close(logoutChan)
9394
err := logout.Execute()
94-
assert.NoError(t, err)
95-
assert.NoFileExists(t, string(config.URL()))
96-
assert.NoFileExists(t, string(config.Session()))
95+
assert.EqualError(t, err, "You are not logged in. Try logging in using 'coder login <url>'.")
9796
}()
9897

99-
pty.ExpectMatch("Are you sure you want to logout?")
100-
pty.WriteLine("yes")
101-
pty.ExpectMatch("You are not logged in. Try logging in using 'coder login <url>'.")
10298
<-logoutChan
10399
})
104100
t.Run("NoSessionFile", func(t *testing.T) {
@@ -107,7 +103,7 @@ func TestLogout(t *testing.T) {
107103
pty := ptytest.New(t)
108104
config := login(t, pty)
109105

110-
// ensure session files exist
106+
// Ensure session files exist.
111107
require.FileExists(t, string(config.URL()))
112108
require.FileExists(t, string(config.Session()))
113109

@@ -123,15 +119,45 @@ func TestLogout(t *testing.T) {
123119
go func() {
124120
defer close(logoutChan)
125121
err = logout.Execute()
126-
assert.NoError(t, err)
127-
assert.NoFileExists(t, string(config.URL()))
128-
assert.NoFileExists(t, string(config.Session()))
122+
assert.EqualError(t, err, "You are not logged in. Try logging in using 'coder login <url>'.")
123+
}()
124+
125+
<-logoutChan
126+
})
127+
t.Run("ConfigDirectoryPermissionDenied", func(t *testing.T) {
128+
t.Parallel()
129+
130+
pty := ptytest.New(t)
131+
config := login(t, pty)
132+
133+
// Ensure session files exist.
134+
require.FileExists(t, string(config.URL()))
135+
require.FileExists(t, string(config.Session()))
136+
137+
// Changing the permissions to throw error during deletion.
138+
err := os.Chmod(string(config), 0500)
139+
require.NoError(t, err)
140+
141+
logoutChan := make(chan struct{})
142+
logout, _ := clitest.New(t, "logout", "--global-config", string(config))
143+
144+
logout.SetIn(pty.Input())
145+
logout.SetOut(pty.Output())
146+
147+
go func() {
148+
defer close(logoutChan)
149+
err := logout.Execute()
150+
errRegex := regexp.MustCompile("Failed to log out.\n\tremove URL file: .+: permission denied\n\tremove session file: .+: permission denied")
151+
assert.Regexp(t, errRegex, err.Error())
129152
}()
130153

131154
pty.ExpectMatch("Are you sure you want to logout?")
132155
pty.WriteLine("yes")
133-
pty.ExpectMatch("You are not logged in. Try logging in using 'coder login <url>'.")
134156
<-logoutChan
157+
158+
// Setting the permissions back for cleanup.
159+
err = os.Chmod(string(config), 0700)
160+
require.NoError(t, err)
135161
})
136162
}
137163

‎coderd/coderd.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,6 @@ func New(options *Options) *API {
206206
r.Get("/first", api.firstUser)
207207
r.Post("/first", api.postFirstUser)
208208
r.Post("/login", api.postLogin)
209-
r.Post("/logout", api.postLogout)
210209
r.Get("/authmethods", api.userAuthMethods)
211210
r.Route("/oauth2", func(r chi.Router) {
212211
r.Route("/github", func(r chi.Router) {
@@ -221,6 +220,7 @@ func New(options *Options) *API {
221220
)
222221
r.Post("/", api.postUser)
223222
r.Get("/", api.users)
223+
r.Post("/logout", api.postLogout)
224224
// These routes query information about site wide roles.
225225
r.Route("/roles", func(r chi.Router) {
226226
r.Get("/", api.assignableSiteRoles)

‎coderd/coderd_test.go

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,10 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
8383
workspaceRBACObj := rbac.ResourceWorkspace.InOrg(organization.ID).WithID(workspace.ID.String()).WithOwner(workspace.OwnerID.String())
8484

8585
// skipRoutes allows skipping routes from being checked.
86+
skipRoutes := map[string]string{
87+
"POST:/api/v2/users/logout": "Logging out deletes the API Key for other routes",
88+
}
89+
8690
type routeCheck struct {
8791
NoAuthorize bool
8892
AssertAction rbac.Action
@@ -96,7 +100,6 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
96100
"GET:/api/v2/users/first": {NoAuthorize: true},
97101
"POST:/api/v2/users/first": {NoAuthorize: true},
98102
"POST:/api/v2/users/login": {NoAuthorize: true},
99-
"POST:/api/v2/users/logout": {NoAuthorize: true},
100103
"GET:/api/v2/users/authmethods": {NoAuthorize: true},
101104

102105
// Has it's own auth
@@ -267,8 +270,20 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
267270
assertRoute[noTrailSlash] = v
268271
}
269272

273+
for k, v := range skipRoutes {
274+
noTrailSlash := strings.TrimRight(k, "/")
275+
if _, ok := skipRoutes[noTrailSlash]; ok && noTrailSlash != k {
276+
t.Errorf("route %q & %q is declared twice", noTrailSlash, k)
277+
t.FailNow()
278+
}
279+
skipRoutes[noTrailSlash] = v
280+
}
281+
270282
err = chi.Walk(api.Handler, func(method string, route string, handler http.Handler, middlewares ...func(http.Handler) http.Handler) error {
271283
name := method + ":" + route
284+
if _, ok := skipRoutes[strings.TrimRight(name, "/")]; ok {
285+
return nil
286+
}
272287
t.Run(name, func(t *testing.T) {
273288
authorizer.reset()
274289
routeAssertions, ok := assertRoute[strings.TrimRight(name, "/")]

‎coderd/users_test.go

Lines changed: 57 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,18 @@ package coderd_test
22

33
import (
44
"context"
5+
"database/sql"
56
"fmt"
67
"net/http"
78
"sort"
9+
"strings"
810
"testing"
911

1012
"github.com/google/uuid"
1113
"github.com/stretchr/testify/require"
1214

1315
"github.com/coder/coder/coderd/coderdtest"
16+
"github.com/coder/coder/coderd/database/databasefake"
1417
"github.com/coder/coder/coderd/httpmw"
1518
"github.com/coder/coder/coderd/rbac"
1619
"github.com/coder/coder/codersdk"
@@ -103,27 +106,71 @@ func TestPostLogin(t *testing.T) {
103106
func TestPostLogout(t *testing.T) {
104107
t.Parallel()
105108

106-
t.Run("ClearCookie", func(t *testing.T) {
109+
// Checks that the cookie is cleared and the API Key is deleted from the database.
110+
t.Run("Logout", func(t *testing.T) {
107111
t.Parallel()
108112

109-
client := coderdtest.New(t, nil)
113+
ctx := context.Background()
114+
client, api := coderdtest.NewWithAPI(t, nil)
115+
_ = coderdtest.CreateFirstUser(t, client)
116+
keyID := strings.Split(client.SessionToken, "-")[0]
117+
118+
apiKey, err := api.Database.GetAPIKeyByID(ctx, keyID)
119+
require.NoError(t, err)
120+
require.Equal(t, keyID, apiKey.ID, "API key should exist in the database")
121+
110122
fullURL, err := client.URL.Parse("/api/v2/users/logout")
111123
require.NoError(t, err, "Server URL should parse successfully")
112124

113-
req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, fullURL.String(), nil)
114-
require.NoError(t, err, "/logout request construction should succeed")
125+
res, err := client.Request(ctx, http.MethodPost, fullURL.String(), nil)
126+
require.NoError(t, err, "/logout request should succeed")
127+
res.Body.Close()
128+
require.Equal(t, http.StatusOK, res.StatusCode)
129+
130+
cookies := res.Cookies()
131+
require.Len(t, cookies, 1, "Exactly one cookie should be returned")
115132

116-
httpClient := &http.Client{}
133+
require.Equal(t, httpmw.SessionTokenKey, cookies[0].Name, "Cookie should be the auth cookie")
134+
require.Equal(t, -1, cookies[0].MaxAge, "Cookie should be set to delete")
117135

118-
response, err := httpClient.Do(req)
136+
apiKey, err = api.Database.GetAPIKeyByID(ctx, keyID)
137+
require.ErrorIs(t, err, sql.ErrNoRows, "API key should not exist in the database")
138+
})
139+
140+
t.Run("LogoutWithoutKey", func(t *testing.T) {
141+
t.Parallel()
142+
143+
ctx := context.Background()
144+
client, api := coderdtest.NewWithAPI(t, nil)
145+
_ = coderdtest.CreateFirstUser(t, client)
146+
keyID := strings.Split(client.SessionToken, "-")[0]
147+
148+
apiKey, err := api.Database.GetAPIKeyByID(ctx, keyID)
149+
require.NoError(t, err)
150+
require.Equal(t, keyID, apiKey.ID, "API key should exist in the database")
151+
152+
// Setting a fake database without the API Key to be used by the API.
153+
// The middleware that extracts the API key is already set to read
154+
// from the original database.
155+
dbWithoutKey := databasefake.New()
156+
api.Database = dbWithoutKey
157+
158+
fullURL, err := client.URL.Parse("/api/v2/users/logout")
159+
require.NoError(t, err, "Server URL should parse successfully")
160+
161+
res, err := client.Request(ctx, http.MethodPost, fullURL.String(), nil)
119162
require.NoError(t, err, "/logout request should succeed")
120-
response.Body.Close()
163+
res.Body.Close()
164+
require.Equal(t, http.StatusInternalServerError, res.StatusCode)
121165

122-
cookies := response.Cookies()
166+
cookies := res.Cookies()
123167
require.Len(t, cookies, 1, "Exactly one cookie should be returned")
124168

125-
require.Equal(t, cookies[0].Name, httpmw.SessionTokenKey, "Cookie should be the auth cookie")
126-
require.Equal(t, cookies[0].MaxAge, -1, "Cookie should be set to delete")
169+
require.Equal(t, httpmw.SessionTokenKey, cookies[0].Name, "Cookie should be the auth cookie")
170+
require.Equal(t, -1, cookies[0].MaxAge, "Cookie should be set to delete")
171+
172+
apiKey, err = api.Database.GetAPIKeyByID(ctx, keyID)
173+
require.ErrorIs(t, err, sql.ErrNoRows, "API key should not exist in the database")
127174
})
128175
}
129176

0 commit comments

Comments
 (0)