fix(ldap): pass through LDAP mail attribute instead of crafting email#834
fix(ldap): pass through LDAP mail attribute instead of crafting email#834itasli wants to merge 3 commits intotinyauthapp:mainfrom
Conversation
TinyAuth was constructing LDAP user emails as username@CookieDomain instead of using the mail attribute stored in the directory. This caused OIDC clients like Grafana to receive a synthetic email rather than the real one. Rename GetUserDN to GetUserInfo and extend it to also fetch the mail attribute in the same LDAP query. Thread the result through UserSearch and use it in both the login flow and the basic auth middleware, falling back to the crafted email only when LDAP returns no mail value. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dosubot
Bot
added
the
size:S
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughLDAP authentication now retrieves the user's email address from the LDAP directory via a new ChangesLDAP Email Retrieval and Threading
Possibly related PRs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/service/ldap_service.go (1)
146-171: ⚡ Quick winConsider making the mail attribute name configurable.
"mail"is the RFC 2256 / inetOrgPerson standard and covers OpenLDAP and Active Directory. However, some deployments use"mailPrimaryAddress","email", or other names. Users on those schemas will silently fall back to the syntheticusername@domainemail and get no benefit from this fix.Adding a field to
LdapConfigwith"mail"as the default keeps backward compatibility and makes the feature complete for all LDAP setups.💡 Suggested approach
In
internal/config/config.go, extendLdapConfig:type LdapConfig struct { ... GroupCacheTTL int `description:"Cache duration for LDAP group membership in seconds." yaml:"groupCacheTTL"` + MailAttribute string `description:"LDAP attribute name for the user's email address." yaml:"mailAttribute"` }In
NewDefaultConfiguration():Ldap: LdapConfig{ Insecure: false, SearchFilter: "(uid=%s)", GroupCacheTTL: 900, + MailAttribute: "mail", },In
LdapServiceConfigand whereverLdapServiceis initialised, thread the field through, then inGetUserInfo:- []string{"dn", "mail"}, + []string{"dn", ldap.config.MailAttribute}, ... - return entry.DN, entry.GetAttributeValue("mail"), nil + return entry.DN, entry.GetAttributeValue(ldap.config.MailAttribute), nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/ldap_service.go` around lines 146 - 171, Add a configurable mail attribute to LDAP config and use it in GetUserInfo: add a MailAttribute string field to LdapConfig with default "mail" in NewDefaultConfiguration, propagate that field through LdapServiceConfig and into LdapService initialization, and then replace the hard-coded "mail" in LdapService.GetUserInfo with ldap.config.MailAttribute (falling back to "mail" if empty) so different schemas like "mailPrimaryAddress" or "email" are supported.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/service/ldap_service.go`:
- Around line 146-171: Add a configurable mail attribute to LDAP config and use
it in GetUserInfo: add a MailAttribute string field to LdapConfig with default
"mail" in NewDefaultConfiguration, propagate that field through
LdapServiceConfig and into LdapService initialization, and then replace the
hard-coded "mail" in LdapService.GetUserInfo with ldap.config.MailAttribute
(falling back to "mail" if empty) so different schemas like "mailPrimaryAddress"
or "email" are supported.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7751862c-66ae-403b-97a9-141e35b899a6
📒 Files selected for processing (5)
internal/config/config.gointernal/controller/user_controller.gointernal/middleware/context_middleware.gointernal/service/auth_service.gointernal/service/ldap_service.go
dosubot
Bot
added
size:M
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
2 participants
TinyAuth was constructing LDAP user emails as username@CookieDomain instead of using the mail attribute stored in the directory. This caused OIDC clients like Grafana to receive a synthetic email rather than the real one.
Rename GetUserDN to GetUserInfo and extend it to also fetch the mail attribute in the same LDAP query. Thread the result through UserSearch and use it in both the login flow and the basic auth middleware, falling back to the crafted email only when LDAP returns no mail value.
Summary by CodeRabbit