diff --git a/pkg/groups/group_test.go b/pkg/groups/group_test.go index 17ca7d860..28d28caee 100644 --- a/pkg/groups/group_test.go +++ b/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", }, } diff --git a/pkg/groups/manager.go b/pkg/groups/manager.go index b01718c1c..10e428c03 100644 --- a/pkg/groups/manager.go +++ b/pkg/groups/manager.go @@ -10,6 +10,7 @@ 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 ( @@ -17,14 +18,6 @@ const ( 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) } diff --git a/pkg/validation/validation.go b/pkg/validation/validation.go index 089fe0da3..b7ba6f3a1 100644 --- a/pkg/validation/validation.go +++ b/pkg/validation/validation.go @@ -7,10 +7,10 @@ import ( "strings" ) -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) == "" { @@ -22,9 +22,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 diff --git a/pkg/validation/validation_test.go b/pkg/validation/validation_test.go index 15bc841d7..5d0da2205 100644 --- a/pkg/validation/validation_test.go +++ b/pkg/validation/validation_test.go @@ -16,33 +16,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 {