feat: ldap group acls#590
Conversation
📝 WalkthroughWalkthroughAdds LDAP support and group-based authorization, migrates session handling to repository-backed sessions (including ldap_groups), introduces LDAP group caching with TTL, renames provider identifiers ("username" → "local"/"ldap"), updates middleware/controllers/services/tests, and adds sqlc mapping, Makefile sql target, and a Delve debug config. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware as Context\r\nMiddleware
participant Repo as Repository\r\n(Session)
participant LdapSvc as LDAP\r\nService
participant AuthSvc as Auth\r\nService
participant Proxy as Proxy\r\nController
Client->>Middleware: Request (cookie or basic auth)
Middleware->>Repo: GetSessionCookie(sessionID)
Repo-->>Middleware: repository.Session {Provider: "ldap"/"local"/"oauth", UserDN, OAuthGroups}
alt Provider == "ldap"
Middleware->>LdapSvc: GetUserGroups(UserDN)
LdapSvc-->>Middleware: []groups
Middleware->>Middleware: Set UserContext.LdapGroups, IsBasicAuth as needed
else Provider == "oauth"
Middleware->>Middleware: Use OAuthGroups -> UserContext.OAuthGroups
else Provider == "local"
Middleware->>Middleware: Mark IsBasicAuth, no group enrichment
end
Middleware->>AuthSvc: IsInLdapGroup(UserContext, requiredGroups)
AuthSvc-->>Middleware: boolean
Middleware->>Proxy: Forward request + UserContext
alt authorized
Proxy-->>Client: 200 OK (forwarded)
else unauthorized (group mismatch)
Proxy-->>Client: 401 + GroupErr query param
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 #590 +/- ##
==========================================
- Coverage 20.25% 19.37% -0.89%
==========================================
Files 41 41
Lines 2389 2488 +99
==========================================
- Hits 484 482 -2
- Misses 1874 1972 +98
- Partials 31 34 +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 (3)
internal/controller/context_controller_test.go (1)
38-50: Inconsistent provider value in test data.The
userContext.Provideris still set to"username"(line 45), but the provider configuration now uses"local"as the ID (line 20). This inconsistency may cause test failures or mask bugs where provider matching is expected.Suggested fix
var userContext = config.UserContext{ Username: "testuser", Name: "testuser", Email: "test@example.com", IsLoggedIn: true, IsBasicAuth: false, OAuth: false, - Provider: "username", + Provider: "local", TotpPending: false, OAuthGroups: "", TotpEnabled: false, OAuthSub: "", }internal/controller/proxy_controller_test.go (2)
146-153: Session type migration looks correct, but provider value is inconsistent.The migration to
repository.Sessionis correct. However, theProvidervalue is still"username"(line 150), which is inconsistent with the new"local"naming convention used elsewhere in this PR.Suggested fix
err := authService.CreateSessionCookie(c, &repository.Session{ Username: "testuser", Name: "testuser", Email: "testuser@example.com", - Provider: "username", + Provider: "local", TotpPending: false, OAuthGroups: "", })
159-174: Provider value inconsistency in test context.The
Providervalue on line 167 is still"username", which should be updated to"local"for consistency with the provider renaming in this PR.Suggested fix
func(c *gin.Context) { c.Set("context", &config.UserContext{ Username: "testuser", Name: "testuser", Email: "testuser@example.com", IsLoggedIn: true, OAuth: false, - Provider: "username", + Provider: "local", TotpPending: false, OAuthGroups: "", TotpEnabled: false, }) c.Next() },
🤖 Fix all issues with AI agents
In @.zed/debug.json:
- Around line 8-13: The JSON contains trailing commas that make it invalid;
remove the commas after the "port": 4000 entry and any extraneous trailing
commas after the closing braces/array so the "tcp_connection" object (and the
surrounding objects/array) are properly closed with no trailing commas, ensuring
the file is valid JSON.
In `@internal/middleware/context_middleware.go`:
- Around line 60-92: Ensure the session cookie's Provider cannot be silently
overridden by m.auth.SearchUser results: require userSearch.Type to match
cookie.Provider (or invalidate the session) before proceeding; if they mismatch
call m.auth.DeleteSessionCookie(c) and goto basic. Use cookie.Provider as the
Provider value when constructing the config.UserContext, and only attempt
LDAP-specific work (calling m.auth.GetLdapUser and loading ldapGroups) when
cookie.Provider == "ldap"; keep the existing m.auth.RefreshSessionCookie(c)
behavior when the provider matches.
In `@internal/service/ldap_service.go`:
- Around line 150-157: GetUserGroups currently hardcodes groupOfUniqueNames and
uniquemember, pulls all groups and filters client-side, and parses names by
stripping "cn=" which breaks for other schemas; update LdapService.GetUserGroups
to accept configurable group objectClass and member attribute via
LdapServiceConfig (e.g., add fields GroupObjectClass and GroupMemberAttribute),
build a server-side search filter that matches membership (use the configured
member attribute with the provided userDN or username as appropriate), and
return group identifiers parsed robustly from the group DN or a chosen attribute
instead of naïve "cn=" stripping; ensure callers construct LdapService with the
new config fields and adjust parsing logic in GetUserGroups to handle AD
(member), groupOfUniqueNames (uniqueMember), and posixGroup (memberUid) formats.
🧹 Nitpick comments (2)
frontend/src/pages/login-page.tsx (1)
52-58: Use consistent strict equality operators.Line 53 uses
!==for the first comparison but!=for the second. For consistency and to avoid type coercion issues in JavaScript/TypeScript, use strict equality throughout.Suggested fix
const oauthProviders = providers.filter( - (provider) => provider.id !== "local" && provider.id != "ldap", + (provider) => provider.id !== "local" && provider.id !== "ldap", );internal/service/ldap_service.go (1)
177-186: Fragile DN parsing - consider using proper LDAP DN utilities.The current parsing assumes the group DN starts with lowercase
cn=. LDAP attribute names are case-insensitive, soCN=Admins,ou=groups,dc=example,dc=comwould not be handled correctly.Suggested fix using go-ldap DN parsing
- // Should work for most ldap providers? - groups := []string{} - - for _, groupDN := range groupDNs { - groupDN = strings.TrimPrefix(groupDN, "cn=") - parts := strings.SplitN(groupDN, ",", 2) - if len(parts) > 0 { - groups = append(groups, parts[0]) - } - } + groups := []string{} + + for _, groupDN := range groupDNs { + dn, err := ldapgo.ParseDN(groupDN) + if err != nil || len(dn.RDNs) == 0 || len(dn.RDNs[0].Attributes) == 0 { + continue + } + // Use the first RDN's value (typically the CN) + groups = append(groups, dn.RDNs[0].Attributes[0].Value) + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.zed/debug.jsonMakefilefrontend/src/pages/login-page.tsxinternal/bootstrap/app_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/service/auth_service.gointernal/service/ldap_service.gosqlc.yml
💤 Files with no reviewable changes (1)
- internal/controller/context_controller.go
🧰 Additional context used
🧬 Code graph analysis (9)
internal/bootstrap/app_bootstrap.go (1)
internal/controller/context_controller.go (1)
Provider(39-43)
internal/controller/user_controller_test.go (1)
internal/controller/context_controller.go (1)
Provider(39-43)
internal/controller/context_controller_test.go (2)
internal/utils/security_utils_test.go (1)
TestGetBasicAuth(58-76)internal/controller/context_controller.go (1)
Status(13-25)
internal/controller/proxy_controller.go (3)
internal/controller/context_controller.go (1)
Provider(39-43)internal/config/config.go (1)
App(186-194)internal/utils/label_utils.go (1)
SanitizeHeader(26-34)
internal/controller/oauth_controller.go (1)
internal/repository/models.go (1)
Session(7-19)
internal/controller/proxy_controller_test.go (1)
internal/repository/models.go (1)
Session(7-19)
internal/controller/user_controller.go (3)
internal/repository/models.go (1)
Session(7-19)internal/utils/string_utils.go (1)
Capitalize(7-12)internal/controller/context_controller.go (1)
Provider(39-43)
internal/service/auth_service.go (2)
internal/config/config.go (4)
UserSearch(146-149)LdapUser(141-144)SessionCookieName(11-11)UserContext(151-165)internal/repository/models.go (1)
Session(7-19)
internal/middleware/context_middleware.go (4)
internal/controller/context_controller.go (1)
Provider(39-43)internal/config/config.go (2)
App(186-194)UserContext(151-165)internal/utils/tlog/log_wrapper.go (1)
App(22-22)internal/utils/string_utils.go (1)
Capitalize(7-12)
🪛 Biome (2.1.2)
.zed/debug.json
[error] 11-11: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 12-12: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 12-13: Expected an array, an object, or a literal but instead found ']'.
Expected an array, an object, or a literal here.
(parse)
🪛 checkmake (0.2.2)
Makefile
[warning] 67-67: Missing required phony target "all"
(minphony)
[warning] 67-67: Missing required phony target "clean"
(minphony)
⏰ 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 (31)
Makefile (1)
66-69: LGTM!The new
sqltarget for sqlc code generation is correctly implemented and properly marked as.PHONY.sqlc.yml (1)
22-23: LGTM!The new override for
sessions.ldap_groupsis consistent with existing column overrides and aligns with theLdapGroupsfield added toUserContext.internal/service/ldap_service.go (1)
121-148: LGTM!The
GetUserDNmethod properly escapes the username to prevent LDAP injection and correctly handles the search with appropriate mutex synchronization.internal/config/config.go (3)
141-144: LGTM!The
LdapUserstruct is well-defined and appropriately encapsulates LDAP user data needed for authentication and group authorization.
151-165: LGTM!The additions of
IsBasicAuthandLdapGroupstoUserContextare consistent with the existing structure and follow the established pattern (e.g.,OAuthGroupsas a string for comma-separated groups).
210-213: LGTM!The
AppLDAPstruct follows the established pattern of other ACL configuration types in this file.internal/controller/context_controller_test.go (1)
19-20: LGTM!The provider naming is correctly updated to "Local"/"local" to align with the multi-provider authentication path introduced in this PR.
internal/controller/user_controller_test.go (3)
207-207: LGTM!Provider value correctly updated to
"local"to align with the multi-provider authentication refactor.
270-270: LGTM!Consistent with the provider renaming across the codebase.
293-293: LGTM!Provider value correctly updated.
internal/controller/proxy_controller_test.go (1)
188-210: LGTM!The test case for ensuring basic auth is disabled for TOTP-enabled users is well-structured. The
IsBasicAuth: trueandProvider: "basic"values correctly simulate the basic auth scenario being tested.internal/bootstrap/app_bootstrap.go (1)
147-161: LGTM!The multi-provider configuration logic is well-structured:
- Local provider is conditionally added when
LocalAuthConfigured()returns true.- LDAP provider is conditionally added when
LdapAuthConfigured()returns true.- Both are correctly marked as non-OAuth providers.
This cleanly supports the new LDAP group ACLs feature while maintaining backward compatibility with local authentication.
internal/controller/oauth_controller.go (3)
10-10: LGTM!Import addition necessary for the
repository.Sessiontype.
192-192: LGTM!Using
strings.Replacewith count1is a reasonable defensive choice, ensuring only the first@is replaced when deriving a pseudo username from the email address.
195-203: LGTM!The migration to
repository.Sessionis correct. The struct is properly initialized with the OAuth-specific fields (OAuthGroups,OAuthName,OAuthSub). TheUUID,Expiry, andCreatedAtfields are generated fresh byCreateSessionCookieinternally (not populated from the input object) and properly mapped intoCreateSessionParamsfor persistence.internal/controller/proxy_controller.go (3)
176-179: Basic-auth TOTP gate is correctly scoped.
Switching toIsBasicAuthkeeps the restriction limited to basic-auth flows while honoring TOTP.
215-238: Provider-aware group checks + GroupErr signaling look good.
The OAuth vs LDAP branching and group-error redirect flag align the authorization flow with the provider.
261-265: Remote-Groups header selection matches provider semantics.
LDAP groups for LDAP and OAuth groups for non-local providers keeps downstream headers consistent.internal/controller/user_controller.go (4)
8-9: Repository-based session import is consistent.
This aligns controller usage with the new session storage model.
115-121: TOTP-pending session now records provider cleanly.
Usingrepository.SessionwithProvider: "local"keeps the TOTP flow consistent.
141-150: Provider default/override logic is clear.
Defaulting tolocaland overriding toldapmakes the intent explicit.
255-260: Post‑TOTP session creation looks good.
The repository session usage mirrors the login flow and keeps provider consistent.internal/middleware/context_middleware.go (3)
48-55: TOTP-pending context now correctly tags local provider.
Keeps downstream logic consistent with session provenance.
169-177: Local basic-auth context now carriesIsBasicAuth.
This is consistent with the downstream basic-auth gate.
183-199: LDAP basic-auth context enrichment looks good.
Fetching LDAP groups and tagging the provider keeps auth decisions consistent.internal/service/auth_service.go (6)
72-85: LDAP DN lookup update is clean and consistent.
Returningunknownon LDAP search failure aligns with the caller expectations.
134-145: GetLdapUser helper is a good addition.
Keeps LDAP user detail retrieval in one place.
206-233: Session creation now aligns with repository.Session.
The mapping preserves existing fields and behavior.
316-362: Repository-based session retrieval looks consistent.
Return paths and expiry handling are updated cleanly to the new type.
365-371: Auth configuration helpers are straightforward.
Clear separation between local and LDAP configuration checks.
413-427: LDAP group check mirrors OAuth group logic well.
Early return on empty requirements keeps behavior predictable.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/service/auth_service.go`:
- Around line 143-172: In GetLdapUser, replace the initial
auth.ldapGroupsMutex.Lock()/Unlock() with a reader lock
(auth.ldapGroupsMutex.RLock()/RUnlock()) when checking ldapGroupsCache to avoid
taking a write lock for reads, and keep the existing write lock when populating
the cache; additionally add a nil check for auth.ldap before calling
auth.ldap.GetUserGroups (return a clear error if auth.ldap is nil) so the call
cannot panic when LDAP is not configured.
♻️ Duplicate comments (2)
internal/service/ldap_service.go (1)
148-187: Make LDAP group search schema configurable.Line 154 hardcodes
groupOfUniqueNames/uniquememberwith a DN-based match, which will fail for common schemas like Active Directory (group/member) or POSIX (posixGroup/memberUid). Consider making objectClass/member attribute (and the returned name attribute) configurable or accepting a configurable group search filter template.🛠️ Suggested direction (requires adding config fields)
- searchRequest := ldapgo.NewSearchRequest( - ldap.config.BaseDN, - ldapgo.ScopeWholeSubtree, ldapgo.NeverDerefAliases, 0, 0, false, - fmt.Sprintf("(&(objectclass=groupOfUniqueNames)(uniquemember=%s))", escapedUserDN), - []string{"dn"}, - nil, - ) + filter := fmt.Sprintf(ldap.config.GroupSearchFilter, escapedUserDN) + attrs := []string{"dn"} + if ldap.config.GroupNameAttribute != "" { + attrs = []string{ldap.config.GroupNameAttribute} + } + searchRequest := ldapgo.NewSearchRequest( + ldap.config.BaseDN, + ldapgo.ScopeWholeSubtree, ldapgo.NeverDerefAliases, 0, 0, false, + filter, + attrs, + nil, + )internal/middleware/context_middleware.go (1)
60-75: Allow basic-auth fallback after provider mismatch.On Line 70-75 the code deletes the cookie and returns, which skips the
basicpath. This blocks basic auth for clients with stale cookies. Consider falling through tobasiclike other invalid-cookie paths.🛠️ Suggested fix
if userSearch.Type != cookie.Provider { tlog.App.Warn().Msg("User type from session cookie does not match user search type") m.auth.DeleteSessionCookie(c) - c.Next() - return + goto basic }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/tinyauth/tinyauth.gointernal/bootstrap/service_bootstrap.gointernal/config/config.gointernal/middleware/context_middleware.gointernal/service/auth_service.gointernal/service/ldap_service.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/middleware/context_middleware.go (1)
internal/config/config.go (1)
UserContext(152-166)
internal/service/auth_service.go (5)
internal/service/ldap_service.go (1)
LdapService(26-31)internal/repository/db.go (1)
Queries(23-25)internal/config/config.go (2)
UserSearch(147-150)LdapUser(142-145)internal/repository/models.go (1)
Session(7-19)internal/utils/security_utils.go (1)
CheckFilter(77-102)
⏰ 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 (20)
internal/bootstrap/service_bootstrap.go (1)
59-71: LGTM: LDAP group cache TTL is wired into AuthServiceConfig.Nice, this keeps the TTL plumbed through initialization as expected.
cmd/tinyauth/tinyauth.go (2)
24-27: LGTM: clarified auth defaults.No functional impact; defaults remain sensible.
35-38: LGTM: default LDAP group cache TTL set to 15 minutes.Clear default and aligns with the new caching behavior.
internal/service/ldap_service.go (1)
119-146: LGTM: GetUserDN rename + injected filter remains safe.Clearer API name and still uses escaped input.
internal/config/config.go (5)
69-79: LGTM: GroupCacheTTL added to LDAP config.Clear, additive config change.
142-145: LGTM: LdapUser encapsulates DN + groups.Good clarity for LDAP user payloads.
152-166: LGTM: UserContext now carries IsBasicAuth + LdapGroups.Consistent with the new auth flows.
187-195: LGTM: App now includes LDAP ACL configuration.Nice extension for per-app LDAP rules.
211-213: LGTM: AppLDAP groups configuration added.Straightforward, additive change.
internal/middleware/context_middleware.go (3)
47-55: LGTM: TOTP-pending sessions set to local provider.Keeps provider semantics stable during TOTP flow.
174-184: LGTM: local basic-auth context now sets Provider + IsBasicAuth.Looks correct and consistent with the new context schema.
188-206: LGTM: LDAP basic-auth context includes groups + IsBasicAuth.Good propagation of LDAP group data.
internal/service/auth_service.go (8)
22-25: LGTM!The cache structure is well-designed with a clear expiration mechanism.
44-56: LGTM!The cache configuration and concurrent access protection with
ldapGroupsMutexare properly structured.
58-67: LGTM!Constructor properly initializes all new fields including the LDAP groups cache map.
81-95: LGTM!The change from
SearchtoGetUserDNand returning"unknown"on error (instead of"error") is a reasonable approach—it treats LDAP lookup failures as "user not found" while still logging the actual error for debugging.
233-271: LGTM!The migration from
config.SessionCookietorepository.Sessionis properly implemented with all required fields correctly mapped.
343-390: LGTM!The return type migration to
repository.Sessionis properly implemented with appropriate error handling and idiomatic Go zero-value returns on error paths.
392-398: LGTM!Clean and straightforward configuration checks.
440-454: LGTM!The implementation correctly mirrors
IsInOAuthGroupand properly usesutils.CheckFilterfor flexible group matching (supporting comma-separated lists and regex patterns).
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
1 participant
Summary by CodeRabbit
New Features
Changes
✏️ Tip: You can customize this high-level summary in your review settings.