From 37cd45d5658c48623b0b2dcad012b707e6b2700a Mon Sep 17 00:00:00 2001 From: kolaente Date: Fri, 19 Jun 2026 17:23:32 +0200 Subject: [PATCH] fix(auth): cache OIDC end-session endpoint at init so logout never blocks on discovery EndSessionEndpoint() previously called setOicdProvider() (which retries with backoff) and re-unmarshalled the discovery document on every logout. Read the end_session_endpoint once at provider init, store it on the provider, and make the accessor fail-fast (cached value, else the static logouturl) so /user/logout stays responsive even when the OP is unreachable. --- pkg/modules/auth/openid/logout.go | 27 ++++++++++----- pkg/modules/auth/openid/logout_test.go | 47 ++++++++++++++++++++++++++ pkg/modules/auth/openid/openid.go | 9 +++-- pkg/modules/auth/openid/providers.go | 2 ++ 4 files changed, 74 insertions(+), 11 deletions(-) diff --git a/pkg/modules/auth/openid/logout.go b/pkg/modules/auth/openid/logout.go index 5a8088b58..e907d38c8 100644 --- a/pkg/modules/auth/openid/logout.go +++ b/pkg/modules/auth/openid/logout.go @@ -24,16 +24,28 @@ import ( "code.vikunja.io/api/pkg/models" ) -// EndSessionEndpoint returns the provider's RP-Initiated Logout endpoint as -// published in its OpenID Connect discovery document (the REQUIRED +// EndSessionEndpoint returns the provider's RP-Initiated Logout endpoint, read +// from the discovery document once at init time (EndSessionURL, the REQUIRED // `end_session_endpoint` metadata, RP-Initiated Logout 1.0 ยง2.1). When the // provider does not publish one, it falls back to the statically configured // `logouturl` so existing setups keep working. +// +// It deliberately never triggers discovery: logout must stay responsive even +// when the OP is unreachable (e.g. right after an API restart, before the +// provider has been re-discovered). func (p *Provider) EndSessionEndpoint() string { + if p.EndSessionURL != "" { + return p.EndSessionURL + } + return p.LogoutURL +} + +// discoveredEndSessionEndpoint reads the `end_session_endpoint` from the already +// fetched discovery document. The discovery JSON is cached on the *oidc.Provider +// by oidc.NewProvider, so Claims only unmarshals in memory and makes no request. +func (p *Provider) discoveredEndSessionEndpoint() string { if p.openIDProvider == nil { - if err := p.setOicdProvider(); err != nil { - return p.LogoutURL - } + return "" } var meta struct { @@ -41,10 +53,7 @@ func (p *Provider) EndSessionEndpoint() string { } if err := p.openIDProvider.Claims(&meta); err != nil { log.Debugf("Could not read end_session_endpoint for provider %s: %v", p.Key, err) - return p.LogoutURL - } - if meta.EndSessionEndpoint == "" { - return p.LogoutURL + return "" } return meta.EndSessionEndpoint } diff --git a/pkg/modules/auth/openid/logout_test.go b/pkg/modules/auth/openid/logout_test.go index 132bd8996..ccbc5df74 100644 --- a/pkg/modules/auth/openid/logout_test.go +++ b/pkg/modules/auth/openid/logout_test.go @@ -131,6 +131,53 @@ func TestBuildEndSessionURLFromDiscovery(t *testing.T) { 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 + // from the cached EndSessionURL and never attempt discovery, so logout stays + // responsive. + p := &Provider{ + Key: "provider1", + LogoutURL: "https://op.example.com/static-logout", + EndSessionURL: "https://op.example.com/end-session", + } + assert.Equal(t, "https://op.example.com/end-session", p.EndSessionEndpoint()) +} + +func TestEndSessionEndpointFallsBackToLogoutURLWhenNotCached(t *testing.T) { + p := &Provider{ + Key: "provider1", + LogoutURL: "https://op.example.com/static-logout", + } + assert.Equal(t, "https://op.example.com/static-logout", p.EndSessionEndpoint()) +} + +func TestEndSessionEndpointCachedFromDiscoveryOnInit(t *testing.T) { + defer CleanupSavedOpenIDProviders() + + server := newMockOIDCServerWithEndSession() + defer server.Close() + + config.AuthOpenIDEnabled.Set(true) + config.AuthOpenIDProviders.Set(map[string]interface{}{ + "provider1": map[string]interface{}{ + "name": "Provider One", + "authurl": server.URL, + "clientid": "client1", + "clientsecret": "secret1", + }, + }) + _ = keyvalue.Del("openid_providers") + _ = keyvalue.Del("openid_provider_provider1") + + provider, err := GetProvider("provider1") + require.NoError(t, err) + require.NotNil(t, provider) + + assert.Equal(t, server.URL+"/logout", provider.EndSessionURL) + assert.Equal(t, server.URL+"/logout", provider.EndSessionEndpoint()) +} + func TestEndSessionEndpointFallsBackToStaticLogoutURL(t *testing.T) { defer CleanupSavedOpenIDProviders() diff --git a/pkg/modules/auth/openid/openid.go b/pkg/modules/auth/openid/openid.go index ac5ba5780..bbf1382a5 100644 --- a/pkg/modules/auth/openid/openid.go +++ b/pkg/modules/auth/openid/openid.go @@ -69,8 +69,13 @@ type Provider struct { ForceUserInfo bool `json:"force_user_info"` RequireAvailability bool `json:"-"` ClientSecret string `json:"-"` - openIDProvider *oidc.Provider - Oauth2Config *oauth2.Config `json:"-"` + // EndSessionURL is the provider's RP-Initiated Logout endpoint, read once + // from the discovery document at init time so logout never triggers a fetch. + // Exported so it survives the gob-encoded redis keyvalue round-trip (gob + // skips unexported fields, like openIDProvider). + EndSessionURL string `json:"-"` + openIDProvider *oidc.Provider + Oauth2Config *oauth2.Config `json:"-"` } type claims struct { diff --git a/pkg/modules/auth/openid/providers.go b/pkg/modules/auth/openid/providers.go index 30f534ad9..366f54190 100644 --- a/pkg/modules/auth/openid/providers.go +++ b/pkg/modules/auth/openid/providers.go @@ -313,6 +313,8 @@ func getProviderFromMap(pi map[string]interface{}, key string) (provider *Provid provider.AuthURL = provider.Oauth2Config.Endpoint.AuthURL + provider.EndSessionURL = provider.discoveredEndSessionEndpoint() + return }