[bugfix] More permissive CSV parsing for perm subs, text parse fix (#3638)

* [bugfix] More permissive CSV parsing for perm subs, text parse fix

* wee

* change the way dry works, slightly

* me oh my, i'm just a little guy

* we're just normal men

internal/api/client/admin/domainpermissionsubscriptiontest_test.go

@@ -39,7 +39,7 @@ type DomainPermissionSubscriptionTestTestSuite struct {
 	AdminStandardTestSuite
 }
 
-func (suite *DomainPermissionSubscriptionTestTestSuite) TestDomainPermissionSubscriptionTest() {
+func (suite *DomainPermissionSubscriptionTestTestSuite) TestDomainPermissionSubscriptionTestCSV() {
 	var (
 		ctx         = context.Background()
 		testAccount = suite.testAccounts["admin_account"]
@@ -120,6 +120,85 @@ func (suite *DomainPermissionSubscriptionTestTestSuite) TestDomainPermissionSubs
 	suite.False(blocked)
 }
 
+func (suite *DomainPermissionSubscriptionTestTestSuite) TestDomainPermissionSubscriptionTestText() {
+	var (
+		ctx         = context.Background()
+		testAccount = suite.testAccounts["admin_account"]
+		permSub     = &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,
+		}
+	)
+
+	// Create a subscription for a plaintext list of baddies.
+	err := suite.state.DB.PutDomainPermissionSubscription(ctx, permSub)
+	if err != nil {
+		suite.FailNow(err.Error())
+	}
+
+	// Prepare the request to the /test endpoint.
+	subPath := strings.ReplaceAll(
+		admin.DomainPermissionSubscriptionTestPath,
+		":id", permSub.ID,
+	)
+	path := "/api" + subPath
+	recorder := httptest.NewRecorder()
+	ginCtx := suite.newContext(recorder, http.MethodPost, nil, path, "application/json")
+	ginCtx.Params = gin.Params{
+		gin.Param{
+			Key:   apiutil.IDKey,
+			Value: permSub.ID,
+		},
+	}
+
+	// Trigger the handler.
+	suite.adminModule.DomainPermissionSubscriptionTestPOSTHandler(ginCtx)
+	suite.Equal(http.StatusOK, recorder.Code)
+
+	// Read the body back.
+	b, err := io.ReadAll(recorder.Body)
+	if err != nil {
+		suite.FailNow(err.Error())
+	}
+
+	dst := new(bytes.Buffer)
+	if err := json.Indent(dst, b, "", "  "); err != nil {
+		suite.FailNow(err.Error())
+	}
+
+	// Ensure expected.
+	suite.Equal(`[
+  {
+    "domain": "bumfaces.net"
+  },
+  {
+    "domain": "peepee.poopoo"
+  },
+  {
+    "domain": "nothanks.com"
+  }
+]`, dst.String())
+
+	// No permissions should be created
+	// since this is a dry run / test.
+	blocked, err := suite.state.DB.AreDomainsBlocked(
+		ctx,
+		[]string{"bumfaces.net", "peepee.poopoo", "nothanks.com"},
+	)
+	if err != nil {
+		suite.FailNow(err.Error())
+	}
+	suite.False(blocked)
+}
+
 func TestDomainPermissionSubscriptionTestTestSuite(t *testing.T) {
 	suite.Run(t, &DomainPermissionSubscriptionTestTestSuite{})
 }

internal/processing/admin/domainpermissionsubscription.go

@@ -272,6 +272,12 @@ func (p *Processor) DomainPermissionSubscriptionRemove(
 		return nil, gtserror.NewErrorNotFound(err, err.Error())
 	}
 
+	// Convert to API perm sub *before* doing the deletion.
+	apiPermSub, errWithCode := p.apiDomainPermSub(ctx, permSub)
+	if errWithCode != nil {
+		return nil, errWithCode
+	}
+
 	// TODO in next PR: if removeChildren, then remove all
 	// domain permissions that are children of this domain
 	// permission subscription. If not removeChildren, then
@@ -282,7 +288,7 @@ func (p *Processor) DomainPermissionSubscriptionRemove(
 		return nil, gtserror.NewErrorInternalError(err)
 	}
 
-	return p.apiDomainPermSub(ctx, permSub)
+	return apiPermSub, nil
 }
 
 func (p *Processor) DomainPermissionSubscriptionTest(

internal/subscriptions/domainperms.go

@@ -345,6 +345,10 @@ func (s *Subscriptions) ProcessDomainPermissionSubscription(
 // processDomainPermission processes one wanted domain
 // permission discovered via a domain permission sub's URI.
 //
+// If dry == true, then the returned boolean indicates whether
+// the permission would actually be created. If dry == false,
+// the bool indicates whether the permission was created or adopted.
+//
 // Error will only be returned in case of an actual database
 // error, else the error will be logged and nil returned.
 func (s *Subscriptions) processDomainPermission(
@@ -355,22 +359,18 @@ func (s *Subscriptions) processDomainPermission(
 	higherPrios []*gtsmodel.DomainPermissionSubscription,
 	dry bool,
 ) (bool, error) {
-	// Set to true if domain permission
-	// actually (would be) created.
-	var created bool
-
 	// If domain is excluded from automatic
 	// permission creation, don't process it.
 	domain := wantedPerm.GetDomain()
 	excluded, err := s.state.DB.IsDomainPermissionExcluded(ctx, domain)
 	if err != nil {
 		// Proper db error.
-		return created, err
+		return false, err
 	}
 
 	if excluded {
 		l.Debug("domain is excluded, skipping")
-		return created, nil
+		return false, err
 	}
 
 	// Check if a permission already exists for
@@ -381,27 +381,27 @@ func (s *Subscriptions) processDomainPermission(
 	)
 	if err != nil {
 		// Proper db error.
-		return created, err
+		return false, err
 	}
 
 	if covered {
 		l.Debug("domain is covered by a higher-priority subscription, skipping")
-		return created, nil
+		return false, err
 	}
 
-	// At this point we know we
-	// should create the perm.
-	created = true
+	// True if a perm already exists.
+	// Note: != nil doesn't work because
+	// of Go interface idiosyncracies.
+	existing := !util.IsNil(existingPerm)
 
 	if dry {
-		// Don't do creation or side
-		// effects if we're dry running.
-		return created, nil
+		// If this is a dry run, return
+		// now without doing any DB changes.
+		return !existing, nil
 	}
 
 	// Handle perm creation differently depending
 	// on whether or not a perm already existed.
-	existing := !util.IsNil(existingPerm)
 	switch {
 
 	case !existing && *permSub.AsDraft:
@@ -512,11 +512,10 @@ func (s *Subscriptions) processDomainPermission(
 
 	if err != nil && !errors.Is(err, db.ErrAlreadyExists) {
 		// Proper db error.
-		return created, err
+		return false, err
 	}
 
-	created = true
-	return created, nil
+	return true, nil
 }
 
 func permsFromCSV(
@@ -533,19 +532,60 @@ func permsFromCSV(
 		return nil, gtserror.NewfAt(3, "error decoding csv column headers: %w", err)
 	}
 
-	if !slices.Equal(
-		columnHeaders,
-		[]string{
-			"#domain",
-			"#severity",
-			"#reject_media",
-			"#reject_reports",
-			"#public_comment",
-			"#obfuscate",
-		},
-	) {
+	var (
+		domainI        *int
+		severityI      *int
+		publicCommentI *int
+		obfuscateI     *int
+	)
+
+	for i, columnHeader := range columnHeaders {
+		// Remove leading # if present.
+		normal := strings.TrimLeft(columnHeader, "#")
+
+		// Find index of each column header we
+		// care about, ensuring no duplicates.
+		switch normal {
+
+		case "domain":
+			if domainI != nil {
 				body.Close()
-		err := gtserror.NewfAt(3, "unexpected column headers in csv: %+v", columnHeaders)
+				err := gtserror.NewfAt(3, "duplicate domain column header in csv: %+v", columnHeaders)
+				return nil, err
+			}
+			domainI = &i
+
+		case "severity":
+			if severityI != nil {
+				body.Close()
+				err := gtserror.NewfAt(3, "duplicate severity column header in csv: %+v", columnHeaders)
+				return nil, err
+			}
+			severityI = &i
+
+		case "public_comment":
+			if publicCommentI != nil {
+				body.Close()
+				err := gtserror.NewfAt(3, "duplicate public_comment column header in csv: %+v", columnHeaders)
+				return nil, err
+			}
+			publicCommentI = &i
+
+		case "obfuscate":
+			if obfuscateI != nil {
+				body.Close()
+				err := gtserror.NewfAt(3, "duplicate obfuscate column header in csv: %+v", columnHeaders)
+				return nil, err
+			}
+			obfuscateI = &i
+		}
+	}
+
+	// Ensure we have at least a domain
+	// index, as that's the bare minimum.
+	if domainI == nil {
+		body.Close()
+		err := gtserror.NewfAt(3, "no domain column header in csv: %+v", columnHeaders)
 		return nil, err
 	}
 
@@ -576,25 +616,19 @@ func permsFromCSV(
 			continue
 		}
 
-		var (
-			domainRaw     = record[0]
-			severity      = record[1]
-			publicComment = record[4]
-			obfuscateStr  = record[5]
-		)
-
+		// Skip records that specify severity
+		// that's not "suspend" (we don't support
+		// "silence" or "limit" or whatever yet).
+		if severityI != nil {
+			severity := record[*severityI]
 			if severity != "suspend" {
 				l.Warnf("skipping non-suspend record: %+v", record)
 				continue
 			}
-
-		obfuscate, err := strconv.ParseBool(obfuscateStr)
-		if err != nil {
-			l.Warnf("couldn't parse obfuscate field of record: %+v", record)
-			continue
 		}
 
 		// Normalize + validate domain.
+		domainRaw := record[*domainI]
 		domain, err := validateDomain(domainRaw)
 		if err != nil {
 			l.Warnf("skipping invalid domain %s: %+v", domainRaw, err)
@@ -611,9 +645,21 @@ func permsFromCSV(
 			perm = &gtsmodel.DomainAllow{Domain: domain}
 		}
 
-		// Set remaining fields.
-		perm.SetPublicComment(publicComment)
+		// Set remaining optional fields
+		// if they're present in the CSV.
+		if publicCommentI != nil {
+			perm.SetPublicComment(record[*publicCommentI])
+		}
+
+		if obfuscateI != nil {
+			obfuscate, err := strconv.ParseBool(record[*obfuscateI])
+			if err != nil {
+				l.Warnf("couldn't parse obfuscate field of record: %+v", record)
+				continue
+			}
+
 			perm.SetObfuscate(&obfuscate)
+		}
 
 		// We're done.
 		perms = append(perms, perm)

internal/subscriptions/subscriptions_test.go

@@ -472,7 +472,7 @@ func (suite *SubscriptionsTestSuite) TestDomainBlocksWrongContentTypeCSV() {
 	suite.Zero(count)
 	suite.WithinDuration(time.Now(), permSub.FetchedAt, 1*time.Minute)
 	suite.Zero(permSub.SuccessfullyFetchedAt)
-	suite.Equal(`ProcessDomainPermissionSubscription: unexpected column headers in csv: [bumfaces.net]`, permSub.Error)
+	suite.Equal(`ProcessDomainPermissionSubscription: no domain column header in csv: [bumfaces.net]`, permSub.Error)
 }
 
 func (suite *SubscriptionsTestSuite) TestDomainBlocksWrongContentTypePlain() {

internal/typeutils/internaltofrontend.go

@@ -2115,7 +2115,7 @@ func (c *Converter) DomainPermToAPIDomainPerm(
 	}
 
 	domainPerm.ID = d.GetID()
-	domainPerm.Obfuscate = *d.GetObfuscate()
+	domainPerm.Obfuscate = util.PtrOrZero(d.GetObfuscate())
 	domainPerm.PrivateComment = d.GetPrivateComment()
 	domainPerm.SubscriptionID = d.GetSubscriptionID()
 	domainPerm.CreatedBy = d.GetCreatedByAccountID()