From 33dbd3ab7a2245cdd7ec56eb26d53d01e2c89f16 Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Fri, 19 Jan 2024 14:13:24 +0100 Subject: [PATCH] [bugfix] Ensure domain block side effects skipped if allow in place (blocklist mode) (#2542) --- internal/processing/admin/domainblock.go | 58 ++++ .../processing/admin/domainpermission_test.go | 276 +++++++++++++++++- 2 files changed, 322 insertions(+), 12 deletions(-) diff --git a/internal/processing/admin/domainblock.go b/internal/processing/admin/domainblock.go index 4161ec12f..9fe5dc847 100644 --- a/internal/processing/admin/domainblock.go +++ b/internal/processing/admin/domainblock.go @@ -26,6 +26,7 @@ "codeberg.org/gruf/go-kv" "github.com/superseriousbusiness/gotosocial/internal/ap" apimodel "github.com/superseriousbusiness/gotosocial/internal/api/model" + "github.com/superseriousbusiness/gotosocial/internal/config" "github.com/superseriousbusiness/gotosocial/internal/db" "github.com/superseriousbusiness/gotosocial/internal/gtserror" "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" @@ -92,6 +93,15 @@ func(ctx context.Context) gtserror.MultiError { {"actionID", actionID}, }...).WithContext(ctx) + skip, err := p.skipBlockSideEffects(ctx, domain) + if err != nil { + return err + } + if skip != "" { + l.Infof("skipping domain block side effects: %s", skip) + return nil + } + l.Info("processing domain block side effects") defer func() { l.Info("finished processing domain block side effects") }() @@ -109,6 +119,54 @@ func(ctx context.Context) gtserror.MultiError { return apiDomainBlock, actionID, nil } +// skipBlockSideEffects checks if side effects of block creation +// should be skipped for the given domain, taking account of +// instance federation mode, and existence of any allows +// which ought to "shield" this domain from being blocked. +// +// If the caller should skip, the returned string will be non-zero +// and will be set to a reason why side effects should be skipped. +// +// - blocklist mode + allow exists: "..." (skip) +// - blocklist mode + no allow: "" (don't skip) +// - allowlist mode + allow exists: "" (don't skip) +// - allowlist mode + no allow: "" (don't skip) +func (p *Processor) skipBlockSideEffects( + ctx context.Context, + domain string, +) (string, gtserror.MultiError) { + var ( + skip string // Assume "" (don't skip). + errs gtserror.MultiError + ) + + // Never skip block side effects in allowlist mode. + fediMode := config.GetInstanceFederationMode() + if fediMode == config.InstanceFederationModeAllowlist { + return skip, errs + } + + // We know we're in blocklist mode. + // + // We want to skip domain block side + // effects if an allow is already + // in place which overrides the block. + + // Check if an explicit allow exists for this domain. + domainAllow, err := p.state.DB.GetDomainAllow(ctx, domain) + if err != nil && !errors.Is(err, db.ErrNoEntries) { + errs.Appendf("error getting domain allow: %w", err) + return skip, errs + } + + if domainAllow != nil { + skip = "running in blocklist mode, and an explicit allow exists for this domain" + return skip, errs + } + + return skip, errs +} + // domainBlockSideEffects processes the side effects of a domain block: // // 1. Strip most info away from the instance entry for the domain. diff --git a/internal/processing/admin/domainpermission_test.go b/internal/processing/admin/domainpermission_test.go index b6de226c1..f1376bd35 100644 --- a/internal/processing/admin/domainpermission_test.go +++ b/internal/processing/admin/domainpermission_test.go @@ -19,12 +19,15 @@ import ( "context" + "net/http" "testing" "github.com/stretchr/testify/suite" apimodel "github.com/superseriousbusiness/gotosocial/internal/api/model" "github.com/superseriousbusiness/gotosocial/internal/config" + "github.com/superseriousbusiness/gotosocial/internal/gtserror" "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" + "github.com/superseriousbusiness/gotosocial/internal/id" "github.com/superseriousbusiness/gotosocial/testrig" ) @@ -49,7 +52,10 @@ type domainPermAction struct { // permission action on each // account on the target domain. // Eg., suite.Zero(account.SuspendedAt) - expected func(*gtsmodel.Account) bool + expected func( + context.Context, + *gtsmodel.Account, + ) bool } type domainPermTest struct { @@ -76,6 +82,9 @@ func (suite *DomainBlockTestSuite) runDomainPermTest(t domainPermTest) { config.SetInstanceFederationMode(t.instanceFederationMode) for _, action := range t.actions { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + // Run the desired action. var actionID string switch action.createOrDelete { @@ -102,7 +111,7 @@ func (suite *DomainBlockTestSuite) runDomainPermTest(t domainPermTest) { } for _, account := range accounts { - if !action.expected(account) { + if !action.expected(ctx, account) { suite.T().FailNow() } } @@ -193,6 +202,42 @@ func (suite *DomainBlockTestSuite) awaitAction(actionID string) { suite.Empty(adminAction.Errors) } +// shortcut to look up an account +// using the Search processor. +func (suite *DomainBlockTestSuite) lookupAccount( + ctx context.Context, + requestingAccount *gtsmodel.Account, + targetAccount *gtsmodel.Account, +) (*apimodel.Account, gtserror.WithCode) { + return suite.processor.Search().Lookup( + ctx, + requestingAccount, + "@"+targetAccount.Username+"@"+targetAccount.Domain, + ) +} + +// shortcut to look up target account's +// statuses using the Account processor. +func (suite *DomainBlockTestSuite) getStatuses( + ctx context.Context, + requestingAccount *gtsmodel.Account, + targetAccount *gtsmodel.Account, +) (*apimodel.PageableResponse, gtserror.WithCode) { + return suite.processor.Account().StatusesGet( + ctx, + requestingAccount, + targetAccount.ID, + 0, // unlimited + false, // include replies + false, // include reblogs + id.Highest, // max ID + id.Lowest, // min ID + false, // don't filter on pinned + false, // don't filter on media + false, // don't filter on public + ) +} + func (suite *DomainBlockTestSuite) TestBlockAndUnblockDomain() { const domain = "fossbros-anonymous.io" @@ -203,7 +248,7 @@ func (suite *DomainBlockTestSuite) TestBlockAndUnblockDomain() { createOrDelete: "create", permissionType: gtsmodel.DomainPermissionBlock, domain: domain, - expected: func(account *gtsmodel.Account) bool { + expected: func(_ context.Context, account *gtsmodel.Account) bool { // Domain was blocked, so each // account should now be suspended. return suite.NotZero(account.SuspendedAt) @@ -213,7 +258,7 @@ func (suite *DomainBlockTestSuite) TestBlockAndUnblockDomain() { createOrDelete: "delete", permissionType: gtsmodel.DomainPermissionBlock, domain: domain, - expected: func(account *gtsmodel.Account) bool { + expected: func(_ context.Context, account *gtsmodel.Account) bool { // Domain was unblocked, so each // account should now be unsuspended. return suite.Zero(account.SuspendedAt) @@ -226,6 +271,9 @@ func (suite *DomainBlockTestSuite) TestBlockAndUnblockDomain() { func (suite *DomainBlockTestSuite) TestBlockAndAllowDomain() { const domain = "fossbros-anonymous.io" + // Use zork for checks within test. + var testAccount = suite.testAccounts["local_account_1"] + suite.runDomainPermTest(domainPermTest{ instanceFederationMode: config.InstanceFederationModeBlocklist, actions: []domainPermAction{ @@ -233,42 +281,246 @@ func (suite *DomainBlockTestSuite) TestBlockAndAllowDomain() { createOrDelete: "create", permissionType: gtsmodel.DomainPermissionBlock, domain: domain, - expected: func(account *gtsmodel.Account) bool { + expected: func(ctx context.Context, account *gtsmodel.Account) bool { // Domain was blocked, so each // account should now be suspended. - return suite.NotZero(account.SuspendedAt) + if account.SuspendedAt.IsZero() { + suite.T().Logf("account %s should be suspended", account.Username) + return false + } + + // Local account 1 should be able to see + // no statuses from suspended account. + statuses, err := suite.getStatuses(ctx, testAccount, account) + if err != nil { + suite.FailNow(err.Error()) + } + if l := len(statuses.Items); l != 0 { + suite.T().Logf("expected statuses of len 0, was %d", l) + return false + } + + // Lookup for this account should return 404. + lookupAcct, err := suite.lookupAccount(ctx, testAccount, account) + if err == nil || err.Code() != http.StatusNotFound { + suite.T().Logf("expected 404 error, got %v", err) + return false + } + if lookupAcct != nil { + suite.T().Logf("expected nil account lookup, got %v", lookupAcct) + return false + } + + return true }, }, { createOrDelete: "create", permissionType: gtsmodel.DomainPermissionAllow, domain: domain, - expected: func(account *gtsmodel.Account) bool { + expected: func(ctx context.Context, account *gtsmodel.Account) bool { // Domain was explicitly allowed, so each // account should now be unsuspended, since // the allow supercedes the block. - return suite.Zero(account.SuspendedAt) + if !account.SuspendedAt.IsZero() { + suite.T().Logf("account %s should not be suspended", account.Username) + return false + } + + // Local account 1 should be able to see + // no statuses from account, because any + // statuses were deleted by the block above. + statuses, err := suite.getStatuses(ctx, testAccount, account) + if err != nil { + suite.FailNow(err.Error()) + } + if l := len(statuses.Items); l != 0 { + suite.T().Logf("expected statuses of len 0, was %d", l) + return false + } + + // Lookup for this account should return OK. + lookupAcct, err := suite.lookupAccount(ctx, testAccount, account) + if err != nil { + suite.T().Logf("expected no error, got %v", err) + return false + } + if lookupAcct == nil { + suite.T().Log("expected not nil account lookup") + return false + } + + return true }, }, { createOrDelete: "delete", permissionType: gtsmodel.DomainPermissionAllow, domain: domain, - expected: func(account *gtsmodel.Account) bool { + expected: func(ctx context.Context, account *gtsmodel.Account) bool { // Deleting the allow now, while there's // still a block in place, should cause // the block to take effect again. - return suite.NotZero(account.SuspendedAt) + if account.SuspendedAt.IsZero() { + suite.T().Logf("account %s should be suspended", account.Username) + return false + } + + // Lookup for this account should return 404. + lookupAcct, err := suite.lookupAccount(ctx, testAccount, account) + if err == nil || err.Code() != http.StatusNotFound { + suite.T().Logf("expected 404 error, got %v", err) + return false + } + if lookupAcct != nil { + suite.T().Logf("expected nil account lookup, got %v", lookupAcct) + return false + } + + return true }, }, { createOrDelete: "delete", permissionType: gtsmodel.DomainPermissionBlock, domain: domain, - expected: func(account *gtsmodel.Account) bool { + expected: func(ctx context.Context, account *gtsmodel.Account) bool { // Deleting the block now should // unsuspend the accounts again. - return suite.Zero(account.SuspendedAt) + if !account.SuspendedAt.IsZero() { + suite.T().Logf("account %s should not be suspended", account.Username) + return false + } + + // Lookup for this account should return OK. + lookupAcct, err := suite.lookupAccount(ctx, testAccount, account) + if err != nil { + suite.T().Logf("expected no error, got %v", err) + return false + } + if lookupAcct == nil { + suite.T().Log("expected not nil account lookup") + return false + } + + return true + }, + }, + }, + }) +} + + +func (suite *DomainBlockTestSuite) TestAllowAndBlockDomain() { + const domain = "fossbros-anonymous.io" + + // Use zork for checks within test. + var testAccount = suite.testAccounts["local_account_1"] + + suite.runDomainPermTest(domainPermTest{ + instanceFederationMode: config.InstanceFederationModeBlocklist, + actions: []domainPermAction{ + { + createOrDelete: "create", + permissionType: gtsmodel.DomainPermissionAllow, + domain: domain, + expected: func(ctx context.Context, account *gtsmodel.Account) bool { + // Domain was explicitly allowed, + // nothing should be suspended. + if !account.SuspendedAt.IsZero() { + suite.T().Logf("account %s should not be suspended", account.Username) + return false + } + + // Local account 1 should be able + // to see statuses from account. + statuses, err := suite.getStatuses(ctx, testAccount, account) + if err != nil { + suite.FailNow(err.Error()) + } + if l := len(statuses.Items); l == 0 { + suite.T().Log("expected some statuses, but length was 0") + return false + } + + // Lookup for this account should return OK. + lookupAcct, err := suite.lookupAccount(ctx, testAccount, account) + if err != nil { + suite.T().Logf("expected no error, got %v", err) + return false + } + if lookupAcct == nil { + suite.T().Log("expected not nil account lookup") + return false + } + + return true + }, + }, + { + createOrDelete: "create", + permissionType: gtsmodel.DomainPermissionBlock, + domain: domain, + expected: func(ctx context.Context, account *gtsmodel.Account) bool { + // Create a block. An allow existed, so + // block side effects should be witheld. + // In other words, we should have the same + // results as before we added the block. + if !account.SuspendedAt.IsZero() { + suite.T().Logf("account %s should not be suspended", account.Username) + return false + } + + // Local account 1 should be able + // to see statuses from account. + statuses, err := suite.getStatuses(ctx, testAccount, account) + if err != nil { + suite.FailNow(err.Error()) + } + if l := len(statuses.Items); l == 0 { + suite.T().Log("expected some statuses, but length was 0") + return false + } + + // Lookup for this account should return OK. + lookupAcct, err := suite.lookupAccount(ctx, testAccount, account) + if err != nil { + suite.T().Logf("expected no error, got %v", err) + return false + } + if lookupAcct == nil { + suite.T().Log("expected not nil account lookup") + return false + } + + return true + }, + }, + { + createOrDelete: "delete", + permissionType: gtsmodel.DomainPermissionAllow, + domain: domain, + expected: func(ctx context.Context, account *gtsmodel.Account) bool { + // Deleting the allow now, while there's + // a block in place, should cause the + // block to take effect. + if account.SuspendedAt.IsZero() { + suite.T().Logf("account %s should be suspended", account.Username) + return false + } + + // Lookup for this account should return 404. + lookupAcct, err := suite.lookupAccount(ctx, testAccount, account) + if err == nil || err.Code() != http.StatusNotFound { + suite.T().Logf("expected 404 error, got %v", err) + return false + } + if lookupAcct != nil { + suite.T().Logf("expected nil account lookup, got %v", lookupAcct) + return false + } + + return true }, }, },