Skip to content
Snippets Groups Projects
Unverified Commit e7b4dc62 authored by dpsutton's avatar dpsutton Committed by GitHub
Browse files

Fix constantly checking token on error (#23815)

* Fix constantly checking token on error

We were catching `clojure.lang.ExceptionInfo` which only catches a small
subset of errors and not any related to what might be thrown in network
calls.

The reason this affected us is that the cache will cache this value but
keep trying what is throwing the error. So each time we look at token
features it attempts again and throws an error.

If we instead do what the code _meant_ to do and we return a value
indicating there was an error, we are all good.

Example:

```clojure
premium-features=> (defn is-odd? [x]
                     (println "computing for " x)
                     (if (odd? x) true (throw (ex-info "boom" {}))))
premium-features=> (def modd (memoize/ttl is-odd? :ttl/threshold 30000))
premium-features=> (modd 3)
computing for  3
true
premium-features=> (modd 5)
computing for  5
true
premium-features=> (modd 3)
true
premium-features=> (modd 2)
computing for  2
Execution error (ExceptionInfo) at metabase.public-settings.premium-features/is-odd? (REPL:289).
boom
premium-features=> (modd 2)
computing for  2
Execution error (ExceptionInfo) at metabase.public-settings.premium-features/is-odd? (REPL:289).
boom
```

Note that `(modd 2)` keeps attempting to compute for 2 since it throws
an error rather than returning a value indicating an error.

When the token check throws an error:

```clojure
premium-features=> (fetch-token-status (apply str (repeat 64 "b")))
Execution error (UnknownHostException) at java.net.Inet6AddressImpl/lookupAllHostAddr (Inet6AddressImpl.java:-2).
store.metabase.com: nodename nor servname provided, or not known
```

When the token check returns a value indicating an error:

```clojure
premium-features=> (fetch-token-status (apply str (repeat 64 "b")))
{:valid false,
 :status "Unable to validate token",
 :error-details "store.metabase.com: nodename nor servname provided, or not known"}
```

(both instances were with wifi turned off locally)

* add test

* Assert we only call the api once when it throws an error

before this PR we would make a call for each setting (perhaps
more). Each time trying to hit the endpoint. Now it catches those errors
and reports not a valid token
parent 60fa245e
No related branches found
No related tags found
No related merge requests found
......@@ -76,7 +76,7 @@
;; if there was an error fetching the token, log it 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 clojure.lang.ExceptionInfo e
(catch Exception e
(log/error e (trs "Error fetching token status:"))
(let [body (u/ignore-exceptions (some-> (ex-data e) :body (json/parse-string keyword)))]
(or
......
......@@ -48,6 +48,10 @@
(def random-fake-token
"d7ad0b5f9ddfd1953b1b427b75d620e4ba91d38e7bcbc09d8982480863dbc611")
(defn- random-token []
(let [alphabet (into [] (concat (range 0 10) (map char (range (int \a) (int \g)))))]
(apply str (repeatedly 64 #(rand-nth alphabet)))))
(deftest fetch-token-status-test
(tt/with-temp User [_user {:email "admin@example.com"}]
(let [print-token "d7ad...c611"]
......@@ -61,6 +65,31 @@
(testing "With the backend unavailable"
(let [result (token-status-response random-fake-token {:status 500})]
(is (false? (:valid result)))))
(testing "On other errors"
(binding [clj-http.client/request (fn [& args]
;; note originally the code caught clojure.lang.ExceptionInfo so don't
;; throw an ex-info here
(throw (Exception. "network issues")))]
(is (= {:valid false
:status "Unable to validate token"
:error-details "network issues"}
(premium-features/fetch-token-status (apply str (repeat 64 "b")))))))
(testing "Only attempt the token once"
(let [call-count (atom 0)]
(binding [clj-http.client/request (fn [& args]
(swap! call-count inc)
(throw (Exception. "no internet")))]
(mt/with-temporary-raw-setting-values [:premium-embedding-token (random-token)]
(doseq [premium-setting [premium-features/hide-embed-branding?
premium-features/enable-whitelabeling?
premium-features/enable-audit-app?
premium-features/enable-sandboxes?
premium-features/enable-sso?
premium-features/enable-advanced-config?
premium-features/enable-content-management?]]
(is (false? (premium-setting))
(str (:name (meta premium-setting)) "is not false")))
(is (= @call-count 1))))))
(testing "With a valid token"
(let [result (token-status-response random-fake-token {:status 200
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment