refactor: unify labels#329
Conversation
WalkthroughRefactors label structures to an Apps-centric model, updates services and controllers to use new AppLabels, IPLabels, and PathLabels, adjusts OAuth/group and IP checks, changes Docker label lookup to AppLabels, limits label decoding to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant PC as ProxyController
participant AS as AuthService
participant DS as DockerService
participant CFG as AppLabels/IP/Path
C->>PC: HTTP request (Host, URI, headers)
PC->>DS: GetLabels(app, host) -> AppLabels
PC->>CFG: Load AppLabels (Config/Response/IP/Path/OAuth/Users)
PC->>AS: IsBypassedIP(IPLabels, clientIP)?
alt Bypassed
PC->>PC: Apply Response.Headers
PC-->>C: 200 (bypassed)
else Check Auth Enabled
PC->>AS: IsAuthEnabled(URI, PathLabels)?
alt Disabled
PC->>PC: Apply Response.Headers
PC-->>C: 200 ("Authenticated")
else Enabled
PC->>AS: CheckIP(IPLabels, clientIP)?
alt IP denied
PC-->>C: 403
else Determine auth method
alt Basic Auth
PC->>PC: Get secret from Response.BasicAuth
PC-->>C: 401 challenge / validate
else OAuth
PC->>AS: IsResourceAllowed(ctx, AppLabels)?
alt Group mismatch
PC->>PC: Build UnauthorizedQuery{GroupErr:true, IP}
PC-->>C: 302 -> /unauthorized?...
else Allowed
PC->>PC: Apply Response.Headers
PC-->>C: 200
end
end
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 #329 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 32 32
Lines 2363 2378 +15
=====================================
- Misses 2363 2378 +15 ☔ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/config/config.go (1)
47-47: Fix typo in configuration field name.There's a typo in the field name.
- FogotPasswordMessage string `mapstructure:"forgot-password-message"` + ForgotPasswordMessage string `mapstructure:"forgot-password-message"`internal/service/auth_service.go (1)
303-324: Critical issue: Incorrect group membership check logic.Line 317 has a logic error - it's checking if the group matches against the entire groups string instead of checking if the current group from the user's groups matches the required groups.
for _, group := range groupsSplit { - if utils.CheckFilter(groups, group) { + if utils.CheckFilter(context.OAuthGroups, group) { return true } }
🧹 Nitpick comments (1)
internal/service/docker_service.go (1)
84-94: Consider adding validation for empty app configurations.The loop iterates over
labels.Appsbut doesn't validate if the app configuration is properly populated. Consider adding validation to ensure required fields are present.for appName, appLabels := range labels.Apps { + // Skip apps with no domain configured + if appLabels.Config.Domain == "" && appName == "" { + log.Debug().Msg("Skipping app with no domain or name configured") + continue + } if appLabels.Config.Domain == domain { log.Debug().Str("id", inspect.ID).Msg("Found matching container by domain") return appLabels, nil }
📜 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 (5)
internal/config/config.go(5 hunks)internal/controller/proxy_controller.go(6 hunks)internal/service/auth_service.go(7 hunks)internal/service/docker_service.go(2 hunks)internal/utils/label_utils.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
internal/service/docker_service.go (2)
internal/utils/label_utils.go (1)
GetLabels(11-20)internal/config/config.go (2)
AppLabels(133-140)Config(17-57)
internal/service/auth_service.go (2)
internal/config/config.go (4)
UserContext(102-112)AppLabels(133-140)PathLabels(173-176)IPLabels(156-160)internal/utils/security_utils.go (1)
CheckFilter(71-96)
internal/controller/proxy_controller.go (3)
internal/utils/label_utils.go (1)
ParseHeaders(22-38)internal/utils/security_utils.go (2)
GetSecret(13-28)GetBasicAuth(43-46)internal/config/config.go (1)
UnauthorizedQuery(116-121)
⏰ 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 (11)
internal/config/config.go (3)
1-8: LGTM! Build metadata variables properly structured.The addition of build-time metadata variables (Version, CommitHash, BuildTimestamp) follows best practices for tracking build information.
9-13: LGTM! Cookie name constants are well-defined.The cookie name constants are properly named and follow a consistent naming convention.
129-176: Well-structured Apps-centric label model!The new label structure with AppLabels as the core entity provides clear separation of concerns:
- ConfigLabels for domain configuration
- UsersLabels for access control
- OAuthLabels for OAuth-specific settings
- IPLabels for IP-based access control
- ResponseLabels for response manipulation
- PathLabels for path-based access control
This is a significant improvement over the previous flat structure and aligns well with the PR objectives.
internal/utils/label_utils.go (1)
11-20: No backward compatibility issues detectedSearch of Docker-compose examples, README.md, and SECURITY.md shows no usages of the removed keys (
tinyauth.users,tinyauth.oauth, etc.), so existing configurations remain valid.internal/service/docker_service.go (1)
58-99: Improved container matching logic with Apps-centric model.The new GetLabels implementation properly handles the Apps-centric model:
- First checks for domain match within app labels
- Falls back to matching by container name
- Returns appropriate AppLabels structure
The logic is clear and aligns with the new label structure.
internal/controller/proxy_controller.go (4)
92-106: Consistent header and basic auth handling across bypass path.The bypass IP path correctly uses the new nested label structure for headers and basic auth.
157-179: IP denial now includes client IP in redirect query.Good addition to include the client IP in the unauthorized query when IP check fails. This provides better debugging information.
234-267: OAuth group check properly signals group errors.The addition of
GroupErr: truein the UnauthorizedQuery when OAuth group checks fail is a good improvement for error differentiation.
249-249: GroupErr handling confirmed
The unauthorized-page.tsx retrievessearchParams.get("groupErr")and conditionally renders the group-specific error UI (frontend/src/pages/unauthorized-page.tsx:19,41).internal/service/auth_service.go (2)
286-301: Improved access control with separate block and allow lists.The new implementation properly handles both block and allow lists for users, with block list taking precedence. This provides more granular access control.
326-354: Clarify and add tests for path-based auth logic
The implementation ofIsAuthEnabledforpath.Blockandpath.Allowlacks documentation or tests and appears inverted—please confirm the intended semantics for both patterns and add unit tests to validate block/allow behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/config/config.go (1)
17-57: Use supportedipv4validator instead ofip4_addrThe
go-playground/validatorlibrary usesipv4, notip4_addr, and no customip4_addrrule is registered in the codebase.- Address string `validate:"required,ip4_addr" mapstructure:"address"` + Address string `validate:"required,ipv4" mapstructure:"address"`internal/service/auth_service.go (1)
323-351: Path regex handling: support slash-delimited patterns and clarify precedence.Current code compiles the raw label; if admins use
/regex/(like other filters), it won’t match as intended. Also, make block-vs-allow precedence explicit: apply block if set; otherwise apply allow.-func (auth *AuthService) IsAuthEnabled(uri string, path config.PathLabels) (bool, error) { - // Check for block list - if path.Block != "" { - regex, err := regexp.Compile(path.Block) - if err != nil { - return true, err - } - if !regex.MatchString(uri) { - return false, nil - } - } - // Check for allow list - if path.Allow != "" { - regex, err := regexp.Compile(path.Allow) - if err != nil { - return true, err - } - if regex.MatchString(uri) { - return false, nil - } - } - return true, nil -} +func (auth *AuthService) IsAuthEnabled(uri string, path config.PathLabels) (bool, error) { + trim := func(p string) string { + p = strings.TrimSpace(p) + if strings.HasPrefix(p, "/") && strings.HasSuffix(p, "/") && len(p) >= 2 { + return p[1 : len(p)-1] + } + return p + } + // Block semantics: allow all by default; block (require auth) on match + if s := strings.TrimSpace(path.Block); s != "" { + re, err := regexp.Compile(trim(s)) + if err != nil { + return true, err + } + if re.MatchString(uri) { + return true, nil // auth required + } + return false, nil // no auth required + } + // Allow semantics: block all by default; allow (skip auth) on match + if s := strings.TrimSpace(path.Allow); s != "" { + re, err := regexp.Compile(trim(s)) + if err != nil { + return true, err + } + if re.MatchString(uri) { + return false, nil // no auth required + } + return true, nil // auth required + } + // Neither provided: require auth by default + return true, nil +}
🧹 Nitpick comments (2)
internal/config/config.go (1)
129-176: Addjson/mapstructuretags to labels to harden decoding across sources.Decoding from Docker labels/JSON benefits from explicit tags (case/format stability), especially for nested fields like
basic-auth,password-file, etc.-type Labels struct { - Apps map[string]AppLabels -} +type Labels struct { + Apps map[string]AppLabels `json:"apps" mapstructure:"apps"` +} -type AppLabels struct { - Config ConfigLabels - Users UsersLabels - OAuth OAuthLabels - IP IPLabels - Response ResponseLabels - Path PathLabels -} +type AppLabels struct { + Config ConfigLabels `json:"config" mapstructure:"config"` + Users UsersLabels `json:"users" mapstructure:"users"` + OAuth OAuthLabels `json:"oauth" mapstructure:"oauth"` + IP IPLabels `json:"ip" mapstructure:"ip"` + Response ResponseLabels `json:"response" mapstructure:"response"` + Path PathLabels `json:"path" mapstructure:"path"` +} -type ConfigLabels struct { - Domain string -} +type ConfigLabels struct { + Domain string `json:"domain" mapstructure:"domain"` +} -type UsersLabels struct { - Allow string - Block string -} +type UsersLabels struct { + Allow string `json:"allow" mapstructure:"allow"` + Block string `json:"block" mapstructure:"block"` +} -type OAuthLabels struct { - Whitelist string - Groups string -} +type OAuthLabels struct { + Whitelist string `json:"whitelist" mapstructure:"whitelist"` + Groups string `json:"groups" mapstructure:"groups"` +} -type IPLabels struct { - Allow []string - Block []string - Bypass []string -} +type IPLabels struct { + Allow []string `json:"allow" mapstructure:"allow"` + Block []string `json:"block" mapstructure:"block"` + Bypass []string `json:"bypass" mapstructure:"bypass"` +} -type ResponseLabels struct { - Headers []string - BasicAuth BasicAuthLabels -} +type ResponseLabels struct { + Headers []string `json:"headers" mapstructure:"headers"` + BasicAuth BasicAuthLabels `json:"basic-auth" mapstructure:"basic-auth"` +} -type BasicAuthLabels struct { - Username string - Password string - PasswordFile string -} +type BasicAuthLabels struct { + Username string `json:"username" mapstructure:"username"` + Password string `json:"password" mapstructure:"password"` + PasswordFile string `json:"password-file" mapstructure:"password-file"` +} -type PathLabels struct { - Allow string - Block string -} +type PathLabels struct { + Allow string `json:"allow" mapstructure:"allow"` + Block string `json:"block" mapstructure:"block"` +}Optional: consider deprecating
BasicAuthLabels.Passwordin favor ofPasswordFilefor security.internal/service/auth_service.go (1)
303-321: Drop unused*gin.Contextparameter fromIsInOAuthGroupand update its call site
- Change signature in
internal/service/auth_service.goto:- func (auth *AuthService) IsInOAuthGroup(c *gin.Context, context config.UserContext, requiredGroups string) bool { + func (auth *AuthService) IsInOAuthGroup(context config.UserContext, requiredGroups string) bool {- In
internal/controller/proxy_controller.go(line 234), update the call to:groupOK := controller.Auth.IsInOAuthGroup(userContext, labels.OAuth.Groups)- Remove the now-unused
github.com/gin-gonic/ginimport fromauth_service.go.
📜 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 (3)
internal/bootstrap/app_bootstrap.go(1 hunks)internal/config/config.go(6 hunks)internal/service/auth_service.go(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T13:42:35.587Z
Learnt from: steveiliop56
PR: steveiliop56/tinyauth#329
File: internal/service/auth_service.go:335-337
Timestamp: 2025-08-29T13:42:35.587Z
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 (3)
internal/bootstrap/app_bootstrap.go (2)
internal/config/config.go (1)
Config(17-57)internal/controller/context_controller.go (1)
ConfiguredProviders(35-44)
internal/service/auth_service.go (2)
internal/config/config.go (4)
UserContext(102-112)AppLabels(133-140)PathLabels(173-176)IPLabels(156-160)internal/utils/security_utils.go (1)
CheckFilter(71-96)
internal/config/config.go (3)
internal/controller/oauth_controller.go (2)
CSRFCookieName(21-27)Config(29-34)internal/service/generic_oauth_service.go (1)
Config(18-25)internal/middleware/context_middleware.go (1)
Config(18-22)
⏰ 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 (9)
internal/config/config.go (5)
3-7: LGTM: build metadata added cleanly.No issues with the build-time vars.
9-14: LGTM: cookie name templates.Consistent naming; matches usage in bootstrap.
61-66: LGTM: Claims (OIDC) struct.Fields and tags look correct;
anyfor Groups is reasonable.
79-112: LGTM: user/session/public types.Naming and fields consistent with usage elsewhere.
116-121: LGTM: UnauthorizedQuery extensions.Adding
GroupErrandIPaligns with controller/service flows.internal/bootstrap/app_bootstrap.go (1)
178-187: LGTM: fixed field rename (ForgotPasswordMessage).Matches the corrected Config field.
internal/service/auth_service.go (3)
286-301: User allow/block logic looks correct; confirm OAuth skip of user lists is intentional.OAuth path short-circuits to whitelist by email and ignores
Users.Block/Allow. If you intend username-based blocks to also affect OAuth users, integrate a block check before returning.
365-397: LGTM: IP allow/block precedence and defaults.Block-first, then allow, then default based on presence of allow list is sound. Good logging.
399-414: LGTM: IP bypass list.Straightforward early-allow for trusted IPs.
1 participant
Fixes #202
Summary by CodeRabbit
New Features
Refactor
Bug Fixes