From 2d3564b36bb75c869497beebd6bc6fe1db7a75f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Viktor=20Tigerstr=C3=B6m?= Date: Tue, 19 Aug 2025 01:17:44 +0200 Subject: [PATCH 1/2] session: add expected results to migration tests In the upcoming commit, we will update the kvdb to sql migration to not always include all sessions in the kvdb store. Therefore, we update the kvdb to migration tests in order to be able to handle such cases by including the expected results for each test case. --- session/sql_migration_test.go | 77 ++++++++++++++++++++++++----------- 1 file changed, 53 insertions(+), 24 deletions(-) diff --git a/session/sql_migration_test.go b/session/sql_migration_test.go index 3c3b018e7..77e003fed 100644 --- a/session/sql_migration_test.go +++ b/session/sql_migration_test.go @@ -91,32 +91,35 @@ func TestSessionsStoreMigration(t *testing.T) { populateDB func( t *testing.T, kvStore *BoltStore, accountStore accounts.Store, - ) + ) []*Session }{ { name: "empty", populateDB: func(t *testing.T, store *BoltStore, - _ accounts.Store) { + _ accounts.Store) []*Session { // Don't populate the DB. + return []*Session{} }, }, { name: "one session no options", populateDB: func(t *testing.T, store *BoltStore, - _ accounts.Store) { + _ accounts.Store) []*Session { _, err := store.NewSession( ctx, "test", TypeMacaroonAdmin, time.Unix(1000, 0), "", ) require.NoError(t, err) + + return getBoltStoreSessions(t, store) }, }, { name: "multiple sessions no options", populateDB: func(t *testing.T, store *BoltStore, - _ accounts.Store) { + _ accounts.Store) []*Session { _, err := store.NewSession( ctx, "session1", TypeMacaroonAdmin, @@ -135,12 +138,14 @@ func TestSessionsStoreMigration(t *testing.T) { time.Unix(1000, 0), "", ) require.NoError(t, err) + + return getBoltStoreSessions(t, store) }, }, { name: "one session with one privacy flag", populateDB: func(t *testing.T, store *BoltStore, - _ accounts.Store) { + _ accounts.Store) []*Session { _, err := store.NewSession( ctx, "test", TypeMacaroonAdmin, @@ -148,12 +153,14 @@ func TestSessionsStoreMigration(t *testing.T) { WithPrivacy(PrivacyFlags{ClearPubkeys}), ) require.NoError(t, err) + + return getBoltStoreSessions(t, store) }, }, { name: "one session with multiple privacy flags", populateDB: func(t *testing.T, store *BoltStore, - _ accounts.Store) { + _ accounts.Store) []*Session { _, err := store.NewSession( ctx, "test", TypeMacaroonAdmin, @@ -164,12 +171,14 @@ func TestSessionsStoreMigration(t *testing.T) { }), ) require.NoError(t, err) + + return getBoltStoreSessions(t, store) }, }, { name: "one session with a feature config", populateDB: func(t *testing.T, store *BoltStore, - _ accounts.Store) { + _ accounts.Store) []*Session { featureConfig := map[string][]byte{ "AutoFees": {1, 2, 3, 4}, @@ -182,12 +191,14 @@ func TestSessionsStoreMigration(t *testing.T) { WithFeatureConfig(featureConfig), ) require.NoError(t, err) + + return getBoltStoreSessions(t, store) }, }, { name: "one session with dev server", populateDB: func(t *testing.T, store *BoltStore, - _ accounts.Store) { + _ accounts.Store) []*Session { _, err := store.NewSession( ctx, "test", TypeMacaroonAdmin, @@ -195,12 +206,14 @@ func TestSessionsStoreMigration(t *testing.T) { WithDevServer(), ) require.NoError(t, err) + + return getBoltStoreSessions(t, store) }, }, { name: "one session with macaroon recipe", populateDB: func(t *testing.T, store *BoltStore, - _ accounts.Store) { + _ accounts.Store) []*Session { // this test uses caveats & perms from the // tlv_test.go @@ -210,12 +223,14 @@ func TestSessionsStoreMigration(t *testing.T) { WithMacaroonRecipe(caveats, perms), ) require.NoError(t, err) + + return getBoltStoreSessions(t, store) }, }, { name: "one session with macaroon recipe nil caveats", populateDB: func(t *testing.T, store *BoltStore, - _ accounts.Store) { + _ accounts.Store) []*Session { // this test uses perms from the tlv_test.go _, err := store.NewSession( @@ -224,12 +239,14 @@ func TestSessionsStoreMigration(t *testing.T) { WithMacaroonRecipe(nil, perms), ) require.NoError(t, err) + + return getBoltStoreSessions(t, store) }, }, { name: "one session with macaroon recipe nil perms", populateDB: func(t *testing.T, store *BoltStore, - _ accounts.Store) { + _ accounts.Store) []*Session { // this test uses caveats from the tlv_test.go _, err := store.NewSession( @@ -238,12 +255,14 @@ func TestSessionsStoreMigration(t *testing.T) { WithMacaroonRecipe(caveats, nil), ) require.NoError(t, err) + + return getBoltStoreSessions(t, store) }, }, { name: "macaroon recipe with nil perms and caveats", populateDB: func(t *testing.T, store *BoltStore, - _ accounts.Store) { + _ accounts.Store) []*Session { _, err := store.NewSession( ctx, "test", TypeMacaroonAdmin, @@ -251,12 +270,14 @@ func TestSessionsStoreMigration(t *testing.T) { WithMacaroonRecipe(nil, nil), ) require.NoError(t, err) + + return getBoltStoreSessions(t, store) }, }, { name: "one session with a linked account", populateDB: func(t *testing.T, store *BoltStore, - acctStore accounts.Store) { + acctStore accounts.Store) []*Session { // Create an account with balance acct, err := acctStore.NewAccount( @@ -289,12 +310,14 @@ func TestSessionsStoreMigration(t *testing.T) { WithMacaroonRecipe(sessCaveats, nil), ) require.NoError(t, err) + + return getBoltStoreSessions(t, store) }, }, { name: "linked session", populateDB: func(t *testing.T, store *BoltStore, - _ accounts.Store) { + _ accounts.Store) []*Session { // First create the initial session for the // group. @@ -325,6 +348,8 @@ func TestSessionsStoreMigration(t *testing.T) { WithLinkedGroupID(&sess1.ID), ) require.NoError(t, err) + + return getBoltStoreSessions(t, store) }, }, { @@ -355,15 +380,7 @@ func TestSessionsStoreMigration(t *testing.T) { // populate the kvStore with the test data, in // preparation for the test. - test.populateDB(t, kvStore, accountStore) - - // Before we migrate the sessions, we fetch all sessions - // from the kv store, to ensure that the migration - // function doesn't mutate the bbolt store sessions. - // We can then compare them to the sql sessions after - // the migration has been executed. - kvSessions, err := kvStore.ListAllSessions(ctx) - require.NoError(t, err) + kvSessions := test.populateDB(t, kvStore, accountStore) // Proceed to create the sql store and execute the // migration. @@ -392,7 +409,7 @@ func TestSessionsStoreMigration(t *testing.T) { // them will contain up to 10 linked sessions. The rest of the session will have // the rest of the session options randomized. func randomizedSessions(t *testing.T, kvStore *BoltStore, - accountsStore accounts.Store) { + accountsStore accounts.Store) []*Session { ctx := context.Background() @@ -547,6 +564,8 @@ func randomizedSessions(t *testing.T, kvStore *BoltStore, err = shiftStateUnsafe(kvStore, activeSess.ID, lastState(i)) require.NoError(t, err) } + + return getBoltStoreSessions(t, kvStore) } // macaroonType returns a macaroon type based on the given index by taking the @@ -741,6 +760,16 @@ func randomString(n int) string { return string(b) } +// getBoltStoreSessions is a helper function that fetches all sessions +// from the kv store, while already asserting that there no error occurs +// when retrieving the sessions. +func getBoltStoreSessions(t *testing.T, db *BoltStore) []*Session { + kvSessions, err := getBBoltSessions(db.DB) + require.NoError(t, err) + + return kvSessions +} + // shiftStateUnsafe updates the state of the session with the given ID to the // "dest" state, without checking if the state transition is legal. // From dfe0fa8b07fb50f0bfb1ca05e4ad7ace4d9a59fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Viktor=20Tigerstr=C3=B6m?= Date: Tue, 19 Aug 2025 01:35:02 +0200 Subject: [PATCH 2/2] session: handle multiple sessions with the same ID The logic to avoid duplicate session IDs was first added with the session linking linking functionality. That means that there may be old legacy sessions that were added prior to that which may have duplicate IDs. Before this commit, such sessions would cause the kvdb to SQL migration to error, as there is a uniqueness constraint on the legacy session alias in the SQL sessions table. We want to keep that constraint, so in such rare cases, we update the kvdb to SQL migration logic to drop all but the session with the latest CreatedAt time if for sessions that share the same ID. That follows the logic in c8b78bd10dd5fee9cb250edc91229aa05d824877 as closely as possible, as that migration ensured that all sessions but the latest one was revoked if multiple sessions shared the same ID. --- session/sql_migration.go | 92 ++++++++++++++++----- session/sql_migration_test.go | 150 ++++++++++++++++++++++++++++++++++ 2 files changed, 220 insertions(+), 22 deletions(-) diff --git a/session/sql_migration.go b/session/sql_migration.go index 428cc0fce..593ceac3b 100644 --- a/session/sql_migration.go +++ b/session/sql_migration.go @@ -40,35 +40,16 @@ func MigrateSessionStoreToSQL(ctx context.Context, kvStore *bbolt.DB, return err } - // If sessions are linked to a group, we must insert the initial session - // of each group before the other sessions in that group. This ensures - // we can retrieve the SQL group ID when inserting the remaining - // sessions. Therefore, we first insert all initial group sessions, - // allowing us to fetch the group IDs and insert the rest of the - // sessions afterward. - // We therefore filter out the initial sessions first, and then migrate - // them prior to the rest of the sessions. - var ( - initialGroupSessions []*Session - linkedSessions []*Session - ) - - for _, kvSession := range kvSessions { - if kvSession.GroupID == kvSession.ID { - initialGroupSessions = append( - initialGroupSessions, kvSession, - ) - } else { - linkedSessions = append(linkedSessions, kvSession) - } - } + initialGroupSessions, linkedSessions := filterSessions(kvSessions) + // Migrate the non-linked sessions first. err = migrateSessionsToSQLAndValidate(ctx, tx, initialGroupSessions) if err != nil { return fmt.Errorf("migration of non-linked session failed: %w", err) } + // Then migrate the linked sessions. err = migrateSessionsToSQLAndValidate(ctx, tx, linkedSessions) if err != nil { return fmt.Errorf("migration of linked session failed: %w", err) @@ -81,6 +62,73 @@ func MigrateSessionStoreToSQL(ctx context.Context, kvStore *bbolt.DB, return nil } +// filterSessions categorizes the sessions into two groups: initial group +// sessions and linked sessions. The initial group sessions are the first +// sessions in a session group, while the linked sessions are those that have a +// linked parent session. These are separated to ensure that we can insert the +// initial group sessions first, which allows us to fetch the SQL group ID when +// inserting the rest of the linked sessions afterward. +// +// Additionally, it checks for duplicate session IDs and drops all but +// one session with the same ID, keeping the one with the latest CreatedAt +// timestamp. Note that users with duplicate session IDs should be extremely +// rare, as it could only occur if colliding session IDs were created prior to +// the introduction of the session linking functionality. +func filterSessions(kvSessions []*Session) ([]*Session, []*Session) { + // First map sessions by their ID. + sessionsByID := make(map[ID][]*Session) + for _, s := range kvSessions { + sessionsByID[s.ID] = append(sessionsByID[s.ID], s) + } + + var ( + initialGroupSessions []*Session + linkedSessions []*Session + ) + + // Process the mapped sessions. If there are duplicate sessions with the + // same ID, we will only iterate the session with the latest CreatedAt + // timestamp, and drop the other sessions. This is to ensure that we can + // keep a UNIQUE constraint for the session ID (alias) in the SQL db. + for id, sessions := range sessionsByID { + sessionToKeep := sessions[0] + if len(sessions) > 1 { + log.Warnf("Found %d sessions with duplicate ID %x, "+ + "keeping only the latest one", len(sessions), + id) + + // Find the session with the latest timestamp. + latestSession := sessions[0] + for _, s := range sessions[1:] { + if s.CreatedAt.After(latestSession.CreatedAt) { + latestSession = s + } + } + sessionToKeep = latestSession + + // Log the sessions that will be dropped. + for _, s := range sessions { + if s == sessionToKeep { + continue + } + log.Warnf("Dropping duplicate session with ID "+ + "%x created at %v", id, s.CreatedAt) + } + } + + // Categorize the session that we are keeping. + if sessionToKeep.GroupID == sessionToKeep.ID { + initialGroupSessions = append( + initialGroupSessions, sessionToKeep, + ) + } else { + linkedSessions = append(linkedSessions, sessionToKeep) + } + } + + return initialGroupSessions, linkedSessions +} + // getBBoltSessions is a helper function that fetches all sessions from the // Bbolt store, by iterating directly over the buckets, without needing to // use any public functions of the BoltStore struct. diff --git a/session/sql_migration_test.go b/session/sql_migration_test.go index 77e003fed..d9244b9e0 100644 --- a/session/sql_migration_test.go +++ b/session/sql_migration_test.go @@ -352,6 +352,115 @@ func TestSessionsStoreMigration(t *testing.T) { return getBoltStoreSessions(t, store) }, }, + { + name: "multiple sessions with the same ID", + populateDB: func(t *testing.T, store *BoltStore, + _ accounts.Store) []*Session { + + // We first add one session which has no other + // session with same ID, to test that this is + // correctly migrated, and included in the + // migration result. + sess1, err := store.NewSession( + ctx, "session1", TypeMacaroonAdmin, + time.Unix(1000, 0), "", + ) + require.NoError(t, err) + + sess2, err := store.NewSession( + ctx, "session2", TypeMacaroonAdmin, + time.Unix(1000, 0), "", + ) + require.NoError(t, err) + + // Then add two sessions with the same ID, to + // test that only the latest session is included + // in the migration result. + sess3, err := store.NewSession( + ctx, "session3", TypeMacaroonAdmin, + time.Unix(1000, 0), "", + ) + require.NoError(t, err) + + // During the addition of the session linking + // functionality, logic was added in the + // NewSession function to ensure we can't create + // multiple sessions with the same ID. Therefore + // we need to manually override the ID of + // the second session to match the first + // session, to simulate such a scenario that + // could occur prior to the addition of that + // logic. + // We also need to update the CreatedAt time + // as the execution of this function is too + // fast for the CreatedAt time of sess2 and + // sess3 to differ. + err = updateSessionIDAndCreatedAt( + store, sess3.ID, sess2.MacaroonRootKey, + sess2.CreatedAt.Add(time.Minute), + ) + require.NoError(t, err) + + // Finally, we add three sessions with the same + // ID, to test we can handle more than two + // sessions with the same ID. + sess4, err := store.NewSession( + ctx, "session4", TypeMacaroonAdmin, + time.Unix(1000, 0), "", + ) + require.NoError(t, err) + + sess5, err := store.NewSession( + ctx, "session5", TypeMacaroonAdmin, + time.Unix(1000, 0), "", + ) + require.NoError(t, err) + + sess6, err := store.NewSession( + ctx, "session6", TypeMacaroonAdmin, + time.Unix(1000, 0), "", + ) + require.NoError(t, err) + + err = updateSessionIDAndCreatedAt( + store, sess5.ID, sess4.MacaroonRootKey, + sess4.CreatedAt.Add(time.Minute), + ) + require.NoError(t, err) + + err = updateSessionIDAndCreatedAt( + store, sess6.ID, sess4.MacaroonRootKey, + sess4.CreatedAt.Add(time.Minute*2), + ) + require.NoError(t, err) + + // Now fetch the updated sessions from the kv + // store, so that we are sure that the new IDs + // have really been persisted in the DB. + kvSessions := getBoltStoreSessions(t, store) + require.Len(t, kvSessions, 6) + + getSessionByName := func(name string) *Session { + for _, session := range kvSessions { + if session.Label == name { + return session + } + } + + t.Fatalf("session %s not found", name) + return nil + } + + // When multiple sessions with the same ID + // exist, we expect only the session with the + // latest creation time to be migrated. + return []*Session{ + getSessionByName(sess1.Label), + getSessionByName(sess3.Label), + getSessionByName(sess6.Label), + } + }, + }, { name: "randomized sessions", populateDB: randomizedSessions, @@ -803,3 +912,44 @@ func shiftStateUnsafe(db *BoltStore, id ID, dest State) error { return putSession(sessionBucket, session) }) } + +// updateSessionIDAndCreatedAt can be used to update the ID, the GroupID, +// the MacaroonRootKey and the CreatedAt time a session in the BoltStore. +// +// NOTE: this function should only be used for testing purposes. Also note that +// we pass the macaroon root key to set the new session ID, as the +// DeserializeSession function derives the session ID from the +// session.MacaroonRootKey. +func updateSessionIDAndCreatedAt(db *BoltStore, oldID ID, newIdRootKey uint64, + newCreatedAt time.Time) error { + + newId := IDFromMacRootKeyID(newIdRootKey) + + if oldID == newId { + return fmt.Errorf("can't update session ID to the same ID: %s", + oldID) + } + + return db.Update(func(tx *bbolt.Tx) error { + // Get the main session bucket. + sessionBkt, err := getBucket(tx, sessionBucketKey) + if err != nil { + return err + } + + // Look up the session using the old ID. + sess, err := getSessionByID(sessionBkt, oldID) + if err != nil { + return err + } + + // Update the session. + sess.ID = newId + sess.GroupID = newId + sess.MacaroonRootKey = newIdRootKey + sess.CreatedAt = newCreatedAt + + // Write it back under the same key (local pubkey). + return putSession(sessionBkt, sess) + }) +}