From 69693169c1b5204f13a3f5918192d9479a277449 Mon Sep 17 00:00:00 2001 From: dfliess Date: Mon, 29 Jun 2026 13:40:45 +0200 Subject: [PATCH 1/6] feat(auth): support standard OIDC providers (Keycloak, Dex, etc.) The auth module assumes Auth0 in three places: issuer URL construction (trailing slash), signup parameter (screen_hint), and logout endpoint (/v2/logout). This breaks any standard OIDC provider. Fix all three, fully backward compatible: - AUTH_DOMAIN with "://" is used verbatim as issuer; without it, the old "https://"+domain+"/" behavior is preserved. - Signup uses prompt=create (OIDC standard) instead of screen_hint. - Logout reads end_session_endpoint from discovery and falls back to Auth0's /v2/logout when absent. Tested with Keycloak 26. Auth0 continues to work unchanged. --- admin/server/auth/auth.go | 41 ++++++++++++++++++++++++----------- admin/server/auth/handlers.go | 24 ++++++++++++-------- 2 files changed, 43 insertions(+), 22 deletions(-) diff --git a/admin/server/auth/auth.go b/admin/server/auth/auth.go index 73a4c156cb1e..dd44345900f1 100644 --- a/admin/server/auth/auth.go +++ b/admin/server/auth/auth.go @@ -2,6 +2,7 @@ package auth import ( "context" + "strings" "github.com/coreos/go-oidc/v3/oidc" "github.com/rilldata/rill/admin" @@ -28,21 +29,34 @@ type AuthenticatorOptions struct { // It provides endpoints for login/logout, creates users, issues cookie-based auth tokens, and provides middleware for authenticating requests. // The implementation was derived from: https://auth0.com/docs/quickstart/webapp/golang/01-login. type Authenticator struct { - logger *zap.Logger - admin *admin.Service - cookies *cookies.Store - opts *AuthenticatorOptions - oidc *oidc.Provider - oauth2 oauth2.Config + logger *zap.Logger + admin *admin.Service + cookies *cookies.Store + opts *AuthenticatorOptions + oidc *oidc.Provider + oauth2 oauth2.Config + endSessionEndpoint string } // NewAuthenticator creates an Authenticator. func NewAuthenticator(logger *zap.Logger, adm *admin.Service, cookieStore *cookies.Store, opts *AuthenticatorOptions) (*Authenticator, error) { - oidcProvider, err := oidc.NewProvider(context.Background(), "https://"+opts.AuthDomain+"/") + // AuthDomain with "://" is a full issuer URL (Keycloak, Dex, etc.); + // without it, assume Auth0-style domain and append trailing slash. + issuerURL := opts.AuthDomain + if !strings.Contains(issuerURL, "://") { + issuerURL = "https://" + issuerURL + "/" + } + + oidcProvider, err := oidc.NewProvider(context.Background(), issuerURL) if err != nil { return nil, err } + var claims struct { + EndSessionEndpoint string `json:"end_session_endpoint"` + } + _ = oidcProvider.Claims(&claims) + oauth2Config := oauth2.Config{ ClientID: opts.AuthClientID, ClientSecret: opts.AuthClientSecret, @@ -52,12 +66,13 @@ func NewAuthenticator(logger *zap.Logger, adm *admin.Service, cookieStore *cooki } a := &Authenticator{ - logger: logger, - admin: adm, - cookies: cookieStore, - opts: opts, - oidc: oidcProvider, - oauth2: oauth2Config, + logger: logger, + admin: adm, + cookies: cookieStore, + opts: opts, + oidc: oidcProvider, + oauth2: oauth2Config, + endSessionEndpoint: claims.EndSessionEndpoint, } return a, nil diff --git a/admin/server/auth/handlers.go b/admin/server/auth/handlers.go index ba8c736f34e1..965ace6747f4 100644 --- a/admin/server/auth/handlers.go +++ b/admin/server/auth/handlers.go @@ -207,9 +207,7 @@ func (a *Authenticator) authStart(w http.ResponseWriter, r *http.Request, signup // Redirect to auth provider (canonical domain flow) redirectURL := a.oauth2.AuthCodeURL(state) if signup { - // Set custom parameters for signup using AuthCodeOption - customOption := oauth2.SetAuthURLParam("screen_hint", "signup") - redirectURL = a.oauth2.AuthCodeURL(state, customOption) + redirectURL = a.oauth2.AuthCodeURL(state, oauth2.SetAuthURLParam("prompt", "create")) } http.Redirect(w, r, redirectURL, http.StatusTemporaryRedirect) @@ -611,16 +609,24 @@ func (a *Authenticator) authLogoutProvider(w http.ResponseWriter, r *http.Reques } } - // Build and redirect to the auth provider logout URL. - logoutURL, err := url.Parse("https://" + a.opts.AuthDomain + "/v2/logout") + // Build the provider logout URL. + // Standard OIDC providers expose end_session_endpoint; Auth0 uses /v2/logout with "returnTo". + logoutEndpoint := a.endSessionEndpoint + redirectParam := "post_logout_redirect_uri" + if logoutEndpoint == "" { + logoutEndpoint = "https://" + a.opts.AuthDomain + "/v2/logout" + redirectParam = "returnTo" + } + + logoutURL, err := url.Parse(logoutEndpoint) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return } - parameters := url.Values{} - parameters.Add("returnTo", a.admin.URLs.AuthLogoutCallback()) - parameters.Add("client_id", a.opts.AuthClientID) - logoutURL.RawQuery = parameters.Encode() + params := url.Values{} + params.Set("client_id", a.opts.AuthClientID) + params.Set(redirectParam, a.admin.URLs.AuthLogoutCallback()) + logoutURL.RawQuery = params.Encode() http.Redirect(w, r, logoutURL.String(), http.StatusTemporaryRedirect) } From a46b10a184384a27058f6a4b27f763b6833a190d Mon Sep 17 00:00:00 2001 From: dfliess Date: Mon, 21 Sep 2026 22:04:05 +0200 Subject: [PATCH 2/6] fix(auth): send screen_hint alongside prompt=create on signup Auth0 ignores prompt=create, so replacing screen_hint=signup sent /auth/signup to the login screen. Send both: Auth0 honors screen_hint and standard OIDC providers honor prompt=create, each ignoring the other. --- admin/server/auth/handlers.go | 3 +- admin/server/auth/handlers_test.go | 59 ++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) create mode 100644 admin/server/auth/handlers_test.go diff --git a/admin/server/auth/handlers.go b/admin/server/auth/handlers.go index 965ace6747f4..85623bf12dbd 100644 --- a/admin/server/auth/handlers.go +++ b/admin/server/auth/handlers.go @@ -207,7 +207,8 @@ func (a *Authenticator) authStart(w http.ResponseWriter, r *http.Request, signup // Redirect to auth provider (canonical domain flow) redirectURL := a.oauth2.AuthCodeURL(state) if signup { - redirectURL = a.oauth2.AuthCodeURL(state, oauth2.SetAuthURLParam("prompt", "create")) + // Send both signup hints: Auth0 only honors screen_hint, standard OIDC providers only honor prompt=create. + redirectURL = a.oauth2.AuthCodeURL(state, oauth2.SetAuthURLParam("screen_hint", "signup"), oauth2.SetAuthURLParam("prompt", "create")) } http.Redirect(w, r, redirectURL, http.StatusTemporaryRedirect) diff --git a/admin/server/auth/handlers_test.go b/admin/server/auth/handlers_test.go new file mode 100644 index 000000000000..2e5409678881 --- /dev/null +++ b/admin/server/auth/handlers_test.go @@ -0,0 +1,59 @@ +package auth + +import ( + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "github.com/rilldata/rill/admin" + "github.com/rilldata/rill/admin/server/cookies" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + "golang.org/x/oauth2" +) + +// newTestAuthenticator returns an Authenticator wired just enough to exercise the login and logout +// redirect handlers without a database or a live auth provider. +func newTestAuthenticator(t *testing.T, authDomain string) *Authenticator { + urls, err := admin.NewURLs("http://localhost:8080", "http://localhost:3000") + require.NoError(t, err) + + return &Authenticator{ + logger: zap.NewNop(), + admin: &admin.Service{URLs: urls}, + cookies: cookies.New(zap.NewNop(), []byte("0123456789abcdef0123456789abcdef"), []byte("0123456789abcdef")), + opts: &AuthenticatorOptions{ + AuthDomain: authDomain, + AuthClientID: "rill-client", + }, + oauth2: oauth2.Config{ + ClientID: "rill-client", + RedirectURL: urls.AuthLoginCallback(), + Endpoint: oauth2.Endpoint{AuthURL: "https://idp.example.com/authorize"}, + }, + } +} + +func TestAuthStartSignup(t *testing.T) { + a := newTestAuthenticator(t, "idp.example.com") + + for _, signup := range []bool{false, true} { + req := httptest.NewRequest(http.MethodGet, "http://localhost:8080/auth/login", nil) + w := httptest.NewRecorder() + a.authStart(w, req, signup) + require.Equal(t, http.StatusTemporaryRedirect, w.Code) + + loc, err := url.Parse(w.Header().Get("Location")) + require.NoError(t, err) + q := loc.Query() + if !signup { + require.Empty(t, q.Get("prompt")) + require.Empty(t, q.Get("screen_hint")) + continue + } + // Auth0 only honors screen_hint, standard OIDC providers only honor prompt=create. + require.Equal(t, "create", q.Get("prompt")) + require.Equal(t, "signup", q.Get("screen_hint")) + } +} From c155839d044179d4704307fd4ea871743d0c15ea Mon Sep 17 00:00:00 2001 From: dfliess Date: Mon, 21 Sep 2026 22:04:58 +0200 Subject: [PATCH 3/6] fix(auth): only fall back to Auth0's /v2/logout for bare Auth0 domains When AUTH_DOMAIN is a full issuer URL and the provider publishes no end_session_endpoint (Dex v2.45.1), the fallback built https://https:///v2/logout. That endpoint only exists on Auth0, so keep the fallback for bare Auth0 domains, unchanged, and otherwise end only the Rill session and warn at startup. --- admin/server/auth/auth.go | 10 +++++++- admin/server/auth/handlers.go | 5 ++++ admin/server/auth/handlers_test.go | 39 ++++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-) diff --git a/admin/server/auth/auth.go b/admin/server/auth/auth.go index dd44345900f1..3725b0535d8e 100644 --- a/admin/server/auth/auth.go +++ b/admin/server/auth/auth.go @@ -43,7 +43,7 @@ func NewAuthenticator(logger *zap.Logger, adm *admin.Service, cookieStore *cooki // AuthDomain with "://" is a full issuer URL (Keycloak, Dex, etc.); // without it, assume Auth0-style domain and append trailing slash. issuerURL := opts.AuthDomain - if !strings.Contains(issuerURL, "://") { + if isBareDomain(issuerURL) { issuerURL = "https://" + issuerURL + "/" } @@ -56,6 +56,9 @@ func NewAuthenticator(logger *zap.Logger, adm *admin.Service, cookieStore *cooki EndSessionEndpoint string `json:"end_session_endpoint"` } _ = oidcProvider.Claims(&claims) + if claims.EndSessionEndpoint == "" && !isBareDomain(opts.AuthDomain) { + logger.Warn("auth provider does not publish an end_session_endpoint, so logging out will only end the Rill session", zap.String("issuer", issuerURL)) + } oauth2Config := oauth2.Config{ ClientID: opts.AuthClientID, @@ -77,3 +80,8 @@ func NewAuthenticator(logger *zap.Logger, adm *admin.Service, cookieStore *cooki return a, nil } + +// isBareDomain reports whether authDomain is an Auth0-style domain (e.g. "rill.auth0.com") rather than a full issuer URL. +func isBareDomain(authDomain string) bool { + return !strings.Contains(authDomain, "://") +} diff --git a/admin/server/auth/handlers.go b/admin/server/auth/handlers.go index 85623bf12dbd..9ad1ad283bb1 100644 --- a/admin/server/auth/handlers.go +++ b/admin/server/auth/handlers.go @@ -615,6 +615,11 @@ func (a *Authenticator) authLogoutProvider(w http.ResponseWriter, r *http.Reques logoutEndpoint := a.endSessionEndpoint redirectParam := "post_logout_redirect_uri" if logoutEndpoint == "" { + if !isBareDomain(a.opts.AuthDomain) { + // The provider has no logout endpoint we can call (e.g. Dex), so only the Rill session is ended. + http.Redirect(w, r, a.admin.URLs.AuthLogoutCallback(), http.StatusTemporaryRedirect) + return + } logoutEndpoint = "https://" + a.opts.AuthDomain + "/v2/logout" redirectParam = "returnTo" } diff --git a/admin/server/auth/handlers_test.go b/admin/server/auth/handlers_test.go index 2e5409678881..910778c02bd8 100644 --- a/admin/server/auth/handlers_test.go +++ b/admin/server/auth/handlers_test.go @@ -57,3 +57,42 @@ func TestAuthStartSignup(t *testing.T) { require.Equal(t, "signup", q.Get("screen_hint")) } } + +func TestAuthLogoutProvider(t *testing.T) { + tests := []struct { + name string + authDomain string + endSessionEndpoint string + want string + }{ + { + // Must stay identical to what Auth0 deployments get today. + name: "auth0 domain without end_session_endpoint", + authDomain: "rill.auth0.com", + want: "https://rill.auth0.com/v2/logout?client_id=rill-client&returnTo=http%3A%2F%2Flocalhost%3A8080%2Fauth%2Flogout%2Fcallback", + }, + { + name: "issuer URL without end_session_endpoint", + authDomain: "https://idp.example.com/realms/rill", + want: "http://localhost:8080/auth/logout/callback", + }, + { + name: "issuer URL with end_session_endpoint", + authDomain: "https://idp.example.com/realms/rill", + endSessionEndpoint: "https://idp.example.com/realms/rill/protocol/openid-connect/logout", + want: "https://idp.example.com/realms/rill/protocol/openid-connect/logout?client_id=rill-client&post_logout_redirect_uri=http%3A%2F%2Flocalhost%3A8080%2Fauth%2Flogout%2Fcallback", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + a := newTestAuthenticator(t, tt.authDomain) + a.endSessionEndpoint = tt.endSessionEndpoint + + req := httptest.NewRequest(http.MethodGet, "http://localhost:8080/auth/logout/provider", nil) + w := httptest.NewRecorder() + a.authLogoutProvider(w, req) + require.Equal(t, http.StatusTemporaryRedirect, w.Code) + require.Equal(t, tt.want, w.Header().Get("Location")) + }) + } +} From 1c078aeb9433893f5c29cc55c2a95caab1bf1841 Mon Sep 17 00:00:00 2001 From: dfliess Date: Mon, 21 Sep 2026 22:06:05 +0200 Subject: [PATCH 4/6] fix(auth): fail startup when the discovery document cannot be parsed Claims() errors were discarded, so a non-string end_session_endpoint silently routed logout to the fallback. Return the error instead, as oidc.NewProvider already does for a broken discovery document. Also extract issuerURL and cover it with a table test over AuthDomain values, and test NewAuthenticator against a fake OIDC provider. --- admin/server/auth/auth.go | 27 +++++--- admin/server/auth/auth_test.go | 111 +++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 10 deletions(-) create mode 100644 admin/server/auth/auth_test.go diff --git a/admin/server/auth/auth.go b/admin/server/auth/auth.go index 3725b0535d8e..2977f8ac6af2 100644 --- a/admin/server/auth/auth.go +++ b/admin/server/auth/auth.go @@ -2,6 +2,7 @@ package auth import ( "context" + "fmt" "strings" "github.com/coreos/go-oidc/v3/oidc" @@ -40,14 +41,8 @@ type Authenticator struct { // NewAuthenticator creates an Authenticator. func NewAuthenticator(logger *zap.Logger, adm *admin.Service, cookieStore *cookies.Store, opts *AuthenticatorOptions) (*Authenticator, error) { - // AuthDomain with "://" is a full issuer URL (Keycloak, Dex, etc.); - // without it, assume Auth0-style domain and append trailing slash. - issuerURL := opts.AuthDomain - if isBareDomain(issuerURL) { - issuerURL = "https://" + issuerURL + "/" - } - - oidcProvider, err := oidc.NewProvider(context.Background(), issuerURL) + issuer := issuerURL(opts.AuthDomain) + oidcProvider, err := oidc.NewProvider(context.Background(), issuer) if err != nil { return nil, err } @@ -55,9 +50,11 @@ func NewAuthenticator(logger *zap.Logger, adm *admin.Service, cookieStore *cooki var claims struct { EndSessionEndpoint string `json:"end_session_endpoint"` } - _ = oidcProvider.Claims(&claims) + if err := oidcProvider.Claims(&claims); err != nil { + return nil, fmt.Errorf("failed to parse the auth provider's discovery document: %w", err) + } if claims.EndSessionEndpoint == "" && !isBareDomain(opts.AuthDomain) { - logger.Warn("auth provider does not publish an end_session_endpoint, so logging out will only end the Rill session", zap.String("issuer", issuerURL)) + logger.Warn("auth provider does not publish an end_session_endpoint, so logging out will only end the Rill session", zap.String("issuer", issuer)) } oauth2Config := oauth2.Config{ @@ -81,6 +78,16 @@ func NewAuthenticator(logger *zap.Logger, adm *admin.Service, cookieStore *cooki return a, nil } +// issuerURL returns the OIDC issuer for authDomain. +// AuthDomain with "://" is a full issuer URL (Keycloak, Dex, etc.) used verbatim; +// without it, assume Auth0-style domain and append trailing slash. +func issuerURL(authDomain string) string { + if isBareDomain(authDomain) { + return "https://" + authDomain + "/" + } + return authDomain +} + // isBareDomain reports whether authDomain is an Auth0-style domain (e.g. "rill.auth0.com") rather than a full issuer URL. func isBareDomain(authDomain string) bool { return !strings.Contains(authDomain, "://") diff --git a/admin/server/auth/auth_test.go b/admin/server/auth/auth_test.go new file mode 100644 index 000000000000..c0b16e01d2b0 --- /dev/null +++ b/admin/server/auth/auth_test.go @@ -0,0 +1,111 @@ +package auth + +import ( + "crypto/rand" + "crypto/rsa" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/go-jose/go-jose/v3" + "github.com/go-jose/go-jose/v3/jwt" + "github.com/rilldata/rill/admin" + "github.com/stretchr/testify/require" + "go.uber.org/zap" +) + +// testProvider is a minimal OIDC provider serving a discovery document and a JWKS, and signing ID tokens with its key. +type testProvider struct { + *httptest.Server + key *rsa.PrivateKey +} + +// newTestProvider starts a testProvider. The discovery document can be extended (or fields overridden) with extra. +func newTestProvider(t *testing.T, extra map[string]any) *testProvider { + key, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + + p := &testProvider{key: key} + mux := http.NewServeMux() + mux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { + doc := map[string]any{ + "issuer": p.URL, + "authorization_endpoint": p.URL + "/authorize", + "token_endpoint": p.URL + "/token", + "jwks_uri": p.URL + "/jwks", + "id_token_signing_alg_values_supported": []string{"RS256"}, + } + for k, v := range extra { + doc[k] = v + } + _ = json.NewEncoder(w).Encode(doc) + }) + mux.HandleFunc("/jwks", func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode(jose.JSONWebKeySet{Keys: []jose.JSONWebKey{{Key: &key.PublicKey, KeyID: "test", Algorithm: "RS256", Use: "sig"}}}) + }) + p.Server = httptest.NewServer(mux) + t.Cleanup(p.Close) + return p +} + +// signIDToken returns an ID token for the given claims, signed with key (the provider's own key if nil). +func (p *testProvider) signIDToken(t *testing.T, key *rsa.PrivateKey, claims map[string]any) string { + if key == nil { + key = p.key + } + signer, err := jose.NewSigner(jose.SigningKey{Algorithm: jose.RS256, Key: key}, (&jose.SignerOptions{}).WithHeader("kid", "test")) + require.NoError(t, err) + raw, err := jwt.Signed(signer).Claims(claims).CompactSerialize() + require.NoError(t, err) + return raw +} + +func TestIssuerURL(t *testing.T) { + tests := []struct { + authDomain string + want string + }{ + {"rill.auth0.com", "https://rill.auth0.com/"}, + {"https://idp.example.com/realms/rill", "https://idp.example.com/realms/rill"}, + {"https://idp.example.com/realms/rill/", "https://idp.example.com/realms/rill/"}, + {"http://localhost:5556/dex", "http://localhost:5556/dex"}, + } + for _, tt := range tests { + require.Equal(t, tt.want, issuerURL(tt.authDomain), tt.authDomain) + } +} + +func TestNewAuthenticator(t *testing.T) { + urls, err := admin.NewURLs("http://localhost:8080", "http://localhost:3000") + require.NoError(t, err) + adm := &admin.Service{URLs: urls} + + tests := []struct { + name string + endSession any // value of end_session_endpoint in the discovery document; nil leaves it out + wantEndSession string + wantErrorSubstring string + }{ + {name: "with end_session_endpoint", endSession: "https://idp.example.com/logout", wantEndSession: "https://idp.example.com/logout"}, + {name: "without end_session_endpoint"}, + {name: "malformed end_session_endpoint", endSession: 42, wantErrorSubstring: "discovery document"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var discovery map[string]any + if tt.endSession != nil { + discovery = map[string]any{"end_session_endpoint": tt.endSession} + } + p := newTestProvider(t, discovery) + + a, err := NewAuthenticator(zap.NewNop(), adm, nil, &AuthenticatorOptions{AuthDomain: p.URL, AuthClientID: "rill-client"}) + if tt.wantErrorSubstring != "" { + require.ErrorContains(t, err, tt.wantErrorSubstring) + return + } + require.NoError(t, err) + require.Equal(t, tt.wantEndSession, a.endSessionEndpoint) + }) + } +} From e1d73c3b2334afa4e7d79fca4215cc9369db2cb9 Mon Sep 17 00:00:00 2001 From: dfliess Date: Mon, 21 Sep 2026 22:09:47 +0200 Subject: [PATCH 5/6] fix(auth): send id_token_hint on RP-initiated logout Without id_token_hint, Keycloak (and Auth0's /oidc/logout) show a logout confirmation page and keep the provider session alive until the user confirms, so closing the tab leaves them signed in at the provider. Keep the raw ID token from login and send it on logout: - Only when the provider publishes an end_session_endpoint, so Auth0 deployments using /v2/logout are unaffected. - In its own cookie, not in the auth cookie: an ID token can push the auth cookie past the 4096-byte limit and fail the login. If it does not fit on its own, it is skipped and logout falls back to the confirmation page. - Scoped to /auth/logout/provider, so browsers only send it on logout rather than with every request to the admin service. - Only if its signature still verifies, ignoring expiry: an invalid hint makes Keycloak fail the logout with an error page, and the spec asks providers to accept expired hints. --- admin/server/auth/handlers.go | 80 ++++++++++++++++++++ admin/server/auth/handlers_test.go | 117 ++++++++++++++++++++++++++++- 2 files changed, 196 insertions(+), 1 deletion(-) diff --git a/admin/server/auth/handlers.go b/admin/server/auth/handlers.go index 9ad1ad283bb1..e6744eba1ef3 100644 --- a/admin/server/auth/handlers.go +++ b/admin/server/auth/handlers.go @@ -13,6 +13,7 @@ import ( "time" "github.com/coreos/go-oidc/v3/oidc" + "github.com/gorilla/sessions" "github.com/rilldata/rill/admin/database" "github.com/rilldata/rill/admin/pkg/urlutil" "github.com/rilldata/rill/runtime/pkg/httputil" @@ -30,6 +31,8 @@ const ( cookieFieldRedirect = "redirect" cookieFieldCustomDomainFlow = "custom_domain_flow" cookieFieldAccessToken = "access_token" + idTokenCookieName = "auth_id_token" // nolint:gosec // cookie name, not a credential + cookieFieldIDToken = "id_token" ) var ( @@ -329,6 +332,9 @@ func (a *Authenticator) authLoginCallback(w http.ResponseWriter, r *http.Request return } + // Keep the ID token for logout. This callback and authLogoutProvider both run on the canonical domain, so it works for both flows below. + a.saveIDTokenHint(w, r, rawIDToken) + // If it's part of a custom domain login flow, redirect back to the custom domain with a short-lived access token for the user. customDomainFlow, ok := sess.Values[cookieFieldCustomDomainFlow].(bool) delete(sess.Values, cookieFieldCustomDomainFlow) @@ -610,6 +616,9 @@ func (a *Authenticator) authLogoutProvider(w http.ResponseWriter, r *http.Reques } } + // Take the ID token saved at login (if any) to send as id_token_hint + idTokenHint := a.takeIDTokenHint(w, r) + // Build the provider logout URL. // Standard OIDC providers expose end_session_endpoint; Auth0 uses /v2/logout with "returnTo". logoutEndpoint := a.endSessionEndpoint @@ -632,10 +641,81 @@ func (a *Authenticator) authLogoutProvider(w http.ResponseWriter, r *http.Reques params := url.Values{} params.Set("client_id", a.opts.AuthClientID) params.Set(redirectParam, a.admin.URLs.AuthLogoutCallback()) + if a.endSessionEndpoint != "" && idTokenHint != "" { + params.Set("id_token_hint", idTokenHint) + } logoutURL.RawQuery = params.Encode() http.Redirect(w, r, logoutURL.String(), http.StatusTemporaryRedirect) } +// saveIDTokenHint keeps the raw ID token from login so that authLogoutProvider can send it as id_token_hint. +// Without the hint, standard OIDC providers (e.g. Keycloak) show a logout confirmation page and keep the user's +// session alive until it is confirmed. +// +// It is only kept when the provider publishes an end_session_endpoint: Auth0's /v2/logout does not use it, so +// Auth0 deployments are unaffected. +// +// It goes in its own cookie rather than in the auth cookie: ID tokens can be large enough to push the auth cookie +// past the 4096-byte cookie limit, which would fail the login. If the ID token does not fit on its own either, it +// is skipped, and logout falls back to the provider's confirmation page. +// +// The cookie is scoped to the path of authLogoutProvider, so browsers only send it on logout rather than adding +// the ID token to every request to the admin service. +func (a *Authenticator) saveIDTokenHint(w http.ResponseWriter, r *http.Request, rawIDToken string) { + if a.endSessionEndpoint == "" { + return + } + + sess := a.cookies.Get(r, idTokenCookieName) + sess.Options = a.idTokenCookieOptions(sess.Options, false) + sess.Values[cookieFieldIDToken] = rawIDToken + if err := sess.Save(r, w); err != nil { + a.logger.Info("not keeping ID token for logout", zap.Error(err), observability.ZapCtx(r.Context())) + } +} + +// takeIDTokenHint returns the ID token kept by saveIDTokenHint and clears its cookie. +// It returns an empty string if there is none, or if its signature no longer verifies (e.g. after the provider +// rotated its keys): providers such as Keycloak fail the whole logout on an invalid hint, whereas without one +// they only ask for confirmation. Expiry is not checked: the Rill session outlives the ID token by weeks, and the +// OIDC RP-Initiated Logout spec asks providers to accept hints whose exp has passed. +func (a *Authenticator) takeIDTokenHint(w http.ResponseWriter, r *http.Request) string { + if _, err := r.Cookie(idTokenCookieName); err != nil { + return "" + } + + sess := a.cookies.Get(r, idTokenCookieName) + rawIDToken, _ := sess.Values[cookieFieldIDToken].(string) + sess.Options = a.idTokenCookieOptions(sess.Options, true) + if err := sess.Save(r, w); err != nil { + a.logger.Info("failed to clear ID token cookie", zap.Error(err), observability.ZapCtx(r.Context())) + } + if rawIDToken == "" { + return "" + } + + verifier := a.oidc.Verifier(&oidc.Config{ClientID: a.oauth2.ClientID, SkipExpiryCheck: true}) + if _, err := verifier.Verify(r.Context(), rawIDToken); err != nil { + a.logger.Info("not sending ID token as logout hint", zap.Error(err), observability.ZapCtx(r.Context())) + return "" + } + return rawIDToken +} + +// idTokenCookieOptions returns a copy of opts for the ID token cookie, scoped to the path of authLogoutProvider. +// It copies because cookies.Store.Get may return the store's shared options. +func (a *Authenticator) idTokenCookieOptions(opts *sessions.Options, expire bool) *sessions.Options { + res := *opts + res.Path = "/" + if u, err := url.Parse(a.admin.URLs.AuthLogoutProvider("")); err == nil { + res.Path = u.Path + } + if expire { + res.MaxAge = -1 + } + return &res +} + // authLogoutCallback is called by the auth provider when a logout flow iniated by authLogout has completed. // // For orgs with a custom domain configured, the auth provider will still redirect back to the canonical domain's authLogoutCallback. diff --git a/admin/server/auth/handlers_test.go b/admin/server/auth/handlers_test.go index 910778c02bd8..aeb57fcbbce0 100644 --- a/admin/server/auth/handlers_test.go +++ b/admin/server/auth/handlers_test.go @@ -1,11 +1,17 @@ package auth import ( + "context" + "crypto/rand" + "crypto/rsa" "net/http" "net/http/httptest" "net/url" + "strings" "testing" + "time" + "github.com/coreos/go-oidc/v3/oidc" "github.com/rilldata/rill/admin" "github.com/rilldata/rill/admin/server/cookies" "github.com/stretchr/testify/require" @@ -19,10 +25,15 @@ func newTestAuthenticator(t *testing.T, authDomain string) *Authenticator { urls, err := admin.NewURLs("http://localhost:8080", "http://localhost:3000") require.NoError(t, err) + // Cookie options as set in server.New + cookieStore := cookies.New(zap.NewNop(), []byte("0123456789abcdef0123456789abcdef"), []byte("0123456789abcdef")) + cookieStore.Options.HttpOnly = true + cookieStore.Options.SameSite = http.SameSiteLaxMode + return &Authenticator{ logger: zap.NewNop(), admin: &admin.Service{URLs: urls}, - cookies: cookies.New(zap.NewNop(), []byte("0123456789abcdef0123456789abcdef"), []byte("0123456789abcdef")), + cookies: cookieStore, opts: &AuthenticatorOptions{ AuthDomain: authDomain, AuthClientID: "rill-client", @@ -96,3 +107,107 @@ func TestAuthLogoutProvider(t *testing.T) { }) } } + +func TestIDTokenHint(t *testing.T) { + p := newTestProvider(t, nil) + provider, err := oidc.NewProvider(context.Background(), p.URL) + require.NoError(t, err) + + newAuthenticator := func(endSessionEndpoint string) *Authenticator { + a := newTestAuthenticator(t, p.URL) + a.oidc = provider + a.endSessionEndpoint = endSessionEndpoint + return a + } + idToken := func(key *rsa.PrivateKey, exp time.Time) string { + return p.signIDToken(t, key, map[string]any{"iss": p.URL, "aud": "rill-client", "sub": "user", "iat": exp.Add(-5 * time.Minute).Unix(), "exp": exp.Unix()}) + } + // save runs saveIDTokenHint as the login callback would and returns the cookie it set, if any. + save := func(a *Authenticator, rawIDToken string) *http.Cookie { + req := httptest.NewRequest(http.MethodGet, "http://localhost:8080/auth/callback", nil) + w := httptest.NewRecorder() + a.saveIDTokenHint(w, req, rawIDToken) + require.Equal(t, http.StatusOK, w.Code) // It never fails the login + for _, c := range w.Result().Cookies() { + if c.Name == idTokenCookieName { + return c + } + } + return nil + } + // logout runs authLogoutProvider with the given cookie and returns the id_token_hint it sent and whether it cleared the cookie. + logout := func(a *Authenticator, c *http.Cookie) (string, bool) { + req := httptest.NewRequest(http.MethodGet, "http://localhost:8080/auth/logout/provider", nil) + req.AddCookie(c) + w := httptest.NewRecorder() + a.authLogoutProvider(w, req) + require.Equal(t, http.StatusTemporaryRedirect, w.Code) + loc, err := url.Parse(w.Header().Get("Location")) + require.NoError(t, err) + cleared := false + for _, rc := range w.Result().Cookies() { + if rc.Name == idTokenCookieName && rc.MaxAge < 0 && rc.Path == "/auth/logout/provider" { + cleared = true + } + } + return loc.Query().Get("id_token_hint"), cleared + } + + t.Run("saved only for providers with an end_session_endpoint", func(t *testing.T) { + require.Nil(t, save(newAuthenticator(""), idToken(nil, time.Now().Add(5*time.Minute)))) + }) + + t.Run("scoped to the logout path", func(t *testing.T) { + c := save(newAuthenticator(p.URL+"/logout"), idToken(nil, time.Now().Add(5*time.Minute))) + require.NotNil(t, c) + require.Equal(t, "/auth/logout/provider", c.Path) + require.True(t, c.HttpOnly) + }) + + t.Run("skipped when too large for a cookie", func(t *testing.T) { + require.Nil(t, save(newAuthenticator(p.URL+"/logout"), strings.Repeat("x", 5000))) + }) + + tests := []struct { + name string + idToken string + wantHint bool + }{ + {"valid", idToken(nil, time.Now().Add(5*time.Minute)), true}, + // The ID token expires within minutes but the Rill session lasts for weeks, and providers accept expired hints. + {"expired", idToken(nil, time.Now().Add(-24*time.Hour)), true}, + // An invalid hint makes Keycloak fail the logout with an error page, so it is dropped. + {"signed by another key", idToken(mustRSAKey(t), time.Now().Add(5*time.Minute)), false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + a := newAuthenticator(p.URL + "/logout") + c := save(a, tt.idToken) + require.NotNil(t, c) + + hint, cleared := logout(a, c) + require.True(t, cleared) + if tt.wantHint { + require.Equal(t, tt.idToken, hint) + } else { + require.Empty(t, hint) + } + }) + } + + t.Run("undecodable cookie leaves the store options alone", func(t *testing.T) { + a := newAuthenticator(p.URL + "/logout") + before := *a.cookies.Options + + hint, cleared := logout(a, &http.Cookie{Name: idTokenCookieName, Value: "garbage"}) + require.Empty(t, hint) + require.True(t, cleared) + require.Equal(t, before, *a.cookies.Options) + }) +} + +func mustRSAKey(t *testing.T) *rsa.PrivateKey { + key, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + return key +} From 3352761db8cd1df92602e47000d818718fe1045d Mon Sep 17 00:00:00 2001 From: dfliess Date: Mon, 21 Sep 2026 22:10:47 +0200 Subject: [PATCH 6/6] fix(auth): make the picture claim optional The login callback returned 500 "claim 'picture' not found" when the ID token had no picture. Dex never emits one, and Keycloak only does for users with a picture attribute, so login failed on both. Treat a missing picture as no photo, as pending members already have. The claim checks move to parseUserProfile, unchanged otherwise, so they can be tested without a database. --- admin/server/auth/handlers.go | 75 ++++++++++++++++++------------ admin/server/auth/handlers_test.go | 52 +++++++++++++++++++++ 2 files changed, 98 insertions(+), 29 deletions(-) diff --git a/admin/server/auth/handlers.go b/admin/server/auth/handlers.go index e6744eba1ef3..e4912ac21568 100644 --- a/admin/server/auth/handlers.go +++ b/admin/server/auth/handlers.go @@ -281,33 +281,9 @@ func (a *Authenticator) authLoginCallback(w http.ResponseWriter, r *http.Request http.Error(w, err.Error(), http.StatusInternalServerError) return } - email, ok := profile["email"].(string) - if !ok || email == "" { - http.Error(w, "claim 'email' not found", http.StatusInternalServerError) - return - } - emailVerified, ok := profile["email_verified"].(bool) - if !ok { - // For SAML flows, it is passed as a string - emailVerifiedStr, ok := profile["email_verified"].(string) - if !ok { - http.Error(w, "claim 'email_verified' not found", http.StatusInternalServerError) - return - } - emailVerified, err = strconv.ParseBool(emailVerifiedStr) - if err != nil { - http.Error(w, fmt.Sprintf("claim 'email_verified' could not be parsed as a boolean (got %q)", emailVerifiedStr), http.StatusInternalServerError) - return - } - } - name, ok := profile["name"].(string) - if !ok { - http.Error(w, "claim 'name' not found", http.StatusInternalServerError) - return - } - photoURL, ok := profile["picture"].(string) - if !ok { - http.Error(w, "claim 'picture' not found", http.StatusInternalServerError) + info, err := parseUserProfile(profile) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) return } @@ -319,14 +295,14 @@ func (a *Authenticator) authLoginCallback(w http.ResponseWriter, r *http.Request delete(sess.Values, cookieFieldRedirect) // Check that the user's email is verified - if !emailVerified { + if !info.emailVerified { redirectURL := a.admin.URLs.WithCustomDomainFromRedirectURL(redirect).AuthVerifyEmailUI() http.Redirect(w, r, redirectURL, http.StatusTemporaryRedirect) return } // Create (or update) user in our DB - user, err := a.admin.CreateOrUpdateUser(r.Context(), email, name, photoURL) + user, err := a.admin.CreateOrUpdateUser(r.Context(), info.email, info.name, info.photoURL) if err != nil { http.Error(w, fmt.Sprintf("failed to update user: %s", err), http.StatusInternalServerError) return @@ -388,6 +364,47 @@ func (a *Authenticator) authLoginCallback(w http.ResponseWriter, r *http.Request http.Redirect(w, r, redirect, http.StatusTemporaryRedirect) } +// userProfile is the user information authLoginCallback reads from the ID token claims. +type userProfile struct { + email string + emailVerified bool + name string + photoURL string +} + +// parseUserProfile reads the user's profile from the ID token claims. +func parseUserProfile(claims map[string]any) (*userProfile, error) { + email, ok := claims["email"].(string) + if !ok || email == "" { + return nil, errors.New("claim 'email' not found") + } + emailVerified, ok := claims["email_verified"].(bool) + if !ok { + // For SAML flows, it is passed as a string + emailVerifiedStr, ok := claims["email_verified"].(string) + if !ok { + return nil, errors.New("claim 'email_verified' not found") + } + var err error + emailVerified, err = strconv.ParseBool(emailVerifiedStr) + if err != nil { + return nil, fmt.Errorf("claim 'email_verified' could not be parsed as a boolean (got %q)", emailVerifiedStr) + } + } + name, ok := claims["name"].(string) + if !ok { + return nil, errors.New("claim 'name' not found") + } + // The picture claim is optional: some providers never emit it (e.g. Dex), or only for users who have one (e.g. Keycloak) + photoURL, _ := claims["picture"].(string) + return &userProfile{ + email: email, + emailVerified: emailVerified, + name: name, + photoURL: photoURL, + }, nil +} + // authLoginCustomDomainCallback first verifies the state for CSRF protection, then extracts // a nonce token from the query parameters and validates it. If valid, it issues a new // long-lived token, stores it in a cookie, and redirects the user to the frontend. diff --git a/admin/server/auth/handlers_test.go b/admin/server/auth/handlers_test.go index aeb57fcbbce0..1a56c601729b 100644 --- a/admin/server/auth/handlers_test.go +++ b/admin/server/auth/handlers_test.go @@ -211,3 +211,55 @@ func mustRSAKey(t *testing.T) *rsa.PrivateKey { require.NoError(t, err) return key } + +func TestParseUserProfile(t *testing.T) { + tests := []struct { + name string + claims map[string]any + want *userProfile + wantErr string + }{ + { + name: "all claims", + claims: map[string]any{"email": "a@example.com", "email_verified": true, "name": "A", "picture": "https://example.com/a.png"}, + want: &userProfile{email: "a@example.com", emailVerified: true, name: "A", photoURL: "https://example.com/a.png"}, + }, + { + // Dex never emits picture, and Keycloak omits it for users without a picture attribute. + name: "without picture", + claims: map[string]any{"email": "a@example.com", "email_verified": true, "name": "A"}, + want: &userProfile{email: "a@example.com", emailVerified: true, name: "A"}, + }, + { + name: "email_verified as a string", + claims: map[string]any{"email": "a@example.com", "email_verified": "false", "name": "A", "picture": ""}, + want: &userProfile{email: "a@example.com", emailVerified: false, name: "A"}, + }, + { + name: "without email", + claims: map[string]any{"email_verified": true, "name": "A"}, + wantErr: "claim 'email' not found", + }, + { + name: "without email_verified", + claims: map[string]any{"email": "a@example.com", "name": "A"}, + wantErr: "claim 'email_verified' not found", + }, + { + name: "without name", + claims: map[string]any{"email": "a@example.com", "email_verified": true}, + wantErr: "claim 'name' not found", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := parseUserProfile(tt.claims) + if tt.wantErr != "" { + require.EqualError(t, err, tt.wantErr) + return + } + require.NoError(t, err) + require.Equal(t, tt.want, got) + }) + } +}