From d4365c8af4445fd31b1f23e7186a13e1e10cab42 Mon Sep 17 00:00:00 2001
From: Noah Moss <32746338+noahmoss@users.noreply.github.com>
Date: Wed, 12 Jul 2023 08:43:33 -0400
Subject: [PATCH] make connection impersonation work when acquiring a
connection via jdbc spec (#32238)
---
.../advanced_permissions/driver/impersonation.clj | 6 +++---
.../advanced_permissions/driver/impersonation_test.clj | 9 +++++++--
src/metabase/driver/sql_jdbc/execute.clj | 5 ++---
3 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/enterprise/backend/src/metabase_enterprise/advanced_permissions/driver/impersonation.clj b/enterprise/backend/src/metabase_enterprise/advanced_permissions/driver/impersonation.clj
index 82e762bee2f..edf086d0e1e 100644
--- a/enterprise/backend/src/metabase_enterprise/advanced_permissions/driver/impersonation.clj
+++ b/enterprise/backend/src/metabase_enterprise/advanced_permissions/driver/impersonation.clj
@@ -45,7 +45,7 @@
Returns `nil` if connection impersonation should not be used for the current user. Throws an exception if multiple
conflicting connection impersonation policies are found."
[database-or-id]
- (when-not api/*is-superuser?*
+ (when (and database-or-id (not api/*is-superuser?*))
(let [group-ids (t2/select-fn-set :group_id PermissionsGroupMembership :user_id api/*current-user-id*)
conn-impersonations (enforced-impersonations
(when (seq group-ids)
@@ -81,8 +81,8 @@
[driver ^Connection conn database]
(when (driver/database-supports? driver :connection-impersonation database)
(try
- (let [default-role (driver.sql/default-database-role driver database)
- impersonation-role (connection-impersonation-role database)]
+ (let [default-role (driver.sql/default-database-role driver database)
+ impersonation-role (connection-impersonation-role database)]
(driver/set-role! driver conn (or impersonation-role default-role)))
(catch Throwable e
(log/debug e (tru "Error setting role on connection"))
diff --git a/enterprise/backend/test/metabase_enterprise/advanced_permissions/driver/impersonation_test.clj b/enterprise/backend/test/metabase_enterprise/advanced_permissions/driver/impersonation_test.clj
index e0761563998..3064a013e9f 100644
--- a/enterprise/backend/test/metabase_enterprise/advanced_permissions/driver/impersonation_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/advanced_permissions/driver/impersonation_test.clj
@@ -50,8 +50,13 @@
(testing "Returns nil for superuser, even if they are in a group with an impersonation policy defined"
(advanced-perms.api.tu/with-impersonations {:impersonations [{:db-id (mt/id) :attribute "impersonation_attr"}]
:attributes {"impersonation_attr" "impersonation_role"}}
- (is (= "impersonation_role"
- (@#'impersonation/connection-impersonation-role (mt/db)))))))
+ (mw.session/as-admin
+ (is (nil? (@#'impersonation/connection-impersonation-role (mt/db)))))))
+
+ (testing "Does not throw an exception if passed a nil `database-or-id`"
+ (advanced-perms.api.tu/with-impersonations {:impersonations [{:db-id (mt/id) :attribute "impersonation_attr"}]
+ :attributes {"impersonation_attr" "impersonation_role"}}
+ (is (nil? (@#'impersonation/connection-impersonation-role nil))))))
(deftest conn-impersonation-test-postgres
(mt/test-driver :postgres
diff --git a/src/metabase/driver/sql_jdbc/execute.clj b/src/metabase/driver/sql_jdbc/execute.clj
index 77e8487e017..634c71c06b4 100644
--- a/src/metabase/driver/sql_jdbc/execute.clj
+++ b/src/metabase/driver/sql_jdbc/execute.clj
@@ -345,9 +345,8 @@
(log/tracef "Setting default connection options with options %s" (pr-str options))
(set-best-transaction-level! driver conn)
(set-time-zone-if-supported! driver conn session-timezone)
- (set-role-if-supported! driver conn (when (u/id db-or-id-or-spec) (if (integer? db-or-id-or-spec)
- (t2/select-one Database db-or-id-or-spec)
- db-or-id-or-spec)))
+ (set-role-if-supported! driver conn (cond (integer? db-or-id-or-spec) (t2/select-one Database db-or-id-or-spec)
+ (u/id db-or-id-or-spec) db-or-id-or-spec))
(let [read-only? (not write?)]
(try
;; Setting the connection to read-only does not prevent writes on some databases, and is meant
--
GitLab