From 5ca86b1c575f9c42ad8d3d4a2b2d3e70c89e90df Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Fri, 19 Jan 2024 14:02:04 +0100 Subject: [PATCH] [chore] Harden up boolptr logic on Accounts, warn if not set (#2544) --- internal/ap/extract.go | 12 -- internal/ap/properties.go | 23 ++++ internal/db/bundb/account_test.go | 1 + internal/typeutils/astointernal.go | 20 +-- internal/typeutils/astointernal_test.go | 157 ++++++++++++++++++++++- internal/typeutils/internaltofrontend.go | 33 ++++- 6 files changed, 208 insertions(+), 38 deletions(-) diff --git a/internal/ap/extract.go b/internal/ap/extract.go index 11c20494c..2b49c6477 100644 --- a/internal/ap/extract.go +++ b/internal/ap/extract.go @@ -492,18 +492,6 @@ func ExtractFields(i WithAttachment) []*gtsmodel.Field { return fields } -// ExtractDiscoverable extracts the Discoverable boolean -// of the given WithDiscoverable interface. Will return -// an error if Discoverable was nil. -func ExtractDiscoverable(i WithDiscoverable) (bool, error) { - discoverableProp := i.GetTootDiscoverable() - if discoverableProp == nil { - return false, gtserror.New("discoverable was nil") - } - - return discoverableProp.Get(), nil -} - // ExtractURL extracts the first URI it can find from the // given WithURL interface, or an error if no URL was set. // The ID of a type will not work, this function wants a URI diff --git a/internal/ap/properties.go b/internal/ap/properties.go index 2b23c7cb2..6103608d6 100644 --- a/internal/ap/properties.go +++ b/internal/ap/properties.go @@ -424,6 +424,8 @@ func SetVotersCount(with WithVotersCount, count int) { } // GetDiscoverable returns the boolean contained in the Discoverable property of 'with'. +// +// Returns default 'false' if property unusable or not set. func GetDiscoverable(with WithDiscoverable) bool { discoverProp := with.GetTootDiscoverable() if discoverProp == nil || !discoverProp.IsXMLSchemaBoolean() { @@ -442,6 +444,27 @@ func SetDiscoverable(with WithDiscoverable, discoverable bool) { discoverProp.Set(discoverable) } +// GetManuallyApprovesFollowers returns the boolean contained in the ManuallyApprovesFollowers property of 'with'. +// +// Returns default 'true' if property unusable or not set. +func GetManuallyApprovesFollowers(with WithManuallyApprovesFollowers) bool { + mafProp := with.GetActivityStreamsManuallyApprovesFollowers() + if mafProp == nil || !mafProp.IsXMLSchemaBoolean() { + return true + } + return mafProp.Get() +} + +// SetManuallyApprovesFollowers sets the given boolean on the ManuallyApprovesFollowers property of 'with'. +func SetManuallyApprovesFollowers(with WithManuallyApprovesFollowers, manuallyApprovesFollowers bool) { + mafProp := with.GetActivityStreamsManuallyApprovesFollowers() + if mafProp == nil { + mafProp = streams.NewActivityStreamsManuallyApprovesFollowersProperty() + with.SetActivityStreamsManuallyApprovesFollowers(mafProp) + } + mafProp.Set(manuallyApprovesFollowers) +} + func getIRIs[T TypeOrIRI](prop Property[T]) []*url.URL { if prop == nil || prop.Len() == 0 { return nil diff --git a/internal/db/bundb/account_test.go b/internal/db/bundb/account_test.go index e03300700..d7f0c1c72 100644 --- a/internal/db/bundb/account_test.go +++ b/internal/db/bundb/account_test.go @@ -336,6 +336,7 @@ func (suite *AccountTestSuite) TestInsertAccountWithDefaults() { suite.Equal("en", newAccount.Language) suite.WithinDuration(time.Now(), newAccount.CreatedAt, 30*time.Second) suite.WithinDuration(time.Now(), newAccount.UpdatedAt, 30*time.Second) + suite.True(*newAccount.Locked) suite.False(*newAccount.Memorial) suite.False(*newAccount.Bot) suite.False(*newAccount.Discoverable) diff --git a/internal/typeutils/astointernal.go b/internal/typeutils/astointernal.go index ec17527c4..8a451adc8 100644 --- a/internal/typeutils/astointernal.go +++ b/internal/typeutils/astointernal.go @@ -126,23 +126,9 @@ func (c *Converter) ASRepresentationToAccount(ctx context.Context, accountable a acct.Sensitive = util.Ptr(false) acct.HideCollections = util.Ptr(false) - // Extract 'manuallyApprovesFollowers', (i.e. locked account) - maf := accountable.GetActivityStreamsManuallyApprovesFollowers() - - switch { - case maf != nil && !maf.IsXMLSchemaBoolean(): - log.Warnf(ctx, "unusable manuallyApprovesFollowers for %s", uri) - fallthrough - - case maf == nil: - // None given, use default. - acct.Locked = util.Ptr(true) - - default: - // Valid bool provided. - locked := maf.Get() - acct.Locked = &locked - } + // Extract 'manuallyApprovesFollowers' aka locked account (default = true). + manuallyApprovesFollowers := ap.GetManuallyApprovesFollowers(accountable) + acct.Locked = &manuallyApprovesFollowers // Extract account discoverability (default = false). discoverable := ap.GetDiscoverable(accountable) diff --git a/internal/typeutils/astointernal_test.go b/internal/typeutils/astointernal_test.go index 4be2434e5..006a26fe7 100644 --- a/internal/typeutils/astointernal_test.go +++ b/internal/typeutils/astointernal_test.go @@ -27,6 +27,7 @@ "github.com/superseriousbusiness/activity/streams" "github.com/superseriousbusiness/activity/streams/vocab" "github.com/superseriousbusiness/gotosocial/internal/ap" + "github.com/superseriousbusiness/gotosocial/internal/cache" "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" ) @@ -35,6 +36,17 @@ type ASToInternalTestSuite struct { } func (suite *ASToInternalTestSuite) jsonToType(in string) vocab.Type { + ctx := context.Background() + b := []byte(in) + + if accountable, err := ap.ResolveAccountable(ctx, b); err == nil { + return accountable + } + + if statusable, err := ap.ResolveStatusable(ctx, b); err == nil { + return statusable + } + m := make(map[string]interface{}) if err := json.Unmarshal([]byte(in), &m); err != nil { suite.FailNow(err.Error()) @@ -45,10 +57,6 @@ func (suite *ASToInternalTestSuite) jsonToType(in string) vocab.Type { suite.FailNow(err.Error()) } - if statusable, ok := t.(ap.Statusable); ok { - ap.NormalizeIncomingContent(statusable, m) - } - return t } @@ -195,7 +203,7 @@ func (suite *ASToInternalTestSuite) TestParseOwncastService() { suite.Equal("https://owncast.example.org/logo/external", acct.AvatarRemoteURL) suite.Equal("https://owncast.example.org/logo/external", acct.HeaderRemoteURL) suite.Equal("Rob's Owncast Server", acct.DisplayName) - suite.Equal("linux audio stuff ", acct.Note) + suite.Equal("linux audio stuff", acct.Note) suite.True(*acct.Bot) suite.False(*acct.Locked) suite.True(*acct.Discoverable) @@ -503,6 +511,145 @@ func (suite *ASToInternalTestSuite) TestParseAnnounce() { suite.Nil(boost.BoostOfAccount) } +func (suite *ASToInternalTestSuite) TestParseHonkAccount() { + // Hopefully comprehensive checks for + // https://github.com/superseriousbusiness/gotosocial/issues/2527. + + const honk_user = `{ + "@context": "https://www.w3.org/ns/activitystreams", + "chatKeyV0": "vIT5wj9bJqGkvwBxhmaz4Lh4eZIeOKnsSIQifShmJUY=", + "followers": "https://honk.example.org/u/honk_user/followers", + "following": "https://honk.example.org/u/honk_user/following", + "icon": { + "mediaType": "image/png", + "type": "Image", + "url": "https://honk.example.org/a?a=https%3A%2F%2Fhonk.example.org%2Fu%2Fhonk_user" + }, + "id": "https://honk.example.org/u/honk_user", + "inbox": "https://honk.example.org/u/honk_user/inbox", + "name": "honk_user", + "outbox": "https://honk.example.org/u/honk_user/outbox", + "preferredUsername": "honk_user", + "publicKey": { + "id": "https://honk.example.org/u/honk_user#key", + "owner": "https://honk.example.org/u/honk_user", + "publicKeyPem": "-----BEGIN PUBLIC KEY-----\nMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA593GZ9TYrvWgMaMKQ6k6\ngkItUapUgNnNXzU9J63GRtYZ7CE/Zi39Kgpsxu77hHBj34vwjr1Oc9AMrVDIMfu9\nEirW1RWxPvrjThBU56VgkpkAXVsieaffJo80BA00QzV4x69Jgat6OT7ox/HMvMxR\nyZ6CXNCPKQALYqQF6v1fX1kO9lhIA+mPd0JN/qMKvZfd1NXABEk9nORUneH7Audt\nIHNdJzKMHC6wPSQWC7SmXT0/nq6o5mR2SgvwTI/JUx6T5r8NDrwSaqB69e+EMJqR\nxKOh9N4A1ba/AQOQZbO/YkFyYY2VE4HWbvS9XpYL74yT9D6Fp4cUovJiXC+ziam0\nNwIDAQAB\n-----END PUBLIC KEY-----\n" + }, + "summary": "

Honk account

", + "type": "Person", + "url": "https://honk.example.org/u/honk_user" +}` + + t := suite.jsonToType(honk_user) + rep, ok := t.(ap.Accountable) + if !ok { + suite.FailNow("type not coercible") + } + + 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) + suite.Equal(`https://honk.example.org/a?a=https%3A%2F%2Fhonk.example.org%2Fu%2Fhonk_user`, acct.AvatarRemoteURL) + suite.Equal("

Honk account

", acct.Note) + suite.Equal("https://honk.example.org/u/honk_user", acct.URI) + suite.Equal("https://honk.example.org/u/honk_user", acct.URL) + suite.Equal("honk_user", acct.Username) + suite.Equal("honk.example.org", acct.Domain) + suite.True(*acct.Locked) + suite.False(*acct.Discoverable) + + // Store the account representation. + acct.ID = "01HMGRMAVQMYQC3DDQ29TPQKJ3" // <- needs an ID + ctx := context.Background() + if err := suite.db.PutAccount(ctx, acct); err != nil { + suite.FailNow(err.Error()) + } + + // Double check fields. + suite.Equal("https://honk.example.org/u/honk_user/followers", acct.FollowersURI) + suite.Equal("https://honk.example.org/u/honk_user/following", acct.FollowingURI) + suite.Equal(`https://honk.example.org/a?a=https%3A%2F%2Fhonk.example.org%2Fu%2Fhonk_user`, acct.AvatarRemoteURL) + suite.Equal("

Honk account

", acct.Note) + suite.Equal("https://honk.example.org/u/honk_user", acct.URI) + suite.Equal("https://honk.example.org/u/honk_user", acct.URL) + suite.Equal("honk_user", acct.Username) + suite.Equal("honk.example.org", acct.Domain) + suite.True(*acct.Locked) + suite.False(*acct.Discoverable) + + // Check DB version. + dbAcct, err := suite.db.GetAccountByID(ctx, acct.ID) + if err != nil { + suite.FailNow(err.Error()) + } + + suite.Equal("https://honk.example.org/u/honk_user/followers", dbAcct.FollowersURI) + suite.Equal("https://honk.example.org/u/honk_user/following", dbAcct.FollowingURI) + suite.Equal(`https://honk.example.org/a?a=https%3A%2F%2Fhonk.example.org%2Fu%2Fhonk_user`, dbAcct.AvatarRemoteURL) + suite.Equal("

Honk account

", dbAcct.Note) + suite.Equal("https://honk.example.org/u/honk_user", dbAcct.URI) + suite.Equal("https://honk.example.org/u/honk_user", dbAcct.URL) + suite.Equal("honk_user", dbAcct.Username) + suite.Equal("honk.example.org", dbAcct.Domain) + suite.True(*dbAcct.Locked) + suite.False(*dbAcct.Discoverable) + + // Update the account. + if err := suite.db.UpdateAccount(ctx, acct); err != nil { + suite.FailNow(err.Error()) + } + + // Double check fields. + suite.Equal("https://honk.example.org/u/honk_user/followers", acct.FollowersURI) + suite.Equal("https://honk.example.org/u/honk_user/following", acct.FollowingURI) + suite.Equal(`https://honk.example.org/a?a=https%3A%2F%2Fhonk.example.org%2Fu%2Fhonk_user`, acct.AvatarRemoteURL) + suite.Equal("

Honk account

", acct.Note) + suite.Equal("https://honk.example.org/u/honk_user", acct.URI) + suite.Equal("https://honk.example.org/u/honk_user", acct.URL) + suite.Equal("honk_user", acct.Username) + suite.Equal("honk.example.org", acct.Domain) + suite.True(*acct.Locked) + suite.False(*acct.Discoverable) + + // Check DB version. + dbAcct, err = suite.db.GetAccountByID(ctx, acct.ID) + if err != nil { + suite.FailNow(err.Error()) + } + + suite.Equal("https://honk.example.org/u/honk_user/followers", dbAcct.FollowersURI) + suite.Equal("https://honk.example.org/u/honk_user/following", dbAcct.FollowingURI) + suite.Equal(`https://honk.example.org/a?a=https%3A%2F%2Fhonk.example.org%2Fu%2Fhonk_user`, dbAcct.AvatarRemoteURL) + suite.Equal("

Honk account

", dbAcct.Note) + suite.Equal("https://honk.example.org/u/honk_user", dbAcct.URI) + suite.Equal("https://honk.example.org/u/honk_user", dbAcct.URL) + suite.Equal("honk_user", dbAcct.Username) + suite.Equal("honk.example.org", dbAcct.Domain) + suite.True(*dbAcct.Locked) + suite.False(*dbAcct.Discoverable) + + // Clear caches. + suite.state.Caches.GTS = cache.GTSCaches{} + suite.state.Caches.GTS.Init() + + dbAcct, err = suite.db.GetAccountByID(ctx, acct.ID) + if err != nil { + suite.FailNow(err.Error()) + } + + suite.Equal("https://honk.example.org/u/honk_user/followers", dbAcct.FollowersURI) + suite.Equal("https://honk.example.org/u/honk_user/following", dbAcct.FollowingURI) + suite.Equal(`https://honk.example.org/a?a=https%3A%2F%2Fhonk.example.org%2Fu%2Fhonk_user`, dbAcct.AvatarRemoteURL) + suite.Equal("

Honk account

", dbAcct.Note) + suite.Equal("https://honk.example.org/u/honk_user", dbAcct.URI) + suite.Equal("https://honk.example.org/u/honk_user", dbAcct.URL) + suite.Equal("honk_user", dbAcct.Username) + suite.Equal("honk.example.org", dbAcct.Domain) + suite.True(*dbAcct.Locked) + suite.False(*dbAcct.Discoverable) +} + func TestASToInternalTestSuite(t *testing.T) { suite.Run(t, new(ASToInternalTestSuite)) } diff --git a/internal/typeutils/internaltofrontend.go b/internal/typeutils/internaltofrontend.go index 941b9e866..ed2979049 100644 --- a/internal/typeutils/internaltofrontend.go +++ b/internal/typeutils/internaltofrontend.go @@ -217,6 +217,31 @@ func (c *Converter) AccountToAPIAccountPublic(ctx context.Context, a *gtsmodel.A } } + // Bool ptrs should be set, but warn + // and use a default if they're not. + var boolPtrDef = func( + pName string, + p *bool, + d bool, + ) bool { + if p != nil { + return *p + } + + log.Warnf(ctx, + "%s ptr was nil, using default %t", + pName, d, + ) + return d + } + + var ( + locked = boolPtrDef("locked", a.Locked, true) + discoverable = boolPtrDef("discoverable", a.Discoverable, false) + bot = boolPtrDef("bot", a.Bot, false) + enableRSS = boolPtrDef("enableRSS", a.EnableRSS, false) + ) + // Remaining properties are simple and // can be populated directly below. @@ -225,9 +250,9 @@ func (c *Converter) AccountToAPIAccountPublic(ctx context.Context, a *gtsmodel.A Username: a.Username, Acct: acct, DisplayName: a.DisplayName, - Locked: *a.Locked, - Discoverable: *a.Discoverable, - Bot: *a.Bot, + Locked: locked, + Discoverable: discoverable, + Bot: bot, CreatedAt: util.FormatISO8601(a.CreatedAt), Note: a.Note, URL: a.URL, @@ -243,7 +268,7 @@ func (c *Converter) AccountToAPIAccountPublic(ctx context.Context, a *gtsmodel.A Fields: fields, Suspended: !a.SuspendedAt.IsZero(), CustomCSS: a.CustomCSS, - EnableRSS: *a.EnableRSS, + EnableRSS: enableRSS, Role: role, Moved: moved, }