fix: add validation to patching project key (ET-305)

master/internal/api_project.go

@@ -824,7 +824,17 @@ func (a *apiServer) PatchProject(
 ) (*apiv1.PatchProjectResponse, error) {
 	curUser, _, err := grpcutil.GetUser(ctx)
 	if err != nil {
-		return nil, fmt.Errorf("failed to get user while updating project")
+		return nil, errors.New("failed to get user while updating project")
+	}
+
+	if req.Project == nil {
+		return nil, errors.New("project in request is nil while updating project")
+	}
+	if req.Project.Key != nil {
+		err = project.ValidateProjectKey(req.Project.Key.Value)
+		if err != nil {
+			return nil, status.Error(codes.InvalidArgument, err.Error())
+		}
 	}
 
 	updatedProject, err := project.UpdateProject(

master/internal/api_project_intg_test.go

@@ -27,6 +27,7 @@ import (
 	"github.com/determined-ai/determined/master/internal/mocks"
 	"github.com/determined-ai/determined/master/internal/project"
 	"github.com/determined-ai/determined/master/pkg/model"
+	"github.com/determined-ai/determined/master/pkg/random"
 	"github.com/determined-ai/determined/master/pkg/syncx/errgroupx"
 	"github.com/determined-ai/determined/proto/pkg/apiv1"
 	"github.com/determined-ai/determined/proto/pkg/projectv1"
@@ -465,7 +466,7 @@ func TestCreateProjectWithProjectKey(t *testing.T) {
 	require.NoError(t, werr)
 
 	projectName := "test-project" + uuid.New().String()
-	projectKey := uuid.New().String()[:project.MaxProjectKeyLength]
+	projectKey := random.String(project.MaxProjectKeyLength)
 	resp, err := api.PostProject(ctx, &apiv1.PostProjectRequest{
 		Name: projectName, WorkspaceId: wresp.Workspace.Id, Key: &projectKey,
 	})
@@ -487,7 +488,7 @@ func TestCreateProjectWithDuplicateProjectKey(t *testing.T) {
 	require.NoError(t, werr)
 
 	projectName := "test-project" + uuid.New().String()
-	projectKey := uuid.New().String()[:project.MaxProjectKeyLength]
+	projectKey := random.String(project.MaxProjectKeyLength)
 	_, err := api.PostProject(ctx, &apiv1.PostProjectRequest{
 		Name: projectName, WorkspaceId: wresp.Workspace.Id, Key: &projectKey,
 	})
@@ -560,7 +561,7 @@ func TestPatchProject(t *testing.T) {
 
 	newName := uuid.New().String()
 	newDescription := uuid.New().String()
-	newKey := uuid.New().String()[:project.MaxProjectKeyLength]
+	newKey := random.String(project.MaxProjectKeyLength)
 	_, err = api.PatchProject(ctx, &apiv1.PatchProjectRequest{
 		Id: resp.Project.Id,
 		Project: &projectv1.PatchProject{
@@ -625,7 +626,7 @@ func TestPatchProjectWithConcurrent(t *testing.T) {
 	newDescription := "new-description"
 	errgrp := errgroupx.WithContext(ctx)
 	for i := 0; i < 20; i++ {
-		newKey := uuid.New().String()[:project.MaxProjectKeyLength]
+		newKey := random.String(project.MaxProjectKeyLength)
 		errgrp.Go(func(context.Context) error {
 			_, err := api.PatchProject(ctx, &apiv1.PatchProjectRequest{
 				Id: resp.Project.Id,
@@ -641,3 +642,46 @@ func TestPatchProjectWithConcurrent(t *testing.T) {
 	}
 	require.NoError(t, errgrp.Wait())
 }
+
+func TestPatchProjectWithInvalidProjectKey(t *testing.T) {
+	api, _, ctx := setupAPITest(t, nil)
+	wresp, werr := api.PostWorkspace(ctx, &apiv1.PostWorkspaceRequest{Name: uuid.New().String()})
+	require.NoError(t, werr)
+
+	projectName := "test-project" + uuid.New().String()
+	resp, err := api.PostProject(ctx, &apiv1.PostProjectRequest{
+		Name: projectName, WorkspaceId: wresp.Workspace.Id,
+	})
+	require.NoError(t, err)
+
+	type TestCase struct {
+		Description string
+		Key         string
+		Err         string
+	}
+	testCases := []TestCase{
+		{
+			Description: "empty key",
+			Key:         "",
+			Err:         "project key cannot be empty",
+		},
+		{
+			Description: "key with special characters",
+			Key:         "!@#$%",
+			Err:         "project key can only contain alphanumeric characters",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.Description, func(t *testing.T) {
+			_, err := api.PatchProject(ctx, &apiv1.PatchProjectRequest{
+				Id: resp.Project.Id,
+				Project: &projectv1.PatchProject{
+					Key: wrapperspb.String(tc.Key),
+				},
+			})
+			require.Error(t, err)
+			require.ErrorContains(t, err, tc.Err)
+		})
+	}
+}

master/internal/project/postgres_project.go

@@ -29,7 +29,7 @@ const (
 	// MaxRetries is the maximum number of retries for transaction conflicts.
 	MaxRetries = 5
 	// ProjectKeyRegex is the regex pattern for a project key.
-	ProjectKeyRegex = "^[A-Z0-9]{1,5}$"
+	ProjectKeyRegex = "^[a-zA-Z0-9]{1,5}$"
 )
 
 // getProjectColumns returns a query with the columns for a project, not including experiment
@@ -171,8 +171,9 @@ func generateProjectKey(ctx context.Context, tx bun.Tx, projectName string) (str
 	var key string
 	found := true
 	for i := 0; i < MaxRetries && found; i++ {
-		prefixLength := min(len(projectName), MaxProjectKeyPrefixLength)
-		prefix := projectName[:prefixLength]
+		sanitizedName := strings.ToUpper(regexp.MustCompile("[^a-zA-Z0-9]").ReplaceAllString(projectName, ""))
+		prefixLength := min(len(sanitizedName), MaxProjectKeyPrefixLength)
+		prefix := sanitizedName[:prefixLength]
 		suffix := random.String(MaxProjectKeyLength - prefixLength)
 		key = strings.ToUpper(prefix + suffix)
 		err := tx.NewSelect().Model(&model.Project{}).Where("key = ?", key).For("UPDATE").Scan(ctx)
@@ -241,7 +242,6 @@ func ValidateProjectKey(key string) error {
 	case len(key) < 1:
 		return errors.New("project key cannot be empty")
 	case !regexp.MustCompile(ProjectKeyRegex).MatchString(key):
-		log.Errorf("project key %s does not match regex %s", key, ProjectKeyRegex)
 		return errors.Errorf(
 			"project key can only contain alphanumeric characters: %s",
 			key,