diff --git a/frontend/src/stores/auth.ts b/frontend/src/stores/auth.ts index c2a682eac..e9a7c8ee0 100644 --- a/frontend/src/stores/auth.ts +++ b/frontend/src/stores/auth.ts @@ -533,8 +533,6 @@ export const useAuthStore = defineStore('auth', () => { // Revoke the server session so the refresh token can't be reused. // Best-effort: if the network call fails, still clean up locally. - // The server builds the OIDC RP-Initiated Logout URL (with id_token_hint, - // post_logout_redirect_uri and client_id) and returns it here. let oidcLogoutUrl = '' try { const HTTP = AuthenticatedHTTPFactory() @@ -551,10 +549,8 @@ export const useAuthStore = defineStore('auth', () => { await router.push({name: 'user.login'}) await checkAuth() - // Redirect to the OIDC provider's end-session endpoint so the provider - // session is ended too. Prefer the server-built URL (RP-Initiated Logout - // with id_token_hint), falling back to the static logout url if the - // server did not return one. + // Redirect to the OIDC provider to end its session too. Prefer the + // server-built RP-Initiated Logout URL, falling back to the static one. if (oidcLogoutUrl) { window.location.href = oidcLogoutUrl return diff --git a/pkg/migration/20260619155410.go b/pkg/migration/20260619155410.go index 0575900c1..97f91bb5e 100644 --- a/pkg/migration/20260619155410.go +++ b/pkg/migration/20260619155410.go @@ -23,9 +23,7 @@ import ( "xorm.io/xorm" ) -// Mirrors models.Session; the two new columns store what is needed to build an -// RP-Initiated Logout request (id_token_hint + which provider to log out from) -// when the session was created via OpenID Connect. +// Mirrors models.Session; adds the two columns RP-Initiated Logout needs. type sessionOIDCLogout20260619155410 struct { ID string `xorm:"varchar(36) not null unique pk"` UserID int64 `xorm:"bigint not null index"` diff --git a/pkg/models/sessions.go b/pkg/models/sessions.go index b2361ec67..9824cf0c3 100644 --- a/pkg/models/sessions.go +++ b/pkg/models/sessions.go @@ -49,12 +49,9 @@ type Session struct { IPAddress string `xorm:"varchar(100)" json:"ip_address" readOnly:"true" doc:"IP address captured from the login request."` // Whether this is a "remember me" session (controls max refresh lifetime). IsLongSession bool `xorm:"not null default false" json:"-"` - // The raw OpenID Connect ID token, kept only for sessions created via OIDC so - // it can be replayed as the id_token_hint in an RP-Initiated Logout request. - // Never exposed over the API. Empty for non-OIDC sessions. + // Raw OIDC ID token, kept so logout can replay it as id_token_hint. Empty for non-OIDC sessions. OIDCIDToken string `xorm:"text" json:"-"` - // The key of the OIDC provider that created this session, used to look up its - // end-session endpoint at logout. Empty for non-OIDC sessions. + // OIDC provider that created this session, used to find its end-session endpoint at logout. OIDCProviderKey string `xorm:"varchar(250)" json:"-"` // When this session was last refreshed. LastActive time.Time `xorm:"not null" json:"last_active" readOnly:"true" doc:"When this session was last refreshed."` @@ -88,9 +85,8 @@ func generateHashedToken() (rawToken, hash string, err error) { return rawToken, HashSessionToken(rawToken), nil } -// SessionOIDCData carries the OpenID Connect metadata persisted on a session so -// an RP-Initiated Logout request can be built later. It is nil for non-OIDC -// logins. +// SessionOIDCData carries the OIDC metadata persisted on a session so an +// RP-Initiated Logout request can be built later. Nil for non-OIDC logins. type SessionOIDCData struct { IDToken string ProviderKey string @@ -98,8 +94,7 @@ type SessionOIDCData struct { // CreateSession creates a new session record and generates a refresh token. // Returns the session with RefreshToken populated (cleartext, shown only once). -// oidc is optional: pass it for OpenID Connect logins so the raw id_token and -// provider key are stored for RP-Initiated Logout; pass nil otherwise. +// Pass oidc for OpenID Connect logins to persist the logout data; nil otherwise. func CreateSession(s *xorm.Session, userID int64, deviceInfo, ipAddress string, isLongSession bool, oidc *SessionOIDCData) (*Session, error) { rawToken, hash, err := generateHashedToken() if err != nil { diff --git a/pkg/modules/auth/auth.go b/pkg/modules/auth/auth.go index 59895beaf..87c89e5aa 100644 --- a/pkg/modules/auth/auth.go +++ b/pkg/modules/auth/auth.go @@ -112,9 +112,8 @@ type IssuedUserToken struct { // IssueUserToken creates a session for the user and mints a JWT access token plus // a refresh token for it. It is the transport-agnostic core both v1 (which writes // the echo response) and v2 (Huma) call; callers set the refresh cookie and the -// Cache-Control header themselves via WriteUserAuthCookies. oidc is optional: -// pass it for OpenID Connect logins so the id_token is stored for RP-Initiated -// Logout, nil otherwise. +// Cache-Control header themselves via WriteUserAuthCookies. Pass oidc for +// OpenID Connect logins to store the logout data; nil otherwise. func IssueUserToken(ctx context.Context, u *user.User, deviceInfo, ipAddress string, long bool, oidc *models.SessionOIDCData) (*IssuedUserToken, error) { s := db.NewSession() defer s.Close() @@ -163,7 +162,7 @@ func WriteUserAuthCookies(c *echo.Context, token *IssuedUserToken) { } // NewUserAuthTokenResponse creates a new user auth token response from a user object. -// oidc is optional OpenID Connect session metadata for RP-Initiated Logout; pass nil for local/LDAP logins. +// Pass oidc for OpenID Connect logins to store the logout data; nil otherwise. func NewUserAuthTokenResponse(u *user.User, c *echo.Context, long bool, oidc *models.SessionOIDCData) error { token, err := IssueUserToken(c.Request().Context(), u, c.Request().UserAgent(), c.RealIP(), long, oidc) if err != nil { diff --git a/pkg/modules/auth/openid/logout.go b/pkg/modules/auth/openid/logout.go index ed354c29e..958ea8765 100644 --- a/pkg/modules/auth/openid/logout.go +++ b/pkg/modules/auth/openid/logout.go @@ -24,15 +24,10 @@ import ( "code.vikunja.io/api/pkg/models" ) -// 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). +// EndSessionEndpoint returns the provider's RP-Initiated Logout endpoint +// (discovery's end_session_endpoint, cached at init), falling back to the static +// logouturl. Never triggers discovery so logout stays responsive when the OP is +// unreachable. func (p *Provider) EndSessionEndpoint() string { if p.EndSessionURL != "" { return p.EndSessionURL @@ -40,9 +35,9 @@ func (p *Provider) EndSessionEndpoint() string { 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. +// discoveredEndSessionEndpoint reads end_session_endpoint from the discovery +// document already cached on the *oidc.Provider, so Claims unmarshals in memory +// without a request. func (p *Provider) discoveredEndSessionEndpoint() string { if p.openIDProvider == nil { return "" @@ -58,25 +53,14 @@ func (p *Provider) discoveredEndSessionEndpoint() string { return meta.EndSessionEndpoint } -// BuildEndSessionURL constructs an OpenID Connect RP-Initiated Logout 1.0 request -// URL for the given provider key and stored session OIDC data. -// -// Per RP-Initiated Logout 1.0 §2 it appends: -// - id_token_hint: the ID token previously issued to this session. RECOMMENDED; -// it lets the OP skip the logout-confirmation prompt and is what makes the OP -// honor post_logout_redirect_uri (the OP MAY require it, §3). -// - post_logout_redirect_uri: where the OP redirects the user agent after -// logout. MUST be pre-registered with the OP. Defaults to service.publicurl -// (the Vikunja frontend) so the user lands back on Vikunja's login page. -// - client_id: the RP's client identifier (§2). Always sent; the OP verifies it -// matches the one in id_token_hint. -// -// It returns "" (and the caller skips the redirect) when neither an +// BuildEndSessionURL builds an OpenID Connect RP-Initiated Logout 1.0 request URL +// (id_token_hint + post_logout_redirect_uri + client_id; see RP-Initiated Logout +// 1.0 §2). post_logout_redirect_uri defaults to service.publicurl, and the OP +// only honors it when id_token_hint is present. Returns "" when neither an // end_session_endpoint nor a static logouturl is configured. func BuildEndSessionURL(providerKey string, oidc *models.SessionOIDCData) (string, error) { - // 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. + // GetProvider would trigger OIDC discovery (a live HTTP GET that blocks when + // the OP is down); the cached static fields are all logout needs. provider, err := getCachedProvider(providerKey) if err != nil { return "", err @@ -98,9 +82,8 @@ func BuildEndSessionURL(providerKey string, oidc *models.SessionOIDCData) (strin ) } -// buildEndSessionURL assembles the RP-Initiated Logout query string onto the -// given end-session endpoint. Empty optional params are omitted. Returns "" when -// no endpoint is configured. +// buildEndSessionURL appends the logout query params onto endpoint, omitting +// empty ones, and returns "" for an empty endpoint. func buildEndSessionURL(endpoint, clientID, idToken, postLogoutRedirectURI string) (string, error) { if endpoint == "" { return "", nil diff --git a/pkg/modules/auth/openid/logout_test.go b/pkg/modules/auth/openid/logout_test.go index 7b08116ca..57e3ea6a2 100644 --- a/pkg/modules/auth/openid/logout_test.go +++ b/pkg/modules/auth/openid/logout_test.go @@ -31,8 +31,8 @@ import ( "github.com/stretchr/testify/require" ) -// newMockOIDCServerWithEndSession serves a discovery document that includes an -// end_session_endpoint, exercising the RP-Initiated Logout discovery path. +// newMockOIDCServerWithEndSession publishes a discovery document with an +// end_session_endpoint. func newMockOIDCServerWithEndSession() *httptest.Server { var server *httptest.Server mux := http.NewServeMux() @@ -137,9 +137,8 @@ func TestBuildEndSessionURLFromCachedProviderWithoutLiveObject(t *testing.T) { 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. + // Seed only the cached static fields (no live openIDProvider), mimicking a + // provider restored from keyvalue whose OP is unreachable. _ = keyvalue.Del("openid_providers") require.NoError(t, keyvalue.Put("openid_provider_provider1", &Provider{ Key: "provider1", @@ -163,10 +162,8 @@ func TestBuildEndSessionURLFromCachedProviderWithoutLiveObject(t *testing.T) { } 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. + // A nil openIDProvider models a provider restored from cache (or an + // unreachable OP): EndSessionEndpoint must answer from the cached URL. p := &Provider{ Key: "provider1", LogoutURL: "https://op.example.com/static-logout", @@ -212,8 +209,7 @@ func TestEndSessionEndpointCachedFromDiscoveryOnInit(t *testing.T) { func TestEndSessionEndpointFallsBackToStaticLogoutURL(t *testing.T) { defer CleanupSavedOpenIDProviders() - // This mock server publishes no end_session_endpoint, so the provider must - // fall back to the statically configured logouturl. + // newMockOIDCServer publishes no end_session_endpoint, forcing the logouturl fallback. server := newMockOIDCServer() defer server.Close() diff --git a/pkg/modules/auth/openid/openid.go b/pkg/modules/auth/openid/openid.go index bbf1382a5..3b82379ac 100644 --- a/pkg/modules/auth/openid/openid.go +++ b/pkg/modules/auth/openid/openid.go @@ -69,10 +69,9 @@ type Provider struct { ForceUserInfo bool `json:"force_user_info"` RequireAvailability bool `json:"-"` ClientSecret string `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). + // RP-Initiated Logout endpoint, cached at init so logout never fetches. + // Exported so it survives the gob keyvalue round-trip (gob skips unexported + // fields like openIDProvider); json:"-" keeps it out of /info. EndSessionURL string `json:"-"` openIDProvider *oidc.Provider Oauth2Config *oauth2.Config `json:"-"` @@ -210,8 +209,7 @@ func AuthenticateCallback(ctx context.Context, cb *Callback, providerKey string) return nil, nil, err } - // Stored so logout can replay it as id_token_hint in an RP-Initiated Logout - // request. See pkg/modules/auth/openid/logout.go. + // Stored so logout can replay it as id_token_hint in an RP-Initiated Logout. oidcData := &models.SessionOIDCData{ IDToken: rawIDToken, ProviderKey: providerKey, diff --git a/pkg/modules/auth/openid/providers.go b/pkg/modules/auth/openid/providers.go index 4aa4a44e7..710870b7d 100644 --- a/pkg/modules/auth/openid/providers.go +++ b/pkg/modules/auth/openid/providers.go @@ -180,10 +180,8 @@ 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. +// getCachedProvider returns the provider from keyvalue without re-establishing +// the live OIDC connection, so the logout path never blocks on an unreachable OP. func getCachedProvider(key string) (provider *Provider, err error) { provider = &Provider{} exists, err := keyvalue.GetWithValue("openid_provider_"+key, provider) diff --git a/pkg/routes/api/shared/auth.go b/pkg/routes/api/shared/auth.go index a36fbe3f3..560ae0f47 100644 --- a/pkg/routes/api/shared/auth.go +++ b/pkg/routes/api/shared/auth.go @@ -197,13 +197,10 @@ func DeleteSession(sid string) error { return err } -// LogoutSession reads the session, builds an OpenID Connect RP-Initiated Logout -// URL when the session was created via OIDC, then deletes the session. It -// returns the end-session URL (empty for non-OIDC sessions or when no logout -// endpoint is configured) so the frontend can redirect the user agent to the -// identity provider's end_session_endpoint with id_token_hint and -// post_logout_redirect_uri. An empty sid is a no-op. The caller clears the -// refresh cookie. +// LogoutSession deletes the session and returns its OIDC RP-Initiated Logout URL +// for the frontend to redirect to (empty for non-OIDC sessions or when no logout +// endpoint is configured). An empty sid is a no-op. The caller clears the refresh +// cookie. func LogoutSession(sid string) (endSessionURL string, err error) { if sid == "" { return "", nil @@ -212,8 +209,8 @@ func LogoutSession(sid string) (endSessionURL string, err error) { s := db.NewSession() defer s.Close() - // Read the session before deleting so the stored id_token can be replayed as - // id_token_hint. A missing session just means there is nothing to log out. + // Read before deleting so the stored id_token survives for the logout URL. + // A missing session just means there is nothing to log out. session, err := models.GetSessionByID(s, sid) if err != nil && !models.IsErrSessionNotFound(err) { _ = s.Rollback() @@ -225,8 +222,7 @@ func LogoutSession(sid string) (endSessionURL string, err error) { ProviderKey: session.OIDCProviderKey, }) if buildErr != nil { - // Don't fail logout just because the logout URL could not be built; - // the session is still destroyed server-side below. + // A failed URL build must not block logout; the session is still deleted below. log.Errorf("Could not build OIDC end-session URL for session %s: %v", sid, buildErr) } else { endSessionURL = url diff --git a/pkg/routes/api/v1/login.go b/pkg/routes/api/v1/login.go index a1974da58..6a4662ae2 100644 --- a/pkg/routes/api/v1/login.go +++ b/pkg/routes/api/v1/login.go @@ -150,14 +150,9 @@ func RefreshToken(c *echo.Context) (err error) { return c.JSON(http.StatusOK, auth.Token{Token: result.AccessToken}) } -// LogoutResponse confirms a successful logout and, for sessions created via -// OpenID Connect, carries the provider's RP-Initiated Logout URL the frontend -// should redirect the user agent to so the OP session is ended too. type LogoutResponse struct { Message string `json:"message"` - // OIDCLogoutURL is the fully-built end_session_endpoint URL (with - // id_token_hint, post_logout_redirect_uri and client_id). Empty for non-OIDC - // sessions. + // RP-Initiated Logout URL the frontend redirects to. Empty for non-OIDC sessions. OIDCLogoutURL string `json:"oidc_logout_url,omitempty"` } diff --git a/pkg/routes/api/v2/auth_login.go b/pkg/routes/api/v2/auth_login.go index 431b44e11..519fcaef1 100644 --- a/pkg/routes/api/v2/auth_login.go +++ b/pkg/routes/api/v2/auth_login.go @@ -45,11 +45,7 @@ type authTokenBody struct { // logoutBody confirms a successful logout. type logoutBody struct { Body struct { - Message string `json:"message" readOnly:"true" doc:"A human-readable confirmation message."` - // OIDCLogoutURL is the provider's RP-Initiated Logout URL (with - // id_token_hint, post_logout_redirect_uri and client_id) for sessions - // created via OpenID Connect. The client should redirect the user agent - // to it to end the provider session too. Empty for non-OIDC sessions. + Message string `json:"message" readOnly:"true" doc:"A human-readable confirmation message."` OIDCLogoutURL string `json:"oidc_logout_url,omitempty" readOnly:"true" doc:"RP-Initiated Logout URL to redirect to for OpenID Connect sessions; empty otherwise."` } }