Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
30 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
6a3081c
correctly handle extra state from application
Dec 19, 2025
1ea3ae4
format unit tests to 80 char width
Dec 19, 2025
2efc5fa
catch generic exception on sync refresh
Dec 19, 2025
b13273e
refactor `cond` in `assoc-access-tokens-in-response`
Dec 19, 2025
690dd3f
remove timeout on refresh request
Dec 19, 2025
265759a
extract error handling form `async-map-values`
Dec 19, 2025
db75c05
add a \n to test
Dec 19, 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
144 changes: 128 additions & 16 deletions src/ring/middleware/oauth2.clj
Original file line number Diff line number Diff line change
Expand Up @@ -114,26 +114,29 @@

(defn- access-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- get-access-token
([profile request]
(-> (http/request (access-token-http-options profile request))
(-> (access-token-http-options profile (request-params profile request))
http/request
(format-access-token)))
([profile request respond raise]
(http/request (-> (access-token-http-options profile request)
(assoc :async? true))
(comp respond format-access-token)
raise)))
(http/request
(-> (access-token-http-options profile
(request-params profile request))
(assoc :async? true))
(comp respond format-access-token)
raise)))

(defn state-mismatch-handler
([_]
Expand Down Expand Up @@ -188,33 +191,142 @@
(respond (redirect-response profile session token)))
raise)))))

(defn- assoc-access-tokens [request]
(if-let [tokens (-> request :session ::access-tokens)]
(defn- get-expired
"Returns expired profile keys and refresh tokens in `access-tokens`."
[access-tokens]
(let [now (new Date)]
(for [[profile-key {:keys [expires refresh-token]}] access-tokens
:when (and expires refresh-token (.before expires now))]
{:profile-key profile-key :refresh-token refresh-token})))

(defn- update-tokens
"If `maybe-grant` is nil, removes `profile-key` from `access-token; otherwise
merges `profile-key` with `maybe-grant`."
[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 socket-timeout 60000)

(defn- refresh-one-token
([profile refresh-token]
(-> (access-token-http-options
profile
{:grant_type "refresh_token" :refresh_token refresh-token})
(assoc :socket-timeout socket-timeout)
http/request
format-access-token))
([profile refresh-token respond raise]
(-> (access-token-http-options
profile
{:grant_type "refresh_token"
:refresh_token refresh-token})
(assoc :async? true
:socket-timeout socket-timeout)
(http/request (comp respond format-access-token) raise))))

(defn- valid-token? [token]
(and token (string? token) (not (str/blank? token))))

(defn- refresh-all-tokens
"Refreshes all expired tokens, yielding an updated map of tokens"
([profiles access-tokens]
(let [refresh-results
(for [{:keys [profile-key refresh-token]} (get-expired access-tokens)
:let [profile (profile-key profiles)]
:when (and profile (valid-token? refresh-token))]
[profile-key
(try (refresh-one-token profile refresh-token)
(catch clojure.lang.ExceptionInfo _
nil))])]
(reduce update-tokens access-tokens refresh-results)))
([profiles access-tokens respond]
;; strategy: launch all requests concurrently, keeping track of completed
;; requests in `results`. When all requests have finished, respond.
(let [expired (get-expired access-tokens)
total (count expired)
results (atom {}) ;; map from profile-key to result
respond-when-done! #(when (= (count @results) total)
(respond (reduce update-tokens access-tokens @results)))]
(if (zero? total)
(respond access-tokens)
(doseq [{:keys [profile-key refresh-token]} expired
:let [profile (profile-key profiles)]
:when (and profile (valid-token? refresh-token))]
(refresh-one-token profile refresh-token
(fn [refresh-result]
(swap! results assoc profile-key refresh-result)
(respond-when-done!))
(fn [_]
(swap! results assoc profile-key nil)
(respond-when-done!))))))))
Copy link
Owner

Choose a reason for hiding this comment

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

It feels as if this function is attempting to do a lot, as we can see from it's length! I think we could split this up into three logical steps:

  1. Collect the expired access tokens
  2. Send them off via HTTP to be refreshed
  3. Update the access token map

Only step 2 is different for sync/async. What we essentially want here is a way of running a bunch of async functions in parallel, and then to collect the results. That way we can have a function like:

(defn- refresh-expired-tokens
  ([profiles access-tokens]
    (pmap (partial refresh-token profiles)
          (expired-access-tokens access-tokens)))
  ([profiles access-tokens respond raise]
    (amap (partial refresh-token profiles)
          (expired-access-tokens access-tokens)
          respond raise)))

If map (and pmap) expect a function (f x), then amap would expect a function (f x respond raise).

I'm running out of review time so I don't have enough time to sketch out the implementation of amap, but if I get time later I'll go ahead and do so.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, you are right that this is a giant function; I usually refrain from writing novel combinators, but you are right that here I have an implementation of a ring-style-async-map that could be extracted.

I will give it a try later to see if I can simplify it.

I am not sure about the use of pmap, i do not think it is worth it to spawn a thread when in almost all cases only a single authentication provider is used.

Copy link
Author

Choose a reason for hiding this comment

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

I gave it a try, but ultimately decided not to implement a generic async map; there are a handful complications which I feel are not worth the headache of generalization (error handling, preserving order); the most recent commit adds a less ambitious, specialized version that works for associative containers and assigns nil to a key on failure. This does greatly simplify refresh-all-tokens though, so I hope this addresses your concern 😄


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

(defn- assoc-access-tokens-in-response
"If any tokens are present, adds to them the `:session` key of `response`."
[response tokens]
(if tokens
(assoc-in response [:session ::access-tokens] tokens)
response))

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

(defn- valid-profile? [{:keys [client-id client-secret]}]
(and (some? client-id) (some? client-secret)))

(defn wrap-oauth2 [handler profiles]
(defn wrap-oauth2
"Middleware that handles OAuth2 authentication flows.

Parameters:
* `handler`: The downstream ring handler
* `profiles`: A map of profiles

Each request URI is matched against the profiles to determine the appropriate
OAuth2 flow handler. If no match is found, the request is passed to the
downstream handler with existing access tokens added to the request under the
`:oauth2/access-tokens` key.

Expired tokens are refreshed using their refresh-token if possible. If refresh
fails, the access token is removed."
[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)]
(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)))))
(let [access-tokens (get-in request [:session ::access-tokens])
refreshed-tokens (refresh-all-tokens profiles access-tokens)]
(-> request
(assoc-access-tokens-in-request refreshed-tokens)
handler
(assoc-access-tokens-in-response refreshed-tokens))))))
Copy link
Owner

Choose a reason for hiding this comment

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

What if there are no refreshed tokens?

Copy link
Author

Choose a reason for hiding this comment

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

i assume the naming was too confusing here --- the refreshed-tokens contain both the newly refreshed tokens as well as the existing, non-expired tokens.

([{: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)))))))
(let [access-tokens (get-in request [:session ::access-tokens])
respond (fn [refreshed-tokens]
(handler
(assoc-access-tokens-in-request
request refreshed-tokens)
(comp respond
#(assoc-access-tokens-in-response
% refreshed-tokens))
raise))]
(refresh-all-tokens profiles access-tokens respond))))))))
105 changes: 99 additions & 6 deletions test/ring/middleware/oauth2_test.clj
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 @@ -203,7 +204,10 @@

(deftest test-access-tokens-key
(let [tokens {:test {:token "defdef", :expires 3600}}]
(is (= {:status 200, :headers {}, :body tokens}
(is (= {:status 200,
:headers {},
:body tokens,
:session {::oauth2/access-tokens tokens}}
(-> (mock/request :get "/")
(assoc :session {::oauth2/access-tokens tokens})
(test-handler))))))
Expand Down Expand Up @@ -376,10 +380,11 @@

(deftest test-handler-passthrough
(let [tokens {:test "tttkkkk"}
session {::oauth2/access-tokens tokens}
request (-> (mock/request :get "/example")
(assoc :session {::oauth2/access-tokens tokens}))]
(assoc :session session))]
(testing "sync handler"
(is (= {:status 200, :headers {}, :body tokens}
(is (= {:status 200, :headers {}, :body tokens :session session}
(test-handler request))))

(testing "async handler"
Expand All @@ -388,5 +393,93 @@
(test-handler request respond raise)
(is (= :empty
(deref raise 100 :empty)))
(is (= {:status 200, :headers {}, :body tokens}
(is (= {:status 200, :headers {}, :body tokens :session session}
(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 (= {:test-1 good-grant} (:body response)))))))))
Loading