Merge pull request #5178 from metabase/fix-permissions-page-for-schemaless-dbs

Fix permissions page for dbs that don't have schemas

frontend/src/metabase/admin/permissions/selectors.js

@@ -147,7 +147,7 @@ function getRevokingAccessToAllTablesWarningModal(database, permissions, groupId
         // allTableEntityIds contains tables from all schemas
         const allTableEntityIds = database.tables().map((table) => ({
             databaseId: table.db_id,
-            schemaName: table.schema,
+            schemaName: table.schema || "",
             tableId: table.id
         }));
 
@@ -457,7 +457,7 @@ export const getDatabasesPermissionsGrid = createSelector(
 
 const getCollections = (state) => state.admin.permissions.collections;
 const getCollectionPermission = (permissions, groupId, { collectionId }) =>
-    getIn(permissions, [groupId, collectionId])
+    getIn(permissions, [groupId, collectionId]);
 
 export const getCollectionsPermissionsGrid = createSelector(
     getCollections, getGroups, getPermissions,
@@ -505,7 +505,6 @@ export const getCollectionsPermissionsGrid = createSelector(
     }
 );
 
-
 export const getDiff = createSelector(
     getMeta, getGroups, getPermissions, getOriginalPermissions,
     (metadata: Metadata, groups: Array<Group>, permissions: GroupsPermissions, originalPermissions: GroupsPermissions) =>

frontend/src/metabase/admin/permissions/selectors.spec.fixtures.js

+import {getMetadata} from "metabase/selectors/metadata";
+// Database 2 contains an imaginary multi-schema database (like Redshift for instance)
+// Database 3 contains an imaginary database which doesn't have any schemas (like MySQL)
+// (A single-schema database was originally Database 1 but it got removed as testing against it felt redundant)
+
+export const normalizedMetadata = {
+    "metrics": {},
+    "segments": {},
+    "databases": {
+        "2": {
+            "name": "Imaginary Multi-Schema Dataset",
+            "tables": [
+                // In schema_1
+                5,
+                6,
+                // In schema_2
+                7,
+                8,
+                9
+            ],
+            "id": 2
+        },
+        "3": {
+            "name": "Imaginary Schemaless Dataset",
+            "tables": [
+                10,
+                11,
+                12,
+                13
+            ],
+            "id": 3
+        }
+    },
+    "tables": {
+        "5": {
+            "schema": "schema_1",
+            "name": "Avian Singles Messages",
+            "id": 5,
+            "db_id": 2
+        },
+        "6": {
+            "schema": "schema_1",
+            "name": "Avian Singles Users",
+            "id": 6,
+            "db_id": 2
+        },
+        "7": {
+            "schema": "schema_2",
+            "name": "Tupac Sightings Sightings",
+            "id": 7,
+            "db_id": 2
+        },
+        "8": {
+            "schema": "schema_2",
+            "name": "Tupac Sightings Categories",
+            "id": 8,
+            "db_id": 2
+        },
+        "9": {
+            "schema": "schema_2",
+            "name": "Tupac Sightings Cities",
+            "id": 9,
+            "db_id": 2
+        },
+        "10": {
+            "schema": null,
+            "name": "Badminton Men's Double Results",
+            "id": 10,
+            "db_id": 3
+        },
+        "11": {
+            "schema": null,
+            "name": "Badminton Mixed Double Results",
+            "id": 11,
+            "db_id": 3
+        },
+        "12": {
+            "schema": null,
+            "name": "Badminton Women's Singles Results",
+            "id": 12,
+            "db_id": 3
+        },
+        "13": {
+            "schema": null,
+            "name": "Badminton Mixed Singles Results",
+            "id": 13,
+            "db_id": 3
+        },
+    },
+    "fields": {/* stripped out */},
+    "revisions": {},
+    "databasesList": [2, 3]
+};
+
+export const denormalizedMetadata = getMetadata({metadata: normalizedMetadata});

frontend/src/metabase/admin/permissions/selectors.spec.js

+/**
+ * Tests granting and revoking permissions against three kinds of datasets:
+ * - dataset with tables in a single PUBLIC schema (for instance H2 or PostgreSQL if no custom schemas created)
+ * - dataset with no schemas (for instance MySQL)
+ * - dataset with multiple schemas (for instance Redshift)
+ */
+
+import { setIn } from "icepick";
+
+jest.mock('metabase/lib/analytics');
+
+import {GroupsPermissions} from "metabase/meta/types/Permissions";
+import { denormalizedMetadata } from "./selectors.spec.fixtures";
+import { getTablesPermissionsGrid, getSchemasPermissionsGrid, getDatabasesPermissionsGrid } from "./selectors";
+
+/******** INITIAL TEST STATE ********/
+
+const groups = [{
+    id: 1,
+    name: "Group starting with full access",
+}, {
+    id: 2,
+    name: "Group starting with no access at all",
+}];
+
+const initialPermissions: GroupsPermissions = {
+    1: {
+        // Sample dataset
+        1: {
+            "native": "write",
+            "schemas": "all"
+        },
+        // Imaginary multi-schema
+        2: {
+            "native": "write",
+            "schemas": "all"
+        },
+        // Imaginary schemaless
+        3: {
+            "native": "write",
+            "schemas": "all"
+        }
+    },
+    2: {
+        // Sample dataset
+        1: {
+            "native": "none",
+            "schemas": "none"
+        },
+        // Imaginary multi-schema
+        2: {
+            "native": "none",
+            "schemas": "none"
+        },
+        // Imaginary schemaless
+        3: {
+            "native": "none",
+            "schemas": "none"
+        }
+    }
+};
+
+
+/******** MANAGING THE CURRENT (SIMULATED) STATE TREE ********/
+
+const initialState = {
+    admin: {
+        permissions: {
+            permissions: initialPermissions,
+            originalPermissions: initialPermissions,
+            groups,
+            databases: denormalizedMetadata.databases
+        }
+    }
+};
+
+var state = initialState;
+const resetState = () => { state = initialState };
+const getPermissionsTree = () => state.admin.permissions.permissions;
+const getPermissionsForDb = ({ entityId, groupId }) => getPermissionsTree()[groupId][entityId.databaseId];
+
+const updatePermissionsInState = (permissions) => {
+    state = setIn(state, ["admin", "permissions", "permissions"], permissions);
+};
+
+const getProps = ({ databaseId, schemaName }) => ({
+    params: {
+        databaseId,
+            schemaName
+    }
+});
+
+/******** HIGH-LEVEL METHODS FOR UPDATING PERMISSIONS ********/
+
+const changePermissionsForEntityInGrid = ({ grid, category, entityId, groupId, permission }) => {
+    const newPermissions = grid.permissions[category].updater(groupId, entityId, permission);
+    updatePermissionsInState(newPermissions);
+    return newPermissions;
+};
+
+const changeDbNativePermissionsForEntity = ({ entityId, groupId, permission }) => {
+    const grid = getDatabasesPermissionsGrid(state, getProps(entityId));
+    return changePermissionsForEntityInGrid({ grid, category: "native", entityId, groupId, permission });
+};
+
+const changeDbDataPermissionsForEntity = ({ entityId, groupId, permission }) => {
+    const grid = getDatabasesPermissionsGrid(state, getProps(entityId));
+    return changePermissionsForEntityInGrid({ grid, category: "schemas", entityId, groupId, permission });
+};
+
+const changeSchemaPermissionsForEntity = ({ entityId, groupId, permission }) => {
+    const grid = getSchemasPermissionsGrid(state, getProps(entityId));
+    return changePermissionsForEntityInGrid({ grid, category: "tables", entityId, groupId, permission });
+};
+
+const changeTablePermissionsForEntity = ({ entityId, groupId, permission }) => {
+    const grid = getTablesPermissionsGrid(state, getProps(entityId));
+    return changePermissionsForEntityInGrid({ grid, category: "fields", entityId, groupId, permission });
+};
+
+const getMethodsForDbAndSchema = (entityId) => ({
+    changeDbNativePermissions: ({ groupId, permission }) =>
+        changeDbNativePermissionsForEntity({ entityId, groupId, permission }),
+    changeDbDataPermissions: ({ groupId, permission }) =>
+        changeDbDataPermissionsForEntity({ entityId, groupId, permission }),
+    changeSchemaPermissions: ({ groupId, permission }) =>
+        changeSchemaPermissionsForEntity({ entityId, groupId, permission }),
+    changeTablePermissions: ({ tableId, groupId, permission }) =>
+        changeTablePermissionsForEntity({ entityId: {...entityId, tableId}, groupId, permission }),
+    getPermissions: ({ groupId }) =>
+        getPermissionsForDb({ entityId, groupId })
+});
+
+/******** ACTUAL TESTS ********/
+
+describe("permissions selectors", () => {
+    beforeEach(resetState);
+
+    describe("for a schemaless dataset", () => {
+        // Schema "name" (better description would be a "permission path identifier") is simply an empty string
+        // for databases where the metadata value for table schema is `null`
+        const schemalessDataset = getMethodsForDbAndSchema({ databaseId: 3, schemaName: "" });
+
+        it("should restrict access correctly on table level", () => {
+            // Revoking access to one table should downgrade the native permissions to "read"
+            schemalessDataset.changeTablePermissions({ tableId: 10, groupId: 1, permission: "none" });
+            expect(schemalessDataset.getPermissions({ groupId: 1})).toMatchObject({
+                "native": "read",
+                "schemas": {
+                    "": {
+                        "10": "none",
+                        "11": "all",
+                        "12": "all",
+                        "13": "all"
+                    }
+                }
+            });
+
+            // Revoking access to the rest of tables one-by-one...
+            schemalessDataset.changeTablePermissions({ tableId: 11, groupId: 1, permission: "none" });
+            schemalessDataset.changeTablePermissions({ tableId: 12, groupId: 1, permission: "none" });
+            schemalessDataset.changeTablePermissions({ tableId: 13, groupId: 1, permission: "none" });
+
+            expect(schemalessDataset.getPermissions({groupId: 1})).toMatchObject({
+                // ...should revoke all permissions for that database
+                "native": "none",
+                "schemas": "none"
+            });
+
+        });
+
+        it("should restrict access correctly on db level", () => {
+            // Should let change the native permission to "read"
+            schemalessDataset.changeDbNativePermissions({ groupId: 1, permission: "read" });
+            expect(schemalessDataset.getPermissions({groupId: 1})).toMatchObject({
+                "native": "read",
+                "schemas": "all"
+            });
+
+            // Should not let change the native permission to none
+            schemalessDataset.changeDbNativePermissions({ groupId: 1, permission: "none" });
+            expect(schemalessDataset.getPermissions({groupId: 1})).toMatchObject({
+                "native": "none",
+                "schemas": "all"
+            });
+
+            resetState(); // ad-hoc state reset for the next test
+            // Revoking the data access to the database at once should revoke all permissions for that database
+            schemalessDataset.changeDbDataPermissions({ groupId: 1, permission: "none" });
+            expect(schemalessDataset.getPermissions({groupId: 1})).toMatchObject({
+                "native": "none",
+                "schemas": "none"
+            });
+        });
+
+        it("should grant more access correctly on table level", () => {
+            // Simply grant an access to a single table
+            schemalessDataset.changeTablePermissions({ tableId: 12, groupId: 2, permission: "all" });
+            expect(schemalessDataset.getPermissions({groupId: 2})).toMatchObject({
+                "native": "none",
+                "schemas": {
+                    "": {
+                        "10": "none",
+                        "11": "none",
+                        "12": "all",
+                        "13": "none"
+                    }
+                }
+            });
+
+            // Grant the access to rest of tables
+            schemalessDataset.changeTablePermissions({ tableId: 10, groupId: 2, permission: "all" });
+            schemalessDataset.changeTablePermissions({ tableId: 11, groupId: 2, permission: "all" });
+            schemalessDataset.changeTablePermissions({ tableId: 13, groupId: 2, permission: "all" });
+            expect(schemalessDataset.getPermissions({groupId: 2})).toMatchObject({
+                "native": "none",
+                "schemas": "all"
+            });
+
+
+            // Should pass changes to native permissions through
+            schemalessDataset.changeDbNativePermissions({ groupId: 2, permission: "read" });
+            expect(schemalessDataset.getPermissions({ groupId: 2 })).toMatchObject({
+                "native": "read",
+                "schemas": "all"
+            });
+        });
+
+        it("should grant more access correctly on db level", () => {
+            // Setting limited access should produce a permission tree where each schema has "none" access
+            // (this is a strange, rather no-op edge case but the UI currently enables this)
+            schemalessDataset.changeDbDataPermissions({ groupId: 2, permission: "controlled" });
+            expect(schemalessDataset.getPermissions({ groupId: 2 })).toMatchObject({
+                "native": "none",
+                "schemas": {
+                    "": "none"
+                }
+            });
+
+            // Granting native access should also grant a full write access
+            schemalessDataset.changeDbNativePermissions({ groupId: 2, permission: "write" });
+            expect(schemalessDataset.getPermissions({ groupId: 2 })).toMatchObject({
+                "native": "write",
+                "schemas": "all"
+            });
+
+            resetState(); // ad-hoc reset (normally run before tests)
+            // test that setting full access works too
+            schemalessDataset.changeDbDataPermissions({ groupId: 2, permission: "all" });
+            expect(schemalessDataset.getPermissions({ groupId: 2 })).toMatchObject({
+                "native": "none",
+                "schemas": "all"
+            });
+        })
+    });
+
+    describe("for a dataset with multiple schemas", () => {
+        const schema1 = getMethodsForDbAndSchema({ databaseId: 2, schemaName: "schema_1" });
+        const schema2 = getMethodsForDbAndSchema({ databaseId: 2, schemaName: "schema_2" });
+
+        it("should restrict access correctly on table level", () => {
+            // Revoking access to one table should downgrade the native permissions to "read"
+            schema1.changeTablePermissions({ tableId: 5, groupId: 1, permission: "none" });
+            expect(schema1.getPermissions({ groupId: 1})).toMatchObject({
+                "native": "read",
+                "schemas": {
+                    "schema_1": {
+                        "5": "none",
+                        "6": "all"
+                    },
+                    "schema_2": "all"
+                }
+            });
+
+            // State where both schemas have mixed permissions
+            schema2.changeTablePermissions({ tableId: 8, groupId: 1, permission: "none" });
+            schema2.changeTablePermissions({ tableId: 9, groupId: 1, permission: "none" });
+            expect(schema2.getPermissions({groupId: 1})).toMatchObject({
+                "native": "read",
+                "schemas": {
+                    "schema_1": {
+                        "5": "none",
+                        "6": "all"
+                    },
+                    "schema_2": {
+                        "7": "all",
+                        "8": "none",
+                        "9": "none"
+                    }
+                }
+            });
+
+            // Completely revoke access to the first schema with table-level changes
+            schema1.changeTablePermissions({ tableId: 6, groupId: 1, permission: "none" });
+
+            expect(schema1.getPermissions({groupId: 1})).toMatchObject({
+                "native": "read",
+                "schemas": {
+                    "schema_1": "none",
+                    "schema_2": {
+                        "7": "all",
+                        "8": "none",
+                        "9": "none"
+                    }
+                }
+            });
+
+            // Revoking all permissions of the other schema should revoke all db permissions too
+            schema2.changeTablePermissions({ tableId: 7, groupId: 1, permission: "none" });
+            expect(schema2.getPermissions({groupId: 1})).toMatchObject({
+                "native": "none",
+                "schemas": "none"
+            });
+        });
+
+        it("should restrict access correctly on schema level", () => {
+            // Revoking access to one schema
+            schema2.changeSchemaPermissions({ groupId: 1, permission: "none" });
+            expect(schema2.getPermissions({groupId: 1})).toMatchObject({
+                "native": "read",
+                "schemas": {
+                    "schema_1": "all",
+                    "schema_2": "none"
+                }
+            });
+
+            // Revoking access to other too
+            schema1.changeSchemaPermissions({ groupId: 1, permission: "none" });
+            expect(schema1.getPermissions({groupId: 1})).toMatchObject({
+                "native": "none",
+                "schemas": "none"
+            });
+        });
+
+        it("should restrict access correctly on db level", () => {
+            // Should let change the native permission to "read"
+            schema1.changeDbNativePermissions({ groupId: 1, permission: "read" });
+            expect(schema1.getPermissions({groupId: 1})).toMatchObject({
+                "native": "read",
+                "schemas": "all"
+            });
+
+            // Should let change the native permission to none
+            schema1.changeDbNativePermissions({ groupId: 1, permission: "none" });
+            expect(schema1.getPermissions({groupId: 1})).toMatchObject({
+                "native": "none",
+                "schemas": "all"
+            });
+
+            resetState(); // ad-hoc state reset for the next test
+            // Revoking the data access to the database at once should revoke all permissions for that database
+            schema1.changeDbDataPermissions({ groupId: 1, permission: "none" });
+            expect(schema1.getPermissions({groupId: 1})).toMatchObject({
+                "native": "none",
+                "schemas": "none"
+            });
+        });
+
+        it("should grant more access correctly on table level", () => {
+            // Simply grant an access to a single table
+            schema2.changeTablePermissions({ tableId: 7, groupId: 2, permission: "all" });
+            expect(schema2.getPermissions({groupId: 2})).toMatchObject({
+                "native": "none",
+                "schemas": {
+                    "schema_1": "none",
+                    "schema_2": {
+                        "7": "all",
+                        "8": "none",
+                        "9": "none"
+                    }
+                }
+            });
+
+            // State where both schemas have mixed permissions
+            schema1.changeTablePermissions({ tableId: 5, groupId: 2, permission: "all" });
+            expect(schema1.getPermissions({groupId: 2})).toMatchObject({
+                "native": "none",
+                "schemas": {
+                    "schema_1": {
+                        "5": "all",
+                        "6": "none"
+                    },
+                    "schema_2": {
+                        "7": "all",
+                        "8": "none",
+                        "9": "none"
+                    }
+                }
+            });
+
+            // Grant full access to the second schema
+            schema2.changeTablePermissions({ tableId: 8, groupId: 2, permission: "all" });
+            schema2.changeTablePermissions({ tableId: 9, groupId: 2, permission: "all" });
+            expect(schema2.getPermissions({groupId: 2})).toMatchObject({
+                "native": "none",
+                "schemas": {
+                    "schema_1": {
+                        "5": "all",
+                        "6": "none"
+                    },
+                    "schema_2": "all"
+                }
+            });
+
+            // Grant the access to whole db (no native yet)
+            schema1.changeTablePermissions({ tableId: 5, groupId: 2, permission: "all" });
+            schema1.changeTablePermissions({ tableId: 6, groupId: 2, permission: "all" });
+            expect(schema1.getPermissions({groupId: 2})).toMatchObject({
+                "native": "none",
+                "schemas": "all"
+            });
+
+            // Should pass changes to native permissions through
+            schema1.changeDbNativePermissions({ groupId: 2, permission: "read" });
+            expect(schema1.getPermissions({ groupId: 2 })).toMatchObject({
+                "native": "read",
+                "schemas": "all"
+            });
+        });
+
+        it("should grant more access correctly on schema level", () => {
+            // Granting full access to one schema
+            schema1.changeSchemaPermissions({ groupId: 2, permission: "all" });
+            expect(schema1.getPermissions({groupId: 2})).toMatchObject({
+                "native": "none",
+                "schemas": {
+                    "schema_1": "all",
+                    "schema_2": "none"
+                }
+            });
+
+            // Granting access to the other as well
+            schema2.changeSchemaPermissions({ groupId: 2, permission: "all" });
+            expect(schema2.getPermissions({groupId: 2})).toMatchObject({
+                "native": "none",
+                "schemas": "all"
+            });
+        });
+
+        it("should grant more access correctly on db level", () => {
+            // Setting limited access should produce a permission tree where each schema has "none" access
+            // (this is a strange, rather no-op edge case but the UI currently enables this)
+            schema1.changeDbDataPermissions({ groupId: 2, permission: "controlled" });
+            expect(schema1.getPermissions({ groupId: 2 })).toMatchObject({
+                "native": "none",
+                "schemas": {
+                    "schema_1": "none",
+                    "schema_2": "none"
+                }
+            });
+
+            // Granting native access should also grant a full write access
+            schema1.changeDbNativePermissions({ groupId: 2, permission: "write" });
+            expect(schema1.getPermissions({ groupId: 2 })).toMatchObject({
+                "native": "write",
+                "schemas": "all"
+            });
+
+            resetState(); // ad-hoc reset (normally run before tests)
+            // test that setting full access works too
+            schema1.changeDbDataPermissions({ groupId: 2, permission: "all" });
+            expect(schema1.getPermissions({ groupId: 2 })).toMatchObject({
+                "native": "none",
+                "schemas": "all"
+            });
+        })
+    });
+});

frontend/src/metabase/lib/permissions.js

@@ -109,13 +109,15 @@ export function downgradeNativePermissionsIfNeeded(permissions: GroupsPermission
     }
 }
 
-// $FlowFixMe
-const metadataTableToTableEntityId = (table: Table): TableEntityId => ({ databaseId: table.db_id, schemaName: table.schema, tableId: table.id });
+const metadataTableToTableEntityId = (table: Table): TableEntityId => ({ databaseId: table.db_id, schemaName: table.schema || "", tableId: table.id });
+
+// TODO Atte Keinänen 6/24/17 See if this method could be simplified
 const entityIdToMetadataTableFields = (entityId: EntityId) => ({
     ...(entityId.databaseId ? {db_id: entityId.databaseId} : {}),
-    ...(entityId.schemaName ? {schema: entityId.schemaName} : {}),
-    ...(entityId.tableId ? {tableId: entityId.tableId} : {})
-})
+    // $FlowFixMe Because schema name can be an empty string, which means an empty schema, this check becomes a little nasty
+    ...(entityId.schemaName !== undefined ? {schema: entityId.schemaName !== "" ? entityId.schemaName : null} : {}),
+    ...(entityId.tableId ? {id: entityId.tableId} : {})
+});
 
 function inferEntityPermissionValueFromChildTables(permissions: GroupsPermissions, groupId: GroupId, entityId: DatabaseEntityId|SchemaEntityId, metadata: Metadata) {
     const { databaseId } = entityId;
@@ -177,7 +179,7 @@ export function updateFieldsPermission(permissions: GroupsPermissions, groupId:
 
 export function updateTablesPermission(permissions: GroupsPermissions, groupId: GroupId, { databaseId, schemaName }: SchemaEntityId, value: string, metadata: Metadata): GroupsPermissions {
     const database = metadata && metadata.database(databaseId);
-    const tableIds: ?number[] = database && database.tables().filter(t => t.schema === schemaName).map(t => t.id);
+    const tableIds: ?number[] = database && database.tables().filter(t => (t.schema || "") === schemaName).map(t => t.id);
 
     permissions = updateSchemasPermission(permissions, groupId, { databaseId }, "controlled", metadata);
     permissions = updatePermission(permissions, groupId, [databaseId, "schemas", schemaName], value, tableIds);
@@ -186,11 +188,12 @@ export function updateTablesPermission(permissions: GroupsPermissions, groupId:
 }
 
 export function updateSchemasPermission(permissions: GroupsPermissions, groupId: GroupId, { databaseId }: DatabaseEntityId, value: string, metadata: Metadata): GroupsPermissions {
-    let database = metadata.database(databaseId);
-    let schemaNames = database && database.schemaNames();
+    const database = metadata.database(databaseId);
+    const schemaNames = database && database.schemaNames();
+    const schemaNamesOrNoSchema = (schemaNames && schemaNames.length > 0) ? schemaNames : [""];
 
     permissions = downgradeNativePermissionsIfNeeded(permissions, groupId, { databaseId }, value, metadata);
-    return updatePermission(permissions, groupId, [databaseId, "schemas"], value, schemaNames);
+    return updatePermission(permissions, groupId, [databaseId, "schemas"], value, schemaNamesOrNoSchema);
 }
 
 export function updateNativePermission(permissions: GroupsPermissions, groupId: GroupId, { databaseId }: DatabaseEntityId, value: string, metadata: Metadata): GroupsPermissions {