Skip to content

Conversation

@simonLeary42
Copy link
Contributor

@simonLeary42 simonLeary42 commented Oct 9, 2025

Changes

problems solved:

  • the utils do not check whether an LDAP operation was successful
  • the ldapsearch_* utils mask errors and return None, and there are no extra checks for None
    • this means the masked error should lead to another uncaught TypeError when that None makes its way into future logic
    • it would be much easier to debug if the original error were preserved
  • some utils break up modifications into a loop, which prevents ldap3 from ensuring predictable results:

    Due to the requirement for atomicity in applying the list of modifications in the Modify Request, the client may expect that no modifications have been performed if the Modify Response received indicates any sort of error, and that all requested modifications have been performed if the Modify Response indicates successful completion of the Modify operation. (source)

also:

  • removed lots of boilerplate
  • enabled ldap3's builtin logging feature
  • added exception info, argument info, stack trace info to log messages
  • added a boolean return value to many utils to indicate success
  • removed try/except from functions which couldn't possibly raise an exception
  • enabled read_only option when binding for read-only purposes (per @ds-04's suggestion)

Thoughts

Because the error handling is stricter, it's possible that a site has existing problems which aren't being reported, which will now be reported. The "reading" functions (ldapsearch_*) now raise exceptions where they used to return None, and they now raise excceptions (KeyError or LDAPException) where they used to return empty results. I was unable to find a definitive answer on whether or not uncaught exceptions are allowed: please advise.

Future work

  • add type hints
    • this was left out to help the github diff algorithm show changes in a sane way

@simonLeary42 simonLeary42 marked this pull request as draft October 9, 2025 14:36
@simonLeary42 simonLeary42 force-pushed the project-openldap-improvements branch 11 times, most recently from 590d774 to 94bfee0 Compare October 9, 2025 16:47
@simonLeary42
Copy link
Contributor Author

simonLeary42 commented Oct 9, 2025

  • TODO confirm what the logger output looks like with extra=conn

@simonLeary42 simonLeary42 changed the title improve project_openldap utils project_openldap: improve utils Oct 9, 2025
@simonLeary42 simonLeary42 force-pushed the project-openldap-improvements branch 7 times, most recently from 26311da to 33a69a2 Compare October 9, 2025 17:44
@ds-04
Copy link
Contributor

ds-04 commented Oct 10, 2025

This looks good

Please can you also make the read connection read-only

Current

def _ldap_read_wrapper(funcname, *args, **kwargs):
    conn = Connection(server, PROJECT_OPENLDAP_BIND_USER, PROJECT_OPENLDAP_BIND_PASSWORD, auto_bind=True)

Generic - note read_only=True

         connection = Connection(
            server_opt, bind_user, bind_password, auto_bind=True, read_only=True
        )

@simonLeary42 simonLeary42 force-pushed the project-openldap-improvements branch 8 times, most recently from 7fdf9c4 to 37d8c52 Compare October 10, 2025 14:09
Comment on lines +160 to +161
if len(conn.entries) == 0:
raise KeyError(dn)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if we can tell the difference between an empty group and a nonexistent group. One should throw KeyError, the other shouldn't. @ds-04 thoughts?

Copy link
Contributor Author

@simonLeary42 simonLeary42 Oct 14, 2025

Choose a reason for hiding this comment

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

We can search for attributes cn and memberuid, assert nonzero entry count, then return just memberuid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a decent way to check existence, but it requires another call:

In [38]: conn.search("cn=simonleary_umass_edu,ou=users,dc=unity,dc=rc,dc=umass,dc=edu", "(objectClass=*)", attributes=[], search_scope=BASE)
Out[38]: True

In [39]: conn.entries
Out[39]: [DN: cn=simonleary_umass_edu,ou=users,dc=unity,dc=rc,dc=umass,dc=edu - STATUS: Read - READ TIME: 2025-10-22T18:07:39.303686]

In [40]: conn.search("cn=foobar,ou=users,dc=unity,dc=rc,dc=umass,dc=edu", "(objectClass=*)", attributes=[], search_scope=BASE)
Out[40]: False

In [41]: conn.entries
Out[41]: []

Signed-off-by: Simon Leary <simon.leary42@proton.me>
@simonLeary42 simonLeary42 force-pushed the project-openldap-improvements branch from 5ddc6cd to 64779be Compare October 17, 2025 12:58
@simonLeary42 simonLeary42 marked this pull request as ready for review October 17, 2025 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants