diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 71a7777b35dd9..c441f2574d5c3 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -5336,14 +5336,18 @@ const docTemplate = `{ "write", "delete", "start", - "stop" + "stop", + "login", + "logout" ], "x-enum-varnames": [ "AuditActionCreate", "AuditActionWrite", "AuditActionDelete", "AuditActionStart", - "AuditActionStop" + "AuditActionStop", + "AuditActionLogin", + "AuditActionLogout" ] }, "codersdk.AuditDiff": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index cb00f795253e7..36e5667e5f835 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -4718,13 +4718,15 @@ }, "codersdk.AuditAction": { "type": "string", - "enum": ["create", "write", "delete", "start", "stop"], + "enum": ["create", "write", "delete", "start", "stop", "login", "logout"], "x-enum-varnames": [ "AuditActionCreate", "AuditActionWrite", "AuditActionDelete", "AuditActionStart", - "AuditActionStop" + "AuditActionStop", + "AuditActionLogin", + "AuditActionLogout" ] }, "codersdk.AuditDiff": { diff --git a/coderd/apikey.go b/coderd/apikey.go index 5a8d7882eb7ad..1269aa232ed51 100644 --- a/coderd/apikey.go +++ b/coderd/apikey.go @@ -70,7 +70,7 @@ func (api *API) postToken(rw http.ResponseWriter, r *http.Request) { return } - cookie, err := api.createAPIKey(ctx, createAPIKeyParams{ + cookie, _, err := api.createAPIKey(ctx, createAPIKeyParams{ UserID: user.ID, LoginType: database.LoginTypeToken, ExpiresAt: database.Now().Add(lifeTime), @@ -108,7 +108,7 @@ func (api *API) postAPIKey(rw http.ResponseWriter, r *http.Request) { } lifeTime := time.Hour * 24 * 7 - cookie, err := api.createAPIKey(ctx, createAPIKeyParams{ + cookie, _, err := api.createAPIKey(ctx, createAPIKeyParams{ UserID: user.ID, LoginType: database.LoginTypePassword, RemoteAddr: r.RemoteAddr, @@ -281,10 +281,10 @@ func (api *API) validateAPIKeyLifetime(lifetime time.Duration) error { return nil } -func (api *API) createAPIKey(ctx context.Context, params createAPIKeyParams) (*http.Cookie, error) { +func (api *API) createAPIKey(ctx context.Context, params createAPIKeyParams) (*http.Cookie, *database.APIKey, error) { keyID, keySecret, err := GenerateAPIKeyIDSecret() if err != nil { - return nil, xerrors.Errorf("generate API key: %w", err) + return nil, nil, xerrors.Errorf("generate API key: %w", err) } hashed := sha256.Sum256([]byte(keySecret)) @@ -310,7 +310,7 @@ func (api *API) createAPIKey(ctx context.Context, params createAPIKeyParams) (*h switch scope { case database.APIKeyScopeAll, database.APIKeyScopeApplicationConnect: default: - return nil, xerrors.Errorf("invalid API key scope: %q", scope) + return nil, nil, xerrors.Errorf("invalid API key scope: %q", scope) } key, err := api.Database.InsertAPIKey(ctx, database.InsertAPIKeyParams{ @@ -333,7 +333,7 @@ func (api *API) createAPIKey(ctx context.Context, params createAPIKeyParams) (*h Scope: scope, }) if err != nil { - return nil, xerrors.Errorf("insert API key: %w", err) + return nil, nil, xerrors.Errorf("insert API key: %w", err) } api.Telemetry.Report(&telemetry.Snapshot{ @@ -349,5 +349,5 @@ func (api *API) createAPIKey(ctx context.Context, params createAPIKeyParams) (*h HttpOnly: true, SameSite: http.SameSiteLaxMode, Secure: api.SecureAuthCookie, - }, nil + }, &key, nil } diff --git a/coderd/audit.go b/coderd/audit.go index 4a642f3f56622..f26d7ba1104d8 100644 --- a/coderd/audit.go +++ b/coderd/audit.go @@ -264,6 +264,12 @@ func auditLogDescription(alog database.GetAuditLogsOffsetRow, additionalFields a codersdk.AuditAction(alog.Action).Friendly(), ) + // API Key resources do not have targets and follow the below format: + // "User {logged in | logged out}" + if alog.ResourceType == database.ResourceTypeApiKey { + return str + } + // Strings for starting/stopping workspace builds follow the below format: // "{user | 'Coder automatically'} started build #{build_number} for workspace {target}" // where target is a workspace (name) instead of a workspace build @@ -488,6 +494,10 @@ func actionFromString(actionString string) string { return actionString case codersdk.AuditActionStop: return actionString + case codersdk.AuditActionLogin: + return actionString + case codersdk.AuditActionLogout: + return actionString default: } return "" diff --git a/coderd/audit/request.go b/coderd/audit/request.go index 9127a66e446ea..4599c8376745f 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -29,8 +29,9 @@ type RequestParams struct { type Request[T Auditable] struct { params *RequestParams - Old T - New T + Old T + New T + UserID uuid.UUID } type BuildAuditParams[T Auditable] struct { @@ -64,6 +65,9 @@ func ResourceTarget[T Auditable](tgt T) string { return typed.PublicKey case database.AuditableGroup: return typed.Group.Name + case database.APIKey: + // this isn't used + return "" default: panic(fmt.Sprintf("unknown resource %T", tgt)) } @@ -85,6 +89,8 @@ func ResourceID[T Auditable](tgt T) uuid.UUID { return typed.UserID case database.AuditableGroup: return typed.Group.ID + case database.APIKey: + return typed.UserID default: panic(fmt.Sprintf("unknown resource %T", tgt)) } @@ -106,6 +112,8 @@ func ResourceType[T Auditable](tgt T) database.ResourceType { return database.ResourceTypeGitSshKey case database.AuditableGroup: return database.ResourceTypeGroup + case database.APIKey: + return database.ResourceTypeApiKey default: panic(fmt.Sprintf("unknown resource %T", tgt)) } @@ -150,11 +158,19 @@ func InitRequest[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request p.AdditionalFields = json.RawMessage("{}") } + var userID uuid.UUID + key, ok := httpmw.APIKeyOptional(p.Request) + if ok { + userID = key.UserID + } else { + userID = req.UserID + } + ip := parseIP(p.Request.RemoteAddr) auditLog := database.AuditLog{ ID: uuid.New(), Time: database.Now(), - UserID: httpmw.APIKey(p.Request).UserID, + UserID: userID, Ip: ip, UserAgent: sql.NullString{String: p.Request.UserAgent(), Valid: true}, ResourceType: either(req.Old, req.New, ResourceType[T]), diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 664f2648e2123..7a6d29b9c6001 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -16,7 +16,9 @@ CREATE TYPE audit_action AS ENUM ( 'write', 'delete', 'start', - 'stop' + 'stop', + 'login', + 'logout' ); CREATE TYPE build_reason AS ENUM ( diff --git a/coderd/database/migrations/000094_add_resource_type_api_key.down.sql b/coderd/database/migrations/000094_add_resource_type_api_key.down.sql new file mode 100644 index 0000000000000..d1d1637f4fa90 --- /dev/null +++ b/coderd/database/migrations/000094_add_resource_type_api_key.down.sql @@ -0,0 +1,2 @@ +-- It's not possible to drop enum values from enum types, so the UP has "IF NOT +-- EXISTS". diff --git a/coderd/database/migrations/000094_add_resource_type_api_key.up.sql b/coderd/database/migrations/000094_add_resource_type_api_key.up.sql new file mode 100644 index 0000000000000..15aac87d81a64 --- /dev/null +++ b/coderd/database/migrations/000094_add_resource_type_api_key.up.sql @@ -0,0 +1,9 @@ +ALTER TYPE audit_action + ADD VALUE IF NOT EXISTS 'login'; + +ALTER TYPE audit_action + ADD VALUE IF NOT EXISTS 'logout'; + +ALTER TYPE resource_type + ADD VALUE IF NOT EXISTS 'api_key'; + diff --git a/coderd/database/models.go b/coderd/database/models.go index 8496c75df2114..e0520428451a8 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -144,6 +144,8 @@ const ( AuditActionDelete AuditAction = "delete" AuditActionStart AuditAction = "start" AuditActionStop AuditAction = "stop" + AuditActionLogin AuditAction = "login" + AuditActionLogout AuditAction = "logout" ) func (e *AuditAction) Scan(src interface{}) error { @@ -187,7 +189,9 @@ func (e AuditAction) Valid() bool { AuditActionWrite, AuditActionDelete, AuditActionStart, - AuditActionStop: + AuditActionStop, + AuditActionLogin, + AuditActionLogout: return true } return false @@ -200,6 +204,8 @@ func AllAuditActionValues() []AuditAction { AuditActionDelete, AuditActionStart, AuditActionStop, + AuditActionLogin, + AuditActionLogout, } } diff --git a/coderd/gitsshkey_test.go b/coderd/gitsshkey_test.go index 7a0932c5ec62b..6b3e860d5c0e2 100644 --- a/coderd/gitsshkey_test.go +++ b/coderd/gitsshkey_test.go @@ -95,8 +95,8 @@ func TestGitSSHKey(t *testing.T) { require.NotEmpty(t, key2.PublicKey) require.NotEqual(t, key2.PublicKey, key1.PublicKey) - require.Len(t, auditor.AuditLogs, 1) - assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[0].Action) + require.Len(t, auditor.AuditLogs, 2) + assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[1].Action) }) } diff --git a/coderd/templates_test.go b/coderd/templates_test.go index 73a846fe35991..8336ff608d778 100644 --- a/coderd/templates_test.go +++ b/coderd/templates_test.go @@ -96,10 +96,11 @@ func TestPostTemplateByOrganization(t *testing.T) { assert.Equal(t, expected.Name, got.Name) assert.Equal(t, expected.Description, got.Description) - require.Len(t, auditor.AuditLogs, 3) - assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[0].Action) - assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[1].Action) - assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[2].Action) + require.Len(t, auditor.AuditLogs, 4) + assert.Equal(t, database.AuditActionLogin, auditor.AuditLogs[0].Action) + assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[1].Action) + assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[2].Action) + assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[3].Action) }) t.Run("AlreadyExists", func(t *testing.T) { @@ -320,8 +321,8 @@ func TestPatchTemplateMeta(t *testing.T) { assert.Equal(t, req.DefaultTTLMillis, updated.DefaultTTLMillis) assert.False(t, req.AllowUserCancelWorkspaceJobs) - require.Len(t, auditor.AuditLogs, 4) - assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[3].Action) + require.Len(t, auditor.AuditLogs, 5) + assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[4].Action) }) t.Run("NoMaxTTL", func(t *testing.T) { @@ -483,8 +484,8 @@ func TestDeleteTemplate(t *testing.T) { err := client.DeleteTemplate(ctx, template.ID) require.NoError(t, err) - require.Len(t, auditor.AuditLogs, 4) - assert.Equal(t, database.AuditActionDelete, auditor.AuditLogs[3].Action) + require.Len(t, auditor.AuditLogs, 5) + assert.Equal(t, database.AuditActionDelete, auditor.AuditLogs[4].Action) }) t.Run("Workspaces", func(t *testing.T) { diff --git a/coderd/templateversions_test.go b/coderd/templateversions_test.go index adf35aeb1c21b..a13a13f939d99 100644 --- a/coderd/templateversions_test.go +++ b/coderd/templateversions_test.go @@ -126,8 +126,8 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { require.Equal(t, "bananas", version.Name) require.Equal(t, provisionerdserver.ScopeOrganization, version.Job.Tags[provisionerdserver.TagScope]) - require.Len(t, auditor.AuditLogs, 1) - assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[0].Action) + require.Len(t, auditor.AuditLogs, 2) + assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[1].Action) }) t.Run("Example", func(t *testing.T) { t.Parallel() @@ -645,8 +645,8 @@ func TestPatchActiveTemplateVersion(t *testing.T) { }) require.NoError(t, err) - require.Len(t, auditor.AuditLogs, 4) - assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[3].Action) + require.Len(t, auditor.AuditLogs, 5) + assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[4].Action) }) } diff --git a/coderd/userauth.go b/coderd/userauth.go index 3fbc1c8f00bfa..fc9d1b5d4d6bb 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -17,6 +17,7 @@ import ( "golang.org/x/oauth2" "golang.org/x/xerrors" + "github.com/coder/coder/coderd/audit" "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/httpapi" "github.com/coder/coder/coderd/httpmw" @@ -66,9 +67,18 @@ func (api *API) userAuthMethods(rw http.ResponseWriter, r *http.Request) { // @Router /users/oauth2/github/callback [get] func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { var ( - ctx = r.Context() - state = httpmw.OAuth2(r) + ctx = r.Context() + state = httpmw.OAuth2(r) + auditor = api.Auditor.Load() + aReq, commitAudit = audit.InitRequest[database.APIKey](rw, &audit.RequestParams{ + Audit: *auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionLogin, + }) ) + aReq.Old = database.APIKey{} + defer commitAudit() oauthClient := oauth2.NewClient(ctx, oauth2.StaticTokenSource(state.Token)) @@ -81,6 +91,9 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { Message: "Internal error fetching authenticated Github user organizations.", Detail: err.Error(), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } @@ -101,6 +114,9 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{ Message: "You aren't a member of the authorized Github organizations!", }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } } @@ -111,6 +127,9 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { Message: "Internal error fetching authenticated Github user.", Detail: err.Error(), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } @@ -139,6 +158,9 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{ Message: fmt.Sprintf("You aren't a member of an authorized team in the %v Github organization(s)!", organizationNames), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } } @@ -149,6 +171,9 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { Message: "Internal error fetching personal Github user.", Detail: err.Error(), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } @@ -164,10 +189,28 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Your primary email must be verified on GitHub!", }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } - cookie, err := api.oauthLogin(r, oauthLoginParams{ + user, link, err := findLinkedUser(ctx, api.Database, githubLinkedID(ghUser), verifiedEmail.GetEmail()) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Failed to find linked user.", + Detail: err.Error(), + }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} + return + } + aReq.UserID = user.ID + + cookie, key, err := api.oauthLogin(r, oauthLoginParams{ + User: user, + Link: link, State: state, LinkedID: githubLinkedID(ghUser), LoginType: database.LoginTypeGithub, @@ -182,6 +225,9 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { Message: httpErr.msg, Detail: httpErr.detail, }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } if err != nil { @@ -189,8 +235,12 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { Message: "Failed to process OAuth login.", Detail: err.Error(), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } + aReq.New = key http.SetCookie(rw, cookie) @@ -225,9 +275,18 @@ type OIDCConfig struct { // @Router /users/oidc/callback [get] func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { var ( - ctx = r.Context() - state = httpmw.OAuth2(r) + ctx = r.Context() + state = httpmw.OAuth2(r) + auditor = api.Auditor.Load() + aReq, commitAudit = audit.InitRequest[database.APIKey](rw, &audit.RequestParams{ + Audit: *auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionLogin, + }) ) + aReq.Old = database.APIKey{} + defer commitAudit() // See the example here: https://github.com/coreos/go-oidc rawIDToken, ok := state.Token.Extra("id_token").(string) @@ -235,6 +294,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "id_token not found in response payload. Ensure your OIDC callback is configured correctly!", }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } @@ -244,6 +306,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { Message: "Failed to verify OIDC token.", Detail: err.Error(), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } @@ -257,6 +322,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { Message: "Failed to extract OIDC claims.", Detail: err.Error(), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } @@ -275,6 +343,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { Message: "Failed to unmarshal user info claims.", Detail: err.Error(), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } for k, v := range userInfoClaims { @@ -285,6 +356,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { Message: "Failed to obtain user information claims.", 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(), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } @@ -304,6 +378,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "No email found in OIDC payload!", }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } emailRaw = username @@ -313,6 +390,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: fmt.Sprintf("Email in OIDC payload isn't a string. Got: %t", emailRaw), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } verifiedRaw, ok := claims["email_verified"] @@ -323,6 +403,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ Message: fmt.Sprintf("Verify the %q email address on your OIDC provider to authenticate!", email), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } api.Logger.Warn(ctx, "allowing unverified oidc email %q") @@ -353,6 +436,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ Message: fmt.Sprintf("Your email %q is not in domains %q !", email, api.OIDCConfig.EmailDomain), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } } @@ -362,7 +448,22 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { picture, _ = pictureRaw.(string) } - cookie, err := api.oauthLogin(r, oauthLoginParams{ + user, link, err := findLinkedUser(ctx, api.Database, oidcLinkedID(idToken), email) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Failed to find linked user.", + Detail: err.Error(), + }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} + return + } + aReq.UserID = user.ID + + cookie, key, err := api.oauthLogin(r, oauthLoginParams{ + User: user, + Link: link, State: state, LinkedID: oidcLinkedID(idToken), LoginType: database.LoginTypeOIDC, @@ -377,6 +478,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { Message: httpErr.msg, Detail: httpErr.detail, }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } if err != nil { @@ -384,8 +488,12 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { Message: "Failed to process OAuth login.", Detail: err.Error(), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } + aReq.New = key http.SetCookie(rw, cookie) @@ -397,6 +505,8 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { } type oauthLoginParams struct { + User database.User + Link database.UserLink State httpmw.OAuth2State LinkedID string LoginType database.LoginType @@ -423,7 +533,7 @@ func (e httpError) Error() string { return e.msg } -func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cookie, error) { +func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cookie, database.APIKey, error) { var ( ctx = r.Context() user database.User @@ -435,10 +545,8 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook err error ) - user, link, err = findLinkedUser(ctx, tx, params.LinkedID, params.Email) - if err != nil { - return xerrors.Errorf("find linked user: %w", err) - } + user = params.User + link = params.Link if user.ID == uuid.Nil && !params.AllowSignups { return httpError{ @@ -599,19 +707,19 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook return nil }, nil) if err != nil { - return nil, xerrors.Errorf("in tx: %w", err) + return nil, database.APIKey{}, xerrors.Errorf("in tx: %w", err) } - cookie, err := api.createAPIKey(ctx, createAPIKeyParams{ + cookie, key, err := api.createAPIKey(ctx, createAPIKeyParams{ UserID: user.ID, LoginType: params.LoginType, RemoteAddr: r.RemoteAddr, }) if err != nil { - return nil, xerrors.Errorf("create API key: %w", err) + return nil, database.APIKey{}, xerrors.Errorf("create API key: %w", err) } - return cookie, nil + return cookie, *key, nil } // githubLinkedID returns the unique ID for a GitHub user. diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index ea707289639ec..ea994bd1dbc37 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -20,6 +20,7 @@ import ( "golang.org/x/xerrors" "github.com/coder/coder/coderd" + "github.com/coder/coder/coderd/audit" "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/coderd/database" "github.com/coder/coder/codersdk" @@ -105,7 +106,9 @@ func TestUserOAuth2Github(t *testing.T) { t.Run("NotInAllowedOrganization", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, GithubOAuth2Config: &coderd.GithubOAuth2Config{ OAuth2Config: &oauth2Config{}, ListOrganizationMemberships: func(ctx context.Context, client *http.Client) ([]*github.Membership, error) { @@ -118,13 +121,19 @@ func TestUserOAuth2Github(t *testing.T) { }, }, }) + numLogs := len(auditor.AuditLogs) resp := oauth2Callback(t, client) + numLogs++ // add an audit log for login require.Equal(t, http.StatusUnauthorized, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("NotInAllowedTeam", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, GithubOAuth2Config: &coderd.GithubOAuth2Config{ AllowOrganizations: []string{"coder"}, AllowTeams: []coderd.GithubOAuth2Team{{"another", "something"}, {"coder", "frontend"}}, @@ -147,12 +156,20 @@ func TestUserOAuth2Github(t *testing.T) { }, }, }) + numLogs := len(auditor.AuditLogs) + resp := oauth2Callback(t, client) + numLogs++ // add an audit log for login + require.Equal(t, http.StatusUnauthorized, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("UnverifiedEmail", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, GithubOAuth2Config: &coderd.GithubOAuth2Config{ OAuth2Config: &oauth2Config{}, AllowOrganizations: []string{"coder"}, @@ -175,13 +192,23 @@ func TestUserOAuth2Github(t *testing.T) { }, }, }) + numLogs := len(auditor.AuditLogs) + _ = coderdtest.CreateFirstUser(t, client) + numLogs++ // add an audit log for user create + resp := oauth2Callback(t, client) + numLogs++ // add an audit log for login + require.Equal(t, http.StatusBadRequest, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("BlockSignups", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, GithubOAuth2Config: &coderd.GithubOAuth2Config{ OAuth2Config: &oauth2Config{}, AllowOrganizations: []string{"coder"}, @@ -205,12 +232,20 @@ func TestUserOAuth2Github(t *testing.T) { }, }, }) + numLogs := len(auditor.AuditLogs) + resp := oauth2Callback(t, client) + numLogs++ // add an audit log for login + require.Equal(t, http.StatusForbidden, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("MultiLoginNotAllowed", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, GithubOAuth2Config: &coderd.GithubOAuth2Config{ OAuth2Config: &oauth2Config{}, AllowOrganizations: []string{"coder"}, @@ -234,16 +269,26 @@ func TestUserOAuth2Github(t *testing.T) { }, }, }) + numLogs := len(auditor.AuditLogs) + // Creates the first user with login_type 'password'. _ = coderdtest.CreateFirstUser(t, client) + numLogs++ // add an audit log for user create + // Attempting to login should give us a 403 since the user // already has a login_type of 'password'. resp := oauth2Callback(t, client) + numLogs++ // add an audit log for login + require.Equal(t, http.StatusForbidden, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("Signup", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, GithubOAuth2Config: &coderd.GithubOAuth2Config{ OAuth2Config: &oauth2Config{}, AllowOrganizations: []string{"coder"}, @@ -272,7 +317,11 @@ func TestUserOAuth2Github(t *testing.T) { }, }, }) + numLogs := len(auditor.AuditLogs) + resp := oauth2Callback(t, client) + numLogs++ // add an audit log for login + require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) client.SetSessionToken(authCookieValue(resp.Cookies())) @@ -281,10 +330,15 @@ func TestUserOAuth2Github(t *testing.T) { require.Equal(t, "kyle@coder.com", user.Email) require.Equal(t, "kyle", user.Username) require.Equal(t, "/hello-world", user.AvatarURL) + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("SignupAllowedTeam", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, GithubOAuth2Config: &coderd.GithubOAuth2Config{ AllowSignups: true, AllowOrganizations: []string{"coder"}, @@ -315,12 +369,20 @@ func TestUserOAuth2Github(t *testing.T) { }, }, }) + numLogs := len(auditor.AuditLogs) + resp := oauth2Callback(t, client) + numLogs++ // add an audit log for login + require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("SignupAllowedTeamInFirstOrganization", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, GithubOAuth2Config: &coderd.GithubOAuth2Config{ AllowSignups: true, AllowOrganizations: []string{"coder", "nil"}, @@ -359,12 +421,20 @@ func TestUserOAuth2Github(t *testing.T) { }, }, }) + numLogs := len(auditor.AuditLogs) + resp := oauth2Callback(t, client) + numLogs++ // add an audit log for login + require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("SignupAllowedTeamInSecondOrganization", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, GithubOAuth2Config: &coderd.GithubOAuth2Config{ AllowSignups: true, AllowOrganizations: []string{"coder", "nil"}, @@ -403,12 +473,20 @@ func TestUserOAuth2Github(t *testing.T) { }, }, }) + numLogs := len(auditor.AuditLogs) + resp := oauth2Callback(t, client) + numLogs++ // add an audit log for login + require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("SignupAllowEveryone", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, GithubOAuth2Config: &coderd.GithubOAuth2Config{ AllowSignups: true, AllowEveryone: true, @@ -433,12 +511,20 @@ func TestUserOAuth2Github(t *testing.T) { }, }, }) + numLogs := len(auditor.AuditLogs) + resp := oauth2Callback(t, client) + numLogs++ // add an audit log for login + require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("SignupFailedInactiveInOrg", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, GithubOAuth2Config: &coderd.GithubOAuth2Config{ AllowSignups: true, AllowOrganizations: []string{"coder"}, @@ -469,8 +555,14 @@ func TestUserOAuth2Github(t *testing.T) { }, }, }) + numLogs := len(auditor.AuditLogs) + resp := oauth2Callback(t, client) + numLogs++ // add an audit log for login + require.Equal(t, http.StatusUnauthorized, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) } @@ -626,6 +718,7 @@ func TestUserOIDC(t *testing.T) { tc := tc t.Run(tc.Name, func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() conf := coderdtest.NewOIDCConfig(t, "") config := conf.OIDCConfig(t, tc.UserInfoClaims) @@ -634,9 +727,13 @@ func TestUserOIDC(t *testing.T) { config.IgnoreEmailVerified = tc.IgnoreEmailVerified client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, OIDCConfig: config, }) + numLogs := len(auditor.AuditLogs) + resp := oidcCallback(t, client, conf.EncodeClaims(t, tc.IDTokenClaims)) + numLogs++ // add an audit log for login assert.Equal(t, tc.StatusCode, resp.StatusCode) ctx, _ := testutil.Context(t) @@ -646,6 +743,9 @@ func TestUserOIDC(t *testing.T) { user, err := client.User(ctx, "me") require.NoError(t, err) require.Equal(t, tc.Username, user.Username) + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) } if tc.AvatarURL != "" { @@ -653,26 +753,33 @@ func TestUserOIDC(t *testing.T) { user, err := client.User(ctx, "me") require.NoError(t, err) require.Equal(t, tc.AvatarURL, user.AvatarURL) + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) } }) } t.Run("AlternateUsername", func(t *testing.T) { t.Parallel() - + auditor := audit.NewMock() conf := coderdtest.NewOIDCConfig(t, "") config := conf.OIDCConfig(t, nil) config.AllowSignups = true client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, OIDCConfig: config, }) + numLogs := len(auditor.AuditLogs) code := conf.EncodeClaims(t, jwt.MapClaims{ "email": "jon@coder.com", }) resp := oidcCallback(t, client, code) + numLogs++ // add an audit log for login + assert.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) ctx, _ := testutil.Context(t) @@ -689,12 +796,17 @@ func TestUserOIDC(t *testing.T) { "sub": "diff", }) resp = oidcCallback(t, client, code) + numLogs++ // add an audit log for login + assert.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) client.SetSessionToken(authCookieValue(resp.Cookies())) user, err = client.User(ctx, "me") require.NoError(t, err) require.True(t, strings.HasPrefix(user.Username, "jon-"), "username %q should have prefix %q", user.Username, "jon-") + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("Disabled", func(t *testing.T) { @@ -706,23 +818,33 @@ func TestUserOIDC(t *testing.T) { t.Run("NoIDToken", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, OIDCConfig: &coderd.OIDCConfig{ OAuth2Config: &oauth2Config{}, }, }) + numLogs := len(auditor.AuditLogs) + resp := oidcCallback(t, client, "asdf") + numLogs++ // add an audit log for login + require.Equal(t, http.StatusBadRequest, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("BadVerify", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() verifier := oidc.NewVerifier("", &oidc.StaticKeySet{ PublicKeys: []crypto.PublicKey{}, }, &oidc.Config{}) provider := &oidc.Provider{} client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, OIDCConfig: &coderd.OIDCConfig{ OAuth2Config: &oauth2Config{ token: (&oauth2.Token{ @@ -735,8 +857,14 @@ func TestUserOIDC(t *testing.T) { Verifier: verifier, }, }) + numLogs := len(auditor.AuditLogs) + resp := oidcCallback(t, client, "asdf") + numLogs++ // add an audit log for login + require.Equal(t, http.StatusBadRequest, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) } diff --git a/coderd/users.go b/coderd/users.go index 6c237f421c2d8..2508049a53d10 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -995,9 +995,24 @@ func (api *API) organizationByUserAndName(rw http.ResponseWriter, r *http.Reques // @Success 201 {object} codersdk.LoginWithPasswordResponse // @Router /users/login [post] func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { - ctx := r.Context() + var ( + ctx = r.Context() + auditor = api.Auditor.Load() + aReq, commitAudit = audit.InitRequest[database.APIKey](rw, &audit.RequestParams{ + Audit: *auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionLogin, + }) + ) + aReq.Old = database.APIKey{} + defer commitAudit() + var loginWithPassword codersdk.LoginWithPasswordRequest if !httpapi.Read(ctx, rw, r, &loginWithPassword) { + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } @@ -1008,15 +1023,23 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error.", }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } + aReq.UserID = user.ID + // If the user doesn't exist, it will be a default struct. equal, err := userpassword.Compare(string(user.HashedPassword), loginWithPassword.Password) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error.", }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } if !equal { @@ -1025,6 +1048,9 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{ Message: "Incorrect email or password.", }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } @@ -1032,6 +1058,9 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ Message: fmt.Sprintf("Incorrect login type, attempting to use %q but user is of login type %q", database.LoginTypePassword, user.LoginType), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } @@ -1040,10 +1069,13 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{ Message: "Your account is suspended. Contact an admin to reactivate your account.", }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } - cookie, err := api.createAPIKey(ctx, createAPIKeyParams{ + cookie, key, err := api.createAPIKey(ctx, createAPIKeyParams{ UserID: user.ID, LoginType: database.LoginTypePassword, RemoteAddr: r.RemoteAddr, @@ -1053,9 +1085,14 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { Message: "Failed to create API key.", Detail: err.Error(), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } + aReq.New = *key + http.SetCookie(rw, cookie) httpapi.Write(ctx, rw, http.StatusCreated, codersdk.LoginWithPasswordResponse{ @@ -1073,7 +1110,18 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { // @Success 200 {object} codersdk.Response // @Router /users/logout [post] func (api *API) postLogout(rw http.ResponseWriter, r *http.Request) { - ctx := r.Context() + var ( + ctx = r.Context() + auditor = api.Auditor.Load() + aReq, commitAudit = audit.InitRequest[database.APIKey](rw, &audit.RequestParams{ + Audit: *auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionLogout, + }) + ) + defer commitAudit() + // Get a blank token cookie. cookie := &http.Cookie{ // MaxAge < 0 means to delete the cookie now. @@ -1085,6 +1133,8 @@ func (api *API) postLogout(rw http.ResponseWriter, r *http.Request) { // Delete the session token from database. apiKey := httpmw.APIKey(r) + aReq.Old = apiKey + err := api.Database.DeleteAPIKeyByID(ctx, apiKey.ID) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ @@ -1138,6 +1188,8 @@ func (api *API) postLogout(rw http.ResponseWriter, r *http.Request) { } } + aReq.New = database.APIKey{} + httpapi.Write(ctx, rw, http.StatusOK, codersdk.Response{ Message: "Logged out!", }) diff --git a/coderd/users_test.go b/coderd/users_test.go index 7e6932073b5bd..7bebba908e0ce 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -10,7 +10,6 @@ import ( "time" "github.com/google/uuid" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/sync/errgroup" @@ -121,7 +120,9 @@ func TestPostLogin(t *testing.T) { t.Parallel() t.Run("InvalidUser", func(t *testing.T) { t.Parallel() - client := coderdtest.New(t, nil) + auditor := audit.NewMock() + client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + numLogs := len(auditor.AuditLogs) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -130,14 +131,20 @@ func TestPostLogin(t *testing.T) { Email: "my@email.org", Password: "password", }) + numLogs++ // add an audit log for login var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode()) + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("BadPassword", func(t *testing.T) { t.Parallel() - client := coderdtest.New(t, nil) + auditor := audit.NewMock() + client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + numLogs := len(auditor.AuditLogs) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -153,17 +160,26 @@ func TestPostLogin(t *testing.T) { Email: req.Email, Password: "badpass", }) + numLogs++ // add an audit log for login var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode()) + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("Suspended", func(t *testing.T) { t.Parallel() - client := coderdtest.New(t, nil) + auditor := audit.NewMock() + client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + numLogs := len(auditor.AuditLogs) first := coderdtest.CreateFirstUser(t, client) + numLogs++ // add an audit log for create user + numLogs++ // add an audit log for login member := coderdtest.CreateAnotherUser(t, client, first.OrganizationID) + numLogs++ // add an audit log for create user ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -173,6 +189,7 @@ func TestPostLogin(t *testing.T) { _, err = client.UpdateUserStatus(ctx, memberUser.Username, codersdk.UserStatusSuspended) require.NoError(t, err, "suspend member") + numLogs++ // add an audit log for update user // Test an existing session _, err = member.User(ctx, codersdk.Me) @@ -186,14 +203,20 @@ func TestPostLogin(t *testing.T) { Email: memberUser.Email, Password: "testpass", }) + numLogs++ // add an audit log for login require.ErrorAs(t, err, &apiErr) require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode()) require.Contains(t, apiErr.Message, "suspended") + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("Success", func(t *testing.T) { t.Parallel() - client := coderdtest.New(t, nil) + auditor := audit.NewMock() + client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + numLogs := len(auditor.AuditLogs) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -205,6 +228,8 @@ func TestPostLogin(t *testing.T) { } _, err := client.CreateFirstUser(ctx, req) require.NoError(t, err) + numLogs++ // add an audit log for create user + numLogs++ // add an audit log for login _, err = client.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{ Email: req.Email, @@ -218,6 +243,9 @@ func TestPostLogin(t *testing.T) { Password: req.Password, }) require.NoError(t, err) + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("Lifetime&Expire", func(t *testing.T) { @@ -299,9 +327,12 @@ func TestPostLogout(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() + auditor := audit.NewMock() + client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + numLogs := len(auditor.AuditLogs) - client := coderdtest.New(t, nil) admin := coderdtest.CreateFirstUser(t, client) + numLogs++ // add an audit log for login ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -315,10 +346,15 @@ func TestPostLogout(t *testing.T) { require.NoError(t, err, "Server URL should parse successfully") res, err := client.Request(ctx, http.MethodPost, fullURL.String(), nil) + numLogs++ // add an audit log for logout + require.NoError(t, err, "/logout request should succeed") res.Body.Close() require.Equal(t, http.StatusOK, res.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogout, auditor.AuditLogs[numLogs-1].Action) + cookies := res.Cookies() var found bool @@ -421,7 +457,11 @@ func TestPostUsers(t *testing.T) { t.Parallel() auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + numLogs := len(auditor.AuditLogs) + user := coderdtest.CreateFirstUser(t, client) + numLogs++ // add an audit log for user create + numLogs++ // add an audit log for login ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -434,8 +474,9 @@ func TestPostUsers(t *testing.T) { }) require.NoError(t, err) - require.Len(t, auditor.AuditLogs, 1) - assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[0].Action) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionCreate, auditor.AuditLogs[numLogs-1].Action) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-2].Action) }) } @@ -486,7 +527,10 @@ func TestUpdateUserProfile(t *testing.T) { t.Parallel() auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + numLogs := len(auditor.AuditLogs) + coderdtest.CreateFirstUser(t, client) + numLogs++ // add an audit log for login ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -497,8 +541,10 @@ func TestUpdateUserProfile(t *testing.T) { }) require.NoError(t, err) require.Equal(t, userProfile.Username, "newusername") - assert.Len(t, auditor.AuditLogs, 1) - assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[0].Action) + numLogs++ // add an audit log for user update + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionWrite, auditor.AuditLogs[numLogs-1].Action) }) } @@ -550,8 +596,14 @@ func TestUpdateUserPassword(t *testing.T) { t.Parallel() auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + numLogs := len(auditor.AuditLogs) + admin := coderdtest.CreateFirstUser(t, client) + numLogs++ // add an audit log for user create + numLogs++ // add an audit log for login + member := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID) + numLogs++ // add an audit log for user create ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -560,9 +612,12 @@ func TestUpdateUserPassword(t *testing.T) { OldPassword: "testpass", Password: "newpassword", }) + numLogs++ // add an audit log for user update + require.NoError(t, err, "member should be able to update own password") - assert.Len(t, auditor.AuditLogs, 2) - assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[1].Action) + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionWrite, auditor.AuditLogs[numLogs-1].Action) }) t.Run("MemberCantUpdateOwnPasswordWithoutOldPassword", func(t *testing.T) { t.Parallel() @@ -582,7 +637,10 @@ func TestUpdateUserPassword(t *testing.T) { t.Parallel() auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + numLogs := len(auditor.AuditLogs) + _ = coderdtest.CreateFirstUser(t, client) + numLogs++ // add an audit log for login ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -590,9 +648,12 @@ func TestUpdateUserPassword(t *testing.T) { err := client.UpdateUserPassword(ctx, "me", codersdk.UpdateUserPasswordRequest{ Password: "newpassword", }) + numLogs++ // add an audit log for user update + require.NoError(t, err, "admin should be able to update own password without providing old password") - assert.Len(t, auditor.AuditLogs, 1) - assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[0].Action) + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionWrite, auditor.AuditLogs[numLogs-1].Action) }) t.Run("ChangingPasswordDeletesKeys", func(t *testing.T) { @@ -864,8 +925,14 @@ func TestPutUserSuspend(t *testing.T) { t.Parallel() auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + numLogs := len(auditor.AuditLogs) + me := coderdtest.CreateFirstUser(t, client) + numLogs++ // add an audit log for user create + numLogs++ // add an audit log for login + _, user := coderdtest.CreateAnotherUserWithUser(t, client, me.OrganizationID) + numLogs++ // add an audit log for user create ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -873,8 +940,10 @@ func TestPutUserSuspend(t *testing.T) { user, err := client.UpdateUserStatus(ctx, user.Username, codersdk.UserStatusSuspended) require.NoError(t, err) require.Equal(t, user.Status, codersdk.UserStatusSuspended) - assert.Len(t, auditor.AuditLogs, 2) - assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[1].Action) + numLogs++ // add an audit log for user update + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionWrite, auditor.AuditLogs[numLogs-1].Action) }) t.Run("SuspendItSelf", func(t *testing.T) { diff --git a/coderd/workspaceapps.go b/coderd/workspaceapps.go index 168345bc2d27e..8c11277a3d192 100644 --- a/coderd/workspaceapps.go +++ b/coderd/workspaceapps.go @@ -745,7 +745,7 @@ func (api *API) workspaceApplicationAuth(rw http.ResponseWriter, r *http.Request if lifetime > int64((time.Hour * 24 * 7).Seconds()) { lifetime = int64((time.Hour * 24 * 7).Seconds()) } - cookie, err := api.createAPIKey(ctx, createAPIKeyParams{ + cookie, _, err := api.createAPIKey(ctx, createAPIKeyParams{ UserID: apiKey.UserID, LoginType: database.LoginTypePassword, ExpiresAt: exp, diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index 7bb8e0a761262..f1c103be6ff0d 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -578,6 +578,7 @@ func TestWorkspaceBuildStatus(t *testing.T) { numLogs := len(auditor.AuditLogs) client, closeDaemon, api := coderdtest.NewWithAPI(t, &coderdtest.Options{IncludeProvisionerDaemon: true, Auditor: auditor}) user := coderdtest.CreateFirstUser(t, client) + numLogs++ // add an audit log for login version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) numLogs++ // add an audit log for template version creation numLogs++ // add an audit log for template version update diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 52784feaa16c4..54161c5fcce08 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -270,8 +270,8 @@ func TestPostWorkspacesByOrganization(t *testing.T) { coderdtest.AwaitTemplateVersionJob(t, client, version.ID) _ = coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) - require.Len(t, auditor.AuditLogs, 4) - assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[3].Action) + require.Len(t, auditor.AuditLogs, 5) + assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[4].Action) }) t.Run("CreateWithDeletedTemplate", func(t *testing.T) { @@ -1283,8 +1283,8 @@ func TestWorkspaceUpdateAutostart(t *testing.T) { interval := next.Sub(testCase.at) require.Equal(t, testCase.expectedInterval, interval, "unexpected interval") - require.Len(t, auditor.AuditLogs, 6) - assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[5].Action) + require.Len(t, auditor.AuditLogs, 7) + assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[6].Action) }) } @@ -1398,8 +1398,8 @@ func TestWorkspaceUpdateTTL(t *testing.T) { require.Equal(t, testCase.ttlMillis, updated.TTLMillis, "expected autostop ttl to equal requested") - require.Len(t, auditor.AuditLogs, 6) - assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[5].Action) + require.Len(t, auditor.AuditLogs, 7) + assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[6].Action) }) } diff --git a/codersdk/audit.go b/codersdk/audit.go index 5a737bf3a9b78..49648e5e9440a 100644 --- a/codersdk/audit.go +++ b/codersdk/audit.go @@ -57,6 +57,8 @@ const ( AuditActionDelete AuditAction = "delete" AuditActionStart AuditAction = "start" AuditActionStop AuditAction = "stop" + AuditActionLogin AuditAction = "login" + AuditActionLogout AuditAction = "logout" ) func (a AuditAction) Friendly() string { @@ -71,6 +73,10 @@ func (a AuditAction) Friendly() string { return "started" case AuditActionStop: return "stopped" + case AuditActionLogin: + return "logged in" + case AuditActionLogout: + return "logged out" default: return "unknown" } diff --git a/docs/admin/audit-logs.md b/docs/admin/audit-logs.md index 192dacc2c8a75..a705b9e371d56 100644 --- a/docs/admin/audit-logs.md +++ b/docs/admin/audit-logs.md @@ -11,6 +11,7 @@ We track the following resources: | Resource | | | ----------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| APIKey
write |
FieldTracked
created_atfalse
expires_atfalse
hashed_secretfalse
idfalse
ip_addressfalse
last_usedfalse
lifetime_secondsfalse
login_typefalse
scopefalse
updated_atfalse
user_idfalse
| | Group
create, write, delete |
FieldTracked
avatar_urltrue
idtrue
memberstrue
nametrue
organization_idfalse
quota_allowancetrue
| | GitSSHKey
create |
FieldTracked
created_atfalse
private_keytrue
public_keytrue
updated_atfalse
user_idtrue
| | Template
write, delete |
FieldTracked
active_version_idtrue
allow_user_cancel_workspace_jobstrue
created_atfalse
created_bytrue
default_ttltrue
deletedfalse
descriptiontrue
display_nametrue
group_acltrue
icontrue
idtrue
is_privatetrue
min_autostart_intervaltrue
nametrue
organization_idfalse
provisionertrue
updated_atfalse
user_acltrue
| diff --git a/docs/api/schemas.md b/docs/api/schemas.md index e36c2d11c464b..bbca5633d5d2c 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -484,6 +484,8 @@ | `delete` | | `start` | | `stop` | +| `login` | +| `logout` | ## codersdk.AuditDiff diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go index 6e4bcfebba935..ffc0f303bd25a 100644 --- a/enterprise/audit/table.go +++ b/enterprise/audit/table.go @@ -22,6 +22,7 @@ var AuditActionMap = map[string][]codersdk.AuditAction{ "Workspace": {codersdk.AuditActionCreate, codersdk.AuditActionWrite, codersdk.AuditActionDelete}, "WorkspaceBuild": {codersdk.AuditActionStart, codersdk.AuditActionStop}, "Group": {codersdk.AuditActionCreate, codersdk.AuditActionWrite, codersdk.AuditActionDelete}, + "APIKey": {codersdk.AuditActionWrite}, } type Action string @@ -109,7 +110,6 @@ var AuditableResources = auditMap(map[any]map[string]Action{ "ttl": ActionTrack, "last_used_at": ActionIgnore, }, - // We don't show any diff for the WorkspaceBuild resource &database.WorkspaceBuild{}: { "id": ActionIgnore, "created_at": ActionIgnore, @@ -133,6 +133,20 @@ var AuditableResources = auditMap(map[any]map[string]Action{ "quota_allowance": ActionTrack, "members": ActionTrack, }, + // We don't show any diff for the APIKey resource + &database.APIKey{}: { + "id": ActionIgnore, + "hashed_secret": ActionIgnore, + "user_id": ActionIgnore, + "last_used": ActionIgnore, + "expires_at": ActionIgnore, + "created_at": ActionIgnore, + "updated_at": ActionIgnore, + "login_type": ActionIgnore, + "lifetime_seconds": ActionIgnore, + "ip_address": ActionIgnore, + "scope": ActionIgnore, + }, }) // auditMap converts a map of struct pointers to a map of struct names as diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 25cfde097d203..f20bb5e8f0e94 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -1031,10 +1031,19 @@ export type APIKeyScope = "all" | "application_connect" export const APIKeyScopes: APIKeyScope[] = ["all", "application_connect"] // From codersdk/audit.go -export type AuditAction = "create" | "delete" | "start" | "stop" | "write" +export type AuditAction = + | "create" + | "delete" + | "login" + | "logout" + | "start" + | "stop" + | "write" export const AuditActions: AuditAction[] = [ "create", "delete", + "login", + "logout", "start", "stop", "write", diff --git a/site/src/components/AuditLogRow/AuditLogDescription.test.tsx b/site/src/components/AuditLogRow/AuditLogDescription.test.tsx index 04bef1a114c77..455642ebfe53a 100644 --- a/site/src/components/AuditLogRow/AuditLogDescription.test.tsx +++ b/site/src/components/AuditLogRow/AuditLogDescription.test.tsx @@ -2,8 +2,12 @@ import { MockAuditLog, MockAuditLogWithWorkspaceBuild, MockWorkspaceCreateAuditLogForDifferentOwner, + MockAuditLogSuccessfulLogin, + MockAuditLogUnsuccessfulLoginKnownUser, + MockAuditLogUnsuccessfulLoginUnknownUser, } from "testHelpers/entities" import { AuditLogDescription } from "./AuditLogDescription" +import { AuditLogRow } from "./AuditLogRow" import { render } from "../../testHelpers/renderHelpers" import { screen } from "@testing-library/react" @@ -59,4 +63,22 @@ describe("AuditLogDescription", () => { ), ).toBeDefined() }) + it("renders the correct string for successful login", async () => { + render() + expect(getByTextContent(`TestUser logged in`)).toBeDefined() + const statusPill = screen.getByRole("status") + expect(statusPill).toHaveTextContent("201") + }) + it("renders the correct string for unsuccessful login for a known user", async () => { + render() + expect(getByTextContent(`TestUser logged in`)).toBeDefined() + const statusPill = screen.getByRole("status") + expect(statusPill).toHaveTextContent("401") + }) + it("renders the correct string for unsuccessful login for an unknown user", async () => { + render() + expect(getByTextContent(`An unknown user logged in`)).toBeDefined() + const statusPill = screen.getByRole("status") + expect(statusPill).toHaveTextContent("401") + }) }) diff --git a/site/src/components/AuditLogRow/AuditLogDescription.tsx b/site/src/components/AuditLogRow/AuditLogDescription.tsx index 11167cbfe50bc..a17f88073f95a 100644 --- a/site/src/components/AuditLogRow/AuditLogDescription.tsx +++ b/site/src/components/AuditLogRow/AuditLogDescription.tsx @@ -12,7 +12,9 @@ export const AuditLogDescription: FC<{ auditLog: AuditLog }> = ({ const { t } = i18next let target = auditLog.resource_target.trim() - let user = auditLog.user?.username.trim() + let user = auditLog.user + ? auditLog.user.username.trim() + : t("auditLog:table.logRow.unknownUser") if (auditLog.resource_type === "workspace_build") { // audit logs with a resource_type of workspace build use workspace name as a target @@ -22,7 +24,7 @@ export const AuditLogDescription: FC<{ auditLog: AuditLog }> = ({ auditLog.additional_fields?.build_reason && auditLog.additional_fields?.build_reason !== "initiator" ? t("auditLog:table.logRow.buildReason") - : auditLog.user?.username.trim() + : user } // SSH key entries have no links diff --git a/site/src/components/AuditLogRow/AuditLogRow.tsx b/site/src/components/AuditLogRow/AuditLogRow.tsx index 10719bbe34b4f..ba84c299b79af 100644 --- a/site/src/components/AuditLogRow/AuditLogRow.tsx +++ b/site/src/components/AuditLogRow/AuditLogRow.tsx @@ -93,7 +93,7 @@ export const AuditLogRow: React.FC = ({ className={styles.fullWidth} > diff --git a/site/src/i18n/en/auditLog.json b/site/src/i18n/en/auditLog.json index c0b9c5864d97a..454a1f9853d70 100644 --- a/site/src/i18n/en/auditLog.json +++ b/site/src/i18n/en/auditLog.json @@ -14,7 +14,8 @@ "browser": "Browser: ", "notAvailable": "Not available", "onBehalfOf": " on behalf of {{owner}}", - "buildReason": "Coder automatically" + "buildReason": "Coder automatically", + "unknownUser": "An unknown user" } }, "paywall": { diff --git a/site/src/pages/AuditPage/AuditPageView.tsx b/site/src/pages/AuditPage/AuditPageView.tsx index 1347a362c035b..ea2202ee7baa8 100644 --- a/site/src/pages/AuditPage/AuditPageView.tsx +++ b/site/src/pages/AuditPage/AuditPageView.tsx @@ -36,14 +36,14 @@ const presetFilters = [ }, { query: "resource_type:template action:create", name: "Added templates" }, { query: "resource_type:user action:delete", name: "Deleted users" }, - { - query: "resource_type:workspace_build action:start", - name: "Started builds", - }, { query: "resource_type:workspace_build action:start build_reason:initiator", name: "Builds started by a user", }, + { + query: "resource_type:api_key action:login", + name: "User logins", + }, ] export interface AuditPageViewProps { diff --git a/site/src/testHelpers/entities.ts b/site/src/testHelpers/entities.ts index 6456fb3abf442..41c3cb176fc1d 100644 --- a/site/src/testHelpers/entities.ts +++ b/site/src/testHelpers/entities.ts @@ -1117,6 +1117,26 @@ export const MockAuditLogGitSSH: TypesGen.AuditLog = { }, } +export const MockAuditLogSuccessfulLogin: TypesGen.AuditLog = { + ...MockAuditLog, + resource_type: "api_key", + resource_target: "", + action: "login", + status_code: 201, + description: "{user} logged in", +} + +export const MockAuditLogUnsuccessfulLoginKnownUser: TypesGen.AuditLog = { + ...MockAuditLogSuccessfulLogin, + status_code: 401, +} + +export const MockAuditLogUnsuccessfulLoginUnknownUser: TypesGen.AuditLog = { + ...MockAuditLogSuccessfulLogin, + status_code: 401, + user: undefined, +} + export const MockWorkspaceQuota: TypesGen.WorkspaceQuota = { credits_consumed: 0, budget: 100,