Skip to content

Commit f980768

Browse files
AbhineetJainkylecarbs
AbhineetJain
authored and
kylecarbs
committed
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
1 parent 6794a73 commit f980768

File tree

10 files changed

+239
-65
lines changed

10 files changed

+239
-65
lines changed

‎cli/logout.go

+30-22
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"
@@ -15,11 +16,16 @@ func logout() *cobra.Command {
1516
Use: "logout",
1617
Short: "Remove the local authenticated session",
1718
RunE: func(cmd *cobra.Command, args []string) error {
18-
var isLoggedOut bool
19+
client, err := createClient(cmd)
20+
if err != nil {
21+
return err
22+
}
23+
24+
var errors []error
1925

2026
config := createConfig(cmd)
2127

22-
_, err := cliui.Prompt(cmd, cliui.PromptOptions{
28+
_, err = cliui.Prompt(cmd, cliui.PromptOptions{
2329
Text: "Are you sure you want to logout?",
2430
IsConfirm: true,
2531
Default: "yes",
@@ -28,38 +34,40 @@ func logout() *cobra.Command {
2834
return err
2935
}
3036

31-
err = config.URL().Delete()
37+
err = client.Logout(cmd.Context())
3238
if err != nil {
33-
// Only throw error if the URL configuration file is present,
34-
// otherwise the user is already logged out, and we proceed
35-
if !os.IsNotExist(err) {
36-
return xerrors.Errorf("remove URL file: %w", err)
37-
}
38-
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))
3947
}
4048

4149
err = config.Session().Delete()
42-
if err != nil {
43-
// Only throw error if the session configuration file is present,
44-
// otherwise the user is already logged out, and we proceed
45-
if !os.IsNotExist(err) {
46-
return xerrors.Errorf("remove session file: %w", err)
47-
}
48-
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))
4954
}
5055

5156
err = config.Organization().Delete()
5257
// If the organization configuration file is absent, we still proceed
5358
if err != nil && !os.IsNotExist(err) {
54-
return xerrors.Errorf("remove organization file: %w", err)
59+
errors = append(errors, xerrors.Errorf("remove organization file: %w", err))
5560
}
5661

57-
// If the user was already logged out, we show them a different message
58-
if isLoggedOut {
59-
_, _ = fmt.Fprintf(cmd.OutOrStdout(), notLoggedInMessage+"\n")
60-
} else {
61-
_, _ = 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)
6269
}
70+
_, _ = fmt.Fprintf(cmd.OutOrStdout(), caret+"You are no longer logged in. You can log in using 'coder login <url>'.\n")
6371
return nil
6472
},
6573
}

‎cli/logout_test.go

+73-16
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
11
package cli_test
22

33
import (
4+
"fmt"
45
"os"
6+
"regexp"
7+
"runtime"
58
"testing"
69

710
"github.com/stretchr/testify/assert"
@@ -21,7 +24,7 @@ func TestLogout(t *testing.T) {
2124
pty := ptytest.New(t)
2225
config := login(t, pty)
2326

24-
// ensure session files exist
27+
// Ensure session files exist.
2528
require.FileExists(t, string(config.URL()))
2629
require.FileExists(t, string(config.Session()))
2730

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

4144
pty.ExpectMatch("Are you sure you want to logout?")
4245
pty.WriteLine("yes")
43-
pty.ExpectMatch("Successfully logged out")
46+
pty.ExpectMatch("You are no longer logged in. You can log in using 'coder login <url>'.")
4447
<-logoutChan
4548
})
4649
t.Run("SkipPrompt", func(t *testing.T) {
@@ -49,7 +52,7 @@ func TestLogout(t *testing.T) {
4952
pty := ptytest.New(t)
5053
config := login(t, pty)
5154

52-
// ensure session files exist
55+
// Ensure session files exist.
5356
require.FileExists(t, string(config.URL()))
5457
require.FileExists(t, string(config.Session()))
5558

@@ -66,7 +69,7 @@ func TestLogout(t *testing.T) {
6669
assert.NoFileExists(t, string(config.Session()))
6770
}()
6871

69-
pty.ExpectMatch("Successfully logged out")
72+
pty.ExpectMatch("You are no longer logged in. You can log in using 'coder login <url>'.")
7073
<-logoutChan
7174
})
7275
t.Run("NoURLFile", func(t *testing.T) {
@@ -75,7 +78,7 @@ func TestLogout(t *testing.T) {
7578
pty := ptytest.New(t)
7679
config := login(t, pty)
7780

78-
// ensure session files exist
81+
// Ensure session files exist.
7982
require.FileExists(t, string(config.URL()))
8083
require.FileExists(t, string(config.Session()))
8184

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

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>'.")
102100
<-logoutChan
103101
})
104102
t.Run("NoSessionFile", func(t *testing.T) {
@@ -107,7 +105,7 @@ func TestLogout(t *testing.T) {
107105
pty := ptytest.New(t)
108106
config := login(t, pty)
109107

110-
// ensure session files exist
108+
// Ensure session files exist.
111109
require.FileExists(t, string(config.URL()))
112110
require.FileExists(t, string(config.Session()))
113111

@@ -123,14 +121,73 @@ func TestLogout(t *testing.T) {
123121
go func() {
124122
defer close(logoutChan)
125123
err = logout.Execute()
126-
assert.NoError(t, err)
127-
assert.NoFileExists(t, string(config.URL()))
128-
assert.NoFileExists(t, string(config.Session()))
124+
assert.EqualError(t, err, "You are not logged in. Try logging in using 'coder login <url>'.")
125+
}()
126+
127+
<-logoutChan
128+
})
129+
t.Run("CannotDeleteFiles", func(t *testing.T) {
130+
t.Parallel()
131+
132+
pty := ptytest.New(t)
133+
config := login(t, pty)
134+
135+
// Ensure session files exist.
136+
require.FileExists(t, string(config.URL()))
137+
require.FileExists(t, string(config.Session()))
138+
139+
var (
140+
err error
141+
urlFile *os.File
142+
sessionFile *os.File
143+
)
144+
if runtime.GOOS == "windows" {
145+
// Opening the files so Windows does not allow deleting them.
146+
urlFile, err = os.Open(string(config.URL()))
147+
require.NoError(t, err)
148+
sessionFile, err = os.Open(string(config.Session()))
149+
require.NoError(t, err)
150+
} else {
151+
// Changing the permissions to throw error during deletion.
152+
err = os.Chmod(string(config), 0500)
153+
require.NoError(t, err)
154+
}
155+
t.Cleanup(func() {
156+
if runtime.GOOS == "windows" {
157+
// Closing the opened files for cleanup.
158+
err = urlFile.Close()
159+
require.NoError(t, err)
160+
err = sessionFile.Close()
161+
require.NoError(t, err)
162+
} else {
163+
// Setting the permissions back for cleanup.
164+
err = os.Chmod(string(config), 0700)
165+
require.NoError(t, err)
166+
}
167+
})
168+
169+
logoutChan := make(chan struct{})
170+
logout, _ := clitest.New(t, "logout", "--global-config", string(config))
171+
172+
logout.SetIn(pty.Input())
173+
logout.SetOut(pty.Output())
174+
175+
go func() {
176+
defer close(logoutChan)
177+
err := logout.Execute()
178+
assert.NotNil(t, err)
179+
var errorMessage string
180+
if runtime.GOOS == "windows" {
181+
errorMessage = "The process cannot access the file because it is being used by another process."
182+
} else {
183+
errorMessage = "permission denied"
184+
}
185+
errRegex := regexp.MustCompile(fmt.Sprintf("Failed to log out.\n\tremove URL file: .+: %s\n\tremove session file: .+: %s", errorMessage, errorMessage))
186+
assert.Regexp(t, errRegex, err.Error())
129187
}()
130188

131189
pty.ExpectMatch("Are you sure you want to logout?")
132190
pty.WriteLine("yes")
133-
pty.ExpectMatch("You are not logged in. Try logging in using 'coder login <url>'.")
134191
<-logoutChan
135192
})
136193
}

‎coderd/coderd.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,6 @@ func New(options *Options) *API {
219219
r.Get("/first", api.firstUser)
220220
r.Post("/first", api.postFirstUser)
221221
r.Post("/login", api.postLogin)
222-
r.Post("/logout", api.postLogout)
223222
r.Get("/authmethods", api.userAuthMethods)
224223
r.Route("/oauth2", func(r chi.Router) {
225224
r.Route("/github", func(r chi.Router) {
@@ -234,6 +233,7 @@ func New(options *Options) *API {
234233
)
235234
r.Post("/", api.postUser)
236235
r.Get("/", api.users)
236+
r.Post("/logout", api.postLogout)
237237
// These routes query information about site wide roles.
238238
r.Route("/roles", func(r chi.Router) {
239239
r.Get("/", api.assignableSiteRoles)

‎coderd/coderd_test.go

+16-1
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,10 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
104104
workspaceRBACObj := rbac.ResourceWorkspace.InOrg(organization.ID).WithID(workspace.ID.String()).WithOwner(workspace.OwnerID.String())
105105

106106
// skipRoutes allows skipping routes from being checked.
107+
skipRoutes := map[string]string{
108+
"POST:/api/v2/users/logout": "Logging out deletes the API Key for other routes",
109+
}
110+
107111
type routeCheck struct {
108112
NoAuthorize bool
109113
AssertAction rbac.Action
@@ -117,7 +121,6 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
117121
"GET:/api/v2/users/first": {NoAuthorize: true},
118122
"POST:/api/v2/users/first": {NoAuthorize: true},
119123
"POST:/api/v2/users/login": {NoAuthorize: true},
120-
"POST:/api/v2/users/logout": {NoAuthorize: true},
121124
"GET:/api/v2/users/authmethods": {NoAuthorize: true},
122125
"POST:/api/v2/csp/reports": {NoAuthorize: true},
123126

@@ -310,8 +313,20 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
310313
assertRoute[noTrailSlash] = v
311314
}
312315

316+
for k, v := range skipRoutes {
317+
noTrailSlash := strings.TrimRight(k, "/")
318+
if _, ok := skipRoutes[noTrailSlash]; ok && noTrailSlash != k {
319+
t.Errorf("route %q & %q is declared twice", noTrailSlash, k)
320+
t.FailNow()
321+
}
322+
skipRoutes[noTrailSlash] = v
323+
}
324+
313325
err = chi.Walk(api.Handler, func(method string, route string, handler http.Handler, middlewares ...func(http.Handler) http.Handler) error {
314326
name := method + ":" + route
327+
if _, ok := skipRoutes[strings.TrimRight(name, "/")]; ok {
328+
return nil
329+
}
315330
t.Run(name, func(t *testing.T) {
316331
authorizer.reset()
317332
routeAssertions, ok := assertRoute[strings.TrimRight(name, "/")]

‎coderd/database/databasefake/databasefake.go

+15
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,21 @@ func (q *fakeQuerier) GetAPIKeyByID(_ context.Context, id string) (database.APIK
126126
return database.APIKey{}, sql.ErrNoRows
127127
}
128128

129+
func (q *fakeQuerier) DeleteAPIKeyByID(_ context.Context, id string) error {
130+
q.mutex.Lock()
131+
defer q.mutex.Unlock()
132+
133+
for index, apiKey := range q.apiKeys {
134+
if apiKey.ID != id {
135+
continue
136+
}
137+
q.apiKeys[index] = q.apiKeys[len(q.apiKeys)-1]
138+
q.apiKeys = q.apiKeys[:len(q.apiKeys)-1]
139+
return nil
140+
}
141+
return sql.ErrNoRows
142+
}
143+
129144
func (q *fakeQuerier) GetFileByHash(_ context.Context, hash string) (database.File, error) {
130145
q.mutex.RLock()
131146
defer q.mutex.RUnlock()

‎coderd/database/querier.go

+1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries.sql.go

+13
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/apikeys.sql

+7
Original file line numberDiff line numberDiff line change
@@ -38,3 +38,10 @@ SET
3838
oauth_expiry = $6
3939
WHERE
4040
id = $1;
41+
42+
-- name: DeleteAPIKeyByID :exec
43+
DELETE
44+
FROM
45+
api_keys
46+
WHERE
47+
id = $1;

0 commit comments

Comments
 (0)