From 466facc0097bf636b6a945a0daebb7c4c5c33c91 Mon Sep 17 00:00:00 2001 From: Unknwon Date: Tue, 17 Mar 2015 21:51:39 -0400 Subject: [PATCH] #1067: Deleting users should remove them from collaborator lists - fix delete user but repository watches are not decreased --- models/action.go | 35 +++++++++++------ models/error.go | 53 +++++++++++++++++++++++++ models/org.go | 35 ++++++----------- models/repo.go | 26 ++++++------ models/user.go | 87 +++++++++++++++++++++++++---------------- modules/mailer/mail.go | 2 +- routers/admin/users.go | 6 +-- routers/org/members.go | 4 +- routers/org/setting.go | 7 ++-- routers/org/teams.go | 2 +- routers/repo/issue.go | 8 ++-- routers/user/home.go | 4 +- routers/user/setting.go | 15 ++----- 13 files changed, 175 insertions(+), 109 deletions(-) diff --git a/models/action.go b/models/action.go index 46ce44e26..037ccbd61 100644 --- a/models/action.go +++ b/models/action.go @@ -61,14 +61,14 @@ func init() { // Action represents user operation type and other information to repository., // it implemented interface base.Actioner so that can be used in template render. type Action struct { - Id int64 - UserId int64 // Receiver user id. + ID int64 `xorm:"pk autoincr"` + UserID int64 // Receiver user id. OpType ActionType - ActUserId int64 // Action user id. + ActUserID int64 // Action user id. ActUserName string // Action user name. ActEmail string ActAvatar string `xorm:"-"` - RepoId int64 + RepoID int64 RepoUserName string RepoName string RefName string @@ -319,10 +319,18 @@ func CommitRepoAction(userId, repoUserId int64, userName, actEmail string, log.Debug("action.CommitRepoAction(updateIssuesCommit): ", err) } - if err = NotifyWatchers(&Action{ActUserId: userId, ActUserName: userName, ActEmail: actEmail, - OpType: opType, Content: string(bs), RepoId: repoId, RepoUserName: repoUserName, - RepoName: repoName, RefName: refName, - IsPrivate: repo.IsPrivate}); err != nil { + if err = NotifyWatchers(&Action{ + ActUserID: userId, + ActUserName: userName, + ActEmail: actEmail, + OpType: opType, + Content: string(bs), + RepoID: repoId, + RepoUserName: repoUserName, + RepoName: repoName, + RefName: refName, + IsPrivate: repo.IsPrivate, + }); err != nil { return errors.New("action.CommitRepoAction(NotifyWatchers): " + err.Error()) } @@ -443,14 +451,15 @@ func CommitRepoAction(userId, repoUserId int64, userName, actEmail string, func newRepoAction(e Engine, u *User, repo *Repository) (err error) { if err = notifyWatchers(e, &Action{ - ActUserId: u.Id, + ActUserID: u.Id, ActUserName: u.Name, ActEmail: u.Email, OpType: CREATE_REPO, - RepoId: repo.Id, + RepoID: repo.Id, RepoUserName: repo.Owner.Name, RepoName: repo.Name, - IsPrivate: repo.IsPrivate}); err != nil { + IsPrivate: repo.IsPrivate, + }); err != nil { return fmt.Errorf("notify watchers '%d/%s'", u.Id, repo.Id) } @@ -465,11 +474,11 @@ func NewRepoAction(u *User, repo *Repository) (err error) { func transferRepoAction(e Engine, actUser, oldOwner, newOwner *User, repo *Repository) (err error) { action := &Action{ - ActUserId: actUser.Id, + ActUserID: actUser.Id, ActUserName: actUser.Name, ActEmail: actUser.Email, OpType: TRANSFER_REPO, - RepoId: repo.Id, + RepoID: repo.Id, RepoUserName: newOwner.Name, RepoName: repo.Name, IsPrivate: repo.IsPrivate, diff --git a/models/error.go b/models/error.go index 6f1a366cb..a434b8d6d 100644 --- a/models/error.go +++ b/models/error.go @@ -8,6 +8,59 @@ import ( "fmt" ) +// ____ ___ +// | | \______ ___________ +// | | / ___// __ \_ __ \ +// | | /\___ \\ ___/| | \/ +// |______//____ >\___ >__| +// \/ \/ + +type ErrUserOwnRepos struct { + UID int64 +} + +func IsErrUserOwnRepos(err error) bool { + _, ok := err.(ErrUserOwnRepos) + return ok +} + +func (err ErrUserOwnRepos) Error() string { + return fmt.Sprintf("user still has ownership of repositories: [uid: %d]", err.UID) +} + +type ErrUserHasOrgs struct { + UID int64 +} + +func IsErrUserHasOrgs(err error) bool { + _, ok := err.(ErrUserHasOrgs) + return ok +} + +func (err ErrUserHasOrgs) Error() string { + return fmt.Sprintf("user still has membership of organizations: [uid: %d]", err.UID) +} + +// ________ .__ __ .__ +// \_____ \_______ _________ ____ |__|____________ _/ |_|__| ____ ____ +// / | \_ __ \/ ___\__ \ / \| \___ /\__ \\ __\ |/ _ \ / \ +// / | \ | \/ /_/ > __ \| | \ |/ / / __ \| | | ( <_> ) | \ +// \_______ /__| \___ (____ /___| /__/_____ \(____ /__| |__|\____/|___| / +// \/ /_____/ \/ \/ \/ \/ \/ + +type ErrLastOrgOwner struct { + UID int64 +} + +func IsErrLastOrgOwner(err error) bool { + _, ok := err.(ErrLastOrgOwner) + return ok +} + +func (err ErrLastOrgOwner) Error() string { + return fmt.Sprintf("user is the last member of owner team: [uid: %d]", err.UID) +} + // __________ .__ __ // \______ \ ____ ______ ____ _____|__|/ |_ ___________ ___.__. // | _// __ \\____ \ / _ \/ ___/ \ __\/ _ \_ __ < | | diff --git a/models/org.go b/models/org.go index 2c2b9b635..32f135cbb 100644 --- a/models/org.go +++ b/models/org.go @@ -18,7 +18,6 @@ var ( ErrTeamAlreadyExist = errors.New("Team already exist") ErrTeamNotExist = errors.New("Team does not exist") ErrTeamNameIllegal = errors.New("Team name contains illegal characters") - ErrLastOrgOwner = errors.New("The user to remove is the last member in owner team") ) // IsOwnedBy returns true if given user is in the owner team. @@ -339,18 +338,20 @@ func RemoveOrgUser(orgId, uid int64) error { has, err := x.Where("uid=?", uid).And("org_id=?", orgId).Get(ou) if err != nil { - return err + return fmt.Errorf("get org-user: %v", err) } else if !has { return nil } u, err := GetUserById(uid) if err != nil { - return err + return fmt.Errorf("GetUserById: %v", err) } org, err := GetUserById(orgId) if err != nil { - return err + return fmt.Errorf("get organization: %v", err) + } else if err = org.GetRepositories(); err != nil { + return fmt.Errorf("GetRepositories: %v", err) } // Check if the user to delete is the last member in owner team. @@ -360,49 +361,39 @@ func RemoveOrgUser(orgId, uid int64) error { return err } if t.NumMembers == 1 { - return ErrLastOrgOwner + return ErrLastOrgOwner{UID: uid} } } sess := x.NewSession() - defer sess.Close() + defer sessionRelease(sess) if err := sess.Begin(); err != nil { return err } if _, err := sess.Id(ou.ID).Delete(ou); err != nil { - sess.Rollback() return err - } else if _, err = sess.Exec("UPDATE `user` SET num_members=num_members-1 WHERE id = ?", orgId); err != nil { - sess.Rollback() + } else if _, err = sess.Exec("UPDATE `user` SET num_members=num_members-1 WHERE id=?", orgId); err != nil { return err } // Delete all repository accesses. - if err = org.GetRepositories(); err != nil { - sess.Rollback() - return err - } - access := &Access{ - UserID: u.Id, - } + access := &Access{UserID: u.Id} for _, repo := range org.Repos { access.RepoID = repo.Id if _, err = sess.Delete(access); err != nil { - sess.Rollback() return err - } else if err = WatchRepo(u.Id, repo.Id, false); err != nil { - sess.Rollback() + } else if err = watchRepo(sess, u.Id, repo.Id, false); err != nil { return err } } // Delete member in his/her teams. - ts, err := GetUserTeams(org.Id, u.Id) + teams, err := getUserTeams(sess, org.Id, u.Id) if err != nil { return err } - for _, t := range ts { + for _, t := range teams { if err = removeTeamMember(sess, org.Id, t.ID, u.Id); err != nil { return err } @@ -902,7 +893,7 @@ func removeTeamMember(e Engine, orgId, teamId, uid int64) error { // Check if the user to delete is the last member in owner team. if t.IsOwnerTeam() && t.NumMembers == 1 { - return ErrLastOrgOwner + return ErrLastOrgOwner{UID: uid} } t.NumMembers-- diff --git a/models/repo.go b/models/repo.go index e5d63c03f..48b6b8bd2 100644 --- a/models/repo.go +++ b/models/repo.go @@ -819,9 +819,9 @@ func DeleteRepository(uid, repoID int64, userName string) error { return err } else if _, err = sess.Delete(&Access{RepoID: repo.Id}); err != nil { return err - } else if _, err = sess.Delete(&Action{RepoId: repo.Id}); err != nil { + } else if _, err = sess.Delete(&Action{RepoID: repo.Id}); err != nil { return err - } else if _, err = sess.Delete(&Watch{RepoId: repoID}); err != nil { + } else if _, err = sess.Delete(&Watch{RepoID: repoID}); err != nil { return err } else if _, err = sess.Delete(&Mirror{RepoId: repoID}); err != nil { return err @@ -1190,9 +1190,9 @@ func (repo *Repository) DeleteCollaborator(u *User) (err error) { // Watch is connection request for receiving repository notification. type Watch struct { - Id int64 - UserId int64 `xorm:"UNIQUE(watch)"` - RepoId int64 `xorm:"UNIQUE(watch)"` + ID int64 `xorm:"pk autoincr"` + UserID int64 `xorm:"UNIQUE(watch)"` + RepoID int64 `xorm:"UNIQUE(watch)"` } // IsWatching checks if user has watched given repository. @@ -1206,7 +1206,7 @@ func watchRepo(e Engine, uid, repoId int64, watch bool) (err error) { if IsWatching(uid, repoId) { return nil } - if _, err = e.Insert(&Watch{RepoId: repoId, UserId: uid}); err != nil { + if _, err = e.Insert(&Watch{RepoID: repoId, UserID: uid}); err != nil { return err } _, err = e.Exec("UPDATE `repository` SET num_watches = num_watches + 1 WHERE id = ?", repoId) @@ -1217,7 +1217,7 @@ func watchRepo(e Engine, uid, repoId int64, watch bool) (err error) { if _, err = e.Delete(&Watch{0, uid, repoId}); err != nil { return err } - _, err = e.Exec("UPDATE `repository` SET num_watches = num_watches - 1 WHERE id = ?", repoId) + _, err = e.Exec("UPDATE `repository` SET num_watches=num_watches-1 WHERE id=?", repoId) } return err } @@ -1229,7 +1229,7 @@ func WatchRepo(uid, repoId int64, watch bool) (err error) { func getWatchers(e Engine, rid int64) ([]*Watch, error) { watches := make([]*Watch, 0, 10) - err := e.Find(&watches, &Watch{RepoId: rid}) + err := e.Find(&watches, &Watch{RepoID: rid}) return watches, err } @@ -1240,24 +1240,24 @@ func GetWatchers(rid int64) ([]*Watch, error) { func notifyWatchers(e Engine, act *Action) error { // Add feeds for user self and all watchers. - watches, err := getWatchers(e, act.RepoId) + watches, err := getWatchers(e, act.RepoID) if err != nil { return fmt.Errorf("get watchers: %v", err) } // Add feed for actioner. - act.UserId = act.ActUserId + act.UserID = act.ActUserID if _, err = e.InsertOne(act); err != nil { return fmt.Errorf("insert new actioner: %v", err) } for i := range watches { - if act.ActUserId == watches[i].UserId { + if act.ActUserID == watches[i].UserID { continue } - act.Id = 0 - act.UserId = watches[i].UserId + act.ID = 0 + act.UserID = watches[i].UserID if _, err = e.InsertOne(act); err != nil { return fmt.Errorf("insert new action: %v", err) } diff --git a/models/user.go b/models/user.go index f3a6c38f6..dcfd0dc5e 100644 --- a/models/user.go +++ b/models/user.go @@ -36,8 +36,6 @@ const ( ) var ( - ErrUserOwnRepos = errors.New("User still have ownership of repositories") - ErrUserHasOrgs = errors.New("User still have membership of organization") ErrUserAlreadyExist = errors.New("User already exist") ErrUserNotExist = errors.New("User does not exist") ErrUserNotKeyOwner = errors.New("User does not the owner of public key") @@ -432,55 +430,75 @@ func UpdateUser(u *User) error { return err } +// DeleteBeans deletes all given beans, beans should contain delete conditions. +func DeleteBeans(e Engine, beans ...interface{}) (err error) { + for i := range beans { + if _, err = e.Delete(beans[i]); err != nil { + return err + } + } + return nil +} + // FIXME: need some kind of mechanism to record failure. HINT: system notice // DeleteUser completely and permanently deletes everything of user. func DeleteUser(u *User) error { // Check ownership of repository. count, err := GetRepositoryCount(u) if err != nil { - return errors.New("GetRepositoryCount: " + err.Error()) + return fmt.Errorf("GetRepositoryCount: %v", err) } else if count > 0 { - return ErrUserOwnRepos + return ErrUserOwnRepos{UID: u.Id} } // Check membership of organization. count, err = u.GetOrganizationCount() if err != nil { - return errors.New("GetOrganizationCount: " + err.Error()) + return fmt.Errorf("GetOrganizationCount: %v", err) } else if count > 0 { - return ErrUserHasOrgs + return ErrUserHasOrgs{UID: u.Id} } - // FIXME: check issues, other repos' commits - // FIXME: roll backable in some point. - - // Delete all followers. - if _, err = x.Delete(&Follow{FollowId: u.Id}); err != nil { - return err + // Get watches before session. + watches := make([]*Watch, 0, 10) + if err = x.Where("user_id=?", u.Id).Find(&watches); err != nil { + return fmt.Errorf("get all watches: %v", err) } - // Delete oauth2. - if _, err = x.Delete(&Oauth2{Uid: u.Id}); err != nil { - return err + repoIDs := make([]int64, 0, len(watches)) + for i := range watches { + repoIDs = append(repoIDs, watches[i].RepoID) } - // Delete all feeds. - if _, err = x.Delete(&Action{UserId: u.Id}); err != nil { - return err - } - // Delete all watches. - if _, err = x.Delete(&Watch{UserId: u.Id}); err != nil { + + // FIXME: check issues, other repos' commits + + sess := x.NewSession() + defer sessionRelease(sess) + if err = sess.Begin(); err != nil { return err } - // Delete all accesses. - if _, err = x.Delete(&Access{UserID: u.Id}); err != nil { + + if err = DeleteBeans(sess, + &Follow{FollowID: u.Id}, + &Oauth2{Uid: u.Id}, + &Action{UserID: u.Id}, + &Access{UserID: u.Id}, + &Collaboration{UserID: u.Id}, + &EmailAddress{Uid: u.Id}, + &Watch{UserID: u.Id}, + ); err != nil { return err } - // Delete all alternative email addresses - if _, err = x.Delete(&EmailAddress{Uid: u.Id}); err != nil { - return err + + // Decrease all watch numbers. + for i := range repoIDs { + if _, err = sess.Exec("UPDATE `repository` SET num_watches=num_watches-1 WHERE id=?", repoIDs[i]); err != nil { + return err + } } + // Delete all SSH keys. keys := make([]*PublicKey, 0, 10) - if err = x.Find(&keys, &PublicKey{OwnerId: u.Id}); err != nil { + if err = sess.Find(&keys, &PublicKey{OwnerId: u.Id}); err != nil { return err } for _, key := range keys { @@ -489,13 +507,16 @@ func DeleteUser(u *User) error { } } + if _, err = sess.Delete(u); err != nil { + return err + } + // Delete user directory. if err = os.RemoveAll(UserPath(u.Name)); err != nil { return err } - _, err = x.Delete(u) - return err + return sess.Commit() } // DeleteInactivateUsers deletes all inactivate users and email addresses. @@ -777,8 +798,8 @@ func SearchUserByName(opt SearchOption) (us []*User, err error) { // Follow is connection request for receiving user notification. type Follow struct { Id int64 - UserId int64 `xorm:"unique(follow)"` - FollowId int64 `xorm:"unique(follow)"` + UserID int64 `xorm:"unique(follow)"` + FollowID int64 `xorm:"unique(follow)"` } // FollowUser marks someone be another's follower. @@ -787,7 +808,7 @@ func FollowUser(userId int64, followId int64) (err error) { defer sess.Close() sess.Begin() - if _, err = sess.Insert(&Follow{UserId: userId, FollowId: followId}); err != nil { + if _, err = sess.Insert(&Follow{UserID: userId, FollowID: followId}); err != nil { sess.Rollback() return err } @@ -812,7 +833,7 @@ func UnFollowUser(userId int64, unFollowId int64) (err error) { defer session.Close() session.Begin() - if _, err = session.Delete(&Follow{UserId: userId, FollowId: unFollowId}); err != nil { + if _, err = session.Delete(&Follow{UserID: userId, FollowID: unFollowId}); err != nil { session.Rollback() return err } diff --git a/modules/mailer/mail.go b/modules/mailer/mail.go index ae8ce6531..9ed338ab2 100644 --- a/modules/mailer/mail.go +++ b/modules/mailer/mail.go @@ -163,7 +163,7 @@ func SendIssueNotifyMail(u, owner *models.User, repo *models.Repository, issue * tos := make([]string, 0, len(ws)) for i := range ws { - uid := ws[i].UserId + uid := ws[i].UserID if u.Id == uid { continue } diff --git a/routers/admin/users.go b/routers/admin/users.go index 4f57407a2..ddd12a8b2 100644 --- a/routers/admin/users.go +++ b/routers/admin/users.go @@ -222,11 +222,11 @@ func DeleteUser(ctx *middleware.Context) { } if err = models.DeleteUser(u); err != nil { - switch err { - case models.ErrUserOwnRepos: + switch { + case models.IsErrUserOwnRepos(err): ctx.Flash.Error(ctx.Tr("admin.users.still_own_repo")) ctx.Redirect(setting.AppSubUrl + "/admin/users/" + ctx.Params(":userid")) - case models.ErrUserHasOrgs: + case models.IsErrUserHasOrgs(err): ctx.Flash.Error(ctx.Tr("admin.users.still_has_org")) ctx.Redirect(setting.AppSubUrl + "/admin/users/" + ctx.Params(":userid")) default: diff --git a/routers/org/members.go b/routers/org/members.go index f571e334e..c8c90cfe4 100644 --- a/routers/org/members.go +++ b/routers/org/members.go @@ -61,14 +61,14 @@ func MembersAction(ctx *middleware.Context) { return } err = org.RemoveMember(uid) - if err == models.ErrLastOrgOwner { + if models.IsErrLastOrgOwner(err) { ctx.Flash.Error(ctx.Tr("form.last_org_owner")) ctx.Redirect(ctx.Org.OrgLink + "/members") return } case "leave": err = org.RemoveMember(ctx.User.Id) - if err == models.ErrLastOrgOwner { + if models.IsErrLastOrgOwner(err) { ctx.Flash.Error(ctx.Tr("form.last_org_owner")) ctx.Redirect(ctx.Org.OrgLink + "/members") return diff --git a/routers/org/setting.go b/routers/org/setting.go index c638a032e..8bc6a2a96 100644 --- a/routers/org/setting.go +++ b/routers/org/setting.go @@ -87,13 +87,12 @@ func SettingsDelete(ctx *middleware.Context) { org := ctx.Org.Organization if ctx.Req.Method == "POST" { - // TODO: validate password. + // FIXME: validate password. if err := models.DeleteOrganization(org); err != nil { - switch err { - case models.ErrUserOwnRepos: + if models.IsErrUserOwnRepos(err) { ctx.Flash.Error(ctx.Tr("form.org_still_own_repo")) ctx.Redirect(setting.AppSubUrl + "/org/" + org.LowerName + "/settings/delete") - default: + } else { ctx.Handle(500, "DeleteOrganization", err) } } else { diff --git a/routers/org/teams.go b/routers/org/teams.go index 40345cf7c..7436e30ee 100644 --- a/routers/org/teams.go +++ b/routers/org/teams.go @@ -91,7 +91,7 @@ func TeamsAction(ctx *middleware.Context) { } if err != nil { - if err == models.ErrLastOrgOwner { + if models.IsErrLastOrgOwner(err) { ctx.Flash.Error(ctx.Tr("form.last_org_owner")) } else { log.Error(3, "Action(%s): %v", ctx.Params(":action"), err) diff --git a/routers/repo/issue.go b/routers/repo/issue.go index abe33bebf..294d3d284 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -270,12 +270,12 @@ func CreateIssuePost(ctx *middleware.Context, form auth.CreateIssueForm) { } act := &models.Action{ - ActUserId: ctx.User.Id, + ActUserID: ctx.User.Id, ActUserName: ctx.User.Name, ActEmail: ctx.User.Email, OpType: models.CREATE_ISSUE, Content: fmt.Sprintf("%d|%s", issue.Index, issue.Name), - RepoId: ctx.Repo.Repository.Id, + RepoID: ctx.Repo.Repository.Id, RepoUserName: ctx.Repo.Owner.Name, RepoName: ctx.Repo.Repository.Name, RefName: ctx.Repo.BranchName, @@ -845,12 +845,12 @@ func Comment(ctx *middleware.Context) { // Notify watchers. act := &models.Action{ - ActUserId: ctx.User.Id, + ActUserID: ctx.User.Id, ActUserName: ctx.User.LowerName, ActEmail: ctx.User.Email, OpType: models.COMMENT_ISSUE, Content: fmt.Sprintf("%d|%s", issue.Index, strings.Split(content, "\n")[0]), - RepoId: ctx.Repo.Repository.Id, + RepoID: ctx.Repo.Repository.Id, RepoUserName: ctx.Repo.Owner.LowerName, RepoName: ctx.Repo.Repository.LowerName, } diff --git a/routers/user/home.go b/routers/user/home.go index d690b3a7c..86e907e34 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -104,7 +104,7 @@ func Dashboard(ctx *middleware.Context) { for _, act := range actions { if act.IsPrivate { // This prevents having to retrieve the repository for each action - repo := &models.Repository{Id: act.RepoId, IsPrivate: true} + repo := &models.Repository{Id: act.RepoID, IsPrivate: true} if act.RepoUserName != ctx.User.LowerName { if has, _ := models.HasAccess(ctx.User, repo, models.ACCESS_MODE_READ); !has { continue @@ -216,7 +216,7 @@ func Profile(ctx *middleware.Context) { continue } // This prevents having to retrieve the repository for each action - repo := &models.Repository{Id: act.RepoId, IsPrivate: true} + repo := &models.Repository{Id: act.RepoID, IsPrivate: true} if act.RepoUserName != ctx.User.LowerName { if has, _ := models.HasAccess(ctx.User, repo, models.ACCESS_MODE_READ); !has { continue diff --git a/routers/user/setting.go b/routers/user/setting.go index a44d3b7e5..8d8ad0ad8 100644 --- a/routers/user/setting.go +++ b/routers/user/setting.go @@ -451,20 +451,13 @@ func SettingsDelete(ctx *middleware.Context) { ctx.Data["PageIsSettingsDelete"] = true if ctx.Req.Method == "POST" { - // tmpUser := models.User{ - // Passwd: ctx.Query("password"), - // Salt: ctx.User.Salt, - // } - // tmpUser.EncodePasswd() - // if tmpUser.Passwd != ctx.User.Passwd { - // ctx.Flash.Error("Password is not correct. Make sure you are owner of this account.") - // } else { + // FIXME: validate password. if err := models.DeleteUser(ctx.User); err != nil { - switch err { - case models.ErrUserOwnRepos: + switch { + case models.IsErrUserOwnRepos(err): ctx.Flash.Error(ctx.Tr("form.still_own_repo")) ctx.Redirect(setting.AppSubUrl + "/user/settings/delete") - case models.ErrUserHasOrgs: + case models.IsErrUserHasOrgs(err): ctx.Flash.Error(ctx.Tr("form.still_has_org")) ctx.Redirect(setting.AppSubUrl + "/user/settings/delete") default: