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

Enhance LDAP errors handling #14561

Merged
merged 1 commit into from Jun 7, 2023

Conversation

trasher
Copy link
Contributor

@trasher trasher commented Apr 19, 2023

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #14556

Meant to replace #14356

@trasher
Copy link
Contributor Author

trasher commented Apr 19, 2023

As draft for now, for first feedbacks.

Copy link
Member

@cedric-anne cedric-anne left a comment

Choose a reason for hiding this comment

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

👍

This is really a great move here. We should always use defensive code when dealing with a remote service, especially when it involves dynamic data entered by the end user.

I think it could be a good idea to add tests that validates that all these failing cases are producing helpful messages, at least with openldap.

src/User.php Outdated Show resolved Hide resolved
src/AuthLDAP.php Outdated Show resolved Hide resolved
src/AuthLDAP.php Outdated Show resolved Hide resolved
src/Auth.php Outdated Show resolved Hide resolved
src/AuthLDAP.php Outdated Show resolved Hide resolved
src/User.php Outdated Show resolved Hide resolved
src/AuthLDAP.php Outdated Show resolved Hide resolved
src/Auth.php Outdated Show resolved Hide resolved
src/AuthLDAP.php Outdated Show resolved Hide resolved
src/AuthLDAP.php Outdated Show resolved Hide resolved
src/Auth.php Outdated Show resolved Hide resolved
tests/LDAP/AuthLdap.php Outdated Show resolved Hide resolved
src/AuthLDAP.php Outdated Show resolved Hide resolved
src/AuthLDAP.php Outdated Show resolved Hide resolved
src/AuthLDAP.php Show resolved Hide resolved
Copy link
Member

@cedric-anne cedric-anne left a comment

Choose a reason for hiding this comment

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

OK for me now.

We could probably add tests, and maybe improve some messages (tests would help to see what we will push in logs).

@cedric-anne cedric-anne added this to the 10.0.8 milestone Apr 20, 2023
@cedric-anne cedric-anne changed the title Add detailled errors when muted ldap operations fails Enhance LDAP errors handling Apr 20, 2023
@trasher trasher marked this pull request as ready for review May 17, 2023 07:20
Silent error in status checker
Properly handle non existing group
Deprecate dead 'clean' parameter
Handle LDAP_NO_SUCH_OBJECT error response from LDAP
Fix return types; limit encapsulation, simplify code
Deprecated useless param
Handle errors on `ldap_parse_result`
@trasher trasher merged commit bcb1afb into glpi-project:10.0/bugfixes Jun 7, 2023
11 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.

None yet

3 participants