FE: Settings access global permission + tweaks (#21460)

* settings global permission

* fix specs

* Enforce Setting permissions (cont) (#21464)

* settings global permission

* more api permissions enforcement

* only admin could call token checks

* address Noah's comments

* clean ns

* clean ns 



Co-authored-by: Aleksandr Lesnenko <alxnddr@gmail.com>

* hide subscriptions buttons for users with no permissions

* fix specs

* review fixes

* update spec

Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com>

enterprise/backend/test/metabase_enterprise/advanced_permissions/api/setting_test.clj

 (ns metabase-enterprise.advanced-permissions.api.setting-test
   "Permisisons tests for API that needs to be enforced by General Permissions to access Admin/Setting pages."
   (:require [clojure.test :refer :all]
+            [metabase.api.geojson-test :as geojson-test]
+            [metabase.api.ldap-test :as ldap-test]
             [metabase.email :as email]
             [metabase.integrations.slack :as slack]
             [metabase.models.permissions :as perms]
             [metabase.models.setting-test :refer [test-setting-1 test-setting-2]]
             [metabase.public-settings.premium-features-test :as premium-features-test]
             [metabase.test :as mt]
-            [metabase.test.fixtures :as fixtures]))
+            [metabase.test.fixtures :as fixtures]
+            [metabase.test.integrations.ldap :as ldap.test]))
 
 (use-fixtures :once (fixtures/initialize :db))
 
@@ -156,3 +159,82 @@
               (get-manifest user 200)
               (set-slack-settings :crowberto 200)
               (get-manifest :crowberto 200))))))))
+
+(deftest ldap-api-test
+  (testing "/api/ldap"
+    (mt/with-user-in-groups
+      [group {:name "New Group"}
+       user  [group]]
+      (letfn [(update-ldap-settings [user status]
+                (testing (format "update ldap settings with %s user" (user->name user))
+                  (ldap.test/with-ldap-server
+                    (mt/user-http-request user :put status "ldap/settings"
+                                          (ldap-test/ldap-test-details)))))]
+
+        (testing "if `advanced-permissions` is disabled, require admins"
+          (premium-features-test/with-premium-features #{}
+            (update-ldap-settings user 403)
+            (update-ldap-settings :crowberto 200)))
+
+        (testing "if `advanced-permissions` is enabled"
+          (premium-features-test/with-premium-features #{:advanced-permissions}
+            (testing "still fail if user's group doesn't have `setting` permission"
+              (update-ldap-settings user 403)
+              (update-ldap-settings :crowberto 200))
+
+            (testing "succeed if user's group has `setting` permission"
+              (perms/grant-general-permissions! group :setting)
+              (update-ldap-settings user 200)
+              (update-ldap-settings :crowberto 200))))))))
+
+
+(deftest geojson-api-test
+  (testing "/api/geojson"
+    (mt/with-user-in-groups
+      [group {:name "New Group"}
+       user  [group]]
+      (letfn [(get-geojson [user status]
+                (testing (format "get geojson with %s user" (user->name user))
+                  (mt/user-http-request user :get status "geojson"
+                                        :url geojson-test/test-geojson-url)))]
+
+        (testing "if `advanced-permissions` is disabled, require admins"
+          (premium-features-test/with-premium-features #{}
+            (get-geojson user 403)
+            (get-geojson :crowberto 200)))
+
+        (testing "if `advanced-permissions` is enabled"
+          (premium-features-test/with-premium-features #{:advanced-permissions}
+            (testing "still fail if user's group doesn't have `setting` permission"
+              (get-geojson user 403)
+              (get-geojson :crowberto 200))
+
+            (testing "succeed if user's group has `setting` permission"
+              (perms/grant-general-permissions! group :setting)
+              (get-geojson user 200)
+              (get-geojson :crowberto 200))))))))
+
+(deftest permissions-group-api-test
+  (testing "/api/permissions"
+    (mt/with-user-in-groups
+      [group {:name "New Group"}
+       user  [group]]
+      (letfn [(get-permission-groups [user status]
+                (testing (format "get permission groups with %s user" (user->name user))
+                  (mt/user-http-request user :get status "permissions/group")))]
+
+        (testing "if `advanced-permissions` is disabled, require admins"
+          (premium-features-test/with-premium-features #{}
+            (get-permission-groups user 403)
+            (get-permission-groups :crowberto 200)))
+
+        (testing "if `advanced-permissions` is enabled"
+          (premium-features-test/with-premium-features #{:advanced-permissions}
+            (testing "still fail if user's group doesn't have `setting` permission"
+              (get-permission-groups user 403)
+              (get-permission-groups :crowberto 200))
+
+            (testing "succeed if user's group has `setting` permission"
+              (perms/grant-general-permissions! group :setting)
+              (get-permission-groups user 200)
+              (get-permission-groups :crowberto 200))))))))

enterprise/frontend/src/metabase-enterprise/general_permissions/index.ts

@@ -6,12 +6,13 @@ import { t } from "ttag";
 import { canManageSubscriptions } from "./selectors";
 import generalPermissionsReducer from "./reducer";
 import { NAV_PERMISSION_GUARD } from "metabase/nav/utils";
-import { canAccessMonitoringItems } from "./utils";
+import { canAccessMonitoringItems, canAccessSettings } from "./utils";
 
 if (hasPremiumFeature("advanced_permissions")) {
-  NAV_PERMISSION_GUARD["audit"] = canAccessMonitoringItems as any;
-  NAV_PERMISSION_GUARD["tools"] = canAccessMonitoringItems as any;
-  NAV_PERMISSION_GUARD["troubleshooting"] = canAccessMonitoringItems as any;
+  NAV_PERMISSION_GUARD["audit"] = canAccessMonitoringItems;
+  NAV_PERMISSION_GUARD["tools"] = canAccessMonitoringItems;
+  NAV_PERMISSION_GUARD["troubleshooting"] = canAccessMonitoringItems;
+  NAV_PERMISSION_GUARD["settings"] = canAccessSettings;
 
   PLUGIN_GENERAL_PERMISSIONS.getRoutes = getRoutes;
   PLUGIN_GENERAL_PERMISSIONS.tabs = [{ name: t`General`, value: `general` }];

enterprise/frontend/src/metabase-enterprise/general_permissions/selectors.ts

@@ -11,10 +11,7 @@ import { GeneralPermissionsState } from "./types/state";
 import { GeneralPermissionKey, GeneralPermissions } from "./types/permissions";
 
 export const canManageSubscriptions = (state: GeneralPermissionsState) =>
-  state.currentUser.permissions.can_access_subscription;
-
-export const canAccessMonitoring = (state: GeneralPermissionsState) =>
-  state.currentUser.permissions.can_access_monitoring;
+  state.currentUser.permissions?.can_access_subscription ?? false;
 
 const getGeneralPermission = (
   permissions: GeneralPermissions,
@@ -30,6 +27,19 @@ export const getIsDirty = createSelector(
     !_.isEqual(permissions, originalPermissions),
 );
 
+const getPermission = (
+  permissions: GeneralPermissions,
+  isAdmin: boolean,
+  groupId: number,
+  permissionKey: GeneralPermissionKey,
+) => ({
+  permission: permissionKey,
+  isDisabled: isAdmin,
+  disabledTooltip: isAdmin ? UNABLE_TO_CHANGE_ADMIN_PERMISSIONS : null,
+  value: getGeneralPermission(permissions, groupId, permissionKey),
+  options: [GENERAL_PERMISSIONS_OPTIONS.yes, GENERAL_PERMISSIONS_OPTIONS.no],
+});
+
 export const getGeneralPermissionEditor = createSelector(
   (state: GeneralPermissionsState) =>
     state.plugins.generalPermissionsPlugin?.generalPermissions,
@@ -46,30 +56,9 @@ export const getGeneralPermissionEditor = createSelector(
         id: group.id,
         name: group.name,
         permissions: [
-          {
-            permission: "subscription",
-            isDisabled: isAdmin,
-            disabledTooltip: isAdmin
-              ? UNABLE_TO_CHANGE_ADMIN_PERMISSIONS
-              : null,
-            value: getGeneralPermission(permissions, group.id, "subscription"),
-            options: [
-              GENERAL_PERMISSIONS_OPTIONS.yes,
-              GENERAL_PERMISSIONS_OPTIONS.no,
-            ],
-          },
-          {
-            permission: "monitoring",
-            isDisabled: isAdmin,
-            disabledTooltip: isAdmin
-              ? UNABLE_TO_CHANGE_ADMIN_PERMISSIONS
-              : null,
-            value: getGeneralPermission(permissions, group.id, "monitoring"),
-            options: [
-              GENERAL_PERMISSIONS_OPTIONS.yes,
-              GENERAL_PERMISSIONS_OPTIONS.no,
-            ],
-          },
+          getPermission(permissions, isAdmin, group.id, "setting"),
+          getPermission(permissions, isAdmin, group.id, "monitoring"),
+          getPermission(permissions, isAdmin, group.id, "subscription"),
         ],
       };
     });
@@ -77,12 +66,13 @@ export const getGeneralPermissionEditor = createSelector(
     return {
       filterPlaceholder: t`Search for a group`,
       columns: [
-        { name: `General settings access` },
-        { name: `Subscriptions and Alerts` },
+        { name: t`Group name` },
+        { name: t`General settings access` },
         {
           name: `Monitoring access`,
           hint: t`This grants access to Tools, Audit, and Troubleshooting`,
         },
+        { name: t`Subscriptions and Alerts` },
       ],
       entities,
     };

enterprise/frontend/src/metabase-enterprise/general_permissions/types/permissions.ts

 import { GroupId } from "metabase-types/api";
 
-export type GeneralPermissionKey = "subscription" | "monitoring";
+export type GeneralPermissionKey = "subscription" | "monitoring" | "setting";
 export type GeneralPermissionValue = "yes" | "no";
 
 export type GroupGeneralPermissions = {

enterprise/frontend/src/metabase-enterprise/general_permissions/types/state.ts

 import { State } from "metabase-types/store";
 import { GeneralPermissions } from "./permissions";
-import { UserWithPermissions } from "./user";
+import { UserWithGeneralPermissions } from "./user";
 
 export interface GeneralPermissionsState extends State {
-  currentUser: UserWithPermissions;
+  currentUser: UserWithGeneralPermissions;
   plugins: {
     generalPermissionsPlugin: {
       generalPermissions: GeneralPermissions;

enterprise/frontend/src/metabase-enterprise/general_permissions/types/user.ts

 import { User } from "metabase-types/api";
 
-export interface UserWithPermissions extends User {
-  permissions: {
+export interface UserWithGeneralPermissions extends User {
+  permissions?: {
     can_access_monitoring: boolean;
-    can_access_settings: boolean;
+    can_access_setting: boolean;
     can_access_subscription: boolean;
   };
 }

enterprise/frontend/src/metabase-enterprise/general_permissions/utils.ts

-import { UserWithPermissions } from "./types/user";
+import { UserWithGeneralPermissions } from "./types/user";
 
-export const canAccessMonitoringItems = (user?: UserWithPermissions) =>
-  user?.permissions.can_access_monitoring ?? false;
+export const canAccessMonitoringItems = (user?: UserWithGeneralPermissions) =>
+  user?.permissions?.can_access_monitoring ?? false;
+
+export const canAccessSettings = (user?: UserWithGeneralPermissions) =>
+  user?.permissions?.can_access_setting ?? false;

enterprise/frontend/src/metabase-enterprise/license/index.js

@@ -10,6 +10,7 @@ PLUGIN_ADMIN_SETTINGS_UPDATES.push(sections =>
       ...license,
       component: LicenseAndBillingSettings,
       name: t`License and Billing`,
+      adminOnly: true,
     };
   }),
 );

frontend/src/metabase/admin/routes.jsx

@@ -10,7 +10,10 @@ import {
 
 import { withBackground } from "metabase/hoc/Background";
 import { ModalRoute } from "metabase/hoc/ModalRoute";
-import { createAdminRouteGuard } from "metabase/admin/utils";
+import {
+  createAdminRouteGuard,
+  createAdminRedirect,
+} from "metabase/admin/utils";
 
 import RedirectToAllowedSettings from "./settings/containers/RedirectToAllowedSettings";
 import AdminApp from "metabase/admin/app/components/AdminApp";
@@ -154,9 +157,9 @@ const getRoutes = (store, CanAccessSettings, IsAdmin) => (
       </Route>
 
       {/* SETTINGS */}
-      <Route path="settings" component={IsAdmin}>
+      <Route path="settings" component={createAdminRouteGuard("settings")}>
+        <IndexRoute component={createAdminRedirect("setup", "general")} />
         <Route title={t`Settings`}>
-          <IndexRedirect to="setup" />
           <Route
             path="premium-embedding-license"
             component={PremiumEmbeddingLicensePage}

frontend/src/metabase/admin/settings/selectors.js

@@ -25,6 +25,7 @@ import { trackTrackingPermissionChanged } from "./analytics";
 
 import { UtilApi } from "metabase/services";
 import { PLUGIN_ADMIN_SETTINGS_UPDATES } from "metabase/plugins";
+import { getUserIsAdmin } from "metabase/selectors/user";
 
 // This allows plugins to update the settings sections
 function updateSectionsWithPlugins(sections) {
@@ -54,6 +55,7 @@ const SECTIONS = updateSectionsWithPlugins({
     order: 1,
     settings: [],
     component: SettingsSetupList,
+    adminOnly: true,
   },
   general: {
     name: t`General`,
@@ -131,6 +133,7 @@ const SECTIONS = updateSectionsWithPlugins({
         type: "boolean",
       },
     ],
+    adminOnly: true,
   },
   email: {
     name: t`Email`,
@@ -418,31 +421,39 @@ export const getNewVersionAvailable = createSelector(getSettings, settings => {
   return MetabaseSettings.newVersionAvailable(settings);
 });
 
-export const getSections = createSelector(getSettings, settings => {
-  if (!settings || _.isEmpty(settings)) {
-    return {};
-  }
+export const getSections = createSelector(
+  getSettings,
+  getUserIsAdmin,
+  (settings, isAdmin) => {
+    if (!settings || _.isEmpty(settings)) {
+      return {};
+    }
 
-  const settingsByKey = _.groupBy(settings, "key");
-  const sectionsWithAPISettings = {};
-  for (const [slug, section] of Object.entries(SECTIONS)) {
-    const settings = section.settings.map(function(setting) {
-      const apiSetting =
-        settingsByKey[setting.key] && settingsByKey[setting.key][0];
-      if (apiSetting) {
-        return {
-          placeholder: apiSetting.default,
-          ...apiSetting,
-          ...setting,
-        };
-      } else {
-        return setting;
+    const settingsByKey = _.groupBy(settings, "key");
+    const sectionsWithAPISettings = {};
+    for (const [slug, section] of Object.entries(SECTIONS)) {
+      if (section.adminOnly && !isAdmin) {
+        continue;
       }
-    });
-    sectionsWithAPISettings[slug] = { ...section, settings };
-  }
-  return sectionsWithAPISettings;
-});
+
+      const settings = section.settings.map(function(setting) {
+        const apiSetting =
+          settingsByKey[setting.key] && settingsByKey[setting.key][0];
+        if (apiSetting) {
+          return {
+            placeholder: apiSetting.default,
+            ...apiSetting,
+            ...setting,
+          };
+        } else {
+          return setting;
+        }
+      });
+      sectionsWithAPISettings[slug] = { ...section, settings };
+    }
+    return sectionsWithAPISettings;
+  },
+);
 
 export const getActiveSectionName = (state, props) => props.params.splat;
 

frontend/src/metabase/admin/utils.js

+import { connect } from "react-redux";
 import { UserAuthWrapper } from "redux-auth-wrapper";
-import { routerActions } from "react-router-redux";
+import { routerActions, push } from "react-router-redux";
 import { canAccessPath } from "metabase/nav/utils";
+import { getUser } from "metabase/selectors/user";
 
 export const createAdminRouteGuard = (routeKey, Component) => {
   const Wrapper = UserAuthWrapper({
@@ -14,3 +16,26 @@ export const createAdminRouteGuard = (routeKey, Component) => {
 
   return Wrapper(Component ?? (({ children }) => children));
 };
+
+const mapStateToProps = state => ({
+  user: getUser(state),
+});
+
+const mapDispatchToProps = {
+  push,
+};
+
+export const createAdminRedirect = (adminPath, nonAdminPath) => {
+  const NonAdminRedirectComponent = connect(
+    mapStateToProps,
+    mapDispatchToProps,
+  )(({ user, push, location }) => {
+    const path = `${location.pathname}/${
+      user.is_superuser ? adminPath : nonAdminPath
+    }`;
+    push(path);
+    return null;
+  });
+
+  return NonAdminRedirectComponent;
+};

frontend/src/metabase/dashboard/components/DashboardActions.jsx

@@ -46,18 +46,13 @@ export const getDashboardActions = (
     dashboard.ordered_cards.some(dashCard => dashCard.card.display !== "text");
 
   const canShareDashboard = hasCards;
+  const canCreateSubscription = hasDataCards && canManageSubscriptions;
 
   if (!isEditing && !isEmpty && !isPublic) {
     // Getting notifications with static text-only cards doesn't make a lot of sense
-    if (hasDataCards) {
+    if (canCreateSubscription) {
       buttons.push(
-        <Tooltip
-          tooltip={
-            canManageSubscriptions
-              ? t`Subscriptions`
-              : t`You don't have permission to create a subscription for this dashboard`
-          }
-        >
+        <Tooltip tooltip={t`Subscriptions`}>
           <span>
             <DashboardHeaderButton
               disabled={!canManageSubscriptions}

frontend/src/metabase/query_builder/components/view/QuestionAlertWidget.jsx

@@ -36,13 +36,7 @@ export default class QuestionAlertWidget extends React.Component {
     const { isOpen, isFrozen } = this.state;
 
     if (!canManageSubscriptions) {
-      return (
-        <Icon
-          name="bell"
-          tooltip={t`You don't have permission to share data from this saved question`}
-          className={cx(className, "text-light")}
-        />
-      );
+      return null;
     }
 
     if (question.isSaved() && Object.values(questionAlerts).length > 0) {

frontend/test/metabase/scenarios/permissions/general-permissions.cy.spec.js

@@ -5,8 +5,9 @@ import {
   modifyPermission,
 } from "__support__/e2e/cypress";
 
-const SUBSCRIPTIONS_INDEX = 0;
+const SETTINGS_INDEX = 0;
 const MONITORING_INDEX = 1;
+const SUBSCRIPTIONS_INDEX = 2;
 
 describeEE("scenarios > admin > permissions > general", () => {
   beforeEach(() => {
@@ -34,33 +35,12 @@ describeEE("scenarios > admin > permissions > general", () => {
 
       it("revokes ability to create dashboard subscriptions", () => {
         cy.visit("/dashboard/1");
-        cy.icon("subscription")
-          .as("subscriptionsButton")
-          .realHover();
-
-        cy.findByText(
-          "You don't have permission to create a subscription for this dashboard",
-        );
-
-        cy.get("@subscriptionsButton").click();
-        cy.findByText("Create a dashboard subscription").should("not.exist");
+        cy.icon("subscription").should("not.exist");
       });
 
       it("revokes ability to create question alerts", () => {
         cy.visit("/question/1");
-        cy.icon("bell")
-          .as("subscriptionsButton")
-          .realHover();
-        cy.findByText(
-          "You don't have permission to share data from this saved question",
-        );
-
-        cy.findByText(
-          "To send alerts, an admin needs to set up email integration.",
-        ).should("not.exist");
-
-        cy.get("@subscriptionsButton").click();
-        cy.findByText("Create a dashboard subscription").should("not.exist");
+        cy.icon("bell").should("not.exist");
       });
     });
 
@@ -153,4 +133,45 @@ describeEE("scenarios > admin > permissions > general", () => {
       });
     });
   });
+
+  describe("settings permission", () => {
+    describe("granted", () => {
+      beforeEach(() => {
+        cy.visit("/admin/permissions/general");
+
+        modifyPermission("All Users", SETTINGS_INDEX, "Yes");
+
+        cy.button("Save changes").click();
+
+        modal().within(() => {
+          cy.findByText("Save permissions?");
+          cy.findByText("Are you sure you want to do this?");
+          cy.button("Yes").click();
+        });
+
+        cy.signInAsNormalUser();
+      });
+
+      it("allows editing settings as a non-admin user", () => {
+        cy.visit("/");
+        cy.icon("gear").click();
+
+        cy.findByText("Admin settings").click();
+
+        cy.url().should("include", "/admin/settings/general");
+
+        cy.findByText("License and Billing").should("not.exist");
+        cy.findByText("Setup").should("not.exist");
+        cy.findByText("Updates").should("not.exist");
+
+        // General smoke test
+        cy.get("#setting-site-name")
+          .clear()
+          .type("new name")
+          .blur();
+
+        cy.findByText("Saved");
+      });
+    });
+  });
 });

src/metabase/api/geojson.clj

@@ -2,6 +2,7 @@
   (:require [clojure.java.io :as io]
             [compojure.core :refer [GET]]
             [metabase.api.common :as api]
+            [metabase.api.common.validation :as validation]
             [metabase.models.setting :as setting :refer [defsetting]]
             [metabase.util.i18n :as ui18n :refer [deferred-tru tru]]
             [metabase.util.schema :as su]
@@ -114,7 +115,7 @@
   This behaves similarly to /api/geojson/:key but doesn't require the custom map to be saved to the DB first."
   [{{:keys [url]} :params} respond raise]
   {url su/NonBlankString}
-  (api/check-superuser)
+  (validation/check-has-general-permission :setting)
   (let [decoded-url (rc/url-decode url)]
     (try
       (when-not (valid-geojson-url? decoded-url)

src/metabase/api/ldap.clj

@@ -4,6 +4,7 @@
             [clojure.tools.logging :as log]
             [compojure.core :refer [PUT]]
             [metabase.api.common :as api]
+            [metabase.api.common.validation :as validation]
             [metabase.integrations.ldap :as ldap]
             [metabase.models.setting :as setting]
             [metabase.util.schema :as su]))
@@ -89,7 +90,7 @@
   "Update LDAP related settings. You must be a superuser to do this."
   [:as {settings :body}]
   {settings su/Map}
-  (api/check-superuser)
+  (validation/check-has-general-permission :setting)
   (let [ldap-settings (-> settings
                           (select-keys (keys mb-settings->ldap-details))
                           (assoc :ldap-port (when-let [^String ldap-port (not-empty (str (:ldap-port settings)))]

src/metabase/api/permissions.clj

@@ -4,6 +4,7 @@
             [compojure.core :refer [DELETE GET POST PUT]]
             [honeysql.helpers :as hh]
             [metabase.api.common :as api]
+            [metabase.api.common.validation :as validation]
             [metabase.api.permission-graph :as pg]
             [metabase.models.permissions :as perms]
             [metabase.models.permissions-group :as group :refer [PermissionsGroup]]
@@ -86,7 +87,7 @@
 (api/defendpoint GET "/group"
   "Fetch all `PermissionsGroups`, including a count of the number of `:members` in that group."
   []
-  (api/check-superuser)
+  (validation/check-has-general-permission :setting)
   (-> (ordered-groups offset-paging/*limit* offset-paging/*offset*)
       (hydrate :member_count)))
 

src/metabase/api/setup.clj

@@ -3,6 +3,7 @@
             [compojure.core :refer [GET POST]]
             [metabase.analytics.snowplow :as snowplow]
             [metabase.api.common :as api]
+            [metabase.api.common.validation :as validation]
             [metabase.api.database :as database-api :refer [DBEngineString]]
             [metabase.config :as config]
             [metabase.driver :as driver]
@@ -300,7 +301,7 @@
 (api/defendpoint GET "/admin_checklist"
   "Return various \"admin checklist\" steps and whether they've been completed. You must be a superuser to see this!"
   []
-  (api/check-superuser)
+  (validation/check-has-general-permission :setting)
   (admin-checklist))
 
 ;; User defaults endpoint

test/metabase/api/geojson_test.clj

@@ -10,7 +10,7 @@
             [metabase.util.schema :as su]
             [schema.core :as s]))
 
-(def ^:private ^String test-geojson-url
+(def ^String test-geojson-url
   "URL of a GeoJSON file used for test purposes."
   "https://raw.githubusercontent.com/metabase/metabase/master/test_resources/test.geojson")
 

test/metabase/api/ldap_test.clj

@@ -6,29 +6,37 @@
             [metabase.test :as mt]
             [metabase.test.integrations.ldap :as ldap.test]))
 
+(defn ldap-test-details
+  []
+  (-> (ldap.test/get-ldap-details)
+      (set/rename-keys (set/map-invert @#'ldap-api/mb-settings->ldap-details))
+      (assoc :ldap-enabled true)))
+
 (deftest ldap-settings-test
   (testing "PUT /api/ldap/settings"
     (ldap.test/with-ldap-server
-      (let [ldap-test-details (-> (ldap.test/get-ldap-details)
-                                  (set/rename-keys (set/map-invert @#'ldap-api/mb-settings->ldap-details))
-                                  (assoc :ldap-enabled true))]
-        (testing "Valid LDAP settings can be saved via an API call"
-          (mt/user-http-request :crowberto :put 200 "ldap/settings" ldap-test-details))
+      (testing "Valid LDAP settings can be saved via an API call"
+        (mt/user-http-request :crowberto :put 200 "ldap/settings" (ldap-test-details)))
+
+      (testing "Invalid LDAP settings return a server error"
+        (mt/user-http-request :crowberto :put 500 "ldap/settings"
+                              (assoc (ldap-test-details) :ldap-password "wrong-password")))
 
-        (testing "Invalid LDAP settings return a server error"
-          (mt/user-http-request :crowberto :put 500 "ldap/settings"
-                                (assoc ldap-test-details :ldap-password "wrong-password")))
+      (testing "Invalid LDAP settings can be saved if LDAP is disabled"
+        (mt/user-http-request :crowberto :put 200 "ldap/settings"
+                              (assoc (ldap-test-details) :ldap-password "wrong-password" :ldap-enabled false)))
 
-        (testing "Invalid LDAP settings can be saved if LDAP is disabled"
-          (mt/user-http-request :crowberto :put 200 "ldap/settings"
-                                (assoc ldap-test-details :ldap-password "wrong-password" :ldap-enabled false)))
+      (testing "Valid LDAP settings can still be saved if port is a integer (#18936)"
+        (mt/user-http-request :crowberto :put 200 "ldap/settings"
+                              (assoc (ldap-test-details)
+                                     :ldap-port (Integer. (ldap.test/get-ldap-port)))))
 
-        (testing "Valid LDAP settings can still be saved if port is a integer (#18936)"
-          (mt/user-http-request :crowberto :put 200 "ldap/settings"
-                                (assoc ldap-test-details
-                                       :ldap-port (Integer. (ldap.test/get-ldap-port)))))
+      (testing "LDAP port is saved as default value if passed as an empty string (#18936)"
+        (mt/user-http-request :crowberto :put 200 "ldap/settings"
+                              (assoc (ldap-test-details) :ldap-port "" :ldap-enabled false))
+        (is (= 389 (ldap/ldap-port))))
 
-        (testing "LDAP port is saved as default value if passed as an empty string (#18936)"
-          (mt/user-http-request :crowberto :put 200 "ldap/settings"
-                                (assoc ldap-test-details :ldap-port "" :ldap-enabled false))
-          (is (= 389 (ldap/ldap-port))))))))
+      (testing "Requires superusers"
+        (is (= "You don't have permissions to do that."
+               (mt/user-http-request :rasta :put 403 "ldap/settings"
+                                     (assoc (ldap-test-details) :ldap-port "" :ldap-enabled false))))))))