refactor(audit): add NoopAuditService to eliminate nil checks

[appleboy]

Summary

• Add AuditLogger interface and NoopAuditService (Null Object Pattern), mirroring the existing NoopCache approach from refactor(cache): add NoopCache to eliminate nil checks in token service #134
• Remove ~37 if auditService != nil checks across 12 service, handler, and middleware files
• Bootstrap conditionally creates real AuditService (enabled) or NoopAuditService (disabled) — no runtime nil checks needed

Changes

• New: internal/core/audit.go — AuditLogger interface + AuditLogEntry struct (moved from services to avoid circular imports)
• New: internal/services/audit_noop.go — zero-allocation no-op implementation with safe return values (non-nil empty slices/maps)
• New: internal/services/audit_noop_test.go — tests for all noop methods
• Modified: internal/services/audit.go — removed enabled field and internal guards, simplified constructor
• Modified: 12 files with nil check removal, 6 bootstrap files with type updates, 15 test files updated

Test plan

• make build passes
• make test — all existing tests pass
• make lint — 0 issues
• Grep confirms zero remaining auditService != nil or auditService == nil checks
• Code review by 3 parallel agents (reuse, quality, efficiency+security) — no issues found

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 85.12000% with 93 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/services/client.go	67.05%	27 Missing and 1 partial ⚠️
internal/services/token_management.go	61.70%	18 Missing ⚠️
internal/services/user.go	92.30%	10 Missing and 1 partial ⚠️
internal/middleware/ratelimit.go	68.00%	5 Missing and 3 partials ⚠️
internal/services/audit.go	0.00%	8 Missing ⚠️
internal/bootstrap/bootstrap.go	0.00%	7 Missing ⚠️
internal/handlers/audit.go	0.00%	3 Missing ⚠️
internal/bootstrap/server.go	0.00%	2 Missing ⚠️
internal/services/authorization.go	97.10%	1 Missing and 1 partial ⚠️
internal/services/dashboard.go	0.00%	1 Missing and 1 partial ⚠️
... and 2 more

📢 Thoughts on this report? Let us know!

[Copilot]

Pull request overview

This PR refactors audit logging to use a core.AuditLogger interface and a NoopAuditService (Null Object Pattern) so callers can invoke audit logging without scattered nil checks, while also moving AuditLogEntry into internal/core to avoid circular dependencies.

Changes:

• Introduces core.AuditLogger + core.AuditLogEntry, and adds services.NoopAuditService with unit tests.
• Updates service/handler/middleware dependencies from *services.AuditService to core.AuditLogger and removes many nil guards.
• Updates bootstrap wiring to instantiate either the real AuditService (enabled) or NoopAuditService (disabled), and updates tests accordingly.

Reviewed changes

Copilot reviewed 45 out of 45 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
internal/services/user.go	Switches to core.AuditLogger and removes audit nil checks in user flows.
internal/services/user_test.go	Updates test construction to pass NewNoopAuditService().
internal/services/user_oauth_test.go	Updates OAuth user service tests to pass NewNoopAuditService().
internal/services/token.go	Switches TokenService to core.AuditLogger.
internal/services/token_test.go	Updates token service/device service test setup to use NewNoopAuditService().
internal/services/token_refresh.go	Removes audit nil guards and logs via core.AuditLogEntry.
internal/services/token_management.go	Removes audit nil guards and logs via core.AuditLogEntry.
internal/services/token_introspect.go	Removes audit nil guards and logs via core.AuditLogEntry.
internal/services/token_introspect_test.go	Updates introspection service construction to use NewNoopAuditService().
internal/services/token_exchange.go	Removes audit nil guards and logs token issuance via core.AuditLogEntry.
internal/services/token_client_credentials.go	Removes audit nil guards and logs issuance via core.AuditLogEntry.
internal/services/token_client_credentials_test.go	Updates CC grant tests to pass NewNoopAuditService().
internal/services/token_cache_test.go	Updates cached token service tests to pass NewNoopAuditService().
internal/services/token_cache_bench_test.go	Updates benchmarks to pass NewNoopAuditService().
internal/services/device.go	Switches DeviceService to core.AuditLogger and removes audit nil checks.
internal/services/device_test.go	Updates device service tests to pass NewNoopAuditService().
internal/services/device_security_test.go	Updates device security tests to pass NewNoopAuditService().
internal/services/dashboard.go	Switches DashboardService to core.AuditLogger.
internal/services/dashboard_test.go	Uses NewNoopAuditService() instead of starting/stopping real audit service.
internal/services/client.go	Switches ClientService to core.AuditLogger and removes audit nil checks.
internal/services/client_user.go	Removes audit nil guards for user-owned client updates/deletes.
internal/services/client_user_test.go	Updates client user tests to pass NewNoopAuditService().
internal/services/client_test.go	Updates client tests to pass NewNoopAuditService().
internal/services/authorization.go	Switches AuthorizationService to core.AuditLogger and removes nil checks.
internal/services/authorization_test.go	Updates authorization service tests to pass NewNoopAuditService().
internal/services/audit.go	Moves AuditLogEntry out, removes enabled guards, and implements core.AuditLogger.
internal/services/audit_noop.go	Adds NoopAuditService implementing core.AuditLogger.
internal/services/audit_noop_test.go	Adds unit tests for NoopAuditService.
internal/middleware/ratelimit.go	Switches rate limit audit dependency to core.AuditLogger and logs with core.AuditLogEntry.
internal/middleware/ratelimit_test.go	Updates rate limiter tests to provide NewNoopAuditService().
internal/handlers/token_introspect_test.go	Uses NewNoopAuditService() in handler test env setup.
internal/handlers/token_client_credentials_test.go	Uses NewNoopAuditService() in handler test env setup.
internal/handlers/token_cache_integration_test.go	Uses NewNoopAuditService() in integration test env setup.
internal/handlers/session_test.go	Uses NewNoopAuditService() in session handler test setup.
internal/handlers/registration.go	Switches handler audit dependency to core.AuditLogger and logs with core.AuditLogEntry.
internal/handlers/registration_test.go	Uses NewNoopAuditService() in registration handler tests.
internal/handlers/device_test.go	Uses NewNoopAuditService() in device handler tests.
internal/handlers/audit.go	Switches handler audit dependency to core.AuditLogger and logs with core.AuditLogEntry.
internal/core/audit.go	Introduces AuditLogger interface and AuditLogEntry in core.
internal/bootstrap/services.go	Updates service initialization to accept core.AuditLogger.
internal/bootstrap/server.go	Updates shutdown/cleanup helpers to accept core.AuditLogger and gates shutdown by config.
internal/bootstrap/router.go	Updates router setup signature to accept core.AuditLogger.
internal/bootstrap/ratelimit.go	Wires core.AuditLogger into rate limiter creation.
internal/bootstrap/handlers.go	Updates handler dependency struct to use core.AuditLogger.
internal/bootstrap/bootstrap.go	Bootstraps real audit service vs noop depending on config.

Comments suppressed due to low confidence (2)

internal/services/device.go:49

• auditService is now a core.AuditLogger and is called unconditionally (s.auditService.Log(...)). Because interfaces can be nil, constructing DeviceService with a nil audit logger will panic at runtime. Consider defaulting to NewNoopAuditService() (or validating non-nil) inside NewDeviceService.

type DeviceService struct {
	store        core.Store
	config       *config.Config
	auditService core.AuditLogger
	metrics      core.Recorder
}

func NewDeviceService(
	s core.Store,
	cfg *config.Config,
	auditService core.AuditLogger,
	m core.Recorder,
) *DeviceService {
	return &DeviceService{
		store:        s,
		config:       cfg,
		auditService: auditService,
		metrics:      m,
	}

internal/services/dashboard.go:31

• DashboardService.GetDashboardStats calls s.auditService.GetAuditLogs(...) unconditionally. Because auditService is an interface, it can still be nil and would panic here. Consider defaulting to NewNoopAuditService() (or validating non-nil) in NewDashboardService.

type DashboardService struct {
	store        core.Store
	auditService core.AuditLogger
}

func NewDashboardService(
	s core.Store,
	auditService core.AuditLogger,
) *DashboardService {
	return &DashboardService{
		store:        s,
		auditService: auditService,
	}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 45 out of 45 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

internal/handlers/audit.go:140

• The audit logging block is gated on a Gin context key "username", but the codebase does not appear to set this key anywhere (search for Set("username") returns no results). This likely means the "Viewed audit logs" event is never emitted. Consider deriving the username from the request context/user object (or omit ActorUsername and let the audit service enrich it) and avoid hard type assertions on Gin context values.

	// Log this action (viewing audit logs)
	if userID, exists := c.Get("user_id"); exists {
		if username, usernameExists := c.Get("username"); usernameExists {
			h.auditService.Log(c.Request.Context(), core.AuditLogEntry{
				EventType:     models.EventTypeAuditLogView,
				Severity:      models.SeverityInfo,
				ActorUserID:   userID.(string),
				ActorUsername: username.(string),
				Action:        "Viewed audit logs",
				Details: models.AuditDetails{
					"page":      params.Page,
					"page_size": params.PageSize,
					"filters":   filters,
				},
				Success:       true,
				RequestPath:   c.Request.URL.Path,
				RequestMethod: c.Request.Method,
				UserAgent:     c.Request.UserAgent(),
			})
		}

internal/handlers/audit.go:299

• The export audit logging block is gated on a Gin context key "username", but the codebase does not appear to set this key anywhere (search for Set("username") returns no results). This likely means the "Exported audit logs to CSV" event is never emitted. Consider deriving the username from the request context/user object (or omit ActorUsername and let the audit service enrich it) and avoid hard type assertions on Gin context values.

	// Log this action
	if userID, exists := c.Get("user_id"); exists {
		if username, usernameExists := c.Get("username"); usernameExists {
			h.auditService.Log(c.Request.Context(), core.AuditLogEntry{
				EventType:     models.EventTypeAuditLogExported,
				Severity:      models.SeverityInfo,
				ActorUserID:   userID.(string),
				ActorUsername: username.(string),
				Action:        "Exported audit logs to CSV",
				Details: models.AuditDetails{
					"record_count": len(logs),
					"filters":      filters,
				},
				Success:       true,
				RequestPath:   c.Request.URL.Path,
				RequestMethod: c.Request.Method,
				UserAgent:     c.Request.UserAgent(),
			})
		}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 46 out of 46 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

internal/handlers/audit.go:129

• The audit-view event logging is gated on c.Get("username"), but this codebase stores username in the session (middleware.SessionUsername) and does not appear to c.Set("username", ...) anywhere. As a result, these audit events will never be recorded. Consider sourcing the username from getUserFromContext(c) (preferred) or from the session, and avoid unchecked type assertions (userID.(string), username.(string)) to prevent panics if the context values are not strings.

	// Log this action (viewing audit logs)
	if userID, exists := c.Get("user_id"); exists {
		if username, usernameExists := c.Get("username"); usernameExists {
			h.auditService.Log(c.Request.Context(), core.AuditLogEntry{
				EventType:     models.EventTypeAuditLogView,
				Severity:      models.SeverityInfo,
				ActorUserID:   userID.(string),
				ActorUsername: username.(string),
				Action:        "Viewed audit logs",

internal/handlers/audit.go:289

• The export audit event logging has the same issue as ListAuditLogs: it's gated on c.Get("username"), but username is stored in the session and never set on the gin context, so this audit event likely never fires. Consider using getUserFromContext(c) / session values for username and using safe type assertions to avoid panics.

	// Log this action
	if userID, exists := c.Get("user_id"); exists {
		if username, usernameExists := c.Get("username"); usernameExists {
			h.auditService.Log(c.Request.Context(), core.AuditLogEntry{
				EventType:     models.EventTypeAuditLogExported,
				Severity:      models.SeverityInfo,
				ActorUserID:   userID.(string),
				ActorUsername: username.(string),
				Action:        "Exported audit logs to CSV",

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.