From 2f3da6d6b39643c848db86f77a267edb0247f4f8 Mon Sep 17 00:00:00 2001 From: Bjarne Koll Date: Wed, 17 Sep 2025 21:26:58 +0200 Subject: [PATCH] Correctly override user unitmodes (#35501) Commit 6a97ab0af4031dd1e8fb0b272218e146b5556ac6 reworked team permission application. The introduced logic overrode the unitModes for *every* team a user is in, max(...) the current value and the team value together. The logic completely fails in case the team does not have a unit for the specific unit type defined, in which case the logic inserted the minimumVisibility, overriding any previous aggregation of access modes for the unit. This is resolved by simply always merging the unit access mode of the team as it will simply default to None in case the team does not have a permission defined for the unit, which will be swallowed by the max(..) call in favour of the previous aggregated permission. --- models/perm/access/repo_permission.go | 6 ++-- models/perm/access/repo_permission_test.go | 33 ++++++++++++++++++++++ 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/models/perm/access/repo_permission.go b/models/perm/access/repo_permission.go index 7de43ecd07..678b18442e 100644 --- a/models/perm/access/repo_permission.go +++ b/models/perm/access/repo_permission.go @@ -348,10 +348,8 @@ func GetUserRepoPermission(ctx context.Context, repo *repo_model.Repository, use for _, u := range repo.Units { for _, team := range teams { - unitAccessMode := minAccessMode - if teamMode, exist := team.UnitAccessModeEx(ctx, u.Type); exist { - unitAccessMode = max(perm.unitsMode[u.Type], unitAccessMode, teamMode) - } + teamMode, _ := team.UnitAccessModeEx(ctx, u.Type) + unitAccessMode := max(perm.unitsMode[u.Type], minAccessMode, teamMode) perm.unitsMode[u.Type] = unitAccessMode } } diff --git a/models/perm/access/repo_permission_test.go b/models/perm/access/repo_permission_test.go index c8675b1ded..d81dfba288 100644 --- a/models/perm/access/repo_permission_test.go +++ b/models/perm/access/repo_permission_test.go @@ -197,4 +197,37 @@ func TestGetUserRepoPermission(t *testing.T) { assert.Equal(t, perm_model.AccessModeWrite, perm.unitsMode[unit.TypeCode]) assert.Equal(t, perm_model.AccessModeRead, perm.unitsMode[unit.TypeIssues]) }) + + repo3 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) // org private repo, same org as repo 32 + require.NoError(t, repo3.LoadOwner(ctx)) + require.True(t, repo3.Owner.IsOrganization()) + require.NoError(t, db.TruncateBeans(ctx, &organization.TeamUnit{}, &Access{})) // The user has access set of that repo, remove it, it is useless for our test + require.NoError(t, db.Insert(ctx, &organization.TeamRepo{OrgID: org.ID, TeamID: team.ID, RepoID: repo3.ID})) + t.Run("DoerWithNoopTeamOnPrivateRepo", func(t *testing.T) { + perm, err := GetUserRepoPermission(ctx, repo3, user) + require.NoError(t, err) + assert.Equal(t, perm_model.AccessModeNone, perm.AccessMode) + assert.Equal(t, perm_model.AccessModeNone, perm.unitsMode[unit.TypeCode]) + assert.Equal(t, perm_model.AccessModeNone, perm.unitsMode[unit.TypeIssues]) + }) + + require.NoError(t, db.Insert(ctx, &organization.TeamUnit{OrgID: org.ID, TeamID: team.ID, Type: unit.TypeCode, AccessMode: perm_model.AccessModeNone})) + require.NoError(t, db.Insert(ctx, &organization.TeamUnit{OrgID: org.ID, TeamID: team.ID, Type: unit.TypeIssues, AccessMode: perm_model.AccessModeRead})) + t.Run("DoerWithReadIssueTeamOnPrivateRepo", func(t *testing.T) { + perm, err := GetUserRepoPermission(ctx, repo3, user) + require.NoError(t, err) + assert.Equal(t, perm_model.AccessModeNone, perm.AccessMode) + assert.Equal(t, perm_model.AccessModeNone, perm.unitsMode[unit.TypeCode]) + assert.Equal(t, perm_model.AccessModeRead, perm.unitsMode[unit.TypeIssues]) + }) + + require.NoError(t, db.Insert(ctx, repo_model.Collaboration{RepoID: repo3.ID, UserID: user.ID, Mode: perm_model.AccessModeWrite})) + require.NoError(t, db.Insert(ctx, Access{RepoID: repo3.ID, UserID: user.ID, Mode: perm_model.AccessModeWrite})) + t.Run("DoerWithReadIssueTeamAndWriteCollaboratorOnPrivateRepo", func(t *testing.T) { + perm, err := GetUserRepoPermission(ctx, repo3, user) + require.NoError(t, err) + assert.Equal(t, perm_model.AccessModeWrite, perm.AccessMode) + assert.Equal(t, perm_model.AccessModeWrite, perm.unitsMode[unit.TypeCode]) + assert.Equal(t, perm_model.AccessModeWrite, perm.unitsMode[unit.TypeIssues]) + }) }