Extend the username uniqueness check to updates as well as creates (#1053)

* Add the check and tests

* Update docs

Author: JonCrowther

docs.md

@@ -523,7 +523,7 @@ When a Token is updated, the following checks take place:
 
 ### Validation Checks
 
-#### Create
+#### Create and Delete
 
 Verifies there aren't any other users with the same username.
 

pkg/resources/management.cattle.io/v3/users/User.md

@@ -1,6 +1,6 @@
 ## Validation Checks
 
-### Create
+### Create and Delete
 
 Verifies there aren't any other users with the same username.
 

pkg/resources/management.cattle.io/v3/users/validator.go

@@ -83,14 +83,8 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp
 	}
 
 	if request.Operation == admissionv1.Create && newUser.Username != "" {
-		users, err := a.userCache.List(labels.Everything())
-		if err != nil {
-			return nil, fmt.Errorf("failed to get users %w", err)
-		}
-		for _, user := range users {
-			if user.Username == newUser.Username {
-				return admission.ResponseBadRequest("username already exists"), nil
-			}
+		if resp, err := a.checkUsernameUniqueness(newUser.Username); err != nil || resp != nil {
+			return resp, err
 		}
 		return &admissionv1.AdmissionResponse{Allowed: true}, nil
 	}
@@ -100,6 +94,12 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp
 		if err := validateUpdateFields(oldUser, newUser, fieldPath); err != nil {
 			return admission.ResponseBadRequest(err.Error()), nil
 		}
+		if oldUser.Username == "" && newUser.Username != "" {
+			if resp, err := a.checkUsernameUniqueness(newUser.Username); err != nil || resp != nil {
+				return resp, err
+			}
+		}
+
 		oldUserEnabled := ptr.Deref(oldUser.Enabled, false)
 		newUserEnabled := ptr.Deref(newUser.Enabled, false)
 
@@ -156,6 +156,23 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp
 	return &admissionv1.AdmissionResponse{Allowed: true}, nil
 }
 
+// checkUsernameUniqueness checks if a given username is already in use by another user.
+func (a *admitter) checkUsernameUniqueness(username string) (*admissionv1.AdmissionResponse, error) {
+	if username == "" {
+		return nil, nil
+	}
+	users, err := a.userCache.List(labels.Everything())
+	if err != nil {
+		return nil, fmt.Errorf("failed to get users: %w", err)
+	}
+	for _, user := range users {
+		if user.Username == username {
+			return admission.ResponseBadRequest("username already exists"), nil
+		}
+	}
+	return nil, nil
+}
+
 // getGroupsFromUserAttributes gets the list of group principals from a UserAttribute.
 //
 // Warning: UserAttributes are only updated when a user logs in, so this may not have the up to date Group Principals.

pkg/resources/management.cattle.io/v3/users/validator_test.go

@@ -30,7 +30,6 @@ const (
 	managerUserName   = "manage-user"
 	defaultUserName   = "test-user"
 	sarErrorUser      = "sar-error-user"
-	ssrErrorUser      = "ssr-error-user"
 	requesterUserName = "requester-user"
 )
 
@@ -222,20 +221,6 @@ func Test_Admit(t *testing.T) {
 			requestUserName: requesterUserName,
 			allowed:         false,
 		},
-		{
-			name: "adding a new username allowed",
-			oldUser: &v3.User{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: defaultUserName,
-				},
-			},
-			newUser:         defaultUser.DeepCopy(),
-			requestUserName: requesterUserName,
-			resolverRulesFor: func(string) ([]rbacv1.PolicyRule, error) {
-				return getPods, nil
-			},
-			allowed: true,
-		},
 		{
 			name:    "removing username not allowed",
 			oldUser: defaultUser.DeepCopy(),
@@ -308,6 +293,52 @@ func Test_Admit(t *testing.T) {
 			allowed: false,
 			wantErr: true,
 		},
+		{
+			name: "username already exists on update",
+			oldUser: &v3.User{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "another-user",
+				},
+			},
+			newUser: &v3.User{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "another-user",
+				},
+				Username: defaultUserName,
+			},
+			requestUserName: managerUserName,
+			mockUserCache: func() controllerv3.UserCache {
+				mock := fake.NewMockNonNamespacedCacheInterface[*v3.User](ctrl)
+				mock.EXPECT().List(labels.Everything()).Return([]*v3.User{
+					&defaultUser,
+				}, nil)
+				return mock
+			},
+			allowed: false,
+		},
+		{
+			name: "setting a unique username on update is allowed",
+			oldUser: &v3.User{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "another-user",
+				},
+			},
+			newUser: &v3.User{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "another-user",
+				},
+				Username: "a-different-username",
+			},
+			requestUserName: managerUserName,
+			mockUserCache: func() controllerv3.UserCache {
+				mock := fake.NewMockNonNamespacedCacheInterface[*v3.User](ctrl)
+				mock.EXPECT().List(labels.Everything()).Return([]*v3.User{
+					&defaultUser,
+				}, nil)
+				return mock
+			},
+			allowed: true,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {