refactor: simplify logic with delete not in behaviour

Author: henkka

server/api/login.go

@@ -314,7 +314,7 @@ func updateRepoPermissions(c *gin.Context, user *model.User, _store store.Store,
 		return err
 	}
 
-	allowedRepoIDs := map[int64]bool{}
+	repoIDs := make([]int64, 0)
 
 	for _, forgeRepo := range repos {
 		// make sure forgeID is set
@@ -342,27 +342,13 @@ func updateRepoPermissions(c *gin.Context, user *model.User, _store store.Store,
 			return err
 		}
 
-		allowedRepoIDs[dbRepo.ID] = true
+		repoIDs = append(repoIDs, dbRepo.ID)
 	}
 
-	currentRepos, err := _store.RepoList(user, false, true, nil)
-	if err != nil {
+	if err := _store.PermPrune(user.ID, repoIDs); err != nil {
 		return err
 	}
 
-	toDelete := make([]int64, 0)
-	for _, r := range currentRepos {
-		if !allowedRepoIDs[r.ID] {
-			toDelete = append(toDelete, r.ID)
-		}
-	}
-
-	if len(toDelete) > 0 {
-		if err := _store.PermDeleteByUserAndRepoIDs(user.ID, toDelete); err != nil {
-			return err
-		}
-	}
-
 	return nil
 }
 

server/api/login_test.go

@@ -164,7 +164,7 @@ func TestHandleAuth(t *testing.T) {
 		_store.On("OrgFindByName", user.Login, user.ForgeID).Return(nil, nil)
 		_store.On("OrgCreate", mock.Anything).Return(nil)
 		_store.On("UpdateUser", mock.Anything).Return(nil)
-		_store.On("RepoList", mock.Anything, false, true, (*model.RepoFilter)(nil)).Return([]*model.Repo{}, nil)
+		_store.On("PermPrune", mock.Anything, []int64{}).Return(nil)
 		_forge.On("Repos", mock.Anything, mock.Anything, mock.Anything).Return(nil, nil)
 
 		api.HandleAuth(c)
@@ -197,7 +197,7 @@ func TestHandleAuth(t *testing.T) {
 		_store.On("GetUserByRemoteID", user.ForgeID, user.ForgeRemoteID).Return(user, nil)
 		_store.On("OrgGet", org.ID).Return(org, nil)
 		_store.On("UpdateUser", mock.Anything).Return(nil)
-		_store.On("RepoList", mock.Anything, false, true, (*model.RepoFilter)(nil)).Return([]*model.Repo{}, nil)
+		_store.On("PermPrune", mock.Anything, []int64{}).Return(nil)
 		_forge.On("Repos", mock.Anything, mock.Anything, mock.Anything).Return(nil, nil)
 
 		api.HandleAuth(c)
@@ -293,7 +293,7 @@ func TestHandleAuth(t *testing.T) {
 		_store.On("OrgFindByName", user.Login, user.ForgeID).Return(nil, types.RecordNotExist)
 		_store.On("OrgCreate", mock.Anything).Return(nil)
 		_store.On("UpdateUser", mock.Anything).Return(nil)
-		_store.On("RepoList", mock.Anything, false, true, (*model.RepoFilter)(nil)).Return([]*model.Repo{}, nil)
+		_store.On("PermPrune", mock.Anything, []int64{}).Return(nil)
 		_forge.On("Repos", mock.Anything, mock.Anything, mock.Anything).Return(nil, nil)
 
 		api.HandleAuth(c)
@@ -328,7 +328,7 @@ func TestHandleAuth(t *testing.T) {
 		_store.On("OrgFindByName", user.Login, user.ForgeID).Return(org, nil)
 		_store.On("OrgUpdate", mock.Anything).Return(nil)
 		_store.On("UpdateUser", mock.Anything).Return(nil)
-		_store.On("RepoList", mock.Anything, false, true, (*model.RepoFilter)(nil)).Return([]*model.Repo{}, nil)
+		_store.On("PermPrune", mock.Anything, []int64{}).Return(nil)
 		_forge.On("Repos", mock.Anything, mock.Anything, mock.Anything).Return(nil, nil)
 
 		api.HandleAuth(c)
@@ -363,7 +363,7 @@ func TestHandleAuth(t *testing.T) {
 		_store.On("OrgGet", user.OrgID).Return(org, nil)
 		_store.On("OrgUpdate", mock.Anything).Return(nil)
 		_store.On("UpdateUser", mock.Anything).Return(nil)
-		_store.On("RepoList", mock.Anything, false, true, (*model.RepoFilter)(nil)).Return([]*model.Repo{}, nil)
+		_store.On("PermPrune", mock.Anything, []int64{}).Return(nil)
 		_forge.On("Repos", mock.Anything, mock.Anything, mock.Anything).Return(nil, nil)
 
 		api.HandleAuth(c)
@@ -398,11 +398,7 @@ func TestHandleAuth(t *testing.T) {
 		_store.On("UpdateUser", mock.Anything).Return(nil)
 
 		_forge.On("Repos", mock.Anything, mock.Anything, mock.Anything).Return([]*model.Repo{}, nil)
-
-		dbRepo := &model.Repo{ID: 42, FullName: "woodpecker-ci/woodpecker", IsActive: true}
-		_store.On("RepoList", mock.Anything, false, true, (*model.RepoFilter)(nil)).Return([]*model.Repo{dbRepo}, nil)
-
-		_store.On("PermDeleteByUserAndRepoIDs", user.ID, []int64{dbRepo.ID}).Return(nil)
+		_store.On("PermPrune", mock.Anything, []int64{}).Return(nil)
 
 		api.HandleAuth(c)
 

server/store/datastore/permission.go

@@ -84,11 +84,17 @@ func userIDAndRepoIDCond(perm *model.Perm) builder.Cond {
 	return builder.Eq{"user_id": perm.UserID, "repo_id": perm.RepoID}
 }
 
-// PermDeleteByUserAndRepoIDs deletes permission rows for a user for the given repo IDs.
-func (s storage) PermDeleteByUserAndRepoIDs(userID int64, repoIDs []int64) error {
-	if len(repoIDs) == 0 {
-		return nil
+// PermPrune deletes all permission rows for a user
+// where the repo_id is NOT IN the provided keepRepoIDs list. If keepRepoIDs
+// is empty, all permissions for the user are deleted.
+func (s storage) PermPrune(userID int64, keepRepoIDs []int64) error {
+	if len(keepRepoIDs) == 0 {
+		_, err := s.engine.Where(builder.Eq{"user_id": userID}).Delete(new(model.Perm))
+		return err
 	}
-	_, err := s.engine.In("repo_id", repoIDs).And("user_id = ?", userID).Delete(new(model.Perm))
+
+	_, err := s.engine.Where(builder.Eq{"user_id": userID}).
+		And(builder.NotIn("repo_id", keepRepoIDs)).
+		Delete(new(model.Perm))
 	return err
 }

server/store/mocks/mock_Store.go

@@ -104,7 +104,7 @@ type Store interface {
 	// Permissions
 	PermFind(user *model.User, repo *model.Repo) (*model.Perm, error)
 	PermUpsert(perm *model.Perm) error
-	PermDeleteByUserAndRepoIDs(userID int64, repoIDs []int64) error
+	PermPrune(userID int64, keepRepoIDs []int64) error
 
 	// Configs
 	ConfigsForPipeline(pipelineID int64) ([]*model.Config, error)