feat: add generic LDAP authentication strategy using ldapts#1496
feat: add generic LDAP authentication strategy using ldapts#14961saac-k wants to merge 7 commits into
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
|
Since this work was done outside of my company, I changed the commit email to my personal email and added GPG signatures to sign the CLA as an individual contributor. |
There was a problem hiding this comment.
Requesting changes for the two required fixes in the inline comments:
- LDAP group lookup errors are currently swallowed as “not a member”.
- Missing LDAP email falls back to an empty string, which can break user upserts with the file DB unique email index.
The remaining inline comments are nits.
|
@dcoric Thank you for the detailed review. I'm a bit tied up right now, but I'll take a look within the next few days! |
Add ldapts (v8.1.7) for modern LDAP client support and passport-custom (v1.1.1) for custom Passport strategy creation. Signed-off-by: Kwangjin Ko <kyet@me.com>
Add LDAP auth type definition to config.schema.json and generated TypeScript types with LdapConfig interface. Signed-off-by: Kwangjin Ko <kyet@me.com>
Add disabled ldap authentication entry with sensible defaults for attribute mappings, and group settings. Signed-off-by: Kwangjin Ko <kyet@me.com>
|
I addressed all review comments and rebased onto the latest main branch. Most of the changes were squashed into the existing commits, and one additional commit (fb0c588) was added. |
Add new LDAP authentication strategy that uses ldapts for LDAP operations and passport-custom for Passport integration. The authentication flow: 1. Bind with service account 2. Search for user entry 3. Check group memberships (user/admin) 4. Verify user password via user bind 5. Sync user profile to database Signed-off-by: Kwangjin Ko <kyet@me.com>
Add ldap module to passport strategy registry and include it in the list of username/password login strategies. Signed-off-by: Kwangjin Ko <kyet@me.com>
Test cases cover: successful auth with admin/non-admin roles, user not found, user group rejection, invalid password, connection errors, multiple entries in search result, missing credentials, and escapeFilterValue with normal strings, LDAP injection attempts, and RFC 4515 special characters. Signed-off-by: Kwangjin Ko <kyet@me.com>
Signed-off-by: Kwangjin Ko <kyet@me.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1496 +/- ##
==========================================
- Coverage 90.21% 89.92% -0.30%
==========================================
Files 69 70 +1
Lines 5511 5728 +217
Branches 944 996 +52
==========================================
+ Hits 4972 5151 +179
- Misses 521 559 +38
Partials 18 18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This PR is still alive. I'm not trying to rush the maintainer; I'm just letting you know that this PR is not stale. |
jescalada
left a comment
jescalada
left a comment
There was a problem hiding this comment.
@1saac-k Thanks for the contribution! 🚀
Would you mind adding a short demo or some screenshots showing a working LDAP auth flow, and the resulting database entries when creating users? We want to make sure that database entries are not malformed: this might happen if LDAP gives an array of strings for the user.email among other fields.
| // Step 6: Check optional admin group membership | ||
| let isAdmin = false; | ||
| try { | ||
| isAdmin = await isUserInGroup(client, ldapConfig, userDN, ldapConfig.adminGroupDN, username); |
There was a problem hiding this comment.
Is the admin group membership check optional? If so, we wouldn't want to make adminGroupDN a required field 🤔
| // Step 7: Extract profile attributes and sync to database | ||
| const userObj = { | ||
| username: String(entry[usernameAttr] || username).toLowerCase(), | ||
| email: String(email).toLowerCase(), |
There was a problem hiding this comment.
Should we be handling potential array values in here (and other fields, if relevant)? I don't know how emails are typically stored in LDAP, but if array values are allowed to coexist with strings, this would add invalid emails on calling db.updateUser later.
Note that GitProxy doesn't currently support multiple emails (#1210), you'd have to either implement support for this, or just select the first entry in case entry.mail is an array of strings.
There was a problem hiding this comment.
Another thing to note: if you're aware of other fields being multi-valued, we ought to have a helper function that preprocesses them so we can write valid database entries 👍🏼
It could be as simple as:
Array.isArray(value) ? value[0] : valueAlthough this brings up the question of whether arrays in LDAP are ordered or not and what the default ordering might be...
|
|
||
| const createClient = (ldapConfig: LDAPConfig): Client => { | ||
| return new Client({ | ||
| url: ldapConfig.url, |
There was a problem hiding this comment.
Just a comment after scanning the PR with AI (please verify for accuracy before implementing):
Should we throw an error here if starttls is disabled but the URL starts with ldap://? If starttls is disabled, the verifyPassword function below would send the user/password in plaintext when binding.
| groupDN: string, | ||
| username: string, | ||
| ): Promise<boolean> => { | ||
| const groupFilter = (ldapConfig.groupSearchFilter || '(member={{dn}})') |
There was a problem hiding this comment.
Another AI-driven review comment:
Check if groupSearchFilter is wrapped in parentheses, and add them if not so. (Otherwise we should document the requirement so we don't end up with malformed filters)
| return done(null, user || false); | ||
| } catch (error: unknown) { | ||
| const message = handleErrorAndLog(error, 'LDAP authentication error'); | ||
| return done(message); |
There was a problem hiding this comment.
passport has its own way of handling errors, we should check if calling done(error) instead of done(message) handles this better or more efficiently (worst case being that it silences the error for some reason).
I'm aware that we're already doing this in src/service/passport/activeDirectory.ts - however if passing the error is the right choice, we should also fix the calls in that file.
Thank you for taking the time to review this.
I will review and address the items you pointed out as soon as possible! |
3 participants
Closes #1488
Add a new
ldapauthentication type that integrates with any standards-compliant LDAP server. The existingactivedirectorytype relies on theactivedirectory2library which is AD-specific and incompatible with lightweightLDAP servers (e.g. lldap). This PR introduces a generic LDAP strategy built on
ldaptsand
passport-custom, lowering the barrier to entry for teams that want simple, manageable user authentication withoutsetting up a full Active Directory.
This implementation is not
lldap-specific — it supports any standards-compliant LDAP server, which means it should also work with Active Directory. As a result, it may be possible in the future to replace the deprecatedactivedirectory2andldapjsdependencies withldapts.Testing