Merge branch '18097-sync-groups-case-insensitive' into main. Closes #18097

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

diff --git a/tools/sync-groups/sync-groups.go b/tools/sync-groups/sync-groups.go
index 24e838c8f1ec64434a13652b36b18689ddb5a216..f0c377078358cde9981e45afa66a989171c0a27c 100644 (file)
--- a/tools/sync-groups/sync-groups.go
+++ b/tools/sync-groups/sync-groups.go
@@ -119,6 +119,7 @@ type ConfigParams struct {
        Path            string
        UserID          string
        Verbose         bool
+       CaseInsensitive bool
        ParentGroupUUID string
        ParentGroupName string
        SysUserUUID     string
@@ -152,6 +153,10 @@ func ParseFlags(config *ConfigParams) error {
                "user-id",
                "email",
                "Attribute by which every user is identified. Valid values are: email and username.")
+       caseInsensitive := flags.Bool(
+               "case-insensitive",
+               false,
+               "Performs case insensitive matching on user IDs. Off by default.")
        verbose := flags.Bool(
                "verbose",
                false,
@@ -196,6 +201,7 @@ func ParseFlags(config *ConfigParams) error {
        config.ParentGroupUUID = *parentGroupUUID
        config.UserID = *userID
        config.Verbose = *verbose
+       config.CaseInsensitive = *caseInsensitive
 
        return nil
 }
@@ -299,7 +305,11 @@ func doMain(cfg *ConfigParams) error {
        }
        defer f.Close()
 
-       log.Printf("%s %s started. Using %q as users id and parent group UUID %q", os.Args[0], version, cfg.UserID, cfg.ParentGroupUUID)
+       iCaseLog := ""
+       if cfg.UserID == "username" && cfg.CaseInsensitive {
+               iCaseLog = " - username matching requested to be case-insensitive"
+       }
+       log.Printf("%s %s started. Using %q as users id and parent group UUID %q%s", os.Args[0], version, cfg.UserID, cfg.ParentGroupUUID, iCaseLog)
 
        // Get the complete user list to minimize API Server requests
        allUsers := make(map[string]arvados.User)
@@ -316,6 +326,12 @@ func doMain(cfg *ConfigParams) error {
                if err != nil {
                        return err
                }
+               if cfg.UserID == "username" && uID != "" && cfg.CaseInsensitive {
+                       uID = strings.ToLower(uID)
+                       if uuid, found := userIDToUUID[uID]; found {
+                               return fmt.Errorf("case insensitive collision for username %q between %q and %q", uID, u.UUID, uuid)
+                       }
+               }
                userIDToUUID[uID] = u.UUID
                if cfg.Verbose {
                        log.Printf("Seen user %q (%s)", u.Username, u.UUID)
@@ -415,6 +431,9 @@ func ProcessFile(
                        membersSkipped++
                        continue
                }
+               if cfg.UserID == "username" && cfg.CaseInsensitive {
+                       groupMember = strings.ToLower(groupMember)
+               }
                if !(groupPermission == "can_read" || groupPermission == "can_write" || groupPermission == "can_manage") {
                        log.Printf("Warning: 3rd field should be 'can_read', 'can_write' or 'can_manage'. Found: %q at line %d, skipping.", groupPermission, lineNo)
                        membersSkipped++
@@ -494,9 +513,7 @@ func GetAll(c *arvados.Client, res string, params arvados.ResourceListParams, pa
                if page.Len() == 0 {
                        break
                }
-               for _, i := range page.GetItems() {
-                       allItems = append(allItems, i)
-               }
+               allItems = append(allItems, page.GetItems()...)
                params.Offset += page.Len()
        }
        return allItems, nil
@@ -634,6 +651,9 @@ func GetRemoteGroups(cfg *ConfigParams, allUsers map[string]arvados.User) (remot
                        if err != nil {
                                return remoteGroups, groupNameToUUID, err
                        }
+                       if cfg.UserID == "username" && cfg.CaseInsensitive {
+                               memberID = strings.ToLower(memberID)
+                       }
                        membersSet[memberID] = u2gLinkSet[link.HeadUUID]
                }
                remoteGroups[group.UUID] = &GroupInfo{
@@ -714,9 +734,7 @@ func RemoveMemberLinksFromGroup(cfg *ConfigParams, user arvados.User, linkNames
                        userID, _ := GetUserID(user, cfg.UserID)
                        return fmt.Errorf("error getting links needed to remove user %q from group %q: %s", userID, group.Name, err)
                }
-               for _, link := range l {
-                       links = append(links, link)
-               }
+               links = append(links, l...)
        }
        for _, item := range links {
                link := item.(arvados.Link)

diff --git a/tools/sync-groups/sync-groups_test.go b/tools/sync-groups/sync-groups_test.go
index ec2f18a307d70c9767efcdef96574aa18d2cc862..69326c98d958cacd7709d24c47b9c63abd690b78 100644 (file)
--- a/tools/sync-groups/sync-groups_test.go
+++ b/tools/sync-groups/sync-groups_test.go
@@ -50,6 +50,7 @@ func (s *TestSuite) SetUpTest(c *C) {
        os.Args = []string{"cmd", "somefile.csv"}
        config, err := GetConfig()
        c.Assert(err, IsNil)
+       config.UserID = "email"
        // Confirm that the parent group was created
        gl = arvados.GroupList{}
        ac.RequestAndDecode(&gl, "GET", "/arvados/v1/groups", nil, params)
@@ -145,10 +146,7 @@ func GroupMembershipExists(ac *arvados.Client, userUUID string, groupUUID string
                }},
        }
        ac.RequestAndDecode(&ll, "GET", "/arvados/v1/links", nil, params)
-       if ll.Len() != 1 {
-               return false
-       }
-       return true
+       return ll.Len() == 1
 }
 
 // If named group exists, return its UUID
@@ -189,11 +187,12 @@ func RemoteGroupExists(cfg *ConfigParams, groupName string) (uuid string, err er
 
 func (s *TestSuite) TestParseFlagsWithPositionalArgument(c *C) {
        cfg := ConfigParams{}
-       os.Args = []string{"cmd", "-verbose", "/tmp/somefile.csv"}
+       os.Args = []string{"cmd", "-verbose", "-case-insensitive", "/tmp/somefile.csv"}
        err := ParseFlags(&cfg)
        c.Assert(err, IsNil)
        c.Check(cfg.Path, Equals, "/tmp/somefile.csv")
        c.Check(cfg.Verbose, Equals, true)
+       c.Check(cfg.CaseInsensitive, Equals, true)
 }
 
 func (s *TestSuite) TestParseFlagsWithoutPositionalArgument(c *C) {
@@ -450,7 +449,7 @@ func (s *TestSuite) TestIgnoreNonexistantUsers(c *C) {
        c.Assert(GroupMembershipExists(s.cfg.Client, activeUserUUID, groupUUID, "can_write"), Equals, true)
 }
 
-// Users listed on the file that don't exist on the system are ignored
+// Entries with missing data are ignored.
 func (s *TestSuite) TestIgnoreEmptyFields(c *C) {
        activeUserEmail := s.users[arvadostest.ActiveUserUUID].Email
        activeUserUUID := s.users[arvadostest.ActiveUserUUID].UUID
@@ -502,7 +501,6 @@ func (s *TestSuite) TestUseUsernames(c *C) {
        s.cfg.Path = tmpfile.Name()
        s.cfg.UserID = "username"
        err = doMain(s.cfg)
-       s.cfg.UserID = "email"
        c.Assert(err, IsNil)
        // Confirm that memberships exist
        groupUUID, err = RemoteGroupExists(s.cfg, "TestGroup1")
@@ -510,3 +508,65 @@ func (s *TestSuite) TestUseUsernames(c *C) {
        c.Assert(groupUUID, Not(Equals), "")
        c.Assert(GroupMembershipExists(s.cfg.Client, activeUserUUID, groupUUID, "can_write"), Equals, true)
 }
+
+func (s *TestSuite) TestUseUsernamesWithCaseInsensitiveMatching(c *C) {
+       activeUserName := strings.ToUpper(s.users[arvadostest.ActiveUserUUID].Username)
+       activeUserUUID := s.users[arvadostest.ActiveUserUUID].UUID
+       // Confirm that group doesn't exist
+       groupUUID, err := RemoteGroupExists(s.cfg, "TestGroup1")
+       c.Assert(err, IsNil)
+       c.Assert(groupUUID, Equals, "")
+       // Create file & run command
+       data := [][]string{
+               {"TestGroup1", activeUserName},
+       }
+       tmpfile, err := MakeTempCSVFile(data)
+       c.Assert(err, IsNil)
+       defer os.Remove(tmpfile.Name()) // clean up
+       s.cfg.Path = tmpfile.Name()
+       s.cfg.UserID = "username"
+       s.cfg.CaseInsensitive = true
+       err = doMain(s.cfg)
+       c.Assert(err, IsNil)
+       // Confirm that memberships exist
+       groupUUID, err = RemoteGroupExists(s.cfg, "TestGroup1")
+       c.Assert(err, IsNil)
+       c.Assert(groupUUID, Not(Equals), "")
+       c.Assert(GroupMembershipExists(s.cfg.Client, activeUserUUID, groupUUID, "can_write"), Equals, true)
+}
+
+func (s *TestSuite) TestUsernamesCaseInsensitiveCollision(c *C) {
+       activeUserName := s.users[arvadostest.ActiveUserUUID].Username
+       activeUserUUID := s.users[arvadostest.ActiveUserUUID].UUID
+
+       nu := arvados.User{}
+       nuUsername := strings.ToUpper(activeUserName)
+       err := s.cfg.Client.RequestAndDecode(&nu, "POST", "/arvados/v1/users", nil, map[string]interface{}{
+               "user": map[string]string{
+                       "username": nuUsername,
+               },
+       })
+       c.Assert(err, IsNil)
+
+       // Manually remove non-fixture user because /database/reset fails otherwise
+       defer s.cfg.Client.RequestAndDecode(nil, "DELETE", "/arvados/v1/users/"+nu.UUID, nil, nil)
+
+       c.Assert(nu.Username, Equals, nuUsername)
+       c.Assert(nu.UUID, Not(Equals), activeUserUUID)
+       c.Assert(nu.Username, Not(Equals), activeUserName)
+
+       data := [][]string{
+               {"SomeGroup", activeUserName},
+       }
+       tmpfile, err := MakeTempCSVFile(data)
+       c.Assert(err, IsNil)
+       defer os.Remove(tmpfile.Name()) // clean up
+
+       s.cfg.Path = tmpfile.Name()
+       s.cfg.UserID = "username"
+       s.cfg.CaseInsensitive = true
+       err = doMain(s.cfg)
+       // Should get an error because of "ACTIVE" and "Active" usernames
+       c.Assert(err, NotNil)
+       c.Assert(err, ErrorMatches, ".*case insensitive collision.*")
+}