Merge pull request #12993 from heww/fix-issue-12968

fix(project): change to use user id to query projects of member

src/common/models/project.go

@@ -24,6 +24,7 @@ import (
 	"github.com/astaxie/beego/orm"
 	"github.com/goharbor/harbor/src/pkg/quota/types"
 	"github.com/goharbor/harbor/src/replication/model"
+	"github.com/lib/pq"
 )
 
 const (
@@ -142,8 +143,13 @@ func (p *Project) FilterByPublic(ctx context.Context, qs orm.QuerySeter, key str
 }
 
 // FilterByOwner returns orm.QuerySeter with owner filter
-func (p *Project) FilterByOwner(ctx context.Context, qs orm.QuerySeter, key string, value string) orm.QuerySeter {
+func (p *Project) FilterByOwner(ctx context.Context, qs orm.QuerySeter, key string, value interface{}) orm.QuerySeter {
-	return qs.FilterRaw("owner_id", fmt.Sprintf("IN (SELECT user_id FROM harbor_user WHERE username = '%s')", value))
+	username, ok := value.(string)
+	if !ok {
+		return qs
+	}
+
+	return qs.FilterRaw("owner_id", fmt.Sprintf("IN (SELECT user_id FROM harbor_user WHERE username = %s)", pq.QuoteLiteral(username)))
 }
 
 // FilterByMember returns orm.QuerySeter with member filter
@@ -152,10 +158,9 @@ func (p *Project) FilterByMember(ctx context.Context, qs orm.QuerySeter, key str
 	if !ok {
 		return qs
 	}
-	subQuery := `SELECT project_id FROM project_member pm, harbor_user u WHERE pm.entity_id = u.user_id AND pm.entity_type = 'u' AND u.username = '%s'`
+	subQuery := fmt.Sprintf(`SELECT project_id FROM project_member WHERE entity_id = %d AND entity_type = 'u'`, query.UserID)
-	subQuery = fmt.Sprintf(subQuery, escape(query.Name))
 	if query.Role > 0 {
-		subQuery = fmt.Sprintf("%s AND pm.role = %d", subQuery, query.Role)
+		subQuery = fmt.Sprintf("%s AND role = %d", subQuery, query.Role)
 	}
 
 	if query.WithPublic {
@@ -187,16 +192,6 @@ func isTrue(i interface{}) bool {
 	}
 }
 
-func escape(str string) string {
-	// TODO: remove this function when resolve the cycle import between lib/orm, common/dao and common/models.
-	// We need to move the function to registry the database from common/dao to lib/database,
-	// then lib/orm no need to depend the PrepareTestForPostgresSQL method in the common/dao
-	str = strings.Replace(str, `\`, `\\`, -1)
-	str = strings.Replace(str, `%`, `\%`, -1)
-	str = strings.Replace(str, `_`, `\_`, -1)
-	return str
-}
-
 // ProjectQueryParam can be used to set query parameters when listing projects.
 // The query condition will be set in the query if its corresponding field
 // is not nil. Leave it empty if you don't want to apply this condition.
@@ -220,6 +215,7 @@ type ProjectQueryParam struct {
 
 // MemberQuery filter by member's username and role
 type MemberQuery struct {
+	UserID   int    // the user id
 	Name     string // the username of member
 	Role     int    // the role of the member has to the project
 	GroupIDs []int  // the group ID of current user belongs to

src/lib/orm/query.go

@@ -24,7 +24,6 @@ import (
 	"github.com/astaxie/beego/orm"
 	"github.com/goharbor/harbor/src/common/dao"
 	"github.com/goharbor/harbor/src/lib/errors"
-	"github.com/goharbor/harbor/src/lib/log"
 	"github.com/goharbor/harbor/src/lib/q"
 )
 
@@ -53,7 +52,7 @@ func ParamPlaceholderForIn(n int) string {
 // Currently, it supports two ways to generate the query setter, the first one is to generate by the fields of the model,
 // and the second one is to generate by the methods their name begins with `FilterBy` of the model.
 // e.g. for the following model the queriable fields are  :
-// "Field2", "customized_field2", "Field3", "field3", "Field4" (or "field4") and "Field5" (or "field5").
+// "Field2", "customized_field2", "Field3", "field3" and "Field4" (or "field4").
 // type Foo struct{
 //   Field1 string `orm:"-"`
 //   Field2 string `orm:"column(customized_field2)"`
@@ -64,11 +63,6 @@ func ParamPlaceholderForIn(n int) string {
 //   // The value is the raw value of key in q.Query
 //	 return qs
 // }
-//
-// func (f *Foo) FilterByField5(ctx context.Context, qs orm.QuerySeter, key, value string) orm.QuerySeter {
-//   // The value string is the value of key in q.Query which is escaped by `Escape`
-//	 return qs
-// }
 func QuerySetter(ctx context.Context, model interface{}, query *q.Query, ignoredCols ...string) (orm.QuerySeter, error) {
 	val := reflect.ValueOf(model)
 	if val.Kind() != reflect.Ptr {
@@ -177,11 +171,6 @@ func queryByMethod(ctx context.Context, qs orm.QuerySeter, key string, value int
 		switch method := mv.Interface().(type) {
 		case func(context.Context, orm.QuerySeter, string, interface{}) orm.QuerySeter:
 			return method(ctx, qs, key, value)
-		case func(context.Context, orm.QuerySeter, string, string) orm.QuerySeter:
-			if str, ok := value.(string); ok {
-				return method(ctx, qs, key, Escape(str))
-			}
-			log.Warningf("expected string type for the value of method %s, but actual is %T", methodName, value)
 		default:
 			return qs
 		}

src/pkg/project/dao/dao_test.go

@@ -57,7 +57,7 @@ func (suite *DaoTestSuite) WithUser(f func(int64, string), usernames ...string)
 	suite.Nil(o.Raw(sql, username, username, email).QueryRow(&userID))
 
 	defer func() {
-		o.Raw("UPDATE harbor_user SET deleted=True WHERE user_id = ?", userID).Exec()
+		o.Raw("UPDATE harbor_user SET deleted=True, username=concat_ws('#', username, user_id), email=concat_ws('#', email, user_id) WHERE user_id = ?", userID).Exec()
 	}()
 
 	f(userID, username)
@@ -244,26 +244,62 @@ func (suite *DaoTestSuite) TestListByOwner() {
 		suite.Nil(err)
 		suite.Len(projects, 0)
 	}
+
+	{
+		// single quotes in owner
+		suite.WithUser(func(userID int64, username string) {
+			project := &models.Project{
+				Name:    "project-owner-name-include-single-quotes",
+				OwnerID: int(userID),
+			}
+			projectID, err := suite.dao.Create(orm.Context(), project)
+			suite.Nil(err)
+
+			defer suite.dao.Delete(orm.Context(), projectID)
+
+			projects, err := suite.dao.List(orm.Context(), q.New(q.KeyWords{"owner": username}))
+			suite.Nil(err)
+			suite.Len(projects, 1)
+		}, "owner include single quotes ' in it")
+	}
+
+	{
+		// sql inject
+		suite.WithUser(func(userID int64, username string) {
+			project := &models.Project{
+				Name:    "project-sql-inject",
+				OwnerID: int(userID),
+			}
+			projectID, err := suite.dao.Create(orm.Context(), project)
+			suite.Nil(err)
+
+			defer suite.dao.Delete(orm.Context(), projectID)
+
+			projects, err := suite.dao.List(orm.Context(), q.New(q.KeyWords{"owner": username}))
+			suite.Nil(err)
+			suite.Len(projects, 1)
+		}, "'owner' OR 1=1")
+	}
 }
 
 func (suite *DaoTestSuite) TestListByMember() {
 	{
 		// project admin
-		projects, err := suite.dao.List(orm.Context(), q.New(q.KeyWords{"member": &models.MemberQuery{Name: "admin", Role: common.RoleProjectAdmin}}))
+		projects, err := suite.dao.List(orm.Context(), q.New(q.KeyWords{"member": &models.MemberQuery{UserID: 1, Role: common.RoleProjectAdmin}}))
 		suite.Nil(err)
 		suite.Len(projects, 1)
 	}
 
 	{
 		// guest
-		projects, err := suite.dao.List(orm.Context(), q.New(q.KeyWords{"member": &models.MemberQuery{Name: "admin", Role: common.RoleGuest}}))
+		projects, err := suite.dao.List(orm.Context(), q.New(q.KeyWords{"member": &models.MemberQuery{UserID: 1, Role: common.RoleGuest}}))
 		suite.Nil(err)
 		suite.Len(projects, 0)
 	}
 
 	{
 		// guest with public projects
-		projects, err := suite.dao.List(orm.Context(), q.New(q.KeyWords{"member": &models.MemberQuery{Name: "admin", Role: common.RoleGuest, WithPublic: true}}))
+		projects, err := suite.dao.List(orm.Context(), q.New(q.KeyWords{"member": &models.MemberQuery{UserID: 1, Role: common.RoleGuest, WithPublic: true}}))
 		suite.Nil(err)
 		suite.Len(projects, 1)
 	}
@@ -291,7 +327,7 @@ func (suite *DaoTestSuite) TestListByMember() {
 				defer o.Raw("DELETE FROM project_member WHERE id = ?", pid)
 
 				memberQuery := &models.MemberQuery{
-					Name:     "admin",
+					UserID:   1,
 					Role:     common.RoleProjectAdmin,
 					GroupIDs: []int{int(groupID)},
 				}

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

@@ -375,7 +375,7 @@ func (a *projectAPI) ListProjects(ctx context.Context, params operation.ListProj
 			if l, ok := secCtx.(*local.SecurityContext); ok {
 				currentUser := l.User()
 				member := &project.MemberQuery{
-					Name:     currentUser.Username,
+					UserID:   currentUser.UserID,
 					GroupIDs: currentUser.GroupIDs,
 				}