Fix item picker suggests to select items user does not have write access to (#15613)

* Test adding question to dashboard

* Test collections filtering when adding a question

When adding a question to dashboard,
we need to display collections a user has "write" access to.
Collection with "read" access have to be hidden

* Fix adding question to dashboard without access

* Add a note about permissions test suite

* Move question permission tests to collection suite

* Revert initial collections filtering

* Fix adding question to dashboard without access

* Remove redundant state field

* Enable #15281 issue repro test

* Remove requireCollectionWritePermission prop

* Filter items user doesn't have `write` access to

* Fix permission tests

* Fix dashboard test

* Fix part of permission tests disabled for nodata user

* Bring back issue reference to Cypress test

* Remove underscore prefixes for component methods

* Test offers saving dashboard to opened collection

* Fix tests nested incorrectly

* Split dashboard permission test

* Fix suggest saving items to read-only collections

* Fix collection permission filtering

See comment:
https://github.com/metabase/metabase/pull/15613#discussion_r614460504

* Move comment

* Fix test failing due to fixed collection suggestion

* Remove `should("exist")` from Cypress tests

* Merge "adding question to dashboard" tests

* Merge similar permission tests

* Merge similar tests into one

* Use sidebar test ID in permissions test

* Select by .AdminSelect

* Remove redundant search test case

* Revert native query test

* Add React list key to ItemPicker items

* Add collection suggestions tests

* Configure Form's `overwriteOnInitialValuesChange`

* Wrap CollectionSelect with `@Collection.loadList`

When suggesting an initial collection,
we need to check a user has `write` access to it.
For that, collection objects have to be present in Redux store,
so we can retrieve a collection by ID and check the `can_write` flag

* Allow modifying CreateDashboardModal's onSave prop

* Fix collection suggestsions for new dashboard

* Fix collection suggestions when copying dashboards

* Add defaultProps to Form

* Fix SaveQuestionModal unit test

* Fix SaveQuestionModal collection suggestion

* Simplify CollectionSelect wrapper

* Fix dashboard header test selector

* Rename permission tests

* Mock HTTP requests at SaveQuestionModal test

* Pass correct params to initialCollectionId

frontend/src/metabase/collections/components/CollectionCopyEntityModal.jsx

 /* eslint-disable react/prop-types */
 import React from "react";
+import { connect } from "react-redux";
 import { dissoc } from "icepick";
 import { t } from "ttag";
 
@@ -9,10 +10,21 @@ import { entityTypeForObject } from "metabase/schema";
 
 import Link from "metabase/components/Link";
 
+import Collections from "metabase/entities/collections";
 import EntityCopyModal from "metabase/entities/containers/EntityCopyModal";
 
+function mapStateToProps(state, props) {
+  return {
+    initialCollectionId: Collections.selectors.getInitialCollectionId(state, {
+      ...props,
+      collectionId: props.entityObject.collection_id,
+    }),
+  };
+}
+
 function CollectionCopyEntityModal({
   entityObject,
+  initialCollectionId,
   onClose,
   onSaved,
   triggerToast,
@@ -20,7 +32,10 @@ function CollectionCopyEntityModal({
   return (
     <EntityCopyModal
       entityType={entityTypeForObject(entityObject)}
-      entityObject={entityObject}
+      entityObject={{
+        ...entityObject,
+        collection_id: initialCollectionId,
+      }}
       copy={async values => {
         return entityObject.copy(dissoc(values, "id"));
       }}
@@ -45,4 +60,4 @@ function CollectionCopyEntityModal({
   );
 }
 
-export default withToast(CollectionCopyEntityModal);
+export default withToast(connect(mapStateToProps)(CollectionCopyEntityModal));

frontend/src/metabase/components/CreateDashboardModal.jsx

@@ -28,21 +28,26 @@ const mapDispatchToProps = {
 )
 export default class CreateDashboardModal extends Component {
   static propTypes = {
+    onSaved: PropTypes.func,
     onClose: PropTypes.func,
   };
 
+  onSaved = dashboard => {
+    const { onClose, onChangeLocation } = this.props;
+    onChangeLocation(Urls.dashboard(dashboard.id));
+    if (onClose) {
+      onClose();
+    }
+  };
+
   render() {
-    const { initialCollectionId, onClose, onChangeLocation } = this.props;
+    const { initialCollectionId, onSaved, onClose } = this.props;
     return (
       <Dashboard.ModalForm
+        overwriteOnInitialValuesChange
         dashboard={{ collection_id: initialCollectionId }}
         onClose={onClose}
-        onSaved={dashboard => {
-          onChangeLocation(Urls.dashboard(dashboard.id));
-          if (onClose) {
-            onClose();
-          }
-        }}
+        onSaved={typeof onSaved === "function" ? onSaved : this.onSaved}
       />
     );
   }

frontend/src/metabase/containers/AddToDashSelectDashModal.jsx

@@ -5,6 +5,7 @@ import { Flex } from "grid-styled";
 import Icon from "metabase/components/Icon";
 import Link from "metabase/components/Link";
 import ModalContent from "metabase/components/ModalContent";
+import CreateDashboardModal from "metabase/components/CreateDashboardModal";
 import DashboardPicker from "metabase/containers/DashboardPicker";
 
 import * as Urls from "metabase/lib/urls";
@@ -15,8 +16,6 @@ import type {
 } from "metabase-types/types/Dashboard";
 import type { Card } from "metabase-types/types/Card";
 
-import Dashboard from "metabase/entities/dashboards";
-
 export default class AddToDashSelectDashModal extends Component {
   state = {
     shouldCreateDashboard: false,
@@ -40,8 +39,8 @@ export default class AddToDashSelectDashModal extends Component {
   render() {
     if (this.state.shouldCreateDashboard) {
       return (
-        <Dashboard.ModalForm
-          dashboard={{ collection_id: this.props.card.collection_id }}
+        <CreateDashboardModal
+          collectionId={this.props.card.collection_id}
           onSaved={dashboard => this.addToDashboard(dashboard.id)}
           onClose={() => this.setState({ shouldCreateDashboard: false })}
         />

frontend/src/metabase/containers/CollectionSelect.jsx

-import ItemSelect from "./ItemSelect";
+/* eslint-disable react/prop-types */
+import Collection from "metabase/entities/collections";
 
+import ItemSelect from "./ItemSelect";
 import CollectionPicker from "./CollectionPicker";
 import CollectionName from "./CollectionName";
 
-const QuestionSelect = ItemSelect(
+const CollectionSelect = ItemSelect(
   CollectionPicker,
   CollectionName,
   "collection",
 );
 
-export default QuestionSelect;
+/**
+ * When suggesting an initial collection,
+ * we need to check a user has `write` access to it.
+ * For that, collection objects have to be present in Redux store,
+ * so we can retrieve a collection by ID and check the `can_write` flag.
+ *
+ * This component is wrapped with @Collection.loadList
+ * to ensure collection are fetched and permissions can be checked.
+ */
+export default Collection.loadList({
+  loadingAndErrorWrapper: false,
+})(CollectionSelect);

frontend/src/metabase/containers/Form.jsx

@@ -227,6 +227,11 @@ export default class Form extends React.Component {
     onSubmit: PropTypes.func.isRequired,
     initialValues: PropTypes.object,
     formName: PropTypes.string,
+    overwriteOnInitialValuesChange: PropTypes.bool,
+  };
+
+  static defaultProps = {
+    overwriteOnInitialValuesChange: false,
   };
 
   static childContextTypes = {
@@ -331,14 +336,14 @@ export default class Form extends React.Component {
 
   render() {
     // eslint-disable-next-line
-    const { formName } = this.props;
+    const { formName, overwriteOnInitialValuesChange } = this.props;
     const formObject = this._getFormObject();
     const initialValues = this._getInitialValues();
     const fieldNames = this._getFieldNames();
     return (
       <ReduxFormComponent
         {...this.props}
-        overwriteOnInitialValuesChange={false}
+        overwriteOnInitialValuesChange={overwriteOnInitialValuesChange}
         formObject={formObject}
         // redux-form props:
         form={formName}

frontend/src/metabase/containers/ItemPicker.jsx

@@ -56,7 +56,7 @@ export default class ItemPicker extends React.Component {
   };
 
   // returns a list of "crumbs" starting with the "root" collection
-  _getCrumbs(collection, collectionsById) {
+  getCrumbs(collection, collectionsById) {
     if (collection && collection.path) {
       return [
         ...collection.path.map(id => [
@@ -76,6 +76,31 @@ export default class ItemPicker extends React.Component {
     }
   }
 
+  checkHasWritePermissionForItem(item, models) {
+    const { collectionsById } = this.props;
+
+    // if user is selecting a collection, they must have a `write` access to it
+    if (models.has("collection") && item.model === "collection") {
+      return item.can_write;
+    }
+
+    // if user is selecting something else (e.g. dashboard),
+    // they must have `write` access to a collection item belongs to
+    const collection = item.collection_id
+      ? collectionsById[item.collection_id]
+      : collectionsById["root"];
+    return collection.can_write;
+  }
+
+  checkCanWriteToCollectionOrItsChildren(collection) {
+    return (
+      collection.can_write ||
+      collection.children.some(child =>
+        this.checkCanWriteToCollectionOrItsChildren(child),
+      )
+    );
+  }
+
   render() {
     const {
       value,
@@ -93,7 +118,7 @@ export default class ItemPicker extends React.Component {
       this.props.models.filter(model => model !== "collection").length > 0;
 
     const collection = collectionsById[parentId];
-    const crumbs = this._getCrumbs(collection, collectionsById);
+    const crumbs = this.getCrumbs(collection, collectionsById);
 
     let allCollections = (collection && collection.children) || [];
 
@@ -102,6 +127,11 @@ export default class ItemPicker extends React.Component {
       allCollections = [collection, ...allCollections];
     }
 
+    // ensure we only display collections a user can write to
+    allCollections = allCollections.filter(collection =>
+      this.checkCanWriteToCollectionOrItsChildren(collection),
+    );
+
     // code below assumes items have a "model" property
     allCollections = allCollections.map(collection => ({
       ...collection,
@@ -172,6 +202,7 @@ export default class ItemPicker extends React.Component {
                   // only show if collection can be selected or has children
                   return canSelect || hasChildren ? (
                     <Item
+                      key={`collection-${collection.id}`}
                       item={collection}
                       name={collection.name}
                       color={COLLECTION_ICON_COLOR}
@@ -205,26 +236,34 @@ export default class ItemPicker extends React.Component {
               >
                 {({ list }) => (
                   <div>
-                    {list
-                      .filter(
-                        item =>
-                          // remove collections unless we're searching
-                          (item.model !== "collection" || !!searchString) &&
-                          // only include desired models (TODO: ideally the endpoint would handle this)
-                          models.has(item.model),
-                      )
-                      .map(item => (
-                        <Item
-                          key={item.id}
-                          item={item}
-                          name={item.getName()}
-                          color={item.getColor()}
-                          icon={item.getIcon()}
-                          selected={isSelected(item)}
-                          canSelect
-                          onChange={onChange}
-                        />
-                      ))}
+                    {list.map(item => {
+                      const hasPermission = this.checkHasWritePermissionForItem(
+                        item,
+                        models,
+                      );
+                      if (
+                        hasPermission &&
+                        // only include desired models (TODO: ideally the endpoint would handle this)
+                        models.has(item.model) &&
+                        // remove collections unless we're searching
+                        // (so a user can navigate through collections)
+                        (item.model !== "collection" || !!searchString)
+                      ) {
+                        return (
+                          <Item
+                            key={item.id}
+                            item={item}
+                            name={item.getName()}
+                            color={item.getColor()}
+                            icon={item.getIcon()}
+                            selected={isSelected(item)}
+                            canSelect={hasPermission}
+                            onChange={onChange}
+                          />
+                        );
+                      }
+                      return null;
+                    })}
                   </div>
                 )}
               </EntityListLoader>

frontend/src/metabase/containers/SaveQuestionModal.jsx

@@ -110,6 +110,7 @@ export default class SaveQuestionModal extends Component {
             { name: "collection_id" },
           ]}
           onSubmit={this.handleSubmit}
+          overwriteOnInitialValuesChange
         >
           {({ values, Form }) => (
             <Form>

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

@@ -8,14 +8,20 @@ import { replace } from "react-router-redux";
 import * as Urls from "metabase/lib/urls";
 
 import Dashboards from "metabase/entities/dashboards";
+import Collections from "metabase/entities/collections";
 
 import EntityCopyModal from "metabase/entities/containers/EntityCopyModal";
 
 import { getDashboardComplete } from "../selectors";
 
 const mapStateToProps = (state, props) => {
+  const dashboard = getDashboardComplete(state, props);
   return {
-    dashboard: getDashboardComplete(state, props),
+    dashboard,
+    initialCollectionId: Collections.selectors.getInitialCollectionId(state, {
+      ...props,
+      collectionId: dashboard.collection_id,
+    }),
   };
 };
 
@@ -36,12 +42,17 @@ class DashboardCopyModal extends React.Component {
       onReplaceLocation,
       copyDashboard,
       dashboard,
+      initialCollectionId,
       ...props
     } = this.props;
     return (
       <EntityCopyModal
         entityType="dashboards"
-        entityObject={dashboard}
+        entityObject={{
+          ...dashboard,
+          collection_id: initialCollectionId,
+        }}
+        overwriteOnInitialValuesChange
         copy={object =>
           copyDashboard(
             { id: this.props.params.dashboardId },

frontend/src/metabase/entities/collections.js

@@ -80,6 +80,7 @@ const Collections = createEntity({
     ),
     getInitialCollectionId: createSelector(
       [
+        state => state.entities.collections,
         // these are listed in order of priority
         (state, { collectionId }) => collectionId,
         (state, { params }) => (params ? params.collectionId : undefined),
@@ -87,9 +88,10 @@ const Collections = createEntity({
           location && location.query ? location.query.collectionId : undefined,
         getUserDefaultCollectionId,
       ],
-      (...collectionIds) => {
+      (collections, ...collectionIds) => {
         for (const collectionId of collectionIds) {
-          if (collectionId !== undefined) {
+          const collection = collections[collectionId];
+          if (collection && collection.can_write) {
             return canonicalCollectionId(collectionId);
           }
         }

frontend/test/__support__/entities-store.js

@@ -5,11 +5,12 @@ import promise from "redux-promise";
 import { thunkWithDispatchAction } from "metabase/store";
 import * as entities from "metabase/redux/entities";
 
-export function getStore() {
+export function getStore(reducers = {}) {
   const reducer = combineReducers({
     entities: entities.reducer,
     requests: (state, action) =>
       requestsReducer(entities.requestsReducer(state, action), action),
+    ...reducers,
   });
 
   const initialState = {};

frontend/test/metabase/containers/SaveQuestionModal.unit.spec.js

 import React from "react";
 import { render, screen, fireEvent } from "@testing-library/react";
+import mock from "xhr-mock";
 
 import SaveQuestionModal from "metabase/containers/SaveQuestionModal";
 import Question from "metabase-lib/lib/Question";
@@ -10,13 +11,13 @@ import {
   PEOPLE,
   metadata,
 } from "__support__/sample_dataset_fixture";
+import { getStore } from "__support__/entities-store";
 
-import { createStore, combineReducers } from "redux";
 import { Provider } from "react-redux";
 import { reducer as form } from "redux-form";
 
 const renderSaveQuestionModal = (question, originalQuestion) => {
-  const store = createStore(combineReducers({ form }));
+  const store = getStore({ form });
   const onCreateMock = jest.fn(() => Promise.resolve());
   const onSaveMock = jest.fn(() => Promise.resolve());
   render(
@@ -35,6 +36,40 @@ const renderSaveQuestionModal = (question, originalQuestion) => {
 };
 
 describe("SaveQuestionModal", () => {
+  const TEST_COLLECTIONS = [
+    {
+      can_write: false,
+      effective_ancestors: [],
+      effective_location: null,
+      id: "root",
+      name: "Our analytics",
+      parent_id: null,
+    },
+    {
+      archived: false,
+      can_write: true,
+      color: "#31698A",
+      description: null,
+      id: 1,
+      location: "/",
+      name: "Bobby Tables's Personal Collection",
+      namespace: null,
+      personal_owner_id: 1,
+      slug: "bobby_tables_s_personal_collection",
+    },
+  ];
+
+  beforeEach(() => {
+    mock.setup();
+    mock.get("/api/collection", {
+      body: JSON.stringify(TEST_COLLECTIONS),
+    });
+  });
+
+  afterEach(() => {
+    mock.teardown();
+  });
+
   it("should call onCreate correctly for a new question", async () => {
     const newQuestion = Question.create({
       databaseId: SAMPLE_DATASET.id,

frontend/test/metabase/scenarios/collections/permissions.cy.spec.js

+/**
+ * FYI, this test suite contains permission tests for different pages
+ *
+ * - Collections (/collection/root)
+ * - Dashboard (/dashboard/:id)
+ * - Question (/question/:id)
+ *
+ * It's a WIP and most likely it will be split later,
+ * when we're sure about our testing strategy for permissions
+ * See discussion: https://github.com/metabase/metabase/pull/15573
+ */
+
 import { onlyOn } from "@cypress/skip-test";
 import { restore, popover } from "__support__/e2e/cypress";
 import { USERS } from "__support__/e2e/cypress_data";
@@ -24,6 +36,19 @@ describe("collection permissions", () => {
                 cy.signIn(user);
               });
 
+              describe("create dashboard", () => {
+                it("should offer to save dashboard to a currently opened collection", () => {
+                  cy.visit("/collection/root");
+                  cy.findByTestId("sidebar").within(() => {
+                    cy.findByText("First collection").click();
+                    cy.findByText("Second collection").click();
+                  });
+                  cy.icon("add").click();
+                  cy.findByText("New dashboard").click();
+                  cy.get(".AdminSelect").findByText("Second collection");
+                });
+              });
+
               describe("pin", () => {
                 it("pinning should work properly for both questions and dashboards", () => {
                   cy.visit("/collection/root");
@@ -148,7 +173,7 @@ describe("collection permissions", () => {
                     cy.get("[class*=PageHeading]")
                       .as("title")
                       .contains("Second collection");
-                    cy.get("[class*=CollectionSidebar]")
+                    cy.findByTestId("sidebar")
                       .as("sidebar")
                       .within(() => {
                         cy.findByText("First collection");
@@ -285,6 +310,22 @@ describe("collection permissions", () => {
                   cy.location("pathname").should("eq", "/collection/root");
                   cy.findByText("Orders").should("not.exist");
                 });
+
+                it("should be able to add question to dashboard", () => {
+                  popover()
+                    .findByText("Add to dashboard")
+                    .click();
+
+                  cy.get(".Modal")
+                    .as("modal")
+                    .findByText("Orders in a dashboard")
+                    .click();
+
+                  cy.get("@modal").should("not.exist");
+                  // By default, the dashboard contains one question
+                  // After we add a new one, we check there are two questions now
+                  cy.get(".DashCard").should("have.length", 2);
+                });
               });
 
               describe("managing dashboard from the dashboard's edit menu", () => {
@@ -367,22 +408,24 @@ describe("collection permissions", () => {
             });
 
             it("should be offered to duplicate dashboard in collections they have `read` access to", () => {
+              const { first_name, last_name } = USERS[user];
               cy.visit("/collection/root");
               openEllipsisMenuFor("Orders in a dashboard");
               popover()
                 .findByText("Duplicate this item")
-                .should("exist");
+                .click();
+              cy.get(".AdminSelect").findByText(
+                `${first_name} ${last_name}'s Personal Collection`,
+              );
             });
 
             ["/", "/collection/root"].forEach(route => {
-              it.skip("should not be offered to save dashboard in collections they have `read` access to (metabase#15281)", () => {
+              it("should not be offered to save dashboard in collections they have `read` access to (metabase#15281)", () => {
                 const { first_name, last_name } = USERS[user];
                 cy.visit(route);
                 cy.icon("add").click();
                 cy.findByText("New dashboard").click();
-                cy.findByLabelText("Name")
-                  .click()
-                  .type("Foo");
+
                 // Coming from the root collection, the initial offered collection will be "Our analytics" (read-only access)
                 cy.findByText(
                   `${first_name} ${last_name}'s Personal Collection`,
@@ -401,6 +444,45 @@ describe("collection permissions", () => {
               });
             });
 
+            describe("managing question from the question's edit dropdown", () => {
+              it("should not be offered to add question to dashboard inside a collection they have `read` access to", () => {
+                cy.visit("/question/1");
+                cy.icon("pencil").click();
+                popover()
+                  .findByText("Add to dashboard")
+                  .click();
+
+                cy.get(".Modal").within(() => {
+                  cy.findByText("Orders in a dashboard").should("not.exist");
+                  cy.icon("search").click();
+                  cy.findByPlaceholderText("Search").type(
+                    "Orders in a dashboard{Enter}",
+                  );
+                  cy.findByText("Orders in a dashboard").should("not.exist");
+                });
+              });
+
+              it("should offer personal collection as a save destination for a new dashboard", () => {
+                const { first_name, last_name } = USERS[user];
+                const personalCollection = `${first_name} ${last_name}'s Personal Collection`;
+                cy.visit("/question/1");
+                cy.icon("pencil").click();
+                popover()
+                  .findByText("Add to dashboard")
+                  .click();
+
+                cy.get(".Modal").within(() => {
+                  cy.findByText("Create a new dashboard").click();
+                  cy.get(".AdminSelect").findByText(personalCollection);
+                  cy.findByLabelText("Name").type("Foo");
+                  cy.findByRole("button", { name: "Create" }).click();
+                });
+                cy.url().should("match", /\/dashboard\/\d+$/);
+                saveDashboard();
+                cy.get(".DashboardHeader").findByText(personalCollection);
+              });
+            });
+
             describe("managing dashboard from the dashboard's edit menu", () => {
               it("should not be offered to change title and description for dashboard in collections they have `read` access to (metabase#15280)", () => {
                 cy.visit("/dashboard/1");
@@ -419,9 +501,15 @@ describe("collection permissions", () => {
               });
 
               it("should be offered to duplicate dashboard in collections they have `read` access to", () => {
+                const { first_name, last_name } = USERS[user];
                 cy.visit("/dashboard/1");
                 cy.icon("ellipsis").click();
-                popover().findByText("Duplicate");
+                popover()
+                  .findByText("Duplicate")
+                  .click();
+                cy.get(".AdminSelect").findByText(
+                  `${first_name} ${last_name}'s Personal Collection`,
+                );
               });
             });
           });