coder · Kira-Pilot · Feb 6, 2023 · Jan 30, 2023 · Jan 30, 2023 · Jan 30, 2023
diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go
diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json
diff --git 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))
 
@@ -315,7 +315,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{
@@ -338,7 +338,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{
@@ -354,5 +354,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
@@ -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
@@ -31,6 +31,10 @@ type Request[T Auditable] struct {
 
 	Old T
 	New T
+
+	// This optional field can be passed in when the userID cannot be determined from the API Key
+	// such as in the case of login, when the audit log is created prior the API Key's existence.
+	UserID uuid.UUID
 }
 
 type BuildAuditParams[T Auditable] struct {
@@ -64,6 +68,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 +92,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 +115,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))
 	}
@@ -130,7 +141,14 @@ func InitRequest[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request
 
 		// If no resources were provided, there's nothing we can audit.
 		if ResourceID(req.Old) == uuid.Nil && ResourceID(req.New) == uuid.Nil {
-			return
+			// If the request action is a login or logout, we always want to audit it even if
+			// there is no diff. This is so we can capture events where an API Key is never created
+			// because an unknown user fails to login.
+			// TODO: introduce the concept of an anonymous user so we always have a userID even
+			// when dealing with a mystery user. https://github.com/coder/coder/issues/6054
+			if req.params.Action != database.AuditActionLogin && req.params.Action != database.AuditActionLogout {
+				return
+			}
 		}
 
 		var diffRaw = []byte("{}")
@@ -150,16 +168,24 @@ 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]),
-			ResourceID:       either(req.Old, req.New, ResourceID[T]),
-			ResourceTarget:   either(req.Old, req.New, ResourceTarget[T]),
+			ResourceType:     either(req.Old, req.New, ResourceType[T], req.params.Action),
+			ResourceID:       either(req.Old, req.New, ResourceID[T], req.params.Action),
+			ResourceTarget:   either(req.Old, req.New, ResourceTarget[T], req.params.Action),
 			Action:           p.Action,
 			Diff:             diffRaw,
 			StatusCode:       int32(sw.Status),
@@ -202,9 +228,9 @@ func BuildAudit[T Auditable](ctx context.Context, p *BuildAuditParams[T]) {
 		UserID:           p.UserID,
 		Ip:               ip,
 		UserAgent:        sql.NullString{},
-		ResourceType:     either(p.Old, p.New, ResourceType[T]),
-		ResourceID:       either(p.Old, p.New, ResourceID[T]),
-		ResourceTarget:   either(p.Old, p.New, ResourceTarget[T]),
+		ResourceType:     either(p.Old, p.New, ResourceType[T], p.Action),
+		ResourceID:       either(p.Old, p.New, ResourceID[T], p.Action),
+		ResourceTarget:   either(p.Old, p.New, ResourceTarget[T], p.Action),
 		Action:           p.Action,
 		Diff:             diffRaw,
 		StatusCode:       int32(p.Status),
@@ -221,11 +247,15 @@ func BuildAudit[T Auditable](ctx context.Context, p *BuildAuditParams[T]) {
 	}
 }
 
-func either[T Auditable, R any](old, new T, fn func(T) R) R {
+func either[T Auditable, R any](old, new T, fn func(T) R, auditAction database.AuditAction) R {
 	if ResourceID(new) != uuid.Nil {
 		return fn(new)
 	} else if ResourceID(old) != uuid.Nil {
 		return fn(old)
+	} else if auditAction == database.AuditActionLogin || auditAction == database.AuditActionLogout {
+		// If the request action is a login or logout, we always want to audit it even if
+		// there is no diff. See the comment in audit.InitRequest for more detail.
+		return fn(old)
 	} else {
 		panic("both old and new are nil")
 	}

diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql
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
@@ -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
@@ -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
diff --git 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
@@ -61,10 +61,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) {
@@ -285,8 +286,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) {
@@ -448,8 +449,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
@@ -127,8 +127,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()
@@ -646,8 +646,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)
 	})
 }
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		-- It's not possible to drop enum values from enum types, so the UP has "IF NOT
		-- EXISTS".