feat: use logged-in user credentials for all LDAP operations#430
Merged
feat: use logged-in user credentials for all LDAP operations#430
Conversation
Summary of ChangesHello @CybotTM, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the application's LDAP interaction model to enhance security and flexibility. By shifting from a primary readonly service account to using the logged-in user's credentials for most LDAP operations, it resolves access permission issues for write actions and enables granular per-user access control for reads. Furthermore, it introduces the option to run the application without a dedicated service account, simplifying deployment for environments that prefer direct user authentication and dynamic LDAP interactions. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant architectural change to use the logged-in user's credentials for LDAP operations, fixing an "Insufficient Access Rights" bug and correctly enforcing per-user ACLs. However, a critical security risk has been identified: the storage of plaintext user passwords in the server-side session, which can lead to password exposure if the session store is compromised, especially with persistent storage. Additionally, there's a logic bug in the indexHandler's fallback mechanism that could impact performance and mask errors. The overall implementation is well-structured, with extensive changes across authentication and data handling, and the introduction of ...FromData functions is a clean approach.
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
There was a problem hiding this comment.
Walkthrough
This PR introduces support for operating without a service account by implementing per-user LDAP authentication via UPN bind. The application now supports two modes: traditional service account with background cache and connection pooling, or per-user credentials where each request uses the authenticated user's LDAP credentials. Configuration validation was relaxed to make readonly credentials optional. All web handlers were refactored to obtain user-specific LDAP connections instead of relying on shared cache. New utility functions enable relationship population from explicit data slices. Health endpoints gracefully handle both operational modes. Test coverage was expanded with unit tests for helper functions and service-account-less scenarios.
Changes
| File(s) | Summary |
|---|---|
internal/options/app.gointernal/options/parse_test.go |
Made readonly-user and readonly-password configuration parameters optional instead of mandatory; removed validation requirements and associated test cases; added comments explaining fallback to per-user credentials when service account is not configured. |
internal/web/auth.go |
Implemented UPN (User Principal Name) bind authentication for Active Directory; added authenticateUser() method to route between service account and UPN bind flows; introduced authenticateViaUPNBind() for temporary LDAP connections using user credentials; added domainFromBaseDN() helper to extract DNS domain from LDAP BaseDN; modified login handler to store username and password in session. |
internal/web/server.go |
Refactored to support optional service account configuration; made readonly LDAP client and background cache conditional; introduced getUserLDAP() method for creating per-user LDAP clients from session credentials; updated indexHandler() to use per-user credentials; added nil-checks for ldapCache and ldapReadonly throughout lifecycle methods; added findByDN() helper function. |
internal/web/users.gointernal/web/groups.gointernal/web/computers.go |
Refactored all handlers to use per-user LDAP connections via getUserLDAP() instead of shared cache; extracted helper functions for loading data, filtering, and finding entities by DN; added proper resource cleanup with deferred Close() calls; implemented nil-checks for optional cache invalidation; removed extensive documentation comments. |
internal/ldap_cache/manager.go |
Added three new public utility functions (PopulateGroupsForUserFromData, PopulateUsersForGroupFromData, PopulateGroupsForComputerFromData) for populating LDAP entity relationships from explicit data slices without cache dependency; includes DN-based map optimization for O(1) lookups and disabled user filtering support. |
internal/web/health.gointernal/web/health_test.go |
Added graceful handling for scenarios without service account across health, readiness, and liveness endpoints; returns simplified status indicating 'per-user credentials' mode; added comprehensive test coverage with setupHealthTestAppNoServiceAccount() helper and three new test functions validating endpoint behavior without service account. |
internal/web/handlers_test.go |
Replaced integration-style tests with unit tests for standalone filter functions; added comprehensive test coverage for helper functions including TestDomainFromBaseDN (7 test cases), TestFindByDN, TestFindGroupByDN, and TestFindComputerByDN; added ldapConfig field initialization in setupTestApp(); removed verbose comments while preserving test logic. |
Sequence Diagram
This diagram shows the interactions between components:
sequenceDiagram
participant Caller
participant PopulateGroupsForUserFromData
participant PopulateUsersForGroupFromData
participant PopulateGroupsForComputerFromData
participant FullLDAPUser
participant FullLDAPGroup
participant FullLDAPComputer
Note over Caller: Three new data-driven population functions
rect rgb(200, 220, 250)
Note over Caller,FullLDAPUser: Populate User Groups Flow
Caller->>PopulateGroupsForUserFromData: user, allGroups[]
activate PopulateGroupsForUserFromData
PopulateGroupsForUserFromData->>FullLDAPUser: Create empty FullLDAPUser
PopulateGroupsForUserFromData->>PopulateGroupsForUserFromData: Get user DN
loop For each group in allGroups
loop For each memberDN in group.Members
alt memberDN matches userDN
PopulateGroupsForUserFromData->>FullLDAPUser: Append group to Groups[]
end
end
end
PopulateGroupsForUserFromData-->>Caller: Return FullLDAPUser with groups
deactivate PopulateGroupsForUserFromData
end
rect rgb(220, 250, 220)
Note over Caller,FullLDAPGroup: Populate Group Members Flow
Caller->>PopulateUsersForGroupFromData: group, allUsers[], allGroups[], showDisabled
activate PopulateUsersForGroupFromData
PopulateUsersForGroupFromData->>FullLDAPGroup: Create empty FullLDAPGroup
PopulateUsersForGroupFromData->>PopulateUsersForGroupFromData: Build usersByDN map for O(1) lookup
loop For each memberDN in group.Members
PopulateUsersForGroupFromData->>PopulateUsersForGroupFromData: Lookup user by DN in map
alt User found in map
alt showDisabled OR user.Enabled
PopulateUsersForGroupFromData->>FullLDAPGroup: Append user to Members[]
end
end
end
PopulateUsersForGroupFromData->>PopulateUsersForGroupFromData: Build groupsByDN map for O(1) lookup
loop For each parentDN in group.MemberOf
PopulateUsersForGroupFromData->>PopulateUsersForGroupFromData: Lookup parent group by DN in map
alt Parent group found in map
PopulateUsersForGroupFromData->>FullLDAPGroup: Append parent to ParentGroups[]
end
end
PopulateUsersForGroupFromData-->>Caller: Return FullLDAPGroup with members and parents
deactivate PopulateUsersForGroupFromData
end
rect rgb(250, 220, 220)
Note over Caller,FullLDAPComputer: Populate Computer Groups Flow
Caller->>PopulateGroupsForComputerFromData: computer, allGroups[]
activate PopulateGroupsForComputerFromData
PopulateGroupsForComputerFromData->>FullLDAPComputer: Create empty FullLDAPComputer
PopulateGroupsForComputerFromData->>PopulateGroupsForComputerFromData: Get computer DN
loop For each group in allGroups
loop For each memberDN in group.Members
alt memberDN matches computerDN
PopulateGroupsForComputerFromData->>FullLDAPComputer: Append group to Groups[]
end
end
end
PopulateGroupsForComputerFromData-->>Caller: Return FullLDAPComputer with groups
deactivate PopulateGroupsForComputerFromData
end
Note over Caller: All functions use DN matching<br/>to establish relationships
🔗 Cross-Repository Impact Analysis
Enable automatic detection of breaking changes across your dependent repositories. → Set up now
Learn more about Cross-Repository Analysis
What It Does
- Automatically identifies repositories that depend on this code
- Analyzes potential breaking changes across your entire codebase
- Provides risk assessment before merging to prevent cross-repo issues
How to Enable
- Visit Settings → Code Management
- Configure repository dependencies
- Future PRs will automatically include cross-repo impact analysis!
Benefits
- 🛡️ Prevent breaking changes across repositories
- 🔍 Catch integration issues before they reach production
- 📊 Better visibility into your multi-repo architecture
Install the extension
Note for Windsurf
Please change the default marketplace provider to the following in the windsurf settings:Marketplace Extension Gallery Service URL: https://marketplace.visualstudio.com/_apis/public/gallery
Marketplace Gallery Item URL: https://marketplace.visualstudio.com/items
Entelligence.ai can learn from your feedback. Simply add 👍 / 👎 emojis to teach it your preferences. More shortcuts below
Emoji Descriptions:
⚠️ Potential Issue - May require further investigation.- 🔒 Security Vulnerability - Fix to ensure system safety.
- 💻 Code Improvement - Suggestions to enhance code quality.
- 🔨 Refactor Suggestion - Recommendations for restructuring code.
- ℹ️ Others - General comments and information.
Interact with the Bot:
- Send a message or request using the format:
@entelligenceai + *your message*
Example: @entelligenceai Can you suggest improvements for this code?
- Help the Bot learn by providing feedback on its responses.
@entelligenceai + *feedback*
Example: @entelligenceai Do not comment on `save_auth` function !
Also you can trigger various commands with the bot by doing
@entelligenceai command
The current supported commands are
config- shows the current configretrigger_review- retriggers the review
More commands to be added soon.
CybotTM
force-pushed
the
feat/user-bound-ldap-credentials
branch
from
b622210 to
9d6f380
Compare
The login page had a "Powered by netresearch/ldap-manager" footer but all authenticated pages were missing it. Add a matching footer with GitHub link and version info to the logged-in layout.
Fix "LDAP Result Code 50 Insufficient Access Rights" when modifying groups/users by using the authenticated user's own LDAP connection instead of the readonly service account. All interactive LDAP operations (reads and writes) now use per-user credentials stored in the server-side session. The readonly service account becomes optional — when not configured, login authenticates via AD UPN bind (user@domain) and background cache is disabled.
- Fix errcheck: use deferred closure for Close() calls - Fix lll: break long function signatures - Fix revive: rename memberDNs to memberDNS per Go conventions - Fix dupl: extract assertNoServiceAccountStatusEndpoint helper - Fix unparam: suppress for intentional utility function - Fix indexHandler fallback: fail fast on real errors, only fall back on ErrUserNotFound - Refactor cache Populate* methods to delegate to FromData functions, eliminating code duplication
- Set proper HTTP status codes in handle500 (500) and fourOhFourHandler (404) - Redirect to login on LDAP bind failure (expired credentials) - Sanitize error messages in modify handler flash responses - Fix readinessHandler to report actual pool health status - Return rate-limit errors as HTML login page instead of plain text - Return Fiber shutdown error from Shutdown() instead of swallowing - Remove dead code: Invalidate, InvalidateByPath, CacheMiddleware - Merge duplicate invalidation functions into single method - URL-encode DNs in redirect paths to prevent injection - Move findUserByDN to users.go for better code organization - Fix TemplateCache.Get() to use RLock for concurrent readers - Remove unused accessedAt field (eviction uses createdAt) - Regenerate session ID after login to prevent session fixation - Validate username chars and domain in UPN bind authentication - Fix Cache.Get() to return a copy preventing data races - Use atomic.Bool for warmupComplete in cache manager - Add graceful shutdown for periodicCacheLogging goroutine - Document per-user credential operating mode in configuration guide Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de> Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
- Fix variable naming: memberDNs→memberDNS, memberGroupDNs→memberGroupDNS - Extract health status string constants (goconst) - Fix gofumpt formatting (trailing newlines, struct alignment) Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de> Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de> Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
CybotTM
force-pushed
the
feat/user-bound-ldap-credentials
branch
from
b484e32 to
8c1e42f
Compare
6 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 5, 2026
## Summary Addresses critical bugs, security issues, and code quality findings from 10 independent code reviews of the per-user LDAP credentials feature (#430). ### Bug Fixes - **Slice mutation during iteration** in `OnRemoveUserFromGroup` — added missing `break` after removal - **Cache pointer escape** — `FindByDN`/`FindBySAMAccountName`/`Find` now return value copies instead of pointers into cache internals - **CSRF token cached in HTML** — detail pages (user/group/computer) excluded from template cache, rendered directly - **`TemplateCache.Stop()` double-close panic** — guarded with `sync.Once` - **`RenderWithCache` ignoring request context** — switched from `context.Background()` to `c.UserContext()` - **`handle500` recursion risk** — rewritten to handle all Fiber error codes, prevent template render recursion, hide internal errors from users - **Rate limiter status code** — fixed middleware response chain ### Security - **Username validation** — added LDAP filter metacharacters (`*`, `(`, `)`, null byte) to rejection list - **Error message leakage** — `handle500` now shows generic message instead of internal error details ### Code Quality - **Dead code removal** — `OptionalAuth`, `GetCachedManifest`, `manifestCache`, `manifestMutex`, `App.logger` - **`createCSRFConfig`** — returns `fiber.Handler` directly instead of `*fiber.Handler` - **Shutdown ordering** — drain Fiber first, then stop caches, then close connections - **`Filter` pre-allocation** — pre-allocates result slice capacity - **Mock thread safety** — added `sync.Mutex` to test helper call count tracking ## Test plan - [x] All existing tests pass - [x] `go vet` clean - [x] `gofumpt` formatting applied - [x] Race detector clean (`-race` on ldap_cache package) - [x] Prettier check passes - [ ] CI pipeline passes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
LDAP_READONLY_USER/LDAP_READONLY_PASSWORDare no longer required — when omitted, login uses AD UPN bind (user@domain) and background cache is disabledChanges
internal/options/app.gointernal/web/auth.gointernal/web/server.gogetUserLDAP()helper; conditional cache initinternal/web/groups.gointernal/web/users.gointernal/web/computers.gointernal/web/health.gointernal/ldap_cache/manager.goSecurity
Password is stored in server-side session (memory or bbolt). Only the session ID cookie reaches the browser. This is the standard pattern for LDAP web apps (phpLDAPadmin, FusionDirectory, etc.).
Test plan
go build ./...compilesgo test ./...passes (all packages)go vet ./...clean