Nicely handle missing user in collaborations (#17049)

* Nicely handle missing user in collaborations

It is possible to have a collaboration in a repository which refers to a no-longer
existing user. This causes the repository transfer to fail with an unusual error.

This PR makes `repo.getCollaborators()` nicely handle the missing user by ghosting
the collaboration but also adds consistency check. It also adds an
Access consistency check.

Fix #17044

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: KN4CK3R <admin@oldschoolhack.me>

models/access.go

@@ -230,6 +230,9 @@ func (repo *Repository) refreshCollaboratorAccesses(e db.Engine, accessMap map[i
 		return fmt.Errorf("getCollaborations: %v", err)
 	}
 	for _, c := range collaborators {
+		if c.User.IsGhost() {
+			continue
+		}
 		updateUserAccess(accessMap, c.User, c.Collaboration.Mode)
 	}
 	return nil

models/repo_collaboration.go

@@ -9,6 +9,7 @@ import (
 	"fmt"
 
 	"code.gitea.io/gitea/models/db"
+	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/timeutil"
 
 	"xorm.io/builder"
@@ -88,16 +89,21 @@ func (repo *Repository) getCollaborators(e db.Engine, listOptions db.ListOptions
 		return nil, fmt.Errorf("getCollaborations: %v", err)
 	}
 
-	collaborators := make([]*Collaborator, len(collaborations))
+	collaborators := make([]*Collaborator, 0, len(collaborations))
-	for i, c := range collaborations {
+	for _, c := range collaborations {
 		user, err := getUserByID(e, c.UserID)
 		if err != nil {
+			if IsErrUserNotExist(err) {
+				log.Warn("Inconsistent DB: User: %d is listed as collaborator of %-v but does not exist", c.UserID, repo)
+				user = NewGhostUser()
+			} else {
 				return nil, err
 			}
-		collaborators[i] = &Collaborator{
+		}
+		collaborators = append(collaborators, &Collaborator{
 			User:          user,
 			Collaboration: c,
-		}
+		})
 	}
 	return collaborators, nil
 }

models/repo_transfer.go

@@ -274,6 +274,14 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) (err e
 	// Dummy object.
 	collaboration := &Collaboration{RepoID: repo.ID}
 	for _, c := range collaborators {
+		if c.IsGhost() {
+			collaboration.ID = c.Collaboration.ID
+			if _, err := sess.Delete(collaboration); err != nil {
+				return fmt.Errorf("remove collaborator '%d': %v", c.ID, err)
+			}
+			collaboration.ID = 0
+		}
+
 		if c.ID != newOwner.ID {
 			isMember, err := isOrganizationMember(sess, newOwner.ID, c.ID)
 			if err != nil {
@@ -286,6 +294,7 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) (err e
 		if _, err := sess.Delete(collaboration); err != nil {
 			return fmt.Errorf("remove collaborator '%d': %v", c.ID, err)
 		}
+		collaboration.UserID = 0
 	}
 
 	// Remove old team-repository relations.

modules/doctor/dbconsistency.go

@@ -14,6 +14,64 @@ import (
 	"code.gitea.io/gitea/modules/setting"
 )
 
+type consistencyCheck struct {
+	Name         string
+	Counter      func() (int64, error)
+	Fixer        func() (int64, error)
+	FixedMessage string
+}
+
+func (c *consistencyCheck) Run(logger log.Logger, autofix bool) error {
+	count, err := c.Counter()
+	if err != nil {
+		logger.Critical("Error: %v whilst counting %s", err, c.Name)
+		return err
+	}
+	if count > 0 {
+		if autofix {
+			var fixed int64
+			if fixed, err = c.Fixer(); err != nil {
+				logger.Critical("Error: %v whilst fixing %s", err, c.Name)
+				return err
+			}
+
+			prompt := "Deleted"
+			if c.FixedMessage != "" {
+				prompt = c.FixedMessage
+			}
+
+			if fixed < 0 {
+				logger.Info(prompt+" %d %s", count, c.Name)
+			} else {
+				logger.Info(prompt+" %d/%d %s", fixed, count, c.Name)
+			}
+		} else {
+			logger.Warn("Found %d %s", count, c.Name)
+		}
+	}
+	return nil
+}
+
+func asFixer(fn func() error) func() (int64, error) {
+	return func() (int64, error) {
+		err := fn()
+		return -1, err
+	}
+}
+
+func genericOrphanCheck(name, subject, refobject, joincond string) consistencyCheck {
+	return consistencyCheck{
+		Name: name,
+		Counter: func() (int64, error) {
+			return models.CountOrphanedObjects(subject, refobject, joincond)
+		},
+		Fixer: func() (int64, error) {
+			err := models.DeleteOrphanedObjects(subject, refobject, joincond)
+			return -1, err
+		},
+	}
+}
+
 func checkDBConsistency(logger log.Logger, autofix bool) error {
 	// make sure DB version is uptodate
 	if err := db.NewEngine(context.Background(), migrations.EnsureUpToDate); err != nil {
@@ -21,282 +79,109 @@ func checkDBConsistency(logger log.Logger, autofix bool) error {
 		return err
 	}
 
+	consistencyChecks := []consistencyCheck{
+		{
 			// find labels without existing repo or org
-	count, err := models.CountOrphanedLabels()
+			Name:    "Orphaned Labels without existing repository or organisation",
-	if err != nil {
+			Counter: models.CountOrphanedLabels,
-		logger.Critical("Error: %v whilst counting orphaned labels", err)
+			Fixer:   asFixer(models.DeleteOrphanedLabels),
-		return err
+		},
-	}
+		{
-	if count > 0 {
-		if autofix {
-			if err = models.DeleteOrphanedLabels(); err != nil {
-				logger.Critical("Error: %v whilst deleting orphaned labels", err)
-				return err
-			}
-			logger.Info("%d labels without existing repository/organisation deleted", count)
-		} else {
-			logger.Warn("%d labels without existing repository/organisation", count)
-		}
-	}
-
 			// find IssueLabels without existing label
-	count, err = models.CountOrphanedIssueLabels()
+			Name:    "Orphaned Issue Labels without existing label",
-	if err != nil {
+			Counter: models.CountOrphanedIssueLabels,
-		logger.Critical("Error: %v whilst counting orphaned issue_labels", err)
+			Fixer:   asFixer(models.DeleteOrphanedIssueLabels),
-		return err
+		},
-	}
+		{
-	if count > 0 {
-		if autofix {
-			if err = models.DeleteOrphanedIssueLabels(); err != nil {
-				logger.Critical("Error: %v whilst deleting orphaned issue_labels", err)
-				return err
-			}
-			logger.Info("%d issue_labels without existing label deleted", count)
-		} else {
-			logger.Warn("%d issue_labels without existing label", count)
-		}
-	}
-
 			// find issues without existing repository
-	count, err = models.CountOrphanedIssues()
+			Name:    "Orphaned Issues without existing repository",
-	if err != nil {
+			Counter: models.CountOrphanedIssues,
-		logger.Critical("Error: %v whilst counting orphaned issues", err)
+			Fixer:   asFixer(models.DeleteOrphanedIssues),
-		return err
+		},
-	}
-	if count > 0 {
-		if autofix {
-			if err = models.DeleteOrphanedIssues(); err != nil {
-				logger.Critical("Error: %v whilst deleting orphaned issues", err)
-				return err
-			}
-			logger.Info("%d issues without existing repository deleted", count)
-		} else {
-			logger.Warn("%d issues without existing repository", count)
-		}
-	}
-
 		// find releases without existing repository
-	count, err = models.CountOrphanedObjects("release", "repository", "release.repo_id=repository.id")
+		genericOrphanCheck("Orphaned Releases without existing repository",
-	if err != nil {
+			"release", "repository", "release.repo_id=repository.id"),
-		logger.Critical("Error: %v whilst counting orphaned objects", err)
-		return err
-	}
-	if count > 0 {
-		if autofix {
-			if err = models.DeleteOrphanedObjects("release", "repository", "release.repo_id=repository.id"); err != nil {
-				logger.Critical("Error: %v whilst deleting orphaned objects", err)
-				return err
-			}
-			logger.Info("%d releases without existing repository deleted", count)
-		} else {
-			logger.Warn("%d releases without existing repository", count)
-		}
-	}
-
 		// find pulls without existing issues
-	count, err = models.CountOrphanedObjects("pull_request", "issue", "pull_request.issue_id=issue.id")
+		genericOrphanCheck("Orphaned PullRequests without existing issue",
-	if err != nil {
+			"pull_request", "issue", "pull_request.issue_id=issue.id"),
-		logger.Critical("Error: %v whilst counting orphaned objects", err)
-		return err
-	}
-	if count > 0 {
-		if autofix {
-			if err = models.DeleteOrphanedObjects("pull_request", "issue", "pull_request.issue_id=issue.id"); err != nil {
-				logger.Critical("Error: %v whilst deleting orphaned objects", err)
-				return err
-			}
-			logger.Info("%d pull requests without existing issue deleted", count)
-		} else {
-			logger.Warn("%d pull requests without existing issue", count)
-		}
-	}
-
 		// find tracked times without existing issues/pulls
-	count, err = models.CountOrphanedObjects("tracked_time", "issue", "tracked_time.issue_id=issue.id")
+		genericOrphanCheck("Orphaned TrackedTimes without existing issue",
-	if err != nil {
+			"tracked_time", "issue", "tracked_time.issue_id=issue.id"),
-		logger.Critical("Error: %v whilst counting orphaned objects", err)
-		return err
-	}
-	if count > 0 {
-		if autofix {
-			if err = models.DeleteOrphanedObjects("tracked_time", "issue", "tracked_time.issue_id=issue.id"); err != nil {
-				logger.Critical("Error: %v whilst deleting orphaned objects", err)
-				return err
-			}
-			logger.Info("%d tracked times without existing issue deleted", count)
-		} else {
-			logger.Warn("%d tracked times without existing issue", count)
-		}
-	}
-
 		// find attachments without existing issues or releases
-	count, err = models.CountOrphanedAttachments()
+		{
-	if err != nil {
+			Name:    "Orphaned Attachments without existing issues or releases",
-		logger.Critical("Error: %v whilst counting orphaned objects", err)
+			Counter: models.CountOrphanedAttachments,
-		return err
+			Fixer:   asFixer(models.DeleteOrphanedAttachments),
-	}
+		},
-	if count > 0 {
-		if autofix {
-			if err = models.DeleteOrphanedAttachments(); err != nil {
-				logger.Critical("Error: %v whilst deleting orphaned objects", err)
-				return err
-			}
-			logger.Info("%d attachments without existing issue or release deleted", count)
-		} else {
-			logger.Warn("%d attachments without existing issue or release", count)
-		}
-	}
-
 		// find null archived repositories
-	count, err = models.CountNullArchivedRepository()
+		{
-	if err != nil {
+			Name:         "Repositories with is_archived IS NULL",
-		logger.Critical("Error: %v whilst counting null archived repositories", err)
+			Counter:      models.CountNullArchivedRepository,
-		return err
+			Fixer:        models.FixNullArchivedRepository,
-	}
+			FixedMessage: "Fixed",
-	if count > 0 {
+		},
-		if autofix {
-			updatedCount, err := models.FixNullArchivedRepository()
-			if err != nil {
-				logger.Critical("Error: %v whilst fixing null archived repositories", err)
-				return err
-			}
-			logger.Info("%d repositories with null is_archived updated", updatedCount)
-		} else {
-			logger.Warn("%d repositories with null is_archived", count)
-		}
-	}
-
 		// find label comments with empty labels
-	count, err = models.CountCommentTypeLabelWithEmptyLabel()
+		{
-	if err != nil {
+			Name:         "Label comments with empty labels",
-		logger.Critical("Error: %v whilst counting label comments with empty labels", err)
+			Counter:      models.CountCommentTypeLabelWithEmptyLabel,
-		return err
+			Fixer:        models.FixCommentTypeLabelWithEmptyLabel,
-	}
+			FixedMessage: "Fixed",
-	if count > 0 {
+		},
-		if autofix {
-			updatedCount, err := models.FixCommentTypeLabelWithEmptyLabel()
-			if err != nil {
-				logger.Critical("Error: %v whilst removing label comments with empty labels", err)
-				return err
-			}
-			logger.Info("%d label comments with empty labels removed", updatedCount)
-		} else {
-			logger.Warn("%d label comments with empty labels", count)
-		}
-	}
-
 		// find label comments with labels from outside the repository
-	count, err = models.CountCommentTypeLabelWithOutsideLabels()
+		{
-	if err != nil {
+			Name:         "Label comments with labels from outside the repository",
-		logger.Critical("Error: %v whilst counting label comments with outside labels", err)
+			Counter:      models.CountCommentTypeLabelWithOutsideLabels,
-		return err
+			Fixer:        models.FixCommentTypeLabelWithOutsideLabels,
-	}
+			FixedMessage: "Removed",
-	if count > 0 {
+		},
-		if autofix {
-			updatedCount, err := models.FixCommentTypeLabelWithOutsideLabels()
-			if err != nil {
-				logger.Critical("Error: %v whilst removing label comments with outside labels", err)
-				return err
-			}
-			log.Info("%d label comments with outside labels removed", updatedCount)
-		} else {
-			log.Warn("%d label comments with outside labels", count)
-		}
-	}
-
 		// find issue_label with labels from outside the repository
-	count, err = models.CountIssueLabelWithOutsideLabels()
+		{
-	if err != nil {
+			Name:         "IssueLabels with Labels from outside the repository",
-		logger.Critical("Error: %v whilst counting issue_labels from outside the repository or organisation", err)
+			Counter:      models.CountIssueLabelWithOutsideLabels,
-		return err
+			Fixer:        models.FixIssueLabelWithOutsideLabels,
-	}
+			FixedMessage: "Removed",
-	if count > 0 {
+		},
-		if autofix {
-			updatedCount, err := models.FixIssueLabelWithOutsideLabels()
-			if err != nil {
-				logger.Critical("Error: %v whilst removing issue_labels from outside the repository or organisation", err)
-				return err
-			}
-			logger.Info("%d issue_labels from outside the repository or organisation removed", updatedCount)
-		} else {
-			logger.Warn("%d issue_labels from outside the repository or organisation", count)
-		}
 	}
 
 	// TODO: function to recalc all counters
 
 	if setting.Database.UsePostgreSQL {
-		count, err = db.CountBadSequences()
+		consistencyChecks = append(consistencyChecks, consistencyCheck{
-		if err != nil {
+			Name:         "Sequence values",
-			logger.Critical("Error: %v whilst checking sequence values", err)
+			Counter:      db.CountBadSequences,
-			return err
+			Fixer:        asFixer(db.FixBadSequences),
-		}
+			FixedMessage: "Updated",
-		if count > 0 {
+		})
-			if autofix {
-				err := db.FixBadSequences()
-				if err != nil {
-					logger.Critical("Error: %v whilst attempting to fix sequences", err)
-					return err
-				}
-				logger.Info("%d sequences updated", count)
-			} else {
-				logger.Warn("%d sequences with incorrect values", count)
-			}
-		}
 	}
 
+	consistencyChecks = append(consistencyChecks,
 		// find protected branches without existing repository
-	count, err = models.CountOrphanedObjects("protected_branch", "repository", "protected_branch.repo_id=repository.id")
+		genericOrphanCheck("Protected Branches without existing repository",
-	if err != nil {
+			"protected_branch", "repository", "protected_branch.repo_id=repository.id"),
-		logger.Critical("Error: %v whilst counting orphaned objects", err)
-		return err
-	}
-	if count > 0 {
-		if autofix {
-			if err = models.DeleteOrphanedObjects("protected_branch", "repository", "protected_branch.repo_id=repository.id"); err != nil {
-				logger.Critical("Error: %v whilst deleting orphaned objects", err)
-				return err
-			}
-			logger.Info("%d protected branches without existing repository deleted", count)
-		} else {
-			logger.Warn("%d protected branches without existing repository", count)
-		}
-	}
-
 		// find deleted branches without existing repository
-	count, err = models.CountOrphanedObjects("deleted_branch", "repository", "deleted_branch.repo_id=repository.id")
+		genericOrphanCheck("Deleted Branches without existing repository",
-	if err != nil {
+			"deleted_branch", "repository", "deleted_branch.repo_id=repository.id"),
-		logger.Critical("Error: %v whilst counting orphaned objects", err)
-		return err
-	}
-	if count > 0 {
-		if autofix {
-			if err = models.DeleteOrphanedObjects("deleted_branch", "repository", "deleted_branch.repo_id=repository.id"); err != nil {
-				logger.Critical("Error: %v whilst deleting orphaned objects", err)
-				return err
-			}
-			logger.Info("%d deleted branches without existing repository deleted", count)
-		} else {
-			logger.Warn("%d deleted branches without existing repository", count)
-		}
-	}
-
 		// find LFS locks without existing repository
-	count, err = models.CountOrphanedObjects("lfs_lock", "repository", "lfs_lock.repo_id=repository.id")
+		genericOrphanCheck("LFS locks without existing repository",
-	if err != nil {
+			"lfs_lock", "repository", "lfs_lock.repo_id=repository.id"),
-		logger.Critical("Error: %v whilst counting orphaned objects", err)
+		// find collaborations without users
+		genericOrphanCheck("Collaborations without existing user",
+			"collaboration", "user", "collaboration.user_id=user.id"),
+		// find collaborations without repository
+		genericOrphanCheck("Collaborations without existing repository",
+			"collaboration", "repository", "collaboration.repo_id=repository.id"),
+		// find access without users
+		genericOrphanCheck("Access entries without existing user",
+			"access", "user", "access.user_id=user.id"),
+		// find access without repository
+		genericOrphanCheck("Access entries without existing repository",
+			"access", "repository", "access.repo_id=repository.id"),
+	)
+
+	for _, c := range consistencyChecks {
+		if err := c.Run(logger, autofix); err != nil {
 			return err
 		}
-	if count > 0 {
-		if autofix {
-			if err = models.DeleteOrphanedObjects("lfs_lock", "repository", "lfs_lock.repo_id=repository.id"); err != nil {
-				logger.Critical("Error: %v whilst deleting orphaned objects", err)
-				return err
-			}
-			logger.Info("%d LFS locks without existing repository deleted", count)
-		} else {
-			logger.Warn("%d LFS locks without existing repository", count)
-		}
 	}
 
 	return nil