From 4707d4b8a9fb0b29c71c46d788a8630e4a878a10 Mon Sep 17 00:00:00 2001 From: zeripath Date: Tue, 28 Sep 2021 07:41:12 +0100 Subject: [PATCH] Nicely handle missing user in collaborations (#17049) (#17166) Backport #17049 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 Co-authored-by: KN4CK3R --- models/access.go | 3 + models/repo_collaboration.go | 16 +- models/repo_transfer.go | 9 + modules/doctor/dbconsistency.go | 381 +++++++++++++------------------- 4 files changed, 171 insertions(+), 238 deletions(-) diff --git a/models/access.go b/models/access.go index d35b900cf..264dbc6f6 100644 --- a/models/access.go +++ b/models/access.go @@ -225,6 +225,9 @@ func (repo *Repository) refreshCollaboratorAccesses(e Engine, accessMap map[int6 return fmt.Errorf("getCollaborations: %v", err) } for _, c := range collaborators { + if c.User.IsGhost() { + continue + } updateUserAccess(accessMap, c.User, c.Collaboration.Mode) } return nil diff --git a/models/repo_collaboration.go b/models/repo_collaboration.go index b9488f5e2..5dbba2124 100644 --- a/models/repo_collaboration.go +++ b/models/repo_collaboration.go @@ -8,6 +8,7 @@ package models import ( "fmt" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/timeutil" "xorm.io/builder" @@ -83,16 +84,21 @@ func (repo *Repository) getCollaborators(e Engine, listOptions ListOptions) ([]* return nil, fmt.Errorf("getCollaborations: %v", err) } - collaborators := make([]*Collaborator, len(collaborations)) - for i, c := range collaborations { + collaborators := make([]*Collaborator, 0, len(collaborations)) + for _, c := range collaborations { user, err := getUserByID(e, c.UserID) if err != nil { - return nil, err + 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 } diff --git a/models/repo_transfer.go b/models/repo_transfer.go index d7ef0a8ca..37fd166a6 100644 --- a/models/repo_transfer.go +++ b/models/repo_transfer.go @@ -269,6 +269,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 { @@ -281,6 +289,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. diff --git a/modules/doctor/dbconsistency.go b/modules/doctor/dbconsistency.go index 23e8331e7..0dadf22be 100644 --- a/modules/doctor/dbconsistency.go +++ b/modules/doctor/dbconsistency.go @@ -13,6 +13,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 := models.NewEngine(context.Background(), migrations.EnsureUpToDate); err != nil { @@ -20,246 +78,103 @@ func checkDBConsistency(logger log.Logger, autofix bool) error { return err } - // find labels without existing repo or org - count, err := models.CountOrphanedLabels() - if err != nil { - logger.Critical("Error: %v whilst counting orphaned labels", err) - 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() - if err != nil { - logger.Critical("Error: %v whilst counting orphaned issue_labels", err) - 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() - if err != nil { - logger.Critical("Error: %v whilst counting orphaned issues", err) - 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 pulls without existing issues - count, err = models.CountOrphanedObjects("pull_request", "issue", "pull_request.issue_id=issue.id") - if err != nil { - 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") - if err != nil { - 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 null archived repositories - count, err = models.CountNullArchivedRepository() - if err != nil { - logger.Critical("Error: %v whilst counting null archived repositories", err) - return err - } - 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 { - logger.Critical("Error: %v whilst counting label comments with empty labels", err) - return err - } - 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 { - logger.Critical("Error: %v whilst counting label comments with outside labels", err) - return err - } - 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 { - logger.Critical("Error: %v whilst counting issue_labels from outside the repository or organisation", err) - return err - } - 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) - } + consistencyChecks := []consistencyCheck{ + { + // find labels without existing repo or org + Name: "Orphaned Labels without existing repository or organisation", + Counter: models.CountOrphanedLabels, + Fixer: asFixer(models.DeleteOrphanedLabels), + }, + { + // find IssueLabels without existing label + Name: "Orphaned Issue Labels without existing label", + Counter: models.CountOrphanedIssueLabels, + Fixer: asFixer(models.DeleteOrphanedIssueLabels), + }, + { + // find issues without existing repository + Name: "Orphaned Issues without existing repository", + Counter: models.CountOrphanedIssues, + Fixer: asFixer(models.DeleteOrphanedIssues), + }, + // find releases without existing repository + genericOrphanCheck("Orphaned Releases without existing repository", + "release", "repository", "release.repo_id=repository.id"), + // find pulls without existing issues + genericOrphanCheck("Orphaned PullRequests without existing issue", + "pull_request", "issue", "pull_request.issue_id=issue.id"), + // find tracked times without existing issues/pulls + genericOrphanCheck("Orphaned TrackedTimes without existing issue", + "tracked_time", "issue", "tracked_time.issue_id=issue.id"), + // find null archived repositories + { + Name: "Repositories with is_archived IS NULL", + Counter: models.CountNullArchivedRepository, + Fixer: models.FixNullArchivedRepository, + FixedMessage: "Fixed", + }, + // find label comments with empty labels + { + Name: "Label comments with empty labels", + Counter: models.CountCommentTypeLabelWithEmptyLabel, + Fixer: models.FixCommentTypeLabelWithEmptyLabel, + FixedMessage: "Fixed", + }, + // find label comments with labels from outside the repository + { + Name: "Label comments with labels from outside the repository", + Counter: models.CountCommentTypeLabelWithOutsideLabels, + Fixer: models.FixCommentTypeLabelWithOutsideLabels, + FixedMessage: "Removed", + }, + // find issue_label with labels from outside the repository + { + Name: "IssueLabels with Labels from outside the repository", + Counter: models.CountIssueLabelWithOutsideLabels, + Fixer: models.FixIssueLabelWithOutsideLabels, + FixedMessage: "Removed", + }, } // TODO: function to recalc all counters if setting.Database.UsePostgreSQL { - count, err = models.CountBadSequences() - if err != nil { - logger.Critical("Error: %v whilst checking sequence values", err) + consistencyChecks = append(consistencyChecks, consistencyCheck{ + Name: "Sequence values", + Counter: models.CountBadSequences, + Fixer: asFixer(models.FixBadSequences), + FixedMessage: "Updated", + }) + } + + consistencyChecks = append(consistencyChecks, + // find protected branches without existing repository + genericOrphanCheck("Protected Branches without existing repository", + "protected_branch", "repository", "protected_branch.repo_id=repository.id"), + // find deleted branches without existing repository + genericOrphanCheck("Deleted Branches without existing repository", + "deleted_branch", "repository", "deleted_branch.repo_id=repository.id"), + // find LFS locks without existing repository + genericOrphanCheck("LFS locks without existing repository", + "lfs_lock", "repository", "lfs_lock.repo_id=repository.id"), + // 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 { - err := models.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) - } - } - } - - // find protected branches without existing repository - count, err = models.CountOrphanedObjects("protected_branch", "repository", "protected_branch.repo_id=repository.id") - if err != nil { - 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") - if err != nil { - 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") - if err != nil { - logger.Critical("Error: %v whilst counting orphaned objects", err) - 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