From 48d4580dd5e975de2e8207bb9b9a2f258711d38c Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 20 Apr 2024 11:15:04 +0800 Subject: [PATCH] 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 --- models/org_team.go | 4 +- models/perm/access/access_test.go | 8 ++-- models/perm/access/repo_permission.go | 56 ++++++++++++++-------- models/perm/access/repo_permission_test.go | 44 +++++++++++++++-- routers/api/v1/api.go | 4 +- routers/api/v1/repo/repo.go | 2 +- routers/web/user/package.go | 4 +- services/context/repo.go | 3 +- services/convert/package.go | 2 +- services/repository/delete.go | 2 +- services/repository/transfer.go | 2 +- services/repository/transfer_test.go | 4 +- tests/integration/api_repo_test.go | 2 +- 13 files changed, 96 insertions(+), 41 deletions(-) diff --git a/models/org_team.go b/models/org_team.go index aecf0d80fd..b6908478c7 100644 --- a/models/org_team.go +++ b/models/org_team.go @@ -118,7 +118,7 @@ func removeAllRepositories(ctx context.Context, t *organization.Team) (err error // Remove watches from all users and now unaccessible repos for _, user := range t.Members { - has, err := access_model.HasAccess(ctx, user.ID, repo) + has, err := access_model.HasAnyUnitAccess(ctx, user.ID, repo) if err != nil { return err } else if has { @@ -544,7 +544,7 @@ func ReconsiderRepoIssuesAssignee(ctx context.Context, repo *repo_model.Reposito } func ReconsiderWatches(ctx context.Context, repo *repo_model.Repository, user *user_model.User) error { - if has, err := access_model.HasAccess(ctx, user.ID, repo); err != nil || has { + if has, err := access_model.HasAnyUnitAccess(ctx, user.ID, repo); err != nil || has { return err } if err := repo_model.WatchRepo(ctx, user, repo, false); err != nil { diff --git a/models/perm/access/access_test.go b/models/perm/access/access_test.go index 79b131fe89..51d625707c 100644 --- a/models/perm/access/access_test.go +++ b/models/perm/access/access_test.go @@ -79,17 +79,17 @@ func TestHasAccess(t *testing.T) { repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) assert.True(t, repo2.IsPrivate) - has, err := access_model.HasAccess(db.DefaultContext, user1.ID, repo1) + has, err := access_model.HasAnyUnitAccess(db.DefaultContext, user1.ID, repo1) assert.NoError(t, err) assert.True(t, has) - _, err = access_model.HasAccess(db.DefaultContext, user1.ID, repo2) + _, err = access_model.HasAnyUnitAccess(db.DefaultContext, user1.ID, repo2) assert.NoError(t, err) - _, err = access_model.HasAccess(db.DefaultContext, user2.ID, repo1) + _, err = access_model.HasAnyUnitAccess(db.DefaultContext, user2.ID, repo1) assert.NoError(t, err) - _, err = access_model.HasAccess(db.DefaultContext, user2.ID, repo2) + _, err = access_model.HasAnyUnitAccess(db.DefaultContext, user2.ID, repo2) assert.NoError(t, err) } diff --git a/models/perm/access/repo_permission.go b/models/perm/access/repo_permission.go index 9cce95b776..0ed116a132 100644 --- a/models/perm/access/repo_permission.go +++ b/models/perm/access/repo_permission.go @@ -24,6 +24,8 @@ type Permission struct { units []*repo_model.RepoUnit unitsMode map[unit.Type]perm_model.AccessMode + + everyoneAccessMode map[unit.Type]perm_model.AccessMode } // IsOwner returns true if current user is the owner of repository. @@ -36,9 +38,24 @@ func (p *Permission) IsAdmin() bool { return p.AccessMode >= perm_model.AccessModeAdmin } -// HasAccess returns true if the current user might have at least read access to any unit of this repository -func (p *Permission) HasAccess() bool { - return len(p.unitsMode) > 0 || p.AccessMode >= perm_model.AccessModeRead +// HasAnyUnitAccess returns true if the user might have at least one access mode to any unit of this repository. +// It doesn't count the "everyone access mode". +func (p *Permission) HasAnyUnitAccess() bool { + for _, v := range p.unitsMode { + if v >= perm_model.AccessModeRead { + return true + } + } + return p.AccessMode >= perm_model.AccessModeRead +} + +func (p *Permission) HasAnyUnitAccessOrEveryoneAccess() bool { + for _, v := range p.everyoneAccessMode { + if v >= perm_model.AccessModeRead { + return true + } + } + return p.HasAnyUnitAccess() } // HasUnits returns true if the permission contains attached units @@ -56,16 +73,16 @@ func (p *Permission) GetFirstUnitRepoID() int64 { } // UnitAccessMode returns current user access mode to the specify unit of the repository +// It also considers "everyone access mode" func (p *Permission) UnitAccessMode(unitType unit.Type) perm_model.AccessMode { - if p.unitsMode != nil { - // if the units map contains the access mode, use it, but admin/owner mode could override it - if m, ok := p.unitsMode[unitType]; ok { - return util.Iif(p.AccessMode >= perm_model.AccessModeAdmin, p.AccessMode, m) - } + // if the units map contains the access mode, use it, but admin/owner mode could override it + if m, ok := p.unitsMode[unitType]; ok { + return util.Iif(p.AccessMode >= perm_model.AccessModeAdmin, p.AccessMode, m) } // if the units map does not contain the access mode, return the default access mode if the unit exists + unitDefaultAccessMode := max(p.AccessMode, p.everyoneAccessMode[unitType]) hasUnit := slices.ContainsFunc(p.units, func(u *repo_model.RepoUnit) bool { return u.Type == unitType }) - return util.Iif(hasUnit, p.AccessMode, perm_model.AccessModeNone) + return util.Iif(hasUnit, unitDefaultAccessMode, perm_model.AccessModeNone) } func (p *Permission) SetUnitsWithDefaultAccessMode(units []*repo_model.RepoUnit, mode perm_model.AccessMode) { @@ -159,14 +176,15 @@ func (p *Permission) LogString() string { } func applyEveryoneRepoPermission(user *user_model.User, perm *Permission) { - if user != nil && user.ID > 0 { - for _, u := range perm.units { - if perm.unitsMode == nil { - perm.unitsMode = make(map[unit.Type]perm_model.AccessMode) - } - if u.EveryoneAccessMode >= perm_model.AccessModeRead && u.EveryoneAccessMode > perm.unitsMode[u.Type] { - perm.unitsMode[u.Type] = u.EveryoneAccessMode + if user == nil || user.ID <= 0 { + return + } + for _, u := range perm.units { + if u.EveryoneAccessMode >= perm_model.AccessModeRead && u.EveryoneAccessMode > perm.everyoneAccessMode[u.Type] { + if perm.everyoneAccessMode == nil { + perm.everyoneAccessMode = make(map[unit.Type]perm_model.AccessMode) } + perm.everyoneAccessMode[u.Type] = u.EveryoneAccessMode } } } @@ -373,8 +391,8 @@ func CanBeAssigned(ctx context.Context, user *user_model.User, repo *repo_model. perm.CanAccessAny(perm_model.AccessModeRead, unit.TypePullRequests), nil } -// HasAccess returns true if user has access to repo -func HasAccess(ctx context.Context, userID int64, repo *repo_model.Repository) (bool, error) { +// HasAnyUnitAccess see the comment of "perm.HasAnyUnitAccess" +func HasAnyUnitAccess(ctx context.Context, userID int64, repo *repo_model.Repository) (bool, error) { var user *user_model.User var err error if userID > 0 { @@ -387,7 +405,7 @@ func HasAccess(ctx context.Context, userID int64, repo *repo_model.Repository) ( if err != nil { return false, err } - return perm.HasAccess(), nil + return perm.HasAnyUnitAccess(), nil } // getUsersWithAccessMode returns users that have at least given access mode to the repository. diff --git a/models/perm/access/repo_permission_test.go b/models/perm/access/repo_permission_test.go index aaa53bb24f..50070c4368 100644 --- a/models/perm/access/repo_permission_test.go +++ b/models/perm/access/repo_permission_test.go @@ -14,16 +14,54 @@ import ( "github.com/stretchr/testify/assert" ) +func TestHasAnyUnitAccess(t *testing.T) { + perm := Permission{} + assert.False(t, perm.HasAnyUnitAccess()) + + perm = Permission{ + units: []*repo_model.RepoUnit{{Type: unit.TypeWiki}}, + } + assert.False(t, perm.HasAnyUnitAccess()) + assert.False(t, perm.HasAnyUnitAccessOrEveryoneAccess()) + + perm = Permission{ + units: []*repo_model.RepoUnit{{Type: unit.TypeWiki}}, + everyoneAccessMode: map[unit.Type]perm_model.AccessMode{unit.TypeIssues: perm_model.AccessModeRead}, + } + assert.False(t, perm.HasAnyUnitAccess()) + assert.True(t, perm.HasAnyUnitAccessOrEveryoneAccess()) + + perm = Permission{ + AccessMode: perm_model.AccessModeRead, + units: []*repo_model.RepoUnit{{Type: unit.TypeWiki}}, + } + assert.True(t, perm.HasAnyUnitAccess()) + + perm = Permission{ + unitsMode: map[unit.Type]perm_model.AccessMode{unit.TypeWiki: perm_model.AccessModeRead}, + } + assert.True(t, perm.HasAnyUnitAccess()) +} + func TestApplyEveryoneRepoPermission(t *testing.T) { perm := Permission{ AccessMode: perm_model.AccessModeNone, units: []*repo_model.RepoUnit{ - {Type: unit.TypeWiki, EveryoneAccessMode: perm_model.AccessModeNone}, + {Type: unit.TypeWiki, EveryoneAccessMode: perm_model.AccessModeRead}, }, } applyEveryoneRepoPermission(nil, &perm) assert.False(t, perm.CanRead(unit.TypeWiki)) + perm = Permission{ + AccessMode: perm_model.AccessModeNone, + units: []*repo_model.RepoUnit{ + {Type: unit.TypeWiki, EveryoneAccessMode: perm_model.AccessModeRead}, + }, + } + applyEveryoneRepoPermission(&user_model.User{ID: 0}, &perm) + assert.False(t, perm.CanRead(unit.TypeWiki)) + perm = Permission{ AccessMode: perm_model.AccessModeNone, units: []*repo_model.RepoUnit{ @@ -40,8 +78,8 @@ func TestApplyEveryoneRepoPermission(t *testing.T) { }, } applyEveryoneRepoPermission(&user_model.User{ID: 1}, &perm) - assert.True(t, perm.CanRead(unit.TypeWiki)) - 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 + // it should work the same as "EveryoneAccessMode: none" because the default AccessMode should be applied to units + assert.True(t, perm.CanWrite(unit.TypeWiki)) perm = Permission{ units: []*repo_model.RepoUnit{ diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index f60c5f21db..5358906f27 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -218,7 +218,7 @@ func repoAssignment() func(ctx *context.APIContext) { } } - if !ctx.Repo.HasAccess() { + if !ctx.Repo.Permission.HasAnyUnitAccess() { ctx.NotFound() return } @@ -412,7 +412,7 @@ func reqRepoReader(unitType unit.Type) func(ctx *context.APIContext) { // reqAnyRepoReader user should have any permission to read repository or permissions of site admin func reqAnyRepoReader() func(ctx *context.APIContext) { return func(ctx *context.APIContext) { - if !ctx.Repo.HasAccess() && !ctx.IsUserSiteAdmin() { + if !ctx.Repo.Permission.HasAnyUnitAccess() && !ctx.IsUserSiteAdmin() { ctx.Error(http.StatusForbidden, "reqAnyRepoReader", "user should have any permission to read repository or permissions of site admin") return } diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go index 822e368fa8..2ac0b7ebd1 100644 --- a/routers/api/v1/repo/repo.go +++ b/routers/api/v1/repo/repo.go @@ -585,7 +585,7 @@ func GetByID(ctx *context.APIContext) { if err != nil { ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err) return - } else if !permission.HasAccess() { + } else if !permission.HasAnyUnitAccess() { ctx.NotFound() return } diff --git a/routers/web/user/package.go b/routers/web/user/package.go index 9af49406c4..2a18796687 100644 --- a/routers/web/user/package.go +++ b/routers/web/user/package.go @@ -82,7 +82,7 @@ func ListPackages(ctx *context.Context) { ctx.ServerError("GetUserRepoPermission", err) return } - repositoryAccessMap[pd.Repository.ID] = permission.HasAccess() + repositoryAccessMap[pd.Repository.ID] = permission.HasAnyUnitAccess() } hasPackages, err := packages_model.HasOwnerPackages(ctx, ctx.ContextUser.ID) @@ -276,7 +276,7 @@ func ViewPackageVersion(ctx *context.Context) { ctx.ServerError("GetUserRepoPermission", err) return } - hasRepositoryAccess = permission.HasAccess() + hasRepositoryAccess = permission.HasAnyUnitAccess() } ctx.Data["HasRepositoryAccess"] = hasRepositoryAccess diff --git a/services/context/repo.go b/services/context/repo.go index 1f4c698afc..b17f99eb17 100644 --- a/services/context/repo.go +++ b/services/context/repo.go @@ -374,8 +374,7 @@ func repoAssignment(ctx *Context, repo *repo_model.Repository) { return } - // Check access. - if !ctx.Repo.Permission.HasAccess() { + if !ctx.Repo.Permission.HasAnyUnitAccessOrEveryoneAccess() { if ctx.FormString("go-get") == "1" { EarlyResponseForGoGetMeta(ctx) return diff --git a/services/convert/package.go b/services/convert/package.go index b5fca21a3c..b27992bea9 100644 --- a/services/convert/package.go +++ b/services/convert/package.go @@ -21,7 +21,7 @@ func ToPackage(ctx context.Context, pd *packages.PackageDescriptor, doer *user_m return nil, err } - if permission.HasAccess() { + if permission.HasAnyUnitAccess() { repo = ToRepo(ctx, pd.Repository, permission) } } diff --git a/services/repository/delete.go b/services/repository/delete.go index 7c7dfe2ddd..cd779b05c3 100644 --- a/services/repository/delete.go +++ b/services/repository/delete.go @@ -374,7 +374,7 @@ func removeRepositoryFromTeam(ctx context.Context, t *organization.Team, repo *r return fmt.Errorf("GetTeamMembers: %w", err) } for _, member := range teamMembers { - has, err := access_model.HasAccess(ctx, member.ID, repo) + has, err := access_model.HasAnyUnitAccess(ctx, member.ID, repo) if err != nil { return err } else if has { diff --git a/services/repository/transfer.go b/services/repository/transfer.go index 83d3032188..3d0bce18d0 100644 --- a/services/repository/transfer.go +++ b/services/repository/transfer.go @@ -387,7 +387,7 @@ func StartRepositoryTransfer(ctx context.Context, doer, newOwner *user_model.Use } // In case the new owner would not have sufficient access to the repo, give access rights for read - hasAccess, err := access_model.HasAccess(ctx, newOwner.ID, repo) + hasAccess, err := access_model.HasAnyUnitAccess(ctx, newOwner.ID, repo) if err != nil { return err } diff --git a/services/repository/transfer_test.go b/services/repository/transfer_test.go index c3f03d6638..67799eddcc 100644 --- a/services/repository/transfer_test.go +++ b/services/repository/transfer_test.go @@ -67,13 +67,13 @@ func TestStartRepositoryTransferSetPermission(t *testing.T) { repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) repo.Owner = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) - hasAccess, err := access_model.HasAccess(db.DefaultContext, recipient.ID, repo) + hasAccess, err := access_model.HasAnyUnitAccess(db.DefaultContext, recipient.ID, repo) assert.NoError(t, err) assert.False(t, hasAccess) assert.NoError(t, StartRepositoryTransfer(db.DefaultContext, doer, recipient, repo, nil)) - hasAccess, err = access_model.HasAccess(db.DefaultContext, recipient.ID, repo) + hasAccess, err = access_model.HasAnyUnitAccess(db.DefaultContext, recipient.ID, repo) assert.NoError(t, err) assert.True(t, hasAccess) diff --git a/tests/integration/api_repo_test.go b/tests/integration/api_repo_test.go index 481732f8df..bc2720d51e 100644 --- a/tests/integration/api_repo_test.go +++ b/tests/integration/api_repo_test.go @@ -222,7 +222,7 @@ func TestAPISearchRepo(t *testing.T) { assert.Len(t, repoNames, expected.count) for _, repo := range body.Data { r := getRepo(t, repo.ID) - hasAccess, err := access_model.HasAccess(db.DefaultContext, userID, r) + hasAccess, err := access_model.HasAnyUnitAccess(db.DefaultContext, userID, r) assert.NoError(t, err, "Error when checking if User: %d has access to %s: %v", userID, repo.FullName, err) assert.True(t, hasAccess, "User: %d does not have access to %s", userID, repo.FullName)