diff --git a/drone/server/server.go b/drone/server/server.go index 9993e1a2d..25ac5204f 100644 --- a/drone/server/server.go +++ b/drone/server/server.go @@ -400,7 +400,7 @@ func setupEvilGlobals(c *cli.Context, v store.Store) { droneserver.Config.Services.Pubsub.Create(context.Background(), "topic/events") droneserver.Config.Services.Registries = registry.New(v) droneserver.Config.Services.Secrets = secrets.New(v) - droneserver.Config.Services.Senders = sender.New(v) + droneserver.Config.Services.Senders = sender.New(v, v) if endpoint := c.String("registry-service"); endpoint != "" { droneserver.Config.Services.Registries = registry.NewRemote(endpoint) } diff --git a/model/config.go b/model/config.go index 291341781..572947596 100644 --- a/model/config.go +++ b/model/config.go @@ -4,15 +4,14 @@ package model type ConfigStore interface { ConfigLoad(int64) (*Config, error) ConfigFind(*Repo, string) (*Config, error) - ConfigUpdate(*Config) error - ConfigInsert(*Config) error + ConfigFindApproved(*Config) (bool, error) + ConfigCreate(*Config) error } // Config represents a pipeline configuration. type Config struct { - ID int64 `json:"-" meddler:"config_id,pk"` - RepoID int64 `json:"-" meddler:"config_repo_id"` - Data string `json:"data" meddler:"config_data"` - Hash string `json:"hash" meddler:"config_hash"` - Approved bool `json:"approved" meddler:"config_approved"` + ID int64 `json:"-" meddler:"config_id,pk"` + RepoID int64 `json:"-" meddler:"config_repo_id"` + Data string `json:"data" meddler:"config_data"` + Hash string `json:"hash" meddler:"config_hash"` } diff --git a/plugins/sender/builtin.go b/plugins/sender/builtin.go index 835877c90..577c1bf2e 100644 --- a/plugins/sender/builtin.go +++ b/plugins/sender/builtin.go @@ -6,15 +6,23 @@ import ( type builtin struct { store model.SenderStore + conf model.ConfigStore } // New returns a new local gating service. -func New(store model.SenderStore) model.SenderService { - return &builtin{store} +func New(store model.SenderStore, conf model.ConfigStore) model.SenderService { + return &builtin{store, conf} } func (b *builtin) SenderAllowed(user *model.User, repo *model.Repo, build *model.Build, conf *model.Config) (bool, error) { - if !conf.Approved { + if build.Event == model.EventPull && build.Sender != user.Login { + // check to see if the configuration has already been used in an + // existing build. If yes it is considered approved. + if ok, _ := b.conf.ConfigFindApproved(conf); ok { + return true, nil + } + // else check to see if the configuration is sent from a user + // account that is a repositroy approver themselves. sender, err := b.store.SenderFind(repo, build.Sender) if err != nil || sender.Block { return false, nil diff --git a/server/build.go b/server/build.go index b362f2bb5..f2da48e00 100644 --- a/server/build.go +++ b/server/build.go @@ -180,10 +180,6 @@ func PostApproval(c *gin.Context) { c.AbortWithError(404, err) return } - if !conf.Approved { - conf.Approved = true - Config.Storage.Config.ConfigUpdate(conf) - } netrc, err := remote_.Netrc(user, repo) if err != nil { @@ -404,10 +400,6 @@ func PostBuild(c *gin.Context) { c.AbortWithError(404, err) return } - if !conf.Approved { - conf.Approved = true - Config.Storage.Config.ConfigUpdate(conf) - } netrc, err := remote_.Netrc(user, repo) if err != nil { diff --git a/server/hook.go b/server/hook.go index e8fb52aa9..73cfa7474 100644 --- a/server/hook.go +++ b/server/hook.go @@ -142,27 +142,17 @@ func PostHook(c *gin.Context) { conf, err := Config.Storage.Config.ConfigFind(repo, sha) if err != nil { conf = &model.Config{ - RepoID: repo.ID, - Data: string(confb), - Hash: sha, - Approved: false, + RepoID: repo.ID, + Data: string(confb), + Hash: sha, } - if user.Login == repo.Owner || build.Event != model.EventPull || repo.IsGated == false { - conf.Approved = true - } - err = Config.Storage.Config.ConfigInsert(conf) + err = Config.Storage.Config.ConfigCreate(conf) if err != nil { logrus.Errorf("failure to persist config for %s. %s", repo.FullName, err) c.AbortWithError(500, err) return } } - if !conf.Approved { - if user.Login == repo.Owner || build.Event != model.EventPull || repo.IsGated == false { - conf.Approved = true - Config.Storage.Config.ConfigUpdate(conf) - } - } build.ConfigID = conf.ID netrc, err := remote_.Netrc(user, repo) diff --git a/store/datastore/config.go b/store/datastore/config.go index ad4309249..ca6d6f24a 100644 --- a/store/datastore/config.go +++ b/store/datastore/config.go @@ -1,13 +1,15 @@ package datastore import ( + gosql "database/sql" + "github.com/drone/drone/model" "github.com/drone/drone/store/datastore/sql" "github.com/russross/meddler" ) func (db *datastore) ConfigLoad(id int64) (*model.Config, error) { - stmt := sql.Lookup(db.driver, "config-find-repo-id") + stmt := sql.Lookup(db.driver, "config-find-id") conf := new(model.Config) err := meddler.QueryRow(db, conf, stmt, id) return conf, err @@ -20,10 +22,18 @@ func (db *datastore) ConfigFind(repo *model.Repo, hash string) (*model.Config, e return conf, err } -func (db *datastore) ConfigUpdate(config *model.Config) error { - return meddler.Update(db, "config", config) +func (db *datastore) ConfigFindApproved(config *model.Config) (bool, error) { + var dest int64 + stmt := sql.Lookup(db.driver, "config-find-approved") + err := db.DB.QueryRow(stmt, config.RepoID, config.ID).Scan(&dest) + if err == gosql.ErrNoRows { + return false, nil + } else if err != nil { + return false, err + } + return true, nil } -func (db *datastore) ConfigInsert(config *model.Config) error { +func (db *datastore) ConfigCreate(config *model.Config) error { return meddler.Insert(db, "config", config) } diff --git a/store/datastore/config_test.go b/store/datastore/config_test.go index bc1082c02..9586fed0a 100644 --- a/store/datastore/config_test.go +++ b/store/datastore/config_test.go @@ -18,12 +18,11 @@ func TestConfig(t *testing.T) { hash = "8d8647c9aa90d893bfb79dddbe901f03e258588121e5202632f8ae5738590b26" ) - if err := s.ConfigInsert( + if err := s.ConfigCreate( &model.Config{ - RepoID: 2, - Data: data, - Hash: hash, - Approved: false, + RepoID: 2, + Data: data, + Hash: hash, }, ); err != nil { t.Errorf("Unexpected error: insert config: %s", err) @@ -47,60 +46,102 @@ func TestConfig(t *testing.T) { if got, want := config.Hash, hash; got != want { t.Errorf("Want config hash %s, got %s", want, got) } - if got, want := config.Approved, false; got != want { - t.Errorf("Want config approved %v, got %v", want, got) - } - config.Approved = true - err = s.ConfigUpdate(config) + loaded, err := s.ConfigLoad(config.ID) if err != nil { - t.Errorf("Want config updated, got error %q", err) + t.Errorf("Want config by id, got error %q", err) return } - - updated, err := s.ConfigFind(&model.Repo{ID: 2}, hash) - if err != nil { - t.Errorf("Want config find, got error %q", err) - return - } - if got, want := updated.Approved, true; got != want { - t.Errorf("Want config approved updated %v, got %v", want, got) + if got, want := loaded.ID, config.ID; got != want { + t.Errorf("Want config by id %d, got %d", want, got) } } -// -// func TestConfigIndexes(t *testing.T) { -// s := newTest() -// defer func() { -// s.Exec("delete from config") -// s.Close() -// }() -// -// if err := s.FileCreate( -// &model.File{ -// BuildID: 1, -// ProcID: 1, -// Name: "hello.txt", -// Size: 11, -// Mime: "text/plain", -// }, -// bytes.NewBufferString("hello world"), -// ); err != nil { -// t.Errorf("Unexpected error: insert file: %s", err) -// return -// } -// -// // fail due to duplicate file name -// if err := s.FileCreate( -// &model.File{ -// BuildID: 1, -// ProcID: 1, -// Name: "hello.txt", -// Mime: "text/plain", -// Size: 11, -// }, -// bytes.NewBufferString("hello world"), -// ); err == nil { -// t.Errorf("Unexpected error: dupliate pid") -// } -// } +func TestConfigApproved(t *testing.T) { + s := newTest() + defer func() { + s.Exec("delete from config") + s.Exec("delete from builds") + s.Close() + }() + + var ( + data = "pipeline: [ { image: golang, commands: [ go build, go test ] } ]" + hash = "8d8647c9aa90d893bfb79dddbe901f03e258588121e5202632f8ae5738590b26" + conf = &model.Config{ + RepoID: 1, + Data: data, + Hash: hash, + } + ) + + if err := s.ConfigCreate(conf); err != nil { + t.Errorf("Unexpected error: insert config: %s", err) + return + } + + s.CreateBuild(&model.Build{ + RepoID: 1, + ConfigID: conf.ID, + Status: model.StatusBlocked, + Commit: "85f8c029b902ed9400bc600bac301a0aadb144ac", + }) + s.CreateBuild(&model.Build{ + RepoID: 1, + ConfigID: conf.ID, + Status: model.StatusPending, + Commit: "85f8c029b902ed9400bc600bac301a0aadb144ac", + }) + + if ok, _ := s.ConfigFindApproved(conf); ok == true { + t.Errorf("Want config not approved, when blocked or pending") + return + } + + s.CreateBuild(&model.Build{ + RepoID: 1, + ConfigID: conf.ID, + Status: model.StatusRunning, + Commit: "85f8c029b902ed9400bc600bac301a0aadb144ac", + }) + + if ok, _ := s.ConfigFindApproved(conf); ok == false { + t.Errorf("Want config approved, when running.") + return + } +} + +func TestConfigIndexes(t *testing.T) { + s := newTest() + defer func() { + s.Exec("delete from config") + s.Close() + }() + + var ( + data = "pipeline: [ { image: golang, commands: [ go build, go test ] } ]" + hash = "8d8647c9aa90d893bfb79dddbe901f03e258588121e5202632f8ae5738590b26" + ) + + if err := s.ConfigCreate( + &model.Config{ + RepoID: 2, + Data: data, + Hash: hash, + }, + ); err != nil { + t.Errorf("Unexpected error: insert config: %s", err) + return + } + + // fail due to duplicate sha + if err := s.ConfigCreate( + &model.Config{ + RepoID: 2, + Data: data, + Hash: hash, + }, + ); err == nil { + t.Errorf("Unexpected error: dupliate sha") + } +} diff --git a/store/datastore/ddl/mysql/16.sql b/store/datastore/ddl/mysql/16.sql index 8a29b1fb1..d1a660d80 100644 --- a/store/datastore/ddl/mysql/16.sql +++ b/store/datastore/ddl/mysql/16.sql @@ -5,7 +5,6 @@ CREATE TABLE config ( ,config_repo_id INTEGER ,config_hash VARCHAR(250) ,config_data MEDIUMBLOB -,config_approved BOOLEAN ,UNIQUE(config_hash, config_repo_id) ); diff --git a/store/datastore/ddl/postgres/16.sql b/store/datastore/ddl/postgres/16.sql index e8ed77ef9..63dbf86a0 100644 --- a/store/datastore/ddl/postgres/16.sql +++ b/store/datastore/ddl/postgres/16.sql @@ -5,7 +5,6 @@ CREATE TABLE config ( ,config_repo_id INTEGER ,config_hash VARCHAR(250) ,config_data BYTEA -,config_approved BOOLEAN ,UNIQUE(config_hash, config_repo_id) ); diff --git a/store/datastore/ddl/sqlite3/16.sql b/store/datastore/ddl/sqlite3/16.sql index 88351bf70..fc6129f49 100644 --- a/store/datastore/ddl/sqlite3/16.sql +++ b/store/datastore/ddl/sqlite3/16.sql @@ -5,7 +5,6 @@ CREATE TABLE config ( ,config_repo_id INTEGER ,config_hash TEXT ,config_data BLOB -,config_approved BOOLEAN ,UNIQUE(config_hash, config_repo_id) ); diff --git a/store/datastore/sql/postgres/files/config.sql b/store/datastore/sql/postgres/files/config.sql index 0385c27ce..2dc954384 100644 --- a/store/datastore/sql/postgres/files/config.sql +++ b/store/datastore/sql/postgres/files/config.sql @@ -5,7 +5,6 @@ SELECT ,config_repo_id ,config_hash ,config_data -,config_approved FROM config WHERE config_id = $1 @@ -16,7 +15,14 @@ SELECT ,config_repo_id ,config_hash ,config_data -,config_approved FROM config WHERE config_repo_id = $1 AND config_hash = $2 + +-- name: config-find-approved + +SELECT build_id FROM builds +WHERE build_repo_id = $1 +AND build_config_id = $2 +AND build_status NOT IN ('blocked', 'pending') +LIMIT 1 diff --git a/store/datastore/sql/postgres/sql_gen.go b/store/datastore/sql/postgres/sql_gen.go index c27fa554f..24f37a610 100644 --- a/store/datastore/sql/postgres/sql_gen.go +++ b/store/datastore/sql/postgres/sql_gen.go @@ -8,6 +8,7 @@ func Lookup(name string) string { var index = map[string]string{ "config-find-id": configFindId, "config-find-repo-hash": configFindRepoHash, + "config-find-approved": configFindApproved, "count-users": countUsers, "count-repos": countRepos, "count-builds": countBuilds, @@ -41,7 +42,6 @@ SELECT ,config_repo_id ,config_hash ,config_data -,config_approved FROM config WHERE config_id = $1 ` @@ -52,12 +52,19 @@ SELECT ,config_repo_id ,config_hash ,config_data -,config_approved FROM config WHERE config_repo_id = $1 AND config_hash = $2 ` +var configFindApproved = ` +SELECT build_id FROM builds +WHERE build_repo_id = $1 +AND build_config_id = $2 +AND build_status NOT IN ('blocked', 'pending') +LIMIT 1 +` + var countUsers = ` SELECT reltuples FROM pg_class WHERE relname = 'users'; diff --git a/store/datastore/sql/sqlite/files/config.sql b/store/datastore/sql/sqlite/files/config.sql index dd07a4f1a..8f29bd5d1 100644 --- a/store/datastore/sql/sqlite/files/config.sql +++ b/store/datastore/sql/sqlite/files/config.sql @@ -5,7 +5,6 @@ SELECT ,config_repo_id ,config_hash ,config_data -,config_approved FROM config WHERE config_id = ? @@ -16,7 +15,14 @@ SELECT ,config_repo_id ,config_hash ,config_data -,config_approved FROM config WHERE config_repo_id = ? AND config_hash = ? + +-- name: config-find-approved + +SELECT build_id FROM builds +WHERE build_repo_id = ? +AND build_config_id = ? +AND build_status NOT IN ('blocked', 'pending') +LIMIT 1 diff --git a/store/datastore/sql/sqlite/sql_gen.go b/store/datastore/sql/sqlite/sql_gen.go index 15cc0e7b6..1a06db44e 100644 --- a/store/datastore/sql/sqlite/sql_gen.go +++ b/store/datastore/sql/sqlite/sql_gen.go @@ -8,6 +8,7 @@ func Lookup(name string) string { var index = map[string]string{ "config-find-id": configFindId, "config-find-repo-hash": configFindRepoHash, + "config-find-approved": configFindApproved, "count-users": countUsers, "count-repos": countRepos, "count-builds": countBuilds, @@ -41,7 +42,6 @@ SELECT ,config_repo_id ,config_hash ,config_data -,config_approved FROM config WHERE config_id = ? ` @@ -52,12 +52,19 @@ SELECT ,config_repo_id ,config_hash ,config_data -,config_approved FROM config WHERE config_repo_id = ? AND config_hash = ? ` +var configFindApproved = ` +SELECT build_id FROM builds +WHERE build_repo_id = ? +AND build_config_id = ? +AND build_status NOT IN ('blocked', 'pending') +LIMIT 1 +` + var countUsers = ` SELECT count(1) FROM users diff --git a/store/store.go b/store/store.go index 18a177822..1949bf823 100644 --- a/store/store.go +++ b/store/store.go @@ -94,8 +94,8 @@ type Store interface { ConfigLoad(int64) (*model.Config, error) ConfigFind(*model.Repo, string) (*model.Config, error) - ConfigUpdate(*model.Config) error - ConfigInsert(*model.Config) error + ConfigFindApproved(*model.Config) (bool, error) + ConfigCreate(*model.Config) error SenderFind(*model.Repo, string) (*model.Sender, error) SenderList(*model.Repo) ([]*model.Sender, error)