From ae2557c17f27ba1c7e2120e1195ba758f61266ca Mon Sep 17 00:00:00 2001
From: Alexander Lesnenko <alxnddr@users.noreply.github.com>
Date: Fri, 8 Apr 2022 13:35:46 -0400
Subject: [PATCH] FE: Settings access global permission + tweaks (#21460)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* 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>
---
.../advanced_permissions/api/setting_test.clj | 84 ++++++++++++++++-
.../general_permissions/index.ts | 9 +-
.../general_permissions/selectors.ts | 50 ++++------
.../general_permissions/types/permissions.ts | 2 +-
.../general_permissions/types/state.ts | 4 +-
.../general_permissions/types/user.ts | 6 +-
.../general_permissions/utils.ts | 9 +-
.../src/metabase-enterprise/license/index.js | 1 +
frontend/src/metabase/admin/routes.jsx | 9 +-
.../src/metabase/admin/settings/selectors.js | 57 +++++++-----
frontend/src/metabase/admin/utils.js | 27 +++++-
.../dashboard/components/DashboardActions.jsx | 11 +--
.../components/view/QuestionAlertWidget.jsx | 8 +-
.../general-permissions.cy.spec.js | 69 +++++++++-----
src/metabase/api/geojson.clj | 3 +-
src/metabase/api/ldap.clj | 3 +-
src/metabase/api/permissions.clj | 3 +-
src/metabase/api/setup.clj | 3 +-
test/metabase/api/geojson_test.clj | 2 +-
test/metabase/api/ldap_test.clj | 46 ++++++----
test/metabase/api/permissions_test.clj | 8 +-
test/metabase/api/premium_features_test.clj | 24 +++++
test/metabase/api/setup_test.clj | 91 ++++++++++---------
.../public_settings/premium_features_test.clj | 2 +-
24 files changed, 351 insertions(+), 180 deletions(-)
create mode 100644 test/metabase/api/premium_features_test.clj
diff --git a/enterprise/backend/test/metabase_enterprise/advanced_permissions/api/setting_test.clj b/enterprise/backend/test/metabase_enterprise/advanced_permissions/api/setting_test.clj
index 2a4d04aaa0f..2edff32c469 100644
--- a/enterprise/backend/test/metabase_enterprise/advanced_permissions/api/setting_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/advanced_permissions/api/setting_test.clj
@@ -1,13 +1,16 @@
(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))))))))
diff --git a/enterprise/frontend/src/metabase-enterprise/general_permissions/index.ts b/enterprise/frontend/src/metabase-enterprise/general_permissions/index.ts
index 6442604c2e0..cd8ba72dc6a 100644
--- a/enterprise/frontend/src/metabase-enterprise/general_permissions/index.ts
+++ b/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` }];
diff --git a/enterprise/frontend/src/metabase-enterprise/general_permissions/selectors.ts b/enterprise/frontend/src/metabase-enterprise/general_permissions/selectors.ts
index 687523e6567..4f71ff226e1 100644
--- a/enterprise/frontend/src/metabase-enterprise/general_permissions/selectors.ts
+++ b/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,
};
diff --git a/enterprise/frontend/src/metabase-enterprise/general_permissions/types/permissions.ts b/enterprise/frontend/src/metabase-enterprise/general_permissions/types/permissions.ts
index 0ec9be60311..73e3bcd6d30 100644
--- a/enterprise/frontend/src/metabase-enterprise/general_permissions/types/permissions.ts
+++ b/enterprise/frontend/src/metabase-enterprise/general_permissions/types/permissions.ts
@@ -1,6 +1,6 @@
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 = {
diff --git a/enterprise/frontend/src/metabase-enterprise/general_permissions/types/state.ts b/enterprise/frontend/src/metabase-enterprise/general_permissions/types/state.ts
index 397e161a687..b882d796241 100644
--- a/enterprise/frontend/src/metabase-enterprise/general_permissions/types/state.ts
+++ b/enterprise/frontend/src/metabase-enterprise/general_permissions/types/state.ts
@@ -1,9 +1,9 @@
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;
diff --git a/enterprise/frontend/src/metabase-enterprise/general_permissions/types/user.ts b/enterprise/frontend/src/metabase-enterprise/general_permissions/types/user.ts
index bee42b5934a..c0bd7197d9d 100644
--- a/enterprise/frontend/src/metabase-enterprise/general_permissions/types/user.ts
+++ b/enterprise/frontend/src/metabase-enterprise/general_permissions/types/user.ts
@@ -1,9 +1,9 @@
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;
};
}
diff --git a/enterprise/frontend/src/metabase-enterprise/general_permissions/utils.ts b/enterprise/frontend/src/metabase-enterprise/general_permissions/utils.ts
index 19ad1fd8a88..43c7580c640 100644
--- a/enterprise/frontend/src/metabase-enterprise/general_permissions/utils.ts
+++ b/enterprise/frontend/src/metabase-enterprise/general_permissions/utils.ts
@@ -1,4 +1,7 @@
-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;
diff --git a/enterprise/frontend/src/metabase-enterprise/license/index.js b/enterprise/frontend/src/metabase-enterprise/license/index.js
index 8a19890ea92..d3ab7f75252 100644
--- a/enterprise/frontend/src/metabase-enterprise/license/index.js
+++ b/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,
};
}),
);
diff --git a/frontend/src/metabase/admin/routes.jsx b/frontend/src/metabase/admin/routes.jsx
index e451ae24af1..97a9db7620e 100644
--- a/frontend/src/metabase/admin/routes.jsx
+++ b/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}
diff --git a/frontend/src/metabase/admin/settings/selectors.js b/frontend/src/metabase/admin/settings/selectors.js
index b5cc3550333..c219d4df425 100644
--- a/frontend/src/metabase/admin/settings/selectors.js
+++ b/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;
diff --git a/frontend/src/metabase/admin/utils.js b/frontend/src/metabase/admin/utils.js
index e83208555a7..21e7a3fb0fa 100644
--- a/frontend/src/metabase/admin/utils.js
+++ b/frontend/src/metabase/admin/utils.js
@@ -1,6 +1,8 @@
+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;
+};
diff --git a/frontend/src/metabase/dashboard/components/DashboardActions.jsx b/frontend/src/metabase/dashboard/components/DashboardActions.jsx
index 0c7f66ba41a..72a7e5928ed 100644
--- a/frontend/src/metabase/dashboard/components/DashboardActions.jsx
+++ b/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}
diff --git a/frontend/src/metabase/query_builder/components/view/QuestionAlertWidget.jsx b/frontend/src/metabase/query_builder/components/view/QuestionAlertWidget.jsx
index 3242c5edf5d..41b3ed23690 100644
--- a/frontend/src/metabase/query_builder/components/view/QuestionAlertWidget.jsx
+++ b/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) {
diff --git a/frontend/test/metabase/scenarios/permissions/general-permissions.cy.spec.js b/frontend/test/metabase/scenarios/permissions/general-permissions.cy.spec.js
index 4717beddc3b..e5c9f7a3ff9 100644
--- a/frontend/test/metabase/scenarios/permissions/general-permissions.cy.spec.js
+++ b/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");
+ });
+ });
+ });
});
diff --git a/src/metabase/api/geojson.clj b/src/metabase/api/geojson.clj
index 0bbfb81872b..79c9f35d7f3 100644
--- a/src/metabase/api/geojson.clj
+++ b/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)
diff --git a/src/metabase/api/ldap.clj b/src/metabase/api/ldap.clj
index 05ddb388859..b5dd74c5617 100644
--- a/src/metabase/api/ldap.clj
+++ b/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)))]
diff --git a/src/metabase/api/permissions.clj b/src/metabase/api/permissions.clj
index 7cc1143dfcd..db393549395 100644
--- a/src/metabase/api/permissions.clj
+++ b/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)))
diff --git a/src/metabase/api/setup.clj b/src/metabase/api/setup.clj
index 946ac51c7cd..810d5bab712 100644
--- a/src/metabase/api/setup.clj
+++ b/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
diff --git a/test/metabase/api/geojson_test.clj b/test/metabase/api/geojson_test.clj
index 2baae4336f5..54d28907153 100644
--- a/test/metabase/api/geojson_test.clj
+++ b/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")
diff --git a/test/metabase/api/ldap_test.clj b/test/metabase/api/ldap_test.clj
index 157743a3059..4ed6c834638 100644
--- a/test/metabase/api/ldap_test.clj
+++ b/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))))))))
diff --git a/test/metabase/api/permissions_test.clj b/test/metabase/api/permissions_test.clj
index 361528622c0..371df8e6b8a 100644
--- a/test/metabase/api/permissions_test.clj
+++ b/test/metabase/api/permissions_test.clj
@@ -36,7 +36,7 @@
(is (schema= {:id (s/eq (:id (group/admin)))
:name (s/eq "Administrators")
:member_count su/IntGreaterThanZero}
- (get id->group (:id (group/admin)))))))]
+ (get id->group (:id (group/admin)))))))]
(let [id->group (u/key-by :id (fetch-groups))]
(check-default-groups-returned id->group))
@@ -48,7 +48,11 @@
(is (schema= {:id su/IntGreaterThanZero
:name su/NonBlankString
:member_count (s/eq 0)}
- (get id->group (:id group)))))))))))
+ (get id->group (:id group))))))))
+
+ (testing "require superusers"
+ (is (= "You don't have permissions to do that."
+ (mt/user-http-request :rasta :get 403 "permissions/group")))))))
(deftest groups-list-limit-test
(testing "GET /api/permissions/group?limit=1&offset=1"
diff --git a/test/metabase/api/premium_features_test.clj b/test/metabase/api/premium_features_test.clj
new file mode 100644
index 00000000000..e13f085bf3b
--- /dev/null
+++ b/test/metabase/api/premium_features_test.clj
@@ -0,0 +1,24 @@
+(ns metabase.api.premium-features-test
+ (:require [clojure.test :refer :all]
+ [metabase.public-settings.premium-features :as premium-features]
+ [metabase.public-settings.premium-features-test :as premium-features-test]
+ [metabase.test :as mt]))
+
+(deftest get-token-status-test
+ (testing "GET /api/premium-features/token/status"
+ (with-redefs [premium-features/fetch-token-status (fn [_x]
+ {:valid true
+ :status "fake"
+ :features ["test" "fixture"]
+ :trial false})]
+ (mt/with-temporary-setting-values [:premium-embedding-token premium-features-test/random-fake-token]
+ (testing "returns correctly"
+ (is (= {:valid true
+ :status "fake"
+ :features ["test" "fixture"]
+ :trial false}
+ (mt/user-http-request :crowberto :get 200 "premium-features/token/status"))))
+
+ (testing "requires superusers"
+ (is (= "You don't have permissions to do that."
+ (mt/user-http-request :rasta :get 403 "premium-features/token/status"))))))))
diff --git a/test/metabase/api/setup_test.clj b/test/metabase/api/setup_test.clj
index 9b4c645d8ac..2e0d10bb311 100644
--- a/test/metabase/api/setup_test.clj
+++ b/test/metabase/api/setup_test.clj
@@ -391,49 +391,54 @@
;; basic sanity check
(deftest admin-checklist-test
- (with-redefs [db/exists? (constantly true)
- db/count (constantly 5)
- email/email-configured? (constantly true)
- slack/slack-configured? (constantly false)]
- (is (= [{:name "Get connected"
- :tasks [{:title "Add a database"
- :completed true
- :triggered true
- :is_next_step false}
- {:title "Set up email"
- :completed true
- :triggered true
- :is_next_step false}
- {:title "Set Slack credentials"
- :completed false
- :triggered true
- :is_next_step true}
- {:title "Invite team members"
- :completed true
- :triggered true
- :is_next_step false}]}
- {:name "Curate your data"
- :tasks [{:title "Hide irrelevant tables"
- :completed true
- :triggered false
- :is_next_step false}
- {:title "Organize questions"
- :completed true
- :triggered false
- :is_next_step false}
- {:title "Create metrics"
- :completed true
- :triggered false
- :is_next_step false}
- {:title "Create segments"
- :completed true
- :triggered false
- :is_next_step false}]}]
- (for [{group-name :name, tasks :tasks} (#'setup-api/admin-checklist)]
- {:name (str group-name)
- :tasks (for [task tasks]
- (-> (select-keys task [:title :completed :triggered :is_next_step])
- (update :title str)))})))))
+ (testing "GET /api/setup/admin_checklist"
+ (with-redefs [db/exists? (constantly true)
+ db/count (constantly 5)
+ email/email-configured? (constantly true)
+ slack/slack-configured? (constantly false)]
+ (is (= [{:name "Get connected"
+ :tasks [{:title "Add a database"
+ :completed true
+ :triggered true
+ :is_next_step false}
+ {:title "Set up email"
+ :completed true
+ :triggered true
+ :is_next_step false}
+ {:title "Set Slack credentials"
+ :completed false
+ :triggered true
+ :is_next_step true}
+ {:title "Invite team members"
+ :completed true
+ :triggered true
+ :is_next_step false}]}
+ {:name "Curate your data"
+ :tasks [{:title "Hide irrelevant tables"
+ :completed true
+ :triggered false
+ :is_next_step false}
+ {:title "Organize questions"
+ :completed true
+ :triggered false
+ :is_next_step false}
+ {:title "Create metrics"
+ :completed true
+ :triggered false
+ :is_next_step false}
+ {:title "Create segments"
+ :completed true
+ :triggered false
+ :is_next_step false}]}]
+ (for [{group-name :name, tasks :tasks} (mt/user-http-request :crowberto :get 200 "setup/admin_checklist")]
+ {:name (str group-name)
+ :tasks (for [task tasks]
+ (-> (select-keys task [:title :completed :triggered :is_next_step])
+ (update :title str)))}))))
+
+ (testing "require superusers"
+ (is (= "You don't have permissions to do that."
+ (mt/user-http-request :rasta :get 403 "setup/admin_checklist"))))))
(deftest user-defaults-test
(testing "with no user defaults configured"
diff --git a/test/metabase/public_settings/premium_features_test.clj b/test/metabase/public_settings/premium_features_test.clj
index da76f52c34e..926006f9624 100644
--- a/test/metabase/public_settings/premium_features_test.clj
+++ b/test/metabase/public_settings/premium_features_test.clj
@@ -43,7 +43,7 @@
:features ["test" "fixture"]
:trial false}))
-(def ^:private random-fake-token
+(def random-fake-token
"d7ad0b5f9ddfd1953b1b427b75d620e4ba91d38e7bcbc09d8982480863dbc611")
(deftest fetch-token-status-test
--
GitLab