Add knowage-enhanced-ldap-security-provider module#977
Add knowage-enhanced-ldap-security-provider module#977egwada wants to merge 5 commits intoKnowageLabs:masterfrom
Conversation
Fixes three critical bugs in the existing knowageldapsecurityprovider for multi-OU Active Directory environments: - bindWithCredentials corrupted service account DN on subtree search - DN_POSTFIX conflated search base with DN suffix, blocking multi-OU search - FullLdapSecurityServiceSupplier built user DN by concatenation instead of subtree search (SUBTREE_SCOPE from BASE_DN) New features: - Automatic user provisioning on first LDAP login (AUTO_CREATE_USER) - Role sync: LDAP groups matching Knowage role names are persisted in SBI_EXT_USER_ROLES at each login (visible in admin UI) - Encrypted password support via ENC(...) wrapper - Fallback to internal authentication when LDAP is unavailable - Docker OpenLDAP test environment (docker-compose.test-ldap.yml + test-ldap/) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
CLA Assistant Lite bot: I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request |
eng-dedalus
left a comment
There was a problem hiding this comment.
Hi @egwada ,
first of all, thank you very much for this contribution and for the work you put into it — we really appreciate the effort behind this PR.
We reviewed the proposal and we would like to clarify one important point to make sure we are understanding the intended flow correctly.
In the current codebase, we already support two different LDAP-related scenarios:
LDAP for authentication only, while authorization/user management stays in Knowage.
In this case, the user must already exist in the Knowage DB, and LDAP is only used to validate credentials.
Full LDAP flow, where the user profile is built directly from LDAP data.
In this case, the user is not expected to be provisioned/persisted in the Knowage DB as part of the login flow.
From your PR description, it seems that the new module introduces automatic user provisioning when an LDAP-authenticated user does not already exist in the Knowage DB, and also synchronizes LDAP roles into the application DB.
Our doubt is about how this design is meant to fit with the second scenario above, i.e. the full LDAP flow.
In particular, it seems that your implementation introduces persistence/synchronization into the Knowage DB as part of the authentication flow, while our current full LDAP approach does not rely on that model.
Could you please clarify whether we are interpreting your proposal correctly?
More specifically:
is the automatic provisioning of users into the Knowage DB intended to be mandatory in your approach?
are LDAP roles always meant to be synchronized into the DB?
how should this module behave in a scenario where LDAP is expected to handle both authentication and roles/groups without persisting the user in the application DB?
Thanks again for the contribution — it is genuinely appreciated.
…TDD)
Bug 1 — ClassCastException on byte[] memberOf (non-ASCII DN):
getUserGroups() hard-cast values.next() to String; JNDI returns byte[] when
the DN contains non-ASCII chars → ClassCastException silently swallowed by the
NamingException catch block → empty group list → access denied.
Fix: check instanceof byte[] and decode with StandardCharsets.UTF_8.
Bug 2 — PartialResultException on AD subtree search:
createContext() had no REFERRAL policy; Active Directory returns LDAP
referrals on subtree search from root → PartialResultException.
Fix: env.put(Context.REFERRAL, "ignore").
Bug 3 — extractCnFromDn() fragile on RFC 4514 escaped commas:
split(",") breaks on "CN=Smith\, John,...".
Fix: scan character by character, skip backslash-escaped chars, break on
first unescaped comma.
Also: getSingleAttributeValue() now handles byte[] values (UTF-8 decode).
Tests: 16 JUnit 5 unit tests in EnhancedLdapConnectorTest covering all three
bugs and edge cases (mixed String/byte[], missing attribute, ACCESS_GROUP_FILTER,
non-ASCII groups). Docker LDAP stack updated with non-ASCII group "Accès Knowage"
(bootstrap.ldif) and integration tests 14-16 added (run-tests.sh,
TestKnowageLdapAuth.java).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaces the hardcoded Context.REFERRAL="ignore" with a configurable parameter LDAP_REFERRAL in enhanced_ldap.properties. Accepted values: ignore (default), follow, throw. Invalid values fall back to "ignore" with a warning. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1. LDAP read timeout (EnhancedLdapConnector): Add com.sun.jndi.ldap.read.timeout=10000ms to JNDI context. Previously only connect timeout was set; a server that accepted the connection but hung mid-query would block a Tomcat thread indefinitely. 2. Role revocation on re-login (EnhancedLdapSecurityServiceSupplier): resolveRoles() now computes a diff between DB roles and current LDAP groups. Roles present in DB but absent from LDAP are deleted via the new deleteSbiUserRoleById() DAO method. Previously a user removed from an AD group kept the corresponding Knowage role until manual cleanup. New DAO method: ISbiUserDAO.deleteSbiUserRoleById(userId, roleId) implemented in SbiUserDAOHibImpl following the same pattern as deleteSbiUserAttributeById(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…imeouts - Section 1: add new features (role revocation, non-ASCII support, referral policy, timeouts) - Section 5.1: add LDAP_REFERRAL parameter with documentation - Section 8: note role revocation in sync step - Section 9: add PartialResultException and role revocation troubleshooting entries - Section 10: add LDAP_REFERRAL to parameter reference table Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fixes three critical bugs in the existing knowageldapsecurityprovider for multi-OU Active Directory environments:
New features: