Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed undefined $search_base #14031

Merged
merged 1 commit into from
Dec 15, 2023
Merged

Fixed undefined $search_base #14031

merged 1 commit into from
Dec 15, 2023

Conversation

snipe
Copy link
Owner

@snipe snipe commented Dec 14, 2023

This should hopefully fix #13961, where if a user didn't pass the script a location ID or a base DN, it would crash with an undefined $search_base error.

@uberbrady can you take a look and test with our LDAP server? I don't have my LDAP test rig up right now.

Signed-off-by: snipe <snipe@snipe.net>
Copy link

what-the-diff bot commented Dec 14, 2023

PR Summary

  • Inclusion of New Variable
    An additional variable named $search_base has been introduced. This gathers value from the specific setting related to the LDAP base DN (Directory Name).

  • Manual Base DN Specification
    A new conditional statement has been incorporated. This provides the ability for a user to manually specify a base DN. This adds flexibility to the system, accommodating situations where automatic DN retrieval may not be appropriate or efficient.

  • Filter Specification
    Another conditional statement has been introduced which grants users the provision of specifying a filter. This move brings in better customization and refined search capabilities to the users.

  • Exception Handling Enhancement
    We've increased the robustness of our software by incorporating a try-catch block. This will effectively handle exceptions, ensuring that instead of causing a software crash, these incidents will now return a user-friendly error message. This will greatly improve the user experience and overall reliability of our system.

Copy link
Collaborator

@uberbrady uberbrady left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works, the logic is sound - it all seems like it's doing the right stuff. I can confirm that without the change, my LDAP syncs would give that same error message. And with this change, I instead get the correct behavior.

The only 'real' problem was that the $search_base variable was falling out of scope, so hoisting it up (as we did here) would've been enough to fix it (the null case of the search_base parameter will just pull in the settings' search_base, so that would've been enough).

So I think just in the process of tweaks here and there and refactors and whatnot we must've somehow knocked this one off-kilter. Whoopsie :(

Thank you for the fix and let's get this hot!

@snipe snipe merged commit 130be26 into develop Dec 15, 2023
7 checks passed
@snipe snipe deleted the bug/ldap_undefined_search_base branch January 8, 2024 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants