Merge pull request #12445 from chlins/fix/preheat-instance-and-policy-name-validation

fix(preheat): validate instance/policy name and set unique name

make/migrations/postgresql/0040_2.1.0_schema.up.sql

@@ -49,7 +49,8 @@ CREATE TABLE p2p_preheat_instance (
   enabled     boolean,
   is_default  boolean,
   insecure    boolean,
-  setup_timestamp int
+  setup_timestamp int,
+  UNIQUE (name)
 );
 
 CREATE TABLE IF NOT EXISTS p2p_preheat_policy (
@@ -62,5 +63,6 @@ CREATE TABLE IF NOT EXISTS p2p_preheat_policy (
     trigger varchar(16),
     enabled boolean,
     creation_time timestamp,
-    update_time timestamp
+    update_time timestamp,
+    UNIQUE (name, project_id)
 );

src/pkg/p2p/preheat/dao/instance/dao.go

@@ -38,12 +38,21 @@ type dao struct{}
 var _ DAO = (*dao)(nil)
 
 // Create adds a new distribution instance.
-func (d *dao) Create(ctx context.Context, instance *provider.Instance) (int64, error) {
+func (d *dao) Create(ctx context.Context, instance *provider.Instance) (id int64, err error) {
-	var o, err = orm.FromContext(ctx)
+	var o beego_orm.Ormer
+	o, err = orm.FromContext(ctx)
 	if err != nil {
-		return 0, err
+		return
 	}
-	return o.Insert(instance)
+
+	id, err = o.Insert(instance)
+	if err != nil {
+		if e := orm.AsConflictError(err, "instance %s already exists", instance.Name); e != nil {
+			err = e
+		}
+		return
+	}
+	return
 }
 
 // Get gets instance from db by id.

src/pkg/p2p/preheat/dao/instance/dao_test.go

@@ -64,6 +64,15 @@ func (is *instanceSuite) TestGet() {
 	assert.Nil(t, i)
 }
 
+// TestCreate tests create instance.
+func (is *instanceSuite) TestCreate() {
+	// test create same name instance, should error
+	sameNameInstance := *defaultInstance
+	sameNameInstance.ID = 1000
+	_, err := is.dao.Create(is.ctx, &sameNameInstance)
+	is.True(errors.IsConflictErr(err))
+}
+
 // TestGetByName tests get a instance by name.
 func (is *instanceSuite) TestGetByName() {
 	instance, err := is.dao.GetByName(is.ctx, defaultInstance.Name)

src/pkg/p2p/preheat/dao/policy/dao_test.go

@@ -84,6 +84,22 @@ func (d *daoTestSuite) TestCreate() {
 	_, err := d.dao.Create(d.ctx, d.defaultPolicy)
 	d.Require().NotNil(err)
 	d.True(errors.IsErr(err, errors.ConflictCode))
+
+	// same name and project id should error
+	sameNamePolicy := *d.defaultPolicy
+	sameNamePolicy.ID = 1000
+	_, err = d.dao.Create(d.ctx, &sameNamePolicy)
+	d.Require().NotNil(err)
+	d.True(errors.IsErr(err, errors.ConflictCode))
+
+	// same name but different project id should not error
+	sameNamePolicyWithDiffProjectID := sameNamePolicy
+	sameNamePolicyWithDiffProjectID.ProjectID = 10
+	_, err = d.dao.Create(d.ctx, &sameNamePolicyWithDiffProjectID)
+	d.Require().Nil(err)
+	// clean
+	err = d.dao.Delete(d.ctx, sameNamePolicyWithDiffProjectID.ID)
+	d.Require().Nil(err)
 }
 
 // Delete tests delete a policy schema.

src/pkg/p2p/preheat/policy/manager.go

@@ -16,7 +16,9 @@ package policy
 
 import (
 	"context"
+	"encoding/json"
 
+	"github.com/goharbor/harbor/src/lib/errors"
 	"github.com/goharbor/harbor/src/lib/q"
 	dao "github.com/goharbor/harbor/src/pkg/p2p/preheat/dao/policy"
 	"github.com/goharbor/harbor/src/pkg/p2p/preheat/models/policy"
@@ -73,12 +75,22 @@ func (m *manager) Update(ctx context.Context, schema *policy.Schema, props ...st
 
 // Get the policy schema by id
 func (m *manager) Get(ctx context.Context, id int64) (schema *policy.Schema, err error) {
-	return m.dao.Get(ctx, id)
+	schema, err = m.dao.Get(ctx, id)
+	if err != nil {
+		return nil, err
+	}
+
+	return parsePolicy(schema)
 }
 
 // Get the policy schema by name
 func (m *manager) GetByName(ctx context.Context, projectID int64, name string) (schema *policy.Schema, err error) {
-	return m.dao.GetByName(ctx, projectID, name)
+	schema, err = m.dao.GetByName(ctx, projectID, name)
+	if err != nil {
+		return nil, err
+	}
+
+	return parsePolicy(schema)
 }
 
 // Delete the policy schema by id
@@ -88,7 +100,20 @@ func (m *manager) Delete(ctx context.Context, id int64) (err error) {
 
 // List policy schemas by query
 func (m *manager) ListPolicies(ctx context.Context, query *q.Query) (schemas []*policy.Schema, err error) {
-	return m.dao.List(ctx, query)
+	schemas, err = m.dao.List(ctx, query)
+	if err != nil {
+		return nil, err
+	}
+
+	for i := range schemas {
+		schema, err := parsePolicy(schemas[i])
+		if err != nil {
+			return nil, err
+		}
+		schemas[i] = schema
+	}
+
+	return schemas, nil
 }
 
 // list policy schema under project
@@ -103,5 +128,56 @@ func (m *manager) ListPoliciesByProject(ctx context.Context, project int64, quer
 	// set project filter
 	query.Keywords["project_id"] = project
 
-	return m.dao.List(ctx, query)
+	return m.ListPolicies(ctx, query)
+}
+
+// parsePolicy parse policy model.
+func parsePolicy(schema *policy.Schema) (*policy.Schema, error) {
+	if schema == nil {
+		return nil, errors.New("policy schema can not be nil")
+	}
+
+	// parse filters
+	filters, err := parseFilters(schema.FiltersStr)
+	if err != nil {
+		return nil, err
+	}
+	schema.Filters = filters
+
+	// parse trigger
+	trigger, err := parseTrigger(schema.TriggerStr)
+	if err != nil {
+		return nil, err
+	}
+	schema.Trigger = trigger
+
+	return schema, nil
+}
+
+// parseFilters parse filterStr to filter.
+func parseFilters(filterStr string) ([]*policy.Filter, error) {
+	if len(filterStr) == 0 {
+		return nil, nil
+	}
+
+	var filters []*policy.Filter
+	if err := json.Unmarshal([]byte(filterStr), &filters); err != nil {
+		return nil, err
+	}
+
+	return filters, nil
+}
+
+// parseTrigger parse triggerStr to trigger.
+func parseTrigger(triggerStr string) (*policy.Trigger, error) {
+	if len(triggerStr) == 0 {
+		return nil, nil
+	}
+
+	trigger := &policy.Trigger{}
+	if err := json.Unmarshal([]byte(triggerStr), trigger); err != nil {
+		return nil, err
+	}
+
+	return trigger, nil
 }

src/pkg/p2p/preheat/policy/manager_test.go

@@ -115,14 +115,14 @@ func (m *managerTestSuite) TestUpdate() {
 
 // TestGet tests Get method.
 func (m *managerTestSuite) TestGet() {
-	m.dao.On("Get").Return(nil, nil)
+	m.dao.On("Get").Return(&policy.Schema{}, nil)
 	_, err := m.mgr.Get(nil, 1)
 	m.Require().Nil(err)
 }
 
 // TestGetByName tests Get method.
 func (m *managerTestSuite) TestGetByName() {
-	m.dao.On("GetByName").Return(nil, nil)
+	m.dao.On("GetByName").Return(&policy.Schema{}, nil)
 	_, err := m.mgr.Get(nil, 1)
 	m.Require().Nil(err)
 }
@@ -136,14 +136,62 @@ func (m *managerTestSuite) TestDelete() {
 
 // TestListPolicies tests ListPolicies method.
 func (m *managerTestSuite) TestListPolicies() {
-	m.dao.On("List").Return(nil, nil)
+	m.dao.On("List").Return([]*policy.Schema{}, nil)
 	_, err := m.mgr.ListPolicies(nil, nil)
 	m.Require().Nil(err)
 }
 
 // TestListPoliciesByProject tests ListPoliciesByProject method.
 func (m *managerTestSuite) TestListPoliciesByProject() {
-	m.dao.On("List").Return(nil, nil)
+	m.dao.On("List").Return([]*policy.Schema{}, nil)
 	_, err := m.mgr.ListPoliciesByProject(nil, 1, nil)
 	m.Require().Nil(err)
 }
+
+// TestParsePolicy tests parsePolicy.
+func (m *managerTestSuite) TestParsePolicy() {
+	schema := &policy.Schema{FiltersStr: "invalid"}
+	_, err := parsePolicy(schema)
+	m.Require().Error(err)
+
+	schema = &policy.Schema{TriggerStr: "invalid"}
+	_, err = parsePolicy(schema)
+	m.Require().Error(err)
+
+	schema = &policy.Schema{
+		FiltersStr: `[
+    {
+        "type": "repository",
+        "value": "dev**"
+    },
+    {
+        "type": "tag",
+        "value": "v1*"
+    },
+    {
+        "type": "label",
+        "value": [
+            "lb1",
+            "lb2"
+        ]
+    },
+    {
+        "type": "signature",
+        "value": true
+    }
+]`,
+		TriggerStr: `{
+    "type": "scheduled",
+    "trigger_setting": {
+        "cron": "0 0 2 1 * ? *"
+    }
+}`,
+	}
+	schema, err = parsePolicy(schema)
+	m.Require().NoError(err)
+	m.Require().NotNil(schema.Trigger)
+	m.Require().Equal("0 0 2 1 * ? *", schema.Trigger.Settings.Cron)
+	m.Require().NotNil(schema.Filters)
+	m.Require().Len(schema.Filters, 4)
+	m.Require().True(schema.Filters[3].Value.(bool))
+}

src/server/v2.0/handler/preheat.go

@@ -4,6 +4,8 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"fmt"
+	"regexp"
 	"time"
 
 	"github.com/go-openapi/runtime/middleware"
@@ -27,6 +29,9 @@ func newPreheatAPI() *preheatAPI {
 
 var _ restapi.PreheatAPI = (*preheatAPI)(nil)
 
+// nameRegex is the regex for name validation.
+const nameRegex = "^[A-Za-z0-9]+(?:[._-][A-Za-z0-9]+)*$"
+
 type preheatAPI struct {
 	BaseAPI
 	preheatCtl preheatCtl.Controller
@@ -274,6 +279,15 @@ func convertParamPolicyToModelPolicy(model *models.PreheatPolicy) (*policy.Schem
 		return nil, errors.New("policy can not be nil")
 	}
 
+	valid, err := regexp.MatchString(nameRegex, model.Name)
+	if err != nil {
+		return nil, err
+	}
+
+	if !valid {
+		return nil, fmt.Errorf("name %s is invalid", model.Name)
+	}
+
 	return &policy.Schema{
 		ID:          model.ID,
 		Name:        model.Name,
@@ -319,7 +333,16 @@ func convertParamInstanceToModelInstance(model *models.Instance) (*instanceModel
 		return nil, errors.New("instance can not be nil")
 	}
 
-	var authData, err = json.Marshal(model.AuthInfo)
+	valid, err := regexp.MatchString(nameRegex, model.Name)
+	if err != nil {
+		return nil, err
+	}
+
+	if !valid {
+		return nil, fmt.Errorf("name %s is invalid", model.Name)
+	}
+
+	authData, err := json.Marshal(model.AuthInfo)
 	if err != nil {
 		return nil, err
 	}

src/server/v2.0/handler/preheat_test.go

@@ -107,6 +107,12 @@ func Test_convertParamPolicyToModelPolicy(t *testing.T) {
 			expect:    nil,
 			shouldErr: true,
 		},
+		{
+			name:      "invalid name",
+			input:     &models.PreheatPolicy{Name: "abc/-.**"},
+			expect:    nil,
+			shouldErr: true,
+		},
 		{
 			name: "should success",
 			input: &models.PreheatPolicy{
@@ -231,6 +237,12 @@ func Test_convertParamInstanceToModelInstance(t *testing.T) {
 			want:    nil,
 			wantErr: true,
 		},
+		{
+			name:    "invalid name",
+			input:   &models.Instance{Name: "_aa/*b"},
+			want:    nil,
+			wantErr: true,
+		},
 		{
 			name: "want ok",
 			input: &models.Instance{