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 c8b78bd as
closely as possible, as that migration ensured that all sessions but the
latest one was revoked if multiple sessions shared the same ID.

Author: ViktorT-11

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.

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)
+	})
+}