[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

@@ -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

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)
 }

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(),

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)
 }

internal/typeutils/astointernal.go

@@ -18,6 +18,7 @@
 package typeutils
 
 import (
+	"cmp"
 	"context"
 	"errors"
 	"net/url"
@@ -33,10 +34,24 @@ import (
 	"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)
 	}
 

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))
 }

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
 }