Skip to content
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

Refresh expired access tokens #56

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

Refresh expired access tokens #56

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
17 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
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
126 changes: 110 additions & 16 deletions src/ring/middleware/oauth2.clj
View file
Open in desktop
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))
Copy link
Owner

Choose a reason for hiding this comment

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

Why has request-params been moved out here? I don't understand how this change connects to the token refreshes. Can you explain?

Copy link
Author

Choose a reason for hiding this comment

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

Ah I should have left a comment for this change, it's a bit confusing: I am trying to re-use access-token-http-options for the refresh http request as they share the same URL and credentials logic, except for the form-params, hence i extracted it as a parameter (in place of constructing those from the request).

If you feel this is convoluted, a possible alternative would be to assoc the :form-params in the caller, same as the :async is currently set from outside.

Copy link
Owner

Choose a reason for hiding this comment

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

In which case, let's name things correctly. We can divide this into three functions:

  1. token-http-options to contain the common code
  2. access-token-http-options to operate as before
  3. refresh-token-http-options to contain the options for refreshing tokens.

That way things are named correctly, and it makes the code a little more understandable later on.

(assoc :async? true))
(comp respond format-access-token)
raise)))

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

(defn- assoc-access-tokens [request]
(if-let [tokens (-> request :session ::access-tokens)]
(defn- expired-access-tokens
[access-tokens]
Copy link
Owner

Choose a reason for hiding this comment

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

We can omit the newline before the argument list if it all fits on one line, e.g.

(defn- expired-access-tokens [access-tokens]

Is just as clear and uses less vertical whitespace.

(let [now (Date.)
expired-access-token? (fn [[_ {:keys [expires refresh-token]}]]
(and refresh-token expires
(.before expires now)))]
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we need to bother factoring the now out. It's not going to make much of a difference that I can see, and makes the code a little harder to parse. In addition, other parts of the code already use their own "now".

Copy link
Author

Choose a reason for hiding this comment

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

  • extracting it leaves expired-access-token? a pure function
  • there is a potentially confusing outcome where two tokens with identical expiration time take go different branches when expiration coincides with the evaluating of the filter

you are right that this has 0 practical impact, I will make the change tomorrow 😄

(->> access-tokens (filter expired-access-token?) (into {}))))

(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-one-token
([profile refresh-token]
(-> (access-token-http-options
profile
{:grant_type "refresh_token" :refresh_token refresh-token})
(assoc :socket-timeout refresh-socket-timeout)
http/request
format-access-token))
Copy link
Owner

Choose a reason for hiding this comment

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

I think this would be clearer:

(-> (http/request (refresh-token-http-options profile refresh-token))
    (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 refresh-socket-timeout)
(http/request (comp respond format-access-token) raise))))

(defn- refresh-all-tokens
([profiles access-tokens]
(let [refresh-results
(for [[profile-key {:keys [refresh-token]}] (expired-access-tokens
access-tokens)
:let [profile (profile-key profiles)]
:when (and profile 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 (expired-access-tokens 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 [[profile-key {:keys [refresh-token]}] expired
:let [profile (profile-key profiles)]
:when (and profile 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!))))))))

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

(defn- assoc-access-tokens-in-response
[response tokens]
(if tokens
(assoc-in response [:session ::access-tokens] tokens)
response))
Copy link
Owner

Choose a reason for hiding this comment

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

This assumes a :session key in the response map from the handler, which I don't believe is something you can rely on.


(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
[handler profiles]
Copy link
Owner

Choose a reason for hiding this comment

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

Newline can be omitted.

{: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)]
weavejester marked this conversation as resolved.
Show resolved Hide resolved
(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])
(refresh-all-tokens profiles))]
(-> request
(assoc-access-tokens-in-request access-tokens)
handler
(assoc-access-tokens-in-response access-tokens))))))
Copy link
Owner

Choose a reason for hiding this comment

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

This could probably be factored out into it's own function. So something like:

(defn- wrap-refresh-access-tokens [handler profiles]
  (fn
    ([request] ...)
    ([request respond raise] ...)))

It should also only update the session if access tokens have expired or refreshed, otherwise the session should not be touched.

Copy link
Author

Choose a reason for hiding this comment

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

I am not sure updating the session in the response if changed is a good idea, as for users this might be rather surprising: If they previously relied on the :session key remaining unset, the breaking behavior only occurs when a token expires, which might be rather hard to debug.

But now that I think about this, we have to be careful as to not prevent users from implementing logout in handler. This could be addressed by checking if :session is explicitly set to nil, in which case we don't touch it.

Copy link
Owner

Choose a reason for hiding this comment

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

The rules that govern the session middleware are:

  • If the response :session key is missing the session is unaltered.
  • If the response :session key is nil, the session is removed.
  • If the response :session key is a map, it replaces the current session.

While it's possible to always replace the session with an identical result, it's not necessary and adds a session write command that may not be necessary. This might be a database update, for example. For that reason we need to only write the session if we want to alter it.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the clarification; The performance overhead is a strong point and I have changed the behavior to only update the :session in the response if the access tokens have changed and if the handler did not already set the session.

([{: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 [access-tokens]
(handler
(assoc-access-tokens-in-request
request access-tokens)
(comp respond
#(assoc-access-tokens-in-response
% access-tokens))
raise))]
(refresh-all-tokens profiles access-tokens respond))))))))
107 changes: 101 additions & 6 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 @@ -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,95 @@
(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)))))))))