Skip to content
Snippets Groups Projects
Unverified Commit 920d1c02 authored by Alexander Polyankin's avatar Alexander Polyankin Committed by GitHub
Browse files

Handle env settings in batch form (#25067)

parent d5cfd8ea
Branches
Tags
No related merge requests found
Hide whitespace changes
Inline Side-by-side
frontend/src/metabase/admin/settings/components/SettingsBatchForm.jsx
+ 2
− 1
View file @ 920d1c02
......@@ -109,10 +109,11 @@ class SettingsBatchForm extends Component {
let valid = true;
const validationErrors = {};
const availableElements = elements.filter(e => !e.is_env_setting);
// Validate form only if LDAP is enabled
if (!enabledKey || formData[enabledKey]) {
elements.forEach(function (element) {
availableElements.forEach(function (element) {
// test for required elements
if (element.required && MetabaseUtils.isEmpty(formData[element.key])) {
valid = false;
......
frontend/src/metabase/admin/settings/components/SettingsSetting.jsx
+ 23
− 21
View file @ 920d1c02
/* eslint-disable react/prop-types */
import React, { Component } from "react";
import PropTypes from "prop-types";
import { assocIn } from "icepick";
import SettingHeader from "./SettingHeader";
import { t } from "ttag";
......@@ -14,7 +13,13 @@ import SettingToggle from "./widgets/SettingToggle";
import SettingSelect from "./widgets/SettingSelect";
import SettingText from "./widgets/SettingText";
import { settingToFormFieldId } from "./../../settings/utils";
import { SettingWarning } from "./SettingsSetting.styled";
import {
SettingContent,
SettingEnvVarMessage,
SettingErrorMessage,
SettingRoot,
SettingWarningMessage,
} from "./SettingsSetting.styled";
const SETTING_WIDGET_MAP = {
string: SettingInput,
......@@ -26,17 +31,6 @@ const SETTING_WIDGET_MAP = {
text: SettingText,
};
const updatePlaceholderForEnvironmentVars = props => {
if (props && props.setting && props.setting.is_env_setting) {
return assocIn(
props,
["setting", "placeholder"],
t`Using ` + props.setting.env_name,
);
}
return props;
};
export default class SettingsSetting extends Component {
static propTypes = {
setting: PropTypes.object.isRequired,
......@@ -63,23 +57,31 @@ export default class SettingsSetting extends Component {
const widgetProps = {
...setting.getProps?.(setting),
...setting.props,
...updatePlaceholderForEnvironmentVars(this.props),
...this.props,
};
return (
// TODO - this formatting needs to be moved outside this component
<li className="m2 mb4">
<SettingRoot>
{!setting.noHeader && (
<SettingHeader id={settingId} setting={setting} />
)}
<div className="flex">
<Widget id={settingId} {...widgetProps} />
</div>
<SettingContent>
{setting.is_env_setting ? (
<SettingEnvVarMessage>
{t`Using ` + setting.env_name}
</SettingEnvVarMessage>
) : (
<Widget id={settingId} {...widgetProps} />
)}
</SettingContent>
{errorMessage && (
<div className="text-error text-bold pt1">{errorMessage}</div>
<SettingErrorMessage>{errorMessage}</SettingErrorMessage>
)}
{setting.warning && (
<SettingWarningMessage>{setting.warning}</SettingWarningMessage>
)}
{setting.warning && <SettingWarning>{setting.warning}</SettingWarning>}
</li>
</SettingRoot>
);
}
}
frontend/src/metabase/admin/settings/components/SettingsSetting.styled.tsx
+ 20
− 1
View file @ 920d1c02
import styled from "@emotion/styled";
import { color } from "metabase/lib/colors";
export const SettingWarning = styled.div`
export const SettingRoot = styled.li`
margin: 1rem 1rem 2rem;
`;
export const SettingContent = styled.div`
display: flex;
`;
export const SettingWarningMessage = styled.div`
color: ${color("accent4")};
font-weight: bold;
padding-top: 0.5rem;
`;
export const SettingErrorMessage = styled.div`
color: ${color("error")};
font-weight: bold;
padding-top: 0.5rem;
`;
export const SettingEnvVarMessage = styled.div`
color: ${color("text-dark")};
font-weight: bold;
`;
src/metabase/api/email.clj
+ 33
− 18
View file @ 920d1c02
......@@ -77,32 +77,47 @@
(name (mb-to-smtp-settings k))
(str/upper-case v))])))
(defn- env-var-values-by-email-setting
"Returns a map of setting names (keywords) and env var values.
If an env var is not set, the setting is not included in the result."
[]
(into {}
(for [setting-name (keys mb-to-smtp-settings)
:let [value (setting/env-var-value setting-name)]
:when (some? value)]
[setting-name value])))
(api/defendpoint PUT "/"
"Update multiple email Settings. You must be a superuser or have `setting` permission to do this."
[:as {settings :body}]
{settings su/Map}
(validation/check-has-application-permission :setting)
;; the frontend has access to an obfuscated version of the password. Watch for whether it sent us a new password or
;; the obfuscated version
(let [obfuscated? (and (:email-smtp-password settings) (email/email-smtp-password)
(let [;; the frontend has access to an obfuscated version of the password. Watch for whether it sent us a new password or
;; the obfuscated version
obfuscated? (and (:email-smtp-password settings) (email/email-smtp-password)
(= (:email-smtp-password settings) (setting/obfuscate-value (email/email-smtp-password))))
settings (-> (cond-> settings
obfuscated?
(assoc :email-smtp-password (email/email-smtp-password)))
(select-keys (keys mb-to-smtp-settings))
(set/rename-keys mb-to-smtp-settings))
settings (cond-> settings
(string? (:port settings)) (update :port #(Long/parseLong ^String %))
(string? (:security settings)) (update :security keyword))
response (email/test-smtp-connection settings)]
;; override `nil` values in the request with environment variables for testing the SMTP connection
env-var-settings (env-var-values-by-email-setting)
settings (merge settings env-var-settings)
settings (-> (cond-> settings
obfuscated?
(assoc :email-smtp-password (email/email-smtp-password)))
(select-keys (keys mb-to-smtp-settings))
(set/rename-keys mb-to-smtp-settings))
settings (cond-> settings
(string? (:port settings)) (update :port #(Long/parseLong ^String %))
(string? (:security settings)) (update :security keyword))
response (email/test-smtp-connection settings)]
(if-not (::email/error response)
;; test was good, save our settings
(cond-> (assoc (setting/set-many! (set/rename-keys response (set/map-invert mb-to-smtp-settings)))
:with-corrections (let [[_ corrections] (data/diff settings response)]
(-> corrections
(set/rename-keys (set/map-invert mb-to-smtp-settings))
humanize-email-corrections)))
obfuscated? (update :email-smtp-password setting/obfuscate-value))
(let [[_ corrections] (data/diff settings response)
new-settings (set/rename-keys response (set/map-invert mb-to-smtp-settings))]
;; don't update settings if they are set by environment variables
(setting/set-many! (apply dissoc new-settings (keys env-var-settings)))
(cond-> (assoc new-settings :with-corrections (-> corrections
(set/rename-keys (set/map-invert mb-to-smtp-settings))
humanize-email-corrections))
obfuscated? (update :email-smtp-password setting/obfuscate-value)))
;; test failed, return response message
{:status 400
:body (humanize-error-messages response)})))
......
test/metabase/api/email_test.clj
+ 54
− 29
View file @ 920d1c02
......@@ -72,35 +72,60 @@
;; [[metabase.email/email-smtp-port]] was originally a string Setting (it predated our introduction of different
;; Settings types) -- make sure our API endpoints still work if you pass in the value as a String rather than an
;; integer.
(doseq [:let [original-values (email-settings)]
body [default-email-settings
(update default-email-settings :email-smtp-port str)]
;; test what happens on both a successful and an unsuccessful connection.
[success? f] {true (fn [thunk]
(with-redefs [email/test-smtp-settings (constantly {::email/error nil})]
(thunk)))
false (fn [thunk]
(with-redefs [email/retry-delay-ms 0]
(thunk)))}]
(tu/discard-setting-changes [email-smtp-host email-smtp-port email-smtp-security email-smtp-username
email-smtp-password email-from-address email-from-name email-reply-to]
(testing (format "SMTP connection is valid? %b\n" success?)
(f (fn []
(testing "API request"
(testing (format "\nRequest body =\n%s" (u/pprint-to-str body))
(if success?
(is (= (-> default-email-settings
(assoc :with-corrections {})
(update :email-smtp-security name))
(mt/user-http-request :crowberto :put 200 "email" body)))
(is (= {:errors {:email-smtp-host "Wrong host or port"
:email-smtp-port "Wrong host or port"}}
(mt/user-http-request :crowberto :put 400 "email" body))))))
(testing "Settings after API request is finished"
(is (= (if success?
default-email-settings
original-values)
(email-settings))))))))))
(let [original-values (email-settings)]
(doseq [body [default-email-settings
(update default-email-settings :email-smtp-port str)]
;; test what happens on both a successful and an unsuccessful connection.
[success? f] {true (fn [thunk]
(with-redefs [email/test-smtp-settings (constantly {::email/error nil})]
(thunk)))
false (fn [thunk]
(with-redefs [email/retry-delay-ms 0]
(thunk)))}]
(tu/discard-setting-changes [email-smtp-host email-smtp-port email-smtp-security email-smtp-username
email-smtp-password email-from-address email-from-name email-reply-to]
(testing (format "SMTP connection is valid? %b\n" success?)
(f (fn []
(testing "API request"
(testing (format "\nRequest body =\n%s" (u/pprint-to-str body))
(if success?
(is (= (-> default-email-settings
(assoc :with-corrections {})
(update :email-smtp-security name))
(mt/user-http-request :crowberto :put 200 "email" body)))
(is (= {:errors {:email-smtp-host "Wrong host or port"
:email-smtp-port "Wrong host or port"}}
(mt/user-http-request :crowberto :put 400 "email" body))))))
(testing "Settings after API request is finished"
(is (= (if success?
default-email-settings
original-values)
(email-settings)))))))))
(testing (format "SMTP connection is still valid when some settings are not specified, but set with env vars")
(let [body (dissoc default-email-settings
:email-smtp-port
:email-smtp-host
:email-smtp-security
:email-smtp-username
:email-smtp-password
:email-from-address)]
(tu/discard-setting-changes [email-smtp-host email-smtp-port email-smtp-security email-smtp-username
email-smtp-password email-from-address email-from-name email-reply-to]
(mt/with-temp-env-var-value [mb-email-smtp-port (:email-smtp-port default-email-settings)
mb-email-smtp-host (:email-smtp-host default-email-settings)
mb-email-smtp-security (name (:email-smtp-security default-email-settings))
mb-email-smtp-username (:email-smtp-username default-email-settings)
mb-email-smtp-password (:email-smtp-password default-email-settings)
mb-email-from-address (:email-from-address default-email-settings)]
(with-redefs [email/test-smtp-settings (constantly {::email/error nil})]
(testing "API request"
(is (= (-> default-email-settings
(assoc :with-corrections {})
(update :email-smtp-security name))
(mt/user-http-request :crowberto :put 200 "email" body))))
(testing "Settings after API request is finished"
(is (= default-email-settings
(email-settings)))))))))))
(testing "Updating values with obfuscated password (#23919)"
(mt/with-temporary-setting-values [email-from-address "notifications@metabase.com"
email-from-name "Sender Name"
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment