EEBus: keep pairing requests pending while devices are being configured (#33046)

Author: andig

server/eebus/eebus.go

@@ -87,6 +87,9 @@ type EEBus struct {
 	// unknown ski may still belong to a device that is not configured yet.
 	configured bool
 
+	// pending contains pairing requests received while still configuring
+	pending map[string]shipapi.ServiceIdentity
+
 	ski string
 
 	paired    []shipapi.ServiceIdentity // devices paired via SHIP Pairing Service
@@ -122,8 +125,21 @@ func ConfigComplete() {
 	}
 
 	instance.mux.Lock()
-	defer instance.mux.Unlock()
 	instance.configured = true
+
+	// deny requests left pending during configuration whose ski remained unknown
+	var deny []shipapi.ServiceIdentity
+	for _, identity := range instance.pending {
+		if len(instance.clients[identity.SKI]) == 0 {
+			deny = append(deny, identity)
+		}
+	}
+	clear(instance.pending)
+	instance.mux.Unlock()
+
+	for _, identity := range deny {
+		instance.service.CancelPairing(identity)
+	}
 }
 
 func GetStatus() any {
@@ -207,10 +223,16 @@ func NewServer(other Config) (*EEBus, error) {
 		ski:       ski,
 		clients:   make(map[string][]Device),
 		connected: make(map[string]bool),
+		pending:   make(map[string]shipapi.ServiceIdentity),
 	}
 
 	c.service = service.NewService(configuration, c)
 	c.service.SetLogging(c)
+
+	// keep pairing requests from unknown skis pending instead of aborting the ship
+	// handshake- the ski may still be registered by a device that is being configured
+	c.service.UserIsAbleToApproveOrCancelPairingRequests(true)
+
 	if err := c.service.Setup(); err != nil {
 		if errors.Is(err, shipapi.ErrInvalidSKI) {
 			const hint = "The stored EEBUS certificate has an invalid Subject Key Identifier (SKI).\n" +
@@ -587,6 +609,7 @@ func (c *EEBus) ServicePairingDetailUpdate(identity shipapi.ServiceIdentity, det
 			// device configuration is still running- leave the request pending
 			// instead of denying a ski that is about to be registered
 			c.log.DEBUG.Printf("pairing request from %s while configuring, left pending", identity.SKI)
+			c.pending[identity.SKI] = identity
 			return
 		}
 
@@ -601,16 +624,22 @@ func (c *EEBus) ServiceAutoTrusted(service eebusapi.ServiceInterface, identity s
 	c.log.INFO.Printf("service trusted: %s", identity.ShipID)
 
 	c.mux.Lock()
-	defer c.mux.Unlock()
 	c.upsertPairing(identity)
 
 	// connect may run before trust is established, so clientsFor skips consumers
 	// registered without ski; wake them now that the device is paired
+	var clients []Device
 	if c.connected[identity.SKI] {
-		for _, client := range c.clientsFor(identity.SKI) {
-			client.Connect(true)
-		}
+		clients = c.clientsFor(identity.SKI)
 	}
+	c.mux.Unlock()
+
+	for _, client := range clients {
+		client.Connect(true)
+	}
+
+	// registering the now trusted identity approves a handshake still pending trust
+	c.service.RegisterRemoteService(identity)
 }
 
 func (c *EEBus) ServiceAutoTrustFailed(service eebusapi.ServiceInterface, identity shipapi.ServiceIdentity, reason error) {

server/eebus/eebus_test.go

@@ -88,13 +88,51 @@ func TestPairingDeniedOnlyWhenConfigured(t *testing.T) {
 	c := &EEBus{
 		log:     util.NewLogger("test"),
 		clients: make(map[string][]Device),
+		pending: make(map[string]shipapi.ServiceIdentity),
 		service: service,
 	}
 
 	// still configuring - no CancelPairing expected
 	c.ServicePairingDetailUpdate(identity, detail)
+	require.Len(t, c.pending, 1)
 
 	service.EXPECT().CancelPairing(identity).Once()
 	c.configured = true
 	c.ServicePairingDetailUpdate(identity, detail)
 }
+
+// TestConfigCompleteResolvesPending guards that a request left pending during
+// configuration is denied afterwards unless its ski belongs to a configured device-
+// ship-go keeps prolonging the pending handshake until somebody decides.
+func TestConfigCompleteResolvesPending(t *testing.T) {
+	identity := shipapi.NewServiceIdentity("aabbcc", "", "")
+
+	newInstance := func(t *testing.T, clients map[string][]Device) *eebusmocks.ServiceInterface {
+		service := eebusmocks.NewServiceInterface(t)
+		instance = &EEBus{
+			log:     util.NewLogger("test"),
+			clients: clients,
+			pending: map[string]shipapi.ServiceIdentity{identity.SKI: identity},
+			service: service,
+		}
+		t.Cleanup(func() { instance = nil })
+		return service
+	}
+
+	t.Run("unknown ski denied", func(t *testing.T) {
+		service := newInstance(t, make(map[string][]Device))
+		service.EXPECT().CancelPairing(identity).Once()
+
+		ConfigComplete()
+		require.True(t, instance.configured)
+		require.Empty(t, instance.pending)
+	})
+
+	t.Run("configured ski kept", func(t *testing.T) {
+		// no CancelPairing expected- the device registered its ski meanwhile
+		newInstance(t, map[string][]Device{identity.SKI: {&mockDevice{}}})
+
+		ConfigComplete()
+		require.True(t, instance.configured)
+	})
+}