Fix group name validation (#2402)

- Move lowercase validation to primary validator
- Remove validation from manager since it's already used in CLI and API

Fix #2355

Author: eleftherias

pkg/groups/group_test.go

@@ -87,14 +87,14 @@ func TestManager_Create(t *testing.T) {
 			groupName:   "MyGroup",
 			setupMock:   func(_ *mocks.MockStore) {}, // validation fails before store access
 			expectError: true,
-			errorMsg:    "invalid group name",
+			errorMsg:    "must be lowercase",
 		},
 		{
 			name:        "invalid name - mixed case",
 			groupName:   "DefAult",
 			setupMock:   func(_ *mocks.MockStore) {}, // validation fails before store access
 			expectError: true,
-			errorMsg:    "invalid group name",
+			errorMsg:    "must be lowercase",
 		},
 	}
 

pkg/groups/manager.go

@@ -10,21 +10,14 @@ import (
 	thverrors "github.com/stacklok/toolhive/pkg/errors"
 	"github.com/stacklok/toolhive/pkg/logger"
 	"github.com/stacklok/toolhive/pkg/state"
+	"github.com/stacklok/toolhive/pkg/validation"
 )
 
 const (
 	// DefaultGroupName is the name of the default group
 	DefaultGroupName = "default"
 )
 
-// ValidateGroupName enforces lowercase-only group names.
-func ValidateGroupName(name string) error {
-	if name != strings.ToLower(name) {
-		return fmt.Errorf("invalid group name: %q (must be lowercase)", name)
-	}
-	return nil
-}
-
 // manager implements the Manager interface
 type manager struct {
 	groupStore state.Store
@@ -42,9 +35,9 @@ func NewManager() (Manager, error) {
 
 // Create creates a new group with the given name
 func (m *manager) Create(ctx context.Context, name string) error {
-	// Enforce lowercase group names
-	if err := ValidateGroupName(name); err != nil {
-		return err
+	// Validate group name
+	if err := validation.ValidateGroupName(name); err != nil {
+		return thverrors.NewInvalidArgumentError(err.Error(), err)
 	}
 	// Check if group already exists
 	exists, err := m.groupStore.Exists(ctx, name)
@@ -64,9 +57,6 @@ func (m *manager) Create(ctx context.Context, name string) error {
 
 // Get retrieves a group by name
 func (m *manager) Get(ctx context.Context, name string) (*Group, error) {
-	if err := ValidateGroupName(name); err != nil {
-		return nil, err
-	}
 	reader, err := m.groupStore.GetReader(ctx, name)
 	if err != nil {
 		return nil, fmt.Errorf("failed to get reader for group: %w", err)
@@ -107,17 +97,11 @@ func (m *manager) List(ctx context.Context) ([]*Group, error) {
 
 // Delete removes a group by name
 func (m *manager) Delete(ctx context.Context, name string) error {
-	if err := ValidateGroupName(name); err != nil {
-		return err
-	}
 	return m.groupStore.Delete(ctx, name)
 }
 
 // Exists checks if a group exists
 func (m *manager) Exists(ctx context.Context, name string) (bool, error) {
-	if err := ValidateGroupName(name); err != nil {
-		return false, err
-	}
 	return m.groupStore.Exists(ctx, name)
 }
 

pkg/validation/validation.go

@@ -9,10 +9,10 @@ import (
 	"golang.org/x/net/http/httpguts"
 )
 
-var validGroupNameRegex = regexp.MustCompile(`^[a-zA-Z0-9_\-\s]+$`)
+var validGroupNameRegex = regexp.MustCompile(`^[a-z0-9_\-\s]+$`)
 
 // ValidateGroupName validates that a group name only contains allowed characters:
-// alphanumeric, underscore, dash, and space.
+// lowercase alphanumeric, underscore, dash, and space.
 // It also enforces no leading/trailing/consecutive spaces and disallows null bytes.
 func ValidateGroupName(name string) error {
 	if name == "" || strings.TrimSpace(name) == "" {
@@ -24,9 +24,14 @@ func ValidateGroupName(name string) error {
 		return fmt.Errorf("group name cannot contain null bytes")
 	}
 
+	// Enforce lowercase-only group names
+	if name != strings.ToLower(name) {
+		return fmt.Errorf("group name must be lowercase")
+	}
+
 	// Validate characters
 	if !validGroupNameRegex.MatchString(name) {
-		return fmt.Errorf("group name can only contain alphanumeric characters, underscores, dashes, and spaces: %q", name)
+		return fmt.Errorf("group name can only contain lowercase alphanumeric characters, underscores, dashes, and spaces: %q", name)
 	}
 
 	// Check for leading/trailing whitespace

pkg/validation/validation_test.go

@@ -17,33 +17,36 @@ func TestValidateGroupName(t *testing.T) {
 		expectErr bool
 	}{
 		// ✅ Valid cases
-		{"valid_simple_name", "TeamAlpha", false},
-		{"valid_with_spaces", "Team Alpha", false},
-		{"valid_with_dash_and_underscore", "Team-Alpha_123", false},
+		{"valid_simple_name", "teamalpha", false},
+		{"valid_with_spaces", "team alpha", false},
+		{"valid_with_dash_and_underscore", "team-alpha_123", false},
 
 		// ❌ Empty or whitespace-only
 		{"empty_string", "", true},
 		{"only_spaces", "    ", true},
 
 		// ❌ Invalid characters
-		{"invalid_special_characters", "Team@Alpha!", true},
+		{"invalid_special_characters", "team@alpha!", true},
 		{"invalid_unicode", "团队🚀", true},
 
 		// ❌ Null byte
-		{"null_byte", "Team\x00Alpha", true},
+		{"null_byte", "team\x00alpha", true},
 
 		// ❌ Leading/trailing whitespace
-		{"leading_space", " TeamAlpha", true},
-		{"trailing_space", "TeamAlpha ", true},
+		{"leading_space", " teamalpha", true},
+		{"trailing_space", "teamalpha ", true},
 
 		// ❌ Consecutive spaces
-		{"consecutive_spaces_middle", "Team  Alpha", true},
-		{"consecutive_spaces_start", "  TeamAlpha", true},
-		{"consecutive_spaces_end", "TeamAlpha  ", true},
+		{"consecutive_spaces_middle", "team  alpha", true},
+		{"consecutive_spaces_start", "  teamalpha", true},
+		{"consecutive_spaces_end", "teamalpha  ", true},
+
+		// ❌ Uppercase letters
+		{"uppercase_letters", "TeamAlpha", true},
 
 		// ✅ Borderline valid
-		{"single_char", "T", false},
-		{"max_typical", "Alpha Team 2025 - Squad_01", false},
+		{"single_char", "t", false},
+		{"max_typical", "alpha team 2025 - squad_01", false},
 	}
 
 	for _, tc := range tests {