`LdapService` should throw if attempting to query with `xhLdapUsername = 'none'`

[amcclain]

~If you are running in a state where you have xhLdapConfig.enabled: true but xhLdapUsername set to default placeholder of none, the service should be able to detect that it's not really enabled and throw a useful exception.~

Core change updated as per revised title + discussion below

[lbwexler]

Hi --- I think the title of this ticket and its description don't quite match.

@amcclain -- were you thinking you wanted the lack of credentials to disable the monitor, OR alternatively to fail the monitor check with a more helpful exception. I thought the latter, but looks like Ryan implemented the former

[amcclain]

I was thinking that LdapService would generally return false from ldapService.getEnabled() if creds left on none - what Ryan implemented. So not any monitor change, although built-in monitor would report as inactive if service not enabled which seems right

[amcclain]

I don't know about the "throw a useful exception" part - wanted to have some hint to developer in case they are staring at xhLdapConfig.enabled and yet service reports not enabled.

Open to any other ideas, but since we spread the configuration across three configs (due to pwd) we are left open to this inconsistency. Did not seem like a great idea to try and actively connect to ldap with username of "none" but 🤷 now I am trying to recall why I cared about this

[lbwexler]

why wouldn't this check just default to not enabled. If it is enabled, when it runs it explicitly checks for both username and password, and if they are not set (e.g. none for username) it just throws with a useful hint that one or both of these things need to be set, or the thing should be disabled!

On Thu, Aug 1, 2024 at 1:39 PM Anselm McClain @.***> wrote:

I don't know about the "throw a useful exception" part - wanted to have some hint to developer in case they are staring at xhLdapConfig.enabled and yet service reports not enabled.

Open to any other ideas, but since we spread the configuration across three configs (due to pwd) we are left open to this inconsistency. Did not seem like a great idea to try and actively connect to ldap with username of "none" but 🤷 now I am trying to recall why I cared about this

— Reply to this email directly, view it on GitHub https://github.com/xh/hoist-core/issues/366#issuecomment-2263609833, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARTL2IDCKGWHY43V3XK3HTZPJXFJAVCNFSM6AAAAABJ6MWPZGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRTGYYDSOBTGM . You are receiving this because you were assigned.Message ID: @.***>

[amcclain]

Hi again - would like to merge or otherwise close out / abandon this change.

We were just looking at the behavior of LdapService itself, not the default status check per se:

• Do we want public ldapService.enabled getter to return true if the config is enabled but xhLdapUsername = none? That is the current behavior, but it seemed a bit sub-optimal in that the service methods would then be enabled and we would attempt to bind the LDAP using that default none user - I am assuming that would always fail with some kind of auth exception.
• I was thinking instead the service would report itself as disabled in that case - since it effectively is, with that placeholder credential. Any attempt to call the public lookup APIs on the service would throw with the newly expanded exception message in the PR.

But maybe a better approach would be:

• Unwind change to ldapService.getEnabled() - that reports straight value of config.
• Check for none placeholder user in new dedicated check at top of ldapService.doQuery() - right after the existing check on enabled - throwing if user=none
• That way we don't attempt to bind to LDAP with none user, and any caller - as well as the status check - would get a more direct / clear exception with pointer to the username config.

That effectively lets the built-in status check warn about this misconfig, and alleviates my concern about bothering the actual ldap system with failing auth queries.

[lbwexler]

yes to second approach. 100% what I had in mind

On Mon, Aug 5, 2024 at 6:39 PM Anselm McClain @.***> wrote:

Hi again - would like to merge or otherwise close out / abandon this change.

We were just looking at the behavior of LdapService itself, not the default status check per se:

• Do we want public ldapService.enabled getter to return true if the config is enabled but xhLdapUsername = none? That is the current behavior, but it seemed a bit sub-optimal in that the service methods would then be enabled and we would attempt to bind the LDAP using that default none user - I am assuming that would always fail with some kind of auth exception.
• I was thinking instead the service would report itself as disabled in that case - since it effectively is, with that placeholder credential. Any attempt to call the public lookup APIs on the service would throw with the newly expanded exception message in the PR.

But maybe a better approach would be:

• Unwind change to ldapService.getEnabled() - that reports straight value of config.
• Check for none placeholder user in new dedicated check at top of ldapService.doQuery() - right after the existing check on enabled - throwing if user=none
• That way we don't attempt to bind to LDAP with none user, and any caller - as well as the status check - would get a more direct / clear exception with pointer to the username config.

That effectively lets the built-in status check warn about this misconfig, and alleviates my concern about bothering the actual ldap system with failing auth queries.

— Reply to this email directly, view it on GitHub https://github.com/xh/hoist-core/issues/366#issuecomment-2270038207, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARTL2PHAGHGE6L3PI3L5CDZP75KRAVCNFSM6AAAAABJ6MWPZGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZQGAZTQMRQG4 . You are receiving this because you were assigned.Message ID: @.***>