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.
This commit is contained in:
kolaente 2026-06-19 17:52:45 +02:00
parent 6b4134a0ad
commit 6f4a21ddad
3 changed files with 60 additions and 1 deletions

View File

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

View File

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

View File

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