Allow admins and org owners to change org member public status (#28294)

Allows admins and org owners to change org member public status.

Before, this would return `Error 403: Cannot publicize another member`
despite the fact that the same user could make the same change through
the GUI.

Fixes #28372

---------

Co-authored-by: Tomáš Ženčák <[email protected]>
Co-authored-by: wxiaoguang <[email protected]>

Author: 3 people

Diff for: routers/api/v1/org/member.go

@@ -8,6 +8,7 @@ import (
 	"net/url"
 
 	"code.gitea.io/gitea/models/organization"
+	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/routers/api/v1/user"
@@ -210,6 +211,20 @@ func IsPublicMember(ctx *context.APIContext) {
 	}
 }
 
+func checkCanChangeOrgUserStatus(ctx *context.APIContext, targetUser *user_model.User) {
+	// allow user themselves to change their status, and allow admins to change any user
+	if targetUser.ID == ctx.Doer.ID || ctx.Doer.IsAdmin {
+		return
+	}
+	// allow org owners to change status of members
+	isOwner, err := ctx.Org.Organization.IsOwnedBy(ctx, ctx.Doer.ID)
+	if err != nil {
+		ctx.APIError(http.StatusInternalServerError, err)
+	} else if !isOwner {
+		ctx.APIError(http.StatusForbidden, "Cannot change member visibility")
+	}
+}
+
 // PublicizeMember make a member's membership public
 func PublicizeMember(ctx *context.APIContext) {
 	// swagger:operation PUT /orgs/{org}/public_members/{username} organization orgPublicizeMember
@@ -240,8 +255,8 @@ func PublicizeMember(ctx *context.APIContext) {
 	if ctx.Written() {
 		return
 	}
-	if userToPublicize.ID != ctx.Doer.ID {
-		ctx.APIError(http.StatusForbidden, "Cannot publicize another member")
+	checkCanChangeOrgUserStatus(ctx, userToPublicize)
+	if ctx.Written() {
 		return
 	}
 	err := organization.ChangeOrgUserStatus(ctx, ctx.Org.Organization.ID, userToPublicize.ID, true)
@@ -282,8 +297,8 @@ func ConcealMember(ctx *context.APIContext) {
 	if ctx.Written() {
 		return
 	}
-	if userToConceal.ID != ctx.Doer.ID {
-		ctx.APIError(http.StatusForbidden, "Cannot conceal another member")
+	checkCanChangeOrgUserStatus(ctx, userToConceal)
+	if ctx.Written() {
 		return
 	}
 	err := organization.ChangeOrgUserStatus(ctx, ctx.Org.Organization.ID, userToConceal.ID, false)

Diff for: tests/integration/api_org_test.go

@@ -22,6 +22,7 @@ import (
 	"code.gitea.io/gitea/tests"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestAPIOrgCreateRename(t *testing.T) {
@@ -110,121 +111,142 @@ func TestAPIOrgCreateRename(t *testing.T) {
 	})
 }
 
-func TestAPIOrgEdit(t *testing.T) {
+func TestAPIOrgGeneral(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
-	session := loginUser(t, "user1")
-
-	token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteOrganization)
-	org := api.EditOrgOption{
-		FullName:    "Org3 organization new full name",
-		Description: "A new description",
-		Website:     "https://try.gitea.io/new",
-		Location:    "Beijing",
-		Visibility:  "private",
-	}
-	req := NewRequestWithJSON(t, "PATCH", "/api/v1/orgs/org3", &org).
-		AddTokenAuth(token)
-	resp := MakeRequest(t, req, http.StatusOK)
+	user1Session := loginUser(t, "user1")
+	user1Token := getTokenForLoggedInUser(t, user1Session, auth_model.AccessTokenScopeWriteOrganization)
+
+	t.Run("OrgGetAll", func(t *testing.T) {
+		// accessing with a token will return all orgs
+		req := NewRequest(t, "GET", "/api/v1/orgs").AddTokenAuth(user1Token)
+		resp := MakeRequest(t, req, http.StatusOK)
+		var apiOrgList []*api.Organization
+
+		DecodeJSON(t, resp, &apiOrgList)
+		assert.Len(t, apiOrgList, 13)
+		assert.Equal(t, "Limited Org 36", apiOrgList[1].FullName)
+		assert.Equal(t, "limited", apiOrgList[1].Visibility)
+
+		// accessing without a token will return only public orgs
+		req = NewRequest(t, "GET", "/api/v1/orgs")
+		resp = MakeRequest(t, req, http.StatusOK)
 
-	var apiOrg api.Organization
-	DecodeJSON(t, resp, &apiOrg)
+		DecodeJSON(t, resp, &apiOrgList)
+		assert.Len(t, apiOrgList, 9)
+		assert.Equal(t, "org 17", apiOrgList[0].FullName)
+		assert.Equal(t, "public", apiOrgList[0].Visibility)
+	})
 
-	assert.Equal(t, "org3", apiOrg.Name)
-	assert.Equal(t, org.FullName, apiOrg.FullName)
-	assert.Equal(t, org.Description, apiOrg.Description)
-	assert.Equal(t, org.Website, apiOrg.Website)
-	assert.Equal(t, org.Location, apiOrg.Location)
-	assert.Equal(t, org.Visibility, apiOrg.Visibility)
-}
+	t.Run("OrgEdit", func(t *testing.T) {
+		org := api.EditOrgOption{
+			FullName:    "Org3 organization new full name",
+			Description: "A new description",
+			Website:     "https://try.gitea.io/new",
+			Location:    "Beijing",
+			Visibility:  "private",
+		}
+		req := NewRequestWithJSON(t, "PATCH", "/api/v1/orgs/org3", &org).AddTokenAuth(user1Token)
+		resp := MakeRequest(t, req, http.StatusOK)
+
+		var apiOrg api.Organization
+		DecodeJSON(t, resp, &apiOrg)
+
+		assert.Equal(t, "org3", apiOrg.Name)
+		assert.Equal(t, org.FullName, apiOrg.FullName)
+		assert.Equal(t, org.Description, apiOrg.Description)
+		assert.Equal(t, org.Website, apiOrg.Website)
+		assert.Equal(t, org.Location, apiOrg.Location)
+		assert.Equal(t, org.Visibility, apiOrg.Visibility)
+	})
 
-func TestAPIOrgEditBadVisibility(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
-	session := loginUser(t, "user1")
-
-	token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteOrganization)
-	org := api.EditOrgOption{
-		FullName:    "Org3 organization new full name",
-		Description: "A new description",
-		Website:     "https://try.gitea.io/new",
-		Location:    "Beijing",
-		Visibility:  "badvisibility",
-	}
-	req := NewRequestWithJSON(t, "PATCH", "/api/v1/orgs/org3", &org).
-		AddTokenAuth(token)
-	MakeRequest(t, req, http.StatusUnprocessableEntity)
-}
+	t.Run("OrgEditBadVisibility", func(t *testing.T) {
+		org := api.EditOrgOption{
+			FullName:    "Org3 organization new full name",
+			Description: "A new description",
+			Website:     "https://try.gitea.io/new",
+			Location:    "Beijing",
+			Visibility:  "badvisibility",
+		}
+		req := NewRequestWithJSON(t, "PATCH", "/api/v1/orgs/org3", &org).AddTokenAuth(user1Token)
+		MakeRequest(t, req, http.StatusUnprocessableEntity)
+	})
 
-func TestAPIOrgDeny(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
-	defer test.MockVariableValue(&setting.Service.RequireSignInViewStrict, true)()
+	t.Run("OrgDeny", func(t *testing.T) {
+		defer test.MockVariableValue(&setting.Service.RequireSignInViewStrict, true)()
 
-	orgName := "user1_org"
-	req := NewRequestf(t, "GET", "/api/v1/orgs/%s", orgName)
-	MakeRequest(t, req, http.StatusNotFound)
+		orgName := "user1_org"
+		req := NewRequestf(t, "GET", "/api/v1/orgs/%s", orgName)
+		MakeRequest(t, req, http.StatusNotFound)
 
-	req = NewRequestf(t, "GET", "/api/v1/orgs/%s/repos", orgName)
-	MakeRequest(t, req, http.StatusNotFound)
+		req = NewRequestf(t, "GET", "/api/v1/orgs/%s/repos", orgName)
+		MakeRequest(t, req, http.StatusNotFound)
 
-	req = NewRequestf(t, "GET", "/api/v1/orgs/%s/members", orgName)
-	MakeRequest(t, req, http.StatusNotFound)
-}
+		req = NewRequestf(t, "GET", "/api/v1/orgs/%s/members", orgName)
+		MakeRequest(t, req, http.StatusNotFound)
+	})
 
-func TestAPIGetAll(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
-	token := getUserToken(t, "user1", auth_model.AccessTokenScopeReadOrganization)
+	t.Run("OrgSearchEmptyTeam", func(t *testing.T) {
+		orgName := "org_with_empty_team"
+		// create org
+		req := NewRequestWithJSON(t, "POST", "/api/v1/orgs", &api.CreateOrgOption{
+			UserName: orgName,
+		}).AddTokenAuth(user1Token)
+		MakeRequest(t, req, http.StatusCreated)
+
+		// create team with no member
+		req = NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/orgs/%s/teams", orgName), &api.CreateTeamOption{
+			Name:                    "Empty",
+			IncludesAllRepositories: true,
+			Permission:              "read",
+			Units:                   []string{"repo.code", "repo.issues", "repo.ext_issues", "repo.wiki", "repo.pulls"},
+		}).AddTokenAuth(user1Token)
+		MakeRequest(t, req, http.StatusCreated)
+
+		// case-insensitive search for teams that have no members
+		req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/orgs/%s/teams/search?q=%s", orgName, "empty")).
+			AddTokenAuth(user1Token)
+		resp := MakeRequest(t, req, http.StatusOK)
+		data := struct {
+			Ok   bool
+			Data []*api.Team
+		}{}
+		DecodeJSON(t, resp, &data)
+		assert.True(t, data.Ok)
+		if assert.Len(t, data.Data, 1) {
+			assert.Equal(t, "Empty", data.Data[0].Name)
+		}
+	})
 
-	// accessing with a token will return all orgs
-	req := NewRequest(t, "GET", "/api/v1/orgs").
-		AddTokenAuth(token)
-	resp := MakeRequest(t, req, http.StatusOK)
-	var apiOrgList []*api.Organization
+	t.Run("User2ChangeStatus", func(t *testing.T) {
+		user2Session := loginUser(t, "user2")
+		user2Token := getTokenForLoggedInUser(t, user2Session, auth_model.AccessTokenScopeWriteOrganization)
 
-	DecodeJSON(t, resp, &apiOrgList)
-	assert.Len(t, apiOrgList, 13)
-	assert.Equal(t, "Limited Org 36", apiOrgList[1].FullName)
-	assert.Equal(t, "limited", apiOrgList[1].Visibility)
+		req := NewRequest(t, "PUT", "/api/v1/orgs/org3/public_members/user2").AddTokenAuth(user2Token)
+		MakeRequest(t, req, http.StatusNoContent)
+		req = NewRequest(t, "DELETE", "/api/v1/orgs/org3/public_members/user2").AddTokenAuth(user2Token)
+		MakeRequest(t, req, http.StatusNoContent)
 
-	// accessing without a token will return only public orgs
-	req = NewRequest(t, "GET", "/api/v1/orgs")
-	resp = MakeRequest(t, req, http.StatusOK)
+		// non admin but org owner could also change other member's status
+		user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user2"})
+		require.False(t, user2.IsAdmin)
+		req = NewRequest(t, "PUT", "/api/v1/orgs/org3/public_members/user1").AddTokenAuth(user2Token)
+		MakeRequest(t, req, http.StatusNoContent)
+		req = NewRequest(t, "DELETE", "/api/v1/orgs/org3/public_members/user1").AddTokenAuth(user2Token)
+		MakeRequest(t, req, http.StatusNoContent)
+	})
 
-	DecodeJSON(t, resp, &apiOrgList)
-	assert.Len(t, apiOrgList, 9)
-	assert.Equal(t, "org 17", apiOrgList[0].FullName)
-	assert.Equal(t, "public", apiOrgList[0].Visibility)
-}
+	t.Run("User4ChangeStatus", func(t *testing.T) {
+		user4Session := loginUser(t, "user4")
+		user4Token := getTokenForLoggedInUser(t, user4Session, auth_model.AccessTokenScopeWriteOrganization)
 
-func TestAPIOrgSearchEmptyTeam(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
-	token := getUserToken(t, "user1", auth_model.AccessTokenScopeWriteOrganization)
-	orgName := "org_with_empty_team"
-
-	// create org
-	req := NewRequestWithJSON(t, "POST", "/api/v1/orgs", &api.CreateOrgOption{
-		UserName: orgName,
-	}).AddTokenAuth(token)
-	MakeRequest(t, req, http.StatusCreated)
-
-	// create team with no member
-	req = NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/orgs/%s/teams", orgName), &api.CreateTeamOption{
-		Name:                    "Empty",
-		IncludesAllRepositories: true,
-		Permission:              "read",
-		Units:                   []string{"repo.code", "repo.issues", "repo.ext_issues", "repo.wiki", "repo.pulls"},
-	}).AddTokenAuth(token)
-	MakeRequest(t, req, http.StatusCreated)
-
-	// case-insensitive search for teams that have no members
-	req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/orgs/%s/teams/search?q=%s", orgName, "empty")).
-		AddTokenAuth(token)
-	resp := MakeRequest(t, req, http.StatusOK)
-	data := struct {
-		Ok   bool
-		Data []*api.Team
-	}{}
-	DecodeJSON(t, resp, &data)
-	assert.True(t, data.Ok)
-	if assert.Len(t, data.Data, 1) {
-		assert.Equal(t, "Empty", data.Data[0].Name)
-	}
+		// user4 is a normal team member, they could change their own status
+		req := NewRequest(t, "PUT", "/api/v1/orgs/org3/public_members/user4").AddTokenAuth(user4Token)
+		MakeRequest(t, req, http.StatusNoContent)
+		req = NewRequest(t, "DELETE", "/api/v1/orgs/org3/public_members/user4").AddTokenAuth(user4Token)
+		MakeRequest(t, req, http.StatusNoContent)
+		req = NewRequest(t, "PUT", "/api/v1/orgs/org3/public_members/user1").AddTokenAuth(user4Token)
+		MakeRequest(t, req, http.StatusForbidden)
+		req = NewRequest(t, "DELETE", "/api/v1/orgs/org3/public_members/user1").AddTokenAuth(user4Token)
+		MakeRequest(t, req, http.StatusForbidden)
+	})
 }

Diff for: tests/integration/api_team_user_test.go

@@ -21,29 +21,31 @@ import (
 
 func TestAPITeamUser(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
-
-	normalUsername := "user2"
-	session := loginUser(t, normalUsername)
-	token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadOrganization)
-	req := NewRequest(t, "GET", "/api/v1/teams/1/members/user1").
-		AddTokenAuth(token)
-	MakeRequest(t, req, http.StatusNotFound)
-
-	req = NewRequest(t, "GET", "/api/v1/teams/1/members/user2").
-		AddTokenAuth(token)
-	resp := MakeRequest(t, req, http.StatusOK)
-	var user2 *api.User
-	DecodeJSON(t, resp, &user2)
-	user2.Created = user2.Created.In(time.Local)
-	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user2"})
-
-	expectedUser := convert.ToUser(db.DefaultContext, user, user)
-
-	// test time via unix timestamp
-	assert.Equal(t, expectedUser.LastLogin.Unix(), user2.LastLogin.Unix())
-	assert.Equal(t, expectedUser.Created.Unix(), user2.Created.Unix())
-	expectedUser.LastLogin = user2.LastLogin
-	expectedUser.Created = user2.Created
-
-	assert.Equal(t, expectedUser, user2)
+	user2Session := loginUser(t, "user2")
+	user2Token := getTokenForLoggedInUser(t, user2Session, auth_model.AccessTokenScopeWriteOrganization)
+
+	t.Run("User2ReadUser1", func(t *testing.T) {
+		req := NewRequest(t, "GET", "/api/v1/teams/1/members/user1").AddTokenAuth(user2Token)
+		MakeRequest(t, req, http.StatusNotFound)
+	})
+
+	t.Run("User2ReadSelf", func(t *testing.T) {
+		// read self user
+		req := NewRequest(t, "GET", "/api/v1/teams/1/members/user2").AddTokenAuth(user2Token)
+		resp := MakeRequest(t, req, http.StatusOK)
+		var user2 *api.User
+		DecodeJSON(t, resp, &user2)
+		user2.Created = user2.Created.In(time.Local)
+		user := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user2"})
+
+		expectedUser := convert.ToUser(db.DefaultContext, user, user)
+
+		// test time via unix timestamp
+		assert.Equal(t, expectedUser.LastLogin.Unix(), user2.LastLogin.Unix())
+		assert.Equal(t, expectedUser.Created.Unix(), user2.Created.Unix())
+		expectedUser.LastLogin = user2.LastLogin
+		expectedUser.Created = user2.Created
+
+		assert.Equal(t, expectedUser, user2)
+	})
 }