Move username validation to happen before manage-users check (#1016)

Author: JonCrowther

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

@@ -73,6 +73,19 @@ func (v *Validator) Admitters() []admission.Admitter {
 }
 
 func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResponse, error) {
+	oldUser, newUser, err := objectsv3.UserOldAndNewFromRequest(&request.AdmissionRequest)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get current User from request: %w", err)
+	}
+
+	// Validate update fields
+	fieldPath := field.NewPath("user")
+	if request.Operation == admissionv1.Update {
+		if err := validateUpdateFields(oldUser, newUser, fieldPath); err != nil {
+			return admission.ResponseBadRequest(err.Error()), nil
+		}
+	}
+
 	// Check if requester has manage-user verb
 	hasManageUsers, err := auth.RequestUserHasVerb(request, gvr, a.sar, manageUsersVerb, "", "")
 	if err != nil {
@@ -83,11 +96,6 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp
 		return &admissionv1.AdmissionResponse{Allowed: true}, nil
 	}
 
-	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(oldUser.Name)
 	if err != nil && !apierrors.IsNotFound(err) {
@@ -118,13 +126,6 @@ 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
 }
 
@@ -144,7 +145,7 @@ func getGroupsFromUserAttribute(userAttribute *v3.UserAttribute) []string {
 	return result
 }
 
-// validateUpdateFields
+// validateUpdateFields validates fields during an update. The manage-users verb does not apply to these validations.
 func validateUpdateFields(oldUser, newUser *v3.User, fieldPath *field.Path) error {
 	const reason = "field is immutable"
 	if oldUser.Username != "" && oldUser.Username != newUser.Username {

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

@@ -115,12 +115,12 @@ func Test_Admit(t *testing.T) {
 			oldUser:         defaultUser.DeepCopy(),
 			requestUserName: requesterUserName,
 			resolverRulesFor: func(s string) ([]rbacv1.PolicyRule, error) {
-				if s == requesterUserName {
-					return getPods, nil
-				} else if s == defaultUserName {
+				switch s {
+				case requesterUserName, defaultUserName:
 					return getPods, nil
+				default:
+					return nil, fmt.Errorf("unexpected error")
 				}
-				return nil, fmt.Errorf("unexpected error")
 			},
 			allowed: true,
 		},
@@ -130,12 +130,12 @@ func Test_Admit(t *testing.T) {
 			newUser:         defaultUser.DeepCopy(),
 			requestUserName: requesterUserName,
 			resolverRulesFor: func(s string) ([]rbacv1.PolicyRule, error) {
-				if s == requesterUserName {
-					return getPods, nil
-				} else if s == defaultUserName {
+				switch s {
+				case requesterUserName, defaultUserName:
 					return getPods, nil
+				default:
+					return nil, fmt.Errorf("unexpected error")
 				}
-				return nil, fmt.Errorf("unexpected error")
 			},
 			allowed: true,
 		},
@@ -144,12 +144,14 @@ func Test_Admit(t *testing.T) {
 			oldUser:         defaultUser.DeepCopy(),
 			requestUserName: requesterUserName,
 			resolverRulesFor: func(s string) ([]rbacv1.PolicyRule, error) {
-				if s == requesterUserName {
+				switch s {
+				case requesterUserName:
 					return starPods, nil
-				} else if s == defaultUserName {
+				case defaultUserName:
 					return getPods, nil
+				default:
+					return nil, fmt.Errorf("unexpected error")
 				}
-				return nil, fmt.Errorf("unexpected error")
 			},
 			allowed: true,
 		},
@@ -159,12 +161,14 @@ func Test_Admit(t *testing.T) {
 			newUser:         defaultUser.DeepCopy(),
 			requestUserName: requesterUserName,
 			resolverRulesFor: func(s string) ([]rbacv1.PolicyRule, error) {
-				if s == requesterUserName {
+				switch s {
+				case requesterUserName:
 					return starPods, nil
-				} else if s == defaultUserName {
+				case defaultUserName:
 					return getPods, nil
+				default:
+					return nil, fmt.Errorf("unexpected error")
 				}
-				return nil, fmt.Errorf("unexpected error")
 			},
 			allowed: true,
 		},
@@ -173,12 +177,14 @@ func Test_Admit(t *testing.T) {
 			oldUser:         defaultUser.DeepCopy(),
 			requestUserName: requesterUserName,
 			resolverRulesFor: func(s string) ([]rbacv1.PolicyRule, error) {
-				if s == requesterUserName {
+				switch s {
+				case requesterUserName:
 					return getPods, nil
-				} else if s == defaultUserName {
+				case defaultUserName:
 					return starPods, nil
+				default:
+					return nil, fmt.Errorf("unexpected error")
 				}
-				return nil, fmt.Errorf("unexpected error")
 			},
 			allowed: false,
 		},
@@ -188,12 +194,14 @@ func Test_Admit(t *testing.T) {
 			newUser:         defaultUser.DeepCopy(),
 			requestUserName: requesterUserName,
 			resolverRulesFor: func(s string) ([]rbacv1.PolicyRule, error) {
-				if s == requesterUserName {
+				switch s {
+				case requesterUserName:
 					return getPods, nil
-				} else if s == defaultUserName {
+				case defaultUserName:
 					return starPods, nil
+				default:
+					return nil, fmt.Errorf("unexpected error")
 				}
-				return nil, fmt.Errorf("unexpected error")
 			},
 			allowed: false,
 		},
@@ -207,10 +215,7 @@ func Test_Admit(t *testing.T) {
 				Username: "new-username",
 			},
 			requestUserName: requesterUserName,
-			resolverRulesFor: func(string) ([]rbacv1.PolicyRule, error) {
-				return getPods, nil
-			},
-			allowed: false,
+			allowed:         false,
 		},
 		{
 			name: "adding a new username allowed",
@@ -235,10 +240,7 @@ func Test_Admit(t *testing.T) {
 				},
 			},
 			requestUserName: requesterUserName,
-			resolverRulesFor: func(string) ([]rbacv1.PolicyRule, error) {
-				return getPods, nil
-			},
-			allowed: false,
+			allowed:         false,
 		},
 	}
 	for _, tt := range tests {