Test LDAP settings before saving

[walterl]

When an admin user enters incorrect/invalid LDAP configuration values, those values are still quietly saved, without being validated.

It would potentially save admins a lot of time if they were able to get feedback about the validity of the settings they entered, instead of having to test manually by attempting an LDAP login.

I can think of two ways in which this could be accomplished:

1. Require valid settings: Validate all LDAP settings before saving. If it's not valid, it doesn't get saved.
2. Add a separate settings testing button, which validates the settings and gives feedback to the admin. Having a separate button could allow the admin to save the settings, even if invalid.

In both cases, validation should include testing that the specified LDAP server can be connected to, with the provided credentials, and queried.

[paoliniluis]

This would be a great testing tool inside LDAP auth that will test if a connection can be established and also pull a single register. Would save endless hours in debugging

[noahmoss]

It appears we already have an endpoint defined on the backend that tests the LDAP connection before saving the settings:

metabase/src/metabase/api/ldap.clj

Lines 88 to 108 in f93680a

	(api/defendpoint PUT "/settings"
	"Update LDAP related settings. You must be a superuser to do this."
	[:as {settings :body}]
	{settings su/Map}
	(api/check-superuser)
	(let [ldap-settings (select-keys settings (keys mb-settings->ldap-details))
	ldap-details (-> (set/rename-keys ldap-settings mb-settings->ldap-details)
	(assoc :port
	(when (seq (:ldap-port settings))
	(Integer/parseInt (:ldap-port settings)))))
	results (if-not (:ldap-enabled settings)
	;; when disabled just respond with a success message
	{:status :SUCCESS}
	;; otherwise validate settings
	(ldap/test-ldap-connection ldap-details))]
	(if (= :SUCCESS (:status results))
	;; test succeeded, save our settings
	(setting/set-many! ldap-settings)
	;; test failed, return result message
	{:status 500
	:body (humanize-error-messages results)})))

Unfortunately the frontend component seems to be hitting /api/settings instead of /api/ldap/settings, so this validation isn't currently being run. I've filed #16173 to fix this.

[nemanjaglumac]

As I wrote in #16187 (comment), entering arbitrary field settings passes without any validation (apart from the one that the port should be numeric). Is this a desired behavior?

Wrote a repro #16205 that reflects this.

[noahmoss]

Closing this issue since I just merged #16187 which hooks up the settings form to the correct endpoint that tests the LDAP connection. This only happens properly when LDAP has been enabled with the toggle at the top of the form, but we should have a separate issue to refactor the UI since the on/off toggle is unintuitive. This is the issue @nemanjaglumac observed.

FYI @paoliniluis, hopefully this helps with debugging LDAP problems :)