Allow disabling LDAP referral chasing on Linux. #54380
Conversation
Before this, changing SessionOptions.ReferralChasing on Linux was ineffective, since we were passing `ref int` to OpenLDAP, which does not follow the pointer passed and interprets any non-zero value as "enabled". This passes a boolean directly instead, which the library is able to detect properly.
...ryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapSessionOptions.Linux.cs
Outdated
Show resolved
Hide resolved
...ryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapSessionOptions.Linux.cs
Outdated
Show resolved
Hide resolved
|
@danmoseley or @krwq, do you have any more feedback for this PR? |
|
@buyaa-n is also out right now; it looks good to me though. The check failures look unrelated, but I'm going to rerun them once before merging. |
|
One thing to note: this PR does not enable controlling the referral types followed (e.g. subordinate or external); it's all or nothing. I believe it is possible to configure the type of referrals followed by setting the I think that this can be handled in a separate PR though. |
...ryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapSessionOptions.Linux.cs
Show resolved
Hide resolved
|
@dotnet/area-system-directory-services: Thanks for looking this over! Do PRs in this area need to be approved by all three of you, or does the auto-merge label need to be set? |
| @@ -14,7 +14,23 @@ public class LdapSessionOptionsTests | |||
| [PlatformSpecific(TestPlatforms.Windows)] | |||
| [InlineData(ReferralChasingOptions.None)] | |||
| [InlineData(ReferralChasingOptions.External)] | |||
| public void ReferralChasing_Set_GetReturnsExpected(ReferralChasingOptions value) | |||
| public void ReferralChasing_Set_GetReturnsExpected_On_Windows(ReferralChasingOptions value) | |||
There was a problem hiding this comment.
this test seems identical to the one below, can you join them together? [PlatformSpecific(TestPlatforms.Windows | TestPlatforms.Linux)] should work
There was a problem hiding this comment.
They have the same content, but different inputs, since setting External is not supported on Linux but it is on Windows. Can we scope the InlineData attribute for each platform? (Sorry if that's obvious: I didn't see examples of this elsewhere.)
|
@iinuwa this change looks correct to me but I'm not too familiar with LDAP, @joperezr is but he's currently on vacation (until around August). We can go about it two ways:
considering this change is scoped to Linux and the referral chasing doesn't seem to work there I personally think it's acceptable to merge this. One thing to note is that we're really close to ask mode (branch locking before release - after that all changes need to be approved before merging to mitigate risk of breaking something) and that is making me slightly uncomfortable. @jeffhandley are you ok with me merging this now (after last comment related to tests) and @joperezr taking a second look later? |
ghost
locked as resolved and limited conversation to collaborators
Before this, changing SessionOptions.ReferralChasing on Linux was ineffective, since we were passing
ref intto OpenLDAP, which does not follow the pointer passed and interprets any non-zero value as "enabled".This passes a boolean directly instead, which the library is able to detect properly.
This is a little bit of a hack, since it relies on the fact that OpenLDAP doesn't actually check for the correct value of
LDAP_OPT_ON, which changes every time the program is run. Instead, it compares whether the passed value is 0 when cast toint *, which makes itfalse; passing any other value evaluates totrue. Actually getting the correct value of LDAP_OPT_ON, I think would require a putting a get_ldap_opt_on() function in a native shim for OpenLDAP, which seems a little overkill. The documentation saysbut maybe it's ok to fudge that.
Fixes #44826.