Merge branch '18858-sync-users-fixes'. Closes #18858

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima@curii.com>

diff --git a/services/api/test/fixtures/users.yml b/services/api/test/fixtures/users.yml
index 1d9bcbb040ab7c5dd955361704da7b9a81a3147b..56f44551e1992b5583a6e3cce990509ed5575b5d 100644 (file)
--- a/services/api/test/fixtures/users.yml
+++ b/services/api/test/fixtures/users.yml
@@ -12,7 +12,6 @@ system_user:
   modified_by_user_uuid: zzzzz-tpzed-000000000000000
   modified_at: 2014-11-27 06:38:21.208036000 Z
   email: root
-  username: root
   first_name: root
   last_name: ''
   identity_url:
@@ -233,7 +232,6 @@ anonymous:
   last_name: anonymouspublic
   is_active: false
   is_admin: false
-  username: anonymous
   prefs: {}
 
 job_reader:

diff --git a/tools/sync-groups/sync-groups.go b/tools/sync-groups/sync-groups.go
index e1e054a663cad7d6613899ce87366ef79c4e1242..daec5c99578fd7e3de0fb73980afa3c027c53344 100644 (file)
--- a/tools/sync-groups/sync-groups.go
+++ b/tools/sync-groups/sync-groups.go
@@ -181,8 +181,10 @@ func ParseFlags(config *ConfigParams) error {
        // Input file as a required positional argument
        if flags.NArg() == 0 {
                return fmt.Errorf("please provide a path to an input file")
+       } else if flags.NArg() > 1 {
+               return fmt.Errorf("please provide just one input file argument")
        }
-       srcPath := &os.Args[flags.NFlag()+1]
+       srcPath := &os.Args[len(os.Args)-1]
 
        // Validations
        if *srcPath == "" {

diff --git a/tools/sync-users/sync-users.go b/tools/sync-users/sync-users.go
index 37b94a9f44b636fcd80ce9632ba271c519d4e2b3..626d9d04221721bc0f56d2a8503b29b25fc3f0b2 100644 (file)
--- a/tools/sync-users/sync-users.go
+++ b/tools/sync-users/sync-users.go
@@ -123,8 +123,10 @@ func ParseFlags(cfg *ConfigParams) error {
        // Input file as a required positional argument
        if flags.NArg() == 0 {
                return fmt.Errorf("please provide a path to an input file")
+       } else if flags.NArg() > 1 {
+               return fmt.Errorf("please provide just one input file argument")
        }
-       srcPath := &os.Args[flags.NFlag()+1]
+       srcPath := &os.Args[len(os.Args)-1]
 
        // Validations
        if *srcPath == "" {
@@ -222,6 +224,7 @@ func doMain(cfg *ConfigParams) error {
        allUsers := make(map[string]arvados.User)
        userIDToUUID := make(map[string]string) // Index by email or username
        dupedEmails := make(map[string][]arvados.User)
+       emptyUserIDs := []string{}
        processedUsers := make(map[string]bool)
        results, err := GetAll(cfg.Client, "users", arvados.ResourceListParams{}, &UserList{})
        if err != nil {
@@ -246,7 +249,11 @@ func doMain(cfg *ConfigParams) error {
                        return err
                }
                if uID == "" {
-                       return fmt.Errorf("%s is empty for user with uuid %q", cfg.UserID, u.UUID)
+                       if u.UUID != cfg.AnonUserUUID && u.UUID != cfg.SysUserUUID {
+                               emptyUserIDs = append(emptyUserIDs, u.UUID)
+                               log.Printf("Empty %s found in user %s - ignoring", cfg.UserID, u.UUID)
+                       }
+                       continue
                }
                if cfg.CaseInsensitive {
                        uID = strings.ToLower(uID)
@@ -327,7 +334,7 @@ func doMain(cfg *ConfigParams) error {
 
        log.Printf("User update successes: %d, skips: %d, failures: %d", len(updatesSucceeded), len(updatesSkipped), len(updatesFailed))
 
-       // Report duplicated emails detection
+       var errors []string
        if len(dupedEmails) > 0 {
                emails := make([]string, len(dupedEmails))
                i := 0
@@ -335,7 +342,13 @@ func doMain(cfg *ConfigParams) error {
                        emails[i] = e
                        i++
                }
-               return fmt.Errorf("skipped %d duplicated email address(es) in the cluster's local user list: %v", len(dupedEmails), emails)
+               errors = append(errors, fmt.Sprintf("skipped %d duplicated email address(es) in the cluster's local user list: %v", len(dupedEmails), emails))
+       }
+       if len(emptyUserIDs) > 0 {
+               errors = append(errors, fmt.Sprintf("skipped %d user account(s) with empty %s: %v", len(emptyUserIDs), cfg.UserID, emptyUserIDs))
+       }
+       if len(errors) > 0 {
+               return fmt.Errorf("%s", strings.Join(errors, "\n"))
        }
 
        return nil
@@ -361,7 +374,7 @@ type userRecord struct {
 
 func needsUpdating(user arvados.User, record userRecord) bool {
        userData := userRecord{"", user.FirstName, user.LastName, user.IsActive, user.IsAdmin}
-       recordData := userRecord{"", record.FirstName, record.LastName, record.Active, record.Admin}
+       recordData := userRecord{"", record.FirstName, record.LastName, record.Active, record.Active && record.Admin}
        return userData != recordData
 }
 
@@ -433,7 +446,6 @@ func ProcessRecord(cfg *ConfigParams, record userRecord, userIDToUUID map[string
                        }
                }
        }
-       allUsers[record.UserID] = user
        if createRequired {
                log.Printf("Created user %q", record.UserID)
        }

diff --git a/tools/sync-users/sync-users_test.go b/tools/sync-users/sync-users_test.go
index 8b5385a32110786c0125896f5507e07a0309b8f5..119564d4df438ea6bee7ca3c0dde8f743250db52 100644 (file)
--- a/tools/sync-users/sync-users_test.go
+++ b/tools/sync-users/sync-users_test.go
@@ -102,7 +102,7 @@ func (s *TestSuite) TestParseFlagsWithoutPositionalArgument(c *C) {
 }
 
 func (s *TestSuite) TestParseFlagsWrongUserID(c *C) {
-       os.Args = []string{"cmd", "-user-id=nickname", "/tmp/somefile.csv"}
+       os.Args = []string{"cmd", "-user-id", "nickname", "/tmp/somefile.csv"}
        err := ParseFlags(&ConfigParams{})
        c.Assert(err, NotNil)
        c.Assert(err, ErrorMatches, ".*user ID must be one of:.*")
@@ -122,7 +122,7 @@ func (s *TestSuite) TestParseFlagsWithPositionalArgument(c *C) {
 
 func (s *TestSuite) TestParseFlagsWithOptionalFlags(c *C) {
        cfg := ConfigParams{}
-       os.Args = []string{"cmd", "-verbose", "-deactivate-unlisted", "-user-id=username", "/tmp/somefile.csv"}
+       os.Args = []string{"cmd", "-verbose", "-deactivate-unlisted", "-user-id", "username", "/tmp/somefile.csv"}
        err := ParseFlags(&cfg)
        c.Assert(err, IsNil)
        c.Assert(cfg.Path, Equals, "/tmp/somefile.csv")
@@ -349,6 +349,24 @@ func (s *TestSuite) TestUserCreationAndUpdate(c *C) {
 
 func (s *TestSuite) TestDeactivateUnlisted(c *C) {
        localUserUuidRegex := regexp.MustCompile(fmt.Sprintf("^%s-tpzed-[0-9a-z]{15}$", s.cfg.ClusterID))
+
+       var user1 arvados.User
+       for _, nr := range []int{1, 2} {
+               var newUser arvados.User
+               err := CreateUser(s.cfg.Client, &newUser, map[string]string{
+                       "email":      fmt.Sprintf("user%d@example.com", nr),
+                       "first_name": "Example",
+                       "last_name":  fmt.Sprintf("User%d", nr),
+                       "is_active":  "true",
+                       "is_admin":   "false",
+               })
+               c.Assert(err, IsNil)
+               c.Assert(newUser.IsActive, Equals, true)
+               if nr == 1 {
+                       user1 = newUser // for later confirmation
+               }
+       }
+
        users, err := ListUsers(s.cfg.Client)
        c.Assert(err, IsNil)
        previouslyActiveUsers := 0
@@ -364,19 +382,21 @@ func (s *TestSuite) TestDeactivateUnlisted(c *C) {
                        previouslyActiveUsers++
                }
        }
-       // At least 3 active users: System root, Anonymous and the current user.
-       // Other active users should exist from fixture.
+       // Active users: System root, Anonymous, current user and the
+       // ones just created (other active users may exist from fixture).
        c.Logf("Initial active users count: %d", previouslyActiveUsers)
-       c.Assert(previouslyActiveUsers > 3, Equals, true)
+       c.Assert(previouslyActiveUsers > 5, Equals, true)
 
-       s.cfg.DeactivateUnlisted = true
-       s.cfg.Verbose = true
+       // Here we omit user2@example.com from the CSV file.
        data := [][]string{
-               {"user1@example.com", "Example", "User1", "0", "0"},
+               {"user1@example.com", "Example", "User1", "1", "0"},
        }
        tmpfile, err := MakeTempCSVFile(data)
        c.Assert(err, IsNil)
        defer os.Remove(tmpfile.Name())
+
+       s.cfg.DeactivateUnlisted = true
+       s.cfg.Verbose = true
        s.cfg.Path = tmpfile.Name()
        err = doMain(s.cfg)
        c.Assert(err, IsNil)
@@ -388,6 +408,7 @@ func (s *TestSuite) TestDeactivateUnlisted(c *C) {
                fmt.Sprintf("%s-tpzed-000000000000000", s.cfg.ClusterID): true,
                fmt.Sprintf("%s-tpzed-anonymouspublic", s.cfg.ClusterID): true,
                s.cfg.CurrentUser.UUID: true,
+               user1.UUID:             true,
        }
        remainingActiveUUIDs := map[string]bool{}
        seenUserEmails := map[string]bool{}
@@ -404,9 +425,10 @@ func (s *TestSuite) TestDeactivateUnlisted(c *C) {
                        currentlyActiveUsers++
                }
        }
-       // 3 active users remaining: System root, Anonymous and the current user.
+       // 4 active users remaining: System root, Anonymous, the current user
+       // and user1@example.com
        c.Logf("Active local users remaining: %v", remainingActiveUUIDs)
-       c.Assert(currentlyActiveUsers, Equals, 3)
+       c.Assert(currentlyActiveUsers, Equals, 4)
 }
 
 func (s *TestSuite) TestFailOnDuplicatedEmails(c *C) {
@@ -434,3 +456,106 @@ func (s *TestSuite) TestFailOnDuplicatedEmails(c *C) {
        c.Assert(err, NotNil)
        c.Assert(err, ErrorMatches, "skipped.*duplicated email address.*")
 }
+
+func (s *TestSuite) TestFailOnEmptyUsernames(c *C) {
+       for i := range []int{1, 2} {
+               var user arvados.User
+               err := CreateUser(s.cfg.Client, &user, map[string]string{
+                       "email":      fmt.Sprintf("johndoe%d@example.com", i),
+                       "username":   "",
+                       "first_name": "John",
+                       "last_name":  "Doe",
+                       "is_active":  "true",
+                       "is_admin":   "false",
+               })
+               c.Assert(err, IsNil)
+               c.Assert(user.Username, Equals, fmt.Sprintf("johndoe%d", i))
+               if i == 1 {
+                       err = UpdateUser(s.cfg.Client, user.UUID, &user, map[string]string{
+                               "username": "",
+                       })
+                       c.Assert(err, IsNil)
+                       c.Assert(user.Username, Equals, "")
+               }
+       }
+
+       s.cfg.Verbose = true
+       data := [][]string{
+               {"johndoe0", "John", "Doe", "0", "0"},
+       }
+       tmpfile, err := MakeTempCSVFile(data)
+       c.Assert(err, IsNil)
+       defer os.Remove(tmpfile.Name())
+       s.cfg.Path = tmpfile.Name()
+       s.cfg.UserID = "username"
+       err = doMain(s.cfg)
+       c.Assert(err, NotNil)
+       c.Assert(err, ErrorMatches, "skipped 1 user account.*with empty username.*")
+}
+
+func (s *TestSuite) TestFailOnDupedUsernameAndCaseInsensitiveMatching(c *C) {
+       for _, i := range []int{1, 2} {
+               var user arvados.User
+               emailPrefix := "john"
+               if i == 1 {
+                       emailPrefix = "JOHN"
+               }
+               err := CreateUser(s.cfg.Client, &user, map[string]string{
+                       "email":      fmt.Sprintf("%sdoe@example.com", emailPrefix),
+                       "username":   "",
+                       "first_name": "John",
+                       "last_name":  "Doe",
+                       "is_active":  "true",
+                       "is_admin":   "false",
+               })
+               c.Assert(err, IsNil)
+               c.Assert(user.Username, Equals, fmt.Sprintf("%sdoe", emailPrefix))
+       }
+
+       s.cfg.Verbose = true
+       data := [][]string{
+               {"johndoe", "John", "Doe", "0", "0"},
+       }
+       tmpfile, err := MakeTempCSVFile(data)
+       c.Assert(err, IsNil)
+       defer os.Remove(tmpfile.Name())
+       s.cfg.Path = tmpfile.Name()
+       s.cfg.UserID = "username"
+       s.cfg.CaseInsensitive = true
+       err = doMain(s.cfg)
+       c.Assert(err, NotNil)
+       c.Assert(err, ErrorMatches, "case insensitive collision for username.*between.*and.*")
+}
+
+func (s *TestSuite) TestSuccessOnUsernameAndCaseSensitiveMatching(c *C) {
+       for _, i := range []int{1, 2} {
+               var user arvados.User
+               emailPrefix := "john"
+               if i == 1 {
+                       emailPrefix = "JOHN"
+               }
+               err := CreateUser(s.cfg.Client, &user, map[string]string{
+                       "email":      fmt.Sprintf("%sdoe@example.com", emailPrefix),
+                       "username":   "",
+                       "first_name": "John",
+                       "last_name":  "Doe",
+                       "is_active":  "true",
+                       "is_admin":   "false",
+               })
+               c.Assert(err, IsNil)
+               c.Assert(user.Username, Equals, fmt.Sprintf("%sdoe", emailPrefix))
+       }
+
+       s.cfg.Verbose = true
+       data := [][]string{
+               {"johndoe", "John", "Doe", "0", "0"},
+       }
+       tmpfile, err := MakeTempCSVFile(data)
+       c.Assert(err, IsNil)
+       defer os.Remove(tmpfile.Name())
+       s.cfg.Path = tmpfile.Name()
+       s.cfg.UserID = "username"
+       s.cfg.CaseInsensitive = false
+       err = doMain(s.cfg)
+       c.Assert(err, IsNil)
+}