refactor: don't export non-needed fields#336
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughMultiple exported struct fields across controllers, middleware, and services were converted to unexported fields; constructors and all internal references were updated. Small behavioral tweaks include LDAP filter escaping, TLS minimum version enforcement, session DB checks, Docker GetLabels signature change, and centralized proxy error/header handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ProxyCtrl as ProxyController
participant DockerSvc as DockerService
participant AuthSvc as AuthService
participant Broker as OAuthBroker
Client->>ProxyCtrl: HTTP request (host/path)
ProxyCtrl->>DockerSvc: GetLabels(host) %% changed signature: single appDomain param
alt labels found
ProxyCtrl->>AuthSvc: IsAuthEnabled / CheckIP / IsResourceAllowed
alt requires auth
AuthSvc-->>ProxyCtrl: auth state
alt OAuth flow
ProxyCtrl->>Broker: GetService(provider)
Broker-->>ProxyCtrl: service or nil
ProxyCtrl->>Client: redirect to <config.AppURL>/login or /continue
else session/basic auth
ProxyCtrl->>AuthSvc: Validate/CreateSessionCookie
AuthSvc-->>ProxyCtrl: session result
ProxyCtrl->>Client: proxy to target or redirect on error (via handleError)
end
else no auth
ProxyCtrl->>Client: proxy to target
end
else labels not found
ProxyCtrl->>Client: 404 / redirect to error (via handleError)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #336 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 32 32
Lines 2394 2372 -22
=====================================
+ Misses 2394 2372 -22 ☔ 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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/service/docker_service.go (1)
58-99: Bug: app parameter is unused in GetLabels.The method accepts app but never uses it, leading to potentially incorrect matches. Filter by app when provided.
- for appName, appLabels := range labels.Apps { + for appName, appLabels := range labels.Apps { + if app != "" && app != appName && app != strings.TrimPrefix(inspect.Name, "/") { + continue + } if appLabels.Config.Domain == domain { log.Debug().Str("id", inspect.ID).Msg("Found matching container by domain") return appLabels, nil } if strings.TrimPrefix(inspect.Name, "/") == appName { log.Debug().Str("id", inspect.ID).Msg("Found matching container by app name") return appLabels, nil } }internal/service/generic_oauth_service.go (1)
45-63: Set HTTP client timeout.Transport is customized, but client has no timeout.
httpClient := &http.Client{ Transport: transport, + Timeout: 10 * time.Second, } ctx := context.Background() ctx = context.WithValue(ctx, oauth2.HTTPClient, httpClient) verifier := oauth2.GenerateVerifier()internal/controller/context_controller.go (1)
66-91: Fix potential nil-deref: using context before checking error.utils.GetContext can fail; you read fields before handling err, risking panic.
-func (controller *ContextController) userContextHandler(c *gin.Context) { - context, err := utils.GetContext(c) - - userContext := UserContextResponse{ - Status: 200, - Message: "Success", - IsLoggedIn: context.IsLoggedIn, - Username: context.Username, - Name: context.Name, - Email: context.Email, - Provider: context.Provider, - OAuth: context.OAuth, - TotpPending: context.TotpPending, - } - - if err != nil { +func (controller *ContextController) userContextHandler(c *gin.Context) { + context, err := utils.GetContext(c) + if err != nil { log.Debug().Err(err).Msg("No user context found in request") - userContext.Status = 401 - userContext.Message = "Unauthorized" - userContext.IsLoggedIn = false - c.JSON(200, userContext) + c.JSON(200, UserContextResponse{ + Status: 401, + Message: "Unauthorized", + IsLoggedIn: false, + Username: "", + Name: "", + Email: "", + Provider: "", + OAuth: false, + TotpPending: false, + }) return } - c.JSON(200, userContext) + c.JSON(200, UserContextResponse{ + Status: 200, + Message: "Success", + IsLoggedIn: context.IsLoggedIn, + Username: context.Username, + Name: context.Name, + Email: context.Email, + Provider: context.Provider, + OAuth: context.OAuth, + TotpPending: context.TotpPending, + })
🧹 Nitpick comments (21)
internal/service/docker_service.go (1)
24-31: Avoid shadowing imported identifier names.Locals named client and loop var named container shadow imports and reduce readability. Prefer cli and ctr.
Example:
cli, err := client.NewClientWithOpts(client.FromEnv) docker.client = cli for _, ctr := range containers { // ... }internal/service/database_service.go (2)
30-30: Validate database path early.Fail fast if DatabasePath is empty; avoids opaque gorm errors.
func (ds *DatabaseService) Init() error { + if ds.config.DatabasePath == "" { + return fmt.Errorf("database path is required") + } gormDB, err := gorm.Open(sqlite.Open(ds.config.DatabasePath), &gorm.Config{})Add import:
import "fmt"
50-51: Provide Close() to release the underlying sql.DB.Without a Close, processes may leak connections on shutdown.
Add:
func (ds *DatabaseService) Close() error { if ds.database == nil { return nil } sqlDB, err := ds.database.DB() if err != nil { return err } return sqlDB.Close() }internal/service/github_oauth_service.go (1)
89-99: Harden requests: timeout + User-Agent.
- Apply per-request timeout via context.
- GitHub recommends setting a User-Agent header.
if github.token == nil { return user, fmt.Errorf("no token present; call VerifyCode first") } client := github.config.Client(github.context, github.token) ctx, cancel := context.WithTimeout(github.context, 10*time.Second) defer cancel() req, err := http.NewRequestWithContext(ctx, "GET", "https://api.github.com/user", nil) if err != nil { return user, err } req.Header.Set("Accept", "application/vnd.github+json") req.Header.Set("User-Agent", "tinyauth") res, err := client.Do(req)Apply the same pattern to the /user/emails request.
internal/service/ldap_service.go (1)
60-78: connect(): locking pattern OK; TLS min version set to 1.2.Consider calling conn.SetTimeout(10*time.Second) after Bind to avoid indefinite LDAP ops.
internal/service/oauth_broker_service.go (1)
57-63: “Configured” vs “initialized” services and ordering.Currently returns names from broker.services (post-Init). If callers expect “configured” regardless of Init success, iterate broker.configs instead and sort for stable output.
-func (broker *OAuthBrokerService) GetConfiguredServices() []string { - services := make([]string, 0, len(broker.services)) - for name := range broker.services { +func (broker *OAuthBrokerService) GetConfiguredServices() []string { + services := make([]string, 0, len(broker.configs)) + for name := range broker.configs { services = append(services, name) } + sort.Strings(services) return services }Remember to import "sort".
internal/service/generic_oauth_service.go (2)
29-41: Constructor wiring LGTM.Minor nit: Go style prefers “URL” capitalization; see below.
24-24: Naming + request hardening: userinfoURL + timeout.
- Use userinfoURL (Go style).
- Use a context-bound request for the userinfo call.
- userinfoUrl string + userinfoURL string @@ - userinfoUrl: config.UserinfoURL, + userinfoURL: config.UserinfoURL, @@ - res, err := client.Get(generic.userinfoUrl) + ctx, cancel := context.WithTimeout(generic.context, 10*time.Second) + defer cancel() + req, err := http.NewRequestWithContext(ctx, "GET", generic.userinfoURL, nil) + if err != nil { + return user, err + } + res, err := client.Do(req)Also applies to: 40-41, 94-97
internal/controller/health_controller.go (1)
16-17: Serve HEAD without a JSON body.Using the same JSON handler for HEAD is non-ideal; return status-only for HEAD.
- controller.router.HEAD("/health", controller.healthHandler) + controller.router.HEAD("/health", func(c *gin.Context) { c.Status(200) })internal/middleware/context_middleware.go (1)
147-152: LDAP email construction may be inaccurate.Consider using the LDAP user’s mail attribute (if available) instead of composing with RootDomain.
internal/controller/resources_controller.go (1)
29-31: Route registration LGTM; consider skipping when ResourcesDir is empty.Optionally avoid registering the route if ResourcesDir is unset to reduce 404 noise.
func (controller *ResourcesController) SetupRoutes() { - controller.router.GET("/resources/*resource", controller.resourcesHandler) + if controller.config.ResourcesDir != "" { + controller.router.GET("/resources/*resource", controller.resourcesHandler) + } }internal/controller/user_controller.go (1)
217-225: Clarify rate-limit message for TOTP.Message refers to “login attempts”; specify “TOTP attempts” for clarity.
- "message": fmt.Sprintf("Too many failed login attempts. Try again in %d seconds", remainingTime), + "message": fmt.Sprintf("Too many failed TOTP attempts. Try again in %d seconds", remainingTime),internal/controller/proxy_controller.go (3)
93-106: DRY the repeated header/basic-auth response setupThe same header-setting logic appears three times. Factor it to a helper to reduce drift.
@@ - headers := utils.ParseHeaders(labels.Response.Headers) - for key, value := range headers { - log.Debug().Str("header", key).Msg("Setting header") - c.Header(key, value) - } - basicPassword := utils.GetSecret(labels.Response.BasicAuth.Password, labels.Response.BasicAuth.PasswordFile) - if labels.Response.BasicAuth.Username != "" && basicPassword != "" { - log.Debug().Str("username", labels.Response.BasicAuth.Username).Msg("Setting basic auth header") - c.Header("Authorization", fmt.Sprintf("Basic %s", utils.GetBasicAuth(labels.Response.BasicAuth.Username, basicPassword))) - } + controller.applyUpstreamHeaders(c, labels)Add once (outside the diffed ranges) in this file:
func (controller *ProxyController) applyUpstreamHeaders(c *gin.Context, labels config.AppLabels) { headers := utils.ParseHeaders(labels.Response.Headers) for key, value := range headers { log.Debug().Str("header", key).Msg("Setting header") c.Header(key, value) } basicPassword := utils.GetSecret(labels.Response.BasicAuth.Password, labels.Response.BasicAuth.PasswordFile) if labels.Response.BasicAuth.Username != "" && basicPassword != "" { log.Debug().Str("username", labels.Response.BasicAuth.Username).Msg("Setting basic auth header") c.Header("Authorization", fmt.Sprintf("Basic %s", utils.GetBasicAuth(labels.Response.BasicAuth.Username, basicPassword))) } }Also applies to: 135-148, 275-286
66-69: Fallback if X-Forwarded-Uri is missingHarden against missing/stripped headers.
- uri := c.Request.Header.Get("X-Forwarded-Uri") + uri := c.Request.Header.Get("X-Forwarded-Uri") + if uri == "" { + uri = c.Request.RequestURI + }Also applies to: 115-116
86-87: Consolidate redirects to helpersMany identical redirects to AppURL/error, /unauthorized, and /login. Centralize to small helpers to cut repetition and reduce chances of inconsistent behavior.
// Example (add once): func (controller *ProxyController) redirectError(c *gin.Context) { c.Redirect(http.StatusTemporaryRedirect, fmt.Sprintf("%s/error", controller.config.AppURL)) }Then replace the repeated redirect lines with controller.redirectError(c) / controller.redirectUnauthorized(...).
Also applies to: 128-130, 173-175, 177-179, 225-227, 229-231, 260-262, 264-266, 309-311, 313-314
internal/controller/oauth_controller.go (3)
172-180: Typo: usename → usernameMinor readability fix.
-var usename string +var username string @@ - usename = user.PreferredUsername + username = user.PreferredUsername @@ - usename = strings.Replace(user.Email, "@", "_", -1) + username = strings.Replace(user.Email, "@", "_", -1) @@ - Username: usename, + Username: username,Also applies to: 183-188
111-113: Deduplicate AppURL/error redirectsSame pattern repeated across branches. Consider a small helper (e.g., controller.redirectError(c)) like in ProxyController.
Also applies to: 122-124, 129-131, 137-139, 143-145, 154-156, 204-206
77-84: Add SameSite=Lax to CSRF/redirect cookies
Gin introduced Context.SetSameSite in v1.6.0 (gin-gonic.com); insertc.SetSameSite(http.SameSiteLaxMode)immediately before each
c.SetCookieinoauth_controller.go(77–84, 115–116, 208–209).internal/service/auth_service.go (3)
233-234: Unscoped() unnecessary for sessionsSession model has no soft-delete field; Unscoped adds no value. Simplify queries.
- res := auth.database.Unscoped().Where("uuid = ?", cookie).Delete(&model.Session{}) + res := auth.database.Where("uuid = ?", cookie).Delete(&model.Session{}) @@ - res := auth.database.Unscoped().Where("uuid = ?", cookie).First(&session) + res := auth.database.Where("uuid = ?", cookie).First(&session) @@ - res := auth.database.Unscoped().Where("uuid = ?", session.UUID).Delete(&model.Session{}) + res := auth.database.Where("uuid = ?", session.UUID).Delete(&model.Session{})Also applies to: 253-254, 266-267
126-127: Reduce log noise for missing local usersThis is a common path; downgrade to debug.
- log.Warn().Str("username", username).Msg("Local user not found") + log.Debug().Str("username", username).Msg("Local user not found")
221-222: Consider SameSite on the session cookieAlign with OAuth cookies; Lax/Strict can mitigate CSRF on cookie use (subject to app flows).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
internal/controller/context_controller.go(2 hunks)internal/controller/health_controller.go(1 hunks)internal/controller/oauth_controller.go(6 hunks)internal/controller/proxy_controller.go(11 hunks)internal/controller/resources_controller.go(1 hunks)internal/controller/user_controller.go(9 hunks)internal/middleware/context_middleware.go(7 hunks)internal/middleware/ui_middleware.go(3 hunks)internal/service/auth_service.go(11 hunks)internal/service/database_service.go(3 hunks)internal/service/docker_service.go(2 hunks)internal/service/generic_oauth_service.go(4 hunks)internal/service/github_oauth_service.go(3 hunks)internal/service/google_oauth_service.go(3 hunks)internal/service/ldap_service.go(7 hunks)internal/service/oauth_broker_service.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
internal/service/database_service.go (1)
internal/config/config.go (1)
Config(17-56)
internal/service/google_oauth_service.go (1)
internal/config/config.go (3)
Config(17-56)OAuthServiceConfig(67-76)Claims(60-65)
internal/controller/user_controller.go (3)
internal/service/auth_service.go (1)
AuthService(37-44)internal/config/config.go (1)
SessionCookie(91-99)internal/utils/string_utils.go (1)
Capitalize(7-12)
internal/middleware/context_middleware.go (4)
internal/service/auth_service.go (1)
AuthService(37-44)internal/service/oauth_broker_service.go (1)
OAuthBrokerService(18-21)internal/config/config.go (1)
UserContext(101-111)internal/utils/string_utils.go (1)
Capitalize(7-12)
internal/controller/proxy_controller.go (2)
internal/service/docker_service.go (1)
DockerService(14-17)internal/service/auth_service.go (1)
AuthService(37-44)
internal/service/auth_service.go (5)
internal/service/docker_service.go (1)
DockerService(14-17)internal/service/ldap_service.go (1)
LdapService(24-28)internal/config/config.go (4)
UserSearch(86-89)Config(17-56)SessionCookieName(11-11)SessionCookie(91-99)internal/utils/security_utils.go (1)
CheckFilter(71-96)internal/model/session_model.go (1)
Session(3-12)
internal/service/github_oauth_service.go (1)
internal/config/config.go (3)
Config(17-56)OAuthServiceConfig(67-76)Claims(60-65)
internal/controller/oauth_controller.go (5)
internal/service/auth_service.go (1)
AuthService(37-44)internal/service/oauth_broker_service.go (1)
OAuthBrokerService(18-21)internal/config/config.go (4)
CSRFCookieName(12-12)RedirectCookieName(13-13)UnauthorizedQuery(115-120)SessionCookie(91-99)internal/utils/app_utils.go (1)
IsRedirectSafe(76-102)internal/utils/string_utils.go (1)
CoalesceToString(14-30)
internal/service/oauth_broker_service.go (4)
internal/config/config.go (2)
OAuthServiceConfig(67-76)Claims(60-65)internal/service/github_oauth_service.go (1)
NewGithubOAuthService(38-48)internal/service/google_oauth_service.go (1)
NewGoogleOAuthService(33-43)internal/service/generic_oauth_service.go (1)
NewGenericOAuthService(27-42)
internal/service/generic_oauth_service.go (1)
internal/config/config.go (3)
Config(17-56)OAuthServiceConfig(67-76)Claims(60-65)
internal/service/ldap_service.go (1)
internal/config/config.go (1)
Config(17-56)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (32)
internal/middleware/ui_middleware.go (1)
14-16: Encapsulation LGTM.Unexporting uiFs/uiFileServer is a good move; external API remains stable.
internal/service/database_service.go (1)
19-21: Encapsulation LGTM.Making config/database unexported aligns with the PR’s goal; external API remains via GetDatabase.
internal/service/google_oauth_service.go (3)
27-30: Good: fields made private to match encapsulation goal.
35-41: Constructor update looks correct.
66-68: LGTM: PKCE challenge used with S256.internal/service/github_oauth_service.go (2)
32-36: Good: fields switched to private.
40-46: Constructor change matches intent.internal/service/ldap_service.go (3)
24-28: Good: internal state (conn, mutex) now private; Config remains exported for consumers.
110-117: Bind(): locking with defer is correct.
151-159: Verify LDAPService fields and backoff API
Could not locate amutexor theconnect()implementation ininternal/service/ldap_service.go; please ensureLDAPServicedeclares a mutex to guardconnand thatconnect()manages locking internally. Also confirm that ingithub.com/cenkalti/backoff/v5v5.0.3,Retryexpects an operation of typefunc(context.Context) error.internal/service/oauth_broker_service.go (3)
19-27: Good: internal maps are now private; ctor initializes them.
31-43: Service wiring LGTM.
45-52: Init flow is clear and logs per-service status.internal/service/generic_oauth_service.go (1)
19-25: Encapsulation improvements look good.internal/controller/health_controller.go (1)
5-7: Encapsulation refactor LGTM.Private router field and constructor assignment look correct.
Also applies to: 10-12
internal/middleware/context_middleware.go (3)
19-22: Privatization of fields and constructor initialization LGTM.Names and wiring updated consistently.
Also applies to: 26-29
86-90: Confirm whitelist policy for non-OAuth logins.You enforce email whitelist for OAuth cookies but not for basic-auth users. If whitelist should apply globally, mirror the check here; otherwise, ignore.
132-141: Guard only missing user via empty Username
GetLocalUser returns a config.User value (never nil), so check only for an empty Username:- user := m.auth.GetLocalUser(basic.Username) - if user == nil || user.Username == "" { + user := m.auth.GetLocalUser(basic.Username) + if user.Username == "" { log.Warn().Str("username", basic.Username).Msg("Local user record missing") c.Next() return }Likely an incorrect or invalid review comment.
internal/controller/context_controller.go (3)
48-57: Privatization refactor LGTM.config/router fields and constructor wiring look good.
60-64: Route setup LGTM.Grouping under /context via private router is correct.
99-107: Config sourcing LGTM.All fields correctly read from controller.config.
internal/controller/resources_controller.go (2)
14-17: Privatization refactor LGTM.config/router/fileServer are properly assigned.
Also applies to: 23-26
33-42: Handler behavior LGTM.404 on missing dir and delegation to fileServer are fine.
internal/controller/user_controller.go (4)
29-39: Privatization refactor LGTM.Fields and constructor wiring updated correctly.
43-47: Route setup LGTM.Private router group under /user is correct.
115-121: TOTP pending cookie LGTM.Cookie creation with TotpPending looks correct.
141-146: Session cookie creation LGTM.Email composition via RootDomain and provider set to "username" are consistent.
Also applies to: 246-251
internal/controller/proxy_controller.go (3)
25-28: Encapsulation: exported → private fields is correctStruct visibility tightened without behavior change. Good.
33-37: Constructor and route wiring updates—LGTMInit and SetupRoutes correctly use the private fields.
Also applies to: 41-43
93-106: Authorization precedence — confirm overwrite is intentionalYou set Authorization from the incoming request, then potentially overwrite with Basic auth from labels. Confirm this precedence is desired for all paths.
internal/controller/oauth_controller.go (1)
30-34: Encapsulation + routes update—LGTMPrivate fields and route grouping updated cleanly.
Also applies to: 38-42, 46-49
internal/service/auth_service.go (1)
38-44: Private fields + constructor init—LGTMEncapsulation improved; constructor populates the new fields correctly.
Also applies to: 48-53
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/service/docker_service.go (1)
58-66: Normalize appDomain to strip port before matching labels.Host header often includes a port (e.g., example.com:443); strict equality would miss matches.
-func (docker *DockerService) GetLabels(appDomain string) (config.AppLabels, error) { +func (docker *DockerService) GetLabels(appDomain string) (config.AppLabels, error) { + host := strings.ToLower(appDomain) + if h, _, err := net.SplitHostPort(appDomain); err == nil { + host = strings.ToLower(h) + } @@ - if !isConnected { + if !isConnected { log.Debug().Msg("Docker not connected, returning empty labels") return config.AppLabels{}, nil }Add import:
import "net"internal/controller/proxy_controller.go (2)
164-176: Potential panic: err checked after using queriesIf query.Values returns an error, queries is nil and queries.Set() will panic. Check err before Set.
- queries, err := query.Values(config.UnauthorizedQuery{ - Resource: strings.Split(host, ".")[0], - }) - - if userContext.OAuth { - queries.Set("username", userContext.Email) - } else { - queries.Set("username", userContext.Username) - } - - if err != nil { + queries, err := query.Values(config.UnauthorizedQuery{ + Resource: strings.Split(host, ".")[0], + }) + if err != nil { log.Error().Err(err).Msg("Failed to encode unauthorized query") c.Redirect(http.StatusTemporaryRedirect, fmt.Sprintf("%s/error", controller.config.AppURL)) return } + if userContext.OAuth { + queries.Set("username", userContext.Email) + } else { + queries.Set("username", userContext.Username) + }
198-213: Same panic risk here: check err before SetReorder as above for the group-mismatch branch.
- queries, err := query.Values(config.UnauthorizedQuery{ - Resource: strings.Split(host, ".")[0], - GroupErr: true, - }) - - if userContext.OAuth { - queries.Set("username", userContext.Email) - } else { - queries.Set("username", userContext.Username) - } - - if err != nil { + queries, err := query.Values(config.UnauthorizedQuery{ + Resource: strings.Split(host, ".")[0], + GroupErr: true, + }) + if err != nil { log.Error().Err(err).Msg("Failed to encode unauthorized query") c.Redirect(http.StatusTemporaryRedirect, fmt.Sprintf("%s/error", controller.config.AppURL)) return } + if userContext.OAuth { + queries.Set("username", userContext.Email) + } else { + queries.Set("username", userContext.Username) + }
♻️ Duplicate comments (6)
internal/service/docker_service.go (4)
15-17: Add per-call timeouts to harden Docker operations and store a default in the service.Prevents hangs and aligns with prior feedback.
type DockerService struct { - client *client.Client - context context.Context + client *client.Client + context context.Context + timeout time.Duration } @@ - docker.client = client - docker.context = ctx + docker.client = client + docker.context = ctx + docker.timeout = 2 * time.SecondAdd import:
import "time"Also applies to: 32-33
38-43: Guard nil client and use a bounded context for ContainerList.Avoids panic if Init wasn’t called and ensures bounded latency.
-func (docker *DockerService) GetContainers() ([]container.Summary, error) { - containers, err := docker.client.ContainerList(docker.context, container.ListOptions{}) +func (docker *DockerService) GetContainers() ([]container.Summary, error) { + if docker.client == nil { + return nil, errors.New("docker client is nil") + } + ctx, cancel := context.WithTimeout(docker.context, docker.timeout) + defer cancel() + containers, err := docker.client.ContainerList(ctx, container.ListOptions{})Add import:
import "errors"
46-51: Same for Inspect: nil-guard and timeout.- inspect, err := docker.client.ContainerInspect(docker.context, containerId) + if docker.client == nil { + return container.InspectResponse{}, errors.New("docker client is nil") + } + ctx, cancel := context.WithTimeout(docker.context, docker.timeout) + defer cancel() + inspect, err := docker.client.ContainerInspect(ctx, containerId)
53-56: Same for Ping: nil-guard and timeout.-func (docker *DockerService) DockerConnected() bool { - _, err := docker.client.Ping(docker.context) +func (docker *DockerService) DockerConnected() bool { + if docker.client == nil { + return false + } + ctx, cancel := context.WithTimeout(docker.context, docker.timeout) + defer cancel() + _, err := docker.client.Ping(ctx) return err == nil }internal/controller/user_controller.go (1)
85-88: Treat both empty and “unknown” user types as not found.Else empty Type slips into password verification path.
- if userSearch.Type == "unknown" { + if userSearch.Type == "" || userSearch.Type == "unknown" {internal/controller/proxy_controller.go (1)
78-88: Ensure Gin trusted proxies are configured for ClientIP()ClientIP() depends on proxy config; verify router.SetTrustedProxies is set appropriately in your bootstrap.
#!/bin/bash # Verify SetTrustedProxies exists and is called rg -nP --type=go -C3 'gin\.(Default|New)\s*\(' rg -nP --type=go -C3 '\bSetTrustedProxies\s*\('
🧹 Nitpick comments (12)
internal/service/oauth_broker_service.go (3)
46-53: Wrap init errors with service name for better upstream diagnostics.Returning the bare error loses which provider failed.
Apply:
- if err != nil { - log.Error().Err(err).Msgf("Failed to initialize OAuth service: %s", name) - return err - } + if err != nil { + log.Error().Err(err).Msgf("Failed to initialize OAuth service: %s", name) + return fmt.Errorf("init oauth service %s: %w", name, err) + }Add import:
import "fmt"
72-78: Minor naming nit: avoid ambiguous “service” identifier.Use serviceName for the string param to avoid confusion with OAuthService vars.
-func (broker *OAuthBrokerService) GetUser(service string) (config.Claims, error) { - oauthService, exists := broker.services[service] +func (broker *OAuthBrokerService) GetUser(serviceName string) (config.Claims, error) { + oauthService, exists := broker.services[serviceName]
58-65: Use stdlib “slices” and “maps” packages
Replace the x/exp import and manual key loop with Go 1.21+ stdlib:-import "golang.org/x/exp/slices" +import ( + "slices" + "maps" +) func (broker *OAuthBrokerService) GetConfiguredServices() []string { - services := make([]string, 0, len(broker.services)) - for name := range broker.services { - services = append(services, name) - } + services := maps.Keys(broker.services) slices.Sort(services) return services }internal/service/docker_service.go (2)
84-93: Case-insensitive domain compare.Domains are case-insensitive; make the comparison robust.
- if appLabels.Config.Domain == appDomain { + if strings.EqualFold(appLabels.Config.Domain, host) { log.Debug().Str("id", inspect.ID).Msg("Found matching container by domain") return appLabels, nil }
66-83: Avoid per-container Inspect when possible; prefer labels from the list call.Container summaries usually include Labels; this saves N API calls on hot paths.
Sketch:
containers, err := docker.GetContainers() for _, ctr := range containers { labels, err := utils.GetLabels(ctr.Labels) if err != nil { /* warn and continue */ } // same matching logic... }Also applies to: 90-93
internal/controller/user_controller.go (2)
115-121: DRY: factor session-cookie creation into a small helper.Same structure repeated; reduces errors and keeps TOTP vs non-TOTP paths consistent.
Example:
func (c *UserController) setUserCookie(ctx *gin.Context, u, name, provider string, totp bool) error { return c.auth.CreateSessionCookie(ctx, &config.SessionCookie{ Username: u, Name: utils.Capitalize(name), Email: fmt.Sprintf("%s@%s", strings.ToLower(name), c.config.RootDomain), Provider: provider, TotpPending: totp, }) }Also applies to: 141-146, 246-251
221-224: Use net/http status constants instead of literals.Improves readability and avoids magic numbers.
- c.JSON(429, gin.H{ - "status": 429, + c.JSON(http.StatusTooManyRequests, gin.H{ + "status": http.StatusTooManyRequests, "message": fmt.Sprintf("Too many failed TOTP attempts. Try again in %d seconds", remainingTime), })Add import:
import "net/http"internal/middleware/context_middleware.go (2)
45-56: Set IsLoggedIn explicitly for TOTP pending contextMinor clarity: set IsLoggedIn to false to make intent explicit for downstream consumers.
- c.Set("context", &config.UserContext{ + c.Set("context", &config.UserContext{ Username: cookie.Username, Name: cookie.Name, Email: cookie.Email, Provider: "username", TotpPending: true, - TotpEnabled: true, + TotpEnabled: true, + IsLoggedIn: false, })
137-138: Avoid duplicating email formatting logicTwo identical fmt.Sprintf("%s@%s", ...) calls; consider a tiny helper (e.g., utils.DefaultEmail(username, m.config.RootDomain)) to centralize this.
Also applies to: 149-151
internal/controller/proxy_controller.go (2)
78-83: Remove duplicate Authorization header settingAuthorization is set in setHeaders(); also setting it before calling setHeaders is redundant and can be confusing when BasicAuth override applies.
- if controller.auth.IsBypassedIP(labels.IP, clientIP) { - c.Header("Authorization", c.Request.Header.Get("Authorization")) - controller.setHeaders(c, labels) + if controller.auth.IsBypassedIP(labels.IP, clientIP) { + controller.setHeaders(c, labels) c.JSON(200, gin.H{ "status": 200, "message": "Authenticated", }) return }- c.Header("Authorization", c.Request.Header.Get("Authorization")) c.Header("Remote-User", utils.SanitizeHeader(userContext.Username)) c.Header("Remote-Name", utils.SanitizeHeader(userContext.Name)) c.Header("Remote-Email", utils.SanitizeHeader(userContext.Email)) c.Header("Remote-Groups", utils.SanitizeHeader(userContext.OAuthGroups)) controller.setHeaders(c, labels)Also applies to: 220-227, 256-272
134-143: Avoid naming collisions with “context”Rename local var to userCtx to avoid shadowing common “context” naming and improve readability.
- context, err := utils.GetContext(c) + userCtx, err := utils.GetContext(c) if err != nil { ... } else { - userContext = context + userContext = userCtx }internal/service/auth_service.go (1)
325-353: Regex compiled on every requestIsAuthEnabled recompiles regexes per call; consider caching compiled patterns with the labels if this is a hot path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
internal/controller/oauth_controller.go(6 hunks)internal/controller/proxy_controller.go(8 hunks)internal/controller/user_controller.go(8 hunks)internal/middleware/context_middleware.go(7 hunks)internal/service/auth_service.go(11 hunks)internal/service/docker_service.go(3 hunks)internal/service/ldap_service.go(7 hunks)internal/service/oauth_broker_service.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/service/ldap_service.go
- internal/controller/oauth_controller.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-25T14:57:03.955Z
Learnt from: steveiliop56
PR: steveiliop56/tinyauth#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.go
🧬 Code graph analysis (6)
internal/controller/proxy_controller.go (5)
internal/service/docker_service.go (1)
DockerService(14-17)internal/service/auth_service.go (1)
AuthService(37-44)internal/utils/label_utils.go (1)
ParseHeaders(22-38)internal/config/config.go (1)
AppLabels(132-139)internal/utils/security_utils.go (2)
GetSecret(13-28)GetBasicAuth(43-46)
internal/service/docker_service.go (2)
internal/utils/label_utils.go (1)
GetLabels(11-20)internal/config/config.go (3)
AppLabels(132-139)Config(17-56)Labels(128-130)
internal/controller/user_controller.go (5)
internal/service/auth_service.go (1)
AuthService(37-44)internal/config/config.go (1)
SessionCookie(91-99)internal/utils/string_utils.go (1)
Capitalize(7-12)internal/controller/resources_controller.go (1)
Config(13-17)internal/controller/health_controller.go (1)
Router(5-7)
internal/middleware/context_middleware.go (4)
internal/service/auth_service.go (1)
AuthService(37-44)internal/service/oauth_broker_service.go (1)
OAuthBrokerService(19-22)internal/config/config.go (1)
UserContext(101-111)internal/utils/string_utils.go (1)
Capitalize(7-12)
internal/service/auth_service.go (4)
internal/service/ldap_service.go (1)
LdapService(24-28)internal/config/config.go (4)
UserSearch(86-89)Config(17-56)SessionCookieName(11-11)SessionCookie(91-99)internal/utils/security_utils.go (1)
CheckFilter(71-96)internal/model/session_model.go (1)
Session(3-12)
internal/service/oauth_broker_service.go (4)
internal/config/config.go (2)
OAuthServiceConfig(67-76)Claims(60-65)internal/service/github_oauth_service.go (1)
NewGithubOAuthService(38-48)internal/service/google_oauth_service.go (2)
NewGoogleOAuthService(33-43)Config(26-31)internal/service/generic_oauth_service.go (2)
NewGenericOAuthService(27-42)Config(18-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
internal/service/oauth_broker_service.go (2)
20-28: Good encapsulation by unexporting internal maps.This aligns with the PR goal and reduces accidental external mutation surface.
58-65: Clarify intended source of “configured” list.This enumerates initialized services, not raw configs. If callers expect to see all configured providers before Init is called, consider iterating broker.configs instead or document the Init requirement.
internal/service/docker_service.go (1)
38-38: Ignore Docker type name suggestion: ListOptions and InspectResponse are correct for SDK v28.3.3. (pkg.go.dev)Likely an incorrect or invalid review comment.
internal/controller/user_controller.go (1)
29-39: Unexported fields and constructor wiring look good.Clearer encapsulation and consistent with the refactor across controllers.
Also applies to: 43-47
internal/middleware/context_middleware.go (1)
60-66: Good: session -> unknown/error now invalidates and falls back cleanlyDeleting the stale session and dropping to basic auth on unknown/error is the right call.
internal/controller/proxy_controller.go (1)
256-285: Centralizing header/redirect handling is solidNice consolidation via setHeaders and handleError; simplifies the handler flow and reduces duplication.
internal/service/auth_service.go (2)
155-183: Login-attempt locking looks correctRWMutex usage and lockout math are sound; resets on success are handled.
68-76: SearchUser Type="error" is handled as deny at all call sites
Verified: both ContextMiddleware and loginHandler treat"error"the same as"unknown"by denying access.
1 participant
Summary by CodeRabbit
Refactor
Bug Fixes
Chores