From a4dbefccf1fc38e9b5a250a41a290a3be5be5aa4 Mon Sep 17 00:00:00 2001 From: qingliu Date: Wed, 29 Oct 2025 22:01:35 +0800 Subject: [PATCH 1/2] fix: allow user to approve when they are both individual approver and group member Previously, when a user was listed as both an individual User approver and a member of a Group approver, the function would return early after checking the first matching approver type. This prevented the user from using their other approval identity entirely. For example, if User entry appeared before Group entry in the approvers list, the user could only approve as an individual user and their Group membership was completely ignored. This fix changes IsUserApprovalChanged to check all approver types without early returns, accumulating valid changes. Now users can exercise all their approval permissions regardless of the order in approvers list. --- pkg/reconciler/webhook/webhook.go | 36 +- pkg/reconciler/webhook/webhook_test.go | 469 +++++++++++++++++++++++++ 2 files changed, 496 insertions(+), 9 deletions(-) create mode 100644 pkg/reconciler/webhook/webhook_test.go diff --git a/pkg/reconciler/webhook/webhook.go b/pkg/reconciler/webhook/webhook.go index ec8b160e..6be983f7 100644 --- a/pkg/reconciler/webhook/webhook.go +++ b/pkg/reconciler/webhook/webhook.go @@ -332,9 +332,18 @@ func hasOnlyInputChanged(oldObjApprover, newObjApprover v1alpha1.ApproverDetails // IsUserApprovalChanged checks if there is a valid input change for the current user. func IsUserApprovalChanged(oldObjApprovers, newObjApprovers []v1alpha1.ApproverDetails, request *admissionv1.AdmissionRequest) (bool, error) { currentUser := request.UserInfo.Username + hasValidChange := false + var validationError error + for i, approver := range oldObjApprovers { if approver.Name == currentUser && v1alpha1.DefaultedApproverType(approver.Type) == "User" { - return hasOnlyInputChanged(approver, newObjApprovers[i]) + changed, err := hasOnlyInputChanged(approver, newObjApprovers[i]) + if err != nil { + validationError = err + } else if changed { + hasValidChange = true + } + // Don't return early - continue checking for group changes } if v1alpha1.DefaultedApproverType(approver.Type) == "Group" { @@ -362,9 +371,10 @@ func IsUserApprovalChanged(oldObjApprovers, newObjApprovers []v1alpha1.ApproverD if i < len(newObjApprovers) { if approver.Input != newObjApprovers[i].Input { if err := hasValidInputValue(newObjApprovers[i].Input); err != nil { - return false, err + validationError = err + } else { + hasValidChange = true } - return true, nil } } @@ -395,13 +405,14 @@ func IsUserApprovalChanged(oldObjApprovers, newObjApprovers []v1alpha1.ApproverD for _, user := range newObjApprovers[i].Users { if user.Name == currentUser { if err := hasValidInputValue(user.Input); err != nil { - return false, err + validationError = err + } else { + hasValidChange = true } - return true, nil + break } } } - return true, nil } // Allow changes to individual user inputs within the group @@ -432,14 +443,21 @@ func IsUserApprovalChanged(oldObjApprovers, newObjApprovers []v1alpha1.ApproverD // Allow user to change their input if they're in both old and new lists if userFoundInOld && userFoundInNew && oldUserInput != newUserInput { if err := hasValidInputValue(newUserInput); err != nil { - return false, err + validationError = err + } else { + hasValidChange = true } - return true, nil } } } } - return false, nil + + // If there was a validation error, return it + if validationError != nil { + return false, validationError + } + + return hasValidChange, nil } // checkIfUserAlreadyDecided checks if a user is trying to re-approve/re-reject a task they've already decided on diff --git a/pkg/reconciler/webhook/webhook_test.go b/pkg/reconciler/webhook/webhook_test.go new file mode 100644 index 00000000..cd13a69b --- /dev/null +++ b/pkg/reconciler/webhook/webhook_test.go @@ -0,0 +1,469 @@ +package webhook + +import ( + "testing" + + "github.com/openshift-pipelines/manual-approval-gate/pkg/apis/approvaltask/v1alpha1" + admissionv1 "k8s.io/api/admission/v1" + authenticationv1 "k8s.io/api/authentication/v1" +) + +// TestIsUserApprovalChanged_UserCanModifyOwnUserAndGroupApproval tests that a user +// who is both listed as an individual User approver and is a member of a Group approver +// can modify either their User entry or their Group entry. +func TestIsUserApprovalChanged_UserCanModifyOwnUserAndGroupApproval(t *testing.T) { + tests := []struct { + name string + oldApprovers []v1alpha1.ApproverDetails + newApprovers []v1alpha1.ApproverDetails + username string + userGroups []string + expectedChanged bool + expectedError bool + description string + }{ + { + name: "user modifies their own User entry only", + oldApprovers: []v1alpha1.ApproverDetails{ + { + Name: "admin", + Type: "User", + Input: "pending", + }, + { + Name: "someone", + Type: "User", + Input: "pending", + }, + { + Name: "team-with-someone", + Type: "Group", + Input: "pending", + }, + }, + newApprovers: []v1alpha1.ApproverDetails{ + { + Name: "admin", + Type: "User", + Input: "pending", + }, + { + Name: "someone", + Type: "User", + Input: "approve", + }, + { + Name: "team-with-someone", + Type: "Group", + Input: "pending", + }, + }, + username: "someone", + userGroups: []string{"team-with-someone"}, + expectedChanged: true, + expectedError: false, + description: "User someone modifies their own User entry from pending to approve", + }, + { + name: "user modifies their Group entry only", + oldApprovers: []v1alpha1.ApproverDetails{ + { + Name: "admin", + Type: "User", + Input: "pending", + }, + { + Name: "someone", + Type: "User", + Input: "approve", + }, + { + Name: "team-with-someone", + Type: "Group", + Input: "pending", + }, + }, + newApprovers: []v1alpha1.ApproverDetails{ + { + Name: "admin", + Type: "User", + Input: "pending", + }, + { + Name: "someone", + Type: "User", + Input: "approve", + }, + { + Name: "team-with-someone", + Type: "Group", + Input: "approve", + }, + }, + username: "someone", + userGroups: []string{"team-with-someone"}, + expectedChanged: true, + expectedError: false, + description: "User someone (who is in team-with-someone) modifies the Group entry from pending to approve", + }, + { + name: "user modifies both their User entry and Group entry", + oldApprovers: []v1alpha1.ApproverDetails{ + { + Name: "admin", + Type: "User", + Input: "pending", + }, + { + Name: "someone", + Type: "User", + Input: "pending", + }, + { + Name: "team-with-someone", + Type: "Group", + Input: "pending", + }, + }, + newApprovers: []v1alpha1.ApproverDetails{ + { + Name: "admin", + Type: "User", + Input: "pending", + }, + { + Name: "someone", + Type: "User", + Input: "approve", + }, + { + Name: "team-with-someone", + Type: "Group", + Input: "approve", + }, + }, + username: "someone", + userGroups: []string{"team-with-someone"}, + expectedChanged: true, + expectedError: false, + description: "User someone modifies both their User entry and Group entry", + }, + { + name: "user has not modified anything", + oldApprovers: []v1alpha1.ApproverDetails{ + { + Name: "admin", + Type: "User", + Input: "pending", + }, + { + Name: "someone", + Type: "User", + Input: "approve", + }, + { + Name: "team-with-someone", + Type: "Group", + Input: "approve", + }, + }, + newApprovers: []v1alpha1.ApproverDetails{ + { + Name: "admin", + Type: "User", + Input: "pending", + }, + { + Name: "someone", + Type: "User", + Input: "approve", + }, + { + Name: "team-with-someone", + Type: "Group", + Input: "approve", + }, + }, + username: "someone", + userGroups: []string{"team-with-someone"}, + expectedChanged: false, + expectedError: false, + description: "User someone has not changed any approvals", + }, + { + name: "user provides invalid input value", + oldApprovers: []v1alpha1.ApproverDetails{ + { + Name: "someone", + Type: "User", + Input: "pending", + }, + { + Name: "team-with-someone", + Type: "Group", + Input: "pending", + }, + }, + newApprovers: []v1alpha1.ApproverDetails{ + { + Name: "someone", + Type: "User", + Input: "invalid", + }, + { + Name: "team-with-someone", + Type: "Group", + Input: "pending", + }, + }, + username: "someone", + userGroups: []string{"team-with-someone"}, + expectedChanged: false, + expectedError: true, + description: "User someone provides invalid input value", + }, + { + name: "user is not in the group and cannot modify group entry", + oldApprovers: []v1alpha1.ApproverDetails{ + { + Name: "someone", + Type: "User", + Input: "pending", + }, + { + Name: "g-other", + Type: "Group", + Input: "pending", + }, + }, + newApprovers: []v1alpha1.ApproverDetails{ + { + Name: "someone", + Type: "User", + Input: "pending", + }, + { + Name: "g-other", + Type: "Group", + Input: "approve", + }, + }, + username: "someone", + userGroups: []string{"team-with-someone"}, + expectedChanged: false, + expectedError: false, + description: "User someone is not in g-other group and should not be able to modify it", + }, + { + name: "user modifies group entry with multiple groups", + oldApprovers: []v1alpha1.ApproverDetails{ + { + Name: "someone", + Type: "User", + Input: "approve", + }, + { + Name: "team-with-someone", + Type: "Group", + Input: "pending", + }, + { + Name: "system:authenticated", + Type: "Group", + Input: "pending", + }, + }, + newApprovers: []v1alpha1.ApproverDetails{ + { + Name: "someone", + Type: "User", + Input: "approve", + }, + { + Name: "team-with-someone", + Type: "Group", + Input: "approve", + }, + { + Name: "system:authenticated", + Type: "Group", + Input: "pending", + }, + }, + username: "someone", + userGroups: []string{"team-with-someone", "system:authenticated"}, + expectedChanged: true, + expectedError: false, + description: "User someone modifies one of their group entries", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + request := &admissionv1.AdmissionRequest{ + UserInfo: authenticationv1.UserInfo{ + Username: tt.username, + Groups: tt.userGroups, + }, + } + + changed, err := IsUserApprovalChanged(tt.oldApprovers, tt.newApprovers, request) + + if tt.expectedError && err == nil { + t.Errorf("%s: expected error but got none", tt.description) + } + if !tt.expectedError && err != nil { + t.Errorf("%s: unexpected error: %v", tt.description, err) + } + if changed != tt.expectedChanged { + t.Errorf("%s: expected changed=%v, got changed=%v", tt.description, tt.expectedChanged, changed) + } + }) + } +} + +// TestIsUserApprovalChanged_GroupUsersList tests scenarios where users are explicitly +// listed in the group's users list +func TestIsUserApprovalChanged_GroupUsersList(t *testing.T) { + tests := []struct { + name string + oldApprovers []v1alpha1.ApproverDetails + newApprovers []v1alpha1.ApproverDetails + username string + userGroups []string + expectedChanged bool + expectedError bool + description string + }{ + { + name: "user modifies their input in group users list", + oldApprovers: []v1alpha1.ApproverDetails{ + { + Name: "team-with-someone", + Type: "Group", + Input: "approve", + Users: []v1alpha1.UserDetails{ + { + Name: "someone", + Input: "pending", + }, + }, + }, + }, + newApprovers: []v1alpha1.ApproverDetails{ + { + Name: "team-with-someone", + Type: "Group", + Input: "approve", + Users: []v1alpha1.UserDetails{ + { + Name: "someone", + Input: "approve", + }, + }, + }, + }, + username: "someone", + userGroups: []string{"team-with-someone"}, + expectedChanged: true, + expectedError: false, + description: "User someone modifies their input in group users list", + }, + { + name: "user adds themselves to group users list", + oldApprovers: []v1alpha1.ApproverDetails{ + { + Name: "team-with-someone", + Type: "Group", + Input: "pending", + Users: []v1alpha1.UserDetails{}, + }, + }, + newApprovers: []v1alpha1.ApproverDetails{ + { + Name: "team-with-someone", + Type: "Group", + Input: "pending", + Users: []v1alpha1.UserDetails{ + { + Name: "someone", + Input: "approve", + }, + }, + }, + }, + username: "someone", + userGroups: []string{"team-with-someone"}, + expectedChanged: true, + expectedError: false, + description: "User someone adds themselves to group users list", + }, + { + name: "user modifies both group-level input and their individual input in users list", + oldApprovers: []v1alpha1.ApproverDetails{ + { + Name: "someone", + Type: "User", + Input: "pending", + }, + { + Name: "team-with-someone", + Type: "Group", + Input: "pending", + Users: []v1alpha1.UserDetails{ + { + Name: "someone", + Input: "pending", + }, + }, + }, + }, + newApprovers: []v1alpha1.ApproverDetails{ + { + Name: "someone", + Type: "User", + Input: "approve", + }, + { + Name: "team-with-someone", + Type: "Group", + Input: "approve", + Users: []v1alpha1.UserDetails{ + { + Name: "someone", + Input: "approve", + }, + }, + }, + }, + username: "someone", + userGroups: []string{"team-with-someone"}, + expectedChanged: true, + expectedError: false, + description: "User someone modifies their User entry, group-level input, and their individual input in users list", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + request := &admissionv1.AdmissionRequest{ + UserInfo: authenticationv1.UserInfo{ + Username: tt.username, + Groups: tt.userGroups, + }, + } + + changed, err := IsUserApprovalChanged(tt.oldApprovers, tt.newApprovers, request) + + if tt.expectedError && err == nil { + t.Errorf("%s: expected error but got none", tt.description) + } + if !tt.expectedError && err != nil { + t.Errorf("%s: unexpected error: %v", tt.description, err) + } + if changed != tt.expectedChanged { + t.Errorf("%s: expected changed=%v, got changed=%v", tt.description, tt.expectedChanged, changed) + } + }) + } +} From d7267dfdb84c2f855d26e4daa52e2eba111f1c13 Mon Sep 17 00:00:00 2001 From: qingliu Date: Fri, 31 Oct 2025 13:54:41 +0800 Subject: [PATCH 2/2] refactor(webhook): simplify slice contains checks with slices.Contains - Replace manual loop-based slice contains logic with slices.Contains - Apply to group membership checks in ifUserExists, IsUserApprovalChanged, CheckOtherUsersForInvalidChanges, and checkIfUserAlreadyDecided - Replace webhookContains helper function with direct slices.Contains usage - Clean up trailing whitespace --- pkg/reconciler/webhook/webhook.go | 93 +++++++++++++------------------ 1 file changed, 38 insertions(+), 55 deletions(-) diff --git a/pkg/reconciler/webhook/webhook.go b/pkg/reconciler/webhook/webhook.go index 6be983f7..990c4183 100644 --- a/pkg/reconciler/webhook/webhook.go +++ b/pkg/reconciler/webhook/webhook.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "fmt" + "slices" "strings" "github.com/openshift-pipelines/manual-approval-gate/pkg/apis/approvaltask/v1alpha1" @@ -99,13 +100,13 @@ func (r *reconciler) Admit(ctx context.Context, request *admissionv1.AdmissionRe logger.Error("Unhandled kind: ", gvk) } - // Decode new object + // Decode new object newObj, err := r.decodeNewObject(newBytes) if err != nil { return webhook.MakeErrorStatus("cannot decode incoming new object: %v", err) } - // Validate structural requirements + // Validate structural requirements if err := validateApprovalTask(newObj, ctx); err != nil { return webhook.MakeErrorStatus("validation failed: %v", err) } @@ -260,10 +261,8 @@ func ifUserExists(approvals []v1alpha1.ApproverDetails, request *admissionv1.Adm } case "Group": // Check if user is in the group by checking the group name against user's groups - for _, userGroup := range request.UserInfo.Groups { - if approval.Name == userGroup { - return true - } + if slices.Contains(request.UserInfo.Groups, approval.Name) { + return true } // Also check if user is explicitly listed in the group's users for _, user := range approval.Users { @@ -281,15 +280,15 @@ func isApprovalRequired(approvaltask v1alpha1.ApprovalTask) bool { if approvaltask.Status.State == "rejected" || approvaltask.Status.State == "approved" { return false } - + // Use the same logic as the controller to count approvals approvedUsers := make(map[string]bool) - + for _, approver := range approvaltask.Spec.Approvers { if approver.Input != "approve" { continue } - + if v1alpha1.DefaultedApproverType(approver.Type) == "User" { approvedUsers[approver.Name] = true } else if v1alpha1.DefaultedApproverType(approver.Type) == "Group" { @@ -300,12 +299,12 @@ func isApprovalRequired(approvaltask v1alpha1.ApprovalTask) bool { } } } - + // If we have enough approvals, the task should be approved (final state) if len(approvedUsers) >= approvaltask.Spec.NumberOfApprovalsRequired { return false } - + return true } @@ -351,11 +350,8 @@ func IsUserApprovalChanged(oldObjApprovers, newObjApprovers []v1alpha1.ApproverD isUserInGroup := false // Check if user is in the group by checking the group name against user's groups - for _, userGroup := range request.UserInfo.Groups { - if approver.Name == userGroup { - isUserInGroup = true - break - } + if slices.Contains(request.UserInfo.Groups, approver.Name) { + isUserInGroup = true } // Also check if user is explicitly listed in the group's users @@ -463,10 +459,10 @@ func IsUserApprovalChanged(oldObjApprovers, newObjApprovers []v1alpha1.ApproverD // checkIfUserAlreadyDecided checks if a user is trying to re-approve/re-reject a task they've already decided on func checkIfUserAlreadyDecided(oldObj *v1alpha1.ApprovalTask, newObj *v1alpha1.ApprovalTask, request *admissionv1.AdmissionRequest) string { currentUser := request.UserInfo.Username - + // Get user's desired new input from the incoming object desiredInput := "" - + // First check if user is an individual approver for _, approver := range newObj.Spec.Approvers { if v1alpha1.DefaultedApproverType(approver.Type) == "User" && approver.Name == currentUser { @@ -474,7 +470,7 @@ func checkIfUserAlreadyDecided(oldObj *v1alpha1.ApprovalTask, newObj *v1alpha1.A break } } - + // If not found as individual user, check if user is in any group if desiredInput == "" { for _, approver := range newObj.Spec.Approvers { @@ -489,21 +485,16 @@ func checkIfUserAlreadyDecided(oldObj *v1alpha1.ApprovalTask, newObj *v1alpha1.A if desiredInput != "" { break } - + // Check if user is in the group via RBAC (group-level input) - for _, userGroup := range request.UserInfo.Groups { - if approver.Name == userGroup { - desiredInput = approver.Input - break - } - } - if desiredInput != "" { + if slices.Contains(request.UserInfo.Groups, approver.Name) { + desiredInput = approver.Input break } } } } - + // Check status.approversResponse to see if user has already made a decision for _, approverResponse := range oldObj.Status.ApproversResponse { if v1alpha1.DefaultedApproverType(approverResponse.Type) == "User" && approverResponse.Name == currentUser { @@ -514,7 +505,7 @@ func checkIfUserAlreadyDecided(oldObj *v1alpha1.ApprovalTask, newObj *v1alpha1.A return "User has already rejected" } } - + // Check if user is in any group that has responded if v1alpha1.DefaultedApproverType(approverResponse.Type) == "Group" { for _, member := range approverResponse.GroupMembers { @@ -529,7 +520,7 @@ func checkIfUserAlreadyDecided(oldObj *v1alpha1.ApprovalTask, newObj *v1alpha1.A } } } - + return "" // No issue found } @@ -548,11 +539,8 @@ func CheckOtherUsersForInvalidChanges(oldObjApprovers, newObjApprover []v1alpha1 isUserInGroup := false // Check if user is in the group by checking the group name against user's groups - for _, userGroup := range request.UserInfo.Groups { - if approver.Name == userGroup { - isUserInGroup = true - break - } + if slices.Contains(request.UserInfo.Groups, approver.Name) { + isUserInGroup = true } // Also check if user is explicitly listed in the group's users @@ -614,13 +602,13 @@ func CheckOtherUsersForInvalidChanges(oldObjApprovers, newObjApprover []v1alpha1 return true } -// validateApprovalTask validates the complete ApprovalTask resource +// validateApprovalTask validates the complete ApprovalTask resource func validateApprovalTask(approvalTask *v1alpha1.ApprovalTask, ctx context.Context) error { // Validate spec if err := validateApprovalTaskSpec(&approvalTask.Spec, ctx); err != nil { return fmt.Errorf("spec validation failed: %w", err) } - + return nil } @@ -640,11 +628,11 @@ func validateApprovalTaskSpec(spec *v1alpha1.ApprovalTaskSpec, ctx context.Conte approverNames := make(map[string]int) // name -> index for i, approver := range spec.Approvers { fieldPath := fmt.Sprintf("approvers[%d]", i) - + if err := validateApprover(approver, fieldPath); err != nil { return err } - + // Check for duplicate approver names approverKey := fmt.Sprintf("%s:%s", v1alpha1.DefaultedApproverType(approver.Type), approver.Name) if existingIndex, exists := approverNames[approverKey]; exists { @@ -683,18 +671,18 @@ func validateApprover(approver v1alpha1.ApproverDetails, fieldPath string) error // Validate users for group type if approverType == "Group" { - + // Track duplicate users within the group groupUsers := make(map[string]int) // username -> index for j, user := range approver.Users { userFieldPath := fmt.Sprintf("%s.users[%d]", fieldPath, j) - + if strings.TrimSpace(user.Name) == "" { return fmt.Errorf("%s.name: required field is missing", userFieldPath) } else if err := validateUserName(user.Name); err != nil { return fmt.Errorf("%s.name: %w", userFieldPath, err) } - + // Check for duplicate users within the group if existingIndex, exists := groupUsers[user.Name]; exists { return fmt.Errorf("%s.name: duplicate user '%s' within group (also found at %s.users[%d])", userFieldPath, user.Name, fieldPath, existingIndex) @@ -715,12 +703,12 @@ func validateNameFormat(name, fieldType string) error { if strings.TrimSpace(name) == "" { return fmt.Errorf("%s cannot be empty", fieldType) } - + // Kubernetes names cannot contain spaces if strings.Contains(name, " ") { return fmt.Errorf("%s cannot contain spaces", fieldType) } - + return nil } @@ -730,11 +718,11 @@ func validateUserName(name string) error { if strings.TrimSpace(name) == "" { return fmt.Errorf("username cannot be empty") } - + if strings.HasPrefix(name, "group:") { return fmt.Errorf("username cannot start with 'group:' prefix - use type: Group for group approvers") } - + return nil } @@ -743,23 +731,18 @@ func validateGroupName(name string) error { if err := validateNameFormat(name, "group name"); err != nil { return err } - + // Group names should not contain colons to avoid confusion with user prefixes if strings.Contains(name, ":") { return fmt.Errorf("group name cannot contain colons") } - + return nil } // webhookContains checks if a slice contains a string func webhookContains(slice []string, item string) bool { - for _, s := range slice { - if s == item { - return true - } - } - return false + return slices.Contains(slice, item) } // decodeNewObject decodes the incoming new object @@ -798,7 +781,7 @@ func validateApproverInputsForCreate(approvalTask *v1alpha1.ApprovalTask) error if approver.Input != "pending" { return fmt.Errorf("approvers[%d].input: must be 'pending' for new ApprovalTask, got '%s'", i, approver.Input) } - + // For group approvers, also validate that all users within the group have pending input if v1alpha1.DefaultedApproverType(approver.Type) == "Group" { for j, user := range approver.Users {