Merge pull request #985 from ellemouton/stateShift

sessions: strict state shifts

Author: ellemouton

docs/release-notes/release-notes-0.14.2.md

@@ -0,0 +1,45 @@
+# Release Notes
+
+- [Lightning Terminal](#lightning-terminal)
+    - [Bug Fixes](#bug-fixes)
+    - [Functional Changes/Additions](#functional-changesadditions)
+    - [Technical and Architectural Updates](#technical-and-architectural-updates)
+- [Integrated Binary Updates](#integrated-binary-updates)
+    - [LND](#lnd)
+    - [Loop](#loop)
+    - [Pool](#pool)
+    - [Faraday](#faraday)
+    - [Taproot Assets](#taproot-assets)
+- [Contributors](#contributors-alphabetical-order)
+
+## Lightning Terminal
+
+### Bug Fixes
+
+### Functional Changes/Additions
+
+### Technical and Architectural Updates
+
+* Correctly [move a session to the Expired 
+  state](https://github.com/lightninglabs/lightning-terminal/pull/985) instead
+  of the Revoked state if it is in fact being revoked due to the session expiry
+  being reached.
+
+
+## RPC Updates
+
+## Integrated Binary Updates
+
+### LND
+
+### Loop
+
+### Pool
+
+### Faraday
+
+### Taproot Assets
+
+# Contributors (Alphabetical Order)
+
+* Elle Mouton

session/interface.go

@@ -27,9 +27,9 @@ const (
 type State uint8
 
 /*
-		/---> StateExpired
+		/---> StateExpired (terminal)
 StateCreated ---
-       		\---> StateRevoked
+       		\---> StateRevoked (terminal)
 */
 
 const (
@@ -59,6 +59,24 @@ const (
 	StateReserved State = 4
 )
 
+// Terminal returns true if the state is a terminal state.
+func (s State) Terminal() bool {
+	return s == StateExpired || s == StateRevoked
+}
+
+// legalStateShifts is a map that defines the legal State transitions that a
+// Session can be put through.
+var legalStateShifts = map[State]map[State]bool{
+	StateCreated: {
+		StateExpired: true,
+		StateRevoked: true,
+	},
+	StateInUse: {
+		StateRevoked: true,
+		StateExpired: true,
+	},
+}
+
 // MacaroonRecipe defines the permissions and caveats that should be used
 // to bake a macaroon.
 type MacaroonRecipe struct {
@@ -197,10 +215,6 @@ type Store interface {
 	// that are in the given states.
 	ListSessionsByState(...State) ([]*Session, error)
 
-	// RevokeSession updates the state of the session with the given local
-	// public key to be revoked.
-	RevokeSession(*btcec.PublicKey) error
-
 	// UpdateSessionRemotePubKey can be used to add the given remote pub key
 	// to the session with the given local pub key.
 	UpdateSessionRemotePubKey(localPubKey,
@@ -225,5 +239,9 @@ type Store interface {
 	// StateReserved state.
 	DeleteReservedSessions() error
 
+	// ShiftState updates the state of the session with the given ID to the
+	// "dest" state.
+	ShiftState(id ID, dest State) error
+
 	IDToGroupIndex
 }

session/kvdb_store.go

@@ -514,30 +514,42 @@ func (db *BoltStore) DeleteReservedSessions() error {
 	})
 }
 
-// RevokeSession updates the state of the session with the given local
-// public key to be revoked.
+// ShiftState updates the state of the session with the given ID to the "dest"
+// state.
 //
 // NOTE: this is part of the Store interface.
-func (db *BoltStore) RevokeSession(key *btcec.PublicKey) error {
-	var session *Session
+func (db *BoltStore) ShiftState(id ID, dest State) error {
 	return db.Update(func(tx *bbolt.Tx) error {
 		sessionBucket, err := getBucket(tx, sessionBucketKey)
 		if err != nil {
 			return err
 		}
 
-		sessionBytes := sessionBucket.Get(key.SerializeCompressed())
-		if len(sessionBytes) == 0 {
-			return ErrSessionNotFound
-		}
-
-		session, err = DeserializeSession(bytes.NewReader(sessionBytes))
+		session, err := getSessionByID(sessionBucket, id)
 		if err != nil {
 			return err
 		}
 
-		session.State = StateRevoked
-		session.RevokedAt = db.clock.Now().UTC()
+		// If the session is already in the desired state, we return
+		// with no error to maintain idempotency.
+		if session.State == dest {
+			return nil
+		}
+
+		// Ensure that the wanted state change is allowed.
+		allowedDestinations, ok := legalStateShifts[session.State]
+		if !ok || !allowedDestinations[dest] {
+			return fmt.Errorf("illegal session state transition "+
+				"from %d to %d", session.State, dest)
+		}
+
+		session.State = dest
+
+		// If the session is terminal, we set the revoked at time to the
+		// current time.
+		if dest.Terminal() {
+			session.RevokedAt = db.clock.Now().UTC()
+		}
 
 		return putSession(sessionBucket, session)
 	})

session/store_test.go

@@ -106,7 +106,7 @@ func TestBasicSessionStore(t *testing.T) {
 	require.Equal(t, session1.State, StateCreated)
 
 	// Now revoke the session and assert that the state is revoked.
-	require.NoError(t, db.RevokeSession(s1.LocalPublicKey))
+	require.NoError(t, db.ShiftState(s1.ID, StateRevoked))
 	s1, err = db.GetSession(s1.LocalPublicKey)
 	require.NoError(t, err)
 	require.Equal(t, s1.State, StateRevoked)
@@ -225,7 +225,7 @@ func TestLinkingSessions(t *testing.T) {
 	require.ErrorContains(t, db.CreateSession(s2), "is still active")
 
 	// Revoke the first session.
-	require.NoError(t, db.RevokeSession(s1.LocalPublicKey))
+	require.NoError(t, db.ShiftState(s1.ID, StateRevoked))
 
 	// Persisting the second linked session should now work.
 	require.NoError(t, db.CreateSession(s2))
@@ -248,16 +248,20 @@ func TestLinkedSessions(t *testing.T) {
 	// the same group. The group ID is equivalent to the session ID of the
 	// first session.
 	s1 := newSession(t, db, clock, "session 1")
-	s2 := newSession(t, db, clock, "session 2", withLinkedGroupID(&s1.GroupID))
-	s3 := newSession(t, db, clock, "session 3", withLinkedGroupID(&s2.GroupID))
+	s2 := newSession(
+		t, db, clock, "session 2", withLinkedGroupID(&s1.GroupID),
+	)
+	s3 := newSession(
+		t, db, clock, "session 3", withLinkedGroupID(&s2.GroupID),
+	)
 
 	// Persist the sessions.
 	require.NoError(t, db.CreateSession(s1))
 
-	require.NoError(t, db.RevokeSession(s1.LocalPublicKey))
+	require.NoError(t, db.ShiftState(s1.ID, StateRevoked))
 	require.NoError(t, db.CreateSession(s2))
 
-	require.NoError(t, db.RevokeSession(s2.LocalPublicKey))
+	require.NoError(t, db.ShiftState(s2.ID, StateRevoked))
 	require.NoError(t, db.CreateSession(s3))
 
 	// Assert that the session ID to group ID index works as expected.
@@ -282,7 +286,7 @@ func TestLinkedSessions(t *testing.T) {
 
 	// Persist the sessions.
 	require.NoError(t, db.CreateSession(s4))
-	require.NoError(t, db.RevokeSession(s4.LocalPublicKey))
+	require.NoError(t, db.ShiftState(s4.ID, StateRevoked))
 
 	require.NoError(t, db.CreateSession(s5))
 
@@ -337,7 +341,7 @@ func TestCheckSessionGroupPredicate(t *testing.T) {
 	require.False(t, ok)
 
 	// Revoke the first session.
-	require.NoError(t, db.RevokeSession(s1.LocalPublicKey))
+	require.NoError(t, db.ShiftState(s1.ID, StateRevoked))
 
 	// Add a new session to the same group as the first one.
 	s2 := newSession(t, db, clock, "label 2", withLinkedGroupID(&s1.GroupID))
@@ -392,6 +396,53 @@ func TestCheckSessionGroupPredicate(t *testing.T) {
 	require.True(t, ok)
 }
 
+// TestStateShift tests that the ShiftState method works as expected.
+func TestStateShift(t *testing.T) {
+	// Set up a new DB.
+	clock := clock.NewTestClock(testTime)
+	db, err := NewDB(t.TempDir(), "test.db", clock)
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		_ = db.Close()
+	})
+
+	// Add a new session to the DB.
+	s1 := newSession(t, db, clock, "label 1")
+	require.NoError(t, db.CreateSession(s1))
+
+	// Check that the session is in the StateCreated state. Also check that
+	// the "RevokedAt" time has not yet been set.
+	s1, err = db.GetSession(s1.LocalPublicKey)
+	require.NoError(t, err)
+	require.Equal(t, StateCreated, s1.State)
+	require.Equal(t, time.Time{}, s1.RevokedAt)
+
+	// Shift the state of the session to StateRevoked.
+	err = db.ShiftState(s1.ID, StateRevoked)
+	require.NoError(t, err)
+
+	// This should have worked. Since it is now in a terminal state, the
+	// "RevokedAt" time should be set.
+	s1, err = db.GetSession(s1.LocalPublicKey)
+	require.NoError(t, err)
+	require.Equal(t, StateRevoked, s1.State)
+	require.True(t, clock.Now().Equal(s1.RevokedAt))
+
+	// Trying to do the same state shift again should succeed since the
+	// session is already in the expected "dest" state. The revoked-at time
+	// should not have changed though.
+	prevTime := clock.Now()
+	clock.SetTime(prevTime.Add(time.Second))
+	err = db.ShiftState(s1.ID, StateRevoked)
+	require.NoError(t, err)
+	require.True(t, prevTime.Equal(s1.RevokedAt))
+
+	// Trying to shift the state from a terminal state back to StateCreated
+	// should also fail since this is not a legal state transition.
+	err = db.ShiftState(s1.ID, StateCreated)
+	require.ErrorContains(t, err, "illegal session state transition")
+}
+
 // testSessionModifier is a functional option that can be used to modify the
 // default test session created by newSession.
 type testSessionModifier func(*Session)

session_rpcserver.go

@@ -154,8 +154,8 @@ func (s *sessionRpcServer) start(ctx context.Context) error {
 					err)
 
 				if perm {
-					err := s.cfg.db.RevokeSession(
-						sess.LocalPublicKey,
+					err := s.cfg.db.ShiftState(
+						sess.ID, session.StateRevoked,
 					)
 					if err != nil {
 						log.Errorf("error revoking "+
@@ -360,7 +360,8 @@ func (s *sessionRpcServer) resumeSession(ctx context.Context,
 		log.Debugf("Not resuming session %x with expiry %s",
 			pubKeyBytes, sess.Expiry)
 
-		if err := s.cfg.db.RevokeSession(pubKey); err != nil {
+		err := s.cfg.db.ShiftState(sess.ID, session.StateExpired)
+		if err != nil {
 			return fmt.Errorf("error revoking session: %v", err)
 		}
 
@@ -436,7 +437,9 @@ func (s *sessionRpcServer) resumeSession(ctx context.Context,
 			log.Debugf("Deadline for session %x has already "+
 				"passed. Revoking session", pubKeyBytes)
 
-			return s.cfg.db.RevokeSession(pubKey)
+			return s.cfg.db.ShiftState(
+				sess.ID, session.StateRevoked,
+			)
 		}
 
 		// Start the deadline timer.
@@ -515,7 +518,7 @@ func (s *sessionRpcServer) resumeSession(ctx context.Context,
 			log.Debugf("Error stopping session: %v", err)
 		}
 
-		err = s.cfg.db.RevokeSession(pubKey)
+		err = s.cfg.db.ShiftState(sess.ID, session.StateRevoked)
 		if err != nil {
 			log.Debugf("error revoking session: %v", err)
 		}
@@ -557,7 +560,13 @@ func (s *sessionRpcServer) RevokeSession(ctx context.Context,
 		return nil, fmt.Errorf("error parsing public key: %v", err)
 	}
 
-	if err := s.cfg.db.RevokeSession(pubKey); err != nil {
+	sess, err := s.cfg.db.GetSession(pubKey)
+	if err != nil {
+		return nil, fmt.Errorf("error fetching session: %v", err)
+	}
+
+	err = s.cfg.db.ShiftState(sess.ID, session.StateRevoked)
+	if err != nil {
 		return nil, fmt.Errorf("error revoking session: %v", err)
 	}