Add role to spec even if used in `:connection-uri` (#44397)

This PR removes `:role` removal from spec, even if it was added in `:connection-uri`.
It seems that `:connection-uri` is ignored on `do-with-connection-options` code path.
Hence use of non-default roles is again available on Snowflake.

.github/workflows/drivers.yml

@@ -809,6 +809,11 @@ jobs:
       MB_SNOWFLAKE_TEST_WAREHOUSE: ${{ secrets.MB_SNOWFLAKE_TEST_WAREHOUSE }}
       MB_SNOWFLAKE_TEST_PK_USER: METABASE PK
       MB_SNOWFLAKE_TEST_PK_PRIVATE_KEY: ${{ secrets.MB_SNOWFLAKE_TEST_PK_PRIVATE_KEY }}
+      # RSA Role testing:
+      MB_SNOWFLAKE_TEST_RSA_ROLE_TEST_DEFAULT_USER: RSA_ROLE_TEST_DEFAULT_USER
+      MB_SNOWFLAKE_TEST_RSA_ROLE_TEST_CUSTOM_USER: RSA_ROLE_TEST_CUSTOM_USER
+      MB_SNOWFLAKE_TEST_RSA_ROLE_TEST_ROLE: RSA_ROLE_TEST_ROLE
+      MB_SNOWFLAKE_TEST_RSA_ROLE_TEST_DB: RSA_ROLE_TEST_DB
     steps:
     - uses: actions/checkout@v4
     - name: Test Snowflake driver

modules/drivers/snowflake/src/metabase/driver/snowflake.clj

@@ -145,9 +145,6 @@
            (not (contains? (connection-str->parameters (:connection-uri spec)) "ROLE")))
     (let [role-opts-str (sql-jdbc.common/additional-opts->string :url {:role (codec/url-encode (:role details))})]
       (-> spec
-          ;; It is advised to use either connection property or url parameter, not both. Eg. in the following link.
-          ;; https://docs.oracle.com/javase/8/docs/api/java/sql/DriverManager.html#getConnection-java.lang.String-java.util.Properties-
-          (dissoc :role)
           (update :connection-uri sql-jdbc.common/conn-str-with-additional-opts :url role-opts-str)))
     spec))
 

modules/drivers/snowflake/test/metabase/driver/snowflake_test.clj

@@ -411,27 +411,114 @@
                                   (merge {:db pk-db :user pk-user} to-merge))]
                   (is (can-connect? details)))))))))))
 
-(deftest can-connect-pk-no-options-test
+(deftest ^:synchronized pk-auth-custom-role-e2e-test
   (mt/test-driver
    :snowflake
-   (testing "Can connect with base64 encoded `:private-key-value` and no `:private-key-options` set (#41852)"
-     (let [pk-key (format-env-key (tx/db-test-env-var-or-throw :snowflake :pk-private-key))
-           pk-user (tx/db-test-env-var :snowflake :pk-user)
-           pk-db "SNOWFLAKE_SAMPLE_DATA"
-           details (-> (:details (mt/db))
-                       (dissoc :password)
-                       (assoc :dbname pk-db
-                              :private-key-value (u/encode-base64 pk-key)
-                              :user pk-user))]
-       (testing "Can connect with base64 encoded `:private-key-value` and no `:private-key-options` set (#41852)"
-         (is (driver/can-connect? :snowflake details)))
-       ;; Following is required when private key and role are used together. See
-       ;; the [[metabase.driver.snowflake/maybe-add-role-to-spec-url]] for the details.
-       (testing "Role is added to connection url, if url is present (#43600)"
-         (let [details-with-role (assoc details :role "SOME_ROLE")
-               conn-str (:connection-uri (sql-jdbc.conn/connection-details->spec :snowflake details-with-role))
-               parsed-params (driver.snowflake/connection-str->parameters conn-str)]
-           (is (= "SOME_ROLE" (get parsed-params "ROLE")))))))))
+   (let [account           (tx/db-test-env-var-or-throw :snowflake :account)
+         warehouse         (tx/db-test-env-var-or-throw :snowflake :warehouse)
+         ;; User with default role PULIC. To access the db custom role has to be used.
+         user              (tx/db-test-env-var-or-throw :snowflake :rsa-role-test-custom-user)
+         private-key-value (format-env-key (tx/db-test-env-var-or-throw :snowflake :pk-private-key))
+         db                (tx/db-test-env-var-or-throw :snowflake :rsa-role-test-db)
+         database          {:name    "Snowflake RSA test DB custom"
+                            :engine  :snowflake
+                            ;; Details as collected from `api handler POST / database` are used.
+                            :details {:role                nil
+                                      :warehouse           warehouse
+                                      :db                  db
+                                      :password            nil
+                                      :private-key-options "uploaded"
+                                      :advanced-options    false
+                                      :schema-filters-type "all"
+                                      :account             account
+                                      :private-key-value   (str "data:application/octet-stream;base64,"
+                                                                (u/encode-base64 private-key-value))
+                                      :tunnel-enabled      false
+                                      :user                user}}]
+     ;; TODO: We should make those message returned when role is incorrect more descriptive!
+     (testing "Database can not be accessed with `nil` default role"
+       (is (= "Looks like the Database name is incorrect."
+              (:message (mt/user-http-request :crowberto :post 400 "database"
+                                              database)))))
+     (testing "Database can not be accessed with PUBLIC role (default)"
+       (is (= "Looks like the Database name is incorrect."
+              (:message (mt/user-http-request :crowberto :post 400 "database"
+                                              (assoc-in database [:details :role] "PUBLIC"))))))
+     (testing "Database can be created using specified role"
+       ;; Map containing :details is expected to be database, hence considering request successful.
+       (is (contains? (mt/user-http-request :crowberto :post 200 "database"
+                                            (assoc-in database [:details :role]
+                                                      (tx/db-test-env-var-or-throw :snowflake :rsa-role-test-role)))
+                      :details))
+        ;; As the request is asynchronous, wait for sync to complete.
+       (Thread/sleep 7000))
+     (let [[db :as dbs]       (t2/select :model/Database :name "Snowflake RSA test DB custom")
+           [table :as tables] (t2/select :model/Table :db_id (:id db))
+           fields             (t2/select :model/Field :table_id (:id table))]
+       (testing "Created database is correctly synced"
+         (testing "Application database contains one database, one table and one new field"
+           (is (= 1 (count dbs)))
+           (is (= 1 (count tables)))
+           (is (= 2 (count fields)))))
+       (testing "Querying the database returns expected results"
+         (is (= [[1 "John Toucan Smith"]]
+                (mt/rows (qp/process-query {:database (:id db)
+                                            :type :query
+                                            :query {:source-table (:id table)}})))))
+       ;; Cleanup
+       (u/ignore-exceptions (t2/delete! :model/Database (:id db)))
+       (u/ignore-exceptions (t2/delete! :model/Table (:id table)))
+       (u/ignore-exceptions (t2/delete! :model/Field :id [:in (map :id fields)]))
+       (u/ignore-exceptions (t2/delete! :model/FieldValues :field_id [:in (map :id fields)]))))))
+
+(deftest ^:synchronized pk-auth-default-role-e2e-test
+  (mt/test-driver
+   :snowflake
+   (let [account           (tx/db-test-env-var-or-throw :snowflake :account)
+         warehouse         (tx/db-test-env-var-or-throw :snowflake :warehouse)
+         ;; User with default role PULIC. To access the db custom role has to be used.
+         user              (tx/db-test-env-var-or-throw :snowflake :rsa-role-test-default-user)
+         private-key-value (format-env-key (tx/db-test-env-var-or-throw :snowflake :pk-private-key))
+         db                (tx/db-test-env-var-or-throw :snowflake :rsa-role-test-db)
+         database          {:name    "Snowflake RSA test DB default"
+                            :engine  :snowflake
+                            ;; Details as collected from `api handler POST / database` are used.
+                            :details {:role                nil
+                                      :warehouse           warehouse
+                                      :db                  db
+                                      :password            nil
+                                      :private-key-options "uploaded"
+                                      :advanced-options    false
+                                      :schema-filters-type "all"
+                                      :account             account
+                                      :private-key-value   (str "data:application/octet-stream;base64,"
+                                                                (u/encode-base64 private-key-value))
+                                      :tunnel-enabled      false
+                                      :user                user}}]
+     (testing "Database can be created using _default_ `nil` role"
+       ;; Map containing :details is expected to be database, hence considering request successful.
+       (is (contains? (mt/user-http-request :crowberto :post 200 "database" database)
+                      :details))
+        ;; As the request is asynchronous, wait for sync to complete.
+       (Thread/sleep 7000))
+     (let [[db :as dbs]       (t2/select :model/Database :name "Snowflake RSA test DB default")
+           [table :as tables] (t2/select :model/Table :db_id (:id db))
+           fields             (t2/select :model/Field :table_id (:id table))]
+       (testing "Created database is correctly synced"
+         (testing "Application database contains one database, one table and one new field"
+           (is (= 1 (count dbs)))
+           (is (= 1 (count tables)))
+           (is (= 2 (count fields)))))
+       (testing "Querying the database returns expected results"
+         (is (= [[1 "John Toucan Smith"]]
+                (mt/rows (qp/process-query {:database (:id db)
+                                            :type :query
+                                            :query {:source-table (:id table)}})))))
+       ;; Cleanup
+       (u/ignore-exceptions (t2/delete! :model/Database (:id db)))
+       (u/ignore-exceptions (t2/delete! :model/Table (:id table)))
+       (u/ignore-exceptions (t2/delete! :model/Field :id [:in (map :id fields)]))
+       (u/ignore-exceptions (t2/delete! :model/FieldValues :field_id [:in (map :id fields)]))))))
 
 (deftest ^:parallel replacement-snippet-date-param-test
   (mt/test-driver :snowflake