bhoriuchi / passport-activedirectory

Active Directory strategy for passport.js
29 stars 16 forks source link

Fix compatibility after 'activedirectory2' upgrade #23

Closed sheppi72 closed 1 year ago

sheppi72 commented 1 year ago

Source

After merging PR #11, the original parameter list changed, but the package version was not upgraded to a new major version, so yarn/npm can update the package with a "breaking change".

Problem

The activedirectory2 package uses an "Option Object" for ldap.attributes, it no longer accepts an attribute array.

Old configuration works with npm/activedirectory

{
  "url": "ldap://dc.domain.com",
  "baseDN": "dc=domain,dc=com",
  "username": "username@domain.com",
  "password": "password",
  "attributes": ["dn", "displayName", "givenName", "sn", "userPrincipalName", "sAMAccountName", "mail"]
}

New configuration works with npm/activedirectory2

{
  "url": "ldap://dc.domain.com",
  "baseDN": "dc=domain,dc=com",
  "username": "username@domain.com",
  "password": "password",
  "attributes": {
    "user": ["dn", "displayName", "givenName", "sn", "userPrincipalName", "sAMAccountName", "mail"]
  }
}

In my environment yarn updated the package to 1.3.0 (with rule "^1.0.4"), then I got the following error:

TypeError: `dn,displayName,givenName,sn,userPrincipalName,sAMAccountName,mail` is not an Option Object
    at module.exports (node_modules/merge-options/index.js:164:10)
    at new ActiveDirectory (node_modules/activedirectory2/lib/activedirectory.js:230:14)

Solution

I simply check, and changed the attributes key from original ldap options, and convert for activedirectory2 standard, if required. It's completely reverse compatible, and accept the new structure too, without major version or documentation change.

tgabi333 commented 1 year ago

ping @bhoriuchi