feat: configurable component-level logging#575
Conversation
- Removed direct usage of zerolog in multiple files and replaced it with a centralized logging utility in the `utils` package. - Introduced `Loggers` struct to manage different loggers (Audit, HTTP, App) with configurable levels and outputs. - Updated all relevant files to utilize the new logging structure, ensuring consistent logging practices across the application. - Enhanced error handling and logging messages for better traceability and debugging.
- Replaced instances of utils logging with tlog in various controllers, services, and middleware. - Introduced audit logging for login success, login failure, and logout events. - Created tlog package with structured logging capabilities using zerolog. - Added tests for the new tlog logger functionality.
📝 WalkthroughWalkthroughReplaces zerolog with a new internal tlog wrapper, adds a structured Log configuration (per-stream app/http/audit), updates env/config examples, introduces audit helper functions, and migrates logging calls across commands, controllers, middleware, services, and tests to use tlog. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #575 +/- ##
==========================================
+ Coverage 19.12% 20.25% +1.13%
==========================================
Files 39 41 +2
Lines 2295 2389 +94
==========================================
+ Hits 439 484 +45
- Misses 1828 1874 +46
- Partials 28 31 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/controller/user_controller.go (2)
104-138: Premature audit success when TOTP is required.
AuditLoginSuccessis called at line 105 before checking if the user has TOTP enabled. If TOTP is required (lines 112-138), the user hasn't fully authenticated yet—only the password was verified. This creates misleading audit entries showing "login success" for incomplete authentication flows.The success audit should only fire after full authentication, i.e., after TOTP verification completes in
totpHandler, or here only when TOTP is not required.🔧 Proposed fix
- tlog.App.Info().Str("username", req.Username).Msg("Login successful") - tlog.AuditLoginSuccess(c, req.Username, "username") + tlog.App.Info().Str("username", req.Username).Msg("Password verified") controller.auth.RecordLoginAttempt(req.Username, true) if userSearch.Type == "local" { user := controller.auth.GetLocalUser(userSearch.Username) if user.TotpSecret != "" { tlog.App.Debug().Str("username", req.Username).Msg("User has TOTP enabled, requiring TOTP verification") err := controller.auth.CreateSessionCookie(c, &config.SessionCookie{ Username: user.Username, Name: utils.Capitalize(req.Username), Email: fmt.Sprintf("%s@%s", strings.ToLower(req.Username), controller.config.CookieDomain), Provider: "username", TotpPending: true, }) if err != nil { tlog.App.Error().Err(err).Msg("Failed to create session cookie") c.JSON(500, gin.H{ "status": 500, "message": "Internal Server Error", }) return } c.JSON(200, gin.H{ "status": 200, "message": "TOTP required", "totpPending": true, }) return } } + tlog.AuditLoginSuccess(c, req.Username, "username") + sessionCookie := config.SessionCookie{
220-229: Missing audit for locked account in TOTP flow.In
loginHandler(line 70), account lockout triggersAuditLoginFailure, but here it's omitted. For audit consistency, consider adding the failure audit when the account is locked during TOTP verification.🔧 Proposed fix
if isLocked { tlog.App.Warn().Str("username", context.Username).Msg("Account is locked due to too many failed TOTP attempts") + tlog.AuditLoginFailure(c, context.Username, "totp") c.Writer.Header().Add("x-tinyauth-lock-locked", "true")
🤖 Fix all issues with AI agents
In @.env.example:
- Around line 22-28: Add the missing audit log level variable to the environment
example: update .env.example so the Audit stream has both enabled and level
variables (TINYAUTH_LOG_STREAMS_AUDIT_ENABLED and
TINYAUTH_LOG_STREAMS_AUDIT_LEVEL) to match how APP/HTTP streams are specified;
this aligns with the LogStreamConfig struct's Level field and ensures the audit
stream can be configured like the others.
In @internal/service/oauth_broker_service.go:
- Line 52: The log call using tlog.App.Error().Err(err).Msgf("Failed to
initialize OAuth service: %T", name) prints the type of name instead of its
value; update the format specifier to "%s" (or "%v") and pass name so the actual
service name is logged, e.g. change the Msgf format to "Failed to initialize
OAuth service: %s" while keeping Err(err) and the same name argument.
🧹 Nitpick comments (7)
config.example.yaml (1)
21-27: Consider addingenabled: trueto app and http streams for documentation clarity.The
auditstream explicitly showsenabled: false, butappandhttpstreams omit this field. While the default is likelytrue, showing it explicitly in the example config would help users understand the full configuration schema.📝 Suggested improvement for clarity
streams: app: + enabled: true level: "warn" http: + enabled: true level: "debug" audit: enabled: falseinternal/service/generic_oauth_service.go (1)
120-120: Consider privacy implications of logging userinfo response.The userinfo response body may contain PII (email, name, profile data). While trace-level logging is typically disabled in production, consider either:
- Redacting sensitive fields before logging, or
- Adding a comment noting this log contains PII for awareness.
The logging migration itself is correct and consistent with the codebase refactor.
internal/utils/tlog/log_wrapper_test.go (1)
42-47: Consider test isolation for global state modification.
Init()modifies global variables (tlog.App,tlog.HTTP,tlog.Audit). If tests run in parallel or other tests depend on a specific global state, this could cause flaky behavior. Consider resetting the global state after the test or usingt.Cleanup()to restore previous values.♻️ Suggested improvement
func TestLoggerInit(t *testing.T) { + // Save original state + originalApp := tlog.App + t.Cleanup(func() { + tlog.App = originalApp + }) + logger := tlog.NewSimpleLogger() logger.Init() assert.Assert(t, tlog.App.GetLevel() != zerolog.Disabled) }cmd/tinyauth/tinyauth.go (1)
11-11: Intentional mixed logger usage is acceptable.The
zerolog/logimport remains forlog.Fatal()calls inmain()beforerunCmdexecutes. This is reasonable since the tlog logger isn't initialized untilrunCmd, and these early failures need immediate logging. Consider adding a brief comment explaining this pattern for future maintainers.Also applies to: 84-84
internal/utils/tlog/log_audit.go (1)
15-23: Consider adding request context for security monitoring.For failed login attempts, additional context like
User-Agentor request path could help identify attack patterns. This is optional and can be added later if needed for security analysis.♻️ Optional enhancement for richer audit data
func AuditLoginFailure(c *gin.Context, username, provider string) { Audit.Warn(). Str("event", "login"). Str("result", "failure"). Str("username", username). Str("provider", provider). Str("ip", c.ClientIP()). + Str("user_agent", c.GetHeader("User-Agent")). Send() }internal/controller/oauth_controller.go (1)
156-171: Consider auditing OAuth login failures for non-whitelisted emails.In
user_controller.go, failed login attempts (unknown user, invalid password) triggerAuditLoginFailure. Here, when a user's email isn't whitelisted, the login attempt fails but isn't audited. For consistent security monitoring, consider adding:tlog.AuditLoginFailure(c, user.Email, req.Provider)internal/utils/tlog/log_wrapper.go (1)
19-23: Document zero-value behavior or initialize to Nop.Before
Init()is called, these globals have zerolog's zero value (logs to stderr at TraceLevel). If accidental pre-initialization usage is a concern, consider initializing tozerolog.Nop()for silent failure, or add a doc comment noting the expected initialization sequence.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.env.examplecmd/tinyauth/create.gocmd/tinyauth/generate.gocmd/tinyauth/healthcheck.gocmd/tinyauth/tinyauth.gocmd/tinyauth/verify.goconfig.example.yamlinternal/bootstrap/app_bootstrap.gointernal/bootstrap/service_bootstrap.gointernal/config/config.gointernal/controller/context_controller.gointernal/controller/context_controller_test.gointernal/controller/oauth_controller.gointernal/controller/proxy_controller.gointernal/controller/proxy_controller_test.gointernal/controller/user_controller.gointernal/controller/user_controller_test.gointernal/middleware/context_middleware.gointernal/middleware/zerolog_middleware.gointernal/service/access_controls_service.gointernal/service/auth_service.gointernal/service/docker_service.gointernal/service/generic_oauth_service.gointernal/service/ldap_service.gointernal/service/oauth_broker_service.gointernal/utils/tlog/log_audit.gointernal/utils/tlog/log_wrapper.gointernal/utils/tlog/log_wrapper_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-25T14:57:03.955Z
Learnt from: steveiliop56
Repo: steveiliop56/tinyauth PR: 211
File: internal/utils/utils.go:373-407
Timestamp: 2025-06-25T14:57:03.955Z
Learning: In Go applications using the gin web framework, IP addresses extracted from gin.Context (like from request headers or remote address) are already validated by the framework and don't require additional validation in utility functions that process them.
Applied to files:
internal/controller/proxy_controller.gointernal/controller/context_controller.go
📚 Learning: 2025-08-29T13:42:35.630Z
Learnt from: steveiliop56
Repo: steveiliop56/tinyauth PR: 329
File: internal/service/auth_service.go:335-337
Timestamp: 2025-08-29T13:42:35.630Z
Learning: In the tinyauth codebase, path filtering works with default behaviors: path.block allows all paths by default and blocks (requires auth for) matching ones, while path.allow blocks all paths by default and allows (skips auth for) matching ones. In IsAuthEnabled function, returning false means authentication is disabled/not required, and returning true means authentication is enabled/required.
Applied to files:
internal/service/auth_service.go
🧬 Code graph analysis (24)
internal/middleware/context_middleware.go (1)
internal/utils/tlog/log_wrapper.go (1)
App(22-22)
cmd/tinyauth/healthcheck.go (1)
internal/utils/tlog/log_wrapper.go (2)
NewSimpleLogger(46-56)App(22-22)
internal/bootstrap/service_bootstrap.go (1)
internal/utils/tlog/log_wrapper.go (1)
App(22-22)
internal/service/generic_oauth_service.go (1)
internal/utils/tlog/log_wrapper.go (1)
App(22-22)
internal/controller/user_controller_test.go (1)
internal/utils/tlog/log_wrapper.go (1)
NewSimpleLogger(46-56)
internal/controller/proxy_controller.go (3)
internal/utils/tlog/log_wrapper.go (1)
App(22-22)internal/config/config.go (2)
App(191-198)UserContext(158-170)internal/utils/security_utils.go (1)
GetSecret(13-28)
internal/utils/tlog/log_wrapper_test.go (2)
internal/config/config.go (3)
LogConfig(80-84)LogStreams(86-90)LogStreamConfig(92-95)internal/utils/tlog/log_wrapper.go (5)
HTTP(21-21)App(22-22)Audit(20-20)NewLogger(25-44)NewSimpleLogger(46-56)
internal/controller/oauth_controller.go (2)
internal/utils/tlog/log_wrapper.go (1)
App(22-22)internal/utils/tlog/log_audit.go (1)
AuditLoginSuccess(5-13)
internal/controller/context_controller.go (1)
internal/utils/tlog/log_wrapper.go (1)
App(22-22)
internal/service/oauth_broker_service.go (1)
internal/utils/tlog/log_wrapper.go (1)
App(22-22)
internal/service/access_controls_service.go (2)
internal/utils/tlog/log_wrapper.go (1)
App(22-22)internal/config/config.go (1)
App(191-198)
cmd/tinyauth/verify.go (2)
internal/utils/tlog/log_wrapper.go (2)
NewSimpleLogger(46-56)App(22-22)internal/config/config.go (1)
App(191-198)
internal/middleware/zerolog_middleware.go (1)
internal/utils/tlog/log_wrapper.go (1)
HTTP(21-21)
internal/controller/context_controller_test.go (1)
internal/utils/tlog/log_wrapper.go (1)
NewSimpleLogger(46-56)
cmd/tinyauth/generate.go (2)
internal/utils/tlog/log_wrapper.go (2)
NewSimpleLogger(46-56)App(22-22)internal/config/config.go (1)
App(191-198)
internal/service/docker_service.go (1)
internal/utils/tlog/log_wrapper.go (1)
App(22-22)
cmd/tinyauth/tinyauth.go (3)
internal/config/config.go (4)
LogConfig(80-84)LogStreams(86-90)LogStreamConfig(92-95)App(191-198)internal/utils/tlog/log_wrapper.go (4)
HTTP(21-21)App(22-22)Audit(20-20)NewLogger(25-44)internal/bootstrap/app_bootstrap.go (1)
NewBootstrapApp(37-41)
internal/utils/tlog/log_audit.go (1)
internal/utils/tlog/log_wrapper.go (1)
Audit(20-20)
internal/bootstrap/app_bootstrap.go (1)
internal/utils/tlog/log_wrapper.go (1)
App(22-22)
internal/controller/user_controller.go (4)
internal/utils/tlog/log_wrapper.go (1)
App(22-22)internal/utils/tlog/log_audit.go (3)
AuditLoginFailure(15-23)AuditLoginSuccess(5-13)AuditLogout(25-33)internal/utils/app_utils.go (1)
GetContext(69-83)internal/controller/context_controller.go (1)
Provider(40-44)
cmd/tinyauth/create.go (1)
internal/utils/tlog/log_wrapper.go (2)
NewSimpleLogger(46-56)App(22-22)
internal/service/auth_service.go (1)
internal/utils/tlog/log_wrapper.go (1)
App(22-22)
internal/controller/proxy_controller_test.go (2)
internal/service/auth_service.go (1)
AuthService(41-48)internal/utils/tlog/log_wrapper.go (1)
NewSimpleLogger(46-56)
internal/utils/tlog/log_wrapper.go (1)
internal/config/config.go (3)
LogConfig(80-84)LogStreams(86-90)LogStreamConfig(92-95)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 19-19: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 21-21: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 21-21: [UnorderedKey] The TINYAUTH_LOG_JSON key should go before the TINYAUTH_LOG_LEVEL key
(UnorderedKey)
[warning] 24-24: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 25-25: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 26-26: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 27-27: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 28-28: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 28-28: [UnorderedKey] The TINYAUTH_LOG_STREAMS_AUDIT_ENABLED key should go before the TINYAUTH_LOG_STREAMS_HTTP_ENABLED key
(UnorderedKey)
🔇 Additional comments (54)
internal/service/oauth_broker_service.go (1)
55-55: LGTM!Good use of structured logging with
.Str("service", name)for the success case.internal/service/docker_service.go (2)
40-48: LGTM!Logging migration is correctly implemented with proper use of structured fields (
.Err(err)for errors, consistent message strings).
91-104: LGTM!Good use of structured logging with contextual fields (container ID, name) for debugging container matches.
cmd/tinyauth/generate.go (3)
92-92: Verify intent: TOTP secret logged in structured field.The TOTP secret is logged via
.Str("secret", secret). While this is a CLI tool for user setup, if JSON logging is enabled or logs are captured elsewhere, the secret could be persisted unexpectedly. Consider whether this should use a plainMsg()orfmt.Println()for CLI output instead of structured logging.
113-113: Same consideration: user credentials logged in structured field.This logs the full user string including password hash and TOTP secret. Same security consideration as above—ensure this is intentional for CLI output and won't be unexpectedly captured in persistent logs.
43-43: LGTM!Proper initialization of the simple logger for CLI command context.
cmd/tinyauth/healthcheck.go (3)
29-29: LGTM!Logger initialization is consistent with other CLI commands.
41-41: LGTM!Good use of structured logging with contextual
app_urlfield.
79-79: LGTM!Appropriate use of
.Interface()to log the health response struct..env.example (1)
15-21: Logging configuration structure looks good.The new logging configuration block is well-organized with clear comments explaining the purpose of each variable. The static analysis warnings about quote characters and key ordering are stylistic—the current logical grouping (global settings before per-stream settings) is more readable than strict alphabetical ordering.
internal/bootstrap/service_bootstrap.go (1)
6-6: LGTM!The logging migration from
log.Warn()totlog.App.Warn()is consistent with the codebase-wide refactor. The warning level and message content remain appropriate for the non-fatal LDAP initialization failure scenario.Also applies to: 36-36
internal/service/ldap_service.go (2)
12-12: LGTM!The logging migration is consistent across all LDAP service operations. Log levels are appropriate:
Infofor mTLS configuration and successful reconnectionErrorfor heartbeat and reconnection failuresDebugfor routine heartbeat checksAlso applies to: 47-47, 69-74
172-172: Logging in background goroutine is appropriate.The debug and info logging in the heartbeat and reconnect flows will now respect the configured log levels via the
tlog.Applogger, which is the intended behavior of this PR.Also applies to: 194-194
internal/controller/proxy_controller_test.go (1)
19-20: TheInit()method returns void—no return value is discarded here.
Init()is a side-effect method that sets global state (Audit,HTTP,App). The*Loggerinstance fromNewSimpleLogger()is created and used only for this initialization, which is a standard pattern used consistently across the test suite and CLI commands. No clarification or refactoring is needed.Likely an incorrect or invalid review comment.
internal/middleware/context_middleware.go (2)
11-11: LGTM - Import updated for tlog migration.The import change from zerolog to the internal tlog package aligns with the project-wide logging refactor.
43-43: Logging migration looks correct.All debug logging statements have been consistently migrated to use
tlog.App.Debug(). The message content and structured fields are preserved, and the debug level is appropriate for these internal authentication flow traces.Also applies to: 65-65, 84-84, 90-90, 115-115, 123-123, 134-141, 150-150, 165-165
internal/controller/user_controller_test.go (1)
27-28: Logger initialization for test setup is appropriate.The
tlog.NewSimpleLogger().Init()call ensures the globaltlog.Applogger is initialized before tests run, matching the pattern used in other test files likecontext_controller_test.go. This is necessary since the code under test usestlog.Appfor logging.Note that
setupUserControlleris called multiple times (e.g., inTestTotpHandlerat lines 262 and 285), which meansInit()is invoked repeatedly. Based on theInit()implementation, this should be safe as it simply reassigns the package-level logger variables.internal/service/access_controls_service.go (2)
8-8: Import updated correctly for tlog migration.
27-53: ACL lookup logging migrated correctly.The debug logging in
lookupStaticACLsandGetAccessControlshas been properly migrated totlog.App.Debug()with all structured fields and messages preserved.internal/controller/proxy_controller.go (5)
12-12: Import updated for tlog migration.
50-61: Error handling and logging is appropriate.The error logging at line 55 correctly captures URI binding failures with the error context.
63-70: Warning-level logging appropriately used for access control issues.The warn-level logs for invalid proxy/method requests and authorization failures are appropriate as they indicate client-side or access control issues rather than server errors.
Also applies to: 185-185, 219-219
87-91: Debug and trace logging levels appropriately differentiated.Debug level is used for flow tracing (browser detection, auth decisions), while trace level is used for detailed data inspection (ACLs, user context). This provides good observability control.
Also applies to: 106-106, 174-174
287-303: Header logging is secure and appropriate.The debug logging in
setHeaderscorrectly logs header operations without exposing sensitive data. The basic auth logging at line 300 only logs the username, not the password, which is good security practice.internal/controller/context_controller_test.go (2)
10-10: Import added for tlog package.
51-53: Logger initialization pattern consistent with other test files.The
tlog.NewSimpleLogger().Init()call mirrors the pattern inuser_controller_test.goandproxy_controller_test.go, ensuring consistent test setup across the controller test suite.internal/middleware/zerolog_middleware.go (1)
52-57: LGTM!The migration from
log.With()totlog.HTTP.With()correctly uses the HTTP stream for HTTP request logging. The sub-logger pattern with structured fields (method, path, address, client_ip, status, latency) is preserved appropriately.internal/service/auth_service.go (2)
76-76: LGTM!The migration to
tlog.Appis appropriate for service-layer logging. Structured error logging withErr(err)and contextual fields likeStr("username", ...)is correctly preserved.
101-120: Audit logging for authentication is already properly implemented at the controller level.The codebase already logs all authentication success and failure events through
AuditLoginSuccess()andAuditLoginFailure()in the controller (internal/controller/user_controller.go). EveryVerifyUser()return—whether false or true—is accompanied by the corresponding audit log at lines 93-96 and 105. This separation of concerns (service layer diagnostics viatlog.App, audit events viatlog.Auditat the controller level) is the correct approach and is fully implemented.internal/controller/context_controller.go (3)
64-64: LGTM!Appropriate use of
tlog.App.Warn()for the security-related configuration warning about disabled UI warnings.
97-97: LGTM!Using
Debuglevel for missing user context is appropriate since this is an expected condition (unauthenticated requests) rather than an error.
111-111: LGTM!Using
Errorlevel for URL parse failure is appropriate as this indicates a configuration issue that should be investigated.internal/bootstrap/app_bootstrap.go (3)
155-202: LGTM!The bootstrap lifecycle logging is well-structured with appropriate log levels: Debug for operational details, Info for significant startup events, and Fatal for unrecoverable server errors.
208-259: LGTM!The heartbeat routine logging is appropriate with Debug for routine operations and Error for failures with proper error context via
Err(err).
262-273: LGTM!Database cleanup routine logging is appropriate with Debug for routine operations and Error for failures.
cmd/tinyauth/verify.go (2)
47-47: LGTM!Correctly initializes the simple logger at command startup.
NewSimpleLogger()uses sensible defaults (info level, console output, app/http streams enabled).
99-113: LGTM!Appropriate use of log levels:
Warnfor the edge case where user provides TOTP code but no secret is configured, andInfofor successful verification messages.cmd/tinyauth/create.go (2)
43-43: LGTM!The simple logger initialization is appropriate for this CLI command context where full configuration isn't available.
77-77: LGTM!The logging migration to
tlog.Appis consistent with the new logging pattern. The "user" field on line 90 intentionally includes the hashed password for the user creation output, which is the expected behavior for this CLI tool.Also applies to: 90-90
internal/config/config.go (2)
80-95: Well-structured logging configuration types.The nested configuration design with per-stream enablement and level override is clean and extensible. The field descriptions and YAML tags are consistent with the existing codebase patterns.
31-31: LGTM!The
Logfield integration into the mainConfigstruct follows the established pattern for other configuration sections.internal/utils/tlog/log_wrapper_test.go (2)
67-93: Good test for verifying log_stream field.This test properly validates that the
log_streamfield is included in JSON output, which is essential for log filtering and routing.
15-32: LGTM!The tests provide good coverage of the logger configuration scenarios: custom levels, simple defaults, and disabled streams.
Also applies to: 34-40, 49-65
cmd/tinyauth/tinyauth.go (2)
38-55: LGTM!Default log configuration is sensible: HTTP and App streams enabled by default, Audit disabled (as noted in PR objectives, audit logs may duplicate app logs and are opt-in).
119-122: LGTM!Logger initialization and usage in
runCmdis correct. The logger is properly created from config and initialized before first use.internal/utils/tlog/log_audit.go (1)
5-33: Well-structured audit logging functions.The consistent field naming (
event,result,username,provider,ip) across all audit functions enables straightforward log filtering and analysis. UsingSend()without a message is appropriate since the structured fields provide all necessary context.internal/controller/user_controller.go (2)
11-11: LGTM!The import of the internal
tlogpackage is correctly added to support the new logging infrastructure.
167-181: LGTM!The logout handler correctly retrieves the user context (set by middleware before handler execution) to conditionally audit the logout event. The session cookie deletion doesn't affect the already-populated gin context.
internal/controller/oauth_controller.go (2)
12-12: LGTM!The import of the internal
tlogpackage is correctly added.
203-214: LGTM!The audit success is correctly placed after successful session cookie creation, ensuring the audit entry only fires for completed OAuth logins.
internal/utils/tlog/log_wrapper.go (4)
25-44: LGTM!The
NewLoggerfunction correctly chains zerolog builders to create a base logger with timestamp, caller info, and level configuration. The ConsoleWriter fallback for non-JSON output is appropriately configured.
58-62: LGTM!The
Initmethod follows a standard write-once pattern for global logger initialization during application startup.
64-74: LGTM!The
createLoggerhelper correctly handles disabled streams withzerolog.Nop(), adds thelog_streamfield for identification, and supports per-stream level overrides.
76-86: LGTM!The
parseLogLevelfunction correctly handles empty strings and invalid levels with sensible defaults. Usinglog.Warn()(zerolog global) for the warning is appropriate here since this runs during initialization beforetlog.Appis available.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/service/oauth_broker_service.go (1)
49-56: Consider using structured fields consistently for the service name.The error log on line 52 embeds
namein the message string, while the success log on line 55 uses.Str("service", name)as a structured field. For consistent log filtering and querying, consider using the structured field approach in both cases.🔧 Suggested fix for consistent structured logging
for name, service := range broker.services { err := service.Init() if err != nil { - tlog.App.Error().Err(err).Msgf("Failed to initialize OAuth service: %s", name) + tlog.App.Error().Err(err).Str("service", name).Msg("Failed to initialize OAuth service") return err } tlog.App.Info().Str("service", name).Msg("Initialized OAuth service") }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.env.exampleconfig.example.yamlinternal/controller/oauth_controller.gointernal/controller/user_controller.gointernal/service/oauth_broker_service.gointernal/utils/tlog/log_audit.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/controller/user_controller.go
- config.example.yaml
- internal/controller/oauth_controller.go
- internal/utils/tlog/log_audit.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/service/oauth_broker_service.go (1)
internal/utils/tlog/log_wrapper.go (1)
App(22-22)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 19-19: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 21-21: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 21-21: [UnorderedKey] The TINYAUTH_LOG_JSON key should go before the TINYAUTH_LOG_LEVEL key
(UnorderedKey)
[warning] 24-24: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 25-25: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 26-26: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 27-27: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 28-28: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 28-28: [UnorderedKey] The TINYAUTH_LOG_STREAMS_AUDIT_ENABLED key should go before the TINYAUTH_LOG_STREAMS_HTTP_ENABLED key
(UnorderedKey)
[warning] 29-29: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 29-29: [UnorderedKey] The TINYAUTH_LOG_STREAMS_AUDIT_LEVEL key should go before the TINYAUTH_LOG_STREAMS_HTTP_ENABLED key
(UnorderedKey)
🔇 Additional comments (4)
internal/service/oauth_broker_service.go (1)
6-10: LGTM!The migration from direct
zerolog/logusage to the newtlogwrapper aligns with the PR objectives for component-level logging. The import change is correct..env.example (3)
15-17: LGTM!The section header is clear and consistent with other sections in the file.
22-29: Per-stream configuration structure looks good.The logical grouping of stream configurations (pairing
_ENABLEDwith_LEVELfor each stream) is more maintainable than alphabetical ordering.Regarding the static analysis warnings:
- QuoteCharacter: The entire file consistently uses quotes for all values, which is a valid style choice for .env files. No action needed.
- UnorderedKey: The current logical grouping (APP, then HTTP, then AUDIT) is more readable than alphabetical ordering. The pairing of
_ENABLEDand_LEVELfor each stream enhances maintainability.The default values are appropriate:
- APP and HTTP streams enabled by default for operational visibility
- AUDIT stream disabled by default (appropriate given potential privacy/compliance considerations)
18-21: No action required. The configuration keysTINYAUTH_LOG_LEVELandTINYAUTH_LOG_JSONin.env.examplefollow the standard paerser naming convention for mapping struct fields to environment variables (convertingLevel→LOG_LEVELandJson→LOG_JSON). These are not renamed keys and have no backward compatibility implications.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @internal/utils/tlog/log_audit.go:
- Around line 18-27: AuditLoginFailure currently ignores the reason parameter so
failure cause is lost; update the AuditLoginFailure function to include the
reason in the audit entry (e.g., add Str("reason", reason) to the Audit.Warn()
chain) so the failure reason is recorded alongside event, result, username,
provider and ip.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/utils/tlog/log_audit.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/utils/tlog/log_audit.go (1)
internal/utils/tlog/log_wrapper.go (1)
Audit(20-20)
🔇 Additional comments (2)
internal/utils/tlog/log_audit.go (2)
7-16: LGTM!The function correctly logs all relevant audit fields for successful logins with appropriate Info level and caller frame adjustment.
29-38: LGTM!The function correctly logs all relevant audit fields for logout events.
2 participants
Fixes: #448
This PR refactors the logging implementation in Tinyauth. The core change is introducing a log wrapper instead of direct zerolog usages. With this, configurable sub-loggers have been added for different components/streams in the application and all existing logs inside the application have been configured to use the sub-logger pattern where every log includes a
log_streamfield.Three categories of log streams have been added to start with:
http: middleware http request logs are now tagged withlog_stream:http. Enabled by defaultapp: everything else that previously existed, except the above is tagged withlog_stream:app, also enabled by default.audit: This stream records login success and failures. Disabled by defaultThese categories are based on my limited view of the codebase and could be split/consolidated further. My use-case however works with these such that I don't want to see the noise from HTTP logs, but only application+audit. There is some duplicate information in audit and application logs which will likely need to be de-duped in a future commit/PR.
Configuration should work with env vars, CLI args, and config file.
Example environment variables:
If users want to only see audit logs, they can simply:
Only selective audit logs have been added for now, but these can be expanded upon easily. Sample audit log:
{"level":"info","log_stream":"audit","event":"login","result":"success","username":"pbal","provider":"pocketid","ip":"100.98.29.58","time":"2026-01-12T01:00:05Z","time":"2026-01-12T01:00:05Z","caller":"/tinyauth/internal/utils/tlog/log_audit.go:12"} {"level":"info","log_stream":"audit","event":"logout","result":"success","username":"pbal","provider":"pocketid","ip":"100.98.29.58","time":"2026-01-12T01:00:09Z","time":"2026-01-12T01:00:09Z","caller":"/tinyauth/internal/utils/tlog/log_audit.go:32"}A side-benefit of this type of logging configuration is that a future enhancement to implement more log destination wouldn't require major configuration changes, just something like
destination: file://app/auth.log, ordestination: s3://mybucket/tinyauth/auth.log, plus this can be configurable on a per-stream basis.One may argue that audit logs shouldn't be streamed to
stdout/stderr, however a future enhancement like the above would make it trivial to change this, even something likesqllite://path/to/auditlogs.dbcould be implemented if needed.Summary by CodeRabbit
New Features
Configuration
Tests
✏️ Tip: You can customize this high-level summary in your review settings.