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

LDAP settings form hitting wrong API endpoint on save #16173

Closed
noahmoss opened this issue May 20, 2021 · 2 comments
Closed

LDAP settings form hitting wrong API endpoint on save #16173

noahmoss opened this issue May 20, 2021 · 2 comments
Labels
Type:Tech Debt or Refactoring
Milestone

Comments

@noahmoss
Copy link
Member

It appears that the LDAP settings page is hitting /api/settings rather than /api/ldap/settings. The latter endpoint validates that a connection to the LDAP server can be established and returns an error if not. This is pretty important to fix since without this validation, it's easy to save invalid LDAP credentials and then only discover the error later.

We're already trying to map the updateSettings prop to updateLdapSettings in SettingsLdapForm.jsx but this is broken, maybe due to something in SettingsBatchForm?

@connect(
null,
{ updateSettings: updateLdapSettings },
)

I'm comparing this to SettingsSlackForm.jsx which does something similar, but doesn't use SettingsBatchForm, and it works properly. I don't really know enough about react/redux to diagnose this further.

To Reproduce
Steps to reproduce the behavior:

  1. Go to Admin -> Authentication -> LDAP
  2. Put in arbitrary settings into the form fields
  3. Click "Save changes"
  4. Observe in the browser's network tab that /api/settings is hit rather than /api/ldap/settings

Expected behavior
We should be hitting /api/ldap/settings and displaying an error if one is returned from the backend

@noahmoss noahmoss added Type:Bug Product defects .Needs Triage labels May 20, 2021
@noahmoss
Copy link
Member Author

noahmoss commented May 20, 2021

This also seems to be true for the email settings component, which also uses SettingsBatchForm

@flamber flamber added Type:Tech Debt or Refactoring and removed .Needs Triage Type:Bug Product defects labels May 21, 2021
@flamber
Copy link
Contributor

flamber commented May 21, 2021

Very related to #11446 and #13313

nemanjaglumac added a commit that referenced this issue May 24, 2021
@noahmoss noahmoss added this to the 0.39.3 milestone May 24, 2021
nemanjaglumac added a commit that referenced this issue May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type:Tech Debt or Refactoring
Projects
None yet
Development

No branches or pull requests

2 participants