From a0d5cb5096652c99f746bb9207bbf0f4220cb79d Mon Sep 17 00:00:00 2001
From: Lucas Di Pentima <lucas@di-pentima.com.ar>
Date: Wed, 20 May 2020 18:23:08 -0300
Subject: [PATCH] 16435: Allows 2 or 3 fields per record on the CSV file.

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas@di-pentima.com.ar>
---
 tools/sync-groups/sync-groups.go      | 15 +++++++++++----
 tools/sync-groups/sync-groups_test.go | 16 ++++++++++++++++
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/tools/sync-groups/sync-groups.go b/tools/sync-groups/sync-groups.go
index 7c5cd0558b..b77d8ccddd 100644
--- a/tools/sync-groups/sync-groups.go
+++ b/tools/sync-groups/sync-groups.go
@@ -134,9 +134,10 @@ func ParseFlags(config *ConfigParams) error {
 
 	// Set up usage message
 	flags.Usage = func() {
-		usageStr := `Synchronize remote groups into Arvados from a CSV format file with 2 columns:
-  * 1st column: Group name
-  * 2nd column: User identifier`
+		usageStr := `Synchronize remote groups into Arvados from a CSV format file with 3 columns:
+  * 1st: Group name
+  * 2nd: User identifier
+  * 3rd (Optional): User permission on the group: read, write or manage. (Default: write)`
 		fmt.Fprintf(os.Stderr, "%s\n\n", usageStr)
 		fmt.Fprintf(os.Stderr, "Usage:\n%s [OPTIONS] <input-file.csv>\n\n", os.Args[0])
 		fmt.Fprintf(os.Stderr, "Options:\n")
@@ -362,7 +363,8 @@ func ProcessFile(
 ) (groupsCreated, membersAdded, membersSkipped int, err error) {
 	lineNo := 0
 	csvReader := csv.NewReader(f)
-	csvReader.FieldsPerRecord = 2
+	// Allow variable number of fields.
+	csvReader.FieldsPerRecord = -1
 	for {
 		record, e := csvReader.Read()
 		if e == io.EOF {
@@ -373,6 +375,11 @@ func ProcessFile(
 			err = fmt.Errorf("error parsing %q, line %d", cfg.Path, lineNo)
 			return
 		}
+		// Only allow 2 or 3 fields per record for backwards compatibility.
+		if len(record) < 2 || len(record) > 3 {
+			err = fmt.Errorf("error parsing %q, line %d: found %d fields but only 2 or 3 are allowed", cfg.Path, lineNo, len(record))
+			return
+		}
 		groupName := strings.TrimSpace(record[0])
 		groupMember := strings.TrimSpace(record[1]) // User ID (username or email)
 		if groupName == "" || groupMember == "" {
diff --git a/tools/sync-groups/sync-groups_test.go b/tools/sync-groups/sync-groups_test.go
index 3ef3600797..77d9756ffc 100644
--- a/tools/sync-groups/sync-groups_test.go
+++ b/tools/sync-groups/sync-groups_test.go
@@ -263,6 +263,22 @@ func (s *TestSuite) TestIgnoreSpaces(c *C) {
 	}
 }
 
+// Error out when records have <2 or >3 records
+func (s *TestSuite) TestWrongNumberOfFields(c *C) {
+	for _, testCase := range [][][]string{
+		{{"field1"}},
+		{{"field1", "field2", "field3", "field4"}},
+		{{"field1", "field2", "field3", "field4", "field5"}},
+	} {
+		tmpfile, err := MakeTempCSVFile(testCase)
+		c.Assert(err, IsNil)
+		defer os.Remove(tmpfile.Name())
+		s.cfg.Path = tmpfile.Name()
+		err = doMain(s.cfg)
+		c.Assert(err, Not(IsNil))
+	}
+}
+
 // The absence of a user membership on the CSV file implies its removal
 func (s *TestSuite) TestMembershipRemoval(c *C) {
 	localUserEmail := s.users[arvadostest.ActiveUserUUID].Email
-- 
2.30.2