Fix Logout Path, enable logout handler capabilities (#538)

* fixes for logout handler

* linter

Author: matoszz

auth/auth.go

@@ -113,10 +113,13 @@ func SetAuthCookies(w http.ResponseWriter, accessToken, refreshToken string, c s
 	sessions.SetCookie(w, refreshToken, RefreshTokenCookie, c)
 }
 
-// ClearAuthCookies is a helper function to clear authentication cookies on a echo
-// request to effectively logger out a user.
-func ClearAuthCookies(w http.ResponseWriter) {
-	sessions.RemoveCookies(w, *sessions.DefaultCookieConfig, AccessTokenCookie, RefreshTokenCookie)
+// ClearAuthCookies clears the access and refresh token cookies to effectively log out a user. The
+// supplied CookieConfig must match the one the cookies were set with via SetAuthCookies (domain,
+// path, secure, samesite); otherwise the browser will not match the expiry cookie and the original
+// cookies will persist. Pass the same SessionConfig.CookieConfig used at login rather than a global
+// default.
+func ClearAuthCookies(w http.ResponseWriter, c sessions.CookieConfig) {
+	sessions.RemoveCookies(w, c, AccessTokenCookie, RefreshTokenCookie)
 }
 
 // CookieExpired checks to see if a cookie is expired

auth/auth_test.go

@@ -259,7 +259,7 @@ func TestClearAuthCookies(t *testing.T) {
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(_ *testing.T) {
-			auth.ClearAuthCookies(tc.ctx.Response().Writer)
+			auth.ClearAuthCookies(tc.ctx.Response().Writer, *sessions.DebugOnlyCookieConfig)
 		})
 	}
 }

sessions/cookiestore.go

@@ -25,7 +25,8 @@ type Store[T any] interface {
 	Get(req *http.Request, name string) (*Session[T], error)
 	// Save writes a Session to the ResponseWriter
 	Save(w http.ResponseWriter, session *Session[T]) error
-	// Destroy removes (expires) a named Session
+	// Destroy expires the named session cookie on the response. This only clears the client cookie
+	// and does not remove any persisted session; use SessionConfig.DestroySession on logout
 	Destroy(w http.ResponseWriter, name string)
 	// GetSessionIDFromCookie returns the key, which should be the sessionID, in the map
 	GetSessionIDFromCookie(sess *Session[T]) string
@@ -115,14 +116,19 @@ func (s *cookieStore[T]) Save(w http.ResponseWriter, session *Session[T]) error
 	return nil
 }
 
+// EncodeCookie encodes the session values into a cookie value string
 func (s *cookieStore[T]) EncodeCookie(session *Session[T]) (string, error) {
 	return securecookie.EncodeMulti(session.Name(), &session.values, s.codecs...)
 }
 
-// Destroy deletes the Session with the given name by issuing an expired
-// session cookie with the same name.
+// Destroy expires the session cookie with the given name by issuing an expired cookie. It only
+// clears the client cookie and does not remove any persisted session; use
+// SessionConfig.DestroySession to also delete the session from the backing store on logout.
 func (s *cookieStore[T]) Destroy(w http.ResponseWriter, name string) {
-	http.SetCookie(w, NewCookie(name, "", &CookieConfig{MaxAge: -1, Path: s.config.Path}))
+	// reuse the full store config so the deletion cookie matches the original on Domain, Path,
+	// Secure, and SameSite; a deletion cookie that omits the Domain will not match a cookie that was
+	// set with one and the browser will keep it
+	RemoveCookie(w, name, *s.config)
 }
 
 // NewSessionCookie creates a cookie from a session id

sessions/destroysession_test.go

@@ -0,0 +1,241 @@
+package sessions_test
+
+import (
+	"context"
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/alicebob/miniredis/v2"
+	"github.com/redis/go-redis/v9"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/theopenlane/iam/sessions"
+)
+
+// errDeleteSession simulates a backing-store failure when deleting a persisted session
+var errDeleteSession = errors.New("delete failed")
+
+// failingDeleteStore wraps a PersistentStore but always fails DeleteSession, used to exercise the
+// error path of DestroySession
+type failingDeleteStore struct {
+	sessions.PersistentStore
+}
+
+// DeleteSession always returns an error
+func (failingDeleteStore) DeleteSession(context.Context, string) error {
+	return errDeleteSession
+}
+
+// newDestroyTestConfig builds a redis-backed session config sharing a single named cookie config
+// between the cookie store and the session config, mirroring how the server wires sessions
+func newDestroyTestConfig(t *testing.T) (sessions.SessionConfig, sessions.Store[map[string]any], *miniredis.Miniredis) {
+	t.Helper()
+
+	mr, err := miniredis.Run()
+	require.NoError(t, err)
+
+	rc := redis.NewClient(&redis.Options{Addr: mr.Addr()})
+
+	cc := sessions.DebugOnlyCookieConfig
+	sm := sessions.NewCookieStore[map[string]any](cc, []byte("my-signing-secret"), []byte("encryptionsecret"))
+
+	sc := sessions.NewSessionConfig(sm, sessions.WithPersistence(rc))
+	sc.CookieConfig = cc
+
+	return sc, sm, mr
+}
+
+// requestWithSessionCookie creates a session in the store and returns a request carrying its cookie
+func requestWithSessionCookie(t *testing.T, sc sessions.SessionConfig, userID string) *http.Request {
+	t.Helper()
+
+	createRec := httptest.NewRecorder()
+	_, err := sc.CreateAndStoreSession(context.Background(), createRec, userID)
+	require.NoError(t, err)
+
+	req := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "/logout", nil)
+	for _, c := range createRec.Result().Cookies() {
+		req.AddCookie(c)
+	}
+
+	return req
+}
+
+func TestDestroySession(t *testing.T) {
+	ctx := context.Background()
+
+	t.Run("removes the persisted session and expires the cookie", func(t *testing.T) {
+		sc, _, mr := newDestroyTestConfig(t)
+		defer mr.Close()
+
+		req := requestWithSessionCookie(t, sc, "user-123")
+
+		session, err := sc.SessionManager.Get(req, sc.CookieConfig.Name)
+		require.NoError(t, err)
+
+		sessionID := sc.SessionManager.GetSessionIDFromCookie(session)
+		require.NotEmpty(t, sessionID)
+
+		exists, err := sc.RedisStore.Exists(ctx, sessionID)
+		require.NoError(t, err)
+		require.Equal(t, int64(1), exists)
+
+		destroyRec := httptest.NewRecorder()
+		err = sc.DestroySession(ctx, destroyRec, req)
+		require.NoError(t, err)
+
+		// the persisted session is gone
+		exists, err = sc.RedisStore.Exists(ctx, sessionID)
+		require.NoError(t, err)
+		assert.Equal(t, int64(0), exists)
+
+		// the cookie is expired on the response
+		cookies := destroyRec.Result().Cookies()
+		require.Len(t, cookies, 1)
+		assert.Equal(t, sc.CookieConfig.Name, cookies[0].Name)
+		assert.Equal(t, -1, cookies[0].MaxAge)
+	})
+
+	t.Run("no session cookie is a no-op that still returns nil", func(t *testing.T) {
+		sc, _, mr := newDestroyTestConfig(t)
+		defer mr.Close()
+
+		req := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "/logout", nil)
+		rec := httptest.NewRecorder()
+
+		err := sc.DestroySession(ctx, rec, req)
+		assert.NoError(t, err)
+	})
+
+	t.Run("malformed session cookie is a no-op that still returns nil", func(t *testing.T) {
+		sc, _, mr := newDestroyTestConfig(t)
+		defer mr.Close()
+
+		req := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "/logout", nil)
+		req.AddCookie(&http.Cookie{
+			Name:     sc.CookieConfig.Name,
+			Value:    "not-a-valid-encoded-cookie",
+			Secure:   true,
+			HttpOnly: true,
+			SameSite: http.SameSiteStrictMode,
+		})
+
+		rec := httptest.NewRecorder()
+
+		err := sc.DestroySession(ctx, rec, req)
+		assert.NoError(t, err)
+	})
+
+	t.Run("returns an error and keeps the cookie when the persisted session cannot be deleted", func(t *testing.T) {
+		sc, _, mr := newDestroyTestConfig(t)
+		defer mr.Close()
+
+		req := requestWithSessionCookie(t, sc, "user-err")
+
+		// swap in a store whose DeleteSession always fails
+		failing := sc
+		failing.RedisStore = failingDeleteStore{PersistentStore: sc.RedisStore}
+
+		destroyRec := httptest.NewRecorder()
+		err := failing.DestroySession(ctx, destroyRec, req)
+
+		// the failure is surfaced, not swallowed
+		assert.ErrorIs(t, err, errDeleteSession)
+
+		// the cookie is NOT expired, so the client is forced to retry rather than believe it logged out
+		assert.Empty(t, destroyRec.Result().Cookies())
+	})
+
+	t.Run("expires the cookie without a persistent store", func(t *testing.T) {
+		cc := sessions.DebugOnlyCookieConfig
+		sm := sessions.NewCookieStore[map[string]any](cc, []byte("my-signing-secret"), []byte("encryptionsecret"))
+
+		// no WithPersistence, so RedisStore is nil
+		sc := sessions.NewSessionConfig(sm)
+		sc.CookieConfig = cc
+		require.Nil(t, sc.RedisStore)
+
+		// manually create a session cookie using the same store
+		setRec := httptest.NewRecorder()
+		session := sm.New(cc.Name)
+		session.Set(sessions.GenerateSessionID(), map[string]any{sessions.UserIDKey: "user-x"})
+		require.NoError(t, session.Save(setRec))
+
+		req := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "/logout", nil)
+		for _, c := range setRec.Result().Cookies() {
+			req.AddCookie(c)
+		}
+
+		destroyRec := httptest.NewRecorder()
+		err := sc.DestroySession(ctx, destroyRec, req)
+		assert.NoError(t, err)
+
+		cookies := destroyRec.Result().Cookies()
+		require.Len(t, cookies, 1)
+		assert.Equal(t, -1, cookies[0].MaxAge)
+	})
+}
+
+// TestCookieStoreDestroy covers the fix that makes Store.Destroy build the deletion cookie from the
+// full store config rather than only the path, so it reliably matches the original cookie
+func TestCookieStoreDestroy(t *testing.T) {
+	t.Run("replaces the original cookie with an expired one matching its identity", func(t *testing.T) {
+		cfg := &sessions.CookieConfig{
+			Name:     "sess",
+			Domain:   "example.com",
+			Path:     "/app",
+			Secure:   true,
+			HTTPOnly: true,
+			SameSite: http.SameSiteStrictMode,
+		}
+		cs := sessions.NewCookieStore[map[string]any](cfg, []byte("my-signing-secret"), []byte("encryptionsecret"))
+
+		// set a real session cookie and capture the original that we intend to remove
+		setRec := httptest.NewRecorder()
+		session := cs.New(cfg.Name)
+		session.Set(sessions.GenerateSessionID(), map[string]any{sessions.UserIDKey: "user-x"})
+		require.NoError(t, session.Save(setRec))
+
+		setCookies := setRec.Result().Cookies()
+		require.Len(t, setCookies, 1)
+
+		original := setCookies[0]
+		require.NotEmpty(t, original.Value)
+		require.Equal(t, "example.com", original.Domain) // the attribute the buggy deletion dropped
+
+		// destroy and capture the deletion cookie
+		destroyRec := httptest.NewRecorder()
+		cs.Destroy(destroyRec, cfg.Name)
+
+		delCookies := destroyRec.Result().Cookies()
+		require.Len(t, delCookies, 1)
+
+		deletion := delCookies[0]
+
+		// the deletion cookie must share the original's identity (name+domain+path) and security
+		// attributes, otherwise the browser stores a second, non-matching cookie and keeps the original.
+		// This is exactly what the old Destroy did by dropping Domain/Secure/SameSite
+		assert.Equal(t, original.Name, deletion.Name)
+		assert.Equal(t, original.Domain, deletion.Domain)
+		assert.Equal(t, original.Path, deletion.Path)
+		assert.Equal(t, original.Secure, deletion.Secure)
+		assert.Equal(t, original.SameSite, deletion.SameSite)
+
+		// and it must actually expire and empty the value so the original is replaced, not duplicated
+		assert.Equal(t, -1, deletion.MaxAge)
+		assert.Empty(t, deletion.Value)
+	})
+
+	t.Run("sets no cookie when no name can be resolved", func(t *testing.T) {
+		cfg := &sessions.CookieConfig{Path: "/"} // empty Name
+		cs := sessions.NewCookieStore[map[string]any](cfg, []byte("my-signing-secret"), []byte("encryptionsecret"))
+
+		rec := httptest.NewRecorder()
+		cs.Destroy(rec, "") // empty name and empty config name resolves to no cookie
+
+		assert.Empty(t, rec.Result().Cookies())
+	})
+}

sessions/middleware.go

@@ -2,6 +2,7 @@ package sessions
 
 import (
 	"context"
+	"errors"
 	"net/http"
 	"slices"
 	"time"
@@ -115,6 +116,43 @@ func (sc *SessionConfig) SaveAndStoreSession(ctx context.Context, w http.Respons
 	return c, nil
 }
 
+// DestroySession is the inverse of CreateAndStoreSession: it removes the session presented on the
+// request by deleting the persisted session from the backing store (e.g. redis) and expiring the
+// session cookie on the response. This is the correct way to invalidate a session on logout, since
+// the cookie-only Store.Destroy and Session.Destroy leave the persisted session in place.
+//
+// A request that carries no resolvable session is treated as already destroyed: the cookie is still
+// expired and nil is returned, which keeps logout idempotent. A cookie that is present but cannot be
+// decoded is anomalous (tampering, corruption, or a dropped signing key); it cannot identify a
+// persisted session to delete, so it is logged and the bad cookie is cleared, but it is not treated
+// as a hard failure since it is not retryable and decode errors can occur benignly during key
+// rotation. An error is only returned when an identified, persisted session could not be deleted, so
+// callers can avoid reporting a logout that did not take effect
+func (sc *SessionConfig) DestroySession(ctx context.Context, w http.ResponseWriter, req *http.Request) error {
+	session, err := sc.SessionManager.Get(req, sc.CookieConfig.Name)
+	if err != nil {
+		// a missing cookie is normal; a cookie that is present but undecodable is anomalous and logged
+		if !errors.Is(err, http.ErrNoCookie) {
+			log.Warn().Err(err).Msg("could not decode session cookie on destroy; clearing it")
+		}
+
+		// no resolvable session to remove server-side, but still expire whatever cookie was presented
+		RemoveCookie(w, sc.CookieConfig.Name, *sc.CookieConfig)
+
+		return nil
+	}
+
+	if sessionID := sc.SessionManager.GetSessionIDFromCookie(session); sessionID != "" && sc.RedisStore != nil {
+		if err := sc.RedisStore.DeleteSession(ctx, sessionID); err != nil {
+			return err
+		}
+	}
+
+	RemoveCookie(w, sc.CookieConfig.Name, *sc.CookieConfig)
+
+	return nil
+}
+
 // LoadAndSave is a middleware function that loads and saves session data using a
 // provided session manager. It takes a `SessionManager` as input and returns a middleware function
 // that can be used with an Echo framework application

sessions/sessions.go

@@ -83,8 +83,10 @@ func (s *Session[T]) Save(w http.ResponseWriter) error {
 	return s.store.Save(w, s)
 }
 
-// Destroy destroys the session. Identical to calling
-// store.Destroy(w, session.name).
+// Destroy expires the session cookie on the response. Identical to calling
+// store.Destroy(w, session.name). This only clears the client cookie and does not remove any
+// persisted session; use SessionConfig.DestroySession to also delete the session from the backing
+// store on logout.
 func (s *Session[T]) Destroy(w http.ResponseWriter) {
 	s.store.Destroy(w, s.name)
 }

tokens/blacklist_test.go

@@ -506,17 +506,18 @@ func TestGeneralTokenBlacklist(t *testing.T) {
 		verifiedClaims, err := tmNoBlacklist.VerifyWithContext(ctx, tokenString)
 		assert.NoError(t, err)
 
-		// Try to revoke (should be no-op)
+		// Try to revoke; without a functional blacklist this surfaces an error rather than
+		// silently succeeding, but the token remains valid since the revocation had no effect
 		err = tmNoBlacklist.RevokeToken(ctx, verifiedClaims.ID, 30*time.Minute)
-		assert.NoError(t, err)
+		assert.ErrorIs(t, err, tokens.ErrRevocationNotConfigured)
 
 		// Token should still work since no blacklist
 		_, err = tmNoBlacklist.VerifyWithContext(ctx, tokenString)
 		assert.NoError(t, err)
 
-		// Try to suspend user (should be no-op)
+		// Try to suspend user; same as revoke, this surfaces an error without a functional blacklist
 		err = tmNoBlacklist.SuspendUser(ctx, "user-no-blacklist", 30*time.Minute)
-		assert.NoError(t, err)
+		assert.ErrorIs(t, err, tokens.ErrRevocationNotConfigured)
 
 		// Token should still work since no blacklist
 		_, err = tmNoBlacklist.VerifyWithContext(ctx, tokenString)

tokens/errors.go

@@ -142,6 +142,9 @@ var (
 	ErrInvalidTokenID = errors.New("invalid token ID")
 	// ErrKeyLifecycleNotEnabled is returned when key lifecycle management is not enabled
 	ErrKeyLifecycleNotEnabled = errors.New("key lifecycle management not enabled")
+	// ErrRevocationNotConfigured is returned when a revocation or suspension operation is attempted
+	// while only the no-op blacklist is installed, meaning the operation has no effect
+	ErrRevocationNotConfigured = errors.New("token revocation is not configured: no functional blacklist is installed")
 )
 
 // The errors that might occur when parsing and validating a token