From 84f932b5b690e56d30816b5d2bd6fa522a1cf1d7 Mon Sep 17 00:00:00 2001
From: metamben <103100869+metamben@users.noreply.github.com>
Date: Fri, 26 Jul 2024 00:30:11 +0300
Subject: [PATCH] Add Azure managed identity support (#46011)
Add Azure managed identity support
---------
Co-authored-by: Case Nelson <case@metabase.com>
---
.clj-kondo/config.edn | 1 +
docs/developers-guide/driver-changelog.md | 6 +
...atabaseAuthProviderSectionField.styled.tsx | 14 +++
.../DatabaseAuthProviderSectionField.tsx | 32 ++++++
.../DatabaseAuthProviderSectionField/index.ts | 2 +
.../DatabaseHostameSectionField.tsx | 2 +-
frontend/src/metabase/databases/constants.tsx | 4 +
src/metabase/api/testing.clj | 12 ++
src/metabase/auth_provider.clj | 45 ++++++++
src/metabase/db/data_source.clj | 68 ++++++++++-
src/metabase/db/env.clj | 48 ++++----
src/metabase/driver.clj | 29 ++++-
src/metabase/driver/common.clj | 26 +++++
src/metabase/driver/postgres.clj | 4 +-
src/metabase/driver/sql_jdbc/connection.clj | 30 ++++-
src/metabase/driver/util.clj | 17 +++
src/metabase/models/database.clj | 6 +-
test/metabase/auth_provider_test.clj | 30 +++++
test/metabase/db/data_source_test.clj | 107 +++++++++++++++++-
test/metabase/db/env_test.clj | 6 +-
.../driver/sql_jdbc/connection_test.clj | 34 ++++++
test/metabase/driver/util_test.clj | 78 +++++++++++++
22 files changed, 554 insertions(+), 47 deletions(-)
create mode 100644 frontend/src/metabase/databases/components/DatabaseAuthProviderSectionField/DatabaseAuthProviderSectionField.styled.tsx
create mode 100644 frontend/src/metabase/databases/components/DatabaseAuthProviderSectionField/DatabaseAuthProviderSectionField.tsx
create mode 100644 frontend/src/metabase/databases/components/DatabaseAuthProviderSectionField/index.ts
create mode 100644 src/metabase/auth_provider.clj
create mode 100644 test/metabase/auth_provider_test.clj
diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn
index f028486b018..b33a1249d2f 100644
--- a/.clj-kondo/config.edn
+++ b/.clj-kondo/config.edn
@@ -266,6 +266,7 @@
metabase.api :any
metabase.async :any
metabase.audit :any
+ metabase.auth-provider #{metabase.util}
metabase.bootstrap #{}
metabase.cmd :any
metabase.compatibility :any
diff --git a/docs/developers-guide/driver-changelog.md b/docs/developers-guide/driver-changelog.md
index 3ed350c4be7..b693024126c 100644
--- a/docs/developers-guide/driver-changelog.md
+++ b/docs/developers-guide/driver-changelog.md
@@ -113,6 +113,12 @@ title: Driver interface changelog
- The`:parameterized-sql` driver feature has been added to distinguish drivers that don't support parametrized SQL in
tests. Currently, this is disabled only for `:sparksql`.
+## Metabase 0.50.17
+
+- Added method `metabase.driver/incorporate-auth-provider-details` for driver specific behavior required to
+ incorporate response of an auth-provider into the DB details. In most cases this means setting the :password
+ and/or :username based on the auth-provider and its response.
+
## Metabase 0.50.16
- `:type/fingerprinting-unsupported` has been added in the `metabase.types` namespace. Similar to
diff --git a/frontend/src/metabase/databases/components/DatabaseAuthProviderSectionField/DatabaseAuthProviderSectionField.styled.tsx b/frontend/src/metabase/databases/components/DatabaseAuthProviderSectionField/DatabaseAuthProviderSectionField.styled.tsx
new file mode 100644
index 00000000000..d96f2d5ef17
--- /dev/null
+++ b/frontend/src/metabase/databases/components/DatabaseAuthProviderSectionField/DatabaseAuthProviderSectionField.styled.tsx
@@ -0,0 +1,14 @@
+import styled from "@emotion/styled";
+
+import Button from "metabase/core/components/Button";
+
+export const SectionButton = styled(Button)`
+ color: var(--mb-color-brand);
+ padding: 0;
+ border: none;
+ border-radius: 0;
+
+ &:hover {
+ background-color: transparent;
+ }
+`;
diff --git a/frontend/src/metabase/databases/components/DatabaseAuthProviderSectionField/DatabaseAuthProviderSectionField.tsx b/frontend/src/metabase/databases/components/DatabaseAuthProviderSectionField/DatabaseAuthProviderSectionField.tsx
new file mode 100644
index 00000000000..b19f046acdd
--- /dev/null
+++ b/frontend/src/metabase/databases/components/DatabaseAuthProviderSectionField/DatabaseAuthProviderSectionField.tsx
@@ -0,0 +1,32 @@
+import { useField } from "formik";
+import { useCallback } from "react";
+import { t } from "ttag";
+
+import FormField from "metabase/core/components/FormField";
+
+import { SectionButton } from "./DatabaseAuthProviderSectionField.styled";
+
+export interface DatabaseAuthProviderSectionFieldProps {
+ name: string;
+}
+
+const DatabaseAuthProviderSectionField = ({
+ name,
+}: DatabaseAuthProviderSectionFieldProps): JSX.Element => {
+ const [{ value }, , { setValue }] = useField(name);
+
+ const handleClick = useCallback(() => {
+ setValue(!value);
+ }, [value, setValue]);
+
+ return (
+ <FormField>
+ <SectionButton type="button" onClick={handleClick}>
+ {value ? t`Use password` : t`Use an authentication provider`}
+ </SectionButton>
+ </FormField>
+ );
+};
+
+// eslint-disable-next-line import/no-default-export -- deprecated usage
+export default DatabaseAuthProviderSectionField;
diff --git a/frontend/src/metabase/databases/components/DatabaseAuthProviderSectionField/index.ts b/frontend/src/metabase/databases/components/DatabaseAuthProviderSectionField/index.ts
new file mode 100644
index 00000000000..067776c5306
--- /dev/null
+++ b/frontend/src/metabase/databases/components/DatabaseAuthProviderSectionField/index.ts
@@ -0,0 +1,2 @@
+// eslint-disable-next-line import/no-default-export -- deprecated usage
+export { default } from "./DatabaseAuthProviderSectionField";
diff --git a/frontend/src/metabase/databases/components/DatabaseHostnameSectionField/DatabaseHostameSectionField.tsx b/frontend/src/metabase/databases/components/DatabaseHostnameSectionField/DatabaseHostameSectionField.tsx
index 9b5a1b13154..dddafc631cb 100644
--- a/frontend/src/metabase/databases/components/DatabaseHostnameSectionField/DatabaseHostameSectionField.tsx
+++ b/frontend/src/metabase/databases/components/DatabaseHostnameSectionField/DatabaseHostameSectionField.tsx
@@ -22,7 +22,7 @@ const DatabaseHostnameSectionField = ({
return (
<FormField>
<SectionButton type="button" onClick={handleClick}>
- {value ? t`Use hostname` : t`Use account name`}
+ {value ? t`Use account name` : t`Use hostname`}
</SectionButton>
</FormField>
);
diff --git a/frontend/src/metabase/databases/constants.tsx b/frontend/src/metabase/databases/constants.tsx
index 89158205106..677c1af661d 100644
--- a/frontend/src/metabase/databases/constants.tsx
+++ b/frontend/src/metabase/databases/constants.tsx
@@ -3,6 +3,7 @@ import { t } from "ttag";
import { SAVED_QUESTIONS_VIRTUAL_DB_ID } from "metabase-lib/v1/metadata/utils/saved-questions";
import DatabaseAuthCodeDescription from "./components/DatabaseAuthCodeDescription";
+import DatabaseAuthProviderSectionField from "./components/DatabaseAuthProviderSectionField";
import DatabaseCacheScheduleField from "./components/DatabaseCacheScheduleField";
import DatabaseClientIdDescription from "./components/DatabaseClientIdDescription";
import DatabaseConnectionSectionField from "./components/DatabaseConnectionSectionField";
@@ -107,6 +108,9 @@ export const FIELD_OVERRIDES: Record<string, EngineFieldOverride> = {
"use-hostname": {
type: DatabaseHostnameSectionField,
},
+ "use-auth-provider": {
+ type: DatabaseAuthProviderSectionField,
+ },
"let-user-control-scheduling": {
type: DatabaseScheduleToggleField,
},
diff --git a/src/metabase/api/testing.clj b/src/metabase/api/testing.clj
index 5bca6786391..65bf6a4d767 100644
--- a/src/metabase/api/testing.clj
+++ b/src/metabase/api/testing.clj
@@ -1,6 +1,7 @@
(ns metabase.api.testing
"Endpoints for testing."
(:require
+ [cheshire.core :as json]
[clojure.java.jdbc :as jdbc]
[clojure.string :as str]
[compojure.core :refer [POST]]
@@ -143,4 +144,15 @@
{:result (if clock :set :reset)
:time (t/instant)}))
+(api/defendpoint GET "/echo"
+ "Simple echo hander. Fails when you GET {\"fail\": true}."
+ [fail body]
+ {fail ms/BooleanValue
+ body ms/JSONString}
+ (if fail
+ {:status 400
+ :body {:error-code "oops"}}
+ {:status 200
+ :body (json/decode body true)}))
+
(api/define-routes)
diff --git a/src/metabase/auth_provider.clj b/src/metabase/auth_provider.clj
new file mode 100644
index 00000000000..320a71d76b8
--- /dev/null
+++ b/src/metabase/auth_provider.clj
@@ -0,0 +1,45 @@
+(ns metabase.auth-provider
+ (:require
+ [cheshire.core :as json]
+ [clj-http.client :as http]
+ [medley.core :as m]))
+
+(def azure-auth-token-renew-slack-seconds
+ "How many seconds before expiry we should prefer renewal.
+ This is a fairly arbitrary value, it's used just to avoid situations when we decide to use an
+ auth token which expires before we can put it to use."
+ 60)
+
+(defmulti fetch-auth
+ "Multimethod for auth-provider implementations.
+ In general, implementations shouldn't change the shape of responses or names
+ so that [[driver/incorporate-auth-provider-details]] can decide how to incorporate into details."
+ (fn [auth-provider _database-id _db-details]
+ auth-provider))
+
+(defmethod fetch-auth :default
+ [_auth-provider _database-id _db-details]
+ nil)
+
+(defn- parse-http-headers [headers]
+ (json/parse-string headers))
+
+(defn- ^:dynamic *fetch-as-json* [url headers]
+ (let [headers (cond-> headers
+ (string? headers) parse-http-headers)
+ response (http/get url (m/assoc-some {:as :json} :headers headers))]
+ (:body response)))
+
+(defmethod fetch-auth :http
+ [_ _database-id {:keys [http-auth-url http-auth-headers]}]
+ (*fetch-as-json* http-auth-url http-auth-headers))
+
+(defmethod fetch-auth :oauth
+ [_ _database-id {:keys [oauth-token-url oauth-token-headers]}]
+ (*fetch-as-json* oauth-token-url oauth-token-headers))
+
+(defmethod fetch-auth :azure-managed-identity
+ [_ _database-id {:keys [azure-managed-identity-client-id]}]
+ (*fetch-as-json* (str "http://169.254.169.254/metadata/identity/oauth2/token?api-version=2018-02-01&resource=https%3A%2F%2Fossrdbms-aad.database.windows.net&client_id="
+ azure-managed-identity-client-id)
+ {"Metadata" "true"}))
diff --git a/src/metabase/db/data_source.clj b/src/metabase/db/data_source.clj
index 6b781adf5cc..c8981a1dc76 100644
--- a/src/metabase/db/data_source.clj
+++ b/src/metabase/db/data_source.clj
@@ -4,6 +4,7 @@
(:require
[clojure.set :as set]
[clojure.string :as str]
+ [metabase.auth-provider :as auth-provider]
[metabase.config :as config]
[metabase.connection-pool :as connection-pool]
[metabase.db.spec :as mdb.spec]
@@ -17,6 +18,48 @@
(set! *warn-on-reflection* true)
+(defn ^:dynamic *current-millis*
+ "Returns the current time millis, but can be overridden for testing."
+ []
+ (System/currentTimeMillis))
+
+(defn- renew-azure-managed-identity-password
+ [client-id]
+ (let [{:keys [access_token expires_in]}
+ (auth-provider/fetch-auth :azure-managed-identity nil {:azure-managed-identity-client-id client-id})]
+ {:password access_token
+ :expiry (+ (*current-millis*) (* (- (parse-long expires_in)
+ auth-provider/azure-auth-token-renew-slack-seconds)
+ 1000))}))
+
+(defn- ensure-azure-managed-identity-password
+ "Make sure there is a \"password\" property in `properties` and returns a [[Properties]]
+ object without the azure managed identity properties.
+ Assumes that the \"azure-managed-identity-client-id\" property is only set if it
+ should be used to manage the password generation."
+ [^Properties properties]
+ (if-let [client-id (.getProperty properties "azure-managed-identity-client-id")]
+ (let [expiry (.get properties "password-expiry-timestamp")]
+ ;; check if we need to acquire the lock
+ (when (or (nil? (.getProperty properties "password"))
+ (nil? expiry) ; should not happen, as expiry should be set when the password is set
+ (<= expiry (*current-millis*)))
+ (locking properties
+ ;; check if a new password has to be generated
+ (let [expiry (.get properties "password-expiry-timestamp")]
+ (when (or (nil? (.getProperty properties "password"))
+ (nil? expiry)
+ (<= expiry (*current-millis*)))
+ (let [{:keys [password expiry]} (renew-azure-managed-identity-password client-id)]
+ (doto properties
+ (.setProperty "password" password)
+ (.put "password-expiry-timestamp" expiry)))))))
+ (doto (Properties.)
+ (.putAll properties)
+ (.remove "azure-managed-identity-client-id")
+ (.remove "password-expiry-timestamp")))
+ properties))
+
;; NOTE: Never instantiate a DataSource directly
;; Use one of our helper functions below to ensure [[update-h2/update-if-needed!]] is called
;; You can use [[raw-connection-string->DataSource]] or [[broken-out-details->DataSource]]
@@ -32,7 +75,7 @@
javax.sql.DataSource
(getConnection [_]
(doto (if properties
- (DriverManager/getConnection url properties)
+ (DriverManager/getConnection url (ensure-azure-managed-identity-password properties))
(DriverManager/getConnection url))
;; MySQL/MariaDB default to REPEATABLE_READ which ends up making everything SLOW because it locks all the time.
;; Postgres defaults to READ_COMMITTED. Explicitly set transaction isolation for new connections so we can make
@@ -58,9 +101,9 @@
(defn raw-connection-string->DataSource
"Return a [[javax.sql.DataSource]] given a raw JDBC connection string."
(^javax.sql.DataSource [s]
- (raw-connection-string->DataSource s nil nil))
+ (raw-connection-string->DataSource s nil nil nil))
- (^javax.sql.DataSource [s username password]
+ (^javax.sql.DataSource [s username password azure-managed-identity-client-id]
{:pre [(string? s)]}
;; normalize the protocol in case someone is trying to trip us up. Heroku is known for this and passes stuff in
;; like `postgres:...` to screw with us.
@@ -89,12 +132,26 @@
(log/error "Connection string contains a username, but MB_DB_USER is specified. MB_DB_USER will be used."))
_ (when (and (:password m) (seq password))
(log/error "Connection string contains a password, but MB_DB_PASS is specified. MB_DB_PASS will be used."))
+ _ (when (and (seq password) (seq azure-managed-identity-client-id))
+ (log/error "Both password and MB_DB_AZURE_MANAGED_IDENTITY_CLIENT_ID are specified. The password will be used."))
m (cond-> m
- (seq username) (assoc :user username)
- (seq password) (assoc :password password))]
+ (seq username) (assoc :user username)
+ (seq password) (assoc :password password)
+ (and (empty? password)
+ (seq azure-managed-identity-client-id)) (assoc :azure-managed-identity-client-id
+ azure-managed-identity-client-id))]
(update-h2/update-if-needed! s)
(->DataSource s (some-> (not-empty m) connection-pool/map->properties)))))
+(defn- remove-shadowed-azure-managed-identity-client-id
+ "A normal password takes precedence over Azure managed identity and we don't want
+ an empty string to be taken for a valid client ID."
+ [spec]
+ (cond-> spec
+ (or (empty? (:azure-managed-identity-client-id spec))
+ (seq (:password spec)))
+ (dissoc :azure-managed-identity-client-id)))
+
(defn broken-out-details->DataSource
"Return a [[javax.sql.DataSource]] given a broken-out Metabase connection details."
^javax.sql.DataSource [db-type details]
@@ -104,6 +161,7 @@
_ (assert subname)
url (format "jdbc:%s:%s" subprotocol subname)
properties (some-> (not-empty (dissoc spec :classname :subprotocol :subname))
+ remove-shadowed-azure-managed-identity-client-id
connection-pool/map->properties)]
(update-h2/update-if-needed! url)
diff --git a/src/metabase/db/env.clj b/src/metabase/db/env.clj
index 7d750aac6e6..471641eb669 100644
--- a/src/metabase/db/env.clj
+++ b/src/metabase/db/env.clj
@@ -77,20 +77,23 @@
(defn- broken-out-details
"Connection details that can be used when pretending the Metabase DB is itself a `Database` (e.g., to use the Generic
SQL driver functions on the Metabase DB itself)."
- [db-type {:keys [mb-db-dbname mb-db-host mb-db-pass mb-db-port mb-db-user], :as env-vars}]
+ [db-type {:keys [mb-db-dbname mb-db-host mb-db-pass mb-db-port mb-db-user mb-db-azure-managed-identity-client-id]
+ :as env-vars}]
(if (= db-type :h2)
(assoc h2-connection-properties
:db (env->db-file env-vars))
- {:host mb-db-host
- :port mb-db-port
- :db mb-db-dbname
- :user mb-db-user
- :password mb-db-pass}))
+ {:host mb-db-host
+ :port mb-db-port
+ :db mb-db-dbname
+ :user mb-db-user
+ :password mb-db-pass
+ :azure-managed-identity-client-id mb-db-azure-managed-identity-client-id}))
(defn- env->DataSource
- [db-type {:keys [mb-db-connection-uri mb-db-user mb-db-pass], :as env-vars}]
+ [db-type {:keys [mb-db-connection-uri mb-db-user mb-db-pass mb-db-azure-managed-identity-client-id], :as env-vars}]
(if mb-db-connection-uri
- (mdb.data-source/raw-connection-string->DataSource mb-db-connection-uri mb-db-user mb-db-pass)
+ (mdb.data-source/raw-connection-string->DataSource
+ mb-db-connection-uri mb-db-user mb-db-pass mb-db-azure-managed-identity-client-id)
(mdb.data-source/broken-out-details->DataSource db-type (broken-out-details db-type env-vars))))
@@ -116,20 +119,21 @@
(defn- env* [db-type]
(merge-with
- (fn [env-value default-value]
- (if (nil? env-value)
- default-value
- env-value))
- {:mb-db-type db-type
- :mb-db-in-memory (config/config-bool :mb-db-in-memory)
- :mb-db-file (config/config-str :mb-db-file)
- :mb-db-connection-uri (config/config-str :mb-db-connection-uri)
- :mb-db-host (config/config-str :mb-db-host)
- :mb-db-port (config/config-int :mb-db-port)
- :mb-db-dbname (config/config-str :mb-db-dbname)
- :mb-db-user (config/config-str :mb-db-user)
- :mb-db-pass (config/config-str :mb-db-pass)}
- (env-defaults db-type)))
+ (fn [env-value default-value]
+ (if (nil? env-value)
+ default-value
+ env-value))
+ {:mb-db-type db-type
+ :mb-db-in-memory (config/config-bool :mb-db-in-memory)
+ :mb-db-file (config/config-str :mb-db-file)
+ :mb-db-connection-uri (config/config-str :mb-db-connection-uri)
+ :mb-db-host (config/config-str :mb-db-host)
+ :mb-db-port (config/config-int :mb-db-port)
+ :mb-db-dbname (config/config-str :mb-db-dbname)
+ :mb-db-user (config/config-str :mb-db-user)
+ :mb-db-pass (config/config-str :mb-db-pass)
+ :mb-db-azure-managed-identity-client-id (config/config-str :mb-db-azure-managed-identity-client-id)}
+ (env-defaults db-type)))
(def env
"Metabase Datatbase environment. Used to setup *application-db* and audit-db for enterprise users."
diff --git a/src/metabase/driver.clj b/src/metabase/driver.clj
index 586516c205b..d0270d693d6 100644
--- a/src/metabase/driver.clj
+++ b/src/metabase/driver.clj
@@ -10,6 +10,7 @@
[clojure.set :as set]
[clojure.string :as str]
[java-time.api :as t]
+ [metabase.auth-provider :as auth-provider]
[metabase.driver.impl :as driver.impl]
[metabase.models.setting :as setting :refer [defsetting]]
[metabase.plugins.classloader :as classloader]
@@ -966,6 +967,30 @@
dispatch-on-uninitialized-driver
:hierarchy #'hierarchy)
+(defmulti incorporate-auth-provider-details
+ "A multimethod for driver specific behavior required to incorporate response of an auth-provider into the DB details.
+ In most cases this means setting the :password and/or :username based on the auth-provider and its response."
+ {:added "0.50.17" :arglists '([driver auth-provider auth-provider-response details])}
+ dispatch-on-initialized-driver
+ :hierarchy #'hierarchy)
+
+(defmethod incorporate-auth-provider-details :default
+ [_driver _auth-provider _auth-provider-response details]
+ details)
+
+(defmethod incorporate-auth-provider-details :sql-jdbc
+ [_driver auth-provider auth-provider-response details]
+ (case auth-provider
+ (:oauth :azure-managed-identity)
+ (let [{:keys [access_token expires_in]} auth-provider-response]
+ (cond-> (assoc details :password access_token)
+ expires_in (assoc :password-expiry-timestamp (+ (System/currentTimeMillis)
+ (* (- (parse-long expires_in)
+ auth-provider/azure-auth-token-renew-slack-seconds)
+ 1000)))))
+
+ (merge details auth-provider-response)))
+
;;; TODO:
;;;
;;; 1. We definitely should not be asking drivers to "update the value for `:details`". Drivers shouldn't touch the
@@ -983,9 +1008,9 @@
:hierarchy #'hierarchy)
(defmethod normalize-db-details ::driver
- [_ db-details]
+ [_ database]
;; no normalization by default
- db-details)
+ database)
(defmulti superseded-by
"Returns the driver that supersedes the given `driver`. A non-nil return value means that the given `driver` is
diff --git a/src/metabase/driver/common.clj b/src/metabase/driver/common.clj
index 2f8724cfa51..58e2bb9f6ed 100644
--- a/src/metabase/driver/common.clj
+++ b/src/metabase/driver/common.clj
@@ -226,6 +226,32 @@
added to the plugin manifest as connection properties, similar to the keys in the `default-options` map."
{:cloud-ip-address-info cloud-ip-address-info})
+(def auth-provider-options
+ "Options for using an auth provider instead of a literal password."
+ [{:name "use-auth-provider"
+ :type :section
+ :default false}
+ {:name "auth-provider"
+ :display-name (deferred-tru "Auth provider")
+ :type :select
+ :options [{:name (deferred-tru "Azure Managed Identity")
+ :value "azure-managed-identity"}
+ {:name (deferred-tru "OAuth")
+ :value "oauth"}]
+ :default "azure-managed-identity"
+ :visible-if {"use-auth-provider" true}}
+ {:name "azure-managed-identity-client-id"
+ :display-name (deferred-tru "Client ID")
+ :required true
+ :visible-if {"auth-provider" "azure-managed-identity"}}
+ {:name "oauth-token-url"
+ :display-name (deferred-tru "Auth token URL")
+ :required true
+ :visible-if {"auth-provider" "oauth"}}
+ {:name "oauth-token-headers"
+ :display-name (deferred-tru "Auth token request headers (a JSON map)")
+ :visible-if {"auth-provider" "oauth"}}])
+
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Class -> Base Type |
diff --git a/src/metabase/driver/postgres.clj b/src/metabase/driver/postgres.clj
index c08d8d424a3..138a6a81612 100644
--- a/src/metabase/driver/postgres.clj
+++ b/src/metabase/driver/postgres.clj
@@ -136,7 +136,9 @@
(assoc driver.common/default-port-details :placeholder 5432)
driver.common/default-dbname-details
driver.common/default-user-details
- driver.common/default-password-details
+ driver.common/auth-provider-options
+ (assoc driver.common/default-password-details
+ :visible-if {"use-auth-provider" false})
driver.common/cloud-ip-address-info
{:name "schema-filters"
:type :schema-filters
diff --git a/src/metabase/driver/sql_jdbc/connection.clj b/src/metabase/driver/sql_jdbc/connection.clj
index 84c34016986..f8b59711353 100644
--- a/src/metabase/driver/sql_jdbc/connection.clj
+++ b/src/metabase/driver/sql_jdbc/connection.clj
@@ -6,6 +6,7 @@
[metabase.connection-pool :as connection-pool]
[metabase.db :as mdb]
[metabase.driver :as driver]
+ [metabase.driver.util :as driver.u]
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.jvm :as lib.metadata.jvm]
[metabase.models.interface :as mi]
@@ -170,12 +171,18 @@ For setting the maximum, see [MB_APPLICATION_DB_MAX_CONNECTION_POOL_SIZE](#mb_ap
(let [details-with-tunnel (driver/incorporate-ssh-tunnel-details ;; If the tunnel is disabled this returned unchanged
driver
(update details :port #(or % (default-ssh-tunnel-target-port driver))))
- spec (connection-details->spec driver details-with-tunnel)
+ details-with-auth (driver.u/fetch-and-incorporate-auth-provider-details
+ driver
+ id
+ details-with-tunnel)
+ spec (connection-details->spec driver details-with-auth)
properties (data-warehouse-connection-pool-properties driver database)]
(merge
- (connection-pool-spec spec properties)
- ;; also capture entries related to ssh tunneling for later use
- (select-keys spec [:tunnel-enabled :tunnel-session :tunnel-tracker :tunnel-entrance-port :tunnel-entrance-host]))))
+ (connection-pool-spec spec properties)
+ ;; also capture entries related to ssh tunneling for later use
+ (select-keys spec [:tunnel-enabled :tunnel-session :tunnel-tracker :tunnel-entrance-port :tunnel-entrance-host])
+ ;; remember when the password expires
+ (select-keys details-with-auth [:password-expiry-timestamp]))))
(defn- destroy-pool! [database-id pool-spec]
(log/debug (u/format-color :red "Closing old connection pool for database %s ..." database-id))
@@ -228,6 +235,10 @@ For setting the maximum, see [MB_APPLICATION_DB_MAX_CONNECTION_POOL_SIZE](#mb_ap
(log/warn (u/format-color :yellow "Hash of database %s details changed; marking pool invalid to reopen it" db-id))
nil)
+(defn- log-password-expiry! [db-id]
+ (log/warn (u/format-color :yellow "Password of database %s expired; marking pool invalid to reopen it" db-id))
+ nil)
+
(defn db->pooled-connection-spec
"Return a JDBC connection spec that includes a cp30 `ComboPooledDataSource`. These connection pools are cached so we
don't create multiple ones for the same DB."
@@ -267,6 +278,12 @@ For setting the maximum, see [MB_APPLICATION_DB_MAX_CONNECTION_POOL_SIZE](#mb_ap
(when log-invalidation?
(log-jdbc-spec-hash-change-msg! db-id))
+ (let [{:keys [password-expiry-timestamp]} details]
+ (and (int? password-expiry-timestamp)
+ (<= password-expiry-timestamp (System/currentTimeMillis))))
+ (when log-invalidation?
+ (log-password-expiry! db-id))
+
(nil? (:tunnel-session details)) ; no tunnel in use; valid
details
@@ -310,7 +327,10 @@ For setting the maximum, see [MB_APPLICATION_DB_MAX_CONNECTION_POOL_SIZE](#mb_ap
[driver details f]
(let [details (update details :port #(or % (default-ssh-tunnel-target-port driver)))]
(ssh/with-ssh-tunnel [details-with-tunnel details]
- (let [spec (connection-details->spec driver details-with-tunnel)]
+ (let [details-with-auth (driver.u/fetch-and-incorporate-auth-provider-details
+ driver
+ details-with-tunnel)
+ spec (connection-details->spec driver details-with-auth)]
(f spec)))))
(defmacro with-connection-spec-for-testing-connection
diff --git a/src/metabase/driver/util.clj b/src/metabase/driver/util.clj
index bbfa7261b86..9880f1ba9b1 100644
--- a/src/metabase/driver/util.clj
+++ b/src/metabase/driver/util.clj
@@ -4,6 +4,7 @@
[clojure.core.memoize :as memoize]
[clojure.set :as set]
[clojure.string :as str]
+ [metabase.auth-provider :as auth-provider]
[metabase.config :as config]
[metabase.db :as mdb]
[metabase.driver :as driver]
@@ -659,3 +660,19 @@
password-fields (filter #(contains? #{:password :secret} (get % :type)) all-fields)]
(into default-sensitive-fields (map (comp keyword :name) password-fields)))
default-sensitive-fields))
+
+(defn fetch-and-incorporate-auth-provider-details
+ "Incorporates auth-provider responses with db-details.
+
+ If you have a database you need to pass the database-id as some providers will need to save the response (e.g. refresh-tokens)."
+ ([driver db-details]
+ (fetch-and-incorporate-auth-provider-details driver nil db-details))
+ ([driver database-id {:keys [use-auth-provider auth-provider] :as db-details}]
+ (if use-auth-provider
+ (let [auth-provider (keyword auth-provider)]
+ (driver/incorporate-auth-provider-details
+ driver
+ auth-provider
+ (auth-provider/fetch-auth auth-provider database-id db-details)
+ db-details))
+ db-details)))
diff --git a/src/metabase/models/database.clj b/src/metabase/models/database.clj
index e3f77547bd3..f63e16c9d2e 100644
--- a/src/metabase/models/database.clj
+++ b/src/metabase/models/database.clj
@@ -173,14 +173,16 @@
[{driver :engine, :as database}]
(letfn [(normalize-details [db]
(binding [*normalizing-details* true]
- (driver/normalize-db-details driver db)))]
+ (driver/normalize-db-details
+ driver
+ (m/update-existing-in db [:details :auth-provider] keyword))))]
(cond-> database
;; TODO - this is only really needed for API responses. This should be a `hydrate` thing instead!
(driver.impl/registered? driver)
(assoc :features (driver.u/features driver (t2.realize/realize database)))
(and (driver.impl/registered? driver)
- (:details database)
+ (map? (:details database))
(not *normalizing-details*))
normalize-details)))
diff --git a/test/metabase/auth_provider_test.clj b/test/metabase/auth_provider_test.clj
new file mode 100644
index 00000000000..2a09f3c5537
--- /dev/null
+++ b/test/metabase/auth_provider_test.clj
@@ -0,0 +1,30 @@
+(ns metabase.auth-provider-test
+ (:require
+ [cheshire.core :as json]
+ [clojure.test :refer [deftest is testing]]
+ [metabase.api.database :as api.database]
+ [metabase.http-client :as client]
+ [metabase.sync :as sync]
+ [metabase.test :as mt]
+ [metabase.test.data.interface :as tx]))
+
+(deftest auth-integration-test
+ (mt/test-drivers #{:postgres :mysql}
+ (let [original-details (:details (mt/db))
+ auth-details {:use-auth-provider true
+ :auth-provider :http
+ :http-auth-url (client/build-url "/testing/echo"
+ {:body (json/encode original-details)})}]
+ (mt/with-temp [:model/Database db
+ {:engine (tx/driver),
+ :details auth-details}]
+ (mt/with-db
+ db
+ (is (= auth-details (:details (mt/db))))
+ (testing "Connection tests"
+ (is (nil? (api.database/test-database-connection (:engine db) (:details db)))))
+ (testing "Syncing does not blow up"
+ (sync/sync-database! (mt/db)))
+ (testing "Querying"
+ (is (= [["Polo Lounge"]]
+ (mt/rows (mt/run-mbql-query venues {:filter [:= $id 60] :fields [$name]}))))))))))
diff --git a/test/metabase/db/data_source_test.clj b/test/metabase/db/data_source_test.clj
index 7a54dccb2ba..e38562f97d8 100644
--- a/test/metabase/db/data_source_test.clj
+++ b/test/metabase/db/data_source_test.clj
@@ -2,10 +2,13 @@
(:require
[clojure.java.jdbc :as jdbc]
[clojure.test :refer :all]
+ [metabase.auth-provider :as auth-provider]
[metabase.config :as config]
[metabase.connection-pool :as connection-pool]
[metabase.db.data-source :as mdb.data-source]
- [metabase.test :as mt]))
+ [metabase.test :as mt])
+ (:import
+ (java.util Properties)))
(set! *warn-on-reflection* true)
@@ -26,6 +29,32 @@
:password "1234"
:db "metabase"}))))
+ (testing :azure-managed-identity
+ (is (= (->DataSource
+ "jdbc:postgresql://localhost:5432/metabase"
+ {"ApplicationName" config/mb-version-and-process-identifier
+ "OpenSourceSubProtocolOverride" "true"
+ "user" "cam"
+ "azure-managed-identity-client-id" "client ID"})
+ (mdb.data-source/broken-out-details->DataSource :postgres {:host "localhost"
+ :port 5432
+ :user "cam"
+ :azure-managed-identity-client-id "client ID"
+ :db "metabase"})))
+ (testing "password takes precedence"
+ (is (= (->DataSource
+ "jdbc:postgresql://localhost:5432/metabase"
+ {"password" "1234"
+ "ApplicationName" config/mb-version-and-process-identifier
+ "OpenSourceSubProtocolOverride" "true"
+ "user" "cam"})
+ (mdb.data-source/broken-out-details->DataSource :postgres {:host "localhost"
+ :port 5432
+ :user "cam"
+ :password "1234"
+ :azure-managed-identity-client-id "client ID"
+ :db "metabase"})))))
+
(testing :h2
(is (= (->DataSource
"jdbc:h2:file:/metabase/metabase.db"
@@ -102,19 +131,28 @@
(is (= (->DataSource
"jdbc:postgresql://metabase"
{"user" "cam", "password" "1234"})
- (mdb.data-source/raw-connection-string->DataSource "postgres://metabase" "cam" "1234"))))
+ (mdb.data-source/raw-connection-string->DataSource "postgres://metabase" "cam" "1234" nil)
+ (mdb.data-source/raw-connection-string->DataSource "postgres://metabase" "cam" "1234" "client ID"))))
(testing "username only"
(is (= (->DataSource
"jdbc:postgresql://metabase"
{"user" "cam"})
- (mdb.data-source/raw-connection-string->DataSource "postgres://metabase" "cam" nil)
- (mdb.data-source/raw-connection-string->DataSource "postgres://metabase" "cam" ""))))
+ (mdb.data-source/raw-connection-string->DataSource "postgres://metabase" "cam" nil nil)
+ (mdb.data-source/raw-connection-string->DataSource "postgres://metabase" "cam" "" nil))))
+ (testing "username and azure-managed-identity-client-id"
+ (is (= (->DataSource
+ "jdbc:postgresql://metabase"
+ {"user" "cam"
+ "azure-managed-identity-client-id" "client ID"})
+ (mdb.data-source/raw-connection-string->DataSource "postgres://metabase" "cam" nil "client ID")
+ (mdb.data-source/raw-connection-string->DataSource "postgres://metabase" "cam" "" "client ID"))))
(testing "password only"
(is (= (->DataSource
"jdbc:postgresql://metabase"
{"password" "1234"})
- (mdb.data-source/raw-connection-string->DataSource "postgres://metabase" nil "1234")
- (mdb.data-source/raw-connection-string->DataSource "postgres://metabase" "" "1234"))))))
+ (mdb.data-source/raw-connection-string->DataSource "postgres://metabase" nil "1234" nil)
+ (mdb.data-source/raw-connection-string->DataSource "postgres://metabase" "" "1234" nil)
+ (mdb.data-source/raw-connection-string->DataSource "postgres://metabase" "" "1234" "client ID"))))))
(deftest ^:parallel equality-test
(testing "Two DataSources with the same URL should be equal"
@@ -124,3 +162,60 @@
(testing "Two DataSources with the same URL and properties should be equal"
(is (= (mdb.data-source/broken-out-details->DataSource :h2 {:db "wow", :x 1})
(mdb.data-source/broken-out-details->DataSource :h2 {:db "wow", :x 1})))))
+
+(deftest ^:parallel ensure-azure-managed-identity-password-test
+ (testing "nothing happens if ensure-azure-managed-identity-client-id is missing"
+ (let [props (Properties.)]
+ (is (empty? (#'mdb.data-source/ensure-azure-managed-identity-password props)))
+ (is (empty? props))))
+ (testing "password is set if it's missing"
+ (let [now 0
+ expiry-secs 1000
+ expiry (+ now (* (- expiry-secs auth-provider/azure-auth-token-renew-slack-seconds) 1000))
+ props (doto (Properties.)
+ (.setProperty "azure-managed-identity-client-id" "client ID"))]
+ (binding [auth-provider/*fetch-as-json* (fn [_url _headers]
+ {:access_token "access token"
+ :expires_in (str expiry-secs)})
+ mdb.data-source/*current-millis* (constantly now)]
+ (is (= {"password" "access token"}
+ (#'mdb.data-source/ensure-azure-managed-identity-password props))))
+ (is (= {"azure-managed-identity-client-id" "client ID"
+ "password" "access token"
+ "password-expiry-timestamp" expiry}
+ props))))
+ (testing "nothing happens if a fresh enough password is present"
+ (let [now 0
+ expiry-secs 1000
+ expiry (+ now (* (- expiry-secs auth-provider/azure-auth-token-renew-slack-seconds) 1000))
+ props (doto (Properties.)
+ (.putAll {"azure-managed-identity-client-id" "client ID"
+ "password" "access token"
+ "password-expiry-timestamp" expiry}))]
+ (binding [auth-provider/*fetch-as-json* (fn [_url _headers]
+ (is false "should not get called"))
+ mdb.data-source/*current-millis* (constantly now)]
+ (is (= {"password" "access token"}
+ (#'mdb.data-source/ensure-azure-managed-identity-password props))))
+ (is (= {"azure-managed-identity-client-id" "client ID"
+ "password" "access token"
+ "password-expiry-timestamp" expiry}
+ props))))
+ (testing "a new password is set if the old one is stale"
+ (let [now 0
+ expiry-secs 1000
+ expiry (+ now (* (- expiry-secs auth-provider/azure-auth-token-renew-slack-seconds) 1000))
+ props (doto (Properties.)
+ (.putAll {"azure-managed-identity-client-id" "client ID"
+ "password" "access token"
+ "password-expiry-timestamp" 0}))]
+ (binding [auth-provider/*fetch-as-json* (fn [_url _headers]
+ {:access_token "new access token"
+ :expires_in (str expiry-secs)})
+ mdb.data-source/*current-millis* (constantly now)]
+ (is (= {"password" "new access token"}
+ (#'mdb.data-source/ensure-azure-managed-identity-password props))))
+ (is (= {"azure-managed-identity-client-id" "client ID"
+ "password" "new access token"
+ "password-expiry-timestamp" expiry}
+ props)))))
diff --git a/test/metabase/db/env_test.clj b/test/metabase/db/env_test.clj
index 0989c6a4e50..f16a0b5a318 100644
--- a/test/metabase/db/env_test.clj
+++ b/test/metabase/db/env_test.clj
@@ -19,14 +19,14 @@
(testing "Raw connection string should support separate username and/or password (#20122)"
(testing "username and password"
- (is (= (mdb.data-source/raw-connection-string->DataSource "jdbc:postgresql://metabase" "cam" "1234")
+ (is (= (mdb.data-source/raw-connection-string->DataSource "jdbc:postgresql://metabase" "cam" "1234" nil)
(#'mdb.env/env->DataSource :postgres {:mb-db-connection-uri "postgres://metabase", :mb-db-user "cam", :mb-db-pass "1234"}))))
(testing "username only"
- (is (= (mdb.data-source/raw-connection-string->DataSource "jdbc:postgresql://metabase" "cam" nil)
+ (is (= (mdb.data-source/raw-connection-string->DataSource "jdbc:postgresql://metabase" "cam" nil nil)
(#'mdb.env/env->DataSource :postgres {:mb-db-connection-uri "postgres://metabase", :mb-db-user "cam"})
(#'mdb.env/env->DataSource :postgres {:mb-db-connection-uri "postgres://metabase", :mb-db-user "cam", :mb-db-pass ""}))))
(testing "password only"
- (is (= (mdb.data-source/raw-connection-string->DataSource "jdbc:postgresql://metabase" nil "1234")
+ (is (= (mdb.data-source/raw-connection-string->DataSource "jdbc:postgresql://metabase" nil "1234" nil)
(#'mdb.env/env->DataSource :postgres {:mb-db-connection-uri "postgres://metabase", :mb-db-pass "1234"})
(#'mdb.env/env->DataSource :postgres {:mb-db-connection-uri "postgres://metabase", :mb-db-user "", :mb-db-pass "1234"}))))))
diff --git a/test/metabase/driver/sql_jdbc/connection_test.clj b/test/metabase/driver/sql_jdbc/connection_test.clj
index c286b45da2b..109f97bac84 100644
--- a/test/metabase/driver/sql_jdbc/connection_test.clj
+++ b/test/metabase/driver/sql_jdbc/connection_test.clj
@@ -3,6 +3,7 @@
[clojure.java.jdbc :as jdbc]
[clojure.string :as str]
[clojure.test :refer :all]
+ [metabase.auth-provider :as auth-provider]
[metabase.config :as config]
[metabase.core :as mbc]
[metabase.db :as mdb]
@@ -230,6 +231,39 @@
server (Server/createTcpServer (into-array args))]
(doto server (.start))))
+(deftest test-auth-provider-connection
+ (mt/test-drivers #{:postgres}
+ (testing "Azure Managed Identity connections can be created and expired passwords get renewed"
+ (let [db-details (:details (mt/db))
+ oauth-db-details (-> db-details
+ (dissoc :password)
+ (assoc :use-auth-provider true
+ :auth-provider :azure-managed-identity
+ :azure-managed-identity-client-id "client ID"))
+ ;; we return an expired token which forces a renewal when a second connection is requested
+ ;; (the first time it is used without checking for expiry)
+ expires-in (atom "0")
+ connection-creations (atom 0)]
+ (binding [auth-provider/*fetch-as-json* (fn [url _headers]
+ (is (str/includes? url "client ID"))
+ (swap! connection-creations inc)
+ {:access_token (:password db-details)
+ :expires_in @expires-in})]
+ (t2.with-temp/with-temp [Database oauth-db {:engine (tx/driver), :details oauth-db-details}]
+ (mt/with-db oauth-db
+ (try
+ ;; since Metabase is running and using the pool of this DB, the sync might fail
+ ;; if the connection pool is shut down during the sync
+ (sync/sync-database! (mt/db))
+ (catch Exception _))
+ ;; after "fixing" the expiry, we should get a connection from a pool that doesn't get shut down
+ (reset! expires-in "10000")
+ (sync/sync-database! (mt/db))
+ (is (= [["Polo Lounge"]]
+ (mt/rows (mt/run-mbql-query venues {:filter [:= $id 60] :fields [$name]}))))
+ ;; we must have created more than one connection
+ (is (> @connection-creations 1)))))))))
+
(deftest test-ssh-tunnel-connection
;; TODO: Formerly this test ran against "all JDBC drivers except this big list":
;; (apply disj (sql-jdbc.tu/sql-jdbc-drivers)
diff --git a/test/metabase/driver/util_test.clj b/test/metabase/driver/util_test.clj
index 41a4faa0d67..fa4f3cfb500 100644
--- a/test/metabase/driver/util_test.clj
+++ b/test/metabase/driver/util_test.clj
@@ -1,15 +1,19 @@
(ns metabase.driver.util-test
(:require
+ [cheshire.core :as json]
[clojure.java.io :as io]
[clojure.string :as str]
[clojure.test :refer :all]
+ [metabase.auth-provider :as auth-provider]
[metabase.driver :as driver]
[metabase.driver.h2 :as h2]
[metabase.driver.util :as driver.u]
+ [metabase.http-client :as client]
[metabase.lib.test-metadata :as meta]
[metabase.lib.test-util :as lib.tu]
[metabase.query-processor.store :as qp.store]
[metabase.test :as mt]
+ [metabase.test.data.interface :as tx]
[metabase.test.fixtures :as fixtures]
[metabase.util :as u])
(:import
@@ -312,3 +316,77 @@
(let [log-messages (mt/with-log-messages-for-level [metabase.driver.util :error]
(is (false? (driver.u/supports? :test-driver :expressions db))))]
(is (nil? log-messages)))))))))
+
+(defmethod auth-provider/fetch-auth ::test-me
+ [_provider _db-id details]
+ (is (= "testing" (:key details)))
+ {:password "qux"})
+
+(deftest ^:parallel simple-test
+ (let [details {:username "test"
+ :password "ignored"
+ :use-auth-provider true
+ :auth-provider ::test-me
+ :key "testing"}]
+ (mt/with-temp [:model/Database db {:details details}]
+ (is (=? (assoc details :password "qux")
+ (driver.u/fetch-and-incorporate-auth-provider-details
+ (:engine db)
+ (:id db)
+ (:details db)))))))
+
+(deftest http-provider-tests
+ (let [original-details (:details (mt/db))
+ provider-details {:use-auth-provider true
+ :auth-provider "http"
+ :http-auth-url (client/build-url "/testing/echo"
+ {:body (json/encode original-details)})}]
+ (is (= original-details (auth-provider/fetch-auth :http nil provider-details)))
+ (is (= (merge provider-details original-details)
+ (driver.u/fetch-and-incorporate-auth-provider-details
+ (tx/driver)
+ provider-details)))))
+
+(deftest oauth-provider-tests
+ (let [oauth-response {:access_token "foobar"
+ :expires_in "84791"}
+ provider-details {:use-auth-provider true
+ :auth-provider :oauth
+ :oauth-token-url (client/build-url "/testing/echo"
+ {:body (json/encode oauth-response)})}]
+ (is (= oauth-response (auth-provider/fetch-auth :oauth nil provider-details)))
+ (is (=? (merge provider-details
+ {:password "foobar"
+ :password-expiry-timestamp #(and (int? %) (> % (System/currentTimeMillis)))})
+ (driver.u/fetch-and-incorporate-auth-provider-details
+ (tx/driver)
+ provider-details)))))
+
+(deftest ^:parallel azure-managed-identity-provider-tests
+ (testing "password gets resolved"
+ (let [client-id "client ID"
+ provider-details {:use-auth-provider true
+ :auth-provider :azure-managed-identity
+ :azure-managed-identity-client-id client-id
+ :password "xyz"}
+ response-body {:access_token "foobar"}]
+ (binding [auth-provider/*fetch-as-json* (fn [url _headers]
+ (is (str/includes? url client-id))
+ response-body)]
+ (is (= response-body (auth-provider/fetch-auth :azure-managed-identity nil provider-details)))
+ (is (= (merge provider-details {:password "foobar"})
+ (driver.u/fetch-and-incorporate-auth-provider-details
+ (tx/driver)
+ provider-details))))))
+ (testing "existing password doesn't get overwritten if not using an auth provider"
+ (let [client-id "client ID"
+ provider-details {:use-auth-provider false
+ :auth-provider :azure-managed-identity
+ :azure-managed-identity-client-id client-id
+ :password "xyz"}]
+ (binding [auth-provider/*fetch-as-json* (fn [_url _headers]
+ (is false "should not get called"))]
+ (is (= provider-details
+ (driver.u/fetch-and-incorporate-auth-provider-details
+ (tx/driver)
+ provider-details)))))))
--
GitLab