From 9e6f722f94aec81bc41549aa485a255d31268796 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sat, 21 Jun 2025 12:15:38 +0200 Subject: [PATCH] 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 Co-authored-by: Earl Warren Co-committed-by: Earl Warren --- models/user/user_system.go | 7 + routers/web/repo/issue.go | 2 +- services/mailer/mail_actions.go | 19 +- services/mailer/mail_actions_now_done_test.go | 270 ++++++++++++------ services/mailer/main_test.go | 8 +- 5 files changed, 201 insertions(+), 105 deletions(-) diff --git a/models/user/user_system.go b/models/user/user_system.go index 82805cc8ee..11f54591b7 100644 --- 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" diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index d8f3bd8d9f..3f17e30cec 100644 --- 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 } diff --git a/services/mailer/mail_actions.go b/services/mailer/mail_actions.go index 7c63603a98..ad2bd7dfab 100644 --- 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 { diff --git a/services/mailer/mail_actions_now_done_test.go b/services/mailer/mail_actions_now_done_test.go index 0d832f2b36..19c6d9670b 100644 --- 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: ¬ifications, + } + 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) + }) + }) + } } diff --git a/services/mailer/main_test.go b/services/mailer/main_test.go index 47e5d5d175..5e9cbe3e99 100644 --- 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}) + } } }