erlef / oidcc

OpenId Connect client library in Erlang & Elixir
https://hexdocs.pm/oidcc
Apache License 2.0
183 stars 49 forks source link

Prevent jwks_expired flood in worker 2nd try #365

Closed maennchen closed 3 months ago

maennchen commented 3 months ago

Fixes #359

maennchen commented 3 months ago

@asabil There's multiple ways a config / jwk is refreshed.

In all cases when a refresh happens, the timeout has to be reset. Additionally, a refresh of the config triggers a refresh of the jwks as well.

Since not all refreshes are triggered by the timeouts, we have to reset the timers, no?

One thing i was thinking about now is if there needs to be a strict dependency between the config and the jwks. If the config does not change the jwk uri, the jwks do not need to be refreshed as long as they haven't expired in the meantime.

Even with that change, we still have to cancel the timers on any not timeout based refresh.

Edit: Something along the line of this:

diff --git a/src/oidcc_provider_configuration_worker.erl b/src/oidcc_provider_configuration_worker.erl
index b0105bb..9e6d459 100644
--- a/src/oidcc_provider_configuration_worker.erl
+++ b/src/oidcc_provider_configuration_worker.erl
@@ -168,15 +168,14 @@ handle_continue(
     load_configuration,
     #state{
         issuer = Issuer,
+        provider_configuration = OldProviderConfiguration,
         provider_configuration_opts = ProviderConfigurationOpts,
-        configuration_refresh_timer = OldConfigurationRefreshTimer,
-        jwks_refresh_timer = OldJwksRefreshTimer,
+        configuration_refresh_timer = RefreshTimer,
         ets_table = EtsTable
     } =
         State
 ) ->
-    maybe_cancel_timer(OldConfigurationRefreshTimer),
-    maybe_cancel_timer(OldJwksRefreshTimer),
+    maybe_cancel_timer(RefreshTimer),

     maybe
         {ok, {Configuration, Expiry}} ?=
@@ -184,15 +183,18 @@ handle_continue(
                 Issuer,
                 ProviderConfigurationOpts
             ),
+        #oidcc_provider_configuration{jwks_uri = JwksUri} = Configuration,
         {ok, NewTimer} = timer:send_after(Expiry, configuration_expired),
         ok = store_in_ets(EtsTable, provider_configuration, Configuration),
-        {noreply,
-            State#state{
-                provider_configuration = Configuration,
-                configuration_refresh_timer = NewTimer,
-                jwks_refresh_timer = undefined
-            },
-            {continue, load_jwks}}
+        NewState = State#state{
+            provider_configuration = Configuration,
+            configuration_refresh_timer = NewTimer
+        },
+        case OldProviderConfiguration of
+            undefined -> {noreply, NewState, {continue, load_jwks}};
+            #oidcc_provider_configuration{jwks_uri = JwksUri} -> {noreply, NewState};
+            #oidcc_provider_configuration{} -> {noreply, NewState, {continue, load_jwks}}
+        end
     else
         {error, Reason} -> handle_backoff_retry(configuration_load_failed, Reason, State)
     end;

Edit 2: We could possible store a ref fot the config and the jwks. Instead of triggering refresh_[configuration|jwks], we could trigger {refresh_[configuration|jwks], Ref} this way we could discard timeouts for older loads. But I don't think that would be prettier overall.

maennchen commented 3 months ago

Going with edit 1 of comment.