vesse
commented
2 years ago Could you share your configuration (without server, user, etc)? The point of that change is to ensure groupDnProperty is included in attributes if both are provided.
Closed popindavibe closed 2 years ago
vesse
commented
2 years ago Could you share your configuration (without server, user, etc)? The point of that change is to ensure groupDnProperty is included in attributes if both are provided.
popindavibe
commented
2 years ago Here is the LDAP section of the config.js file used for Hedgedoc:
"ldap": {
"bindCredentials": "XXXXXXX",
"bindDn": "cn=bind_user,ou=apps,dc=example,dc=fr",
"searchAttributes": [
"uid",
"displayName",
"mail"
],
"searchBase": "ou=People,dc=example,dc=fr",
"searchFilter": "&(objectclass=inetOrgPerson)(|(memberOf=CN=gardener,OU=Group,DC=example,DC=fr)(memberOf=CN=cook,OU=Group,DC=example,DC=fr))(uid={{username}})",
"tlsOptions": {
"port": 636
},
"tlsca": "/etc/pki/example/CA/cacert.crt",
"url": "ldaps://ldap.example.fr",
"useridField": "uid",
"usernameField": "cn"
},This has been working for years without any issue (aside from the self-signed CA, which took some time).
This config works when I comment out the if block.
vesse
commented
2 years ago Sorry for leaving this hanging but I had a vacation. But, it looks to me that the config you have defined is not directly passed to this library as there are unsupported options, ie. Hedgehoc probably constructs it? Could you debug what the properties used in that if actually contain? ie. opts.attributes and self.opts.groupDnProperty
davidmehren
commented
2 years ago Hi there, HedgeDoc dev here.
Tl;dr: This is a bug on our side.
For reference, we received a report about this problem here: https://github.com/hedgedoc/hedgedoc/issues/2561
Our latest release 1.9.4 contains ldapauth-fork 5.0.5, the previous release used 5.0.2. The ldapauth-commit causing this is https://github.com/vesse/node-ldapauth-fork/commit/741a648df98d789856b3301d65103b74872fdeea, which tries to call .push on opts.attributes.
HedgeDoc 1 uses deep-freeze to make the config object read-only after it has been generated. When ldapauth tries to push new data into the array, it breaks.
One could argue that expecting the config object to be writable since 5.0.3 is an API-break on your part, but I guess anyone who freezes their config object is responsible for the consequences 😄
We'll fix this by passing a mutable clone of our LDAP config to the LDAP strategy.
By the way, I noticed while debugging: In line 89 of ldapauth.js, this.opts.groupDnProperty either has a truthy value or gets assigned a default. In line 326 self.opts.groupDnProperty gets checked for truthiness again, but at this point, it should always be true.
popindavibe
commented
2 years ago Oh, thanks @davidmehren for jumping in, I wasn't finding the time to follow through on this :disappointed:
vesse
commented
2 years ago @davidmehren yeah you're right about that mutation change being an API break. The whole thing was sort of a nightmare because it seemed trivial enough to merge but didn't really work and is altogether ensuring the strategy is configured properly, I'd better move the check to an assert and throw an error if the attribute needed is not included in the attributes to fetch.
Hello,
Context
I am using Hedgedoc with LDAP authentication, following the last upgrade authentication breaks with:
→
at LdapAuth._findUser (/opt/codimd/codimd-git/node_modules/ldapauth-fork/lib/ldapauth.js:329If I comment the newly introduce
ifblock, authentication to do Group filtering, it 'just works', as before:I make no use of group filters anywhere in my LDAP configuration, so I don't understand why this is being applied. Also having a hard time to understand the last condition of the
if.