From dac50d07ef078a3ed563bfbc3219696fd400e009 Mon Sep 17 00:00:00 2001
From: github-automation-metabase
<166700802+github-automation-metabase@users.noreply.github.com>
Date: Mon, 19 Aug 2024 18:38:58 -0400
Subject: [PATCH] resolves conflicts (#46988)
Co-authored-by: Sloan Sparger <sloansparger@gmail.com>
---
.../helpers/e2e-permissions-helpers.js | 24 +
.../permissions/admin-permissions.cy.spec.js | 10 +
.../permissions-reproductions.cy.spec.js | 39 --
.../permissions/view-data.cy.spec.js | 438 +++++++++++-------
.../middleware/row_level_restrictions.clj | 16 +-
.../permissions/block_permissions_test.clj | 6 +-
.../sandbox/api/card_test.clj | 31 ++
.../row_level_restrictions_test.clj | 34 +-
.../advanced_permissions/graph.ts | 71 ++-
.../advanced_permissions/graph.unit.spec.ts | 13 +-
.../advanced_permissions/index.js | 29 +-
.../EditSandboxingModal.tsx | 13 +-
.../EditSandboxingModal.unit.spec.tsx | 6 +-
.../src/metabase-types/api/permissions.ts | 4 +-
.../DataPermissionsHelp.tsx | 30 +-
.../PermissionHelpDescription.tsx | 7 +-
.../metabase/admin/permissions/permissions.js | 31 +-
.../{confirmations.ts => confirmations.tsx} | 96 +++-
.../selectors/data-permissions/fields.ts | 9 +-
.../data-permissions/permission-editor.ts | 4 +-
.../selectors/data-permissions/schemas.ts | 12 +-
.../selectors/data-permissions/tables.ts | 30 +-
.../src/metabase/admin/permissions/types.ts | 4 +-
.../admin/permissions/utils/data-entity-id.ts | 20 +-
.../utils/graph/data-permissions.ts | 174 ++++---
.../utils/graph/data-permissions.unit.spec.ts | 216 +++++++++
.../utils/graph/partial-updates.ts | 12 +-
.../utils/graph/partial-updates.unit.spec.ts | 39 +-
.../ConfirmContent/ConfirmContent.tsx | 28 +-
frontend/src/metabase/plugins/index.ts | 4 +-
src/metabase/api/permission_graph.clj | 17 +-
src/metabase/models/data_permissions.clj | 46 +-
.../models/data_permissions/graph.clj | 3 +-
src/metabase/models/query/permissions.clj | 100 ++--
.../middleware/permissions.clj | 9 +-
test/metabase/api/card_test.clj | 170 +++++--
test/metabase/api/database_test.clj | 8 +
test/metabase/api/dataset_test.clj | 1 -
.../models/data_permissions/graph_test.clj | 18 +
.../metabase/models/data_permissions_test.clj | 29 +-
test/metabase/query_processor/card_test.clj | 3 +
41 files changed, 1333 insertions(+), 521 deletions(-)
rename frontend/src/metabase/admin/permissions/selectors/{confirmations.ts => confirmations.tsx} (68%)
create mode 100644 frontend/src/metabase/admin/permissions/utils/graph/data-permissions.unit.spec.ts
diff --git a/e2e/support/helpers/e2e-permissions-helpers.js b/e2e/support/helpers/e2e-permissions-helpers.js
index 16c96896eae..1895b188267 100644
--- a/e2e/support/helpers/e2e-permissions-helpers.js
+++ b/e2e/support/helpers/e2e-permissions-helpers.js
@@ -98,3 +98,27 @@ export const dismissSplitPermsModal = () => {
.findByRole("button", { name: "Got it" })
.click();
};
+
+export function savePermissions() {
+ cy.findByTestId("edit-bar").button("Save changes").click();
+ cy.findByRole("dialog").findByText("Yes").click();
+ cy.findByTestId("edit-bar").should("not.exist");
+}
+
+export function selectImpersonatedAttribute(attribute) {
+ cy.findByRole("dialog").within(() => {
+ cy.findByTestId("select-button").click();
+ });
+
+ popover().findByText(attribute).click();
+}
+
+export function saveImpersonationSettings() {
+ cy.findByRole("dialog").findByText("Save").click();
+}
+
+export function assertSameBeforeAndAfterSave(assertionCallback) {
+ assertionCallback();
+ savePermissions();
+ assertionCallback();
+}
diff --git a/e2e/test/scenarios/permissions/admin-permissions.cy.spec.js b/e2e/test/scenarios/permissions/admin-permissions.cy.spec.js
index 115f30e5095..74bb3872d27 100644
--- a/e2e/test/scenarios/permissions/admin-permissions.cy.spec.js
+++ b/e2e/test/scenarios/permissions/admin-permissions.cy.spec.js
@@ -54,6 +54,16 @@ describe("scenarios > admin > permissions", { tags: "@OSS" }, () => {
]);
});
+ it("should not show view data column on OSS", () => {
+ cy.visit(`/admin/permissions/data/group/${ALL_USERS_GROUP}`);
+
+ cy.findByTestId("permission-table").within(() => {
+ cy.findByText("Database name").should("exist");
+ cy.findByText("View data").should("not.exist");
+ cy.findByText("Create queries").should("exist");
+ });
+ });
+
it("should display error on failed save", () => {
// revoke some permissions
cy.visit(`/admin/permissions/data/group/${ALL_USERS_GROUP}`);
diff --git a/e2e/test/scenarios/permissions/permissions-reproductions.cy.spec.js b/e2e/test/scenarios/permissions/permissions-reproductions.cy.spec.js
index aea901d0ad7..ba501bfcb71 100644
--- a/e2e/test/scenarios/permissions/permissions-reproductions.cy.spec.js
+++ b/e2e/test/scenarios/permissions/permissions-reproductions.cy.spec.js
@@ -164,45 +164,6 @@ describeEE("postgres > user > query", { tags: "@external" }, () => {
});
});
-describeEE("issue 17763", () => {
- beforeEach(() => {
- restore();
- cy.signInAsAdmin();
- setTokenFeatures("all");
-
- cy.updatePermissionsGraph({
- [ALL_USERS_GROUP]: {
- 1: {
- "view-data": "blocked",
- "create-queries": "no",
- },
- },
- });
- });
-
- it('should be able to edit tables permissions in granular view after "block" permissions (metabase#17763)', () => {
- cy.visit(`/admin/permissions/data/database/${SAMPLE_DB_ID}`);
-
- // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
- cy.findByText("Blocked").click();
-
- popover().contains("Granular").click();
-
- cy.location("pathname").should(
- "eq",
- `/admin/permissions/data/group/${ALL_USERS_GROUP}/database/${SAMPLE_DB_ID}`,
- );
-
- cy.findByTestId("permission-table").within(() => {
- cy.findAllByText("Can view").first().click();
- });
-
- popover().within(() => {
- cy.findByText("Sandboxed");
- });
- });
-});
-
describe.skip("issue 17777", () => {
function hideTables(tables) {
cy.request("PUT", "/api/table", {
diff --git a/e2e/test/scenarios/permissions/view-data.cy.spec.js b/e2e/test/scenarios/permissions/view-data.cy.spec.js
index 5aa48b3859b..aebde2c9f5d 100644
--- a/e2e/test/scenarios/permissions/view-data.cy.spec.js
+++ b/e2e/test/scenarios/permissions/view-data.cy.spec.js
@@ -1,5 +1,6 @@
import { SAMPLE_DB_ID, USER_GROUPS } from "e2e/support/cypress_data";
import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
+import { ORDERS_QUESTION_ID } from "e2e/support/cypress_sample_instance_data";
import {
assertPermissionTable,
modal,
@@ -14,14 +15,22 @@ import {
getPermissionRowPermissions,
createTestRoles,
selectPermissionRow,
+ savePermissions,
+ selectImpersonatedAttribute,
+ saveImpersonationSettings,
+ assertSameBeforeAndAfterSave,
+ createNativeQuestion,
+ visitQuestion,
} from "e2e/support/helpers";
const { ORDERS_ID } = SAMPLE_DATABASE;
const { ALL_USERS_GROUP, COLLECTION_GROUP } = USER_GROUPS;
-const DATA_ACCESS_PERMISSION_INDEX = 0;
-const NATIVE_QUERIES_PERMISSION_INDEX = 1;
-const DOWNLOAD_RESULTS_PERMISSION_INDEX = 2;
+const DATA_ACCESS_PERM_IDX = 0;
+const CREATE_QUERIES_PERM_IDX = 1;
+const DOWNLOAD_PERM_IDX = 2;
+
+// EDITOR RELATED TESTS
describeEE("scenarios > admin > permissions > view data > blocked", () => {
beforeEach(() => {
@@ -30,59 +39,83 @@ describeEE("scenarios > admin > permissions > view data > blocked", () => {
setTokenFeatures("all");
});
+ const g = "All Users";
+
it("should allow saving 'blocked' and disable create queries dropdown when set", () => {
- cy.visit(`/admin/permissions/data/database/${SAMPLE_DB_ID}`);
+ cy.visit(
+ `/admin/permissions/data/database/${SAMPLE_DB_ID}/schema/PUBLIC/table/${ORDERS_ID}`, // table level
+ );
- // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
- cy.findByText("All Users")
- .closest("tr")
- .as("allUsersRow")
- .within(() => {
- isPermissionDisabled(
- DATA_ACCESS_PERMISSION_INDEX,
- "Can view",
- false,
- ).click();
- isPermissionDisabled(NATIVE_QUERIES_PERMISSION_INDEX, "No", false);
- });
+ assertPermissionForItem(g, DATA_ACCESS_PERM_IDX, "Can view", false);
+ assertPermissionForItem(g, CREATE_QUERIES_PERM_IDX, "No", false);
+ assertPermissionForItem(g, DOWNLOAD_PERM_IDX, "1 million rows", false);
- popover().contains("Block").click();
+ modifyPermission(g, DATA_ACCESS_PERM_IDX, "Blocked");
- cy.get("@allUsersRow").within(() => {
- isPermissionDisabled(DATA_ACCESS_PERMISSION_INDEX, "Block", false);
- isPermissionDisabled(NATIVE_QUERIES_PERMISSION_INDEX, "No", true);
+ assertSameBeforeAndAfterSave(() => {
+ assertPermissionForItem(g, DATA_ACCESS_PERM_IDX, "Blocked", false);
+ assertPermissionForItem(g, CREATE_QUERIES_PERM_IDX, "No", true);
+ assertPermissionForItem(g, DOWNLOAD_PERM_IDX, "No", true);
});
- cy.button("Save changes").click();
+ cy.visit(`/admin/permissions/data/database/${SAMPLE_DB_ID}`); // database level
- modal().within(() => {
- cy.findByText("Save permissions?");
- cy.button("Yes").click();
+ assertPermissionForItem(g, DATA_ACCESS_PERM_IDX, "Granular", false);
+ assertPermissionForItem(g, CREATE_QUERIES_PERM_IDX, "No", false);
+ assertPermissionForItem(g, DOWNLOAD_PERM_IDX, "1 million rows", false);
+
+ modifyPermission(g, DATA_ACCESS_PERM_IDX, "Blocked");
+
+ assertSameBeforeAndAfterSave(() => {
+ assertPermissionForItem(g, DATA_ACCESS_PERM_IDX, "Blocked", false);
+ assertPermissionForItem(g, CREATE_QUERIES_PERM_IDX, "No", true);
+ assertPermissionForItem(g, DOWNLOAD_PERM_IDX, "No", true);
});
+ });
- assertPermissionTable([
- [
- "Administrators",
- "Can view",
- "Query builder and native",
- "1 million rows",
- "Yes",
- "Yes",
- ],
- // expect that the view data permissions has been automatically droped to query builder only
- ["All Users", "Blocked", "No", "No", "No", "No"],
- ["collection", "Can view", "No", "1 million rows", "No", "No"],
- [
- "data",
- "Can view",
- "Query builder and native",
- "1 million rows",
- "No",
- "No",
- ],
- ["nosql", "Can view", "Query builder only", "1 million rows", "No", "No"],
- ["readonly", "Can view", "No", "1 million rows", "No", "No"],
- ]);
+ it("should prevent user from upgrading db/schema create query permissions if a child schema/table contains blocked permissions", () => {
+ cy.visit(
+ `/admin/permissions/data/group/${ALL_USERS_GROUP}/database/${SAMPLE_DB_ID}`,
+ ); // table level
+
+ cy.log("ensure that modify tables do not affect other rows");
+ // this is the test is admittedly a little opaque, we're trying to test that
+ // the calculation that upgrades view data permissions does not apply to tables
+ modifyPermission("Orders", DATA_ACCESS_PERM_IDX, "Blocked"); // add blocked to one table
+ modifyPermission("Products", CREATE_QUERIES_PERM_IDX, "Query builder only"); // increase permissions of another table to trigger upgrade flow
+ assertPermissionForItem("Orders", DATA_ACCESS_PERM_IDX, "Blocked"); // orders table should stay the same
+
+ selectSidebarItem("All Users");
+ assertPermissionForItem(
+ "Sample Database",
+ DATA_ACCESS_PERM_IDX,
+ "Granular",
+ );
+ modifyPermission(
+ "Sample Database",
+ CREATE_QUERIES_PERM_IDX,
+ "Query builder only",
+ );
+
+ modal()
+ .should("exist")
+ .within(() => {
+ cy.findByText(
+ "This will also set the View Data permission to “Can View†to allow this group to create queries. Okay?",
+ ).should("exist");
+ cy.findByText("Okay").click();
+ });
+
+ assertPermissionForItem(
+ "Sample Database",
+ DATA_ACCESS_PERM_IDX,
+ "Can view",
+ );
+ assertPermissionForItem(
+ "Sample Database",
+ CREATE_QUERIES_PERM_IDX,
+ "Query builder only",
+ );
});
});
@@ -104,24 +137,6 @@ describe("scenarios > admin > permissions > view data > granular", () => {
});
describeEE("scenarios > admin > permissions > view data > granular", () => {
- function makeOrdersSandboxed() {
- modifyPermission("Orders", DATA_ACCESS_PERMISSION_INDEX, "Sandboxed");
-
- cy.url().should(
- "include",
- `/admin/permissions/data/group/${ALL_USERS_GROUP}/database/${SAMPLE_DB_ID}/schema/PUBLIC/${ORDERS_ID}/segmented`,
- );
-
- cy.findByText("Grant sandboxed access to this table");
- cy.button("Save").should("be.disabled");
-
- cy.findByText("Pick a column").click();
- cy.findByText("User ID").click();
-
- cy.findByText("Pick a user attribute").click();
- cy.findByText("attr_uid").click();
- cy.button("Save").click();
- }
beforeEach(() => {
restore();
cy.signInAsAdmin();
@@ -132,7 +147,7 @@ describeEE("scenarios > admin > permissions > view data > granular", () => {
it("should allow making permissions granular in the database focused view", () => {
cy.visit(`/admin/permissions/data/database/${SAMPLE_DB_ID}`);
- modifyPermission("All Users", DATA_ACCESS_PERMISSION_INDEX, "Granular");
+ modifyPermission("All Users", DATA_ACCESS_PERM_IDX, "Granular");
cy.url().should(
"include",
@@ -165,11 +180,7 @@ describeEE("scenarios > admin > permissions > view data > granular", () => {
it("should allow making permissions granular in the group focused view", () => {
cy.visit(`/admin/permissions/data/group/${ALL_USERS_GROUP}`);
- modifyPermission(
- "Sample Database",
- DATA_ACCESS_PERMISSION_INDEX,
- "Granular",
- );
+ modifyPermission("Sample Database", DATA_ACCESS_PERM_IDX, "Granular");
cy.url().should(
"include",
@@ -204,11 +215,7 @@ describeEE("scenarios > admin > permissions > view data > granular", () => {
cy.visit(`/admin/permissions/data/group/${ALL_USERS_GROUP}`);
- modifyPermission(
- "Sample Database",
- DATA_ACCESS_PERMISSION_INDEX,
- "Granular",
- );
+ modifyPermission("Sample Database", DATA_ACCESS_PERM_IDX, "Granular");
makeOrdersSandboxed();
@@ -224,7 +231,7 @@ describeEE("scenarios > admin > permissions > view data > granular", () => {
.closest("a")
.click();
- modifyPermission("Orders", DATA_ACCESS_PERMISSION_INDEX, "Can view");
+ modifyPermission("Orders", DATA_ACCESS_PERM_IDX, "Can view");
selectSidebarItem("All Users");
@@ -233,22 +240,14 @@ describeEE("scenarios > admin > permissions > view data > granular", () => {
]);
});
- it("should set a new default for children if parent is currently selected to a top-level only permission before going granular", () => {
+ it("should preserve parent value for children when selecting granular for permissions available to child entities", () => {
cy.visit(`/admin/permissions/data/group/${ALL_USERS_GROUP}`);
- modifyPermission(
- "Sample Database",
- DATA_ACCESS_PERMISSION_INDEX,
- "Blocked",
- );
+ modifyPermission("Sample Database", DATA_ACCESS_PERM_IDX, "Blocked");
- modifyPermission(
- "Sample Database",
- DATA_ACCESS_PERMISSION_INDEX,
- "Granular",
- );
+ modifyPermission("Sample Database", DATA_ACCESS_PERM_IDX, "Granular");
- assertPermissionForItem("Orders", DATA_ACCESS_PERMISSION_INDEX, "Can view");
+ assertPermissionForItem("Orders", DATA_ACCESS_PERM_IDX, "Blocked");
});
});
@@ -264,15 +263,11 @@ describeEE("scenarios > admin > permissions > view data > impersonated", () => {
cy.visit(`/admin/permissions/data/group/${ALL_USERS_GROUP}`);
// Check there is no Impersonated option on H2
- selectPermissionRow("Sample Database", DATA_ACCESS_PERMISSION_INDEX);
+ selectPermissionRow("Sample Database", DATA_ACCESS_PERM_IDX);
popover().should("not.contain", "Impersonated");
// Set impersonated access on Postgres database
- modifyPermission(
- "QA Postgres12",
- DATA_ACCESS_PERMISSION_INDEX,
- "Impersonated",
- );
+ modifyPermission("QA Postgres12", DATA_ACCESS_PERM_IDX, "Impersonated");
selectImpersonatedAttribute("role");
saveImpersonationSettings();
@@ -312,7 +307,7 @@ describeEE("scenarios > admin > permissions > view data > impersonated", () => {
// Edit impersonated permission
modifyPermission(
"QA Postgres12",
- DATA_ACCESS_PERMISSION_INDEX,
+ DATA_ACCESS_PERM_IDX,
"Edit Impersonated",
);
@@ -329,11 +324,7 @@ describeEE("scenarios > admin > permissions > view data > impersonated", () => {
it("should warns when All Users group has 'impersonated' access and the target group has unrestricted access", () => {
cy.visit(`/admin/permissions/data/group/${COLLECTION_GROUP}`);
- modifyPermission(
- "QA Postgres12",
- DATA_ACCESS_PERMISSION_INDEX,
- "Impersonated",
- );
+ modifyPermission("QA Postgres12", DATA_ACCESS_PERM_IDX, "Impersonated");
// Warns that All Users group has greater access
cy.findByRole("dialog").within(() => {
@@ -349,7 +340,7 @@ describeEE("scenarios > admin > permissions > view data > impersonated", () => {
savePermissions();
getPermissionRowPermissions("QA Postgres12")
- .eq(DATA_ACCESS_PERMISSION_INDEX)
+ .eq(DATA_ACCESS_PERM_IDX)
.findByLabelText("warning icon")
.realHover();
@@ -361,17 +352,13 @@ describeEE("scenarios > admin > permissions > view data > impersonated", () => {
it("allows switching to the granular access and update table permissions", () => {
cy.visit(`/admin/permissions/data/group/${ALL_USERS_GROUP}`);
- modifyPermission(
- "QA Postgres12",
- DATA_ACCESS_PERMISSION_INDEX,
- "Impersonated",
- );
+ modifyPermission("QA Postgres12", DATA_ACCESS_PERM_IDX, "Impersonated");
selectImpersonatedAttribute("role");
saveImpersonationSettings();
savePermissions();
- modifyPermission("QA Postgres12", DATA_ACCESS_PERMISSION_INDEX, "Granular");
+ modifyPermission("QA Postgres12", DATA_ACCESS_PERM_IDX, "Granular");
// Resets table permissions from Impersonated to Can view
assertPermissionTable(
@@ -407,18 +394,14 @@ describeEE("scenarios > admin > permissions > view data > impersonated", () => {
// Try leaving the page
cy.visit(`/admin/permissions/data/group/${ALL_USERS_GROUP}`);
- modifyPermission(
- "QA Postgres12",
- DATA_ACCESS_PERMISSION_INDEX,
- "Impersonated",
- );
+ modifyPermission("QA Postgres12", DATA_ACCESS_PERM_IDX, "Impersonated");
selectImpersonatedAttribute("role");
saveImpersonationSettings();
modifyPermission(
"QA Postgres12",
- DATA_ACCESS_PERMISSION_INDEX,
+ DATA_ACCESS_PERM_IDX,
"Edit Impersonated",
);
@@ -444,6 +427,19 @@ describeEE("scenarios > admin > permissions > view data > impersonated", () => {
cy.focused().should("have.attr", "placeholder", "username");
});
+
+ it("should set unrestricted for children if database is set to impersonated before going granular", () => {
+ cy.visit(`/admin/permissions/data/group/${ALL_USERS_GROUP}`);
+
+ modifyPermission("QA Postgres12", DATA_ACCESS_PERM_IDX, "Impersonated");
+
+ selectImpersonatedAttribute("role");
+ saveImpersonationSettings();
+
+ modifyPermission("Sample Database", DATA_ACCESS_PERM_IDX, "Granular");
+
+ assertPermissionForItem("Orders", DATA_ACCESS_PERM_IDX, "Can view");
+ });
});
describeEE(
@@ -460,11 +456,11 @@ describeEE(
// and test that it does not exist
cy.visit(`/admin/permissions/data/group/${ALL_USERS_GROUP}`);
- selectPermissionRow("Sample Database", DATA_ACCESS_PERMISSION_INDEX);
+ selectPermissionRow("Sample Database", DATA_ACCESS_PERM_IDX);
popover().should("not.contain", "No self-service (Deprecated)");
- selectPermissionRow("Sample Database", NATIVE_QUERIES_PERMISSION_INDEX);
- isPermissionDisabled(NATIVE_QUERIES_PERMISSION_INDEX, "No", false);
+ selectPermissionRow("Sample Database", CREATE_QUERIES_PERM_IDX);
+ isPermissionDisabled(CREATE_QUERIES_PERM_IDX, "No", false);
// load the page w/ legacy value in the graph and test that it does exist
cy.reload();
@@ -496,32 +492,24 @@ describeEE(
]);
// User should not be able to modify Create queries permission while set to legacy-no-self-service
- isPermissionDisabled(NATIVE_QUERIES_PERMISSION_INDEX, "No", true);
+ isPermissionDisabled(CREATE_QUERIES_PERM_IDX, "No", true);
- modifyPermission(
- "Sample Database",
- DATA_ACCESS_PERMISSION_INDEX,
- "Can view",
- );
+ modifyPermission("Sample Database", DATA_ACCESS_PERM_IDX, "Can view");
modifyPermission(
"Sample Database",
- NATIVE_QUERIES_PERMISSION_INDEX,
+ CREATE_QUERIES_PERM_IDX,
"Query builder and native",
);
modifyPermission(
"Sample Database",
- DATA_ACCESS_PERMISSION_INDEX,
+ DATA_ACCESS_PERM_IDX,
"No self-service (Deprecated)",
);
// change something else so we can save
- modifyPermission(
- "Sample Database",
- DOWNLOAD_RESULTS_PERMISSION_INDEX,
- "No",
- );
+ modifyPermission("Sample Database", DOWNLOAD_PERM_IDX, "No");
// User setting the value back to legacy-no-self-service should result in Create queries going back to No
const finalExpectedRows = [
@@ -568,13 +556,13 @@ describeEE("scenarios > admin > permissions > view data > sandboxed", () => {
// permissions are droped to query builder only after we sandbox a table
modifyPermission(
"All Users",
- NATIVE_QUERIES_PERMISSION_INDEX,
+ CREATE_QUERIES_PERM_IDX,
"Query builder and native",
);
selectSidebarItem("Orders");
- modifyPermission("All Users", DATA_ACCESS_PERMISSION_INDEX, "Sandboxed");
+ modifyPermission("All Users", DATA_ACCESS_PERM_IDX, "Sandboxed");
modal().within(() => {
cy.findByText("Change access to this database to “Sandboxed�");
@@ -586,7 +574,7 @@ describeEE("scenarios > admin > permissions > view data > sandboxed", () => {
`/admin/permissions/data/database/${SAMPLE_DB_ID}/schema/PUBLIC/table/${ORDERS_ID}/segmented/group/${ALL_USERS_GROUP}`,
);
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
- cy.findByText("Grant sandboxed access to this table");
+ cy.findByText("Restrict access to this table");
cy.button("Save").should("be.disabled");
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
@@ -619,7 +607,7 @@ describeEE("scenarios > admin > permissions > view data > sandboxed", () => {
modifyPermission(
"All Users",
- DATA_ACCESS_PERMISSION_INDEX,
+ DATA_ACCESS_PERM_IDX,
"Edit sandboxed access",
);
@@ -628,11 +616,11 @@ describeEE("scenarios > admin > permissions > view data > sandboxed", () => {
`/admin/permissions/data/database/${SAMPLE_DB_ID}/schema/PUBLIC/table/${ORDERS_ID}/segmented/group/${ALL_USERS_GROUP}`,
);
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
- cy.findByText("Grant sandboxed access to this table");
+ cy.findByText("Restrict access to this table");
cy.button("Save").click();
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
- cy.findByText("Grant sandboxed access to this table").should("not.exist");
+ cy.findByText("Restrict access to this table").should("not.exist");
cy.button("Save changes").click();
@@ -647,13 +635,13 @@ describeEE("scenarios > admin > permissions > view data > sandboxed", () => {
// permissions are droped to query builder only after we sandbox a table
modifyPermission(
"Sample Database",
- NATIVE_QUERIES_PERMISSION_INDEX,
+ CREATE_QUERIES_PERM_IDX,
"Query builder and native",
);
cy.get("a").contains("Sample Database").click();
- modifyPermission("Orders", DATA_ACCESS_PERMISSION_INDEX, "Sandboxed");
+ modifyPermission("Orders", DATA_ACCESS_PERM_IDX, "Sandboxed");
modal().within(() => {
cy.findByText("Change access to this database to “Sandboxed�");
@@ -665,7 +653,7 @@ describeEE("scenarios > admin > permissions > view data > sandboxed", () => {
`/admin/permissions/data/group/${ALL_USERS_GROUP}/database/${SAMPLE_DB_ID}/schema/PUBLIC/${ORDERS_ID}/segmented`,
);
modal().within(() => {
- cy.findByText("Grant sandboxed access to this table");
+ cy.findByText("Restrict access to this table");
cy.button("Save").should("be.disabled");
cy.findByText("Pick a column").click();
});
@@ -694,18 +682,14 @@ describeEE("scenarios > admin > permissions > view data > sandboxed", () => {
assertPermissionTable(expectedFinalPermissions);
- modifyPermission(
- "Orders",
- DATA_ACCESS_PERMISSION_INDEX,
- "Edit sandboxed access",
- );
+ modifyPermission("Orders", DATA_ACCESS_PERM_IDX, "Edit sandboxed access");
cy.url().should(
"include",
`/admin/permissions/data/group/${ALL_USERS_GROUP}/database/${SAMPLE_DB_ID}/schema/PUBLIC/${ORDERS_ID}/segmented`,
);
- modal().findByText("Grant sandboxed access to this table");
+ modal().findByText("Restrict access to this table");
cy.button("Save").click();
@@ -752,10 +736,10 @@ describeEE(
cy.get("a").contains("Sample Database").click();
- modifyPermission("Orders", DATA_ACCESS_PERMISSION_INDEX, "Sandboxed");
+ modifyPermission("Orders", DATA_ACCESS_PERM_IDX, "Sandboxed");
modal().within(() => {
- cy.findByText("Grant sandboxed access to this table");
+ cy.findByText("Restrict access to this table");
cy.button("Save").should("be.disabled");
cy.findByText("Pick a column").click();
});
@@ -765,11 +749,7 @@ describeEE(
popover().findByText("attr_uid").click();
modal().button("Save").click();
- modifyPermission(
- "Orders",
- NATIVE_QUERIES_PERMISSION_INDEX,
- "Query builder only",
- );
+ modifyPermission("Orders", CREATE_QUERIES_PERM_IDX, "Query builder only");
savePermissions();
@@ -777,14 +757,10 @@ describeEE(
expect(response.statusCode).to.equal(200);
});
+ assertPermissionForItem("Orders", DATA_ACCESS_PERM_IDX, "Sandboxed");
assertPermissionForItem(
"Orders",
- DATA_ACCESS_PERMISSION_INDEX,
- "Sandboxed",
- );
- assertPermissionForItem(
- "Orders",
- NATIVE_QUERIES_PERMISSION_INDEX,
+ CREATE_QUERIES_PERM_IDX,
"Query builder only",
);
});
@@ -800,18 +776,14 @@ describeEE(
cy.visit(`/admin/permissions/data/group/${ALL_USERS_GROUP}`);
// Set impersonated access on Postgres database
- modifyPermission(
- "QA Postgres12",
- DATA_ACCESS_PERMISSION_INDEX,
- "Impersonated",
- );
+ modifyPermission("QA Postgres12", DATA_ACCESS_PERM_IDX, "Impersonated");
selectImpersonatedAttribute("role");
saveImpersonationSettings();
modifyPermission(
"QA Postgres12",
- NATIVE_QUERIES_PERMISSION_INDEX,
+ CREATE_QUERIES_PERM_IDX,
"Query builder only",
);
@@ -823,12 +795,12 @@ describeEE(
assertPermissionForItem(
"QA Postgres12",
- DATA_ACCESS_PERMISSION_INDEX,
+ DATA_ACCESS_PERM_IDX,
"Impersonated",
);
assertPermissionForItem(
"QA Postgres12",
- NATIVE_QUERIES_PERMISSION_INDEX,
+ CREATE_QUERIES_PERM_IDX,
"Query builder only",
);
});
@@ -844,7 +816,7 @@ describeEE("scenarios > admin > permissions > view data > unrestricted", () => {
it("should allow perms to be set to from 'can view' to 'block' and back from database view", () => {
cy.visit(`/admin/permissions/data/database/${SAMPLE_DB_ID}`);
- modifyPermission("All Users", DATA_ACCESS_PERMISSION_INDEX, "Blocked");
+ modifyPermission("All Users", DATA_ACCESS_PERM_IDX, "Blocked");
cy.intercept("PUT", "/api/permissions/graph").as("saveGraph");
@@ -859,7 +831,7 @@ describeEE("scenarios > admin > permissions > view data > unrestricted", () => {
expect(response.statusCode).to.equal(200);
});
- modifyPermission("All Users", DATA_ACCESS_PERMISSION_INDEX, "Can view");
+ modifyPermission("All Users", DATA_ACCESS_PERM_IDX, "Can view");
cy.button("Save changes").click();
@@ -874,20 +846,128 @@ describeEE("scenarios > admin > permissions > view data > unrestricted", () => {
});
});
-function savePermissions() {
- cy.findByTestId("edit-bar").button("Save changes").click();
- cy.findByRole("dialog").findByText("Yes").click();
- cy.findByTestId("edit-bar").should("not.exist");
+// ENFORMCENT RELATED TESTS
+
+describeEE(
+ "scenarios > admin > permissions > view data > blocked (enforcement)",
+ () => {
+ beforeEach(() => {
+ restore();
+ cy.signInAsAdmin();
+ setTokenFeatures("all");
+ });
+
+ it("should deny view access to a query builder question that makes use of a blocked table", () => {
+ assertCollectionGroupUserHasAccess(ORDERS_QUESTION_ID, true);
+ cy.visit(
+ `/admin/permissions/data/database/${SAMPLE_DB_ID}/schema/PUBLIC/table/${ORDERS_ID}`,
+ );
+ removeCollectionGroupPermissions();
+ assertCollectionGroupHasNoAccess(ORDERS_QUESTION_ID, true);
+ });
+
+ it("should deny view access to a query builder question that makes use of a blocked database", () => {
+ assertCollectionGroupUserHasAccess(ORDERS_QUESTION_ID, true);
+ cy.visit(`/admin/permissions/data/database/${SAMPLE_DB_ID}`);
+ removeCollectionGroupPermissions();
+ assertCollectionGroupHasNoAccess(ORDERS_QUESTION_ID, true);
+ });
+
+ it("should deny view access to any native question if the user has blocked view data for any table or database", () => {
+ createNativeQuestion({
+ native: { query: "select 1" },
+ }).then(({ body: { id: nativeQuestionId } }) => {
+ assertCollectionGroupUserHasAccess(nativeQuestionId, false);
+ cy.visit(
+ `/admin/permissions/data/database/${SAMPLE_DB_ID}/schema/PUBLIC/table/${ORDERS_ID}`,
+ );
+ removeCollectionGroupPermissions();
+ assertCollectionGroupHasNoAccess(nativeQuestionId, false);
+ });
+ });
+ },
+);
+
+function lackPermissionsView(isQbQuestion, shouldExist) {
+ if (isQbQuestion) {
+ cy.findByText("There was a problem with your question").should(
+ shouldExist ? "exist" : "not.exist",
+ );
+
+ if (shouldExist) {
+ cy.findByText("Show error details").click();
+ }
+ }
+
+ cy.findByText(/You do not have permissions to run this query/).should(
+ shouldExist ? "exist" : "not.exist",
+ );
}
-function selectImpersonatedAttribute(attribute) {
- cy.findByRole("dialog").within(() => {
- cy.findByTestId("select-button").click();
- });
+// NOTE: all helpers below make user of the "sandboxed" user and "collection" group to test permissions
+// as this user is of only one group and has permission to view existing question
+
+function assertCollectionGroupUserHasAccess(questionId, isQbQuestion) {
+ cy.signOut();
+ cy.signIn("sandboxed");
+
+ visitQuestion(questionId);
+ lackPermissionsView(isQbQuestion, false);
+
+ cy.signOut();
+ cy.signInAsAdmin();
+}
+
+function assertCollectionGroupHasNoAccess(questionId, isQbQuestion) {
+ cy.signOut();
+ cy.signIn("sandboxed");
+
+ visitQuestion(questionId);
+
+ lackPermissionsView(isQbQuestion, true);
+}
- popover().findByText(attribute).click();
+function removeCollectionGroupPermissions() {
+ assertPermissionForItem("All Users", DATA_ACCESS_PERM_IDX, "Can view", false);
+ assertPermissionForItem(
+ "collection",
+ DATA_ACCESS_PERM_IDX,
+ "Can view",
+ false,
+ );
+ modifyPermission("All Users", DATA_ACCESS_PERM_IDX, "Blocked");
+ modifyPermission("collection", DATA_ACCESS_PERM_IDX, "Blocked");
+ assertSameBeforeAndAfterSave(() => {
+ assertPermissionForItem(
+ "All Users",
+ DATA_ACCESS_PERM_IDX,
+ "Blocked",
+ false,
+ );
+ assertPermissionForItem(
+ "collection",
+ DATA_ACCESS_PERM_IDX,
+ "Blocked",
+ false,
+ );
+ });
}
-function saveImpersonationSettings() {
- cy.findByRole("dialog").findByText("Save").click();
+function makeOrdersSandboxed() {
+ modifyPermission("Orders", DATA_ACCESS_PERM_IDX, "Sandboxed");
+
+ cy.url().should(
+ "include",
+ `/admin/permissions/data/group/${ALL_USERS_GROUP}/database/${SAMPLE_DB_ID}/schema/PUBLIC/${ORDERS_ID}/segmented`,
+ );
+
+ cy.findByText("Restrict access to this table");
+ cy.button("Save").should("be.disabled");
+
+ cy.findByText("Pick a column").click();
+ cy.findByText("User ID").click();
+
+ cy.findByText("Pick a user attribute").click();
+ cy.findByText("attr_uid").click();
+ cy.button("Save").click();
}
diff --git a/enterprise/backend/src/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions.clj b/enterprise/backend/src/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions.clj
index 40c1b09ca84..b580e7f6108 100644
--- a/enterprise/backend/src/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions.clj
+++ b/enterprise/backend/src/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions.clj
@@ -16,6 +16,8 @@
[metabase.lib.schema.id :as lib.schema.id]
[metabase.lib.util.match :as lib.util.match]
[metabase.models.card :refer [Card]]
+ [metabase.models.data-permissions :as data-perms]
+ [metabase.models.database :as database]
[metabase.models.query.permissions :as query-perms]
[metabase.public-settings.premium-features :refer [defenterprise]]
[metabase.query-processor.error-type :as qp.error-type]
@@ -259,8 +261,18 @@
(query-perms/required-perms-for-query (:dataset-query (lib.metadata.protocols/card (qp.store/metadata-provider) card-id))
:throw-exceptions? true))
- (let [table-ids (sandbox->table-ids sandbox)]
- {:perms/view-data (zipmap table-ids (repeat :unrestricted))
+ (let [table-ids (sandbox->table-ids sandbox)
+ table-id->db-id (into {} (mapv (juxt identity database/table-id->database-id) table-ids))
+ unblocked-table-ids (filter (fn [table-id] (data-perms/user-has-permission-for-table?
+ api/*current-user-id*
+ :perms/view-data
+ :unrestricted
+ (get table-id->db-id table-id)
+ table-id))
+ table-ids)]
+ ;; Here, we grant view-data to only unblocked table ids. Otherwise sandboxed users with a joined table that's
+ ;; _blocked_ can be queried against from the query builder
+ {:perms/view-data (zipmap unblocked-table-ids (repeat :unrestricted))
:perms/create-queries (zipmap table-ids (repeat :query-builder))})))
(defn- merge-perms
diff --git a/enterprise/backend/test/metabase_enterprise/advanced_permissions/models/permissions/block_permissions_test.clj b/enterprise/backend/test/metabase_enterprise/advanced_permissions/models/permissions/block_permissions_test.clj
index 19100fe03d5..5028648a405 100644
--- a/enterprise/backend/test/metabase_enterprise/advanced_permissions/models/permissions/block_permissions_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/advanced_permissions/models/permissions/block_permissions_test.clj
@@ -119,12 +119,12 @@
(is (not (t2/exists? GroupTableAccessPolicy :group_id group-id))))))))
(deftest update-graph-data-perms-should-delete-block-perms-test
- (testing "granting data permissions for a table should delete existing block permissions"
+ (testing "granting data permissions for a table should not delete existing block permissions"
(mt/with-temp [PermissionsGroup {group-id :id} {}]
(data-perms/set-database-permission! group-id (mt/id) :perms/view-data :blocked)
(is (nil? (test-db-perms group-id)))
(data-perms/set-table-permission! group-id (mt/id :venues) :perms/view-data :unrestricted)
- (is (= {"PUBLIC" :unrestricted}
+ (is (= {"PUBLIC" {(mt/id :venues) :unrestricted}}
(test-db-perms group-id))))))
(deftest update-graph-disallow-native-query-perms-test
@@ -216,7 +216,7 @@
(check-block-perms)))
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
- #"Blocked: you are not allowed to run queries against Database \d+"
+ #"You do not have permissions to run this query"
(run-saved-question))))))))))))
(deftest legacy-no-self-service-test
diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/api/card_test.clj b/enterprise/backend/test/metabase_enterprise/sandbox/api/card_test.clj
index 9a301733fce..fa96a10172c 100644
--- a/enterprise/backend/test/metabase_enterprise/sandbox/api/card_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/sandbox/api/card_test.clj
@@ -102,6 +102,37 @@
(api.card-test/mbql-count-query db table))
:collection_id (u/the-id collection)))))))))))
+(deftest users-with-data-access-and-query-create-may-access-cards
+ (mt/with-temp [:model/User {user-id :id} {}
+ :model/PermissionsGroup group {}
+ :model/PermissionsGroupMembership _ {:user_id user-id
+ :group_id (u/the-id group)}
+ :model/Card card {:name "Some Name" :dataset_query {:database (mt/id),
+ :type :query,
+ :query {:source-table (mt/id :venues)}}}]
+ (let [cases [[:unrestricted :query-builder-and-native "Request Permitted"]
+ [:unrestricted :query-builder "Request Permitted"]
+ [:unrestricted :no "Request Permitted"]
+ [:legacy-no-self-service :no "You do not have permissions to run this query."]
+ [:blocked :no "You do not have permissions to run this query."]]
+ ;; These are invalid permission combinations, so we don't test them:
+ invalid-cases [[:legacy-no-self-service :query-builder-and-native]
+ [:legacy-no-self-service :query-builder]
+ [:blocked :query-builder-and-native]
+ [:blocked :query-builder]]]
+ (is (= (count cases)
+ (- (* (-> data-perms/Permissions :perms/view-data :values count)
+ (-> data-perms/Permissions :perms/create-queries :values count))
+ (count invalid-cases)))
+ "Please test these permissions settings behaviors exhaustively: if you add perms, add the tests for them.")
+ (mt/with-no-data-perms-for-all-users!
+ (doseq [[view-perm create-perm expected] cases]
+ (data-perms/set-table-permission! group (mt/id :venues) :perms/view-data view-perm)
+ (data-perms/set-table-permission! group (mt/id :venues) :perms/create-queries create-perm)
+ (testing (str "view-data: " view-perm ", create-queries: " create-perm)
+ (is (= expected (:error (mt/user-http-request user-id :post 202 (str "card/" (u/the-id card) "/query"))
+ "Request Permitted")))))))))
+
(deftest sandbox-join-permissions-test
(testing "Sandboxed query can't be saved when sandboxed table is joined to a table that the current user doesn't have access to"
(mt/with-temp [:model/Collection collection {}]
diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj b/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj
index 0e173552d65..223b7695cc7 100644
--- a/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj
@@ -222,8 +222,7 @@
:display_name "Count"
:source :aggregation
:field_ref [:aggregation 0]}]
- ::query-perms/perms {:gtaps {:perms/view-data {(mt/id :checkins) :unrestricted
- (mt/id :venues) :unrestricted}
+ ::query-perms/perms {:gtaps {:perms/view-data {(mt/id :checkins) :unrestricted}
:perms/create-queries {(mt/id :checkins) :query-builder
(mt/id :venues) :query-builder}}}})
(apply-row-level-permissions
@@ -1170,3 +1169,34 @@
clojure.lang.ExceptionInfo
#"You do not have permissions to run this query"
(qp/process-query query))))))))
+
+(deftest sandbox-join-permissions-unrestricted-test
+ (testing "sandboxing with unrestricted data perms on the sandboxed table works"
+ (met/with-gtaps! (mt/$ids orders
+ {:gtaps {:orders {:remappings {"user_id" [:dimension $user_id->people.id]}}}
+ :attributes {"user_id" 1}})
+ (data-perms/set-table-permission! &group (mt/id :people) :perms/view-data :unrestricted)
+ (let [query (mt/mbql-query orders)]
+ (is (= 11 (count (mt/rows (qp/process-query query)))))))))
+
+(deftest sandbox-join-permissions-not-allowed-when-table-blocked-test
+ (testing "sandboxed query fails when sandboxed table is joined to a table that the current user is blocked on"
+ (met/with-gtaps! (mt/$ids orders
+ {:gtaps {:orders {:remappings {"user_id" [:dimension $user_id->people.id]}}}
+ :attributes {"user_id" 1}})
+ (data-perms/set-table-permission! &group (mt/id :people) :perms/view-data :blocked)
+ (let [query (mt/mbql-query orders)]
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"You do not have permissions to run this query"
+ (qp/process-query query)))))))
+
+(deftest sandbox-join-permissions-test-uses-nested-sandboxes-test
+ (testing "Nested sandbox query works when sandboxed definition is based on a fk to another sandboxed table"
+ (met/with-gtaps! (mt/$ids orders
+ {:attributes {"user_id" 1}
+ :gtaps {:orders {:remappings {"user_id" [:dimension $user_id->people.id]}}
+ ;; Since noone's zipcode == 1, this sandboxed table will return nothing
+ :people {:remappings {"user_id" [:dimension $people.zip]}}}})
+ (data-perms/set-table-permission! &group (mt/id :people) :perms/view-data :unrestricted)
+ (is (= 0 (count (mt/rows (qp/process-query (mt/mbql-query orders)))))))))
diff --git a/enterprise/frontend/src/metabase-enterprise/advanced_permissions/graph.ts b/enterprise/frontend/src/metabase-enterprise/advanced_permissions/graph.ts
index 4ab8025eb47..c6973836d33 100644
--- a/enterprise/frontend/src/metabase-enterprise/advanced_permissions/graph.ts
+++ b/enterprise/frontend/src/metabase-enterprise/advanced_permissions/graph.ts
@@ -1,16 +1,19 @@
import _ from "underscore";
-import type {
- DatabaseEntityId,
- EntityId,
-} from "metabase/admin/permissions/types";
+import type { EntityId } from "metabase/admin/permissions/types";
import {
DataPermission,
DataPermissionValue,
} from "metabase/admin/permissions/types";
import {
+ isTableEntityId,
+ isSchemaEntityId,
+} from "metabase/admin/permissions/utils/data-entity-id";
+import {
+ getEntityPermission,
getSchemasPermission,
- updateSchemasPermission,
+ hasPermissionValueInSubgraph,
+ updateEntityPermission,
} from "metabase/admin/permissions/utils/graph";
import type Database from "metabase-lib/v1/metadata/Database";
import type { GroupsPermissions, NativePermissions } from "metabase-types/api";
@@ -30,38 +33,72 @@ export function shouldRestrictNativeQueryPermissions(
DataPermission.CREATE_QUERIES,
);
- return (
- value === DataPermissionValue.SANDBOXED &&
- currDbNativePermission === DataPermissionValue.QUERY_BUILDER_AND_NATIVE
- );
+ if (isTableEntityId(entityId)) {
+ return (
+ (value === DataPermissionValue.SANDBOXED ||
+ value === DataPermissionValue.BLOCKED) &&
+ currDbNativePermission === DataPermissionValue.QUERY_BUILDER_AND_NATIVE
+ );
+ }
+
+ if (isSchemaEntityId(entityId)) {
+ return (
+ value === DataPermissionValue.BLOCKED &&
+ currDbNativePermission === DataPermissionValue.QUERY_BUILDER_AND_NATIVE
+ );
+ }
+
+ return false;
}
export function upgradeViewPermissionsIfNeeded(
permissions: GroupsPermissions,
groupId: number,
- entityId: DatabaseEntityId,
+ entityId: EntityId,
value: NativePermissions,
database: Database,
) {
- const dbPermission = getSchemasPermission(
+ // get permission for item up one level or db if we're already at the top most entity:
+ // table -> schema, schema -> database, database -> database
+ const parentOrDbEntityId = isTableEntityId(entityId)
+ ? _.pick(entityId, ["databaseId", "schemaName"])
+ : _.pick(entityId, ["databaseId"]);
+
+ const parentOrDbPermission = getEntityPermission(
permissions,
groupId,
- { databaseId: entityId.databaseId },
+ parentOrDbEntityId,
DataPermission.VIEW_DATA,
);
- if (
+ const isGrantingNativeQueryAccessWithoutProperViewAccess =
value === DataPermissionValue.QUERY_BUILDER_AND_NATIVE &&
- dbPermission !== DataPermissionValue.IMPERSONATED
- ) {
- permissions = updateSchemasPermission(
+ parentOrDbPermission !== DataPermissionValue.UNRESTRICTED &&
+ parentOrDbPermission !== DataPermissionValue.IMPERSONATED;
+
+ const isGrantingQueryAccessWithBlockedChild =
+ value !== DataPermissionValue.NO &&
+ !isTableEntityId(entityId) &&
+ hasPermissionValueInSubgraph(
permissions,
groupId,
entityId,
+ database,
+ DataPermission.VIEW_DATA,
+ DataPermissionValue.BLOCKED,
+ );
+
+ if (
+ isGrantingNativeQueryAccessWithoutProperViewAccess ||
+ isGrantingQueryAccessWithBlockedChild
+ ) {
+ permissions = updateEntityPermission(
+ permissions,
+ groupId,
+ parentOrDbEntityId,
DataPermissionValue.UNRESTRICTED,
database,
DataPermission.VIEW_DATA,
- false,
);
}
diff --git a/enterprise/frontend/src/metabase-enterprise/advanced_permissions/graph.unit.spec.ts b/enterprise/frontend/src/metabase-enterprise/advanced_permissions/graph.unit.spec.ts
index 3268241f5e8..46caf91b998 100644
--- a/enterprise/frontend/src/metabase-enterprise/advanced_permissions/graph.unit.spec.ts
+++ b/enterprise/frontend/src/metabase-enterprise/advanced_permissions/graph.unit.spec.ts
@@ -3,8 +3,9 @@ import {
DataPermissionValue,
} from "metabase/admin/permissions/types";
import Database from "metabase-lib/v1/metadata/Database";
+import Schema from "metabase-lib/v1/metadata/Schema";
import type { SchemasPermissions } from "metabase-types/api";
-import { createMockDatabase } from "metabase-types/api/mocks";
+import { createMockDatabase, createMockSchema } from "metabase-types/api/mocks";
import { upgradeViewPermissionsIfNeeded } from "./graph";
@@ -20,6 +21,16 @@ const database = new Database({
tables: [tableId],
});
+// mock out schemas as real Schema
+database.schemas = [
+ new Schema(
+ createMockSchema({
+ id: "100",
+ name: schema,
+ }),
+ ),
+];
+
const createGraph = (viewPermissions: SchemasPermissions) => ({
[groupId]: {
[databaseId]: {
diff --git a/enterprise/frontend/src/metabase-enterprise/advanced_permissions/index.js b/enterprise/frontend/src/metabase-enterprise/advanced_permissions/index.js
index d1dded518a5..07d5dd99f0c 100644
--- a/enterprise/frontend/src/metabase-enterprise/advanced_permissions/index.js
+++ b/enterprise/frontend/src/metabase-enterprise/advanced_permissions/index.js
@@ -11,6 +11,8 @@ import {
PLUGIN_REDUCERS,
PLUGIN_ADVANCED_PERMISSIONS,
PLUGIN_ADMIN_PERMISSIONS_DATABASE_ROUTES,
+ PLUGIN_ADMIN_PERMISSIONS_TABLE_OPTIONS,
+ PLUGIN_ADMIN_PERMISSIONS_TABLE_FIELDS_OPTIONS,
PLUGIN_ADMIN_PERMISSIONS_DATABASE_POST_ACTIONS,
PLUGIN_ADMIN_PERMISSIONS_DATABASE_GROUP_ROUTES,
PLUGIN_DATA_PERMISSIONS,
@@ -42,16 +44,16 @@ const BLOCK_PERMISSION_OPTION = {
if (hasPremiumFeature("advanced_permissions")) {
const addSelectedAdvancedPermission = (options, value) => {
- switch (value) {
- case BLOCK_PERMISSION_OPTION.value:
- return [...options, BLOCK_PERMISSION_OPTION];
- case IMPERSONATED_PERMISSION_OPTION.value:
- return [...options, IMPERSONATED_PERMISSION_OPTION];
+ if (value === IMPERSONATED_PERMISSION_OPTION.value) {
+ return [...options, IMPERSONATED_PERMISSION_OPTION];
}
return options;
};
+ PLUGIN_ADMIN_PERMISSIONS_TABLE_OPTIONS.push(BLOCK_PERMISSION_OPTION);
+ PLUGIN_ADMIN_PERMISSIONS_TABLE_FIELDS_OPTIONS.push(BLOCK_PERMISSION_OPTION);
+
PLUGIN_ADVANCED_PERMISSIONS.addTablePermissionOptions =
addSelectedAdvancedPermission;
PLUGIN_ADVANCED_PERMISSIONS.addSchemaPermissionOptions =
@@ -87,23 +89,18 @@ if (hasPremiumFeature("advanced_permissions")) {
value === BLOCK_PERMISSION_OPTION.value;
PLUGIN_ADVANCED_PERMISSIONS.getDatabaseLimitedAccessPermission = value => {
- if (
- value === BLOCK_PERMISSION_OPTION.value ||
- value === IMPERSONATED_PERMISSION_OPTION.value
- ) {
+ if (value === IMPERSONATED_PERMISSION_OPTION.value) {
return DataPermissionValue.UNRESTRICTED;
}
return null;
};
-
PLUGIN_ADVANCED_PERMISSIONS.isAccessPermissionDisabled = (value, subject) => {
- return (
- ["tables", "fields"].includes(subject) &&
- [DataPermissionValue.BLOCKED, DataPermissionValue.IMPERSONATED].includes(
- value,
- )
- );
+ if (subject === "tables" || subject === "fields") {
+ return value === DataPermissionValue.IMPERSONATED;
+ } else {
+ return false;
+ }
};
PLUGIN_ADVANCED_PERMISSIONS.isRestrictivePermission = value => {
diff --git a/enterprise/frontend/src/metabase-enterprise/sandboxes/components/EditSandboxingModal/EditSandboxingModal.tsx b/enterprise/frontend/src/metabase-enterprise/sandboxes/components/EditSandboxingModal/EditSandboxingModal.tsx
index d56feb6bdb2..5bb68b78cfe 100644
--- a/enterprise/frontend/src/metabase-enterprise/sandboxes/components/EditSandboxingModal/EditSandboxingModal.tsx
+++ b/enterprise/frontend/src/metabase-enterprise/sandboxes/components/EditSandboxingModal/EditSandboxingModal.tsx
@@ -130,16 +130,17 @@ const EditSandboxingModal = ({
return (
<div>
- <h2 className={CS.p3}>{t`Grant sandboxed access to this table`}</h2>
+ <h2 className={CS.p3}>{t`Restrict access to this table`}</h2>
<div>
<div className={cx(CS.px3, CS.pb3)}>
- <div className={CS.pb3}>
- {t`When users in this group view this table they'll see a version of it that's filtered by their user attributes, or a custom view of it based on a saved question.`}
+ <div className={CS.pb2}>
+ {t`When the following rules are applied, this group will see a customized version of the table.`}
</div>
- <h4 className={CS.pb1}>
- {t`How do you want to filter this table for users in this group?`}
- </h4>
+ <div className={CS.pb4}>
+ {t`These rules don’t apply to native queries.`}
+ </div>
+ <h4 className={CS.pb1}>{t`How do you want to filter this table?`}</h4>
<Radio
value={!shouldUseSavedQuestion}
options={[
diff --git a/enterprise/frontend/src/metabase-enterprise/sandboxes/components/EditSandboxingModal/EditSandboxingModal.unit.spec.tsx b/enterprise/frontend/src/metabase-enterprise/sandboxes/components/EditSandboxingModal/EditSandboxingModal.unit.spec.tsx
index 2e29f47fbcf..a27970ccafb 100644
--- a/enterprise/frontend/src/metabase-enterprise/sandboxes/components/EditSandboxingModal/EditSandboxingModal.unit.spec.tsx
+++ b/enterprise/frontend/src/metabase-enterprise/sandboxes/components/EditSandboxingModal/EditSandboxingModal.unit.spec.tsx
@@ -124,7 +124,7 @@ describe("EditSandboxingModal", () => {
const { onSave } = setup();
expect(
- screen.getByText("Grant sandboxed access to this table"),
+ screen.getByText("Restrict access to this table"),
).toBeInTheDocument();
expect(screen.getByRole("button", { name: "Save" })).toBeDisabled();
@@ -156,7 +156,7 @@ describe("EditSandboxingModal", () => {
const { onSave } = setup({ shouldMockQuestions: true });
expect(
- screen.getByText("Grant sandboxed access to this table"),
+ screen.getByText("Restrict access to this table"),
).toBeInTheDocument();
expect(screen.getByRole("button", { name: "Save" })).toBeDisabled();
@@ -206,7 +206,7 @@ describe("EditSandboxingModal", () => {
});
expect(
- screen.getByText("Grant sandboxed access to this table"),
+ screen.getByText("Restrict access to this table"),
).toBeInTheDocument();
expect(screen.getByRole("button", { name: "Save" })).toBeDisabled();
diff --git a/frontend/src/metabase-types/api/permissions.ts b/frontend/src/metabase-types/api/permissions.ts
index 9c3c256eae6..78f05eaac29 100644
--- a/frontend/src/metabase-types/api/permissions.ts
+++ b/frontend/src/metabase-types/api/permissions.ts
@@ -87,6 +87,7 @@ export type SchemasPermissions =
export type TablesPermissions =
| DataPermissionValue.UNRESTRICTED
| DataPermissionValue.LEGACY_NO_SELF_SERVICE
+ | DataPermissionValue.BLOCKED
| {
[key: TableId]: FieldsPermissions;
};
@@ -94,7 +95,8 @@ export type TablesPermissions =
export type FieldsPermissions =
| DataPermissionValue.UNRESTRICTED
| DataPermissionValue.LEGACY_NO_SELF_SERVICE
- | DataPermissionValue.SANDBOXED;
+ | DataPermissionValue.SANDBOXED
+ | DataPermissionValue.BLOCKED;
export type CollectionPermissionsGraph = {
groups: CollectionPermissions;
diff --git a/frontend/src/metabase/admin/permissions/components/DataPermissionsHelp/DataPermissionsHelp.tsx b/frontend/src/metabase/admin/permissions/components/DataPermissionsHelp/DataPermissionsHelp.tsx
index f0077205db7..1f913ef139d 100644
--- a/frontend/src/metabase/admin/permissions/components/DataPermissionsHelp/DataPermissionsHelp.tsx
+++ b/frontend/src/metabase/admin/permissions/components/DataPermissionsHelp/DataPermissionsHelp.tsx
@@ -16,6 +16,7 @@ import {
Text,
Title,
Icon,
+ List,
} from "metabase/ui";
import { hasPermissionValueInGraph } from "../../utils/graph/data-permissions";
@@ -120,7 +121,34 @@ export const DataPermissionsHelp = () => {
icon="permissions_limited"
iconColor="brand"
name={t`Sandboxed (Pro)`}
- description={t`Let's you specify row and column-level permissions. Can be set up via user attributes and SSO.`}
+ description={t`Lets you specify row and column-level permissions. Can be set up via user attributes and SSO.`}
+ />
+
+ <PermissionHelpDescription
+ hasUpgradeNotice={!isAdvancedPermissionsFeatureEnabled}
+ icon="close"
+ iconColor="danger"
+ name={t`Blocked (Pro)`}
+ description={
+ <>
+ <Text>{t`The group can’t view:`}</Text>
+ <List style={{ marginInlineEnd: "1rem" }}>
+ <List.Item>
+ <Text>{t`The schema/table when browsing data.`}</Text>
+ </List.Item>
+ <List.Item>
+ <Text>
+ {t`Query-builder questions using that schema/table.`}
+ </Text>
+ </List.Item>
+ <List.Item>
+ <Text>
+ {t`ANY native questions querying the database, regardless of schema/table.`}
+ </Text>
+ </List.Item>
+ </List>
+ </>
+ }
/>
</Stack>
</Accordion.Panel>
diff --git a/frontend/src/metabase/admin/permissions/components/PermissionHelpDescription/PermissionHelpDescription.tsx b/frontend/src/metabase/admin/permissions/components/PermissionHelpDescription/PermissionHelpDescription.tsx
index 4d5f2bc2dcf..ed8bb76b694 100644
--- a/frontend/src/metabase/admin/permissions/components/PermissionHelpDescription/PermissionHelpDescription.tsx
+++ b/frontend/src/metabase/admin/permissions/components/PermissionHelpDescription/PermissionHelpDescription.tsx
@@ -39,7 +39,12 @@ export const PermissionHelpDescription = ({
{name}
</Title>
</Flex>
- {description && <Text>{description}</Text>}
+ {description &&
+ (typeof description === "string" ? (
+ <Text>{description}</Text>
+ ) : (
+ description
+ ))}
{hasUpgradeNotice ? (
<>
diff --git a/frontend/src/metabase/admin/permissions/permissions.js b/frontend/src/metabase/admin/permissions/permissions.js
index 9b7f771368c..61ddf79e79e 100644
--- a/frontend/src/metabase/admin/permissions/permissions.js
+++ b/frontend/src/metabase/admin/permissions/permissions.js
@@ -9,7 +9,7 @@ import {
updateSchemasPermission,
updateTablesPermission,
updatePermission,
- restrictNativeQueryPermissionsIfNeeded,
+ restrictCreateQueriesPermissionsIfNeeded,
} from "metabase/admin/permissions/utils/graph";
import { getGroupFocusPermissionsUrl } from "metabase/admin/permissions/utils/urls";
import Group from "metabase/entities/groups";
@@ -219,12 +219,18 @@ export const saveDataPermissions = createThunkAction(
allGroupIds,
advancedPermissions.modifiedGroupIds,
);
+ const modifiedGroupIds = Object.keys(modifiedGroups);
- return await PermissionsApi.updateGraph({
+ const response = await PermissionsApi.updateGraph({
groups: modifiedGroups,
revision: dataPermissionsRevision,
...advancedPermissions.permissions,
});
+
+ return {
+ ...response,
+ modifiedGroupIds,
+ };
},
);
@@ -315,7 +321,11 @@ const dataPermissions = handleActions(
},
[SAVE_DATA_PERMISSIONS]: {
next: (state, { payload }) =>
- mergeGroupsPermissionsUpdates(state, payload.groups),
+ mergeGroupsPermissionsUpdates(
+ state,
+ payload.groups,
+ payload.modifiedGroupIds,
+ ),
},
[UPDATE_DATA_PERMISSION]: {
next: (state, { payload }) => {
@@ -352,7 +362,7 @@ const dataPermissions = handleActions(
);
}
- state = restrictNativeQueryPermissionsIfNeeded(
+ state = restrictCreateQueriesPermissionsIfNeeded(
state,
groupId,
entityId,
@@ -361,9 +371,6 @@ const dataPermissions = handleActions(
database,
);
- const shouldDowngradeNative =
- permissionInfo.type === DataPermissionType.ACCESS;
-
if (entityId.tableId != null) {
const updatedPermissions = updateFieldsPermission(
state,
@@ -372,7 +379,6 @@ const dataPermissions = handleActions(
value,
database,
permissionInfo.permission,
- shouldDowngradeNative,
);
return inferAndUpdateEntityPermissions(
updatedPermissions,
@@ -380,7 +386,6 @@ const dataPermissions = handleActions(
entityId,
database,
permissionInfo.permission,
- shouldDowngradeNative,
);
} else if (entityId.schemaName != null) {
return updateTablesPermission(
@@ -390,7 +395,6 @@ const dataPermissions = handleActions(
value,
database,
permissionInfo.permission,
- shouldDowngradeNative,
);
} else {
return updateSchemasPermission(
@@ -400,7 +404,6 @@ const dataPermissions = handleActions(
value,
database,
permissionInfo.permission,
- shouldDowngradeNative,
);
}
},
@@ -422,7 +425,11 @@ const originalDataPermissions = handleActions(
},
[SAVE_DATA_PERMISSIONS]: {
next: (state, { payload }) =>
- mergeGroupsPermissionsUpdates(state, payload.groups),
+ mergeGroupsPermissionsUpdates(
+ state,
+ payload.groups,
+ payload.modifiedGroupIds,
+ ),
},
},
null,
diff --git a/frontend/src/metabase/admin/permissions/selectors/confirmations.ts b/frontend/src/metabase/admin/permissions/selectors/confirmations.tsx
similarity index 68%
rename from frontend/src/metabase/admin/permissions/selectors/confirmations.ts
rename to frontend/src/metabase/admin/permissions/selectors/confirmations.tsx
index dba6c7dc528..1ec52da8bde 100644
--- a/frontend/src/metabase/admin/permissions/selectors/confirmations.ts
+++ b/frontend/src/metabase/admin/permissions/selectors/confirmations.tsx
@@ -8,8 +8,11 @@ import {
import {
getFieldsPermission,
getSchemasPermission,
+ hasPermissionValueInSubgraph,
} from "metabase/admin/permissions/utils/graph";
+import Alert from "metabase/core/components/Alert";
import { PLUGIN_ADVANCED_PERMISSIONS } from "metabase/plugins";
+import { Flex, Text } from "metabase/ui";
import type Database from "metabase-lib/v1/metadata/Database";
import type {
Group,
@@ -17,7 +20,7 @@ import type {
ConcreteTableId,
} from "metabase-types/api";
-import type { EntityId } from "../types";
+import type { DatabaseEntityId, EntityId, SchemaEntityId } from "../types";
import { DataPermission, DataPermissionValue } from "../types";
export const getDefaultGroupHasHigherAccessText = (defaultGroup: Group) =>
@@ -157,39 +160,88 @@ export function getWillRevokeNativeAccessWarningModal(
}
}
-export function getRawQueryWarningModal(
+export function getViewDataPermissionsTooRestrictiveWarningModal(
permissions: GroupsPermissions,
groupId: Group["id"],
- entityId: EntityId,
+ entityId: DatabaseEntityId | SchemaEntityId,
+ database: Database,
value: DataPermissionValue,
) {
- const nativePermission = getSchemasPermission(
- permissions,
- groupId,
- entityId,
- DataPermission.CREATE_QUERIES,
- );
+ // if user sets 'Query builder and native' for a DB, warn them that view data permissions must be 'Can view'
+ if (!isSchemaEntityId(entityId)) {
+ const nativePermission = getSchemasPermission(
+ permissions,
+ groupId,
+ entityId,
+ DataPermission.CREATE_QUERIES,
+ );
- const viewPermission = getSchemasPermission(
+ const viewPermission = getSchemasPermission(
+ permissions,
+ groupId,
+ entityId,
+ DataPermission.VIEW_DATA,
+ );
+
+ const isAddingNativeQueryPermissions =
+ value === DataPermissionValue.QUERY_BUILDER_AND_NATIVE &&
+ nativePermission !== DataPermissionValue.QUERY_BUILDER_AND_NATIVE;
+
+ const canNotViewNativeQueryResults =
+ viewPermission !== DataPermissionValue.UNRESTRICTED &&
+ viewPermission !== DataPermissionValue.IMPERSONATED;
+
+ if (
+ isAddingNativeQueryPermissions &&
+ canNotViewNativeQueryResults &&
+ PLUGIN_ADVANCED_PERMISSIONS.shouldShowViewDataColumn
+ ) {
+ return {
+ title: t`Allow native query editing?`,
+ message: t`This will also change this group's data access to “Can view†for this database.`,
+ confirmButtonText: t`Allow`,
+ cancelButtonText: t`Cancel`,
+ };
+ }
+ }
+
+ // if user sets 'No' for a DB/Schema and a sub schema/tables contains 'Blocked' permissions, warn them
+ // that we'll automatically upgrade the DB/Schema to 'Can view' view access
+ const hasCreateQueryAccess = value !== DataPermissionValue.NO;
+ if (!hasCreateQueryAccess) {
+ return;
+ }
+
+ const hasChildWithBlockedPermission = hasPermissionValueInSubgraph(
permissions,
groupId,
entityId,
+ database,
DataPermission.VIEW_DATA,
+ DataPermissionValue.BLOCKED,
);
- if (
- value === DataPermissionValue.QUERY_BUILDER_AND_NATIVE &&
- nativePermission !== DataPermissionValue.QUERY_BUILDER_AND_NATIVE &&
- PLUGIN_ADVANCED_PERMISSIONS.shouldShowViewDataColumn &&
- ![
- DataPermissionValue.UNRESTRICTED,
- DataPermissionValue.IMPERSONATED,
- ].includes(viewPermission)
- ) {
+ if (hasChildWithBlockedPermission) {
+ const isSchema = isSchemaEntityId(entityId);
+ const entityType = isSchema ? t`schema` : t`database`;
+
+ const coreMessage = isSchema
+ ? t`This schema contains one or more tables with “Blocked†permissions, which prevents access to the query builder. To grant Create query permissions for this schema, Metabase will also change the View data permissions on this schema to “Can viewâ€.`
+ : t`This database contains one or more schemas and tables with “Blocked†permissions, which prevents access to the query builder. To grant Create query permissions for this database, Metabase will also change the View data permissions on this database to “Can viewâ€.`;
+
+ const resetGranularSettingsWarnging = t`Updating access will reset your granular settings for this ${entityType}. To keep those settings, you’ll need to manually change the View data permissions for the schemas or tables that are set to “Blockedâ€.`;
+
return {
- title: t`Allow native query editing?`,
- message: t`This will also change this group's data access to Unrestricted for this database.`,
- confirmButtonText: t`Allow`,
+ title: t`This will also set the View Data permission to “Can View†to allow this group to create queries. Okay?`,
+ message: (
+ <Flex direction="column" gap="lg">
+ <Text>{coreMessage}</Text>
+ <Alert variant="warning" icon="warning">
+ {resetGranularSettingsWarnging}
+ </Alert>
+ </Flex>
+ ),
+ confirmButtonText: t`Okay`,
cancelButtonText: t`Cancel`,
};
}
diff --git a/frontend/src/metabase/admin/permissions/selectors/data-permissions/fields.ts b/frontend/src/metabase/admin/permissions/selectors/data-permissions/fields.ts
index 56c1a7b658b..6cd4caeaedb 100644
--- a/frontend/src/metabase/admin/permissions/selectors/data-permissions/fields.ts
+++ b/frontend/src/metabase/admin/permissions/selectors/data-permissions/fields.ts
@@ -75,6 +75,7 @@ const buildAccessPermission = (
"fields",
defaultGroup,
groupId,
+ undefined,
),
...PLUGIN_ADMIN_PERMISSIONS_TABLE_FIELDS_CONFIRMATIONS.map(confirmation =>
confirmation(permissions, groupId, entityId, newValue),
@@ -99,12 +100,8 @@ const buildAccessPermission = (
);
const isDisabled =
isAdmin ||
- (!isAdmin &&
- (options.length <= 1 ||
- PLUGIN_ADVANCED_PERMISSIONS.isAccessPermissionDisabled(
- value,
- "fields",
- )));
+ options.length <= 1 ||
+ PLUGIN_ADVANCED_PERMISSIONS.isAccessPermissionDisabled(value, "fields");
return {
permission: DataPermission.VIEW_DATA,
diff --git a/frontend/src/metabase/admin/permissions/selectors/data-permissions/permission-editor.ts b/frontend/src/metabase/admin/permissions/selectors/data-permissions/permission-editor.ts
index 8edd46e892d..a6f23af6bf8 100644
--- a/frontend/src/metabase/admin/permissions/selectors/data-permissions/permission-editor.ts
+++ b/frontend/src/metabase/admin/permissions/selectors/data-permissions/permission-editor.ts
@@ -223,7 +223,7 @@ export const getDatabasesPermissionEditor = createSelector(
),
};
});
- } else if (databaseId != null) {
+ } else if (database && databaseId != null) {
const maybeDbEntities = metadata
?.database(databaseId)
?.getSchemas()
@@ -242,6 +242,7 @@ export const getDatabasesPermissionEditor = createSelector(
permissions,
originalPermissions,
defaultGroup,
+ database,
),
};
});
@@ -401,6 +402,7 @@ export const getGroupsDataPermissionEditor: GetGroupsDataPermissionEditorSelecto
permissions,
originalPermissions,
defaultGroup,
+ database,
);
} else if (databaseId != null) {
groupPermissions = buildSchemasPermissions(
diff --git a/frontend/src/metabase/admin/permissions/selectors/data-permissions/schemas.ts b/frontend/src/metabase/admin/permissions/selectors/data-permissions/schemas.ts
index 25b72bac7c2..2974d16ae68 100644
--- a/frontend/src/metabase/admin/permissions/selectors/data-permissions/schemas.ts
+++ b/frontend/src/metabase/admin/permissions/selectors/data-permissions/schemas.ts
@@ -26,7 +26,7 @@ import { DataPermission, DataPermissionType } from "../../types";
import {
getPermissionWarning,
getPermissionWarningModal,
- getRawQueryWarningModal,
+ getViewDataPermissionsTooRestrictiveWarningModal,
} from "../confirmations";
const buildAccessPermission = (
@@ -119,6 +119,7 @@ const buildNativePermission = (
isAdmin: boolean,
permissions: GroupsPermissions,
defaultGroup: Group,
+ database: Database,
accessPermissionValue: DataPermissionValue,
): PermissionSectionConfig => {
const value = getSchemasPermission(
@@ -158,7 +159,13 @@ const buildNativePermission = (
defaultGroup,
groupId,
),
- getRawQueryWarningModal(permissions, groupId, entityId, newValue),
+ getViewDataPermissionsTooRestrictiveWarningModal(
+ permissions,
+ groupId,
+ entityId,
+ database,
+ newValue,
+ ),
];
return {
@@ -207,6 +214,7 @@ export const buildSchemasPermissions = (
isAdmin,
permissions,
defaultGroup,
+ database,
accessPermission.value,
);
diff --git a/frontend/src/metabase/admin/permissions/selectors/data-permissions/tables.ts b/frontend/src/metabase/admin/permissions/selectors/data-permissions/tables.ts
index bac0f660197..b5ea9ec1c01 100644
--- a/frontend/src/metabase/admin/permissions/selectors/data-permissions/tables.ts
+++ b/frontend/src/metabase/admin/permissions/selectors/data-permissions/tables.ts
@@ -6,9 +6,11 @@ import {
getTablesPermission,
} from "metabase/admin/permissions/utils/graph";
import {
+ PLUGIN_ADMIN_PERMISSIONS_TABLE_OPTIONS,
PLUGIN_ADVANCED_PERMISSIONS,
PLUGIN_FEATURE_LEVEL_PERMISSIONS,
} from "metabase/plugins";
+import type Database from "metabase-lib/v1/metadata/Database";
import type { Group, GroupsPermissions } from "metabase-types/api";
import { DATA_PERMISSION_OPTIONS } from "../../constants/data-permissions";
@@ -23,6 +25,7 @@ import {
import {
getPermissionWarning,
getPermissionWarningModal,
+ getViewDataPermissionsTooRestrictiveWarningModal,
getWillRevokeNativeAccessWarningModal,
} from "../confirmations";
@@ -79,21 +82,20 @@ const buildAccessPermission = (
DATA_PERMISSION_OPTIONS.controlled,
originalValue === DATA_PERMISSION_OPTIONS.noSelfServiceDeprecated.value &&
DATA_PERMISSION_OPTIONS.noSelfServiceDeprecated,
+ ...PLUGIN_ADMIN_PERMISSIONS_TABLE_OPTIONS,
]),
value,
);
+ const isDisabled =
+ isAdmin ||
+ options.length <= 1 ||
+ PLUGIN_ADVANCED_PERMISSIONS.isAccessPermissionDisabled(value, "tables");
+
return {
permission: DataPermission.VIEW_DATA,
type: DataPermissionType.ACCESS,
- isDisabled:
- isAdmin ||
- (!isAdmin &&
- (options.length <= 1 ||
- PLUGIN_ADVANCED_PERMISSIONS.isAccessPermissionDisabled(
- value,
- "tables",
- ))),
+ isDisabled,
isHighlighted: isAdmin,
disabledTooltip: isAdmin ? UNABLE_TO_CHANGE_ADMIN_PERMISSIONS : null,
value,
@@ -112,6 +114,7 @@ const buildNativePermission = (
isAdmin: boolean,
permissions: GroupsPermissions,
accessPermissionValue: DataPermissionValue,
+ database: Database,
): PermissionSectionConfig => {
const dbValue = getSchemasPermission(
permissions,
@@ -149,8 +152,15 @@ const buildNativePermission = (
postActions: {
controlled: () => navigateToGranularPermissions(groupId, entityId),
},
- confirmations: () => [
+ confirmations: (newValue: DataPermissionValue) => [
getWillRevokeNativeAccessWarningModal(permissions, groupId, entityId),
+ getViewDataPermissionsTooRestrictiveWarningModal(
+ permissions,
+ groupId,
+ entityId,
+ database,
+ newValue,
+ ),
],
};
};
@@ -162,6 +172,7 @@ export const buildTablesPermissions = (
permissions: GroupsPermissions,
originalPermissions: GroupsPermissions,
defaultGroup: Group,
+ database: Database,
): PermissionSectionConfig[] => {
const accessPermission = buildAccessPermission(
entityId,
@@ -178,6 +189,7 @@ export const buildTablesPermissions = (
isAdmin,
permissions,
accessPermission.value,
+ database,
);
const hasAnyAccessOptions = accessPermission.options.length > 1;
diff --git a/frontend/src/metabase/admin/permissions/types.ts b/frontend/src/metabase/admin/permissions/types.ts
index 2461c97d730..2d5e3bb7dfd 100644
--- a/frontend/src/metabase/admin/permissions/types.ts
+++ b/frontend/src/metabase/admin/permissions/types.ts
@@ -1,3 +1,5 @@
+import type { ReactNode } from "react";
+
export type GroupRouteParams = {
groupId?: number;
databaseId?: number;
@@ -112,7 +114,7 @@ export type PermissionSectionConfig = {
confirmations?: (newValue: DataPermissionValue) => (
| {
title: string;
- message: string;
+ message: string | ReactNode;
confirmButtonText: string;
cancelButtonText: string;
}
diff --git a/frontend/src/metabase/admin/permissions/utils/data-entity-id.ts b/frontend/src/metabase/admin/permissions/utils/data-entity-id.ts
index 63b018f7e45..5358b03b6bf 100644
--- a/frontend/src/metabase/admin/permissions/utils/data-entity-id.ts
+++ b/frontend/src/metabase/admin/permissions/utils/data-entity-id.ts
@@ -4,7 +4,12 @@ import type Schema from "metabase-lib/v1/metadata/Schema";
import type Table from "metabase-lib/v1/metadata/Table";
import type { ConcreteTableId } from "metabase-types/api";
-import type { EntityId } from "../types";
+import type {
+ DatabaseEntityId,
+ EntityId,
+ SchemaEntityId,
+ TableEntityId,
+} from "../types";
export const getDatabaseEntityId = (databaseEntity: Database) => ({
databaseId: databaseEntity.id,
@@ -21,11 +26,16 @@ export const getTableEntityId = (tableEntity: Table) => ({
tableId: tableEntity.id as ConcreteTableId,
});
-export const isTableEntityId = (entityId: Partial<EntityId>) =>
- entityId.tableId != null;
-export const isSchemaEntityId = (entityId: Partial<EntityId>) =>
+export const isTableEntityId = (
+ entityId: Partial<EntityId>,
+): entityId is TableEntityId => entityId.tableId != null;
+export const isSchemaEntityId = (
+ entityId: Partial<EntityId>,
+): entityId is SchemaEntityId =>
entityId.schemaName != null && !isTableEntityId(entityId);
-export const isDatabaseEntityId = (entityId: Partial<EntityId>) =>
+export const isDatabaseEntityId = (
+ entityId: Partial<EntityId>,
+): entityId is DatabaseEntityId =>
entityId.databaseId != null &&
!isSchemaEntityId(entityId) &&
!isTableEntityId(entityId);
diff --git a/frontend/src/metabase/admin/permissions/utils/graph/data-permissions.ts b/frontend/src/metabase/admin/permissions/utils/graph/data-permissions.ts
index 3fb62734c37..ff941cc0fd1 100644
--- a/frontend/src/metabase/admin/permissions/utils/graph/data-permissions.ts
+++ b/frontend/src/metabase/admin/permissions/utils/graph/data-permissions.ts
@@ -22,6 +22,7 @@ import type {
TableEntityId,
} from "../../types";
import { DataPermission, DataPermissionValue } from "../../types";
+import { isSchemaEntityId, isTableEntityId } from "../data-entity-id";
export const isRestrictivePermission = (value: DataPermissionValue) =>
value === DataPermissionValue.NO ||
@@ -221,7 +222,7 @@ export const getFieldsPermission = (
}
};
-const getEntityPermission = (
+export const getEntityPermission = (
permissions: GroupsPermissions,
groupId: number,
entityId: EntityId,
@@ -246,14 +247,78 @@ const getEntityPermission = (
}
};
+// subtypes to make testing easier and avoid using deprecated Database / Schema types
+type SchemaPartial = {
+ name: string;
+ getTables: () => { id: number | string }[];
+};
+type DatabasePartial = {
+ schemas?: SchemaPartial[];
+ schema(schemaName: string | undefined): SchemaPartial | null | undefined;
+};
+
+export function hasPermissionValueInSubgraph(
+ permissions: GroupsPermissions,
+ groupId: number,
+ entityId: DatabaseEntityId | SchemaEntityId,
+ database: DatabasePartial,
+ permission: DataPermission,
+ value: DataPermissionValue,
+) {
+ const schemasToSearch = _.compact(
+ isSchemaEntityId(entityId)
+ ? [database.schema(entityId.schemaName)]
+ : database.schemas,
+ );
+
+ if (schemasToSearch) {
+ const hasSchemaWithMatchingPermission = schemasToSearch.some(schema => {
+ const currVal = getTablesPermission(
+ permissions,
+ groupId,
+ { databaseId: entityId.databaseId, schemaName: schema.name },
+ permission,
+ );
+ return value === currVal;
+ });
+
+ if (hasSchemaWithMatchingPermission) {
+ return true;
+ }
+ }
+
+ return schemasToSearch.some(schema => {
+ return schema.getTables().some(table => {
+ return (
+ value ===
+ getFieldsPermission(
+ permissions,
+ groupId,
+ {
+ databaseId: entityId.databaseId,
+ schemaName: schema.name,
+ tableId: table.id as ConcreteTableId,
+ },
+ permission,
+ )
+ );
+ });
+ });
+}
+
// return boolean if able to find if a value is present in all or a portion of the permissions graph
+// NOTE: default values are omitted from the graph, and given the way this function was written, it won't return
+// the right answer for those permissions. for now, those default values have been omitted from allowed values to avoid bugs
export function hasPermissionValueInGraph(
permissions:
| GroupsPermissions
| GroupPermissions
| DatabasePermissions
| DataPermissionValue,
- permissionValue: DataPermissionValue,
+ permissionValue: Omit<
+ DataPermissionValue,
+ DataPermissionValue.BLOCKED | DataPermissionValue.NO // omit default values
+ >,
): boolean {
if (permissions === permissionValue) {
return true;
@@ -313,10 +378,7 @@ export function hasPermissionValueInEntityGraphs(
});
}
-// Ideally this would live in downgradeNativePermissionsIfNeeded, but originally that function was
-// created to only be called if a view permission was changing. there needs to be some reworking
-// in some of the setter methods to make sure the downgrading will always happen at the appropriate time
-export function restrictNativeQueryPermissionsIfNeeded(
+export function restrictCreateQueriesPermissionsIfNeeded(
permissions: GroupsPermissions,
groupId: number,
entityId: EntityId,
@@ -324,7 +386,7 @@ export function restrictNativeQueryPermissionsIfNeeded(
value: DataPermissionValue,
database: Database,
) {
- const currDbNativePermission = getSchemasPermission(
+ const currDbCreateQueriesPermission = getSchemasPermission(
permissions,
groupId,
{ databaseId: entityId.databaseId },
@@ -335,10 +397,10 @@ export function restrictNativeQueryPermissionsIfNeeded(
permission === DataPermission.CREATE_QUERIES &&
value !== DataPermissionValue.QUERY_BUILDER_AND_NATIVE &&
(entityId.tableId != null || entityId.schemaName != null) &&
- currDbNativePermission === DataPermissionValue.QUERY_BUILDER_AND_NATIVE;
+ currDbCreateQueriesPermission ===
+ DataPermissionValue.QUERY_BUILDER_AND_NATIVE;
- const shouldDowngradeNative =
- isMakingGranularCreateQueriesChange ||
+ const shouldRestrictForSomeReason =
PLUGIN_DATA_PERMISSIONS.shouldRestrictNativeQueryPermissions(
permissions,
groupId,
@@ -348,7 +410,10 @@ export function restrictNativeQueryPermissionsIfNeeded(
database,
);
- if (shouldDowngradeNative) {
+ const shouldRestrictNative =
+ isMakingGranularCreateQueriesChange || shouldRestrictForSomeReason;
+
+ if (shouldRestrictNative) {
const schemaNames = (database && database.schemaNames()) ?? [null];
schemaNames.forEach(schemaName => {
@@ -366,31 +431,21 @@ export function restrictNativeQueryPermissionsIfNeeded(
});
}
- return permissions;
-}
-
-export function downgradeNativePermissionsIfNeeded(
- permissions: GroupsPermissions,
- groupId: number,
- { databaseId }: DatabaseEntityId,
- value: DataPermissionValue,
-) {
- // remove query creation permissions if view permission is getting restricted
if (
isRestrictivePermission(value) ||
value === DataPermissionValue.LEGACY_NO_SELF_SERVICE
) {
- return updatePermission(
+ permissions = updateEntityPermission(
permissions,
groupId,
- databaseId,
- DataPermission.CREATE_QUERIES,
- [],
+ entityId,
DataPermissionValue.NO,
+ database,
+ DataPermission.CREATE_QUERIES,
);
- } else {
- return permissions;
}
+
+ return permissions;
}
const metadataTableToTableEntityId = (table: Table) => ({
@@ -444,7 +499,6 @@ export function inferAndUpdateEntityPermissions(
entityId: EntityId,
database: Database,
permission: DataPermission,
- downgradeNative?: boolean,
) {
const { databaseId } = entityId;
const schemaName = (entityId as SchemaEntityId).schemaName ?? "";
@@ -465,7 +519,6 @@ export function inferAndUpdateEntityPermissions(
tablesPermissionValue,
database,
permission,
- downgradeNative,
);
}
@@ -485,17 +538,7 @@ export function inferAndUpdateEntityPermissions(
schemasPermissionValue,
database,
permission,
- downgradeNative,
);
-
- if (downgradeNative) {
- permissions = downgradeNativePermissionsIfNeeded(
- permissions,
- groupId,
- { databaseId },
- schemasPermissionValue,
- );
- }
}
return permissions;
@@ -508,7 +551,6 @@ export function updateFieldsPermission(
value: any,
database: Database,
permission: DataPermission,
- downgradeNative?: boolean,
) {
const { databaseId, tableId } = entityId;
const schemaName = entityId.schemaName || "";
@@ -520,7 +562,6 @@ export function updateFieldsPermission(
DataPermissionValue.CONTROLLED,
database,
permission,
- downgradeNative,
);
permissions = updatePermission(
permissions,
@@ -530,7 +571,6 @@ export function updateFieldsPermission(
[schemaName, tableId],
value,
);
-
return permissions;
}
@@ -541,7 +581,6 @@ export function updateTablesPermission(
value: any,
database: Database,
permission: DataPermission,
- downgradeNative?: boolean,
) {
const schema = database.schema(schemaName);
const tableIds = schema?.getTables().map((t: Table) => t.id);
@@ -553,7 +592,6 @@ export function updateTablesPermission(
DataPermissionValue.CONTROLLED,
database,
permission,
- downgradeNative,
);
permissions = updatePermission(
permissions,
@@ -575,7 +613,6 @@ export function updateSchemasPermission(
value: DataPermissionValue,
database: Database,
permission: DataPermission,
- downgradeNative?: boolean,
) {
const schemaNames = database && database.schemaNames();
const schemaNamesOrNoSchema =
@@ -585,15 +622,6 @@ export function updateSchemasPermission(
? schemaNames
: [""];
- if (downgradeNative) {
- permissions = downgradeNativePermissionsIfNeeded(
- permissions,
- groupId,
- { databaseId },
- value,
- );
- }
-
return updatePermission(
permissions,
groupId,
@@ -604,3 +632,41 @@ export function updateSchemasPermission(
schemaNamesOrNoSchema,
);
}
+
+export function updateEntityPermission(
+ permissions: GroupsPermissions,
+ groupId: number,
+ entityId: EntityId,
+ value: DataPermissionValue,
+ database: Database,
+ permission: DataPermission,
+) {
+ if (isTableEntityId(entityId)) {
+ return updateFieldsPermission(
+ permissions,
+ groupId,
+ entityId,
+ value,
+ database,
+ permission,
+ );
+ } else if (isSchemaEntityId(entityId)) {
+ return updateTablesPermission(
+ permissions,
+ groupId,
+ entityId,
+ value,
+ database,
+ permission,
+ );
+ } else {
+ return updateSchemasPermission(
+ permissions,
+ groupId,
+ entityId,
+ value,
+ database,
+ permission,
+ );
+ }
+}
diff --git a/frontend/src/metabase/admin/permissions/utils/graph/data-permissions.unit.spec.ts b/frontend/src/metabase/admin/permissions/utils/graph/data-permissions.unit.spec.ts
new file mode 100644
index 00000000000..697bb651ab3
--- /dev/null
+++ b/frontend/src/metabase/admin/permissions/utils/graph/data-permissions.unit.spec.ts
@@ -0,0 +1,216 @@
+import _ from "underscore";
+
+import { PLUGIN_ADVANCED_PERMISSIONS } from "metabase/plugins";
+import type { GroupsPermissions } from "metabase-types/api";
+
+import { DataPermission, DataPermissionValue } from "../../types";
+
+import { hasPermissionValueInSubgraph } from "./data-permissions";
+
+describe("data permissions", () => {
+ describe("hasPermissionValueInSubgraph", () => {
+ it("should handle database entity ids", async () => {
+ const schemas = [{ name: "", getTables: () => [{ id: 1 }, { id: 2 }] }];
+ const database = {
+ schemas,
+ schema: (name: string) => schemas.find(schema => schema.name === name),
+ };
+
+ const testPermissions: GroupsPermissions = {
+ "1": {
+ "1": {
+ [DataPermission.VIEW_DATA]: DataPermissionValue.UNRESTRICTED,
+ [DataPermission.CREATE_QUERIES]: DataPermissionValue.QUERY_BUILDER,
+ },
+ "2": {
+ [DataPermission.VIEW_DATA]: DataPermissionValue.BLOCKED,
+ [DataPermission.CREATE_QUERIES]:
+ DataPermissionValue.QUERY_BUILDER_AND_NATIVE,
+ },
+ },
+ };
+
+ const testFn1 = _.partial(
+ hasPermissionValueInSubgraph,
+ testPermissions,
+ 1,
+ { databaseId: 1 },
+ database,
+ );
+
+ expect(
+ testFn1(DataPermission.VIEW_DATA, DataPermissionValue.UNRESTRICTED),
+ ).toBe(true);
+ expect(
+ testFn1(
+ DataPermission.CREATE_QUERIES,
+ DataPermissionValue.QUERY_BUILDER,
+ ),
+ ).toBe(true);
+
+ const testFn2 = _.partial(
+ hasPermissionValueInSubgraph,
+ testPermissions,
+ 1,
+ { databaseId: 2 },
+ database,
+ );
+
+ expect(
+ testFn2(DataPermission.VIEW_DATA, DataPermissionValue.UNRESTRICTED),
+ ).toBe(false);
+
+ expect(
+ testFn2(
+ DataPermission.CREATE_QUERIES,
+ DataPermissionValue.QUERY_BUILDER,
+ ),
+ ).toBe(false);
+ });
+
+ it("should handle databases with multiple schemas", async () => {
+ const schemas = [
+ { name: "public", getTables: () => [{ id: 1 }] },
+ { name: "public2", getTables: () => [{ id: 2 }] },
+ ];
+ const database = {
+ schemas,
+ schema: (name: string) => schemas.find(schema => schema.name === name),
+ };
+
+ const testPermissions: GroupsPermissions = {
+ "1": {
+ "1": {
+ [DataPermission.VIEW_DATA]: {
+ public: DataPermissionValue.UNRESTRICTED,
+ public2: DataPermissionValue.LEGACY_NO_SELF_SERVICE,
+ },
+ },
+ },
+ };
+
+ const testFn = _.partial(
+ hasPermissionValueInSubgraph,
+ testPermissions,
+ 1,
+ { databaseId: 1 },
+ database,
+ DataPermission.VIEW_DATA,
+ );
+
+ expect(testFn(DataPermissionValue.UNRESTRICTED)).toBe(true);
+ expect(testFn(DataPermissionValue.LEGACY_NO_SELF_SERVICE)).toBe(true);
+ expect(testFn(DataPermissionValue.BLOCKED)).toBe(false);
+ });
+
+ it("should handle schema entity ids", async () => {
+ const schemas = [
+ { name: "public", getTables: () => [{ id: 1 }] },
+ { name: "public2", getTables: () => [{ id: 2 }] },
+ ];
+ const database = {
+ schemas,
+ schema: (name: string) => schemas.find(schema => schema.name === name),
+ };
+
+ const testPermissions: GroupsPermissions = {
+ "1": {
+ "1": {
+ [DataPermission.VIEW_DATA]: {
+ public: DataPermissionValue.UNRESTRICTED,
+ public2: DataPermissionValue.BLOCKED,
+ },
+ },
+ },
+ };
+
+ expect(
+ hasPermissionValueInSubgraph(
+ testPermissions,
+ 1,
+ { databaseId: 1, schemaName: "public" },
+ database,
+ DataPermission.VIEW_DATA,
+ DataPermissionValue.UNRESTRICTED,
+ ),
+ ).toBe(true);
+
+ expect(
+ hasPermissionValueInSubgraph(
+ testPermissions,
+ 1,
+ { databaseId: 1, schemaName: "public2" },
+ database,
+ DataPermission.VIEW_DATA,
+ DataPermissionValue.BLOCKED,
+ ),
+ ).toBe(true);
+
+ expect(
+ hasPermissionValueInSubgraph(
+ testPermissions,
+ 1,
+ { databaseId: 1, schemaName: "public" },
+ database,
+ DataPermission.VIEW_DATA,
+ DataPermissionValue.BLOCKED,
+ ),
+ ).toBe(false);
+
+ expect(
+ hasPermissionValueInSubgraph(
+ testPermissions,
+ 1,
+ { databaseId: 1, schemaName: "public2" },
+ database,
+ DataPermission.VIEW_DATA,
+ DataPermissionValue.UNRESTRICTED,
+ ),
+ ).toBe(false);
+ });
+
+ it("should handle default permissions omitted from the graph", async () => {
+ const schemas = [
+ { name: "public", getTables: () => [{ id: 1 }] },
+ { name: "public2", getTables: () => [{ id: 2 }] },
+ ];
+ const database = {
+ schemas,
+ schema: (name: string) => schemas.find(schema => schema.name === name),
+ };
+
+ const testPermissions: GroupsPermissions = {
+ "1": {
+ "1": {
+ [DataPermission.VIEW_DATA]: {
+ public: DataPermissionValue.UNRESTRICTED,
+ // public2 omitted from graph to indicate blocked
+ },
+ },
+ },
+ };
+
+ expect(
+ hasPermissionValueInSubgraph(
+ testPermissions,
+ 1,
+ { databaseId: 1, schemaName: "public2" },
+ database,
+ DataPermission.VIEW_DATA,
+ PLUGIN_ADVANCED_PERMISSIONS.defaultViewDataPermission,
+ ),
+ ).toBe(true);
+
+ expect(
+ hasPermissionValueInSubgraph(
+ testPermissions,
+ 1,
+ { databaseId: 1, schemaName: "public2" },
+ database,
+ DataPermission.CREATE_QUERIES,
+ DataPermissionValue.NO,
+ ),
+ ).toBe(true);
+ });
+ });
+});
diff --git a/frontend/src/metabase/admin/permissions/utils/graph/partial-updates.ts b/frontend/src/metabase/admin/permissions/utils/graph/partial-updates.ts
index 2a45b7a712b..b136edfe784 100644
--- a/frontend/src/metabase/admin/permissions/utils/graph/partial-updates.ts
+++ b/frontend/src/metabase/admin/permissions/utils/graph/partial-updates.ts
@@ -34,19 +34,27 @@ export function getModifiedGroupsPermissionsGraphParts(
export function mergeGroupsPermissionsUpdates(
originalDataPermissions: GroupsPermissions | null | undefined,
newDataPermissions: GroupsPermissions,
+ modifiedGroupIds: string[],
) {
if (!originalDataPermissions) {
return newDataPermissions;
}
+ const modifiedGroupIdsSet = new Set(modifiedGroupIds);
+
const allGroupIds = _.uniq([
...Object.keys(originalDataPermissions),
...Object.keys(newDataPermissions),
]);
const latestPermissionsEntries = allGroupIds.map(groupId => {
- const permissions =
- newDataPermissions[groupId] ?? originalDataPermissions[groupId];
+ // values can be omitted from the graph to save space or to indicate that the group has default permissions for all entities
+ // this means we need to determine the value if we need to use the value currently in memory or default to an empty object
+ // which is the FE definition of completely default permissions for all entities
+ const defaultValue = modifiedGroupIdsSet.has(groupId)
+ ? {}
+ : originalDataPermissions[groupId];
+ const permissions = newDataPermissions[groupId] ?? defaultValue;
return [groupId, permissions];
});
diff --git a/frontend/src/metabase/admin/permissions/utils/graph/partial-updates.unit.spec.ts b/frontend/src/metabase/admin/permissions/utils/graph/partial-updates.unit.spec.ts
index 2d54b16ee09..c1bc7b4c3e7 100644
--- a/frontend/src/metabase/admin/permissions/utils/graph/partial-updates.unit.spec.ts
+++ b/frontend/src/metabase/admin/permissions/utils/graph/partial-updates.unit.spec.ts
@@ -77,8 +77,11 @@ describe("mergeGroupsPermissionsUpdates", () => {
"1": { [DataPermission.VIEW_DATA]: DataPermissionValue.UNRESTRICTED },
},
};
+ const modifiedGroupIds = Object.keys(update);
- expect(mergeGroupsPermissionsUpdates(undefined, update)).toBe(update);
+ expect(
+ mergeGroupsPermissionsUpdates(undefined, update, modifiedGroupIds),
+ ).toBe(update);
});
it("should only apply updates to groups that have been modified", async () => {
@@ -95,6 +98,8 @@ describe("mergeGroupsPermissionsUpdates", () => {
"2": { "1": { [DataPermission.VIEW_DATA]: DataPermissionValue.NO } },
};
+ const modifiedGroupIds = Object.keys(update);
+
const expectedResult = {
"1": {
"1": { [DataPermission.VIEW_DATA]: DataPermissionValue.UNRESTRICTED },
@@ -102,9 +107,35 @@ describe("mergeGroupsPermissionsUpdates", () => {
"2": { "1": { [DataPermission.VIEW_DATA]: DataPermissionValue.NO } },
};
- expect(mergeGroupsPermissionsUpdates(permissions, update)).toEqual(
- expectedResult,
- );
+ expect(
+ mergeGroupsPermissionsUpdates(permissions, update, modifiedGroupIds),
+ ).toEqual(expectedResult);
+ });
+
+ it("should return empty objects for modified groups that have empty values in the update object (this means they have default values for all permissions)", async () => {
+ const permissions: GroupsPermissions = {
+ "1": {
+ "1": { [DataPermission.VIEW_DATA]: DataPermissionValue.UNRESTRICTED },
+ },
+ "2": {
+ "1": { [DataPermission.VIEW_DATA]: DataPermissionValue.UNRESTRICTED },
+ },
+ };
+
+ const update: GroupsPermissions = {};
+
+ const modifiedGroupIds = ["2"];
+
+ const expectedResult = {
+ "1": {
+ "1": { [DataPermission.VIEW_DATA]: DataPermissionValue.UNRESTRICTED },
+ },
+ "2": {},
+ };
+
+ expect(
+ mergeGroupsPermissionsUpdates(permissions, update, modifiedGroupIds),
+ ).toEqual(expectedResult);
});
});
diff --git a/frontend/src/metabase/components/ConfirmContent/ConfirmContent.tsx b/frontend/src/metabase/components/ConfirmContent/ConfirmContent.tsx
index cecc405dd77..1df575f6db9 100644
--- a/frontend/src/metabase/components/ConfirmContent/ConfirmContent.tsx
+++ b/frontend/src/metabase/components/ConfirmContent/ConfirmContent.tsx
@@ -1,3 +1,4 @@
+import type { ReactNode } from "react";
import { t } from "ttag";
import _ from "underscore";
@@ -9,11 +10,13 @@ interface ConfirmContentProps {
"data-testid"?: string;
title: string;
content?: string | null;
- message?: string;
+ message?: string | ReactNode;
onClose?: () => void;
onAction?: () => void;
onCancel?: () => void;
confirmButtonText?: string;
+ confirmButtonPrimary?: boolean;
+ confirmButtonDanger?: boolean;
cancelButtonText?: string;
}
@@ -26,6 +29,8 @@ const ConfirmContent = ({
onAction = _.noop,
onCancel = _.noop,
confirmButtonText = t`Yes`,
+ confirmButtonPrimary = false,
+ confirmButtonDanger = !confirmButtonPrimary,
cancelButtonText = t`Cancel`,
}: ConfirmContentProps) => (
<ModalContent
@@ -42,16 +47,19 @@ const ConfirmContent = ({
<p className={CS.mb4}>{message}</p>
<div className={CS.mlAuto}>
+ {cancelButtonText && (
+ <Button
+ onClick={() => {
+ onCancel();
+ onClose();
+ }}
+ >
+ {cancelButtonText}
+ </Button>
+ )}
<Button
- onClick={() => {
- onCancel();
- onClose();
- }}
- >
- {cancelButtonText}
- </Button>
- <Button
- danger
+ primary={confirmButtonPrimary}
+ danger={confirmButtonDanger}
className={CS.ml2}
onClick={() => {
onAction();
diff --git a/frontend/src/metabase/plugins/index.ts b/frontend/src/metabase/plugins/index.ts
index cab615444ec..a63aae71e8e 100644
--- a/frontend/src/metabase/plugins/index.ts
+++ b/frontend/src/metabase/plugins/index.ts
@@ -97,6 +97,8 @@ export const PLUGIN_ADMIN_PERMISSIONS_DATABASE_ACTIONS = {
impersonated: [],
};
+export const PLUGIN_ADMIN_PERMISSIONS_TABLE_OPTIONS = [];
+
export const PLUGIN_ADMIN_PERMISSIONS_TABLE_ROUTES = [];
export const PLUGIN_ADMIN_PERMISSIONS_TABLE_GROUP_ROUTES = [];
export const PLUGIN_ADMIN_PERMISSIONS_TABLE_FIELDS_OPTIONS = [];
@@ -133,7 +135,7 @@ export const PLUGIN_DATA_PERMISSIONS: {
| ((
permissions: GroupsPermissions,
groupId: number,
- { databaseId }: DatabaseEntityId,
+ entityId: EntityId,
value: any,
database: Database,
permission: DataPermission,
diff --git a/src/metabase/api/permission_graph.clj b/src/metabase/api/permission_graph.clj
index 0b77e2745b1..ec2d738e259 100644
--- a/src/metabase/api/permission_graph.clj
+++ b/src/metabase/api/permission_graph.clj
@@ -53,17 +53,18 @@
;;; ------------------------------------------------ Data Permissions ------------------------------------------------
+(def ^:private Perms
+ "Perms that get reused for TablePerms and SchemaPerms"
+ [:enum
+ :all :segmented :none :full :limited :unrestricted :legacy-no-self-service :sandboxed :query-builder :no :blocked])
+
(def ^:private TablePerms
- [:or
- [:enum :all :segmented :none :full :limited :unrestricted :legacy-no-self-service :sandboxed :query-builder :no]
- [:map
- [:read {:optional true} [:enum :all :none]]
- [:query {:optional true} [:enum :all :none :segmented]]]])
+ [:or Perms [:map
+ [:read {:optional true} [:enum :all :none]]
+ [:query {:optional true} [:enum :all :none :segmented]]]])
(def ^:private SchemaPerms
- [:or
- [:enum :all :segmented :none :full :limited :unrestricted :legacy-no-self-service :sandboxed :query-builder :no]
- [:map-of Id TablePerms]])
+ [:or Perms [:map-of Id TablePerms]])
(def ^:private SchemaGraph
[:map-of
diff --git a/src/metabase/models/data_permissions.clj b/src/metabase/models/data_permissions.clj
index 97714ba433e..2fce4920dc5 100644
--- a/src/metabase/models/data_permissions.clj
+++ b/src/metabase/models/data_permissions.clj
@@ -385,23 +385,26 @@
(mu/defn schema-permission-for-user :- PermissionValue
"Returns the effective *schema-level* permission value for a given user, permission type, and database ID, and
schema name. If the user has multiple permissions for the given type in different groups, they are coalesced into a
- single value. The schema-level permission is the *least* restrictive table-level permission within that schema."
- [user-id perm-type database-id schema-name]
- (when (not= :model/Table (model-by-perm-type perm-type))
- (throw (ex-info (tru "Permission type {0} is not a table-level permission." perm-type)
- {perm-type (Permissions perm-type)})))
- (if (is-superuser? user-id)
- (most-permissive-value perm-type)
- ;; The schema-level permission is the most-restrictive table-level permission within a schema. So for each group,
- ;; select the most-restrictive table-level permission. Then use normal coalesce logic to select the *least*
- ;; restrictive group permission.
- (let [perm-values (->> (get-permissions user-id perm-type database-id)
- (filter #(or (= (:schema_name %) schema-name)
- (nil? (:table_id %))))
- (map :perm_value)
- (into #{}))]
- (or (coalesce perm-type perm-values)
- (least-permissive-value perm-type)))))
+ single value. The schema-level permission is the *least* restrictive table-level permission within that schema.
+
+ For databases without a schema, the schema name will be nil, but we want to compare that against the empty string instead."
+ [user-id perm-type database-id schema-name :- [:maybe :string]]
+ (let [schema-name (or schema-name "")]
+ (when (not= :model/Table (model-by-perm-type perm-type))
+ (throw (ex-info (tru "Permission type {0} is not a table-level permission." perm-type)
+ {perm-type (Permissions perm-type)})))
+ (if (is-superuser? user-id)
+ (most-permissive-value perm-type)
+ ;; The schema-level permission is the most-restrictive table-level permission within a schema. So for each group,
+ ;; select the most-restrictive table-level permission. Then use normal coalesce logic to select the *least*
+ ;; restrictive group permission.
+ (let [perm-values (->> (get-permissions user-id perm-type database-id)
+ (filter #(or (= (:schema_name %) schema-name)
+ (nil? (:table_id %))))
+ (map :perm_value)
+ (into #{}))]
+ (or (coalesce perm-type perm-values)
+ (least-permissive-value perm-type))))))
(mu/defn user-has-permission-for-schema? :- :boolean
"Returns a Boolean indicating whether the user has the specified permission value for the given database ID and schema,
@@ -608,8 +611,7 @@
"Sets a single permission to a specified value for a given group and database. If a permission value already exists
for the specified group and object, it will be updated to the new value.
- Block permissions (i.e. :perms/view-data :blocked) can only be set at the database-level, despite :perms/view-data
- being a table-level permission."
+ Block permissions (i.e. :perms/view-data :blocked) can be set at the table or database-level."
[group-or-id :- TheIdable
db-or-id :- TheIdable
perm-type :- PermissionType
@@ -727,7 +729,7 @@
group and table, it will be updated to the new value.
`table-perms` is a map from tables or table ID to the permission value for each table. All tables in the list must
- belong to the same database.
+ belong to the same database, or this will throw.
If this permission is currently set at the database-level, the database-level permission
is removed and table-level rows are are added for all of its tables. Similarly, if setting a table-level permission to a value
@@ -739,9 +741,6 @@
(throw (ex-info (tru "Permission type {0} cannot be set on tables." perm-type)
{perm-type (Permissions perm-type)})))
(let [values (set (vals table-perms))]
- (when (values :blocked)
- (throw (ex-info (tru "Block permissions must be set at the database-level only.")
- {})))
;; if `table-perms` is empty, there's nothing to do
(when (seq table-perms)
(t2/with-transaction [_conn]
@@ -792,7 +791,6 @@
;; If the previous database-level permission can't be set at
;; the table-level, we need to provide a new default
:query-builder-and-native :query-builder
- :blocked :unrestricted
existing-db-perm-value)
:db_id db-id
:table_id (:id table)
diff --git a/src/metabase/models/data_permissions/graph.clj b/src/metabase/models/data_permissions/graph.clj
index 905be23aa6c..58fc418a1d1 100644
--- a/src/metabase/models/data_permissions/graph.clj
+++ b/src/metabase/models/data_permissions/graph.clj
@@ -328,7 +328,8 @@
;; If the table is sandboxed, we set `view-data` to `unrestricted` since
;; sandboxes are stored separately in the `sandboxes` table
:sandboxed :unrestricted
- :legacy-no-self-service :legacy-no-self-service))))]
+ :legacy-no-self-service :legacy-no-self-service
+ :blocked :blocked))))]
(data-perms/set-table-permissions! group-id :perms/view-data new-table-perms)))
(defn- update-schema-level-view-data-permissions!
diff --git a/src/metabase/models/query/permissions.clj b/src/metabase/models/query/permissions.clj
index 4d93b796177..2cd1403ad9f 100644
--- a/src/metabase/models/query/permissions.clj
+++ b/src/metabase/models/query/permissions.clj
@@ -182,42 +182,57 @@
(throw (ex-info (tru "Invalid query type: {0}" query-type)
{:query query}))))))
-(defn check-db-level-perms
+(defn- has-perm-for-db?
"Checks that the current user has at least `required-perm` for the entire DB specified by `db-id`."
[perm-type required-perm gtap-perms db-id]
(or
- (data-perms/at-least-as-permissive? perm-type
- (data-perms/full-db-permission-for-user api/*current-user-id* perm-type db-id)
- required-perm)
- (when gtap-perms
- (data-perms/at-least-as-permissive? perm-type gtap-perms required-perm))
- (throw (perms-exception {db-id {perm-type required-perm}}))))
+ (data-perms/at-least-as-permissive? perm-type
+ (data-perms/full-db-permission-for-user api/*current-user-id* perm-type db-id)
+ required-perm)
+ (when gtap-perms
+ (data-perms/at-least-as-permissive? perm-type gtap-perms required-perm))))
-(defn check-table-level-perms
+(defn- has-perm-for-table?
"Checks that the current user has the permissions for tables specified in `table-id->perm`. This can be satisfied via
the user's permissions stored in the database, or permissions in `gtap-table-perms` which are supplied by the
- row-level-restrictions QP middleware when sandboxing is in effect. Throws an exception if the permission check fails;
- else returns `true`."
+ row-level-restrictions QP middleware when sandboxing is in effect. Returns true if access is allowed, otherwise false."
[perm-type table-id->required-perm gtap-table-perms db-id]
- (if table-id->required-perm
- (doseq [[table-id required-perm] table-id->required-perm]
- (or
- (data-perms/user-has-permission-for-table?
- api/*current-user-id*
- perm-type
- required-perm
- db-id
- table-id)
- (when-let [gtap-perm (if (keyword? gtap-table-perms)
- ;; gtap-table-perms can be a keyword representing the DB permission...
- gtap-table-perms
- ;; ...or a map from table IDs to table permissions
- (get gtap-table-perms table-id))]
- (data-perms/at-least-as-permissive? perm-type
- gtap-perm
- required-perm))
- (throw (perms-exception {db-id {perm-type {table-id required-perm}}}))))
- true))
+ (let [table-id->has-perm?
+ (into {} (for [[table-id required-perm] table-id->required-perm]
+ [table-id (boolean
+ (or (data-perms/user-has-permission-for-table?
+ api/*current-user-id*
+ perm-type
+ required-perm
+ db-id
+ table-id)
+ (when-let [gtap-perm (if (keyword? gtap-table-perms)
+ ;; gtap-table-perms can be a keyword representing the DB permission...
+ gtap-table-perms
+ ;; ...or a map from table IDs to table permissions
+ (get gtap-table-perms table-id))]
+ (data-perms/at-least-as-permissive? perm-type gtap-perm required-perm))))]))]
+ (every? true? (vals table-id->has-perm?))))
+
+(mu/defn has-perm-for-query? :- :boolean
+ "Returns true when the query is accessible for the given perm-type and required-perms for individual tables, or the
+ entire DB, false otherwise. Only throws if the permission format is incorrect."
+ [{{gtap-perms :gtaps} ::perms, db-id :database :as _query} perm-type required-perms]
+ (boolean
+ (if-let [db-or-table-perms (perm-type required-perms)]
+ ;; In practice, `view-data` will be defined at the table-level, and `create-queries` will either be table-level
+ ;; or :query-builder-and-native for the entire DB. But we should enforce whatever `required-perms` are provided,
+ ;; in case that ever changes.
+ (cond
+ (keyword? db-or-table-perms)
+ (has-perm-for-db? perm-type db-or-table-perms (perm-type gtap-perms) db-id)
+
+ (map? db-or-table-perms)
+ (has-perm-for-table? perm-type db-or-table-perms (perm-type gtap-perms) db-id)
+
+ :else
+ (throw (ex-info (tru "Invalid permissions format") required-perms)))
+ true)))
(defn check-data-perms
"Checks whether the current user has sufficient view data and query permissions to run `query`. Returns `true` if the
@@ -225,34 +240,19 @@
`throw-exceptions?` to `false`).
If the [:gtap ::perms] path is present in the query, these perms are implicitly granted to the current user."
- [{{gtap-perms :gtaps} ::perms, db-id :database} required-perms & {:keys [throw-exceptions?]
+ [{{gtap-perms :gtaps} ::perms, :as query} required-perms & {:keys [throw-exceptions?]
:or {throw-exceptions? true}}]
(try
;; Check any required v1 paths
(when-let [paths (:paths required-perms)]
- (let [paths-excluding-gtap-paths (set/difference paths (-> gtap-perms :paths))]
+ (let [paths-excluding-gtap-paths (set/difference paths (:paths gtap-perms))]
(or (perms/set-has-full-permissions-for-set? @api/*current-user-permissions-set* paths-excluding-gtap-paths)
(throw (perms-exception paths)))))
- ;; Check view-data and create-queries permissions, for individual tables or the entire DB
- (doseq [perm-type [:perms/view-data :perms/create-queries]]
- (when-let [db-or-table-perms (perm-type required-perms)]
- ;; In practice, `view-data` will be defined at the table-level, and `create-queries` will either be table-level
- ;; or :query-builder-and-native for the entire DB. But we should enforce whatever `required-perms` are provided,
- ;; in case that ever changes.
- (cond
- (keyword? db-or-table-perms)
- (check-db-level-perms perm-type db-or-table-perms (perm-type gtap-perms) db-id)
-
- (map? db-or-table-perms)
- (check-table-level-perms perm-type
- db-or-table-perms
- (perm-type gtap-perms)
- db-id)
-
- :else
- (throw (ex-info (tru "Invalid required permissions")
- required-perms)))))
+ ;; Check view-data and create-queries permissions, for individual tables or the entire DB:
+ (when (or (not (has-perm-for-query? query :perms/view-data required-perms))
+ (not (has-perm-for-query? query :perms/create-queries required-perms)))
+ (throw (perms-exception required-perms)))
true
(catch clojure.lang.ExceptionInfo e
diff --git a/src/metabase/query_processor/middleware/permissions.clj b/src/metabase/query_processor/middleware/permissions.clj
index 17048b92017..f9468a5ff0e 100644
--- a/src/metabase/query_processor/middleware/permissions.clj
+++ b/src/metabase/query_processor/middleware/permissions.clj
@@ -90,14 +90,15 @@
card-id
(do
(check-card-read-perms database-id card-id)
- (when-not (query-perms/check-data-perms outer-query required-perms :throw-exceptions? false)
- (check-block-permissions outer-query)))
+
+ (when-not (query-perms/has-perm-for-query? outer-query :perms/view-data required-perms)
+ (throw (query-perms/perms-exception required-perms))))
;; set when querying for field values of dashboard filters, which only require
;; collection perms for the dashboard and not ad-hoc query perms
*param-values-query*
- (when-not (query-perms/check-data-perms outer-query required-perms :throw-exceptions? false)
- (check-block-permissions outer-query))
+ (when-not (query-perms/has-perm-for-query? outer-query :perms/view-data required-perms)
+ (throw (query-perms/perms-exception required-perms)))
:else
(do
diff --git a/test/metabase/api/card_test.clj b/test/metabase/api/card_test.clj
index e6af90cb247..a039fd39a6c 100644
--- a/test/metabase/api/card_test.clj
+++ b/test/metabase/api/card_test.clj
@@ -23,6 +23,7 @@
:refer [Card CardBookmark Collection Dashboard Database ModerationReview
Pulse PulseCard PulseChannel PulseChannelRecipient Table Timeline
TimelineEvent]]
+ [metabase.models.data-permissions :as data-perms]
[metabase.models.interface :as mi]
[metabase.models.moderation-review :as moderation-review]
[metabase.models.permissions :as perms]
@@ -3503,37 +3504,152 @@
(is (=? {:can_run_adhoc_query false}
(mt/user-http-request :crowberto :get 200 (str "card/" (:id no-query))))))))
+(deftest data-and-collection-query-permissions-test
+ (mt/with-temp [:model/Collection collection {}
+ :model/Card card {:dataset_query {:database (mt/id)
+ :type :native
+ :native {:query "SELECT id FROM venues ORDER BY id ASC LIMIT 2;"}}
+ :database_id (mt/id)
+ :collection_id (u/the-id collection)}]
+ (letfn [(process-query []
+ (mt/user-http-request :rasta :post (format "card/%d/query" (u/the-id card))))
+ (blocked? [response] (or
+ (= "You don't have permissions to do that." response)
+ (re-matches #"Blocked: you are not allowed to run queries against Database \d+."
+ (:error response))))]
+ ;; | Data perms | Collection perms | outcome
+ ;; ------------ | ---------------- | --------
+ ;; | no | no | blocked
+ ;; | yes | no | blocked
+ ;; | no | yes | blocked
+ ;; | yes | yes | OK
+
+ (testing "Should NOT be able to run the parent Card with :blocked data-perms and no collection perms"
+ (mt/with-no-data-perms-for-all-users!
+ (mt/with-non-admin-groups-no-collection-perms collection
+ (data-perms/set-table-permission! (perms-group/all-users) (mt/id :venues) :perms/view-data :blocked)
+ (perms/revoke-collection-permissions! (perms-group/all-users) collection)
+ (mt/with-test-user :rasta
+ (is (not (mi/can-read? collection)))
+ (is (not (mi/can-read? card))))
+ (is (blocked? (process-query))))))
+
+ (testing "Should NOT be able to run the parent Card with valid data-perms and no collection perms"
+ (mt/with-no-data-perms-for-all-users!
+ (mt/with-non-admin-groups-no-collection-perms collection
+ (data-perms/set-table-permission! (perms-group/all-users) (mt/id :venues) :perms/view-data :unrestricted)
+ (perms/revoke-collection-permissions! (perms-group/all-users) collection)
+ (mt/with-test-user :rasta
+ (is (not (mi/can-read? collection)))
+ (is (not (mi/can-read? card))))
+ (is (blocked? (process-query))))))
+
+ (testing "should NOT be able to run native queries with :blocked data-perms on any table"
+ (mt/with-no-data-perms-for-all-users!
+ (mt/with-non-admin-groups-no-collection-perms collection
+ (data-perms/set-table-permission! (perms-group/all-users) (mt/id :venues) :perms/view-data :blocked)
+ (perms/grant-collection-read-permissions! (perms-group/all-users) collection)
+ (mt/with-test-user :rasta
+ (is (mi/can-read? collection))
+ (is (mi/can-read? card)))
+ (is (process-query)))))
+
+ ;; delete these in place so we can reset them below, you cannot set them twice in a row
+ (perms/revoke-collection-permissions! (perms-group/all-users) collection)
+
+ (testing "should NOT be able to run the parent Card when data-perms and valid collection perms"
+ (mt/with-no-data-perms-for-all-users!
+ (mt/with-non-admin-groups-no-collection-perms collection
+ (data-perms/set-database-permission! (perms-group/all-users) (mt/id) :perms/view-data :unrestricted)
+ (perms/grant-collection-read-permissions! (perms-group/all-users) collection)
+ (mt/with-test-user :rasta
+ (is (mi/can-read? collection))
+ (is (mi/can-read? card)))
+ (is (= [[1] [2]] (mt/rows (process-query))))))))))
+
(deftest nested-query-permissions-test
- (testing "Should be able to run a Card with another Card as its source query with just perms for the former (#15131)"
- (mt/with-no-data-perms-for-all-users!
- (mt/with-non-admin-groups-no-root-collection-perms
- (mt/with-temp [:model/Collection allowed-collection {}
- :model/Collection disallowed-collection {}
- :model/Card parent-card {:dataset_query {:database (mt/id)
- :type :native
- :native {:query "SELECT id FROM venues ORDER BY id ASC LIMIT 2;"}}
- :database_id (mt/id)
- :collection_id (u/the-id disallowed-collection)}
- :model/Card child-card {:dataset_query {:database (mt/id)
- :type :query
- :query {:source-table (format "card__%d" (u/the-id parent-card))}}
- :collection_id (u/the-id allowed-collection)}]
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (mt/with-temp [:model/Collection disallowed-collection {}
+ :model/Card parent-card {:dataset_query {:database (mt/id)
+ :type :native
+ :native {:query "SELECT id FROM venues ORDER BY id ASC LIMIT 2;"}}
+ :database_id (mt/id)
+ :collection_id (u/the-id disallowed-collection)}
+ :model/Collection allowed-collection {}
+ :model/Card child-card {:dataset_query {:database (mt/id)
+ :type :query
+ :query {:source-table (format "card__%d" (u/the-id parent-card))}}
+ :collection_id (u/the-id allowed-collection)}]
+ (letfn [(rasta-view-data-perm= [perm] (is (= perm
+ (get-in (data-perms/permissions-for-user (mt/user->id :rasta)) [(mt/id) :perms/view-data]))
+ "rasta should be blocked for this table."))]
+ (mt/with-no-data-perms-for-all-users!
+ (data-perms/set-database-permission! (perms-group/all-users) (mt/id) :perms/view-data :blocked)
(perms/grant-collection-read-permissions! (perms-group/all-users) allowed-collection)
(letfn [(process-query-for-card [card]
(mt/user-http-request :rasta :post (format "card/%d/query" (u/the-id card))))]
- (testing "Should not be able to run the parent Card"
- (mt/with-test-user :rasta
- (is (not (mi/can-read? disallowed-collection)))
- (is (not (mi/can-read? parent-card))))
- (is (= "You don't have permissions to do that."
- (process-query-for-card parent-card))))
- (testing "Should be able to run the child Card (#15131)"
- (mt/with-test-user :rasta
- (is (not (mi/can-read? parent-card)))
- (is (mi/can-read? allowed-collection))
- (is (mi/can-read? child-card)))
- (is (= [[1] [2]]
- (mt/rows (process-query-for-card child-card)))))))))))
+ (testing "Should be able to run a Card with another Card as its source query with just perms for the former (#15131)"
+ (testing "Should not be able to run the parent Card"
+ (mt/with-test-user :rasta
+ (is (not (mi/can-read? disallowed-collection)))
+ (is (not (mi/can-read? parent-card))))
+ (is (= "You don't have permissions to do that."
+ (process-query-for-card parent-card))))
+ (testing "Should be able to run the child Card (#15131)"
+ (mt/with-test-user :rasta
+ (is (not (mi/can-read? parent-card)))
+ (is (mi/can-read? allowed-collection))
+ (is (mi/can-read? child-card)))
+ (testing "Data perms prohibit running queries"
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"You do not have permissions to run this query."
+ (mt/rows (process-query-for-card child-card)))
+ "Even if the user has can-write? on a Card, they should not be able to run it because they are blocked on Card's db"))))
+ (testing "view-data = unrestricted is required to allow running the query"
+ (mt/with-restored-data-perms!
+ (data-perms/set-database-permission! (perms-group/all-users) (mt/id) :perms/view-data :unrestricted)
+ (rasta-view-data-perm= :unrestricted)
+ (is (= [[1] [2]] (mt/rows (process-query-for-card child-card)))
+ "view-data = unrestricted is sufficient to allow running the query")))))))))
+
+(deftest cannot-run-any-native-queries-when-blocked-test
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (mt/with-temp [:model/Collection allowed-collection {}
+ :model/Collection disallowed-collection {}
+ :model/Card parent-card {:dataset_query {:database (mt/id)
+ :type :native
+ :native {:query "SELECT id FROM venues ORDER BY id ASC LIMIT 2;"}}
+ :database_id (mt/id)
+ :collection_id (u/the-id disallowed-collection)}
+ :model/Card child-card {:dataset_query {:database (mt/id)
+ :type :query
+ :query {:source-table (format "card__%d" (u/the-id parent-card))}}
+ :collection_id (u/the-id allowed-collection)}]
+ (letfn [(process-query-for-card [card]
+ (mt/user-http-request :rasta :post (format "card/%d/query" (u/the-id card))))]
+ (testing "Cannot run native queries when a single table is unrestricted and the rest are blocked"
+ (mt/with-no-data-perms-for-all-users!
+ (data-perms/set-database-permission! (perms-group/all-users) (mt/id) :perms/view-data :blocked)
+ (data-perms/set-table-permission! (perms-group/all-users) (mt/id :venues) :perms/view-data :unrestricted)
+ (perms/grant-collection-read-permissions! (perms-group/all-users) allowed-collection)
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"You do not have permissions to run this query."
+ (mt/rows (process-query-for-card child-card)))
+ "Someone with `:blocked` permissions on ANY table in the database cannot run ANY card with native queries, including as a source for another card.")))
+ ;; update collection perms in place:
+ (perms/revoke-collection-permissions! (perms-group/all-users) allowed-collection)
+ (testing "Cannot run native queries when a single table is blocked and the rest are unrestricted"
+ (mt/with-no-data-perms-for-all-users!
+ (data-perms/set-database-permission! (perms-group/all-users) (mt/id) :perms/view-data :unrestricted)
+ (data-perms/set-table-permission! (perms-group/all-users) (mt/id :venues) :perms/view-data :blocked)
+ (perms/grant-collection-read-permissions! (perms-group/all-users) allowed-collection)
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"You do not have permissions to run this query."
+ (mt/rows (process-query-for-card child-card)))
+ "Someone with `:blocked` permissions on ANY table in the database cannot run ANY card with native queries, including as a source for another card.")))))))
(deftest query-metadata-test
(mt/with-temp
diff --git a/test/metabase/api/database_test.clj b/test/metabase/api/database_test.clj
index 431426ad9c7..98ef8ce97bc 100644
--- a/test/metabase/api/database_test.clj
+++ b/test/metabase/api/database_test.clj
@@ -1522,6 +1522,14 @@
(is (= ["" " "]
(mt/user-http-request :lucky :get 200 (format "database/%d/schemas" db-id))))))))
+(deftest ^:parallel blank-schema-identifier-test
+ (testing "We should handle Databases with blank schema correctly (#12450)"
+ (t2.with-temp/with-temp [Database {db-id :id} {:name "my/database"}]
+ (doseq [schema-name [nil ""]]
+ (testing (str "schema name = " (pr-str schema-name))
+ (t2.with-temp/with-temp [Table _ {:db_id db-id, :schema schema-name, :name "just a table"}]
+ (is (= [""] (mt/user-http-request :rasta :get 200 (format "database/%d/schemas" db-id))))))))))
+
(deftest get-syncable-schemas-test
(testing "GET /api/database/:id/syncable_schemas"
(testing "Multiple schemas are ordered by name"
diff --git a/test/metabase/api/dataset_test.clj b/test/metabase/api/dataset_test.clj
index 55d55299fb8..cf879b37c53 100644
--- a/test/metabase/api/dataset_test.clj
+++ b/test/metabase/api/dataset_test.clj
@@ -210,7 +210,6 @@
:type :query
:query {:source-table (str "card__" (u/the-id card))}}))]
(is (some? result))
- (def result result)
(when (some? result)
(is (= 16
(count (csv/read-csv result)))))))]
diff --git a/test/metabase/models/data_permissions/graph_test.clj b/test/metabase/models/data_permissions/graph_test.clj
index ba1dbfe8a55..e8124fae9eb 100644
--- a/test/metabase/models/data_permissions/graph_test.clj
+++ b/test/metabase/models/data_permissions/graph_test.clj
@@ -378,6 +378,9 @@
{:perms/view-data
{"PUBLIC" {1 :unrestricted
2 :unrestricted}}} {:view-data {"PUBLIC" :unrestricted}}
+ {:perms/view-data
+ {"PUBLIC" {1 :blocked ;; table level blocked is removed:
+ 2 :unrestricted}}} {:view-data {"PUBLIC" {2 :unrestricted}}}
{:perms/view-data
{"PUBLIC" {1 :legacy-no-self-service
2 :legacy-no-self-service}}} {:view-data {"PUBLIC" :legacy-no-self-service}}
@@ -427,6 +430,21 @@
(is (= {(mt/id :categories) :query-builder, (mt/id :venues) :query-builder}
(test-query-graph group)))))))
+(deftest graph-set-blocked-permissions-for-table-test
+ (let [view-data (fn view-data [group]
+ (get-in (data-perms.graph/api-graph)
+ [:groups (u/the-id group) (mt/id) :view-data]))]
+ (testing "It is possible to set :blocked permissions for a table -- #46542"
+ (mt/with-temp [:model/PermissionsGroup group]
+ (data-perms/set-database-permission! group (mt/id) :perms/view-data :unrestricted)
+ (testing "before"
+ (data-perms/set-table-permission! group (mt/id :venues) :perms/create-queries :query-builder)
+ (is (= :unrestricted (view-data group))))
+ (testing "after"
+ (data-perms/set-table-permission! group (mt/id :categories) :perms/view-data :blocked)
+ (is (malli= [:sequential {:min 1} :int] (keys (get (view-data group) "PUBLIC"))))
+ (is (not (contains? (view-data group) (mt/id :categories)))))))))
+
(deftest audit-db-update-test
(testing "Throws exception when we attempt to change the audit db permission manually."
(mt/with-temp [:model/PermissionsGroup group {}]
diff --git a/test/metabase/models/data_permissions_test.clj b/test/metabase/models/data_permissions_test.clj
index 5437a36b65a..24eec6abbce 100644
--- a/test/metabase/models/data_permissions_test.clj
+++ b/test/metabase/models/data_permissions_test.clj
@@ -12,7 +12,9 @@
(deftest ^:parallel coalesce-test
(testing "`coalesce` correctly returns the most permissive value by default"
(are [expected args] (= expected (apply data-perms/coalesce args))
- :unrestricted [:perms/view-data #{:unrestricted :legacy-no-self-service :blocked}]
+ :unrestricted [:perms/view-data #{:unrestricted :legacy-no-self-service :blocked}]
+ :unrestricted [:perms/view-data #{:unrestricted :legacy-no-self-service}]
+ :blocked [:perms/view-data #{:legacy-no-self-service :blocked}]
:blocked [:perms/view-data #{:blocked}]
nil [:perms/view-data #{}])))
@@ -109,11 +111,8 @@
#"Permission type :perms/create-queries cannot be set to :invalid"
(data-perms/set-table-permissions! group-id :perms/create-queries {table-id-1 :invalid}))))
- (testing "A table-level permission cannot be set to :block"
- (is (thrown-with-msg?
- ExceptionInfo
- #"Block permissions must be set at the database-level only."
- (data-perms/set-table-permissions! group-id :perms/view-data {table-id-1 :blocked}))))
+ (testing "A table-level permission can be set to :block"
+ (is (= nil (data-perms/set-table-permissions! group-id :perms/view-data {table-id-1 :blocked}))))
(testing "Table-level permissions can only be set in bulk for tables in the same database"
(is (thrown-with-msg?
@@ -129,6 +128,24 @@
(is (nil? (create-queries-perm-value table-id-2)))
(is (nil? (create-queries-perm-value table-id-3))))))))
+(deftest native-queries-against-db-with-some-blocked-table-is-illegal-test
+ (mt/with-temp [:model/Card {card-id :id {db-id :database} :dataset_query} {:dataset_query (mt/native-query {:query "select 1"})}]
+ (mt/with-no-data-perms-for-all-users!
+ (data-perms/set-database-permission! (perms-group/all-users) db-id :perms/create-queries (data-perms/most-permissive-value :perms/create-queries))
+ (data-perms/set-database-permission! (perms-group/all-users) db-id :perms/view-data (data-perms/most-permissive-value :perms/view-data))
+ ;; rasta has access to the database:
+ (is (= "Can Run Query"
+ (:error (mt/user-http-request :rasta :post 202 (format "card/%d/query" card-id))
+ "Can Run Query")))
+
+ ;; block a single table on the db:
+ (let [tables-in-db (map :id (:tables (t2/hydrate (t2/select-one :model/Database db-id) :tables)))
+ table-id (rand-nth tables-in-db)]
+ (data-perms/set-table-permissions! (perms-group/all-users) :perms/view-data {table-id :blocked}))
+
+ (is (= "You do not have permissions to run this query."
+ (:error (mt/user-http-request :rasta :post 202 (format "card/%d/query" card-id))))))))
+
(deftest database-permission-for-user-test
(mt/with-temp [:model/PermissionsGroup {group-id-1 :id} {}
:model/PermissionsGroup {group-id-2 :id} {}
diff --git a/test/metabase/query_processor/card_test.clj b/test/metabase/query_processor/card_test.clj
index 2db499c78e5..b2c4ad2aead 100644
--- a/test/metabase/query_processor/card_test.clj
+++ b/test/metabase/query_processor/card_test.clj
@@ -5,6 +5,7 @@
[clojure.test :refer :all]
[metabase.api.common :as api]
[metabase.models :refer [Card]]
+ [metabase.models.data-permissions :as data-perms]
[metabase.models.interface :as mi]
[metabase.models.permissions :as perms]
[metabase.models.permissions-group :as perms-group]
@@ -198,6 +199,8 @@
:query {:source-table (format "card__%d" (u/the-id parent-card))}}
:collection_id (u/the-id allowed-collection)}]
(perms/grant-collection-read-permissions! (perms-group/all-users) allowed-collection)
+ (data-perms/set-database-permission! (perms-group/all-users) (mt/id) :perms/create-queries :query-builder-and-native)
+ (data-perms/set-database-permission! (perms-group/all-users) (mt/id) :perms/view-data :unrestricted)
(mt/with-test-user :rasta
(letfn [(process-query-for-card [card]
(qp.card/process-query-for-card
--
GitLab