Merge pull request #1133 from ViktorTigerstrom/2025-08-session-migration-duplicate-id-fix

[sql-50] session migration duplicate ID fix
This commit is contained in:
Elle 2025-09-02 16:20:37 +02:00 committed by GitHub
commit bce4c049a9
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 273 additions and 46 deletions

View file

@ -41,35 +41,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)
@ -82,6 +63,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.

View file

@ -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,117 @@ func TestSessionsStoreMigration(t *testing.T) {
WithLinkedGroupID(&sess1.ID),
)
require.NoError(t, err)
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),
}
},
},
{
@ -355,15 +489,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 +518,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 +673,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 +869,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.
//
@ -774,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)
})
}