diff --git a/server/model/perm.go b/server/model/perm.go index 513c5efd0..13bc6a685 100644 --- a/server/model/perm.go +++ b/server/model/perm.go @@ -19,7 +19,6 @@ package model type PermStore interface { PermFind(user *User, repo *Repo) (*Perm, error) PermUpsert(perm *Perm) error - PermBatch(perms []*Perm) error PermDelete(perm *Perm) error PermFlush(user *User, before int64) error } diff --git a/server/shared/userSyncer.go b/server/shared/userSyncer.go index a46bdde6f..4e5949c9c 100644 --- a/server/shared/userSyncer.go +++ b/server/shared/userSyncer.go @@ -16,6 +16,7 @@ package shared import ( "context" + "fmt" "time" "github.com/woodpecker-ci/woodpecker/server/model" @@ -70,24 +71,24 @@ func (s *Syncer) Sync(ctx context.Context, user *model.User) error { return err } - var remoteRepos []*model.Repo - var perms []*model.Perm - + remoteRepos := make([]*model.Repo, 0, len(repos)) for _, repo := range repos { if s.Match(repo) { - remoteRepos = append(remoteRepos, repo) - perm := model.Perm{ + repo.Perm = &model.Perm{ UserID: user.ID, + RepoID: repo.ID, Repo: repo.FullName, - Pull: true, Synced: unix, } remotePerm, err := s.Remote.Perm(ctx, user, repo.Owner, repo.Name) - if err == nil && remotePerm != nil { - perm.Push = remotePerm.Push - perm.Admin = remotePerm.Admin + if err != nil { + return fmt.Errorf("could not fetch permission of repo '%s': %v", repo.FullName, err) } - perms = append(perms, &perm) + repo.Perm.Pull = remotePerm.Pull + repo.Perm.Push = remotePerm.Push + repo.Perm.Admin = remotePerm.Admin + + remoteRepos = append(remoteRepos, repo) } } @@ -96,11 +97,6 @@ func (s *Syncer) Sync(ctx context.Context, user *model.User) error { return err } - err = s.Store.PermBatch(perms) - if err != nil { - return err - } - // this is here as a precaution. I want to make sure that if an api // call to the version control system fails and (for some reason) returns // an empty list, we don't wipe out the user repository permissions. diff --git a/server/store/datastore/feed_test.go b/server/store/datastore/feed_test.go index 42eb592a5..90f1e2d2a 100644 --- a/server/store/datastore/feed_test.go +++ b/server/store/datastore/feed_test.go @@ -55,10 +55,12 @@ func TestRepoListLatest(t *testing.T) { assert.NoError(t, store.CreateRepo(repo2)) assert.NoError(t, store.CreateRepo(repo3)) - assert.NoError(t, store.PermBatch([]*model.Perm{ + for _, perm := range []*model.Perm{ {UserID: user.ID, Repo: repo1.FullName, Push: true, Admin: false}, {UserID: user.ID, Repo: repo2.FullName, Push: true, Admin: true}, - })) + } { + assert.NoError(t, store.PermUpsert(perm)) + } build1 := &model.Build{ RepoID: repo1.ID, diff --git a/server/store/datastore/permission.go b/server/store/datastore/permission.go index 9adfb6fb7..d841e7a55 100644 --- a/server/store/datastore/permission.go +++ b/server/store/datastore/permission.go @@ -17,6 +17,8 @@ package datastore import ( "fmt" + "xorm.io/xorm" + "github.com/woodpecker-ci/woodpecker/server/model" ) @@ -29,19 +31,26 @@ func (s storage) PermFind(user *model.User, repo *model.Repo) (*model.Perm, erro } func (s storage) PermUpsert(perm *model.Perm) error { - if len(perm.Repo) == 0 && perm.RepoID == 0 { - return fmt.Errorf("could not determine repo for permission: %v", perm) - } - sess := s.engine.NewSession() defer sess.Close() if err := sess.Begin(); err != nil { return err } - // lookup repo based on name if possible (preserve old behaviour) - // TODO: check if needed - if len(perm.Repo) != 0 { + if err := s.permUpsert(sess, perm); err != nil { + return err + } + + return sess.Commit() +} + +func (s storage) permUpsert(sess *xorm.Session, perm *model.Perm) error { + if perm.RepoID == 0 && len(perm.Repo) == 0 { + return fmt.Errorf("could not determine repo for permission: %v", perm) + } + + // lookup repo based on name if possible + if perm.RepoID == 0 && len(perm.Repo) != 0 { if r, err := s.getRepoName(sess, perm.Repo); err != nil { return err } else { @@ -62,20 +71,7 @@ func (s storage) PermUpsert(perm *model.Perm) error { // only Insert set auto created ID back to object _, err = sess.Insert(perm) } - if err != nil { - return err - } - - return sess.Commit() -} - -func (s storage) PermBatch(perms []*model.Perm) error { - for i := range perms { - if err := s.PermUpsert(perms[i]); err != nil { - return err - } - } - return nil + return err } func (s storage) PermDelete(perm *model.Perm) error { diff --git a/server/store/datastore/repo.go b/server/store/datastore/repo.go index 1aa8024fb..f88646adf 100644 --- a/server/store/datastore/repo.go +++ b/server/store/datastore/repo.go @@ -74,6 +74,8 @@ func (s storage) RepoList(user *model.User, owned bool) ([]*model.Repo, error) { Find(&repos) } +// RepoBatch Sync batch of repos from SCM (with permissions) to store (create if not exist else update) +// TODO: only store activated repos ... func (s storage) RepoBatch(repos []*model.Repo) error { sess := s.engine.NewSession() defer sess.Close() @@ -86,12 +88,14 @@ func (s storage) RepoBatch(repos []*model.Repo) error { log.Debug().Msgf("skip insert/update repo: %#v", repos[i]) continue } + exist, err := sess. Where("repo_owner = ? AND repo_name = ?", repos[i].Owner, repos[i].Name). Exist(new(model.Repo)) if err != nil { return err } + if exist { if _, err := sess. Where("repo_owner = ? AND repo_name = ?", repos[i].Owner, repos[i].Name). @@ -112,6 +116,14 @@ func (s storage) RepoBatch(repos []*model.Repo) error { return err } } + + if repos[i].Perm != nil { + repos[i].Perm.RepoID = repos[i].ID + repos[i].Perm.Repo = repos[i].FullName + if err := s.permUpsert(sess, repos[i].Perm); err != nil { + return err + } + } } return sess.Commit() diff --git a/server/store/datastore/repo_test.go b/server/store/datastore/repo_test.go index 78e43b174..278037e19 100644 --- a/server/store/datastore/repo_test.go +++ b/server/store/datastore/repo_test.go @@ -16,6 +16,7 @@ package datastore import ( "testing" + "time" "github.com/franela/goblin" "github.com/stretchr/testify/assert" @@ -290,6 +291,13 @@ func TestRepoBatch(t *testing.T) { Owner: "foo", Name: "bar", IsActive: true, + Perm: &model.Perm{ + UserID: 1, + Pull: true, + Push: true, + Admin: true, + Synced: time.Now().Unix(), + }, }, { UserID: 1, @@ -317,16 +325,33 @@ func TestRepoBatch(t *testing.T) { return } + // check DB state + perm, err := store.PermFind(&model.User{ID: 1}, repos[0]) + assert.NoError(t, err) + assert.True(t, perm.Admin) + repo := &model.Repo{ FullName: "foo/bar", Owner: "foo", Name: "bar", + Perm: &model.Perm{ + UserID: 1, + Pull: true, + Push: true, + Admin: false, + Synced: time.Now().Unix(), + }, } assert.NoError(t, store.RepoBatch([]*model.Repo{repo})) assert.EqualValues(t, repos[0].ID, repo.ID) - _, err := store.engine.ID(repo.ID).Get(repo) + + // check current DB state + _, err = store.engine.ID(repo.ID).Get(repo) assert.NoError(t, err) assert.True(t, repo.IsActive) + perm, err = store.PermFind(&model.User{ID: 1}, repos[0]) + assert.NoError(t, err) + assert.False(t, perm.Admin) allRepos := make([]*model.Repo, 0, 4) assert.NoError(t, store.engine.Find(&allRepos)) diff --git a/server/store/datastore/users_test.go b/server/store/datastore/users_test.go index 694ebf2c2..4a5030db7 100644 --- a/server/store/datastore/users_test.go +++ b/server/store/datastore/users_test.go @@ -213,10 +213,12 @@ func TestUsers(t *testing.T) { g.Assert(store.CreateRepo(repo2)).IsNil() g.Assert(store.CreateRepo(repo3)).IsNil() - g.Assert(store.PermBatch([]*model.Perm{ + for _, perm := range []*model.Perm{ {UserID: user.ID, Repo: repo1.FullName, Push: true, Admin: false}, {UserID: user.ID, Repo: repo2.FullName, Push: false, Admin: true}, - })).IsNil() + } { + g.Assert(store.PermUpsert(perm)).IsNil() + } build1 := &model.Build{ RepoID: repo1.ID, diff --git a/server/store/store.go b/server/store/store.go index d4241ccae..b2c843ec4 100644 --- a/server/store/store.go +++ b/server/store/store.go @@ -106,12 +106,11 @@ type Store interface { // RepoList TODO: paginate RepoList(user *model.User, owned bool) ([]*model.Repo, error) RepoListLatest(*model.User) ([]*model.Feed, error) - // RepoBatch TODO: only store activated repos ... + // RepoBatch Sync batch of repos from SCM (with permissions) to store (create if not exist else update) RepoBatch([]*model.Repo) error PermFind(user *model.User, repo *model.Repo) (*model.Perm, error) PermUpsert(perm *model.Perm) error - PermBatch(perms []*model.Perm) error PermDelete(perm *model.Perm) error PermFlush(user *model.User, before int64) error