Skip to content

Commit

Permalink
fix: add validation to patching project key (ET-305)
Browse files Browse the repository at this point in the history
(cherry picked from commit 86ed69f)
  • Loading branch information
corban-beaird authored and keita-determined committed Jun 25, 2024
1 parent d8b80ad commit eaabab1
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 9 deletions.
12 changes: 11 additions & 1 deletion master/internal/api_project.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
9 changes: 5 additions & 4 deletions master/internal/api_project_intg_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
})
Expand All @@ -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,
})
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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,
Expand Down
8 changes: 4 additions & 4 deletions master/internal/project/postgres_project.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -172,8 +172,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)
Expand Down Expand Up @@ -242,7 +243,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,
Expand Down

0 comments on commit eaabab1

Please sign in to comment.