From 19b9650595d0725b3dd1563454ab1b9a831586a4 Mon Sep 17 00:00:00 2001 From: Umputun Date: Thu, 28 Dec 2023 00:41:28 -0600 Subject: [PATCH] delete aproved user from the persistent storage too --- app/bot/mocks/approved_store.go | 84 ++++++++++++++++++++++++++++++ app/bot/spam.go | 14 ++++- app/bot/spam_test.go | 25 ++++----- app/main.go | 6 +-- app/main_test.go | 10 ++-- app/storage/approved_users.go | 14 +++++ app/storage/approved_users_test.go | 54 +++++++++++++++++++ app/webapi/mocks/approved_users.go | 55 +++++++++++++++++++ app/webapi/webapi.go | 15 ++++-- app/webapi/webapi_test.go | 6 ++- 10 files changed, 259 insertions(+), 24 deletions(-) create mode 100644 app/bot/mocks/approved_store.go diff --git a/app/bot/mocks/approved_store.go b/app/bot/mocks/approved_store.go new file mode 100644 index 00000000..0fe2c331 --- /dev/null +++ b/app/bot/mocks/approved_store.go @@ -0,0 +1,84 @@ +// Code generated by moq; DO NOT EDIT. +// github.com/matryer/moq + +package mocks + +import ( + "sync" +) + +// ApprovedUsersStoreMock is a mock implementation of bot.ApprovedUsersStore. +// +// func TestSomethingThatUsesApprovedUsersStore(t *testing.T) { +// +// // make and configure a mocked bot.ApprovedUsersStore +// mockedApprovedUsersStore := &ApprovedUsersStoreMock{ +// DeleteFunc: func(id string) error { +// panic("mock out the Delete method") +// }, +// } +// +// // use mockedApprovedUsersStore in code that requires bot.ApprovedUsersStore +// // and then make assertions. +// +// } +type ApprovedUsersStoreMock struct { + // DeleteFunc mocks the Delete method. + DeleteFunc func(id string) error + + // calls tracks calls to the methods. + calls struct { + // Delete holds details about calls to the Delete method. + Delete []struct { + // ID is the id argument value. + ID string + } + } + lockDelete sync.RWMutex +} + +// Delete calls DeleteFunc. +func (mock *ApprovedUsersStoreMock) Delete(id string) error { + if mock.DeleteFunc == nil { + panic("ApprovedUsersStoreMock.DeleteFunc: method is nil but ApprovedUsersStore.Delete was just called") + } + callInfo := struct { + ID string + }{ + ID: id, + } + mock.lockDelete.Lock() + mock.calls.Delete = append(mock.calls.Delete, callInfo) + mock.lockDelete.Unlock() + return mock.DeleteFunc(id) +} + +// DeleteCalls gets all the calls that were made to Delete. +// Check the length with: +// +// len(mockedApprovedUsersStore.DeleteCalls()) +func (mock *ApprovedUsersStoreMock) DeleteCalls() []struct { + ID string +} { + var calls []struct { + ID string + } + mock.lockDelete.RLock() + calls = mock.calls.Delete + mock.lockDelete.RUnlock() + return calls +} + +// ResetDeleteCalls reset all the calls that were made to Delete. +func (mock *ApprovedUsersStoreMock) ResetDeleteCalls() { + mock.lockDelete.Lock() + mock.calls.Delete = nil + mock.lockDelete.Unlock() +} + +// ResetCalls reset all the calls that were made to all mocked methods. +func (mock *ApprovedUsersStoreMock) ResetCalls() { + mock.lockDelete.Lock() + mock.calls.Delete = nil + mock.lockDelete.Unlock() +} diff --git a/app/bot/spam.go b/app/bot/spam.go index 2261e1af..a67a7cc9 100644 --- a/app/bot/spam.go +++ b/app/bot/spam.go @@ -19,11 +19,13 @@ import ( ) //go:generate moq --out mocks/detector.go --pkg mocks --skip-ensure --with-resets . Detector +//go:generate moq --out mocks/approved_store.go --pkg mocks --skip-ensure --with-resets . ApprovedUsersStore // SpamFilter bot checks if a user is a spammer using lib.Detector // Reloads spam samples, stop words and excluded tokens on file change. type SpamFilter struct { Detector + ApprovedUsersStore params SpamConfig } @@ -58,9 +60,14 @@ type Detector interface { ApprovedUsers() (res []string) } +// ApprovedUsersStore is a storage interface for approved users. +type ApprovedUsersStore interface { + Delete(id string) error +} + // NewSpamFilter creates new spam filter -func NewSpamFilter(ctx context.Context, detector Detector, params SpamConfig) *SpamFilter { - res := &SpamFilter{Detector: detector, params: params} +func NewSpamFilter(ctx context.Context, detector Detector, aStore ApprovedUsersStore, params SpamConfig) *SpamFilter { + res := &SpamFilter{Detector: detector, params: params, ApprovedUsersStore: aStore} go func() { if err := res.watch(ctx, params.WatchDelay); err != nil { log.Printf("[WARN] samples file watcher failed: %v", err) @@ -134,6 +141,9 @@ func (s *SpamFilter) RemoveApprovedUsers(id int64, ids ...int64) { sids := make([]string, len(combinedIDs)) for i, id := range combinedIDs { sids[i] = strconv.FormatInt(id, 10) + if err := s.ApprovedUsersStore.Delete(sids[i]); err != nil { + log.Printf("[WARN] failed to delete approved user from storage %q: %v", sids[i], err) + } } s.Detector.RemoveApprovedUsers(sids...) } diff --git a/app/bot/spam_test.go b/app/bot/spam_test.go index b45a7295..22e09c00 100644 --- a/app/bot/spam_test.go +++ b/app/bot/spam_test.go @@ -31,7 +31,7 @@ func TestSpamFilter_OnMessage(t *testing.T) { } t.Run("spam detected", func(t *testing.T) { - s := NewSpamFilter(ctx, det, SpamConfig{SpamMsg: "detected", SpamDryMsg: "detected dry"}) + s := NewSpamFilter(ctx, det, nil, SpamConfig{SpamMsg: "detected", SpamDryMsg: "detected dry"}) resp := s.OnMessage(Message{Text: "spam", From: User{ID: 1, Username: "john"}}) assert.Equal(t, Response{Text: `detected: "john" (1)`, Send: true, BanInterval: PermanentBanDuration, User: User{ID: 1, Username: "john"}, DeleteReplyTo: true, @@ -40,7 +40,7 @@ func TestSpamFilter_OnMessage(t *testing.T) { }) t.Run("spam detected, dry", func(t *testing.T) { - s := NewSpamFilter(ctx, det, SpamConfig{SpamMsg: "detected", SpamDryMsg: "detected dry", Dry: true}) + s := NewSpamFilter(ctx, det, nil, SpamConfig{SpamMsg: "detected", SpamDryMsg: "detected dry", Dry: true}) resp := s.OnMessage(Message{Text: "spam", From: User{ID: 1, Username: "john"}}) assert.Equal(t, `detected dry: "john" (1)`, resp.Text) assert.True(t, resp.Send) @@ -48,7 +48,7 @@ func TestSpamFilter_OnMessage(t *testing.T) { }) t.Run("ham detected", func(t *testing.T) { - s := NewSpamFilter(ctx, det, SpamConfig{SpamMsg: "detected", SpamDryMsg: "detected dry"}) + s := NewSpamFilter(ctx, det, nil, SpamConfig{SpamMsg: "detected", SpamDryMsg: "detected dry"}) resp := s.OnMessage(Message{Text: "good", From: User{ID: 1, Username: "john"}}) assert.Equal(t, Response{CheckResults: []lib.CheckResult{{Name: "already approved", Spam: false, Details: "some ham"}}}, resp) }) @@ -151,7 +151,7 @@ func TestSpamFilter_reloadSamples(t *testing.T) { HamDynamicFile: "optional", } tc.modify(¶ms) - s := NewSpamFilter(ctx, mockDirector, params) + s := NewSpamFilter(ctx, mockDirector, nil, params) err = s.ReloadSamples() @@ -201,7 +201,7 @@ func TestSpamFilter_watch(t *testing.T) { _, err = os.Create(stopWordsFile) require.NoError(t, err) - NewSpamFilter(ctx, mockDetector, SpamConfig{ + NewSpamFilter(ctx, mockDetector, nil, SpamConfig{ ExcludedTokensFile: excludedTokensFile, SpamSamplesFile: spamSamplesFile, HamSamplesFile: hamSamplesFile, @@ -270,7 +270,7 @@ func TestSpamFilter_WatchMultipleUpdates(t *testing.T) { _, err = os.Create(stopWordsFile) require.NoError(t, err) - NewSpamFilter(ctx, mockDetector, SpamConfig{ + NewSpamFilter(ctx, mockDetector, nil, SpamConfig{ ExcludedTokensFile: excludedTokensFile, SpamSamplesFile: spamSamplesFile, HamSamplesFile: hamSamplesFile, @@ -318,7 +318,7 @@ func TestSpamFilter_Update(t *testing.T) { }, } - sf := NewSpamFilter(ctx, mockDetector, SpamConfig{}) + sf := NewSpamFilter(ctx, mockDetector, nil, SpamConfig{}) t.Run("good update", func(t *testing.T) { mockDetector.ResetCalls() @@ -385,10 +385,11 @@ func TestSpamFilter_AddApprovedUsers(t *testing.T) { func TestSpamFilter_RemoveApprovedUsers(t *testing.T) { mockDirector := &mocks.DetectorMock{RemoveApprovedUsersFunc: func(ids ...string) {}} + mockApprovedStore := &mocks.ApprovedUsersStoreMock{DeleteFunc: func(id string) error { return nil }} t.Run("remove single approved user", func(t *testing.T) { mockDirector.ResetCalls() - sf := SpamFilter{Detector: mockDirector} + sf := SpamFilter{Detector: mockDirector, ApprovedUsersStore: mockApprovedStore} sf.RemoveApprovedUsers(1) require.Equal(t, 1, len(mockDirector.RemoveApprovedUsersCalls())) assert.Equal(t, []string{"1"}, mockDirector.RemoveApprovedUsersCalls()[0].Ids) @@ -396,7 +397,7 @@ func TestSpamFilter_RemoveApprovedUsers(t *testing.T) { t.Run("remove multiple approved users", func(t *testing.T) { mockDirector.ResetCalls() - sf := SpamFilter{Detector: mockDirector} + sf := SpamFilter{Detector: mockDirector, ApprovedUsersStore: mockApprovedStore} sf.RemoveApprovedUsers(1, 2, 3) require.Equal(t, 1, len(mockDirector.RemoveApprovedUsersCalls())) assert.Equal(t, []string{"1", "2", "3"}, mockDirector.RemoveApprovedUsersCalls()[0].Ids) @@ -404,7 +405,7 @@ func TestSpamFilter_RemoveApprovedUsers(t *testing.T) { t.Run("remove empty list of approved users", func(t *testing.T) { mockDirector.ResetCalls() - sf := SpamFilter{Detector: mockDirector} + sf := SpamFilter{Detector: mockDirector, ApprovedUsersStore: mockApprovedStore} sf.RemoveApprovedUsers(1, 2, 3) require.Equal(t, 1, len(mockDirector.RemoveApprovedUsersCalls())) assert.Equal(t, []string{"1", "2", "3"}, mockDirector.RemoveApprovedUsersCalls()[0].Ids) @@ -428,7 +429,7 @@ func TestSpamFilter_DynamicSamples(t *testing.T) { spamFile.Close() hamFile.Close() - sf := NewSpamFilter(context.Background(), &mocks.DetectorMock{}, SpamConfig{ + sf := NewSpamFilter(context.Background(), &mocks.DetectorMock{}, nil, SpamConfig{ SpamDynamicFile: spamFile.Name(), HamDynamicFile: hamFile.Name(), }) @@ -495,7 +496,7 @@ func TestSpamFilter_RemoveDynamiSample(t *testing.T) { _, err = hamFile.WriteString("ham1\nham2\n") require.NoError(t, err) - return NewSpamFilter(context.Background(), mockDirector, SpamConfig{ + return NewSpamFilter(context.Background(), mockDirector, nil, SpamConfig{ SpamDynamicFile: spamFile.Name(), HamDynamicFile: hamFile.Name(), SpamSamplesFile: spamSamplesFile, diff --git a/app/main.go b/app/main.go index c6eb43a9..a93c48e1 100644 --- a/app/main.go +++ b/app/main.go @@ -201,7 +201,7 @@ func execute(ctx context.Context, opts options) error { }() // make spam bot - spamBot, err := makeSpamBot(ctx, opts, detector) + spamBot, err := makeSpamBot(ctx, opts, detector, approvedUsersStore) if err != nil { return fmt.Errorf("can't make spam bot, %w", err) } @@ -391,7 +391,7 @@ func makeDetector(opts options) *lib.Detector { return detector } -func makeSpamBot(ctx context.Context, opts options, detector *lib.Detector) (*bot.SpamFilter, error) { +func makeSpamBot(ctx context.Context, opts options, detector *lib.Detector, aStore *storage.ApprovedUsers) (*bot.SpamFilter, error) { spamBotParams := bot.SpamConfig{ SpamSamplesFile: filepath.Join(opts.Files.SamplesDataPath, samplesSpamFile), HamSamplesFile: filepath.Join(opts.Files.SamplesDataPath, samplesHamFile), @@ -404,7 +404,7 @@ func makeSpamBot(ctx context.Context, opts options, detector *lib.Detector) (*bo SpamDryMsg: opts.Message.Dry, Dry: opts.Dry, } - spamBot := bot.NewSpamFilter(ctx, detector, spamBotParams) + spamBot := bot.NewSpamFilter(ctx, detector, aStore, spamBotParams) log.Printf("[DEBUG] spam bot config: %+v", spamBotParams) if err := spamBot.ReloadSamples(); err != nil { diff --git a/app/main_test.go b/app/main_test.go index 2aa01bab..1dfc5e61 100644 --- a/app/main_test.go +++ b/app/main_test.go @@ -189,7 +189,7 @@ func Test_makeSpamBot(t *testing.T) { t.Run("no options", func(t *testing.T) { var opts options - _, err := makeSpamBot(ctx, opts, nil) + _, err := makeSpamBot(ctx, opts, nil, nil) assert.Error(t, err) }) @@ -207,8 +207,12 @@ func Test_makeSpamBot(t *testing.T) { require.NoError(t, err) opts.Files.SamplesDataPath = tmpDir - - res, err := makeSpamBot(ctx, opts, makeDetector(opts)) + detector := makeDetector(opts) + db, err := storage.NewSqliteDB(":memory:") + require.NoError(t, err) + store, err := loadApprovedUsers(db, detector) + require.NoError(t, err) + res, err := makeSpamBot(ctx, opts, detector, store) assert.NoError(t, err) assert.NotNil(t, res) }) diff --git a/app/storage/approved_users.go b/app/storage/approved_users.go index db1200d3..11da7542 100644 --- a/app/storage/approved_users.go +++ b/app/storage/approved_users.go @@ -98,3 +98,17 @@ func (au *ApprovedUsers) Timestamp(id string) (time.Time, error) { } return timestamp, nil } + +// Delete deletes the given id from the storage +func (au *ApprovedUsers) Delete(id string) error { + idVal, err := strconv.ParseInt(id, 10, 64) + if err != nil { + return fmt.Errorf("failed to parse id %s: %w", id, err) + } + + _, err = au.db.Exec("DELETE FROM approved_users WHERE id = ?", idVal) + if err != nil { + return fmt.Errorf("failed to delete id %s: %w", id, err) + } + return nil +} diff --git a/app/storage/approved_users_test.go b/app/storage/approved_users_test.go index 85ddeab4..ddca864d 100644 --- a/app/storage/approved_users_test.go +++ b/app/storage/approved_users_test.go @@ -134,3 +134,57 @@ func TestApprovedUsers_Timestamp(t *testing.T) { assert.Error(t, err, "Should return error for non-existing ID") }) } + +func TestApprovedUsers_Delete(t *testing.T) { + db, err := sqlx.Open("sqlite", ":memory:") + require.NoError(t, err) + defer db.Close() + + au, err := NewApprovedUsers(db) + require.NoError(t, err) + + t.Run("existing ID", func(t *testing.T) { + err = au.Store([]string{"123"}) + require.NoError(t, err) + err = au.Store([]string{"456"}) + require.NoError(t, err) + + err = au.Delete("123") + require.NoError(t, err) + + var count int + err = db.Get(&count, "SELECT COUNT(*) FROM approved_users") + require.NoError(t, err) + assert.Equal(t, 1, count, "There should be only one record left") + }) + + t.Run("non-existing ID", func(t *testing.T) { + err = au.Store([]string{"123"}) + require.NoError(t, err) + err = au.Store([]string{"456"}) + require.NoError(t, err) + + err = au.Delete("789") + require.NoError(t, err) + + var count int + err = db.Get(&count, "SELECT COUNT(*) FROM approved_users") + require.NoError(t, err) + assert.Equal(t, 2, count, "There should be two records left") + }) + + t.Run("invalid ID", func(t *testing.T) { + err = au.Store([]string{"123"}) + require.NoError(t, err) + err = au.Store([]string{"456"}) + require.NoError(t, err) + + err = au.Delete("aaa789") + require.Error(t, err) + + var count int + err = db.Get(&count, "SELECT COUNT(*) FROM approved_users") + require.NoError(t, err) + assert.Equal(t, 2, count, "There should be two records left") + }) +} diff --git a/app/webapi/mocks/approved_users.go b/app/webapi/mocks/approved_users.go index c60f0318..a6a2c6c4 100644 --- a/app/webapi/mocks/approved_users.go +++ b/app/webapi/mocks/approved_users.go @@ -14,6 +14,9 @@ import ( // // // make and configure a mocked webapi.ApprovedUsersStore // mockedApprovedUsersStore := &ApprovedUsersStoreMock{ +// DeleteFunc: func(id string) error { +// panic("mock out the Delete method") +// }, // StoreFunc: func(ids []string) error { // panic("mock out the Store method") // }, @@ -27,6 +30,9 @@ import ( // // } type ApprovedUsersStoreMock struct { + // DeleteFunc mocks the Delete method. + DeleteFunc func(id string) error + // StoreFunc mocks the Store method. StoreFunc func(ids []string) error @@ -35,6 +41,11 @@ type ApprovedUsersStoreMock struct { // calls tracks calls to the methods. calls struct { + // Delete holds details about calls to the Delete method. + Delete []struct { + // ID is the id argument value. + ID string + } // Store holds details about calls to the Store method. Store []struct { // Ids is the ids argument value. @@ -46,10 +57,50 @@ type ApprovedUsersStoreMock struct { ID string } } + lockDelete sync.RWMutex lockStore sync.RWMutex lockTimestamp sync.RWMutex } +// Delete calls DeleteFunc. +func (mock *ApprovedUsersStoreMock) Delete(id string) error { + if mock.DeleteFunc == nil { + panic("ApprovedUsersStoreMock.DeleteFunc: method is nil but ApprovedUsersStore.Delete was just called") + } + callInfo := struct { + ID string + }{ + ID: id, + } + mock.lockDelete.Lock() + mock.calls.Delete = append(mock.calls.Delete, callInfo) + mock.lockDelete.Unlock() + return mock.DeleteFunc(id) +} + +// DeleteCalls gets all the calls that were made to Delete. +// Check the length with: +// +// len(mockedApprovedUsersStore.DeleteCalls()) +func (mock *ApprovedUsersStoreMock) DeleteCalls() []struct { + ID string +} { + var calls []struct { + ID string + } + mock.lockDelete.RLock() + calls = mock.calls.Delete + mock.lockDelete.RUnlock() + return calls +} + +// ResetDeleteCalls reset all the calls that were made to Delete. +func (mock *ApprovedUsersStoreMock) ResetDeleteCalls() { + mock.lockDelete.Lock() + mock.calls.Delete = nil + mock.lockDelete.Unlock() +} + // Store calls StoreFunc. func (mock *ApprovedUsersStoreMock) Store(ids []string) error { if mock.StoreFunc == nil { @@ -130,6 +181,10 @@ func (mock *ApprovedUsersStoreMock) ResetTimestampCalls() { // ResetCalls reset all the calls that were made to all mocked methods. func (mock *ApprovedUsersStoreMock) ResetCalls() { + mock.lockDelete.Lock() + mock.calls.Delete = nil + mock.lockDelete.Unlock() + mock.lockStore.Lock() mock.calls.Store = nil mock.lockStore.Unlock() diff --git a/app/webapi/webapi.go b/app/webapi/webapi.go index 8ddff959..6f2cc171 100644 --- a/app/webapi/webapi.go +++ b/app/webapi/webapi.go @@ -80,6 +80,7 @@ type Locator interface { type ApprovedUsersStore interface { Store(ids []string) error Timestamp(id string) (time.Time, error) + Delete(id string) error } // NewServer creates a new web API server. @@ -142,9 +143,17 @@ func (s *Server) routes(router *chi.Mux) *chi.Mux { authApi.Put("/samples", s.reloadDynamicSamplesHandler) // reload samples authApi.Route("/users", func(r chi.Router) { // manage approved users - r.Post("/add", s.updateApprovedUsersHandler(s.Detector.AddApprovedUsers)) // add user to the approved list - r.Post("/delete", s.updateApprovedUsersHandler(s.Detector.RemoveApprovedUsers)) // remove user from approved list - r.Get("/", s.getApprovedUsersHandler) // get approved users + r.Post("/add", s.updateApprovedUsersHandler(s.Detector.AddApprovedUsers)) // add user to the approved list + r.Post("/delete", s.updateApprovedUsersHandler(func(id ...string) { // remove user from approved list + s.Detector.RemoveApprovedUsers(id...) // remove user from the approved list + for _, userID := range id { + // remove user from the storage + if err := s.ApprovedUsersStore.Delete(userID); err != nil { + log.Printf("[WARN] failed to delete user %s: %v", userID, err) + } + } + })) + r.Get("/", s.getApprovedUsersHandler) // get approved users }) }) diff --git a/app/webapi/webapi_test.go b/app/webapi/webapi_test.go index da737da8..c3ba7fbd 100644 --- a/app/webapi/webapi_test.go +++ b/app/webapi/webapi_test.go @@ -155,7 +155,8 @@ func TestServer_routes(t *testing.T) { }, } approvedUsersMock := &mocks.ApprovedUsersStoreMock{ - StoreFunc: func(ids []string) error { return nil }, + StoreFunc: func(ids []string) error { return nil }, + DeleteFunc: func(id string) error { return nil }, } server := NewServer(Config{Detector: detectorMock, SpamFilter: spamFilterMock, @@ -291,6 +292,7 @@ func TestServer_routes(t *testing.T) { t.Run("remove user by id", func(t *testing.T) { detectorMock.ResetCalls() locatorMock.ResetCalls() + approvedUsersMock.ResetCalls() req, err := http.NewRequest("POST", ts.URL+"/users/delete", bytes.NewBuffer([]byte(`{"user_id" : "id1"}`))) require.NoError(t, err) @@ -301,6 +303,8 @@ func TestServer_routes(t *testing.T) { assert.Equal(t, "application/json; charset=utf-8", resp.Header.Get("Content-Type")) assert.Equal(t, 1, len(detectorMock.RemoveApprovedUsersCalls())) assert.Equal(t, []string{"id1"}, detectorMock.RemoveApprovedUsersCalls()[0].Ids) + assert.Equal(t, 1, len(approvedUsersMock.DeleteCalls())) + assert.Equal(t, "id1", approvedUsersMock.DeleteCalls()[0].ID) }) t.Run("remove user by name", func(t *testing.T) {