From 3c82206d98dc67e69d232b68144402e91dc856d3 Mon Sep 17 00:00:00 2001
From: Ngoc Khuat <qn.khuat@gmail.com>
Date: Wed, 22 Jun 2022 18:04:20 +0700
Subject: [PATCH] Grant readwrite to root snippets for all users (#21940)
* Grant Snippets root collection for all users
* update perms path
* update cypress test
* Update repro for #17268
* remove trailing spaces
* don't run migration on existing EE instances
* fix migration failed in mysql and mariadb
* run migration for new instances only
Co-authored-by: Nemanja <31325167+nemanjaglumac@users.noreply.github.com>
---
.../snippets/snippet-permissions.cy.spec.js | 31 ++++++++----
.../native/snippets/snippets.cy.spec.js | 7 +--
resources/migrations/000_migrations.yaml | 30 +++++++++++-
test/metabase/db/schema_migrations_test.clj | 49 ++++++++++++++++++-
4 files changed, 98 insertions(+), 19 deletions(-)
diff --git a/frontend/test/metabase/scenarios/native/snippets/snippet-permissions.cy.spec.js b/frontend/test/metabase/scenarios/native/snippets/snippet-permissions.cy.spec.js
index 9dde85772f6..c26d8456965 100644
--- a/frontend/test/metabase/scenarios/native/snippets/snippet-permissions.cy.spec.js
+++ b/frontend/test/metabase/scenarios/native/snippets/snippet-permissions.cy.spec.js
@@ -6,13 +6,16 @@ import {
openNativeEditor,
} from "__support__/e2e/cypress";
+import { USER_GROUPS } from "__support__/e2e/cypress_data";
+
+const { ALL_USERS_GROUP } = USER_GROUPS;
+
describeEE("scenarios > question > snippets", () => {
beforeEach(() => {
restore();
});
- // Please uncomment "normal" user when #21581 gets fixed
- ["admin" /*"normal"*/].forEach(user => {
+ ["admin", "normal"].forEach(user => {
it(`${user} user can create a snippet (metabase#21581)`, () => {
cy.intercept("POST", "/api/native-query-snippet").as("snippetCreated");
@@ -119,6 +122,7 @@ describeEE("scenarios > question > snippets", () => {
openNativeEditor();
cy.icon("snippet").click();
+ // Edit permissions for a snippet folder
cy.findByTestId("sidebar-right").within(() => {
cy.findByText("Snippet Folder")
.next()
@@ -128,21 +132,21 @@ describeEE("scenarios > question > snippets", () => {
cy.findByText("Change permissions").click();
- // Update permissions for "All users"
+ // Update permissions for "All users" and let them only "View" this folder
modal().within(() => {
- cy.findByTestId("permission-table")
- .find(".Icon-close")
- .first()
+ getPermissionsForUserGroup("All Users")
+ .should("contain", "Curate")
.click();
});
- cy.findAllByRole("option")
+ popover()
.contains("View")
.click();
cy.button("Save").click();
cy.wait("@updatePermissions");
+ // Now let's do the sanity check for the top level (root) snippet permissions and make sure nothing changed there
cy.findByText("Snippets")
.parent()
.next()
@@ -152,16 +156,23 @@ describeEE("scenarios > question > snippets", () => {
// UI check
modal().within(() => {
- cy.icon("eye").should("not.exist");
+ getPermissionsForUserGroup("All Users").should("contain", "Curate");
});
// API check
cy.get("@updatePermissions").then(intercept => {
const { groups } = intercept.response.body;
- const allUsers = groups["1"];
+ const allUsers = groups[ALL_USERS_GROUP];
- expect(allUsers.root).to.equal("none");
+ expect(allUsers.root).to.equal("write");
});
});
});
});
+
+function getPermissionsForUserGroup(userGroup) {
+ return cy
+ .findByText(userGroup)
+ .closest("tr")
+ .find("[data-testid=permissions-select]");
+}
diff --git a/frontend/test/metabase/scenarios/native/snippets/snippets.cy.spec.js b/frontend/test/metabase/scenarios/native/snippets/snippets.cy.spec.js
index a3b4b726e34..f738bd6b1a1 100644
--- a/frontend/test/metabase/scenarios/native/snippets/snippets.cy.spec.js
+++ b/frontend/test/metabase/scenarios/native/snippets/snippets.cy.spec.js
@@ -12,15 +12,10 @@ function _clearAndIterativelyTypeUsingLabel(label, string) {
}
}
-// NOTE: - Had to change user role to "admin" on 2020-11-19.
-// - Normal users don't have permission to create/edit snippets in `ee` version.
-// - CI runs this test twice (both contexts), so it fails on `ee`.
-// - There is a related issue: https://github.com/metabase/metabase-enterprise/issues/543
-// TODO: Once the above issue is (re)solved, change back to `cy.signInAsNormalUser`
describe("scenarios > question > snippets", () => {
beforeEach(() => {
restore();
- cy.signInAsAdmin();
+ cy.signInAsNormalUser();
});
it("should let you create and use a snippet", () => {
diff --git a/resources/migrations/000_migrations.yaml b/resources/migrations/000_migrations.yaml
index 2b66f45e910..4e348e5a48e 100644
--- a/resources/migrations/000_migrations.yaml
+++ b/resources/migrations/000_migrations.yaml
@@ -11864,6 +11864,35 @@ databaseChangeLog:
columnName: last_name
columnDataType: varchar(254)
+ - changeSet:
+ id: v44.00-033
+ author: qnkhuat
+ comment: >-
+ Added 0.43.0. Grant the 'All Users' Permissions Group readwrite perms for the Root Snippets Collection.
+ preConditions:
+ - onFail: MARK_RAN
+ # HACK: only run this on new instances (#21940)
+ - sqlCheck:
+ expectedResult: 0
+ sql: >-
+ SELECT count(*) AS user_count FROM core_user
+ changes:
+ - sql:
+ sql: >-
+ INSERT INTO permissions (group_id, object)
+ SELECT
+ all_users_group.id AS group_id,
+ '/collection/namespace/snippets/root/' AS object
+ FROM (
+ SELECT id
+ FROM permissions_group
+ WHERE name = 'All Users'
+ ) all_users_group
+ LEFT JOIN permissions p
+ ON all_users_group.id = p.group_id
+ AND p.object = '/collection/namespace/snippets/root/'
+ WHERE p.object IS NULL;
+
- changeSet:
id: v44.00-038
author: metamben
@@ -11880,7 +11909,6 @@ databaseChangeLog:
constraints:
nullable: false
-
# >>>>>>>>>> DO NOT ADD NEW MIGRATIONS BELOW THIS LINE! ADD THEM ABOVE <<<<<<<<<<
########################################################################################################################
diff --git a/test/metabase/db/schema_migrations_test.clj b/test/metabase/db/schema_migrations_test.clj
index 159c7b59c3a..24f0b3bf790 100644
--- a/test/metabase/db/schema_migrations_test.clj
+++ b/test/metabase/db/schema_migrations_test.clj
@@ -15,10 +15,9 @@
[clojure.test :refer :all]
[metabase.db.schema-migrations-test.impl :as impl]
[metabase.driver :as driver]
- [metabase.models :refer [Card Collection Dashboard Database Field Permissions PermissionsGroup Pulse Setting Table]]
+ [metabase.models :refer [Card Collection Dashboard Database Field Permissions PermissionsGroup Pulse Setting Table User]]
[metabase.models.interface :as mi]
[metabase.models.permissions-group :as perms-group]
- [metabase.models.user :refer [User]]
[metabase.test.fixtures :as fixtures]
[metabase.test.util :as tu]
[metabase.util :as u]
@@ -622,3 +621,49 @@
(migrate!)
(is (= []
(:parameter_mappings (first (db/simple-select Card {:where [:= :id card-id]})))))))))
+
+(deftest grant-all-users-root-snippets-collection-readwrite-perms-test
+ (letfn [(perms-path [] "/collection/namespace/snippets/root/")
+ (all-users-group-id []
+ (-> (db/query {:select [:id], :from [PermissionsGroup],
+ :where [:= :name perms-group/all-users-group-name]})
+ first
+ :id))
+ (get-perms [] (map :name (db/query {:select [:pg.name]
+ :from [[Permissions :p]]
+ :left-join [[PermissionsGroup :pg] [:= :p.group_id :pg.id]]
+ :where [:= :p.object (perms-path)]})))]
+ (testing "Migration v44.00-033: create a Root Snippets Collection entry for All Users\n"
+ (testing "Should run for new OSS instances"
+ (impl/test-migrations ["v44.00-033" "v44.00-034"] [migrate!]
+ (migrate!)
+ (is (= ["All Users"] (get-perms)))))
+
+ (testing "Should run for new EE instances"
+ (impl/test-migrations ["v44.00-033" "v44.00-034"] [migrate!]
+ (db/simple-insert! Setting {:key "premium-embedding-token"
+ :value "fake-key"})
+ (migrate!)
+ (is (= ["All Users"] (get-perms)))))
+
+ (testing "Should not run for existing OSS instances"
+ (impl/test-migrations ["v44.00-033" "v44.00-034"] [migrate!]
+ (create-raw-user! "ngoc@metabase.com")
+ (migrate!)
+ (is (= [] (get-perms)))))
+
+ (testing "Should not run for existing EE instances"
+ (impl/test-migrations ["v44.00-033" "v44.00-034"] [migrate!]
+ (create-raw-user! "ngoc@metabase.com")
+ (db/simple-insert! Setting {:key "premium-embedding-token"
+ :value "fake-key"})
+ (migrate!)
+ (is (= [] (get-perms)))))
+
+ (testing "Should not fail if permissions already exist"
+ (impl/test-migrations ["v44.00-033" "v44.00-034"] [migrate!]
+ (db/execute! {:insert-into Permissions
+ :values [{:object (perms-path)
+ :group_id (all-users-group-id)}]})
+ (migrate!)
+ (is (= ["All Users"] (get-perms))))))))
--
GitLab