polish connection impersonation FE (#31708)

e2e/test/scenarios/permissions/impersonated.cy.spec.js

@@ -2,6 +2,7 @@ import {
   assertPermissionTable,
   createTestRoles,
   describeEE,
+  isPermissionDisabled,
   modifyPermission,
   openNativeEditor,
   popover,
@@ -14,6 +15,7 @@ const { ALL_USERS_GROUP } = USER_GROUPS;
 
 const PG_DB_ID = 2;
 const DATA_ACCESS_PERMISSION_INDEX = 0;
+const NATIVE_QUERIES_PERMISSION_INDEX = 1;
 
 describeEE("impersonated permission", () => {
   describe("admins", () => {
@@ -38,6 +40,7 @@ describeEE("impersonated permission", () => {
       );
 
       selectImpersonatedAttribute("role");
+      saveImpersonationSettings();
       savePermissions();
 
       assertPermissionTable([
@@ -59,6 +62,62 @@ describeEE("impersonated permission", () => {
         ],
       ]);
 
+      // Checking it shows the right state on the tables level
+      cy.get("main").findByText("QA Postgres12").click();
+
+      assertPermissionTable([
+        ["Accounts", "Impersonated", "No", "1 million rows", "No", "No"],
+        ["Analytic Events", "Impersonated", "No", "1 million rows", "No", "No"],
+        ["Feedback", "Impersonated", "No", "1 million rows", "No", "No"],
+        ["Invoices", "Impersonated", "No", "1 million rows", "No", "No"],
+        ["Orders", "Impersonated", "No", "1 million rows", "No", "No"],
+        [
+          "People",
+          "Impersonated",
+          "No", // FIXME: should be "Yes"
+          "1 million rows",
+          "No",
+          "No",
+        ],
+        [
+          "Products",
+          "Impersonated",
+          "No", // FIXME: should be "Yes"
+          "1 million rows",
+          "No",
+          "No",
+        ],
+        [
+          "Reviews",
+          "Impersonated",
+          "No", // FIXME: should be "Yes"
+          "1 million rows",
+          "No",
+          "No",
+        ],
+      ]);
+
+      cy.get("main")
+        .findByText("Orders")
+        .closest("tr")
+        .within(() => {
+          isPermissionDisabled(
+            DATA_ACCESS_PERMISSION_INDEX,
+            "Impersonated",
+            true,
+          ).click();
+          isPermissionDisabled(NATIVE_QUERIES_PERMISSION_INDEX, "No", true);
+
+          cy.findAllByText("No").eq(0).realHover();
+        });
+
+      // eslint-disable-next-line no-unscoped-text-selectors
+      cy.findByText(
+        "Native query editor access requires full data access.",
+      ).should("not.exist");
+
+      cy.get("main").findByText("All Users group").click();
+
       // Edit impersonated permission
       modifyPermission(
         "QA Postgres12",
@@ -67,6 +126,7 @@ describeEE("impersonated permission", () => {
       );
 
       selectImpersonatedAttribute("attr_uid");
+      saveImpersonationSettings();
       savePermissions();
 
       assertPermissionTable([
@@ -114,6 +174,49 @@ describeEE("impersonated permission", () => {
         ],
       ]);
     });
+
+    it("impersonation modal should be positioned behind the page leave confirmation modal", () => {
+      // Try leaving the page
+      cy.visit("/admin/permissions/data/group/1");
+
+      modifyPermission(
+        "QA Postgres12",
+        DATA_ACCESS_PERMISSION_INDEX,
+        "Impersonated",
+      );
+
+      selectImpersonatedAttribute("role");
+      saveImpersonationSettings();
+
+      modifyPermission(
+        "QA Postgres12",
+        DATA_ACCESS_PERMISSION_INDEX,
+        "Edit Impersonated",
+      );
+
+      cy.findByRole("dialog").findByText("Edit settings").click();
+
+      // Page leave confirmation should be on top
+      cy.findAllByRole("dialog")
+        .eq(0)
+        .as("leaveConfirmation")
+        .findByText("Discard your unsaved changes?")
+        .should("be.visible");
+
+      // Cancel
+      cy.get("@leaveConfirmation").findByText("Cancel").click();
+
+      // Ensure the impersonation modal is still open
+      cy.findByRole("dialog")
+        .findByText("Map a user attribute to database roles")
+        .should("be.visible");
+
+      // Go to settings
+      cy.findByRole("dialog").findByText("Edit settings").click();
+      cy.get("@leaveConfirmation").findByText("Discard changes").click();
+
+      cy.focused().should("have.attr", "placeholder", "username");
+    });
   });
 
   describe("impersonated users", () => {
@@ -200,5 +303,8 @@ function selectImpersonatedAttribute(attribute) {
   });
 
   popover().findByText(attribute).click();
+}
+
+function saveImpersonationSettings() {
   cy.findByRole("dialog").findByText("Save").click();
 }

enterprise/frontend/src/metabase-enterprise/advanced_permissions/components/ImpersonationModal/ImpersonationModal.tsx

@@ -52,9 +52,11 @@ const _ImpersonationModal = ({ route, params }: ImpersonationModalProps) => {
     async (
       groupId: number,
       databaseId: number,
-    ): Promise<Impersonation | undefined> => {
-      return ImpersonationApi.get({ db_id: databaseId, group_id: groupId });
-    },
+    ): Promise<Impersonation | undefined> =>
+      ImpersonationApi.get({
+        db_id: databaseId,
+        group_id: groupId,
+      }),
     [],
   );
 
@@ -85,16 +87,16 @@ const _ImpersonationModal = ({ route, params }: ImpersonationModalProps) => {
 
   const handleSave = useCallback(
     attribute => {
-      if (attribute !== selectedAttribute) {
-        dispatch(
-          updateDataPermission({
-            groupId,
-            permission: { type: "access", permission: "data" },
-            value: "impersonated",
-            entityId: { databaseId },
-          }),
-        );
+      dispatch(
+        updateDataPermission({
+          groupId,
+          permission: { type: "access", permission: "data" },
+          value: "impersonated",
+          entityId: { databaseId },
+        }),
+      );
 
+      if (attribute !== selectedAttribute) {
         dispatch(
           updateImpersonation({
             attribute,

enterprise/frontend/src/metabase-enterprise/advanced_permissions/components/ImpersonationModal/ImpersonationModal.unit.spec.tsx

+import { combineReducers } from "@reduxjs/toolkit";
+import { waitForElementToBeRemoved } from "@testing-library/react";
+import { Route } from "react-router";
+import userEvent from "@testing-library/user-event";
+import fetchMock from "fetch-mock";
+import { renderWithProviders, screen, waitFor } from "__support__/ui";
+import { ImpersonationModal } from "metabase-enterprise/advanced_permissions/components/ImpersonationModal/ImpersonationModal";
+import { shared } from "metabase-enterprise/shared/reducer";
+import { advancedPermissionsSlice } from "metabase-enterprise/advanced_permissions/reducer";
+import {
+  setupDatabaseEndpoints,
+  setupUserAttributesEndpoint,
+} from "__support__/server-mocks";
+import { createMockDatabase, createMockTable } from "metabase-types/api/mocks";
+import {
+  setupExistingImpersonationEndpoint,
+  setupMissingImpersonationEndpoint,
+} from "__support__/server-mocks/impersonation";
+import { createMockImpersonation } from "metabase-types/api/mocks/permissions";
+import { getImpersonations } from "metabase-enterprise/advanced_permissions/selectors";
+import { AdvancedPermissionsStoreState } from "metabase-enterprise/advanced_permissions/types";
+
+const groupId = 2;
+const databaseId = 1;
+const selectedAttribute = "foo";
+const defaultUserAttributes = ["foo", "bar"];
+
+const setup = async ({
+  userAttributes = defaultUserAttributes,
+  hasImpersonation = true,
+} = {}) => {
+  const database = createMockDatabase({
+    id: databaseId,
+    tables: [createMockTable()],
+  });
+  setupDatabaseEndpoints(database);
+  fetchMock.get(
+    {
+      url: `path:/api/database/${databaseId}/metadata`,
+      query: { include_hidden: true },
+    },
+    database,
+  );
+  setupUserAttributesEndpoint(userAttributes);
+
+  if (hasImpersonation) {
+    setupExistingImpersonationEndpoint(
+      createMockImpersonation({
+        db_id: databaseId,
+        group_id: groupId,
+        attribute: selectedAttribute,
+      }),
+    );
+  } else {
+    setupMissingImpersonationEndpoint(databaseId, groupId);
+  }
+
+  const { store } = renderWithProviders(
+    <>
+      <Route path="/" />
+      <Route
+        path="database/:databaseId/impersonated/group/:groupId"
+        component={ImpersonationModal}
+      />
+    </>,
+    {
+      initialRoute: `database/${databaseId}/impersonated/group/${groupId}`,
+      withRouter: true,
+      customReducers: {
+        plugins: combineReducers({
+          shared: shared.reducer,
+          advancedPermissionsPlugin: advancedPermissionsSlice.reducer,
+        }),
+      },
+    },
+  );
+
+  await waitForElementToBeRemoved(() => screen.queryByText("Loading..."));
+
+  return store;
+};
+
+describe("impersonation modal", () => {
+  it("should render the content", async () => {
+    await setup();
+    expect(
+      await screen.findByText("Map a user attribute to database roles"),
+    ).toBeInTheDocument();
+
+    expect(
+      await screen.findByText(
+        "When the person runs a query (including native queries), Metabase will impersonate the privileges of the database role you associate with the user attribute.",
+      ),
+    ).toBeInTheDocument();
+
+    expect(
+      await screen.findByRole("link", { name: /learn more/i }),
+    ).toHaveAttribute(
+      "href",
+      "https://www.metabase.com/docs/latest/permissions/data.html",
+    );
+
+    expect(
+      await screen.findByText(
+        "Make sure the main database credential has access to everything different user groups may need access to. It's what Metabase uses to sync table information.",
+      ),
+    ).toBeInTheDocument();
+
+    expect(
+      await screen.findByRole("link", { name: /edit settings/i }),
+    ).toHaveAttribute("href", "/admin/databases/1");
+  });
+
+  it("should not update impersonation if it has not changed", async () => {
+    const store = await setup({ userAttributes: ["foo"] });
+
+    userEvent.click(screen.getByText(/save/i));
+
+    expect(
+      getImpersonations(store.getState() as AdvancedPermissionsStoreState),
+    ).toHaveLength(0);
+  });
+
+  it("should create impersonation", async () => {
+    const store = await setup({ hasImpersonation: false });
+
+    userEvent.click(await screen.findByText(/pick a user attribute/i));
+    userEvent.click(await screen.findByText("foo"));
+
+    expect(await screen.findByRole("button", { name: /save/i })).toBeEnabled();
+    userEvent.click(await screen.findByRole("button", { name: /save/i }));
+
+    await waitFor(() => {
+      expect(
+        getImpersonations(store.getState() as AdvancedPermissionsStoreState),
+      ).toStrictEqual([
+        {
+          attribute: "foo",
+          db_id: 1,
+          group_id: 2,
+        },
+      ]);
+    });
+  });
+
+  it("should update impersonation", async () => {
+    const store = await setup();
+
+    userEvent.click(await screen.findByText(selectedAttribute));
+    userEvent.click(await screen.findByText("bar"));
+
+    expect(await screen.findByRole("button", { name: /save/i })).toBeEnabled();
+    userEvent.click(await screen.findByRole("button", { name: /save/i }));
+
+    await waitFor(() => {
+      expect(
+        getImpersonations(store.getState() as AdvancedPermissionsStoreState),
+      ).toStrictEqual([
+        {
+          attribute: "bar",
+          db_id: 1,
+          group_id: 2,
+        },
+      ]);
+    });
+  });
+
+  it("should show only already selected attribute if attributes array is empty", async () => {
+    await setup({ hasImpersonation: true, userAttributes: [] });
+
+    await screen.findByText(selectedAttribute);
+    expect(await screen.findByRole("button", { name: /save/i })).toBeEnabled();
+  });
+
+  it("should show the link to people settings if there is no impersonation and no attributes", async () => {
+    await setup({ hasImpersonation: false, userAttributes: [] });
+
+    expect(
+      await screen.findByText(
+        "To associate a user with a database role, you'll need to give that user at least one user attribute.",
+      ),
+    ).toBeInTheDocument();
+
+    expect(
+      await screen.findByRole("link", { name: /edit user settings/i }),
+    ).toHaveAttribute("href", "/admin/people");
+
+    expect(await screen.findByRole("button", { name: /close/i })).toBeEnabled();
+  });
+});

enterprise/frontend/src/metabase-enterprise/advanced_permissions/components/ImpersonationModal/ImpersonationModalView.tsx

@@ -46,11 +46,19 @@ export const ImpersonationModalView = ({
       (attributes.length === 1 ? attributes[0] : undefined),
   };
 
-  const hasAttributes = attributes.length > 0;
-  const attributeOptions = useMemo(
-    () => attributes.map(attribute => ({ name: attribute, value: attribute })),
-    [attributes],
-  );
+  const attributeOptions = useMemo(() => {
+    const selectableAttributes =
+      selectedAttribute && !attributes.includes(selectedAttribute)
+        ? [selectedAttribute, ...attributes]
+        : attributes;
+
+    return selectableAttributes.map(attribute => ({
+      name: attribute,
+      value: attribute,
+    }));
+  }, [attributes, selectedAttribute]);
+
+  const hasAttributes = attributeOptions.length > 0;
 
   const handleSubmit = ({ attribute }: { attribute?: UserAttribute }) => {
     if (attribute != null) {

frontend/src/metabase-types/api/mocks/permissions.ts

@@ -2,6 +2,7 @@ import {
   Database,
   Group,
   GroupsPermissions,
+  Impersonation,
   PermissionsGraph,
 } from "metabase-types/api";
 
@@ -36,3 +37,14 @@ export const createMockPermissionsGraph = ({
     revision: 1,
   };
 };
+
+export const createMockImpersonation = (
+  data: Partial<Impersonation>,
+): Impersonation => {
+  return {
+    db_id: 1,
+    group_id: 1,
+    attribute: "foo",
+    ...data,
+  };
+};

frontend/src/metabase/admin/permissions/components/PermissionsPageLayout/PermissionsPageLayout.tsx

@@ -81,7 +81,7 @@ function PermissionsPageLayout({
   const navigateToTab = (tab: PermissionsPageTab) =>
     dispatch(push(`/admin/permissions/${tab}`));
   const navigateToLocation = (location: Location) =>
-    dispatch(push(location.pathname));
+    dispatch(push(location.pathname + location.hash));
   const clearSaveError = () => dispatch(clearPermissionsSaveError());
 
   const handleToggleHelpReference = useCallback(() => {

frontend/src/metabase/admin/permissions/hooks/use-leave-confirmation.jsx

@@ -41,7 +41,7 @@ export const useLeaveConfirmation = ({
   };
 
   return (
-    <Modal isOpen={isConfirmationVisible}>
+    <Modal isOpen={isConfirmationVisible} zIndex={5}>
       <ConfirmContent
         title={t`Discard your unsaved changes?`}
         message={t`If you leave this page now, your changes won't be saved.`}

frontend/src/metabase/admin/permissions/selectors/data-permissions/fields.ts

@@ -10,6 +10,7 @@ import {
   PLUGIN_FEATURE_LEVEL_PERMISSIONS,
 } from "metabase/plugins";
 import { Group, GroupsPermissions } from "metabase-types/api";
+import { getNativePermissionDisabledTooltip } from "metabase/admin/permissions/selectors/data-permissions/shared";
 import Database from "metabase-lib/metadata/Database";
 import {
   getPermissionWarning,
@@ -17,10 +18,7 @@ import {
   getControlledDatabaseWarningModal,
   getRevokingAccessToAllTablesWarningModal,
 } from "../confirmations";
-import {
-  NATIVE_PERMISSION_REQUIRES_DATA_ACCESS,
-  UNABLE_TO_CHANGE_ADMIN_PERMISSIONS,
-} from "../../constants/messages";
+import { UNABLE_TO_CHANGE_ADMIN_PERMISSIONS } from "../../constants/messages";
 import { DATA_PERMISSION_OPTIONS } from "../../constants/data-permissions";
 import { TableEntityId, PermissionSectionConfig } from "../../types";
 
@@ -95,14 +93,16 @@ const buildNativePermission = (
   groupId: number,
   isAdmin: boolean,
   permissions: GroupsPermissions,
+  accessPermissionValue: string,
 ) => {
   return {
     permission: "data",
     type: "native",
     isDisabled: true,
-    disabledTooltip: isAdmin
-      ? UNABLE_TO_CHANGE_ADMIN_PERMISSIONS
-      : NATIVE_PERMISSION_REQUIRES_DATA_ACCESS,
+    disabledTooltip: getNativePermissionDisabledTooltip(
+      isAdmin,
+      accessPermissionValue,
+    ),
     isHighlighted: isAdmin,
     value: getNativePermission(permissions, groupId, entityId),
     options: [DATA_PERMISSION_OPTIONS.write, DATA_PERMISSION_OPTIONS.none],
@@ -131,6 +131,7 @@ export const buildFieldsPermissions = (
     groupId,
     isAdmin,
     permissions,
+    accessPermission.value,
   );
 
   return [

frontend/src/metabase/admin/permissions/selectors/data-permissions/schemas.ts

 import {
   getNativePermission,
   getSchemasPermission,
-  isRestrictivePermission,
 } from "metabase/admin/permissions/utils/graph";
 import {
   PLUGIN_ADMIN_PERMISSIONS_DATABASE_ACTIONS,
@@ -10,12 +9,10 @@ import {
   PLUGIN_FEATURE_LEVEL_PERMISSIONS,
 } from "metabase/plugins";
 import { Group, GroupsPermissions } from "metabase-types/api";
+import { getNativePermissionDisabledTooltip } from "metabase/admin/permissions/selectors/data-permissions/shared";
 import type Database from "metabase-lib/metadata/Database";
 import { DATA_PERMISSION_OPTIONS } from "../../constants/data-permissions";
-import {
-  NATIVE_PERMISSION_REQUIRES_DATA_ACCESS,
-  UNABLE_TO_CHANGE_ADMIN_PERMISSIONS,
-} from "../../constants/messages";
+import { UNABLE_TO_CHANGE_ADMIN_PERMISSIONS } from "../../constants/messages";
 import {
   getPermissionWarning,
   getPermissionWarningModal,
@@ -94,20 +91,6 @@ const buildAccessPermission = (
   };
 };
 
-const getNativePermissionDisabledTooltip = (
-  isAdmin: boolean,
-  accessPermissionValue: string,
-) => {
-  if (isAdmin) {
-    return UNABLE_TO_CHANGE_ADMIN_PERMISSIONS;
-  }
-
-  if (isRestrictivePermission(accessPermissionValue)) {
-    return NATIVE_PERMISSION_REQUIRES_DATA_ACCESS;
-  }
-  return null;
-};
-
 const buildNativePermission = (
   entityId: DatabaseEntityId,
   groupId: number,

frontend/src/metabase/admin/permissions/selectors/data-permissions/shared.ts

+import {
+  NATIVE_PERMISSION_REQUIRES_DATA_ACCESS,
+  UNABLE_TO_CHANGE_ADMIN_PERMISSIONS,
+} from "metabase/admin/permissions/constants/messages";
+import { isRestrictivePermission } from "metabase/admin/permissions/utils/graph";
+
+export const getNativePermissionDisabledTooltip = (
+  isAdmin: boolean,
+  accessPermissionValue: string,
+) => {
+  if (isAdmin) {
+    return UNABLE_TO_CHANGE_ADMIN_PERMISSIONS;
+  }
+
+  if (isRestrictivePermission(accessPermissionValue)) {
+    return NATIVE_PERMISSION_REQUIRES_DATA_ACCESS;
+  }
+  return null;
+};

frontend/src/metabase/admin/permissions/selectors/data-permissions/tables.ts

@@ -9,17 +9,15 @@ import {
   PLUGIN_FEATURE_LEVEL_PERMISSIONS,
 } from "metabase/plugins";
 import { Group, GroupsPermissions } from "metabase-types/api";
+import { getNativePermissionDisabledTooltip } from "metabase/admin/permissions/selectors/data-permissions/shared";
 import { DATA_PERMISSION_OPTIONS } from "../../constants/data-permissions";
+import { UNABLE_TO_CHANGE_ADMIN_PERMISSIONS } from "../../constants/messages";
 import {
-  NATIVE_PERMISSION_REQUIRES_DATA_ACCESS,
-  UNABLE_TO_CHANGE_ADMIN_PERMISSIONS,
-} from "../../constants/messages";
-import {
+  getControlledDatabaseWarningModal,
   getPermissionWarning,
   getPermissionWarningModal,
-  getControlledDatabaseWarningModal,
 } from "../confirmations";
-import { SchemaEntityId, PermissionSectionConfig } from "../../types";
+import { PermissionSectionConfig, SchemaEntityId } from "../../types";
 import { getGroupFocusPermissionsUrl } from "../../utils/urls";
 
 const buildAccessPermission = (
@@ -86,14 +84,16 @@ const buildNativePermission = (
   groupId: number,
   isAdmin: boolean,
   permissions: GroupsPermissions,
+  accessPermissionValue: string,
 ) => {
   return {
     permission: "data",
     type: "native",
     isDisabled: true,
-    disabledTooltip: isAdmin
-      ? UNABLE_TO_CHANGE_ADMIN_PERMISSIONS
-      : NATIVE_PERMISSION_REQUIRES_DATA_ACCESS,
+    disabledTooltip: getNativePermissionDisabledTooltip(
+      isAdmin,
+      accessPermissionValue,
+    ),
     isHighlighted: isAdmin,
     value: getNativePermission(permissions, groupId, entityId),
     options: [DATA_PERMISSION_OPTIONS.write, DATA_PERMISSION_OPTIONS.none],
@@ -120,6 +120,7 @@ export const buildTablesPermissions = (
     groupId,
     isAdmin,
     permissions,
+    accessPermission.value,
   );
 
   return [

frontend/src/metabase/components/Modal/WindowModal.tsx

@@ -18,6 +18,7 @@ export type WindowModalProps = BaseModalProps & {
   formModal?: boolean;
   style?: CSSProperties;
   "data-testid"?: string;
+  zIndex?: number;
 } & {
   [size in ModalSize]?: boolean;
 };
@@ -36,6 +37,10 @@ export class WindowModal extends Component<WindowModalProps> {
 
     this._modalElement = document.createElement("div");
     this._modalElement.className = "ModalContainer";
+
+    if (props.zIndex != null) {
+      this._modalElement.setAttribute("style", `z-index:${props.zIndex}`);
+    }
     document.body.appendChild(this._modalElement);
   }
 

frontend/src/metabase/core/components/FormFooter/FormFooter.styled.tsx

 import styled from "@emotion/styled";
+import { color } from "metabase/lib/colors";
 
 const FormFooter = styled.div`
+  border-top: 1px solid ${color("border")};
+  margin-top: 0.5rem;
+  padding-top: 1.5rem;
   display: flex;
   align-items: center;
   justify-content: flex-end;

frontend/test/__support__/server-mocks/impersonation.ts

+import fetchMock from "fetch-mock";
+import { DatabaseId, GroupId, Impersonation } from "metabase-types/api";
+
+export const setupExistingImpersonationEndpoint = (
+  impersonation: Impersonation,
+) => {
+  fetchMock.get(
+    {
+      url: `path:/api/ee/advanced-permissions/impersonation`,
+      query: {
+        db_id: impersonation.db_id,
+        group_id: impersonation.group_id,
+      },
+    },
+    impersonation,
+  );
+};
+
+export const setupMissingImpersonationEndpoint = (
+  databaseId: DatabaseId,
+  groupId: GroupId,
+) => {
+  fetchMock.get(
+    {
+      url: `path:/api/ee/advanced-permissions/impersonation`,
+      query: {
+        db_id: databaseId,
+        group_id: groupId,
+      },
+    },
+    () => null,
+  );
+};

frontend/test/__support__/server-mocks/user.ts

 import fetchMock from "fetch-mock";
-import { User, UserListResult } from "metabase-types/api";
+import { User, UserAttribute, UserListResult } from "metabase-types/api";
 
 export function setupUsersEndpoints(users: UserListResult[]) {
   fetchMock.get("path:/api/user", users);
@@ -8,3 +8,7 @@ export function setupUsersEndpoints(users: UserListResult[]) {
 export function setupCurrentUserEndpoint(user: User) {
   fetchMock.get("path:/api/user/current", user);
 }
+
+export function setupUserAttributesEndpoint(attributes: UserAttribute[]) {
+  fetchMock.get(`path:/api/mt/user/attributes`, attributes);
+}