fix: getUIDandGID is able to resolve non-existing users and groups

Makefile

@@ -66,6 +66,10 @@ minikube-setup:
 test: out/executor
 	@ ./scripts/test.sh
 
+test-with-coverage: test
+	go tool cover -html=out/coverage.out
+
+
 .PHONY: integration-test
 integration-test:
 	@ ./scripts/integration-test.sh

pkg/util/command_util.go

@@ -38,8 +38,7 @@ import (
 
 // for testing
 var (
-	getIdsFromUsernameAndGroup = getIdsFromUsernameAndGroupImpl
-	getGroupFromStr            = getGroup
+	GetUIDAndGID = getUIDAndGID
 )
 
 const (
@@ -353,7 +352,7 @@ func GetUserGroup(chownStr string, env []string) (int64, int64, error) {
 		return -1, -1, err
 	}
 
-	uid32, gid32, err := GetUIDAndGIDFromString(chown, true)
+	uid32, gid32, err := getUIDAndGIDFromString(chown, true)
 	if err != nil {
 		return -1, -1, err
 	}
@@ -365,94 +364,82 @@ func GetUserGroup(chownStr string, env []string) (int64, int64, error) {
 // If fallbackToUID is set, the gid is equal to uid if the group is not specified
 // otherwise gid is set to zero.
 // UserID and GroupID don't need to be present on the system.
-func GetUIDAndGIDFromString(userGroupString string, fallbackToUID bool) (uint32, uint32, error) {
+func getUIDAndGIDFromString(userGroupString string, fallbackToUID bool) (uint32, uint32, error) {
 	userAndGroup := strings.Split(userGroupString, ":")
 	userStr := userAndGroup[0]
 	var groupStr string
 	if len(userAndGroup) > 1 {
 		groupStr = userAndGroup[1]
 	}
-
-	// checkif userStr is a valid id
-	uid, err := strconv.ParseUint(userStr, 10, 32)
-	if err != nil {
-		return getIdsFromUsernameAndGroup(userStr, groupStr, fallbackToUID)
-	}
-
-	gid, err := strconv.ParseUint(groupStr, 10, 32)
-	if err != nil {
-		// check if the group is specified as a string
-		group, err := getGroupFromStr(groupStr)
-		if err != nil && !fallbackToUID {
-			return 0, 0, err
-		}
-		gid, err := strconv.ParseUint(group.Gid, 10, 32)
-		if err != nil && !fallbackToUID {
-			// no fallback and group is not a valid uid (this should never be the case, because then the whole group is malformed)
-			return 0, 0, err
-		} else if err != nil && fallbackToUID {
-			return uint32(uid), uint32(uid), nil
-		}
-		return uint32(uid), uint32(gid), nil
-	}
-
-	return uint32(uid), uint32(gid), nil
+	return GetUIDAndGID(userStr, groupStr, fallbackToUID)
 }
 
-func getIdsFromUsernameAndGroupImpl(userStr string, groupStr string, fallbackToUID bool) (uint32, uint32, error) {
-	// Lookup by username
-	userObj, err := LookupUser(userStr)
+func getUIDAndGID(userStr string, groupStr string, fallbackToUID bool) (uint32, uint32, error) {
+	uid32, err := LookupUID(userStr)
 	if err != nil {
 		return 0, 0, err
 	}
 
-	// Same dance with groups
-	var group *user.Group
-	if groupStr != "" {
-		group, err = getGroupFromStr(groupStr)
-		if err != nil {
-			return 0, 0, err
+	gid, err := getGIDFromName(groupStr, fallbackToUID)
+	if err != nil {
+		if errors.Is(err, fallbackToUIDError) {
+			return uid32, uid32, nil
 		}
+		return 0, 0, err
 	}
+	return uid32, uint32(gid), nil
+}
 
-	uid := userObj.Uid
-	gid := "0"
-	if fallbackToUID {
-		gid = userObj.Gid
-	}
-	if group != nil {
-		gid = group.Gid
-	}
+func getUID()
 
-	uid64, err := strconv.ParseUint(uid, 10, 32)
-	if err != nil {
-		return 0, 0, err
-	}
-	gid64, err := strconv.ParseUint(gid, 10, 32)
+// getGID tries to parse the gid or falls back to getGroupFromName if it's not an id
+func getGID(groupStr string, fallbackToUID bool) (uint32, error) {
+	gid, err := strconv.ParseUint(groupStr, 10, 32)
 	if err != nil {
-		return 0, 0, err
+		return 0, fallbackToUIDOrError(err, fallbackToUID)
 	}
-	return uint32(uid64), uint32(gid64), nil
+	return uint32(gid), nil
 }
 
-func getGroup(groupStr string) (*user.Group, error) {
+// getGIDFromName tries to parse the groupStr into an existing group.
+// if the group doesn't exist, fallback to getGID to parse non-existing valid GIDs.
+func getGIDFromName(groupStr string, fallbackToUID bool) (uint32, error) {
 	group, err := user.LookupGroup(groupStr)
 	if err != nil {
-		if _, ok := err.(user.UnknownGroupError); !ok {
-			return nil, err
+		// unknown group error could relate to a non existing group
+		var groupErr *user.UnknownGroupError
+		if errors.Is(err, groupErr) {
+			return getGID(groupStr, fallbackToUID)
 		}
 		group, err = user.LookupGroupId(groupStr)
 		if err != nil {
-			return nil, err
+			return getGID(groupStr, fallbackToUID)
 		}
 	}
-	return group, nil
+	return getGID(group.Gid, fallbackToUID)
+}
+
+var fallbackToUIDError = new(fallbackToUIDErrorType)
+
+type fallbackToUIDErrorType struct{}
+
+func (e fallbackToUIDErrorType) Error() string {
+	return "fallback to uid"
+}
+
+func fallbackToUIDOrError(err error, fallbackToUID bool) error {
+	if fallbackToUID {
+		return fallbackToUIDError
+	} else {
+		return err
+	}
 }
 
 func LookupUser(userStr string) (*user.User, error) {
 	userObj, err := user.Lookup(userStr)
 	if err != nil {
 		if _, ok := err.(user.UnknownUserError); !ok {
+			// try parsing the UID
 			return nil, err
 		}
 
@@ -467,3 +454,21 @@ func LookupUser(userStr string) (*user.User, error) {
 
 	return userObj, nil
 }
+
+func LookupUID(userStr string) (uint32, error) {
+	// checkif userStr is a valid id
+	uid, err := strconv.ParseUint(userStr, 10, 32)
+	if err != nil {
+		// userStr is not a valid uid, so lookup the name
+		user, err := LookupUser(userStr)
+		if err != nil {
+			return 0, err
+		}
+		// returned uid of the user is not a valid uid. This is rarely the case
+		uid, err = strconv.ParseUint(user.Uid, 10, 32)
+		if err != nil {
+			return 0, err
+		}
+	}
+	return uint32(uid), nil
+}

pkg/util/command_util_test.go

@@ -596,42 +596,14 @@ func TestGetUserGroup(t *testing.T) {
 			expectedU: -1,
 			expectedG: -1,
 		},
-		{
-			description: "with uid instead of username",
-			chown:       "1001:1001",
-			env:         []string{},
-			mockIDGetter: func(string, string, bool) (uint32, uint32, error) {
-				return 0, 0, fmt.Errorf("should not be called")
-			},
-			expectedU: 1001,
-			expectedG: 1001,
-		},
-		{
-			description: "uid but group is a name",
-			chown:       "1001:mygroup",
-			env:         []string{},
-			mockIDGetter: func(string, string, bool) (uint32, uint32, error) {
-				return 0, 0, fmt.Errorf("should not be called")
-			},
-			mockGroupIDGetter: func(groupStr string) (*user.Group, error) {
-				// random uid for mygroup since it's a test
-				return &user.Group{Gid: "2000"}, nil
-			},
-			expectedU: 1001,
-			expectedG: 2000,
-			shdErr:    false,
-		},
 	}
 	for _, tc := range tests {
 		t.Run(tc.description, func(t *testing.T) {
-			originalIDGetter := getIdsFromUsernameAndGroup
-			originalGroupIDGetter := getGroupFromStr
+			originalIDGetter := GetUIDAndGID
 			defer func() {
-				getIdsFromUsernameAndGroup = originalIDGetter
-				getGroupFromStr = originalGroupIDGetter
+				GetUIDAndGID = originalIDGetter
 			}()
-			getIdsFromUsernameAndGroup = tc.mockIDGetter
-			getGroupFromStr = tc.mockGroupIDGetter
+			GetUIDAndGID = tc.mockIDGetter
 			uid, gid, err := GetUserGroup(tc.chown, tc.env)
 			testutil.CheckErrorAndDeepEqual(t, tc.shdErr, err, uid, tc.expectedU)
 			testutil.CheckErrorAndDeepEqual(t, tc.shdErr, err, gid, tc.expectedG)
@@ -709,19 +681,140 @@ func Test_GetUIDAndGIDFromString(t *testing.T) {
 	}
 	primaryGroup := primaryGroupObj.Name
 
-	testCases := []string{
-		fmt.Sprintf("%s:%s", currentUser.Uid, currentUser.Gid),
-		fmt.Sprintf("%s:%s", currentUser.Username, currentUser.Gid),
-		fmt.Sprintf("%s:%s", currentUser.Uid, primaryGroup),
-		fmt.Sprintf("%s:%s", currentUser.Username, primaryGroup),
+	type args struct {
+		userGroupStr  string
+		fallbackToUID bool
+	}
+
+	type expected struct {
+		userID  uint32
+		groupID uint32
+	}
+
+	expectedCurrentUserID, _ := strconv.ParseUint(currentUser.Uid, 10, 32)
+	expectedCurrentUserGID, _ := strconv.ParseUint(currentUser.Gid, 10, 32)
+
+	expectedCurrentUser := expected{
+		userID:  uint32(expectedCurrentUserID),
+		groupID: uint32(expectedCurrentUserGID),
+	}
+
+	testCases := []struct {
+		testname string
+		args     args
+		expected expected
+		wantErr  bool
+	}{
+		{
+			testname: "current user uid and gid",
+			args: args{
+				userGroupStr: fmt.Sprintf("%s:%s", currentUser.Uid, currentUser.Gid),
+			},
+			expected: expectedCurrentUser,
+		},
+		{
+			testname: "current user username and gid",
+			args: args{
+				userGroupStr: fmt.Sprintf("%s:%s", currentUser.Username, currentUser.Gid),
+			},
+			expected: expectedCurrentUser,
+		},
+		{
+			testname: "current user username and primary group",
+			args: args{
+				userGroupStr: fmt.Sprintf("%s:%s", currentUser.Username, primaryGroup),
+			},
+			expected: expectedCurrentUser,
+		},
+		{
+			testname: "current user uid and primary group",
+			args: args{
+				userGroupStr: fmt.Sprintf("%s:%s", currentUser.Uid, primaryGroup),
+			},
+			expected: expectedCurrentUser,
+		},
+		{
+			testname: "non-existing valid uid and gid",
+			args: args{
+				userGroupStr: fmt.Sprintf("%d:%d", 1001, 50000),
+			},
+			expected: expected{
+				userID:  1001,
+				groupID: 50000,
+			},
+		},
+		{
+			testname: "uid and existing group",
+			args: args{
+				userGroupStr: fmt.Sprintf("%d:%s", 1001, primaryGroup),
+			},
+			expected: expected{
+				userID:  1001,
+				groupID: uint32(expectedCurrentUserGID),
+			},
+		},
+		{
+			testname: "uid and non existing group-name with fallbackToUID",
+			args: args{
+				userGroupStr:  fmt.Sprintf("%d:%s", 1001, "hello-world-group"),
+				fallbackToUID: true,
+			},
+			expected: expected{
+				userID:  1001,
+				groupID: 1001,
+			},
+		},
+		{
+			testname: "uid and non existing group-name",
+			args: args{
+				userGroupStr: fmt.Sprintf("%d:%s", 1001, "hello-world-group"),
+			},
+			wantErr: true,
+		},
+		{
+			testname: "name and non existing gid",
+			args: args{
+				userGroupStr: fmt.Sprintf("%s:%d", currentUser.Username, 50000),
+			},
+			expected: expected{
+				userID:  expectedCurrentUser.userID,
+				groupID: 50000,
+			},
+		},
+		{
+			testname: "only uid and fallback is false",
+			args: args{
+				userGroupStr:  fmt.Sprintf("%d", expectedCurrentUserID),
+				fallbackToUID: false,
+			},
+			wantErr: true,
+		},
+		{
+			testname: "only uid and fallback is true",
+			args: args{
+				userGroupStr:  fmt.Sprintf("%d", expectedCurrentUserID),
+				fallbackToUID: true,
+			},
+			expected: expected{
+				userID:  expectedCurrentUser.userID,
+				groupID: expectedCurrentUser.userID,
+			},
+		},
+		{
+			testname: "non-existing user without group",
+			args: args{
+				userGroupStr: "helloworlduser",
+			},
+			wantErr: true,
+		},
 	}
-	expectedU, _ := strconv.ParseUint(currentUser.Uid, 10, 32)
-	expectedG, _ := strconv.ParseUint(currentUser.Gid, 10, 32)
 	for _, tt := range testCases {
-		uid, gid, err := GetUIDAndGIDFromString(tt, false)
-		if uid != uint32(expectedU) || gid != uint32(expectedG) || err != nil {
-			t.Logf("Error: %v", err)
-			t.Errorf("Could not correctly decode %s to uid/gid %d:%d. Result: %d:%d", tt, expectedU, expectedG,
+		uid, gid, err := getUIDAndGIDFromString(tt.args.userGroupStr, tt.args.fallbackToUID)
+		testutil.CheckError(t, tt.wantErr, err)
+		if uid != tt.expected.userID || gid != tt.expected.groupID {
+			t.Errorf("Could not correctly decode %s to uid/gid %d:%d. Result: %d:%d",
+				tt.args.userGroupStr,
+				tt.expected.userID, tt.expected.groupID,
 				uid, gid)
 		}
 	}

pkg/util/syscall_credentials.go

@@ -25,7 +25,7 @@ import (
 )
 
 func SyscallCredentials(userStr string) (*syscall.Credential, error) {
-	uid, gid, err := GetUIDAndGIDFromString(userStr, true)
+	uid, gid, err := getUIDAndGIDFromString(userStr, true)
 	if err != nil {
 		return nil, errors.Wrap(err, "get uid/gid")
 	}