mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2026-05-14 06:50:25 +00:00
Fixes #1529. This adds an "Add member" button to the list of members of an organization, offering a more intuitive way to add a user to an organization (instead of going through the list of teams). This follows the design proposed in #1529. This PR can already be reviewed as such, but I plan to work on follow-up improvements: - adding a confirmation dialog when adding the new member to the "Owners" team, since they get absolute rights on the org - adding a text input to filter the list of teams, making it easier to select the desired teams when there are many of them - potentially, improving the team creation link so that it brings the user back to the modal dialog once the team is created (but I'm not sure there's a ton of value behind this added complexity, since currently, creating a team will lead the user to the team page, which is a good place to add the member to the team) This new way of adding members does not support issuing email invites, since we decided in #9884 that the invite feature hasn't got good enough of a UX to advertise it yet. Following [this discussion](https://codeberg.org/forgejo/discussions/issues/441), I am planning to work on enabling invites everywhere (potentially even making it the default). ## Checklist ### Tests for Go changes - I added test coverage for Go changes... - [ ] in their respective `*_test.go` for unit tests. - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I ran... - [x] `make pr-go` before pushing ### Tests for JavaScript changes - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [x] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation I plan to update https://docs.codeberg.org/collaborating/create-organization/#people once we are ready to take final screenshots of the feature. ### Release notes - [x] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change. - [ ] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change. ### Screenshots <!--start release-notes-assistant--> ## Release notes <!--URL:https://codeberg.org/forgejo/forgejo--> - Features - [PR](https://codeberg.org/forgejo/forgejo/pulls/11848): <!--number 11848 --><!--line 0 --><!--description IkFkZCBtZW1iZXIiIGJ1dHRvbiBpbiBvcmcgbWVtYmVycyBsaXN0-->"Add member" button in org members list<!--description--> <!--end release-notes-assistant--> Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11848 Reviewed-by: 0ko <0ko@noreply.codeberg.org> Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org> Co-authored-by: Antonin Delpeuch <antonin@delpeuch.eu> Co-committed-by: Antonin Delpeuch <antonin@delpeuch.eu>
223 lines
8.4 KiB
Go
223 lines
8.4 KiB
Go
// Copyright 2026 The Forgejo Authors
|
|
// SPDX-License-Identifier: GPL-3.0-or-later
|
|
|
|
package integration
|
|
|
|
import (
|
|
"fmt"
|
|
"net/http"
|
|
"strings"
|
|
"testing"
|
|
|
|
"forgejo.org/models/db"
|
|
"forgejo.org/models/organization"
|
|
"forgejo.org/models/unittest"
|
|
user_model "forgejo.org/models/user"
|
|
"forgejo.org/tests"
|
|
|
|
"github.com/stretchr/testify/assert"
|
|
"github.com/stretchr/testify/require"
|
|
)
|
|
|
|
func TestOrgMembersPage(t *testing.T) {
|
|
defer tests.PrepareTestEnv(t)()
|
|
|
|
testPage := "/org/org3/members"
|
|
|
|
t.Run("Guest PoV", func(t *testing.T) {
|
|
defer tests.PrintCurrentTest(t)()
|
|
|
|
doc := NewHTMLParser(t, MakeRequest(t, NewRequest(t, "GET", testPage), http.StatusOK).Body)
|
|
/* No interactive buttons - though such evaluation is easy to break in rename */
|
|
assert.Equal(t, 0, doc.Find(".members .list .link-action").Length())
|
|
assert.Equal(t, 0, doc.Find(".members .list .delete-button").Length())
|
|
/* Guests cannot add members to organizations */
|
|
doc.AssertElement(t, "#add-org-member-button", false)
|
|
})
|
|
|
|
t.Run("Member PoV", func(t *testing.T) {
|
|
defer tests.PrintCurrentTest(t)()
|
|
|
|
session := loginUser(t, "user4") // user4 is a member of org3
|
|
doc := NewHTMLParser(t, session.MakeRequest(t, NewRequest(t, "GET", testPage), http.StatusOK).Body)
|
|
/* Interactive buttons are only available for own entry in the list */
|
|
assert.Equal(t, 1, doc.Find(".members .list .link-action").Length())
|
|
assert.Equal(t, 1, doc.Find(".members .list .delete-button").Length())
|
|
/* Adding new members is not possible, as the member isn't an owner */
|
|
doc.AssertElement(t, "#add-org-member-button", false)
|
|
})
|
|
|
|
t.Run("Owner PoV", func(t *testing.T) {
|
|
defer tests.PrintCurrentTest(t)()
|
|
|
|
session := loginUser(t, "user2") // user2 owns org3
|
|
doc := NewHTMLParser(t, session.MakeRequest(t, NewRequest(t, "GET", testPage), http.StatusOK).Body)
|
|
/* Interactive buttons are available for all entries in the list (> 2) */
|
|
assert.Less(t, 2, doc.Find(".members .list .link-action").Length())
|
|
assert.Less(t, 2, doc.Find(".members .list .delete-button").Length())
|
|
/* Adding new members is possible */
|
|
doc.AssertElement(t, "#add-org-member-button", true)
|
|
})
|
|
}
|
|
|
|
func TestOrgAddMember(t *testing.T) {
|
|
defer tests.PrepareTestEnv(t)()
|
|
|
|
org := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3})
|
|
team1 := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: 1})
|
|
team2 := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: 2})
|
|
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5})
|
|
|
|
isMember, err := organization.IsTeamMember(db.DefaultContext, team1.OrgID, team1.ID, user.ID)
|
|
require.NoError(t, err)
|
|
assert.False(t, isMember)
|
|
isMember, err = organization.IsTeamMember(db.DefaultContext, team2.OrgID, team2.ID, user.ID)
|
|
require.NoError(t, err)
|
|
assert.False(t, isMember)
|
|
|
|
session := loginUser(t, "user2")
|
|
|
|
teamURL := fmt.Sprintf("/org/%s/members", org.Name)
|
|
req := NewRequestWithValues(t, "POST", teamURL+"/action/add", map[string]string{
|
|
"uid": "2",
|
|
"uname": user.LoginName,
|
|
"team_1": "on",
|
|
"team_2": "on",
|
|
})
|
|
resp := session.MakeRequest(t, req, http.StatusSeeOther)
|
|
assert.Equal(t, teamURL, resp.Header().Get("Location"))
|
|
|
|
user = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5})
|
|
isMember, err = organization.IsTeamMember(db.DefaultContext, team1.OrgID, team1.ID, user.ID)
|
|
require.NoError(t, err)
|
|
assert.True(t, isMember)
|
|
isMember, err = organization.IsTeamMember(db.DefaultContext, team2.OrgID, team2.ID, user.ID)
|
|
require.NoError(t, err)
|
|
assert.True(t, isMember)
|
|
}
|
|
|
|
func TestOrgAddMemberToNoTeam(t *testing.T) {
|
|
defer tests.PrepareTestEnv(t)()
|
|
|
|
org := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3})
|
|
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5})
|
|
|
|
isOrgMember, err := org.IsOrgMember(db.DefaultContext, user.ID)
|
|
require.NoError(t, err)
|
|
assert.False(t, isOrgMember)
|
|
|
|
session := loginUser(t, "user2")
|
|
|
|
teamURL := fmt.Sprintf("/org/%s/members", org.Name)
|
|
resp := session.MakeRequest(t, NewRequestWithValues(t, "POST", teamURL+"/action/add", map[string]string{
|
|
"uid": "2",
|
|
"uname": user.LoginName,
|
|
}), http.StatusSeeOther)
|
|
assert.Equal(t, teamURL, resp.Header().Get("Location"))
|
|
|
|
doc := NewHTMLParser(t, session.MakeRequest(t, NewRequest(t, "GET", teamURL), http.StatusOK).Body)
|
|
assert.Contains(t, strings.TrimSpace(doc.Find(".flash-error").Text()), "Organization members must belong to at least one team.")
|
|
|
|
isOrgMember, err = org.IsOrgMember(db.DefaultContext, user.ID)
|
|
require.NoError(t, err)
|
|
assert.False(t, isOrgMember)
|
|
}
|
|
|
|
func TestOrgAddMemberToForeignTeam(t *testing.T) {
|
|
defer tests.PrepareTestEnv(t)()
|
|
|
|
org := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3})
|
|
foreignTeam := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: 5})
|
|
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5})
|
|
|
|
isOrgMember, err := org.IsOrgMember(db.DefaultContext, user.ID)
|
|
require.NoError(t, err)
|
|
assert.False(t, isOrgMember)
|
|
|
|
isForeignTeamMember, err := organization.IsTeamMember(db.DefaultContext, foreignTeam.OrgID, foreignTeam.ID, user.ID)
|
|
require.NoError(t, err)
|
|
assert.False(t, isForeignTeamMember)
|
|
|
|
session := loginUser(t, "user2")
|
|
|
|
teamURL := fmt.Sprintf("/org/%s/members", org.Name)
|
|
// This test scenario is here for security purposes as the UI will not offer adding the user to
|
|
// a foreign team in the first place (but a malicious request could be performed in other ways).
|
|
resp := session.MakeRequest(t, NewRequestWithValues(t, "POST", teamURL+"/action/add", map[string]string{
|
|
"uid": "2",
|
|
"uname": user.LoginName,
|
|
"team_5": "on",
|
|
}), http.StatusSeeOther)
|
|
assert.Equal(t, teamURL, resp.Header().Get("Location"))
|
|
|
|
doc := NewHTMLParser(t, session.MakeRequest(t, NewRequest(t, "GET", teamURL), http.StatusOK).Body)
|
|
// This error message isn't specific to this "exploit" attempt, but shows that no action was taken.
|
|
assert.Contains(t, strings.TrimSpace(doc.Find(".flash-error").Text()), "Organization members must belong to at least one team.")
|
|
|
|
isOrgMember, err = org.IsOrgMember(db.DefaultContext, user.ID)
|
|
require.NoError(t, err)
|
|
assert.False(t, isOrgMember)
|
|
|
|
isForeignTeamMember, err = organization.IsTeamMember(db.DefaultContext, foreignTeam.OrgID, foreignTeam.ID, user.ID)
|
|
require.NoError(t, err)
|
|
assert.False(t, isForeignTeamMember)
|
|
}
|
|
|
|
func TestOrgAddMemberWithoutProperRights(t *testing.T) {
|
|
defer tests.PrepareTestEnv(t)()
|
|
|
|
org := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3})
|
|
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5})
|
|
|
|
isOrgMember, err := org.IsOrgMember(db.DefaultContext, user.ID)
|
|
require.NoError(t, err)
|
|
assert.False(t, isOrgMember)
|
|
|
|
session := loginUser(t, user.Name) // user5 is not a member of this org, so it cannot add anyone to it
|
|
|
|
teamURL := fmt.Sprintf("/org/%s/members", org.Name)
|
|
req := NewRequestWithValues(t, "POST", teamURL+"/action/add", map[string]string{
|
|
"uid": "5",
|
|
"uname": user.LoginName,
|
|
"team_2": "on",
|
|
})
|
|
session.MakeRequest(t, req, http.StatusNotFound)
|
|
}
|
|
|
|
func TestOrgAddExistingMemberFails(t *testing.T) {
|
|
defer tests.PrepareTestEnv(t)()
|
|
|
|
org := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3})
|
|
team := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: 2})
|
|
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 28})
|
|
|
|
members, _, err := organization.FindOrgMembers(db.DefaultContext, &organization.FindOrgMembersOpts{
|
|
OrgID: org.ID,
|
|
})
|
|
require.NoError(t, err)
|
|
assert.Len(t, members, 2)
|
|
isOrgMember, err := org.IsOrgMember(db.DefaultContext, user.ID)
|
|
require.NoError(t, err)
|
|
assert.True(t, isOrgMember)
|
|
isTeamMember, err := organization.IsTeamMember(db.DefaultContext, team.OrgID, team.ID, user.ID)
|
|
require.NoError(t, err)
|
|
assert.False(t, isTeamMember)
|
|
|
|
session := loginUser(t, "user2")
|
|
|
|
teamURL := fmt.Sprintf("/org/%s/members", org.Name)
|
|
req := NewRequestWithValues(t, "POST", teamURL+"/action/add", map[string]string{
|
|
"uid": "2",
|
|
"uname": user.LoginName,
|
|
"team_2": "on",
|
|
})
|
|
resp := session.MakeRequest(t, req, http.StatusSeeOther)
|
|
assert.Equal(t, teamURL, resp.Header().Get("Location"))
|
|
doc := NewHTMLParser(t, session.MakeRequest(t, NewRequest(t, "GET", teamURL), http.StatusOK).Body)
|
|
assert.Contains(t, strings.TrimSpace(doc.Find(".flash-error").Text()), "This user is already a member of the organization.")
|
|
|
|
user = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 28})
|
|
isTeamMember, err = organization.IsTeamMember(db.DefaultContext, team.OrgID, team.ID, user.ID)
|
|
require.NoError(t, err)
|
|
assert.False(t, isTeamMember)
|
|
}
|