From 5212a1057ed05085d4d332976510880c6a692a8e Mon Sep 17 00:00:00 2001 From: kim <89579420+NyaaaWhatsUpDoc@users.noreply.github.com> Date: Tue, 13 Aug 2024 09:01:50 +0000 Subject: [PATCH] [bugfix] relax missing preferred_username, instead using webfingered username (#3189) * support no preferred_username, instead using webfingered username * add tests for the new preferred_username behaviour --- internal/ap/extract.go | 11 +--- .../api/activitypub/users/userget_test.go | 4 +- internal/federation/dereferencing/account.go | 39 +++++++---- internal/federation/dereferencing/finger.go | 18 ++--- internal/typeutils/astointernal.go | 36 ++++++++-- internal/typeutils/astointernal_test.go | 66 +++++++++++++++++-- internal/util/punycode.go | 51 +++++--------- 7 files changed, 148 insertions(+), 77 deletions(-) diff --git a/internal/ap/extract.go b/internal/ap/extract.go index ce1e2d421..f69fb299e 100644 --- a/internal/ap/extract.go +++ b/internal/ap/extract.go @@ -195,17 +195,12 @@ func ExtractPollOptionables(arr []TypeOrIRI) ([]PollOptionable, []TypeOrIRI) { // ExtractPreferredUsername returns a string representation of // an interface's preferredUsername property. Will return an // error if preferredUsername is nil, not a string, or empty. -func ExtractPreferredUsername(i WithPreferredUsername) (string, error) { +func ExtractPreferredUsername(i WithPreferredUsername) string { u := i.GetActivityStreamsPreferredUsername() if u == nil || !u.IsXMLSchemaString() { - return "", gtserror.New("preferredUsername nil or not a string") + return "" } - - if u.GetXMLSchemaString() == "" { - return "", gtserror.New("preferredUsername was empty") - } - - return u.GetXMLSchemaString(), nil + return u.GetXMLSchemaString() } // ExtractName returns the first string representation it diff --git a/internal/api/activitypub/users/userget_test.go b/internal/api/activitypub/users/userget_test.go index 2a68b309c..504b69806 100644 --- a/internal/api/activitypub/users/userget_test.go +++ b/internal/api/activitypub/users/userget_test.go @@ -86,7 +86,7 @@ func (suite *UserGetTestSuite) TestGetUser() { suite.True(ok) // convert person to account - a, err := suite.tc.ASRepresentationToAccount(context.Background(), person, "") + a, err := suite.tc.ASRepresentationToAccount(context.Background(), person, "", "") suite.NoError(err) suite.EqualValues(targetAccount.Username, a.Username) } @@ -154,7 +154,7 @@ func (suite *UserGetTestSuite) TestGetUserPublicKeyDeleted() { suite.True(ok) // convert person to account - a, err := suite.tc.ASRepresentationToAccount(context.Background(), person, "") + a, err := suite.tc.ASRepresentationToAccount(context.Background(), person, "", "") suite.NoError(err) suite.EqualValues(targetAccount.Username, a.Username) } diff --git a/internal/federation/dereferencing/account.go b/internal/federation/dereferencing/account.go index 3abf628d1..b0118380d 100644 --- a/internal/federation/dereferencing/account.go +++ b/internal/federation/dereferencing/account.go @@ -18,6 +18,7 @@ package dereferencing import ( + "cmp" "context" "errors" "net/url" @@ -509,10 +510,16 @@ func (d *Dereferencer) enrichAccount( } if account.Username != "" { - // A username was provided so we can attempt a webfinger, this ensures up-to-date accountdomain info. - accDomain, accURI, err := d.fingerRemoteAccount(ctx, tsport, account.Username, account.Domain) - switch { + // A username was provided so we can attempt to webfinger, + // this ensures up-to-date account domain, and handles some + // edge cases where servers don't provide a preferred_username. + accUsername, accDomain, accURI, err := d.fingerRemoteAccount(ctx, + tsport, + account.Username, + account.Domain, + ) + switch { case err != nil && account.URI == "": // This is a new account (to us) with username@domain // but failed webfinger, nothing more we can do. @@ -554,6 +561,9 @@ func (d *Dereferencer) enrichAccount( account.URI = accURI.String() account.Domain = accDomain uri = accURI + + // Specifically only update username if not already set. + account.Username = cmp.Or(account.Username, accUsername) } } @@ -609,7 +619,7 @@ func (d *Dereferencer) enrichAccount( if err != nil { // ResolveAccountable will set gtserror.WrongType // on the returned error, so we don't need to do it here. - err = gtserror.Newf("error resolving accountable %s: %w", uri, err) + err := gtserror.Newf("error resolving accountable %s: %w", uri, err) return nil, nil, err } @@ -656,15 +666,18 @@ func (d *Dereferencer) enrichAccount( latestAcc, err := d.converter.ASRepresentationToAccount(ctx, apubAcc, account.Domain, + account.Username, ) if err != nil { // ASRepresentationToAccount will set Malformed on the // returned error, so we don't need to do it here. - err = gtserror.Newf("error converting %s to gts model: %w", uri, err) + err := gtserror.Newf("error converting %s to gts model: %w", uri, err) return nil, nil, err } if account.Username == "" { + var accUsername string + // Assume the host from the // ActivityPub representation. id := ap.GetJSONLDId(apubAcc) @@ -685,7 +698,7 @@ func (d *Dereferencer) enrichAccount( // https://example.org/@someone@somewhere.else and we've been redirected // from example.org to somewhere.else: we want to take somewhere.else // as the accountDomain then, not the example.org we were redirected from. - latestAcc.Domain, _, err = d.fingerRemoteAccount(ctx, + accUsername, latestAcc.Domain, _, err = d.fingerRemoteAccount(ctx, tsport, latestAcc.Username, accHost, @@ -698,6 +711,9 @@ func (d *Dereferencer) enrichAccount( latestAcc.Username, accHost, err, ) } + + // Specifically only update username if not already set. + latestAcc.Username = cmp.Or(latestAcc.Username, accUsername) } if latestAcc.Domain == "" { @@ -706,23 +722,20 @@ func (d *Dereferencer) enrichAccount( return nil, nil, gtserror.Newf("empty domain for %s", uri) } - // Ensure the final parsed account URI or URL matches + // Ensure the final parsed account URI matches // the input URI we fetched (or received) it as. - matches, err := util.URIMatches( + if matches, err := util.URIMatches( uri, append( ap.GetURL(apubAcc), // account URL(s) ap.GetJSONLDId(apubAcc), // account URI )..., - ) - if err != nil { + ); err != nil { return nil, nil, gtserror.Newf( "error checking dereferenced account uri %s: %w", latestAcc.URI, err, ) - } - - if !matches { + } else if !matches { return nil, nil, gtserror.Newf( "dereferenced account uri %s does not match %s", latestAcc.URI, uri.String(), diff --git a/internal/federation/dereferencing/finger.go b/internal/federation/dereferencing/finger.go index 1b3e915ba..d11950a1e 100644 --- a/internal/federation/dereferencing/finger.go +++ b/internal/federation/dereferencing/finger.go @@ -45,6 +45,7 @@ func (d *Dereferencer) fingerRemoteAccount( username string, host string, ) ( + string, // discovered username string, // discovered account domain *url.URL, // discovered account URI error, @@ -55,31 +56,30 @@ func (d *Dereferencer) fingerRemoteAccount( b, err := transport.Finger(ctx, username, host) if err != nil { err = gtserror.Newf("error webfingering %s: %w", target, err) - return "", nil, err + return "", "", nil, err } var resp apimodel.WellKnownResponse if err := json.Unmarshal(b, &resp); err != nil { err = gtserror.Newf("error parsing response as JSON for %s: %w", target, err) - return "", nil, err + return "", "", nil, err } if len(resp.Links) == 0 { err = gtserror.Newf("no links found in response for %s", target) - return "", nil, err + return "", "", nil, err } if resp.Subject == "" { err = gtserror.Newf("no subject found in response for %s", target) - return "", nil, err + return "", "", nil, err } accUsername, accDomain, err := util.ExtractWebfingerParts(resp.Subject) if err != nil { - err = gtserror.Newf("error extracting subject parts for %s: %w", target, err) - return "", nil, err + return "", "", nil, gtserror.Newf("error extracting subject parts for %s: %w", target, err) } else if accUsername != username { - return "", nil, gtserror.Newf("response username does not match input for %s: %w", target, err) + return "", "", nil, gtserror.Newf("response username does not match input for %s: %w", target, err) } // Look through links for the first @@ -122,8 +122,8 @@ func (d *Dereferencer) fingerRemoteAccount( } // All looks good, return happily! - return accDomain, uri, nil + return accUsername, accDomain, uri, nil } - return "", nil, gtserror.Newf("no suitable self, AP-type link found in webfinger response for %s", target) + return "", "", nil, gtserror.Newf("no suitable self, AP-type link found in webfinger response for %s", target) } diff --git a/internal/typeutils/astointernal.go b/internal/typeutils/astointernal.go index 2946c8d09..5ff60b09c 100644 --- a/internal/typeutils/astointernal.go +++ b/internal/typeutils/astointernal.go @@ -18,6 +18,7 @@ package typeutils import ( + "cmp" "context" "errors" "net/url" @@ -33,10 +34,24 @@ "github.com/superseriousbusiness/gotosocial/internal/util" ) -// ASRepresentationToAccount converts a remote account/person/application representation into a gts model account. +// ASRepresentationToAccount converts a remote account / person +// / application representation into a gts model account. // -// If accountDomain is provided then this value will be used as the account's Domain, else the AP ID host. -func (c *Converter) ASRepresentationToAccount(ctx context.Context, accountable ap.Accountable, accountDomain string) (*gtsmodel.Account, error) { +// If accountDomain is provided then this value will be +// used as the account's Domain, else the AP ID host. +// +// If accountUsername is provided then this is used as +// a fallback when no preferredUsername is provided. Else +// a lack of username will result in error return. +func (c *Converter) ASRepresentationToAccount( + ctx context.Context, + accountable ap.Accountable, + accountDomain string, + accountUsername string, +) ( + *gtsmodel.Account, + error, +) { var err error // Extract URI from accountable @@ -70,10 +85,17 @@ func (c *Converter) ASRepresentationToAccount(ctx context.Context, accountable a return nil, gtserror.SetMalformed(err) } - // Extract preferredUsername, this is a *requirement*. - acct.Username, err = ap.ExtractPreferredUsername(accountable) - if err != nil { - err := gtserror.Newf("unusable username for %s", uri) + // Set account username. + acct.Username = cmp.Or( + + // Prefer the AP model provided username. + ap.ExtractPreferredUsername(accountable), + + // Fallback username. + accountUsername, + ) + if acct.Username == "" { + err := gtserror.Newf("missing username for %s", uri) return nil, gtserror.SetMalformed(err) } diff --git a/internal/typeutils/astointernal_test.go b/internal/typeutils/astointernal_test.go index 3efc15999..998a3e974 100644 --- a/internal/typeutils/astointernal_test.go +++ b/internal/typeutils/astointernal_test.go @@ -65,7 +65,7 @@ func (suite *ASToInternalTestSuite) jsonToType(in string) vocab.Type { func (suite *ASToInternalTestSuite) TestParsePerson() { testPerson := suite.testPeople["https://unknown-instance.com/users/brand_new_person"] - acct, err := suite.typeconverter.ASRepresentationToAccount(context.Background(), testPerson, "") + acct, err := suite.typeconverter.ASRepresentationToAccount(context.Background(), testPerson, "", "") suite.NoError(err) suite.Equal("https://unknown-instance.com/users/brand_new_person", acct.URI) @@ -87,7 +87,7 @@ func (suite *ASToInternalTestSuite) TestParsePerson() { func (suite *ASToInternalTestSuite) TestParsePersonWithSharedInbox() { testPerson := suite.testPeople["https://turnip.farm/users/turniplover6969"] - acct, err := suite.typeconverter.ASRepresentationToAccount(context.Background(), testPerson, "") + acct, err := suite.typeconverter.ASRepresentationToAccount(context.Background(), testPerson, "", "") suite.NoError(err) suite.Equal("https://turnip.farm/users/turniplover6969", acct.URI) @@ -145,7 +145,7 @@ func (suite *ASToInternalTestSuite) TestParseGargron() { suite.FailNow("type not coercible") } - acct, err := suite.typeconverter.ASRepresentationToAccount(context.Background(), rep, "") + acct, err := suite.typeconverter.ASRepresentationToAccount(context.Background(), rep, "", "") suite.NoError(err) suite.Equal("https://mastodon.social/inbox", *acct.SharedInboxURI) suite.Equal([]string{"https://tooting.ai/users/Gargron"}, acct.AlsoKnownAsURIs) @@ -196,7 +196,7 @@ func (suite *ASToInternalTestSuite) TestParseOwncastService() { suite.FailNow("type not coercible") } - acct, err := suite.typeconverter.ASRepresentationToAccount(context.Background(), rep, "") + acct, err := suite.typeconverter.ASRepresentationToAccount(context.Background(), rep, "", "") suite.NoError(err) suite.Equal("rgh", acct.Username) @@ -547,7 +547,7 @@ func (suite *ASToInternalTestSuite) TestParseHonkAccount() { suite.FailNow("type not coercible") } - acct, err := suite.typeconverter.ASRepresentationToAccount(context.Background(), rep, "") + acct, err := suite.typeconverter.ASRepresentationToAccount(context.Background(), rep, "", "") suite.NoError(err) suite.Equal("https://honk.example.org/u/honk_user/followers", acct.FollowersURI) suite.Equal("https://honk.example.org/u/honk_user/following", acct.FollowingURI) @@ -651,6 +651,62 @@ func (suite *ASToInternalTestSuite) TestParseHonkAccount() { suite.False(*dbAcct.Discoverable) } +func (suite *ASToInternalTestSuite) TestParseAccountableWithoutPreferredUsername() { + ctx, cncl := context.WithCancel(context.Background()) + defer cncl() + + testPerson := suite.testPeople["https://unknown-instance.com/users/brand_new_person"] + // preferredUsername := "newish_person_actually" + username := "brand_new_person" + + // Specifically unset the preferred_username field. + testPerson.SetActivityStreamsPreferredUsername(nil) + + // Attempt to parse account model from ActivityStreams. + // This should fall back to the passed username argument as no preferred_username is set. + acc, err := suite.typeconverter.ASRepresentationToAccount(ctx, testPerson, "", username) + suite.NoError(err) + suite.Equal(acc.Username, username) +} + +func (suite *ASToInternalTestSuite) TestParseAccountableWithoutAnyUsername() { + ctx, cncl := context.WithCancel(context.Background()) + defer cncl() + + testPerson := suite.testPeople["https://unknown-instance.com/users/brand_new_person"] + // preferredUsername := "newish_person_actually" + // username := "brand_new_person" + + // Specifically unset the preferred_username field. + testPerson.SetActivityStreamsPreferredUsername(nil) + + // Attempt to parse account model from ActivityStreams. + // This should return error as we provide no username and no preferred_username is set. + acc, err := suite.typeconverter.ASRepresentationToAccount(ctx, testPerson, "", "") + suite.Equal(err.Error(), "ASRepresentationToAccount: missing username for https://unknown-instance.com/users/brand_new_person") + suite.Nil(acc) +} + +func (suite *ASToInternalTestSuite) TestParseAccountableWithPreferredUsername() { + ctx, cncl := context.WithCancel(context.Background()) + defer cncl() + + testPerson := suite.testPeople["https://unknown-instance.com/users/brand_new_person"] + preferredUsername := "newish_person_actually" + username := "brand_new_person" + + // Specifically set a known preferred_username field. + prop := streams.NewActivityStreamsPreferredUsernameProperty() + prop.SetXMLSchemaString(preferredUsername) + testPerson.SetActivityStreamsPreferredUsername(prop) + + // Attempt to parse account model from ActivityStreams. + // This should use the ActivityStreams preferred_username, instead of the passed argument. + acc, err := suite.typeconverter.ASRepresentationToAccount(ctx, testPerson, "", username) + suite.NoError(err) + suite.Equal(acc.Username, preferredUsername) +} + func TestASToInternalTestSuite(t *testing.T) { suite.Run(t, new(ASToInternalTestSuite)) } diff --git a/internal/util/punycode.go b/internal/util/punycode.go index cc1b57a27..3e9f71408 100644 --- a/internal/util/punycode.go +++ b/internal/util/punycode.go @@ -48,19 +48,18 @@ func DePunify(domain string) (string, error) { // any of the given URIs, taking account of punycode. func URIMatches(expect *url.URL, uris ...*url.URL) (bool, error) { // Normalize expect to punycode. - expectPuny, err := PunifyURI(expect) + expectStr, err := PunifyURIToStr(expect) if err != nil { return false, err } - expectStr := expectPuny.String() for _, uri := range uris { - uriPuny, err := PunifyURI(uri) + uriStr, err := PunifyURIToStr(uri) if err != nil { return false, err } - if uriPuny.String() == expectStr { + if uriStr == expectStr { // Looks good. return true, nil } @@ -73,40 +72,26 @@ func URIMatches(expect *url.URL, uris ...*url.URL) (bool, error) { // PunifyURI returns a copy of the given URI // with the 'host' part converted to punycode. func PunifyURI(in *url.URL) (*url.URL, error) { - // Take a copy of in. + punyHost, err := Punify(in.Host) + if err != nil { + return nil, err + } out := new(url.URL) *out = *in - - // Normalize host to punycode. - var err error - out.Host, err = Punify(in.Host) - return out, err + out.Host = punyHost + return out, nil } -// PunifyURIStr returns a copy of the given URI -// string with the 'host' part converted to punycode. -func PunifyURIStr(in string) (string, error) { - inURI, err := url.Parse(in) +// PunifyURIToStr returns given URI serialized +// with the 'host' part converted to punycode. +func PunifyURIToStr(in *url.URL) (string, error) { + punyHost, err := Punify(in.Host) if err != nil { return "", err } - - outURIPuny, err := Punify(inURI.Host) - if err != nil { - return "", err - } - - if outURIPuny == in { - // Punify did nothing, so in was - // already punified, return as-is. - return in, nil - } - - // Take a copy of in. - outURI := new(url.URL) - *outURI = *inURI - - // Normalize host to punycode. - outURI.Host = outURIPuny - return outURI.String(), err + oldHost := in.Host + in.Host = punyHost + str := in.String() + in.Host = oldHost + return str, nil }