Skip to content

Commit 809dc7b

Browse files
Kira-PilotKira-Pilot
committed
audit oauth and write tests
1 parent e3a6587 commit 809dc7b

File tree

2 files changed

+98
-10
lines changed

2 files changed

+98
-10
lines changed

‎coderd/userauth.go

Lines changed: 61 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,6 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
240240
aReq.New = database.APIKey{UserID: uuid.New()}
241241
return
242242
}
243-
244243
aReq.New = key
245244

246245
http.SetCookie(rw, cookie)
@@ -276,16 +275,28 @@ type OIDCConfig struct {
276275
// @Router /users/oidc/callback [get]
277276
func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
278277
var (
279-
ctx = r.Context()
280-
state = httpmw.OAuth2(r)
278+
ctx = r.Context()
279+
state = httpmw.OAuth2(r)
280+
auditor = api.Auditor.Load()
281+
aReq, commitAudit = audit.InitRequest[database.APIKey](rw, &audit.RequestParams{
282+
Audit: *auditor,
283+
Log: api.Logger,
284+
Request: r,
285+
Action: database.AuditActionLogin,
286+
})
281287
)
288+
aReq.Old = database.APIKey{}
289+
defer commitAudit()
282290

283291
// See the example here: https://github.com/coreos/go-oidc
284292
rawIDToken, ok := state.Token.Extra("id_token").(string)
285293
if !ok {
286294
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
287295
Message: "id_token not found in response payload. Ensure your OIDC callback is configured correctly!",
288296
})
297+
// We pass a disposable user ID just to force an audit diff
298+
// and generate a log for a failed login
299+
aReq.New = database.APIKey{UserID: uuid.New()}
289300
return
290301
}
291302

@@ -295,6 +306,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
295306
Message: "Failed to verify OIDC token.",
296307
Detail: err.Error(),
297308
})
309+
// We pass a disposable user ID just to force an audit diff
310+
// and generate a log for a failed login
311+
aReq.New = database.APIKey{UserID: uuid.New()}
298312
return
299313
}
300314

@@ -308,6 +322,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
308322
Message: "Failed to extract OIDC claims.",
309323
Detail: err.Error(),
310324
})
325+
// We pass a disposable user ID just to force an audit diff
326+
// and generate a log for a failed login
327+
aReq.New = database.APIKey{UserID: uuid.New()}
311328
return
312329
}
313330

@@ -326,6 +343,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
326343
Message: "Failed to unmarshal user info claims.",
327344
Detail: err.Error(),
328345
})
346+
// We pass a disposable user ID just to force an audit diff
347+
// and generate a log for a failed login
348+
aReq.New = database.APIKey{UserID: uuid.New()}
329349
return
330350
}
331351
for k, v := range userInfoClaims {
@@ -336,6 +356,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
336356
Message: "Failed to obtain user information claims.",
337357
Detail: "The OIDC provider returned no claims as part of the `id_token`. The attempt to fetch claims via the UserInfo endpoint failed: " + err.Error(),
338358
})
359+
// We pass a disposable user ID just to force an audit diff
360+
// and generate a log for a failed login
361+
aReq.New = database.APIKey{UserID: uuid.New()}
339362
return
340363
}
341364

@@ -355,6 +378,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
355378
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
356379
Message: "No email found in OIDC payload!",
357380
})
381+
// We pass a disposable user ID just to force an audit diff
382+
// and generate a log for a failed login
383+
aReq.New = database.APIKey{UserID: uuid.New()}
358384
return
359385
}
360386
emailRaw = username
@@ -364,6 +390,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
364390
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
365391
Message: fmt.Sprintf("Email in OIDC payload isn't a string. Got: %t", emailRaw),
366392
})
393+
// We pass a disposable user ID just to force an audit diff
394+
// and generate a log for a failed login
395+
aReq.New = database.APIKey{UserID: uuid.New()}
367396
return
368397
}
369398
verifiedRaw, ok := claims["email_verified"]
@@ -374,6 +403,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
374403
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
375404
Message: fmt.Sprintf("Verify the %q email address on your OIDC provider to authenticate!", email),
376405
})
406+
// We pass a disposable user ID just to force an audit diff
407+
// and generate a log for a failed login
408+
aReq.New = database.APIKey{UserID: uuid.New()}
377409
return
378410
}
379411
api.Logger.Warn(ctx, "allowing unverified oidc email %q")
@@ -404,6 +436,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
404436
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
405437
Message: fmt.Sprintf("Your email %q is not in domains %q !", email, api.OIDCConfig.EmailDomain),
406438
})
439+
// We pass a disposable user ID just to force an audit diff
440+
// and generate a log for a failed login
441+
aReq.New = database.APIKey{UserID: uuid.New()}
407442
return
408443
}
409444
}
@@ -413,7 +448,22 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
413448
picture, _ = pictureRaw.(string)
414449
}
415450

416-
cookie, _, err := api.oauthLogin(r, oauthLoginParams{
451+
user, link, err := findLinkedUser(ctx, api.Database, oidcLinkedID(idToken), email)
452+
if err != nil {
453+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
454+
Message: "Failed to find linked user.",
455+
Detail: err.Error(),
456+
})
457+
// We pass a disposable user ID just to force an audit diff
458+
// and generate a log for a failed login
459+
aReq.New = database.APIKey{UserID: uuid.New()}
460+
return
461+
}
462+
aReq.UserID = user.ID
463+
464+
cookie, key, err := api.oauthLogin(r, oauthLoginParams{
465+
User: user,
466+
Link: link,
417467
State: state,
418468
LinkedID: oidcLinkedID(idToken),
419469
LoginType: database.LoginTypeOIDC,
@@ -428,15 +478,22 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
428478
Message: httpErr.msg,
429479
Detail: httpErr.detail,
430480
})
481+
// We pass a disposable user ID just to force an audit diff
482+
// and generate a log for a failed login
483+
aReq.New = database.APIKey{UserID: uuid.New()}
431484
return
432485
}
433486
if err != nil {
434487
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
435488
Message: "Failed to process OAuth login.",
436489
Detail: err.Error(),
437490
})
491+
// We pass a disposable user ID just to force an audit diff
492+
// and generate a log for a failed login
493+
aReq.New = database.APIKey{UserID: uuid.New()}
438494
return
439495
}
496+
aReq.New = key
440497

441498
http.SetCookie(rw, cookie)
442499

@@ -490,11 +547,6 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook
490547

491548
user = params.User
492549
link = params.Link
493-
//
494-
// user, link, err = findLinkedUser(ctx, tx, params.LinkedID, params.Email)
495-
// if err != nil {
496-
// return xerrors.Errorf("find linked user: %w", err)
497-
// }
498550

499551
if user.ID == uuid.Nil && !params.AllowSignups {
500552
return httpError{

‎coderd/userauth_test.go

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -718,6 +718,7 @@ func TestUserOIDC(t *testing.T) {
718718
tc := tc
719719
t.Run(tc.Name, func(t *testing.T) {
720720
t.Parallel()
721+
auditor := audit.NewMock()
721722
conf := coderdtest.NewOIDCConfig(t, "")
722723

723724
config := conf.OIDCConfig(t, tc.UserInfoClaims)
@@ -726,9 +727,13 @@ func TestUserOIDC(t *testing.T) {
726727
config.IgnoreEmailVerified = tc.IgnoreEmailVerified
727728

728729
client := coderdtest.New(t, &coderdtest.Options{
730+
Auditor: auditor,
729731
OIDCConfig: config,
730732
})
733+
numLogs := len(auditor.AuditLogs)
734+
731735
resp := oidcCallback(t, client, conf.EncodeClaims(t, tc.IDTokenClaims))
736+
numLogs++ // add an audit log for login
732737
assert.Equal(t, tc.StatusCode, resp.StatusCode)
733738

734739
ctx, _ := testutil.Context(t)
@@ -738,33 +743,43 @@ func TestUserOIDC(t *testing.T) {
738743
user, err := client.User(ctx, "me")
739744
require.NoError(t, err)
740745
require.Equal(t, tc.Username, user.Username)
746+
747+
require.Len(t, auditor.AuditLogs, numLogs)
748+
require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action)
741749
}
742750

743751
if tc.AvatarURL != "" {
744752
client.SetSessionToken(authCookieValue(resp.Cookies()))
745753
user, err := client.User(ctx, "me")
746754
require.NoError(t, err)
747755
require.Equal(t, tc.AvatarURL, user.AvatarURL)
756+
757+
require.Len(t, auditor.AuditLogs, numLogs)
758+
require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action)
748759
}
749760
})
750761
}
751762

752763
t.Run("AlternateUsername", func(t *testing.T) {
753764
t.Parallel()
754-
765+
auditor := audit.NewMock()
755766
conf := coderdtest.NewOIDCConfig(t, "")
756767

757768
config := conf.OIDCConfig(t, nil)
758769
config.AllowSignups = true
759770

760771
client := coderdtest.New(t, &coderdtest.Options{
772+
Auditor: auditor,
761773
OIDCConfig: config,
762774
})
775+
numLogs := len(auditor.AuditLogs)
763776

764777
code := conf.EncodeClaims(t, jwt.MapClaims{
765778
"email": "jon@coder.com",
766779
})
767780
resp := oidcCallback(t, client, code)
781+
numLogs++ // add an audit log for login
782+
768783
assert.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)
769784

770785
ctx, _ := testutil.Context(t)
@@ -781,12 +796,17 @@ func TestUserOIDC(t *testing.T) {
781796
"sub": "diff",
782797
})
783798
resp = oidcCallback(t, client, code)
799+
numLogs++ // add an audit log for login
800+
784801
assert.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)
785802

786803
client.SetSessionToken(authCookieValue(resp.Cookies()))
787804
user, err = client.User(ctx, "me")
788805
require.NoError(t, err)
789806
require.True(t, strings.HasPrefix(user.Username, "jon-"), "username %q should have prefix %q", user.Username, "jon-")
807+
808+
require.Len(t, auditor.AuditLogs, numLogs)
809+
require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action)
790810
})
791811

792812
t.Run("Disabled", func(t *testing.T) {
@@ -798,23 +818,33 @@ func TestUserOIDC(t *testing.T) {
798818

799819
t.Run("NoIDToken", func(t *testing.T) {
800820
t.Parallel()
821+
auditor := audit.NewMock()
801822
client := coderdtest.New(t, &coderdtest.Options{
823+
Auditor: auditor,
802824
OIDCConfig: &coderd.OIDCConfig{
803825
OAuth2Config: &oauth2Config{},
804826
},
805827
})
828+
numLogs := len(auditor.AuditLogs)
829+
806830
resp := oidcCallback(t, client, "asdf")
831+
numLogs++ // add an audit log for login
832+
807833
require.Equal(t, http.StatusBadRequest, resp.StatusCode)
834+
require.Len(t, auditor.AuditLogs, numLogs)
835+
require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action)
808836
})
809837

810838
t.Run("BadVerify", func(t *testing.T) {
811839
t.Parallel()
840+
auditor := audit.NewMock()
812841
verifier := oidc.NewVerifier("", &oidc.StaticKeySet{
813842
PublicKeys: []crypto.PublicKey{},
814843
}, &oidc.Config{})
815844
provider := &oidc.Provider{}
816845

817846
client := coderdtest.New(t, &coderdtest.Options{
847+
Auditor: auditor,
818848
OIDCConfig: &coderd.OIDCConfig{
819849
OAuth2Config: &oauth2Config{
820850
token: (&oauth2.Token{
@@ -827,8 +857,14 @@ func TestUserOIDC(t *testing.T) {
827857
Verifier: verifier,
828858
},
829859
})
860+
numLogs := len(auditor.AuditLogs)
861+
830862
resp := oidcCallback(t, client, "asdf")
863+
numLogs++ // add an audit log for login
864+
831865
require.Equal(t, http.StatusBadRequest, resp.StatusCode)
866+
require.Len(t, auditor.AuditLogs, numLogs)
867+
require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action)
832868
})
833869
}
834870

0 commit comments

Comments
 (0)