From d2113a27e0a12406ffb1b3163c47d1014ae9590e Mon Sep 17 00:00:00 2001
From: Chris Truter <crisptrutski@users.noreply.github.com>
Date: Tue, 23 Jan 2024 15:59:33 +0200
Subject: [PATCH] Fix potential deadlock when loading feature flags
Fixes https://github.com/metabase/metabase/issues/38009
### Description
This makes the feature flag loading code more robust by making the function re-entrant. To understand how this function could deadlock before, see the description in https://github.com/metabase/metabase/pull/38027.
It works by ensuring that the call to initialize the nonce comes from the same thread that initially locked on fetching the tokens.
There is a subtle behavior change here - was allow for a 10s timeout on each call individually now. Since it is unlikely that both endpoints will timeout consecutively, and the combined wait time is still fairly short, I think this is OK.
### How to verify
You can use the same steps as https://github.com/metabase/metabase/pull/38027, either of these PRs are sufficient to fix the bug.
---
.../public_settings/premium_features.clj | 65 ++++++++++---------
1 file changed, 36 insertions(+), 29 deletions(-)
diff --git a/src/metabase/public_settings/premium_features.clj b/src/metabase/public_settings/premium_features.clj
index af1067898c2..88ca1e5988a 100644
--- a/src/metabase/public_settings/premium_features.clj
+++ b/src/metabase/public_settings/premium_features.clj
@@ -96,15 +96,28 @@
[:trial {:optional true} :boolean]
[:valid-thru {:optional true} ms/NonBlankString]]) ; ISO 8601 timestamp
-(defn- fetch-token-and-parse-body
- [token base-url]
+
+(defn- fetch-token-and-parse-body*
+ [token base-url site-uuid]
(some-> (token-status-url token base-url)
(http/get {:query-params {:users (cached-active-users-count)
- :site-uuid (setting/get :site-uuid-for-premium-features-token-checks)
+ :site-uuid site-uuid
:mb-version (:tag config/mb-version-info)}})
:body
(json/parse-string keyword)))
+(defn- fetch-token-and-parse-body
+ [token base-url site-uuid]
+ (let [fut (future (fetch-token-and-parse-body* token base-url site-uuid))
+ result (deref fut fetch-token-status-timeout-ms ::timed-out)]
+ (if (not= result ::timed-out)
+ result
+ (do
+ (future-cancel fut)
+ {:valid false
+ :status (tru "Unable to validate token")
+ :error-details (tru "Token validation timed out.")}))))
+
(mu/defn ^:private fetch-token-status* :- TokenStatus
"Fetch info about the validity of `token` from the MetaStore."
[token :- :string]
@@ -117,32 +130,26 @@
{:valid false
:status "invalid"
:error-details (trs "Token should be 64 hexadecimal characters.")})
- (let [fut (future
- (try (fetch-token-and-parse-body token token-check-url)
- (catch Exception e1
- (log/error e1 (trs "Error fetching token status from {0}:" token-check-url))
- ;; Try the fallback URL, which was the default URL prior to 45.2
- (try (fetch-token-and-parse-body token store-url)
- ;; if there was an error fetching the token from both the normal and fallback URLs, log the
- ;; first error and return a generic message about the token being invalid. This message
- ;; will get displayed in the Settings page in the admin panel so we do not want something
- ;; complicated
- (catch Exception e2
- (log/error e2 (trs "Error fetching token status from {0}:" store-url))
- (let [body (u/ignore-exceptions (some-> (ex-data e1) :body (json/parse-string keyword)))]
- (or
- body
- {:valid false
- :status (tru "Unable to validate token")
- :error-details (.getMessage e1)})))))))
- result (deref fut fetch-token-status-timeout-ms ::timed-out)]
- (if (= result ::timed-out)
- (do
- (future-cancel fut)
- {:valid false
- :status (tru "Unable to validate token")
- :error-details (tru "Token validation timed out.")})
- result))))
+ (let [site-uuid (setting/get :site-uuid-for-premium-features-token-checks)]
+ (try (fetch-token-and-parse-body token token-check-url site-uuid)
+ (catch Exception e1
+ ;; Unwrap exception from inside the future
+ (let [e1 (ex-cause e1)]
+ (log/error e1 (trs "Error fetching token status from {0}:" token-check-url))
+ ;; Try the fallback URL, which was the default URL prior to 45.2
+ (try (fetch-token-and-parse-body token store-url site-uuid)
+ ;; if there was an error fetching the token from both the normal and fallback URLs, log the
+ ;; first error and return a generic message about the token being invalid. This message
+ ;; will get displayed in the Settings page in the admin panel so we do not want something
+ ;; complicated
+ (catch Exception e2
+ (log/error (ex-cause e2) (trs "Error fetching token status from {0}:" store-url))
+ (let [body (u/ignore-exceptions (some-> (ex-data e1) :body (json/parse-string keyword)))]
+ (or
+ body
+ {:valid false
+ :status (tru "Unable to validate token")
+ :error-details (.getMessage e1)}))))))))))
(def ^{:arglists '([token])} fetch-token-status
"TTL-memoized version of `fetch-token-status*`. Caches API responses for 5 minutes. This is important to avoid making
--
GitLab