From 8a63f0368e5573014739d4c31ea9c2d53ac00701 Mon Sep 17 00:00:00 2001 From: Girish Ramakrishnan Date: Tue, 6 Feb 2024 16:43:05 +0100 Subject: [PATCH] Fix parsing of displayName Currently, we only have one field for the name. The first part is first name. The rest is last name. Obviously, this won't work in all cases but is the best we can do for the moment. --- CHANGES | 1 + src/directoryserver.js | 11 ++++------- src/ldapserver.js | 12 ++++-------- src/oidc.js | 6 +++--- src/routes/test/users-test.js | 1 - src/test/users-test.js | 14 ++++++++++++++ src/users.js | 14 ++++++++++++++ 7 files changed, 40 insertions(+), 19 deletions(-) diff --git a/CHANGES b/CHANGES index be7828599..748d5aa7b 100644 --- a/CHANGES +++ b/CHANGES @@ -2741,4 +2741,5 @@ * OIDC avatar support via picture claim * backupcleaner: fix bug where preserved backups were removed incorrectly * directoryserver: cloudflare warning +* oidc/ldap: fix display name parsing to send anything after first name as the last name diff --git a/src/directoryserver.js b/src/directoryserver.js index ac2da55a5..4502eff45 100644 --- a/src/directoryserver.js +++ b/src/directoryserver.js @@ -218,10 +218,9 @@ async function userSearch(req, res, next) { const dn = ldap.parseDN(`cn=${user.id},ou=users,dc=cloudron`); const displayName = user.displayName || user.username || ''; // displayName can be empty and username can be null - const nameParts = displayName.split(' '); - const firstName = nameParts[0]; - const lastName = nameParts.length > 1 ? nameParts[nameParts.length - 1] : ''; // choose last part, if it exists + const { firstName, lastName, middleName } = users.parseDisplayName(displayName); + // https://datatracker.ietf.org/doc/html/rfc2798 const obj = { dn: dn.toString(), attributes: { @@ -234,6 +233,8 @@ async function userSearch(req, res, next) { mailAlternateAddress: user.fallbackEmail, displayname: displayName, givenName: firstName, + sn: lastName, + middleName: middleName, username: user.username, samaccountname: user.username, // to support ActiveDirectory clients memberof: allGroups.filter(function (g) { return g.userIds.indexOf(user.id) !== -1; }).map(function (g) { return `cn=${g.name},ou=groups,dc=cloudron`; }) @@ -242,10 +243,6 @@ async function userSearch(req, res, next) { if (user.twoFactorAuthenticationEnabled) obj.attributes.twoFactorAuthenticationEnabled = true; - // http://www.zytrax.com/books/ldap/ape/core-schema.html#sn has 'name' as SUP which is a DirectoryString - // which is required to have atleast one character if present - if (lastName.length !== 0) obj.attributes.sn = lastName; - // ensure all filter values are also lowercase const lowerCaseFilter = safe(function () { return ldap.parseFilter(req.filter.toString().toLowerCase()); }, null); if (!lowerCaseFilter) return next(new ldap.OperationsError(safe.error.message)); diff --git a/src/ldapserver.js b/src/ldapserver.js index 8f79b1c57..b929caf13 100644 --- a/src/ldapserver.js +++ b/src/ldapserver.js @@ -161,10 +161,9 @@ async function userSearch(req, res, next) { const dn = ldap.parseDN('cn=' + user.id + ',ou=users,dc=cloudron'); const displayName = user.displayName || user.username || ''; // displayName can be empty and username can be null - const nameParts = displayName.split(' '); - const firstName = nameParts[0]; - const lastName = nameParts.length > 1 ? nameParts[nameParts.length - 1] : ''; // choose last part, if it exists + const { firstName, lastName, middleName } = users.parseDisplayName(displayName); + // https://datatracker.ietf.org/doc/html/rfc2798 const obj = { dn: dn.toString(), attributes: { @@ -177,17 +176,14 @@ async function userSearch(req, res, next) { mailAlternateAddress: user.fallbackEmail, displayname: displayName, givenName: firstName, + sn: lastName, + middleName: middleName, username: user.username, samaccountname: user.username, // to support ActiveDirectory clients memberof: allGroups.filter(function (g) { return g.userIds.indexOf(user.id) !== -1; }).map(function (g) { return `cn=${g.name},ou=groups,dc=cloudron`; }) } }; - // http://www.zytrax.com/books/ldap/ape/core-schema.html#sn has 'name' as SUP which is a DirectoryString - // which is required to have atleast one character if present - if (lastName.length !== 0) obj.attributes.sn = lastName; - - // ensure all filter values are also lowercase const lowerCaseFilter = safe(function () { return ldap.parseFilter(req.filter.toString().toLowerCase()); }, null); if (!lowerCaseFilter) return next(new ldap.OperationsError(safe.error.message)); diff --git a/src/oidc.js b/src/oidc.js index 54d99b3a4..068187aae 100644 --- a/src/oidc.js +++ b/src/oidc.js @@ -648,17 +648,17 @@ async function claims(userId/*, use, scope*/) { if (error) return { error: 'user not found' }; const displayName = user.displayName || user.username || ''; // displayName can be empty and username can be null - const nameParts = displayName.split(' '); - const firstName = nameParts[0]; - const lastName = nameParts.length > 1 ? nameParts[nameParts.length - 1] : ''; // choose last part, if it exists + const { firstName, lastName, middleName } = users.parseDisplayName(displayName); const { fqdn:dashboardFqdn } = await dashboard.getLocation(); + // https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims const claims = { sub: user.username, // it is essential to always return a sub claim email: user.email, email_verified: true, family_name: lastName, + middle_name: middleName, given_name: firstName, locale: 'en-US', name: user.displayName, diff --git a/src/routes/test/users-test.js b/src/routes/test/users-test.js index 744416296..4568a1fb4 100644 --- a/src/routes/test/users-test.js +++ b/src/routes/test/users-test.js @@ -638,4 +638,3 @@ describe('Users API', function () { }); }); }); - diff --git a/src/test/users-test.js b/src/test/users-test.js index 02acd35a5..5e13db915 100644 --- a/src/test/users-test.js +++ b/src/test/users-test.js @@ -623,4 +623,18 @@ describe('User', function () { expect(result.length).to.be(0); // should have been removed by mandatory 2fa setting change }); }); + + describe('parseDisplayName', function () { + it('parses names', function () { + const names = [ + { in: 'James', out: { firstName: 'James', lastName: '', middleName: '' } }, + { in: 'James Anderson', out: { firstName: 'James', lastName: 'Anderson', middleName: '' } }, + { in: 'James Philip Anderson', out: { firstName: 'James', lastName: 'Philip Anderson', middleName: '' } }, + ]; + + for (const name of names) { + expect(users.parseDisplayName(name.in)).to.eql(name.out); + } + }); + }); }); diff --git a/src/users.js b/src/users.js index ff396ce3c..c73878ecd 100644 --- a/src/users.js +++ b/src/users.js @@ -56,6 +56,8 @@ exports = module.exports = { resetSource, + parseDisplayName, + AP_MAIL: 'mail', AP_WEBADMIN: 'webadmin', @@ -981,3 +983,15 @@ async function setProfileConfig(profileConfig) { async function resetSource() { await database.query('UPDATE users SET source = ?', [ '' ]); } + +function parseDisplayName(displayName) { + assert.strictEqual(typeof displayName, 'string'); + + const middleName = ''; + const idx = displayName.indexOf(' '); + if (idx === -1) return { firstName: displayName, lastName: '', middleName }; + + const firstName = displayName.substring(0, idx); + const lastName = displayName.substring(idx+1); + return { firstName, lastName, middleName }; +}