From cec29e2a8d2d6ca49bb6c789f0ed3226849a7359 Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Wed, 2 Aug 2023 09:31:09 +0200 Subject: [PATCH] [bugfix] Allow instance accounts to be shown in search results in certain circumstances (#2053) --- internal/api/client/search/searchget_test.go | 142 ++++++++++++++++++- internal/processing/search/accounts.go | 21 ++- internal/processing/search/get.go | 26 +++- internal/processing/search/lookup.go | 14 +- internal/processing/search/util.go | 6 +- internal/visibility/account.go | 9 +- 6 files changed, 203 insertions(+), 15 deletions(-) diff --git a/internal/api/client/search/searchget_test.go b/internal/api/client/search/searchget_test.go index edaac2fc..e811dd32 100644 --- a/internal/api/client/search/searchget_test.go +++ b/internal/api/client/search/searchget_test.go @@ -19,6 +19,8 @@ package search_test import ( "context" + "crypto/rand" + "crypto/rsa" "encoding/json" "fmt" "io" @@ -30,6 +32,7 @@ import ( "testing" "github.com/stretchr/testify/suite" + "github.com/superseriousbusiness/gotosocial/internal/ap" "github.com/superseriousbusiness/gotosocial/internal/api/client/search" apimodel "github.com/superseriousbusiness/gotosocial/internal/api/model" apiutil "github.com/superseriousbusiness/gotosocial/internal/api/util" @@ -1001,7 +1004,7 @@ func (suite *SearchGetTestSuite) TestSearchAAccounts() { suite.Len(searchResult.Hashtags, 0) } -func (suite *SearchGetTestSuite) TestSearchAAccountsLimit1() { +func (suite *SearchGetTestSuite) TestSearchAccountsLimit1() { var ( requestingAccount = suite.testAccounts["local_account_1"] token = suite.testTokens["local_account_1"] @@ -1078,12 +1081,14 @@ func (suite *SearchGetTestSuite) TestSearchLocalInstanceAccountByURI() { suite.FailNow(err.Error()) } - suite.Len(searchResult.Accounts, 0) + // Should be able to get instance + // account by exact URI. + suite.Len(searchResult.Accounts, 1) suite.Len(searchResult.Statuses, 0) suite.Len(searchResult.Hashtags, 0) } -func (suite *SearchGetTestSuite) TestSearchInstanceAccountFull() { +func (suite *SearchGetTestSuite) TestSearchLocalInstanceAccountFull() { // Namestring excludes ':' in usernames, so we // need to fiddle with the instance account a // bit to get it to look like a different domain. @@ -1125,12 +1130,14 @@ func (suite *SearchGetTestSuite) TestSearchInstanceAccountFull() { suite.FailNow(err.Error()) } - suite.Len(searchResult.Accounts, 0) + // Should be able to get instance + // account by full namestring. + suite.Len(searchResult.Accounts, 1) suite.Len(searchResult.Statuses, 0) suite.Len(searchResult.Hashtags, 0) } -func (suite *SearchGetTestSuite) TestSearchInstanceAccountPartial() { +func (suite *SearchGetTestSuite) TestSearchLocalInstanceAccountPartial() { // Namestring excludes ':' in usernames, so we // need to fiddle with the instance account a // bit to get it to look like a different domain. @@ -1172,6 +1179,131 @@ func (suite *SearchGetTestSuite) TestSearchInstanceAccountPartial() { suite.FailNow(err.Error()) } + // Query was a partial namestring from our + // instance, so will return the instance account. + suite.Len(searchResult.Accounts, 1) + suite.Len(searchResult.Statuses, 0) + suite.Len(searchResult.Hashtags, 0) +} + +func (suite *SearchGetTestSuite) TestSearchLocalInstanceAccountEvenMorePartial() { + // Namestring excludes ':' in usernames, so we + // need to fiddle with the instance account a + // bit to get it to look like a different domain. + newDomain := "example.org" + suite.bodgeLocalInstance(newDomain) + + var ( + requestingAccount = suite.testAccounts["local_account_1"] + token = suite.testTokens["local_account_1"] + user = suite.testUsers["local_account_1"] + maxID *string = nil + minID *string = nil + limit *int = nil + offset *int = nil + resolve *bool = nil + query = newDomain + queryType *string = nil + following *bool = nil + expectedHTTPStatus = http.StatusOK + expectedBody = "" + ) + + searchResult, err := suite.getSearch( + requestingAccount, + token, + apiutil.APIv2, + user, + maxID, + minID, + limit, + offset, + query, + queryType, + resolve, + following, + expectedHTTPStatus, + expectedBody) + if err != nil { + suite.FailNow(err.Error()) + } + + // Query was just 'example.org' which doesn't + // look like a namestring, so search should + // fall back to text search and therefore give + // 0 results back. + suite.Len(searchResult.Accounts, 0) + suite.Len(searchResult.Statuses, 0) + suite.Len(searchResult.Hashtags, 0) +} + +func (suite *SearchGetTestSuite) TestSearchRemoteInstanceAccountPartial() { + // Insert an instance account that's not + // from our instance, and try to search + // for it with a partial namestring. + theirDomain := "example.org" + + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + suite.FailNow(err.Error()) + } + + if err := suite.db.PutAccount(context.Background(), >smodel.Account{ + ID: "01H6RWPG8T6DNW6VNXPBCJBH5S", + Username: theirDomain, + Domain: theirDomain, + URI: "http://" + theirDomain + "/users/" + theirDomain, + URL: "http://" + theirDomain + "/@" + theirDomain, + PublicKeyURI: "http://" + theirDomain + "/users/" + theirDomain + "#main-key", + InboxURI: "http://" + theirDomain + "/users/" + theirDomain + "/inbox", + OutboxURI: "http://" + theirDomain + "/users/" + theirDomain + "/outbox", + FollowersURI: "http://" + theirDomain + "/users/" + theirDomain + "/followers", + FollowingURI: "http://" + theirDomain + "/users/" + theirDomain + "/following", + FeaturedCollectionURI: "http://" + theirDomain + "/users/" + theirDomain + "/collections/featured", + ActorType: ap.ActorPerson, + PrivateKey: key, + PublicKey: &key.PublicKey, + }); err != nil { + suite.FailNow(err.Error()) + } + + var ( + requestingAccount = suite.testAccounts["local_account_1"] + token = suite.testTokens["local_account_1"] + user = suite.testUsers["local_account_1"] + maxID *string = nil + minID *string = nil + limit *int = nil + offset *int = nil + resolve *bool = nil + query = "@" + theirDomain + queryType *string = nil + following *bool = nil + expectedHTTPStatus = http.StatusOK + expectedBody = "" + ) + + searchResult, err := suite.getSearch( + requestingAccount, + token, + apiutil.APIv2, + user, + maxID, + minID, + limit, + offset, + query, + queryType, + resolve, + following, + expectedHTTPStatus, + expectedBody) + if err != nil { + suite.FailNow(err.Error()) + } + + // Search for instance account from + // another domain should return 0 results. suite.Len(searchResult.Accounts, 0) suite.Len(searchResult.Statuses, 0) suite.Len(searchResult.Hashtags, 0) diff --git a/internal/processing/search/accounts.go b/internal/processing/search/accounts.go index eb88647a..cfcc65b2 100644 --- a/internal/processing/search/accounts.go +++ b/internal/processing/search/accounts.go @@ -49,6 +49,13 @@ func (p *Processor) Accounts( resolve bool, following bool, ) ([]*apimodel.Account, gtserror.WithCode) { + // Don't include instance accounts in this search. + // + // We don't want someone to start typing '@mastodon' + // and then get a million instance service accounts + // in their search results. + const includeInstanceAccounts = false + var ( foundAccounts = make([]*gtsmodel.Account, 0, limit) appendAccount = func(foundAccount *gtsmodel.Account) { foundAccounts = append(foundAccounts, foundAccount) } @@ -83,7 +90,12 @@ func (p *Processor) Accounts( // if caller supplied an offset greater than 0, return // nothing as though there were no additional results. if offset > 0 { - return p.packageAccounts(ctx, requestingAccount, foundAccounts) + return p.packageAccounts( + ctx, + requestingAccount, + foundAccounts, + includeInstanceAccounts, + ) } // Return all accounts we can find that match the @@ -106,5 +118,10 @@ func (p *Processor) Accounts( } // Return whatever we got (if anything). - return p.packageAccounts(ctx, requestingAccount, foundAccounts) + return p.packageAccounts( + ctx, + requestingAccount, + foundAccounts, + includeInstanceAccounts, + ) } diff --git a/internal/processing/search/get.go b/internal/processing/search/get.go index 8e1881ab..830e3481 100644 --- a/internal/processing/search/get.go +++ b/internal/processing/search/get.go @@ -70,6 +70,13 @@ func (p *Processor) Get( queryType = strings.TrimSpace(strings.ToLower(req.QueryType)) // Trim trailing/leading whitespace; convert to lowercase. resolve = req.Resolve following = req.Following + + // Include instance accounts in the first + // parts of this search. This will be + // changed to 'false' when doing text + // search in the database in the latter + // parts of this function. + includeInstanceAccounts = true ) // Validate query. @@ -109,7 +116,12 @@ func (p *Processor) Get( // supply an offset greater than 0, return nothing as // though there were no additional results. if req.Offset > 0 { - return p.packageSearchResult(ctx, account, nil, nil, nil, req.APIv1) + return p.packageSearchResult( + ctx, + account, + nil, nil, nil, // No results. + req.APIv1, includeInstanceAccounts, + ) } var ( @@ -167,6 +179,7 @@ func (p *Processor) Get( foundStatuses, foundTags, req.APIv1, + includeInstanceAccounts, ) } } @@ -196,6 +209,7 @@ func (p *Processor) Get( foundStatuses, foundTags, req.APIv1, + includeInstanceAccounts, ) } @@ -236,11 +250,20 @@ func (p *Processor) Get( foundStatuses, foundTags, req.APIv1, + includeInstanceAccounts, ) } // As a last resort, search for accounts and // statuses using the query as arbitrary text. + // + // At this point we no longer want to include + // instance accounts in the results, since searching + // for something like 'mastodon', for example, will + // include a million instance/service accounts that + // have 'mastodon' in the domain, and therefore in + // the username, making the search results useless. + includeInstanceAccounts = false if err := p.byText( ctx, account, @@ -267,6 +290,7 @@ func (p *Processor) Get( foundStatuses, foundTags, req.APIv1, + includeInstanceAccounts, ) } diff --git a/internal/processing/search/lookup.go b/internal/processing/search/lookup.go index d5018322..8b48d451 100644 --- a/internal/processing/search/lookup.go +++ b/internal/processing/search/lookup.go @@ -44,6 +44,13 @@ func (p *Processor) Lookup( requestingAccount *gtsmodel.Account, query string, ) (*apimodel.Account, gtserror.WithCode) { + // Include instance accounts in this search. + // + // Lookup is for one specific account so we + // can't return loads of instance accounts by + // accident. + const includeInstanceAccounts = true + // Validate query. query = strings.TrimSpace(query) if query == "" { @@ -96,7 +103,12 @@ func (p *Processor) Lookup( // using the packageAccounts function to return it. This // may cause the account to be filtered out if it's not // visible to the caller, so anticipate this. - accounts, errWithCode := p.packageAccounts(ctx, requestingAccount, []*gtsmodel.Account{account}) + accounts, errWithCode := p.packageAccounts( + ctx, + requestingAccount, + []*gtsmodel.Account{account}, + includeInstanceAccounts, + ) if errWithCode != nil { return nil, errWithCode } diff --git a/internal/processing/search/util.go b/internal/processing/search/util.go index 171d0e57..c0eac0ca 100644 --- a/internal/processing/search/util.go +++ b/internal/processing/search/util.go @@ -48,11 +48,12 @@ func (p *Processor) packageAccounts( ctx context.Context, requestingAccount *gtsmodel.Account, accounts []*gtsmodel.Account, + includeInstanceAccounts bool, ) ([]*apimodel.Account, gtserror.WithCode) { apiAccounts := make([]*apimodel.Account, 0, len(accounts)) for _, account := range accounts { - if account.IsInstance() { + if !includeInstanceAccounts && account.IsInstance() { // No need to show instance accounts. continue } @@ -169,8 +170,9 @@ func (p *Processor) packageSearchResult( statuses []*gtsmodel.Status, tags []*gtsmodel.Tag, v1 bool, + includeInstanceAccounts bool, ) (*apimodel.SearchResult, gtserror.WithCode) { - apiAccounts, errWithCode := p.packageAccounts(ctx, requestingAccount, accounts) + apiAccounts, errWithCode := p.packageAccounts(ctx, requestingAccount, accounts, includeInstanceAccounts) if errWithCode != nil { return nil, errWithCode } diff --git a/internal/visibility/account.go b/internal/visibility/account.go index 6c7a0059..4d42b597 100644 --- a/internal/visibility/account.go +++ b/internal/visibility/account.go @@ -19,10 +19,10 @@ package visibility import ( "context" - "fmt" "github.com/superseriousbusiness/gotosocial/internal/cache" "github.com/superseriousbusiness/gotosocial/internal/config" + "github.com/superseriousbusiness/gotosocial/internal/gtserror" "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" "github.com/superseriousbusiness/gotosocial/internal/log" ) @@ -66,7 +66,7 @@ func (f *Filter) isAccountVisibleTo(ctx context.Context, requester *gtsmodel.Acc // Check whether target account is visible to anyone. visible, err := f.isAccountVisible(ctx, account) if err != nil { - return false, fmt.Errorf("isAccountVisibleTo: error checking account %s visibility: %w", account.ID, err) + return false, gtserror.Newf("error checking account %s visibility: %w", account.ID, err) } if !visible { @@ -83,7 +83,7 @@ func (f *Filter) isAccountVisibleTo(ctx context.Context, requester *gtsmodel.Acc // If requester is not visible, they cannot *see* either. visible, err = f.isAccountVisible(ctx, requester) if err != nil { - return false, fmt.Errorf("isAccountVisibleTo: error checking account %s visibility: %w", account.ID, err) + return false, gtserror.Newf("error checking account %s visibility: %w", account.ID, err) } if !visible { @@ -97,7 +97,7 @@ func (f *Filter) isAccountVisibleTo(ctx context.Context, requester *gtsmodel.Acc account.ID, ) if err != nil { - return false, fmt.Errorf("isAccountVisibleTo: error checking account blocks: %w", err) + return false, gtserror.Newf("error checking account blocks: %w", err) } if blocked { @@ -121,6 +121,7 @@ func (f *Filter) isAccountVisible(ctx context.Context, account *gtsmodel.Account // Fetch the local user model for this account. user, err := f.state.DB.GetUserByAccountID(ctx, account.ID) if err != nil { + err := gtserror.Newf("db error getting user for account %s: %w", account.ID, err) return false, err }