From 0f05438735c1c40e565150afa45cc05fd88fbfa7 Mon Sep 17 00:00:00 2001 From: Christian Richter Date: Thu, 12 Feb 2026 12:42:10 +0100 Subject: [PATCH 01/11] add additional validation to logout token Signed-off-by: Christian Richter Co-authored-by: Michael Barz --- .../pkg/staticroutes/backchannellogout.go | 71 ++++++++++++++----- 1 file changed, 53 insertions(+), 18 deletions(-) diff --git a/services/proxy/pkg/staticroutes/backchannellogout.go b/services/proxy/pkg/staticroutes/backchannellogout.go index 0c67a4d951..f016b6c437 100644 --- a/services/proxy/pkg/staticroutes/backchannellogout.go +++ b/services/proxy/pkg/staticroutes/backchannellogout.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net/http" + "strings" "github.com/go-chi/render" "github.com/opencloud-eu/opencloud/pkg/oidc" @@ -25,6 +26,12 @@ func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Re return } + if r.PostFormValue("logout_token") == "" { + logger.Warn().Msg("logout_token is missing") + render.Status(r, http.StatusBadRequest) + render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: "logout_token is missing"}) + } + logoutToken, err := s.OidcClient.VerifyLogoutToken(r.Context(), r.PostFormValue("logout_token")) if err != nil { logger.Warn().Err(err).Msg("VerifyLogoutToken failed") @@ -33,24 +40,41 @@ func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Re return } - records, err := s.UserInfoCache.Read(logoutToken.SessionId) - if errors.Is(err, microstore.ErrNotFound) || len(records) == 0 { - render.Status(r, http.StatusOK) - render.JSON(w, r, nil) - return - } - if err != nil { - logger.Error().Err(err).Msg("Error reading userinfo cache") + var records []*microstore.Record + + if strings.TrimSpace(logoutToken.SessionId) != "" { + records, err = s.UserInfoCache.Read(logoutToken.SessionId) + if errors.Is(err, microstore.ErrNotFound) || len(records) == 0 { + render.Status(r, http.StatusOK) + render.JSON(w, r, nil) + return + } + if err != nil { + logger.Error().Err(err).Msg("Error reading userinfo cache") + render.Status(r, http.StatusBadRequest) + render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()}) + return + } + } else if strings.TrimSpace(logoutToken.Subject) != "" { + records, err = s.UserInfoCache.Read(logoutToken.Subject) + if errors.Is(err, microstore.ErrNotFound) || len(records) == 0 { + render.Status(r, http.StatusOK) + render.JSON(w, r, nil) + return + } + if err != nil { + logger.Error().Err(err).Msg("Error reading userinfo cache") + render.Status(r, http.StatusBadRequest) + render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()}) + return + } + } else { + logger.Warn().Msg("invalid logout token") render.Status(r, http.StatusBadRequest) - render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()}) - return + render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: "invalid logout token"}) } for _, record := range records { - err := s.publishBackchannelLogoutEvent(r.Context(), record, logoutToken) - if err != nil { - s.Logger.Warn().Err(err).Msg("could not publish backchannel logout event") - } err = s.UserInfoCache.Delete(string(record.Value)) if err != nil && !errors.Is(err, microstore.ErrNotFound) { // Spec requires us to return a 400 BadRequest when the session could not be destroyed @@ -59,13 +83,24 @@ func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Re render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()}) return } + err := s.publishBackchannelLogoutEvent(r.Context(), record, logoutToken) + if err != nil { + s.Logger.Warn().Err(err).Msg("could not publish backchannel logout event") + } logger.Debug().Msg("Deleted userinfo from cache") } - // we can ignore errors when cleaning up the lookup table - err = s.UserInfoCache.Delete(logoutToken.SessionId) - if err != nil { - logger.Debug().Err(err).Msg("Failed to cleanup sessionid lookup entry") + if strings.TrimSpace(logoutToken.SessionId) != "" { + // we can ignore errors when cleaning up the lookup table + err = s.UserInfoCache.Delete(logoutToken.SessionId) + if err != nil { + logger.Debug().Err(err).Msg("Failed to cleanup sessionid lookup entry") + } + } else if strings.TrimSpace(logoutToken.Subject) != "" { + err = s.UserInfoCache.Delete(logoutToken.Subject) + if err != nil { + logger.Debug().Err(err).Msg("Failed to cleanup subject lookup entry") + } } render.Status(r, http.StatusOK) From 95af43b69b6a768f020807d59c33b34143e4b82f Mon Sep 17 00:00:00 2001 From: Christian Richter Date: Thu, 12 Feb 2026 16:41:19 +0100 Subject: [PATCH 02/11] add mapping to backchannel logout for subject => sessionid Signed-off-by: Christian Richter --- .../pkg/staticroutes/backchannellogout.go | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/services/proxy/pkg/staticroutes/backchannellogout.go b/services/proxy/pkg/staticroutes/backchannellogout.go index f016b6c437..ad680813ee 100644 --- a/services/proxy/pkg/staticroutes/backchannellogout.go +++ b/services/proxy/pkg/staticroutes/backchannellogout.go @@ -30,6 +30,7 @@ func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Re logger.Warn().Msg("logout_token is missing") render.Status(r, http.StatusBadRequest) render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: "logout_token is missing"}) + return } logoutToken, err := s.OidcClient.VerifyLogoutToken(r.Context(), r.PostFormValue("logout_token")) @@ -56,6 +57,7 @@ func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Re return } } else if strings.TrimSpace(logoutToken.Subject) != "" { + // TODO: enter a mapping table between subject and sessionid when the oidc session is refreshed records, err = s.UserInfoCache.Read(logoutToken.Subject) if errors.Is(err, microstore.ErrNotFound) || len(records) == 0 { render.Status(r, http.StatusOK) @@ -68,10 +70,21 @@ func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Re render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()}) return } + for _, record := range records { + // take all previous records retrieved for this subject, and fetch the corresponding sessions + rs, err := s.UserInfoCache.Read(string(record.Value)) + if errors.Is(err, microstore.ErrNotFound) || len(rs) == 0 { + // we do not care about errors here, since we already have entries from the subjects that need to be addressed + continue + } + // we append the additional sessions found through the mapping for later deletion + records = append(records, rs...) + } } else { logger.Warn().Msg("invalid logout token") render.Status(r, http.StatusBadRequest) render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: "invalid logout token"}) + return } for _, record := range records { @@ -95,11 +108,18 @@ func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Re err = s.UserInfoCache.Delete(logoutToken.SessionId) if err != nil { logger.Debug().Err(err).Msg("Failed to cleanup sessionid lookup entry") + render.Status(r, http.StatusBadRequest) + render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()}) + return } } else if strings.TrimSpace(logoutToken.Subject) != "" { + // TODO: do a lookup subject => sessionid and delete both entries err = s.UserInfoCache.Delete(logoutToken.Subject) if err != nil { logger.Debug().Err(err).Msg("Failed to cleanup subject lookup entry") + render.Status(r, http.StatusBadRequest) + render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()}) + return } } From bc4b8772e70d911eda53b47818d4fc274a24f788 Mon Sep 17 00:00:00 2001 From: Christian Richter Date: Thu, 12 Feb 2026 16:51:42 +0100 Subject: [PATCH 03/11] create mapping in cache for subject => sessionid Signed-off-by: Christian Richter --- services/proxy/pkg/middleware/oidc_auth.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/services/proxy/pkg/middleware/oidc_auth.go b/services/proxy/pkg/middleware/oidc_auth.go index 18e0ef3344..46b4733dc9 100644 --- a/services/proxy/pkg/middleware/oidc_auth.go +++ b/services/proxy/pkg/middleware/oidc_auth.go @@ -124,6 +124,18 @@ func (m *OIDCAuthenticator) getClaims(token string, req *http.Request) (map[stri if err != nil { m.Logger.Error().Err(err).Msg("failed to write session lookup cache") } + + // create an additional entry mapping subject to session id + if sub := aClaims.Subject; sub != "" { + err = m.userInfoCache.Write(&store.Record{ + Key: sub, + Value: []byte(sid), + Expiry: time.Until(expiration), + }) + if err != nil { + m.Logger.Error().Err(err).Msg("failed to write subject lookup cache") + } + } } } }() From 92fe9a6928d508de5554dcdb57c3921cb053e37f Mon Sep 17 00:00:00 2001 From: Christian Richter Date: Thu, 12 Feb 2026 17:46:04 +0100 Subject: [PATCH 04/11] refactor deletion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Jörn Dreyer Co-authored-by: Michael Barz Signed-off-by: Christian Richter --- services/proxy/pkg/middleware/oidc_auth.go | 3 +- .../pkg/staticroutes/backchannellogout.go | 88 ++++++++----------- 2 files changed, 38 insertions(+), 53 deletions(-) diff --git a/services/proxy/pkg/middleware/oidc_auth.go b/services/proxy/pkg/middleware/oidc_auth.go index 46b4733dc9..48dbd768d5 100644 --- a/services/proxy/pkg/middleware/oidc_auth.go +++ b/services/proxy/pkg/middleware/oidc_auth.go @@ -3,6 +3,7 @@ package middleware import ( "context" "encoding/base64" + "fmt" "net" "net/http" "strings" @@ -128,7 +129,7 @@ func (m *OIDCAuthenticator) getClaims(token string, req *http.Request) (map[stri // create an additional entry mapping subject to session id if sub := aClaims.Subject; sub != "" { err = m.userInfoCache.Write(&store.Record{ - Key: sub, + Key: fmt.Sprintf("%s.%s", sub, sid), Value: []byte(sid), Expiry: time.Until(expiration), }) diff --git a/services/proxy/pkg/staticroutes/backchannellogout.go b/services/proxy/pkg/staticroutes/backchannellogout.go index ad680813ee..eb5574112c 100644 --- a/services/proxy/pkg/staticroutes/backchannellogout.go +++ b/services/proxy/pkg/staticroutes/backchannellogout.go @@ -41,29 +41,17 @@ func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Re return } - var records []*microstore.Record + var cacheKeys map[string]bool if strings.TrimSpace(logoutToken.SessionId) != "" { - records, err = s.UserInfoCache.Read(logoutToken.SessionId) + cacheKeys[logoutToken.SessionId] = true + } else if strings.TrimSpace(logoutToken.Subject) != "" { + records, err := s.UserInfoCache.Read(fmt.Sprintf("%s.*", logoutToken.Subject)) if errors.Is(err, microstore.ErrNotFound) || len(records) == 0 { - render.Status(r, http.StatusOK) - render.JSON(w, r, nil) - return - } - if err != nil { - logger.Error().Err(err).Msg("Error reading userinfo cache") render.Status(r, http.StatusBadRequest) render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()}) return } - } else if strings.TrimSpace(logoutToken.Subject) != "" { - // TODO: enter a mapping table between subject and sessionid when the oidc session is refreshed - records, err = s.UserInfoCache.Read(logoutToken.Subject) - if errors.Is(err, microstore.ErrNotFound) || len(records) == 0 { - render.Status(r, http.StatusOK) - render.JSON(w, r, nil) - return - } if err != nil { logger.Error().Err(err).Msg("Error reading userinfo cache") render.Status(r, http.StatusBadRequest) @@ -71,14 +59,8 @@ func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Re return } for _, record := range records { - // take all previous records retrieved for this subject, and fetch the corresponding sessions - rs, err := s.UserInfoCache.Read(string(record.Value)) - if errors.Is(err, microstore.ErrNotFound) || len(rs) == 0 { - // we do not care about errors here, since we already have entries from the subjects that need to be addressed - continue - } - // we append the additional sessions found through the mapping for later deletion - records = append(records, rs...) + cacheKeys[string(record.Value)] = true + cacheKeys[record.Key] = false } } else { logger.Warn().Msg("invalid logout token") @@ -87,52 +69,54 @@ func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Re return } - for _, record := range records { - err = s.UserInfoCache.Delete(string(record.Value)) + for key, isSID := range cacheKeys { + if isSID { + records, err := s.UserInfoCache.Read(key) + if err != nil && !errors.Is(err, microstore.ErrNotFound) { + logger.Error().Err(err).Msg("Error reading userinfo cache") + render.Status(r, http.StatusBadRequest) + render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()}) + return + } + for _, record := range records { + err = s.UserInfoCache.Delete(string(record.Value)) + if err != nil && !errors.Is(err, microstore.ErrNotFound) { + logger.Error().Err(err).Msg("Error deleting userinfo cache") + render.Status(r, http.StatusBadRequest) + render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()}) + return + } + } + } + + err = s.UserInfoCache.Delete(key) if err != nil && !errors.Is(err, microstore.ErrNotFound) { // Spec requires us to return a 400 BadRequest when the session could not be destroyed - logger.Err(err).Msg("could not delete user info from cache") + logger.Err(err).Msg(fmt.Errorf("could not delete session from cache (%s)", key).Error()) + // We only return on requests that do only attempt to destroy a single session, not multiple render.Status(r, http.StatusBadRequest) render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()}) return } - err := s.publishBackchannelLogoutEvent(r.Context(), record, logoutToken) - if err != nil { - s.Logger.Warn().Err(err).Msg("could not publish backchannel logout event") + if isSID { + err := s.publishBackchannelLogoutEvent(r.Context(), key, logoutToken) + if err != nil { + s.Logger.Warn().Err(err).Msg("could not publish backchannel logout event") + } } logger.Debug().Msg("Deleted userinfo from cache") } - if strings.TrimSpace(logoutToken.SessionId) != "" { - // we can ignore errors when cleaning up the lookup table - err = s.UserInfoCache.Delete(logoutToken.SessionId) - if err != nil { - logger.Debug().Err(err).Msg("Failed to cleanup sessionid lookup entry") - render.Status(r, http.StatusBadRequest) - render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()}) - return - } - } else if strings.TrimSpace(logoutToken.Subject) != "" { - // TODO: do a lookup subject => sessionid and delete both entries - err = s.UserInfoCache.Delete(logoutToken.Subject) - if err != nil { - logger.Debug().Err(err).Msg("Failed to cleanup subject lookup entry") - render.Status(r, http.StatusBadRequest) - render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()}) - return - } - } - render.Status(r, http.StatusOK) render.JSON(w, r, nil) } // publishBackchannelLogoutEvent publishes a backchannel logout event when the callback revived from the identity provider -func (s StaticRouteHandler) publishBackchannelLogoutEvent(ctx context.Context, record *microstore.Record, logoutToken *oidc.LogoutToken) error { +func (s StaticRouteHandler) publishBackchannelLogoutEvent(ctx context.Context, cacheKey string, logoutToken *oidc.LogoutToken) error { if s.EventsPublisher == nil { return fmt.Errorf("the events publisher is not set") } - urecords, err := s.UserInfoCache.Read(string(record.Value)) + urecords, err := s.UserInfoCache.Read(cacheKey) if err != nil { return fmt.Errorf("reading userinfo cache: %w", err) } From 787c5a4e3a3f17be983115c41814861ba51d00cc Mon Sep 17 00:00:00 2001 From: Florian Schade Date: Fri, 13 Feb 2026 20:41:16 +0100 Subject: [PATCH 05/11] enhancement: finalize backchannel logout --- services/proxy/pkg/command/server.go | 2 + .../pkg/config/defaults/defaultconfig.go | 2 + services/proxy/pkg/middleware/oidc_auth.go | 51 ++--- .../pkg/staticroutes/backchannellogout.go | 129 +++++------ .../backchannellogout/backchannellogout.go | 125 ++++++++++ .../backchannellogout_test.go | 213 ++++++++++++++++++ .../proxy/pkg/staticroutes/staticroutes.go | 3 +- 7 files changed, 431 insertions(+), 94 deletions(-) create mode 100644 services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout.go create mode 100644 services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout_test.go diff --git a/services/proxy/pkg/command/server.go b/services/proxy/pkg/command/server.go index 25c1d045dc..9209ec30fb 100644 --- a/services/proxy/pkg/command/server.go +++ b/services/proxy/pkg/command/server.go @@ -10,6 +10,7 @@ import ( gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" "github.com/justinas/alice" + "github.com/opencloud-eu/opencloud/pkg/config/configlog" "github.com/opencloud-eu/opencloud/pkg/generators" "github.com/opencloud-eu/opencloud/pkg/log" @@ -72,6 +73,7 @@ func Server(cfg *config.Config) *cobra.Command { microstore.Nodes(cfg.PreSignedURL.SigningKeys.Nodes...), microstore.Database("proxy"), microstore.Table("signing-keys"), + store.DisablePersistence(cfg.PreSignedURL.SigningKeys.DisablePersistence), store.Authentication(cfg.PreSignedURL.SigningKeys.AuthUsername, cfg.PreSignedURL.SigningKeys.AuthPassword), ) diff --git a/services/proxy/pkg/config/defaults/defaultconfig.go b/services/proxy/pkg/config/defaults/defaultconfig.go index 187e52be6f..69510cb7c5 100644 --- a/services/proxy/pkg/config/defaults/defaultconfig.go +++ b/services/proxy/pkg/config/defaults/defaultconfig.go @@ -45,6 +45,8 @@ func DefaultConfig() *config.Config { AccessTokenVerifyMethod: config.AccessTokenVerificationJWT, SkipUserInfo: false, UserinfoCache: &config.Cache{ + // toDo: + // check if "memory" is the right default or if "nats-js-kv" is a better match Store: "memory", Nodes: []string{"127.0.0.1:9233"}, Database: "cache-userinfo", diff --git a/services/proxy/pkg/middleware/oidc_auth.go b/services/proxy/pkg/middleware/oidc_auth.go index 48dbd768d5..01120053a6 100644 --- a/services/proxy/pkg/middleware/oidc_auth.go +++ b/services/proxy/pkg/middleware/oidc_auth.go @@ -3,19 +3,20 @@ package middleware import ( "context" "encoding/base64" - "fmt" "net" "net/http" "strings" "time" - "github.com/opencloud-eu/opencloud/pkg/log" - "github.com/opencloud-eu/opencloud/pkg/oidc" + "github.com/golang-jwt/jwt/v5" "github.com/pkg/errors" "github.com/vmihailenco/msgpack/v5" - store "go-micro.dev/v4/store" + "go-micro.dev/v4/store" "golang.org/x/crypto/sha3" "golang.org/x/oauth2" + + "github.com/opencloud-eu/opencloud/pkg/log" + "github.com/opencloud-eu/opencloud/pkg/oidc" ) const ( @@ -115,28 +116,26 @@ func (m *OIDCAuthenticator) getClaims(token string, req *http.Request) (map[stri m.Logger.Error().Err(err).Msg("failed to write to userinfo cache") } - if sid := aClaims.SessionID; sid != "" { - // reuse user cache for session id lookup - err = m.userInfoCache.Write(&store.Record{ - Key: sid, - Value: []byte(encodedHash), - Expiry: time.Until(expiration), - }) - if err != nil { - m.Logger.Error().Err(err).Msg("failed to write session lookup cache") - } - - // create an additional entry mapping subject to session id - if sub := aClaims.Subject; sub != "" { - err = m.userInfoCache.Write(&store.Record{ - Key: fmt.Sprintf("%s.%s", sub, sid), - Value: []byte(sid), - Expiry: time.Until(expiration), - }) - if err != nil { - m.Logger.Error().Err(err).Msg("failed to write subject lookup cache") - } - } + subject, sessionId := aClaims.Subject, aClaims.SessionID + // if no session id is present, we can't do a session lookup, + // so we can skip the cache entry for that. + if sessionId == "" { + return + } + + // if the claim has no subject, we can leave it empty, + // it's important to keep the dot in the key to prevent + // sufix and prefix exploration in the cache. + // + // ok: {key: ".sessionId"} + // ok: {key: "subject.sessionId"} + key := strings.Join([]string{subject, sessionId}, ".") + if err := m.userInfoCache.Write(&store.Record{ + Key: key, + Value: []byte(encodedHash), + Expiry: time.Until(expiration), + }); err != nil { + m.Logger.Error().Err(err).Msg("failed to write session lookup cache") } } }() diff --git a/services/proxy/pkg/staticroutes/backchannellogout.go b/services/proxy/pkg/staticroutes/backchannellogout.go index eb5574112c..4dee667607 100644 --- a/services/proxy/pkg/staticroutes/backchannellogout.go +++ b/services/proxy/pkg/staticroutes/backchannellogout.go @@ -7,15 +7,24 @@ import ( "strings" "github.com/go-chi/render" - "github.com/opencloud-eu/opencloud/pkg/oidc" - "github.com/opencloud-eu/reva/v2/pkg/events" - "github.com/opencloud-eu/reva/v2/pkg/utils" "github.com/pkg/errors" "github.com/vmihailenco/msgpack/v5" microstore "go-micro.dev/v4/store" + + bcl "github.com/opencloud-eu/opencloud/services/proxy/pkg/staticroutes/internal/backchannellogout" + "github.com/opencloud-eu/reva/v2/pkg/events" + "github.com/opencloud-eu/reva/v2/pkg/utils" ) -// handle backchannel logout requests as per https://openid.net/specs/openid-connect-backchannel-1_0.html#BCRequest +// BackchannelLogout handles backchannel logout requests from the identity provider and invalidates the related sessions in the cache +// spec: https://openid.net/specs/openid-connect-backchannel-1_0.html#BCRequest +// +// toDo: +// - keyCloak "Sign out all active sessions" fails to log out, no incoming request +// - if the keycloak setting "Backchannel logout session required" is disabled, +// we resolve the session by the subject which can lead to multiple session records, +// we then send a logout event to each connected client and delete our stored record (subject.session & claim). +// but the session still exists in the identity provider. func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Request) { // parse the application/x-www-form-urlencoded POST request logger := s.Logger.SubloggerWithRequestID(r.Context()) @@ -26,13 +35,6 @@ func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Re return } - if r.PostFormValue("logout_token") == "" { - logger.Warn().Msg("logout_token is missing") - render.Status(r, http.StatusBadRequest) - render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: "logout_token is missing"}) - return - } - logoutToken, err := s.OidcClient.VerifyLogoutToken(r.Context(), r.PostFormValue("logout_token")) if err != nil { logger.Warn().Err(err).Msg("VerifyLogoutToken failed") @@ -41,70 +43,63 @@ func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Re return } - var cacheKeys map[string]bool + subject, session := strings.Join(strings.Fields(logoutToken.Subject), ""), strings.Join(strings.Fields(logoutToken.SessionId), "") + if subject == "" && session == "" { + jseErr := jse{Error: "invalid_request", ErrorDescription: "invalid logout token: subject and session id are empty"} + logger.Warn().Msg(jseErr.ErrorDescription) + render.Status(r, http.StatusBadRequest) + render.JSON(w, r, jseErr) + return + } - if strings.TrimSpace(logoutToken.SessionId) != "" { - cacheKeys[logoutToken.SessionId] = true - } else if strings.TrimSpace(logoutToken.Subject) != "" { - records, err := s.UserInfoCache.Read(fmt.Sprintf("%s.*", logoutToken.Subject)) - if errors.Is(err, microstore.ErrNotFound) || len(records) == 0 { - render.Status(r, http.StatusBadRequest) - render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()}) - return - } - if err != nil { - logger.Error().Err(err).Msg("Error reading userinfo cache") - render.Status(r, http.StatusBadRequest) - render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()}) - return - } - for _, record := range records { - cacheKeys[string(record.Value)] = true - cacheKeys[record.Key] = false - } - } else { - logger.Warn().Msg("invalid logout token") + requestSubjectAndSession := bcl.SuSe{Session: session, Subject: subject} + // find out which mode of backchannel logout we are in + // by checking if the session or subject is present in the token + logoutMode := bcl.GetLogoutMode(requestSubjectAndSession) + lookupRecords, err := bcl.GetLogoutRecords(requestSubjectAndSession, logoutMode, s.UserInfoCache) + if errors.Is(err, microstore.ErrNotFound) || len(lookupRecords) == 0 { + render.Status(r, http.StatusOK) + render.JSON(w, r, nil) + return + } + if err != nil { + logger.Error().Err(err).Msg("Error reading userinfo cache") render.Status(r, http.StatusBadRequest) - render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: "invalid logout token"}) + render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()}) return } - for key, isSID := range cacheKeys { - if isSID { - records, err := s.UserInfoCache.Read(key) - if err != nil && !errors.Is(err, microstore.ErrNotFound) { - logger.Error().Err(err).Msg("Error reading userinfo cache") - render.Status(r, http.StatusBadRequest) - render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()}) - return - } - for _, record := range records { - err = s.UserInfoCache.Delete(string(record.Value)) - if err != nil && !errors.Is(err, microstore.ErrNotFound) { - logger.Error().Err(err).Msg("Error deleting userinfo cache") - render.Status(r, http.StatusBadRequest) - render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()}) - return - } - } + for _, record := range lookupRecords { + // the record key is in the format "subject.session" or ".session" + // the record value is the key of the record that contains the claim in its value + key, value := record.Key, string(record.Value) + + subjectSession, ok := bcl.NewSuSe(key) + if !ok { + logger.Warn().Msgf("invalid logout record key: %s", key) + continue } - err = s.UserInfoCache.Delete(key) + err := s.publishBackchannelLogoutEvent(r.Context(), subjectSession.Session, value) + if err != nil { + s.Logger.Warn().Err(err).Msg("could not publish backchannel logout event") + } + + err = s.UserInfoCache.Delete(value) if err != nil && !errors.Is(err, microstore.ErrNotFound) { - // Spec requires us to return a 400 BadRequest when the session could not be destroyed - logger.Err(err).Msg(fmt.Errorf("could not delete session from cache (%s)", key).Error()) - // We only return on requests that do only attempt to destroy a single session, not multiple + // we have to return a 400 BadRequest when we fail to delete the session + // https://openid.net/specs/openid-connect-backchannel-1_0.html#rfc.section.2.8 + logger.Err(err).Msg("could not delete user info from cache") render.Status(r, http.StatusBadRequest) render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()}) return } - if isSID { - err := s.publishBackchannelLogoutEvent(r.Context(), key, logoutToken) - if err != nil { - s.Logger.Warn().Err(err).Msg("could not publish backchannel logout event") - } + + // we can ignore errors when deleting the lookup record + err = s.UserInfoCache.Delete(key) + if err != nil { + logger.Debug().Err(err).Msg("Failed to cleanup sessionid lookup entry") } - logger.Debug().Msg("Deleted userinfo from cache") } render.Status(r, http.StatusOK) @@ -112,20 +107,20 @@ func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Re } // publishBackchannelLogoutEvent publishes a backchannel logout event when the callback revived from the identity provider -func (s StaticRouteHandler) publishBackchannelLogoutEvent(ctx context.Context, cacheKey string, logoutToken *oidc.LogoutToken) error { +func (s *StaticRouteHandler) publishBackchannelLogoutEvent(ctx context.Context, sessionId, claimKey string) error { if s.EventsPublisher == nil { return fmt.Errorf("the events publisher is not set") } - urecords, err := s.UserInfoCache.Read(cacheKey) + claimRecords, err := s.UserInfoCache.Read(claimKey) if err != nil { return fmt.Errorf("reading userinfo cache: %w", err) } - if len(urecords) == 0 { + if len(claimRecords) == 0 { return fmt.Errorf("userinfo not found") } var claims map[string]interface{} - if err = msgpack.Unmarshal(urecords[0].Value, &claims); err != nil { + if err = msgpack.Unmarshal(claimRecords[0].Value, &claims); err != nil { return fmt.Errorf("could not unmarshal userinfo: %w", err) } @@ -141,7 +136,7 @@ func (s StaticRouteHandler) publishBackchannelLogoutEvent(ctx context.Context, c e := events.BackchannelLogout{ Executant: user.GetId(), - SessionId: logoutToken.SessionId, + SessionId: sessionId, Timestamp: utils.TSNow(), } diff --git a/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout.go b/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout.go new file mode 100644 index 0000000000..92f9ab0d59 --- /dev/null +++ b/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout.go @@ -0,0 +1,125 @@ +// package backchannellogout provides functions to classify and lookup +// backchannel logout records from the cache store. + +package backchannellogout + +import ( + "fmt" + "strings" + + "github.com/pkg/errors" + microstore "go-micro.dev/v4/store" +) + +// SuSe 🦎 ;) is a struct that groups the subject and session together +// to prevent mix-ups for ('session, subject' || 'subject, session') +// return values. +type SuSe struct { + Subject string + Session string +} + +// NewSuSe parses the subject and session id from the given key and returns a SuSe struct +func NewSuSe(key string) (SuSe, bool) { + var subject, session string + switch keys := strings.Split(strings.Join(strings.Fields(key), ""), "."); { + case len(keys) == 2 && keys[0] == "" && keys[1] != "": + session = keys[1] + case len(keys) == 2 && keys[0] != "" && keys[1] == "": + subject = keys[0] + case len(keys) == 2 && keys[0] != "" && keys[1] != "": + subject = keys[0] + session = keys[1] + case len(keys) == 1 && keys[0] != "": + session = keys[0] + default: + return SuSe{}, false + } + + return SuSe{Session: session, Subject: subject}, true +} + +// LogoutMode defines the mode of backchannel logout, either by session or by subject +type LogoutMode int + +const ( + // LogoutModeUnknown is used when the logout mode cannot be determined + LogoutModeUnknown LogoutMode = iota + // LogoutModeSession is used when the logout mode is determined by the session id + LogoutModeSession + // LogoutModeSubject is used when the logout mode is determined by the subject + LogoutModeSubject +) + +// GetLogoutMode determines the backchannel logout mode based on the presence of subject and session in the SuSe struct +func GetLogoutMode(suse SuSe) LogoutMode { + switch { + case suse.Session != "": + return LogoutModeSession + case suse.Subject != "": + return LogoutModeSubject + default: + return LogoutModeUnknown + } +} + +// ErrSuspiciousCacheResult is returned when the cache result is suspicious +var ErrSuspiciousCacheResult = errors.New("suspicious cache result") + +// GetLogoutRecords retrieves the records from the user info cache based on the backchannel +// logout mode and the provided SuSe struct. +// it uses a seperator to prevent sufix and prefix exploration in the cache and checks +// if the retrieved records match the requested subject and or session id as well, to prevent false positives. +func GetLogoutRecords(suse SuSe, mode LogoutMode, store microstore.Store) ([]*microstore.Record, error) { + var key string + var opts []microstore.ReadOption + switch mode { + case LogoutModeSession: + // the dot at the beginning prevents sufix exploration in the cache, + // so only keys that end with '*.session' will be returned, but not '*sion'. + key = "." + suse.Session + opts = append(opts, microstore.ReadSuffix()) + case LogoutModeSubject: + // the dot at the end prevents prefix exploration in the cache, + // so only keys that start with 'subject.*' will be returned, but not 'sub*'. + key = suse.Subject + "." + opts = append(opts, microstore.ReadPrefix()) + default: + return nil, fmt.Errorf("%w: cannot determine logout mode", ErrSuspiciousCacheResult) + } + + records, err := store.Read(key, append(opts, microstore.ReadLimit(1000))...) + if err != nil { + return nil, err + } + + if len(records) == 0 { + return nil, microstore.ErrNotFound + } + + if mode == LogoutModeSession && len(records) > 1 { + return nil, fmt.Errorf("%w: multiple session records found", ErrSuspiciousCacheResult) + } + + // double-check if the found records match the requested subject and or session id as well, + // to prevent false positives. + for _, record := range records { + recordSuSe, ok := NewSuSe(record.Key) + if !ok { + return nil, microstore.ErrNotFound + } + + switch { + // in session mode, the session id must match, but the subject can be different + case mode == LogoutModeSession && suse.Session == recordSuSe.Session: + continue + // in subject mode, the subject must match, but the session id can be different + case mode == LogoutModeSubject && suse.Subject == recordSuSe.Subject: + continue + } + + return nil, fmt.Errorf("%w: record key does not match the requested subject or session", ErrSuspiciousCacheResult) + } + + return records, nil +} diff --git a/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout_test.go b/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout_test.go new file mode 100644 index 0000000000..9250a4885b --- /dev/null +++ b/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout_test.go @@ -0,0 +1,213 @@ +package backchannellogout + +import ( + "slices" + "strings" + "testing" + + "github.com/stretchr/testify/require" + "go-micro.dev/v4/store" +) + +func TestNewSuSe(t *testing.T) { + tests := []struct { + name string + key string + wantsuSe SuSe + wantOk bool + }{ + { + name: ".session", + key: ".session", + wantsuSe: SuSe{Session: "session", Subject: ""}, + wantOk: true, + }, + { + name: ".session", + key: ".session", + wantsuSe: SuSe{Session: "session", Subject: ""}, + wantOk: true, + }, + { + name: "session", + key: "session", + wantsuSe: SuSe{Session: "session", Subject: ""}, + wantOk: true, + }, + { + name: "subject.", + key: "subject.", + wantsuSe: SuSe{Session: "", Subject: "subject"}, + wantOk: true, + }, + { + name: "subject.session", + key: "subject.session", + wantsuSe: SuSe{Session: "session", Subject: "subject"}, + wantOk: true, + }, + { + name: "dot", + key: ".", + wantsuSe: SuSe{Session: "", Subject: ""}, + wantOk: false, + }, + { + name: "empty", + key: "", + wantsuSe: SuSe{Session: "", Subject: ""}, + wantOk: false, + }, + { + name: "whitespace . whitespace", + key: " . ", + wantsuSe: SuSe{Session: "", Subject: ""}, + wantOk: false, + }, + { + name: "whitespace subject whitespace . whitespace", + key: " subject . ", + wantsuSe: SuSe{Session: "", Subject: "subject"}, + wantOk: true, + }, + { + name: "whitespace . whitespace session whitespace", + key: " . session ", + wantsuSe: SuSe{Session: "session", Subject: ""}, + wantOk: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + suSe, ok := NewSuSe(tt.key) + require.Equal(t, tt.wantOk, ok) + require.Equal(t, tt.wantsuSe, suSe) + }) + } +} + +func TestGetLogoutMode(t *testing.T) { + tests := []struct { + name string + suSe SuSe + want LogoutMode + }{ + { + name: ".session", + suSe: SuSe{Session: "session", Subject: ""}, + want: LogoutModeSession, + }, + { + name: "subject.session", + suSe: SuSe{Session: "session", Subject: "subject"}, + want: LogoutModeSession, + }, + { + name: "subject.", + suSe: SuSe{Session: "", Subject: "subject"}, + want: LogoutModeSubject, + }, + { + name: "", + suSe: SuSe{Session: "", Subject: ""}, + want: LogoutModeUnknown, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mode := GetLogoutMode(tt.suSe) + require.Equal(t, tt.want, mode) + }) + } +} + +func TestGetLogoutRecords(t *testing.T) { + sessionStore := store.NewMemoryStore() + + recordClaimA := &store.Record{Key: "claim-a", Value: []byte("claim-a-data")} + recordClaimB := &store.Record{Key: "claim-b", Value: []byte("claim-b-data")} + recordClaimC := &store.Record{Key: "claim-c", Value: []byte("claim-c-data")} + recordClaimD := &store.Record{Key: "claim-d", Value: []byte("claim-d-data")} + recordSessionA := &store.Record{Key: "session-a", Value: []byte(recordClaimA.Key)} + recordSessionB := &store.Record{Key: "session-b", Value: []byte(recordClaimB.Key)} + recordSubjectASessionC := &store.Record{Key: "subject-a.session-c", Value: []byte(recordSessionA.Key)} + recordSubjectASessionD := &store.Record{Key: "subject-a.session-d", Value: []byte(recordSessionB.Key)} + + for _, r := range []*store.Record{ + recordClaimA, + recordClaimB, + recordClaimC, + recordClaimD, + recordSessionA, + recordSessionB, + recordSubjectASessionC, + recordSubjectASessionD, + } { + require.NoError(t, sessionStore.Write(r)) + } + + tests := []struct { + name string + suSe SuSe + mode LogoutMode + store store.Store + wantRecords []*store.Record + wantError error + }{ + { + name: "session-c", + suSe: SuSe{Session: "session-c"}, + mode: LogoutModeSession, + store: sessionStore, + wantRecords: []*store.Record{recordSubjectASessionC}, + }, + { + name: "ession-c", + suSe: SuSe{Session: "ession-c"}, + mode: LogoutModeSession, + store: sessionStore, + wantError: store.ErrNotFound, + wantRecords: []*store.Record{}, + }, + { + name: "subject-a", + suSe: SuSe{Subject: "subject-a"}, + mode: LogoutModeSubject, + store: sessionStore, + wantRecords: []*store.Record{recordSubjectASessionC, recordSubjectASessionD}, + }, + { + name: "subject-", + suSe: SuSe{Subject: "subject-"}, + mode: LogoutModeSubject, + store: sessionStore, + wantError: store.ErrNotFound, + wantRecords: []*store.Record{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + records, err := GetLogoutRecords(tt.suSe, tt.mode, tt.store) + require.ErrorIs(t, err, tt.wantError) + require.Len(t, records, len(tt.wantRecords)) + + sortRecords := func(r []*store.Record) []*store.Record { + slices.SortFunc(r, func(a, b *store.Record) int { + return strings.Compare(a.Key, b.Key) + }) + + return r + } + + records = sortRecords(records) + for i, wantRecords := range sortRecords(tt.wantRecords) { + require.True(t, len(records) >= i+1) + require.Equal(t, wantRecords.Key, records[i].Key) + require.Equal(t, wantRecords.Value, records[i].Value) + } + }) + } +} diff --git a/services/proxy/pkg/staticroutes/staticroutes.go b/services/proxy/pkg/staticroutes/staticroutes.go index f2f76740f1..61d4f0ca5b 100644 --- a/services/proxy/pkg/staticroutes/staticroutes.go +++ b/services/proxy/pkg/staticroutes/staticroutes.go @@ -4,12 +4,13 @@ import ( "net/http" "github.com/go-chi/chi/v5" + microstore "go-micro.dev/v4/store" + "github.com/opencloud-eu/opencloud/pkg/log" "github.com/opencloud-eu/opencloud/pkg/oidc" "github.com/opencloud-eu/opencloud/services/proxy/pkg/config" "github.com/opencloud-eu/opencloud/services/proxy/pkg/user/backend" "github.com/opencloud-eu/reva/v2/pkg/events" - microstore "go-micro.dev/v4/store" ) // StaticRouteHandler defines a Route Handler for static routes From f0acb9d5a3409a4d653782d11b3250f829ef9d6d Mon Sep 17 00:00:00 2001 From: Florian Schade Date: Mon, 16 Feb 2026 13:44:01 +0100 Subject: [PATCH 06/11] enhancement: document idp side-effects --- .../pkg/staticroutes/backchannellogout.go | 27 +++++++++++++------ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/services/proxy/pkg/staticroutes/backchannellogout.go b/services/proxy/pkg/staticroutes/backchannellogout.go index 4dee667607..c213e090a1 100644 --- a/services/proxy/pkg/staticroutes/backchannellogout.go +++ b/services/proxy/pkg/staticroutes/backchannellogout.go @@ -16,17 +16,28 @@ import ( "github.com/opencloud-eu/reva/v2/pkg/utils" ) -// BackchannelLogout handles backchannel logout requests from the identity provider and invalidates the related sessions in the cache +// backchannelLogout handles backchannel logout requests from the identity provider and invalidates the related sessions in the cache // spec: https://openid.net/specs/openid-connect-backchannel-1_0.html#BCRequest // -// toDo: -// - keyCloak "Sign out all active sessions" fails to log out, no incoming request -// - if the keycloak setting "Backchannel logout session required" is disabled, -// we resolve the session by the subject which can lead to multiple session records, -// we then send a logout event to each connected client and delete our stored record (subject.session & claim). -// but the session still exists in the identity provider. +// known side effects of backchannel logout in keycloak: +// +// - keyCloak "Sign out all active sessions" does not send a backchannel logout request, +// as the devs mention, this may lead to thousands of backchannel logout requests, +// therefore, they recommend a short token lifetime. +// https://github.com/keycloak/keycloak/issues/27342#issuecomment-2408461913 +// +// - keyCloak user self-service portal, "Sign out all devices" may not send a backchannel +// logout request for each session, it's not mentionex explicitly, +// but maybe the reason for that is the same as for "Sign out all active sessions" +// to prevent a flood of backchannel logout requests. +// +// - if the keycloak setting "Backchannel logout session required" is disabled (or the token has no session id), +// we resolve the session by the subject which can lead to multiple session records (subject.*), +// we then send a logout event (sse) to each connected client and delete our stored cache record (subject.session & claim). +// all sessions besides the one that triggered the backchannel logout continue to exist in the identity provider, +// so the user will not be fully logged out until all sessions are logged out or expired. +// this leads to the situation that web renders the logout view even if the instance is not fully logged out yet. func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Request) { - // parse the application/x-www-form-urlencoded POST request logger := s.Logger.SubloggerWithRequestID(r.Context()) if err := r.ParseForm(); err != nil { logger.Warn().Err(err).Msg("ParseForm failed") From dfa10d1d9f2d9a54048d958fe0117ac29c58e2a9 Mon Sep 17 00:00:00 2001 From: Florian Schade Date: Mon, 16 Feb 2026 14:02:57 +0100 Subject: [PATCH 07/11] chore: cleanup backchannel logout pr for review --- services/proxy/pkg/config/defaults/defaultconfig.go | 2 -- services/proxy/pkg/middleware/oidc_auth.go | 2 +- services/proxy/pkg/staticroutes/staticroutes.go | 3 +-- 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/services/proxy/pkg/config/defaults/defaultconfig.go b/services/proxy/pkg/config/defaults/defaultconfig.go index 69510cb7c5..187e52be6f 100644 --- a/services/proxy/pkg/config/defaults/defaultconfig.go +++ b/services/proxy/pkg/config/defaults/defaultconfig.go @@ -45,8 +45,6 @@ func DefaultConfig() *config.Config { AccessTokenVerifyMethod: config.AccessTokenVerificationJWT, SkipUserInfo: false, UserinfoCache: &config.Cache{ - // toDo: - // check if "memory" is the right default or if "nats-js-kv" is a better match Store: "memory", Nodes: []string{"127.0.0.1:9233"}, Database: "cache-userinfo", diff --git a/services/proxy/pkg/middleware/oidc_auth.go b/services/proxy/pkg/middleware/oidc_auth.go index 01120053a6..075f088b11 100644 --- a/services/proxy/pkg/middleware/oidc_auth.go +++ b/services/proxy/pkg/middleware/oidc_auth.go @@ -116,7 +116,7 @@ func (m *OIDCAuthenticator) getClaims(token string, req *http.Request) (map[stri m.Logger.Error().Err(err).Msg("failed to write to userinfo cache") } - subject, sessionId := aClaims.Subject, aClaims.SessionID + subject, sessionId := strings.Join(strings.Fields(aClaims.Subject), ""), strings.Join(strings.Fields(aClaims.SessionID), "") // if no session id is present, we can't do a session lookup, // so we can skip the cache entry for that. if sessionId == "" { diff --git a/services/proxy/pkg/staticroutes/staticroutes.go b/services/proxy/pkg/staticroutes/staticroutes.go index 61d4f0ca5b..f2f76740f1 100644 --- a/services/proxy/pkg/staticroutes/staticroutes.go +++ b/services/proxy/pkg/staticroutes/staticroutes.go @@ -4,13 +4,12 @@ import ( "net/http" "github.com/go-chi/chi/v5" - microstore "go-micro.dev/v4/store" - "github.com/opencloud-eu/opencloud/pkg/log" "github.com/opencloud-eu/opencloud/pkg/oidc" "github.com/opencloud-eu/opencloud/services/proxy/pkg/config" "github.com/opencloud-eu/opencloud/services/proxy/pkg/user/backend" "github.com/opencloud-eu/reva/v2/pkg/events" + microstore "go-micro.dev/v4/store" ) // StaticRouteHandler defines a Route Handler for static routes From 74b8a3f4096b15a093654a6bea5b725825110f3c Mon Sep 17 00:00:00 2001 From: Florian Schade Date: Mon, 16 Feb 2026 23:20:39 +0100 Subject: [PATCH 08/11] test: add more backchannellogout tests --- services/proxy/.mockery.yaml | 5 + .../pkg/staticroutes/backchannellogout.go | 15 +- .../backchannellogout/backchannellogout.go | 16 +- .../backchannellogout_test.go | 222 +++++--- .../internal/backchannellogout/mocks/store.go | 509 ++++++++++++++++++ 5 files changed, 693 insertions(+), 74 deletions(-) create mode 100644 services/proxy/pkg/staticroutes/internal/backchannellogout/mocks/store.go diff --git a/services/proxy/.mockery.yaml b/services/proxy/.mockery.yaml index a490457301..d3ae0a3817 100644 --- a/services/proxy/.mockery.yaml +++ b/services/proxy/.mockery.yaml @@ -12,3 +12,8 @@ packages: github.com/opencloud-eu/opencloud/services/proxy/pkg/userroles: interfaces: UserRoleAssigner: {} + go-micro.dev/v4/store: + config: + dir: pkg/staticroutes/internal/backchannellogout/mocks + interfaces: + Store: {} diff --git a/services/proxy/pkg/staticroutes/backchannellogout.go b/services/proxy/pkg/staticroutes/backchannellogout.go index c213e090a1..0d44e7cd1c 100644 --- a/services/proxy/pkg/staticroutes/backchannellogout.go +++ b/services/proxy/pkg/staticroutes/backchannellogout.go @@ -37,6 +37,9 @@ import ( // all sessions besides the one that triggered the backchannel logout continue to exist in the identity provider, // so the user will not be fully logged out until all sessions are logged out or expired. // this leads to the situation that web renders the logout view even if the instance is not fully logged out yet. +// +// toDo: +// - check logs and errors to not contain any sensitive information like session ids or user ids (keys too) func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Request) { logger := s.Logger.SubloggerWithRequestID(r.Context()) if err := r.ParseForm(); err != nil { @@ -85,14 +88,14 @@ func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Re // the record value is the key of the record that contains the claim in its value key, value := record.Key, string(record.Value) - subjectSession, ok := bcl.NewSuSe(key) - if !ok { - logger.Warn().Msgf("invalid logout record key: %s", key) + subjectSession, err := bcl.NewSuSe(key) + if err != nil { + // never leak any key-related information + logger.Warn().Err(err).Msgf("invalid logout record key: %s", "XXX") continue } - err := s.publishBackchannelLogoutEvent(r.Context(), subjectSession.Session, value) - if err != nil { + if err := s.publishBackchannelLogoutEvent(r.Context(), subjectSession.Session, value); err != nil { s.Logger.Warn().Err(err).Msg("could not publish backchannel logout event") } @@ -109,7 +112,7 @@ func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Re // we can ignore errors when deleting the lookup record err = s.UserInfoCache.Delete(key) if err != nil { - logger.Debug().Err(err).Msg("Failed to cleanup sessionid lookup entry") + logger.Debug().Err(err).Msg("Failed to cleanup sessionId lookup entry") } } diff --git a/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout.go b/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout.go index 92f9ab0d59..306febd0ca 100644 --- a/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout.go +++ b/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout.go @@ -19,8 +19,11 @@ type SuSe struct { Session string } +// ErrInvalidSessionOrSubject is returned when the provided key does not match the expected key format +var ErrInvalidSessionOrSubject = errors.New("invalid session or subject") + // NewSuSe parses the subject and session id from the given key and returns a SuSe struct -func NewSuSe(key string) (SuSe, bool) { +func NewSuSe(key string) (SuSe, error) { var subject, session string switch keys := strings.Split(strings.Join(strings.Fields(key), ""), "."); { case len(keys) == 2 && keys[0] == "" && keys[1] != "": @@ -33,10 +36,10 @@ func NewSuSe(key string) (SuSe, bool) { case len(keys) == 1 && keys[0] != "": session = keys[0] default: - return SuSe{}, false + return SuSe{}, ErrInvalidSessionOrSubject } - return SuSe{Session: session, Subject: subject}, true + return SuSe{Session: session, Subject: subject}, nil } // LogoutMode defines the mode of backchannel logout, either by session or by subject @@ -104,9 +107,10 @@ func GetLogoutRecords(suse SuSe, mode LogoutMode, store microstore.Store) ([]*mi // double-check if the found records match the requested subject and or session id as well, // to prevent false positives. for _, record := range records { - recordSuSe, ok := NewSuSe(record.Key) - if !ok { - return nil, microstore.ErrNotFound + recordSuSe, err := NewSuSe(record.Key) + if err != nil { + // never leak any key-related information + return nil, fmt.Errorf("%w %w: failed to parse logout record key: %s", err, ErrSuspiciousCacheResult, "XXX") } switch { diff --git a/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout_test.go b/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout_test.go index 9250a4885b..238323c728 100644 --- a/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout_test.go +++ b/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout_test.go @@ -5,84 +5,80 @@ import ( "strings" "testing" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "go-micro.dev/v4/store" + + "github.com/opencloud-eu/opencloud/services/proxy/pkg/staticroutes/internal/backchannellogout/mocks" ) func TestNewSuSe(t *testing.T) { tests := []struct { name string key string - wantsuSe SuSe - wantOk bool + wantSuSe SuSe + wantErr error }{ { - name: ".session", + name: "key variation: '.session'", key: ".session", - wantsuSe: SuSe{Session: "session", Subject: ""}, - wantOk: true, + wantSuSe: SuSe{Session: "session", Subject: ""}, }, { - name: ".session", + name: "key variation: '.session'", key: ".session", - wantsuSe: SuSe{Session: "session", Subject: ""}, - wantOk: true, + wantSuSe: SuSe{Session: "session", Subject: ""}, }, { - name: "session", + name: "key variation: 'session'", key: "session", - wantsuSe: SuSe{Session: "session", Subject: ""}, - wantOk: true, + wantSuSe: SuSe{Session: "session", Subject: ""}, }, { - name: "subject.", + name: "key variation: 'subject.'", key: "subject.", - wantsuSe: SuSe{Session: "", Subject: "subject"}, - wantOk: true, + wantSuSe: SuSe{Session: "", Subject: "subject"}, }, { - name: "subject.session", + name: "key variation: 'subject.session'", key: "subject.session", - wantsuSe: SuSe{Session: "session", Subject: "subject"}, - wantOk: true, + wantSuSe: SuSe{Session: "session", Subject: "subject"}, }, { - name: "dot", + name: "key variation: 'dot'", key: ".", - wantsuSe: SuSe{Session: "", Subject: ""}, - wantOk: false, + wantSuSe: SuSe{Session: "", Subject: ""}, + wantErr: ErrInvalidSessionOrSubject, }, { - name: "empty", + name: "key variation: 'empty'", key: "", - wantsuSe: SuSe{Session: "", Subject: ""}, - wantOk: false, + wantSuSe: SuSe{Session: "", Subject: ""}, + wantErr: ErrInvalidSessionOrSubject, }, { - name: "whitespace . whitespace", + name: "key variation: 'whitespace . whitespace'", key: " . ", - wantsuSe: SuSe{Session: "", Subject: ""}, - wantOk: false, + wantSuSe: SuSe{Session: "", Subject: ""}, + wantErr: ErrInvalidSessionOrSubject, }, { - name: "whitespace subject whitespace . whitespace", + name: "key variation: 'whitespace subject whitespace . whitespace'", key: " subject . ", - wantsuSe: SuSe{Session: "", Subject: "subject"}, - wantOk: true, + wantSuSe: SuSe{Session: "", Subject: "subject"}, }, { - name: "whitespace . whitespace session whitespace", + name: "key variation: 'whitespace . whitespace session whitespace'", key: " . session ", - wantsuSe: SuSe{Session: "session", Subject: ""}, - wantOk: true, + wantSuSe: SuSe{Session: "session", Subject: ""}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { suSe, ok := NewSuSe(tt.key) - require.Equal(t, tt.wantOk, ok) - require.Equal(t, tt.wantsuSe, suSe) + require.ErrorIs(t, tt.wantErr, ok) + require.Equal(t, tt.wantSuSe, suSe) }) } } @@ -94,22 +90,22 @@ func TestGetLogoutMode(t *testing.T) { want LogoutMode }{ { - name: ".session", + name: "key variation: '.session'", suSe: SuSe{Session: "session", Subject: ""}, want: LogoutModeSession, }, { - name: "subject.session", + name: "key variation: 'subject.session'", suSe: SuSe{Session: "session", Subject: "subject"}, want: LogoutModeSession, }, { - name: "subject.", + name: "key variation: 'subject.'", suSe: SuSe{Session: "", Subject: "subject"}, want: LogoutModeSubject, }, { - name: "", + name: "key variation: 'empty'", suSe: SuSe{Session: "", Subject: ""}, want: LogoutModeUnknown, }, @@ -130,8 +126,8 @@ func TestGetLogoutRecords(t *testing.T) { recordClaimB := &store.Record{Key: "claim-b", Value: []byte("claim-b-data")} recordClaimC := &store.Record{Key: "claim-c", Value: []byte("claim-c-data")} recordClaimD := &store.Record{Key: "claim-d", Value: []byte("claim-d-data")} - recordSessionA := &store.Record{Key: "session-a", Value: []byte(recordClaimA.Key)} - recordSessionB := &store.Record{Key: "session-b", Value: []byte(recordClaimB.Key)} + recordSessionA := &store.Record{Key: ".session-a", Value: []byte(recordClaimA.Key)} + recordSessionB := &store.Record{Key: ".session-b", Value: []byte(recordClaimB.Key)} recordSubjectASessionC := &store.Record{Key: "subject-a.session-c", Value: []byte(recordSessionA.Key)} recordSubjectASessionD := &store.Record{Key: "subject-a.session-d", Value: []byte(recordSessionB.Key)} @@ -152,46 +148,148 @@ func TestGetLogoutRecords(t *testing.T) { name string suSe SuSe mode LogoutMode - store store.Store + store func(t *testing.T) store.Store wantRecords []*store.Record - wantError error + wantErrs []error }{ { - name: "session-c", - suSe: SuSe{Session: "session-c"}, - mode: LogoutModeSession, - store: sessionStore, + name: "fails if mode is unknown", + suSe: SuSe{Session: "session-a"}, + mode: LogoutModeUnknown, + store: func(t *testing.T) store.Store { + return sessionStore + }, + wantRecords: []*store.Record{}, + wantErrs: []error{ErrSuspiciousCacheResult}, + }, + { + name: "fails if mode is any random int", + suSe: SuSe{Session: "session-a"}, + mode: 999, + store: func(t *testing.T) store.Store { + return sessionStore + }, + wantRecords: []*store.Record{}, + wantErrs: []error{ErrSuspiciousCacheResult}}, + { + name: "fails if multiple session records are found", + suSe: SuSe{Session: "session-a"}, + mode: LogoutModeSession, + store: func(t *testing.T) store.Store { + s := mocks.NewStore(t) + s.EXPECT().Read(mock.Anything, mock.Anything).Return([]*store.Record{ + recordSessionA, + recordSessionB, + }, nil) + return s + }, + wantRecords: []*store.Record{}, + wantErrs: []error{ErrSuspiciousCacheResult}}, + { + name: "fails if the record key is not ok", + suSe: SuSe{Session: "session-a"}, + mode: LogoutModeSession, + store: func(t *testing.T) store.Store { + s := mocks.NewStore(t) + s.EXPECT().Read(mock.Anything, mock.Anything).Return([]*store.Record{ + &store.Record{Key: "invalid.record.key"}, + }, nil) + return s + }, + wantRecords: []*store.Record{}, + wantErrs: []error{ErrInvalidSessionOrSubject, ErrSuspiciousCacheResult}, + }, + { + name: "fails if the session does not match the retrieved record", + suSe: SuSe{Session: "session-a"}, + mode: LogoutModeSession, + store: func(t *testing.T) store.Store { + s := mocks.NewStore(t) + s.EXPECT().Read(mock.Anything, mock.Anything).Return([]*store.Record{ + recordSessionB, + }, nil) + return s + }, + wantRecords: []*store.Record{}, + wantErrs: []error{ErrSuspiciousCacheResult}}, + { + name: "fails if the subject does not match the retrieved record", + suSe: SuSe{Subject: "subject-a"}, + mode: LogoutModeSubject, + store: func(t *testing.T) store.Store { + s := mocks.NewStore(t) + s.EXPECT().Read(mock.Anything, mock.Anything).Return([]*store.Record{ + recordSessionB, + }, nil) + return s + }, + wantRecords: []*store.Record{}, + wantErrs: []error{ErrSuspiciousCacheResult}}, + // key variation tests + { + name: "key variation: 'session-a'", + suSe: SuSe{Session: "session-a"}, + mode: LogoutModeSession, + store: func(*testing.T) store.Store { + return sessionStore + }, + wantRecords: []*store.Record{recordSessionA}, + }, + { + name: "key variation: 'session-b'", + suSe: SuSe{Session: "session-b"}, + mode: LogoutModeSession, + store: func(*testing.T) store.Store { + return sessionStore + }, + wantRecords: []*store.Record{recordSessionB}, + }, + { + name: "key variation: 'session-c'", + suSe: SuSe{Session: "session-c"}, + mode: LogoutModeSession, + store: func(*testing.T) store.Store { + return sessionStore + }, wantRecords: []*store.Record{recordSubjectASessionC}, }, { - name: "ession-c", - suSe: SuSe{Session: "ession-c"}, - mode: LogoutModeSession, - store: sessionStore, - wantError: store.ErrNotFound, + name: "key variation: 'ession-c'", + suSe: SuSe{Session: "ession-c"}, + mode: LogoutModeSession, + store: func(*testing.T) store.Store { + return sessionStore + }, wantRecords: []*store.Record{}, + wantErrs: []error{store.ErrNotFound}, }, { - name: "subject-a", - suSe: SuSe{Subject: "subject-a"}, - mode: LogoutModeSubject, - store: sessionStore, + name: "key variation: 'subject-a'", + suSe: SuSe{Subject: "subject-a"}, + mode: LogoutModeSubject, + store: func(*testing.T) store.Store { + return sessionStore + }, wantRecords: []*store.Record{recordSubjectASessionC, recordSubjectASessionD}, }, { - name: "subject-", - suSe: SuSe{Subject: "subject-"}, - mode: LogoutModeSubject, - store: sessionStore, - wantError: store.ErrNotFound, + name: "key variation: 'subject-'", + suSe: SuSe{Subject: "subject-"}, + mode: LogoutModeSubject, + store: func(*testing.T) store.Store { + return sessionStore + }, wantRecords: []*store.Record{}, + wantErrs: []error{store.ErrNotFound}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - records, err := GetLogoutRecords(tt.suSe, tt.mode, tt.store) - require.ErrorIs(t, err, tt.wantError) + records, err := GetLogoutRecords(tt.suSe, tt.mode, tt.store(t)) + for _, wantErr := range tt.wantErrs { + require.ErrorIs(t, err, wantErr) + } require.Len(t, records, len(tt.wantRecords)) sortRecords := func(r []*store.Record) []*store.Record { diff --git a/services/proxy/pkg/staticroutes/internal/backchannellogout/mocks/store.go b/services/proxy/pkg/staticroutes/internal/backchannellogout/mocks/store.go new file mode 100644 index 0000000000..359ea9cc2b --- /dev/null +++ b/services/proxy/pkg/staticroutes/internal/backchannellogout/mocks/store.go @@ -0,0 +1,509 @@ +// Code generated by mockery; DO NOT EDIT. +// github.com/vektra/mockery +// template: testify + +package mocks + +import ( + mock "github.com/stretchr/testify/mock" + "go-micro.dev/v4/store" +) + +// NewStore creates a new instance of Store. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewStore(t interface { + mock.TestingT + Cleanup(func()) +}) *Store { + mock := &Store{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} + +// Store is an autogenerated mock type for the Store type +type Store struct { + mock.Mock +} + +type Store_Expecter struct { + mock *mock.Mock +} + +func (_m *Store) EXPECT() *Store_Expecter { + return &Store_Expecter{mock: &_m.Mock} +} + +// Close provides a mock function for the type Store +func (_mock *Store) Close() error { + ret := _mock.Called() + + if len(ret) == 0 { + panic("no return value specified for Close") + } + + var r0 error + if returnFunc, ok := ret.Get(0).(func() error); ok { + r0 = returnFunc() + } else { + r0 = ret.Error(0) + } + return r0 +} + +// Store_Close_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Close' +type Store_Close_Call struct { + *mock.Call +} + +// Close is a helper method to define mock.On call +func (_e *Store_Expecter) Close() *Store_Close_Call { + return &Store_Close_Call{Call: _e.mock.On("Close")} +} + +func (_c *Store_Close_Call) Run(run func()) *Store_Close_Call { + _c.Call.Run(func(args mock.Arguments) { + run() + }) + return _c +} + +func (_c *Store_Close_Call) Return(err error) *Store_Close_Call { + _c.Call.Return(err) + return _c +} + +func (_c *Store_Close_Call) RunAndReturn(run func() error) *Store_Close_Call { + _c.Call.Return(run) + return _c +} + +// Delete provides a mock function for the type Store +func (_mock *Store) Delete(key string, opts ...store.DeleteOption) error { + var tmpRet mock.Arguments + if len(opts) > 0 { + tmpRet = _mock.Called(key, opts) + } else { + tmpRet = _mock.Called(key) + } + ret := tmpRet + + if len(ret) == 0 { + panic("no return value specified for Delete") + } + + var r0 error + if returnFunc, ok := ret.Get(0).(func(string, ...store.DeleteOption) error); ok { + r0 = returnFunc(key, opts...) + } else { + r0 = ret.Error(0) + } + return r0 +} + +// Store_Delete_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Delete' +type Store_Delete_Call struct { + *mock.Call +} + +// Delete is a helper method to define mock.On call +// - key string +// - opts ...store.DeleteOption +func (_e *Store_Expecter) Delete(key interface{}, opts ...interface{}) *Store_Delete_Call { + return &Store_Delete_Call{Call: _e.mock.On("Delete", + append([]interface{}{key}, opts...)...)} +} + +func (_c *Store_Delete_Call) Run(run func(key string, opts ...store.DeleteOption)) *Store_Delete_Call { + _c.Call.Run(func(args mock.Arguments) { + var arg0 string + if args[0] != nil { + arg0 = args[0].(string) + } + var arg1 []store.DeleteOption + var variadicArgs []store.DeleteOption + if len(args) > 1 { + variadicArgs = args[1].([]store.DeleteOption) + } + arg1 = variadicArgs + run( + arg0, + arg1..., + ) + }) + return _c +} + +func (_c *Store_Delete_Call) Return(err error) *Store_Delete_Call { + _c.Call.Return(err) + return _c +} + +func (_c *Store_Delete_Call) RunAndReturn(run func(key string, opts ...store.DeleteOption) error) *Store_Delete_Call { + _c.Call.Return(run) + return _c +} + +// Init provides a mock function for the type Store +func (_mock *Store) Init(options ...store.Option) error { + var tmpRet mock.Arguments + if len(options) > 0 { + tmpRet = _mock.Called(options) + } else { + tmpRet = _mock.Called() + } + ret := tmpRet + + if len(ret) == 0 { + panic("no return value specified for Init") + } + + var r0 error + if returnFunc, ok := ret.Get(0).(func(...store.Option) error); ok { + r0 = returnFunc(options...) + } else { + r0 = ret.Error(0) + } + return r0 +} + +// Store_Init_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Init' +type Store_Init_Call struct { + *mock.Call +} + +// Init is a helper method to define mock.On call +// - options ...store.Option +func (_e *Store_Expecter) Init(options ...interface{}) *Store_Init_Call { + return &Store_Init_Call{Call: _e.mock.On("Init", + append([]interface{}{}, options...)...)} +} + +func (_c *Store_Init_Call) Run(run func(options ...store.Option)) *Store_Init_Call { + _c.Call.Run(func(args mock.Arguments) { + var arg0 []store.Option + var variadicArgs []store.Option + if len(args) > 0 { + variadicArgs = args[0].([]store.Option) + } + arg0 = variadicArgs + run( + arg0..., + ) + }) + return _c +} + +func (_c *Store_Init_Call) Return(err error) *Store_Init_Call { + _c.Call.Return(err) + return _c +} + +func (_c *Store_Init_Call) RunAndReturn(run func(options ...store.Option) error) *Store_Init_Call { + _c.Call.Return(run) + return _c +} + +// List provides a mock function for the type Store +func (_mock *Store) List(opts ...store.ListOption) ([]string, error) { + var tmpRet mock.Arguments + if len(opts) > 0 { + tmpRet = _mock.Called(opts) + } else { + tmpRet = _mock.Called() + } + ret := tmpRet + + if len(ret) == 0 { + panic("no return value specified for List") + } + + var r0 []string + var r1 error + if returnFunc, ok := ret.Get(0).(func(...store.ListOption) ([]string, error)); ok { + return returnFunc(opts...) + } + if returnFunc, ok := ret.Get(0).(func(...store.ListOption) []string); ok { + r0 = returnFunc(opts...) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]string) + } + } + if returnFunc, ok := ret.Get(1).(func(...store.ListOption) error); ok { + r1 = returnFunc(opts...) + } else { + r1 = ret.Error(1) + } + return r0, r1 +} + +// Store_List_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'List' +type Store_List_Call struct { + *mock.Call +} + +// List is a helper method to define mock.On call +// - opts ...store.ListOption +func (_e *Store_Expecter) List(opts ...interface{}) *Store_List_Call { + return &Store_List_Call{Call: _e.mock.On("List", + append([]interface{}{}, opts...)...)} +} + +func (_c *Store_List_Call) Run(run func(opts ...store.ListOption)) *Store_List_Call { + _c.Call.Run(func(args mock.Arguments) { + var arg0 []store.ListOption + var variadicArgs []store.ListOption + if len(args) > 0 { + variadicArgs = args[0].([]store.ListOption) + } + arg0 = variadicArgs + run( + arg0..., + ) + }) + return _c +} + +func (_c *Store_List_Call) Return(strings []string, err error) *Store_List_Call { + _c.Call.Return(strings, err) + return _c +} + +func (_c *Store_List_Call) RunAndReturn(run func(opts ...store.ListOption) ([]string, error)) *Store_List_Call { + _c.Call.Return(run) + return _c +} + +// Options provides a mock function for the type Store +func (_mock *Store) Options() store.Options { + ret := _mock.Called() + + if len(ret) == 0 { + panic("no return value specified for Options") + } + + var r0 store.Options + if returnFunc, ok := ret.Get(0).(func() store.Options); ok { + r0 = returnFunc() + } else { + r0 = ret.Get(0).(store.Options) + } + return r0 +} + +// Store_Options_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Options' +type Store_Options_Call struct { + *mock.Call +} + +// Options is a helper method to define mock.On call +func (_e *Store_Expecter) Options() *Store_Options_Call { + return &Store_Options_Call{Call: _e.mock.On("Options")} +} + +func (_c *Store_Options_Call) Run(run func()) *Store_Options_Call { + _c.Call.Run(func(args mock.Arguments) { + run() + }) + return _c +} + +func (_c *Store_Options_Call) Return(options store.Options) *Store_Options_Call { + _c.Call.Return(options) + return _c +} + +func (_c *Store_Options_Call) RunAndReturn(run func() store.Options) *Store_Options_Call { + _c.Call.Return(run) + return _c +} + +// Read provides a mock function for the type Store +func (_mock *Store) Read(key string, opts ...store.ReadOption) ([]*store.Record, error) { + var tmpRet mock.Arguments + if len(opts) > 0 { + tmpRet = _mock.Called(key, opts) + } else { + tmpRet = _mock.Called(key) + } + ret := tmpRet + + if len(ret) == 0 { + panic("no return value specified for Read") + } + + var r0 []*store.Record + var r1 error + if returnFunc, ok := ret.Get(0).(func(string, ...store.ReadOption) ([]*store.Record, error)); ok { + return returnFunc(key, opts...) + } + if returnFunc, ok := ret.Get(0).(func(string, ...store.ReadOption) []*store.Record); ok { + r0 = returnFunc(key, opts...) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*store.Record) + } + } + if returnFunc, ok := ret.Get(1).(func(string, ...store.ReadOption) error); ok { + r1 = returnFunc(key, opts...) + } else { + r1 = ret.Error(1) + } + return r0, r1 +} + +// Store_Read_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Read' +type Store_Read_Call struct { + *mock.Call +} + +// Read is a helper method to define mock.On call +// - key string +// - opts ...store.ReadOption +func (_e *Store_Expecter) Read(key interface{}, opts ...interface{}) *Store_Read_Call { + return &Store_Read_Call{Call: _e.mock.On("Read", + append([]interface{}{key}, opts...)...)} +} + +func (_c *Store_Read_Call) Run(run func(key string, opts ...store.ReadOption)) *Store_Read_Call { + _c.Call.Run(func(args mock.Arguments) { + var arg0 string + if args[0] != nil { + arg0 = args[0].(string) + } + var arg1 []store.ReadOption + var variadicArgs []store.ReadOption + if len(args) > 1 { + variadicArgs = args[1].([]store.ReadOption) + } + arg1 = variadicArgs + run( + arg0, + arg1..., + ) + }) + return _c +} + +func (_c *Store_Read_Call) Return(records []*store.Record, err error) *Store_Read_Call { + _c.Call.Return(records, err) + return _c +} + +func (_c *Store_Read_Call) RunAndReturn(run func(key string, opts ...store.ReadOption) ([]*store.Record, error)) *Store_Read_Call { + _c.Call.Return(run) + return _c +} + +// String provides a mock function for the type Store +func (_mock *Store) String() string { + ret := _mock.Called() + + if len(ret) == 0 { + panic("no return value specified for String") + } + + var r0 string + if returnFunc, ok := ret.Get(0).(func() string); ok { + r0 = returnFunc() + } else { + r0 = ret.Get(0).(string) + } + return r0 +} + +// Store_String_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'String' +type Store_String_Call struct { + *mock.Call +} + +// String is a helper method to define mock.On call +func (_e *Store_Expecter) String() *Store_String_Call { + return &Store_String_Call{Call: _e.mock.On("String")} +} + +func (_c *Store_String_Call) Run(run func()) *Store_String_Call { + _c.Call.Run(func(args mock.Arguments) { + run() + }) + return _c +} + +func (_c *Store_String_Call) Return(s string) *Store_String_Call { + _c.Call.Return(s) + return _c +} + +func (_c *Store_String_Call) RunAndReturn(run func() string) *Store_String_Call { + _c.Call.Return(run) + return _c +} + +// Write provides a mock function for the type Store +func (_mock *Store) Write(r *store.Record, opts ...store.WriteOption) error { + var tmpRet mock.Arguments + if len(opts) > 0 { + tmpRet = _mock.Called(r, opts) + } else { + tmpRet = _mock.Called(r) + } + ret := tmpRet + + if len(ret) == 0 { + panic("no return value specified for Write") + } + + var r0 error + if returnFunc, ok := ret.Get(0).(func(*store.Record, ...store.WriteOption) error); ok { + r0 = returnFunc(r, opts...) + } else { + r0 = ret.Error(0) + } + return r0 +} + +// Store_Write_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Write' +type Store_Write_Call struct { + *mock.Call +} + +// Write is a helper method to define mock.On call +// - r *store.Record +// - opts ...store.WriteOption +func (_e *Store_Expecter) Write(r interface{}, opts ...interface{}) *Store_Write_Call { + return &Store_Write_Call{Call: _e.mock.On("Write", + append([]interface{}{r}, opts...)...)} +} + +func (_c *Store_Write_Call) Run(run func(r *store.Record, opts ...store.WriteOption)) *Store_Write_Call { + _c.Call.Run(func(args mock.Arguments) { + var arg0 *store.Record + if args[0] != nil { + arg0 = args[0].(*store.Record) + } + var arg1 []store.WriteOption + var variadicArgs []store.WriteOption + if len(args) > 1 { + variadicArgs = args[1].([]store.WriteOption) + } + arg1 = variadicArgs + run( + arg0, + arg1..., + ) + }) + return _c +} + +func (_c *Store_Write_Call) Return(err error) *Store_Write_Call { + _c.Call.Return(err) + return _c +} + +func (_c *Store_Write_Call) RunAndReturn(run func(r *store.Record, opts ...store.WriteOption) error) *Store_Write_Call { + _c.Call.Return(run) + return _c +} From f1229243b178d953763e5be35c22574d48b03ee8 Mon Sep 17 00:00:00 2001 From: Florian Schade Date: Tue, 17 Feb 2026 11:13:05 +0100 Subject: [PATCH 09/11] chore: change naming --- .../internal/backchannellogout/backchannellogout.go | 11 ++++++++--- .../backchannellogout/backchannellogout_test.go | 6 +++--- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout.go b/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout.go index 306febd0ca..fb5c15ff36 100644 --- a/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout.go +++ b/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout.go @@ -26,13 +26,17 @@ var ErrInvalidSessionOrSubject = errors.New("invalid session or subject") func NewSuSe(key string) (SuSe, error) { var subject, session string switch keys := strings.Split(strings.Join(strings.Fields(key), ""), "."); { + // key: '.session' case len(keys) == 2 && keys[0] == "" && keys[1] != "": session = keys[1] + // key: 'subject.' case len(keys) == 2 && keys[0] != "" && keys[1] == "": subject = keys[0] + // key: 'subject.session' case len(keys) == 2 && keys[0] != "" && keys[1] != "": subject = keys[0] session = keys[1] + // key: 'session' case len(keys) == 1 && keys[0] != "": session = keys[0] default: @@ -46,8 +50,8 @@ func NewSuSe(key string) (SuSe, error) { type LogoutMode int const ( - // LogoutModeUnknown is used when the logout mode cannot be determined - LogoutModeUnknown LogoutMode = iota + // LogoutModeUndefined is used when the logout mode cannot be determined + LogoutModeUndefined LogoutMode = iota // LogoutModeSession is used when the logout mode is determined by the session id LogoutModeSession // LogoutModeSubject is used when the logout mode is determined by the subject @@ -62,7 +66,7 @@ func GetLogoutMode(suse SuSe) LogoutMode { case suse.Subject != "": return LogoutModeSubject default: - return LogoutModeUnknown + return LogoutModeUndefined } } @@ -91,6 +95,7 @@ func GetLogoutRecords(suse SuSe, mode LogoutMode, store microstore.Store) ([]*mi return nil, fmt.Errorf("%w: cannot determine logout mode", ErrSuspiciousCacheResult) } + // the go micro memory store requires a limit to work, why??? records, err := store.Read(key, append(opts, microstore.ReadLimit(1000))...) if err != nil { return nil, err diff --git a/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout_test.go b/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout_test.go index 238323c728..d442f06184 100644 --- a/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout_test.go +++ b/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout_test.go @@ -107,7 +107,7 @@ func TestGetLogoutMode(t *testing.T) { { name: "key variation: 'empty'", suSe: SuSe{Session: "", Subject: ""}, - want: LogoutModeUnknown, + want: LogoutModeUndefined, }, } @@ -155,7 +155,7 @@ func TestGetLogoutRecords(t *testing.T) { { name: "fails if mode is unknown", suSe: SuSe{Session: "session-a"}, - mode: LogoutModeUnknown, + mode: LogoutModeUndefined, store: func(t *testing.T) store.Store { return sessionStore }, @@ -192,7 +192,7 @@ func TestGetLogoutRecords(t *testing.T) { store: func(t *testing.T) store.Store { s := mocks.NewStore(t) s.EXPECT().Read(mock.Anything, mock.Anything).Return([]*store.Record{ - &store.Record{Key: "invalid.record.key"}, + {Key: "invalid.record.key"}, }, nil) return s }, From 544acb81406688f10ddd0e76c8460af504882c08 Mon Sep 17 00:00:00 2001 From: Florian Schade Date: Fri, 20 Feb 2026 16:45:38 +0100 Subject: [PATCH 10/11] fix: use base64 record keys to prevent separator clashes with subjects or sessionIds that contain a dot --- services/proxy/pkg/middleware/oidc_auth.go | 18 +- .../pkg/staticroutes/backchannellogout.go | 78 +++++--- .../backchannellogout/backchannellogout.go | 117 ++++++++---- .../backchannellogout_test.go | 174 +++++++++++------- 4 files changed, 253 insertions(+), 134 deletions(-) diff --git a/services/proxy/pkg/middleware/oidc_auth.go b/services/proxy/pkg/middleware/oidc_auth.go index 075f088b11..3a3ed29fdc 100644 --- a/services/proxy/pkg/middleware/oidc_auth.go +++ b/services/proxy/pkg/middleware/oidc_auth.go @@ -17,6 +17,7 @@ import ( "github.com/opencloud-eu/opencloud/pkg/log" "github.com/opencloud-eu/opencloud/pkg/oidc" + "github.com/opencloud-eu/opencloud/services/proxy/pkg/staticroutes" ) const ( @@ -116,22 +117,21 @@ func (m *OIDCAuthenticator) getClaims(token string, req *http.Request) (map[stri m.Logger.Error().Err(err).Msg("failed to write to userinfo cache") } - subject, sessionId := strings.Join(strings.Fields(aClaims.Subject), ""), strings.Join(strings.Fields(aClaims.SessionID), "") - // if no session id is present, we can't do a session lookup, - // so we can skip the cache entry for that. - if sessionId == "" { - return - } - // if the claim has no subject, we can leave it empty, // it's important to keep the dot in the key to prevent // sufix and prefix exploration in the cache. // // ok: {key: ".sessionId"} + // ok: {key: "subject."} // ok: {key: "subject.sessionId"} - key := strings.Join([]string{subject, sessionId}, ".") + subjectSessionKey, err := staticroutes.NewRecordKey(aClaims.Subject, aClaims.SessionID) + if err != nil { + m.Logger.Error().Err(err).Msg("failed to build subject.session") + return + } + if err := m.userInfoCache.Write(&store.Record{ - Key: key, + Key: subjectSessionKey, Value: []byte(encodedHash), Expiry: time.Until(expiration), }); err != nil { diff --git a/services/proxy/pkg/staticroutes/backchannellogout.go b/services/proxy/pkg/staticroutes/backchannellogout.go index 0d44e7cd1c..d7e10d5da8 100644 --- a/services/proxy/pkg/staticroutes/backchannellogout.go +++ b/services/proxy/pkg/staticroutes/backchannellogout.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "net/http" - "strings" "github.com/go-chi/render" "github.com/pkg/errors" @@ -16,6 +15,9 @@ import ( "github.com/opencloud-eu/reva/v2/pkg/utils" ) +// NewRecordKey converts the subject and session to a base64 encoded key +var NewRecordKey = bcl.NewKey + // backchannelLogout handles backchannel logout requests from the identity provider and invalidates the related sessions in the cache // spec: https://openid.net/specs/openid-connect-backchannel-1_0.html#BCRequest // @@ -37,9 +39,6 @@ import ( // all sessions besides the one that triggered the backchannel logout continue to exist in the identity provider, // so the user will not be fully logged out until all sessions are logged out or expired. // this leads to the situation that web renders the logout view even if the instance is not fully logged out yet. -// -// toDo: -// - check logs and errors to not contain any sensitive information like session ids or user ids (keys too) func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Request) { logger := s.Logger.SubloggerWithRequestID(r.Context()) if err := r.ParseForm(); err != nil { @@ -51,22 +50,31 @@ func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Re logoutToken, err := s.OidcClient.VerifyLogoutToken(r.Context(), r.PostFormValue("logout_token")) if err != nil { - logger.Warn().Err(err).Msg("VerifyLogoutToken failed") + msg := "failed to verify logout token" + logger.Warn().Err(err).Msg(msg) render.Status(r, http.StatusBadRequest) - render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()}) + render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: msg}) return } - subject, session := strings.Join(strings.Fields(logoutToken.Subject), ""), strings.Join(strings.Fields(logoutToken.SessionId), "") - if subject == "" && session == "" { - jseErr := jse{Error: "invalid_request", ErrorDescription: "invalid logout token: subject and session id are empty"} - logger.Warn().Msg(jseErr.ErrorDescription) + lookupKey, err := bcl.NewKey(logoutToken.Subject, logoutToken.SessionId) + if err != nil { + msg := "failed to build key from logout token" + logger.Warn().Err(err).Msg(msg) + render.Status(r, http.StatusBadRequest) + render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: msg}) + return + } + + requestSubjectAndSession, err := bcl.NewSuSe(lookupKey) + if err != nil { + msg := "failed to build subjec.session from lookupKey" + logger.Error().Err(err).Msg(msg) render.Status(r, http.StatusBadRequest) - render.JSON(w, r, jseErr) + render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: msg}) return } - requestSubjectAndSession := bcl.SuSe{Session: session, Subject: subject} // find out which mode of backchannel logout we are in // by checking if the session or subject is present in the token logoutMode := bcl.GetLogoutMode(requestSubjectAndSession) @@ -77,9 +85,10 @@ func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Re return } if err != nil { - logger.Error().Err(err).Msg("Error reading userinfo cache") + msg := "failed to read userinfo cache" + logger.Error().Err(err).Msg(msg) render.Status(r, http.StatusBadRequest) - render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()}) + render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: msg}) return } @@ -91,28 +100,36 @@ func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Re subjectSession, err := bcl.NewSuSe(key) if err != nil { // never leak any key-related information - logger.Warn().Err(err).Msgf("invalid logout record key: %s", "XXX") + logger.Warn().Err(err).Msgf("failed to parse key: %s", key) continue } - if err := s.publishBackchannelLogoutEvent(r.Context(), subjectSession.Session, value); err != nil { - s.Logger.Warn().Err(err).Msg("could not publish backchannel logout event") + session, err := subjectSession.Session() + if err != nil { + logger.Warn().Err(err).Msgf("failed to read session for: %s", key) + continue + } + + if err := s.publishBackchannelLogoutEvent(r.Context(), session, value); err != nil { + s.Logger.Warn().Err(err).Msgf("failed to publish backchannel logout event for: %s", key) + continue } err = s.UserInfoCache.Delete(value) if err != nil && !errors.Is(err, microstore.ErrNotFound) { // we have to return a 400 BadRequest when we fail to delete the session // https://openid.net/specs/openid-connect-backchannel-1_0.html#rfc.section.2.8 - logger.Err(err).Msg("could not delete user info from cache") + msg := "failed to delete record" + s.Logger.Warn().Err(err).Msgf("%s for: %s", msg, key) render.Status(r, http.StatusBadRequest) - render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: err.Error()}) + render.JSON(w, r, jse{Error: "invalid_request", ErrorDescription: msg}) return } // we can ignore errors when deleting the lookup record err = s.UserInfoCache.Delete(key) if err != nil { - logger.Debug().Err(err).Msg("Failed to cleanup sessionId lookup entry") + logger.Debug().Err(err).Msgf("failed to delete record for: %s", key) } } @@ -123,29 +140,30 @@ func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Re // publishBackchannelLogoutEvent publishes a backchannel logout event when the callback revived from the identity provider func (s *StaticRouteHandler) publishBackchannelLogoutEvent(ctx context.Context, sessionId, claimKey string) error { if s.EventsPublisher == nil { - return fmt.Errorf("the events publisher is not set") + return errors.New("events publisher not set") } + claimRecords, err := s.UserInfoCache.Read(claimKey) - if err != nil { - return fmt.Errorf("reading userinfo cache: %w", err) - } - if len(claimRecords) == 0 { - return fmt.Errorf("userinfo not found") + switch { + case err != nil: + return fmt.Errorf("failed to read userinfo cache: %w", err) + case len(claimRecords) == 0: + return fmt.Errorf("no claim found for key: %s", claimKey) } var claims map[string]interface{} if err = msgpack.Unmarshal(claimRecords[0].Value, &claims); err != nil { - return fmt.Errorf("could not unmarshal userinfo: %w", err) + return fmt.Errorf("failed to unmarshal claims: %w", err) } oidcClaim, ok := claims[s.Config.UserOIDCClaim].(string) if !ok { - return fmt.Errorf("could not get claim %w", err) + return fmt.Errorf("failed to get claim %w", err) } user, _, err := s.UserProvider.GetUserByClaims(ctx, s.Config.UserCS3Claim, oidcClaim) if err != nil || user.GetId() == nil { - return fmt.Errorf("could not get user by claims: %w", err) + return fmt.Errorf("failed to get user by claims: %w", err) } e := events.BackchannelLogout{ @@ -155,7 +173,7 @@ func (s *StaticRouteHandler) publishBackchannelLogoutEvent(ctx context.Context, } if err := events.Publish(ctx, s.EventsPublisher, e); err != nil { - return fmt.Errorf("could not publish user created event %w", err) + return fmt.Errorf("failed to publish user logout event %w", err) } return nil } diff --git a/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout.go b/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout.go index fb5c15ff36..1863047031 100644 --- a/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout.go +++ b/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout.go @@ -4,46 +4,97 @@ package backchannellogout import ( - "fmt" + "encoding/base64" + "errors" "strings" - "github.com/pkg/errors" microstore "go-micro.dev/v4/store" ) +// keyEncoding is the base64 encoding used for session and subject keys +var keyEncoding = base64.URLEncoding + +// ErrInvalidKey indicates that the provided key does not conform to the expected format. +var ErrInvalidKey = errors.New("invalid key format") + +// NewKey converts the subject and session to a base64 encoded key +func NewKey(subject, session string) (string, error) { + subjectSession := strings.Join([]string{ + keyEncoding.EncodeToString([]byte(subject)), + keyEncoding.EncodeToString([]byte(session)), + }, ".") + + if subjectSession == "." { + return "", ErrInvalidKey + } + + return subjectSession, nil +} + +// ErrDecoding is returned when decoding fails +var ErrDecoding = errors.New("failed to decode") + // SuSe 🦎 ;) is a struct that groups the subject and session together // to prevent mix-ups for ('session, subject' || 'subject, session') // return values. type SuSe struct { - Subject string - Session string + encodedSubject string + encodedSession string +} + +// Subject decodes and returns the subject or an error +func (suse SuSe) Subject() (string, error) { + subject, err := keyEncoding.DecodeString(suse.encodedSubject) + if err != nil { + return "", errors.Join(errors.New("failed to decode subject"), ErrDecoding, err) + } + + return string(subject), nil +} + +// Session decodes and returns the session or an error +func (suse SuSe) Session() (string, error) { + subject, err := keyEncoding.DecodeString(suse.encodedSession) + if err != nil { + return "", errors.Join(errors.New("failed to decode session"), ErrDecoding, err) + } + + return string(subject), nil } -// ErrInvalidSessionOrSubject is returned when the provided key does not match the expected key format -var ErrInvalidSessionOrSubject = errors.New("invalid session or subject") +// ErrInvalidSubjectOrSession is returned when the provided key does not match the expected key format +var ErrInvalidSubjectOrSession = errors.New("invalid subject or session") // NewSuSe parses the subject and session id from the given key and returns a SuSe struct func NewSuSe(key string) (SuSe, error) { - var subject, session string + suse := SuSe{} switch keys := strings.Split(strings.Join(strings.Fields(key), ""), "."); { // key: '.session' case len(keys) == 2 && keys[0] == "" && keys[1] != "": - session = keys[1] + suse.encodedSession = keys[1] // key: 'subject.' case len(keys) == 2 && keys[0] != "" && keys[1] == "": - subject = keys[0] + suse.encodedSubject = keys[0] // key: 'subject.session' case len(keys) == 2 && keys[0] != "" && keys[1] != "": - subject = keys[0] - session = keys[1] + suse.encodedSubject = keys[0] + suse.encodedSession = keys[1] // key: 'session' case len(keys) == 1 && keys[0] != "": - session = keys[0] + suse.encodedSession = keys[0] default: - return SuSe{}, ErrInvalidSessionOrSubject + return suse, ErrInvalidSubjectOrSession + } + + if _, err := suse.Subject(); err != nil { + return suse, errors.Join(ErrInvalidSubjectOrSession, err) } - return SuSe{Session: session, Subject: subject}, nil + if _, err := suse.Session(); err != nil { + return suse, errors.Join(ErrInvalidSubjectOrSession, err) + } + + return suse, nil } // LogoutMode defines the mode of backchannel logout, either by session or by subject @@ -52,19 +103,19 @@ type LogoutMode int const ( // LogoutModeUndefined is used when the logout mode cannot be determined LogoutModeUndefined LogoutMode = iota - // LogoutModeSession is used when the logout mode is determined by the session id - LogoutModeSession // LogoutModeSubject is used when the logout mode is determined by the subject LogoutModeSubject + // LogoutModeSession is used when the logout mode is determined by the session id + LogoutModeSession ) // GetLogoutMode determines the backchannel logout mode based on the presence of subject and session in the SuSe struct func GetLogoutMode(suse SuSe) LogoutMode { switch { - case suse.Session != "": - return LogoutModeSession - case suse.Subject != "": + case suse.encodedSession == "" && suse.encodedSubject != "": return LogoutModeSubject + case suse.encodedSession != "": + return LogoutModeSession default: return LogoutModeUndefined } @@ -81,18 +132,18 @@ func GetLogoutRecords(suse SuSe, mode LogoutMode, store microstore.Store) ([]*mi var key string var opts []microstore.ReadOption switch mode { - case LogoutModeSession: - // the dot at the beginning prevents sufix exploration in the cache, - // so only keys that end with '*.session' will be returned, but not '*sion'. - key = "." + suse.Session - opts = append(opts, microstore.ReadSuffix()) case LogoutModeSubject: // the dot at the end prevents prefix exploration in the cache, // so only keys that start with 'subject.*' will be returned, but not 'sub*'. - key = suse.Subject + "." + key = suse.encodedSubject + "." opts = append(opts, microstore.ReadPrefix()) + case LogoutModeSession: + // the dot at the beginning prevents sufix exploration in the cache, + // so only keys that end with '*.session' will be returned, but not '*sion'. + key = "." + suse.encodedSession + opts = append(opts, microstore.ReadSuffix()) default: - return nil, fmt.Errorf("%w: cannot determine logout mode", ErrSuspiciousCacheResult) + return nil, errors.Join(errors.New("cannot determine logout mode"), ErrSuspiciousCacheResult) } // the go micro memory store requires a limit to work, why??? @@ -106,7 +157,7 @@ func GetLogoutRecords(suse SuSe, mode LogoutMode, store microstore.Store) ([]*mi } if mode == LogoutModeSession && len(records) > 1 { - return nil, fmt.Errorf("%w: multiple session records found", ErrSuspiciousCacheResult) + return nil, errors.Join(errors.New("multiple session records found"), ErrSuspiciousCacheResult) } // double-check if the found records match the requested subject and or session id as well, @@ -115,19 +166,19 @@ func GetLogoutRecords(suse SuSe, mode LogoutMode, store microstore.Store) ([]*mi recordSuSe, err := NewSuSe(record.Key) if err != nil { // never leak any key-related information - return nil, fmt.Errorf("%w %w: failed to parse logout record key: %s", err, ErrSuspiciousCacheResult, "XXX") + return nil, errors.Join(errors.New("failed to parse key"), ErrSuspiciousCacheResult, err) } switch { - // in session mode, the session id must match, but the subject can be different - case mode == LogoutModeSession && suse.Session == recordSuSe.Session: - continue // in subject mode, the subject must match, but the session id can be different - case mode == LogoutModeSubject && suse.Subject == recordSuSe.Subject: + case mode == LogoutModeSubject && suse.encodedSubject == recordSuSe.encodedSubject: + continue + // in session mode, the session id must match, but the subject can be different + case mode == LogoutModeSession && suse.encodedSession == recordSuSe.encodedSession: continue } - return nil, fmt.Errorf("%w: record key does not match the requested subject or session", ErrSuspiciousCacheResult) + return nil, errors.Join(errors.New("key does not match the requested subject or session"), ErrSuspiciousCacheResult) } return records, nil diff --git a/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout_test.go b/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout_test.go index d442f06184..a653be2247 100644 --- a/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout_test.go +++ b/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout_test.go @@ -12,73 +12,123 @@ import ( "github.com/opencloud-eu/opencloud/services/proxy/pkg/staticroutes/internal/backchannellogout/mocks" ) -func TestNewSuSe(t *testing.T) { +func mustNewKey(t *testing.T, subject, session string) string { + key, err := NewKey(subject, session) + require.NoError(t, err) + return key +} + +func mustNewSuSe(t *testing.T, subject, session string) SuSe { + suse, err := NewSuSe(mustNewKey(t, subject, session)) + require.NoError(t, err) + return suse +} + +func TestNewKey(t *testing.T) { tests := []struct { - name string - key string - wantSuSe SuSe - wantErr error + name string + subject string + session string + wantKey string + wantErr error }{ { - name: "key variation: '.session'", - key: ".session", - wantSuSe: SuSe{Session: "session", Subject: ""}, + name: "key variation: 'subject.session'", + subject: "subject", + session: "session", + wantKey: "c3ViamVjdA==.c2Vzc2lvbg==", }, { - name: "key variation: '.session'", - key: ".session", - wantSuSe: SuSe{Session: "session", Subject: ""}, + name: "key variation: 'subject.'", + subject: "subject", + wantKey: "c3ViamVjdA==.", }, { - name: "key variation: 'session'", - key: "session", - wantSuSe: SuSe{Session: "session", Subject: ""}, + name: "key variation: '.session'", + session: "session", + wantKey: ".c2Vzc2lvbg==", }, { - name: "key variation: 'subject.'", - key: "subject.", - wantSuSe: SuSe{Session: "", Subject: "subject"}, + name: "key variation: '.'", + wantErr: ErrInvalidKey, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + key, err := NewKey(tt.subject, tt.session) + require.ErrorIs(t, err, tt.wantErr) + require.Equal(t, tt.wantKey, key) + }) + } +} + +func TestNewSuSe(t *testing.T) { + tests := []struct { + name string + key string + wantSubject string + wantSession string + wantErr error + }{ + { + name: "key variation: '.session'", + key: mustNewKey(t, "", "session"), + wantSession: "session", }, { - name: "key variation: 'subject.session'", - key: "subject.session", - wantSuSe: SuSe{Session: "session", Subject: "subject"}, + name: "key variation: 'session'", + key: mustNewKey(t, "", "session"), + wantSession: "session", }, { - name: "key variation: 'dot'", - key: ".", - wantSuSe: SuSe{Session: "", Subject: ""}, - wantErr: ErrInvalidSessionOrSubject, + name: "key variation: 'subject.'", + key: mustNewKey(t, "subject", ""), + wantSubject: "subject", }, { - name: "key variation: 'empty'", - key: "", - wantSuSe: SuSe{Session: "", Subject: ""}, - wantErr: ErrInvalidSessionOrSubject, + name: "key variation: 'subject.session'", + key: mustNewKey(t, "subject", "session"), + wantSubject: "subject", + wantSession: "session", }, { - name: "key variation: 'whitespace . whitespace'", - key: " . ", - wantSuSe: SuSe{Session: "", Subject: ""}, - wantErr: ErrInvalidSessionOrSubject, + name: "key variation: 'dot'", + key: ".", + wantErr: ErrInvalidSubjectOrSession, }, { - name: "key variation: 'whitespace subject whitespace . whitespace'", - key: " subject . ", - wantSuSe: SuSe{Session: "", Subject: "subject"}, + name: "key variation: 'empty'", + key: "", + wantErr: ErrInvalidSubjectOrSession, }, { - name: "key variation: 'whitespace . whitespace session whitespace'", - key: " . session ", - wantSuSe: SuSe{Session: "session", Subject: ""}, + name: "key variation: string('subject.session')", + key: "subject.session", + wantErr: ErrInvalidSubjectOrSession, + }, + { + name: "key variation: string('subject.')", + key: "subject.", + wantErr: ErrInvalidSubjectOrSession, + }, + { + name: "key variation: string('.session')", + key: ".session", + wantErr: ErrInvalidSubjectOrSession, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - suSe, ok := NewSuSe(tt.key) - require.ErrorIs(t, tt.wantErr, ok) - require.Equal(t, tt.wantSuSe, suSe) + suSe, err := NewSuSe(tt.key) + require.ErrorIs(t, err, tt.wantErr) + + subject, _ := suSe.Subject() + require.Equal(t, tt.wantSubject, subject) + + session, _ := suSe.Session() + require.Equal(t, tt.wantSession, session) }) } } @@ -91,22 +141,22 @@ func TestGetLogoutMode(t *testing.T) { }{ { name: "key variation: '.session'", - suSe: SuSe{Session: "session", Subject: ""}, + suSe: mustNewSuSe(t, "", "session"), want: LogoutModeSession, }, { name: "key variation: 'subject.session'", - suSe: SuSe{Session: "session", Subject: "subject"}, + suSe: mustNewSuSe(t, "subject", "session"), want: LogoutModeSession, }, { name: "key variation: 'subject.'", - suSe: SuSe{Session: "", Subject: "subject"}, + suSe: mustNewSuSe(t, "subject", ""), want: LogoutModeSubject, }, { name: "key variation: 'empty'", - suSe: SuSe{Session: "", Subject: ""}, + suSe: SuSe{}, want: LogoutModeUndefined, }, } @@ -126,10 +176,10 @@ func TestGetLogoutRecords(t *testing.T) { recordClaimB := &store.Record{Key: "claim-b", Value: []byte("claim-b-data")} recordClaimC := &store.Record{Key: "claim-c", Value: []byte("claim-c-data")} recordClaimD := &store.Record{Key: "claim-d", Value: []byte("claim-d-data")} - recordSessionA := &store.Record{Key: ".session-a", Value: []byte(recordClaimA.Key)} - recordSessionB := &store.Record{Key: ".session-b", Value: []byte(recordClaimB.Key)} - recordSubjectASessionC := &store.Record{Key: "subject-a.session-c", Value: []byte(recordSessionA.Key)} - recordSubjectASessionD := &store.Record{Key: "subject-a.session-d", Value: []byte(recordSessionB.Key)} + recordSessionA := &store.Record{Key: mustNewKey(t, "", "session-a"), Value: []byte(recordClaimA.Key)} + recordSessionB := &store.Record{Key: mustNewKey(t, "", "session-b"), Value: []byte(recordClaimB.Key)} + recordSubjectASessionC := &store.Record{Key: mustNewKey(t, "subject-a", "session-c"), Value: []byte(recordSessionA.Key)} + recordSubjectASessionD := &store.Record{Key: mustNewKey(t, "subject-a", "session-d"), Value: []byte(recordSessionA.Key)} for _, r := range []*store.Record{ recordClaimA, @@ -154,7 +204,7 @@ func TestGetLogoutRecords(t *testing.T) { }{ { name: "fails if mode is unknown", - suSe: SuSe{Session: "session-a"}, + suSe: mustNewSuSe(t, "", "session-a"), mode: LogoutModeUndefined, store: func(t *testing.T) store.Store { return sessionStore @@ -164,7 +214,7 @@ func TestGetLogoutRecords(t *testing.T) { }, { name: "fails if mode is any random int", - suSe: SuSe{Session: "session-a"}, + suSe: mustNewSuSe(t, "", "session-a"), mode: 999, store: func(t *testing.T) store.Store { return sessionStore @@ -173,7 +223,7 @@ func TestGetLogoutRecords(t *testing.T) { wantErrs: []error{ErrSuspiciousCacheResult}}, { name: "fails if multiple session records are found", - suSe: SuSe{Session: "session-a"}, + suSe: mustNewSuSe(t, "", "session-a"), mode: LogoutModeSession, store: func(t *testing.T) store.Store { s := mocks.NewStore(t) @@ -187,7 +237,7 @@ func TestGetLogoutRecords(t *testing.T) { wantErrs: []error{ErrSuspiciousCacheResult}}, { name: "fails if the record key is not ok", - suSe: SuSe{Session: "session-a"}, + suSe: mustNewSuSe(t, "", "session-a"), mode: LogoutModeSession, store: func(t *testing.T) store.Store { s := mocks.NewStore(t) @@ -197,11 +247,11 @@ func TestGetLogoutRecords(t *testing.T) { return s }, wantRecords: []*store.Record{}, - wantErrs: []error{ErrInvalidSessionOrSubject, ErrSuspiciousCacheResult}, + wantErrs: []error{ErrInvalidSubjectOrSession, ErrSuspiciousCacheResult}, }, { name: "fails if the session does not match the retrieved record", - suSe: SuSe{Session: "session-a"}, + suSe: mustNewSuSe(t, "", "session-a"), mode: LogoutModeSession, store: func(t *testing.T) store.Store { s := mocks.NewStore(t) @@ -214,7 +264,7 @@ func TestGetLogoutRecords(t *testing.T) { wantErrs: []error{ErrSuspiciousCacheResult}}, { name: "fails if the subject does not match the retrieved record", - suSe: SuSe{Subject: "subject-a"}, + suSe: mustNewSuSe(t, "subject-a", ""), mode: LogoutModeSubject, store: func(t *testing.T) store.Store { s := mocks.NewStore(t) @@ -228,7 +278,7 @@ func TestGetLogoutRecords(t *testing.T) { // key variation tests { name: "key variation: 'session-a'", - suSe: SuSe{Session: "session-a"}, + suSe: mustNewSuSe(t, "", "session-a"), mode: LogoutModeSession, store: func(*testing.T) store.Store { return sessionStore @@ -237,7 +287,7 @@ func TestGetLogoutRecords(t *testing.T) { }, { name: "key variation: 'session-b'", - suSe: SuSe{Session: "session-b"}, + suSe: mustNewSuSe(t, "", "session-b"), mode: LogoutModeSession, store: func(*testing.T) store.Store { return sessionStore @@ -246,7 +296,7 @@ func TestGetLogoutRecords(t *testing.T) { }, { name: "key variation: 'session-c'", - suSe: SuSe{Session: "session-c"}, + suSe: mustNewSuSe(t, "", "session-c"), mode: LogoutModeSession, store: func(*testing.T) store.Store { return sessionStore @@ -255,7 +305,7 @@ func TestGetLogoutRecords(t *testing.T) { }, { name: "key variation: 'ession-c'", - suSe: SuSe{Session: "ession-c"}, + suSe: mustNewSuSe(t, "", "ession-c"), mode: LogoutModeSession, store: func(*testing.T) store.Store { return sessionStore @@ -265,7 +315,7 @@ func TestGetLogoutRecords(t *testing.T) { }, { name: "key variation: 'subject-a'", - suSe: SuSe{Subject: "subject-a"}, + suSe: mustNewSuSe(t, "subject-a", ""), mode: LogoutModeSubject, store: func(*testing.T) store.Store { return sessionStore @@ -274,7 +324,7 @@ func TestGetLogoutRecords(t *testing.T) { }, { name: "key variation: 'subject-'", - suSe: SuSe{Subject: "subject-"}, + suSe: mustNewSuSe(t, "subject-", ""), mode: LogoutModeSubject, store: func(*testing.T) store.Store { return sessionStore From f1bd274c210ff6abc5819fc8ffd2c3e8b8c969a5 Mon Sep 17 00:00:00 2001 From: Florian Schade Date: Fri, 20 Feb 2026 16:52:29 +0100 Subject: [PATCH 11/11] refactor: make the logout mode private --- services/proxy/pkg/middleware/oidc_auth.go | 7 ++- .../pkg/staticroutes/backchannellogout.go | 5 +-- .../backchannellogout/backchannellogout.go | 41 +++++++++-------- .../backchannellogout_test.go | 44 +++---------------- 4 files changed, 33 insertions(+), 64 deletions(-) diff --git a/services/proxy/pkg/middleware/oidc_auth.go b/services/proxy/pkg/middleware/oidc_auth.go index 3a3ed29fdc..00822cdc99 100644 --- a/services/proxy/pkg/middleware/oidc_auth.go +++ b/services/proxy/pkg/middleware/oidc_auth.go @@ -8,7 +8,6 @@ import ( "strings" "time" - "github.com/golang-jwt/jwt/v5" "github.com/pkg/errors" "github.com/vmihailenco/msgpack/v5" "go-micro.dev/v4/store" @@ -117,13 +116,13 @@ func (m *OIDCAuthenticator) getClaims(token string, req *http.Request) (map[stri m.Logger.Error().Err(err).Msg("failed to write to userinfo cache") } - // if the claim has no subject, we can leave it empty, - // it's important to keep the dot in the key to prevent - // sufix and prefix exploration in the cache. + // fail if creating the storage key fails, + // it means there is no subject and no session. // // ok: {key: ".sessionId"} // ok: {key: "subject."} // ok: {key: "subject.sessionId"} + // fail: {key: "."} subjectSessionKey, err := staticroutes.NewRecordKey(aClaims.Subject, aClaims.SessionID) if err != nil { m.Logger.Error().Err(err).Msg("failed to build subject.session") diff --git a/services/proxy/pkg/staticroutes/backchannellogout.go b/services/proxy/pkg/staticroutes/backchannellogout.go index d7e10d5da8..53375d63bb 100644 --- a/services/proxy/pkg/staticroutes/backchannellogout.go +++ b/services/proxy/pkg/staticroutes/backchannellogout.go @@ -75,10 +75,7 @@ func (s *StaticRouteHandler) backchannelLogout(w http.ResponseWriter, r *http.Re return } - // find out which mode of backchannel logout we are in - // by checking if the session or subject is present in the token - logoutMode := bcl.GetLogoutMode(requestSubjectAndSession) - lookupRecords, err := bcl.GetLogoutRecords(requestSubjectAndSession, logoutMode, s.UserInfoCache) + lookupRecords, err := bcl.GetLogoutRecords(requestSubjectAndSession, s.UserInfoCache) if errors.Is(err, microstore.ErrNotFound) || len(lookupRecords) == 0 { render.Status(r, http.StatusOK) render.JSON(w, r, nil) diff --git a/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout.go b/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout.go index 1863047031..86ee00556b 100644 --- a/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout.go +++ b/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout.go @@ -97,27 +97,27 @@ func NewSuSe(key string) (SuSe, error) { return suse, nil } -// LogoutMode defines the mode of backchannel logout, either by session or by subject -type LogoutMode int +// logoutMode defines the mode of backchannel logout, either by session or by subject +type logoutMode int const ( - // LogoutModeUndefined is used when the logout mode cannot be determined - LogoutModeUndefined LogoutMode = iota - // LogoutModeSubject is used when the logout mode is determined by the subject - LogoutModeSubject - // LogoutModeSession is used when the logout mode is determined by the session id - LogoutModeSession + // logoutModeUndefined is used when the logout mode cannot be determined + logoutModeUndefined logoutMode = iota + // logoutModeSubject is used when the logout mode is determined by the subject + logoutModeSubject + // logoutModeSession is used when the logout mode is determined by the session id + logoutModeSession ) -// GetLogoutMode determines the backchannel logout mode based on the presence of subject and session in the SuSe struct -func GetLogoutMode(suse SuSe) LogoutMode { +// getLogoutMode determines the backchannel logout mode based on the presence of subject and session in the SuSe struct +func getLogoutMode(suse SuSe) logoutMode { switch { case suse.encodedSession == "" && suse.encodedSubject != "": - return LogoutModeSubject + return logoutModeSubject case suse.encodedSession != "": - return LogoutModeSession + return logoutModeSession default: - return LogoutModeUndefined + return logoutModeUndefined } } @@ -128,16 +128,19 @@ var ErrSuspiciousCacheResult = errors.New("suspicious cache result") // logout mode and the provided SuSe struct. // it uses a seperator to prevent sufix and prefix exploration in the cache and checks // if the retrieved records match the requested subject and or session id as well, to prevent false positives. -func GetLogoutRecords(suse SuSe, mode LogoutMode, store microstore.Store) ([]*microstore.Record, error) { +func GetLogoutRecords(suse SuSe, store microstore.Store) ([]*microstore.Record, error) { + // get subject.session mode + mode := getLogoutMode(suse) + var key string var opts []microstore.ReadOption switch mode { - case LogoutModeSubject: + case logoutModeSubject: // the dot at the end prevents prefix exploration in the cache, // so only keys that start with 'subject.*' will be returned, but not 'sub*'. key = suse.encodedSubject + "." opts = append(opts, microstore.ReadPrefix()) - case LogoutModeSession: + case logoutModeSession: // the dot at the beginning prevents sufix exploration in the cache, // so only keys that end with '*.session' will be returned, but not '*sion'. key = "." + suse.encodedSession @@ -156,7 +159,7 @@ func GetLogoutRecords(suse SuSe, mode LogoutMode, store microstore.Store) ([]*mi return nil, microstore.ErrNotFound } - if mode == LogoutModeSession && len(records) > 1 { + if mode == logoutModeSession && len(records) > 1 { return nil, errors.Join(errors.New("multiple session records found"), ErrSuspiciousCacheResult) } @@ -171,10 +174,10 @@ func GetLogoutRecords(suse SuSe, mode LogoutMode, store microstore.Store) ([]*mi switch { // in subject mode, the subject must match, but the session id can be different - case mode == LogoutModeSubject && suse.encodedSubject == recordSuSe.encodedSubject: + case mode == logoutModeSubject && suse.encodedSubject == recordSuSe.encodedSubject: continue // in session mode, the session id must match, but the subject can be different - case mode == LogoutModeSession && suse.encodedSession == recordSuSe.encodedSession: + case mode == logoutModeSession && suse.encodedSession == recordSuSe.encodedSession: continue } diff --git a/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout_test.go b/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout_test.go index a653be2247..617bd6d9e0 100644 --- a/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout_test.go +++ b/services/proxy/pkg/staticroutes/internal/backchannellogout/backchannellogout_test.go @@ -137,33 +137,33 @@ func TestGetLogoutMode(t *testing.T) { tests := []struct { name string suSe SuSe - want LogoutMode + want logoutMode }{ { name: "key variation: '.session'", suSe: mustNewSuSe(t, "", "session"), - want: LogoutModeSession, + want: logoutModeSession, }, { name: "key variation: 'subject.session'", suSe: mustNewSuSe(t, "subject", "session"), - want: LogoutModeSession, + want: logoutModeSession, }, { name: "key variation: 'subject.'", suSe: mustNewSuSe(t, "subject", ""), - want: LogoutModeSubject, + want: logoutModeSubject, }, { name: "key variation: 'empty'", suSe: SuSe{}, - want: LogoutModeUndefined, + want: logoutModeUndefined, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - mode := GetLogoutMode(tt.suSe) + mode := getLogoutMode(tt.suSe) require.Equal(t, tt.want, mode) }) } @@ -197,34 +197,13 @@ func TestGetLogoutRecords(t *testing.T) { tests := []struct { name string suSe SuSe - mode LogoutMode store func(t *testing.T) store.Store wantRecords []*store.Record wantErrs []error }{ - { - name: "fails if mode is unknown", - suSe: mustNewSuSe(t, "", "session-a"), - mode: LogoutModeUndefined, - store: func(t *testing.T) store.Store { - return sessionStore - }, - wantRecords: []*store.Record{}, - wantErrs: []error{ErrSuspiciousCacheResult}, - }, - { - name: "fails if mode is any random int", - suSe: mustNewSuSe(t, "", "session-a"), - mode: 999, - store: func(t *testing.T) store.Store { - return sessionStore - }, - wantRecords: []*store.Record{}, - wantErrs: []error{ErrSuspiciousCacheResult}}, { name: "fails if multiple session records are found", suSe: mustNewSuSe(t, "", "session-a"), - mode: LogoutModeSession, store: func(t *testing.T) store.Store { s := mocks.NewStore(t) s.EXPECT().Read(mock.Anything, mock.Anything).Return([]*store.Record{ @@ -238,7 +217,6 @@ func TestGetLogoutRecords(t *testing.T) { { name: "fails if the record key is not ok", suSe: mustNewSuSe(t, "", "session-a"), - mode: LogoutModeSession, store: func(t *testing.T) store.Store { s := mocks.NewStore(t) s.EXPECT().Read(mock.Anything, mock.Anything).Return([]*store.Record{ @@ -252,7 +230,6 @@ func TestGetLogoutRecords(t *testing.T) { { name: "fails if the session does not match the retrieved record", suSe: mustNewSuSe(t, "", "session-a"), - mode: LogoutModeSession, store: func(t *testing.T) store.Store { s := mocks.NewStore(t) s.EXPECT().Read(mock.Anything, mock.Anything).Return([]*store.Record{ @@ -265,7 +242,6 @@ func TestGetLogoutRecords(t *testing.T) { { name: "fails if the subject does not match the retrieved record", suSe: mustNewSuSe(t, "subject-a", ""), - mode: LogoutModeSubject, store: func(t *testing.T) store.Store { s := mocks.NewStore(t) s.EXPECT().Read(mock.Anything, mock.Anything).Return([]*store.Record{ @@ -279,7 +255,6 @@ func TestGetLogoutRecords(t *testing.T) { { name: "key variation: 'session-a'", suSe: mustNewSuSe(t, "", "session-a"), - mode: LogoutModeSession, store: func(*testing.T) store.Store { return sessionStore }, @@ -288,7 +263,6 @@ func TestGetLogoutRecords(t *testing.T) { { name: "key variation: 'session-b'", suSe: mustNewSuSe(t, "", "session-b"), - mode: LogoutModeSession, store: func(*testing.T) store.Store { return sessionStore }, @@ -297,7 +271,6 @@ func TestGetLogoutRecords(t *testing.T) { { name: "key variation: 'session-c'", suSe: mustNewSuSe(t, "", "session-c"), - mode: LogoutModeSession, store: func(*testing.T) store.Store { return sessionStore }, @@ -306,7 +279,6 @@ func TestGetLogoutRecords(t *testing.T) { { name: "key variation: 'ession-c'", suSe: mustNewSuSe(t, "", "ession-c"), - mode: LogoutModeSession, store: func(*testing.T) store.Store { return sessionStore }, @@ -316,7 +288,6 @@ func TestGetLogoutRecords(t *testing.T) { { name: "key variation: 'subject-a'", suSe: mustNewSuSe(t, "subject-a", ""), - mode: LogoutModeSubject, store: func(*testing.T) store.Store { return sessionStore }, @@ -325,7 +296,6 @@ func TestGetLogoutRecords(t *testing.T) { { name: "key variation: 'subject-'", suSe: mustNewSuSe(t, "subject-", ""), - mode: LogoutModeSubject, store: func(*testing.T) store.Store { return sessionStore }, @@ -336,7 +306,7 @@ func TestGetLogoutRecords(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - records, err := GetLogoutRecords(tt.suSe, tt.mode, tt.store(t)) + records, err := GetLogoutRecords(tt.suSe, tt.store(t)) for _, wantErr := range tt.wantErrs { require.ErrorIs(t, err, wantErr) }