search

claudioc / jingo

Node.js based Wiki
MIT License
1.02k stars 184 forks source link

Add support for specifying ldap auth option, searchAttributes, from config.yaml #179

Closed warrenfalk closed 7 years ago

warrenfalk commented 7 years ago

Fixes issue #178 by allowing the "searchAttributes" option of the passport ldap module to be specified from the config.yaml file.

The following example from config.yaml will limit the returned search attributes to just displayName and mail

    searchAttributes:
      - displayName
      - mail
claudioc commented 7 years ago

Ah, good one. Thanks (/cc @everpcpc)

everpcpc commented 7 years ago

@warrenfalk It's a good practice limiting the search attributes. But this may break users using old config without searchAttributes. Since only this two attributes are necessary for us here, I think it's better putting searchAttributes: ['displayName', 'mail'] here rather than in config.yaml. cc @claudioc

warrenfalk commented 7 years ago

@everpcpc this doesn't break old configs though. Passing undefined in searchAttributes functions the same as not passing it at all, so old configs will behave as they already did.

It is not a bad idea to default to ['displayName', 'mail'] as a default here when not specified so that this is more likely to work by default, but I went with this change because it had the least impact.

My thought also is that some users may fork to add special behavior for certain attributes, and so it seemed better to err on the side of configurability.