Enhance Contact Ownership Validation Security and Observability

internal/webhooks/notification/v1alpha1/contact_ownership_validator.go

@@ -8,9 +8,12 @@ import (
 	notificationv1alpha1 "go.miloapis.com/milo/pkg/apis/notification/v1alpha1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
 
+var ownershipLog = logf.Log.WithName("contact-ownership-validator")
+
 // ValidateContactOwnership checks if the authenticated user (when in user context) owns the contact.
 // This validation only applies when the request is made through a user context
 // (i.e., via /apis/iam.miloapis.com/v1alpha1/users/{user}/control-plane).
@@ -26,33 +29,74 @@ import (
 //   - A Forbidden error if the ownership check fails
 //   - nil if the ownership is valid or no user context is present
 func ValidateContactOwnership(ctx context.Context, contact *notificationv1alpha1.Contact, resource schema.GroupResource, resourceName string, operation string) error {
+	ownershipLog.Info("Starting contact ownership validation",
+		"resource", resource.String(),
+		"resourceName", resourceName,
+		"operation", operation,
+		"contactName", contact.Name,
+		"contactNamespace", contact.Namespace)
+
 	// Only enforce ownership validation if the request was made through a user context
 	// (i.e., via /apis/iam.miloapis.com/v1alpha1/users/{user}/control-plane)
 	req, err := admission.RequestFromContext(ctx)
 	if err != nil {
 		// No admission request in context, skip validation
+		ownershipLog.Info("No admission request in context, skipping ownership validation",
+			"resource", resource.String(),
+			"resourceName", resourceName)
 		return nil
 	}
 
 	_, hasUserContext := req.UserInfo.Extra[iamv1alpha1.ParentNameExtraKey]
 	if !hasUserContext {
 		// Not in user context, skip validation
+		ownershipLog.Info("Not in user context, skipping ownership validation",
+			"resource", resource.String(),
+			"resourceName", resourceName,
+			"userUID", req.UserInfo.UID)
 		return nil
 	}
 
+	ownershipLog.Info("User context detected, validating ownership",
+		"resource", resource.String(),
+		"resourceName", resourceName,
+		"userUID", req.UserInfo.UID)
+
 	// Validate that the user can only perform operations for their own contact
 	// The Contact's subject.name should match the authenticated user's UID
 	if contact.Spec.SubjectRef == nil {
-		return nil
+		ownershipLog.Info("Ownership validation failed: contact has no subject reference in user context",
+			"resource", resource.String(),
+			"resourceName", resourceName,
+			"operation", operation,
+			"userUID", req.UserInfo.UID,
+			"contactName", contact.Name)
+		return errors.NewForbidden(
+			resource,
+			resourceName,
+			fmt.Errorf("you do not have permission to %s for this contact", operation),
+		)
 	}
 
 	if contact.Spec.SubjectRef.Name != string(req.UserInfo.UID) {
+		ownershipLog.Info("Ownership validation failed: user does not own the contact",
+			"resource", resource.String(),
+			"resourceName", resourceName,
+			"operation", operation,
+			"userUID", req.UserInfo.UID,
+			"contactSubjectName", contact.Spec.SubjectRef.Name)
 		return errors.NewForbidden(
 			resource,
 			resourceName,
 			fmt.Errorf("you do not have permission to %s for this contact", operation),
 		)
 	}
 
+	ownershipLog.V(1).Info("Ownership validation succeeded",
+		"resource", resource.String(),
+		"resourceName", resourceName,
+		"operation", operation,
+		"userUID", req.UserInfo.UID)
+
 	return nil
 }

internal/webhooks/notification/v1alpha1/contactgroupmembershipremoval_webhook.go

@@ -72,9 +72,6 @@ func (v *ContactGroupMembershipRemovalValidator) ValidateCreate(ctx context.Cont
 			return nil, errors.NewInternalError(fmt.Errorf("failed to get ContactGroup: %w", err))
 		}
 	}
-	if contactGroup.Spec.Visibility == notificationv1alpha1.ContactGroupVisibilityPrivate {
-		errs = append(errs, field.Invalid(field.NewPath("spec", "contactGroupRef", "visibility"), contactGroup.Spec.Visibility, "ContactGroup must be public for creating a ContactGroupMembershipRemoval"))
-	}
 
 	// Prevent duplicate removals
 	var existing notificationv1alpha1.ContactGroupMembershipRemovalList