Merge pull request #998 from TowyTowy/fix/plugin-messenger-application-id

fix: create internal application when plugin adds Messenger after init

Author: eternal-flame-AD

plugin/manager.go

@@ -323,6 +323,20 @@ func (m *Manager) initializeSingleUserPlugin(userCtx compat.UserContext, p compa
 	m.instances[pluginConf.ID] = instance
 
 	if compat.HasSupport(instance, compat.Messenger) {
+		if pluginConf.ApplicationID == 0 {
+			// The Messenger capability was added after this plugin was first
+			// initialized for the user, so no internal application exists yet.
+			// Create one now, otherwise messages would be stored with
+			// application_id = 0 and become orphaned (not shown, not deletable).
+			app, err := m.createInternalApplication(info, userID)
+			if err != nil {
+				return err
+			}
+			pluginConf.ApplicationID = app.ID
+			if err := m.db.UpdatePluginConf(pluginConf); err != nil {
+				return err
+			}
+		}
 		instance.SetMessageHandler(redirectToChannel{
 			ApplicationID: pluginConf.ApplicationID,
 			UserID:        pluginConf.UserID,
@@ -399,15 +413,8 @@ func (m *Manager) createPluginConf(instance compat.PluginInstance, info compat.I
 		pluginConf.Config, _ = yaml.Marshal(instance.DefaultConfig())
 	}
 	if compat.HasSupport(instance, compat.Messenger) {
-		tokenPublic, _ := auth.GenerateApplicationToken()
-		app := &model.Application{
-			Token:       tokenPublic,
-			Name:        info.String(),
-			UserID:      userID,
-			Internal:    true,
-			Description: fmt.Sprintf("auto generated application for %s", info.ModulePath),
-		}
-		if err := m.db.CreateApplication(app); err != nil {
+		app, err := m.createInternalApplication(info, userID)
+		if err != nil {
 			return nil, err
 		}
 		pluginConf.ApplicationID = app.ID
@@ -417,3 +424,20 @@ func (m *Manager) createPluginConf(instance compat.PluginInstance, info compat.I
 	}
 	return pluginConf, nil
 }
+
+// createInternalApplication creates the auto generated internal application a
+// Messenger plugin uses to publish its messages.
+func (m *Manager) createInternalApplication(info compat.Info, userID uint) (*model.Application, error) {
+	tokenPublic, _ := auth.GenerateApplicationToken()
+	app := &model.Application{
+		Token:       tokenPublic,
+		Name:        info.String(),
+		UserID:      userID,
+		Internal:    true,
+		Description: fmt.Sprintf("auto generated application for %s", info.ModulePath),
+	}
+	if err := m.db.CreateApplication(app); err != nil {
+		return nil, err
+	}
+	return app, nil
+}

plugin/manager_test.go

@@ -443,6 +443,105 @@ func TestNewManager_InternalApplicationManagement(t *testing.T) {
 	}
 }
 
+func TestNewManager_MessengerAddedAfterInit_createsApplication(t *testing.T) {
+	db := testdb.NewDBWithDefaultUser(t)
+
+	// Simulate a plugin conf that was created on a previous startup when the
+	// plugin did not yet implement the Messenger interface: it has no
+	// associated internal application (ApplicationID == 0).
+	assert.NoError(t, db.CreatePluginConf(&model.PluginConf{
+		UserID:     1,
+		ModulePath: mock.ModulePath,
+		Enabled:    true,
+		Token:      auth.GeneratePluginToken(),
+	}))
+
+	manager, err := NewManager(db, "", nil, nil)
+	assert.Nil(t, err)
+	assert.Nil(t, manager.LoadPlugin(new(mock.Plugin)))
+	// The mock plugin supports Messenger, so re-initializing must back-fill the
+	// missing internal application instead of leaving ApplicationID at 0.
+	assert.Nil(t, manager.InitializeForUserID(1))
+
+	conf, err := db.GetPluginConfByUserAndPath(1, mock.ModulePath)
+	assert.NoError(t, err)
+	if assert.NotNil(t, conf) {
+		assert.NotZero(t, conf.ApplicationID, "an internal application should have been created for the messenger plugin")
+
+		app, err := db.GetApplicationByID(conf.ApplicationID)
+		assert.NoError(t, err)
+		if assert.NotNil(t, app) {
+			assert.True(t, app.Internal)
+			assert.Equal(t, uint(1), app.UserID)
+		}
+	}
+}
+
+// failingDB wraps a real test database so that individual operations can be
+// forced to fail, allowing the error branches around internal-application
+// creation to be exercised.
+type failingDB struct {
+	*testdb.Database
+	failCreateApplication error
+	failUpdatePluginConf  error
+}
+
+func (d *failingDB) CreateApplication(app *model.Application) error {
+	if d.failCreateApplication != nil {
+		return d.failCreateApplication
+	}
+	return d.Database.CreateApplication(app)
+}
+
+func (d *failingDB) UpdatePluginConf(conf *model.PluginConf) error {
+	if d.failUpdatePluginConf != nil {
+		return d.failUpdatePluginConf
+	}
+	return d.Database.UpdatePluginConf(conf)
+}
+
+func seedMessengerConfWithoutApplication(t *testing.T, db Database) {
+	t.Helper()
+	assert.NoError(t, db.CreatePluginConf(&model.PluginConf{
+		UserID:     1,
+		ModulePath: mock.ModulePath,
+		Enabled:    true,
+		Token:      auth.GeneratePluginToken(),
+	}))
+}
+
+func TestNewManager_MessengerAddedAfterInit_createApplicationError(t *testing.T) {
+	db := &failingDB{
+		Database:              testdb.NewDBWithDefaultUser(t),
+		failCreateApplication: errors.New("create application failed"),
+	}
+	seedMessengerConfWithoutApplication(t, db)
+
+	manager, err := NewManager(db, "", nil, nil)
+	assert.Nil(t, err)
+	assert.Nil(t, manager.LoadPlugin(new(mock.Plugin)))
+
+	// Back-filling the missing internal application must surface the database
+	// error instead of silently continuing with ApplicationID == 0.
+	assert.EqualError(t, manager.InitializeForUserID(1), "create application failed")
+}
+
+func TestNewManager_MessengerAddedAfterInit_updatePluginConfError(t *testing.T) {
+	db := &failingDB{
+		Database:             testdb.NewDBWithDefaultUser(t),
+		failUpdatePluginConf: errors.New("update plugin conf failed"),
+	}
+	seedMessengerConfWithoutApplication(t, db)
+
+	manager, err := NewManager(db, "", nil, nil)
+	assert.Nil(t, err)
+	assert.Nil(t, manager.LoadPlugin(new(mock.Plugin)))
+
+	// Persisting the back-filled ApplicationID may fail; that error must be
+	// propagated as well.
+	assert.EqualError(t, manager.InitializeForUserID(1), "update plugin conf failed")
+}
+
 func TestPluginFileLoadError(t *testing.T) {
 	err := pluginFileLoadError{Filename: "test.so", UnderlyingError: errors.New("test error")}
 	assert.Error(t, err)

plugin/messagehandler.go

@@ -1,6 +1,7 @@
 package plugin
 
 import (
+	"errors"
 	"time"
 
 	"github.com/gotify/server/v2/model"
@@ -21,6 +22,13 @@ type MessageWithUserID struct {
 
 // SendMessage sends a message to the underlying message channel.
 func (c redirectToChannel) SendMessage(msg compat.Message) error {
+	if c.ApplicationID == 0 {
+		// Final safety net: the internal application should always be set up by
+		// Manager.initializeSingleUserPlugin. If it somehow isn't, refuse the
+		// message instead of storing it with application_id = 0, where it would
+		// be orphaned (not shown in the UI and not deletable).
+		return errors.New("plugin messenger has no associated internal application")
+	}
 	c.Messages <- MessageWithUserID{
 		Message: model.MessageExternal{
 			ApplicationID: c.ApplicationID,

plugin/messagehandler_test.go

@@ -0,0 +1,31 @@
+package plugin
+
+import (
+	"testing"
+
+	"github.com/gotify/server/v2/plugin/compat"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestRedirectToChannel_SendMessage_rejectsMissingApplication(t *testing.T) {
+	messages := make(chan MessageWithUserID, 1)
+	handler := redirectToChannel{ApplicationID: 0, UserID: 1, Messages: messages}
+
+	err := handler.SendMessage(compat.Message{Message: "orphan"})
+
+	assert.Error(t, err)
+	assert.Empty(t, messages, "no message should be queued when the internal application is missing")
+}
+
+func TestRedirectToChannel_SendMessage_forwardsWithApplication(t *testing.T) {
+	messages := make(chan MessageWithUserID, 1)
+	handler := redirectToChannel{ApplicationID: 7, UserID: 3, Messages: messages}
+
+	assert.NoError(t, handler.SendMessage(compat.Message{Message: "hi", Title: "t"}))
+
+	got := <-messages
+	assert.Equal(t, uint(7), got.Message.ApplicationID)
+	assert.Equal(t, uint(3), got.UserID)
+	assert.Equal(t, "hi", got.Message.Message)
+	assert.Equal(t, "t", got.Message.Title)
+}