[Cmd Palette] Do not show admin and settings links to users who do not have...

[Cmd Palette] Do not show admin and settings links to users who do not have access (#50887) (#51592)

* fixes showing admin links to not admins in the command palette

* updates calculation to take into account EE advanced permissions allowing certain users permissions to change settings who are not admins

* admin routes for non-admins

* fix unit tests

* actually fix unit tests

* adding checks for other permissions

---------

Co-authored-by: Sloan Sparger <sloansparger@users.noreply.github.com>
Co-authored-by: Nick Fitzpatrick <nickfitz.582@gmail.com>

e2e/test/scenarios/onboarding/command-palette.cy.spec.js

@@ -4,6 +4,7 @@ import {
   ORDERS_COUNT_QUESTION_ID,
   ORDERS_DASHBOARD_ID,
 } from "e2e/support/cypress_sample_instance_data";
+import { describeEE } from "e2e/support/helpers";
 
 const { admin } = USERS;
 
@@ -150,30 +151,143 @@ describe("command palette", () => {
     });
   });
 
-  it("should render links to site settings in settings pages", () => {
-    cy.visit("/admin");
-    cy.findByRole("heading", { name: "Getting set up" }).should("exist");
-    H.openCommandPalette();
+  describe("admin settings links", () => {
+    it("should render links to all admin settings pages for admins", () => {
+      cy.visit("/");
+      cy.findByTestId("home-page")
+        .findByText(/see what metabase can do/i)
+        .should("exist");
+
+      H.openCommandPalette();
+      H.commandPalette().within(() => {
+        H.commandPaletteInput().type("Settings -");
+        cy.log("check admin sees all settings links");
+        H.commandPaletteAction("Settings - Setup").should("exist");
+        H.commandPaletteAction("Settings - General").should("exist");
+        H.commandPaletteInput().clear();
+
+        cy.log("shouldsee admin links");
+        H.commandPaletteInput().type("Performance");
+        H.commandPaletteAction("Performance").should("exist");
+      });
+    });
 
-    H.commandPalette().within(() => {
-      H.commandPaletteInput().type("Custom Homepage");
-      cy.findByRole("option", { name: "Custom Homepage" }).click();
+    it("should not render any links to settings or admin pages for non-admins without privledged access", () => {
+      cy.signInAsNormalUser();
+      cy.visit("/");
+      cy.findByTestId("home-page")
+        .findByText(/see what metabase can do/i)
+        .should("exist");
+
+      H.openCommandPalette();
+      H.commandPalette().within(() => {
+        cy.log("check normal user does not see any setting links");
+        H.commandPaletteInput().type("Settings -");
+        H.commandPaletteAction("Settings - Setup").should("not.exist");
+        H.commandPaletteAction("Settings - General").should("not.exist");
+        H.commandPaletteInput().clear();
+
+        cy.log("should not see admin links");
+        H.commandPaletteInput().type("Performance");
+        H.commandPaletteAction("Performance").should("not.exist");
+        H.commandPaletteInput().clear();
+
+        // Tools and Troubleshooting
+
+        H.commandPaletteInput().type("Troub");
+        H.commandPaletteAction("Troubleshooting").should("not.exist");
+        H.commandPaletteInput().clear().type("tool");
+        H.commandPaletteAction("Tools").should("not.exist");
+        H.commandPaletteInput().clear();
+
+        //Database and table metadata
+
+        H.commandPaletteInput().type("data");
+        H.commandPaletteAction("Databases").should("not.exist");
+        H.commandPaletteInput().clear().type("tabl");
+        H.commandPaletteAction("Table Metadata").should("not.exist");
+      });
     });
 
-    cy.findByTestId("custom-homepage-setting").should("be.visible");
+    describeEE("with advanced permissions", () => {
+      it("should render links for non-admins that have specific privileges", () => {
+        // setup
+        cy.log("setup permissions");
 
-    cy.location("pathname").should("contain", "settings/general");
-    cy.location("hash").should("contain", "#custom-homepage");
+        H.setTokenFeatures("all");
+        cy.visit("/admin/permissions/application");
 
-    H.openCommandPalette();
+        const SETTINGS_INDEX = 0;
+        const MONITORING_INDEX = 1;
+        H.modifyPermission("All Users", SETTINGS_INDEX, "Yes");
+        H.modifyPermission("All Users", MONITORING_INDEX, "Yes");
 
-    H.commandPalette().within(() => {
-      H.commandPaletteInput().clear().type("Week");
-      cy.findByRole("option", { name: "First day of the week" }).click();
-    });
+        cy.button("Save changes").click();
+
+        H.modal().within(() => {
+          cy.findByText("Save permissions?");
+          cy.findByText("Are you sure you want to do this?");
+          cy.button("Yes").click();
+        });
 
-    cy.location("pathname").should("contain", "settings/localization");
-    cy.location("hash").should("contain", "#start-of-week");
+        cy.findByRole("radiogroup").findByText("Data").click();
+        cy.findByRole("menuitem", { name: "All Users" }).click();
+
+        const TABLE_METADATA_INDEX = 3;
+        const DATABASE_INDEX = 4;
+
+        H.modifyPermission("Sample Database", TABLE_METADATA_INDEX, "Yes");
+        H.modifyPermission("Sample Database", DATABASE_INDEX, "Yes");
+
+        cy.button("Save changes").click();
+
+        H.modal().within(() => {
+          cy.findByText("Save permissions?");
+          cy.findByText("Are you sure you want to do this?");
+          cy.button("Yes").click();
+        });
+
+        cy.signInAsNormalUser();
+
+        // test
+        cy.visit("/");
+        cy.findByTestId("home-page")
+          .findByText(/see what metabase can do/i)
+          .should("exist");
+
+        H.openCommandPalette();
+        H.commandPalette().within(() => {
+          // Settings Pages
+          H.commandPaletteInput().type("Settings -");
+          cy.log(
+            "check user with settings permissions see non-admin restricted settings links",
+          );
+          H.commandPaletteAction("Settings - Setup").should("not.exist");
+          H.commandPaletteAction("Settings - General").should("exist");
+          H.commandPaletteInput().clear();
+
+          // Tools and Troubleshooting
+
+          H.commandPaletteInput().type("Troub");
+          H.commandPaletteAction("Troubleshooting").should("exist");
+          H.commandPaletteInput().clear().type("tool");
+          H.commandPaletteAction("Tools").should("exist");
+          H.commandPaletteInput().clear();
+
+          //Database and table metadata
+
+          H.commandPaletteInput().type("data");
+          H.commandPaletteAction("Databases").should("exist");
+          H.commandPaletteInput().clear().type("tabl");
+          H.commandPaletteAction("Table Metadata").should("exist");
+          H.commandPaletteInput().clear();
+
+          cy.log("should not see other admin links");
+          H.commandPaletteInput().type("Performance");
+          H.commandPaletteAction("Performance").should("not.exist");
+        });
+      });
+    });
   });
 
   it("should not be accessible when doing full app embedding", () => {

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

@@ -9,7 +9,7 @@ import { hasPremiumFeature } from "metabase-enterprise/settings";
 
 import applicationPermissionsReducer from "./reducer";
 import getRoutes from "./routes";
-import { canManageSubscriptions } from "./selectors";
+import { canAccessSettings, canManageSubscriptions } from "./selectors";
 import {
   monitoringPermissionAllowedPathGetter,
   settingsPermissionAllowedPathGetter,
@@ -25,6 +25,7 @@ if (hasPremiumFeature("advanced_permissions")) {
   ];
 
   PLUGIN_APPLICATION_PERMISSIONS.selectors = {
+    canAccessSettings,
     canManageSubscriptions,
   };
   PLUGIN_REDUCERS.applicationPermissionsPlugin = applicationPermissionsReducer;

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

@@ -37,6 +37,11 @@ export const canManageSubscriptions = createSelector(
   user => user?.permissions?.can_access_subscription ?? false,
 );
 
+export const canAccessSettings = createSelector(
+  (state: ApplicationPermissionsState) => state.currentUser,
+  user => user?.permissions?.can_access_setting ?? false,
+);
+
 const getApplicationPermission = (
   permissions: ApplicationPermissions,
   groupId: number,

frontend/src/metabase/palette/components/test/PaletteResults/PaletteResults-oss.unit.spec.tsx

@@ -123,18 +123,36 @@ describe("PaletteResults", () => {
     expect(fetchMock.calls("path:/api/search").length).toBe(2);
   });
 
-  it("should provide links to settings pages", async () => {
-    setup({ query: "setu" });
+  it("should provide links to settings pages for admins", async () => {
+    setup({ query: "setu", isAdmin: true });
     expect(await screen.findByText("Admin")).toBeInTheDocument();
     expect(await screen.findByText("Settings - Setup")).toBeInTheDocument();
   });
 
-  it("should provide links to admin pages", async () => {
-    setup({ query: "permi" });
+  it("should not provide links to settings pages for non-admins", async () => {
+    setup({ query: "setu", isAdmin: false });
+    expect(
+      await screen.findByText(`Search documentation for "setu"`),
+    ).toBeInTheDocument();
+    expect(screen.queryByText("Admin")).not.toBeInTheDocument();
+    expect(screen.queryByText("Settings - Setup")).not.toBeInTheDocument();
+  });
+
+  it("should provide links to admin pages for admins", async () => {
+    setup({ query: "permi", isAdmin: true });
     expect(await screen.findByText("Admin")).toBeInTheDocument();
     expect(await screen.findByText("Permissions")).toBeInTheDocument();
   });
 
+  it("should not provide links to admin pages for non-admins", async () => {
+    setup({ query: "permi", isAdmin: false });
+    expect(
+      await screen.findByText(`Search documentation for "permi"`),
+    ).toBeInTheDocument();
+    expect(screen.queryByText("Admin")).not.toBeInTheDocument();
+    expect(screen.queryByText("Permissions")).not.toBeInTheDocument();
+  });
+
   it("should not compute search results if 'search-typeahead-enabled' is diabled", async () => {
     setup({ query: "ques", settings: { "search-typeahead-enabled": false } });
     expect(

frontend/src/metabase/palette/components/test/PaletteResults/setup.tsx

@@ -24,6 +24,7 @@ import {
   createMockDatabase,
   createMockRecentCollectionItem,
   createMockTokenFeatures,
+  createMockUser,
 } from "metabase-types/api/mocks";
 import {
   createMockAdminAppState,
@@ -106,6 +107,7 @@ export interface CommonSetupProps {
   settings?: Partial<Settings>;
   recents?: RecentItem[];
   isEE?: boolean;
+  isAdmin?: boolean;
 }
 
 export const commonSetup = ({
@@ -113,18 +115,25 @@ export const commonSetup = ({
   settings = {},
   recents = [recents_1, recents_2],
   isEE,
+  isAdmin = false,
 }: CommonSetupProps = {}) => {
   setupDatabasesEndpoints([DATABASE]);
   setupSearchEndpoints([model_1, model_2, dashboard]);
   setupRecentViewsEndpoints(recents);
+  const adminState = isAdmin
+    ? createMockAdminState({
+        app: createMockAdminAppState({
+          paths: getAdminPaths(),
+        }),
+      })
+    : createMockAdminState();
 
   const storeInitialState = createMockState({
-    admin: createMockAdminState({
-      app: createMockAdminAppState({
-        paths: getAdminPaths(),
-      }),
-    }),
+    admin: adminState,
     settings: mockSettings({ ...settings, "token-features": TOKEN_FEATURES }),
+    currentUser: createMockUser({
+      is_superuser: isAdmin,
+    }),
   });
 
   if (isEE) {

frontend/src/metabase/palette/hooks/useCommandPalette.tsx

@@ -24,6 +24,7 @@ import {
   getDocsUrl,
   getSettings,
 } from "metabase/selectors/settings";
+import { canAccessSettings, getUserIsAdmin } from "metabase/selectors/user";
 import { getShowMetabaseLinks } from "metabase/selectors/whitelabel";
 import { Icon, type IconName } from "metabase/ui";
 import {
@@ -43,6 +44,10 @@ export const useCommandPalette = ({
   const dispatch = useDispatch();
   const docsUrl = useSelector(state => getDocsUrl(state, {}));
   const showMetabaseLinks = useSelector(getShowMetabaseLinks);
+
+  const isAdmin = useSelector(getUserIsAdmin);
+  const canUserAccessSettings = useSelector(canAccessSettings);
+
   const isSearchTypeaheadEnabled = useSetting("search-typeahead-enabled");
 
   // Used for finding actions within the list
@@ -258,9 +263,10 @@ export const useCommandPalette = ({
   const adminActions = useMemo<PaletteAction[]>(() => {
     // Subpaths - i.e. paths to items within the main Admin tabs - are needed
     // in the command palette but are not part of the main list of admin paths
-    const adminSubpaths = getPerformanceAdminPaths(
-      PLUGIN_CACHING.getTabMetadata(),
-    );
+    const adminSubpaths = isAdmin
+      ? getPerformanceAdminPaths(PLUGIN_CACHING.getTabMetadata())
+      : [];
+
     const paths = [...adminPaths, ...adminSubpaths];
     return paths.map(adminPath => ({
       id: `admin-page-${adminPath.key}`,
@@ -269,15 +275,23 @@ export const useCommandPalette = ({
       perform: () => dispatch(push(adminPath.path)),
       section: "admin",
     }));
-  }, [adminPaths, dispatch]);
+  }, [isAdmin, adminPaths, dispatch]);
+
+  const settingsActions = useMemo<PaletteAction[]>(() => {
+    if (!canUserAccessSettings) {
+      return [];
+    }
 
-  const adminSettingsActions = useMemo<PaletteAction[]>(() => {
     return Object.entries(settingsSections)
       .filter(([slug, section]) => {
         if (section.getHidden?.(settingValues)) {
           return false;
         }
 
+        if (section.adminOnly && !isAdmin) {
+          return false;
+        }
+
         return !slug.includes("/");
       })
       .map(([slug, section]) => ({
@@ -287,12 +301,19 @@ export const useCommandPalette = ({
         perform: () => dispatch(push(`/admin/settings/${slug}`)),
         section: "admin",
       }));
-  }, [settingsSections, settingValues, dispatch]);
+  }, [
+    canUserAccessSettings,
+    isAdmin,
+    settingsSections,
+    settingValues,
+    dispatch,
+  ]);
 
-  useRegisterActions(
-    hasQuery ? [...adminActions, ...adminSettingsActions] : [],
-    [adminActions, adminSettingsActions, hasQuery],
-  );
+  useRegisterActions(hasQuery ? [...adminActions, ...settingsActions] : [], [
+    adminActions,
+    settingsActions,
+    hasQuery,
+  ]);
 };
 
 export const getSearchResultSubtext = (wrappedSearchResult: any) => {

frontend/src/metabase/plugins/index.ts

@@ -479,6 +479,7 @@ export const PLUGIN_APPLICATION_PERMISSIONS = {
   getRoutes: (): ReactNode => null,
   tabs: [] as any,
   selectors: {
+    canAccessSettings: (_state: any) => false,
     canManageSubscriptions: (_state: any) => true,
   },
 };

frontend/src/metabase/selectors/user.ts

@@ -21,6 +21,14 @@ export const canManageSubscriptions = createSelector(
   (isAdmin, canManageSubscriptions) => isAdmin || canManageSubscriptions,
 );
 
+export const canAccessSettings = createSelector(
+  [
+    getUserIsAdmin,
+    state => PLUGIN_APPLICATION_PERMISSIONS.selectors.canAccessSettings(state),
+  ],
+  (isAdmin, canAccessSettings) => isAdmin || canAccessSettings,
+);
+
 export const getUserAttributes = createSelector(
   [getUser],
   user => user?.login_attributes || {},