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

Added #15506: Explicitly request used LDAP attributes #15507

Merged

Conversation

setpill
Copy link
Contributor

@setpill setpill commented Sep 13, 2024

Description

Couple changes here, with atomic commits to make it easier to review.

First commit refactors the $ldap_result_* variables in app/Command/Console/LdapSync.php into an associative array ($ldap_map). By itself, this does not add functionality (of course; it's a refactor after all) but it is "self-contained" in that it can be applied without causing errors. And since most of the changes in this commit are a simple search/replace, separating it out has the benefit of making code review easier.

The second commit contains the meat of the PR. It changes the findLdapUsers function in app/Models/Ldap.php to add an $attributes parameter, which is used in the ldap_search call later. This parameter defaults to [], which is the existing behaviour. The handle function in app/Command/Console/LdapSync.php is changed to make use of this new parameter, by passing a filtered version of the $ldap_map with the empty values taken out.

Fixes #15506

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Local testing with a LDAP server that provides an NSACCOUNTLOCK operational attribute on "disabled" accounts. Before this PR, the attribute was not seen by Snipe-IT as by default, only non-operational attributes are returned. The specific setup I'm working with is rather complex to replicate, and I'm not sure what makes for an easy substsitute LDAP server (or server mockup).

Checklist:

Copy link

what-the-diff bot commented Sep 13, 2024

PR Summary

  • Consolidation of LDAP User Settings Retrieval
    This PR introduces a new method to gather LDAP user settings, grouping them into a centralized associative array ($ldap_map), streamlining the process and making it easier to manage.

  • Extension to LDAP Users Search Functionality
    A significant change has been made to Ldap::findLdapUsers, allowing for additional specific LDAP attributes to be requested. This grants more flexibility during the search process.

  • Modification of Code for Improved Readability
    Code within LdapSync.php has been altered to utilize the new $ldap_map array. This amendment not only simplifies the referencing of LDAP fields but also improves the understanding and maintainability of the code.

  • Optimized LDAP Search Query
    The LDAP search query has been updated to make use of the $attributes specified instead of fetching all attributes, which optimizes the data retrieval process.

  • Refined Error Handling
    Modifications have been made to the error handling process, ensuring LDAP attribute access aligns with the new mapping structure within the handle() function in LdapSync.php. This ensures a consistent experience when handling any unexpected occurrences.

@Godmartinz
Copy link
Collaborator

Looks good to me 👍

@snipe snipe merged commit d7bde37 into snipe:develop Sep 19, 2024
8 of 9 checks passed
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.

3 participants