Harden datastore (#1940)

- only explicit when filters
 - write all more complex filter with builder
 - reuse builder

server/store/datastore/config.go

@@ -30,11 +30,10 @@ func (s storage) ConfigsForPipeline(pipelineID int64) ([]*model.Config, error) {
 }
 
 func (s storage) ConfigFindIdentical(repoID int64, hash string) (*model.Config, error) {
-	conf := &model.Config{
+	conf := new(model.Config)
-		RepoID: repoID,
+	if err := wrapGet(s.engine.Where(
-		Hash:   hash,
+		builder.Eq{"config_repo_id": repoID, "config_hash": hash},
-	}
+	).Get(conf)); err != nil {
-	if err := wrapGet(s.engine.Get(conf)); err != nil {
 		return nil, err
 	}
 	return conf, nil
@@ -43,8 +42,7 @@ func (s storage) ConfigFindIdentical(repoID int64, hash string) (*model.Config,
 func (s storage) ConfigFindApproved(config *model.Config) (bool, error) {
 	return s.engine.Table("pipelines").Select("pipelines.pipeline_id").
 		Join("INNER", "pipeline_config", "pipelines.pipeline_id = pipeline_config.pipeline_id").
-		Where(builder.Eq{"pipelines.pipeline_repo_id": config.RepoID}.
+		Where(builder.Eq{"pipelines.pipeline_repo_id": config.RepoID, "pipeline_config.config_id": config.ID}.
-			And(builder.Eq{"pipeline_config.config_id": config.ID}).
 			And(builder.NotIn("pipelines.pipeline_status", model.StatusBlocked, model.StatusPending))).
 		Exist()
 }

server/store/datastore/cron.go

@@ -29,11 +29,8 @@ func (s storage) CronCreate(cron *model.Cron) error {
 }
 
 func (s storage) CronFind(repo *model.Repo, id int64) (*model.Cron, error) {
-	cron := &model.Cron{
+	cron := new(model.Cron)
-		RepoID: repo.ID,
+	return cron, wrapGet(s.engine.ID(id).Where("repo_id = ?", repo.ID).Get(cron))
-		ID:     id,
-	}
-	return cron, wrapGet(s.engine.Get(cron))
 }
 
 func (s storage) CronList(repo *model.Repo, p *model.ListOptions) ([]*model.Cron, error) {

server/store/datastore/permission.go

@@ -24,11 +24,10 @@ import (
 )
 
 func (s storage) PermFind(user *model.User, repo *model.Repo) (*model.Perm, error) {
-	perm := &model.Perm{
+	perm := new(model.Perm)
-		UserID: user.ID,
+	return perm, wrapGet(s.engine.
-		RepoID: repo.ID,
+		Where(builder.Eq{"perm_user_id": user.ID, "perm_repo_id": repo.ID}).
-	}
+		Get(perm))
-	return perm, wrapGet(s.engine.Get(perm))
 }
 
 func (s storage) PermUpsert(perm *model.Perm) error {
@@ -59,15 +58,13 @@ func (s storage) permUpsert(sess *xorm.Session, perm *model.Perm) error {
 		perm.RepoID = r.ID
 	}
 
-	exist, err := sess.Where("perm_user_id = ? AND perm_repo_id = ?", perm.UserID, perm.RepoID).
+	exist, err := sess.Where(userIDAndRepoIDCond(perm)).Exist(new(model.Perm))
-		Exist(new(model.Perm))
 	if err != nil {
 		return err
 	}
 
 	if exist {
-		_, err = sess.Where("perm_user_id = ? AND perm_repo_id = ?", perm.UserID, perm.RepoID).
+		_, err = sess.Where(userIDAndRepoIDCond(perm)).AllCols().Update(perm)
-			AllCols().Update(perm)
 	} else {
 		// only Insert set auto created ID back to object
 		_, err = sess.Insert(perm)
@@ -76,9 +73,7 @@ func (s storage) permUpsert(sess *xorm.Session, perm *model.Perm) error {
 }
 
 func (s storage) PermDelete(perm *model.Perm) error {
-	return wrapDelete(s.engine.
+	return wrapDelete(s.engine.Where(userIDAndRepoIDCond(perm)).Delete(new(model.Perm)))
-		Where("perm_user_id = ? AND perm_repo_id = ?", perm.UserID, perm.RepoID).
-		Delete(new(model.Perm)))
 }
 
 func (s storage) PermFlush(user *model.User, before int64) error {
@@ -95,3 +90,7 @@ func userPushOrAdminCondition(userID int64) builder.Cond {
 		And(builder.Eq{"perms.perm_push": true}.
 			Or(builder.Eq{"perms.perm_admin": true}))
 }
+
+func userIDAndRepoIDCond(perm *model.Perm) builder.Cond {
+	return builder.Eq{"perm_user_id": perm.UserID, "perm_repo_id": perm.RepoID}
+}

server/store/datastore/pipeline.go

@@ -17,6 +17,7 @@ package datastore
 import (
 	"time"
 
+	"xorm.io/builder"
 	"xorm.io/xorm"
 
 	"github.com/woodpecker-ci/woodpecker/server/model"
@@ -28,47 +29,40 @@ func (s storage) GetPipeline(id int64) (*model.Pipeline, error) {
 }
 
 func (s storage) GetPipelineNumber(repo *model.Repo, num int64) (*model.Pipeline, error) {
-	pipeline := &model.Pipeline{
+	pipeline := new(model.Pipeline)
-		RepoID: repo.ID,
+	return pipeline, wrapGet(s.engine.Where(
-		Number: num,
+		builder.Eq{"pipeline_repo_id": repo.ID, "pipeline_number": num},
-	}
+	).Get(pipeline))
-	return pipeline, wrapGet(s.engine.Get(pipeline))
 }
 
 func (s storage) GetPipelineRef(repo *model.Repo, ref string) (*model.Pipeline, error) {
-	pipeline := &model.Pipeline{
+	pipeline := new(model.Pipeline)
-		RepoID: repo.ID,
+	return pipeline, wrapGet(s.engine.Where(
-		Ref:    ref,
+		builder.Eq{"pipeline_repo_id": repo.ID, "pipeline_ref": ref},
-	}
+	).Get(pipeline))
-	return pipeline, wrapGet(s.engine.Get(pipeline))
 }
 
 func (s storage) GetPipelineCommit(repo *model.Repo, sha, branch string) (*model.Pipeline, error) {
-	pipeline := &model.Pipeline{
+	pipeline := new(model.Pipeline)
-		RepoID: repo.ID,
+	return pipeline, wrapGet(s.engine.Where(
-		Branch: branch,
+		builder.Eq{"pipeline_repo_id": repo.ID, "pipeline_branch": branch, "pipeline_commit": sha},
-		Commit: sha,
+	).Get(pipeline))
-	}
-	return pipeline, wrapGet(s.engine.Get(pipeline))
 }
 
 func (s storage) GetPipelineLast(repo *model.Repo, branch string) (*model.Pipeline, error) {
-	pipeline := &model.Pipeline{
+	pipeline := new(model.Pipeline)
-		RepoID: repo.ID,
+	return pipeline, wrapGet(s.engine.
-		Branch: branch,
+		Desc("pipeline_number").
-		Event:  model.EventPush,
+		Where(builder.Eq{"pipeline_repo_id": repo.ID, "pipeline_branch": branch, "pipeline_event": model.EventPush}).
-	}
+		Get(pipeline))
-	return pipeline, wrapGet(s.engine.Desc("pipeline_number").Get(pipeline))
 }
 
 func (s storage) GetPipelineLastBefore(repo *model.Repo, branch string, num int64) (*model.Pipeline, error) {
-	pipeline := &model.Pipeline{
+	pipeline := new(model.Pipeline)
-		RepoID: repo.ID,
-		Branch: branch,
-	}
 	return pipeline, wrapGet(s.engine.
 		Desc("pipeline_number").
-		Where("pipeline_id < ?", num).
+		Where(builder.Lt{"pipeline_id": num}.
+			And(builder.Eq{"pipeline_repo_id": repo.ID, "pipeline_branch": branch})).
 		Get(pipeline))
 }
 
@@ -111,7 +105,10 @@ func (s storage) CreatePipeline(pipeline *model.Pipeline, stepList ...*model.Ste
 
 	// calc pipeline number
 	var number int64
-	if _, err := sess.SQL("SELECT MAX(pipeline_number) FROM `pipelines` WHERE pipeline_repo_id = ?", pipeline.RepoID).Get(&number); err != nil {
+	if _, err := sess.Select("MAX(pipeline_number)").
+		Table(new(model.Pipeline)).
+		Where("pipeline_repo_id = ?", pipeline.RepoID).
+		Get(&number); err != nil {
 		return err
 	}
 	pipeline.Number = number + 1

server/store/datastore/redirection.go

@@ -15,8 +15,10 @@
 package datastore
 
 import (
-	"github.com/woodpecker-ci/woodpecker/server/model"
+	"xorm.io/builder"
 	"xorm.io/xorm"
+
+	"github.com/woodpecker-ci/woodpecker/server/model"
 )
 
 func (s storage) GetRedirection(fullName string) (*model.Redirection, error) {
@@ -43,5 +45,7 @@ func (s storage) createRedirection(e *xorm.Session, redirect *model.Redirection)
 }
 
 func (s storage) HasRedirectionForRepo(repoID int64, fullName string) (bool, error) {
-	return s.engine.Where("repo_id = ? ", repoID).And("repo_full_name = ?", fullName).Get(new(model.Redirection))
+	return s.engine.Where(
+		builder.Eq{"repo_id": repoID, "repo_full_name": fullName},
+	).Exist(new(model.Redirection))
 }

server/store/datastore/registry.go

@@ -15,15 +15,16 @@
 package datastore
 
 import (
+	"xorm.io/builder"
+
 	"github.com/woodpecker-ci/woodpecker/server/model"
 )
 
 func (s storage) RegistryFind(repo *model.Repo, addr string) (*model.Registry, error) {
-	reg := &model.Registry{
+	reg := new(model.Registry)
-		RepoID:  repo.ID,
+	return reg, wrapGet(s.engine.Where(
-		Address: addr,
+		builder.Eq{"registry_repo_id": repo.ID, "registry_addr": addr},
-	}
+	).Get(reg))
-	return reg, wrapGet(s.engine.Get(reg))
 }
 
 func (s storage) RegistryList(repo *model.Repo, p *model.ListOptions) ([]*model.Registry, error) {

server/store/datastore/secret.go

@@ -23,11 +23,10 @@ import (
 const orderSecretsBy = "secret_name"
 
 func (s storage) SecretFind(repo *model.Repo, name string) (*model.Secret, error) {
-	secret := &model.Secret{
+	secret := new(model.Secret)
-		RepoID: repo.ID,
+	return secret, wrapGet(s.engine.Where(
-		Name:   name,
+		builder.Eq{"secret_repo_id": repo.ID, "secret_name": name},
-	}
+	).Get(secret))
-	return secret, wrapGet(s.engine.Get(secret))
 }
 
 func (s storage) SecretList(repo *model.Repo, includeGlobalAndOrgSecrets bool, p *model.ListOptions) ([]*model.Secret, error) {
@@ -61,11 +60,10 @@ func (s storage) SecretDelete(secret *model.Secret) error {
 }
 
 func (s storage) OrgSecretFind(owner, name string) (*model.Secret, error) {
-	secret := &model.Secret{
+	secret := new(model.Secret)
-		Owner: owner,
+	return secret, wrapGet(s.engine.Where(
-		Name:  name,
+		builder.Eq{"secret_owner": owner, "secret_name": name},
-	}
+	).Get(secret))
-	return secret, wrapGet(s.engine.Get(secret))
 }
 
 func (s storage) OrgSecretList(owner string, p *model.ListOptions) ([]*model.Secret, error) {
@@ -74,13 +72,15 @@ func (s storage) OrgSecretList(owner string, p *model.ListOptions) ([]*model.Sec
 }
 
 func (s storage) GlobalSecretFind(name string) (*model.Secret, error) {
-	secret := &model.Secret{
+	secret := new(model.Secret)
-		Name: name,
+	return secret, wrapGet(s.engine.Where(
-	}
+		builder.Eq{"secret_owner": "", "secret_repo_id": 0, "secret_name": name},
-	return secret, wrapGet(s.engine.Where(builder.And(builder.Eq{"secret_owner": ""}, builder.Eq{"secret_repo_id": 0})).Get(secret))
+	).Get(secret))
 }
 
 func (s storage) GlobalSecretList(p *model.ListOptions) ([]*model.Secret, error) {
 	secrets := make([]*model.Secret, 0)
-	return secrets, s.paginate(p).Where(builder.And(builder.Eq{"secret_owner": ""}, builder.Eq{"secret_repo_id": 0})).OrderBy(orderSecretsBy).Find(&secrets)
+	return secrets, s.paginate(p).Where(
+		builder.Eq{"secret_owner": "", "secret_repo_id": 0},
+	).OrderBy(orderSecretsBy).Find(&secrets)
 }

server/store/datastore/server_config.go

@@ -3,11 +3,8 @@ package datastore
 import "github.com/woodpecker-ci/woodpecker/server/model"
 
 func (s storage) ServerConfigGet(key string) (string, error) {
-	config := &model.ServerConfig{
+	config := new(model.ServerConfig)
-		Key: key,
+	err := wrapGet(s.engine.ID(key).Get(config))
-	}
-
-	err := wrapGet(s.engine.Get(config))
 	if err != nil {
 		return "", err
 	}

server/store/datastore/step.go

@@ -15,6 +15,7 @@
 package datastore
 
 import (
+	"xorm.io/builder"
 	"xorm.io/xorm"
 
 	"github.com/woodpecker-ci/woodpecker/server/model"
@@ -26,25 +27,24 @@ func (s storage) StepLoad(id int64) (*model.Step, error) {
 }
 
 func (s storage) StepFind(pipeline *model.Pipeline, pid int) (*model.Step, error) {
-	step := &model.Step{
+	step := new(model.Step)
-		PipelineID: pipeline.ID,
+	return step, wrapGet(s.engine.Where(
-		PID:        pid,
+		builder.Eq{"step_pipeline_id": pipeline.ID, "step_pid": pid},
-	}
+	).Get(step))
-	return step, wrapGet(s.engine.Get(step))
 }
 
 func (s storage) StepByUUID(uuid string) (*model.Step, error) {
 	step := new(model.Step)
-	return step, wrapGet(s.engine.Where("step_uuid = ?", uuid).Get(step))
+	return step, wrapGet(s.engine.Where(
+		builder.Eq{"step_uuid": uuid},
+	).Get(step))
 }
 
 func (s storage) StepChild(pipeline *model.Pipeline, ppid int, child string) (*model.Step, error) {
-	step := &model.Step{
+	step := new(model.Step)
-		PipelineID: pipeline.ID,
+	return step, wrapGet(s.engine.Where(
-		PPID:       ppid,
+		builder.Eq{"step_pipeline_id": pipeline.ID, "step_ppid": ppid, "step_name": child},
-		Name:       child,
+	).Get(step))
-	}
-	return step, wrapGet(s.engine.Get(step))
 }
 
 func (s storage) StepList(pipeline *model.Pipeline) ([]*model.Step, error) {