fix: only send Forgejo Actions notifications to one user (#8227)

- If the run was attributed to a system user (which is the case for scheduled runs for instance), ignore it and fallback to the mail of the owner. System users may have email addresses, but they are not to be used. - If the owner is a system user or an organization with no email associated with it, do nothing. - If a user with an email exists, check if they did not disable notifications and send the email. Refs: https://codeberg.org/forgejo/forgejo/issues/8187 Refs: https://codeberg.org/forgejo/forgejo/issues/8233 ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests - I added test coverage for Go changes... - [x] in their respective `*_test.go` for unit tests. ### Documentation - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [x] I do not want this change to show in the release notes. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8227 Reviewed-by: Christopher Besch <mail@chris-besch.com> Co-authored-by: Earl Warren <contact@earl-warren.org> Co-committed-by: Earl Warren <contact@earl-warren.org>
2025-06-27 16:35:57 +00:00 · 2025-06-21 12:15:38 +02:00 · 2025-06-21 12:15:38 +02:00 · 9e6f722f94
commit 9e6f722f94
parent 25d596d387
5 changed files with 201 additions and 105 deletions
--- a/models/user/user_system.go
+++ b/models/user/user_system.go
@ -12,6 +12,13 @@ import (
 	"forgejo.org/modules/structs"
 )

+// IsSystem returns true if the user has a fixed
+// negative ID, is never stored in the database and
+// is generated on the fly when needed.
+func (u *User) IsSystem() bool {
+	return u.IsGhost() || u.IsActions()
+}
+
 const (
 	GhostUserID        = -1
 	GhostUserName      = "Ghost"
--- a/routers/web/repo/issue.go
+++ b/routers/web/repo/issue.go
@ -1308,7 +1308,7 @@ func roleDescriptor(ctx stdCtx.Context, repo *repo_model.Repository, poster *use
 	}

 	// Special user that can't have associated contributions and permissions in the repo.
-	if poster.IsGhost() || poster.IsActions() || poster.IsAPServerActor() {
+	if poster.IsSystem() || poster.IsAPServerActor() {
 		return roleDescriptor, nil
 	}

--- a/services/mailer/mail_actions.go
+++ b/services/mailer/mail_actions.go
@ -23,19 +23,20 @@ func MailActionRun(run *actions_model.ActionRun, priorStatus actions_model.Statu
 		return nil
 	}

-	if run.TriggerUser.Email != "" && run.TriggerUser.EmailNotificationsPreference != user_model.EmailNotificationsDisabled {
-		if err := sendMailActionRun(run.TriggerUser, run, priorStatus, lastRun); err != nil {
-			return err
-		}
+	user := run.TriggerUser
+	// this happens e.g. when this is a scheduled run
+	if user.IsSystem() {
+		user = run.Repo.Owner
+	}
+	if user.IsSystem() || user.Email == "" {
+		return nil
 	}

-	if run.Repo.Owner.Email != "" && run.Repo.Owner.Email != run.TriggerUser.Email && run.Repo.Owner.EmailNotificationsPreference != user_model.EmailNotificationsDisabled {
-		if err := sendMailActionRun(run.Repo.Owner, run, priorStatus, lastRun); err != nil {
-			return err
-		}
+	if user.EmailNotificationsPreference == user_model.EmailNotificationsDisabled {
+		return nil
 	}

-	return nil
+	return sendMailActionRun(user, run, priorStatus, lastRun)
 }

 func sendMailActionRun(to *user_model.User, run *actions_model.ActionRun, priorStatus actions_model.Status, lastRun *actions_model.ActionRun) error {
--- a/services/mailer/mail_actions_now_done_test.go
+++ b/services/mailer/mail_actions_now_done_test.go
@ -4,42 +4,53 @@
 package mailer

 import (
+	"slices"
 	"testing"

 	actions_model "forgejo.org/models/actions"
 	"forgejo.org/models/db"
+	organization_model "forgejo.org/models/organization"
 	repo_model "forgejo.org/models/repo"
 	user_model "forgejo.org/models/user"
+	"forgejo.org/modules/optional"
 	"forgejo.org/modules/setting"
+	"forgejo.org/modules/test"
 	notify_service "forgejo.org/services/notify"

 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )

-func getActionsNowDoneTestUsers(t *testing.T) []*user_model.User {
+func getActionsNowDoneTestUser(t *testing.T, name, email, notifications string) *user_model.User {
 	t.Helper()
-	newTriggerUser := new(user_model.User)
-	newTriggerUser.Name = "new_trigger_user"
-	newTriggerUser.Language = "en_US"
-	newTriggerUser.IsAdmin = false
-	newTriggerUser.Email = "new_trigger_user@example.com"
-	newTriggerUser.LastLoginUnix = 1693648327
-	newTriggerUser.CreatedUnix = 1693648027
-	newTriggerUser.EmailNotificationsPreference = user_model.EmailNotificationsEnabled
-	require.NoError(t, user_model.CreateUser(db.DefaultContext, newTriggerUser))
+	user := new(user_model.User)
+	user.Name = name
+	user.Language = "en_US"
+	user.IsAdmin = false
+	user.Email = email
+	user.LastLoginUnix = 1693648327
+	user.CreatedUnix = 1693648027
+	opts := user_model.CreateUserOverwriteOptions{
+		AllowCreateOrganization:      optional.Some(true),
+		EmailNotificationsPreference: &notifications,
+	}
+	require.NoError(t, user_model.AdminCreateUser(db.DefaultContext, user, &opts))
+	return user
+}

-	newOwner := new(user_model.User)
-	newOwner.Name = "new_owner"
-	newOwner.Language = "en_US"
-	newOwner.IsAdmin = false
-	newOwner.Email = "new_owner@example.com"
-	newOwner.LastLoginUnix = 1693648329
-	newOwner.CreatedUnix = 1693648029
-	newOwner.EmailNotificationsPreference = user_model.EmailNotificationsEnabled
-	require.NoError(t, user_model.CreateUser(db.DefaultContext, newOwner))
-
-	return []*user_model.User{newTriggerUser, newOwner}
+func getActionsNowDoneTestOrg(t *testing.T, name, email string, owner *user_model.User) *user_model.User {
+	t.Helper()
+	org := new(organization_model.Organization)
+	org.Name = name
+	org.Language = "en_US"
+	org.IsAdmin = false
+	// contact email for the organization, for display purposes but otherwise not used as of v12
+	org.Email = email
+	org.LastLoginUnix = 1693648327
+	org.CreatedUnix = 1693648027
+	org.Email = email
+	require.NoError(t, organization_model.CreateOrganization(db.DefaultContext, org, owner))
+	return (*user_model.User)(org)
 }

 func assertTranslatedLocaleMailActionsNowDone(t *testing.T, msgBody string) {
@ -49,98 +60,169 @@ func assertTranslatedLocaleMailActionsNowDone(t *testing.T, msgBody string) {
 func TestActionRunNowDoneNotificationMail(t *testing.T) {
 	ctx := t.Context()

-	users := getActionsNowDoneTestUsers(t)
-	defer CleanUpUsers(ctx, users)
-	triggerUser := users[0]
-	ownerUser := users[1]
+	defer test.MockVariableValue(&setting.Admin.DisableRegularOrgCreation, false)()
+
+	actionsUser := user_model.NewActionsUser()
+	require.NotEmpty(t, actionsUser.Email)

 	repo := repo_model.Repository{
 		Name:        "some repo",
 		Description: "rockets are cool",
-		Owner:       ownerUser,
-		OwnerID:     ownerUser.ID,
 	}

 	// Do some funky stuff with the action run's ids:
 	// The run with the larger ID finished first.
 	// This is odd but something that must work.
-	run1 := &actions_model.ActionRun{ID: 2, Repo: &repo, RepoID: repo.ID, Title: "some workflow", TriggerUser: triggerUser, TriggerUserID: triggerUser.ID, Status: actions_model.StatusFailure, Stopped: 1745821796, TriggerEvent: "workflow_dispatch"}
-	run2 := &actions_model.ActionRun{ID: 1, Repo: &repo, RepoID: repo.ID, Title: "some workflow", TriggerUser: triggerUser, TriggerUserID: triggerUser.ID, Status: actions_model.StatusSuccess, Stopped: 1745822796, TriggerEvent: "push"}
+	run1 := &actions_model.ActionRun{ID: 2, Repo: &repo, RepoID: repo.ID, Title: "some workflow", Status: actions_model.StatusFailure, Stopped: 1745821796, TriggerEvent: "workflow_dispatch"}
+	run2 := &actions_model.ActionRun{ID: 1, Repo: &repo, RepoID: repo.ID, Title: "some workflow", Status: actions_model.StatusSuccess, Stopped: 1745822796, TriggerEvent: "push"}
+
+	assignUsers := func(triggerUser, owner *user_model.User) {
+		for _, run := range []*actions_model.ActionRun{run1, run2} {
+			run.TriggerUser = triggerUser
+			run.TriggerUserID = triggerUser.ID
+		}
+		repo.Owner = owner
+		repo.OwnerID = owner.ID
+	}

 	notify_service.RegisterNotifier(NewNotifier())

+	orgOwner := getActionsNowDoneTestUser(t, "org_owner", "org_owner@example.com", "disabled")
+	defer CleanUpUsers(ctx, []*user_model.User{orgOwner})
+
 	t.Run("DontSendNotificationEmailOnFirstActionSuccess", func(t *testing.T) {
+		user := getActionsNowDoneTestUser(t, "new_user", "new_user@example.com", "enabled")
+		defer CleanUpUsers(ctx, []*user_model.User{user})
+		assignUsers(user, user)
 		defer MockMailSettings(func(msgs ...*Message) {
 			assert.Fail(t, "no mail should be sent")
 		})()
 		notify_service.ActionRunNowDone(ctx, run2, actions_model.StatusRunning, nil)
 	})

-	t.Run("SendNotificationEmailOnActionRunFailed", func(t *testing.T) {
-		mailSentToOwner := false
-		mailSentToTriggerUser := false
-		defer MockMailSettings(func(msgs ...*Message) {
-			assert.LessOrEqual(t, len(msgs), 2)
-			for _, msg := range msgs {
-				switch msg.To {
-				case triggerUser.EmailTo():
-					assert.False(t, mailSentToTriggerUser, "sent mail twice")
-					mailSentToTriggerUser = true
-				case ownerUser.EmailTo():
-					assert.False(t, mailSentToOwner, "sent mail twice")
-					mailSentToOwner = true
-				default:
-					assert.Fail(t, "sent mail to unknown sender", msg.To)
-				}
-				assert.Contains(t, msg.Body, triggerUser.HTMLURL())
-				assert.Contains(t, msg.Body, triggerUser.Name)
-				// what happened
-				assert.Contains(t, msg.Body, "failed")
-				// new status of run
-				assert.Contains(t, msg.Body, "failure")
-				// prior status of this run
-				assert.Contains(t, msg.Body, "waiting")
-				assertTranslatedLocaleMailActionsNowDone(t, msg.Body)
-			}
-		})()
-		notify_service.ActionRunNowDone(ctx, run1, actions_model.StatusWaiting, nil)
-		assert.True(t, mailSentToOwner)
-		assert.True(t, mailSentToTriggerUser)
-	})
+	for _, testCase := range []struct {
+		name        string
+		triggerUser *user_model.User
+		owner       *user_model.User
+		expected    string
+		expectMail  bool
+	}{
+		{
+			// if the action is assigned a trigger user in a repository
+			// owned by a regular user, the mail is sent to the trigger user
+			name:        "RegularTriggerUser",
+			triggerUser: getActionsNowDoneTestUser(t, "new_trigger_user0", "new_trigger_user0@example.com", user_model.EmailNotificationsEnabled),
+			owner:       getActionsNowDoneTestUser(t, "new_owner0", "new_owner0@example.com", user_model.EmailNotificationsEnabled),
+			expected:    "trigger",
+			expectMail:  true,
+		},
+		{
+			// if the action is assigned to a system user (e.g. ActionsUser)
+			// in a repository owned by a regular user, the mail is sent to
+			// the user that owns the repository
+			name:        "SystemTriggerUserAndRegularOwner",
+			triggerUser: actionsUser,
+			owner:       getActionsNowDoneTestUser(t, "new_owner1", "new_owner1@example.com", user_model.EmailNotificationsEnabled),
+			expected:    "owner",
+			expectMail:  true,
+		},
+		{
+			// if the action is assigned a trigger user with disabled notifications in a repository
+			// owned by a regular user, no mail is sent
+			name:        "RegularTriggerUserNotificationsDisabled",
+			triggerUser: getActionsNowDoneTestUser(t, "new_trigger_user2", "new_trigger_user2@example.com", user_model.EmailNotificationsDisabled),
+			owner:       getActionsNowDoneTestUser(t, "new_owner2", "new_owner2@example.com", user_model.EmailNotificationsEnabled),
+			expectMail:  false,
+		},
+		{
+			// if the action is assigned to a system user (e.g. ActionsUser)
+			// owned by a regular user with disabled notifications, no mail is sent
+			name:        "SystemTriggerUserAndRegularOwnerNotificationsDisabled",
+			triggerUser: actionsUser,
+			owner:       getActionsNowDoneTestUser(t, "new_owner3", "new_owner3@example.com", user_model.EmailNotificationsDisabled),
+			expectMail:  false,
+		},
+		{
+			// if the action is assigned to a system user (e.g. ActionsUser)
+			// in a repository owned by an organization with an email contact, the mail is sent to
+			// this email contact
+			name:        "SystemTriggerUserAndOrgOwner",
+			triggerUser: actionsUser,
+			owner:       getActionsNowDoneTestOrg(t, "new_org1", "new_org_owner0@example.com", orgOwner),
+			expected:    "owner",
+			expectMail:  true,
+		},
+		{
+			// if the action is assigned to a system user (e.g. ActionsUser)
+			// in a repository owned by an organization without an email contact, no mail is sent
+			name:        "SystemTriggerUserAndNoMailOrgOwner",
+			triggerUser: actionsUser,
+			owner:       getActionsNowDoneTestOrg(t, "new_org2", "", orgOwner),
+			expectMail:  false,
+		},
+	} {
+		t.Run(testCase.name, func(t *testing.T) {
+			assignUsers(testCase.triggerUser, testCase.owner)
+			defer CleanUpUsers(ctx, slices.DeleteFunc([]*user_model.User{testCase.triggerUser, testCase.owner}, func(user *user_model.User) bool {
+				return user.IsSystem()
+			}))

-	t.Run("SendNotificationEmailOnActionRunRecovered", func(t *testing.T) {
-		mailSentToOwner := false
-		mailSentToTriggerUser := false
-		defer MockMailSettings(func(msgs ...*Message) {
-			assert.LessOrEqual(t, len(msgs), 2)
-			for _, msg := range msgs {
-				switch msg.To {
-				case triggerUser.EmailTo():
-					assert.False(t, mailSentToTriggerUser, "sent mail twice")
-					mailSentToTriggerUser = true
-				case ownerUser.EmailTo():
-					assert.False(t, mailSentToOwner, "sent mail twice")
-					mailSentToOwner = true
-				default:
-					assert.Fail(t, "sent mail to unknown sender", msg.To)
-				}
-				assert.Contains(t, msg.Body, triggerUser.HTMLURL())
-				assert.Contains(t, msg.Body, triggerUser.Name)
-				// what happened
-				assert.Contains(t, msg.Body, "recovered")
-				// old status of run
-				assert.Contains(t, msg.Body, "failure")
-				// new status of run
-				assert.Contains(t, msg.Body, "success")
-				// prior status of this run
-				assert.Contains(t, msg.Body, "running")
-				assertTranslatedLocaleMailActionsNowDone(t, msg.Body)
-			}
-		})()
-		assert.NotNil(t, setting.MailService)
+			t.Run("SendNotificationEmailOnActionRunFailed", func(t *testing.T) {
+				mailSent := false
+				defer MockMailSettings(func(msgs ...*Message) {
+					assert.Len(t, msgs, 1)
+					msg := msgs[0]
+					assert.False(t, mailSent, "sent mail twice")
+					expectedEmail := testCase.triggerUser.Email
+					if testCase.expected == "owner" { // otherwise "trigger"
+						expectedEmail = testCase.owner.Email
+					}
+					require.Contains(t, msg.To, expectedEmail, "sent mail to unknown sender")
+					mailSent = true
+					assert.Contains(t, msg.Body, testCase.triggerUser.HTMLURL())
+					assert.Contains(t, msg.Body, testCase.triggerUser.Name)
+					// what happened
+					assert.Contains(t, msg.Body, "failed")
+					// new status of run
+					assert.Contains(t, msg.Body, "failure")
+					// prior status of this run
+					assert.Contains(t, msg.Body, "waiting")
+					assertTranslatedLocaleMailActionsNowDone(t, msg.Body)
+				})()
+				require.NotNil(t, setting.MailService)

-		notify_service.ActionRunNowDone(ctx, run2, actions_model.StatusRunning, run1)
-		assert.True(t, mailSentToOwner)
-		assert.True(t, mailSentToTriggerUser)
-	})
+				notify_service.ActionRunNowDone(ctx, run1, actions_model.StatusWaiting, nil)
+				assert.Equal(t, testCase.expectMail, mailSent)
+			})
+
+			t.Run("SendNotificationEmailOnActionRunRecovered", func(t *testing.T) {
+				mailSent := false
+				defer MockMailSettings(func(msgs ...*Message) {
+					assert.Len(t, msgs, 1)
+					msg := msgs[0]
+					assert.False(t, mailSent, "sent mail twice")
+					expectedEmail := testCase.triggerUser.Email
+					if testCase.expected == "owner" { // otherwise "trigger"
+						expectedEmail = testCase.owner.Email
+					}
+					require.Contains(t, msg.To, expectedEmail, "sent mail to unknown sender")
+					mailSent = true
+					assert.Contains(t, msg.Body, testCase.triggerUser.HTMLURL())
+					assert.Contains(t, msg.Body, testCase.triggerUser.Name)
+					// what happened
+					assert.Contains(t, msg.Body, "recovered")
+					// old status of run
+					assert.Contains(t, msg.Body, "failure")
+					// new status of run
+					assert.Contains(t, msg.Body, "success")
+					// prior status of this run
+					assert.Contains(t, msg.Body, "running")
+				})()
+				require.NotNil(t, setting.MailService)
+
+				notify_service.ActionRunNowDone(ctx, run2, actions_model.StatusRunning, run1)
+				assert.Equal(t, testCase.expectMail, mailSent)
+			})
+		})
+	}
 }
--- a/services/mailer/main_test.go
+++ b/services/mailer/main_test.go
@ -8,6 +8,7 @@ import (
 	"testing"

 	"forgejo.org/models/db"
+	organization_model "forgejo.org/models/organization"
 	"forgejo.org/models/unittest"
 	user_model "forgejo.org/models/user"
 	"forgejo.org/modules/setting"
@ -51,6 +52,11 @@ func MockMailSettings(send func(msgs ...*Message)) func() {

 func CleanUpUsers(ctx context.Context, users []*user_model.User) {
 	for _, u := range users {
-		db.DeleteByID[user_model.User](ctx, u.ID)
+		if u.IsOrganization() {
+			organization_model.DeleteOrganization(ctx, (*organization_model.Organization)(u))
+		} else {
+			db.DeleteByID[user_model.User](ctx, u.ID)
+			db.DeleteByBean(ctx, &user_model.EmailAddress{UID: u.ID})
+		}
 	}
 }