From 61ac2b5f0fc83a68e6e42e0f714b14bf82ab71eb 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 f9dc08a90ba26..4b4510a6cdf42 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 c8675b1ded5bc..d81dfba288e2c 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]) + }) }