From 6f4a21ddad077bc35db6973dc0dc1e2000b6ffb9 Mon Sep 17 00:00:00 2001
From: kolaente <k@knt.li>
Date: Fri, 19 Jun 2026 17:52:45 +0200
Subject: [PATCH] fix(auth): read cached provider for OIDC logout so it never
 blocks on discovery

BuildEndSessionURL called GetProvider, which unconditionally runs
setOicdProvider -> oidc.NewProvider, a live HTTP GET of the OP's
discovery document (with RetryWithBackoff). On every logout this is an
unnecessary round-trip, and when the OP is unreachable the retries make
/user/logout hang, defeating the "logout stays responsive" guarantee.

The logout path only needs static fields already cached on the provider
(EndSessionURL/LogoutURL/ClientID); it never touches the live
openIDProvider/Oauth2Config. Add a cache-only getCachedProvider accessor
(GetProvider without the trailing setOicdProvider) and have
BuildEndSessionURL use it.

Login/callback keep using GetProvider: token exchange and ID-token
verification legitimately need the live-provider rehydration.
---
 pkg/modules/auth/openid/logout.go      |  5 ++++-
 pkg/modules/auth/openid/logout_test.go | 31 ++++++++++++++++++++++++++
 pkg/modules/auth/openid/providers.go   | 25 +++++++++++++++++++++
 3 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/pkg/modules/auth/openid/logout.go b/pkg/modules/auth/openid/logout.go
index e907d38c8..ed354c29e 100644
--- a/pkg/modules/auth/openid/logout.go
+++ b/pkg/modules/auth/openid/logout.go
@@ -74,7 +74,10 @@ func (p *Provider) discoveredEndSessionEndpoint() string {
 // It returns "" (and the caller skips the redirect) when neither an
 // end_session_endpoint nor a static logouturl is configured.
 func BuildEndSessionURL(providerKey string, oidc *models.SessionOIDCData) (string, error) {
-	provider, err := GetProvider(providerKey)
+	// Read the cached provider rather than GetProvider: logout must never trigger
+	// OIDC discovery (a live HTTP GET that retries/blocks when the OP is down), and
+	// the static EndSessionURL/LogoutURL/ClientID needed here are already cached.
+	provider, err := getCachedProvider(providerKey)
 	if err != nil {
 		return "", err
 	}
diff --git a/pkg/modules/auth/openid/logout_test.go b/pkg/modules/auth/openid/logout_test.go
index ccbc5df74..7b08116ca 100644
--- a/pkg/modules/auth/openid/logout_test.go
+++ b/pkg/modules/auth/openid/logout_test.go
@@ -131,6 +131,37 @@ func TestBuildEndSessionURLFromDiscovery(t *testing.T) {
 	assert.Equal(t, "client1", q.Get("client_id"))
 }
 
+func TestBuildEndSessionURLFromCachedProviderWithoutLiveObject(t *testing.T) {
+	defer CleanupSavedOpenIDProviders()
+
+	config.AuthOpenIDEnabled.Set(true)
+	config.ServicePublicURL.Set("https://vikunja.example.com/")
+
+	// Seed only the cached static fields, mimicking a provider restored from
+	// keyvalue whose OP is unreachable (no live openIDProvider). BuildEndSessionURL
+	// must build the logout URL from the cache without triggering discovery.
+	_ = keyvalue.Del("openid_providers")
+	require.NoError(t, keyvalue.Put("openid_provider_provider1", &Provider{
+		Key:           "provider1",
+		ClientID:      "client1",
+		EndSessionURL: "https://op.example.com/end-session",
+	}))
+
+	got, err := BuildEndSessionURL("provider1", &models.SessionOIDCData{
+		IDToken:     "raw-id-token",
+		ProviderKey: "provider1",
+	})
+	require.NoError(t, err)
+
+	u, err := url.Parse(got)
+	require.NoError(t, err)
+	q := u.Query()
+	assert.Equal(t, "https://op.example.com/end-session", u.Scheme+"://"+u.Host+u.Path)
+	assert.Equal(t, "raw-id-token", q.Get("id_token_hint"))
+	assert.Equal(t, "https://vikunja.example.com/", q.Get("post_logout_redirect_uri"))
+	assert.Equal(t, "client1", q.Get("client_id"))
+}
+
 func TestEndSessionEndpointUsesCachedURLWithoutDiscovery(t *testing.T) {
 	// A nil openIDProvider models a provider restored from the keyvalue cache
 	// (or one whose OP is currently unreachable). EndSessionEndpoint must answer
diff --git a/pkg/modules/auth/openid/providers.go b/pkg/modules/auth/openid/providers.go
index 366f54190..4aa4a44e7 100644
--- a/pkg/modules/auth/openid/providers.go
+++ b/pkg/modules/auth/openid/providers.go
@@ -180,6 +180,31 @@ func GetProvider(key string) (provider *Provider, err error) {
 	return
 }
 
+// getCachedProvider returns the provider from keyvalue WITHOUT re-establishing the
+// live OIDC connection (setOicdProvider). The logout path only needs the cached
+// static fields (EndSessionURL/LogoutURL/ClientID) and must not block on an
+// unreachable OP. The cache is populated at init / first GetAllProviders.
+func getCachedProvider(key string) (provider *Provider, err error) {
+	provider = &Provider{}
+	exists, err := keyvalue.GetWithValue("openid_provider_"+key, provider)
+	if err != nil {
+		return nil, err
+	}
+	if !exists {
+		_, err = GetAllProviders() // This will put all providers in cache
+		if err != nil {
+			return nil, err
+		}
+
+		_, err = keyvalue.GetWithValue("openid_provider_"+key, provider)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	return provider, nil
+}
+
 // parseBoolField reads a boolean-valued config field from a provider map,
 // tolerating both native bools (from YAML/JSON) and strings (from env vars or
 // the GetConfigValueFromFile path, which always return strings). Missing or