Add update validation for User.UserName (#943)

* Add update validation for User.UserName

* Fix unit test

Author: JonCrowther

docs.md

@@ -524,6 +524,12 @@ When a user is updated or deleted, a check occurs to ensure that the user making
 
 If the user making the request has the verb `manage-users` for the resource `users`, then it is allowed to bypass the check. Note that the wildcard `*` includes the `manage-users` verb.
 
+#### Invalid Fields - Update
+
+Users can update the following fields if they had not been set. But after getting initial values, the fields cannot be changed:
+
+- UserName
+
 ## UserAttribute
 
 ### Validation Checks

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

@@ -4,4 +4,10 @@
 
 When a user is updated or deleted, a check occurs to ensure that the user making the request has permissions greater than or equal to the user being updated or deleted. To get the user's groups, the user's UserAttributes are checked. This is best effort, because UserAttributes are only updated when a User logs in, so it may not be perfectly up to date.
 
-If the user making the request has the verb `manage-users` for the resource `users`, then it is allowed to bypass the check. Note that the wildcard `*` includes the `manage-users` verb.
+If the user making the request has the verb `manage-users` for the resource `users`, then it is allowed to bypass the check. Note that the wildcard `*` includes the `manage-users` verb.
+
+### Invalid Fields - Update
+
+Users can update the following fields if they had not been set. But after getting initial values, the fields cannot be changed:
+
+- UserName

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

@@ -14,6 +14,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/apiserver/pkg/authentication/user"
 	authorizationv1 "k8s.io/client-go/kubernetes/typed/authorization/v1"
 	"k8s.io/kubernetes/pkg/registry/rbac/validation"
@@ -81,26 +82,27 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp
 	if hasManageUsers {
 		return &admissionv1.AdmissionResponse{Allowed: true}, nil
 	}
-	userObj, err := objectsv3.UserFromRequest(&request.AdmissionRequest)
+
+	oldUser, newUser, err := objectsv3.UserOldAndNewFromRequest(&request.AdmissionRequest)
 	if err != nil {
 		return nil, fmt.Errorf("failed to get current User from request: %w", err)
 	}
 
 	// Need the UserAttribute to find the groups
-	userAttribute, err := a.userAttributeCache.Get(userObj.Name)
+	userAttribute, err := a.userAttributeCache.Get(oldUser.Name)
 	if err != nil && !apierrors.IsNotFound(err) {
-		return nil, fmt.Errorf("failed to get UserAttribute for %s: %w", userObj.Name, err)
+		return nil, fmt.Errorf("failed to get UserAttribute for %s: %w", oldUser.Name, err)
 	}
 
 	userInfo := &user.DefaultInfo{
-		Name:   userObj.Name,
+		Name:   oldUser.Name,
 		Groups: getGroupsFromUserAttribute(userAttribute),
 	}
 
 	// Get all rules for the user being modified
 	rules, err := a.resolver.RulesFor(context.Background(), userInfo, "")
 	if err != nil {
-		return nil, fmt.Errorf("failed to get rules for user %v: %w", userObj, err)
+		return nil, fmt.Errorf("failed to get rules for user %v: %w", oldUser, err)
 	}
 
 	// Ensure that rules of the user being modified aren't greater than the rules of the user making the request
@@ -115,6 +117,14 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp
 			},
 		}, nil
 	}
+
+	fieldPath := field.NewPath("user")
+	if request.Operation == admissionv1.Update {
+		if err := validateUpdateFields(oldUser, newUser, fieldPath); err != nil {
+			return admission.ResponseBadRequest(err.Error()), nil
+		}
+	}
+
 	return &admissionv1.AdmissionResponse{Allowed: true}, nil
 }
 
@@ -133,3 +143,12 @@ func getGroupsFromUserAttribute(userAttribute *v3.UserAttribute) []string {
 	}
 	return result
 }
+
+// validateUpdateFields
+func validateUpdateFields(oldUser, newUser *v3.User, fieldPath *field.Path) error {
+	const reason = "field is immutable"
+	if oldUser.Username != "" && oldUser.Username != newUser.Username {
+		return field.Invalid(fieldPath.Child("username"), newUser.Username, reason)
+	}
+	return nil
+}

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

@@ -35,6 +35,7 @@ var (
 		ObjectMeta: metav1.ObjectMeta{
 			Name: defaultUserName,
 		},
+		Username: defaultUserName,
 	}
 	getPods = []rbacv1.PolicyRule{
 		{
@@ -196,6 +197,49 @@ func Test_Admit(t *testing.T) {
 			},
 			allowed: false,
 		},
+		{
+			name:    "changing the username not allowed",
+			oldUser: defaultUser.DeepCopy(),
+			newUser: &v3.User{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: defaultUserName,
+				},
+				Username: "new-username",
+			},
+			requestUserName: requesterUserName,
+			resolverRulesFor: func(string) ([]rbacv1.PolicyRule, error) {
+				return getPods, nil
+			},
+			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(),
+			newUser: &v3.User{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: defaultUserName,
+				},
+			},
+			requestUserName: requesterUserName,
+			resolverRulesFor: func(string) ([]rbacv1.PolicyRule, error) {
+				return getPods, nil
+			},
+			allowed: false,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {