Fix duplicated bind operation, only one is needed #270
Conversation
Either we would have bound the connection thanks to auto_bind, or we would have had an unhandled error. Due to this, we can remove this.
consideRatio
force-pushed
the
pr/avoid-dup-bind
branch
from
f1362ad
to
a57aaf8
Compare
| if not conn.bind(): | ||
| self.log.warning( | ||
| f"Failed to connect to LDAP server with search user '{self.lookup_dn_search_user}'" | ||
| ) | ||
| if not conn: |
There was a problem hiding this comment.
This is right after calling get_connection, which always does a bind operation because we have auto_bind set to a value making it so. This makes the if not conn.bind() too much and it should at most be checking conn.bound property.
This PR updated the get_connection to return none and log a warning if the bind operation failed though, so we now just check if conn is set truthy or not going onwards here.
| if not is_bound: | ||
| self.log.warning(f"Invalid password for user '{username}'") |
There was a problem hiding this comment.
This warning about incorrect password wasn't always correct. It could say incorrect password even though it failed to bind for another reason, such as not finding a user with the userdn or similar.
I think there have been several issues about invalid password that really wasn't about invalid password, but about failing to bind because the user didn't exist - because the get_connection was passed a userdn that wasn't constructed correctly etc.
| self.log.debug(msg) | ||
| if is_bound: | ||
| conn = self.get_connection(userdn, password) | ||
| if conn: | ||
| break |
There was a problem hiding this comment.
This logic moved to get_connection, but no longer including logging details about the specific username (but still includes the userdn which should be sufficient).
consideRatio
force-pushed
the
pr/avoid-dup-bind
branch
from
e14495a
to
1ba6e0f
Compare
|
Thanks! Go ahead and merge |
To bind is to authenticate as a user against the user, where the user is specified by a DN (Distinguished Name). This operation can prompt inputting one time passwords on a mobile device etc, so its relevant to not do twice in a row.
When we create the connection object, we automatically bind as part of doing so in all cases. Due to that, its not relevant to first acquire the connection and then do
conn.bind().In this PR you'll see how I move logic to
get_connectionand letting it handle raised errors failing on failing to bind by emitting a warning and returning None. Usingget_connectioncan then be reduced to calling it and checking the return value. Any connectivity or encryption issues will still be raised and not handled by get_connection.