diff --git a/pkg/resources/management.cattle.io/v3/users/validator.go b/pkg/resources/management.cattle.io/v3/users/validator.go index e1e9fc38e..467d7298e 100644 --- a/pkg/resources/management.cattle.io/v3/users/validator.go +++ b/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 { diff --git a/pkg/resources/management.cattle.io/v3/users/validator_test.go b/pkg/resources/management.cattle.io/v3/users/validator_test.go index bf4336e36..a1a2d9e59 100644 --- a/pkg/resources/management.cattle.io/v3/users/validator_test.go +++ b/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 {