search

simplesamlphp / simplesamlphp-module-ldap

Module that provides authentication against LDAP stores
GNU Lesser General Public License v2.1
4 stars 8 forks source link

Restore the ability to read attributes using a privileged account #51

Closed grawity closed 7 months ago

grawity commented 7 months ago

We have certain attributes that are marked as 'confidential' in AD and therefore can only be read using a privileged account. The ability to do that was lost in commit 2d111bd9c5341273c7b532e5445e63daade4c21e – the settings were read but not used anywhere.

It's almost 5am so this is probably not quite complete, but it did the job.

tvdijen commented 7 months ago

The best code is written at 5 AM ;) Thanks a lot @grawity ! The only thing missing here, I think, is the same change but for the authproc-filters

codecov[bot] commented 7 months ago

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (831459b) 20.76% compared to head (342ad9d) 20.64%. Report is 5 commits behind head on master.

:exclamation: Current head 342ad9d differs from pull request most recent head 33f27ab. Consider uploading reports for the commit 33f27ab to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #51 +/- ## ============================================ - Coverage 20.76% 20.64% -0.13% - Complexity 141 145 +4 ============================================ Files 10 10 Lines 703 712 +9 ============================================ + Hits 146 147 +1 - Misses 557 565 +8 ```
tvdijen commented 7 months ago

Tagged v2.3.2

grawity commented 7 months ago

The only thing missing here, I think, is the same change but for the authproc-filters

Ah, I forgot to take a look at this, but it's probably not an issue because authproc filters don't bind() as the user anyway (they don't have the user's credentials after all) so after the authproc binds as the privileged account to do the search, it just continues using that session for the read.

(Come to think of it, I could've just used an authproc to fetch the attributes without needing to re-add priv.read at all...)

Though I don't remember if authproc also had the option to use different 'search' and 'priv read' accounts like here – I'll take a look – but I don't really know when anyone would need them to be different anyway. (even here with the authsource, I'm actually using the exact same values in search.username and priv.username. I fixed it to work the way docs say, but otherwise I would've just made it use a single account for both.)