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