Refresh expired access tokens #56

studer-l · 2025-01-10T19:00:21Z

Adds an additional flow to the existing middleware to refresh expired
tokens. The implementation handles multiple active grants. Expired
grants that failed to refresh are removed.

The refresh workflow is unconditionally activated. Since a token refresh
may occur for any request, access tokens are now always added to the
response's :session. This may break code which previously relied on the
:session only being set during the initial grant workflow. I do not
think this can be avoided.

If a refresh occurs, the tokens in (:session request) are left as-is,
the updated access tokens are accessibly via the existing
:oauth2/access-token key. This allows downstream handlers to observe
that a token refresh occurred.

There is a potential bug, where concurrent requests with expired tokens
may race. For example, consider a page containing a css and a js
resource. If a user's access token was to expire exactly as the
index.html finishes loading, their browser may concurrently fetch both
the css and js resources, triggering two concurrent token refresh
attempts with the same token, one of which may fail. I do not see a way to
address this without introducing considerable complexity.

I have added a timeout of 60 seconds to the refresh http request, which
means a slow oauth backend will cause users to become logged out. I
think this is more informative for users than hanging forever.

Fixes #40

Adds an additional flow to the existing middleware to refresh expired tokens. The implementation handles multiple active grants. Expired grants that failed to refresh are removed. The refresh workflow is unconditionally activated. Since a token refresh may occur for any request, access tokens are now always added to the response's `:session`. This may break code which previously relied on the `:session` only being set during the initial grant workflow. I do not think this can be avoided. If a refresh occurs, the tokens in `(:session request)` are left as-is, the updated access tokens are accessibly via the existing `:oauth2/access-token` key. This allows downstream handlers to observe that a token refresh occurred. There is a potential bug, where concurrent requests with expired tokens may race. For example, consider a page containing a `css` and a `js` resource. If a user's access token was to expire exactly as the `index.html` finishes loading, their browser may concurrently fetch both the `css` and `js` resources, triggering two concurrent token refresh attempts with the same token, one of which may fail. I do not see a way to address this without introducing considerable complexity. I have added a timeout of 60 seconds to the refresh http request, which means a slow oauth backend will cause users to become logged out. I think this is more informative for users than hanging forever. Fixes weavejester#40

weavejester · 2025-01-10T20:44:29Z

Thanks for the PR. This might take a while for me to get through. Please ensure that the commit contains only changes relevant to this feature, that all lines are within 80 characters, and that only public functions have docstrings.

weavejester

If I have time I'll do a more thorough review a little later, but these are my initial thoughts.

weavejester · 2025-01-10T20:46:02Z

src/ring/middleware/oauth2.clj

-                 raise)))
+   (http/request
+    (-> (access-token-http-options profile
+                                   (request-params profile request))


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

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.

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

token-http-options to contain the common code

access-token-http-options to operate as before

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.

weavejester · 2025-01-10T20:48:23Z

src/ring/middleware/oauth2.clj

@@ -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


Generally speaking, we should avoid using 'get-' as part of pure function names. Instead expired-access-tokens would likely be a better name.

weavejester · 2025-01-10T20:49:01Z

src/ring/middleware/oauth2.clj

-(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`."


Docstrings on private functions should be avoided.

weavejester · 2025-01-10T20:49:17Z

src/ring/middleware/oauth2.clj

+(defn- get-expired
+  "Returns expired profile keys and refresh tokens in `access-tokens`."
+  [access-tokens]
+  (let [now (new Date)]


Prefer (Date.) over (new Date).

weavejester · 2025-01-10T20:59:36Z

src/ring/middleware/oauth2.clj

+    (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})))


Perhaps something like:

(defn- expired-access-token? [[k {:keys [expires]}]] (and expires (.before expires (Date.)))) (defn- expired-access-tokens [access-tokens] (into {} (filter expired-access-token?) access-tokens)

That way we're just filtering the map and the output type is the same as the input type.

very good point! indeed this is just a filter

weavejester · 2025-01-10T21:01:05Z

src/ring/middleware/oauth2.clj

+       (http/request (comp respond format-access-token) raise))))
+
+(defn- valid-token? [token]
+  (and token (string? token) (not (str/blank? token))))


Do we need this check? I'm unsure of its purpose.

from my reading of the RFC, we should check whether the token is set, since it is optional for the authorizing server to provide one --- the other two predicates are... just garnish, and unlikely to do anything, so I did remove them and inline it

weavejester · 2025-01-10T21:20:09Z

src/ring/middleware/oauth2.clj

+(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!))))))))


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:

Collect the expired access tokens

Send them off via HTTP to be refreshed

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.

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.

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 😄

weavejester · 2025-01-10T21:21:33Z

src/ring/middleware/oauth2.clj

+  "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."


This docstring addition should be part of another commit ideally.

src/ring/middleware/oauth2.clj

weavejester · 2025-01-10T21:23:05Z

src/ring/middleware/oauth2.clj

+                 refreshed-tokens (refresh-all-tokens profiles access-tokens)]
+             (-> request
+                 (assoc-access-tokens-in-request refreshed-tokens)
+                 handler
+                 (assoc-access-tokens-in-response refreshed-tokens))))))


What if there are no refreshed tokens?

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.

studer-l

Hi @weavejester thanks for the very prompt and thorough review 😄

I will see about doing some more work to refactor the giant function you mentioned, don't hesitate to let me know if anything else looks clunky 😅

studer-l · 2025-01-12T18:04:02Z

src/ring/middleware/oauth2.clj

+    (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})))


very good point! indeed this is just a filter

studer-l · 2025-01-12T18:04:56Z

src/ring/middleware/oauth2.clj

+       (http/request (comp respond format-access-token) raise))))
+
+(defn- valid-token? [token]
+  (and token (string? token) (not (str/blank? token))))


from my reading of the RFC, we should check whether the token is set, since it is optional for the authorizing server to provide one --- the other two predicates are... just garnish, and unlikely to do anything, so I did remove them and inline it

studer-l · 2025-01-12T18:14:05Z

src/ring/middleware/oauth2.clj

+(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!))))))))


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.

studer-l · 2025-01-12T18:15:10Z

src/ring/middleware/oauth2.clj

+                 refreshed-tokens (refresh-all-tokens profiles access-tokens)]
+             (-> request
+                 (assoc-access-tokens-in-request refreshed-tokens)
+                 handler
+                 (assoc-access-tokens-in-response refreshed-tokens))))))


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.

studer-l · 2025-01-12T18:16:16Z

src/ring/middleware/oauth2.clj

-                 raise)))
+   (http/request
+    (-> (access-token-http-options profile
+                                   (request-params profile request))


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.

src/ring/middleware/oauth2.clj

weavejester

Thank you very much for the updates! I have some additional feedback, but we're getting there. One thing that does need to be taken account of is what happens if the handler doesn't return a session. In that case, we should take the session from the request instead.

weavejester · 2025-01-15T15:32:24Z

src/ring/middleware/oauth2.clj

-                 raise)))
+   (http/request
+    (-> (access-token-http-options profile
+                                   (request-params profile request))


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

token-http-options to contain the common code

access-token-http-options to operate as before

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.

weavejester · 2025-01-15T15:35:23Z

src/ring/middleware/oauth2.clj

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


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.

weavejester · 2025-01-15T15:37:05Z

src/ring/middleware/oauth2.clj

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


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".

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 😄

weavejester · 2025-01-15T15:49:11Z

src/ring/middleware/oauth2.clj

+   (-> (access-token-http-options
+        profile
+        {:grant_type "refresh_token" :refresh_token refresh-token})
+       (assoc :socket-timeout refresh-socket-timeout)
+       http/request
+       format-access-token))


I think this would be clearer:

(-> (http/request (refresh-token-http-options profile refresh-token)) (format-access-token))

weavejester · 2025-01-15T16:00:25Z

src/ring/middleware/oauth2.clj

+           (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))))))


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.

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.

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.

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.

weavejester · 2025-01-15T16:01:04Z

src/ring/middleware/oauth2.clj

+(defn- assoc-access-tokens-in-response
+  [response tokens]
+  (if tokens
+    (assoc-in response [:session ::access-tokens] tokens)
+    response))


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

src/ring/middleware/oauth2.clj

weavejester · 2025-01-15T16:27:24Z

src/ring/middleware/oauth2.clj

+(defn wrap-oauth2
+  [handler profiles]


Newline can be omitted.

- extract `wrap-refresh-acceess-tokens` - only set response `:session` if changed and if handler did not set it already - add a test for the above

weavejester · 2025-01-24T04:48:04Z

src/ring/middleware/oauth2.clj

    (assoc request :oauth2/access-tokens tokens)
    request))

+(defn- assoc-access-tokens-in-response
+  [original-tokens updated-tokens response]
+  (if (and (not (contains? response :session))


This doesn't look quite correct. I believe it should be:

(and (not (and (contains? response :session) (nil? (:session response)))) (not= original-tokens updated-tokens))

Or more clearly:

(and (not (nil-session? response)) (not= original-tokens updated-tokens))

In that the only time we don't want to update the response is if the tokens haven't changed, or the session has been set explicitly to nil and is therefore being deleted.

Should be addressed in 26f2b34, with de morgan's law and branch order flipped

ieugen · 2025-02-12T11:08:35Z

hi @studer-l ,

I'm working on an app that uses oauth2 and I would need token refresh - since I have 5 min tokens.
Do you have time to finish this PR?
I'll see if I can implement and test it.
Please ping me when you have anything new pushed.

Thanks,
Eugen

studer-l · 2025-02-12T22:38:00Z

hi @studer-l ,

I'm working on an app that uses oauth2 and I would need token refresh - since I have 5 min tokens. Do you have time to finish this PR? I'll see if I can implement and test it. Please ping me when you have anything new pushed.

Thanks, Eugen

Hi @ieugen
Thank you for pinging me about this, sorry for letting this pull request linger; I should be able to find the time to finish this, no worries.

weavejester requested changes Jan 10, 2025

View reviewed changes

Lukas Studer added 8 commits January 12, 2025 18:13

reflow to 80 characters per line

9f0c3f4

rename get-expired → expired-access-tokens

0a0b106

remove docstrings on non-public functions; remove unrelated changes

cbfa368

prefer (Date.) over (new Date)

ba38ad2

remove token type check

65e81fc

refactor expired-access-tokens as filter

5327380

fix confusing naming

448624e

rename socket-timeout constant to be more specific

1c0b548

studer-l commented Jan 12, 2025

View reviewed changes

refactor refresh-all-tokens

94ad054

weavejester requested changes Jan 15, 2025

View reviewed changes

Lukas Studer added 6 commits January 15, 2025 19:57

refactor http param options

9339b82

remove spurious newlines in defn

e8d25b9

inline now

67c8543

slightly refactor refresh-one-token for clarity

b0c3f83

rework token refresh into a middle function

a4f5e2c

- extract `wrap-refresh-acceess-tokens` - only set response `:session` if changed and if handler did not set it already - add a test for the above

minor refactor

069e4b5

weavejester reviewed Jan 24, 2025

View reviewed changes

update response session unless nil or tokens unchanged

26f2b34

Refresh expired access tokens #56

Are you sure you want to change the base?

Refresh expired access tokens #56

Conversation

studer-l commented Jan 10, 2025 • edited Loading

weavejester commented Jan 10, 2025

weavejester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

studer-l left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weavejester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ieugen commented Feb 12, 2025

studer-l commented Feb 12, 2025

studer-l commented Jan 10, 2025 •

edited

Loading