From f9203f835542f28cc50d73743ede4525cc78cbca 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 fca38bad0fe7ba305b39ed95a317de375e4e7200 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]) + }) }