Skip to content

Refresh expired access tokens #56

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Refresh expired access tokens #56

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
1a76e62
Refresh expired access tokens
Jan 10, 2025
9f0c3f4
reflow to 80 characters per line
Jan 12, 2025
0a0b106
rename get-expired → expired-access-tokens
Jan 12, 2025
cbfa368
remove docstrings on non-public functions; remove unrelated changes
Jan 12, 2025
ba38ad2
prefer `(Date.)` over `(new Date)`
Jan 12, 2025
65e81fc
remove token type check
Jan 12, 2025
5327380
refactor `expired-access-tokens` as filter
Jan 12, 2025
448624e
fix confusing naming
Jan 12, 2025
1c0b548
rename socket-timeout constant to be more specific
Jan 12, 2025
94ad054
refactor `refresh-all-tokens`
Jan 15, 2025
9339b82
refactor http param options
Jan 15, 2025
e8d25b9
remove spurious newlines in `defn`
Jan 15, 2025
67c8543
inline `now`
Jan 15, 2025
b0c3f83
slightly refactor `refresh-one-token` for clarity
Jan 15, 2025
a4f5e2c
rework token refresh into a middle function
Jan 21, 2025
069e4b5
minor refactor
Jan 21, 2025
26f2b34
update response session unless nil or tokens unchanged
Jan 24, 2025
ec2989a
cosmetic change
Apr 13, 2025
257c77e
rename refresh-token-request-options -> refresh-token-http-options
Apr 16, 2025
46472f0
refactor expired-access-tokens
Apr 16, 2025
84589a1
replace `:let` with `let`
Apr 16, 2025
60b950d
remove extra space
Apr 16, 2025
b057931
more concise `wrap-refresh-access-tokens`
Apr 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 105 additions & 12 deletions src/ring/middleware/oauth2.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@
(defn- get-code-verifier [request]
(get-in request [:session ::code-verifier]))

(defn- request-params [{:keys [pkce?] :as profile} request]
(defn- access-token-request-params [{:keys [pkce?] :as profile} request]
(-> {:grant_type "authorization_code"
:code (get-authorization-code request)
:redirect_uri (redirect-uri profile request)}
Expand All @@ -112,19 +112,22 @@
(merge {:client_id id
:client_secret secret}))))

(defn- access-token-http-options
(defn- token-http-options
[{:keys [access-token-uri client-id client-secret basic-auth?]
:or {basic-auth? false} :as profile}
request]
:or {basic-auth? false}}
form-params]
(let [opts {:method :post
:url access-token-uri
:accept :json
:as :json
:form-params (request-params profile request)}]
:form-params form-params}]
(if basic-auth?
(add-header-credentials opts client-id client-secret)
(add-form-credentials opts client-id client-secret))))

(defn- access-token-http-options [profile request]
(token-http-options profile (access-token-request-params profile request)))

(defn- get-access-token
([profile request]
(-> (http/request (access-token-http-options profile request))
Expand Down Expand Up @@ -188,11 +191,100 @@
(respond (redirect-response profile session token)))
raise)))))

(defn- assoc-access-tokens [request]
(if-let [tokens (-> request :session ::access-tokens)]
(defn- expired-access-token? [{:keys [expires refresh-token]}]
(and refresh-token expires (.before expires (Date.))))

(defn expired-access-tokens [tokens]
(into {} (filter (comp expired-access-token? val)) tokens))

(defn- update-tokens [access-tokens [profile-key maybe-grant]]
(if maybe-grant
;; `update ... merge` to properly handle case where authorization server
;; does not update the refresh token after use and we should re-use the
;; existing refresh token
(update access-tokens profile-key merge maybe-grant)
(dissoc access-tokens profile-key)))

(def refresh-socket-timeout 60000)

(defn- refresh-token-http-options [profile refresh-token]
(-> (token-http-options profile {:grant_type "refresh_token"
:refresh_token refresh-token})
(assoc :socket-timeout refresh-socket-timeout)))

(defn- refresh-one-token
([profile refresh-token]
(-> (http/request (refresh-token-http-options profile refresh-token))
format-access-token))
([profile refresh-token respond raise]
(let [opts (-> (refresh-token-http-options profile refresh-token)
(assoc :async? true))]
(http/request opts (comp respond format-access-token) raise))))

(defn- refresh-tasks [profiles access-tokens]
(->> (expired-access-tokens access-tokens)
(keep (fn [[profile-key {:keys [refresh-token]}]]
(when (and (get profiles profile-key) refresh-token)
[profile-key [(get profiles profile-key) refresh-token]])))))

(defn- async-map-values [f respond m]
(let [total (count m)
results (atom {})
respond-when-done #(when (= (count %) total) (respond %))]
(if (zero? total)
(respond {})
(doseq [[k v] m]
(let [respond #(respond-when-done (swap! results assoc k %))
raise (fn [_] (respond nil))]
(f v respond raise))))))

(defn- refresh-all-tokens
([profiles access-tokens]
(->> (refresh-tasks profiles access-tokens)
(map (fn [[profile-key [profile refresh-token]]]
[profile-key
(try (refresh-one-token profile refresh-token)
(catch clojure.lang.ExceptionInfo _ nil))]))
(reduce update-tokens access-tokens)))
([profiles access-tokens respond]
(async-map-values
(fn [[profile refresh-token] respond raise]
(refresh-one-token profile refresh-token respond raise))
Comment on lines +251 to +252
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the synchronous map, the error catching function is in the map iteration function, but in async-map-values, the error catching function is built in.

My suggestion would be to make these two maps consistent. We assume that the mapping function, f, will not raise an exception:

(defn- async-map-values [f respond m]
  (let [total (count m)
        results (atom {})
        respond-when-done #(when (= (count %) total) (respond %))]
    (if (zero? total)
      (respond {})
      (doseq [[k v] m]
        (f v #(respond-when-done (swap! results assoc k %)))))

We then do the equivalent of "catching" the exception when calling it in refresh-all-tokens:

   (async-map-values
    (fn [[profile refresh-token] respond]
      (refresh-one-token profile refresh-token respond (fn [_] (respond nil)))
    (fn [refreshed-tokens]
      (respond (reduce update-tokens access-tokens refreshed-tokens)))
    (refresh-tasks profiles access-tokens))))

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed your refactor is more elegant, but its behavior differs from what I initially wrote. Assume two tokens require refresh, one of which fails:

  • When following your suggestion, refresh-all-tokens's continuation is called with nil. So no refresh occurs.
  • In the version you reviewed, the results in async-map-values will accumulate both a nil and a new access token, and the user's session will receive the one updated access token. The failed token will be retried on subsequent requests.

Do you have a preferred behavior in this case?

(fn [refreshed-tokens]
(respond
(reduce update-tokens access-tokens refreshed-tokens)))
(refresh-tasks profiles access-tokens))))

(defn- assoc-access-tokens-in-request [request tokens]
(if tokens
(assoc request :oauth2/access-tokens tokens)
request))

(defn- nil-session? [response]
(and (contains? response :session) (nil? (:session response))))

(defn- assoc-access-tokens-in-response [original-tokens updated-tokens response]
(if (or (nil-session? response) (= original-tokens updated-tokens))
response
(assoc-in response [:session ::access-tokens] updated-tokens)))

(defn- wrap-refresh-access-tokens [handler profiles]
(fn ([request]
(let [tokens (get-in request [:session ::access-tokens])
tokens' (refresh-all-tokens profiles tokens)
request (assoc-access-tokens-in-request request tokens')
response (handler request)]
(assoc-access-tokens-in-response tokens tokens' response)))
([request respond raise]
(let [tokens (get-in request [:session ::access-tokens])]
(refresh-all-tokens
profiles tokens
(fn [tokens']
(let [request (assoc-access-tokens-in-request request tokens')
respond #(respond (assoc-access-tokens-in-response
tokens tokens' %))]
(handler request respond raise))))))))

(defn- parse-redirect-url [{:keys [redirect-uri]}]
(.getPath (java.net.URI. redirect-uri)))

Expand All @@ -201,20 +293,21 @@

(defn wrap-oauth2 [handler profiles]
{:pre [(every? valid-profile? (vals profiles))]}
(let [profiles (for [[k v] profiles] (assoc v :id k))
launches (into {} (map (juxt :launch-uri identity)) profiles)
redirects (into {} (map (juxt parse-redirect-url identity)) profiles)]
(let [id-profiles (for [[k v] profiles] (assoc v :id k))
launches (into {} (map (juxt :launch-uri identity)) id-profiles)
redirects (into {} (map (juxt parse-redirect-url identity)) id-profiles)
handler (wrap-refresh-access-tokens handler profiles)]
(fn
([{:keys [uri] :as request}]
(if-let [profile (launches uri)]
((make-launch-handler profile) request)
(if-let [profile (redirects uri)]
((:redirect-handler profile (make-redirect-handler profile)) request)
(handler (assoc-access-tokens request)))))
(handler request))))
([{:keys [uri] :as request} respond raise]
(if-let [profile (launches uri)]
((make-launch-handler profile) request respond raise)
(if-let [profile (redirects uri)]
((:redirect-handler profile (make-redirect-handler profile))
request respond raise)
(handler (assoc-access-tokens request) respond raise)))))))
(handler request respond raise)))))))
132 changes: 130 additions & 2 deletions test/ring/middleware/oauth2_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,9 @@
b-ms (.getTime b)]
(< (- a-ms 1000) b-ms (+ a-ms 1000))))

(defn- seconds-from-now-to-date [secs]
(-> (Instant/now) (.plusSeconds secs) (Date/from)))
(defn- seconds-from-now-to-date
([now secs] (-> now (.plusSeconds secs) (Date/from)))
([secs] (seconds-from-now-to-date (Instant/now) secs)))

(deftest test-redirect-uri
(fake/with-fake-routes
Expand Down Expand Up @@ -390,3 +391,130 @@
(deref raise 100 :empty)))
(is (= {:status 200, :headers {}, :body tokens}
(deref respond 100 :empty)))))))

(def refresh-token-response
{:status 200
:headers {"Content-Type" "application/json"}
:body "{\"access_token\":\"newtoken\",\"expires_in\":3600,
\"refresh_token\":\"newrefresh\",\"foo\":\"bar\"}"})

(deftest test-token-refresh-success
(fake/with-fake-routes
{"https://example.com/oauth2/access-token"
(fn [req]
(let [params (codec/form-decode (slurp (:body req)))]
(is (= "refresh_token" (get params "grant_type")))
(is (= "oldrefresh" (get params "refresh_token")))
refresh-token-response))}

(let [now (Instant/now)
old-expires (seconds-from-now-to-date now -60)
new-expires (seconds-from-now-to-date now 3600)
new-token {:token "newtoken"
:refresh-token "newrefresh"
:extra-data {:foo "bar"}}
request (-> (mock/request :get "/")
(assoc :session
{::oauth2/access-tokens
{:test {:token "oldtoken"
:refresh-token "oldrefresh"
:expires old-expires}}}))]
(testing "sync refresh"
(let [response (test-handler request)]
(is (= 200 (:status response)))
;; then handler has new token
(is (= new-token (dissoc (get-in response [:body :test]) :expires)))
(is (approx-eq new-expires (get-in response [:body :test :expires])))
;; and the user's session is updated
(is (= new-token
(dissoc (get-in response
[:session ::oauth2/access-tokens :test])
:expires)))))
(testing "async refresh"
(let [respond (promise)
raise (promise)]
(test-handler request respond raise)
(is (= :empty (deref raise 100 :empty)))
(let [response (deref respond 100 :empty)]
;; then handler has new token
(is (not= response :empty))
(is (= new-token (dissoc (get-in response [:body :test]) :expires)))
;; user session is updated
(is (= new-token
(dissoc (get-in response [:session ::oauth2/access-tokens
:test])
:expires)))))))))

(def refresh-token-error-response
{:headers {"content-type" "application/json"},
:status 400,
:body "{\"error\": \"invalid_grant\"}"})

(deftest test-token-refresh-failure
(fake/with-fake-routes
{"https://example.com/oauth2/access-token"
(constantly refresh-token-error-response)}

;; setup a session with two grants, where one grant is expired and which
;; will error on refresh
(let [profiles {:test-0 test-profile :test-1 test-profile}
handler (wrap-oauth2 token-handler profiles)
good-grant {:token "good-token"
:refresh-token "refresh-token"
:expires (seconds-from-now-to-date 3600)}
expired-grant {:token "expired-token"
:refresh-token "invalid"
:expires (seconds-from-now-to-date -60)}
request (-> (mock/request :get "/")
(assoc :session
{::oauth2/access-tokens
{:test-0 expired-grant
:test-1 good-grant}}))]
(testing "sync handler"
(let [response (handler request)]
(is (= {:test-1 good-grant}
(:body response)))))
(testing "async refresh"
(let [respond (promise)
raise (promise)]
(handler request respond raise)
(is (= :empty (deref raise 100 :empty)))
(let [response (deref respond 100 :empty)]
(is (not= response :empty))
(is (= {:test-1 good-grant} (:body response)))))))))

(deftest test-token-refresh-clear-session
(fake/with-fake-routes
{"https://example.com/oauth2/access-token"
(constantly refresh-token-response)}

(let [clear-response {:status 200 :headers {} :body nil :session nil}
session-clear-handler (fn
([_request] clear-response)
([_request respond _raise]
(respond clear-response)))
handler (wrap-oauth2 session-clear-handler {:test test-profile})
now (Instant/now)
old-expires (seconds-from-now-to-date now -60)
request (-> (mock/request :get "/")
(assoc :session
{::oauth2/access-tokens
{:test {:token "oldtoken"
:refresh-token "oldrefresh"
:expires old-expires}}}))]

(testing "sync handler"
(let [response (handler request)]
(is (= 200 (:status response)))
(is (nil? (:session response)))))

(testing "async handler"
(let [respond (promise)
raise (promise)]
(handler request respond raise)
(let [response (deref respond 100 :empty)
error (deref raise 100 :empty)]
(is (not= :empty response))
(is (= :empty error))
(is (= 200 (:status response)))
(is (nil? (:session response)))))))))