From f9cf5ecb357a765d16dbbac070de4da956fb6236 Mon Sep 17 00:00:00 2001 From: Mr3Jane Date: Tue, 12 Jul 2022 22:23:08 +0200 Subject: [PATCH] Fix degrading landing performance Extracted relationship fetch calls from getDetails to separate methods for a more granular control of amount of retrieved data. This could be improved with proper usage of relations, but this is out of scope of the issue in question. --- controllers/api/campaign.go | 52 ++++++++++++++++++ controllers/phish_test.go | 105 +++++++++++++++++++++++++++++++++++- models/campaign.go | 101 +++++++++++++++++----------------- models/campaign_test.go | 8 +++ models/maillog.go | 4 +- models/maillog_test.go | 44 +++++++++++++-- 6 files changed, 259 insertions(+), 55 deletions(-) diff --git a/controllers/api/campaign.go b/controllers/api/campaign.go index 33c08fe7..a6bb9963 100644 --- a/controllers/api/campaign.go +++ b/controllers/api/campaign.go @@ -21,6 +21,32 @@ func (as *Server) Campaigns(w http.ResponseWriter, r *http.Request) { if err != nil { log.Error(err) } + for i := range cs { + err = cs[i].FetchEvents() + if err != nil { + log.Error(err) + } + + err = cs[i].FetchPage() + if err != nil { + log.Error(err) + } + + err = cs[i].FetchResults() + if err != nil { + log.Error(err) + } + + err = cs[i].FetchSMTP() + if err != nil { + log.Error(err) + } + + err = cs[i].FetchTemplate() + if err != nil { + log.Error(err) + } + } JSONResponse(w, cs, http.StatusOK) //POST: Create a new campaign and return it as JSON case r.Method == "POST": @@ -70,8 +96,34 @@ func (as *Server) Campaign(w http.ResponseWriter, r *http.Request) { JSONResponse(w, models.Response{Success: false, Message: "Campaign not found"}, http.StatusNotFound) return } + switch { case r.Method == "GET": + err = c.FetchEvents() + if err != nil { + log.Error(err) + } + + err = c.FetchPage() + if err != nil { + log.Error(err) + } + + err = c.FetchResults() + if err != nil { + log.Error(err) + } + + err = c.FetchSMTP() + if err != nil { + log.Error(err) + } + + err = c.FetchTemplate() + if err != nil { + log.Error(err) + } + JSONResponse(w, c, http.StatusOK) case r.Method == "DELETE": err = models.DeleteCampaign(id) diff --git a/controllers/phish_test.go b/controllers/phish_test.go index 87490bed..b1451ff1 100644 --- a/controllers/phish_test.go +++ b/controllers/phish_test.go @@ -24,6 +24,27 @@ func getFirstCampaign(t *testing.T) models.Campaign { func getFirstEmailRequest(t *testing.T) models.EmailRequest { campaign := getFirstCampaign(t) + + err := campaign.FetchResults() + if err != nil { + t.Fatalf("error fetching results received. expected %v got %v", nil, err) + } + + err = campaign.FetchPage() + if err != nil { + t.Fatalf("error fetching page received. expected %v got %v", nil, err) + } + + err = campaign.FetchTemplate() + if err != nil { + t.Fatalf("error fetching page received. expected %v got %v", nil, err) + } + + err = campaign.FetchSMTP() + if err != nil { + t.Fatalf("error fetching page received. expected %v got %v", nil, err) + } + req := models.EmailRequest{ TemplateId: campaign.TemplateId, Template: campaign.Template, @@ -35,7 +56,7 @@ func getFirstEmailRequest(t *testing.T) models.EmailRequest { SMTP: campaign.SMTP, FromAddress: campaign.SMTP.FromAddress, } - err := models.PostEmailRequest(&req) + err = models.PostEmailRequest(&req) if err != nil { t.Fatalf("error creating email request: %v", err) } @@ -156,6 +177,12 @@ func TestOpenedPhishingEmail(t *testing.T) { ctx := setupTest(t) defer tearDown(t, ctx) campaign := getFirstCampaign(t) + + err := campaign.FetchResults() + if err != nil { + t.Fatalf("error fetching results received. expected %v got %v", nil, err) + } + result := campaign.Results[0] if result.Status != models.StatusSending { t.Fatalf("unexpected result status received. expected %s got %s", models.StatusSending, result.Status) @@ -164,6 +191,17 @@ func TestOpenedPhishingEmail(t *testing.T) { openEmail(t, ctx, result.RId) campaign = getFirstCampaign(t) + + err = campaign.FetchResults() + if err != nil { + t.Fatalf("error fetching results received. expected %v got %v", nil, err) + } + + err = campaign.FetchEvents() + if err != nil { + t.Fatalf("error fetching events received. expected %v got %v", nil, err) + } + result = campaign.Results[0] lastEvent := campaign.Events[len(campaign.Events)-1] if result.Status != models.EventOpened { @@ -181,6 +219,12 @@ func TestReportedPhishingEmail(t *testing.T) { ctx := setupTest(t) defer tearDown(t, ctx) campaign := getFirstCampaign(t) + + err := campaign.FetchResults() + if err != nil { + t.Fatalf("error fetching results received. expected %v got %v", nil, err) + } + result := campaign.Results[0] if result.Status != models.StatusSending { t.Fatalf("unexpected result status received. expected %s got %s", models.StatusSending, result.Status) @@ -189,7 +233,19 @@ func TestReportedPhishingEmail(t *testing.T) { reportedEmail(t, ctx, result.RId) campaign = getFirstCampaign(t) + + err = campaign.FetchResults() + if err != nil { + t.Fatalf("error fetching results received. expected %v got %v", nil, err) + } + result = campaign.Results[0] + + err = campaign.FetchEvents() + if err != nil { + t.Fatalf("error fetching events received. expected %v got %v", nil, err) + } + lastEvent := campaign.Events[len(campaign.Events)-1] if result.Reported != true { @@ -207,16 +263,39 @@ func TestClickedPhishingLinkAfterOpen(t *testing.T) { ctx := setupTest(t) defer tearDown(t, ctx) campaign := getFirstCampaign(t) + + err := campaign.FetchResults() + if err != nil { + t.Fatalf("error fetching results received. expected %v got %v", nil, err) + } + result := campaign.Results[0] if result.Status != models.StatusSending { t.Fatalf("unexpected result status received. expected %s got %s", models.StatusSending, result.Status) } + err = campaign.FetchPage() + if err != nil { + t.Fatalf("error fetching page received. expected %v got %v", nil, err) + } + openEmail(t, ctx, result.RId) clickLink(t, ctx, result.RId, campaign.Page.HTML) campaign = getFirstCampaign(t) + + err = campaign.FetchResults() + if err != nil { + t.Fatalf("error fetching results received. expected %v got %v", nil, err) + } + result = campaign.Results[0] + + err = campaign.FetchEvents() + if err != nil { + t.Fatalf("error fetching events received. expected %v got %v", nil, err) + } + lastEvent := campaign.Events[len(campaign.Events)-1] if result.Status != models.EventClicked { t.Fatalf("unexpected result status received. expected %s got %s", models.EventClicked, result.Status) @@ -265,6 +344,12 @@ func TestCompletedCampaignClick(t *testing.T) { ctx := setupTest(t) defer tearDown(t, ctx) campaign := getFirstCampaign(t) + + err := campaign.FetchResults() + if err != nil { + t.Fatalf("error fetching results received. expected %v got %v", nil, err) + } + result := campaign.Results[0] if result.Status != models.StatusSending { t.Fatalf("unexpected result status received. expected %s got %s", models.StatusSending, result.Status) @@ -273,6 +358,12 @@ func TestCompletedCampaignClick(t *testing.T) { openEmail(t, ctx, result.RId) campaign = getFirstCampaign(t) + + err = campaign.FetchResults() + if err != nil { + t.Fatalf("error fetching results received. expected %v got %v", nil, err) + } + result = campaign.Results[0] if result.Status != models.EventOpened { t.Fatalf("unexpected result status received. expected %s got %s", models.EventOpened, result.Status) @@ -283,6 +374,12 @@ func TestCompletedCampaignClick(t *testing.T) { clickLink404(t, ctx, result.RId) campaign = getFirstCampaign(t) + + err = campaign.FetchResults() + if err != nil { + t.Fatalf("error fetching results received. expected %v got %v", nil, err) + } + result = campaign.Results[0] if result.Status != models.EventOpened { t.Fatalf("unexpected result status received. expected %s got %s", models.EventOpened, result.Status) @@ -348,6 +445,12 @@ func TestTransparencyRequest(t *testing.T) { ctx := setupTest(t) defer tearDown(t, ctx) campaign := getFirstCampaign(t) + + err := campaign.FetchResults() + if err != nil { + t.Fatalf("error fetching results received. expected %v got %v", nil, err) + } + result := campaign.Results[0] rid := fmt.Sprintf("%s%s", result.RId, TransparencySuffix) transparencyRequest(t, ctx, result, rid, "/") diff --git a/models/campaign.go b/models/campaign.go index a9e24382..93318b05 100644 --- a/models/campaign.go +++ b/models/campaign.go @@ -176,59 +176,73 @@ func AddEvent(e *Event, campaignID int64) error { return db.Save(e).Error } -// getDetails retrieves the related attributes of the campaign -// from the database. If the Events and the Results are not available, -// an error is returned. Otherwise, the attribute name is set to [Deleted], -// indicating the user deleted the attribute (template, smtp, etc.) -func (c *Campaign) getDetails() error { +// FetchEvents retrieves the related Events attribute of the campaign. +// If the Events are not available, an error is returned. +func (c *Campaign) FetchEvents() error { + err := db.Model(c).Related(&c.Events).Error + if err != nil { + return err + } + return nil +} + +// FetchResults retrieves the related Results attribute of the campaign. +// If the Results are not available, an error is returned. +func (c *Campaign) FetchResults() error { err := db.Model(c).Related(&c.Results).Error if err != nil { - log.Warnf("%s: results not found for campaign", err) return err } - err = db.Model(c).Related(&c.Events).Error - if err != nil { - log.Warnf("%s: events not found for campaign", err) - return err - } - err = db.Table("templates").Where("id=?", c.TemplateId).Find(&c.Template).Error - if err != nil { - if err != gorm.ErrRecordNotFound { - return err - } - c.Template = Template{Name: "[Deleted]"} - log.Warnf("%s: template not found for campaign", err) - } - err = db.Where("template_id=?", c.Template.Id).Find(&c.Template.Attachments).Error - if err != nil && err != gorm.ErrRecordNotFound { - log.Warn(err) - return err - } - err = db.Table("pages").Where("id=?", c.PageId).Find(&c.Page).Error - if err != nil { - if err != gorm.ErrRecordNotFound { - return err - } - c.Page = Page{Name: "[Deleted]"} - log.Warnf("%s: page not found for campaign", err) - } - err = db.Table("smtp").Where("id=?", c.SMTPId).Find(&c.SMTP).Error + return nil +} + +// FetchSMTP retrieves the related SMTP attribute of the campaign. +// If SMTP is not available, the attribute name is set to [Deleted], +// indicating the user deleted the attribute. +func (c *Campaign) FetchSMTP() error { + err := db.Table("smtp").Where("id=?", c.SMTPId).Find(&c.SMTP).Error if err != nil { // Check if the SMTP was deleted if err != gorm.ErrRecordNotFound { return err } c.SMTP = SMTP{Name: "[Deleted]"} - log.Warnf("%s: sending profile not found for campaign", err) } err = db.Where("smtp_id=?", c.SMTP.Id).Find(&c.SMTP.Headers).Error if err != nil && err != gorm.ErrRecordNotFound { - log.Warn(err) return err } return nil } +// FetchTemplate retrieves the related Template attribute of the campaign. +// If SMTP is not available, the attribute name is set to [Deleted], +// indicating the user deleted the attribute. +func (c *Campaign) FetchTemplate() error { + err := db.Table("templates").Where("id=?", c.TemplateId).Find(&c.Template).Error + if err != nil { + if err != gorm.ErrRecordNotFound { + return err + } + c.Template = Template{Name: "[Deleted]"} + } + return nil +} + +// FetchPage retrieves the related Page attribute of the campaign. +// If SMTP is not available, the attribute name is set to [Deleted], +// indicating the user deleted the attribute. +func (c *Campaign) FetchPage() error { + err := db.Table("pages").Where("id=?", c.PageId).Find(&c.Page).Error + if err != nil { + if err != gorm.ErrRecordNotFound { + return err + } + c.Page = Page{Name: "[Deleted]"} + } + return nil +} + // getBaseURL returns the Campaign's configured URL. // This is used to implement the TemplateContext interface. func (c *Campaign) getBaseURL() string { @@ -238,6 +252,10 @@ func (c *Campaign) getBaseURL() string { // getFromAddress returns the Campaign's configured SMTP "From" address. // This is used to implement the TemplateContext interface. func (c *Campaign) getFromAddress() string { + err := c.FetchSMTP() + if err != nil { + log.Warnf("%s: sending profile not found for campaign", err) + } return c.SMTP.FromAddress } @@ -308,12 +326,6 @@ func GetCampaigns(uid int64) ([]Campaign, error) { if err != nil { log.Error(err) } - for i := range cs { - err = cs[i].getDetails() - if err != nil { - log.Error(err) - } - } return cs, err } @@ -402,7 +414,6 @@ func GetCampaign(id int64, uid int64) (Campaign, error) { log.Errorf("%s: campaign not found", err) return c, err } - err = c.getDetails() return c, err } @@ -439,12 +450,6 @@ func GetQueuedCampaigns(t time.Time) ([]Campaign, error) { log.Error(err) } log.Infof("Found %d Campaigns to run\n", len(cs)) - for i := range cs { - err = cs[i].getDetails() - if err != nil { - log.Error(err) - } - } return cs, err } diff --git a/models/campaign_test.go b/models/campaign_test.go index 6491987e..3d48ddcd 100644 --- a/models/campaign_test.go +++ b/models/campaign_test.go @@ -121,6 +121,10 @@ func (s *ModelsSuite) TestDeleteCampaignAlsoDeletesMailLogs(c *check.C) { campaign := s.createCampaign(c) ms, err := GetMailLogsByCampaign(campaign.Id) c.Assert(err, check.Equals, nil) + + err = campaign.FetchResults() + c.Assert(err, check.Equals, nil) + c.Assert(len(ms), check.Equals, len(campaign.Results)) err = DeleteCampaign(campaign.Id) @@ -135,6 +139,10 @@ func (s *ModelsSuite) TestCompleteCampaignAlsoDeletesMailLogs(c *check.C) { campaign := s.createCampaign(c) ms, err := GetMailLogsByCampaign(campaign.Id) c.Assert(err, check.Equals, nil) + + err = campaign.FetchResults() + c.Assert(err, check.Equals, nil) + c.Assert(len(ms), check.Equals, len(campaign.Results)) err = CompleteCampaign(campaign.Id, campaign.UserId) diff --git a/models/maillog.go b/models/maillog.go index 7ab3a2ef..9bc52891 100644 --- a/models/maillog.go +++ b/models/maillog.go @@ -160,7 +160,7 @@ func (m *MailLog) GetSmtpFrom() (string, error) { return "", err } - f, err := mail.ParseAddress(c.SMTP.FromAddress) + f, err := mail.ParseAddress(c.getFromAddress()) return f.Address, err } @@ -184,7 +184,7 @@ func (m *MailLog) Generate(msg *gomail.Message) error { f, err := mail.ParseAddress(c.Template.EnvelopeSender) if err != nil { - f, err = mail.ParseAddress(c.SMTP.FromAddress) + f, err = mail.ParseAddress(c.getFromAddress()) if err != nil { return err } diff --git a/models/maillog_test.go b/models/maillog_test.go index 88619f93..05df2462 100644 --- a/models/maillog_test.go +++ b/models/maillog_test.go @@ -17,9 +17,12 @@ import ( ) func (s *ModelsSuite) emailFromFirstMailLog(campaign Campaign, ch *check.C) *email.Email { + err := campaign.FetchResults() + ch.Assert(err, check.Equals, nil) + result := campaign.Results[0] m := &MailLog{} - err := db.Where("r_id=? AND campaign_id=?", result.RId, campaign.Id). + err = db.Where("r_id=? AND campaign_id=?", result.RId, campaign.Id). Find(m).Error ch.Assert(err, check.Equals, nil) @@ -65,9 +68,13 @@ func (s *ModelsSuite) TestGetQueuedMailLogs(ch *check.C) { func (s *ModelsSuite) TestMailLogBackoff(ch *check.C) { campaign := s.createCampaign(ch) + + err := campaign.FetchResults() + ch.Assert(err, check.Equals, nil) + result := campaign.Results[0] m := &MailLog{} - err := db.Where("r_id=? AND campaign_id=?", result.RId, campaign.Id). + err = db.Where("r_id=? AND campaign_id=?", result.RId, campaign.Id). Find(m).Error ch.Assert(err, check.Equals, nil) ch.Assert(m.SendAttempt, check.Equals, 0) @@ -97,6 +104,9 @@ func (s *ModelsSuite) TestMailLogBackoff(ch *check.C) { campaign, err = GetCampaign(campaign.Id, int64(1)) ch.Assert(err, check.Equals, nil) + err = campaign.FetchEvents() + ch.Assert(err, check.Equals, nil) + // We expect MaxSendAttempts + the initial campaign created event ch.Assert(len(campaign.Events), check.Equals, MaxSendAttempts+1) @@ -107,9 +117,13 @@ func (s *ModelsSuite) TestMailLogBackoff(ch *check.C) { func (s *ModelsSuite) TestMailLogError(ch *check.C) { campaign := s.createCampaign(ch) + + err := campaign.FetchResults() + ch.Assert(err, check.Equals, nil) + result := campaign.Results[0] m := &MailLog{} - err := db.Where("r_id=? AND campaign_id=?", result.RId, campaign.Id). + err = db.Where("r_id=? AND campaign_id=?", result.RId, campaign.Id). Find(m).Error ch.Assert(err, check.Equals, nil) ch.Assert(m.RId, check.Equals, result.RId) @@ -130,6 +144,9 @@ func (s *ModelsSuite) TestMailLogError(ch *check.C) { campaign, err = GetCampaign(campaign.Id, int64(1)) ch.Assert(err, check.Equals, nil) + err = campaign.FetchEvents() + ch.Assert(err, check.Equals, nil) + expectedEventLength := 2 ch.Assert(len(campaign.Events), check.Equals, expectedEventLength) @@ -148,14 +165,22 @@ func (s *ModelsSuite) TestMailLogError(ch *check.C) { ms, err := GetMailLogsByCampaign(campaign.Id) ch.Assert(err, check.Equals, nil) + + err = campaign.FetchResults() + ch.Assert(err, check.Equals, nil) + ch.Assert(len(ms), check.Equals, len(campaign.Results)-1) } func (s *ModelsSuite) TestMailLogSuccess(ch *check.C) { campaign := s.createCampaign(ch) + + err := campaign.FetchResults() + ch.Assert(err, check.Equals, nil) + result := campaign.Results[0] m := &MailLog{} - err := db.Where("r_id=? AND campaign_id=?", result.RId, campaign.Id). + err = db.Where("r_id=? AND campaign_id=?", result.RId, campaign.Id). Find(m).Error ch.Assert(err, check.Equals, nil) ch.Assert(m.RId, check.Equals, result.RId) @@ -172,6 +197,9 @@ func (s *ModelsSuite) TestMailLogSuccess(ch *check.C) { campaign, err = GetCampaign(campaign.Id, int64(1)) ch.Assert(err, check.Equals, nil) + err = campaign.FetchEvents() + ch.Assert(err, check.Equals, nil) + expectedEventLength := 2 ch.Assert(len(campaign.Events), check.Equals, expectedEventLength) @@ -188,6 +216,10 @@ func (s *ModelsSuite) TestMailLogSuccess(ch *check.C) { ms, err := GetMailLogsByCampaign(campaign.Id) ch.Assert(err, check.Equals, nil) + + err = campaign.FetchResults() + ch.Assert(err, check.Equals, nil) + ch.Assert(len(ms), check.Equals, len(campaign.Results)-1) } @@ -249,6 +281,10 @@ func (s *ModelsSuite) TestMailLogGetSmtpFrom(ch *check.C) { func (s *ModelsSuite) TestMailLogGenerate(ch *check.C) { campaign := s.createCampaign(ch) + + err := campaign.FetchResults() + ch.Assert(err, check.Equals, nil) + result := campaign.Results[0] expected := &email.Email{ From: "test@test.com", // Default smtp.FromAddress