diff --git a/e2e/test/scenarios/permissions/admin-permissions.cy.spec.js b/e2e/test/scenarios/permissions/admin-permissions.cy.spec.js
index 5023d66774f8ea8eabdf502167e8521827be8382..87fef34ca5a976d0be87f4480e9227b6bb3f4922 100644
--- a/e2e/test/scenarios/permissions/admin-permissions.cy.spec.js
+++ b/e2e/test/scenarios/permissions/admin-permissions.cy.spec.js
@@ -19,7 +19,8 @@ import {
setTokenFeatures,
} from "e2e/support/helpers";
-const { ALL_USERS_GROUP, ADMIN_GROUP, COLLECTION_GROUP } = USER_GROUPS;
+const { ALL_USERS_GROUP, ADMIN_GROUP, COLLECTION_GROUP, DATA_GROUP } =
+ USER_GROUPS;
const COLLECTION_ACCESS_PERMISSION_INDEX = 0;
@@ -581,3 +582,41 @@ describe("scenarios > admin > permissions", () => {
});
});
});
+
+describe("scenarios > admin > permissions", () => {
+ beforeEach(() => {
+ restore();
+ cy.signInAsAdmin();
+ });
+
+ it("partial data permission updates should not remove permissions from other unmodified groups", () => {
+ // check the we have an expected initial state
+ cy.visit(`admin/permissions/data/group/${DATA_GROUP}`);
+ assertPermissionTable([["Sample Database", "Query builder and native"]]);
+
+ // make a change to the permissions of another group
+ selectSidebarItem("nosql");
+ assertPermissionTable([["Sample Database", "Query builder only"]]);
+ modifyPermission("Sample Database", NATIVE_QUERIES_PERMISSION_INDEX, "No");
+
+ // observe the save change request and assert that we don't get back
+ // values for groups we did not modify
+ cy.intercept("PUT", "/api/permissions/graph").as("updateGraph");
+
+ // save changes
+ cy.button("Save changes").click();
+ modal().within(() => {
+ cy.button("Yes").click();
+ });
+
+ cy.wait("@updateGraph").then(interception => {
+ const requestGroupIds = Object.keys(interception.request.body.groups);
+ const responseGroupIds = Object.keys(interception.response.body.groups);
+ expect(requestGroupIds).to.deep.equal(responseGroupIds);
+ });
+
+ // make sure that our other group's permission data did not get changed
+ selectSidebarItem("data");
+ assertPermissionTable([["Sample Database", "Query builder and native"]]);
+ });
+});
diff --git a/enterprise/backend/src/metabase_enterprise/advanced_permissions/models/connection_impersonation.clj b/enterprise/backend/src/metabase_enterprise/advanced_permissions/models/connection_impersonation.clj
index 045764c89fd5fb62afb99dc485dd472eff9cf712..9cd5557c23578c94bb5b39a9c256b558b4570363 100644
--- a/enterprise/backend/src/metabase_enterprise/advanced_permissions/models/connection_impersonation.clj
+++ b/enterprise/backend/src/metabase_enterprise/advanced_permissions/models/connection_impersonation.clj
@@ -21,13 +21,14 @@
(defenterprise add-impersonations-to-permissions-graph
"Augment a provided permissions graph with active connection impersonation policies."
:feature :advanced-permissions
- [graph & {:keys [group-id db-id audit-db?]}]
+ [graph & {:keys [group-ids group-id db-id audit-db?]}]
(m/deep-merge
graph
(let [impersonations (t2/select :model/ConnectionImpersonation
{:where [:and
(when db-id [:= :db_id db-id])
(when group-id [:= :group_id group-id])
+ (when group-ids [:in :group_id group-ids])
(when-not audit-db? [:not [:= :db_id audit/audit-db-id]])]})]
(reduce (fn [acc {:keys [db_id group_id]}]
(assoc-in acc [group_id db_id :view-data] :impersonated))
diff --git a/enterprise/backend/src/metabase_enterprise/sandbox/models/group_table_access_policy.clj b/enterprise/backend/src/metabase_enterprise/sandbox/models/group_table_access_policy.clj
index d9c85336cbc9616f0cf4426df5b80307d8624253..6b1bfa22bb24912a637a11ddd0b1348e105d3d9f 100644
--- a/enterprise/backend/src/metabase_enterprise/sandbox/models/group_table_access_policy.clj
+++ b/enterprise/backend/src/metabase_enterprise/sandbox/models/group_table_access_policy.clj
@@ -120,13 +120,14 @@
(defenterprise add-sandboxes-to-permissions-graph
"Augments a provided permissions graph with active sandboxing policies."
:feature :sandboxes
- [graph & {:keys [group-id db-id audit?]}]
+ [graph & {:keys [group-ids group-id db-id audit?]}]
(let [sandboxes (t2/select :model/GroupTableAccessPolicy
{:select [:s.group_id :s.table_id :t.db_id :t.schema]
:from [[:sandboxes :s]]
:join [[:metabase_table :t] [:= :s.table_id :t.id]]
:where [:and
(when group-id [:= :s.group_id group-id])
+ (when group-ids [:in :s.group_id group-ids])
(when db-id [:= :t.db_id db-id])
(when-not audit? [:not [:= :t.db_id audit/audit-db-id]])]})]
;; Incorporate each sandbox policy into the permissions graph.
diff --git a/enterprise/frontend/src/metabase-enterprise/advanced_permissions/index.js b/enterprise/frontend/src/metabase-enterprise/advanced_permissions/index.js
index f779955abf9ab419b4bbb968c5ad5ac3c88c59b4..d1dded518a5270ade4eea7ba0f61a2105562d1d1 100644
--- a/enterprise/frontend/src/metabase-enterprise/advanced_permissions/index.js
+++ b/enterprise/frontend/src/metabase-enterprise/advanced_permissions/index.js
@@ -121,9 +121,13 @@ if (hasPremiumFeature("advanced_permissions")) {
PLUGIN_REDUCERS.advancedPermissionsPlugin = advancedPermissionsSlice.reducer;
- PLUGIN_DATA_PERMISSIONS.permissionsPayloadExtraSelectors.push(state => ({
- impersonations: getImpersonations(state),
- }));
+ PLUGIN_DATA_PERMISSIONS.permissionsPayloadExtraSelectors.push(
+ (state, data) => {
+ const impersonations = getImpersonations(state);
+ const impersonationGroupIds = impersonations.map(i => `${i.group_id}`);
+ return [{ impersonations }, impersonationGroupIds];
+ },
+ );
PLUGIN_DATA_PERMISSIONS.hasChanges.push(
state => getImpersonations(state).length > 0,
diff --git a/enterprise/frontend/src/metabase-enterprise/sandboxes/index.js b/enterprise/frontend/src/metabase-enterprise/sandboxes/index.js
index 7655105c102d2eb3cd065c5226f21de0ba083fda..ce8670612efd5aef25e1e421f29dad3c80b8ec32 100644
--- a/enterprise/frontend/src/metabase-enterprise/sandboxes/index.js
+++ b/enterprise/frontend/src/metabase-enterprise/sandboxes/index.js
@@ -1,5 +1,6 @@
import { push } from "react-router-redux";
import { t } from "ttag";
+import _ from "underscore";
import { DataPermissionValue } from "metabase/admin/permissions/types";
import {
@@ -88,9 +89,8 @@ if (hasPremiumFeature("sandboxes")) {
PLUGIN_DATA_PERMISSIONS.permissionsPayloadExtraSelectors.push(state => {
const sandboxes = getDraftPolicies(state);
- return {
- sandboxes,
- };
+ const modifiedGroupIds = _.uniq(sandboxes.map(sb => sb.group_id));
+ return [{ sandboxes }, modifiedGroupIds];
});
PLUGIN_DATA_PERMISSIONS.hasChanges.push(hasPolicyChanges);
diff --git a/frontend/src/metabase-types/api/permissions.ts b/frontend/src/metabase-types/api/permissions.ts
index 392997e379bb422457a023ba0a94a7fe1a0f7de6..388cc369a1534fd7d36d8e129ee8adf4d21acc3d 100644
--- a/frontend/src/metabase-types/api/permissions.ts
+++ b/frontend/src/metabase-types/api/permissions.ts
@@ -18,7 +18,7 @@ export type PermissionsGraph = {
};
export type GroupsPermissions = {
- [key: GroupId]: GroupPermissions;
+ [key: GroupId | string]: GroupPermissions;
};
export type GroupPermissions = {
diff --git a/frontend/src/metabase/admin/permissions/permissions.js b/frontend/src/metabase/admin/permissions/permissions.js
index e7ce9a10928af8f9015ce421135fd16c725fb96e..91767ebd38cc663ac673bb2c0460b17d7dd22539 100644
--- a/frontend/src/metabase/admin/permissions/permissions.js
+++ b/frontend/src/metabase/admin/permissions/permissions.js
@@ -31,6 +31,10 @@ import { CollectionsApi, PermissionsApi } from "metabase/services";
import { trackPermissionChange } from "./analytics";
import { DataPermissionType, DataPermission } from "./types";
import { isDatabaseEntityId } from "./utils/data-entity-id";
+import {
+ getModifiedGroupsPermissionsGraphParts,
+ mergeGroupsPermissionsUpdates,
+} from "./utils/graph/partial-updates";
const INITIALIZE_DATA_PERMISSIONS =
"metabase/admin/permissions/INITIALIZE_DATA_PERMISSIONS";
@@ -189,32 +193,37 @@ export const saveDataPermissions = createThunkAction(
() => async (_dispatch, getState) => {
MetabaseAnalytics.trackStructEvent("Permissions", "save");
const state = getState();
- const groupIds = Object.keys(state.entities.groups);
- const { dataPermissions, dataPermissionsRevision } =
- getState().admin.permissions;
-
- // catch edge case where user has deleted a group but has loaded the permissions graph
- // with state for the now deleted group still in the graph
- const groupDataPermissions = _.pick(dataPermissions, groupIds);
-
- const extraData =
+ const allGroupIds = Object.keys(state.entities.groups);
+ const {
+ originalDataPermissions,
+ dataPermissions,
+ dataPermissionsRevision,
+ } = state.admin.permissions;
+
+ const advancedPermissions =
PLUGIN_DATA_PERMISSIONS.permissionsPayloadExtraSelectors.reduce(
(data, selector) => {
+ const [extraData, modifiedGroupIds] = selector(state);
return {
- ...data,
- ...selector(getState()),
+ permissions: { ...data.permissions, ...extraData },
+ modifiedGroupIds: [...data.modifiedGroupIds, ...modifiedGroupIds],
};
},
- {},
+ { modifiedGroupIds: [], permissions: {} },
);
- const permissionsGraph = {
- groups: groupDataPermissions,
- revision: dataPermissionsRevision,
- ...extraData,
- };
+ const modifiedGroups = getModifiedGroupsPermissionsGraphParts(
+ dataPermissions,
+ originalDataPermissions,
+ allGroupIds,
+ advancedPermissions.modifiedGroupIds,
+ );
- return await PermissionsApi.updateGraph(permissionsGraph);
+ return await PermissionsApi.updateGraph({
+ groups: modifiedGroups,
+ revision: dataPermissionsRevision,
+ ...advancedPermissions.permissions,
+ });
},
);
@@ -288,7 +297,10 @@ const dataPermissions = handleActions(
[LOAD_DATA_PERMISSIONS_FOR_DB]: {
next: (state, { payload }) => merge(payload.groups, state),
},
- [SAVE_DATA_PERMISSIONS]: { next: (_state, { payload }) => payload.groups },
+ [SAVE_DATA_PERMISSIONS]: {
+ next: (state, { payload }) =>
+ mergeGroupsPermissionsUpdates(state, payload.groups),
+ },
[UPDATE_DATA_PERMISSION]: {
next: (state, { payload }) => {
if (payload == null) {
@@ -393,7 +405,8 @@ const originalDataPermissions = handleActions(
next: (state, { payload }) => merge(payload.groups, state),
},
[SAVE_DATA_PERMISSIONS]: {
- next: (_state, { payload }) => payload.groups,
+ next: (state, { payload }) =>
+ mergeGroupsPermissionsUpdates(state, payload.groups),
},
},
null,
@@ -452,7 +465,7 @@ const originalCollectionPermissions = handleActions(
next: (_state, { payload }) => payload.groups,
},
[SAVE_COLLECTION_PERMISSIONS]: {
- next: (_state, { payload }) => payload.groups,
+ next: (state, { payload }) => payload.groups,
},
},
null,
diff --git a/frontend/src/metabase/admin/permissions/utils/graph/partial-updates.ts b/frontend/src/metabase/admin/permissions/utils/graph/partial-updates.ts
new file mode 100644
index 0000000000000000000000000000000000000000..e569093bad7eb67495af8e98a799916d18adf535
--- /dev/null
+++ b/frontend/src/metabase/admin/permissions/utils/graph/partial-updates.ts
@@ -0,0 +1,51 @@
+import _ from "underscore";
+
+import type { GroupsPermissions } from "metabase-types/api";
+
+// utils for dealing with partial graph updates
+
+// select only the parts of the permission graph that contain some kind of edit
+// currently only selects values based on some kind of modification happening anywhere
+// in the graph for a particular group
+export function getModifiedGroupsPermissionsGraphParts(
+ dataPermissions: GroupsPermissions,
+ originalDataPermissions: GroupsPermissions,
+ allGroupIds: string[],
+ externallyModifiedGroupIds: string[],
+) {
+ const dataPermissionsModifiedGroupIds = allGroupIds.filter(groupId => {
+ return !_.isEqual(
+ dataPermissions[groupId],
+ originalDataPermissions[groupId],
+ );
+ });
+
+ const allModifiedGroupIds = _.uniq([
+ ...dataPermissionsModifiedGroupIds,
+ ...externallyModifiedGroupIds,
+ ]);
+
+ return _.pick(dataPermissions, allModifiedGroupIds);
+}
+
+export function mergeGroupsPermissionsUpdates(
+ originalDataPermissions: GroupsPermissions | null | undefined,
+ newDataPermissions: GroupsPermissions,
+) {
+ if (!originalDataPermissions) {
+ return newDataPermissions;
+ }
+
+ const allGroupIds = _.uniq([
+ ...Object.keys(originalDataPermissions),
+ ...Object.keys(newDataPermissions),
+ ]);
+
+ const latestPermissionsEntries = allGroupIds.map(groupId => {
+ const permissions =
+ newDataPermissions[groupId] ?? originalDataPermissions[groupId];
+ return [groupId, permissions];
+ });
+
+ return Object.fromEntries(latestPermissionsEntries);
+}
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
new file mode 100644
index 0000000000000000000000000000000000000000..bfee33ba6bf58d861969325ff7608afe11534682
--- /dev/null
+++ b/frontend/src/metabase/admin/permissions/utils/graph/partial-updates.unit.spec.ts
@@ -0,0 +1,108 @@
+import _ from "underscore";
+
+import type { GroupsPermissions } from "metabase-types/api";
+
+import { DataPermission, DataPermissionValue } from "../../types";
+
+import {
+ getModifiedGroupsPermissionsGraphParts,
+ mergeGroupsPermissionsUpdates,
+} from "./partial-updates";
+
+describe("getModifiedGroupsPermissionsGraphParts", () => {
+ it("should only include groups that have had data permission updated", async () => {
+ const simpleUpdate = getModifiedGroupsPermissionsGraphParts(
+ {
+ "1": {
+ "1": { [DataPermission.VIEW_DATA]: DataPermissionValue.UNRESTRICTED },
+ },
+ "2": { "1": { [DataPermission.VIEW_DATA]: DataPermissionValue.NO } },
+ },
+ {
+ "1": {
+ "1": { [DataPermission.VIEW_DATA]: DataPermissionValue.UNRESTRICTED },
+ },
+ "2": {
+ "1": { [DataPermission.VIEW_DATA]: DataPermissionValue.UNRESTRICTED },
+ },
+ },
+ ["1", "2"],
+ [],
+ );
+
+ // should not contain group that had not been modified
+ expect(simpleUpdate).not.toHaveProperty("1");
+ expect(simpleUpdate).toEqual({
+ "2": { "1": { [DataPermission.VIEW_DATA]: DataPermissionValue.NO } },
+ });
+ });
+
+ it("should include groups that have been externally modified", async () => {
+ const externalUpdate = getModifiedGroupsPermissionsGraphParts(
+ {
+ "1": {
+ "1": { [DataPermission.VIEW_DATA]: DataPermissionValue.UNRESTRICTED },
+ },
+ "2": {
+ "1": { [DataPermission.VIEW_DATA]: DataPermissionValue.UNRESTRICTED },
+ },
+ },
+ {
+ "1": {
+ "1": { [DataPermission.VIEW_DATA]: DataPermissionValue.UNRESTRICTED },
+ },
+ "2": {
+ "1": { [DataPermission.VIEW_DATA]: DataPermissionValue.UNRESTRICTED },
+ },
+ },
+ ["1", "2"],
+ ["1"],
+ );
+
+ expect(externalUpdate).toEqual({
+ "1": {
+ "1": { [DataPermission.VIEW_DATA]: DataPermissionValue.UNRESTRICTED },
+ },
+ });
+ });
+});
+
+describe("mergeGroupsPermissionsUpdates", () => {
+ // test is a product of bad typings... ideally our state should never be null
+ // but our reducers are typed such that it could be null
+ it("should take only new permissions if there's not previous state", async () => {
+ const update: GroupsPermissions = {
+ "1": {
+ "1": { [DataPermission.VIEW_DATA]: DataPermissionValue.UNRESTRICTED },
+ },
+ };
+
+ expect(mergeGroupsPermissionsUpdates(undefined, update)).toBe(update);
+ });
+
+ it("should only apply updates to groups that have been modified", async () => {
+ const permissions: GroupsPermissions = {
+ "1": {
+ "1": { [DataPermission.VIEW_DATA]: DataPermissionValue.UNRESTRICTED },
+ },
+ "2": {
+ "1": { [DataPermission.VIEW_DATA]: DataPermissionValue.UNRESTRICTED },
+ },
+ };
+
+ const update: GroupsPermissions = {
+ "2": { "1": { [DataPermission.VIEW_DATA]: DataPermissionValue.NO } },
+ };
+
+ const expectedResult = {
+ "1": {
+ "1": { [DataPermission.VIEW_DATA]: DataPermissionValue.UNRESTRICTED },
+ },
+ "2": { "1": { [DataPermission.VIEW_DATA]: DataPermissionValue.NO } },
+ };
+
+ expect(mergeGroupsPermissionsUpdates(permissions, update)).toEqual(
+ expectedResult,
+ );
+ });
+});
diff --git a/frontend/src/metabase/plugins/index.ts b/frontend/src/metabase/plugins/index.ts
index c22d2adf22437e6bbd7213c9dd2ac6afe9318a69..8cf87c50255cc61b5a4870feaeba6dea90b92802 100644
--- a/frontend/src/metabase/plugins/index.ts
+++ b/frontend/src/metabase/plugins/index.ts
@@ -117,7 +117,7 @@ export const PLUGIN_ADMIN_PERMISSIONS_TABLE_FIELDS_POST_ACTION = {
export const PLUGIN_DATA_PERMISSIONS: {
permissionsPayloadExtraSelectors: ((
state: State,
- ) => Record<string, unknown>)[];
+ ) => [Record<string, undefined | { group_id: string }[]>, string[]])[];
hasChanges: ((state: State) => boolean)[];
shouldRestrictNativeQueryPermissions: (
permissions: GroupsPermissions,
diff --git a/src/metabase/api/permissions.clj b/src/metabase/api/permissions.clj
index accd691a90f690bdfbcae901d3474846239c45ef..df106a97604ec039947b4d728fc8d6c99906b792 100644
--- a/src/metabase/api/permissions.clj
+++ b/src/metabase/api/permissions.clj
@@ -101,9 +101,10 @@
(upsert-sandboxes! sandbox-updates))
impersonation-updates (:impersonations graph)
impersonations (when impersonation-updates
- (insert-impersonations! impersonation-updates))]
+ (insert-impersonations! impersonation-updates))
+ group-ids (-> graph :groups keys)]
(merge {:revision (perms-revision/latest-id)}
- (when-not skip-graph {:groups (:groups (data-perms.graph/api-graph {}))})
+ (when-not skip-graph {:groups (:groups (data-perms.graph/api-graph {:group-ids group-ids}))})
(when sandboxes {:sandboxes sandboxes})
(when impersonations {:impersonations impersonations}))))))
diff --git a/src/metabase/models/data_permissions.clj b/src/metabase/models/data_permissions.clj
index 62687367335c973b0971e85b0c6df288ede6b394..474e22ef04b063fce377af95b6d01185a363d21e 100644
--- a/src/metabase/models/data_permissions.clj
+++ b/src/metabase/models/data_permissions.clj
@@ -541,7 +541,7 @@
"Returns a tree representation of all data permissions. Can be optionally filtered by group ID, database ID,
and/or permission type. This is intended to power the permissions editor in the admin panel, and should not be used
for permission enforcement, as it will read much more data than necessary."
- [& {:keys [group-id db-id perm-type audit?]}]
+ [& {:keys [group-id group-ids db-id perm-type audit?]}]
(let [data-perms (t2/select [:model/DataPermissions
[:perm_type :type]
[:group_id :group-id]
@@ -553,6 +553,7 @@
(when perm-type [:= :perm_type (u/qualified-name perm-type)])
(when db-id [:= :db_id db-id])
(when group-id [:= :group_id group-id])
+ (when group-ids [:in :group_id group-ids])
(when-not audit? [:not= :db_id audit/audit-db-id])]})]
(reduce
(fn [graph {group-id :group-id
diff --git a/src/metabase/models/data_permissions/graph.clj b/src/metabase/models/data_permissions/graph.clj
index e9e0105c2c4c9cc2704763631826ea73a53f5265..5e28a307d6b928fe896360fa453c02b58104dea1 100644
--- a/src/metabase/models/data_permissions/graph.clj
+++ b/src/metabase/models/data_permissions/graph.clj
@@ -135,18 +135,22 @@
(defn- add-admin-perms-to-permissions-graph
"These are not stored in the data-permissions table, but the API expects them to be there (for legacy reasons), so here we populate it.
For every db in the incoming graph, adds on admin permissions."
- [api-graph {:keys [db-id group-id audit?]}]
+ [api-graph {:keys [db-id group-ids group-id audit?]}]
(let [admin-group-id (u/the-id (perms-group/admin))
db-ids (if db-id [db-id] (t2/select-pks-vec :model/Database
{:where [:and
(when-not audit? [:not= :id audit/audit-db-id])]}))]
- (if (and group-id (not= group-id admin-group-id))
- ;; Don't add admin perms when we're fetching the perms for a specific non-admin group
- api-graph
+ ;; Don't add admin perms when we're fetching the perms for a specific non-admin group or set of groups
+ (if (or (= group-id admin-group-id)
+ (contains? (set group-ids) admin-group-id)
+ ;; If we're not filtering on specific group IDs, always include the admin group
+ (and (nil? group-id)
+ (nil? (seq group-ids))))
(reduce (fn [api-graph db-id]
(assoc-in api-graph [admin-group-id db-id] admin-perms))
api-graph
- db-ids))))
+ db-ids)
+ api-graph)))
(defn remove-empty-vals
"Recursively walks a nested map from bottom-up, removing keys with nil or empty map values."
@@ -171,6 +175,7 @@
([& {:as opts}
:- [:map
[:group-id {:optional true} [:maybe pos-int?]]
+ [:group-ids {:optional true} [:maybe [:sequential pos-int?]]]
[:db-id {:optional true} [:maybe pos-int?]]
[:audit? {:optional true} [:maybe :boolean]]
[:perm-type {:optional true} [:maybe data-perms/PermissionType]]]]
@@ -425,7 +430,8 @@
"Takes an API-style perms graph and sets the permissions in the database accordingly. Additionally validates the revision number,
logs the changes, and ensures impersonations and sandboxes are consistent."
([new-graph :- api.permission-graph/StrictData]
- (let [old-graph (api-graph)
+ (let [group-ids (-> new-graph :groups keys)
+ old-graph (api-graph {:group-ids group-ids})
[old new] (data/diff (:groups old-graph) (:groups new-graph))
old (or old {})
new (or new {})]