From 591c58c915cb3d7822c9d170e12089a632231ef7 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 16 Aug 2020 11:01:10 +0100 Subject: [PATCH 1/2] Fix bug preventing transfer to private organization The code assessing whether a private organization was visible to a user before allowing transfer was incorrect due to testing membership the wrong way round This PR fixes this issue and renames the function performing the test to be clearer. Further looking at the API for transfer repository - no testing was performed to ensure that the acting user could actually see the new owning organization. Signed-off-by: Andrew Thornton --- models/org.go | 2 +- models/user.go | 8 ++++---- routers/api/v1/repo/transfer.go | 11 ++++++++++- routers/repo/setting.go | 2 +- 4 files changed, 16 insertions(+), 7 deletions(-) diff --git a/models/org.go b/models/org.go index 0915e7fd56b9c..31e5cf81c99a3 100644 --- a/models/org.go +++ b/models/org.go @@ -435,7 +435,7 @@ func hasOrgVisible(e Engine, org *User, user *User) bool { return true } - if (org.Visibility == structs.VisibleTypePrivate || user.IsRestricted) && !org.isUserPartOfOrg(e, user.ID) { + if (org.Visibility == structs.VisibleTypePrivate || user.IsRestricted) && !org.hasMemberWithUserID(e, user.ID) { return false } return true diff --git a/models/user.go b/models/user.go index 6bd9d18b07735..1c1745393041c 100644 --- a/models/user.go +++ b/models/user.go @@ -610,12 +610,12 @@ func (u *User) IsUserOrgOwner(orgID int64) bool { return isOwner } -// IsUserPartOfOrg returns true if user with userID is part of the u organisation. -func (u *User) IsUserPartOfOrg(userID int64) bool { - return u.isUserPartOfOrg(x, userID) +// HasMemberWithUserID returns true if user with userID is part of the u organisation. +func (u *User) HasMemberWithUserID(userID int64) bool { + return u.hasMemberWithUserID(x, userID) } -func (u *User) isUserPartOfOrg(e Engine, userID int64) bool { +func (u *User) hasMemberWithUserID(e Engine, userID int64) bool { isMember, err := isOrganizationMember(e, u.ID, userID) if err != nil { log.Error("IsOrganizationMember: %v", err) diff --git a/routers/api/v1/repo/transfer.go b/routers/api/v1/repo/transfer.go index 847028d1067d5..b1271b77219f6 100644 --- a/routers/api/v1/repo/transfer.go +++ b/routers/api/v1/repo/transfer.go @@ -12,6 +12,7 @@ import ( "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/convert" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/structs" api "code.gitea.io/gitea/modules/structs" repo_service "code.gitea.io/gitea/services/repository" ) @@ -53,13 +54,21 @@ func Transfer(ctx *context.APIContext, opts api.TransferRepoOption) { newOwner, err := models.GetUserByName(opts.NewOwner) if err != nil { if models.IsErrUserNotExist(err) { - ctx.Error(http.StatusNotFound, "GetUserByName", err) + ctx.Error(http.StatusNotFound, "", "The new owner does not exist or cannot be found") return } ctx.InternalServerError(err) return } + if newOwner.Type == models.UserTypeOrganization { + if !ctx.User.IsAdmin && newOwner.Visibility == structs.VisibleTypePrivate && !newOwner.HasMemberWithUserID(ctx.User.ID) { + // The user shouldn't know about this organization + ctx.Error(http.StatusNotFound, "", "The new owner does not exist or cannot be found") + return + } + } + var teams []*models.Team if opts.TeamIDs != nil { if !newOwner.IsOrganization() { diff --git a/routers/repo/setting.go b/routers/repo/setting.go index 02331c232b081..e03bf556beb47 100644 --- a/routers/repo/setting.go +++ b/routers/repo/setting.go @@ -418,7 +418,7 @@ func SettingsPost(ctx *context.Context, form auth.RepoSettingForm) { } if newOwner.Type == models.UserTypeOrganization { - if !ctx.User.IsAdmin && newOwner.Visibility == structs.VisibleTypePrivate && !ctx.User.IsUserPartOfOrg(newOwner.ID) { + if !ctx.User.IsAdmin && newOwner.Visibility == structs.VisibleTypePrivate && !newOwner.HasMemberWithUserID(ctx.User.ID) { // The user shouldn't know about this organization ctx.RenderWithErr(ctx.Tr("form.enterred_invalid_owner_name"), tplSettingsOptions, nil) return From ad62c32c856742bd1639019e9e39af4c456e9ef1 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 16 Aug 2020 12:50:33 +0100 Subject: [PATCH 2/2] change IsUserPartOfOrg everywhere --- models/migrations/v111.go | 4 ++-- templates/user/profile.tmpl | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/models/migrations/v111.go b/models/migrations/v111.go index 66ff4843e52ac..6a94298ddcad7 100644 --- a/models/migrations/v111.go +++ b/models/migrations/v111.go @@ -148,7 +148,7 @@ func addBranchProtectionCanPushAndEnableWhitelist(x *xorm.Engine) error { if user == nil { hasOrgVisible = repoOwner.Visibility == VisibleTypePublic } else if !user.IsAdmin { - isUserPartOfOrg, err := sess. + hasMemberWithUserID, err := sess. Where("uid=?", user.ID). And("org_id=?", repoOwner.ID). Table("org_user"). @@ -156,7 +156,7 @@ func addBranchProtectionCanPushAndEnableWhitelist(x *xorm.Engine) error { if err != nil { hasOrgVisible = false } - if (repoOwner.Visibility == VisibleTypePrivate || user.IsRestricted) && !isUserPartOfOrg { + if (repoOwner.Visibility == VisibleTypePrivate || user.IsRestricted) && !hasMemberWithUserID { hasOrgVisible = false } } diff --git a/templates/user/profile.tmpl b/templates/user/profile.tmpl index bc2960b3b9c28..f453898f9460b 100644 --- a/templates/user/profile.tmpl +++ b/templates/user/profile.tmpl @@ -52,7 +52,7 @@
    • {{range .Orgs}} - {{if (or .Visibility.IsPublic (and ($.SignedUser) (or .Visibility.IsLimited (and (.IsUserPartOfOrg $.SignedUserID) .Visibility.IsPrivate) ($.IsAdmin))))}} + {{if (or .Visibility.IsPublic (and ($.SignedUser) (or .Visibility.IsLimited (and (.HasMemberWithUserID $.SignedUserID) .Visibility.IsPrivate) ($.IsAdmin))))}}