Skip to content

Add knowage-enhanced-ldap-security-provider module#977

Open
egwada wants to merge 5 commits intoKnowageLabs:masterfrom
egwada:enhance-ldap-security-provider
Open

Add knowage-enhanced-ldap-security-provider module#977
egwada wants to merge 5 commits intoKnowageLabs:masterfrom
egwada:enhance-ldap-security-provider

Conversation

@egwada
Copy link
Copy Markdown

@egwada egwada commented Mar 30, 2026

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/)

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>
@github-actions
Copy link
Copy Markdown
Contributor

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request

Copy link
Copy Markdown
Contributor

@eng-dedalus eng-dedalus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

egwada and others added 4 commits April 17, 2026 10:16
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants