reviewboard/accounts/backends/ad.py (Diff revision 1)Show all issues
Review Request #11896 — Created Dec. 17, 2021 and submitted
The Active Directory support was largely built around
rather than supporting
ldaps://. This did attempt to invoke TLS
support, which would switch over to a secure connection if available,
but that either wasn't working or didn't work in all configurations.
This change reworks the TLS support to do a few things:
1) Assign a default port of 636 (the LDAP TLS port) if an explicit port
was not provided and TLS was enabled.
2) Assume TLS if the port is 636 (even if TLS is not enabled -- mostly
intended for DNS-based AD domain controller lookup).
3) Specifically opt into LDAP 3, which should be safe for modern usage
(and we opt into it for the standard LDAP backend). This is
documented as being the default anyway, but it's better to opt into
ldaps://when TLS is enabled.
5) Avoid calling
start_tls_s(), which may not be required for
URLs (and appears to cause errors stating that TLS has already been
ldaps://). Based on other python-ldap examples for
Active Directory, it seems this should not be required.
Unit tests passed.
Tested this with a customer using TLS, with and without an explicit
port configured. Tested that
start_tls_s()failed with a
URI and that TLS support worked with
F841 local variable 'E' is assigned to but never used
Fixed a missing
eand an accidental capitalization of another.
Revision 2 (+68 -50)