From 1ece042e381b201d169315856267563ae14939b6 Mon Sep 17 00:00:00 2001 From: Louis Chan Date: Mon, 19 Aug 2024 19:26:42 +0800 Subject: [PATCH] Clean up Redis Hash holding sessions when a user is deleted or anonymized ref DEV-1809 --- pkg/lib/facade/coordinator.go | 30 +++++++++++++++++-- pkg/lib/oauth/redis/store.go | 14 +++++++++ pkg/lib/oauth/session_manager.go | 4 +++ pkg/lib/oauth/store_grant.go | 2 ++ pkg/lib/oauth/store_grant_mock.go | 14 +++++++++ pkg/lib/session/idpsession/manager.go | 4 +++ pkg/lib/session/idpsession/store.go | 2 ++ pkg/lib/session/idpsession/store_mock_test.go | 14 +++++++++ pkg/lib/session/idpsession/store_redis.go | 13 ++++++++ 9 files changed, 95 insertions(+), 2 deletions(-) diff --git a/pkg/lib/facade/coordinator.go b/pkg/lib/facade/coordinator.go index a15ae7a6aa..87e10721df 100644 --- a/pkg/lib/facade/coordinator.go +++ b/pkg/lib/facade/coordinator.go @@ -126,6 +126,7 @@ type OAuthService interface { type SessionManager interface { Delete(session session.ListableSession) error List(userID string) ([]session.ListableSession, error) + CleanUpForDeletingUserID(userID string) error } type StdAttrsService interface { @@ -671,7 +672,7 @@ func (c *Coordinator) UserDelete(userID string, isScheduledDeletion bool) error } // Sessions: - if err = c.terminateAllSessions(userID); err != nil { + if err = c.terminateAllSessionsAndCleanUp(userID); err != nil { return err } @@ -826,6 +827,31 @@ func (c *Coordinator) markOAuthEmailAsVerified(info *identity.Info) error { return nil } +func (c *Coordinator) terminateAllSessionsAndCleanUp(userID string) error { + err := c.terminateAllSessions(userID) + if err != nil { + return err + } + + // According to https://redis.io/docs/latest/commands/hdel/ + // When the last field of a Redis hash is deleted, the hash is deleted. + // So in most case, we do not actually need this. + // However, List() does not actually return every key that the Redis hash has, + // so having CleanUpForDeletingUserID ensures we delete the hash. + + err = c.IDPSessions.CleanUpForDeletingUserID(userID) + if err != nil { + return err + } + + err = c.OAuthSessions.CleanUpForDeletingUserID(userID) + if err != nil { + return err + } + + return nil +} + func (c *Coordinator) terminateAllSessions(userID string) error { idpSessions, err := c.IDPSessions.List(userID) if err != nil { @@ -1062,7 +1088,7 @@ func (c *Coordinator) UserAnonymize(userID string, IsScheduledAnonymization bool } // Sessions: - if err = c.terminateAllSessions(userID); err != nil { + if err = c.terminateAllSessionsAndCleanUp(userID); err != nil { return err } diff --git a/pkg/lib/oauth/redis/store.go b/pkg/lib/oauth/redis/store.go index 415bdf04c7..3137fbbac1 100644 --- a/pkg/lib/oauth/redis/store.go +++ b/pkg/lib/oauth/redis/store.go @@ -727,3 +727,17 @@ func (s *Store) ConsumePreAuthenticatedURLToken(tokenHash string) (*oauth.PreAut return t, nil } + +func (s *Store) CleanUpForDeletingUserID(userID string) (err error) { + ctx := context.Background() + listKey := offlineGrantListKey(string(s.AppID), userID) + + err = s.Redis.WithConn(func(conn *goredis.Conn) error { + _, err := conn.Del(ctx, listKey).Result() + if err != nil { + return err + } + return nil + }) + return +} diff --git a/pkg/lib/oauth/session_manager.go b/pkg/lib/oauth/session_manager.go index d0a443cfc2..e126673d77 100644 --- a/pkg/lib/oauth/session_manager.go +++ b/pkg/lib/oauth/session_manager.go @@ -94,3 +94,7 @@ func (m *SessionManager) TerminateAllExcept(userID string, currentSession sessio return deletedSessions, nil } + +func (m *SessionManager) CleanUpForDeletingUserID(userID string) error { + return m.Store.CleanUpForDeletingUserID(userID) +} diff --git a/pkg/lib/oauth/store_grant.go b/pkg/lib/oauth/store_grant.go index c924a43e61..4f5c7cc7e1 100644 --- a/pkg/lib/oauth/store_grant.go +++ b/pkg/lib/oauth/store_grant.go @@ -48,6 +48,8 @@ type OfflineGrantStore interface { ListOfflineGrants(userID string) ([]*OfflineGrant, error) ListClientOfflineGrants(clientID string, userID string) ([]*OfflineGrant, error) + + CleanUpForDeletingUserID(userID string) error } type IDPSessionProvider interface { diff --git a/pkg/lib/oauth/store_grant_mock.go b/pkg/lib/oauth/store_grant_mock.go index e8af6dc4d8..e1baef2046 100644 --- a/pkg/lib/oauth/store_grant_mock.go +++ b/pkg/lib/oauth/store_grant_mock.go @@ -214,6 +214,20 @@ func (mr *MockOfflineGrantStoreMockRecorder) AddOfflineGrantRefreshToken(grantID return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddOfflineGrantRefreshToken", reflect.TypeOf((*MockOfflineGrantStore)(nil).AddOfflineGrantRefreshToken), grantID, expireAt, tokenHash, clientID, scopes, authorizationID, dpopJKT) } +// CleanUpForDeletingUserID mocks base method. +func (m *MockOfflineGrantStore) CleanUpForDeletingUserID(userID string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CleanUpForDeletingUserID", userID) + ret0, _ := ret[0].(error) + return ret0 +} + +// CleanUpForDeletingUserID indicates an expected call of CleanUpForDeletingUserID. +func (mr *MockOfflineGrantStoreMockRecorder) CleanUpForDeletingUserID(userID interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CleanUpForDeletingUserID", reflect.TypeOf((*MockOfflineGrantStore)(nil).CleanUpForDeletingUserID), userID) +} + // CreateOfflineGrant mocks base method. func (m *MockOfflineGrantStore) CreateOfflineGrant(offlineGrant *OfflineGrant, expireAt time.Time) error { m.ctrl.T.Helper() diff --git a/pkg/lib/session/idpsession/manager.go b/pkg/lib/session/idpsession/manager.go index 9299ede7a5..782f04431b 100644 --- a/pkg/lib/session/idpsession/manager.go +++ b/pkg/lib/session/idpsession/manager.go @@ -79,3 +79,7 @@ func (m *Manager) TerminateAllExcept(userID string, currentSession session.Resol return deletedSessions, nil } + +func (m *Manager) CleanUpForDeletingUserID(userID string) error { + return m.Store.CleanUpForDeletingUserID(userID) +} diff --git a/pkg/lib/session/idpsession/store.go b/pkg/lib/session/idpsession/store.go index e5dadb3bfa..f29ea85d94 100644 --- a/pkg/lib/session/idpsession/store.go +++ b/pkg/lib/session/idpsession/store.go @@ -19,4 +19,6 @@ type Store interface { Delete(*IDPSession) error // List lists the sessions belonging to the user, in ascending creation time order List(userID string) ([]*IDPSession, error) + // CleanUpForDeletingUserID cleans up for a deleting user ID. + CleanUpForDeletingUserID(userID string) error } diff --git a/pkg/lib/session/idpsession/store_mock_test.go b/pkg/lib/session/idpsession/store_mock_test.go index a7d101547c..28f60ebd99 100644 --- a/pkg/lib/session/idpsession/store_mock_test.go +++ b/pkg/lib/session/idpsession/store_mock_test.go @@ -34,6 +34,20 @@ func (m *MockStore) EXPECT() *MockStoreMockRecorder { return m.recorder } +// CleanUpForDeletingUserID mocks base method. +func (m *MockStore) CleanUpForDeletingUserID(userID string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CleanUpForDeletingUserID", userID) + ret0, _ := ret[0].(error) + return ret0 +} + +// CleanUpForDeletingUserID indicates an expected call of CleanUpForDeletingUserID. +func (mr *MockStoreMockRecorder) CleanUpForDeletingUserID(userID interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CleanUpForDeletingUserID", reflect.TypeOf((*MockStore)(nil).CleanUpForDeletingUserID), userID) +} + // Create mocks base method. func (m *MockStore) Create(s *IDPSession, expireAt time.Time) error { m.ctrl.T.Helper() diff --git a/pkg/lib/session/idpsession/store_redis.go b/pkg/lib/session/idpsession/store_redis.go index 70f111acc4..1ddf9487d2 100644 --- a/pkg/lib/session/idpsession/store_redis.go +++ b/pkg/lib/session/idpsession/store_redis.go @@ -169,6 +169,19 @@ func (s *StoreRedis) Delete(session *IDPSession) (err error) { return } +func (s *StoreRedis) CleanUpForDeletingUserID(userID string) (err error) { + ctx := context.Background() + listKey := sessionListKey(s.AppID, userID) + err = s.Redis.WithConn(func(conn *goredis.Conn) error { + _, err := conn.Del(ctx, listKey).Result() + if err != nil { + return err + } + return nil + }) + return +} + //nolint:gocognit func (s *StoreRedis) List(userID string) (sessions []*IDPSession, err error) { ctx := context.Background()