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 #13372: Put guard around assigning location via LDAP #13397

Merged
merged 1 commit into from
Aug 7, 2023

Conversation

uberbrady
Copy link
Collaborator

A customer [fd-37146] also noticed some errors around LDAP syncs, where if they set no default location at all, the sync returns an error. This puts in a simple tristate to only try to retrieve the ID of the location if the location is not null.

I did a local edit on the customer instance to see if this fix actually does fix the problem and it seems to.

fixes #13372

@what-the-diff
Copy link

what-the-diff bot commented Aug 2, 2023

PR Summary

  • User Location Handling Update
    The code was updated to ensure that if there is no specified location for a user, the system will now correctly identify and register it as 'Unspecified' (Null). This happens in a part of our system that syncs user data.

@mihaiolimpiu
Copy link

mihaiolimpiu commented Aug 2, 2023

Careful, the fix I posted in the mentioned issue was wrong, will update.

Edit: I installed the current fix and this does not import any location.
Ideally we should import only the locations for the user from LDAP and if location is NULL, ignore.

What happens now, all users have a NULL location :( and ALWAYS updates all users.

@mihaiolimpiu
Copy link

Commits on Apr 25, 2023
added ldap_location to settings

@uberbrady: Retested everything, my comment above was based on the wrong impression that my users were having a location assigned automatically by mistake.

Restored a backup and... well realized my mistake. code looks good.

Thank you!

@mihaiolimpiu mihaiolimpiu mentioned this pull request Aug 3, 2023
2 tasks
@lerroot
Copy link

lerroot commented Aug 4, 2023

What version of snipeIT will this fix be incorporated into?

@uberbrady
Copy link
Collaborator Author

Should be merged to master in the next week or so, and then when we cut a new point-release it should include the fix.

@snipe snipe merged commit 32d8d8c into snipe:develop Aug 7, 2023
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.

4 participants