From 4e1c8e68f820a7b89dcab88ecf09872029c321c0 Mon Sep 17 00:00:00 2001 From: Trong Huu Nguyen Date: Fri, 13 Jun 2025 09:09:36 +0200 Subject: [PATCH] feat(openid): retry front-channel logouts --- pkg/handler/handler.go | 34 +++++++++++++++---- pkg/openid/client/client.go | 4 --- pkg/openid/client/client_test.go | 4 --- pkg/openid/client/logout_frontchannel.go | 27 --------------- pkg/openid/client/logout_frontchannel_test.go | 34 ------------------- 5 files changed, 27 insertions(+), 76 deletions(-) delete mode 100644 pkg/openid/client/logout_frontchannel.go delete mode 100644 pkg/openid/client/logout_frontchannel_test.go diff --git a/pkg/handler/handler.go b/pkg/handler/handler.go index a3a427d..579f997 100644 --- a/pkg/handler/handler.go +++ b/pkg/handler/handler.go @@ -417,23 +417,43 @@ func (s *Standalone) LogoutFrontChannel(w http.ResponseWriter, r *http.Request) // Unconditionally destroy all local references to the session. cookie.Clear(w, cookie.Session, s.GetCookieOptions(r)) - lfc := s.Client.LogoutFrontchannel(r) - if lfc.MissingSidParameter() { + // sid is the session identifier that SHOULD be included as a parameter in the front-channel logout request. + sid := r.URL.Query().Get("sid") + if sid == "" { span.SetAttributes(attribute.Bool("logout.frontchannel.missing_sid_parameter", true)) logger.Debugf("front-channel logout: sid parameter not found in request; ignoring") w.WriteHeader(http.StatusAccepted) return } - id := lfc.Sid() - err := s.SessionManager.DeleteForExternalID(r.Context(), id) - if err != nil { - logger.Warnf("front-channel logout: destroying session with id %q: %+v", id, err) + if err := s.SessionManager.DeleteForExternalID(r.Context(), sid); err != nil { + if errors.Is(err, session.ErrNotFound) { + w.WriteHeader(http.StatusOK) + return + } + + logger.Infof("front-channel logout: destroying session with id %q: %+v", sid, err) + go func() { + // attempt background delete with retries + err := retry.Do(context.Background(), func(ctx context.Context) error { + err = s.SessionManager.DeleteForExternalID(ctx, sid) + if err == nil || errors.Is(err, session.ErrNotFound) { + return nil + } + return retry.RetryableError(err) + }, retry.WithMax(10*time.Minute)) + if err != nil { + logger.Warnf("front-channel logout: retries exhausted for deletion of session with id %q: %+v", sid, err) + } else { + logger.WithField("sid", sid).Info("front-channel logout: session deleted in background") + } + }() + w.WriteHeader(http.StatusAccepted) return } - logger.WithField("sid", id).Info("front-channel logout: session deleted") + logger.WithField("sid", sid).Info("front-channel logout: session deleted") cookie.Clear(w, cookie.Retry, s.GetCookieOptions(r)) metrics.ObserveLogout(metrics.LogoutOperationFrontChannel) w.WriteHeader(http.StatusOK) diff --git a/pkg/openid/client/client.go b/pkg/openid/client/client.go index bf20bc3..f05f320 100644 --- a/pkg/openid/client/client.go +++ b/pkg/openid/client/client.go @@ -87,10 +87,6 @@ func (c *Client) LogoutCallback(r *http.Request, cookie *openid.LogoutCookie, va return NewLogoutCallback(c, r, cookie, validator) } -func (c *Client) LogoutFrontchannel(r *http.Request) *LogoutFrontchannel { - return NewLogoutFrontchannel(r) -} - func (c *Client) AuthCodeGrant(ctx context.Context, code string, opts []oauth2.AuthCodeOption) (*oauth2.Token, error) { ctx = context.WithValue(ctx, oauth2.HTTPClient, c.httpClient) return c.oauth2Config.Exchange(ctx, code, opts...) diff --git a/pkg/openid/client/client_test.go b/pkg/openid/client/client_test.go index 2dd9d7f..50edbe9 100644 --- a/pkg/openid/client/client_test.go +++ b/pkg/openid/client/client_test.go @@ -86,7 +86,3 @@ func newTestClientWithConfig(config *mock.TestConfiguration) *client.Client { jwksProvider := mock.NewTestJwksProvider() return client.NewClient(config, jwksProvider) } - -func newTestClient() *client.Client { - return newTestClientWithConfig(mock.NewTestConfiguration(mock.Config())) -} diff --git a/pkg/openid/client/logout_frontchannel.go b/pkg/openid/client/logout_frontchannel.go deleted file mode 100644 index ac0fe46..0000000 --- a/pkg/openid/client/logout_frontchannel.go +++ /dev/null @@ -1,27 +0,0 @@ -package client - -import ( - "net/http" -) - -type LogoutFrontchannel struct { - sid string -} - -func NewLogoutFrontchannel(r *http.Request) *LogoutFrontchannel { - params := r.URL.Query() - sid := params.Get("sid") - - return &LogoutFrontchannel{ - sid: sid, - } -} - -// Sid is the session identifier which SHOULD be included as a parameter in the front-channel logout request. -func (l *LogoutFrontchannel) Sid() string { - return l.sid -} - -func (l *LogoutFrontchannel) MissingSidParameter() bool { - return len(l.sid) <= 0 -} diff --git a/pkg/openid/client/logout_frontchannel_test.go b/pkg/openid/client/logout_frontchannel_test.go deleted file mode 100644 index 8e48537..0000000 --- a/pkg/openid/client/logout_frontchannel_test.go +++ /dev/null @@ -1,34 +0,0 @@ -package client_test - -import ( - "net/http" - "net/http/httptest" - "testing" - - "github.com/stretchr/testify/assert" - - "github.com/nais/wonderwall/pkg/openid/client" -) - -func TestLogoutFrontchannel_Sid(t *testing.T) { - t.Run("missing sid parameter in request", func(t *testing.T) { - url := "http://localhost/oauth2/logout/frontchannel" - lf := newLogoutFrontchannel(url) - - assert.Empty(t, lf.Sid()) - assert.True(t, lf.MissingSidParameter()) - }) - - t.Run("has sid parameter in request", func(t *testing.T) { - url := "http://localhost/oauth2/logout/frontchannel?sid=some-session-id" - lf := newLogoutFrontchannel(url) - - assert.Equal(t, "some-session-id", lf.Sid()) - assert.False(t, lf.MissingSidParameter()) - }) -} - -func newLogoutFrontchannel(url string) *client.LogoutFrontchannel { - req := httptest.NewRequest(http.MethodGet, url, nil) - return newTestClient().LogoutFrontchannel(req) -}