diff --git a/e2e/support/cypress_sample_instance_data.js b/e2e/support/cypress_sample_instance_data.js
index 4f4bdf418054a5e6e685d49fd7cecb9abf247e6a..d4d699f80f627685f8995eac3ea2551acee2fac6 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 bfb8d4a177713fdbab059af3bc4eb968a7f8067f..500265c6b930c8012e629d1829546d06d4908dfe 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 77ad2b34e0afc56f486ba1054ed4cc3d693efcce..de34977a6299a3078cb4e9d9d8e08b691975a87f 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 2889a7d416afe7e08e0b3e6266436838306c6069..01c2e166bbde07645e7f9354e71193081d581860 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 0a4329dc7a5f00eec7ab54d3dd6935ede5e303a8..2a53be28f8546569047177720f6b498fbb47b449 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 13c0bd030c406d54742eb92df17cd5bc03007c93..4fbec8df0a65e189bc800b947ded624b7c8b5d74 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 e9d36c956301ed705854a64d5318499eff94def3..581f34396c73436b1588bbdcd9e309ed56025e13 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)))))))