[bugfix] Fix existing perm adoption (#3651)

* [bugfix] Fix existing perm adoption

* go fmt

* test, small log fix

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 == ""
+}

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 == ""
+}

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.

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 == ""
+}

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 }

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:
+	case existingPerm.IsOrphan():
-		// Perm exists but we should adopt/take
+		// Perm already exists, but it's not managed
-		// it by copying over desired fields.
+		// by a subscription, ie., it's an orphan.
-		existingPerm.SetCreatedByAccountID(wantedPerm.GetCreatedByAccountID())
+		if !*permSub.AdoptOrphans {
-		existingPerm.SetCreatedByAccount(wantedPerm.GetCreatedByAccount())
+			l.Debug("permission exists as an orphan that we shouldn't adopt, skipping")
-		existingPerm.SetSubscriptionID(permSub.ID)
+			return false, nil
-		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)
 		}
 
+		// 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.
+		// Perm exists and is managed by us.
-		l.Debug("domain is covered by a higher-priority subscription, skipping")
+		//
+		// 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
+}

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 = &gtsmodel.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 = &gtsmodel.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 = &gtsmodel.DomainBlock{
+			ID:                 "01JHX2V5WN250TKB6FQ1M3QE1H",
+			Domain:             "bumfaces.net",
+			CreatedByAccount:   testAccount,
+			CreatedByAccountID: testAccount.ID,
+		}
+
+		// Block managed by existingSubscription which
+		// also exists on the list of testSubscription.
+		existingBlock2 = &gtsmodel.DomainBlock{
+			ID:                 "01JHX3EZAYG3KKC56C1YTKBRK7",
+			Domain:             "peepee.poopoo",
+			CreatedByAccount:   testAccount,
+			CreatedByAccountID: testAccount.ID,
+			SubscriptionID:     existingSubscription.ID,
+		}
+
+		// Already existing block
+		// managed by testSubscription.
+		existingBlock3 = &gtsmodel.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))
 }