From a569772e9c3117b8de09d0aa13883aa86bd9c233 Mon Sep 17 00:00:00 2001 From: Azareal Date: Sat, 4 Jan 2020 15:30:25 +1000 Subject: [PATCH] Add DisableRegLog configuration setting. Unit tests for DisablePostIP. Awkward but does the job for now. --- common/misc_logs.go | 16 ++++---- common/site.go | 1 + misc_test.go | 95 ++++++++++++++++++++++++++------------------- routes/account.go | 25 +++++++----- tickloop.go | 6 +++ 5 files changed, 87 insertions(+), 56 deletions(-) diff --git a/common/misc_logs.go b/common/misc_logs.go index 875d75c3..a3ffb115 100644 --- a/common/misc_logs.go +++ b/common/misc_logs.go @@ -4,7 +4,7 @@ import ( "database/sql" "time" - "github.com/Azareal/Gosora/query_gen" + qgen "github.com/Azareal/Gosora/query_gen" ) var RegLogs RegLogStore @@ -16,7 +16,7 @@ type RegLogItem struct { Email string FailureReason string Success bool - IP string + IP string DoneAt string } @@ -81,7 +81,7 @@ func (s *SQLRegLogStore) Count() (count int) { return count } -func (s *SQLRegLogStore) GetOffset(offset int, perPage int) (logs []RegLogItem, err error) { +func (s *SQLRegLogStore) GetOffset(offset, perPage int) (logs []RegLogItem, err error) { rows, err := s.getOffset.Query(offset, perPage) if err != nil { return logs, err @@ -102,11 +102,11 @@ func (s *SQLRegLogStore) GetOffset(offset int, perPage int) (logs []RegLogItem, } type LoginLogItem struct { - ID int - UID int - Success bool - IP string - DoneAt string + ID int + UID int + Success bool + IP string + DoneAt string } type LoginLogStmts struct { diff --git a/common/site.go b/common/site.go index 5a751864..af59ab81 100644 --- a/common/site.go +++ b/common/site.go @@ -99,6 +99,7 @@ type config struct { DisableLastIP bool DisablePostIP bool DisablePollIP bool + DisableRegLog bool DisableLiveTopicList bool DisableJSAntispam bool diff --git a/misc_test.go b/misc_test.go index 82b264c5..b4a037de 100644 --- a/misc_test.go +++ b/misc_test.go @@ -449,12 +449,19 @@ func TestTopicStore(t *testing.T) { tcache := c.NewMemoryTopicCache(c.Config.TopicCacheCapacity) c.Topics, err = c.NewDefaultTopicStore(tcache) expectNilErr(t, err) - topicStoreTest(t, 2) + c.Config.DisablePostIP = false + topicStoreTest(t, 2, "::1") + c.Config.DisablePostIP = true + topicStoreTest(t, 3, "0") + c.Topics, err = c.NewDefaultTopicStore(nil) expectNilErr(t, err) - topicStoreTest(t, 3) + c.Config.DisablePostIP = false + topicStoreTest(t, 4, "::1") + c.Config.DisablePostIP = true + topicStoreTest(t, 5, "0") } -func topicStoreTest(t *testing.T, newID int) { +func topicStoreTest(t *testing.T, newID int, ip string) { var topic *c.Topic var err error @@ -469,18 +476,15 @@ func topicStoreTest(t *testing.T, newID int) { // TODO: Add BulkGetMap() to the TopicStore - ok := c.Topics.Exists(-1) - expect(t, !ok, "TID #-1 shouldn't exist") - ok = c.Topics.Exists(0) - expect(t, !ok, "TID #0 shouldn't exist") - ok = c.Topics.Exists(1) - expect(t, ok, "TID #1 should exist") + expect(t, !c.Topics.Exists(-1), "TID #-1 shouldn't exist") + expect(t, !c.Topics.Exists(0), "TID #0 shouldn't exist") + expect(t, c.Topics.Exists(1), "TID #1 should exist") count := c.Topics.Count() expect(t, count == 1, fmt.Sprintf("Global count for topics should be 1, not %d", count)) //Create(fid int, topicName string, content string, uid int, ip string) (tid int, err error) - tid, err := c.Topics.Create(2, "Test Topic", "Topic Content", 1, "::1") + tid, err := c.Topics.Create(2, "Test Topic", "Topic Content", 1, ip) expectNilErr(t, err) expect(t, tid == newID, fmt.Sprintf("TID for the new topic should be %d, not %d", newID, tid)) expect(t, c.Topics.Exists(newID), fmt.Sprintf("TID #%d should exist", newID)) @@ -522,27 +526,27 @@ func topicStoreTest(t *testing.T, newID int) { expectNilErr(t, err) } - testTopic(newID, "Test Topic", "Topic Content", 1, "::1", 2, false, false) + testTopic(newID, "Test Topic", "Topic Content", 1, ip, 2, false, false) expectNilErr(t, topic.Lock()) shouldNotBeIn(newID) - testTopic(newID, "Test Topic", "Topic Content", 1, "::1", 2, true, false) + testTopic(newID, "Test Topic", "Topic Content", 1, ip, 2, true, false) expectNilErr(t, topic.Unlock()) shouldNotBeIn(newID) - testTopic(newID, "Test Topic", "Topic Content", 1, "::1", 2, false, false) + testTopic(newID, "Test Topic", "Topic Content", 1, ip, 2, false, false) expectNilErr(t, topic.Stick()) shouldNotBeIn(newID) - testTopic(newID, "Test Topic", "Topic Content", 1, "::1", 2, false, true) + testTopic(newID, "Test Topic", "Topic Content", 1, ip, 2, false, true) expectNilErr(t, topic.Unstick()) shouldNotBeIn(newID) - testTopic(newID, "Test Topic", "Topic Content", 1, "::1", 2, false, false) + testTopic(newID, "Test Topic", "Topic Content", 1, ip, 2, false, false) expectNilErr(t, topic.MoveTo(1)) shouldNotBeIn(newID) - testTopic(newID, "Test Topic", "Topic Content", 1, "::1", 1, false, false) + testTopic(newID, "Test Topic", "Topic Content", 1, ip, 1, false, false) // TODO: Add more tests for more *Topic methods expectNilErr(t, topic.Delete()) @@ -907,6 +911,13 @@ func TestReplyStore(t *testing.T) { _, err = c.Rstore.Get(0) recordMustNotExist(t, err, "RID #0 shouldn't exist") + c.Config.DisablePostIP = false + testReplyStore(t, 2, 1, "::1") + c.Config.DisablePostIP = true + testReplyStore(t, 5, 3, "0") +} + +func testReplyStore(t *testing.T, newID int, newPostCount int, ip string) { replyTest2 := func(reply *c.Reply, err error, rid int, parentID int, createdBy int, content string, ip string) { expectNilErr(t, err) expect(t, reply.ID == rid, fmt.Sprintf("RID #%d has the wrong ID. It should be %d not %d", rid, rid, reply.ID)) @@ -928,42 +939,42 @@ func TestReplyStore(t *testing.T) { //_, err = c.Rstore.GetCache().Get(1) //recordMustNotExist(t, err, "RID #1 shouldn't be in the cache") - _, err = c.Rstore.Get(2) + _, err := c.Rstore.Get(newID) recordMustNotExist(t, err, "RID #2 shouldn't exist") topic, err := c.Topics.Get(1) expectNilErr(t, err) - expect(t, topic.PostCount == 1, fmt.Sprintf("TID #1's post count should be one, not %d", topic.PostCount)) + expect(t, topic.PostCount == newPostCount, fmt.Sprintf("TID #%d's post count should be %d, not %d", topic.ID, newPostCount, topic.PostCount)) - _, err = c.Rstore.GetCache().Get(2) - recordMustNotExist(t, err, "RID #2 shouldn't be in the cache") + _, err = c.Rstore.GetCache().Get(newID) + recordMustNotExist(t, err, "RID #%d shouldn't be in the cache", newID) - rid, err := c.Rstore.Create(topic, "Fofofo", "::1", 1) + rid, err := c.Rstore.Create(topic, "Fofofo", ip, 1) expectNilErr(t, err) - expect(t, rid == 2, fmt.Sprintf("The next reply ID should be 2 not %d", rid)) - expect(t, topic.PostCount == 1, fmt.Sprintf("The old TID #1 in memory's post count should be one, not %d", topic.PostCount)) + expect(t, rid == newID, fmt.Sprintf("The next reply ID should be %d not %d", newID, rid)) + expect(t, topic.PostCount == newPostCount, fmt.Sprintf("The old TID #1 in memory's post count should be %d, not %d", newPostCount+1, topic.PostCount)) // TODO: Test the reply count on the topic - replyTest(2, 1, 1, "Fofofo", "::1") + replyTest(newID, 1, 1, "Fofofo", ip) topic, err = c.Topics.Get(1) expectNilErr(t, err) - expect(t, topic.PostCount == 2, fmt.Sprintf("TID #1's post count should be two, not %d", topic.PostCount)) + expect(t, topic.PostCount == newPostCount+1, fmt.Sprintf("TID #1's post count should be %d, not %d", newPostCount+1, topic.PostCount)) - err = topic.CreateActionReply("destroy", "::1", 1) + err = topic.CreateActionReply("destroy", ip, 1) expectNilErr(t, err) - expect(t, topic.PostCount == 2, fmt.Sprintf("The old TID #1 in memory's post count should be two, not %d", topic.PostCount)) - replyTest(3, 1, 1, "", "::1") + expect(t, topic.PostCount == newPostCount+1, fmt.Sprintf("The old TID #1 in memory's post count should be %d, not %d", newPostCount+1, topic.PostCount)) + replyTest(newID+1, 1, 1, "", ip) // TODO: Check the actionType field of the reply, this might not be loaded by TopicStore, maybe we should add it there? topic, err = c.Topics.Get(1) expectNilErr(t, err) - expect(t, topic.PostCount == 3, fmt.Sprintf("TID #1's post count should be three, not %d", topic.PostCount)) + expect(t, topic.PostCount == newPostCount+2, fmt.Sprintf("TID #1's post count should be %d, not %d", newPostCount+2, topic.PostCount)) // TODO: Expand upon this - rid, err = c.Rstore.Create(topic, "hiii", "::1", 1) + rid, err = c.Rstore.Create(topic, "hiii", ip, 1) expectNilErr(t, err) - replyTest(rid, topic.ID, 1, "hiii", "::1") + replyTest(rid, topic.ID, 1, "hiii", ip) reply, err := c.Rstore.Get(rid) expectNilErr(t, err) @@ -1001,28 +1012,34 @@ func TestProfileReplyStore(t *testing.T) { _, err = c.Prstore.Get(1) recordMustNotExist(t, err, "PRID #1 shouldn't exist") + c.Config.DisablePostIP = false + testProfileReplyStore(t, 1, "::1") + c.Config.DisablePostIP = true + testProfileReplyStore(t, 2, "0") +} +func testProfileReplyStore(t *testing.T, newID int, ip string) { // ? - Commented this one out as strong constraints like this put an unreasonable load on the database, we only want errors if a delete which should succeed fails //profileReply := c.BlankProfileReply(1) //err = profileReply.Delete() //expect(t,err != nil,"You shouldn't be able to delete profile replies which don't exist") profileID := 1 - prid, err := c.Prstore.Create(profileID, "Haha", 1, "::1") + prid, err := c.Prstore.Create(profileID, "Haha", 1, ip) expectNilErr(t, err) - expect(t, prid == 1, "The first profile reply should have an ID of 1") + expect(t, prid == newID, fmt.Sprintf("The first profile reply should have an ID of %d", newID)) - profileReply, err := c.Prstore.Get(1) + profileReply, err := c.Prstore.Get(newID) expectNilErr(t, err) - expect(t, profileReply.ID == 1, fmt.Sprintf("The profile reply should have an ID of 1 not %d", profileReply.ID)) + expect(t, profileReply.ID == newID, fmt.Sprintf("The profile reply should have an ID of %d not %d", newID, profileReply.ID)) expect(t, profileReply.ParentID == 1, fmt.Sprintf("The parent ID of the profile reply should be 1 not %d", profileReply.ParentID)) expect(t, profileReply.Content == "Haha", fmt.Sprintf("The profile reply's contents should be 'Haha' not '%s'", profileReply.Content)) expect(t, profileReply.CreatedBy == 1, fmt.Sprintf("The profile reply's creator should be 1 not %d", profileReply.CreatedBy)) - expect(t, profileReply.IP == "::1", fmt.Sprintf("The profile reply's IP should be '::1' not '%s'", profileReply.IP)) + expect(t, profileReply.IP == ip, fmt.Sprintf("The profile reply's IP should be '%s' not '%s'", ip, profileReply.IP)) err = profileReply.Delete() expectNilErr(t, err) - _, err = c.Prstore.Get(1) - expect(t, err != nil, "PRID #1 shouldn't exist after being deleted") + _, err = c.Prstore.Get(newID) + expect(t, err != nil, fmt.Sprintf("PRID #%d shouldn't exist after being deleted", newID)) // TODO: Test profileReply.SetBody() and profileReply.Creator() } @@ -1447,7 +1464,7 @@ func TestWidgets(t *testing.T) { ewidget := &c.WidgetEdit{widget, map[string]string{"Name": "Test", "Text": "Testing"}} wid, err := ewidget.Create() expectNilErr(t, err) - expect(t,wid==1,"wid should be 1") + expect(t, wid == 1, "wid should be 1") // TODO: Do a test for the widget body widget2, err := c.Widgets.Get(1) diff --git a/routes/account.go b/routes/account.go index 60243e2c..3df433e3 100644 --- a/routes/account.go +++ b/routes/account.go @@ -262,9 +262,11 @@ func AccountRegisterSubmit(w http.ResponseWriter, r *http.Request, user c.User) } regLog := c.RegLogItem{Username: name, Email: email, FailureReason: regErrReason, Success: regSuccess, IP: user.GetIP()} - _, err = regLog.Create() - if err != nil { - return c.InternalError(err, w, r) + if !c.Config.DisableRegLog { + _, err := regLog.Create() + if err != nil { + return c.InternalError(err, w, r) + } } if !regSuccess { return c.LocalError(regErrMsg, w, r, user) @@ -280,27 +282,32 @@ func AccountRegisterSubmit(w http.ResponseWriter, r *http.Request, user c.User) group = c.Config.ActivationGroup } + addReason := func(reason string) error { + if c.Config.DisableRegLog { + return nil + } + regLog.FailureReason += reason + return regLog.Commit() + } + // TODO: Do the registration attempt logging a little less messily (without having to amend the result after the insert) uid, err := c.Users.Create(name, password, email, group, active) if err != nil { regLog.Success = false if err == c.ErrAccountExists { - regLog.FailureReason += "username-exists" - err = regLog.Commit() + err = addReason("username-exists") if err != nil { return c.InternalError(err, w, r) } return c.LocalError(p.GetErrorPhrase("register_username_unavailable"), w, r, user) } else if err == c.ErrLongUsername { - regLog.FailureReason += "username-too-long" - err = regLog.Commit() + err = addReason("username-too-long") if err != nil { return c.InternalError(err, w, r) } return c.LocalError(p.GetErrorPhrase("register_username_too_long_prefix")+strconv.Itoa(c.Config.MaxUsernameLength), w, r, user) } - regLog.FailureReason += "internal-error" - err2 := regLog.Commit() + err2 := addReason("internal-error") if err2 != nil { return c.InternalError(err2, w, r) } diff --git a/tickloop.go b/tickloop.go index 821d9952..36e564b8 100644 --- a/tickloop.go +++ b/tickloop.go @@ -170,6 +170,12 @@ func asmMatches() { func dailies() { asmMatches() + if c.Config.DisableRegLog { + _, err := qgen.NewAcc().Purge("registration_logs").Exec() + if err != nil { + c.LogError(err) + } + } if c.Config.LogPruneCutoff > -1 { f := func(tbl string) { _, err := qgen.NewAcc().Delete(tbl).DateOlderThan("doneAt", c.Config.LogPruneCutoff, "day").Run()