From 36a09dd0df06e1b6f64347ed3d3c2c8ca963fc73 Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Thu, 30 Sep 2021 10:56:02 +0200 Subject: [PATCH] handle remote account deletion more systematically (#254) --- internal/api/s2s/user/inboxpost_test.go | 107 ++++++++++++++++++++- internal/api/s2s/user/user_test.go | 2 + internal/federation/federatingdb/delete.go | 5 +- internal/processing/account/delete.go | 20 ++-- internal/processing/fromfederator.go | 8 +- internal/processing/fromfederator_test.go | 81 ++++++++++++++++ internal/processing/processor_test.go | 2 + 7 files changed, 212 insertions(+), 13 deletions(-) diff --git a/internal/api/s2s/user/inboxpost_test.go b/internal/api/s2s/user/inboxpost_test.go index 904586b98..7e08ff77d 100644 --- a/internal/api/s2s/user/inboxpost_test.go +++ b/internal/api/s2s/user/inboxpost_test.go @@ -322,7 +322,6 @@ func (suite *InboxPostTestSuite) TestPostUpdate() { suite.EqualValues(updatedAccount.HeaderMediaAttachmentID, dbUpdatedAccount.HeaderMediaAttachmentID) suite.EqualValues(updatedAccount.HeaderMediaAttachment, dbUpdatedAccount.HeaderMediaAttachment) suite.EqualValues(updatedAccount.HeaderRemoteURL, dbUpdatedAccount.HeaderRemoteURL) - // suite.EqualValues(updatedAccount.Fields, dbUpdatedAccount.Fields) suite.EqualValues(updatedAccount.Note, dbUpdatedAccount.Note) suite.EqualValues(updatedAccount.Memorial, dbUpdatedAccount.Memorial) suite.EqualValues(updatedAccount.AlsoKnownAs, dbUpdatedAccount.AlsoKnownAs) @@ -343,7 +342,6 @@ func (suite *InboxPostTestSuite) TestPostUpdate() { suite.EqualValues(updatedAccount.FollowersURI, dbUpdatedAccount.FollowersURI) suite.EqualValues(updatedAccount.FeaturedCollectionURI, dbUpdatedAccount.FeaturedCollectionURI) suite.EqualValues(updatedAccount.ActorType, dbUpdatedAccount.ActorType) - // suite.EqualValues(updatedAccount.PrivateKey, dbUpdatedAccount.PrivateKey) suite.EqualValues(updatedAccount.PublicKey, dbUpdatedAccount.PublicKey) suite.EqualValues(updatedAccount.PublicKeyURI, dbUpdatedAccount.PublicKeyURI) suite.EqualValues(updatedAccount.SensitizedAt, dbUpdatedAccount.SensitizedAt) @@ -353,6 +351,111 @@ func (suite *InboxPostTestSuite) TestPostUpdate() { suite.EqualValues(updatedAccount.SuspensionOrigin, dbUpdatedAccount.SuspensionOrigin) } +func (suite *InboxPostTestSuite) TestPostDelete() { + deletedAccount := *suite.testAccounts["remote_account_1"] + receivingAccount := suite.testAccounts["local_account_1"] + + // create a delete + delete := streams.NewActivityStreamsDelete() + + // set the appropriate actor on it + deleteActor := streams.NewActivityStreamsActorProperty() + deleteActor.AppendIRI(testrig.URLMustParse(deletedAccount.URI)) + delete.SetActivityStreamsActor(deleteActor) + + // Set the account iri as the 'object' property. + deleteObject := streams.NewActivityStreamsObjectProperty() + deleteObject.AppendIRI(testrig.URLMustParse(deletedAccount.URI)) + delete.SetActivityStreamsObject(deleteObject) + + // Set the To of the delete as public + deleteTo := streams.NewActivityStreamsToProperty() + deleteTo.AppendIRI(testrig.URLMustParse("https://www.w3.org/ns/activitystreams#Public")) + delete.SetActivityStreamsTo(deleteTo) + + // set some random-ass ID for the activity + deleteID := streams.NewJSONLDIdProperty() + deleteID.SetIRI(testrig.URLMustParse("http://fossbros-anonymous.io/d360613a-dc8d-4563-8f0b-b6161caf0f2b")) + delete.SetJSONLDId(deleteID) + + targetURI := testrig.URLMustParse(receivingAccount.InboxURI) + + signature, digestHeader, dateHeader := testrig.GetSignatureForActivity(delete, deletedAccount.PublicKeyURI, deletedAccount.PrivateKey, targetURI) + bodyI, err := streams.Serialize(delete) + suite.NoError(err) + + bodyJson, err := json.Marshal(bodyI) + suite.NoError(err) + body := bytes.NewReader(bodyJson) + + tc := testrig.NewTestTransportController(testrig.NewMockHTTPClient(nil), suite.db) + federator := testrig.NewTestFederator(suite.db, tc, suite.storage) + processor := testrig.NewTestProcessor(suite.db, suite.storage, federator) + err = processor.Start(context.Background()) + suite.NoError(err) + userModule := user.New(suite.config, processor, suite.log).(*user.Module) + + // setup request + recorder := httptest.NewRecorder() + ctx, _ := gin.CreateTestContext(recorder) + ctx.Request = httptest.NewRequest(http.MethodPost, targetURI.String(), body) // the endpoint we're hitting + ctx.Request.Header.Set("Signature", signature) + ctx.Request.Header.Set("Date", dateHeader) + ctx.Request.Header.Set("Digest", digestHeader) + ctx.Request.Header.Set("Content-Type", "application/activity+json") + + // we need to pass the context through signature check first to set appropriate values on it + suite.securityModule.SignatureCheck(ctx) + + // normally the router would populate these params from the path values, + // but because we're calling the function directly, we need to set them manually. + ctx.Params = gin.Params{ + gin.Param{ + Key: user.UsernameKey, + Value: receivingAccount.Username, + }, + } + + // trigger the function being tested + userModule.InboxPOSTHandler(ctx) + result := recorder.Result() + defer result.Body.Close() + b, err := ioutil.ReadAll(result.Body) + suite.NoError(err) + suite.Empty(b) + suite.Equal(http.StatusOK, result.StatusCode) + + // sleep for a sec so side effects can process in the background + time.Sleep(2 * time.Second) + + // local account 2 blocked foss_satan, that block should be gone now + testBlock := suite.testBlocks["local_account_2_block_remote_account_1"] + dbBlock := >smodel.Block{} + err = suite.db.GetByID(ctx, testBlock.ID, dbBlock) + suite.ErrorIs(err, db.ErrNoEntries) + + // no statuses from foss satan should be left in the database + dbStatuses, err := suite.db.GetAccountStatuses(ctx, deletedAccount.ID, 0, false, "", false, false) + suite.ErrorIs(err, db.ErrNoEntries) + suite.Empty(dbStatuses) + + dbAccount, err := suite.db.GetAccountByID(ctx, deletedAccount.ID) + suite.NoError(err) + + suite.Empty(dbAccount.Note) + suite.Empty(dbAccount.DisplayName) + suite.Empty(dbAccount.AvatarMediaAttachmentID) + suite.Empty(dbAccount.AvatarRemoteURL) + suite.Empty(dbAccount.HeaderMediaAttachmentID) + suite.Empty(dbAccount.HeaderRemoteURL) + suite.Empty(dbAccount.Reason) + suite.Empty(dbAccount.Fields) + suite.True(dbAccount.HideCollections) + suite.False(dbAccount.Discoverable) + suite.WithinDuration(time.Now(), dbAccount.SuspendedAt, 30*time.Second) + suite.Equal(dbAccount.ID, dbAccount.SuspensionOrigin) +} + func TestInboxPostTestSuite(t *testing.T) { suite.Run(t, &InboxPostTestSuite{}) } diff --git a/internal/api/s2s/user/user_test.go b/internal/api/s2s/user/user_test.go index eba0f61f0..86aa0bdf4 100644 --- a/internal/api/s2s/user/user_test.go +++ b/internal/api/s2s/user/user_test.go @@ -53,6 +53,7 @@ type UserStandardTestSuite struct { testAccounts map[string]*gtsmodel.Account testAttachments map[string]*gtsmodel.MediaAttachment testStatuses map[string]*gtsmodel.Status + testBlocks map[string]*gtsmodel.Block // module being tested userModule *user.Module @@ -66,6 +67,7 @@ func (suite *UserStandardTestSuite) SetupSuite() { suite.testAccounts = testrig.NewTestAccounts() suite.testAttachments = testrig.NewTestAttachments() suite.testStatuses = testrig.NewTestStatuses() + suite.testBlocks = testrig.NewTestBlocks() } func (suite *UserStandardTestSuite) SetupTest() { diff --git a/internal/federation/federatingdb/delete.go b/internal/federation/federatingdb/delete.go index abc3715da..9aa36ee90 100644 --- a/internal/federation/federatingdb/delete.go +++ b/internal/federation/federatingdb/delete.go @@ -89,10 +89,7 @@ func (f *federatingDB) Delete(ctx context.Context, id *url.URL) error { a, err := f.db.GetAccountByURI(ctx, id.String()) if err == nil { // it's an account - l.Debugf("uri is for an account with id: %s", a.ID) - if err := f.db.DeleteByID(ctx, a.ID, >smodel.Account{}); err != nil { - return fmt.Errorf("DELETE: err deleting account: %s", err) - } + l.Debugf("uri is for an account with id %s, passing delete message to the processor", a.ID) fromFederatorChan <- messages.FromFederator{ APObjectType: ap.ObjectProfile, APActivityType: ap.ActivityDelete, diff --git a/internal/processing/account/delete.go b/internal/processing/account/delete.go index 5de706045..632f49751 100644 --- a/internal/processing/account/delete.go +++ b/internal/processing/account/delete.go @@ -31,7 +31,7 @@ import ( // Delete handles the complete deletion of an account. // -// TODO in this function: +// To be done in this function: // 1. Delete account's application(s), clients, and oauth tokens // 2. Delete account's blocks // 3. Delete account's emoji @@ -51,12 +51,16 @@ import ( // 17. Delete account's timeline // 18. Delete account itself func (p *processor) Delete(ctx context.Context, account *gtsmodel.Account, origin string) error { - l := p.log.WithFields(logrus.Fields{ + fields := logrus.Fields{ "func": "Delete", "username": account.Username, - }) + } + if account.Domain != "" { + fields["domain"] = account.Domain + } + l := p.log.WithFields(fields) - l.Debugf("beginning account delete process for username %s", account.Username) + l.Debug("beginning account delete process") // 1. Delete account's application(s), clients, and oauth tokens // we only need to do this step for local account since remote ones won't have any tokens or applications on our server @@ -214,10 +218,16 @@ selectStatusesLoop: // 10. Delete account's notifications l.Debug("deleting account notifications") + // first notifications created by account if err := p.db.DeleteWhere(ctx, []db.Where{{Key: "origin_account_id", Value: account.ID}}, &[]*gtsmodel.Notification{}); err != nil { l.Errorf("error deleting notifications created by account: %s", err) } + // now notifications targeting account + if err := p.db.DeleteWhere(ctx, []db.Where{{Key: "target_account_id", Value: account.ID}}, &[]*gtsmodel.Notification{}); err != nil { + l.Errorf("error deleting notifications targeting account: %s", err) + } + // 11. Delete account's bookmarks l.Debug("deleting account bookmarks") if err := p.db.DeleteWhere(ctx, []db.Where{{Key: "account_id", Value: account.ID}}, &[]*gtsmodel.StatusBookmark{}); err != nil { @@ -267,8 +277,6 @@ selectStatusesLoop: account.HideCollections = true account.Discoverable = false - account.UpdatedAt = time.Now() - account.SuspendedAt = time.Now() account.SuspensionOrigin = origin diff --git a/internal/processing/fromfederator.go b/internal/processing/fromfederator.go index feb22b2ba..ffaf625d3 100644 --- a/internal/processing/fromfederator.go +++ b/internal/processing/fromfederator.go @@ -181,7 +181,13 @@ func (p *processor) ProcessFromFederator(ctx context.Context, federatorMsg messa return p.deleteStatusFromTimelines(ctx, statusToDelete) case ap.ObjectProfile: // DELETE A PROFILE/ACCOUNT - // TODO: handle side effects of account deletion here: delete all objects, statuses, media etc associated with account + // handle side effects of account deletion here: delete all objects, statuses, media etc associated with account + account, ok := federatorMsg.GTSModel.(*gtsmodel.Account) + if !ok { + return errors.New("account delete was not parseable as *gtsmodel.Account") + } + + return p.accountProcessor.Delete(ctx, account, account.ID) } case ap.ActivityAccept: // ACCEPT diff --git a/internal/processing/fromfederator_test.go b/internal/processing/fromfederator_test.go index ba2aaf03e..605a18bdc 100644 --- a/internal/processing/fromfederator_test.go +++ b/internal/processing/fromfederator_test.go @@ -20,6 +20,7 @@ package processing_test import ( "context" + "fmt" "testing" "time" @@ -276,6 +277,86 @@ func (suite *FromFederatorTestSuite) TestProcessFaveWithDifferentReceivingAccoun suite.Empty(stream.Messages) } +func (suite *FromFederatorTestSuite) TestProcessAccountDelete() { + ctx := context.Background() + + deletedAccount := suite.testAccounts["remote_account_1"] + receivingAccount := suite.testAccounts["local_account_1"] + + // before doing the delete.... + // make local_account_1 and remote_account_1 into mufos + zorkFollowSatan := >smodel.Follow{ + ID: "01FGRY72ASHBSET64353DPHK9T", + CreatedAt: time.Now().Add(-1 * time.Hour), + UpdatedAt: time.Now().Add(-1 * time.Hour), + AccountID: deletedAccount.ID, + TargetAccountID: receivingAccount.ID, + ShowReblogs: true, + URI: fmt.Sprintf("%s/follows/01FGRY72ASHBSET64353DPHK9T", deletedAccount.URI), + Notify: false, + } + err := suite.db.Put(ctx, zorkFollowSatan) + suite.NoError(err) + + satanFollowZork := >smodel.Follow{ + ID: "01FGRYAVAWWPP926J175QGM0WV", + CreatedAt: time.Now().Add(-1 * time.Hour), + UpdatedAt: time.Now().Add(-1 * time.Hour), + AccountID: receivingAccount.ID, + TargetAccountID: deletedAccount.ID, + ShowReblogs: true, + URI: fmt.Sprintf("%s/follows/01FGRYAVAWWPP926J175QGM0WV", receivingAccount.URI), + Notify: false, + } + err = suite.db.Put(ctx, satanFollowZork) + suite.NoError(err) + + // now they are mufos! + + err = suite.processor.ProcessFromFederator(ctx, messages.FromFederator{ + APObjectType: ap.ObjectProfile, + APActivityType: ap.ActivityDelete, + GTSModel: deletedAccount, + ReceivingAccount: receivingAccount, + }) + suite.NoError(err) + + // local account 2 blocked foss_satan, that block should be gone now + testBlock := suite.testBlocks["local_account_2_block_remote_account_1"] + dbBlock := >smodel.Block{} + err = suite.db.GetByID(ctx, testBlock.ID, dbBlock) + suite.ErrorIs(err, db.ErrNoEntries) + + // the mufos should be gone now too + satanFollowsZork, err := suite.db.IsFollowing(ctx, deletedAccount, receivingAccount) + suite.NoError(err) + suite.False(satanFollowsZork) + zorkFollowsSatan, err := suite.db.IsFollowing(ctx, receivingAccount, deletedAccount) + suite.NoError(err) + suite.False(zorkFollowsSatan) + + // no statuses from foss satan should be left in the database + dbStatuses, err := suite.db.GetAccountStatuses(ctx, deletedAccount.ID, 0, false, "", false, false) + suite.ErrorIs(err, db.ErrNoEntries) + suite.Empty(dbStatuses) + + dbAccount, err := suite.db.GetAccountByID(ctx, deletedAccount.ID) + suite.NoError(err) + + suite.Empty(dbAccount.Note) + suite.Empty(dbAccount.DisplayName) + suite.Empty(dbAccount.AvatarMediaAttachmentID) + suite.Empty(dbAccount.AvatarRemoteURL) + suite.Empty(dbAccount.HeaderMediaAttachmentID) + suite.Empty(dbAccount.HeaderRemoteURL) + suite.Empty(dbAccount.Reason) + suite.Empty(dbAccount.Fields) + suite.True(dbAccount.HideCollections) + suite.False(dbAccount.Discoverable) + suite.WithinDuration(time.Now(), dbAccount.SuspendedAt, 30*time.Second) + suite.Equal(dbAccount.ID, dbAccount.SuspensionOrigin) +} + func TestFromFederatorTestSuite(t *testing.T) { suite.Run(t, &FromFederatorTestSuite{}) } diff --git a/internal/processing/processor_test.go b/internal/processing/processor_test.go index daaf46726..e0f44b0d7 100644 --- a/internal/processing/processor_test.go +++ b/internal/processing/processor_test.go @@ -62,6 +62,7 @@ type ProcessingStandardTestSuite struct { testTags map[string]*gtsmodel.Tag testMentions map[string]*gtsmodel.Mention testAutheds map[string]*oauth.Auth + testBlocks map[string]*gtsmodel.Block processor processing.Processor } @@ -83,6 +84,7 @@ func (suite *ProcessingStandardTestSuite) SetupSuite() { Account: suite.testAccounts["local_account_1"], }, } + suite.testBlocks = testrig.NewTestBlocks() } func (suite *ProcessingStandardTestSuite) SetupTest() {