From 634d4f408f5a19d0ef93b7b7d6a4e3c552cff810 Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Sat, 18 Jan 2025 17:55:27 +0000 Subject: [PATCH] [bugfix] Fix existing perm adoption (#3651) * [bugfix] Fix existing perm adoption * go fmt * test, small log fix --- internal/gtsmodel/domainallow.go | 4 + internal/gtsmodel/domainblock.go | 4 + internal/gtsmodel/domainpermission.go | 4 + internal/gtsmodel/domainpermissiondraft.go | 4 + internal/gtsmodel/domainpermissionexclude.go | 1 + internal/subscriptions/domainperms.go | 94 ++++++++++--- internal/subscriptions/subscriptions_test.go | 132 +++++++++++++++++++ 7 files changed, 225 insertions(+), 18 deletions(-) diff --git a/internal/gtsmodel/domainallow.go b/internal/gtsmodel/domainallow.go index 7b947333b..3a7ca8774 100644 --- a/internal/gtsmodel/domainallow.go +++ b/internal/gtsmodel/domainallow.go @@ -104,3 +104,7 @@ func (d *DomainAllow) SetSubscriptionID(i string) { func (d *DomainAllow) GetType() DomainPermissionType { return DomainPermissionAllow } + +func (d *DomainAllow) IsOrphan() bool { + return d.SubscriptionID == "" +} diff --git a/internal/gtsmodel/domainblock.go b/internal/gtsmodel/domainblock.go index e99fea301..4a0e1c5b7 100644 --- a/internal/gtsmodel/domainblock.go +++ b/internal/gtsmodel/domainblock.go @@ -104,3 +104,7 @@ func (d *DomainBlock) SetSubscriptionID(i string) { func (d *DomainBlock) GetType() DomainPermissionType { return DomainPermissionBlock } + +func (d *DomainBlock) IsOrphan() bool { + return d.SubscriptionID == "" +} diff --git a/internal/gtsmodel/domainpermission.go b/internal/gtsmodel/domainpermission.go index f1db4de59..b08036a91 100644 --- a/internal/gtsmodel/domainpermission.go +++ b/internal/gtsmodel/domainpermission.go @@ -43,6 +43,10 @@ type DomainPermission interface { GetSubscriptionID() string SetSubscriptionID(i string) GetType() DomainPermissionType + + // Return true if this DomainPermission + // does not have a subscription id set. + IsOrphan() bool } // Domain permission type. diff --git a/internal/gtsmodel/domainpermissiondraft.go b/internal/gtsmodel/domainpermissiondraft.go index 0829dca16..d233df6f9 100644 --- a/internal/gtsmodel/domainpermissiondraft.go +++ b/internal/gtsmodel/domainpermissiondraft.go @@ -104,3 +104,7 @@ func (d *DomainPermissionDraft) SetSubscriptionID(i string) { func (d *DomainPermissionDraft) GetType() DomainPermissionType { return d.PermissionType } + +func (d *DomainPermissionDraft) IsOrphan() bool { + return d.SubscriptionID == "" +} diff --git a/internal/gtsmodel/domainpermissionexclude.go b/internal/gtsmodel/domainpermissionexclude.go index 2a0925ba7..10492a5c1 100644 --- a/internal/gtsmodel/domainpermissionexclude.go +++ b/internal/gtsmodel/domainpermissionexclude.go @@ -90,3 +90,4 @@ func (d *DomainPermissionExclude) SetObfuscate(_ *bool) {} func (d *DomainPermissionExclude) GetSubscriptionID() string { return "" } func (d *DomainPermissionExclude) SetSubscriptionID(_ string) {} func (d *DomainPermissionExclude) GetType() DomainPermissionType { return DomainPermissionUnknown } +func (d *DomainPermissionExclude) IsOrphan() bool { return true } diff --git a/internal/subscriptions/domainperms.go b/internal/subscriptions/domainperms.go index dc4224899..ee4730023 100644 --- a/internal/subscriptions/domainperms.go +++ b/internal/subscriptions/domainperms.go @@ -317,7 +317,7 @@ func (s *Subscriptions) ProcessDomainPermissionSubscription( // Iterate through wantedPerms and // create (or dry create) each one. for _, wantedPerm := range wantedPerms { - l = l.WithField("domain", wantedPerm.GetDomain()) + l := l.WithField("domain", wantedPerm.GetDomain()) created, err := s.processDomainPermission( ctx, l, wantedPerm, @@ -486,26 +486,53 @@ func (s *Subscriptions) processDomainPermission( // side effects of permission. err = s.state.AdminActions.Run(ctx, action, actionF) - case existingPerm.GetSubscriptionID() != "" || *permSub.AdoptOrphans: - // Perm exists but we should adopt/take - // it by copying over desired fields. - existingPerm.SetCreatedByAccountID(wantedPerm.GetCreatedByAccountID()) - existingPerm.SetCreatedByAccount(wantedPerm.GetCreatedByAccount()) - existingPerm.SetSubscriptionID(permSub.ID) - existingPerm.SetObfuscate(wantedPerm.GetObfuscate()) - existingPerm.SetPrivateComment(wantedPerm.GetPrivateComment()) - existingPerm.SetPublicComment(wantedPerm.GetPublicComment()) - - switch p := existingPerm.(type) { - case *gtsmodel.DomainBlock: - err = s.state.DB.UpdateDomainBlock(ctx, p) - case *gtsmodel.DomainAllow: - err = s.state.DB.UpdateDomainAllow(ctx, p) + case existingPerm.IsOrphan(): + // Perm already exists, but it's not managed + // by a subscription, ie., it's an orphan. + if !*permSub.AdoptOrphans { + l.Debug("permission exists as an orphan that we shouldn't adopt, skipping") + return false, nil } + // Orphan is adoptable, so adopt + // it by rewriting some fields. + // + // TODO: preserve previous private + // + public comment in some way. + l.Debug("adopting orphan permission") + err = s.adoptPerm( + ctx, + existingPerm, + permSub, + wantedPerm.GetObfuscate(), + permSub.URI, + wantedPerm.GetPublicComment(), + ) + + case existingPerm.GetSubscriptionID() != permSub.ID: + // Perm already exists, and is managed + // by a lower-priority subscription. + // Take it for ourselves. + // + // TODO: preserve previous private + // + public comment in some way. + l.Debug("taking over permission from lower-priority subscription") + err = s.adoptPerm( + ctx, + existingPerm, + permSub, + wantedPerm.GetObfuscate(), + permSub.URI, + wantedPerm.GetPublicComment(), + ) + default: - // Perm exists but we should leave it alone. - l.Debug("domain is covered by a higher-priority subscription, skipping") + // Perm exists and is managed by us. + // + // TODO: update public/private comment + // from latest version if it's changed. + l.Debug("permission already exists and is managed by this subscription, skipping") + return false, nil } if err != nil && !errors.Is(err, db.ErrAlreadyExists) { @@ -829,3 +856,34 @@ func(permSub *gtsmodel.DomainPermissionSubscription) bool { return } + +func (s *Subscriptions) adoptPerm( + ctx context.Context, + perm gtsmodel.DomainPermission, + permSub *gtsmodel.DomainPermissionSubscription, + obfuscate *bool, + privateComment string, + publicComment string, +) error { + // Set to our sub ID + this subs's + // account as we're managing it now. + perm.SetSubscriptionID(permSub.ID) + perm.SetCreatedByAccountID(permSub.CreatedByAccount.ID) + perm.SetCreatedByAccount(permSub.CreatedByAccount) + + // Set new metadata on the perm. + perm.SetObfuscate(obfuscate) + perm.SetPrivateComment(privateComment) + perm.SetPublicComment(publicComment) + + // Update the perm in the db. + var err error + switch p := perm.(type) { + case *gtsmodel.DomainBlock: + err = s.state.DB.UpdateDomainBlock(ctx, p) + case *gtsmodel.DomainAllow: + err = s.state.DB.UpdateDomainAllow(ctx, p) + } + + return err +} diff --git a/internal/subscriptions/subscriptions_test.go b/internal/subscriptions/subscriptions_test.go index 1f6e437fb..6c47fed21 100644 --- a/internal/subscriptions/subscriptions_test.go +++ b/internal/subscriptions/subscriptions_test.go @@ -533,6 +533,138 @@ func (suite *SubscriptionsTestSuite) TestDomainBlocksWrongContentTypePlain() { suite.Equal(`fetch successful but parsed zero usable results`, permSub.Error) } +func (suite *SubscriptionsTestSuite) TestAdoption() { + var ( + ctx = context.Background() + testStructs = testrig.SetupTestStructs(rMediaPath, rTemplatePath) + testAccount = suite.testAccounts["admin_account"] + subscriptions = subscriptions.New( + testStructs.State, + testStructs.TransportController, + testStructs.TypeConverter, + ) + + // A subscription for a plain list + // of baddies, which adopts orphans. + testSubscription = >smodel.DomainPermissionSubscription{ + ID: "01JGE681TQSBPAV59GZXPKE62H", + Priority: 255, + Title: "whatever!", + PermissionType: gtsmodel.DomainPermissionBlock, + AsDraft: util.Ptr(false), + AdoptOrphans: util.Ptr(true), + CreatedByAccountID: testAccount.ID, + CreatedByAccount: testAccount, + URI: "https://lists.example.org/baddies.txt", + ContentType: gtsmodel.DomainPermSubContentTypePlain, + } + + // A lower-priority subscription + // than the one we're testing. + existingSubscription = >smodel.DomainPermissionSubscription{ + ID: "01JHX2ENFM8YGGR9Q9J5S66KSB", + Priority: 128, + Title: "lower prio subscription", + PermissionType: gtsmodel.DomainPermissionBlock, + AsDraft: util.Ptr(false), + AdoptOrphans: util.Ptr(false), + CreatedByAccountID: testAccount.ID, + CreatedByAccount: testAccount, + URI: "https://whatever.example.org/lowerprios.txt", + ContentType: gtsmodel.DomainPermSubContentTypePlain, + } + + // Orphan block which also exists + // on the list of testSubscription. + existingBlock1 = >smodel.DomainBlock{ + ID: "01JHX2V5WN250TKB6FQ1M3QE1H", + Domain: "bumfaces.net", + CreatedByAccount: testAccount, + CreatedByAccountID: testAccount.ID, + } + + // Block managed by existingSubscription which + // also exists on the list of testSubscription. + existingBlock2 = >smodel.DomainBlock{ + ID: "01JHX3EZAYG3KKC56C1YTKBRK7", + Domain: "peepee.poopoo", + CreatedByAccount: testAccount, + CreatedByAccountID: testAccount.ID, + SubscriptionID: existingSubscription.ID, + } + + // Already existing block + // managed by testSubscription. + existingBlock3 = >smodel.DomainBlock{ + ID: "01JHX3N1AGYT72BR3TWDCZKYGE", + Domain: "nothanks.com", + CreatedByAccount: testAccount, + CreatedByAccountID: testAccount.ID, + SubscriptionID: testSubscription.ID, + } + ) + defer testrig.TearDownTestStructs(testStructs) + + // Store test subscription. + if err := testStructs.State.DB.PutDomainPermissionSubscription( + ctx, testSubscription, + ); err != nil { + suite.FailNow(err.Error()) + } + + // Store the lower-priority subscription. + if err := testStructs.State.DB.PutDomainPermissionSubscription( + ctx, existingSubscription, + ); err != nil { + suite.FailNow(err.Error()) + } + + // Store the existing blocks. + for _, block := range []*gtsmodel.DomainBlock{ + existingBlock1, + existingBlock2, + existingBlock3, + } { + if err := testStructs.State.DB.CreateDomainBlock( + ctx, block, + ); err != nil { + suite.FailNow(err.Error()) + } + } + + // Process all subscriptions. + subscriptions.ProcessDomainPermissionSubscriptions(ctx, testSubscription.PermissionType) + + var err error + + // existingBlock1 should now be adopted by + // testSubscription, as it previously an orphan. + if existingBlock1, err = testStructs.State.DB.GetDomainBlockByID( + ctx, existingBlock1.ID, + ); err != nil { + suite.FailNow(err.Error()) + } + suite.Equal(testSubscription.ID, existingBlock1.SubscriptionID) + + // existingBlock2 should now be + // managed by testSubscription. + if existingBlock2, err = testStructs.State.DB.GetDomainBlockByID( + ctx, existingBlock2.ID, + ); err != nil { + suite.FailNow(err.Error()) + } + suite.Equal(testSubscription.ID, existingBlock2.SubscriptionID) + + // existingBlock3 should still be + // managed by testSubscription. + if existingBlock3, err = testStructs.State.DB.GetDomainBlockByID( + ctx, existingBlock3.ID, + ); err != nil { + suite.FailNow(err.Error()) + } + suite.Equal(testSubscription.ID, existingBlock3.SubscriptionID) +} + func TestSubscriptionTestSuite(t *testing.T) { suite.Run(t, new(SubscriptionsTestSuite)) }