diff --git a/config/config_test.go b/config/config_test.go index 10d55fdbb..d69e15bed 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -69,7 +69,7 @@ func TestLoad(t *testing.T) { } func TestLoadDeletion(t *testing.T) { - t.Run("reads namespace keys and rules", func(t *testing.T) { + t.Run("reads namespace keys, rules and the user delete email", func(t *testing.T) { want := deleter.Config{ Enabled: true, Resources: map[string]deleter.Rules{ @@ -85,6 +85,10 @@ func TestLoadDeletion(t *testing.T) { deleter.ParentOrganization: deleter.RuleAllow, }, }, + UserDeleteNotice: deleter.UserDeleteNoticeConfig{ + Subject: "Your account was deleted", + Body: "

Hi {{.User.Title}}

", + }, } got, err := Load("testdata/deletion.yaml") diff --git a/config/sample.config.yaml b/config/sample.config.yaml index 2a5e09083..4176f9819 100644 --- a/config/sample.config.yaml +++ b/config/sample.config.yaml @@ -243,6 +243,10 @@ app: compute/machine: { project: block, organization: allow } compute/order: { project: block_unless_deletable, organization: block_unless_deletable } storage/file: { organization: allow } + # emailed after an account delete, even when enabled is false + user_delete_notice: + subject: "Your account was deleted" + body: "Hi {{.User.Title}},

Your account {{.User.Email}} was deleted." # metaschema cache configuration metaschema: diff --git a/config/testdata/deletion.yaml b/config/testdata/deletion.yaml index 508d16308..da4620b5e 100644 --- a/config/testdata/deletion.yaml +++ b/config/testdata/deletion.yaml @@ -7,3 +7,6 @@ app: compute/machine: { project: block, organization: allow } compute/order: { project: block_unless_deletable, organization: block_unless_deletable } storage/file: { organization: allow } + user_delete_notice: + subject: "Your account was deleted" + body: "

Hi {{.User.Title}}

" diff --git a/core/deleter/config.go b/core/deleter/config.go index a361140b0..f7d9f990c 100644 --- a/core/deleter/config.go +++ b/core/deleter/config.go @@ -31,6 +31,13 @@ type Rules map[Parent]Rule type Config struct { Enabled bool `yaml:"enabled" mapstructure:"enabled" default:"false"` Resources map[string]Rules `yaml:"resources" mapstructure:"resources"` + // sent whether or not Enabled is set + UserDeleteNotice UserDeleteNoticeConfig `yaml:"user_delete_notice" mapstructure:"user_delete_notice"` +} + +type UserDeleteNoticeConfig struct { + Subject string `yaml:"subject" mapstructure:"subject"` + Body string `yaml:"body" mapstructure:"body"` } func (c Config) Validate() error { diff --git a/core/deleter/delete_notice.go b/core/deleter/delete_notice.go index 3706a8c7f..4744f4822 100644 --- a/core/deleter/delete_notice.go +++ b/core/deleter/delete_notice.go @@ -28,8 +28,15 @@ const forfeitRecordsLimit = 1000 const ( defaultDeleteNoticeSubject = "Your organization was deleted" defaultDeleteNoticeBody = `{{if .User.Title}}Hi {{.User.Title}},{{else}}Hi,{{end}}

Your organization {{if .Org.Title}}{{.Org.Title}}{{else}}{{.Org.Name}}{{end}} was deleted{{if .DeletedBy}} by {{.DeletedBy}}{{end}}.{{if .Purchased}} Unused purchased tokens will be settled by the support team.{{end}}` + + defaultUserDeleteNoticeSubject = "Your account was deleted" + defaultUserDeleteNoticeBody = `{{if .User.Title}}Hi {{.User.Title}},{{else}}Hi,{{end}}

Your account {{.User.Email}} was deleted.` ) +type userDeleteNoticeData struct { + User user.User +} + type deleteNoticeData struct { // Amount is the total number of tokens the delete forfeited. Amount int64 @@ -202,23 +209,11 @@ func (d Service) sendDeleteNotices(ctx context.Context, org organization.Organiz slog.WarnContext(ctx, "the organization was deleted but no owner could be notified", "org_id", org.ID, "amount", notice.Amount, "purchased", notice.Purchased) return } - subjectTpl := d.deleteNoticeConfig.Subject - if subjectTpl == "" { - subjectTpl = defaultDeleteNoticeSubject - } - bodyTpl := d.deleteNoticeConfig.Body - if bodyTpl == "" { - bodyTpl = defaultDeleteNoticeBody - } // the templates are the same for every owner; parse them once - subjectTmpl, err := texttemplate.New("subject").Parse(subjectTpl) + tmpl, err := parseMailTemplate(d.deleteNoticeConfig.Subject, d.deleteNoticeConfig.Body, + defaultDeleteNoticeSubject, defaultDeleteNoticeBody) if err != nil { - slog.WarnContext(ctx, "failed to parse the delete notice subject template", "org_id", org.ID, "error", err) - return - } - bodyTmpl, err := htmltemplate.New("body").Parse(bodyTpl) - if err != nil { - slog.WarnContext(ctx, "failed to parse the delete notice body template", "org_id", org.ID, "error", err) + slog.WarnContext(ctx, "failed to parse the delete notice template", "org_id", org.ID, "error", err) return } @@ -231,22 +226,7 @@ func (d Service) sendDeleteNotices(ctx context.Context, org organization.Organiz Org: org, DeletedBy: deletedBy, } - var subject, body bytes.Buffer - if err := subjectTmpl.Execute(&subject, data); err != nil { - slog.WarnContext(ctx, "failed to render the delete notice subject", "org_id", org.ID, "user_email", owner.Email, "error", err) - continue - } - if err := bodyTmpl.Execute(&body, data); err != nil { - slog.WarnContext(ctx, "failed to render the delete notice body", "org_id", org.ID, "user_email", owner.Email, "error", err) - continue - } - - msg := mail.NewMessage() - msg.SetHeader("From", d.mailDialer.FromHeader()) - msg.SetHeader("To", owner.Email) - msg.SetHeader("Subject", subject.String()) - msg.SetBody("text/html", body.String()) - if err := d.mailDialer.DialAndSend(msg); err != nil { + if err := d.sendMail(tmpl, owner.Email, data); err != nil { slog.WarnContext(ctx, "failed to send the delete notice", "org_id", org.ID, "user_email", owner.Email, "error", err) continue } @@ -254,6 +234,64 @@ func (d Service) sendDeleteNotices(ctx context.Context, org organization.Organiz } } +func (d Service) sendUserDeleteNotice(ctx context.Context, usr user.User) { + if d.mailDialer == nil { + slog.WarnContext(ctx, "no mail dialer configured, skipping the user delete notice", "user_id", usr.ID) + return + } + notice := d.deletionConfig.UserDeleteNotice + tmpl, err := parseMailTemplate(notice.Subject, notice.Body, + defaultUserDeleteNoticeSubject, defaultUserDeleteNoticeBody) + if err != nil { + slog.WarnContext(ctx, "failed to parse the user delete notice template", "user_id", usr.ID, "error", err) + return + } + if err := d.sendMail(tmpl, usr.Email, userDeleteNoticeData{User: usr}); err != nil { + slog.WarnContext(ctx, "failed to send the user delete notice", "user_id", usr.ID, "error", err) + return + } + slog.InfoContext(ctx, "sent the user delete notice", "user_id", usr.ID) +} + +type mailTemplate struct { + subject *texttemplate.Template + body *htmltemplate.Template +} + +func parseMailTemplate(subject, body, defaultSubject, defaultBody string) (mailTemplate, error) { + if subject == "" { + subject = defaultSubject + } + if body == "" { + body = defaultBody + } + subjectTmpl, err := texttemplate.New("subject").Parse(subject) + if err != nil { + return mailTemplate{}, fmt.Errorf("failed to parse the subject template: %w", err) + } + bodyTmpl, err := htmltemplate.New("body").Parse(body) + if err != nil { + return mailTemplate{}, fmt.Errorf("failed to parse the body template: %w", err) + } + return mailTemplate{subject: subjectTmpl, body: bodyTmpl}, nil +} + +func (d Service) sendMail(tmpl mailTemplate, to string, data any) error { + var subject, body bytes.Buffer + if err := tmpl.subject.Execute(&subject, data); err != nil { + return fmt.Errorf("failed to render the subject: %w", err) + } + if err := tmpl.body.Execute(&body, data); err != nil { + return fmt.Errorf("failed to render the body: %w", err) + } + msg := mail.NewMessage() + msg.SetHeader("From", d.mailDialer.FromHeader()) + msg.SetHeader("To", to) + msg.SetHeader("Subject", subject.String()) + msg.SetBody("text/html", body.String()) + return d.mailDialer.DialAndSend(msg) +} + // deletedByFromContext names the caller who ran the delete, when the // context carries one. func deletedByFromContext(ctx context.Context) string { diff --git a/core/deleter/delete_notice_test.go b/core/deleter/delete_notice_test.go index 9d0a51686..c40a6183e 100644 --- a/core/deleter/delete_notice_test.go +++ b/core/deleter/delete_notice_test.go @@ -1,6 +1,7 @@ package deleter import ( + "bytes" "context" "errors" "testing" @@ -9,9 +10,11 @@ import ( "github.com/raystack/frontier/core/auditrecord" "github.com/raystack/frontier/core/deleter/mocks" pkgauditrecord "github.com/raystack/frontier/pkg/auditrecord" + mailermocks "github.com/raystack/frontier/pkg/mailer/mocks" "github.com/raystack/frontier/pkg/metadata" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "gopkg.in/mail.v2" ) func deletedRecord(accountID string, meta metadata.Metadata) auditrecord.AuditRecord { @@ -74,3 +77,73 @@ func TestRecoverForfeitFromAudit(t *testing.T) { assert.Equal(t, deleteNotice{}, got) }) } + +func TestParseMailTemplate(t *testing.T) { + render := func(t *testing.T, tmpl mailTemplate) (string, string) { + t.Helper() + var subject, body bytes.Buffer + assert.NoError(t, tmpl.subject.Execute(&subject, nil)) + assert.NoError(t, tmpl.body.Execute(&body, nil)) + return subject.String(), body.String() + } + + t.Run("an empty subject and body fall back to the defaults", func(t *testing.T) { + tmpl, err := parseMailTemplate("", "", "default subject", "default body") + assert.NoError(t, err) + subject, body := render(t, tmpl) + assert.Equal(t, "default subject", subject) + assert.Equal(t, "default body", body) + }) + + t.Run("a configured subject and body win over the defaults", func(t *testing.T) { + tmpl, err := parseMailTemplate("set subject", "set body", "default subject", "default body") + assert.NoError(t, err) + subject, body := render(t, tmpl) + assert.Equal(t, "set subject", subject) + assert.Equal(t, "set body", body) + }) + + t.Run("a broken subject fails", func(t *testing.T) { + _, err := parseMailTemplate("{{.Name", "", "", "default body") + assert.ErrorContains(t, err, "subject") + }) + + t.Run("a broken body fails", func(t *testing.T) { + _, err := parseMailTemplate("", "{{.Name", "default subject", "") + assert.ErrorContains(t, err, "body") + }) +} + +func TestSendMail(t *testing.T) { + tmpl, err := parseMailTemplate("Hi {{.Name}}", "{{.Name}}", "", "") + assert.NoError(t, err) + + t.Run("sends one mail to the address", func(t *testing.T) { + dialer := mailermocks.NewDialer(t) + dialer.EXPECT().FromHeader().Return("no-reply@frontier.test") + dialer.EXPECT().DialAndSend(mock.Anything).Run(func(msg *mail.Message) { + assert.Equal(t, []string{"no-reply@frontier.test"}, msg.GetHeader("From")) + assert.Equal(t, []string{"maya@example.com"}, msg.GetHeader("To")) + assert.Equal(t, []string{"Hi Maya"}, msg.GetHeader("Subject")) + }).Return(nil) + + err := Service{mailDialer: dialer}.sendMail(tmpl, "maya@example.com", struct{ Name string }{Name: "Maya"}) + assert.NoError(t, err) + }) + + t.Run("a subject that fails to render sends nothing", func(t *testing.T) { + dialer := mailermocks.NewDialer(t) + + err := Service{mailDialer: dialer}.sendMail(tmpl, "maya@example.com", struct{}{}) + assert.ErrorContains(t, err, "subject") + }) + + t.Run("a body that fails to render sends nothing", func(t *testing.T) { + bodyFails, err := parseMailTemplate("Hi", "{{.Name}}", "", "") + assert.NoError(t, err) + dialer := mailermocks.NewDialer(t) + + err = Service{mailDialer: dialer}.sendMail(bodyFails, "maya@example.com", struct{}{}) + assert.ErrorContains(t, err, "body") + }) +} diff --git a/core/deleter/service.go b/core/deleter/service.go index f0e267cc3..e20659606 100644 --- a/core/deleter/service.go +++ b/core/deleter/service.go @@ -652,6 +652,11 @@ func isOrgOwner(m membership.Member) bool { // DeleteUser stops on any blocker the check finds. Otherwise it deletes the orgs // where the user is the only member and leaves the others, disabled orgs too. func (d Service) DeleteUser(ctx context.Context, userID string) error { + usr, err := d.getUser(ctx, userID) + // a missing user still goes through the cleanup below; the row delete then answers not found + if err != nil && !errors.Is(err, user.ErrNotExist) { + return err + } check, err := d.checkUser(ctx, userID, true) if err != nil { return err @@ -675,7 +680,25 @@ func (d Service) DeleteUser(ctx context.Context, userID string) error { if err := d.userPATService.DeleteAllByUser(ctx, userID); err != nil { return fmt.Errorf("failed to delete user PATs: %w", err) } - return d.userService.Delete(ctx, userID) + if err := d.userService.Delete(ctx, userID); err != nil { + return err + } + d.sendUserDeleteNotice(ctx, usr) + return nil +} + +func (d Service) getUser(ctx context.Context, userID string) (user.User, error) { + if !utils.IsValidUUID(userID) { + return user.User{}, user.ErrInvalidUUID + } + users, err := d.userService.GetByIDsIncludingDisabled(ctx, []string{userID}) + if err != nil { + return user.User{}, err + } + if len(users) == 0 { + return user.User{}, user.ErrNotExist + } + return users[0], nil } // leaveOrg runs after the check, and the org can change in between. diff --git a/core/deleter/service_test.go b/core/deleter/service_test.go index fb42d2988..79d9b307c 100644 --- a/core/deleter/service_test.go +++ b/core/deleter/service_test.go @@ -1840,15 +1840,37 @@ func expectEmptyOrgDelete(m deleterMocks, steps *[]string) { expectEmptyOrgTail(m, steps) } +// The check reads org members with this same call, so register it before +// expectUserOrg. Once makes it answer only the first read. +func expectUserRead(m deleterMocks, userID string) { + m.usrSvc.EXPECT().GetByIDsIncludingDisabled(mock.Anything, []string{userID}). + Return([]user.User{{ID: userID, Email: "maya@example.com"}}, nil).Once() +} + +func expectUserNotice(m deleterMocks, userID string) { + expectUserRead(m, userID) + m.dialer.EXPECT().FromHeader().Return("no-reply@frontier.test") + m.dialer.EXPECT().DialAndSend(mock.Anything).Return(nil) +} + +func expectUserTeardown(m deleterMocks, userID string) { + m.mbrSvc.EXPECT().RemovePrincipalPolicies(mock.Anything, userID, schema.UserPrincipal).Return(nil) + m.patSvc.EXPECT().DeleteAllByUser(mock.Anything, userID).Return(nil) + m.usrSvc.EXPECT().Delete(mock.Anything, userID).Return(nil) +} + func TestDeleteUser(t *testing.T) { - const userID = "user-1" + const userID = "6f1c9e2a-3b4d-4e5f-8a6b-7c8d9e0f1a2b" solo := organization.Organization{ID: "org-1", Name: "solo", State: organization.Disabled} shared := organization.Organization{ID: "org-2", Name: "shared"} - t.Run("deletes the orgs that go with the user, leaves the others, then removes every policy it still holds, its PATs, and the user", func(t *testing.T) { + t.Run("deletes the orgs that go with the user, leaves the others, then removes every policy it still holds, its PATs, and the user, then emails the user", func(t *testing.T) { m := newMocks(t) var steps []string + m.usrSvc.EXPECT().GetByIDsIncludingDisabled(mock.Anything, []string{userID}). + Run(func(context.Context, []string) { steps = append(steps, "read") }). + Return([]user.User{{ID: userID, Email: "maya@example.com", Title: "Maya"}}, nil).Once() expectUserOrgs(m, solo.ID, shared.ID) expectUserOrg(m, solo, []membership.Member{orgMember(userID, orgOwner)}, userID) expectNoBillingAccounts(m, solo.ID) @@ -1862,16 +1884,29 @@ func TestDeleteUser(t *testing.T) { Run(func(context.Context, string) { steps = append(steps, "pats") }).Return(nil) m.usrSvc.EXPECT().Delete(mock.Anything, userID). Run(func(context.Context, string) { steps = append(steps, "user") }).Return(nil) + m.dialer.EXPECT().FromHeader().Return("no-reply@frontier.test") + m.dialer.EXPECT().DialAndSend(mock.Anything).Run(func(msg *mail.Message) { + steps = append(steps, "mail") + assert.Equal(t, []string{"maya@example.com"}, msg.GetHeader("To")) + assert.Equal(t, []string{"Your account was deleted"}, msg.GetHeader("Subject")) + var raw bytes.Buffer + _, err := msg.WriteTo(&raw) + assert.NoError(t, err) + body := strings.ReplaceAll(raw.String(), "=\r\n", "") + assert.Contains(t, body, "Hi Maya,") + assert.Contains(t, body, "maya@example.com was deleted") + }).Return(nil) err := m.build().DeleteUser(context.Background(), userID) assert.NoError(t, err) - assert.Equal(t, []string{"org", "leave:org-2", "policies", "pats", "user"}, steps) + assert.Equal(t, []string{"read", "org", "leave:org-2", "policies", "pats", "user", "mail"}, steps) }) t.Run("refuses with every blocker, judging invoices on provider data, before changing anything", func(t *testing.T) { m := newMocks(t) c := customer.Customer{ID: "cust-1", OrgID: solo.ID, ProviderID: "stripe-1"} + expectUserRead(m, userID) expectUserOrgs(m, solo.ID, shared.ID) expectUserOrg(m, solo, []membership.Member{orgMember(userID, orgOwner)}, userID) m.custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: solo.ID}).Return([]customer.Customer{c}, nil) @@ -1904,6 +1939,7 @@ func TestDeleteUser(t *testing.T) { m.deletion = blockerConfig() other := organization.Organization{ID: "org-3", Name: "other"} + expectUserRead(m, userID) expectUserOrgs(m, solo.ID, other.ID) for _, org := range []organization.Organization{solo, other} { expectUserOrg(m, org, []membership.Member{orgMember(userID, orgOwner)}, userID) @@ -1925,6 +1961,7 @@ func TestDeleteUser(t *testing.T) { t.Run("a failed org delete stops before the user leaves the other orgs", func(t *testing.T) { m := newMocks(t) + expectUserRead(m, userID) expectUserOrgs(m, solo.ID, shared.ID) expectUserOrg(m, solo, []membership.Member{orgMember(userID, orgOwner)}, userID) expectNoBillingAccounts(m, solo.ID) @@ -1943,6 +1980,7 @@ func TestDeleteUser(t *testing.T) { m := newMocks(t) var steps []string + expectUserNotice(m, userID) expectUserOrgs(m, solo.ID) expectUserOrg(m, solo, []membership.Member{orgMember(userID, orgOwner)}, userID) expectNoBillingAccounts(m, solo.ID) @@ -1962,6 +2000,7 @@ func TestDeleteUser(t *testing.T) { t.Run("a user removed from an org since the check does not stop the delete", func(t *testing.T) { m := newMocks(t) + expectUserNotice(m, userID) expectUserOrgs(m, shared.ID) expectUserOrg(m, shared, []membership.Member{orgMember(userID, orgViewer), orgMember("user-2", orgOwner)}, userID, "user-2") m.mbrSvc.EXPECT().RemoveOrganizationMemberOnDelete(mock.Anything, shared.ID, userID, schema.UserPrincipal). @@ -1976,6 +2015,7 @@ func TestDeleteUser(t *testing.T) { t.Run("an org the user leaves that was deleted since the check does not stop the delete", func(t *testing.T) { m := newMocks(t) + expectUserNotice(m, userID) expectUserOrgs(m, shared.ID) expectUserOrg(m, shared, []membership.Member{orgMember(userID, orgViewer), orgMember("user-2", orgOwner)}, userID, "user-2") m.mbrSvc.EXPECT().RemoveOrganizationMemberOnDelete(mock.Anything, shared.ID, userID, schema.UserPrincipal). @@ -1990,6 +2030,7 @@ func TestDeleteUser(t *testing.T) { t.Run("an org that lost its other owner since the check refuses the delete with the blocker", func(t *testing.T) { m := newMocks(t) + expectUserRead(m, userID) expectUserOrgs(m, shared.ID) expectUserOrg(m, shared, []membership.Member{orgMember(userID, orgOwner), orgMember("user-2", orgOwner)}, userID, "user-2") m.mbrSvc.EXPECT().RemoveOrganizationMemberOnDelete(mock.Anything, shared.ID, userID, schema.UserPrincipal). @@ -2004,6 +2045,7 @@ func TestDeleteUser(t *testing.T) { t.Run("an org removal failure stops the delete before the remaining policies", func(t *testing.T) { m := newMocks(t) + expectUserRead(m, userID) expectUserOrgs(m, shared.ID) expectUserOrg(m, shared, []membership.Member{orgMember(userID, orgViewer), orgMember("user-2", orgOwner)}, userID, "user-2") m.mbrSvc.EXPECT().RemoveOrganizationMemberOnDelete(mock.Anything, shared.ID, userID, schema.UserPrincipal). @@ -2016,6 +2058,7 @@ func TestDeleteUser(t *testing.T) { t.Run("a policy removal failure stops the delete before the PATs and the user", func(t *testing.T) { m := newMocks(t) + expectUserRead(m, userID) expectUserOrgs(m) m.mbrSvc.EXPECT().RemovePrincipalPolicies(mock.Anything, userID, schema.UserPrincipal). Return(errors.New("spicedb unavailable")) @@ -2027,6 +2070,7 @@ func TestDeleteUser(t *testing.T) { t.Run("aborts before userService.Delete when PAT cleanup fails", func(t *testing.T) { m := newMocks(t) + expectUserRead(m, userID) expectUserOrgs(m) m.mbrSvc.EXPECT().RemovePrincipalPolicies(mock.Anything, userID, schema.UserPrincipal).Return(nil) m.patSvc.EXPECT().DeleteAllByUser(mock.Anything, userID).Return(errors.New("pat cleanup boom")) @@ -2039,12 +2083,112 @@ func TestDeleteUser(t *testing.T) { t.Run("a failed check stops the delete before anything changes", func(t *testing.T) { m := newMocks(t) + expectUserRead(m, userID) m.mbrSvc.EXPECT().ListResourcesByPrincipal(mock.Anything, mock.Anything, schema.OrganizationNamespace, mock.Anything). Return(nil, errors.New("read failed")) err := m.build().DeleteUser(context.Background(), userID) assert.EqualError(t, err, "read failed") }) + + t.Run("the email can use the fields of the deleted user", func(t *testing.T) { + m := newMocks(t) + m.deletion.UserDeleteNotice = deleter.UserDeleteNoticeConfig{ + Subject: "Goodbye {{.User.Name}}", + Body: "{{.User.ID}} {{.User.Title}} {{.User.Email}} {{.User.State}}", + } + + m.usrSvc.EXPECT().GetByIDsIncludingDisabled(mock.Anything, []string{userID}).Return([]user.User{{ + ID: userID, + Name: "maya", + Email: "maya@example.com", + Title: "Maya", + State: user.Enabled, + }}, nil) + expectUserOrgs(m) + expectUserTeardown(m, userID) + m.dialer.EXPECT().FromHeader().Return("no-reply@frontier.test") + m.dialer.EXPECT().DialAndSend(mock.Anything).Run(func(msg *mail.Message) { + assert.Equal(t, []string{"Goodbye maya"}, msg.GetHeader("Subject")) + var raw bytes.Buffer + _, err := msg.WriteTo(&raw) + assert.NoError(t, err) + body := strings.ReplaceAll(raw.String(), "=\r\n", "") + assert.Contains(t, body, userID+" Maya maya@example.com enabled") + }).Return(nil) + + assert.NoError(t, m.build().DeleteUser(context.Background(), userID)) + }) + + t.Run("a failed email does not fail the delete", func(t *testing.T) { + m := newMocks(t) + + expectUserRead(m, userID) + expectUserOrgs(m) + expectUserTeardown(m, userID) + m.dialer.EXPECT().FromHeader().Return("no-reply@frontier.test") + m.dialer.EXPECT().DialAndSend(mock.Anything).Return(errors.New("smtp down")) + + assert.NoError(t, m.build().DeleteUser(context.Background(), userID)) + }) + + t.Run("a broken configured template still deletes the user, without an email", func(t *testing.T) { + m := newMocks(t) + m.deletion.UserDeleteNotice = deleter.UserDeleteNoticeConfig{Body: "{{.User.Email"} + + expectUserRead(m, userID) + expectUserOrgs(m) + expectUserTeardown(m, userID) + + assert.NoError(t, m.build().DeleteUser(context.Background(), userID)) + }) + + t.Run("a disabled user gets the email too", func(t *testing.T) { + m := newMocks(t) + + m.usrSvc.EXPECT().GetByIDsIncludingDisabled(mock.Anything, []string{userID}). + Return([]user.User{{ID: userID, Email: "maya@example.com", State: user.Disabled}}, nil) + expectUserOrgs(m) + expectUserTeardown(m, userID) + m.dialer.EXPECT().FromHeader().Return("no-reply@frontier.test") + m.dialer.EXPECT().DialAndSend(mock.Anything).Run(func(msg *mail.Message) { + assert.Equal(t, []string{"maya@example.com"}, msg.GetHeader("To")) + }).Return(nil) + + assert.NoError(t, m.build().DeleteUser(context.Background(), userID)) + }) + + t.Run("a failed user read stops the delete before anything changes", func(t *testing.T) { + m := newMocks(t) + + m.usrSvc.EXPECT().GetByIDsIncludingDisabled(mock.Anything, []string{userID}). + Return(nil, errors.New("read failed")) + + err := m.build().DeleteUser(context.Background(), userID) + assert.EqualError(t, err, "read failed") + }) + + t.Run("an id that is not a uuid is refused before anything changes", func(t *testing.T) { + m := newMocks(t) + + err := m.build().DeleteUser(context.Background(), "user-1") + assert.ErrorIs(t, err, user.ErrInvalidUUID) + }) + + t.Run("a user that is already deleted, or never existed, still has its leftovers cleared and answers not found", func(t *testing.T) { + m := newMocks(t) + + m.usrSvc.EXPECT().GetByIDsIncludingDisabled(mock.Anything, []string{userID}).Return(nil, nil) + // the user has no row, so the check skips the org it still holds a policy in + expectUserOrgs(m, shared.ID) + expectUserOrg(m, shared, []membership.Member{orgMember(userID, orgOwner), orgMember("user-2", orgViewer)}, "user-2") + m.mbrSvc.EXPECT().RemovePrincipalPolicies(mock.Anything, userID, schema.UserPrincipal).Return(nil) + m.patSvc.EXPECT().DeleteAllByUser(mock.Anything, userID).Return(nil) + m.usrSvc.EXPECT().Delete(mock.Anything, userID).Return(user.ErrNotExist) + + err := m.build().DeleteUser(context.Background(), userID) + assert.ErrorIs(t, err, user.ErrNotExist) + }) } func TestCheckUserDelete(t *testing.T) { diff --git a/docs/content/docs/reference/configurations.mdx b/docs/content/docs/reference/configurations.mdx index beb784378..41fd25b97 100644 --- a/docs/content/docs/reference/configurations.mdx +++ b/docs/content/docs/reference/configurations.mdx @@ -174,6 +174,10 @@ app: compute/machine: { project: block, organization: allow } compute/order: { project: block_unless_deletable, organization: block_unless_deletable } storage/file: { organization: allow } + # emailed after an account delete, even when enabled is false + user_delete_notice: + subject: "Your account was deleted" + body: "Hi {{.User.Title}},

Your account {{.User.Email}} was deleted." # metaschema cache configuration metaschema: # how often each server reloads the metaschema cache from the database, so a @@ -336,6 +340,13 @@ An unknown key or rule, a namespace with no rules, or a namespace not shaped lik | **app.deletion.resources.\.project** | Rule for a project delete. Missing, it stops the delete. | `block` | No | | **app.deletion.resources.\.organization** | Rule for an organization delete. Missing, it stops the delete. | `allow` | No | +`user_delete_notice` sets the email a user gets after their account is deleted, by the user or by an admin. It is sent whether or not `enabled` is set, and a disabled user gets it too. The subject and body are Go templates. `.User` holds the deleted user's `ID`, `Name`, `Email`, `Title`, `Avatar` and `State`, as in `{{.User.Title}}`. If sending fails, the delete still succeeds. It only goes out when `app.mailer.smtp_host` is set. Without it, Frontier uses a stand-in mailer that sends nothing, and the log still says the email was sent. + +| **Field** | **Description** | **Example** | **Required** | +| --------------------------------------------- | ---------------------------------------------------------------------------------- | ---------------------------- | ------------ | +| **app.deletion.user_delete_notice.subject** | Subject of the email. Empty, it is `Your account was deleted`. | `"Your account was deleted"` | No | +| **app.deletion.user_delete_notice.body** | HTML body of the email. Empty, it is a short built-in text with the user's email. | `"Hi {{.User.Title}}, ..."` | No | + ### Admin Configurations | **Field** | **Description** | **Example** | **Required** | diff --git a/internal/api/v1beta1connect/user.go b/internal/api/v1beta1connect/user.go index c20d4ea30..c11f02eae 100644 --- a/internal/api/v1beta1connect/user.go +++ b/internal/api/v1beta1connect/user.go @@ -457,7 +457,7 @@ func (h *ConnectHandler) deleteUser(ctx context.Context, request connect.AnyRequ switch { case errors.Is(err, user.ErrNotExist): return connect.NewError(connect.CodeNotFound, ErrUserNotExist) - case errors.Is(err, user.ErrInvalidID): + case errors.Is(err, user.ErrInvalidID), errors.Is(err, user.ErrInvalidUUID): return connect.NewError(connect.CodeInvalidArgument, ErrBadRequest) default: return connect.NewError(connect.CodeInternal, fmt.Errorf("%s: user_id=%s: %w", operation, userID, err)) diff --git a/internal/api/v1beta1connect/user_test.go b/internal/api/v1beta1connect/user_test.go index 635648cd6..97b2ca7dc 100644 --- a/internal/api/v1beta1connect/user_test.go +++ b/internal/api/v1beta1connect/user_test.go @@ -866,6 +866,17 @@ func TestConnectHandler_DeleteUser(t *testing.T) { want: nil, err: connect.CodeInvalidArgument, }, + { + title: "should return invalid argument error for a user ID that is not a uuid", + setup: func(cd *mocks.CascadeDeleter) { + cd.EXPECT().DeleteUser(mock.Anything, "not-a-uuid").Return(user.ErrInvalidUUID) + }, + req: &frontierv1beta1.DeleteUserRequest{ + Id: "not-a-uuid", + }, + want: nil, + err: connect.CodeInvalidArgument, + }, { title: "should return internal error if deleter service returns unexpected error", setup: func(cd *mocks.CascadeDeleter) {