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

Rework disable bitmask logic #84

Open
tlater-famedly opened this issue Nov 4, 2024 · 0 comments
Open

Rework disable bitmask logic #84

tlater-famedly opened this issue Nov 4, 2024 · 0 comments
Labels
tech debt Work on this should be planned according to tech debt policy Technical Debt This Issue is a technical debt. Mark issues with this label to have an overview of the code quality.

Comments

@tlater-famedly
Copy link
Contributor

          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.

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 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.
@tlater-famedly tlater-famedly added Technical Debt This Issue is a technical debt. Mark issues with this label to have an overview of the code quality. good first issue Good for newcomers tech debt Work on this should be planned according to tech debt policy and removed good first issue Good for newcomers labels Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech debt Work on this should be planned according to tech debt policy Technical Debt This Issue is a technical debt. Mark issues with this label to have an overview of the code quality.
Projects
None yet
Development

No branches or pull requests

1 participant