diff --git a/docs/docs/30-administration/10-configuration/10-server.md b/docs/docs/30-administration/10-configuration/10-server.md index 1f95760df..f8162cccc 100644 --- a/docs/docs/30-administration/10-configuration/10-server.md +++ b/docs/docs/30-administration/10-configuration/10-server.md @@ -18,7 +18,7 @@ You can also restrict the registration: WOODPECKER_ADMIN=john.smith,jane_doe ``` -- open registration and filtering by organizational affiliation with the setting `WOODPECKER_ORGS` +- open registration and filtering by organizational affiliation with the setting `WOODPECKER_ORGS`. For GitLab forges group names have to exact match, so members of group `group` are not permitted access if you set `WOODPECKER_ORGS=group/subgroup`. ```ini WOODPECKER_OPEN=true @@ -712,7 +712,7 @@ Example: `WOODPECKER_ADMIN=user1,user2` - Name: `WOODPECKER_ORGS` - Default: none -Comma-separated list of approved organizations. +Comma-separated list of approved organizations. For GitLab forges this is the `full_path` [attribute](https://docs.gitlab.com/api/groups/). Example: `org1,org2` diff --git a/server/forge/gitlab/fixtures/groups.go b/server/forge/gitlab/fixtures/groups.go new file mode 100644 index 000000000..c3ad6fcdd --- /dev/null +++ b/server/forge/gitlab/fixtures/groups.go @@ -0,0 +1,42 @@ +// Copyright 2026 Woodpecker Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package fixtures + +// Two groups both named "woodpecker": the real top-level one and one an attacker +// created below their own namespace. They are only distinguishable by full_path. +var groupsPayload = []byte(` +[ + { + "id": 1, + "name": "woodpecker", + "path": "woodpecker", + "full_name": "woodpecker", + "full_path": "woodpecker", + "parent_id": null, + "web_url": "https://gitlab.com/groups/woodpecker", + "avatar_url": "https://gitlab.com/uploads/group/avatar/1/woodpecker.png" + }, + { + "id": 2, + "name": "woodpecker", + "path": "woodpecker", + "full_name": "eve / woodpecker", + "full_path": "eve/woodpecker", + "parent_id": 99, + "web_url": "https://gitlab.com/groups/eve/woodpecker", + "avatar_url": "https://gitlab.com/uploads/group/avatar/2/woodpecker.png" + } +] +`) diff --git a/server/forge/gitlab/fixtures/testdata.go b/server/forge/gitlab/fixtures/testdata.go index c37a3bbb7..d00a892dd 100644 --- a/server/forge/gitlab/fixtures/testdata.go +++ b/server/forge/gitlab/fixtures/testdata.go @@ -76,6 +76,9 @@ func NewServer(t *testing.T) *httptest.Server { case "/api/v4/user": _, _ = w.Write(currentUserPayload) return + case "/api/v4/groups": + _, _ = w.Write(groupsPayload) + return } // else return a 404 diff --git a/server/forge/gitlab/gitlab.go b/server/forge/gitlab/gitlab.go index 30e127607..bb8515f23 100644 --- a/server/forge/gitlab/gitlab.go +++ b/server/forge/gitlab/gitlab.go @@ -203,7 +203,7 @@ func (g *GitLab) Teams(ctx context.Context, user *model.User, p *model.ListOptio for i := range groups { teams = append( teams, &model.Team{ - Login: groups[i].Name, + Login: groups[i].FullPath, Avatar: groups[i].AvatarURL, }, ) diff --git a/server/forge/gitlab/gitlab_test.go b/server/forge/gitlab/gitlab_test.go index 74bcfa36f..2c4304b25 100644 --- a/server/forge/gitlab/gitlab_test.go +++ b/server/forge/gitlab/gitlab_test.go @@ -104,6 +104,19 @@ func Test_GitLab(t *testing.T) { assert.True(t, _repo.Perm.Push) }) + // Test teams membership method + t.Run("Should identify groups by full path, not display name", func(t *testing.T) { + teams, err := client.Teams(ctx, &user, &model.ListOptions{Page: 1, PerPage: 10}) + assert.NoError(t, err) + + logins := make([]string, 0, len(teams)) + for _, team := range teams { + logins = append(logins, team.Login) + } + // both groups are named "woodpecker", only the full path tells them apart + assert.Equal(t, []string{"woodpecker", "eve/woodpecker"}, logins) + }) + // Test activate method t.Run("Activate, success", func(t *testing.T) { err := client.Activate(ctx, &user, &repo, "http://example.com/api/hook?access_token=token") diff --git a/server/services/permissions/orgs_test.go b/server/services/permissions/orgs_test.go index 0a1852387..326c315f1 100644 --- a/server/services/permissions/orgs_test.go +++ b/server/services/permissions/orgs_test.go @@ -32,3 +32,18 @@ func TestOrgs(t *testing.T) { assert.False(t, empty.IsMember([]*model.Team{{Login: "woodpecker-ci"}})) assert.False(t, empty.IsMember([]*model.Team{{Login: "not-woodpecker-ci"}})) } + +func TestOrgsRejectsSameNamedGroup(t *testing.T) { + org := NewOrgs([]string{"woodpecker"}) + + // the real top-level group + assert.True(t, org.IsMember([]*model.Team{{Login: "woodpecker"}})) + + // anyone can create a group named "woodpecker" below their own namespace, + // membership there must never satisfy an allowed org of "woodpecker" + assert.False(t, org.IsMember([]*model.Team{{Login: "eve/woodpecker"}})) + assert.False(t, org.IsMember([]*model.Team{{Login: "eve/sub/woodpecker"}})) + + // subgroups are not matched either, only exact membership counts + assert.False(t, org.IsMember([]*model.Team{{Login: "woodpecker/infra"}})) +}