From 70c8d96c7c4cbc5adcc50b2b986ab49bfa7bae81 Mon Sep 17 00:00:00 2001
From: Jesse Devaney <22608765+JesseSDevaney@users.noreply.github.com>
Date: Mon, 13 Nov 2023 15:46:08 -0700
Subject: [PATCH] Hide read-only items in the Archive (#33602)
* refactor use-list-select hook
- toggleAll(items) felt unintuitive as some items could be selected while simultaneously unselecting others
- switched to selectOnlyTheseItems(items) to be more explicit on what the function is doing
- updated consumers of useListSelect to use the new API
* convert to functional component
* convert HoC ListSelect to hook useListSelect
* convert redux connects to useSelectors
* refactor ArchiveApp and improve types
* hide read only items in the archive
* verify desired behavior
* refactor beforeEach
* remove unused eslint disable line
* refactor item can_write accessor logic
* Add :can_write key into the search data response. uses `mi/can-write?`
* update archive app for archived search changes
* refactor filtering of list
* replace hard-coded collection ids in spec
* move specific helper outside of generic helpers file
* remove admin gets unfiltered data
* BE sends only can_write true results when search with archived?=true
This change modifies the `check-permissions-for-model` method to check for write access if the SearchContext has
archived true.
This means that the only results sent to the frontend are those results that the user can take actions on, namely,
unarchiving them.
* reomve unused require
* update FE to account for BE changes
---------
Co-authored-by: adam-james <21064735+adam-james-v@users.noreply.github.com>
---
e2e/support/cypress_sample_instance_data.js | 5 ++
.../scenarios/collections/archive.cy.spec.js | 50 ++++++++++++++++++-
frontend/src/metabase-types/api/collection.ts | 1 +
.../archive/containers/ArchiveApp.tsx | 33 +++++++-----
frontend/src/metabase/entities/search.js | 9 ++++
src/metabase/api/search.clj | 32 +++++++-----
test/metabase/api/search_test.clj | 18 +++++++
7 files changed, 122 insertions(+), 26 deletions(-)
diff --git a/e2e/support/cypress_sample_instance_data.js b/e2e/support/cypress_sample_instance_data.js
index 4f4bdf41805..d4d699f80f6 100644
--- a/e2e/support/cypress_sample_instance_data.js
+++ b/e2e/support/cypress_sample_instance_data.js
@@ -35,6 +35,11 @@ export const NORMAL_PERSONAL_COLLECTION_ID = _.findWhere(
{ name: "Robert Tableton's Personal Collection" },
).id;
+export const READ_ONLY_PERSONAL_COLLECTION_ID = _.findWhere(
+ SAMPLE_INSTANCE_DATA.collections,
+ { name: "Read Only Tableton's Personal Collection" },
+).id;
+
export const NO_DATA_PERSONAL_COLLECTION_ID = _.findWhere(
SAMPLE_INSTANCE_DATA.collections,
{ name: "No Data Tableton's Personal Collection" },
diff --git a/e2e/test/scenarios/collections/archive.cy.spec.js b/e2e/test/scenarios/collections/archive.cy.spec.js
index bfb8d4a1777..500265c6b93 100644
--- a/e2e/test/scenarios/collections/archive.cy.spec.js
+++ b/e2e/test/scenarios/collections/archive.cy.spec.js
@@ -1,6 +1,9 @@
import { restore } from "e2e/support/helpers";
import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
-import { FIRST_COLLECTION_ID } from "e2e/support/cypress_sample_instance_data";
+import {
+ FIRST_COLLECTION_ID,
+ READ_ONLY_PERSONAL_COLLECTION_ID,
+} from "e2e/support/cypress_sample_instance_data";
const { ORDERS, ORDERS_ID, PEOPLE_ID } = SAMPLE_DATABASE;
@@ -141,6 +144,42 @@ describe("scenarios > collections > archive", () => {
cy.findByTestId(`archive-item-${QUESTION_NAME}`).should("not.exist");
});
+ it("should hide read-only archived items (metabase#24018)", () => {
+ const READ_ONLY_NAME = "read-only dashboard";
+ const CURATEABLE_NAME = "curate-able dashboard";
+
+ // setup archive with read-only collection items
+ createAndArchiveDashboard({
+ name: READ_ONLY_NAME,
+ collection_id: null,
+ });
+
+ // setup archive with curate-able collection items (user created items)
+ cy.signIn("readonly");
+
+ createAndArchiveDashboard({
+ name: CURATEABLE_NAME,
+ collection_id: READ_ONLY_PERSONAL_COLLECTION_ID,
+ });
+
+ // assert on desired behavior for read-only user
+ cy.visit("/archive");
+
+ cy.get("main").within(() => {
+ cy.findByText(READ_ONLY_NAME).should("not.exist");
+ cy.findByText(CURATEABLE_NAME).should("be.visible");
+ });
+
+ // assert on desired behavior for admin user
+ cy.signInAsAdmin();
+ cy.visit("/archive");
+
+ cy.get("main").within(() => {
+ cy.findByText(READ_ONLY_NAME).should("be.visible");
+ cy.findByText(CURATEABLE_NAME).should("be.visible");
+ });
+ });
+
it("should load initially hidden archived items on scroll (metabase#24213)", () => {
const stubbedItems = Array.from({ length: 50 }, (v, i) => ({
name: "Item " + i,
@@ -180,3 +219,12 @@ describe("scenarios > collections > archive", () => {
});
});
});
+
+function createAndArchiveDashboard({ name, collection_id }) {
+ cy.request("POST", "/api/dashboard/", {
+ name: name,
+ collection_id: collection_id,
+ }).then(({ body: { id: dashboardId } }) => {
+ cy.request("PUT", `/api/dashboard/${dashboardId}`, { archived: true });
+ });
+}
diff --git a/frontend/src/metabase-types/api/collection.ts b/frontend/src/metabase-types/api/collection.ts
index 77ad2b34e0a..de34977a629 100644
--- a/frontend/src/metabase-types/api/collection.ts
+++ b/frontend/src/metabase-types/api/collection.ts
@@ -82,6 +82,7 @@ export interface CollectionItem {
database_id?: DatabaseId;
moderated_status?: string;
type?: string;
+ can_write?: boolean;
getIcon: () => { name: IconName };
getUrl: (opts?: Record<string, unknown>) => string;
setArchived?: (isArchived: boolean) => void;
diff --git a/frontend/src/metabase/archive/containers/ArchiveApp.tsx b/frontend/src/metabase/archive/containers/ArchiveApp.tsx
index 2889a7d416a..01c2e166bbd 100644
--- a/frontend/src/metabase/archive/containers/ArchiveApp.tsx
+++ b/frontend/src/metabase/archive/containers/ArchiveApp.tsx
@@ -1,9 +1,18 @@
import { useCallback, useEffect, useMemo } from "react";
import { t } from "ttag";
+import type { CollectionItem } from "metabase-types/api";
+
import { useDispatch, useSelector } from "metabase/lib/redux";
-import type { CollectionItem } from "metabase-types/api";
+import Search from "metabase/entities/search";
+import { useListSelect } from "metabase/hooks/use-list-select";
+import { useSearchListQuery } from "metabase/common/hooks";
+import { isSmallScreen, getMainElement } from "metabase/lib/dom";
+
+import { openNavbar } from "metabase/redux/app";
+import { getIsNavbarOpen } from "metabase/selectors/app";
+import { getUserIsAdmin } from "metabase/selectors/user";
import LoadingAndErrorWrapper from "metabase/components/LoadingAndErrorWrapper/LoadingAndErrorWrapper";
import { Button } from "metabase/ui";
@@ -14,15 +23,6 @@ import { StackedCheckBox } from "metabase/components/StackedCheckBox";
import VirtualizedList from "metabase/components/VirtualizedList";
import { ArchivedItem } from "metabase/components/ArchivedItem/ArchivedItem";
-import Search from "metabase/entities/search";
-import { useListSelect } from "metabase/hooks/use-list-select";
-import { useSearchListQuery } from "metabase/common/hooks";
-
-import { openNavbar } from "metabase/redux/app";
-import { getIsNavbarOpen } from "metabase/selectors/app";
-import { getUserIsAdmin } from "metabase/selectors/user";
-import { isSmallScreen, getMainElement } from "metabase/lib/dom";
-
import {
ArchiveBarContent,
ArchiveBarText,
@@ -54,12 +54,19 @@ export function ArchiveApp() {
useListSelect<CollectionItem>(item => `${item.model}:${item.id}`);
const list = useMemo(() => {
- clear();
- return data ?? [];
- }, [data, clear]);
+ clear(); // clear selected items if data is ever refreshed
+
+ if (!Array.isArray(data)) {
+ return [];
+ }
+
+ return data;
+ }, [clear, data]);
+
const selectAllItems = useCallback(() => {
selectOnlyTheseItems(list);
}, [list, selectOnlyTheseItems]);
+
const allSelected = useMemo(
() => selected.length === list.length,
[selected, list],
diff --git a/frontend/src/metabase/entities/search.js b/frontend/src/metabase/entities/search.js
index 0a4329dc7a5..2a53be28f85 100644
--- a/frontend/src/metabase/entities/search.js
+++ b/frontend/src/metabase/entities/search.js
@@ -124,6 +124,15 @@ export default createEntity({
},
objectSelectors: {
+ getCollection: object => {
+ const entity = entityForObject(object);
+ return entity
+ ? entity?.objectSelectors?.getCollection?.(object) ??
+ object?.collection ??
+ null
+ : warnEntityAndReturnObject(object);
+ },
+
getName: object => {
const entity = entityForObject(object);
return entity
diff --git a/src/metabase/api/search.clj b/src/metabase/api/search.clj
index 13c0bd030c4..4fbec8df0a6 100644
--- a/src/metabase/api/search.clj
+++ b/src/metabase/api/search.clj
@@ -333,25 +333,33 @@
[(into [:case] case-clauses)]))
(defmulti ^:private check-permissions-for-model
- {:arglists '([search-result])}
- (comp keyword :model))
+ {:arglists '([archived? search-result])}
+ (fn [_ search-result] ((comp keyword :model) search-result)))
(defmethod check-permissions-for-model :default
- [_]
- ;; We filter what we can (ie. everything that is in a collection) out already when querying
- true)
+ [archived? instance]
+ (if archived?
+ (mi/can-write? instance)
+ ;; We filter what we can (ie. everything that is in a collection) out already when querying
+ true))
(defmethod check-permissions-for-model :metric
- [instance]
- (mi/can-read? instance))
+ [archived? instance]
+ (if archived?
+ (mi/can-write? instance)
+ (mi/can-read? instance)))
(defmethod check-permissions-for-model :segment
- [instance]
- (mi/can-read? instance))
+ [archived? instance]
+ (if archived?
+ (mi/can-write? instance)
+ (mi/can-read? instance)))
(defmethod check-permissions-for-model :database
- [instance]
- (mi/can-read? instance))
+ [archived? instance]
+ (if archived?
+ (mi/can-write? instance)
+ (mi/can-read? instance)))
(mu/defn query-model-set :- [:set SearchableModel]
"Queries all models with respect to query for one result to see if we get a result or not"
@@ -416,7 +424,7 @@
xf (comp
(map t2.realize/realize)
(map to-toucan-instance)
- (filter check-permissions-for-model)
+ (filter (partial check-permissions-for-model (:archived? search-ctx)))
;; MySQL returns `:bookmark` and `:archived` as `1` or `0` so convert those to boolean as
;; needed
(map #(update % :bookmark api/bit->boolean))
diff --git a/test/metabase/api/search_test.clj b/test/metabase/api/search_test.clj
index e9d36c95630..581f34396c7 100644
--- a/test/metabase/api/search_test.clj
+++ b/test/metabase/api/search_test.clj
@@ -1519,3 +1519,21 @@
(is (= #{"dashboard" "collection"}
(set (mt/user-http-request :crowberto :get 200 "search/models" :q search-term
:filter_items_in_personal_collection "exclude"))))))))
+
+(deftest archived-search-results-with-no-write-perms-test
+ (testing "Results which the searching user has no write permissions for are filtered out. #33602"
+ (mt/with-temp [Collection {collection-id :id} (archived {:name "collection test collection"})
+ Card _ (archived {:name "card test card is returned"})
+ Card _ (archived {:name "card test card"
+ :collection_id collection-id})
+ Card _ (archived {:name "dataset test dataset" :dataset true
+ :collection_id collection-id})
+ Dashboard _ (archived {:name "dashboard test dashboard"
+ :collection_id collection-id})]
+ ;; remove read/write access and add back read access to the collection
+ (perms/revoke-collection-permissions! (perms-group/all-users) collection-id)
+ (perms/grant-collection-read-permissions! (perms-group/all-users) collection-id)
+ (is (= ["card test card is returned"]
+ (->> (mt/user-http-request :lucky :get 200 "search" :archived true :q "test")
+ :data
+ (map :name)))))))
--
GitLab