Skip to content

Commit 48d4580

Browse files
wxiaoguangGiteaBot
wxiaoguang
and
GiteaBot
authored
Clarify permission "HasAccess" behavior (#30585)
Follow #30495 "HasAccess" behavior wasn't clear, to make it clear: * Use a new name `HasAnyUnitAccess`, it will be easier to review related code and permission problems. * Separate everyone access mode to a separate field, then all calls to HasAccess are reverted to old behavior before #30495. * Add new tests. --------- Co-authored-by: Giteabot <[email protected]>
1 parent 89e3987 commit 48d4580

File tree

13 files changed

+96
-41
lines changed

13 files changed

+96
-41
lines changed

‎models/org_team.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ func removeAllRepositories(ctx context.Context, t *organization.Team) (err error
118118

119119
// Remove watches from all users and now unaccessible repos
120120
for _, user := range t.Members {
121-
has, err := access_model.HasAccess(ctx, user.ID, repo)
121+
has, err := access_model.HasAnyUnitAccess(ctx, user.ID, repo)
122122
if err != nil {
123123
return err
124124
} else if has {
@@ -544,7 +544,7 @@ func ReconsiderRepoIssuesAssignee(ctx context.Context, repo *repo_model.Reposito
544544
}
545545

546546
func ReconsiderWatches(ctx context.Context, repo *repo_model.Repository, user *user_model.User) error {
547-
if has, err := access_model.HasAccess(ctx, user.ID, repo); err != nil || has {
547+
if has, err := access_model.HasAnyUnitAccess(ctx, user.ID, repo); err != nil || has {
548548
return err
549549
}
550550
if err := repo_model.WatchRepo(ctx, user, repo, false); err != nil {

‎models/perm/access/access_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -79,17 +79,17 @@ func TestHasAccess(t *testing.T) {
7979
repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3})
8080
assert.True(t, repo2.IsPrivate)
8181

82-
has, err := access_model.HasAccess(db.DefaultContext, user1.ID, repo1)
82+
has, err := access_model.HasAnyUnitAccess(db.DefaultContext, user1.ID, repo1)
8383
assert.NoError(t, err)
8484
assert.True(t, has)
8585

86-
_, err = access_model.HasAccess(db.DefaultContext, user1.ID, repo2)
86+
_, err = access_model.HasAnyUnitAccess(db.DefaultContext, user1.ID, repo2)
8787
assert.NoError(t, err)
8888

89-
_, err = access_model.HasAccess(db.DefaultContext, user2.ID, repo1)
89+
_, err = access_model.HasAnyUnitAccess(db.DefaultContext, user2.ID, repo1)
9090
assert.NoError(t, err)
9191

92-
_, err = access_model.HasAccess(db.DefaultContext, user2.ID, repo2)
92+
_, err = access_model.HasAnyUnitAccess(db.DefaultContext, user2.ID, repo2)
9393
assert.NoError(t, err)
9494
}
9595

‎models/perm/access/repo_permission.go

Lines changed: 37 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ type Permission struct {
2424

2525
units []*repo_model.RepoUnit
2626
unitsMode map[unit.Type]perm_model.AccessMode
27+
28+
everyoneAccessMode map[unit.Type]perm_model.AccessMode
2729
}
2830

2931
// IsOwner returns true if current user is the owner of repository.
@@ -36,9 +38,24 @@ func (p *Permission) IsAdmin() bool {
3638
return p.AccessMode >= perm_model.AccessModeAdmin
3739
}
3840

39-
// HasAccess returns true if the current user might have at least read access to any unit of this repository
40-
func (p *Permission) HasAccess() bool {
41-
return len(p.unitsMode) > 0 || p.AccessMode >= perm_model.AccessModeRead
41+
// HasAnyUnitAccess returns true if the user might have at least one access mode to any unit of this repository.
42+
// It doesn't count the "everyone access mode".
43+
func (p *Permission) HasAnyUnitAccess() bool {
44+
for _, v := range p.unitsMode {
45+
if v >= perm_model.AccessModeRead {
46+
return true
47+
}
48+
}
49+
return p.AccessMode >= perm_model.AccessModeRead
50+
}
51+
52+
func (p *Permission) HasAnyUnitAccessOrEveryoneAccess() bool {
53+
for _, v := range p.everyoneAccessMode {
54+
if v >= perm_model.AccessModeRead {
55+
return true
56+
}
57+
}
58+
return p.HasAnyUnitAccess()
4259
}
4360

4461
// HasUnits returns true if the permission contains attached units
@@ -56,16 +73,16 @@ func (p *Permission) GetFirstUnitRepoID() int64 {
5673
}
5774

5875
// UnitAccessMode returns current user access mode to the specify unit of the repository
76+
// It also considers "everyone access mode"
5977
func (p *Permission) UnitAccessMode(unitType unit.Type) perm_model.AccessMode {
60-
if p.unitsMode != nil {
61-
// if the units map contains the access mode, use it, but admin/owner mode could override it
62-
if m, ok := p.unitsMode[unitType]; ok {
63-
return util.Iif(p.AccessMode >= perm_model.AccessModeAdmin, p.AccessMode, m)
64-
}
78+
// if the units map contains the access mode, use it, but admin/owner mode could override it
79+
if m, ok := p.unitsMode[unitType]; ok {
80+
return util.Iif(p.AccessMode >= perm_model.AccessModeAdmin, p.AccessMode, m)
6581
}
6682
// if the units map does not contain the access mode, return the default access mode if the unit exists
83+
unitDefaultAccessMode := max(p.AccessMode, p.everyoneAccessMode[unitType])
6784
hasUnit := slices.ContainsFunc(p.units, func(u *repo_model.RepoUnit) bool { return u.Type == unitType })
68-
return util.Iif(hasUnit, p.AccessMode, perm_model.AccessModeNone)
85+
return util.Iif(hasUnit, unitDefaultAccessMode, perm_model.AccessModeNone)
6986
}
7087

7188
func (p *Permission) SetUnitsWithDefaultAccessMode(units []*repo_model.RepoUnit, mode perm_model.AccessMode) {
@@ -159,14 +176,15 @@ func (p *Permission) LogString() string {
159176
}
160177

161178
func applyEveryoneRepoPermission(user *user_model.User, perm *Permission) {
162-
if user != nil && user.ID > 0 {
163-
for _, u := range perm.units {
164-
if perm.unitsMode == nil {
165-
perm.unitsMode = make(map[unit.Type]perm_model.AccessMode)
166-
}
167-
if u.EveryoneAccessMode >= perm_model.AccessModeRead && u.EveryoneAccessMode > perm.unitsMode[u.Type] {
168-
perm.unitsMode[u.Type] = u.EveryoneAccessMode
179+
if user == nil || user.ID <= 0 {
180+
return
181+
}
182+
for _, u := range perm.units {
183+
if u.EveryoneAccessMode >= perm_model.AccessModeRead && u.EveryoneAccessMode > perm.everyoneAccessMode[u.Type] {
184+
if perm.everyoneAccessMode == nil {
185+
perm.everyoneAccessMode = make(map[unit.Type]perm_model.AccessMode)
169186
}
187+
perm.everyoneAccessMode[u.Type] = u.EveryoneAccessMode
170188
}
171189
}
172190
}
@@ -373,8 +391,8 @@ func CanBeAssigned(ctx context.Context, user *user_model.User, repo *repo_model.
373391
perm.CanAccessAny(perm_model.AccessModeRead, unit.TypePullRequests), nil
374392
}
375393

376-
// HasAccess returns true if user has access to repo
377-
func HasAccess(ctx context.Context, userID int64, repo *repo_model.Repository) (bool, error) {
394+
// HasAnyUnitAccess see the comment of "perm.HasAnyUnitAccess"
395+
func HasAnyUnitAccess(ctx context.Context, userID int64, repo *repo_model.Repository) (bool, error) {
378396
var user *user_model.User
379397
var err error
380398
if userID > 0 {
@@ -387,7 +405,7 @@ func HasAccess(ctx context.Context, userID int64, repo *repo_model.Repository) (
387405
if err != nil {
388406
return false, err
389407
}
390-
return perm.HasAccess(), nil
408+
return perm.HasAnyUnitAccess(), nil
391409
}
392410

393411
// getUsersWithAccessMode returns users that have at least given access mode to the repository.

‎models/perm/access/repo_permission_test.go

Lines changed: 41 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,16 +14,54 @@ import (
1414
"github.com/stretchr/testify/assert"
1515
)
1616

17+
func TestHasAnyUnitAccess(t *testing.T) {
18+
perm := Permission{}
19+
assert.False(t, perm.HasAnyUnitAccess())
20+
21+
perm = Permission{
22+
units: []*repo_model.RepoUnit{{Type: unit.TypeWiki}},
23+
}
24+
assert.False(t, perm.HasAnyUnitAccess())
25+
assert.False(t, perm.HasAnyUnitAccessOrEveryoneAccess())
26+
27+
perm = Permission{
28+
units: []*repo_model.RepoUnit{{Type: unit.TypeWiki}},
29+
everyoneAccessMode: map[unit.Type]perm_model.AccessMode{unit.TypeIssues: perm_model.AccessModeRead},
30+
}
31+
assert.False(t, perm.HasAnyUnitAccess())
32+
assert.True(t, perm.HasAnyUnitAccessOrEveryoneAccess())
33+
34+
perm = Permission{
35+
AccessMode: perm_model.AccessModeRead,
36+
units: []*repo_model.RepoUnit{{Type: unit.TypeWiki}},
37+
}
38+
assert.True(t, perm.HasAnyUnitAccess())
39+
40+
perm = Permission{
41+
unitsMode: map[unit.Type]perm_model.AccessMode{unit.TypeWiki: perm_model.AccessModeRead},
42+
}
43+
assert.True(t, perm.HasAnyUnitAccess())
44+
}
45+
1746
func TestApplyEveryoneRepoPermission(t *testing.T) {
1847
perm := Permission{
1948
AccessMode: perm_model.AccessModeNone,
2049
units: []*repo_model.RepoUnit{
21-
{Type: unit.TypeWiki, EveryoneAccessMode: perm_model.AccessModeNone},
50+
{Type: unit.TypeWiki, EveryoneAccessMode: perm_model.AccessModeRead},
2251
},
2352
}
2453
applyEveryoneRepoPermission(nil, &perm)
2554
assert.False(t, perm.CanRead(unit.TypeWiki))
2655

56+
perm = Permission{
57+
AccessMode: perm_model.AccessModeNone,
58+
units: []*repo_model.RepoUnit{
59+
{Type: unit.TypeWiki, EveryoneAccessMode: perm_model.AccessModeRead},
60+
},
61+
}
62+
applyEveryoneRepoPermission(&user_model.User{ID: 0}, &perm)
63+
assert.False(t, perm.CanRead(unit.TypeWiki))
64+
2765
perm = Permission{
2866
AccessMode: perm_model.AccessModeNone,
2967
units: []*repo_model.RepoUnit{
@@ -40,8 +78,8 @@ func TestApplyEveryoneRepoPermission(t *testing.T) {
4078
},
4179
}
4280
applyEveryoneRepoPermission(&user_model.User{ID: 1}, &perm)
43-
assert.True(t, perm.CanRead(unit.TypeWiki))
44-
assert.False(t, perm.CanWrite(unit.TypeWiki)) // because there is no unit mode, so the everyone-mode is used as the unit's access mode
81+
// it should work the same as "EveryoneAccessMode: none" because the default AccessMode should be applied to units
82+
assert.True(t, perm.CanWrite(unit.TypeWiki))
4583

4684
perm = Permission{
4785
units: []*repo_model.RepoUnit{

‎routers/api/v1/api.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ func repoAssignment() func(ctx *context.APIContext) {
218218
}
219219
}
220220

221-
if !ctx.Repo.HasAccess() {
221+
if !ctx.Repo.Permission.HasAnyUnitAccess() {
222222
ctx.NotFound()
223223
return
224224
}
@@ -412,7 +412,7 @@ func reqRepoReader(unitType unit.Type) func(ctx *context.APIContext) {
412412
// reqAnyRepoReader user should have any permission to read repository or permissions of site admin
413413
func reqAnyRepoReader() func(ctx *context.APIContext) {
414414
return func(ctx *context.APIContext) {
415-
if !ctx.Repo.HasAccess() && !ctx.IsUserSiteAdmin() {
415+
if !ctx.Repo.Permission.HasAnyUnitAccess() && !ctx.IsUserSiteAdmin() {
416416
ctx.Error(http.StatusForbidden, "reqAnyRepoReader", "user should have any permission to read repository or permissions of site admin")
417417
return
418418
}

‎routers/api/v1/repo/repo.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -585,7 +585,7 @@ func GetByID(ctx *context.APIContext) {
585585
if err != nil {
586586
ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err)
587587
return
588-
} else if !permission.HasAccess() {
588+
} else if !permission.HasAnyUnitAccess() {
589589
ctx.NotFound()
590590
return
591591
}

‎routers/web/user/package.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ func ListPackages(ctx *context.Context) {
8282
ctx.ServerError("GetUserRepoPermission", err)
8383
return
8484
}
85-
repositoryAccessMap[pd.Repository.ID] = permission.HasAccess()
85+
repositoryAccessMap[pd.Repository.ID] = permission.HasAnyUnitAccess()
8686
}
8787

8888
hasPackages, err := packages_model.HasOwnerPackages(ctx, ctx.ContextUser.ID)
@@ -276,7 +276,7 @@ func ViewPackageVersion(ctx *context.Context) {
276276
ctx.ServerError("GetUserRepoPermission", err)
277277
return
278278
}
279-
hasRepositoryAccess = permission.HasAccess()
279+
hasRepositoryAccess = permission.HasAnyUnitAccess()
280280
}
281281
ctx.Data["HasRepositoryAccess"] = hasRepositoryAccess
282282

‎services/context/repo.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -374,8 +374,7 @@ func repoAssignment(ctx *Context, repo *repo_model.Repository) {
374374
return
375375
}
376376

377-
// Check access.
378-
if !ctx.Repo.Permission.HasAccess() {
377+
if !ctx.Repo.Permission.HasAnyUnitAccessOrEveryoneAccess() {
379378
if ctx.FormString("go-get") == "1" {
380379
EarlyResponseForGoGetMeta(ctx)
381380
return

‎services/convert/package.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ func ToPackage(ctx context.Context, pd *packages.PackageDescriptor, doer *user_m
2121
return nil, err
2222
}
2323

24-
if permission.HasAccess() {
24+
if permission.HasAnyUnitAccess() {
2525
repo = ToRepo(ctx, pd.Repository, permission)
2626
}
2727
}

‎services/repository/delete.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -374,7 +374,7 @@ func removeRepositoryFromTeam(ctx context.Context, t *organization.Team, repo *r
374374
return fmt.Errorf("GetTeamMembers: %w", err)
375375
}
376376
for _, member := range teamMembers {
377-
has, err := access_model.HasAccess(ctx, member.ID, repo)
377+
has, err := access_model.HasAnyUnitAccess(ctx, member.ID, repo)
378378
if err != nil {
379379
return err
380380
} else if has {

‎services/repository/transfer.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -387,7 +387,7 @@ func StartRepositoryTransfer(ctx context.Context, doer, newOwner *user_model.Use
387387
}
388388

389389
// In case the new owner would not have sufficient access to the repo, give access rights for read
390-
hasAccess, err := access_model.HasAccess(ctx, newOwner.ID, repo)
390+
hasAccess, err := access_model.HasAnyUnitAccess(ctx, newOwner.ID, repo)
391391
if err != nil {
392392
return err
393393
}

‎services/repository/transfer_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,13 +67,13 @@ func TestStartRepositoryTransferSetPermission(t *testing.T) {
6767
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3})
6868
repo.Owner = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
6969

70-
hasAccess, err := access_model.HasAccess(db.DefaultContext, recipient.ID, repo)
70+
hasAccess, err := access_model.HasAnyUnitAccess(db.DefaultContext, recipient.ID, repo)
7171
assert.NoError(t, err)
7272
assert.False(t, hasAccess)
7373

7474
assert.NoError(t, StartRepositoryTransfer(db.DefaultContext, doer, recipient, repo, nil))
7575

76-
hasAccess, err = access_model.HasAccess(db.DefaultContext, recipient.ID, repo)
76+
hasAccess, err = access_model.HasAnyUnitAccess(db.DefaultContext, recipient.ID, repo)
7777
assert.NoError(t, err)
7878
assert.True(t, hasAccess)
7979

‎tests/integration/api_repo_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,7 @@ func TestAPISearchRepo(t *testing.T) {
222222
assert.Len(t, repoNames, expected.count)
223223
for _, repo := range body.Data {
224224
r := getRepo(t, repo.ID)
225-
hasAccess, err := access_model.HasAccess(db.DefaultContext, userID, r)
225+
hasAccess, err := access_model.HasAnyUnitAccess(db.DefaultContext, userID, r)
226226
assert.NoError(t, err, "Error when checking if User: %d has access to %s: %v", userID, repo.FullName, err)
227227
assert.True(t, hasAccess, "User: %d does not have access to %s", userID, repo.FullName)
228228

0 commit comments

Comments
 (0)