You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This code is growing in complexity. We should keep it as-is for this PR, because clearly there's some time pressure behind this, but let's mark rethinking the logic from scratch here as a maintenance day task.
Since we are splitting attention between both stringly-typed and bytely-typed status flags, and also have a separate configuration option to switch between two different modes of parsing statuses, the current state is a bit of a mess. I believe cleaner logic is possible, we should think through it again.
For context, the reason this mess exists is that ldap values may be either strings or bytes. In the protocol, fields that contain string values are delimited with :, and fields that contain byte values are delimited with ::.
The ldap3 crate, however, does not handle these specially. It instead automatically converts byte values that happen to be valid UTF-8 strings to Strings, and keeps others as Vec<u8>s (or rather, it has two different functions to retrieve either type, and both need to be called separately if it can be ambiguous which it is in practice).
As a result, we use this StringOrBytes enum to deal with that complexity for the most part, handling the value correctly according to where it ultimately ends up being used.
Since ldap is ldap, how fields are represented depends on the specific implementation, and we need to support both openldap and AD. The user status field on openldap contains a string (which may be "TRUE" or "FALSE"), while the AD status field contains a byte array to which a mask can be applied to figure out whether a user is active or not (but different bytes are set depending on... stuff).
As requirements have changed and grown, this has resulted in some quite awkward and complex code. We should figure out if there's a better way to do this.
Things I can imagine might work:
Go upstream and figure out a better way for ldap3 to handle fields that may either be strings or bytes
Come up with a clean way to represent TRUE vs FALSE and a bitmask as one, clean option, which does not limit the features of bitmasks, as customers may use different status bits.
Factor the logic for determining this stuff out of that function and rewrite it a bit more cleanly.
The text was updated successfully, but these errors were encountered:
Originally posted by @tlater-famedly in #82 (comment)
Since we are splitting attention between both stringly-typed and bytely-typed status flags, and also have a separate configuration option to switch between two different modes of parsing statuses, the current state is a bit of a mess. I believe cleaner logic is possible, we should think through it again.
For context, the reason this mess exists is that ldap values may be either strings or bytes. In the protocol, fields that contain string values are delimited with
:
, and fields that contain byte values are delimited with::
.The
ldap3
crate, however, does not handle these specially. It instead automatically converts byte values that happen to be valid UTF-8 strings toString
s, and keeps others asVec<u8>
s (or rather, it has two different functions to retrieve either type, and both need to be called separately if it can be ambiguous which it is in practice).As a result, we use this
StringOrBytes
enum to deal with that complexity for the most part, handling the value correctly according to where it ultimately ends up being used.Since ldap is ldap, how fields are represented depends on the specific implementation, and we need to support both openldap and AD. The user status field on openldap contains a string (which may be
"TRUE"
or"FALSE"
), while the AD status field contains a byte array to which a mask can be applied to figure out whether a user is active or not (but different bytes are set depending on... stuff).As requirements have changed and grown, this has resulted in some quite awkward and complex code. We should figure out if there's a better way to do this.
Things I can imagine might work:
ldap3
to handle fields that may either be strings or bytesTRUE
vsFALSE
and a bitmask as one, clean option, which does not limit the features of bitmasks, as customers may use different status bits.The text was updated successfully, but these errors were encountered: